Skip to content

Complete merge-queue lifecycle and retry guards - #46

Merged
mchwang merged 21 commits into
mainfrom
codex/merge-queue-k2-k3
Sep 25, 2026
Merged

mchwang merged 21 commits into
mainfrom
codex/merge-queue-k2-k3

Conversation

@mchwang

@mchwang mchwang commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Closes #24.

Outcome

Completes lane K2/K3 on baseline 13a7b308011c1ea5ef57ec974c5c338584538ec2:

  • persists submitting -> queued -> merged | removed | failed attempts against the exact plan revision, snapshot, review version, and reviewed head;
  • distinguishes confirmed GitHub refusal from ambiguous cancellation, timeout, transport, or unknown submission outcomes, preserving durable diagnostics;
  • captures a stable pre-enqueue GitHub timeline cursor and accepts exactly one correlated post-cursor enqueue sequence;
  • performs a final fresh remote validation after queue correlation and rechecks the local review generation before durable ownership and command execution;
  • polls queue-backed attempts through a narrow endpoint with bounded exponential browser backoff while direct ambiguous attempts reconcile on full refresh;
  • keeps retry disabled until a full refresh revalidates current remote blockers;
  • requires current-snapshot and current-plan-revision approvals after replacement or amendment;
  • bounds the entire merge operation and shutdown drain, including partial request bodies, while preserving committed-action responses;
  • guards stale poll publication by attempt identity, legal-state compare-and-swap, timeline cursor, and browser generation;
  • preserves UI selection, drafts, attachments, navigation, and durable queue state during narrow polling.

Lifecycle/state-holder and interface details are recorded in issue #24 and docs/implementation/merge-queue.md. Queue support remains fail-closed for adapters that do not implement the K1 observation contract.

Validation

Validated head: 3ebac31eea990ce9d0f23711a1269d67dfec62c9 (exact pushed head).

  • Local exact-head npm test: 400 tests across 16 files passed.
  • Local exact-head npm run typecheck: passed.
  • Local exact-head npm run test:browser: 53 tests passed.
  • GitHub CI run 36084502248: passed.
  • GitHub CI run 36084504959: passed.

Controlled regressions cover ambiguous direct and queue-backed outcomes, durable restart recovery, exact attempt/event correlation, ten-page history boundaries, queue-mode changes, replacement-head and restored-SHA re-review, same-snapshot plan amendments, stale browser publication, queue-only bounded polling, retry revalidation, committed-result fallbacks, admitted-request draining, bounded status shutdown, partial-body shutdown, and UI input preservation.

Review rounds

  • Self-review rounds 1-2: fixed exact-head observation validation and stale poll invalidation; second pass found no new issues.
  • Copilot rounds 1-4: fixed replacement-review gating, queue-mode stability, ambiguous enqueue outcomes, stable cursor correlation, restored-SHA review, and the post-cursor validation window.
  • Copilot rounds 5-7: fixed stale UI publication, bounded cursor pagination, exact enqueue-sequence correlation, and generalized current-snapshot approval gating.
  • Copilot round 8: reproduced and fixed unbounded browser polling and stale retry readiness in 3e32a22.
  • Copilot round 9 summary concerns: reproduced and fixed one overall request deadline, admitted-request draining, and durable ambiguous-submission diagnostics in 4710da7; stabilized the existing real-Git test budget in ec2738d.
  • Copilot round 10: fixed durable post-command queue publication in d538d4f, then current-plan approval gating, bounded shutdown drain, and committed-result fallback coverage in 61f5039.
  • Copilot round 11: no inline findings; summary-only categories were captured for bounded investigation in Investigate summary-only merge lifecycle concerns #48.
  • Copilot round 12: fixed post-attempt fresh-review evidence, same-snapshot plan-amendment UI state, and API shutdown admission in f741d16 and 7c80774.
  • Copilot round 13: fixed durable ownership for ambiguous direct merges and bounded request-scoped status inspection in c43a073.
  • Copilot round 14: fixed partial-body shutdown ownership and preserved active polling across transient observation failures in 5fe030d; preserved diagnostic 409 responses for downstream work in 877fa58.
  • Copilot round 15: stopped queue polling for direct ambiguous attempts in e551f73; declined the proposed pagination rewrite because the loop already consumes page ten, and added a regression proving page-ten consumption and fail-closed overflow.
  • Copilot round 16: confirmed queue-entry identity already precedes UNMERGEABLE classification and added an adversarial other-head fixture in 3ebac31; the follow-up review returned no new inline findings.
  • Declined findings: the page-ten off-by-one report and the UNMERGEABLE ordering report were inconsistent with the reviewed code; both received explicit regression coverage.
  • Deferred follow-up: Investigate summary-only merge lifecycle concerns #48 (remaining summary-only concerns; non-blocking unless reproduced).

Review-lesson audit

  • Existing AGENTS.md rules cover current plan/snapshot retry context, stale publication, cache invalidation, committed-action response handling, reason preservation, controlled race reproduction, partial-request shutdown, and fail-closed external record validation.
  • New rules in this branch cover replacement-generation evidence, lifecycle-mode stability, ambiguous irreversible-command outcomes, stable event-boundary correlation, bounded external polling, and bounded shutdown draining.
  • One-off findings: pagination post-increment interpretation; covered by a focused boundary regression rather than a new standing rule.

Current head is mergeable, both exact-head CI runs are green, unresolved inline review threads are 0, and the authorized squash merge is ready.

Copilot AI lite review requested due to automatic review settings September 24, 2026 18:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved fresh-review, queue identity, retry, and shutdown race conditions can leave state inconsistent or permit unsafe merges.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Completes durable merge-queue lifecycle tracking, guarded retries, queue polling, shutdown handling, and UI state preservation.

Changes:

  • Persists merge attempts and terminal outcomes.
  • Adds queue observation and stale-response protection.
  • Updates UI behavior and regression coverage.
File Summary
web/​server.ts Exposes queue polling endpoints.
web/​public/​app.js Renders and polls merge-queue state.
test/​store.test.ts Tests persistence and stale transitions.
test/​merge.test.ts Tests lifecycle and concurrency behavior.
test/​browser/​review.spec.ts Tests queue UI workflows.
runner/​store.ts Persists merge-attempt lifecycle state.
runner/​merge.ts Coordinates enqueue, polling, retries, and shutdown.
github/​merge.ts Observes GitHub merge-queue state.
docs/​implementation/​merge-queue.md Documents lifecycle and runtime behavior.

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

Comment thread runner/merge.ts Outdated
Comment thread runner/merge.ts
Copilot AI review requested due to automatic review settings September 24, 2026 19:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Seven moderate review findings remain unresolved.

Review effort: Lite
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve submitting state for ambiguous enqueue outcomes

runner/​merge.ts:155

This catch marks every enqueue-side error, including cancellation or timeout, as a terminal failed attempt and makes it retryable. If gh pr merge has already been accepted by GitHub when the local process is aborted (especially during close()), the existing queue entry is no longer polled; after restart the UI can offer a second enqueue for the same head. Preserve the submitting record for ambiguous command outcomes and recover it through queue inspection, only recording failed when the adapter can establish that submission was refused before commit.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical enqueue/retry correlation issues and additional lifecycle race and shutdown issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity

Open (3)

Comment thread runner/merge.ts Outdated
Comment thread runner/merge.ts
Comment thread runner/merge.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 19:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved moderate findings affect retry correctness, failure handling, stale UI state, and queue polling load.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (1)

Copilot AI review requested due to automatic review settings September 24, 2026 19:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Address the retry-attempt timestamp boundary and classify reviewed-head mismatches as terminal fresh-review outcomes.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread github/merge.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 19:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate lifecycle, review-gating, retry, and polling issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread runner/merge.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 19:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Five unresolved review findings remain, including four moderate issues.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Stale polling response can overwrite newer review state

web/​public/​app.js:140

This poll is guarded only by mergeGeneration, but review actions in act() update data and reviewGeneration without incrementing mergeGeneration. If a note/assignment completes while this request is in flight, the older response can overwrite the current merge queue status (for example, re-enable retry using the pre-action review version). Capture the current review token/generation when polling starts and discard the response if it changed.

Copilot AI review requested due to automatic review settings September 24, 2026 23:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Four moderate findings remain unresolved, including lost abort diagnostics, incorrect submitting-state UI, and incomplete shutdown cancellation.

Review effort: Lite
Findings: 2 High severity

Open (2)

Copilot AI review requested due to automatic review settings September 25, 2026 01:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved polling and cache-freshness issues remain, and partial request bodies can bypass the shutdown deadline.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (5)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Queue inspection failure stops polling active attempts

runner/​merge.ts:230

On a non-abort queue-inspection failure this passes undefined explicitly to #queueStatus, so it returns null instead of the still-active durable attempt. The browser then replaces merge.queue with null and scheduleMergePoll() stops polling, even though SQLite remains in submitting/queued; a timeout or malformed GitHub response can therefore strand reconciliation in the UI. Return the current attempt with the observation error so polling remains active and the diagnostic is surfaced.

Comment thread web/server.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Four unresolved moderate findings affect abort diagnostics, direct-attempt polling, shutdown draining, and fresh retry validation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread runner/merge.ts
Copilot AI review requested due to automatic review settings September 25, 2026 01:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Four unresolved findings remain, including one critical pagination issue and three moderate issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Response exposes stale merge-enabled status

web/​server.ts:58

The response exposes merged.status, which is the pre-command status and still has ready: true and action: 'merge'. The browser masks this locally, but any other API client (or a second tab) can interpret the committed response as permission to submit the same merge again. Return a post-command status with the action disabled, while retaining the durable queue snapshot.

Comment thread github/merge.ts
Copilot AI review requested due to automatic review settings September 25, 2026 01:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Four moderate unresolved findings affect durable diagnostics, direct submission reconciliation, and merge status reporting.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)

Copilot AI review requested due to automatic review settings September 25, 2026 01:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate review findings remain in merge validation and lifecycle coordination.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread github/merge.ts
Copilot AI review requested due to automatic review settings September 25, 2026 02:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Three unresolved moderate findings affect durable diagnostics and merge-queue UI/state correctness.

Review effort: Lite
Findings: None

Resolved since last review (1)

@mchwang
mchwang merged commit 62ad463 into main Sep 25, 2026
3 checks passed
@mchwang
mchwang deleted the codex/merge-queue-k2-k3 branch September 25, 2026 02:10
mchwang added a commit that referenced this pull request Sep 26, 2026
The guarded merge gate doc still said merge-queue branches stay blocked.
#46 (closing #24) added queue lifecycle support. Point to merge-queue.md,
and state that adapters without queue inspection still fail closed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mchwang added a commit that referenced this pull request Sep 26, 2026
* docs: reconcile the design plan with the code

Record where the code differs from the approved plan and update stale status:

- Record Ask as an interim exception to R1: it runs the vendor CLI on the
  host with tools off until lane F moves it into the lane D container.
- Amend D20: there is no "Merge anyway"; to override a blocker, merge on
  GitHub. Matches docs/implementation/guarded-merge.md.
- Tick T1, T2, T4, T5, T10, T13, T14 with test evidence; point Files lines
  at core/linking.ts and core/approvals.ts instead of never-created modules.
- Mark increment 1 merged; add a lane status table (C, D, K done; E, F, H
  progress); record decided open questions (issue ranking, AgentDiff).
- Add a verified status note for design tasks DT2-DT15; none newly ticked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: note merge-queue support in the guarded merge doc

The guarded merge gate doc still said merge-queue branches stay blocked.
#46 (closing #24) added queue lifecycle support. Point to merge-queue.md,
and state that adapters without queue inspection still fail closed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: bring plan status up to date with F1 and H4a

Merge main. Record the F1 lifecycle contract (#49) and open F1a-F1c
(#53, #56, #57); record the ranked Issues screen (H4a, #55) with H4b's
trust action remaining; Issues is now a menu link, not a placeholder.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: add F1d and Ask PRs to lane status

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Align merge-queue wording with the merged K2/K3 support

README no longer says merge queues block merging; it describes the
enqueue-then-confirm behaviour. The plan's wave-3 note records the old
block as history instead of a live instruction.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Record the K-lane queue block as history in the task table

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* README: disclose that Ask runs the agent CLI on the host

The plan (R1 exception) says README states this limit; it did not.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Add F1e (#60) and the #51 merge condition to the F lane row

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* README: distinguish queue-removal retry from changed-head review

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Plan: mark the install preflight and npx entry as planned

The CLI checks only the Node version today; say so instead of describing
the git/gh/container/sign-in preflight as current.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
mchwang added a commit that referenced this pull request Sep 27, 2026
* docs: reconcile the design plan with the code

Record where the code differs from the approved plan and update stale status:

- Record Ask as an interim exception to R1: it runs the vendor CLI on the
  host with tools off until lane F moves it into the lane D container.
- Amend D20: there is no "Merge anyway"; to override a blocker, merge on
  GitHub. Matches docs/implementation/guarded-merge.md.
- Tick T1, T2, T4, T5, T10, T13, T14 with test evidence; point Files lines
  at core/linking.ts and core/approvals.ts instead of never-created modules.
- Mark increment 1 merged; add a lane status table (C, D, K done; E, F, H
  progress); record decided open questions (issue ranking, AgentDiff).
- Add a verified status note for design tasks DT2-DT15; none newly ticked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: note merge-queue support in the guarded merge doc

The guarded merge gate doc still said merge-queue branches stay blocked.
#46 (closing #24) added queue lifecycle support. Point to merge-queue.md,
and state that adapters without queue inspection still fail closed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Run Ask in the lane D agent container

Ask used to run the claude/codex CLI on the host with each CLI's own
restrictions, an interim exception to R1. It now uses lane D's invocation
boundary in the read-only "questions" phase: a clone of the reviewed
snapshot head at /work, no commands, vendor-only network, and no other
host files. There is no host fallback.

- runner/question-container.ts: build image, clone, allocate bounded
  storage, capture, start the Claude/Codex adapter; release storage only
  after the invocation settles. Deps are injectable for unit tests.
- runner/question-worker.ts: lane D setup is synchronous, so a worker
  thread owns it and the review server stays responsive.
- runner/question-agent.ts: QuestionWorker bridge; a question settles only
  when the worker reports the container and storage are gone.
- Credentials come from the environment only: CLAUDE_CODE_OAUTH_TOKEN for
  Claude, CODEBOOST_CODEX_AUTH_FILE or CODEX_HOME/auth.json for Codex.
- Provider failures include the vendor's short message (e.g. a 401).
- test/agent-question.test.ts runs the path on real Docker (Agent
  isolation workflow); its live case needs the auth-probe credentials.
- Plan, README, Settings copy and implementation docs updated; the R1
  exception is closed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: bring plan status up to date with F1 and H4a

Merge main. Record the F1 lifecycle contract (#49) and open F1a-F1c
(#53, #56, #57); record the ranked Issues screen (H4a, #55) with H4b's
trust action remaining; Issues is now a menu link, not a placeholder.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: add F1d and Ask PRs to lane status

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Align merge-queue wording with the merged K2/K3 support

README no longer says merge queues block merging; it describes the
enqueue-then-confirm behaviour. The plan's wave-3 note records the old
block as history instead of a live instruction.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Record the K-lane queue block as history in the task table

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* README: disclose that Ask runs the agent CLI on the host

The plan (R1 exception) says README states this limit; it did not.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Add F1e (#60) and the #51 merge condition to the F lane row

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Bind Ask answers to their attempt and keep cleanup ownership

- Reuse the persisted answer attempt as the invocation attempt, and the
  note's contextId as referencedCodeHash. Accept a result only when its
  attempt and context match the captured invocation and the worker reply
  carries the same attempt.
- Treat a missing exit code or any signal as a failure, not an answer.
- Keep task storage whose removal Docker did not confirm, retry removal
  before the next question, and refuse Ask while any remains.
- After a worker crash, fail closed instead of starting a replacement:
  its containers and storage may still exist, and reclaiming them needs
  lane D's scoped recovery (#51 item 4).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Run the Docker Ask suite when runner/questions.ts changes

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Record Ask storage left at shutdown and keep Ask off until it is gone

Terminating the question worker dropped its only handles to storage
that Docker had not removed. Shutdown now asks the worker for one last
bounded removal, records anything still unremoved beside the review
database, and the next session refuses Ask, with the removal commands,
while any recorded container or volume still exists. The record clears
itself once they are gone; an unreadable record or unreachable daemon
keeps Ask off. Removal through D waits for its recovery handles (#51).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Keep Ask off after a setup failure that leaves unidentifiable storage

When task storage setup fails and lane D cannot confirm its own cleanup,
D returns no handle, so Ask cannot name the leftovers. Ask now counts
the failure, stays off for the session, records it at shutdown, and
after a restart stays off while any io.codeboost.task-storage container
or volume exists. Caller-provided allocation IDs (#51 item 3) would let
Ask name these resources instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Make the Ask leftover gate bounded and fail closed on unknown state

- A worker crash, or no release report at shutdown, is recorded at once
  as unidentified leftovers instead of an empty, clean release.
- The pre-question check is two label queries (docker ps, docker volume
  ls) under one 15-second limit that the question's signal can cancel,
  instead of up to 300 sequential inspects.
- Entries beyond the record's cap become unidentified leftovers; none
  are dropped.
- Removal commands list only resources that still exist, so a missing
  keeper no longer blocks volume removal.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Bound Ask against unsettled lane D cleanup and scan all D labels

- Scan containers, volumes and networks for every label lane D applies
  (allocation, invocation, egress), so a leftover seeder or proxy keeps
  Ask off.
- The first question of each process scans even without a record, so a
  process killed before writing one cannot bypass the gate.
- A question not settled 30 s after its deadline, or still settling
  after a 20 s shutdown grace, abandons the worker: unknown leftovers are
  recorded, waiters rejected and the worker stopped, so D's unbounded
  cleanup retries (#51 item 1) cannot hang Ask or shutdown.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Own the host staging directory like the Docker allocation

If the host copy of the reviewed code cannot be deleted, the worker now
keeps its path and retries before the next question, shutdown records
it, and the next leftover check deletes it. Ask stays off while any copy
remains. The record accepts only codeboost-question-* staging paths.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Keep credentials out of setup subprocesses and settle abandon in order

- The question worker snapshots credentials for the adapters and removes
  credential-like variables from its own environment, so the image
  build, clone and other setup subprocesses cannot inherit them. Leftover
  Docker queries use lane D's minimal PATH/DOCKER_HOST environment.
- Missing sign-in is reported before the leftover scan or any Docker work.
- Abandoning a worker records unknown leftovers, then waits (bounded)
  for the thread to stop before rejecting its questions, so their slots
  stay owned until a synchronous Docker or Git call has returned.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Own every host copy through a recorded Ask root; fix CI env dependency

- The bridge creates one Ask root per worker (<tmp>/codeboost-ask-*),
  records it before the worker starts, and runs the worker with it as
  TMPDIR, so the reviewed clone, lane D's input directory and its Codex
  auth copy all live inside it. The root is deleted after the thread
  stops (clean shutdown, crash or abandon); otherwise the next check
  deletes it, and Ask stays off while an earlier root remains.
- The record accepts only direct children of the real temp directory
  named codeboost-ask-XXXXXX, so a lookalike path elsewhere is refused
  instead of deleted.
- Test fix: the bridge checks sign-in before asking, so the stub worker
  now gets its own Codex auth file instead of depending on ~/.codex.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Delete an abandoned worker's root once its thread finally stops

If the bounded wait for an abandoned worker ends while its thread is
still inside a synchronous Docker or Git call, its ownership is already
durable (unknown leftovers and the recorded Ask root) and no new
question is admitted. The root is now also deleted, and dropped from
the record, as soon as that thread does stop.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Serialize Ask per review with a lock and bound the release timeout

- Take an exclusive per-review Ask lock (PID lock file next to the
  leftover record) before the startup scan and hold it until the worker
  has stopped; only the holder scans, starts a worker or writes the
  record. A lock left by a dead process is taken over.
- A worker that does not answer the final release request now goes
  through the bounded abandon path instead of an unbounded terminate,
  keeping its root and the lock until the thread stops.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Test that a refused second process cannot delete a live Ask root

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Give the Ask worker an allowlisted environment; credentials as data

Replace the name-based credential scrub with an explicit allowlist:
the worker's environment is only PATH, DOCKER_HOST and its Ask root as
TMPDIR, so every setup subprocess (including the image build) inherits
no credentials, home directory, Docker config or agent socket. The
credential lookup's four variables reach the worker via workerData and
go only to the adapters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Use an OS lock for Ask and key it by the canonical database path

- Replace the PID file and liveness takeover with an exclusive SQLite
  transaction on the lock file: an OS file lock the operating system
  releases when its process ends, so PID reuse cannot let two holders
  overlap and no takeover is needed.
- Key the lock and leftover record by the database's realpath, so
  relative, absolute and symlinked spellings share them; refuse Ask on a
  database with other hard links.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Clean up the Ask root when the worker cannot be constructed

If the Worker constructor throws after the root was created and
recorded, delete the root and drop it from the record, so close() can
release the per-review Ask lock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Serialize Ask worker abandonment and bound the host clone up front

- All abandon triggers (crash, watchdogs, shutdown) share one bounded
  termination promise, so a second trigger cannot reject questions and
  free their slots while the thread is still in a synchronous call.
- Before lane D's unbounded host clone, measure the checkout at the
  reviewed head and the object store with Git plumbing and refuse a
  repository that would not fit the question's storage allocation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Never drop recorded Ask roots; keep the marker for unnamed resources

- Ask roots are never sliced from the record; recording one past the
  cap is refused, which also refuses to start another worker.
- Any labelled Docker resource that is not part of a still-listed
  allocation (a seeder, agent container, proxy or network) keeps the
  unidentified marker after the named entries are gone, and it clears
  only when none remain. Named removal commands are reported first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Make the Ask startup scan single-flight

Concurrent first questions now share one startup scan instead of each
running their own, so a second scan cannot see the first question's new
labelled resources and record them as earlier-session leftovers. Each
caller can stop waiting through its own signal, and a failed scan is
retried by the next question.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Wait for an in-progress abandonment when the Ask worker closes

If a crash or watchdog is already abandoning the worker when shutdown
calls close(), close() now awaits that bounded settlement instead of
returning at once, so the thread, its recorded root and the lock are
settled before Questions.close() finishes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Key the Ask lock by file identity; finish the startup scan under it

- The lock file is keyed by the database's device and inode in the temp
  directory, so every spelling and every later name of the file,
  including an atomic rename while a server runs, finds the same lock.
  The durable record stays next to the canonical database path.
- close() waits for a shared startup scan still in flight before
  releasing the lock, so the scan cannot write the record unlocked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Find Ask roots by their owner stamp, not only through the record

The durable record sits beside the database path, so after a rename a
new process would not see roots recorded under the old name. Each Ask
root now carries an .owner stamp naming its lock, written under a
preparation name before the folder is renamed into place. The first
check of a process deletes unrecorded codeboost-ask-* folders whose
owner lock is free and leaves those whose owner is still running.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Harden the Git size check and trust only codeboost lock stamps

- The pre-clone Git measurement now uses the same hardening as lane D's
  clone: GIT_NO_LAZY_FETCH, protocol.allow=never, no replace objects,
  no hooks, no graft file, no submodule recursion.
- An .owner stamp is probed only when it names a codeboost lock file in
  the temp directory; anything else counts as no owner, so a lookalike
  root cannot make startup open or create a file elsewhere. Every
  ledger's lock now lives there under that name.
- Regression test: startup still scans Docker after deleting a recorded
  root (the reported bypass does not reproduce).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Name the labels the Ask leftover scan actually checks

The docs and one Ask error message still said io.codeboost.task-storage,
but the scan checks containers, volumes and networks labelled
io.codeboost.allocation, io.codeboost.invocation or io.codeboost.egress.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Write the Ask leftover record through an exclusive random temp file

The record was written via a predictable <record>.<pid>.tmp name with
the default "w" flag, so a planted link at that name would be followed
and its target overwritten. Use a random name opened with "wx" and
delete it if the write or rename fails.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Stop Ask admission when shutdown begins; keep the root if recording fails

- The server stops question admission in the same turn it starts
  shutting down, so a request still arriving cannot start an agent or a
  container worker during the drain. A question it saved gets a
  retryable "Server stopped" answer without any agent starting; the
  existing drain test now expects zero agent calls instead of one
  started-then-cancelled call.
- If the final release report cannot be saved, the worker's root is no
  longer deleted: it stays on disk and in the record for the next
  session, and Docker leftovers remain covered by the startup scan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Release the Ask lock when the final record write fails

After a failed release-report write the worker thread has stopped and
the root is already recorded, so let go of the root in this process:
it stays on disk and in the record for the next check, and close() can
release the per-review lock instead of holding it for the process.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Treat a failed worker termination as not stopped

A rejected terminate() no longer counts as a stopped thread: the Ask
root stays on disk and in the record, and the lock stays held, instead
of being removed while the worker may still be alive. Later cleanup runs
only after a termination that actually settled.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Add AGENTS.md rules from the PR #54 review-lesson audit

Four rules for owned host and Docker resources: durable cleanup
ownership, allowlisted subprocess environments and credential channels,
untrusted record and on-disk paths, and cross-process OS locks keyed by
stable identity.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Close the store on failed Ask cleanup; private locks; keep lock after abandon

- web/server.ts closes the review store in a finally block when Ask's
  cleanup fails, and the CLI exits non-zero instead of hanging.
- Ask lock files live in a private per-user directory under the temp
  directory (mode 0700, checked ownership); a lock path that is a
  symlink or not a plain file is refused, never opened.
- After any abandonment the review lock is kept until the process
  exits: Docker CLI children the terminated thread started can outlive
  it and cannot be awaited until lane D exposes process groups (#51).
- AGENTS.md: the untrusted-path rule also forbids following links and
  requires a private directory for plantable files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Pass the Ask stop reason as a typed value; leave foreign Ask folders alone

- Closes #64: the stop reason (timeout, shutdown, cancelled) now travels
  as a StopError value from Questions through the worker message to
  handle.cancel(), instead of being rebuilt from message wording.
- The orphan-root scan deletes only folders this user owns that carry a
  valid createAskRoot stamp naming a lock in the private lock directory
  whose owner is gone; unstamped, tampered or foreign folders stay.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Describe which Ask folders the orphan scan deletes

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Read the Ask record and owner stamps without following links

The leftover record and each folder's .owner stamp are now read through
O_NOFOLLOW and accepted only as regular, single-link files within a size
limit. A linked record makes the ledger unreadable (Ask fails closed and
never acts on the record it points to); a linked stamp leaves the folder
alone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support and verify merge-queue lifecycle

2 participants