Repository navigation
review-pr: block on a premise that does not reproduce, a failure that survives, or a fix that patches the symptom - #2227
Conversation
…at survives The bar asked only whether the current behaviour is wrong, so a diff that is correct line by line, tested and accurately described had no rule to fail when it rested on a measurement that does not reproduce, or left the failure it names happening. Adds the E family and the fourth blocking kind it reports under. Over PRs 1950-2200 the seven refactors a card cleared with no finding landed at net +395 to +13 production lines; the six a human sent back over the same weeks landed at net -184 to -26, same authors and same kind of change. E3 is that count. Raises the SKILL.md budget to 365. The fourth kind and Q6 are unconditional, so they belong in the file every review reads rather than behind the rules index.
Both sides changed S1 and the rule count. Main's #2216 added S8 for broad excepts and repointed S1's closing sentence at it; this branch widened S1 to statements leaving a try body, cleanup that discards an exit code, and the swallow that hid the defect a fix repairs. Kept both: S1 carries the wider triggers and now cites S8 rather than AGENTS.md for the broad case. The count reads 80 rules across 11 families -- 75 at the fork, plus E1-E4 here and S8 from main.
Code metrics gate: PASSEDJudged: the 6 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
47 findings in files this change does not touchOutside this change (informational): 47
...and 22 more; the job log lists every row.
Thresholds and sources: |
…cating what the gate decides A fix that patches where the error surfaced rather than where the value is born was not reportable: the code is right, the description is right, no AGENTS.md bullet is broken. Across #2188 #2099 #2015 #1981 #1982 #1983 the layer judgement was made every time and every time as a non-blocking follow-up -- on #2188 six seconds after the approval, on #1983 twenty-five minutes after the merge. Q2 now places the diff and the cause on one vocabulary, Q6 names the scenario in actual values, both reach the card, and a divergence with the upstream end untouched is E4. Step 5 drops wiring, twins and constants, which returned no hit in 27 recorded diagnostics and sit inside C3/V7, D4/C2/D1 and P1/P2/P6, and gains patch-shape: six diff-visible tells taken from those same six PRs. A1, A8, A9, A10, A11 and A12 are deleted. scripts/code_metrics_design.py already decides them under those ids, so a review adjudicating them re-decided a gate: 119 verdict lines and no finding in 23 recorded reviews. D12 and D14 now fire only under a baseline-raise waiver, which is the one question the job cannot answer. Advisory-only rules take no verdict line -- the contract cannot publish them. Step 0 goes, its two questions being Q1 and Q2 asked once more without an artifact, and BLIND goes, written 30 times for no late finding. 815 verdict lines across 23 reviews carried 12 FIRE between them, all from X2, V8, V5 and C2.
… reach the card E1 told the reviewer to reproduce a complexity premise with ruff --select C901. The gate pins radon 6.0.1, which counts every and, or, ternary, comprehension and assert; mccabe does not. On the two PRs E1 cited as premises that did not reproduce, ruff puts _validate_restore_record at 8 and load_controller_publication at 18 and radon puts them at 23 and 30 against a ceiling of 20. Both premises hold, #2046's body had said 30 all along, and the rule was citing its own measurement error as evidence. E1 now names the tool for each dimension and keeps the pair as the example of why; D12's Evidence line said Ruff where its own invocations are radon. The card template offered [verified|inferred] while pr-review-publish.sh:76 refuses any card whose blocking findings are not all [verified], and an E2 evidence gap cannot name the input that triggers it. The template now offers only [verified], and the threshold says that naming where you looked and what was absent is what verifies a gap -- otherwise the fourth blocking kind is unpublishable. Q2's layer words had no definition and no direction, and both E4 and the new Layer: line turn on which end is upstream: they are now admission, producer, intermediate, consumer, upstream-first, each with what it does. "Claims to fix something" gated three obligations and was untestable; it is now E2's trigger, a fix prefix or a named failure. patch-shape takes N/A off a fix PR rather than a false CLEAN. Step 7 handed card.md to the independent reader, which Step 8 writes; it hands the surviving findings. Adding a new rule moves to rules.md, where the work it describes happens, paying the budget back to 356.
…3's number D12 and D14 were narrowed to the one question the code-metrics job cannot answer -- whether a PR waived by baseline-raise gives its reason -- but their index rows still said "changes any non-test file under src/" and "adds or changes a function signature", so every src/ PR kept selecting them and the narrowing changed nothing. The waiver is the trigger, and it was in no artifact: fetch.sh fetched no labels. It now asks for them in the gh pr view call it already makes, as label: lines that leave meta.txt's key-value parsing intact, and one index row replaces the two. E3 asked the reviewer to sum numstat.txt by hand over src/ outside tests/. fetch.sh prints it: production files, added, deleted, net, and the binary rows it excluded because "-" is not arithmetic. Verified against #2216 -- 107 production files, net -15.
fetch.sh collects all three comment streams -- review bodies, inline review comments and issue comments -- then keeps the ones written by the author, a write or admin collaborator, or the bot. The login list comes from `jq -r`, which writes CRLF on some platforms. A login carrying the CR matches neither the author nor the bot literal, and the permission URL built from it fails to parse, so every candidate fell through: trusted.json came out empty and comments.txt with it, without the fetch failing. On this host, reviewing #2102 produced a 0-line comments.txt while the PR carried twelve comments, including the author's reproduction of the failure and a withdrawn LGTM. The review read neither and recorded the file as empty. `tr -d '\015'` on the login stream; the octal escape rather than a literal so the fix cannot reintroduce the byte it removes.
…cannot fire V2's and V7's own bodies read "Not a finding when: n/a -- this is a precondition ... not a finding", so each wrote a guaranteed-CLEAR line on every review while the step that runs them already records the work: V2 is the output contract and the verification depth, V7 is Q4's enumeration, whose SKIPPED case the protocol in Step 8 already carries. V1 keeps its line -- it has a real Report as and the encoding-rewrite scan, which no step records elsewhere. Two stale cross-references: the A-family calibration still read A2-A14 after A8-A12 were deleted, and re-review.md filtered earlier cards by "an author it trusts" while pr-review-publish.sh matches the "## PR #<n>" heading alone, so the filter would skip prior_verdicts.txt and the gate would withhold the verdict.
xiaofei-zheng
changed the title
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2227 -- review-pr: block on a premise that does not reproduce, a failure that survives, or a fix that patches the symptom
What it does: changes the review-pr skill. It adds a fourth blocking kind, Ineffective, with the E family (E1-E4): a premise that does not reproduce, a failure that survives at the head, a refactor that grows code, and a fix at the wrong layer. It adds Scenario:/Layer: to fix cards and drops Step 0, the BLIND line and three Step 5 checks. It deletes A1 and A8-A12 because code-metrics decides them. In fetch.sh it strips CR from logins (an empty comments.txt on Windows), and adds label lines to meta.txt and a new netlines.txt.
Blocking issues: 1
- [X2] Deleting A8 drops a blocking check that
code-metricsdoes not run [verified]
Problem:.claude/skills/review-pr/rules.md:1051andreferences/ci.md:5-8delete A1 and A8-A12 because the gate decides them "under those same rule ids", and the description says the same. That holds for every case except one.flag_argument_problems(scripts/code_metrics_design.py:18-22, :377) checks only A8's shape (a), the flag argument. Onmain, A8 also blocks shape (c) for "a parameter ... that only tests set to a non-default value" (blocking by owner decision 2026-10-12, citingAGENTS.mdCode shape: "no parameter that is derivable, unread or set only by tests"). No gate measures that shape:- ruff selects no ARG rule (pyproject.toml:602-613);
- vulture reports only an unread parameter (checked with vulture 2.16:
def f(a, b): return areportsb, while a keyword parameter that only tests set toTruereports nothing); - no rule at this head mentions it (
grep -i "only tests"over rules.md and SKILL.md finds nothing).
A8 (a) is gated only in part.main's A8 fires when "the diff adds a parameter to, or adds adef", butflag_argument_problemswalks onlychange.new_functions()(code_metrics_design.py:249-255, :379): functions whose name is absent at the base. A flag parameter added to an existing function is therefore lost too. A9, A10, A11 and A12's single-implementation case were already advisory onmain, and A1's import case, A12's forwarder and A8 (d) (vulture) are gated, so nothing else is lost.
Impact: once this merges, two shapes thatmain's review blocks pass both the gate and the review: a production parameter that only tests vary, and a flag parameter added to an existing function. ci.md also tells reviewers that raising it is a review defect, while the description states the deletion takes away no check.
Action: keep A8 for the two shapes the gate does not run, the test-only parameter and a flag parameter added to an existing function, on its index row for functions that add or change a signature. Narrow the deletion note in rules.md, ci.md and the description to the A8 shape the gate does run: a new function that starts by branching on its flag. Alternatively, extendscripts/code_metrics_design.pyto cover both shapes in this PR or a predecessor, and keep the deletion.
Checked: diff, body and commits at head. fetch.sh changes: the CR strip, the label lines and the netlines awk with its binary-row exclusion. rules.md index-to-body consistency at head: 74 bodies; every index id has a body, and only V2 and V4-V7 sit on no row, as SKILL.md puts V1-V8 on every list. References to the deleted ids and the removed Step 0, BLIND and Step 5 checks across .claude, .github and scripts. The budget raise 350 -> 365 and its reason in commit 3623586. The pr-review-publish.sh [verified] check (.github/scripts/pr-review-publish.sh:75-76). main's severities of A1 and A8-A12 against the code-metrics.yml header and code_metrics_design.py (A9, A10, A11 and A12's single-implementation case are advisory there) | Ran: vulture 2.16 on a two-function probe (an unread parameter is reported; a test-only keyword parameter is not); git merge-tree against main (clean); gh run list at head (the two cancelled runs were superseded by passing reruns) | Skill: review-pr 56bad557b6ff | Model: claude-opus-5-5 | Base: ef83d0d | Head: 21591e9
Deleting A1 and A8-A12 as gated was right for four of them and wrong for A8. scripts/code_metrics_design.py::flag_argument_problems walks change.new_functions(), whose units are absent from the base tree, so a flag parameter added to a function that already existed is invisible to it; and nothing in the repo measures A8's test-only parameter, which main marks blocking by owner decision -- pyproject.toml selects no ARG rule, and vulture reports a parameter no body reads, not a read one whose only non-default caller is a test (checked with vulture 2.16 on a two-function probe). A8 returns narrowed to exactly those two shapes, with an index row that fires on a parameter added to a def that exists at the merge base. The other five stay deleted: of the shapes they carried, only A1's added import and A12's pure forwarder were blocking, and the gate reports both. Raised by chaojhou on #2227; verified here against code_metrics_design.py, pyproject.toml, vulture and main's severity lines before applying.
|
@chaojhou the X2 finding is correct and is now fixed in
I also checked your "nothing else is lost" claim rather than assuming it. On I took your first option. A8 is back, narrowed to exactly the two shapes the gate cannot see, with its own index row ( I did not take the second option — extending One note for anyone reading this thread later: the root error was mine and it was a verification error of exactly the kind E1 exists to catch. I confirmed that seven functions in |
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2227 -- review-pr: block on a premise that does not reproduce, a failure that survives, or a fix that patches the symptom
What it does: changes the review-pr skill. It adds a fourth blocking kind, Ineffective, with the E family (E1-E4): a premise that does not reproduce, a failure that survives at the head, a refactor that grows code, and a fix at the wrong layer. It adds Scenario:/Layer: to fix cards and drops Step 0, the BLIND line and three Step 5 checks. It deletes A1 and A9-A12 because code-metrics decides them, and narrows A8 to what no gate measures. In fetch.sh it strips CR from logins (an empty comments.txt on Windows), and adds label lines to meta.txt and a new netlines.txt.
Blocking issues: 1
- [X2] Restored A8 still drops the test-only parameter on a new function [verified]
Problem: held since 21591e9. cd386d6 restores A8, but only for "the diff adds a parameter to adeforasync defthat exists at the merge base" (.claude/skills/review-pr/rules.md, A8 Fires when; index row "adds a parameter to adefthat already exists at the merge base"). It excludes "the function is new at this head, whereflag_argument_problemsalready decides it" (A8 Not a finding when). That exclusion holds for shape (a), the flag argument. It does not hold for shape (b), the parameter only tests vary.flag_argument_problemsreports only a function whose first statement isif <flag>:ending inreturn/raise(scripts/code_metrics_design.py:377-410,flag_argument_problemsand_selecting_flag), so a new function's test-only parameter reaches neither gate.main's A8 fired on "adds a parameter to, or adds adefundersrc/with ... (c) a parameter ... that only tests set to a non-default value", blocking by owner decision 2026-10-12. The same mismatch is inreferences/ci.md("Those two are A8's, which is why it survives") and the description's A8 sentence.
Impact: a new production function that takes a parameter only its tests pass passes both the gate and a review under this skill.main's review blocks it, and new functions are where such seams are usually introduced.
Action: key A8 by shape, not by whether the function is new. Shape (a), the flag argument: only on a function that existed at the merge base. Shape (b), the test-only parameter: on any added or wideneddefundersrc/outsidetests/. Widen the index row to match (adds adef, or adds a parameter to one), and drop "the function is new at this head" from Not a finding when for shape (b).
Checked: git diff 21591e911 cd386d677 (SKILL.md rule count, references/ci.md, rules.md: the A8 body and index row, the A-family deletion note); rules.md index-to-body consistency at head (75 bodies, no index id without a body; only V2 and V4-V7 sit on no row, as V1-V8 are on every list); flag_argument_problems, new_units and _selecting_flag in scripts/code_metrics_design.py:249-255 and :377-410; main's A8 Fires-when and Severity; the author's 10-27 12:38 reply | Ran: git merge-tree against main (clean); gh run list at head (the two cancelled runs are superseded by reruns) | Skill: review-pr 56bad557b6ff | Model: claude-opus-5-5 | Prior: X2 moved | Base: ef83d0d | Head: cd386d6
…s not exempt cd386d6 restored A8 behind one trigger -- a parameter added to a def that exists at the merge base -- and that limit belongs to the flag argument alone. flag_argument_problems covers the flag on a new function, so the review only needs the older ones. Nothing covers the test-only parameter on any function: pyproject.toml selects no ARG rule, vulture reports a parameter no body reads rather than a read one whose only non-default caller is a test, and _selecting_flag (code_metrics_design.py:405-410) matches only a body whose first statement is `if <flag>:` ending in return or raise, which a test seam is usually not. A new production function taking a parameter only its tests pass therefore reached neither the gate nor the review, while main's A8 blocks it -- and new functions are where such seams are introduced. Fires-when now scopes the two shapes separately, the index row widens to a def or a parameter added to one, and the "function is new at this head" exemption is marked as shape (a)'s only. Raised by chaojhou on #2227, second round; _selecting_flag and main's Fires-when read here before applying.
|
@chaojhou correct again, and the defect was in my fix rather than in the original deletion. Fixed in What I got wrong: I took "the gate covers new functions" from shape (a) and applied it to the whole rule. It only holds for the flag argument. I read Reading it now ( def write_summary(rows, write_reports=True):
if write_reports:
_emit_reports(rows)
return _summarize(rows)matches neither condition — the A8 is now keyed by shape:
The index row widens to Both rounds of this review found the same class of error on my side: I verified that a name matched — seven functions carrying the A-family ids, then one function iterating new units — and treated the match as coverage without reading what was actually measured. That is what E1 asks of a premise, applied to my own. |
…d fix a trigger that names the wrong artifact Three defects, all the same shape as the two rounds before them: a rule id matched, so coverage was assumed. A1's gate resolves ownership with Packages.unit(), built from .importlinter -- 19 units on this tree, with hyperloom.orchestrator as one. A1's own owner is hyperloom.<layer>.<subpkg>, which is finer, so a private import between two subpackages of one unit is reported by nobody. The shape is live: orchestrator/actions/executors/profile.py:19,22 reach into orchestrator.trace_analysis._capture_shapes, with four more in predictor/ and kernel/. A1 returns, narrowed to that case plus the cross-unit case on a baseline-raise PR, where code_metrics_report.py:136-138 waives the gate's own finding (PRIVATE_REACH and FORWARDER are not in ADVISORY). D12 and D14 were re-keyed to the waiver by the previous commit, but their Fires-when named ci.txt, which fetch.sh writes as name/conclusion/url -- and a waived run reports success like any other. The label lands in meta.txt, which is where this now reads it. A13's severity line credited the design checks with the unread parameter; that is vulture's, run as a ratchet. The advisory exemption added earlier also had to say that a SKIPPED line is not a finding, so V4's # PARTIAL: lines still reach the card. Found by running the skill on this PR, which the previous nine commits never did; rules.md's Adding-a-rule step now requires it.
…ver into A8, and stop the permission swallow Fourth instance of the same error, found by running the skill on this PR as rules.md now requires. code_metrics_design.py::_forwarded_to computes params = _parameter_names(...) and returns None when `not params` (:692-693), so forwarder_problems reports a forwarder that takes an argument and never one that does not. A12 returns, narrowed to the parameterless shape plus the cross-shape case a baseline-raise label waives. The same waiver reaches A8: FLAG_ARGUMENT is absent from the gate's ADVISORY set, so a labelled PR turns the flag-argument finding into a listed pass and shape (a) comes back to the reviewer even on a new function. SKILL.md's Step 7 stopped asking the independent reader for a card.md that Step 8 writes, but .github/workflows/pr-review.yml:204 still asked for it, and :195-196 says the workflow overrides the skill -- so the fix did not reach where the skill actually runs, while the description claimed it did. The workflow joins the PR. fetch.sh swallowed every failure of the permission lookup behind `2>/dev/null || true`, which is what let the CRLF defect empty comments.txt in silence; a rate limit would do it again. A 404 stays the valid negative and anything else now dies. Verified end to end on #2227: comments.txt 152 lines.
|
@chaojhou both your rounds are fixed and I then ran this skill on its own PR, three independent times, which is what your two findings should have prompted me to do in the first place. It found four more of the same class. Head is now The class is the one you named twice: a gate function exists, carries the rule's id, and its coverage is assumed without reading what it measures. Four instances, including both of yours:
A1's case is live in the tree: Three further things that run came up with, all verified before applying:
One thing the PR now says about itself: the E family would not have caught any of these four. E1 asks for a premise to be reproduced with the repo's own tool, but "the gate already decides A1" is a scope claim about a tool, not a measurement with a number to re-run. That gap is stated in the description rather than papered over, and |
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2227 -- review-pr: block on a premise that does not reproduce, a failure that survives, or a fix that patches the symptom
What it does: Changes the review-pr skill.
- It adds a fourth blocking kind,
Ineffective, with the E family (E1-E4), addsScenario:/Layer:to fix cards, and drops Step 0, theBLINDline and three Step 5 checks. - It deletes A9-A11 and A12's single-implementation case as gated, and restores A1, A8 and A12, each narrowed to the part the
code-metricsgate's code does not reach. - In
fetch.shit strips CR from logins, makes a failed permission lookup other than a 404 fatal, and adds label lines tometa.txtand a newnetlines.txt. - It aligns the pr-review workflow's Step 7 instruction.
Re-reviewed at 4257aec.
Blocking issues: 2
-
[free:fetch-permission-404-dies] The new permission lookup kills
fetch.shon exactly the 404 it means to accept, with no message [verified]
Problem:.claude/skills/review-pr/fetch.sh:182-185runsperm=$(gh api ".../collaborators/$login/permission" --jq .permission 2>"$raw/perm.err")and then tests[ $? -ne 0 ] && ! grep -q 'HTTP 404'. The script runs underset -euo pipefail(:9). A failing command substitution in an assignment trips errexit, so the subshell running thewhileloop exits before theifis reached.pipefailthen fails the script. Because stderr went toperm.err, nothing is printed. I reproduced the exact block underset -euo pipefail, with a lookup that printsHTTP 404and exits 1: the script exits 1 without reachingdieor the next login. On this repository,gh api repos/AMD-AGI/Hyperloom/collaborators/Copilot/permissionreturnsCopilot is not a user (HTTP 404). That is the login GitHub's Copilot code review posts under. A rate limit or network error also exits silently, not through the newdiemessage.
Impact: a PR with any comment or review from a login the permission endpoint 404s on makes the fetch, and with it the pr-review workflow, stop with exit 1 and no output. This is the silent failure the change was written to remove, now on the branch it declares valid.
Action: capture the status without tripping errexit, e.g.if ! perm=$(gh api ... 2>"$raw/perm.err"); then grep -q 'HTTP 404' "$raw/perm.err" || die "..."; perm=""; fi. Add a check that a 404 login is skipped and a non-404 failure dies with its message. -
[X3]
references/ci.mdand the A-family preamble still say A12 is gated and gone, while the PR restores an A12 rule [verified]
Problem:.claude/skills/review-pr/references/ci.md:5-8listsforwarder_problemsamong the checks "reporting them as A9, A10, A11 and A12", and says "Those four are not review rules: a finding on one of them belongs to thecode-metricsjob, and a card that re-derives it is re-flagging a gated rule.".claude/skills/review-pr/rules.md:1061opens "Four of this family's rules are gone: A9, A10, A11 and A12". Both contradict the restored### A12body (rules.md, the parameterless forwarder, and any forwarder on abaseline-raisePR), the new index rowadds a function whose body is one forwarding call | A12, and the description's "A1, A8 and A12 stay".
Impact: a reviewer who reaches ci.md (SKILL.md sends every reviewer there before quoting CI) is told an A12 finding is a review defect, while rules.md tells the same reviewer to fire it. One of the two instructions is obeyed at random.
Action: in ci.md, say that only A12's parameter-taking forwarder and single-implementation case are the gate's, and that the parameterless one and anybaseline-raisePR are A12's. In rules.md:1061, name A12's single-implementation case, not A12, as gone.
Checked: git diff cd386d677 4257aec50: SKILL.md rule count, fetch.sh permission block, references/ci.md, rules.md (preamble, Adding a rule step 5, index rows, A1, A8, A12, D12/D14 triggers, A-family preamble), pr-review.yml Step 7. rules.md has 77 bodies, matching SKILL.md's "77 rules". The live gh api permission answers on this repo: Copilot gives 404, the bots give none, an outside login gives read. | Ran: a reproduction of fetch.sh:182-185 under set -euo pipefail with a lookup returning HTTP 404 (exit 1, no message). git merge-tree against origin/main (clean). CI at 4257aec: 36 pass, 6 skipped. | Skill: 56bad557b6ff | Model: claude-opus-5-5 | Prior: X2 fixed | Base: ef83d0d | Head: 4257aec
4257aec replaced a `2>/dev/null || true` swallow with a `die`, but the loop holding it is the middle of a pipeline, so it runs in a subshell: the `die` exits that subshell and nothing else. Reproduced on a three-element probe -- the message prints, the script continues, the downstream jq writes a trusted.json holding only the entries collected before the failure, and the run exits 0. That is the shape S2 names: a path that did not finish reporting success, and worse than the silence it replaced, because the output now looks complete. The loop reads logins.txt instead, so `die` is in the top-level shell. A 404 is still the valid negative. Verified both paths: #2227 fetches 201 comment lines on the success path, and the same structure on a probe exits 1 without running the line after the loop.
…ion block really did references/ci.md and the A-family preamble still listed A12 among the rules the gate decides and declared it gone, while the same PR restores an A12 body, an index row and a Severity line that make the parameterless forwarder a review finding. A reviewer reaching ci.md -- which SKILL.md sends everyone to before quoting CI -- was told that firing A12 is a review defect while rules.md told them to fire it. Both now name A12's single-implementation case, not A12, as the gate's, and ci.md states the split. 90c9b74's comment described the failure it removed as "the message prints, the script continues, the run exits 0". That is not what happened. Under set -euo pipefail a bare `perm=$(gh api ...)` trips errexit on the failing substitution, so the `if` below it was dead code and the fetch died with no message at all -- on the 404 it meant to accept. `Copilot`, the login GitHub's code review posts under, 404s on this repository, so any PR it has commented on would have killed the fetch. Reproduced both ways: the old block exits 1 in silence, and the current one skips the 404 login, keeps the collaborator, and leaves the loop with status 0. Raised by chaojhou, third round.
Superseded by the re-review at 24b9dd7: both findings are fixed.
PR #2227 -- review-pr: block on a premise that does not reproduce, a failure that survives, or a fix that patches the symptomWhat it does: Changes the review-pr skill.
Re-reviewed at 24b9dd7: both findings from 4257aec are fixed. Blocking issues: none Checked:
Ran:
Skill: 56bad557b6ff | Model: claude-opus-5-5 | Prior: free:fetch-permission-404-dies fixed, X3 fixed | Base: ef83d0d | Head: 24b9dd7 LGTM |
…gger, and name each narrowing V8's new evidence step told the reviewer to run `git merge-tree <base> HEAD origin/main` and grep the sha it writes. That is git's deprecated three-argument form: run here it prints a "changed in both" diff listing and no sha at all, so the check could not be performed as written. `git merge-tree --write-tree HEAD origin/main` prints the sha on its first line; verified both on this repo. A1 and A12 carry a `baseline-raise` clause in their Fires-when, because the label waives the gate's design findings. A8 said the same thing in its Severity and Not-a-finding paragraphs but not in its trigger, so the waiver never reached the condition that selects it. references/ci.md listed only PRIVATE_REACH and FORWARDER as outside ADVISORY; FLAG_ARGUMENT is outside it too, which is what makes A8's case real. The A-family preamble listed the three narrowings positionally -- "a unit coarser than the rule's package, an empty parameter list, a new function" -- which pairs A8 with A12's shape and A12 with "a new function", and a new function is the gate's own territory for both, since flag_argument_problems and forwarder_problems each walk change.new_functions(). Each narrowing now names its rule. Found by running the skill on this PR, sixth round.
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2227 -- review-pr: block on a premise that does not reproduce, a failure that survives, or a fix that patches the symptom
What it does: Changes the review-pr skill. It adds the Ineffective kind with the E family (E1-E4), adds Scenario:/Layer: to fix cards, drops Step 0, the BLIND line and three Step 5 checks, deletes A9-A11 and A12's single-implementation case as gated, and restores A1, A8 and A12, each narrowed to what the code-metrics gate's code does not reach. In fetch.sh it strips CR from logins, makes a non-404 permission failure abort the fetch, and adds label lines to meta.txt and a new netlines.txt. Re-reviewed at 759bae1, which follows the LGTM at 24b9dd7. 759bae1 corrects V8's git merge-tree command, carries the baseline-raise waiver into A8 shape (a)'s trigger, and names each narrowing in the A-family preamble.
Blocking issues: 1
-
[X3] The new preamble sentence hands every new function to the gate for A8 and A12, while the A8 and A12 bodies fire on new functions [verified]
Problem: .claude/skills/review-pr/rules.md:1069-1071 (new in 759bae1) says "A new function is the gate's own territory for both A8 and A12 --flag_argument_problemsandforwarder_problemseach walkchange.new_functions()." The rule bodies say otherwise:- A8's Fires when takes shape (b), the test-only parameter, "on any function, new or widened alike", and its Not a finding when exempts only shape (a) on a new function.
- A12's Fires when (rules.md:1172) is an added
defthat forwards with no parameter. That is a new function by construction, andforwarder_problemsskips it because_forwarded_toreturnsNoneon an empty parameter list.
The same paragraph's previous sentence lists those two shapes as A8's and A12's.
In references/ci.md:17-19, the waiver sentence now names three findings (PRIVATE_REACH,FLAG_ARGUMENT,FORWARDER) but still says the label "turns both into" listed-but-passing findings.
Impact: a reviewer who reads the A-family preamble is told to leave every new function to the gate. That drops exactly the two shapes this PR restored A8 (b) and A12 for: a test-only parameter on a new function, and a new parameterless forwarder.
Action: in rules.md, limit that sentence to A8 shape (a) and A12's parameter-taking forwarder, which are the cases the gate decides on a new function. In ci.md, change "both" to "all three".
Checked: git diff 24b9dd7b8 759bae1d7 (references/ci.md, rules.md: the A-family preamble, A8 Fires when, V8's merge-tree example). Against scripts/code_metrics_design.py at head: ADVISORY (:71) does not contain FLAG_ARGUMENT, and flag_argument_problems (:377-379) and forwarder_problems (:664-667) both walk change.new_functions(), so those claims hold. The V8 command git merge-tree --write-tree HEAD origin/main matches git's current form. | Ran: git merge-tree against origin/main (clean) | Skill: 56bad557b6ff | Model: claude-opus-5-5 | Prior: none (LGTM at 24b9dd7) | Base: ef83d0d | Head: 759bae1
…op restating the gate split Seven review rounds on this PR found seven classes of defect, and three mechanisms produced all of them. Restated facts. "Does A12 belong to the gate or the review" was written in 11 places -- an index row, four sentences of the A-family preamble, the rule's Severity, Fires-when, Not-a-finding and Report-as, and three sentences of ci.md -- so a correction had to land in ten more, and four times it did not. ci.md is now a table of which id is decided where and nothing else; the A-family preamble points at it and at each rule's Severity instead of repeating them. A12 goes from 11 mentions to 5, A8 from 10 to 4, A1 from 6 to 4. Unrun commands. D12/D14 read a label out of ci.txt, where fetch.sh writes none, and V8 named `git merge-tree <base> HEAD origin/main`, the deprecated form, which prints a diff listing and no sha for the `git grep` after it. Adding a rule now requires running every command the body names and pasting the output into the commit. Unevidenced coverage. Four rules were deleted because a function carried the matching id, without reading what the function decides: `Packages.unit()` resolves at .importlinter granularity, `flag_argument_problems` walks `change.new_functions()`, `_forwarded_to` returns None on `not params`, and a baseline-raise label waives all of it. E5 makes the covering predicate, quoted with its boundary, the only evidence that something else already checks this. It is E1's other half: E1 for a premise with a number to re-run, E5 for one with a boundary to read.
… a stand-in declare its context A backtest of the seven defects this PR already shipped, run against the skill as it now stands, put E5 on three of them and nothing at all on two others. Both misses are the same gap: the requirement that a rule's commands have been run lives in rules.md's *Adding a rule*, which is addressed to whoever adds a rule, while Step 1b sends the reviewer straight to the index below it. So a trigger reading a label out of ci.txt, where fetch.sh writes none, and V8's deprecated three-argument `git merge-tree`, which prints a diff listing and no sha for the `git grep` after it, would both ship again. Step 5's `claims` now covers a command or artifact name the diff adds to any instruction: run it once and read the output. V2 asked for the changed function to be called directly and said nothing about what a stand-in must match. That is how 90c9b74's commit message came to describe a failure that does not reproduce -- the probe called `die` directly, the real block was under `set -euo pipefail` where a bare assignment tripped errexit first and `die` was dead code. A stand-in now lists its context against the real one: set options, pipeline position, redirection target, interpreter version, what is mocked.
Superseded by the re-review at c2c040f: the finding is fixed.
PR #2227 -- review-pr: block on a premise that does not reproduce, a failure that survives, or a fix that patches the symptomWhat it does: Changes the review-pr skill.
Re-reviewed at c2c040f. Blocking issues: none Checked:
Ran:
Skill: 56bad557b6ff | Model: claude-opus-5-5 | Prior: X3 fixed | Base: ef83d0d | Head: c2c040f LGTM |
Problem
A review could pass a PR in full while the problem it was opened for stayed live. The bar in
SKILL.mdwas one question — if this is not changed, is the current behaviour wrong? — so a diff that is correct line by line, tested, and accurately described has no rule to fail, even when the reason it was opened on does not reproduce, or the failure it names still happens at the head.The same bar made "this patches where the error surfaced, not where the value is born" unreportable. Across #2188, #2099, #2015, #1981, #1982 and #1983 that judgement was made every time and every time as a non-blocking follow-up — on #2188 six seconds after the approval, on #1983 twenty-five minutes after the merge.
Meanwhile the adjudication step had stopped paying for itself. Across 23 recorded
verdicts.txt, 815 verdict lines carried 12FIREbetween them (1.5%), all from X2, V8, V5 and C2.Change
A fourth blocking kind,
Ineffective, and theEfamily it reports under. E1 reproduces the premise at the merge base with the repo's own tool. E2 reconstructs what the named failure takes, traces which branch the head runs under exactly those conditions, and states the outcome. E3 makes a refactor that grows production code name the mechanism it removed, in net lines. E4 fixes where the input is admitted, not where it finally breaks. E5 makes the covering predicate, quoted with its boundary, the only evidence that something else already checks a thing — a matching id is not.A fix is judged by its scenario and its layer. Q2 places the diff and the cause on one upstream-first vocabulary (
admission,producer,intermediate,consumer); Q6 names the triggering scenario in actual values and order. Both reach the card asScenario:andLayer:on fix PRs. Step 5 dropswiring,twinsandconstants— no hit in 27 recorded diagnostics, and each sits inside C3/V7, D4/C2/D1 and P1/P2/P6 — and gainspatch-shape, six diff-visible tells taken from the six PRs above.Work the gate already does stops being re-done — where it really does it. A9, A10, A11 and A12's single-implementation case are deleted:
scripts/code_metrics_design.pydecides them under those same ids, and the A family wrote 119 verdict lines and no finding across 23 reviews. A1, A8 and A12 stay, each narrowed to the part of its shape the gate's own code does not reach:Packages.unit()is built from.importlinter— 19 units, withhyperloom.orchestratoras one, coarser than the rule'shyperloom.<layer>.<subpkg>flag_argument_problemswalkschange.new_functions()ARGrule inpyproject.toml; vulture reports unread, not test-only_forwarded_toreturnsNonewhennot params(:692-693)A
baseline-raiselabel waives the gate's design findings entirely (code_metrics_report.py:136-142;PRIVATE_REACH,FLAG_ARGUMENTandFORWARDERare not inADVISORY), so all three return to the reviewer on a labelled PR. D12 and D14 are re-keyed to that same waiver, read frommeta.txt's newlabel:lines. Advisory-only rules, and V2 and V7 whose own bodies say they cannot be findings, take no verdict line. Step 0 goes (it was Q1 and Q2 asked once more without an artifact) and so does Step 6'sBLINDline (written 30 times, no late finding).Four existing rules widen to shapes they were missing: V3 no longer accepts the symptom on the body's word and is adjudicated whatever else the review already found; S1 reads in both directions — a statement leaving a
trybody (#2010), a cleanup discarding an exit code (#2122), and the swallow that let the fixed defect run unnoticed (#2169); D4 covers the second copy this diff creates (#2012, #2009, #2018); V8 greps the merge tree for a moved symbol (#2132 broke collection on every shard ofmain).Defects found while auditing the above
comments.txtwas empty on this platform. The login list comes fromjq -r, which writes CRLF here; a login carrying the CR matched neither the author nor the bot literal and made the permission URL unparseable, sotrusted.jsoncame out empty andcomments.txtwith it — without the fetch failing. Reviewing [RSI] Build the Ray preflight retry spec from the installed version #2102 produced a 0-linecomments.txtwhile the PR carried twelve comments, including the author's reproduction of the real cause and a withdrawn LGTM. Fixed; the same PR now yields 228 lines.fetch.shwrapped the permission lookup in2>/dev/null || true, which is why the CRLF defect was silent rather than loud. A 404 stays the valid negative; anything else now dies.[verified|inferred]whilepr-review-publish.sh:76refuses any card whose blocking findings are not all[verified], and an E2 evidence gap cannot name the input that triggers it. The template now offers only[verified], and the threshold says naming where you looked and what was absent is what verifies a gap.card.mdthat Step 8 writes. Fixed inSKILL.md— and in.github/workflows/pr-review.yml:204, which restates the same instruction and whose header says it overrides the skill, so the skill-only fix would not have reached CI.What this PR does not fix
Four of its own defects were the same error: a gate function existed, carried the right rule id, and its coverage was assumed without reading what it measures. Each was caught by someone running the review process — twice by @chaojhou, twice by running it on this PR, which
rules.md's Adding a rule step now requires.E1 would not have caught any of them: it asks for a premise to be reproduced with the repo's own tool, and "
code_metrics_design.pyalready decides A1" is a scope claim with no number to re-run. E5 is that other half — it fires when a check is dropped because another mechanism is said to cover it, and takes only the covering function's own predicate, quoted with its boundary, as evidence.A backtest of all seven defects against the finished skill puts E5 on three of them (the A1, A8 and A12 deletions), leaves one a partial hit, and reaches two others only through Step 5's
claims, which this PR extends to cover a command the diff adds to an instruction. The seventh — adieinside a pipeline subshell — is reached by no rule: nothing inrules.mdtriggers on a shell-only diff, which is a pre-existing gap this PR does not close.Testing
fetch.shrun end to end on#2148,#2216and#2227:netlines.txt(107 production files, net -15 on #2216, binary rows excluded), thelabel:lines inmeta.txt(confirming its key-value parsing still readstitleandmergeable), andcomments.txt(0 lines before the CRLF fix, 228 after on #2102, 152 on #2227).bash -nonfetch.sh.Every PR cited as evidence was opened at the sha in question. The gate claims were reproduced rather than read:
Packages.unit()resolved on this tree (19 units;orchestrator.actionsandorchestrator.trace_analysisboth resolve tohyperloom.orchestrator),_forwarded_toread at:692-693,ADVISORYandwaivable_failedread incode_metrics_design.pyandcode_metrics_report.py, vulture 2.16 run on a two-function probe, and the complexity figures measured with both radon and ruff at each merge base.SKILL.mdis at 359 lines against its 365 budget;rules.mdholds 78 rules across 11 families, which is whatSKILL.mdstates.A correction this PR carries
An earlier revision of E1 cited #2009 and #2046 as premises that did not reproduce, measured with
ruff --select C901. The gate pins radon 6.0.1, which counts everyand,or, ternary, comprehension andassert. On the same two functions ruff reads 8 and 18 while radon reads 23 and 30 against a ceiling of 20 — both premises hold, and #2046's body had said 30 all along. E1 now names the tool for each dimension and keeps the pair as the example of why a premise checked with the wrong tool is worse than one not checked at all.