fix(review): a heavy file in a Step-3A diff must not demand invariant agents

wenshao · wenshao · commit 7c499d19396f · 2026-07-15T11:33:19.000+08:00
From a human review of QwenLM#6892 (doudouOUC). `heavy` is decided independently of topology (lib/heavy.ts): a ~300-line source file with ~120 changed lines clears the rewrite-ratio branch while srcDiffLines stays under 500 — a Step 3A review. The invariant-agent loop in requiredAgents ran in both topologies, so it added invariant-a/b/c to the roster of a 3A review that never launches them; check-coverage then reported them as missingRoles and exit-3'd, and compose-review capped the verdict — an otherwise-complete small PR, falsely blocked. Gate the loop on isTerritoryFanOut. Step 3A's dimension agents each walk the whole diff, so one already sees both ends of a rewritten file; invariant agents are a 3B mechanism for when the diff is carved into territories and no single agent holds the whole file. roster.test.ts now pins the 3A-heavy case. Also, same review: merge() in coverage.ts copied its first tuple and pushes copies, so it no longer mutates a tuple owned by rec.diffReads (harmless today, pure now).
diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts
@@ -215,7 +215,12 @@ function pointedAt(prompt: string, plan: Plan): Array<[number, number]> {
 function merge(ranges: Array<[number, number]>): Array<[number, number]> {
   if (ranges.length < 2) return ranges;
   const sorted = [...ranges].sort((a, b) => a[0] - b[0] || a[1] - b[1]);
-  const out: Array<[number, number]> = [sorted[0]];
+  // Start with a COPY of the first tuple, and push copies. `sorted` shares its
+  // element references with the caller's array — which includes `rec.diffReads` —
+  // so writing `last[1] = …` below would mutate a tuple the record owns. Harmless
+  // today (the record is not read again after this), but a pure function here is
+  // one fewer latent foot-gun for the next caller.
+  const out: Array<[number, number]> = [[...sorted[0]]];
   for (const [s, e] of sorted.slice(1)) {
     const last = out[out.length - 1];
     // `s <= last[1] + 1` — abutting counts. Lines 1-200 then 201-400 is one walk of
diff --git a/packages/cli/src/commands/review/lib/roster.test.ts b/packages/cli/src/commands/review/lib/roster.test.ts
@@ -174,3 +174,32 @@ describe('requiredAgents — Step 3B', () => {
     expect(keys(heavy)).not.toContain('invariant-a--src/small.ts');
   });
 });
+
+describe('a heavy file in a Step-3A-sized diff', () => {
+  it('does NOT demand invariant agents — Step 3A never launches them', () => {
+    // `heavy` is decided independently of topology: a 300-line source file with
+    // ~120 changed lines clears the rewrite-ratio branch while `srcDiffLines` stays
+    // under 500 — a Step 3A review. Requiring invariant agents there demanded agents
+    // the review never launches, and `check-coverage` then exit-3'd an otherwise
+    // complete small PR. (A real finding from a human review of this change.)
+    const smallButHeavy = {
+      ...PR, // srcDiffLines 200, diffLines 300 → Step 3A
+      files: [
+        {
+          path: 'src/rewritten.ts',
+          kind: 'source',
+          removedLines: 40,
+          heavy: true,
+        },
+      ],
+    };
+    expect(isTerritoryFanOut(smallButHeavy)).toBe(false);
+    const k = keys(smallButHeavy);
+    expect(k).not.toContain('invariant-a--src/rewritten.ts');
+    expect(k).not.toContain('invariant-b--src/rewritten.ts');
+    expect(k).not.toContain('invariant-c--src/rewritten.ts');
+    // It is still a normal 3A review: the dimension agents each walk the whole diff,
+    // and one that walks the whole diff already sees both ends of the file.
+    expect(k).toEqual(expect.arrayContaining(['1a', '2', '6a']));
+  });
+});
diff --git a/packages/cli/src/commands/review/lib/roster.ts b/packages/cli/src/commands/review/lib/roster.ts
@@ -171,10 +171,21 @@ export function requiredAgents(plan: RosterPlan): RequiredAgent[] {
   // invariant sit two thousand lines apart. Three agents, one checklist slice each
   // — measured, one agent holding all eight checks found one of five defects and
   // the same model split three ways found all five.
-  for (const file of heavyFiles(plan)) {
-    add('invariant-a', file);
-    add('invariant-b', file);
-    add('invariant-c', file);
+  //
+  // **Step 3B only.** `heavy` is decided independently of topology (`lib/heavy.ts`):
+  // a 300-line source file with ~120 changed lines clears the rewrite-ratio branch
+  // while `srcDiffLines` stays under 500 — a Step 3A review. Step 3A launches no
+  // invariant agents (its dimension agents each walk the whole diff, so they
+  // already see both ends of a file), and the skill's 3A section never mentions
+  // them. Requiring them there demanded agents the review was never meant to launch,
+  // and `check-coverage` then exit-3'd an otherwise-complete small PR. Gate the loop
+  // on the topology that actually runs them.
+  if (isTerritoryFanOut(plan)) {
+    for (const file of heavyFiles(plan)) {
+      add('invariant-a', file);
+      add('invariant-b', file);
+      add('invariant-c', file);
+    }
   }
 
   return out;
