docs: add deletion probes for testing necessity empirically - #50
Conversation
The YAGNI standard, the sixth review dimension and the plan/implementation rules are all judgement-based: someone reads the code and decides whether a piece of machinery is needed. That judgement is exactly what fails, because the author's local justification almost always holds up on its own terms. Adds a "Deletion probes" subsection to coding-standards.md: delete the candidate, run the verification, and read the result. Records the part that matters most, that a clean run has two meanings and conflating them turns a coverage gap into a mistaken deletion. Wires it into all three phases in CLAUDE.md: plan states what breaks without each non-obvious item, the implementer probes its own additions before review, and the over-engineering dimension probes rather than argues when necessity is contested.
📝 WalkthroughWalkthroughThe documentation adds deletion probes for testing whether questionable code is necessary. Planning, implementation, and over-engineering review guidance now requires recording omission impact and interpreting verification results. ChangesDeletion probe guidance
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 78-83: Update the deletion-probe guidance around “delete it, run
the full verification” to require a clean baseline verification on unchanged
code first, followed by a comparable verification after deleting only the
candidate. Count only new, reproducible failures as evidence that the candidate
is load-bearing; if either run is incomplete, non-comparable, pre-existing, or
flaky, restore the candidate and report the probe as inconclusive.
🪄 Autofix
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: d3dbfafe-6856-4193-ba83-5c2b8e4797da
📒 Files selected for processing (2)
CLAUDE.mdcoding-standards.md
| Whether a piece of machinery is needed is an empirical question, so answer it empirically instead of by reasoning about the code. For each candidate: **delete it, run the full verification, and read the result.** | ||
|
|
||
| - **Something fails** → it is load-bearing. Restore it, and record *what* failed: that failure is the evidence it earns its place, and it belongs in the PR description or the protected list. | ||
| - **Nothing fails** → exactly one of two things is true, and you must say which: | ||
| 1. the code was genuinely unnecessary — delete it for real; or | ||
| 2. **the verification is too weak to notice** — restore the code and report a coverage gap. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require a clean baseline before interpreting a deletion probe.
Line 80 treats any post-deletion failure as evidence that the candidate is load-bearing. A pre-existing or flaky failure can produce the same result. Run the same verification on the unchanged code first. Then run it after deleting only the candidate. Treat only a new, reproducible failure as evidence. If either run is incomplete or not comparable, restore the candidate and report the probe as inconclusive.
Proposed probe sequence
-For each candidate: **delete it, run the full verification, and read the result.**
+For each candidate, first run the full verification on the unchanged code and confirm that it completes cleanly. Then delete only that candidate and rerun the same verification.
-- **Something fails** → it is load-bearing. Restore it, and record *what* failed: that failure is the evidence it earns its place, and it belongs in the PR description or the protected list.
+- **A new, reproducible failure appears only after deletion** → it is load-bearing. Restore it, and record *what* failed: that failure is the evidence it earns its place, and it belongs in the PR description or the protected list.
+- If either verification run is incomplete or not comparable, restore the candidate and report the probe as inconclusive.As per coding guidelines, verification must be explicit and must not silently accept invalid or incomplete results.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Whether a piece of machinery is needed is an empirical question, so answer it empirically instead of by reasoning about the code. For each candidate: **delete it, run the full verification, and read the result.** | |
| - **Something fails** → it is load-bearing. Restore it, and record *what* failed: that failure is the evidence it earns its place, and it belongs in the PR description or the protected list. | |
| - **Nothing fails** → exactly one of two things is true, and you must say which: | |
| 1. the code was genuinely unnecessary — delete it for real; or | |
| 2. **the verification is too weak to notice** — restore the code and report a coverage gap. | |
| Whether a piece of machinery is needed is an empirical question, so answer it empirically instead of by reasoning about the code. For each candidate, first run the full verification on the unchanged code and confirm that it completes cleanly. Then delete only that candidate and rerun the same verification. | |
| - **A new, reproducible failure appears only after deletion** → it is load-bearing. Restore it, and record *what* failed: that failure is the evidence it earns its place, and it belongs in the PR description or the protected list. | |
| - **Nothing fails** → exactly one of two things is true, and you must say which: | |
| 1. the code was genuinely unnecessary — delete it for real; or | |
| 2. **the verification is too weak to notice** — restore the code and report a coverage gap. | |
| - If either verification run is incomplete or not comparable, restore the candidate and report the probe as inconclusive. |
🤖 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 78 - 83, Update the deletion-probe guidance
around “delete it, run the full verification” to require a clean baseline
verification on unchanged code first, followed by a comparable verification
after deleting only the candidate. Count only new, reproducible failures as
evidence that the candidate is load-bearing; if either run is incomplete,
non-comparable, pre-existing, or flaky, restore the candidate and report the
probe as inconclusive.
Source: Coding guidelines
What
A "Deletion probes" subsection under "Verifying a Refactor" in
coding-standards.md, wired into all three phases inCLAUDE.md.Delete the candidate, run the verification, read the result. Necessity is an empirical question; stop answering it by reading the code.
Why
#44, #46 and #48 are all judgement-based — someone decides, by reading, whether a piece of machinery is needed. That judgement is precisely what fails: the author's local justification for a guard, a parameter or a fallback almost always holds up on its own terms, which is why over-engineering survives review even when reviewers are hunting for it.
Evidence, in both directions
From the six-PR cleanup that prompted this whole thread:
timeout => 1800over Puppet's 300s defaultGOPRIVATE/GIT_CONFIG_GLOBALbeside a redirectedHOMENo amount of reading the manifest establishes the 301-second fact. A build does, in one run.
The part actually worth writing down
"Nothing broke" has two meanings, and conflating them is how a coverage gap gets mistaken for dead code:
If deleting a guard that protects a real invariant breaks no test, the honest finding is "this guard is untested", not "this guard is unnecessary". Those cases point straight at the hole that let the risk in, and are usually worth more than the deletion would have been.
Where it lands
The through-line: a parameter proven necessary by a failing build is no longer a matter of opinion.
Scope
18 added lines across two files.
Closes #49
Summary by CodeRabbit