fix(codex): honor live noninteractive permission hooks - #20860
andrew12-forge wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughCodex permission requests with Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The updated permission flow preserves human prompts while suppressing attention for verified automatic permissions. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The changes satisfy the live Codex hook variant of issue Resolution Implement detection and propagation for the actual “Approve for me” auto-review outcome, or an equivalent authoritative noninteractive state. Suppress attention notifications while that state handles the request. Add a regression test that covers the issue’s auto-review scenario and confirms that genuine human waits still notify.
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The SSH relay path drops the new codexNonInteractivePermission proof before ingestRemote, so on SSH a Codex noninteractive PermissionRequest still lands as waiting — the exact false "needs input" this PR claims to fix. Local and WSL behave correctly.
Reviewed changes
- Codex permission classifier — new
isCodexNonInteractivePermissionRequesttreatsPermissionRequest+permission_mode: "bypassPermissions"+ a non-question tool as host-side proof the hook cannot ask a human. - Host normalization —
codex-events/codex-tool-fieldsmap that case toworkingwith no approval card;codex-stateadds aPreToolUse+ ask-user →waitingbranch and honors the proof in remote reconciliation. - Relay wire + main ingest — optional
codexNonInteractivePermissionon the envelope, re-validated inserver-ingest-remote, consumed byreconcileRemoteCodexState, and stripped before persistence. - Tests — listener, server-ingest, and renderer attention suites; all synthetic.
🚨 SSH ingress drops the proof before ingestRemote
Unlike the WSL link (which forwards the whole JSON‑RPC params object), the SSH session reconstructs the envelope field‑by‑field and omits the new flag. ingestRemote reads it from that reconstructed object, so on SSH it is always undefined; reconcileRemoteCodexState then re-derives waiting. This leaves the primary remote scenario advertised as blocked with no answerable card (the host already cleared interactivePrompt). The new tests call server.ingestRemote directly, so they never exercise this boundary and stay green.
Technical details
# SSH relay discards `codexNonInteractivePermission`
## Affected sites
- `src/main/ssh/ssh-relay-session.ts:1575-1610` — the explicit remap into `agentHookServer.ingestRemote` enumerates `grokPromptBoundary`, `compactTrigger`, `toolUseId`, `shedFields`, etc., but never `codexNonInteractivePermission`. Add it beside `compactTrigger` (line 1593).
- `src/main/agent-hooks/server/server-ingest-remote.ts:266-272` — the sole consumer reads `envelope.codexNonInteractivePermission`; with the field absent it produces `undefined`.
- `src/main/agent-hooks/server/server-status-update.ts:86-94` → `reconcileRemoteCodexState` → `codexLeadStateForHookEvent('PermissionRequest', false)` returns `waiting`.
- `src/main/ssh/ssh-relay-session-agent-hooks.integration.test.ts:580-637` — the "forwards remote hook transition metadata" test enumerates fields and would not catch the drop; `server-codex-noninteractive-permissions.test.ts` bypasses the mapping via direct `ingestRemote`.
- Reference: the WSL path is unaffected because `src/main/agent-hooks/wsl-hook-relay-link.ts:43` forwards `params` wholesale.
## Required outcome
- On SSH, a Codex `PermissionRequest` with `permission_mode: "bypassPermissions"` and a non-`request_user_input` tool reaches `ingestRemote` with the proof, reconciles to `working`, and dispatches no attention.
- A `default`-mode Codex approval and any `request_user_input` question still reconcile to `waiting`/attention on SSH.
- An SSH-boundary assertion covers the new field so the next wire field cannot be silently dropped.
## Suggested approach
Add `codexNonInteractivePermission: envelope.codexNonInteractivePermission === true ? true : undefined` to the literal in `wireUpAgentHookEvents`, and extend the SSH agent-hooks integration test's `objectContaining` with the flag for a codex `PermissionRequest` envelope.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| eventName: unknown, | ||
| hookPayload: Record<string, unknown> | ||
| ): boolean { | ||
| // Codex maps approval_policy=never to bypassPermissions, including sandbox auto-review. |
There was a problem hiding this comment.
The "including sandbox auto-review" claim does not hold against the same Codex source: guardian auto-review routes only when approval_policy ∈ {OnRequest, Granular} with approvals_reviewer == AutoReview, which maps to "default", not "bypassPermissions". Those auto-review requests therefore still surface waiting. The PR body scopes this out explicitly, so the comment is the only misleading part — consider dropping the auto-review clause.
|
+1 |
|
Superseded by #21389, which is merged. You identified the right defect — a Codex permission hook is not necessarily a question for a person — and the One thing worth recording, because we only learned it by measuring: Thanks for the work, and for spotting that the event is pre-decision. |
3 participants
ELI5
A Codex permission hook is not necessarily a question for a person. Orca currently marks even hooks whose live policy forbids human approvals as waiting, so a long command can produce a false “needs input” alert. This change honors the live hook's noninteractive mode and keeps real approvals and questions actionable.
What Changed
PermissionRequestwithpermission_mode: "bypassPermissions"to working and omit the fabricated human approval card. Preserverequest_user_inputon both PreToolUse and PermissionRequest.codexNonInteractivePermissionproof through the SSH relay so client reconciliation preserves the execution host's decision, including when a child genuinely needs input. The SSH session forwards the flag through its explicit envelope mapping. Do not persist that proof.Why
Codex's hook documentation describes PermissionRequest as a pre-decision hook. PostToolUse can arrive only when a long unified-exec command finishes. In Codex 0.154.0's hook implementation, approval policy
nevermaps tobypassPermissions; it is not proof that the sandbox is disabled. The approval implementation runs permission hooks before resolving approval.Use that current per-hook policy instead of launch arguments, absent terminal prompts, transcript guesses, or a longer timeout. Missing, malformed, unknown, and human-review modes retain attention.
Linked Issue
Related to #13600; also relevant to #19618. Reuses the existing report rather than opening another issue.
This is a scoped fix for the noninteractive-hook variant. It does not close #13600: ordinary “Approve for me” routes
OnRequest/GranularplusAutoReviewthrough Codex's guardian reviewer and reportspermission_mode: "default". That broader case remains unresolved by this PR. Existing PR #11046 proposes launch-argument ownership; #14715 defers synthetic bells. This change addresses live noninteractive status at the producer and leaves those broader proposals separate.Visual Proof
N/A — no new rendered UI. Status and notification routing are covered by synthetic tests; no live desktop notification screenshot was captured.
Before/after regression: replay PreToolUse → noninteractive PermissionRequest → ten seconds without PostToolUse. With this classification disabled, the real notification coordinator emits an unwanted alert at 1.5 seconds and the test fails. With the fix, no attention or completion is dispatched during the ten seconds. An unanswered human approval and question still notify after 1.5 seconds.
Testing
All checks ran on macOS with
ORCA_BACKGROUND_LAUNCH=1.pnpm tc: passed.pnpm lint: passed.pnpm run check:code-quality:changed: passed, zero new findings.pnpm run build:relayandpnpm run build:electron-vite: passed. These are build checks, not live Windows/Linux/SSH runs; host-only dependencies were installed.Reproduce the targeted tests with
ORCA_BACKGROUND_LAUNCH=1 pnpm test src/shared/agent-hook-listener-codex-permissions.test.ts src/main/agent-hooks/server-codex-noninteractive-permissions.test.ts src/renderer/src/components/terminal-pane/agent-completion-codex-live-permissions.test.ts.AI Disclosure
Implemented and reviewed with OpenAI Codex.
Review
Pullfrog found that the initial SSH tests bypassed a field-by-field session mapping which dropped the new flag. The follow-up fixes that missed boundary and adds transport-level regression coverage. It also removes wording that conflated noninteractive hooks with ordinary “Approve for me.”
Agent skill upstream boundary
Notes
All new reproduction data is synthetic. Raw session evidence, screenshots, local paths, and unrelated workspace content are omitted.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred) — exact local coverage and limits are listed above