Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions skills/review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,31 @@ whether they hold, not to guess them.

Then review the diff against the author's stated intent and proof.

## Test-integrity gate (both lenses, before line-level review)

A green gate proves the tests passed, not that the change is right, and the cheapest
path to green is to change the test. Before line-level review:

- **Reviewer A reads the test changes before the code changes** — a test edit is the
fastest way to make a red change look green. Both lenses apply the same rule.
- Treat a diff that rewrites, deletes or skips **many** tests as a red flag: read each
one, compare the new assertion with the old, and require the stated reason.
- Never accept, as the way to a passing gate: a deleted or skipped test, a relaxed
lint/format/type/coverage or `tsconfig` setting, or an assertion edited to match the
new behaviour without a stated reason in the PR body. The signer oracles
(`AGENTS.md` "Hard constraints") are pinned; their expectations are never editable.
- Coverage is not this check: it proves a line ran, not that a test would notice if the
line were wrong. Mutation testing is the check coverage cannot give.

A changeset that reaches green by weakening the check instead of fixing the behaviour
is `request-changes` (L1), citing this section and the exact rule broken — before
approving the behaviour change it hides.

## Reviewer A — Correctness & regression
- Does the change do what the task state says it should?
- Tests: do new tests fail without the change (bug) or cover the feature
surface (feature)? Are oracle/signature tests still green?
- Tests: read the test changes first (test-integrity gate); do new tests fail without
the change (bug) or cover the feature surface (feature)? Are oracle/signature tests
still green?
- Edge cases: empty inputs, part boundary, resumable-upload checkpoint,
non-browser transports, tree-shaking impact (no cross-provider references).

Expand Down Expand Up @@ -66,6 +87,9 @@ Then review the diff against the author's stated intent and proof.
## Done when
- The PR-contract gate passed: the body carried all four author sections (a missing
one was a `request-changes` finding, never skipped in favour of line-level review).
- The test-integrity gate passed: the test diff was read before the code diff, and no
test was deleted/skipped and no assertion or gate config was weakened — or that was
the `request-changes` finding, with the rule cited in the review output.
- Both reviewer runs issued a verdict into their own session and result file; all
L1/L2 problems resolved or explicitly declined with reasons; `verify` gates green;
verdict recorded in the run log.
26 changes: 24 additions & 2 deletions skills/verify/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ description: Run the repo's full check gates (lint, format, types, tests, build)

## Steps
1. Run each gate. Fix what it reports — formatting/lint/type issues are fixed
in the working tree, never suppressed.
in the working tree, never suppressed or configured away.
2. If a test fails because the code is mid-change, finish the change first;
if it fails for an unrelated reason, stop and report (do not "fix" by
skipping).
skipping, deleting or editing the test).
3. Re-run all gates until green. Record outputs (trimmed) in the run log.
4. Never claim verification success without the final command outputs.
5. Collect the PR-contract evidence while the run is fresh (`docs/norms/ops.md`,
Expand All @@ -29,6 +29,28 @@ description: Run the repo's full check gates (lint, format, types, tests, build)
stage that opens a PR carries all four in the PR body; the host refuses a
`pr-opened` whose body is missing any of them.

## Never make a gate pass by weakening it

Green is evidence only if the check that produced it still measures the same thing.
When a gate is red, the fix is the behaviour, not the gate:

- Do not delete, skip (`it.skip`/`describe.skip`/`todo`) or rename away a test to hide
a failure.
- Do not relax a gate's configuration — `.oxlintrc.json`, `.oxfmtrc.json`,
`tsconfig*.json`, the `vitest.config.ts` coverage block, or a `package.json` script —
to silence it.
- Do not edit an assertion, snapshot or fixture to match the new behaviour without
recording the reason in the PR body. The signer-oracle expectations
(`AGENTS.md` "Hard constraints") are pinned and never editable.
- If you believe a test itself is wrong, state that in the PR body and leave the test
alone; a reviewer decides, not the diff.

A gate that fails because of the change you are making means the change is not
finished; a gate that fails for an unrelated reason is a reported blocker, not an edit.

Coverage is not this rule: it proves a line ran, not that a test would notice if the
line were wrong. Mutation testing is the signal coverage cannot give.

## Done when
- All five gates pass with recorded output, or a gate failure is reported as a
blocker with evidence.
Expand Down
Loading