diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 3e553e86..bbc7b9b5 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 6280032d..6365ac80 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 a15f6da4..051dffce 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'; @@ -97,6 +98,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' },