SuperAPI AI Directory
Partner中文AI newsFavorites
Skillscode-review-in-stack

code-review-in-stack

Testing & quality

View repository

Updated 10-15 22:41

About this skill

Code review in stack

Review one slice — a single PR of a stack — against the ticket it claims to close, then dispose of every gap. The verdict axis is the ticket's own acceptance criteria, not taste: a slice is done when its criteria are met, and a criterion is met when an artifact proves it.

Two things make this different from a plain diff review. A slice sits in a stack, so a gap may already be closed by a later layer, and recommending work without looking is how two agents do one job twice. And a slice has a boundary, so a gap that needs a decision or crosses layers is handed forward as a ticket rather than smuggled into the PR under review. A published layer is also immutable (ADR-0010): a fix for a stacked slice lands as a new PR on the tip, never as a commit into the slice.

Invoke with a PR id: /code-review-in-stack 253.

Hard rules

  • Edit no code. This skill produces a review and, on approval, a ticket. A fix is described, never applied.
  • Publish nothing without approval. Posting the review to the PR, filing a ticket, and editing a ticket's criteria each wait for explicit approval of that specific action.
  • Every criterion gets a verdict. Report the tally ("11 of 12 met") and account for each one. A criterion you could not check is its own verdict, named as such. Accounted for is not the same as written out: met criteria collapse to one line, and only gaps get prose (step 7).
  • The ticket's scope wins. Where an ADR, a design doc, or a code comment implies work the ticket does not ask for, the ticket governs the verdict and the surplus becomes a suggestion.
  • A gap is an unmet criterion. Anything else is a finding, and it says so. "13 of 13 met" followed by a gap block reads as a contradiction and costs the reader their trust in the tally. A problem the ticket never asked about is labelled outside the criteria in its own first words, so the score and the findings can never appear to disagree.

Process

1. Pin the slice

Resolve the PR id to: head sha, base ref, the ticket it claims (Closes #N, or the ticket named in the title/body), and the parent spec. Fetch the ticket and the spec through the repo's issue-tracker workflow (docs/agents/issue-tracker.md).

Then pin the slice's place in the stack with the stack-sweep skill's step 1 (stack-map.sh <pr>): parents, children, siblings, tip, whether the stack is a registered GitHub Stack, and whether the fetch that produced those facts succeeded. Carry its freshness line into the report. in_stack: false means the dispositions in step 5 collapse to their single-PR forms.

No ticket, no criteria, no review: ask the user for the ticket before going further.

2. Prove the head green

Check the PR head out clean (a worktree, never the user's working copy) and run the repo's own gate. Record the numbers in the report. A red head is the first finding and it outranks every criterion verdict, because a criterion "met" on a red build is unproven.

3. One verdict per criterion

Walk the criteria in order. Each verdict names its evidence: the test that pins the rule, the symbol that implements it, the fixture that no longer needs its workaround. A plausibility argument is not evidence, and neither is a doc comment claiming the behaviour.

Read each criterion for its escape hatch — a clause that hands part of the work forward ("assert at the primitive if #205 keeps compile from seeing the case, and say so in the test's own doc"). A criterion carrying one is met by taking it, and taking it is the evidence. This is where inference from an absolute claim in an ADR most often contradicts the ticket; the ticket governs.

Also check the claims the PR body makes about itself, Closes #N first. A slice claiming to close a ticket whose criteria are not all met is a finding of its own.

4. Sweep the stack before recommending anything

For every gap, hand the behaviour it is made of to the stack-sweep skill as a construct, and let its step 2 look for it in the later layers and the sibling branches before costing a fix. That skill counts behaviour per branch rather than reading the prose around it, and it returns one of three outcomes; each changes the recommendation:

  • Addressed downstream. Name the branch or PR that closes it. Recommend nothing; the gap is a note in the report so the next reviewer does not re-find it.
  • Unchanged through the tip. Say so with the evidence ("all four sites unchanged from layer 1 to layer 15"), which is what makes the disposition in step 5 defensible.
  • Changed but not addressed. A later layer rewrites the same lines and does something else. Report it as a conflict hazard: whichever of the two lands second takes the conflict.

5. Dispose of every gap

Each gap gets exactly one disposition, and each carries what to do next:

  • Fix in this PR — only when the PR is not in a stack. The fix is local to files this slice already touches, needs no decision, and adds no criterion. Say which symbol changes and which test pins it.
  • Fix in a PR on the tip — the same local, decision-free fix when the PR is in a stack. A published layer is never edited (ADR-0010), so the fix is a new PR branched from the tip and based on it, which makes that PR the new tip. Name the tip to branch from, the symbol that changes, the test that pins it, and whether the stack is registered — a registered stack needs the new PR linked in (gh stack link, re-listed bottom-to-top) or it sits outside the atomic merge.
  • Hand forward as a ticket — the fix needs a human decision, crosses layers, or widens the slice past its criteria. Draft the ticket in step 6.
  • No work — met, or addressed downstream, or out of scope by the ticket's own words.

Two questions size it. Does closing the gap require a decision nobody has made? Then it is a ticket, whatever its diff size. Is the PR in a stack? Then no disposition may touch it, and the mechanics of the PR on the tip belong in the report, not in a footnote.

Say what the gap does and does not break. A criterion can be unmet while the ticket's headline property holds — an id that stays null still serialises deterministically, so the fingerprint is stable and only the diagnostics go blind. Stating both halves is what keeps a diagnostic-only gap from blocking a slice, and what keeps a byte-moving gap from being waved through.

6. Draft what you recommend

For every ticket disposition, draft the issue and show it before filing: what is wrong and where, why it is still owed, what is already settled, the decisions a human still owes, the acceptance criteria, and the coordination hazard from step 4. State plainly that it does not block the slice under review when it does not.

For every fix in this PR or fix in a PR on the tip disposition, write the change as instructions, not as a patch.

Where a criterion needs amending — a half delivered, a half handed forward — propose the exact new wording and let the user rule.

A draft is long by nature, so it does not go in the report. Keep it out of band: name it in one line ("ticket drafted for the nested half, want it?") and show it when asked, or write it to a file and give the path. The only exception is a one-line criterion rewording, which is shorter to show than to describe.

7. Report to a human with minutes, not hours

The reader is a human deciding what to do next, and a report they skim is a report that loses its own findings. Write for the skim, in this order:

  1. One sentence carrying the verdict: the tally and the single thing that matters ("11 of 12 criteria met, one gap, diagnostic-only, unchanged through the stack tip").
  2. The gate result, one line, with the numbers.
  3. The met criteria as one line, not one block each — a list of numbers, or "all but 4". A met criterion's evidence is held in reserve for a challenge, not spent on the page.
  4. One block per gap, and nothing else gets a block. Five short labelled lines: the criterion quoted, the evidence, the sweep result, the disposition with its next action, and what it does and does not break.
  5. The blocking question last, when you have one, with nothing after it.

Budget the whole thing at a screen plus one block per gap. What does not fit is named and offered, never silently dropped: a warning, a number, a scoped condition, and a conflict hazard stay in even when everything else goes. Bold the lead-in of each block so the bold alone carries the verdict, the recommendation, and the risk.

Then ask, per action, before posting the review, filing a ticket, or editing a criterion.

8. The comment posted to the PR is shorter than the report

The report argues to a reader who asked for it. The PR comment is read by someone who did not, between two other tasks, often on a phone. So it carries the score and the gaps, and nothing else — no met-criteria list, no evidence spine, no freshness note, no self-claim paragraph. Everything you cut is still in your report and still available on challenge.

The whole comment, exactly:

  1. The score, one bold line. **12 of 14 criteria met. 2 gaps.**
  2. One block per gap, three bold lead-ins and nothing more:
    • **Gap: <what is wrong, in plain English>.** then one or two sentences of the concrete detail — the symbol, the file:line, the behaviour. Say why it slipped when that is what makes it understandable.
    • **Do this:** <the next action>. A test to add, a body line to change, or the decision a human owes. Never leave a gap without one.
    • **Impact:** <what breaks and what does not>.
  3. Nothing after the last gap.

A slice with no gaps gets one line: **11 of 11 criteria met. No gaps.** Nothing to do. Add at most one sentence when residue is visible in the diff but closes in a later layer, so a reader seeing it does not re-file it.

Two failure modes to avoid, both of which read as vagueness:

  • A gap with no next action. "The error value is constructed by no test anywhere in the stack" tells a reader something is wrong and leaves them to work out what to type. The disposition from step 5 is the answer; put it in the comment as an imperative.
  • Evidence in place of the finding. Test names, grep counts and branch-by-branch tallies are how you became sure. They are not what the reader needs to act. Keep the fact, drop the proof.

Rewriting a comment you already posted is normal and cheap: gh api -X PATCH repos/<owner>/<repo>/issues/comments/<id> -F body=@file.md. Give each comment a leading HTML comment marker (<!-- code-review-in-stack -->) when you post it, so you can find your own comment ids again later.

The standards axis

This skill owns the criteria-and-stack axis only. When the user also wants the diff judged against the repo's conventions, invoke the code-review skill for that axis and report the two side by side without merging them: a slice can meet every criterion and still read badly, and one axis masking the other is what keeping them apart prevents.

Traps

  • A code comment citing the wrong ticket. A None filed under a ticket whose scope cannot cover it reads as tracked work and is not tracked at all. Check that the cited ticket's scope actually contains the gap; when it does not, the mis-citation is itself a finding.
  • An ADR's absolute claim. "There is no site at which this cannot be derived" is a decision, not a scope. It tells you the gap is real; the ticket tells you whose slice owns it.
  • A fix recommended into a layer. "Add the test in this PR" reads naturally and is wrong whenever the PR has a child: the layer is published, and the fix belongs on the tip.
  • A stale account or ref. The sweep's freshness line is part of the report. A failing gh call and a stale remote-tracking ref both produce confident wrong answers about the stack; say which facts came from a fetch that worked.