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.
1---2name: speq-code-review3description: Code review tag taxonomy and findings output format — guardrail violations, dead code, test quality, bad comments, optimizations, YAGNI/over-engineering, error handling, and design depth. Triggered by code-reviewer.4---56# Code Review Taxonomy78Analyze each changed file for the categories below.910**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.1112## 1. Guardrail Violations1314Per `/speq-code-guardrails`:15- `[TOO_MANY_ARGUMENTS]`: more than 3 arguments16- `[SIDE_EFFECT]`: function has side effects17- `[BOOLEAN_FLAG_PARAMETER]`: boolean flag parameter18- `[MAGIC_NUMBER]`: magic number without a named constant (standing in for a failure, it is `[SENTINEL_ERROR_VALUE]`, not this tag)19- `[MISSING_DOC_COMMENT]`: missing doc comment on a public interface20- `[INLINE_COMMENT]`: inline comment present (TODOs and other work-tracking comments are `[WORK_TRACKING_COMMENT]`, not this tag)21- `[SELECTOR_ARGUMENT]`: an argument (of any type, not just boolean) that picks which branch a function takes22- `[OUTPUT_PARAMETER]`: a value returned via a mutated argument instead of the return value23- `[MIXED_ABSTRACTION_LEVEL]`: a function mixes high-level orchestration with low-level detail24- `[COMMAND_QUERY_MIX]`: a single call both mutates something and hands back an answer25- `[WEASEL_NAME]`: a name that states no responsibility (Manager, Processor, Handler, Data, Info, Util)26- `[IMPLEMENTATION_IN_NAME]`: a name that bakes in a transport, vendor, or format instead of the abstraction2728## 2. Dead Code2930- `[UNUSED_FUNCTION]`: unused function or method31- `[UNREACHABLE_CODE]`: unreachable code path32- `[UNUSED_IMPORT]`: import not used33- `[UNUSED_VARIABLE]`: variable assigned but never read3435## 3. Test Quality3637Per `/speq-code-guardrails`' Tests section. Tests are quality subjects, not only removal candidates:38- `[OBSOLETE_TEST]`: tests removed functionality39- `[DUPLICATE_TEST]`: duplicate test coverage40- `[ASSERTION_FREE_TEST]`: test always passes, no assertions41- `[VAGUE_TEST_NAME]`: test name does not state the condition and expected behavior42- `[NONDETERMINISTIC_TEST]`: test depends on real clock, network, filesystem, or unseeded randomness43- `[IMPLEMENTATION_COUPLED_TEST]`: test asserts internal state instead of observable behavior44- `[UNTESTED_ERROR_PATH]`: a failure path with no test45- `[MISSING_BOUNDARY_TEST]`: no test for empty, single, maximum, off-by-one, or transition input46- `[SKIPPED_TEST]`: test is skipped or ignored rather than fixed or deleted47- `[SUPPRESSED_WARNING]`: a lint or compiler warning is silenced instead of resolved4849## 4. Bad Comments5051- `[REDUNDANT_COMMENT]`: describes "what" not "why"52- `[OUTDATED_COMMENT]`: does not match the code53- `[COMMENTED_OUT_CODE]`: commented-out code block54- `[WORK_TRACKING_COMMENT]`: TODO, FIXME, ticket refs5556## 5. Optimization Opportunities5758The 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.5960- `[PERFORMANCE_ISSUE]`: obvious performance issue61- `[UNNECESSARY_ALLOCATION]`: unnecessary allocation in a loop62- `[DUPLICATE_OPERATION]`: operation that repeats work already done63- `[UNMEASURED_OPTIMIZATION]`: a change justified as a performance optimization with no measurement behind it6465## 6. YAGNI / Over-Engineering6667Per `/speq-code-guardrails`'s YAGNI Checks:68- `[STANDARD_LIBRARY_DUPLICATE]`: logic that reimplements something the language's standard library already provides69- `[SHRINKABLE]`: same logic expressible in meaningfully fewer lines70- `[DEAD_FLEXIBILITY]`: a feature flag, extension point, or parameter that is never varied71- `[UNNEEDED_DEPENDENCY]`: a dependency added for something the standard library or an already-installed dependency already covers72- `[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 party7374## 7. Error Handling7576- `[SENTINEL_ERROR_VALUE]`: a magic value or in-band signal stands in for an error instead of the language's own error mechanism77- `[CONTEXTLESS_ERROR]`: an error that does not state what was attempted, the input that failed, or the constraint violated78- `[SWALLOWED_ERROR]`: an error is discarded instead of handled or propagated79- `[BROAD_CATCH]`: a catch broader than the specific error it handles80- `[LEAKED_PROVIDER_ERROR]`: a third-party error type crosses a module boundary unwrapped81- `[ERROR_AS_CONTROL_FLOW]`: an error mechanism used for expected, non-exceptional flow8283## 8. Design Depth8485Per `/speq-design-philosophy`:86- `[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)87- `[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 changed88- `[TACTICAL_SHORTCUT]`: a shortcut taken with no follow-up to invest in the design89- `[MISSING_DESIGN_INTENT]`: a public/interface comment states purpose but not the design intent or rationale a non-obvious abstraction needs90- `[BOUNDARY_VIOLATION]`: business logic names a delivery mechanism, storage engine, or framework directly91- `[IO_IN_BUSINESS_LOGIC]`: I/O performed directly inside business logic instead of through an injected abstraction92- `[AMBIENT_STATE_READ]`: environment or global state read in place instead of injected93- `[LEAKED_BOUNDARY_TYPE]`: a framework, storage, or third-party type crosses a module boundary94- `[DEPENDENCY_CYCLE]`: a cycle in the module dependency graph95- `[SELF_CONSTRUCTED_DEPENDENCY]`: a module constructs its own concrete dependency instead of receiving it96- `[PROVIDER_SHAPED_ABSTRACTION]`: an abstraction shaped around a provider's API instead of the consumer's own vocabulary97- `[FEATURE_ENVY]`: a function reaches into another module's data more than its own9899## Output Format100101Write 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:102103```104CODE REVIEW: <n> findings — standard: <n>, expert: <n> — specs/_plans/<plan-name>/review-findings.md105```106107Never return the findings as response text. The implementer agents read them from the file.108109Each finding's `Fix:` field follows the template's rules: an imperative addressed to the consuming implementer agent, never an optional suggestion.110111## Routing112113You 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`.114115Every 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.
marconae