From c3d7906844196c68c2f6d18643721204ff5949da Mon Sep 17 00:00:00 2001 From: Ricky Schema Cascade Date: Fri, 17 Jul 2026 13:28:54 +0200 Subject: [PATCH 1/2] feat(review-kit): strip the model's preamble from the review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit defineReviewAgent posted `run.output.trim()` — the harness's raw stdout, which 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." Every kit-built reviewer publishes the search alongside the findings. This cannot be fixed in a charter, and #281's trap list did not include it because I only learned it after filing. Measured on a real reviewer against a 150-word cap: 537 words, then 256 once the ban was sharpened, then 191, then 187 after moving the entire output contract to the top of the charter where it is read first. Four rewrites of increasing severity bought 4 words. Stripping in code took the same reviewer to 153 on the next run. The reason is simple in hindsight: you cannot instruct a model not to think, only decline to publish the thinking. So the charter owns the SHAPE — which it gets right reliably, every review nailed the finding format — and the kit owns the BOUNDARY. Same reason agents/review strips its own trailing READY sentinel instead of trusting the model to omit it. reviewBody() cuts at the LAST verdict line: a model that drafts before writing emits two, and the real review is the final one. No verdict line means the model ignored the format — pass it through rather than post nothing, since for an advisory agent silence reads as approval. Exported so a lens with a bespoke handler gets the same boundary. Tests cover the three cases: preamble stripped, last-verdict-wins, and passthrough/empty. Note: `pnpm --filter @agentworkforce/review-kit typecheck` and test `traps 2-4` already fail on pristine origin/main in a local checkout — both assert trigger `paths`, which needs persona-kit >= 4.1.26 (#276). Unrelated to this change; verified by stashing it. Co-Authored-By: Claude Opus 4.8 --- packages/review-kit/src/agent.ts | 38 +++++++++++++++++++- packages/review-kit/src/index.ts | 1 + packages/review-kit/src/review-kit.test.ts | 40 ++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/packages/review-kit/src/agent.ts b/packages/review-kit/src/agent.ts index dd82861..14a8c1a 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,42 @@ 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. + * + * 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:/gmu)) { + 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..2dc309f 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,42 @@ 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(''), ''); +}); From 668ffbba23f9eef5f3b51672127ddb6acf40b6ad Mon Sep 17 00:00:00 2001 From: Ricky Schema Cascade Date: Fri, 17 Jul 2026 13:32:11 +0200 Subject: [PATCH 2/2] review-kit: tolerate how the model bolds the verdict line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From @gemini-code-assist's review: /^\*\*Verdict:/ assumed one exact format. Verified the concern is real — the old pattern misses three variants models actually produce: **Verdict**: Blocker (colon outside the bold) -> MISS **Verdict** : Blocker (space before colon) -> MISS **verdict:** Blocker (casing) -> MISS **Verdict: Blocker.** (what production emits) -> match A miss is not a graceful degradation here: an unmatched line publishes the entire preamble, which is the one thing this function exists to prevent. Being lax costs nothing; being strict costs the whole feature on a formatting wobble. The charter asks for one form and the production reviewer emits it — but the point of stripping in code was precisely that the model's output is not something you can legislate. The ^ anchor still does the real work: a verdict mentioned mid-sentence in prose is not matched. Covered by a test. Tests for all five variants plus the prose case, per the review. Co-Authored-By: Claude Opus 4.8 --- packages/review-kit/src/agent.ts | 9 ++++++++- packages/review-kit/src/review-kit.test.ts | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/review-kit/src/agent.ts b/packages/review-kit/src/agent.ts index 14a8c1a..22e0759 100644 --- a/packages/review-kit/src/agent.ts +++ b/packages/review-kit/src/agent.ts @@ -265,6 +265,13 @@ function createParsedReviewHandler( * 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. @@ -272,7 +279,7 @@ function createParsedReviewHandler( export function reviewBody(output: string): string { const text = (output ?? '').trim(); let start = -1; - for (const match of text.matchAll(/^\*\*Verdict:/gmu)) { + for (const match of text.matchAll(/^\*\*Verdict(?:\*\*)?\s*:/gimu)) { start = match.index ?? start; } return start >= 0 ? text.slice(start).trim() : text; diff --git a/packages/review-kit/src/review-kit.test.ts b/packages/review-kit/src/review-kit.test.ts index 2dc309f..63555c8 100644 --- a/packages/review-kit/src/review-kit.test.ts +++ b/packages/review-kit/src/review-kit.test.ts @@ -691,3 +691,20 @@ test('reviewBody passes through a body with no verdict line', () => { 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); +});