Skip to content

fix(codex): preserve plugin app approvals in side conversations#98812

Merged
steipete merged 1 commit into
mainfrom
codex/fix-codex-side-fork-approvals
Jul 2, 2026
Merged

fix(codex): preserve plugin app approvals in side conversations#98812
steipete merged 1 commit into
mainfrom
codex/fix-codex-side-fork-approvals

Conversation

@steipete

@steipete steipete commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Related: #98501
Related: #97327
Related: #98499

What Problem This Solves

Codex side conversations created with /btw fork from a fresh request config. That fork path did not replay the persisted plugin app policy context, so an app configured with allow_destructive_actions: "ask" could lose its app-scoped approvals_reviewer: "user" boundary in the side conversation. Reusing the thread-level reviewer would not be equivalent: it could either miss the ask app or broaden approval routing for sibling apps.

Why This Change Was Made

The side-question owner now merges buildCodexPluginAppsConfigPatchFromPolicyContext(binding.pluginAppPolicyContext) into the fork config, alongside the existing native-hook, runtime, network-proxy, sandbox, and base-reviewer fields. This uses the same canonical persisted-policy replay already used by thread start/resume, keeps the "ask" hard-cut naming from #98501, and does not introduce a compatibility path.

Focused regression coverage proves that an ask-mode app keeps app-scoped user review after the fork while sibling true, false, and "auto" apps retain their own policy and the top-level reviewer remains unchanged.

Contributor provenance: Kevin Lin (@kevinslin) authored the app-scoped approval design in #97327 and the "ask" hard cut in #98501. This follow-up preserves that boundary at the previously omitted side-fork owner.

AI-assisted: yes. Codex implemented the repair and completed a clean gpt-5.6-sol xhigh autoreview against the exact synced commit.

User Impact

When a user opens a Codex side conversation from a thread containing an ask-mode plugin app, that app's destructive actions still route to the user for approval. Mixed-policy sibling apps remain unchanged, and unrelated thread-level approvals are not broadened.

Evidence

  • Exact base: 3e50f41dd6ea3446b5c98a2f19ec70982ac908e6; its substantive main run 28555775438 completed with 111 successes, one intentional skip, and zero failures.
  • Exact candidate: f11fec224f1ce283a13402f610b96931831d99e3; parent is the exact base above; tree is c105a1a74cfe02e86be1766595fcdb9bd58944eb.
  • The synced candidate has the same stable patch id (f5343a7972cd568dfb3891e60958b2a22fb62702) as the approved candidate and remains limited to extensions/codex/src/app-server/side-question.ts and extensions/codex/src/app-server/side-question.test.ts (+114/-1).
  • Focused tests passed: 69/69 across side-question.test.ts and plugin-thread-config.test.ts.
  • Blacksmith Testbox tbx_01kwg2ym3gm44kb6rp1c5end72 (jade-shrimp) passed the changed gate in run 28556494609: extension production/test typechecks, lint, repository guards, DB/sidecar guards, and import-cycle checks. The same Testbox passed the full corepack pnpm build, including SDK declarations/exports and UI build.
  • Fresh genuine autoreview used workspace Codex 0.142.4 with gpt-5.6-sol and xhigh reasoning against the exact pinned base: no accepted/actionable findings, 0.99 confidence.
  • Direct dependency inspection used the pinned sibling Codex rust-v0.142.4 source: codex-rs/app-server/src/request_processors/thread_processor.rs:3280 and :3371 confirm fork config reload; codex-rs/app-server-protocol/src/protocol/v2/thread.rs:484 and :531 define the fork request/config override; codex-rs/config/src/types.rs:451 and :456 define app-scoped approvals_reviewer; codex-rs/core/src/connectors.rs:548 confirms app -> _default -> global precedence; codex-rs/core/src/session/mcp.rs:508 routes codex_apps elicitation through it; codex-rs/core/src/connectors_tests.rs:314 covers precedence.
  • git diff --check passed.

Commands:

OPENCLAW_HEAVY_CHECK_LOCK_SCOPE=worktree OPENCLAW_VITEST_FS_MODULE_CACHE_PATH=/tmp/openclaw-codex-side-fork-synced-cache node scripts/run-vitest.mjs extensions/codex/src/app-server/side-question.test.ts extensions/codex/src/app-server/plugin-thread-config.test.ts

node scripts/crabbox-wrapper.mjs run --id tbx_01kwg2ym3gm44kb6rp1c5end72 --provider blacksmith --env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 --env OPENCLAW_CHANGED_LANES_RAW_SYNC=1 -- corepack pnpm check:changed

node scripts/crabbox-wrapper.mjs run --reclaim --id tbx_01kwg2ym3gm44kb6rp1c5end72 --provider blacksmith -- corepack pnpm build

.agents/skills/autoreview/scripts/autoreview --mode branch --base 3e50f41dd6ea3446b5c98a2f19ec70982ac908e6 --engine codex --codex-bin node_modules/.bin/codex --model gpt-5.6-sol --thinking xhigh --prompt '<side-fork approval-boundary review>' --stream-engine-output

Known proof gap: no external live /btw plugin-app mutation was performed. The direct Codex contract check, focused fork regression, changed gate, full build, and clean model review cover the repaired boundary without creating a live destructive action.

@steipete steipete self-assigned this Jul 2, 2026
@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 8:38 PM ET / 00:38 UTC.

Summary
The PR replays persisted Codex plugin app approval policy into /btw side-thread fork config and adds regression coverage for mixed ask/true/false/auto app policies.

PR surface: Source +9, Tests +104. Total +113 across 2 files.

Reproducibility: yes. source-level reproduction is high confidence: current main replays app policy on thread start/resume but omits it from the side-thread fork config, while Codex reloads fork config from request overrides. I did not run a live /btw destructive plugin action during this read-only review.

Review metrics: 1 noteworthy metric.

  • Codex app policy replay: 1 fork config replay path added. The changed path affects how persisted plugin-app approval policy is carried into a side-thread session.

Stored data model
Persistent data-model change detected: database schema: extensions/codex/src/app-server/side-question.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🌊 off-meta tidepool
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] The change affects Codex plugin-app approval routing in forked session state; exact-head checks and maintainer review should remain the merge gate because no live destructive /btw plugin-app mutation was performed.

Maintainer options:

  1. Land after maintainer signoff (recommended)
    Accept the focused session-state and approval-boundary risk if maintainers are satisfied with the source trace, Codex contract check, focused tests, changed gate, and build proof.
  2. Request live side-fork proof
    Ask for a live /btw run with an ask-mode plugin app if maintainers want runtime evidence beyond the focused regression and Codex contract inspection.

Next step before merge

  • [P1] No automated repair is needed; the remaining action is maintainer review and merge handling for a protected-label PR.

Security
Cleared: The diff does not add dependencies, workflows, secret handling, or broad execution paths; its security-sensitive approval-routing change matches the stated fix.

Review details

Best possible solution:

Land or close this maintainer-labeled PR after maintainer review; the best code shape is the focused side-fork replay, not a broader global reviewer or config rebuild change.

Do we have a high-confidence way to reproduce the issue?

Yes, source-level reproduction is high confidence: current main replays app policy on thread start/resume but omits it from the side-thread fork config, while Codex reloads fork config from request overrides. I did not run a live /btw destructive plugin action during this read-only review.

Is this the best way to solve the issue?

Yes, this appears to be the best focused fix: it replays the same persisted app-policy patch already used by thread lifecycle at the omitted side-fork owner. Broader alternatives like changing the top-level reviewer or rebuilding plugin inventory inside /btw would be more invasive and could broaden sibling app behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 273729bd95ef.

Label changes

Label changes:

  • add P2: The PR addresses a bounded Codex plugin approval routing bug with limited surface area.
  • add merge-risk: 🚨 session-state: The diff changes which persisted plugin-app policy state is replayed into Codex side-thread forks.
  • add merge-risk: 🚨 security-boundary: The replayed state controls app-scoped destructive-action approval routing for Codex plugin apps.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR has the maintainer label, so the external contributor proof gate does not apply; the body still documents focused tests, Testbox changed gate/build, and a known live /btw proof gap.

Label justifications:

  • P2: The PR addresses a bounded Codex plugin approval routing bug with limited surface area.
  • merge-risk: 🚨 session-state: The diff changes which persisted plugin-app policy state is replayed into Codex side-thread forks.
  • merge-risk: 🚨 security-boundary: The replayed state controls app-scoped destructive-action approval routing for Codex plugin apps.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR has the maintainer label, so the external contributor proof gate does not apply; the body still documents focused tests, Testbox changed gate/build, and a known live /btw proof gap.
Evidence reviewed

PR surface:

Source +9, Tests +104. Total +113 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 10 1 +9
Tests 1 104 0 +104
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 114 1 +113

What I checked:

Likely related people:

  • kevinslin: Authored the merged per-app approval reviewer policy and ask-mode PRs that define the policy this follow-up preserves. (role: feature owner; confidence: high; commits: 389c355bcf3f, d9b5afad180b; files: extensions/codex/src/app-server/plugin-thread-config.ts, extensions/codex/src/app-server/thread-lifecycle.ts)
  • kevinlin-openai: Merged the ask-mode PR that changed the persisted destructive approval mode now replayed by this patch. (role: merger; confidence: medium; commits: d9b5afad180b; files: extensions/codex/src/app-server/plugin-thread-config.ts)
  • Eva: Current side-question fork config code and the replay helper are blamed to the grafted commit that added the current Codex app-server files in this checkout. (role: introduced side-question module; confidence: medium; commits: 72653b600c81; files: extensions/codex/src/app-server/side-question.ts, extensions/codex/src/app-server/plugin-thread-config.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jul 2, 2026
@steipete

steipete commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Land-ready proof for exact head f11fec224f1ce283a13402f610b96931831d99e3:

  • Ancestry/tree/scope: parent 3e50f41dd6ea3446b5c98a2f19ec70982ac908e6, tree c105a1a74cfe02e86be1766595fcdb9bd58944eb, stable patch id f5343a7972cd568dfb3891e60958b2a22fb62702; only extensions/codex/src/app-server/side-question.ts (+10/-1) and side-question.test.ts (+104) changed. The repository prepare flow preserved that exact head and skipped its push because the remote SHA already matched. Mainline drift through c49714f7b6d2a6690ccd03363a568c6e0a4b68fc does not touch either target file.
  • Behavior: the side-fork owner replays persisted pluginAppPolicyContext with the canonical app config helper. The regression proves an ask app retains app-scoped approvals_reviewer: "user"; sibling true, false, and "auto" policies plus the top-level reviewer remain unchanged. Native hooks, runtime config, network proxy, sandbox, and fix(codex): rename destructive approval mode to ask #98501's hard-cut naming are preserved.
  • Focused proof: 69/69 tests passed across side-question.test.ts and plugin-thread-config.test.ts; git diff --check passed.
  • Remote proof: pinned-base main run 28555775438 passed with 111 successes, one intentional skip, zero failures. Blacksmith Testbox tbx_01kwg2ym3gm44kb6rp1c5end72 (jade-shrimp) passed the changed gate in 28556494609 and the full build. Exact PR-head CI 28557111795 completed successfully with 44 successful jobs and zero failures.
  • Independent review: fresh workspace Codex 0.142.4 with gpt-5.6-sol / xhigh returned no findings at 0.99 confidence.
  • Direct pinned Codex contract checked: codex-rs/app-server/src/request_processors/thread_processor.rs:3280 and :3371 (fresh fork config load); codex-rs/app-server-protocol/src/protocol/v2/thread.rs:484 and :531 (fork config override); codex-rs/config/src/types.rs:451 and :456 (app reviewer); codex-rs/core/src/connectors.rs:548 (app -> _default -> global precedence); codex-rs/core/src/session/mcp.rs:508 (codex_apps elicitation routing); codex-rs/core/src/connectors_tests.rs:314 (precedence coverage).
  • Provenance: Kevin Lin (@kevinslin) authored the app-scoped approval design in fix(codex): preserve per-app approval reviewer policy #97327 and the ask hard cut in fix(codex): rename destructive approval mode to ask #98501; this follow-up preserves his boundary at the omitted side-fork path.

Known proof gap: no external live /btw destructive plugin mutation was performed. The direct dependency contract, captured fork regression, changed gate, full build, exact-head CI, and clean autoreview cover the repair without creating a destructive live action.

@steipete
steipete merged commit 27104c1 into main Jul 2, 2026
147 of 158 checks passed
@steipete
steipete deleted the codex/fix-codex-side-fork-approvals branch July 2, 2026 00:42
@steipete

steipete commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

vincentkoc added a commit that referenced this pull request Jul 2, 2026
@vincentkoc
Merge remote-tracking branch 'origin/main' into release/2026.7.1
01164c5 
* origin/main:
  test(gateway): isolate live release agent state
  test(plugins): repair prerelease validation fixtures
  fix(discord): guard JSON.parse against malformed API response bodies (#97889)
  fix(codex): preserve app approvals in side forks (#98812)
  fix(agents): don't inject A2A turns into isolated-cron sessions_send (#92257) (#92283)
  fix(browser): guard setDeep against empty keys array (#98138)
  docs(telegram): move maintainer decisions into scoped AGENTS.md
  fix(ios): generate light app store screenshots only
  fix(gateway): distinguish reachable gateway from failed status probe (#98183)
@Haderach-Ram Haderach-Ram mentioned this pull request Jul 2, 2026
Open
@github-actions github-actions Bot mentioned this pull request Jul 2, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 2, 2026
@steipete
fix(codex): preserve app approvals in side forks (openclaw#98812)
90ea0bb 
Co-authored-by: Peter Steinberger <[email protected]>
@steipete steipete mentioned this pull request Jul 2, 2026
Merged
This was referenced Jul 2, 2026
Merged
Merged
Closed
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 3, 2026
@steipete @chenyangjun-xy
fix(codex): preserve app approvals in side forks (openclaw#98812)
97088b2 
Co-authored-by: Peter Steinberger <[email protected]>
sheyanmin pushed a commit to sheyanmin/openclaw that referenced this pull request Jul 8, 2026
@steipete @sheyanmin
fix(codex): preserve app approvals in side forks (openclaw#98812)
ca9b4cc 
Co-authored-by: Peter Steinberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

extensions: codex maintainer Maintainer-authored PR merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete