Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/review-subject-restatement-and-body-stats.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"review": minor
---

Stop posting a subject line that restates the discussion, and stage body-size stats with every submission plan.

The 2026-08-20 by-version audit of webapp reviews found a prose repetition cluster in the v1.11.0-v1.13.0 windows (5 of 29 blind-judged bodies restated one fact two to four times, vs 1 of 60 before), and the v1.8.0 task-mode removal (#284/#288/#289) deleted the orchestrator rewrite pass that used to absorb subject/discussion overlap; `joinProse` concatenates the label contract's `subject` and `discussion` verbatim with no overlap check. This fixes the mechanical subset (a subject that token-for-token restates the discussion's lede); the audited fail bodies are mostly paraphrase-level restatement, which stays producer-side (finding-contract wording, tracked in PRA-46). `joinProse` now drops a subject whose folded tokens are all contained in the discussion's first sentence (stopwords ignored on the subject side, light inflection folding on both sides, markdown and trailing punctuation stripped); a subject restating a later sentence or carrying any token the first sentence lacks is kept whole. `buildClaims`' first-sentence split then recovers the discussion's own opening claim as the subject, so no downstream field goes empty, and the `failure_scenario` salvage for a dropped subject reads the discussion instead, keeping dedup's `comparedText` on its discussion branch. Expected output-shape effect: median inline comment bodies shrink by up to one sentence each (the drop removes exactly the duplicated subject line and adds nothing); review bodies move only where a PR-level claim folds in (`renderPrLevelFold` renders the same joined prose, and a dropped subject changes the one-line header the HOLD and over-cap collapsed lists print), shrinking by the same one duplicated sentence.

The same audit found the +60% median-body step (557 to 889 chars across webapp's v1.7.0 to v1.11.0 bump) shipped with no changeset naming it, and a by-version audit four versions later was the detection mechanism. `submission-plan.json` now carries `bodyStats` (comment count, median/p90/max/total rendered chars over the final comment bodies, footer rides included, plus the review body's length), echoed in the CLI's run-log summary, so the next render-path regression is visible in the first runs' artifacts; a copy of the plan is staged under `out/`, the one directory the run uploads. Artifact-only; nothing gates on it. The README's versioning section now requires render-path changesets to state their expected output-shape effect.
18 changes: 18 additions & 0 deletions workflows/review/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,24 @@ bump lands in the same Version Packages commit that gets tagged.
literals do not match the `review` package version (releases v1.3.0 through
v1.4.0 shipped still pointing at v1.2.2, before the sync existed).

### Output-shape changes belong in the changeset

A change to the render path (`lib/submission.ts`, `lib/render-comment.ts`,
`lib/dispatch-contracts.ts` prose composition) that alters what authors see
must say so in its changeset, including the expected direction of body-size
change. The v1.8.0 task-mode removal shipped a +60% median-body step (557 to
889 chars across webapp's v1.7.0 to v1.11.0 bump, sketch share 15.6% to 48.6%)
with changesets framed entirely as conformance wins; the regression went
unattributed for four versions until a by-version audit reconstructed it
(PRA-46). Each staged `submission-plan.json` now carries `bodyStats`
(per-comment median/p90/max/total chars and the review body's length), and a
copy of the plan is staged under `out/` so it rides the run's uploaded
artifact (Step 9's `upload-artifact` matches the staging-relative `out/**`;
the absolute `/tmp/gh-aw/review/out/**` pattern alongside it matches nothing
under gh-aw v0.81.6 and is kept only as future-proofing); compare a
release's first live artifacts against the prior version's when a render
change ships.

### Version attribution

Semver is the behavior contract: a release that changes the reviewer's behavior bumps
Expand Down
95 changes: 95 additions & 0 deletions workflows/review/lib/dispatch-contracts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,68 @@ describe("label-contract enforcement (run 29897276810)", () => {
expect(joinProse("Only a subject", "")).toBe("Only a subject");
expect(joinProse("", "Only a discussion.")).toBe("Only a discussion.");
});

it("drops a subject that restates the discussion's first sentence (PRA-46 W4-W5 repetition mode)", () => {
// Verbatim restatement: the discussion opens with the subject.
expect(
joinProse(
"The merger drops flagged turns.",
"The merger drops flagged turns whenever moderation flags a turn mid-stream.",
),
).toBe(
"The merger drops flagged turns whenever moderation flags a turn mid-stream.",
);
// Inflected restatement: "dropped" vs "drops" still folds together.
expect(
joinProse(
"Flagged turns are dropped by the merger.",
"The merger drops flagged turns because the filter runs before the merge, so a flagged turn never reaches the sink.",
),
).toBe(
"The merger drops flagged turns because the filter runs before the merge, so a flagged turn never reaches the sink.",
);
// Markdown wrapping does not defeat the comparison.
expect(
joinProse(
"`counts.go` recomputes the total.",
"counts.go recomputes the total on every call.",
),
).toBe("counts.go recomputes the total on every call.");
});

it("keeps a subject that carries information the opening sentence lacks", () => {
// Restating a LATER sentence keeps the subject: dropping it would
// make buildClaims recover the discussion's opening SETUP sentence
// as claim.subject, which renderPrLevelFold and the HOLD/over-cap
// collapsed lists print as the finding's one-line header.
expect(
joinProse(
"A delete leaves the stale entry behind.",
"The cache is written in save(). A delete leaves the stale entry behind.",
),
).toBe(
"A delete leaves the stale entry behind. The cache is written in save(). A delete leaves the stale entry behind.",
);
// "never invalidated" is not in the first sentence: kept whole.
expect(
joinProse(
"The cache is never invalidated.",
"The cache is written in save(). A delete leaves the stale entry behind.",
),
).toBe(
"The cache is never invalidated. The cache is written in save(). A delete leaves the stale entry behind.",
);
// A subject summarizing ACROSS sentences (no single sentence holds
// all its tokens) is a genuine lede and survives.
expect(
joinProse(
"save() caches, delete leaves it stale.",
"The cache is written in save(). A delete leaves the stale entry behind.",
),
).toBe(
"save() caches, delete leaves it stale. The cache is written in save(). A delete leaves the stale entry behind.",
);
});
});

describe("label-shape lens assignment", () => {
Expand Down Expand Up @@ -435,6 +497,39 @@ describe("verification mechanics", () => {
confidence: 0.7,
});
});

it("recovers the discussion's opening claim and a discussion-salvaged failure_scenario when the restatement drop fires (PRA-46)", () => {
const {candidates} = parseFinderOutput(
"correctness-reviewer",
JSON.stringify({
findings: [
{
path: "a.ts",
line: 2,
label: "issue (blocking)",
// Inflected restatement of the discussion's first
// sentence: joinProse drops it.
subject: "Flagged turns are dropped by the merger.",
discussion:
"The merger drops flagged turns. The filter runs before the merge.",
},
],
}),
new Set(),
);
const [claimed] = buildClaims(candidates);
// The HOLD/over-cap collapsed lists and renderPrLevelFold print
// claim.subject as the finding's one-line header: after the drop it
// is the discussion's own opening claim, never empty.
expect(claimed?.subject).toBe("The merger drops flagged turns.");
// The salvage skips the dropped subject: dedup's comparedText reads
// the discussion only when failure_scenario prefix-matches
// claim.subject, and the inflected subject would fail that test and
// compare the claim on one sentence plus its own restatement.
expect(claimed?.failure_scenario).toBe(
"The merger drops flagged turns. The filter runs before the merge.",
);
});
});

describe("applyVerifications: corrected-field validation", () => {
Expand Down
145 changes: 142 additions & 3 deletions workflows/review/lib/dispatch-contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,18 +87,146 @@ export type Candidate = {
authorDispute?: string;
};

/**
* Fold a prose token toward its stem so an inflection difference does not
* defeat the restatement check below ("drops" vs "dropped", "cache" vs
* "caches"). Deliberately crude: strip one of ing/ed/es/s, collapse a
* doubled final consonant ("dropped" -> "dropp" -> "drop"), then strip a
* trailing "e" so "caches" -> "cach" meets "cache" -> "cach". Both sides
* of every comparison fold identically, and a miss is safe — the subject
* is kept and the body merely stays as long as it is today.
*/
const foldToken = (token: string): string => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): ~110 lines of stemming/stopword machinery for a mode whose common case is a verbatim prefix. Three of the four positive tests are near-verbatim restatements; only one needs inflection folding. The repo already has a normalize-and-compare-prefix idiom for precisely "does this field restate that one" in dedup.ts's comparisonKey/comparedText.

A sketch, not a committable replacement:

Consider starting with normalized substring containment (`comparisonKey(subject)` contained in `comparisonKey(sentence)`) and adding the stemmer only if live artifacts show the inflected case is common enough to matter — the token-fold path can then be justified by measured misses rather than anticipated ones.
review details found by first-principles

if (token.length <= 3) {
return token;
}
let folded = token;
for (const suffix of ["ing", "ed", "es", "s"]) {
if (folded.endsWith(suffix) && folded.length - suffix.length >= 3) {
folded = folded.slice(0, folded.length - suffix.length);
break;
}
}
if (/([b-df-hj-np-tv-z])\1$/.test(folded)) {
folded = folded.slice(0, -1);
}
return folded.length >= 4 && folded.endsWith("e")
? folded.slice(0, -1)
: folded;
};

/**
* Function words that carry no claim content; ignored on the SUBJECT side
* of the restatement check so "turns are dropped" still matches "drops
* turns" (the sentence has no "are"). Never filtered from the sentence
* side — there they can only help containment, not hurt it. Distinct from
* dedup-text.ts's STOPWORDS (near-identical list, different semantics:
* that one filters both sides of a similarity score).
*/
const SUBJECT_STOPWORDS = new Set([
"a",
"an",
"the",
"is",
"are",
"was",
"were",
"be",
"been",
"being",
"it",
"its",
"this",
"that",
"these",
"those",
"and",
"or",
"of",
"to",
"in",
"on",
"by",
"for",
"with",
"as",
"at",
"so",
]);

/**
* The comparable word tokens of a prose fragment: markdown emphasis
* stripped, lowercased, internal punctuation kept (`counts.go` is one
* token) but trailing punctuation shed ("turns." and "turns" are the same
* word).
*/
const proseTokens = (text: string): string[] =>
(
text
.toLowerCase()
.replace(/[`*_]/g, "")
.match(/[a-z0-9][a-z0-9./:-]*/g) ?? []
).map((token) => token.replace(/[./:-]+$/, ""));

/**
* Whether the subject merely restates the discussion's FIRST sentence:
* every folded subject token already appears there, so prepending the
* subject adds repetition and no vocabulary. This is the mechanical
* subset of the prose repetition the 2026-08-20 version audit measured
* in v1.11.0-v1.13.0 bodies (5 of 29 sampled bodies restated one fact two
* to four times, vs 1 of 60 before): the v1.8.0 task-mode removal deleted
* the orchestrator rewrite pass that used to absorb subject/discussion
* overlap (PRA-46). Re-fetching the 5 audited fail bodies shows their
* restatement is mostly paraphrase (same fact, different vocabulary),
* which a token-containment check deliberately does not touch; that mode
* is producer-side (finding-contract wording, PRA-46 follow-up). This
* drop removes only the strict duplicate, where firing is provably safe.
*
* First-sentence-only is deliberate. When the drop fires, `buildClaims`'
* first-sentence split recovers the discussion's opening sentence as
* `claim.subject`, and that string is a visible header downstream (the
* HOLD_FOR_HUMAN and over-cap collapsed lists, `renderPrLevelFold`), so it
* must be the claim; matching a later sentence would leave setup prose
* there. A subject restating a later sentence, one summarizing across
* sentences, or one carrying any token the first sentence lacks is kept
* whole. The comparison is an unordered token bag, so a subject reusing
* the sentence's exact vocabulary to state a different relation would be
* dropped too; accepted, since the audited failure mode is restatement and
* the sentence carrying that vocabulary still posts.
*/
const subjectRestatesDiscussion = (
subject: string,
discussion: string,
): boolean => {
const subjectTokens = proseTokens(subject)
.filter((token) => !SUBJECT_STOPWORDS.has(token))
.map(foldToken);
if (subjectTokens.length === 0) {
return false;
}
const firstSentence = discussion.split(/(?<=[.!?])\s/, 1)[0] ?? "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): The sentence split discussion.split(/(?<=[.!?])\s/, 1)[0] breaks on an abbreviation's period, so a discussion whose opening sentence contains "e.g. "/"i.e. "/"vs. " yields a truncated first sentence — which can both fire the drop and then become claim.subject, the one-line header printed by renderPrLevelFold, the HOLD list, and the blocking-only collapsed list.

review details found by skill-auditor (out-of-lane) | also flagged by conventions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note (non-blocking): Description says "a single discussion sentence"; the code checks only the first sentence. The PR description states the drop fires when the subject's folded tokens "are all contained in a single discussion sentence" and that only "a subject carrying any token no single sentence holds is kept whole", but subjectRestatesDiscussion splits off just the first sentence and the new test at dispatch-contracts.test.ts:296-303 pins that a later-sentence restatement is kept whole. The narrowing is deliberate and well justified in the JSDoc (buildClaims must recover a claim, not setup prose) and the changeset states it correctly — only the PR description overstates the scope, leaving part of the audited repetition mode unfixed. (No Jira/Confluence access was available in this sub-agent, so KORE-2513 was not consulted; this is judged against the PR description alone.)

review details found by completeness

const sentenceTokens = new Set(proseTokens(firstSentence).map(foldToken));
return subjectTokens.every((token) => sentenceTokens.has(token));
};

/**
* Join the label contract's `subject` and `discussion` into one prose block.
* A subject with no terminal punctuation gets a sentence break, not a bare
* space (run 29897276810 posted "...memory Both TestExpiration..."); the
* break also keeps `buildClaims`' first-sentence split recovering the
* subject.
*
* A subject that restates the discussion's opening sentence
Comment thread
khan-actions-bot marked this conversation as resolved.
* ({@link subjectRestatesDiscussion}) is dropped instead of joined: the
* posted body then opens with the discussion's own first claim, and
* `buildClaims`' first-sentence split recovers that as the subject, so no
* downstream field goes empty; the body loses the duplicate sentence.
*/
export const joinProse = (subject: string, discussion: string): string => {
if (discussion === "") {
return subject.trim();
}
if (subject === "") {
if (subject === "" || subjectRestatesDiscussion(subject, discussion)) {
Comment thread
khan-actions-bot marked this conversation as resolved.
Comment thread
khan-actions-bot marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): The diagnosed root cause is a lost producer-side instruction, but the fix is entirely render-side. The description attributes the regression to v1.7.0's prompt guidance ("trim anything that isn't the problem or the fix") being deleted with the orchestrator pass, yet review.md's finding contracts still say only "subject": "one line", "discussion": "1-2 sentences, optional" (lines 1463, 2071, 2144, 2207, 2284, 2351, 2580) with nothing telling producers the subject must add vocabulary the discussion lacks — and this diff does not touch them. Was restoring one clause in the shared contract considered alongside (or instead of) the lexical filter, given the filter can only ever remove the single subject sentence while the measured mode is 2-4 restatements per body?

review details found by first-principles

return discussion.trim();
}
const trimmed = subject.trimEnd();
Expand Down Expand Up @@ -167,9 +295,20 @@ const fromLabelShape = (
// valid labels with only {id, anchor, discussion}; rejecting it for
// the missing failure_scenario voided the whole correctness
// dimension twice, which is strictly worse than validating against
// the discussion prose.
// the discussion prose. A subject joinProse drops (restatement)
// salvages from the discussion too: dedup's comparedText reads the
// discussion only when failure_scenario prefix-matches
// claim.subject (dedup.ts), and after the drop claim.subject is
// the discussion's first sentence, which prefix-matches the
// discussion itself but not an inflected or reordered dropped
// subject; salvaging that subject would compare the claim on one
// sentence plus its own restatement, the exact shape run
// 30301235749 failed to merge.
failure_scenario:
raw["failure_scenario"] ?? (subject !== "" ? subject : discussion),
raw["failure_scenario"] ??
(subject !== "" && !subjectRestatesDiscussion(subject, discussion)
? subject
: discussion),
producing_hunt: `dispatch:${agentName}`,
model_authored_prose: joinProse(subject, discussion),
// Suggestion salvage, like the anchor/subject salvage above: run
Expand Down
Loading
Loading