Repository navigation
fix(wizard): skip a plugin only when it is actually installed, not merely recorded - #563
Conversation
|
Claude finished @kshivang's task in 6m 11s —— View job Review: wizard skip guard, installed-vs-usable
Verdict: the one-line change at 1. The other half of the defect is still there:
|
|
Addressed the substantive result-reporting finding in The original skip-guard fix and six tests remain @arjun28115's contribution; these repairs are separate reviewer work. The original head passed all nine hosted checks. Fresh-head CI and the shared-lock local test/ktlint/detekt run remain pending. The tests still do not exercise the complete wizard loop; that limitation is explicit in the updated description. The user-disabled/resident/stale-path cleanup scenario is recorded as the review's nonblocking watch item. Public manager API renaming and helper naming suggestions are outside this focused change. No duplicate of this predicate fix was found in current dev/main, merged #443/#429, or excluded draft #590 (read-only context). No merge or auto-merge enabled. |
|
@arjun28115 Your skip-guard change fixes a real problem: reopening the wizard no longer counts a rejected/deleted plugin as installed just because the manager retained an entry. The original commit and credit are preserved, along with reviewer commit Final merge assessment: not ready; handing back one remaining lifecycle regression risk. This supersedes my earlier treatment of Claude finding 2 as merely a watch item. The newly authorized merge gate requires resolving substantive regression risks, and the possible artifact loss deserves a guard/test before merging:
Please add a safe resident-plugin/reinstall decision or staged artifact ownership/rollback protection before promotion/cleanup. Preserve the current behavior for rejected/unloaded plugins, ordinary access-disabled installs, and LOADED plugins with stale paths. Merely suppressing deletion after overwriting the destination would not establish artifact preservation. Required regression validation: seed an existing final artifact and persistence row plus a resident DISABLED plugin with stale manager path; attempt the wizard install and assert the old artifact bytes and persistence reference survive the refusal, with an honest result. Also cover rejected/unloaded retry, registration-incompatible failure, access-disabled success, and LOADED stale-path skip. Please exercise the service's decision/cleanup wiring, since the current predicate-only tests cannot detect reverting its call site. Run targeted desktop tests, ktlintCheck and detekt, and obtain green required CI on the resulting head. Validation infrastructure is separate: original CI was 9/9 green. Current-head CI 34736682589 is still queued/running (shell/version/pgTAP passed), and local targeted tests/ktlint/detekt are waiting for the shared build lock. These are not evidence of an author defect or a failing test. No final-head pass is claimed. The PR is being converted to draft under the handoff policy, rather than merging with the remaining regression risk. No shared branch or main was changed. |
|
Pushed
One blank line before the class brace, removed. No test and no behaviour touched. It arrived with On the lifecycle risk you handed back, I read it as: a user-disabled plugin stays resident while manager state is DISABLED, a stale |
…its jar The handed-back lifecycle risk on risa-labs-inc#563. `disablePlugin` unregisters panels and sets DISABLED but never unloads, so a user-disabled plugin keeps its id in the loader. A DISABLED entry is only counted as installed while its file exists, so once its recorded jarPath goes stale the wizard offers it again. The Store path then moved the download onto the installed artifact, `loadPlugin` refused the resident id, and the failure branch deleted the file `installed.json` still named. The plugin was left with a persistence row pointing at nothing. Residency is now checked BEFORE the promotion, in a new `stageAndInstall`. The refusal leaves the existing artifact untouched and removes only the download, which this call owns. Checking after the move could not be rolled back, which is why suppressing the delete alone would not have been enough. Cleanup is also by ownership rather than by outcome: a failure removes the final file only when this call created it. When the move landed on a path that already held a jar, that jar belonged to a previous install and deleting it would strand its persistence row in the same way. Residency needed a query. `DynamicPluginManager.isInstalled` reports whether a manager ENTRY exists, which is exactly what does not answer this: a rejected binary-incompatible plugin has an entry and is not resident, and a user-disabled one is resident whatever its entry says. Added `isPluginResident`, delegating to the loader's own `isLoaded`. `install` is injected so the decision and the cleanup are testable without a DynamicPluginManager. WizardStagedInstallTest adds 9 tests over real files and a real move: the seeded-artifact refusal, that the loader is never reached, that the download is cleaned up, that a rejected plugin still retries, that a failure removes a jar this call created but keeps one that was already there, and that the original loader failure is passed through. Unchanged on purpose: the GitHub path, which downloads straight to its final name and never promotes over an installed artifact. ktlint 1.8.0 clean on all three files; no added line over 120 columns. No local test run is claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed The decision moved ahead of the promotion. New Cleanup is now by ownership, not by outcome. A failure removes the final file only when this call created it. If the move landed on a path that already held a jar, that jar came from a previous install and deleting it would strand its persistence row in exactly the way this PR is about. That is the narrower half of the same bug, so it is fixed here too. Residency needed a query, and I verified the premise rather than assuming it: On exercising the wiring. That is the part your finding 3 asked for. A predicate-level test cannot notice this being reverted at its call site; these fail if the residency check moves back after the move, or if the cleanup stops distinguishing ownership. Scope I deliberately did not take. The GitHub path is unchanged: it downloads straight to its final name and never promotes over an installed artifact, so the loss shape does not exist there. I also left the Correction on my own push. ktlint 1.8.0 is clean on all three and no added line exceeds 120 columns; I now run the pinned ktlint locally rather than discovering that on CI. No local test run is claimed, so the suite result on this head is the real check. |
…its jar The handed-back lifecycle risk on risa-labs-inc#563. `disablePlugin` unregisters panels and sets DISABLED but never unloads, so a user-disabled plugin keeps its id in the loader. A DISABLED entry is only counted as installed while its file exists, so once its recorded jarPath goes stale the wizard offers it again. The Store path then moved the download onto the installed artifact, `loadPlugin` refused the resident id, and the failure branch deleted the file `installed.json` still named. The plugin was left with a persistence row pointing at nothing. Residency is now checked BEFORE the promotion, in a new `stageAndInstall`. The refusal leaves the existing artifact untouched and removes only the download, which this call owns. Checking after the move could not be rolled back, which is why suppressing the delete alone would not have been enough. Cleanup is also by ownership rather than by outcome: a failure removes the final file only when this call created it. When the move landed on a path that already held a jar, that jar belonged to a previous install and deleting it would strand its persistence row in the same way. Residency needed a query. `DynamicPluginManager.isInstalled` reports whether a manager ENTRY exists, which is exactly what does not answer this: a rejected binary-incompatible plugin has an entry and is not resident, and a user-disabled one is resident whatever its entry says. Added `isPluginResident`, delegating to the loader's own `isLoaded`. `install` is injected so the decision and the cleanup are testable without a DynamicPluginManager. WizardStagedInstallTest adds 9 tests over real files and a real move: the seeded-artifact refusal, that the loader is never reached, that the download is cleaned up, that a rejected plugin still retries, that a failure removes a jar this call created but keeps one that was already there, and that the original loader failure is passed through. Unchanged on purpose: the GitHub path, which downloads straight to its final name and never promotes over an installed artifact. ktlint 1.8.0 clean on all three files; no added line over 120 columns. No local test run is claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
51ac06d to
9cee17b
Compare
|
Rebased onto current
Your commits are preserved in order with their authorship. The SHAs changed because a rebase rewrites them; nothing else did. I checked file by file that every file this PR touches is byte-identical to what the branch held before the rebase, so the only intended change is the base. Flagging one thing rather than leaving you to find it: while rebasing #596 earlier I force-pushed over three of your commits on that branch before restoring them. I have since checked each branch's full history before touching it, which is what I should have done first. If you would rather I never rebase a branch you have commits on, say so and I will just tell you when one needs it. |
|
The blocker you named is cleared. Current head The dev-integration recheck you listed as the other step is also done, and it needed doing for a different reason: Marking ready is yours under the bounded-review policy, so I have not touched the draft flag. Flagging it because the condition you set has been met rather than leaving it to a poll. |
|
@claude Please review current head 9cee17b against dev, including the contributor residency/staging follow-up. Prior review predates that change. Focus on artifact preservation for both Store and GitHub retry paths, download failures, and what the new file tests establish. One review request for this sweep. |
|
Claude finished @kshivang's task in 6m 27s —— View job Review: residency/staging follow-up at
|
|
@arjun28115 Thank you for the concrete follow-up: Two artifact-preservation gaps remain after reviewing the actual call paths:
Please add byte-for-byte assertions for both routes with an existing artifact and persistence reference; cover mid-stream failure, resident refusal, failed replacement, rejected/unloaded retry, registration-incompatible failure, and access-disabled success. Exercise the production service wiring as well as the extracted helper. Run targeted desktop tests, ktlint and detekt plus required CI on the repaired head. The Store resident-refusal test is meaningful, but it is not a full service/persistence integration test. These are static control-flow findings, not a claimed live reproduction. I am returning the PR to draft rather than merging with unresolved artifact-loss risk; no source changes were pushed in this sweep. The latest Claude review was requested once for the new staging head and is pending. The previous review's result-classification/KDoc findings are addressed; API renaming and helper naming remain nonblocking. Issue #600 concerns separate concurrent host updates and is read-only context, not a request to take over that issue. Original contributor and reviewer credit remain intact. |
|
Both findings are right, and the first one contradicts something I asserted, so let me be direct about that before the fix. I claimed the GitHub path was safe and it was not. My words were that it "downloads straight to its final name and never promotes over an installed artifact", and I treated the absence of a promotion as the absence of a loss. Downloading straight to the final name is the overwrite. Fixed at 1. GitHub stages first. The download lands on a unique staging name that deliberately does not end in 2. Replacement now rolls back. You are right that keeping the pathname is not preserving the artifact, and right that my test proved nothing: asserting One more I found while doing it. A throw from the loader escaped past the cleanup entirely, leaving the incumbent overwritten with no rollback at all. It is caught now, and Tests go from 9 to 12 and assert bytes rather than existence: original contents restored on failure, sidecar restored with its jar, a successful replacement leaving no backup behind, and a throwing loader still restoring. What I am not claiming. There is no service-level integration test. Thank you for reading the actual call paths rather than the description. The GitHub finding is one I would not have found by re-reading my own reasoning, because my reasoning was the thing that was wrong. |
|
All ten checks pass on What the fix does, briefly, since it took four heads to land:
Tests are 9 to 12 and assert bytes, not existence. The one you identified, On the three extra heads. Every one was detekt, and the causes are worth recording rather than hiding:
I cannot run detekt on this repository locally, only ktlint, so those surfaced one head at a time instead of together. That is on me, not on the gate. One thing for you rather than for this PR: the baseline entry for Still not claimed: a service-level integration test, or any native reproduction of #551's symptom. What these tests exercise is the promotion and rollback both routes call. Marking ready since the artifact-loss risk you returned it for is resolved and CI is green. If you would rather re-draft it, that is entirely fair. |
|
Claude finished @kshivang's task in 5m 7s —— View job Review: staging/rollback repairs at
|
|
@arjun28115 I re-read the four new commits through The new rollback has remaining failure paths that prevent merge:
The Store download still uses the fixed, scannable These are static control-flow findings, not a claimed native reproduction. I have not pushed a narrow cleanup patch that leaves the other transaction failures unvalidated. The host remains the correct home for this install orchestration. Separately, GitHub reports this head CONFLICTING with current dev. That is shared-base drift, not evidence the original contribution was defective. Reconcile with latest dev while preserving changes already squash-merged to main (#649 included); the large ancestry diff must not reintroduce or revert unrelated work. Then run targeted I requested one current-head Claude review: #563 (comment). It is still in progress at this handoff; no approval is claimed. Returning this PR to draft for the above correctness issues. Your original work and subsequent repairs remain credited separately from earlier Codex repairs. Related wizard UX PRs #590/#623 and shared update issue #600 remain separate, read-only context. |
|
Heads up before that review lands: I found a flaw in my own sidecar handling while re-reading it, and pushed
Two tests, 12 to 14. The unbackable case is forced by occupying the backup path with a directory, which is the only portable way I know to make Worth saying why I looked: your review request named sidecar failure handling specifically, and that prompted me to check the branch I had the least evidence for rather than wait to be told. It was the right place to look. |
|
Claude finished @kshivang's task in 4m 51s —— View job Review: partial promotion cleanup & destination ownership at
|
| Occupant | Listed in the plugin manager? | Refusal message |
|---|---|---|
| binary-incompatible, jar intact | yes (DISABLED entry) | actionable |
| signature-verification failure | no entry at all | dead end |
| leaked by finding 1 | no entry at all | dead end |
The middle row is not hypothetical: loadPlugin fails before registration, so installPlugin creates no entry (DynamicPluginManager.kt:969-978 records a load gate and returns failure), and DynamicPluginLoader.kt:610-612 deliberately leaves the artifact in place - "will keep being rejected until it is reinstalled or removed". Reinstalling over it was that remedy. The Toolbox route still works, because StoreMissingDependencyInstaller targets <id>_<version>.jar where the wizard targets <id>-<version>.jar, so it does not meet the same occupant - but the wizard's own sentence sends the user somewhere that cannot help. Either widen the wording to name the file path, or branch on whether the occupant is a known plugin. Replacing it instead would reintroduce the loss shape, so the wording is the fix, not the guard.
4. The refusal can be made exclusive for free, rather than documented as serialized
On the concurrency point: I do not think there is a live same-path race to reproduce. The wizard's destination is <id>-<version>.jar; StoreMissingDependencyInstaller uses <id>_<version>.jar (:219) and StoreVersionInstaller.targetFor the same underscore form, and the wizard loop is sequential - so no other installer in the tree writes the wizard's destination name. But the guarantee is currently "no caller does this", which is what drifts.
It does not need a lock or an untestable race regression: Files.move without REPLACE_EXISTING throws FileAlreadyExistsException when the destination exists, which is the atomic form of exactly this refusal. A moveIfAbsent beside atomicMoveFrom in AtomicFileWrite.kt would collapse :839's check-then-move into one operation, and its failure is then a refusal rather than a silent overwrite. atomicMoveFrom's own KDoc is the right place to say why the two exist.
5. Nits
- The displaced KDoc has moved again, third round.
:676-680("Marks a download still in flight… breaks the.jarsuffix… carries a nonce") sits directly aboveprivate val GITHUB_URLat:681. It documentsSTAGING_MARKER/stagingNameForat:718-720, which have none. - The resident refusal still deletes
downloadedFileunconditionally (:850).movingIntoPlaceis computed at:828and is false when the paths are equal, in which case the refusal deletes the artifact it exists to protect - as does:877. Neither call site can reach it today, but the function isinternal, it models the case, and the test that used to establish that shape (a download that needs no move is still cleaned up on failure) was dropped this round, so nothing records the intent any more. writeStagedDownloadis still untested, and it is this round's actual repair on the GitHub side. It isinternaland takes anInputStream, so a throwing stream plus a temp dir is the entire test: assert the partial file is gone and theIOExceptionpropagates.- Call-site pinning remains open (previous review's finding 3). All 11 tests call the free
stageAndInstall, so reverting either call site -:224or:413- back to inlineatomicMoveFrom+installPlugin+finalFile.delete()leaves them green.isResidentis injected; a third injected collaborator for the staging step, with one test asserting the manifest id is what gets passed, is the closure. Theresidency is asked about the id it is givencase is not it - it passes a literal into the parameter and asserts it comes back. usableWizardInstallResultturning a registration-incompatible success into a failure now meansstageAndInstalldeletes the jar the manager's fresh DISABLED entry names. That is consistent withinstalledAndOnDisk(jar absent → retried next run) and I think it is what you want, but it is an in-memory entry pointing at a deleted path and deserves the one line of comment that says so deliberately.finalFileForStagedusessubstringBefore, so an asset name literally containing.downloading.promotes to a truncated name. Contrived, and the round-trip test does not cover it.- The stale detekt baseline entry recording
installFromGitHubasResult<Unit>is real and independent of this PR, as the author noted.
Not verified: I did not run the suite, ktlint or detekt - a :composeApp:desktopTest run needs a full module compile in this environment. Findings 1-4 are static control-flow and file-API reads (moveTo outside the try, Files.move semantics with and without REPLACE_EXISTING, verifySignatureOrThrow's present-but-invalid policy, the loadExternalPlugins extension filter, installer target-name comparison), not live reproductions. git diff origin/dev...HEAD reports no merge base in this checkout, so I diffed against the merge base 5a2e818 and read the current file whole.
• branch fix/wizard-installed-predicate
…print Review found that refusing an occupied JAR does not make promotion indivisible, and it was right. `SignedArtifact.moveTo` moved the jar and then its sidecar, and `stageAndInstall` called it BEFORE the try/cleanup block. If the second move failed - the final `.sig` path being a nonempty directory is enough, and a permission change between the check and the move will do it too - the jar was left at its scannable installed name with its signature stranded at the staging name, nothing cleaned it up, and the occupied-destination refusal then blocked every retry. Three changes: **Promotion is inside the failure boundary.** `moveTo` records each path as it lands and deletes exactly those on failure, so a partial promotion leaves the destination as it was found. Deleting rather than moving back is deliberate: every path was established free beforehand, so deleting cannot destroy an artifact this call did not create, whereas a move back has a failure of its own that would leave the same mess. What is lost is a download, which the retry re-fetches. The loader is never reached. **The refusal covers the whole footprint**, not just the jar. A stale `.sig` beside a jar removed by hand is still somebody else's file, and promoting an unsigned download next to it would leave an old signature asserting bytes it never vetted - which PluginSignatureSidecar's own contract calls out as the thing never to do, and which is worse than no signature because the loader reads it as present-and-invalid. Refusing also means every path this call writes was established free, which is what makes the cleanup precise rather than indiscriminate. The message names the obstructing file so the user knows what to remove. **The destination-ownership claim is corrected, not restated.** The check and the move are two operations, so it holds against what is already on disk, not against a second installer racing this one. `ATOMIC_MOVE` cannot close that gap: on POSIX it is rename(2), which replaces the target silently - verified with a probe on this platform rather than assumed. Promotion therefore uses a plain non-replacing move, so a racing installer collides with FileAlreadyExistsException and this call reverts, instead of the two overwriting each other silently. Serialising installs is the only thing that would make the race impossible, and that does not belong in this function; the KDoc says so rather than implying a guarantee that is not there. Three regression tests over real files and real moves: a blocked destination sidecar (loader not called, no jar at the installed name, the obstruction and its contents untouched, and the retry succeeds once it is removed); an unsigned download meeting an old sidecar; and a promotion onto an occupied path failing rather than replacing it. Reported-by: kshivang Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@kshivang confirmed and fixed in Promotion is now inside the failure boundary. The refusal now covers the whole footprint, not just the jar — which also answers your unsigned-download case. A stale On destination ownership — you were right that the claim was too strong, and I have not restated it. I probed the actual semantics rather than reasoning about them: So Tests — three new, over real files and real moves, all confirmed to fail against
The blocked-sidecar test covers what you asked for end to end: loader not called, no jar at the scannable installed name, the obstruction and its contents untouched, and the retry succeeding once the obstruction is removed.
One process note worth recording: I got BossConsole building locally for the first time this session, and it immediately caught a test compile error in this very change that would otherwise have gone to CI. Two of my earlier CI failures on this PR were the same class of mistake. That gap is closed at my end. No native wizard check performed, as you noted — the mixed-batch and signed-retry paths still want a manual pass in a disposable profile. |
…rload `assertContains(charSequence, other, message)` binds the message to `ignoreCase: Boolean`, so overload resolution falls through to the Iterable and array candidates and the call does not compile. The message moves to a comment; the assertion is unchanged. This was already fixed in my working tree when 3d7f888 was pushed - the amend that should have carried it was part of a compound command that was denied, and I pushed the pre-fix commit without re-checking. The commit, not the working tree, is what CI builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correction on my own record, since you'd have seen That failure was not in the promotion change — it was a two-argument-vs-three-argument Corrected in I've also stopped running only the classes you named. The full desktop suite now passes locally against the committed tree — 4,932 tests across 515 classes, 0 failures, plus The substantive review points from your last pass are unchanged in |
d22d2bf to
77c4a40
Compare
bdcf620 to
bc9aa3b
Compare
The branch was 198 commits behind dev, and its last CI ran on 2026-09-22. Merged rather than rebased because the branch carries a commit by @kshivang, and a merge keeps every existing SHA reviewers have referred to in this PR's thread. No conflicts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@fluck-boss @kshivang out of draft. I had left it there on 2026-09-15 for you to decide, and it did not get decided, so here is where it stands:
Not done, and said in the description: a live wizard run. |
|
Merging on a green post-sync gate (CI all-green on 8fcf9a6, checks 07:23Z) with reviewer rounds verified against the head: the store-path residency ordering and the GitHub-path staged download (writeStagedDownload) are both in place. Follow-ups carried (tracked in #1590):
|
…talled predicate (#1640) - writeStagedDownload is tested on both halves of its contract: the whole stream lands, and a stream that dies mid-download removes its partial file and rethrows. The dying stream serves 16 KiB first, so bytes are on disk when it fails; a stream that failed on its first read would pass against a version that never deleted anything. - Both install paths are pinned to stageAndInstall by a source test (comments stripped, since the KDoc names atomicMoveFrom on purpose). Reverting a call site to an inline atomicMoveFrom kept every behavioural test green; now it fails one. - The resident refusal no longer deletes the download when the download IS the destination. stageAndInstall models that case explicitly, and in it the unconditional delete removed the installed jar - in the one refusal that exists because something is still running from that file. - DynamicPluginManager.isInstalled is renamed hasEntry rather than deleted. It still had five production callers (PluginRemoval x3, DependentRestartPrompt, and the manager's own uninstall-cancelled log), so deleting it would have broken plugin removal. The name was the hazard: an entry survives a rejected, deleted jar, which is the reading #563 proved wrong. Its KDoc now says so and points at installedAndOnDisk. - The KDoc displacement was already in order on the merged code; nothing to change. Mutation-checked: each guard's removal fails exactly one named test. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: fluck-boss <fluck-boss@users.noreply.github.com>
* fix(wizard): skip a plugin only when it is actually installed, not merely recorded The first-run wizard's skip guard called DynamicPluginManager.isInstalled, which is pluginStates.containsKey. An entry is not a usable plugin. installPlugin registers a DISABLED entry for a jar it rejected as binary-incompatible and then deletes that jar, so the wizard announced "already installed, skipping" for a plugin that was not there, added its id to installedIds, and finished reporting success for something it never installed. The user picked it during setup and did not get it. This is the trap AGENTS.md already records for the dependency prompt, at a call site that was not converted with it. The same file was already using the correct predicate after the loop, for the dependency report. It now uses PluginDependencyResolution.installedAndOnDisk, the codebase's single definition: LOADED, or the jar exists and the plugin is not recorded as binary incompatible. Both clauses carry their weight here. Requiring only an entry is the bug above. Requiring only the jar would refuse to skip a running plugin whose file has moved, because neither PluginJarReconciler nor the updater repoints the manager's jarPath, and reinstalling it fails with "already loaded". Recomputed per call rather than hoisted, since the loop installs plugins and a snapshot taken before it would go stale exactly when a later entry depends on an earlier one. Six tests, written around the cases where the two predicates disagree, since the cases where they agree would pass either way. The last one pins the disagreement as a set difference, so a future change to either definition shows up as a changed disagreement rather than silently converging. 4405 composeApp tests pass. ktlint and detekt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(wizard): reject unusable install results before reporting success * fix(wizard): refuse to overwrite an installed plugin, rather than roll back Takes the bounded alternative offered in review, and collapses four commits of my own CI repairs into the change they were repairing. **Refuse an occupied destination.** Nothing already at the installed path is replaced. The previous attempt moved over it and rolled back on failure, and that grew a tail of its own failure paths: a backup that will not rename, a promotion that throws between backup and install, a restore that fails silently, a fixed-name backup that can destroy the only original left by an earlier failure. Each could lose the artifact the rollback existed to protect, and each needed an injected file-operation seam to test. Refusing has no tail: nothing is overwritten, so nothing needs undoing. It also makes cleanup unambiguous, because everything at the destination afterwards belongs to this call. **The signature travels with the jar.** `RemotePluginRepository` writes the sidecar beside the DOWNLOAD path, so promotion is when it has to move. Moving only the jar left every signed store install loading unsigned while its signature sat at the staging name. `SignedArtifact` moves and deletes the jar with whichever sidecar exists, carrying both suffixes the loader knows. **Both downloads stage.** The Store path used a fixed `-downloading.jar`, which a directory scan reads as a plugin and which two installs of one id collide on. Both paths now use a unique name whose marker breaks the `.jar` suffix, with one constant defining it so writer and reader cannot drift. `isPluginResident` answers what `isInstalled` cannot: that reports whether an entry exists, while `disablePlugin` leaves a DISABLED plugin resident in the loader, which is the case that made this reachable at all. 11 tests over real files and real moves: an occupied destination refused with its bytes intact and the loader never reached, a resident plugin refused before anything moves, the signature arriving with its jar, a failure removing both and leaving no orphan sidecar, a throwing loader not escaping the cleanup, and the staging name being unique, unscannable and reversible. Rebased onto current dev, which has now been rewritten four times today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(wizard): make promotion all-or-nothing, and refuse the whole footprint Review found that refusing an occupied JAR does not make promotion indivisible, and it was right. `SignedArtifact.moveTo` moved the jar and then its sidecar, and `stageAndInstall` called it BEFORE the try/cleanup block. If the second move failed - the final `.sig` path being a nonempty directory is enough, and a permission change between the check and the move will do it too - the jar was left at its scannable installed name with its signature stranded at the staging name, nothing cleaned it up, and the occupied-destination refusal then blocked every retry. Three changes: **Promotion is inside the failure boundary.** `moveTo` records each path as it lands and deletes exactly those on failure, so a partial promotion leaves the destination as it was found. Deleting rather than moving back is deliberate: every path was established free beforehand, so deleting cannot destroy an artifact this call did not create, whereas a move back has a failure of its own that would leave the same mess. What is lost is a download, which the retry re-fetches. The loader is never reached. **The refusal covers the whole footprint**, not just the jar. A stale `.sig` beside a jar removed by hand is still somebody else's file, and promoting an unsigned download next to it would leave an old signature asserting bytes it never vetted - which PluginSignatureSidecar's own contract calls out as the thing never to do, and which is worse than no signature because the loader reads it as present-and-invalid. Refusing also means every path this call writes was established free, which is what makes the cleanup precise rather than indiscriminate. The message names the obstructing file so the user knows what to remove. **The destination-ownership claim is corrected, not restated.** The check and the move are two operations, so it holds against what is already on disk, not against a second installer racing this one. `ATOMIC_MOVE` cannot close that gap: on POSIX it is rename(2), which replaces the target silently - verified with a probe on this platform rather than assumed. Promotion therefore uses a plain non-replacing move, so a racing installer collides with FileAlreadyExistsException and this call reverts, instead of the two overwriting each other silently. Serialising installs is the only thing that would make the race impossible, and that does not belong in this function; the KDoc says so rather than implying a guarantee that is not there. Three regression tests over real files and real moves: a blocked destination sidecar (loader not called, no jar at the installed name, the obstruction and its contents untouched, and the retry succeeds once it is removed); an unsigned download meeting an old sidecar; and a promotion onto an occupied path failing rather than replacing it. Reported-by: kshivang Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(test): call the two-argument assertContains, not the Iterable overload `assertContains(charSequence, other, message)` binds the message to `ignoreCase: Boolean`, so overload resolution falls through to the Iterable and array candidates and the call does not compile. The message moves to a comment; the assertion is unchanged. This was already fixed in my working tree when 3d7f888 was pushed - the amend that should have carried it was part of a compound command that was denied, and I pushed the pre-fix commit without re-checking. The commit, not the working tree, is what CI builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Shivang <shivang.iitk@gmail.com>
…talled predicate (#1640) - writeStagedDownload is tested on both halves of its contract: the whole stream lands, and a stream that dies mid-download removes its partial file and rethrows. The dying stream serves 16 KiB first, so bytes are on disk when it fails; a stream that failed on its first read would pass against a version that never deleted anything. - Both install paths are pinned to stageAndInstall by a source test (comments stripped, since the KDoc names atomicMoveFrom on purpose). Reverting a call site to an inline atomicMoveFrom kept every behavioural test green; now it fails one. - The resident refusal no longer deletes the download when the download IS the destination. stageAndInstall models that case explicitly, and in it the unconditional delete removed the installed jar - in the one refusal that exists because something is still running from that file. - DynamicPluginManager.isInstalled is renamed hasEntry rather than deleted. It still had five production callers (PluginRemoval x3, DependentRestartPrompt, and the manager's own uninstall-cancelled log), so deleting it would have broken plugin removal. The name was the hazard: an entry survives a rejected, deleted jar, which is the reading #563 proved wrong. Its KDoc now says so and points at installedAndOnDisk. - The KDoc displacement was already in order on the merged code; nothing to change. Mutation-checked: each guard's removal fails exactly one named test. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: fluck-boss <fluck-boss@users.noreply.github.com>
The Toolbox Setup Wizard used the manager's entry-existence check to skip selected plugins. A rejected plugin can leave a DISABLED entry after its jar is deleted, so reopening the wizard counted it as installed without attempting installation. The skip guard now uses the shared
PluginDependencyResolution.installedAndOnDiskpredicate.A loaded plugin still counts when its recorded jar path is stale. An access-disabled plugin with its jar intact also counts. Missing or binary-incompatible disabled plugins are retried.
The review follow-up also checks the results of both Store and GitHub installs before recording success: registration-time binary incompatibility returns a successful Result containing a DISABLED plugin, which must instead reach the failure list. Access-disabled installations remain successful. This fixes result reporting without changing the manager's public API or the wizard's navigation.
Contributors
5bd71037e2864690e89c74dec6c77032e11debcf.3f15e9c), promotion inside the failure boundary with a per-path revert and the refusal widened to the whole jar-plus-sidecar footprint (3d7f888), and a test-call fix (f77c177).The draft blocker, and how it was retired
This was left in draft over artifact preservation: a DISABLED plugin with a stale recorded path could reach the promote-then-load-failure cleanup and lose a persisted artifact. The fix was to stop needing that cleanup to be clever, in two steps.
3f15e9crefuses an occupied destination instead of replacing it and rolling back. Every rollback fix had revealed another way to lose the artifact it was protecting; refusing has no tail, because nothing is overwritten, so nothing needs undoing.3d7f888makes promotion all-or-nothing, which was @kshivang's last blocking finding: moving the jar and then its sidecar is two moves, and they sat outside the failure boundary.moveTonow records each path as it lands and deletes exactly those on failure, and the refusal covers the whole footprint, sidecars included.Together: every path this call writes was established free beforehand, so a load failure after promotion can only delete what this call itself published. A resident artifact, however stale its recorded path, is never at a path this call writes to, so it cannot be the thing deleted.
3d7f888andf77c177have not been reviewed yet. The last review was of3f15e9c. That is the one thing this PR is waiting on.Validation
Tested on
devas of 2026-09-24, merged in as8fcf9a69(198 commits; a merge rather than a rebase so every SHA above stays valid; no conflicts)../gradlew :composeApp:desktopTest: 6372 tests, 0 failures, 5 skipped. This PR's ownWizardInstalledPredicateTest11/11 andWizardStagedInstallTest14/14, run fresh../gradlew detekt ktlintCheck --continue: clean across all modules.Not tested
The tests exercise the shared predicates, the install-result classification and the staging with real files. No live wizard run has been done: reopen the Toolbox Setup Wizard with a rejected or deleted plugin selected, and it should attempt the install rather than report it already installed; a still-incompatible download should appear as a failure; loaded plugins with stale recorded paths and access-disabled plugins with intact jars should still be skipped.
🤖 Generated with Claude Code