fix: hooks reset no longer re-arms a disabled hook - #551
Conversation
DEFAULT_CONFIG.enabled now defaults to false, so a fresh checkout or a project whose config file was deleted no longer silently behaves as enabled. Only an explicit `hooks on` writes enabled:true. Every runHook()/runStopHook() test fixture that relied on the old true default now writes an explicit enabled config via a new mkEnabledTmp() test helper (or an existing raw config write, updated to carry enabled:true through a full overwrite). One test whose premise (a clean edit with zero footprint) is no longer constructible under the new default is removed as redundant with the adjacent opted-in-project test.
reset() now walks HOOK_MANIFEST_TARGETS and prunes impeccable's entries from every installed provider manifest (destRel/sharedDestRel), mirroring repairHookManifests()'s existing use of pruneImpeccableHookFromManifest(). Unlike the install path, the prune loop is not gated on the skill folder still existing, so a reset mid-uninstall still cleans up. Combined with the prior DEFAULT_CONFIG.enabled flip, a user who disables the hook and then resets ends with it disabled, not silently re-armed: fresh -> disabled, on -> installs, off -> disabled, reset -> removes config/consent/manifests, final status -> disabled. Full suite: 213/222 passing, matching the 9 pre-existing failures on main (Windows symlink/path-separator issues unrelated to this change).
Code review caught reset() pruning target.sharedDestRel alongside destRel -- but `hooks on`/repairHookManifests() only ever reads a shared/committed manifest (e.g. team .claude/settings.json) to check existing coverage, never writes it. Pruning it too meant a single developer's local `hooks reset` could strip the hook from a file the whole team shares. reset() now only prunes destRel, matching that write-scope asymmetry. Also: mergeHookConfig()'s hardcoded enabled-default (only reachable via setEnabled(), which immediately overwrites it) now matches the new DEFAULT_CONFIG.enabled:false semantics instead of the stale true-by-default one, closing a latent trap for any future caller. Tightened two reset() tests to assert the full composed status message (both the removed-config and pruned-manifest fragments, and the all-defaults fallback) instead of only half of it, and added a test proving the shared manifest survives reset untouched.
Two pre-existing/out-of-scope adversarial-review findings and two low- severity residual risks surfaced during ce-code-review on this branch, consciously not applied to keep this PR scoped to issue pbakaus#512.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryHook reset now restores configuration and leaves manifests unchanged when configuration persistence fails. It also exits with an error when a provider manifest cannot be pruned, while continuing cleanup for other providers. Skills installation enables opted-in hooks without overriding explicit shared or local opt-outs. Confidence Score: 5/5Safe to merge: the reset and installation lifecycle behaviors were exercised in isolated projects and produced the intended outcomes. No blocking failure remains.
 T-Rex LogsWhat T-Rex did
Reviews (5): Last reviewed commit: "fix: reset() reports failure when manife..." | Re-trigger Greptile |
…nabled-by-default-false semantics Cursor Bugbot review caught a real gap: `npx impeccable skills install` -- the CLI's own install path, separate from `/impeccable hooks on` -- writes manifests and records consent but never wrote `hook.enabled: true`. It worked only because it relied on the old `true` default; with that default now false, a CLI-installed project would have manifests wired up that never fire. decideHookInstall() now affirms `hook.enabled: true` via a new setHookEnabled() (mirrors setHookConsent()) on every path that decides the hook should be on, including pre-existing consent recorded before this fix existed. context.mjs's hookEnabledAt() had its own hardcoded `true` default, independent of hook-lib.mjs's DEFAULT_CONFIG. Left unfixed, this would have doubly masked the problem: MANUAL_DETECTOR_REQUIRED only fires when this function reports the hook inactive, so a stale default would skip the real hook AND suppress the fallback warning telling the agent to scan by hand. Flipped to match, with two existing tests updated to record explicit consent (a manifest alone is no longer sufficient to count as active). Greptile's review caught a second real gap in reset() itself: the pre-existing config-removal loop already swallowed fs errors silently, and reset() now prunes manifests regardless -- so a config write failure (permissions, disk full) could leave `hook.enabled: true` on disk while the manifest is already gone, with `status` reporting enabled even though nothing invokes the hook anymore. reset() now surfaces that failure and aborts before touching any manifest, exactly the ordering the config/manifest consistency this whole fix depends on. Tests: new coverage for the CLI install-enables-hook-in-config path (implicitly exercised via skills-cli.test.js's existing install/update suite, which passed against this change with zero new failures beyond the suite's pre-existing Windows HOME/symlink environment gaps -- verified identical 43/33/8 pass/fail/skip against unmodified origin/main), the two context.mjs fixture updates, and a new reset() test proving manifests survive an unwritable config.
|
Both automated reviews caught real gaps — thanks. Fixed in Cursor Bugbot — correct: Greptile — also correct: Full suite re-verified after both fixes: zero regressions (identical pre-existing Windows HOME/symlink environment failures on |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ee65ced. Configure here.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…manifest pruning Greptile's follow-up finding was real: the previous fix threw on a config persistence failure, but did so only *after* the loop over both config files, with no rollback. If config.json succeeded and config.local.json then failed, config.json was left reset (deleted or rewritten) while config.local.json -- which may still carry `hook.enabled: true` -- was untouched. That partial state is neither the old state nor the new one, and cache/manifest cleanup still never ran, so nothing about it was actually safe. reset() now backs up each config file's exact on-disk bytes immediately before mutating it. On any failure, every already-applied mutation in this call is rolled back before reset() throws, so the config pair is genuinely all-or-nothing: either both files reach the new state, or neither is touched. While auditing the rest of the function for the same class of gap (per instruction to fix this pattern for good, not just this one report): pruneImpeccableHookFromManifest()'s own writes are not internally try/caught, so a single provider's manifest failing to prune would previously crash reset() uncaught -- after config had already been fully reset -- with no report of which manifests did succeed. Each target is now try/caught independently and failures are reported rather than left to propagate. The cache/pending removal loop's previously-silent catch now reports failures too, for the same reason config's silent catch had to go -- even though cache/pending files are inert bookkeeping `status` never reports on, so their failure isn't fatal to the rest of reset(), it should still be visible rather than swallowed. New test proves the exact two-file rollback scenario Greptile reproduced: shared config succeeds, local config fails, shared config is restored to its exact prior content rather than left reset. Full suite re-verified: hook.test.mjs 216/225 pass (9 pre-existing Windows-environment failures, unchanged from baseline), context.test.mjs 96/98 pass (2 pre-existing, unchanged). Zero regressions.
|
Fixed in `c207e974` — Greptile's finding was real: my previous fix threw after the config-removal loop, with no rollback, so a shared-config-succeeds-then-local-config-fails sequence left the shared config reset while the local one (which may still carry `hook.enabled: true`) was untouched — a partial state that's neither the old one nor the new one. `reset()` now backs up each config file's exact bytes before mutating it and rolls back every already-applied mutation if a later step in the same call fails, so the config pair is genuinely all-or-nothing. While in there, audited the rest of the function for the same class of gap rather than patching just this one report: `pruneImpeccableHookFromManifest()`'s writes weren't try/caught in the new pruning loop (one provider failing would crash uncaught after config was already reset, with no report of what succeeded), and the cache/pending removal loop silently swallowed failures inconsistently with everything else now surfacing them. Both fixed. New test reproduces the exact two-file scenario Greptile's repro exercised and asserts the shared config is restored to its exact prior content. Full suite re-verified, zero regressions. |
Bugbot's finding on the previous commit was real and still open: decideHookInstall()'s enable() helper unconditionally force-wrote hook.enabled: true on every affirmative path, including when consent was already 'accepted' or the hook was already installed. 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` had consent:'accepted' alongside enabled:false. A routine `skills install`/`update` run silently flipped that back to true. New getHookEnabled() mirrors the runtime's own resolution order (shared config.json, then config.local.json overriding it) to read the currently effective value. enable() now only writes hook.enabled: true when it isn't already explicitly false, so install/update still wire up manifests but never re-arm a deliberate opt-out. A fresh install with no prior explicit state is unaffected -- it still correctly writes enabled: true, closing the original gap this whole line of fixes started from. Two new tests: an accepted-consent + explicit-off project survives skills install untouched, and a fresh install still affirms enabled. Full skills-cli.test.js suite: 45/86 pass (33 pre-existing Windows HOME/symlink environment failures, unchanged from the verified origin/main baseline), 2 new tests both pass.
|
Fixed in `8eb4b8c0` — this was a real, still-open finding from an earlier round I'd missed replying to: `decideHookInstall()`'s `enable()` helper force-wrote `hook.enabled: true` on every affirmative path, including when consent was already `accepted`. Since consent and enabled are tracked on separate paths (consent by this CLI install flow, `enabled` by `/impeccable hooks on|off`), a user who accepted the hook long ago and later ran `hooks off` would have that opt-out silently re-armed by a routine `skills install`/`update`. New `getHookEnabled()` reads the currently effective value (mirroring the runtime's own resolution order); `enable()` now only writes `true` when it isn't already explicitly `false`. Manifests still get wired up either way — only the enabled flag itself is now guarded. Two new tests cover both the guarded case and confirm a fresh install is unaffected. I did a full audit of the review threads on this PR and don't see any other open findings — flag if I've missed something. |
…nfig Greptile's third-round finding: manifest-pruning failures were caught and reported in the returned message, but reset() still returned normally (exit 0). A surviving manifest entry after permissions/disk issues means the exact artifact this fix exists to prune can still invoke a hook, and burying that in ordinary success-looking output gave callers no signal the reset was incomplete. Config failures were already fatal; manifest failures now are too, for the same reason -- this is not a partial success. Config still gets fully reset either way (fail-safe: config ends up disabled regardless of whether manifest cleanup also succeeds), matching Greptile's suggested fix of failing the status rather than rolling back what already succeeded. New test proves both halves: reset throws when a manifest can't be pruned, and config is still fully reset in that same run. Full suite: 314/324 pass (10 pre-existing Windows-environment failures, unchanged from baseline). Zero regressions.
|
Fixed in `45547f0b` — fair catch: manifest-pruning failures were caught and reported in the returned message, but `reset()` still exited 0. A surviving manifest entry after a permissions/disk failure is the exact artifact this whole fix exists to prune, so burying that in ordinary-looking output gave no signal the reset was incomplete. Manifest failures are now fatal too, same as config failures already were — config still ends up fully reset either way (fail-safe), but the command now correctly reports failure when manifest cleanup doesn't complete. New test proves both halves at once. |
github-actions
Bot
added
the
ready to merge
1 participant
Before opening
This repo is issue-first for outside contributions. I am not
pbakausorabdulwahabone, and there is no maintainer approval or request on the linked issue — opening this in good faith since the fix matches the reporter's own suggested fix exactly, with the understanding it may be closed without review per repo policy.hooks resetre-arms a previously disabled hook: it leaves the manifest behind whileDEFAULT_CONFIG.enabledistrue#512pbakausorabdulwahaboneWhat was broken
hooks offcorrectly disables the hook. Runninghooks resetafterward silently re-enabled it:reset()deleted the config file (including theenabled: falsethe user set) but never touched the hook manifests it wrote into.claude/settings.local.json,.codex/hooks.json,.cursor/hooks.json, or.github/hooks/impeccable.json. BecauseDEFAULT_CONFIG.enableddefaulted totrue, the next config read treated the missing key as "on," and the surviving manifest entries fired again.Repro from the issue:
The fix
Exactly the reporter's own two-part suggestion:
DEFAULT_CONFIG.enabled: false(skill/scripts/hook-lib.mjs). Absence of configuration now means "not consented" — onlyhooks onwritestrue, together with the consent record and the manifest entries.reset()prunes impeccable's entries from every installed manifest (skill/scripts/hook-admin.mjs), reusing the existingpruneImpeccableHookFromManifest()— the same functionrepairHookManifests()already calls, so no new stripping logic was written. Unlike the install path, the prune loop is not gated on the skill folder still existing, so a reset mid-uninstall (skill files already removed, manifest cleanup still pending) still works.One deliberate scope narrowing versus a first pass at this fix:
reset()only prunesdestRel(e.g..claude/settings.local.json), neversharedDestRel(e.g. a team-committed.claude/settings.json) — matching the write-scope asymmetryhooks on/repairHookManifests()already maintain (they only ever read the shared file to check existing coverage, never write it). A single developer's localresetshould not strip the hook from a file the whole team shares.With both changes, the issue's exact sequence now ends disabled:
Testing
tests/hook.test.mjs:reset()happy path, all-four-providers, no-manifests-installed (behavior unchanged), no-marker (manifest untouched), skill-folder-absent (mid-uninstall), shared-manifest-untouched, and the fullon/off/reset/statussequence reproducing the issue verbatim.DEFAULT_CONFIG.enabled: falsehas a wider blast radius than the line count suggests: roughly a dozenrunHook()/runStopHook()test fixtures relied on the oldtruedefault to reach real detection logic on a bare temp dir with no config file. Added amkEnabledTmp()helper and swapped it into every affected fixture; one test whose premise (a clean edit with literally zero.impeccable/footprint) became impossible to construct once enabling requires a config file was removed as redundant with the adjacent "opted-in project" test that already covers the same case correctly.main(Windows symlink-permission and path-separator issues unrelated to this change) — zero regressions introduced.ce-code-review(correctness, testing, adversarial, project-standards). Two findings applied: the shared-manifest write-scope fix above, and a latent (currently inert) default mismatch inmergeHookConfig(). Two lower-severity findings — a pre-existing substring-matching quirk in the manifest-stripping logic, and no file locking against concurrenthooksinvocations — are pre-existing/out-of-scope and recorded indocs/residual-review-findings/441bf5b0.mdrather than actioned here.AI-assisted change (Claude Code), following the reporter's own repro and suggested fix in #512.
Note
Medium Risk
Changes default hook behavior project-wide and mutates provider manifest files on reset; behavior is well-tested but affects consent semantics and local dev tooling.
Overview
Fixes #512:
hooks offfollowed byhooks resetno longer leaves the design hook effectively armed.Consent model —
DEFAULT_CONFIG.enabledis nowfalseinhook-lib.mjs, with the same default inhookEnabledAt()(context.mjs) andmergeHookConfig()(hook-admin.mjs). Missing config means disabled; only an explicithooks on(or equivalent) opts in. Tests usemkEnabledTmp()where hook execution is under test.hooks reset— After clearing hook/detector config and cache,reset()prunes Impeccable entries from each provider’s local manifest (destRelonly, not team-sharedsettings.json). Config edits are all-or-nothing with rollback on failure; manifest prune failures throw instead of reporting success. Sibling hook entries are preserved.CLI install path —
decideHookInstallwriteshook.enabled: trueto sharedconfig.jsonwhen install/update wires manifests (so they actually run under the new default), but does not overwrite an explicitenabled: falsefromhooks off.Extensive new/updated tests cover reset scenarios, the full
on→off→reset→statusrepro, and skills CLI enable guards.Reviewed by Cursor Bugbot for commit 45547f0. Bugbot is set up for automated code reviews on this repo. Configure here.