Shipkit Review Shipping

shipkit-review-shipping

Review-only verification — Claude reviews your work across 12 quality dimensions and reports findings with evidence. You decide what to fix.

Report, don't fix. Scans changes, classifies findings by severity, and presents a structured report. The reviewer never modifies code.

When to Invoke

• /verify — review all recent changes
• /verify <feature> — focus on specific area
• "Check my work", "Am I ready to commit?"

For deeper review: If user asks for "deep check", "thorough review", or "really scrutinize this" → see Deeper Review Option below.

Arguments

If $ARGUMENTS is provided (e.g. /shipkit-review-shipping auth module), use it as the explicit verification scope. Skip scope detection and treat it as the answer to "What should I verify?".

If $ARGUMENTS is empty, proceed normally with automatic scope detection (git diff, session context).

Process

Completion Tracking

After detecting scope, create tasks:

• TaskCreate: "Pre-scan: Quick pass across 12 dimensions"
• TaskCreate: "Pattern ripple: Expand scope to affected files"
• TaskCreate: "Deep review: All 12 dimensions with evidence"
• TaskCreate: "Write verification-report.json"

TaskUpdate each task to in_progress when starting it, completed when done. Track dimension progress: "Reviewing dimension {N}/12: {name}". The pre-scan is NOT the final report — the deep review (Step 3) must follow. Do NOT present findings until verification-report.json is written to disk.

Step 1: Detect Scope

Determine what to verify:

# Get uncommitted changes
git diff --name-only HEAD

# If nothing uncommitted, check recent commits
git diff --name-only HEAD~3..HEAD

Also consider: What did Claude work on this session?

If unclear, ask: "What should I verify?"

Step 1.1: Quick Pre-Scan (Before Loop)

Run a lightweight first pass across the 12 dimensions on the changed files only (no pattern ripple yet). This is a fast scan to surface the landscape of findings.

Present a summary to the user:

## Pre-Scan Summary

Found findings across these themes:

| # | Finding | L · I · E |
|---|---------|-----------|
| 1 | 🔴 Missing auth checks on 2 API routes | L:High · I:High · E:Low |
| 2 | 🔴 Broken import in Dashboard.tsx | L:High · I:High · E:Low |
| 3 | 🟡 Magic numbers in config (3 instances) | L:Low · I:Low · E:Low |
| 4 | 🟡 Missing loading states on admin pages | L:Medium · I:Medium · E:Medium |
| 5 | 🟢 console.logs left in (4 files) | L:Low · I:Low · E:Low |

**Any themes to dismiss?** I'll skip dismissed themes during the detailed review.
(Reply "none" to proceed with all, or list numbers to dismiss)

Wait for user response. The user may:

• Dismiss themes: "dismiss 3 and 4" → those won't be flagged or fixed
• Dismiss none: "none" or "go" → proceed with everything
• Add context: "3 is intentional, 4 is low priority" → record the reasoning

Step 1.2: Expand Scope via Pattern Ripple

Changed files may affect other files using the same patterns. Expand verification scope:

Index-Accelerated Ripple — If .shipkit/codebase-index.json exists, use concepts mapping to immediately identify related files by concept area (e.g., if a changed file is in concepts.auth, all auth files are in the ripple scope). This narrows the Explore agent's search significantly.

USE SUBAGENT FOR PATTERN RIPPLE - Launch Explore subagent for efficient parallel scanning:

Agent tool with subagent_type: "Explore"
Prompt: "Detect pattern ripple from these changed files: [list files]
[If index exists, include: 'The codebase index maps these concept areas: [list concepts with their files]. Use this to immediately identify same-concept files. Focus your scanning on cross-concept ripple effects the index doesn't capture.']

For each file, identify which patterns it uses:
- Auth: getSession, requireAuth, isAuthorized
- API Response: NextResponse, Response.json
- Error Handling: try/catch, .catch(
- Validation: zod, schema.parse
- Data Fetching: fetch, useSWR, useQuery

Then expand: find ALL other files using the same patterns.
Return: pattern type → list of affected files (both changed and ripple)."

Why subagent: Pattern ripple requires multiple parallel greps across the codebase. Explore agent handles this efficiently and returns a focused summary.

Fallback (if subagent unavailable) - Manual pattern detection:

Expansion logic:

1. For each changed file, grep for pattern indicators
2. If pattern detected, expand scope:
   • Auth pattern → Grep: pattern="getSession|requireAuth|isAuthorized" glob="**/*.{ts,tsx}"
   • API route changed → Glob: pattern="**/api/**/route.{ts,js}"
   • Validation pattern → Grep: pattern="zod|schema\.parse" glob="**/*.{ts,tsx}"
3. Add ALL matches to verification scope
4. Mark as RIPPLE:pattern-type in output

Scope categories:

Report ripple scope in output:

Reviewed: 3 changed + 7 ripple files
- Changed: src/api/trips/route.ts, src/lib/auth.ts, src/components/Form.tsx
- Ripple (auth): +4 files (detected auth pattern change)
- Ripple (api): +3 files (detected API route change)

When to expand:

• Auth file changed → Check ALL auth usages for consistency
• API route changed → Check other routes for response shape consistency
• Validation schema changed → Check all forms using that schema
• Error handling pattern changed → Check all async code for consistency

When NOT to expand:

• Simple typo fix in a comment
• Style-only changes
• Test file changes (contained scope)

Step 2: Read Context

Load relevant context for the review:

Step 3: Work Through Quality Dimensions

Review changes against these 12 dimensions, emphasizing based on what changed.

FOR LARGE CHANGE SETS (10+ files), USE PARALLEL SUBAGENTS:

Launch these Agent subagents IN PARALLEL (single message, multiple tool calls):

1. STRUCTURAL & SPEC AGENT (subagent_type: "Explore")
   Prompt: "Verify structural integrity and spec alignment for these files: [list files]
   STRUCTURAL: Check for orphan code, missing wiring, broken imports, circular deps, incomplete refactors.
   SPEC: If .shipkit/specs/active/* exists, verify implementation matches spec criteria.
   Report findings with file:line evidence and classification (NOT_CREATED, CREATED_UNUSED, WIRING_MISSING, etc.)."

2. ERROR & SECURITY AGENT (subagent_type: "Explore")
   Prompt: "Verify error resilience and security for these files: [list files]
   ERROR: Check for unhandled async, missing try/catch, empty state handling, swallowed errors.
   SECURITY: Check for auth on routes, input validation, SQL injection risk, hardcoded secrets, XSS, IDOR.
   Report findings with file:line evidence and severity (critical/should-fix/minor)."

3. STATE & PERFORMANCE AGENT (subagent_type: "Explore")
   Prompt: "Verify state management, edge cases, and performance for these files: [list files]
   STATE: Check for race conditions, stale state, missing loading states, N+1 queries.
   EDGE CASES: Check for empty states, boundary values, unicode, timezone issues.
   PERFORMANCE: Check for unnecessary re-renders, missing memoization, heavy deps, unoptimized queries.
   Report findings with file:line evidence."

4. UX & MAINTAINABILITY AGENT (subagent_type: "Explore")
   Prompt: "Verify UX completeness and maintainability for these files: [list files]
   UX: Check for loading indicators, action feedback, destructive confirmations, data-testid/ARIA attrs.
   MAINTAINABILITY: Check for magic numbers, duplicated logic, component duplication, missing types, TODOs, console.logs.
   Report findings with file:line evidence."

When to use parallel agents:

• 10+ files in change set (including ripple files)
• Multiple categories of changes (UI + API + DB)
• Deep verify requested

When to use single-pass:

• Small change set (< 10 files)
• Single category focus
• Quick pre-commit check

IMPORTANT: When using subagents, include dismissed themes in the prompt: "Skip these dismissed themes: [list]"

Step 4: Classify and Report

After scanning all dimensions, classify each finding:

1. 🔴 Critical — Will cause bugs, security issues, or crashes. Must fix before commit.
2. 🟡 Should Fix — Quality issues, tech debt, incomplete work. Fix soon.
3. 🟢 Minor — Suggestions, nice-to-haves, style. Consider.

Write the verification report to .shipkit/verification-report.json (see agent output schema). Present findings to the user with evidence and L-I-E ratings.

Run-scoped output (parallel-safe). verification-report.json is a transient per-run result. Under the orchestration engine, write/read it under the run root (<runDir>/verification-report.json) per install/shared/references/run-artifacts.md; with no run context it stays at .shipkit/verification-report.json (back-compatible). Writer and readers resolve the same base.

The 12 Quality Dimensions

1. Structural Integrity

• Orphan code (defined but never used)
• Missing wiring (component exists but not rendered, route not registered)
• Broken imports (importing deleted/moved file)
• Circular dependencies
• Incomplete refactors (changed in one place, not others)

2. Spec & Intent Alignment

• Implementation matches active spec
• All acceptance criteria covered
• Edge cases from spec handled
• Feature actually solves the stated problem
• Fix didn't break the general case — if a bug spec exists (.shipkit/specs/active/bug-*.json), check that the fix handles the reported case WITHOUT regressing the happy path or other documented edge cases. Read fix.robustness.findings for what was considered.
• Green-but-mock — a surface the spec declares live (in functionalSurface / scenarios) but that still reads mock/stub/hardcoded data is NOT done. This is a hard FAIL — see the Data-Reality Gate below.

3. Error Resilience

• Unhandled promise rejections
• Missing try/catch on async operations
• Empty/null/undefined states not handled
• API failure handling missing
• Missing error boundaries (React)
• Errors swallowed silently

4. State & Data

• Race conditions possible
• Stale cache / state issues
• Missing loading states
• Optimistic updates without rollback
• N+1 query patterns

5. Security

• Auth checks missing on new routes
• Input validation/sanitization missing
• Raw SQL queries (injection risk)
• Secrets/credentials in code
• XSS vulnerabilities
• IDOR (accessing other users' data)

6. Edge Cases

• Empty state not handled (zero items)
• Boundary values (0, negative, very large)
• Unicode/special characters
• Timezone issues
• Concurrent access scenarios
• Overengineered edge case handling — fix for one edge case broke or degraded the happy path or other cases (e.g., added loading guard that blocks normal render, validation so strict it rejects valid input, error handling that swallows success paths)

7. Performance Landmines

• Unnecessary re-renders
• Missing memoization where needed
• Heavy dependency for light use (moment.js for one format)
• Unoptimized queries
• Missing pagination on lists

8. UX Completeness

• Missing loading indicators
• No feedback on user actions
• Broken/incomplete flows
• Missing confirmation on destructive actions
• Inconsistent patterns vs rest of app
• Interactive elements missing data-testid or ARIA attributes (blocks AI testing)

8.5. Design System Compliance (when .shipkit/design-system/ exists)

• If changes include UI code and .shipkit/design-system/ exists: verify design tokens are used instead of hardcoded color/spacing/typography values, and component patterns align with design principles
• If .shipkit/design-system/ does not exist, skip this check entirely

9. Maintainability

• Magic numbers/strings
• Duplicated logic (same thing multiple ways)
• Component duplication (same component in multiple feature directories instead of shared)
• Not using shared components (shared version exists but feature has local copy)
• Similar-purpose components (Modal vs Dialog, Card vs Tile — should standardize)
• Confusing/misleading naming
• Missing types on public interfaces
• TODOs/FIXMEs left behind
• console.logs left in

10. Environment & Config

• New env vars not documented
• Hardcoded URLs/endpoints
• Works locally but will break in prod
• Docker/CI config not updated for new deps
• Missing feature flags for WIP features

10.5. External Service Boundaries

• External API calls without timeout config
• LLM/AI calls without token/output limits
• Serverless routes with external calls missing maxDuration export
• Streaming endpoints without keepalive signals
• Sequential external call chains without cumulative timeout tracking
• Pay-per-call services without cost visibility (token tracking, usage counters)

11. API Contract

• Breaking changes to existing endpoints
• Inconsistent response shapes
• Missing input validation
• Undocumented new endpoints
• Version compatibility issues

12. Database Integrity

• Schema changes without migrations
• Missing indexes on query patterns
• Orphan records possible (no cascading delete)
• Data type mismatches
• Missing foreign key constraints

Verification Integrity Protocol

Critical Rule: Never claim without evidence.

Every finding MUST be backed by actual tool output. Claims like "file not created" or "component unused" without verification are verification theater.

The 5-Step Verification Gate

Before reporting ANY finding, execute these steps:

Language Precision Rules

Use precise language that matches verification evidence:

Never say:

• ❌ "This file is not used" (without grepping for imports)
• ❌ "This component doesn't exist" (without globbing)
• ❌ "The route is missing" (without checking route registration)

Always say:

• ✅ "Grep for UserCard returned 0 matches → unused"
• ✅ "Glob for **/auth-callback.* returned empty → not created"
• ✅ "Found import UserCard but Glob for UserCard.tsx returned empty → broken import"

State Classification

Classify each finding into ONE of these states:

Evidence Requirements by Dimension

Structural Integrity (Dimension 1):

Orphan code:
  1. Glob: Find the file (confirm exists)
  2. Grep: Search for imports/uses of exported symbols
  3. If Grep returns 0: CREATED_UNUSED

Missing wiring:
  1. Glob: Confirm component exists
  2. Grep: Search for render/registration (JSX tags, route config)
  3. If Grep returns 0: WIRING_MISSING

Broken imports:
  1. Read: Find import statements
  2. For each import, Glob: Does target exist?
  3. If Glob returns empty: BROKEN_IMPORT

Error Resilience (Dimension 3):

Unhandled async:
  1. Grep: pattern="await " in changed files
  2. Read: Context around each await
  3. Check: Is it wrapped in try/catch?
  4. Report: file:line for unwrapped awaits

Security (Dimension 5):

Hardcoded secrets:
  1. Grep: pattern="(password|secret|key|token)\s*[:=]\s*['\"]"
  2. Exclude: .env files, test fixtures
  3. Report: file:line for matches

Spec Alignment (Dimension 2):

Acceptance criteria check:
  1. Read: Active spec file
  2. Extract: Each acceptance criterion
  3. For each criterion:
     - Grep/Read: Find implementing code
     - Verify: Does implementation satisfy criterion?
  4. Report: Which criteria verified vs not found

Updated Output Format

Each finding MUST include evidence and an L-I-E assessment:

### 🔴 Critical (fix before commit)

**Structural: Orphan component** [CREATED_UNUSED] — `L:Medium · I:Low · E:Low`
- Evidence: `Glob("**/UserCard.tsx")` → found at src/components/UserCard.tsx
- Evidence: `Grep("UserCard" in "**/*.{ts,tsx}")` → 1 match (definition only)
- File: `src/components/UserCard.tsx`
- Classification: CREATED_UNUSED (exported but never imported)
- Why: Dead code ships to production, likely incomplete feature

**Structural: Broken import** [BROKEN_IMPORT] — `L:High · I:High · E:Low`
- Evidence: `Read("src/pages/Dashboard.tsx")` line 5: `import { AuthCard } from './AuthCard'`
- Evidence: `Glob("**/AuthCard.*")` → 0 matches
- File: `src/pages/Dashboard.tsx:5`
- Classification: BROKEN_IMPORT (import target doesn't exist)
- Why: Will crash at build or runtime on any page that loads Dashboard

Anti-Patterns (What NOT to Do)

❌ Assumption without verification:

"The UserCard component appears unused"
→ No Grep was run to verify

❌ Vague classification:

"There might be issues with the auth flow"
→ No specific finding, no evidence, no file:line

❌ Tool mention without execution:

"You should check if UserCard is imported anywhere"
→ YOU should check, not tell user to check

❌ Conflating states:

"UserCard is missing"
→ Missing = not created? Or created but not wired? Different fixes.

✅ Correct pattern:

1. Run Glob to confirm file exists
2. Run Grep to check for usage
3. Classify: NOT_CREATED / CREATED_UNUSED / WIRING_MISSING
4. Report with evidence and file:line

See references/detection-patterns.md for Glob/Grep patterns by issue type.

Data-Reality Gate (green-but-mock = FAIL)

This is the one hard gate in an otherwise report-only skill. Everything else here is advisory; this produces a FAIL verdict the engine acts on. It enforces frontend-implies-backend as a build gate — the spec's no-gaps checklist declared the contracts and datastores; this confirms the built surfaces actually read them, not a mock seam.

Why: the most expensive rework pattern is a surface that looks done — renders, passes a glance — while reading mock/stub/hardcoded data because its backing view / API / table was never wired. It "works" in the demo and breaks the moment real data flows. A green-but-mock surface is not done.

What to scan

For every surface the spec declares live (present in the spec's functionalSurface or in a scenario's then, and NOT listed in deferred / acceptanceCriteria.wontHave), check the built code for mock seams:

Optional: get the evidence from the detector

The Grep patterns above find seams; they cannot tell you whether a seam sits on a surface the spec declares live — which is the only thing that makes a seam a FAIL. mock-seam-detector.py does that cross-check deterministically, and ships with shipkit-semantic-qa:

FID=".claude/skills/shipkit-semantic-qa/tools/fidelity"
[ -d "$FID" ] || FID="$HOME/.claude/skills/shipkit-semantic-qa/tools/fidelity"

python "$FID/mock-seam-detector.py" . \
  --spec .shipkit/specs/shipped/*.json --report

Each finding comes back with surface + declaredLive, and summary.gatingSeams is the count that actually fails this gate. Use it to find candidates and check the declared-live question — then confirm each one by Read before you fail a build on it. Two properties matter here:

• It fails open. A seam it cannot tie to a declared surface returns declaredLive: false — advisory, not gating. Undeclared and deferred surfaces are the false-positive class this gate must not fire on.
• It is a regex heuristic. It has no opinion on whether a seam is real. declaredLive: true narrows where to look; your Read is still the evidence.

Findings on deferred surfaces are suppressed automatically (deferred[] / acceptanceCriteria.wontHave), matching the rule below.

The rule

• A surface the spec declares live that still reads mock/stub/hardcoded data → 🔴 Critical, verdict FAIL. Report it with file:line evidence (Grep/Read output), classified CREATED_WRONG (built, but reading mock instead of the declared contract).
• A mock seam on a surface the spec explicitly defers (deferred / wontHave) is fine → note it, don't fail.
• Record the result on the report: dataReality: { status: "pass" | "fail", mockSeams: [ { surface, file, line, declaredLive, evidence } ] }. Any declaredLive mock seam sets the overall verdict to FAIL — the engine (steered mode) reads this and re-dispatches the surface to be wired to live data before it advances.

This is the build-time counterpart to the spec-time no-gaps gate: the spec named the contracts; this proves the surfaces consume them.

Contextual Emphasis

Not all dimensions every time. Emphasize based on what changed:

Output Format

Final report after scanning all dimensions:

## Verification Report

Reviewed: 5 files (src/api/auth/*, src/middleware/*)
Context: Auth feature implementation

### 🔴 Critical — Fix Before Commit

| Finding | L · I · E |
|---------|-----------|
| **Security: Missing auth check** | L:High · I:High · E:Low |

- `src/api/trips/route.ts:12` — no getSession() call on protected route
- Evidence: Grep for `getSession` returned 0 matches in file

### 🟡 Should Fix

| Finding | L · I · E |
|---------|-----------|
| **UX: Missing loading state** | L:Medium · I:Medium · E:Low |

- `src/components/LoginForm.tsx` — no loading indicator during submit

### 🟢 Minor / Consider

| Finding | L · I · E |
|---------|-----------|
| **Maintainability: Magic string** | L:Low · I:Low · E:Low |

- `src/api/auth/login.ts:12` — hardcoded "7d" for token expiry
- Consider: `const TOKEN_EXPIRY = "7d"`

### ⏭️ Dismissed (by user)

- Magic numbers in config (user: "intentional")
- Missing loading states on admin pages (user: "low priority")

---

1 critical | 1 should fix | 1 minor | 2 dismissed

Priority Definitions

L-I-E Assessment (Likelihood · Impact · Effort)

Every finding MUST include a compact L-I-E rating to help the user prioritize.

Notation: Use inline format — L:High · I:High · E:Low

Priority should follow from L-I-E:

• 🔴 Critical = High likelihood OR high impact (regardless of effort)
• 🟡 Should Fix = Medium likelihood + medium impact
• 🟢 Minor = Low likelihood AND low impact

What This Skill Does NOT Do

• Make architectural decisions — reports findings, defers all fixes to user
• Override dismissals — dismissed themes stay dismissed for the entire session
• Block commits — advisory, not a gate — except the Data-Reality Gate: a spec-declared-live surface reading mock/stub data produces a hard FAIL verdict the engine acts on (green-but-mock is not done)
• Fix code — read-only reviewer, never modifies source code
• Run linters/tests — this is Claude reasoning, not tooling
• Guarantee completeness — best effort review

Team Mode

When running as part of an Agent Team:

• Check .shipkit/team-state.local.json — if present, you're in team mode
• Read the spec from the team state's specPath for acceptance criteria
• Message the lead with the verification verdict (PASS/FAIL + details)
• Message teammates directly when issues found in their files
• Broadcast summary to the whole team when verification completes
• The lead typically runs this after all implementation tasks complete

Context Files This Skill Reads

Context Files This Skill Writes

• verification-report.json (run-scoped under <runDir>/, else .shipkit/) — QA verdict written on every run (summary + per-check results the engine reads in steered mode for routing decisions). Includes dataReality: { status, mockSeams[] } — any spec-declared-live mock seam forces the overall verdict to FAIL.

Deeper Review Options

This skill provides a fast single-pass review — good for daily use.

For visual UI verification, use the dedicated visual QA skill:

• /shipkit-qa-visual — maintains UI goals (pages × personas), generates Playwright tests, runs headlessly

For thorough multi-agent review, two plugins are available:

# Install (once)
/plugin install code-review@claude-code-plugins
/plugin install pr-review-toolkit@claude-code-plugins

# Use
/code-review                      # 4-agent review
/pr-review-toolkit:review-pr      # 6-agent deep analysis

When to use deeper review:

• Before merging to main
• Complex changes touching multiple systems
• Security-sensitive code
• When fast verify flags concerns worth investigating further

Trade-off: 4-6x token cost, slower, but catches more subtle issues.

After Completion

Assessment written to verification-report.json (run-scoped under <runDir>/ when the engine set a run root; .shipkit/verification-report.json otherwise).

Next: This skill is the per-unit review work the engine (shipkit-orchestrate, steered mode) fans out — one unit per app-area / screen (frontend + backend). The engine reads this assessment as ground truth and surfaces it to the user to steer:

• If gaps found (including a Data-Reality FAIL — a spec-declared-live surface still on mock/stub data): the engine re-dispatches the affected upstream work for revision, then re-runs this reviewer against the same unit. Green-but-mock does not advance.
• If pass: the engine advances that unit and reports it as met against the bar.

The engine (main session) owns the dispatch/reconcile loop; this skill owns the per-unit judgment. It can also be run directly for an ad-hoc review of the current diff.

Want deeper scrutiny? Use /code-review (4 agents) or /pr-review-toolkit:review-pr (6 agents) — requires plugins.

Success Criteria

• Scope detected from git diff or session context
• Pre-scan presented to user with dismissal option
• Relevant quality dimensions emphasized (excluding dismissed)
• Specs/architecture read for compliance
• All findings backed by tool evidence (Glob/Grep/Read)
• Findings classified with L-I-E ratings
• Data-Reality Gate run: spec-declared-live surfaces checked for mock seams; any green-but-mock surface sets verdict FAIL (dataReality recorded on report)
• Final report shows: critical, should-fix, minor, dismissed