diff --git a/AGENTS.md b/AGENTS.md index 13891a34..556c1fcc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,6 +59,7 @@ A race regression must assert that the interleaving it names actually happened ( - Each self-review and independent-agent review pass examines the full current PR diff against the base, including changed tests, configuration and documentation, and rereads every changed function in full. Do not review only the fixes or lines changed since the previous round. Code unchanged since the first commit of the PR still gets reviewed in every pass. - Treat every behavioural claim the change makes, in code comments, the PR body or docs (for example "pauses every 1,000 entries", "settles only after exit", "never throws", "bounded by N seconds"), as something to verify. Trace each claim through every path that can break it, including nested loops, callbacks, error paths and early returns, and give it a test that fails if the claim is false. - A test's setup must leave the state the production path would: if production never runs a step (such as a commit that refreshes Git's index), the test must not run it before the behaviour under test either. +- A fake of an external tool (a stand-in `docker`, `git` or `gh` on `PATH`, or a stubbed client) must give, for every call the code reads, each part of the answer the code reads (exit status, stdout, stderr) as the real tool gives it, error cases included. Where it leaves out output the code does not read, say so beside the fake. Check those answers against the real tool and name the version checked beside the fake. Where versions the code may meet answer differently (for example `docker rm --force` on a missing name), the code must not depend on that answer: confirm the outcome with a call whose answer is stable, and give the fake each version's answer. - Independent-agent review is mandatory before every Copilot review request, including the first request and every re-request, for all changes, including documentation-only changes. The author's self-review is not a substitute. Use a reviewer that did not implement the change and does not share the author's working context: `/codex review`, a separate review agent, or `/code-review` at `high` effort or above. - Repeat independent-agent review -> address findings -> re-run relevant validation -> independent-agent review until a complete pass on the latest head reports no new issues and no earlier valid findings remain unresolved. A pass that found issues is not clean merely because they were fixed afterward; the fixes require another independent full-diff pass. Do not stop after a fixed number of rounds. - Only after that independent loop is clean may Copilot review be requested on the same reviewed and validated head. If Copilot finds issues, including summary-only concerns, address them and re-run relevant validation, then repeat the independent-agent review/fix loop until clean before requesting another Copilot review. Repeat this sequence until Copilot also completes a review with no new issues and no earlier valid findings remain unresolved. Never request Copilot in parallel with an unfinished independent-review cycle. diff --git a/CLAUDE.md b/CLAUDE.md index ecd01954..a9c9ffcd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,6 +29,7 @@ In QA mode, flag any code that doesn't match DESIGN.md. - Each self-review and independent-agent review pass examines the full current PR diff against the base, including changed tests, configuration and documentation, and rereads every changed function in full. Do not review only the fixes or lines changed since the previous round. Code unchanged since the first commit of the PR still gets reviewed in every pass. - Treat every behavioural claim the change makes, in code comments, the PR body or docs (for example "pauses every 1,000 entries", "settles only after exit", "never throws", "bounded by N seconds"), as something to verify. Trace each claim through every path that can break it, including nested loops, callbacks, error paths and early returns, and give it a test that fails if the claim is false. - A test's setup must leave the state the production path would: if production never runs a step (such as a commit that refreshes Git's index), the test must not run it before the behaviour under test either. +- A fake of an external tool (a stand-in `docker`, `git` or `gh` on `PATH`, or a stubbed client) must give, for every call the code reads, each part of the answer the code reads (exit status, stdout, stderr) as the real tool gives it, error cases included. Where it leaves out output the code does not read, say so beside the fake. Check those answers against the real tool and name the version checked beside the fake. Where versions the code may meet answer differently (for example `docker rm --force` on a missing name), the code must not depend on that answer: confirm the outcome with a call whose answer is stable, and give the fake each version's answer. - Independent-agent review is mandatory before every Copilot review request, including the first request and every re-request, for all changes, including documentation-only changes. The author's self-review is not a substitute. Use a reviewer that did not implement the change and does not share the author's working context: `/codex review`, a separate review agent, or `/code-review` at `high` effort or above. - Repeat independent-agent review -> address findings -> re-run relevant validation -> independent-agent review until a complete pass on the latest head reports no new issues and no earlier valid findings remain unresolved. A pass that found issues is not clean merely because they were fixed afterward; the fixes require another independent full-diff pass. Do not stop after a fixed number of rounds. - Only after that independent loop is clean may Copilot review be requested on the same reviewed and validated head. If Copilot finds issues, including summary-only concerns, address them and re-run relevant validation, then repeat the independent-agent review/fix loop until clean before requesting another Copilot review. Repeat this sequence until Copilot also completes a review with no new issues and no earlier valid findings remain unresolved. Never request Copilot in parallel with an unfinished independent-review cycle. diff --git a/README.md b/README.md index f83204f2..0885539f 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ A configuration with a `github` block may also add a `runner` block. The runner - `diagnosticsCapBytes`: the size retention trims that directory back to. It is a target, not a hard limit: the file just saved is always kept, even when it alone passes the cap. The default is 256 MiB. - `limits`: task storage limits (`workBytes`, `workInodes`, `metadataBytes`, `metadataInodes`). -The runner needs Docker and `CLAUDE_CODE_OAUTH_TOKEN`. At start, before the server opens, codeboost recovers what an earlier run left and builds the agent image; a recovery it cannot finish safely stops startup with what to do. `POST /api/runner` with `start` or `resume` runs the plan; the review screen has no runner buttons yet. A task paused for an out-of-scope change stays paused until the plan is amended, every changed path is declared, the remaining items validate against the runner's audited tree, and a person sends `approve-continuation` with the current `expectedStateVersion`, `expectedReviewVersion`, and an action ID. Then `resume` starts at the first unfinished item. `GET /api/runner` reports the checkpoint and whether the continuation is approved. When a run has completed every item, codeboost pushes the task head to a `codeboost/…` branch with your `gh` credentials and opens a ready pull request into `baseBranch`; a task that needs a person gets a draft pull request with its problems. Before merge, `prepare-merge` refreshes the published PR's exact base/head, rebases onto a moved base, refreshes attribution and approvals, and runs allowed `cmd:` checks in a credential-free read-only container. The check result counts only for that exact head and command list. A publish that was refused (for example, the branch holds a commit codeboost did not make) is retried with the `publish` action, and `GET /api/runner` shows the last outcome under `publish`. When a task is cancelled, codeboost closes the open pull requests it opened for it (the branch is kept). A pull request a person moved is left open and reported, and `close-pull-requests` retries a close that failed. Merge PR then merges that pull request, so `github.pullRequest` can be left out; if it is set, it must name the task's pull request, or merging is blocked. Demos never publish. +The runner needs Docker and `CLAUDE_CODE_OAUTH_TOKEN`. At start, before the server opens, codeboost recovers what an earlier run left and builds the agent image; a recovery it cannot finish safely stops startup with what to do. It then checks `pathIdentity` against the task storage the agent edits, a Linux tmpfs (`caseSensitive: true`, `unicodeNormalization: "none"`), and refuses to start when they differ. `POST /api/runner` with `start` or `resume` runs the plan; the review screen has no runner buttons yet. A task paused for an out-of-scope change stays paused until the plan is amended, every changed path is declared, the remaining items validate against the runner's audited tree, and a person sends `approve-continuation` with the current `expectedStateVersion`, `expectedReviewVersion`, and an action ID. Then `resume` starts at the first unfinished item. `GET /api/runner` reports the checkpoint and whether the continuation is approved. When a run has completed every item, codeboost pushes the task head to a `codeboost/…` branch with your `gh` credentials and opens a ready pull request into `baseBranch`; a task that needs a person gets a draft pull request with its problems. Before merge, `prepare-merge` refreshes the published PR's exact base/head, rebases onto a moved base, refreshes attribution and approvals, and runs allowed `cmd:` checks in a credential-free read-only container. The check result counts only for that exact head and command list. A publish that was refused (for example, the branch holds a commit codeboost did not make) is retried with the `publish` action, and `GET /api/runner` shows the last outcome under `publish`. When a task is cancelled, codeboost closes the open pull requests it opened for it (the branch is kept). A pull request a person moved is left open and reported, and `close-pull-requests` retries a close that failed. Merge PR then merges that pull request, so `github.pullRequest` can be left out; if it is set, it must name the task's pull request, or merging is blocked. Demos never publish. ## Library diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 194e755c..de5c718b 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -3,10 +3,11 @@ import { randomUUID } from 'node:crypto'; import { lstatSync } from 'node:fs'; import { join } from 'node:path'; import type { TaskClone } from '../contract.ts'; +import type { PathIdentity } from '../../core/path-identity.ts'; import { assertTaskClone } from '../../git/clone.ts'; import { assertBuiltAgentImage } from './image.ts'; import { createOutcomeUnknown, DOCKER_ID } from '../client-outcome.ts'; -import { DockerError, pause, runDocker, type DockerOutcome } from '../docker.ts'; +import { DockerError, NOT_STARTED, pause, runDocker, type DockerOutcome } from '../docker.ts'; import { MAXIMUM_TIMER_MS, runInProcessGroup, type ProcessGroup } from '../process-group.ts'; import { runTrackedProcess, type ProcessGroupLifecycle } from '../tracked-docker.ts'; import { MAXIMUM_DECLARED_LINKS, MAXIMUM_NAME_BYTES, TREE_SCRIPT } from './tree-script.ts'; @@ -265,6 +266,10 @@ function* allocationObjects(allocationId: string, expected: number, remaining: ( } } +/** The local-driver options of every task storage volume: a size- and inode-limited tmpfs owned by the storage user. */ +const TASK_VOLUME_OPTIONS = (bytes: number, inodes: number): readonly string[] => + ['type=tmpfs', 'device=tmpfs', `o=size=${bytes},nr_inodes=${inodes},uid=10001,gid=10001,mode=0755,nosuid,nodev`]; + /** The allocation, as a sequence of Docker calls. Both variants run these same steps; only how each call runs differs. */ function* allocation(clone: TaskClone, limits: TaskStorageLimits, imageId: string, owner: ResourceOwner, timeoutMs: number): Steps { @@ -306,8 +311,7 @@ function* allocation(clone: TaskClone, limits: TaskStorageLimits, imageId: strin try { for (const [kind, name, bytes, inodes] of [['work', workVolume, limits.workBytes, limits.workInodes], ['metadata', metadataVolume, limits.metadataBytes, limits.metadataInodes]] as const) { - yield* allocate(name, ['volume', 'create', '--driver', 'local', '--opt', 'type=tmpfs', '--opt', 'device=tmpfs', - '--opt', `o=size=${bytes},nr_inodes=${inodes},uid=10001,gid=10001,mode=0755,nosuid,nodev`, + yield* allocate(name, ['volume', 'create', '--driver', 'local', ...TASK_VOLUME_OPTIONS(bytes, inodes).flatMap(option => ['--opt', option]), '--label', `io.codeboost.task-storage=${kind}`, ...labels, name]); // The check above and this create are not atomic across processes. Once our first object exists, it must be // the only one carrying the ID: of two racing allocations, the later check sees both and backs out. @@ -400,6 +404,90 @@ export async function prepareTaskFilesystemsAsync(clone: TaskClone, limits: Task } } +// In the directory given as $1, writes `case` and NFC `caf\u00e9`, then asks whether `CASE` and NFD `cafe\u0301` name +// the same entries. printf writes the UTF-8 bytes, so the script itself stays ASCII. Exported so a test can run it on a +// host folder whose answer is known. +export const PATH_IDENTITY_SCRIPT = ['set -eu', 'cd "$1"', ': > case', ': > "$(printf \'caf\\303\\251\')"', + 'if [ -e CASE ]; then c=false; else c=true; fi', 'if [ -e "$(printf \'cafe\\314\\201\')" ]; then u=NFC; else u=none; fi', + 'printf \'codeboost-path-identity %s %s\\n\' "$c" "$u"'].join('; '); +/** The probe's `/work`: an anonymous local volume with task storage's tmpfs options, as one `--mount` value. */ +export const PROBE_VOLUME_MOUNT = ['type=volume', 'target=/work', 'volume-driver=local', + ...TASK_VOLUME_OPTIONS(1024 * 1024, 64).map(option => `volume-opt=${option}`)] + // A field with a comma in it is quoted, as Docker's CSV parser reads --mount. + .map(field => field.includes(',') ? `"${field}"` : field).join(','); +/** + * The path identity of task storage (#162): how the filesystem the agent edits, where the runner inspects task changes, tells two + * spellings of a path apart. It is measured, not assumed, on a volume made with the same options as task storage, in a + * container hardened like the keeper. The volume is anonymous and the container `--rm`, so once the container has + * started the daemon removes both when it exits, even if this process does not live to see it. A client stopped before + * then (its deadline) may leave the container created but never started, which `--rm` does not remove, so a failed + * probe removes its container, and that volume, by the unique name it gave it. Not covered: a create that lands after + * the settle window, or a runner that dies before the removal; the probe carries no owner labels, so recovery does not + * find that container (`docker ps -a --filter name=codeboost-path-identity-` does). Nor is the volume's removal + * confirmed: `--rm` and `rm --volumes` remove it, but Docker only logs a failure to. A recovery sweep is #181. + */ +export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000, cleanupMs = PROBE_CLEANUP_MS): Promise { + if (!/^sha256:[0-9a-f]{64}$/.test(imageId)) throw new Error('The path identity probe requires the immutable built image ID.'); + assertBuiltAgentImage(imageId); + const name = `codeboost-path-identity-${randomUUID()}`; + const args = ['run', '--rm', '--name', name, '--read-only', '--user', '10001:10001', '--network=none', '--cap-drop=ALL', + '--security-opt=no-new-privileges', '--security-opt=seccomp=builtin', '--runtime=runc', '--pids-limit=32', '--memory=128m', + '--cpus=.25', '--mount', PROBE_VOLUME_MOUNT, '--entrypoint', 'sh', imageId, '-c', PATH_IDENTITY_SCRIPT, 'probe', '/work']; + const outcome = await runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs }); + if (outcome.status !== 0) { + const failure = new DockerError(args, outcome); + // A client that never started sent nothing to the daemon, so there is nothing to remove. + const error = outcome.error as NodeJS.ErrnoException | undefined; + if (outcome.status === null && (error?.code === NOT_STARTED || error?.syscall?.startsWith('spawn'))) throw failure; + // Only a refusal from the daemon itself settles at once. Any other outcome (a killed client, a connection lost after + // the create was sent, exit 125 from the client) leaves the daemon's outcome unknown: a create may still land, so + // only absence after the settle window counts. + const refused = outcome.status !== null && /Error response from daemon/.test(outcome.stderr); + try { await removeProbeContainer(name, refused ? 0 : PROBE_SETTLE_MS, cleanupMs); } + catch (error) { + throw new Error(`${failure.message} The probe container ${name} may remain; remove it with docker rm --force --volumes ${name}.`, + { cause: new AggregateError([failure, error]) }); + } + throw failure; + } + const answer = /^codeboost-path-identity (true|false) (none|NFC)$/.exec(outcome.stdout.trim()); + if (!answer) throw new Error('The path identity probe did not report an identity.'); + return { caseSensitive: answer[1] === 'true', unicodeNormalization: answer[2] as PathIdentity['unicodeNormalization'] }; +} + +/** One cleanup call's deadline: the time left before giving up, capped at 10 s and at least 1 s. */ +export const probeCleanupCallMs = (leftMs: number): number => Math.min(10_000, Math.max(1_000, Math.ceil(leftMs))); +/** How long a failed probe's cleanup waits for an outcome Docker did not report (a killed client's create may land). */ +export const PROBE_SETTLE_MS = CREATE_SETTLE_MS; +/** How long a failed probe's cleanup keeps asking after its settle window before it gives up. */ +export const PROBE_CLEANUP_MS = 30_000; +/** + * Remove a failed probe's container and its anonymous volume by name. `rm --force` exits 0 for a name that does not + * exist, so only an inspect that reports the name absent, after the settle window, proves the probe is gone. + */ +async function removeProbeContainer(name: string, settleMs: number, cleanupMs: number): Promise { + const settleBy = performance.now() + settleMs, giveUpAt = settleBy + cleanupMs; + let last: DockerOutcome | undefined; + // Each call gets the time left before giving up, capped at 10 s and at least 1 s (so the last ask can still be + // answered), and after the first call none starts once that time has passed: a daemon that stops answering + // stretches the bound by at most one call's floor, its stop grace and one 250 ms pause. + const ask = async (args: readonly string[]) => { + if (last && performance.now() >= giveUpAt) + throw new Error(`Docker did not confirm that ${name} is gone.`, { cause: last }); + return last = await runInProcessGroup('docker', args, { env: dockerEnvironment(), + timeoutMs: probeCleanupCallMs(giveUpAt - performance.now()) }); + }; + for (;;) { + // Whatever rm answered (older CLIs exit non-zero for a missing name, newer ones 0), the inspect decides. + await ask(['rm', '--force', '--volumes', name]); + // The window is judged by when the inspect was asked: a slow answer to an ask made inside it proves nothing. + const asked = performance.now(); + if (absent(await ask(['container', 'inspect', name])) && asked >= settleBy) return; + // Still there (a removal already in progress), not yet landed, or Docker did not answer: ask again. + await pause(250); + } +} + /** * Task storage that `recoverLeftovers` found after a restart, when the `TaskFilesystems` value that allocated it is * gone. A crash during allocation can leave any subset of the parts, so each is optional. D issues a handle only after diff --git a/docs/implementation/plan-library-alignment.md b/docs/implementation/plan-library-alignment.md index 4ad993c5..7e76cac8 100644 --- a/docs/implementation/plan-library-alignment.md +++ b/docs/implementation/plan-library-alignment.md @@ -16,7 +16,7 @@ Issue #6 tracked the v1 plan library contract and the runner obligations that ca | Stable plan identity in approvals and attribution choices | Done before this change (PR #4) | `core/approvals.ts`; `test/identity.test.ts` ("binds approval to stable identity, item ID, and file metadata", "cannot transfer standalone acceptance to another plan") | | Retained schemas loaded through the registry | **This change** | `loadPlanSchemas` in `core/plan.ts`; `test/schema-versions.test.ts` | | Released snapshots and semantics never change | **This change** | SHA-256 digests in `test/schema-versions.test.ts` | -| Trusted base entries and path identity | **This change** (tests); probe at startup is #162 | `runner/review.ts`; `test/review.test.ts` | +| Trusted base entries and path identity | **This change** (tests); task storage is probed at runner startup since #162 | `runner/review.ts`; `test/review.test.ts`. #162: `agents/container/storage.ts` (`probeTaskStoragePathIdentity`), `runner/production.ts` (`setUpRunner`); `test/runner-production.test.ts`, `test/path-identity-probe.test.ts`, `test/runner-workspace.test.ts` ("#162") | | Hostile repository, issue, lesson, feedback, previous-plan and continuation-context text in every authoring prompt | **This change** (revised drafts, suggestions and the continuation context) | `test/planning-author.test.ts` | | Container mounts: Git metadata on its own filesystem, read-only, with no writable hard links or aliases into it | Done before this change | `agents/container/profile.ts`, probe `metadata-alias` in `agents/policy.ts`; `test/agent-container.test.ts` ("keeps Git metadata read-only, on another filesystem, and mounted against replacement", "keeps Git metadata unchanged under link, alias, truncation and replacement attempts during %s"), negative controls in `test/agent-gate.test.ts` (real Docker) | | Process termination: output overflow, deadline and cancellation stop the whole container, including a process that ignores SIGTERM | Done before this change | `agents/adapters/supervisor.ts`; `test/agent-supervisor.test.ts` ("terminates %s at bounded output limits", "enforces a finite wall deadline and force-settles the container", "preserves the first cancellation reason until an ignored SIGTERM fully settles") (real Docker) | diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index fe6817b3..f36b27ef 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -6,7 +6,7 @@ Started from `dd6a1e7`, after merging the SQLite store PR. This delivers the loc `npm ci --ignore-scripts`, then `npm run demo`. Open the private loopback URL printed by the command. The demo uses a real disposable Git repository and SQLite database under ignored `.codeboost-local/demo`; restart preserves approvals and notes. No source files in the codeboost checkout are edited by the demo. Stop with Ctrl+C. -For an existing trusted store, use `npm start -- --config /absolute/path/review.json`. The JSON has `database`, `repository`, `identity` (`repositoryId`, `taskId`, `planId`), and `pathIdentity` (`caseSensitive` boolean, `unicodeNormalization` equal to `none` or `NFC`). Paths should be absolute. Supply actual checkout identity rules, not an OS guess. The demo probes its local filesystem; it uses only simple ASCII fixture paths. Unsupported or more complex filesystem equivalence must not be approximated by this adapter. +For an existing trusted store, use `npm start -- --config /absolute/path/review.json`. The JSON has `database`, `repository`, `identity` (`repositoryId`, `taskId`, `planId`), and `pathIdentity` (`caseSensitive` boolean, `unicodeNormalization` equal to `none` or `NFC`). Paths should be absolute. Supply actual checkout identity rules, not an OS guess. The demo probes its local filesystem; it uses only simple ASCII fixture paths. With a `runner` block, startup probes case sensitivity and Unicode normalization on task storage, the filesystem the agent edits and where the runner inspects and commits task changes, and refuses to start when `pathIdentity` disagrees, naming the probed values (#162). Task storage is a tmpfs volume in the Docker daemon's Linux kernel, not a folder under `runner.root`, so the probe runs in a container, with the built image, on a volume made with task storage's options. It always reports `caseSensitive true` and `unicodeNormalization none`, including on a macOS host whose own folders fold both. The setting is kept and checked, not derived, so a review never silently changes its plan path keys. The runner's host checkouts (the staging clone and the rebase worktree) can still fold names on such a host; that is #179. Unsupported or more complex filesystem equivalence must not be approximated by this adapter. The selected repository's current HEAD is compared to the stored base. Refresh observes new heads and creates an immutable snapshot; approvals are recomputed for the resulting changes. It does not fetch, rebase, run tests, invoke agents, or merge. Histories still obey the library's linear/complete/bounded-history constraints. diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index afe01249..0b91ebf5 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -320,7 +320,7 @@ This runs before the coordinator opens. 7. **Unowned preparation check.** If any attempt has `preparation_started_at` set and `preparation_pgid` null (see "Launch"), stop here: do not open the coordinator, name each attempt and directory, and wait for `--release-preparation`. Step 4 does not remove those directories. 8. Open the coordinator. -**Production wiring (#91, #22).** The runner is opt-in: the review configuration's `runner` block names its root, an optional diagnostics directory and cap, the committer and optional storage limits (`runner/production.ts`, `parseRunnerConfig`). Without the block, or in a demo, there is no coordinator and runner actions answer that the runner is not configured. With it, `web/cli.ts` takes the lock, `startServer` opens the `Store` and then, before it listens, runs the planning setup when planning is on (`web/planning.ts`: verify the lock, then fail every planning request an earlier process left `pending`, #124). That step runs with or without a runner block: every start takes the lock, and planning is on for any non-demo review with a `github` block. Then it runs `setUpRunner`: it checks for the github block (the prompt carries the issue) and `CLAUDE_CODE_OAUTH_TOKEN`, calls the lock's `verify()` (decision 1, step 4), reads the runner token for the locked file, and runs `recoverStartup` with D's own `recoverLeftovers`, `exportTaskDiff` and `removeTaskFilesystemsAsync`. The agent image is built after D's recovery has stopped every leftover agent, so no agent keeps writing to its storage during a long build, and before the first export's deadline starts (the build blocks the event loop, so a deadline armed before it would expire as soon as it returned). Any failure closes the `Store`, releases the lock and exits with its message. To export a recovered storage, F saves the storage's `metadataBaseline` and the commit it was seeded from (`attempts.metadata_baseline`, `attempts.storage_base`, schema v9) as soon as D's allocation returns; a row without them gets "Partial output could not be exported: its storage baseline was never saved". Diagnostics are kept per database, in `//diagnostics`: retention reads references only from its own `Store`, so runners of databases sharing a directory must never see each other's files. Recovered diffs are saved through `saveDiagnostic`, with the same owner-only file and retention as the live path; retention keeps every diff this recovery saved, because none is referenced until the finalization transaction. A diff D cut at its byte limit ends with a `codeboost:` line that says so, on this path and the live one. The issue text a prompt carries (title, body and collaborators' comments) is read once and reused for later items for up to five minutes; the base tree listing is read once per base commit. A first Ctrl+C during startup waits for startup to finish; a second stops the process at once, which recovery treats like a crash at the next start. The composed deps accept `execute` and credential-free `check` attempts (`RunnerDeps.kinds`); admission refuses every other kind before it writes anything. A plan item runs only through `ItemExecutor`, so `/api/runner` refuses to retry an execute attempt on its own; a task's plan runs again through `resume` (below). +**Production wiring (#91, #22).** The runner is opt-in: the review configuration's `runner` block names its root, an optional diagnostics directory and cap, the committer and optional storage limits (`runner/production.ts`, `parseRunnerConfig`). Without the block, or in a demo, there is no coordinator and runner actions answer that the runner is not configured. With it, `web/cli.ts` takes the lock, `startServer` opens the `Store` and then, before it listens, runs the planning setup when planning is on (`web/planning.ts`: verify the lock, then fail every planning request an earlier process left `pending`, #124). That step runs with or without a runner block: every start takes the lock, and planning is on for any non-demo review with a `github` block. Then it runs `setUpRunner`: it checks for the github block (the prompt carries the issue) and `CLAUDE_CODE_OAUTH_TOKEN`, calls the lock's `verify()` (decision 1, step 4), reads the runner token for the locked file, and runs `recoverStartup` with D's own `recoverLeftovers`, `exportTaskDiff` and `removeTaskFilesystemsAsync`. The agent image is built after D's recovery has stopped every leftover agent, so no agent keeps writing to its storage during a long build, and before the first export's deadline starts (the build blocks the event loop, so a deadline armed before it would expire as soon as it returned). With the built image, it then probes task storage's path identity and refuses to start when `pathIdentity` disagrees (#162, `read-only-review.md`). Any failure closes the `Store`, releases the lock and exits with its message. To export a recovered storage, F saves the storage's `metadataBaseline` and the commit it was seeded from (`attempts.metadata_baseline`, `attempts.storage_base`, schema v9) as soon as D's allocation returns; a row without them gets "Partial output could not be exported: its storage baseline was never saved". Diagnostics are kept per database, in `//diagnostics`: retention reads references only from its own `Store`, so runners of databases sharing a directory must never see each other's files. Recovered diffs are saved through `saveDiagnostic`, with the same owner-only file and retention as the live path; retention keeps every diff this recovery saved, because none is referenced until the finalization transaction. A diff D cut at its byte limit ends with a `codeboost:` line that says so, on this path and the live one. The issue text a prompt carries (title, body and collaborators' comments) is read once and reused for later items for up to five minutes; the base tree listing is read once per base commit. A first Ctrl+C during startup waits for startup to finish; a second stops the process at once, which recovery treats like a crash at the next start. The composed deps accept `execute` and credential-free `check` attempts (`RunnerDeps.kinds`); admission refuses every other kind before it writes anything. A plan item runs only through `ItemExecutor`, so `/api/runner` refuses to retry an execute attempt on its own; a task's plan runs again through `resume` (below). **F5 production extension (#22).** The production dependency set also accepts `check` attempts. `prepare-merge` resolves the same task-owned pull request as merge admission, fetches its exact base/head without moving a runner ref, and stops for refreshed review when the head moved. A moved base is handled through the durable F3/F4 rebase marker and mapping. Attribution and approvals are recomputed before each allowed `cmd:` list runs sequentially in a fresh read-only workspace at the exact resulting head. The fixed image dispatcher reads bounded JSON argv arrays from the input mount, invokes them without a shell, and receives no provider credential or allowed external host. Its completed result records both the head and argv digest; failure, cancellation, interruption, a changed head, or changed argv does not count. A final fresh remote read turns movement during preparation into `review-required`, never ready. The saved user-action replay is settled with the background result even during shutdown; startup marks an interrupted preparation failed after resource recovery, so it never replays `preparing` forever. `GET /api/runner` exposes the active and last in-memory pre-merge result without rebuilding history during polling. diff --git a/runner/path-identity.ts b/runner/path-identity.ts new file mode 100644 index 00000000..6fae713f --- /dev/null +++ b/runner/path-identity.ts @@ -0,0 +1,27 @@ +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import type { PathIdentity } from '../core/path-identity.ts'; + +/** + * Probes the host filesystem that holds `directory`: writes `case` and NFC `caf\u00e9` in a fresh child directory, then + * asks whether `CASE` and NFD `cafe\u0301` name the same files. The child directory is removed again. The demo reviews a + * checkout on this filesystem; the runner's task checkouts are on task storage instead (`probeTaskStoragePathIdentity`). + */ +export function probePathIdentity(directory: string): PathIdentity { + const probe = mkdtempSync(join(directory, 'identity-')); + try { + writeFileSync(join(probe, 'case'), ''); + writeFileSync(join(probe, 'caf\u00e9'), ''); + return { caseSensitive: !existsSync(join(probe, 'CASE')), unicodeNormalization: existsSync(join(probe, 'cafe\u0301')) ? 'NFC' : 'none' }; + } finally { rmSync(probe, { recursive: true, force: true }); } +} + +/** + * Refuses a configured `pathIdentity` that disagrees with the probed one (#162). A wrong setting gives two spellings of + * one file different plan keys (or one key to two files), so plan validation and linking would be wrong. + */ +export function assertPathIdentity(configured: PathIdentity, probed: PathIdentity, where: string): void { + if (probed.caseSensitive === configured.caseSensitive && probed.unicodeNormalization === configured.unicodeNormalization) return; + const describe = (identity: PathIdentity) => `caseSensitive ${identity.caseSensitive}, unicodeNormalization ${identity.unicodeNormalization}`; + throw new Error(`pathIdentity (${describe(configured)}) does not match ${where} (${describe(probed)}). Set pathIdentity to the probed values.`); +} diff --git a/runner/production.ts b/runner/production.ts index cf015259..d276ef71 100644 --- a/runner/production.ts +++ b/runner/production.ts @@ -2,7 +2,7 @@ import { randomUUID } from 'node:crypto'; import { chmodSync, mkdirSync, writeFileSync } from 'node:fs'; import { isAbsolute, join } from 'node:path'; import { buildAgentImage } from '../agents/container/image.ts'; -import { exportTaskDiff, removeTaskFilesystemsAsync, type TaskStorageLimits } from '../agents/container/storage.ts'; +import { exportTaskDiff, probeTaskStoragePathIdentity, removeTaskFilesystemsAsync, type TaskStorageLimits } from '../agents/container/storage.ts'; import { recoverLeftovers } from '../agents/recovery.ts'; import { startClaudeInvocation } from '../agents/adapters/claude.ts'; import { startRunnerCommandInvocation } from '../agents/adapters/runner.ts'; @@ -26,6 +26,8 @@ import { createTaskWorkspace, workspaceFilesystems } from './workspace.ts'; import { commandCheckDeps } from './checks.ts'; import { PreMergeCoordinator, type PreMergeAuthorization } from './pre-merge.ts'; import type { RunnerCoordinator } from './coordinator.ts'; +import { assertPathIdentity } from './path-identity.ts'; +import type { PathIdentity } from '../core/path-identity.ts'; /** * The production runner (#91): D's real workspace, launcher, recovery, export and removal, under the database's runner @@ -196,13 +198,16 @@ export interface RunnerAssembly { /** * Startup with a runner, after the Store opened and before the server admits anything (runner-lifecycle.md, "Startup * recovery"): verify the lock still names the database, run recovery under the database's runner token, build the - * agent image (once D's recovery returns, if it found any storage; otherwise after recovery), then assemble the - * execution deps. Any failure stops startup; the caller closes the Store. + * agent image (once D's recovery returns, if it found any storage; otherwise after recovery), check pathIdentity against + * task storage with that image (#162), then assemble the execution deps. Any failure stops startup; the caller closes + * the Store. */ export async function setUpRunner(o: { service: ReviewService; capability: ShutdownCapability; config: RunnerConfig; lock: Pick; env: Readonly>; buildImage?: () => string; recovery?: (image: () => string) => RecoveryDeps; /** Told before the slow part (recovery and the image build) starts. */ - onSlowStart?: () => void }): Promise { + onSlowStart?: () => void; + /** Measures task storage's path identity with the built image; Docker's own by default. */ + probeTaskStorage?: (imageId: string) => Promise }): Promise { const { service, config } = o, review = service.config; if (review.demo) throw new Error('Demos never run the runner.'); if (!review.github) throw new Error(RUNNER_NEEDS_GITHUB); @@ -267,6 +272,12 @@ export async function setUpRunner(o: { service: ReviewService; capability: Shutd service.store.settleInterruptedMergeJobs(identity); const repository = await getRepository(); const imageId = image(); + // #162: plan path keys follow pathIdentity, so it must match the filesystem the agent edits, where the runner inspects + // and commits task changes. That is task storage in the Docker daemon's kernel, not the host folder the staging clone + // is made in, so it is probed there, with the built image, before anything is admitted. Host checkouts (the staging + // clone, the rebase worktree) are #179. + assertPathIdentity(review.pathIdentity, await (o.probeTaskStorage ?? probeTaskStoragePathIdentity)(imageId), + 'the task storage the agent edits'); const workspace = createTaskWorkspace({ store: service.store, runnerRoot: config.root, runnerOwner, repository, imageId, limits: { ...EXECUTE_STORAGE, ...config.limits }, committer: config.committer }); const issues = new GhIssueGateway(review.github.repository); diff --git a/scripts/demo.ts b/scripts/demo.ts index 3f306cd0..ecc23d3a 100644 --- a/scripts/demo.ts +++ b/scripts/demo.ts @@ -1,4 +1,4 @@ -import { existsSync, mkdirSync, mkdtempSync, rmSync, readFileSync, writeFileSync, chmodSync, lstatSync, realpathSync } from 'node:fs'; +import { existsSync, mkdirSync, readFileSync, writeFileSync, chmodSync, lstatSync, realpathSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { resolve, join, relative, isAbsolute, dirname } from 'node:path'; import { execFileSync } from 'node:child_process'; @@ -6,6 +6,7 @@ import { randomUUID } from 'node:crypto'; import { Store } from '../runner/store.ts'; import type { ReviewConfig } from '../runner/review.ts'; import type { Plan } from '../core/plan.ts'; +import { probePathIdentity } from '../runner/path-identity.ts'; import { HARDENED_GIT_OPTIONS, hardenedGitEnvironment } from './git-environment.ts'; /** Disposable fixture only. Never runs against the user's working repository. */ export function createDemo(directory: string): ReviewConfig { @@ -51,10 +52,7 @@ export function createDemo(directory: string): ReviewConfig { {id:'P2',title:'Document retry behavior',intent:'Describe the retry strategy for maintainers.',files:[{path:'README.md',kind:'edit',renamed_from:null,change:'Explain the capped exponential delay.'}],acceptance:[{type:'check',text:'Documentation agrees with the implementation.'}],depends_on:['P1']}, {id:'P3',title:'Confirm API compatibility',intent:'Confirm the function signature is unchanged.',files:[{path:'retry.ts',kind:'edit',renamed_from:null,change:'Keep the public function signature.'}],acceptance:[{type:'check',text:'No API change is needed.'}],depends_on:['P1']}, ]}; - const probe=mkdtempSync(join(root,'identity-')); - let pathIdentity:ReviewConfig['pathIdentity']; - try { writeFileSync(join(probe,'case'),'');writeFileSync(join(probe,'café'),'');pathIdentity={caseSensitive:!existsSync(join(probe,'CASE')),unicodeNormalization:existsSync(join(probe,'cafe\u0301'))?'NFC':'none'}; } - finally {rmSync(probe,{recursive:true});} + const pathIdentity=probePathIdentity(root); const config:ReviewConfig={database:join(root,'review.sqlite'),repository,identity,pathIdentity,demo:true}; const store=new Store(config.database); try { store.createPlan(JSON.stringify(plan),'json',{identity,issue:3,baseEntries:['retry.ts','README.md','run.sh'].map(path=>({path,kind:'file' as const})),pathKey:p=>p,allowedCommands:[]},base,head); diff --git a/test/cli.test.ts b/test/cli.test.ts index aaefcd9e..c483f940 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -17,22 +17,32 @@ const cli = fileURLToPath(new URL('../web/cli.ts', import.meta.url)); const roots: string[] = []; afterEach(() => { for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); }); /** A non-demo review with a runner block, and a HOME of its own so the lock directory is the test's. */ -function review(github: Record | null = { baseBranch: 'main' }) { +function review(github: Record | null = { baseBranch: 'main' }, + pathIdentity: { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' } = { caseSensitive: true, unicodeNormalization: 'none' }) { const root = mkdtempSync(join(tmpdir(), 'codeboost-cli-')); roots.push(root); const demo = createDemo(join(root, 'demo')), store = new Store(demo.database), issue = store.getPlan(demo.identity).issue; store.close(); mkdirSync(join(root, 'runner'), { mode: 0o700 }); mkdirSync(join(root, 'home'), { mode: 0o700 }); const config = join(root, 'review.json'); - writeFileSync(config, JSON.stringify({ ...demo, demo: false, ...(github ? { github: { repository: 'owner/repo', pullRequest: 1, issue, ...github } } : {}), + // What task storage reports (a Linux tmpfs), and what the fake docker below answers the startup probe with (#162). + writeFileSync(config, JSON.stringify({ ...demo, demo: false, pathIdentity, ...(github ? { github: { repository: 'owner/repo', pullRequest: 1, issue, ...github } } : {}), runner: { root: join(root, 'runner'), committer: { name: 'codeboost', email: 'runner@codeboost.invalid' } } })); /** - * A `docker` that answers every call with nothing (no leftovers, an image that matches the pinned profile) once the hold - * file is gone: startup recovery waits on it, as on a slow daemon, for as long as the test keeps the file. + * A `docker` that answers every call with nothing (no leftovers, an image that matches the pinned profile, and the + * path identity of a Linux tmpfs to the startup probe) once the hold file is gone: startup recovery waits on it, as on + * a slow daemon, for as long as the test keeps the file. It models only the calls a successful startup makes, checked + * against Docker 27.4.0: recovery's label listings (`ps --all --quiet --no-trunc`, `volume ls --quiet`, + * `network ls --quiet --no-trunc`) with nothing to list answer as here, exit 0 and no output; of `build` only the exit + * status is read; `image inspect` answers a one-element array, of which the image builder reads only `Id`, + * `Config.User` and `Config.Labels`, given here in Docker's shape; and the probe on real task storage prints the line + * given here (runner-workspace.test.ts). The probe's failure cleanup, whose rm and inspect answers matter, is tested against its + * own fake in path-identity-probe.test.ts. */ const fakeDocker = () => { const bin = join(root, 'bin'), hold = join(bin, 'hold'), image = JSON.stringify([{ Id: `sha256:${'a'.repeat(64)}`, Config: { User: '10001:10001', Labels: { 'org.opencontainers.image.base.name': BASE_IMAGE, 'io.codeboost.codex.version': CODEX_VERSION, 'io.codeboost.claude.version': CLAUDE_VERSION, 'io.codeboost.profile.version': '1' } } }]); mkdirSync(bin, { mode: 0o700 }); writeFileSync(hold, ''); - writeFileSync(join(bin, 'docker'), `#!/bin/sh\nwhile [ -e '${hold}' ]; do sleep 0.05; done\nif [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; fi\nexit 0\n`); + writeFileSync(join(bin, 'docker'), `#!/bin/sh\nwhile [ -e '${hold}' ]; do sleep 0.05; done\nif [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; fi\n` + + `if [ "$1" = run ]; then case "$*" in *codeboost-path-identity*) echo 'codeboost-path-identity true none';; esac; fi\nexit 0\n`); chmodSync(join(bin, 'docker'), 0o755); return { path: `${bin}:${process.env.PATH}`, release: () => { if (existsSync(hold)) unlinkSync(hold); } }; }; @@ -189,6 +199,17 @@ exit 1 try { expect(reopened.lastPublish(identity)).toMatchObject({ outcome: 'failed', draft: true }); } finally { reopened.close(); } } finally { cli.child.kill('SIGKILL'); } }); +it('refuses with a message, not a stack, when pathIdentity disagrees with task storage (#162)', async () => { + // A macOS host folder would report this; the agent edits a Linux tmpfs, which the fake docker reports as Docker would. + const { fakeDocker, start } = review(undefined, { caseSensitive: false, unicodeNormalization: 'NFC' }), docker = fakeDocker(); + docker.release(); + const cli = start(docker); + try { + expect(await cli.exited).toBe(1); + expect(cli.err().trim()).toBe('pathIdentity (caseSensitive false, unicodeNormalization NFC) does not match the task storage the agent edits ' + + '(caseSensitive true, unicodeNormalization none). Set pathIdentity to the probed values.'); + } finally { cli.child.kill('SIGKILL'); } +}); it('refuses with a message, not a stack, when the database path changes during a runner startup', async () => { const { fakeDocker, start, database } = review(), docker = fakeDocker(); const cli = start(docker); diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts new file mode 100644 index 00000000..a0198512 --- /dev/null +++ b/test/path-identity-probe.test.ts @@ -0,0 +1,307 @@ +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { BASE_IMAGE, CLAUDE_VERSION, CODEX_VERSION, buildAgentImage } from '../agents/container/image.ts'; +import { PATH_IDENTITY_SCRIPT, PROBE_CLEANUP_MS, PROBE_SETTLE_MS, probeCleanupCallMs, probeTaskStoragePathIdentity } from '../agents/container/storage.ts'; + +// The probe's own handling of Docker's answers (#162), against a `docker` on PATH that answers as the daemon would. The +// measurement itself runs against real Docker in runner-workspace.test.ts. +const root = mkdtempSync(join(tmpdir(), 'codeboost-probe-')), bin = join(root, 'bin'); +const calls = join(root, 'calls'), mode = join(root, 'mode'), landAt = join(root, 'land-at'), removed = join(root, 'removed'); +const slowLand = join(root, 'slow-land'), slowAnswered = join(root, 'slow-answered'), inspects = join(root, 'inspects'), lateMark = join(root, 'late'), rmLog = join(root, 'rm-log'), slow = join(root, 'slow'), busy = join(root, 'busy'), oldCli = join(root, 'old-cli'), hangAt = join(root, 'hang-at'), hung = join(root, 'hung'), unreach = join(root, 'unreachable'), identity = join(root, 'identity'); +const savedPath = process.env.PATH; +let imageId = ''; +beforeAll(() => { + mkdirSync(bin, { mode: 0o700 }); + const image = JSON.stringify([{ Id: `sha256:${'b'.repeat(64)}`, Config: { User: '10001:10001', Labels: { 'org.opencontainers.image.base.name': BASE_IMAGE, + 'io.codeboost.codex.version': CODEX_VERSION, 'io.codeboost.claude.version': CLAUDE_VERSION, 'io.codeboost.profile.version': '1' } } }]); + // `run`: per the mode file, answer (the identity file's line if there is one, as a filesystem other than tmpfs would + // answer; otherwise tmpfs's), refuse as the daemon does, or hang until the client is killed. In `late` mode the + // create lands just after the first inspect has answered that it is absent, after the client is gone. `rm --force` and `container inspect` answer as Docker 27 + // does: rm exits 0 whether or not the name exists (printing "No such container" when it does not); inspect exits 1. + // While the busy file holds a count, rm of a container that exists refuses as Docker does while an auto-remove is in + // progress, and inspect still reports it. From the time in the hang-at file, rm and inspect never answer (a daemon + // that stopped); each such call is logged in the hung file. With the slow file, rm and inspect answer after 300 ms. With the slow-land file, the sixth inspect finds nothing, then the create lands while that answer takes 9 s to arrive (a daemon under load); the slow-answered file marks that the answer was given. Of rm and inspect, the code reads only the exit status + // and, for a missing name, the "No such container" text; for an existing container this fake prints less stdout + // than Docker (the name; the full inspect array), which nothing reads. `unreachable` is a daemon the client cannot + // reach: run, rm and inspect answer as Docker 27.4.0 does with DOCKER_HOST=tcp://127.0.0.1:1 (run exit 125; rm + // exit 1; inspect `[]` and exit 1, all with "Cannot connect"). `garbled` and `preceded` exit 0 with output that is not one line. Checked against Docker 27.4.0, as are `build` (only its exit status is read) and `image + // inspect` (a one-element array; only `Id`, `Config.User` and `Config.Labels` are read, given in Docker's shape). With the old-cli file, rm of a missing name exits 1, as + // CLIs before 20.10 did by the Docker 20.10.0 release notes (docker/cli#2678); no such CLI was run here, so its + // stderr text (Docker 27's wording here) is not checked. + writeFileSync(join(bin, 'docker'), `#!/bin/sh +printf '%s\\n' "$*" >> '${calls}' +exists() { [ ! -e '${removed}' ] && [ -e '${landAt}' ] && [ "$(date +%s)" -ge "$(cat '${landAt}')" ]; } +if [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; exit 0; fi +if [ "$1" = run ]; then + case "$(cat '${mode}')" in + answer) if [ -e '${identity}' ]; then cat '${identity}'; else echo 'codeboost-path-identity true none'; fi;; + garbled) printf '%s\\n' 'codeboost-path-identity true none' 'extra';; + preceded) printf '%s\\n' 'noise' 'codeboost-path-identity true none';; + unreachable) echo 1 > '${unreach}'; printf '%s\\n' 'docker: Cannot connect to the Docker daemon at tcp://127.0.0.1:1. Is the docker daemon running?.' "See 'docker run --help'." >&2; exit 125;; + refused) printf '%s\\n' 'docker: Error response from daemon: unknown or invalid runtime name: x.' "See 'docker run --help'." >&2; exit 125;; + created) echo 0 > '${landAt}'; exec sleep 30;; + late) : > '${lateMark}'; exec sleep 30;; + hang) exec sleep 30;; + esac +fi +if [ -e '${hangAt}' ] && [ "$(date +%s)" -ge "$(cat '${hangAt}')" ] && { [ "$1" = rm ] || [ "$1 $2" = "container inspect" ]; }; then + echo "$1" >> '${hung}'; exec sleep 30 +fi +if [ -e '${unreach}' ] && { [ "$1" = rm ] || [ "$1 $2" = "container inspect" ]; }; then + [ "$1" = rm ] || echo '[]' + echo 'Cannot connect to the Docker daemon at tcp://127.0.0.1:1. Is the docker daemon running?' >&2; exit 1 +fi +if [ -e '${slow}' ] && { [ "$1" = rm ] || [ "$1 $2" = "container inspect" ]; }; then sleep 0.3; fi +if [ "$1" = rm ]; then + if exists && [ -s '${busy}' ] && [ "$(cat '${busy}')" -gt 0 ]; then + echo $(( $(cat '${busy}') - 1 )) > '${busy}' + echo "Error response from daemon: removal of container $4 is already in progress" >&2; exit 1 + fi + if exists; then echo found >> '${rmLog}'; : > '${removed}'; exit 0; fi + echo missing >> '${rmLog}' + echo "Error response from daemon: No such container: $4" >&2 + if [ -e '${oldCli}' ]; then exit 1; fi + exit 0 +fi +if [ "$1 $2" = "container inspect" ]; then + echo x >> '${inspects}' + if [ -e '${slowLand}' ] && [ "$(wc -l < '${inspects}')" -eq 6 ]; then + echo 0 > '${landAt}'; sleep 9 + : > '${slowAnswered}'; echo '[]'; echo "Error response from daemon: No such container: $3" >&2; exit 1 + fi + if exists; then echo '[{}]'; exit 0; fi + if [ -e '${lateMark}' ] && [ ! -e '${landAt}' ]; then echo 0 > '${landAt}'; fi + echo '[]'; echo "Error response from daemon: No such container: $3" >&2; exit 1 +fi +exit 0 +`); + chmodSync(join(bin, 'docker'), 0o755); + process.env.PATH = `${bin}:${savedPath}`; + imageId = buildAgentImage(); +}); +afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); +beforeEach(() => { for (const file of [calls, landAt, removed, slowLand, slowAnswered, inspects, lateMark, rmLog, slow, busy, oldCli, hangAt, hung, unreach, identity]) if (existsSync(file)) rmSync(file); }); +const logged = () => (existsSync(calls) ? readFileSync(calls, 'utf8') : '').trim().split('\n').filter(line => !line.startsWith('build ') && !line.startsWith('image ')); +const probeName = () => /--name (codeboost-path-identity-[0-9a-f-]{36}) /.exec(logged()[0]!)![1]!; + +const removals = () => logged().slice(1); +const left = () => existsSync(landAt) && !existsSync(removed); + +describe('task storage probe against Docker answers', () => { + it('reads the identity from a probe that ran, and removes nothing by hand (--rm did)', async () => { + writeFileSync(mode, 'answer'); + expect(await probeTaskStoragePathIdentity(imageId)).toEqual({ caseSensitive: true, unicodeNormalization: 'none' }); + expect(removals()).toEqual([]); + }); + it('runs on a volume made like task storage, in a container hardened like the keeper', async () => { + // Any Linux volume would answer `true none`, so only the arguments show the probe measures what task storage is. + writeFileSync(mode, 'answer'); + await probeTaskStoragePathIdentity(imageId); + const run = logged()[0]!, script = run.slice(run.indexOf(' -c ') + 4); + expect(run.slice(0, run.indexOf(' -c '))).toBe(`run --rm --name ${probeName()} --read-only --user 10001:10001 --network=none --cap-drop=ALL` + + ' --security-opt=no-new-privileges --security-opt=seccomp=builtin --runtime=runc --pids-limit=32 --memory=128m --cpus=.25' + + ' --mount type=volume,target=/work,volume-driver=local,volume-opt=type=tmpfs,volume-opt=device=tmpfs,' + + '"volume-opt=o=size=1048576,nr_inodes=64,uid=10001,gid=10001,mode=0755,nosuid,nodev"' + + ` --entrypoint sh ${imageId}`); + // The measuring script itself: a constant printed instead would pass on any Linux volume. + expect(script).toBe(`${PATH_IDENTITY_SCRIPT} probe /work`); + }); + it('removes its container and volume by name when the daemon refuses the run, and confirms it is gone', async () => { + writeFileSync(mode, 'refused'); + await expect(probeTaskStoragePathIdentity(imageId)).rejects + .toThrow(/^docker run failed \(exit 125\): docker: Error response from daemon: unknown or invalid runtime name: x\.\nSee 'docker run --help'\.$/); + expect(removals()).toEqual([`rm --force --volumes ${probeName()}`, `container inspect ${probeName()}`]); + }); + it('removes a container a killed client left created but never started', async () => { + writeFileSync(mode, 'created'); + await expect(probeTaskStoragePathIdentity(imageId, 2_000)).rejects.toThrow('docker run failed (client killed or not started)'); + // The interleaving this test is about: the create landed before the client was killed. + expect(existsSync(landAt)).toBe(true); + expect(removals()).toContain(`rm --force --volumes ${probeName()}`); + expect(left()).toBe(false); + }, 60_000); + it('removes a create that lands after the killed client, within the settle window', async () => { + // The first rm and inspect find nothing (rm still exits 0); the create lands after that, so only asking again, + // rm included, until the window closes removes it. + writeFileSync(mode, 'late'); + const started = performance.now(); + await expect(probeTaskStoragePathIdentity(imageId, 2_000)).rejects.toThrow('client killed or not started'); + // The window starts when the client is stopped and ends 10 s later: the first absent answer after it is accepted. + const elapsed = performance.now() - started; + expect(elapsed).toBeGreaterThanOrEqual(2_000 + 10_000); + expect(elapsed).toBeLessThan(2_000 + 10_000 + 2_500); + // The interleaving this test is about: nothing at the first rm, and a later rm removed what landed. + const rms = readFileSync(rmLog, 'utf8').trim().split('\n'); + expect(rms[0]).toBe('missing'); + expect(rms).toContain('found'); + expect(existsSync(removed)).toBe(true); + expect(new Set(removals())).toEqual(new Set([`rm --force --volumes ${probeName()}`, `container inspect ${probeName()}`])); + }, 60_000); + it('removes nothing, and does not wait, when docker never started', async () => { + writeFileSync(mode, 'answer'); + const empty = join(root, 'empty'); mkdirSync(empty, { recursive: true }); + process.env.PATH = empty; + const started = performance.now(); + try { await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (client killed or not started)'); } + finally { process.env.PATH = `${bin}:${savedPath}`; } + expect(performance.now() - started).toBeLessThan(2_000); + expect(existsSync(calls)).toBe(false); + }); + it('gives up when Docker keeps reporting the container, and names it for the person to remove', async () => { + writeFileSync(mode, 'created'); writeFileSync(busy, '100000'); + const started = performance.now(); + const failure = await probeTaskStoragePathIdentity(imageId, 2_000, 1_000).catch((error: Error) => error); + // Giving up is counted from the end of the settle window, which starts when the client is stopped. + expect(performance.now() - started).toBeGreaterThanOrEqual(2_000 + 10_000 + 1_000); + // The interleaving this test is about: the create landed before the client was killed. + expect(existsSync(landAt)).toBe(true); + const name = probeName(); + expect((failure as Error).message).toBe('docker run failed (client killed or not started): docker run ETIMEDOUT after 2000 ms.' + + ` The probe container ${name} may remain; remove it with docker rm --force --volumes ${name}.`); + const cause = (failure as Error).cause as AggregateError; + expect(cause.errors.map((error: Error) => error.message)).toEqual([expect.stringMatching(/^docker run failed/), `Docker did not confirm that ${name} is gone.`]); + // The last thing Docker said: the inspect still found the container, or (when giving up fell due during the rm) + // the rm's refusal while its removal was in progress. + const said = cause.errors[1].cause as { status: number | null; stderr: string }; + expect(said.status === 0 || (said.status === 1 && /is already in progress/.test(said.stderr))).toBe(true); + }, 60_000); + it('removes nothing when the client was refused before it started', async () => { + writeFileSync(mode, 'answer'); + // A deadline runInProcessGroup refuses (NOT_STARTED): nothing reached Docker. + await expect(probeTaskStoragePathIdentity(imageId, 0)).rejects.toThrow('Process deadline must be a positive integer'); + expect(existsSync(calls)).toBe(false); + }); + it('asks again while Docker refuses the removal of a container that exists', async () => { + writeFileSync(mode, 'created'); writeFileSync(busy, '2'); + await expect(probeTaskStoragePathIdentity(imageId, 2_000)).rejects.toThrow('client killed or not started'); + // The interleaving this test is about: the create landed before the client was killed. + expect(existsSync(landAt)).toBe(true); + expect(removals().filter(call => call.startsWith('rm ')).length).toBeGreaterThanOrEqual(3); + expect(left()).toBe(false); + // The interleaving this test is about: both refusals were answered, and a later rm then removed the container. + expect(readFileSync(busy, 'utf8').trim()).toBe('0'); + expect(readFileSync(rmLog, 'utf8').trim().split('\n')).toContain('found'); + }, 60_000); + it('settles at once on a CLI whose rm exits non-zero for a missing name', async () => { + writeFileSync(mode, 'refused'); writeFileSync(oldCli, ''); + const started = performance.now(); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125)'); + expect(performance.now() - started).toBeLessThan(5_000); + expect(removals()).toEqual([`rm --force --volumes ${probeName()}`, `container inspect ${probeName()}`]); + }); + it('gives up on time when the daemon stops answering the cleanup', async () => { + // Every rm and inspect hangs: each call must be cut at the time left, not at its own 10 s deadline. + writeFileSync(mode, 'refused'); writeFileSync(hangAt, '0'); + const started = performance.now(); + await expect(probeTaskStoragePathIdentity(imageId, 60_000, 1_000)).rejects.toThrow('may remain'); + // One hung call, cut at the 1 s floor, then the give-up: a floor much over 1 s at the call site fails here. + expect(performance.now() - started).toBeLessThan(2_500); + }, 60_000); + it('starts no call after giving up is due, when the daemon stops answering partway', async () => { + // Present (removal in progress) at first, then no answer from about four seconds before the give-up time. Only the + // call that was running then may hang; starting another would stretch the bound by a second floor. + writeFileSync(mode, 'created'); writeFileSync(busy, '100000'); + writeFileSync(hangAt, String(Math.floor(Date.now() / 1000) + 12)); + const failure = await probeTaskStoragePathIdentity(imageId, 2_000, 4_000).catch((error: Error) => error); + // The interleaving this test is about: the create landed before the client was killed. + expect(existsSync(landAt)).toBe(true); + expect(readFileSync(hung, 'utf8').trim().split('\n')).toHaveLength(1); + expect(((failure as Error).cause as AggregateError).errors[1].message).toBe(`Docker did not confirm that ${probeName()} is gone.`); + }, 60_000); + it('gives up with the last outcome, not an earlier absent answer, when the daemon stops answering', async () => { + // Absent at every answered ask (a killed client whose create never landed), then no answer before the window ends: + // absence inside the settle window proves nothing, so this is a give-up whose cause is the unanswered call. + writeFileSync(mode, 'hang'); writeFileSync(hangAt, String(Math.floor(Date.now() / 1000) + 5)); + const failure = await probeTaskStoragePathIdentity(imageId, 500, 2_000).catch((error: Error) => error); + // The interleaving this test is about: an ask was answered "absent" before Docker stopped answering. + expect(readFileSync(rmLog, 'utf8').trim().split('\n')[0]).toBe('missing'); + const cleanup = ((failure as Error).cause as AggregateError).errors[1] as Error; + expect(cleanup.message).toBe(`Docker did not confirm that ${probeName()} is gone.`); + expect(cleanup.cause).toMatchObject({ status: null, error: { code: 'ETIMEDOUT' } }); + }, 60_000); + it('waits out the settle window, then gives up, when docker run exits without a daemon answer', async () => { + // Exit 125 from the client itself: with no daemon answer, the probe cannot know no create was sent, and with the + // daemon still unreachable nothing can confirm the container is gone. + writeFileSync(mode, 'unreachable'); + const started = performance.now(); + const failure = await probeTaskStoragePathIdentity(imageId, 60_000, 1_000).catch((error: Error) => error); + expect((failure as Error).message).toMatch(/^docker run failed \(exit 125\): docker: Cannot connect to the Docker daemon[^]* The probe container codeboost-path-identity-[0-9a-f-]+ may remain;/); + expect(performance.now() - started).toBeGreaterThanOrEqual(10_000); + }, 60_000); + it.each(['garbled', 'preceded'])('refuses a probe that exits 0 without exactly one identity line (%s)', async garble => { + writeFileSync(mode, garble); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('The path identity probe did not report an identity.'); + }); + it.each([ + ['false none', { caseSensitive: false, unicodeNormalization: 'none' }], + ['true NFC', { caseSensitive: true, unicodeNormalization: 'NFC' }], + ['false NFC', { caseSensitive: false, unicodeNormalization: 'NFC' }], + ] as const)('reports what the container measured: %s', async (line, expected) => { + // Each field on its own: a probe that returned tmpfs's answer whatever the container said would fail here. + writeFileSync(mode, 'answer'); writeFileSync(identity, `codeboost-path-identity ${line}\n`); + expect(await probeTaskStoragePathIdentity(imageId)).toEqual(expected); + }); + it('cuts each cleanup call at 10 s, so a daemon that answers again is asked again', async () => { + // A killed client (10 s settle window), then every rm and inspect hangs. With the cap the first call is cut at + // 10 s and a second runs until giving up; without it the first would hang all the way to the give-up time. + writeFileSync(mode, 'hang'); writeFileSync(hangAt, '0'); + await expect(probeTaskStoragePathIdentity(imageId, 500, 2_000)).rejects.toThrow('may remain'); + expect(readFileSync(hung, 'utf8').trim().split('\n')).toEqual(['rm', 'container']); + }, 60_000); + it('gives each cleanup call at least 1 s, so a slow daemon is still heard at the give-up time', async () => { + // Almost no time left: without the floor the first call would be cut before Docker could answer it. + writeFileSync(mode, 'refused'); writeFileSync(slow, ''); + const failure = await probeTaskStoragePathIdentity(imageId, 60_000, 1).catch((error: Error) => error); + const cleanup = ((failure as Error).cause as AggregateError).errors[1] as Error; + expect((cleanup.cause as { status: number | null }).status).not.toBeNull(); + }, 60_000); + it('judges the settle window by when an inspect was asked, not when it answered', async () => { + // An absent answer asked inside the window, but arriving after it, while the create lands: not proof of absence. + writeFileSync(mode, 'hang'); writeFileSync(slowLand, ''); + await expect(probeTaskStoragePathIdentity(imageId, 500)).rejects.toThrow('client killed or not started'); + // The interleaving this test is about: the slow inspect answered "absent" (it was not cut at its deadline), the + // create landed while it ran, and cleanup asked again after it. + expect(existsSync(slowAnswered)).toBe(true); + expect(existsSync(landAt)).toBe(true); + expect(readFileSync(inspects, 'utf8').split('\n').filter(Boolean).length).toBeGreaterThan(6); + // And a later rm removed it. + expect(existsSync(removed)).toBe(true); + }, 60_000); + it('gives up 30 s after the settle window by default', async () => { + // The production default: an unreachable daemon (no daemon answer, so the 10 s window applies), then 30 s of asks. + writeFileSync(mode, 'unreachable'); + const started = performance.now(); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('may remain'); + const elapsed = performance.now() - started; + // 10 s settle + 30 s of asks; each fake call answers at once, so the overshoot is the last pause and call. + expect(elapsed).toBeGreaterThanOrEqual(40_000); + expect(elapsed).toBeLessThan(43_000); + }, 90_000); + it('gives each cleanup call the time left, capped at exactly 10 s and at least exactly 1 s', () => { + // The helper's values, exactly. The timing tests above show the cleanup calls use it, to within about 1–2 s (a cap + // of roughly 9–12 s and a floor of roughly 0.4–2.5 s), not exactly. + expect([-5, 0, 1, 999, 999.2, 1_000, 1_001, 4_200.5, 9_999, 10_000, 10_001, 40_000].map(probeCleanupCallMs)) + .toEqual([1_000, 1_000, 1_000, 1_000, 1_000, 1_000, 1_001, 4_201, 9_999, 10_000, 10_000, 10_000]); + }); + it('waits 10 s for an unreported outcome and keeps asking 30 s more, exactly', () => { + // The values the cleanup uses (the default-bound timing test shows they are the ones in effect, to within 3 s). + expect([PROBE_SETTLE_MS, PROBE_CLEANUP_MS]).toEqual([10_000, 30_000]); + }); + it('removes nothing, and does not wait, when spawning docker throws (E2BIG)', async () => { + // Node throws from spawn itself (syscall `spawn`) for an environment the OS refuses: nothing reached Docker. + writeFileSync(mode, 'answer'); + const saved = process.env.DOCKER_HOST; + process.env.DOCKER_HOST = 'x'.repeat(4 * 1024 * 1024); + const started = performance.now(); + let failure: Error | undefined; + try { await probeTaskStoragePathIdentity(imageId); } catch (error) { failure = error as Error; } + finally { if (saved === undefined) delete process.env.DOCKER_HOST; else process.env.DOCKER_HOST = saved; } + // The path this test is about: the synchronous throw, not the asynchronous `spawn docker` error event. + expect((failure!.cause as NodeJS.ErrnoException).syscall).toBe('spawn'); + expect(failure!.message).toMatch(/^docker run failed \(client killed or not started\): spawn E2BIG/); + expect(performance.now() - started).toBeLessThan(2_000); + expect(existsSync(calls)).toBe(false); + }); +}); diff --git a/test/path-identity.test.ts b/test/path-identity.test.ts new file mode 100644 index 00000000..6ce22134 --- /dev/null +++ b/test/path-identity.test.ts @@ -0,0 +1,74 @@ +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { PATH_IDENTITY_SCRIPT } from '../agents/container/storage.ts'; +import { assertPathIdentity, probePathIdentity } from '../runner/path-identity.ts'; + +const roots: string[] = []; +afterEach(() => { for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); }); +const directory = () => { const root = mkdtempSync(join(tmpdir(), 'codeboost-path-identity-')); roots.push(root); return root; }; + +describe('host path identity probe', () => { + it('reports the filesystem identity and removes its probe files', () => { + const root = directory(); + const probed = probePathIdentity(root); + expect(typeof probed.caseSensitive).toBe('boolean'); + expect(['none', 'NFC']).toContain(probed.unicodeNormalization); + expect(readdirSync(root)).toEqual([]); + // The same filesystem answers the same way twice. + expect(probePathIdentity(root)).toEqual(probed); + }); + it.runIf(process.platform === 'linux')('reports a Linux filesystem as case-sensitive without normalization', () => { + // The CI runner's ext4: a probe that compared the same spelling twice would report NFC here. + expect(probePathIdentity(directory())).toEqual({ caseSensitive: true, unicodeNormalization: 'none' }); + }); + it('fails when the directory cannot hold the probe', () => { + expect(() => probePathIdentity(join(directory(), 'missing'))).toThrow(); + }); +}); + +describe('path identity check', () => { + const identity = { caseSensitive: true, unicodeNormalization: 'none' } as const; + it('accepts an identical identity and refuses one that differs on case or Unicode', () => { + expect(() => assertPathIdentity(identity, { ...identity }, 'the storage')).not.toThrow(); + expect(() => assertPathIdentity(identity, { ...identity, caseSensitive: false }, 'the storage')) + .toThrow('pathIdentity (caseSensitive true, unicodeNormalization none) does not match the storage (caseSensitive false, unicodeNormalization none). Set pathIdentity to the probed values.'); + expect(() => assertPathIdentity(identity, { ...identity, unicodeNormalization: 'NFC' }, 'the storage')) + .toThrow('does not match the storage (caseSensitive true, unicodeNormalization NFC)'); + }); +}); + +describe('task storage probe script', () => { + // The container probe runs this script; here it runs on a host folder whose answer the host probe knows. A script + // that stopped measuring and printed a constant fails here unless the constant is this host's own answer (`false NFC` + // on macOS APFS, `true none` on Linux CI). + it('reports what the host probe reports for the same folder', () => { + const root = directory(); + const out = execFileSync('sh', ['-c', PATH_IDENTITY_SCRIPT, 'probe', root], { encoding: 'utf8' }); + const expected = probePathIdentity(root); + expect(out).toBe(`codeboost-path-identity ${expected.caseSensitive} ${expected.unicodeNormalization}\n`); + // What it wrote, as the host lists it: `case` and the NFC spelling, on any host (both are stored as given). + expect(readdirSync(root).sort()).toEqual(['caf\u00e9', 'case']); + }); + it('reports what it finds, on any host: names that already answer to CASE and NFD read as folded', () => { + // Pre-seeded names make the honest answer `false NFC` on any host, so a constant other than that fails here + // everywhere. A script printing `false NFC` itself passes both tests on macOS; Linux CI (above) and the real-Docker + // test (task storage answers `true none`) catch it. + const root = directory(); + writeFileSync(join(root, 'CASE'), ''); writeFileSync(join(root, 'cafe\u0301'), ''); + expect(execFileSync('sh', ['-c', PATH_IDENTITY_SCRIPT, 'probe', root], { encoding: 'utf8' })).toBe('codeboost-path-identity false NFC\n'); + }); + it.each([ + ['CASE', 'case'], + ['cafe\u0301', 'Unicode'], + ] as const)('reads each field from its own name: only %s present folds only %s', (seed, field) => { + // One seeded name at a time, so a script whose case check looked at the NFD name (or the reverse) fails on a host + // that folds neither (Linux CI); the other field must read as this host reads it. + const root = directory(), host = probePathIdentity(directory()); + writeFileSync(join(root, seed), ''); + const expected = field === 'case' ? `false ${host.unicodeNormalization}` : `${host.caseSensitive} NFC`; + expect(execFileSync('sh', ['-c', PATH_IDENTITY_SCRIPT, 'probe', root], { encoding: 'utf8' })).toBe(`codeboost-path-identity ${expected}\n`); + }); +}); diff --git a/test/runner-production.test.ts b/test/runner-production.test.ts index b30ec341..bf33693d 100644 --- a/test/runner-production.test.ts +++ b/test/runner-production.test.ts @@ -80,6 +80,8 @@ const recovery = (calls: string[], over: Partial = {}): RecoveryDe }); const lock = (calls: string[], fail = false) => ({ file: { dev: 1n, ino: 2n }, verify: () => { calls.push('verify'); if (fail) throw new Error('The database path changed while opening. Refusing to start.'); } }); const capability = { run: (fn: () => T) => fn() }; +// Task storage that agrees with the configured identity, so these tests need no Docker (the real probe is in runner-workspace). +const matching = (service: ReviewService) => async () => ({ ...service.config.pathIdentity }); describe('runner configuration', () => { it('accepts a complete block and refuses relative paths, bad committers and unknown limits', () => { @@ -98,9 +100,30 @@ describe('runner configuration', () => { }); describe('runner startup', () => { + it('refuses to start when pathIdentity disagrees with the task storage the agent edits (#162)', async () => { + const { root, service } = fixture(), configured = service.config.pathIdentity; + for (const probed of [{ ...configured, caseSensitive: !configured.caseSensitive }, + { ...configured, unicodeNormalization: configured.unicodeNormalization === 'NFC' ? 'none' as const : 'NFC' as const }]) { + const calls: string[] = []; + await expect(setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => { calls.push('image'); return 'sha256:x'; }, recovery: () => { calls.push('deps'); return recovery(calls); }, + probeTaskStorage: async imageId => { calls.push(`probe ${imageId}`); return probed; } })) + .rejects.toThrow(`pathIdentity (caseSensitive ${configured.caseSensitive}, unicodeNormalization ${configured.unicodeNormalization}) does not match ` + + `the task storage the agent edits (caseSensitive ${probed.caseSensitive}, unicodeNormalization ${probed.unicodeNormalization})`); + // Probed with the built image, after recovery and the image build. + expect(calls).toEqual(['verify', 'deps', 'recover', 'image', 'probe sha256:x']); + } + // A probe that fails stops startup too: an identity that cannot be measured is not assumed. + await expect(setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'sha256:x', recovery: () => recovery([]), probeTaskStorage: () => Promise.reject(new Error('docker run failed')) })) + .rejects.toThrow('docker run failed'); + // The probed identity itself starts. + await expect(setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'sha256:x', recovery: () => recovery([]), probeTaskStorage: async () => ({ ...configured }) })).resolves.toBeDefined(); + }); it('verifies the lock, recovers, then builds the image, before it assembles anything', async () => { const { root, service } = fixture(), calls: string[] = []; - const assembly = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + const assembly = await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => { calls.push('image'); return 'sha256:x'; }, recovery: () => { calls.push('deps'); return recovery(calls); } }); // D's recovery stops every leftover agent before the (possibly long) image build. expect(calls).toEqual(['verify', 'deps', 'recover', 'image']); @@ -128,14 +151,14 @@ describe('runner startup', () => { const { root, service } = fixture(), calls: string[] = []; const actionId = randomUUID(); service.store.beginMergeJob(service.config.identity, { actionId, token: 'review-token' }); - await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'sha256:x', recovery: () => recovery(calls) }); expect(service.store.getMergeJob(service.config.identity, actionId)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/server restarted/) }); }); it('pushes a prepared rewrite only under an exact lease on the PR head it rewrote', async () => { const { root, service } = fixture(), calls: string[] = []; - const assembly = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + const assembly = await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'sha256:x', recovery: () => recovery(calls) }); const push = vi.spyOn(GitBranchPusher.prototype, 'push').mockResolvedValue(); const read = vi.spyOn(GitBranchPusher.prototype, 'readBranch').mockResolvedValue(null); @@ -160,7 +183,7 @@ describe('runner startup', () => { const { root, service } = fixture(); service.store.setIssueTrust({ repository: 'owner/repo', issue: service.store.getPlan(service.config.identity).issue, authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); - const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + const { sources } = await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([]) }); issueReads.length = 0; const signal = new AbortController().signal, identity = service.config.identity; @@ -179,7 +202,7 @@ describe('runner startup', () => { }); it('does not reuse collaborator-filtered comments without a complete collaborator snapshot', async () => { const { root, service } = fixture(); - const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + const { sources } = await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([]) }); issueReads.length = 0; collaboratorComments = ['Former collaborator note.']; @@ -197,7 +220,7 @@ describe('runner startup', () => { issueAuthor = 'outside'; issueCollaborator = false; service.store.setIssueTrust({ repository: 'owner/repo', issue: service.store.getPlan(service.config.identity).issue, authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); - const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + const { sources } = await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([]) }); await expect(sources.issue(service.config.identity, new AbortController().signal)).resolves.toMatchObject({ text: { comments: ['Outside note.'] } }); service.store.setIssueTrust({ repository: 'owner/repo', issue: service.store.getPlan(service.config.identity).issue, @@ -206,7 +229,7 @@ describe('runner startup', () => { }); it('refuses when access changes between the execute admission and text read', async () => { const { root, service } = fixture(); - const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + const { sources } = await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([]) }); // The gateway's first read admits the collaborator; its text read observes the changed current access and refuses. afterIssueAccess = () => { issueCollaborator = false; }; @@ -217,7 +240,7 @@ describe('runner startup', () => { issueAuthor = 'outside'; issueCollaborator = false; const issue = service.store.getPlan(service.config.identity).issue; service.store.setIssueTrust({ repository: 'owner/repo', issue, authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); - const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + const { sources } = await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([]) }); issueReads.length = 0; beforeIssueTextReturn = () => { service.store.setIssueTrust({ repository: 'owner/repo', issue, authorLogin: issueAuthor, @@ -233,7 +256,7 @@ describe('runner startup', () => { issueAuthor = 'outside'; issueCollaborator = false; const issue = service.store.getPlan(service.config.identity).issue; service.store.setIssueTrust({ repository: 'owner/repo', issue, authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); - const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + const { sources } = await setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([]) }); const guarded = await sources.issue(service.config.identity, new AbortController().signal); expect(guarded.text.comments).toEqual(['Outside note.']); @@ -244,7 +267,7 @@ describe('runner startup', () => { for (const [patch, env, message] of [[{ github: undefined }, { CLAUDE_CODE_OAUTH_TOKEN: 't' }, /github block/], [{}, {}, RUNNER_CREDENTIAL_MISSING], [{ demo: true }, { CLAUDE_CODE_OAUTH_TOKEN: 't' }, /Demos never/]] as const) { const { root, service, config } = fixture(), calls: string[] = []; service.config = { ...config, ...patch }; - await expect(setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock(calls), env, + await expect(setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock(calls), env, buildImage: () => { calls.push('image'); return 'x'; }, recovery: () => recovery(calls) })).rejects.toThrow(message); expect(calls).toEqual([]); expect(existsSync(join(root, 'runner'))).toBe(false); @@ -252,16 +275,16 @@ describe('runner startup', () => { }); it('stops startup when the lock no longer names the database, or recovery fails, and admits nothing', async () => { const { root, service } = fixture(), calls: string[] = []; - await expect(setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock(calls, true), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + await expect(setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock(calls, true), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => { calls.push('image'); return 'x'; }, recovery: () => recovery(calls) })).rejects.toThrow(/path changed/); expect(calls).toEqual(['verify']); - await expect(setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + await expect(setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([], { recoverLeftovers: async () => { throw new Error('docker down'); } }) })).rejects.toThrow(/docker down/); }); it('refuses a configured runnerRepository that is not the runner\'s own', async () => { const { root, service, config } = fixture(); service.config = { ...config, runnerRepository: join(root, 'elsewhere.git') }; - await expect(setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + await expect(setUpRunner({ service, capability, probeTaskStorage: matching(service), config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([]) })).rejects.toThrow(/is not the runner's repository/); }); }); diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts index dff74939..0ac8ef92 100644 --- a/test/runner-workspace.test.ts +++ b/test/runner-workspace.test.ts @@ -1,3 +1,4 @@ +import { randomUUID } from 'node:crypto'; import { execFile, execFileSync } from 'node:child_process'; import { existsSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; @@ -5,7 +6,7 @@ import { join } from 'node:path'; import { promisify } from 'node:util'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { buildAgentImage } from '../agents/container/image.ts'; -import type { TaskFilesystems } from '../agents/container/storage.ts'; +import { PROBE_VOLUME_MOUNT, probeTaskStoragePathIdentity, type TaskFilesystems } from '../agents/container/storage.ts'; import type { Plan, PlanContext } from '../core/plan.ts'; import { RunnerCoordinator } from '../runner/coordinator.ts'; import { ItemExecutor, SafetyFindings, executionDeps, type ExecutionSources } from '../runner/execution.ts'; @@ -145,3 +146,41 @@ describe('real task workspace (#87)', () => { review.close(); }, 600_000); }); + +describe('task storage path identity (#162)', () => { + // Only volumes made with the probe's own tmpfs options count, so other users of the daemon do not change the result. + // Precondition: no other probe runs on this daemon during the test (another runner's startup, or this file run from + // another checkout); its short-lived container and volume would match too. The docker project runs files one at a + // time, and no other file probes. + const probeVolumes = () => new Set(docker('volume', 'ls', '-q').split('\n').filter(Boolean).filter(volume => { + try { return docker('volume', 'inspect', '--format', '{{json .Options}}', volume).includes('size=1048576,nr_inodes=64,'); } + catch { return false; } // removed between the listing and the inspect + })); + it('measures the tmpfs the agent edits, whatever the host folds, and leaves no container or volume behind', async () => { + const before = probeVolumes(); + // Task storage is a tmpfs in the daemon's Linux kernel: case-sensitive, and names are compared as bytes. A macOS + // host folder would report the reverse, which is why the probe does not read the host. + expect(await probeTaskStoragePathIdentity(imageId)).toEqual({ caseSensitive: true, unicodeNormalization: 'none' }); + // In any state: a container created but never started would outlive --rm. + expect(docker('ps', '-aq', '--filter', 'name=codeboost-path-identity-')).toBe(''); + expect([...probeVolumes()].filter(volume => !before.has(volume))).toEqual([]); + }, 120_000); + it('removes a never-started probe container and its volume with the cleanup\'s own rm', () => { + // The failure path's claim: a container created but never started (a killed client) loses its anonymous volume to + // `rm --force --volumes`. Made with the probe's exact mount; removed with the exact command the cleanup runs. + const name = `codeboost-path-identity-${randomUUID()}`; + docker('create', '--name', name, '--mount', PROBE_VOLUME_MOUNT, '--entrypoint', 'true', imageId); + let volume = ''; + try { + // This container's own volume, by name: the anonymous volume the mount made. + volume = docker('container', 'inspect', '--format', '{{range .Mounts}}{{.Name}}{{end}}', name); + expect(volume).toMatch(/^[0-9a-f]{64}$/); + expect(docker('volume', 'inspect', '--format', '{{json .Options}}', volume)).toContain('size=1048576,nr_inodes=64,'); + } finally { docker('rm', '--force', '--volumes', name); } + expect(() => docker('volume', 'inspect', volume)).toThrow(); + }, 120_000); + it('runs only the image the trusted builder produced', async () => { + await expect(probeTaskStoragePathIdentity(`sha256:${'0'.repeat(64)}`)).rejects.toThrow('Agent image was not produced by the trusted validated builder.'); + await expect(probeTaskStoragePathIdentity('alpine')).rejects.toThrow('requires the immutable built image ID'); + }); +});