164 lines
6.0 KiB
Markdown
164 lines
6.0 KiB
Markdown
---
|
|
name: essential-code-review
|
|
description: Use for non-trivial code implementation, refactoring, or review to find the smallest correct solution, reuse existing code, and remove AI-generated accidental complexity before completion.
|
|
license: AGPL-3.0-only
|
|
compatibility: opencode
|
|
---
|
|
|
|
# Essential code review
|
|
|
|
Identify recurring failure modes, preserve required behavior, and rewrite weak
|
|
drafts rather than decorating them. Code should be no more complex than the
|
|
problem requires.
|
|
|
|
## Task
|
|
|
|
For implementation or refactoring:
|
|
|
|
1. State the observable outcome in one sentence.
|
|
2. Find the existing owner and the closest working path before designing.
|
|
3. Separate intrinsic complexity from complexity introduced by the draft.
|
|
4. Implement the smallest complete change.
|
|
5. Verify behavior, then simplify the diff before finishing.
|
|
|
|
For review, report accidental complexity as a correctness and maintenance
|
|
problem. Do not praise breadth, novelty, abstraction, or line count.
|
|
|
|
## The essence pass
|
|
|
|
Answer these before editing. Keep the answers internal unless they affect a
|
|
decision or reveal a blocker.
|
|
|
|
- What must become true for the user?
|
|
- What must remain unchanged?
|
|
- Where does this concern already live?
|
|
- Which existing function, Score, capability, type, or test already does part
|
|
of it?
|
|
- How many real variants, providers, consumers, or trust boundaries exist now?
|
|
- What is the smallest acceptance test that proves the outcome?
|
|
|
|
If the problem cannot be stated without naming proposed helpers or types, the
|
|
solution is driving the problem. Restate it in user-visible terms.
|
|
|
|
## Complexity budget
|
|
|
|
Every new concept must pay for itself against current requirements.
|
|
|
|
- Add a branch for a real behavioral variant.
|
|
- Add a type when it prevents an invalid state or names a domain concept.
|
|
- Add a helper when it makes one cohesive operation clearer or has a second
|
|
real caller.
|
|
- Add a trait when multiple implementations or a required capability boundary
|
|
exist now.
|
|
- Add configuration only when a caller needs to choose.
|
|
- Add a crate only when dependency direction or independent reuse requires it.
|
|
|
|
One hypothetical future does not count. A long function is not automatically
|
|
wrong if it expresses one linear operation. Several short functions are not
|
|
automatically clean if they scatter one concern.
|
|
|
|
## Patterns to remove
|
|
|
|
Look for clusters, not isolated syntax.
|
|
|
|
### Existing feature blindness
|
|
|
|
The draft creates a new path instead of composing the existing one. Search by
|
|
behavior and domain vocabulary, not only by the proposed name. Prefer extending
|
|
the owning Score or module over a parallel helper, harness, or CLI path.
|
|
|
|
### Speculative layers
|
|
|
|
Typical signs:
|
|
|
|
- a trait with one implementation and no required boundary;
|
|
- an enum with one live variant;
|
|
- a builder for a small value constructed once;
|
|
- a wrapper that only forwards calls;
|
|
- a config option no caller sets;
|
|
- a registry, plugin seam, or generic parameter for an imagined consumer.
|
|
|
|
Delete the layer and use the concrete operation until reality supplies the
|
|
second case.
|
|
|
|
### Helper confetti
|
|
|
|
Tiny helpers can hide the operation's shape and force readers to jump between
|
|
files. Keep a sequence together when the steps share one purpose, state, and
|
|
error boundary. Split only an independently meaningful operation.
|
|
|
|
### Conflated ownership
|
|
|
|
The opposite failure is one function or type owning unrelated reasons to
|
|
change. Split by concern when the parts have different callers, lifecycles,
|
|
trust boundaries, or tests. Do not split merely to shorten a function.
|
|
|
|
### Duplicate knowledge
|
|
|
|
Two copies of the same condition, default, mapping, or deployment recipe in one
|
|
trust domain are a defect. Choose one authoritative home and compose it.
|
|
Validation at separate trust boundaries is not duplication.
|
|
|
|
### Framework-shaped application code
|
|
|
|
Do not move code into the framework because it looks reusable. Require a real
|
|
second consumer or an established domain capability. Keep product-specific
|
|
policy in its deploy crate.
|
|
|
|
### Flashy PR scope
|
|
|
|
Reject opportunistic redesign, broad renames, future variants, unrelated
|
|
cleanup, and large documentation claims around a small behavior change. A PR
|
|
should be easy to describe in terms of the problem it closes.
|
|
|
|
### Tests for the implementation rather than behavior
|
|
|
|
Do not add a helper solely so it can have unit tests. Test the observable
|
|
contract at the lowest reliable boundary. Avoid duplicating production logic in
|
|
fixtures.
|
|
|
|
### Narrative comments and names
|
|
|
|
Comments explain a constraint or reason. Names identify domain meaning. Remove
|
|
comments that narrate statements and names that advertise patterns rather than
|
|
the problem.
|
|
|
|
## False positives
|
|
|
|
Preserve complexity when it belongs to the problem:
|
|
|
|
- protocol state machines with real failure transitions;
|
|
- retries, rollback, and idempotency required by external systems;
|
|
- duplicate validation across trusted and untrusted boundaries;
|
|
- typed wrappers that enforce a meaningful invariant;
|
|
- provider differences already exercised by supported deployments;
|
|
- deployment sequencing imposed by generated credentials or readiness.
|
|
|
|
Do not flatten these into code that is shorter but wrong. Instead, keep the
|
|
complexity in one cohesive owner and make the invariant visible.
|
|
|
|
## Simplification pass
|
|
|
|
After tests pass, inspect only the diff and ask:
|
|
|
|
1. Can existing code replace anything added here?
|
|
2. Can any new type, trait, option, helper, or module disappear?
|
|
3. Is the same condition or knowledge represented twice?
|
|
4. Did application policy leak into framework code?
|
|
5. Did test setup reimplement production behavior?
|
|
6. Can control flow become linear and local?
|
|
7. Does every changed file need to be in this PR?
|
|
|
|
Rewrite when the answer exposes accidental complexity. Run verification again.
|
|
|
|
## Review output
|
|
|
|
Lead with concrete findings and file references. For each issue, state:
|
|
|
|
- the user-visible or maintenance risk;
|
|
- the accidental complexity pattern;
|
|
- the smaller correction using existing code where possible.
|
|
|
|
If the implementation already matches the problem, say so. Do not invent a
|
|
simplification to justify the review.
|