review: drop a subject that restates the discussion; stage body-size stats - #365
review: drop a subject that restates the discussion; stage body-size stats#365jwbron wants to merge 3 commits into
Conversation
…the discussion; stage body-size stats (PRA-46)
🦋 Changeset detectedLatest commit: fab4730 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * 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 => { |
There was a problem hiding this comment.
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…rop to the first sentence; salvage failure_scenario from the discussion when it fires Addresses PR 365 review feedback: - Dropping the subject broke dedup's comparedText prefix test for the non-verbatim cases: the salvaged failure_scenario stayed the dropped subject, claim.subject became the discussion's first sentence, and the claim compared on one sentence plus its own restatement (run 30301235749's unmerged shape). The salvage now reads the discussion when the drop fires, keeping comparedText on its discussion branch. - subjectRestatesDiscussion now checks the FIRST sentence only, so the recovered claim.subject (a visible header in renderPrLevelFold and the HOLD/over-cap collapsed lists) is always the claim, never setup prose; a subject restating a later sentence is kept. This also makes joinProse's docstring accurate. Un-exported (no external consumer). - stagePlan writes a copy of the plan under out/, the only directory Step 9 uploads, so bodyStats actually rides the run artifact. - Tests: buildClaims end-to-end drop case (subject and failure_scenario after the drop), unordered computeBodyStats input, out/ copy. - Changeset states the expected output-shape direction (its own rule).
|
Pushed 36d3da7 addressing these.
Two declines:
On whether bodyStats closes the gap or restates the audit: the audit needed someone to notice a smell, pull artifacts across 4 versions, and reconstruct medians by hand. bodyStats makes the same number a one-line read in any single run's artifact (now that it actually rides the artifact) and in the run log, so the after-a-render-change comparison the README asks for is cheap enough to actually happen. It's detection infrastructure, not detection; the README rule is the part that says when to look. |
There was a problem hiding this comment.
Changes requested — see inline comments.
8 of 9 prior review threads resolved; 1 still unaddressed as of 36d3da7:
1 non-blocking thread still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-contracts.ts:99: ~110 lines of stemming/stopword machinery for a mode whose common case is a verbatim prefix. Three of the four positive...
Full finding
The description's W-window shorthand is never defined. "Root-causes PRA-46's W3-W4 verbosity regression" and "5 of 29 blind-judged W4-W5 bodies restated one fact 2-4 times, vs 1 of 60 in W0-W3" use a W-numbering that appears nowhere in this repo outside this PR's own new code comment (dispatch-contracts.ts:172-174) and is never defined, while the same periods are elsewhere named by version (v1.7.0 to v1.11.0). Either define the windows once at first use ("W(n) = the nth weekly audit window; W4-W5 span v1.11.0-v1.13.0") or use the version ranges the rest of the description already uses — e.g. "5 of 29 blind-judged bodies from v1.11.0-v1.13.0 restated one fact 2-4 times, vs 1 of 60 from v1.0.0-v1.10.0".
review details
found by documentationreview details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation|
|
||
| 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 root cause is mechanical: `joinProse` concatenates the label contract's `subject` and `discussion` verbatim, producers routinely emit a subject that restates a discussion claim, and the v1.8.0 task-mode removal (#284/#288/#289) deleted the orchestrator rewrite pass that used to absorb the overlap. `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 are unchanged. |
There was a problem hiding this comment.
issue (blocking, best-practice): Changeset must state the render change's output-shape effect on every surface it alters. workflows/review/README.md (the review-workflow-contract skill file), in the section this PR itself adds, states the rule: "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 changeset's stated effect is: "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 are unchanged."
That last clause is wrong, so the review-body surface gets no statement of its direction — it gets a denial. joinProse has exactly one caller, model_authored_prose: joinProse(subject, discussion) (dispatch-contracts.ts:307), and buildClaims sets discussion: prose from that same field (dispatch-contracts.ts:666). The review body renders PR-level claims through renderPrLevelFold, which folds claim.discussion verbatim under 400 chars and prints claim.subject as the header above it otherwise (submission.ts:281-293, called at submission.ts:599); the blocking-only collapsed <details> list and the hold-comment claim list print claim.subject one-liners too. So when the restatement drop fires on a PR-level finding, the review body loses the duplicated sentence and its header string changes — and PR-level folds are live for both known consumers, since documentation is an enabled reviewer and its "title/description finding carries no path/line and posts PR-level, folded into the review body" (README). The new bodyStats.bodyChars field this same PR stages exists precisely to measure that number.
Fix: state the review-body direction instead of denying it — e.g. "review bodies shrink only where a PR-level finding folds in (rare), by the same one duplicated sentence; inline comments are where the median moves."
A sketch, not a committable replacement:
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 finding folds in (`renderPrLevelFold`) or a claim's subject heads a collapsed one-liner, shrinking by the same one duplicated sentence.
Lower-confidence observations (1)
workflows/review/lib/dispatch-contracts.ts:201thought (non-blocking): First-sentence-only exists to protect a derived header, and deliberately lets the target failure mode through. (first-principles)
review details
found by skill-auditor | also flagged by holistic| * Write the staged plan (`submission-plan.json`) and hand it back. A second | ||
| * copy lands under `out/` because that is the only directory the run | ||
| * uploads (review.md Step 9's `upload-artifact` allows | ||
| * `/tmp/gh-aw/review/out/**` and nothing else): the copy is what makes |
There was a problem hiding this comment.
suggestion (non-blocking, documentation): stagePlan's comment cites the upload pattern review.md says never matches. The new comment says the copy lands under out/ because "review.md Step 9's upload-artifact allows /tmp/gh-aw/review/out/** and nothing else", but review.md:136-138 allows two patterns and its own note at :127 says "An absolute pattern like "/tmp/gh-aw/review/out/**" therefore matches nothing, ever" — the staging-relative out/** is the one that matches. The README's new section repeats the same claim ("Step 9 uploads only /tmp/gh-aw/review/out/**") and needs the same correction.
A sketch, not a committable replacement:
* Write the staged plan (`submission-plan.json`) and hand it back. A second
* copy lands under `out/` because that is the only directory the run
* uploads (review.md Step 9's `upload-artifact` allows the
* staging-relative `out/**`, plus an absolute `/tmp/gh-aw/review/out/**`
* kept only as future-proofing): the copy is what makes
review details
found by documentation| if (subjectTokens.length === 0) { | ||
| return false; | ||
| } | ||
| const firstSentence = discussion.split(/(?<=[.!?])\s/, 1)[0] ?? ""; |
There was a problem hiding this comment.
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| if (subjectTokens.length === 0) { | ||
| return false; | ||
| } | ||
| const firstSentence = discussion.split(/(?<=[.!?])\s/, 1)[0] ?? ""; |
There was a problem hiding this comment.
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 json = JSON.stringify(staged, null, 2); | ||
| fs.writeFileSync(`${REVIEW_DIR}/submission-plan.json`, json); | ||
| fs.mkdirSync(`${REVIEW_DIR}/out`, {recursive: true}); | ||
| fs.writeFileSync(`${REVIEW_DIR}/out/submission-plan.json`, json); |
There was a problem hiding this comment.
suggestion (non-blocking): bodyStats is staged into out/ but never wired into the counters pipeline that is the repo's actual by-version detection mechanism. counters-report.ts already aggregates per-run out/ artifacts by run into the weekly report it describes as the "wire the counters somewhere someone looks" half of counters.ts, and DEFAULT_RUN_ARTIFACT_LAYOUT already lists out/dispatch-result.json and out/rereview-plan.json — the same shape this change stages. Stopping at the artifact leaves the detection story dependent on a human remembering to diff JSON, which is the failure mode the PR's own framing indicts.
A sketch, not a committable replacement:
// counters.ts: add the plan to the layout so the weekly report can carry
// a median/p90 body-size counter alongside the cost and verdict ones.
export const DEFAULT_RUN_ARTIFACT_LAYOUT: RunArtifactLayout = {
claims: "claims.json",
validator: "out/claim-validator.json",
summary: "summary.json",
rereviewPlan: "out/rereview-plan.json",
dispatchResult: "out/dispatch-result.json",
submissionPlan: "out/submission-plan.json",
};
review details
found by holistic| return subject.trim(); | ||
| } | ||
| if (subject === "") { | ||
| if (subject === "" || subjectRestatesDiscussion(subject, discussion)) { |
There was a problem hiding this comment.
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| * turns" (the sentence has no "are"). Never filtered from the sentence | ||
| * side — there they can only help containment, not hurt it. | ||
| */ | ||
| const STOPWORDS = new Set([ |
There was a problem hiding this comment.
note (non-blocking): Second module-level STOPWORDS in the same lib, alongside dedup-text.ts's tokenizer. dedup-text.ts is the package's declared home for this primitive — "The text-similarity primitives every dedup tier scores with: content tokenization (lowercased alphanumerics, stopwords and short words dropped)" — and already defines const STOPWORDS = new Set("the a an and or of to in is are was be for on with that this it as not no by at from so its their they".split(" ")) (dedup-text.ts:11), which overlaps the new const STOPWORDS = new Set([...]) at dispatch-contracts.ts:124 on nearly every entry. The new list's subject-side-only semantics are genuinely different from dedup's, so co-locating may be wrong — but a distinguishing name (e.g. SUBJECT_STOPWORDS) would keep the two greppable apart.
review details
found by conventions| const json = JSON.stringify(staged, null, 2); | ||
| fs.writeFileSync(`${REVIEW_DIR}/submission-plan.json`, json); | ||
| fs.mkdirSync(`${REVIEW_DIR}/out`, {recursive: true}); | ||
| fs.writeFileSync(`${REVIEW_DIR}/out/submission-plan.json`, json); |
There was a problem hiding this comment.
note (non-blocking): The new out/ copy of the submission plan is unmentioned in the PR description. The duplicate write (and the new mkdirSync) is documented in the changeset and the README addition, but the PR description's bullet describes only the added bodyStats field and the CLI log echo. Worth naming in the description since it changes what the uploaded run artifact contains, not just the plan's shape.
review details
found by completeness…view-body claim and the upload-pattern citations; scope the restatement claim to the mechanical subset The changeset denied a review-body effect, but renderPrLevelFold renders the same joined prose, so a dropped subject moves PR-level folds and the one-line headers the HOLD and over-cap lists print; state the direction instead. The stagePlan comment and README cited the upload pattern review.md says matches nothing (the staging-relative out/** is the one that matches under gh-aw v0.81.6). Re-fetching the 5 audited W4-W5 repetition bodies shows their restatement is paraphrase-level, which the token-containment drop does not fire on (0 of 5, under both this predicate and plain normalized-substring containment), so the JSDoc and changeset now claim only the mechanical subset; the paraphrase mode stays producer-side (PRA-46 follow-up). STOPWORDS renamed to SUBJECT_STOPWORDS to stay greppable apart from dedup-text.ts's set.
|
Addressed in fab4730, plus one finding that reshapes the answers to the open design questions. The finding first: the task log admitted the joinProse attribution was "named from the diff, not confirmed against posted W4 bodies", so I re-fetched the 5 blind-judged repetition bodies the audit names (3754335178, 3694218854, 3768804982, 3769221714, 3762664308) and ran both predicates over every sentence pair. Neither fires on any of them, 0 of 5, for the shipped token-containment check and for plain normalized-substring containment alike. The audited restatement is paraphrase (e.g. 3762664308 states "no doc comment" 3 times with different vocabulary each time). So the drop fixes the strict mechanical duplicate, where firing is provably safe, and the audited mode is producer-side. The changeset, JSDoc, and PR description now claim only that subset. On the individual threads:
|
|
Holding for human review — the automated review could not complete safely this run. 5 non-blocking threads still open (1 previously reported)
|
2 participants
Root-causes PRA-46's verbosity regression (the v1.7.0 to v1.11.0 window) and fixes the render-side part that's mechanically self-inflicted.
The 2026-08-20 by-version audit found median finding bodies jumped 557 to 889 chars (+60%) across webapp's v1.7.0 to v1.11.0 bump, with a prose repetition cluster right after (5 of 29 blind-judged bodies from v1.11.0-v1.13.0 restated one fact 2-4 times, vs 1 of 60 from v1.0.0-v1.10.0). Turns out no model drifted: the v1.8.0 task-mode-to-scripted migration (#282/#284/#288/#289, all landing on webapp at once at the v1.11.0 bump) deleted the orchestrator's editorial pass. v1.7.0's prompt said "one or two sentences; trim anything that isn't the problem or the fix" and gated suggestion fences on "only when the fix is clear"; renderClaimComment posts the claim prose verbatim and the #288 sketch fallback preserves every non-drop-in suggestion, so what the orchestrator used to drop now posts (sketch share 15.6% to 48.6%). The changesets framed all of it as conformance wins, so nothing named the output-size effect.
One scoping correction from review: re-fetching the 5 audited repetition bodies shows their restatement is mostly paraphrase (same fact, different vocabulary each time), which no token-level check fires on (0 of 5 under both the predicate here and plain normalized-substring containment). So the joinProse drop below fixes the strict mechanical duplicate, where firing is provably safe; the paraphrase mode is producer-side and gets a finding-contract wording follow-up (tracked in PRA-46).
Two changes here, plus a doc note:
Not here: the sketch-length cap (the remaining owner of the +332 median chars). Truncating code mid-function can be worse than the wall of code it replaces, so picking truncate-vs-drop and N wants a read of live sketches first; it'll be its own PR, along with wiring bodyStats into the counters report.
Tests: new joinProse restatement/lede cases, computeBodyStats percentile pinning, plan-level bodyStats assertions; 1888 green, lint and typecheck clean.
Tracked as PRA-46 in ~/khan/plans/pr-review-agent.
KORE-2513