Require attribution resolution before item approval

Author: mchwang

docs/implementation/read-only-review.md

@@ -34,3 +34,7 @@ A large-change regression reproduced HTTP 413 when the browser sent the full con
 ## 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.
+
+## Review round 2
+
+Extended the attribution guard to mixed items with both owned and ambiguous segments. The regression reproduced ordinary approval succeeding with unresolved attribution. Approval now rejects any item-specific ambiguity, previously approved affected items become stale, and the UI routes every such item to attribution resolution. Added browser coverage for this mixed case. No findings declined.

runner/review.ts

@@ -49,7 +49,7 @@ export class ReviewService {
     });
     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';
+      if (states[item.id] === 'approved' && (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);
@@ -82,7 +82,7 @@ export class ReviewService {
     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.');
+      if (item && item.ambiguousCount > 0) throw new Error('Resolve this item’s ambiguous changes before approval.');
       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') {

test/browser/review.spec.ts

@@ -69,3 +69,8 @@ test('shows the whole-plan empty state without claiming checks passed',async({pa
  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('routes mixed owned and ambiguous items to attribution resolution',async({page})=>{
+ const {repository,identity}=app.service.config;writeFileSync(join(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:repository,stdio:'pipe'});
+ const head=execFileSync('git',['rev-parse','HEAD'],{cwd:repository,encoding:'utf8'}).trim(),snapshot=app.service.store.getSnapshot(identity);app.service.store.recordHistory(identity,{revision:1,snapshotId:snapshot.id},snapshot.base,head,[{sha:head,owner:'P2',origin:'owned',sourceSha:null}]);
+ await page.goto(app.url);await page.getByRole('button',{name:/P2 Document retry behavior/}).click();await page.getByRole('button',{name:'Resolve ambiguous changes',exact:true}).click();await expect(page.getByRole('heading',{name:'Ambiguous',exact:true})).toBeVisible();
+});

test/review.test.ts

@@ -39,4 +39,6 @@ it('refuses no-change confirmation while the item still owns ambiguous changes',
  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);
+ expect(view.items[1]!.count).toBeGreaterThan(0);
+ expect(()=>service.act({action:'approve',item:'P2',token:view.token})).toThrow(/ambiguous/i);
 });

web/public/app.js

@@ -159,10 +159,10 @@ function render() {
     ? `<p>${esc(item.intent)}</p>${item.reasons.map((reason) => `<p class="warn">! Stale: ${esc(reason)}</p>`).join("")}`
     : "";
   $("approve").hidden = !item;
-  $("approve").textContent = item?.count
-    ? `Approve ${item.id}`
-    : item?.ambiguousCount
-      ? "Resolve ambiguous changes"
+  $("approve").textContent = item?.ambiguousCount
+    ? "Resolve ambiguous changes"
+    : item?.count
+      ? `Approve ${item.id}`
       : "Confirm no change needed";
   $("approve").disabled = item?.state === "approved";
   $("view-toggle").innerHTML =
@@ -324,7 +324,7 @@ function showDialog(html) {
 }
 $("approve").onclick = () => {
   const item = data?.items.find((item) => item.id === selected);
-  if (item?.count === 0 && item.ambiguousCount > 0) {
+  if (item?.ambiguousCount > 0) {
     select("Ambiguous");
     return;
   }