review: a COMMENT middle verdict when medium findings post and nothing blocks - #384
Conversation
🦋 Changeset detectedLatest commit: 26a1fcb 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 |
3440940 to
fe54dc4
Compare
fe54dc4 to
335b53a
Compare
![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.
question (non-blocking): The consumer-facing contract doc is not updated for the fourth verdict: it still says the workflow "submits an approve / request-changes review", describes the dispatch gate as enforcing "no blocking inline comment under an APPROVE", and names blocking-only the rollback dial "if the medium tier inflates on a consumer" — which no longer restores approvals, since collapsed mediums still demote.
review details
found by skill-auditor (out-of-lane)review details
review-v1.19.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| // A run with zero blocking labels is never REQUEST_CHANGES (review.md | ||
| // Step 4) — unless a prior review's blocking thread is still open, in | ||
| // which case the earlier objection is the actionable feedback. | ||
| // (f) Medium-importance claims — the PRA-7 middle verdict's signal. |
There was a problem hiding this comment.
suggestion (non-blocking): Signal (f) is appended to the precedence comment instead of joining signals (a)–(e). Every other signal in this function opens its own block above its own code — e.g. line 223 // (e) Kept blocking threads — the re-review flip rule. A still-open directly precedes const keptBlocking = input.keptBlockingCount ?? 0; — but the new // (f) Medium-importance claims — the PRA-7 middle verdict's signal. is glued to the tail of the // Precedence: a blocking finding is actionable on its own, so it wins. paragraph, splitting that paragraph from its if. Moving the (f) block up to sit after (e) restores both the lettered-signal section and the precedence comment's adjacency to its branch.
A sketch, not a committable replacement:
// (e) Kept blocking threads — the re-review flip rule. A still-open
// blocking objection from an earlier review blocks like a fresh one.
const keptBlocking = input.keptBlockingCount ?? 0;
if (keptBlocking > 0) {
reasons.push({code: "kept-blocking-thread", count: keptBlocking});
}
// (f) Medium-importance claims — the PRA-7 middle verdict's signal.
const mediumCount = input.mediumCount ?? 0;
if (mediumCount > 0) {
reasons.push({code: "medium-importance", count: mediumCount});
}
// Precedence: a blocking finding is actionable on its own, so it wins.
// A run with zero blocking labels is never REQUEST_CHANGES (review.md
// Step 4) — unless a prior review's blocking thread is still open, in
// which case the earlier objection is the actionable feedback.
if (
review details
found by conventions | also flagged by documentation: The new `(f)` block splits the precedence comment from the `if` it introduces.| // count the notes line records. Collapsed mediums count too; the | ||
| // verdict follows what the run FOUND, not which surface showed it | ||
| // (the same invariant that keeps a 21st blocking claim blocking). | ||
| mediumCount, |
There was a problem hiding this comment.
suggestion (non-blocking): The eval harness's copy of the verdict path was not updated with the medium signal. submission.ts now feeds mediumCount into computeVerdict, but eval/runner.ts — the offline replica that plans the same review — has no notion of importance at all (grep for "importance" under workflows/review/eval returns nothing), so its planned event diverges from what the shipped path would submit for the same case.
A sketch, not a committable replacement:
// eval/runner.ts: derive the post-veto medium count from the posted
// candidates and pass it through, and widen PlannedReview["event"] /
// submitEvent to include "COMMENT", so the eval keeps replicating the
// verdict the production path computes.
review details
found by holistic…t stops denying the verdict, counters gain COMMENT
|
Pushed a4243bc for the first round; both blocking findings were right:
Also taken: the eval runner feeds mediumCount so the offline replica plans the same event (pre-veto, noted: the harness has no changed-lines veto and fixtures model a vetoed medium as advisory); gate rule 5's flip veto covers COMMENT the way verdict.ts's floor does; the head reads "findings found" instead of "posted" (true under blocking-only collapse too); a cache-record COMMENT test; signal (f) gets its own block and the docstring re-wrap; and the consumer README's contract text now describes the three-event verdict, the gate's APPROVE-or-COMMENT rule, and the honest rollback story (blocking-only quiets the inline surface only; the verdict follows the medium count, so sustained inflation is a calibration fix, not a ROUTING dial). On the step 7/8 question: taken for step 8, which now runs on APPROVE or COMMENT (a COMMENT run found something worth human eyes and forces no later run that would route them); step 7 stays APPROVE-only since the guidance comment is a notification-surface cost and the COMMENT body already carries the findings. |
|
/review |
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
…t stops denying the verdict, counters gain COMMENT
a4243bc to
932a7bc
Compare
There was a problem hiding this comment.
Changes requested — see inline comments.
7 of 9 prior review threads resolved; 2 still unaddressed as of 932a7bc:
2 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/submission.ts:587: The eval harness's copy of the verdict path was not updated with the medium signal. submission.ts now feeds mediumCount... - suggestion (non-blocking)
workflows/review/lib/verdict.ts: Signal (f) is appended to the precedence comment instead of joining signals (a)–(e). Every other signal in this function...
review details
review-v1.19.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| @@ -0,0 +1,7 @@ | |||
| --- | |||
| "review": minor | |||
There was a problem hiding this comment.
suggestion (non-blocking, best-practice): Versioning: semver as the behavior contract for the review package. The README's Versioning section states the rule: "Semver is the behavior contract: a release that changes the reviewer's behavior bumps the major version, so a consumer pinned to review-v<major> can assume the fundamental behavior holds within a major." This changeset declares "review": minor for a change that alters the review event the workflow submits (APPROVE becomes COMMENT whenever the post-veto medium count is nonzero). I checked workflows/review/CHANGELOG.md: prior minors did carry behavior changes (v1.17.0's downvote adjudication, v1.19.0's adjudicated-corpus key drop), so minor is the house habit — but none of them changed the set of GitHub review events a consumer receives, which is the surface that sentence protects. Bump the major, or record in the changeset why a new submitted review event is within-major?
A sketch, not a committable replacement:
---
"review": major
---
review details
found by skill-auditor|
|
||
| const isVerdictEvent = (value: unknown): value is VerdictEvent => | ||
| value === "APPROVE" || | ||
| value === "COMMENT" || |
There was a problem hiding this comment.
suggestion (non-blocking): No test drives a COMMENT run through the verdict histogram. grep -n COMMENT counters.test.ts counters-report.test.ts returns nothing: the only new assertion is the rendered table string | 1 | 0 | 0 | 1 |, which pins the COMMENT column at zero and never exercises the two predicates that would put a nonzero there. Since the PR description makes this histogram the day-one rollout instrument, it is worth one end-to-end case.
A sketch, not a committable replacement:
// counters.test.ts
it("tallies a COMMENT run under its own bucket, not the hold fallback", () => {
const run = normalizeRunArtifacts({summary: {verdict: "COMMENT"}}, "r");
expect(run.verdict).toBe("COMMENT");
const counters = computeRunCounters([runOf({verdict: "COMMENT"})]);
expect(counters.verdictMix.COMMENT).toBe(1);
expect(counters.verdictMix.HOLD_FOR_HUMAN).toBe(0);
});
// counters-report.test.ts
it("derives a COMMENT verdict from the emitted submission", () => {
const summary = synthesizeSummaryFromGhAw({
safeOutputs:
'{"type":"submit_pull_request_review","event":"COMMENT"}',
});
expect(summary["verdict"]).toBe("COMMENT");
});
review details
found by test-adequacy| // submit. The harness has no changed-lines veto (fixtures are | ||
| // hand-anchored), so this is the pre-veto count; a fixture that | ||
| // wants a vetoed medium models it as advisory. | ||
| mediumCount: postedCandidates.filter( |
There was a problem hiding this comment.
suggestion (non-blocking): The eval harness's mediumCount mirror is never exercised. grep -rho 'severity": "[a-z]*' eval/corpus yields only advisory (23) and blocking (48), and grep -rn medium eval/ outside runner.ts hits only confidence prose in smoke.test.ts — so no fixture or synthetic case ever gives this filter a nonzero count. suite.test.ts's case builder already makes adding one cheap.
A sketch, not a committable replacement:
// suite.test.ts, alongside the existing verdict cases
it("a posted medium finding plans the COMMENT middle verdict", () => {
const {result} = runOne(
caseWith({
findings: [{id: "m", severity: "medium" /* ...case fields */}],
expected: {verdict: "COMMENT"},
}),
);
expect(result.verdict.event).toBe("COMMENT");
expect(result.plannedReview.event).toBe("COMMENT");
});
review details
found by test-adequacy
|
Pushed 9b9f49a; the two design blockers were both correct and both taught me GitHub semantics I'd reasoned past:
|
There was a problem hiding this comment.
Approved — no blocking issues found.
7 of 13 prior review threads resolved; 6 still unaddressed as of 9b9f49a:
6 non-blocking threads still open (2 previously reported)
- suggestion (non-blocking, best-practice)
.changeset/pra7-comment-verdict.md:2: Versioning: semver as the behavior contract for the review package. The README's Versioning section states the rule: "Se... - suggestion (non-blocking)
workflows/review/eval/runner.ts:473: The eval harness's mediumCount mirror is never exercised.grep -rho 'severity": "[a-z]*' eval/corpusyields only `advi... - suggestion (non-blocking)
workflows/review/lib/counters.ts:385: No test drives a COMMENT run through the verdict histogram.grep -n COMMENT counters.test.ts counters-report.test.tsr... - question (non-blocking)
workflows/review/lib/verdict.ts:255: Does a COMMENT review actually withdraw an approval a prior run already submitted? On GitHub a COMMENTED review does not... - suggestion (non-blocking)
workflows/review/lib/submission.ts:587 - suggestion (non-blocking)
workflows/review/lib/verdict.ts
Non-blocking observations (11)
workflows/review/lib/submission.ts:894suggestion (non-blocking): The upgrade note goes toplan.notes, which never reaches the review body, so the upgraded APPROVE explains nothing to the author. (correctness-reviewer)workflows/review/lib/verdict.ts:239suggestion (non-blocking, documentation): Added comment repeats the sentence 11 lines above, and strands the original over unrelated code. (documentation)workflows/review/lib/counters-report.ts:122suggestion (non-blocking):inon an object literal also matches inheritedObject.prototypekeys, so a junk event can be latched as the verdict and then block every real one. (correctness-reviewer)workflows/review/lib/submission.ts:895note (non-blocking): The upgrade's "note" lands in the plan artifact, not in the posted review body. (completeness)workflows/review/lib/counters-report.ts:117suggestion (non-blocking): The new three-value verdict rank has no test that feeds it a COMMENT event. (test-adequacy)workflows/review/lib/counters-report.ts:115nitpick (non-blocking): constant lookup table declared inside the loop instead of at module scope. (conventions)workflows/review/lib/counters-report.ts:112suggestion (non-blocking, documentation): New comment restates the tie-break rule already stated above the loop and the rank map below it. (documentation)workflows/review/lib/submission.ts:889suggestion (non-blocking): The prior-state guard is one-sided: it rescues a stale REQUEST_CHANGES but leaves a stale APPROVE standing under a COMMENT. (holistic)workflows/review/lib/submission.ts:891question (non-blocking): The new upgrade guard depends onpriorStamp, whose only production carrier is the GitHub Actions cache-memory record, so an evicted cache silently disables the guard and the run submits COMMENT while the bot's own prior REQUEST_CHANGES state stays on the PR. (skill-auditor (out-of-lane))workflows/review/lib/verdict.ts:232nitpick (non-blocking): the (f) signal block sits inside the precedence prose rather than after (e). (conventions)- suggestion (non-blocking, documentation): The description states the verdict precedence twice, and the two statements disagree. (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
…t stops denying the verdict, counters gain COMMENT
9b9f49a to
917e8c0
Compare
…t stops denying the verdict, counters gain COMMENT
917e8c0 to
540bf1b
Compare
…dium findings post and nothing blocks
…t stops denying the verdict, counters gain COMMENT
… a COMMENT never strands the bot's own prior block
540bf1b to
26a1fcb
Compare
There was a problem hiding this comment.
Changes requested — see inline comments.
6 of 6 prior review threads are still unaddressed as of 26a1fcb:
6 non-blocking threads still open (6 previously reported)
- suggestion (non-blocking, best-practice)
.changeset/pra7-comment-verdict.md:2 - suggestion (non-blocking)
workflows/review/eval/runner.ts:473 - suggestion (non-blocking)
workflows/review/lib/counters.ts:345 - suggestion (non-blocking)
workflows/review/lib/submission.ts:587 - suggestion (non-blocking)
workflows/review/lib/verdict.ts - question (non-blocking)
workflows/review/lib/verdict.ts:255
Non-blocking observations (1)
workflows/review/lib/submission.ts:894suggestion (non-blocking): The COMMENT→APPROVE upgrade posts no body trace of the demotion. (holistic)
review details
review-v1.19.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| const commentWouldStrandPriorRc = | ||
| verdict.event === "COMMENT" && | ||
| priorStamp !== null && | ||
| priorStamp.verdict === "REQUEST_CHANGES"; |
There was a problem hiding this comment.
todo (blocking): The COMMENT→APPROVE upgrade guard is only tested on the branch it takes. In submission-blocking-medium.test.ts the sole prior-stamp case ("upgrades COMMENT to APPROVE over a prior REQUEST_CHANGES stamp") writes verdict: "REQUEST_CHANGES", and the other COMMENT cases stage no stamp at all, so priorStamp is null there; the non-null-but-not-REQUEST_CHANGES stamp — the ordinary second run on a PR that was approved once — has no case, so deleting the priorStamp.verdict === "REQUEST_CHANGES" clause at submission.ts:891-892 would still pass the suite. Adding the APPROVE-stamp variant pins that only a prior block upgrades.
A sketch, not a committable replacement:
it("keeps COMMENT when the prior stamp is not a request-changes", () => {
const files = staged({
depth: "scoped",
claims: [claim({id: "medium", importance: "medium"})],
});
files[`${REVIEW}/pr-context.json`] = JSON.stringify({number: 7});
files[`/tmp/gh-aw/cache-memory/pr-7.json`] = JSON.stringify({
verdict: "APPROVE",
wasDraft: false,
reviewedHunks: {"a.ts": ["h1"]},
});
const plan = runSubmissionCli(makeFakeFs(files));
expect(plan.event).toBe("COMMENT");
});
review details
found by test-adequacy2 participants
Stacked on #383 (under it #382 and #380). The verdict half of the medium tier, decided in session: an approval shouldn't ride on top of findings the run itself verified as worth fixing before merge, so a run with medium findings and nothing blocking now submits a COMMENT review instead of an APPROVE.
The verdict stays mechanical, one new branch in lib/verdict.ts with precedence REQUEST_CHANGES > COMMENT > HOLD_FOR_HUMAN > APPROVE:
Plumbing: the stamp and cache-record machinery accept the new verdict (a COMMENT run still records its fingerprint, so re-review depth planning is unaffected and the next run isn't forced to full), the conformance gate's no-blocking-inline rule covers COMMENT the same as APPROVE, step 7 (guidance comment) stays APPROVE-only, step 8 (reviewer requests) runs on APPROVE or COMMENT, and the redundant-approval skip is untouched (a prior COMMENT never suppresses a submission). The frontmatter's allowed-events gains COMMENT; consumers inherit that at bump time, and if the gh-aw engine turns out to validate the event against its own enum rather than passing it to GitHub's review API, the bump PR's audit run is where that surfaces.
One rollout note: this makes the middle verdict live as soon as reviewers start emitting the importance field, so the day-one medium-count instrument now also measures how often approvals demote. If medium inflates somewhere, the consumer rollback is the same as before (strict blocking-only ROUTING plus the validator strip), and the verdict follows the count back down.
KORE-2464