Skip to content

[Feature] Add a write-time entry point for the review checks #206

Description

@WarningRan

Feature Description

A second skill that works the checks in .claude/skills/pr-review/ktir-checks.md against a branch before a pull request exists, so this repository's traps are met while the code is being written rather than for the first time in review.

Blocked on #201 — do not start this until that merges. ktir-checks.md and pr-review/SKILL.md do not exist on main today; they land in #201, and everything below refers to them. Starting earlier would mean building against files whose shape is still under review.

Motivation / Problem

ktir-checks.md names no pull request. Only one item in it needs one to exist — the check on PR descriptions, which asks for a what is not changing section. The rest describe code:

  • the MLIR semantic-fidelity traps: NaN propagation, rounding direction, signed versus unsigned, ordered versus unordered comparison predicates;
  • stick-versus-byte HBM addressing, and the carriers stick-granular accounting requires;
  • inplace_outs aliasing under MLIR value semantics;
  • the dynamic-shape rule that parser support alone is not support.

Each of those applies at the moment the handler is written.

Nothing routes there at that moment. A skill is reached through its description, and pr-review's says to use it when reviewing a PR — so an author about to push a branch never arrives at the list.

The cost is review rounds spent on what was decidable at write time. The boundary-validation and guard checks are the most broadly attested group in that file, at five reviewers across seven PRs. The divergence between the regex parser's catch-all fallback and the frontend's NotImplementedError carries its own guard test, tests/mlir_frontend/test_registry_consistency.py, precisely because in that test's own words it "has bitten us repeatedly".

Proposed Solution

A separate skill directory with its own description and its own allowed-tools (git diff, git merge-base, gh issue view, uv run pytest — not gh pr *), owning only what genuinely differs from pr-review:

  1. The branch is the entry, not a PR. git diff $(git merge-base origin/main HEAD)...HEAD plus the linked issue. There is no PR description yet, so nothing stands between the change and the requirement and the issue's acceptance criteria are measured directly.
  2. Two passes do not survive being run by the author, and the skill has to say so. Whether a maintainer would expect this shape in this place, and which spec layer settles a question, both fail when the author's model of the repository is wrong — re-running them with the same model returns clean. A clean pass is therefore not evidence that the design holds or that the change is spec-safe, and neither claim belongs in a PR description on its strength.
  3. One check only the author can make. Does each new regression test fail without the fix? The review skill notes that CI cannot report this and directs the reviewer to ask the author instead; the author can answer it against a scratch checkout of the merge-base.
  4. No verdict, no severities read as a verdict, no finding labels. The output is a work list ordered by severity, plus the material the description check asks for: the neighbouring boundaries left untouched, what was run, what was deferred.

Everything else delegates — the design and coverage lenses to pr-review/SKILL.md, the traps to ktir-checks.md. One copy of every rule, two callers.

Alternatives Considered

  • A section inside pr-review/SKILL.md rather than a second skill. A skill is reached through its description, and that one triggers on reviewing a PR, so a section inside it would not be read at the moment it is needed.
  • Put the write-time content in CLAUDE.md. Weighed in [Feature] Add a pr-review skill: review procedure plus this repo's known traps #200's Alternatives Considered and rejected there: CLAUDE.md is in context for every task, so it is paid for on every unrelated turn, and it cannot express the generators-before-list ordering that does the work.
  • Adopt a generic agent development framework. Frameworks of the obra/superpowers kind ship process — test-driven development, worktrees, plan-writing — and nothing repository-specific, so they layer with ktir-checks.md rather than replacing it. They also do not generally accept new skills upstream, so this repository's traps could not live there.

Context / Background

Raised by @lasch while discussing #201: framing these as review checks makes a clean review the target for a PR author. The part answered in #201 is what the checks are for, and the one item that could be satisfied vacuously. This issue is the other part — that most of the content applies at write time and nothing routes to it then.

Acceptance Criteria

  • A second skill whose description fires when an author is about to push, not when someone is reviewing a PR.
  • No rule text duplicated from ktir-checks.md or pr-review/SKILL.md; both are referenced.
  • The two passes that do not survive self-review are named in the skill, with the consequence for the PR description stated.
  • Ships with the same kind of evidence [Feature] Add a pr-review skill: review procedure plus this repo's known traps #201 carried: worked against a real branch before pushing, with what it caught reported in the PR.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions