diff --git a/packages/review-kit/src/agent.ts b/packages/review-kit/src/agent.ts index dd82861..22e0759 100644 --- a/packages/review-kit/src/agent.ts +++ b/packages/review-kit/src/agent.ts @@ -211,7 +211,7 @@ function createParsedReviewHandler( `${parsed.lens} review harness failed (exit ${run.exitCode}) for PR #${pullRequest.number}` ); } - const body = run.output.trim(); + const body = reviewBody(run.output); if (!body) { throw new Error(`${parsed.lens} review harness produced no review for PR #${pullRequest.number}`); } @@ -242,6 +242,49 @@ function createParsedReviewHandler( }; } +/** + * The review is what follows the verdict line — everything before it is thinking. + * + * `run.output` is the harness's raw stdout, so it carries whatever the model said + * on its way to an answer: "Evidence confirmed.", "I have all evidence needed.", + * a draft findings list, "Writing the review now." Posting that verbatim buries + * the findings under the search that produced them. + * + * This cannot be fixed in the charter. A charter can forbid a preamble, and the + * model still emits one, because you cannot instruct a model not to think — only + * decline to publish the thinking. Measured on a real reviewer against a + * 150-word cap: 537 words, then 256 after the ban was sharpened, then 191, then + * 187 after the whole contract was moved to the top of the charter where it is + * read first. Stripping in code took the same reviewer to 153 on the next run. + * The findings were always inside budget; only the preamble was not. + * + * So the charter owns the SHAPE (which it gets right reliably) and the kit owns + * the BOUNDARY. Same reason `agents/review` strips its own trailing `READY` + * sentinel rather than trusting the model to omit it. + * + * Cuts at the LAST verdict line, not the first: a model that drafts its findings + * before writing them emits two, and the real review is the final one. + * + * The match is deliberately tolerant of how the model bolds the line — + * `**Verdict:`, `**Verdict**:`, `**Verdict** :`, any casing. The charter asks for + * one exact form and the production reviewer emits it, but a stricter pattern + * fails OPEN in the worst way: an unmatched line publishes the entire preamble, + * which is the thing this function exists to prevent. Being lax here costs + * nothing; being strict costs the whole feature on a formatting wobble. + * + * A body with no verdict line at all means the model ignored the format; return + * it unchanged rather than nothing, because a malformed review still beats + * silence — for an advisory agent, silence reads as approval. + */ +export function reviewBody(output: string): string { + const text = (output ?? '').trim(); + let start = -1; + for (const match of text.matchAll(/^\*\*Verdict(?:\*\*)?\s*:/gimu)) { + start = match.index ?? start; + } + return start >= 0 ? text.slice(start).trim() : text; +} + export function idempotencyMarker(lens: string, headSha: string): string { return ``; } diff --git a/packages/review-kit/src/index.ts b/packages/review-kit/src/index.ts index de22076..d7a7cd4 100644 --- a/packages/review-kit/src/index.ts +++ b/packages/review-kit/src/index.ts @@ -1,6 +1,7 @@ export { defineReviewAgent, idempotencyMarker, + reviewBody, reviewInput, reviewMountPaths, REVIEW_KIT_VERSION, diff --git a/packages/review-kit/src/review-kit.test.ts b/packages/review-kit/src/review-kit.test.ts index 6fc3997..63555c8 100644 --- a/packages/review-kit/src/review-kit.test.ts +++ b/packages/review-kit/src/review-kit.test.ts @@ -16,6 +16,7 @@ import { defineReviewPersona, prDiff, readPullRequest, + reviewBody, reviewInput } from './index.js'; import { createReviewHandler } from './agent.js'; @@ -651,3 +652,59 @@ async function withRelayfileTransportEnv( } } } + +test('reviewBody strips the model preamble and keeps the review', () => { + // Shape observed in production: the harness stdout carries the model's + // thinking, then the review. Only the review should reach the PR. + const output = [ + 'Evidence confirmed. The 8 probe commits added only comment lines.', + 'I have all evidence needed. Writing the review now.', + '', + '**Verdict: Blocker.** Second stat-math path outside lib/aggregates.ts.', + '', + '- **Blocker** — `lib/recentForm.ts:15` — duplicate math. Fix: use computeStandings.' + ].join('\n'); + + const body = reviewBody(output); + assert.ok(body.startsWith('**Verdict: Blocker.**'), 'must open on the verdict line'); + assert.ok(!body.includes('Evidence confirmed'), 'preamble must be gone'); + assert.ok(!body.includes('Writing the review now'), 'narration must be gone'); + assert.ok(body.includes('lib/recentForm.ts:15'), 'findings must survive'); +}); + +test('reviewBody cuts at the LAST verdict line so a drafted review loses to the real one', () => { + // A model that drafts before writing emits two verdict lines; the real + // review is the final one. + const output = [ + '**Verdict: Note.** draft — I might downgrade this.', + 'Actually, checking the published-only rule changes it.', + '**Verdict: Blocker.** ignores the published-only filter.' + ].join('\n'); + + assert.equal(reviewBody(output), '**Verdict: Blocker.** ignores the published-only filter.'); +}); + +test('reviewBody passes through a body with no verdict line', () => { + // The model ignored the format. A malformed review still beats silence — + // for an advisory agent, silence reads as approval. + assert.equal(reviewBody('no verdict here, just prose'), 'no verdict here, just prose'); + assert.equal(reviewBody(' '), ''); + assert.equal(reviewBody(''), ''); +}); + +test('reviewBody tolerates how the model bolds the verdict line', () => { + // The charter asks for one exact form and production emits it, but a strict + // pattern fails open in the worst way: an unmatched line publishes the whole + // preamble. Tolerate the variants LLMs actually drift to. + assert.equal(reviewBody('thinking\n**Verdict**: Blocker'), '**Verdict**: Blocker'); + assert.equal(reviewBody('thinking\n**Verdict:** Blocker'), '**Verdict:** Blocker'); + assert.equal(reviewBody('thinking\n**Verdict** : Blocker'), '**Verdict** : Blocker'); + assert.equal(reviewBody('thinking\n**verdict:** Blocker'), '**verdict:** Blocker'); + assert.equal(reviewBody('thinking\n**VERDICT**: Blocker'), '**VERDICT**: Blocker'); +}); + +test('reviewBody ignores a verdict mentioned mid-line, not at a line start', () => { + // The ^ anchor is what keeps prose about a verdict from being mistaken for one. + const prose = 'I think the **Verdict:** below is too harsh, reconsidering.'; + assert.equal(reviewBody(prose), prose); +});