Conversation
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>
There was a problem hiding this comment.
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
Open (3)
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.
Comment on lines
+59
to
+60
| export function answerFromResult(provider: Provider, result: InvocationResult): string { | ||
| if (result.stopReason) throw new Error(stopMessages[result.stopReason]); |
Comment on lines
+26
to
+30
| const fail = (error: Error) => { | ||
| if (this.worker !== worker) return; | ||
| this.worker = undefined; | ||
| for (const job of this.pending.values()) job.reject(new Error(`The agent container worker stopped: ${error.message}`)); | ||
| this.pending.clear(); |
Comment on lines
+108
to
+112
| const failures: unknown[] = []; | ||
| if (filesystems) try { deps.removeFilesystems(filesystems); } catch (error) { failures.push(error); } | ||
| try { chmodSync(input, 0o700); } catch { /* not created */ } | ||
| try { rmSync(root, { recursive: true, force: true }); } catch (error) { failures.push(error); } | ||
| if (failures.length) throw new AggregateError(failures, 'Question container cleanup did not settle.'); |
…nto ask-in-container # Conflicts: # docs/designs/codeboost-plan-indexed-review.md
Comment on lines
+62
to
+64
| if (result.exitCode !== 0) { | ||
| const detail = result.stdout.replace(/\s+/g, ' ').trim().slice(0, 300); | ||
| throw new Error(`${name} could not answer. Check its sign-in and usage limits.${detail ? ` ${name} said: ${detail}` : ''}`); |
| const invocation = deps.capture({ clone, phase: 'questions', vendor: question.provider, approvedArgv: [], | ||
| deadline: question.deadline, attemptId: question.attemptId, | ||
| context: { snapshotId: question.snapshotId, planId: question.planId, planRevision: question.planRevision, | ||
| assignmentId: question.noteId, referencedCodeHash: createHash('sha256').update(question.prompt).digest('hex'), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #52 (base
docs/reconcile-plan-with-code). When #52 merges, GitHub retargets this PR tomain.Summary
Ask no longer runs the
claudeorcodexCLI on the host. It runs in lane D's container, in the read-onlyquestionsphase. This closes the interim R1 exception that #52 recorded./workquestionsphase: read, list and search only, enforced by lane D's policyCLAUDE_CODE_OAUTH_TOKEN(fromclaude setup-token) or Codexauth.json, read from the environment and never storedHow it works
runner/question-container.tsbuilds the image once per worker, clones the head, allocates storage, captures the invocation (no approved argv), and starts the adapter. Storage is removed only aftersettled, as the D5 handoff requires.runner/question-worker.ts: every lane D entry point is synchronous (Docker and Git calls), so a worker thread owns them and the review server stays responsive. Lane D's trust registries live in that one worker.runner/question-agent.ts(QuestionWorker): a question settles only when the worker reports that the container and storage are gone, so a cancelled or timed-out container can't outlive the state that started it.auth.jsonor no Docker fails with a message that names what's missing. Provider failures now include the vendor's short reason, for example401 Invalid bearer token.Behavior changes for users
npm run demo, needs Docker plus a token orauth.json. The first question builds the agent image. If that takes longer than the two-minute question deadline, the question fails; Docker keeps the finished layers, and a retry continues from them.Tests
test/question-agent.test.ts, orchestration with injected lane D dependencies:questionsphase with no argv and the reviewed head;schema.json;Workerwith a stub: answer passthrough, cancellation that waits for the worker's reply, and crash recovery.test/questions.test.ts: the agent receives the configured repository and snapshot head. The timeout-then-retry regression required by AGENTS.md is already covered by "keeps cancelled invocations tracked until they settle".test/agent-question.test.ts(real Docker; added to the Agent isolation workflow and excluded from main CI like the other Docker suites):401. Only a request that left the container through the vendor proxy can get that response;CODEBOOST_RUN_AUTH_PROBES=1) asks for a random word that exists only in a file in the repository, so a correct answer shows the agent read/work.Validation
Evidence on the merged head (after merging the updated #52 branch, which includes
mainat5e97aa3):npm run typecheck: passed.npm run test:browser: 58/58 passed.test/agent-question.test.tson local Docker (earlier headf5b2f09): passed, with the live case skipped. CI'sreal-dockerjob also passed.CLAUDE_CODE_OAUTH_TOKEN.Ownership note
The plan assigns this move to lane F. F1 (#49, merged) was docs only. F1c (#57) also edits
runner/questions.ts, so whichever of #54 and #57 lands second must be rebased, fitting Ask's worker shutdown (Questions.close→QuestionWorker.close) into F1c's shutdown order. This PR edits shared files owned by the integration owner (runner/questions.ts,web/public/app.js,.github/workflows/ci.yml,.github/workflows/agent-isolation.yml), so it needs that owner's review.Review-lesson audit
No review findings yet.
🤖 Generated with Claude Code