fix: uninstall GJC plugin bundles - #3875
Conversation
691790c to
ebbf1bb
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Red-team review of the GJC bundle uninstall path (head ebbf1bbe0, base dev).
Verified working (manual CLI attack matrix, isolated GJC_CODING_AGENT_DIR)
install --userthenuninstall --user: plugin root removed + registry entry removed;list --json→gjc: []- Reinstall after uninstall: clean install, no
already_installedresidue - Same bundle in both scopes, unqualified
uninstall: clean "installed in both scopes; specify --user or --project" error, exit 1;--user/--projectresolution works - Project-scope uninstall removes only the project entry
safeInstalledRoottraversal guard: registrypluginRootpointed at an outside directory → typedinvalid_targeterror, exit 3, outside directory untouched- Dangling root (dir already deleted): uninstall still clears the registry entry (self-healing)
--jsonoutput shape{"uninstalled":{"kind":"gjc-bundle","scope":...,"name":...}}- All 16 tests in the two touched files pass locally; CI green (both test files, ts-build, native-build, gjc-state-gates)
Blocking findings
1. Regression: a corrupt or malformed GJC registry bricks all uninstall commands, including npm/marketplace fallback.
handleUninstall now calls listGjcBundles({ cwd }) unconditionally before the marketplace/npm paths. Two crash modes, both reproduced:
- Corrupt
registry.json(invalid JSON) →uninstallGjcBundlenever runs;readRegistrythrowsGjcPluginLoadError: Corrupt GJC plugin registryas an uncaught exception.gjc plugin uninstall <npm-plugin>(which never touched the GJC registry before this PR) now dies with exit 1 and a crash log. - Structurally-invalid entry (e.g. missing
surfaces.subskills) →toBundleSummary/surfaceSummariesthrows a rawTypeErrorinlistGjcBundlesbefore the target is even matched.
Net effect: the exact broken state this fix exists to let users recover from (bad/partial registry data) makes the recovery command itself crash — including npm and marketplace uninstall. plugin list already had this exposure pre-PR; uninstall did not. This is a new dependency of all uninstall paths on GJC registry integrity.
2. No failure cleanup / rollback on uninstall (asymmetric with the install transaction).
uninstallGjcBundle does fs.rm(root, { recursive: true, force: true }) then writeRegistryUnlocked, with no try/catch, rollback, or remnants. If fs.rm throws (Windows EBUSY on a file open by a running agent session, EACCES, EPERM), the exception propagates uncaught: the registry keeps the entry while the root is partially deleted, and reinstall is then blocked by already_installed_use_upgrade with no CLI salvage path. If the registry write fails after a successful rm, the entry dangles. force: true only swallows ENOENT. The install path (runGjcBundleTransaction) has backup-dir + rename rollback and remnants reporting; uninstall has none of it.
Non-blocking
- Wrong-scope uninstall (
uninstall <bundle> --projectwhen the bundle is only in user scope) falls through to npm and prints a misleading "npm uninstall failed for " instead of a scope-aware "not installed" message. - Marketplace path still uses
flags.scope, so--user/--projectsilently do not scope marketplace/npm uninstalls (inconsistent with the new bundle path).
Requested changes (all small)
- Make
handleUninstallresilient to GJC registry corruption: the npm/marketplace fallback must work even when the GJC registry is corrupt, and the bundle path must be reachable when a different entry is malformed (e.g. resolve the target by direct locked registry lookup and only summarize on success, or skip malformed entries inlistGjcBundles). - Make
uninstallGjcBundlea typed two-phase op: rename root → tombstone under the scope root (same-dir, atomic), update registry, then best-effort remove the tombstone; any rm failure returns a typed result with recovery guidance instead of an uncaught throw. Keep thesafeInstalledRootguard on the tombstone path. - Add regression tests for: corrupt registry + npm/marketplace uninstall survival, malformed-entry uninstall, both-scope ambiguity,
--projectuninstall,--jsonoutput, traversal guard (invalid_target), and reinstall-after-uninstall.
Core fix is correct and verified; the resilience + failure-cleanup gaps are the blockers.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Corrupt registries and filesystem failures previously made uninstall crash or leave a half-removed bundle. Resolve GJC targets by scope, use a tombstone rename with registry rollback, and cover recovery paths with regression tests.
|
Addressed the blocking review findings in commit
Verification:
|
The fallback may report either a successful or failed package-manager uninstall depending on the runner's installed package state. Assert the important contract: it reaches the fallback without exposing a corrupt GJC registry crash.
|
CI found one environment-dependent assertion in the corrupt-registry fallback regression test: package-manager uninstall may exit 0 or 1 depending on runner state. Adjusted commit Local verification remains green:
|
Summary
gjc plugin uninstallto GJC bundle lifecycle before npm fallbackFixes #3874
Verification
bun test packages/coding-agent/test/gjc-plugin-lifecycle.test.ts packages/coding-agent/test/plugin-command.test.tsbun --cwd=packages/coding-agent run checkbun run build:native && bun run install:dev:bin