review: a non-blocking inline budget, and the collapsed tail names its best finding - #380
Conversation
…d the collapsed tail names its best finding
🦋 Changeset detectedLatest commit: dafcf08 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested — see inline comments.
suggestion (non-blocking, documentation): PR description explains the budget's value with an undefined shorthand, "Q8's fiat-now number".
Full finding
The description reads "The 3 is Q8's fiat-now number: at the measured 2.91 findings/run it binds rarely and backstops the wall-of-comments failure mode", and "Q8" names a decision the description never identifies, so a reader cannot tell what settled on 3. Plainly: "3 was chosen by fiat rather than measurement: at the observed 2.91 findings/run it binds rarely." ("PRA-7" a few lines up is recoverable from the opening paragraph, so it needs no change.)
review details
found by documentationreview details
review-v1.19.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| if (blockingOnly || claim.confidence < MIN_INLINE_CONFIDENCE) { | ||
| return false; | ||
| } | ||
| if (labelToken(claim.label) === "nitpick") { |
There was a problem hiding this comment.
issue (blocking): Suppressing nitpick (non-blocking) inline empties the nits autofix's input for that label — the exact harm the documentation exemption three lines below guards against. I traced the consumer: findingLabelsForScope("nits") (workflows/autofix/lib/scope.ts:237) returns NON_BLOCKING_LABELS, which includes nitpick (non-blocking) (render-comment.ts:56-64), and worklist.ts:80-107 selects work by parseLeadingLabel over posted threads only — nothing reads the collapsed <details> section. Introduced by this change: the reasoning that earned DOCUMENTATION_LABEL its exemption at line 658 applies verbatim to nitpick, which is suppressed unconditionally rather than merely budgeted.
Lower-confidence observations (1)
workflows/review/lib/submission.ts:662note (non-blocking):budgetLeftis spent inside the filter but the 20-comment cap is applied after it, so the shed count under-reports. (correctness-reviewer)
review details
found by correctness-reviewer| isBlockingLabel(claim.label) || | ||
| (!blockingOnly && claim.confidence >= MIN_INLINE_CONFIDENCE), | ||
| ); | ||
| let budgetLeft = nonBlockingBudget; |
There was a problem hiding this comment.
thought (non-blocking): The budget selects the 3 inline non-blocking slots by confidence, the key the PR says is the wrong one. Before this change every non-blocking claim at >=0.5 confidence posted inline up to 20; now the filter admits only the first 3 in confidence order, and the PR body states the severity tier that would order by importance is a later PR, while the new summary line names only the first of the collapsed entries (17 of them in the new test at submission-trial-followups.test.ts:162). Is there a reason to land the budget before that tier, rather than ship the collapsed-tail summary change now and the budget alongside the tier?
review details
found by first-principles| if (labelToken(claim.label) === "nitpick") { | ||
| return false; | ||
| } | ||
| if (claim.label === DOCUMENTATION_LABEL) { |
There was a problem hiding this comment.
question (non-blocking): The documentation exemption's justification is not documentation-specific. dedup-threads.ts suppresses re-posts by matching still-open bot threads, and a line in the collapsed <details> block is not a thread, so the posted-thread dependency the exemption protects the autofix from also applies to suppression and to human resolution. Should the carve-out be stated as "non-blocking findings stop being threads when the budget filters them" rather than as one downstream consumer's special case?
review details
found by first-principles… disclosure, subject in the top tag, nitpicks rank last
|
Pushed fcce9d8 addressing the review:
On the first-principles question (why frame the carve-out per consumer rather than "non-blocking findings stop being threads"): the general statement is true and #383 is the general fix; the per-consumer exemption here is deliberately temporary and dies there. On the thought about landing the budget before the tier: the tier is #382, stacked right on this, so the two tune together; sequencing them as separate PRs keeps this one deterministic-only. |
There was a problem hiding this comment.
Changes requested — see inline comments.
12 of 15 prior review threads resolved; 3 still unaddressed as of fcce9d8:
- issue (blocking)
workflows/review/lib/submission.ts: Suppressingnitpick (non-blocking)inline empties the nits autofix's input for that label — the exact harm the documen...
2 non-blocking threads still open
- thought (non-blocking)
workflows/review/lib/submission.ts:658: The budget selects the 3 inline non-blocking slots by confidence, the key the PR says is the wrong one. Before this chan... - question (non-blocking)
workflows/review/lib/submission.ts:672: The documentation exemption's justification is not documentation-specific. dedup-threads.ts suppresses re-posts by match...
Non-blocking observations (9)
workflows/review/lib/submission.ts:685suggestion (non-blocking): pr-level collapsed claims never join the ranking, so the namedtopcan be the weakest entry in the tail. (correctness-reviewer)workflows/review/lib/submission.ts:672suggestion (non-blocking): The documentation exemption is unbounded in code, so documentation findings can post inline up to the 20-comment cap. (correctness-reviewer)workflows/review/lib/submission.ts:639suggestion (non-blocking): The budget's zero value is pinned at the parser but never at the posting surface. (test-adequacy)workflows/review/README.md:406suggestion (non-blocking, documentation): Name theblocking-onlymodifier instead of "a reduced-depth modifier". (documentation)workflows/review/lib/render-comment.ts:61nitpick (non-blocking):NON_BLOCKING_LABELSstill re-spells "nitpick (non-blocking)" instead of referencing the newNITPICK_LABELconstant. (correctness-reviewer)workflows/review/lib/submission.ts:641question (non-blocking): The non-blocking inline budget ships with a default of 3, so every consumer's posting surface changes without adding anynon-blocking-budgetROUTING line, unlike there-reviewdial which ships inert atfulluntil a repo opts in. (skill-auditor (out-of-lane))workflows/review/lib/submission.ts:656thought (non-blocking): The summary's "top" entry is picked by confidence, not by how much the finding matters. (first-principles).changeset/pra7-posting-surface.md:2suggestion (non-blocking, best-practice):workflows/review/README.mdstates "Semver is the behavior contract: a release that changes the reviewer's behavior bumps the major version, so a consumer pinned toreview-v<major>can assume the fundamental behavior holds within a major", and this changeset ships the posting-surface change as"review": minor. (skill-auditor)- suggestion (non-blocking, documentation): The description uses two names it never defines: "PRA-7" and "quiet-the-human-surface lane". (documentation)
review details
review-v1.19.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| nitpickShed++; | ||
| return false; | ||
| } | ||
| if (claim.label === DOCUMENTATION_LABEL) { |
There was a problem hiding this comment.
issue (blocking): The budget shrinks the autofix: nits work list for the same reason the documentation label is exempted from it. This line exempts the documentation label because the docs autofix selects its work by parsing that label off posted threads, but autofix: nits is thread-sourced the same way (workflows/autofix/README.md:19, The reviewer's open non-blocking threads), so every non-blocking claim the budget sheds also leaves the nits scope. The README edit in this diff discloses only the nitpick ban as a cause of that shrinkage, never the budget.
A sketch, not a committable replacement:
| `autofix: nits` | The reviewer's open non-blocking threads (suggestions, nitpicks, questions, thoughts, notes). The reviewer's posting surface never posts `nitpick (non-blocking)` findings as inline threads, and posts at most `non-blocking-budget` (default 3) other non-blocking findings per review (review-v1.20+), so both nitpick-class items and over-budget findings reach this scope only once the work list also reads the review body's collapsed section. |
review details
found by holistic…tail re-ranks with pr-level claims, budget-zero pinned
|
Pushed d965e56 for the second round:
Declines, with reasoning: the default-of-3 shipping active rather than inert is the deliberate fiat-now decision this PR exists to make (the changeset states it; the collapsed tail plus disclosure is the safety net, and per-consumer ROUTING is the opt-out), and the top-by-confidence thought is answered by the stacked #382, which ranks medium-importance claims first. The documentation exemption's unboundedness is real but dies entirely in #383, so bounding it here would be dead code in the same release train. The semver observation is a fair catch of a doc/practice mismatch: the README says behavior changes bump major, while v1.7 (scoped default), v1.14, and v1.18 (the prose judge) all shipped behavior as minor; I've kept minor to match fleet practice and am flagging the mismatch to jwies rather than resolving it in this PR. |
There was a problem hiding this comment.
Approved — no blocking issues found.
4 of 4 prior review threads are still unaddressed as of d965e56:
- issue (blocking)
workflows/review/lib/submission.ts: Suppressingnitpick (non-blocking)inline empties the nits autofix's input for that label — the exact harm the documen... - issue (blocking)
workflows/review/lib/submission.ts:675: The budget shrinks theautofix: nitswork list for the same reason the documentation label is exempted from it. This l...
2 non-blocking threads still open (2 previously reported)
- thought (non-blocking)
workflows/review/lib/submission.ts:661 - question (non-blocking)
workflows/review/lib/submission.ts:675
Non-blocking observations (3)
workflows/review/lib/submission.ts:728suggestion (non-blocking): Thetop:rationale still says pr-level claims append after the ranked list, which line 692's re-sort makes false. (correctness-reviewer)workflows/review/lib/submission-trial-followups.test.ts:373suggestion (non-blocking): The file's docblock still enumerates only the post-trial follow-ups, not the two new budget describes. (conventions)workflows/review/lib/submission.ts:678suggestion (non-blocking): No test pins that the non-blocking budget spends on the highest-confidence claims. (test-adequacy)
review details
review-v1.19.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…ionale comment, docblock, budget spend-order test
…ce (section rides the top comment on this branch)
There was a problem hiding this comment.
Changes requested — see inline comments.
4 of 4 prior review threads are still unaddressed as of 0ecdf27:
- issue (blocking)
workflows/review/lib/submission.ts: Suppressingnitpick (non-blocking)inline empties the nits autofix's input for that label — the exact harm the documen... - issue (blocking)
workflows/review/lib/submission.ts:675: The budget shrinks theautofix: nitswork list for the same reason the documentation label is exempted from it. This l...
2 non-blocking threads still open (2 previously reported)
- thought (non-blocking)
workflows/review/lib/submission.ts:661 - question (non-blocking)
workflows/review/lib/submission.ts:675
Non-blocking observations (3)
workflows/review/lib/submission.ts:737suggestion (non-blocking): The named-top tag puts the claim's fullsubjectinto a one-line<summary>. (first-principles)workflows/review/lib/submission.ts:737nitpick (non-blocking): Top-entry tag renders the location without the backticks the section's own lines use. (conventions)workflows/review/lib/submission-trial-followups.test.ts:374suggestion (non-blocking): Budget tests land in the trial-followups file rather than a topic-named split. (conventions)
review details
review-v1.19.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…p subject; backtick its location
|
Pushed dafcf08 for the round-3 findings:
|
There was a problem hiding this comment.
Approved — no blocking issues found.
1 of 5 prior review threads resolved; 4 still unaddressed as of dafcf08:
- issue (blocking)
workflows/review/lib/submission.ts: Suppressingnitpick (non-blocking)inline empties the nits autofix's input for that label — the exact harm the documen... - issue (blocking)
workflows/review/lib/submission.ts:678: The budget shrinks theautofix: nitswork list for the same reason the documentation label is exempted from it. This l...
2 non-blocking threads still open (2 previously reported)
- thought (non-blocking)
workflows/review/lib/submission.ts:664 - question (non-blocking)
workflows/review/lib/submission.ts:678
Non-blocking observations (8)
workflows/review/lib/submission-trial-followups.test.ts:526suggestion (non-blocking): The test named "truncates a long one" never exercises the truncation branch it names. (correctness-reviewer)workflows/review/lib/submission.ts:743suggestion (non-blocking): The named-top tag can put a multi-line subject inside<summary>; escaping and the 120-char cap don't collapse the newline. (correctness-reviewer)workflows/review/lib/submission.ts:745suggestion (non-blocking): The named-top subject's 120-char truncation is never asserted. (test-adequacy)workflows/review/lib/submission.ts:743note (non-blocking): Shipped summary line carries the claim subject; the PR description's example shows location + label only. (completeness)workflows/review/lib/submission.ts:745nitpick (non-blocking): The subject truncation drops the.trimEnd()the repo's other display truncation uses. (conventions)workflows/review/lib/submission.ts:748suggestion (non-blocking): The named-top tag renders inline instead of in the claim-rendering module. (conventions)workflows/review/lib/submission.ts:743suggestion (non-blocking): The named-top escaping is undone by the sanitizer, so the collapse can still be broken open. (holistic)- suggestion (non-blocking, documentation): The description invents "the shed" and never defines it. (documentation)
review details
review-v1.19.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 0441b7c6a581 (10 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
2 participants
Turns out three of today's approving re-reviews hid their best findings behind "Non-blocking observations (N)" collapsed blocks: #367's tail led with a report that the acknowledgment feature's own reply guard never fires (verified real against the merged code), #371's with a line-wrapped bypass of the gh-aw-update ban, #366's with a calibration band that's documented but never asserted. A retroactive measurement over every collapsed entry since 08-10 (91 entries across 17 PRs) is in the plans repo as records/pra7-collapsed-tail-2026-08-24.md.
This is the deterministic half of the PRA-7 fix (the plans-repo task tracking this program; its lane doc, quiet-the-human-surface, owns the reader-attention budget design), four posting-surface changes and no model behavior:
non-blocking-budget <n>ROUTING line; a non-default value shows in the version footer. The 3 was chosen by fiat rather than measurement (the quiet-the-human-surface lane's standing decision): at the measured 2.91 findings/run it binds rarely and backstops the wall-of-comments failure mode (webapp#41440's 13 non-blocking inline comments).nitpick (non-blocking)never posts inline. Just the nitpick label: documentation suggestions are exempt from the budget entirely, because the documentation autofix selects its work by parsing that label off posted threads, so budgeting those would silently shrink a shipped feature's scope. A follow-up PR teaches the autofix to read the collapsed section, then the exemption goes.Mechanical notes: submission.ts blew its max-lines budget, so the claim renderer moved to submission-render.ts (re-exported, the dispatch-contracts precedent); same for the new router tests (router-non-blocking-budget.test.ts). The medium severity tier that decides which non-blocking findings deserve the slots is the next PR; this one keeps ranking on confidence only.
KORE-2464