hani-q /qstack-review
Review a change against two layers of rules and report findings with a computed score. The result is analysis, not permission to edit, commit, push, publish, deploy, or comment anywhere.
Resolve the scope
Take the scope from the user's request, in this order:
- A named pull request. Read its diff and record its head commit.
- A branch. Diff it against the merge base with its base branch.
- Otherwise the working tree. Include committed changes on this branch plus uncommitted and untracked files.
Name the exact commit before reading anything, and put it in the report. For a pull request or a branch, that is the head being reviewed. For a working tree, it is the commit the uncommitted work sits on, reported as the base rather than as the reviewed state, because uncommitted work has no commit of its own. A review that cannot name what it read cannot be checked afterwards.
"Committed changes on this branch" means the commits above the merge base with the base branch. On a branch with no base, a default branch or a repository whose only commit is its root, there are none, and the working tree is the whole scope. Ask when two candidates are equally plausible.
Discover the rules
Layer 1 is the generic baseline below. It ships with this skill and applies in every repository, including one that has never heard of this skill.
Layer 2 is the repository's own law. Read CODE_REVIEW_RULES.md at the
repository root when it exists, then every CODE_REVIEW_RULES.md in a directory
that contains a changed file. A nested file governs only the changed files below
its own directory.
A rules file holds two kinds of row. A Flag rule says "if you see this, say something, and here is how serious it is". A Leave-alone rule says "this looks wrong and is not, and here is why". Layer 2 wins on its own subjects and adds to layer 1 everywhere else.
Where a root row and a nested row cover the same subject, the nested row wins for the files below its directory, because writing a nested file is how a directory sharpens or softens a root rule. A nested file contributes Flag and Leave-alone rows. Take the severity definitions and the score table from the root file when it supplies them, and from this skill otherwise.
Both layers apply to the whole of each changed file, not to the diff hunk alone. A rule that fires on a line the change did not touch is still a finding, because the change is what brought the file under review. Say in the finding that it predates the change, so a reader can weigh it.
With no rules file anywhere, the baseline runs alone and the report says so, so a reader can tell an unruled repository from a clean one.
Read the change
Read every changed file in full rather than the diff excerpt alone, plus the code around each change that the change depends on. Apply both layers to each file.
Every finding carries a path:line, the consequence, and the smallest fix that
removes it. Set severity by consequence, never by how much code the change
touches.
The read is done when every changed file has been read and is either cited in a finding or explicitly cleared. A file left unread for size or format is neither: name it in the report.
The generic baseline
Correctness invariants that hold in any repository, each written as a thing to check rather than a subject to worry about. A violation is a finding at the severity its consequence earns.
- Sentinels: check that no value the domain can legitimately hold also means missing, failed, or not yet set. Ask what this returns when there is nothing to return, then ask whether real data can equal that answer. Zero, -1, an empty string, and an empty collection are the usual overloads. A reachable collision takes the wrong branch on valid input and reports nothing, so its severity is whatever that branch then does.
- State that outlives one call: a buffer, cursor, registry entry, or thread slot reused across calls needs a named owner, a progress invariant, and a shutdown order. Ask who releases it, what guarantees the cursor advances, and what stops teardown from racing a live user of it. Check each of the three: a missing owner is unbounded growth, a missing progress invariant is a loop that can stall, and a missing shutdown order frees something still in use.
- A test observes what it names: check that a new or changed test watches the process, bytes, or registry entry it claims to verify, rather than a wrapper around it, a mock of the code under test, or a log line that would appear anyway. Ask whether the test would fail if the implementation were deleted. One that would still pass is a defect in the test, cited at the test's own line.
- Lowering keeps the construct: where a parser, compiler, transformer, or serialiser handles a set of supported constructs, check that every one of them survives the change. Silently dropped, reinterpreted as a different construct, and accepted then lowered to empty are three separate failure modes. Ask which construct now falls through to the default arm. Accept-as-empty is the worst of the three, because the output is well formed and wrong.
- Public endpoints: an endpoint that accepts or returns sensitive data has to authenticate the caller, constrain the origins that may reach it, and take rate-limit identity only from a proxy chain it trusts. Ask which header decides who this caller is, and whether the caller can set that header. Reading a client-supplied address for rate-limit identity is a bypass, not a missing hardening step.
- Declared paths reach the artifact: for a packaging, container, image, or wrapper-script change, trace every declared path, entry point, and dependency through to the artifact that actually ships. Ask whether that file exists at that path in the built output. A declared entry point absent from the built artifact fails on first start, which is the P0 case under Severity below however small the diff.
The generic Leave-alone rules
Verification duties rather than subjects: what to establish before writing a finding, so a wrong finding is caught before it reaches the report. Each duty carries its own verification step, because "intentional" without evidence is not a reason.
These three withhold a finding only while the evidence they name is in hand. None of them puts a subject out of bounds, and a duty that cannot be discharged leaves the finding standing.
- An absence claim: reading the changed file in full is the floor here, not the proof. Before writing that an import, symbol, path, or branch is missing, search the tree for it, because the name may sit above the excerpt, in a sibling module, in generated output, or in a companion change that landed separately. Verify by quoting the line that would have to exist and does not, or by naming the search that found nothing and where it looked. An unscoped search proves nothing.
- A wait that got shorter: a reduced or deleted sleep, timeout, or retry delay is unsafe only while nothing else orders the work. Verify by finding the explicit readiness signal that now orders it, a condition wait, a handshake, a health check, or an awaited event, and by finding the test that exercises that signal. With both, the shortened wait is the fix rather than the risk. With either missing, write the finding and name which one is missing.
- A guard that looks too strict: verify by reading the public contract the guard enforces, the documented path a developer takes to work inside it such as a flag, a fixture, or a documented override, and the tests that assert the rejection. All three present means the strictness is deliberate, so loosening it changes a contract: raise that as a question rather than a finding. With any of the three missing, the strictness may be accidental and the finding stands.
Severity
- P0: merged as written, it breaks production.
- P1: a real defect, or a violation of a Flag rule that names no severity of its own.
- P2: an improvement that never blocks.
A Flag rule that names its own severity wins; these three are the fallback for a finding no rule assigned.
The score table
The score is arithmetic over the findings. Produce findings with severities, count them, and read the first row that matches. Nothing here asks whether the work feels like a four.
| Counts | Score |
|---|---|
| More than one P0 | 1 |
| Any P0 | 2 |
| Three or more P1 | 3 |
| One or two P1 | 4 |
| Otherwise | 5 |
Rows are tried top to bottom and the first match ends the search: one P0 with six P1 scores 2, because "any P0" matches before "three or more P1". P2 findings never move the number, so 5/5 means exactly one thing, no P0 and no P1 in what was reviewed.
An empty scope is not a clean one. When the scope resolves to no changed file at all, say so in the report's Scope part and give no score, because a number over zero findings reads as a pass on work nobody looked at.
The number counts findings, so how findings are split decides it. One finding per smallest fix, and this rule governs every other rule in this file that speaks of findings: two consequences that one edit removes are one finding, and two consequences needing two edits are two, whatever rule each fired. One sentinel value read by three callers is one finding when changing the return fixes it, and three when each caller needs its own guard. Report the consequences of a merged finding in its own text rather than dropping them. Without this the arithmetic above rests on a judgement about splitting, which is the judgement the table exists to remove.
When the root rules file carries its own score table, that table replaces this one. Name the table that produced the number.
The report
Report in the final response, in this order:
- Scope: what was reviewed, the exact commit, and any file left unread.
- Rules: which rules files applied, or that the baseline ran alone.
- Findings, worst first: severity,
path:line, what goes wrong, and the smallest fix. Say plainly when there are none. - Score: the number, the P0, P1, and P2 counts, and the row that produced it.
- Rules delta: Flag or Leave-alone rows worth adding to this repository's
CODE_REVIEW_RULES.md, written as rows the reader can paste. Propose a row only where this review found the evidence for it.
Read, report, stop. Delivery belongs to whoever ran the skill: this run writes
no file, posts no comment, appends no board event, and commits nothing, on a
clean diff as much as on a bad one. Inside an active QStack execution loop the
durable record stays that plan's execution.md, so add no second audit file.