clarity-review

SKILL.md

name	clarity-review
description	Lint the current diff for the AI-authored-code patterns that repeatedly draw pushback in this repo's PR reviews — comment slop, unverified technical claims, dead code, tautological tests, missing feature flags, single-letter names, unneeded complexity, concurrency footguns, and inconsistent formatting. Run before opening a PR, not after — this is a pre-flight lint, not a bug-catching pass (use hny-review-general/hny-review-thorough for that).
disable-model-invocation	true

Clarity review

Calibrated against roughly 200 real review comments on hound PRs over the prior three months — courtesy of our resident AI slop proliferation curmudgeon — plus recurring #eng-ai/#misc-ai discussion of where AI-authored code and prose fall short.

This is a pre-PR lint pass you run yourself, locally, before opening a PR. It does not post anything, does not touch CI, and is not a substitute for hny-review-general / hny-review-thorough — those look for bugs (state machines, concurrency hazards, missing error handling); this looks for the style and rigor issues that make a PR harder to review or erode trust in it, and it belongs before the PR exists, not as a comment littering one that's already open.

PR description quality is /pr's job (.agents/commands/pr.md, enforced by CLAUDE.md rule #3) — don't duplicate its rules here. This pass looks only at the diff itself.

Scope

Diff the current branch against main, plus the commit messages of every commit being reviewed:

git diff "$(git merge-base HEAD main)"...HEAD
git log "$(git merge-base HEAD main)"...HEAD

Read whatever you need to for evidence — callers, referenced source, existing tests/benchmarks, config — but only report findings on changed lines. Skip anything already squarely in general/thorough/performance territory (correctness bugs, concurrency hazards unrelated to throughput, generic perf issues) — this pass is about the patterns below specifically.

1. AI-authored comment slop

The single most frequent and most strongly worded complaint in this repo's review history — and one nearly everyone on the team has voiced independently, not just one reviewer. Flag every occurrence:

• A comment restates what the code already says (paraphrases the name/signature, narrates "increment the counter", explains how append or a well-known stdlib/language feature works). Litmus test: if a reader who knows the language loses nothing by deleting it, cut it.
• A comment explains the behavior of some other package, a hypothetical implementation, or an alternative approach that isn't actually present in the code. Reviewers have caught fabricated technical claims this way more than once.
• The same explanation is repeated across multiple comments/files instead of stated once at its most specific home.
• Filler in places nobody reads for documentation (e.g. LaunchDarkly flag descriptions, generated-looking test assertion messages).
• Any doc comment or inline comment longer than 2-3 lines is suspect by default — it's almost always restated mechanism the code beside it already shows. Cut to the one non-obvious "why." Exception: Go package doc comments (// Package x ...) and CLAUDE.md/AGENTS.md-style instruction files, which are expected to run long.
• A field, variable, or constant comment that just restates what its name and type already convey (a mutex field explained as being for locking, a counter like rowCount explained as counting rows) — cut it; comment a field only when the name and type genuinely don't convey its purpose.
• A function's doc comment describes how or why calling code uses it, rather than its own contract — API documentation never documents its callers. Exception: a genuinely single-purpose method where other callers are expressly disallowed, and even then flag the restriction itself as worth a second look.

Quote the exact offending text and say which failure mode it is (restates code / narrates a hypothetical / repeated elsewhere / over length / documents a caller instead of the callee).

2. Unverified technical claims — including your own

Reviewers repeatedly reject claims of "faster," "confirmed working," "no impact," or descriptions of why something behaves a certain way when there's no benchmark, profile, or test backing it up. The default posture here is to distrust AI-generated technical claims until checked — prior AI-authored review commentary and code comments have both been caught asserting incorrect things about cache/concurrency/algorithm behavior that didn't hold up once someone checked the actual code.

• If the diff makes a performance or correctness claim (in a comment or commit message), check whether a benchmark/test actually demonstrates it, and whether that benchmark reflects the real worst-case or majority-case workload (not just the easy case).
• If you're about to state a claim about the behavior of other code, a library, or a cache in this diff, verify it against the actual referenced source first. Don't repeat a plausible-sounding explanation without checking it.
• Watch for assumptions that an extra pass, map build, or allocation is "free."

3. Dead code and unrealized surface

• A function returns a value that no caller ever uses (return bool/nothing instead), or an unexported helper with no caller left in the package — narrow it or drop it. Do not recommend dropping or narrowing an exported type/field/function on the strength of an in-repo caller search alone — external consumers, reflection, or generated code may use it and a repo-local grep can't rule that out.
• A type/function/field newly introduced in this diff is exported (capitalized) with no caller of it outside its own package, anywhere in the diff or the rest of the repo. This is the mirror case of the one above: a brand-new symbol can't yet have an external consumer to worry about, so flag the unnecessary export and suggest lowercasing it until an outside caller actually exists.
• Speculative code paths, parameters, or config for a scenario that doesn't currently occur ("future-proofing") instead of adding it when the need is real.
• Old/fallback logic kept "just in case" alongside a replacement, with no plan or flag to remove it.

4. Tautological or shallow tests

• A test that's almost entirely setup with one low-information assertion at the end (e.g. asserting non-nil, or a debug print instead of an exact expected value) — the kind of test that would still pass if the logic under test were wrong.
• Test coverage that only exercises the simplest case when the diff's own logic implies harder cases exist (multiple filter sets, concurrent access, non-empty duplicates, etc.) — check directly whether the new/changed cases are actually the ones being asserted on.

5. Feature-flag discipline

For a behavior change to retriever/schemas query execution or read paths (a new optimization path, storage/codec read format): is it gated behind a partition- or dataset-scoped LaunchDarkly flag, default-off, rather than a hardcoded selector or unconditional rollout? If not, and there's no clear reason to skip a flag, flag it.

For a change to a peer-synchronized write path, don't apply this rule as-is — a flag that can read differently per replica risks peer divergence. Note the gap and defer to hny-review-thorough for the rollout-safety call instead of demanding a LaunchDarkly flag.

6. Naming and terminology

• Single-character variable names beyond a loop index or receiver (already a repo rule; reviewers call this out close to every time they see it).
• Metaphor-borrowed jargon that doesn't match this codebase's existing vocabulary (e.g. needle/haystack instead of filter/search terms already used elsewhere).
• Generic names (write, data, process) on non-trivial functions or in large functions where a specific name would help.
• Unexplained abbreviations that aren't expanded inline.

7. Complexity for its own sake

• Two-phase or lazy-init patterns, extra boolean flags, or wrapper structs that don't reduce complexity versus a direct approach — ask "why not just do X directly."
• Edge-case handling (nil keys, duplicate IDs, unusual identity comparisons) added without explanation — flag the missing justification as a clarity gap (why does this case need handling?). This pass doesn't judge whether the handling itself is correct; that's hny-review-general/hny-review-thorough territory.
• Any construction where a simpler, more obvious formulation is available in the same diff.