fix(cli): preserve dangerouslyBypassApprovalsAndSandbox on standalone personas#125
Conversation
… personas assertStandaloneHarnessSettings rebuilt the resolved HarnessSettings object field-by-field (reasoning, timeoutSeconds, sandboxMode, approvalPolicy, workspaceWriteNetworkAccess, webSearch) but never copied dangerouslyBypassApprovalsAndSandbox. Standalone local personas (intent + no extends) silently lost the flag before codex launch, so codex started with its default approval policy and kept prompting for confirmations — including MCP tool-call approvals. The extends/overlay merge path was unaffected (object spread preserves it); only the standalone path dropped it. Also adds the boolean type check to assertPartialHarnessSettingsShape for parity with the other codex-only settings, and a regression test asserting the flag survives standalone resolution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 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)
📝 WalkthroughWalkthroughAccepts and validates ChangesHarness settings flag propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (settings.dangerouslyBypassApprovalsAndSandbox !== undefined) { | ||
| out.dangerouslyBypassApprovalsAndSandbox = | ||
| settings.dangerouslyBypassApprovalsAndSandbox as boolean; | ||
| } |
There was a problem hiding this comment.
🟡 Missing mutual exclusion validation for dangerouslyBypassApprovalsAndSandbox in assertStandaloneHarnessSettings
The assertStandaloneHarnessSettings function now preserves dangerouslyBypassApprovalsAndSandbox but does not enforce the mutual exclusion constraint with sandboxMode, approvalPolicy, and workspaceWriteNetworkAccess. The canonical validator at packages/persona-kit/src/parse.ts:189-196 rejects any configuration where dangerouslyBypassApprovalsAndSandbox is present alongside those fields (even when false), and the type documentation at packages/persona-kit/src/types.ts:47-49 explicitly declares them mutually exclusive. A standalone local persona declaring both dangerouslyBypassApprovalsAndSandbox: true and sandboxMode: 'workspace-write' would pass validation silently, and at runtime the bypass flag wins (packages/persona-kit/src/interactive-spec.ts:256), causing sandboxMode/approvalPolicy/workspaceWriteNetworkAccess to be silently ignored without warning.
| if (settings.dangerouslyBypassApprovalsAndSandbox !== undefined) { | |
| out.dangerouslyBypassApprovalsAndSandbox = | |
| settings.dangerouslyBypassApprovalsAndSandbox as boolean; | |
| } | |
| if (settings.dangerouslyBypassApprovalsAndSandbox !== undefined) { | |
| const conflicts: string[] = []; | |
| if (settings.sandboxMode !== undefined) conflicts.push('sandboxMode'); | |
| if (settings.approvalPolicy !== undefined) conflicts.push('approvalPolicy'); | |
| if (settings.workspaceWriteNetworkAccess !== undefined) conflicts.push('workspaceWriteNetworkAccess'); | |
| if (conflicts.length > 0) { | |
| throw new Error( | |
| `${context}.dangerouslyBypassApprovalsAndSandbox is mutually exclusive with: ${conflicts.join(', ')}` | |
| ); | |
| } | |
| out.dangerouslyBypassApprovalsAndSandbox = | |
| settings.dangerouslyBypassApprovalsAndSandbox as boolean; | |
| } | |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/local-personas.ts (1)
756-759: ⚡ Quick winAvoid another hand-maintained
HarnessSettingsallowlist.
assertStandaloneHarnessSettingsstill rebuilds the object field-by-field. The nextHarnessSettingsproperty added upstream can validate and merge fine elsewhere, but still get silently dropped for standalone personas here—the same class of regression this PR just fixed. Please centralize the materialization so validation and standalone construction share one source of truth.🤖 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/cli/src/local-personas.ts` around lines 756 - 759, assertStandaloneHarnessSettings currently rebuilds a HarnessSettings object field-by-field (e.g., the manual assignment to out.dangerouslyBypassApprovalsAndSandbox), which risks dropping new properties; refactor it to centralize materialization by delegating to a single constructor/utility that merges/validates partial settings (for example a HarnessSettings.from/merge/normalize function or a new createHarnessSettingsFromPartial(settings) helper) and then validate/return that canonical object instead of manually copying fields—replace the manual per-field assignments in assertStandaloneHarnessSettings with a single call to that shared materializer so validation and standalone construction use the same source of truth.
🤖 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.
Nitpick comments:
In `@packages/cli/src/local-personas.ts`:
- Around line 756-759: assertStandaloneHarnessSettings currently rebuilds a
HarnessSettings object field-by-field (e.g., the manual assignment to
out.dangerouslyBypassApprovalsAndSandbox), which risks dropping new properties;
refactor it to centralize materialization by delegating to a single
constructor/utility that merges/validates partial settings (for example a
HarnessSettings.from/merge/normalize function or a new
createHarnessSettingsFromPartial(settings) helper) and then validate/return that
canonical object instead of manually copying fields—replace the manual per-field
assignments in assertStandaloneHarnessSettings with a single call to that shared
materializer so validation and standalone construction use the same source of
truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6e465bb-3e05-4962-aec8-ad2e31fd5ad0
📒 Files selected for processing (2)
packages/cli/src/local-personas.test.tspackages/cli/src/local-personas.ts
1 participant
Problem
A standalone local persona (one with
intent+ noextends) that setsharnessSettings.dangerouslyBypassApprovalsAndSandbox: truestill had codex prompt for confirmations at runtime — including MCP tool-call approval prompts (e.g. the nango docs MCP server).Root cause: the standalone local-persona harness-settings path was rebuilding the resolved
HarnessSettingsobject field-by-field and did not share persona-kit's canonical harness-settings parser, so new settings could be dropped or validated differently from packaged persona specs.The
extends/overlay merge path was unaffected by the original drop because it uses object spread ({ ...base.harnessSettings, ...override.harnessSettings }). Only the standalone path dropped the field.Fix
harnessSettingsmaterialization to persona-kit's canonicalparseHarnessSettingsparser.dangerouslyBypassApprovalsAndSandboxon standalone local personas.dangerouslyBypassApprovalsAndSandboxversus explicit codex sandbox/approval settings.Review feedback addressed
HarnessSettingsallowlist with the shared persona-kit parser.dangerouslyBypassApprovalsAndSandboxand explicitsandboxModeconflicts.Verification
corepack pnpm -r buildcorepack pnpm --filter @agentworkforce/cli test→ 188/188 passcorepack pnpm --filter @agentworkforce/cli lint🤖 Generated with Claude Code