fix(upm): commit a module update only after the next start shows it loaded (#505, #513, #518, #517) - #561
Open
wisdommen wants to merge 91 commits into
Open
fix(upm): commit a module update only after the next start shows it loaded (#505, #513, #518, #517)#561wisdommen wants to merge 91 commits into
wisdommen wants to merge 91 commits into
Conversation
…on observation - ModuleUpdateTransactionTest: staging leaves the modules folder unchanged and writes one PENDING record; the next start applies before loading and commits only after the module is observed at the new version, loaded from the new JAR - ModuleUpdateStartupOrderTest: the start-up class path is computed after the apply - ModuleUpdateFixtures: module JARs built with JarOutputStream, loaded-module stand-ins Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…served /upm update no longer writes into the live modules folder or deletes the old JAR (whose ignored delete() result was the reported defect). It downloads the new JAR into upm-transactions/, a sibling of the modules folder, and writes a PENDING record atomically. The reply says the update takes effect at the next start and is kept only if the new version actually loads. At the next start, before the module class loader is built, the old JAR is moved aside and the new one put in place (APPLIED). After the modules load, the update is committed only when the module is loaded from the new JAR at the new version. No pre-restart prediction of loadability is made anywhere. Record file names are confined to their folder and canonicalised; the working folder is derived from a hash of the record's type and key. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ding rule, replies - ModuleUpdateRecoveryTest: a module absent after the start is rolled back with one line naming both versions; a rollback blocked by a held-open JAR finishes at the next start; an old JAR that cannot be moved aside (injected, and a read-only folder on POSIX) or a new JAR that cannot be moved in changes nothing and is reported SEVERE and kept FAILED; the next /upm update reports it; a file at the new name is never replaced; the five crash windows are reproduced by crashing the real code at named points; records naming ../ or a link leaving the folder are refused - ModuleUpdateStagingTest: staging failures leave nothing behind; a second update while one is staged changes nothing; an uninstall cancels a staged update - ModuleUpdateCommandTest: replies say staged and next start, never updated; update-all reports per module; a refused uninstall cancels nothing - ModuleFileCatalogueTest: every line is in en.json and zh.json with the same arguments - ModuleUpdateStartupOrderTest: a module load that throws counts as not loaded Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… call Mockito reports UnfinishedStubbing when a mock is created inside another stubbing call; the command tests now create the result first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…r every crash window - After the modules load, an update not observed loaded from the new JAR at the new version is rolled back: the new JAR (recognised by the SHA-256 recorded at staging, never by name) is removed and the old JAR restored, with one line naming both versions. A rollback that cannot remove the new JAR now is recorded ROLLING_BACK and finished at the next start, before modules load. - A move that fails at apply leaves the modules folder as it was (the old JAR is moved back if it had been moved), is logged SEVERE with the file and the error, and is kept FAILED; the next /upm update of the module reports it before staging again. A file already at the new JAR's name is never replaced. - An APPLIED record found at a start means the previous start ended before deciding; nothing was observed, so it is rolled back before loading. COMMITTING and ROLLING_BACK records are finished. Stale temporary records and staging-only orphan folders are cleaned; a folder holding a kept old JAR without a record is reported, never deleted. - One staged update per module: a second /upm update names the staged version and changes nothing. /upm update all stages each module independently and reports each. - An uninstall that goes ahead cancels the module's waiting update; a refused one cancels nothing. - FEATURES.md / UAT-CHECKLIST.md: update and update-all rows rewritten; new rows for the already-staged refusal, the start-up apply, a failed apply, an unconfirmed apply, the commit and the rollback; the install-version row no longer describes the old update. - COMPATIBILITY.md: /upm update takes effect at the next start and is committed only after the module is observed loaded; measured consumers: none. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e next start - ModuleRemovalDeferredTest: with a modules folder that is not writable, the uninstall raises RemovalDeferredException naming the JAR and why, and writes a REMOVE record; the next start deletes it before loading and says so; a deletion that still fails is reported SEVERE and kept; a JAR replaced since the uninstall is never deleted - ModuleRemovalDeferredCommandTest: the reply says the JAR is held open or not writable and is deleted at the next start before modules load, never "delete it by hand" Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…he next start On Windows the shared module class loader keeps every module JAR open while the server runs, so /upm uninstall of a loaded module could never delete its JAR and told the operator to delete it by hand, which the same lock prevents. - The uninstall records the JARs it could not delete (name, size, SHA-256) as a REMOVE record beside the modules folder and raises RemovalDeferredException, a FileSystemException that still names every file (getFile plus one suppressed per further file). Only a record that cannot be written leaves the plain failure. - The next start deletes each recorded JAR before the module class loader is built, only if it is still the recorded file; one that still cannot be deleted is reported SEVERE and kept; one replaced since is left alone with a WARNING. - The reply says the JAR is held open or not writable and is deleted at the next start before modules load. - FEATURES.md / UAT-CHECKLIST.md: uninstall row amended; new deferred-uninstall and start-up deletion rows. COMPATIBILITY.md entry. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… caller ModulesFolderSingleSourceTest fails on each second computation of "data folder plus plugins" under src/main/java (eight today, including PluginManager#init reading user.dir), and when the class loader, the start-up scan, install, update, uninstall or the transaction folder does not call ModuleFileTransactions#modulesFolder. A control proves the pattern finds each spelling. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Premise corrected by measurement (origin/alpha dfe71e0): the module class loader's URLs already came from the data folder (UltiTools#getModuleUrls), as did install, update and uninstall; only PluginManager#init's scan read user.dir. The class loader decides which classes can load at all and the data folder follows Bukkit's plugin directory, so the data folder is the authority. - ModuleFileTransactions#modulesFolder(dataFolder) is the one computation; PluginManager#init, UltiTools#getModuleUrls and #initPluginModules, install, install-version, update staging, uninstall, the uninstall replies and the transaction folder all call it. user.dir stays where it means the server root. - COMPATIBILITY.md entry. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…versions With the transactions folder not writable, the commit cleanup still deleted the kept old JAR although COMMITTING was never recorded; the next start, reading APPLIED, would then remove the new JAR as unconfirmed and leave no version at all. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…n disk If COMMITTING cannot be written, the commit stops before any cleanup; the record still says APPLIED, so the next start restores the old version as unconfirmed instead of finding neither version. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… it again An uninstall naming the module by a declared name different from its runtime name does not match the staged record, and the start moved the new JAR in anyway. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…older At apply, an old JAR that is neither in the modules folder nor kept aside means the module was removed after staging (an uninstall by any name, or by hand); the update is abandoned with one WARNING line instead of installing the module again. The uninstall still cancels by runtime name; this closes the other names without a second matcher. FEATURES.md apply row amended. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ed module back Gate-1 review findings (both reviewers): cancellation matched only the runtime name; an update ran even when its old JAR was recorded for deletion and that deletion failed again; recorded deletions ran before updates only by file-name sorting; an install of the same bytes was deleted by an older recorded deletion; the recorded size was never checked. - ModuleUpdateUninstallTest: cancel through the removed JAR's name; an update whose old JAR is pending deletion is abandoned; an install cancels the recorded deletion of its file name; a copy-back with another timestamp is kept; removals run first - command tests: cancellation receives the removed or deferred JAR names; the deferred reply says a start that still cannot delete reports it in its log Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…d module back Gate-1 review, swept as one class (every point where an uninstall and a staged update of the same module meet): - Recorded deletions run before updates by rule, in their own pass, not by how record names sort; an update whose old JAR is gone or still pending deletion is abandoned. - The uninstall cancels an update staged under the name given, or one that would replace a JAR it removed or recorded for removal. - A recorded deletion applies only while the file has the recorded size, modification time and SHA-256, and an install through /upm of that file name forgets it. - The deferred-deletion reply no longer promises the next start succeeds: it says a start that still cannot delete the JAR reports that in its log. - FEATURES.md / UAT-CHECKLIST.md rows amended. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… leftover kept JAR Gate-1 review: the static lock was held for the whole download, so an uninstall on the main thread (cancel, record a deferred deletion) waited for it; and a working folder left with a kept old JAR but no record was reused, the kept JAR taken as this transaction's backup. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…wnload - stageUpdate takes the lock for the record checks, the leftover check and the module lookup, marks the module as downloading, and releases it; the catalogue lookups and the download run outside it; the record write takes it again. An uninstall on the main thread (cancel, record a deferred deletion) no longer waits for a download. - A second /upm update of a module being downloaded is refused as busy. - A working folder holding a kept old JAR without a record is never adopted as this transaction's backup: staging is refused and names the folder. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ds; no kept JAR, no deletion Gate-1 review: confinement canonicalised the folder too, so a derived folder that is a link passed; names were not required to be JARs; a record missing a field crashed the observation; and a rollback deleted the file whose hash matched even with no kept old JAR to put back. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…; never roll back without a kept JAR Gate-1 review: - The transaction's own working folder and its backup/staged folders must not be symbolic links; confinement canonicalised the folder as well, so a linked folder passed. Such a record is refused. - Every name a record carries must be a JAR name, so a record can never move a file of another kind into the modules folder under a .jar name. - A record missing a field its type needs is refused when read, and one transaction's unexpected failure during the observation is reported instead of ending the start (and so never hides a load failure). - A rollback removes the new JAR only while the kept old JAR is there to put back; without it the new JAR stays and a WARNING names the missing file. A rollback an earlier start already completed is cleaned up without a second line. - The class javadoc no longer claims the modules folder never changes while the server runs: a rollback right after loading does change it. - FEATURES.md apply and observe rows amended. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ng normaliser Gate-1 review (info): ModuleFileTransactions re-implemented streaming SHA-256, string SHA-256 and hex encoding (ResourceHashSidecar has them) and copied PluginInstallUtils#normalizeIdentifyString line for line; two normalisers drifting would stop a staged update from ever matching its loaded module. No behaviour change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Gate-1 review: the unconfirmed-apply row asked for a kill -9 inside a window that is usually under a second; it now uses a fixture whose registerSelf() waits 60 seconds. The rows named an internal fixture plan; they now say how to build each fixture. The staged file name is the lower-cased identify-string, which the rows now say. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…/ on another file system fails truthfully - The records folder is <server root>/.ultikits/upm-transactions, beside the credential store and outside plugins/, resolved from the data folder's grandparent (a relative data folder resolves the same server root). - A modules folder on another file system: the atomic move is refused, nothing in the modules folder changes, the record is FAILED, one SEVERE line names both folders with its own key, and no copy is made; the next /upm update reports it. - Every transaction test now uses a server layout (<root>/plugins/UltiTools), so no test writes records outside its own temporary folder. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…lugins/ on another file system truthfully Maintainer decision (2026-09-30, follow-up 18; closes gate-1 finding A-01): the pending-update and pending-removal records move out of plugins/UltiTools/ -- which the panel's file interface can write when its file writing is enabled -- to <server root>/.ultikits/upm-transactions, beside the credential store. The panel file interface is unchanged. - ModuleFileTransactions#transactionsFolder resolves the server root the way CredentialStore does: the data folder's grandparent, from the absolute data folder, never user.dir. - The start-up swap stays an atomic rename. When plugins/ is on another file system the rename is refused (AtomicMoveNotSupportedException, no longer wrapped): nothing in the modules folder changes, the record is FAILED, and one SEVERE line with its own catalogue key names both folders. No copy fallback: a copy is not atomic and the crash windows rely on renames. - FEATURES/UAT rows and COMPATIBILITY name .ultikits/upm-transactions/; a new UAT row exercises the cross-file-system failure with a tmpfs-backed modules folder. - The unrecordable-decision test reads the file store of the server root (the records folder does not exist before the first record). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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. |
5 tasks done
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 486 |
🟢 Coverage 83.95% diff coverage · +0.37% coverage variation
Metric Results Coverage variation ✅ +0.37% coverage variation (-1.00%) Diff coverage ✅ 83.95% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (dfe71e0) 15352 12560 81.81% Head commit (2ed6dd7) 16144 (+792) 13267 (+707) 82.18% (+0.37%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#561) 835 701 83.95% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: 6a2386abe6
ℹ️ 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".
…iour - NPath: stageUpdate (2880) splits into reserve() and locate() behind a small Reservation value; download (324) moves the fetch and the record write into fetch() and writeStaged(); applyUpdate (216) moves the missing-staged-JAR and abandon branches into their own methods; applyRemoval (336) moves the "still the recorded file" test into isStillRecorded(). Same checks, same order, same replies. - Path construction: the four sites Codacy names are the helpers every record path goes through (a hashed id, a constant, or a name confined() has refused unless it is a plain JAR name directly in its folder); suppressed with the rationale ahead of the marker. Test sites likewise (a constant language, a listing of the test's own folder). - Tests: owner-only POSIX permissions for the read-only folders; fields before the nested crash class; UltiTools imports UltiToolsPlugin and Supplier instead of qualifying them. - The staging "nothing changed" snapshot now covers the server root, so it still includes the records folder after the move under .ultikits. Local PMD (NPathComplexity, FieldDeclarationsShouldBeAtStartOfClass, UnnecessaryFullyQualifiedName) and Opengrep (path traversal, file permission rules) report nothing in this pull request's changed code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…d JAR replaced after staging Codex review on #561, round 1: - P1: a JAR two loaded modules came from is refused at staging, before any download, naming the other module; nothing changes. - P2: an old JAR replaced under the same name after staging (an install of the current version, a hand-made hotfix) is neither moved aside nor deleted by the commit: the update is abandoned with one WARNING and the replacement stays. The same bytes put back still count as the old JAR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…staging The P2 fix records the old JAR's SHA-256 at staging; when it cannot be read, staging refuses (named reason) rather than writing a record that could not verify the file at the next start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…und 14) ultitools.boot.module-update-apply: the new JAR is returned by its recorded hash before the kept old JAR goes back, and an undo that cannot finish is finished in the same order by the next start or the next /upm update. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Member
Author
Codex round 14 — the recovery invariant, swept by defect class (17-44 follow-up 19)Invariant (class javadoc of Moves and writes that can be interrupted. Apply: old → kept ( Placements: old in Modules / Kept aside / Gone; new Staged / in Modules / Gone. Reachable combinations
|
| Old | New | Before the fix | After |
|---|---|---|---|
| M | S / G | holds | holds |
| K | S / G | holds (old back) | holds |
| K | M | violated: the old JAR moved back beside the new one, then the record and work folder deleted — both JARs, no record left; same name: refused as occupied, stuck | holds (undoApply) |
Violating rows before the fix: 2 (FAILED / kept / modules at a start, and at /upm update's discard), each in both file-name
variants: 3 of 86 test cases failed the one-JAR check, 6 of 86 with the assertion that a FAILED record ends with the old version in place.
Fixed by one helper, undoApply (the undo failApply already ran: returnStagedJar by hash, then restoreBackup), now used by
failApply, restoreAfterFailure and discardFailed. No second recovery path.
Combinations no crash or failure can leave (why)
| State | Placements | Why unreachable | If made by hand |
|---|---|---|---|
| PENDING | M/M | the new JAR moves in only after the old JAR moved aside | — |
| PENDING | G/M, G/G | the new JAR moves in only after the old is kept; the staged JAR is removed only by hand | G/G: the operator removed the module and the staged JAR; FAILED with no JAR (the removal exception) |
| APPLIED | any with new staged; M/M; G/any | APPLIED is written after both moves; after it only a rollback moves files, removing the new JAR first; nothing deletes the kept JAR before a commit |
— |
| COMMITTING | M/any; K/S, K/G, G/S, G/G | after the decision the old JAR never returns and the new JAR never leaves the modules folder | K/G, G/G: the committed new JAR deleted by hand; the commit deletes the kept JAR and the module is gone |
| ROLLING_BACK | M/M; any with new staged; G/any | a rollback removes the new JAR before the old returns, never stages it, never deletes the kept JAR | — |
| FAILED | M/M; G/any | the undo returns the new JAR before the old; an apply never deletes the old JAR | M/M: the operator placed a copy of the new version in the modules folder (a duplicate copy, see B-I-03) |
| (no record) | a work folder left | the record is written last at staging and deleted last everywhere else | an orphan work folder is deleted at start, or reported if it holds a kept JAR |
中文摘要：不变量——任何一次启动之后，对每条事务记录，模块目录中该模块恰好有一个 JAR（旧或新，不会两个都在，除移除外也不会一个都没有）；不在模块目录中的 JAR 只在暂存位置或旧版本保留位置，按记录的哈希识别。修复前违反的行：2 行（FAILED / 旧版本保留 / 新版本在模块目录，分别在启动时和 /upm update 丢弃记录时），由 undoApply 一处修复（fda50c27）。
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…-assertion suppressions Codacy reported 6 new issues on 8874522; every site of both rules in the two files is handled in one pass (Codacy names one instance per rule per file): - Opengrep SpotbugsPathTraversalAbsolute: returnStagedJar's staged path (the same confined() construction backupOf already marks) and the test's paths built from its own staging's record; a nosemgrep marker directly above each, with the reason first. - PMD JUnitTestsShouldIncludeAssert on the two parameterised tests, whose assertions live in Scenario#assertInvariant / #assertOldVersionInPlace. No behaviour change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb05fd3b85
ℹ️ 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".
… names in cancellation (Codex round 16) Codex round 16 on cb05fd3, two P2s in code predating this follow-up, outside the recovery state machine: - /upm update snapshotted the loaded modules before staging took its lock; an uninstall completing in that gap found no download marker and no record, and the stale snapshot staged the removed module's update. The test lets an uninstall's cancellation arrive while staging reads the loaded modules: the download must end cancelled and no record be written. - The uninstall's cancellation matched code-source JAR names outside the modules folder; a module loaded from an external JAR cancelled the update of an unrelated module JAR with the same file name. RED: compilation error (stageUpdate has no loaded-modules supplier form); the second case is shown behaviourally after the fix. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ules-folder JAR names only (Codex round 16) Codex round 16 on cb05fd3, two P2s in code predating this follow-up and outside the recovery state machine, fixed as ordinary defects: 1. PluginInstallUtils.stageUpdate snapshotted the plugin list before ModuleFileTransactions took its lock. An uninstall completing in between (unload, then its cancellation under the lock) found neither a download marker nor a record; the stale snapshot then staged the removed module's update, and where the uninstall could not delete or record the old JAR the next start applied it. stageUpdate now has a form taking a supplier, called once under the lock; the command passes one reading the live list. The uninstall removes the module from the list before its cancellation takes the lock, so either staging sees the module gone (refused as not loaded) or the download is marked running before the cancellation looks (cancelled). The List forms delegate to it. Consumers: /upm update and /upm update all, both through PluginInstallUtils.stageUpdate. 2. The uninstall's resolved identity carried the file names of every target code-source JAR, including ones outside the modules folder, and a cancellation matches an update's old JAR by file name. Only names of JARs directly in the modules folder are kept; an update's old JAR is always there. Other JAR-name sources were already confined to the modules folder (the loader's index, the entries the uninstall identified). GREEN: UltiTools-Reborn-505-codex-r16-GREEN.log (329 related tests, 0 failures). Behavioural RED with the list read before the lock and the external names kept: 2 of 2 fail (-codex-r16-RED-behavioural.log). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 811 |
🟢 Coverage 86.61% diff coverage · +0.61% coverage variation
Metric Results Coverage variation ✅ +0.61% coverage variation (-1.00%) Diff coverage ✅ 86.61% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (dfe71e0) 15352 12560 81.81% Head commit (aa8ca8b) 16412 (+1060) 13527 (+967) 82.42% (+0.61%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#561) 1120 970 86.61% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f80dd63924
ℹ️ 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".
…p the update (Codex round 17) Codex round 17 on f80dd63 (P2, in the B-I-03 copy check this follow-up added): the start-up builds the module class path only from JARs passing SecurityPolicy.isValidModuleJar (UltiTools#collectModuleJarUrls) and scans only those (PluginManager#loadPluginMainClass), so a copy failing it -- here one over the entry limit, readable and declaring the module's main class, sorting first -- can never load first; refusing the update because of it is a false refusal. RED: 1 of 18 fails (UltiTools-Reborn-505-codex-r17-RED.log). Also Codacy on f80dd63: the round-16 test's fully qualified catchThrowable is the static import the file already has. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ut it on the class path (Codex round 17) Codex round 17 on f80dd63, P2, in the B-I-03 copy check: a readable JAR declaring the module's main class and sorting first, but failing SecurityPolicy.isValidModuleJar (over the size or entry limit), refused the update, although the start-up never puts such a JAR on the module class path (UltiTools#collectModuleJarUrls) nor scans it (PluginManager#loadPluginMainClass), so it cannot load first. The check now applies the same validation, in a helper (loadsAhead) so copiesLoadingFirst's complexity does not grow. FEATURES ultitools.upm.update says so. This corrects the reasoning recorded while B-I-03 was designed, that every .jar file in the folder is on the class path; the class path is built from the validated JARs only. GREEN: UltiTools-Reborn-505-codex-r17-GREEN.log (330 related tests, 0 failures); RED before the fix: 1 of 18 (-codex-r17-RED.log). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88472f91ac
ℹ️ 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".
…nd replaced by uninstall (Codex round 18) Codex round 18 on 88472f9 (P2, pre-existing #518 code): a removal record that parses but whose removals list holds a null entry passes readRecord, and forgetDeferredRemoval dereferenced each entry, so /upm install failed after its JAR was written and later records were not cleared. Swept over every consumer of removal entries outside start-up (which already refuses such a record through problemWith): recordDeferredRemoval merges an existing record's entries and dereferences them the same way. RED: both fail with a NullPointerException (UltiTools-Reborn-505-codex-r18-RED.log). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…g its entries (Codex round 18) Codex round 18 on 88472f9, P2, in pre-existing #518 code: a removal record that parses but fails start-up's validation -- a null entry, an entry without a name or hash, a wrong file name -- was used directly by the two operations that read removal records while the server runs, and a null entry threw a NullPointerException out of them. Both now apply problemWith, the validation start-up already applies, in one helper each (so the callers' complexity does not grow): - forgetDeferredRemoval (install): removalRecordIn skips a record that cannot be read or carried out with a warning naming it; the others are still cleared. - recordDeferredRemoval (uninstall): mergeableRemovals merges nothing from an earlier record of the module that cannot be read or carried out; the new record replaces it, with a warning naming it. Start-up (applyBeforeLoad) already refused such a record; there is no other consumer of removal entries. GREEN: UltiTools-Reborn-505-codex-r18-GREEN.log (332 related tests, 0 failures); RED before the fix: 2 of 2 NullPointerException (-codex-r18-RED.log). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00d80764f9
ℹ️ 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".
… the foreign-file dimension (Codex round 19) Codex round 19 on 00d8076: two P2s in the commit and rollback of the update recovery, both a file at the new JAR's name that is no longer the staged JAR. Coordinator's decision: close the whole category with one rule rather than case by case -- the transaction only ever moves, replaces or deletes a file whose SHA-256 matches the record -- and add the table dimension. The invariant javadoc states the rule. A new parameterised family puts a foreign JAR at each of the four locations a record uses (the old JAR's name, the kept-old location, the staged location, the new JAR's name) for each of the 19 reachable rows, new version loading or not (152 cases), and asserts that the start never moves or deletes the foreign file, never deletes the last identified copy of the module, never moves an identified JAR into the modules folder beside it, and leaves nothing a later start still acts on. The 86 existing cases are unchanged. RED: 75 of 152 cases, 38 of the 76 new (row, location) rows, including both round-19 cases (COMMITTING and ROLLING_BACK, kept / modules, foreign file at the new JAR's name); 0 of the 86 existing cases. Log: UltiTools-Reborn-505-codex-r19-foreign-RED.log. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ecord; otherwise NEEDS_OPERATOR (Codex round 19) Codex round 19 on 00d8076, two P2s in commit and rollback: a file at the new JAR's name that is no longer the staged JAR was committed to (the kept old JAR deleted) or had the old JAR restored beside it. Coordinator's decision: close the category -- another actor changing files while a transaction is pending -- with one rule instead of case by case, on the maintainer's precedent for ambiguous identity (write nothing, warn). The rule: the transaction only ever moves, replaces or deletes a file whose SHA-256 matches the record (the staged new JAR or the kept old JAR), and only into a place that is free. One guard, requireRecorded, runs before every destructive step with the files that step touches, source and target: - the apply's swap (old name, kept-old location, staged JAR, new name); - undoing a failed apply, shared by failApply, later starts and /upm update's discard (undoApply); - the commit (kept old JAR, staged location, and the new JAR it commits to, which must still be the staged one); - the rollback (new name, kept old JAR, old name, staged location); - the uninstall's cancellation of a staged update (kept old, staged). When a file is not the recorded one, nothing is done to any file: the record is kept in a new state, NEEDS_OPERATOR, which later starts do not act on (they repeat the SEVERE line); one SEVERE line (new en/zh key NEEDS_OPERATOR) names each unexpected file with its expected and actual hash and says to resolve it by checking the files and deleting the record and its folder; /upm update of the module refuses with REASON_NEEDS_OPERATOR, and /upm uninstall refuses before unloading anything (ModuleFileTransactions#heldForOperator). Behaviour changes, tests updated: a different file under the old JAR's name, a changed staged JAR, or a file already at the new JAR's name used to abandon the update or fail the apply; each now holds the record for the operator. The UPDATE_ABANDONED_REPLACED key and its branch are gone (the guard replaces them); an old JAR that is gone (an uninstall) still abandons the update, as no file is foreign. A changed staged JAR after a crash that left the old JAR kept now leaves it kept (named in the SEVERE line) instead of moving it back: the rule does nothing to any file. Tests: the 86 existing invariant cases are unchanged and green; the 152 foreign-file cases (38 of 76 rows RED before) are green and assert the SEVERE line when held; /upm update and /upm uninstall refusal tests added. Guard off: the same 75 of 238 fail (-codex-r19-foreign-RED-behavioural.log). Related suites: 486 tests, 0 failures (-codex-r19-foreign-GREEN.log). Also Codacy on 00d8076: the two fully qualified names in ModuleUpdateUninstallTest. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… NEEDS_OPERATOR (Codex round 19) ultitools.boot.module-update-apply: the transaction only moves, replaces or deletes a file whose SHA-256 matches the record; otherwise nothing is done, the record is kept as NEEDS_OPERATOR with one SEVERE line naming each file and how to resolve it (check the files, delete the record and its folder), later starts repeat the line only, and /upm update and /upm uninstall refuse. The two sentences it replaces (a replaced old JAR abandons the update; a changed staged JAR fails the apply) described the previous behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ultitools.boot.module-update-apply.neg-needs-operator: a copy of the old JAR under the new JAR's name before the first start after staging; the start moves nothing and logs one SEVERE line naming the file with its expected and found hash; the record is kept as NEEDS_OPERATOR; /upm update and /upm uninstall of the module refuse; the next start repeats the line only. Every quoted literal matches the en.json value and the refusal texts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Member
Author
Codex round 19 — the foreign-file dimension of the recovery table (17-44 follow-up 19)Rule (coordinator decision; class javadoc of Dimension. A foreign JAR (another hash) at each of the four locations a record uses — Old name, Kept-old location, Staged RED before the guard: 38 of the 76 (row, location) rows (75 of 152 cases); 0 of the 86 existing cases. All 76 rows green after it;
Measured per case after the guard (both "new loads" variants; one cell differs between them). "held" = the start left every file as it was and kept the record as Threat model (maintainer decision, 2026-10-01). Another actor changing files mid-transaction is outside PR #561's threat model; the 中文摘要：交易只移动、替换或删除 SHA-256 与记录一致的文件；每个破坏性步骤前由同一个守卫 |
Member
Author
|
Local Codex review — run 1 (maintainer's local-review rule, 2026-10-01)
本地 Codex 复审第 1 次（ |
…) in the round-19 staging test Two calls in ModuleUpdateStagingTest qualified a fixture method the file already imports statically (UnnecessaryFullyQualifiedName). Every test file this branch touched was swept for the same pattern; these were the only two. No behaviour change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request targets
alphadeliberately (integration branch for 6.3.0), not the defaultmain./upm updateno longer replaces a module's JAR while the server runs and no longer reports "Update successful" for an update that did not happen: it stages the new JAR, the next start swaps the files before any module class loads, and the update is kept only when that start shows the module actually loaded from the new JAR at the new version. An uninstall JAR that cannot be deleted now (Windows holds it open) is deleted at the next start, and the modules folder is computed in one place. Plan 17-44 adds module identity on disk: module JARs are discovered in file-name order for both the scan and the class loader (#476); the loader records which JARs declare each main class,/upm uninstalldeletes every one of them (#516), and one start-up warning names every duplicate copy (UltiTools-Dev-Doc#96); an uninstall cancels the module's staged update by the identity it resolved, on every outcome (Codex round 10).本 PR 有意指向
alpha（6.3.0 集成分支）。/upm update不再在服务器运行时替换模块 JAR，也不再对没有发生的更新报告「更新成功」：命令只暂存新 JAR，下次启动在加载任何模块类之前换文件，只有这次启动确实从新 JAR 以新版本加载了该模块，更新才保留，否则恢复旧版本。卸载时删不掉的 JAR（Windows 占用）改为下次启动再删；模块目录只在一处计算。计划 17-44 补上磁盘上的模块身份：扫描与类加载器都按文件名顺序发现模块 JAR（#476）；加载器记录每个主类由哪些 JAR 声明，/upm uninstall删除其中每一个（#516）；重复副本只在启动时输出一条列出所有副本的警告（UltiTools-Dev-Doc#96）；卸载在任何结果下都按解析出的模块身份取消暂存的更新（Codex 第 10 轮）。Threat model
This pull request defends the module-file transactions against crashes, restarts, the operator's normal commands and panel actions (
/upm install,/upm update,/upm update all,/upm uninstall, restarts from the panel), and concurrency among those. Findings whose trigger lies outside that scope — a Java security manager denying file access, hand-edited framework-internal files, a third party changing the modules folder or the transaction folder while a transaction is pending, and the like — are not fixed in this pull request; they are tracked in its follow-up issue, #565. The foreign-file guard (d76b3d79: the transaction only moves, replaces or deletes files whose SHA-256 matches the record, otherwise it holds the record asNEEDS_OPERATOR) stays, because it is done and tested; it is not a claim that such scenarios are in scope. Review is run locally under the maintainer's 2026-10-01 rule (at most two local Codex runs; P1 always fixed, P2 fixed only inside this threat model).本 PR 的威胁模型：崩溃、重启、管理员的正常命令与面板操作，以及这些操作之间的并发。超出此范围的发现（安全管理器、手工修改框架内部文件、交易进行中第三方改动模块目录等）不在本 PR 修复，统一记录在后续 issue #565 中。外来文件守卫已完成并有测试，予以保留。
Issue closure
Closing keywords do not fire for pull requests into
alpha; these issues are closed by hand after the merge.Closes #505
Closes #513
Closes #518
Closes #517
Closes #516
Closes #476
Why observation, not prediction
Before this change,
/upm updatedownloaded the new JAR into the modules folder, calledFile#delete()on the old one while ignoring the result, and reported success. A failed delete left two versions to race at the next start (#505); a JAR that could not load was only discovered after the old one was gone (#513).PR #508 tried to close #505 by predicting, before the restart, whether a downloaded JAR would load: does it declare
name:, is the main class present, readable, concrete, named correctly, within the first 1,000 classes, with its superclass inside the JAR, and so on. Each fix exposed the next way the prediction differed from the real loader. Over 29 review rounds it collected 51 findings, 13 of which named the previous round's fix as their cause; replacing the hand-written class-header parser with the loader's own scan only moved the same divergence one level up (loader topology, parent-first ranges, URL order).On 2026-09-29 the maintainer decided to stop predicting and observe instead: the next start shows whether the module loaded. This pull request implements that. No code in it loads, scans or parses the classes of a downloaded JAR; staging reads only its
plugin.yml(identity and version) and applies the existing size/entry guard. PR #508 is closed in favour of this one; its branch is kept for its history.What changes
ModuleFileTransactions(@ApiStatus.Internal)./upm updatedownloads into<server root>/.ultikits/upm-transactions/, writes aPENDINGrecord atomically, and replies "staged, takes effect at the next start". At the next start, beforenew URLClassLoader(...), the old JAR is moved aside (kept) and the new one moved in (APPLIED). AfterPluginManager#initreturns — or throws, which counts as not loaded — the update commits only if the module is loaded from the new JAR at the new version; otherwise the old JAR is restored, the new one removed, and one line names both versions.FAILED, and the next/upm updateof that module reports it first. One pending update per module; update-all stages each module independently; an uninstall cancels that module's staged update.APPLIED; after the commit or rollback decision). AnAPPLIEDrecord found at a start (the previous start ended before deciding) is rolled back before loading..jarfile name, resolved and canonicalised against the one folder each belongs to;.., separators, links leaving the folder, symbolic-link working folders and missing fields make the record refused with a warning.<server root>/.ultikits/upm-transactions/, beside the credential store, found as the data folder's grandparent — not underplugins/UltiTools/, which the panel's file interface can write when file writing is enabled (a forged removal record could have deleted a module JAR). The panel file interface is unchanged. The start-up swap stays an atomic rename: whenplugins/is on another file system the rename is refused, nothing in the modules folder changes, the record isFAILED, and one SEVERE line names both folders. There is deliberately no copy fallback./upm uninstallcannot delete a JAR, the deletion is recorded (with size, modification time and SHA-256) and the reply says it will be deleted at the next start; that start deletes it before any module loads, only if the file is unchanged, and logs it. An install through/upmof the same file name forgets the recorded deletion.ModuleFileTransactions#modulesFolder(data folder +plugins) is the one computation; the class loader, the start-up scan (which readuser.dir), install, update and uninstall all use it. The issue's premise was corrected by measurement: only the scan readuser.dir.ModuleFileTransactions#moduleJarslists the modules folder once, sorted by file name (plainString#compareTo); the start-up scan (PluginManager#discoverModuleClasses) and the module class loader (UltiTools#collectModuleJarUrls) both read it, and the uninstall's listing andfindPluginJaruse the same order. Before, both tookFile#listFiles()order, which is hash order on ext4.plugin.ymlmain:class, every JAR that declares it and the JAR the class loaded from (ModuleJarIndex,@ApiStatus.Internal) — the loader's own read, not a second scan./upm uninstalltreats an entry whoseplugin.ymlmain:names a target's main class as the module's, whatevername:it declares, so a renamed second copy is deleted and named; a main class another loaded module has is excluded; a file is judged by what it declares at uninstall time. No class is read out of any archive.en.json/zh.json) naming every copy and the JAR the classes load from. A copy whose main class loads from another JAR in the modules folder that declares the samemain:is still refused, now reported by that warning instead of a per-copy SEVERE line; a class borrowed from a JAR that does not declare it keeps its SEVERE refusal. The legacy-order log line and javadoc now say file-name order./upm uninstallreports the cancelled versions on every exit.ModuleFileTransactions#cancelStagedUpdates(RemovedModule)replaces the name-and-JAR form (which never shipped)./upm updaterefuses when another JAR in the modules folder declares the module'smain:(from the loader'sModuleJarIndex; the old JAR'splugin.ymlas fallback) and sorts before the new JAR's name — that copy would load instead and every update would roll back. Nothing is downloaded, recorded or touched; a dedicated en/zh reason names the new JAR and every such copy. Each copy is judged by its currentplugin.yml, so the retry after removal needs no restart; a copy that sorts after does not stop the update; the observation stays the backstop for a copy added after staging (ebe7e862,b3f8146b, rows02e3a546).Decisions applied
<server root>/.ultikits/upm-transactions/; do not change the panel file interface; aplugins/folder on another file system is reported honestly, never partially updated. A copy+verify+delete fallback was allowed only if it provably kept every crash window's guarantee; it was not added.plugin.ymland a loaded instance's code source; no class is read out of a candidate archive.Checklist rows amended
FEATURES.md:ultitools.upm.update,ultitools.upm.update-all,ultitools.upm.uninstall(deferred deletion),ultitools.boot.deferred-removal,ultitools.boot.module-update-apply,ultitools.boot.module-update-observe.FEATURES.mdalso states the shared-JAR refusal, the replaced-old-JAR abandon and the unlistable-folder report (unit-pinned).17-44:
FEATURES.mdultitools.upm.uninstall(main:-based identity, identity-based cancellation), newultitools.boot.duplicate-module-jars,ultitools.boot.plugin-load-order-legacy(file-name order), reconciliation note (nineeventrows).UAT-CHECKLIST.mdnew rowsultitools.upm.uninstall.second-copy,ultitools.upm.uninstall.cancels-staged-update,ultitools.boot.duplicate-module-jars,ultitools.boot.duplicate-module-jars.neg-none;ultitools.boot.plugin-load-order-legacyquotes the reworded line; every row relying on an enforcedchmodnow requires a non-root server process (gate 1). Every cited precondition row is earlier in the file. Follow-up 19:FEATURES.mdultitools.upm.update(the refusal), newUAT-CHECKLIST.mdrowultitools.upm.update.neg-copy-loads-first(beforeultitools.upm.update, whose staging then follows the copy's removal).17-43:
UAT-CHECKLIST.md, in file order:ultitools.upm.install-version,ultitools.upm.uninstall.deferred,ultitools.upm.update,ultitools.upm.update-all,ultitools.upm.update.neg-already-staged,ultitools.boot.deferred-removal,ultitools.boot.module-update-apply,ultitools.boot.module-update-apply.neg-apply-failed,ultitools.boot.module-update-apply.neg-cross-file-system(new: modules folder on a tmpfs),ultitools.boot.module-update-apply.neg-unconfirmed,ultitools.boot.module-update-observe,ultitools.boot.module-update-observe.neg-rolled-back. Every row names.ultikits/upm-transactions/; the existing/upm updateand/upm uninstallrows were swept for the eight checklist defect classes.Red-when-reverted evidence
Each behaviour has a
test(...)commit, then a fix commit. With only the fix reverted, the named tests fail:1bbc352b)ModuleUpdateTransactionTeste5fc3fcd)ModuleUpdateRecoveryTestf0c0a898)ModuleRemovalDeferredTesta2887ee5)ModulesFolderSingleSourceTestb6e77039)ModuleUpdateRecoveryTest$UnrecordableDecision51490842)ModuleUpdateRecoveryTestd381c13e)ModuleRemovalDeferredCommandTest,ModuleUpdateUninstallTestaf925ff7)ModuleUpdateRecoveryTest,ModuleUpdateStagingTestc9a0ab96)ModuleUpdateRecoveryTest$Confinement6a2386ab)ModuleUpdateTransactionTest,ModuleUpdateRecoveryTest,ModulesFolderSingleSourceTestd99731d4)ModuleUpdateStagingTest,ModuleUpdateRecoveryTestbe4760ef)ModuleUpdateRecoveryTest,ModuleUpdateStagingTest60847e7b)ModuleUpdateRecoveryTestcf10b491)ModuleUpdateRecoveryTestb293e223)ModuleUpdateRecoveryTest$UncheckedExceptionInOneTransaction4108101b)ModuleUpdateRecoveryTest$UncheckedExceptionPastTheApply5d019894)ModuleUpdateUninstallTestff837fe1)ModuleUpdateStagingTestbe31008e)ModuleUpdateStagingTestc708e793)ModuleUpdateStagingTest2ed6dd78)ModuleUpdateStagingTestfc19baa6)ModuleJarOrderTest,PluginManagerDiscoveryOrderTest7a0e819f)ModuleJarIndexUninstallTestmain:match disabled, 2 of 5 fail14deceb6)PluginManagerClassScanningTest3a833e93)UninstallCancelsByIdentityTestfc9407f8)UninstallCancelsByIdentityTest8bcf5057)ModuleUpdateCommandTestf56ee026)UninstallCancelsByIdentityTestb3f8146b)ModuleUpdateDuplicateCopyTestebc13975)ModuleUpdateStagingTestab93f344)ModuleUpdateDeniedRecordWriteTestfda50c27)ModuleUpdateRecoveryInvariantTest(86 cases)f80dd639)ModuleUpdateStagingTest,UninstallCancelsByIdentityTest88472f91)ModuleUpdateDuplicateCopyTestFull
mvn -B clean verify -DexcludedGroups=ataa8ca8b6: 6371 tests, 0 failures (at28fe0208: 6111); CJK scope gate 0 violations. (At2ed6dd78, before 17-44: 6091.) A #504 regression check (UninstallStrayEntriesRegressionTest: stray entries, a space in the server path, every archive closed) passes before and after, as expected — #504 was fixed onalphaby2fef89e6.Review
Gate 1: two independent reviewers plus an executor self-review raised 1 blocker, 11 warnings and 6 informational findings (distinct). All are fixed on this branch, each behavioural fix test-first with a revert proof. The blocker (records writable through the panel file interface) was closed in two steps:
.jar-only record names and no rollback without a kept JAR (c9a0ab96), then the maintainer's decision to move the records under<server root>/.ultikits/(6a2386ab). Round-trip check: one finding was caused by an earlier fix on this branch (a new "abandon" signal whose consumer, the order of removals before updates, was implicit); every meeting point of an uninstall and a staged update was listed and closed in one commit (d381c13e). No rule was revised twice.Gate 1, second section (17-44, whole branch): two independent read-only reviewers plus an executor self-review; blocker 0, warning 3, info 4. Fixed: the round-10 cancellation matched identify-strings and runtime names without the bystander exclusion (self-review and reviewer A, the same class; swept over every form the removed-module identity carries in
f56ee026), a cancelled update unreported when the uninstall ends in an unexpected exception (8bcf5057), a UAT row that loses the module's JAR on a root server (swept over everychmod-based row), a sort-order precondition, and stale "a module needs no plugin.yml" text. B-I-03 (an old duplicate copy that sorts first makes every update roll back) was first left unchanged and is now fixed per maintainer follow-up 19:/upm updaterefuses before download (table above).Gate 2 (third-party review)
2ebd65cband86185ce7(methods split, owner-only test permissions, justified suppressions at the four path helpers every record path goes through and at two test sites). Codacy is green frombe4760ef.6a2386ab): P1 shared JAR, P2 replaced old JAR — fixed ind99731d4.d99731d4): P2 unlistable records folder — fixed inbe4760ef, swept to the kept-JAR folder readers.be4760ef): P2 a denied listing in the temporary-record clean-up — a sweep miss of round 2, fixed at every listing in60847e7b.60847e7b): two P2 threads. Per-transaction containment is closed by changing path — one boundary catch per transaction routed into the existing apply-failure path (b293e223) instead of a third per-site guard; the staged JAR is now checked against its recorded SHA-256 before anything moves (cf10b491).15cef5de): a malformed record's uncheckedJsonParseExceptionescaped the install path — fixed in5d019894(one place:readRecord; both deferred-removal paths skip an unreadable record with a warning).4108101b): a download declaring a version other than the catalogue's latest was accepted — fixed inff837fe1.ff837fe1): an incomplete record was reported as "already staged" — fixed inbe31008e(one shared validation).be31008e): P1 — the shared-JAR check exempted instances with the same identify-string, which is the real shared-archive case (identify-string comes from the archive'splugin.yml); a missed condition of the round-1 fix, fixed inc708e793(only the selected instance is exempt), with a sweep of every identify-string/name/version comparison listed in the commit.c708e793): an uninstall during a staging download did not cancel it — fixed in2ed6dd78.2ed6dd78): one P2 (cancellation matches the operator's argument rather than the uninstall's resolved identity, missed when an alias is used and no JAR path comes back) — predates the review rounds; fixed under plan 17-44 in3a833e93(see the table above), with the unlistable-folder variant tested.02e3a546, after follow-up 19): one P2 in staging code predating the follow-up — two loaded modules in different JARs with one identify-string made/upm updateselect the first; fixed inebc13975(refused before download, reply names every such module), thread replied.ebc13975): one P2 in record-write code predating the follow-up — aSecurityExceptionfrom the record'sFilescalls escaped callers that catch onlyIOException; fixed by class inab93f344(record writes, both public record operations, both phases of staging,deleteTree), thread replied.ab93f344): one P2 in the start-up restore path (byte-identical to28fe0208): aFAILEDrecord left with the new JAR in the modules folder and the old kept aside was restored only from the kept JAR, putting both on the class path. Fixed by defect class infda50c27: the undofailApplyruns is one helper,undoApply, now also used by later starts and by/upm update's discard (which had the same defect); every reachable record state × file placement is tested against the recovery invariant (table: fix(upm): commit a module update only after the next start shows it loaded (#505, #513, #518, #517) #561 (comment); 2 rows violated it before the fix); thread replied and resolved.88745228): no finding. Codacy then reported 6 new issues (path-construction markers, helper-assertion suppressions), fixed incb05fd3b; the first request oncb05fd3breturned a Codex error and no review.cb05fd3b): two P2s in code predating the follow-up, outside the recovery state machine —/upm updatesnapshotted the loaded modules before staging's lock, so an uninstall in between was missed; the uninstall's cancellation matched JAR names from outside the modules folder. Both fixed inf80dd639(test29864b5e); threads replied and resolved.f80dd639): one P2 in the follow-up-19 copy check — a copy failingSecurityPolicy.isValidModuleJaris never on the module class path, so it cannot load first; fixed in88472f91(test7793c770), thread replied and resolved. The follow-up's review budget is spent;88472f91itself has not been reviewed by Codex.88472f91): one P2, pre-existing — a removal record with a null entry threw out of install; fixed by class in00d80764(install and uninstall validate removal records as start-up does), resolved.00d80764): two P2s in commit and rollback (a file at the new JAR's name that is no longer the staged JAR). Handled by one rule for the category ind76b3d79(testfd64f6ec): the foreign-file guard andNEEDS_OPERATOR; foreign-file table (38 of 76 new rows RED before, all green after): fix(upm): commit a module update only after the next start shows it loaded (#505, #513, #518, #517) #561 (comment); threads replied and resolved. The GitHub review loop ends here; gate 2 continues under the local-review rule (see Threat model).ca5f6407,codex review --base alpha,gpt-6.1-sol, effort high): no finding. A first attempt on the same head produced no review (Codex's sandbox did not start, nothing was read) and is not counted. No fix commit, so no confirmation run is due (maintainer's rule: at most two local runs, the second after the single fix commit).aa8ca8b6under the local-review rule: run 1 found nothing;aa8ca8b6is a test-only Codacy fix; CI green. Out-of-model items: Module-file transactions: out-of-threat-model review findings (PR #561) #565.Compatibility
COMPATIBILITY.mdstates that/upm updatenow takes effect at the next start and is committed only after the module is observed loaded, where the records live, and that aplugins/folder on another file system is reported rather than updated.PluginInstallUtils.updatePlugin(String)keeps its signature; itstruenow means "staged".PluginInstallUtils.stageUpdate(String)andModuleFileTransactionsare@ApiStatus.Internal. 17-44 adds:updatePlugin(String)returnsfalsewhen another copy of the module would load first (follow-up 19), file-name discovery order, the duplicate-copy warning,main:-based uninstall identity, and the identity-based cancellation (whichPluginInstallUtils.uninstallPlugin(String)now performs too; the command reads the versions throughuninstallPluginReporting(String, List),@ApiStatus.Internal).ModuleJarIndexandPluginManager#getModuleJarIndex()are@ApiStatus.Internal.Consumer impact
Measured with
git grep -ion the fifteen moduleorigin/mastertrees and UltiTools-External-Example:PluginInstallUtils,ModuleFileTransactionsandupm-transactionshave 0 hits in Java sources in every tree (control:UltiToolsPluginhits in every tree, 2 to 137)./upmappears in those repositories only in inventory documents, changelogs and javadoc describing uninstall's unload order, which this pull request does not change.Load order (17-44): discovery is now file-name order. It decides which of several JARs carrying the same class supplies it, which copy of a duplicated module is read first, which module a
plugin.ymlname:shared by two modules resolves to, and the order of the opt-in legacy load. Modules without a dependency between them already loaded in alphabetical order of their class names and still do. An install that relied on one copy winning by listing order may see the other one win after upgrading — once, and then the same one on every start and file system.Documentation
Companion pull request, into docs
alpha: UltiKits/UltiTools-Dev-Doc#106 —module-versioning.md(English and Chinese): the real trigger ofhasNewerVersionLoaded(UltiTools-Dev-Doc#96, closed by hand after it merges), two copies of one module (file-name order, the start-up warning,/upm uninstalldeleting every copy), and updating/uninstalling on a server (/upm updatestaged and committed only after the next start observes the module loaded, rollback and failure lines; uninstall cancelling staged updates and deferring deletion).🤖 Generated with Claude Code