From 234486dbb3616dfb61ed4f82ac9fbaef847ced6b Mon Sep 17 00:00:00 2001 From: eastagiletracker <310448263+eastagiletracker@users.noreply.github.com> Date: Mon, 12 Oct 2026 07:37:20 +0000 Subject: [PATCH 01/40] Probe checkout path identity at runner startup Runner startup now probes case sensitivity and Unicode normalization on the filesystem that holds the task checkouts and refuses to start when the configured pathIdentity disagrees. The demo uses the same shared probe. --- docs/implementation/read-only-review.md | 2 +- runner/path-identity.ts | 29 ++++++++++++++++++++++ runner/production.ts | 4 +++ scripts/demo.ts | 8 +++--- test/path-identity.test.ts | 33 +++++++++++++++++++++++++ test/runner-production.test.ts | 22 +++++++++++++++++ 6 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 runner/path-identity.ts create mode 100644 test/path-identity.test.ts diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index fe6817b3..fb2b6e07 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 the filesystem that holds the task checkouts (`runner.root`) and refuses to start when `pathIdentity` disagrees, naming the probed values; the setting is kept, not derived, so a review never silently changes its plan path keys. 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/runner/path-identity.ts b/runner/path-identity.ts new file mode 100644 index 00000000..fcd01492 --- /dev/null +++ b/runner/path-identity.ts @@ -0,0 +1,29 @@ +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; + +/** How a checkout's filesystem tells two path spellings apart (#162). */ +export interface PathIdentity { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' } + +/** + * Probes the filesystem that holds `directory`: writes `case` and NFC `café` in a fresh child directory, then asks + * whether `CASE` and NFD `café` name the same files. The child directory is removed again. + */ +export function probePathIdentity(directory: string): PathIdentity { + const probe = mkdtempSync(join(directory, 'identity-')); + try { + writeFileSync(join(probe, 'case'), ''); + writeFileSync(join(probe, 'café'), ''); + return { caseSensitive: !existsSync(join(probe, 'CASE')), unicodeNormalization: existsSync(join(probe, 'café')) ? 'NFC' : 'none' }; + } finally { rmSync(probe, { recursive: true, force: true }); } +} + +/** + * Refuses a configured `pathIdentity` that disagrees with the filesystem under `directory`. 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, directory: string): void { + const probed = probePathIdentity(directory); + 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 the filesystem that holds the task checkouts at ${directory} (${describe(probed)}). Set pathIdentity to the probed values.`); +} diff --git a/runner/production.ts b/runner/production.ts index cf015259..494593f3 100644 --- a/runner/production.ts +++ b/runner/production.ts @@ -26,6 +26,7 @@ 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'; /** * The production runner (#91): D's real workspace, launcher, recovery, export and removal, under the database's runner @@ -215,6 +216,9 @@ export async function setUpRunner(o: { service: ReviewService; capability: Shutd o.lock.verify(); const runnerOwner = service.store.runnerOwnerToken(o.lock.file); ownerOnlyDirectory(config.root); + // #162: plan path keys follow pathIdentity, so it must match the filesystem the task checkouts live on. Checked before + // recovery and the image build, so a wrong setting is reported at once. + assertPathIdentity(review.pathIdentity, ownerOnlyDirectory(config.root, runnerOwner, 'attempts')); // Per database (its runner token): retention reads references from this Store only, so it must never see another // database's files. The lock is per database, so two runners can share a root or a configured directory. const diagnosticsDir = ownerOnlyDirectory(config.diagnosticsDir ?? config.root, runnerOwner, 'diagnostics'); 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/path-identity.test.ts b/test/path-identity.test.ts new file mode 100644 index 00000000..dad35d7a --- /dev/null +++ b/test/path-identity.test.ts @@ -0,0 +1,33 @@ +import { mkdtempSync, readdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +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('checkout 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('accepts the probed identity and refuses one that disagrees on case or Unicode', () => { + const root = directory(), probed = probePathIdentity(root); + expect(() => assertPathIdentity(probed, root)).not.toThrow(); + expect(() => assertPathIdentity({ ...probed, caseSensitive: !probed.caseSensitive }, root)) + .toThrow(`pathIdentity (caseSensitive ${!probed.caseSensitive}, unicodeNormalization ${probed.unicodeNormalization}) does not match`); + const other = probed.unicodeNormalization === 'NFC' ? 'none' : 'NFC'; + expect(() => assertPathIdentity({ ...probed, unicodeNormalization: other }, root)).toThrow(`unicodeNormalization ${other}) does not match`); + expect(readdirSync(root)).toEqual([]); + }); + it('fails when the directory cannot hold the probe', () => { + expect(() => probePathIdentity(join(directory(), 'missing'))).toThrow(); + }); +}); diff --git a/test/runner-production.test.ts b/test/runner-production.test.ts index b30ec341..6fb3ac98 100644 --- a/test/runner-production.test.ts +++ b/test/runner-production.test.ts @@ -14,6 +14,7 @@ import type { InvocationInput } from '../agents/contract.ts'; import { ISSUE_READ_ACTIVE_MS, type IssueAccess } from '../github/issues.ts'; import { recoverLeftovers } from '../agents/recovery.ts'; import { exportTaskDiff, removeTaskFilesystemsAsync } from '../agents/container/storage.ts'; +import { probePathIdentity } from '../runner/path-identity.ts'; import { RunnerCoordinator, type RunnerDeps } from '../runner/coordinator.ts'; import { SafetyFindings, type ExecutionSources } from '../runner/execution.ts'; import type { GitRebaser } from '../runner/rebase.ts'; @@ -98,6 +99,27 @@ describe('runner configuration', () => { }); describe('runner startup', () => { + it('refuses to start when pathIdentity disagrees with the filesystem that holds the task checkouts (#162)', async () => { + const { root, service } = fixture(), runnerRoot = join(root, 'runner'); + mkdirSync(runnerRoot, { mode: 0o700 }); + const probed = probePathIdentity(runnerRoot); + for (const pathIdentity of [{ ...probed, caseSensitive: !probed.caseSensitive }, + { ...probed, unicodeNormalization: probed.unicodeNormalization === 'NFC' ? 'none' as const : 'NFC' as const }]) { + service.config = { ...service.config, pathIdentity }; + const calls: string[] = []; + await expect(setUpRunner({ service, capability, config: { root: runnerRoot, committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => { calls.push('image'); return 'x'; }, recovery: () => { calls.push('deps'); return recovery(calls); } })) + .rejects.toThrow(/pathIdentity .* does not match the filesystem that holds the task checkouts/); + // Refused before recovery and the image build, and the probe leaves nothing behind. + expect(calls).toEqual(['verify']); + const attempts = join(runnerRoot, service.store.runnerOwnerToken({ dev: 1n, ino: 2n }), 'attempts'); + expect(readdirSync(attempts)).toEqual([]); + } + // The probed identity itself starts. + service.config = { ...service.config, pathIdentity: probed }; + await expect(setUpRunner({ service, capability, config: { root: runnerRoot, committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'x', recovery: () => recovery([]) })).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' }, From cbfd6dd26e8797b6376b73100d814b73cadc00f4 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 11:45:44 -0700 Subject: [PATCH 02/40] Probe task storage, not the host folder, for path identity (#162) The agent edits, and the runner inspects, a tmpfs volume in the Docker daemon's Linux kernel. The host attempts folder holds only the staging clone. On a macOS host the earlier probe reported APFS's folding and refused the setting that is correct for task storage. Startup now runs the probe with the built image, in a hardened --rm container on an anonymous volume made with task storage's own tmpfs options, and refuses on a mismatch. The host probe stays for the demo. The staging clone's own case collapse is #179. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 36 ++++++++++- docs/implementation/plan-library-alignment.md | 2 +- docs/implementation/read-only-review.md | 2 +- runner/path-identity.ts | 22 +++---- runner/production.ts | 20 +++--- test/cli.test.ts | 25 ++++++-- test/path-identity.test.ts | 24 ++++--- test/runner-production.test.ts | 63 ++++++++++--------- test/runner-workspace.test.ts | 18 +++++- 9 files changed, 143 insertions(+), 69 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 194e755c..75560e0b 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -3,6 +3,7 @@ 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'; @@ -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,34 @@ export async function prepareTaskFilesystemsAsync(clone: TaskClone, limits: Task } } +// 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. +const PATH_IDENTITY_SCRIPT = ['set -eu', 'cd /work', ': > 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 path identity of task storage (#162): how the filesystem the agent edits and the runner inspects 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 seeder. The volume is anonymous and the container `--rm`, so the daemon removes both when + * the probe exits, even if this process does not live to see it. + */ +export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000): 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 volume = ['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(','); + const args = ['run', '--rm', '--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', volume, '--entrypoint', 'sh', imageId, '-c', PATH_IDENTITY_SCRIPT]; + const outcome = await runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs }); + if (outcome.status !== 0) throw new DockerError(args, outcome); + 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'] }; +} + /** * 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..cd65f8ea 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` | | 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 fb2b6e07..9c1f322b 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. With a `runner` block, startup probes case sensitivity and Unicode normalization on the filesystem that holds the task checkouts (`runner.root`) and refuses to start when `pathIdentity` disagrees, naming the probed values; the setting is kept, not derived, so a review never silently changes its plan path keys. 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 the runner inspects, 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. 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/runner/path-identity.ts b/runner/path-identity.ts index fcd01492..6fae713f 100644 --- a/runner/path-identity.ts +++ b/runner/path-identity.ts @@ -1,29 +1,27 @@ import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; - -/** How a checkout's filesystem tells two path spellings apart (#162). */ -export interface PathIdentity { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' } +import type { PathIdentity } from '../core/path-identity.ts'; /** - * Probes the filesystem that holds `directory`: writes `case` and NFC `café` in a fresh child directory, then asks - * whether `CASE` and NFD `café` name the same files. The child directory is removed again. + * 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é'), ''); - return { caseSensitive: !existsSync(join(probe, 'CASE')), unicodeNormalization: existsSync(join(probe, 'café')) ? 'NFC' : 'none' }; + 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 filesystem under `directory`. 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. + * 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, directory: string): void { - const probed = probePathIdentity(directory); +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 the filesystem that holds the task checkouts at ${directory} (${describe(probed)}). Set pathIdentity to the probed values.`); + 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 494593f3..f12f666f 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'; @@ -27,6 +27,7 @@ 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 @@ -197,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); @@ -216,9 +220,6 @@ export async function setUpRunner(o: { service: ReviewService; capability: Shutd o.lock.verify(); const runnerOwner = service.store.runnerOwnerToken(o.lock.file); ownerOnlyDirectory(config.root); - // #162: plan path keys follow pathIdentity, so it must match the filesystem the task checkouts live on. Checked before - // recovery and the image build, so a wrong setting is reported at once. - assertPathIdentity(review.pathIdentity, ownerOnlyDirectory(config.root, runnerOwner, 'attempts')); // Per database (its runner token): retention reads references from this Store only, so it must never see another // database's files. The lock is per database, so two runners can share a root or a configured directory. const diagnosticsDir = ownerOnlyDirectory(config.diagnosticsDir ?? config.root, runnerOwner, 'diagnostics'); @@ -271,6 +272,11 @@ 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 and the runner inspects. + // 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. + 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/test/cli.test.ts b/test/cli.test.ts index aaefcd9e..7d4ee8f7 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -17,22 +17,26 @@ 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. */ 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 +193,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.test.ts b/test/path-identity.test.ts index dad35d7a..cbbaf5f9 100644 --- a/test/path-identity.test.ts +++ b/test/path-identity.test.ts @@ -8,7 +8,7 @@ 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('checkout path identity probe', () => { +describe('host path identity probe', () => { it('reports the filesystem identity and removes its probe files', () => { const root = directory(); const probed = probePathIdentity(root); @@ -18,16 +18,22 @@ describe('checkout path identity probe', () => { // The same filesystem answers the same way twice. expect(probePathIdentity(root)).toEqual(probed); }); - it('accepts the probed identity and refuses one that disagrees on case or Unicode', () => { - const root = directory(), probed = probePathIdentity(root); - expect(() => assertPathIdentity(probed, root)).not.toThrow(); - expect(() => assertPathIdentity({ ...probed, caseSensitive: !probed.caseSensitive }, root)) - .toThrow(`pathIdentity (caseSensitive ${!probed.caseSensitive}, unicodeNormalization ${probed.unicodeNormalization}) does not match`); - const other = probed.unicodeNormalization === 'NFC' ? 'none' : 'NFC'; - expect(() => assertPathIdentity({ ...probed, unicodeNormalization: other }, root)).toThrow(`unicodeNormalization ${other}) does not match`); - expect(readdirSync(root)).toEqual([]); + 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)'); + }); +}); diff --git a/test/runner-production.test.ts b/test/runner-production.test.ts index 6fb3ac98..8b7c4121 100644 --- a/test/runner-production.test.ts +++ b/test/runner-production.test.ts @@ -14,7 +14,6 @@ import type { InvocationInput } from '../agents/contract.ts'; import { ISSUE_READ_ACTIVE_MS, type IssueAccess } from '../github/issues.ts'; import { recoverLeftovers } from '../agents/recovery.ts'; import { exportTaskDiff, removeTaskFilesystemsAsync } from '../agents/container/storage.ts'; -import { probePathIdentity } from '../runner/path-identity.ts'; import { RunnerCoordinator, type RunnerDeps } from '../runner/coordinator.ts'; import { SafetyFindings, type ExecutionSources } from '../runner/execution.ts'; import type { GitRebaser } from '../runner/rebase.ts'; @@ -81,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', () => { @@ -99,30 +100,30 @@ describe('runner configuration', () => { }); describe('runner startup', () => { - it('refuses to start when pathIdentity disagrees with the filesystem that holds the task checkouts (#162)', async () => { - const { root, service } = fixture(), runnerRoot = join(root, 'runner'); - mkdirSync(runnerRoot, { mode: 0o700 }); - const probed = probePathIdentity(runnerRoot); - for (const pathIdentity of [{ ...probed, caseSensitive: !probed.caseSensitive }, - { ...probed, unicodeNormalization: probed.unicodeNormalization === 'NFC' ? 'none' as const : 'NFC' as const }]) { - service.config = { ...service.config, pathIdentity }; + 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: runnerRoot, committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, - buildImage: () => { calls.push('image'); return 'x'; }, recovery: () => { calls.push('deps'); return recovery(calls); } })) - .rejects.toThrow(/pathIdentity .* does not match the filesystem that holds the task checkouts/); - // Refused before recovery and the image build, and the probe leaves nothing behind. - expect(calls).toEqual(['verify']); - const attempts = join(runnerRoot, service.store.runnerOwnerToken({ dev: 1n, ino: 2n }), 'attempts'); - expect(readdirSync(attempts)).toEqual([]); + 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; refused before the workspace is assembled. + 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. - service.config = { ...service.config, pathIdentity: probed }; - await expect(setUpRunner({ service, capability, config: { root: runnerRoot, committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, - buildImage: () => 'x', recovery: () => recovery([]) })).resolves.toBeDefined(); + 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']); @@ -150,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); @@ -182,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; @@ -201,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.']; @@ -219,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, @@ -228,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; }; @@ -239,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, @@ -255,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.']); @@ -266,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); @@ -274,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..90c1c5ad 100644 --- a/test/runner-workspace.test.ts +++ b/test/runner-workspace.test.ts @@ -5,7 +5,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 { 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 +145,19 @@ describe('real task workspace (#87)', () => { review.close(); }, 600_000); }); + +describe('task storage path identity (#162)', () => { + it('measures the tmpfs the agent edits, whatever the host folds, and leaves no container or volume behind', async () => { + const volumes = () => new Set(docker('volume', 'ls', '-q').split('\n').filter(Boolean)); + const before = volumes(); + // 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' }); + expect(docker('ps', '-aq', '--filter', `ancestor=${imageId}`, '--filter', 'status=exited')).toBe(''); + expect([...volumes()].filter(volume => !before.has(volume))).toEqual([]); + }, 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'); + }); +}); From 52dec124970103ff155523594816165ce5ba839b Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 12:00:30 -0700 Subject: [PATCH 03/40] Narrow the task storage claim and test the probe script on the host Independent review of #180: the runner's host checkouts (the staging clone and the rebase worktree) are not task storage, so the comment and docs now say only what the probe covers, and point at #179. The probe script takes its directory, so a test runs it on a host folder and compares it with the host probe: a script that stopped measuring fails on a host that folds names. A test comment no longer claims an order the test does not check. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 11 ++++++----- docs/implementation/read-only-review.md | 2 +- runner/production.ts | 7 ++++--- test/path-identity.test.ts | 13 +++++++++++++ test/runner-production.test.ts | 2 +- 5 files changed, 25 insertions(+), 10 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 75560e0b..f56d737c 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -404,13 +404,14 @@ export async function prepareTaskFilesystemsAsync(clone: TaskClone, limits: Task } } -// 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. -const PATH_IDENTITY_SCRIPT = ['set -eu', 'cd /work', ': > case', ': > "$(printf \'caf\\303\\251\')"', +// 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 path identity of task storage (#162): how the filesystem the agent edits and the runner inspects tells two + * 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 seeder. The volume is anonymous and the container `--rm`, so the daemon removes both when * the probe exits, even if this process does not live to see it. @@ -424,7 +425,7 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = .map(field => field.includes(',') ? `"${field}"` : field).join(','); const args = ['run', '--rm', '--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', volume, '--entrypoint', 'sh', imageId, '-c', PATH_IDENTITY_SCRIPT]; + '--cpus=.25', '--mount', volume, '--entrypoint', 'sh', imageId, '-c', PATH_IDENTITY_SCRIPT, 'probe', '/work']; const outcome = await runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs }); if (outcome.status !== 0) throw new DockerError(args, outcome); const answer = /^codeboost-path-identity (true|false) (none|NFC)$/.exec(outcome.stdout.trim()); diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 9c1f322b..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. With a `runner` block, startup probes case sensitivity and Unicode normalization on task storage, the filesystem the agent edits and the runner inspects, 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. 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/runner/production.ts b/runner/production.ts index f12f666f..d276ef71 100644 --- a/runner/production.ts +++ b/runner/production.ts @@ -272,9 +272,10 @@ 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 and the runner inspects. - // 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. + // #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, diff --git a/test/path-identity.test.ts b/test/path-identity.test.ts index cbbaf5f9..06bac699 100644 --- a/test/path-identity.test.ts +++ b/test/path-identity.test.ts @@ -1,7 +1,9 @@ +import { execFileSync } from 'node:child_process'; import { mkdtempSync, readdirSync, rmSync } 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[] = []; @@ -37,3 +39,14 @@ describe('path identity check', () => { .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. On a macOS + // host (APFS folds both) a script that stopped measuring, and printed a constant, would disagree. + 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`); + }); +}); diff --git a/test/runner-production.test.ts b/test/runner-production.test.ts index 8b7c4121..bf33693d 100644 --- a/test/runner-production.test.ts +++ b/test/runner-production.test.ts @@ -110,7 +110,7 @@ describe('runner startup', () => { 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; refused before the workspace is assembled. + // 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. From f6181812bae64eb861a826c2f31e9ff10ec5fd5e Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 12:18:32 -0700 Subject: [PATCH 04/40] Remove a failed path identity probe by name Independent review of #180, round 2: --rm removes the probe only once its container has started. A client killed between create and start (its deadline, on a slow daemon) left an unnamed container and its volume that nothing would ever remove. The probe now names its container and, on any failure, removes it and its volume by that name, asking through the settle window when the client did not finish. If that does not settle, the error names the container to remove. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 36 ++++++++++++++-- test/path-identity-probe.test.ts | 70 ++++++++++++++++++++++++++++++++ test/runner-workspace.test.ts | 3 +- 3 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 test/path-identity-probe.test.ts diff --git a/agents/container/storage.ts b/agents/container/storage.ts index f56d737c..348a6d55 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -413,8 +413,10 @@ export const PATH_IDENTITY_SCRIPT = ['set -eu', 'cd "$1"', ': > case', ': > "$(p /** * 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 seeder. The volume is anonymous and the container `--rm`, so the daemon removes both when - * the probe exits, even if this process does not live to see it. + * container hardened like the seeder. 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: every failed + * probe therefore removes its container, and that volume, by the unique name it gave it. */ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000): Promise { if (!/^sha256:[0-9a-f]{64}$/.test(imageId)) throw new Error('The path identity probe requires the immutable built image ID.'); @@ -423,16 +425,42 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = ...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(','); - const args = ['run', '--rm', '--read-only', '--user', '10001:10001', '--network=none', '--cap-drop=ALL', + 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', volume, '--entrypoint', 'sh', imageId, '-c', PATH_IDENTITY_SCRIPT, 'probe', '/work']; const outcome = await runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs }); - if (outcome.status !== 0) throw new DockerError(args, outcome); + if (outcome.status !== 0) { + const failure = new DockerError(args, outcome); + // A client that did not finish leaves the daemon's outcome unknown: a killed create may still land, so only absence + // after the settle window counts. A daemon answer settles at once. + try { await removeProbeContainer(name, outcome.status === null ? CREATE_SETTLE_MS : 0); } + 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'] }; } +/** Remove a failed probe's container and its anonymous volume by name, until Docker reports it absent. */ +async function removeProbeContainer(name: string, settleMs: number): Promise { + const settleBy = performance.now() + settleMs, giveUpAt = performance.now() + settleMs + 30_000; + for (;;) { + const args = ['rm', '--force', '--volumes', name]; + const result = await runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs: 10_000 }); + // Removed: nothing else can create this name, so the probe is gone. + if (result.status === 0) return; + if (absent(result) && performance.now() >= settleBy) return; + // Absent within the settle window, or a removal Docker refused for now (one already in progress): ask again. + if (performance.now() >= giveUpAt) throw new DockerError(args, result); + 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/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts new file mode 100644 index 00000000..2101c5e7 --- /dev/null +++ b/test/path-identity-probe.test.ts @@ -0,0 +1,70 @@ +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 { 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'), removals = join(root, 'removals'); +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, refuse as the daemon does, or hang until the client is killed (a create that may have + // landed). `rm`: per the removals file, report the container removed or absent. + writeFileSync(join(bin, 'docker'), `#!/bin/sh +printf '%s\\n' "$*" >> '${calls}' +if [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; exit 0; fi +if [ "$1" = run ]; then + case "$(cat '${mode}')" in + answer) echo 'codeboost-path-identity true none';; + refused) echo 'docker: Error response from daemon: no such runtime' >&2; exit 125;; + hang) exec sleep 30;; + esac +fi +if [ "$1" = rm ]; then + if [ "$(cat '${removals}')" = removed ]; then exit 0; fi + echo "Error response from daemon: No such container: $4" >&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(() => { if (existsSync(calls)) rmSync(calls); writeFileSync(removals, 'absent'); }); +const logged = () => 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]!; + +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(logged()).toHaveLength(1); + }); + it('removes its container and volume by name when the daemon refuses the run', async () => { + writeFileSync(mode, 'refused'); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125): docker: Error response from daemon: no such runtime'); + expect(logged().slice(1)).toEqual([`rm --force --volumes ${probeName()}`]); + }); + it('removes a container a killed client may have created but never started', async () => { + writeFileSync(mode, 'hang'); writeFileSync(removals, 'removed'); + await expect(probeTaskStoragePathIdentity(imageId, 500)).rejects.toThrow('docker run failed (client killed or not started)'); + expect(logged().slice(1)).toEqual([`rm --force --volumes ${probeName()}`]); + }, 30_000); + it('keeps asking through the settle window while a killed create may still land', async () => { + writeFileSync(mode, 'hang'); + const started = performance.now(); + await expect(probeTaskStoragePathIdentity(imageId, 500)).rejects.toThrow('client killed or not started'); + // Absent at every ask, so it asked until the window closed rather than trusting the first answer. + expect(performance.now() - started).toBeGreaterThanOrEqual(10_000); + expect(logged().slice(1).length).toBeGreaterThan(1); + expect(new Set(logged().slice(1))).toEqual(new Set([`rm --force --volumes ${probeName()}`])); + }, 60_000); +}); diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts index 90c1c5ad..b5e7f0f3 100644 --- a/test/runner-workspace.test.ts +++ b/test/runner-workspace.test.ts @@ -153,7 +153,8 @@ describe('task storage path identity (#162)', () => { // 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' }); - expect(docker('ps', '-aq', '--filter', `ancestor=${imageId}`, '--filter', 'status=exited')).toBe(''); + // In any state: a container created but never started would outlive --rm. + expect(docker('ps', '-aq', '--filter', 'name=codeboost-path-identity-')).toBe(''); expect([...volumes()].filter(volume => !before.has(volume))).toEqual([]); }, 120_000); it('runs only the image the trusted builder produced', async () => { From 3ae86faa4cd4a6296bb571806849f0ff9b4b2e7a Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 12:45:59 -0700 Subject: [PATCH 05/40] Confirm a failed probe's removal by inspect, as Docker answers Independent review of #180, round 3: `docker rm --force` exits 0 for a name that does not exist (Docker 27), so the first removal ended the settle window and a create that landed after the killed client stayed behind. Removal is now confirmed by `container inspect` reporting the name absent after the window. A docker that never started sent nothing, so it is neither removed nor waited for. The test's fake docker now answers rm and inspect as Docker does, and can land a late create. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 27 +++++++++----- test/path-identity-probe.test.ts | 60 ++++++++++++++++++++++---------- 2 files changed, 59 insertions(+), 28 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 348a6d55..4ac88c99 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -7,7 +7,7 @@ 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'; @@ -432,6 +432,9 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 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; // A client that did not finish leaves the daemon's outcome unknown: a killed create may still land, so only absence // after the settle window counts. A daemon answer settles at once. try { await removeProbeContainer(name, outcome.status === null ? CREATE_SETTLE_MS : 0); } @@ -446,16 +449,22 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = return { caseSensitive: answer[1] === 'true', unicodeNormalization: answer[2] as PathIdentity['unicodeNormalization'] }; } -/** Remove a failed probe's container and its anonymous volume by name, until Docker reports it absent. */ +/** + * 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): Promise { - const settleBy = performance.now() + settleMs, giveUpAt = performance.now() + settleMs + 30_000; + const settleBy = performance.now() + settleMs, giveUpAt = settleBy + 30_000; + const docker = (args: readonly string[]) => runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs: 10_000 }); for (;;) { - const args = ['rm', '--force', '--volumes', name]; - const result = await runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs: 10_000 }); - // Removed: nothing else can create this name, so the probe is gone. - if (result.status === 0) return; - if (absent(result) && performance.now() >= settleBy) return; - // Absent within the settle window, or a removal Docker refused for now (one already in progress): ask again. + let args: readonly string[] = ['rm', '--force', '--volumes', name]; + let result = await docker(args); + if (result.status === 0) { + args = ['container', 'inspect', name]; + result = await docker(args); + if (absent(result) && performance.now() >= settleBy) return; + } + // Still there (a removal already in progress), not yet landed, or Docker did not answer: ask again. if (performance.now() >= giveUpAt) throw new DockerError(args, result); await pause(250); } diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 2101c5e7..dd873668 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -8,28 +8,36 @@ import { 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'), removals = join(root, 'removals'); +const calls = join(root, 'calls'), mode = join(root, 'mode'), landAt = join(root, 'land-at'), removed = join(root, 'removed'); 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, refuse as the daemon does, or hang until the client is killed (a create that may have - // landed). `rm`: per the removals file, report the container removed or absent. + // `run`: per the mode file, answer, refuse as the daemon does, or hang until the client is killed. In `late` mode the + // create lands two seconds later, 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. 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) echo 'codeboost-path-identity true none';; refused) echo 'docker: Error response from daemon: no such runtime' >&2; exit 125;; + created) echo 0 > '${landAt}'; exec sleep 30;; + late) echo $(( $(date +%s) + 2 )) > '${landAt}'; exec sleep 30;; hang) exec sleep 30;; esac fi if [ "$1" = rm ]; then - if [ "$(cat '${removals}')" = removed ]; then exit 0; fi - echo "Error response from daemon: No such container: $4" >&2; exit 1 + if exists; then : > '${removed}'; else echo "Error response from daemon: No such container: $4" >&2; fi + exit 0 +fi +if [ "$1 $2" = "container inspect" ]; then + if exists; then echo '[{}]'; exit 0; fi + echo "Error response from daemon: No such container: $3" >&2; exit 1 fi exit 0 `); @@ -38,33 +46,47 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { if (existsSync(calls)) rmSync(calls); writeFileSync(removals, 'absent'); }); -const logged = () => readFileSync(calls, 'utf8').trim().split('\n').filter(line => !line.startsWith('build ') && !line.startsWith('image ')); +beforeEach(() => { for (const file of [calls, landAt, removed]) 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(logged()).toHaveLength(1); + expect(removals()).toEqual([]); }); - it('removes its container and volume by name when the daemon refuses the run', async () => { + 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: no such runtime'); - expect(logged().slice(1)).toEqual([`rm --force --volumes ${probeName()}`]); + expect(removals()).toEqual([`rm --force --volumes ${probeName()}`, `container inspect ${probeName()}`]); }); - it('removes a container a killed client may have created but never started', async () => { - writeFileSync(mode, 'hang'); writeFileSync(removals, 'removed'); + it('removes a container a killed client left created but never started', async () => { + writeFileSync(mode, 'created'); await expect(probeTaskStoragePathIdentity(imageId, 500)).rejects.toThrow('docker run failed (client killed or not started)'); - expect(logged().slice(1)).toEqual([`rm --force --volumes ${probeName()}`]); - }, 30_000); - it('keeps asking through the settle window while a killed create may still land', async () => { - writeFileSync(mode, 'hang'); + 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 removals find nothing (rm still exits 0); only asking until the window closes catches the late create. + writeFileSync(mode, 'late'); const started = performance.now(); await expect(probeTaskStoragePathIdentity(imageId, 500)).rejects.toThrow('client killed or not started'); - // Absent at every ask, so it asked until the window closed rather than trusting the first answer. expect(performance.now() - started).toBeGreaterThanOrEqual(10_000); - expect(logged().slice(1).length).toBeGreaterThan(1); - expect(new Set(logged().slice(1))).toEqual(new Set([`rm --force --volumes ${probeName()}`])); + 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); + }); }); From ce51c58c91f44af9c9d799d33f528f272485d59e Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 13:05:45 -0700 Subject: [PATCH 06/40] Test the probe's retries, its give-up and an any-host negative control Independent review of #180, round 4. The script is now run on a folder that already holds CASE and the NFD name, which must read as folded on any host, so a script that printed a constant fails on Linux CI too. The cleanup's retry while Docker refuses a removal, its give-up with the name to remove, and a client refused before it started each have a test; a give-up after Docker still reports the container says so. The comment no longer claims every failed probe is removed: a create that lands after the window, or a runner that dies first, can leave one. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 19 +++++++++++-------- test/path-identity-probe.test.ts | 31 +++++++++++++++++++++++++++++-- test/path-identity.test.ts | 9 ++++++++- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 4ac88c99..7511d7c0 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -413,12 +413,14 @@ export const PATH_IDENTITY_SCRIPT = ['set -eu', 'cd "$1"', ': > case', ': > "$(p /** * 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 seeder. The volume is anonymous and the container `--rm`, so once the container has + * 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: every failed - * probe therefore removes its container, and that volume, by the unique name it gave it. + * 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). */ -export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000): Promise { +export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000, cleanupMs = 30_000): 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 volume = ['type=volume', 'target=/work', 'volume-driver=local', @@ -437,7 +439,7 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = if (outcome.status === null && (error?.code === NOT_STARTED || error?.syscall?.startsWith('spawn'))) throw failure; // A client that did not finish leaves the daemon's outcome unknown: a killed create may still land, so only absence // after the settle window counts. A daemon answer settles at once. - try { await removeProbeContainer(name, outcome.status === null ? CREATE_SETTLE_MS : 0); } + try { await removeProbeContainer(name, outcome.status === null ? CREATE_SETTLE_MS : 0, 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]) }); @@ -453,8 +455,8 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = * 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): Promise { - const settleBy = performance.now() + settleMs, giveUpAt = settleBy + 30_000; +async function removeProbeContainer(name: string, settleMs: number, cleanupMs: number): Promise { + const settleBy = performance.now() + settleMs, giveUpAt = settleBy + cleanupMs; const docker = (args: readonly string[]) => runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs: 10_000 }); for (;;) { let args: readonly string[] = ['rm', '--force', '--volumes', name]; @@ -465,7 +467,8 @@ async function removeProbeContainer(name: string, settleMs: number): Promise= settleBy) return; } // Still there (a removal already in progress), not yet landed, or Docker did not answer: ask again. - if (performance.now() >= giveUpAt) throw new DockerError(args, result); + if (performance.now() >= giveUpAt) + throw result.status === 0 ? new Error(`Docker still reports ${name} after removing it.`) : new DockerError(args, result); await pause(250); } } diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index dd873668..3e789390 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,6 +9,7 @@ import { probeTaskStoragePathIdentity } from '../agents/container/storage.ts'; // 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 busy = join(root, 'busy'), stuck = join(root, 'stuck'); const savedPath = process.env.PATH; let imageId = ''; beforeAll(() => { @@ -18,9 +19,11 @@ beforeAll(() => { // `run`: per the mode file, answer, refuse as the daemon does, or hang until the client is killed. In `late` mode the // create lands two seconds later, 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 refuses as Docker does during an auto-remove; with the stuck file, a removed + // container is still reported. writeFileSync(join(bin, 'docker'), `#!/bin/sh printf '%s\\n' "$*" >> '${calls}' -exists() { [ ! -e '${removed}' ] && [ -e '${landAt}' ] && [ "$(date +%s)" -ge "$(cat '${landAt}')" ]; } +exists() { { [ ! -e '${removed}' ] || [ -e '${stuck}' ]; } && [ -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 @@ -32,6 +35,10 @@ if [ "$1" = run ]; then esac fi if [ "$1" = rm ]; then + if [ -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 : > '${removed}'; else echo "Error response from daemon: No such container: $4" >&2; fi exit 0 fi @@ -46,7 +53,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, busy, stuck]) 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]!; @@ -89,4 +96,24 @@ describe('task storage probe against Docker answers', () => { expect(performance.now() - started).toBeLessThan(2_000); expect(existsSync(calls)).toBe(false); }); + it('asks again while Docker refuses the removal for now, and keeps the probe failure as the error', async () => { + writeFileSync(mode, 'refused'); writeFileSync(busy, '2'); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125)'); + expect(removals()).toEqual([...Array(3).fill(`rm --force --volumes ${probeName()}`), `container inspect ${probeName()}`]); + }); + it('gives up when Docker keeps reporting the container, and names it for the person to remove', async () => { + writeFileSync(mode, 'created'); writeFileSync(stuck, ''); + const failure = await probeTaskStoragePathIdentity(imageId, 500, 1_000).catch((error: Error) => error); + const name = probeName(); + expect((failure as Error).message).toBe('docker run failed (client killed or not started): docker run ETIMEDOUT after 500 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 still reports ${name} after removing it.`]); + }, 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); + }); }); diff --git a/test/path-identity.test.ts b/test/path-identity.test.ts index 06bac699..9b02f457 100644 --- a/test/path-identity.test.ts +++ b/test/path-identity.test.ts @@ -1,5 +1,5 @@ import { execFileSync } from 'node:child_process'; -import { mkdtempSync, readdirSync, rmSync } from 'node:fs'; +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'; @@ -49,4 +49,11 @@ describe('task storage probe script', () => { const expected = probePathIdentity(root); expect(out).toBe(`codeboost-path-identity ${expected.caseSensitive} ${expected.unicodeNormalization}\n`); }); + it('reports what it finds, on any host: names that already answer to CASE and NFD read as folded', () => { + // On Linux CI the honest answer above is the same constant task storage gives; this one is not, so a script that + // printed a constant instead of looking fails here everywhere. + 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'); + }); }); From 82c6c9b4887eb5c42eb110fba37409a4609aaf7b Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 13:25:31 -0700 Subject: [PATCH 07/40] Pin the path identity probe's volume options and hardening in a test Independent review of #180, round 5: any Linux volume answers `true none`, so only the probe's arguments show it measures a volume made like task storage, in a container hardened like the keeper. A fake-docker test now checks the exact run arguments. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 3e789390..758a7298 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -66,6 +66,18 @@ describe('task storage probe against Docker answers', () => { 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}`); + expect(script.endsWith(' probe /work')).toBe(true); + }); 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: no such runtime'); From 5ebd8e6291ecb6bae435d46def7d20d219a51af3 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 13:44:56 -0700 Subject: [PATCH 08/40] Confirm a failed probe's removal by inspect whatever rm answers Independent review of #180, round 6: older Docker CLIs exit non-zero for `rm --force` on a missing name, and the cleanup then never asked inspect, so it waited out its whole window and said a container may remain when none did. Every removal is now followed by an inspect, as allocation's own removal does. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 11 ++++------- test/path-identity-probe.test.ts | 11 +++++++++-- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 7511d7c0..39c8732e 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -459,13 +459,10 @@ async function removeProbeContainer(name: string, settleMs: number, cleanupMs: n const settleBy = performance.now() + settleMs, giveUpAt = settleBy + cleanupMs; const docker = (args: readonly string[]) => runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs: 10_000 }); for (;;) { - let args: readonly string[] = ['rm', '--force', '--volumes', name]; - let result = await docker(args); - if (result.status === 0) { - args = ['container', 'inspect', name]; - result = await docker(args); - if (absent(result) && performance.now() >= settleBy) return; - } + // Whatever rm answered (older CLIs exit non-zero for a missing name, newer ones 0), the inspect decides. + await docker(['rm', '--force', '--volumes', name]); + const args = ['container', 'inspect', name], result = await docker(args); + if (absent(result) && performance.now() >= settleBy) return; // Still there (a removal already in progress), not yet landed, or Docker did not answer: ask again. if (performance.now() >= giveUpAt) throw result.status === 0 ? new Error(`Docker still reports ${name} after removing it.`) : new DockerError(args, result); diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 758a7298..b883452e 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -108,10 +108,11 @@ describe('task storage probe against Docker answers', () => { expect(performance.now() - started).toBeLessThan(2_000); expect(existsSync(calls)).toBe(false); }); - it('asks again while Docker refuses the removal for now, and keeps the probe failure as the error', async () => { + it('confirms by inspect even when rm exits non-zero, and keeps the probe failure as the error', async () => { writeFileSync(mode, 'refused'); writeFileSync(busy, '2'); await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125)'); - expect(removals()).toEqual([...Array(3).fill(`rm --force --volumes ${probeName()}`), `container inspect ${probeName()}`]); + // Each refused rm is followed by an inspect; the container never existed, so the first inspect settles it. + expect(removals()).toEqual([`rm --force --volumes ${probeName()}`, `container inspect ${probeName()}`]); }); it('gives up when Docker keeps reporting the container, and names it for the person to remove', async () => { writeFileSync(mode, 'created'); writeFileSync(stuck, ''); @@ -128,4 +129,10 @@ describe('task storage probe against Docker answers', () => { 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, 500)).rejects.toThrow('client killed or not started'); + expect(removals().filter(call => call.startsWith('rm ')).length).toBeGreaterThanOrEqual(3); + expect(left()).toBe(false); + }, 60_000); }); From 4250a656ebbe7114d2554663b703ed9c2382a964 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 13:52:25 -0700 Subject: [PATCH 09/40] Point the probe's leftover-container note at #181 Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 39c8732e..41d850d8 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -418,7 +418,7 @@ export const PATH_IDENTITY_SCRIPT = ['set -eu', 'cd "$1"', ': > case', ': > "$(p * 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). + * find that container (`docker ps -a --filter name=codeboost-path-identity-` does). A recovery sweep is #181. */ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000, cleanupMs = 30_000): Promise { if (!/^sha256:[0-9a-f]{64}$/.test(imageId)) throw new Error('The path identity probe requires the immutable built image ID.'); From ea56d449c0e1d8c950e59d1467fcc7ee816960bd Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 14:28:46 -0700 Subject: [PATCH 10/40] Require test fakes to answer as the real tool does Review lesson from #180: a fake `docker` answered `rm --force` on a missing name with exit 1, while Docker 27 exits 0, so a cleanup that trusted that exit code passed its tests while it would leak against the real daemon. Added to the mirrored "Review readiness" section of AGENTS.md and CLAUDE.md in the same commit. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 1 + CLAUDE.md | 1 + 2 files changed, 2 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 13891a34..b3ade114 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 answer every call the code reads with the exit status, stdout and stderr the real tool gives, error cases included. Check those answers against the real tool and name the version checked beside the fake. Where supported versions 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..022b269b 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 answer every call the code reads with the exit status, stdout and stderr the real tool gives, error cases included. Check those answers against the real tool and name the version checked beside the fake. Where supported versions 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. From 667d91016d9cf97f514bf0a24a97a41de39402bb Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 14:28:46 -0700 Subject: [PATCH 11/40] Hold this branch's fakes to the new rule The probe fake names the Docker version its answers were checked against and gives the older CLI's rm answer for a missing name; the CLI test's fake says it models only a successful startup's calls. Co-Authored-By: Claude Opus 5.5 --- test/cli.test.ts | 4 +++- test/path-identity-probe.test.ts | 18 ++++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/test/cli.test.ts b/test/cli.test.ts index 7d4ee8f7..89b7c427 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -29,7 +29,9 @@ function review(github: Record | null = { baseBranch: 'main' }, /** * 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. + * a slow daemon, for as long as the test keeps the file. It models only the calls a successful startup makes; the + * probe's failure cleanup, whose rm and inspect answers matter, is tested against a Docker-27 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', diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index b883452e..ff6a0e13 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,7 +9,7 @@ import { probeTaskStoragePathIdentity } from '../agents/container/storage.ts'; // 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 busy = join(root, 'busy'), stuck = join(root, 'stuck'); +const busy = join(root, 'busy'), stuck = join(root, 'stuck'), oldCli = join(root, 'old-cli'); const savedPath = process.env.PATH; let imageId = ''; beforeAll(() => { @@ -20,7 +20,8 @@ beforeAll(() => { // create lands two seconds later, 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 refuses as Docker does during an auto-remove; with the stuck file, a removed - // container is still reported. + // container is still reported. Checked against Docker 27.4.0. With the old-cli file, rm of a missing name exits 1, as + // CLIs before 20.10 did by the docker/cli changelog (not run here). writeFileSync(join(bin, 'docker'), `#!/bin/sh printf '%s\\n' "$*" >> '${calls}' exists() { { [ ! -e '${removed}' ] || [ -e '${stuck}' ]; } && [ -e '${landAt}' ] && [ "$(date +%s)" -ge "$(cat '${landAt}')" ]; } @@ -39,7 +40,9 @@ if [ "$1" = rm ]; 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 : > '${removed}'; else echo "Error response from daemon: No such container: $4" >&2; fi + if exists; then : > '${removed}'; exit 0; fi + 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 @@ -53,7 +56,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, busy, stuck]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, busy, stuck, oldCli]) 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]!; @@ -135,4 +138,11 @@ describe('task storage probe against Docker answers', () => { expect(removals().filter(call => call.startsWith('rm ')).length).toBeGreaterThanOrEqual(3); expect(left()).toBe(false); }, 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()}`]); + }); }); From e6cefdf30fb09402d551a09ceee3ef6e695b535c Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 14:42:32 -0700 Subject: [PATCH 12/40] Word the fakes rule by the versions the code may meet No repository document defines a supported Docker, git or gh range, so "supported versions" left a fake's completeness undecidable. Changed in both mirrored files. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- CLAUDE.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b3ade114..2ab8bf05 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,7 +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 answer every call the code reads with the exit status, stdout and stderr the real tool gives, error cases included. Check those answers against the real tool and name the version checked beside the fake. Where supported versions 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. +- A fake of an external tool (a stand-in `docker`, `git` or `gh` on `PATH`, or a stubbed client) must answer every call the code reads with the exit status, stdout and stderr the real tool gives, error cases included. 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 022b269b..65c28a33 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,7 +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 answer every call the code reads with the exit status, stdout and stderr the real tool gives, error cases included. Check those answers against the real tool and name the version checked beside the fake. Where supported versions 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. +- A fake of an external tool (a stand-in `docker`, `git` or `gh` on `PATH`, or a stubbed client) must answer every call the code reads with the exit status, stdout and stderr the real tool gives, error cases included. 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. From 3590e0c40e202cb47c08d5b7712c6e22881e5d72 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 14:42:32 -0700 Subject: [PATCH 13/40] Make the fakes answer exactly as Docker 27.4.0 does The probe fake's inspect of a missing name now prints [] on stdout and its refused run uses Docker's own wording; the CLI test's fake names the version its success-path answers were checked against. Co-Authored-By: Claude Opus 5.5 --- test/cli.test.ts | 8 +++++--- test/path-identity-probe.test.ts | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/test/cli.test.ts b/test/cli.test.ts index 89b7c427..df93aaae 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -29,9 +29,11 @@ function review(github: Record | null = { baseBranch: 'main' }, /** * 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; the - * probe's failure cleanup, whose rm and inspect answers matter, is tested against a Docker-27 fake in - * path-identity-probe.test.ts. + * 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`, `volume ls --quiet`) with nothing to list + * answer as here, exit 0 and no output, 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', diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index ff6a0e13..40ebf592 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -29,7 +29,7 @@ if [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; exit 0; fi if [ "$1" = run ]; then case "$(cat '${mode}')" in answer) echo 'codeboost-path-identity true none';; - refused) echo 'docker: Error response from daemon: no such runtime' >&2; exit 125;; + refused) echo 'docker: Error response from daemon: unknown or invalid runtime name: x.' >&2; exit 125;; created) echo 0 > '${landAt}'; exec sleep 30;; late) echo $(( $(date +%s) + 2 )) > '${landAt}'; exec sleep 30;; hang) exec sleep 30;; @@ -47,7 +47,7 @@ if [ "$1" = rm ]; then fi if [ "$1 $2" = "container inspect" ]; then if exists; then echo '[{}]'; exit 0; fi - echo "Error response from daemon: No such container: $3" >&2; exit 1 + echo '[]'; echo "Error response from daemon: No such container: $3" >&2; exit 1 fi exit 0 `); @@ -83,7 +83,7 @@ describe('task storage probe against Docker answers', () => { }); 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: no such runtime'); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125): docker: Error response from daemon: unknown or invalid runtime name: x.'); expect(removals()).toEqual([`rm --force --volumes ${probeName()}`, `container inspect ${probeName()}`]); }); it('removes a container a killed client left created but never started', async () => { From b6d763f1b47371effa11c909c70fa98da659be68 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 14:52:22 -0700 Subject: [PATCH 14/40] Give the fakes Docker's full answers and list every modelled call Independent review of #180, round 13: a refused run's stderr has a second line on Docker 27.4.0, now in the fake and asserted in full; the CLI test's fake lists the network listing and build among its calls. Co-Authored-By: Claude Opus 5.5 --- test/cli.test.ts | 6 +++--- test/path-identity-probe.test.ts | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/test/cli.test.ts b/test/cli.test.ts index df93aaae..6e206514 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -30,9 +30,9 @@ function review(github: Record | null = { baseBranch: 'main' }, * 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`, `volume ls --quiet`) with nothing to list - * answer as here, exit 0 and no output, 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 + * 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; 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 = () => { diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 40ebf592..55497a5c 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -29,7 +29,7 @@ if [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; exit 0; fi if [ "$1" = run ]; then case "$(cat '${mode}')" in answer) echo 'codeboost-path-identity true none';; - refused) echo 'docker: Error response from daemon: unknown or invalid runtime name: x.' >&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) echo $(( $(date +%s) + 2 )) > '${landAt}'; exec sleep 30;; hang) exec sleep 30;; @@ -83,7 +83,8 @@ describe('task storage probe against Docker answers', () => { }); 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.'); + 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 () => { From d97a98a39860dd5c47b9c957580b64a6ba6a6f60 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 15:00:59 -0700 Subject: [PATCH 15/40] Name every call the fakes answer, image inspect and build included Independent review of #180, round 14. Co-Authored-By: Claude Opus 5.5 --- test/cli.test.ts | 4 +++- test/path-identity-probe.test.ts | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/test/cli.test.ts b/test/cli.test.ts index 6e206514..c483f940 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -32,7 +32,9 @@ function review(github: Record | null = { baseBranch: 'main' }, * 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; 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 + * 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 = () => { diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 55497a5c..45727663 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -20,7 +20,8 @@ beforeAll(() => { // create lands two seconds later, 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 refuses as Docker does during an auto-remove; with the stuck file, a removed - // container is still reported. Checked against Docker 27.4.0. With the old-cli file, rm of a missing name exits 1, as + // container is still reported. 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/cli changelog (not run here). writeFileSync(join(bin, 'docker'), `#!/bin/sh printf '%s\\n' "$*" >> '${calls}' From d2d47e0f78330029ffd8538bc8690a6af8342fad Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 15:41:26 -0700 Subject: [PATCH 16/40] Bound the probe cleanup per call, test volume removal, document the step Independent review of #180, round 15. Each cleanup call now gets the time left before giving up (at least 1 s), so a daemon that stops answering cannot stretch the bound, and the give-up reports the last answer Docker gave. A real-Docker test shows a never-started container made with the probe's exact mount loses its volume to the cleanup's own rm. The runner lifecycle doc and README name the startup check. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- agents/container/storage.ts | 29 ++++++++++++++++--------- docs/implementation/runner-lifecycle.md | 2 +- test/path-identity-probe.test.ts | 12 ++++++++-- test/runner-workspace.test.ts | 13 ++++++++++- 5 files changed, 43 insertions(+), 15 deletions(-) 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 41d850d8..19042d03 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -410,6 +410,11 @@ export async function prepareTaskFilesystemsAsync(clone: TaskClone, limits: Task 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 @@ -423,14 +428,10 @@ export const PATH_IDENTITY_SCRIPT = ['set -eu', 'cd "$1"', ': > case', ': > "$(p export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000, cleanupMs = 30_000): 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 volume = ['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(','); 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', volume, '--entrypoint', 'sh', imageId, '-c', PATH_IDENTITY_SCRIPT, 'probe', '/work']; + '--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); @@ -457,15 +458,23 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = */ async function removeProbeContainer(name: string, settleMs: number, cleanupMs: number): Promise { const settleBy = performance.now() + settleMs, giveUpAt = settleBy + cleanupMs; - const docker = (args: readonly string[]) => runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs: 10_000 }); + // Each call gets the time left before giving up (at least 1 s, so the last ask can still be answered): a daemon that + // stops answering stretches the bound by at most that floor and the stop grace of one call. + const docker = (args: readonly string[]) => runInProcessGroup('docker', args, { env: dockerEnvironment(), + timeoutMs: Math.min(10_000, Math.max(1_000, Math.ceil(giveUpAt - performance.now()))) }); + const args = ['container', 'inspect', name]; + let answered: DockerOutcome | undefined, last: DockerOutcome; for (;;) { // Whatever rm answered (older CLIs exit non-zero for a missing name, newer ones 0), the inspect decides. await docker(['rm', '--force', '--volumes', name]); - const args = ['container', 'inspect', name], result = await docker(args); - if (absent(result) && performance.now() >= settleBy) return; + last = await docker(args); + if (last.status !== null) answered = last; + if (absent(last) && performance.now() >= settleBy) return; // Still there (a removal already in progress), not yet landed, or Docker did not answer: ask again. - if (performance.now() >= giveUpAt) - throw result.status === 0 ? new Error(`Docker still reports ${name} after removing it.`) : new DockerError(args, result); + if (performance.now() >= giveUpAt) { + const report = answered ?? last; + throw report.status === 0 ? new Error(`Docker still reports ${name} after removing it.`) : new DockerError(args, report); + } await pause(250); } } 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/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 45727663..c8cd1d00 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,7 +9,7 @@ import { probeTaskStoragePathIdentity } from '../agents/container/storage.ts'; // 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 busy = join(root, 'busy'), stuck = join(root, 'stuck'), oldCli = join(root, 'old-cli'); +const busy = join(root, 'busy'), stuck = join(root, 'stuck'), oldCli = join(root, 'old-cli'), unanswered = join(root, 'unanswered'); const savedPath = process.env.PATH; let imageId = ''; beforeAll(() => { @@ -36,6 +36,7 @@ if [ "$1" = run ]; then hang) exec sleep 30;; esac fi +if [ -e '${unanswered}' ] && { [ "$1" = rm ] || [ "$1 $2" = "container inspect" ]; }; then exec sleep 30; fi if [ "$1" = rm ]; then if [ -s '${busy}' ] && [ "$(cat '${busy}')" -gt 0 ]; then echo $(( $(cat '${busy}') - 1 )) > '${busy}' @@ -57,7 +58,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, busy, stuck, oldCli]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, busy, stuck, oldCli, unanswered]) 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]!; @@ -147,4 +148,11 @@ describe('task storage probe against Docker answers', () => { 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(unanswered, ''); + const started = performance.now(); + await expect(probeTaskStoragePathIdentity(imageId, 60_000, 1_000)).rejects.toThrow('may remain'); + expect(performance.now() - started).toBeLessThan(4_000); + }, 60_000); }); diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts index b5e7f0f3..191c24ed 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 { probeTaskStoragePathIdentity, 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'; @@ -157,6 +158,16 @@ describe('task storage path identity (#162)', () => { expect(docker('ps', '-aq', '--filter', 'name=codeboost-path-identity-')).toBe(''); expect([...volumes()].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 volumes = () => new Set(docker('volume', 'ls', '-q').split('\n').filter(Boolean)); + const before = volumes(), name = `codeboost-path-identity-${randomUUID()}`; + docker('create', '--name', name, '--mount', PROBE_VOLUME_MOUNT, '--entrypoint', 'true', imageId); + try { expect([...volumes()].filter(volume => !before.has(volume))).toHaveLength(1); } + finally { docker('rm', '--force', '--volumes', name); } + expect([...volumes()].filter(volume => !before.has(volume))).toEqual([]); + }, 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'); From 54827ac1127705785f42fc9b67e65002445442e6 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 15:56:28 -0700 Subject: [PATCH 17/40] Start no cleanup call after giving up is due, and report its real cause Independent review of #180, round 16. After the first ask, the probe cleanup checks the give-up time before each call, so a daemon that stops answering partway adds at most one call's floor and stop grace. On give-up it reports the latest outcome of either call (an unanswered rm included), or that Docker still reported the container. The fake no longer has states Docker never answers: a removal in progress applies only to a container that exists, and a hanging daemon starts at a set time. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 35 +++++++++++++++++--------- test/path-identity-probe.test.ts | 43 +++++++++++++++++++++----------- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 19042d03..d1be6704 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -458,23 +458,34 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = */ async function removeProbeContainer(name: string, settleMs: number, cleanupMs: number): Promise { const settleBy = performance.now() + settleMs, giveUpAt = settleBy + cleanupMs; - // Each call gets the time left before giving up (at least 1 s, so the last ask can still be answered): a daemon that - // stops answering stretches the bound by at most that floor and the stop grace of one call. + // Each call gets the time left before giving up (at least 1 s, so the last ask can still be answered), and after the + // first ask no call starts once that time has passed: a daemon that stops answering stretches the bound by at most + // one call's floor and its stop grace. const docker = (args: readonly string[]) => runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs: Math.min(10_000, Math.max(1_000, Math.ceil(giveUpAt - performance.now()))) }); - const args = ['container', 'inspect', name]; - let answered: DockerOutcome | undefined, last: DockerOutcome; + const remove = ['rm', '--force', '--volumes', name], inspect = ['container', 'inspect', name]; + // The latest outcome of either call, and the latest inspect that found the container. + let last: { args: readonly string[]; outcome: DockerOutcome } | undefined, present: DockerOutcome | undefined; + const giveUp = (): never => { + // Docker stopping answering after it reported the container present does not hide that it was present. + if (present && last!.outcome.status === null) throw new Error(`Docker still reports ${name} after removing it.`); + const { args, outcome } = last!; + throw outcome.status === 0 ? new Error(`Docker still reports ${name} after removing it.`) : new DockerError(args, outcome); + }; for (;;) { + if (last && performance.now() >= giveUpAt) giveUp(); // Whatever rm answered (older CLIs exit non-zero for a missing name, newer ones 0), the inspect decides. - await docker(['rm', '--force', '--volumes', name]); - last = await docker(args); - if (last.status !== null) answered = last; - if (absent(last) && performance.now() >= settleBy) return; + const removal = await docker(remove); + // Only an unanswered rm is worth reporting: what it answers is never read. + if (removal.status === null) last = { args: remove, outcome: removal }; + if (last && performance.now() >= giveUpAt) giveUp(); + const found = await docker(inspect); + last = { args: inspect, outcome: found }; + if (found.status === 0) present = found; + else if (absent(found)) present = undefined; + if (absent(found) && performance.now() >= settleBy) return; // Still there (a removal already in progress), not yet landed, or Docker did not answer: ask again. - if (performance.now() >= giveUpAt) { - const report = answered ?? last; - throw report.status === 0 ? new Error(`Docker still reports ${name} after removing it.`) : new DockerError(args, report); - } + if (performance.now() >= giveUpAt) giveUp(); await pause(250); } } diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index c8cd1d00..cdf701db 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,7 +9,7 @@ import { probeTaskStoragePathIdentity } from '../agents/container/storage.ts'; // 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 busy = join(root, 'busy'), stuck = join(root, 'stuck'), oldCli = join(root, 'old-cli'), unanswered = join(root, 'unanswered'); +const busy = join(root, 'busy'), oldCli = join(root, 'old-cli'), hangAt = join(root, 'hang-at'), hung = join(root, 'hung'); const savedPath = process.env.PATH; let imageId = ''; beforeAll(() => { @@ -19,13 +19,14 @@ beforeAll(() => { // `run`: per the mode file, answer, refuse as the daemon does, or hang until the client is killed. In `late` mode the // create lands two seconds later, 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 refuses as Docker does during an auto-remove; with the stuck file, a removed - // container is still reported. Checked against Docker 27.4.0, as are `build` (only its exit status is read) and `image + // 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. 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/cli changelog (not run here). writeFileSync(join(bin, 'docker'), `#!/bin/sh printf '%s\\n' "$*" >> '${calls}' -exists() { { [ ! -e '${removed}' ] || [ -e '${stuck}' ]; } && [ -e '${landAt}' ] && [ "$(date +%s)" -ge "$(cat '${landAt}')" ]; } +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 @@ -36,9 +37,11 @@ if [ "$1" = run ]; then hang) exec sleep 30;; esac fi -if [ -e '${unanswered}' ] && { [ "$1" = rm ] || [ "$1 $2" = "container inspect" ]; }; then exec sleep 30; fi +if [ -e '${hangAt}' ] && [ "$(date +%s)" -ge "$(cat '${hangAt}')" ] && { [ "$1" = rm ] || [ "$1 $2" = "container inspect" ]; }; then + echo "$1" >> '${hung}'; exec sleep 30 +fi if [ "$1" = rm ]; then - if [ -s '${busy}' ] && [ "$(cat '${busy}')" -gt 0 ]; 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 @@ -58,7 +61,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, busy, stuck, oldCli, unanswered]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, busy, oldCli, hangAt, hung]) 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]!; @@ -114,14 +117,8 @@ describe('task storage probe against Docker answers', () => { expect(performance.now() - started).toBeLessThan(2_000); expect(existsSync(calls)).toBe(false); }); - it('confirms by inspect even when rm exits non-zero, and keeps the probe failure as the error', async () => { - writeFileSync(mode, 'refused'); writeFileSync(busy, '2'); - await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125)'); - // Each refused rm is followed by an inspect; the container never existed, so the first inspect settles it. - expect(removals()).toEqual([`rm --force --volumes ${probeName()}`, `container inspect ${probeName()}`]); - }); it('gives up when Docker keeps reporting the container, and names it for the person to remove', async () => { - writeFileSync(mode, 'created'); writeFileSync(stuck, ''); + writeFileSync(mode, 'created'); writeFileSync(busy, '100000'); const failure = await probeTaskStoragePathIdentity(imageId, 500, 1_000).catch((error: Error) => error); const name = probeName(); expect((failure as Error).message).toBe('docker run failed (client killed or not started): docker run ETIMEDOUT after 500 ms.' @@ -150,9 +147,25 @@ describe('task storage probe against Docker answers', () => { }); 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(unanswered, ''); + writeFileSync(mode, 'refused'); writeFileSync(hangAt, '0'); const started = performance.now(); await expect(probeTaskStoragePathIdentity(imageId, 60_000, 1_000)).rejects.toThrow('may remain'); expect(performance.now() - started).toBeLessThan(4_000); }, 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 two 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, 500, 4_000).catch((error: Error) => error); + expect(readFileSync(hung, 'utf8').trim().split('\n')).toHaveLength(1); + // Docker had reported the container present; the hung call that followed does not hide that. + expect(((failure as Error).cause as AggregateError).errors[1].message).toBe(`Docker still reports ${probeName()} after removing it.`); + }, 60_000); + it('reports a daemon that stopped answering, not an earlier absent answer', async () => { + // Absent at every answered ask (a killed client whose create never landed), then no answer before the window ends. + writeFileSync(mode, 'hang'); writeFileSync(hangAt, String(Math.floor(Date.now() / 1000) + 5)); + const failure = await probeTaskStoragePathIdentity(imageId, 500, 2_000).catch((error: Error) => error); + expect(((failure as Error).cause as AggregateError).errors[1].message).toMatch(/^docker (rm|container) failed \(client killed or not started\): docker (rm|container) ETIMEDOUT/); + }, 60_000); }); From 76272c49e31b7207db7a0d5dbb886f0c59654d82 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 16:10:06 -0700 Subject: [PATCH 18/40] Simplify the probe cleanup's give-up, and settle unless the daemon refused Independent review of #180, round 17. One helper now makes every cleanup call and checks the give-up time before each, and the give-up says only that Docker did not confirm the container is gone, with Docker's last outcome as the cause. A client-side exit 125 (a lost connection after the create was sent) no longer skips the settle window: only a refusal from the daemon itself does. The fake notes which rm and inspect output the code reads. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 41 ++++++++++++-------------------- test/path-identity-probe.test.ts | 29 ++++++++++++++++------ 2 files changed, 37 insertions(+), 33 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index d1be6704..c82f493e 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -438,9 +438,11 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = // 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; - // A client that did not finish leaves the daemon's outcome unknown: a killed create may still land, so only absence - // after the settle window counts. A daemon answer settles at once. - try { await removeProbeContainer(name, outcome.status === null ? CREATE_SETTLE_MS : 0, cleanupMs); } + // 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 : CREATE_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]) }); @@ -458,34 +460,21 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = */ 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 (at least 1 s, so the last ask can still be answered), and after the - // first ask no call starts once that time has passed: a daemon that stops answering stretches the bound by at most - // one call's floor and its stop grace. - const docker = (args: readonly string[]) => runInProcessGroup('docker', args, { env: dockerEnvironment(), - timeoutMs: Math.min(10_000, Math.max(1_000, Math.ceil(giveUpAt - performance.now()))) }); - const remove = ['rm', '--force', '--volumes', name], inspect = ['container', 'inspect', name]; - // The latest outcome of either call, and the latest inspect that found the container. - let last: { args: readonly string[]; outcome: DockerOutcome } | undefined, present: DockerOutcome | undefined; - const giveUp = (): never => { - // Docker stopping answering after it reported the container present does not hide that it was present. - if (present && last!.outcome.status === null) throw new Error(`Docker still reports ${name} after removing it.`); - const { args, outcome } = last!; - throw outcome.status === 0 ? new Error(`Docker still reports ${name} after removing it.`) : new DockerError(args, outcome); + // first call none starts once that time has passed: a daemon that stops answering stretches the bound by at most one + // call's floor and its stop grace. + 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: Math.min(10_000, Math.max(1_000, Math.ceil(giveUpAt - performance.now()))) }); }; for (;;) { - if (last && performance.now() >= giveUpAt) giveUp(); // Whatever rm answered (older CLIs exit non-zero for a missing name, newer ones 0), the inspect decides. - const removal = await docker(remove); - // Only an unanswered rm is worth reporting: what it answers is never read. - if (removal.status === null) last = { args: remove, outcome: removal }; - if (last && performance.now() >= giveUpAt) giveUp(); - const found = await docker(inspect); - last = { args: inspect, outcome: found }; - if (found.status === 0) present = found; - else if (absent(found)) present = undefined; - if (absent(found) && performance.now() >= settleBy) return; + await ask(['rm', '--force', '--volumes', name]); + if (absent(await ask(['container', 'inspect', name])) && performance.now() >= settleBy) return; // Still there (a removal already in progress), not yet landed, or Docker did not answer: ask again. - if (performance.now() >= giveUpAt) giveUp(); await pause(250); } } diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index cdf701db..c9dca804 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -21,7 +21,10 @@ beforeAll(() => { // 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. Checked against Docker 27.4.0, as are `build` (only its exit status is read) and `image + // that stopped); each such call is logged in the hung file. 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. `lost` is the client's own exit 125 when the + // connection drops after the create was sent (wording as Docker 27's CLI, not reproduced here). 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/cli changelog (not run here). writeFileSync(join(bin, 'docker'), `#!/bin/sh @@ -31,6 +34,7 @@ if [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; exit 0; fi if [ "$1" = run ]; then case "$(cat '${mode}')" in answer) echo 'codeboost-path-identity true none';; + lost) echo 'error during connect: Post "http://docker.example/v1.47/containers/create": EOF' >&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) echo $(( $(date +%s) + 2 )) > '${landAt}'; exec sleep 30;; @@ -124,7 +128,9 @@ describe('task storage probe against Docker answers', () => { expect((failure as Error).message).toBe('docker run failed (client killed or not started): docker run ETIMEDOUT after 500 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 still reports ${name} after removing it.`]); + 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. + expect((cause.errors[1].cause as { status: number | null }).status).toBe(0); }, 60_000); it('removes nothing when the client was refused before it started', async () => { writeFileSync(mode, 'answer'); @@ -159,13 +165,22 @@ describe('task storage probe against Docker answers', () => { writeFileSync(hangAt, String(Math.floor(Date.now() / 1000) + 12)); const failure = await probeTaskStoragePathIdentity(imageId, 500, 4_000).catch((error: Error) => error); expect(readFileSync(hung, 'utf8').trim().split('\n')).toHaveLength(1); - // Docker had reported the container present; the hung call that followed does not hide that. - expect(((failure as Error).cause as AggregateError).errors[1].message).toBe(`Docker still reports ${probeName()} after removing it.`); + expect(((failure as Error).cause as AggregateError).errors[1].message).toBe(`Docker did not confirm that ${probeName()} is gone.`); }, 60_000); - it('reports a daemon that stopped answering, not an earlier absent answer', async () => { - // Absent at every answered ask (a killed client whose create never landed), then no answer before the window ends. + 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); - expect(((failure as Error).cause as AggregateError).errors[1].message).toMatch(/^docker (rm|container) failed \(client killed or not started\): docker (rm|container) ETIMEDOUT/); + 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 when docker run exits without a daemon answer', async () => { + // Exit 125 from the client itself (a connection lost after the create was sent): the create may still land. + writeFileSync(mode, 'lost'); + const started = performance.now(); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125): error during connect'); + expect(performance.now() - started).toBeGreaterThanOrEqual(10_000); }, 60_000); }); From bb6d5dc45104800c984596b0caf199e71e0c687e Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 16:24:38 -0700 Subject: [PATCH 19/40] Accept either last answer in the give-up test; use Docker's real 125 text Independent review of #180, round 18: when giving up falls due during the rm, the last answer is the rm's refusal, not the inspect, so the test accepts both. The unreachable-daemon fake prints what Docker 27.4.0 prints. The cleanup comment names the 10 s cap on each call. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 6 +++--- test/path-identity-probe.test.ts | 18 ++++++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index c82f493e..c3f039d2 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -461,9 +461,9 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 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 (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 and its stop grace. + // 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 and its stop grace. const ask = async (args: readonly string[]) => { if (last && performance.now() >= giveUpAt) throw new Error(`Docker did not confirm that ${name} is gone.`, { cause: last }); diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index c9dca804..398989eb 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -23,8 +23,8 @@ beforeAll(() => { // 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. 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. `lost` is the client's own exit 125 when the - // connection drops after the create was sent (wording as Docker 27's CLI, not reproduced here). Checked against Docker 27.4.0, as are `build` (only its exit status is read) and `image + // than Docker (the name; the full inspect array), which nothing reads. `unreachable` is the client's own exit 125 + // with no daemon answer, as Docker 27.4.0 prints it with DOCKER_HOST=tcp://127.0.0.1:1. 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/cli changelog (not run here). writeFileSync(join(bin, 'docker'), `#!/bin/sh @@ -34,7 +34,7 @@ if [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; exit 0; fi if [ "$1" = run ]; then case "$(cat '${mode}')" in answer) echo 'codeboost-path-identity true none';; - lost) echo 'error during connect: Post "http://docker.example/v1.47/containers/create": EOF' >&2; exit 125;; + unreachable) 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) echo $(( $(date +%s) + 2 )) > '${landAt}'; exec sleep 30;; @@ -129,8 +129,10 @@ describe('task storage probe against Docker answers', () => { + ` 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. - expect((cause.errors[1].cause as { status: number | null }).status).toBe(0); + // 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'); @@ -177,10 +179,10 @@ describe('task storage probe against Docker answers', () => { expect(cleanup.cause).toMatchObject({ status: null, error: { code: 'ETIMEDOUT' } }); }, 60_000); it('waits out the settle window when docker run exits without a daemon answer', async () => { - // Exit 125 from the client itself (a connection lost after the create was sent): the create may still land. - writeFileSync(mode, 'lost'); + // Exit 125 from the client itself: with no daemon answer, a create may have been sent and may still land. + writeFileSync(mode, 'unreachable'); const started = performance.now(); - await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125): error during connect'); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125): docker: Cannot connect to the Docker daemon'); expect(performance.now() - started).toBeGreaterThanOrEqual(10_000); }, 60_000); }); From 70efd6f7310c04c42679d3bc548e13f651420043 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 16:42:06 -0700 Subject: [PATCH 20/40] Test the probe's strict parse; model an unreachable daemon fully Independent review of #180, round 19: a probe that exits 0 with output other than one identity line is refused, now under test. The unreachable fake answers rm and inspect as Docker 27.4.0 does without a daemon, and the old-CLI note cites the 20.10.0 release notes it rests on. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 398989eb..a8ef463b 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,7 +9,7 @@ import { probeTaskStoragePathIdentity } from '../agents/container/storage.ts'; // 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 busy = join(root, 'busy'), oldCli = join(root, 'old-cli'), hangAt = join(root, 'hang-at'), hung = join(root, 'hung'); +const busy = join(root, 'busy'), oldCli = join(root, 'old-cli'), hangAt = join(root, 'hang-at'), hung = join(root, 'hung'), unreach = join(root, 'unreachable'); const savedPath = process.env.PATH; let imageId = ''; beforeAll(() => { @@ -23,10 +23,11 @@ beforeAll(() => { // 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. 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 the client's own exit 125 - // with no daemon answer, as Docker 27.4.0 prints it with DOCKER_HOST=tcp://127.0.0.1:1. Checked against Docker 27.4.0, as are `build` (only its exit status is read) and `image + // 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` exits 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/cli changelog (not run here). + // CLIs before 20.10 did by the Docker 20.10.0 release notes (docker/cli#2678); no such CLI was run here. writeFileSync(join(bin, 'docker'), `#!/bin/sh printf '%s\\n' "$*" >> '${calls}' exists() { [ ! -e '${removed}' ] && [ -e '${landAt}' ] && [ "$(date +%s)" -ge "$(cat '${landAt}')" ]; } @@ -34,7 +35,8 @@ if [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; exit 0; fi if [ "$1" = run ]; then case "$(cat '${mode}')" in answer) echo 'codeboost-path-identity true none';; - unreachable) 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;; + garbled) printf '%s\\n' 'codeboost-path-identity true none' 'extra';; + 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) echo $(( $(date +%s) + 2 )) > '${landAt}'; exec sleep 30;; @@ -44,6 +46,10 @@ 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 [ "$1" = rm ]; then if exists && [ -s '${busy}' ] && [ "$(cat '${busy}')" -gt 0 ]; then echo $(( $(cat '${busy}') - 1 )) > '${busy}' @@ -65,7 +71,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, busy, oldCli, hangAt, hung]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, busy, oldCli, hangAt, hung, unreach]) 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]!; @@ -178,11 +184,17 @@ describe('task storage probe against Docker answers', () => { 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 when docker run exits without a daemon answer', async () => { - // Exit 125 from the client itself: with no daemon answer, a create may have been sent and may still land. + 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(); - await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('docker run failed (exit 125): docker: Cannot connect to the Docker daemon'); + 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('refuses a probe that exits 0 without exactly one identity line', async () => { + writeFileSync(mode, 'garbled'); + await expect(probeTaskStoragePathIdentity(imageId)).rejects.toThrow('The path identity probe did not report an identity.'); + }); }); From acfb73269e4298063aa0114d197ccdeeba33c1ad Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 16:59:12 -0700 Subject: [PATCH 21/40] Pin the script the probe container runs, and cite the probe's evidence Independent review of #180, round 20: the argv test now checks the -c argument is the measuring script itself, so a constant printed in its place fails; the plan-library alignment row cites the probe's code and tests. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/plan-library-alignment.md | 2 +- test/path-identity-probe.test.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/implementation/plan-library-alignment.md b/docs/implementation/plan-library-alignment.md index cd65f8ea..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); task storage is probed at runner startup since #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/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index a8ef463b..d3fe5e96 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -3,7 +3,7 @@ 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 { probeTaskStoragePathIdentity } from '../agents/container/storage.ts'; +import { PATH_IDENTITY_SCRIPT, 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. @@ -94,7 +94,8 @@ describe('task storage probe against Docker answers', () => { + ' --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}`); - expect(script.endsWith(' probe /work')).toBe(true); + // 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'); From fde4e5aaee99412aa46fd5d16b0639083d0b196a Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 17:15:41 -0700 Subject: [PATCH 22/40] Test each field the probe reports, and the 10 s cap per cleanup call Independent review of #180, round 21: every fake answered tmpfs's true none, so a probe that ignored the container's answer passed; the fake can now answer any identity and each field is checked on its own. A hung-daemon test shows the first cleanup call is cut at 10 s. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index d3fe5e96..9255f381 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,14 +9,15 @@ import { PATH_IDENTITY_SCRIPT, probeTaskStoragePathIdentity } from '../agents/co // 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 busy = join(root, 'busy'), oldCli = join(root, 'old-cli'), hangAt = join(root, 'hang-at'), hung = join(root, 'hung'), unreach = join(root, 'unreachable'); +const 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, refuse as the daemon does, or hang until the client is killed. In `late` mode the + // `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 two seconds later, 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 @@ -34,7 +35,7 @@ exists() { [ ! -e '${removed}' ] && [ -e '${landAt}' ] && [ "$(date +%s)" -ge "$ if [ "$1 $2" = "image inspect" ]; then printf '%s' '${image}'; exit 0; fi if [ "$1" = run ]; then case "$(cat '${mode}')" in - answer) echo 'codeboost-path-identity true none';; + 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';; 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;; @@ -71,7 +72,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, busy, oldCli, hangAt, hung, unreach]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, 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]!; @@ -198,4 +199,20 @@ describe('task storage probe against Docker answers', () => { writeFileSync(mode, 'garbled'); 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); }); From 6d5fac3a1094fb8d067f8e0300293e59b160c2e0 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 17:32:47 -0700 Subject: [PATCH 23/40] Test the probe's 1 s floor, the parse's start anchor and its writes Independent review of #180, round 22: a slow daemon with almost no time left is still heard; output with a line before the identity is refused; the script's directory listing is checked on any host; and the tests that need a create to land assert that it did, with a 2 s client deadline to keep that margin under load. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 43 +++++++++++++++++++++++--------- test/path-identity.test.ts | 2 ++ 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 9255f381..eada602f 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,7 +9,7 @@ import { PATH_IDENTITY_SCRIPT, probeTaskStoragePathIdentity } from '../agents/co // 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 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 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(() => { @@ -22,11 +22,11 @@ beforeAll(() => { // 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. Of rm and inspect, the code reads only the exit status + // that stopped); each such call is logged in the hung file. With the slow file, rm and inspect answer after 300 ms. 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` exits 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 + // 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. writeFileSync(join(bin, 'docker'), `#!/bin/sh @@ -37,6 +37,7 @@ 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;; @@ -51,6 +52,7 @@ if [ -e '${unreach}' ] && { [ "$1" = rm ] || [ "$1 $2" = "container inspect" ]; [ "$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}' @@ -72,7 +74,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, busy, oldCli, hangAt, hung, unreach, identity]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, 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]!; @@ -106,7 +108,9 @@ describe('task storage probe against Docker answers', () => { }); it('removes a container a killed client left created but never started', async () => { writeFileSync(mode, 'created'); - await expect(probeTaskStoragePathIdentity(imageId, 500)).rejects.toThrow('docker run failed (client killed or not started)'); + 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); @@ -114,7 +118,9 @@ describe('task storage probe against Docker answers', () => { // The first removals find nothing (rm still exits 0); only asking until the window closes catches the late create. writeFileSync(mode, 'late'); const started = performance.now(); - await expect(probeTaskStoragePathIdentity(imageId, 500)).rejects.toThrow('client killed or not started'); + 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(performance.now() - started).toBeGreaterThanOrEqual(10_000); expect(existsSync(removed)).toBe(true); expect(new Set(removals())).toEqual(new Set([`rm --force --volumes ${probeName()}`, `container inspect ${probeName()}`])); @@ -131,9 +137,11 @@ describe('task storage probe against Docker answers', () => { }); 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 failure = await probeTaskStoragePathIdentity(imageId, 500, 1_000).catch((error: Error) => error); + const failure = await probeTaskStoragePathIdentity(imageId, 2_000, 1_000).catch((error: Error) => error); + // 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 500 ms.' + 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.`]); @@ -150,7 +158,9 @@ describe('task storage probe against Docker answers', () => { }); it('asks again while Docker refuses the removal of a container that exists', async () => { writeFileSync(mode, 'created'); writeFileSync(busy, '2'); - await expect(probeTaskStoragePathIdentity(imageId, 500)).rejects.toThrow('client killed or not started'); + 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); }, 60_000); @@ -173,7 +183,9 @@ describe('task storage probe against Docker answers', () => { // 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, 500, 4_000).catch((error: Error) => error); + 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); @@ -195,8 +207,8 @@ describe('task storage probe against Docker answers', () => { 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('refuses a probe that exits 0 without exactly one identity line', async () => { - writeFileSync(mode, 'garbled'); + 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([ @@ -215,4 +227,11 @@ describe('task storage probe against Docker answers', () => { 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); }); diff --git a/test/path-identity.test.ts b/test/path-identity.test.ts index 9b02f457..f71e49e2 100644 --- a/test/path-identity.test.ts +++ b/test/path-identity.test.ts @@ -48,6 +48,8 @@ describe('task storage probe script', () => { 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', () => { // On Linux CI the honest answer above is the same constant task storage gives; this one is not, so a script that From bde7ba5b407fa374ee8bb22251f182ba25675187 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 17:58:15 -0700 Subject: [PATCH 24/40] Land the late create after the first inspect, and assert that order Independent review of #180, round 23: the late-create fake landed before the first rm, so the test never exercised a create that lands after the cleanup has found nothing. It now lands after the first inspect, and the test checks that the first rm found nothing and a later one removed it. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index eada602f..6809b368 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,7 +9,7 @@ import { PATH_IDENTITY_SCRIPT, probeTaskStoragePathIdentity } from '../agents/co // 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 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 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(() => { @@ -18,7 +18,7 @@ beforeAll(() => { '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 two seconds later, after the client is gone. `rm --force` and `container inspect` answer as Docker 27 + // 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 @@ -41,7 +41,7 @@ if [ "$1" = run ]; then 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) echo $(( $(date +%s) + 2 )) > '${landAt}'; exec sleep 30;; + late) : > '${lateMark}'; exec sleep 30;; hang) exec sleep 30;; esac fi @@ -58,13 +58,15 @@ if [ "$1" = rm ]; 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 : > '${removed}'; exit 0; 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 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 @@ -74,7 +76,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, slow, busy, oldCli, hangAt, hung, unreach, identity]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, 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]!; @@ -115,13 +117,16 @@ describe('task storage probe against Docker answers', () => { expect(left()).toBe(false); }, 60_000); it('removes a create that lands after the killed client, within the settle window', async () => { - // The first removals find nothing (rm still exits 0); only asking until the window closes catches the late create. + // 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 interleaving this test is about: the create landed before the client was killed. - expect(existsSync(landAt)).toBe(true); expect(performance.now() - started).toBeGreaterThanOrEqual(10_000); + // 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); From e20ea0156da9180ac5a5e00d9a87f4438019e635 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 18:18:37 -0700 Subject: [PATCH 25/40] Assert the absent-then-silent order, and say what the old-CLI fake leaves unchecked Independent review of #180, round 24. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 6809b368..7e73a7cf 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -28,7 +28,8 @@ beforeAll(() => { // 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. + // 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}')" ]; } @@ -184,7 +185,7 @@ describe('task storage probe against Docker answers', () => { expect(performance.now() - started).toBeLessThan(4_000); }, 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 two seconds before the give-up time. Only the + // 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)); @@ -199,6 +200,8 @@ describe('task storage probe against Docker answers', () => { // 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' } }); From d39d624b02e6893224a2cb783e532679f020bd8b Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 18:36:50 -0700 Subject: [PATCH 26/40] Judge the probe cleanup's settle window by when each inspect was asked Independent review of #180, round 25: an absent answer to an inspect asked inside the window, arriving after it while a create lands, was taken as settled and left the container. The window now uses the time the inspect was asked; a fake with a slow, mid-landing inspect tests it. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 4 +++- test/path-identity-probe.test.ts | 21 ++++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index c3f039d2..8aa8fe0c 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -473,7 +473,9 @@ async function removeProbeContainer(name: string, settleMs: number, cleanupMs: n 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]); - if (absent(await ask(['container', 'inspect', name])) && performance.now() >= settleBy) return; + // 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); } diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 7e73a7cf..8909c275 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,7 +9,7 @@ import { PATH_IDENTITY_SCRIPT, probeTaskStoragePathIdentity } from '../agents/co // 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 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 slowLand = join(root, 'slow-land'), 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(() => { @@ -22,7 +22,7 @@ beforeAll(() => { // 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. Of rm and inspect, the code reads only the exit status + // 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.5 s to arrive (a daemon under load). 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 @@ -66,6 +66,11 @@ if [ "$1" = rm ]; then 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.5 + 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 @@ -77,7 +82,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, lateMark, rmLog, slow, busy, oldCli, hangAt, hung, unreach, identity]) if (existsSync(file)) rmSync(file); }); +beforeEach(() => { for (const file of [calls, landAt, removed, slowLand, 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]!; @@ -242,4 +247,14 @@ describe('task storage probe against Docker answers', () => { 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 ran, and the create landed during it. + expect(Number(readFileSync(inspects, 'utf8').split('\n').filter(Boolean).length)).toBeGreaterThan(6); + expect(existsSync(landAt)).toBe(true); + // And a later rm removed it. + expect(existsSync(removed)).toBe(true); + }, 60_000); }); From f361a98f525bfa691707f6a97d9f317d7305e2ec Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 18:50:39 -0700 Subject: [PATCH 27/40] Assert the slow inspect answered, and test the default 30 s give-up Independent review of #180, round 26. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 8909c275..d3bff0d9 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -9,7 +9,7 @@ import { PATH_IDENTITY_SCRIPT, probeTaskStoragePathIdentity } from '../agents/co // 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'), 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 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(() => { @@ -22,7 +22,7 @@ beforeAll(() => { // 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.5 s to arrive (a daemon under load). Of rm and inspect, the code reads only the exit status + // 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 @@ -68,8 +68,8 @@ fi if [ "$1 $2" = "container inspect" ]; then echo x >> '${inspects}' if [ -e '${slowLand}' ] && [ "$(wc -l < '${inspects}')" -eq 6 ]; then - echo 0 > '${landAt}'; sleep 9.5 - echo '[]'; echo "Error response from daemon: No such container: $3" >&2; exit 1 + 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 @@ -82,7 +82,7 @@ exit 0 imageId = buildAgentImage(); }); afterAll(() => { process.env.PATH = savedPath; rmSync(root, { recursive: true, force: true }); }); -beforeEach(() => { for (const file of [calls, landAt, removed, slowLand, inspects, lateMark, rmLog, slow, busy, oldCli, hangAt, hung, unreach, identity]) if (existsSync(file)) rmSync(file); }); +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]!; @@ -251,10 +251,21 @@ describe('task storage probe against Docker answers', () => { // 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 ran, and the create landed during it. - expect(Number(readFileSync(inspects, 'utf8').split('\n').filter(Boolean).length)).toBeGreaterThan(6); + // 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; + expect(elapsed).toBeGreaterThanOrEqual(40_000); + expect(elapsed).toBeLessThan(50_000); + }, 90_000); }); From 4d2c8ba853e907cf2b0040835bfd4f617f8c67c5 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 19:05:09 -0700 Subject: [PATCH 28/40] Pin the default give-up from both sides Independent review of #180, round 27: the default-bound test accepted up to 50 s, so a longer settle window or give-up passed; it now allows 3 s over the 40 s it states. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index d3bff0d9..7f3d1794 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -265,7 +265,8 @@ describe('task storage probe against Docker answers', () => { 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(50_000); + expect(elapsed).toBeLessThan(43_000); }, 90_000); }); From 33943553c0725eedfcfda83ac7be0dcc08af3d06 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 19:35:22 -0700 Subject: [PATCH 29/40] Pin the cleanup call deadline's floor and cap exactly Independent review of #180, round 28: timing tests caught only some changes to the 1 s floor and 10 s cap. The deadline is now a small exported function with exact values tested in both directions; the timing tests still show the cleanup uses it. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 4 +++- test/path-identity-probe.test.ts | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 8aa8fe0c..4756a111 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -458,6 +458,8 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = * 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. */ +/** 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))); async function removeProbeContainer(name: string, settleMs: number, cleanupMs: number): Promise { const settleBy = performance.now() + settleMs, giveUpAt = settleBy + cleanupMs; let last: DockerOutcome | undefined; @@ -468,7 +470,7 @@ async function removeProbeContainer(name: string, settleMs: number, cleanupMs: n 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: Math.min(10_000, Math.max(1_000, Math.ceil(giveUpAt - performance.now()))) }); + timeoutMs: probeCleanupCallMs(giveUpAt - performance.now()) }); }; for (;;) { // Whatever rm answered (older CLIs exit non-zero for a missing name, newer ones 0), the inspect decides. diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 7f3d1794..23c788a0 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -3,7 +3,7 @@ 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, probeTaskStoragePathIdentity } from '../agents/container/storage.ts'; +import { PATH_IDENTITY_SCRIPT, 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. @@ -269,4 +269,9 @@ describe('task storage probe against Docker answers', () => { 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', () => { + // Pinned in both directions; the timing tests above show the call actually uses this deadline. + 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]); + }); }); From 15dfb3d6e108e03be43cc057c1561937e82fa686 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 19:53:33 -0700 Subject: [PATCH 30/40] Name and pin the probe cleanup's settle and give-up values; reattach a doc Independent review of #180, round 29: changes under about 3 s to the default give-up or the settle window passed the timing test. Both are now named exports with their exact values tested; the timing test shows they are the ones in effect. The removeProbeContainer doc sits above its function again. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 12 ++++++++---- test/path-identity-probe.test.ts | 6 +++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 4756a111..1806c530 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -425,7 +425,7 @@ export const PROBE_VOLUME_MOUNT = ['type=volume', 'target=/work', 'volume-driver * 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). A recovery sweep is #181. */ -export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000, cleanupMs = 30_000): Promise { +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()}`; @@ -442,7 +442,7 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = // 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 : CREATE_SETTLE_MS, cleanupMs); } + 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]) }); @@ -454,12 +454,16 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 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. */ -/** 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))); async function removeProbeContainer(name: string, settleMs: number, cleanupMs: number): Promise { const settleBy = performance.now() + settleMs, giveUpAt = settleBy + cleanupMs; let last: DockerOutcome | undefined; diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 23c788a0..561f4279 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -3,7 +3,7 @@ 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, probeCleanupCallMs, probeTaskStoragePathIdentity } from '../agents/container/storage.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. @@ -274,4 +274,8 @@ describe('task storage probe against Docker answers', () => { 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]); + }); }); From 5c90eb6bb6ebf8364e217e59142f56ff11745bd4 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 20:12:37 -0700 Subject: [PATCH 31/40] Check that the probe's settle window starts when the client is stopped Independent review of #180, round 30. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 561f4279..5ba1c80c 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -128,7 +128,8 @@ describe('task storage probe against Docker answers', () => { writeFileSync(mode, 'late'); const started = performance.now(); await expect(probeTaskStoragePathIdentity(imageId, 2_000)).rejects.toThrow('client killed or not started'); - expect(performance.now() - started).toBeGreaterThanOrEqual(10_000); + // The window starts when the client is stopped (its 2 s deadline), not when it was started. + expect(performance.now() - started).toBeGreaterThanOrEqual(2_000 + 10_000); // 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'); From 30454b46df247ed60fd1285347c7599f8f1313ed Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 20:55:25 -0700 Subject: [PATCH 32/40] Test the never-started check's synchronous spawn failure (E2BIG) Independent review of #180, round 32. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 5ba1c80c..eab256b8 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -279,4 +279,19 @@ describe('task storage probe against Docker answers', () => { // 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); + }); }); From c578e8168f78fd84eacf4791881056d29f4518d9 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 21:25:11 -0700 Subject: [PATCH 33/40] Scope the real-Docker volume checks to the probe's own volumes Independent review of #180, round 33: both tests compared every volume on the daemon, so another run on it could fail them. They now count only volumes with the probe's tmpfs options, or the container's own volume by name. The per-call deadline comment states the tolerance the timing tests give. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 3 ++- test/runner-workspace.test.ts | 25 +++++++++++++++++-------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index eab256b8..04631cd8 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -271,7 +271,8 @@ describe('task storage probe against Docker answers', () => { 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', () => { - // Pinned in both directions; the timing tests above show the call actually uses this deadline. + // 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–3 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]); }); diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts index 191c24ed..75144746 100644 --- a/test/runner-workspace.test.ts +++ b/test/runner-workspace.test.ts @@ -148,25 +148,34 @@ describe('real task workspace (#87)', () => { }); describe('task storage path identity (#162)', () => { + // Only volumes made with the probe's own tmpfs options count, so another run on the same daemon cannot change the + // result. + 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 volumes = () => new Set(docker('volume', 'ls', '-q').split('\n').filter(Boolean)); - const before = volumes(); + 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([...volumes()].filter(volume => !before.has(volume))).toEqual([]); + 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 volumes = () => new Set(docker('volume', 'ls', '-q').split('\n').filter(Boolean)); - const before = volumes(), name = `codeboost-path-identity-${randomUUID()}`; + const name = `codeboost-path-identity-${randomUUID()}`; docker('create', '--name', name, '--mount', PROBE_VOLUME_MOUNT, '--entrypoint', 'true', imageId); - try { expect([...volumes()].filter(volume => !before.has(volume))).toHaveLength(1); } - finally { docker('rm', '--force', '--volumes', name); } - expect([...volumes()].filter(volume => !before.has(volume))).toEqual([]); + 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.'); From 86b10ec9a8a21801c033f1f1be523cb80924f457 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 21:41:47 -0700 Subject: [PATCH 34/40] State the real-Docker probe test's one-probe-per-daemon precondition Independent review of #180, round 34. Co-Authored-By: Claude Opus 5.5 --- test/runner-workspace.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts index 75144746..0ac8ef92 100644 --- a/test/runner-workspace.test.ts +++ b/test/runner-workspace.test.ts @@ -148,8 +148,10 @@ describe('real task workspace (#87)', () => { }); describe('task storage path identity (#162)', () => { - // Only volumes made with the probe's own tmpfs options count, so another run on the same daemon cannot change the - // result. + // 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 From 911331b06f1023a232f2260ce32f9ea7756b316c Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 22:16:54 -0700 Subject: [PATCH 35/40] Say which constant script each script test catches, and where Independent review of #180, round 36. Co-Authored-By: Claude Opus 5.5 --- test/path-identity.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/path-identity.test.ts b/test/path-identity.test.ts index f71e49e2..b2af5cc3 100644 --- a/test/path-identity.test.ts +++ b/test/path-identity.test.ts @@ -41,8 +41,9 @@ describe('path identity check', () => { }); describe('task storage probe script', () => { - // The container probe runs this script; here it runs on a host folder whose answer the host probe knows. On a macOS - // host (APFS folds both) a script that stopped measuring, and printed a constant, would disagree. + // 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' }); @@ -52,8 +53,9 @@ describe('task storage probe script', () => { 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', () => { - // On Linux CI the honest answer above is the same constant task storage gives; this one is not, so a script that - // printed a constant instead of looking fails here everywhere. + // 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'); From 085b9c6825e2bdd9625a93063072dadf678626de Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 22:42:07 -0700 Subject: [PATCH 36/40] Word the fakes rule by the parts of an answer the code reads Independent review of #180, round 37: as written, the rule required a fake to reproduce output the code never reads (a removed container's name, full inspect JSON, build logs). Changed in both mirrored files. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- CLAUDE.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2ab8bf05..556c1fcc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,7 +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 answer every call the code reads with the exit status, stdout and stderr the real tool gives, error cases included. 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. +- 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 65c28a33..a9c9ffcd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,7 +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 answer every call the code reads with the exit status, stdout and stderr the real tool gives, error cases included. 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. +- 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. From ac89ce15e1e70663c32464d05d2a0c05f4209abe Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 22:42:07 -0700 Subject: [PATCH 37/40] Pin the probe cleanup's give-up start and the settle window's end Independent review of #180, round 37: giving up counted from the start of docker run, or a 15 s window, passed every test. The give-up test now needs the client deadline, the window and the cleanup time; the late-create test bounds the window from above. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 04631cd8..0e5e06d0 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -128,8 +128,10 @@ describe('task storage probe against Docker answers', () => { 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 (its 2 s deadline), not when it was started. - expect(performance.now() - started).toBeGreaterThanOrEqual(2_000 + 10_000); + // 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'); @@ -149,7 +151,10 @@ describe('task storage probe against Docker answers', () => { }); 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(); From 609ed784e7eaa19ad12d70131199edcf7e943924 Mon Sep 17 00:00:00 2001 From: mchwang Date: Mon, 19 Oct 2026 23:26:53 -0700 Subject: [PATCH 38/40] State the cleanup bound's pause and the unconfirmed volume removal Independent review of #180, round 38. The overshoot bound includes one 250 ms pause. The volume's removal is Docker's and is not confirmed; the maintainer accepted that as part of #181. Co-Authored-By: Claude Opus 5.5 --- agents/container/storage.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 1806c530..de5c718b 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -423,7 +423,8 @@ export const PROBE_VOLUME_MOUNT = ['type=volume', 'target=/work', 'volume-driver * 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). A recovery sweep is #181. + * 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.'); @@ -469,7 +470,7 @@ async function removeProbeContainer(name: string, settleMs: number, cleanupMs: n 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 and its stop grace. + // 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 }); From 0b74c052090437b836e7eee1657ad38f1220a52b Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 00:31:47 -0700 Subject: [PATCH 39/40] Seed one name per field in the script tests; assert the busy refusals Independent review of #180, round 42: with CASE and the NFD name seeded together, a script whose case check read the NFD name (or the reverse) passed everywhere; one seeded name per field catches it on Linux CI. The busy test now shows both refusals were answered before a later rm removed the container. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 3 +++ test/path-identity.test.ts | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 0e5e06d0..281dd7e5 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -180,6 +180,9 @@ describe('task storage probe against Docker answers', () => { 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, ''); diff --git a/test/path-identity.test.ts b/test/path-identity.test.ts index b2af5cc3..6ce22134 100644 --- a/test/path-identity.test.ts +++ b/test/path-identity.test.ts @@ -60,4 +60,15 @@ describe('task storage probe script', () => { 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`); + }); }); From bd65ff2e1b63a3590a0eef3632e34861357bcc9b Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 00:56:24 -0700 Subject: [PATCH 40/40] Tighten the hung-daemon test so a floor near 4 s fails Independent review of #180, round 43: a 3.7 s per-call floor at the call site passed under the 4 s ceiling; the ceiling is now 2.5 s and the stated floor tolerance roughly 0.4-2.5 s. Co-Authored-By: Claude Opus 5.5 --- test/path-identity-probe.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/path-identity-probe.test.ts b/test/path-identity-probe.test.ts index 281dd7e5..a0198512 100644 --- a/test/path-identity-probe.test.ts +++ b/test/path-identity-probe.test.ts @@ -196,7 +196,8 @@ describe('task storage probe against Docker answers', () => { writeFileSync(mode, 'refused'); writeFileSync(hangAt, '0'); const started = performance.now(); await expect(probeTaskStoragePathIdentity(imageId, 60_000, 1_000)).rejects.toThrow('may remain'); - expect(performance.now() - started).toBeLessThan(4_000); + // 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 @@ -280,7 +281,7 @@ describe('task storage probe against Docker answers', () => { }, 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–3 s), not exactly. + // 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]); });