[siliangchen-amd]

• Description: what and why

  review-pr reviews every push of a pull request from scratch. The earlier cards sit in comments.txt as prose, so a finding fixed since can vanish without a word, one that still holds can come back under a new name, and two cards on one head can disagree with neither saying why. Nothing on a card says which version of the skill or which model wrote it: none of the 125 cards on the last 100 merged PRs (refactor(kb_store): move derive_kernel_names into kb_store.identity.kernel_names #2028..Document which roles the agent backend governs and how deepseek joins the Forge relay #2174) does. On fix(report): count dispatches, kernel outcomes and critic passes once each #2045 a second review of the head the author's own clean card had passed found a defect the author then fixed, and nothing marked it as a miss.

  Two commits; please use Rebase and merge:

  1. The card's Checked: line names Skill: (the tree hash of .claude/skills/review-pr, +dirty with local edits) and Model:, which fetch.sh records in skill.txt; fetch.sh stops when git cannot name the tree. pr-review-publish.sh applies the bot's verdict only when the card names exactly this run's skill and model, with a clean tree and a known model. The CI failure classification moves unchanged to references/ci.md, so SKILL.md stays within its 350-line budget.
  2. When comments.txt holds an earlier card, the review first decides each finding of the latest card, FIXED, HOLDS or MOVED, into prior_verdicts.txt, then reviews the whole diff as a first review does (references/re-review.md). A finding that holds keeps its key and says held since <sha>; a new finding on code the earlier card already passed says (missed at <sha>); a class that comes back after the author changed the code names the cause the findings share. The card's Checked: line adds Prior: C1 fixed, T2 holds, or Prior: none. pr-review-publish.sh applies no verdict to such a PR when the review left no prior_verdicts.txt, the card's Prior does not list it key for key, or a HOLDS/MOVED key is missing from the numbered findings (a clean card beside Prior: C1 holds would otherwise dismiss the bot's earlier request-changes review). comments.txt now heads each entry with its time, so the latest card can be told apart.

• Linked issue(s): none

• Tests: added/updated? commands run? (what to test; if a test was replaced, where its assertions live now):

  • Added scripts/tests/test_pr_review_publish.py, pr-review-publish.sh against a stub gh. A card naming this run's skill and model gets its verdict; an unknown model, a dirty or another skill, or a missing skill.txt gets none. A first review beside another PR's card and a re-review that settles the earlier card (fixed; holds and fixed; moved on the card; none) get their verdict. A re-review with no prior_verdicts.txt, a malformed line, none beside findings, a Prior that disagrees or is missing, a HOLDS/MOVED key dropped from a clean card, a Prior on a PR with no earlier card, or a review that left no comments.txt gets none.
  • scripts/tests/test_review_pr_fetch.py gains skill.txt, the stop outside a git checkout, and the timed comments.txt headers.
  • Ran on each commit: the repository's pre-commit hooks, ruff on the changed files and the two test files. On the last: ruff check ., ruff format --check ., check_broad_suppress.py src scripts, check_cli_references.py, pytest scripts/tests (816 passed, 2 skipped, neither in these files), code_metrics.py --base-ref origin/main (passed). The ruff pre-commit hook fails on main the same way (--force-exclude passed twice), so ruff ran directly. After the self-review fix: pytest scripts/tests/test_pr_review_publish.py scripts/tests/test_review_pr_fetch.py (30 passed), and ruff check . and ruff format --check . with the pinned ruff 0.16.2.

• Size/complexity triggers crossed: split, or why the shape is right: none; no production Python changes, and most of the added lines are the two test files.

• If this simplifies or refactors: which mechanism was removed, which contract was preserved, and the tests that show it: the CI failure classification moves verbatim from SKILL.md to references/ci.md; nothing else moves.

• Observable effect: what an operator will now see, and what the old behaviour cost them (if none, why the change is unobservable):

  • Every card's last line names what produced it: Checked: ... | Ran: ... | Skill: <sha> | Model: <name> | Base: ... | Head: ....
  • A re-review says what became of each earlier finding (Prior: C1 fixed, T2 holds), keeps a finding that still holds under its key with held since <sha>, and marks one an earlier card missed (missed at <sha>) instead of presenting it as new.
  • The bot applies no verdict, and posts "Automated review: no verdict applied" instead, when its card does not name the run's skill and model or a re-review did not settle the earlier card.
  • What the old behaviour cost: authors could not tell which earlier points were settled, two cards on one head said nothing about each other, and no card said what produced it.

• Breaking changes: yes/no (details if yes): no. The stricter publish checks apply to the bot's cards, whose CI contract now asks for Skill, Model and, on a re-review, Prior; local runs work as before. An earlier card is recognised by its ## PR #<n> heading alone, so older cards count.

• PR addresses single concern: yes/no (details if no): yes. Re-reviews that account for the earlier card; naming the skill and the model is what makes two cards comparable.

• Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed: n/a

[github-actions]

Code metrics gate: PASSED

Judged: the 8 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.

Dimension	Limit	New	Worse	Baseline out of date	Baseline entries	vs base
Cyclomatic complexity	fails > 20	0	0	0	92	+0
Cognitive complexity	fails > 30	0	0	0	43	+0
Function length (lines)	fails > 50	0	0	0	527	+0
Nested block depth	fails > 4	0	0	0	43	+0
Parameter count	fails > 5	0	0	0	3	+0
Module length (lines)	warns > 800, fails > 1200	0	0	0	44	+0
Duplicated lines per file	fails > 0	0	0	0	91	+0
Dead code (vulture)	fails > 0	0	0	0	2	+0
Directory size (entries)	fails > 50	0	0	0	2	+0

Check	Findings	Effect
Comments	0
Broad catch without a reason	0
English only	0
Production code imports test code	0
Repeated literals	0
A1 Private name of another package	0
A8 Flag argument	0
A9 Tuple return	0
A10 Raw vocabulary comparison	0
A11 Hidden global write	0
A12 Pure forwarder	0
A12 Single-implementation abstraction	0

17 findings in files this change does not touch

Outside this change (informational): 17

Unit	Dimension	Baseline	Now
src/hyperloom/orchestrator/kernel/serving_log_evidence.py resolve_forge_server_log	Cognitive complexity	absent	49
src/hyperloom_kb/collect/mapping.py _experience	Cognitive complexity	absent	31
src/kernelforge/cli/forge_loop_command.py forge_loop	Cognitive complexity	absent	31
src/kernelforge/loop/runner.py IterationLoop._collect_session_candidate	Cognitive complexity	absent	44
src/kernelforge/loop/runner.py IterationLoop._rule_on_attempt	Cognitive complexity	absent	38
src/hyperloom/orchestrator/kernel/integrate_accuracy.py grade_integrate_accuracy	Function length (lines)	absent	56
src/hyperloom/orchestrator/loop/_resume_recovery.py ResumeRecoveryMixin._resume_recover_pending_warm_replay	Function length (lines)	absent	75
src/hyperloom_kb/http_service.py	Module length (lines)	absent	1522
src/kernelforge/cli/forge_loop_command.py	Module length (lines)	absent	2003
src/hyperloom/inference_optimizer/cli/preflight.py _probe_rocm_build	Function length (lines)	54	57
src/kernelforge/gemm_tune/tuners/_aiter_dense_common.py run_aiter_dense_tuner	Function length (lines)	118	119
src/hyperloom/inference_optimizer/breakdown/schema.py	Module length (lines)	2681	2686
src/hyperloom/orchestrator/loop/intent_router.py	Module length (lines)	1365	1373
src/hyperloom/orchestrator/prompts/prompt_builder.py	Module length (lines)	1205	1207
src/kernelforge/fusion/command.py	Module length (lines)	1584	1708
src/kernelforge/loop/runner.py	Module length (lines)	6051	6510
src/kernelforge/loop/task_preparer.py	Module length (lines)	1299	1557

Verdict computed by the gate scripts in /home/runner/work/_temp/code-metrics-gate, not by this tree's copy.

_{Thresholds and sources: [tool.hyperloom.code_metrics] in pyproject.toml. Tools: complexipy 8.0.1, jscpd 5.4.1, radon 6.0.1, ruff 0.16.2, vulture 2.16.}

[ZhengGong-amd]

The goal (re-reviews that continue from earlier cards, a named skill and model, a way to measure) is right. The route adds mechanisms the repo already covers, and several of them fail silently. Please rework it as one PR along these lines.

1. Re-review reuses what exists; drop the second card parser.

   • Earlier cards from the author, writers and the bot are already in comments.txt (fetch.sh trusted filter). prior.txt re-parses them with scripts/review_pr_cards.py, while pr-review-publish.sh parses the same card contract in shell. That is two owners for one contract.
   • Replace with an instruction in SKILL.md / references/re-review.md: read the latest card in comments.txt, settle each finding FIXED / HOLDS / MOVED in prior_verdicts.txt, keep HOLDS under its key. Make the publish.sh gate a shell check using the same strict matching publish.sh already applies to the card.
   • Delete the prior and delta subcommands, prior.txt, deltafiles.txt, delta.txt and the gh compare logic. re-review.md itself says the delta "never limits what is read", so it only reorders reading.
   • Sync the text that still contradicts this: SKILL.md Step 7 and the comments.txt row ("do not repeat a point already made"), Step 1b (re-review.md adds HOLDS/MOVED rules, Step 1b does not), the publish.sh header comment, and the Output contract for the new card fields.

2. Round is not what the skill says it is. prior_text sets round = len(cards) + 1, which counts every trusted card, including re-runs on the same head and local reviews. The "round 3: ask the author to reconsider the approach" rule can fire with no author change. Remove Round, or define it as pushes that changed a patch.

3. Silent failures to remove.

   • fetch.sh writes Skill: unknown / Model: unknown and nothing enforces them, so cards can ship without the version the PR exists to record. Fail the fetch or reject the card.
   • gh api compare ... || true reports any API failure as "not an ancestor (rebased?)", while the next merge_base_commit call has no guard. Let it fail visibly.
   • grep -q '^card [0-9]' "$WORK/prior.txt" 2>/dev/null in publish.sh skips the gate when its input is missing, and Prior: .+ accepts any text.
   • A card the parser does not recognise turns the whole feature off (round 1, no gate) with no message.
   • _prior() silently drops malformed items; the eval score job is green with no replay results (continue-on-error plus mkdir -p results).

4. Delete scripts/review_pr_stats.py and its tests. Nothing in the repo consumes it; its numbers are already in the description. Also drop fields nothing reads outside tests: Finding.missed_at, Finding.verified, Card.review_round, Card.prior, Card.deferred, Card.base, Card.title.

5. The eval part does not measure what it claims; reconsider it before keeping it.

   • Positive cases come from the rules.md "Seen in" PRs (#1564, #1444, #1497, #1567, #1563, #1526). The replayed agent reads the current rules.md, which already describes each bug, so the replay checks whether the agent finds the answer written in the skill, not whether the skill finds problems.
   • The agent runs unrestricted with a read token, and fetch.sh only cuts its own artifacts, so later comments on the merged PR are reachable.
   • 7 positive cases at 1 replay each cannot separate a skill change from model variance.
   • If kept: pick cases that are not quoted in rules.md, say how later information is excluded, size the repeats, and reuse pr-review.yml via workflow_call instead of copying its checkout/node/gh/merge-base steps. Drop --json, the agreement metric and the load_cases validation for a repo-owned file.

6. Tests.

   • Remove: the stats tests, len(cases) >= 10, the malformed-case tests, the matrix exact-string startswith test.
   • Add: tests for the new publish.sh gate and for pr-review-run.sh (the description says both were only checked by hand; the eval workflow has never run). The [result] line test uses a hand-written line while the real one comes from the jq in pr-review-run.sh; nothing ties them together.

Per AGENTS.md: "Simplify by removing a mechanism", "Trust the caller" and "Finish the replacement" (owners: pr-review-publish.sh and comments.txt for card handling; pr-review.yml for the review job). Please update the description to match the reduced diff.

[siliangchen-amd]

Reworked along these lines; the PR is now two commits and no production Python.

1. Done: prior.txt, delta.txt, deltafiles.txt, the prior/delta subcommands, the compare logic and scripts/review_pr_cards.py are gone. A re-review reads the latest card in comments.txt and settles it in prior_verdicts.txt. The publish gate is shell and fires on any ## PR #<n> heading in comments.txt, on purpose looser than the match on the card being published: earlier cards include ones people posted from local runs, and a stricter match would turn the gate off silently (your point 3). Step 7, the comments.txt row, Step 1b, the publish header and the Output contract now agree. A HOLDS/MOVED key must also be back among the numbered findings, so a clean card cannot dismiss the bot's earlier request-changes review while Prior itself says the defect is still there.

2. Round is removed. A HOLDS finding opens held since <sha>; the shared-cause rule fires only for a class an earlier card raised on an earlier head.

3. fetch.sh fails when it cannot name the skill tree. Model: unknown is rejected at publish, not at fetch: a local run names its model from its system prompt, which fetch.sh cannot see, and most cards come from local runs. The gate fails closed without comments.txt, and Prior: must equal prior_verdicts.txt key for key. The other items left with the code they lived in.

4. Done; the description's numbers came from that script at 86da0ee.

5. Agreed, removed with the replay mode, cases, scorer, run-script extraction and workflow. A sound eval needs cases outside rules.md, an agent with no path to later history, and enough cases and repeats; that is its own PR.

6. Done; scripts/tests/test_pr_review_publish.py covers the gate against a stub gh.