| name | deburr-edge-cases |
|---|---|
| description | Strip AI-codegen cruft from a branch — defensive checks the type system should make unrepresentable, redundant validation, speculative abstraction, no-op wrappers — while preserving load-bearing seams. Use after a feature lands and before merge, when asked to "simplify", "de-ornament", "remove cruft", "make erroneous states unrepresentable", or "clean up" a diff. |
A structured pass to remove accumulated cruft from a branch — particularly the failure mode where code solves, at a narrow local layer, a problem that should be addressed globally or structurally. The canonical example: instead of enforcing design intent in the type system (making erroneous states unrepresentable), the code accumulates defensive runtime checks to detect and flag bad states that a better type would forbid at compile time.
This is a quality pass, not a bug hunt. Every change must be behavior-preserving, with the existing test suite as the safety net. It complements a correctness-focused review (which hunts bugs); route bug findings there, not here. If you trip over a genuine correctness bug mid-pass, note and surface it separately — do not fix it as part of this pass.
A recognizable shape — an unwrap_or
, a single-impl
trait — is a prompt to ask whether the construct earns its keep here, not a finding in itself. Reason about intent and invariants, not form.
- After a feature branch lands, before merge — especially work assembled by several contributors or agents, which tends to accrete per-phase scaffolding that reads as cruft once the whole is in view.
- When the user asks to "simplify", "de-ornament", "remove cruft / ceremony / boilerplate", "make bad states unrepresentable", "tighten the types", or "clean up" a set of changes.
Defensive coding the type system should forbid. Runtime checks, branches, or asserts guarding states a tighter type would make unrepresentable — for example a tri-stateOption<bool>
, a sentinel (""
/-1
/0
) standing in for anOption
/enum, a field validated on every read rather than made unconstructable-when-invalid, or a "this can't happen" guard.Redundant / over-eager validation. Re-checking an invariant already guaranteed upstream (e.g. re-validating what a parse step already proved).Speculative generality / over-abstraction. A trait, generic, indirection layer, config knob, or helper with a single call site and no second implementor in view. No-op wrapper methods that just forward to another method with no distinct behavior.Ornamental error handling. Error/diagnostic variants for conditions that cannot occur, or a fan-out of variants where one would do.Dead-but-dressed-up scaffolding that isnota deliberate, documented forward seam.
Do not strip things that earn their keep. Call them out as intentional rather than flagging them:
Genuine architectural seams: a trait that is the documented extension point for future implementors (even with one impl today), mirroring an established pattern in the codebase.The sole enforcement point: input validation that is theonlyguard (e.g. validating tool-call arguments when the schema is advisory and never enforced at the boundary).Concurrency primitives matching the actual ownership (anAtomicBool
guard through a shared&self
can't be replaced by take-by-valueOption
if the caller holds`&self`
).**Test seams**: a generalization (e.g. over`AsyncWrite`
) that has a real second caller in tests.Required safety rails: a bound or check mandated by a spec/requirement is not "added cruft" even if the current structure makes it hard to hit — keep it.- A DI/context struct that is over-built today but has a confirmed near-term second consumer.
When unsure whether something is cruft or load-bearing: it is load-bearing until proven otherwise. Default to keeping.
Finding no ornamentation is a valid and common result — especially on small or already-tight diffs. Report "clean" rather than inventing a finding to justify the pass.
Run the steps directly, or route the fan-out and edits through whatever review/delegation process the repo otherwise follows. Keep edits one-at-a-time and behavior-preserving, and review each before moving on. Match effort to the diff: for a few files, scan and fix directly; the clustering and per-cluster fan-out below are for large, multi-author branches.
Compute the cumulative branch diff against the merge base and list the changed source files, excluding generated/state files (lockfiles, task-tracker state) unless relevant. This is the review surface.
Partition the changed files into coherent clusters (e.g. parse/types, registry/storage, config/state-modeling, wiring/transport). Take one cluster at a time — or, if the repo supports parallel read-only reviewers, one reviewer per cluster. For each, apply:
- the precise lens above (the five ornamentation categories + the preserve list),
- the file cluster and the specific suspects to scrutinize,
verify suspicious editor diagnostics against before reporting,
cargo check --all-targets
- a consistent finding shape: one-line title; confidence (high/med/low that it's a real
simplification, not a behavior change);
file:location
; the specific cruft; a crisp remedy (prefer "replace runtime check X with type Y"); behavior-preserving vs. behavior-changing. Separate clear wins from judgment calls, and name anything judged intentional / load-bearing so it is not re-litigated. The review stage reports findings only — it does not edit.
Merge the findings. Drop:
- anything behavior-changing that isn't clearly an improvement,
- anything that removes a spec-required rail,
- low-value cosmetic churn.
Keep the type-system wins and genuine no-op removals. The headline finding is usually a single
structural remodel (e.g. tri-state
Option<bool>
→Option<Resolved>
so the bad state is unrepresentable); the rest are small isolated cleanups.
Take one finding (or one tightly-coupled pair) at a time. Pin the exact contract before editing — especially for a structural refactor, write the target type design and the invariant truth-table the change must preserve, plus a test-migration map (migrate tests to the new shape; never weaken assertions to pass). Execute in increasing invasiveness: small, isolated, behavior-preserving cleanups first; the big structural remodel last, on an already-cleaned base. After each change, verify cargo and review it before moving on. If no existing test exercises the changed path, the suite is not a safety net for it — add a characterization test first, or lower the finding's confidence and flag the gap.
Before deleting a "no-op", confirm it is one. A classic trap: a let _ = (&field, …)
discard or
an #[allow]
that looks ornamental but actually suppresses a real dead_code
/lint warning
(e.g. `#[derive(Deserialize)]`
does not count as a read, and `#[derive(Debug)]`
is ignored by dead-code analysis — so fields only read via derives genuinely warn). If removing the "cruft" produces a warning or breaks a check, restore it and report rather than deleting into a broken build. Don't trade ornament for a warning.
Run the full check suite on the whole de-ornamented stack: cargo fmt --check
,
cargo clippy --all-targets --all-features
, cargo nextest run
, ratchets check
. Confirm no assertion was weakened or deleted to make a change pass — tests that only covered genuinely-removed code may go with it, but everything else must be migrated, not loosened.
Tri-state collapse → unrepresentable bad state.enabled: Option<bool>
rewrittenNone → Some(true)
at assembly and read viaunwrap_or(false)
, then carrieddeadinto a component that only exists when enabled. Fix: split the parse DTO from a resolved type; make the consumer fieldOption<ResolvedConfig>
withno, so "disabled" =enabled
fieldNone
by construction. Deletes the rewrite, the accessor, theunwrap_or
, the dead field, and the explanatory comments that existed only to compensate for the confusing type.Sentinel → A helper returning an ownedOption
.String
with""
meaning "absent", forcing a downstream!is_empty() && …
guard. Fix: returnOption<&str>
; the guard and an allocation vanish.Namespacing struct → free function. A zero-fieldstruct FooThing;
whose only purpose is to host one associatedfn
that is never called on an instance and implements no trait. Fix: make it a module-level freefn
, delete the struct/impl. (Distinguish from therealseam — the trait + concrete type it delegates to — which stays.)No-op wrapper method. A trait defaultsummarize()
that just returns`self.list()`
, with no override and one caller. Fix: delete it; the caller uses`list()`
. Reintroduce only if a second, genuinely-distinct behavior ever materializes.
- Deleting something into a compiler/clippy warning (verify first; restore if it warns).
- Removing a spec-mandated safety bound because the current structure makes it hard to hit.
- Stripping a documented forward seam or the sole enforcement point of an invariant.
- Weakening or deleting test assertions to make a refactor pass — migrate them instead.
- Doing the big structural remodel first, or batching everything into one giant unreviewable commit.
- Treating the pass as a bug hunt — it is quality-only; route correctness findings to a dedicated bug-hunting review.