diff --git a/cli/bin/commands/skills.mjs b/cli/bin/commands/skills.mjs index c57c4d0a6..afde2cb91 100644 --- a/cli/bin/commands/skills.mjs +++ b/cli/bin/commands/skills.mjs @@ -17,7 +17,7 @@ import { get } from 'node:https'; import { createHash } from 'node:crypto'; import { tmpdir, homedir } from 'node:os'; import { unzipSync } from 'fflate'; -import { getHookConsent, setHookConsent } from '../../lib/impeccable-config.mjs'; +import { getHookConsent, setHookConsent, setHookEnabled, getHookEnabled } from '../../lib/impeccable-config.mjs'; const __dirname = dirname(fileURLToPath(import.meta.url)); const API_BASE = 'https://impeccable.style'; @@ -1658,24 +1658,38 @@ const HOOK_EXPLAINER = [ // first time, records the answer in .impeccable/config.local.json, and never // re-asks: a recorded decision or an already-installed hook short-circuits, and // non-interactive runs keep the historical install-by-default behavior. +// Every `true` return also affirms `hook.enabled: true` in the shared +// config.json (issue #512): the hook runtime's DEFAULT_CONFIG.enabled +// defaults to false, and this install path -- unlike `/impeccable hooks on` +// -- never otherwise writes that key. Without it, an install done here wires +// up manifests that never actually fire; idempotent to re-affirm on every run, +// including for consent recorded before this fix existed. +// The one thing it must never do is override an explicit `hooks off`: a +// routine `skills install`/`update` run is not the user opting back in, so +// `enable()` skips the write entirely when `enabled` is already explicitly +// false, leaving a deliberate opt-out exactly as the user left it. async function decideHookInstall(root, targets, { yes } = {}) { if (targets.length === 0) return false; + const enable = () => { + if (getHookEnabled(root) !== false) setHookEnabled(root, true); + return true; + }; const consent = getHookConsent(root); if (consent === 'declined') return false; - if (consent === 'accepted') return true; + if (consent === 'accepted') return enable(); // Existing hook users (hook already wired up) are never nagged. if (targets.length > 0 && targets.every(provider => hookInstalledForProvider(root, provider))) { - return true; + return enable(); } // Undecided and not yet installed. Non-interactive (-y or no TTY) keeps the // historical default-on behavior without recording a (re-promptable) decision. - if (yes || !process.stdin.isTTY) return true; + if (yes || !process.stdin.isTTY) return enable(); process.stdout.write(HOOK_EXPLAINER); const ans = await ask('Install the design hook? (Y/n) '); const accepted = !(ans === 'n' || ans === 'no'); setHookConsent(root, accepted ? 'accepted' : 'declined'); - return accepted; + return accepted ? enable() : false; } function resolveLinkSource(sourceValue, root) { diff --git a/cli/lib/impeccable-config.mjs b/cli/lib/impeccable-config.mjs index 827b26845..da7120a35 100644 --- a/cli/lib/impeccable-config.mjs +++ b/cli/lib/impeccable-config.mjs @@ -584,6 +584,44 @@ export function setHookConsent(root, value) { return filePath; } +/** + * The currently effective `hook.enabled` value, mirroring the runtime's own + * resolution order in hook-lib.mjs's readConfig() / context.mjs's + * hookEnabledAt() -- shared config.json first, then config.local.json + * overriding it when it also has an explicit key. Returns undefined when + * neither file has one (the runtime treats that as disabled per + * DEFAULT_CONFIG, issue #512). + */ +export function getHookEnabled(root) { + let enabled; + for (const filePath of [getConfigPath(root), getLocalConfigPath(root)]) { + const hook = hookSection(safeReadJson(filePath)); + if (hook && Object.prototype.hasOwnProperty.call(hook, 'enabled')) { + enabled = hook.enabled !== false; + } + } + return enabled; +} + +/** + * Persist `hook.enabled` to the shared config.json, preserving any sibling + * keys. Mirrors skill/scripts/hook-admin.mjs's setEnabled(cwd, true): the + * hook runtime's DEFAULT_CONFIG.enabled defaults to false (absence of + * consent must not mean "on"), so this CLI install path -- the only other + * place manifests get written -- must explicitly opt in, or an install done + * through `npx impeccable skills install` would wire up manifests that never + * actually fire. + */ +export function setHookEnabled(root, value) { + const filePath = getConfigPath(root); + const existing = safeReadJson(filePath) || {}; + const hook = hookSection(existing) || {}; + const next = { ...existing, hook: { ...hook, enabled: value } }; + mkdirSync(dirname(filePath), { recursive: true }); + writeFileSync(filePath, `${JSON.stringify(next, null, 2)}\n`); + return filePath; +} + const EXCLUDE_OPEN = '# impeccable-config-ignore-start'; const EXCLUDE_CLOSE = '# impeccable-config-ignore-end'; const EXCLUDE_PATTERNS = ['.impeccable/config.local.json']; diff --git a/docs/plans/2026-08-10-001-fix-hooks-reset-rearms-disabled-hook-plan.md b/docs/plans/2026-08-10-001-fix-hooks-reset-rearms-disabled-hook-plan.md new file mode 100644 index 000000000..ec5f8f648 --- /dev/null +++ b/docs/plans/2026-08-10-001-fix-hooks-reset-rearms-disabled-hook-plan.md @@ -0,0 +1,172 @@ +--- +artifact_contract: ce-unified-plan/v1 +artifact_readiness: implementation-ready +execution: code +product_contract_source: ce-plan-bootstrap +title: "fix: hooks reset no longer re-arms a disabled hook" +type: fix +created: 2026-08-10 +origin: "https://github.com/pbakaus/impeccable/issues/512" +--- + +# fix: hooks reset no longer re-arms a disabled hook + +**Target repo:** pbakaus/impeccable (working from fork `SomSamantray/impeccable`, branch `fix/512-hooks-reset-rearms-disabled-hook`) + +--- + +## Summary + +`hooks off` correctly disables Impeccable's PostToolUse/Stop hook. Running `hooks reset` afterward silently re-enables it: `reset()` deletes the config file (including the `enabled: false` the user set) but never touches the hook manifests it wrote into `.claude/settings.json`, `.codex/hooks.json`, `.cursor/hooks.json`, or `.github/hooks/impeccable.json`. Because `DEFAULT_CONFIG.enabled` defaults to `true`, the next config read treats the missing key as "on," and the surviving manifest entries fire again. A deliberate opt-out does not survive a reset — issue [#512](https://github.com/pbakaus/impeccable/issues/512). + +## Problem Frame + +- **R1: Reset must not re-enable a previously disabled hook.** After `hooks off` then `hooks reset`, the effective state must be disabled, not enabled. +- **R2: Reset must fully remove installed manifest entries.** `reset()` currently deletes config/cache/pending files but leaves every provider's manifest (`.claude/settings.json` or `.claude/settings.local.json`, `.codex/hooks.json`, `.cursor/hooks.json`, `.github/hooks/impeccable.json`) untouched, so the hook keeps running even though the user asked for a clean slate. +- **R3: Absence of any config must mean "not consented," not "on."** A fresh checkout with no consent record currently reports `enabled` (the milder symptom the issue also names) — `DEFAULT_CONFIG.enabled` should default to `false`, matching the model that only an explicit `hooks on` (with its consent record) turns the hook on. + +## Key Technical Decisions + +**KTD1: Flip `DEFAULT_CONFIG.enabled` to `false`.** In `skill/scripts/hook-lib.mjs`, `DEFAULT_CONFIG.enabled` currently defaults to `true`; only `hooks on` should produce an enabled state, and it already writes `enabled: true` explicitly (`setEnabled(cwd, true)` in `skill/scripts/hook-admin.mjs`). The source-of-truth field is only ever overridden when the raw config file actually has an `enabled` key: `applyConfigSource()` in `hook-lib.mjs` (invoked from `readConfig()`) does `if (Object.prototype.hasOwnProperty.call(raw, 'enabled')) { config.enabled = raw.enabled === false ? false : true; }`. So a fresh or reset repo (no `enabled` key on disk) now correctly falls through to the new `false` default, while every existing explicit `hooks on`/`hooks off` write is unaffected. + +**This default flip has a wider blast radius than the line-count suggests — see U1 below.** `runHook()`/`runStopHook()` (`hook-lib.mjs` ~line 1691-1694 and ~2053-2056) gate on `if (config.enabled === false) return result({ skipped: 'config-disabled', ... })`. `tests/hook.test.mjs` has roughly a dozen `describe` blocks whose `beforeEach` calls the shared `mkTmp()` helper (a bare `fs.mkdtempSync`, no config written) and then exercises `runHook()`/`runStopHook()` directly — those tests currently reach real scan/dedupe/render/cache logic only because the old `true` default carries them past the enabled-gate. Flipping the default without also fixing those fixtures would make every one of them short-circuit to `skipped: 'config-disabled'` and fail. U1's test-file changes are scoped to cover this, not just the `readConfig()` assertions. + +**KTD2: `reset()` prunes impeccable's entries from every installed manifest target, reusing the existing `repairHookManifests()` machinery.** `hook-admin.mjs` already has everything needed: `HOOK_MANIFEST_TARGETS` (the list of provider manifest paths), and `pruneImpeccableHookFromManifest(manifestPath)` (verified at `hook-admin.mjs` lines 512-545: returns `false` immediately if the file has no impeccable marker or fails to parse, otherwise strips impeccable's entries — deleting the file outright if nothing else remains, or rewriting it — and returns `true`; sibling entries and unrelated keys are preserved via `stripImpeccableHookEntries`, already covered by the existing "unrelated local hook fields survive" test pattern). `repairHookManifests()` calls `pruneImpeccableHookFromManifest()` today only as a side effect of re-installing (when a shared manifest already carries the marker). `reset()` needs the same prune applied unconditionally, for both `target.destRel` and `target.sharedDestRel` when present, regardless of whether the skill folder (`target.skillRel`) still exists — a user may be resetting during an uninstall, so gating on `skillRel` presence (as `repairHookManifests()` does before writing) would skip exactly the manifests that most need cleanup. + +**KTD3: `reset()`'s return message reports pruned manifests alongside removed config/cache files.** Mirrors the existing `setEnabled()` messaging convention (`Installed or repaired hook manifests for: ...`) so `reset` is not silently different in verbosity from `on`/`off`. + +## Scope Boundaries + +**In scope:** `DEFAULT_CONFIG.enabled` default; `reset()` manifest pruning; message text; new/updated tests in `tests/hook.test.mjs`. + +**Out of scope (not touched by this fix):** +- `repairHookManifests()`'s own logic and its `skillRel`-gated install path — unchanged. +- Any other `hook-admin.mjs` action (`on`, `off`, `ignore-rule`, `ignore-file`, `ignore-value`, `status`) beyond reading the new default. +- The two milder pre-existing behaviors the issue mentions only as context (fresh-checkout wording, consent-record semantics) beyond what KTD1 already fixes. + +--- + +## Implementation Units + +### U1. Default to disabled absent explicit consent + +**Goal:** `DEFAULT_CONFIG.enabled` becomes `false`, so any config state lacking an explicit `enabled` key (fresh checkout, or post-reset) reports and behaves as disabled. + +**Requirements:** R3 (KTD1) + +**Dependencies:** none + +**Files:** +- `skill/scripts/hook-lib.mjs` — change `DEFAULT_CONFIG.enabled: true` (line 151) to `false`. +- `tests/hook.test.mjs` — two categories of change: + 1. Update `readConfig()` tests that assert the bare/no-config default is `enabled === true` (around line 220, `describe('readConfig()')`) to assert `false` instead. + 2. Add a shared `mkEnabledTmp()` helper next to the existing `mkTmp()` (line 68) that creates the tmp dir and writes `.impeccable/config.json` with `{ hook: { enabled: true } }`. Swap `cwd = mkTmp()` for `cwd = mkEnabledTmp()` in the `beforeEach` of every describe block that exercises `runHook()`/`runStopHook()` without itself writing an explicit `enabled` value — confirmed at minimum: `runHook()` (~1295), `runHook() — cache write gating` (~1902), `runHook() — oversized files` (~1991), `runHook() — the session cache tracks the current scan` (~2105), `runHook() — clean-ack noise` (~2198), `runHook() — co-located stylesheet scan` (~2674), `runHook() — events without file_path` (~2839), `runHook() — configured template extensions` (~2860), `runHook() — emission enrichment` (~3340), `runHook() — per-edit tiering` (~3373), `runStopHook()` (~3494). Verify `expandScanTargets()` (~2602), `resolveProjectPlatform()/isNativePlatform()` (~2945), and `Cursor hook scripts` (~2970) against the same criterion — swap only if their tests call `runHook()`/`runStopHook()` on the bare fixture. + +**Approach:** +- The `DEFAULT_CONFIG.enabled` change itself is one line. `applyConfigSource()` (hook-lib.mjs, invoked from `readConfig()`) already only overrides `config.enabled` when the raw file has an own `enabled` property, so this change only affects the fallback path, not any explicit `true`/`false` write. +- The real scope is the test-fixture update: `runHook()`/`runStopHook()` (hook-lib.mjs ~1691-1694, ~2053-2056) short-circuit to `skipped: 'config-disabled'` when `config.enabled === false`. Every describe block listed above currently reaches real scan/dedupe/render/cache logic only because the old `true` default carried bare `mkTmp()` fixtures past that gate — without the `mkEnabledTmp()` swap, this unit breaks the bulk of the existing `runHook`/`runStopHook` suite rather than just the four `readConfig()` cases originally scoped. +- Do NOT change `mkTmp()` itself — the `hook-admin.mjs` describe block (~654) and the `readConfig()` describe block (~213) both rely on `mkTmp()` returning a truly bare, config-less directory to exercise the new disabled-by-default behavior; a global default would defeat that. + +**Patterns to follow:** existing `readConfig()` describe block (`tests/hook.test.mjs` ~line 213) already tests default vs. explicit-`true` vs. explicit-`false` cases; extend those, don't restructure them. The existing `writeFixture`/helper-function style at the top of each describe block (e.g. `runHook()`'s own local helpers ~1298-1341) is the pattern for adding `mkEnabledTmp()` as a shared top-of-file helper rather than a per-block one. + +**Test scenarios:** +- `readConfig()` on a cwd with no `.impeccable/config.json` at all returns `enabled: false` (was `true`). +- `readConfig()` with an explicit `{ hook: { enabled: true } }` still returns `enabled: true` (regression guard — explicit `on` must still work). +- `readConfig()` with an explicit `{ hook: { enabled: false } }` still returns `enabled: false`. +- `hooks status` on a completely fresh cwd (no config, no manifests) reports disabled, not enabled — covers the issue's "milder symptom." +- Every `runHook()`/`runStopHook()` describe block swapped to `mkEnabledTmp()` continues to pass unmodified otherwise — i.e., the swap alone (no other test-body change) restores prior behavior, confirming the fixtures were the only thing relying on the old default. + +**Verification:** existing `readConfig()` suite and a fresh-checkout `status` assertion pass with the new default; the full `tests/hook.test.mjs` suite passes after the `mkEnabledTmp()` swap, confirmed via full suite run in U3 rather than assumed from a partial read. + +--- + +### U2. `reset()` prunes impeccable entries from every installed manifest + +**Goal:** `hooks reset` removes impeccable's hook entries from `.claude/settings.json`/`.claude/settings.local.json`, `.codex/hooks.json`, `.cursor/hooks.json`, and `.github/hooks/impeccable.json` — whichever exist — in addition to the config/cache/pending cleanup it already does. + +**Requirements:** R1, R2 (KTD2, KTD3) + +**Dependencies:** none (independent of U1; both land in the same commit since they fix the same issue) + +**Files:** +- `skill/scripts/hook-admin.mjs` — extend `reset(cwd)` (currently ~line 742) to loop `HOOK_MANIFEST_TARGETS` and call `pruneImpeccableHookFromManifest()` for each target's `destRel` and `sharedDestRel` (when defined), collecting which providers were actually pruned; fold that into the returned message. +- `tests/hook.test.mjs` — add a `reset` test block near the existing `hook-admin.mjs` describe (~line 654) using the same fixture pattern as "hooks on accepts declined consent and installs missing provider manifests" (~line 917): create provider skill dirs, write manifest files containing impeccable's hook entries, then run `reset` and assert those entries are gone while sibling entries and unrelated keys survive. + +**Approach:** +- In `reset(cwd)`, after the existing config/cache/pending removal loops, add: for each `target` in `HOOK_MANIFEST_TARGETS`, for each of `[target.destRel, target.sharedDestRel].filter(Boolean)`, resolve the absolute path and call `pruneImpeccableHookFromManifest(absPath)`; track providers where it returned a truthy prune (per its existing return contract — see `pruneImpeccableHookFromManifest`'s current callers) in a `prunedManifests` list. +- Do **not** gate this loop on `fs.existsSync(path.join(cwd, target.skillRel))` the way `repairHookManifests()` gates its install path — `pruneImpeccableHookFromManifest()` already no-ops safely on a missing/markerless file (`if (!fileHasImpeccableHookMarker(manifestPath)) return false;`), and gating on the skill folder would skip cleanup during an uninstall, which is exactly when a full manifest prune matters most. +- Extend the returned message: when `prunedManifests.length > 0`, append `Removed hook entries from: .` to the existing removed-files sentence (mirrors the `setEnabled()` message-composition style at hook-admin.mjs ~line 376-390 — build a `parts` array and `join(' ')`, rather than a single template string, so this stays easy to extend). +- Reuse `pruneImpeccableHookFromManifest` and `HOOK_MANIFEST_TARGETS` exactly as they exist for `repairHookManifests()` — no signature changes to either. + +**Technical design** (directional, not literal code): +``` +function reset(cwd) { + const removed = [...existing config/cache/pending removal...] + const prunedManifests = [] + for (const target of HOOK_MANIFEST_TARGETS) { + let prunedThisTarget = false + for (const rel of [target.destRel, target.sharedDestRel].filter(Boolean)) { + if (pruneImpeccableHookFromManifest(path.join(cwd, rel))) prunedThisTarget = true + } + if (prunedThisTarget) prunedManifests.push(target.provider) + } + ...compose message including prunedManifests... +} +``` + +**Patterns to follow:** `repairHookManifests()`'s own `HOOK_MANIFEST_TARGETS` loop (hook-admin.mjs ~line 393-429) for target iteration; `setEnabled()`'s `parts.push(...)` message-composition style (~line 376-390) for the returned string; the "hooks on ... installs missing provider manifests" test (~line 917-957) for fixture shape (per-provider skill dirs, manifest files with pre-existing entries, asserting exact post-command manifest contents). + +**Test scenarios:** +- **Happy path:** fixture with `.claude` skill folder + `.claude/settings.local.json` containing an impeccable PostToolUse+Stop entry alongside an unrelated `OtherTool` entry (same shape as the existing "hooks on" test). Run `off` then `reset`. Assert: impeccable's entries are gone from the manifest, the unrelated `OtherTool` entry survives, and `hooks status` afterward reports disabled (not just "no config" — actually re-read via `readConfig`/`status` to close the loop end-to-end). +- **Multi-provider:** fixture with all four providers (`.claude`, `.agents`/`.codex`, `.cursor`, `.github`) installed via `on` first, then `reset`. Assert each of the four manifest files either has its impeccable entries stripped or (for providers whose manifest becomes empty) matches `pruneImpeccableHookFromManifest`'s existing empty-file behavior (delete `hooks`/`description`/`version` keys, or remove the file entirely if nothing remains — per current `pruneImpeccableHookFromManifest` behavior, don't re-specify it, just assert on it). +- **Sibling entries survive:** a manifest with both an impeccable entry and an unrelated tool's entry — after `reset`, only the unrelated entry remains (regression guard mirroring the existing "on" test's `local-hook.mjs` assertion). +- **Edge case — no manifests installed:** fixture with only a config file (no provider skill dirs, no manifest files) — `reset` behaves exactly as before (removes config/cache/pending, no crash, no spurious "Removed hook entries from" clause since `prunedManifests` stays empty). +- **Edge case — manifest present but has no impeccable marker:** a `.claude/settings.local.json` with only unrelated hooks — `reset` leaves it byte-for-byte unchanged (guards against `pruneImpeccableHookFromManifest` being invoked destructively on unrelated files). +- **Edge case — skill folder absent, manifest still present (the case KTD2's unconditional-loop decision exists for):** fixture with the impeccable-tagged manifest entries in `.claude/settings.local.json` but no `.claude/skills/impeccable` skill folder on disk (simulating mid-uninstall: skill files removed, manifest not yet cleaned). Assert `reset` still prunes the manifest — proving the loop is correctly *not* gated on `target.skillRel` existence the way `repairHookManifests()`'s install path is. +- **Integration — full revocation survives reset (covers the issue's own repro):** replay the issue's exact sequence — `on` → `off` → `reset` → `status` — and assert the final `status` output reports disabled, matching the issue's "Applied locally as a stopgap" expected final state. + +**Verification:** the multi-provider and full-revocation-sequence scenarios above directly reproduce and close the issue's own repro steps; existing "hooks on" test continues to pass unmodified (no shared code path was changed for `on`/`repairHookManifests`). + +--- + +### U3. Full suite verification + +**Goal:** confirm the two changes compose correctly and nothing else in the existing suite assumed the old `enabled: true` default. + +**Requirements:** R1, R2, R3 + +**Dependencies:** U1, U2 + +**Files:** none (verification only) + +**Approach:** +- Run the full test suite (`tests/hook.test.mjs` at minimum, plus whatever the repo's standard test command covers) and confirm no unrelated test broke on the `DEFAULT_CONFIG.enabled` flip — the `hooks on` test at line 917 and the "quiet flag survives on/off toggle" test at line 900 both write explicit `enabled`/`quiet` values, so they should be unaffected, but must be re-run to confirm. + +**Test expectation:** none beyond what U1/U2 already specify — this unit is a full-suite regression check, not new behavior. + +**Verification:** full test suite green; manual `hooks on` → `off` → `reset` → `status` sequence (in a scratch directory with provider skill folders present) matches the issue's expected final state: disabled, config/cache/pending removed, manifest entries removed. + +--- + +## Verification Contract + +- `readConfig()` / `status` on a config-less cwd reports `enabled: false`. +- `hooks on` still results in `enabled: true` and installed manifests (unchanged behavior — regression-guarded by the existing test at line 917). +- `hooks off` still results in `enabled: false` (unchanged — regression-guarded by the existing test at line 900). +- `hooks reset` after `hooks off`: manifests across all installed providers lose their impeccable entries; sibling/unrelated manifest entries and keys survive; `hooks status` afterward reports disabled. +- `hooks reset` on a cwd with no manifests installed: unchanged behavior (config/cache/pending removed, no error). + +## Definition of Done + +- [ ] U1: `DEFAULT_CONFIG.enabled` is `false`; `readConfig()` tests updated; every `runHook()`/`runStopHook()` fixture that relied on the old default is swapped to `mkEnabledTmp()` and still passes. +- [ ] U2: `reset()` prunes impeccable entries from all `HOOK_MANIFEST_TARGETS` manifests; new tests covering happy path, multi-provider, sibling-survival, no-manifest, no-marker, skillRel-absent, and full-revocation-sequence scenarios all pass. +- [ ] U3: full test suite green; manual repro of the issue's exact sequence confirms final state is disabled. + +## Review Notes (headless ce-doc-review, round 1) + +Three personas ran (coherence, feasibility, adversarial — `product-lens`/`design-lens`/`security-lens`/`scope-guardian` did not activate; greenfield plan with no upstream Product Contract source, `Origin: none`). All actionable findings (confidence ≥65) were applied directly above: +- **Feasibility (P0, confidence 100):** U1 as originally scoped would break ~13 existing `runHook()`/`runStopHook()` test blocks relying on the old `enabled: true` default. Verified against `hook-lib.mjs` and `tests/hook.test.mjs` directly — confirmed real. Fixed by expanding U1 to add `mkEnabledTmp()` and swap it into the affected fixtures. +- **Feasibility (P3, confidence 75):** KTD1 cited a nonexistent `mergeHookConfig` in `hook-lib.mjs`; the real hasOwnProperty-gated function is `applyConfigSource()`. Corrected the citation. +- **Coherence (P2, confidence 65):** U2's test scenarios never covered the skillRel-absent case that KTD2's unconditional-loop decision is specifically justified by. Added that scenario. +- **Adversarial (P1, confidence 75):** KTD2 asserted `pruneImpeccableHookFromManifest`'s return contract without citing it. Verified the function directly (lines 512-545) and cited its confirmed true/false behavior in KTD2. +- **Coherence (P3, confidence 50, FYI — not applied):** flagged the `.agents`/`.codex` notation in U2's multi-provider scenario as inconsistent-looking. Left as-is: it accurately reflects that the codex provider's `HOOK_MANIFEST_TARGETS` entry has a different `skillRel` (`.agents/skills/impeccable`) from its `destRel` (`.codex/hooks.json`), unlike the other three providers where both paths share a top-level directory. diff --git a/docs/residual-review-findings/441bf5b0.md b/docs/residual-review-findings/441bf5b0.md new file mode 100644 index 000000000..574a36a75 --- /dev/null +++ b/docs/residual-review-findings/441bf5b0.md @@ -0,0 +1,12 @@ +# Residual Review Findings + +Source: `ce-code-review` (correctness, testing, adversarial, project-standards) on branch `fix/512-hooks-reset-rearms-disabled-hook`, head `441bf5b0`. + +Applied findings are in the commit history (see `ee68b52a`, `c41bf797`, `441bf5b0`). The following were surfaced but consciously **not** applied in this PR — each is either pre-existing behavior this fix did not introduce, or disproportionate scope for a targeted bug fix. + +## Residual Review Findings + +- **P2 (adversarial, confidence 70, pre-existing)** — `skill/scripts/hook-admin.mjs:489` `stripImpeccableHookEntry` uses substring matching (`valueHasImpeccableHookMarker`) to decide whether a manifest hook entry belongs to Impeccable, and nulls the *entire* entry on a match. A hand-authored entry that chains Impeccable's hook with the user's own command in the same `command`/`args` string (e.g. `node "a.mjs" && node ".claude/skills/impeccable/scripts/hook.mjs"`) would have the user's portion silently deleted along with Impeccable's. This behavior predates this PR (both `repairHookManifests()`'s existing prune-on-reinstall path and the new `reset()` path share it) — not introduced here, and fixing it correctly (partial-string editing instead of whole-entry deletion) is a larger, separate change. +- **P2 (adversarial, confidence 60)** — `reset()`, `setEnabled()` (`on`/`off`) all perform multiple non-atomic file reads/writes across config, cache, and manifest files with no locking. Two concurrent `hooks` invocations on the same project (two agents, or an agent and a human) could interleave and leave config/manifest state inconsistent. This is a pre-existing architectural property of the whole `hook-admin.mjs` module (not unique to `reset()`), and introducing a lock file is out of scope for this targeted fix. +- **Advisory (adversarial residual risk)** — `fileHasImpeccableHookMarker` scans every string value under a manifest's `hooks` key for the marker substring, while `stripImpeccableHookEntry` only nulls entries whose `command`/`args`/`bash`/`powershell` fields match. A marker substring living in an unrelated field (e.g. a custom `statusMessage`) would make the prune function report `true` (rewrite the file, claim success) without actually removing anything. Low likelihood given how manifests are actually generated; noted for awareness, not actioned. +- **Advisory (adversarial residual risk)** — `pruneImpeccableHookFromManifest` silently no-ops on a manifest that fails `JSON.parse`, with no user-visible signal. If a provider's own hook loader is more lenient (tolerates trailing commas/comments) than this admin tool's strict parser, a manifest could stay live and un-prunable with no warning. Not actioned — matches this function's pre-existing error-handling posture everywhere else it's called. diff --git a/skill/scripts/context.mjs b/skill/scripts/context.mjs index 5c119022f..67fa34afd 100644 --- a/skill/scripts/context.mjs +++ b/skill/scripts/context.mjs @@ -1237,7 +1237,13 @@ function valueHasHookMarker(value) { function hookEnabledAt(root) { if (truthyEnv(process.env.IMPECCABLE_HOOK_DISABLED)) return false; - let enabled = true; + // Matches hook-lib.mjs's DEFAULT_CONFIG.enabled (issue #512): absence of an + // explicit `hook.enabled` key means disabled, not enabled. Getting this + // wrong here is doubly silent -- MANUAL_DETECTOR_REQUIRED (below) only + // fires when this function reports the hook inactive, so a stale `true` + // default would both skip the real hook (per hook-lib.mjs) AND suppress + // the fallback warning that would otherwise tell the agent to scan by hand. + let enabled = false; for (const name of ['.impeccable/config.json', '.impeccable/config.local.json']) { const raw = readJson(path.join(root, name)); if (raw?.hook && Object.prototype.hasOwnProperty.call(raw.hook, 'enabled')) { diff --git a/skill/scripts/hook-admin.mjs b/skill/scripts/hook-admin.mjs index e8d9e2ada..9262403f3 100644 --- a/skill/scripts/hook-admin.mjs +++ b/skill/scripts/hook-admin.mjs @@ -259,7 +259,11 @@ function writeDetectorConfig(cwd, detectorConfig, opts = {}) { function mergeHookConfig(existing) { const base = existing && typeof existing === 'object' ? existing : {}; return { - enabled: base.enabled === false ? false : true, + // Match DEFAULT_CONFIG.enabled (issue #512): absence of an explicit value + // means disabled, not enabled. setEnabled() overwrites this immediately + // for its own call site, but a future caller relying on this default + // alone must not silently re-arm. + enabled: base.enabled === true, limits: { maxFindings: Number.isFinite(base?.limits?.maxFindings) ? base.limits.maxFindings : DEFAULT_CONFIG.limits.maxFindings, maxChars: Number.isFinite(base?.limits?.maxChars) ? base.limits.maxChars : DEFAULT_CONFIG.limits.maxChars, @@ -739,35 +743,132 @@ function addIgnoreValue(cwd, args) { return `Added ${parsed.rule}=${parsed.value}${scopeSuffix} to ${scope} (${path.relative(cwd, target) || target}).`; } +// Best-effort restore of files reset() already mutated, used when a later +// step in the same reset() call fails. Never throws itself -- a restore +// failure (the underlying disk problem persisting) is reported as part of +// the original error, not swallowed, but must not mask it with a second +// exception. +function restoreConfigBackups(backups) { + const restoreFailures = []; + for (const { filePath, existed, content } of backups) { + try { + if (existed) { + fs.writeFileSync(filePath, content); + } else { + fs.unlinkSync(filePath); + } + } catch (err) { + restoreFailures.push(`${filePath} (${err.message || err})`); + } + } + return restoreFailures; +} + function reset(cwd) { const removed = []; + const configBackups = []; + const configFailures = []; // Unified files may hold non-hook keys (e.g. updateCheck); strip only the // hook/detector subtrees and keep the rest, deleting the file only if nothing remains. + // + // Config + local config reset as one all-or-nothing step: back up each + // file's exact on-disk bytes before mutating it, so that if the second + // file fails after the first already succeeded, the first is rolled back + // rather than left reset while the second (which may still carry + // `hook.enabled: true`) is untouched -- a partial reset that is neither + // the old state nor the new one, and that manifest pruning below must + // never run against. for (const filePath of [getConfigPath(cwd), getLocalConfigPath(cwd)]) { try { const raw = readRawConfigFile(filePath).raw; if (!raw || typeof raw !== 'object' || Array.isArray(raw) || (!('hook' in raw) && !('detector' in raw))) continue; const { hook, detector, ...rest } = raw; + const existed = fs.existsSync(filePath); + configBackups.push({ filePath, existed, content: existed ? fs.readFileSync(filePath) : null }); if (Object.keys(rest).length === 0) { fs.unlinkSync(filePath); } else { fs.writeFileSync(filePath, JSON.stringify(rest, null, 2) + '\n'); } removed.push(path.relative(cwd, filePath) || filePath); - } catch { /* ignore */ } + } catch (err) { + configFailures.push(`${path.relative(cwd, filePath) || filePath} (${err.message || err})`); + } } - // State files are wholly ours; delete outright. + if (configFailures.length) { + // A write/unlink failure here (permissions, disk full) must not be + // silently swallowed: this file may still carry `hook.enabled: true`, + // so pruning manifests below would leave `status` reporting enabled + // while nothing actually invokes the hook -- reset()'s own version of + // the exact config/manifest mismatch issue #512 was about. + const restoreFailures = restoreConfigBackups(configBackups); + const restoreNote = restoreFailures.length + ? ` Additionally could not restore: ${restoreFailures.join(', ')}.` + : ' Restored to the prior state.'; + throw new Error(`Could not reset hook config, so leaving manifests untouched to avoid reporting a stale enabled state: ${configFailures.join(', ')}.${restoreNote}`); + } + // State files are wholly ours; delete outright. A failure here is reported + // (never silently dropped, matching config/manifest handling above) but is + // not fatal to the rest of reset() -- unlike config, the cache/pending + // files are internal bookkeeping `status` never reports on, so a stale one + // is inert, not misleading. + const stateFailures = []; for (const filePath of [getCachePath(cwd), getPendingPath(cwd)]) { try { if (fs.existsSync(filePath)) { fs.unlinkSync(filePath); removed.push(path.relative(cwd, filePath) || filePath); } - } catch { /* ignore */ } + } catch (err) { + stateFailures.push(`${path.relative(cwd, filePath) || filePath} (${err.message || err})`); + } + } + // Manifests written by `hooks on` outlive config/cache removal (issue #512): + // with DEFAULT_CONFIG.enabled now false, a surviving manifest entry would + // still invoke a hook whose config no longer opts in. Prune every installed + // target regardless of whether its skill folder still exists on disk -- a + // reset mid-uninstall (skill files gone, manifest not yet cleaned) is + // exactly the case that most needs this, and pruneImpeccableHookFromManifest + // already no-ops safely on a missing or markerless file. + // + // destRel only, never sharedDestRel: `on`/repairHookManifests() only ever + // reads sharedDestRel (e.g. a team-committed .claude/settings.json) to + // decide whether it already covers the install -- it never writes there. + // Pruning sharedDestRel would make a single developer's local `reset` rip + // the hook out of a file the whole team shares, which is a materially + // larger blast radius than the local revocation this fix is about. + // + // pruneImpeccableHookFromManifest's own writes are not internally + // try/caught, so one target failing (permissions) must not abort the + // remaining targets or crash uncaught with no report of what did succeed. + const prunedManifests = []; + const manifestFailures = []; + for (const target of HOOK_MANIFEST_TARGETS) { + try { + if (pruneImpeccableHookFromManifest(path.join(cwd, target.destRel))) { + prunedManifests.push(target.provider); + } + } catch (err) { + manifestFailures.push(`${target.provider} (${err.message || err})`); + } } - return removed.length - ? `Reset design hook config and cache (removed: ${removed.join(', ')}).` - : 'No hook config or cache to remove. Already at defaults.'; + + const parts = []; + if (removed.length) parts.push(`Reset design hook config and cache (removed: ${removed.join(', ')}).`); + if (prunedManifests.length) parts.push(`Removed hook entries from: ${prunedManifests.join(', ')}.`); + if (stateFailures.length) parts.push(`Could not remove: ${stateFailures.join(', ')}.`); + if (manifestFailures.length) { + // A surviving manifest entry (permissions, disk full) means the exact + // artifact this fix set out to prune can still invoke a hook -- config + // has already been fully reset by this point, so an agent or human + // reading a bare success message would have no reason to expect that. + // Fatal for the same reason a config persistence failure is fatal above: + // this is not a partial success, it's an incomplete reset. + parts.push(`Could not prune manifests for: ${manifestFailures.join(', ')}.`); + throw new Error(parts.join(' ')); + } + if (!parts.length) return 'No hook config, cache, or manifest entries to remove. Already at defaults.'; + return parts.join(' '); } function main() { diff --git a/skill/scripts/hook-lib.mjs b/skill/scripts/hook-lib.mjs index 9170aa696..39ff28d91 100644 --- a/skill/scripts/hook-lib.mjs +++ b/skill/scripts/hook-lib.mjs @@ -148,7 +148,7 @@ export function isAdvisoryFinding(finding) { } export const DEFAULT_CONFIG = Object.freeze({ - enabled: true, + enabled: false, quiet: false, auditLog: null, designSystem: { enabled: true }, diff --git a/tests/context.test.mjs b/tests/context.test.mjs index e02934ed3..4514098f6 100644 --- a/tests/context.test.mjs +++ b/tests/context.test.mjs @@ -1066,10 +1066,15 @@ describe('context.mjs CLI', () => { const project = path.join(scratch, 'project'); fs.mkdirSync(path.join(project, '.codex'), { recursive: true }); + fs.mkdirSync(path.join(project, '.impeccable'), { recursive: true }); fs.writeFileSync(path.join(project, 'PRODUCT.md'), '# Acme\n'); fs.writeFileSync(path.join(project, '.codex', 'hooks.json'), JSON.stringify({ hooks: { Stop: [{ hooks: [{ command: 'node .agents/skills/impeccable/scripts/hook.mjs' }] }] }, })); + // hookEnabledAt() defaults to disabled absent explicit consent (#512); a + // manifest alone -- with no config recording that consent -- is no + // longer sufficient to count the hook as active. + fs.writeFileSync(path.join(project, '.impeccable', 'config.json'), JSON.stringify({ hook: { enabled: true } })); const res = spawnSync(process.execPath, [path.join(scripts, 'context.mjs')], { cwd: project, @@ -1097,10 +1102,13 @@ describe('context.mjs CLI', () => { const project = path.join(scratch, 'project'); fs.mkdirSync(path.join(project, '.cursor'), { recursive: true }); + fs.mkdirSync(path.join(project, '.impeccable'), { recursive: true }); fs.writeFileSync(path.join(project, 'PRODUCT.md'), '# Acme\n'); fs.writeFileSync(path.join(project, '.cursor', 'hooks.json'), JSON.stringify({ hooks: { preToolUse: [{ command: 'node .cursor/skills/impeccable/scripts/hook-before-edit.mjs' }] }, })); + // hookEnabledAt() defaults to disabled absent explicit consent (#512). + fs.writeFileSync(path.join(project, '.impeccable', 'config.json'), JSON.stringify({ hook: { enabled: true } })); const res = spawnSync(process.execPath, [path.join(scripts, 'context.mjs')], { cwd: project, diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index d4c9589db..945b55b1b 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -69,6 +69,17 @@ function mkTmp() { return fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-hook-')); } +// DEFAULT_CONFIG.enabled defaults to false (issue #512: absence of consent +// must not mean "on"). Suites that exercise runHook()/runStopHook() detection +// logic rather than the enabled/disabled gate itself need an explicit +// enabled config so a bare mkTmp() cwd doesn't short-circuit to 'config-disabled'. +function mkEnabledTmp() { + const cwd = mkTmp(); + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: true } })); + return cwd; +} + function fakeDetector(findings) { return { detectText: () => findings, @@ -216,8 +227,9 @@ describe('readConfig()', () => { afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); it('returns defaults when file missing', () => { + // Issue #512: absence of consent must not mean "on". const cfg = readConfig(cwd); - assert.equal(cfg.enabled, true); + assert.equal(cfg.enabled, false); assert.equal(cfg.limits.maxFindings, DEFAULT_CONFIG.limits.maxFindings); }); @@ -289,7 +301,7 @@ describe('readConfig()', () => { fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(getConfigPath(cwd), '{ not json'); const cfg = readConfig(cwd); - assert.equal(cfg.enabled, true); + assert.equal(cfg.enabled, false); }); it('ignores malformed local config while preserving valid shared config', () => { @@ -956,6 +968,210 @@ describe('hook-admin.mjs', () => { assert.match(github.hooks.postToolUse[0].bash, /\.github\/skills\/impeccable\/scripts\/hook\.mjs/); }); + it('reset prunes impeccable entries from an installed manifest, sibling entries survive', () => { + fs.mkdirSync(path.join(cwd, '.claude', 'skills', 'impeccable', 'scripts'), { recursive: true }); + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: false } })); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), JSON.stringify({ + description: 'Impeccable design detector', + hooks: { + PostToolUse: [ + { matcher: 'OtherTool', hooks: [{ type: 'command', command: 'node "./local-hook.mjs"' }] }, + { matcher: 'Edit|Write|MultiEdit', hooks: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }, + ], + Stop: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }], + }, + })); + + const out = runAdmin(['reset']); + assert.match(out, /Reset design hook config and cache \(removed:/); + assert.match(out, /Removed hook entries from: \.claude\./); + + const claude = JSON.parse(fs.readFileSync(path.join(cwd, '.claude', 'settings.local.json'), 'utf-8')); + assert.equal(claude.hooks.PostToolUse.length, 1); + assert.match(claude.hooks.PostToolUse[0].hooks[0].command, /local-hook\.mjs/); + assert.equal(claude.hooks.Stop, undefined, 'the impeccable-only Stop array should be dropped entirely'); + }); + + it('reset never touches the shared/committed manifest, only the local one', () => { + // .claude/settings.json is the team-shared, typically version-controlled + // file; `on` only ever reads it (to decide whether the team already + // covers the install) and never writes it. reset() must honor the same + // write-scope asymmetry -- a single developer's local reset must not + // strip the team's committed hook entries. + fs.mkdirSync(path.join(cwd, '.claude', 'skills', 'impeccable', 'scripts'), { recursive: true }); + const shared = JSON.stringify({ + hooks: { PostToolUse: [{ matcher: 'Edit', hooks: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }] }, + }); + fs.mkdirSync(path.join(cwd, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.json'), shared); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), shared); + + runAdmin(['reset']); + + assert.equal(fs.readFileSync(path.join(cwd, '.claude', 'settings.json'), 'utf-8'), shared, 'shared settings.json must survive reset untouched'); + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), false, 'the local settings.local.json is still pruned'); + }); + + it('reset prunes all four provider manifests installed via `on`', () => { + for (const provider of ['.claude', '.agents', '.cursor', '.github']) { + fs.mkdirSync(path.join(cwd, provider, 'skills', 'impeccable', 'scripts'), { recursive: true }); + } + runAdmin(['on']); + assert.match(fs.readFileSync(path.join(cwd, '.claude', 'settings.local.json'), 'utf-8'), /skills\/impeccable\/scripts\/hook\.mjs/); + + const out = runAdmin(['reset']); + assert.match(out, /Reset design hook config and cache \(removed:/); + assert.match(out, /Removed hook entries from: \.claude, \.agents, \.cursor, \.github/); + + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), false, 'nothing else was in the manifest, so it is removed entirely'); + assert.equal(fs.existsSync(path.join(cwd, '.codex', 'hooks.json')), false); + assert.equal(fs.existsSync(path.join(cwd, '.cursor', 'hooks.json')), false); + assert.equal(fs.existsSync(path.join(cwd, '.github', 'hooks', 'impeccable.json')), false); + }); + + it('reset with no manifests installed behaves exactly as before (config/cache only)', () => { + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: false } })); + + const out = runAdmin(['reset']); + assert.match(out, /Reset design hook config and cache/); + assert.doesNotMatch(out, /Removed hook entries from/); + assert.equal(fs.existsSync(getConfigPath(cwd)), false); + }); + + it('reset leaves a manifest with no impeccable marker byte-for-byte unchanged', () => { + fs.mkdirSync(path.join(cwd, '.claude'), { recursive: true }); + const unrelated = JSON.stringify({ + hooks: { PostToolUse: [{ matcher: 'OtherTool', hooks: [{ type: 'command', command: 'node "./unrelated.mjs"' }] }] }, + }); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), unrelated); + + const out = runAdmin(['reset']); + + assert.match(out, /Already at defaults/); + assert.equal(fs.readFileSync(path.join(cwd, '.claude', 'settings.local.json'), 'utf-8'), unrelated); + }); + + it('reset still prunes the manifest when the skill folder no longer exists (mid-uninstall)', () => { + // No .claude/skills/impeccable/ on disk -- simulates skill files already + // removed, manifest cleanup still pending. repairHookManifests()'s install + // path gates on the skill folder; reset()'s prune loop must not. + fs.mkdirSync(path.join(cwd, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), JSON.stringify({ + description: 'Impeccable design detector', + hooks: { + PostToolUse: [{ matcher: 'Edit|Write|MultiEdit', hooks: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }], + Stop: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }], + }, + })); + + const out = runAdmin(['reset']); + assert.match(out, /Removed hook entries from: \.claude\./); + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), false); + }); + + it('reset aborts before pruning manifests when config cannot be persisted', () => { + // A config rewrite failure (permissions, disk full) must not be silently + // swallowed while manifest pruning proceeds anyway -- that leaves `status` + // reporting enabled (the unwritten config) with no manifest left to fire + // it, reset()'s own version of the exact mismatch issue #512 fixed. + fs.mkdirSync(path.join(cwd, '.claude', 'skills', 'impeccable', 'scripts'), { recursive: true }); + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + const configPath = getConfigPath(cwd); + // A sibling key (updateCheck) forces the writeFileSync branch instead of + // unlink, so a read-only file actually exercises the write failure. + fs.writeFileSync(configPath, JSON.stringify({ updateCheck: true, hook: { enabled: true } })); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), JSON.stringify({ + hooks: { Stop: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }, + })); + fs.chmodSync(configPath, 0o444); + + try { + assert.throws(() => runAdmin(['reset']), /Could not reset hook config/); + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), true, 'manifest must survive when config reset failed'); + const stillThere = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + assert.equal(stillThere.hook.enabled, true, 'the unwritable config is unchanged, still reporting enabled'); + } finally { + fs.chmodSync(configPath, 0o644); + } + }); + + it('reset throws when a manifest cannot be pruned, even though config was already reset', () => { + // Greptile's second follow-up finding: config reset and manifest pruning + // are separate steps, and only config failures were fatal. A manifest + // surviving (permissions) after config was already cleared is still an + // incomplete reset -- the manifest is the exact artifact this whole fix + // exists to remove -- so it must exit non-zero too, not report success. + fs.mkdirSync(path.join(cwd, '.claude', 'skills', 'impeccable', 'scripts'), { recursive: true }); + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: false } })); + const manifestPath = path.join(cwd, '.claude', 'settings.local.json'); + fs.writeFileSync(manifestPath, JSON.stringify({ + hooks: { + PostToolUse: [ + { matcher: 'OtherTool', hooks: [{ type: 'command', command: 'node "./local-hook.mjs"' }] }, + { matcher: 'Edit', hooks: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }, + ], + }, + })); + fs.chmodSync(manifestPath, 0o444); + + try { + assert.throws(() => runAdmin(['reset']), /Could not prune manifests for: \.claude/); + // Fail-safe: config is disabled either way, even though the manifest + // pruning failure is what makes the overall command exit non-zero. + assert.equal(fs.existsSync(getConfigPath(cwd)), false, 'config was still fully reset even though manifest pruning failed'); + } finally { + fs.chmodSync(manifestPath, 0o644); + } + }); + + it('reset rolls back the shared config when the local config fails after it (partial-reset guard)', () => { + // Greptile's follow-up finding: shared config.json succeeds, then + // config.local.json fails -- reset() must not leave the shared file + // reset while the local one (which may still carry hook.enabled: true) + // is untouched. That combination is neither the old state nor the new + // one, and cache/manifest cleanup must not run against it either. + fs.mkdirSync(path.join(cwd, '.claude', 'skills', 'impeccable', 'scripts'), { recursive: true }); + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + const sharedPath = getConfigPath(cwd); + const localPath = getLocalConfigPath(cwd); + const sharedOriginal = JSON.stringify({ updateCheck: true, hook: { enabled: true } }); + const localOriginal = JSON.stringify({ pinnedVersion: '1.0.0', hook: { consent: 'accepted' } }); + fs.writeFileSync(sharedPath, sharedOriginal); + fs.writeFileSync(localPath, localOriginal); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), JSON.stringify({ + hooks: { Stop: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }, + })); + fs.chmodSync(localPath, 0o444); + + try { + assert.throws(() => runAdmin(['reset']), /Could not reset hook config/); + assert.equal(fs.readFileSync(sharedPath, 'utf-8'), sharedOriginal, 'shared config must be rolled back to its exact prior content, not left reset'); + assert.equal(fs.readFileSync(localPath, 'utf-8'), localOriginal, 'local config is unchanged (its write never succeeded)'); + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), true, 'manifest must survive a rolled-back reset'); + } finally { + fs.chmodSync(localPath, 0o644); + } + }); + + it('full revocation survives reset -- on, off, reset, status ends disabled (issue #512 repro)', () => { + fs.mkdirSync(path.join(cwd, '.claude', 'skills', 'impeccable', 'scripts'), { recursive: true }); + runAdmin(['on']); + runAdmin(['off']); + + const beforeReset = runAdmin(['status']); + assert.match(beforeReset, /state:\s+disabled/); + + runAdmin(['reset']); + + assert.equal(fs.existsSync(getConfigPath(cwd)), false); + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), false); + const afterReset = runAdmin(['status']); + assert.match(afterReset, /state:\s+disabled/); + }); + it('ignore-rule overused-font requires explicit broad suppression', () => { assert.throws( () => runAdmin(['ignore-rule', 'overused-font']), @@ -1060,6 +1276,7 @@ describe('hook-admin.mjs', () => { fs.mkdirSync(path.dirname(getConfigPath(cwd)), { recursive: true }); fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ + hook: { enabled: true }, detector: { extensions: [{ ext: '.blade.php', engine: 'html' }] }, })); @@ -1292,7 +1509,7 @@ describe('payload()', () => { describe('runHook()', () => { let cwd; - beforeEach(() => { cwd = mkTmp(); }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); function eventFor(file, sessionId = 'sid-1') { @@ -1503,7 +1720,7 @@ rounded: it('config quiet:true suppresses the clean ack like the env switch', async () => { fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); - fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { quiet: true } })); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: true, quiet: true } })); const file = writeFixture('src/Quiet.tsx', 'noop'); const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), @@ -1616,6 +1833,7 @@ rounded: writeDesignMd(); fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ + hook: { enabled: true }, detector: { designSystem: { enabled: false } }, })); const file = writeFixture('src/Card.tsx', '.card { font-family: "Poppins", sans-serif; }'); @@ -1635,6 +1853,7 @@ rounded: writeDesignMd(); fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ + hook: { enabled: true }, detector: { ignoreValues: [ { rule: 'design-system-font', value: 'Poppins' }, @@ -1714,7 +1933,7 @@ rounded: const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd, detector: det }); assert.equal(r.stdout, ''); assert.equal(r.audit.skipped, 'outside-project'); - assert.ok(!fs.existsSync(path.join(cwd, '.impeccable')), 'out-of-project edit must not dirty the cache'); + assert.ok(!fs.existsSync(path.join(cwd, '.impeccable', 'hook.cache.json')), 'out-of-project edit must not dirty the cache'); } finally { fs.rmSync(scratch, { recursive: true, force: true }); } @@ -1753,6 +1972,7 @@ rounded: const file = writeFixture('src/legacy/Foo.tsx', 'noop'); fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ + hook: { enabled: true }, detector: { ignoreFiles: ['src/legacy/**'] }, })); const det = fakeDetector([finding('side-tab', 1)]); @@ -1838,7 +2058,7 @@ rounded: // The fixture's finding (side-tab) sits in the deferred tier, so restore // the full per-edit rule set for this test via the config override. fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); - fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { perEditRules: 'all' } })); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: true, perEditRules: 'all' } })); const file = writeFixture('index.html', [ '', '', @@ -1862,7 +2082,7 @@ rounded: // overused-font is deferred-tier; use the perEditRules override so the // per-edit pass surfaces it here. fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); - fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { perEditRules: 'all' } })); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: true, perEditRules: 'all' } })); const flagged = writeFixture('src/Flagged.tsx', 'const css = "font-family: Inter";'); const flaggedRun = await runHook({ stdinJson: JSON.stringify(eventFor(flagged)), env: {}, cwd, detector: { detectHtml, detectText }, @@ -1894,12 +2114,14 @@ rounded: }); describe('runHook() — cache write gating (issues #344, #305)', () => { - // The hook must be a no-op on disk in projects that never earned an - // `.impeccable/` footprint: skipped files never dirty the cache, and a - // dirty cache is only persisted when there are fresh findings or the - // project already opted in (an `.impeccable/` dir exists). + // The hook must not persist a cache footprint for skipped files: fresh + // findings are the only thing that earns a `hook.cache.json` write. cwd + // uses mkEnabledTmp() (issue #512: enabled defaults to false, so a config + // file consenting to the hook is now a precondition, not itself evidence + // of a footprint) — assertions below check the cache file specifically, + // not the whole `.impeccable/` directory. let cwd; - beforeEach(() => { cwd = mkTmp(); }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); function eventFor(file, sessionId = 'gate-sid') { @@ -1926,27 +2148,17 @@ describe('runHook() — cache write gating (issues #344, #305)', () => { env: {}, cwd, detector: fakeDetector([finding('side-tab', 1)]), }); assert.equal(r.audit.skipped, 'extension'); - assert.ok(!fs.existsSync(path.join(cwd, '.impeccable')), '.impeccable should not exist'); + assert.ok(!fs.existsSync(path.join(cwd, '.impeccable', 'hook.cache.json')), 'no cache should be written'); }); - it('clean UI edit in a project with no footprint does not create .impeccable/, still acks', async () => { - const file = write('src/Card.tsx', 'noop'); - const r = await runHook({ - stdinJson: JSON.stringify(eventFor(file)), - env: {}, cwd, detector: fakeDetector([]), - }); - assert.match(r.stdout, /No deterministic design-quality issues found/); - assert.ok(!fs.existsSync(path.join(cwd, '.impeccable')), '.impeccable should not exist'); - }); - - it('detector-missing path does not create .impeccable/', async () => { + it('detector-missing path does not create a cache footprint', async () => { const file = write('src/Card.tsx', 'noop'); const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd, detector: {}, }); assert.equal(r.audit.skipped, 'detector-missing'); - assert.ok(!fs.existsSync(path.join(cwd, '.impeccable')), '.impeccable should not exist'); + assert.ok(!fs.existsSync(path.join(cwd, '.impeccable', 'hook.cache.json')), 'no cache should be written'); }); it('fresh findings create the cache, and dedup works on the next run', async () => { @@ -1972,9 +2184,17 @@ describe('runHook() — cache write gating (issues #344, #305)', () => { it('umbrella launch keys the cache to the edited file\'s project root', async () => { // cwd is the umbrella: no .git / package.json / .impeccable of its own. + // `.impeccable/` is itself a project-root marker (resolveCacheCwd), so the + // shared beforeEach's mkEnabledTmp() config would wrongly make the umbrella + // look like a project root and short-circuit the climb to `child`. Undo it + // here and put the enabling config where the hook will actually look: + // the child project resolveCacheCwd climbs to. + fs.rmSync(path.join(cwd, '.impeccable'), { recursive: true, force: true }); write('app/package.json', '{"name":"child"}'); const file = write('app/src/Card.tsx', 'noop'); const child = path.join(cwd, 'app'); + fs.mkdirSync(path.join(child, '.impeccable'), { recursive: true }); + fs.writeFileSync(path.join(child, '.impeccable', 'config.json'), JSON.stringify({ hook: { enabled: true } })); const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd, detector: fakeDetector([finding('text-overflow', 1)]), @@ -1988,10 +2208,7 @@ describe('runHook() — cache write gating (issues #344, #305)', () => { describe('runHook() — oversized files', () => { let cwd; - beforeEach(() => { - cwd = mkTmp(); - fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); - }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); const event = (file) => JSON.stringify({ @@ -2088,7 +2305,7 @@ describe('runHook() — oversized files', () => { it('honors a configured limits.maxFileBytes', async () => { fs.writeFileSync(path.join(cwd, '.impeccable', 'config.json'), JSON.stringify({ - hook: { limits: { maxFileBytes: 1024 } }, + hook: { enabled: true, limits: { maxFileBytes: 1024 } }, })); const file = path.join(cwd, 'small.css'); fs.writeFileSync(file, `/* ${'x'.repeat(4096)} */`); @@ -2102,10 +2319,7 @@ describe('runHook() — oversized files', () => { describe('runHook() — the session cache tracks the current scan', () => { let cwd; - beforeEach(() => { - cwd = mkTmp(); - fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); - }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); function eventFor(file, sessionId = 'sid-1') { @@ -2195,10 +2409,7 @@ describe('runHook() — the session cache tracks the current scan', () => { describe('runHook() — clean-ack noise', () => { let cwd; - beforeEach(() => { - cwd = mkTmp(); - fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); - }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); const event = (file, sessionId = 'sid-1') => JSON.stringify({ @@ -2671,7 +2882,7 @@ describe('expandScanTargets()', () => { describe('runHook() — co-located stylesheet scan', () => { let cwd; - beforeEach(() => { cwd = mkTmp(); }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); function write(rel, body) { @@ -2857,7 +3068,7 @@ describe('runHook() — events without file_path', () => { describe('runHook() — configured template extensions (issue #316)', () => { let cwd; - beforeEach(() => { cwd = mkTmp(); }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); function eventFor(file) { @@ -2879,7 +3090,9 @@ describe('runHook() — configured template extensions (issue #316)', () => { function writeExtensionsConfig(extensions) { fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); - fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ detector: { extensions } })); + // Full overwrite of the config mkEnabledTmp() wrote — carry `hook.enabled` + // forward so this doesn't fall back to the new disabled-by-default (#512). + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: true }, detector: { extensions } })); } function recordingDetector(findings = []) { @@ -2967,7 +3180,7 @@ describe('resolveProjectPlatform() / isNativePlatform()', () => { describe('Cursor hook scripts', () => { let cwd; - beforeEach(() => { cwd = mkTmp(); }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); it('preToolUse denies proposed writes with detector findings before they land', () => { @@ -3078,6 +3291,7 @@ describe('Cursor hook scripts', () => { // With a detector.extensions entry the same proposed write is scanned and denied. fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(path.join(cwd, '.impeccable', 'config.json'), JSON.stringify({ + hook: { enabled: true }, detector: { extensions: [{ ext: '.blade.php' }] }, })); const payload = JSON.parse(run()); @@ -3093,6 +3307,7 @@ describe('Cursor hook scripts', () => { const filePath = path.join(cwd, 'resources/views/hero.blade.php'); fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(path.join(cwd, '.impeccable', 'config.json'), JSON.stringify({ + hook: { enabled: true }, detector: { extensions: [{ ext: '.blade.php' }] }, })); @@ -3337,7 +3552,7 @@ describe('Cursor hook scripts', () => { describe('runHook() — emission enrichment', () => { let cwd; - beforeEach(() => { cwd = mkTmp(); }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); function write(rel, content) { @@ -3370,7 +3585,7 @@ describe('runHook() — per-edit tiering', () => { // The per-edit pass surfaces only IMMEDIATE_TIER_RULES; everything else is // deferred to the Stop deep pass. See hook-lib.mjs for the tier rationale. let cwd; - beforeEach(() => { cwd = mkTmp(); }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); function eventFor(file, sessionId = 'tier-sid') { @@ -3442,7 +3657,7 @@ describe('runHook() — per-edit tiering', () => { it('config hook.perEditRules:"all" restores the full per-edit rule set', async () => { fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); - fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { perEditRules: 'all' } })); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: true, perEditRules: 'all' } })); const file = write('src/Card.tsx', 'noop'); const det = fakeDetector([finding('marketing-buzzword', 2)]); const r = await runHook({ stdinJson: JSON.stringify(eventFor(file, 'tier-all')), env: {}, cwd, detector: det }); @@ -3478,7 +3693,7 @@ describe('runHook() — per-edit tiering', () => { it('includes advisory findings per edit when detector.advisoryRules is "include"', async () => { fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ - hook: { perEditRules: 'all' }, + hook: { enabled: true, perEditRules: 'all' }, detector: { advisoryRules: 'include' }, })); const file = write('src/Copy.tsx', 'noop'); @@ -3491,7 +3706,7 @@ describe('runHook() — per-edit tiering', () => { describe('runStopHook()', () => { let cwd; - beforeEach(() => { cwd = mkTmp(); }); + beforeEach(() => { cwd = mkEnabledTmp(); }); afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); function write(rel, body) { @@ -3594,6 +3809,7 @@ describe('runStopHook()', () => { const sid = 'stop-ignored'; fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ + hook: { enabled: true }, detector: { ignoreRules: ['marketing-buzzword'] }, })); const file = write('src/Card.tsx', 'noop'); @@ -3622,6 +3838,7 @@ describe('runStopHook()', () => { const sid = 'stop-advisory-include'; fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ + hook: { enabled: true }, detector: { advisoryRules: 'include' }, })); const file = write('src/Card.tsx', 'noop'); diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js index 427f33e25..9dea83529 100644 --- a/tests/skills-cli.test.js +++ b/tests/skills-cli.test.js @@ -1384,6 +1384,39 @@ describe('skills install/update: local universal bundle e2e', () => { rmSync(tmp, { recursive: true, force: true }); }, 15000); + test('does not re-enable a hook the user explicitly turned off (#512 follow-up)', async () => { + // consent and enabled are tracked on separate paths (consent by this CLI + // install flow, enabled by `/impeccable hooks on|off`), so a user who + // accepted the hook long ago and later ran `hooks off` has + // consent:'accepted' alongside enabled:false. A routine `skills + // install`/`update` run is not the user opting back in. + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-enable-guard-')); + execSync('git init', { cwd: tmp }); + mkdirSync(join(tmp, '.impeccable'), { recursive: true }); + writeFileSync(join(tmp, '.impeccable', 'config.json'), JSON.stringify({ hook: { consent: 'accepted', enabled: false } })); + + const wantHooks = await decideHookInstall(tmp, ['claude'], { yes: true }); + + expect(wantHooks).toBe(true); // manifests still get wired up / updated + const config = JSON.parse(readFileSync(join(tmp, '.impeccable', 'config.json'), 'utf8')); + expect(config.hook.enabled).toBe(false); // but the explicit opt-out survives + + rmSync(tmp, { recursive: true, force: true }); + }, 15000); + + test('affirms hook.enabled:true on a fresh install with no prior explicit state', async () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-enable-fresh-')); + execSync('git init', { cwd: tmp }); + + const wantHooks = await decideHookInstall(tmp, ['claude'], { yes: true }); + + expect(wantHooks).toBe(true); + const config = JSON.parse(readFileSync(join(tmp, '.impeccable', 'config.json'), 'utf8')); + expect(config.hook.enabled).toBe(true); + + rmSync(tmp, { recursive: true, force: true }); + }, 15000); + test('--no-hooks installs skills without hook manifests', () => { const tmp = mkdtempSync(join(tmpdir(), 'imp-test-local-no-hooks-')); execSync('git init', { cwd: tmp });