Validate review-summary edge cases (#17)

* Validate review summary edge cases

* Strengthen edge case validation evidence

* Require direct evidence in summary concern fixtures

* Type rename validation paths explicitly

* Make rename fixture plan scope explicit

* Clarify validation evidence provenance

Author: mchwang

AGENTS.md

@@ -44,6 +44,8 @@ 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.
 - 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.

docs/experiments/review-summary-edge-cases.md

@@ -0,0 +1,25 @@
+# Review-summary edge-case validation (#10)
+
+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
+
+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.
+
+## 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. 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.
+
+## 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`.

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,23 @@ 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(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);
 
 it('rejects experiment destinations inside the source, including symlink aliases', () => {
  const root=mkdtempSync(join(tmpdir(),'codeboost-plant-destination-'));roots.push(root);

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,23 @@ 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:[]}];
+ 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');
+ 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 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(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',()=>{
  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);