Repository navigation
feat(hook): pre-commit tidies only staged vendor files (#318 4/6) - #356
Conversation
…what it wrote (closes #320) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
stackbilt-admin
left a comment
stackbilt-admin
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED
Adversarial review of #356 (PR 4/6 of #318, closes #320) at 33c0d1d. I built it on Windows and compared it with a build of main @ 01d2834. Each scenario ran in a fresh scratch repo through the hook that hook install --pre-commit generates, with an npx shim pointing at the build under test. The tidy core is solid. The golden fixture matches a real main build byte for byte, --print-paths stdout is clean, the upgrade path works and the mutation I tried was caught. Three problems in the hook's failure handling and messages need fixing before merge.
Required changes
-
git addunderset -ecan abort the hook, so the tidy block can fail a commit. This is atpackages/cli/src/commands/hook.ts:178and.githooks/pre-commit:49.- Reproduced (S6):
.ai/backend.adfis gitignored and absent, and a bloated CLAUDE.md is staged. Tidy creates the module, thengit add -- .ai/backend.adfexits 1 ("paths are ignored").set -eukills the hook with git's raw advice text and no[pre-commit]line. - State left behind: the working-tree CLAUDE.md has already been rewritten and re-staged as the pointer, and the module is unstaged. Main passes this case, because its
git add .ai/*.adf 2>/dev/null || truehides the failure. - Contradiction: the CHANGELOG says "The tidy block never fails a commit". The generated hook still runs
set -eu, and the repo hook runsset -euo pipefail. - Fix:
git add -- "$f" || echo "[pre-commit] could not stage $f; ...". Add an integration test with an ignored module path.
- Reproduced (S6):
-
Ignored module paths are reported as "unstaged work", which newly blocks a local-only
.ai/setup. This is athook.ts:166and.githooks/pre-commit:37.- Reproduced (S1):
.ai/is in.gitignore, which is a valid local-only Charter setup. A bloated CLAUDE.md is staged. - What happens:
[ -e "$p" ] && ! git ls-files --error-unmatchtreats the ignored module as untracked WIP. The hook prints "tidy would write into files with unstaged work: .ai/backend.adf", tidy is skipped, and doctor's bloat check then fails the commit. - No way out: the user cannot "stage or stash" an ignored file. Main passes this case.
- Fix: exclude ignored paths from the WIP guard (
git check-ignore -q -- "$p"). Do not try to stage them either, and print one line saying the module is ignored and was not staged.
- Reproduced (S1):
-
Divergence between the dry run and the real run is silent, and the success line is then wrong. This is at
hook.ts:176-181and.githooks/pre-commit:47-52.- Reproduced (DV): I used a shim whose real run also writes and reports
module .ai/extra.adf. The hook correctly does not stage it. It still prints "Vendor files tidied; re-staged only the files tidy wrote." and the commit succeeds with?? .ai/extra.adfleft behind and no mention of it. - Second gap: a path that appears only in the real run never went through the WIP guard. Tidy may therefore have written into a file that has unstaged work.
- Why it matters: CP-1 makes staging the intersection, which is correct. If the vendor pointer is staged but a module it now relies on is not, the commit drops content.
- Fix: collect the paths in WROTE that are not in PLAN and print, for example,
[pre-commit] tidy also wrote (left unstaged, review before committing): <paths>. Reword the success line to match, and add a shim-based test.
- Reproduced (DV): I used a shim whose real run also writes and reports
-
Messages and tests around CP-3/CP-4 must match what the default generated hook actually does. See the item-4 analysis below; the operator still has to pick a direction.
- Hook messages: at
hook.ts:152andhook.ts:171, the skip messages read as harmless ("not auto-tidying it"). In the default hook (noverify:adfscript) the commit is then refused by the evidence gate (scenarios C, D, D3). - Minimum fix: whichever option is chosen, the skip message must say the gate will block the commit while the file holds bloat, and give the ways out: stage or stash and retry, run the
adf tidy --sourcecommand andgit addthe result, orgit commit --no-verify. - README:
packages/cli/README.md:302says the hook "skips the whole commit". It means it skips tidy for that commit. - Tests: the CP-4 tests at
precommit-hook.test.ts:292and:315assert "the commit still succeeds" using thetidyOnlyharness, which removes the gate. Either rename them ("the tidy block does not fail the commit") or add full-hook counterparts to the existing H6-boundary test that pin the real exit 1 for D and D3.
- Hook messages: at
Item 4 — the evidence gate after a scoped tidy
When there is no verify:adf script, the generated hook runs charter doctor --adf-only --ci after tidy. Doctor's adf vendor bloat check scans the working tree: more than 80 lines, H2 overlap with ADF section keys, or trigger-keyword density. Main got past this by tidying every vendor file in the working tree, which is the bug #320 fixes. PR4 tidies only staged files, so the gate now sees bloat that main used to clear.
Results, PR build vs main build, generated hook:
| Scenario | main | PR4 | Message the user gets |
|---|---|---|---|
| S0 staged CLAUDE.md bloat | pass | pass | — |
| S5 planned module fully staged | pass | pass (not wrongly skipped) | — |
| UN unborn repo, first commit | pass | pass | — |
CR core.autocrlf=true, CRLF file |
pass | pass | — |
| N no vendor file staged, GEMINI.md WT bloat | block | block | unchanged |
S3/S4 git rm or git mv CLAUDE.md |
block | block | doctor vendor mode (PR3), unchanged |
| B staged clean CLAUDE.md edit, unstaged GEMINI.md bloat | pass (silently rewrote GEMINI.md) | block | doctor names GEMINI.md, then "Run: charter adf tidy --dry-run" |
| B2 as B, with staged CLAUDE.md bloat | pass | block | same; tidied CLAUDE.md and .ai/backend.adf are left staged |
| C partially staged CLAUDE.md (CP-3) | pass (committed the unstaged hunk) | block | "not auto-tidying it… or run: charter adf tidy --source CLAUDE.md", then FAILED |
| D planned module has unstaged WIP (CP-4) | pass (swept the WIP) | block | "Not auto-tidying…: .ai/backend.adf", then FAILED |
| D3 planned module untracked (CP-4) | pass (swept it) | block | same |
S1 .ai/ gitignored |
pass | block | wrong: "unstaged work" (required change 2) |
| S6 ignored module created by tidy | pass | block | raw git error (required change 1) |
Of 13 scenarios, PR4 newly blocks 7 that main passed. S1 and S6 are bugs, covered by required changes 1–2. The other 5 (B, B2, C, D, D3) are blocks that main avoided only by doing what #320 forbids. Repos that do have a verify:adf script, and this repo's own hook (adf evidence only), are unaffected (the impl's D2 run passes).
In every case the messages are accurate but don't help much. The user sees a benign-sounding skip line, then a 15-line doctor dump that ends in "ADF verification FAILED". Nothing says that the skip caused the failure, or that git commit --no-verify is the escape hatch. For B, doctor's own advice, a whole-tree charter adf tidy --dry-run, would also rewrite files the user did not stage.
So CP-3/CP-4 "skip, non-blocking" holds for the tidy block only. In practice the default hook blocks those commits. The CHANGELOG and cli-reference do say so ("still fails on vendor bloat left in the working tree"), so the documentation is accurate. But the operator signed off on "non-blocking", so this needs an explicit decision.
Options, cheapest first:
- A. Accept the block and fix the messages (required change 4 minimum). The hook stays as it is. The skip lines say the gate will block and give the three ways out. Cost: a few echo lines. CP-3/CP-4 are re-signed as "skip, and blocked by the gate, with an explanation".
- B. Limit doctor's bloat scan in hook context to the staged vendor files. For example, pass
--vendor-files "$NAMES"or an env var such asCHARTER_HOOK_STAGED. This fixes B and B2, and also N, which improves on main. It does not fix C, D or D3, because those staged files are bloated themselves. Cost: a new doctor flag, plus a test that hook and doctor agree on the list. - C. Report
adf vendor bloatas a warning, not a failure, in hook context for the files the tidy block skipped, with every other doctor check still blocking. This makes CP-3/CP-4 actually non-blocking, as signed off. Cost: bloat can reach a commit in C, D and D3 (main already committed it in C, along with the WIP), anddoctor --ciin CI still catches it. Needs a hook-to-doctor channel, like B. - D. Make doctor read the staged blob for staged vendor files and ignore files that are only in the working tree. These are exact "what is being committed" semantics and give the best results, but it is the most work and touches the index reading that PR4 explicitly left out.
Recommendation: A in this PR, and B + C as one follow-up issue (they share the hook-to-doctor channel). D only if the index reading needs to happen anyway.
Verified OK
- Shell quoting (both copies): vendor names come from a fixed list, and every git pathspec is
-- "$x".grep -xFagainst--name-onlyoutput means a quoted name such as"a\nb"cannot match.--diff-filter=dexcludes index deletions, sogit rm --cached CLAUDE.mdis not resurrected; main did resurrect it (S2). Using>/dev/nullinstead of-qon piped greps is correct under pipefail. Commands that can fail all sit insideif,||or$(…) ||, apart from thegit addin required change 1. A repo path containing a space works (impl H and test). A file namedrelease notes.mdalongside a vendor file works. - CRLF:
--print-pathsemits\nonly on Windows (checked withod -c), so$(…)leaves no\r. - No new
npxcalls. Two calls were replaced with two, same as main. - INV-P4-1:
- A planned module that is fully staged is tidied and committed, not wrongly skipped (S5).
- A tracked planned module deleted only in the working tree, or an untracked one, is guarded (D3 and the tests).
git rm CLAUDE.mdis not resurrected (S3).- A rename onto a vendor name is tidied under its new name (S4).
- Item 5:
- Golden fixture: I wrote my own runner and checked the fixture against a real main build: all 7 cases match on exit code, stdout, stderr and files. The PR build also matches once
pathsis removed. - Exit 2 cases: a trailing
--sourceexits 2,--source --print-pathsexits 2, and--print-paths --format jsonexits 2. In each case stdout is empty and stderr gets one line. - De-dup:
--source CLAUDE.md --source CLAUDE.md --source ./CLAUDE.mdyields one row.AGENTS.mdandagents.mdon NTFS collapse into one row. - Warnings and exit codes: warnings go to stderr.
--ciwith bloat still exits 1.
- Golden fixture: I wrote my own runner and checked the fixture against a real main build: all 7 cases match on exit code, stdout, stderr and files. The PR build also matches once
- Item 6:
- A main-generated hook under
core.hooksPathgives one INFO row, anddoctor --ci --adf-onlystill exits 0. - Reinstalling without
--forcereplaces it and the row disappears. A foreign or missing hook gives no row. - The whole-block parity test is strong: the generated block, the repo block and the template are compared exactly.
- Mutation: removing the untracked branch of the WIP guard from the template makes the "CP-4: an untracked file at a planned module path" test fail. The mutation was reverted afterwards.
- A main-generated hook under
- Item 7: the precommit, hook and tidy-sources suites pass locally (75/75). On main, the two precommit tests fail on Windows (2 of 5), and both now exercise real tidy (they assert the CLI calls and the committed pointer).
- Item 8: in a fresh clone of the PR branch, built under the scratch dir with
core.hooksPath=.githooks, a staged bloated CLAUDE.md commits as the pointer plus.ai/backend.adf. UnrelatedM .ai/core.adfWIP stays unstaged, and the untracked.ai/context.snapshot.jsonand.ai/context.adfare never staged.
Non-blocking notes
- The unquoted
for p in $(…)andfor line in $(…)loops are subject to globbing and word splitting. Tidy cannot currently produce module names with spaces or glob characters (my spaced-module attempt routed tobackend.adf), butset -faround the loops would harden them cheaply. - Version skew fails safe but quietly: with a new hook and an older local CLI,
--print-pathsis ignored, so PLAN contains no typed lines and the hook prints "Vendor config files are clean." Worth noting in the CHANGELOG reinstall paragraph. --source=CLAUDE.md(equals form) is silently ignored and a whole-tree tidy runs. This is not new (main'sgetFlagbehaves the same), butgetFlagValuescould reject it with exit 2 now..github/copilot-instructions.md, the usual Copilot location, is not inVENDOR_FILES. Not new.adf.ts:956: the tidy help line is now indented with 4 spaces instead of 2.- Tidy still creates modules that are not registered, such as
.ai/backend.adfin this repo's own clone test. That gap is known and out of scope. npx charterwith a non-TTY stdin auto-installs whatever package is published undercharterwhen@stackbilt/cliis not local. This is not new to PR4, but it deserves an issue.
Scripts and transcripts are in the reviewer's scratch dir rv356b/ (lib.sh, s1.sh–s4.sh, cli.sh, up.sh, golden.cjs, clone.sh).
…ks (refs #320) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…des (#360) Since #356 the pre-commit hook tidies only staged vendor files, but its doctor gate still scanned the whole working tree for bloat, so a commit could be blocked by an unrelated unstaged file, or by a file the hook had deliberately skipped (partially staged, or a planned module with work in progress); the old hook had only avoided that by committing changes nobody staged. The generated gate now runs doctor with --staged and --tidy-skipped: only bloat in files the commit includes, and that tidy did not skip, blocks; the rest is reported as INFO. Every other doctor check still blocks, plain doctor and CI are byte-identical to before, and an unreadable index falls back to checking everything. #356-era hooks get a doctor INFO row asking for `charter hook install --pre-commit`. Closes #357 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Closes #320
Refs #318
Summary
PR 4 of 6 in the #318 sequence, option 2 of #320. The pre-commit hook now tidies only the staged vendor files and re-stages only the files tidy wrote. The
git add .ai/*.adfsweep is gone.adf tidy--sourceis repeatable. Values are processed in argument order, and two names for one file are de-duplicated byfileIdentity(a case-insensitive alias or a hard link).--sourcewith no value is a usage error (exit 2). It used to fall back to tidying the whole tree silently.--print-pathsprints only typedvendor <path>/module <path>lines on stdout. Warnings go to stderr. Combining it with--format jsonexits 2.pathsfield.hook.tsgenerator and.githooks/pre-commit)HOOK_TIDY_BLOCK_TEMPLATE.{CLI}isnpx charterin the generator andnode packages/cli/dist/bin.jsin the repo copy.PRE_COMMIT_SCOPE_MARKER.--sourceper staged vendor file. Only paths reported by both runs are staged.doctor: a new INFOpre-commit hookrow appears only for an old Charter-managed hook, meaning one withPRE_COMMIT_HOOK_MARKERand without the scope marker. It honourscore.hooksPath.adf.ts: the tidy help line is edited in place (0 net lines, 1027/1041).adf-migrate.tsandindex.tsare untouched.Before → after (newly installed hook)
git addof staged vendor files and all of.ai/*.adf.ai/*.adfWIPagents.md.ai/)totalExtracted= 0)Operator sign-offs applied
git add .ai/*.adf.agents.mdis in the hook's vendor list.^(vendor|module), so a restore-only rewrite of an already-staged vendor file is staged.totalExtractedparity pin is replaced by pins on the scope marker,--print-paths, and the absence ofgit add .ai/*.Deviations from the addendum's hook text (each needed to keep a stated invariant)
grep '^module '. This PR gates on^(vendor|module)so that restore-only rewrites are staged.[ -e "$p" ] && …misses this case, and tidy would then recreate the file and stage it, violating INV-P4-1. The guard is now! git diff --quiet -- "$p" || { [ -e "$p" ] && ! tracked; }. The guard is tested.grep -xFinstead ofgrep -qxfor the staged-name match..cursorruleswould otherwise match as a regex.>/dev/nullinstead of-q. With-q, grep exits early, which can SIGPIPE the writer under the repo hook'sset -o pipefailand turn a match into a miss.Upgrade path
charter hook install --pre-commitalready overwrites a Charter-managed hook without--force. Re-running it is the upgrade, and install semantics are unchanged. The CHANGELOG tells users to do this.Verified end to end:
[info] pre-commit hook..ai/state.adfWIP into the commit.hook install --pre-commitwith no--force. The hook is overwritten.Note: the evidence gate after the block is unchanged
In the generated hook (repos without a
verify:adfscript), the existing gatecharter doctor --adf-only --cifails on vendor bloat left in the working tree. So when tidy is skipped (CP-3, CP-4 orCHARTER_SKIP_TIDY), or when an unstaged vendor file is bloated, the commit is still blocked, by that pre-existing gate rather than by tidy. The hook's messages say to stage or stash and commit again. A repo withverify:adf, including this one, proceeds. This is pinned in a test ("H6 boundary") and listed in the CHANGELOG. Changing the gate is out of scope here.Invariants and where they are tested
precommit-hook.test.tsprecommit-hook.test.ts"never tidies a vendor file that is not staged", andadf-tidy-sources.test.ts"leaves an unnamed vendor file alone".precommit-hook.test.ts, the two INV-P4-3 cases. A logging shim records every CLI call.hook.test.ts"the generated and repo blocks are the template, differing only in {CLI}".--sourceis byte-identical to main):adf-tidy-sources.test.ts. It compares text, JSON and the resulting files against output captured from main @ 01d2834 (fixtures/tidy-single-source.main-01d2834.json).pathsremoved, plus the--print-paths/pathsequality test.hook.test.tsrunssh -n,dash -nandbash --posix -non both hooks, skipping any shell that is not installed, and has a syntax denylist. The tidy block was also executed under realdash.hook.test.ts"doctor: pre-commit hook row". It covers INFO for an old hook, no row for current, foreign or missing hooks,core.hooksPath, and that--ciis unaffected.adf evidence1027/1041.Windows harness:
precommit-hook.test.tsnow installs the generated hook withnode "<forward-slash path>", behind a logging shim, under a repo path containing spaces. The 2 previously failing tests ("auto-tidies bloated CLAUDE.md", "re-stages modified .adf modules") now pass, and they assert that the real tidy calls ran.NOT in PR4
agents.mdrename and record-key move (PR6).hook installoverwrite semantics.--format paths.Implemented via constellation-loop against a locked blueprint.
🤖 Generated with Claude Code