Skip to content

fix(codex): apply permission settings to new Chat sessions - #19206

Closed
J-TN wants to merge 4 commits into
stablyai:mainfrom
J-TN:fix/codex-chat-yolo-permissions
Closed

J-TN wants to merge 4 commits into
stablyai:mainfrom
J-TN:fix/codex-chat-yolo-permissions

Conversation

@J-TN

@J-TN J-TN commented Sep 7, 2026 •

Copy link
Copy Markdown

ELI5

Starting a new Codex Chat can restrict file access and ask for approval even when YOLO is enabled in Orca Settings. Codex's app-server accepts the terminal YOLO flag but does not apply it. This change translates the configured permission flags into the configuration that a new Chat actually uses.

What Changed

  • Translate YOLO into -c approval_policy="never" -c sandbox_mode="danger-full-access" when parsing configured arguments for a new Codex Chat.
  • Translate explicit approval and sandbox flags, including short and inline forms, so restricted settings also take effect.
  • Add parser regression tests for POSIX, PowerShell and cmd, YOLO disabled, and explicit restricted settings.

The final diff changes only the argument parser and its test file. Saved-record launch resolution and its tests are identical to the upstream base. There is no migration of existing conversations or change to thread resume behavior.

Why

With codex-cli 0.153.4, an isolated thread/start through codex --dangerously-bypass-approvals-and-sandbox app-server returned approvalPolicy: "on-request" and a readOnly sandbox. Equivalent configuration overrides returned approvalPolicy: "never" and dangerFullAccess. No model turn was needed for that reproduction.

The existing parser already converts reasoning effort into app-server configuration. This extends that conversion for permission settings. The normal session-creation path stores the converted arguments, so a second conversion at process launch is unnecessary.

Linked Issue

Related to #10330. This addresses the new desktop Chat manifestation reported during investigation. It does not auto-close the original intermittent mobile terminal-launch issue.

Mobile path review: ordinary new Codex sessions on a host advertising structured-session support use agentSession.create and the same host-side configured-argument parser. That path should benefit on an updated host. Prompted/quick-command launches and fallback terminal sessions use a separate terminal command builder. Mobile behavior has not been manually verified.

Visual Proof

N/A — No rendered UI change; the backend launch configuration changes. The contributor performed a desktop before/after comparison with a shell write outside the opened workspace:

Case Observed result
Baseline fb322046e82b, fresh Chat Shell returned operation not permitted; before.txt absent
Initial fix 21bee8392790, fresh Chat Exit 0 without escalation or permission changes; after.txt independently verified to contain orca-yolo-probe and a newline
Baseline conversation resumed in the fixed app Still blocked; resumed.txt absent
Narrowed fix 096b20366011, contributor-confirmed new Chat with restricted arguments Exit 1 / operation not permitted; restricted.txt independently confirmed absent

The desktop comparison used macOS and GPT-6 Astra. The final scope reduction preserves the tested parser conversion and removes the redundant saved-record conversion. The positive YOLO test has not been repeated on the narrowed revision; the restricted-mode control was performed there. Probe files were checked independently from the Chat tool response. No screenshots are attached.

Testing

  • I manually tested these changes locally (isolated app-server protocol check and contributor desktop comparison on the initial fix revision, as detailed above).
  • Automated tests added/updated.
  • Manual restricted-mode control in a fresh desktop Chat (contributor-confirmed; probe-file absence independently verified).

The contributor confirmed creating a new Chat after setting Codex Arguments to -a on-request -s read-only. The shell write was blocked. The model label was missing in that Chat; after restoring YOLO in Settings, the UI displayed Astra with medium effort. The exact model and effective runtime arguments for the restricted Chat were not independently verified. The missing label is a separate observation, not diagnosed or fixed by this PR.

Checks after narrowing the diff, with ORCA_BACKGROUND_LAUNCH=1:

Check Result
Targeted parser, launch resolution, runtime wiring, adapter, lifecycle and thread-open tests Passed: 62 tests across six files
Oxlint and formatting on the two final changed files Passed
pnpm tc:node Fails on existing TS1117 duplicate keys in unchanged src/main/agent-hooks/first-work-branch-rename.test.ts, lines 106 and 179
pnpm run build:electron-vite Passed (bundle check; does not run full typechecking)

The full contributor checks were run before scope reduction on the same upstream base. pnpm lint, pnpm typecheck, and pnpm build failed on the same unrelated duplicate-key file. pnpm test reported 75,442 passed, 371 skipped, and one failure in unchanged config/scripts/skill-recipe-shell.test.mjs: macOS Bash 3.2 emits vercel_args[@]: unbound variable rather than the expected cleanup message. That failure also reproduced in isolation and does not import either Codex module. Full checks were not repeated after the removal; the relevant targeted checks above were rerun. No unrelated fixes are included.

Earlier mobile routing tests (27) and structured-session host RPC tests (38) passed. Linux, Windows, SSH and mobile were not exercised end to end; shell parsing is covered by platform-independent tests.

AI Disclosure

OpenAI Codex (GPT-6) assisted with investigation, implementation, regression tests, compatibility review, and PR preparation.

Review

  • Cross-platform: uses the existing shell tokenizer and argv launch path; no platform-specific behavior added.
  • SSH/remote/local: configuration remains execution-host-owned; no new RPC fields, stream frames or local fallback. Remote execution not verified end to end.
  • Agents/integrations/providers: scoped to Codex structured Chat configuration; other agents and terminal command builders are unchanged.
  • Performance: extends an existing linear pass over configured arguments; no per-message work.
  • UI quality: no rendered UI changes; fresh desktop Chat behavior was manually compared as recorded above.
  • Security: bypass is added only for an explicit configured YOLO flag; empty arguments add no permissions, and explicit restricted flags are translated. Values are passed as argv rather than interpolated into shell commands. Conflicting custom overrides remain order-sensitive.
  • Lifecycle: saved-record launching, thread start/resume requests, journal ordering, cancellation and handoff implementation are unchanged. Worktree and folder resolution remain unchanged.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Existing conversations can retain their earlier restricted permissions. Reopening a baseline conversation in the fixed app did not resolve that behavior; saved-thread permission restoration is outside this PR. Start a new Chat to use the corrected configured permissions.

The original mobile terminal symptom may have a separate cause. No release/version changes are included. The manual restricted-mode control is complete. This PR is ready for maintainer review; the known full-check failures remain documented. At the readiness check, GitHub reported no merge conflicts and no check runs for the current head. Maintainer/CI validation remains outstanding.

Author

X/Twitter: Not provided.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

All four full commands were run previously; the final checkbox remains unchecked because they did not all pass. See the validation scope and failures above.

@J-TN J-TN changed the title fix(codex): apply Chat launch permission settings fix(codex): apply permission settings to new Chat sessions Sep 7, 2026
@J-TN
J-TN marked this pull request as ready for review September 7, 2026 09:13
@coderabbitai

coderabbitai Bot commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: aad855ca-2c14-46a2-84a9-afae45bc1726

📥 Commits

Reviewing files that changed from the base of the PR and between 9e0409d and 98abea0.

📒 Files selected for processing (1)
  • src/main/codex/codex-structured-app-server-args.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Codex app-server argument resolution now converts approval and sandbox flags into -c configuration assignments. The TUI bypass flag maps to approval_policy="never" and sandbox_mode="danger-full-access". Tests use shared helpers for YOLO defaults, explicit restricted permissions, and empty configuration.

Merge Risk: ⚪ Minimal · up to 98abe

New Chat sessions now apply configured approval and sandbox permissions through app-server configuration, including the YOLO bypass mapping. The covered permission modes preserve the intended restricted and disabled behavior, with no remaining merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: applying Codex permission settings to new Chat sessions.
Description check ✅ Passed The description is complete and relevant. It includes the required sections, explains the change and motivation, references issue #10330, documents testing and known unrelated failures, and marks visu…
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • YOLO → app-server conversion — resolveCodexStructuredAppServerArgs now maps --dangerously-bypass-approvals-and-sandbox to -c approval_policy="never" -c sandbox_mode="danger-full-access".
  • Explicit permission flags — -a/--ask-for-approval and -s/--sandbox are translated to -c approval_policy=… / -c sandbox_mode=… config overrides.
  • Regression tests — YOLO across posix/powershell/cmd, restricted inline + short forms, and empty args.

The change is correct and narrowly scoped. I verified the short-flag mapping against src/cli/codex-command-classification.ts (where -a is --ask-for-approval and -s is --sandbox, while --add-dir has no short form) and the legacy src/main/codex-cli/codex-read-only-app-server-args.ts (-s read-only -a never). The conversion is consumed only by the structured Chat path (resolveConfiguredStructuredLaunchArgs); terminal launches pass the raw YOLO flag, which the codex TUI applies natively. The config key names match config-settings-promotion.ts, and the bypass flag is intercepted before the BOOLEAN_FLAGS pass so nothing is pushed twice. The order-sensitivity of conflicting -c overrides is pre-existing and correctly acknowledged in the description.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Satisfies CodeRabbit's docstring-coverage pre-merge check on the touched
test functions and removes the duplicated expected-config arrays.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Test assertions routed through a helper — the three permission-permission assertions in codex-structured-app-server-args.test.ts were extracted into a documented expectPermissionConfig helper, with each case preserving its exact toEqual expectation verbatim.

The delta since the prior review is test-only: the production logic in codex-structured-app-server-args.ts is unchanged, and no defective assertion was introduced by the refactor (the YOLO, restricted, and empty-args cases assert the same approval_policy / sandbox_mode values as before).

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

CodeRabbit's docstring-coverage check counts anonymous callbacks in the
touched test block, so the new cases run through documented functions.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@nwparker

Copy link
Copy Markdown
Contributor

Thank you for this PR and for pinning down that the app-server ignores the terminal YOLO flag. New structured Codex chats now take their permission from a per-agent setting that maps to approval policy and sandbox directly (#30853, #31315), so full access takes effect. Since this branch is stale, we are closing it as superseded. Please open an issue if a new chat still ignores your permission setting on the latest build.

@nwparker nwparker closed this Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants