Harden review actions and complete bounded file previews

Author: mchwang

.gitattributes

@@ -0,0 +1,4 @@
+# These are hand-authored application sources, not build output.
+web/public/*.js linguist-generated=false linguist-vendored=false
+web/public/*.css linguist-generated=false linguist-vendored=false
+web/public/*.html linguist-generated=false linguist-vendored=false

core/linking.ts

@@ -1,7 +1,7 @@
 import { diffArrays } from 'diff';
 import type { Plan } from './plan.ts';
 
-export interface FileVersion { oid: string; mode: string; text: string | null }
+export interface FileVersion { oid: string; mode: string; text: string | null; byteSize?: number; preview?: string }
 export interface ContextRange { oldStart: number; oldCount: number; newStart: number; newCount: number; name: string }
 export interface FileDelta {
   oldPath: string | null; newPath: string | null;

docs/implementation/read-only-review.md

@@ -21,10 +21,16 @@ Store schema v2 adds the review counter and per-item notes through a transaction
 ## Known limits and remaining gates
 
 - No agent answers, test execution, AI findings ingestion, send-to-agent action, or merge control is included. These belong after the go/no-go gate. Four checks distinguish unavailable evidence from success.
-- File cards show mode/path/object IDs; binary/image previews and byte-size retrieval are not implemented and explicitly say unavailable. The pure history API does not expose binary bytes.
+- File cards show mode/path/object IDs and blob byte sizes. PNG/JPEG/GIF/WebP previews are bounded to 1 MiB each and 4 MiB across a history; unsupported/oversized images say unavailable. Gitlink byte sizes are not applicable. SVG/HTML is never embedded.
 - The protocol at `docs/experiments/review-protocol.md` must be filled with the real pairs and committed before the first timed review. The manual assignment, real paired PRs, human timing, and final result are pending. Do not mark issue #3 closed or claim the gate passed.
 - The planting helper is intentionally limited to disposable clones and supported regular top-level paths; it never publishes PRs.
 
 ## Validation
 
 Baseline before this slice: 157 tests. Run `npm run typecheck`, `npm test`, and `npm run test:browser`. Browser tests use real Git and SQLite with Chromium and cover persistence, assignments, metadata, no-change approval, stale views, unsafe origins, keyboard/breakpoints, error display, and untrusted text. A browser regression exposed false stale reasons from JSON field order; structural comparison replaced that check. The plant test verifies source HEAD stays unchanged and both plants retain ledger ownership.
+
+A large-change regression reproduced HTTP 413 when the browser sent the full content-based choice key for a 20 KB segment. Browser segment IDs are now bounded SHA-256 identifiers; the runner reconstructs the original identity/content/copy key before saving the choice. This retains choice expiry semantics without sending source content back in a review command.
+
+## Review round 1
+
+Reproduced and fixed no-change approval with item-owned ambiguous segments; the runner refuses it and the UI directs the user to resolve attribution first. Previously approved no-change items become stale if ambiguous work appears. Reproduced malformed non-ASCII credentials returning a generic conflict instead of unauthorized; credentials now require the expected ASCII hex shape before constant-time comparison. Reproduced stale item controls surviving a failed refresh; errors now discard the loaded view and require refresh. Added acceptance persistence and whole-plan empty-state browser coverage. No findings declined.

git/history.ts

@@ -89,7 +89,8 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim
     return { sha, parent, files: [] as FileDelta[] };
   });
   if (expectedParent !== head) throw new Error('The base must be an ancestor of the head.');
-  const blobs = new Map<string, string | null>();
+  const blobs = new Map<string, { text: string | null; byteSize: number; preview?: string }>();
+  let previewBytes = 0;
   const version = (oid: string, mode: string): FileVersion | null => {
     if (/^0+$/.test(oid)) return null;
     if (mode === '160000') return { oid, mode, text: null }; // gitlink is not a local blob
@@ -100,9 +101,18 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim
       blobBytes += data.length;
       let text: string | null = null;
       if (!data.includes(0)) { try { text = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(data); } catch { /* binary */ } }
-      blobs.set(oid, text);
+      let preview: string | undefined;
+      // Only bounded raster images; never embed SVG/HTML or fetch external references.
+      const mime = data.subarray(0,8).equals(Buffer.from([137,80,78,71,13,10,26,10])) ? 'image/png' :
+        data.subarray(0,3).equals(Buffer.from([255,216,255])) ? 'image/jpeg' :
+        /^GIF8[79]a$/.test(data.subarray(0,6).toString('ascii')) ? 'image/gif' :
+        data.subarray(0,4).toString('ascii') === 'RIFF' && data.subarray(8,12).toString('ascii') === 'WEBP' ? 'image/webp' : null;
+      if (mime && data.length <= 1024 * 1024 && previewBytes + data.length <= 4 * 1024 * 1024) {
+        preview = `data:${mime};base64,${data.toString('base64')}`; previewBytes += data.length;
+      }
+      blobs.set(oid, { text, byteSize: data.length, ...(preview ? { preview } : {}) });
     }
-    return { oid, mode, text: blobs.get(oid)! };
+    return { oid, mode, ...blobs.get(oid)! };
   };
   const diff = (from: string, to: string, contexts: boolean): FileDelta[] => {
     const raw = accountDiff(run('diff', '--ignore-submodules=none', '--no-relative', '--raw', '-z', '--no-abbrev', '--no-ext-diff', '--no-textconv', '-M', from, to, '--'));

runner/review.ts

@@ -25,20 +25,32 @@ export class ReviewService {
     const history = readHistory(repository, snapshot.base, 'HEAD');
     if (history.head !== snapshot.head) snapshot = this.store.recordHistory(identity, { revision: plan.revision, snapshotId: snapshot.id, reviewVersion }, history.base, history.head, []);
     const pathKey = (path: string) => {
+      if (!pathIdentity.caseSensitive && /[^\x20-\x7e]/.test(path)) throw new Error('Non-ASCII case-insensitive paths require a filesystem-specific identity adapter.');
       const normalized = pathIdentity.unicodeNormalization === 'NFC' ? path.normalize('NFC') : path;
       return pathIdentity.caseSensitive ? normalized : normalized.toLowerCase();
     };
     const raw = linkHistory(plan, history, this.store.ownership(identity, plan.revision), pathKey);
     const saved = this.store.getReview(identity), keys = choiceKeys(raw, identity);
+    const deltas = new Map(history.final.map(file => [JSON.stringify([file.newPath ?? file.oldPath, file.oldPath]), file]));
+    let previewBudget = 6 * 1024 * 1024;
+    const preview = (value: string | undefined) => {
+      if (!value || value.length > previewBudget) return null;
+      previewBudget -= value.length; return value;
+    };
     const segments = applyChoices(plan, raw, saved.choices, identity).map((segment, index) => {
       const target = plan.items.find(item => item.id === segment.row);
       if (target && segment.row !== raw[index]!.row) {
         const declared = new Set(target.files.flatMap(file => [file.path, ...(file.renamed_from ? [file.renamed_from] : [])]).map(pathKey));
         segment.scope = declared.has(pathKey(segment.path)) ? 'in-scope' : 'out-of-scope';
       }
-      return { ...segment, key: keys[index]!, originalRow: raw[index]!.row };
+      const delta = segment.kind === 'file' ? deltas.get(JSON.stringify([segment.path, segment.oldPath])) : undefined;
+      const file = delta ? { oldSize: delta.before?.byteSize ?? null, newSize: delta.after?.byteSize ?? null, beforePreview: preview(delta.before?.preview), afterPreview: preview(delta.after?.preview) } : null;
+      return { ...segment, file, key: createHash('sha256').update(keys[index]!).digest('hex'), originalRow: raw[index]!.row };
     });
     const states = approvalStates(plan, segments, saved.approvals, identity);
+    for (const item of plan.items) {
+      if (states[item.id] === 'approved' && ((!segments.some(segment => segment.row === item.id) && segments.some(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id))) || item.depends_on.some(id => states[id] === 'stale'))) states[item.id] = 'stale';
+    }
     const expected: ReviewState = { revision: plan.revision, snapshotId: snapshot.id, reviewVersion };
     const notes = this.store.getReviewNotes(identity);
     if (this.store.reviewVersion(identity) !== reviewVersion || this.store.getPlan(identity).revision !== plan.revision || this.store.getSnapshot(identity).id !== snapshot.id) throw new Error('Stale review state. Reload before writing.');
@@ -55,7 +67,7 @@ export class ReviewService {
         for (const dep of item.depends_on) if (states[dep] === 'stale') reasons.push(`Depends on ${dep}, which changed`);
         if (!reasons.length) reasons.push('Code or plan definition changed');
       }
-      return { ...item, state: states[item.id], count: owned.length, reasons, before,
+      return { ...item, state: states[item.id], count: owned.length, ambiguousCount: ambiguous, reasons, before,
         checks: { attributed: ambiguous ? `! ${ambiguous} ambiguous` : owned.length ? '✓ Attributed' : '– No changes', scope: outside.length ? `✕ ${new Set(outside).size} out of scope` : owned.length ? '✓ In scope' : '– No changes', tests: item.acceptance.some(check => check.type === 'cmd') ? '– Not run' : '– No tests defined', ai: '– Not run' }, outside: [...new Set(outside)],
       };
     });
@@ -69,13 +81,16 @@ export class ReviewService {
     if (command.token !== view.token) throw new Error('Stale review state. Reload before writing.');
     const { identity } = this.config;
     if (command.action === 'approve' && typeof command.item === 'string') {
+      const item = view.items.find(item => item.id === command.item);
+      if (item && item.count === 0 && item.ambiguousCount > 0) throw new Error('Resolve this item’s ambiguous changes before confirming no change is needed.');
       const approval = approveItem(view.plan, view.segments, command.item, identity, command.confirmNoChange === true);
       this.store.saveReview(identity, view.expected, [approval], []);
     } else if ((command.action === 'assign' || command.action === 'accept') && typeof command.key === 'string') {
       const segment = view.segments.find(segment => segment.key === command.key);
       if (!segment || !['Ambiguous', 'Unplanned'].includes(segment.row)) throw new Error('This change cannot be assigned or accepted.');
       const item = command.action === 'assign' && typeof command.item === 'string' ? command.item : null;
-      this.store.saveReview(identity, view.expected, [], [{ key: segment.key, action: command.action, item }]);
+      const storedKey = choiceKeys(view.segments, identity)[view.segments.indexOf(segment)]!;
+      this.store.saveReview(identity, view.expected, [], [{ key: storedKey, action: command.action, item }]);
     } else if (command.action === 'note' && typeof command.item === 'string' && typeof command.text === 'string' && (command.kind === 'question' || command.kind === 'change')) {
       this.store.addReviewNote(identity, view.expected, command.item, command.kind, command.text);
     } else throw new Error('Unknown review command.');

test/browser/review.spec.ts

@@ -45,3 +45,27 @@ test('shows an honest history error and keeps markup in notes as text',async({pa
  const repository=app.service.config.repository;execFileSync('git',['checkout','--orphan','unrelated'],{cwd:repository,stdio:'pipe'});execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Unrelated history'],{cwd:repository,stdio:'pipe'});
  await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('Could not read this branch’s history.',{exact:false})).toBeVisible();await expect(page.getByRole('button',{name:'Approve P1',exact:true})).not.toBeVisible();
 });
+test('can assign a large foreign change without sending its content back in the command',async({request})=>{
+ const repository=app.service.config.repository;writeFileSync(join(repository,'debug.log'),'x'.repeat(20000)+'\n');execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-am','Large foreign change'],{cwd:repository,stdio:'pipe'});
+ const base=app.url.split('#')[0]!,headers={'x-codeboost-token':app.token};const view=await(await request.get(base+'api/review',{headers})).json();const segment=view.segments.find((s:{content:string;row:string})=>s.row==='Unplanned'&&s.content.length>19000);
+ const response=await request.post(base+'api/action',{headers:{...headers,'Content-Type':'application/json'},data:{action:'assign',item:'P1',key:segment.key,token:view.token}});
+ expect(response.status()).toBe(200);
+});
+test('shows bounded raster previews and byte sizes for file-change cards',async({page})=>{
+ const repository=app.service.config.repository;writeFileSync(join(repository,'pixel.png'),Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+aRHsAAAAASUVORK5CYII=','base64'));execFileSync('git',['add','pixel.png'],{cwd:repository});execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Add image'],{cwd:repository,stdio:'pipe'});
+ await page.goto(app.url);await page.getByRole('button',{name:/Unplanned changes/}).click();await expect(page.getByRole('img',{name:'Current image in pixel.png'})).toBeVisible();await expect(page.getByRole('img',{name:'Current image in pixel.png'})).toHaveJSProperty('naturalWidth',1);await expect(page.getByText('Size: N/A → 68 bytes',{exact:true})).toBeVisible();
+});
+test('rejects malformed non-ASCII credentials consistently',async({request})=>{
+ const response=await request.get(app.url.split('#')[0]+'api/review',{headers:{'x-codeboost-token':'é'.repeat(64)}});expect(response.status()).toBe(403);
+});
+test('keeps stale item controls unavailable after a failed refresh',async({page})=>{
+ await page.goto(app.url);const repository=app.service.config.repository;execFileSync('git',['checkout','--orphan','unrelated'],{cwd:repository,stdio:'pipe'});execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Other history'],{cwd:repository,stdio:'pipe'});
+ await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('Could not read this branch’s history.',{exact:false})).toBeVisible();await expect(page.getByRole('button',{name:/P3 Confirm API compatibility/})).toHaveCount(0);
+});
+test('accepts a foreign segment and keeps that choice across reload',async({page})=>{
+ await page.goto(app.url);await page.getByRole('button',{name:/Unplanned changes/}).click();await page.getByRole('button',{name:'Accept as is',exact:true}).first().click();await page.getByRole('button',{name:/Accepted 1/}).click();await expect(page.getByRole('article').first()).toBeVisible();await page.reload();await page.getByRole('button',{name:/Accepted 1/}).click();await expect(page.getByRole('article').first()).toBeVisible();
+});
+test('shows the whole-plan empty state without claiming checks passed',async({page})=>{
+ const {repository,identity}=app.service.config;const base=app.service.store.getSnapshot(identity).base;execFileSync('git',['reset','--hard',base],{cwd:repository,stdio:'pipe'});
+ await page.goto(app.url);await expect(page.getByRole('heading',{name:'No code changes yet'})).toBeVisible();await expect(page.getByRole('button',{name:'Confirm no change needed',exact:true})).toBeVisible();await expect(page.getByRole('button',{name:'AI review: – Not run',exact:true})).toBeVisible();
+});

test/review.test.ts

@@ -1,10 +1,12 @@
-import { afterEach, it, expect } from 'vitest';
+import { afterEach, it, expect, vi } from 'vitest';
 import { mkdtempSync, rmSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { DatabaseSync } from 'node:sqlite';
 import { createDemo } from '../scripts/demo.ts';
 import { ReviewService } from '../runner/review.ts';
+// Each integration case performs several bounded real-Git reads.
+vi.setConfig({ testTimeout: 15000 });
 const roots:string[]=[];const services:ReviewService[]=[];
 afterEach(()=>{services.splice(0).forEach(service=>service.close());roots.splice(0).forEach(root=>rmSync(root,{recursive:true,force:true}));});
 function fixture(){const root=mkdtempSync(join(tmpdir(),'codeboost-review-'));roots.push(root);const config=createDemo(join(root,'demo'));const service=new ReviewService(config);services.push(service);return {service,config};}
@@ -29,3 +31,12 @@ it('rejects a concurrent store review edit through the atomic review counter',()
  other.store.addReviewNote(config.identity,view.expected,'P1','change','Please explain');
  expect(()=>service.store.saveReview(config.identity,view.expected,[],[])).toThrow(/Stale/);
 });
+it('refuses no-change confirmation while the item still owns ambiguous changes',async()=>{
+ const {service,config}=fixture();const {writeFileSync}=await import('node:fs');const {execFileSync}=await import('node:child_process');
+ writeFileSync(join(config.repository,'retry.ts'),'export function delay(attempt: number) {\n  return Math.min(10000, 200 * 2 ** attempt);\n}\n');
+ execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-am','P2 changes retry'],{cwd:config.repository,stdio:'pipe'});
+ const head=execFileSync('git',['rev-parse','HEAD'],{cwd:config.repository,encoding:'utf8'}).trim();const snapshot=service.store.getSnapshot(config.identity);
+ service.store.recordHistory(config.identity,{revision:1,snapshotId:snapshot.id},snapshot.base,head,[{sha:head,owner:'P2',origin:'owned',sourceSha:null}]);
+ const view=service.load();expect(view.items[0]!.count).toBe(0);expect(view.segments.some(s=>s.row==='Ambiguous'&&s.owners.includes('P1'))).toBe(true);
+ expect(()=>service.act({action:'approve',item:'P1',confirmNoChange:true,token:view.token})).toThrow(/ambiguous/i);
+});