fix(hook): scope the doctor gate in verify:adf repos

[stackbilt-admin]

Refs #357

Bug

#360 made the pre-commit hook's doctor gate commit-scoped (doctor --adf-only --ci --staged --tidy-skipped "$TIDY_SKIPPED"), but only in the else branch. When package.json has a verify:adf script, the hook ran pnpm run verify:adf instead. charter setup and charter bootstrap write that script as charter doctor --adf-only --ci --format json && charter adf evidence --auto-measure --ci --format json. So in every bootstrapped Node repo the scoped gate never ran, and commits were still blocked by bloat in unstaged or unrelated vendor files (the #356 regression that #357 was meant to fix).

A second bug: the hook called pnpm by name, so the gate failed in npm and yarn repos on machines without pnpm.

Design

• The scoped doctor always runs first. It is the same command as fix(hook): doctor gate only blocks on bloat in files the commit includes #360's else branch, now outside the branch. If it fails, the gate stops there.
  • Behaviour change: in a repo with a verify:adf script, the hook now runs npx charter doctor --adf-only --ci --staged … on every commit, even when a custom verify:adf leaves doctor out. The CHANGELOG and docs/cli-reference.md say so too. (setup also rewrites a custom verify:adf back to the default, so customisations were already fragile.)
• Then verify:adf if the script exists, otherwise adf evidence. The verify:adf script runs unchanged, so users' custom commands in it still run and still block.
• Package manager: pnpm when there is a pnpm-lock.yaml and command -v pnpm succeeds; yarn when there is a yarn.lock and yarn is installed; npm otherwise. npm ships with node, and the hook's npx needs it anyway, so pnpm is never required. The lockfile check keeps pnpm repos on pnpm (and their .npmrc settings). The failure hint names the runner the hook used.
  • Yarn PnP is not supported yet. The gate's first step, like the tidy block, is npx charter, which cannot find a CLI installed under PnP (there is no node_modules/.bin). So the gate fails before it reaches yarn run verify:adf. That was already true before this PR, and hook: pre-commit gate fails under Yarn Plug'n'Play (npx charter can't resolve the PnP binary) #366 tracks it.
• No duplicate bloat check inside verify:adf: for that run only, the hook sets CHARTER_HOOK_SCOPED_DOCTOR. When it is honoured, the only effect is that doctor's adf vendor bloat check becomes an INFO row, adf vendor bloat (checked by pre-commit hook), which still lists the bloat it found. Doctor also prints a line on stderr: [doctor] adf vendor bloat: not enforced in this run; .... Every other doctor check still blocks. --staged takes precedence over the variable.

CI safety: option (b), a one-time token

The hook makes a token $$-$(date +%s) and writes it to its own file, $(git rev-parse --git-path charter-hook-scoped-doctor-<token>), one file per hook run. It puts the same value in CHARTER_HOOK_SCOPED_DOCTOR. A trap on EXIT, HUP, INT and TERM removes only that run's file.

Doctor accepts the variable only if:

• it has the token form <digits>-<digits>, so it is never turned into an arbitrary path;
• charter-hook-scoped-doctor-<token> exists;
• that file holds exactly the token.

When the variable is unset, doctor does not call git at all.

Overlapping commits: two hooks running at once in one worktree each have their own file, so neither deletes or overwrites the other's. Git's index lock does not serialise hooks, and the review reproduced the overlap. This is tested with two overlapping git hook run pre-commit runs: both relax, and each sees the right files.

Write failure: both the token-file write ({ printf … > "$SCOPE_FILE"; } 2>/dev/null) and the git rev-parse --git-path step (|| SCOPE_FILE="") fail quietly. In either case the hook prints one line, [pre-commit] Could not write the scope token in the git dir, so this time the doctor inside verify:adf checks vendor bloat in every file, as CI does., then carries on with verify:adf at full strictness. There is no raw shell error, and no abort under set -e.

Why this option:

• (a), the git hook environment. I checked what git exports to a pre-commit hook: GIT_INDEX_FILE=.git/index, GIT_EDITOR=:, GIT_PREFIX and the author variables. Those are ordinary environment variables that a CI job or wrapper can also carry, so they are weak evidence that a hook is running.
• (c), ignore the variable when CI=true. This depends on every CI system setting CI, and it would silently change local behaviour too.
• (b) chosen. A CI misconfiguration that sets the variable has no matching file in a fresh checkout, so it does nothing, and doctor --ci stays byte-identical to main (tested). A stale file left by a SIGKILLed hook holds an old token, so it does not match either. The token is not a secret: the threat is accidental weakening, not an attacker, since anyone who controls the environment can skip the hook anyway. (c)'s loud notice is included as well.
• Fail-safe: if the token file cannot be written, the variable is not set, a notice is printed, and verify:adf runs at full strictness.

The constants and hookScopeFileName() live in a new import-free module, hook-scope.ts. Importing them into doctor from hook.ts hit a circular-import ordering under vitest that left the message reading undefined.

Upgrade path

There is a fourth marker, # Charter pre-commit evidence gate: scoped in verify:adf repos too, run with the repo package manager. A #360-era hook has the first three markers but not this one. Doctor's pre-commit hook INFO row fires for such a hook only when package.json has a verify:adf script, using the same "verify:adf" test the hook applies at the repo root. Without the script, a #360 hook already runs the scoped doctor and then adf evidence, exactly as the new hook does, so there is nothing to fix and no row. This way the row never carries the pnpm wording where it does not apply. Existing installs keep the old text until charter hook install --pre-commit is re-run.

This repo's own .githooks/pre-commit needs no change: its gate runs only adf evidence, never doctor or verify:adf. The tidy block is unchanged, and the parity test still passes.

E2E: main (48cda68) vs PR

Each scenario ran in a fresh mktemp -d repo under the scratchpad (set -euo pipefail; GIT_DIR, GIT_INDEX_FILE and GIT_WORK_TREE unset; git rev-parse --show-toplevel checked before any write). The hook was installed with each build's hook install --pre-commit, and npx/charter PATH shims pointed at that build. The verify:adf flavor uses setup's exact verify:adf plus a package-lock.json.

The last column compares plain doctor on the pre-commit state, with each of "", --ci and --adf-only --ci, in text and json. Each combination compares the main build, the PR build, and the PR build with CHARTER_HOOK_SCOPED_DOCTOR=1 set: 204 comparisons, all byte-identical. No token file was left behind in any run.

Scenario	no verify:adf: main	PR	verify:adf: main	PR	plain doctor identical
S0 staged bloated CLAUDE.md	pass	pass	pass	pass	yes
S5 planned module fully staged	pass	pass	pass	pass	yes
UN unborn repo, first commit	pass	pass	pass	pass	yes
CR core.autocrlf=true, CRLF	pass	pass	pass	pass	yes
N no vendor staged, GEMINI.md bloat	pass	pass	block	pass	yes
S3 git rm recorded CLAUDE.md	block	block	block	block	yes
S4 git mv CLAUDE.md → AGENTS.md	block	block	block	block	yes
B clean staged CLAUDE.md, unstaged GEMINI.md bloat	pass	pass	block	pass	yes
B2 as B, CLAUDE.md bloated	pass	pass	block	pass	yes
C partially staged (CP-3)	pass	pass	block	pass	yes
D planned-module WIP (CP-4)	pass	pass	block	pass	yes
D3 untracked planned module (CP-4)	pass	pass	block	pass	yes
S1 .ai/ gitignored	pass	pass	pass	pass	yes
S6 ignored module created by tidy	pass	pass	pass	pass	yes
X1 staged bloat tidy cannot move	block	block	block	block	yes
X2 malformed VENDORS + CP-3	block	block	block	block	yes
X3 staged bloat, CHARTER_SKIP_TIDY=1	block	block	block	block	yes
NPM-B B with no pnpm on the machine	-	-	block (pnpm missing)	pass (npm)	-
NPM-X1 X1 with no pnpm	-	-	block	block	-

Tests

• precommit-hook.test.ts: the whole hook: doctor gate blocks commits on bloat in files that aren't part of the commit #357 full-hook group now runs in both flavors: without verify:adf, and with verify:adf plus an npm lockfile and a failing pnpm shim on PATH. In the verify flavor, each passing case also asserts that verify:adf ran via npm (never pnpm), that its doctor reported the INFO row and the stderr line, and that the token file is gone. Each blocking case asserts that nothing ran after the scoped doctor. New verify-only tests:
  • a failing custom command in verify:adf blocks;
  • a non-bloat WARN that only the doctor inside verify:adf sees (missing .charter/config.json) blocks;
  • pnpm-lock.yaml with pnpm on PATH uses pnpm;
  • running npm run verify:adf by hand with CHARTER_HOOK_SCOPED_DOCTOR=1 still WARNs on bloat;
  • the token file cannot be written (--git-path answers with a nonexistent directory), and git rev-parse --git-path fails: each prints the one notice, shows no raw shell error, carries on into verify:adf, runs it strict (the unstaged bloat blocks) and leaves no file. These run the hook directly under sh with a git shim first on PATH, because git puts its own dirs ahead of PATH for hooks;
  • two overlapping hook runs in one worktree: the first run's verify:adf sleeps 8s, and the second starts once the first's file exists. The second sees 2 scope files, finishes, and removes only its own. The first still relaxes. Both exit 0, and no file is left.
• doctor-staged-bloat.test.ts:
  • each of these gives checks equal to the unset run, WARN, exit 1 and no stderr:
    • the variable set to 1, or to the right token, with no file;
    • only another run's file;
    • its own file holding a different token;
    • only the old shared file name;
    • a path-like value (../x) whose target holds it;
    • the file alone;
  • a matching token gives INFO, the stderr line, and --ci passes;
  • two runs' files side by side: each token relaxes with its own file, and removing one file affects only that token;
  • with a matching token, malformed VENDORS and a missing default module still fail;
  • --staged takes precedence.
• hook.test.ts:
  • a fix(hook): doctor gate only blocks on bloat in files the commit includes #360-era hook in a repo with verify:adf gets the new INFO row, and reinstalling clears it. With no package.json, or one without verify:adf, it gets no row;
  • the gate text is pinned: the scoped doctor sits before and outside the verify:adf branch, "$PM" run, no pnpm run, the per-run --git-path with || SCOPE_FILE="", the braced quiet write, the notice, the trap lines and 5 npx charter;
  • parity, sh -n, dash -n and bash --posix -n all pass. Git for Windows ships /usr/bin/dash, so the INV-P4-7 dash probe ran here too. (An earlier version of this description wrongly said it was skipped.)
• Mutation checks, second round (all killed):
  • a shared token file name in the hook fails the overlap test;
  • a shared name in doctor fails 4;
  • no token-form check fails the ../x case;
  • the unbraced redirect (shell error leaks) fails 1;
  • rev-parse without || SCOPE_FILE="" (aborts under set -e) fails 1;
  • no notice fails 2;
  • the fix(hook): doctor gate only blocks on bloat in files the commit includes #360 row regardless of verify:adf fails 2.
• Mutation checks, first round (all killed):
  • dropping the token export fails 9 tests;
  • hard-coded pnpm run fails 9;
  • always-npm fails 1;
  • running the scoped doctor only without package.json fails 10;
  • honouring the variable without the file fails 3;
  • no INFO row for fix(hook): doctor gate only blocks on bloat in files the commit includes #360 hooks fails 1;
  • no stderr notice fails 1.
• Re-run after the review fixes (main 48cda68 vs 1384073): verify-flavor B, N and C still go block → pass, X1 still blocks, NPM-B still goes block → pass, and NPM-X1 still blocks. Plain doctor, with and without CHARTER_HOOK_SCOPED_DOCTOR=1, is byte-identical to main in every comparison, and no token file is left.
• Full suite: 1446 passed. The 3 failures are the known Windows-only ones (bootstrap bootstrap: phases 6/7 collect warnings without printing them; absoluteAiDirArg misses Windows paths on POSIX #307 fixture, context-refresh "captures git signals", serve-context "returns existing snapshot"). Typecheck and pnpm run verify:adf pass.

Upgrade note

Re-run charter hook install --pre-commit. Until you do, charter doctor reports a #360-era hook with an INFO pre-commit hook row. With an older local CLI, the new hook's flags and variable are ignored and the gate behaves as before.

🤖 Generated with Claude Code

[stackbilt-admin]

Adversarial review: PR #365 (head 1dcdcd9)

Verdict: changes requested. The fix works. One user-facing doc is still wrong, and there are some smaller points to look at.

What I checked and confirmed

• CI is green: build-and-test, Charter Governance, CodeQL, Dependency Review and SBOM all pass.
• I ran the suite locally in a fresh worktree (pnpm install && pnpm build && pnpm test): 1437 pass and 3 fail. The 3 are the known Windows-only failures (bootstrap #307 fixture, context-refresh "captures git signals", serve-context "returns existing snapshot").
• I ran each scenario in a fresh mktemp -d repo made with the PR build's bootstrap and hook install --pre-commit. node_modules/.bin/charter pointed at the PR build, and the repo had an npm lockfile.
  • B/N, unstaged GEMINI.md bloat with notes.md staged: the commit passes. The scoped doctor shows the bloat as INFO. npm run verify:adf ran, and its doctor printed adf vendor bloat (checked by pre-commit hook) and the stderr line. No token file was left behind.
  • Variable set, no file (CHARTER_HOOK_SCOPED_DOCTOR=1 charter doctor --adf-only --ci): WARN, rc=1. The relaxation does not apply.
  • verify:adf with a failing extra command (&& node -e "process.exit(3)"): the commit fails with rc=1 and the npm run verify:adf hint. The same happens when the hook runs under dash. The EXIT trap does not change the exit status. I also checked separately that exit 7 with the same traps under dash still returns 7.
  • Linked worktree at a path with spaces: --git-path resolves to .git/worktrees/<name>/charter-hook-scoped-doctor, a separate file for each worktree. Commits running at the same time in the main worktree and the linked one both pass, and each uses its own token.
  • Old local CLI (published 1.9.3): given --staged --tidy-skipped "" and --tidy-skipped " GEMINI.md", it ignores them and runs a normal doctor, as the PR says.
  • dash -n on the generated hook passes.
• Doctor makes no git call when the variable is unset or empty (if (!token) return false comes before runGit), and --staged takes precedence. Every non-bloat check is still pushed as before, because only the bloat branch changed.

Findings

1. [must fix] packages/cli/README.md:303 is stale. It still says the hook "validates ceilings with charter adf evidence --auto-measure --ci (or pnpm run verify:adf when that script exists)". This PR stops the hook from calling pnpm by name and always runs the scoped doctor first. This README is the npm package page, and the PR updated docs/cli-reference.md and the CHANGELOG but not this file. The upgrade sentence at the end of that paragraph also does not mention the #360-era hook.

2. [low] The #360-era INFO row also appears in repos with no verify:adf, and there its wording is wrong. doctor-gate-enforcement.ts:71 fires DOCTOR_VERIFY_ADF_GATE_MSG for any hook that has the first three markers but not the fourth. It does not look at package.json. In a repo with no verify:adf script, the #360 hook was already scoped. The row then tells the user their hook "runs pnpm run verify:adf instead … which fails where pnpm is not installed", which is not true for that repo. It is only INFO, but it is misleading. Either check for the script, or word the message as conditional.

3. [low] When the token cannot be written, the fail-safe prints a raw shell error and gives no reason for the strict run. At hook.ts:279 the line is printf … > "$SCOPE_FILE" 2>/dev/null. Redirections apply left to right, so if the > fails, the shell reports it before stderr is redirected. I reproduced this with dash: if printf '%s\n' x > /nonexistent/d/f 2>/dev/null; … prints cannot create /nonexistent/d/f: Directory nonexistent. verify:adf then runs at full strictness, so bloat in an unstaged file blocks the commit, and nothing says the relaxation was skipped. The fix is { printf … > "$SCOPE_FILE"; } 2>/dev/null plus a one-line notice. Separately, SCOPE_FILE=$(git rev-parse …) under set -e would end the hook with no [pre-commit] message if git failed there. That is unlikely, since the hook has already run git several times by then.

4. [low] Two commits in the same worktree at once share one token file, and the PR does not mention this. The PR's reasoning assumes git's index lock serialises hooks. It does not: my probe saw no index.lock during the hook, and two git commits in the same worktree ran their hooks at the same time. Whichever hook finishes first runs its EXIT trap, which deletes the other hook's file, or the second write overwrites the first token. Either way the other doctor falls back to strict, so the gate never relaxes when it should not (it fails closed). The cost is a possible spurious block. A per-hook name such as --git-path charter-hook-scoped-doctor.$$ would avoid it, and doctor could take the path from a second variable.

5. [low, partly pre-existing] The Yarn PnP claim in the PR body is overstated. The body says the lockfile check keeps Yarn PnP on its own tool. But the gate's first step is now npx charter doctor … --staged, which also applies in verify:adf repos. Under PnP there is no node_modules/.bin, so npx cannot find the locally installed CLI. It falls back to $PATH, and then to the npm registry package named charter. That package is an unrelated 2022 package with no bin, so the gate fails before yarn run verify:adf is reached. I did not run this (it needs yarn berry and a network install). The same npx charter problem exists in the tidy block and in the old else branch.

6. [low] Behaviour change in verify:adf repos is not called out. The hook now always runs npx charter doctor --adf-only --ci --staged, even when a user's verify:adf leaves doctor out. The PR body's "users' custom commands in it still run" is true but incomplete. Also, setup.ts:1091 rewrites a custom verify:adf back to the default, so customisations are fragile anyway.

7. [nit] The PR body is wrong about dash. It says dash is not installed on this host, so the INV-P4-7 probe skips. In fact Git for Windows ships /usr/bin/dash, and node's spawnSync('dash') finds it here, so the dash parse test ran and passed. That is good news for the PR; only the body needs correcting.

Not verified

• Yarn berry/PnP and yarn classic end to end (yarn here is only a corepack shim).
• pnpm-lock with pnpm missing inside a real pnpm workspace with .npmrc settings. Reasoning: npm run adds node_modules/.bin to PATH, and pnpm puts the charter shim there, so it should work.
• A read-only .git with a full commit. Only the redirection behaviour was reproduced.
• SIGKILL leaving a stale file (checked by reading the code only: the token is pid plus epoch second, so a stale file cannot match a new run).
• Submodules (by reasoning: --git-path resolves to the submodule's own git dir).