From 7bc6051862cf0e4a72bf0eec58b563944d38a164 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 17:21:31 -0700 Subject: [PATCH 1/6] Validate review summary edge cases --- docs/experiments/review-summary-edge-cases.md | 25 +++++++++++++++++++ test/plant.test.ts | 19 +++++++++++++- test/review.test.ts | 15 ++++++++++- 3 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 docs/experiments/review-summary-edge-cases.md diff --git a/docs/experiments/review-summary-edge-cases.md b/docs/experiments/review-summary-edge-cases.md new file mode 100644 index 0000000..d27c3bb --- /dev/null +++ b/docs/experiments/review-summary-edge-cases.md @@ -0,0 +1,25 @@ +# Review-summary edge-case validation (#10) + +Validated on 2026-09-23 from `5a2685c`, before selecting real issues for the plan-indexed review experiment. These were summary-only concerns from PR #9, not reproduced defects. No production behavior changed during this validation. + +## Renamed-file reassignment + +A real-Git fixture declares `retry.ts` → `renamed.ts`, then creates an unplanned rename with a content edit. The review exposes unplanned segments on both the removed old path and added new path. Manually assigning every segment to the rename item leaves each segment in scope. + +Result: no defect reproduced. `ReviewService` evaluates a manual assignment against both `path` and `renamed_from`. The integration fixture remains as coverage. + +## Literal pathspec metacharacters + +A planting fixture uses the literal declared filename `*.txt` beside a decoy `a.txt`. An owned commit edits the literal file; a later commit removes it while retaining the decoy. The plant helper rejects the history at its declared-file transition check with `Declared plant needs a regular file retained through the remaining history.` + +Result: no defect reproduced. The decoy does not satisfy the literal transition check. The fixture remains as coverage for Git pathspec metacharacters. + +## Accepted-change status styling + +The existing browser flow accepts an unplanned change, opens the Accepted row, and reloads it. The status reads `Accepted outside plan` and uses the existing muted treatment. A proposed error-color assertion failed because the rendered class is `muted`, confirming the summary did not describe the current styling accurately enough to imply a patch. + +Result: no defect reproduced. The summary supplied no expected color, state token, contrast failure, or browser failure. The explicit label already distinguishes the acknowledged exception, so no new color semantics were inferred and the existing browser coverage remains unchanged. + +## Gate result + +All three concerns are resolved as validated behavior. No issue supplied a failing case that justified a production patch. Issue #3's manual assignment and paired human go/no-go experiment remain pending and must use the frozen protocol in `review-protocol.md`. diff --git a/test/plant.test.ts b/test/plant.test.ts index a24985d..75ad05c 100644 --- a/test/plant.test.ts +++ b/test/plant.test.ts @@ -1,11 +1,13 @@ import { it,expect,afterEach,vi } from 'vitest'; -import { mkdtempSync,readFileSync,rmSync,symlinkSync } from 'node:fs'; +import { mkdirSync,mkdtempSync,readFileSync,rmSync,symlinkSync,writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { execFileSync } from 'node:child_process'; import { createDemo } from '../scripts/demo.ts'; import { plant } from '../scripts/plant.ts'; import { ReviewService } from '../runner/review.ts'; +import { Store } from '../runner/store.ts'; +import type { Plan } from '../core/plan.ts'; const roots:string[]=[];afterEach(()=>roots.splice(0).forEach(root=>rmSync(root,{recursive:true,force:true}))); it('plants in an isolated clone, retains ledger attribution, and leaves source history unchanged',()=>{ const root=mkdtempSync(join(tmpdir(),'codeboost-plant-'));roots.push(root);const config=createDemo(join(root,'source')); @@ -25,6 +27,21 @@ it('rejects canonical declared and existing path collisions before creating a cl const config=createDemo(join(root,'source'));config.pathIdentity.caseSensitive=false; for (const path of ['RETRY.TS','RUN.SH']) expect(()=>plant(config,join(root,'experiment'),{declaredText:'// extra',undeclaredText:'diagnostic',undeclaredPath:path})).toThrow(/outside every declared file|already exists/); },15000); +it('treats metacharacters literally when checking declared-file transitions',()=>{ + const root=mkdtempSync(join(tmpdir(),'codeboost-plant-literal-'));roots.push(root);const repository=join(root,'source');mkdirSync(repository); + const git=(...args:string[])=>execFileSync('git',args,{cwd:repository,encoding:'utf8',stdio:['ignore','pipe','pipe']}).trim(); + git('init','-b','main');git('config','user.name','Test');git('config','user.email','test@example.invalid');git('config','commit.gpgsign','false'); + const commit=(message:string)=>{git('add','-A');git('commit','-m',message);return git('rev-parse','HEAD');}; + writeFileSync(join(repository,'*.txt'),'base\n');writeFileSync(join(repository,'a.txt'),'decoy\n');const base=commit('Base'); + writeFileSync(join(repository,'*.txt'),'owned change\n');const owned=commit('Owned change'); + rmSync(join(repository,'*.txt'));writeFileSync(join(repository,'a.txt'),'changed decoy\n');const head=commit('Remove literal path'); + const identity={repositoryId:'literal-repo',taskId:'literal-task',planId:'literal-plan'}; + const plan:Plan={schema_version:1,revision:1,issue:10,summary:'Literal path validation',questions:[],items:[{id:'P1',title:'Edit literal path',intent:'Exercise path handling.',files:[{path:'*.txt',kind:'edit',renamed_from:null,change:'Edit the literal file.'}],acceptance:[{type:'check',text:'Literal file remains present.'}],depends_on:[]}]}; + const config={database:join(root,'review.sqlite'),repository,identity,pathIdentity:{caseSensitive:true as const,unicodeNormalization:'none' as const},demo:false}; + const store=new Store(config.database); + try{store.createPlan(JSON.stringify(plan),'json',{identity,issue:10,baseEntries:['*.txt','a.txt'].map(path=>({path,kind:'file' as const})),pathKey:path=>path,allowedCommands:[]},base,head);store.recordHistory(identity,{revision:1,snapshotId:store.getSnapshot(identity).id},base,head,[{sha:owned,owner:'P1',origin:'owned',sourceSha:null}]);}finally{store.close();} + expect(()=>plant(config,join(root,'experiment'),{declaredText:'planted',undeclaredText:'outside',undeclaredPath:'extra.txt'})).toThrow(/Declared plant needs a regular file retained/); +},15000); it('rejects experiment destinations inside the source, including symlink aliases', () => { const root=mkdtempSync(join(tmpdir(),'codeboost-plant-destination-'));roots.push(root); diff --git a/test/review.test.ts b/test/review.test.ts index 718c05d..999dc12 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -1,7 +1,8 @@ import { afterEach, it, expect, vi } from 'vitest'; -import { mkdtempSync, rmSync } from 'node:fs'; +import { mkdtempSync, renameSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { execFileSync } from 'node:child_process'; import { DatabaseSync } from 'node:sqlite'; import { createDemo } from '../scripts/demo.ts'; import { ReviewService } from '../runner/review.ts'; @@ -16,6 +17,18 @@ it('expires a browser token after another view assigns a segment and recomputes expect(next.items[0]!.checks.scope).toContain('out of scope'); expect(()=>service.act({action:'approve',item:'P1',token:view.token})).toThrow(/Stale/); }); +it('keeps both sides of a declared rename in scope after manual reassignment',()=>{ + const {service,config}=fixture(),identity=config.identity,plan=service.store.getPlan(identity); + plan.items=[{...plan.items[0]!,files:[{path:'renamed.ts',kind:'rename',renamed_from:'retry.ts',change:'Rename the implementation.'}],depends_on:[]}]; + service.store.importRevision(JSON.stringify(plan),'json',{identity,issue:plan.issue,baseEntries:['retry.ts','README.md','run.sh'].map(path=>({path,kind:'file' as const})),pathKey:path=>path,allowedCommands:[]},plan.revision); + renameSync(join(config.repository,'retry.ts'),join(config.repository,'renamed.ts')); + writeFileSync(join(config.repository,'renamed.ts'),'export function delay(attempt: number) {\n return Math.min(5000, 200 * 2 ** attempt);\n}\n'); + execFileSync('git',['-c','core.hooksPath=/dev/null','add','-A'],{cwd:config.repository}); + execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Rename retry implementation'],{cwd:config.repository,stdio:'pipe'}); + let view=service.load();const candidates=view.segments.filter(segment=>segment.row==='Unplanned'&&['retry.ts','renamed.ts'].includes(segment.path)); + expect(new Set(candidates.map(segment=>segment.path))).toEqual(new Set(['retry.ts','renamed.ts'])); + for(const candidate of candidates){view=service.act({action:'assign',key:candidate.key,item:'P1',token:view.token});expect(view.segments.find(segment=>segment.key===candidate.key)?.scope).toBe('in-scope');} +}); it('persists bounded per-item notes without creating a plan revision',()=>{ const {service,config}=fixture();const view=service.load();service.act({action:'note',item:'P1',kind:'question',text:'Why this limit?',token:view.token}); const reopened=new ReviewService(config);services.push(reopened);expect(reopened.load().notes[0]!.text).toBe('Why this limit?');expect(reopened.load().plan.revision).toBe(1); From 66ac3b36ac418fc0c76f1a3cee62dec4e0ebd7f7 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 17:26:26 -0700 Subject: [PATCH 2/6] Strengthen edge case validation evidence --- docs/experiments/review-summary-edge-cases.md | 4 ++-- test/plant.test.ts | 2 ++ test/review.test.ts | 5 +++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/experiments/review-summary-edge-cases.md b/docs/experiments/review-summary-edge-cases.md index d27c3bb..f0fe07a 100644 --- a/docs/experiments/review-summary-edge-cases.md +++ b/docs/experiments/review-summary-edge-cases.md @@ -4,13 +4,13 @@ Validated on 2026-09-23 from `5a2685c`, before selecting real issues for the pla ## Renamed-file reassignment -A real-Git fixture declares `retry.ts` → `renamed.ts`, then creates an unplanned rename with a content edit. The review exposes unplanned segments on both the removed old path and added new path. Manually assigning every segment to the rename item leaves each segment in scope. +A real-Git fixture declares `retry.ts` → `renamed.ts`, then creates an unplanned rename with a content edit. The review exposes removed and added text segments with `-` and `+` operations. The fixture resolves the removed side through `oldPath` and the added side through `path`, verifies both declared names, then manually assigns both segments. Each remains in scope. Result: no defect reproduced. `ReviewService` evaluates a manual assignment against both `path` and `renamed_from`. The integration fixture remains as coverage. ## Literal pathspec metacharacters -A planting fixture uses the literal declared filename `*.txt` beside a decoy `a.txt`. An owned commit edits the literal file; a later commit removes it while retaining the decoy. The plant helper rejects the history at its declared-file transition check with `Declared plant needs a regular file retained through the remaining history.` +A planting fixture uses the literal declared filename `*.txt` beside a decoy `a.txt`. An owned commit edits the literal file; a later commit removes it while retaining the decoy. Direct controls prove `git ls-tree ... -- '*.txt'` returns no match and that `ls-tree` rejects `:(glob)` pathspec magic as unsupported. The plant helper then rejects the history at its declared-file transition check with `Declared plant needs a regular file retained through the remaining history.` Result: no defect reproduced. The decoy does not satisfy the literal transition check. The fixture remains as coverage for Git pathspec metacharacters. diff --git a/test/plant.test.ts b/test/plant.test.ts index 75ad05c..840f05d 100644 --- a/test/plant.test.ts +++ b/test/plant.test.ts @@ -40,6 +40,8 @@ it('treats metacharacters literally when checking declared-file transitions',()= const config={database:join(root,'review.sqlite'),repository,identity,pathIdentity:{caseSensitive:true as const,unicodeNormalization:'none' as const},demo:false}; const store=new Store(config.database); try{store.createPlan(JSON.stringify(plan),'json',{identity,issue:10,baseEntries:['*.txt','a.txt'].map(path=>({path,kind:'file' as const})),pathKey:path=>path,allowedCommands:[]},base,head);store.recordHistory(identity,{revision:1,snapshotId:store.getSnapshot(identity).id},base,head,[{sha:owned,owner:'P1',origin:'owned',sourceSha:null}]);}finally{store.close();} + expect(git('ls-tree','--name-only',head,'--','*.txt')).toBe(''); + expect(()=>git('ls-tree','--name-only',head,'--',':(glob)*.txt')).toThrow(/pathspec magic not supported/); expect(()=>plant(config,join(root,'experiment'),{declaredText:'planted',undeclaredText:'outside',undeclaredPath:'extra.txt'})).toThrow(/Declared plant needs a regular file retained/); },15000); diff --git a/test/review.test.ts b/test/review.test.ts index 999dc12..210642d 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -25,8 +25,9 @@ it('keeps both sides of a declared rename in scope after manual reassignment',() writeFileSync(join(config.repository,'renamed.ts'),'export function delay(attempt: number) {\n return Math.min(5000, 200 * 2 ** attempt);\n}\n'); execFileSync('git',['-c','core.hooksPath=/dev/null','add','-A'],{cwd:config.repository}); execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Rename retry implementation'],{cwd:config.repository,stdio:'pipe'}); - let view=service.load();const candidates=view.segments.filter(segment=>segment.row==='Unplanned'&&['retry.ts','renamed.ts'].includes(segment.path)); - expect(new Set(candidates.map(segment=>segment.path))).toEqual(new Set(['retry.ts','renamed.ts'])); + let view=service.load();const candidates=view.segments.filter(segment=>segment.row==='Unplanned'&&segment.kind==='text'&&['-','+'].includes(segment.operation)&&['retry.ts','renamed.ts'].includes(segment.operation==='-'?segment.oldPath??segment.path:segment.path)); + expect(new Set(candidates.map(segment=>segment.operation))).toEqual(new Set(['-','+'])); + expect(new Set(candidates.map(segment=>segment.operation==='-'?segment.oldPath??segment.path:segment.path))).toEqual(new Set(['retry.ts','renamed.ts'])); for(const candidate of candidates){view=service.act({action:'assign',key:candidate.key,item:'P1',token:view.token});expect(view.segments.find(segment=>segment.key===candidate.key)?.scope).toBe('in-scope');} }); it('persists bounded per-item notes without creating a plan revision',()=>{ From b8f428e040e2d10222020951e12bb3a3c31f2257 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 17:27:00 -0700 Subject: [PATCH 3/6] Require direct evidence in summary concern fixtures --- AGENTS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/AGENTS.md b/AGENTS.md index 568d9bf..b9f7a34 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,5 +45,6 @@ Every reproduced race requires a failing-before and passing-after regression. As - Report current test counts separately from historical milestone counts. - Before requesting automated review, report the current head, CI state, mergeability, unresolved threads, and deferred follow-up issues. - Reproduce summary-only review concerns or turn them into a concrete follow-up issue. Do not repeatedly patch vague wording without a failure case. +- A validation fixture for a summary-only concern must assert the disputed intermediate representation or state before using a downstream outcome as evidence that the concern was exercised. - For each review round, record what changed, what was declined and why, and the regression evidence. Re-request review until a round returns no new findings. - Treat review-lesson extraction as a merge gate. Before invoking merge, classify every review finding in the PR body as: covered by an existing rule (cite it), captured by a new rule in this branch (cite it), or one-off (record why). Do not merge until this audit is complete and every required `AGENTS.md` update is included in the reviewed head. Omit rules that merely repeat existing guidance. From 08327a15c5e4789bfdc2104ec44c59b28a0b9a81 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 17:28:21 -0700 Subject: [PATCH 4/6] Type rename validation paths explicitly --- test/review.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/review.test.ts b/test/review.test.ts index 210642d..f5d773d 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -25,9 +25,12 @@ it('keeps both sides of a declared rename in scope after manual reassignment',() writeFileSync(join(config.repository,'renamed.ts'),'export function delay(attempt: number) {\n return Math.min(5000, 200 * 2 ** attempt);\n}\n'); execFileSync('git',['-c','core.hooksPath=/dev/null','add','-A'],{cwd:config.repository}); execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Rename retry implementation'],{cwd:config.repository,stdio:'pipe'}); - let view=service.load();const candidates=view.segments.filter(segment=>segment.row==='Unplanned'&&segment.kind==='text'&&['-','+'].includes(segment.operation)&&['retry.ts','renamed.ts'].includes(segment.operation==='-'?segment.oldPath??segment.path:segment.path)); + let view=service.load();const segmentPath=(segment:typeof view.segments[number]):string=>{ + const path=segment.operation==='-'?(segment.oldPath??segment.path):segment.path;return path??''; + }; + const candidates=view.segments.filter(segment=>segment.row==='Unplanned'&&segment.kind==='text'&&(segment.operation==='-'||segment.operation==='+')&&['retry.ts','renamed.ts'].includes(segmentPath(segment))); expect(new Set(candidates.map(segment=>segment.operation))).toEqual(new Set(['-','+'])); - expect(new Set(candidates.map(segment=>segment.operation==='-'?segment.oldPath??segment.path:segment.path))).toEqual(new Set(['retry.ts','renamed.ts'])); + expect(new Set(candidates.map(segmentPath))).toEqual(new Set(['retry.ts','renamed.ts'])); for(const candidate of candidates){view=service.act({action:'assign',key:candidate.key,item:'P1',token:view.token});expect(view.segments.find(segment=>segment.key===candidate.key)?.scope).toBe('in-scope');} }); it('persists bounded per-item notes without creating a plan revision',()=>{ From c4d580b2ba6d4a338b53923bfb49857194193254 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 17:30:24 -0700 Subject: [PATCH 5/6] Make rename fixture plan scope explicit --- docs/experiments/review-summary-edge-cases.md | 2 +- test/review.test.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/experiments/review-summary-edge-cases.md b/docs/experiments/review-summary-edge-cases.md index f0fe07a..2652a59 100644 --- a/docs/experiments/review-summary-edge-cases.md +++ b/docs/experiments/review-summary-edge-cases.md @@ -4,7 +4,7 @@ Validated on 2026-09-23 from `5a2685c`, before selecting real issues for the pla ## Renamed-file reassignment -A real-Git fixture declares `retry.ts` → `renamed.ts`, then creates an unplanned rename with a content edit. The review exposes removed and added text segments with `-` and `+` operations. The fixture resolves the removed side through `oldPath` and the added side through `path`, verifies both declared names, then manually assigns both segments. Each remains in scope. +A real-Git fixture reduces the demo plan to P1, declares `retry.ts` → `renamed.ts`, then creates an unplanned rename with a content edit. The review exposes removed and added text segments with `-` and `+` operations. The fixture resolves the removed side through `oldPath` and the added side through `path`, verifies both declared names, then manually assigns both segments. Each remains in scope. Result: no defect reproduced. `ReviewService` evaluates a manual assignment against both `path` and `renamed_from`. The integration fixture remains as coverage. diff --git a/test/review.test.ts b/test/review.test.ts index f5d773d..ecba988 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -20,6 +20,7 @@ it('expires a browser token after another view assigns a segment and recomputes it('keeps both sides of a declared rename in scope after manual reassignment',()=>{ const {service,config}=fixture(),identity=config.identity,plan=service.store.getPlan(identity); plan.items=[{...plan.items[0]!,files:[{path:'renamed.ts',kind:'rename',renamed_from:'retry.ts',change:'Rename the implementation.'}],depends_on:[]}]; + expect(plan.items.map(item=>item.id)).toEqual(['P1']); service.store.importRevision(JSON.stringify(plan),'json',{identity,issue:plan.issue,baseEntries:['retry.ts','README.md','run.sh'].map(path=>({path,kind:'file' as const})),pathKey:path=>path,allowedCommands:[]},plan.revision); renameSync(join(config.repository,'retry.ts'),join(config.repository,'renamed.ts')); writeFileSync(join(config.repository,'renamed.ts'),'export function delay(attempt: number) {\n return Math.min(5000, 200 * 2 ** attempt);\n}\n'); From 2708ae97c6247cdf3487653674de815d67d56bb9 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 17:43:52 -0700 Subject: [PATCH 6/6] Clarify validation evidence provenance --- AGENTS.md | 1 + docs/experiments/review-summary-edge-cases.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index b9f7a34..67e7703 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,6 +44,7 @@ Every reproduced race requires a failing-before and passing-after regression. As - Run final validation against the exact pushed head after the last change. - Report current test counts separately from historical milestone counts. - Before requesting automated review, report the current head, CI state, mergeability, unresolved threads, and deferred follow-up issues. +- In evidence records, label cited commits as baselines, intermediate checkpoints, or validated heads. Keep final exact-head results in a place that can name the resulting commit, such as the PR body or CI record. - Reproduce summary-only review concerns or turn them into a concrete follow-up issue. Do not repeatedly patch vague wording without a failure case. - A validation fixture for a summary-only concern must assert the disputed intermediate representation or state before using a downstream outcome as evidence that the concern was exercised. - For each review round, record what changed, what was declined and why, and the regression evidence. Re-request review until a round returns no new findings. diff --git a/docs/experiments/review-summary-edge-cases.md b/docs/experiments/review-summary-edge-cases.md index 2652a59..fc393fb 100644 --- a/docs/experiments/review-summary-edge-cases.md +++ b/docs/experiments/review-summary-edge-cases.md @@ -1,6 +1,6 @@ # Review-summary edge-case validation (#10) -Validated on 2026-09-23 from `5a2685c`, before selecting real issues for the plan-indexed review experiment. These were summary-only concerns from PR #9, not reproduced defects. No production behavior changed during this validation. +Validated on 2026-09-23 before selecting real issues for the plan-indexed review experiment. The validation branch started from baseline `5a2685c`; final exact-head results are recorded in PR #17. These were summary-only concerns from PR #9, not reproduced defects. No production behavior changed during this validation. ## Renamed-file reassignment