refactor(codex): store app-server thread bindings in SQLite plugin state#101210
Conversation
… the binding store
openclaw-barnacle
Bot
added
gateway
|
Codex review: needs changes before merge. Reviewed July 6, 2026, 10:01 PM ET / 02:01 UTC. Summary PR surface: Source +1917, Tests +1795, Docs 0. Total +3712 across 68 files. Reproducibility: yes. by source inspection: a non-default agent with an unscoped/global session key reaches session_end with ctx.agentId resolved from the key as the default agent, so Codex retires the wrong stable binding row. Review metrics: 2 noteworthy metrics.
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:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Keep the SQLite plugin-state binding design, but make session lifecycle hook payloads preserve the explicit owning agent and add regression coverage for non-default/global session keys before merge. Do we have a high-confidence way to reproduce the issue? Yes, by source inspection: a non-default agent with an unscoped/global session key reaches session_end with ctx.agentId resolved from the key as the default agent, so Codex retires the wrong stable binding row. Is this the best way to solve the issue? No, not yet. The storage direction is the right owner-boundary fix, but the mergeable solution must preserve explicit owner agent identity through session lifecycle hook payloads before Codex uses it for binding retirement. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f3106057e62b. Label changesLabel justifications:
Evidence reviewedPR surface: Source +1917, Tests +1795, Docs 0. Total +3712 across 68 files. View PR surface stats
Acceptance criteria:
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
Review history (1 earlier review cycle)
|
clawsweeper
Bot
added
proof: sufficient
openclaw-barnacle
Bot
added
docs
![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: f999d6d03c
ℹ️ 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".
| sessionBindingIdentity({ | ||
| sessionId: event.sessionId, | ||
| ...(sessionKey ? { sessionKey } : {}), | ||
| ...(ctx.agentId ? { agentId: ctx.agentId } : {}), |
There was a problem hiding this comment.
When a non-default agent uses a global session key, gateway session_end emissions carry the real owner as agentId, but the session hook context derives ctx.agentId only from the session key/config, so ctx.agentId falls back to the default agent for keys like global. This retires the wrong session-key:<agent>:<hash> row here, leaving the Codex SQLite binding for the actual agent live after idle/delete/shutdown and allowing stale thread resumes.
Useful? React with 👍 / 👎.
|
Merged via squash.
|
…ate (openclaw#101210) * test(codex): shorten placeholder auth fixture strings * feat(plugin-state): add reject-new overflow policy for keyed namespaces * feat(agents): thread agent identity and session generation through reset and hooks * refactor(codex): add SQLite-backed app-server thread binding store * refactor(codex): move app-server runtime callers onto the binding store * refactor(codex): move conversation, entry, and thread-tool flows onto the binding store * refactor(codex): move command handlers onto the binding store * feat(codex): import legacy binding sidecars via doctor state migration * fix(codex): keep doctor sidecar scan inside stateDir * fix(codex): make binding migration landable * chore(changelog): defer release note * test(plugin-state): stabilize durable-capacity ordering
…ate (openclaw#101210) * test(codex): shorten placeholder auth fixture strings * feat(plugin-state): add reject-new overflow policy for keyed namespaces * feat(agents): thread agent identity and session generation through reset and hooks * refactor(codex): add SQLite-backed app-server thread binding store * refactor(codex): move app-server runtime callers onto the binding store * refactor(codex): move conversation, entry, and thread-tool flows onto the binding store * refactor(codex): move command handlers onto the binding store * feat(codex): import legacy binding sidecars via doctor state migration * fix(codex): keep doctor sidecar scan inside stateDir * fix(codex): make binding migration landable * chore(changelog): defer release note * test(plugin-state): stabilize durable-capacity ordering
1 participant
Related: #93313
What Problem This Solves
Codex app-server thread bindings are stored as JSON sidecar files next to each session transcript (
<sessionFile>.codex-app-server.json), serialized with cross-process file locks that are held across guarded app-server RPCs. This violates the repo's SQLite-only storage rule for runtime state, leaks orphaned sidecar files as sessions rotate, and ties binding ownership to a transcript path instead of the session identity that actually owns the Codex thread.Why This Change Was Made
This ports the SQLite binding-store portion of #93313 onto current main. Bindings now live in the plugin-state keyed store (namespace
app-server-thread-bindingsin the shared state DB) as zod-validated rows keyed by owner identity (session:<agent>:<id>,session-key:<agent>:<sha256>, orconversation:<bindingId>). Row-level leases (token + expiry + heartbeat renewal) replace the file locks; a stored session-generation fence (sessionId + retired tombstones + adopt/retire/reclaim) prevents a stale physical session generation from clearing or reclaiming its successor's binding. A newopenclaw doctor --fixstate migration imports legacy sidecars (schema v1 and v2), records Codex harness ownership on the owning session entry, and archives each imported sidecar as*.migrated; runtime code never reads sidecars. Core additions kept minimal: a plugin-stateoverflowPolicy: "reject-new"option,agentIdon harness reset params and hook/command contexts,previousSessionIdon the after-compaction hook, and doctor env threading. The store schema is grafted onto main's current binding shape (schemaVersion-2 plugin-app policy incl.ask, typed service tier, context-engine state); the PR's unrelated runtime refactor (shared clients, turn routing, native context usage accounting) is intentionally not ported.User Impact
No day-to-day behavior change for Codex sessions. Existing sidecar bindings are imported once by
openclaw doctor --fix(also via auto-migration) and the legacy files are archived, so bound threads keep resuming after upgrade; sidecar files no longer accumulate next to transcripts. Session reset, compaction rotation, and conversation rebinding now fence stale generations through the store instead of relying on file presence.Evidence
tbx_01kwwqk3j24q1n65s1f32485k2):pnpm test extensions/codex— 1,958 tests passed (rebased onto current main, incl. the connected-account app policy variant from Let owner-operated Codex agents use connected account apps #100973 grafted into the store schema); focused batch (plugin-state store, state-migrations, gateway reset cleanup/hooks, auto-reply session, compaction hooks/overflow/timeout, harness registry, hook mappers, plugin commands, session-binding, thread-lifecycle bindings, codex commands, doctor contract) — 8 Vitest shards passed;pnpm buildgreen.git diff --checkall clean.stateDirwhen a session store sits at its exact parent) fixed infix(codex): keep doctor sidecar scan inside stateDirwith a regression test proven to fail pre-fix; one finding (short clear-tombstone TTL on stable keys) rejected as the intended fencing design — recovery is owned by generation ownership checks plusreclaim-generation, and durable fences come fromretireSessionGeneration, now documented inline.codexcheckout:codex-rs/app-server-protocol/src/protocol/v2/thread.rs(ThreadResumeParams.thread_idis the canonical resume handle) andcodex-rs/app-server/src/request_processors/thread_processor.rs(thread_resume_innerrehydrates from Codex's own thread store/rollouts), so binding rows only map session identity tothreadIdand clearing a row never loses Codex-side thread data.doctor-contract-api.test.tsimports a shipped v2 sidecar (incl.destructiveApprovalMode: "ask"), writes session + legacy-conversation rows, recordsagentHarnessId: "codex"on the session entry, and archives the sidecar.