code-quality-review

Files (1)

• SKILL.md

SKILL.md (first lines)

# Full Code Quality Review

Review completed work for the best behavior-preserving implementation: correct, direct, cohesive, well-owned, easy to reason about, and free of accidental complexity.

This is not a style pass and not a contest to minimize lines. “Simple” means the fewest justified concepts, branches, states, layers, and invariants—not compressed or clever code.

Use this specialized skill when the user explicitly requests a deep structural/simplification review; that request makes behavior-preserving simplification the primary in-scope target. It may also run opportunistically during ordinary nontrivial work when a concrete useful quality, structure, simplicity, or ownership question warrants a read-only background lane. Opportunistic review is nonblocking, does not require a known defect, stops when no novel useful target remains, and cannot authorize fixes or delay readiness unless the parent validates and accepts a concrete finding into the primary scope.

Standalone deep quality reviews enter `review` for orchestration, finding partitions, and standards, including its initial fanout of at least three fresh parallel reviewers with distinct evidence targets. Manager-attached reviews stay in their existing `manager-workflow` stage. Opportunistic quality lanes remain nonblocking and do not create another review stage. This skill supplies quality methods and lenses, not orchestration. Route cleanup-only requests and every fix pass through `manager-workflow`.

## Default boundary

- Review-only by default. Do not edit project/source files or perform mutating VCS/filesystem operations.
- Treat `autofix`, “fix the findings,” “apply the cleanup,” or equivalent explicit wording as a request to enter fix mode for validated, in-scope findings. It does not bypass `manager-workflow` approval, stage, or scope requirements.
- Keep the requested behavior and approved product scope fixed. Question the implementation, not the product decision.
- Ask before any change to behavior, architecture, schema, config, security policy, data, compatibility, public API, or user workflow.
- Default scope is the total effective diff plus directly affected callers, callees, tests, contracts, and docs—not the whole repository.
- Review the whole repository only when explicitly requested.
- Ignore staging hygiene, dirty-tree state, untracked scratch artifacts, and pre-existing defects unless they affect the reviewed behavior or the user asked for repository hygiene.

## Core standard

A quality finding must be more than preference. It needs:

1. a concrete location or symbol;
2. the actual reachable code path or structural relationship;
3. a specific correctness, reasoning, ownership, testability, maintenance, or operational cost;
4. a smaller safe direction;
5. evidence that the direction preserves intended behavior, or an explicit statement of what remains unverified.

“No finding” is valid. Never manufacture issues to make the review look thorough.

## Workflow

### 1. Establish scope and intent

Before judging code:

- Read project instructions, the task/plan/spec, and relevant prior decisions.
- Inventory the full effective diff:
  - tracked changes against the correct base or `HEAD`;
  - staged and unstaged changes together;
  - in-scope untracked file bodies separately.
- State the intended behavior in one sentence.
- Identify the behavior owner: entrypoint, canonical module/layer, outputs, side effects, and proof surface.
- For each changed section or newly introduced concept, state what it does, why it exists, who owns it, and what would break if it were removed. If its purpose or necessity is not justified by the approved behavior, treat it as a deletion or simplification candidate.
- List changed files and directly affected unchanged code that must be traced.
- Distinguish intended feature work from cleanup/refactor work.

If intent or review range cannot be established, ask one focused question. When the answer is unavailable, return `INCONCLUSIVE` and name the missing fact. Do not guess a base, contract, or desired behavior.

### 2. Lock behavior before proposing simplification

Build a compact behavior-preservation contract from code, tests, docs, and live evidence where safe:

- inputs and accepted shapes;
- outputs and public contracts;
- reachable errors and failure semantics when the affected path or contract defines them;
- side effects and persistence;
- ordering, retries, concurrency, and atomicity;
- observability that callers/operators rely on;
- compatibility requirements proven to exist.

Trace each changed behavior end to end through the real path: entrypoint → branches → state changes → output/side effect. Inspect unchanged seams around the diff.

Do not add or preserve defensive logic for imagined values. Verify live/runtime shapes when tools can answer. A fallback, guard, shim, or compatibility path is justified only by a reachable case, explicit contract, released consumer, or observed failure.

When a producer, feature, state transition, or persisted contract was removed, trace every dependent consumer, flag, branch, test, comment, and compatibility path. Delete machinery whose source and reachable consumers no longer exist; do not preserve dormant paths for hypothetical restoration.

When a value comes from a known internal source and its shape is enforced by an inspected type, validator, or invariant owner, trust that contract downstream. Do not duplicate validation or add fallback branches for shapes the source cannot produce. Validate once at the real boundary.

### 3. Run the mandatory simpler-design pass

Keep behavior fixed and ask, in order:

1. Can code be deleted because the problem is already solved elsewhere?
2. Can an existing canonical helper, type, module, or framework path replace new machinery?
3. Can the same goal be solved at the natural owner layer instead of through cross-layer plumbing?
4. Can branches, modes, nullable flags, wrappers, adapters, or intermediate representations disappear?
5. Can state or data flow be modeled so invalid combinations are unrepresentable?
6. Can one direct flow replace duplicated or divergent paths?
7. Does an abstraction reduce what callers must know, or merely rename/move complexity?

Prefer structural simplification that deletes concepts over cosmetic extraction. Prefer direct, boring code over magic. The best solution is not automatically the smallest diff or fewest lines.

Do not recommend:

- an abstraction solely because two or three snippets look similar;
- a design pattern from textbook purity rather than local need;
- file splitting based only on a line threshold;
- broad rewrites without a concrete behavior-preserving migration;
- moving complexity between files while preserving the same cognitive load;
- generic configurability, compatibility, or extensibility without a current consumer.

When proposing a major simplification, show the current concepts that disappear and the resulting ownership/control flow. If that cannot be stated concretely, do not report it as a finding.

### 4. Review by distinct lenses

Assess the applicability of each lens before reviewing it so broad checklists do not blur evidence. Correctness, simplicity, and structure are presumptively applicable to code changes; investigate other lenses only when the affected path reaches them. Do not output empty checklist sections.

Core lenses:

1. **Correctness and behavior preservation**
   - invariants, reachable states/failures, relevant transitions or concurrency, and unintended behavior drift;
2. **Simplicity and accidental complexity**
   - unnecessary branches, modes, wrappers, layers, fallback paths, indirection, configuration, or generalization;
3. **Structure and ownership**
   - cohesion, dependency direction, canonical layer, source of truth, boundaries, naming, data/control flow;
4. **Types and contracts**
   - casts, `any`/`unknown`, loose object bags, avoidable optionality, duplicated representations, hidden invariants;
5. **Tests and proof quality**
   - behavior assertions, reachable boundary cases relevant to the change, over-mocked paths, and fresh diagnostics;
   - tests that merely copy hardcoded values, restate constants or configuration, or verify guarantees already owned by the type system, schema, validator, or framework. A useful test must prove observable behavior, a meaningful invariant or wiring relationship, an edge case, or a failure mode;
6. **Slop and clarity**
   - narration comments, speculative defensive checks, dead helpers, pass-through wrappers, stale rationale, noisy prose, debug artifacts, compressed cleverness.

Preserve still-valid comments that explain why a non-obvious choice or value exists. Update or relocate them with the owning code. Remove only narration, stale rationale, and comments made false by the change.

Add only applicable domain lenses: security/privacy, performance/hot paths, API compatibility, data/migrations, UI/accessibility, infrastructure/operations, or developer experience. For agent/runtime/config work, prioritize actual runtime behavior, reliability, evidence quality, context/tool boundaries, recovery, and completion over peripheral analytics or wrapper-only concerns.

From som1shi/pi-config, MIT.

More from som1shi/pi-config