docs: add YAGNI and comment-density guidance - #44
Conversation
A reviewer on a batch of agent-authored PRs said the changes were "so overengineered i had a hard time getting through it". Two problems sat behind that: code doing more than any caller asked for, and comment bloat burying it. For one feature the human version was 58 lines against 343; comment-to-code on added lines ran 37-66%, and a one-line config change carried a four-line comment. Neither was a rule violation, because no rule existed. - coding-standards.md: add "Simplicity & Scope (YAGNI)" (build for the current caller, rule-of-three before abstracting, don't validate the impossible, pre-PR self-check) and "Comments" (default to none, 1-2 lines, ~15% budget, explicit delete list, good/bad example). - coding-standards.md: replace the weak inline-comments line under Documentation with a pointer, so there is one source of truth. - CLAUDE.md: clarify §5 in place - elegance means fewer moving parts, not more sophisticated ones - since "is there a more elegant way?" reads cold as an invitation to elaborate. Fold YAGNI into the existing Simplicity First principle and add a comment-density principle. - git-workflow.md: add over-engineering and comment density to the pre-commit review dimensions so the gate catches both. Closes #43
cristim
added
documentation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe documentation now defines YAGNI, simpler implementation scope, limited validation and abstraction, concise comments, and pre-commit checks for over-engineering and comment density. ChangesEngineering Guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…correcting A line-by-line audit of the 343-line manifest against the reviewer's 58-line one showed roughly a third of the extra code was load-bearing, not fat: a binary_name parameter a real caller needs, GOPRIVATE and submodule auth for private module fetches, a timeout the platform default gets wrong, and an ensure => absent guard. That nuance is what makes the rule usable, so state it: - The test is not "is this correct?" or "could this ever happen?" but "does a current caller need this, and can this state be reached given the callers that exist?" The clearest case in that batch was ~15 lines of build-stamp idempotence machinery that was correct, reasoned and tested, and still waste, because Packer runs the code once per image and then discards the machine. - Prefer removing the hazard to defending against it: collapsing one shell-interpolated onlyif into a constant string deleted the injection path and made the surrounding validation unnecessary. - Don't double up validation (a type constraint and a runtime assert for the same value). - Closing caution so this doesn't read as "shorter is better": cut speculation, not behaviour.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
coding-standards.md (1)
164-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd exceptions for required documentation and stable constraint references.
The “Delete on sight” list includes past-bug references and JSDoc/docstrings, but this section permits comments for external-bug workarounds and the Documentation section requires API docs to stay synchronized. Retain concise issue or CVE references and semantic API documentation when they explain a constraint. Delete only redundant restatements and review-history narrative.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-standards.md` around lines 164 - 167, Update the “Delete on sight” guidance in coding-standards.md to explicitly preserve concise external-bug or CVE references when documenting workarounds, and retain semantic JSDoc/docstrings required by the Documentation section for API contracts. Continue removing only redundant type restatements and review-history narrative.CLAUDE.md (1)
185-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the simplicity heuristics across both documents.
The guidance should reject unnecessary machinery, not justified abstractions or readable implementations.
CLAUDE.md#L185-L185: qualify the rule so an added concept is acceptable when it removes duplication, clarifies an invariant, or preserves a boundary contract.coding-standards.md#L47-L48: make the rule-of-three a consideration, not a mandatory abstraction threshold.coding-standards.md#L52-L56: replace fixed consumer and line-count tests with checks for current need, correctness, clarity, and total complexity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` at line 185, Align the simplicity guidance across all three sites: in CLAUDE.md lines 185-185, qualify the rule so added concepts are acceptable when they remove duplication, clarify an invariant, or preserve a boundary contract; in coding-standards.md lines 47-48, make the rule-of-three a consideration rather than a mandatory abstraction threshold; and in coding-standards.md lines 52-56, replace fixed consumer and line-count tests with evaluation of current need, correctness, clarity, and total complexity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-standards.md`:
- Line 49: Revise the “Don't validate the impossible” guidance to distinguish
redundant internal checks from invariant enforcement: retain validation at trust
boundaries, but require explicit assertions or errors when internal invariants
are violated. Limit the recommendation to checks guaranteed by a maintained
contract, rather than treating all defensive handling of unreachable states as
dead code.
- Around line 169-180: Update both context.WithTimeout examples to call defer
cancel() immediately after creating the timeout context, preserving the existing
comments and timeout behavior while ensuring each context’s resources are
released when its operation completes.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 185: Align the simplicity guidance across all three sites: in CLAUDE.md
lines 185-185, qualify the rule so added concepts are acceptable when they
remove duplication, clarify an invariant, or preserve a boundary contract; in
coding-standards.md lines 47-48, make the rule-of-three a consideration rather
than a mandatory abstraction threshold; and in coding-standards.md lines 52-56,
replace fixed consumer and line-count tests with evaluation of current need,
correctness, clarity, and total complexity.
In `@coding-standards.md`:
- Around line 164-167: Update the “Delete on sight” guidance in
coding-standards.md to explicitly preserve concise external-bug or CVE
references when documenting workarounds, and retain semantic JSDoc/docstrings
required by the Documentation section for API contracts. Continue removing only
redundant type restatements and review-history narrative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b10e0b3d-fbe1-42a2-a770-0f97ff780e9b
📒 Files selected for processing (3)
CLAUDE.mdcoding-standards.mdgit-workflow.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
The two context.WithTimeout examples left `cancel` unused, which does not compile and leaks the timer; add `defer cancel()` to both. "Delete on sight" listed "past bugs" while the bullet above sanctions a comment for an external-bug workaround. Say "our own past bugs" and note that the sanctioned case carries an upstream issue or CVE link, so the two rules no longer read as contradicting each other.
|
Triage of the review, thanks @coderabbitai: Addressed in 08e6ede
Dismissed
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 14 minutes. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 08e6ede
git show --format=fuller --find-renames --find-copies 08e6ede -- \
CLAUDE.md coding-standards.md git-workflow.md
printf '\n--- current occurrences and context ---\n'
rg -n -i -C 8 \
'context\.WithTimeout|defer cancel|Delete on sight|our own past bugs|external.*(issue|CVE)|JSDoc|docstrings|Rule of three|rule of three|added concept|Cut speculation|current caller|consumer|lines' \
CLAUDE.md coding-standards.md git-workflow.mdLength of output: 39085
I withdraw the JSDoc/docstring finding. The rule is limited to type-restating documentation. The Documentation section still requires semantic API documentation. I also accept the decision on the simplicity heuristics. The section states that real caller requirements and load-bearing behavior remain valid, regardless of line count. The firm self-check prompts are consistent with the stated purpose of this PR. ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-standards.md`:
- Line 172: Update the “Delete on sight” bullet in coding-standards.md to escape
or inline-code the “<date>” placeholder so Markdown renders it visibly as text
rather than interpreting it as an HTML tag.
In `@git-workflow.md`:
- Around line 47-48: Align the review loop marked “NO EXCEPTIONS” with the skip
policy in CLAUDE.md: define the trivial/mechanical-edit exception once, or
restrict the loop to non-trivial or substantial changes. Keep the existing
checks for changes that remain in scope and update the accompanying comment so
it accurately describes the resulting policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8ef1662-d209-48e2-8a00-c5f831f2c3d6
📒 Files selected for processing (3)
CLAUDE.mdcoding-standards.mdgit-workflow.md
GitHub's Markdown strips `<date>` as an unknown HTML tag, so the bullet rendered as `as of ""` and the placeholder disappeared. Wrap the phrase in backticks so it renders literally.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 49 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 52 minutes. |
) * docs: add refactor-verification guidance and the worktree hook trap The YAGNI section added in #44 says what to cut but nothing about proving the cut preserved behaviour, which leaves "delete aggressively" as advice without a safety net. Adds a "Verifying a Refactor" section covering subtractive-only edits, byte-comparing every generated artifact rather than trusting tests, carrying a protected list into the work, never removing security assertions on your own judgement, and treating "nothing worth removing" as a complete answer. Also records that git hooks silently do not run in a worktree when core.hooksPath points at an install-generated directory, which matters because worktrees are the default for non-trivial work. * docs: make over-engineering the sixth review dimension The YAGNI guidance only bit when someone remembered to look for it. Adding it to the review dimensions puts it in the plan-review gate, the post- implementation review, the local review loop and the pre-commit loop, so pruning happens on every change rather than as an occasional cleanup. Flagged as an adversarial check on purpose: the author's local justification for a piece of machinery almost always holds up, so the reviewer has to ask what the calling system actually does instead. Updates the five-dimension references across git-workflow.md and the two autopilot prompts to match.
1 participant
Closes #43
Why
A reviewer on a batch of agent-authored PRs: "wow, that was so overengineered i had a hard time getting through it." Two problems sat behind that sentence, and no rule covered either one.
Volume. For the same feature (a Puppet profile that builds a Go binary and installs it), the reviewer's version was 58 lines; the agent's was 343.
Comment bloat on top of it. Measured on added lines: 66% comment-to-code on the worst file (214 comment lines to 111 code lines), a four-line comment on a one-line config change, and three other PRs at 44%, 40% and 37%.
A line-by-line audit of that manifest is what shaped the guidance, because the honest result was not "the extra 285 lines were fat":
binary_nameparameter a real caller needs,GOPRIVATE+ submodule auth for private module fetches,timeout => 1800where Puppet's exec default of 300s is wrong, running the build as the checkout owner, anensure: absentguard.Pattern[...]constraints plusassert_typeguards for the same values.The build-stamp machinery is the instructive case: correct, carefully reasoned, and tested, and still waste, because the state it guarded was unreachable. That is what the section is built around. Reworked, the manifest went 343 -> 100 lines with every load-bearing behaviour intact, and comments 214 -> 20.
What changed
coding-standards.mdgains two sections:## Simplicity & Scope (YAGNI)— leads with the test that the audit produced: not "is this correct?" or "could this ever happen?" but "does a current caller need this, and can this state be reached given the callers that exist?" Then: build only what a current caller needs; DRY but rule-of-three, since premature abstraction is over-engineering too and harder to undo than duplication; validate at trust boundaries and don't double up; prefer removing the hazard to defending against it (if a value needs validating because it reaches a shell, check whether it needs to reach a shell at all). Ends with a four-question pre-PR self-check and a closing caution that cutting means removing speculation, not behaviour — "shorter" is the symptom of getting this right, not the goal.## Comments— default to no comment; comment only where the WHY isn't deducible from the code; 1-2 lines, never paragraphs; a ~15% comment-to-code budget on added lines as a self-check; an explicit delete-on-sight list (restatements, rationale essays, review-round references, JSDoc restating the signature). Framing: rationale belongs in the PR description, the source carries only what a future editor needs in order not to break something. One good/bad example pair.How the existing guidance was reconciled
This is the part worth reviewing, since the risk was bolting contradicting rules alongside what's already there.
CLAUDE.md§5 "Demand Elegance (Balanced)" opened with "pause and ask 'is there a more elegant way?'", which reads cold as an invitation to elaborate and plausibly contributed to the outcome above. Rather than adding a competing rule, §5 is clarified in place: the question becomes "is there a simpler way?", and elegance is defined as fewer moving parts, not more sophisticated ones — explicitly a prompt to remove machinery, never to add it. Section number and heading are unchanged, so the existing§5cross-references still resolve.## Documentationline ("Inline comments only for non-obvious logic") becomes a pointer to the new## Commentssection, leaving one source of truth instead of two overlapping statements.git-workflow.mdpre-commit review dimensions gain "Over-engineering & scope" and extend "Comment accuracy" to "Comment accuracy & density" — the gate previously checked whether comments had rotted but never whether there were too many. Without this the new standards would have had no enforcement point.Scope
52 insertions, 6 deletions across three files. Kept deliberately tight: writing a treatise about not writing treatises would undercut the point.
Summary by CodeRabbit