docs(review): encode the rejection categories the GFQL stack kept hitting - #1924
Merged
Conversation
…ting The owner rejected the same classes across three consecutive PRs (#1894, #1895, #1897): mechanism narration where a name/test/structure should carry the meaning, perf and asymptotics claims that belong in pyg-bench, and Any/type-ignore/hygiene-pin instead of a narrow type. Their diagnosis was that this skill is incomplete or unused; both were true, and 'unused' was the larger half -- it is written for reviewing others' PRs and was never run against our own diffs across ~14 authored PRs. Adds three sections: an Encoding table (narration, why-a-fix-was-made, perf/asymptotics, engine-mechanics prose, restating the signature), a Typing table (Any with a known domain, new type: ignore, hygiene-ok on new code, cast() to satisfy the checker, and why 'no growth' is not sufficient), and a Self-review gate with the greps to run over <base>..HEAD before pushing -- plus the rule that fix-cycle briefs must carry these rules inline, since a brief that gates correctness and mentions style in passing produces code that satisfies what was measured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
A reviewer may be commenting while you push: re-fetch comments (with timestamps) before declaring a review addressed, treat comment anchors as belonging to their commit rather than to current line numbers, and run the self-review gate on the remediation diff too -- remediation commits are new code and draw their own review round. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
Six causes, from four consecutive PRs rejected for the same classes: authoring writes discovery logs; fix-cycle briefs put agents in explain-mode without saying where reasoning goes; perf briefs specifically invite code annotations; no automated gate existed for this class alone; a stack-wide audit leaves every ancestor PR unfixed while reviewers review individually; and self-judged 'justify each keep' biases toward keeping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
An audit kept six comments with written justifications; the owner rejected three, including two documenting real engine traps. The bar is not 'can I defend this?' but 'can a test or a name carry this instead?' -- a trap a test can demonstrate belongs in a pin named for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
This was referenced Aug 15, 2026
113 added lines -> 22, in a file that is always in context. The removed bulk was narrative, not instruction: a seven-item root-cause essay on why the class kept escaping, a three-bullet account of concurrent human review, and two tables whose rows restated each other. Writing a verbose essay about not being verbose is the failure it describes. What survives is every actionable verdict: what to delete from a diff and what carries it instead, the typing verdicts, and the pre-push gate. The root-cause material is dropped rather than relocated -- `bin/ci_comment_density_guard.py` now enforces those three checks mechanically, so prose explaining why humans miss them is no longer the control. The documented gate was wrong as written and is now verified. It fired on `from typing import ... cast ... Any` and on test-file section dividers, so in practice it returned noise and would have been ignored. Scoped to non-test sources with import lines excluded: it returns NOTHING on #1927's diff and 93 hits on #1895's -- a real positive control rather than a grep that is always silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
lmeyerov
added a commit
that referenced
this pull request
Aug 15, 2026
…1924/#1926/#1894) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Standalone, no code — lands independently of the GFQL stack.
You rejected the same categories across #1894, #1895 and #1897 and diagnosed
skills/review/SKILL.mdas "incomplete or unused". Both were true, and unused was the larger half: the skill is written for reviewing other people's PRs, and across ~14 PRs I authored it was never once run against my own diff before pushing. No gate existed, so nothing caught it.The contributing cause is mine too. My fix-cycle briefs to subagents carried ~40 lines of correctness gates (oracles, byte-identical failure sets, GPU sweeps) and one line about comments — so agents optimized exactly what was measured. And I authored some of the violations directly: the two
hygiene-ok: explicit-anypins onparams: Anyyou flagged were mine, i.e. I used the grandfathering mechanism on new code, which is its misuse.Three sections added:
Anywith a known domain, newtype: ignore,hygiene-okon new code,cast()to satisfy the checker, and why the guard reporting "no growth" is not sufficient (it holds debt constant; a PR touching a file should drive its count down).<base>..HEADthat every branch must pass before pushing, plus the standing rule that fix-cycle briefs paste these rules inline.Remediation already applied under the new rules: #1894 is fixed (real
CypherScalar/CypherParamsaliases,is_series_likereplacing hasattr duck-typing, atype: ignoreremoved by restructuring, the null-ish label set unified — it turned out to have 5 copies, not 2 — and the hygiene baseline ratcheted down). #1895 and #1897 are in flight with the rules pasted into their briefs, including the requirement that deleting a comment adds the pin carrying its meaning, since otherwise it is information loss rather than cleanup.🤖 Generated with Claude Code
https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi