Skip to content

Commit 89fc4e1

Browse files
authored
Merge branch 'main' into renovate/floci-floci
2 parents 8fab89f + b0929ee commit 89fc4e1

187 files changed

Lines changed: 18936 additions & 1858 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.claude/skills/atmos-sbom

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
../../agent-skills/skills/atmos-sbom
Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
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

Comments
 (0)