Run Ask in the lane D agent container

[mchwang]

Closes #64. Lane F item: move Ask onto lane D's invocation contract. This ends the interim R1 exception that #52 recorded. Base: main (retargeted after #52 merged).

Summary

Ask no longer runs the claude or codex CLI on the host. It runs in lane D's container, in the read-only questions phase.

	Before	After
Where the agent runs	Host process, temporary folder, your environment	Lane D container: read-only root, no capabilities, bounded storage
Code it can see	Only the context in the prompt	That context, plus a read-only clone of the reviewed snapshot head at /work
Commands	Turned off by CLI flags	questions phase: read, list and search only, enforced by lane D's policy
Network	Host network	Vendor-only proxy
Sign-in	Host CLI login or keychain	CLAUDE_CODE_OAUTH_TOKEN (from claude setup-token) or Codex auth.json, never stored

How it works

• runner/question-container.ts builds the image once per worker and clones the reviewed head. It allocates storage and captures the invocation (no approved argv), then starts the adapter. Storage is removed only after settled.
• runner/question-worker.ts: lane D's entry points are synchronous (Docker and Git calls), so one worker thread owns them and the review server stays responsive.
• runner/question-agent.ts (QuestionWorker) settles a question only when the worker reports that its container and storage are gone.
• There is no host fallback. A missing token, missing auth.json or missing Docker fails with a message that names what is missing.

Fail-closed ownership stopgaps (added during review). Lane D does not yet label resources with their owner, bound its clone, or recover leftovers after a restart (#51 items 1, 3 and 4). Until it does, Ask owns and records its own leftovers in runner/question-leftovers.ts, next to the review database:

• Result identity: the invocation reuses the persisted answer attempt, and referencedCodeHash is the note's contextId. Results with another attempt or context, a missing exit code or any signal are rejected.
• Durable cleanup:
  • Storage whose removal Docker did not confirm is retained and retried, then recorded at shutdown.
  • Each worker gets one Ask root (<tmp>/codeboost-ask-*), used as its TMPDIR and recorded before the worker starts. It holds the clone, lane D's input and the Codex auth copy. Each root carries an owner stamp, so it can be found without the record.
  • A crash, a missing release report, or a question that does not settle 30 s after its deadline abandons the worker once. Waiters are released only after the thread stops (bounded), and unknown leftovers are recorded.
• Admission:
  • The first question of each process scans Docker for every lane D label (io.codeboost.allocation, io.codeboost.invocation, io.codeboost.egress). The scan runs single-flight.
  • Ask stays off while recorded or unidentified leftovers remain, and the error names removal commands.
  • Shutdown stops question admission in the same turn as the server's stopping flag.
• Exclusion: one process per review database, via an exclusive SQLite (OS) lock keyed by the database file's device and inode. The record is keyed by the canonical path. Hard-linked databases are refused.
• Isolation:
  • The worker's environment is an allowlist (PATH, DOCKER_HOST, TMPDIR); credentials reach it as workerData.
  • The Git size check before cloning uses lane D's hardened invocation and refuses repositories larger than the 512 MiB allocation.
  • Record entries and owner stamps are validated by exact location, and the record is written through an exclusive random temporary file.

Behavior changes for users

• Ask, including npm run demo, needs Docker plus a token or auth.json. The first question builds the agent image. If that exceeds the two-minute question deadline, the question fails, and a retry reuses the finished layers.
• A repository larger than the 512 MiB storage allocation is refused before anything is copied.
• After a crash or kill, Ask may stay off until labelled leftovers are removed. The error message gives the commands.

Scope decision for the reviewer

About 400 lines of the stopgaps above exist only because lane D lacks owner labels, a bounded clone and scoped recovery (#51). The per-review lock overlaps with F1d's single-runner lock (#59). The options are: merge now and replace these parts when #51 and F1d land (tracked in #65), or hold this PR for #51.

Ownership and coordination

• This PR edits files owned by the integration owner: runner/questions.ts, web/server.ts, web/public/app.js, web/cli.ts, .github/workflows/ci.yml and .github/workflows/agent-isolation.yml. It needs that owner's review.
• F1c (F1c: shutdown wiring and /api/runner #57) also edits runner/questions.ts and the server's shutdown order. Whichever of Run Ask in the lane D agent container #54 and F1c: shutdown wiring and /api/runner #57 lands second must fit Questions.stopAdmission() → drain → Questions.close() → QuestionWorker.close() into F1c's shutdown sequence.

Validation

Validated head: 64ac028 (current main merged in at 63a11cf).

• npm run typecheck: passed.
• Unit tests with CI's excludes: 486/486 passed (18 files).
• npm run test:browser: 58/58 passed (on 88bb96f; later commits touch only Ask's leftover module, its tests and docs).
• CI (test, real-docker) on 64ac028 is the final exact-head check; on the earlier head 6f047ac both passed.
• test/agent-question.test.ts (real Docker) ran in CI's real-docker job. The live Claude case has not run; it needs a real CLAUDE_CODE_OAUTH_TOKEN and CODEBOOST_RUN_AUTH_PROBES=1.
• Each review fix has a regression test. For each, I broke the fix on purpose and confirmed the test failed before restoring it.

Review rounds

About 20 Copilot rounds produced 60 inline findings. All 60 threads are resolved; the reply in each thread cites the fixing commit and its regression test. Rounds that reviewed an older head re-reported findings that were already fixed; those threads cite the earlier fix. As AGENTS.md requires, summary-only concerns without a location were reproduced where possible, or filed as follow-ups: #64 (stop reason passed as message text; Copilot later pinpointed it, and this PR now fixes and closes it) and #65 (shared-daemon isolation; open, depends on #51 item 3). One finding did not reproduce (startup marker, 4112551858). A regression for the reported case is included.

Review-lesson audit

Every finding is classified below. Rule citations refer to AGENTS.md in this head.

Covered by existing rules

• "Never apply a background response without proving it is still current": result identity and context (4110696969).
• "Validate coupled lifecycle fields as allowed combinations": null exit code or signal accepted as success (4110993674).
• "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": abandon before termination, the timer race, concurrent abandon, close during abandon, failed terminate treated as stopped (4112271725, 4112295157, 4112361784, 4112456414, 4112728747).
• "Bound the HTTP drain during shutdown…" and "Track an in-flight irreversible subprocess as part of server shutdown…": unbounded D settlement, shutdown deadlock, unbounded release-timeout terminate, scan outliving close(), Docker children outliving an abandoned thread (the lock is now kept until process exit), pending questions released when the termination wait expires (answered: the durable handoff is in place; no code change) (4112245838, 4112245880, 4112311246, 4112484482, 4113923388, 4113971096).
• "Begin shutdown by rejecting new work … then cancel and await jobs, then close storage": a failed Ask cleanup skipped closing the store (4113923423).
• "Begin shutdown by rejecting new work at the outer admission boundary" and "Set the shutdown admission flag before snapshotting active work…": admission after shutdown began (4112645321).
• "Batch and briefly cache read-only status probes, and give the combined operation an overall deadline": unbounded per-resource cleanup gate (4112207416).
• "A bounded safety scan must fail closed when its limit is exceeded. Never truncate evidence": record truncation, root overflow (4112207449, 4112376096).
• "When startup acquires … resource before later dependency construction, close that resource on every construction failure": worker construction failure (4112376078).

Captured by new rules in this head (AGENTS.md, "Owned host and Docker resources")

• Durable cleanup ownership: worker crash, lost removal handles, shutdown discarding retained storage, crash treated as clean release, setup failure without a handle, abandoned staging paths, the Codex auth copy, missing record at startup, failed final record write, lock kept after failed write, rename losing the record, unnamed labelled resources, the seeder missed by labels (4110696991, 4110697011, 4112180878, 4112180891, 4112207386, 4112207433, 4112219113, 4112245818, 4112245859, 4112282859, 4112282891, 4112376104, 4112519512, 4112645299, 4112708625).
• Allowlisted environments and hardened Git: credential leakage to setup subprocesses, the heuristic scrub, an unhardened Git size check (4112271713, 4112334205, 4112334222, 4112551830).
• Untrusted paths and exclusive writes (extended in this head to forbid following links and to require a private directory for plantable files): basename-only path validation, an untrusted owner stamp opened as a file, a predictable temporary file, a symlinked lock file in the shared temp directory, deleting unauthenticated lookalike folders, a linked owner stamp, a linked record file (4112282910, 4112551842, 4112609105, 4113923407, 4113933100, 4113971119, 4113971130).
• Cross-process OS locks and single-flight startup: no admission lock, unserialized record writes, reclaiming a live root, PID reuse, stale-lock takeover race, a path-keyed lock, a lock not surviving a rename, the double startup scan (4112311217, 4112311270, 4112323715, 4112345944, 4112345961, 4112361825, 4112484508, 4112432968).

Summary-only findings with a location (no inline thread)

• Stop reason inferred from message text (question-container.ts): covered by the existing rule "Preserve the original timeout, cancellation, and shutdown reason through every layer". Fixed in this PR; closes Ask: pass the stop reason as a typed value across the worker boundary #64.
• Lane D resource scan not scoped to this review: a known limit that depends on D follow-ups required by the F1 runner lifecycle contract #51 item 3. Tracked in Ask: tell apart Docker leftovers of different reviews sharing one daemon #65.

One-off (recorded why)

• 4110993691, referencedCodeHash hashed the prompt: a wrong field choice, specific to this contract.
• 4112168277, workflow path filter missed runner/questions.ts: a file-naming slip.
• 4112207464, && in the displayed removal command: a message-formatting bug.
• 4112271730, credential check ran after the Docker scan: step ordering in one entry point.
• 4112361805, unbounded host clone: a lane D API gap. The size check before cloning is a stopgap until D provides a bounded clone (D follow-ups required by the F1 runner lifecycle contract #51).
• 4112551858: did not reproduce; a regression for the reported case is included.
• 4113992060, worker started with the full environment: did not reproduce. workerEnvironment() allowlists PATH, DOCKER_HOST and TMPDIR, and the existing integration test covers it. The suggested this.env is the credential source and would leak the token.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical and moderate findings remain around invocation integrity, cleanup recovery, worker crashes, and CI coverage.

Review effort: Lite
Findings: 1 · 2

Open (3)

• Invocation result identity and context are discarded · New
• Worker crashes leak Docker resources and replacement state · New
• Failed filesystem cleanup loses resource ownership · New

What changed in this PR

Moves Ask from host CLI execution into lane D’s isolated Docker container workflow.

Changes:

• Adds worker-thread orchestration and container-backed Claude/Codex execution.
• Adds credential, storage, cancellation, cleanup, and Docker integration tests.
• Updates UI, CLI guidance, documentation, and CI workflows.

Unresolved findings concern invocation identity/context, resource cleanup and crash recovery, and workflow path coverage.

File	Summary
web/​public/​app.js	Updated settings guidance
web/​cli.ts	Updated CLI guidance
test/​questions.test.ts	Question lifecycle coverage
test/​question-agent.test.ts	Orchestration and worker tests
test/​fixtures/​question-worker-stub.ts	Worker test fixture
test/​agent-question.test.ts	Real Docker integration tests
runner/​questions.ts	Ask orchestration and scope
runner/​question-worker.ts	Lane D worker bridge
runner/​question-container.ts	Container lifecycle and cleanup
runner/​question-agent.ts	Worker lifecycle and cancellation
README.md	Containerized Ask documentation
docs/​implementation/​read-only-review.md	Superseded host-adapter documentation
docs/​implementation/​agent-isolation.md	Ask isolation documentation
docs/​designs/​codeboost-plan-indexed-review.md	Updated roadmap and safety status
.github/​workflows/​ci.yml	Excludes Docker tests from main CI
.github/​workflows/​agent-isolation.yml	Runs Ask isolation tests

---

💡 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, cleanup, cancellation, identity, permissions, result-validation, and CI coverage findings remain.

Review effort: Lite
Findings: 2 · 3

Open (5)

• Reject null or signalled invocation results · New
• Invocation result identity and context are discarded
• Use the assigned code hash for referencedCodeHash · New
• Failed filesystem cleanup loses resource ownership
• Worker crashes leak Docker resources and replacement state

[Copilot]

Copilot review overview

🔵 Needs a closer look

Four moderate findings remain around cancellation, crash recovery, cleanup recovery, and stderr diagnostics.

Review effort: Lite
Findings: 2 · 3

Open (5)

• Reject null or signalled invocation results
• Invocation result identity and context are discarded
• Use the assigned code hash for referencedCodeHash
• Failed filesystem cleanup loses resource ownership
• Worker crashes leak Docker resources and replacement state

[Copilot]

Copilot review overview

🟡 Changes recommended

Moderate resource-lifecycle, worker, container, and workflow coverage issues remain unresolved.

Review effort: Lite
Findings: 2 · 4

Open (6)

• Reject null or signalled invocation results
• Invocation result identity and context are discarded
• Path filter omits runner/questions.ts · New
• Use the assigned code hash for referencedCodeHash
• Failed filesystem cleanup loses resource ownership
• Worker crashes leak Docker resources and replacement state

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved lifecycle, cancellation, resource-ownership, and shutdown error-handling issues remain.

Review effort: Lite
Findings: 1

Open (1)

• Preserve worker ownership when termination fails · New

Resolved since last review (1)

• Failed ledger write leaves review lock unreleased

[Copilot]

Copilot review overview

🔵 Needs a closer look

Two unresolved moderate issues remain in cancellation handling and shared Docker-resource isolation.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Preserve worker ownership when termination fails

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate lifecycle, cleanup, locking, and timeout issues remain.

Review effort: Lite
Findings: 3

Open (3)

• Terminate Docker subprocesses before releasing review state · New
• Prevent symlink attacks on SQLite lock files · New
• Close the store when worker cleanup persistence fails · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Six unresolved findings remain, including one critical cleanup issue.

Review effort: Lite
Findings: 4

Open (4)

• Startup scan can delete unauthenticated lookalike directories · New
• Close the store when worker cleanup persistence fails
• Prevent symlink attacks on SQLite lock files
• Terminate Docker subprocesses before releasing review state

Previously missed (2)

In code that hasn't changed since last review

Transport typed stop reasons instead of inferring them from messages

runner/​question-container.ts:209

The D StopReason is reconstructed from user-facing error-message text (and the worker bridge transports only that text). A wording change, or an unrelated message containing timed out or Server stopped, can turn a timeout into shutdown/cancellation or vice versa, losing the lifecycle reason required by the invocation contract. Carry a typed reason separately from the display message through the worker boundary.

Lane-D resource scan is not scoped to this review

runner/​question-leftovers.ts:293

This scan treats every resource with a lane-D label as belonging to this review and marks any unlisted resource as a blocker. Since the current labels have no runner identity, a different codeboost review sharing the Docker daemon makes this review refuse all Ask until the other review's resources disappear. Scope the scan by a runner-owner label/recovery API before removing this stopgap, or keep the feature gated on that prerequisite.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved stop-reason propagation and cleanup issues remain, along with outdated documentation.

Review effort: Lite
Findings: 1

Open (1)

• Startup scan can delete unauthenticated lookalike directories

Resolved since last review (3)

• Close the store when worker cleanup persistence fails
• Prevent symlink attacks on SQLite lock files
• Terminate Docker subprocesses before releasing review state

Previously missed (1)

In code that hasn't changed since last review

Pass typed lane-D stop reason to cancellation handler

runner/​question-container.ts:209

The lane-D StopReason is still reconstructed from user-facing error wording here. Any wording change, localization, or non-Error abort reason silently maps timeout/shutdown to cancelled, so the adapter and persisted answer lose the original lifecycle reason. Carry a typed reason through the worker request and pass it directly to handle.cancel(); keep the message only for display.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate lifecycle, cleanup, and symlink-safety findings remain.

Review effort: Lite
Findings: 3

Open (3)

• Retain pending ownership until worker termination settles · New
• Read owner stamp without following symlinks · New
• Open ledger safely without following symlinks · New

Resolved since last review (1)

• Startup scan can delete unauthenticated lookalike directories

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved lifecycle and isolation findings remain in a broad resource-management change requiring human review.

Review effort: Lite
Findings: 3

Open (3)

• Open ledger safely without following symlinks
• Read owner stamp without following symlinks
• Retain pending ownership until worker termination settles

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved environment-isolation and resource-cleanup findings remain, including one critical finding.

Review effort: Lite
Findings: 1

Open (1)

• Worker receives unrestricted process environment instead of allowlisted variabl… · New

Resolved since last review (3)

• Open ledger safely without following symlinks
• Read owner stamp without following symlinks
• Retain pending ownership until worker termination settles

[mchwang]

Follow-ups from Copilot's summary-only review concerns (no inline thread):

• Ask: pass the stop reason as a typed value across the worker boundary #64: pass the Ask stop reason as a typed value across the worker boundary. Fixed in this PR (88bb96f); this PR closes it on merge.
• Ask: tell apart Docker leftovers of different reviews sharing one daemon #65: tell apart Docker leftovers of different reviews sharing one daemon. Still open. It depends on D follow-ups required by the F1 runner lifecycle contract #51 item 3 (owner labels on lane D resources). Until then Ask fails closed: another review's resources can make this review's Ask refuse to start, but nothing is deleted.

🤖 Generated with Claude Code