refactor(codex): simplify app-server runtime ownership#93313
Conversation
openclaw-barnacle
Bot
added
docs
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 10:35 PM ET / 02:35 UTC. Summary Reproducibility: yes. at source level. A legacy app-server binding with no stored Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Fix the duplicate Do we have a high-confidence way to reproduce the issue? Yes at source level. A legacy app-server binding with no stored Is this the best way to solve the issue? No. The ownership direction is plausible, but this PR is not the best merge path until the wrong-agent guard and duplicate sandbox-guard fields are fixed, conflicts are resolved, and current-head lifecycle proof is refreshed. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 816038e97a5d. Label changesLabel justifications:
Evidence reviewedSecurity concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🦪 silver shellfish
clawsweeper
Bot
added
rating: 🧂 unranked krab
Mantis Telegram Desktop ProofSummary: Mantis captured native Telegram Desktop before/after GIFs showing Codex replies delivered in chat.
Motion-trimmed clips: |
clawsweeper
Bot
added
proof: 🎥 video
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7daba184b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { | ||
| createCodexCliSessionNodeHostCommands, | ||
| createCodexCliSessionNodeInvokePolicies, | ||
| listCodexCliSessionsOnNode, | ||
| resumeCodexCliSessionOnNode, | ||
| resolveCodexCliSessionForBindingOnNode, | ||
| } from "./src/node-cli-sessions.js"; | ||
| import { createCodexWebSearchProvider } from "./src/web-search-provider.js"; | ||
| } from "./src/node-cli-session-registration.js"; |
There was a problem hiding this comment.
This import rewrite drops createCodexWebSearchProvider, but register() still calls it when the Codex plugin is loaded. In any runtime that loads the plugin, registration now throws a ReferenceError before the later migration provider, commands, and lifecycle hooks can be registered, so the Codex plugin surface fails to start.
Useful? React with 👍 / 👎.
| ? modelScopedAppServer.approvalPolicy | ||
| : (binding.approvalPolicy ?? modelScopedAppServer.approvalPolicy); | ||
| const sandbox = useModelScopedPolicy | ||
| let sandbox = useModelScopedPolicy | ||
| ? modelScopedAppServer.sandbox | ||
| : (binding.sandbox ?? modelScopedAppServer.sandbox); | ||
| const nativeProviderWebSearchSupport = |
There was a problem hiding this comment.
The refactor removed the local nativeToolSurfaceEnabled = shouldEnableCodexAppServerNativeToolSurface(...) assignment, but this path still reads nativeToolSurfaceEnabled while starting every Codex /btw side question. As soon as a side question reaches this block it throws before it can fork the side thread, so /btw is unusable for Codex sessions.
Useful? React with 👍 / 👎.
| return createCodexDynamicToolBridge({ | ||
| tools, | ||
| signal: input.signal, | ||
| loading: resolveCodexDynamicToolsLoading(input.pluginConfig), |
There was a problem hiding this comment.
runCodexAppServerSideQuestion() destructures { toolBridge, webSearchPlan } from this helper and immediately reads webSearchPlan.threadConfig, but the helper now returns only the CodexDynamicToolBridge object. After the native-tool flag issue is fixed, Codex /btw still crashes here with webSearchPlan undefined instead of carrying the web-search plan into the forked thread config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d4d07175d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const agentId = resolveSessionAgentIds({ | ||
| sessionKey, | ||
| config: params.config, | ||
| }).sessionAgentId; |
There was a problem hiding this comment.
When a Codex sidecar is owned by an agent-specific legacy store whose session key has not been canonicalized yet (for example agents/<agent>/sessions/sessions.json entries before the core legacy-session migration runs), resolving the owner from sessionKey alone falls back to the default agent. The migration then writes the binding under the default agent’s session-key: row, while runtime lookup uses the actual agent from sessionBindingIdentity, so upgraded non-default-agent sessions lose their existing Codex thread binding and start over instead of resuming it. Carry the agent id associated with the storePath/surface into this owner resolution when the key itself does not encode one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e773981088
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pluginThreadConfigRequired, | ||
| resolvedPluginPolicy, | ||
| enabledPluginConfigKeys, | ||
| pluginAppCacheKey, |
There was a problem hiding this comment.
Every Codex app-server startup reaches this debug payload before the inner startupAttempt() computes pluginAppCacheKey, so evaluating the object throws a ReferenceError before a client is leased or a thread can start. Move this log after the key is computed or omit the field until then.
Useful? React with 👍 / 👎.
| let sandbox = useModelScopedPolicy | ||
| ? modelScopedAppServer.sandbox | ||
| : (binding.sandbox ?? modelScopedAppServer.sandbox); |
There was a problem hiding this comment.
When the Codex /btw side-question module is loaded, this redeclares sandbox in the same block where the sandbox context was already declared above, so the module fails to parse and side questions cannot run. Keep the resolved sandbox context under one name and store the app-server sandbox policy in a separate variable.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
clawsweeper
Bot
added
rating: 🧂 unranked krab
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7641ec8ba7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| agentId, | ||
| sessionKey: event.sessionKey ?? ctx.sessionKey, | ||
| agentId: data.agentId, |
There was a problem hiding this comment.
When a Codex app-server conversation binding has no stored agentId but the inbound claim context does (for example an older/legacy binding claimed under a non-default agent), this newly added shorthand agentId is immediately overwritten by the existing agentId: data.agentId. The native execution/sandbox guard then evaluates the undefined/default agent instead of the actual ctx.agentId, so agent-scoped sandbox or exec-host=node policy can be bypassed for bound Codex conversation turns; pass the resolved agentId only.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: 🎥 video
|
Closing this as superseded: the branch forked from Disposition of the PR's content:
The branch stays available as the design reference. If any piece of the follow-up work turns out to need code lifted verbatim from here, we will cherry-pick from this branch rather than reopen the PR. |
|
The turn-routing and client-runtime-ownership track of this PR has now landed on main, re-implemented on current shapes in #101376 (commit 554d772): the keyed turn router, the client-scoped rate-limit cache with per-limit revisions (cross-client usage-limit bleed fix), the per-physical-client auth/rate-limit runtime handlers, the thread-resume subscription-safety split, and the client-factory deletion. The SQLite session-binding track landed separately in #101210. Still unique to this branch: the native-subagent-monitor status-revision rework and the compact/review native-turn generalization (main has since shipped its own compaction completion-watch design, so that part would need a fresh evaluation). Tracking for the remainder: #101338. |
…02060) Codex native subagent results now survive parent cleanup, app-server persistence races, resumptions, and transient delivery failures. The monitor treats app-server thread/read and thread/turns/list history as canonical recovery state (works on non-stdio transports), retains the shared physical client only while detached children need monitoring, fences stale reads across lifecycle changes, keeps interrupted children resumable, preserves multi-agent V2 subAgentActivity lineage, and bounds terminal delivery retries with a durable failed state. Removes rollout-file discovery, transcript parsing, periodic whole-client rescans, and the deferred one-shot cleanup path. Adds an opt-in live E2E lane (OPENCLAW_LIVE_CODEX_NATIVE_SUBAGENT=1) proving detached delivery and history-only recovery against a real app-server with an OpenAI key, restores the registered-child user-spoof regression test, and registers both codex opt-in live files in the release shard optional-env map. Closes #97593 Related: #93313
…enclaw#102060) Codex native subagent results now survive parent cleanup, app-server persistence races, resumptions, and transient delivery failures. The monitor treats app-server thread/read and thread/turns/list history as canonical recovery state (works on non-stdio transports), retains the shared physical client only while detached children need monitoring, fences stale reads across lifecycle changes, keeps interrupted children resumable, preserves multi-agent V2 subAgentActivity lineage, and bounds terminal delivery retries with a durable failed state. Removes rollout-file discovery, transcript parsing, periodic whole-client rescans, and the deferred one-shot cleanup path. Adds an opt-in live E2E lane (OPENCLAW_LIVE_CODEX_NATIVE_SUBAGENT=1) proving detached delivery and history-only recovery against a real app-server with an OpenAI key, restores the registered-child user-spoof regression test, and registers both codex opt-in live files in the release shard optional-env map. Closes openclaw#97593 Related: openclaw#93313
1 participant
Summary
Codex source checked
../codexat current OpenAI Codex sources and the pinned@openai/[email protected]behavior.codex-rs/core/src/session/turn.rs,codex-rs/protocol/src/openai_models.rs,codex-rs/core/src/codex_delegate.rs,codex-rs/core/src/tasks/review.rs,codex-rs/core/src/session/review.rs,codex-rs/core/src/session/mod.rs, andcodex-rs/app-server/src/request_processors/thread_processor.rs.Verification
Remote isolated Linux dev box:
node scripts/run-vitest.mjs extensions/codex/src/app-server/run-attempt.test.ts✅ 103 testsnode scripts/run-vitest.mjs extensions/codex/src/app-server/startup-binding.test.ts extensions/codex/src/app-server/thread-resume.test.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/run-attempt.test.ts extensions/codex/src/app-server/run-attempt.context-engine.test.ts extensions/codex/src/app-server/compact.test.ts extensions/codex/src/app-server/native-subagent-monitor.test.ts extensions/codex/src/conversation-control.test.ts✅ 293 testsnode scripts/run-vitest.mjs run --config test/vitest/vitest.extension-codex.config.ts extensions/codex✅ 90 files / 1,653 testsnode_modules/oxfmt/bin/oxfmt --check --threads=1 <changed files>✅OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.extensions.json extensions/codex✅ plain oxlintnode scripts/run-tsgo.mjs -p tsconfig.extensions.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions.tsbuildinfo✅Local non-OpenClaw checks:
git diff --check✅node_modules/oxfmt/bin/oxfmt --write --threads=1 ...used for formatter-only rewritesKnown proof gaps / follow-up before merge
origin/maintimed out after ~40 minutes on a ~194k-char bundle.esbuildwhile the configured registry returns 403 without auth. I did not bypass the configured dependency registry/cooldown policy.origin/main; a rebase attempt hit conflicts in Codex config/migration/lifecycle files because main moved across the same surfaces. Resolve that rebase before merge, then rerun the focused Codex suite plus Docker/live Codex lanes.Suggested final E2E after rebase/package auth
OPENCLAW_LIVE_CODEX_HARNESS_AUTH=api-key OPENCLAW_LIVE_CODEX_BIND_MODEL=gpt-5.5 pnpm test:docker:live-codex-bindOPENCLAW_LIVE_CODEX_HARNESS_MODEL=codex/gpt-5.5 pnpm test:docker:live-codex-harnessOPENCLAW_CODEX_NPM_PLUGIN_MODEL=codex/gpt-5.5 pnpm test:docker:live-codex-npm-plugin