Code Review Taxonomy

Analyze each changed file for the categories below.

Non-goal: a deviation the brief notes as authorized by an active project hook (a skipped guardrail, a relaxed convention) is a settled, intentional choice. Do not raise it as a finding under any category.

1. Guardrail Violations

Per /speq-code-guardrails:

• [TOO_MANY_ARGUMENTS]: more than 3 arguments
• [SIDE_EFFECT]: function has side effects
• [BOOLEAN_FLAG_PARAMETER]: boolean flag parameter
• [MAGIC_NUMBER]: magic number without a named constant (standing in for a failure, it is [SENTINEL_ERROR_VALUE], not this tag)
• [MISSING_DOC_COMMENT]: missing doc comment on a public interface
• [INLINE_COMMENT]: inline comment present (TODOs and other work-tracking comments are [WORK_TRACKING_COMMENT], not this tag)
• [SELECTOR_ARGUMENT]: an argument (of any type, not just boolean) that picks which branch a function takes
• [OUTPUT_PARAMETER]: a value returned via a mutated argument instead of the return value
• [MIXED_ABSTRACTION_LEVEL]: a function mixes high-level orchestration with low-level detail
• [COMMAND_QUERY_MIX]: a single call both mutates something and hands back an answer
• [WEASEL_NAME]: a name that states no responsibility (Manager, Processor, Handler, Data, Info, Util)
• [IMPLEMENTATION_IN_NAME]: a name that bakes in a transport, vendor, or format instead of the abstraction

2. Dead Code

• [UNUSED_FUNCTION]: unused function or method
• [UNREACHABLE_CODE]: unreachable code path
• [UNUSED_IMPORT]: import not used
• [UNUSED_VARIABLE]: variable assigned but never read

3. Test Quality

Per /speq-code-guardrails' Tests section. Tests are quality subjects, not only removal candidates:

• [OBSOLETE_TEST]: tests removed functionality
• [DUPLICATE_TEST]: duplicate test coverage
• [ASSERTION_FREE_TEST]: test always passes, no assertions
• [VAGUE_TEST_NAME]: test name does not state the condition and expected behavior
• [NONDETERMINISTIC_TEST]: test depends on real clock, network, filesystem, or unseeded randomness
• [IMPLEMENTATION_COUPLED_TEST]: test asserts internal state instead of observable behavior
• [UNTESTED_ERROR_PATH]: a failure path with no test
• [MISSING_BOUNDARY_TEST]: no test for empty, single, maximum, off-by-one, or transition input
• [SKIPPED_TEST]: test is skipped or ignored rather than fixed or deleted
• [SUPPRESSED_WARNING]: a lint or compiler warning is silenced instead of resolved

4. Bad Comments

• [REDUNDANT_COMMENT]: describes "what" not "why"
• [OUTDATED_COMMENT]: does not match the code
• [COMMENTED_OUT_CODE]: commented-out code block
• [WORK_TRACKING_COMMENT]: TODO, FIXME, ticket refs

5. Optimization Opportunities

The Evidence Rule applies here: raise a finding in this category only with a measurement. Without one, the finding is [UNMEASURED_OPTIMIZATION] against the code that was optimized speculatively.

• [PERFORMANCE_ISSUE]: obvious performance issue
• [UNNECESSARY_ALLOCATION]: unnecessary allocation in a loop
• [DUPLICATE_OPERATION]: operation that repeats work already done
• [UNMEASURED_OPTIMIZATION]: a change justified as a performance optimization with no measurement behind it

6. YAGNI / Over-Engineering

Per /speq-code-guardrails's YAGNI Checks:

• [STANDARD_LIBRARY_DUPLICATE]: logic that reimplements something the language's standard library already provides
• [SHRINKABLE]: same logic expressible in meaningfully fewer lines
• [DEAD_FLEXIBILITY]: a feature flag, extension point, or parameter that is never varied
• [UNNEEDED_DEPENDENCY]: a dependency added for something the standard library or an already-installed dependency already covers
• [SPECULATIVE_ABSTRACTION]: an interface, generic type, or configuration value with exactly one implementation or caller, and not a seam over I/O, nondeterminism, or a third party

7. Error Handling

• [SENTINEL_ERROR_VALUE]: a magic value or in-band signal stands in for an error instead of the language's own error mechanism
• [CONTEXTLESS_ERROR]: an error that does not state what was attempted, the input that failed, or the constraint violated
• [SWALLOWED_ERROR]: an error is discarded instead of handled or propagated
• [BROAD_CATCH]: a catch broader than the specific error it handles
• [LEAKED_PROVIDER_ERROR]: a third-party error type crosses a module boundary unwrapped
• [ERROR_AS_CONTROL_FLOW]: an error mechanism used for expected, non-exceptional flow

8. Design Depth

Per /speq-design-philosophy:

• [SHALLOW_MODULE]: learning the interface takes almost as much effort as the implementation behind it would, or classitis (many small modules named for a role, not a responsibility; a purely naming defect with no structural symptom is [WEASEL_NAME], not this tag)
• [INFORMATION_LEAKAGE]: a single design choice (a format, a protocol, an execution-order split) shows up in more than one module and would need editing in both if it changed
• [TACTICAL_SHORTCUT]: a shortcut taken with no follow-up to invest in the design
• [MISSING_DESIGN_INTENT]: a public/interface comment states purpose but not the design intent or rationale a non-obvious abstraction needs
• [BOUNDARY_VIOLATION]: business logic names a delivery mechanism, storage engine, or framework directly
• [IO_IN_BUSINESS_LOGIC]: I/O performed directly inside business logic instead of through an injected abstraction
• [AMBIENT_STATE_READ]: environment or global state read in place instead of injected
• [LEAKED_BOUNDARY_TYPE]: a framework, storage, or third-party type crosses a module boundary
• [DEPENDENCY_CYCLE]: a cycle in the module dependency graph
• [SELF_CONSTRUCTED_DEPENDENCY]: a module constructs its own concrete dependency instead of receiving it
• [PROVIDER_SHAPED_ABSTRACTION]: an abstraction shaped around a provider's API instead of the consumer's own vocabulary
• [FEATURE_ENVY]: a function reaches into another module's data more than its own

Output Format

Write the findings document to specs/_plans/<plan-name>/review-findings.md per references/review-findings-template.md. Then return exactly one line and nothing else:

CODE REVIEW: <n> findings — standard: <n>, expert: <n> — specs/_plans/<plan-name>/review-findings.md

Never return the findings as response text. The implementer agents read them from the file.

Each finding's Fix: field follows the template's rules: an imperative addressed to the consuming implementer agent, never an optional suggestion.

Routing

You partition the findings. The orchestrator never sees them individually. Place each finding under ## Standard fixes or ## Expert fixes in the findings document. The partition decides which single agent applies the whole fix pass: any Expert finding routes both sections to implementer-expert-agent. With no Expert finding, implementer-agent applies ## Standard fixes.

Every tag across all 8 categories is eligible for either section. Route a finding to ## Expert fixes when its fix has cross-file, concurrency, or subtle-correctness implications: removing a dependency or abstraction with several call sites, correcting a dependency-direction or boundary violation, or any change whose failure mode is a passing test over wrong behavior. Everything else goes to ## Standard fixes. You hold the context for this call. Decide it here, do not defer it.