A code review skill you can steal

~/.claude/skills/samueleastdev/
  SKILL.md                    # the workflow, below
  references/
    architecture.md           # the standard, below
    contracts.md              # the contracts guide, below

# then, in any repo:
cd ~/code/some-project
claude

/samueleastdev arch         # install the standard, then review against it
/samueleastdev contracts    # set up contracts across your clients

---
name: samueleastdev
description: "Set a project's engineering standards up and hold code to them. `/samueleastdev arch` installs an architecture and code quality standard then reviews against it. `/samueleastdev contracts` sets up cross-platform contracts so every client makes the same decisions. Use for /samueleastdev, 'architecture review', 'review this repo', 'set up contracts', or when starting a new project."
---

# samueleastdev

Two modes. The argument picks one.

| Command | Does |
|---|---|
| `/samueleastdev arch` | Install the architecture standard in this repo, then review the code against it |
| `/samueleastdev contracts` | Set up a `contracts/` folder, or add and maintain one contract |

With no argument, say what the two do and ask which. Do not guess.

Self-contained: everything is in `references/`, so this folder can be copied
onto any machine and used as-is.

---

## Mode: arch

The standard is `references/architecture.md`. Read it in full, 21 sections,
before judging anything. It is principles, not a checklist, so do not grep it
for rules to match. The review output format is its last section.

### 1. Install it, if it is not installed

A standard nothing points at does not get followed. Check for a `CLAUDE.md` at
the repo root that references an architecture standard. If there is none:

- Copy `references/architecture.md` to `docs/architecture.md` in the repo.
- Create or append to `CLAUDE.md`, naming the file and saying that code in
  this repo is held to it.
- Say what you added. Do not do it silently.

The repo's own copy is then the one that governs. If the repo already has a
standard, that one wins, even where it disagrees with this one. Say where they
differ rather than overriding it.

### 2. Review

Default to the working tree diff against the base branch. That is the code
about to ship and the only place a review changes an outcome. Whole repo only
when asked, or when the diff is empty. Say which you did, in the first line.

Before judging any area, answer section 1's four questions for it: what
problem is being solved, which component should own it, what it interacts
with, and what its inputs and outputs are. A finding that misreads ownership
is noise.

Match the house style already in the repo. Consistency with the surrounding
code beats the reviewer's preference. If the repo's convention is itself the
problem, say so once, as one finding, not once per instance.

Pay particular attention to where agent-written code goes wrong: patterns
applied without a demonstrated problem (section 7), singletons that became
dependency dumping grounds, engines that became containers for every related
feature, and the smells in section 21.

### 3. Report

Classify every finding CRITICAL, IMPORTANT, SUGGESTION or GOOD. For each:
what is wrong, why it matters, where, a concrete improvement, and whether
fixing it now is worthwhile. Then ask whether to fix the accepted ones. Do not
start changing code as part of a review.

---

## Mode: contracts

The guide is `references/contracts.md`. Read it in full first.

### 1. Decide whether this repo needs them

Three tests, all of which must pass: more than one client implements the same
behaviour; at least one of them cannot import the shared implementation and
hand ports it instead; and getting it wrong is silent rather than loud.

If they do not all pass, say so and stop. A single-client repo does not need
this ceremony, and adding it is the "patterns without a demonstrated problem"
failure the arch standard warns about.

### 2. Find the real contracts

Do not invent a taxonomy. Find the decisions that already exist in more than
one place:

- Where do two clients implement the same logic in different languages?
- What has already gone wrong on one platform and not another?
- Which values are restated in several files, so changing one means
  remembering the rest?

Each answer is a candidate. Name them after the decision they govern, not the
subsystem.

### 3. Scaffold

`contracts/README.md` with the index table, one folder per contract each
holding a `SKILL.md`, and a symlink from `.claude/skills/<name>` to each
folder so agents load them automatically. Start with the master contract and
the parity contract, per the guide.

Write real content for one contract, fully, as the worked example. Leave the
rest as named stubs with a one-line scope. A folder of empty contracts is
worse than two real ones.

### 4. Gate what can be gated

A contract with a gate is a standard. A contract without one is a document,
and documents drift. Propose the specific check: a conformance suite both
implementations reproduce, a parity data file with a build gate, or a mirrored
test in the ported language. Say in each contract which parts are enforced and
which are on trust.

---

## Hold the line on these

- **Do not manufacture issues to fill a report.** "No significant
  architectural issues found" is a valid result. Use it when it is true.
- **Do not optimise for cleverness**, and do not propose a pattern because it
  is considered best practice. The simplest architecture that cleanly solves
  the problem is usually preferable.
- **Complexity in the data is not complexity in the design.** Some problems
  are legitimately hard. Do not flag a design for matching its domain.
- **Rank honestly.** If everything is IMPORTANT then nothing is. Three real
  findings beat twenty.
- **Do not create structure ahead of need.** Neither mode should leave a repo
  with folders waiting for content that may never arrive.

# Architecture & code quality principles

The standard this skill reviews against. Principles, not a procedure, so
read it whole rather than grepping it for rules to match. The review output
format is the last section.

---

## Purpose

Act as a senior software engineer and architecture reviewer.

The goal is to produce code that is:

- Simple
- Readable
- Maintainable
- Testable
- Loosely coupled
- Easy to diagnose
- Consistent with the existing codebase
- Appropriately abstracted
- Easy for another engineer to change safely

Do not optimise for cleverness.

Do not introduce architectural patterns simply because they are considered "best practice."

The simplest architecture that cleanly solves the problem is usually preferable.

---

# 1. Understand Before Coding

Before making a significant change, understand:

1. What problem are we solving?
2. Which component should own this responsibility?
3. What existing components does it interact with?
4. What are its inputs and outputs?
5. What state does it own?
6. What external side effects does it cause?
7. What existing patterns does the repository already use?
8. What platform-specific behaviour exists?
9. What could fail?
10. How can the behaviour be tested?

Inspect relevant existing code before inventing a new architecture.

Do not create a second architectural approach when the repository already has a suitable established pattern.

For non-trivial changes, briefly state the intended responsibilities and boundaries before implementation.

---

# 2. Single Responsibility

Every:

- module
- component
- class
- service
- hook
- function
- engine
- controller

should have a clear reason to exist.

Be suspicious when one component:

- fetches data
- transforms data
- manages UI
- owns business state
- handles persistence
- reports analytics
- manages errors
- constructs dependencies

all at once.

Split responsibilities when doing so creates meaningful boundaries.

Do NOT split code purely to make files smaller.

---

# 3. Separation of Concerns

Keep unrelated concerns separate.

Examples:

UI → presentation and user interaction

Business/domain logic → application behaviour

Networking → API communication

Persistence → storage

Analytics → reporting

Platform integration → OS/device-specific behaviour

Configuration → runtime/environment configuration

Construction → factories/bootstrap/dependency wiring

A change to one concern should ideally not require unrelated parts of the system to change.

---

# 4. Coupling

Avoid unnecessary tight coupling.

A component should know as little as reasonably possible about the implementation details of another component.

Prefer narrow contracts between components.

Ask:

"If I replaced this implementation tomorrow, how much unrelated code would need changing?"

If the answer is "a lot," investigate the coupling.

Do not create interfaces for everything.

Interfaces, protocols or abstractions should exist when they provide real value such as:

- multiple implementations
- testing boundaries
- platform independence
- dependency inversion
- isolation from unstable dependencies

---

# 5. DRY — Don't Repeat Yourself

Look for duplicated:

- business rules
- validation
- transformations
- state transitions
- error handling
- configuration
- algorithms

Extract shared behaviour when duplication represents the same concept.

However:

DO NOT remove duplication when doing so creates a worse abstraction.

Three readable lines duplicated twice can be better than a generic framework nobody understands.

Prefer:

meaningful reuse

over:

reuse for the sake of reuse.

---

# 6. Abstraction

Before creating an abstraction ask:

"What problem does this abstraction solve?"

Good abstractions:

- reduce coupling
- hide genuine complexity
- provide a stable contract
- enable substitution
- improve testability
- centralise a genuine shared concept

Bad abstractions:

- merely wrap another function
- exist only to follow a pattern
- require jumping through many files to understand simple behaviour
- make debugging harder
- predict hypothetical future requirements

Do not design for imaginary future problems.

---

# 7. Design Patterns

Patterns are tools, not goals.

Consider patterns such as:

- Factory
- Strategy
- Adapter
- Observer
- Repository
- Dependency Injection
- Singleton
- Engine
- Controller
- State Machine

only when they solve a demonstrated architectural problem.

## Factories

Consider a factory when:

- construction is complex
- implementation depends on platform/configuration
- multiple implementations exist
- callers should not know construction details

Do not create a factory just to call a constructor.

## Singletons

Use singletons cautiously.

Consider them only when there genuinely should be one shared lifecycle or resource.

Avoid turning a singleton into:

- global mutable state
- a dependency dumping ground
- an invisible service locator

Prefer explicit dependencies where practical.

## Engines

An engine can be appropriate when coordinating a substantial domain lifecycle.

Examples:

PlaybackEngine
RenderingEngine
RulesEngine

An engine should coordinate behaviour rather than become a giant container for every related feature.

---

# 8. Platform-Agnostic Code

Shared code should remain platform agnostic where practical.

Do not allow:

Android assumptions
iOS assumptions
Web assumptions
device-specific behaviour

to leak into shared/domain code unnecessarily.

Prefer:

Shared behaviour
↓
Platform contract
↓
Platform implementation

But do not create a platform abstraction when behaviour is genuinely universal.

---

# 9. Composition

Prefer small components collaborating through clear boundaries over giant components.

Prefer composition over deep inheritance hierarchies.

Avoid "Manager" classes that gradually become responsible for everything.

When a component becomes large, ask whether it is coordinating several distinct responsibilities.

---

# 10. Functions

Functions should be understandable quickly.

Avoid:

- huge functions
- excessive nesting
- unclear side effects
- boolean argument puzzles
- deeply nested conditionals
- clever one-liners that reduce readability

Prefer:

- early returns
- descriptive names
- small logical units
- explicit behaviour
- predictable control flow

Do not split a readable function into many tiny functions merely to satisfy arbitrary line limits.

There is no fixed maximum function length.

Complexity and responsibility matter more than line count.

---

# 11. Readability

Code is read more often than it is written.

Prefer code that another engineer can understand without mentally decoding it.

Avoid unnecessary:

- abstraction
- generics
- metaprogramming
- indirection
- clever syntax
- compressed expressions

Names should communicate intent.

Bad:

process()
handle()
doThing()
manager
data

Better:

createPlaybackSession()
validateEntitlement()
reportPlaybackFailure()
manifestParser

Comments should explain WHY something exists when the reason is not obvious.

Do not use comments to explain confusing code that could simply be made clearer.

---

# 12. State

State ownership must be clear.

Ask:

- Who owns this state?
- Who may modify it?
- Who observes it?
- What is the source of truth?
- What happens when it changes?

Avoid:

- duplicated sources of truth
- hidden mutable global state
- unrelated components modifying shared state
- state synchronisation hacks

Prefer predictable state transitions.

Consider a state machine when behaviour has meaningful explicit states and transitions.

---

# 13. Dependencies

Dependencies should be visible.

Avoid components secretly reaching into global services where practical.

Dependency injection can be useful when it:

- improves testing
- allows implementations to change
- makes dependencies explicit

Do not build a dependency injection framework when simple constructor/function injection is enough.

---

# 14. Error Handling

Failures should be understandable and diagnosable.

Do not silently swallow errors.

Errors should provide useful context about:

- what failed
- where it failed
- relevant identifiers/state
- underlying cause where appropriate

Avoid excessive catch/rethrow chains that add no useful information.

Consider failure isolation.

One failing component should not unnecessarily break unrelated behaviour.

---

# 15. Logging & Observability

Logs should help diagnose real problems.

Avoid noisy logs that obscure useful information.

Important operations should expose enough context to reconstruct failures.

Consider:

- structured logging
- identifiers
- lifecycle events
- timing
- failure reasons

Never log secrets, credentials or sensitive user information.

---

# 16. Testability

Code that contains meaningful behaviour should normally be testable.

Prioritise tests around:

- business logic
- transformations
- state transitions
- failure behaviour
- important edge cases
- public contracts
- regressions

Avoid tests that merely verify implementation details.

Tests should allow refactoring without unnecessarily breaking.

Prefer:

"Given X, behaviour Y occurs."

over:

"Private method Z was called exactly twice."

Do not add meaningless tests simply to increase coverage.

---

# 17. Change Scope

Keep changes focused.

Do not refactor unrelated areas while implementing a feature unless necessary.

Avoid "while I'm here" rewrites.

Large unrelated refactors:

- increase review difficulty
- increase regression risk
- hide important changes

If an unrelated architectural problem is discovered, identify it separately.

---

# 18. Dead Code

Remove obsolete code created by the change where safe.

Do not leave:

- unused functions
- abandoned implementations
- commented-out code
- unnecessary compatibility branches
- duplicate implementations

unless there is a documented reason.

---

# 19. Performance

Do not prematurely optimise.

First prefer correct, readable code.

Investigate performance when there is:

- measured evidence
- known scale requirements
- hot-path behaviour
- obvious unnecessary work

Watch for:

- repeated expensive operations
- unnecessary allocations
- repeated network calls
- N+1 queries
- unnecessary renders
- unnecessary serialization
- unbounded collections

Optimisation should not destroy maintainability without justification.

---

# 20. Security

Consider trust boundaries.

Validate untrusted input.

Do not expose:

- credentials
- tokens
- secrets
- internal implementation details
- sensitive user information

Use existing security mechanisms rather than creating custom cryptography or authentication mechanisms.

---

# 21. Architecture Smells

Actively look for:

## God Object

One component owns too many responsibilities.

## Spaghetti Code

Behaviour crosses many components without clear ownership.

## Tight Coupling

Components depend heavily on each other's internals.

## Shotgun Surgery

One simple behaviour change requires modifications throughout the codebase.

## Feature Envy

A component repeatedly manipulates another component's internal data.

## Primitive Obsession

Complex concepts are represented by loosely related primitive values.

## Boolean Explosion

Behaviour is controlled by combinations of many boolean flags.

## Hidden Dependencies

Components access dependencies that are not obvious from their API.

## Premature Abstraction

A framework exists before there is a real requirement for it.

## Copy/Paste Architecture

Similar behaviour exists in multiple places and gradually diverges.

## Manager Explosion

Numerous vaguely named Manager classes hide unclear responsibilities.

---

# 24. Complexity Challenge

Before finalising a solution ask:

"Could this provide the same functionality with less code, fewer abstractions, fewer dependencies or fewer moving parts while remaining clear and maintainable?"

If YES, simplify it.

Do not confuse:

more architecture

with:

better architecture.

---

# 25. Future Change Test

Ask:

"If this requirement changes six months from now, which components need changing?"

Good boundaries should contain the blast radius of likely changes.

Do not attempt to predict every future requirement.

Optimise for reasonable change, not hypothetical infinite flexibility.

---

# Core Principle

The objective is not to produce the most sophisticated architecture.

The objective is to produce the simplest code that:

- clearly expresses its intent
- has well-defined responsibilities
- interacts cleanly with the rest of the system
- can be tested
- can be diagnosed
- can be changed safely
- can be understood by the next engineer

# 26. Review Output

When reviewing existing code, classify findings as:

### CRITICAL

Likely bug, data loss, security issue, major architectural problem or severe maintainability issue.

### IMPORTANT

Coupling, unclear responsibility, poor testability, duplication, difficult diagnosis or significant unnecessary complexity.

### SUGGESTION

Readability, naming, simplification or minor structural improvement.

### GOOD

Call out architectural decisions that should be preserved.

For every problem provide:

1. What is wrong.
2. Why it matters.
3. Where it occurs.
4. A concrete improvement.
5. Whether fixing it now is worthwhile.

Do not manufacture issues merely to produce review comments.

"No significant architectural issues found" is a valid result.

---

# Contracts

How to give a project a set of contracts, and how to keep them honest.

---

## What a contract is

A contract is one written rule that every client in a project must follow,
kept as prose next to the code rather than inside it.

It exists because the same decision gets made in more than one place, in more
than one language, and the copies drift. A drifted copy does not fail loudly.
It produces a plausible wrong answer on one platform while the others stay
right, which is the most expensive kind of bug to find.

## When a project needs them

Contracts earn their place when **all three** are true:

1. More than one client implements the same behaviour.
2. At least one client cannot import the shared implementation, so it hand
   ports the logic. Swift and Kotlin next to a TypeScript engine is the usual
   shape.
3. Getting it wrong is silent. The platforms disagree rather than crash.

If every client imports one implementation, you do not need contracts. You
need that implementation. Do not add this ceremony to a single-client repo.

## Shape

```
contracts/
  README.md                     the index: one table, Contract | Governs
  <contract-name>/
    SKILL.md                    the contract itself
  <contract-name>/
    SKILL.md
```

Each contract is a folder with a `SKILL.md` inside, so coding agents load it
the way they load any other skill. Symlink each one into `.claude/skills/`:

```sh
ln -s ../../contracts/<name> .claude/skills/<name>
```

The files under `contracts/` are the real ones. The symlinks are how agents
find them. Keeping one copy is the point: a contract that exists twice is
already drifting.

## Writing one

Every contract answers a question that a client has to answer before it can
proceed, and that more than one client answers today.

**Frontmatter.** The `description` is the trigger, so write it as the list of
things that should make someone open the file. Name the functions, the files
and the symptoms, not the topic:

```markdown
---
name: playback-negotiation
description: How clients decide which manifest to request and which DRM key
  system to wire up. Read this before touching a mint request body, a protocol
  choice, a user-agent sniff, a key system, OtnetDrm or pickVariant. Also read
  it when one platform plays a title and another cannot, or when a licence
  request fails on one device only.
---
```

A description that says "covers playback negotiation" never gets opened. One
that names `pickVariant` and "fails on one device only" gets opened by the
person who needs it, at the moment they need it.

**The rule.** One paragraph, blockquoted, stating the invariant. If it cannot
be said in a sentence or two, it is probably two contracts.

> A client declares what it can play. The server decides what it gets. The
> decision is explicit in the response, and a client that cannot honour it
> fails loudly rather than guessing.

**The corollaries.** Three or four, each the specific mistake the rule
prevents, phrased as a prohibition: never pick a protocol from a user agent
string, never hardcode a key system. Prefer corollaries that have actually
been violated in this repo, and say so. A rule nobody has broken is a guess.

**The table.** Where the rule maps conditions to required behaviour, a table
beats paragraphs. Condition columns on the left, required outcome on the
right, and a "why" column so the next reader can tell a deliberate choice from
an accident.

**Why the naive version fails.** The shortest path a reasonable engineer would
take, and what breaks when they take it. This is the section that stops the
contract being re-litigated every six months.

## The two that come first

Most projects want a **master contract** and a **parity contract**.

The master holds the model every client implements, and the others specialise
it. Without one, the specialised contracts each restate the same assumptions
slightly differently.

Parity says what the product does and which clients do it. Back it with a data
file and a gate that fails the build when a client silently drops a feature.
Numerically perfect clients that are missing half the product is a real
failure mode, and nothing else catches it.

## Keeping them honest

**Read the contract before changing a client. Write the rule back into it
afterwards. Never just change the code.** That sentence belongs in the
`README.md` and it is the only process rule that matters. Everything else is
the contract text itself.

Where a contract can be checked by a machine, check it:

- A conformance suite both implementations must reproduce, one set of expected
  answers, run against each client.
- A gate on the parity data file.
- A test in the ported language that mirrors the shared implementation's test
  case for case, asserting the same published values.

A contract with a gate is a standard. A contract without one is a document,
and documents drift. Prefer the gate, and say in the contract which parts are
enforced and which are on trust.

## Index

`README.md` carries one table, in the order a newcomer should read them:

```markdown
| Contract | Governs |
|---|---|
| [master-contract](master-contract/SKILL.md) | **The master contract.** ... |
| [feature-parity](feature-parity/SKILL.md) | What the product does, and which clients do it |
```

Then a short "why these exist as prose" paragraph naming which client cannot
import the shared implementation, because that is the whole reason the folder
is there, and a clients table saying where each one lives.

## What not to do

- Do not write a contract for a decision only one client makes. That is a
  comment, or a function name.
- Do not restate the implementation. A contract says what must be true, not
  how the current code does it. If it has to change when you rename a
  function, it is documentation of the code rather than a rule for it.
- Do not let a contract accumulate history. It states the rule as it is now.
  Why it changed belongs in the commit.
- Do not create the folder before the second client exists. Write the rule
  down at the moment a port is about to disagree, not in advance of it.