review-code-maintainability

Files (1)

• SKILL.md

SKILL.md (first lines)

# review-code-maintainability

Phase 2 code review. Judge whether the change is easy to evolve safely, easy for a human to navigate, and free of avoidable slop.

This is not a style pass. Look for real future-change pressure.

## Read first

- `../references/task-packet-contract.md`
- `../references/persistent-artifacts-contract.md`
- `../references/review-phases-contract.md`
- `../references/compatibility-policy.md`
- `../references/ownership-and-reuse-policy.md`
- `../references/final-state-authoring-policy.md`
- `../references/initiative-workflow-contract.md` when reviewing an initiative child packet
- `../../references/communication-mode.md`
- relevant tracked `AGENTS.md` chain
- applicable `AGENTS.override.md` only if local execution constraints matter
- current task packet: `CODE_REVIEW.md`, `AMENDMENTS.md`, `PLAN.md`, optional `PLAN_SATISFACTION_REVIEW.md` for unresolved `PS#` detail

## Core scope

Check:

- maintainability
- evolvability
- anti-slop
- human complexity
- ownership clarity
- change amplification
- hidden coupling
- speculative abstractions
- needless indirection
- navigation cost for the next human/agent
- final-state coherence in canonical code/docs/contracts

Allowed fallback:

- raise a correctness/regression finding if you encounter one while doing this pass

## Core workflow

1. Establish review scope.
   - Read the thin handoff from phase 1: `CODE_REVIEW.md` as the primary rollup source, `AMENDMENTS.md` as required companion, and `PLAN.md` for original intent / owner-boundary context.
   - Read `PLAN_SATISFACTION_REVIEW.md` only when an unresolved `PS#` needs more detail.
   - Identify the exact implementation commit under review, the exact diff from that commit, changed boundaries, and the modules that now own the behavior.
   - If the lead did not provide the commit hash and the workspace is dirty, stop and ask which commit is the review target instead of guessing from a floating workspace.
   - If the workspace has drift beyond the named commit, review the named commit and call out the drift explicitly so the lead can re-anchor the review.
2. Run a maintainability pass.
   - Look for duplication, hidden coupling, scattered sources of truth, and logic that will drift if changed in more than one place.
   - Call out design debt only when it creates a realistic future bug surface or makes the new behavior harder to evolve safely.
3. Run a human maintainability pass.
   - Judge how quickly a human can find the owner code, rebuild the mental model, and make a safe edit without holding too much hidden context in working memory.
   - Treat high navigation cost, scattered logic, and heavy cross-file recall as maintainability problems.
4. Run an architecture-drift pass.
   - Check whether the change weakens module boundaries, introduces a second source of truth, leaks internal invariants across APIs, or increases change amplification.
   - Prefer findings that explain future breakage pressure: `this works, but every future change now has to update three places`.
5. Run an ownership-and-reuse pass.
   - Flag parallel same-purpose entities unless the new boundary is explicit and materially non-overlapping.
6. Run a final-state-authoring pass.
   - Flag dual vocabularies, dual status models, or dual owner semantics that survive inside canonical code/docs/contracts.
   - Flag files that read like migration notes, patch notes, or append-only overlays when one current model should be obvious.
   - Flag implementations that keep old/new same-purpose paths alive when no real external boundary requires that residue.
   - Flag changes that should have reused the current owner after a small tidy-first refactor.
   - Flag designs where one invariant or behavior now has multiple owners.
   - Flag implementations that require coordinated edits across same-purpose modules to keep one behavior correct.
7. Run an anti-slop pass.
   - Flag generic abstractions without leverage.
   - Flag speculative extensibility that is not earning its complexity today.
   - Flag helper or adapter sprawl that makes ownership harder to see.
   - Flag AI-shaped indirection, duplicated glue, or ceremonial wrappers that do not reduce real risk.
   - Flag prose/config/code duplication pressure that nudges the system toward multiple sources of truth.
   - Flag internal-only backward-compatibility scaffolding as unnecessary complexity unless the human explicitly required it for a real external boundary.
8. Check tests as maintenance evidence when relevant.
   - If the new structure makes correctness harder to prove or leaves fragile behavior weakly tested, treat that as a maintainability finding.
9. Write or update `MAINTAINABILITY_REVIEW.md`.
   - Emit only concrete `M#` findings with real future-change pressure.
10. Update `CODE_REVIEW.md` with the fixed rollup fields this phase owns.
   - `Maintainability verdict:` `strong`, `acceptable`, or `fragile`
   - unresolved `M#`
   - rerun notes
   - latest rolled-up status if the phase-2 verdict or follow-up status changed
   - keep key dispositions already decided in `AMENDMENTS.md` current when new review dispositions were resolved after phase 1
11. If the review exposes durable semantic/navigation residue, append it to `ARTIFACT_CANDIDATES.md`.
12. Update `INDEX.md` if packet status changes.

## How to inspect changes

- Start from the named implementation commit.
- Prefer `git show --stat <commit>`, then targeted diff reads, then final-code reads.
- Use workspace diff only to detect drift beyond the named review commit, not to silently move the review target.

## Maintainability heuristics

Look for:

- one behavior split across many files without a strong reason
- readers and writers of the same state no longer agreeing on one model
- logic duplicated across provider, adapter, and UI layers
- local edits that now require synchronized changes elsewhere
- ownership that is obvious only if you already know the backstory
- tests that prove the happy path but not the risky maintenance path
- compatibility aliases, wrappers, or dual-support paths that exist only for internal callers
- a new helper/service/adapter/store/module whose purpose substantially overlaps an existing owner
- old and new same-purpose implementations kept alive together after the change

## Human maintainability rule

Good code lets a person quickly:

- find the owner logic
- understand the contract
- identify likely change points
- make a safe edit without holding half the system in mind
- know which single owner to modify for one behavior

If the patch raises time-to-confidence or navigation cost materially, that is review-worthy.

## Findings bar

From siarhei-belavus/agent-public, MIT.

More from siarhei-belavus/agent-public