diff --git a/README.md b/README.md index 59a7095..6423432 100644 --- a/README.md +++ b/README.md @@ -492,3 +492,21 @@ code changes are needed. - **Loop paused unexpectedly** — check `get_loop_status` → `pending_approval`; a safety guard tripped. Review the change, then re-run with adjusted scope. ``` + + +### Verifier regression hardening (October 2026) + +- A checklist item without a machine check requires at least one actual passing + verification command. An empty verification list is not evidence of success. +- File checks require an existing regular file with both lexical and resolved + paths inside the project. Escaping symlinks are rejected. This is a defensive + check, not an OS sandbox against concurrent malicious filesystem rewrites. +- Git change capture uses NUL-delimited paths, preserving whitespace, quotes, + tabs/newlines and literal ` -> ` text. Renames appear as delete/add so both + affected paths reach the guards. +- Untracked symlinks are not followed for line counts. A final newline no longer + adds a phantom line to the change totals. + +`npm test` runs the mocked integration suite, not Codex inference. The added tests +use temporary synthetic Git projects. Actual model/CLI behavior is not certified +by these checks; no model calls are needed to execute the suite. diff --git a/src/verifier.ts b/src/verifier.ts index 7bd1bc0..df85edc 100644 --- a/src/verifier.ts +++ b/src/verifier.ts @@ -98,9 +98,11 @@ async function hasHead(cwd: string): Promise { async function countLines(file: string): Promise { try { + // A git symlink is a path record, not permission to read its target. + if (!(await fs.lstat(file)).isFile()) return 0; const data = await fs.readFile(file, 'utf8'); if (data.length === 0) return 0; - return data.split('\n').length; + return data.split('\n').length - (data.endsWith('\n') ? 1 : 0); } catch { return 0; } @@ -129,12 +131,12 @@ export async function getDiffStat(projectDir: string): Promise { // numstat for tracked changes (added\tdeleted\tpath); binary => '-' const numstatRef = headExists ? 'HEAD' : ''; const numstat = await git( - ['diff', '--numstat', ...(numstatRef ? [numstatRef] : [])], + ['diff', '--no-renames', '--numstat', '-z', ...(numstatRef ? [numstatRef] : [])], cwd, ); const counts = new Map(); if (numstat) { - for (const line of numstat.split('\n')) { + for (const line of numstat.split('\0')) { if (!line.trim()) continue; const parts = line.split('\t'); if (parts.length < 3) continue; @@ -146,17 +148,16 @@ export async function getDiffStat(projectDir: string): Promise { // porcelain status for the authoritative file+status list (incl. untracked) const status = await git( - ['status', '--porcelain=v1', '--untracked-files=all'], + ['status', '--porcelain=v1', '-z', '--untracked-files=all', '--no-renames'], cwd, ); if (status) { - for (const line of status.split('\n')) { + for (const line of status.split('\0')) { if (!line.trim()) continue; const xy = line.slice(0, 2); - let filePart = line.slice(3); - // renames: "old -> new" (use the last segment in case a path contains ' -> ') - if (filePart.includes(' -> ')) filePart = filePart.split(' -> ').pop()!; - const p = filePart.trim(); + // NUL-delimited porcelain leaves the filename exact, without C quoting. + // Rename detection is disabled so both old/new paths remain reviewable. + const p = line.slice(3); const isUntracked = xy === '??'; const isDeleted = xy.includes('D'); const statusLetter = isUntracked ? 'A' : xy.trim() || 'M'; @@ -207,6 +208,22 @@ export async function runVerification( return results; } +/** Resolve an existing file only when both its lexical and real paths stay inside the project. */ +async function projectCheckFile(cwd: string, requested: string): Promise { + try { + const inside = (root: string, file: string): boolean => { + const relative = path.relative(root, file); + return relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative); + }; + const candidate = path.resolve(cwd, requested); + if (!inside(cwd, candidate)) return null; + const root = await fs.realpath(cwd); + const real = await fs.realpath(candidate); + if (!inside(root, real) || !(await fs.stat(real)).isFile()) return null; + return real; + } catch { return null; } +} + /** Evaluate a single checklist machine check against the repo. */ export async function evaluateCheck( projectDir: string, @@ -216,15 +233,16 @@ export async function evaluateCheck( switch (check.type) { case 'fileExists': { try { - await fs.access(path.resolve(cwd, check.path)); - return true; + return (await projectCheckFile(cwd, check.path)) !== null; } catch { return false; } } case 'fileContains': { try { - const data = await fs.readFile(path.resolve(cwd, check.path), 'utf8'); + const file = await projectCheckFile(cwd, check.path); + if (file === null) return false; + const data = await fs.readFile(file, 'utf8'); try { return new RegExp(check.pattern).test(data); } catch { @@ -269,7 +287,7 @@ export async function review( evidence = done ? describeCheck(item.check) : undefined; } else { // verification-gated item - done = allVerificationPassed; + done = verification.length > 0 && allVerificationPassed; evidence = done ? 'all verification commands passed' : undefined; } updated.push({ ...item, done, evidence }); @@ -313,3 +331,4 @@ function describeCheck(check: CheckSpec): string { return `command passed: ${check.command}`; } } + diff --git a/test/verifier-regression.test.ts b/test/verifier-regression.test.ts new file mode 100644 index 0000000..56148cc --- /dev/null +++ b/test/verifier-regression.test.ts @@ -0,0 +1,81 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { promises as fs } from 'node:fs'; +import * as path from 'node:path'; +import { evaluateCheck, getDiffStat, review, runCommand } from '../src/verifier.js'; +import { makeProject } from './helpers.js'; + +const emptyDiff = { files: [], totalAdded: 0, totalDeleted: 0, isGitRepo: true, rawSummary: '' }; + +test('plain checklist without verification cannot claim completion', async () => { + const dir = await makeProject(); + try { + const result = await review(dir, [{id:'a',text:'Verify manually',done:false}], [], emptyDiff); + assert.equal(result.review.complete, false); + assert.equal(result.checklist[0]?.done, false); + } finally { await fs.rm(dir,{recursive:true,force:true}); } +}); + +test('file checks reject lexical and symlink escapes', async () => { + const dir = await makeProject(); + const outside = await fs.mkdtemp(path.join(path.dirname(dir), 'outside-check-')); + try { + await fs.writeFile(path.join(outside,'fixture.txt'), 'external fixture'); + await fs.symlink(outside, path.join(dir,'external'), 'dir'); + for (const name of [path.join(outside,'fixture.txt'), path.relative(dir,path.join(outside,'fixture.txt')), 'external/fixture.txt']) { + assert.equal(await evaluateCheck(dir, {type:'fileExists',path:name}), false, name); + assert.equal(await evaluateCheck(dir, {type:'fileContains',path:name,pattern:'external'}), false, name); + } + await fs.writeFile(path.join(dir,'safe.txt'),'safe'); + assert.equal(await evaluateCheck(dir,{type:'fileContains',path:'safe.txt',pattern:'safe'}),true); + } finally { await fs.rm(dir,{recursive:true,force:true}); await fs.rm(outside,{recursive:true,force:true}); } +}); + +test('git filenames retain whitespace, quotes, tabs and arrow text', async () => { + const dir = await makeProject(); + try { + const names=[' leading.txt','trailing.txt ','a\tb.txt','a\nb.txt','old -> new.txt','quote".txt']; + for(const name of names) await fs.writeFile(path.join(dir,name),'one\n'); + let diff=await getDiffStat(dir); + assert.deepEqual(diff.files.map(f=>f.path).sort(), [...names].sort()); + for(const file of diff.files) assert.equal(file.added,1,file.path); + await runCommand('git add -A && git commit -qm names',dir,30000); + for(const name of names) await fs.appendFile(path.join(dir,name),'two\n'); + diff=await getDiffStat(dir); + assert.deepEqual(diff.files.map(f=>f.path).sort(), [...names].sort()); + for(const file of diff.files) assert.equal(file.added,1,file.path); + } finally { await fs.rm(dir,{recursive:true,force:true}); } +}); + +test('untracked symlinks are not read as target file content', async () => { + const dir=await makeProject(); + try { + await fs.symlink('PROJECT_GOAL.md',path.join(dir,'goal-link')); + const diff=await getDiffStat(dir); + assert.equal(diff.files.find(f=>f.path==='goal-link')?.added,0); + } finally { await fs.rm(dir,{recursive:true,force:true}); } +}); + +test('rename preserves both source deletion and destination addition', async()=> { + const dir=await makeProject(); + try { + await fs.rename(path.join(dir,'greet.test.js'),path.join(dir,'renamed test.js')); + await runCommand('git add -A',dir,30000); + const diff=await getDiffStat(dir); + assert.ok(diff.files.some(f=>f.path==='greet.test.js' && f.status==='D')); + assert.ok(diff.files.some(f=>f.path==='renamed test.js' && f.added>0)); + } finally { await fs.rm(dir,{recursive:true,force:true}); } +}); + +test('untracked line counts handle empty and unterminated files', async()=> { + const dir=await makeProject(); + try { + await fs.writeFile(path.join(dir,'empty.txt'),''); + await fs.writeFile(path.join(dir,'one.txt'),'one'); + await fs.writeFile(path.join(dir,'two.txt'),'one\ntwo\n'); + const diff=await getDiffStat(dir); + for(const [name,count] of [['empty.txt',0],['one.txt',1],['two.txt',2]] as const) { + assert.equal(diff.files.find(f=>f.path===name)?.added,count); + } + } finally { await fs.rm(dir,{recursive:true,force:true}); } +});