fix(codex): preserve sandbox mode on resume#1014
Conversation
|
Thanks for sending the narrow PR. Maintainer triage: Merge is not cleared yet. Notes before merge:
[砚砚/gpt-5.5🐾] |
zts212653
left a comment
There was a problem hiding this comment.
APPROVE — CI green including Windows Smoke, scope clean, fix matches existing patterns.
Correctness
- ✅ Refutes the existing
CodexAgentService.ts:533assumption ("sandbox locked at creation") and updates the comment to match. That assumption was the root cause of the resume drift. - ✅
sandboxConfigArgslives in thededup()chain, so user-suppliedcliConfigArgsoverrides still win —userConfigKeys(L464–473) seessandbox_modeanddedup()correctly skips the system entry. - ✅
toTomlString(sandboxMode)matches the majority pattern in this file (L253, L258, L310, L323, L420, L495, L514). The loneapproval_policy="${approvalPolicy}"outlier (L439) is pre-existing tech debt, not introduced here. - ✅ Scope is minimal: only the
resumebranch argv is extended; freshexecbranch and its--sandbox <mode>flag are unchanged. Verified viagrep -c sandboxConfigArgson the diff — 2 hits (1 declaration + 1 usage in resume only).
Tests
- ✅ Two new + two modified tests cover: default-resume sandbox, env-configured-resume sandbox, no
--sandboxflag on resume, no--add-diron resume while requiringsandbox_modeconfig. - ✅ Windows Smoke job passing is the highest-signal check here — it confirms the
--config sandbox_mode=channel actually maps to the non-elevated runtime on Win, which is exactly the path under suspicion in #987. - Tests stop at the argv mock boundary; that's the right scope for cat-cafe. Deeper validation lives in Windows Smoke + reporter-side re-validation.
Pre-merge note (non-blocking)
Closes #987 may be over-claiming. The bug report doesn't pin down whether the failure happens on exec or exec resume — the wording "let GPT-5.5 set up the proxy" reads more like a turn-1 / fresh-exec path, which this PR does not touch. Two clean options at merge time:
- Change
Closes→Refsin the PR body, then reopen #987 after merge to await reporter re-validation on Windows. - Merge as-is, then manually reopen #987 with a comment asking the reporter to re-test against the fix.
Either works; pick whichever is lighter at merge. The fix itself stands on its own merit regardless of #987 disposition.
Thanks for the careful, narrowly-scoped fix and the regression coverage @snvtac.
— Reviewed by 宪宪 (布偶猫 Opus 4.7) on behalf of cat-cafe / Clowder AI maintainers. [宪宪/Opus 4.7🐾]
PR Type
Related Issue
Closes #987
Feature Doc (Feature PRs only)
N/A
What
codex exec resumevia--config sandbox_mode="...".--sandboxand--add-dirflags.Why
Issue #987 reports a Windows PowerShell launch failure where Clowder believes Codex is running in
danger-full-access, but the failure path reaches the Windows managed sandbox runner. Fresh Codex exec already passes--sandbox; resume did not replay the sandbox mode and could drift back to a CLI default.This keeps the fix narrow to the API resume argv path. Draft PR #980 covers a broader nearby surface; this PR intentionally does not include the UI default or Windows CLI resolver changes from that branch.
Tradeoff
This is argv-level coverage rather than a Windows manual repro. The change is limited to the supported resume config channel because
codex exec resumedoes not accept--sandbox.Test Evidence
AC Checklist (Feature PRs only)
N/A