feat(review): carry the fix's premises beside its claim — add fixConstraint

packages/cli/src/commands/review/agent-prompt.test.ts

@@ -2775,6 +2775,38 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
     );
   });
 
+  it('welds the fix-constraint format into the launched finder briefs', () => {
+    // The premise half of #10153, pinned where it reaches the agents. Four
+    // clauses have to survive together: the format has to ASK for the fact,
+    // the omission has to stay an omission (a finder copying the Fix witness
+    // habit would write `N/A` and lengthen every comment), the evidence bar
+    // has to stay at witness grade (a wrong constraint is misdirection the
+    // fixer follows, so prose with no source is forbidden outright), and the
+    // field must not become a bar on reporting.
+    const brief = buildRoleBrief(PLAN, '1a');
+    expect(brief).toContain(
+      '**Fix constraint:** <an existing fact the fix must not violate, with its source',
+    );
+    expect(brief).toContain(
+      'OMIT THIS LINE when you observed none; never write "N/A"',
+    );
+    expect(brief).toContain(
+      'quote the constant or give the `file:line`, or omit the line',
+    );
+    expect(brief).toContain(
+      'is forbidden in this field exactly as "this looks risky" is forbidden in the failure scenario',
+    );
+    expect(brief).toContain(
+      'Like Fix witness, this field never gates reporting',
+    );
+    // And the two fields stay two: the constraint paragraph opens by parting
+    // claim from premise, so a rewrite that folds one into the other — "put
+    // the limit in the Fix witness" — reds here rather than shipping green.
+    expect(brief).toContain(
+      "Fix witness pins the fix's *claim*: does it do what it says. Nothing pins the fix's *premises*",
+    );
+  });
+
   it('keeps the language-agnostic falsy-zero shape in the Agent 1a brief', () => {
     // The #9788 split moved the language-pitfall CHECKLIST and wrapper/proxy
     // routing out of 1a, but the falsy-zero shape is general correctness, not

packages/cli/src/commands/review/agent-prompt.ts

@@ -381,6 +381,7 @@ const FINDING_FORMAT = `Format each finding using this structure:
 - **Failure scenario:** <the concrete trigger and the concrete wrong outcome: what input, state, timing, or config makes this code misbehave, and what incorrect output / crash / leak / exposure results>
 - **Suggested fix:** <concrete code suggestion when possible, or "N/A">
 - **Fix witness:** <the test that must go RED if that fix is removed — the test file and the behaviour it pins — or "N/A" when the fix adds no guard, branch or behaviour a test can pin>
+- **Fix constraint:** <an existing fact the fix must not violate, with its source — the quoted constant or the file:line — and OMIT THIS LINE when you observed none; never write "N/A">
 - **Severity:** Critical | Suggestion | Nice to have
 - **Confidence:** high | low
 
@@ -395,7 +396,9 @@ const FINDING_FORMAT = `Format each finding using this structure:
 
 **The failure scenario is the finding's evidence, and it gates reporting.** For a quality finding, state the concrete cost instead of a crash — what is duplicated, wasted, or made harder to change — or quote the rule it violates. A **Suggestion** or **Nice to have** whose failure scenario you cannot fill in concretely **is not a finding: do not report it.** A suspected **Critical** whose trigger you cannot pin down IS still reported, at \`Confidence: low\`, with the scenario naming the mechanism and what remains uncertain — a later verification stage rules on it. "This looks risky", with no nameable trigger and no nameable cost, is how a hallucinated finding reaches a pull request.
 
-**A fix that adds a guard owes a test that fails without it — say so in the finding.** The fix round is this loop's largest single source of its own next round: measured across six multi-round pull requests, roughly a third of every post-first-round finding was introduced by the fix immediately before it, and the dominant shape was a guard or branch added with no test of its own. The suite re-runs only the tests that exist, so an unwitnessed guard passes every gate and its hole comes back as next round's finding. So when your **Suggested fix** adds or changes a guard, a branch, or a behaviour, fill **Fix witness** with the test that must go red without it — the file and what it asserts — and, where you can, the mutation that proves it: remove the guard, run that test, watch it fail. Write \`N/A\` when there is genuinely nothing to pin — a rename, a comment, a docs line, a type-only change, a fix whose whole content is deleting code. **This field never gates reporting**: a finding whose fix you cannot pin is still filed, with \`N/A\`. It is an acceptance criterion for the author, not a bar for you.`;
+**A fix that adds a guard owes a test that fails without it — say so in the finding.** The fix round is this loop's largest single source of its own next round: measured across six multi-round pull requests, roughly a third of every post-first-round finding was introduced by the fix immediately before it, and the dominant shape was a guard or branch added with no test of its own. The suite re-runs only the tests that exist, so an unwitnessed guard passes every gate and its hole comes back as next round's finding. So when your **Suggested fix** adds or changes a guard, a branch, or a behaviour, fill **Fix witness** with the test that must go red without it — the file and what it asserts — and, where you can, the mutation that proves it: remove the guard, run that test, watch it fail. Write \`N/A\` when there is genuinely nothing to pin — a rename, a comment, a docs line, a type-only change, a fix whose whole content is deleting code. **This field never gates reporting**: a finding whose fix you cannot pin is still filed, with \`N/A\`. It is an acceptance criterion for the author, not a bar for you.
+
+**A fix that introduces a premise owes the fact it must respect — with its source, or not at all.** Fix witness pins the fix's *claim*: does it do what it says. Nothing pins the fix's *premises* — the assumptions a fix newly introduces — and those pass a witnessed test cleanly. Two Criticals on one merged fix each had the test that reds without the guard, and were still wrong: a hand-picked \`hops < 16\` lineage cap sat below the user-configurable \`MAX_SUBAGENT_DEPTH_LIMIT = 100\`, so deep lineages silently got back the hang the fix was for; and parking several runtimes' approvals on one registry entry broke a \`callId\` uniqueness that dedup and resolve relied on elsewhere, so a user's answer reached the wrong agent. So when your **Suggested fix** introduces a bound, shares a resource, or changes a shape, and you have already seen the existing fact it must not violate — a configured limit any new bound must stay within, a second site that reads the same field, a uniqueness the resource's key currently guarantees — fill **Fix constraint** with that fact and where it lives. The bar is the **Witness** bar, not the Fix witness bar: **quote the constant or give the \`file:line\`, or omit the line.** The costs are asymmetric — a wrong Fix witness is one test not written; a wrong constraint is confidently-stated misdirection the fixer will follow. "Be careful about concurrency", "keep this consistent with the other path" — a caution with no quoted fact and no location — is forbidden in this field exactly as "this looks risky" is forbidden in the failure scenario. And when you observed nothing, **omit the line entirely** — not \`N/A\`, not "none observed": an absent constraint says nothing and would lengthen every finding. Like Fix witness, this field never gates reporting.`;
 
 /**
  * What not to report.

packages/cli/src/commands/review/findings.test.ts

@@ -2105,4 +2105,62 @@ describe('validateFindings — the canonical artifact round-trips', () => {
     ).toBe('N/A');
     expect(validateFindings([{ ...base }])[0].fixWitness).toBeUndefined();
   });
+
+  it('keeps fixConstraint, and drops the N/A its sibling field allows', () => {
+    // The fact the fix must not violate — the premise half of #10153, beside
+    // the claim half `fixWitness` carries. It round-trips like every sibling
+    // so Step 7's comment body reads it from data rather than re-deriving a
+    // constant or a file:line the finder already quoted.
+    const constraint =
+      'any bound here must be <= MAX_SUBAGENT_DEPTH_LIMIT = 100 ' +
+      '(packages/core/src/config/config.ts:1533)';
+    const [f] = validateFindings([
+      {
+        ...base,
+        fixWitness: 'N/A',
+        fixConstraint: constraint,
+      },
+    ]);
+    expect(f.fixConstraint).toBe(constraint);
+    expect(f.fixWitness).toBe('N/A');
+    expect(
+      validateFindings([{ ...base, fix_constraint: constraint }])[0]
+        .fixConstraint,
+    ).toBe(constraint);
+    // Absence stays absence: the field has no `N/A` form, because an empty
+    // constraint carries no information and would lengthen every posted
+    // comment (#9177). A finder that copies the fixWitness habit and writes
+    // the placeholder anyway must not hand the poster a "constraint" — the
+    // literal is normalised to absence so presence alone is the signal.
+    expect(validateFindings([{ ...base }])[0].fixConstraint).toBeUndefined();
+    for (const placeholder of [
+      'N/A',
+      'n/a',
+      'NA',
+      'none',
+      'None.',
+      ' N/A ',
+      // The omission literals the finding format and the posting rule name
+      // — the finder told to omit the line is the one most likely to write
+      // one, and carried through it would hand Step 7 a "constraint" that
+      // names no constant and no file:line.
+      'none observed',
+      'None observed',
+      'None observed.',
+      'no constraints observed',
+    ]) {
+      expect(
+        validateFindings([{ ...base, fixConstraint: placeholder }])[0]
+          .fixConstraint,
+      ).toBeUndefined();
+    }
+    // And the drop is narrow: a real constraint that merely CONTAINS one of
+    // the words survives — the bar for the field is a quoted constant or a
+    // file:line, and neither collapses to a placeholder.
+    expect(
+      validateFindings([
+        { ...base, fixConstraint: 'none of the callers pass 0 (src/a.ts:12)' },
+      ])[0].fixConstraint,
+    ).toBe('none of the callers pass 0 (src/a.ts:12)');
+  });
 });

packages/cli/src/commands/review/findings.ts

@@ -128,6 +128,25 @@ export interface Finding {
    * recorded string instead of transcribing it twice more.
    */
   fixWitness?: string;
+  /**
+   * An existing fact the fix must not violate, with its source — the quoted
+   * constant or a `file:line`: a configured limit any new bound must stay
+   * within, a second site that reads the field a shape change touches, a
+   * uniqueness a newly shared resource's key currently guarantees.
+   *
+   * `fixWitness` pins the fix's CLAIM — does it do what it says. This pins
+   * the fix's PREMISES — do the assumptions it newly introduces hold. Those
+   * are a different defect class and pass a witnessed test cleanly: two
+   * Criticals on one merged fix each had the test that reds without the
+   * guard, and were still wrong — a hand-picked lineage cap below the
+   * user-configurable `MAX_SUBAGENT_DEPTH_LIMIT`, and a shared registry that
+   * broke a `callId` uniqueness relied on elsewhere (#10153).
+   *
+   * Absence is the whole signal: there is no `N/A` form, because an empty
+   * constraint carries no information and would lengthen every comment, so
+   * the validator drops a literal `N/A` rather than carrying it.
+   */
+  fixConstraint?: string;
   /** Free-form kebab-case tag (`correctness`, `security`, `test-coverage`, …). */
   category?: string;
   /** Every location, in report order. A standalone finding has exactly one. */
@@ -183,6 +202,18 @@ function asString(o: Record<string, unknown>, key: string): string | undefined {
   return typeof v === 'string' && v.trim() !== '' ? v : undefined;
 }
 
+/** `N/A`, `n/a`, `NA`, `none`, `none observed`, `no constraints observed` —
+ * the placeholders a field with no `N/A` form must not carry; the two long
+ * ones are the exact omission literals the pipeline itself names, so the
+ * finder told to omit the line is the one the drop catches. Kept narrow on
+ * purpose: a real constraint quotes a constant or a `file:line`, and none
+ * of those collapse to one of these words. */
+function isNotApplicable(v: string): boolean {
+  return /^(none observed|no constraints observed|n\/?a|none)\.?$/i.test(
+    v.trim(),
+  );
+}
+
 /** A non-empty array of non-empty strings, or undefined; anything else fails
  * with the finding's index so a typo'd shape is named rather than dropped. */
 function stringArray(
@@ -422,6 +453,19 @@ export function validateFindings(raw: unknown): Finding[] {
     // a later round comparing what it asked for against what landed.
     const fixWitness = asString(o, 'fixWitness') ?? asString(o, 'fix_witness');
 
+    // And `fixConstraint` beside it — the existing fact the fix must not
+    // violate. Unlike `fixWitness` it has no `N/A` form: absence is the whole
+    // signal, and a finder that copies the sibling field's habit and writes
+    // `N/A` here would otherwise hand Step 7 a "constraint" to post. The
+    // literal is normalised to absence so the comment body can key on
+    // presence alone.
+    const fixConstraintRaw =
+      asString(o, 'fixConstraint') ?? asString(o, 'fix_constraint');
+    const fixConstraint =
+      fixConstraintRaw && !isNotApplicable(fixConstraintRaw)
+        ? fixConstraintRaw
+        : undefined;
+
     return {
       id,
       severity,
@@ -434,6 +478,7 @@ export function validateFindings(raw: unknown): Finding[] {
       failureScenario,
       ...(witness ? { witness } : {}),
       ...(fixWitness ? { fixWitness } : {}),
+      ...(fixConstraint ? { fixConstraint } : {}),
       ...(asString(o, 'suggestedFix') || asString(o, 'suggested_fix')
         ? {
             suggestedFix: (asString(o, 'suggestedFix') ??

packages/core/src/skills/bundled/review/DESIGN.md

@@ -1168,3 +1168,9 @@ Provenance analysis of six multi-round takeover pull requests attributed each po
 The fix landed first on the loop side (#9578: the autofix agent must mutation-probe each new guard before it commits), and that half reaches exactly one fixer. The reviewer-side half has to reach all of them, because most pull requests are not fixed by a bot the review can configure: whoever fixes a finding — contributor, maintainer, or agent — reads only the comment. So the acceptance criterion moves into the finding itself (`fixWitness`) and into the posted comment: name the test that must go red if the fix is removed. A fixer told the criterion closes the hole in the round they are already working; a fixer not told it ships the guard and meets the criterion as a finding next round.
 
 Two properties keep the rule from costing what it saves. It **never gates reporting** — a finding whose fix cannot be pinned is filed with `N/A`, because a bar on reporting would trade rounds for missed defects, and the reviewer's own evidence rule (`witness`) is the one that governs what confirms. And it is one sentence of ordinary prose at the end of the body, not a section: the comment budget is the scarce resource this review already trims the deferral list to protect.
+
+### The fix whose premises were wrong (#10153)
+
+`fixWitness` pins the fix's **claim** — does the fix do what it says. Nothing pinned the fix's **premises** — the assumptions a fix newly introduces — and those are a different defect class that passes a `fixWitness`-grade check cleanly. Two Criticals on one merged nested-approval fix (#9793) were both of this class; both had a test that went red when the fix was removed, and both were still wrong. A hand-picked `hops < 16` lineage-walk cap sat below the user-configurable `MAX_SUBAGENT_DEPTH_LIMIT = 100`, so deep lineages silently reintroduced the very hang the fix was for — the test proved the bridge worked, not that the bound was right. And parking approvals from several runtimes onto one registry entry broke a `callId` uniqueness (`call_qwen_1` restarts per conversation) that dedup and resolve relied on elsewhere, so a user's answer was routed to the wrong agent — the test proved approvals parked, not that the key was still unique. The shapes are **a new constant that must respect an existing configured limit** and **a newly shared resource invalidating a uniqueness assumption held elsewhere**, and the agent that found each defect had already run the greps that would have named the fact, so carrying it costs close to nothing.
+
+So the finding gains a second optional field beside `fixWitness`, carried the same way (`fixConstraint`): an existing fact the fix must not violate, with its source. Two properties differ from its sibling, and both are deliberate. It is **omitted when nothing was observed, never `N/A`** — `fixWitness` means something as `N/A` ("nothing to pin"), an absent constraint carries no information, and comment volume is a measured concern (#9177); the command drops the literal so the comment body can key on presence alone. And its evidence bar is the **`witness` bar, not the `fixWitness` bar** — quote the constant or give the `file:line`, or omit — because the failure costs are asymmetric: a wrong `fixWitness` is one test not written, a wrong constraint is confidently-stated misdirection the fixer will follow, so "be careful about concurrency" is forbidden in the field exactly as "this looks risky" is forbidden in the failure scenario. The field reaches the fixer who reads the posted comment, which is the loop with no other brake; the `--fix` path's own audit of a fix's premises is #10154. A fix that invents a new mechanism rather than following the suggested one is outside both.