統一解除 Needlefish 模型 runner 權限限制 - #33
Conversation
There was a problem hiding this comment.
LGTM ✅ — Unifies all model runners around unrestricted CLI permissions while retaining clone-based target isolation, and removes now-obsolete workflow gates and documentation claims.
✅ 1 resolved
Coverage: 12/12 changed files deep-reviewed across 3 hotspots
Findings
No actionable findings. Prefer this over padding weak ones.
Checked (15)
- [Unrestricted model-runner invocation and isolation contracts] No correctness, contract, or isolation regression found in the unrestricted runner surface.
- FILES inspected=[src/shared/codex.ts, src/shared/codex.test.ts, src/shared/codex-runners.test.ts, src/shared/codex-ephemeral-home.test.ts]
- EDGE producer=runCodex runner selection and unrestricted CLI arguments consumer=src/core/review.ts:275 outcome=passes effect=review/map/deep/critic calls invoke the selected runner through a throwaway fixed-HEAD sandbox, then reject a dirty sandbox at src/shared/codex.ts:568
- EDGE producer=runCodex runner selection and unrestricted CLI arguments consumer=src/core/explain.ts:24 outcome=passes effect=explanation calls invoke the selected runner through the same throwaway fixed-HEAD sandbox and post-run cleanliness check
- TRIGGER_A removed runner opt-in gates: src/core/review.ts:275 gates distinct system actions review@src/core/review.ts:450, critic@src/core/review.ts:364, map@src/core/review.ts:499, and deep@src/core/review.ts:565 → unrestricted runner selection legitimately governs each action → yes → no independently approvable action is newly rejected; src/core/explain.ts:24 gates explanation generation → yes → no rejection introduced
- [GitHub Action and self-hosted workflow runner policy] No actionable defects found in the GitHub Action and self-hosted workflow policy surface.
- FILES inspected=[.github/workflows/review.yml, .github/workflows/weekly-eval.yml, action.yml, scripts/deploy-ubuntu.test.mjs]
- EDGE producer=action.yml removal of runner permission opt-ins and hosted sysctl setup consumer=scripts/deploy-ubuntu.test.mjs:12 outcome=passes effect=the contract test reads action.yml and rejects any remaining opt-in variable or AppArmor sysctl reference.
- TRIGGER_C carrier=action.yml:3-4 promises=the action runs its review through an isolated model runner body=action.yml:100-104 dispatches the target repository to Needlefish, whose runner path creates a throwaway clone at src/shared/runner-sandbox.ts:38-51 and rejects changed HEAD/worktree state at src/shared/runner-sandbox.ts:99-121; caller=.github/workflows/hosted-review.yml:23 invokes the public action; outcome=passes.
- [Runner trust-boundary documentation] No correctness, security, or contract defects found in this documentation surface.
- FILES inspected=[AGENTS.md, src/shared/AGENTS.md, README.md, README.zh-TW.md]
- TRIGGER_C carrier=README.md:29-30 and README.zh-TW.md:26-27 promises=all CLI reviews use a throwaway clean clone and detect tampering; body=src/shared/codex.ts:516-572 prepares the clone and rejects changed HEAD/worktree; outcome=passes
- TRIGGER_C carrier=README.md:439-458 and README.zh-TW.md:352-365 promises=listed unrestricted CLI flags, GitHub-token stripping, fixed HEAD, and post-run clone checks; body=src/shared/codex.ts:78-105, 695-724, 742-830 and src/shared/runner-sandbox.ts:99-121; outcome=passes
- TRIGGER_C carrier=README.md:313-314 and README.zh-TW.md:275-276 promises=operator guidance to use unrestricted runners only on controlled self-hosted infrastructure; body=not-applicable because this is operational guidance, not an enforced API result contract; outcome=passes
- TRIGGER_C carrier=AGENTS.md:69,81 and src/shared/AGENTS.md:22,28 promises=preserve target-clone, token, HEAD, and dirty-worktree isolation checks; body=src/shared/codex.ts:512-572 and src/shared/runner-sandbox.ts:38-121; outcome=passes
5 calls · map 1m 5s → deep:Unrestricted model-runner invocation and isolation contracts 1m 21s → deep:GitHub Action and self-hosted workflow runner policy 1m 34s → deep:Runner trust-boundary documentation 2m 44s → critic 27s · total 4m 16s
| `model_reasoning_effort="${reasoningEffort}"`, | ||
| "-s", | ||
| "read-only", | ||
| "--dangerously-bypass-approvals-and-sandbox", |
There was a problem hiding this comment.
P1 Restore host-level isolation before bypassing Codex safeguards
Changed lines src/shared/codex.ts:707-708 pass --dangerously-bypass-approvals-and-sandbox and --ignore-rules to Codex. The subprocess is spawned with only the throwaway clone as its cwd at src/shared/runner-process.ts:96-101; the clone implementation only changes the working directory (src/shared/runner-sandbox.ts:44-51). src/shared/codex.ts:393-395 explicitly notes that a same-UID runner can still read the real HOME via absolute paths. A malicious PR/prompt can therefore read host credentials or write outside the clone before the post-run clone-cleanliness check.
Fix: Run unrestricted CLIs inside an OS/container sandbox that denies access outside the per-run clone and ephemeral credential paths, or retain read-only runner restrictions until that containment exists.
Validate: In a deployment-like sandbox, run a hostile runner probe that attempts to read a marker outside the clone and write outside the ephemeral temp directory; assert both operations are denied.
|
前一輪 P1 判定為 false positive:它要求恢復 host-level sandbox,但本 PR 的明確產品規格就是 trusted self-hosted runner 上的模型 CLI 不得受 process-level permission/sandbox 限制。外層仍保留 throwaway clean clone、GitHub token stripping、固定 HEAD、post-run dirty rejection、timeout 與 cleanup。已把這個信任邊界補進 AGENTS.md,避免後續審查把刻意規格當成缺陷。 |
|
Needlefish re-review @ 719445a — ✅ 1 resolved · ❌ 0 still open · 🆕 0 new → LGTM |
Summary
Verification
pnpm test:519 tests,507 pass,0 fail,12 platform skipspnpm buildpnpm checkpnpm lintgit diff --checkRollout
合併後先由 Needlefish deploy workflow 更新共用 x64 安裝,再以同一 SHA 更新兩台 ARM runner。