F1: runner lifecycle and state-holder contract (for review)

[mchwang]

Lane F, step F1 (related: #22). Docs only, no code.

The plan requires F1's lifecycle contract to be published and reviewed before implementation. The plan starts F implementation after D5 merges. This PR publishes the contract now, so implementation can start as soon as D5 lands. The contract uses only the D interface already on main (agents/contract.ts).

Adds docs/implementation/runner-lifecycle.md, which covers:

• attempt states (pending → running → completed | failed | cancelled | stale) with guarded legal transitions, and coordinator open → closing
• the owner and release point for each state holder (durable records, in-memory jobs, containers, HTTP requests, UI)
• when a retry is allowed, and compare-and-swap publication
• shutdown order and startup recovery
• the production planning endpoints G4 needs
• the feedback-event contract J needs
• the proposed schema v6
• how the question, E3 suggestion and merge-attempt lifecycles map to this vocabulary
• the AGENTS.md race-regression list, mapped to F1 test cases

Decisions (approved by the user on 2026-09-25):

1. One runner per database: an exclusive lock file. Changed in review round 23 from a PID and start-time file to an OS lock held on a dedicated SQLite lock database, because PID-file takeover isn't race-safe. No lease row.
2. E3 keeps writing cancelled before the provider settles, for read-only phases only. New F records become final only after settlement.
3. Process shutdown is a hard stop. The running task is cancelled with the reason "Stopped by shutdown" and requeued on restart.
4. F sets no time limit of its own on settlement at shutdown and relies on D's kill escalation. D5 must prove that settlement always ends; if it can't, this decision reopens before F1 merges.

The plan's lane-status table is not changed here. It is being edited on docs/reconcile-plan-with-code, and the plan has one documentation owner.

Validation: docs only; git diff --check clean.

Prerequisites before the F1 implementation merges (both owned by D): (1) settlement must be proven to end, which means bounding or proving D4's unbounded cleanup retries; (3) InvocationInput gains runnerOwner, and every D resource is labelled with it; (4) async, abortable, process-group variants of createTaskClone and prepareTaskFilesystems, and an asynchronous D start whose Docker setup runs inside the handle; runnerOwner, attempt ID and a caller-provided allocation ID on every pre-invocation allocator; authenticated recovery handles for kept task storage. Before F2's writable attempts: (5) a bounded task-volume diff export; (2) D must add a recoverLeftovers(runnerOwner) startup recovery API that is scoped to one database by a runner owner label.

Review round 1 (Copilot, on e82e8ca): 5 inline findings, all accepted and fixed in 838a1df. Nothing declined.

1. Slot released after a terminal write was only attempted: now released only after the write succeeds; otherwise an unresolved marker blocks retry until startup recovery.
2. Task time budget wrote cancelled before settlement: now it records the first reason and writes terminal cancelled after settled.
3. The shutdown coordinator barrier was described as existing: it is new. Today an admitted body-reading request can call questions.start() after stopping is set. Step 1 now requires a synchronous closing check in every coordinator.
4. task-closed for a merge cannot share a transaction with GitHub: it is now written with the confirmed merged outcome, and startup reconciliation inserts any missing events idempotently.
5. check/rebase are not D phases: added F attempt kinds mapped onto D's closed Phase union.

Each fix adds a race-regression row where one applies (1, 3, 4). The overview also mentions "8 moderate findings" with no inline locations or failure cases. Per AGENTS.md, I did not patch those blind; a new review round will surface any that are concrete.

Review round 2 (Copilot, on 838a1df): 5 inline findings, all accepted and fixed in 1a7647a. Nothing declined.

1. The task state version was both captured and increased by the attempt's own transitions, so completion could never pass: split into a task state version (ordering and user CAS) and a context generation (the value sent in D's stateVersion).
2. The admission table said in-memory guards were checked inside a SQLite transaction: admission now reserves the slot synchronously, then runs the Store transaction, and releases the reservation on refusal.
3. A "read-only" review screen for a second process would still write (ReviewService.load() calls recordHistory): a second process now exits before opening the Store.
4. The cancel endpoint missed ready suggestions: ready now uses Store.cancelSuggestions.
5. The feedback-event unique key was global: now scoped by plan and task.

Each fix adds a regression row except 4, which reuses the Store's existing behavior.

Review round 3 (Copilot, on 1a7647a): 1 inline finding, accepted and fixed in ffef6ea. A pending attempt that ends before D returns a handle had no settled promise, so its slot could never be freed. Added a "Launch" table with a release path for each case, and recorded D4's start shape as part of D's contract: throw only after cleanup, otherwise return a handle. Regression row added.

Review round 4 (Copilot, on ffef6ea): 2 inline findings, both accepted and fixed in 1402c71.

1. Shutdown claimed D guarantees settlement, but D4 retries cleanup every 1 s with no limit. Proven settlement is now a merge prerequisite for the F1 implementation, and decision 4 is marked conditional.
2. feedbackEvents(taskId) wasn't scoped: taskId is only one part of PlanIdentity. Tasks, attempts, events and user actions are now keyed by the full plan key, and the regression row is updated.

Review round 5 (Copilot, on 1402c71): 7 inline findings, all accepted and fixed in 3b2bc37. Nothing declined.

1. The terminal guards didn't enforce first-reason precedence: they now require a matching or absent first reason.
2. The retry guard ignored the unresolved marker: added.
3. The retry guard omitted plan ID and context generation: it now lists every "Current" field.
4. Shutdown skipped pending (preparing) attempts: they are now cancelled, awaited and terminalized.
5. task-closed reconciliation could close a requeued task: now only for confirmed merges.
6. Recovery didn't tell clean shutdown from a crash: it now handles both and lists them for I3 requeue.
7. The event uniqueness key rejected superseding events: added actionId; uniqueness is now (planKey, kind, actionId).

Three regression rows added (1, 4 and 6, 7).

Review round 6 (Copilot, on 1402c71, before round 5's fixes): 7 inline findings. 6 were fixed in 11a1a41, and 1 was already fixed in 3b2bc37 (shutdown of pending attempts). Nothing declined.

1. Preparation had no cancellation or deadline: it now has an abort signal, awaits its own subprocesses, and counts against the deadline.
2. Shutdown never released the runner lock: added step 8.
3. D has no recovery API for leftovers after a crash: added recoverLeftovers() as a second prerequisite before the F1 implementation merges (owned by D).
4. Recovery ignored the recorded first reason: it now finalizes leftovers by the same mapping.
5. attempts.id wasn't declared unique: now the primary key, with foreign keys.
6. There was no durable place for results: added a bounded result column and stated where larger artifacts live.

Three regression rows added.

Review round 7 (Copilot, on 3b2bc37): 4 inline findings, all accepted and fixed in 5abca29. Nothing declined.

1. There was no path for preparation failing before launch: added one.
2. Retry could reopen closed or human-gated tasks: retry is now gated on task status running or queued.
3. rejected was missing from the closed statuses: added, and excluded from requeue.
4. The current assignment and code hash weren't stored durably: added them to tasks, and every context-changing Store method bumps the generation atomically.

Three regression rows added.

Review round 8 (Copilot, on 5abca29): 1 inline finding, accepted and fixed in 3fb2c65. Recovery by generic io.codeboost.* labels could remove another database's live resources. Recovery now requires a per-database runner owner token, applied to every D resource type, and removes only matching resources. Regression row added.

Review round 9 (Copilot, on 3fb2c65): 13 inline findings, all accepted and fixed in 26cd86b. Nothing declined.

• Launch: a synchronous current-context check runs right before D start, and pending guards and launch errors follow first-reason precedence.
• First reason: kept in memory until the terminal write, which stores it; added a distinct time-limit reason that ends in needs human, including on recovery.
• Shutdown: keeps existing first reasons; the server rechecks stopping before every mutating dispatch, not only merge.
• Recovery: added ownership for partial output and interrupted rebases before the I3 rebuild; the runner token is read after the Store opens under the lock, and a copied database gets a new token.
• Persistence: the merge outcome, tasks.status and task-closed share one transaction; added a composite current-attempt foreign key; a context change bumps both counters.
• Premise: the D prerequisites are now stated in "When implementation starts".

Seven regression rows added.

Review round 10 (Copilot, on 26cd86b): 7 inline findings, all accepted and fixed in 801f8f4. Nothing declined.

• Irreversible actions also re-read review_version.
• The GET handlers that write (/api/review, /api/merge) now honour shutdown.
• Unowned legacy D resources fail closed.
• UI-generated actionId idempotency keys make replays exact.
• Added the v5→v6 task backfill.
• The lock comes from the canonical path and is checked against file identity.
• GET /api/runner exposes the unresolved marker.

Seven regression rows added.

Review round 11 (Copilot, on 801f8f4): 2 inline findings, both accepted and fixed in 8f3820d. Retry and run-again had no idempotency record, and the event key allowed one action ID under two kinds. A single user_actions table now records every writing action once, with its request hash and saved response. Two regression rows added.

Review round 12 (Copilot, on 8f3820d): 8 inline findings, all accepted and fixed in f36d610. Nothing declined.

• Time limits: the task budget during preparation or at launch records time-limit, and an expired attempt deadline fails without calling D.
• A time limit never changes a closed task.
• Shutdown: a Store write gate replaces per-handler checks and covers direct-merge reconciliation after a GitHub await.
• Cancel: "cancel attempt" and "cancel task" are now separate actions.
• Replay: refused outcomes are recorded too, and idempotency-only actions are named.
• Reject: "Reject with feedback" requeues the task instead of closing it, following design step 8. The design's L1 text (line 533) still lists "rejected" as closing a task and should be reconciled by the plan's documentation owner.

Five regression rows added.

Review round 13 (Copilot, on f36d610): 4 inline findings, all accepted and fixed in 7de33c2. Every admission now requires an active task; coordinators' settlement paths hold the write-gate capability; hard-linked databases are refused; and the lock-first bootstrap for a new database is specified. Four regression rows added.

Review round 14 (Copilot, on 7de33c2): 4 inline findings, fixed in 17cef3b.

• Preparation subprocesses now use kill escalation.
• Cancel task is refused during an active merge.
• Merge attempts are excluded from F's generic retry.
• The lock is updated through its descriptor. This finding's failure scenario doesn't hold: the rename is atomic, so O_EXCL still refuses a second runner. I accepted the simpler descriptor approach anyway.

Three regression rows added.

Review round 15 (Copilot, on 17cef3b): 4 inline findings, all accepted and fixed in 0a760ae. Cancel task with an active attempt is now two-phase; the shutdown gate maps to 503 with no view; D- and F-owned processes are separated; and preparation files have a fixed, recoverable attempt directory. Three regression rows added.

Review round 16 (Copilot, on 0a760ae): 7 inline findings, all accepted and fixed in ae44417. Finding 3's premise was partly wrong: D doesn't reclaim volumes at settlement. But F still can't read Docker volumes, so a D export is needed.

• Publication CAS: added the task-open and no-pending-cancel checks.
• Partial output: added a D bounded diff export.
• Preparation: the process group is saved and killed on recovery; the async abortable clone helper is a new D prerequisite.
• Merge initiation now goes through user_actions.
• Bootstrap: added path safety.
• Status: added a stopRequested field.

Five regression rows added.

Review round 17 (Copilot, on ae44417): 5 inline findings, fixed in 505e5d1. Nothing declined. One claim was narrowed rather than "fixed": node:sqlite opens only by path, so a same-user path swap is detected, not prevented, and the doc now says so.

• Preparation: an unknown child now fails closed.
• Cancel task beats a time-limit settlement.
• Startup recovery fails closed if its transaction fails.
• Planning endpoints use the user-action protocol.

Four regression rows added.

Review round 18 (Copilot, on 505e5d1): 7 inline findings, all accepted and fixed in 95fae39.

• Attempt IDs are now path-safe.
• Added an explicit transition for recording a stop reason.
• The merge coordinator's catches rethrow the shutdown error first (confirmed in runner/merge.ts).
• D recovery keeps volumes for export, and startup stops if D recovery fails.
• A no-reason crash on a changed context maps to stale.
• Feedback-producing actions are now enumerated.

Five regression rows added.

Review round 19 (Copilot, on 95fae39): 10 inline findings, all accepted and fixed in 523d363.

• Terminal-state precedence is now deterministic: no reason with a non-current context gives stale before D reasons, and a D stop before shutdown wins.
• pending → running requires no first reason.
• Path-safe runner token.
• runnerOwner on every pre-invocation allocator.
• Authenticated recovered storage handles from D (ownership was a process-local WeakMap).
• Unowned preparation fails closed in startup.
• Deadlines are persisted.
• Pre-open hard-link check.

Seven regression rows added.

Review round 20 (Copilot, on 523d363): 3 inline findings, all accepted and fixed in ad7e909: recovered storage is now bound to its attempt, the parent directory is checked before any lock operation, and the non-current check comes first in recovery. Three regression rows added.

Review round 21 (Copilot, on ad7e909): 1 inline finding, fixed in 65d02bf. Task-storage ownership is now split from D's container ownership, because storage outlives settlement until F exports and removes it. One regression row added.

Review round 22 (Copilot, on 65d02bf): 3 low-severity findings, all fixed in 7924df7. The round-21 holder rule ("remove storage before the terminal write") was stricter than the rest of the contract. I corrected it to: export → terminal write (with diagnostic_ref) → removeTaskFilesystems → free the slot. Publication, recovery and the regressions now follow it. One regression row added.

Review round 23 (Copilot, on 7924df7): 6 inline findings, all accepted and fixed in 3fdd128.

• Async abortable storage allocation is a new D prerequisite (prepareTaskFilesystems is synchronous today).
• The shutdown slot is freed only after storage removal.
• Suggestion cancel goes through retained handles.
• actionId is validated before any transaction.
• Decision 1's lock is now an OS lock (SQLite exclusive locking on a dedicated lock file), because PID-file takeover isn't race-safe.

Five regression rows added.

Review round 24 (Copilot, on 3fdd128): 3 inline findings, all fixed in 16b2d83: a durable requeue claim (requeue_pending, cleared by CAS), no PID diagnostic from the OS lock, and the clean-restart regression updated for the OS lock. One regression row added.

Review round 25 (Copilot, on 16b2d83): 2 findings, both fixed in 621dfb8: a durable rebase marker, and complete v5 backfill defaults (the budget starts at the first move to running). One regression row added.

Review round 26 (Copilot, on 621dfb8): 5 findings, all addressed in a17d980. One was a clarification: the budget did cover preparation, because task status and attempt state are separate.

• Releasing an unknown preparation now requires a verified no-open-files check.
• New D prerequisite: an asynchronous launch (D4's setup is synchronous today).
• F2 extracts a completed attempt's work before storage removal.
• Suggestion cancel after a restart relies on D recovery.

Four regression rows added.

Review round 27 (Copilot, on a17d980): 3 findings, addressed in a31ae05: a machine-wide runner lock for legacy cleanup, and F2's ledger write inside the publication transaction after the guard. The design L1 conflict is now an explicit governance note. It can't be fixed here because the design doc has its own owner. Two regression rows added.

Deferred follow-up for the plan's documentation owner: reconcile docs/designs/codeboost-plan-indexed-review.md:533 (L1 says a task closes when "rejected") with step 8 (Reject with feedback requeues the task).

Review round 28 (Copilot, on a31ae05): 3 findings, addressed in b2e515b.

• codeboost no longer deletes unowned legacy resources itself, because there is no safe proof older runners are gone. It lists them with manual commands and keeps admission closed. The machine-wide lock from round 27 is removed.
• Recovery applies D-reason precedence when a D stop_reason is saved. It also states the limit: a reason lost before settlement costs one bounded re-run.

Regressions updated.

Review round 29 (Copilot, on b2e515b): 2 findings, fixed in 5f4d81a. The completion guard now uses defined statuses, and D recovery keeps the task-storage keeper container. One regression row added.

Review round 30 (Copilot, on 5f4d81a): 3 findings, fixed in c230853.

• Launch: a first-reason refusal settles normally.
• F reserves the allocation ID before async allocation (a new D parameter).
• Recovery: an export phase, then one finalization transaction.

Two regression rows added.

Review round 31 (Copilot, on c230853): no findings; all three round-30 findings are shown as resolved. The overview's one-line summary ("resolve the outstanding lifecycle, recovery… issues") names no location or failure case. Per AGENTS.md, I made no change for it.

Readiness report

• Validated head: 2ee6fb6 (last change: the round-36 fix)
• Base: main at af8f3c2 (D5 merged)
• CI: test passed on 2ee6fb6
• Mergeability: mergeable, merge state clean
• Unresolved review threads: 0 (148 threads, all resolved)
• Latest Copilot review (2ee6fb6): no findings; the round-36 thread is shown as resolved. Copilot reported that it could not review files in this pass, so the last full review was of 748b85a, whose only finding is fixed in 2ee6fb6.
• Files changed: docs/implementation/runner-lifecycle.md only. Docs-only, so no test counts apply; git diff --check is clean.
• Deferred follow-ups :
  1. D prerequisites: tracked in D follow-ups required by the F1 runner lifecycle contract #51. D5 (D5: Add the combined real-Docker isolation gate #50) merged but delivered none of them. The F1 implementation must not merge until D follow-ups required by the F1 runner lifecycle contract #51's pre-F1 items land.
  2. The design owner should reconcile docs/designs/codeboost-plan-indexed-review.md:533 (L1 "rejected" closes a task) with step 8.
  3. The plan's lane-status table should record this PR (owner: the docs/reconcile-plan-with-code branch).

Review round 32 (Copilot, two reviews on 6eb29e5/e44d04a): 4 inline findings, all fixed in 46067fa.

• Recovery stops leftover preparation groups before D recovery and storage.
• The runner lock is now keyed by the database file's inode, so a renamed database meets the same lock.
• The export has a 60-second abortable deadline (D follow-ups required by the F1 runner lifecycle contract #51 updated).
• The recovery time-limit mapping keeps the closed-task and pending-cancel guards.

Four regression rows added.

Review round 33 (Copilot, on 46067fa): 4 inline findings, fixed in 05f4c26.

• The startup order is explicit (2a–2d).
• Publication is skipped with an unsaved in-memory reason.
• Retention never leaves a dangling diagnostic_ref.
• Lock files live in ~/.codeboost/locks, keyed by device and inode, so moves between directories keep the lock.

Two regression rows added.

Review round 34 (Copilot, on d273b30): 1 finding, fixed in d92177e. Read-only attempts also get task storage, so recovery now exports only stopped writable attempts and removes every recovered handle. One regression row added. The overview's "ten unresolved comments" refers to threads that were already replied to and resolved.

Review round 35 (Copilot, on d92177e): 1 finding, fixed in 748b85a. A task's status is frozen while an attempt is active, and completion requires status running. F1a (#53) was updated to match, with a test.

Review round 36 (Copilot, on 748b85a): 1 finding, fixed in 2ee6fb6. Pre-launch cleanup removes only host-side files; task storage always waits for the terminal write.

Review-lesson audit:

• 1 and 2: covered by the existing rules "Do not release a concurrency slot when cancellation is requested…" and "A deadline must abort and await the underlying operation before releasing its in-flight ownership".
• 3: covered by "Set the shutdown admission flag before snapshotting active work, and enforce it again at the irreversible action boundary…".
• 4: covered by "Treat a successful external command as the transition it actually performed" and the rule on ambiguous irreversible outcomes.
• 5: one-off, because the doc didn't match an existing closed type. No new AGENTS.md rule.
• Round 2, finding 1: covered by "Never apply a background response without proving it is still current. Use a generation, attempt ID, version…". It was a design slip in how the generation is chosen, not a missing rule.
• Round 2, finding 2: covered by "Do not release a concurrency slot…" together with the rule on shutdown admission flags. A reservation is the direct application.
• Round 2, finding 3: covered by "When startup acquires a store, process, listener, or other resource before later dependency construction…". The lock now comes before the Store.
• Round 2, finding 4: one-off API omission. No rule.
• Round 4, finding 1: covered by "Reproduce summary-only review concerns or turn them into a concrete follow-up issue" together with the existing decision-4 condition. The doc had stated as fact a guarantee that D has not proven.
• Round 4, finding 2: covered by "Preserve repository identity with pull request numbers in cross-reference scans…" (scope IDs by their owning identity).
• Round 5, findings 1–4 and 6: covered by existing async-job rules ("Preserve the original timeout, cancellation, and shutdown reason through every layer", "Do not let a retry replace a locally active job", "Validate retry context against the current snapshot, plan revision, assignment, and referenced code", "Begin shutdown by rejecting new work… then cancel and await jobs"). The contract hadn't applied them to every case.
• Round 5, finding 5: covered by "Treat a successful external command as the transition it actually performed"; attempt termination is not task closure.
• Round 5, finding 7: one-off data-model slip (idempotency vs. identity). No rule.
• Round 6: 1, 2 and 4 are covered by existing rules ("A deadline must abort and await the underlying operation…", "When startup acquires a store, process, listener, or other resource… close that resource on every construction failure", "Preserve the original timeout, cancellation, and shutdown reason through every layer"). 3 is a missing D capability, now tracked as a prerequisite. 5 and 6 are one-off schema omissions. No new rule.
• Round 7: 1 and 4 are covered by "Validate retry context against the current snapshot, plan revision, assignment, and referenced code" and "Do not release a concurrency slot…"; the contract lacked durable holders for them. 2 and 3 are one-off status-model omissions. No new rule.
• Round 8: covered by "Exclude the subject of a duplicate or supersession check by stable identity only. A shared branch name or other mutable attribute does not prove two records are the same subject." A generic label doesn't prove ownership. No new rule.
• Round 9: covered by existing rules ("Preserve the original timeout, cancellation, and shutdown reason through every layer"; "Set the shutdown admission flag before snapshotting active work, and enforce it again at the irreversible action boundary for requests admitted before shutdown began"; "After the final asynchronous external validation, re-read the local generation immediately before an irreversible action"; "Exclude the subject… by stable identity only"). Composite-key and partial-output ownership are one-off contract omissions. No new rule.
• Round 10: covered by existing rules ("After the final asynchronous external validation, re-read the local generation…", "Set the shutdown admission flag… enforce it again…", "Missing or malformed identities must fail closed", "Exclude the subject… by stable identity only"). The idempotency key, backfill and marker field are one-off contract omissions. No new rule.
• Round 11: one-off gap in the idempotency design introduced in round 10. No new rule.
• Round 12: covered by existing rules ("Set the shutdown admission flag… enforce it again at the irreversible action boundary", "Preserve the original timeout, cancellation, and shutdown reason", "A deadline must abort and await the underlying operation"). The reject status was a misreading of the design. Candidate new rule, not added: enforce shutdown write refusal at the storage boundary rather than per handler. Not added because the Store gate isn't implemented yet; propose it with the F1 implementation.
• Round 13: covered by existing rules ("Begin shutdown by rejecting new work… then cancel and await jobs, then close storage", "Exclude the subject… by stable identity only", "When startup acquires a store… close that resource on every construction failure"). No new rule.
• Round 14: covered by existing rules ("An abort error fires, then subprocess close arrives later"; "When an irreversible command has an ambiguous… outcome, retain durable in-flight ownership"; "When a durable external-action attempt is bound to an older snapshot, require approvals… before another action"). No new rule.
• Round 15: covered by existing rules ("Do not release a concurrency slot… keep the job tracked until… terminated", "Preserve the original… shutdown reason… Do not replace actionable errors with generic… text", "When startup acquires a… resource… close that resource on every construction failure"). No new rule.
• Round 16: covered by existing rules ("Track an in-flight irreversible subprocess as part of server shutdown. Abort it, await its settlement"; "An abort error fires, then subprocess close arrives later"; "When an irreversible command has an ambiguous… outcome, retain durable in-flight ownership"). Path safety and the status overlay are one-off. No new rule.
• Round 17: covered by existing rules ("A bounded safety scan must fail closed"; "When startup acquires a store… close that resource on every construction failure"). The narrowed lock claim follows "Reproduce summary-only review concerns or turn them into a concrete follow-up": the limit is now stated, not hidden. No new rule.
• Round 18: covered by existing rules ("Preserve the original… shutdown reason through every layer. Do not replace actionable errors with generic… text"; "A bounded safety scan must fail closed"; "Validate retry context against the current snapshot…"). Path-safe IDs and the explicit transition are one-off. No new rule.
• Round 19: covered by existing rules ("Preserve the original timeout, cancellation, and shutdown reason through every layer"; "Validate coupled lifecycle fields as allowed combinations"; "Missing or malformed identities must fail closed"). No new rule.
• Round 20: covered by existing rules ("Exclude the subject… by stable identity only"; "Validate coupled lifecycle fields as allowed combinations"). No new rule.
• Round 21: one-off holder-table inconsistency. No new rule.
• Round 22: a consistency fix to my own round-21 change. No new rule.
• Round 23: covered by existing rules ("Missing or malformed identities must fail closed"; "Do not release a concurrency slot…"; "An abort error fires, then subprocess close arrives later"). No new rule.
• Round 24: covered by "Do not let a retry replace a locally active job…". The requeue claim applies it across restarts. The rest are follow-ups to round 23. No new rule.
• Round 25: one-off schema omissions. No new rule.
• Round 26: covered by existing rules ("A bounded safety scan must fail closed"; "Track an in-flight irreversible subprocess as part of server shutdown"). No new rule.
• Round 27: covered by existing rules ("A bounded safety scan must fail closed"; "Exclude the subject… by stable identity only"). The design conflict is recorded as a follow-up. No new rule.
• Round 28: covered by "A bounded safety scan must fail closed. Never truncate evidence and report the result as clear". An unprovable check now leads to manual action, not automation. No new rule.
• Round 29: one-off wording and ownership consistency fixes. No new rule.
• Round 30: covered by existing rules ("Do not release a concurrency slot…"; "When startup acquires a… resource… close that resource on every construction failure"). No new rule.
• Round 32: covered by existing rules ("A deadline must abort and await the underlying operation"; "Exclude the subject… by stable identity only"). No new rule.
• Round 33: covered by existing rules ("Preserve the original… cancellation… reason through every layer"; "Exclude the subject… by stable identity only"). No new rule.
• Round 34: one-off scope slip in the export rule. No new rule.
• Round 35: covered by "Validate coupled lifecycle fields as allowed combinations". No new rule.
• Round 36: consistency with the round-22 storage order. No new rule.
• Round 3: covered by "Do not release a concurrency slot when cancellation is requested. Keep the job tracked until its underlying invocation or subprocess has terminated." The gap was a missing case in the contract (no invocation yet), not a missing rule.
• Round 2, finding 5: covered by "Preserve repository identity with pull request numbers in cross-reference scans…" (scope identifiers by their owner).

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical and moderate contract issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 1

Open (5)

• Releasing jobs before terminal write success permits unsafe retries · New
• Cancellation must become terminal only after settlement · New
• Shutdown lacks an immediate coordinator admission barrier · New
• External merge cannot share a transaction with its event · New
• Invocation phase union cannot represent documented attempt phases · New

What changed in this PR

This docs-only PR publishes the F1 runner lifecycle and state-holder contract ahead of implementation.

Changes:

• Defines attempt states, ownership, retries, shutdown, and recovery.
• Specifies planning APIs, feedback events, and schema v6.
• Maps existing lifecycles and required race regressions.

File	Description	Final review findings
docs/​implementation/​runner-lifecycle.md	Adds the F1 lifecycle and state-holder contract.	5 critical and 8 moderate findings remain, covering interface mismatches, shutdown/recovery races, settlement ownership, endpoint behavior, event consistency, identity scoping, and regression-test feasibility.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved lifecycle, concurrency, recovery, API, and schema inconsistencies must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 2

Open (5)

• Immutable invocation version conflicts with lifecycle state increments · New
• Separate in-memory reservations from transactional guards · New
• Enforce read-only mode when another runner holds the lock · New
• Support cancellation of ready suggestions · New
• Scope event uniqueness by stable task or plan identity · New

Resolved since last review (5)

• External merge cannot share a transaction with its event
• Shutdown lacks an immediate coordinator admission barrier
• Cancellation must become terminal only after settlement
• Releasing jobs before terminal write success permits unsafe retries
• Invocation phase union cannot represent documented attempt phases

[Copilot]

Copilot review overview

🟡 Changes recommended

The contract has unresolved lifecycle, recovery, concurrency, API, and persistence issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Pending attempts can permanently hold global slots · New

Resolved since last review (5)

• Enforce read-only mode when another runner holds the lock
• Separate in-memory reservations from transactional guards
• Immutable invocation version conflicts with lifecycle state increments
• Scope event uniqueness by stable task or plan identity
• Support cancellation of ready suggestions

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate contract issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Make settlement termination a prerequisite, not an assumption · New
• Scope event reads by full plan and task identity · New

Resolved since last review (1)

• Pending attempts can permanently hold global slots

Previously missed (2)

In code that hasn't changed since last review

Guard terminal reasons against precedence and context changes

docs/​implementation/​runner-lifecycle.md:105

The failed and stale rows do not guard against an existing first reason or a context change, so an implementation can record failed after a stale/cancel request and override the first-reason precedence in the table above. Require failed to have no first reason and a current context, and make stale apply only to a recorded stale reason or an otherwise unreasoned context change; otherwise the terminal classification is nondeterministic.

This issue also appears on line 192 of the same file.

Separate replay identity from mutable assignment choices

docs/​implementation/​runner-lifecycle.md:249

The append-only and uniqueness rules contradict each other for mutable choices: sourceRef is the choice key, so changing an assignment from P1 to P2 within the same plan revision must emit a new segment-assign event that supersedes the old one, but the unique key rejects that second event. Use a separate stable action/idempotency identity for replay deduplication, or include the choice value/version in the key while preserving sourceRef for supersession lookup.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate contract issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 7

Open (7)

• Transition guards ignore first-reason precedence · New
• Retry guard ignores unresolved durable-outcome marker · New
• Retry table omits plan ID and context generation · New
• Shutdown omits pending preparation jobs · New
• Recovery may close tasks before requeueing shutdown attempts · New
• Recovery does not requeue clean-shutdown cancellations · New
• Uniqueness key blocks superseding choice events · New

Resolved since last review (2)

• Scope event reads by full plan and task identity
• Make settlement termination a prerequisite, not an assumption

Previously missed (1)

In code that hasn't changed since last review

Feedback events API lacks plan identity

docs/​implementation/​runner-lifecycle.md:265

The event table and uniqueness rule scope records by planKey and taskId, but the only reader API named here is Store.feedbackEvents(taskId). A bare task ID cannot identify the plan/repository whose events should be returned, so J could mix events from different plans or read the wrong task. Require the stable plan identity in this API and query both identity and task ID.

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical lifecycle, recovery, shutdown, and persistence contract issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 7

Open (7)

• Unbounded preparation promise can hold attempts indefinitely · New
• Shutdown skips pending jobs holding preparation slots · New
• Shutdown omits releasing the exclusive runner lock · New
• Missing D recovery API prevents post-crash resource cleanup · New
• Recovery overwrites recorded cancellation and stale reasons · New
• Attempt ID uniqueness is not enforced in schema · New
• Attempt schema lacks durable result storage · New

Resolved since last review (7)

• Uniqueness key blocks superseding choice events
• Recovery does not requeue clean-shutdown cancellations
• Recovery may close tasks before requeueing shutdown attempts
• Shutdown omits pending preparation jobs
• Retry table omits plan ID and context generation
• Retry guard ignores unresolved durable-outcome marker
• Transition guards ignore first-reason precedence

Previously missed (1)

In code that hasn't changed since last review

Retry guard omits unresolved markers and context fields

docs/​implementation/​runner-lifecycle.md:167

The retry checklist is weaker than the surrounding contract: it omits unresolved markers (which line 156 says must block retry) and omits planId and contextGeneration even though Current requires every captured-context field to match. An implementation following this table could admit a retry after a failed terminal write or with a mismatched context; include the marker and the full context in the Store guard.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Resolve the outstanding lifecycle, recovery, deadline, storage-bound, and API-contract issues.

Review effort: Lite
Findings: None

Resolved since last review (3)

• Diagnostic export cannot occur inside SQLite transaction
• Async preparation can lose durable allocation identity
• Stop during preparation can leave pending rows blocked

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate lifecycle, recovery, locking, and contract issues remain.

Review effort: Lite
Findings: 2

Open (2)

• Terminate preparation groups before recovering task storage · New
• Use inode-stable locking across database renames · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Two critical and six additional unresolved findings remain.

Review effort: Lite
Findings: 4

Open (4)

• Bound export operations and continue terminalization on failure · New
• Apply closed-task and pending-cancel guards during recovery · New
• Use inode-stable locking across database renames
• Terminate preparation groups before recovering task storage

Previously missed (2)

In code that hasn't changed since last review

Pass a caller-generated allocation ID to network creation

docs/​implementation/​runner-lifecycle.md:250

The prerequisite requires a caller-provided allocation ID for every pre-invocation allocator, including vendor-network creation, but this contract only defines and persists F's ID for task-storage allocation; the network API remains implicitly D-generated. Without an F-supplied ID there is no durable attempt-to-network correlation for crash recovery. Define one F-generated ID (or a separately persisted network ID) and pass it to the network allocator as well.

Capture preparation diagnostics before finalizing recovery

docs/​implementation/​runner-lifecycle.md:279

Recovery finalizes the attempt and persists diagnostic_ref in step 3 before this step kills preparation processes and captures their directory. If a clone/preparation process is still writing when the runner crashes, the diagnostic produced here has no subsequent transaction that records its reference, so it can become an unreferenced file and be removed by retention. Capture the preparation diagnostic before finalization, or add a guarded post-capture write that persists its reference before admission opens.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate lifecycle, locking, recovery, retention, and idempotency issues remain.

Review effort: Lite
Findings: 3 · 1

Open (4)

• Publication ignores in-memory cancellation reason · New
• Retention deletes files referenced by durable attempts · New
• Lock path breaks across database directory renames · New
• Startup sequence has ambiguous Store and recovery ordering · New

Resolved since last review (4)

• Apply closed-task and pending-cancel guards during recovery
• Bound export operations and continue terminalization on failure
• Use inode-stable locking across database renames
• Terminate preparation groups before recovering task storage

[Copilot]

Copilot review overview

🟡 Changes recommended

Ten unresolved review comments remain, including eight moderate-severity issues.

Review effort: Lite
Findings: 1

Open (1)

• Filter partial exports to stopped writable attempts · New

Resolved since last review (4)

• Lock path breaks across database directory renames
• Retention deletes files referenced by durable attempts
• Publication ignores in-memory cancellation reason
• Startup sequence has ambiguous Store and recovery ordering

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate contract inconsistencies remain.

Review effort: Lite
Findings: 1

Open (1)

• Settlement can complete tasks awaiting human action · New

Resolved since last review (1)

• Filter partial exports to stopped writable attempts

Previously missed (1)

In code that hasn't changed since last review

Lock file location is inconsistently specified

docs/​implementation/​runner-lifecycle.md:530

The decision summary says the lock file is next to the database, but the approved algorithm below stores it under ~/.codeboost/locks and keys it by device/inode (line 534). Keeping both descriptions makes the lock location ambiguous and could lead an implementation to derive the path from the database directory, defeating the stated rename/move behavior.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved moderate and critical contract inconsistencies must be addressed before approval.

Review effort: Lite
Findings: 1

Open (1)

• Retain task storage until terminal state is durable · New

Resolved since last review (1)

• Settlement can complete tasks awaiting human action

[Copilot]

🔵 Needs a closer look

Resolve the documented contract ambiguities and inconsistencies before approval.

Copilot wasn't able to review any files in this pull request. Check if the Files changed in this pull request are included in default exclusions.

Resolved since last review (1)

• ✅ docs/implementation/runner-lifecycle.md#L137 — Resolved by @​mchwang - These pre-launch paths say to remove “anything it created” before the terminal write. Task storage…