diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 676796a..c898882 100644 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -36,7 +36,14 @@ 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 + # 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="" @@ -82,7 +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 - else + 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 505a092..7c237b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,31 @@ 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. 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`. + + 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 is fixed separately (`#380`). **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 GitHub workflow that `charter setup --ci github` and `charter bootstrap --ci github` generate no longer runs a bare `npx charter`** (`#380`). When `@stackbilt/cli` was not installed, `npx charter` fetched the unrelated `charter` registry package (no executable, so the step failed with `could not determine executable to run`). Each step now runs the CLI the install step put in `node_modules`: `pnpm exec charter …` for pnpm, and `npm exec --no --package=@stackbilt/cli -- charter …` for npm. Neither form installs anything; without a local `@stackbilt/cli` the step fails. `setup` and `bootstrap` add it to `devDependencies`, except under `--no-dependency-sync` or in a repo with no root `package.json`. The npm form still contains `charter validate --ci`, so doctor's gate-enforcement check recognizes it. The template has no Yarn form (it supports npm and pnpm only), so a Yarn repo gets the npm form. The previous template (003ec8d, with `npx charter`) has the same permissions and triggers, so `setup`/`bootstrap` refresh it without a warning, as before. Its `KNOWN_WORKFLOWS` entry stays `safe` and records what the refresh changes. - **`charter adf compile --target claude --write --mode compiled` no longer carries the template's empty `## Environment` placeholder into the retained block** (`#364`). A `CLAUDE.md` written by any bootstrap since 0.3.3 (the 1.9.3 upgrade path included) holds `## Environment` with only the `` comment. When you had added a second `## Environment` of your own, converting the pointer to compiled output carried both, comment included, and reported `kept ## Environment, ## Environment`. Each retained section is now compared with the pointer templates on its own (ignoring CRLF, whitespace at line ends and blank lines around the body), so the untouched placeholder is dropped as template text and only your section is carried. A placeholder you wrote under is yours and is carried as before. A retained block that an earlier compile already wrote is copied verbatim as always: if yours holds the placeholder, delete it from the block by hand. - **`charter adf migrate` and `charter bootstrap` wrote the generic "This project uses [ADF]…" pointer instead of the named template when run from the CLI** (`#355`). This hit `GEMINI.md`, `AGENTS.md`, `.cursorrules` and `copilot-instructions.md` in the default mode, and `CLAUDE.md` too under `--replace`. `adf-migrate` built its template table when the module loaded, and the CLI loads it while `adf.ts` is still initialising (`index` → `doctor` → `adf` → `adf-migrate`), so every entry was `undefined`. The template is now looked up when the pointer is written. Unit tests that imported `adf-migrate` directly never saw the cycle. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 1c49761..13c4acd 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -78,8 +78,40 @@ 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. 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 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. - `--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..6e88eef 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 @@ -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 (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__/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 new file mode 100644 index 0000000..58e317c --- /dev/null +++ b/packages/cli/src/__tests__/hook-pm.test.ts @@ -0,0 +1,533 @@ +/** + * #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 / 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'; +import * as fs from 'node:fs'; +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, + PRE_COMMIT_PM_CLI_MARKER, + YARNRC_PNP_LINKER_ERE, + yarnrcPnpLinkerRegExp, +} from '../commands/hook'; + +const shAvailable = (() => { + const probe = spawnSync('sh', ['-c', 'exit 0']); + return !probe.error && probe.status === 0; +})(); + +const STUBS = ['yarn', 'pnpm', 'npx', 'npm', 'charter'] as const; +type Stub = typeof STUBS[number]; + +/** 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 (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: Stub): string { + return `#!/bin/sh +{ + printf '%s' '${name}' + for a in "$@"; do printf ' [%s]' "$a"; done + 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 +is_charter=0 +[ "${name}" = "charter" ] && is_charter=1 +case " $* " in + *" 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 "*) + 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 + case "$*" in + *"$STUB_FAIL_ON"*) echo "${name}: stub failure" >&2; exit 1 ;; + esac +fi +exit 0 +`; +} + +/** 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 && !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 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 && ! 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?: Stub[]; + yarnVersion?: string; + 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). */ + hook?: string; +} + +interface HookRun { + status: number | null; + stdout: string; + stderr: string; + 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; + 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 }); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + fs.rmSync(stubDir, { recursive: true, force: true }); + }); + + function runHook(fx: Fixture): HookRun { + fs.writeFileSync(path.join(dir, 'hook.sh'), fx.hook ?? PRE_COMMIT_HOOK_CONTENT); + 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); + } + 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); + } + 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_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; + 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) + : []; + return { status: result.status, stdout: result.stdout, stderr: result.stderr, calls }; + } + + /** 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 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); + 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([]); + // 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', () => { + 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(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(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(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)); + }); + + 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(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)); + }); + + it.each([ + ['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, localBin: 'node_modules/.bin/charter' in files }); + expect(run.status).toBe(0); + expect(run.calls).toEqual(expectedCalls(NPM)); + }); + + 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)); + }); + + 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(withoutYarnVersion(run.calls)).toEqual(expectedCalls(YARN)); + }); + }); + + it.each([ + ['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(withoutYarnVersion(run.calls)).toEqual([...expectedCalls(runner).slice(0, 3), verify]); + }); + + describe('failure hints', () => { + 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.'); + 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: the exact command, no devDependency hint', () => { + const run = runHook({ files: LOCAL_BIN, failOn: 'evidence' }); + expect(run.status).toBe(1); + 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': '' }, 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 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 (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 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.'); + 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('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, 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.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, 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); + 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)); + }); + + 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 ${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', () => { + 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 ${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': '', ...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).slice(0, 3), 'npm [run] [verify:adf] token-set']); + }); + + 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); + 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], + ['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], + ['# 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/__tests__/hook.test.ts b/packages/cli/src/__tests__/hook.test.ts index 8318a57..db8ec52 100644 --- a/packages/cli/src/__tests__/hook.test.ts +++ b/packages/cli/src/__tests__/hook.test.ts @@ -6,12 +6,16 @@ import * as path from 'node:path'; import { HOOK_SCOPED_DOCTOR_ENV, HOOK_SCOPED_DOCTOR_FILE, + HOOK_CLI, + HOOK_CLI_PROBE_BLOCK, + 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 +234,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 +306,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}${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; + const HOOK_365 = toPre366(PRE_COMMIT_HOOK_CONTENT); async function doctorChecks(): Promise> { const logs: string[] = []; @@ -376,11 +386,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>&1)'); + }); + + 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 +445,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..b87c92f 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'; // ============================================================================ @@ -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,7 +180,11 @@ 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, 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'); 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..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_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'; @@ -51,6 +59,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 +70,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 +84,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 +94,17 @@ export function checkPreCommitScope(): PreCommitScopeCheck | undefined { }; } +/** + * 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 (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". */ 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..9f66902 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, 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 installed by the hook'; /** - * 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 @@ -177,7 +184,14 @@ 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 + # 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="" @@ -223,7 +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 - else + 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 @@ -235,11 +250,222 @@ 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'; + +/** 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, 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(' \\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 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 + * 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} +CHARTER_PNP=0 +if ${PNP_MARKER_FILES.map(f => `[ -f "${f}" ]`).join(' || ')}; then + CHARTER_PNP=1 +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 +elif [ -f "pnpm-lock.yaml" ]; then + LOCK_PM=pnpm +elif [ -f "yarn.lock" ]; then + LOCK_PM=yarn +fi +PM=npm +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_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 [ "$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) + # 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}" + 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 + ;; +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 +# Runs exactly the command the hints print ($CHARTER_RUN), or nothing when Charter cannot be used. +charter_cli() { + 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. 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>&1) || CHARTER_PROBE_RC=$? + fi + 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 + ;; + 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 +`; + export const PRE_COMMIT_HOOK_CONTENT = `#!/usr/bin/env sh ${PRE_COMMIT_HOOK_MARKER} set -eu -${renderHookTidyBlock('npx charter')} +${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} ${PRE_COMMIT_VERIFY_SCOPE_MARKER} @@ -248,25 +474,30 @@ 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" - else - echo ' Run: npx charter doctor --adf-only --ci --staged --format text' - 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 - # 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 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 @@ -289,10 +520,10 @@ 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" exit 1 } fi @@ -527,8 +758,14 @@ 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. 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). 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(''); console.log(' print --claude: Print the Claude Code session hook config snippet to stdout.');