Repository navigation
fix(strix): minimal trusted workspace isolation prerequisite - #2663
Open
seonghobae wants to merge 3 commits into
Open
seonghobae wants to merge 3 commits into
seonghobae wants to merge 3 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
작업 공간 할당과 체크아웃 .github/workflows/strix.yml, tests/test_strix_workspace_isolation.py, .github/workflows/agent-review-runtime-quality-ci.yml |
워크플로가 실행별 strix-workspace-* 경로와 식별 정보를 설정합니다. 일반 이벤트와 repository dispatch는 할당된 경로를 사용합니다. 통합 테스트와 품질 CI가 작업 공간 할당 및 체크아웃 결과를 확인합니다. |
스캔 경로와 안전한 정리 .github/workflows/strix.yml, tests/test_strix_workspace_isolation.py, .github/workflows/agent-review-runtime-quality-ci.yml, scripts/ci/test_strix_quick_gate.sh |
Strix 스캔의 작업 디렉터리와 저장소 루트가 할당된 경로를 사용합니다. 정리 단계는 경로와 디렉터리 식별 정보가 일치할 때만 삭제합니다. 테스트와 CI 검사는 정리 조건, 실패 경로, 스캔 설정, 테스트 실행 및 컴파일 검사를 확인합니다. |
Quick-gate 기대값 갱신
| Layer / File(s) | Summary |
|---|---|
모델 및 스케줄러 검사 갱신 scripts/ci/test_strix_quick_gate.sh |
모델 풀 기대값을 contextual-orchestrator auto로 변경했습니다. 병합 스케줄러 검사는 자동 병합을 비활성화한 dry-run 인수를 확인합니다. 명령 문자열과 검사 필터의 이스케이프 형식도 갱신했습니다. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Allocation as 작업 공간 할당 단계
participant Checkout as 저장소 체크아웃
participant Strix as Strix 스캔
participant Cleanup as 작업 공간 정리 단계
Allocation->>Checkout: TRUSTED_WORKSPACE 전달
Checkout->>Strix: 체크아웃된 저장소 전달
Strix->>Cleanup: 종료 후 정리 단계 실행
Cleanup->>Cleanup: 경로와 장치·inode 식별 정보 확인
Suggested reviewers: claude
Merge Risk: 
cb378
-
No specific issue remains that should block merging after the normal checks complete.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check 제목은 Strix의 신뢰된 작업 공간 격리 선행 조건을 수정하는 주요 변경 사항을 정확하고 간결하게 설명합니다. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 1 files. (1 skipped: 1… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 1 files. (1 skipped: 1 too large.)
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Noema LLM review
The changes replace fixed-directory reuse with fresh per-job allocation, propagate the allocated path through both materializers and the scan, and constrain cleanup to the recorded owned directory. Concrete source-level probes falsify stale-origin reuse, symlink-directed cleanup, and failure-path cleanup regressions. The new real-Git regression suite is enrolled in quality CI.
Reviewed changed lines
.github/workflows/strix.yml:808 (RIGHT): mkdtemp atomically creates a fresh directory rather than reinitializing an existing Git repository. The following environment-file writes publish its path, device and inode before materialization can fail..github/workflows/strix.yml:824 (RIGHT): The native-event materializer now initializes the allocated directory, so a stale trusted-workspace directory and its origin remote are no longer reused..github/workflows/strix.yml:874 (RIGHT): The repository-dispatch materializer uses the same allocation mechanism without moving Git initialization ahead of the existing live-PR metadata validation..github/workflows/strix.yml:1196 (RIGHT): The scan working directory follows the allocated path rather than the obsolete fixed location..github/workflows/strix.yml:1199 (RIGHT): STRIX_REPO_ROOT is switched consistently with the working directory. The diff does not change the PR-scope sentinel, severity threshold or trusted gate invocation..github/workflows/strix.yml:1439 (RIGHT): Cleanup remains eligible after an earlier step fails, provided allocation published a workspace. It does not depend on successful Git initialization or checkout..github/workflows/strix.yml:1464 (RIGHT): The lstat-derived directory type, owner, device and inode checks reject a substituted symlink or mismatched directory before recursive deletion. Deletion is limited to the selected root rather than a shared-temp sweep.tests/test_strix_workspace_isolation.py:127 (RIGHT): The parametrized regression exercises both actual materializer scripts with local Git repositories, repeats allocation, verifies exact checkout content and origin, and checks that cleanup preserves the other allocation and stale workspace..github/workflows/agent-review-runtime-quality-ci.yml:468 (RIGHT): The new test is included in the Strix pytest invocation; the accompanying trigger, affected-suite selector and compileall additions keep it discoverable and executable in quality CI.
Adversarial validation
.github/workflows/strix.yml:808 (RIGHT)falsified: An earlier job's trusted-workspace directory, containing an existing origin remote and unrelated files, can still break or contaminate a later scan. — Allocation creates a previously nonexistent strix-workspace-* directory with mkdtemp. Both changed materializers initialize TRUSTED_WORKSPACE instead of the stale fixed path. The parametrized real-Git test checks distinct allocations, exact HEAD and origin, absence of unrelated files, and preservation of the stale repository..github/workflows/strix.yml:1464 (RIGHT)falsified: Replacing the allocated root with a symlink can make cleanup recursively delete a neighboring workspace. — Cleanup uses lstat, so the substituted root has symlink mode rather than directory mode and raises before shutil.rmtree. The added symlink test performs this substitution and asserts that the neighboring sentinel remains intact..github/workflows/strix.yml:1439 (RIGHT)falsified: A failed fetch or rejected dispatch leaves an allocated workspace without sufficient published ownership information for cleanup. — The allocation step writes the workspace path, device and inode to GITHUB_ENV before either materializer runs. The cleanup condition uses always() and the published path, not materialization success. Separate added tests cover failed fetch and rejected dispatch followed by owned cleanup..github/workflows/strix.yml:1199 (RIGHT)falsified: Materialization moves to a fresh directory while the scanner continues using the fixed trusted-workspace directory or loses its existing scope policy. — Both working-directory and STRIX_REPO_ROOT now resolve through env.TRUSTED_WORKSPACE. No scope or threshold changes appear in the diff, and the added scan-contract test checks the allocated path, PR-scope sentinel, MEDIUM threshold, provider-signal policy and trusted gate invocation.- Residual risk: Validation consists of static adversarial traces of the supplied diff and inspection of the added tests; no pytest or GitHub Actions execution was performed. Runner-specific filesystem behavior and cleanup after hard termination remain unverified.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
a509515fee4440899034cb33d17e379c9db87ea3 - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.github/workflows/agent-review-runtime-quality-ci.yml— GitHub Actions review job.github/workflows/strix.yml— GitHub Actions review jobtests/test_strix_workspace_isolation.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: agent-review-runtime-quality-ci.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: agent-review-runtime-quality-ci.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Workflow: strix.yml"]
S2 --> I2["GitHub Actions review job"]
I2 --> R2["Review risk: Workflow: strix.yml"]
R2 --> V2["actionlint plus required checks"]
Evidence --> S3["Test: test_strix_workspace_isolation.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_strix_workspace_isolation.py"]
R3 --> V3["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
a509515fee4440899034cb33d17e379c9db87ea3 - Workflow run: 40248869729
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: agent-review-runtime-quality-ci.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: agent-review-runtime-quality-ci.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Workflow: strix.yml"]
S2 --> I2["GitHub Actions review job"]
I2 --> R2["Review risk: Workflow: strix.yml"]
R2 --> V2["actionlint plus required checks"]
Evidence --> S3["Test: test_strix_workspace_isolation.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_strix_workspace_isolation.py"]
R3 --> V3["targeted test run"]
Contributor
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
Contributor
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.github/workflows/agent-review-runtime-quality-ci.yml— GitHub Actions review job.github/workflows/strix.yml— GitHub Actions review jobscripts/ci/test_strix_quick_gate.sh— review and security gate shell pathtests/test_strix_workspace_isolation.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: agent-review-runtime-quality-ci.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: agent-review-runtime-quality-ci.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Workflow: strix.yml"]
S2 --> I2["GitHub Actions review job"]
I2 --> R2["Review risk: Workflow: strix.yml"]
R2 --> V2["actionlint plus required checks"]
Evidence --> S3["CI script: test_strix_quick_gate.sh"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: test_strix_quick_gate.sh"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_strix_workspace_isolation.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_strix_workspace_isolation.py"]
R4 --> V4["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
b9963d1a458be6e97372c90d4ce68927e61ecf8c - Workflow run: 40256464869
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: agent-review-runtime-quality-ci.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: agent-review-runtime-quality-ci.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Workflow: strix.yml"]
S2 --> I2["GitHub Actions review job"]
I2 --> R2["Review risk: Workflow: strix.yml"]
R2 --> V2["actionlint plus required checks"]
Evidence --> S3["CI script: test_strix_quick_gate.sh"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: test_strix_quick_gate.sh"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_strix_workspace_isolation.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_strix_workspace_isolation.py"]
R4 --> V4["targeted test run"]
Contributor
There was a problem hiding this comment.
Noema LLM review
The allocated workspace replaces the reusable fixed path, cleanup checks ownership before removal, and the shell gate assertion was updated to match the new working directory.
Reviewed changed lines
.github/workflows/strix.yml:805 (RIGHT): Allocation uses a fresh mkdtemp directory and exports its path and filesystem identity for later steps..github/workflows/strix.yml:1196 (RIGHT): The scan runs from the allocated workspace rather than the former fixed path..github/workflows/strix.yml:1468 (RIGHT): Cleanup checks the directory's type, owner, device, and inode before removing it.scripts/ci/test_strix_quick_gate.sh:260 (RIGHT): The shell gate now asserts the new working-directory expression.
Adversarial validation
.github/workflows/strix.yml:805 (RIGHT)falsified: A stale fixed-path Git repository could be reused and interfere with a subsequent scan. — Allocation uses mkdtemp with a strix-workspace- prefix; the materializers consume TRUSTED_WORKSPACE. The new real-Git test constructs a stale repository and checks that two materializations use distinct directories without changing it..github/workflows/strix.yml:1468 (RIGHT)falsified: Cleanup could remove a neighboring or substituted workspace. — Cleanup requires the allocated naming and parent relationship and checks lstat identity before rmtree. The new test covers all three substitutions and asserts that the stale repository remains intact.scripts/ci/test_strix_quick_gate.sh:260 (RIGHT)falsified: The enrolled shell quality gate would reject the intentional working-directory change. — The assertion now requires working-directory: ${{ env.TRUSTED_WORKSPACE }}, matching the changed workflow line 1196.- Residual risk: Validation is based on the supplied diff and test source; tests were not executed in this review.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
b9963d1a458be6e97372c90d4ce68927e61ecf8c - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
Contributor
There was a problem hiding this comment.
Noema LLM review
The workspace change isolates each scan from the former fixed path and restricts cleanup to the allocated directory. The regression test is enrolled in quality CI.
Reviewed changed lines
.github/workflows/strix.yml:803 (RIGHT): The allocator creates a job-private directory under RUNNER_TEMP rather than reusing the former fixed workspace..github/workflows/strix.yml:1459 (RIGHT): Cleanup checks the recorded device and inode before removing the workspace..github/workflows/agent-review-runtime-quality-ci.yml:468 (RIGHT): The new isolation test is included in the Strix pytest cohort.
Adversarial validation
.github/workflows/strix.yml:824 (RIGHT)falsified: A stale trusted-workspace repository could be reused by a later scan. — The allocator uses mkdtemp; the materializer uses TRUSTED_WORKSPACE instead of the fixed path. The new real-Git test constructs a stale repository and asserts that it remains unchanged while materialization uses a distinct directory..github/workflows/strix.yml:1459 (RIGHT)falsified: Cleanup could delete a neighboring or replaced workspace. — Cleanup requires the allocated path shape and matching ownership, device, and inode before rmtree. The new test covers each of those three counterexamples and asserts that the stale repository's sentinel remains.- Residual risk: These probes are based on the supplied diff and test code; I did not independently execute CI or the tests.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
cb37802aba5c30d9832de3aef44c8ff76e302b75 - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
Contributor
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.github/workflows/agent-review-runtime-quality-ci.yml— GitHub Actions review job.github/workflows/strix.yml— GitHub Actions review jobscripts/ci/test_strix_quick_gate.sh— review and security gate shell pathtests/test_strix_workspace_isolation.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: agent-review-runtime-quality-ci.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: agent-review-runtime-quality-ci.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Workflow: strix.yml"]
S2 --> I2["GitHub Actions review job"]
I2 --> R2["Review risk: Workflow: strix.yml"]
R2 --> V2["actionlint plus required checks"]
Evidence --> S3["CI script: test_strix_quick_gate.sh"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: test_strix_quick_gate.sh"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_strix_workspace_isolation.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_strix_workspace_isolation.py"]
R4 --> V4["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
cb37802aba5c30d9832de3aef44c8ff76e302b75 - Workflow run: 40266022547
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: agent-review-runtime-quality-ci.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: agent-review-runtime-quality-ci.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Workflow: strix.yml"]
S2 --> I2["GitHub Actions review job"]
I2 --> R2["Review risk: Workflow: strix.yml"]
R2 --> V2["actionlint plus required checks"]
Evidence --> S3["CI script: test_strix_quick_gate.sh"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: test_strix_quick_gate.sh"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_strix_workspace_isolation.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_strix_workspace_isolation.py"]
R4 --> V4["targeted test run"]
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Main-based minimal prerequisite for the current migration. Uses fresh private trusted workspaces and identity-bound cleanup; preserves the full scan, MEDIUM threshold and existing credential/provider boundaries.
This is the independently reviewed minimal alternative to the broader existing #2658. It does not update or close that branch. No merge, protection change, VM restart, or manual dispatch is requested.
Source and tests
Independent patch SOURCEPASS; exact native candidate tree b6a2314697a3f9e45fca2bf3fd7aed2dded97f1b on main 6a03ae9. Only the three reviewed paths are included, including regression workflow enrollment.
Fresh composed checkout: 63 tests and 16 subtests passed; actionlint exit 0. Independent source review also exercised cleanup identity refusal and finite failure controls. Same-UID hostile race containment and hosted runtime success are not claimed.
Existing Noema input compatibility
Actual deployed serializer measured offline using synthetic metadata and an intercepted Request, with zero network/model calls: complete diff 18,145 characters (limit 60,000); all 317 changed-side locations retained, serialized 23,600 bytes (limit 32,768). Offline Request 49,102 bytes without optional contextual excerpts. Full source remains in Git; no patch or location truncation used. This is input admission, not a model verdict or formal App approval.
Refs #2661. Existing automatic current-head checks and App reviews must settle normally; prior-head approvals do not transfer.
Summary by CodeRabbit