fix(cli): clean up hooks from Claude settings on package postuninstal… - #629
fix(cli): clean up hooks from Claude settings on package postuninstal…#629AVPthegreat wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a post-uninstall lifecycle script that removes failproofai hooks from Claude settings files, handles cleanup errors without throwing, and adds Vitest coverage for missing files and successful hook removal. ChangesUninstall hook cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
__tests__/scripts/postuninstall.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. package.jsonESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. scripts/postuninstall.mjsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__tests__/scripts/postuninstall.test.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlace this unit test under
__tests__/hooks/.As per coding guidelines, “Unit tests live in
__tests__/hooks/.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/scripts/postuninstall.test.ts` at line 1, Move the unit test currently located under the scripts test area into the __tests__/hooks/ directory, preserving its existing test behavior and contents.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/postuninstall.mjs`:
- Around line 11-15: The isFailproofaiHook classifier is too broad and can
remove unrelated hooks containing similar text. In scripts/postuninstall.mjs
lines 11-15, match only the explicit FailproofAI marker properties or the exact
installer-generated command form; in __tests__/scripts/postuninstall.test.ts
lines 37-66, add coverage proving a non-FailproofAI hook with similar command
text is preserved.
---
Nitpick comments:
In `@__tests__/scripts/postuninstall.test.ts`:
- Line 1: Move the unit test currently located under the scripts test area into
the __tests__/hooks/ directory, preserving its existing test behavior and
contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf6178aa-c7a7-407c-987e-48bf09f7287d
📒 Files selected for processing (3)
__tests__/scripts/postuninstall.test.tspackage.jsonscripts/postuninstall.mjs
| function isFailproofaiHook(hook) { | ||
| if (!hook || typeof hook !== "object") return false; | ||
| if (hook.is_failproofai === true || hook.__failproofai_hook === true) return true; | ||
| const cmd = typeof hook.command === "string" ? hook.command : ""; | ||
| return cmd.includes("failproofai") && cmd.includes("--hook"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching postuninstall:\n'
fd -a 'postuninstall|postinstall' . || true
printf '\nRelevant file outlines:\n'
for f in $(fd 'postuninstall|mjs$|test\.ts$' . | sed 's#^\./##' | head -50); do
if grep -qE 'isFailproofaiHook|cleanClaudeSettings|hook' "$f" 2>/dev/null; then
printf '\n--- %s\n' "$f"
wc -l "$f"
ast-grep outline "$f" --match cleanClaudeSettings || true
fi
done
printf '\nSearch hook classifier usages and test hook fixtures:\n'
rg -n "isFailproofaiHook|failproofai|__failproofai_hook|cleanClaudeSettings|hooks" scripts __tests__ package.json 2>/dev/null || true
printf '\nShow relevant source/tests:\n'
if [ -f scripts/postuninstall.mjs ]; then
cat -n scripts/postuninstall.mjs
fi
if [ -f __tests__/scripts/postuninstall.test.ts ]; then
sed -n '1,120p' __tests__/scripts/postuninstall.test.ts | cat -n
fiRepository: FailproofAI/failproofai
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'scripts/postuninstall.mjs:\n'
cat -n scripts/postuninstall.mjs
printf '\n__tests__/scripts/postuninstall.test.ts first 140 lines:\n'
sed -n '1,140p' __tests__/scripts/postuninstall.test.ts | cat -n
printf '\nBehavioral probe for isFailproofaiHook classifying fixtures:\n'
node - <<'JS'
function isFailproofaiHook(hook) {
if (!hook || typeof hook !== "object") return false;
if (hook.is_failproofai === true || hook.__failproofai_hook === true) return true;
const cmd = typeof hook.command === "string" ? hook.command : "";
return cmd.includes("failproofai") && cmd.includes("--hook");
}
const hooks = [
{ command: "failproofai --hook PreToolUse" },
{ is_failproofai: true, command: "failproofai --hook PreToolUse" },
{ __failproofai_hook: true, command: "failproofai --hook PreToolUse" },
{ command: "npx -y failproofai --hook PreToolUse" },
{ command: "npx -y failproofai policies --install block-rm-rf" },
{ command: "my-failproofai-helper --hook PreToolUse" },
{ name: "failproofai", type: "task", args: [] },
];
for (const hook of hooks) {
console.log(JSON.stringify(hook), '=>', isFailproofaiHook(hook));
}
JSRepository: FailproofAI/failproofai
Length of output: 6150
Use a precise FailproofAI hook identity contract. The substring classifier will clean hooks just because they contain failproofai and --hook, e.g. my-failproofai-helper --hook PreToolUse. Match only explicit FailproofAI markers or the exact installer-generated command form, and add a test case that asserts non-FailproofAI hooks with similar text are preserved.
📍 Affects 2 files
scripts/postuninstall.mjs#L11-L15(this comment)__tests__/scripts/postuninstall.test.ts#L37-L66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/postuninstall.mjs` around lines 11 - 15, The isFailproofaiHook
classifier is too broad and can remove unrelated hooks containing similar text.
In scripts/postuninstall.mjs lines 11-15, match only the explicit FailproofAI
marker properties or the exact installer-generated command form; in
__tests__/scripts/postuninstall.test.ts lines 37-66, add coverage proving a
non-FailproofAI hook with similar command text is preserved.
Source: Coding guidelines
Summary
Fixes #20 by adding a standalone, fail-open
postuninstallscript (scripts/postuninstall.mjs) and registering it inpackage.json. When a user runsnpm uninstall -g failproofaiorbun remove -g failproofai, hook entries are automatically cleaned up from Claude settings (.claude/settings.json), preventing dangling commands in user/project environments.❌ Root Cause Analysis
Previously,
package.jsonhad nopostuninstalllifecycle script:npm uninstall -g failproofaideleted the global binary from Node/npm global modules, but left--hookentries inside.claude/settings.json.command not found: failproofaierrors because Claude attempted to execute non-existent binaries referenced in its settings file.✅ Fix Details
scripts/postuninstall.mjs:Created a zero-dependency script executed automatically on npm/bun uninstallation:
~/.claude/settings.json), project (.claude/settings.json), and local (.claude/settings.local.json) settings files.failproofaimarked hook entries (is_failproofai: trueor--hookcommand signatures).package.jsonConfiguration:Registered
"postuninstall": "node scripts/postuninstall.mjs"under"scripts".🧪 Unit Tests & Verification
Added unit test suite in
__tests__/scripts/postuninstall.test.ts:returns 0 when settings file does not existremoves failproofai hook entries from Claude settings and writes clean fileTest Results:
vitest run __tests__/scripts/postuninstall.test.ts✓ 2 passed (2)Summary by CodeRabbit
New Features
Bug Fixes
Tests