|
| 1 | +--- |
| 2 | +name: code-hygiene |
| 3 | +description: "Reviews code changes for architectural smells that mechanical checks (lint, tests, coverage) structurally can't catch: duplicated 'shared' abstractions, missing sentinel errors, generics that discard their own type info, self-aware nolint suppressions of mandated rules, business logic in the wrong layer, admitted-but-unshipped gaps, config/flags that validate but silently no-op or always error, features that are implemented but never wired up, and documentation/code mismatches in either direction (docs describing an unimplemented or since-changed feature, or a shipped feature with no docs at all). Distinct from the general code-review skill (bugs/security/perf) and from lint (mechanical/syntactic) — this catches CLAUDE.md architectural-mandate violations that need reading intent, not just syntax. Patch-scoped against origin/main by default; a full-repo sweep runs only on an explicit human request. Invoke on explicit requests like \"check for vibe coding\" / \"run code hygiene\" / \"audit this PR's architecture\", or automatically as a step in the fix-all cycle." |
| 4 | +metadata: |
| 5 | + copyright: Copyright Cloud Posse, LLC 2026 |
| 6 | + version: "1.0.0" |
| 7 | +--- |
| 8 | + |
| 9 | +# Code Hygiene |
| 10 | + |
| 11 | +A narrow, calibrated review pass — not a general bug hunt. `lint` catches syntactic violations, |
| 12 | +`test-coverage` catches untested lines, and a normal correctness-focused review catches bugs — none |
| 13 | +of those structurally catch an abstraction that got duplicated instead of reused, a feature that |
| 14 | +validates but silently does nothing, or a comment that admits a gap nobody closed. This skill's job |
| 15 | +is specifically the class of smell that only shows up when you read code against this repo's own |
| 16 | +architectural mandates (CLAUDE.md) and ask "does this actually do what it claims to," not just |
| 17 | +"does this compile and pass its own tests." |
| 18 | + |
| 19 | +**This skill reports; it does not redesign.** A real architectural smell usually needs a design |
| 20 | +decision, not a mechanical patch — report it clearly enough that a human (or a follow-up planning |
| 21 | +pass) can make that call. See "Auto-fix policy" below for the one narrow exception. |
| 22 | + |
| 23 | +## Scope: patch-aware by default |
| 24 | + |
| 25 | +Default to the same scoping convention as `lint`/`test-coverage`: review only the files/packages |
| 26 | +touched by `git diff origin/main...HEAD --name-only` (or the currently open PR's diff). This never |
| 27 | +goes hunting through the other 340+ packages the current patch didn't touch. Unlike `lint` (which |
| 28 | +wraps a real `atmos fix lint` command with an actual `--new-from-rev` flag), this skill has no |
| 29 | +underlying CLI command — there's no literal flag to pass, only the phrasing below. |
| 30 | + |
| 31 | +## Full-repo mode (explicit only) |
| 32 | + |
| 33 | +Only when a human explicitly asks for a full sweep (e.g. "audit the whole repo", "full code |
| 34 | +hygiene sweep" — never inferred, never run from `fix-all`'s automated cycle). A whole-repo pass is |
| 35 | +expensive and belongs in an on-demand invocation, not an hourly loop. |
| 36 | + |
| 37 | +## Dedup: skip a re-run against an unchanged diff |
| 38 | + |
| 39 | +Before reviewing, hash `git diff origin/main...HEAD` (the diff content itself — this skill's |
| 40 | +dedup key is "has the patch changed," not an external event set) and compare against |
| 41 | +`.claude/state/code-hygiene/<branch-slug>.json`'s last-recorded hash. The `.claude/state/<name>/ |
| 42 | +<branch-slug>.json` file shape is borrowed from `.claude/hooks/security-remediate-trigger.sh` — the |
| 43 | +one existing precedent for this pattern in this repo, not an established multi-skill convention — |
| 44 | +adapted here to hash the diff instead of a set of alert IDs, since what's being deduped against is |
| 45 | +different (a review cycle re-running on unchanged code, not a repeated external notification). If |
| 46 | +the hash matches, skip re-reviewing and reuse the cached `findings` array |
| 47 | +from that state file — report it as a one-line no-op citing the cached result count, don't burn |
| 48 | +tokens re-deriving the same answer from an unchanged patch. If the hash differs (or no state file |
| 49 | +exists yet), do the full review below, then write `{hash, findings, ts}` back to the state file |
| 50 | +after reporting. |
| 51 | + |
| 52 | +## The checklist |
| 53 | + |
| 54 | +For each touched file/package, check for: |
| 55 | + |
| 56 | +1. **Duplicated "shared" abstraction.** Near-identical logic (same loop/branch structure, same |
| 57 | + algorithm) appears two or more times inside a package whose entire purpose is to be *the* |
| 58 | + shared implementation — e.g. two separate tree-walk-and-hash functions in a package that only |
| 59 | + needs one, parameterized by what varies between the call sites. |
| 60 | + |
| 61 | +2. **Missing sentinel errors.** A new or heavily-touched package returns errors but has zero |
| 62 | + `errors.New`/sentinel `var` declarations backing them — every error site is an ad-hoc |
| 63 | + `fmt.Errorf("... %w", err)` string literal instead of a wrapped static sentinel, violating |
| 64 | + CLAUDE.md's "All errors MUST be wrapped using static errors" mandate. |
| 65 | + |
| 66 | +3. **A generic that discards its own type.** `func f[T A | B](...)` whose body immediately does a |
| 67 | + runtime type-switch/type-assert on `T` (`switch any(x).(type) { case A: ...; case B: ... }`) on |
| 68 | + every call path — the generic buys nothing over just accepting `any` or, better, unifying `A` |
| 69 | + and `B` into one real type. This is a strong "these two types should be one type" signal, not a |
| 70 | + legitimate use of generics. |
| 71 | + |
| 72 | +4. **Self-aware suppression of a mandated rule.** A `//nolint`, `//revive:disable`, `#nosec`, or |
| 73 | + similar suppression comment that turns off a rule CLAUDE.md explicitly mandates (Options |
| 74 | + Pattern, cyclomatic complexity, file-length limits, sentinel errors) — especially one whose own |
| 75 | + comment text admits the violation ("too many params", "TODO: refactor", "temporary"). |
| 76 | + |
| 77 | +5. **Business logic in the wrong layer.** A `cmd/*.go` file containing loops, external API/network |
| 78 | + calls (git, GitHub, HTTP), or non-trivial branching beyond flag parsing and dispatch — that |
| 79 | + logic belongs in `pkg/`. Also flag any *new* file added under `internal/exec/` — this repo has a |
| 80 | + standing direction to stop growing that package; new abstractions belong under `pkg/`. |
| 81 | + |
| 82 | +6. **Admitted-but-unshipped gap.** A `TODO`/`FIXME`/"not yet"/"doesn't support X yet" comment on a |
| 83 | + code path reachable from a documented, non-experimental command or config field, where the gap |
| 84 | + itself is *not* reflected anywhere a user would see it (`--help`, Docusaurus docs, error |
| 85 | + message) as "not supported." |
| 86 | + |
| 87 | +7. **Fake/stub feature — validates but doesn't work.** A config field or CLI flag that passes |
| 88 | + schema/flag validation but whose only runtime behavior is a hardcoded no-op or an unconditional |
| 89 | + error — the schema is lying about what's usable today. |
| 90 | + |
| 91 | +8. **Implemented but never wired up.** An exported function with real test coverage but zero |
| 92 | + non-test callers anywhere in the repo, especially one whose name implies it backs a user-facing |
| 93 | + command that doesn't actually expose it. |
| 94 | + |
| 95 | +9. **Documentation/code mismatch, either direction.** Check every command, flag, or config field |
| 96 | + this patch adds, changes, or removes against the docs that describe it (Docusaurus pages under |
| 97 | + `website/docs/`, `docs/prd/*.md`, agent-skill reference docs, `--help` text) for all four |
| 98 | + failure modes: |
| 99 | + - **Docs describe a feature that isn't actually implemented** — aspirational/promised |
| 100 | + documentation for something that doesn't exist yet, or no longer exists, in the code. |
| 101 | + - **Docs describe how something used to work** — accurate once, but a later change in this |
| 102 | + patch (or a recent one) shifted the real behavior and nobody updated the prose describing it. |
| 103 | + - **Missing documentation** — a new, shipped, non-experimental command/flag/config field with |
| 104 | + no corresponding doc update at all, per CLAUDE.md's own "All new commands/flags/parameters |
| 105 | + MUST have Docusaurus documentation" and "Update all schemas... when adding config options" |
| 106 | + mandates. |
| 107 | + - **Implemented but undocumented** — the same failure as above, viewed from the code side: a |
| 108 | + capability that's fully built and reachable, but a user reading the docs would never learn it |
| 109 | + exists. |
| 110 | + |
| 111 | +## What NOT to flag (avoid false positives) |
| 112 | + |
| 113 | +- A stub that's honestly documented as unavailable (`--help` text, Docusaurus docs, or an |
| 114 | + intentionally loud error naming exactly what's missing and why) — the smell is a *silent* or |
| 115 | + *misleadingly-validated* stub, not a disclosed one. |
| 116 | +- Legitimate generics whose body does real compile-time-dispatched work, not a runtime type-switch |
| 117 | + on every path. |
| 118 | +- Pre-existing code outside the current patch, in default (patch-aware) mode — that's what an |
| 119 | + explicit full-repo sweep is for. |
| 120 | +- Errors already using this repo's sentinel pattern correctly (`errors/errors.go` or a |
| 121 | + package-local `errors.go` mirroring it). |
| 122 | +- `TODO`/stub markers inside test files, example fixtures, or explicitly experimental |
| 123 | + (`ATMOS_EXPERIMENTAL`-gated) commands — those are expected to be incomplete by design. |
| 124 | +- A doc gap on code outside the current patch, in default (patch-aware) mode — flag it only if |
| 125 | + this patch touched the feature or its docs; a pre-existing, unrelated doc gap belongs to an |
| 126 | + explicit full-repo sweep, not a routine cycle. |
| 127 | +- Internal/unexported helpers, and code under `internal/`, that were never meant to have |
| 128 | + user-facing docs in the first place — item 9 is about user-facing surface area, not every |
| 129 | + function needing a comment. |
| 130 | + |
| 131 | +## Reporting |
| 132 | + |
| 133 | +Use the `ReportFindings` tool, most-severe first (empty array if nothing survives). For each |
| 134 | +finding, cite the exact file/line and which checklist item (1-9 above) it matches, plus a concrete |
| 135 | +failure scenario (what a user configures/calls, and what silently goes wrong). Mark `CONFIRMED` |
| 136 | +when the code unambiguously matches a checklist item, `PLAUSIBLE` when it's a suspicious pattern |
| 137 | +that needs a human's architectural judgment call either way — don't force a PLAUSIBLE into |
| 138 | +CONFIRMED just to simplify the report. |
| 139 | + |
| 140 | +## Auto-fix policy |
| 141 | + |
| 142 | +Only one class of finding is safe to fix without a human decision: **item 2 (missing sentinel |
| 143 | +errors)**, and only when the fix is a pure mechanical conversion — replace a dynamic |
| 144 | +`fmt.Errorf`/`errors.New` string with a declared sentinel + `%w` wrap, preserving the exact |
| 145 | +original message text and error chain. Nothing else on the checklist gets auto-fixed. Items 1, 3, |
| 146 | +5, 6, 7, and 8 are architectural judgment calls by nature — report them, don't guess at a fix. |
| 147 | +Item 9 (doc/code mismatch) is never auto-fixed either, even though it can look mechanical: writing |
| 148 | +accurate docs means following this repo's Docusaurus template and house style, not just filling a |
| 149 | +gap with the first plausible sentence — report it and let a human (or a dedicated doc pass) write |
| 150 | +the actual content. Item 4 (nolint suppression) is judgment-adjacent: report it, and only remove |
| 151 | +the suppression yourself if you also fix the underlying violation in the same pass; never just |
| 152 | +delete a `//nolint` comment and leave the violation live. |
| 153 | + |
| 154 | +## Related |
| 155 | + |
| 156 | +- **[`fix-all` skill](../fix-all/SKILL.md)** — invokes this skill at its own step 8 as part of the |
| 157 | + hourly/on-demand PR-readiness cycle; an unresolved finding blocks the "ready for final review" |
| 158 | + state the same way an unfixed lint finding or an invalid-but-unresolved CodeRabbit thread does. |
| 159 | +- **[`lint` skill](../lint/SKILL.md)** — catches mechanical/syntactic violations `golangci-lint` |
| 160 | + can express as a rule; this skill catches the semantic/architectural ones that require reading |
| 161 | + intent, which a linter structurally cannot do. |
| 162 | +- **`code-review`** (native skill) — general bug/security/performance/simplification review, not |
| 163 | + scoped to this repo's own CLAUDE.md architectural mandates. Use `code-review ultra` instead of |
| 164 | + this skill when you want a deep, adversarially-verified, multi-agent pass across a whole PR. |
| 165 | +- `.claude/hooks/security-remediate-trigger.sh` — the precedent this skill's dedup-by-hash pattern |
| 166 | + is modeled on, applied inline here instead of as a separate hook. |
0 commit comments