Skip to content

fix(uninstall): require host cleanup before binary removal - #1177

Open
backnotprop wants to merge 1 commit into
mainfrom
codex-retire-skip-hosts-1175
Open

fix(uninstall): require host cleanup before binary removal#1177
backnotprop wants to merge 1 commit into
mainfrom
codex-retire-skip-hosts-1175

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

Summary

  • retire the host-cleanup bypass from CLI parsing, types, runtime branches, help, docs, diagnostics, and tests
  • always run recognized host plugin/config cleanup before binary removal
  • give host-specific manual cleanup commands or config-field guidance, followed by the correct uninstall rerun command
  • preserve the existing fail-safe gate that keeps the CLI and Windows PATH entry available after cleanup errors
  • add regressions for malformed and uneditable host config, unavailable and failing plugin managers, manual-cleanup retry, data retention, purge, and platform safeguards

Validation

  • focused uninstall/CLI suite: 56 passed, 0 failed
  • full suite: 2,670 passed, 204 skipped, 0 failed
  • bun run typecheck
  • bun run build:marketing
  • git diff --check
  • exhaustive repository and read-only Mintlify docs searches found no remaining retired-flag references

Docs

The separate Mintlify docs checkout contains no references requiring a companion change.

Closes #1175

@backnotprop

Copy link
Copy Markdown
Owner Author

Review (at 0b5ae578)

Verdict: merge as-is. Full suite 2670 pass / 0 fail; focused uninstall/cli/install tests 148 pass. Typecheck clean. scripts/ and .github/ untouched, so the install-script invariants and CI gates are unaffected by construction.

Branch topology: not stacked and not divergent. #1170 was squash-merged to main, and git diff between the old branch head and main over every uninstall file is empty, so this is a single clean commit on current content. All three of #1170's late hardening mechanisms were re-verified empirically on this PR's compiled binary: case-variant purge refusal, the TOCTOU boundary against a hostile mid-run data-dir swap, and symlink/hardlink-safe unlinking.

The design reversal holds. Retiring --skip-hosts was the risk here, since the flag existed to escape dead-ends. All three original dead-end repros were run end to end on a compiled binary, and each is now a real recovery loop: block with guidance naming the exact file and field, follow the guidance, rerun completes with the binary removed and unrelated content intact. That includes the chmod-400 host config containing a managed hook (the user's own hook in the same file survived) and the malformed-but-unrelated Gemini settings. No unrecoverable state was found, and grep over the whole repo returns zero residual mentions of the retired flag. The compiled binary rejects --skip-hosts with exit 1. The retry command correctly propagates --purge.

The new tests are genuine. All six fail when pointed at pre-PR code, and every item in the PR body's regression list has a real assertion behind it, including the full block, repair, rerun-completes loop for both the malformed and uneditable cases, and the escaped-"plannotator"-spelling fail-safe.

Fail-safe gate verified: after a host error the CLI stays on PATH and runnable, data is preserved, and a post-repair rerun completes fully. Windows PATH retention tests all pass.

Two should-fixes for a follow-up, neither blocking:

1. The missing-host-CLI guidance omits the recovery that works when the host is gone for good. The Droid message says only "restore droid on PATH and run the plugin uninstall", but a permanently uninstalled Droid is the usual reason it is off PATH, and that was exactly the case the retired flag used to cover. The working alternative was verified: removing the plannotator@plannotator entry from enabledPlugins in ~/.factory/settings.json makes detection go false and the next run completes. The message should name it (uninstall.ts:516).

2. The headline invariant is exact-match only (pre-existing). isManagedHook (uninstall.ts:1540) requires exact equality with the managed command strings, so a user who customized their hook with a documented flag, for example plannotator --browser Safari, is unmatched: reproduced with the binary deleted and the hook left dangling with no error. Not a regression, but it is now the claim this PR makes, so it deserves closing: before binary removal, re-scan host configs for any residual command containing a managed binary path and error (or at least warn with the filename) instead of proceeding.

Two nits: the malformed-config guidance tells users to remove Plannotator hooks from files that contain none (the actionable strict-JSON clause comes first, so harmless), and custom Kiro/Amp files at managed paths land in preserved while still referencing Plannotator, which is deliberate but technically a dangling reference under the strict reading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keep uninstall atomic when host integration cleanup fails

1 participant