Skip to content

fix(codex): preserve per-app approval reviewer policy#97327

Merged
kevinslin merged 10 commits into
mainfrom
dev/kevinlin/codex-app-inventory-version-key
Jun 29, 2026
Merged

fix(codex): preserve per-app approval reviewer policy#97327
kevinslin merged 10 commits into
mainfrom
dev/kevinlin/codex-app-inventory-version-key

Conversation

@kevinslin

@kevinslin kevinslin commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where enabling Codex plugins could overwrite a thread's approval reviewer globally, causing non-destructive app reads to inherit user review and making resumed or recovered threads diverge from the original per-app policy. It also fixes stale Codex app inventories after plugin upgrades.

Why This Change Was Made

Plugin configuration now leaves the top-level reviewer untouched and assigns approvals_reviewer: "user" only to concrete enabled apps whose resolved destructive-action mode is always. The same resolved policy is persisted and replayed for resume and recovery, while the inventory cache key includes the app/plugin version so upgraded inventories refresh.

The patch relies on Codex's native app tool approval-mode semantics instead of doing pre-start MCP tool discovery, so configured apps are not hidden before their thread-effective config exists. Existing app enablement, destructive/open-world flags, tool approval modes, activation validation, and fail-closed defaults remain intact.

AI-assisted: yes. Codex implemented the changes and completed a clean repository autoreview.

User Impact

Calendar and other read-only app actions can continue without an unnecessary user-review override. Apps with an effective allow_destructive_actions = "always" still route destructive approvals to the user at the app boundary, while auto, allow, and deny inherit the thread's configured reviewer. The behavior remains consistent after thread resume, recovery, and replay.

Evidence

  • Focused plugin/thread lifecycle, binding, and elicitation tests: 5 files passed, 202 tests passed.
  • Focused run-attempt resume-policy test: 1 passed, 114 skipped by the test filter.
  • File-scoped extension lint: passed.
  • Repository autoreview: clean, with no accepted or actionable findings.
  • Final-head Docker/Slack probe on 0ab4bbd175: Google Calendar read returned CALENDAR_READ_OK 7 without elicitation. A Calendar write reached create_event, was rejected by the configured auto reviewer, emitted no Slack elicitation, and an exact-title search confirmed that no event was created.

Commands:

node scripts/run-vitest.mjs extensions/codex/src/app-server/plugin-thread-config.test.ts extensions/codex/src/app-server/thread-lifecycle.binding.test.ts extensions/codex/src/app-server/thread-lifecycle.test.ts extensions/codex/src/app-server/session-binding.test.ts extensions/codex/src/app-server/elicitation-bridge.test.ts

OPENCLAW_VITEST_FS_MODULE_CACHE_PATH=/tmp/openclaw-vitest-run-attempt-reviewer-fixed node scripts/run-vitest.mjs extensions/codex/src/app-server/run-attempt.test.ts -t "passes configured app-server policy, sandbox, service tier, and model on resume"

node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.extensions.json extensions/codex/src/app-server/plugin-thread-config.ts extensions/codex/src/app-server/plugin-thread-config.test.ts extensions/codex/src/app-server/session-binding.ts extensions/codex/src/app-server/session-binding.test.ts extensions/codex/src/app-server/thread-lifecycle.binding.test.ts extensions/codex/src/app-server/elicitation-bridge.test.ts

.agents/skills/autoreview/scripts/autoreview --mode local --prompt-file /tmp/openclaw-codex-reviewer-autoreview.md --stream-engine-output

Real behavior proof

  • Behavior or issue addressed: Codex plugin setup must not force the root reviewer or _default reviewer; only concrete enabled apps with effective destructive mode always receive the user reviewer. App inventory must refresh after plugin version changes and persisted policy must replay identically.
  • Real environment tested: Local OpenClaw Docker image openclaw-debug:per-app-reviewer-final-0ab4bbd175-20260629t082935z (linux/amd64, local image ID sha256:3a1999f55f14…) built from a clean detached worktree at exact PR head 0ab4bbd175, with a healthy gateway at 127.0.0.1:18789. It was exercised through the kevinlin Slack/OpenClaw integration profile with the required five-plugin policy (google-calendar=auto, linear=always). The remote openclaw-gateway-kevinlin StatefulSet remained at zero replicas and its pod was absent.
  • Exact steps or command run after this patch: Built the full Codex+Slack image on the amd64 build daemon, transferred it directly to the local run daemon without a registry, asserted the compiled dist/thread-lifecycle-* reviewer branches, replaced the isolated local gateway container, verified health and plugin policy, then sent the canonical Google Calendar read and write/read-back requests through Slack. After the rejected write, searched Calendar for the exact unique title in the bounded test window.
  • Evidence after fix: Checklist, read request, and read reply show CALENDAR_READ_OK 7 with no elicitation. The write request reached codex_apps.google_calendar.create_event twice; both tool results were user rejected MCP tool call, the Slack thread reported that no event was created, and a direct exact-title search returned zero events. Final focused tests passed 202/202, the filtered run-attempt test passed, exact-head CI passed, oxlint passed, and autoreview was clean.
  • Observed result after fix: Read-only Calendar use no longer elicited in the live path. Destructive-write policy reached the app approval boundary without silently creating an event. Unit coverage confirms reviewer omission for root, _default, disabled patches, and all non-always effective modes, plus per-app user review for inherited or explicit always across initial and reconstructed patches.
  • What was not tested: A successful Calendar write was not observed because the integration profile intentionally configures google-calendar=auto, and its auto reviewer rejected the destructive call without emitting a Slack approval card. The live probe therefore proves final-head read behavior and the write rejection/no-side-effect path, while per-app always → user routing and resume/recovery replay remain covered by focused tests rather than a live always Calendar profile. The full run-attempt.test.ts file was not green locally: three environment-dependent tool-policy tests saw the local computer-use/MCP delegation configuration; the directly affected resume-policy case passed. Local pnpm check:changed was blocked before checks by a registry 403 fetching acpx.

@clawsweeper

clawsweeper Bot commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 5:29 AM ET / 09:29 UTC.

Summary
The PR adds OpenClaw/Codex plugin versions to app-inventory cache keys, scopes app-level user approval to always-destructive Codex plugin apps, and replays persisted plugin app policy on thread resume.

PR surface: Source +59, Tests +165. Total +224 across 8 files.

Reproducibility: yes. at source level: current main lacks app-level reviewer assignment and resume replay for persisted plugin app config, while PR-head source and tests cover the expected behavior. I did not run a live current-main repro in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Cache-key dimensions: 2 added. The app inventory cache key now includes OpenClaw and Codex plugin build versions, so maintainers should confirm the intended lifecycle is module/process reload rather than same-process package observation.
  • Approval replay surface: 1 app-level reviewer rule added; 1 resume replay path added. Approval routing and persisted binding replay affect compatibility for existing Codex plugin app sessions across resume and recovery.

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

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

  • [P1] Add redacted current-head Docker/Slack or gateway proof for Calendar read without elicitation and destructive create approval or rejection without side effects.
  • Include resume or recovery behavior in the proof, or explain why focused tests are the only practical coverage for that path.
  • After adding proof, update the PR body so ClawSweeper can re-review automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: Insufficient: the PR body provides live Docker/Slack proof for 0ab4bbd, but latest head is e450e44 after a later revert, so current-head redacted proof is still needed before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A real Slack smoke would directly prove the current-head user-visible Codex plugin approval behavior that is stale in the PR body. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis slack desktop smoke: verify current head e450e447 Calendar read has no elicitation and Calendar create reaches approval or rejection without creating an event, with private details redacted.

Risk before merge

  • [P1] The PR has the protected maintainer label, so conservative cleanup must keep it open for explicit maintainer handling.
  • [P1] The PR body's live Docker/Slack proof is for 0ab4bbd, but latest head is e450e44 after a later revert, so current-head real behavior remains unproven.
  • [P1] The diff changes Codex app config, app inventory cache scoping, and persisted binding replay, so a bad merge could affect upgraded app inventories or resumed/recovered plugin app threads.

Maintainer options:

  1. Refresh exact-head live proof (recommended)
    Run e450e44 or newer through a real Codex plugin app read/write plus resume or recovery flow, then update the PR body with redacted proof from that exact head.
  2. Accept source and test proof with owner risk
    A maintainer can choose to land based on source review, focused tests, and CI, but should explicitly own the stale live-proof gap and plugin-app upgrade/session risk.

Next step before merge

  • [P1] Protected maintainer label plus stale exact-head proof and compatibility/session-state risk require human maintainer handling rather than an automated repair.

Security
Cleared: No concrete security or supply-chain regression was found; the diff changes Codex runtime config/cache logic and tests without new dependencies, workflows, lockfiles, downloads, or secret-handling expansion.

Review details

Best possible solution:

Land this direction only after maintainers accept exact-head proof for read/no-elicitation, destructive approval or rejection without side effects, and resume/recovery policy replay.

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

Yes at source level: current main lacks app-level reviewer assignment and resume replay for persisted plugin app config, while PR-head source and tests cover the expected behavior. I did not run a live current-main repro in this read-only review.

Is this the best way to solve the issue?

Yes for the code direction: OpenClaw owns the plugin app config patch and binding replay, and upstream Codex supports app-level reviewer selection. It is not merge-ready until current-head real behavior proof covers the user-visible plugin app path.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority Codex plugin app approval/cache correctness PR with limited blast radius and no evidence of a production outage.
  • merge-risk: 🚨 compatibility: The diff changes Codex plugin app config defaults and cache scoping that can affect existing plugin-app users and upgrade behavior.
  • merge-risk: 🚨 session-state: The diff changes persisted Codex app-server binding replay, so an incorrect merge could make resumed or recovered threads diverge from their original plugin app policy.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Insufficient: the PR body provides live Docker/Slack proof for 0ab4bbd, but latest head is e450e44 after a later revert, so current-head redacted proof is still needed before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +59, Tests +165. Total +224 across 8 files.

View PR surface stats
Area Files Added Removed Net
Source 4 75 16 +59
Tests 4 178 13 +165
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 8 253 29 +224

What I checked:

Likely related people:

  • kevinslin: Recent merged history introduced native plugin app support, auto/always plugin approval modes, stale app snapshot recovery, and remote app-server plugin support on the same files and behavior. (role: feature history owner and current area contributor; confidence: high; commits: a1ac559ed7e6, e82d19fb06b5, c5d34c8376f8; files: extensions/codex/src/app-server/plugin-thread-config.ts, extensions/codex/src/app-server/app-inventory-cache.ts, extensions/codex/src/app-server/thread-lifecycle.ts)
  • vincentkoc: Path history shows recent work on Codex app-server lifecycle, dynamic tool protocol, network proxy, and reviewer/provider behavior adjacent to the resume and reviewer paths. (role: recent adjacent Codex app-server contributor; confidence: medium; commits: 77012f980785, 10b8b32380ea, 5c3469549159; files: extensions/codex/src/app-server/thread-lifecycle.ts)
  • steipete: Path history shows app inventory cache expiry, Codex app-server utility docs, and adjacent lifecycle repairs near the changed cache and replay surfaces. (role: adjacent cache and lifecycle contributor; confidence: medium; commits: 5fde637ba88f, 23258c86bec8, 77d9ac30bb8d; files: extensions/codex/src/app-server/app-inventory-cache.ts, extensions/codex/src/app-server/thread-lifecycle.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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 27, 2026
@kevinslin

kevinslin commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

The PR body now includes redacted executable output from the production cache-key implementation, captured and replay-verified in an isolated OpenClaw integration workspace. It shows the serialized key contains both build versions and changes when either version changes independently. No gateway, deployment, devbox, or external service was started.

@clawsweeper

clawsweeper Bot commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@harjothkhara

harjothkhara commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Small proof/lifecycle note before the re-review: the updated PR body proves the lower-level buildCodexAppInventoryCacheKey serializer changes when injected version strings change, but the runtime path does not receive those versions as per-call inputs.

In the PR head, CODEX_PLUGIN_VERSION is read once at module evaluation, and buildCodexPluginAppCacheKey then passes that module-level value plus OPENCLAW_VERSION into the key builder on each call:

  • openclaw/extensions/codex/src/app-server/plugin-app-cache-key.ts

    Line 18 in 020be8f

    const CODEX_PLUGIN_VERSION = readPluginPackageVersion({ require });

  • openclaw/extensions/codex/src/app-server/plugin-app-cache-key.ts

    Lines 31 to 45 in 020be8f

    export function buildCodexPluginAppCacheKey(params: CodexPluginAppCacheKeyParams): string {
    return buildCodexAppInventoryCacheKey(
    {
    codexHome:
    params.runtimeIdentity?.codexHome ??
    resolveCodexPluginAppCacheCodexHome(params.appServer, params.agentDir),
    endpoint: resolveCodexPluginAppCacheEndpoint(params.appServer),
    authProfileId: params.authProfileId,
    accountId: params.accountId,
    envApiKeyFingerprint: params.envApiKeyFingerprint,
    appServerVersion: params.appServerVersion ?? params.runtimeIdentity?.serverVersion,
    runtimeIdentity: params.runtimeIdentity,
    },
    OPENCLAW_VERSION,
    CODEX_PLUGIN_VERSION,

So I’d be careful about treating the new proof as evidence that a still-running OpenClaw process observes an installed Codex plugin version change. It demonstrates the serializer/key shape, not the runtime wrapper observing changed package metadata after startup. If the intended contract is “distinct cache scope after process/plugin module reload,” this patch shape looks consistent; if the target is same-process upgrade invalidation, the version read probably needs to happen at the reload/install boundary or per key-build instead of as a module-level constant.

@kevinslin

kevinslin commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Good distinction. The intended contract here is a distinct cache key for runtime/plugin module instances loaded with different build identities, not same-process observation of package metadata changing after module evaluation.

I updated the PR body to state that lifecycle boundary explicitly and to list same-process hot package updates under “What was not tested.” The code still reads CODEX_PLUGIN_VERSION once per module evaluation; this PR does not claim or add a per-call package metadata refresh.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 27, 2026
@kevinslin kevinslin changed the title fix(codex): refresh app inventory after upgrades fix(codex): preserve per-app approval reviewer policy Jun 29, 2026
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. and removed status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jun 29, 2026
@kevinslin

kevinslin commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Jun 29, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Open
This was referenced Jul 1, 2026
Closed
Merged
@steipete steipete mentioned this pull request Jul 2, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: codex maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. 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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kevinslin @harjothkhara