Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Review status
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb255e7393
ℹ️ 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".
| stdin: Readable, | ||
| stdout: Writable | ||
| ): Promise<void> { | ||
| const db = new Database(input.dbPath); |
There was a problem hiding this comment.
When this runtime connects to the live daemon database, any concurrent daemon write can make the immediate claimGuardian update throw SQLITE_BUSY, because this independent connection does not inherit the daemon connection's PRAGMA busy_timeout = 5000 from database-core.ts. The exception bypasses the false-result cleanup path, so the guardian sends no READY and leaves the launch reserved and unowned; configure a busy timeout or retry transient lock failures before attempting the claim.
Useful? React with 👍 / 👎.
|
Closing as stale — open since 2026-09-11 with failing CI, no approval on any head, and no activity in nine days. It also conflicts with #4364 stays open. This closes the attempt, not the work. Nine files is a substantial branch, and it is preserved on One thing worth carrying into whoever picks #4364 up next: the guardian state machine in |
Build a direct SDK-root guardian that survives daemon loss long enough to stop its actual child and record a durable exit receipt. This is additive; no provider, CLI entrypoint, SessionManager adapter, or public start uses it yet.
A random guardian instance must atomically claim a reserved launch before READY. Authorization and terminal receipts require that durable owner. GO reloads committed authorization and the exact running attempt/stop fence. Dedicated control/reply pipes remain separate from SDK stdin/stdout and are not inherited by the SDK.
Parent loss before GO records never_started. After GO, the guardian signals only its actual ChildProcess and waits for root exit. It never kills a numeric process group or infers death from a reused PID. Receipt-write failure retains unknown durable state and retries the same attestation without spawning again; killing the guardian before receipt remains unknown. This preserves the existing SDK-root shutdown guarantee, not arbitrary descendant/container cleanup.
Validation: 22 focused ledger and real-subprocess tests passed; check and independent review passed. Tests SIGKILL the daemon before/after GO, kill the guardian, inject SQLite receipt failures, reject a competing guardian, verify the actual root is gone after receipt, and check stop-fence/identity behavior. Source Bun fixture exercises the resource owner; Deno/compiled entrypoints, stderr/exit-drain forwarding and SDK handle adaptation remain unwired.
Measured200 production/290 test changed lines under300/320 caps. Migration259 adds exclusive guardian ownership to the merged ledger. Runtime/state resources remain owned explicitly; pure guardian decisions reuse the merged protocol gates.
Closes #4364. Stream #4164; restart receipt consumption remains required before public start.