From 63cadb5d741961bec8df3be2e1e218f341a7d3e3 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Wed, 5 Aug 2026 22:47:12 +0100 Subject: [PATCH 1/2] fix(review): accept a husky-reclaimed hooksPath when the overlay is provably gated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In an overlay-mode repo, husky's committed `prepare` resets `core.hooksPath` to `.husky/_` on **every install**, un-wiring overlay's `.devkit/hooks` pointer. Commits stay gated — the opt-in `~/.config/husky/init.sh` shim is sourced by husky's `_/h` before the committed hook and runs the overlay's gates — but `devkit review` hard-failed on the literal: ``` core.hooksPath is ".husky/_", expected .devkit/hooks — run 'devkit doctor --fix'. ``` So a single `pnpm install` broke `devkit review` in a repo whose commits were still fully gated, and the remedy it printed could not repair `core.hooksPath` at all — `doctor` only warned. In overlay mode `review-target.sh` **hardcodes** `-c core.hooksPath=.devkit/hooks` for its private gate run and derives the chain run from `dirname "$chain"`. The captured value reaches git only in the non-overlay branch. So in overlay mode the check was never a steering input — it was an assertion that the repo is gated, and after the reclaim plus the shim, it still is. Same class as sc-1329 (`devkit ship` unconditionally required `.husky/_` and rejected a valid standalone install); the fix there was *resolve, don't hardcode*. **Widen the assertion, don't drop it.** New `cli/lib/ship/review/overlay-hooks-path.mts` accepts `.devkit/hooks` outright and `.husky/_` only when every link of the surviving chain is proven **by content**: - the devkit block in the resolved `init.sh` is complete (both markers + the line that runs the hook) - `.husky/_/h` exists and sources `husky/init.sh` - `.husky/_/pre-commit` is executable and sources `_/h` - a committed `.husky/pre-commit` exists — husky 9.1.7's `_/h:6` runs `[ ! -f "$s" ] && exit 0` **before** sourcing `init.sh`, so without it the shim never fires and commits are genuinely ungated - the overlay chain resolves through `.husky` (an overlay installed before husky records `origHooksPath: ''` and chains to `.git/hooks`, which husky's runner would never execute) Any missing link still fails, naming the first broken one and the resolved `init.sh` path so a `HOME`/`XDG_CONFIG_HOME` mismatch is distinguishable from an uninstalled shim. **The frozen value is canonicalized.** Overlay always freezes `.devkit/hooks` — the value the gate run actually uses — and validates the live value through the predicate. This matters: `setup-runtime.mts` re-compares the frozen value against the live one throughout a review, and the live value is *transient* (both `git ci` and `doctor --fix` re-point it). Freezing the reclaimed `.husky/_` would let a concurrent `git ci` in another terminal abort an in-flight review. Canonicalizing also meant `setup-manifest-parse.mts` and the `review-target.sh` field guard needed no change at all. **`devkit doctor --fix` now re-points `core.hooksPath`**, so the remedy devkit prints is real. It refuses inside a linked worktree — that config lives in the shared `.git/config` and `.devkit/hooks` is relative, so writing it there would re-point sibling worktrees at a path most of them lack. 12 new tests: the acceptance matrix (shim absent, block truncated after its start marker, no committed `.husky/pre-commit`, non-executable stub, `_/h` not sourcing `init.sh`, chain not through `.husky`), a capture→verify round-trip, a **mid-review re-point** regression, and three `doctor --fix` cases including the linked-worktree refusal. - `tsc --noEmit` and `biome check` clean - 112 passed across the six affected suites; 3048 passed across the full unit project - `guard-decisions check overlay-self-heal` exits 0 - impact analysis LOW on `effectiveHooksPath`, `verifySource`, `runOverlayDoctor` New Target on `overlay-self-heal`, citing the `devkit-owned-hook-runner-delivery` "`--fix` is file-content-only" ruling in an `--evidence-change` — a reversible git *config* write differs from the git *index* write that ruling rejected, and this change removes the review-side coupling that record cited as the reason not to re-point. `devkit ship`'s dist-integrity preflight blocked this change on `dist/cli/lib/ship/review/shared/common.mjs` — an artifact that six already-tracked dist files import but which was never force-added when `shared/common.mts` was extracted (sc-1414). It is untracked on `main` today, so the tracked dist is currently unresolvable at that path. It is included here because the preflight is fail-closed and this PR is what surfaced it; it is not part of the feature. --- cli/__tests__/overlay.test.mts | 59 ++++- cli/__tests__/review-setup-manifest.test.mts | 202 +++++++++++++++++- cli/__tests__/review-setup-runtime.test.mts | 85 +++++++- cli/commands/doctor.mts | 109 +--------- cli/lib/doctor/hook-checks.mts | 39 ++++ cli/lib/doctor/overlay-doctor.mts | 129 +++++++++++ cli/lib/overlay-global-hook.mts | 32 ++- cli/lib/ship/review/setup-manifest.mts | 65 ++++-- cli/lib/ship/review/setup-profile.mts | 7 + cli/lib/ship/review/setup-runtime.mts | 34 ++- .../ship/review/setup/overlay-hooks-path.mts | 163 ++++++++++++++ dist/cli/lib/doctor/overlay-doctor.mjs | 97 +++++++++ .../ship/review/setup/overlay-hooks-path.mjs | 146 +++++++++++++ dist/cli/lib/ship/review/shared/common.mjs | 33 +++ docs/decisions/INDEX.md | 2 +- docs/decisions/overlay-self-heal.md | 16 ++ eslint/baselines/size-lines.json | 2 +- 17 files changed, 1087 insertions(+), 133 deletions(-) create mode 100644 cli/lib/doctor/overlay-doctor.mts create mode 100644 cli/lib/ship/review/setup/overlay-hooks-path.mts create mode 100644 dist/cli/lib/doctor/overlay-doctor.mjs create mode 100644 dist/cli/lib/ship/review/setup/overlay-hooks-path.mjs create mode 100644 dist/cli/lib/ship/review/shared/common.mjs diff --git a/cli/__tests__/overlay.test.mts b/cli/__tests__/overlay.test.mts index 824d1f95..aea4ad66 100644 --- a/cli/__tests__/overlay.test.mts +++ b/cli/__tests__/overlay.test.mts @@ -8,7 +8,7 @@ import { execFileSync } from 'node:child_process'; // Reason: test scenario setup is intentionally explicit + self-contained per install mode (package/standalone/overlay/monorepo); shared bits already live in __tests__/_helpers.mjs // fallow-ignore-next-line code-duplication -import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { cpSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import doctorRun from '../commands/doctor.mts'; @@ -989,6 +989,63 @@ describe('overlay hook regeneration (syncOverlayHook + doctor --fix)', () => { expect(await doctorRun(['--fix'], root)).toBe(0); expect(readHook(root)).toContain('devkit-gates: chain start'); }); + + // husky's `prepare` reclaims core.hooksPath on every install. `doctor --fix` used to only WARN + // about that, while `devkit review` told users to run exactly this command to repair it. + const hooksPathOf = (root) => + execFileSync('git', ['config', '--get', 'core.hooksPath'], { + cwd: root, + encoding: 'utf8', + }).trim(); + + it('doctor --fix re-points a reclaimed core.hooksPath; read-only doctor only warns', async () => { + const root = workRepo(); + await initOverlay(root); + execFileSync('git', ['config', 'core.hooksPath', '.husky/_'], { cwd: root }); + + // read-only: reports the drift, changes nothing + expect(await doctorRun([], root)).toBe(1); + expect(hooksPathOf(root)).toBe('.husky/_'); + + expect(await doctorRun(['--fix'], root)).toBe(0); + expect(hooksPathOf(root)).toBe('.devkit/hooks'); + }); + + it('doctor --fix never leaves a re-pointed hooksPath aiming at a missing hook', async () => { + // The two halves must heal together: re-pointing at a .devkit/hooks with no hook in it would + // turn a loud warning into a SILENT zero-gate state. --fix regenerates the hook first (so + // hookOk holds by the time the re-point is considered) and only then moves the pointer. + const root = workRepo(); + await initOverlay(root); + rmSync(join(root, '.devkit', 'hooks'), { recursive: true, force: true }); + execFileSync('git', ['config', 'core.hooksPath', '.husky/_'], { cwd: root }); + + expect(await doctorRun(['--fix'], root)).toBe(0); + expect(hooksPathOf(root)).toBe('.devkit/hooks'); + expect(existsSync(join(root, '.devkit', 'hooks', 'pre-commit'))).toBe(true); + }); + + it('doctor --fix leaves the shared core.hooksPath alone inside a linked worktree', async () => { + // core.hooksPath lives in the SHARED .git/config (only --worktree scope is per-checkout) and + // `.devkit/hooks` is relative, so writing it here would re-point every sibling worktree at a + // directory most of them do not have. + const root = workRepo(); + await initOverlay(root); + execFileSync('git', ['config', 'core.hooksPath', '.husky/_'], { cwd: root }); + const linked = join(mkTmp('overlay-linked-'), 'wt'); + execFileSync('git', ['worktree', 'add', '-q', '--detach', linked], { cwd: root }); + // `.devkit/` is git-ignored (overlay is invisible), so a fresh worktree has none. Copy it in, + // otherwise doctor exits "not initialized" and the worktree guard is never reached. + cpSync(join(root, '.devkit'), join(linked, '.devkit'), { recursive: true }); + expect(existsSync(join(linked, '.devkit', 'hooks', 'pre-commit'))).toBe(true); + + // Exit 1 (not 2 "not initialized") proves the overlay branch ran, and the hook IS present, so + // the only thing that can have refused the write is the linked-worktree guard. + expect(await doctorRun(['--fix'], linked)).toBe(1); + + expect(hooksPathOf(root)).toBe('.husky/_'); + expect(hooksPathOf(linked)).toBe('.husky/_'); + }); }); // `devkit upgrade` in an overlay repo used to BAIL (exit 1, "re-run devkit init --overlay to re-sync"). diff --git a/cli/__tests__/review-setup-manifest.test.mts b/cli/__tests__/review-setup-manifest.test.mts index 8298ddb1..ad1bb838 100644 --- a/cli/__tests__/review-setup-manifest.test.mts +++ b/cli/__tests__/review-setup-manifest.test.mts @@ -1,11 +1,20 @@ import { spawnSync } from 'node:child_process'; import { createHash } from 'node:crypto'; -import { mkdirSync, readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { + chmodSync, + mkdirSync, + readFileSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest'; import { normalizeSelection, structureCmdFor } from '../lib/components.mts'; import { buildOverlayHook, buildStandaloneHook } from '../lib/husky/husky-block.mts'; +import { globalInitPath, installGlobalHook } from '../lib/overlay-global-hook.mts'; import { captureReviewSetup, type ReviewSetupManifest, @@ -58,6 +67,13 @@ function setup(name: string, overlay = false) { return { parent, root, manifest: join(parent, 'setup.json') }; } +// husky 9.1.7's real `_/h`, verbatim in the part that matters: it builds the XDG init.sh path and +// sources it. The acceptance predicate greps for that, so a fixture that paraphrases it proves +// nothing about the arrangement devkit is agreeing to trust. +const HUSKY_RUNNER_H = + // biome-ignore lint/suspicious/noTemplateCurlyInString: shell ${VAR:-default}, not a JS template + '#!/usr/bin/env sh\ni="${XDG_CONFIG_HOME:-$HOME/.config}/husky/init.sh"\n[ -f "$i" ] && . "$i"\n'; + function pathRecord(manifest: ReviewSetupManifest, id: string) { return manifest.setup.paths.find((entry) => entry.id === id); } @@ -415,3 +431,185 @@ describe('review setup manifest', () => { expect(() => verifyReviewSetup(root, manifest)).toThrow(/profile has invalid guards/); }); }); + +// husky's committed `prepare` resets core.hooksPath to `.husky/_` on EVERY install, un-wiring the +// overlay's `.devkit/hooks` pointer. Commits stay gated when the opt-in `~/.config/husky/init.sh` +// shim is installed — husky's own `_/h` sources it before running the committed hook — so review +// must accept that arrangement instead of failing on the literal. It is accepted only on proof: +// every link of that chain has to be intact, or a genuinely ungated repo would pass review. +describe('review setup manifest — husky-reclaimed overlay hooksPath', () => { + const origXdg = process.env.XDG_CONFIG_HOME; + + afterEach(() => { + if (origXdg === undefined) delete process.env.XDG_CONFIG_HOME; + else process.env.XDG_CONFIG_HOME = origXdg; + }); + + /** + * An overlay repo AFTER husky reclaimed core.hooksPath: the overlay hook is still installed and + * chains to the committed `.husky/pre-commit`, but git now runs husky's `.husky/_` runner. + * `shim: false` sandboxes XDG at an empty dir, so the global block is genuinely absent. + */ + function reclaimed(name: string, { shim = true } = {}) { + const parent = mkTmp(`devkit-review-reclaimed-${name}-`); + process.env.XDG_CONFIG_HOME = join(parent, 'xdg'); + if (shim) installGlobalHook(); + const root = join(parent, 'target'); + mkdirSync(root); + git(root, 'init', '-q'); + // husky 9.1.7's real runner shape: `_/h` sources the XDG init.sh, `_/` sources `_/h`. + write(root, '.husky/_/h', HUSKY_RUNNER_H); + write(root, '.husky/_/pre-commit', '#!/usr/bin/env sh\n. "$(dirname "$0")/h"\n', true); + write(root, '.husky/pre-commit', '#!/bin/sh\necho team hook\n', true); + write(root, '.devkit/hooks/pre-commit', buildOverlayHook(selection, '.husky/pre-commit'), true); + write( + root, + '.devkit/config.json', + `${JSON.stringify(config(true, { origHooksPath: '.husky/_' }), null, 2)}\n`, + ); + git(root, 'config', 'core.hooksPath', '.husky/_'); + return { parent, root, manifest: join(parent, 'setup.json') }; + } + + it('accepts the reclaimed path and still freezes the canonical .devkit/hooks', () => { + const { root, manifest } = reclaimed('accepted'); + + const captured = captureReviewSetup(root, manifest); + + // Canonical, NOT the live `.husky/_`: review-target.sh hardcodes `.devkit/hooks` for its gate + // run, and freezing the transient value would let a concurrent `git ci` abort a live review. + expect(captured.setup.hooksPath).toBe('.devkit/hooks'); + expect(captured.setup.overlay).toBe(true); + expect(captured.setup.chain).toEqual({ path: '.husky/pre-commit', sourcePath: '.husky' }); + expect(verifyReviewSetup(root, manifest)).toEqual(captured); + }); + + it('rejects the reclaimed path when the global shim is absent, naming the resolved init.sh', () => { + const { root, manifest } = reclaimed('no-shim', { shim: false }); + + expect(() => captureReviewSetup(root, manifest)).toThrow( + new RegExp(`no devkit block in ${globalInitPath().replaceAll('.', '\\.')}`), + ); + expect(() => captureReviewSetup(root, manifest)).toThrow(/--global-commit-gate/); + }); + + it('rejects a shim block truncated after its start marker', () => { + const { root, manifest } = reclaimed('truncated-shim'); + const initSh = globalInitPath(); + const truncated = readFileSync(initSh, 'utf8').split('\n').slice(0, 2).join('\n'); + expect(truncated).toContain('>>> devkit overlay global pre-commit gate >>>'); + writeFileSync(initSh, `${truncated}\n`); + + expect(() => captureReviewSetup(root, manifest)).toThrow(/no devkit block in/); + }); + + it('rejects the reclaimed path when no committed .husky/pre-commit exists', () => { + // husky's `_/h` runs `[ ! -f "$s" ] && exit 0` BEFORE sourcing init.sh, so with no committed + // hook the shim never fires at all and every commit is silently ungated. + const { root, manifest } = reclaimed('no-committed-hook'); + rmSync(join(root, '.husky/pre-commit')); + writeFileSync(join(root, '.devkit/hooks/pre-commit'), buildOverlayHook(selection, ''), { + mode: 0o755, + }); + + expect(() => captureReviewSetup(root, manifest)).toThrow( + /husky's runner would never reach it|exits before sourcing the shim/, + ); + }); + + it('rejects a runner that only MENTIONS init.sh without sourcing it', () => { + // husky's own `_/h` prints a deprecation notice for `~/.huskyrc` that embeds the literal + // `~/.config/husky/init.sh`. A runner stripped of its `. "$i"` line but still carrying that + // echo satisfies a substring test while never reaching the shim — presence is not proof. + const { root, manifest } = reclaimed('mentions-only'); + write( + root, + '.husky/_/h', + '#!/usr/bin/env sh\nif [ -f "$HOME/.huskyrc" ]; then\n\techo "husky - DEPRECATED, move your code to ~/.config/husky/init.sh"\nfi\n', + ); + + expect(() => captureReviewSetup(root, manifest)).toThrow(/never sources husky\/init\.sh/); + }); + + it('accepts a runner that sources init.sh from an indented if-block', () => { + // Valid shell that genuinely sources: the dot-source sits at a line start WITH indentation, + // not after a `;`/`&&`. Rejecting it would hard-fail a repo that IS gated — the false-reject + // mirror of the echo/comment traps, and just as much a defect. + const { root, manifest } = reclaimed('indented-source'); + write( + root, + '.husky/_/h', + // biome-ignore lint/suspicious/noTemplateCurlyInString: shell ${VAR:-default}, not a JS template + '#!/usr/bin/env sh\ni="${XDG_CONFIG_HOME:-$HOME/.config}/husky/init.sh"\nif [ -f "$i" ]; then\n . "$i"\nfi\n', + ); + + expect(captureReviewSetup(root, manifest).setup.hooksPath).toBe('.devkit/hooks'); + }); + + it('rejects a runner whose source line is commented out', () => { + // A comment still supplies the `;` a source-command pattern keys on, so a disabled line reads + // as an active one unless comments are stripped first. Same trap as the echo, different shape. + const { root, manifest } = reclaimed('commented-source'); + write( + root, + '.husky/_/h', + // biome-ignore lint/suspicious/noTemplateCurlyInString: shell ${VAR:-default}, not a JS template + '#!/usr/bin/env sh\ni="${XDG_CONFIG_HOME:-$HOME/.config}/husky/init.sh"\n# disabled; . "$i"\n', + ); + + expect(() => captureReviewSetup(root, manifest)).toThrow(/never sources husky\/init\.sh/); + }); + + it('rejects a stub whose source of the runner is commented out', () => { + const { root, manifest } = reclaimed('commented-stub'); + write(root, '.husky/_/pre-commit', '#!/usr/bin/env sh\n# . "$(dirname "$0")/h"\n', true); + + expect(() => captureReviewSetup(root, manifest)).toThrow( + /pre-commit never sources \.husky\/_\/h/, + ); + }); + + it('rejects a runner that cannot reach the shim', () => { + const { root, manifest } = reclaimed('broken-runner'); + write(root, '.husky/_/h', '#!/usr/bin/env sh\nexit 0\n'); + + expect(() => captureReviewSetup(root, manifest)).toThrow(/never sources husky\/init\.sh/); + + write(root, '.husky/_/h', HUSKY_RUNNER_H); + chmodSync(join(root, '.husky/_/pre-commit'), 0o644); + expect(() => captureReviewSetup(root, manifest)).toThrow( + /\.husky\/_\/pre-commit is not executable/, + ); + }); + + it('rejects a reclaimed overlay whose chain does not resolve through .husky', () => { + // An overlay installed BEFORE husky records origHooksPath '' and chains to .git/hooks — husky's + // runner would never execute that, so review must not vouch for a chain it never ran. + const { root, manifest } = reclaimed('foreign-chain'); + writeFileSync(join(root, '.git/hooks/pre-commit'), '#!/bin/sh\necho chained\n', { + mode: 0o755, + }); + write( + root, + '.devkit/hooks/pre-commit', + buildOverlayHook(selection, '.git/hooks/pre-commit'), + true, + ); + write( + root, + '.devkit/config.json', + `${JSON.stringify(config(true, { origHooksPath: '' }), null, 2)}\n`, + ); + + expect(() => captureReviewSetup(root, manifest)).toThrow(/chains to \.git\/hooks, not \.husky/); + }); + + it('still rejects an overlay hooksPath that is neither the overlay nor husky runner', () => { + const { root, manifest } = reclaimed('foreign-path'); + git(root, 'config', 'core.hooksPath', '.githooks'); + + expect(() => captureReviewSetup(root, manifest)).toThrow( + /core\.hooksPath is "\.githooks", expected \.devkit\/hooks/, + ); + }); +}); diff --git a/cli/__tests__/review-setup-runtime.test.mts b/cli/__tests__/review-setup-runtime.test.mts index 2293c170..f8077255 100644 --- a/cli/__tests__/review-setup-runtime.test.mts +++ b/cli/__tests__/review-setup-runtime.test.mts @@ -14,8 +14,9 @@ import { } from 'node:fs'; import { dirname, join, relative } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest'; import { buildOverlayHook, buildStandaloneHook } from '../lib/husky/husky-block.mts'; +import { globalInitPath, installGlobalHook } from '../lib/overlay-global-hook.mts'; import { captureReviewSetup } from '../lib/ship/review/setup-manifest.mts'; import { encodeReviewSetupRuntimeFields, @@ -29,6 +30,12 @@ const HERE = dirname(fileURLToPath(import.meta.url)); const CLI = join(HERE, '../lib/ship/review/setup-runtime.mts'); const { git, mkTmp, selection, write } = reviewSetupFixtures(); +// husky 9.1.7's real `_/h`, verbatim in the part that matters: it builds the XDG init.sh path and +// sources it — which is what the acceptance predicate greps for. +const HUSKY_RUNNER_H = + // biome-ignore lint/suspicious/noTemplateCurlyInString: shell ${VAR:-default}, not a JS template + '#!/usr/bin/env sh\ni="${XDG_CONFIG_HOME:-$HOME/.config}/husky/init.sh"\n[ -f "$i" ] && . "$i"\n'; + function config(overlay: boolean, pkgRel = '') { return { stack: 'generic', @@ -105,6 +112,82 @@ function seedSnapshot(fx: ReturnType, overlay = false): void { } } +// An overlay repo whose core.hooksPath husky has reclaimed to `.husky/_`, with the global init.sh +// shim keeping it gated. The frozen value is the canonical `.devkit/hooks`, so the live value is +// re-validated by the acceptance predicate rather than compared literally. +describe('private review setup runtime — husky-reclaimed overlay hooksPath', () => { + const origXdg = process.env.XDG_CONFIG_HOME; + + afterEach(() => { + if (origXdg === undefined) delete process.env.XDG_CONFIG_HOME; + else process.env.XDG_CONFIG_HOME = origXdg; + }); + + function reclaimedFixture(name: string) { + const parent = mkTmp(`devkit-review-runtime-reclaimed-${name}-`); + process.env.XDG_CONFIG_HOME = join(parent, 'xdg'); + installGlobalHook(); + const gitRoot = join(parent, 'source'); + mkdirSync(gitRoot, { recursive: true }); + git(gitRoot, 'init', '-q'); + write(gitRoot, '.husky/_/h', HUSKY_RUNNER_H); + write(gitRoot, '.husky/_/pre-commit', '#!/usr/bin/env sh\n. "$(dirname "$0")/h"\n', true); + write(gitRoot, '.husky/pre-commit', '#!/bin/sh\necho team hook\n', true); + write( + gitRoot, + '.devkit/hooks/pre-commit', + buildOverlayHook(selection, '.husky/pre-commit'), + true, + ); + write( + gitRoot, + '.devkit/config.json', + `${JSON.stringify({ ...config(true), origHooksPath: '.husky/_' }, null, 2)}\n`, + ); + git(gitRoot, 'config', 'core.hooksPath', '.husky/_'); + const setupManifest = join(parent, 'setup.json'); + captureReviewSetup(gitRoot, setupManifest); + const destination = join(parent, 'private'); + mkdirSync(destination); + return { + parent, + gitRoot, + setupManifest, + destination, + runtimeManifest: join(parent, 'runtime.json'), + }; + } + + it('survives a mid-review re-point back to .devkit/hooks', () => { + // `git ci` (overlay's self-heal alias) and `devkit doctor --fix` both re-point core.hooksPath. + // Either can fire from another terminal while a review runs; because the overlay gate run never + // consults the live value, that must not abort the run. + const fx = reclaimedFixture('flip'); + materializeReviewSetupRuntime(fx.setupManifest, fx.destination, fx.runtimeManifest); + + // Before the flip: live `.husky/_` against a frozen `.devkit/hooks` — literal equality (what + // this check used to be) would abort here. + expect(git(fx.gitRoot, 'config', '--get', 'core.hooksPath')).toBe('.husky/_'); + expect(() => verifyReviewSetupSource(fx.setupManifest, fx.gitRoot)).not.toThrow(); + + git(fx.gitRoot, 'config', 'core.hooksPath', '.devkit/hooks'); + + expect(() => verifyReviewSetupSource(fx.setupManifest, fx.gitRoot)).not.toThrow(); + expect(() => verifyReviewSetupRuntime(fx.setupManifest, fx.runtimeManifest)).not.toThrow(); + }); + + it('still aborts when the reclaimed setup stops being provably gated mid-review', () => { + const fx = reclaimedFixture('shim-removed'); + materializeReviewSetupRuntime(fx.setupManifest, fx.destination, fx.runtimeManifest); + + rmSync(globalInitPath()); + + expect(() => verifyReviewSetupRuntime(fx.setupManifest, fx.runtimeManifest)).toThrow( + /core\.hooksPath changed/, + ); + }); +}); + describe('private review setup runtime', () => { it('maps nested target setup, merges identical snapshot files, and dereferences the runner', () => { const fx = fixture('nested', { nested: true, linkedRunner: true }); diff --git a/cli/commands/doctor.mts b/cli/commands/doctor.mts index 50e594c5..467f7102 100644 --- a/cli/commands/doctor.mts +++ b/cli/commands/doctor.mts @@ -15,12 +15,9 @@ import { expectedExtends, repairExtends, } from '../lib/doctor/extends-checks.mts'; -import { - adviseSearchIndex, - checkGuardConfig, - SEARCH_INDEX_CHECK, -} from '../lib/doctor/guard-config-checks.mts'; +import { checkGuardConfig, SEARCH_INDEX_CHECK } from '../lib/doctor/guard-config-checks.mts'; import { hookChecks } from '../lib/doctor/hook-checks.mts'; +import { runOverlayDoctor } from '../lib/doctor/overlay-doctor.mts'; import { checkLockPin, checkPin } from '../lib/doctor/pin-checks.mts'; import { runSelfHostDoctor } from '../lib/doctor/self-host-doctor.mts'; import { packageDir, readJson } from '../lib/fs-helpers.mts'; @@ -32,8 +29,6 @@ import { SUPPORTED_AGENT_PROVIDERS, } from '../lib/install/agent-assets/agent-providers.mts'; import { selectedHookAssets } from '../lib/install/hook-registration-ledger/selection.mts'; -import { HEAL_ALIAS_NAME, isHealAlias, syncOverlayHook } from '../lib/overlay.mts'; -import { globalHookInstalled, globalInitPath } from '../lib/overlay-global-hook.mts'; import { cmpSemver, fetchLatestTag } from './update.mts'; // Devkit modules are .mts in source and .mjs when installed; runtime string paths need the live ext. @@ -295,104 +290,6 @@ const DEFAULT_DOCTOR_SEL: Partial = { guards: [...RECOMMENDED_GUARD_IDS], }; -// Overlay health is gated by its local hook + hooksPath; agent assets and fallow are advisory. -// Reason: flat signal reporting keeps the exit code gated only on hook + path. -// fallow-ignore-next-line complexity -async function runOverlayDoctor(cwd: string, cfg: DevkitConfig, fix: boolean): Promise { - // hooksPath and its alias are repo-wide, including for a monorepo package. - const { gitRoot } = detectGitRoot(cwd); - const gitGet = (key: string): string => { - try { - return execFileSync('git', ['config', '--get', key], { - cwd: gitRoot, - encoding: 'utf8', - }).trim(); - } catch { - return ''; // unset - } - }; - const hooksPath = gitGet('core.hooksPath'); - const aliasOurs = isHealAlias(gitGet(`alias.${HEAL_ALIAS_NAME}`)); - // Compare the ignored overlay hook with a fresh build; --fix rewrites stale/missing copies. - const sync = syncOverlayHook(gitRoot, cwd, cfg, { dryRun: !fix }); - const hookOk = existsSync(join(gitRoot, '.devkit', 'hooks', 'pre-commit')); // post-fix presence - const pathOk = hooksPath === '.devkit/hooks'; - console.log('devkit doctor — overlay (local-only)\n'); - if (!hookOk) - console.log( - ' ✗ .devkit/hooks/pre-commit MISSING — run `devkit doctor --fix` (or `devkit init --overlay`)', - ); - else if (fix && (sync.missing || sync.drift)) - console.log( - ' ✓ .devkit/hooks/pre-commit regenerated (was stale/missing — refreshed to the current devkit)', - ); - else if (sync.drift) - console.log( - ' ⚠ .devkit/hooks/pre-commit is STALE (predates the current devkit) — run `devkit doctor --fix` to refresh', - ); - else console.log(' ✓ .devkit/hooks/pre-commit present'); - console.log( - ` ${pathOk ? '✓' : '⚠'} core.hooksPath = ${hooksPath || '(unset)'}${pathOk ? '' : ` — heal with \`git ${HEAL_ALIAS_NAME}\` (re-points it) or re-run \`devkit init --overlay\``}`, - ); - // Advisory only — never affects the exit code (hook + path are the real health signal). - if (aliasOurs && !hookOk) - console.log( - ` ⚠ git ${HEAL_ALIAS_NAME} points at a missing .devkit/hooks — run \`devkit clean\``, - ); - else if (aliasOurs) console.log(` ✓ git ${HEAL_ALIAS_NAME} self-heal alias`); - else - console.log( - ` · self-heal off (git ${HEAL_ALIAS_NAME} re-points core.hooksPath; or re-run \`devkit init --overlay\`)`, - ); - // The opt-in global shim gates plain commits after Husky reclaims hooksPath; advisory here. - if (globalHookInstalled()) { - console.log(` ✓ global pre-commit gate (${globalInitPath()}) — plain \`git commit\` gated`); - if (aliasOurs) - console.log( - ` (git ${HEAL_ALIAS_NAME} is the CLI fast-path; shim + alias don't double-run)`, - ); - // Husky cannot source the shim without a committed .husky/pre-commit. - const huskyPresent = - existsSync(join(gitRoot, '.husky', '_')) || existsSync(join(gitRoot, '.husky')); - if (huskyPresent && !existsSync(join(gitRoot, '.husky', 'pre-commit'))) - console.log( - ` ⚠ no committed .husky/pre-commit — husky won't source the shim for pre-commit; a plain \`git commit\` stays ungated here (use \`git ${HEAL_ALIAS_NAME}\`)`, - ); - } else if (!pathOk) { - console.log( - ` · plain \`git commit\` is ungated (husky reclaimed core.hooksPath); \`git ${HEAL_ALIAS_NAME}\` heals it, or wire it permanently with \`devkit init --overlay --global-commit-gate\``, - ); - } - // Agent-half + fallow checks — ADVISORY (printed, never gate the exit code; a re-run re-syncs them). - const recorded: Partial = cfg?.components ?? {}; - const surfaces = resolveExistingAgentProviders(gitRoot, recorded.agentTargets); - const sel: Partial = { ...recorded, agentTargets: surfaces }; - const advise = (r: CheckResult) => - console.log(` ${r.status === 'OK' ? '✓' : '·'} ${r.name}: ${r.detail}`); - const hooks = selectedHookAssets(sel, { searchSteering: false }); - if (sel.skills && surfaces.length) advise(checkAgentAssets(cwd, 'skills', surfaces, sel)); - if (sel.agents && surfaces.length) advise(checkAgentAssets(cwd, 'agents', surfaces)); - if (hooks.scripts.length && surfaces.length) - advise(checkAgentAssets(cwd, 'hooks', surfaces, { expected: hooks.scripts })); - if (surfaces.length) advise(checkRegistrations(cwd, hooks.components, surfaces, true)); - // Overlay short-circuits before collectResults, so the dup gate's silent opt-out would otherwise - // be undetectable here. Advisory: overlay health is gated on hook + hooksPath. - await adviseSearchIndex(cwd, sel); - printQavisAdvisoryHealth(cwd, sel.guards ?? []); - if (sel.fallow) { - const wired = - hookOk && - readFileSync(join(gitRoot, '.devkit', 'hooks', 'pre-commit'), 'utf8').includes( - 'fallow audit', - ); - console.log( - ` ${wired ? '✓' : '·'} fallow gate: ${wired ? 'wired in the local hook' : 'not wired'}`, - ); - } - // A stale hook is unhealthy (exit 1) so CI/agents notice; --fix having just regenerated it heals this run. - return hookOk && pathOk && (fix || !sync.drift) ? 0 : 1; -} - // Self-host (the devkit repo dogfooding itself) doctor: the ONE health signal is whether the // committed source hook still matches what the CURRENT generator produces — a mismatch means the // generator changed without a regen, or the hook was hand-edited. `--fix` regenerates it. Skills/ @@ -494,7 +391,7 @@ export default async function run(args: string[], cwd: string): Promise } const cfg = (readJson(join(cwd, '.devkit', 'config.json')) ?? {}) as DevkitConfig; - if (cfg.overlay) return runOverlayDoctor(cwd, cfg, fix); + if (cfg.overlay) return runOverlayDoctor(cwd, cfg, fix, printQavisAdvisoryHealth); if (cfg.selfHost) return runSelfHostDoctor(cwd, cfg, fix); const { results, sel } = await collectResults(cwd, cfg, configResult); diff --git a/cli/lib/doctor/hook-checks.mts b/cli/lib/doctor/hook-checks.mts index 67c2a6c9..98576caf 100644 --- a/cli/lib/doctor/hook-checks.mts +++ b/cli/lib/doctor/hook-checks.mts @@ -14,10 +14,49 @@ import { REVIEWABLE_GUARD_IDS } from '../components.mts'; import { detectGitRoot } from '../detect-git-root.mts'; import { markEnd, markStart } from '../husky/husky.mts'; import { extractGuardBlock, QAVIS_ADVISORY_ID } from '../husky/husky-block.mts'; +import { firstLine } from '../standalone.mts'; import { type CheckResult, check } from './check-result.mts'; import { strayGateCalls } from './stray-gate-calls.mts'; import { checkFailOpenGuards } from './unguarded-gate-calls.mts'; +/** + * `doctor --fix`'s repair for a husky-reclaimed `core.hooksPath` in OVERLAY mode. husky's `prepare` + * resets it to `.husky/_` on every install, and until now `--fix` only WARNED — while `devkit review` + * told users to run exactly this command to repair it. Transient by nature (the next install + * reclaims it again); `devkit init --overlay --global-commit-gate` is the durable fix. + * + * Lives here rather than in `doctor.mts` for the same reason the checks above do — that file is at + * its recorded size budget — and beside them because this module already owns doctor's hooksPath + * reasoning. + * + * Two refusals, both fail-safe: + * - no `.devkit/hooks/pre-commit` → re-pointing would aim core.hooksPath at a directory with no + * hook in it, turning a loud warning into a silent zero-gate state. Belt-and-braces in practice: + * the caller runs syncOverlayHook first, which regenerates a missing hook under `--fix`, so this + * only fires if that ever stops guaranteeing the file. The pointer must never lead the hook. + * - a LINKED worktree → core.hooksPath lives in the SHARED .git/config (only `--worktree` scope is + * per-checkout) and `.devkit/hooks` is relative, so writing it from here would re-point every + * sibling worktree at a path most of them do not have. Print the main-checkout command instead. + */ +export function repointHooksPath(gitRoot: string, hookOk: boolean): boolean { + if (!hookOk) return false; + try { + const git = (...args: string[]) => + execFileSync('git', args, { cwd: gitRoot, encoding: 'utf8' }).trim(); + if (git('rev-parse', '--git-dir') !== git('rev-parse', '--git-common-dir')) { + console.log( + ' · linked worktree — core.hooksPath is shared with every other worktree, so --fix leaves it alone; re-point from the main checkout: git config --local core.hooksPath .devkit/hooks', + ); + return false; + } + git('config', '--local', 'core.hooksPath', '.devkit/hooks'); + return true; // the caller reports the healed path on its own core.hooksPath line + } catch (e) { + console.log(` ! could not re-point core.hooksPath: ${firstLine(e)}`); + return false; + } +} + // Selection-aware: only the SELECTED guards must be present in the block (a deselected // guard being absent is correct, not drift). Monorepo: the hook lives at the git root and the // block is package-scoped — resolve both from cwd. diff --git a/cli/lib/doctor/overlay-doctor.mts b/cli/lib/doctor/overlay-doctor.mts new file mode 100644 index 00000000..4a92eb0a --- /dev/null +++ b/cli/lib/doctor/overlay-doctor.mts @@ -0,0 +1,129 @@ +/** + * Overlay-mode doctor. Overlay health is gated by its local hook + `core.hooksPath`; agent assets + * and fallow are advisory (printed, never in the exit code) because a re-run re-syncs them. + * + * Lives here beside `self-host-doctor.mts` — the same shape, a mode-specific doctor in its own + * module — rather than in `doctor.mts`, which is at its line budget. + */ + +import { execFileSync } from 'node:child_process'; +import { existsSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import type { Selection } from '../components.mts'; +import { detectGitRoot } from '../detect-git-root.mts'; +import { resolveExistingAgentProviders } from '../install/agent-assets/agent-providers.mts'; +import { selectedHookAssets } from '../install/hook-registration-ledger/selection.mts'; +import { HEAL_ALIAS_NAME, isHealAlias, syncOverlayHook } from '../overlay.mts'; +import { globalHookInstalled, globalInitPath } from '../overlay-global-hook.mts'; +import { checkAgentAssets, checkRegistrations } from './asset-checks.mts'; +import type { CheckResult } from './check-result.mts'; +import { adviseSearchIndex } from './guard-config-checks.mts'; +import { repointHooksPath } from './hook-checks.mts'; + +/** The recorded `.devkit/config.json` fields the overlay doctor consults. */ +export interface OverlayDoctorConfig { + components?: Partial; +} + +// Reason: flat signal reporting keeps the exit code gated only on hook + path. +// fallow-ignore-next-line complexity +export async function runOverlayDoctor( + cwd: string, + cfg: OverlayDoctorConfig, + fix: boolean, + printQavisAdvisoryHealth: (cwd: string, guards: string[]) => void, +): Promise { + // hooksPath and its alias are repo-wide, including for a monorepo package. + const { gitRoot } = detectGitRoot(cwd); + const gitGet = (key: string): string => { + try { + return execFileSync('git', ['config', '--get', key], { + cwd: gitRoot, + encoding: 'utf8', + }).trim(); + } catch { + return ''; // unset + } + }; + const hooksPath = gitGet('core.hooksPath'); + const aliasOurs = isHealAlias(gitGet(`alias.${HEAL_ALIAS_NAME}`)); + // Compare the ignored overlay hook with a fresh build; --fix rewrites stale/missing copies. + const sync = syncOverlayHook(gitRoot, cwd, cfg, { dryRun: !fix }); + const hookOk = existsSync(join(gitRoot, '.devkit', 'hooks', 'pre-commit')); // post-fix presence + const healed = fix && hooksPath !== '.devkit/hooks' && repointHooksPath(gitRoot, hookOk); + const pathOk = healed || hooksPath === '.devkit/hooks'; + console.log('devkit doctor — overlay (local-only)\n'); + if (!hookOk) + console.log( + ' ✗ .devkit/hooks/pre-commit MISSING — run `devkit doctor --fix` (or `devkit init --overlay`)', + ); + else if (fix && (sync.missing || sync.drift)) + console.log( + ' ✓ .devkit/hooks/pre-commit regenerated (was stale/missing — refreshed to the current devkit)', + ); + else if (sync.drift) + console.log( + ' ⚠ .devkit/hooks/pre-commit is STALE (predates the current devkit) — run `devkit doctor --fix` to refresh', + ); + else console.log(' ✓ .devkit/hooks/pre-commit present'); + console.log( + ` ${pathOk ? '✓' : '⚠'} core.hooksPath = ${healed ? `.devkit/hooks (re-pointed from ${hooksPath || '(unset)'}; husky reclaims it on every install — make it durable with \`devkit init --overlay --global-commit-gate\`)` : hooksPath || '(unset)'}${pathOk ? '' : ` — heal with \`git ${HEAL_ALIAS_NAME}\` (re-points it), \`devkit doctor --fix\`, or re-run \`devkit init --overlay\``}`, + ); + // Advisory only — never affects the exit code (hook + path are the real health signal). + if (aliasOurs && !hookOk) + console.log( + ` ⚠ git ${HEAL_ALIAS_NAME} points at a missing .devkit/hooks — run \`devkit clean\``, + ); + else if (aliasOurs) console.log(` ✓ git ${HEAL_ALIAS_NAME} self-heal alias`); + else + console.log( + ` · self-heal off (git ${HEAL_ALIAS_NAME} re-points core.hooksPath; or re-run \`devkit init --overlay\`)`, + ); + // The opt-in global shim gates plain commits after Husky reclaims hooksPath; advisory here. + if (globalHookInstalled()) { + console.log(` ✓ global pre-commit gate (${globalInitPath()}) — plain \`git commit\` gated`); + if (aliasOurs) + console.log( + ` (git ${HEAL_ALIAS_NAME} is the CLI fast-path; shim + alias don't double-run)`, + ); + // Husky cannot source the shim without a committed .husky/pre-commit. + const huskyPresent = + existsSync(join(gitRoot, '.husky', '_')) || existsSync(join(gitRoot, '.husky')); + if (huskyPresent && !existsSync(join(gitRoot, '.husky', 'pre-commit'))) + console.log( + ` ⚠ no committed .husky/pre-commit — husky won't source the shim for pre-commit; a plain \`git commit\` stays ungated here (use \`git ${HEAL_ALIAS_NAME}\`)`, + ); + } else if (!pathOk) { + console.log( + ` · plain \`git commit\` is ungated (husky reclaimed core.hooksPath); \`git ${HEAL_ALIAS_NAME}\` heals it, or wire it permanently with \`devkit init --overlay --global-commit-gate\``, + ); + } + // Agent-half + fallow checks — ADVISORY (printed, never gate the exit code; a re-run re-syncs them). + const recorded: Partial = cfg?.components ?? {}; + const surfaces = resolveExistingAgentProviders(gitRoot, recorded.agentTargets); + const sel: Partial = { ...recorded, agentTargets: surfaces }; + const advise = (r: CheckResult) => + console.log(` ${r.status === 'OK' ? '✓' : '·'} ${r.name}: ${r.detail}`); + const hooks = selectedHookAssets(sel, { searchSteering: false }); + if (sel.skills && surfaces.length) advise(checkAgentAssets(cwd, 'skills', surfaces, sel)); + if (sel.agents && surfaces.length) advise(checkAgentAssets(cwd, 'agents', surfaces)); + if (hooks.scripts.length && surfaces.length) + advise(checkAgentAssets(cwd, 'hooks', surfaces, { expected: hooks.scripts })); + if (surfaces.length) advise(checkRegistrations(cwd, hooks.components, surfaces, true)); + // Overlay short-circuits before collectResults, so the dup gate's silent opt-out would otherwise + // be undetectable here. Advisory: overlay health is gated on hook + hooksPath. + await adviseSearchIndex(cwd, sel); + printQavisAdvisoryHealth(cwd, sel.guards ?? []); + if (sel.fallow) { + const wired = + hookOk && + readFileSync(join(gitRoot, '.devkit', 'hooks', 'pre-commit'), 'utf8').includes( + 'fallow audit', + ); + console.log( + ` ${wired ? '✓' : '·'} fallow gate: ${wired ? 'wired in the local hook' : 'not wired'}`, + ); + } + // A stale hook is unhealthy (exit 1) so CI/agents notice; --fix having just regenerated it heals this run. + return hookOk && pathOk && (fix || !sync.drift) ? 0 : 1; +} diff --git a/cli/lib/overlay-global-hook.mts b/cli/lib/overlay-global-hook.mts index f5ad74e0..753e1e7a 100644 --- a/cli/lib/overlay-global-hook.mts +++ b/cli/lib/overlay-global-hook.mts @@ -23,6 +23,9 @@ import { dirname, join } from 'node:path'; const MARK_START = '# >>> devkit overlay global pre-commit gate >>>'; const MARK_END = '# <<< devkit overlay global pre-commit gate <<<'; const TRAILING_NEWLINES = /\n+$/; // hoisted (perf: never recompile per install/remove) +// The overlay hook the block invokes. Interpolated into BLOCK (never re-typed) so the string +// `globalHookWired` greps for cannot drift from the string the block actually runs. +const OVERLAY_HOOK_REL = '.devkit/hooks/pre-commit'; // The devkit block. Guarded so it ONLY acts in an overlaid repo and is otherwise inert: // - HUSKY=0 (husky's documented skip-hooks escape hatch) → skip. _/h's own HUSKY=0 exit is at @@ -39,8 +42,8 @@ const BLOCK = `${MARK_START} # HUSKY=0. Repo root resolved via git so worktrees / submodules / git -C still gate the right tree. if [ "\${HUSKY:-}" != "0" ] && [ "\${0##*/}" = "pre-commit" ]; then __dk_root=$(git rev-parse --show-toplevel 2>/dev/null) || __dk_root= - if [ -n "$__dk_root" ] && [ -x "$__dk_root/.devkit/hooks/pre-commit" ]; then - DEVKIT_VIA_HUSKY_INIT=1 sh "$__dk_root/.devkit/hooks/pre-commit" "$@" || exit $? + if [ -n "$__dk_root" ] && [ -x "$__dk_root/${OVERLAY_HOOK_REL}" ]; then + DEVKIT_VIA_HUSKY_INIT=1 sh "$__dk_root/${OVERLAY_HOOK_REL}" "$@" || exit $? fi unset __dk_root fi @@ -62,6 +65,31 @@ export function globalHookInstalled() { } } +/** + * True iff the global init.sh carries a COMPLETE, still-wired devkit block: both markers AND the + * line that actually runs the overlay hook between them. + * + * `globalHookInstalled` (MARK_START alone) stays doctor's advisory signal — a start marker is + * enough to say "you opted in". `devkit review` needs the stronger claim, because this shim is the + * ONLY thing keeping an overlay repo gated once husky reclaims core.hooksPath: a block truncated + * after its start marker (a half-applied hand edit, an interrupted write) passes the advisory test + * while gating nothing. + */ +export function globalHookWired() { + const file = globalInitPath(); + try { + if (!existsSync(file)) return false; + const content = readFileSync(file, 'utf8'); + const start = content.indexOf(MARK_START); + if (start === -1) return false; + const end = content.indexOf(MARK_END, start); + if (end === -1) return false; + return content.slice(start, end).includes(OVERLAY_HOOK_REL); + } catch { + return false; + } +} + // Slice the devkit block (markers inclusive) out of `content`, collapsing the blank-line join that // preceded it (and one trailing newline). Returns the remainder (possibly ''). Never touches text // outside the markers, so a hand-written init.sh survives. diff --git a/cli/lib/ship/review/setup-manifest.mts b/cli/lib/ship/review/setup-manifest.mts index a809d25b..fd4b446f 100644 --- a/cli/lib/ship/review/setup-manifest.mts +++ b/cli/lib/ship/review/setup-manifest.mts @@ -16,6 +16,11 @@ import { isSafeReviewRelativePath, reviewPathWithin, } from './runtime-paths.mts'; +import { + OVERLAY_HOOKS_PATH, + type OverlayHooksPathContext, + overlayHooksPathRejection, +} from './setup/overlay-hooks-path.mts'; import { REVIEW_SETUP_ABSENT, REVIEW_SETUP_VERSION, @@ -24,6 +29,7 @@ import { import { parseReviewSetupManifest } from './setup-manifest-parse.mts'; import { REVIEW_SETUP_DOCTOR as DOCTOR, + REVIEW_SETUP_OVERLAY_DOCTOR as OVERLAY_DOCTOR, parseReviewSetupProfile, type RawReviewConfig, } from './setup-profile.mts'; @@ -228,14 +234,35 @@ function readHooksPath(root: string): string { return decodeHooksPath(result.status, result.stdout); } -function effectiveHooksPath(root: string, overlay: boolean): string { +/** + * The hooksPath to FREEZE, after validating the live one. + * + * Package mode freezes what it reads — review-target.sh actually runs the gates with it. Overlay + * mode always freezes the canonical `.devkit/hooks`, which is what review-target.sh hardcodes for + * its private gate run, and validates the live value through the acceptance predicate instead. + * + * Canonicalizing is what keeps the frozen value STABLE. The live value is transient in overlay mode + * — the `git ci` alias (overlay.mts) and `devkit doctor --fix` both re-point it — and the frozen + * value is re-compared against the live one throughout a review (setup-runtime.mts verifySource, + * re-run at several points in review-target.sh). Freezing the reclaimed `.husky/_` would let a + * concurrent `git ci` in another terminal abort an in-flight review. + */ +function effectiveHooksPath( + root: string, + overlay: boolean, + context: OverlayHooksPathContext, +): string { const value = readHooksPath(root); - const expected = overlay ? '.devkit/hooks' : '.husky/_'; - if (value !== expected) - fail( - `core.hooksPath is ${JSON.stringify(value || '(unset)')}, expected ${expected} — ${DOCTOR}`, - ); - return value; + if (!overlay) { + if (value !== '.husky/_') + fail( + `core.hooksPath is ${JSON.stringify(value || '(unset)')}, expected .husky/_ — ${DOCTOR}`, + ); + return value; + } + const rejection = overlayHooksPathRejection(value, context); + if (rejection) fail(`${rejection} — ${OVERLAY_DOCTOR}`); + return OVERLAY_HOOKS_PATH; } function captureSetupPaths( @@ -260,7 +287,12 @@ function captureOverlayChain( gitRoot: string, targetRoot: string, config: RawReviewConfig, -): { chain: NonNullable; paths: ReviewSetupPath[] } { +): { + chain: NonNullable; + paths: ReviewSetupPath[]; + /** The chain hook exists and is executable — the acceptance predicate's proof, not a re-stat. */ + present: boolean; +} { const origHooksPath = typeof config.origHooksPath === 'string' ? config.origHooksPath @@ -271,10 +303,11 @@ function captureOverlayChain( if (sourcePath === '.') fail('root-level overlay pre-commit chains are not supported by devkit review.'); const chainState = pathState('git', gitRoot, 'overlay-chain', path, false, true); + const present = chainState.fingerprint !== REVIEW_SETUP_ABSENT; const paths = [chainState]; - if (chainState.fingerprint !== REVIEW_SETUP_ABSENT) + if (present) paths.push(pathState('git', gitRoot, 'overlay-chain-source', sourcePath, false, false)); - return { chain: { path, sourcePath }, paths }; + return { chain: { path, sourcePath }, paths, present }; } function captureState(targetRoot: string, gitRoot: string): ReviewSetupState { @@ -286,9 +319,17 @@ function captureState(targetRoot: string, gitRoot: string): ReviewSetupState { if (cause instanceof Error && cause.message.startsWith('devkit review:')) throw cause; return fail(`could not read .devkit/config.json (${errorMessage(cause)}) — ${DOCTOR}`); } - const hooksPath = effectiveHooksPath(gitRoot, parsed.overlay); - const paths = captureSetupPaths(targetRoot, gitRoot, parsed.overlay); + // The chain is captured BEFORE the hooksPath so the acceptance predicate can reuse its verdict: + // whether the repo's committed hook exists and where it lives are two of the preconditions for + // accepting a husky-reclaimed hooksPath, and re-stat'ing them here would let the manifest and the + // acceptance disagree. const overlay = parsed.overlay ? captureOverlayChain(gitRoot, targetRoot, parsed.raw) : null; + const hooksPath = effectiveHooksPath(gitRoot, parsed.overlay, { + gitRoot, + chain: overlay?.chain ?? null, + chainPresent: overlay?.present ?? false, + }); + const paths = captureSetupPaths(targetRoot, gitRoot, parsed.overlay); return { overlay: parsed.overlay, hooksPath, diff --git a/cli/lib/ship/review/setup-profile.mts b/cli/lib/ship/review/setup-profile.mts index 063394fe..7c92a1b2 100644 --- a/cli/lib/ship/review/setup-profile.mts +++ b/cli/lib/ship/review/setup-profile.mts @@ -15,6 +15,13 @@ import { normalizeSafeReviewRelativePath } from './runtime-paths.mts'; import { fail, objectValue } from './shared/common.mts'; export const REVIEW_SETUP_DOCTOR = "run 'devkit doctor --fix'."; +/** + * Overlay's hooksPath remedy. Leads with the DURABLE fix: `doctor --fix` re-points core.hooksPath, + * but husky's `prepare` reclaims it again on the very next install, so pointing there first would + * hand the user a repair that undoes itself. + */ +export const REVIEW_SETUP_OVERLAY_DOCTOR = + "run 'devkit init --overlay --global-commit-gate' (durable — survives husky's reclaim), or 'devkit doctor --fix' (transient — the next install reclaims it)."; export interface RawReviewConfig { overlay?: unknown; diff --git a/cli/lib/ship/review/setup-runtime.mts b/cli/lib/ship/review/setup-runtime.mts index b1a9a5bc..b1d6973c 100644 --- a/cli/lib/ship/review/setup-runtime.mts +++ b/cli/lib/ship/review/setup-runtime.mts @@ -12,6 +12,7 @@ import { isSafeReviewRelativePath, reviewPathWithin, } from './runtime-paths.mts'; +import { overlayHooksPathRejection } from './setup/overlay-hooks-path.mts'; import { copyMergedReviewSetup, reviewSetupStat, @@ -27,6 +28,7 @@ import { reviewSetupRuntimeHash, REVIEW_SETUP_RUNTIME_VERSION as VERSION, } from './setup-runtime-format.mts'; +import { fail } from './shared/common.mts'; import { type ReviewSourceProjection, resolveReviewSource } from './source-projection.mts'; const CHAIN_MIRROR = '.devkit/review-chain-root'; @@ -55,10 +57,6 @@ interface VerifiedSetupSource { fingerprint: string; } -function fail(message: string): never { - throw new Error(`devkit review: ${message}`); -} - function canonicalManifestRoot(path: string, label: string): string { const canonical = canonicalReviewDirectory(path, label); if (canonical !== path) fail(`${label} no longer resolves to its captured location.`); @@ -136,10 +134,32 @@ function readHooksPath(gitRoot: string): string { return result.stdout.subarray(0, -1).toString(); } -function verifySource(context: SetupContext): VerifiedSetupSource[] { - if (readHooksPath(context.gitRoot) !== context.manifest.setup.hooksPath) { - fail('target core.hooksPath changed after setup capture; retry.'); +/** + * Package mode froze the value it read, so literal equality is the whole check. Overlay froze the + * canonical `.devkit/hooks` (what review-target.sh hardcodes for the gate run), so re-run the + * ACCEPTANCE PREDICATE against the live value instead: both `git ci` and `devkit doctor --fix` + * re-point core.hooksPath, and a re-point in either direction mid-review must not abort a run whose + * gates never consulted the live value. It still fails closed — a live value that is no longer + * provably gated (the shim removed, the committed hook deleted) aborts exactly as before. + */ +function verifyHooksPath(context: SetupContext): void { + const live = readHooksPath(context.gitRoot); + const { setup } = context.manifest; + if (!setup.overlay) { + if (live !== setup.hooksPath) fail('target core.hooksPath changed after setup capture; retry.'); + return; } + const chain = setup.paths.find((entry) => entry.id === 'overlay-chain'); + const rejection = overlayHooksPathRejection(live, { + gitRoot: context.gitRoot, + chain: setup.chain, + chainPresent: chain !== undefined && chain.fingerprint !== REVIEW_SETUP_ABSENT, + }); + if (rejection) fail(`target core.hooksPath changed after setup capture (${rejection}); retry.`); +} + +function verifySource(context: SetupContext): VerifiedSetupSource[] { + verifyHooksPath(context); return context.manifest.setup.paths.map((entry) => { const current = inspectSource(context, entry); if (entry.required && current.fingerprint === REVIEW_SETUP_ABSENT) { diff --git a/cli/lib/ship/review/setup/overlay-hooks-path.mts b/cli/lib/ship/review/setup/overlay-hooks-path.mts new file mode 100644 index 00000000..1e0c7129 --- /dev/null +++ b/cli/lib/ship/review/setup/overlay-hooks-path.mts @@ -0,0 +1,163 @@ +/** + * Which live `core.hooksPath` values `devkit review` accepts in OVERLAY mode. + * + * Overlay points core.hooksPath at `.devkit/hooks`, but husky's committed `prepare` resets it to + * `.husky/_` on every install (husky/index.js sets it unconditionally). Review used to treat that + * as fatal drift, so a single `pnpm install` broke `devkit review` in a repo whose commits were + * still fully gated — the opt-in `~/.config/husky/init.sh` shim survives the reclaim and runs the + * overlay hook from inside husky's own chain (see overlay-global-hook.mts, docs/decisions/ + * overlay-self-heal.md). + * + * The literal was the wrong proxy: in overlay mode review-target.sh HARDCODES + * `-c core.hooksPath=.devkit/hooks` for its private gate run, so the captured value steers nothing + * there — it is purely an assertion that the target repo is gated. So widen the assertion instead + * of dropping it: accept the reclaimed value only when every link of the surviving chain is intact, + * and keep failing loudly otherwise. A repo whose gates are genuinely unwired must NOT pass review. + * + * Presence is not proof — `.husky/_/h` and `.husky/_/pre-commit` are checked by CONTENT, because a + * hand-edited or foreign runner with the right filenames would satisfy a presence-only test while + * never reaching the shim. + */ + +import { existsSync, lstatSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { globalHookWired, globalInitPath } from '../../../overlay-global-hook.mts'; + +/** The hooksPath an overlay install writes, and the one review's private gate run always uses. */ +export const OVERLAY_HOOKS_PATH = '.devkit/hooks'; +/** The hooksPath husky reclaims to on every `prepare`. */ +const HUSKY_RUNNER_HOOKS_PATH = '.husky/_'; +/** husky's committed hook scripts live here; `_/h` resolves the real script as `../`. */ +const HUSKY_SCRIPT_DIR = '.husky'; +/** husky 9's per-hook stub is `. "$(dirname "$0")/h"` — it must actually source the runner. */ +const HUSKY_STUB_SOURCES_RUNNER = /^\s*\.\s+.*\/h"?\s*$/m; +/** The variable `_/h` assigns the XDG init.sh path to: `i="${XDG_CONFIG_HOME:-$HOME/.config}/…"`. */ +const HUSKY_INIT_ASSIGNMENT = /(?:^|\s)([A-Za-z_]\w*)=\s*"?[^"\n]*husky\/init\.sh/m; +/** + * A `.`/`source` command in command position. Three lead-ins, because all are valid shell and all + * genuinely source: line start — INDENTED or not, since a dot-source nested in an `if` block is a + * common shape; after a `;`/`&&`/`||`, which is husky's own `[ -f "$i" ] && . "$i"`; and after a + * block keyword, as in `if [ -f "$i" ]; then . "$i"; fi`. Anchoring at a bare `^` (as this did) + * rejects the indented form and hard-fails a genuinely gated repo — the very outcome this module + * exists to prevent, and inconsistent with HUSKY_STUB_SOURCES_RUNNER's own `^\s*`. + */ +const sourceCommand = (target: string) => + new RegExp( + String.raw`(?:^[ \t]*|[;&|]\s*)(?:(?:then|do|else)\s+)?(?:\.|source)\s+["']?${target}`, + 'm', + ); +// A `#` at line start or after whitespace opens a comment. Parameter expansions like `${0##*/}` +// are untouched: their `#` follows a non-space character. +const SHELL_COMMENT = /(^|\s)#.*$/; + +/** + * The script with comments stripped, so a classifier only ever sees lines that RUN. + * + * Without this, `# noop; . "$i"` satisfies a source-command match: the pattern needs only a `;` + * before the dot-source, and a commented-out line still supplies one. Deliberately approximate in + * ONE direction — a `#` inside a quoted string truncates the line early, which can only lose a + * match and cause a REJECTION. This classifier must fail closed: refusing a gated repo is a loud + * error the user can fix, accepting an ungated one is the silent hole this module exists to close. + */ +function executableLines(script: string): string { + return script + .split('\n') + .map((line) => line.replace(SHELL_COMMENT, '$1')) + .filter((line) => line.trim() !== '') + .join('\n'); +} + +/** + * Does `_/h` actually SOURCE the XDG init.sh — not merely mention it? + * + * A bare `includes('husky/init.sh')` is not enough, and the trap is in husky's own runner: its + * deprecation notice for `~/.huskyrc` embeds the literal `~/.config/husky/init.sh` in an echo. A + * runner stripped of its `[ -f "$i" ] && . "$i"` line but still carrying that warning would pass a + * substring test while never reaching the shim — precisely the presence-not-proof hole this module + * exists to close. So find the variable the path is ASSIGNED to, then require a source of it (or a + * direct source of a literal init.sh path). + */ +function runnerSourcesInit(script: string): boolean { + const runner = executableLines(script); + if (sourceCommand(String.raw`[^\n]*husky/init\.sh`).test(runner)) return true; + const assigned = HUSKY_INIT_ASSIGNMENT.exec(runner)?.[1]; + return assigned !== undefined && sourceCommand(String.raw`\$\{?${assigned}\}?`).test(runner); +} + +export interface OverlayHooksPathContext { + gitRoot: string; + /** captureOverlayChain's recorded chain (the repo's own committed hook), or null. */ + chain: { path: string; sourcePath: string } | null; + /** Whether that chain hook exists and is executable — its frozen fingerprint is not ABSENT. */ + chainPresent: boolean; +} + +function readIfFile(path: string): string | null { + const stat = lstatSync(path, { throwIfNoEntry: false }); + if (stat === undefined || !stat.isFile()) return null; + try { + return readFileSync(path, 'utf8'); + } catch { + return null; + } +} + +function isExecutableFile(path: string): boolean { + const stat = lstatSync(path, { throwIfNoEntry: false }); + if (!stat?.isFile()) return false; + return (stat.mode & 0o111) !== 0; +} + +/** + * Why a husky-reclaimed core.hooksPath is NOT provably gated, or null when it is. The order walks + * the chain a real `git commit` takes, so the first failure names the first broken link. + */ +function huskyReclaimRejection(context: OverlayHooksPathContext): string | null { + const { gitRoot } = context; + if (!existsSync(join(gitRoot, OVERLAY_HOOKS_PATH, 'pre-commit'))) + return `${OVERLAY_HOOKS_PATH}/pre-commit is missing, so nothing would run it`; + if (!globalHookWired()) + return `no devkit block in ${globalInitPath()} — husky reclaimed core.hooksPath and nothing re-wires the overlay`; + + const runner = readIfFile(join(gitRoot, HUSKY_RUNNER_HOOKS_PATH, 'h')); + if (runner === null) return `${HUSKY_RUNNER_HOOKS_PATH}/h is missing`; + if (!runnerSourcesInit(runner)) return `${HUSKY_RUNNER_HOOKS_PATH}/h never sources husky/init.sh`; + + const stubPath = join(gitRoot, HUSKY_RUNNER_HOOKS_PATH, 'pre-commit'); + const stub = readIfFile(stubPath); + if (stub === null) return `${HUSKY_RUNNER_HOOKS_PATH}/pre-commit is missing`; + if (!isExecutableFile(stubPath)) return `${HUSKY_RUNNER_HOOKS_PATH}/pre-commit is not executable`; + if (!HUSKY_STUB_SOURCES_RUNNER.test(executableLines(stub))) + return `${HUSKY_RUNNER_HOOKS_PATH}/pre-commit never sources ${HUSKY_RUNNER_HOOKS_PATH}/h`; + + // The chain hook is load-bearing TWICE over. husky 9's `_/h` does `[ ! -f "$s" ] && exit 0` + // BEFORE it sources init.sh, so with no committed .husky/pre-commit the shim never fires at all + // and commits are genuinely ungated (the residual hole overlay-self-heal.md documents). And the + // chain must resolve through .husky: an overlay installed BEFORE husky records origHooksPath '' + // and chains to .git/hooks, which husky's runner would never execute — review would then clear a + // commit whose real hook chain it never ran. + if (context.chain === null || context.chain.sourcePath !== HUSKY_SCRIPT_DIR) + return `the overlay chains to ${context.chain?.sourcePath ?? '(nothing)'}, not ${HUSKY_SCRIPT_DIR} — husky's runner would never reach it`; + // Stricter than husky's own `[ ! -f "$s" ]` test on purpose: captureOverlayChain freezes the + // chain hook as executable, and a mode-0644 committed hook is a setup devkit will not vouch for. + if (!context.chainPresent) + return `${context.chain.path} is missing or not executable, so husky's runner exits before sourcing the shim`; + return null; +} + +/** + * Why `value` is unacceptable as an overlay repo's live core.hooksPath, or null when it is fine. + * `.devkit/hooks` is accepted outright; `.husky/_` only on proof; anything else is drift. + */ +export function overlayHooksPathRejection( + value: string, + context: OverlayHooksPathContext, +): string | null { + if (value === OVERLAY_HOOKS_PATH) return null; + if (value !== HUSKY_RUNNER_HOOKS_PATH) + return `core.hooksPath is ${JSON.stringify(value || '(unset)')}, expected ${OVERLAY_HOOKS_PATH}`; + const rejection = huskyReclaimRejection(context); + return rejection === null + ? null + : `core.hooksPath is ${JSON.stringify(value)} (husky reclaimed it) and the overlay is not provably still gated: ${rejection}`; +} diff --git a/dist/cli/lib/doctor/overlay-doctor.mjs b/dist/cli/lib/doctor/overlay-doctor.mjs new file mode 100644 index 00000000..a28e181d --- /dev/null +++ b/dist/cli/lib/doctor/overlay-doctor.mjs @@ -0,0 +1,97 @@ +/** + * Overlay-mode doctor. Overlay health is gated by its local hook + `core.hooksPath`; agent assets + * and fallow are advisory (printed, never in the exit code) because a re-run re-syncs them. + * + * Lives here beside `self-host-doctor.mts` — the same shape, a mode-specific doctor in its own + * module — rather than in `doctor.mts`, which is at its line budget. + */ +import { execFileSync } from 'node:child_process'; +import { existsSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { detectGitRoot } from "../detect-git-root.mjs"; +import { resolveExistingAgentProviders } from "../install/agent-assets/agent-providers.mjs"; +import { selectedHookAssets } from "../install/hook-registration-ledger/selection.mjs"; +import { HEAL_ALIAS_NAME, isHealAlias, syncOverlayHook } from "../overlay.mjs"; +import { globalHookInstalled, globalInitPath } from "../overlay-global-hook.mjs"; +import { checkAgentAssets, checkRegistrations } from "./asset-checks.mjs"; +import { adviseSearchIndex } from "./guard-config-checks.mjs"; +import { repointHooksPath } from "./hook-checks.mjs"; +// Reason: flat signal reporting keeps the exit code gated only on hook + path. +// fallow-ignore-next-line complexity +export async function runOverlayDoctor(cwd, cfg, fix, printQavisAdvisoryHealth) { + // hooksPath and its alias are repo-wide, including for a monorepo package. + const { gitRoot } = detectGitRoot(cwd); + const gitGet = (key) => { + try { + return execFileSync('git', ['config', '--get', key], { + cwd: gitRoot, + encoding: 'utf8', + }).trim(); + } + catch { + return ''; // unset + } + }; + const hooksPath = gitGet('core.hooksPath'); + const aliasOurs = isHealAlias(gitGet(`alias.${HEAL_ALIAS_NAME}`)); + // Compare the ignored overlay hook with a fresh build; --fix rewrites stale/missing copies. + const sync = syncOverlayHook(gitRoot, cwd, cfg, { dryRun: !fix }); + const hookOk = existsSync(join(gitRoot, '.devkit', 'hooks', 'pre-commit')); // post-fix presence + const healed = fix && hooksPath !== '.devkit/hooks' && repointHooksPath(gitRoot, hookOk); + const pathOk = healed || hooksPath === '.devkit/hooks'; + console.log('devkit doctor — overlay (local-only)\n'); + if (!hookOk) + console.log(' ✗ .devkit/hooks/pre-commit MISSING — run `devkit doctor --fix` (or `devkit init --overlay`)'); + else if (fix && (sync.missing || sync.drift)) + console.log(' ✓ .devkit/hooks/pre-commit regenerated (was stale/missing — refreshed to the current devkit)'); + else if (sync.drift) + console.log(' ⚠ .devkit/hooks/pre-commit is STALE (predates the current devkit) — run `devkit doctor --fix` to refresh'); + else + console.log(' ✓ .devkit/hooks/pre-commit present'); + console.log(` ${pathOk ? '✓' : '⚠'} core.hooksPath = ${healed ? `.devkit/hooks (re-pointed from ${hooksPath || '(unset)'}; husky reclaims it on every install — make it durable with \`devkit init --overlay --global-commit-gate\`)` : hooksPath || '(unset)'}${pathOk ? '' : ` — heal with \`git ${HEAL_ALIAS_NAME}\` (re-points it), \`devkit doctor --fix\`, or re-run \`devkit init --overlay\``}`); + // Advisory only — never affects the exit code (hook + path are the real health signal). + if (aliasOurs && !hookOk) + console.log(` ⚠ git ${HEAL_ALIAS_NAME} points at a missing .devkit/hooks — run \`devkit clean\``); + else if (aliasOurs) + console.log(` ✓ git ${HEAL_ALIAS_NAME} self-heal alias`); + else + console.log(` · self-heal off (git ${HEAL_ALIAS_NAME} re-points core.hooksPath; or re-run \`devkit init --overlay\`)`); + // The opt-in global shim gates plain commits after Husky reclaims hooksPath; advisory here. + if (globalHookInstalled()) { + console.log(` ✓ global pre-commit gate (${globalInitPath()}) — plain \`git commit\` gated`); + if (aliasOurs) + console.log(` (git ${HEAL_ALIAS_NAME} is the CLI fast-path; shim + alias don't double-run)`); + // Husky cannot source the shim without a committed .husky/pre-commit. + const huskyPresent = existsSync(join(gitRoot, '.husky', '_')) || existsSync(join(gitRoot, '.husky')); + if (huskyPresent && !existsSync(join(gitRoot, '.husky', 'pre-commit'))) + console.log(` ⚠ no committed .husky/pre-commit — husky won't source the shim for pre-commit; a plain \`git commit\` stays ungated here (use \`git ${HEAL_ALIAS_NAME}\`)`); + } + else if (!pathOk) { + console.log(` · plain \`git commit\` is ungated (husky reclaimed core.hooksPath); \`git ${HEAL_ALIAS_NAME}\` heals it, or wire it permanently with \`devkit init --overlay --global-commit-gate\``); + } + // Agent-half + fallow checks — ADVISORY (printed, never gate the exit code; a re-run re-syncs them). + const recorded = cfg?.components ?? {}; + const surfaces = resolveExistingAgentProviders(gitRoot, recorded.agentTargets); + const sel = { ...recorded, agentTargets: surfaces }; + const advise = (r) => console.log(` ${r.status === 'OK' ? '✓' : '·'} ${r.name}: ${r.detail}`); + const hooks = selectedHookAssets(sel, { searchSteering: false }); + if (sel.skills && surfaces.length) + advise(checkAgentAssets(cwd, 'skills', surfaces, sel)); + if (sel.agents && surfaces.length) + advise(checkAgentAssets(cwd, 'agents', surfaces)); + if (hooks.scripts.length && surfaces.length) + advise(checkAgentAssets(cwd, 'hooks', surfaces, { expected: hooks.scripts })); + if (surfaces.length) + advise(checkRegistrations(cwd, hooks.components, surfaces, true)); + // Overlay short-circuits before collectResults, so the dup gate's silent opt-out would otherwise + // be undetectable here. Advisory: overlay health is gated on hook + hooksPath. + await adviseSearchIndex(cwd, sel); + printQavisAdvisoryHealth(cwd, sel.guards ?? []); + if (sel.fallow) { + const wired = hookOk && + readFileSync(join(gitRoot, '.devkit', 'hooks', 'pre-commit'), 'utf8').includes('fallow audit'); + console.log(` ${wired ? '✓' : '·'} fallow gate: ${wired ? 'wired in the local hook' : 'not wired'}`); + } + // A stale hook is unhealthy (exit 1) so CI/agents notice; --fix having just regenerated it heals this run. + return hookOk && pathOk && (fix || !sync.drift) ? 0 : 1; +} diff --git a/dist/cli/lib/ship/review/setup/overlay-hooks-path.mjs b/dist/cli/lib/ship/review/setup/overlay-hooks-path.mjs new file mode 100644 index 00000000..42db627b --- /dev/null +++ b/dist/cli/lib/ship/review/setup/overlay-hooks-path.mjs @@ -0,0 +1,146 @@ +/** + * Which live `core.hooksPath` values `devkit review` accepts in OVERLAY mode. + * + * Overlay points core.hooksPath at `.devkit/hooks`, but husky's committed `prepare` resets it to + * `.husky/_` on every install (husky/index.js sets it unconditionally). Review used to treat that + * as fatal drift, so a single `pnpm install` broke `devkit review` in a repo whose commits were + * still fully gated — the opt-in `~/.config/husky/init.sh` shim survives the reclaim and runs the + * overlay hook from inside husky's own chain (see overlay-global-hook.mts, docs/decisions/ + * overlay-self-heal.md). + * + * The literal was the wrong proxy: in overlay mode review-target.sh HARDCODES + * `-c core.hooksPath=.devkit/hooks` for its private gate run, so the captured value steers nothing + * there — it is purely an assertion that the target repo is gated. So widen the assertion instead + * of dropping it: accept the reclaimed value only when every link of the surviving chain is intact, + * and keep failing loudly otherwise. A repo whose gates are genuinely unwired must NOT pass review. + * + * Presence is not proof — `.husky/_/h` and `.husky/_/pre-commit` are checked by CONTENT, because a + * hand-edited or foreign runner with the right filenames would satisfy a presence-only test while + * never reaching the shim. + */ +import { existsSync, lstatSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { globalHookWired, globalInitPath } from "../../../overlay-global-hook.mjs"; +/** The hooksPath an overlay install writes, and the one review's private gate run always uses. */ +export const OVERLAY_HOOKS_PATH = '.devkit/hooks'; +/** The hooksPath husky reclaims to on every `prepare`. */ +const HUSKY_RUNNER_HOOKS_PATH = '.husky/_'; +/** husky's committed hook scripts live here; `_/h` resolves the real script as `../`. */ +const HUSKY_SCRIPT_DIR = '.husky'; +/** husky 9's per-hook stub is `. "$(dirname "$0")/h"` — it must actually source the runner. */ +const HUSKY_STUB_SOURCES_RUNNER = /^\s*\.\s+.*\/h"?\s*$/m; +/** The variable `_/h` assigns the XDG init.sh path to: `i="${XDG_CONFIG_HOME:-$HOME/.config}/…"`. */ +const HUSKY_INIT_ASSIGNMENT = /(?:^|\s)([A-Za-z_]\w*)=\s*"?[^"\n]*husky\/init\.sh/m; +/** + * A `.`/`source` command in command position. Three lead-ins, because all are valid shell and all + * genuinely source: line start — INDENTED or not, since a dot-source nested in an `if` block is a + * common shape; after a `;`/`&&`/`||`, which is husky's own `[ -f "$i" ] && . "$i"`; and after a + * block keyword, as in `if [ -f "$i" ]; then . "$i"; fi`. Anchoring at a bare `^` (as this did) + * rejects the indented form and hard-fails a genuinely gated repo — the very outcome this module + * exists to prevent, and inconsistent with HUSKY_STUB_SOURCES_RUNNER's own `^\s*`. + */ +const sourceCommand = (target) => new RegExp(String.raw `(?:^[ \t]*|[;&|]\s*)(?:(?:then|do|else)\s+)?(?:\.|source)\s+["']?${target}`, 'm'); +// A `#` at line start or after whitespace opens a comment. Parameter expansions like `${0##*/}` +// are untouched: their `#` follows a non-space character. +const SHELL_COMMENT = /(^|\s)#.*$/; +/** + * The script with comments stripped, so a classifier only ever sees lines that RUN. + * + * Without this, `# noop; . "$i"` satisfies a source-command match: the pattern needs only a `;` + * before the dot-source, and a commented-out line still supplies one. Deliberately approximate in + * ONE direction — a `#` inside a quoted string truncates the line early, which can only lose a + * match and cause a REJECTION. This classifier must fail closed: refusing a gated repo is a loud + * error the user can fix, accepting an ungated one is the silent hole this module exists to close. + */ +function executableLines(script) { + return script + .split('\n') + .map((line) => line.replace(SHELL_COMMENT, '$1')) + .filter((line) => line.trim() !== '') + .join('\n'); +} +/** + * Does `_/h` actually SOURCE the XDG init.sh — not merely mention it? + * + * A bare `includes('husky/init.sh')` is not enough, and the trap is in husky's own runner: its + * deprecation notice for `~/.huskyrc` embeds the literal `~/.config/husky/init.sh` in an echo. A + * runner stripped of its `[ -f "$i" ] && . "$i"` line but still carrying that warning would pass a + * substring test while never reaching the shim — precisely the presence-not-proof hole this module + * exists to close. So find the variable the path is ASSIGNED to, then require a source of it (or a + * direct source of a literal init.sh path). + */ +function runnerSourcesInit(script) { + const runner = executableLines(script); + if (sourceCommand(String.raw `[^\n]*husky/init\.sh`).test(runner)) + return true; + const assigned = HUSKY_INIT_ASSIGNMENT.exec(runner)?.[1]; + return assigned !== undefined && sourceCommand(String.raw `\$\{?${assigned}\}?`).test(runner); +} +function readIfFile(path) { + const stat = lstatSync(path, { throwIfNoEntry: false }); + if (stat === undefined || !stat.isFile()) + return null; + try { + return readFileSync(path, 'utf8'); + } + catch { + return null; + } +} +function isExecutableFile(path) { + const stat = lstatSync(path, { throwIfNoEntry: false }); + if (!stat?.isFile()) + return false; + return (stat.mode & 0o111) !== 0; +} +/** + * Why a husky-reclaimed core.hooksPath is NOT provably gated, or null when it is. The order walks + * the chain a real `git commit` takes, so the first failure names the first broken link. + */ +function huskyReclaimRejection(context) { + const { gitRoot } = context; + if (!existsSync(join(gitRoot, OVERLAY_HOOKS_PATH, 'pre-commit'))) + return `${OVERLAY_HOOKS_PATH}/pre-commit is missing, so nothing would run it`; + if (!globalHookWired()) + return `no devkit block in ${globalInitPath()} — husky reclaimed core.hooksPath and nothing re-wires the overlay`; + const runner = readIfFile(join(gitRoot, HUSKY_RUNNER_HOOKS_PATH, 'h')); + if (runner === null) + return `${HUSKY_RUNNER_HOOKS_PATH}/h is missing`; + if (!runnerSourcesInit(runner)) + return `${HUSKY_RUNNER_HOOKS_PATH}/h never sources husky/init.sh`; + const stubPath = join(gitRoot, HUSKY_RUNNER_HOOKS_PATH, 'pre-commit'); + const stub = readIfFile(stubPath); + if (stub === null) + return `${HUSKY_RUNNER_HOOKS_PATH}/pre-commit is missing`; + if (!isExecutableFile(stubPath)) + return `${HUSKY_RUNNER_HOOKS_PATH}/pre-commit is not executable`; + if (!HUSKY_STUB_SOURCES_RUNNER.test(executableLines(stub))) + return `${HUSKY_RUNNER_HOOKS_PATH}/pre-commit never sources ${HUSKY_RUNNER_HOOKS_PATH}/h`; + // The chain hook is load-bearing TWICE over. husky 9's `_/h` does `[ ! -f "$s" ] && exit 0` + // BEFORE it sources init.sh, so with no committed .husky/pre-commit the shim never fires at all + // and commits are genuinely ungated (the residual hole overlay-self-heal.md documents). And the + // chain must resolve through .husky: an overlay installed BEFORE husky records origHooksPath '' + // and chains to .git/hooks, which husky's runner would never execute — review would then clear a + // commit whose real hook chain it never ran. + if (context.chain === null || context.chain.sourcePath !== HUSKY_SCRIPT_DIR) + return `the overlay chains to ${context.chain?.sourcePath ?? '(nothing)'}, not ${HUSKY_SCRIPT_DIR} — husky's runner would never reach it`; + // Stricter than husky's own `[ ! -f "$s" ]` test on purpose: captureOverlayChain freezes the + // chain hook as executable, and a mode-0644 committed hook is a setup devkit will not vouch for. + if (!context.chainPresent) + return `${context.chain.path} is missing or not executable, so husky's runner exits before sourcing the shim`; + return null; +} +/** + * Why `value` is unacceptable as an overlay repo's live core.hooksPath, or null when it is fine. + * `.devkit/hooks` is accepted outright; `.husky/_` only on proof; anything else is drift. + */ +export function overlayHooksPathRejection(value, context) { + if (value === OVERLAY_HOOKS_PATH) + return null; + if (value !== HUSKY_RUNNER_HOOKS_PATH) + return `core.hooksPath is ${JSON.stringify(value || '(unset)')}, expected ${OVERLAY_HOOKS_PATH}`; + const rejection = huskyReclaimRejection(context); + return rejection === null + ? null + : `core.hooksPath is ${JSON.stringify(value)} (husky reclaimed it) and the overlay is not provably still gated: ${rejection}`; +} diff --git a/dist/cli/lib/ship/review/shared/common.mjs b/dist/cli/lib/ship/review/shared/common.mjs new file mode 100644 index 00000000..e7d808ea --- /dev/null +++ b/dist/cli/lib/ship/review/shared/common.mjs @@ -0,0 +1,33 @@ +/** + * Shared helpers for the review runtime family (cli/lib/ship/review/**). Extracted when the + * clone gate surfaced the same fail/errorMessage/objectValue/gitEnvironment bodies copy-pasted + * across repository/, cache/, and the setup-manifest files (sc-1414). New review modules should + * import from here instead of re-declaring. + */ +/** Uniform review-runtime failure: every thrown message carries the `devkit review:` prefix. */ +export function fail(message) { + throw new Error(`devkit review: ${message}`); +} +export function errorMessage(cause) { + return cause instanceof Error ? cause.message : String(cause); +} +/** Assert a parsed JSON value is a plain object; `message` is the caller's exact failure text. */ +export function objectValue(value, message) { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + fail(message); + } + return value; +} +/** + * A GIT_*-stripped environment for spawning read-only git commands: inherited GIT_DIR / + * GIT_INDEX_FILE etc. from an enclosing hook must never leak into children. Always forbids + * opportunistic lock-taking (GIT_OPTIONAL_LOCKS=0); callers layer extra pins on top. + */ +export function gitEnvironment(extra = {}) { + const env = { ...process.env }; + for (const name of Object.keys(env)) { + if (name.startsWith('GIT_')) + delete env[name]; + } + return { ...env, GIT_OPTIONAL_LOCKS: '0', ...extra }; +} diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 01ba15c2..09a15c13 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -39,7 +39,7 @@ timeline. New rationale lives in the per-axis file. | [new-optional-component-offer](new-optional-component-offer.md) | A single registry, OPTIONAL_COMPONENTS in components.mts, drives a generic upgrade step (3c) that offers every optional component a repo has never been asked about. 'Never asked' is detected by the ABSENCE of the recorded key, not a falsy value: applyInit writes every component key on every run, so a repo that answered — yes OR no — carries the key and is never asked again, while a repo predating the component has no key at all. No per-repo 'offers made' state. Nothing is ever auto-added: non-TTY REPORTS only, matching the step-3 gates policy, because an opt-in component arriving because someone ran upgrade in CI is a defect. Correspondingly, a run where nobody was actually asked (non-TTY, or a cancelled prompt) passes those ids to applyInit as InitPlan.undecided, and EVERY config writer — applyInit and applyOverlay alike — runs dropUndecided, so their keys stay absent; otherwise step 4's broad refresh records the normalized 'false' as a decision nobody made and suppresses the offer permanently. The interactive wizard seeds an already-installed optional component into its defaults for the same reason: accepting the defaults on a re-run must not silently drop one. | devkit ships opt-in components after most consumer repos are alread… | 2026-07-29 | | [non-devkit-asset-collision-preserve](non-devkit-asset-collision-preserve.md) | a sync treats a name as the CONSUMER's (preserve, never clobber) iff it (1) exists under a target surface, (2) is NOT recorded in devkit's prior manifest, AND (3) its on-disk bytes DIVERGE from the bundle. Default everywhere is PRESERVE. `devkit init` interactive offers a per-asset `multiselect` (keyed `${kind}:${name}`) to adopt specific collisions; `--force` (package/standalone/overlay) and the standalone `sync-skills`/`sync-agents --force` adopt all. A preserved name is left off the manifest (devkit never claims a file it didn't write). devkit's OWN copies — manifest-owned, or unmanifested-but-byte-identical to the bundle — keep overwriting, so version-bump propagation and self-dogfood are intact. `clean`'s no-manifest fallback gains the same content/tracked guard so it never deletes a preserved untracked user asset. | the sync step (syncSkills/syncAgents/syncHookScripts) hardcoded `wr… | 2026-06-30 | | [open-ended-reviewer-gold-slots](open-ended-reviewer-gold-slots.md) | The completeness bench scores the reviewer against per-case GOLD SLOTS (gaps that must surface, each with target severity) plus DECOYS (recorded decisions / out-of-scope items it must not flag), mapped by an LLM matcher that asks one forced-choice question per slot (never one holistic list-to-list call), votes majority-of-K, and is itself audited (committed labels, Cohen's kappa >= 0.7 to be trusted) and hashed into the baseline (matcherHash) so a matcher edit invalidates comparisons exactly like a gate edit. Headline metrics are gap recall (hard floor) and decoy false-flag rate (hard ceiling); severity calibration is warn-tier; the flip gate clusters by CASE because slots within a case share one reviewer transcript. | A prompt/model edit to the 18KB feature-completeness-reviewer brief… | 2026-07-05 | -| [overlay-self-heal](overlay-self-heal.md) | `devkit upgrade` handles overlay in a self-contained branch mirroring the self-host branch. It resolves the pin install-agnostically from config.json's `devkitRef` (falling back to the running CLI version), chases a newer PUBLISHED tag exactly like package mode (`fetchLatestTag` → `update` → `needsRerun`/`NEEDS_RERUN`) — where the "install" for overlay is `bun add -g` (the global CLI is node_modules' analog) — then re-syncs via `applyInit({ overlay:true, devkitRef:'v'+target, globalCommitGate: cfg.globalCommitGate })` and runs `doctor`. `globalCommitGate` is threaded from the recorded config so a re-sync never un-wires an opted-in machine-global shim (applyOverlay reads `plan.globalCommitGate`, NOT the file). Deliberately omitted vs package upgrade: the gate-reconcile multiselect and `computeMigration` — overlay's `guard.config.json`/lint configs are `writeIfAbsent` extend-configs, and the load-bearing refresh is the regenerated `.devkit/hooks/pre-commit`. | `devkit upgrade` bailed on an overlay repo (`overlay (local-only) r… | 2026-07-14 | +| [overlay-self-heal](overlay-self-heal.md) | The hooksPath literal is an ASSERTION in overlay mode, not an input — review-target.sh hardcodes -c core.hooksPath=.devkit/hooks for its private gate run — so widen the assertion rather than drop it. Review freezes the canonical .devkit/hooks and validates the LIVE value through an acceptance predicate: .devkit/hooks outright, .husky/_ only when every link of the surviving chain is proven by content (devkit block complete in the resolved init.sh, _/h sourcing husky/init.sh, an executable _/pre-commit sourcing _/h, a committed .husky/pre-commit, and a chain resolving through .husky). devkit doctor --fix additionally re-points core.hooksPath, refusing inside a linked worktree. | The 2026-06-29 Target closed the plain-commit gap with the global h… | 2026-08-05 | | [plan-critique-evidence-loop](plan-critique-evidence-loop.md) | Keep feature-critique plan-only and preserve its substantive reasoning guidance. Return one strict normalized JSON response; allow one fresh recheck only after a blocking result. Treat Claude, Codex, and Cursor as fresh-install agent targets by default, while activating fail-open private evidence capture only where the audited provider contract exposes the exact completed critique and trustworthy parent-plan correlation. The current capability registry therefore enables capture for Claude and records explicit capability skips for Codex and Cursor; default installation does not imply capture availability. Repo-graph imports sanitized projections through its existing memory layer. Commit gates observe a deterministic would-inject projection only and never change reviewer inputs or outcomes until paired benchmarks justify injection. | Feature critiques currently overwrite provider-specific runtime fil… | 2026-07-19 | | [published-version-tags-immutable](published-version-tags-immutable.md) | A published version tag is immutable and published history is never rewritten. This is a hard constraint, not a preference, because bun records in bun.lock the OBJECT a tag resolved to — the annotated tag object for a github: shorthand pin, the peeled commit for a git+ssh/git+https pin — never the tag name; re-pointing a tag therefore orphans every SHA already recorded downstream. Enforced in depth: devkit release refuses a tag that exists locally OR on origin (remoteTagExists, network failure = proceed so offline is not blocked), and devkit doctor's new 'devkit lock' check verifies the recorded object still resolves on the remote the consumer actually pinned, so a dead pin is found on a working machine instead of in someone else's CI. | sc-1449: a consumer's bun install died with 'no commit matching "59… | 2026-08-04 | | [qavis-advisory-gate](qavis-advisory-gate.md) | Split by ownership. (1) qavis owns the classifier (`route --staged --diff --gate`, a haiku judge that reads the diff — bench-picked at recall 0.96 / false-advise 0.00, clearing the floors regex can't) and a content-addressed pass-receipt (`.qavis/receipt.json`, sha256 over each changed file's blob sha, so a staged index and an identical committed tree match). (2) devkit is a THIN CHANNEL: `gate-engine/qavis-advisory` shells `qavis route --staged --gate`, and fail-OPENS (exit 0) when qavis or a `.qavis/recipe.json` is absent — the fallow precedent, zero weight for non-qavis consumers. (3) The exit contract is advisory: 0 = continue (SILENT / advisory-only on a normal commit / receipt-cleared / qavis absent), 3 = ADVISE under a strict ship (`GUARD_AI_STRICT`) — the ship blocks until qavis runs (writing a receipt that clears it) or an override is set. There is NO exit 1 and NO fail-closed on outage (unlike completeness): an advisor's own failure must never block a ship. Overrides: `GUARD_QAVIS_OK=1` ships without QA, `GUARD_NO_QAVIS_ADVISORY=1` disables. Wired as its own husky fragment (bunx/standalone/overlay) with its own remedy line, NOT an AI_GUARD (different exit contract). | qavis (a computer-vision PR QA agent) only ran on an explicit reque… | 2026-07-07 | diff --git a/docs/decisions/overlay-self-heal.md b/docs/decisions/overlay-self-heal.md index 984d99f3..cda4c26c 100644 --- a/docs/decisions/overlay-self-heal.md +++ b/docs/decisions/overlay-self-heal.md @@ -65,3 +65,19 @@ created: 2026-06-26 **Anchored-bet:** [VALIDATED] **Scope:** cli/commands/upgrade.mts,cli/commands/update.mts **Source:** collab · overlay-upgrade-resync + +## Target · 2026-08-05 — review accepts a husky-reclaimed hooksPath when the overlay is provably still gated; doctor --fix owns the re-point + +**Context:** The 2026-06-29 Target closed the plain-commit gap with the global husky init.sh shim, but nothing re-pointed core.hooksPath itself, and devkit review asserted the literal '.devkit/hooks'. So on owners-web a single pnpm install (husky's prepare resets core.hooksPath to .husky/_ on EVERY install) broke devkit review outright while commits stayed fully gated through the shim — review refused to run on a repo it had no reason to distrust. The remedy it printed, 'devkit doctor --fix', could not repair core.hooksPath at all: doctor only warned. So the failure was unfixable by the command the failure named, and recurred after every install. +**Ruling:** The hooksPath literal is an ASSERTION in overlay mode, not an input — review-target.sh hardcodes -c core.hooksPath=.devkit/hooks for its private gate run — so widen the assertion rather than drop it. Review freezes the canonical .devkit/hooks and validates the LIVE value through an acceptance predicate: .devkit/hooks outright, .husky/_ only when every link of the surviving chain is proven by content (devkit block complete in the resolved init.sh, _/h sourcing husky/init.sh, an executable _/pre-commit sourcing _/h, a committed .husky/pre-commit, and a chain resolving through .husky). devkit doctor --fix additionally re-points core.hooksPath, refusing inside a linked worktree. +**Consequences:** +- Positive: devkit review keeps working across the pnpm installs that a shared repo runs constantly, instead of failing on a proxy that says nothing about whether the repo is gated; and the remedy devkit prints is now a remedy that works. A genuinely unwired overlay still fails review loudly, and the failure names the first broken link plus the resolved init.sh path, so a HOME/XDG mismatch is distinguishable from an uninstalled shim. +- Negative: Review's gating claim now depends on a machine-owned file outside both manifest roots (~/.config/husky/init.sh), which is checked but never fingerprinted — removing it mid-review is caught only at the next verify, not by the frozen-path set. The predicate encodes husky 9's runner shape (the _/h early-exit, the _/pre-commit source line), so a husky major that restructures its runner turns acceptance into a false rejection. And doctor --fix now writes a consumer's git config, a wider blast radius than the file-content-only rule it previously held to. +**Vision-fit:** n/a — internal dev tooling; overlay runs guardrails on a shared repo a team would reject a PR for. +**Researched:** Verified against husky 9.1.7 on disk: .husky/_/h line 6 '[ ! -f "$s" ] && exit 0' precedes the XDG init.sh source at lines 11-12, so a repo with no committed .husky/pre-commit is genuinely ungated and must still be rejected. Traced the frozen value to its three consumers (setup-manifest-parse.mts, review-target.sh field guard, setup-runtime.mts verifySource) and confirmed review-target.sh:646 hardcodes .devkit/hooks for the overlay gate run while $HOOKS_PATH reaches git only in the non-overlay branch at :651. Precedent: sc-1329 (devkit ship unconditionally required .husky/_ and rejected a valid standalone install) and prepare-gate-worktree.sh's 'resolved, never hardcoded' hook-dir projection. A feature-critique pass returned PROCEED_WITH_CHANGES and drove the canonicalisation and the linked-worktree refusal. +**Rejected:** (a) drop the overlay hooksPath check entirely and rely on the hook-content drift check — REJECTED: review would then greenlight a repo where husky reclaimed the path and no shim exists, i.e. commits genuinely ungated, which is the one thing the check exists to prevent. (b) freeze the raw reclaimed '.husky/_' and thread a second accepted literal through the parser and the shell field guard — REJECTED: the frozen value is re-compared against the live one throughout a review, and the live value is transient (both git ci and doctor --fix re-point it), so a concurrent re-point in another terminal would abort an in-flight review. (c) fix only the misleading remedy string and leave the check fatal — REJECTED: review stays broken after every pnpm install, which is the actual recurrence. (d) accept on presence of _/h and _/pre-commit rather than their content — REJECTED: that reinstates a weaker proxy, and the whole point is that acceptance must be proof. +**Anchored-bet:** [VALIDATED] +**Revisit-when:** husky changes its runner contract — either _/h stops sourcing $XDG_CONFIG_HOME/husky/init.sh, or it stops exiting before that source when no committed hook exists — or husky's prepare stops setting core.hooksPath unconditionally, at which point the reclaim this works around disappears. +**Scope:** cli/lib/ship/review/**,cli/lib/ship/review-target.sh,cli/commands/doctor.mts,cli/lib/overlay-global-hook.mts +**Source:** collab · sc-1329 +**Evidence-change:** The 2026-07-22 devkit-owned-hook-runner-delivery Target rejected re-pointing core.hooksPath partly BECAUSE it broke review's exact string equality against '.husky/_' — this change removes that coupling, so the reason no longer holds. That record's '--fix is file-content-only' rule was drawn around a git INDEX write (git add -f), which stages files the caller never asked to stage; a core.hooksPath write is local, reversible, already performed by init and clean, and is the exact repair the failing command advertises. The narrower rule that survives: --fix never mutates the git index, and never moves the pointer ahead of the hook it points at. diff --git a/eslint/baselines/size-lines.json b/eslint/baselines/size-lines.json index 8823bb72..daf9c925 100644 --- a/eslint/baselines/size-lines.json +++ b/eslint/baselines/size-lines.json @@ -2,7 +2,7 @@ "maxLines": 500, "maxTestLines": 0, "files": { - "cli/commands/doctor.mts": 525, + "cli/commands/doctor.mts": 426, "cli/commands/init.mts": 1280, "cli/lib/generate/generate-structure-baseline.mts": 876, "cli/lib/husky/husky-block.mts": 510, From 2a4d2b9fbf1080c0f4723647fbc6ceaf86297395 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Thu, 6 Aug 2026 09:59:09 +0100 Subject: [PATCH 2/2] fix(review): match the global shim block verbatim; harden stat failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #351. globalHookWired compared a substring (the overlay hook path) between the markers, which `# .devkit/hooks/pre-commit` satisfies while husky runs nothing. It now matches this devkit's generated BLOCK byte-for-byte — the same standard reviewHookDrift already applies to the pre-commit block, and it needs no shell parsing. A block devkit did not generate reads as NOT wired, which is fail-closed; `devkit init --overlay --global-commit-gate` restores it (installGlobalHook is strip-then-reinsert). readIfFile and isExecutableFile called lstatSync outside their try blocks. `throwIfNoEntry: false` only silences ENOENT, so an unreadable parent (EACCES) or a symlink cycle (ELOOP) escaped as an unhandled error from predicates whose job is to return a diagnostic. Both now fail closed. size-lines.json: doctor.mts recorded 426 against an actual gate count of 422. The gate counts split('\n').length (size-disable.mts:138), i.e. wc -l + 1; audited by that method, every other entry is already exact. --- cli/__tests__/review-setup-manifest.test.mts | 16 +++++++++ cli/lib/overlay-global-hook.mts | 27 +++++++-------- .../ship/review/setup/overlay-hooks-path.mts | 18 ++++++---- dist/cli/lib/overlay-global-hook.mjs | 33 +++++++++++++++++-- .../ship/review/setup/overlay-hooks-path.mjs | 19 +++++++---- eslint/baselines/size-lines.json | 2 +- 6 files changed, 86 insertions(+), 29 deletions(-) diff --git a/cli/__tests__/review-setup-manifest.test.mts b/cli/__tests__/review-setup-manifest.test.mts index ad1bb838..24448054 100644 --- a/cli/__tests__/review-setup-manifest.test.mts +++ b/cli/__tests__/review-setup-manifest.test.mts @@ -493,6 +493,22 @@ describe('review setup manifest — husky-reclaimed overlay hooksPath', () => { expect(() => captureReviewSetup(root, manifest)).toThrow(/--global-commit-gate/); }); + it('rejects a shim whose hook invocation is commented out but still present', () => { + // Both markers intact and the hook path still in the text, so any substring test passes — yet + // husky runs nothing. The block is compared verbatim against the generator for exactly this. + const { root, manifest } = reclaimed('commented-shim'); + const initSh = globalInitPath(); + const disabled = readFileSync(initSh, 'utf8').replace( + /^(\s*)(DEVKIT_VIA_HUSKY_INIT=1 sh )/m, + '$1# $2', + ); + expect(disabled).toContain('.devkit/hooks/pre-commit'); // the path survives the edit + expect(disabled).toContain('<<< devkit overlay global pre-commit gate <<<'); // markers intact + writeFileSync(initSh, disabled); + + expect(() => captureReviewSetup(root, manifest)).toThrow(/no devkit block in/); + }); + it('rejects a shim block truncated after its start marker', () => { const { root, manifest } = reclaimed('truncated-shim'); const initSh = globalInitPath(); diff --git a/cli/lib/overlay-global-hook.mts b/cli/lib/overlay-global-hook.mts index 753e1e7a..559bfbbd 100644 --- a/cli/lib/overlay-global-hook.mts +++ b/cli/lib/overlay-global-hook.mts @@ -66,25 +66,26 @@ export function globalHookInstalled() { } /** - * True iff the global init.sh carries a COMPLETE, still-wired devkit block: both markers AND the - * line that actually runs the overlay hook between them. + * True iff the global init.sh carries this devkit's block VERBATIM. * * `globalHookInstalled` (MARK_START alone) stays doctor's advisory signal — a start marker is - * enough to say "you opted in". `devkit review` needs the stronger claim, because this shim is the - * ONLY thing keeping an overlay repo gated once husky reclaims core.hooksPath: a block truncated - * after its start marker (a half-applied hand edit, an interrupted write) passes the advisory test - * while gating nothing. + * enough to say "you opted in". `devkit review` needs a stronger claim, because this shim is the + * ONLY thing keeping an overlay repo gated once husky reclaims core.hooksPath. + * + * Exact-match rather than grepping for the hook path between the markers: any substring test is + * satisfied by text that never executes, and `# .devkit/hooks/pre-commit` inside the block would + * pass one while husky runs nothing. Byte-equality against the generator is the same standard + * `reviewHookDrift` already applies to the pre-commit block, and it needs no shell parsing at all. + * + * A block this devkit did not generate (an older release's wording, a hand edit) therefore reads as + * NOT wired. That is deliberate and fail-closed: devkit can only vouch for a block whose behaviour + * it knows. `installGlobalHook` is strip-then-reinsert, so re-running + * `devkit init --overlay --global-commit-gate` restores the exact block. */ export function globalHookWired() { const file = globalInitPath(); try { - if (!existsSync(file)) return false; - const content = readFileSync(file, 'utf8'); - const start = content.indexOf(MARK_START); - if (start === -1) return false; - const end = content.indexOf(MARK_END, start); - if (end === -1) return false; - return content.slice(start, end).includes(OVERLAY_HOOK_REL); + return existsSync(file) && readFileSync(file, 'utf8').includes(BLOCK); } catch { return false; } diff --git a/cli/lib/ship/review/setup/overlay-hooks-path.mts b/cli/lib/ship/review/setup/overlay-hooks-path.mts index 1e0c7129..5a0bbad1 100644 --- a/cli/lib/ship/review/setup/overlay-hooks-path.mts +++ b/cli/lib/ship/review/setup/overlay-hooks-path.mts @@ -92,20 +92,26 @@ export interface OverlayHooksPathContext { chainPresent: boolean; } +// `throwIfNoEntry: false` only silences ENOENT — an unreadable parent directory still throws +// EACCES, and a symlink cycle ELOOP. Both must read as "cannot prove this link is intact" and fall +// through to the caller's rejection message, not escape as an unhandled error from a predicate whose +// whole job is to return a diagnostic. function readIfFile(path: string): string | null { - const stat = lstatSync(path, { throwIfNoEntry: false }); - if (stat === undefined || !stat.isFile()) return null; try { - return readFileSync(path, 'utf8'); + const stat = lstatSync(path, { throwIfNoEntry: false }); + return stat?.isFile() ? readFileSync(path, 'utf8') : null; } catch { return null; } } function isExecutableFile(path: string): boolean { - const stat = lstatSync(path, { throwIfNoEntry: false }); - if (!stat?.isFile()) return false; - return (stat.mode & 0o111) !== 0; + try { + const stat = lstatSync(path, { throwIfNoEntry: false }); + return stat?.isFile() === true && (stat.mode & 0o111) !== 0; + } catch { + return false; + } } /** diff --git a/dist/cli/lib/overlay-global-hook.mjs b/dist/cli/lib/overlay-global-hook.mjs index 60dcd87d..94ed1178 100644 --- a/dist/cli/lib/overlay-global-hook.mjs +++ b/dist/cli/lib/overlay-global-hook.mjs @@ -21,6 +21,9 @@ import { dirname, join } from 'node:path'; const MARK_START = '# >>> devkit overlay global pre-commit gate >>>'; const MARK_END = '# <<< devkit overlay global pre-commit gate <<<'; const TRAILING_NEWLINES = /\n+$/; // hoisted (perf: never recompile per install/remove) +// The overlay hook the block invokes. Interpolated into BLOCK (never re-typed) so the string +// `globalHookWired` greps for cannot drift from the string the block actually runs. +const OVERLAY_HOOK_REL = '.devkit/hooks/pre-commit'; // The devkit block. Guarded so it ONLY acts in an overlaid repo and is otherwise inert: // - HUSKY=0 (husky's documented skip-hooks escape hatch) → skip. _/h's own HUSKY=0 exit is at // line 14, AFTER it sources init.sh (line 12), so the shim must self-check it here. @@ -36,8 +39,8 @@ const BLOCK = `${MARK_START} # HUSKY=0. Repo root resolved via git so worktrees / submodules / git -C still gate the right tree. if [ "\${HUSKY:-}" != "0" ] && [ "\${0##*/}" = "pre-commit" ]; then __dk_root=$(git rev-parse --show-toplevel 2>/dev/null) || __dk_root= - if [ -n "$__dk_root" ] && [ -x "$__dk_root/.devkit/hooks/pre-commit" ]; then - DEVKIT_VIA_HUSKY_INIT=1 sh "$__dk_root/.devkit/hooks/pre-commit" "$@" || exit $? + if [ -n "$__dk_root" ] && [ -x "$__dk_root/${OVERLAY_HOOK_REL}" ]; then + DEVKIT_VIA_HUSKY_INIT=1 sh "$__dk_root/${OVERLAY_HOOK_REL}" "$@" || exit $? fi unset __dk_root fi @@ -57,6 +60,32 @@ export function globalHookInstalled() { return false; } } +/** + * True iff the global init.sh carries this devkit's block VERBATIM. + * + * `globalHookInstalled` (MARK_START alone) stays doctor's advisory signal — a start marker is + * enough to say "you opted in". `devkit review` needs a stronger claim, because this shim is the + * ONLY thing keeping an overlay repo gated once husky reclaims core.hooksPath. + * + * Exact-match rather than grepping for the hook path between the markers: any substring test is + * satisfied by text that never executes, and `# .devkit/hooks/pre-commit` inside the block would + * pass one while husky runs nothing. Byte-equality against the generator is the same standard + * `reviewHookDrift` already applies to the pre-commit block, and it needs no shell parsing at all. + * + * A block this devkit did not generate (an older release's wording, a hand edit) therefore reads as + * NOT wired. That is deliberate and fail-closed: devkit can only vouch for a block whose behaviour + * it knows. `installGlobalHook` is strip-then-reinsert, so re-running + * `devkit init --overlay --global-commit-gate` restores the exact block. + */ +export function globalHookWired() { + const file = globalInitPath(); + try { + return existsSync(file) && readFileSync(file, 'utf8').includes(BLOCK); + } + catch { + return false; + } +} // Slice the devkit block (markers inclusive) out of `content`, collapsing the blank-line join that // preceded it (and one trailing newline). Returns the remainder (possibly ''). Never touches text // outside the markers, so a hand-written init.sh survives. diff --git a/dist/cli/lib/ship/review/setup/overlay-hooks-path.mjs b/dist/cli/lib/ship/review/setup/overlay-hooks-path.mjs index 42db627b..35cd4959 100644 --- a/dist/cli/lib/ship/review/setup/overlay-hooks-path.mjs +++ b/dist/cli/lib/ship/review/setup/overlay-hooks-path.mjs @@ -76,22 +76,27 @@ function runnerSourcesInit(script) { const assigned = HUSKY_INIT_ASSIGNMENT.exec(runner)?.[1]; return assigned !== undefined && sourceCommand(String.raw `\$\{?${assigned}\}?`).test(runner); } +// `throwIfNoEntry: false` only silences ENOENT — an unreadable parent directory still throws +// EACCES, and a symlink cycle ELOOP. Both must read as "cannot prove this link is intact" and fall +// through to the caller's rejection message, not escape as an unhandled error from a predicate whose +// whole job is to return a diagnostic. function readIfFile(path) { - const stat = lstatSync(path, { throwIfNoEntry: false }); - if (stat === undefined || !stat.isFile()) - return null; try { - return readFileSync(path, 'utf8'); + const stat = lstatSync(path, { throwIfNoEntry: false }); + return stat?.isFile() ? readFileSync(path, 'utf8') : null; } catch { return null; } } function isExecutableFile(path) { - const stat = lstatSync(path, { throwIfNoEntry: false }); - if (!stat?.isFile()) + try { + const stat = lstatSync(path, { throwIfNoEntry: false }); + return stat?.isFile() === true && (stat.mode & 0o111) !== 0; + } + catch { return false; - return (stat.mode & 0o111) !== 0; + } } /** * Why a husky-reclaimed core.hooksPath is NOT provably gated, or null when it is. The order walks diff --git a/eslint/baselines/size-lines.json b/eslint/baselines/size-lines.json index daf9c925..49c81c5b 100644 --- a/eslint/baselines/size-lines.json +++ b/eslint/baselines/size-lines.json @@ -2,7 +2,7 @@ "maxLines": 500, "maxTestLines": 0, "files": { - "cli/commands/doctor.mts": 426, + "cli/commands/doctor.mts": 422, "cli/commands/init.mts": 1280, "cli/lib/generate/generate-structure-baseline.mts": 876, "cli/lib/husky/husky-block.mts": 510,