skills/change-review/SKILL.md

---
name: change-review
description: Project change review for a concrete diff, branch, PR, patch, migration, fix, implementation result, or project plan that needs a new evidence-backed readiness verdict, or when another Keystone skill needs the Review Gate evaluated.
---

# Change Review

## Core principle
Change Review is an independent, read-only attempt to disprove readiness.

Ask two questions at the same time:
1. **Spec axis:** does the work satisfy the stated requirements and acceptance criteria?
2. **Standards axis:** is it secure, correct, maintainable, tested, and safe to operate?

Do not assume changed lines are the blast radius. Trace callers, callees, contracts,
data flow, tests, runtime paths, and user impact before giving a verdict.

## Load when
Load when a concrete software project artifact needs review: a diff, branch, PR, patch,
migration, fix, implementation result, or project plan that must be assessed against
its specification and engineering standards.

Also load when another Keystone skill needs `../../references/gates/review.md` satisfied before shipping.

At entry, use the full Keystone path when there is an inspectable project artifact and
a readiness or blocker verdict is the outcome. Handle general critiques, prose review,
and standalone explanations directly. Explicit invocation selects the full Change Review behavior.

## Not for
Do not use Change Review for:
- fixing, refactoring, formatting, or rewriting code
- committing, merging, tagging, publishing, or shipping
- initial implementation planning before a reviewable artifact exists
- open-ended context-survey with no concrete artifact to assess
- debugging where the requested outcome is a fix

If asked to review and fix, review first, stop, and hand findings to `implementation`, `root-cause-analysis`,
`context-survey`, `shipping`, or a human only after explicit permission.

## Outcome contract
A complete review returns:
- verdict: **Block**, **Caution**, or **Looks good**
- findings ordered P0, P1, P2, P3, then Nitpicks
- evidence for every finding: file/line, behavior path, contract, test, log, or doc
- user impact and why the severity is justified
- remediation guidance without applying the fix
- tests that should be added or updated for affected behavior
- scope reviewed, validation run, limitations, and read-only confirmation

The review is incomplete if it only inspects the diff, only comments on style, or
cannot explain how the work behaves at runtime.

## Change Review passes
Perform multiple passes. New evidence from one pass expands later passes.
### Pass 0: scope and baseline
- Identify artifact reviewed: diff, branch, files, release candidate, or plan result.
- Read the user request, issue, spec, acceptance criteria, and claimed completion.
- Check repository status without modifying files.
- Record uncommitted work as context, not cleanup.

### Pass 1: spec compliance
- Compare implementation against explicit requirements and non-goals.
- Check edge cases, error states, and acceptance criteria.
- Separate spec misses from standards concerns.
- Treat a clean implementation of the wrong behavior as a finding.

### Pass 2: correctness and runtime paths
- Trace primary success and failure paths end to end.
- Follow changed functions into helpers, services, adapters, persistence, UI, jobs, and
  serializers.
- Validate inputs, outputs, invariants, state transitions, retries, ordering,
  concurrency assumptions, and error propagation.
- Look for nullability, off-by-one, time, encoding, pagination, caching, idempotency,
  cancellation, and partial-failure issues.

### Pass 3: regression and compatibility
- Identify callers, consumers, and workflows that rely on old behavior.
- Check public APIs, CLIs, schemas, migrations, persisted data, environment variables,
  feature flags, configuration defaults, and documentation.
- Consider rollback, downgrade, mixed-version, and incremental rollout risks.
- Search for tests or fixtures that encode previous behavior.

### Pass 4: security, privacy, and abuse resistance
- Review authentication, authorization, tenancy, secrets, logging, validation,
  injection, XSS, SSRF, path traversal, unsafe deserialization, and RCE surfaces.
- Check whether sensitive data leaks through errors, logs, telemetry, URLs, caches,
  exports, screenshots, or third-party calls.
- Consider malicious users, compromised clients, replay, races, resource exhaustion,
  privilege escalation, and denial of service.

### Pass 5: tests and proof
- Map changed behavior to existing tests.
- Identify missing unit, integration, contract, regression, migration, security,
  accessibility, performance, or end-to-end coverage.
- Prefer behavior assertions over implementation trivia.
- Run focused read-only validation when practical: existing tests, type checks, lint,
  builds, or targeted commands.
- If validation cannot run, state why and what should be run.

### Pass 6: maintainability and architecture
- Assess clarity, cohesion, naming, dependency direction, duplication, complexity,
  observability, and debuggability.
- Check architectural boundaries, local conventions, and API contracts.
- Flag brittle abstractions, hidden coupling, unnecessary cleverness, and premature
  generalization when they create real maintenance risk.

### Pass 7: user impact and final consistency
- Translate technical issues into affected personas, workflows, data, accessibility,
  performance, reliability, and support burden.
- Re-rank findings by blast radius, likelihood, recoverability, and detectability.
- De-duplicate findings, verify evidence, and state limitations honestly.

## Severity rubric
Severity reflects realistic impact, not fix size.

### P0: Critical blocker
Immediate or likely severe harm. Examples:
- data loss, corruption, or irreversible destructive action
- unauthorized access, privilege escalation, secret exposure, or major privacy breach
- production outage or release artifact that cannot safely deploy
- legal/compliance risk with material impact

P0 means do not ship or merge without accountable human acceptance and mitigation.

### P1: Blocking defect
High-impact issue that violates core requirements or creates serious regression risk.
Examples:
- primary workflow broken for a meaningful user segment
- incorrect billing, permissions, persistence, or business logic
- migration or compatibility gap that can break real deployments
- high-risk behavior lacking tests plus a plausible failure mode

P1 normally blocks shipping.

### P2: Important non-blocker or conditional blocker
Material issue with bounded impact, lower likelihood, or workaround. Examples:
- edge case with clear user impact
- moderate-risk test gap
- maintainability issue likely to cause near-term bugs
- weak observability for a risky path

State whether release context makes it blocking.

### P3: Low-risk improvement
Valid concern with limited impact. Examples:
- confusing name or local complexity that slows future work
- minor non-hot-path performance inefficiency
- incomplete docs for non-critical behavior
- small test organization weakness

P3 should not block unless it compounds with related risks.

### Nitpick
Cosmetic, preference-level, or optional feedback: unenforced formatting, wording tweaks,
or style suggestions with no correctness or maintainability impact. Keep nitpicks
separate from severity findings.

## Impact tracing
For each meaningful change, trace:
- **Entry points:** user action, API route, CLI, job, event, hook, or import.
- **Callers:** who invokes this and what assumptions they make.
- **Callees:** helpers, libraries, persistence, network calls, and side effects.
- **Data flow:** input, validation, transformation, storage, serialization, output.
- **Contracts:** types, schemas, public APIs, flags, config, docs, and errors.
- **Runtime paths:** success, failure, retry, timeout, cancellation, concurrency.
- **Tests:** existing coverage, missing assertions, fixtures, mocks, snapshots.
- **Users:** visible behavior, accessibility, performance, reliability, trust.

If tracing leaves uncertainty, gather more read-only evidence or report the limitation.
Do not invent confidence.

## Security and regression checklist
Ask for every non-trivial review:
- Can a user access, modify, infer, or delete data they should not?
- Are authn, authz, tenancy, and ownership checked at the right layer?
- Can untrusted input reach queries, interpreters, shells, paths, templates, redirects,
  or deserializers unsafely?
- Are secrets, tokens, PII, or internal identifiers exposed in logs, errors, telemetry,
  URLs, caches, or client bundles?
- Did defaults, permissions, feature flags, or safeguards become unsafe?
- Are races, duplicate submissions, retries, replay, and out-of-order events safe?
- Can persisted data be corrupted, stranded, or made hard to rollback?
- Are public APIs, stored data, configs, and integrations backward compatible?
- Does failure degrade safely without hidden partial success?
- Are performance, resource use, accessibility, localization, and platform differences
  acceptable for realistic users and abuse?
- Do tests cover the affected behavior and important regression paths?

## Subagents and reasoning
Use read-only subagents for separable risks: security/privacy, test coverage,
architecture/API compatibility, persistence/migration, accessibility/user impact,
performance, concurrency, or release risk. Use deeper analysis for security-sensitive,
data-loss, billing, permissions, public API, migration, or cross-system reviews. When delegation is available, encode required evidence depth and review standard in the prompt.

Subagents must receive the read-only contract and return evidence-backed findings, not
patches. Reconcile duplicates and conflicts before reporting. The primary reviewer
owns final severity and verdict.

## Hard rules
- Read-only: inspect files without editing, formatting, generating, staging, committing, merging, tagging, or publishing them.
- Report issues without silently fixing them.
- Do not run destructive or project-mutating commands.
- Do not rely only on changed lines; inspect impacted code paths and contracts.
- Do not approve solely because tests pass.
- Do not report speculation as fact; mark uncertainty.
- Do not bury blockers under minor comments.
- Do not disguise style preferences as correctness findings.
- Do not omit needed tests when behavior changed.
- Do not satisfy `../../references/gates/review.md` unless blockers and non-blockers are separated.
- Load `../../references/gates/review.md` before the verdict; Change Review owns the review execution and supplies the gate evidence.
- Run the checkpoint gate before the final response; if review passes and delivery/finalization was requested, route to `shipping` or leave an explicit shipping prompt.

## Failure modes
Avoid these anti-patterns:
- **Single-pass skim:** one read of changed lines plus generic comments.
- **Diff tunnel vision:** missing callers, callees, contracts, and user impact.
- **Checklist theater:** naming security/tests without tracing actual risk.
- **Green-test rubber stamp:** assuming current tests prove new behavior.
- **Spec blindness:** judging code quality while requirements are unmet.
- **Standards blindness:** accepting unsafe or fragile code because the narrow spec passes.
- **Severity inflation:** turning preferences into blockers.
- **Severity deflation:** downgrading real user harm because the fix is small.
- **Patch creep:** fixing, refactoring, or committing instead of reviewing.
- **Unowned uncertainty:** failing to state what was not verified.
- **Lost next event:** Change Review passes but never routes or prompts for `shipping` when finalization remains.

## Output format
Worked finding example:
```markdown
### P1
- Missing tenant check on invoice export
  - Evidence: `api/exportInvoice.ts:42` accepts `invoiceId` and loads the invoice without comparing `invoice.accountId` to the authenticated account; `/invoices/:id/export` is reachable by any logged-in user.
  - Impact: A user who guesses another invoice ID can download billing data from a different account, which is a privacy and authorization breach.
  - Recommendation: Enforce tenant ownership before export and return the existing unauthorized response on mismatch.
  - Tests needed: Add an integration test where account A requests account B's invoice and receives 403/no file, plus a happy-path same-account export test.
```

Use this structure:
```markdown
## Verdict
Block | Caution | Looks good

## Scope reviewed
- Artifact reviewed:
- Key files/paths inspected:
- Validation run:
- Review limitations:
- Read-only confirmation: no files changed by this review

## Findings
### P0
- [Title]
  - Evidence:
  - Impact:
  - Recommendation:
  - Tests needed:
### P1
None

### P2
None

### P3
None

## Nitpicks
None

## Tests to add or update
- Behavior:
  - Suggested coverage:
  - Why it matters:
## Handoff
- Blockers:
- Non-blocking follow-up:
- Suggested owner module: implementation, root-cause-analysis, context-survey, shipping, or human

### Checkpoint
Use the required fields from `../../references/gates/checkpoint.md`.

```

If a severity has no findings, write `None`. Recommendations must be actionable but
must not be applied by Change Review.

## Shared standards

For architecture-sensitive or code-quality-sensitive work, load `../../references/engineering-standards.md` and apply it as reference, not dogma.