From 93b13c2db8409c9cf3585742ed765c1697646b0b Mon Sep 17 00:00:00 2001 From: Will Date: Thu, 22 Oct 2026 15:57:23 +0800 Subject: [PATCH 1/3] fix: preserve finding boundaries in mixed reviewer replies --- CHANGELOG.md | 2 ++ docs/usage.md | 4 +++ lib/loop.js | 18 ++++++------ test/language-replies.test.js | 54 +++++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55e424b..1492a9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Keep agreed findings settled in mixed replies with explicit dispute IDs, and + stop rebuttal evidence before a separate new finding. - Support a saved `--language` preference for reviewer findings, judge explanations, replies and code-fix summaries, including language switches on resumed runs (#124). diff --git a/docs/usage.md b/docs/usage.md index af8866d..69e5817 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -184,6 +184,10 @@ JSON keys, verdict values and exact protocol/stop markers remain unchanged. Code comments, string literals and test names follow the repository's conventions and task requirements rather than the response-language preference. +Replies with explicit `DISPUTE: ` markers reopen only the named +findings. Agreements with other findings remain settled. Each complete rebuttal +ends at the next live dispute or new finding; quoted examples stay in its evidence. + ## Token usage The CLI and saved-run console show input, output and total tokens for every review, diff --git a/lib/loop.js b/lib/loop.js index 0ba2d8e..11d2ec1 100644 --- a/lib/loop.js +++ b/lib/loop.js @@ -388,13 +388,14 @@ export async function recordReplyDisputes(dir, agent, thread, { report, verdict // claim: one contested item must not spend the whole run's budget. let explicitlyDisputed = false; const replyLines = (report ?? "").split("\n"); + const hasDisputeMarkers = replyMarkers(replyLines).some(marker => marker.id !== null); for (const f of thread) { if (f.status === "open") continue; const disputed = disputeLine(replyLines, f) >= 0; if (disputed) explicitlyDisputed = true; - // A sign-off may name the title or path while agreeing with the verdict. - // Only an explicit dispute can contradict that approval. - if (disputed || (verdict !== "clean" && mentions(report, f))) { + // Explicit IDs distinguish objections from agreements in a mixed reply. + // Keep title/path matching only for legacy replies without live markers. + if (disputed || (!hasDisputeMarkers && verdict !== "clean" && mentions(report, f))) { await appendEvent(dir, { t: "finding.turn", id: f.id, agent, who: agent, text: excerpt(report, f), @@ -418,10 +419,11 @@ function same(f, nf) { function disputeLine(lines, f) { if (!f.id) return -1; - return disputeMarkers(lines).find(marker => marker.id === f.id)?.line ?? -1; + return replyMarkers(lines).find(marker => marker.id === f.id)?.line ?? -1; } -function disputeMarkers(lines) { +// Both new findings and disputes delimit evidence; quoted examples do not. +function replyMarkers(lines) { const markers = []; let fence = null; for (const [i, line] of lines.entries()) { @@ -435,8 +437,8 @@ function disputeMarkers(lines) { else if (marker[1][0] === fence[0] && marker[1].length >= fence.length && !marker[2].trim()) fence = null; continue; } - const dispute = !fence && line.trim().match(/^DISPUTE: (.+)$/); - if (dispute) markers.push({ id: dispute[1], line: i }); + const protocol = !fence && line.trim().match(/^(DISPUTE|FINDING): (.+)$/); + if (protocol) markers.push({ id: protocol[1] === "DISPUTE" ? protocol[2] : null, line: i }); } return markers; } @@ -457,7 +459,7 @@ function mentions(report, f) { function excerpt(report, f) { const lines = (report ?? "").split("\n"); - const markers = disputeMarkers(lines); + const markers = replyMarkers(lines); const sections = []; for (const [i, marker] of markers.entries()) { if (marker.id === f.id) { diff --git a/test/language-replies.test.js b/test/language-replies.test.js index 0de04e1..a6070d2 100644 --- a/test/language-replies.test.js +++ b/test/language-replies.test.js @@ -228,3 +228,57 @@ for (const fence of ['```', '~~~']) { } }); } + +for (const [language, claim, agreement, objection] of [ + ['zh-CN', '[P2] 数值错误', '关于 B 的数值错误，我同意已经修复。', '匿名请求仍然能执行受保护操作。'], + ['en', '[P2] Numeric overflow', 'I agree that B: Numeric overflow is fixed.', 'Anonymous requests can still perform the protected action.'], +]) test(`explicit dispute IDs keep mixed agreements settled: ${language}`, async t => { + const dir = await mkdtemp(path.join(tmpdir(), 'jury-mixed-reply-')); + t.after(() => rm(dir, {recursive: true, force: true})); + const report = `DISPUTE: A\n${objection}\n\n${agreement}`; + const agent = {name: 'fixture', argv: [process.execPath, '-e', `console.log(${JSON.stringify(report)})`]}; + // The agreed finding comes first so marker handling cannot depend on thread order. + for (const [id, title, status] of [['B', claim, 'accepted'], ['A', '[P2] Missing authorization', 'rejected']]) { + await appendEvent(dir, {t: 'finding.raised', id, agent: agent.name, round: 1, claim: title, loc: `${id}.js:1`}); + await appendEvent(dir, {t: 'finding.resolved', id, verdict: status, reason: 'Checked'}); + } + const [answer] = await replyRound({dir, pool: [agent], cfg: {stopToken: 'NO NEW FINDINGS'}, + worktree: dir, sha: 'abc', round: 1, language, findings: await findingsIn(dir)}); + assert.equal(answer.failed, false); + assert.equal(answer.clean, false, 'the disputed finding still needs the judge'); + const events = await readEvents(dir), findings = await findingsIn(dir); + assert.deepEqual({ + turns: events.filter(e => e.t === 'finding.turn').map(e => e.id), + pending: outstanding(findings, events).map(f => f.id), + agreed: {status: findings.get('B').status, contested: findings.get('B').contested, turns: findings.get('B').turns}, + }, {turns: ['A'], pending: ['A'], agreed: {status: 'accepted', contested: false, turns: 0}}); + assert.ok(findings.get('A').rebuttals[0].includes(objection)); + assert.deepEqual(findings.get('B').rebuttals, []); +}); + +test('quoted dispute markers do not disable legacy title matching', async t => { + const report = 'Earlier example:\n```text\nDISPUTE: unknown\n```\n我仍然不同意：缺少权限检查的问题依然存在。'; + const {events, findings} = await reply(t, '[P2] 缺少权限检查', report); + assert.deepEqual(events.filter(e => e.t === 'finding.turn').map(e => e.id), ['A']); + assert.deepEqual(outstanding(findings, events).map(f => f.id), ['A']); +}); + +test('complete rebuttal sections preserve code examples and repeated disputes without other findings', async t => { + const first = [ + 'DISPUTE: A', 'The quoted example is part of the reproduction:', '', + '```text', 'DISPUTE: B', '```', '', + 'Important evidence after the example: the protected write succeeds.', + ].join('\n'); + const second = 'DISPUTE: A\nAn additional request confirms the same unauthorized write.'; + const report = `Introduction.\n\n${first}\n\nDISPUTE: B\nUnrelated dispute evidence.\n\n${second}\n\n` + + 'FINDING: A separate defect\nWHERE: other.js:2\nUnrelated new finding evidence.'; + const {events, findings} = await reply(t, '[P2] Missing authorization validation', report, 'en'); + const saved = events.find(e => e.t === 'finding.turn' && e.id === 'A').text; + assert.equal(saved, `${first}\n\n${second}`, 'keep all sections for the exact ID and respect live boundaries'); + const prompt = triagePrompt({finding: findings.get('A'), trunk: 'master', stopToken: 'NO NEW FINDINGS'}); + assert.ok(prompt.includes('Important evidence after the example: the protected write succeeds.')); + assert.ok(prompt.includes('An additional request confirms the same unauthorized write.')); + assert.ok(!prompt.includes('Unrelated dispute evidence.')); + assert.ok(!prompt.includes('Unrelated new finding evidence.')); +}); + From c133286dff7274be87218fcfc21801639b0218f7 Mon Sep 17 00:00:00 2001 From: Will Date: Thu, 22 Oct 2026 15:57:55 +0800 Subject: [PATCH 2/3] style: trim trailing test whitespace --- test/language-replies.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/language-replies.test.js b/test/language-replies.test.js index a6070d2..0d5a539 100644 --- a/test/language-replies.test.js +++ b/test/language-replies.test.js @@ -281,4 +281,3 @@ test('complete rebuttal sections preserve code examples and repeated disputes wi assert.ok(!prompt.includes('Unrelated dispute evidence.')); assert.ok(!prompt.includes('Unrelated new finding evidence.')); }); - From 4ef414b2188f39cf8ecf32bc2f5dea0cd0a6d6ae Mon Sep 17 00:00:00 2001 From: Will Date: Thu, 22 Oct 2026 16:05:08 +0800 Subject: [PATCH 3/3] fix: round 1 review findings --- CHANGELOG.md | 3 ++- docs/usage.md | 3 ++- lib/loop.js | 10 ++++++++-- test/language-replies.test.js | 31 +++++++++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1492a9e..242c00d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,8 @@ ## Unreleased - Keep agreed findings settled in mixed replies with explicit dispute IDs, and - stop rebuttal evidence before a separate new finding. + stop rebuttal evidence before a separate new finding while preserving restatements + of the disputed finding. - Support a saved `--language` preference for reviewer findings, judge explanations, replies and code-fix summaries, including language switches on resumed runs (#124). diff --git a/docs/usage.md b/docs/usage.md index 69e5817..a6d84d5 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -186,7 +186,8 @@ and task requirements rather than the response-language preference. Replies with explicit `DISPUTE: ` markers reopen only the named findings. Agreements with other findings remain settled. Each complete rebuttal -ends at the next live dispute or new finding; quoted examples stay in its evidence. +ends at the next live dispute or separate new finding; restatements of the disputed +finding and quoted examples stay in its evidence. ## Token usage diff --git a/lib/loop.js b/lib/loop.js index 11d2ec1..18765ae 100644 --- a/lib/loop.js +++ b/lib/loop.js @@ -13,7 +13,7 @@ import { findingPr } from "./review-group.js"; // effect of agreeing. import path from "node:path"; import { randomUUID } from "node:crypto"; -import { runAgent } from "./agents.js"; +import { runAgent, parseFindings } from "./agents.js"; import { buildReply, threadFor, replyArgv } from "./reply.js"; import { findingsIn, gate, settledList } from "./findings.js"; import { appendEvent, writeArtifact } from "./store.js"; @@ -459,7 +459,13 @@ function mentions(report, f) { function excerpt(report, f) { const lines = (report ?? "").split("\n"); - const markers = replyMarkers(lines); + const markers = replyMarkers(lines).filter((marker, i, all) => { + if (marker.id !== null) return true; + // A restatement is deduplicated by replyRound, so its evidence must stay + // in the rebuttal. Use the same identity check for both paths. + const [finding] = parseFindings(lines.slice(marker.line, all[i + 1]?.line).join("\n")); + return !finding || !same(f, finding); + }); const sections = []; for (const [i, marker] of markers.entries()) { if (marker.id === f.id) { diff --git a/test/language-replies.test.js b/test/language-replies.test.js index 0d5a539..cb117c8 100644 --- a/test/language-replies.test.js +++ b/test/language-replies.test.js @@ -281,3 +281,34 @@ test('complete rebuttal sections preserve code examples and repeated disputes wi assert.ok(!prompt.includes('Unrelated dispute evidence.')); assert.ok(!prompt.includes('Unrelated new finding evidence.')); }); + +for (const [label, claim, loc] of [ + ['same title and location', 'Missing authorization', 'auth.js:20'], + ['same location with a revised title', 'Alternate route permits anonymous writes', 'auth.js:20'], + ['normalized title with a different location', 'Missing authorization!', 'alternate.js:30'], +]) test(`restated disputed findings retain evidence and stop at separate findings: ${label}`, async t => { + const dir = await mkdtemp(path.join(tmpdir(), 'jury-restated-reply-')); + t.after(() => rm(dir, {recursive: true, force: true})); + const evidence = 'New evidence: anonymous POST to the alternate route returns 200 and changes protected data.'; + const section = `DISPUTE: A\nFINDING: ${claim}\nWHERE: ${loc}\n${evidence}`; + const separate = 'FINDING: Numeric overflow\nWHERE: balance.js:8\nUnrelated new finding evidence.'; + const report = `${section}\n\n${separate}\n\nDISPUTE: B\nUnrelated dispute evidence.`; + const agent = {name: 'fixture', argv: [process.execPath, '-e', `console.log(${JSON.stringify(report)})`]}; + await appendEvent(dir, {t: 'finding.raised', id: 'A', agent: agent.name, round: 1, + claim: 'Missing authorization', loc: 'auth.js:20'}); + await appendEvent(dir, {t: 'finding.resolved', id: 'A', verdict: 'rejected', reason: 'Checked'}); + const [answer] = await replyRound({dir, pool: [agent], cfg: {stopToken: 'NO NEW FINDINGS'}, + worktree: dir, sha: 'abc', round: 1, language: 'en', findings: await findingsIn(dir)}); + assert.equal(answer.failed, false); + assert.equal(answer.clean, false); + assert.equal(answer.raised, 1, 'only the separate finding is raised'); + const events = await readEvents(dir), findings = await findingsIn(dir); + assert.deepEqual(events.filter(e => e.t === 'finding.raised' && e.id !== 'A') + .map(({claim, loc}) => ({claim, loc})), [{claim: 'Numeric overflow', loc: 'balance.js:8'}]); + assert.equal(events.find(e => e.t === 'finding.turn' && e.id === 'A').text, section); + assert.deepEqual(findings.get('A').rebuttals, [section]); + const prompt = triagePrompt({finding: findings.get('A'), trunk: 'master', language: 'en'}); + assert.ok(prompt.includes(evidence)); + assert.ok(!prompt.includes('Unrelated new finding evidence.')); + assert.ok(!prompt.includes('Unrelated dispute evidence.')); +});