From 9d1a55378f7cb9c8ef63b1e103b6c680fbc97f92 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Sat, 17 Oct 2026 19:01:58 -0500 Subject: [PATCH 1/4] fix(hook): run every Charter call in the pre-commit hook through the repo package manager (#366) The generated pre-commit hook called a bare `npx charter` for tidy, the scoped doctor gate (#365) and adf evidence. Under Yarn Plug'n'Play npx cannot resolve the installed CLI and falls back to the unrelated registry package `charter`, so the gate failed before `yarn run verify:adf`. The hook now picks the package manager once (PnP markers, then the #365 lockfile + PATH order) and defines charter_cli: `yarn charter` (`yarn --silent charter` on Yarn 1), `pnpm exec charter`, or `npx --no-install charter`, none of which downloads a package. A missing pm binary warns and falls back to npx --no-install / npm run. A new marker lets doctor report a pre-#366 hook as INFO in PnP repos. The CHARTER_HOOK_SCOPED_DOCTOR token flow is unchanged. Tests run the generated hook with stub yarn/pnpm/npx/npm on PATH for yarn PnP, yarn classic, pnpm and npm fixtures, plus missing-pm fallbacks. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + docs/cli-reference.md | 5 +- packages/cli/src/__tests__/hook-pm.test.ts | 234 ++++++++++++++++++ packages/cli/src/__tests__/hook.test.ts | 67 ++++- .../integration/precommit-hook.test.ts | 9 +- .../src/commands/doctor-gate-enforcement.ts | 20 +- packages/cli/src/commands/hook.ts | 107 ++++++-- 7 files changed, 411 insertions(+), 32 deletions(-) create mode 100644 packages/cli/src/__tests__/hook-pm.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index c737e38..152deba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ The format is based on Keep a Changelog and follows Semantic Versioning. ### Fixed +- **The generated pre-commit hook could not run Charter under Yarn Plug'n'Play, and could fetch an unrelated package** (`#366`). Every Charter call in the hook (vendor tidy, the scoped `doctor --staged` gate #365 added, `adf evidence`) was a bare `npx charter`. Under PnP there is no `node_modules/.bin`, so npx missed the installed CLI and fell back to the registry package `charter` (0.0.2, which has no binary), and the gate failed before `yarn run verify:adf` was reached. The same fallback fetched that package in any repo without Charter installed. The hook now picks the package manager once, with the lockfile and `PATH` detection #365 added for `verify:adf` plus Plug'n'Play detection (`.pnp.cjs`, `.pnp.js`, or `.yarnrc.yml` with `nodeLinker: pnp`). It runs Charter as `yarn charter` (`yarn --silent charter` on Yarn 1), `pnpm exec charter` or `npx --no-install charter`, and none of these downloads a package. When the lockfile's package manager is not on `PATH`, the hook warns on stderr (under PnP the warning adds `corepack enable`) and falls back to `npx --no-install charter` and `npm run verify:adf`. A failed gate names the resolved command and says to add `@stackbilt/cli` as a devDependency. The `CHARTER_HOOK_SCOPED_DOCTOR` token flow is unchanged. **Installed hooks do not update themselves:** run `charter hook install --pre-commit` again (no `--force` needed for a Charter-managed hook). This is needed under Yarn PnP and recommended everywhere else. `charter doctor` reports an older hook as INFO in a PnP repo. - **`charter init --guided` with a piped or non-TTY stdin stopped partway through the questions, wrote nothing, and exited 0** (`#375`), so a script or CI job could not tell it from a successful run. There were two causes. First, with piped stdin readline emits every buffered line at once, and a line that arrived while no question was pending was dropped, so even a complete set of piped answers (`printf 'typescript\ny\ny\n' | charter init --guided`) stalled after the second question. Second, a pending `rl.question` callback never fires when stdin closes, so the CLI's promise never settled and Node exited 0 once the event loop drained. Guided mode now queues every input line and gives each question the next one, so piped answers work the same as typed ones and write the same files. If stdin closes before the last answer (a short pipe, `` silently ignored `--preset`** (`#375`). It now seeds the answer: the preset is used when the language answer names no known stack (blank, or something like `cobol`), and the first question says `blank keeps --preset `. A recognized language or a Cloudflare `y` still decides, as before. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 9476005..24cf508 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -78,8 +78,9 @@ npx charter hook install --commit-msg --force # overwrite existing hooks - Only the files tidy wrote that its dry run planned are re-staged, never `.ai/*.adf` as a whole. A gitignored module is written but not staged, and the hook says so. A file the hook cannot stage, or one the real run wrote beyond the plan, is named and left unstaged. Skip tidy with `CHARTER_SKIP_TIDY=1`. - The tidy block never fails the commit: a tidy failure prints one line and the hook carries on. - The evidence gate after it can. In the generated hook it runs `charter doctor --adf-only --ci --staged --tidy-skipped ""` ([#357](https://github.com/Stackbilt-dev/charter/issues/357)). Vendor bloat blocks the commit only in a staged vendor file that the tidy block did not skip. A staged file that tidy ran on but could not clean still blocks, and so does one left untidied because tidy failed or `CHARTER_SKIP_TIDY=1` is set. Bloat in unstaged vendor files, and in files tidy skipped, shows as an INFO row. A planned-module skip (tidy would write into a module with unstaged or untracked work) skips tidy for every staged vendor file in that commit, so it lifts the bloat gate for all of them. Scoping is by file name: the bloat check still reads each vendor file in the working tree, not its staged content. Every other doctor check blocks as usual. CI's plain `charter doctor --ci` still fails on all vendor bloat. The skip message says how to tidy the file: stage or stash the unstaged work and commit again, or run `charter adf tidy --source ` and `git add` the result. - - The scoped doctor runs in every repo, including one whose custom `verify:adf` leaves doctor out: such a repo now gets `npx charter doctor --adf-only --ci --staged …` on every commit. Next, if `package.json` has a `verify:adf` script (`charter setup` and `charter bootstrap` add one), the gate runs it; otherwise it runs `charter adf evidence --auto-measure --ci`. `verify:adf` runs with `pnpm` when the repo has a `pnpm-lock.yaml` and pnpm is installed, with `yarn` for a `yarn.lock` and yarn installed, and with `npm` otherwise, so pnpm is never required. The doctor inside `verify:adf` would check vendor bloat across the whole tree again. To prevent that, the hook sets `CHARTER_HOOK_SCOPED_DOCTOR` to a one-time token for that run and writes the token to its own file in the git directory (`charter-hook-scoped-doctor-`, one per hook run, so overlapping commits in one worktree do not interfere), which it removes on exit. If the file cannot be written, the hook prints one `[pre-commit]` line saying so, and that time the doctor inside `verify:adf` checks every vendor file. When the two match, doctor reports `adf vendor bloat (checked by pre-commit hook)` as INFO and prints a line on stderr. All its other checks still block, and so do the script's other commands. The variable without the matching file does nothing, so `charter doctor --ci` in CI keeps full strictness even if the variable is set there. Run by hand, `npm run verify:adf` checks every vendor file, as CI does. - - A hook installed before [#320](https://github.com/Stackbilt-dev/charter/issues/320) re-stages all of `.ai/*.adf`. A hook installed before #357 blocks on vendor bloat outside the commit, and so, in a repo with a `verify:adf` script, does one from the first #357 fix, which also ran `pnpm run verify:adf` by name. Re-run `charter hook install --pre-commit` to replace any of them (no `--force` needed for a Charter-managed hook). `charter doctor` reports an old hook as INFO. It reports a hook from the first #357 fix only where `package.json` has a `verify:adf` script, since without one that hook already runs the scoped gate. + - The scoped doctor runs in every repo, including one whose custom `verify:adf` leaves doctor out: such a repo now gets `charter doctor --adf-only --ci --staged …` on every commit. Next, if `package.json` has a `verify:adf` script (`charter setup` and `charter bootstrap` add one), the gate runs it; otherwise it runs `charter adf evidence --auto-measure --ci`. `verify:adf` runs with ` run verify:adf`, where `` is the package manager described in the next item. + - Every Charter call in the generated hook (tidy, the scoped doctor, `adf evidence`) runs through the repo's package manager ([#366](https://github.com/Stackbilt-dev/charter/issues/366)), picked once at the top of the hook: Yarn when the repo uses Plug'n'Play (`.pnp.cjs` or `.pnp.js`, or `.yarnrc.yml` with `nodeLinker: pnp`), then pnpm for a `pnpm-lock.yaml`, then Yarn for a `yarn.lock`, and npm otherwise. Charter then runs as `yarn charter` (`yarn --silent charter` on Yarn 1, so its run banner stays out of the output tidy parses), `pnpm exec charter`, or `npx --no-install charter`. All three use a locally installed `@stackbilt/cli`; npx also finds a global install. None of them downloads anything: the npm package named `charter` is unrelated, and a plain `npx charter` would fetch it where Charter is not installed. Install `@stackbilt/cli` as a devDependency. If the CLI cannot be found, the gate fails, naming the command to re-run and that fix. If the lockfile names Yarn or pnpm but that binary is not on `PATH`, the hook warns on stderr and uses `npx --no-install charter` and `npm run verify:adf` instead; under Plug'n'Play, which has no `node_modules`, the warning adds that npx only finds a global Charter there, and to run `corepack enable`. On Windows the hook runs under Git for Windows' `sh`, which finds `yarn`, `pnpm` and `npx` as an extensionless shim (as npm and corepack install them) or an `.exe`. A package manager present only as a `.cmd` file is not found there, so the hook falls back as described. A `package.json` script named `charter` shadows the binary for `yarn charter`. The doctor inside `verify:adf` would check vendor bloat across the whole tree again. To prevent that, the hook sets `CHARTER_HOOK_SCOPED_DOCTOR` to a one-time token for that run and writes the token to its own file in the git directory (`charter-hook-scoped-doctor-`, one per hook run, so overlapping commits in one worktree do not interfere), which it removes on exit. If the file cannot be written, the hook prints one `[pre-commit]` line saying so, and that time the doctor inside `verify:adf` checks every vendor file. When the two match, doctor reports `adf vendor bloat (checked by pre-commit hook)` as INFO and prints a line on stderr. All its other checks still block, and so do the script's other commands. The variable without the matching file does nothing, so `charter doctor --ci` in CI keeps full strictness even if the variable is set there. Run by hand, `npm run verify:adf` checks every vendor file, as CI does. + - A hook installed before [#320](https://github.com/Stackbilt-dev/charter/issues/320) re-stages all of `.ai/*.adf`. A hook installed before #357 blocks on vendor bloat outside the commit, and so, in a repo with a `verify:adf` script, does one from the first #357 fix, which also ran `pnpm run verify:adf` by name. Re-run `charter hook install --pre-commit` to replace any of them (no `--force` needed for a Charter-managed hook). `charter doctor` reports an old hook as INFO. It reports a hook from the first #357 fix only where `package.json` has a `verify:adf` script, since without one that hook already runs the scoped gate. A hook installed before #366 calls a bare `npx charter`: it fails under Yarn Plug'n'Play unless Charter is installed globally, and where Charter is not installed at all it fetches the unrelated `charter` package. Reinstalling is recommended in every repo and needed under Plug'n'Play, where `charter doctor` reports such a hook as INFO. Elsewhere `npx charter` still finds `node_modules/.bin`, so doctor shows no row for it. - `--force` — overwrite existing non-Charter hooks Must specify at least one of `--commit-msg` or `--pre-commit`. diff --git a/packages/cli/src/__tests__/hook-pm.test.ts b/packages/cli/src/__tests__/hook-pm.test.ts new file mode 100644 index 0000000..c808faa --- /dev/null +++ b/packages/cli/src/__tests__/hook-pm.test.ts @@ -0,0 +1,234 @@ +/** + * #366: the generated pre-commit hook runs every Charter call through the repo's + * package manager. These tests run the real hook text with stub yarn / pnpm / + * npx / npm executables first on PATH; each stub records its argv to a log and + * exits 0 (or the code STUB_FAIL_ON names). No network, no real package manager. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync, spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { HOOK_CLI, PRE_COMMIT_HOOK_CONTENT, PRE_COMMIT_PM_CLI_MARKER } from '../commands/hook'; + +const shAvailable = (() => { + const probe = spawnSync('sh', ['-c', 'exit 0']); + return !probe.error && probe.status === 0; +})(); + +const PMS = ['yarn', 'pnpm', 'npx', 'npm'] as const; +type Pm = typeof PMS[number]; + +// Each stub logs "" then each argument in brackets, so an empty argument +// stays visible. yarn answers --version with $STUB_YARN_VERSION. A stub exits 1 +// when its joined argv contains $STUB_FAIL_ON. `npm run` also logs whether the +// scope token reached it. +function stubScript(name: Pm): string { + return `#!/bin/sh +{ + printf '%s' '${name}' + for a in "$@"; do printf ' [%s]' "$a"; done + if [ "${name}" = "npm" ] || [ "${name}" = "yarn" ] || [ "${name}" = "pnpm" ]; then + if [ "\${1:-}" = "run" ] && [ -n "\${CHARTER_HOOK_SCOPED_DOCTOR:-}" ]; then printf ' token-set'; fi + fi + printf '\\n' +} >> "$STUB_LOG" +if [ "${name}" = "yarn" ] && [ "\${1:-}" = "--version" ]; then + echo "\${STUB_YARN_VERSION:-4.5.0}" + exit 0 +fi +if [ -n "\${STUB_FAIL_ON:-}" ]; then + case "$*" in + *"$STUB_FAIL_ON"*) echo "${name}: stub failure" >&2; exit 1 ;; + esac +fi +exit 0 +`; +} + +/** PATH entries that hold none of the package-manager binaries (any Windows extension). */ +function pathWithoutPms(): string { + const exts = ['', '.cmd', '.exe', '.ps1', '.bat']; + return (process.env.PATH ?? '') + .split(path.delimiter) + .filter(dir => dir && !PMS.some(pm => exts.some(ext => fs.existsSync(path.join(dir, pm + ext))))) + .join(path.delimiter); +} + +interface Fixture { + files?: Record; + stubs?: Pm[]; + yarnVersion?: string; + failOn?: string; + /** Use a PATH with every real package manager removed, so only the stubs exist. */ + isolatePath?: boolean; +} + +interface HookRun { + status: number | null; + stdout: string; + stderr: string; + calls: string[]; +} + +describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the package manager (#366)', { timeout: 30_000 }, () => { + let dir: string; + let stubDir: string; + let logFile: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'charter-hook-pm-')); + stubDir = fs.mkdtempSync(path.join(os.tmpdir(), 'charter-hook-pm-bin-')); + logFile = path.join(stubDir, 'calls.log'); + execFileSync('git', ['init', '-q'], { cwd: dir }); + execFileSync('git', ['config', 'core.autocrlf', 'false'], { cwd: dir }); + fs.mkdirSync(path.join(dir, '.ai')); + fs.writeFileSync(path.join(dir, '.ai', 'manifest.adf'), 'ADF: 0.1\n'); + // A staged, hand-written vendor file, so the tidy block calls the CLI too. + fs.writeFileSync(path.join(dir, 'CLAUDE.md'), '# Project\n\nSome notes.\n'); + execFileSync('git', ['add', 'CLAUDE.md'], { cwd: dir }); + fs.writeFileSync(path.join(dir, 'hook.sh'), PRE_COMMIT_HOOK_CONTENT); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + fs.rmSync(stubDir, { recursive: true, force: true }); + }); + + function runHook(fx: Fixture): HookRun { + for (const [name, content] of Object.entries(fx.files ?? {})) { + fs.writeFileSync(path.join(dir, name), content); + } + for (const pm of fx.stubs ?? PMS) { + const stub = path.join(stubDir, pm); + fs.writeFileSync(stub, stubScript(pm)); + fs.chmodSync(stub, 0o755); + } + const basePath = fx.isolatePath ? pathWithoutPms() : (process.env.PATH ?? ''); + const env: NodeJS.ProcessEnv = { + ...process.env, + PATH: `${stubDir}${path.delimiter}${basePath}`, + STUB_LOG: logFile, + STUB_YARN_VERSION: fx.yarnVersion ?? '4.5.0', + STUB_FAIL_ON: fx.failOn ?? '', + }; + delete env.CHARTER_HOOK_SCOPED_DOCTOR; + const result = spawnSync('sh', ['hook.sh'], { cwd: dir, env, encoding: 'utf-8' }); + const calls = fs.existsSync(logFile) + ? fs.readFileSync(logFile, 'utf-8').split('\n').filter(Boolean) + : []; + return { status: result.status, stdout: result.stdout, stderr: result.stderr, calls }; + } + + /** The three CLI calls a manifest repo without verify:adf makes, prefixed by the runner. */ + function expectedCalls(runner: string): string[] { + return [ + `${runner} [adf] [tidy] [--dry-run] [--print-paths] [--source] [CLAUDE.md]`, + `${runner} [doctor] [--adf-only] [--ci] [--staged] [--tidy-skipped] [] [--format] [text]`, + `${runner} [adf] [evidence] [--auto-measure] [--ci]`, + ]; + } + + const withoutVersionProbe = (calls: string[]): string[] => calls.filter(c => c !== 'yarn [--version]'); + + it('the generated hook carries the #366 marker and never calls a bare npx charter', () => { + expect(PRE_COMMIT_HOOK_CONTENT).toContain(PRE_COMMIT_PM_CLI_MARKER); + expect(PRE_COMMIT_HOOK_CONTENT).not.toMatch(/npx charter/); + // Outside echo lines, Charter is only ever reached through charter_cli. + const code = PRE_COMMIT_HOOK_CONTENT.split('\n').filter(l => !/^\s*(#|echo )/.test(l)); + const direct = code.filter(l => /\bcharter (adf|doctor)\b/.test(l) && !l.includes(HOOK_CLI) && !l.includes('GENERATED by charter adf compile')); + expect(direct).toEqual([]); + }); + + it('Yarn Berry PnP (.pnp.cjs): yarn charter for every call', () => { + const run = runHook({ files: { 'yarn.lock': '', '.pnp.cjs': '' } }); + expect(run.status).toBe(0); + expect(run.calls).toContain('yarn [--version]'); + expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + expect(run.stderr).not.toContain('Warning'); + }); + + it('Yarn PnP from .yarnrc.yml alone (nodeLinker: pnp, not installed yet): yarn charter', () => { + const run = runHook({ files: { '.yarnrc.yml': 'nodeLinker: pnp\n' } }); + expect(run.status).toBe(0); + expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + }); + + it('Yarn classic (yarn.lock, yarn 1.x): yarn --silent charter, so no run banner reaches the tidy plan', () => { + const run = runHook({ files: { 'yarn.lock': '' }, yarnVersion: '1.22.22' }); + expect(run.status).toBe(0); + expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [--silent] [charter]')); + }); + + it('pnpm (pnpm-lock.yaml): pnpm exec charter', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': "lockfileVersion: '9.0'\n" } }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + }); + + it.each([ + ['package-lock.json', { 'package-lock.json': '{}' }], + ['no lockfile', {}], + ])('npm (%s): npx --no-install charter, which never downloads a package', (_label, files) => { + const run = runHook({ files }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls('npx [--no-install] [charter]')); + }); + + it.each([ + ['yarn PnP', { 'yarn.lock': '', '.pnp.cjs': '' }, 'yarn [charter]', 'yarn [run] [verify:adf] token-set'], + ['pnpm', { 'pnpm-lock.yaml': '' }, 'pnpm [exec] [charter]', 'pnpm [run] [verify:adf] token-set'], + ['npm', {}, 'npx [--no-install] [charter]', 'npm [run] [verify:adf] token-set'], + ])('verify:adf repo (%s): scoped doctor through the package manager, then run verify:adf with the token', (_label, files, runner, verify) => { + const run = runHook({ files: { ...files, 'package.json': JSON.stringify({ scripts: { 'verify:adf': 'charter doctor --adf-only --ci' } }) } }); + expect(run.status).toBe(0); + expect(withoutVersionProbe(run.calls)).toEqual([...expectedCalls(runner).slice(0, 2), verify]); + }); + + it('a failing doctor blocks the commit and the hint names the resolved command', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': '' }, failOn: 'doctor' }); + expect(run.status).toBe(1); + expect(run.stdout).toContain('[pre-commit] ADF verification FAILED.'); + expect(run.stdout).toContain(' Run: pnpm exec charter doctor --adf-only --ci --staged --format text'); + expect(run.stdout).toContain('add @stackbilt/cli as a devDependency'); + }); + + describe('package manager binary missing from PATH', () => { + // Needs a PATH with git and the POSIX tools but no real package manager. + const isolatedOk = (() => { + if (!shAvailable) return false; + const probe = spawnSync('sh', ['-c', 'command -v git && command -v sed && command -v grep && command -v date && ! command -v yarn && ! command -v pnpm'], { + env: { ...process.env, PATH: pathWithoutPms() }, + }); + return !probe.error && probe.status === 0; + })(); + + it.skipIf(!isolatedOk)('Yarn PnP without yarn: warns (with the corepack fix) and falls back to npx --no-install', () => { + const run = runHook({ files: { 'yarn.lock': '', '.pnp.cjs': '' }, stubs: ['npx', 'npm'], isolatePath: true }); + expect(run.status).toBe(0); + expect(run.stderr).toContain('[pre-commit] Warning: this repo uses yarn, but yarn is not on PATH; running Charter with npx --no-install (and verify:adf with npm) instead.'); + expect(run.stderr).toContain('corepack enable'); + expect(run.calls).toEqual(expectedCalls('npx [--no-install] [charter]')); + }); + + it.skipIf(!isolatedOk)('pnpm repo without pnpm: warns and falls back to npx --no-install / npm run', () => { + const run = runHook({ + files: { 'pnpm-lock.yaml': '', 'package.json': JSON.stringify({ scripts: { 'verify:adf': 'x' } }) }, + stubs: ['npx', 'npm'], + isolatePath: true, + }); + expect(run.status).toBe(0); + expect(run.stderr).toContain('this repo uses pnpm, but pnpm is not on PATH'); + expect(run.stderr).not.toContain('corepack enable'); + expect(run.calls).toEqual([...expectedCalls('npx [--no-install] [charter]').slice(0, 2), 'npm [run] [verify:adf] token-set']); + }); + }); + + it('prints no package-manager warning and makes no call without .ai/manifest.adf', () => { + fs.rmSync(path.join(dir, '.ai'), { recursive: true }); + const run = runHook({ files: { 'yarn.lock': '' }, stubs: ['npx', 'npm'], isolatePath: true }); + expect(run.status).toBe(0); + expect(run.stderr).toBe(''); + expect(run.calls).toEqual([]); + }); +}); diff --git a/packages/cli/src/__tests__/hook.test.ts b/packages/cli/src/__tests__/hook.test.ts index 8318a57..64b9617 100644 --- a/packages/cli/src/__tests__/hook.test.ts +++ b/packages/cli/src/__tests__/hook.test.ts @@ -6,12 +6,15 @@ import * as path from 'node:path'; import { HOOK_SCOPED_DOCTOR_ENV, HOOK_SCOPED_DOCTOR_FILE, + HOOK_CLI, + HOOK_CLI_RESOLVE_BLOCK, HOOK_TIDY_BLOCK_TEMPLATE, hookCommand, PRE_COMMIT_GATE_SCOPE_MARKER, PRE_COMMIT_VERIFY_SCOPE_MARKER, PRE_COMMIT_HOOK_CONTENT, PRE_COMMIT_HOOK_MARKER, + PRE_COMMIT_PM_CLI_MARKER, PRE_COMMIT_SCOPE_MARKER, printClaudeHookConfig, renderHookTidyBlock, @@ -230,10 +233,10 @@ describe('hookCommand', () => { it('INV-P4-4: the generated and repo blocks are the template, differing only in {CLI}', async () => { const generated = tidyBlock(await generatedHook()); const repo = tidyBlock(repoHook()); - expect(generated).toBe(renderHookTidyBlock('npx charter')); + expect(generated).toBe(renderHookTidyBlock(HOOK_CLI)); expect(repo).toBe(renderHookTidyBlock(REPO_CLI)); - expect(generated.split('npx charter').join('{CLI}')).toBe(repo.split(REPO_CLI).join('{CLI}')); - expect(generated.split('npx charter').join('{CLI}')).toBe(HOOK_TIDY_BLOCK_TEMPLATE); + expect(generated.split(HOOK_CLI).join('{CLI}')).toBe(repo.split(REPO_CLI).join('{CLI}')); + expect(generated.split(HOOK_CLI).join('{CLI}')).toBe(HOOK_TIDY_BLOCK_TEMPLATE); }); it('pins the scope marker, the compiled fast path, --print-paths and staged-only re-staging', async () => { @@ -302,7 +305,13 @@ if [ -f ".ai/manifest.adf" ]; then fi fi `; - const HOOK_360 = PRE_COMMIT_HOOK_CONTENT.slice(0, PRE_COMMIT_HOOK_CONTENT.indexOf('# ADF evidence gate')) + GATE_360; + // A pre-#366 hook: no CLI resolution block, every call a bare `npx charter`. + const toPre366 = (hook: string): string => hook + .replace(`${HOOK_CLI_RESOLVE_BLOCK}\n`, '') + .split(HOOK_CLI).join('npx charter') + .split('$CHARTER_RUN').join('npx charter'); + const HOOK_360 = toPre366(PRE_COMMIT_HOOK_CONTENT.slice(0, PRE_COMMIT_HOOK_CONTENT.indexOf('# ADF evidence gate'))) + GATE_360; + const HOOK_365 = toPre366(PRE_COMMIT_HOOK_CONTENT); async function doctorChecks(): Promise> { const logs: string[] = []; @@ -376,11 +385,52 @@ fi expect((await doctorChecks()).some(c => c.name === 'pre-commit hook')).toBe(false); }); + it('#366: HOOK_365 is the current hook minus CLI resolution', () => { + expect(HOOK_365).toContain(PRE_COMMIT_VERIFY_SCOPE_MARKER); + expect(HOOK_365).not.toContain(PRE_COMMIT_PM_CLI_MARKER); + expect(HOOK_365).not.toContain(HOOK_CLI); + expect(HOOK_365).toContain(' npx charter doctor --adf-only --ci --staged --tidy-skipped "$TIDY_SKIPPED" --format text || {'); + expect(HOOK_365).toContain('PLAN=$(npx charter adf tidy --dry-run --print-paths $SOURCES 2>/dev/null)'); + }); + + it.each([ + ['.pnp.cjs', '.pnp.cjs', ''], + ['.pnp.js', '.pnp.js', ''], + ['.yarnrc.yml nodeLinker: pnp', '.yarnrc.yml', 'yarnPath: .yarn/releases/yarn-4.5.0.cjs\nnodeLinker: pnp\n'], + ['.yarnrc.yml nodeLinker: "pnp"', '.yarnrc.yml', 'nodeLinker: "pnp" # PnP\n'], + ])('#366: INFO for a pre-#366 hook in a Yarn PnP repo (%s), gone after reinstall', async (_label, file, content) => { + writeHook(path.join(tempDir, '.git', 'hooks'), HOOK_365); + fs.writeFileSync(path.join(tempDir, file), content); + const rows = (await doctorChecks()).filter(c => c.name === 'pre-commit hook'); + expect(rows).toEqual([{ + name: 'pre-commit hook', + status: 'INFO', + details: "The Charter pre-commit hook at .git/hooks/pre-commit runs Charter with npx charter, which cannot find a Charter installed under Yarn Plug'n'Play, as this repo uses, so its gate fails unless Charter is also installed globally. Re-run: charter hook install --pre-commit (the new hook runs yarn charter)", + }]); + + vi.spyOn(console, 'log').mockImplementation(() => {}); + expect(await hookCommand(baseOptions, ['install', '--pre-commit'])).toBe(0); + vi.restoreAllMocks(); + expect((await doctorChecks()).some(c => c.name === 'pre-commit hook')).toBe(false); + }); + + it.each([ + ['no lockfile', null, ''], + ['yarn.lock (node_modules linker)', 'yarn.lock', ''], + ['.yarnrc.yml nodeLinker: node-modules', '.yarnrc.yml', 'nodeLinker: node-modules\n'], + ['.yarnrc.yml nodeLinker: pnpm', '.yarnrc.yml', 'nodeLinker: pnpm\n'], + ['pnpm-lock.yaml', 'pnpm-lock.yaml', "lockfileVersion: '9.0'\n"], + ])('#366: no row for a pre-#366 hook outside Yarn PnP (%s): npx charter still finds node_modules/.bin there', async (_label, file, content) => { + writeHook(path.join(tempDir, '.git', 'hooks'), HOOK_365); + if (file !== null) fs.writeFileSync(path.join(tempDir, file), content); + expect((await doctorChecks()).some(c => c.name === 'pre-commit hook')).toBe(false); + }); + it('#357: the generated gate always runs the scoped doctor, then verify:adf (with the token) or evidence', () => { const gate = PRE_COMMIT_HOOK_CONTENT.slice(PRE_COMMIT_HOOK_CONTENT.indexOf('\n# ADF evidence gate')); expect(gate).toContain(PRE_COMMIT_GATE_SCOPE_MARKER); expect(gate).toContain(PRE_COMMIT_VERIFY_SCOPE_MARKER); - const doctorAt = gate.indexOf(' npx charter doctor --adf-only --ci --staged --tidy-skipped "$TIDY_SKIPPED" --format text || {'); + const doctorAt = gate.indexOf(' charter_cli doctor --adf-only --ci --staged --tidy-skipped "$TIDY_SKIPPED" --format text || {'); const branchAt = gate.indexOf(`if [ -f "package.json" ] && grep -q '"verify:adf"' package.json; then`); // The scoped doctor runs before, and outside, the verify:adf branch. expect(doctorAt).toBeGreaterThan(-1); @@ -394,10 +444,13 @@ fi expect(gate).toContain('[pre-commit] Could not write the scope token in the git dir'); expect(gate).toContain(` ${HOOK_SCOPED_DOCTOR_ENV}="$SCOPE_TOKEN"\n export ${HOOK_SCOPED_DOCTOR_ENV}\n`); expect(gate).toContain(`trap 'rm -f "$SCOPE_FILE"' EXIT`); - expect(gate).toContain(' npx charter adf evidence --auto-measure --ci || {'); + expect(gate).toContain(' charter_cli adf evidence --auto-measure --ci || {'); expect(gate).not.toContain('Blocked because'); expect(gate).not.toContain('totalExtracted'); - expect(gate.match(/npx charter/g)?.length).toBe(5); // two calls, three echoed hints + // #366: no bare npx charter; two calls through charter_cli, three echoed hints naming $CHARTER_RUN. + expect(gate).not.toContain('npx charter'); + expect(gate.match(/^\s*charter_cli /gm)?.length).toBe(2); + expect(gate.match(/Run: \$CHARTER_RUN /g)?.length).toBe(3); }); it('re-running hook install --pre-commit replaces the old hook without --force, and the row goes away', async () => { diff --git a/packages/cli/src/__tests__/integration/precommit-hook.test.ts b/packages/cli/src/__tests__/integration/precommit-hook.test.ts index a64aa29..e6b5b7d 100644 --- a/packages/cli/src/__tests__/integration/precommit-hook.test.ts +++ b/packages/cli/src/__tests__/integration/precommit-hook.test.ts @@ -2,7 +2,7 @@ * Integration tests: the real pre-commit hook in real git repos (#320). * * Each repo installs the hook `charter hook install --pre-commit` generates - * (PRE_COMMIT_HOOK_CONTENT), with `npx charter` replaced by the built CLI behind + * (PRE_COMMIT_HOOK_CONTENT), with its charter_cli function (#366) redefined to run the built CLI behind * a logging shim, so these tests exercise the shipped text and can prove which * CLI calls the hook made. Paths are passed to the shell double-quoted with * forward slashes: Node accepts C:/... on Windows and Git's sh leaves them alone. @@ -19,7 +19,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { execFileSync, spawn, spawnSync } from 'node:child_process'; import { afterEach, describe, expect, it } from 'vitest'; -import { HOOK_SCOPED_DOCTOR_FILE, PRE_COMMIT_HOOK_CONTENT } from '../../commands/hook'; +import { HOOK_CLI, HOOK_CLI_RESOLVE_BLOCK, HOOK_SCOPED_DOCTOR_FILE, PRE_COMMIT_HOOK_CONTENT } from '../../commands/hook'; import { BLOAT_SCOPE_MSG } from '../../commands/doctor'; // ============================================================================ @@ -179,7 +179,10 @@ function writeHookFixtureRepo(label: string, opts: FixtureOpts = {}): Repo { // The generated hook, as `charter hook install --pre-commit` writes it. const hookDir = path.join(tmp, '.githooks'); fs.mkdirSync(hookDir, { recursive: true }); - const full = PRE_COMMIT_HOOK_CONTENT.split('npx charter').join(`node "${posix(shim)}"`); + // #366: charter_cli is redefined right after the generated one, so package-manager + // detection still runs (verify:adf uses it) and every Charter call reaches the shim. + const full = PRE_COMMIT_HOOK_CONTENT.replace(HOOK_CLI_RESOLVE_BLOCK, `${HOOK_CLI_RESOLVE_BLOCK}${HOOK_CLI}() { node "${posix(shim)}" "$@"; }\n`); + expect(full).not.toBe(PRE_COMMIT_HOOK_CONTENT); const hook = opts.tidyOnly ? full.slice(0, full.indexOf('# ADF evidence gate')) : full; expect(hook).toContain('# Charter pre-commit vendor tidy: scoped to staged files'); fs.writeFileSync(path.join(hookDir, 'pre-commit'), hook); diff --git a/packages/cli/src/commands/doctor-gate-enforcement.ts b/packages/cli/src/commands/doctor-gate-enforcement.ts index b34389b..eb94778 100644 --- a/packages/cli/src/commands/doctor-gate-enforcement.ts +++ b/packages/cli/src/commands/doctor-gate-enforcement.ts @@ -1,7 +1,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { runGit } from '../git-helpers'; -import { PRE_COMMIT_GATE_SCOPE_MARKER, PRE_COMMIT_HOOK_MARKER, PRE_COMMIT_SCOPE_MARKER, PRE_COMMIT_VERIFY_SCOPE_MARKER } from './hook'; +import { PRE_COMMIT_GATE_SCOPE_MARKER, PRE_COMMIT_HOOK_MARKER, PRE_COMMIT_PM_CLI_MARKER, PRE_COMMIT_SCOPE_MARKER, PRE_COMMIT_VERIFY_SCOPE_MARKER } from './hook'; interface GateEnforcementCheck { name: 'gate enforcement'; @@ -51,6 +51,7 @@ interface PreCommitScopeCheck { export const DOCTOR_OLD_PRE_COMMIT_MSG = 'The Charter pre-commit hook at {hookPath} predates staged-only tidy: it tidies every vendor file and re-stages all of .ai/*.adf, which can sweep unrelated edits into a commit. Re-run: charter hook install --pre-commit'; export const DOCTOR_UNSCOPED_GATE_MSG = 'The Charter pre-commit hook at {hookPath} predates the commit-scoped evidence gate: it blocks a commit on vendor bloat in files the commit does not include, and in files its tidy step skipped. Re-run: charter hook install --pre-commit'; export const DOCTOR_VERIFY_ADF_GATE_MSG = 'The Charter pre-commit hook at {hookPath} predates the commit-scoped gate for repos with a verify:adf script, as this one has: it runs pnpm run verify:adf in place of that gate, so its doctor blocks a commit on vendor bloat in files the commit does not include, and the gate fails where pnpm is not installed. Re-run: charter hook install --pre-commit'; +export const DOCTOR_PNP_CLI_MSG = 'The Charter pre-commit hook at {hookPath} runs Charter with npx charter, which cannot find a Charter installed under Yarn Plug\'n\'Play, as this repo uses, so its gate fails unless Charter is also installed globally. Re-run: charter hook install --pre-commit (the new hook runs yarn charter)'; /** * INFO when the active pre-commit hook is a Charter-generated one from before @@ -61,7 +62,9 @@ export const DOCTOR_VERIFY_ADF_GATE_MSG = 'The Charter pre-commit hook at {hookP * doctor and then adf evidence, exactly as the current hook does there, so it * gets no row: there is nothing to fix. The script test is the hook's own * (`grep '"verify:adf"' package.json` at the repo root), so the row appears as - * soon as the hook would take the unscoped path. + * soon as the hook would take the unscoped path. A pre-#366 hook (no + * PRE_COMMIT_PM_CLI_MARKER) gets a row only in a Yarn PnP repo, the one place + * its `npx charter` cannot find a locally installed Charter. * Undefined, so no row at all, for a current, foreign or missing hook. * INFO, not WARN: doctor --ci fails on WARN, and an old hook over-stages but * enforces nothing less, so warning would break CI on upgrade (#186). @@ -73,7 +76,8 @@ export function checkPreCommitScope(): PreCommitScopeCheck | undefined { const message = !content.includes(PRE_COMMIT_SCOPE_MARKER) ? DOCTOR_OLD_PRE_COMMIT_MSG : !content.includes(PRE_COMMIT_GATE_SCOPE_MARKER) ? DOCTOR_UNSCOPED_GATE_MSG : !content.includes(PRE_COMMIT_VERIFY_SCOPE_MARKER) && repoHasVerifyAdfScript() ? DOCTOR_VERIFY_ADF_GATE_MSG - : undefined; + : !content.includes(PRE_COMMIT_PM_CLI_MARKER) && repoUsesYarnPnp() ? DOCTOR_PNP_CLI_MSG + : undefined; if (!message) return undefined; return { name: 'pre-commit hook', @@ -82,6 +86,16 @@ export function checkPreCommitScope(): PreCommitScopeCheck | undefined { }; } +/** + * The hook's own PnP test (#366): .pnp.cjs or .pnp.js at the repo root, or a + * .yarnrc.yml whose nodeLinker is pnp. Same regex as HOOK_CLI_RESOLVE_BLOCK. + */ +function repoUsesYarnPnp(): boolean { + const repoRoot = getGitPath(['rev-parse', '--show-toplevel']) || process.cwd(); + if (fs.existsSync(path.join(repoRoot, '.pnp.cjs')) || fs.existsSync(path.join(repoRoot, '.pnp.js'))) return true; + return /^[ \t]*nodeLinker:[ \t]*["']?pnp(["' \t#]|$)/m.test(readText(path.join(repoRoot, '.yarnrc.yml'))); +} + /** The hook's own test: package.json at the repo root mentions "verify:adf". */ function repoHasVerifyAdfScript(): boolean { const repoRoot = getGitPath(['rev-parse', '--show-toplevel']) || process.cwd(); diff --git a/packages/cli/src/commands/hook.ts b/packages/cli/src/commands/hook.ts index 73d99ef..b0119cd 100644 --- a/packages/cli/src/commands/hook.ts +++ b/packages/cli/src/commands/hook.ts @@ -136,10 +136,17 @@ export const PRE_COMMIT_GATE_SCOPE_MARKER = '# Charter pre-commit evidence gate: // doctor, so its whole-tree doctor still blocked on unrelated bloat, and the gate // failed wherever pnpm is not installed. export const PRE_COMMIT_VERIFY_SCOPE_MARKER = '# Charter pre-commit evidence gate: scoped in verify:adf repos too, run with the repo package manager'; +// Fifth marker (#366): every Charter call goes through charter_cli, which runs the +// CLI with the repo's package manager (yarn, pnpm exec, npx --no-install). An +// older hook called `npx charter`, which cannot resolve Charter under Yarn +// Plug'n'Play and, where Charter is not installed, fetches the unrelated registry +// package `charter`. +export const PRE_COMMIT_PM_CLI_MARKER = '# Charter pre-commit CLI: run through the repo package manager, never downloaded'; /** - * The vendor tidy block of the pre-commit hook (#320). {CLI} is `npx charter` - * here and `node packages/cli/dist/bin.js` in the repo's own .githooks/pre-commit; + * The vendor tidy block of the pre-commit hook (#320). {CLI} is `charter_cli` + * (the shell function HOOK_CLI_RESOLVE_BLOCK defines) here and + * `node packages/cli/dist/bin.js` in the repo's own .githooks/pre-commit; * that is the only difference allowed between the two, and hook.test.ts checks it. * POSIX sh: no arrays, no [[, no jq. Works under set -eu and set -euo pipefail: * greps fed by a pipe read all their input (no -q), so under pipefail an early @@ -235,11 +242,78 @@ export function renderHookTidyBlock(cli: string): string { return HOOK_TIDY_BLOCK_TEMPLATE.split('{CLI}').join(cli); } +/** The shell function the generated hook runs Charter with (#366). */ +export const HOOK_CLI = 'charter_cli'; + +/** + * Picks the package manager and defines charter_cli (#366). POSIX sh, no output + * unless a warning is due. Precedence (the #365 verify:adf order, plus PnP): + * 1. Yarn Plug'n'Play: .pnp.cjs / .pnp.js, or .yarnrc.yml with nodeLinker: pnp + * 2. pnpm-lock.yaml -> pnpm + * 3. yarn.lock -> yarn + * 4. otherwise npm + * A yarn or pnpm repo whose binary is not on PATH falls back to npm, with a + * warning (only when .ai/manifest.adf exists, as nothing else would run). + * charter_cli: yarn charter (yarn --silent charter on Yarn 1, whose run banner + * would otherwise land in the tidy block's parsed stdout), pnpm exec charter, or + * npx --no-install charter. --no-install makes npx use only a local or global + * install and never download: the registry package named "charter" is unrelated. + */ +export const HOOK_CLI_RESOLVE_BLOCK = `# Charter CLI resolution (#366): run Charter with the package manager this repo uses. +${PRE_COMMIT_PM_CLI_MARKER} +CHARTER_PNP=0 +if [ -f ".pnp.cjs" ] || [ -f ".pnp.js" ]; then + CHARTER_PNP=1 +elif [ -f ".yarnrc.yml" ] && grep -E "^[[:space:]]*nodeLinker:[[:space:]]*[\\"']?pnp([\\"'[:space:]#]|\\$)" .yarnrc.yml >/dev/null 2>&1; then + CHARTER_PNP=1 +fi +LOCK_PM=npm +if [ "$CHARTER_PNP" = "1" ]; then + LOCK_PM=yarn +elif [ -f "pnpm-lock.yaml" ]; then + LOCK_PM=pnpm +elif [ -f "yarn.lock" ]; then + LOCK_PM=yarn +fi +PM=npm +if [ "$LOCK_PM" != "npm" ]; then + if command -v "$LOCK_PM" >/dev/null 2>&1; then + PM="$LOCK_PM" + elif [ -f ".ai/manifest.adf" ]; then + echo "[pre-commit] Warning: this repo uses $LOCK_PM, but $LOCK_PM is not on PATH; running Charter with npx --no-install (and verify:adf with npm) instead." >&2 + if [ "$CHARTER_PNP" = "1" ]; then + echo "[pre-commit] Yarn Plug'n'Play has no node_modules, so npx finds Charter only if it is installed globally. To fix: corepack enable (or install yarn), then commit again." >&2 + fi + fi +fi +CHARTER_YARN_FLAGS="" +case "$PM" in + yarn) + CHARTER_RUN="yarn charter" + if [ -f ".ai/manifest.adf" ]; then + case "$(yarn --version 2>/dev/null || true)" in + 1.*) CHARTER_YARN_FLAGS="--silent" ;; + esac + fi + ;; + pnpm) CHARTER_RUN="pnpm exec charter" ;; + *) CHARTER_RUN="npx --no-install charter" ;; +esac +charter_cli() { + case "$PM" in + yarn) yarn $CHARTER_YARN_FLAGS charter "$@" ;; + pnpm) pnpm exec charter "$@" ;; + *) npx --no-install charter "$@" ;; + esac +} +`; + export const PRE_COMMIT_HOOK_CONTENT = `#!/usr/bin/env sh ${PRE_COMMIT_HOOK_MARKER} set -eu -${renderHookTidyBlock('npx charter')} +${HOOK_CLI_RESOLVE_BLOCK} +${renderHookTidyBlock(HOOK_CLI)} # ADF evidence gate: check LOC ceilings if manifest exists ${PRE_COMMIT_GATE_SCOPE_MARKER} ${PRE_COMMIT_VERIFY_SCOPE_MARKER} @@ -248,25 +322,20 @@ if [ -f ".ai/manifest.adf" ]; then # --staged: vendor bloat blocks only in files this commit stages, and not in # the ones the tidy block skipped (TIDY_SKIPPED); doctor reports the rest as # INFO. Every other doctor check blocks as before. Runs with or without verify:adf. - npx charter doctor --adf-only --ci --staged --tidy-skipped "$TIDY_SKIPPED" --format text || { + ${HOOK_CLI} doctor --adf-only --ci --staged --tidy-skipped "$TIDY_SKIPPED" --format text || { echo '' echo '[pre-commit] ADF verification FAILED.' if [ -n "$TIDY_SKIPPED" ]; then - echo " Run: npx charter doctor --adf-only --ci --staged --tidy-skipped \\"\${TIDY_SKIPPED# }\\" --format text" + echo " Run: $CHARTER_RUN doctor --adf-only --ci --staged --tidy-skipped \\"\${TIDY_SKIPPED# }\\" --format text" else - echo ' Run: npx charter doctor --adf-only --ci --staged --format text' + echo " Run: $CHARTER_RUN doctor --adf-only --ci --staged --format text" fi + echo ' If charter itself was not found: add @stackbilt/cli as a devDependency and install it (this hook never downloads Charter).' exit 1 } if [ -f "package.json" ] && grep -q '"verify:adf"' package.json; then - # The package manager the lockfile names, when it is installed; npm otherwise - # (it ships with node, and npx above needs it anyway). - PM=npm - if [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then - PM=pnpm - elif [ -f "yarn.lock" ] && command -v yarn >/dev/null 2>&1; then - PM=yarn - fi + # PM is the package manager the lockfile names, when it is installed; npm + # otherwise (it ships with node). See the CLI resolution block above. # The doctor inside verify:adf would re-check vendor bloat in every file. The # scoped doctor above has checked it for this commit, so verify:adf gets a # one-time token, matched against a file in the git dir named after it (one @@ -289,10 +358,11 @@ if [ -f ".ai/manifest.adf" ]; then exit 1 } else - npx charter adf evidence --auto-measure --ci || { + ${HOOK_CLI} adf evidence --auto-measure --ci || { echo '' echo '[pre-commit] ADF verification FAILED.' - echo ' Run: npx charter adf evidence --auto-measure --ci' + echo " Run: $CHARTER_RUN adf evidence --auto-measure --ci" + echo ' If charter itself was not found: add @stackbilt/cli as a devDependency and install it (this hook never downloads Charter).' exit 1 } fi @@ -528,7 +598,10 @@ function printHelp(): void { console.log(' with unstaged changes is skipped. The gate (doctor --staged) blocks on vendor bloat only in'); console.log(' staged files tidy did not skip. If package.json has a verify:adf script, it runs next,'); console.log(' with the package manager the lockfile names (npm if none), and its doctor leaves'); - console.log(' vendor bloat to that scoped check. Re-run this command to upgrade an older hook.'); + console.log(' vendor bloat to that scoped check. Every Charter call runs through the same package'); + console.log(' manager: yarn charter (Yarn, including Plug\'n\'Play), pnpm exec charter, or'); + console.log(' npx --no-install charter, which never downloads a package. Re-run this command to'); + console.log(' upgrade an older hook.'); console.log(' Skip tidy with CHARTER_SKIP_TIDY=1. Only gates when .ai/manifest.adf exists.'); console.log(''); console.log(' print --claude: Print the Claude Code session hook config snippet to stdout.'); From d65eb42af2b6e98fff7bfff0b167501d264ad1b4 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Sat, 17 Oct 2026 19:37:02 -0500 Subject: [PATCH 2/4] fix(hook): resolve @stackbilt/cli via npm exec, restore the #365 cascade, scope the CLI hint (#366) Round 2 on review findings: - npm runner is `npm exec --no --no-update-notifier -- @stackbilt/cli`, never the bare name `charter`. Verified on npm 11.17 against an unreachable registry: a local install runs with no registry request; without one npm requests @stackbilt/cli metadata only (and may run a global install when online), and never installs anything. - Package manager choice walks PnP -> pnpm-lock -> yarn.lock -> npm and takes the first whose marker exists and whose binary is on PATH (the #365 cascade), warning when the first marker's binary is missing. - PnP detection comes from one source (PNP_MARKER_FILES, YARNRC_PNP_LINKER_ERE / yarnrcPnpLinkerRegExp) for the hook and doctor. - The devDependency hint prints only when `charter_cli --version` also fails, not when doctor ran and reported findings. - Docs: upgrading-to-2.0 drops the PnP known limitation; CHANGELOG and cli-reference state the exact network behaviour, global-install pickup, corepack first-use download, and that the fix covers the pre-commit hook only. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 15 +- docs/cli-reference.md | 18 +- docs/upgrading-to-2.0.md | 4 +- packages/cli/src/__tests__/hook-pm.test.ts | 281 +++++++++++++----- .../src/commands/doctor-gate-enforcement.ts | 19 +- packages/cli/src/commands/hook.ts | 110 ++++--- 6 files changed, 333 insertions(+), 114 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 152deba..daf9537 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,20 @@ The format is based on Keep a Changelog and follows Semantic Versioning. ### Fixed -- **The generated pre-commit hook could not run Charter under Yarn Plug'n'Play, and could fetch an unrelated package** (`#366`). Every Charter call in the hook (vendor tidy, the scoped `doctor --staged` gate #365 added, `adf evidence`) was a bare `npx charter`. Under PnP there is no `node_modules/.bin`, so npx missed the installed CLI and fell back to the registry package `charter` (0.0.2, which has no binary), and the gate failed before `yarn run verify:adf` was reached. The same fallback fetched that package in any repo without Charter installed. The hook now picks the package manager once, with the lockfile and `PATH` detection #365 added for `verify:adf` plus Plug'n'Play detection (`.pnp.cjs`, `.pnp.js`, or `.yarnrc.yml` with `nodeLinker: pnp`). It runs Charter as `yarn charter` (`yarn --silent charter` on Yarn 1), `pnpm exec charter` or `npx --no-install charter`, and none of these downloads a package. When the lockfile's package manager is not on `PATH`, the hook warns on stderr (under PnP the warning adds `corepack enable`) and falls back to `npx --no-install charter` and `npm run verify:adf`. A failed gate names the resolved command and says to add `@stackbilt/cli` as a devDependency. The `CHARTER_HOOK_SCOPED_DOCTOR` token flow is unchanged. **Installed hooks do not update themselves:** run `charter hook install --pre-commit` again (no `--force` needed for a Charter-managed hook). This is needed under Yarn PnP and recommended everywhere else. `charter doctor` reports an older hook as INFO in a PnP repo. +- **The generated pre-commit hook could not run Charter under Yarn Plug'n'Play, and could fetch an unrelated package** (`#366`). Every Charter call in the hook (vendor tidy, the scoped `doctor --staged` gate #365 added, `adf evidence`) was a bare `npx charter`. Under PnP there is no `node_modules/.bin`, so npx missed the installed CLI and fell back to the registry package `charter` (0.0.2, which has no binary), and the gate failed before `yarn run verify:adf` was reached. The same fallback fetched that package in any repo without Charter installed. The hook now picks the package manager once. It takes the first candidate whose marker exists and whose binary is on `PATH`, in this order: + 1. Yarn Plug'n'Play (`.pnp.cjs`, `.pnp.js`, or `.yarnrc.yml` with `nodeLinker: pnp`) with `yarn`. + 2. `pnpm-lock.yaml` with `pnpm`. + 3. `yarn.lock` with `yarn`. + 4. npm otherwise. + + This is #365's `verify:adf` cascade with PnP in front, and `verify:adf` uses the same choice. When the first marker found has no binary on `PATH`, the hook warns on stderr with the runner it uses instead; under PnP the warning adds `corepack enable`. + + Charter runs as `yarn charter` (`yarn --silent charter` on Yarn 1), `pnpm exec charter` or `npm exec --no -- @stackbilt/cli`. The npm form names Charter's own package, never the bare name `charter`. None of the three installs a package, and the hook never runs a downloaded one. Network use that remains: + - With a local install, npm makes no registry request; npm's update check is off (`--no-update-notifier`). + - Without one, npm requests `@stackbilt/cli`'s metadata. If the registry is reachable it may run a globally installed `@stackbilt/cli` of any version; offline the request fails after npm's retries. + - Corepack may download Yarn or pnpm on first use. + + Install `@stackbilt/cli` as a devDependency. When a gate step fails, the hook prints the command to re-run it. It also says to add the devDependency, but only if the CLI itself could not run; a doctor that ran and reported findings gets no such line. `charter doctor` and the hook now detect PnP from one shared source. The `CHARTER_HOOK_SCOPED_DOCTOR` token flow is unchanged. Scope: this fix covers the pre-commit hook only; the CI workflow `charter setup` generates still calls `npx charter` (tracked separately). **Installed hooks do not update themselves:** run `charter hook install --pre-commit` again (no `--force` needed for a Charter-managed hook). This is needed under Yarn PnP and recommended everywhere else. `charter doctor` reports an older hook as INFO in a PnP repo. - **`charter init --guided` with a piped or non-TTY stdin stopped partway through the questions, wrote nothing, and exited 0** (`#375`), so a script or CI job could not tell it from a successful run. There were two causes. First, with piped stdin readline emits every buffered line at once, and a line that arrived while no question was pending was dropped, so even a complete set of piped answers (`printf 'typescript\ny\ny\n' | charter init --guided`) stalled after the second question. Second, a pending `rl.question` callback never fires when stdin closes, so the CLI's promise never settled and Node exited 0 once the event loop drained. Guided mode now queues every input line and gives each question the next one, so piped answers work the same as typed ones and write the same files. If stdin closes before the last answer (a short pipe, `` silently ignored `--preset`** (`#375`). It now seeds the answer: the preset is used when the language answer names no known stack (blank, or something like `cobol`), and the first question says `blank keeps --preset `. A recognized language or a Cloudflare `y` still decides, as before. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 24cf508..2806e38 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -79,8 +79,22 @@ npx charter hook install --commit-msg --force # overwrite existing hooks - The tidy block never fails the commit: a tidy failure prints one line and the hook carries on. - The evidence gate after it can. In the generated hook it runs `charter doctor --adf-only --ci --staged --tidy-skipped ""` ([#357](https://github.com/Stackbilt-dev/charter/issues/357)). Vendor bloat blocks the commit only in a staged vendor file that the tidy block did not skip. A staged file that tidy ran on but could not clean still blocks, and so does one left untidied because tidy failed or `CHARTER_SKIP_TIDY=1` is set. Bloat in unstaged vendor files, and in files tidy skipped, shows as an INFO row. A planned-module skip (tidy would write into a module with unstaged or untracked work) skips tidy for every staged vendor file in that commit, so it lifts the bloat gate for all of them. Scoping is by file name: the bloat check still reads each vendor file in the working tree, not its staged content. Every other doctor check blocks as usual. CI's plain `charter doctor --ci` still fails on all vendor bloat. The skip message says how to tidy the file: stage or stash the unstaged work and commit again, or run `charter adf tidy --source ` and `git add` the result. - The scoped doctor runs in every repo, including one whose custom `verify:adf` leaves doctor out: such a repo now gets `charter doctor --adf-only --ci --staged …` on every commit. Next, if `package.json` has a `verify:adf` script (`charter setup` and `charter bootstrap` add one), the gate runs it; otherwise it runs `charter adf evidence --auto-measure --ci`. `verify:adf` runs with ` run verify:adf`, where `` is the package manager described in the next item. - - Every Charter call in the generated hook (tidy, the scoped doctor, `adf evidence`) runs through the repo's package manager ([#366](https://github.com/Stackbilt-dev/charter/issues/366)), picked once at the top of the hook: Yarn when the repo uses Plug'n'Play (`.pnp.cjs` or `.pnp.js`, or `.yarnrc.yml` with `nodeLinker: pnp`), then pnpm for a `pnpm-lock.yaml`, then Yarn for a `yarn.lock`, and npm otherwise. Charter then runs as `yarn charter` (`yarn --silent charter` on Yarn 1, so its run banner stays out of the output tidy parses), `pnpm exec charter`, or `npx --no-install charter`. All three use a locally installed `@stackbilt/cli`; npx also finds a global install. None of them downloads anything: the npm package named `charter` is unrelated, and a plain `npx charter` would fetch it where Charter is not installed. Install `@stackbilt/cli` as a devDependency. If the CLI cannot be found, the gate fails, naming the command to re-run and that fix. If the lockfile names Yarn or pnpm but that binary is not on `PATH`, the hook warns on stderr and uses `npx --no-install charter` and `npm run verify:adf` instead; under Plug'n'Play, which has no `node_modules`, the warning adds that npx only finds a global Charter there, and to run `corepack enable`. On Windows the hook runs under Git for Windows' `sh`, which finds `yarn`, `pnpm` and `npx` as an extensionless shim (as npm and corepack install them) or an `.exe`. A package manager present only as a `.cmd` file is not found there, so the hook falls back as described. A `package.json` script named `charter` shadows the binary for `yarn charter`. The doctor inside `verify:adf` would check vendor bloat across the whole tree again. To prevent that, the hook sets `CHARTER_HOOK_SCOPED_DOCTOR` to a one-time token for that run and writes the token to its own file in the git directory (`charter-hook-scoped-doctor-`, one per hook run, so overlapping commits in one worktree do not interfere), which it removes on exit. If the file cannot be written, the hook prints one `[pre-commit]` line saying so, and that time the doctor inside `verify:adf` checks every vendor file. When the two match, doctor reports `adf vendor bloat (checked by pre-commit hook)` as INFO and prints a line on stderr. All its other checks still block, and so do the script's other commands. The variable without the matching file does nothing, so `charter doctor --ci` in CI keeps full strictness even if the variable is set there. Run by hand, `npm run verify:adf` checks every vendor file, as CI does. - - A hook installed before [#320](https://github.com/Stackbilt-dev/charter/issues/320) re-stages all of `.ai/*.adf`. A hook installed before #357 blocks on vendor bloat outside the commit, and so, in a repo with a `verify:adf` script, does one from the first #357 fix, which also ran `pnpm run verify:adf` by name. Re-run `charter hook install --pre-commit` to replace any of them (no `--force` needed for a Charter-managed hook). `charter doctor` reports an old hook as INFO. It reports a hook from the first #357 fix only where `package.json` has a `verify:adf` script, since without one that hook already runs the scoped gate. A hook installed before #366 calls a bare `npx charter`: it fails under Yarn Plug'n'Play unless Charter is installed globally, and where Charter is not installed at all it fetches the unrelated `charter` package. Reinstalling is recommended in every repo and needed under Plug'n'Play, where `charter doctor` reports such a hook as INFO. Elsewhere `npx charter` still finds `node_modules/.bin`, so doctor shows no row for it. + - Every Charter call in the generated hook (tidy, the scoped doctor, `adf evidence`) runs through the repo's package manager ([#366](https://github.com/Stackbilt-dev/charter/issues/366)), picked once at the top of the hook. The hook walks these candidates in order and takes the first one whose marker exists **and** whose binary is on `PATH`: + 1. Yarn Plug'n'Play (`.pnp.cjs` or `.pnp.js` at the repo root, or a `.yarnrc.yml` line `nodeLinker: pnp`, optionally quoted) with `yarn` on `PATH`: Yarn. + 2. `pnpm-lock.yaml` with `pnpm` on `PATH`: pnpm. + 3. `yarn.lock` with `yarn` on `PATH`: Yarn. + 4. Otherwise: npm. + + So a repo with both lockfiles uses pnpm, or Yarn when only Yarn is installed. If the first marker found has no binary on `PATH`, the hook prints a warning on stderr naming the runner it uses instead. Under Plug'n'Play the warning adds `corepack enable`, since PnP packages load only through Yarn. `verify:adf` runs with the same package manager (` run verify:adf`). + - Charter then runs as `yarn charter` (`yarn --silent charter` on Yarn 1, so its run banner stays out of the output tidy parses), `pnpm exec charter`, or `npm exec --no -- @stackbilt/cli`. The npm form names Charter's own package, never the bare name `charter`: that is an unrelated registry package, which a plain `npx charter` fetches where Charter is not installed. None of the three installs a package, and the hook never runs a downloaded one. Network use that remains: + - With `@stackbilt/cli` installed locally, `npm exec` makes no registry request; the hook passes `--no-update-notifier`, so npm's own version check is off too. + - Without a local install, `npm exec` requests `@stackbilt/cli`'s metadata from the registry. Offline that request fails, after npm's retries (up to about a minute with default settings). + - Corepack may download Yarn or pnpm itself the first time the hook calls one. + - **Global installs:** without a local install, `npm exec --no -- @stackbilt/cli` runs a globally installed `@stackbilt/cli` of any version when the registry is reachable. A global 1.x CLI lacks the flags this hook passes (`--staged`, `--print-paths`). Install `@stackbilt/cli` as a devDependency, so the hook runs the version the repo pins. + - If a gate step fails, the hook prints the command to re-run it. If the CLI itself cannot run (its `--version` fails too), it adds that Charter could not be run with that command and should be added as a devDependency. A doctor that ran and reported problems gets no such line. + - On Windows the hook runs under Git for Windows' `sh`, which finds `yarn`, `pnpm` and `npm` as an extensionless shim (as npm and corepack install them) or an `.exe`. A package manager present only as a `.cmd` file is not found there, so the cascade skips it. A `package.json` script named `charter` shadows the binary for `yarn charter`. + - The doctor inside `verify:adf` would check vendor bloat across the whole tree again. To prevent that, the hook sets `CHARTER_HOOK_SCOPED_DOCTOR` to a one-time token for that run and writes the token to its own file in the git directory (`charter-hook-scoped-doctor-`, one per hook run, so overlapping commits in one worktree do not interfere), which it removes on exit. If the file cannot be written, the hook prints one `[pre-commit]` line saying so, and that time the doctor inside `verify:adf` checks every vendor file. When the two match, doctor reports `adf vendor bloat (checked by pre-commit hook)` as INFO and prints a line on stderr. All its other checks still block, and so do the script's other commands. The variable without the matching file does nothing, so `charter doctor --ci` in CI keeps full strictness even if the variable is set there. Run by hand, `npm run verify:adf` checks every vendor file, as CI does. + - A hook installed before [#320](https://github.com/Stackbilt-dev/charter/issues/320) re-stages all of `.ai/*.adf`. A hook installed before #357 blocks on vendor bloat outside the commit, and so, in a repo with a `verify:adf` script, does one from the first #357 fix, which also ran `pnpm run verify:adf` by name. Re-run `charter hook install --pre-commit` to replace any of them (no `--force` needed for a Charter-managed hook). `charter doctor` reports an old hook as INFO. It reports a hook from the first #357 fix only where `package.json` has a `verify:adf` script, since without one that hook already runs the scoped gate. A hook installed before #366 calls a bare `npx charter`. It fails under Yarn Plug'n'Play unless Charter is installed globally, and where Charter is not installed at all it fetches the unrelated `charter` package. Reinstalling is recommended in every repo and needed under Plug'n'Play, where `charter doctor` reports such a hook as INFO. Elsewhere `npx charter` still finds `node_modules/.bin`, so doctor shows no row for it. - `--force` — overwrite existing non-Charter hooks Must specify at least one of `--commit-msg` or `--pre-commit`. diff --git a/docs/upgrading-to-2.0.md b/docs/upgrading-to-2.0.md index 84cc9ed..293a806 100644 --- a/docs/upgrading-to-2.0.md +++ b/docs/upgrading-to-2.0.md @@ -89,9 +89,9 @@ Installed hooks are not upgraded for you. Replace every pre-2.0 hook by re-runni The new pre-commit hook calls `charter adf tidy --print-paths` and `charter doctor --staged`. With a 1.x CLI in `node_modules`, `--print-paths` is unknown, so the hook reports files as clean and tidies nothing. Upgrade the CLI as well as the hook. -> **`verify:adf` script.** `charter setup` and `bootstrap` add a `verify:adf` script. The hook runs it after the scoped doctor, with your package manager: `pnpm` (with `pnpm-lock.yaml` and pnpm installed), `yarn` (with `yarn.lock` and yarn installed), otherwise `npm`. pnpm is no longer required. Inside the hook, the doctor in `verify:adf` reports vendor bloat as INFO, `adf vendor bloat (checked by pre-commit hook)`, because the scoped check already ran. Its other checks still block. Running `npm run verify:adf` (or your package manager's equivalent) by hand or in CI still checks every vendor file. The hook runs `npx charter doctor …` even if your custom `verify:adf` leaves doctor out. +> **`verify:adf` script.** `charter setup` and `bootstrap` add a `verify:adf` script. The hook runs it after the scoped doctor, with your package manager. That is the first match in this order: Yarn for a Plug'n'Play repo (`.pnp.cjs`, `.pnp.js`, or `.yarnrc.yml` with `nodeLinker: pnp`) with yarn installed, then `pnpm` (with `pnpm-lock.yaml` and pnpm installed), then `yarn` (with `yarn.lock` and yarn installed), otherwise `npm`. pnpm is no longer required. Inside the hook, the doctor in `verify:adf` reports vendor bloat as INFO, `adf vendor bloat (checked by pre-commit hook)`, because the scoped check already ran. Its other checks still block. Running `npm run verify:adf` (or your package manager's equivalent) by hand or in CI still checks every vendor file. The hook runs the scoped `charter doctor …` even if your custom `verify:adf` leaves doctor out. > -> **Known limitation:** Yarn Plug'n'Play repos are not supported by the hook yet ([#366](https://github.com/Stackbilt-dev/charter/issues/366)). +> **How the hook runs Charter** ([#366](https://github.com/Stackbilt-dev/charter/issues/366)): every Charter call goes through that same package manager, as `yarn charter`, `pnpm exec charter` or `npm exec --no -- @stackbilt/cli`. This works under Yarn Plug'n'Play, and the hook never installs or runs a downloaded package. Install `@stackbilt/cli` as a devDependency; without a local install, npm may run a global one of any version. A hook installed before this change calls a bare `npx charter`, so re-run `charter hook install --pre-commit` (required under Plug'n'Play). See the `[Unreleased]` entry in the [CHANGELOG](../CHANGELOG.md) and [`charter hook`](./cli-reference.md#charter-hook) for details. ## What doctor's new lines mean diff --git a/packages/cli/src/__tests__/hook-pm.test.ts b/packages/cli/src/__tests__/hook-pm.test.ts index c808faa..6be25fe 100644 --- a/packages/cli/src/__tests__/hook-pm.test.ts +++ b/packages/cli/src/__tests__/hook-pm.test.ts @@ -2,14 +2,22 @@ * #366: the generated pre-commit hook runs every Charter call through the repo's * package manager. These tests run the real hook text with stub yarn / pnpm / * npx / npm executables first on PATH; each stub records its argv to a log and - * exits 0 (or the code STUB_FAIL_ON names). No network, no real package manager. + * exits 0 (or fails as the fixture asks). No network, no real package manager. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { execFileSync, spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; -import { HOOK_CLI, PRE_COMMIT_HOOK_CONTENT, PRE_COMMIT_PM_CLI_MARKER } from '../commands/hook'; +import { + HOOK_CLI, + HOOK_CLI_RESOLVE_BLOCK, + PNP_MARKER_FILES, + PRE_COMMIT_HOOK_CONTENT, + PRE_COMMIT_PM_CLI_MARKER, + YARNRC_PNP_LINKER_ERE, + yarnrcPnpLinkerRegExp, +} from '../commands/hook'; const shAvailable = (() => { const probe = spawnSync('sh', ['-c', 'exit 0']); @@ -19,24 +27,34 @@ const shAvailable = (() => { const PMS = ['yarn', 'pnpm', 'npx', 'npm'] as const; type Pm = typeof PMS[number]; +const NPM_RUNNER = 'npm [exec] [--no] [--no-update-notifier] [--] [@stackbilt/cli]'; + // Each stub logs "" then each argument in brackets, so an empty argument -// stays visible. yarn answers --version with $STUB_YARN_VERSION. A stub exits 1 -// when its joined argv contains $STUB_FAIL_ON. `npm run` also logs whether the -// scope token reached it. +// stays visible. yarn answers a leading --version with $STUB_YARN_VERSION. +// A Charter invocation (argv naming charter or @stackbilt/cli) exits 127 when +// $STUB_UNRESOLVED is 1, as if the CLI were not installed. Any call exits 1 when +// its joined argv contains $STUB_FAIL_ON, unless it is the `--version` probe. +// ` run` also logs whether the scope token reached it. function stubScript(name: Pm): string { return `#!/bin/sh { printf '%s' '${name}' for a in "$@"; do printf ' [%s]' "$a"; done - if [ "${name}" = "npm" ] || [ "${name}" = "yarn" ] || [ "${name}" = "pnpm" ]; then - if [ "\${1:-}" = "run" ] && [ -n "\${CHARTER_HOOK_SCOPED_DOCTOR:-}" ]; then printf ' token-set'; fi - fi + if [ "\${1:-}" = "run" ] && [ -n "\${CHARTER_HOOK_SCOPED_DOCTOR:-}" ]; then printf ' token-set'; fi printf '\\n' } >> "$STUB_LOG" if [ "${name}" = "yarn" ] && [ "\${1:-}" = "--version" ]; then echo "\${STUB_YARN_VERSION:-4.5.0}" exit 0 fi +case " $* " in + *" charter "*|*" @stackbilt/cli "*) + if [ "\${STUB_UNRESOLVED:-0}" = "1" ]; then echo "${name}: charter: command not found" >&2; exit 127; fi + ;; +esac +case " $* " in + *" --version "*) exit 0 ;; +esac if [ -n "\${STUB_FAIL_ON:-}" ]; then case "$*" in *"$STUB_FAIL_ON"*) echo "${name}: stub failure" >&2; exit 1 ;; @@ -55,13 +73,25 @@ function pathWithoutPms(): string { .join(path.delimiter); } +// Needs a PATH with git and the POSIX tools but no real package manager. +const isolatedOk = (() => { + if (!shAvailable) return false; + const probe = spawnSync('sh', ['-c', 'command -v git && command -v sed && command -v grep && command -v date && ! command -v yarn && ! command -v pnpm && ! command -v npm'], { + env: { ...process.env, PATH: pathWithoutPms() }, + }); + return !probe.error && probe.status === 0; +})(); + interface Fixture { files?: Record; stubs?: Pm[]; yarnVersion?: string; failOn?: string; + unresolved?: boolean; /** Use a PATH with every real package manager removed, so only the stubs exist. */ isolatePath?: boolean; + /** Hook text to run instead of PRE_COMMIT_HOOK_CONTENT (mutation checks). */ + hook?: string; } interface HookRun { @@ -71,6 +101,18 @@ interface HookRun { calls: string[]; } +/** The four lines of the PM cascade that pick yarn for PnP and pnpm for pnpm-lock.yaml. */ +const PNP_THEN_PNPM = `if [ "$CHARTER_PNP" = "1" ] && command -v yarn >/dev/null 2>&1; then + PM=yarn +elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then + PM=pnpm +`; +const PNPM_THEN_YARN_LOCK = `elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then + PM=pnpm +elif [ -f "yarn.lock" ] && command -v yarn >/dev/null 2>&1; then + PM=yarn +`; + describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the package manager (#366)', { timeout: 30_000 }, () => { let dir: string; let stubDir: string; @@ -87,7 +129,6 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack // A staged, hand-written vendor file, so the tidy block calls the CLI too. fs.writeFileSync(path.join(dir, 'CLAUDE.md'), '# Project\n\nSome notes.\n'); execFileSync('git', ['add', 'CLAUDE.md'], { cwd: dir }); - fs.writeFileSync(path.join(dir, 'hook.sh'), PRE_COMMIT_HOOK_CONTENT); }); afterEach(() => { @@ -96,6 +137,7 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack }); function runHook(fx: Fixture): HookRun { + fs.writeFileSync(path.join(dir, 'hook.sh'), fx.hook ?? PRE_COMMIT_HOOK_CONTENT); for (const [name, content] of Object.entries(fx.files ?? {})) { fs.writeFileSync(path.join(dir, name), content); } @@ -111,6 +153,7 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack STUB_LOG: logFile, STUB_YARN_VERSION: fx.yarnVersion ?? '4.5.0', STUB_FAIL_ON: fx.failOn ?? '', + STUB_UNRESOLVED: fx.unresolved ? '1' : '0', }; delete env.CHARTER_HOOK_SCOPED_DOCTOR; const result = spawnSync('sh', ['hook.sh'], { cwd: dir, env, encoding: 'utf-8' }); @@ -131,104 +174,204 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack const withoutVersionProbe = (calls: string[]): string[] => calls.filter(c => c !== 'yarn [--version]'); - it('the generated hook carries the #366 marker and never calls a bare npx charter', () => { + it('the generated hook carries the #366 marker and never resolves the bare name charter through npx/npm', () => { expect(PRE_COMMIT_HOOK_CONTENT).toContain(PRE_COMMIT_PM_CLI_MARKER); - expect(PRE_COMMIT_HOOK_CONTENT).not.toMatch(/npx charter/); + expect(PRE_COMMIT_HOOK_CONTENT).not.toMatch(/npx/); + expect(PRE_COMMIT_HOOK_CONTENT).not.toMatch(/\bnpm exec[^\n]*\bcharter\b/); // Outside echo lines, Charter is only ever reached through charter_cli. const code = PRE_COMMIT_HOOK_CONTENT.split('\n').filter(l => !/^\s*(#|echo )/.test(l)); const direct = code.filter(l => /\bcharter (adf|doctor)\b/.test(l) && !l.includes(HOOK_CLI) && !l.includes('GENERATED by charter adf compile')); expect(direct).toEqual([]); }); - it('Yarn Berry PnP (.pnp.cjs): yarn charter for every call', () => { - const run = runHook({ files: { 'yarn.lock': '', '.pnp.cjs': '' } }); - expect(run.status).toBe(0); - expect(run.calls).toContain('yarn [--version]'); - expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); - expect(run.stderr).not.toContain('Warning'); - }); + describe('cascade: first candidate whose marker exists and whose binary is on PATH', () => { + it.each(PNP_MARKER_FILES.map(f => [f]))('Yarn PnP from %s alone (no yarn.lock): yarn charter', (marker) => { + const run = runHook({ files: { [marker]: '' } }); + expect(run.status).toBe(0); + expect(run.calls).toContain('yarn [--version]'); + expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + expect(run.stderr).not.toContain('Warning'); + }); - it('Yarn PnP from .yarnrc.yml alone (nodeLinker: pnp, not installed yet): yarn charter', () => { - const run = runHook({ files: { '.yarnrc.yml': 'nodeLinker: pnp\n' } }); - expect(run.status).toBe(0); - expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); - }); + it('Yarn PnP from .yarnrc.yml alone (nodeLinker: pnp, not installed yet): yarn charter', () => { + const run = runHook({ files: { '.yarnrc.yml': 'nodeLinker: pnp\n' } }); + expect(run.status).toBe(0); + expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + }); - it('Yarn classic (yarn.lock, yarn 1.x): yarn --silent charter, so no run banner reaches the tidy plan', () => { - const run = runHook({ files: { 'yarn.lock': '' }, yarnVersion: '1.22.22' }); - expect(run.status).toBe(0); - expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [--silent] [charter]')); - }); + it('PnP plus pnpm-lock.yaml, both binaries on PATH: yarn wins', () => { + const run = runHook({ files: { '.pnp.cjs': '', 'pnpm-lock.yaml': '' } }); + expect(run.status).toBe(0); + expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + }); - it('pnpm (pnpm-lock.yaml): pnpm exec charter', () => { - const run = runHook({ files: { 'pnpm-lock.yaml': "lockfileVersion: '9.0'\n" } }); - expect(run.status).toBe(0); - expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); - }); + it('pnpm-lock.yaml plus yarn.lock, both binaries on PATH: pnpm wins', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': '', 'yarn.lock': '' } }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + }); - it.each([ - ['package-lock.json', { 'package-lock.json': '{}' }], - ['no lockfile', {}], - ])('npm (%s): npx --no-install charter, which never downloads a package', (_label, files) => { - const run = runHook({ files }); - expect(run.status).toBe(0); - expect(run.calls).toEqual(expectedCalls('npx [--no-install] [charter]')); + it('Yarn classic (yarn.lock, yarn 1.x): yarn --silent charter, so no run banner reaches the tidy plan', () => { + const run = runHook({ files: { 'yarn.lock': '' }, yarnVersion: '1.22.22' }); + expect(run.status).toBe(0); + expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [--silent] [charter]')); + }); + + it('pnpm (pnpm-lock.yaml): pnpm exec charter', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': "lockfileVersion: '9.0'\n" } }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + }); + + it.each([ + ['package-lock.json', { 'package-lock.json': '{}' }], + ['no lockfile', {}], + ])('npm (%s): npm exec --no -- @stackbilt/cli, never the bare name charter', (_label, files) => { + const run = runHook({ files }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls(NPM_RUNNER)); + }); + + it('mutation: swapping the PnP and pnpm-lock.yaml branches changes the PnP-plus-pnpm result', () => { + expect(PRE_COMMIT_HOOK_CONTENT).toContain(PNP_THEN_PNPM); + const swapped = PRE_COMMIT_HOOK_CONTENT.replace(PNP_THEN_PNPM, `if [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then + PM=pnpm +elif [ "$CHARTER_PNP" = "1" ] && command -v yarn >/dev/null 2>&1; then + PM=yarn +`); + const run = runHook({ files: { '.pnp.cjs': '', 'pnpm-lock.yaml': '' }, hook: swapped }); + expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + }); + + it('mutation: swapping the pnpm-lock.yaml and yarn.lock branches changes the two-lockfile result', () => { + expect(PRE_COMMIT_HOOK_CONTENT).toContain(PNPM_THEN_YARN_LOCK); + const swapped = PRE_COMMIT_HOOK_CONTENT.replace(PNPM_THEN_YARN_LOCK, `elif [ -f "yarn.lock" ] && command -v yarn >/dev/null 2>&1; then + PM=yarn +elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then + PM=pnpm +`); + const run = runHook({ files: { 'pnpm-lock.yaml': '', 'yarn.lock': '' }, hook: swapped }); + expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + }); }); it.each([ ['yarn PnP', { 'yarn.lock': '', '.pnp.cjs': '' }, 'yarn [charter]', 'yarn [run] [verify:adf] token-set'], ['pnpm', { 'pnpm-lock.yaml': '' }, 'pnpm [exec] [charter]', 'pnpm [run] [verify:adf] token-set'], - ['npm', {}, 'npx [--no-install] [charter]', 'npm [run] [verify:adf] token-set'], + ['npm', {}, NPM_RUNNER, 'npm [run] [verify:adf] token-set'], ])('verify:adf repo (%s): scoped doctor through the package manager, then run verify:adf with the token', (_label, files, runner, verify) => { const run = runHook({ files: { ...files, 'package.json': JSON.stringify({ scripts: { 'verify:adf': 'charter doctor --adf-only --ci' } }) } }); expect(run.status).toBe(0); expect(withoutVersionProbe(run.calls)).toEqual([...expectedCalls(runner).slice(0, 2), verify]); }); - it('a failing doctor blocks the commit and the hint names the resolved command', () => { - const run = runHook({ files: { 'pnpm-lock.yaml': '' }, failOn: 'doctor' }); - expect(run.status).toBe(1); - expect(run.stdout).toContain('[pre-commit] ADF verification FAILED.'); - expect(run.stdout).toContain(' Run: pnpm exec charter doctor --adf-only --ci --staged --format text'); - expect(run.stdout).toContain('add @stackbilt/cli as a devDependency'); + describe('failure hints', () => { + it('doctor ran and reported problems: blocks, names the command, no devDependency hint', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': '' }, failOn: 'doctor' }); + expect(run.status).toBe(1); + expect(run.stdout).toContain('[pre-commit] ADF verification FAILED.'); + expect(run.stdout).toContain(' Run: pnpm exec charter doctor --adf-only --ci --staged --format text'); + expect(run.calls).toContain('pnpm [exec] [charter] [--version]'); + expect(run.stdout).not.toContain('devDependency'); + expect(run.stdout).not.toContain('could not be run'); + }); + + it('evidence ran and failed: no devDependency hint either', () => { + const run = runHook({ files: {}, failOn: 'evidence' }); + expect(run.status).toBe(1); + expect(run.stdout).toContain(` Run: npm exec --no -- @stackbilt/cli adf evidence --auto-measure --ci`); + expect(run.stdout).not.toContain('devDependency'); + }); + + it.each([ + ['pnpm', { 'pnpm-lock.yaml': '' }, 'pnpm exec charter'], + ['npm', {}, 'npm exec --no -- @stackbilt/cli'], + ['yarn', { '.pnp.cjs': '' }, 'yarn charter'], + ])('%s: the CLI cannot be resolved (exit 127): blocks with the devDependency hint', (_label, files, runner) => { + const run = runHook({ files, unresolved: true }); + expect(run.status).toBe(1); + expect(run.stdout).toContain('[pre-commit] ADF verification FAILED.'); + expect(run.stdout).toContain(` Charter itself could not be run with: ${runner}`); + expect(run.stdout).toContain(' Add @stackbilt/cli as a devDependency and install it. This hook never installs Charter.'); + }); }); describe('package manager binary missing from PATH', () => { - // Needs a PATH with git and the POSIX tools but no real package manager. - const isolatedOk = (() => { - if (!shAvailable) return false; - const probe = spawnSync('sh', ['-c', 'command -v git && command -v sed && command -v grep && command -v date && ! command -v yarn && ! command -v pnpm'], { - env: { ...process.env, PATH: pathWithoutPms() }, - }); - return !probe.error && probe.status === 0; - })(); - - it.skipIf(!isolatedOk)('Yarn PnP without yarn: warns (with the corepack fix) and falls back to npx --no-install', () => { - const run = runHook({ files: { 'yarn.lock': '', '.pnp.cjs': '' }, stubs: ['npx', 'npm'], isolatePath: true }); + it.skipIf(!isolatedOk)('Yarn PnP without yarn: warns (with the corepack fix) and falls back to npm exec', () => { + const run = runHook({ files: { 'yarn.lock': '', '.pnp.cjs': '' }, stubs: ['npm'], isolatePath: true }); expect(run.status).toBe(0); - expect(run.stderr).toContain('[pre-commit] Warning: this repo uses yarn, but yarn is not on PATH; running Charter with npx --no-install (and verify:adf with npm) instead.'); + expect(run.stderr).toContain('[pre-commit] Warning: this repo uses yarn, but yarn is not on PATH; running Charter with npm exec --no -- @stackbilt/cli (and verify:adf with npm run) instead.'); expect(run.stderr).toContain('corepack enable'); - expect(run.calls).toEqual(expectedCalls('npx [--no-install] [charter]')); + expect(run.calls).toEqual(expectedCalls(NPM_RUNNER)); + }); + + it.skipIf(!isolatedOk)('PnP plus pnpm-lock.yaml without yarn: the cascade moves on to pnpm, with a warning', () => { + const run = runHook({ files: { '.pnp.cjs': '', 'pnpm-lock.yaml': '' }, stubs: ['pnpm', 'npm'], isolatePath: true }); + expect(run.status).toBe(0); + expect(run.stderr).toContain('this repo uses yarn, but yarn is not on PATH; running Charter with pnpm exec charter (and verify:adf with pnpm run) instead.'); + expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + }); + + it.skipIf(!isolatedOk)('both lockfiles with only yarn installed: yarn (the #365 cascade), not npm', () => { + const run = runHook({ + files: { 'pnpm-lock.yaml': '', 'yarn.lock': '', 'package.json': JSON.stringify({ scripts: { 'verify:adf': 'x' } }) }, + stubs: ['yarn', 'npm'], + isolatePath: true, + }); + expect(run.status).toBe(0); + expect(run.stderr).toContain('this repo uses pnpm, but pnpm is not on PATH; running Charter with yarn charter (and verify:adf with yarn run) instead.'); + expect(withoutVersionProbe(run.calls)).toEqual([...expectedCalls('yarn [charter]').slice(0, 2), 'yarn [run] [verify:adf] token-set']); }); - it.skipIf(!isolatedOk)('pnpm repo without pnpm: warns and falls back to npx --no-install / npm run', () => { + it.skipIf(!isolatedOk)('pnpm repo without pnpm: warns and falls back to npm exec / npm run', () => { const run = runHook({ files: { 'pnpm-lock.yaml': '', 'package.json': JSON.stringify({ scripts: { 'verify:adf': 'x' } }) }, - stubs: ['npx', 'npm'], + stubs: ['npm'], isolatePath: true, }); expect(run.status).toBe(0); expect(run.stderr).toContain('this repo uses pnpm, but pnpm is not on PATH'); expect(run.stderr).not.toContain('corepack enable'); - expect(run.calls).toEqual([...expectedCalls('npx [--no-install] [charter]').slice(0, 2), 'npm [run] [verify:adf] token-set']); + expect(run.calls).toEqual([...expectedCalls(NPM_RUNNER).slice(0, 2), 'npm [run] [verify:adf] token-set']); + }); + + it.skipIf(!isolatedOk)('prints no package-manager warning and makes no call without .ai/manifest.adf', () => { + fs.rmSync(path.join(dir, '.ai'), { recursive: true }); + const run = runHook({ files: { 'yarn.lock': '' }, stubs: ['npm'], isolatePath: true }); + expect(run.status).toBe(0); + expect(run.stderr).toBe(''); + expect(run.calls).toEqual([]); }); }); - it('prints no package-manager warning and makes no call without .ai/manifest.adf', () => { - fs.rmSync(path.join(dir, '.ai'), { recursive: true }); - const run = runHook({ files: { 'yarn.lock': '' }, stubs: ['npx', 'npm'], isolatePath: true }); - expect(run.status).toBe(0); - expect(run.stderr).toBe(''); - expect(run.calls).toEqual([]); + describe('one PnP source for the hook and doctor', () => { + it('the hook text is built from PNP_MARKER_FILES and YARNRC_PNP_LINKER_ERE', () => { + for (const f of PNP_MARKER_FILES) expect(HOOK_CLI_RESOLVE_BLOCK).toContain(`[ -f "${f}" ]`); + const escaped = YARNRC_PNP_LINKER_ERE.replace(/[\\"$`]/g, ch => `\\${ch}`); + expect(HOOK_CLI_RESOLVE_BLOCK).toContain(`grep -E "${escaped}" .yarnrc.yml`); + }); + + // The hook's own grep line, run by sh, against doctor's RegExp on the same file. + const grepLine = (HOOK_CLI_RESOLVE_BLOCK.match(/grep -E "(?:[^"\\]|\\.)*" \.yarnrc\.yml/) ?? [''])[0]; + it.each([ + ['nodeLinker: pnp\n', true], + ['nodeLinker: "pnp"\n', true], + ["nodeLinker: 'pnp' # comment\n", true], + ['nodeLinker: pnp\r\n', true], + [' nodeLinker:pnp\n', true], + ['yarnPath: .yarn/releases/yarn-4.5.0.cjs\nnodeLinker: pnp', true], + ['nodeLinker: pnpm\n', false], + ['nodeLinker: node-modules\n', false], + ['nodeLinker: pnp-loose\n', false], + ['# nodeLinker: pnp\n', false], + ['"nodeLinker": pnp\n', false], + ['', false], + ])('agree on %j (PnP: %s)', (content, expected) => { + expect(grepLine).not.toBe(''); + fs.writeFileSync(path.join(dir, '.yarnrc.yml'), content); + const sh = spawnSync('sh', ['-c', `${grepLine} >/dev/null`], { cwd: dir }); + expect(sh.status === 0).toBe(expected); + expect(yarnrcPnpLinkerRegExp().test(content)).toBe(expected); + }); }); }); diff --git a/packages/cli/src/commands/doctor-gate-enforcement.ts b/packages/cli/src/commands/doctor-gate-enforcement.ts index eb94778..505335a 100644 --- a/packages/cli/src/commands/doctor-gate-enforcement.ts +++ b/packages/cli/src/commands/doctor-gate-enforcement.ts @@ -1,7 +1,15 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { runGit } from '../git-helpers'; -import { PRE_COMMIT_GATE_SCOPE_MARKER, PRE_COMMIT_HOOK_MARKER, PRE_COMMIT_PM_CLI_MARKER, PRE_COMMIT_SCOPE_MARKER, PRE_COMMIT_VERIFY_SCOPE_MARKER } from './hook'; +import { + PNP_MARKER_FILES, + PRE_COMMIT_GATE_SCOPE_MARKER, + PRE_COMMIT_HOOK_MARKER, + PRE_COMMIT_PM_CLI_MARKER, + PRE_COMMIT_SCOPE_MARKER, + PRE_COMMIT_VERIFY_SCOPE_MARKER, + yarnrcPnpLinkerRegExp, +} from './hook'; interface GateEnforcementCheck { name: 'gate enforcement'; @@ -87,13 +95,14 @@ export function checkPreCommitScope(): PreCommitScopeCheck | undefined { } /** - * The hook's own PnP test (#366): .pnp.cjs or .pnp.js at the repo root, or a - * .yarnrc.yml whose nodeLinker is pnp. Same regex as HOOK_CLI_RESOLVE_BLOCK. + * The hook's own PnP test (#366), from the same source the hook text is built + * from: a PNP_MARKER_FILES file at the repo root, or a .yarnrc.yml matching + * YARNRC_PNP_LINKER_ERE (via yarnrcPnpLinkerRegExp). */ function repoUsesYarnPnp(): boolean { const repoRoot = getGitPath(['rev-parse', '--show-toplevel']) || process.cwd(); - if (fs.existsSync(path.join(repoRoot, '.pnp.cjs')) || fs.existsSync(path.join(repoRoot, '.pnp.js'))) return true; - return /^[ \t]*nodeLinker:[ \t]*["']?pnp(["' \t#]|$)/m.test(readText(path.join(repoRoot, '.yarnrc.yml'))); + if (PNP_MARKER_FILES.some(f => fs.existsSync(path.join(repoRoot, f)))) return true; + return yarnrcPnpLinkerRegExp().test(readText(path.join(repoRoot, '.yarnrc.yml'))); } /** The hook's own test: package.json at the repo root mentions "verify:adf". */ diff --git a/packages/cli/src/commands/hook.ts b/packages/cli/src/commands/hook.ts index b0119cd..956be2b 100644 --- a/packages/cli/src/commands/hook.ts +++ b/packages/cli/src/commands/hook.ts @@ -137,11 +137,11 @@ export const PRE_COMMIT_GATE_SCOPE_MARKER = '# Charter pre-commit evidence gate: // failed wherever pnpm is not installed. export const PRE_COMMIT_VERIFY_SCOPE_MARKER = '# Charter pre-commit evidence gate: scoped in verify:adf repos too, run with the repo package manager'; // Fifth marker (#366): every Charter call goes through charter_cli, which runs the -// CLI with the repo's package manager (yarn, pnpm exec, npx --no-install). An +// CLI with the repo's package manager (yarn, pnpm exec, npm exec @stackbilt/cli). An // older hook called `npx charter`, which cannot resolve Charter under Yarn // Plug'n'Play and, where Charter is not installed, fetches the unrelated registry // package `charter`. -export const PRE_COMMIT_PM_CLI_MARKER = '# Charter pre-commit CLI: run through the repo package manager, never downloaded'; +export const PRE_COMMIT_PM_CLI_MARKER = '# Charter pre-commit CLI: run through the repo package manager, never installed by the hook'; /** * The vendor tidy block of the pre-commit hook (#320). {CLI} is `charter_cli` @@ -245,28 +245,59 @@ export function renderHookTidyBlock(cli: string): string { /** The shell function the generated hook runs Charter with (#366). */ export const HOOK_CLI = 'charter_cli'; +/** Files whose presence at the repo root means Yarn Plug'n'Play (#366). Shared with doctor. */ +export const PNP_MARKER_FILES = ['.pnp.cjs', '.pnp.js'] as const; + +/** + * POSIX ERE for a .yarnrc.yml line that selects the PnP linker (`nodeLinker: pnp`, + * optionally quoted, then a quote, space, comment or end of line, so `pnpm` and + * `pnp-loose` do not match). The hook greps with it; doctor uses + * yarnrcPnpLinkerRegExp(), the same pattern with [:space:] spelled \s. + */ +export const YARNRC_PNP_LINKER_ERE = `^[[:space:]]*nodeLinker:[[:space:]]*["']?pnp(["'[:space:]#]|$)`; + +/** YARNRC_PNP_LINKER_ERE as a JS RegExp over a whole file (multiline). */ +export function yarnrcPnpLinkerRegExp(): RegExp { + return new RegExp(YARNRC_PNP_LINKER_ERE.split('[:space:]').join('\\s'), 'm'); +} + +/** Escape a string for the inside of a POSIX sh double-quoted word. */ +function shDoubleQuoted(value: string): string { + return value.replace(/[\\"$`]/g, ch => `\\${ch}`); +} + /** * Picks the package manager and defines charter_cli (#366). POSIX sh, no output - * unless a warning is due. Precedence (the #365 verify:adf order, plus PnP): - * 1. Yarn Plug'n'Play: .pnp.cjs / .pnp.js, or .yarnrc.yml with nodeLinker: pnp - * 2. pnpm-lock.yaml -> pnpm - * 3. yarn.lock -> yarn + * unless a warning is due (and then only when .ai/manifest.adf exists, as nothing + * else in the hook runs without it). The first candidate whose marker exists and + * whose binary is on PATH wins: the #365 verify:adf cascade with PnP in front. + * 1. Yarn Plug'n'Play (a PNP_MARKER_FILES file, or YARNRC_PNP_LINKER_ERE in + * .yarnrc.yml) and yarn on PATH -> yarn + * 2. pnpm-lock.yaml and pnpm on PATH -> pnpm + * 3. yarn.lock and yarn on PATH -> yarn * 4. otherwise npm - * A yarn or pnpm repo whose binary is not on PATH falls back to npm, with a - * warning (only when .ai/manifest.adf exists, as nothing else would run). - * charter_cli: yarn charter (yarn --silent charter on Yarn 1, whose run banner - * would otherwise land in the tidy block's parsed stdout), pnpm exec charter, or - * npx --no-install charter. --no-install makes npx use only a local or global - * install and never download: the registry package named "charter" is unrelated. + * When the first marker found has no binary on PATH, the hook warns and names + * the runner it uses instead. + * charter_cli runs: yarn charter (yarn --silent charter on Yarn 1, whose run + * banner would otherwise land in the tidy block's parsed stdout), pnpm exec + * charter, or npm exec --no -- @stackbilt/cli. The npm form names Charter's own + * package, never the bare name `charter` (an unrelated registry package), and + * --no refuses to install. A local install runs with no registry request. + * Without one, npm requests @stackbilt/cli's metadata and may run a global + * install, but never installs or runs a downloaded package. + * --no-update-notifier skips npm's own version check. + * charter_cli_hint prints the devDependency advice only when charter_cli cannot + * run `--version`, so a doctor that ran and reported problems gets no such hint. */ export const HOOK_CLI_RESOLVE_BLOCK = `# Charter CLI resolution (#366): run Charter with the package manager this repo uses. ${PRE_COMMIT_PM_CLI_MARKER} CHARTER_PNP=0 -if [ -f ".pnp.cjs" ] || [ -f ".pnp.js" ]; then +if ${PNP_MARKER_FILES.map(f => `[ -f "${f}" ]`).join(' || ')}; then CHARTER_PNP=1 -elif [ -f ".yarnrc.yml" ] && grep -E "^[[:space:]]*nodeLinker:[[:space:]]*[\\"']?pnp([\\"'[:space:]#]|\\$)" .yarnrc.yml >/dev/null 2>&1; then +elif [ -f ".yarnrc.yml" ] && grep -E "${shDoubleQuoted(YARNRC_PNP_LINKER_ERE)}" .yarnrc.yml >/dev/null 2>&1; then CHARTER_PNP=1 fi +# LOCK_PM: the first candidate whose marker exists. PM: the first whose binary is also on PATH. LOCK_PM=npm if [ "$CHARTER_PNP" = "1" ]; then LOCK_PM=yarn @@ -276,15 +307,12 @@ elif [ -f "yarn.lock" ]; then LOCK_PM=yarn fi PM=npm -if [ "$LOCK_PM" != "npm" ]; then - if command -v "$LOCK_PM" >/dev/null 2>&1; then - PM="$LOCK_PM" - elif [ -f ".ai/manifest.adf" ]; then - echo "[pre-commit] Warning: this repo uses $LOCK_PM, but $LOCK_PM is not on PATH; running Charter with npx --no-install (and verify:adf with npm) instead." >&2 - if [ "$CHARTER_PNP" = "1" ]; then - echo "[pre-commit] Yarn Plug'n'Play has no node_modules, so npx finds Charter only if it is installed globally. To fix: corepack enable (or install yarn), then commit again." >&2 - fi - fi +if [ "$CHARTER_PNP" = "1" ] && command -v yarn >/dev/null 2>&1; then + PM=yarn +elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then + PM=pnpm +elif [ -f "yarn.lock" ] && command -v yarn >/dev/null 2>&1; then + PM=yarn fi CHARTER_YARN_FLAGS="" case "$PM" in @@ -292,20 +320,32 @@ case "$PM" in CHARTER_RUN="yarn charter" if [ -f ".ai/manifest.adf" ]; then case "$(yarn --version 2>/dev/null || true)" in - 1.*) CHARTER_YARN_FLAGS="--silent" ;; + 1.*) CHARTER_YARN_FLAGS="--silent"; CHARTER_RUN="yarn --silent charter" ;; esac fi ;; pnpm) CHARTER_RUN="pnpm exec charter" ;; - *) CHARTER_RUN="npx --no-install charter" ;; + *) CHARTER_RUN="npm exec --no -- @stackbilt/cli" ;; esac +if [ "$PM" != "$LOCK_PM" ] && [ -f ".ai/manifest.adf" ]; then + echo "[pre-commit] Warning: this repo uses $LOCK_PM, but $LOCK_PM is not on PATH; running Charter with $CHARTER_RUN (and verify:adf with $PM run) instead." >&2 + if [ "$CHARTER_PNP" = "1" ]; then + echo "[pre-commit] Yarn Plug'n'Play needs yarn to load packages. To fix: corepack enable (or install yarn), then commit again." >&2 + fi +fi charter_cli() { case "$PM" in yarn) yarn $CHARTER_YARN_FLAGS charter "$@" ;; pnpm) pnpm exec charter "$@" ;; - *) npx --no-install charter "$@" ;; + *) npm exec --no --no-update-notifier -- @stackbilt/cli "$@" ;; esac } +charter_cli_hint() { + if ! charter_cli --version >/dev/null 2>&1; then + echo " Charter itself could not be run with: $CHARTER_RUN" + echo ' Add @stackbilt/cli as a devDependency and install it. This hook never installs Charter.' + fi +} `; export const PRE_COMMIT_HOOK_CONTENT = `#!/usr/bin/env sh @@ -330,12 +370,12 @@ if [ -f ".ai/manifest.adf" ]; then else echo " Run: $CHARTER_RUN doctor --adf-only --ci --staged --format text" fi - echo ' If charter itself was not found: add @stackbilt/cli as a devDependency and install it (this hook never downloads Charter).' + charter_cli_hint exit 1 } if [ -f "package.json" ] && grep -q '"verify:adf"' package.json; then - # PM is the package manager the lockfile names, when it is installed; npm - # otherwise (it ships with node). See the CLI resolution block above. + # PM is the first package manager in the cascade above whose lockfile or + # marker exists and whose binary is on PATH; npm otherwise (it ships with node). # The doctor inside verify:adf would re-check vendor bloat in every file. The # scoped doctor above has checked it for this commit, so verify:adf gets a # one-time token, matched against a file in the git dir named after it (one @@ -362,7 +402,7 @@ if [ -f ".ai/manifest.adf" ]; then echo '' echo '[pre-commit] ADF verification FAILED.' echo " Run: $CHARTER_RUN adf evidence --auto-measure --ci" - echo ' If charter itself was not found: add @stackbilt/cli as a devDependency and install it (this hook never downloads Charter).' + charter_cli_hint exit 1 } fi @@ -597,11 +637,11 @@ function printHelp(): void { console.log(' Bloat is routed to .adf modules; only the files tidy wrote are re-staged. A vendor file'); console.log(' with unstaged changes is skipped. The gate (doctor --staged) blocks on vendor bloat only in'); console.log(' staged files tidy did not skip. If package.json has a verify:adf script, it runs next,'); - console.log(' with the package manager the lockfile names (npm if none), and its doctor leaves'); - console.log(' vendor bloat to that scoped check. Every Charter call runs through the same package'); - console.log(' manager: yarn charter (Yarn, including Plug\'n\'Play), pnpm exec charter, or'); - console.log(' npx --no-install charter, which never downloads a package. Re-run this command to'); - console.log(' upgrade an older hook.'); + console.log(' with the first package manager whose lockfile (or Plug\'n\'Play marker) exists and that'); + console.log(' is on PATH (npm if none), and its doctor leaves vendor bloat to that scoped check.'); + console.log(' Every Charter call runs through the same package manager: yarn charter, pnpm exec'); + console.log(' charter, or npm exec --no -- @stackbilt/cli, none of which installs a package.'); + console.log(' Re-run this command to upgrade an older hook.'); console.log(' Skip tidy with CHARTER_SKIP_TIDY=1. Only gates when .ai/manifest.adf exists.'); console.log(''); console.log(' print --claude: Print the Claude Code session hook config snippet to stdout.'); From 7b0540112c94ca9f9f5c6def79a1c57199d8ae18 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Sat, 17 Oct 2026 20:17:52 -0500 Subject: [PATCH 3/4] fix(hook): resolve Charter once per commit, no network without a local install (#366) Round 3 on review findings: - Resolve once at the top of the hook. Under npm, a local install (node_modules/.bin/charter or charter.cmd) runs through `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`; without one a global `charter` on PATH runs directly; with neither, charter_cli runs nothing, tidy is skipped and the gate fails after one devDependency hint, with no network request. Measured offline (npm 11.17, registry unreachable): local 6 s, none 0 s, global 2 s. - One `--version` probe per commit (HOOK_CLI_PROBE_BLOCK): unresolvable -> hint once; 1.x -> one warning to upgrade to @stackbilt/cli 2.x. - charter_cli runs $CHARTER_RUN, so the printed Run: commands are exactly what ran (HOOK_CLI_COMMANDS is the single source for hook, help, tests). - Shared PnP regex uses [ \t\r\f\v] instead of \s; the agreement test covers `nodeLinker:\n pnp`. A mocked-module test fails if doctor stops using the shared source. - hook --help lists the cascade order and the exact commands. - A failed tidy dry run no longer prints "Vendor config files are clean." (template and the repo's own .githooks/pre-commit). - docs/upgrading-to-2.0.md no longer says the hooks call `npx charter`. Co-Authored-By: Claude Opus 5.5 --- .githooks/pre-commit | 6 +- CHANGELOG.md | 16 +- docs/cli-reference.md | 18 +- docs/upgrading-to-2.0.md | 4 +- .../src/__tests__/doctor-pnp-source.test.ts | 60 +++++ packages/cli/src/__tests__/hook-pm.test.ts | 209 ++++++++++++------ packages/cli/src/__tests__/hook.test.ts | 3 +- .../integration/precommit-hook.test.ts | 10 +- packages/cli/src/commands/hook.ts | 139 ++++++++---- 9 files changed, 331 insertions(+), 134 deletions(-) create mode 100644 packages/cli/src/__tests__/doctor-pnp-source.test.ts diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 676796a..658020d 100644 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -36,7 +36,8 @@ if [ "${CHARTER_SKIP_TIDY:-0}" != "1" ] && [ -f ".ai/manifest.adf" ]; then if [ -n "$SOURCES" ]; then echo "[pre-commit] Checking staged vendor files:$NAMES" - PLAN=$(node packages/cli/dist/bin.js adf tidy --dry-run --print-paths $SOURCES 2>/dev/null) || { echo "[pre-commit] adf tidy --dry-run failed; skipping auto-tidy."; PLAN=""; } + PLAN_OK=1 + PLAN=$(node packages/cli/dist/bin.js adf tidy --dry-run --print-paths $SOURCES 2>/dev/null) || { echo "[pre-commit] adf tidy --dry-run failed; skipping auto-tidy."; PLAN=""; PLAN_OK=0; } if printf '%s\n' "$PLAN" | grep -E '^(vendor|module) ' >/dev/null; then BLOCKED="" IGNORED="" @@ -82,7 +83,8 @@ if [ "${CHARTER_SKIP_TIDY:-0}" != "1" ] && [ -f ".ai/manifest.adf" ]; then fi echo "[pre-commit] Vendor files tidied; re-staged only the planned files tidy wrote." fi - else + elif [ "$PLAN_OK" = "1" ]; then + # A failed dry run has said so above; it proves nothing about the files. echo "[pre-commit] Vendor config files are clean." fi fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 08bf70c..c1655ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,12 +22,18 @@ The format is based on Keep a Changelog and follows Semantic Versioning. This is #365's `verify:adf` cascade with PnP in front, and `verify:adf` uses the same choice. When the first marker found has no binary on `PATH`, the hook warns on stderr with the runner it uses instead; under PnP the warning adds `corepack enable`. - Charter runs as `yarn charter` (`yarn --silent charter` on Yarn 1), `pnpm exec charter` or `npm exec --no -- @stackbilt/cli`. The npm form names Charter's own package, never the bare name `charter`. None of the three installs a package, and the hook never runs a downloaded one. Network use that remains: - - With a local install, npm makes no registry request; npm's update check is off (`--no-update-notifier`). - - Without one, npm requests `@stackbilt/cli`'s metadata. If the registry is reachable it may run a globally installed `@stackbilt/cli` of any version; offline the request fails after npm's retries. - - Corepack may download Yarn or pnpm on first use. + Charter runs as `yarn charter` (`yarn --silent charter` on Yarn 1) or `pnpm exec charter`. Under npm the hook checks for a local install first: + - With `node_modules/.bin/charter` (or `charter.cmd`): `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`. It names Charter's own package, never the bare name `charter`, and makes no registry request. + - Without one: a global `charter` on `PATH`, run directly. + - With neither: no Charter call at all, so no network request. The hook says once to add `@stackbilt/cli` as a devDependency, skips tidy, and fails the gate. - Install `@stackbilt/cli` as a devDependency. When a gate step fails, the hook prints the command to re-run it. It also says to add the devDependency, but only if the CLI itself could not run; a doctor that ran and reported findings gets no such line. `charter doctor` and the hook now detect PnP from one shared source. The `CHARTER_HOOK_SCOPED_DOCTOR` token flow is unchanged. Scope: this fix covers the pre-commit hook only; the CI workflow `charter setup` generates still calls `npx charter` (tracked separately). **Installed hooks do not update themselves:** run `charter hook install --pre-commit` again (no `--force` needed for a Charter-managed hook). This is needed under Yarn PnP and recommended everywhere else. `charter doctor` reports an older hook as INFO in a PnP repo. + The hook never installs a package or runs a downloaded one. The only network use left is Corepack downloading Yarn or pnpm on first use. Measured offline with npm 11.17, a commit took 6 s with a local install, 0 s with no install at all, and 2 s with a global one. The `Run:` hints print exactly the command the hook ran. + + The hook runs `--version` once per commit, before any other Charter call: + - If it fails, the devDependency hint is printed once and Charter is not called again. + - If it reports 1.x, the hook warns once to upgrade to `@stackbilt/cli` 2.x, since 1.x ignores `--staged` and `--print-paths`. + + A doctor that ran and reported findings gets no devDependency hint. A failed tidy dry run no longer goes on to say "Vendor config files are clean." `charter doctor` and the hook now detect PnP from one shared source. The `CHARTER_HOOK_SCOPED_DOCTOR` token flow is unchanged. Scope: this fix covers the pre-commit hook only; the CI workflow `charter setup` generates still calls `npx charter` (tracked separately). **Installed hooks do not update themselves:** run `charter hook install --pre-commit` again (no `--force` needed for a Charter-managed hook). This is needed under Yarn PnP and recommended everywhere else. `charter doctor` reports an older hook as INFO in a PnP repo. - **`charter adf populate`, and so every `charter bootstrap`, re-added stack constraints to a `CONSTRAINTS` section you had edited** (`#371`; 2.1.0 listed this as still open under `#354`). Deleting a bullet from `core.adf` and re-running bootstrap brought a stack rule back (for example the Cloudflare `No Node.js-specific APIs in Worker handlers` rule), because populate appended any detected stack constraint it did not find. Populate now remembers what it has added or offered in a new file, `.ai/.populate.json` (`{ "version": 1, "constraints": { "core.adf": { "added": [...], "offered": [...] } } }`, sorted, written only when it changes). Commit it with the rest of `.ai/`; bootstrap lists it under what it created, and its `git add .ai` next step already includes it. The rules: - A constraint recorded as added or offered is never added again, whether it is still in the section or you deleted it. Deleting the one rule populate added to a plain Worker repo now sticks, and a deleted `CONSTRAINTS` section is not recreated once the file has a record. - New constraints go only into a section that is untouched: the template's bullets, in order, followed by nothing but bullets populate recorded as added. A missing section is created only while there is no record. It goes right after whichever of `GUIDE` and `CONTEXT` comes later, which in the scaffold's order is its own place, rather than at the end of the file (when populate adds a missing `CONTEXT` in the same run, that one is appended at the end and `CONSTRAINTS` follows it); its header is written as the scaffold writes it (`⚠️ CONSTRAINTS [load-bearing]:`). diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 16ec37a..bf87c49 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -86,12 +86,18 @@ npx charter hook install --commit-msg --force # overwrite existing hooks 4. Otherwise: npm. So a repo with both lockfiles uses pnpm, or Yarn when only Yarn is installed. If the first marker found has no binary on `PATH`, the hook prints a warning on stderr naming the runner it uses instead. Under Plug'n'Play the warning adds `corepack enable`, since PnP packages load only through Yarn. `verify:adf` runs with the same package manager (` run verify:adf`). - - Charter then runs as `yarn charter` (`yarn --silent charter` on Yarn 1, so its run banner stays out of the output tidy parses), `pnpm exec charter`, or `npm exec --no -- @stackbilt/cli`. The npm form names Charter's own package, never the bare name `charter`: that is an unrelated registry package, which a plain `npx charter` fetches where Charter is not installed. None of the three installs a package, and the hook never runs a downloaded one. Network use that remains: - - With `@stackbilt/cli` installed locally, `npm exec` makes no registry request; the hook passes `--no-update-notifier`, so npm's own version check is off too. - - Without a local install, `npm exec` requests `@stackbilt/cli`'s metadata from the registry. Offline that request fails, after npm's retries (up to about a minute with default settings). - - Corepack may download Yarn or pnpm itself the first time the hook calls one. - - **Global installs:** without a local install, `npm exec --no -- @stackbilt/cli` runs a globally installed `@stackbilt/cli` of any version when the registry is reachable. A global 1.x CLI lacks the flags this hook passes (`--staged`, `--print-paths`). Install `@stackbilt/cli` as a devDependency, so the hook runs the version the repo pins. - - If a gate step fails, the hook prints the command to re-run it. If the CLI itself cannot run (its `--version` fails too), it adds that Charter could not be run with that command and should be added as a devDependency. A doctor that ran and reported problems gets no such line. + - Charter then runs as exactly one of these commands, and the hook's `Run:` hints print the same command: + - Yarn: `yarn charter`, or `yarn --silent charter` on Yarn 1, so its run banner stays out of the output tidy parses. + - pnpm: `pnpm exec charter`. + - npm with a local install (`node_modules/.bin/charter` or `charter.cmd` exists): `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`. This names Charter's own package, never the bare name `charter`: that is an unrelated registry package, which a plain `npx charter` fetches where Charter is not installed. It makes no registry request. + - npm without a local install, but a `charter` on `PATH`: that global `charter`, run directly. + - npm with neither: nothing. The hook says once that Charter could not be run and to add `@stackbilt/cli` as a devDependency, skips tidy, and fails the gate without calling Charter or npm, so it makes no network request. + - The hook never installs a package or runs a downloaded one. The only network use left is Corepack downloading Yarn or pnpm itself the first time the hook calls one. Measured offline (registry unreachable, npm 11.17): a commit with a local install took 6 s, one with neither a local nor a global install 0 s, and one with a global install 2 s. + - **Once per commit,** before any other Charter call, the hook runs Charter's `--version`: + - If it fails (Charter not resolvable with that command), the hook prints the devDependency hint once and skips the rest as above. + - If it reports 1.x, the hook warns once that it needs `@stackbilt/cli` 2.x. A 1.x CLI ignores `--staged` and `--print-paths`, so its tidy does nothing and its gate checks every file. The hook still runs it. + - When a gate step fails after that, the hook prints only the command to re-run. A doctor that ran and reported problems gets no devDependency hint. + - **Global installs** are used only under npm without a local install, and in any version, so a global 1.x CLI gets the warning above. Install `@stackbilt/cli` as a devDependency, so the hook runs the version the repo pins. - On Windows the hook runs under Git for Windows' `sh`, which finds `yarn`, `pnpm` and `npm` as an extensionless shim (as npm and corepack install them) or an `.exe`. A package manager present only as a `.cmd` file is not found there, so the cascade skips it. A `package.json` script named `charter` shadows the binary for `yarn charter`. - The doctor inside `verify:adf` would check vendor bloat across the whole tree again. To prevent that, the hook sets `CHARTER_HOOK_SCOPED_DOCTOR` to a one-time token for that run and writes the token to its own file in the git directory (`charter-hook-scoped-doctor-`, one per hook run, so overlapping commits in one worktree do not interfere), which it removes on exit. If the file cannot be written, the hook prints one `[pre-commit]` line saying so, and that time the doctor inside `verify:adf` checks every vendor file. When the two match, doctor reports `adf vendor bloat (checked by pre-commit hook)` as INFO and prints a line on stderr. All its other checks still block, and so do the script's other commands. The variable without the matching file does nothing, so `charter doctor --ci` in CI keeps full strictness even if the variable is set there. Run by hand, `npm run verify:adf` checks every vendor file, as CI does. - A hook installed before [#320](https://github.com/Stackbilt-dev/charter/issues/320) re-stages all of `.ai/*.adf`. A hook installed before #357 blocks on vendor bloat outside the commit, and so, in a repo with a `verify:adf` script, does one from the first #357 fix, which also ran `pnpm run verify:adf` by name. Re-run `charter hook install --pre-commit` to replace any of them (no `--force` needed for a Charter-managed hook). `charter doctor` reports an old hook as INFO. It reports a hook from the first #357 fix only where `package.json` has a `verify:adf` script, since without one that hook already runs the scoped gate. A hook installed before #366 calls a bare `npx charter`. It fails under Yarn Plug'n'Play unless Charter is installed globally, and where Charter is not installed at all it fetches the unrelated `charter` package. Reinstalling is recommended in every repo and needed under Plug'n'Play, where `charter doctor` reports such a hook as INFO. Elsewhere `npx charter` still finds `node_modules/.bin`, so doctor shows no row for it. diff --git a/docs/upgrading-to-2.0.md b/docs/upgrading-to-2.0.md index 293a806..2b94e59 100644 --- a/docs/upgrading-to-2.0.md +++ b/docs/upgrading-to-2.0.md @@ -9,7 +9,7 @@ The full list of changes is in the [CHANGELOG](../CHANGELOG.md). Flag details ar Run these from the repository root, starting from a clean working tree (`git status` shows nothing to commit). ```bash -# 1. Upgrade the CLI. The git hooks call `npx charter`, so the local devDependency must be 2.0 too. +# 1. Upgrade the CLI. The pre-commit hook runs the repo's local @stackbilt/cli, so the devDependency must be 2.x too. npm install --save-dev @stackbilt/cli@latest # or: pnpm add -D @stackbilt/cli@latest npx charter --version @@ -91,7 +91,7 @@ The new pre-commit hook calls `charter adf tidy --print-paths` and `charter doct > **`verify:adf` script.** `charter setup` and `bootstrap` add a `verify:adf` script. The hook runs it after the scoped doctor, with your package manager. That is the first match in this order: Yarn for a Plug'n'Play repo (`.pnp.cjs`, `.pnp.js`, or `.yarnrc.yml` with `nodeLinker: pnp`) with yarn installed, then `pnpm` (with `pnpm-lock.yaml` and pnpm installed), then `yarn` (with `yarn.lock` and yarn installed), otherwise `npm`. pnpm is no longer required. Inside the hook, the doctor in `verify:adf` reports vendor bloat as INFO, `adf vendor bloat (checked by pre-commit hook)`, because the scoped check already ran. Its other checks still block. Running `npm run verify:adf` (or your package manager's equivalent) by hand or in CI still checks every vendor file. The hook runs the scoped `charter doctor …` even if your custom `verify:adf` leaves doctor out. > -> **How the hook runs Charter** ([#366](https://github.com/Stackbilt-dev/charter/issues/366)): every Charter call goes through that same package manager, as `yarn charter`, `pnpm exec charter` or `npm exec --no -- @stackbilt/cli`. This works under Yarn Plug'n'Play, and the hook never installs or runs a downloaded package. Install `@stackbilt/cli` as a devDependency; without a local install, npm may run a global one of any version. A hook installed before this change calls a bare `npx charter`, so re-run `charter hook install --pre-commit` (required under Plug'n'Play). See the `[Unreleased]` entry in the [CHANGELOG](../CHANGELOG.md) and [`charter hook`](./cli-reference.md#charter-hook) for details. +> **How the hook runs Charter** ([#366](https://github.com/Stackbilt-dev/charter/issues/366)): every Charter call goes through that same package manager, as `yarn charter`, `pnpm exec charter` or (with a local install) `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`. This works under Yarn Plug'n'Play, and the hook never installs or runs a downloaded package. Install `@stackbilt/cli` as a devDependency. Under npm without a local install, the hook runs a global `charter` from `PATH` (any version; it warns about 1.x), or, with none, skips Charter and fails the gate with a hint. A hook installed before this change calls a bare `npx charter`, so re-run `charter hook install --pre-commit` (required under Plug'n'Play). See the `[Unreleased]` entry in the [CHANGELOG](../CHANGELOG.md) and [`charter hook`](./cli-reference.md#charter-hook) for details. ## What doctor's new lines mean diff --git a/packages/cli/src/__tests__/doctor-pnp-source.test.ts b/packages/cli/src/__tests__/doctor-pnp-source.test.ts new file mode 100644 index 0000000..90e3112 --- /dev/null +++ b/packages/cli/src/__tests__/doctor-pnp-source.test.ts @@ -0,0 +1,60 @@ +/** + * #366: doctor's Yarn PnP check must read the same source the hook text is built + * from (PNP_MARKER_FILES, yarnrcPnpLinkerRegExp). The hook module is mocked with + * different values here: a doctor that hard-codes its own marker list or regex + * would miss both and fail these tests. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +vi.mock('../commands/hook', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + PNP_MARKER_FILES: ['.custom-pnp-marker'], + yarnrcPnpLinkerRegExp: () => /^customLinker: on$/m, + }; +}); + +import { checkPreCommitScope, DOCTOR_PNP_CLI_MSG } from '../commands/doctor-gate-enforcement'; +import { PRE_COMMIT_HOOK_CONTENT, PRE_COMMIT_PM_CLI_MARKER } from '../commands/hook'; + +describe('doctor PnP detection uses the hook module as its only source (#366)', () => { + let originalCwd: string; + let tempDir: string; + // A Charter hook from before #366: every marker but the CLI one. + const OLD_HOOK = PRE_COMMIT_HOOK_CONTENT.split(PRE_COMMIT_PM_CLI_MARKER).join(''); + + beforeEach(() => { + originalCwd = process.cwd(); + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'charter-doctor-pnp-')); + process.chdir(tempDir); + execFileSync('git', ['init', '-q'], { stdio: 'ignore' }); + fs.mkdirSync(path.join(tempDir, '.git', 'hooks'), { recursive: true }); + fs.writeFileSync(path.join(tempDir, '.git', 'hooks', 'pre-commit'), OLD_HOOK); + }); + + afterEach(() => { + process.chdir(originalCwd); + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + const row = () => checkPreCommitScope(); + + it('a marker file from the mocked PNP_MARKER_FILES counts as PnP', () => { + expect(row()).toBeUndefined(); + fs.writeFileSync(path.join(tempDir, '.custom-pnp-marker'), ''); + expect(row()?.details).toBe(DOCTOR_PNP_CLI_MSG.replace('{hookPath}', '.git/hooks/pre-commit')); + }); + + it('a .yarnrc.yml matching the mocked regex counts as PnP; the real markers no longer do', () => { + fs.writeFileSync(path.join(tempDir, '.pnp.cjs'), ''); + fs.writeFileSync(path.join(tempDir, '.yarnrc.yml'), 'nodeLinker: pnp\n'); + expect(row()).toBeUndefined(); + fs.writeFileSync(path.join(tempDir, '.yarnrc.yml'), 'customLinker: on\n'); + expect(row()?.status).toBe('INFO'); + }); +}); diff --git a/packages/cli/src/__tests__/hook-pm.test.ts b/packages/cli/src/__tests__/hook-pm.test.ts index 6be25fe..9e49349 100644 --- a/packages/cli/src/__tests__/hook-pm.test.ts +++ b/packages/cli/src/__tests__/hook-pm.test.ts @@ -1,8 +1,8 @@ /** * #366: the generated pre-commit hook runs every Charter call through the repo's * package manager. These tests run the real hook text with stub yarn / pnpm / - * npx / npm executables first on PATH; each stub records its argv to a log and - * exits 0 (or fails as the fixture asks). No network, no real package manager. + * npx / npm / charter executables first on PATH; each stub records its argv to a + * log and exits 0 (or fails as the fixture asks). No network, no real package manager. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { execFileSync, spawnSync } from 'node:child_process'; @@ -11,6 +11,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { HOOK_CLI, + HOOK_CLI_COMMANDS, HOOK_CLI_RESOLVE_BLOCK, PNP_MARKER_FILES, PRE_COMMIT_HOOK_CONTENT, @@ -24,18 +25,28 @@ const shAvailable = (() => { return !probe.error && probe.status === 0; })(); -const PMS = ['yarn', 'pnpm', 'npx', 'npm'] as const; -type Pm = typeof PMS[number]; +const STUBS = ['yarn', 'pnpm', 'npx', 'npm', 'charter'] as const; +type Stub = typeof STUBS[number]; -const NPM_RUNNER = 'npm [exec] [--no] [--no-update-notifier] [--] [@stackbilt/cli]'; +/** A HOOK_CLI_COMMANDS entry the way the stub log shows it: " [arg] [arg]...". */ +const logged = (command: string): string => { + const [bin, ...args] = command.split(' '); + return [bin, ...args.map(a => `[${a}]`)].join(' '); +}; +const NPM = logged(HOOK_CLI_COMMANDS.npm); +const PNPM = logged(HOOK_CLI_COMMANDS.pnpm); +const YARN = logged(HOOK_CLI_COMMANDS.yarn); +const YARN1 = logged(HOOK_CLI_COMMANDS.yarn1); +const GLOBAL = logged(HOOK_CLI_COMMANDS.global); // Each stub logs "" then each argument in brackets, so an empty argument // stays visible. yarn answers a leading --version with $STUB_YARN_VERSION. -// A Charter invocation (argv naming charter or @stackbilt/cli) exits 127 when -// $STUB_UNRESOLVED is 1, as if the CLI were not installed. Any call exits 1 when -// its joined argv contains $STUB_FAIL_ON, unless it is the `--version` probe. +// A Charter invocation (the charter stub itself, or argv naming charter or +// @stackbilt/cli) exits 127 when $STUB_UNRESOLVED is 1, as if the CLI were not +// installed, and answers --version with $STUB_CHARTER_VERSION. Any other call +// exits 1 when its joined argv contains $STUB_FAIL_ON. // ` run` also logs whether the scope token reached it. -function stubScript(name: Pm): string { +function stubScript(name: Stub): string { return `#!/bin/sh { printf '%s' '${name}' @@ -47,14 +58,17 @@ if [ "${name}" = "yarn" ] && [ "\${1:-}" = "--version" ]; then echo "\${STUB_YARN_VERSION:-4.5.0}" exit 0 fi +is_charter=0 +[ "${name}" = "charter" ] && is_charter=1 case " $* " in - *" charter "*|*" @stackbilt/cli "*) - if [ "\${STUB_UNRESOLVED:-0}" = "1" ]; then echo "${name}: charter: command not found" >&2; exit 127; fi - ;; -esac -case " $* " in - *" --version "*) exit 0 ;; + *" charter "*|*" @stackbilt/cli "*) is_charter=1 ;; esac +if [ "$is_charter" = "1" ]; then + if [ "\${STUB_UNRESOLVED:-0}" = "1" ]; then echo "${name}: charter: command not found" >&2; exit 127; fi + case " $* " in + *" --version "*) echo "\${STUB_CHARTER_VERSION:-charter v2.1.0}"; exit 0 ;; + esac +fi if [ -n "\${STUB_FAIL_ON:-}" ]; then case "$*" in *"$STUB_FAIL_ON"*) echo "${name}: stub failure" >&2; exit 1 ;; @@ -64,31 +78,35 @@ exit 0 `; } -/** PATH entries that hold none of the package-manager binaries (any Windows extension). */ +/** PATH entries that hold none of the stubbed binaries (any Windows extension). */ function pathWithoutPms(): string { const exts = ['', '.cmd', '.exe', '.ps1', '.bat']; return (process.env.PATH ?? '') .split(path.delimiter) - .filter(dir => dir && !PMS.some(pm => exts.some(ext => fs.existsSync(path.join(dir, pm + ext))))) + .filter(dir => dir && !STUBS.some(pm => exts.some(ext => fs.existsSync(path.join(dir, pm + ext))))) .join(path.delimiter); } -// Needs a PATH with git and the POSIX tools but no real package manager. +// Needs a PATH with git and the POSIX tools but no real package manager or charter. const isolatedOk = (() => { if (!shAvailable) return false; - const probe = spawnSync('sh', ['-c', 'command -v git && command -v sed && command -v grep && command -v date && ! command -v yarn && ! command -v pnpm && ! command -v npm'], { + const probe = spawnSync('sh', ['-c', 'command -v git && command -v sed && command -v grep && command -v date && ! command -v yarn && ! command -v pnpm && ! command -v npm && ! command -v charter'], { env: { ...process.env, PATH: pathWithoutPms() }, }); return !probe.error && probe.status === 0; })(); +/** A local npm install, as the hook detects it. */ +const LOCAL_BIN = { 'node_modules/.bin/charter': '#!/bin/sh\n' }; + interface Fixture { files?: Record; - stubs?: Pm[]; + stubs?: Stub[]; yarnVersion?: string; + charterVersion?: string; failOn?: string; unresolved?: boolean; - /** Use a PATH with every real package manager removed, so only the stubs exist. */ + /** Use a PATH with every real package manager and charter removed, so only the stubs exist. */ isolatePath?: boolean; /** Hook text to run instead of PRE_COMMIT_HOOK_CONTENT (mutation checks). */ hook?: string; @@ -139,9 +157,10 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack function runHook(fx: Fixture): HookRun { fs.writeFileSync(path.join(dir, 'hook.sh'), fx.hook ?? PRE_COMMIT_HOOK_CONTENT); for (const [name, content] of Object.entries(fx.files ?? {})) { + fs.mkdirSync(path.dirname(path.join(dir, name)), { recursive: true }); fs.writeFileSync(path.join(dir, name), content); } - for (const pm of fx.stubs ?? PMS) { + for (const pm of fx.stubs ?? STUBS.filter(s => s !== 'charter')) { const stub = path.join(stubDir, pm); fs.writeFileSync(stub, stubScript(pm)); fs.chmodSync(stub, 0o755); @@ -152,10 +171,12 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack PATH: `${stubDir}${path.delimiter}${basePath}`, STUB_LOG: logFile, STUB_YARN_VERSION: fx.yarnVersion ?? '4.5.0', + STUB_CHARTER_VERSION: fx.charterVersion ?? 'charter v2.1.0', STUB_FAIL_ON: fx.failOn ?? '', STUB_UNRESOLVED: fx.unresolved ? '1' : '0', }; delete env.CHARTER_HOOK_SCOPED_DOCTOR; + delete env.CHARTER_SKIP_TIDY; const result = spawnSync('sh', ['hook.sh'], { cwd: dir, env, encoding: 'utf-8' }); const calls = fs.existsSync(logFile) ? fs.readFileSync(logFile, 'utf-8').split('\n').filter(Boolean) @@ -163,16 +184,17 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack return { status: result.status, stdout: result.stdout, stderr: result.stderr, calls }; } - /** The three CLI calls a manifest repo without verify:adf makes, prefixed by the runner. */ + /** The probe and the three CLI calls a manifest repo without verify:adf makes, prefixed by the runner. */ function expectedCalls(runner: string): string[] { return [ + `${runner} [--version]`, `${runner} [adf] [tidy] [--dry-run] [--print-paths] [--source] [CLAUDE.md]`, `${runner} [doctor] [--adf-only] [--ci] [--staged] [--tidy-skipped] [] [--format] [text]`, `${runner} [adf] [evidence] [--auto-measure] [--ci]`, ]; } - const withoutVersionProbe = (calls: string[]): string[] => calls.filter(c => c !== 'yarn [--version]'); + const withoutYarnVersion = (calls: string[]): string[] => calls.filter(c => c !== 'yarn [--version]'); it('the generated hook carries the #366 marker and never resolves the bare name charter through npx/npm', () => { expect(PRE_COMMIT_HOOK_CONTENT).toContain(PRE_COMMIT_PM_CLI_MARKER); @@ -182,6 +204,9 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack const code = PRE_COMMIT_HOOK_CONTENT.split('\n').filter(l => !/^\s*(#|echo )/.test(l)); const direct = code.filter(l => /\bcharter (adf|doctor)\b/.test(l) && !l.includes(HOOK_CLI) && !l.includes('GENERATED by charter adf compile')); expect(direct).toEqual([]); + // charter_cli runs $CHARTER_RUN, the same string the "Run:" hints print. + expect(HOOK_CLI_RESOLVE_BLOCK).toContain(' $CHARTER_RUN "$@"\n'); + for (const command of Object.values(HOOK_CLI_COMMANDS)) expect(HOOK_CLI_RESOLVE_BLOCK).toContain(`CHARTER_RUN="${command}"`); }); describe('cascade: first candidate whose marker exists and whose binary is on PATH', () => { @@ -189,47 +214,48 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack const run = runHook({ files: { [marker]: '' } }); expect(run.status).toBe(0); expect(run.calls).toContain('yarn [--version]'); - expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); - expect(run.stderr).not.toContain('Warning'); + expect(withoutYarnVersion(run.calls)).toEqual(expectedCalls(YARN)); + expect(run.stderr).toBe(''); }); it('Yarn PnP from .yarnrc.yml alone (nodeLinker: pnp, not installed yet): yarn charter', () => { const run = runHook({ files: { '.yarnrc.yml': 'nodeLinker: pnp\n' } }); expect(run.status).toBe(0); - expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + expect(withoutYarnVersion(run.calls)).toEqual(expectedCalls(YARN)); }); it('PnP plus pnpm-lock.yaml, both binaries on PATH: yarn wins', () => { const run = runHook({ files: { '.pnp.cjs': '', 'pnpm-lock.yaml': '' } }); expect(run.status).toBe(0); - expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + expect(withoutYarnVersion(run.calls)).toEqual(expectedCalls(YARN)); }); it('pnpm-lock.yaml plus yarn.lock, both binaries on PATH: pnpm wins', () => { const run = runHook({ files: { 'pnpm-lock.yaml': '', 'yarn.lock': '' } }); expect(run.status).toBe(0); - expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + expect(run.calls).toEqual(expectedCalls(PNPM)); }); it('Yarn classic (yarn.lock, yarn 1.x): yarn --silent charter, so no run banner reaches the tidy plan', () => { const run = runHook({ files: { 'yarn.lock': '' }, yarnVersion: '1.22.22' }); expect(run.status).toBe(0); - expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [--silent] [charter]')); + expect(withoutYarnVersion(run.calls)).toEqual(expectedCalls(YARN1)); }); it('pnpm (pnpm-lock.yaml): pnpm exec charter', () => { const run = runHook({ files: { 'pnpm-lock.yaml': "lockfileVersion: '9.0'\n" } }); expect(run.status).toBe(0); - expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + expect(run.calls).toEqual(expectedCalls(PNPM)); }); it.each([ - ['package-lock.json', { 'package-lock.json': '{}' }], - ['no lockfile', {}], - ])('npm (%s): npm exec --no -- @stackbilt/cli, never the bare name charter', (_label, files) => { + ['package-lock.json', { ...LOCAL_BIN, 'package-lock.json': '{}' }], + ['no lockfile', LOCAL_BIN], + ['charter.cmd only (Windows install)', { 'node_modules/.bin/charter.cmd': '@echo off\r\n' }], + ])('npm with a local install (%s): npm exec @stackbilt/cli, never the bare name charter', (_label, files) => { const run = runHook({ files }); expect(run.status).toBe(0); - expect(run.calls).toEqual(expectedCalls(NPM_RUNNER)); + expect(run.calls).toEqual(expectedCalls(NPM)); }); it('mutation: swapping the PnP and pnpm-lock.yaml branches changes the PnP-plus-pnpm result', () => { @@ -240,7 +266,7 @@ elif [ "$CHARTER_PNP" = "1" ] && command -v yarn >/dev/null 2>&1; then PM=yarn `); const run = runHook({ files: { '.pnp.cjs': '', 'pnpm-lock.yaml': '' }, hook: swapped }); - expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + expect(run.calls).toEqual(expectedCalls(PNPM)); }); it('mutation: swapping the pnpm-lock.yaml and yarn.lock branches changes the two-lockfile result', () => { @@ -251,65 +277,107 @@ elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then PM=pnpm `); const run = runHook({ files: { 'pnpm-lock.yaml': '', 'yarn.lock': '' }, hook: swapped }); - expect(withoutVersionProbe(run.calls)).toEqual(expectedCalls('yarn [charter]')); + expect(withoutYarnVersion(run.calls)).toEqual(expectedCalls(YARN)); }); }); it.each([ - ['yarn PnP', { 'yarn.lock': '', '.pnp.cjs': '' }, 'yarn [charter]', 'yarn [run] [verify:adf] token-set'], - ['pnpm', { 'pnpm-lock.yaml': '' }, 'pnpm [exec] [charter]', 'pnpm [run] [verify:adf] token-set'], - ['npm', {}, NPM_RUNNER, 'npm [run] [verify:adf] token-set'], + ['yarn PnP', { 'yarn.lock': '', '.pnp.cjs': '' }, YARN, 'yarn [run] [verify:adf] token-set'], + ['pnpm', { 'pnpm-lock.yaml': '' }, PNPM, 'pnpm [run] [verify:adf] token-set'], + ['npm', LOCAL_BIN, NPM, 'npm [run] [verify:adf] token-set'], ])('verify:adf repo (%s): scoped doctor through the package manager, then run verify:adf with the token', (_label, files, runner, verify) => { const run = runHook({ files: { ...files, 'package.json': JSON.stringify({ scripts: { 'verify:adf': 'charter doctor --adf-only --ci' } }) } }); expect(run.status).toBe(0); - expect(withoutVersionProbe(run.calls)).toEqual([...expectedCalls(runner).slice(0, 2), verify]); + expect(withoutYarnVersion(run.calls)).toEqual([...expectedCalls(runner).slice(0, 3), verify]); }); describe('failure hints', () => { - it('doctor ran and reported problems: blocks, names the command, no devDependency hint', () => { - const run = runHook({ files: { 'pnpm-lock.yaml': '' }, failOn: 'doctor' }); + it.each([ + ['pnpm', { 'pnpm-lock.yaml': '' }, HOOK_CLI_COMMANDS.pnpm], + ['npm', LOCAL_BIN, HOOK_CLI_COMMANDS.npm], + ['yarn', { '.pnp.cjs': '' }, HOOK_CLI_COMMANDS.yarn], + ])('%s: doctor ran and reported problems: blocks; the Run: line is exactly what ran; no devDependency hint', (_label, files, command) => { + const run = runHook({ files, failOn: 'doctor' }); expect(run.status).toBe(1); expect(run.stdout).toContain('[pre-commit] ADF verification FAILED.'); - expect(run.stdout).toContain(' Run: pnpm exec charter doctor --adf-only --ci --staged --format text'); - expect(run.calls).toContain('pnpm [exec] [charter] [--version]'); - expect(run.stdout).not.toContain('devDependency'); - expect(run.stdout).not.toContain('could not be run'); + const runLine = run.stdout.split('\n').find(l => l.startsWith(' Run: ')); + expect(runLine).toBe(` Run: ${command} doctor --adf-only --ci --staged --format text`); + expect(run.calls).toContain(`${logged(command)} [doctor] [--adf-only] [--ci] [--staged] [--tidy-skipped] [] [--format] [text]`); + expect(run.stdout + run.stderr).not.toContain('devDependency'); + expect(run.calls.filter(c => c.endsWith('[--version]') && c !== 'yarn [--version]')).toHaveLength(1); }); - it('evidence ran and failed: no devDependency hint either', () => { - const run = runHook({ files: {}, failOn: 'evidence' }); + it('evidence ran and failed: the exact command, no devDependency hint', () => { + const run = runHook({ files: LOCAL_BIN, failOn: 'evidence' }); expect(run.status).toBe(1); - expect(run.stdout).toContain(` Run: npm exec --no -- @stackbilt/cli adf evidence --auto-measure --ci`); - expect(run.stdout).not.toContain('devDependency'); + expect(run.stdout).toContain(` Run: ${HOOK_CLI_COMMANDS.npm} adf evidence --auto-measure --ci`); + expect(run.stdout + run.stderr).not.toContain('devDependency'); }); it.each([ - ['pnpm', { 'pnpm-lock.yaml': '' }, 'pnpm exec charter'], - ['npm', {}, 'npm exec --no -- @stackbilt/cli'], - ['yarn', { '.pnp.cjs': '' }, 'yarn charter'], - ])('%s: the CLI cannot be resolved (exit 127): blocks with the devDependency hint', (_label, files, runner) => { + ['pnpm', { 'pnpm-lock.yaml': '' }, HOOK_CLI_COMMANDS.pnpm], + ['npm (local bin present)', LOCAL_BIN, HOOK_CLI_COMMANDS.npm], + ['yarn', { '.pnp.cjs': '' }, HOOK_CLI_COMMANDS.yarn], + ])('%s: the CLI cannot be resolved (exit 127): one probe, the hint once, tidy skipped, the gate fails', (_label, files, command) => { const run = runHook({ files, unresolved: true }); expect(run.status).toBe(1); + expect(withoutYarnVersion(run.calls)).toEqual([`${logged(command)} [--version]`]); + expect(run.stderr).toContain(`[pre-commit] Charter could not be run with: ${command}`); + expect(run.stderr.split('Add @stackbilt/cli as a devDependency').length - 1).toBe(1); + expect(run.stdout).not.toContain('Checking staged vendor files'); expect(run.stdout).toContain('[pre-commit] ADF verification FAILED.'); - expect(run.stdout).toContain(` Charter itself could not be run with: ${runner}`); - expect(run.stdout).toContain(' Add @stackbilt/cli as a devDependency and install it. This hook never installs Charter.'); + expect(run.stdout).toContain('Charter was not run: it could not be found'); + }); + + it('a 1.x CLI: one upgrade warning naming 2.x; the hook still runs it', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': '' }, charterVersion: 'charter v1.9.3' }); + expect(run.status).toBe(0); + expect(run.stderr).toContain(`[pre-commit] Warning: ${HOOK_CLI_COMMANDS.pnpm} --version reports charter v1.9.3; this hook needs @stackbilt/cli 2.x`); + expect(run.stderr).toContain('Install or upgrade the devDependency: @stackbilt/cli@^2.'); + expect(run.stderr.split('Warning:').length - 1).toBe(1); + expect(run.calls).toEqual(expectedCalls(PNPM)); + }); + + it('a failed tidy dry run does not claim the vendor files are clean', () => { + const run = runHook({ files: LOCAL_BIN, failOn: 'dry-run' }); + expect(run.stdout).toContain('[pre-commit] adf tidy --dry-run failed; skipping auto-tidy.'); + expect(run.stdout).not.toContain('Vendor config files are clean'); + }); + + it('a dry run that plans nothing still says the vendor files are clean', () => { + const run = runHook({ files: LOCAL_BIN }); + expect(run.stdout).toContain('[pre-commit] Vendor config files are clean.'); }); }); - describe('package manager binary missing from PATH', () => { - it.skipIf(!isolatedOk)('Yarn PnP without yarn: warns (with the corepack fix) and falls back to npm exec', () => { - const run = runHook({ files: { 'yarn.lock': '', '.pnp.cjs': '' }, stubs: ['npm'], isolatePath: true }); + describe('npm without a local install, and missing binaries', () => { + it.skipIf(!isolatedOk)('no local install, no global charter: no Charter call at all, the hint once, the gate fails', () => { + const run = runHook({ stubs: ['npm', 'npx'], isolatePath: true }); + expect(run.status).toBe(1); + expect(run.calls).toEqual([]); + expect(run.stderr).toContain(`[pre-commit] Charter could not be run with: ${HOOK_CLI_COMMANDS.npm}`); + expect(run.stderr.split('Add @stackbilt/cli as a devDependency').length - 1).toBe(1); + }); + + it.skipIf(!isolatedOk)('no local install, a global charter on PATH: runs it directly, never npm exec', () => { + const run = runHook({ stubs: ['npm', 'charter'], isolatePath: true }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls(GLOBAL)); + }); + + it.skipIf(!isolatedOk)('Yarn PnP without yarn: warns (with the corepack fix) and falls back to npm', () => { + const run = runHook({ files: { 'yarn.lock': '', '.pnp.cjs': '', ...LOCAL_BIN }, stubs: ['npm'], isolatePath: true }); expect(run.status).toBe(0); - expect(run.stderr).toContain('[pre-commit] Warning: this repo uses yarn, but yarn is not on PATH; running Charter with npm exec --no -- @stackbilt/cli (and verify:adf with npm run) instead.'); + expect(run.stderr).toContain(`[pre-commit] Warning: this repo uses yarn, but yarn is not on PATH; running Charter with ${HOOK_CLI_COMMANDS.npm} (and verify:adf with npm run) instead.`); expect(run.stderr).toContain('corepack enable'); - expect(run.calls).toEqual(expectedCalls(NPM_RUNNER)); + expect(run.calls).toEqual(expectedCalls(NPM)); }); it.skipIf(!isolatedOk)('PnP plus pnpm-lock.yaml without yarn: the cascade moves on to pnpm, with a warning', () => { const run = runHook({ files: { '.pnp.cjs': '', 'pnpm-lock.yaml': '' }, stubs: ['pnpm', 'npm'], isolatePath: true }); expect(run.status).toBe(0); - expect(run.stderr).toContain('this repo uses yarn, but yarn is not on PATH; running Charter with pnpm exec charter (and verify:adf with pnpm run) instead.'); - expect(run.calls).toEqual(expectedCalls('pnpm [exec] [charter]')); + expect(run.stderr).toContain(`this repo uses yarn, but yarn is not on PATH; running Charter with ${HOOK_CLI_COMMANDS.pnpm} (and verify:adf with pnpm run) instead.`); + expect(run.calls).toEqual(expectedCalls(PNPM)); }); it.skipIf(!isolatedOk)('both lockfiles with only yarn installed: yarn (the #365 cascade), not npm', () => { @@ -319,23 +387,23 @@ elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then isolatePath: true, }); expect(run.status).toBe(0); - expect(run.stderr).toContain('this repo uses pnpm, but pnpm is not on PATH; running Charter with yarn charter (and verify:adf with yarn run) instead.'); - expect(withoutVersionProbe(run.calls)).toEqual([...expectedCalls('yarn [charter]').slice(0, 2), 'yarn [run] [verify:adf] token-set']); + expect(run.stderr).toContain(`this repo uses pnpm, but pnpm is not on PATH; running Charter with ${HOOK_CLI_COMMANDS.yarn} (and verify:adf with yarn run) instead.`); + expect(withoutYarnVersion(run.calls)).toEqual([...expectedCalls(YARN).slice(0, 3), 'yarn [run] [verify:adf] token-set']); }); it.skipIf(!isolatedOk)('pnpm repo without pnpm: warns and falls back to npm exec / npm run', () => { const run = runHook({ - files: { 'pnpm-lock.yaml': '', 'package.json': JSON.stringify({ scripts: { 'verify:adf': 'x' } }) }, + files: { 'pnpm-lock.yaml': '', ...LOCAL_BIN, 'package.json': JSON.stringify({ scripts: { 'verify:adf': 'x' } }) }, stubs: ['npm'], isolatePath: true, }); expect(run.status).toBe(0); expect(run.stderr).toContain('this repo uses pnpm, but pnpm is not on PATH'); expect(run.stderr).not.toContain('corepack enable'); - expect(run.calls).toEqual([...expectedCalls(NPM_RUNNER).slice(0, 2), 'npm [run] [verify:adf] token-set']); + expect(run.calls).toEqual([...expectedCalls(NPM).slice(0, 3), 'npm [run] [verify:adf] token-set']); }); - it.skipIf(!isolatedOk)('prints no package-manager warning and makes no call without .ai/manifest.adf', () => { + it.skipIf(!isolatedOk)('prints nothing and makes no call without .ai/manifest.adf', () => { fs.rmSync(path.join(dir, '.ai'), { recursive: true }); const run = runHook({ files: { 'yarn.lock': '' }, stubs: ['npm'], isolatePath: true }); expect(run.status).toBe(0); @@ -359,7 +427,10 @@ elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then ["nodeLinker: 'pnp' # comment\n", true], ['nodeLinker: pnp\r\n', true], [' nodeLinker:pnp\n', true], + ['nodeLinker:\tpnp\t\n', true], ['yarnPath: .yarn/releases/yarn-4.5.0.cjs\nnodeLinker: pnp', true], + ['nodeLinker:\n pnp\n', false], + ['nodeLinker: pnp\nx', true], ['nodeLinker: pnpm\n', false], ['nodeLinker: node-modules\n', false], ['nodeLinker: pnp-loose\n', false], diff --git a/packages/cli/src/__tests__/hook.test.ts b/packages/cli/src/__tests__/hook.test.ts index 64b9617..3e87021 100644 --- a/packages/cli/src/__tests__/hook.test.ts +++ b/packages/cli/src/__tests__/hook.test.ts @@ -7,6 +7,7 @@ import { HOOK_SCOPED_DOCTOR_ENV, HOOK_SCOPED_DOCTOR_FILE, HOOK_CLI, + HOOK_CLI_PROBE_BLOCK, HOOK_CLI_RESOLVE_BLOCK, HOOK_TIDY_BLOCK_TEMPLATE, hookCommand, @@ -307,7 +308,7 @@ fi `; // A pre-#366 hook: no CLI resolution block, every call a bare `npx charter`. const toPre366 = (hook: string): string => hook - .replace(`${HOOK_CLI_RESOLVE_BLOCK}\n`, '') + .replace(`${HOOK_CLI_RESOLVE_BLOCK}${HOOK_CLI_PROBE_BLOCK}\n`, '') .split(HOOK_CLI).join('npx charter') .split('$CHARTER_RUN').join('npx charter'); const HOOK_360 = toPre366(PRE_COMMIT_HOOK_CONTENT.slice(0, PRE_COMMIT_HOOK_CONTENT.indexOf('# ADF evidence gate'))) + GATE_360; diff --git a/packages/cli/src/__tests__/integration/precommit-hook.test.ts b/packages/cli/src/__tests__/integration/precommit-hook.test.ts index e6b5b7d..b87c92f 100644 --- a/packages/cli/src/__tests__/integration/precommit-hook.test.ts +++ b/packages/cli/src/__tests__/integration/precommit-hook.test.ts @@ -129,7 +129,8 @@ function writeHookFixtureRepo(label: string, opts: FixtureOpts = {}): Repo { fs.writeFileSync(path.join(binDir, 'pnpm.cmd'), `@echo pnpm %*>> "${pnpmLog}"\r\n@exit /b 97\r\n`); fs.writeFileSync(shim, [ `const args = process.argv.slice(2);`, - `require('fs').appendFileSync(${JSON.stringify(log)}, args.join(' ') + '\\n');`, + // The hook's one --version probe (#366) is not logged, so the call lists stay about tidy and the gate. + `if (args[0] !== '--version') require('fs').appendFileSync(${JSON.stringify(log)}, args.join(' ') + '\\n');`, `if (process.env.HOOK_TEST_FAIL_TIDY === '1' && args[0] === 'adf' && args[1] === 'tidy') process.exit(3);`, // Test hooks for paths tidy reports: a ghost path in both runs, or an extra write in the real run only. `if (args[0] === 'adf' && args[1] === 'tidy' && args.includes('--print-paths')) process.on('exit', () => {`, @@ -179,9 +180,10 @@ function writeHookFixtureRepo(label: string, opts: FixtureOpts = {}): Repo { // The generated hook, as `charter hook install --pre-commit` writes it. const hookDir = path.join(tmp, '.githooks'); fs.mkdirSync(hookDir, { recursive: true }); - // #366: charter_cli is redefined right after the generated one, so package-manager - // detection still runs (verify:adf uses it) and every Charter call reaches the shim. - const full = PRE_COMMIT_HOOK_CONTENT.replace(HOOK_CLI_RESOLVE_BLOCK, `${HOOK_CLI_RESOLVE_BLOCK}${HOOK_CLI}() { node "${posix(shim)}" "$@"; }\n`); + // #366: charter_cli is redefined right after the generated one, ahead of the + // --version probe, so package-manager detection still runs (verify:adf uses it) + // and every Charter call, the probe included, reaches the shim. + const full = PRE_COMMIT_HOOK_CONTENT.replace(HOOK_CLI_RESOLVE_BLOCK, `${HOOK_CLI_RESOLVE_BLOCK}${HOOK_CLI}() { node "${posix(shim)}" "$@"; }\nCHARTER_STATE=ok\n`); expect(full).not.toBe(PRE_COMMIT_HOOK_CONTENT); const hook = opts.tidyOnly ? full.slice(0, full.indexOf('# ADF evidence gate')) : full; expect(hook).toContain('# Charter pre-commit vendor tidy: scoped to staged files'); diff --git a/packages/cli/src/commands/hook.ts b/packages/cli/src/commands/hook.ts index 956be2b..d92ef9b 100644 --- a/packages/cli/src/commands/hook.ts +++ b/packages/cli/src/commands/hook.ts @@ -184,7 +184,8 @@ if [ "\${CHARTER_SKIP_TIDY:-0}" != "1" ] && [ -f ".ai/manifest.adf" ]; then if [ -n "$SOURCES" ]; then echo "[pre-commit] Checking staged vendor files:$NAMES" - PLAN=$({CLI} adf tidy --dry-run --print-paths $SOURCES 2>/dev/null) || { echo "[pre-commit] adf tidy --dry-run failed; skipping auto-tidy."; PLAN=""; } + PLAN_OK=1 + PLAN=$({CLI} adf tidy --dry-run --print-paths $SOURCES 2>/dev/null) || { echo "[pre-commit] adf tidy --dry-run failed; skipping auto-tidy."; PLAN=""; PLAN_OK=0; } if printf '%s\\n' "$PLAN" | grep -E '^(vendor|module) ' >/dev/null; then BLOCKED="" IGNORED="" @@ -230,7 +231,8 @@ if [ "\${CHARTER_SKIP_TIDY:-0}" != "1" ] && [ -f ".ai/manifest.adf" ]; then fi echo "[pre-commit] Vendor files tidied; re-staged only the planned files tidy wrote." fi - else + elif [ "$PLAN_OK" = "1" ]; then + # A failed dry run has said so above; it proves nothing about the files. echo "[pre-commit] Vendor config files are clean." fi fi @@ -250,44 +252,52 @@ export const PNP_MARKER_FILES = ['.pnp.cjs', '.pnp.js'] as const; /** * POSIX ERE for a .yarnrc.yml line that selects the PnP linker (`nodeLinker: pnp`, - * optionally quoted, then a quote, space, comment or end of line, so `pnpm` and - * `pnp-loose` do not match). The hook greps with it; doctor uses - * yarnrcPnpLinkerRegExp(), the same pattern with [:space:] spelled \s. + * optionally quoted, then a quote, blank, comment or end of line, so `pnpm` and + * `pnp-loose` do not match). The hook greps with it, one line at a time; doctor + * uses yarnrcPnpLinkerRegExp(), the same pattern with [:space:] spelled + * [ \t\r\f\v] so that, like grep, it never matches across a newline. */ export const YARNRC_PNP_LINKER_ERE = `^[[:space:]]*nodeLinker:[[:space:]]*["']?pnp(["'[:space:]#]|$)`; /** YARNRC_PNP_LINKER_ERE as a JS RegExp over a whole file (multiline). */ export function yarnrcPnpLinkerRegExp(): RegExp { - return new RegExp(YARNRC_PNP_LINKER_ERE.split('[:space:]').join('\\s'), 'm'); + return new RegExp(YARNRC_PNP_LINKER_ERE.split('[:space:]').join(' \\t\\r\\f\\v'), 'm'); } +/** + * The exact commands charter_cli runs, which the hook also prints in its "Run:" + * hints (#366). The npm form names Charter's own package, never the bare name + * `charter` (an unrelated registry package); --no refuses to install, and + * --fetch-retries=0 makes any registry request fail at once rather than retry. + */ +export const HOOK_CLI_COMMANDS = { + yarn: 'yarn charter', + yarn1: 'yarn --silent charter', + pnpm: 'pnpm exec charter', + npm: 'npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli', + global: 'charter', +} as const; + /** Escape a string for the inside of a POSIX sh double-quoted word. */ function shDoubleQuoted(value: string): string { return value.replace(/[\\"$`]/g, ch => `\\${ch}`); } /** - * Picks the package manager and defines charter_cli (#366). POSIX sh, no output - * unless a warning is due (and then only when .ai/manifest.adf exists, as nothing - * else in the hook runs without it). The first candidate whose marker exists and - * whose binary is on PATH wins: the #365 verify:adf cascade with PnP in front. + * Picks the package manager and how to run Charter, once per hook run (#366). + * POSIX sh, no output. The first candidate whose marker exists and whose binary + * is on PATH wins: the #365 verify:adf cascade with PnP in front. * 1. Yarn Plug'n'Play (a PNP_MARKER_FILES file, or YARNRC_PNP_LINKER_ERE in * .yarnrc.yml) and yarn on PATH -> yarn * 2. pnpm-lock.yaml and pnpm on PATH -> pnpm * 3. yarn.lock and yarn on PATH -> yarn * 4. otherwise npm - * When the first marker found has no binary on PATH, the hook warns and names - * the runner it uses instead. - * charter_cli runs: yarn charter (yarn --silent charter on Yarn 1, whose run - * banner would otherwise land in the tidy block's parsed stdout), pnpm exec - * charter, or npm exec --no -- @stackbilt/cli. The npm form names Charter's own - * package, never the bare name `charter` (an unrelated registry package), and - * --no refuses to install. A local install runs with no registry request. - * Without one, npm requests @stackbilt/cli's metadata and may run a global - * install, but never installs or runs a downloaded package. - * --no-update-notifier skips npm's own version check. - * charter_cli_hint prints the devDependency advice only when charter_cli cannot - * run `--version`, so a doctor that ran and reported problems gets no such hint. + * CHARTER_RUN is the HOOK_CLI_COMMANDS entry for the result, and charter_cli runs + * exactly that. Under npm: a local install (node_modules/.bin/charter or + * charter.cmd) runs through npm exec, which then makes no registry request; + * without one, a `charter` on PATH (a global install) runs directly; with + * neither, CHARTER_STATE is "missing" and charter_cli runs nothing, so the hook + * makes no network request at all. */ export const HOOK_CLI_RESOLVE_BLOCK = `# Charter CLI resolution (#366): run Charter with the package manager this repo uses. ${PRE_COMMIT_PM_CLI_MARKER} @@ -314,18 +324,28 @@ elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then elif [ -f "yarn.lock" ] && command -v yarn >/dev/null 2>&1; then PM=yarn fi -CHARTER_YARN_FLAGS="" +# CHARTER_STATE: ok, missing (charter_cli runs nothing) or old (a 1.x CLI); set by the probe below. +CHARTER_STATE=ok case "$PM" in yarn) - CHARTER_RUN="yarn charter" + CHARTER_RUN="${HOOK_CLI_COMMANDS.yarn}" if [ -f ".ai/manifest.adf" ]; then case "$(yarn --version 2>/dev/null || true)" in - 1.*) CHARTER_YARN_FLAGS="--silent"; CHARTER_RUN="yarn --silent charter" ;; + 1.*) CHARTER_RUN="${HOOK_CLI_COMMANDS.yarn1}" ;; esac fi ;; - pnpm) CHARTER_RUN="pnpm exec charter" ;; - *) CHARTER_RUN="npm exec --no -- @stackbilt/cli" ;; + pnpm) CHARTER_RUN="${HOOK_CLI_COMMANDS.pnpm}" ;; + *) + CHARTER_RUN="${HOOK_CLI_COMMANDS.npm}" + if [ ! -f "node_modules/.bin/charter" ] && [ ! -f "node_modules/.bin/charter.cmd" ]; then + if command -v charter >/dev/null 2>&1; then + CHARTER_RUN="${HOOK_CLI_COMMANDS.global}" + else + CHARTER_STATE=missing + fi + fi + ;; esac if [ "$PM" != "$LOCK_PM" ] && [ -f ".ai/manifest.adf" ]; then echo "[pre-commit] Warning: this repo uses $LOCK_PM, but $LOCK_PM is not on PATH; running Charter with $CHARTER_RUN (and verify:adf with $PM run) instead." >&2 @@ -333,26 +353,49 @@ if [ "$PM" != "$LOCK_PM" ] && [ -f ".ai/manifest.adf" ]; then echo "[pre-commit] Yarn Plug'n'Play needs yarn to load packages. To fix: corepack enable (or install yarn), then commit again." >&2 fi fi +# Runs exactly the command the hints print ($CHARTER_RUN), or nothing when Charter is missing. charter_cli() { - case "$PM" in - yarn) yarn $CHARTER_YARN_FLAGS charter "$@" ;; - pnpm) pnpm exec charter "$@" ;; - *) npm exec --no --no-update-notifier -- @stackbilt/cli "$@" ;; - esac -} -charter_cli_hint() { - if ! charter_cli --version >/dev/null 2>&1; then - echo " Charter itself could not be run with: $CHARTER_RUN" - echo ' Add @stackbilt/cli as a devDependency and install it. This hook never installs Charter.' + if [ "$CHARTER_STATE" = "missing" ]; then + return 127 fi + $CHARTER_RUN "$@" } `; +/** + * Runs `charter_cli --version` once per hook run, before any other Charter call + * (#366), and only when .ai/manifest.adf exists. A CLI that cannot run sets + * CHARTER_STATE=missing: the hook says so once, skips tidy, and the gate fails + * without calling Charter again. A 1.x CLI (it ignores --staged and + * --print-paths, so tidy would do nothing and the gate would check every file) + * sets CHARTER_STATE=old and gets a one-time upgrade warning. A separate block + * so that tests can replace charter_cli between the two. + */ +export const HOOK_CLI_PROBE_BLOCK = `# Charter CLI probe (#366): one --version call per hook run. +if [ -f ".ai/manifest.adf" ]; then + CHARTER_VERSION="" + if [ "$CHARTER_STATE" != "missing" ]; then + CHARTER_VERSION=$(${HOOK_CLI} --version 2>/dev/null) || CHARTER_STATE=missing + fi + case "$CHARTER_STATE:$CHARTER_VERSION" in + missing:*) + echo "[pre-commit] Charter could not be run with: $CHARTER_RUN" >&2 + echo '[pre-commit] Add @stackbilt/cli as a devDependency and install it. This hook never installs Charter.' >&2 + CHARTER_SKIP_TIDY=1 + ;; + *"charter v0."*|*"charter v1."*) + CHARTER_STATE=old + echo "[pre-commit] Warning: $CHARTER_RUN --version reports $CHARTER_VERSION; this hook needs @stackbilt/cli 2.x (1.x ignores --staged and --print-paths, so tidy does nothing and the gate checks every file). Install or upgrade the devDependency: @stackbilt/cli@^2." >&2 + ;; + esac +fi +`; + export const PRE_COMMIT_HOOK_CONTENT = `#!/usr/bin/env sh ${PRE_COMMIT_HOOK_MARKER} set -eu -${HOOK_CLI_RESOLVE_BLOCK} +${HOOK_CLI_RESOLVE_BLOCK}${HOOK_CLI_PROBE_BLOCK} ${renderHookTidyBlock(HOOK_CLI)} # ADF evidence gate: check LOC ceilings if manifest exists ${PRE_COMMIT_GATE_SCOPE_MARKER} @@ -370,7 +413,9 @@ if [ -f ".ai/manifest.adf" ]; then else echo " Run: $CHARTER_RUN doctor --adf-only --ci --staged --format text" fi - charter_cli_hint + if [ "$CHARTER_STATE" = "missing" ]; then + echo ' Charter was not run: it could not be found (see the note above).' + fi exit 1 } if [ -f "package.json" ] && grep -q '"verify:adf"' package.json; then @@ -402,7 +447,9 @@ if [ -f ".ai/manifest.adf" ]; then echo '' echo '[pre-commit] ADF verification FAILED.' echo " Run: $CHARTER_RUN adf evidence --auto-measure --ci" - charter_cli_hint + if [ "$CHARTER_STATE" = "missing" ]; then + echo ' Charter was not run: it could not be found (see the note above).' + fi exit 1 } fi @@ -637,11 +684,13 @@ function printHelp(): void { console.log(' Bloat is routed to .adf modules; only the files tidy wrote are re-staged. A vendor file'); console.log(' with unstaged changes is skipped. The gate (doctor --staged) blocks on vendor bloat only in'); console.log(' staged files tidy did not skip. If package.json has a verify:adf script, it runs next,'); - console.log(' with the first package manager whose lockfile (or Plug\'n\'Play marker) exists and that'); - console.log(' is on PATH (npm if none), and its doctor leaves vendor bloat to that scoped check.'); - console.log(' Every Charter call runs through the same package manager: yarn charter, pnpm exec'); - console.log(' charter, or npm exec --no -- @stackbilt/cli, none of which installs a package.'); - console.log(' Re-run this command to upgrade an older hook.'); + console.log(' with the package manager below, and its doctor leaves vendor bloat to that scoped check.'); + console.log(' Package manager: the first match of (1) Yarn Plug\'n\'Play (.pnp.cjs, .pnp.js, or .yarnrc.yml nodeLinker: pnp)'); + console.log(' with yarn on PATH, (2) pnpm-lock.yaml with pnpm, (3) yarn.lock with yarn, (4) npm.'); + console.log(` Charter then runs as: ${HOOK_CLI_COMMANDS.yarn} (${HOOK_CLI_COMMANDS.yarn1} on Yarn 1),`); + console.log(` ${HOOK_CLI_COMMANDS.pnpm}, or ${HOOK_CLI_COMMANDS.npm}`); + console.log(` (npm, with node_modules/.bin/charter; else a global ${HOOK_CLI_COMMANDS.global} on PATH).`); + console.log(' The hook never installs Charter. Re-run this command to upgrade an older hook.'); console.log(' Skip tidy with CHARTER_SKIP_TIDY=1. Only gates when .ai/manifest.adf exists.'); console.log(''); console.log(' print --claude: Print the Claude Code session hook config snippet to stdout.'); From 9d0f5a38ac14b1b353e38a5cad9620f0c24c71b8 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Sat, 17 Oct 2026 21:02:35 -0500 Subject: [PATCH 4/4] fix(hook): global fallback for non-PnP Yarn, clearer probe outcomes, tidy notes (#366) Round 4 on review nits: - Yarn with a node_modules linker and no node_modules/.bin/charter now runs a global `charter` from PATH (as 2.1.0's `npx charter` found it), else reports Charter missing with no calls. Yarn PnP stays `yarn charter`. pnpm reports missing when neither a local bin nor a global charter exists (pnpm exec searches PATH itself). - The --version probe keeps stderr. A found binary whose --version fails is "installed but failed to run: ", with no devDependency hint. Output without `charter v` warns that the charter found is not @stackbilt/cli and runs nothing else. `charter v10.0.0` is not flagged as 1.x (tested). The 1.x warning says to upgrade @stackbilt/cli to 2.x (as a devDependency, or your global install). - With nothing resolved, the gate prints no Run: command. - Tidy's stderr notes from the dry run (an unmanaged file it skips) are shown under "adf tidy notes:", and the hook no longer then claims the vendor files are clean (template and .githooks/pre-commit). - Docs: one table for local/global/neither per package manager in cli-reference; upgrading-to-2.0 and the CHANGELOG describe the global behaviour for npm, pnpm and Yarn. Co-Authored-By: Claude Opus 5.5 --- .githooks/pre-commit | 12 +- CHANGELOG.md | 29 ++-- docs/cli-reference.md | 35 +++-- docs/upgrading-to-2.0.md | 2 +- packages/cli/src/__tests__/hook-pm.test.ts | 105 ++++++++++++-- packages/cli/src/__tests__/hook.test.ts | 2 +- packages/cli/src/commands/hook.ts | 151 +++++++++++++++------ 7 files changed, 259 insertions(+), 77 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 658020d..c898882 100644 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -37,7 +37,13 @@ if [ "${CHARTER_SKIP_TIDY:-0}" != "1" ] && [ -f ".ai/manifest.adf" ]; then if [ -n "$SOURCES" ]; then echo "[pre-commit] Checking staged vendor files:$NAMES" PLAN_OK=1 - PLAN=$(node packages/cli/dist/bin.js adf tidy --dry-run --print-paths $SOURCES 2>/dev/null) || { echo "[pre-commit] adf tidy --dry-run failed; skipping auto-tidy."; PLAN=""; PLAN_OK=0; } + # stderr is kept: tidy explains there a file it leaves alone (say, an unmanaged one). + PLAN=$(node packages/cli/dist/bin.js adf tidy --dry-run --print-paths $SOURCES 2>&1) || { echo "[pre-commit] adf tidy --dry-run failed; skipping auto-tidy."; PLAN=""; PLAN_OK=0; } + NOTES=$(printf '%s\n' "$PLAN" | grep -v -E '^(vendor|module) ' | grep -v '^ *$') || NOTES="" + if [ -n "$NOTES" ]; then + echo "[pre-commit] adf tidy notes:" + printf '%s\n' "$NOTES" + fi if printf '%s\n' "$PLAN" | grep -E '^(vendor|module) ' >/dev/null; then BLOCKED="" IGNORED="" @@ -83,8 +89,8 @@ if [ "${CHARTER_SKIP_TIDY:-0}" != "1" ] && [ -f ".ai/manifest.adf" ]; then fi echo "[pre-commit] Vendor files tidied; re-staged only the planned files tidy wrote." fi - elif [ "$PLAN_OK" = "1" ]; then - # A failed dry run has said so above; it proves nothing about the files. + elif [ "$PLAN_OK" = "1" ] && [ -z "$NOTES" ]; then + # A failed dry run, or one that left a file alone, has said so above. echo "[pre-commit] Vendor config files are clean." fi fi diff --git a/CHANGELOG.md b/CHANGELOG.md index c1655ef..7d427fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,18 +22,23 @@ The format is based on Keep a Changelog and follows Semantic Versioning. This is #365's `verify:adf` cascade with PnP in front, and `verify:adf` uses the same choice. When the first marker found has no binary on `PATH`, the hook warns on stderr with the runner it uses instead; under PnP the warning adds `corepack enable`. - Charter runs as `yarn charter` (`yarn --silent charter` on Yarn 1) or `pnpm exec charter`. Under npm the hook checks for a local install first: - - With `node_modules/.bin/charter` (or `charter.cmd`): `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`. It names Charter's own package, never the bare name `charter`, and makes no registry request. - - Without one: a global `charter` on `PATH`, run directly. - - With neither: no Charter call at all, so no network request. The hook says once to add `@stackbilt/cli` as a devDependency, skips tidy, and fails the gate. - - The hook never installs a package or runs a downloaded one. The only network use left is Corepack downloading Yarn or pnpm on first use. Measured offline with npm 11.17, a commit took 6 s with a local install, 0 s with no install at all, and 2 s with a global one. The `Run:` hints print exactly the command the hook ran. - - The hook runs `--version` once per commit, before any other Charter call: - - If it fails, the devDependency hint is printed once and Charter is not called again. - - If it reports 1.x, the hook warns once to upgrade to `@stackbilt/cli` 2.x, since 1.x ignores `--staged` and `--print-paths`. - - A doctor that ran and reported findings gets no devDependency hint. A failed tidy dry run no longer goes on to say "Vendor config files are clean." `charter doctor` and the hook now detect PnP from one shared source. The `CHARTER_HOOK_SCOPED_DOCTOR` token flow is unchanged. Scope: this fix covers the pre-commit hook only; the CI workflow `charter setup` generates still calls `npx charter` (tracked separately). **Installed hooks do not update themselves:** run `charter hook install --pre-commit` again (no `--force` needed for a Charter-managed hook). This is needed under Yarn PnP and recommended everywhere else. `charter doctor` reports an older hook as INFO in a PnP repo. + How Charter runs depends on the package manager. A local install means `node_modules/.bin/charter` (or `charter.cmd`) exists; a global install means a `charter` on `PATH`. + - **Yarn Plug'n'Play:** always `yarn charter`. There is no `node_modules` to check, so a global install is never used. + - **Yarn with a `node_modules` linker:** `yarn charter` (`yarn --silent charter` on Yarn 1) with a local install. Otherwise a global `charter`, run directly, as 2.1.0's `npx charter` would have found it. + - **pnpm:** `pnpm exec charter`, which also finds a global install on `PATH`. + - **npm:** with a local install, `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`. It names Charter's own package, never the bare name `charter`, and makes no registry request. Otherwise a global `charter`, run directly. + - **Nothing found:** no Charter or package-manager call at all, so no network request. The hook says once to add `@stackbilt/cli` as a devDependency, skips tidy, and fails the gate with no `Run:` command. + + The hook never installs a package or runs a downloaded one. The only network use left is Corepack downloading Yarn or pnpm on first use. Measured offline with npm 11.17, a commit took 6-7 s with a local install, 0 s with no install at all, and 2 s with a global one. The `Run:` hints print exactly the command the hook ran. + + The hook runs `--version` once per commit, before any other Charter call, and keeps its stderr: + - **Not resolvable:** the devDependency hint, once. + - **Installed, but `--version` failed:** `Charter is installed but failed to run:` with the first error line, and no devDependency hint. + - **Output without `charter v`:** a warning that the `charter` found is not `@stackbilt/cli`. + - In each of those three cases, Charter is not called again. + - **1.x:** one warning to upgrade `@stackbilt/cli` to 2.x (as a devDependency, or your global install), since 1.x ignores `--staged` and `--print-paths`. + + A doctor that ran and reported findings gets no devDependency hint. When tidy leaves a staged vendor file alone (for example, an unmanaged one), its note is shown under `[pre-commit] adf tidy notes:`. Neither that case nor a failed tidy dry run goes on to say "Vendor config files are clean." `charter doctor` and the hook now detect PnP from one shared source. The `CHARTER_HOOK_SCOPED_DOCTOR` token flow is unchanged. Scope: this fix covers the pre-commit hook only; the CI workflow `charter setup` generates still calls `npx charter` (tracked separately). **Installed hooks do not update themselves:** run `charter hook install --pre-commit` again (no `--force` needed for a Charter-managed hook). This is needed under Yarn PnP and recommended everywhere else. `charter doctor` reports an older hook as INFO in a PnP repo. - **`charter adf populate`, and so every `charter bootstrap`, re-added stack constraints to a `CONSTRAINTS` section you had edited** (`#371`; 2.1.0 listed this as still open under `#354`). Deleting a bullet from `core.adf` and re-running bootstrap brought a stack rule back (for example the Cloudflare `No Node.js-specific APIs in Worker handlers` rule), because populate appended any detected stack constraint it did not find. Populate now remembers what it has added or offered in a new file, `.ai/.populate.json` (`{ "version": 1, "constraints": { "core.adf": { "added": [...], "offered": [...] } } }`, sorted, written only when it changes). Commit it with the rest of `.ai/`; bootstrap lists it under what it created, and its `git add .ai` next step already includes it. The rules: - A constraint recorded as added or offered is never added again, whether it is still in the section or you deleted it. Deleting the one rule populate added to a plain Worker repo now sticks, and a deleted `CONSTRAINTS` section is not recreated once the file has a record. - New constraints go only into a section that is untouched: the template's bullets, in order, followed by nothing but bullets populate recorded as added. A missing section is created only while there is no record. It goes right after whichever of `GUIDE` and `CONTEXT` comes later, which in the scaffold's order is its own place, rather than at the end of the file (when populate adds a missing `CONTEXT` in the same run, that one is appended at the end and `CONSTRAINTS` follows it); its header is written as the scaffold writes it (`⚠️ CONSTRAINTS [load-bearing]:`). diff --git a/docs/cli-reference.md b/docs/cli-reference.md index bf87c49..b0a68b7 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -86,18 +86,29 @@ npx charter hook install --commit-msg --force # overwrite existing hooks 4. Otherwise: npm. So a repo with both lockfiles uses pnpm, or Yarn when only Yarn is installed. If the first marker found has no binary on `PATH`, the hook prints a warning on stderr naming the runner it uses instead. Under Plug'n'Play the warning adds `corepack enable`, since PnP packages load only through Yarn. `verify:adf` runs with the same package manager (` run verify:adf`). - - Charter then runs as exactly one of these commands, and the hook's `Run:` hints print the same command: - - Yarn: `yarn charter`, or `yarn --silent charter` on Yarn 1, so its run banner stays out of the output tidy parses. - - pnpm: `pnpm exec charter`. - - npm with a local install (`node_modules/.bin/charter` or `charter.cmd` exists): `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`. This names Charter's own package, never the bare name `charter`: that is an unrelated registry package, which a plain `npx charter` fetches where Charter is not installed. It makes no registry request. - - npm without a local install, but a `charter` on `PATH`: that global `charter`, run directly. - - npm with neither: nothing. The hook says once that Charter could not be run and to add `@stackbilt/cli` as a devDependency, skips tidy, and fails the gate without calling Charter or npm, so it makes no network request. - - The hook never installs a package or runs a downloaded one. The only network use left is Corepack downloading Yarn or pnpm itself the first time the hook calls one. Measured offline (registry unreachable, npm 11.17): a commit with a local install took 6 s, one with neither a local nor a global install 0 s, and one with a global install 2 s. - - **Once per commit,** before any other Charter call, the hook runs Charter's `--version`: - - If it fails (Charter not resolvable with that command), the hook prints the devDependency hint once and skips the rest as above. - - If it reports 1.x, the hook warns once that it needs `@stackbilt/cli` 2.x. A 1.x CLI ignores `--staged` and `--print-paths`, so its tidy does nothing and its gate checks every file. The hook still runs it. - - When a gate step fails after that, the hook prints only the command to re-run. A doctor that ran and reported problems gets no devDependency hint. - - **Global installs** are used only under npm without a local install, and in any version, so a global 1.x CLI gets the warning above. Install `@stackbilt/cli` as a devDependency, so the hook runs the version the repo pins. + - Charter then runs as exactly one of these commands, and the hook's `Run:` hints print the same command. A local install means `node_modules/.bin/charter` (or `charter.cmd`) exists; a global install means a `charter` on `PATH`. + + | Package manager | Local install | Global install only | Neither | + |---|---|---|---| + | Yarn Plug'n'Play | `yarn charter` | `yarn charter` (no global fallback) | `yarn charter` | + | Yarn, `node_modules` linker | `yarn charter` (`yarn --silent charter` on Yarn 1) | `charter`, run directly | nothing | + | pnpm | `pnpm exec charter` | `pnpm exec charter` | nothing | + | npm | `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli` | `charter`, run directly | nothing | + + - **Yarn Plug'n'Play** has no `node_modules` to check, so the hook always runs `yarn charter`, which resolves only a dependency of the repo. A global install is not used there. + - **Yarn with a `node_modules` linker:** `yarn charter` would not find a global install, so the hook runs the global `charter` itself. Yarn 1 gets `--silent` so its run banner stays out of the output tidy parses. + - **pnpm:** `pnpm exec` looks in `node_modules/.bin` first and then on `PATH`, so it also finds a global install. + - **npm:** the command names Charter's own package, never the bare name `charter`. That is an unrelated registry package, which a plain `npx charter` fetches where Charter is not installed. With a local install it makes no registry request. + - **Nothing found** (no local install, and no global one where the table falls back to it): the hook calls neither Charter nor the package manager. It prints once that Charter could not be found and to add `@stackbilt/cli` as a devDependency, skips tidy, and fails the gate. It prints no `Run:` command, since none would work. + - The hook never installs a package or runs a downloaded one. The only network use left is Corepack downloading Yarn or pnpm itself the first time the hook calls one. Measured offline (registry unreachable, npm 11.17): a commit with a local install took 6-7 s, one with no install at all 0 s, and one with only a global install 2 s. + - **Once per commit,** before any other Charter call, the hook runs Charter's `--version` and keeps its stdout and stderr: + - **Exit 127, or no binary was found** (as under Plug'n'Play when Yarn cannot resolve it): the devDependency hint once, then as "nothing found" above. + - **A binary exists but `--version` fails:** `Charter is installed but failed to run:` and the first line of its error, with no devDependency hint. Tidy is skipped and the gate fails, still printing the `Run:` command so you can reproduce it. + - **Output with no `charter v` in it:** the hook warns that the `charter` it found is not `@stackbilt/cli`, runs nothing else, and fails the gate. + - **`charter v0.x` or `v1.x`:** one warning to upgrade `@stackbilt/cli` to 2.x (as a devDependency, or your global install). A 1.x CLI ignores `--staged` and `--print-paths`, so its tidy does nothing and its gate checks every file. The hook still runs it. + - When a gate step fails after a successful probe, the hook prints only the command to re-run. A doctor that ran and reported problems gets no devDependency hint. + - **Global installs** of any version are used as the table shows, so a global 1.x CLI gets the warning above. Install `@stackbilt/cli` as a devDependency, so the hook runs the version the repo pins. + - Tidy's own notes about a staged vendor file it leaves alone (for example, one not managed by Charter) are printed under `[pre-commit] adf tidy notes:`. The hook then does not say the vendor files are clean. - On Windows the hook runs under Git for Windows' `sh`, which finds `yarn`, `pnpm` and `npm` as an extensionless shim (as npm and corepack install them) or an `.exe`. A package manager present only as a `.cmd` file is not found there, so the cascade skips it. A `package.json` script named `charter` shadows the binary for `yarn charter`. - The doctor inside `verify:adf` would check vendor bloat across the whole tree again. To prevent that, the hook sets `CHARTER_HOOK_SCOPED_DOCTOR` to a one-time token for that run and writes the token to its own file in the git directory (`charter-hook-scoped-doctor-`, one per hook run, so overlapping commits in one worktree do not interfere), which it removes on exit. If the file cannot be written, the hook prints one `[pre-commit]` line saying so, and that time the doctor inside `verify:adf` checks every vendor file. When the two match, doctor reports `adf vendor bloat (checked by pre-commit hook)` as INFO and prints a line on stderr. All its other checks still block, and so do the script's other commands. The variable without the matching file does nothing, so `charter doctor --ci` in CI keeps full strictness even if the variable is set there. Run by hand, `npm run verify:adf` checks every vendor file, as CI does. - A hook installed before [#320](https://github.com/Stackbilt-dev/charter/issues/320) re-stages all of `.ai/*.adf`. A hook installed before #357 blocks on vendor bloat outside the commit, and so, in a repo with a `verify:adf` script, does one from the first #357 fix, which also ran `pnpm run verify:adf` by name. Re-run `charter hook install --pre-commit` to replace any of them (no `--force` needed for a Charter-managed hook). `charter doctor` reports an old hook as INFO. It reports a hook from the first #357 fix only where `package.json` has a `verify:adf` script, since without one that hook already runs the scoped gate. A hook installed before #366 calls a bare `npx charter`. It fails under Yarn Plug'n'Play unless Charter is installed globally, and where Charter is not installed at all it fetches the unrelated `charter` package. Reinstalling is recommended in every repo and needed under Plug'n'Play, where `charter doctor` reports such a hook as INFO. Elsewhere `npx charter` still finds `node_modules/.bin`, so doctor shows no row for it. diff --git a/docs/upgrading-to-2.0.md b/docs/upgrading-to-2.0.md index 2b94e59..6e88eef 100644 --- a/docs/upgrading-to-2.0.md +++ b/docs/upgrading-to-2.0.md @@ -91,7 +91,7 @@ The new pre-commit hook calls `charter adf tidy --print-paths` and `charter doct > **`verify:adf` script.** `charter setup` and `bootstrap` add a `verify:adf` script. The hook runs it after the scoped doctor, with your package manager. That is the first match in this order: Yarn for a Plug'n'Play repo (`.pnp.cjs`, `.pnp.js`, or `.yarnrc.yml` with `nodeLinker: pnp`) with yarn installed, then `pnpm` (with `pnpm-lock.yaml` and pnpm installed), then `yarn` (with `yarn.lock` and yarn installed), otherwise `npm`. pnpm is no longer required. Inside the hook, the doctor in `verify:adf` reports vendor bloat as INFO, `adf vendor bloat (checked by pre-commit hook)`, because the scoped check already ran. Its other checks still block. Running `npm run verify:adf` (or your package manager's equivalent) by hand or in CI still checks every vendor file. The hook runs the scoped `charter doctor …` even if your custom `verify:adf` leaves doctor out. > -> **How the hook runs Charter** ([#366](https://github.com/Stackbilt-dev/charter/issues/366)): every Charter call goes through that same package manager, as `yarn charter`, `pnpm exec charter` or (with a local install) `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`. This works under Yarn Plug'n'Play, and the hook never installs or runs a downloaded package. Install `@stackbilt/cli` as a devDependency. Under npm without a local install, the hook runs a global `charter` from `PATH` (any version; it warns about 1.x), or, with none, skips Charter and fails the gate with a hint. A hook installed before this change calls a bare `npx charter`, so re-run `charter hook install --pre-commit` (required under Plug'n'Play). See the `[Unreleased]` entry in the [CHANGELOG](../CHANGELOG.md) and [`charter hook`](./cli-reference.md#charter-hook) for details. +> **How the hook runs Charter** ([#366](https://github.com/Stackbilt-dev/charter/issues/366)): every Charter call goes through that same package manager, as `yarn charter`, `pnpm exec charter` or (with a local install) `npm exec --no --no-update-notifier --fetch-retries=0 -- @stackbilt/cli`. This works under Yarn Plug'n'Play, and the hook never installs or runs a downloaded package. Install `@stackbilt/cli` as a devDependency. Without a local install, a global `charter` on `PATH` is used under npm and under Yarn with a `node_modules` linker (the hook runs it directly), and under pnpm (`pnpm exec` searches `PATH`). Under Yarn Plug'n'Play it is never used, since there is no `node_modules` to check. A global CLI of any version is used, and the hook warns about 1.x. With nothing found, the hook skips Charter and fails the gate with a hint. A hook installed before this change calls a bare `npx charter`, so re-run `charter hook install --pre-commit` (required under Plug'n'Play). See the `[Unreleased]` entry in the [CHANGELOG](../CHANGELOG.md) and [`charter hook`](./cli-reference.md#charter-hook) for details. ## What doctor's new lines mean diff --git a/packages/cli/src/__tests__/hook-pm.test.ts b/packages/cli/src/__tests__/hook-pm.test.ts index 9e49349..58e317c 100644 --- a/packages/cli/src/__tests__/hook-pm.test.ts +++ b/packages/cli/src/__tests__/hook-pm.test.ts @@ -66,7 +66,11 @@ esac if [ "$is_charter" = "1" ]; then if [ "\${STUB_UNRESOLVED:-0}" = "1" ]; then echo "${name}: charter: command not found" >&2; exit 127; fi case " $* " in - *" --version "*) echo "\${STUB_CHARTER_VERSION:-charter v2.1.0}"; exit 0 ;; + *" --version "*) + if [ "\${STUB_VERSION_FAIL:-0}" = "1" ]; then echo "Error: Cannot find module 'commander'" >&2; exit 1; fi + echo "\${STUB_CHARTER_VERSION:-charter v2.1.0}"; exit 0 ;; + *" --dry-run "*) + if [ -n "\${STUB_TIDY_NOTE:-}" ]; then echo "\${STUB_TIDY_NOTE}" >&2; fi ;; esac fi if [ -n "\${STUB_FAIL_ON:-}" ]; then @@ -106,6 +110,12 @@ interface Fixture { charterVersion?: string; failOn?: string; unresolved?: boolean; + /** --version exits 1 with an error on stderr (an installed but broken CLI). */ + versionFails?: boolean; + /** A line the tidy dry run writes to stderr. */ + tidyNote?: string; + /** Write node_modules/.bin/charter (a local install). Default true. */ + localBin?: boolean; /** Use a PATH with every real package manager and charter removed, so only the stubs exist. */ isolatePath?: boolean; /** Hook text to run instead of PRE_COMMIT_HOOK_CONTENT (mutation checks). */ @@ -156,7 +166,8 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack function runHook(fx: Fixture): HookRun { fs.writeFileSync(path.join(dir, 'hook.sh'), fx.hook ?? PRE_COMMIT_HOOK_CONTENT); - for (const [name, content] of Object.entries(fx.files ?? {})) { + const files = { ...(fx.localBin === false ? {} : LOCAL_BIN), ...(fx.files ?? {}) }; + for (const [name, content] of Object.entries(files)) { fs.mkdirSync(path.dirname(path.join(dir, name)), { recursive: true }); fs.writeFileSync(path.join(dir, name), content); } @@ -173,6 +184,8 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack STUB_YARN_VERSION: fx.yarnVersion ?? '4.5.0', STUB_CHARTER_VERSION: fx.charterVersion ?? 'charter v2.1.0', STUB_FAIL_ON: fx.failOn ?? '', + STUB_VERSION_FAIL: fx.versionFails ? '1' : '0', + STUB_TIDY_NOTE: fx.tidyNote ?? '', STUB_UNRESOLVED: fx.unresolved ? '1' : '0', }; delete env.CHARTER_HOOK_SCOPED_DOCTOR; @@ -253,7 +266,7 @@ describe.skipIf(!shAvailable)('pre-commit hook resolves Charter through the pack ['no lockfile', LOCAL_BIN], ['charter.cmd only (Windows install)', { 'node_modules/.bin/charter.cmd': '@echo off\r\n' }], ])('npm with a local install (%s): npm exec @stackbilt/cli, never the bare name charter', (_label, files) => { - const run = runHook({ files }); + const run = runHook({ files, localBin: 'node_modules/.bin/charter' in files }); expect(run.status).toBe(0); expect(run.calls).toEqual(expectedCalls(NPM)); }); @@ -322,22 +335,60 @@ elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then const run = runHook({ files, unresolved: true }); expect(run.status).toBe(1); expect(withoutYarnVersion(run.calls)).toEqual([`${logged(command)} [--version]`]); - expect(run.stderr).toContain(`[pre-commit] Charter could not be run with: ${command}`); + expect(run.stderr).toContain(`[pre-commit] Charter could not be found (tried: ${command}).`); expect(run.stderr.split('Add @stackbilt/cli as a devDependency').length - 1).toBe(1); expect(run.stdout).not.toContain('Checking staged vendor files'); expect(run.stdout).toContain('[pre-commit] ADF verification FAILED.'); - expect(run.stdout).toContain('Charter was not run: it could not be found'); + expect(run.stdout).toContain(' Charter was not run (see the note above).'); + expect(run.stdout).not.toContain('Run:'); }); it('a 1.x CLI: one upgrade warning naming 2.x; the hook still runs it', () => { const run = runHook({ files: { 'pnpm-lock.yaml': '' }, charterVersion: 'charter v1.9.3' }); expect(run.status).toBe(0); - expect(run.stderr).toContain(`[pre-commit] Warning: ${HOOK_CLI_COMMANDS.pnpm} --version reports charter v1.9.3; this hook needs @stackbilt/cli 2.x`); - expect(run.stderr).toContain('Install or upgrade the devDependency: @stackbilt/cli@^2.'); + expect(run.stderr).toContain(`[pre-commit] Warning: ${HOOK_CLI_COMMANDS.pnpm} --version reports charter v1.9.3; this hook needs 2.x`); + expect(run.stderr).toContain('Upgrade @stackbilt/cli to 2.x (as a devDependency, or your global install).'); expect(run.stderr.split('Warning:').length - 1).toBe(1); expect(run.calls).toEqual(expectedCalls(PNPM)); }); + it('an installed CLI whose --version fails: "installed but failed to run" with its error, no devDependency hint', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': '' }, versionFails: true }); + expect(run.status).toBe(1); + expect(run.stderr).toContain("[pre-commit] Charter is installed but failed to run: Error: Cannot find module 'commander'"); + expect(run.stdout + run.stderr).not.toContain('devDependency'); + expect(run.calls).toEqual([`${PNPM} [--version]`]); + expect(run.stdout).toContain(` Run: ${HOOK_CLI_COMMANDS.pnpm} doctor --adf-only --ci --staged --format text`); + expect(run.stdout).toContain(' Charter was not run: it failed to start (see the note above).'); + }); + + it('a charter whose --version is not charter v: warns it is not @stackbilt/cli and runs nothing else', () => { + const run = runHook({ files: LOCAL_BIN, charterVersion: 'Usage: charter ' }); + expect(run.status).toBe(1); + expect(run.stderr).toContain(`[pre-commit] Warning: the charter this hook found (${HOOK_CLI_COMMANDS.npm}) is not @stackbilt/cli: its --version printed: Usage: charter `); + expect(run.calls).toEqual([`${NPM} [--version]`]); + expect(run.stdout).not.toContain('Run:'); + }); + + it.each([['charter v10.0.0'], ['charter v2.1.0'], ['charter v3.0.0-beta.1']])('%s is not flagged as 1.x or foreign', (version) => { + const run = runHook({ files: LOCAL_BIN, charterVersion: version }); + expect(run.status).toBe(0); + expect(run.stderr).toBe(''); + expect(run.calls).toEqual(expectedCalls(NPM)); + }); + + it.each([['charter v0.9.0'], ['charter v1.0.0']])('%s is flagged as 1.x', (version) => { + const run = runHook({ files: LOCAL_BIN, charterVersion: version }); + expect(run.stderr).toContain(`--version reports ${version}; this hook needs 2.x`); + }); + + it('tidy that leaves a staged file alone (say, unmanaged) shows its note and does not claim the files are clean', () => { + const note = ' ⚠ CLAUDE.md is not managed by Charter; tidy leaves it as is.'; + const run = runHook({ files: LOCAL_BIN, tidyNote: note }); + expect(run.stdout).toContain(`[pre-commit] adf tidy notes:\n${note}`); + expect(run.stdout).not.toContain('Vendor config files are clean'); + }); + it('a failed tidy dry run does not claim the vendor files are clean', () => { const run = runHook({ files: LOCAL_BIN, failOn: 'dry-run' }); expect(run.stdout).toContain('[pre-commit] adf tidy --dry-run failed; skipping auto-tidy.'); @@ -352,19 +403,53 @@ elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then describe('npm without a local install, and missing binaries', () => { it.skipIf(!isolatedOk)('no local install, no global charter: no Charter call at all, the hint once, the gate fails', () => { - const run = runHook({ stubs: ['npm', 'npx'], isolatePath: true }); + const run = runHook({ stubs: ['npm', 'npx'], isolatePath: true, localBin: false }); expect(run.status).toBe(1); expect(run.calls).toEqual([]); - expect(run.stderr).toContain(`[pre-commit] Charter could not be run with: ${HOOK_CLI_COMMANDS.npm}`); + expect(run.stderr).toContain(`[pre-commit] Charter could not be found (tried: ${HOOK_CLI_COMMANDS.npm}).`); + expect(run.stdout).not.toContain('Run:'); expect(run.stderr.split('Add @stackbilt/cli as a devDependency').length - 1).toBe(1); }); it.skipIf(!isolatedOk)('no local install, a global charter on PATH: runs it directly, never npm exec', () => { - const run = runHook({ stubs: ['npm', 'charter'], isolatePath: true }); + const run = runHook({ stubs: ['npm', 'charter'], isolatePath: true, localBin: false }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls(GLOBAL)); + }); + + it.skipIf(!isolatedOk)('Yarn (not PnP) without a local install: a global charter on PATH, run directly', () => { + const run = runHook({ files: { 'yarn.lock': '' }, stubs: ['yarn', 'npm', 'charter'], isolatePath: true, localBin: false }); expect(run.status).toBe(0); expect(run.calls).toEqual(expectedCalls(GLOBAL)); }); + it.skipIf(!isolatedOk)('Yarn (not PnP) with neither a local nor a global install: no Charter call, the hint once', () => { + const run = runHook({ files: { 'yarn.lock': '' }, stubs: ['yarn', 'npm'], isolatePath: true, localBin: false }); + expect(run.status).toBe(1); + expect(run.calls).toEqual([]); + expect(run.stderr).toContain(`[pre-commit] Charter could not be found (tried: ${HOOK_CLI_COMMANDS.yarn}).`); + expect(run.stderr.split('Add @stackbilt/cli as a devDependency').length - 1).toBe(1); + }); + + it.skipIf(!isolatedOk)('Yarn PnP: always yarn charter, even with a global charter and no node_modules', () => { + const run = runHook({ files: { '.pnp.cjs': '' }, stubs: ['yarn', 'npm', 'charter'], isolatePath: true, localBin: false }); + expect(run.status).toBe(0); + expect(withoutYarnVersion(run.calls)).toEqual(expectedCalls(YARN)); + }); + + it.skipIf(!isolatedOk)('pnpm with only a global charter: pnpm exec charter (it searches PATH too)', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': '' }, stubs: ['pnpm', 'npm', 'charter'], isolatePath: true, localBin: false }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls(PNPM)); + }); + + it.skipIf(!isolatedOk)('pnpm with neither a local nor a global install: no Charter call, the hint once', () => { + const run = runHook({ files: { 'pnpm-lock.yaml': '' }, stubs: ['pnpm', 'npm'], isolatePath: true, localBin: false }); + expect(run.status).toBe(1); + expect(run.calls).toEqual([]); + expect(run.stderr).toContain(`[pre-commit] Charter could not be found (tried: ${HOOK_CLI_COMMANDS.pnpm}).`); + }); + it.skipIf(!isolatedOk)('Yarn PnP without yarn: warns (with the corepack fix) and falls back to npm', () => { const run = runHook({ files: { 'yarn.lock': '', '.pnp.cjs': '', ...LOCAL_BIN }, stubs: ['npm'], isolatePath: true }); expect(run.status).toBe(0); diff --git a/packages/cli/src/__tests__/hook.test.ts b/packages/cli/src/__tests__/hook.test.ts index 3e87021..db8ec52 100644 --- a/packages/cli/src/__tests__/hook.test.ts +++ b/packages/cli/src/__tests__/hook.test.ts @@ -391,7 +391,7 @@ fi expect(HOOK_365).not.toContain(PRE_COMMIT_PM_CLI_MARKER); expect(HOOK_365).not.toContain(HOOK_CLI); expect(HOOK_365).toContain(' npx charter doctor --adf-only --ci --staged --tidy-skipped "$TIDY_SKIPPED" --format text || {'); - expect(HOOK_365).toContain('PLAN=$(npx charter adf tidy --dry-run --print-paths $SOURCES 2>/dev/null)'); + expect(HOOK_365).toContain('PLAN=$(npx charter adf tidy --dry-run --print-paths $SOURCES 2>&1)'); }); it.each([ diff --git a/packages/cli/src/commands/hook.ts b/packages/cli/src/commands/hook.ts index d92ef9b..9f66902 100644 --- a/packages/cli/src/commands/hook.ts +++ b/packages/cli/src/commands/hook.ts @@ -185,7 +185,13 @@ if [ "\${CHARTER_SKIP_TIDY:-0}" != "1" ] && [ -f ".ai/manifest.adf" ]; then if [ -n "$SOURCES" ]; then echo "[pre-commit] Checking staged vendor files:$NAMES" PLAN_OK=1 - PLAN=$({CLI} adf tidy --dry-run --print-paths $SOURCES 2>/dev/null) || { echo "[pre-commit] adf tidy --dry-run failed; skipping auto-tidy."; PLAN=""; PLAN_OK=0; } + # stderr is kept: tidy explains there a file it leaves alone (say, an unmanaged one). + PLAN=$({CLI} adf tidy --dry-run --print-paths $SOURCES 2>&1) || { echo "[pre-commit] adf tidy --dry-run failed; skipping auto-tidy."; PLAN=""; PLAN_OK=0; } + NOTES=$(printf '%s\\n' "$PLAN" | grep -v -E '^(vendor|module) ' | grep -v '^ *$') || NOTES="" + if [ -n "$NOTES" ]; then + echo "[pre-commit] adf tidy notes:" + printf '%s\\n' "$NOTES" + fi if printf '%s\\n' "$PLAN" | grep -E '^(vendor|module) ' >/dev/null; then BLOCKED="" IGNORED="" @@ -231,8 +237,8 @@ if [ "\${CHARTER_SKIP_TIDY:-0}" != "1" ] && [ -f ".ai/manifest.adf" ]; then fi echo "[pre-commit] Vendor files tidied; re-staged only the planned files tidy wrote." fi - elif [ "$PLAN_OK" = "1" ]; then - # A failed dry run has said so above; it proves nothing about the files. + elif [ "$PLAN_OK" = "1" ] && [ -z "$NOTES" ]; then + # A failed dry run, or one that left a file alone, has said so above. echo "[pre-commit] Vendor config files are clean." fi fi @@ -324,24 +330,55 @@ elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then elif [ -f "yarn.lock" ] && command -v yarn >/dev/null 2>&1; then PM=yarn fi -# CHARTER_STATE: ok, missing (charter_cli runs nothing) or old (a 1.x CLI); set by the probe below. +# CHARTER_STATE: ok; old (a 1.x CLI); missing, broken or foreign (charter_cli runs +# nothing). CHARTER_FOUND=1 when a charter binary is known to exist (local or global). CHARTER_STATE=ok +CHARTER_LOCAL=0 +if [ -f "node_modules/.bin/charter" ] || [ -f "node_modules/.bin/charter.cmd" ]; then + CHARTER_LOCAL=1 +fi +CHARTER_GLOBAL=0 +if command -v charter >/dev/null 2>&1; then + CHARTER_GLOBAL=1 +fi +CHARTER_FOUND=0 case "$PM" in yarn) CHARTER_RUN="${HOOK_CLI_COMMANDS.yarn}" - if [ -f ".ai/manifest.adf" ]; then + if [ "$CHARTER_PNP" != "1" ] && [ "$CHARTER_LOCAL" = "0" ]; then + # No node_modules/.bin/charter: yarn charter would not find a global one. + if [ "$CHARTER_GLOBAL" = "1" ]; then + CHARTER_RUN="${HOOK_CLI_COMMANDS.global}" + CHARTER_FOUND=1 + else + CHARTER_STATE=missing + fi + elif [ "$CHARTER_PNP" != "1" ]; then + CHARTER_FOUND=1 + fi + if [ "$CHARTER_RUN" = "${HOOK_CLI_COMMANDS.yarn}" ] && [ "$CHARTER_STATE" = "ok" ] && [ -f ".ai/manifest.adf" ]; then case "$(yarn --version 2>/dev/null || true)" in 1.*) CHARTER_RUN="${HOOK_CLI_COMMANDS.yarn1}" ;; esac fi ;; - pnpm) CHARTER_RUN="${HOOK_CLI_COMMANDS.pnpm}" ;; + pnpm) + # pnpm exec puts node_modules/.bin ahead of PATH, so it also finds a global charter. + CHARTER_RUN="${HOOK_CLI_COMMANDS.pnpm}" + if [ "$CHARTER_LOCAL" = "1" ] || [ "$CHARTER_GLOBAL" = "1" ]; then + CHARTER_FOUND=1 + else + CHARTER_STATE=missing + fi + ;; *) CHARTER_RUN="${HOOK_CLI_COMMANDS.npm}" - if [ ! -f "node_modules/.bin/charter" ] && [ ! -f "node_modules/.bin/charter.cmd" ]; then - if command -v charter >/dev/null 2>&1; then + CHARTER_FOUND=1 + if [ "$CHARTER_LOCAL" = "0" ]; then + if [ "$CHARTER_GLOBAL" = "1" ]; then CHARTER_RUN="${HOOK_CLI_COMMANDS.global}" else + CHARTER_FOUND=0 CHARTER_STATE=missing fi fi @@ -353,40 +390,72 @@ if [ "$PM" != "$LOCK_PM" ] && [ -f ".ai/manifest.adf" ]; then echo "[pre-commit] Yarn Plug'n'Play needs yarn to load packages. To fix: corepack enable (or install yarn), then commit again." >&2 fi fi -# Runs exactly the command the hints print ($CHARTER_RUN), or nothing when Charter is missing. +# Runs exactly the command the hints print ($CHARTER_RUN), or nothing when Charter cannot be used. charter_cli() { - if [ "$CHARTER_STATE" = "missing" ]; then - return 127 - fi + case "$CHARTER_STATE" in + missing|broken|foreign) return 127 ;; + esac $CHARTER_RUN "$@" } `; /** * Runs `charter_cli --version` once per hook run, before any other Charter call - * (#366), and only when .ai/manifest.adf exists. A CLI that cannot run sets - * CHARTER_STATE=missing: the hook says so once, skips tidy, and the gate fails - * without calling Charter again. A 1.x CLI (it ignores --staged and - * --print-paths, so tidy would do nothing and the gate would check every file) - * sets CHARTER_STATE=old and gets a one-time upgrade warning. A separate block - * so that tests can replace charter_cli between the two. + * (#366), and only when .ai/manifest.adf exists. Its stdout and stderr are kept. + * - Not resolvable (no binary found, or exit 127): CHARTER_STATE=missing; the + * devDependency hint, once. + * - A binary was found but --version failed: CHARTER_STATE=broken; "installed + * but failed to run" with the first output line, and no devDependency hint. + * - Output without `charter v`: CHARTER_STATE=foreign; the `charter` + * found is not @stackbilt/cli. + * - In each of those, tidy is skipped and the gate fails without calling + * Charter again. + * - charter v0.x / v1.x (it ignores --staged and --print-paths, so tidy would + * do nothing and the gate would check every file): CHARTER_STATE=old and a + * one-time upgrade warning; the hook still runs it. + * A separate block so that tests can replace charter_cli between the two. */ export const HOOK_CLI_PROBE_BLOCK = `# Charter CLI probe (#366): one --version call per hook run. if [ -f ".ai/manifest.adf" ]; then CHARTER_VERSION="" + CHARTER_PROBE_RC=0 if [ "$CHARTER_STATE" != "missing" ]; then - CHARTER_VERSION=$(${HOOK_CLI} --version 2>/dev/null) || CHARTER_STATE=missing + CHARTER_VERSION=$(${HOOK_CLI} --version 2>&1) || CHARTER_PROBE_RC=$? fi - case "$CHARTER_STATE:$CHARTER_VERSION" in - missing:*) - echo "[pre-commit] Charter could not be run with: $CHARTER_RUN" >&2 + CHARTER_FIRST_LINE=$(printf '%s\\n' "$CHARTER_VERSION" | sed -n '/[^[:space:]]/{p;q;}') + if [ "$CHARTER_PROBE_RC" != "0" ]; then + if [ "$CHARTER_FOUND" = "1" ] && [ "$CHARTER_PROBE_RC" != "127" ]; then + CHARTER_STATE=broken + else + CHARTER_STATE=missing + fi + fi + case "$CHARTER_STATE" in + missing) + echo "[pre-commit] Charter could not be found (tried: $CHARTER_RUN)." >&2 echo '[pre-commit] Add @stackbilt/cli as a devDependency and install it. This hook never installs Charter.' >&2 - CHARTER_SKIP_TIDY=1 ;; - *"charter v0."*|*"charter v1."*) - CHARTER_STATE=old - echo "[pre-commit] Warning: $CHARTER_RUN --version reports $CHARTER_VERSION; this hook needs @stackbilt/cli 2.x (1.x ignores --staged and --print-paths, so tidy does nothing and the gate checks every file). Install or upgrade the devDependency: @stackbilt/cli@^2." >&2 + broken) + echo "[pre-commit] Charter is installed but failed to run: $CHARTER_FIRST_LINE" >&2 + echo "[pre-commit] (from: $CHARTER_RUN --version)" >&2 ;; + *) + case "$CHARTER_VERSION" in + *"charter v0."*|*"charter v1."*) + CHARTER_STATE=old + echo "[pre-commit] Warning: $CHARTER_RUN --version reports $CHARTER_FIRST_LINE; this hook needs 2.x (1.x ignores --staged and --print-paths, so tidy does nothing and the gate checks every file). Upgrade @stackbilt/cli to 2.x (as a devDependency, or your global install)." >&2 + ;; + *"charter v"[0-9]*) ;; + *) + CHARTER_STATE=foreign + echo "[pre-commit] Warning: the charter this hook found ($CHARTER_RUN) is not @stackbilt/cli: its --version printed: $CHARTER_FIRST_LINE" >&2 + echo '[pre-commit] Charter was not run. Install @stackbilt/cli as a devDependency.' >&2 + ;; + esac + ;; + esac + case "$CHARTER_STATE" in + missing|broken|foreign) CHARTER_SKIP_TIDY=1 ;; esac fi `; @@ -408,14 +477,22 @@ if [ -f ".ai/manifest.adf" ]; then ${HOOK_CLI} doctor --adf-only --ci --staged --tidy-skipped "$TIDY_SKIPPED" --format text || { echo '' echo '[pre-commit] ADF verification FAILED.' - if [ -n "$TIDY_SKIPPED" ]; then - echo " Run: $CHARTER_RUN doctor --adf-only --ci --staged --tidy-skipped \\"\${TIDY_SKIPPED# }\\" --format text" - else - echo " Run: $CHARTER_RUN doctor --adf-only --ci --staged --format text" - fi - if [ "$CHARTER_STATE" = "missing" ]; then - echo ' Charter was not run: it could not be found (see the note above).' - fi + case "$CHARTER_STATE" in + missing|foreign) + # No command to suggest: nothing resolved to @stackbilt/cli. + echo ' Charter was not run (see the note above).' + ;; + *) + if [ -n "$TIDY_SKIPPED" ]; then + echo " Run: $CHARTER_RUN doctor --adf-only --ci --staged --tidy-skipped \\"\${TIDY_SKIPPED# }\\" --format text" + else + echo " Run: $CHARTER_RUN doctor --adf-only --ci --staged --format text" + fi + if [ "$CHARTER_STATE" = "broken" ]; then + echo ' Charter was not run: it failed to start (see the note above).' + fi + ;; + esac exit 1 } if [ -f "package.json" ] && grep -q '"verify:adf"' package.json; then @@ -447,9 +524,6 @@ if [ -f ".ai/manifest.adf" ]; then echo '' echo '[pre-commit] ADF verification FAILED.' echo " Run: $CHARTER_RUN adf evidence --auto-measure --ci" - if [ "$CHARTER_STATE" = "missing" ]; then - echo ' Charter was not run: it could not be found (see the note above).' - fi exit 1 } fi @@ -689,7 +763,8 @@ function printHelp(): void { console.log(' with yarn on PATH, (2) pnpm-lock.yaml with pnpm, (3) yarn.lock with yarn, (4) npm.'); console.log(` Charter then runs as: ${HOOK_CLI_COMMANDS.yarn} (${HOOK_CLI_COMMANDS.yarn1} on Yarn 1),`); console.log(` ${HOOK_CLI_COMMANDS.pnpm}, or ${HOOK_CLI_COMMANDS.npm}`); - console.log(` (npm, with node_modules/.bin/charter; else a global ${HOOK_CLI_COMMANDS.global} on PATH).`); + console.log(` (npm, with node_modules/.bin/charter). Without a local install, npm and non-PnP Yarn run a`); + console.log(` global ${HOOK_CLI_COMMANDS.global} from PATH, pnpm exec finds it itself, and Yarn PnP never uses one.`); console.log(' The hook never installs Charter. Re-run this command to upgrade an older hook.'); console.log(' Skip tidy with CHARTER_SKIP_TIDY=1. Only gates when .ai/manifest.adf exists.'); console.log('');