From 64477e06c03df8dcd7179866511fae5eb2379108 Mon Sep 17 00:00:00 2001 From: aaddrick Date: Tue, 28 Jul 2026 15:05:06 -0400 Subject: [PATCH 1/6] feat(gate-findings): add rebuttal primitives for evaluator-fed fix stages (#167) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 1 of #167: behaviour-neutral primitives that let a fixer treat a reviewer's finding as a hypothesis to verify rather than a command to obey. - FIX_SCHEMA.rebutted ({finding_id, evidence}), not in required — mirrors REVIEW_SCHEMA.issues staying out of its own required list. - FINDING_HYPOTHESIS_ASK beside HANDOFF_ASK/COMMIT_SHA_ASK: verify before acting, record disproof in `rebutted` if wrong, rebut only what you didn't fix, only bracketed-id findings are rebuttable, and a gate-agnostic consequence clause (no immediate-exit claim, since pr-review continues into another review round). - normalizeRebuttals(raw, findings): pure, fails toward today's behavior on every drop (non-array, blank finding_id/evidence, unrendered id); carries the matched finding's summary through. - contestedBlock(ctx): pure, renders rebutted-but-not-adjudicated findings back to the next reviewer with an inverted (vs settledBlock) closing contract, and states the iteration-2+ do-not-re-flag instruction doesn't apply to a contested entry. Never calls settleDecision(). Rendered immediately after settledBlock() at the quality/spec/code review prompts; emits '' until a later task populates ctx.contested. - retypeGateDisposition(ctx, gate, from, to): pure, moves one disposition count between buckets without touching count/severity; from === to is a documented count-preserving no-op, reachable at MAX_QUALITY_ITERATIONS. - ctx.metrics.rebuttal_only_rounds, plus tests/harness.js freshMetrics() parity. None of these are wired into the fix stages' control flow yet (task 2/3); FINDING_HYPOTHESIS_ASK and retypeGateDisposition are unused in production until then. 45 new unit tests in tests/gate-findings.test.js. node scripts/lint-engine.js --fix kept .claude/workflows/ticketmill.js in lockstep. node --test: 763/763 passing. --- .claude/workflows/ticketmill.js | 142 ++++++++++++++++- tests/gate-findings.test.js | 271 ++++++++++++++++++++++++++++++++ tests/harness.js | 2 +- workflows/ticketmill.js | 142 ++++++++++++++++- 4 files changed, 550 insertions(+), 7 deletions(-) diff --git a/.claude/workflows/ticketmill.js b/.claude/workflows/ticketmill.js index 3674917..3c78a8a 100644 --- a/.claude/workflows/ticketmill.js +++ b/.claude/workflows/ticketmill.js @@ -466,6 +466,17 @@ const FIX_SCHEMA = { status: { enum: ['success', 'error'] }, commit: { type: ['string', 'null'] }, files_changed: { type: 'array', items: { type: 'string' } }, fixes_applied: { type: 'array', items: { type: 'string' } }, summary: { type: 'string' }, error: { type: ['string', 'null'] }, + // rebutted (issue #167): a fixer's disagreement with a finding it judged wrong + // instead of fixing — mirrors how REVIEW_SCHEMA.issues stays out of + // REVIEW_SCHEMA's own required list (:450), so a fixer that never disagrees + // (the entire population before this issue) omits the key and produces a + // byte-identical response to today. normalizeRebuttals() below is the sole + // consumer; `id` is never part of this schema — finding_id must match an id + // the engine already assigned via normalizeFindings() and rendered to this + // fixer with a bracketed prefix (see FINDING_HYPOTHESIS_ASK). + rebutted: { type: 'array', items: { type: 'object', required: ['finding_id', 'evidence'], properties: { + finding_id: { type: 'string' }, evidence: { type: 'string' }, + } } }, notes_for_downstream: { type: 'array', items: { type: 'string' } }, }, } @@ -1878,6 +1889,36 @@ function settledBlock(ctx) { ].join('\n') } +// ----- contested-findings ledger (issue #167) ----- +// A finding a fixer rebutted (FIX_SCHEMA.rebutted, normalized by +// normalizeRebuttals() below) is a DISPUTE, not a resolution — nobody has +// judged whether the rebuttal's evidence actually disproves the finding. +// This renders those disputes back to the NEXT reviewer so it can adjudicate +// them, deliberately NOT reusing settleDecision()/settledBlock(): a settled +// entry carries a "don't re-open without new evidence" contract because +// someone already ruled on it; a contested entry carries the OPPOSITE +// contract, because no one has — this function never calls settleDecision(), +// only a reviewer (or eventually a human) closes a contested entry. Mirrors +// settledBlock's shape (defensive read, last-6 window, per-field slice caps, +// '' when empty) so both render identically at every site that pairs them. +function contestedBlock(ctx) { + const contested = (ctx && ctx.contested) || [] + if (!contested.length) return '' + return [ + '## Contested findings (rebutted by a fixer, NOT adjudicated — verify, do not assume)', + contested.slice(-6).map(function (c) { + return '- [' + c.gate + '] [' + c.id + '] ' + String(c.summary || '').slice(0, 400) + + '\n Fixer rebuttal: ' + String(c.evidence || '').slice(0, 300) + }).join('\n'), + 'A contested finding is NOT resolved: verify the rebuttal\'s evidence yourself. If it holds, say so and drop', + 'the finding. If it does not hold, re-raise it as a finding this iteration — do not let it sit contested', + 'indefinitely with neither outcome.', + 'A contested finding is NOT "already addressed" — no code changed for it and no one has ruled on it. The', + 'iteration-2+ instruction elsewhere in this prompt not to re-flag issues already addressed or accepted does', + 'NOT apply to anything in this block.', + ].join('\n') +} + // ----- handoff notes (agent -> future-agent context, 3-4 stages downstream) ----- const HANDOFF_ASK = 'If you discovered environment quirks, workarounds, or gotchas that later agents will need ' + '(test/env setup, shifted line numbers after deletes, tooling oddities), also return notes_for_downstream ' + @@ -1888,6 +1929,33 @@ const COMMIT_SHA_ASK = 'Get the exact commit SHA by running: git -C l 'full 40-character SHA. Paste that literal command output verbatim in the comment. Never type, shorten, guess, ' + 'or recall a SHA from memory.' +// ----- finding-hypothesis framing (issue #167) ----- +// A finding from a reviewer is a HYPOTHESIS to verify, not a command to obey +// unconditionally — the two contrarian revision stages (approach re-evaluate, +// plan re-plan) already carry this framing; nothing at the fix stages did, +// leaving a fixer that disagrees no path but silent compliance or +// status:'error'. Wired into exactly the three EVALUATOR-FED fix stages +// (quality-fix, test-quality-fix, pr-fix) — never the two ORACLE-fed ones +// (test-fix, browser-fix), which already carry a correct anti-rebuttal guard +// ("fix the real defect — do NOT delete/weaken assertions just to make the +// failure disappear") that this framing would invert: a failing test or a +// broken page is ground truth, not a hypothesis. The consequence clause is +// deliberately GATE-AGNOSTIC (no "ends this gate immediately" / "no further +// fix round runs" language) so the same string stays true whichever of the +// three gates renders it, including pr-review, where a rebuttal-only round +// continues into another review iteration rather than halting on the spot. +const FINDING_HYPOTHESIS_ASK = [ + 'Each finding above is a HYPOTHESIS the reviewer formed, not a command — verify it against the actual code before acting on it.', + 'If a finding is wrong, do NOT change code to satisfy it: record it in `rebutted` with the concrete check you ran ' + + 'that disproves it (a command, a line reference, a test result — not just disagreement).', + 'List everything you actually fixed in `fixes_applied`; rebut ONLY the findings you did not fix — never rebut a ' + + 'finding you also changed code for.', + 'Only a finding rendered above with a bracketed id (e.g. "[code-i1-2]") can be rebutted — anything you see only ' + + 'as prose must be fixed or addressed in `summary`, not rebutted.', + 'A round that rebuts every finding and applies no fix never counts as resolving this gate: it is recorded as an ' + + 'unresolved verification gap for a human to read, and it can never approve this gate on your say-so.', +].join('\n') + // ----- fixes_applied id-prefix ask (issue #162): shared by every fix stage fed // through findingsBlock() (quality-fix, test-quality-fix, pr-fix) so a human // skimming fixes_applied can map each entry straight back to the id-prefixed @@ -2034,6 +2102,40 @@ function recordGateOutcome(ctx, gate, findings, disposition) { g.disposition[d] = (g.disposition[d] || 0) + 1 } +// retypeGateDisposition (issue #167): moves exactly ONE count from disposition +// bucket `from` to bucket `to` within an already-recorded +// ctx.gate_findings[gate] entry, WITHOUT touching that gate's overall `count` +// or `severity` mix — both already reflect the findings themselves, which +// retyping a disposition label does not change. Exists because +// recordGateOutcome() above books a disposition BEFORE a fix stage runs (e.g. +// the final quality iteration books 'carried-unresolved' at :2933, then the +// fix stage's rebuttal is only known after that); this lets a later step +// correct the label on a bucket that already exists rather than double-count +// a second recordGateOutcome() call. No-ops (does nothing) when `gate` was +// never recorded, or when the `from` bucket doesn't exist — there is nothing +// to move. +// from === to IS A SUPPORTED, COUNT-PRESERVING NO-OP, not an error case to +// special-case away: it re-buckets a count into itself (subtract 1, then add +// 1 back to the same key), netting zero change. This is reachable in +// practice, not just theoretically — MAX_QUALITY_ITERATIONS is 5 (:46) and +// the quality gate already books 'carried-unresolved' on iteration 5 before +// any fix runs (:2933); a rebuttal-only fix on THAT iteration has nothing +// meaningful to retype the bucket to, so the call site retypes +// 'carried-unresolved' to itself for symmetry with every other iteration's +// call, rather than special-casing the last iteration to skip the call. +function retypeGateDisposition(ctx, gate, from, to) { + if (!ctx || !ctx.gate_findings) return + const key = String(gate || '').trim() + if (!key || !ctx.gate_findings[key]) return + const g = ctx.gate_findings[key] + const fromKey = String(from || '').trim() + const toKey = String(to || '').trim() + if (!fromKey || !toKey || !g.disposition[fromKey]) return + g.disposition[fromKey]-- + if (g.disposition[fromKey] <= 0) delete g.disposition[fromKey] + g.disposition[toKey] = (g.disposition[toKey] || 0) + 1 +} + // normalizeFindings (issue #162): turns a REVIEW_SCHEMA `issues` array into the // engine's structured finding shape, or signals "the reviewer omitted the key // entirely" so callers can fall back to today's prose-only path byte-for-byte. @@ -2065,6 +2167,39 @@ function normalizeFindings(raw, source) { }) } +// normalizeRebuttals (issue #167): turns FIX_SCHEMA's `rebutted` array into a +// validated list the engine can act on, mirroring normalizeFindings()'s +// fail-toward-existing-behavior contract just above it. `raw` not being an +// array (including undefined/null — a fixer that never disagreed, i.e. every +// fixer before this issue) returns [] rather than null: unlike +// normalizeFindings, there is no prose-fallback distinction worth preserving +// here — "nothing to act on" is the only meaningful outcome either way. Every +// drop (blank finding_id, blank evidence, or a finding_id absent from +// `findings` — the exact, possibly-null array actually rendered to this +// fixer) fails toward TODAY's behavior: the entry is silently dropped, never +// trusted, so a fixer cannot fabricate a rebuttal against a finding id it was +// never shown (FINDING_HYPOTHESIS_ASK's own "only a bracketed id can be +// rebutted" clause is enforced here, not just asked for in prose). A +// surviving entry carries the matched finding's `summary` through so +// contestedBlock() can render a self-contained line without a second lookup. +function normalizeRebuttals(raw, findings) { + if (!Array.isArray(raw)) return [] + const byId = {} + for (const f of (Array.isArray(findings) ? findings : [])) { + if (f && f.id) byId[f.id] = f + } + const out = [] + for (const entry of raw) { + const findingId = String((entry && entry.finding_id) || '').trim() + const evidence = String((entry && entry.evidence) || '').trim() + if (!findingId || !evidence) continue + const match = byId[findingId] + if (!match) continue + out.push({ finding_id: findingId, evidence: evidence, summary: match.summary || '' }) + } + return out +} + // findingsBlock (issue #162): the single renderer feeding every fix stage // (quality- fix, pr-fix, test-quality-fix) — the ONE place that decides what a // fix agent sees of a review, so structured findings and prose comments never @@ -2906,6 +3041,7 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { '## Decision chain (context from prior stages)', decisionChain(ctx), settledBlock(ctx), + contestedBlock(ctx), '', 'This is a task-level quality check inside the implementation workflow, NOT a full PR review.', 'Check: code patterns and standards, consistency with codebase conventions, potential bugs, security concerns.', @@ -4727,7 +4863,7 @@ async function reviewAndMerge(ctx) { 'Verify PR #' + ctx.pr + ' achieves the goals of issue #' + ctx.issue + ' (spec review iteration ' + iter + ').', '', '## Decision chain', decisionChain(ctx), - settledBlock(ctx), '', + settledBlock(ctx), contestedBlock(ctx), '', learn('workflow'), // issue #88: spec review sees prior-run workflow/scope learnings 'Check goal achievement, not code quality. Flag scope creep.', 'IMPORTANT: before flagging any acceptance criterion as missing, check base branch ' + TARGET + ' — if the', @@ -4748,7 +4884,7 @@ async function reviewAndMerge(ctx) { 'Review code quality of PR #' + ctx.pr + ' against base ' + TARGET + ' for issue #' + ctx.issue + ' (code review iteration ' + iter + ').', '', '## Decision chain', decisionChain(ctx), - settledBlock(ctx), '', + settledBlock(ctx), contestedBlock(ctx), '', // issue #88: the merge-gate reviewer sees prior-run error patterns and // quality-loop learnings — the bug classes earlier runs caught late. learn('error_patterns'), @@ -4995,7 +5131,7 @@ async function processIssue(pre) { // reasons:[]}) matches computeRevisitRisk's clean no-op shape so a preflight // that never carried revisit_risk (e.g. a resume-skip stub) never crashes. revisit_risk: (pre.revisit_risk && typeof pre.revisit_risk === 'object') ? pre.revisit_risk : { flagged: false, reasons: [] }, - metrics: { approach_iters: 0, plan_iters: 0, tasks_done: 0, tasks_failed: 0, task_review_attempts: 0, quality_iters: 0, quality_scopes: 0, quality_degrades: 0, test_iters: 0, browser_iters: 0, pr_review_iters: 0, merge_auto_resolved: 0, merge_thrash: 0, test_quality_fix_rounds: 0, findings_empty_exits: 0 }, + metrics: { approach_iters: 0, plan_iters: 0, tasks_done: 0, tasks_failed: 0, task_review_attempts: 0, quality_iters: 0, quality_scopes: 0, quality_degrades: 0, test_iters: 0, browser_iters: 0, pr_review_iters: 0, merge_auto_resolved: 0, merge_thrash: 0, test_quality_fix_rounds: 0, findings_empty_exits: 0, rebuttal_only_rounds: 0 }, tokens: { total: 0, byModel: {}, byStage: {}, tracked: false }, // per-stage token deltas from spentTokens(); see stage() // Retained-changed-files / friction signals (issue #87). null (not []) means // "never captured" — probeChangedFiles() sets these once, unconditionally, diff --git a/tests/gate-findings.test.js b/tests/gate-findings.test.js index 2130597..b53e706 100644 --- a/tests/gate-findings.test.js +++ b/tests/gate-findings.test.js @@ -333,3 +333,274 @@ test('nothingToFix: a changes_requested result with a non-empty findings array i assert.strictEqual(context.nothingToFix(changesRequestedReal, findings), false) }) + +// ---- FIX_SCHEMA.rebutted (issue #167) ---- +// +// Mirrors REVIEW_SCHEMA.issues (:450): declared on the schema so a fixer CAN +// disagree, but not in FIX_SCHEMA's own required list, so a fixer that never +// disagrees (every fixer before this issue) omits the key and validates +// exactly as before. + +test('FIX_SCHEMA: required is still exactly [status, summary] — rebutted is declared but optional (#167)', function () { + const context = harness.boot() + const schema = harness.readGlobal(context, 'FIX_SCHEMA') + + assert.deepStrictEqual(Array.from(schema.required), ['status', 'summary']) + assert.deepStrictEqual(Array.from(schema.properties.rebutted.items.required), ['finding_id', 'evidence']) + assert.strictEqual(schema.properties.rebutted.type, 'array') +}) + +// ---- FINDING_HYPOTHESIS_ASK (issue #167) ---- +// +// Shared verbatim by the three evaluator-fed fix stages (quality-fix, +// test-quality-fix, pr-fix). The consequence clause is gate-agnostic on +// purpose: it must read true whichever of the three gates renders it, +// including pr-review, where a rebuttal-only round continues into another +// review iteration rather than halting the gate on the spot — so the string +// must NOT claim an immediate exit or that no further fix round runs. + +test('FINDING_HYPOTHESIS_ASK: contains no immediate-exit wording (#167)', function () { + const context = harness.boot() + const ask = harness.readGlobal(context, 'FINDING_HYPOTHESIS_ASK') + + assert.ok(!/ends this gate immediately/i.test(ask), 'must not claim the gate ends immediately: ' + ask) + assert.ok(!/no further fix round runs/i.test(ask), 'must not claim no further fix round runs: ' + ask) +}) + +test('FINDING_HYPOTHESIS_ASK: carries the verify-before-acting, rebut-only-what-you-did-not-fix, and bracketed-id clauses', function () { + const context = harness.boot() + const ask = harness.readGlobal(context, 'FINDING_HYPOTHESIS_ASK') + + assert.ok(/hypothesis/i.test(ask), 'must frame findings as hypotheses: ' + ask) + assert.ok(/rebutted/.test(ask), 'must name the `rebutted` field: ' + ask) + assert.ok(/fixes_applied/.test(ask), 'must name the `fixes_applied` field: ' + ask) + assert.ok(/bracketed id/i.test(ask), 'must restrict rebuttal to bracketed-id findings: ' + ask) +}) + +// ---- normalizeRebuttals(raw, findings) (issue #167) ---- +// +// Turns FIX_SCHEMA's `rebutted` array into a validated list, mirroring +// normalizeFindings()'s fail-toward-existing-behavior contract: every drop +// (blank finding_id/evidence, an id absent from the rendered finding set) +// silently drops the entry rather than trusting it. + +test('normalizeRebuttals: a non-array raw (undefined — a fixer that never disagreed) returns []', function () { + const context = harness.boot() + const findings = context.normalizeFindings([{ severity: 'major', summary: 'a' }], 'code-i1') + + harness.assertVmEqual(context.normalizeRebuttals(undefined, findings), []) + harness.assertVmEqual(context.normalizeRebuttals(null, findings), []) + harness.assertVmEqual(context.normalizeRebuttals('not an array', findings), []) +}) + +test('normalizeRebuttals: drops entries with a blank/missing finding_id', function () { + const context = harness.boot() + const findings = context.normalizeFindings([{ severity: 'major', summary: 'a' }], 'code-i1') + + const out = context.normalizeRebuttals([ + { finding_id: '', evidence: 'checked it, still wrong' }, + { evidence: 'no finding_id key at all' }, + { finding_id: ' ', evidence: 'whitespace-only id' }, + ], findings) + + harness.assertVmEqual(out, []) +}) + +test('normalizeRebuttals: drops entries with a blank/missing evidence', function () { + const context = harness.boot() + const findings = context.normalizeFindings([{ severity: 'major', summary: 'a' }], 'code-i1') + + const out = context.normalizeRebuttals([ + { finding_id: 'code-i1-1', evidence: '' }, + { finding_id: 'code-i1-1' }, + { finding_id: 'code-i1-1', evidence: ' ' }, + ], findings) + + harness.assertVmEqual(out, []) +}) + +test('normalizeRebuttals: drops a finding_id absent from the rendered finding set — a fixer cannot rebut an id it was never shown', function () { + const context = harness.boot() + const findings = context.normalizeFindings([{ severity: 'major', summary: 'a' }], 'code-i1') + + const out = context.normalizeRebuttals([ + { finding_id: 'code-i1-99', evidence: 'this id was never rendered to me' }, + ], findings) + + harness.assertVmEqual(out, []) +}) + +test('normalizeRebuttals: findings === null (issues omitted, prose-fallback path) drops every rebuttal — nothing was ever rendered with an id', function () { + const context = harness.boot() + + const out = context.normalizeRebuttals([ + { finding_id: 'code-i1-1', evidence: 'some evidence' }, + ], null) + + harness.assertVmEqual(out, []) +}) + +test('normalizeRebuttals: a surviving entry carries the matched finding\'s summary through', function () { + const context = harness.boot() + const findings = context.normalizeFindings([ + { severity: 'major', summary: 'the null guard is too loose' }, + { severity: 'minor', summary: 'nit: naming' }, + ], 'code-i1') + + const out = context.normalizeRebuttals([ + { finding_id: 'code-i1-1', evidence: 'ran the reproducer at commit abc123, guard already covers this input' }, + ], findings) + + harness.assertVmEqual(out, [ + { + finding_id: 'code-i1-1', + evidence: 'ran the reproducer at commit abc123, guard already covers this input', + summary: 'the null guard is too loose', + }, + ]) +}) + +test('normalizeRebuttals: arity is NOT preserved (unlike normalizeFindings) — drops are just dropped, never placeholder-preserved', function () { + const context = harness.boot() + const findings = context.normalizeFindings([{ severity: 'major', summary: 'a' }], 'code-i1') + + const out = context.normalizeRebuttals([ + { finding_id: 'code-i1-1', evidence: 'valid' }, + { finding_id: '', evidence: 'blank id, dropped' }, + { finding_id: 'code-i1-1', evidence: 'a second valid rebuttal of the same finding, also kept' }, + ], findings) + + assert.strictEqual(out.length, 2) +}) + +// ---- contestedBlock(ctx) (issue #167) ---- +// +// Renders findings a fixer rebutted but nobody has adjudicated yet, back to +// the NEXT reviewer. Deliberately does NOT call settleDecision() — this is a +// dispute ledger, not an adjudication ledger. + +test('contestedBlock: an empty/absent ctx.contested renders as the empty string', function () { + const context = harness.boot() + + assert.strictEqual(context.contestedBlock({ contested: [] }), '') + assert.strictEqual(context.contestedBlock({}), '') + assert.strictEqual(context.contestedBlock(null), '') + assert.strictEqual(context.contestedBlock(undefined), '') +}) + +test('contestedBlock: a single entry renders its gate, id, finding summary, and the fixer\'s evidence', function () { + const context = harness.boot() + const block = context.contestedBlock({ + contested: [ + { gate: 'quality', id: 'quality-task-1-i1-2', summary: 'the null guard is too loose', evidence: 'ran the reproducer, guard already covers this input' }, + ], + }) + + assert.ok(block.includes('quality'), 'must name the gate: ' + block) + assert.ok(block.includes('quality-task-1-i1-2'), 'must name the finding id: ' + block) + assert.ok(block.includes('the null guard is too loose'), 'must include the finding summary: ' + block) + assert.ok(block.includes('ran the reproducer, guard already covers this input'), 'must include the fixer\'s evidence: ' + block) +}) + +test('contestedBlock: the heading names entries as contested and explicitly NOT adjudicated', function () { + const context = harness.boot() + const block = context.contestedBlock({ contested: [{ gate: 'quality', id: 'x-1', summary: 's', evidence: 'e' }] }) + const heading = block.split('\n')[0] + + assert.ok(/contested/i.test(heading), 'heading must name entries as contested: ' + heading) + assert.ok(/not adjudicated/i.test(heading), 'heading must state these are NOT adjudicated: ' + heading) +}) + +test('contestedBlock: closing contract is INVERTED from settledBlock — it tells the reviewer to adjudicate, not to leave settled decisions alone', function () { + const context = harness.boot() + const block = context.contestedBlock({ contested: [{ gate: 'quality', id: 'x-1', summary: 's', evidence: 'e' }] }) + + // settledBlock's contract discourages re-opening without new evidence; + // contestedBlock's contract does the opposite — it requires the reviewer to + // actually make a call (verify -> drop, or re-raise as a fresh finding). + assert.ok(/verify the rebuttal/i.test(block), 'must instruct the reviewer to verify the rebuttal: ' + block) + assert.ok(/drop/i.test(block), 'must offer "drop the finding" as one adjudicated outcome: ' + block) + assert.ok(/re-raise/i.test(block), 'must offer "re-raise" as the other adjudicated outcome: ' + block) + assert.ok(!/re-litigating.*process failure/i.test(block), 'must NOT carry settledBlock\'s discourage-reopening contract: ' + block) +}) + +test('contestedBlock: states a contested finding is not "already addressed" and that the iteration-2+ do-not-re-flag instruction does not apply to it', function () { + const context = harness.boot() + const block = context.contestedBlock({ contested: [{ gate: 'quality', id: 'x-1', summary: 's', evidence: 'e' }] }) + + assert.ok(/not.*already addressed/i.test(block), 'must state a contested finding is not "already addressed": ' + block) + assert.ok(/does not apply|not apply/i.test(block), 'must state the iteration-2+ do-not-re-flag instruction does not apply here: ' + block) +}) + +test('contestedBlock: renders only the last 6 entries', function () { + const context = harness.boot() + const contested = [] + for (let i = 1; i <= 9; i++) contested.push({ gate: 'quality', id: 'x-' + i, summary: 'summary ' + i, evidence: 'evidence ' + i }) + + const block = context.contestedBlock({ contested: contested }) + + for (let i = 1; i <= 3; i++) assert.ok(!new RegExp('\\bx-' + i + '\\b').test(block), 'entry x-' + i + ' should have been dropped by the last-6 window: ' + block) + for (let i = 4; i <= 9; i++) assert.ok(new RegExp('\\bx-' + i + '\\b').test(block), 'entry x-' + i + ' should be within the last-6 window: ' + block) +}) + +// ---- retypeGateDisposition(ctx, gate, from, to) (issue #167) ---- +// +// Moves exactly one count from disposition bucket `from` to `to` within an +// already-recorded ctx.gate_findings[gate] entry, without touching `count` +// or `severity`. + +test('retypeGateDisposition: moves one count from an existing bucket to a new one, leaving count/severity untouched', function () { + const context = harness.boot() + const ctx = harness.makeCtx({ issue: 10 }) + context.recordGateOutcome(ctx, 'quality', [{ severity: 'major', summary: 'a' }], 'carried-unresolved') + + context.retypeGateDisposition(ctx, 'quality', 'carried-unresolved', 'rebutted-unresolved') + + const g = ctx.gate_findings.quality + assert.strictEqual(g.count, 1) + harness.assertVmEqual(g.severity, { critical: 0, major: 1, minor: 0 }) + harness.assertVmEqual(g.disposition, { 'rebutted-unresolved': 1 }) +}) + +test('retypeGateDisposition: is a no-op when the `from` bucket does not exist on that gate', function () { + const context = harness.boot() + const ctx = harness.makeCtx({ issue: 11 }) + context.recordGateOutcome(ctx, 'quality', [{ severity: 'minor', summary: 'a' }], 'accepted') + + context.retypeGateDisposition(ctx, 'quality', 'carried-unresolved', 'rebutted-unresolved') + + harness.assertVmEqual(ctx.gate_findings.quality.disposition, { accepted: 1 }) +}) + +test('retypeGateDisposition: is a no-op when the gate itself was never recorded — never throws', function () { + const context = harness.boot() + const ctx = harness.makeCtx({ issue: 12 }) + + assert.doesNotThrow(function () { context.retypeGateDisposition(ctx, 'quality', 'carried-unresolved', 'rebutted-unresolved') }) + assert.deepStrictEqual(ctx.gate_findings, {}) +}) + +test('retypeGateDisposition: defensive against a missing/partial ctx — never throws', function () { + const context = harness.boot() + + assert.doesNotThrow(function () { context.retypeGateDisposition(null, 'quality', 'a', 'b') }) + assert.doesNotThrow(function () { context.retypeGateDisposition({}, 'quality', 'a', 'b') }) +}) + +test('retypeGateDisposition: from === to is a count-preserving no-op — reachable at MAX_QUALITY_ITERATIONS, where iteration 5 already books carried-unresolved before any fix runs', function () { + const context = harness.boot() + const ctx = harness.makeCtx({ issue: 13 }) + context.recordGateOutcome(ctx, 'quality', [{ severity: 'major', summary: 'a' }], 'carried-unresolved') + context.recordGateOutcome(ctx, 'quality', [{ severity: 'minor', summary: 'b' }], 'carried-unresolved') + context.recordGateOutcome(ctx, 'quality', [{ severity: 'critical', summary: 'c' }], 'carried-unresolved') + + const before = JSON.parse(JSON.stringify(ctx.gate_findings.quality)) + + context.retypeGateDisposition(ctx, 'quality', 'carried-unresolved', 'carried-unresolved') + + const g = ctx.gate_findings.quality + assert.strictEqual(g.count, 3) + harness.assertVmEqual(g.severity, { critical: 1, major: 1, minor: 1 }) + harness.assertVmEqual(g.disposition, { 'carried-unresolved': 3 }) + assert.deepStrictEqual(JSON.parse(JSON.stringify(g)), before) +}) diff --git a/tests/harness.js b/tests/harness.js index e2ec9d8..90013cd 100644 --- a/tests/harness.js +++ b/tests/harness.js @@ -85,7 +85,7 @@ function freshMetrics() { task_review_attempts: 0, quality_iters: 0, quality_scopes: 0, quality_degrades: 0, test_iters: 0, browser_iters: 0, pr_review_iters: 0, merge_auto_resolved: 0, merge_thrash: 0, test_quality_fix_rounds: 0, - findings_empty_exits: 0, + findings_empty_exits: 0, rebuttal_only_rounds: 0, } } diff --git a/workflows/ticketmill.js b/workflows/ticketmill.js index 3674917..3c78a8a 100644 --- a/workflows/ticketmill.js +++ b/workflows/ticketmill.js @@ -466,6 +466,17 @@ const FIX_SCHEMA = { status: { enum: ['success', 'error'] }, commit: { type: ['string', 'null'] }, files_changed: { type: 'array', items: { type: 'string' } }, fixes_applied: { type: 'array', items: { type: 'string' } }, summary: { type: 'string' }, error: { type: ['string', 'null'] }, + // rebutted (issue #167): a fixer's disagreement with a finding it judged wrong + // instead of fixing — mirrors how REVIEW_SCHEMA.issues stays out of + // REVIEW_SCHEMA's own required list (:450), so a fixer that never disagrees + // (the entire population before this issue) omits the key and produces a + // byte-identical response to today. normalizeRebuttals() below is the sole + // consumer; `id` is never part of this schema — finding_id must match an id + // the engine already assigned via normalizeFindings() and rendered to this + // fixer with a bracketed prefix (see FINDING_HYPOTHESIS_ASK). + rebutted: { type: 'array', items: { type: 'object', required: ['finding_id', 'evidence'], properties: { + finding_id: { type: 'string' }, evidence: { type: 'string' }, + } } }, notes_for_downstream: { type: 'array', items: { type: 'string' } }, }, } @@ -1878,6 +1889,36 @@ function settledBlock(ctx) { ].join('\n') } +// ----- contested-findings ledger (issue #167) ----- +// A finding a fixer rebutted (FIX_SCHEMA.rebutted, normalized by +// normalizeRebuttals() below) is a DISPUTE, not a resolution — nobody has +// judged whether the rebuttal's evidence actually disproves the finding. +// This renders those disputes back to the NEXT reviewer so it can adjudicate +// them, deliberately NOT reusing settleDecision()/settledBlock(): a settled +// entry carries a "don't re-open without new evidence" contract because +// someone already ruled on it; a contested entry carries the OPPOSITE +// contract, because no one has — this function never calls settleDecision(), +// only a reviewer (or eventually a human) closes a contested entry. Mirrors +// settledBlock's shape (defensive read, last-6 window, per-field slice caps, +// '' when empty) so both render identically at every site that pairs them. +function contestedBlock(ctx) { + const contested = (ctx && ctx.contested) || [] + if (!contested.length) return '' + return [ + '## Contested findings (rebutted by a fixer, NOT adjudicated — verify, do not assume)', + contested.slice(-6).map(function (c) { + return '- [' + c.gate + '] [' + c.id + '] ' + String(c.summary || '').slice(0, 400) + + '\n Fixer rebuttal: ' + String(c.evidence || '').slice(0, 300) + }).join('\n'), + 'A contested finding is NOT resolved: verify the rebuttal\'s evidence yourself. If it holds, say so and drop', + 'the finding. If it does not hold, re-raise it as a finding this iteration — do not let it sit contested', + 'indefinitely with neither outcome.', + 'A contested finding is NOT "already addressed" — no code changed for it and no one has ruled on it. The', + 'iteration-2+ instruction elsewhere in this prompt not to re-flag issues already addressed or accepted does', + 'NOT apply to anything in this block.', + ].join('\n') +} + // ----- handoff notes (agent -> future-agent context, 3-4 stages downstream) ----- const HANDOFF_ASK = 'If you discovered environment quirks, workarounds, or gotchas that later agents will need ' + '(test/env setup, shifted line numbers after deletes, tooling oddities), also return notes_for_downstream ' + @@ -1888,6 +1929,33 @@ const COMMIT_SHA_ASK = 'Get the exact commit SHA by running: git -C l 'full 40-character SHA. Paste that literal command output verbatim in the comment. Never type, shorten, guess, ' + 'or recall a SHA from memory.' +// ----- finding-hypothesis framing (issue #167) ----- +// A finding from a reviewer is a HYPOTHESIS to verify, not a command to obey +// unconditionally — the two contrarian revision stages (approach re-evaluate, +// plan re-plan) already carry this framing; nothing at the fix stages did, +// leaving a fixer that disagrees no path but silent compliance or +// status:'error'. Wired into exactly the three EVALUATOR-FED fix stages +// (quality-fix, test-quality-fix, pr-fix) — never the two ORACLE-fed ones +// (test-fix, browser-fix), which already carry a correct anti-rebuttal guard +// ("fix the real defect — do NOT delete/weaken assertions just to make the +// failure disappear") that this framing would invert: a failing test or a +// broken page is ground truth, not a hypothesis. The consequence clause is +// deliberately GATE-AGNOSTIC (no "ends this gate immediately" / "no further +// fix round runs" language) so the same string stays true whichever of the +// three gates renders it, including pr-review, where a rebuttal-only round +// continues into another review iteration rather than halting on the spot. +const FINDING_HYPOTHESIS_ASK = [ + 'Each finding above is a HYPOTHESIS the reviewer formed, not a command — verify it against the actual code before acting on it.', + 'If a finding is wrong, do NOT change code to satisfy it: record it in `rebutted` with the concrete check you ran ' + + 'that disproves it (a command, a line reference, a test result — not just disagreement).', + 'List everything you actually fixed in `fixes_applied`; rebut ONLY the findings you did not fix — never rebut a ' + + 'finding you also changed code for.', + 'Only a finding rendered above with a bracketed id (e.g. "[code-i1-2]") can be rebutted — anything you see only ' + + 'as prose must be fixed or addressed in `summary`, not rebutted.', + 'A round that rebuts every finding and applies no fix never counts as resolving this gate: it is recorded as an ' + + 'unresolved verification gap for a human to read, and it can never approve this gate on your say-so.', +].join('\n') + // ----- fixes_applied id-prefix ask (issue #162): shared by every fix stage fed // through findingsBlock() (quality-fix, test-quality-fix, pr-fix) so a human // skimming fixes_applied can map each entry straight back to the id-prefixed @@ -2034,6 +2102,40 @@ function recordGateOutcome(ctx, gate, findings, disposition) { g.disposition[d] = (g.disposition[d] || 0) + 1 } +// retypeGateDisposition (issue #167): moves exactly ONE count from disposition +// bucket `from` to bucket `to` within an already-recorded +// ctx.gate_findings[gate] entry, WITHOUT touching that gate's overall `count` +// or `severity` mix — both already reflect the findings themselves, which +// retyping a disposition label does not change. Exists because +// recordGateOutcome() above books a disposition BEFORE a fix stage runs (e.g. +// the final quality iteration books 'carried-unresolved' at :2933, then the +// fix stage's rebuttal is only known after that); this lets a later step +// correct the label on a bucket that already exists rather than double-count +// a second recordGateOutcome() call. No-ops (does nothing) when `gate` was +// never recorded, or when the `from` bucket doesn't exist — there is nothing +// to move. +// from === to IS A SUPPORTED, COUNT-PRESERVING NO-OP, not an error case to +// special-case away: it re-buckets a count into itself (subtract 1, then add +// 1 back to the same key), netting zero change. This is reachable in +// practice, not just theoretically — MAX_QUALITY_ITERATIONS is 5 (:46) and +// the quality gate already books 'carried-unresolved' on iteration 5 before +// any fix runs (:2933); a rebuttal-only fix on THAT iteration has nothing +// meaningful to retype the bucket to, so the call site retypes +// 'carried-unresolved' to itself for symmetry with every other iteration's +// call, rather than special-casing the last iteration to skip the call. +function retypeGateDisposition(ctx, gate, from, to) { + if (!ctx || !ctx.gate_findings) return + const key = String(gate || '').trim() + if (!key || !ctx.gate_findings[key]) return + const g = ctx.gate_findings[key] + const fromKey = String(from || '').trim() + const toKey = String(to || '').trim() + if (!fromKey || !toKey || !g.disposition[fromKey]) return + g.disposition[fromKey]-- + if (g.disposition[fromKey] <= 0) delete g.disposition[fromKey] + g.disposition[toKey] = (g.disposition[toKey] || 0) + 1 +} + // normalizeFindings (issue #162): turns a REVIEW_SCHEMA `issues` array into the // engine's structured finding shape, or signals "the reviewer omitted the key // entirely" so callers can fall back to today's prose-only path byte-for-byte. @@ -2065,6 +2167,39 @@ function normalizeFindings(raw, source) { }) } +// normalizeRebuttals (issue #167): turns FIX_SCHEMA's `rebutted` array into a +// validated list the engine can act on, mirroring normalizeFindings()'s +// fail-toward-existing-behavior contract just above it. `raw` not being an +// array (including undefined/null — a fixer that never disagreed, i.e. every +// fixer before this issue) returns [] rather than null: unlike +// normalizeFindings, there is no prose-fallback distinction worth preserving +// here — "nothing to act on" is the only meaningful outcome either way. Every +// drop (blank finding_id, blank evidence, or a finding_id absent from +// `findings` — the exact, possibly-null array actually rendered to this +// fixer) fails toward TODAY's behavior: the entry is silently dropped, never +// trusted, so a fixer cannot fabricate a rebuttal against a finding id it was +// never shown (FINDING_HYPOTHESIS_ASK's own "only a bracketed id can be +// rebutted" clause is enforced here, not just asked for in prose). A +// surviving entry carries the matched finding's `summary` through so +// contestedBlock() can render a self-contained line without a second lookup. +function normalizeRebuttals(raw, findings) { + if (!Array.isArray(raw)) return [] + const byId = {} + for (const f of (Array.isArray(findings) ? findings : [])) { + if (f && f.id) byId[f.id] = f + } + const out = [] + for (const entry of raw) { + const findingId = String((entry && entry.finding_id) || '').trim() + const evidence = String((entry && entry.evidence) || '').trim() + if (!findingId || !evidence) continue + const match = byId[findingId] + if (!match) continue + out.push({ finding_id: findingId, evidence: evidence, summary: match.summary || '' }) + } + return out +} + // findingsBlock (issue #162): the single renderer feeding every fix stage // (quality- fix, pr-fix, test-quality-fix) — the ONE place that decides what a // fix agent sees of a review, so structured findings and prose comments never @@ -2906,6 +3041,7 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { '## Decision chain (context from prior stages)', decisionChain(ctx), settledBlock(ctx), + contestedBlock(ctx), '', 'This is a task-level quality check inside the implementation workflow, NOT a full PR review.', 'Check: code patterns and standards, consistency with codebase conventions, potential bugs, security concerns.', @@ -4727,7 +4863,7 @@ async function reviewAndMerge(ctx) { 'Verify PR #' + ctx.pr + ' achieves the goals of issue #' + ctx.issue + ' (spec review iteration ' + iter + ').', '', '## Decision chain', decisionChain(ctx), - settledBlock(ctx), '', + settledBlock(ctx), contestedBlock(ctx), '', learn('workflow'), // issue #88: spec review sees prior-run workflow/scope learnings 'Check goal achievement, not code quality. Flag scope creep.', 'IMPORTANT: before flagging any acceptance criterion as missing, check base branch ' + TARGET + ' — if the', @@ -4748,7 +4884,7 @@ async function reviewAndMerge(ctx) { 'Review code quality of PR #' + ctx.pr + ' against base ' + TARGET + ' for issue #' + ctx.issue + ' (code review iteration ' + iter + ').', '', '## Decision chain', decisionChain(ctx), - settledBlock(ctx), '', + settledBlock(ctx), contestedBlock(ctx), '', // issue #88: the merge-gate reviewer sees prior-run error patterns and // quality-loop learnings — the bug classes earlier runs caught late. learn('error_patterns'), @@ -4995,7 +5131,7 @@ async function processIssue(pre) { // reasons:[]}) matches computeRevisitRisk's clean no-op shape so a preflight // that never carried revisit_risk (e.g. a resume-skip stub) never crashes. revisit_risk: (pre.revisit_risk && typeof pre.revisit_risk === 'object') ? pre.revisit_risk : { flagged: false, reasons: [] }, - metrics: { approach_iters: 0, plan_iters: 0, tasks_done: 0, tasks_failed: 0, task_review_attempts: 0, quality_iters: 0, quality_scopes: 0, quality_degrades: 0, test_iters: 0, browser_iters: 0, pr_review_iters: 0, merge_auto_resolved: 0, merge_thrash: 0, test_quality_fix_rounds: 0, findings_empty_exits: 0 }, + metrics: { approach_iters: 0, plan_iters: 0, tasks_done: 0, tasks_failed: 0, task_review_attempts: 0, quality_iters: 0, quality_scopes: 0, quality_degrades: 0, test_iters: 0, browser_iters: 0, pr_review_iters: 0, merge_auto_resolved: 0, merge_thrash: 0, test_quality_fix_rounds: 0, findings_empty_exits: 0, rebuttal_only_rounds: 0 }, tokens: { total: 0, byModel: {}, byStage: {}, tracked: false }, // per-stage token deltas from spentTokens(); see stage() // Retained-changed-files / friction signals (issue #87). null (not []) means // "never captured" — probeChangedFiles() sets these once, unconditionally, From e122632f25af9c635bcc9aaac079bd939b2c87a8 Mon Sep 17 00:00:00 2001 From: aaddrick Date: Tue, 28 Jul 2026 15:18:10 -0400 Subject: [PATCH 2/6] feat(gate-findings): wire rebuttal-only exits into the quality and test loops (#167) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renders FINDING_HYPOTHESIS_ASK in the quality-fix and test-quality-fix prompts only when the reviewer/validator named structured findings, and wires normalizeRebuttals()/retypeGateDisposition()/contestedBlock() (from task 1) into both evaluator-fed fix loops so a fix that rebuts every finding and applies none can never approve its own gate: - runQualityLoop: a third loop-exit flag `rebutted`, kept separate from `degraded` so the rolling degrade window is untouched, retypes the disposition already booked for that iteration to 'carried-unresolved', records the rebuttal as contested (not settled), and rolls up to exactly one VERIFY_SKIPS line per issue (ctx.quality_rebuttals / quality_rebuttal_skip_index), mirroring the existing cap roll-up. - runTestLoop: same predicate at test-quality-fix, exits through the loop's own { ok: true } path (never ok:false) so a rebuttal-only round can never route through the 'test-loop' merge-block stage key. No gate key to retype here (test-quality books none today). The two ORACLE-fed fix stages (test-fix, browser-fix) are untouched and keep their anti-rebuttal guards — a failing test is ground truth, not a hypothesis to verify. Extends tests/quality-loop.test.js and tests/test-loop.test.js with the rebuttal-only exit, partial-rebuttal/omitted-field/blank-evidence non-triggers, the rolled-up VERIFY_SKIPS line, and scope-pinning assertions proving FINDING_HYPOTHESIS_ASK renders only at the three evaluator-fed sites and never at test-fix. --- .claude/workflows/ticketmill.js | 82 +++++++++- tests/quality-loop.test.js | 281 ++++++++++++++++++++++++++++++++ tests/test-loop.test.js | 149 +++++++++++++++++ workflows/ticketmill.js | 82 +++++++++- 4 files changed, 588 insertions(+), 6 deletions(-) diff --git a/.claude/workflows/ticketmill.js b/.claude/workflows/ticketmill.js index 3c78a8a..2f71d0d 100644 --- a/.claude/workflows/ticketmill.js +++ b/.claude/workflows/ticketmill.js @@ -3001,7 +3001,14 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { let runSimplify = !Array.isArray(filesChanged) || filesChanged.length === 0 || filesChanged.some(inScope) let approved = false let degraded = false - for (let iter = 1; !approved && !degraded && iter <= MAX_QUALITY_ITERATIONS; iter++) { + // rebutted (issue #167): a third, separate loop-exit flag from `degraded` — + // a fix that rebuts every finding and applies none is NOT an agent failure + // (degraded stays false, so the rolling degrade window/quality_degrades + // below are untouched), but it also cannot approve itself, so the loop must + // still stop rather than spend its remaining iterations re-litigating a + // dispute only a human/reviewer can adjudicate. + let rebutted = false + for (let iter = 1; !approved && !degraded && !rebutted && iter <= MAX_QUALITY_ITERATIONS; iter++) { if (STOP.tripped) return 'halted' ctx.metrics.quality_iters++ if (iter === 1) ctx.metrics.quality_scopes++ @@ -3066,7 +3073,8 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { break } - recordGateOutcome(ctx, 'quality', revFindings || [], iter === MAX_QUALITY_ITERATIONS ? 'carried-unresolved' : 're-litigated') + const bookedDisposition = iter === MAX_QUALITY_ITERATIONS ? 'carried-unresolved' : 're-litigated' + recordGateOutcome(ctx, 'quality', revFindings || [], bookedDisposition) const fixAgent = pickFixAgent(rev.recommended_fix_agent, null) const fix = await stage(ctx, 'quality-fix-' + prefix + '-i' + iter, [ implementerBlock(fixAgent), @@ -3080,6 +3088,7 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { 'After committing, post an issue comment "## Quality Fix (' + stepLabel + ', iteration ' + iter + ')" with the', 'commit SHA and the fixes applied in 2-4 lines (gh issue comment ' + ctx.issue + ' --repo ' + REPO + ').', fixesAppliedIdAsk('[quality-task-1-i1-2] tightened the null guard'), + revFindings !== null ? FINDING_HYPOTHESIS_ASK : '', COMMIT_SHA_ASK, bwFeedback(ctx), HANDOFF_ASK, @@ -3088,6 +3097,42 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { if (!fix || fix.status === 'error') { degraded = true; log('#' + ctx.issue + ' quality ' + prefix + ' i' + iter + ': fix degraded'); break } collectNotes(ctx, 'quality-fix', fix) collectPostedCommit(ctx, 'quality-fix-' + prefix + '-i' + iter, fix) + + // rebuttal-only exit (issue #167): a fix that rebutted every finding it was + // shown and applied none is a DISPUTE, not a resolution — see the + // rebuttalOnly comment above runTestLoop's mirror of this block for the + // full contract this predicate enforces. Evaluated AFTER collectNotes/ + // collectPostedCommit above (so handoff notes and the posted-commit ledger + // still see this round) but BEFORE tallyTouches just below, which is a + // verified no-op here anyway since a true rebuttalOnly round always has an + // empty files_changed by construction. + const normRebuttals = normalizeRebuttals(fix.rebutted, revFindings || []) + const rebuttalOnly = normRebuttals.length > 0 && (fix.fixes_applied || []).length === 0 && (fix.files_changed || []).length === 0 + if (rebuttalOnly) { + rebutted = true + retypeGateDisposition(ctx, 'quality', bookedDisposition, 'carried-unresolved') + if (!ctx.contested) ctx.contested = [] + for (const r of normRebuttals) { + ctx.contested.push({ gate: 'quality-' + prefix, id: r.finding_id, summary: r.summary, evidence: r.evidence }) + } + pushDecision(ctx, 'Gate: findings contested, none applied', 'Quality fix (' + stepLabel + ', iteration ' + iter + ') rebutted every finding and applied no fix — carried as contested, not resolved.') + ctx.metrics.rebuttal_only_rounds++ + // Rolled up to exactly ONE VERIFY_SKIPS line per issue, mirroring the + // quality-cap roll-up just below (runQualityLoop runs once per task plus + // once per PR-fix round, so without this a chatty issue would print one + // line per rebuttal-only round). + if (!ctx.quality_rebuttals) ctx.quality_rebuttals = [] + ctx.quality_rebuttals.push(stepLabel) + const rebutMsg = '#' + ctx.issue + ': quality gate rebuttal-only, no fix applied (' + ctx.quality_rebuttals.join(', ') + ') — findings contested, carried for human review' + if (typeof ctx.quality_rebuttal_skip_index === 'number' && ctx.quality_rebuttal_skip_index < VERIFY_SKIPS.length) { + VERIFY_SKIPS[ctx.quality_rebuttal_skip_index] = rebutMsg + } else { + ctx.quality_rebuttal_skip_index = VERIFY_SKIPS.length + VERIFY_SKIPS.push(rebutMsg) + } + break + } + tallyTouches(ctx, fix.files_changed) if (!runSimplify && (fix.files_changed || []).some(inScope)) runSimplify = true } @@ -3110,7 +3155,12 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { // exit via the degrade-window halt just below, which both callers // (reviewAndMerge, the per-task loop) turn into a hard fail() — so this // line must not claim anything merged on a capped-then-halted issue. - if (!approved && !degraded) { + // Gated to !rebutted too (issue #167): a rebuttal-only exit already pushed + // its own rolled-up VERIFY_SKIPS line above and broke the loop before + // reaching MAX_QUALITY_ITERATIONS — it must not ALSO be counted as a cap + // exhaustion, which would misreport a contested-findings round as the loop + // having simply run out of iterations. + if (!approved && !degraded && !rebutted) { if (!ctx.quality_caps) ctx.quality_caps = [] ctx.quality_caps.push(stepLabel) const capMsg = '#' + ctx.issue + ': quality gate capped at ' + MAX_QUALITY_ITERATIONS + ' iterations without a clean review (' + ctx.quality_caps.join(', ') + ')' @@ -3579,6 +3629,7 @@ async function runTestLoop(ctx, forced) { 'After committing, post an issue comment "## Test Quality Fix (iteration ' + iter + ')" with the commit SHA and', 'what was added/strengthened (gh issue comment ' + ctx.issue + ' --repo ' + REPO + ').', fixesAppliedIdAsk('[test-i1-2] added a missing edge-case test'), + vFindings !== null ? FINDING_HYPOTHESIS_ASK : '', COMMIT_SHA_ASK, HANDOFF_ASK, 'Add missing assertions, remove TODOs, add edge-case tests, etc. Commit. Return status, commit, files_changed, fixes_applied, summary.', @@ -3586,6 +3637,31 @@ async function runTestLoop(ctx, forced) { if (!qfix || qfix.status === 'error') return { ok: false, error: 'test-quality-fix stage failed — halting test loop' } collectNotes(ctx, 'test-quality-fix', qfix) collectPostedCommit(ctx, 'test-quality-fix-i' + iter, qfix) + + // rebuttal-only exit (issue #167): mirrors runQualityLoop's rebuttalOnly + // block above, minus the recordGateOutcome/retypeGateDisposition calls — + // this loop books no gate key in ctx.gate_findings today (there is no + // 'test-quality' entry to retype) and this issue does not add one. Exits + // through this loop's OWN non-clean path (return { ok: true }, never + // { ok: false }) so nothing routes through the 'test-loop' stage key that + // a merge-block failure would use — a rebuttal-only round here can never + // block a merge, only get carried to the next reviewer as contested. No + // roll-up index needed (unlike the quality-cap/quality-rebuttal roll-ups + // above): runTestLoop runs once per issue, not once per task, so a single + // VERIFY_SKIPS.push() here can never produce more than one line. + const normRebuttals = normalizeRebuttals(qfix.rebutted, vFindings || []) + const rebuttalOnly = normRebuttals.length > 0 && (qfix.fixes_applied || []).length === 0 && (qfix.files_changed || []).length === 0 + if (rebuttalOnly) { + if (!ctx.contested) ctx.contested = [] + for (const r of normRebuttals) { + ctx.contested.push({ gate: 'test-quality', id: r.finding_id, summary: r.summary, evidence: r.evidence }) + } + pushDecision(ctx, 'Gate: findings contested, none applied', 'Test quality fix (iteration ' + iter + ') rebutted every finding and applied no fix — carried as contested, not resolved.') + VERIFY_SKIPS.push('#' + ctx.issue + ': test quality fix (iteration ' + iter + ') rebutted every finding and applied no fix — carried as contested, not resolved, for human review') + ctx.metrics.rebuttal_only_rounds++ + return { ok: true } + } + tallyTouches(ctx, qfix.files_changed) ctx.metrics.test_quality_fix_rounds++ } diff --git a/tests/quality-loop.test.js b/tests/quality-loop.test.js index f9b216f..9c6bea3 100644 --- a/tests/quality-loop.test.js +++ b/tests/quality-loop.test.js @@ -533,3 +533,284 @@ test('runQualityLoop: a loop that converges to "approved" pushes no VERIFY_SKIPS const verifySkips = harness.readGlobal(context, 'VERIFY_SKIPS') assert.strictEqual(verifySkips.length, 0) }) + +// ---- issue #167: the finding-hypothesis framing and the rebuttal-only exit ---- + +test('runQualityLoop: FINDING_HYPOTHESIS_ASK renders in the quality-fix prompt when the reviewer named structured findings', async function () { + const context = harness.boot() + context.__seed({ PROFILE: { simplify_globs: ['src/**'] } }) + + let reviewCalls = 0 + const scriptedAgent = harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':simplify-') !== -1) throw new Error('simplify must not run: filesChanged has no in-scope files') + if (label.indexOf(':quality-review-') !== -1) { + reviewCalls++ + if (reviewCalls === 1) return { result: 'changes_requested', comments: 'fix this', issues: ['x'], recommended_fix_agent: null, summary: 'needs work' } + return { result: 'approved', comments: '', issues: [], recommended_fix_agent: null, summary: 'looks good' } + } + if (label.indexOf(':quality-fix-') !== -1) return { status: 'success', summary: 'fixed', commit: 'deadbeef', files_changed: [] } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 68 }) + const result = await context.runQualityLoop(ctx, 'task-1', 'do the thing', ['docs/readme.md']) + + assert.strictEqual(result, 'approved') + const fixCall = scriptedAgent.calls.find(function (c) { return ((c.opts && c.opts.label) || '').indexOf(':quality-fix-') !== -1 }) + assert.ok(fixCall, 'fix stage must have run') + assert.ok(fixCall.prompt.includes('HYPOTHESIS the reviewer formed'), 'quality-fix prompt must render FINDING_HYPOTHESIS_ASK when findings are structured: ' + fixCall.prompt.slice(0, 2000)) +}) + +test('runQualityLoop: FINDING_HYPOTHESIS_ASK does NOT render in the quality-fix prompt when `issues` is omitted entirely (prose-fallback path stays byte-identical)', async function () { + const context = harness.boot() + context.__seed({ PROFILE: { simplify_globs: ['src/**'] } }) + + let reviewCalls = 0 + const scriptedAgent = harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':simplify-') !== -1) throw new Error('simplify must not run: filesChanged has no in-scope files') + if (label.indexOf(':quality-review-') !== -1) { + reviewCalls++ + if (reviewCalls === 1) return { result: 'changes_requested', comments: 'fix this', recommended_fix_agent: null, summary: 'needs work' } + return { result: 'approved', comments: '', issues: [], recommended_fix_agent: null, summary: 'looks good' } + } + if (label.indexOf(':quality-fix-') !== -1) return { status: 'success', summary: 'fixed', commit: 'deadbeef', files_changed: [] } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 69 }) + const result = await context.runQualityLoop(ctx, 'task-1', 'do the thing', ['docs/readme.md']) + + assert.strictEqual(result, 'approved') + const fixCall = scriptedAgent.calls.find(function (c) { return ((c.opts && c.opts.label) || '').indexOf(':quality-fix-') !== -1 }) + assert.ok(fixCall, 'fix stage must have run') + assert.ok(!fixCall.prompt.includes('HYPOTHESIS the reviewer formed'), 'quality-fix prompt must NOT render FINDING_HYPOTHESIS_ASK on the prose-fallback path: ' + fixCall.prompt.slice(0, 2000)) +}) + +test('runQualityLoop: a fix that rebuts every finding and applies none exits the loop as "degraded" with a "carried-unresolved" disposition, a contested entry, a decision, a VERIFY_SKIPS line, and the rebuttal counter — no cap line', async function () { + const context = harness.boot() + context.__seed({ PROFILE: { simplify_globs: ['src/**'] } }) + + harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':simplify-') !== -1) throw new Error('simplify must not run: filesChanged has no in-scope files') + if (label.indexOf(':quality-review-') !== -1) return { result: 'changes_requested', comments: '', issues: [{ severity: 'major', summary: 'bug' }], recommended_fix_agent: null, summary: 'needs work' } + if (label.indexOf(':quality-fix-') !== -1) { + return { + status: 'success', summary: 'disagree', commit: null, files_changed: [], fixes_applied: [], + rebutted: [{ finding_id: 'quality-task-1-i1-1', evidence: 'ran the reproducer, guard already covers this input' }], + } + } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 70 }) + const result = await context.runQualityLoop(ctx, 'task-1', 'do the thing', ['docs/readme.md']) + + assert.strictEqual(result, 'degraded') + // rebutted stays a SEPARATE flag from `degraded`: this is a dispute, not an + // agent failure, so the rolling degrade window/quality_degrades must be + // untouched. + assert.strictEqual(ctx.metrics.quality_degrades, 0) + assert.strictEqual(ctx.degrades[ctx.degrades.length - 1], false) + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 1) + assert.strictEqual(ctx.metrics.quality_iters, 1) + + // recordGateOutcome booked 're-litigated' (iteration 1 of 5) before the fix + // ran; the rebuttal-only exit must retype that same bucket to + // 'carried-unresolved', not double-book a fresh entry. + harness.assertVmEqual(ctx.gate_findings.quality, { + count: 1, + severity: { critical: 0, major: 1, minor: 0 }, + disposition: { 'carried-unresolved': 1 }, + }) + + assert.strictEqual(ctx.contested.length, 1) + assert.strictEqual(ctx.contested[0].id, 'quality-task-1-i1-1') + assert.strictEqual(ctx.contested[0].summary, 'bug') + assert.ok(ctx.contested[0].evidence.includes('ran the reproducer')) + + assert.ok(ctx.decisions.some(function (d) { return d.entry.includes('Gate: findings contested, none applied') }), 'expected a pushDecision entry for the contested-only round: ' + JSON.stringify(ctx.decisions)) + + const verifySkips = harness.readGlobal(context, 'VERIFY_SKIPS') + assert.strictEqual(verifySkips.length, 1, 'expected exactly one VERIFY_SKIPS line, no separate cap line: ' + JSON.stringify(verifySkips)) + assert.ok(verifySkips[0].includes('#70'), 'expected the line to name this issue: ' + verifySkips[0]) + assert.ok(verifySkips[0].includes('task 1'), 'expected the line to name the scope: ' + verifySkips[0]) + assert.ok(!verifySkips[0].includes('capped at'), 'a rebuttal-only exit must not read as a cap exhaustion: ' + verifySkips[0]) +}) + +test('runQualityLoop: a round that rebuts one finding but applies a real fix for another proceeds normally — not treated as rebuttal-only', async function () { + const context = harness.boot() + context.__seed({ PROFILE: { simplify_globs: ['src/**'] } }) + + let reviewCalls = 0 + harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':simplify-') !== -1) throw new Error('simplify must not run: filesChanged has no in-scope files') + if (label.indexOf(':quality-review-') !== -1) { + reviewCalls++ + if (reviewCalls === 1) { + return { + result: 'changes_requested', comments: '', recommended_fix_agent: null, summary: 'needs work', + issues: [{ severity: 'major', summary: 'real bug' }, { severity: 'minor', summary: 'not actually a bug' }], + } + } + return { result: 'approved', comments: '', issues: [], recommended_fix_agent: null, summary: 'looks good' } + } + if (label.indexOf(':quality-fix-') !== -1) { + return { + // files_changed stays OUTSIDE simplify_globs (src/**) deliberately — + // an in-scope touch would flip runSimplify on for iteration 2 and + // this scenario's ':simplify-' branch is scripted to throw, to prove + // (per the sibling tests above) that simplify never runs on an + // out-of-scope change. + status: 'success', summary: 'fixed one, rebutted the other', commit: 'deadbeef', + files_changed: ['docs/other.md'], fixes_applied: ['[quality-task-1-i1-1] fixed the real bug'], + rebutted: [{ finding_id: 'quality-task-1-i1-2', evidence: 'checked — not a bug' }], + } + } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 71 }) + const result = await context.runQualityLoop(ctx, 'task-1', 'do the thing', ['docs/readme.md']) + + assert.strictEqual(result, 'approved') + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 0) + assert.strictEqual(ctx.touch_counts['docs/other.md'], 1) + assert.ok(!ctx.contested || ctx.contested.length === 0, 'a partial rebuttal must not be recorded as contested: ' + JSON.stringify(ctx.contested)) + const verifySkips = harness.readGlobal(context, 'VERIFY_SKIPS') + assert.strictEqual(verifySkips.length, 0) +}) + +test('runQualityLoop: a fix response that omits `rebutted` entirely behaves exactly as today (no rebuttal-only exit)', async function () { + const context = harness.boot() + context.__seed({ PROFILE: { simplify_globs: ['src/**'] } }) + + let reviewCalls = 0 + harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':simplify-') !== -1) throw new Error('simplify must not run: filesChanged has no in-scope files') + if (label.indexOf(':quality-review-') !== -1) { + reviewCalls++ + if (reviewCalls === 1) return { result: 'changes_requested', comments: '', issues: [{ severity: 'major', summary: 'bug' }], recommended_fix_agent: null, summary: 'needs work' } + return { result: 'approved', comments: '', issues: [], recommended_fix_agent: null, summary: 'looks good' } + } + // `rebutted` deliberately OMITTED — the pre-#167 shape every fixer used. + // files_changed stays outside simplify_globs so iteration 2 does not + // flip runSimplify on (see the sibling test above for why that matters). + if (label.indexOf(':quality-fix-') !== -1) return { status: 'success', summary: 'fixed', commit: 'deadbeef', files_changed: ['docs/other.md'] } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 72 }) + const result = await context.runQualityLoop(ctx, 'task-1', 'do the thing', ['docs/readme.md']) + + assert.strictEqual(result, 'approved') + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 0) + assert.ok(!ctx.contested || ctx.contested.length === 0) +}) + +test('runQualityLoop: a rebutted entry with blank evidence is not a rebuttal — normalizeRebuttals drops it, so the round is NOT treated as rebuttal-only', async function () { + const context = harness.boot() + context.__seed({ PROFILE: { simplify_globs: ['src/**'] } }) + + let reviewCalls = 0 + harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':simplify-') !== -1) throw new Error('simplify must not run: filesChanged has no in-scope files') + if (label.indexOf(':quality-review-') !== -1) { + reviewCalls++ + if (reviewCalls === 1) return { result: 'changes_requested', comments: '', issues: [{ severity: 'major', summary: 'bug' }], recommended_fix_agent: null, summary: 'needs work' } + return { result: 'approved', comments: '', issues: [], recommended_fix_agent: null, summary: 'looks good' } + } + if (label.indexOf(':quality-fix-') !== -1) { + return { + status: 'success', summary: 'claims disagreement but gives no evidence', commit: null, files_changed: [], fixes_applied: [], + rebutted: [{ finding_id: 'quality-task-1-i1-1', evidence: '' }], + } + } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 73 }) + const result = await context.runQualityLoop(ctx, 'task-1', 'do the thing', ['docs/readme.md']) + + assert.strictEqual(result, 'approved') + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 0) + assert.ok(!ctx.contested || ctx.contested.length === 0) +}) + +test('runQualityLoop: `fixes_applied` omitted but `files_changed` non-empty is not a rebuttal-only round', async function () { + const context = harness.boot() + context.__seed({ PROFILE: { simplify_globs: ['src/**'] } }) + + let reviewCalls = 0 + harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':simplify-') !== -1) throw new Error('simplify must not run: filesChanged has no in-scope files') + if (label.indexOf(':quality-review-') !== -1) { + reviewCalls++ + if (reviewCalls === 1) return { result: 'changes_requested', comments: '', issues: [{ severity: 'major', summary: 'bug' }], recommended_fix_agent: null, summary: 'needs work' } + return { result: 'approved', comments: '', issues: [], recommended_fix_agent: null, summary: 'looks good' } + } + // `fixes_applied` OMITTED, but files_changed is non-empty — a fixer that + // changed code without listing it in fixes_applied is still not a + // rebuttal-only round: the (fix.files_changed||[]).length === 0 arm of + // the predicate must fail this round. + if (label.indexOf(':quality-fix-') !== -1) { + return { + // files_changed stays outside simplify_globs so iteration 2 does not + // flip runSimplify on (see the sibling tests above). + status: 'success', summary: 'fixed it, forgot to list it', commit: 'deadbeef', files_changed: ['docs/other.md'], + rebutted: [{ finding_id: 'quality-task-1-i1-1', evidence: 'thought this was unrelated' }], + } + } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 74 }) + const result = await context.runQualityLoop(ctx, 'task-1', 'do the thing', ['docs/readme.md']) + + assert.strictEqual(result, 'approved') + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 0) + assert.ok(!ctx.contested || ctx.contested.length === 0) +}) + +test('runQualityLoop: two rebuttal-only scopes on the same ctx (a task, then a PR-fix round) roll up to exactly one VERIFY_SKIPS entry naming both, and the counter reflects both rounds', async function () { + const context = harness.boot() + context.__seed({ PROFILE: { simplify_globs: ['src/**'] } }) + + harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':simplify-') !== -1) throw new Error('simplify must not run: filesChanged has no in-scope files') + if (label.indexOf(':quality-review-') !== -1) return { result: 'changes_requested', comments: '', issues: [{ severity: 'major', summary: 'bug' }], recommended_fix_agent: null, summary: 'needs work' } + if (label.indexOf(':quality-fix-task-1-') !== -1) { + return { + status: 'success', summary: 'disagree', commit: null, files_changed: [], fixes_applied: [], + rebutted: [{ finding_id: 'quality-task-1-i1-1', evidence: 'checked, no bug here' }], + } + } + if (label.indexOf(':quality-fix-pr-fix-i1-') !== -1) { + return { + status: 'success', summary: 'disagree', commit: null, files_changed: [], fixes_applied: [], + rebutted: [{ finding_id: 'quality-pr-fix-i1-i1-1', evidence: 'checked, no bug here' }], + } + } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 75 }) + const firstResult = await context.runQualityLoop(ctx, 'task-1', 'do the thing', ['docs/readme.md']) + const secondResult = await context.runQualityLoop(ctx, 'pr-fix-i1', 'fix pr feedback', ['docs/readme.md']) + + assert.strictEqual(firstResult, 'degraded') + assert.strictEqual(secondResult, 'degraded') + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 2) + + const verifySkips = harness.readGlobal(context, 'VERIFY_SKIPS') + assert.strictEqual(verifySkips.length, 1, 'a second rebuttal-only scope on the same ctx must rewrite the existing entry in place, not append a second one: ' + JSON.stringify(verifySkips)) + assert.ok(verifySkips[0].includes('task 1'), 'expected the rolled-up entry to still name the first scope: ' + verifySkips[0]) + assert.ok(verifySkips[0].includes('PR-fix round 1'), 'expected the rolled-up entry to also name the second scope: ' + verifySkips[0]) +}) diff --git a/tests/test-loop.test.js b/tests/test-loop.test.js index 563a18f..8b4505e 100644 --- a/tests/test-loop.test.js +++ b/tests/test-loop.test.js @@ -220,3 +220,152 @@ test('runTestLoop: an existing issues: ["x"] fixture still runs test-quality-fix assert.ok(fixCall, 'test-quality-fix stage must have run for an issues: ["x"] fixture') assert.ok(/- \[test-i1-1\] \[unspecified\] x -> /.test(fixCall.prompt), 'fix prompt must render the id-prefixed finding line: ' + fixCall.prompt.slice(0, 2000)) }) + +// ---- issue #167: the finding-hypothesis framing, the rebuttal-only exit at +// the evaluator-fed test-quality-fix stage, and byte-identity proof that the +// ORACLE-fed test-fix stage (ground truth, not a hypothesis) never reads +// `rebutted` at all ---- + +test('runTestLoop: FINDING_HYPOTHESIS_ASK renders in the test-quality-fix prompt when the validator named structured findings', async function () { + const context = harness.boot() + context.__seed({ PROFILE: {}, TEST_CMD: 'npm test' }) + + let validateCalls = 0 + const scriptedAgent = harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':test-run-') !== -1) return { result: 'passed', summary: 'all green', total_tests: 3, passed_tests: 3, failed_tests: 0, failures: [] } + if (label.indexOf(':test-validate-') !== -1) { + validateCalls++ + if (validateCalls === 1) return { result: 'changes_requested', comments: 'hollow assertion', issues: ['x'], summary: 'needs work' } + return { result: 'approved', comments: '', issues: [], summary: 'covered now' } + } + if (label.indexOf(':test-quality-fix-') !== -1) return { status: 'success', summary: 'strengthened tests', commit: 'deadbeef', files_changed: [] } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 76 }) + const result = await context.runTestLoop(ctx) + + assert.strictEqual(result.ok, true) + const fixCall = scriptedAgent.calls.find(function (c) { return ((c.opts && c.opts.label) || '').indexOf(':test-quality-fix-') !== -1 }) + assert.ok(fixCall, 'test-quality-fix stage must have run') + assert.ok(fixCall.prompt.includes('HYPOTHESIS the reviewer formed'), 'test-quality-fix prompt must render FINDING_HYPOTHESIS_ASK when findings are structured: ' + fixCall.prompt.slice(0, 2000)) +}) + +test('runTestLoop: FINDING_HYPOTHESIS_ASK does NOT render in the test-quality-fix prompt when `issues` is omitted entirely (prose-fallback path stays byte-identical)', async function () { + const context = harness.boot() + context.__seed({ PROFILE: {}, TEST_CMD: 'npm test' }) + + let validateCalls = 0 + const scriptedAgent = harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':test-run-') !== -1) return { result: 'passed', summary: 'all green', total_tests: 3, passed_tests: 3, failed_tests: 0, failures: [] } + if (label.indexOf(':test-validate-') !== -1) { + validateCalls++ + if (validateCalls === 1) return { result: 'changes_requested', comments: 'hollow assertion', summary: 'needs work' } + return { result: 'approved', comments: '', issues: [], summary: 'covered now' } + } + if (label.indexOf(':test-quality-fix-') !== -1) return { status: 'success', summary: 'strengthened tests', commit: 'deadbeef', files_changed: [] } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 77 }) + const result = await context.runTestLoop(ctx) + + assert.strictEqual(result.ok, true) + const fixCall = scriptedAgent.calls.find(function (c) { return ((c.opts && c.opts.label) || '').indexOf(':test-quality-fix-') !== -1 }) + assert.ok(fixCall, 'test-quality-fix stage must have run') + assert.ok(!fixCall.prompt.includes('HYPOTHESIS the reviewer formed'), 'test-quality-fix prompt must NOT render FINDING_HYPOTHESIS_ASK on the prose-fallback path: ' + fixCall.prompt.slice(0, 2000)) +}) + +test('runTestLoop: the test-fix prompt (oracle-fed — a failing test is ground truth, not a hypothesis) does NOT contain FINDING_HYPOTHESIS_ASK and still carries its own anti-rebuttal guard verbatim', async function () { + const context = harness.boot() + context.__seed({ PROFILE: {}, TEST_CMD: 'npm test' }) + + const scriptedAgent = harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':test-run-') !== -1) return { result: 'failed', summary: 'failing on purpose', total_tests: 1, passed_tests: 0, failed_tests: 1, failures: [{ test: 'x', message: 'always fails' }] } + if (label.indexOf(':test-fix-') !== -1) return { status: 'success', summary: 'attempted a fix', commit: 'deadbeef', files_changed: ['tests/foo.test.js'] } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 78 }) + await context.runTestLoop(ctx) + + const fixCall = scriptedAgent.calls.find(function (c) { return ((c.opts && c.opts.label) || '').indexOf(':test-fix-') !== -1 }) + assert.ok(fixCall, 'test-fix stage must have run') + assert.ok(!fixCall.prompt.includes('HYPOTHESIS the reviewer formed'), 'test-fix prompt must never carry FINDING_HYPOTHESIS_ASK — a failing test is ground truth: ' + fixCall.prompt.slice(0, 2000)) + assert.ok(fixCall.prompt.includes('Fix the real defect — do NOT delete or weaken assertions just to make the failure disappear.'), 'test-fix prompt must still carry its own anti-rebuttal guard verbatim: ' + fixCall.prompt.slice(0, 2000)) +}) + +test('runTestLoop: a test-quality-fix that rebuts every finding and applies none exits the loop with ok:true (never ok:false), a contested entry, a decision, a VERIFY_SKIPS line, and the rebuttal counter', async function () { + const context = harness.boot() + context.__seed({ PROFILE: {}, TEST_CMD: 'npm test' }) + + harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':test-run-') !== -1) return { result: 'passed', summary: 'all green', total_tests: 3, passed_tests: 3, failed_tests: 0, failures: [] } + if (label.indexOf(':test-validate-') !== -1) return { result: 'changes_requested', comments: '', issues: [{ severity: 'minor', summary: 'hollow assertion' }], summary: 'needs work' } + if (label.indexOf(':test-quality-fix-') !== -1) { + return { + status: 'success', summary: 'disagree', commit: null, files_changed: [], fixes_applied: [], + rebutted: [{ finding_id: 'test-i1-1', evidence: 'ran it — assertion already covers the edge case' }], + } + } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 79 }) + const result = await context.runTestLoop(ctx) + + // Never { ok: false } — a rebuttal-only round here must not be able to + // route through the 'test-loop' merge-block stage key. It ends this loop's + // OWN iteration cleanly, same as the empty-findings exit above. + assert.strictEqual(result.ok, true) + assert.strictEqual(result.error, undefined) + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 1) + assert.strictEqual(ctx.metrics.test_quality_fix_rounds, 0) + + assert.strictEqual(ctx.contested.length, 1) + assert.strictEqual(ctx.contested[0].id, 'test-i1-1') + assert.strictEqual(ctx.contested[0].summary, 'hollow assertion') + assert.ok(ctx.contested[0].evidence.includes('assertion already covers')) + + assert.ok(ctx.decisions.some(function (d) { return d.entry.includes('Gate: findings contested, none applied') }), 'expected a pushDecision entry for the contested-only round: ' + JSON.stringify(ctx.decisions)) + + const verifySkips = harness.readGlobal(context, 'VERIFY_SKIPS') + assert.strictEqual(verifySkips.length, 1) + assert.ok(verifySkips[0].includes('#79'), 'expected the line to name this issue: ' + verifySkips[0]) +}) + +test('runTestLoop: a test-fix response carrying `rebutted` with empty fixes_applied/files_changed drives the loop exactly as today — the field is unread at the ORACLE-fed test-fix stage', async function () { + const context = harness.boot() + context.__seed({ PROFILE: {}, TEST_CMD: 'npm test' }) + + let runCalls = 0 + harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':test-run-') !== -1) { + runCalls++ + if (runCalls === 1) return { result: 'failed', summary: 'failing', total_tests: 1, passed_tests: 0, failed_tests: 1, failures: [{ test: 'x', message: 'always fails' }] } + return { result: 'passed', summary: 'all green', total_tests: 1, passed_tests: 1, failed_tests: 0, failures: [] } + } + if (label.indexOf(':test-fix-') !== -1) { + // `rebutted` present with empty fixes_applied/files_changed — the exact + // shape that would be rebuttal-only at an EVALUATOR-fed gate. test-fix + // has no such gating: the loop must simply `continue` to the next + // test-run, same as any other live, non-error fix response. + return { status: 'success', summary: 'attempted a fix', commit: null, files_changed: [], fixes_applied: [], rebutted: [{ finding_id: 'x', evidence: 'y' }] } + } + if (label.indexOf(':test-validate-') !== -1) return { result: 'approved', comments: '', issues: [], summary: 'looks good' } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 80 }) + const result = await context.runTestLoop(ctx) + + assert.strictEqual(result.ok, true) + assert.strictEqual(ctx.metrics.test_iters, 2) + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 0) + assert.ok(!ctx.contested || ctx.contested.length === 0, '`rebutted` on a test-fix response must never populate ctx.contested: ' + JSON.stringify(ctx.contested)) +}) diff --git a/workflows/ticketmill.js b/workflows/ticketmill.js index 3c78a8a..2f71d0d 100644 --- a/workflows/ticketmill.js +++ b/workflows/ticketmill.js @@ -3001,7 +3001,14 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { let runSimplify = !Array.isArray(filesChanged) || filesChanged.length === 0 || filesChanged.some(inScope) let approved = false let degraded = false - for (let iter = 1; !approved && !degraded && iter <= MAX_QUALITY_ITERATIONS; iter++) { + // rebutted (issue #167): a third, separate loop-exit flag from `degraded` — + // a fix that rebuts every finding and applies none is NOT an agent failure + // (degraded stays false, so the rolling degrade window/quality_degrades + // below are untouched), but it also cannot approve itself, so the loop must + // still stop rather than spend its remaining iterations re-litigating a + // dispute only a human/reviewer can adjudicate. + let rebutted = false + for (let iter = 1; !approved && !degraded && !rebutted && iter <= MAX_QUALITY_ITERATIONS; iter++) { if (STOP.tripped) return 'halted' ctx.metrics.quality_iters++ if (iter === 1) ctx.metrics.quality_scopes++ @@ -3066,7 +3073,8 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { break } - recordGateOutcome(ctx, 'quality', revFindings || [], iter === MAX_QUALITY_ITERATIONS ? 'carried-unresolved' : 're-litigated') + const bookedDisposition = iter === MAX_QUALITY_ITERATIONS ? 'carried-unresolved' : 're-litigated' + recordGateOutcome(ctx, 'quality', revFindings || [], bookedDisposition) const fixAgent = pickFixAgent(rev.recommended_fix_agent, null) const fix = await stage(ctx, 'quality-fix-' + prefix + '-i' + iter, [ implementerBlock(fixAgent), @@ -3080,6 +3088,7 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { 'After committing, post an issue comment "## Quality Fix (' + stepLabel + ', iteration ' + iter + ')" with the', 'commit SHA and the fixes applied in 2-4 lines (gh issue comment ' + ctx.issue + ' --repo ' + REPO + ').', fixesAppliedIdAsk('[quality-task-1-i1-2] tightened the null guard'), + revFindings !== null ? FINDING_HYPOTHESIS_ASK : '', COMMIT_SHA_ASK, bwFeedback(ctx), HANDOFF_ASK, @@ -3088,6 +3097,42 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { if (!fix || fix.status === 'error') { degraded = true; log('#' + ctx.issue + ' quality ' + prefix + ' i' + iter + ': fix degraded'); break } collectNotes(ctx, 'quality-fix', fix) collectPostedCommit(ctx, 'quality-fix-' + prefix + '-i' + iter, fix) + + // rebuttal-only exit (issue #167): a fix that rebutted every finding it was + // shown and applied none is a DISPUTE, not a resolution — see the + // rebuttalOnly comment above runTestLoop's mirror of this block for the + // full contract this predicate enforces. Evaluated AFTER collectNotes/ + // collectPostedCommit above (so handoff notes and the posted-commit ledger + // still see this round) but BEFORE tallyTouches just below, which is a + // verified no-op here anyway since a true rebuttalOnly round always has an + // empty files_changed by construction. + const normRebuttals = normalizeRebuttals(fix.rebutted, revFindings || []) + const rebuttalOnly = normRebuttals.length > 0 && (fix.fixes_applied || []).length === 0 && (fix.files_changed || []).length === 0 + if (rebuttalOnly) { + rebutted = true + retypeGateDisposition(ctx, 'quality', bookedDisposition, 'carried-unresolved') + if (!ctx.contested) ctx.contested = [] + for (const r of normRebuttals) { + ctx.contested.push({ gate: 'quality-' + prefix, id: r.finding_id, summary: r.summary, evidence: r.evidence }) + } + pushDecision(ctx, 'Gate: findings contested, none applied', 'Quality fix (' + stepLabel + ', iteration ' + iter + ') rebutted every finding and applied no fix — carried as contested, not resolved.') + ctx.metrics.rebuttal_only_rounds++ + // Rolled up to exactly ONE VERIFY_SKIPS line per issue, mirroring the + // quality-cap roll-up just below (runQualityLoop runs once per task plus + // once per PR-fix round, so without this a chatty issue would print one + // line per rebuttal-only round). + if (!ctx.quality_rebuttals) ctx.quality_rebuttals = [] + ctx.quality_rebuttals.push(stepLabel) + const rebutMsg = '#' + ctx.issue + ': quality gate rebuttal-only, no fix applied (' + ctx.quality_rebuttals.join(', ') + ') — findings contested, carried for human review' + if (typeof ctx.quality_rebuttal_skip_index === 'number' && ctx.quality_rebuttal_skip_index < VERIFY_SKIPS.length) { + VERIFY_SKIPS[ctx.quality_rebuttal_skip_index] = rebutMsg + } else { + ctx.quality_rebuttal_skip_index = VERIFY_SKIPS.length + VERIFY_SKIPS.push(rebutMsg) + } + break + } + tallyTouches(ctx, fix.files_changed) if (!runSimplify && (fix.files_changed || []).some(inScope)) runSimplify = true } @@ -3110,7 +3155,12 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { // exit via the degrade-window halt just below, which both callers // (reviewAndMerge, the per-task loop) turn into a hard fail() — so this // line must not claim anything merged on a capped-then-halted issue. - if (!approved && !degraded) { + // Gated to !rebutted too (issue #167): a rebuttal-only exit already pushed + // its own rolled-up VERIFY_SKIPS line above and broke the loop before + // reaching MAX_QUALITY_ITERATIONS — it must not ALSO be counted as a cap + // exhaustion, which would misreport a contested-findings round as the loop + // having simply run out of iterations. + if (!approved && !degraded && !rebutted) { if (!ctx.quality_caps) ctx.quality_caps = [] ctx.quality_caps.push(stepLabel) const capMsg = '#' + ctx.issue + ': quality gate capped at ' + MAX_QUALITY_ITERATIONS + ' iterations without a clean review (' + ctx.quality_caps.join(', ') + ')' @@ -3579,6 +3629,7 @@ async function runTestLoop(ctx, forced) { 'After committing, post an issue comment "## Test Quality Fix (iteration ' + iter + ')" with the commit SHA and', 'what was added/strengthened (gh issue comment ' + ctx.issue + ' --repo ' + REPO + ').', fixesAppliedIdAsk('[test-i1-2] added a missing edge-case test'), + vFindings !== null ? FINDING_HYPOTHESIS_ASK : '', COMMIT_SHA_ASK, HANDOFF_ASK, 'Add missing assertions, remove TODOs, add edge-case tests, etc. Commit. Return status, commit, files_changed, fixes_applied, summary.', @@ -3586,6 +3637,31 @@ async function runTestLoop(ctx, forced) { if (!qfix || qfix.status === 'error') return { ok: false, error: 'test-quality-fix stage failed — halting test loop' } collectNotes(ctx, 'test-quality-fix', qfix) collectPostedCommit(ctx, 'test-quality-fix-i' + iter, qfix) + + // rebuttal-only exit (issue #167): mirrors runQualityLoop's rebuttalOnly + // block above, minus the recordGateOutcome/retypeGateDisposition calls — + // this loop books no gate key in ctx.gate_findings today (there is no + // 'test-quality' entry to retype) and this issue does not add one. Exits + // through this loop's OWN non-clean path (return { ok: true }, never + // { ok: false }) so nothing routes through the 'test-loop' stage key that + // a merge-block failure would use — a rebuttal-only round here can never + // block a merge, only get carried to the next reviewer as contested. No + // roll-up index needed (unlike the quality-cap/quality-rebuttal roll-ups + // above): runTestLoop runs once per issue, not once per task, so a single + // VERIFY_SKIPS.push() here can never produce more than one line. + const normRebuttals = normalizeRebuttals(qfix.rebutted, vFindings || []) + const rebuttalOnly = normRebuttals.length > 0 && (qfix.fixes_applied || []).length === 0 && (qfix.files_changed || []).length === 0 + if (rebuttalOnly) { + if (!ctx.contested) ctx.contested = [] + for (const r of normRebuttals) { + ctx.contested.push({ gate: 'test-quality', id: r.finding_id, summary: r.summary, evidence: r.evidence }) + } + pushDecision(ctx, 'Gate: findings contested, none applied', 'Test quality fix (iteration ' + iter + ') rebutted every finding and applied no fix — carried as contested, not resolved.') + VERIFY_SKIPS.push('#' + ctx.issue + ': test quality fix (iteration ' + iter + ') rebutted every finding and applied no fix — carried as contested, not resolved, for human review') + ctx.metrics.rebuttal_only_rounds++ + return { ok: true } + } + tallyTouches(ctx, qfix.files_changed) ctx.metrics.test_quality_fix_rounds++ } From 7bb1fe0485c47be047c089265f88e0b7b9893256 Mon Sep 17 00:00:00 2001 From: aaddrick Date: Tue, 28 Jul 2026 15:31:27 -0400 Subject: [PATCH 3/6] feat(gate-findings): wire the pr-review merge gate's rebuttal-only continue Renders FINDING_HYPOTHESIS_ASK in the pr-fix prompt when either reviewer returned structured findings, and evaluates the rebuttalOnly predicate over the union of both reviewers' rendered finding sets. Unlike the quality and test loops (which exit their gate on a rebuttal-only round), pr-review CONTINUES: it retypes the iteration's disposition to carried-unresolved, carries the dispute into ctx.contested so the next iteration's reviewers see it, records one un-rolled Verification Gaps line, and moves on to the next review iteration instead of ending the gate. A per-call counter permits exactly one such round per issue; a second sets haltReason and falls through to the existing needs_human path, leaving the PR open. The `continue` precedes runQualityLoop so a rebuttal-only round (empty files_changed) never burns a quality gate against an untouched tree. Issue #167 --- .claude/workflows/ticketmill.js | 57 +++++++++ tests/browser-check.test.js | 27 +++++ tests/pr-review-gate.test.js | 199 ++++++++++++++++++++++++++++++++ workflows/ticketmill.js | 57 +++++++++ 4 files changed, 340 insertions(+) diff --git a/.claude/workflows/ticketmill.js b/.claude/workflows/ticketmill.js index 2f71d0d..8ee5b85 100644 --- a/.claude/workflows/ticketmill.js +++ b/.claude/workflows/ticketmill.js @@ -4927,6 +4927,10 @@ async function implementIssue(ctx) { async function reviewAndMerge(ctx) { let approved = false let haltReason = null + // rebuttalRoundsUsed (issue #167): per-reviewAndMerge-call counter permitting + // exactly ONE rebuttal-only pr-fix round before this gate stops giving a + // disputing fixer another iteration — see the rebuttalOnly block below. + let rebuttalRoundsUsed = 0 for (let iter = 1; iter <= MAX_PR_REVIEW_ITERATIONS && !approved; iter++) { if (STOP.tripped) return fail(ctx, 'halted', 'pr-review', 'stopped: ' + STOP.reason) ctx.metrics.pr_review_iters = iter @@ -5046,6 +5050,7 @@ async function reviewAndMerge(ctx) { 'After pushing, post a PR comment "## PR Review Fix (iteration ' + iter + ')" with the commit SHA and the fixes', 'applied in 2-4 lines (gh pr comment ' + ctx.pr + ' --repo ' + REPO + ').', fixesAppliedIdAsk('[code-i1-2] tightened the null guard'), + (specFindings !== null || codeFindings !== null) ? FINDING_HYPOTHESIS_ASK : '', COMMIT_SHA_ASK, bwFeedback(ctx), HANDOFF_ASK, @@ -5056,6 +5061,58 @@ async function reviewAndMerge(ctx) { pushDecision(ctx, 'PR Review Fix (i' + iter + ')', fix.summary || 'fixes applied') collectNotes(ctx, 'pr-fix', fix) collectPostedCommit(ctx, 'pr-fix-i' + iter, fix) + + // rebuttal-only exit (issue #167): mirrors runQualityLoop's/runTestLoop's + // rebuttalOnly block, evaluated over the UNION of both reviewers' rendered + // finding sets (the fixer saw both blocks in one prompt above) — see + // normalizeRebuttals' doc comment for why `findings` must be exactly what + // was rendered. Evaluated AFTER pushDecision/collectNotes/ + // collectPostedCommit above (so the fixer's summary, handoff notes, and + // the posted-commit ledger all still see this round) but BEFORE + // tallyTouches/runQualityLoop below: tallyTouches is a verified no-op on a + // true rebuttal-only round's empty files_changed, but runQualityLoop is + // NOT — runSimplify fails open on an empty filesChanged array and would + // burn a full quality gate against an untouched tree, so `continue` MUST + // precede that call, not follow it. + // + // Unlike the quality/test loops, this gate does NOT push a second + // pushDecision here — the one above already fires for every non-error fix + // and renders the fixer's summary; a rebuttal-only round has nothing + // further to narrate that recordGateOutcome/retypeGateDisposition and the + // contested-block push below don't already carry into the next iteration. + // + // No id-equality halt: REVIEW_SCHEMA ids are `source + '-' + (i+1)` with + // the iteration baked into `source` (see spec/code review call sites + // above), so a second round's ids are disjoint from the first round's by + // construction — an id-equality check here would be permanently dead code. + // Instead, rebuttalRoundsUsed (declared above the loop) permits exactly + // ONE rebuttal-only round per reviewAndMerge() call: a second one sets + // haltReason and breaks into the existing `if (!approved)` needs_human + // path below, same shape as the bothNothingToFix/capReached breaks above. + const prFixFindings = (specFindings || []).concat(codeFindings || []) + const normRebuttals = normalizeRebuttals(fix.rebutted, prFixFindings) + const rebuttalOnly = normRebuttals.length > 0 && (fix.fixes_applied || []).length === 0 && (fix.files_changed || []).length === 0 + if (rebuttalOnly) { + if (rebuttalRoundsUsed >= 1) { + haltReason = 'PR #' + ctx.pr + ' review iteration ' + iter + ': a second rebuttal-only PR fix round rebutted every finding and applied no fix — left open for human review' + break + } + rebuttalRoundsUsed++ + retypeGateDisposition(ctx, 'pr-review', prReviewDisposition, 'carried-unresolved') + if (!ctx.contested) ctx.contested = [] + for (const r of normRebuttals) { + ctx.contested.push({ gate: 'pr-review', id: r.finding_id, summary: r.summary, evidence: r.evidence }) + } + ctx.metrics.rebuttal_only_rounds++ + // A single un-rolled line: bounded at two per issue by construction + // (capReached breaks above the fix stage, and pr-fix only runs at + // iterations 1 and 2 of MAX_PR_REVIEW_ITERATIONS=3), so unlike the + // quality loop's rolled-up VERIFY_SKIPS index this never needs + // rewriting in place — it is never retracted. + VERIFY_SKIPS.push('#' + ctx.issue + ': PR review fix (iteration ' + iter + ') rebutted every finding and applied no fix — carried as contested, not resolved, for human review') + continue + } + tallyTouches(ctx, fix.files_changed) const q = await runQualityLoop(ctx, 'pr-fix-i' + iter, 'post-review fixes for PR #' + ctx.pr, fix.files_changed) diff --git a/tests/browser-check.test.js b/tests/browser-check.test.js index aad5173..087aab8 100644 --- a/tests/browser-check.test.js +++ b/tests/browser-check.test.js @@ -140,6 +140,33 @@ test('runBrowserCheck: fails after MAX_BROWSER_ITERATIONS of persistent failures assert.strictEqual(ctx.metrics.browser_iters, maxIters) }) +// ---- issue #167 scope pin: browser-fix is ORACLE-fed (a failing browser +// scenario is ground truth), not evaluator-fed like quality-fix/test-quality- +// fix/pr-fix — FINDING_HYPOTHESIS_ASK must never render here, and the +// existing anti-rebuttal guard it already carries must stay verbatim. ---- +test('runBrowserCheck: the browser-fix prompt does NOT render FINDING_HYPOTHESIS_ASK and keeps its anti-rebuttal guard verbatim', async function () { + const context = harness.boot() + harness.readGlobal(context, "BROWSER = { ui_globs: ['src/**'] }") + + const scriptedAgent = harness.installScriptedAgent(context, function (prompt, opts) { + const label = (opts && opts.label) || '' + if (label.indexOf(':ui-probe-') !== -1) return { ui_files: ['src/App.jsx'] } + if (label.indexOf(':browser-cleanup-') !== -1) return { posted: true } + if (label.indexOf(':browser-fix-') !== -1) return { status: 'success', summary: 'attempted a fix', commit: 'deadbeef', files_changed: ['src/App.jsx'] } + if (label.indexOf(':browser-') !== -1) return { result: 'failed', summary: 'still broken', scenarios: [], failures: ['button does nothing'] } + throw new Error('unexpected stage label in this scenario: ' + label) + }) + + const ctx = harness.makeCtx({ issue: 67 }) + await context.runBrowserCheck(ctx, 'implement') + + const fixCall = scriptedAgent.calls.find(function (c) { return ((c.opts && c.opts.label) || '').indexOf(':browser-fix-') !== -1 }) + assert.ok(fixCall, 'browser-fix must have run') + const prompt = String(fixCall.prompt) + assert.ok(!prompt.includes('HYPOTHESIS the reviewer formed'), 'browser-fix is oracle-fed — it must NOT render FINDING_HYPOTHESIS_ASK: ' + prompt.slice(0, 2000)) + assert.ok(prompt.includes('Fix the real defect — do NOT hide the symptom (e.g. removing the interaction that fails).'), 'browser-fix must still carry its anti-rebuttal guard verbatim: ' + prompt.slice(0, 2000)) +}) + test('runBrowserCheck: cleanup still runs, under the lock, when the browser verifier itself dies', async function () { const context = harness.boot() harness.readGlobal(context, "BROWSER = { ui_globs: ['src/**'] }") diff --git a/tests/pr-review-gate.test.js b/tests/pr-review-gate.test.js index 7bf41b1..14f4547 100644 --- a/tests/pr-review-gate.test.js +++ b/tests/pr-review-gate.test.js @@ -447,3 +447,202 @@ test('reviewAndMerge(): a typed mixed-severity issues array makes gate_findings[ assert.strictEqual(g.count, 3) assert.deepStrictEqual(JSON.parse(JSON.stringify(g.severity)), { critical: 1, major: 1, minor: 1 }) }) + +// ---- issue #167: the finding-hypothesis framing and the rebuttal-only exit +// at the pr-review merge gate — the ONE evaluator-fed gate that CONTINUES +// (rather than exiting the loop like quality/test) on a rebuttal-only round, +// because pr-review is a multi-iteration loop with another reviewer pair +// waiting downstream, not a terminal fix gate. ---- + +const REBUTTAL_ONLY_FIX = { + status: 'success', commit: null, files_changed: [], fixes_applied: [], summary: 'disagree, guard already exists', + rebutted: [{ finding_id: 'code-i1-1', evidence: 'ran the reproducer at src/foo.js:12 — the guard already covers this input' }], +} +const CODE_FINDING = { result: 'changes_requested', comments: 'fix the guard', issues: [{ severity: 'major', summary: 'missing null check', recommendation: 'add a guard' }], recommended_fix_agent: null, summary: 'one issue' } + +test('reviewAndMerge(): a rebuttal-only pr-fix round at i1 continues into i2 (not needs_human), retypes the disposition to "carried-unresolved", records a contested entry, skips runQualityLoop, and a clean i2 approves and merges with the Verification Gaps line still present', async function () { + const context = harness.boot() + seedReviewFlow(context) + + installScriptedResponder(context, { + 'spec-review-i1': APPROVED_REVIEW, + 'code-review-i1': CODE_FINDING, + 'gate-state-pr-review-i1': GATE_STATE_POSTED, + 'pr-fix-i1': REBUTTAL_ONLY_FIX, + // simplify-pr-fix-i1-i1 / quality-review-pr-fix-i1-i1 deliberately + // unscripted below — proving runQualityLoop never runs on a rebuttal-only + // round (the `continue` must precede that call). + 'spec-review-i2': APPROVED_REVIEW, + 'code-review-i2': APPROVED_REVIEW, + 'gate-state-pr-review-i2': GATE_STATE_POSTED, + 'changed-files-probe': CHANGED_FILES_PROBE_OK, + merge: MERGE_OK, + }) + + const ctx = harness.makeCtx({ issue: 40, pr: 400 }) + const result = await context.reviewAndMerge(ctx) + + assert.strictEqual(result.status, 'completed') + assert.strictEqual(ctx.metrics.pr_review_iters, 2) + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 1) + + // Iteration 1 booked 're-litigated' (a real finding, cap not reached), then + // the rebuttal-only exit retypes that same bucket to 'carried-unresolved'; + // iteration 2 books 'accepted'. Never a fresh double-booked entry. + const g = ctx.gate_findings['pr-review'] + assert.deepStrictEqual(JSON.parse(JSON.stringify(g.disposition)), { 'carried-unresolved': 1, accepted: 1 }) + + assert.strictEqual(ctx.contested.length, 1) + assert.strictEqual(ctx.contested[0].gate, 'pr-review') + assert.strictEqual(ctx.contested[0].id, 'code-i1-1') + assert.ok(ctx.contested[0].evidence.includes('ran the reproducer')) + + // NO second pushDecision for the rebuttal-only round — the existing "PR + // Review Fix (i1)" decision (pushed for every non-error fix) already + // renders the fixer's summary. + assert.strictEqual(ctx.decisions.filter(function (d) { return d.entry.includes('PR Review Fix (i1)') }).length, 1) + assert.ok(!ctx.decisions.some(function (d) { return d.entry.includes('Gate: findings contested') }), 'pr-review must not push a second decision for a rebuttal-only round: ' + JSON.stringify(ctx.decisions)) + + const skips = harness.readGlobal(context, 'VERIFY_SKIPS') + assert.ok(skips.some(function (s) { return /PR review fix \(iteration 1\)/.test(s) }), 'expected a rebuttal-only VERIFY_SKIPS line: ' + JSON.stringify(skips)) + + const keys = context.agent.calls.map(stageKeyOf) + assert.deepStrictEqual(keys, [ + 'spec-review-i1', 'code-review-i1', 'gate-state-pr-review-i1', 'pr-fix-i1', + 'spec-review-i2', 'code-review-i2', 'gate-state-pr-review-i2', + 'changed-files-probe', 'merge', + ]) + for (const shouldNotRun of ['simplify-pr-fix-i1-i1', 'quality-review-pr-fix-i1-i1']) { + assert.ok(!keys.includes(shouldNotRun), 'stage "' + shouldNotRun + '" must not run on a rebuttal-only round; ran: ' + keys.join(', ')) + } + + // The i2 reviewer prompts must carry the contested block so both i2 + // reviewers see the disputed finding. + const spec2 = context.agent.calls.find(function (c) { return stageKeyOf(c) === 'spec-review-i2' }) + const code2 = context.agent.calls.find(function (c) { return stageKeyOf(c) === 'code-review-i2' }) + assert.ok(String(spec2.prompt).includes('Contested findings'), 'expected the contested block in the i2 spec-review prompt') + assert.ok(String(code2.prompt).includes('Contested findings'), 'expected the contested block in the i2 code-review prompt') + + // The pr-fix-i1 prompt itself must carry FINDING_HYPOTHESIS_ASK, and NOT + // any gate-specific immediate-exit wording — the framing is deliberately + // gate-agnostic since pr-review, unlike quality/test, continues rather + // than exiting on a rebuttal-only round. + const fix1 = context.agent.calls.find(function (c) { return stageKeyOf(c) === 'pr-fix-i1' }) + const fixPrompt = String(fix1.prompt) + assert.ok(fixPrompt.includes('HYPOTHESIS the reviewer formed'), 'expected FINDING_HYPOTHESIS_ASK in the pr-fix prompt') + assert.ok(!fixPrompt.includes('ends this gate immediately'), 'the framing must stay gate-agnostic (no immediate-exit wording): ' + fixPrompt.slice(0, 2000)) + assert.ok(!fixPrompt.includes('no further fix round'), 'the framing must stay gate-agnostic (no immediate-exit wording): ' + fixPrompt.slice(0, 2000)) +}) + +test('reviewAndMerge(): a second rebuttal-only pr-fix round halts needs_human with the PR left open — only ONE rebuttal-only round is permitted per issue', async function () { + const context = harness.boot() + seedReviewFlow(context) + + function rebuttalFix(id) { + return { + status: 'success', commit: null, files_changed: [], fixes_applied: [], summary: 'disagree again', + rebutted: [{ finding_id: id, evidence: 'ran the reproducer, guard already covers this input' }], + } + } + + installScriptedResponder(context, { + 'spec-review-i1': APPROVED_REVIEW, + 'code-review-i1': CODE_FINDING, + 'gate-state-pr-review-i1': GATE_STATE_POSTED, + 'pr-fix-i1': rebuttalFix('code-i1-1'), + 'spec-review-i2': APPROVED_REVIEW, + 'code-review-i2': CODE_FINDING, + 'gate-state-pr-review-i2': GATE_STATE_POSTED, + 'pr-fix-i2': rebuttalFix('code-i2-1'), + // spec-review-i3/code-review-i3/merge deliberately unscripted below — + // proving the halt happens without a third review iteration or a merge. + }) + + const ctx = harness.makeCtx({ issue: 41, pr: 410 }) + const result = await context.reviewAndMerge(ctx) + + assert.strictEqual(result.status, 'needs_human') + assert.strictEqual(result.stage, 'pr-review') + assert.match(result.error || '', /second rebuttal-only/) + assert.strictEqual(ctx.metrics.pr_review_iters, 2) + // Only the FIRST rebuttal-only round counts — the second is a halt, not a + // recorded round. + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 1) + + const keys = context.agent.calls.map(stageKeyOf) + assert.deepStrictEqual(keys, [ + 'spec-review-i1', 'code-review-i1', 'gate-state-pr-review-i1', 'pr-fix-i1', + 'spec-review-i2', 'code-review-i2', 'gate-state-pr-review-i2', 'pr-fix-i2', + 'halt-note-pr-review', + ]) + assert.ok(!keys.includes('merge'), 'merge must never run — the PR is left open; ran: ' + keys.join(', ')) +}) + +test('reviewAndMerge(): FINDING_HYPOTHESIS_ASK renders in the pr-fix prompt when only ONE reviewer returned structured findings (the other approved via prose, `issues` omitted)', async function () { + const context = harness.boot() + seedReviewFlow(context) + + const SPEC_FINDING = { result: 'changes_requested', comments: 'goal not fully met', issues: [{ severity: 'major', summary: 'missing acceptance criterion', recommendation: 'implement it' }], recommended_fix_agent: null, summary: 'one spec finding' } + // `issues` deliberately omitted (not `[]`) — codeFindings normalizes to + // null, isolating the `specFindings !== null || codeFindings !== null` OR. + const CODE_APPROVED_NO_ISSUES_KEY = { result: 'approved', comments: '', recommended_fix_agent: null, summary: 'looks fine' } + + installScriptedResponder(context, { + 'spec-review-i1': SPEC_FINDING, + 'code-review-i1': CODE_APPROVED_NO_ISSUES_KEY, + 'gate-state-pr-review-i1': GATE_STATE_POSTED, + 'pr-fix-i1': FIX_OK, + 'simplify-pr-fix-i1-i1': SIMPLIFY_OK, + 'quality-review-pr-fix-i1-i1': QUALITY_REVIEW_APPROVED, + 'spec-review-i2': APPROVED_REVIEW, + 'code-review-i2': APPROVED_REVIEW, + 'gate-state-pr-review-i2': GATE_STATE_POSTED, + 'commit-sha-probe': COMMIT_PROBE_OK, + 'changed-files-probe': CHANGED_FILES_PROBE_OK, + merge: MERGE_OK, + }) + + const ctx = harness.makeCtx({ issue: 42, pr: 420 }) + const result = await context.reviewAndMerge(ctx) + + assert.strictEqual(result.status, 'completed') + const fixCall = context.agent.calls.find(function (c) { return stageKeyOf(c) === 'pr-fix-i1' }) + assert.ok(fixCall, 'pr-fix-i1 must have run') + assert.ok(String(fixCall.prompt).includes('HYPOTHESIS the reviewer formed'), 'expected FINDING_HYPOTHESIS_ASK to render when even one reviewer returned structured findings: ' + String(fixCall.prompt).slice(0, 2000)) +}) + +test('reviewAndMerge(): a pr-fix response that omits `rebutted` entirely never triggers the rebuttal-only exit, even with empty fixes_applied/files_changed — byte-identical to pre-#167 behavior', async function () { + const context = harness.boot() + seedReviewFlow(context) + + // No `rebutted` key at all — mirrors every fixer response before issue #167. + const NO_REBUTTED_FIX = { status: 'success', commit: 'deadbeef', files_changed: [], fixes_applied: [], summary: 'looked into it, nothing needed changing' } + + installScriptedResponder(context, { + 'spec-review-i1': APPROVED_REVIEW, + 'code-review-i1': CODE_FINDING, + 'gate-state-pr-review-i1': GATE_STATE_POSTED, + 'pr-fix-i1': NO_REBUTTED_FIX, + 'simplify-pr-fix-i1-i1': SIMPLIFY_OK, + 'quality-review-pr-fix-i1-i1': QUALITY_REVIEW_APPROVED, + 'commit-sha-probe': COMMIT_PROBE_OK, + 'spec-review-i2': APPROVED_REVIEW, + 'code-review-i2': APPROVED_REVIEW, + 'gate-state-pr-review-i2': GATE_STATE_POSTED, + 'changed-files-probe': CHANGED_FILES_PROBE_OK, + merge: MERGE_OK, + }) + + const ctx = harness.makeCtx({ issue: 43, pr: 430 }) + const result = await context.reviewAndMerge(ctx) + + assert.strictEqual(result.status, 'completed') + assert.strictEqual(ctx.metrics.rebuttal_only_rounds, 0) + assert.ok(!ctx.contested || ctx.contested.length === 0) + + // runQualityLoop DID run — proves the loop fell through to the normal path, + // not the rebuttal-only `continue` branch. + const keys = context.agent.calls.map(stageKeyOf) + assert.ok(keys.includes('simplify-pr-fix-i1-i1'), 'runQualityLoop must run when `rebutted` is omitted; ran: ' + keys.join(', ')) + assert.ok(keys.includes('quality-review-pr-fix-i1-i1'), 'runQualityLoop must run when `rebutted` is omitted; ran: ' + keys.join(', ')) +}) diff --git a/workflows/ticketmill.js b/workflows/ticketmill.js index 2f71d0d..8ee5b85 100644 --- a/workflows/ticketmill.js +++ b/workflows/ticketmill.js @@ -4927,6 +4927,10 @@ async function implementIssue(ctx) { async function reviewAndMerge(ctx) { let approved = false let haltReason = null + // rebuttalRoundsUsed (issue #167): per-reviewAndMerge-call counter permitting + // exactly ONE rebuttal-only pr-fix round before this gate stops giving a + // disputing fixer another iteration — see the rebuttalOnly block below. + let rebuttalRoundsUsed = 0 for (let iter = 1; iter <= MAX_PR_REVIEW_ITERATIONS && !approved; iter++) { if (STOP.tripped) return fail(ctx, 'halted', 'pr-review', 'stopped: ' + STOP.reason) ctx.metrics.pr_review_iters = iter @@ -5046,6 +5050,7 @@ async function reviewAndMerge(ctx) { 'After pushing, post a PR comment "## PR Review Fix (iteration ' + iter + ')" with the commit SHA and the fixes', 'applied in 2-4 lines (gh pr comment ' + ctx.pr + ' --repo ' + REPO + ').', fixesAppliedIdAsk('[code-i1-2] tightened the null guard'), + (specFindings !== null || codeFindings !== null) ? FINDING_HYPOTHESIS_ASK : '', COMMIT_SHA_ASK, bwFeedback(ctx), HANDOFF_ASK, @@ -5056,6 +5061,58 @@ async function reviewAndMerge(ctx) { pushDecision(ctx, 'PR Review Fix (i' + iter + ')', fix.summary || 'fixes applied') collectNotes(ctx, 'pr-fix', fix) collectPostedCommit(ctx, 'pr-fix-i' + iter, fix) + + // rebuttal-only exit (issue #167): mirrors runQualityLoop's/runTestLoop's + // rebuttalOnly block, evaluated over the UNION of both reviewers' rendered + // finding sets (the fixer saw both blocks in one prompt above) — see + // normalizeRebuttals' doc comment for why `findings` must be exactly what + // was rendered. Evaluated AFTER pushDecision/collectNotes/ + // collectPostedCommit above (so the fixer's summary, handoff notes, and + // the posted-commit ledger all still see this round) but BEFORE + // tallyTouches/runQualityLoop below: tallyTouches is a verified no-op on a + // true rebuttal-only round's empty files_changed, but runQualityLoop is + // NOT — runSimplify fails open on an empty filesChanged array and would + // burn a full quality gate against an untouched tree, so `continue` MUST + // precede that call, not follow it. + // + // Unlike the quality/test loops, this gate does NOT push a second + // pushDecision here — the one above already fires for every non-error fix + // and renders the fixer's summary; a rebuttal-only round has nothing + // further to narrate that recordGateOutcome/retypeGateDisposition and the + // contested-block push below don't already carry into the next iteration. + // + // No id-equality halt: REVIEW_SCHEMA ids are `source + '-' + (i+1)` with + // the iteration baked into `source` (see spec/code review call sites + // above), so a second round's ids are disjoint from the first round's by + // construction — an id-equality check here would be permanently dead code. + // Instead, rebuttalRoundsUsed (declared above the loop) permits exactly + // ONE rebuttal-only round per reviewAndMerge() call: a second one sets + // haltReason and breaks into the existing `if (!approved)` needs_human + // path below, same shape as the bothNothingToFix/capReached breaks above. + const prFixFindings = (specFindings || []).concat(codeFindings || []) + const normRebuttals = normalizeRebuttals(fix.rebutted, prFixFindings) + const rebuttalOnly = normRebuttals.length > 0 && (fix.fixes_applied || []).length === 0 && (fix.files_changed || []).length === 0 + if (rebuttalOnly) { + if (rebuttalRoundsUsed >= 1) { + haltReason = 'PR #' + ctx.pr + ' review iteration ' + iter + ': a second rebuttal-only PR fix round rebutted every finding and applied no fix — left open for human review' + break + } + rebuttalRoundsUsed++ + retypeGateDisposition(ctx, 'pr-review', prReviewDisposition, 'carried-unresolved') + if (!ctx.contested) ctx.contested = [] + for (const r of normRebuttals) { + ctx.contested.push({ gate: 'pr-review', id: r.finding_id, summary: r.summary, evidence: r.evidence }) + } + ctx.metrics.rebuttal_only_rounds++ + // A single un-rolled line: bounded at two per issue by construction + // (capReached breaks above the fix stage, and pr-fix only runs at + // iterations 1 and 2 of MAX_PR_REVIEW_ITERATIONS=3), so unlike the + // quality loop's rolled-up VERIFY_SKIPS index this never needs + // rewriting in place — it is never retracted. + VERIFY_SKIPS.push('#' + ctx.issue + ': PR review fix (iteration ' + iter + ') rebutted every finding and applied no fix — carried as contested, not resolved, for human review') + continue + } + tallyTouches(ctx, fix.files_changed) const q = await runQualityLoop(ctx, 'pr-fix-i' + iter, 'post-review fixes for PR #' + ctx.pr, fix.files_changed) From 6e35ded24dff20fb138b02c611aedd5197a844fb Mon Sep 17 00:00:00 2001 From: aaddrick Date: Tue, 28 Jul 2026 15:40:39 -0400 Subject: [PATCH 4/6] docs(gate-hygiene): document the rebuttal framing for evaluator-fed fix stages (#167) Add a "Rebuttal: a finding is a hypothesis, not a command" section to gate-hygiene.md covering what a rebuttal is and its evidence bar, why the framing is scoped to quality-fix/test-quality-fix/pr-fix and not test-fix/browser-fix, FIX_SCHEMA.rebutted being schema-wide but read at only three sites, normalizeRebuttals' fail-toward-today drops, the three per-gate exits and why only pr-review can block a merge, contestedBlock vs settledBlock's inverted contract, and rebuttal_only_rounds (a continuation, not an exit, on its first pr-review increment). Extend the gate-hygiene.md row in index.md's file map, and correct three now-stale passages in engine-internals.md that #167 falsifies: the carried-unresolved disposition enumeration (missing the rebuttal-only route), the "only findings_empty_exits distinguishes them" claim at pr-review, and findingsBlock's description of a rendered finding's job list. --- docs/architecture/engine-internals.md | 63 +++++++-- docs/architecture/gate-hygiene.md | 194 ++++++++++++++++++++++++++ docs/architecture/index.md | 2 +- 3 files changed, 248 insertions(+), 11 deletions(-) diff --git a/docs/architecture/engine-internals.md b/docs/architecture/engine-internals.md index 8f85a26..664fdcb 100644 --- a/docs/architecture/engine-internals.md +++ b/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 diff --git a/docs/architecture/gate-hygiene.md b/docs/architecture/gate-hygiene.md index b81bf51..5a5a27f 100644 --- a/docs/architecture/gate-hygiene.md +++ b/docs/architecture/gate-hygiene.md @@ -518,6 +518,200 @@ 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)," and `normalizeRebuttals` enforces +the same bar mechanically, not just in prose: an entry with a blank +`evidence` (or a blank `finding_id`) is silently dropped, so bare +disagreement can never survive normalization to become 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` still fires here too, moving the + iteration's booked disposition to `carried-unresolved`. + +### `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: the iteration-2+ + instruction elsewhere in the same prompt not to re-flag issues "already + addressed or accepted" does NOT apply to anything in this block, because + a contested finding is neither — no code changed for it, and no one has + ruled on it. + +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()`; only a reviewer (or eventually a human) closes a +contested entry, by ruling on it in a later iteration. + +### `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. Only a *second* +rebuttal-only round in the same call is an exit, joining the cap-reached and +empty-findings breaks on the `needs_human` path (`pr-fix` runs at most at +iterations 1 and 2 of `MAX_PR_REVIEW_ITERATIONS = 3`, so this bounds an +issue to at most two rebuttal-only pr-fix rounds regardless). 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 diff --git a/docs/architecture/index.md b/docs/architecture/index.md index e6d17b1..1183dc8 100644 --- a/docs/architecture/index.md +++ b/docs/architecture/index.md @@ -12,7 +12,7 @@ plain JavaScript; every unit of actual work is a schema-validated subagent call. | [invocation-and-guardrails.md](invocation-and-guardrails.md) | Invocation, the sandbox lint, and the engine-owned path guardrail. | | [branching-and-merge.md](branching-and-merge.md) | The batch-branch model, release stage, and merge auto-resolve. | | [metrics.md](metrics.md) | Friction and churn, rework tax, gate yield, and outcome grading. | -| [gate-hygiene.md](gate-hygiene.md) | Typed review findings, engine-assigned ids, the absent-vs-empty distinction, the three loop predicates, gate-outcome tallying (the quality gate's disposition map, its cap, the pooled friction denominator, and its supersession of `metrics.md:13-14` and `:114`), and the durable per-issue "## Gate State" comment (write boundaries, the four-state read contract, and the trust model). | +| [gate-hygiene.md](gate-hygiene.md) | Typed review findings, engine-assigned ids, the absent-vs-empty distinction, the three loop predicates, gate-outcome tallying (the quality gate's disposition map, its cap, the pooled friction denominator, and its supersession of `metrics.md:13-14` and `:114`), rebuttal (a finding as a hypothesis a fixer can verify or disprove, `FIX_SCHEMA.rebutted`, the three per-gate exits, and `contestedBlock`), and the durable per-issue "## Gate State" comment (write boundaries, the four-state read contract, and the trust model). | | [failure-semantics.md](failure-semantics.md) | How the run fails, halts, and resumes. | | [cost-and-tokens.md](cost-and-tokens.md) | Token tracking, cost estimation, and the token_budget guard. | | [scheduling.md](scheduling.md) | Claims interop, the consolidation gate, and lane scheduling. | From eaa7458852342727342154a17b53feb0fc9e4c96 Mon Sep 17 00:00:00 2001 From: aaddrick Date: Tue, 28 Jul 2026 16:02:48 -0400 Subject: [PATCH 5/6] fix(gate-findings): trim standing engine prose, close a disposition-tally gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move the #167 rebuttal-framing comment blocks (contestedBlock, FINDING_HYPOTHESIS_ASK, retypeGateDisposition, normalizeRebuttals, and the three in-loop rebuttalOnly explainers) out of workflows/ticketmill.js into one-sentence pointers at docs/architecture/gate-hygiene.md, where the same rationale already lives — clears lint-engine's 92% size warning. - reviewAndMerge()'s second (halting) rebuttal-only pr-fix round now calls retypeGateDisposition before breaking, so gate_findings['pr-review'] never leaves a leftover 're-litigated' count on the run a human is sent to inspect. - Reword gate-hygiene.md's normalizeRebuttals claim: it enforces only non-blankness, not the prompt's concrete-evidence bar. - Make contestedBlock's do-not-re-flag override site-agnostic so it also covers spec review's "stay consistent with prior reviews" instruction. - Correct the contestedBlock comment and gate-hygiene.md to state plainly that nothing closes a ctx.contested entry today — a known gap, not the "only a reviewer closes it" claim that didn't match the code. Fixes code review findings code-i1-1 through code-i1-5 on PR #185. --- .claude/workflows/ticketmill.js | 144 ++++++++---------------------- docs/architecture/gate-hygiene.md | 57 +++++++++--- tests/gate-findings.test.js | 6 +- tests/pr-review-gate.test.js | 11 +++ workflows/ticketmill.js | 144 ++++++++---------------------- 5 files changed, 134 insertions(+), 228 deletions(-) diff --git a/.claude/workflows/ticketmill.js b/.claude/workflows/ticketmill.js index 8ee5b85..97c76a2 100644 --- a/.claude/workflows/ticketmill.js +++ b/.claude/workflows/ticketmill.js @@ -1890,17 +1890,10 @@ function settledBlock(ctx) { } // ----- contested-findings ledger (issue #167) ----- -// A finding a fixer rebutted (FIX_SCHEMA.rebutted, normalized by -// normalizeRebuttals() below) is a DISPUTE, not a resolution — nobody has -// judged whether the rebuttal's evidence actually disproves the finding. -// This renders those disputes back to the NEXT reviewer so it can adjudicate -// them, deliberately NOT reusing settleDecision()/settledBlock(): a settled -// entry carries a "don't re-open without new evidence" contract because -// someone already ruled on it; a contested entry carries the OPPOSITE -// contract, because no one has — this function never calls settleDecision(), -// only a reviewer (or eventually a human) closes a contested entry. Mirrors -// settledBlock's shape (defensive read, last-6 window, per-field slice caps, -// '' when empty) so both render identically at every site that pairs them. +// Renders a fixer's unadjudicated rebuttal back to the NEXT reviewer, the +// opposite trust contract from settledBlock; nothing ever removes an entry +// from ctx.contested once pushed, a known gap, not a design decision. +// Full rationale: docs/architecture/gate-hygiene.md#contestedblock-versus-settledblock-a-deliberate-contract-inversion function contestedBlock(ctx) { const contested = (ctx && ctx.contested) || [] if (!contested.length) return '' @@ -1913,9 +1906,9 @@ function contestedBlock(ctx) { 'A contested finding is NOT resolved: verify the rebuttal\'s evidence yourself. If it holds, say so and drop', 'the finding. If it does not hold, re-raise it as a finding this iteration — do not let it sit contested', 'indefinitely with neither outcome.', - 'A contested finding is NOT "already addressed" — no code changed for it and no one has ruled on it. The', - 'iteration-2+ instruction elsewhere in this prompt not to re-flag issues already addressed or accepted does', - 'NOT apply to anything in this block.', + 'A contested finding is NOT "already addressed" — no code changed for it and no one has ruled on it. Whichever', + 'instruction this prompt carries elsewhere — not to re-flag issues already addressed or accepted, or to stay consistent with', + 'your own prior reviews — does NOT apply to anything in this block.', ].join('\n') } @@ -1930,20 +1923,10 @@ const COMMIT_SHA_ASK = 'Get the exact commit SHA by running: git -C l 'or recall a SHA from memory.' // ----- finding-hypothesis framing (issue #167) ----- -// A finding from a reviewer is a HYPOTHESIS to verify, not a command to obey -// unconditionally — the two contrarian revision stages (approach re-evaluate, -// plan re-plan) already carry this framing; nothing at the fix stages did, -// leaving a fixer that disagrees no path but silent compliance or -// status:'error'. Wired into exactly the three EVALUATOR-FED fix stages -// (quality-fix, test-quality-fix, pr-fix) — never the two ORACLE-fed ones -// (test-fix, browser-fix), which already carry a correct anti-rebuttal guard -// ("fix the real defect — do NOT delete/weaken assertions just to make the -// failure disappear") that this framing would invert: a failing test or a -// broken page is ground truth, not a hypothesis. The consequence clause is -// deliberately GATE-AGNOSTIC (no "ends this gate immediately" / "no further -// fix round runs" language) so the same string stays true whichever of the -// three gates renders it, including pr-review, where a rebuttal-only round -// continues into another review iteration rather than halting on the spot. +// Wired into exactly the three EVALUATOR-FED fix stages (quality-fix, +// test-quality-fix, pr-fix) — never the two ORACLE-fed ones (test-fix, +// browser-fix), whose findings are ground truth, not a judgment call. +// Full rationale: docs/architecture/gate-hygiene.md#rebuttal-a-finding-is-a-hypothesis-not-a-command-issue-167 const FINDING_HYPOTHESIS_ASK = [ 'Each finding above is a HYPOTHESIS the reviewer formed, not a command — verify it against the actual code before acting on it.', 'If a finding is wrong, do NOT change code to satisfy it: record it in `rebutted` with the concrete check you ran ' + @@ -2104,25 +2087,10 @@ function recordGateOutcome(ctx, gate, findings, disposition) { // retypeGateDisposition (issue #167): moves exactly ONE count from disposition // bucket `from` to bucket `to` within an already-recorded -// ctx.gate_findings[gate] entry, WITHOUT touching that gate's overall `count` -// or `severity` mix — both already reflect the findings themselves, which -// retyping a disposition label does not change. Exists because -// recordGateOutcome() above books a disposition BEFORE a fix stage runs (e.g. -// the final quality iteration books 'carried-unresolved' at :2933, then the -// fix stage's rebuttal is only known after that); this lets a later step -// correct the label on a bucket that already exists rather than double-count -// a second recordGateOutcome() call. No-ops (does nothing) when `gate` was -// never recorded, or when the `from` bucket doesn't exist — there is nothing -// to move. -// from === to IS A SUPPORTED, COUNT-PRESERVING NO-OP, not an error case to -// special-case away: it re-buckets a count into itself (subtract 1, then add -// 1 back to the same key), netting zero change. This is reachable in -// practice, not just theoretically — MAX_QUALITY_ITERATIONS is 5 (:46) and -// the quality gate already books 'carried-unresolved' on iteration 5 before -// any fix runs (:2933); a rebuttal-only fix on THAT iteration has nothing -// meaningful to retype the bucket to, so the call site retypes -// 'carried-unresolved' to itself for symmetry with every other iteration's -// call, rather than special-casing the last iteration to skip the call. +// ctx.gate_findings[gate] entry, without touching that gate's `count`/ +// `severity`. from === to is a supported, count-preserving no-op (reachable +// on a rebuttal-only fix at a loop's final iteration), not an error case. +// Full rationale: docs/architecture/gate-hygiene.md#the-three-per-gate-exits-and-why-only-pr-review-can-block-a-merge function retypeGateDisposition(ctx, gate, from, to) { if (!ctx || !ctx.gate_findings) return const key = String(gate || '').trim() @@ -2168,20 +2136,14 @@ function normalizeFindings(raw, source) { } // normalizeRebuttals (issue #167): turns FIX_SCHEMA's `rebutted` array into a -// validated list the engine can act on, mirroring normalizeFindings()'s -// fail-toward-existing-behavior contract just above it. `raw` not being an -// array (including undefined/null — a fixer that never disagreed, i.e. every -// fixer before this issue) returns [] rather than null: unlike -// normalizeFindings, there is no prose-fallback distinction worth preserving -// here — "nothing to act on" is the only meaningful outcome either way. Every -// drop (blank finding_id, blank evidence, or a finding_id absent from -// `findings` — the exact, possibly-null array actually rendered to this -// fixer) fails toward TODAY's behavior: the entry is silently dropped, never -// trusted, so a fixer cannot fabricate a rebuttal against a finding id it was -// never shown (FINDING_HYPOTHESIS_ASK's own "only a bracketed id can be -// rebutted" clause is enforced here, not just asked for in prose). A -// surviving entry carries the matched finding's `summary` through so -// contestedBlock() can render a self-contained line without a second lookup. +// validated list, mirroring normalizeFindings()'s fail-toward-existing- +// behavior contract just above it: every drop (non-array raw, blank +// finding_id/evidence, or a finding_id absent from `findings` — the exact +// array actually rendered to this fixer) fails toward TODAY's behavior, a +// silently dropped entry, so a fixer can't fabricate a rebuttal against an +// id it was never shown. The bar enforced here is non-blankness only, not +// evidence quality — see gate-hygiene.md for that distinction. +// Full rationale: docs/architecture/gate-hygiene.md#normalizerebuttals-every-drop-fails-toward-todays-behavior function normalizeRebuttals(raw, findings) { if (!Array.isArray(raw)) return [] const byId = {} @@ -3098,14 +3060,10 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { collectNotes(ctx, 'quality-fix', fix) collectPostedCommit(ctx, 'quality-fix-' + prefix + '-i' + iter, fix) - // rebuttal-only exit (issue #167): a fix that rebutted every finding it was - // shown and applied none is a DISPUTE, not a resolution — see the - // rebuttalOnly comment above runTestLoop's mirror of this block for the - // full contract this predicate enforces. Evaluated AFTER collectNotes/ - // collectPostedCommit above (so handoff notes and the posted-commit ledger - // still see this round) but BEFORE tallyTouches just below, which is a - // verified no-op here anyway since a true rebuttalOnly round always has an - // empty files_changed by construction. + // rebuttal-only exit (issue #167): a fix that rebutted every finding and + // applied none is a DISPUTE, not a resolution — stops this loop rather + // than spending remaining iterations on it. + // Full rationale: docs/architecture/gate-hygiene.md#the-three-per-gate-exits-and-why-only-pr-review-can-block-a-merge const normRebuttals = normalizeRebuttals(fix.rebutted, revFindings || []) const rebuttalOnly = normRebuttals.length > 0 && (fix.fixes_applied || []).length === 0 && (fix.files_changed || []).length === 0 if (rebuttalOnly) { @@ -3638,17 +3596,11 @@ async function runTestLoop(ctx, forced) { collectNotes(ctx, 'test-quality-fix', qfix) collectPostedCommit(ctx, 'test-quality-fix-i' + iter, qfix) - // rebuttal-only exit (issue #167): mirrors runQualityLoop's rebuttalOnly - // block above, minus the recordGateOutcome/retypeGateDisposition calls — - // this loop books no gate key in ctx.gate_findings today (there is no - // 'test-quality' entry to retype) and this issue does not add one. Exits - // through this loop's OWN non-clean path (return { ok: true }, never - // { ok: false }) so nothing routes through the 'test-loop' stage key that - // a merge-block failure would use — a rebuttal-only round here can never - // block a merge, only get carried to the next reviewer as contested. No - // roll-up index needed (unlike the quality-cap/quality-rebuttal roll-ups - // above): runTestLoop runs once per issue, not once per task, so a single - // VERIFY_SKIPS.push() here can never produce more than one line. + // rebuttal-only exit (issue #167): mirrors runQualityLoop's block above, + // minus recordGateOutcome/retypeGateDisposition — this loop books no + // 'test-quality' gate_findings entry to retype. Exits { ok: true }, never + // { ok: false }: a rebuttal-only round here can never block a merge. + // Full rationale: docs/architecture/gate-hygiene.md#the-three-per-gate-exits-and-why-only-pr-review-can-block-a-merge const normRebuttals = normalizeRebuttals(qfix.rebutted, vFindings || []) const rebuttalOnly = normRebuttals.length > 0 && (qfix.fixes_applied || []).length === 0 && (qfix.files_changed || []).length === 0 if (rebuttalOnly) { @@ -5063,38 +5015,18 @@ async function reviewAndMerge(ctx) { collectPostedCommit(ctx, 'pr-fix-i' + iter, fix) // rebuttal-only exit (issue #167): mirrors runQualityLoop's/runTestLoop's - // rebuttalOnly block, evaluated over the UNION of both reviewers' rendered - // finding sets (the fixer saw both blocks in one prompt above) — see - // normalizeRebuttals' doc comment for why `findings` must be exactly what - // was rendered. Evaluated AFTER pushDecision/collectNotes/ - // collectPostedCommit above (so the fixer's summary, handoff notes, and - // the posted-commit ledger all still see this round) but BEFORE - // tallyTouches/runQualityLoop below: tallyTouches is a verified no-op on a - // true rebuttal-only round's empty files_changed, but runQualityLoop is - // NOT — runSimplify fails open on an empty filesChanged array and would - // burn a full quality gate against an untouched tree, so `continue` MUST - // precede that call, not follow it. - // - // Unlike the quality/test loops, this gate does NOT push a second - // pushDecision here — the one above already fires for every non-error fix - // and renders the fixer's summary; a rebuttal-only round has nothing - // further to narrate that recordGateOutcome/retypeGateDisposition and the - // contested-block push below don't already carry into the next iteration. - // - // No id-equality halt: REVIEW_SCHEMA ids are `source + '-' + (i+1)` with - // the iteration baked into `source` (see spec/code review call sites - // above), so a second round's ids are disjoint from the first round's by - // construction — an id-equality check here would be permanently dead code. - // Instead, rebuttalRoundsUsed (declared above the loop) permits exactly - // ONE rebuttal-only round per reviewAndMerge() call: a second one sets - // haltReason and breaks into the existing `if (!approved)` needs_human - // path below, same shape as the bothNothingToFix/capReached breaks above. + // block, over the UNION of both reviewers' rendered findings. `continue` + // MUST precede runQualityLoop below (runSimplify fails open on an empty + // filesChanged). rebuttalRoundsUsed permits exactly ONE such round per + // reviewAndMerge() call before halting to needs_human. + // Full rationale: docs/architecture/gate-hygiene.md#the-three-per-gate-exits-and-why-only-pr-review-can-block-a-merge const prFixFindings = (specFindings || []).concat(codeFindings || []) const normRebuttals = normalizeRebuttals(fix.rebutted, prFixFindings) const rebuttalOnly = normRebuttals.length > 0 && (fix.fixes_applied || []).length === 0 && (fix.files_changed || []).length === 0 if (rebuttalOnly) { if (rebuttalRoundsUsed >= 1) { haltReason = 'PR #' + ctx.pr + ' review iteration ' + iter + ': a second rebuttal-only PR fix round rebutted every finding and applied no fix — left open for human review' + retypeGateDisposition(ctx, 'pr-review', prReviewDisposition, 'carried-unresolved') break } rebuttalRoundsUsed++ diff --git a/docs/architecture/gate-hygiene.md b/docs/architecture/gate-hygiene.md index 5a5a27f..550a2ea 100644 --- a/docs/architecture/gate-hygiene.md +++ b/docs/architecture/gate-hygiene.md @@ -537,11 +537,15 @@ passing as a resolved gate. 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)," and `normalizeRebuttals` enforces -the same bar mechanically, not just in prose: an entry with a blank -`evidence` (or a blank `finding_id`) is silently dropped, so bare -disagreement can never survive normalization to become a rebuttal the -engine acts on. A rebuttal can also only target a finding the fixer was +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`, @@ -650,8 +654,23 @@ stakes: `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` still fires here too, moving the - iteration's booked disposition to `carried-unresolved`. + 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 @@ -669,18 +688,28 @@ deliberate, not an oversight to reconcile: 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: the iteration-2+ - instruction elsewhere in the same prompt not to re-flag issues "already - addressed or accepted" does NOT apply to anything in this block, because - a contested finding is neither — no code changed for it, and no one has - ruled on it. + 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()`; only a reviewer (or eventually a human) closes a -contested entry, by ruling on it in a later iteration. +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 diff --git a/tests/gate-findings.test.js b/tests/gate-findings.test.js index b53e706..31e50e1 100644 --- a/tests/gate-findings.test.js +++ b/tests/gate-findings.test.js @@ -524,12 +524,14 @@ test('contestedBlock: closing contract is INVERTED from settledBlock — it tell assert.ok(!/re-litigating.*process failure/i.test(block), 'must NOT carry settledBlock\'s discourage-reopening contract: ' + block) }) -test('contestedBlock: states a contested finding is not "already addressed" and that the iteration-2+ do-not-re-flag instruction does not apply to it', function () { +test('contestedBlock: states a contested finding is not "already addressed" and that neither the do-not-re-flag nor the stay-consistent instruction applies to it', function () { const context = harness.boot() const block = context.contestedBlock({ contested: [{ gate: 'quality', id: 'x-1', summary: 's', evidence: 'e' }] }) assert.ok(/not.*already addressed/i.test(block), 'must state a contested finding is not "already addressed": ' + block) - assert.ok(/does not apply|not apply/i.test(block), 'must state the iteration-2+ do-not-re-flag instruction does not apply here: ' + block) + assert.ok(/re-flag issues already addressed or accepted/i.test(block), 'must name the code-review do-not-re-flag instruction: ' + block) + assert.ok(/stay consistent with/i.test(block), 'must name the spec-review stay-consistent instruction: ' + block) + assert.ok(/does not apply|not apply/i.test(block), 'must state neither instruction applies here: ' + block) }) test('contestedBlock: renders only the last 6 entries', function () { diff --git a/tests/pr-review-gate.test.js b/tests/pr-review-gate.test.js index 14f4547..8f1ea10 100644 --- a/tests/pr-review-gate.test.js +++ b/tests/pr-review-gate.test.js @@ -576,6 +576,17 @@ test('reviewAndMerge(): a second rebuttal-only pr-fix round halts needs_human wi 'halt-note-pr-review', ]) assert.ok(!keys.includes('merge'), 'merge must never run — the PR is left open; ran: ' + keys.join(', ')) + + // code-i1-2: the halting iteration must ALSO retype its just-booked + // 're-litigated' disposition to 'carried-unresolved' — the same call the + // continuing (first) rebuttal-only round makes — so a human reading + // gate_findings['pr-review'] for the run this halted on sees the run as + // carried-unresolved, not as a still-open re-litigation. Iteration 1 books + // 're-litigated' then retypes to 'carried-unresolved' (continuing round); + // iteration 2 books 're-litigated' again then must retype it too (halting + // round) — leaving NO leftover 're-litigated' count from either iteration. + const g = ctx.gate_findings['pr-review'] + assert.deepStrictEqual(JSON.parse(JSON.stringify(g.disposition)), { 'carried-unresolved': 2 }) }) test('reviewAndMerge(): FINDING_HYPOTHESIS_ASK renders in the pr-fix prompt when only ONE reviewer returned structured findings (the other approved via prose, `issues` omitted)', async function () { diff --git a/workflows/ticketmill.js b/workflows/ticketmill.js index 8ee5b85..97c76a2 100644 --- a/workflows/ticketmill.js +++ b/workflows/ticketmill.js @@ -1890,17 +1890,10 @@ function settledBlock(ctx) { } // ----- contested-findings ledger (issue #167) ----- -// A finding a fixer rebutted (FIX_SCHEMA.rebutted, normalized by -// normalizeRebuttals() below) is a DISPUTE, not a resolution — nobody has -// judged whether the rebuttal's evidence actually disproves the finding. -// This renders those disputes back to the NEXT reviewer so it can adjudicate -// them, deliberately NOT reusing settleDecision()/settledBlock(): a settled -// entry carries a "don't re-open without new evidence" contract because -// someone already ruled on it; a contested entry carries the OPPOSITE -// contract, because no one has — this function never calls settleDecision(), -// only a reviewer (or eventually a human) closes a contested entry. Mirrors -// settledBlock's shape (defensive read, last-6 window, per-field slice caps, -// '' when empty) so both render identically at every site that pairs them. +// Renders a fixer's unadjudicated rebuttal back to the NEXT reviewer, the +// opposite trust contract from settledBlock; nothing ever removes an entry +// from ctx.contested once pushed, a known gap, not a design decision. +// Full rationale: docs/architecture/gate-hygiene.md#contestedblock-versus-settledblock-a-deliberate-contract-inversion function contestedBlock(ctx) { const contested = (ctx && ctx.contested) || [] if (!contested.length) return '' @@ -1913,9 +1906,9 @@ function contestedBlock(ctx) { 'A contested finding is NOT resolved: verify the rebuttal\'s evidence yourself. If it holds, say so and drop', 'the finding. If it does not hold, re-raise it as a finding this iteration — do not let it sit contested', 'indefinitely with neither outcome.', - 'A contested finding is NOT "already addressed" — no code changed for it and no one has ruled on it. The', - 'iteration-2+ instruction elsewhere in this prompt not to re-flag issues already addressed or accepted does', - 'NOT apply to anything in this block.', + 'A contested finding is NOT "already addressed" — no code changed for it and no one has ruled on it. Whichever', + 'instruction this prompt carries elsewhere — not to re-flag issues already addressed or accepted, or to stay consistent with', + 'your own prior reviews — does NOT apply to anything in this block.', ].join('\n') } @@ -1930,20 +1923,10 @@ const COMMIT_SHA_ASK = 'Get the exact commit SHA by running: git -C l 'or recall a SHA from memory.' // ----- finding-hypothesis framing (issue #167) ----- -// A finding from a reviewer is a HYPOTHESIS to verify, not a command to obey -// unconditionally — the two contrarian revision stages (approach re-evaluate, -// plan re-plan) already carry this framing; nothing at the fix stages did, -// leaving a fixer that disagrees no path but silent compliance or -// status:'error'. Wired into exactly the three EVALUATOR-FED fix stages -// (quality-fix, test-quality-fix, pr-fix) — never the two ORACLE-fed ones -// (test-fix, browser-fix), which already carry a correct anti-rebuttal guard -// ("fix the real defect — do NOT delete/weaken assertions just to make the -// failure disappear") that this framing would invert: a failing test or a -// broken page is ground truth, not a hypothesis. The consequence clause is -// deliberately GATE-AGNOSTIC (no "ends this gate immediately" / "no further -// fix round runs" language) so the same string stays true whichever of the -// three gates renders it, including pr-review, where a rebuttal-only round -// continues into another review iteration rather than halting on the spot. +// Wired into exactly the three EVALUATOR-FED fix stages (quality-fix, +// test-quality-fix, pr-fix) — never the two ORACLE-fed ones (test-fix, +// browser-fix), whose findings are ground truth, not a judgment call. +// Full rationale: docs/architecture/gate-hygiene.md#rebuttal-a-finding-is-a-hypothesis-not-a-command-issue-167 const FINDING_HYPOTHESIS_ASK = [ 'Each finding above is a HYPOTHESIS the reviewer formed, not a command — verify it against the actual code before acting on it.', 'If a finding is wrong, do NOT change code to satisfy it: record it in `rebutted` with the concrete check you ran ' + @@ -2104,25 +2087,10 @@ function recordGateOutcome(ctx, gate, findings, disposition) { // retypeGateDisposition (issue #167): moves exactly ONE count from disposition // bucket `from` to bucket `to` within an already-recorded -// ctx.gate_findings[gate] entry, WITHOUT touching that gate's overall `count` -// or `severity` mix — both already reflect the findings themselves, which -// retyping a disposition label does not change. Exists because -// recordGateOutcome() above books a disposition BEFORE a fix stage runs (e.g. -// the final quality iteration books 'carried-unresolved' at :2933, then the -// fix stage's rebuttal is only known after that); this lets a later step -// correct the label on a bucket that already exists rather than double-count -// a second recordGateOutcome() call. No-ops (does nothing) when `gate` was -// never recorded, or when the `from` bucket doesn't exist — there is nothing -// to move. -// from === to IS A SUPPORTED, COUNT-PRESERVING NO-OP, not an error case to -// special-case away: it re-buckets a count into itself (subtract 1, then add -// 1 back to the same key), netting zero change. This is reachable in -// practice, not just theoretically — MAX_QUALITY_ITERATIONS is 5 (:46) and -// the quality gate already books 'carried-unresolved' on iteration 5 before -// any fix runs (:2933); a rebuttal-only fix on THAT iteration has nothing -// meaningful to retype the bucket to, so the call site retypes -// 'carried-unresolved' to itself for symmetry with every other iteration's -// call, rather than special-casing the last iteration to skip the call. +// ctx.gate_findings[gate] entry, without touching that gate's `count`/ +// `severity`. from === to is a supported, count-preserving no-op (reachable +// on a rebuttal-only fix at a loop's final iteration), not an error case. +// Full rationale: docs/architecture/gate-hygiene.md#the-three-per-gate-exits-and-why-only-pr-review-can-block-a-merge function retypeGateDisposition(ctx, gate, from, to) { if (!ctx || !ctx.gate_findings) return const key = String(gate || '').trim() @@ -2168,20 +2136,14 @@ function normalizeFindings(raw, source) { } // normalizeRebuttals (issue #167): turns FIX_SCHEMA's `rebutted` array into a -// validated list the engine can act on, mirroring normalizeFindings()'s -// fail-toward-existing-behavior contract just above it. `raw` not being an -// array (including undefined/null — a fixer that never disagreed, i.e. every -// fixer before this issue) returns [] rather than null: unlike -// normalizeFindings, there is no prose-fallback distinction worth preserving -// here — "nothing to act on" is the only meaningful outcome either way. Every -// drop (blank finding_id, blank evidence, or a finding_id absent from -// `findings` — the exact, possibly-null array actually rendered to this -// fixer) fails toward TODAY's behavior: the entry is silently dropped, never -// trusted, so a fixer cannot fabricate a rebuttal against a finding id it was -// never shown (FINDING_HYPOTHESIS_ASK's own "only a bracketed id can be -// rebutted" clause is enforced here, not just asked for in prose). A -// surviving entry carries the matched finding's `summary` through so -// contestedBlock() can render a self-contained line without a second lookup. +// validated list, mirroring normalizeFindings()'s fail-toward-existing- +// behavior contract just above it: every drop (non-array raw, blank +// finding_id/evidence, or a finding_id absent from `findings` — the exact +// array actually rendered to this fixer) fails toward TODAY's behavior, a +// silently dropped entry, so a fixer can't fabricate a rebuttal against an +// id it was never shown. The bar enforced here is non-blankness only, not +// evidence quality — see gate-hygiene.md for that distinction. +// Full rationale: docs/architecture/gate-hygiene.md#normalizerebuttals-every-drop-fails-toward-todays-behavior function normalizeRebuttals(raw, findings) { if (!Array.isArray(raw)) return [] const byId = {} @@ -3098,14 +3060,10 @@ async function runQualityLoop(ctx, prefix, taskDesc, filesChanged) { collectNotes(ctx, 'quality-fix', fix) collectPostedCommit(ctx, 'quality-fix-' + prefix + '-i' + iter, fix) - // rebuttal-only exit (issue #167): a fix that rebutted every finding it was - // shown and applied none is a DISPUTE, not a resolution — see the - // rebuttalOnly comment above runTestLoop's mirror of this block for the - // full contract this predicate enforces. Evaluated AFTER collectNotes/ - // collectPostedCommit above (so handoff notes and the posted-commit ledger - // still see this round) but BEFORE tallyTouches just below, which is a - // verified no-op here anyway since a true rebuttalOnly round always has an - // empty files_changed by construction. + // rebuttal-only exit (issue #167): a fix that rebutted every finding and + // applied none is a DISPUTE, not a resolution — stops this loop rather + // than spending remaining iterations on it. + // Full rationale: docs/architecture/gate-hygiene.md#the-three-per-gate-exits-and-why-only-pr-review-can-block-a-merge const normRebuttals = normalizeRebuttals(fix.rebutted, revFindings || []) const rebuttalOnly = normRebuttals.length > 0 && (fix.fixes_applied || []).length === 0 && (fix.files_changed || []).length === 0 if (rebuttalOnly) { @@ -3638,17 +3596,11 @@ async function runTestLoop(ctx, forced) { collectNotes(ctx, 'test-quality-fix', qfix) collectPostedCommit(ctx, 'test-quality-fix-i' + iter, qfix) - // rebuttal-only exit (issue #167): mirrors runQualityLoop's rebuttalOnly - // block above, minus the recordGateOutcome/retypeGateDisposition calls — - // this loop books no gate key in ctx.gate_findings today (there is no - // 'test-quality' entry to retype) and this issue does not add one. Exits - // through this loop's OWN non-clean path (return { ok: true }, never - // { ok: false }) so nothing routes through the 'test-loop' stage key that - // a merge-block failure would use — a rebuttal-only round here can never - // block a merge, only get carried to the next reviewer as contested. No - // roll-up index needed (unlike the quality-cap/quality-rebuttal roll-ups - // above): runTestLoop runs once per issue, not once per task, so a single - // VERIFY_SKIPS.push() here can never produce more than one line. + // rebuttal-only exit (issue #167): mirrors runQualityLoop's block above, + // minus recordGateOutcome/retypeGateDisposition — this loop books no + // 'test-quality' gate_findings entry to retype. Exits { ok: true }, never + // { ok: false }: a rebuttal-only round here can never block a merge. + // Full rationale: docs/architecture/gate-hygiene.md#the-three-per-gate-exits-and-why-only-pr-review-can-block-a-merge const normRebuttals = normalizeRebuttals(qfix.rebutted, vFindings || []) const rebuttalOnly = normRebuttals.length > 0 && (qfix.fixes_applied || []).length === 0 && (qfix.files_changed || []).length === 0 if (rebuttalOnly) { @@ -5063,38 +5015,18 @@ async function reviewAndMerge(ctx) { collectPostedCommit(ctx, 'pr-fix-i' + iter, fix) // rebuttal-only exit (issue #167): mirrors runQualityLoop's/runTestLoop's - // rebuttalOnly block, evaluated over the UNION of both reviewers' rendered - // finding sets (the fixer saw both blocks in one prompt above) — see - // normalizeRebuttals' doc comment for why `findings` must be exactly what - // was rendered. Evaluated AFTER pushDecision/collectNotes/ - // collectPostedCommit above (so the fixer's summary, handoff notes, and - // the posted-commit ledger all still see this round) but BEFORE - // tallyTouches/runQualityLoop below: tallyTouches is a verified no-op on a - // true rebuttal-only round's empty files_changed, but runQualityLoop is - // NOT — runSimplify fails open on an empty filesChanged array and would - // burn a full quality gate against an untouched tree, so `continue` MUST - // precede that call, not follow it. - // - // Unlike the quality/test loops, this gate does NOT push a second - // pushDecision here — the one above already fires for every non-error fix - // and renders the fixer's summary; a rebuttal-only round has nothing - // further to narrate that recordGateOutcome/retypeGateDisposition and the - // contested-block push below don't already carry into the next iteration. - // - // No id-equality halt: REVIEW_SCHEMA ids are `source + '-' + (i+1)` with - // the iteration baked into `source` (see spec/code review call sites - // above), so a second round's ids are disjoint from the first round's by - // construction — an id-equality check here would be permanently dead code. - // Instead, rebuttalRoundsUsed (declared above the loop) permits exactly - // ONE rebuttal-only round per reviewAndMerge() call: a second one sets - // haltReason and breaks into the existing `if (!approved)` needs_human - // path below, same shape as the bothNothingToFix/capReached breaks above. + // block, over the UNION of both reviewers' rendered findings. `continue` + // MUST precede runQualityLoop below (runSimplify fails open on an empty + // filesChanged). rebuttalRoundsUsed permits exactly ONE such round per + // reviewAndMerge() call before halting to needs_human. + // Full rationale: docs/architecture/gate-hygiene.md#the-three-per-gate-exits-and-why-only-pr-review-can-block-a-merge const prFixFindings = (specFindings || []).concat(codeFindings || []) const normRebuttals = normalizeRebuttals(fix.rebutted, prFixFindings) const rebuttalOnly = normRebuttals.length > 0 && (fix.fixes_applied || []).length === 0 && (fix.files_changed || []).length === 0 if (rebuttalOnly) { if (rebuttalRoundsUsed >= 1) { haltReason = 'PR #' + ctx.pr + ' review iteration ' + iter + ': a second rebuttal-only PR fix round rebutted every finding and applied no fix — left open for human review' + retypeGateDisposition(ctx, 'pr-review', prReviewDisposition, 'carried-unresolved') break } rebuttalRoundsUsed++ From af03409144bf12baa495e030e79862e1961fa38d Mon Sep 17 00:00:00 2001 From: aaddrick Date: Tue, 28 Jul 2026 16:10:36 -0400 Subject: [PATCH 6/6] docs(gate-hygiene): correct rebuttal_only_rounds halt-round description [code-i2-1] The halting second pr-fix rebuttal-only round retypes the disposition and breaks without incrementing the counter (by design, pinned at tests/pr-review-gate.test.js:568-570), but the prose said the counter "bounds an issue to at most two rebuttal-only pr-fix rounds" without noting the halt itself is never counted. Clarify that the counter tracks continuations, not rounds, and that the halt is already carried by haltReason and the needs_human status. --- docs/architecture/gate-hygiene.md | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/docs/architecture/gate-hygiene.md b/docs/architecture/gate-hygiene.md index 550a2ea..c1b926e 100644 --- a/docs/architecture/gate-hygiene.md +++ b/docs/architecture/gate-hygiene.md @@ -722,13 +722,18 @@ 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. Only a *second* -rebuttal-only round in the same call is an exit, joining the cap-reached and -empty-findings breaks on the `needs_human` path (`pr-fix` runs at most at -iterations 1 and 2 of `MAX_PR_REVIEW_ITERATIONS = 3`, so this bounds an -issue to at most two rebuttal-only pr-fix rounds regardless). Reading -`rebuttal_only_rounds` at pr-review without also checking whether the issue -ultimately reached `approved` will misread a continuation as a stall. +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