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