fix(persona-kit): warn and skip approvalPolicy on codex 0.1.77+#167
Conversation
The `--ask-for-approval` flag was removed from codex in 0.1.77 (replaced by `--sandbox <mode>` + `--dangerously-bypass-approvals-and-sandbox`). `buildInteractiveSpec` was still generating `--ask-for-approval <policy>` for any persona that set `harnessSettings.approvalPolicy`, causing every codex agent step to fail immediately with a parse error: error: unexpected argument '--ask-for-approval' found This surfaced in ricky's auto-fix loop as a warning that prevented workflow repair and caused every auto-fix iteration to burn its full retry budget (~hours per spec in overnight runs). Fix: emit a deprecation warning and skip the flag entirely. The bypass case is already handled by `dangerouslyBypassApprovalsAndSandbox: true`. Mark `approvalPolicy` as @deprecated in the type definition. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pprovalPolicy ts-json-schema-generator with jsDoc:'extended' emits the @deprecated tag text as the value when text is present, producing a non-standard string. Separate the tag from the description so the generator emits `"deprecated": true` + `"description": "..."` per JSON Schema 2020-12. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR deprecates ChangesDeprecate approvalPolicy flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request deprecates the approvalPolicy configuration across schemas, types, and the spec builder, as the --ask-for-approval flag has been removed in codex 0.1.77+. Instead of passing the flag, the code now emits a deprecation warning. The reviewer pointed out that this warning is currently skipped when dangerouslyBypassApprovalsAndSandbox is enabled because the check is nested inside an else block, and suggested refactoring the check to be outside of the conditional block so that the warning is always emitted.
| if (harnessSettings?.approvalPolicy) { | ||
| args.push('--ask-for-approval', harnessSettings.approvalPolicy); | ||
| // `--ask-for-approval` was removed in codex 0.1.77+ (replaced by | ||
| // `--sandbox` + `--dangerously-bypass-approvals-and-sandbox`). | ||
| // Emit a warning and skip the flag rather than pass an unknown arg | ||
| // that causes codex to exit immediately with a parse error. | ||
| warnings.push( | ||
| `codex harnessSettings.approvalPolicy ("${harnessSettings.approvalPolicy}") is not supported in codex 0.1.77+; ` + | ||
| `the --ask-for-approval flag was removed. Use dangerouslyBypassApprovalsAndSandbox: true for non-interactive execution, ` + | ||
| `or sandboxMode for filesystem access control.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
When dangerouslyBypassApprovalsAndSandbox is set to true, the else block is skipped entirely. As a result, the deprecation warning for approvalPolicy will not be emitted even if it is defined in harnessSettings. Since approvalPolicy is deprecated and ignored in all cases, the deprecation warning should be checked and emitted regardless of whether dangerouslyBypassApprovalsAndSandbox is enabled. Consider refactoring the approvalPolicy check to be outside of the if-else block.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/persona-kit/src/interactive-spec.ts (1)
256-275:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmit
approvalPolicydeprecation warning regardless of bypass mode.When both
dangerouslyBypassApprovalsAndSandboxandapprovalPolicyare set,approvalPolicyis still ignored but no warning is emitted due to branch placement. The warning should fire wheneverapprovalPolicyis present.Proposed fix
const args = ['-m', stripProviderPrefix(model)]; if (mcpServers && Object.keys(mcpServers).length > 0) { appendCodexMcpServerArgs(args, mcpServers, warnings); } + if (harnessSettings?.approvalPolicy) { + warnings.push( + `codex harnessSettings.approvalPolicy ("${harnessSettings.approvalPolicy}") is not supported in codex 0.1.77+; ` + + `the --ask-for-approval flag was removed. Use dangerouslyBypassApprovalsAndSandbox: true for non-interactive execution, ` + + `or sandboxMode for filesystem access control.` + ); + } if (harnessSettings?.dangerouslyBypassApprovalsAndSandbox) { // Single combined flag — collapses "no sandbox + never ask" and // suppresses codex's interactive "are you sure?" startup // confirmation. The two-flag form below still prompts. args.push('--dangerously-bypass-approvals-and-sandbox'); } else { if (harnessSettings?.sandboxMode) { args.push('--sandbox', harnessSettings.sandboxMode); } - if (harnessSettings?.approvalPolicy) { - // `--ask-for-approval` was removed in codex 0.1.77+ (replaced by - // `--sandbox` + `--dangerously-bypass-approvals-and-sandbox`). - // Emit a warning and skip the flag rather than pass an unknown arg - // that causes codex to exit immediately with a parse error. - warnings.push( - `codex harnessSettings.approvalPolicy ("${harnessSettings.approvalPolicy}") is not supported in codex 0.1.77+; ` + - `the --ask-for-approval flag was removed. Use dangerouslyBypassApprovalsAndSandbox: true for non-interactive execution, ` + - `or sandboxMode for filesystem access control.` - ); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/persona-kit/src/interactive-spec.ts` around lines 256 - 275, The deprecation warning for harnessSettings.approvalPolicy should be emitted regardless of dangerouslyBypassApprovalsAndSandbox; currently it's inside the else branch and skipped when dangerouslyBypassApprovalsAndSandbox is true. Move or duplicate the approvalPolicy check so that, after inspecting harnessSettings.dangerouslyBypassApprovalsAndSandbox, you still run a conditional that checks harnessSettings.approvalPolicy and pushes the same message into warnings (referring to the existing warnings array and the same message string), while leaving the existing args logic for args.push('--dangerously-bypass-approvals-and-sandbox') and sandbox handling (harnessSettings.sandboxMode) unchanged.
🧹 Nitpick comments (1)
packages/persona-kit/src/interactive-spec.test.ts (1)
117-130: ⚡ Quick winAdd regression coverage for
approvalPolicy+ bypass together.Current assertions only cover
approvalPolicywithoutdangerouslyBypassApprovalsAndSandbox. A focused case for both fields prevents warning-regression in this deprecation path.Suggested test addition
+test('codex still warns about deprecated approvalPolicy when bypass flag is set', () => { + const result = buildInteractiveSpec({ + harness: 'codex', + personaId: 'test-persona', + model: 'openai-codex/gpt-5.3-codex', + systemPrompt: 'x', + harnessSettings: { + reasoning: 'high', + timeoutSeconds: 1200, + approvalPolicy: 'never', + dangerouslyBypassApprovalsAndSandbox: true + } + }); + assert.ok(result.args.includes('--dangerously-bypass-approvals-and-sandbox')); + assert.ok(result.warnings.some((w) => w.includes('approvalPolicy') && w.includes('not supported'))); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/persona-kit/src/interactive-spec.test.ts` around lines 117 - 130, Add a focused test that exercises both approvalPolicy and dangerouslyBypassApprovalsAndSandbox together: construct the same invocation that produced result in interactive-spec.test.ts but include dangerouslyBypassApprovalsAndSandbox: true alongside approvalPolicy, then assert the resulting result.args still matches the expected flags (the same array currently asserted) and assert result.warnings contains the deprecation message for approvalPolicy but does NOT include any additional warning about bypass; reference the existing result variable and the assertion blocks around result.args and result.warnings to add this new check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/persona-kit/src/interactive-spec.ts`:
- Around line 256-275: The deprecation warning for
harnessSettings.approvalPolicy should be emitted regardless of
dangerouslyBypassApprovalsAndSandbox; currently it's inside the else branch and
skipped when dangerouslyBypassApprovalsAndSandbox is true. Move or duplicate the
approvalPolicy check so that, after inspecting
harnessSettings.dangerouslyBypassApprovalsAndSandbox, you still run a
conditional that checks harnessSettings.approvalPolicy and pushes the same
message into warnings (referring to the existing warnings array and the same
message string), while leaving the existing args logic for
args.push('--dangerously-bypass-approvals-and-sandbox') and sandbox handling
(harnessSettings.sandboxMode) unchanged.
---
Nitpick comments:
In `@packages/persona-kit/src/interactive-spec.test.ts`:
- Around line 117-130: Add a focused test that exercises both approvalPolicy and
dangerouslyBypassApprovalsAndSandbox together: construct the same invocation
that produced result in interactive-spec.test.ts but include
dangerouslyBypassApprovalsAndSandbox: true alongside approvalPolicy, then assert
the resulting result.args still matches the expected flags (the same array
currently asserted) and assert result.warnings contains the deprecation message
for approvalPolicy but does NOT include any additional warning about bypass;
reference the existing result variable and the assertion blocks around
result.args and result.warnings to add this new check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6916843-408d-402f-8d24-396c700503ae
📒 Files selected for processing (4)
packages/persona-kit/schemas/persona.schema.jsonpackages/persona-kit/src/interactive-spec.test.tspackages/persona-kit/src/interactive-spec.tspackages/persona-kit/src/types.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The deprecation warning was inside the `else` block, so it was silently skipped when `dangerouslyBypassApprovalsAndSandbox: true` was also set. Move the warning before the if/else so it always fires when approvalPolicy is present, regardless of the bypass flag. Add a test asserting the warning fires even with both fields set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Supersedes #166 (clean rebase onto latest main).
Root cause
buildInteractiveSpecwas generatingcodex exec ... --ask-for-approval <policy>. That flag was removed in codex 0.1.77+ (replaced by--sandbox+--dangerously-bypass-approvals-and-sandbox). Any persona withharnessSettings.approvalPolicyset (e.g.proactive-agent-builder.jsonwith"approvalPolicy": "on-request") caused every codex agent step to exit immediately:This surfaced in ricky's auto-fix/repair loop as a captured warning, burning the full 7-retry budget (~hours per spec in overnight runs).
Fix
interactive-spec.ts: emit a deprecation warning and skip the flag —on-request(most common value) is default behavior anyway;neveris already covered bydangerouslyBypassApprovalsAndSandbox: truetypes.ts: split@deprecatedfrom@descriptionsots-json-schema-generatoremits"deprecated": true(boolean, per JSON Schema 2020-12) + a separatedescriptionpersona.schema.json: regenerated — now has"deprecated": true, "description": "..."instead of the previous"deprecated": "<string>"interactive-spec.test.ts: assert flag is NOT emitted and deprecation warning IS presentVerification
npm testinpackages/persona-kit→ 229/229 pass🤖 Generated with Claude Code