feat(gate-findings): frame findings as hypotheses at evaluator-fed fix stages

docs/architecture/engine-internals.md

@@ -709,16 +709,48 @@ ctx.unresolved) — or (b) for pr-review (issue #162)
 and quality (issue #163) alike, the reviewer(s)
 requested changes while naming no structured
 findings to fix, so a fix stage would have had
-nothing to act on. For pr-review, (a) and (b) land
-on the same needs_human outcome; only
-ctx.metrics.findings_empty_exits distinguishes
-them. quality has a single reviewer, not a pair,
-so its (b) is one changes_requested verdict with
-issues: [] — that branch sets runQualityLoop's own
-approved = true (see below) even though it still
-tallies carried-unresolved here: "clean" for the
-loop's control flow and "clean" for this gate
-tally answer different questions.
+nothing to act on — or (c), added by issue #167, a
+fix agent shown this gate's findings rebutted every
+one of them (FIX_SCHEMA.rebutted, normalized by
+normalizeRebuttals()) and applied no fix
+(fixes_applied and files_changed both empty):
+retypeGateDisposition() retypes that iteration's
+already-booked disposition to carried-unresolved
+after the fact, at quality and pr-review — the two
+of these gates whose disposition this tally actually
+records. test-quality has the identical
+rebuttal-only exit (same predicate, same
+FIX_SCHEMA.rebutted field) but books no
+gate_findings entry in the first place (it isn't one
+of the four gates this tally covers), so route (c)
+at test-quality is visible only via
+ctx.metrics.rebuttal_only_rounds and the
+contested-findings ledger, never in this tally. For
+pr-review, (a) and (b) land on the same needs_human
+outcome; only ctx.metrics.findings_empty_exits
+distinguishes them from each other. (c) at pr-review
+does NOT land on needs_human the first time it
+fires — a single rebuttal-only pr-fix round
+continues the review loop into another iteration
+instead; only a second rebuttal-only round in the
+same reviewAndMerge() call joins (a) and (b) on
+needs_human. ctx.metrics.rebuttal_only_rounds is
+what distinguishes (c) from (a)/(b): a
+carried-unresolved pr-review tally with
+rebuttal_only_rounds > 0 for that iteration means a
+fixer disputed the findings, not that the cap ran out
+or nothing was named. quality has a single reviewer,
+not a pair, so its (b) is one changes_requested
+verdict with issues: [] — that branch sets
+runQualityLoop's own approved = true (see below) even
+though it still tallies carried-unresolved here:
+"clean" for the loop's control flow and "clean" for
+this gate tally answer different questions. Route (c)
+at quality also sets its own loop-exit flag
+(`rebutted`, kept separate from `degraded`) rather
+than approved = true — a rebuttal-only round is a
+dispute, not a clean review, so it does not get the
+same "clean for control flow" treatment (b) does.
 re-litigated        - neither of the above: the loop revises and
 re-contests, so these findings get judged again
 next iteration (a fix stage for pr-review, a
@@ -777,6 +809,17 @@ findings.length > 0 - render the work list using the existing pr-fix line
 shape (:4259) with the id prefixed, then the prose
 `comments` below under a context-only heading — the
 fix agent's job list is the findings, not the prose.
+(issue #167: a fix agent may rebut, not fix, any
+finding rendered here — record the disagreement in
+FIX_SCHEMA.rebutted with the concrete evidence that
+disproves it, rather than changing code to satisfy a
+finding it judged wrong. Only a finding that reaches
+this branch, carrying the bracketed id this renderer
+prefixes onto each line, can be rebutted; a finding
+that shows up only in the `comments` prose below, or
+only via the findings === null fallback above, has no
+id to rebut against and must be fixed outright or
+addressed in the fixer's own summary instead.)
 findings.length === 0 - a reviewer that validated `issues: []` alongside
 changes_requested (reached only by the pr-review gate
 in task 2, where one reviewer has zero findings and

docs/architecture/gate-hygiene.md

@@ -518,6 +518,234 @@ same issue, run twice, can report a different quality contribution for
 reasons that have nothing to do with how hard it fought. Compare quality
 friction only within reports generated by the same version of this engine.
 
+## Rebuttal: a finding is a hypothesis, not a command (issue #167)
+
+Before this issue, a fix agent shown a reviewer's findings had exactly two
+moves: comply, or return `status: 'error'` and degrade the whole gate. There
+was no schema field for "I checked this and the reviewer is wrong" — a
+fixer that disagreed either silently rewrote code to satisfy a finding it
+believed was mistaken, or burned a `status: 'error'` disproportionate to the
+actual disagreement. This section covers the fix: a `rebutted` field on
+`FIX_SCHEMA`, a shared prompt framing that tells a fixer a finding is a
+hypothesis to verify rather than an instruction to obey, and the
+deterministic machinery that keeps a rebut-everything round from quietly
+passing as a resolved gate.
+
+### What a rebuttal is, and the evidence it must carry
+
+`FIX_SCHEMA.rebutted` is `[{finding_id, evidence}]`. A fixer that judged a
+rendered finding wrong records it here instead of touching code for it.
+`evidence` is not "I disagree" — the shared prompt framing asks for "the
+concrete check you ran that disproves it (a command, a line reference, a
+test result — not just disagreement)." That concrete-evidence bar is
+prompt-only: it cannot be checked mechanically, since the engine has no way
+to judge whether a string is actually a command, a line reference, or a
+test result versus prose that merely looks like one. What `normalizeRebuttals`
+enforces mechanically is narrower — only non-blankness: an entry with a
+blank `evidence` (or a blank `finding_id`) is silently dropped, so literally
+empty disagreement can never survive normalization, but a non-blank
+`evidence: 'I disagree'` passes every mechanical check and becomes a
+rebuttal the engine acts on. A rebuttal can also only target a finding the fixer was
+actually shown with a bracketed id (e.g. `[code-i1-2]`) — the same
+`normalizeFindings`-assigned id `findingsBlock()` prefixes onto every
+rendered finding line. Anything a fixer sees only as prose (`comments`,
+`summary`) has no id to rebut against; it must be fixed outright or
+addressed in the fixer's own `summary`. This is enforced twice, once as an
+instruction (the last clause of the shared framing below) and once
+mechanically (`normalizeRebuttals` matches `finding_id` against exactly the
+finding set rendered to that fixer, and drops anything that doesn't match —
+see below).
+
+### Three evaluator-fed gates, not five
+
+The shared framing lives in one constant, `FINDING_HYPOTHESIS_ASK`, wired
+into exactly three fix prompts: quality-fix, test-quality-fix, and pr-fix.
+These three share a trait the other two fix stages don't: their findings
+come from a reviewer's *judgment* of the diff — a code reviewer, a test
+validator, a spec/code reviewer pair — which can be wrong the same way any
+review can be wrong. That is genuinely a hypothesis to verify.
+
+The two oracle-fed fix stages, test-fix and browser-fix, are deliberately
+untouched. Their "findings" are not a reviewer's opinion; they are the
+direct output of running something — a failing test, a broken page
+interaction — which is ground truth, not a judgment call. Both already
+carry a correct anti-rebuttal guard that predates this issue: test-fix
+reads "Fix the real defect — do NOT delete or weaken assertions just to
+make the failure disappear," and browser-fix reads "Fix the real defect —
+do NOT hide the symptom (e.g. removing the interaction that fails)." Wiring
+`FINDING_HYPOTHESIS_ASK`'s "verify before acting, rebut if wrong" framing
+into either of those prompts would tell the same fixer, in the same
+response, to treat a failing assertion or a broken click as a hypothesis it
+might disprove — directly inverting a guard that exists precisely because a
+fixer's own doubt about a failing test is not evidence the test is wrong.
+This is why the framing is scoped to exactly three gates rather than all
+five fix stages that share `FIX_SCHEMA`.
+
+### `FIX_SCHEMA.rebutted` is schema-wide; only three sites read it
+
+`FIX_SCHEMA` is one shared schema feeding every `agent()` call that returns
+a fix — six call sites in total: quality-fix, browser-fix, test-fix,
+test-quality-fix, the per-task review fix, and pr-fix. Adding `rebutted` to
+`FIX_SCHEMA` makes it schema-valid at all six; nothing in the schema itself
+scopes it to the three evaluator-fed gates. The scoping is enforced by
+control flow instead: `normalizeRebuttals(fix.rebutted, findings)` — the
+sole consumer of the field — is only ever called at quality-fix,
+test-quality-fix, and pr-fix. The other three fix stages never read
+`fix.rebutted` at all. A model at test-fix, browser-fix, or the task-review
+fix that populates `rebutted` anyway (nothing in the schema stops it) gets
+no framing telling it the field exists, and the engine silently ignores
+whatever it returned — those three fixers behave identically to before this
+issue, byte for byte. This mirrors the precedent `REVIEW_SCHEMA.issues`
+already set: `rebutted` stays out of `FIX_SCHEMA.required`, so a fixer that
+never disagrees — the entire population before this issue, and every
+oracle-fed or task-review fixer after it — omits the key and produces an
+unchanged response.
+
+### `normalizeRebuttals`: every drop fails toward today's behavior
+
+`normalizeRebuttals(raw, findings)` turns the raw `rebutted` array into the
+validated list the three evaluator-fed gates act on. Every failure mode
+drops the offending entry rather than trusting it: a non-array `raw`
+(including the omitted-field case, the common one) returns `[]`; an entry
+with a blank `finding_id` or blank `evidence` is dropped; an entry whose
+`finding_id` doesn't match any id in `findings` — the exact,
+possibly-`null` array actually rendered to that fixer (the union of
+`specFindings`/`codeFindings` at pr-fix, since one fixer sees both
+reviewers' blocks in one prompt) — is also dropped. There is no failure
+path that trusts an unverifiable or spoofed rebuttal; a dropped entry is
+simply absent from the list the gate acts on, the same as if the fixer had
+never mentioned it, so a malformed or fabricated rebuttal degrades to
+silence rather than to something the engine might mistakenly honor.
+
+### The three per-gate exits, and why only `pr-review` can block a merge
+
+All three evaluator-fed gates share one predicate, evaluated after a fix
+stage returns: a round is rebuttal-only when it rebutted at least one
+finding and applied none (`normalizeRebuttals(...).length > 0 &&
+fixes_applied.length === 0 && files_changed.length === 0`). What each gate
+does with that fact differs, because the three gates don't carry the same
+stakes:
+
+- **quality-fix** cannot block a merge — `runQualityLoop` only gates one
+  task's implementation or one PR-fix round's cleanup. A rebuttal-only round
+  there sets a third loop-exit flag (`rebutted`, kept separate from
+  `degraded` so it doesn't inflate `quality_degrades` or trip the rolling
+  degrade window) and stops the loop immediately rather than spending its
+  remaining iterations re-litigating a dispute only a reviewer or a human
+  can adjudicate. `retypeGateDisposition` moves that iteration's
+  already-booked disposition to `carried-unresolved` after the fact.
+- **test-quality-fix** cannot block a merge either — `runTestLoop` only
+  ever returns `{ ok: true }` or `{ ok: false }` for a dead agent; a
+  rebuttal-only round there returns `{ ok: true }`, the same clean-exit
+  shape the loop uses elsewhere, so nothing routes through a path that
+  could fail the run. Unlike quality, this loop books no `gate_findings`
+  entry at all (`test-quality` isn't one of the four gates
+  `recordGateOutcome` tracks), so there is no disposition to retype here —
+  the round is visible only through `ctx.contested` and the metrics counter
+  below.
+- **pr-fix** is the one gate whose clean verdict (`prReviewClean`, both
+  reviewers approved) is the *only* condition that may set
+  `reviewAndMerge`'s `approved = true` and let the PR proceed to
+  `gh pr merge --squash`. Treating a rebuttal-only round there as a clean
+  exit the way quality and test-quality can would let a fixer's own,
+  unadjudicated disagreement stand in for a reviewer's approval — exactly
+  the judgment call this codebase already reserves for a human (see the
+  empty-findings exit above, which reasons the same way about
+  `changes_requested`). So pr-fix does not exit on a rebuttal-only round;
+  it `continue`s the review loop into another iteration instead, with the
+  disputed findings now carried in `contestedBlock` for the next reviewer
+  to adjudicate. `retypeGateDisposition` fires here too, moving the
+  iteration's booked disposition to `carried-unresolved` — on every
+  rebuttal-only round, not just the first, including the halting one below.
+
+A local counter, `rebuttalRoundsUsed`, permits exactly ONE rebuttal-only
+pr-fix round per `reviewAndMerge()` call before this gate stops giving a
+disputing fixer another iteration: a second one in the same call sets
+`haltReason` and breaks into the existing `needs_human` path, the same
+shape as the `bothNothingToFix`/`capReached` breaks above it, rather than
+`continue`-ing indefinitely. There's no id-equality check guarding that
+counter because none is needed: `REVIEW_SCHEMA` ids are
+`source + '-' + (i + 1)` with the iteration baked into `source`, so a
+second round's ids are disjoint from the first round's by construction —
+`rebuttalRoundsUsed` alone is sufficient. Unlike the quality/test loops,
+pr-fix does not push a second `pushDecision` on a rebuttal-only round
+either; the `pushDecision` already fired for every non-error fix, right
+after the stage returns, covers it.
+
+### `contestedBlock` versus `settledBlock`: a deliberate contract inversion
+
+`contestedBlock(ctx)` renders `ctx.contested` — the list a rebuttal-only
+round pushes onto — back to the *next* reviewer at all three review prompts
+that already render `settledBlock`. It is shaped like `settledBlock` on
+purpose (same defensive read, same last-6 window, same `''`-when-empty
+render) but it carries the opposite trust contract, and that inversion is
+deliberate, not an oversight to reconcile:
+
+- `settledBlock` renders a decision an earlier gate already *adjudicated* —
+  its instruction is "don't re-open this without new evidence; re-litigating
+  a settled decision without new evidence is itself a process failure."
+- `contestedBlock` renders a rebuttal nobody has adjudicated yet — its
+  instruction is the opposite: verify the fixer's evidence yourself, drop
+  the finding if it holds, re-raise it as a finding this iteration if it
+  doesn't, and never let it sit contested indefinitely with neither
+  outcome. It also carries an explicit override: whichever iteration-2+
+  instruction the same prompt carries elsewhere — code review's "don't
+  re-flag issues already addressed or accepted," spec review's "stay
+  consistent with your own prior reviews" — does NOT apply to anything in
+  this block, because a contested finding is neither already addressed nor
+  previously ruled on.
+
+Reusing `settleDecision()`/`settledBlock()` for a rebuttal would tell the
+next reviewer to treat an unadjudicated dispute as already-settled, which
+is precisely the failure mode this whole framing exists to avoid: a
+fixer's own say-so standing in for a real verdict. `contestedBlock` never
+calls `settleDecision()` — and, as of this issue, nothing else removes an
+entry from `ctx.contested` either. There is no code path that closes a
+contested entry: a reviewer's verify-then-drop-or-re-raise ruling is
+advisory prose in that review's own response, not a ledger mutation, so a
+contested entry persists for the life of the issue and keeps re-rendering
+to every later review of that gate — including a quality-review at task N
+re-rendering an entry a task-1 rebuttal-only round contested — no matter
+how many iterations follow the round that contested it, and even after a
+later reviewer has actually ruled on it in prose. Closing the ledger entry
+once a later review rules on it is a known gap left for a follow-up issue,
+not a design decision.
+
+### `rebuttal_only_rounds`: at `pr-review`, the first increment is a continuation, not an exit
+
+`ctx.metrics.rebuttal_only_rounds` increments at all three rebuttal-only
+exits above — quality, test-quality, and pr-review — the same run-wide,
+not-per-gate shape `findings_empty_exits` already uses. At quality and
+test-quality, every increment corresponds 1:1 with an exit from that loop:
+the loop stops, the round is done. At pr-review, that is NOT true for the
+first increment in a given `reviewAndMerge()` call: a single rebuttal-only
+pr-fix round `continue`s into another review iteration rather than halting,
+so `rebuttal_only_rounds` going from 0 to 1 on an issue can mean nothing
+more than "the merge gate looped once more" — the same issue can still go
+on to reach `prReviewClean` cleanly on a later iteration. A *second*
+rebuttal-only round in the same call is a halt, not a continuation, joining
+the cap-reached and empty-findings breaks on the `needs_human` path — but
+the counter does not increment on that second round: it tracks
+continuations, not rounds, and the halt is already carried by `haltReason`
+and the `needs_human` status, so counting it again would double-book a
+signal the status code already carries. Since `pr-fix` runs at most at
+iterations 1 and 2 of `MAX_PR_REVIEW_ITERATIONS = 3`, this still bounds an
+issue to at most two rebuttal-only pr-fix rounds regardless of what the
+counter reads. Reading `rebuttal_only_rounds` at pr-review without also
+checking whether the issue ultimately reached `approved` will misread a
+continuation as a stall.
+
+One gap worth naming plainly rather than leaving implicit: `rebuttal_only_rounds`
+is not one of `FRICTION_WEIGHTS`' drivers, and `test-quality` has no
+`gate_findings` entry to retype in the first place (see above), so a
+rebuttal-only round at test-quality is invisible to `computeFriction` and to
+every `gate_findings` rollup — it shows up only in this counter, in
+`ctx.contested`/`contestedBlock`, and in its own `VERIFY_SKIPS` line. This
+is accepted, not overlooked: a rebuttal genuinely costs less rework than a
+fix round did, and the two gates that do retype a disposition
+(quality, pr-review) still carry the signal into `gate_findings` where a
+rollup can see it.
+
 ## Durable per-issue gate state
 
 Issue #166 gave every issue a durable record of its own gate/contrarian