docs: add refactor-verification guidance and the worktree hook trap - #46
Conversation
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.
📝 WalkthroughWalkthroughThe documentation adds rules for proving refactors preserve behavior, expands review guidance from five to six dimensions, and documents worktree Git hook verification with manual fallback checks. ChangesDocumentation guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 68: Update the generated-artifact comparison guidance in
coding-standards.md to require deterministic generation before byte comparison.
For unavoidable volatility such as timestamps, absolute paths, generated
identifiers, or ordering, define and document a canonical comparison that
normalizes only those fields and explicitly reports them; retain the requirement
to compare every emitted file.
- Line 67: Update the “Subtractive only” guidance in coding-standards.md to
scope it to behavior-preserving cleanup: state that deletion and inlining are
preferred, while justified abstractions remain allowed when required by existing
callers and the rule of three. Preserve the warning against untested
behavior-changing restructures.
In `@git-workflow.md`:
- Line 96: Update the worktree hook verification guidance in git-workflow.md to
resolve the effective core.hooksPath from the worktree root and check each
required hook file, especially pre-commit, for both existence and executable
permissions rather than checking only the directory. Instruct users to run the
corresponding checks manually when any required hook is missing or
non-executable, without using --no-verify.
🪄 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: 560a3369-94f3-497b-96ae-34b605899aed
📒 Files selected for processing (2)
coding-standards.mdgit-workflow.md
|
|
||
| A cleanup that changes behaviour is a bug wearing a tidy diff. "Delete aggressively" is only safe paired with proof you deleted nothing that mattered, so cutting and proving are one task, not two. | ||
|
|
||
| - **Subtractive only.** Delete and inline; never introduce an abstraction to "simplify". A clever restructure is a behaviour change you haven't tested yet. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scope the “Subtractive only” rule.
Line 50 says to introduce an abstraction at the rule of three. The PR objective says to favor subtractive edits, not to forbid every new abstraction. State that deletion and inlining are preferred for behavior-preserving cleanup, while justified abstractions remain allowed when current callers require them.
Proposed wording
-- **Subtractive only.** Delete and inline; never introduce an abstraction to "simplify".
+- **Prefer subtractive edits.** Delete and inline where this removes unused code. Introduce an abstraction only when a current caller or the rule of three requires it, and verify that behavior is preserved.🤖 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` at line 67, Update the “Subtractive only” guidance in
coding-standards.md to scope it to behavior-preserving cleanup: state that
deletion and inlining are preferred, while justified abstractions remain allowed
when required by existing callers and the rule of three. Preserve the warning
against untested behavior-changing restructures.
| A cleanup that changes behaviour is a bug wearing a tidy diff. "Delete aggressively" is only safe paired with proof you deleted nothing that mattered, so cutting and proving are one task, not two. | ||
|
|
||
| - **Subtractive only.** Delete and inline; never introduce an abstraction to "simplify". A clever restructure is a behaviour change you haven't tested yet. | ||
| - **Compare the generated output, not just the tests.** Where code emits an artifact (IaC plans/synth, migrations, codegen, bundles, snapshots), byte-compare it before and after and require it identical. Green tests routinely coexist with a changed artifact: in IaC a renamed logical id silently destroys and recreates live infrastructure, and no unit test sees it. Compare *every* emitted file, not just the obvious one. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define the handling of non-deterministic artifacts.
Line 68 requires byte-identical output for every generated artifact. Some generators include timestamps, absolute paths, generated identifiers, or unstable ordering. Require deterministic generation first. If that is not possible, define a canonical comparison for documented volatile fields and report those fields explicitly.
Proposed wording
- Compare the generated output, not just the tests. Where code emits an artifact (IaC plans/synth, migrations, codegen, bundles, snapshots), byte-compare it before and after and require it identical.
+ Compare the generated output, not just the tests. Make generation deterministic, then byte-compare every emitted file before and after and require it identical. If determinism is impossible, define and review a canonical comparison for documented volatile fields.🤖 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` at line 68, Update the generated-artifact comparison
guidance in coding-standards.md to require deterministic generation before byte
comparison. For unavoidable volatility such as timestamps, absolute paths,
generated identifiers, or ordering, define and document a canonical comparison
that normalizes only those fields and explicitly reports them; retain the
requirement to compare every emitted file.
|
|
||
| After committing, run a quick sanity scan (`git show HEAD`) to catch anything the pre-commit loop missed. If this finds issues, treat it as a process failure (the pre-commit loop should have caught them). Fix-forward in a new commit only when strictly necessary (e.g., pre-commit hook caught a legitimate issue that required the commit to land first). | ||
|
|
||
| **Hooks may silently not run in a worktree.** When a repo sets `core.hooksPath` to a gitignored, install-generated directory (husky's `.husky/_` is the common case), that path exists only where the install ran — usually the main checkout. Git skips hooks with no warning when it doesn't resolve, so commits from a worktree can quietly bypass lint, formatting and generated-artifact rebuilds. Since §1b puts non-trivial work in worktrees, check once per worktree (`git config core.hooksPath`, then confirm the directory exists) and run the checks by hand if it doesn't. Never paper over it with `--no-verify`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
root=$(git rev-parse --show-toplevel)
hooks=$(git config --get core.hooksPath || git rev-parse --git-path hooks)
case "$hooks" in /*) ;; *) hooks="$root/$hooks" ;; esac
test -x "$hooks/pre-commit"Repository: LeanerCloud/dotclaude
Length of output: 159
🏁 Script executed:
printf '%s\n' '--- git-workflow.md ---'
sed -n '80,105p' git-workflow.md
printf '%s\n' '--- hook-related guidance ---'
rg -n -C 3 'core\.hooksPath|pre-commit|hooks|worktree' --glob '!node_modules' .
printf '%s\n' '--- repository state ---'
git config --show-origin --get core.hooksPath || true
git rev-parse --show-toplevel
git rev-parse --git-dir
git rev-parse --git-path hooksRepository: LeanerCloud/dotclaude
Length of output: 50379
🌐 Web query:
Git core.hooksPath relative path worktree git rev-parse --git-path hooks executable hook behavior
💡 Result:
The behavior of core.hooksPath, particularly regarding relative paths and git worktrees, is governed by how Git resolves directory paths during execution [1][2][3]. Core.hooksPath Behavior Git allows you to override the default hooks directory ($GIT_DIR/hooks) using the core.hooksPath configuration variable [1][2][3]. When set to a relative path, Git interprets this path relative to the current working directory from which the Git command is executed, not necessarily the repository root or the.git directory [1][3]. Before executing a hook, Git changes its working directory to the root of the working tree (in non-bare repositories) or to the $GIT_DIR (in bare repositories) [2][3]. Git Worktree Considerations Git worktrees often lead to complications when using relative paths for core.hooksPath [4][5][6]. Because Git resolves relative paths based on the current working directory, a path that works in the main repository may fail in a worktree if the working directory structure differs [4][6]. For instance, if core.hooksPath is set to a relative path like.hooks, and you run a Git command from inside a worktree, Git will look for.hooks relative to that specific worktree's root, which may not contain the expected hooks directory [5][6]. To ensure consistent behavior across worktrees, it is generally recommended to use absolute paths [4][5][6]. Git rev-parse --git-path hooks The command git rev-parse --git-path hooks is designed to respect the core.hooksPath configuration [7][8]. If core.hooksPath is set, git rev-parse --git-path hooks will return the configured path rather than the default $GIT_DIR/hooks location [7][9]. This command is a reliable way to check where Git expects to find hooks for the current context [7]. You can use the --path-format=(absolute|relative) option (available since Git v2.31) to force the output to be an absolute or relative path, as desired [9][10]. Executable Hook Behavior Regardless of where the hooks are located (via default or core.hooksPath), Git will only execute files that have the executable bit set [11]. If a hook file is found at the specified path but lacks the executable permission, Git will ignore it [11].
Citations:
- 1: https://github.com/git/git/blob/master/Documentation/config/core.adoc
- 2: https://code.googlesource.com/git/+/HEAD/Documentation/githooks.adoc
- 3: ayu-ch/git@6675f50
- 4: core.hooksPath incorrectly calculated for git worktrees cachix/git-hooks.nix#688
- 5: Claude Code agent harness writes broken per-worktree core.hooksPath anthropics/claude-code#60620
- 6: core.hooksPath not propagated to git worktrees — merge hooks don't run gastownhall/beads#2414
- 7: git/git@9445b49
- 8: https://public-inbox.org/git/594839500.1229861.1471423409171.JavaMail.zimbra@ensimag.grenoble-inp.fr/T/
- 9: https://stackoverflow.com/questions/44855669/git-rev-parse-git-path-hooks-always-return-absolute-path
- 10: https://git-scm.com/docs/git-rev-parse.html
- 11: https://git-scm.com/docs/githooks
🏁 Script executed:
git --version
git help --no-man-viewer --no-info 'githooks' 2>/dev/null | sed -n '1,180p' || true
git help --no-man-viewer --no-info 'git-config' 2>/dev/null | rg -n -C 5 'core\.hooksPath|hooksPath' || true
git rev-parse --git-path --path-format=absolute hooks 2>/dev/null || trueRepository: LeanerCloud/dotclaude
Length of output: 212
🏁 Script executed:
set -u
printf '%s\n' '--- effective hook paths ---'
for value in '' '.hooks' '../shared-hooks' '/tmp/absolute-hooks'; do
if [ -n "$value" ]; then
printf 'core.hooksPath=%s\n' "$value"
git -c "core.hooksPath=$value" rev-parse --git-path hooks
git -c "core.hooksPath=$value" --path-format=absolute rev-parse --git-path hooks 2>&1 || true
else
printf '%s\n' 'core.hooksPath=<unset>'
git rev-parse --git-path hooks
git --path-format=absolute rev-parse --git-path hooks 2>&1 || true
fi
done
printf '%s\n' '--- config and repository roots ---'
git config --get core.hooksPath; printf 'config_exit=%s\n' "$?"
git rev-parse --show-toplevel
git rev-parse --git-dir
git rev-parse --git-common-dirRepository: LeanerCloud/dotclaude
Length of output: 2303
Check the hook file and its executable bit, not only the directory.
A present directory does not prove that pre-commit is available. Resolve the effective hooks path from the worktree root, then verify that each required hook exists and is executable. Run the checks manually if a required hook is missing or not executable.
🤖 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 `@git-workflow.md` at line 96, Update the worktree hook verification guidance
in git-workflow.md to resolve the effective core.hooksPath from the worktree
root and check each required hook file, especially pre-commit, for both
existence and executable permissions rather than checking only the directory.
Instruct users to run the corresponding checks manually when any required hook
is missing or non-executable, without using --no-verify.
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.
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 `@issue-pr-autopilot.worker.prompt.md`:
- Line 77: Update the implementation workflow step around “Self-review the six
dimensions” to require a separate Opus reviewer handoff for every implementation
change, in addition to implementer self-review. Explicitly include the Reuse and
scope-discipline checks, while preserving the authoritative CLAUDE.md
requirements and avoiding duplicated review responsibilities.
🪄 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: 4d9830f4-5d34-406b-b4f6-0c2c61ae1ee3
📒 Files selected for processing (4)
CLAUDE.mdgit-workflow.mdissue-pr-autopilot.plan.prompt.mdissue-pr-autopilot.worker.prompt.md
🚧 Files skipped from review as they are similar to previous changes (1)
- git-workflow.md
| b. Fetch + checkout the plan branch: git fetch origin $BR && git switch $BR. Read plan.md from the branch. | ||
| c. PLAN-STALENESS guard: rebase the auto branch onto the current base: git fetch origin feat/multicloud-web-frontend && git rebase origin/feat/multicloud-web-frontend. Re-validate plan.md against the rebased tree. If the base diverged enough that the plan is stale/invalid, re-plan inline (adjust to the current code) or, if it now needs a human design call, log 'deferred-plan-stale' and skip - do not build on a rotten plan. | ||
| d. Implement to repo standards; reuse existing helpers; do not duplicate. Self-review the five dimensions (completeness, correctness, security, bugs, duplication); fix findings. | ||
| d. Implement to repo standards; reuse existing helpers; do not duplicate. Self-review the six dimensions (completeness, correctness, security, bugs, duplication, over-engineering); fix findings. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Preserve the independent reviewer requirement.
CLAUDE.md line 124 requires a dedicated Opus reviewer for each implementation change. This step requires only implementer self-review. Because line 5 declares the global CLAUDE.md rules authoritative, the worker can satisfy this step while skipping reviewer separation. Add the explicit reviewer handoff and the Reuse and scope-discipline checks.
Based on the cross-file review contract in CLAUDE.md line 124.
🤖 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 `@issue-pr-autopilot.worker.prompt.md` at line 77, Update the implementation
workflow step around “Self-review the six dimensions” to require a separate Opus
reviewer handoff for every implementation change, in addition to implementer
self-review. Explicitly include the Reuse and scope-discipline checks, while
preserving the authoritative CLAUDE.md requirements and avoiding duplicated
review responsibilities.
1 participant
What
Adds a "Verifying a Refactor" section to
coding-standards.md(directly after Simplicity & Scope), plus one paragraph ingit-workflow.mdon a worktree trap.#44 encoded what to cut. This encodes how to prove the cut was safe — the half that was missing.
Why
coding-standards.mdcontains no rule about proving a refactor preserved behaviour, andCLAUDE.md§4/§6 only require regression tests for bug fixes. So the guidance currently says "delete aggressively" with no paired discipline. That is arguably worse than saying nothing.Where this came from
A six-PR cleanup driven by #44 removed ~2,240 lines across CDKTF, Puppet and GitHub Actions code. Every safeguard that made it safe was improvised per-brief rather than written down:
cdk.tf.jsonandmoved.tfandremoved.tf, across four stack shapes. That last pair is the point: a renamed Terraform logical id destroys and recreates live AWS resources, and no unit test observes it. Green tests would have shipped it.opt-outtag, deliberately non-positional CloudWatch alarm ids — read as cruft to anyone who wasn't there, so they get cleaned up first.The worktree hook trap
Recorded in
git-workflow.mdbecauseCLAUDE.md§1b makes worktrees the default for non-trivial work.When
core.hooksPathpoints at a gitignored, install-generated directory (husky's.husky/_), that directory exists only where the install ran. Git skips hooks silently when the path doesn't resolve. In the batch above it was present in 2 of 5 worktrees; commits from the other 3 bypassed lint, formatting and a bundle rebuild with no warning at all.Also: over-engineering becomes the sixth review dimension
The YAGNI rule only bit when someone remembered to look for it. This adds it to the review dimensions in
CLAUDE.md, which feed the plan-review gate, the post-implementation review, the §1c local loop and the pre-commit loop — so pruning happens on every change rather than as an occasional cleanup pass.It is flagged as an adversarial check deliberately. The author's local justification for a piece of machinery almost always holds up on its own terms; the reviewer's job is to ask what the calling system actually does and what would break if the machinery were deleted.
The five-dimension references in
git-workflow.mdand the two autopilot prompts are updated to match.Scope
25 added / 9 changed lines across six files. Deliberately terse — a section arguing for restraint should show some.
Closes #45