feat(cli): add gitnexus uninstall to reverse setup (#2060)#2062
Conversation
…#2060) `gitnexus uninstall` was documented in abhigyanpatwari#168 but never implemented, so the CLI rejected it with "error: unknown command 'uninstall'" (abhigyanpatwari#2060). Add an `uninstall` command that reverses `gitnexus setup` target-by-target: removes the GitNexus MCP server entries (Cursor, Claude Code, Antigravity, OpenCode, Codex), the installed skill directories, and the Claude Code / Antigravity hook entries plus their bundled hook scripts. Edits are surgical and idempotent — only gitnexus-owned keys/entries/dirs are touched, and JSONC comments/indentation are preserved. Defaults to a dry-run preview; `--force` applies. Per-repo indexes and the global npm package are left alone with printed hints, since both are destructive in ways setup never caused. Adds i18n entries (en + zh-CN), help wiring, README/CHANGELOG docs, and unit tests covering MCP/hook/skill/Codex-TOML removal, dry-run, corrupt-file safety, and the no-op case.
|
@NilotpalK is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10793 tests passed 16 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Please revert the changes from the CHANGELOG.md files. |
magyargergo
left a comment
magyargergo
left a comment
There was a problem hiding this comment.
Tri-review: PR #2062 — gitnexus uninstall
Reviewed three ways: 8 Claude reviewer lanes (gitnexus risk / test-CI / security-boundary; CE correctness / adversarial / reliability / maintainability / testing) + Codex (live, independent engine). 8 of 9 lanes are Claude under different persona prompts (correlated priors); Codex is the only independent engine, so Codex+Claude agreement is weighted strong and Claude-only agreement as "consistent across personas."
Credit — the core is sound
- Reversal is correct. The setup→uninstall target map was checked target-by-target (correctness lane, independently confirmed by Codex): every MCP path & keyPath (
['mcpServers','gitnexus'], OpenCode's distinct['mcp','gitnexus'], Codex's[mcp_servers.gitnexus]), both hook needles/events, both hook-script dirs, and all 5 skill dirs matchsetup.tsexactly. No missed target. - Dry-run is fully side-effect-free across every branch (adversarial verified). Symlinks are safe —
fs.rmremoves the link, not its target. Command-injection clean — allexecFile('codex',…)args are constant. No bidi/whitespace/secret hygiene issues.
Inline findings
- [P2 — latent, P1 blast radius] Empty skill name wipes the entire editor skills dir.
path.basename('.md','.md') === ''→path.join(targetDir,'') === targetDir→fs.rm(targetDir,{recursive,force})deletes the WHOLE skills dir (×5 editors), taking user skills with it. Unguarded. LATENT today (the shippedgitnexus/skills/has no bare.md); reachable via a future stray file or theGITNEXUS_TEST_SKILLS_ROOToverride. One-line fix: skip empty derived names. Lanes: adversarial (proven e2e) + security (P1) + my own repro. [reproduced] - [P2] Corrupt
settings.json→ dangling hook. uninstall.ts:371 (& :388):removeDir(hookScriptDir)runs unconditionally, even whenremoveHookEntriesreturned'corrupt'. The hook ENTRY is correctly left in place, but its script dir is deleted → the editor then runs a registered hook pointing at a missing script (ENOENT) on every matched tool call. Fix: gate theremoveDironstatus !== 'corrupt'. Lanes: reliability (conf 95) + my code-read. [code-read] - [P2 — cross-engine] Whole-entry hook deletion destroys a co-located user hook. uninstall.ts:152 removes the entire matcher entry when ANY command in its
hooks[]matches the needle, whereas setup INSERTS at element granularity. A user who adds their own command into gitnexus's entry'shooks[]loses it silently — contradicting this file's own docstring ("other hooks … preserved", :8–11). Fix: filter only the gitnexus command fromentry.hooks; delete the entry only ifhooks[]becomes empty. Lanes: Codex + adversarial (proven e2e) + correctness — the one cross-engine-corroborated inline finding. [reproduced]
Body findings
- [P2 — cross-engine] Deletion is provenance-blind. Nothing distinguishes a gitnexus-installed artifact from a user-owned one with the same name. Codex (independent) flagged this as its top issue: a user dir/skill named like a bundled skill, user files inside a gitnexus skill dir, or a user's own MCP server literally named
gitnexusare all deleted purely by name match (Codex rated the skill-dir case P0; realistic trigger is narrower → P2). Adversarial + security noted the same absence of an ownership check. Suggested direction: havesetupdrop an ownership marker (e.g..gitnexus-managed) thatuninstallverifies before deleting. This and #1 share the fix surface (removeDir). - [P2/P3 — cross-engine, fallback-path only] Hand-rolled TOML stripper is fragile (runs only when the
codexbinary is absent). Codex + adversarial + correctness agree: (a) a[mcp_servers.gitnexus.env]child sub-table survives the strip as leftover dead/stale config when the parent[mcp_servers.gitnexus]is removed (still valid TOML, but a stale reference to a now-unregistered server); (b) the whole-file blank-line reflow (\n{3,}→\n\n, leading-newline strip) touches unrelated formatting, contradicting the "preserve formatting" claim; adversarial additionally showed a[mcp_servers.gitnexus]literal inside a multiline string truncates user data. Fix: a real TOML parser, or scope the section boundary to non-descendant headers + track multiline-string state. - [P2 — maintainability] setup.ts ↔ uninstall.ts duplicate the target map with no shared source of truth — ~14 file paths, 2 keyPaths, 3 event names, 2 needles, and
detectIndentation(copy-pasted). The next editor/path change to setup will silently stop being reversed, with no compile/test signal. Recommend extracting a sharededitor-targets.tsboth import. Not a bug today; a real drift hazard for a command whose whole job is to mirror setup. - [P2 — testing] Coverage is Claude-path-only. Untested: OpenCode MCP (the distinct
['mcp','gitnexus']keyPath), Antigravity MCP, Antigravity hooks (AfterTool /gitnexus-antigravity-hook), thecodex mcp removeSUCCESS path (the mock forces failure), 4 of 5 skill targets, the directory-layout skill branch, multi-match-in-one-event hook removal, and dry-run for hooks/skills. Only Claude skills prove a user skill survives — the data-loss preservation guarantee is untested for the other 4 targets. For a destructive command, strongly worth filling before merge. (uninstall.test.ts runs only in the ubuntu/coverage job, not the cross-platform subset — asymmetric with setup's tests.) - [P3 — minor / pre-existing] Partial uninstall exits 0 on accumulated errors (Codex + reliability; mirrors setup — recommend
process.exitCode=1); notimeoutoncodex mcp remove(can hang; mirrors setup); an emptymcpServers/mcpcontainer is left behind after leaf removal (cosmetic, not exact reversal);GITNEXUS_TEST_SKILLS_ROOThonored in production code (pre-existing from setup.ts:836 — track as a follow-up, not this PR).
Refuted / reconciled (validation is a feature)
- "Trailing-comma / BOM judged corrupt → silent incomplete uninstall" (adversarial) — refuted as a setup-divergence by both correctness and Codex:
parseTree/parseshare the same defaultParseOptions, and setup gates on the same parse, so any file setup wrote is parse-clean for uninstall. It survives only as a robustness edge if a user later hand-edits the file — and in that case it amplifies #2. Not posted as a standalone bug. - Needle cross-match (
gitnexus-hookvsgitnexus-antigravity-hook) — checked, they don't collide (different substrings, different files).
Recommendation
Not production-ready as-is — the core reversal works, but #1–#3 are data-loss / correctness landmines on a destructive command and should be fixed first (all have small, local fixes). #5–#7 are worth folding in while here. None are deep architectural problems. Merge state: mergeable, CI checks pending at review time (so not yet mergeable on checks).
CI
lint / format / typecheck / typecheck-web pass; full test + platform jobs were pending at review time; the Vercel "fail" is an auth-required deploy gate, unrelated to the code.
Coverage
Full diff reviewed (656 lines / 8 files; all source read). The happy-path reversal is correct; the findings are edge-case data-loss landmines, robustness gaps, and thin tests on a destructive command — none block the common path, but #1–#3 are worth addressing before this ships.
Automated multi-tool digest (8 Claude lanes + Codex) — verify before acting.
|
is support uninstall now? |
…yanpatwari#2062) Address review findings on the uninstall command: - Empty derived skill name no longer wipes the whole skills dir: a bare '.md' source file would make basename() return '', resolving to the skills dir itself. Skip empty names in derivation and reject empty/'.'/'..'/separator names in removeSkillsFrom. - Corrupt settings.json no longer orphans the hook: gate the hook-script dir removal on status !== 'corrupt' so we don't delete a script while a still-registered entry points at it (Claude + Antigravity blocks). - Hook removal is now element-granular: delete only the gitnexus command inside an entry's hooks[], removing the whole entry only when it becomes empty. Preserves a user command co-located in the same entry. - Fallback TOML stripper: also remove descendant sub-tables ([mcp_servers.gitnexus.env]), track multiline strings so a bracketed line inside a value isn't treated as a header, and stop reflowing unrelated blank lines. - Set process.exitCode=1 on partial failure; add a 10s timeout to 'codex mcp remove'. Tests expanded 7 -> 17: empty-skill guard, corrupt-settings hook preservation, shared-entry hook removal, OpenCode MCP keyPath, Antigravity MCP + AfterTool hooks, codex-remove success path, TOML sub-table + multiline-string cases, dry-run for hooks/skills, and the directory-layout skill branch.
Following up on two points from the review body (no inline thread on these, so replying here).
Before adding the marker, one design concern I'd like your call on. Migration hole: a marker only lands on installs done after it ships, so The risk also isn't uniform: MCP writes Inclination for this PR: keep dry-run + name-derivation, document the boundary, and treat true provenance (marker + legacy fallback) as a follow-up with the migration story worked out. Happy to do the marker here instead — just
Agreed this is the real drift hazard. One scoping note: Two options:
|
|
Thanks for laying this out. The migration concern is valid. On provenance, I agree that adding a marker in isolation would create a false sense of safety. It would only cover future installs, would still require a heuristic fallback for existing installs, and would not protect user modifications inside a previously installed directory. Proper provenance probably needs a versioned ownership manifest and a defined legacy migration policy rather than just a boolean marker. For this PR, I am comfortable keeping the current name-derived removal with dry-run by default, provided that:
On the target map, I would prefer option 1, but with the extraction limited to the declarative metadata you described: locations, key paths, events, needles, script directories, and shared formatting helpers. The format-specific mutation logic should remain in A target module consumed only by Please also add the setup --> uninstall round-trip integration test. The shared metadata prevents structural drift, while the round-trip test verifies that the two implementations remain behaviourally symmetrical. So my preference is:
|
…k (review abhigyanpatwari#2062) Maintainer review follow-ups: - Extract editor target identities into editor-targets.ts (MCP paths/keyPaths, Codex TOML section, skill dirs, hook settings/events/needles/script dirs, shared detectIndentation). Both setup.ts and uninstall.ts consume it, so a target change updates both sides — killing the silent drift hazard. - Add a setup -> uninstall round-trip integration test that iterates getEditorTargets(): setup writes every target, uninstall removes all of them, and a co-located user MCP server + user hook survive. Drift tripwire in both directions. - Preview now prints the exact paths it would remove; command output + README state skills are matched by bundled gitnexus skill name. (Provenance marker deferred to a tracked follow-up.) Hardening of the hand-rolled Codex TOML fallback (found in code review): - Strip a section header that has a trailing inline comment (was matched as a header but failed the exact classify check -> section left behind while reported removed). - Preserve CRLF line endings instead of rewriting the whole file to LF. - Fix multiline-string scan: a line with an odd count of BOTH """ and ''' no longer mis-picks the delimiter and desyncs the scanner (left->right scan). - removeSkillsFrom guard also rejects absolute names. Regression tests added for each. Full setup/uninstall suite green.
|
Thank you! |
3 participants
gitnexus uninstallwas documented in #168 but never implemented, so the CLI rejected it with "error: unknown command 'uninstall'" (#2060).Add an
uninstallcommand that reversesgitnexus setuptarget-by-target: removes the GitNexus MCP server entries (Cursor, Claude Code, Antigravity, OpenCode, Codex), the installed skill directories, and the Claude Code / Antigravity hook entries plus their bundled hook scripts. Edits are surgical and idempotent — only gitnexus-owned keys/entries/dirs are touched, and JSONC comments/indentation are preserved. Defaults to a dry-run preview;--forceapplies. Per-repo indexes and the global npm package are left alone with printed hints, since both are destructive in ways setup never caused.Adds i18n entries (en + zh-CN), help wiring, README/CHANGELOG docs, and unit tests covering MCP/hook/skill/Codex-TOML removal, dry-run, corrupt-file safety, and the no-op case.
Testing & verification
cd gitnexus && npm testcd gitnexus && npm run test:integration(if core/indexing/MCP paths changed)cd gitnexus && npx tsc --noEmitcd gitnexus-web && npm test(if web changed)cd gitnexus-web && npx tsc -b --noEmit(if web changed)gitnexus-web/e2e/)