Plan a feature: scope first, then the engineering review

Review a plan, not code: do not build the feature. If the user has not given you the plan or feature description, ask for it and stop.

Start here. In the repository (cd into it first: the shell starts in the workspace root), run with execute to see what already exists:

git log --oneline -10
ls

Then read the files the plan touches. Work through Scope Challenge, then Sections 1-4 in order; for each finding give severity, a recommendation and the alternative. Ask about decisions that change scope in one message and stop until the user answers. Finish with the revised plan: what to build, in order, the files it touches, the tests that prove it, and what is NOT in scope.

My engineering preferences (use these to guide your recommendations):

• Shared code: require common behavior and improved reliability or net savings; similar-looking code alone is insufficient.
• Tests: every behavior tested; no test without a regression it would catch.
• Enough engineering: avoid fragility and premature abstraction/complexity.
• Edge cases: thorough handling over speed.
• Explicit over clever.
• Right-sized diff: smallest clear change; rewrite a broken foundation when necessary.
• Data model: storage follows the data's queries, integrity rules and update boundaries, not file count: fields queried, constrained or updated on their own get explicit columns or tables; JSON suits a document read and written whole.

A. Assess the target

Complete these checks before proposing scope changes. Do not write findings into the plan yet.

• What already solves each sub-problem? Inspect helpers, libraries, callers and reusable outputs: behavior and dependency/deployment boundaries. Cite authored sources; label proposed callers with their motivating plan requirement and assumptions.

• What minimum changes achieve the goal? Flag work deferrable without blocking it; challenge scope creep.

• Complexity check: Count the selected work, not files read only as evidence: for a plan, its proposed changed files and new classes/services; for a diff, changed files and classes/services introduced by that diff; for a file/directory, files in that selected scope and any explicitly proposed new classes/services. Count each once, label estimates, and seek fewer moving parts. At 8+ files or 2+ new classes or services, propose a smaller arrangement and ask before changing scope.

• Distribution check: For new artifacts, verify build/publish CI/CD, target OS/architectures and download/install channels. Put deferrals in "NOT in scope".

Review Sections (after scope is agreed)

Evaluate Architecture → Code Quality → Tests → Performance. Never condense, abbreviate or skip a section, including strategy/spec/infra plans.

1. Architecture review

Evaluate:

• System/component boundaries, dependencies and coupling.
• Data flow, bottlenecks, scaling and single points of failure.
• Security: auth, data access and API boundaries.
• Key flows needing ASCII diagrams in plans/code.
• One realistic production failure per new path/integration; does the plan handle it?
• Distribution architecture: New artifacts' build, publish and update paths; included or deferred CI/CD.

Two shapes before one (design-twice)

Run this only when the plan introduces a new module, service, package or boundary: a public surface other code calls; otherwise say so in one line.

Ask for the second shape. A plan with one design usually took the first shape that compiled in the author's head. Before judging it, require a second shape that is structurally different: another owner for the state, another split of responsibilities, another place for the boundary. A point fix inside the first shape is not a second candidate. When the plan has none, sketch one in five lines (types and signatures, bodies not implemented) and record it as the alternative. Compare the candidates on interface depth: how much behavior and policy each hides behind how small a public surface, and prefer the one that hides more behind less. A deep module is not a deep call chain: the chain scatters the understanding across layers, the module concentrates it behind one interface.

The eight red flags. Screen the chosen shape before Section 1 closes. Assume the next contributor is an agent that sees only the files it opened, copies the nearest example and takes the shortest path that compiles; prefer the design where a change that looks right from one file is right for the whole repo. A flag is a finding with a file or symbol and a reason to revise the shape, not a comment asking for care.

1. Shallow module. A large interface over little hidden complexity: several calls per operation, options that expose internal stages.
2. Information leakage. One decision (a representation, a policy, a wire or storage detail) lives in more than one module. Re-exported transport types are leakage; parse external data into domain types at the boundary.
3. Temporal decomposition. Modules split by execution order (load, validate, transform, save) instead of by the knowledge they own.
4. Pass-through method. A function that forwards the same arguments to another with the same shape; remove it or move the responsibility.
5. Split ownership. Two modules write the same state or each keep a copy; an agent editing one cannot see the other. One owner per piece of state.
6. Two ways to do one task. An agent copies whichever it meets first. Keep one; move the callers and delete the rest in the same change.
7. Importable internals. The shortest path that compiles imports them and they become interface. Make an outside import fail the build.
8. Hand-synced list. The same items in two places; an agent that sees one list edits one. Derive the second, or fail the build when they disagree.

Record under Architecture: both shapes, the pick, the depth argument for it, and each flag with its fix.

2. Code quality review

Evaluate:

• Organization and module structure.
• Shared-code opportunities in the target and related callers. No standalone history/PR sweep or quotas. Check proposed caller assumptions against existing interfaces.
• Explicitly flag error handling gaps and missing edge cases.
• Technical debt, fragility and needless complexity per engineering preferences.
• Accuracy of touched files' ASCII diagrams.

3. Test review

For shared-code changes, audit existing/missing shared-contract tests (behavior, errors, side effects, boundaries) and each migrated caller's integration/differences. Rejected extractions still need coverage for real duplicated-code defects.

For every new codepath, branch and error path in the plan, name the test that proves it (unit, integration or end-to-end) and the regression it would catch. Missing tests become plan items.

4. Performance review

Evaluate:

• N+1 queries and database access patterns.
• Memory usage.
• Caching opportunities.
• Slow or complex paths.

On per-request or looped paths, check queries in loops, unbounded queries or caches, missing indexes, whole-file loads and blocking calls without timeouts. Give each finding's scale (rows, requests/s, bytes) or mark it unknown; never invent benchmarks.