From 6efdf52bf4b9859e247471fd000dac193833de97 Mon Sep 17 00:00:00 2001 From: Agent Stewie Date: Thu, 20 Aug 2026 13:32:26 +0000 Subject: [PATCH] fix: do not grade stale comments as MEDIUM Callers fail the review check at MEDIUM and above. A comment or internal doc that disagrees with the code beside it is not an executable hole, so it is LOW. Public API/CLI/protocol/test guarantees still gate. The worked MEDIUM example is now a test that certifies the wrong walk, not a sentence in RELEASING.md. Co-authored-by: William Grzybowski --- review/rubric.md | 36 +++++++++++++++++++++++++----------- review/schema.json | 2 +- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/review/rubric.md b/review/rubric.md index 68d3d6c..ad56465 100644 --- a/review/rubric.md +++ b/review/rubric.md @@ -22,19 +22,33 @@ the list looks short. - on a path a caller would normally take -> BLOCKER - only under specific conditions -> HIGH -2. Does the change assert something untrue? A type that contradicts what the - value can be, a comment or doc describing behaviour the code does not have, - a guarantee nothing enforces. -> MEDIUM - -3. Does it leave a mechanism that will silently stop working the next time +2. Does the *executable* change assert something untrue? A type that + contradicts what the value can be; a public API, CLI, protocol, or test + guarantee nothing enforces; a test that cannot fail or that certifies the + wrong walk. + Comments, commit messages, internal docs, and CI annotation text that + disagree with the code beside them are not this. They are LOW. A sentence + a compiler never saw does not hold a merge. -> MEDIUM + +3. Does it leave a *mechanism* that will silently stop working the next time someone does an ordinary thing to this repo — a regeneration, a dependency - bump, a routine refactor? -> MEDIUM + bump, a routine refactor? + A stale comment that might mislead a future editor is not this. A CI step + that still fails the job but with a worse error message is not this. + -> MEDIUM 4. Otherwise -> LOW -**If you cannot state the failing input for 1, or quote the untrue claim for 2 -or 3, the finding is LOW.** Severity requires the specific thing that makes it -severe, not a description of the risk. +**If you cannot state the failing input for 1, or quote the untrue executable +claim for 2 or 3, the finding is LOW.** A comment is not that claim. Severity +requires the specific thing that makes it severe, not a description of the +risk. + +Pre-existing product behaviour that a test or coverage PR merely discovered +is not a finding on that PR. File an issue. + +A finding whose only harm is "a future reader might remove a nearby guard" +is LOW. The guard is still there. Reporting no findings is a valid and useful result. Do not manufacture a finding, or raise one's severity, to demonstrate thoroughness. @@ -47,8 +61,8 @@ the finding (in the structured output the `severity` field already carries it, so `summary` stays plain — it is rendered into a CI annotation that is already prefixed with the level): -> **MEDIUM** — `RELEASING.md:18` documents the old behaviour. The table says -> the release type is `minor`, which this change makes untrue. +> **MEDIUM** — `TestParseFlag` asserts that an empty argument errors; +> `parseFlag` returns a default and nil. The test certifies the wrong walk. That applies to inline comments and to the top-level comment alike. A reader should be able to tell a BLOCKER from a LOW without inferring it from how diff --git a/review/schema.json b/review/schema.json index 37dbdeb..77666db 100644 --- a/review/schema.json +++ b/review/schema.json @@ -16,7 +16,7 @@ "properties": { "severity": { "enum": ["BLOCKER", "HIGH", "MEDIUM", "LOW"], - "description": "Assigned by the rubric in review/rubric.md. A finding that cannot name its failing input, or quote the claim it says is untrue, is LOW." + "description": "Assigned by the rubric in review/rubric.md. A finding that cannot name its failing input, or quote the untrue executable claim (not a comment), is LOW." }, "file": { "type": "string",