Code Review Standards

Code Review Standards

Purpose

This skill defines the structured checklist that the code-critic agent applies during Stage 4 of the code production pipeline. The checklist is severity-tagged so that PM and engineer both know exactly which findings block delivery and which are advisory. Engineers may load this skill for self-review before requesting a critic pass.

The checklist exists because unstructured code review in multi-agent systems produces inconsistent signal: one critic dispatch flags naming; another flags security; neither flags the same things. Severity tagging makes the review deterministic across dispatches.

The Severity-Tagged Checklist

CRITICAL (must fix, blocks delivery)

• [ ] No secrets, API keys, or credentials hardcoded
• [ ] No SQL injection vectors (parameterized queries only)
• [ ] No arbitrary code execution paths (no eval, exec, unrestricted pickle.loads)
• [ ] Authentication/authorization not bypassable
• [ ] No infinite loops without escape conditions

HIGH (must fix, blocks delivery)

• [ ] Type hints on all public functions and classes
• [ ] mypy --strict passes with zero errors
• [ ] pytest passes with zero failures
• [ ] Test coverage >= 90% on new code
• [ ] No bare except clauses
• [ ] No mutable default arguments
• [ ] No global mutable state
• [ ] No synchronous I/O inside async functions
• [ ] No N+1 query patterns
• [ ] Error cases handled explicitly (not silently swallowed)

MEDIUM (flag, note in report, proceed)

• [ ] Functions <= 20 lines (prefer <= 10)
• [ ] No nested loops where hash map would reduce complexity
• [ ] list.pop(0) replaced with deque.popleft() where relevant
• [ ] asyncio.gather uses return_exceptions=True where appropriate
• [ ] Async operations have explicit timeouts
• [ ] Docstrings on public methods (Google or NumPy style)
• [ ] No Any types in production code paths

Efficiency (see criteria-efficiency.md):

• [ ] No nested loops over two collections that should be a hash-map lookup (O(n*m) → O(n+m))
• [ ] No per-iteration I/O (queries/RPCs/fetches inside a loop) — batch outside the loop (HIGH on hot paths; see "No N+1 query patterns")
• [ ] Repeated deep property/selector resolution cached in a local (no greedy data access)
• [ ] String accumulation in loops uses list+join / StringBuilder, not +=
• [ ] No SELECT * / over-fetching in production query paths

Transferability (see criteria-transferability.md):

• [ ] No dead/unreachable code (statements after unconditional return/break/raise; uncalled private members)
• [ ] Long if/else if chains (3+ branches on one key) replaced by switch/match or dispatch map
• [ ] No nested switch/match — extract inner switch to a named function
• [ ] No actively misleading names (name contradicts the value it holds)

LOW (note only)

• [ ] PEP 8 compliance (black + isort handles this automatically)
• [ ] Variable naming is clear and descriptive
• [ ] No commented-out code left in
• [ ] Import ordering is clean
• [ ] Naming consistency across the change (same concept, same name; consistent casing)
• [ ] No file managing too many responsibilities (see 800-line file limit)

Verdict Format

Critic output MUST begin with the verdict on the first line, followed by the finding table, followed by a summary paragraph.

First line format:

VERDICT: APPROVE

or

VERDICT: WARN

or

VERDICT: BLOCK

Finding table format:

Verdict definitions:

APPROVE means the implementation is ready for security review. MEDIUM and LOW findings in an APPROVE review are passed to the Documentation agent as notes — they do not block delivery but are preserved for future reference.

WARN means the implementation has structural issues that should be fixed but do not represent exploitable defects or correctness failures. PM proceeds to security review and appends the WARN finding table to the documentation handoff message. PM also logs the findings (KB entry or todo) so they are not silently dropped.

BLOCK means the implementation has at least one defect that, if shipped, creates a security vulnerability, data loss risk, or silent failure mode. PM halts the pipeline immediately, presents the critic finding table verbatim to the user, and awaits explicit direction. PM MUST NOT auto-retry the engineer without user input.

80% Confidence Filter

A clean review is a valid review. Do not manufacture findings.

Only report issues with >80% confidence they are real problems. Do not flag:

• Style preferences as HIGH or CRITICAL
• "This could theoretically be an issue in an edge case" without specific evidence
• Patterns that look unusual but may be intentional and correct
• Missing features that were not in scope (check the Stage 1 spec)

When confidence is below 80%, note the concern as a question in the summary paragraph rather than as a finding in the table. Example: "The process_batch() function did not appear to handle empty input — verify whether the caller guarantees non-empty batches."

This filter prevents the critic from becoming a noise generator that trains PM to ignore findings. Each finding in the table should be actionable: engineer reads it, knows exactly what to fix, and can do so without asking for clarification.

Navigation

For detailed criteria with examples:

• [CRITICAL Criteria](references/criteria-critical.md): Detailed explanations and examples for each CRITICAL item
• [HIGH Criteria](references/criteria-high.md): Detailed explanations and examples for each HIGH item
• [MEDIUM Criteria](references/criteria-medium.md): Detailed explanations and examples for each MEDIUM item
• [LOW Criteria](references/criteria-low.md): Detailed explanations and examples for each LOW item
• [Efficiency Criteria](references/criteria-efficiency.md): Algorithmic/data-access patterns (nested loops, fetch-in-loop, greedy access, loop concatenation, over-fetching)
• [Transferability Criteria](references/criteria-transferability.md): Maintainability patterns (dead code, long if/else-if chains, nested switch, naming hygiene)
• [Verdict Protocol](references/verdict-protocol.md): Full PM behavior for each verdict, failure loop templates

The Efficiency and Transferability criteria are derived from CAST Highlight code

quality indicators (https://doc.casthighlight.com/), paraphrased with original

examples.

CRITICAL Criteria — Detailed Explanations

Overview

CRITICAL findings block delivery. Any CRITICAL in the critic output produces a BLOCK verdict. The implementation must be returned to Stage 3 and the specific CRITICAL issues fixed before the pipeline can proceed.

CRITICAL criteria represent conditions where the shipped code would create an exploitable vulnerability, data breach risk, or unrecoverable system failure.

---

1. No hardcoded secrets, API keys, or credentials

What to look for:

• String literals matching patterns: sk-, ghp_, AWS, AKIA, password=, secret=, api_key=
• Credentials assigned directly in source: API_KEY = "abc123", DB_PASSWORD = "hunter2" <!-- pragma: allowlist secret -->
• Tokens committed in test fixtures or default config values

Why CRITICAL: Hardcoded secrets are extracted from source control or deployed artifacts. They cannot be rotated without a code change. Every developer who clones the repo has the credential.

Required fix: Move to environment variable. Access via os.environ["KEY_NAME"] or a secrets manager. Add the variable name (not value) to .env.example.

False positive filter: Placeholder values like "YOUR_API_KEY_HERE" or "<replace_me>" are not findings. Test fixtures that use obviously fake values ("test-secret-abc" alongside @pytest.mark) may be acceptable — note as a question if confidence is below 80%.

---

2. No SQL injection vectors

What to look for:

• String formatting in SQL queries: f"SELECT * FROM users WHERE id = {user_id}"
• .format() in SQL strings
• String concatenation building SQL: "SELECT * FROM " + table_name

Why CRITICAL: SQL injection allows attackers to read, modify, or delete arbitrary database data.

Required fix: Use parameterized queries: cursor.execute("SELECT * FROM users WHERE id = %s", (user_id,)) or ORM query builders that handle parameterization automatically.

---

3. No arbitrary code execution paths

What to look for:

• eval(user_input) or eval(f"...")
• exec(user_input) or exec(config_value)
• pickle.loads(data) where data comes from an untrusted source (network, user upload, DB)
• subprocess.run(user_input, shell=True) or os.system(user_controlled_string)

Why CRITICAL: These patterns allow remote code execution (RCE) — an attacker can run arbitrary commands on the server.

Required fix: Replace eval/exec with safe alternatives (AST parsing, allowed-list dispatch). Replace pickle with json or msgpack for untrusted data. Use subprocess.run with a list argument (not shell=True) and a validated allow-list of commands.

---

4. Authentication/authorization not bypassable

What to look for:

• Auth checks that only run on some code paths (missing decorator on one route)
• if user_id == admin_id: comparisons using user-supplied values without DB verification
• JWT validation that checks signature but not expiry, or expiry but not signature
• Role checks that can be bypassed by manipulating request parameters

Why CRITICAL: Auth bypass allows unprivileged users to access privileged data or operations.

Required fix: Ensure every protected endpoint has auth validation at the framework level (decorator, middleware) — not inline. Verify tokens fully: signature, expiry, issuer, audience.

---

5. No infinite loops without escape conditions

What to look for:

• while True: without a break condition reachable under all inputs
• Recursive functions without a guaranteed base case
• Retry loops without a maximum attempt count and backoff

Why CRITICAL: Infinite loops cause server hangs, CPU exhaustion, and denial-of-service conditions.

Required fix: Add explicit termination: maximum iteration count, timeout, or guaranteed base case. For retry loops: use exponential backoff with a maximum retry ceiling.

---

TODO: Expand with language-specific patterns

<!-- TODO: Add TypeScript/JavaScript patterns (prototype pollution, XSS via innerHTML) --> <!-- TODO: Add Go patterns (goroutine leaks, nil pointer dereference patterns) --> <!-- TODO: Add examples for each criterion with before/after code snippets --> <!-- TODO: Add guidance on ORM-level injection (e.g., SQLAlchemy raw() misuse) -->

EFFICIENCY Criteria — Detailed Explanations

Overview

EFFICIENCY findings cover algorithmic and data-access patterns that waste CPU, memory, or I/O at scale. Most are MEDIUM by default: they rarely break correctness, but they degrade performance as input size grows and they signal that a different data model or algorithm was warranted. Escalate to HIGH only when the pattern sits on a hot path with unbounded input (e.g., a request handler iterating an attacker-controllable collection).

These criteria add an Efficiency dimension to the checklist, complementing the existing Security and Robustness items.

Source note: The Efficiency families below are derived from CAST Highlight code

quality indicators (https://doc.casthighlight.com/). Patterns are paraphrased with

original examples; thresholds are presented as guidance, not CAST's proprietary

calibration.

---

1. Nested loops over two collections (O(n*m))

Severity: MEDIUM (HIGH on a hot path with large/unbounded inputs)

A loop nested directly inside another loop to correlate two collections produces O(n*m) behavior. For small fixed ranges this is fine; for data-driven collections it is usually a missing index/hash-map, and occasionally a sign the data model itself is wrong (the correlation should have been a join or a precomputed mapping).

Violation:

for user in users:
    for order in orders:               # O(n*m)
        if order.user_id == user.id:
            link(user, order)

Required fix — build a lookup once, then iterate once (O(n+m)):

orders_by_user: dict[int, list[Order]] = {}
for order in orders:
    orders_by_user.setdefault(order.user_id, []).append(order)

for user in users:
    for order in orders_by_user.get(user.id, []):
        link(user, order)

False-positive filter: Nested iteration over genuinely small fixed dimensions (a 3x3 grid, a fixed matrix) is not a violation. A nested loop where the inner range is bounded by a small constant is acceptable — flag only when both ranges scale with input. An inner loop guarded by an if/branch, or a loop calling a helper that itself loops, is weaker evidence — note as a question rather than a finding.

---

2. Query or fetch inside a loop (the N+1 pattern, generalized)

Severity: HIGH (this is the existing "N+1 query patterns" item, broadened)

Issuing a database query, RPC, or remote fetch once per iteration multiplies round trips. The HIGH checklist already flags ORM N+1; this entry generalizes it to any per-iteration I/O — a cursor advanced inside a loop, a requests.get per element, a cache read per row.

Violation:

for order in orders:
    customer = db.query(Customer).get(order.customer_id)  # 1 query per order
    enrich(order, customer)

Required fix — batch the access outside the loop:

ids = {o.customer_id for o in orders}
customers = {c.id: c for c in db.query(Customer).filter(Customer.id.in_(ids))}
for order in orders:
    enrich(order, customers[order.customer_id])

False-positive filter: A loop that legitimately must call out per item (e.g., a fan-out where each call targets a different host and batching is impossible) is not a violation — but it should then use bounded concurrency and timeouts (see MEDIUM async items). Flag the unbatched serial case; question the unavoidable fan-out case.

---

3. Greedy data access — repeated deep property/selector resolution

Severity: MEDIUM

Re-resolving a deep member chain or re-running a selector/query on every use forces the runtime to walk the resolution path each time. If a deeply nested value or a DOM/ORM selector result is read more than once in a scope, cache it in a local.

Violation:

if (config.services.auth.tokens.refresh.enabled) {
  rotate(config.services.auth.tokens.refresh.ttl);   // path walked twice
}

Required fix:

const refresh = config.services.auth.tokens.refresh;
if (refresh.enabled) {
  rotate(refresh.ttl);
}

The same applies to repeated document.querySelector(...), repeated ORM relationship access that triggers lazy loads, and repeated dictionary lookups of the same key.

False-positive filter: A property read once, or reads separated by a mutation that could change the value, are not violations. Only flag when the same path is read 2+ times in one scope with no intervening write.

---

4. String concatenation accumulated in a loop

Severity: MEDIUM

Because strings are immutable in Python, Java, JavaScript, and most managed runtimes, += inside a loop allocates a new string each iteration, yielding O(n²) work and churning the allocator.

Violation:

html = "<table>"
for last, first in employees:
    html += f"<tr><td>{last}, {first}</td></tr>"   # new string each pass
html += "</table>"

Required fix — accumulate in a list, join once:

parts = ["<table>"]
for last, first in employees:
    parts.append(f"<tr><td>{last}, {first}</td></tr>")
parts.append("</table>")
html = "".join(parts)

(Java: StringBuilder. JavaScript: push to an array and join.)

False-positive filter: A handful of concatenations outside a loop, or building a short fixed-size string, is not a violation. Flag only accumulation inside a loop whose trip count scales with input.

---

5. SELECT * and over-fetching

Severity: MEDIUM

Retrieving all columns when only a few are needed inflates network payload, defeats covering indexes, and couples the caller to column order. Name the columns required.

Violation: SELECT * FROM orders WHERE status = 'open' Required fix: SELECT id, customer_id, total FROM orders WHERE status = 'open'

False-positive filter: Ad-hoc diagnostics, small parameter/reference tables, and ORM models that genuinely use every column are acceptable. Flag SELECT * in production query paths that return wide rows or large result sets.

---

How Efficiency findings affect the verdict

Efficiency items are MEDIUM unless they sit on a hot path with unbounded input, in which case the N+1/fetch-in-loop case is HIGH (consistent with the existing HIGH "No N+1 query patterns" item). MEDIUM Efficiency findings are logged in the documentation handoff and do not block delivery. As with all criteria, apply the 80% confidence filter — a nested loop over two small fixed ranges is not a finding.

HIGH Criteria — Detailed Explanations

Overview

HIGH findings block delivery (WARN verdict). Zero CRITICAL and one or more HIGH = WARN. PM proceeds to Stage 5 but logs findings. If user directs fixes, engineer resolves HIGH findings and re-runs tests — no re-critic required for WARN resolutions.

HIGH criteria represent conditions that, if left unfixed, will cause correctness failures, type safety breakdowns, or silent errors that are difficult to debug in production.

---

1. Type hints on all public functions and classes

What to check: Every public function (not prefixed _) must have type annotations on:

• All parameters (including self — skip, it's implied)
• Return type

Violation example:

def process(data, timeout):  # no annotations
    return data

Required fix:

def process(data: list[dict], timeout: float) -> list[dict]:
    return data

---

2. mypy --strict passes with zero errors

Engineer must provide mypy --strict output in Stage 3 response. If output is absent or shows errors, this is a HIGH finding.

Check for: error: lines in mypy output, Found X errors in Y files.

---

3. pytest passes with zero failures

Engineer must provide pytest output. Any FAILED or ERROR lines are a HIGH finding. The Stage 3.5 gate should have caught this — if it reaches critic, it's a process error, but the critic still flags it.

---

4. Test coverage >= 90% on new code

Check --cov-report=term-missing output. If new files show coverage below 90%, flag as HIGH. Note the specific files and their actual coverage percentages.

---

5. No bare except clauses

Violation:

try:
    result = fetch()
except:  # catches KeyboardInterrupt, SystemExit, etc.
    pass

Required fix:

try:
    result = fetch()
except (TimeoutError, ConnectionError) as e:
    logger.error("fetch failed: %s", e)
    raise

---

6. No mutable default arguments

Violation:

def append_item(item: str, items: list[str] = []) -> list[str]:
    items.append(item)
    return items

The default [] is created once and shared across all calls. This is a classic Python footgun that causes state leakage between calls.

Required fix:

def append_item(item: str, items: list[str] | None = None) -> list[str]:
    if items is None:
        items = []
    items.append(item)
    return items

---

7. No global mutable state

Module-level mutable variables shared across function calls create implicit state dependencies that cause non-deterministic behavior and test pollution.

Violation:

_cache: dict[str, str] = {}  # module-level mutable dict

def get_cached(key: str) -> str | None:
    return _cache.get(key)

Required fix: Pass state through function arguments or use a class with explicit lifecycle management. If a module-level cache is required by design, flag as a question (not a finding) — it may be intentional.

---

8. No synchronous I/O inside async functions

Violation:

async def fetch_user(user_id: int) -> dict:
    response = requests.get(f"/users/{user_id}")  # blocks event loop
    return response.json()

Required fix:

async def fetch_user(user_id: int) -> dict:
    async with httpx.AsyncClient() as client:
        response = await client.get(f"/users/{user_id}")
    return response.json()

Also check: open() for file I/O in async context — use aiofiles. Database calls: psycopg2 in async context — use asyncpg or psycopg3 async.

---

9. No N+1 query patterns

Violation:

users = db.query(User).all()
for user in users:
    user.orders = db.query(Order).filter_by(user_id=user.id).all()  # N queries

Required fix: Use eager loading or a JOIN: db.query(User).options(joinedload(User.orders)).all()

---

10. Error cases handled explicitly (not silently swallowed)

Violation:

try:
    result = parse(data)
except Exception:
    result = None  # error swallowed — caller doesn't know what went wrong

Required fix: Either re-raise, log and re-raise, or convert to a domain exception:

try:
    result = parse(data)
except ParseError as e:
    logger.error("parse failed for data=%r: %s", data, e)
    raise ProcessingError("failed to parse input") from e

---

TODO: Expand with framework-specific patterns

<!-- TODO: Add Django ORM N+1 patterns (select_related, prefetch_related) --> <!-- TODO: Add FastAPI sync-in-async patterns (run_in_executor, BackgroundTasks) --> <!-- TODO: Add SQLAlchemy async session patterns --> <!-- TODO: Add coverage exception annotations (@pragma: no cover) guidance -->

LOW Criteria — Detailed Explanations

Overview

LOW findings are noted in the critic report but do not affect the verdict. An APPROVE with LOW findings is still APPROVE. LOW findings are passed to the Documentation agent for future reference. Engineers may address them at their discretion without a re-critic pass.

LOW criteria represent style and cleanliness issues that automated formatters handle, or issues so minor that the risk of flagging them as anything higher would create noise that dilutes the signal from genuine problems.

---

1. PEP 8 compliance (black + isort handles this automatically)

What this covers: Line length (88 chars for black), whitespace around operators, blank lines between functions and classes, trailing whitespace.

Why it's LOW: black and isort are typically run as pre-commit hooks or CI steps. If the implementation went through the CI pipeline, PEP 8 compliance is automated. Flagging it manually wastes finding table space.

When to flag: Only flag if PEP 8 issues are so severe they impair readability and the project clearly does not use an autoformatter. Even then, flag as LOW — the fix is running black . and isort ., not a code logic change.

---

2. Variable naming is clear and descriptive

What to check: Single-letter variable names outside of:

• Loop indices (i, j, k in short for loops are acceptable)
• Coordinates (x, y, z in geometric code are acceptable)
• Mathematical notation (n, m in algorithms following a standard formula)

Violations:

def process(d: dict, l: list[str]) -> None:  # d, l are unclear
    for i in l:
        d[i] = True

Low-impact: naming issues rarely cause bugs. Flag as LOW unless the name is actively misleading (e.g., result = get_user() then using result as a list of orders without reassignment — that's a MEDIUM readability issue at most).

---

3. No commented-out code left in

What to check: Blocks of code commented out with #:

# old_result = fetch_legacy(url)
result = fetch_new(url)

Why it's LOW: Commented-out code adds noise and creates confusion about whether the old code should be restored. Git history preserves deleted code — comments are unnecessary.

Exception: Comments that explain why code was removed ("# removed: was causing OOM on large inputs, see issue #123") are informative and acceptable.

---

4. Import ordering is clean

What to check: Standard library imports, third-party imports, and local imports should be in separate groups, in that order, each group alphabetically sorted.

isort handles this automatically. Flag as LOW if isort has clearly not been run.

Standard order:

# 1. Standard library
import asyncio
import os
from typing import Any

# 2. Third-party
import httpx
from pydantic import BaseModel

# 3. Local
from myapp.models import User
from myapp.utils import parse

---

When Not to Flag LOW Issues

If the implementation has CRITICAL or HIGH findings, do not fill the finding table with LOW issues — it buries the important signal. In a BLOCK verdict, limit the finding table to CRITICAL items only. In a WARN verdict, list HIGH items and optionally note LOW/MEDIUM in the summary paragraph rather than the table.

LOW issues consume finding table space that should be reserved for actionable findings. A critic report with 10 LOW findings and 0 HIGH findings looks comprehensive but is not useful — it suggests the critic spent time on noise rather than substance.

---

TODO: Expand with language-specific style guides

<!-- TODO: Add TypeScript/ESLint equivalents for each LOW criterion --> <!-- TODO: Add Go gofmt/goimports equivalents --> <!-- TODO: Add guidance on when to escalate naming issues to MEDIUM (actively misleading names) --> <!-- TODO: Add examples of acceptable commented-out code (with issue references) -->

MEDIUM Criteria — Detailed Explanations

Overview

MEDIUM findings do not block delivery. Critic includes them in the finding table, PM logs them in the documentation handoff, and the pipeline continues. If user requests fixes, engineer resolves them — no re-critic required.

MEDIUM criteria represent maintainability and performance issues that will cause problems at scale or make the codebase harder to maintain, but do not create immediate correctness failures.

---

1. Functions <= 20 lines (prefer <= 10)

What to check: Count lines from def to the last line of the function body. Exclude blank lines between logical sections. Flag functions over 20 lines.

Why this matters: Functions longer than 20 lines are difficult to test in isolation, harder to reason about, and usually doing more than one thing (violating Single Responsibility).

Required fix: Extract sub-operations into helper functions with descriptive names. If a function is long because it handles many error conditions, consider a result type or exception hierarchy that lets each error case be short.

False positive filter: State machine dispatch tables and match/case blocks with many short arms may legitimately exceed 20 lines — use judgment. Flag as a question if uncertain.

---

2. No nested loops where hash map would reduce complexity

Violation:

for user in users:
    for order in orders:
        if order.user_id == user.id:  # O(n*m)
            process(user, order)

Required fix:

orders_by_user: dict[int, list[Order]] = {}
for order in orders:
    orders_by_user.setdefault(order.user_id, []).append(order)

for user in users:
    for order in orders_by_user.get(user.id, []):  # O(n+m)
        process(user, order)

---

3. list.pop(0) replaced with deque.popleft() where relevant

list.pop(0) is O(n) because it shifts all remaining elements. If the code is implementing a queue pattern (FIFO), use collections.deque with popleft().

Only flag when: the list is used as a queue (items appended to one end, popped from the other). Random-access lists are fine with pop(0) if used rarely.

---

4. asyncio.gather uses return_exceptions=True where appropriate

When to flag: asyncio.gather(*tasks) without return_exceptions=True raises the first exception and cancels remaining tasks. If the caller wants to process partial results or collect all errors, it should use return_exceptions=True.

Only flag when: the gather call processes heterogeneous tasks where partial failure is a realistic scenario. Do not flag gather calls where all tasks are equivalent and failure of any = failure of all.

---

5. Async operations have explicit timeouts

What to check: await some_client.get(url) without a timeout parameter. asyncio.wait_for(coroutine, timeout=...) — if timeout is absent.

Required fix:

async with httpx.AsyncClient(timeout=30.0) as client:
    response = await client.get(url)

---

6. Docstrings on public methods (Google or NumPy style)

Public methods (not prefixed _) on classes should have docstrings explaining:

• What the method does (one line)
• Parameters (if non-obvious)
• Return value (if non-obvious)
• Exceptions raised (if relevant)

Only flag for: public API methods. Internal helpers and private methods are optional. Do not flag for trivial getters/setters where the name is self-documenting.

---

7. No Any types in production code paths

Any disables type checking for a variable and propagates to callers, undermining the value of type annotations. Flag Any in production code — test files may use Any for mock objects if needed.

Common false positive: cast(Any, ...) used deliberately at an integration boundary with an untyped library — note as a question if the usage looks intentional.

---

TODO: Expand with examples

<!-- TODO: Add complexity examples with Big O analysis --> <!-- TODO: Add deque vs list benchmark numbers for queue patterns --> <!-- TODO: Add Google docstring format template --> <!-- TODO: Add NumPy docstring format template --> <!-- TODO: Add guidance on when "no docstring" is acceptable (trivial property) -->

TRANSFERABILITY Criteria — Detailed Explanations

Overview

TRANSFERABILITY findings cover how easily a new engineer can read, understand, and take ownership of the code. These are maintainability concerns: dead code that misleads readers, inconsistent naming that slows comprehension, and control-flow shapes (long if/else if chains, nested switch) that obscure intent. Most are MEDIUM or LOW — they do not break correctness, but they raise the cost of every future change.

This adds a Transferability dimension to the checklist, complementing the existing Security, Robustness, and (new) Efficiency dimensions.

Source note: The Transferability families below are derived from CAST Highlight

code quality indicators (https://doc.casthighlight.com/), which themselves reference

open standards (SonarSource, CodeNarc, CWE-561 for dead code). Patterns are

paraphrased with original examples.

---

1. Dead code — unreachable statements

Severity: MEDIUM

Dead code is any statement that can never execute: code after an unconditional return/break/continue/raise in the same block, branches whose condition is a constant, or private methods with no callers. It misleads readers into thinking behavior exists that does not, and it rots silently because no test exercises it.

Violation:

def fee(amount: float) -> float:
    return amount * RATE
    log.debug("computed fee")   # unreachable — never runs

Required fix: Delete it. Git history preserves anything that might be restored; leaving "just in case" code in place is the anti-pattern.

False-positive filter: Code after a return inside a conditional branch is not dead. Platform-specific branches guarded by a runtime check are not dead. Flag only genuinely unreachable statements and provably uncalled private members. This overlaps with the LOW "no commented-out code" item — actual unreachable active code is MEDIUM (it compiles and misleads), commented-out code is LOW.

(Reference: CWE-561 Dead Code.)

---

2. Long if / else if chains that should be a switch or dispatch map

Severity: MEDIUM

Three or more else if branches selecting on the same variable are hard to scan and easy to extend incorrectly (a forgotten branch, a duplicated condition). Prefer a switch/match, or — better — a dispatch dictionary mapping keys to handlers.

Violation:

if kind == "circle":
    area = pi * r * r
elif kind == "square":
    area = s * s
elif kind == "triangle":      # 3rd branch — readability degrades
    area = 0.5 * b * h
else:
    raise ValueError(kind)

Required fix (dispatch map — most extensible):

AREA = {
    "circle":   lambda: pi * r * r,
    "square":   lambda: s * s,
    "triangle": lambda: 0.5 * b * h,
}
try:
    area = AREA[kind]()
except KeyError:
    raise ValueError(kind)

False-positive filter: Two branches do not warrant a switch. Chains where each branch tests a different variable (genuine sequential logic, not a single-key dispatch) are not violations. (Threshold guidance — three or more branches on one key — derived from CAST/CodeNarc conventions.)

---

3. Nested switch / match statements

Severity: MEDIUM

A switch inside a switch is hard to read: a reader can mistake an inner case for an outer one, and missing breaks become invisible. Extract the inner switch into its own well-named function.

Violation:

switch (state) {
  case "open":
    switch (event) {        // nested — extract this
      case "close": ...
      case "expire": ...
    }
  case "closed": ...
}

Required fix: Move the inner switch into handleOpen(event) and call it from the outer case. The outer switch now reads as a flat state table.

False-positive filter: A single inner switch that is trivially short (two cases) may be acceptable inline — use judgment, note as a question if uncertain.

---

4. Naming consistency and hygiene

Severity: MEDIUM for actively misleading names; LOW for merely terse names

Names are the primary documentation a future maintainer reads. Two concerns:

• Consistency: the same concept should have the same name everywhere

(user_id vs uid vs userIdentifier for one thing forces the reader to prove they are equal). Casing should follow the project convention (snake_case in Python, camelCase in JS/TS) uniformly.

• Descriptiveness: identifiers should be long enough to convey intent. Single-letter

names are acceptable only for loop indices (i, j), coordinates (x, y), and standard mathematical notation.

Violation (misleading — MEDIUM):

users = get_orders()        # name says users, value is orders

Violation (terse — LOW):

def p(d, l): ...            # unclear params

False-positive filter: Established domain abbreviations (url, db, id) and project-local conventions are fine. Naming rarely causes bugs — keep it LOW unless the name actively contradicts the value it holds. This overlaps with the existing LOW "variable naming is clear" item; escalate to MEDIUM only for actively misleading names.

---

5. Module/file managing too many responsibilities

Severity: LOW

A single file that imports and coordinates an unusually large number of other modules/files is a transferability smell: it concentrates knowledge and is hard to hand off. This pairs with the repo's 800-line file limit (plan modularization at 600).

What to check: A file at or over the size limit, or one whose import block spans dozens of unrelated modules. Suggest extracting cohesive sub-modules.

False-positive filter: Aggregator/__init__.py barrel files and dependency- injection composition roots legitimately reference many modules — not a violation.

---

How Transferability findings affect the verdict

Transferability items are MEDIUM or LOW; none block delivery on its own. They are logged to the documentation handoff. In a BLOCK or WARN verdict, keep these out of the finding table (per the LOW-criteria guidance about not burying high-signal findings) and mention them in the summary paragraph instead. Apply the 80% confidence filter — do not flag intentional, idiomatic patterns as transferability problems.

Verdict Protocol — Full PM Behavior

Overview

The verdict protocol defines exactly what PM does after receiving a critic verdict. PM MUST NOT use judgment to soften, override, or reinterpret the verdict. The protocol is deterministic: verdict in → PM action out.

---

APPROVE

Condition: Zero CRITICAL findings AND zero HIGH findings.

PM Actions:

1. Proceed to Stage 5 (Security) — dispatch security agent immediately 2. Pass any MEDIUM/LOW findings from the finding table to the Documentation agent handoff message as "notes for future reference — not blocking" 3. No further action required from engineer

Critic output may include MEDIUM/LOW findings in an APPROVE. Those findings are informational only and do not affect the pipeline continuation.

PM handoff template to Security agent:

Security review task — implementation cleared Stage 4 (Critic: APPROVE).

[paste Stage 3 source files verbatim]

Critic APPROVE finding table (MEDIUM/LOW only, FYI):
[paste table if non-empty, or omit if empty]

Apply OWASP Top 10 security review. Output SECURITY VERDICT as first line.

---

WARN

Condition: Zero CRITICAL findings AND one or more HIGH findings.

PM Actions:

1. Proceed to Stage 5 (Security) — do NOT halt the pipeline 2. Append the full critic finding table to the Documentation agent handoff message (not just a summary — the full table with file+line citations) 3. Log the findings in a persistent note:

• KB entry with topic "code-review-findings", or
• Todo tracker entry for the implementing engineer

4. PM MUST NOT silently discard HIGH findings — they must appear somewhere persistent

WARN does not require engineer action before Stage 5. If user later requests fixes for the HIGH findings, engineer resolves them without a re-critic pass.

PM session log entry (required for WARN):

WARN logged: critic found HIGH issues in [task name], [date].
High findings: [brief description of each HIGH item]
Tracked in: [KB entry ID or todo ID]

PM handoff template to Security agent:

Security review task — implementation cleared Stage 4 (Critic: WARN — HIGH findings logged).

[paste Stage 3 source files verbatim]

Critic WARN finding table (HIGH findings — logged for follow-up, not blocking):
[paste full finding table]

Apply OWASP Top 10 security review. Output SECURITY VERDICT as first line.

---

BLOCK

Condition: Any CRITICAL finding (one or more).

PM Actions:

1. Halt the pipeline immediately — do NOT dispatch security agent 2. Surface the critic finding table verbatim to the user 3. State exactly:

Critic has blocked this implementation. The following CRITICAL issue(s) must be
resolved before the pipeline can continue. Please confirm your direction:

[paste full finding table, CRITICAL items highlighted]

Options:
A) Fix and retry — return to Stage 3 with these findings as input
B) Override with justification — provide written justification; PM logs and proceeds
C) Abandon — discard this implementation

Awaiting your direction.

4. Await explicit user response — do NOT auto-retry

PM MUST NOT auto-delegate to engineer after a BLOCK without user input. This is the key safeguard: the user must consciously decide to retry, not have PM silently loop.

When user selects "Fix and retry":

Return to Stage 3. Dispatch the engineer with this additional input alongside the original Stage 1 spec and Stage 2 interface:

PIPELINE GATE FAILURE — Return to Stage 3

Critic verdict: BLOCK
CRITICAL findings requiring resolution:

[paste finding table, CRITICAL rows only]

Instructions:
- Fix ALL CRITICAL items above
- Re-run pytest (must pass green)
- Re-run mypy --strict (must pass)
- Return updated source files with fresh test output

Do NOT return until all CRITICAL findings are resolved and tests pass.
If fixing a CRITICAL item requires changing the Stage 2 interface, flag to PM
before proceeding — do not change the public API unilaterally.

When user selects "Override with justification":

PM logs the override in a KB entry:

Override logged: critic BLOCK bypassed for [task], [date].
Justification: [user's stated justification verbatim]
CRITICAL findings overridden: [list]

PM then proceeds to Stage 5 (Security) — a BLOCK override does not skip security.

---

Return-to-Stage-2 Condition

If the CRITICAL finding reveals a fundamental flaw in the Stage 2 interface itself (not just the implementation), PM returns to Stage 2, not Stage 3.

Example: Critic flags that the authentication interface has no mechanism to represent authentication failure (returns User | None but the type system allows None to propagate silently to callers). This is an interface design flaw — the Stage 2 interface needs to be amended before Stage 3 can be re-done correctly.

PM judgment call: interface design flaw vs. implementation bug. When uncertain, ask the user before proceeding.

---

TODO: Expand with case studies

<!-- TODO: Add real examples of BLOCK → fix → APPROVE cycles with before/after diffs --> <!-- TODO: Add guidance on distinguishing interface flaws from implementation bugs --> <!-- TODO: Add template for multi-CRITICAL BLOCK responses (prioritizing which to fix first) --> <!-- TODO: Add KB entry format for tracking override decisions -->