review-behavior-change

Files (1)

• SKILL.md

SKILL.md (first lines)

# Surface Change Review

## Execution Model

Delegate the ENTIRE workflow below to a single top-level sub-agent (Agent tool, **foreground**, `model: "sonnet"`). The main context must not perform the analysis itself — this prevents conversation history from biasing the review. Pass the sub-agent the full skill instructions (everything below this section), the `$ARGUMENTS`, and the repository path. The sub-agent performs all phases and returns the final report. The main context relays the report to the user and handles any follow-up (CHANGELOG approval, etc.).

---

Review recent code changes (uncommitted, commit, or branch diff) for behavioral changes at the system's surface — the outermost layer consumers interact with (HTTP endpoints, CLI entry points, exported functions, library public APIs). Flag any surface-level behavioral change that is not explicitly justified in a docs/ artifact (ADR, PRD, tech spec, or implementation plan).

## Change Severity: ADR vs CHANGELOG

Not every surface change warrants an ADR. Classify each unjustified change by severity:

**ADR-required** (architectural/breaking changes):
- Removing or renaming a public endpoint, function, or field
- Changing a function signature in a way that breaks existing callers
- Changing the semantics of an existing API (same inputs produce fundamentally different outputs)
- Switching protocols, storage backends, or communication patterns
- Changes that require consumers to update their code

**CHANGELOG-only** (minor behavioral improvements):
- Improving error messages (better wording, adding context/field names)
- Tightening validation (rejecting previously-accepted invalid input)
- Returning more specific error codes where a generic one was used before
- Adding fields to a response (non-breaking addition)
- Fixing a status code to be more semantically correct (e.g., 500 → 422 for bad input)
- Improving output formatting without changing data content
- Any change that makes the API "more correct" without breaking existing valid usage

The key distinction: if existing consumers of the API **would break** or **get different results for the same valid inputs**, it's ADR-worthy. If the change only affects error paths, invalid inputs, or adds information — it's a CHANGELOG entry.

## CHANGELOG Format

When suggesting changelog entries, use this format (entries go in `CHANGELOG.md` at the repo root under `## Behavioral Changes`):

```
- **YYYY-MM-DD** `<surface identifier>` — <what changed>: <new behavior> (was: <old behavior>)
```

Examples:
```
- **2026-05-17** `POST /enroll` — Improved error: now returns field name in validation errors (was: generic "invalid request")
- **2026-05-17** `GET /items` — Tightened: malformed cursor now returns 400 with message (was: empty 200)
- **2026-05-15** `Run()` — Fixed: negative qty now returns exit code 1 with message (was: panic)
```

## Human vs Agent Invocation

When this skill is invoked directly by a human (the default — no `--agent` flag in `$ARGUMENTS`), suggest CHANGELOG entries for minor behavioral changes. Present the suggested entries and let the user approve before any file is modified.

When invoked with `--agent` in `$ARGUMENTS` (called from another skill or automation), only report violations — do not suggest changelog entries or prompt for approval.

## What Counts as "The Surface"

The surface is defined by the same philosophy as the `surface-testing` skill:

- **HTTP/gRPC API**: endpoints, request/response shapes, status codes, error codes, headers
- **CLI**: flags, subcommands, output format, exit codes
- **Library public API**: exported function signatures, return types, error types, exported struct fields
- **Internal library boundaries**: packages that expose a complete, self-contained API (could be extracted independently)

## Quick Signal: Test File Changes

Before deep analysis, check whether existing test files were **modified** (not just added). This is the fastest indicator of a surface change:

- **Only new test files or only additive changes** (new test functions, new test cases added to existing tables): likely a new feature — no violation.
- **Modified or deleted assertions in existing tests**: the surface likely changed. This triggers deep analysis.
- **Renamed test functions or restructured test tables**: could be refactoring OR a behavioral change — triggers deep analysis.

If no existing test assertions were modified/deleted, AND the deep analysis (Phase 1) finds no other surface changes, report clean and exit early.

## Inputs

Determine the diff to review based on `$ARGUMENTS`:

- No arguments: use uncommitted changes (`git diff` + `git diff --cached`)
- `--commit <sha>`: diff that specific commit against its parent
- `--branch` or no flag with a branch name: diff current branch against main (`git diff main...HEAD`)
- A file path: diff only that file (uncommitted changes)

Also locate the docs directory:
- Look for `docs/` at the repository root
- Within docs, look for: `adr/`, `features/` (containing PRDs, tech specs), and any `*-plan.md` or `implementation-plan.md` files
- List all doc files found (paths only — don't read them yet)

## Phase 1: Change Analysis Agent

Spawn a sub-agent (Agent tool, **foreground**, `model: "sonnet"`) to analyze the diff and classify changes.

**Prompt structure:**

```
You are analyzing a code diff for surface-level behavioral changes. Your job is to identify changes that alter the system's public interface or observable behavior — NOT internal refactoring.

DIFF:
<full diff content>

TEST FILE CHANGES (if any):
<subset of diff that touches test files — show full hunks>

Classify each meaningful change as one of:

### Surface Change <N>: <short title>
**Type**: <one of: endpoint-added, endpoint-removed, endpoint-modified, signature-changed, return-type-changed, error-behavior-changed, field-added, field-removed, field-renamed, status-code-changed, flag-added, flag-removed, output-format-changed, breaking-removal>
**Severity**: <ADR — breaking/semantic change to valid usage | CHANGELOG — improvement to error paths, validation, or non-breaking addition>
**File**: <path:line>
**Before**: <what it was (quote old code or describe)>
**After**: <what it is now (quote new code or describe)>
**Breaking**: <yes/no — would existing consumers break without code changes?>
**Test impact**: <were existing tests modified to accommodate this change? cite the test file and what changed>

SEVERITY RULES:
- If existing valid inputs would produce different outputs or errors → ADR
- If a public symbol is removed or renamed → ADR
- If only invalid inputs are now handled differently (better errors, tighter validation) → CHANGELOG
- If a response adds new fields without removing/changing existing ones → CHANGELOG
- If an error message text changed but error type/code is the same → CHANGELOG
- If a wrong status code is corrected (e.g., 500→422 for invalid input) → CHANGELOG

From thrawn01/claude-dotfiles, MIT.

More from thrawn01/claude-dotfiles