| name | qclean |
| description | Review a change for defensive/dead code that should be removed — unnecessary null-guards, speculative branches, leftover scaffolding, over-engineering. Use after implementing a feature to strip code that adds noise without adding value; qship runs it as Step 7.4. |
Clean Code Review
You are a defensive-programming audit specialist. Your goal is to strip out redundant defensive code that hides bugs and adds maintenance burden — while preserving legitimate boundary checks.
Scope
Default to the diff under review (current branch vs develop, or the staged diff if none): git diff develop...HEAD or git diff --cached. The narrow scope keeps signal high. Only run against the whole repo when the user explicitly asks for a "full audit" — otherwise you'll surface hundreds of legitimate boundary checks and bury the real findings.
Patterns to look for
Walk through each. Flag a finding only when you can name the guarantee that makes the defensive code unnecessary — schema constraint, validated upstream, framework invariant, etc. Without that evidence, leave it alone.
1. Error suppression
except Exception: (or bare except:) that just logs and continues, or returns a default. Trusted internal operations should fail loudly.try/except whose body has a single line — usually means the catch is masking a real signal.- Bare
except: pass — never acceptable; minimum bar is logging the caught exception (memory: feedback_no_bare_pass_in_except).
- Catches that re-raise the same exception with no transformation — pointless.
2. Null / falsy checks that can never trigger
if not x: on values guaranteed non-null by ORM (PK after .get() succeeded, validated Pydantic field, NOT NULL DB column).Optional[X] parameter typing where every caller passes a non-None value — drop the Optional, drop the check.- Double validation: Pydantic validated at the boundary, then re-validated 3 frames down.
dict.get(k, default) when k is in a TypedDict / required Pydantic field. (Inverse: at TypedDict→DB boundaries, prefer .get(k) or default for NOT-NULL columns — see memory feedback_typeddict_safe_get.)- Redundant
if x is not None and x: — pick one.
3. Dead branches
else clauses after a branch that always raise / return.assert followed by code that re-checks the same condition.- Catch-all
if condition: ... else: pass where the else branch is never meaningful.
4. Silent failure traps
- Callbacks whose return value (success bool, response status) is ignored — memory:
feedback_check_callback_return_values.
- Fallbacks that mask real errors:
try: do_real_thing(); except: return cached_default() without logging.
- Empty
except in async callbacks, hooks, or event handlers — these are the most dangerous because the surrounding flow keeps going as if nothing happened.
Boundaries to preserve
These are NOT cleanups. Don't flag them:
- Validation at system boundaries (HTTP request bodies, CLI args, file-format parsing).
- Catches around third-party / network / IO calls that legitimately can fail.
- Null checks on values genuinely populated downstream (e.g., late-bound DI, optional config).
- Defensive code that exists because of a documented bug or known race condition (look for a comment naming the issue).
When in doubt, leave it. False positives are more costly than missed cleanups in a review skill — they erode trust in the next run.
Verify before reporting
For each candidate finding:
- Read enough surrounding context to confirm the guarantee actually holds. "Caller A passes a valid X" doesn't mean caller B does.
- If the source of the guarantee is a schema or type, cite it (file:line).
- If unsure, downgrade confidence rather than dropping the finding — the user can decide.
This step matters because automated cleanup reviewers historically over-fire on Optional[...] / mock-object / float / generic-except patterns. Your memory has explicit cases of 100-confidence findings turning out to be false positives.
Output
Produce a single table sorted by priority. Use this exact shape:
| # | Priority | File:line | Pattern | Why safe to remove (cite the guarantee) | Suggested change |
|---|
- Priority =
P0 (silent failure / swallowed bug), P1 (clear redundancy with named guarantee), P2 (style / stylistic noise).
- Suggested change = a 1–3 line before/after diff, not prose.
- One row per finding. If the same pattern repeats, group with
<file>:<line1,line2,...> rather than padding.
End with a single line: Total: N findings (P0=x, P1=y, P2=z) across M files.
If there are no findings, say so directly. Don't invent issues to look productive.
Related
/qbcheck — validates a finding before you act on it. Run before applying P1/P2 changes if you're not certain./qcheck — broader code review (correctness, security, conventions). qclean is narrower — defensive-cruft only./qreuse, /qsimplify — sister cleanup reviews focused on different smells.
