Skip to content

Commit 7bc6051

Browse files
committed
Validate review summary edge cases
1 parent 5a2685c commit 7bc6051

3 files changed

Lines changed: 57 additions & 2 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# Review-summary edge-case validation (#10)
2+
3+
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.
4+
5+
## Renamed-file reassignment
6+
7+
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.
8+
9+
Result: no defect reproduced. `ReviewService` evaluates a manual assignment against both `path` and `renamed_from`. The integration fixture remains as coverage.
10+
11+
## Literal pathspec metacharacters
12+
13+
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.`
14+
15+
Result: no defect reproduced. The decoy does not satisfy the literal transition check. The fixture remains as coverage for Git pathspec metacharacters.
16+
17+
## Accepted-change status styling
18+
19+
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.
20+
21+
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.
22+
23+
## Gate result
24+
25+
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‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import { it,expect,afterEach,vi } from 'vitest';
2-
import { mkdtempSync,readFileSync,rmSync,symlinkSync } from 'node:fs';
2+
import { mkdirSync,mkdtempSync,readFileSync,rmSync,symlinkSync,writeFileSync } from 'node:fs';
33
import { tmpdir } from 'node:os';
44
import { join } from 'node:path';
55
import { execFileSync } from 'node:child_process';
66
import { createDemo } from '../scripts/demo.ts';
77
import { plant } from '../scripts/plant.ts';
88
import { ReviewService } from '../runner/review.ts';
9+
import { Store } from '../runner/store.ts';
10+
import type { Plan } from '../core/plan.ts';
911
const roots:string[]=[];afterEach(()=>roots.splice(0).forEach(root=>rmSync(root,{recursive:true,force:true})));
1012
it('plants in an isolated clone, retains ledger attribution, and leaves source history unchanged',()=>{
1113
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
2527
const config=createDemo(join(root,'source'));config.pathIdentity.caseSensitive=false;
2628
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/);
2729
},15000);
30+
it('treats metacharacters literally when checking declared-file transitions',()=>{
31+
const root=mkdtempSync(join(tmpdir(),'codeboost-plant-literal-'));roots.push(root);const repository=join(root,'source');mkdirSync(repository);
32+
const git=(...args:string[])=>execFileSync('git',args,{cwd:repository,encoding:'utf8',stdio:['ignore','pipe','pipe']}).trim();
33+
git('init','-b','main');git('config','user.name','Test');git('config','user.email','test@example.invalid');git('config','commit.gpgsign','false');
34+
const commit=(message:string)=>{git('add','-A');git('commit','-m',message);return git('rev-parse','HEAD');};
35+
writeFileSync(join(repository,'*.txt'),'base\n');writeFileSync(join(repository,'a.txt'),'decoy\n');const base=commit('Base');
36+
writeFileSync(join(repository,'*.txt'),'owned change\n');const owned=commit('Owned change');
37+
rmSync(join(repository,'*.txt'));writeFileSync(join(repository,'a.txt'),'changed decoy\n');const head=commit('Remove literal path');
38+
const identity={repositoryId:'literal-repo',taskId:'literal-task',planId:'literal-plan'};
39+
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:[]}]};
40+
const config={database:join(root,'review.sqlite'),repository,identity,pathIdentity:{caseSensitive:true as const,unicodeNormalization:'none' as const},demo:false};
41+
const store=new Store(config.database);
42+
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();}
43+
expect(()=>plant(config,join(root,'experiment'),{declaredText:'planted',undeclaredText:'outside',undeclaredPath:'extra.txt'})).toThrow(/Declared plant needs a regular file retained/);
44+
},15000);
2845

2946
it('rejects experiment destinations inside the source, including symlink aliases', () => {
3047
const root=mkdtempSync(join(tmpdir(),'codeboost-plant-destination-'));roots.push(root);

‎test/review.test.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { afterEach, it, expect, vi } from 'vitest';
2-
import { mkdtempSync, rmSync } from 'node:fs';
2+
import { mkdtempSync, renameSync, rmSync, writeFileSync } from 'node:fs';
33
import { tmpdir } from 'node:os';
44
import { join } from 'node:path';
5+
import { execFileSync } from 'node:child_process';
56
import { DatabaseSync } from 'node:sqlite';
67
import { createDemo } from '../scripts/demo.ts';
78
import { ReviewService } from '../runner/review.ts';
@@ -16,6 +17,18 @@ it('expires a browser token after another view assigns a segment and recomputes
1617
expect(next.items[0]!.checks.scope).toContain('out of scope');
1718
expect(()=>service.act({action:'approve',item:'P1',token:view.token})).toThrow(/Stale/);
1819
});
20+
it('keeps both sides of a declared rename in scope after manual reassignment',()=>{
21+
const {service,config}=fixture(),identity=config.identity,plan=service.store.getPlan(identity);
22+
plan.items=[{...plan.items[0]!,files:[{path:'renamed.ts',kind:'rename',renamed_from:'retry.ts',change:'Rename the implementation.'}],depends_on:[]}];
23+
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);
24+
renameSync(join(config.repository,'retry.ts'),join(config.repository,'renamed.ts'));
25+
writeFileSync(join(config.repository,'renamed.ts'),'export function delay(attempt: number) {\n return Math.min(5000, 200 * 2 ** attempt);\n}\n');
26+
execFileSync('git',['-c','core.hooksPath=/dev/null','add','-A'],{cwd:config.repository});
27+
execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Rename retry implementation'],{cwd:config.repository,stdio:'pipe'});
28+
let view=service.load();const candidates=view.segments.filter(segment=>segment.row==='Unplanned'&&['retry.ts','renamed.ts'].includes(segment.path));
29+
expect(new Set(candidates.map(segment=>segment.path))).toEqual(new Set(['retry.ts','renamed.ts']));
30+
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');}
31+
});
1932
it('persists bounded per-item notes without creating a plan revision',()=>{
2033
const {service,config}=fixture();const view=service.load();service.act({action:'note',item:'P1',kind:'question',text:'Why this limit?',token:view.token});
2134
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);

0 commit comments

Comments
 (0)