HUUUUGE HONGALABONGAMAHOOOOGS #670
No reviewers
Labels
No labels
accepted
bug
dependencies
docker
documentation
duplicate
enhancement
github-actions
github_actions
good first issue
gradle
hacktoberfest-accepted
help wanted
invalid
javascript
not-an-issue
npm
question
rejected
wontfix
Working on it
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
Griefed/ServerPackCreator!670
Loading…
Reference in a new issue
No description provided.
Delete branch "develop"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Both pins are RED at this commit — the fix follows in the next one. Forge's scanner is chosen by Minecraft era, and both call-sites make that choice from the minor component alone: ModListCompiler.kt:146 mcVersions[1].toInt() > 12 MetadataScanner.kt:87 minecraftVersion.split(".")[1] > 12 Minecraft has two versioning schemes (1.x.y and the newer YY.x.y), so 26.2's minor is 2, which reads as the 1.2 era and sends a modern pack to ForgeAnnotationScanner — the scanner for 1.12-and-older. This is the exact pattern serverpackcreator-api/CLAUDE.md's versioning-scheme landmine forbids. The failure is silent: the annotation scanner finds no fml_cache_annotation.json in a modern jar, every jar falls back to the never-drop-a-jar default of SERVER, and auto-exclusion quietly stops working on Forge 26.x while logging one ERROR per mod. It fails safe (everything is included), which is why it went unnoticed. Both tests loop over 1.20.1 and 26.2 against a real jar carrying a modern META-INF/mods.toml, and assert the outcome rather than which scanner was picked: ModListCompilerTest the CLIENT-declaring jar must be auto-excluded, the BOTH-declaring one kept MetadataScannerTest CLIENT vs SERVER_OR_BOTH Observed failing for the right reason — the 1.20.1 iteration passes in both, so the fixtures are valid and only the era selection is wrong: ModListCompilerTest Minecraft 26.2: the CLIENT-declaring mods.toml jar must be auto-excluded ==> expected: <[clientonly.jar]> but was: <[]> MetadataScannerTest Minecraft 26.2: a CLIENT-declaring mods.toml must be read as CLIENT ==> expected: <CLIENT> but was: <SERVER_OR_BOTH> The existing autoDiscoveryReachesScannerBranchPerLoader only covers 1.12.2 and 1.16.5, so nothing pinned the newer scheme. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Behaviour-preserving. Every existing assertion is untouched and green; the only test-file change is none at all. The package had five scanners that each carried their own copy of the same loop, an abstraction nobody outside -api could see, and a loader->scanner dispatch duplicated across a module boundary. Four extractions: 1. ModJarScanner (public) replaces the internal Scanner<T, U>. Both type parameters had exactly one instantiation across all five implementations (List<ScannedMod>, Collection<File>) — unused generality. More to the point, `internal` meant -clientside and -grinder could not see it, which is why MetadataScanner hand-wrote dispatch over concrete types. Removing an internal type is not an API break; adding the public one lets a plugin implement a scanner for the first time. 2. DescriptorScanner owns the walk-the-jars loop and the "one ScannedMod per input jar, whatever happened" contract that all five repeated. Subclasses now implement read(File) for a single jar and may simply throw — which is why scan() is final. That total-result contract is the one thing no scanner may get wrong (a dropped entry is a mod missing from the finished pack), and it can no longer drift between implementations. 3. FabricFamilyScanner absorbs what Fabric and Quilt genuinely share: id and environment reading, differing only in the path to those fields, including the subtle "no environment entry means SERVER" default. Dependencies stay abstract — Fabric declares an object keyed by mod id, Quilt an array of either objects or bare strings, so the block's shape differs, not its path. 4. ModScanner.scannerFor(modloader, minecraftVersion) is now the single dispatch, consumed by both ModListCompiler and MetadataScanner. The era rules and their magic versions live in one place, and QuiltPackScanner holds the two-descriptor merge that only ModListCompiler used to implement — clientside CLAUDE.md's "kept in sync deliberately, it is not shared code" is obsolete. Returning null for an unknown loader keeps both callers' existing handling. Quilt merge equivalence, since the two callers differed: ModListCompiler kept the Quilt entry unless Quilt said SERVER and Fabric said CLIENT; MetadataScanner unioned the two clientside sets. Both yield CLIENT iff either scanner did, so QuiltPackScanner reproduces ModListCompiler's rule exactly and MetadataScanner's answer is unchanged. The difference the union lost — which ScannedMod (and so which id and dependency list) survives — is preserved, and it matters for the downstream dependency-rescue. API compatibility: JsonBasedScanner is published, so it does NOT gain the abstract read() — a plugin subclass compiled against it must keep compiling. It stays a standalone @Deprecated(ReplaceWith("JsonDescriptorScanner")) helper delegating to the same internal readJarJson the new base uses, so the facade cannot drift from its replacement. Every other public member is unchanged: QuiltScanner.dependencyExclusions, ForgeTomlScanner.neoForgeMinecraft/client and ForgeAnnotationScanner.dependencyCheck/dependencyReplace stay public, and ForgeTomlScanner stays open for NeoForgeTomlScanner. Also removes the Qodana UnusedSymbol (JsonBasedScanner's never-read `log`), the same in ForgeTomlScanner once its catch moved to the base, and a dead NullPointerException catch in FabricScanner around a ModDependency construction that cannot throw. Two logging changes, cosmetic and stated rather than hidden: the per-jar failure is now logged from the base, so ForgeAnnotationScanner's variant that passed the exception (stack trace) uses the message form the other four already used; and ModListCompiler's two NeoForge "Scanning using X scanner." debug lines are gone with the branch that emitted them. Code lines, comments and blanks stripped: ModListCompiler.kt 169 -> 131 MetadataScanner.kt 43 -> 21 modscanning package 523 -> 527 (three new files carrying the shared contract) api 296 (1 skip), clientside 88, app 80, grinder 233 — all suites green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>ModScanner.scannerFor is now the single place a modloader and Minecraft version become the scanner that reads a pack, for both a generation and the clientside engine's metadata signal. Until now it was only covered by outcome — which mod ended up excluded — so nothing named the collaborator that should have been picked. Six tests, asserted on identity because what matters is *which* scanner comes back: - Fabric and LegacyFabric always get the Fabric scanner - Quilt gets the composite, not either scanner alone - the Forge era boundary (1.13) and the NeoForge one (1.20.5) are each read from the whole version, across both the 1.x.y and YY.x.y schemes - an unparseable version falls back to the modern Forge scanner and does not throw — "26" used to raise IndexOutOfBoundsException out of the bare- component parsing, so this pins the runCatching guard as load-bearing - an unrecognised loader yields null, the signal both callers turn into "keep every mod" These are green on arrival; the red-first evidence for the era rule itself is commitf8cb89bff, which failed on 26.2 before the fix. What they add is a pin on the dispatch as such, so a future change to the selection cannot pass merely because the fixture jars happened to be unreadable either way. api 302 (1 skip) — suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>VersionChecker had zero tests, and the next commit restructures the boolean chains Qodana flagged inside it. Version comparison is this project's documented silent-failure category — a wrong branch yields a plausible version, not an error — so it gets pinned before it gets touched. The class is abstract and its only data source is allVersions(), so the entire alpha/beta path runs offline against a canned list: no repository, no network. Seven tests over the protected isUpdateAvailable. Writing them surfaced a quirk, now pinned as characterization rather than "fixed" — isPreReleaseNewer compares only the number after the dot and is blind to the channel, while isUpdateAvailable consults beta before alpha: 3.1.0-alpha.2 -> 3.1.0-beta.3 (offered a beta, because 3 > 2) 3.1.0-alpha.5 -> up_to_date (offered nothing, though alpha.5 and beta.3 are both published: 3 > 5 fails for the beta, 5 > 5 for its own channel) Offering a beta to an alpha user is arguably right, but that is not what decides it — the numeric accident is. Pinned so the restructuring cannot change it silently; whether to change it deliberately is a separate call, and not one this branch makes. app 81 (from 80) — green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Qodana RedundantIf, 13 sites across api, app and grinder. Behaviour-preserving; all existing assertions untouched and green, and VersionChecker gained its characterization pin in the previous commit before being touched. Straight `if (c) true else x` -> `c || x` collapses: ConfigurationHandler.checkIconAndProperties, FileUtilities.isLink, InclusionsEditor.canImport, LoaderCache's idle check, ConfigEditorViewModel.hasUnsavedChanges, VersionChecker's alpha/beta checks, MigrationManager.older/newer. Two became `when` instead, because collapsing them would have changed meaning or lost it: - BooleanUtilities.convert: the recognised-false branch and the fallback both yield false, but only the fallback warns. A plain `||` would have fired the "couldn't parse" warning on every valid "false"/"0"/"no" — Qodana's suggestion is naive about the side effect. The `when` keeps the branches distinct and now says why in a comment. - JsonUtilities.getNestedBoolean: kept three-way with the throw. Note for the next reader, added as a comment: toBooleanStrictOrNull() is NOT a replacement here, it is case-sensitive and this accepts "True"/"FALSE". ConfigEditor.checkJava was inverted to a guard clause rather than folded into a `||`, since its other branch is a 25-line JOptionPane `when`. Swept up in the same files: ConfigEditorViewModel.requiredJavaVersion becomes Optional.orElse("?") (both directions already pinned by ConfigEditorViewModelTest), and FileUtilities.isLink's unused `ex` becomes `_` with a note on why the InvalidPathException is intentionally swallowed. api 302 (1 skip), app 81, grinder 233 — green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Written against the CURRENT (double-lookup) implementation and committed before the commit that changes it, so they are characterization tests rather than a description of the new code. Both services had zero coverage. RunConfigurationControllerTest and EventControllerTest exist but `mockk()` the services away, so the look-up-or- store loops in them were never executed by any test — and the persistence layer is MongoDB, where a save-on-absent path is not something you want to reason about from the source alone. 13 tests over the two: EventService.submit a known error is replaced by the stored entry; an unknown one is saved and the saved entry kept; a mixed batch keeps input order; no errors and an empty list both leave the error repository untouched RunConfigurationService .createRunConfig start args split on whitespace, mods on commas; known entries reused, unknown ones saved; blank inputs fall back to Aikar's flags and the configured mod-lists; an existing run configuration is returned instead of a duplicate being saved What is pinned is the OUTCOME — which entries the built object holds, and which reach `save`. Deliberately NOT how many times each repository is queried: the lookup count is an implementation detail, and pinning it would both make these red for the next commit and turn any future change of that shape into a failure for no reason. Verified passing against this tree, i.e. against the two-lookup code they characterize. The next commit halves those lookups and must leave all 13 green. app 88 -> 101. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Behaviour-preserving. All suites green. ReplaceManualRangeWithIndicesCalls (4): `for (i in 0 until x.size)` -> `for (i in x.indices)` in EventService and RunConfigurationService's three loops. All four bodies assign through the index, so `indices` is the fit rather than withIndex(). Swept up while in those same four loops, because it is the same line: each called its repository's finder TWICE per element — once for isPresent, once for get(). Now one lookup reused via orElseGet, halving the queries on every run-configuration save and every event with errors. Same result, and orElseGet keeps save() lazy so it still only runs when nothing was found. ConvertTwoComparisonsToRangeCheck (6): Tetris bounds checks become `x !in 0 until boardWidth`, matching the `0 until boardWidth` already in the file one line below the first of them. MayBeConstant (1): CurseForgePartition.CAP is `const` — it aliases CurseForgeCandidateSource.MAX_INDEX, itself a const. Two findings are NOT applied, deliberately: - UsePropertyAccessSyntax, LarsonScanner:1368. `g2d.renderingHints = ...` does not compile: Graphics2D's getter returns RenderingHints while the setter takes Map, so Kotlin exposes the property read-only. Tried it, the build failed, and the call now carries a comment so the next reader does not repeat it. - DestructuringDeclaration (3), ClientsideReportRenderer x2 and Grinder. All three are `for (verdict in report.perLoader)` over LoaderVerdict, a data class with eight-plus fields. Positional destructuring there costs every speaking name the loop bodies rely on, and componentN is positional — a reordered property would silently rebind every variable rather than fail to compile. That is precisely the silent-failure class this codebase guards against, so the named receiver stays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Qodana CanConvertToMultiDollarString (8) and CanUnescapeDollarLiteral (5), which overlap on the same five literals. Kotlin is 2.3.20, so the $$ prefix is stable and needs no opt-in. Behaviour-preserving; every literal must come out byte-identical, and each is verified rather than assumed. Three Spring cron placeholders (DatabaseCleanupSchedule, FileCleanupSchedule, VersionRefreshSchedule): "\${...}" -> $$"${...}". No test loads the scheduling context, so these were verified by measurement — javap on the compiled classes shows the constant-pool entry is still the placeholder with its braces: #106 = Utf8 ${de.griefed.serverpackcreator.spring.schedules.database.cleanup} BootLogClassifier's outOfMemoryMarkers: the `Killed "$JAVA"` alternative loses a layer of backslashes. Already pinned — aKilledServerIsInconclusiveNotCrashed feeds exactly that line and is green, which matters because this regex is what stops host memory pressure from manufacturing a HIGH-confidence clientside verdict. MigrationManager's lambda-suffix regex needed a seam before it could be touched at all: it was written out TWICE, in migration discovery and in version parsing, in two escaping-heavy copies with no coverage — and version parsing is this project's documented silent-failure category. So it is hoisted to one documented internal constant, LAMBDA_SUFFIX, converted there, and pinned by theLambdaSuffixIsStrippedFromMethodNames (the compiler's $0lambda$1, the bare $lambda$, multi-digit, plus the names that must survive untouched). The extraction and its test land together because the constant IS what makes the regex reachable from a test — the same enabling-change carve-out CLAUDE.md records for per-parameter KDoc on single-line constructors. Teeth checked, not assumed: with LAMBDA_SUFFIX broken to "[0-9]*lambda[0-9]*" the pin fails with `expected: <SixDotZeroDotZero> but was: <SixDotZeroDotZero$$1>`, then passes again on restore. app 82, clientside 88 — all suites green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The last 7 Qodana High findings. All were a deprecated member being used exactly where it still has to be, so the fix is to say so, not to change the code — `@Suppress("DEPRECATION")` on the three declarations, each with a doc comment giving the reason. ScriptTemplatesConfig.scriptTemplates (2 warnings) Calls the equally-deprecated defaultScriptTemplates() because both belong to the pre-6.0.0 flat-list representation, and this accessor exists to keep answering in THAT representation. Delegating to startScriptTemplates instead would change what it returns, which is the one thing a deprecated facade must not do. MigrationManager.FivePointZeroPointZero (3) and SixPointZeroPointZero (2) Migrating an old installation means touching the representation that old version wrote. Pointing these at the replacement would migrate the wrong setting — for SixPointZeroPointZero the deprecated flat list IS the input it converts into the per-type map. Also moved scriptTemplates' KDoc above its @Deprecated annotation. It sat between the annotation and the declaration, which is legal but leaves the property undocumented as far as dokka is concerned. Verified rather than assumed: `grep -c 'is deprecated. Deprecated as of 6.0.0'` over a --rerun-tasks compile of both modules goes 7 -> 0. The deprecation warnings that remain are third-party Java ones Qodana did not flag (nightconfig valueMap, Jackson fields, java.util.Locale(String)) and are untouched. Full build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Both RED at this commit — the fix follows in the next one. These replace the characterization test that recorded the first defect as a quirk (73e16ba9c), now that the decision to fix it has been made. 1. anAlphaIsOfferedTheBetaOfTheSameVersionWhateverTheNumbers isPreReleaseNewer compares only the number after the dot and is blind to the channel, so what an alpha user is offered depends on a numeric accident: alpha.2 gets beta.3 (3 > 2), alpha.5 gets nothing (3 > 5 fails for the beta, 5 > 5 for its own channel) with both beta.3 and alpha.5 published. expected: <3.1.0-beta.3> but was: <up_to_date> 2. aNewerVersionsPreReleaseWinsOverAHigherNumberedOlderOne latestBeta/latestAlpha keep a candidate only if it is BOTH semantically newer-or-equal AND higher-numbered, so a newer version restarting its count (3.2.0-beta.1 after 3.1.0-beta.3) loses to the older one. Whether that is observable depends on the order the repository returns versions in, which is not guaranteed anywhere. expected: <3.2.0-beta.1> but was: <3.1.0-beta.3> The second pin took two attempts to make honest, which is the point of watching it fail. A newest-first fixture passes, because latestBeta's wrong answer is masked by isUpdateAvailable's fall-through to latestVersion(). And a current version that is already the newest beta passes for the same reason. It only reaches the user when the beta branch itself fires and hands back latestBeta() directly — hence the oldest-first list and a current version old enough (3.1.0-beta.1) to trigger it. The fake's latestVersion() now COMPUTES the newest instead of taking the list head, so a fixture may be given in any order. The real allVersions() comes from a repository API whose ordering is not guaranteed, and a test that silently depends on that ordering cannot catch code that does the same. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>RED at this commit — the fix follows in the next one. Found while investigating why a normal api test run emits 159 scan-failure log lines. 28 of them are ScanningException("No dependencies specified."), raised by ForgeTomlScanner.getMapOfDependencyLists when a mods.toml declares no [[dependencies]] block — which is a perfectly ordinary descriptor, not an error. The exception aborts read() mid-way, so the mod falls back to the unreadable-jar defaults and its successfully-read modId is replaced by the FILENAME: expected: <lonelymod> but was: <lonelymod-1.0.0> The verdict itself is unaffected — a mod declaring no dependencies has no clientside signal, so SERVER is the only possible answer down either path, which is why this never broke a pack. What it does is discard a mod id that had already been parsed, and log an ERROR (with a stack trace, since6026f3640) for an entirely normal mod. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the pin from the previous commit green. ForgeTomlScanner.getMapOfDependencyLists raised ScanningException("No dependencies specified.") when a mods.toml carried no [[dependencies]] block. That is an ordinary descriptor — a mod is allowed to depend on nothing — and raising aborted read() mid-way, so the mod fell back to the unreadable-jar defaults and lost the modId it had already parsed to the filename. It now returns an empty map. The mod is then read normally: real id, SERVER verdict, empty dependency list. No change to any pack. A mod declaring no dependencies has no clientside signal, so SERVER was the answer down both paths; what changes is that ScannedMod.modID is now the declared id rather than the jar's filename. Nothing can regress from that — the id is joined on only by the dependency rescue, which looks up *disabled* mods, and a mod on this path is always SERVER and therefore never disabled. ScanningException had no other thrower and is deleted along with the stale @Throws on getSidenessesAndDependencies. It was internal, so nothing outside -api could reference it. Measured on a full :serverpackcreator-api:test run: scan-failure log lines 159 -> 131, i.e. exactly the 28 ScanningException lines, removed at the source rather than muted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Repositories were declared in 13 places: an allprojects{} block in the root, all six convention plugins, and five module build files. A new module got whatever set its convention plugin happened to carry, and nothing said which list was authoritative. They now live in one dependencyResolutionManagement block, with RepositoriesMode.FAIL_ON_PROJECT_REPOS so a stray project-level repositories{} is a build failure rather than a silent override. Verified: the build is green with that mode on, i.e. nothing in the tree still declares its own. Measured: mavenCentral() declarations 13 -> 2. The two are settings.gradle.kts and buildSrc/build.gradle.kts, which is a separate build and cannot read the root settings. Two removals worth calling out: - mavenLocal() was FIRST in buildSrc's repository list, so any stale artifact in ~/.m2 shadowed the real one and builds stopped being reproducible between machines. Gone. Also gone from buildSrc: the explicit "https://plugins.gradle.org/m2/" mirror, which is what gradlePluginPortal() already resolves to, and google(), an Android repository nothing here uses. - google() and gradlePluginPortal() are likewise gone from the dependency repositories; plugin resolution does not use them, and no dependency in the tree comes from either. Kept, with the reason recorded next to each: jitpack (serves com.github.MCRcortex:nekodetector, published nowhere else), spring milestones and the ej-technologies repository (both needed by -app). ./gradlew build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>It sat at the repository root, which is not where Gradle looks, so both settings files needed an explicit versionCatalogs { from(files(...)) } block to find it — the kind of wiring a newcomer has to read twice to understand is doing nothing special. At the conventional path the root build finds it by itself; that block is gone. buildSrc keeps its own, because it is a separate build and does NOT inherit the root catalog. Verified rather than assumed: removing the block fails buildSrc compilation with "Unresolved reference: libs" on all 11 dependency lines (Gradle 8.14.4). The comment now records that, so the next person does not retry the same simplification. Net: 2 explicit wiring blocks -> 1, and the file is where every Gradle user expects it. ./gradlew build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>No functional change; every removal is either a verbatim duplicate or a statement that never did anything. Duplicates: - java-conventions configured tasks.test TWICE, ~60 lines apart, which is where the cleanup() call was hiding. Merged into the one block. - spring-conventions applied org.springframework.boot, io.spring.dependency-management, kotlin allopen and kotlin spring in plugins{} and then AGAIN via apply(plugin = ...) directly below. The four apply() lines are gone. - kotlin-conventions and buildSrc/build.gradle.kts each held two byte-identical compileKotlin/compileTestKotlin blocks; both collapse to one withType<KotlinCompile>().configureEach. allWarningsAsErrors = false went with them — it is the default. - kotlin-conventions also set jvmToolchain(21) while java-conventions already pins the toolchain to 21 for the same projects. Dropped; JVM 21 is now stated in two places (the toolchain and the Kotlin jvmTarget) rather than four. Dead: - 35 lines of commented-out cyclonedx configuration in the root, plus its three commented imports and commented plugin line. - `tasks.build { doLast { tasks.dokkaGeneratePublicationJavadoc } }` in -api and the same shape in -app: an expression statement that resolves a task provider and throws it away. It reads as if it triggers the task; the finalizedBy on the following line is what actually does. - install4j's installDir if/else ended with an `else` branch identical to its first condition, so the Linux branch and the fallback were merged. Noise removed from every build: quasar-conventions printed "I am running on: <arch>" at configuration time, and four compile tasks each logged "Configuring <name> with version ..." at lifecycle level. ./gradlew build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>java-conventions applied maven-publish and signing to all six modules, giving each a mavenJava publication, three remote repositories, a sources jar and a javadoc jar. CI publishes ONE module: .gitlab-ci.yml runs four `:serverpackcreator-api:publish...` invocations and nothing else. Moved verbatim into a new serverpackcreator.publishing-conventions, applied by -api alone. The file opens with why it exists, so the next reader does not have to reconstruct the "only one module publishes" fact from the CI config. Verified per module, since getting this wrong breaks releases silently: api:publishMavenJavaPublicationToGitHubPackagesRepository EXISTS api:publishMavenJavaPublicationToGitLabRepository EXISTS api:publishMavenJavaPublicationToGitGriefedRepository EXISTS api:publishToSonatype EXISTS app / clientside / grinder / plugin-example gone Three call-sites existed only because publishing was global and are removed with it: -app's `tasks.signMavenJavaPublication` and `tasks.sourcesJar` wiring, and -plugin-example's sourcesJar wiring (a comment there says it existed purely to silence a Gradle 8 warning about a task that is now absent). Non-api modules no longer produce -sources.jar / -javadoc.jar. Checked before removing: neither .gitlab-ci.yml, the GitHub workflows, nor spc.install4j reference either artifact outside -api. Side effect worth having: `signing` no longer runs for five modules where findProperty("signingKey").toString() yielded the literal string "null". ./gradlew build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>serverpackcreator-api/build.gradle.kts held fifteen bare `copy { }` calls inside the `processResources` CONFIGURATION block. They executed whenever that task was configured rather than as task actions, so they had no inputs, no outputs, no up-to-date checking and no cacheability — and they wrote into two source trees (src/main/resources and serverpackcreator-help/Writerside/topics). Measured before: a second, fully UP-TO-DATE `:serverpackcreator-api:processResources` still rewrote both destinations. Measured after: both are untouched, and shipRootDocuments itself reports UP-TO-DATE. Now three declared Copy tasks — shipRootDocuments, shipWritersideDocuments, shipWritersideImages — over one shared list of the seven shipped documents, which also removes the fifteen-fold repetition of the same from/into pair. Correction to an earlier claim of mine, since it is in the branch's history: I described these as running on "every Gradle invocation". They did not. Configure-on-demand plus task-configuration-avoidance meant `gradlew help` and even `:serverpackcreator-api:help` left the files alone; it took a build that realized processResources. The defect is real but its scope was narrower than I first said, and the measurement above is what it actually was. Making the copies visible to Gradle immediately exposed a coupling the old approach had hidden: sourcesJar packages src/main/resources, which shipRootDocuments now writes, and Gradle failed the build for an undeclared dependency. That ordering was previously luck. Declared. Verified beyond the build passing: - all seven documents plus img/ (120 files) land in the Writerside topics, with LICENSE renamed to LICENSE.md as before - a real content change to the root README propagates to both destinations, i.e. the tasks are not permanently up-to-date The destinations are unchanged, so ShippedResourceTrackingTest's assertions about the repository's ignore rules still hold. ./gradlew build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Unwinds the three coupled constructs in build.gradle.kts: allprojects { tasks.withType<Test> { jvmArgs(...) } } evaluationDependsOnChildren() project("serverpackcreator-app").tasks.build.get().mustRunAfter( tasks.getByName("generateLicenseReport"), project("serverpackcreator-web-frontend").tasks.build.get()) project.childProjects["serverpackcreator-plugin-example"]?.tasks?.jar?.get() ?.archiveFile?.get()?.asFile?.toPath() // + !! at both use sites They were one knot: `.tasks.build.get()` on another project only resolves if that project is already evaluated, which is exactly what evaluationDependsOnChildren() was there to guarantee. - the two test JVM args move into java-conventions, where every module already gets its test configuration, with a note on why they exist (agent self-attach) - the ordering constraint is declared by -app itself, by task PATH. A string path resolves lazily; reaching into another project's task container does not - the example-plugin jar is consumed as an ARTIFACT. plugin-example exposes a consumable `pluginArtifact` configuration, the root depends on it, and the two copy tasks use that. This removes the nullable chain, both `!!`, and the evaluation-order dependency in one move. Deliberately NOT the legacy `archives` configuration, which Gradle 9 removes - evaluationDependsOnChildren() then has nothing left needing it, and is gone Also removes two more configuration-time filesystem calls: `appPlugins.mkdirs()` ran on every configuration of the root project, and both destination paths were raw `File(...)` relative to the working directory rather than project-relative. Verified, since these tasks feed a pf4j test: - copyPluginsApiUnitTests and copyExamplePluginsToApp still place serverpackcreator-plugin-example-dev.jar in both destinations - ApiPluginsTest PASSES against the jar the new wiring produces, i.e. pf4j still discovers all six extension points. The regenerated jar was then reverted, per the "don't commit rebuilds" rule in serverpackcreator-api/CLAUDE.md - ./gradlew clean build green CORRECTION to my own stated rationale, measured rather than assumed: I said this was a prerequisite for the configuration cache. It is not. `build --dry-run --configuration-cache` reports the SAME 20 problems (13 unique) before and after this commit, and configuration time is unchanged at ~4.95s either way. Those constructs block project ISOLATION, not the configuration cache. The 20 problems come from elsewhere and are listed in the next commit's documentation. What this commit actually buys: no eager cross-project evaluation, no `!!` and no nullable task chain left in the build, a real artifact dependency in place of a path dug out of another project's task container, and the prerequisite for project isolation if that is ever wanted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Two entries in the root CLAUDE.md build section. The first states the no-cross-project-configuration rule and names all four constructs that were removed, so none is re-introduced. The second corrects a claim I made and then measured: unwinding allprojects/evaluationDependsOnChildren/cross-project task access was NOT what stood between this build and the configuration cache. Same 20 problems before and after, same ~4.95s configuration time. It buys project isolation, which is a different feature. The real blockers are enumerated so nobody has to rediscover them: one is third-party (:generateLicenseReport holds a Project reference), the rest are ours (the filter{} and doFirst{cleanup()} in java-conventions capture the enclosing script; -app's test.doFirst also captures projectDir). With the third-party one unfixable, the honest ceiling is fewer problems rather than zero — recorded so the next attempt starts with the right expectation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Build cleanup, steps 1-4 of the build analysis. No functional change to what is produced; every step is verified by measurement rather than by tests, per the build-logic convention. mavenCentral() declarations 13 -> 2 hardcoded coordinates 62 -> 0 live (2 commented-out lines remain) catalog entries 21 (plugins only) -> 71 (plugins + 50 libraries) modules with publishing 6 -> 1 source-tree writes on an up-to-date run 15 files, two trees -> 0 - Repositories are declared once in settings.gradle.kts with FAIL_ON_PROJECT_REPOS, so a stray project-level block fails the build. buildSrc keeps its own (separate build) and no longer lists mavenLocal(), which was FIRST there and let a stale ~/.m2 artifact shadow the real one. - The version catalog moved to gradle/libs.versions.toml and now holds the libraries too. Verified inert: resolved runtimeClasspath + testRuntimeClasspath diffs are 0 lines for api, clientside, grinder and app. - plugin-example's stray kotlin-test-junit5:2.4.10 was dragging that module's whole Kotlin stack -- kotlin-stdlib included -- to 2.4.10 by conflict resolution, so it compiled with the 2.3.20 compiler against a 2.4.10 stdlib. Aligned to 2.3.21 like every other module; that is the one intended graph change (158 lines). - Publishing and signing apply to -api alone, matching CI. All four :serverpackcreator-api:publish... tasks CI invokes were verified present, and absent everywhere else. - The fifteen configuration-time copy{} calls in -api became three Copy tasks with declared inputs and outputs. Making them visible immediately exposed a real undeclared dependency (sourcesJar packages what shipRootDocuments writes) that had been ordering by luck. Also removes duplicated and dead build logic: a second tasks.test block, four redundant apply(plugin=...) calls, two pairs of byte-identical compile blocks, 35 lines of commented-out cyclonedx config, two no-op doLast statements, and configuration-time println/lifecycle logging on every build.Build cleanup, step 5: no cross-project configuration in the root build. Removes allprojects{}, evaluationDependsOnChildren(), the root's project("serverpackcreator-app").tasks.build.get().mustRunAfter(...), and the childProjects[...] chain that dug the example-plugin jar out of another project's task container behind two !! operators. They were one knot: reaching another project's tasks eagerly only works if that project is already evaluated, which is what evaluationDependsOnChildren() guaranteed. Now the ordering is declared by -app itself using lazy task paths, and the plugin jar is consumed as an artifact through a consumable `pluginArtifact` configuration -- which removed the build's last two !!. Verified where it mattered: the copy tasks feed a pf4j test, so both were run and ApiPluginsTest passes against the jar the new wiring produces (all six extension points discovered). The regenerated jar was reverted per the don't-commit- rebuilds rule. CORRECTION carried in the branch: this was NOT the configuration-cache prerequisite I claimed. Measured -- build --dry-run --configuration-cache reports the same 20 problems (13 unique) before and after, and configuration time is ~4.95s either way. These constructs block project ISOLATION, a different feature. The real blockers are now enumerated in CLAUDE.md: one is third-party (:generateLicenseReport holds a Project reference), the rest are ours (script object references captured by the filter{} and doFirst{cleanup()} in java-conventions, plus -app's test.doFirst capturing projectDir). With the third-party one unfixable, the ceiling is fewer problems, not zero.Six items. All links were checked first and are fine — 16 internal links resolve and the 8 project-owned external ones return 200 — so the staleness was all in content. 1. The commandline-argument table listed 7 of 17 arguments. It now lists all 17, with descriptions taken from Mode.kt's per-entry KDoc so the two cannot drift silently again. The ten that were missing: -config, --destination, -feelinglucky, -withallinconfigdir, --home, -lang, and the four clientside verbs -scan, -clientsidereport, -verifyclientside, -clientsideapply The four clientside ones get a note that they are maintainer tools and that -verifyclientside boots a real server, so nobody runs it expecting a quick answer. Verified after the edit: 17 of 17 documented, none missing. 2. "ServerPackCreator will crash, complaining about JDBC-related things" in the webservice setup was left over from the H2/JPA era. There is no JDBC anywhere in main source — persistence has been MongoDB since Spring Boot 4. Reworded to what that first run is actually for (creating the home-directory) and what goes wrong (no database configured yet), without asserting a specific error text I have not reproduced. 3. The Gradle-Groovy dependency snippet was fenced as ```kotlin while containing Groovy single-quote syntax. Now ```groovy. The Kotlin snippets are untouched and remain compiler-gated by ReadmeExamplesTest, which passes. 4. Dropped `version: '3'` from the docker-compose example. Compose v2 treats the key as obsolete and warns about it on every invocation. 5. Link text read `docker/docker/init-mongo.js` while pointing at the correct `docker/init-mongo.js`. 6. Documented SPC_CONFIGURATION_AIKAR and SPC_LOG_LEVEL, two operator-facing variables present in both shipped compose-files but absent from the table. Everything else was checked and is current: every de.griefed.serverpackcreator.* property and SPC_* variable in the README still exists in the code, the Java 21 requirement holds, the five-modloader list is right, and the API example's symbols all resolve. The generated copies under -api's resources and Writerside's topics are not tracked; a build regenerates both, verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Found while checking a stale README line that told self-hosters to expect a crash "complaining about JDBC-related things". There is no JDBC in this app — persistence has been MongoDB since Spring Boot 4 — but the properties files still carried the old stack's settings. serverpackcreator-app/src/test/resources/application.properties, 12 lines, including the one that matters: spring.data.mongodb.uri=jdbc:h2:mem:testdb A MongoDB URI holding a JDBC URL. ConnectionString accepts only mongodb:// and mongodb+srv://, so this is a hard startup failure the moment Mongo autoconfiguration runs. It never has, purely because WebServiceTest is @SpringBootTest(classes = [WebServiceTest::class]) and therefore boots a context of exactly one class. Dormant, not harmless: the first real @SpringBootTest anyone writes inherits it and fails with a message pointing nowhere near the cause. serverpackcreator-app/src/main/resources/application.properties, 4 lines: spring.transaction.default-timeout and three spring.datasource.tomcat.* pool settings for a datasource that does not exist. Verified dead before removing, rather than assumed: @Transactional in main source 0 files JPA / JDBC / DataSource types in main source 0 files hibernate, tomcat-jdbc or h2 on the runtime classpath 0 hits The unused `testRuntimeOnly(libs.h2)` dependency and its catalog entry go too — no test source references H2 at all. serverpackcreator-app/CLAUDE.md records why these were dormant and why that made them a trap rather than a curiosity. ./gradlew clean build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Remove the JPA/H2/JDBC relics from the Spring properties, and audit both branches. Found while checking the stale README line about "JDBC-related things": the properties files still carried the pre-MongoDB stack's settings, including spring.data.mongodb.uri=jdbc:h2:mem:testdb a MongoDB URI holding a JDBC URL. ConnectionString accepts only mongodb:// and mongodb+srv://, so it is a hard startup failure the moment Mongo autoconfiguration runs. It never has, purely because WebServiceTest boots a context of exactly one class. Dormant, not harmless -- the first real @SpringBootTest anyone writes inherits it and fails somewhere far from the cause. 16 dead properties removed across the two application.properties, plus the unused testRuntimeOnly H2 dependency and its catalog entry. Verified dead by measurement rather than inspection: 0 @Transactional, 0 JPA/JDBC/DataSource types in main source, 0 hits for hibernate, tomcat-jdbc or h2 on the runtime classpath. REFACTOR-AUDIT.md carries the audit of both branches: no HIGH findings, one MEDIUM (this commit removes main-source runtime configuration that no test could have protected -- mitigated by the measurement above, and the real gap is WebServiceTest asserting nothing), three LOW.Behaviour-preserving. No test file is touched and the existing suite is green. WebService.start() built the eight-location --spring.config.location argument inline and handed it straight to Spring Boot, so the composition could not be asserted from anywhere. It is now WebService.configLocationArgument(), a pure function taking the three inputs it actually depends on — the properties file, the overrides file, and the user's home directory. Exactly the extraction springArguments already had, and for the same stated reason: start() boots Spring, so anything welded to it is untestable. The produced string is unchanged. The only shape change is that the user-home locations are built from an injected File rather than from System.getProperty("user.home") read inside the method; File(File, String) and File(String, String) resolve identically, and the next commit pins the full eight-location output so this is guarded rather than argued. Why this matters more than a tidy-up: later locations win, and the last two are overrides.properties — the file the docker image's init-spc-config script composes SPC_DATABASE_* into, and therefore where spring.data.mongodb.uri arrives from in a container. A location silently going missing here is a property-file that is never read, which for the database URI is a hard startup failure (see the landmine in serverpackcreator-app/CLAUDE.md). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The old WebServiceTest was @SpringBootTest(classes = [WebServiceTest::class]) — a context consisting of exactly one class, itself — with an empty contextLoads() body. It could not fail for any reason involving ServerPackCreator, which is why this module's CLAUDE.md said to replace rather than extend it. Deleted. WebServiceContextTest boots the REAL context (classes = [WebService::class]) and asserts it wires up: every @RestController registered, the four services the controllers delegate to resolvable, and ServerPackCreator's own property-files actually present on the environment — the other end of the config-location chain the previous commit pinned. It needs no database, which I checked rather than assumed: the MongoDB driver connects lazily, so the whole context starts and every injection point resolves with nothing listening on 27017. The driver logs a ConnectionException in the background and startup continues. That makes the wiring half testable offline; exercising an actual query still needs a live Mongo and belongs in an integration test. Teeth verified, since a context test that cannot fail is exactly what was being replaced: removing @Service from EventService fails it with NoSuchBeanDefinitionException: No qualifying bean of type '...EventService' and it passes again on restore. (A first attempt broke a constructor signature instead, which failed compilation before the context could start — not a valid check, so it was redone.) The three schedules are disabled via Spring's CRON_DISABLED ("-") rather than left on their midnight crons. FileCleanupSchedule deletes modpack files whose IDs are absent from the database; a suite that happens to run at 00:30 against an unreachable database should not be the thing that discovers what that does. Also moves the file from package de.griefed.serverpackcreator.web to ...app.web, matching every other test in the module. app 105 -> 108. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Separate from the configuration-cache commit before it, because it is a separate concern — and, measured, it contributes nothing to cache compatibility. With only the capture removals in place, `build --dry-run --configuration-cache` already reported 7 problems / 1 unique, the same as after this commit. The copies were never the cache's problem; the expand() reading script properties was. What they ARE is the anti-pattern already fixed in -api: three bare `copy { }` calls inside the processResources CONFIGURATION block, so they ran whenever that task was configured — including on runs where processResources was UP-TO-DATE and did nothing — with no inputs, no outputs, no caching, writing into the source tree every time. Measured on a second, up-to-date `:serverpackcreator-plugin-example:processResources`: before README.md and LICENSE rewritten after both untouched Found while measuring: the third copy has been dead. There is no CHANGELOG.md at this module's root, so `copy { from(...CHANGELOG.md) }` matched nothing. `include(...)` behaves identically, so the behaviour is unchanged and this commit does not touch it — but serverpackcreator-plugin-example/src/main/resources/CHANGELOG.md is a TRACKED 14 KB file that nothing generates and nothing updates, and it ships inside the example plugin's jar. Surfaced rather than silently deleted; it wants its own decision, since "delete a tracked file that ships to users" is not a build cleanup. Verified the three documents still reach their destination and the jar: LICENSE, README.md and CHANGELOG.md all present in src/main/resources and all present in the built jar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Replaces the last 10 eager task lookups with tasks.named(...). Verified after the change: 0 `tasks.getByName` / `tasks[...]` remain anywhere in the build. serverpackcreator-api/build.gradle.kts 5 (fixMissingResources) build.gradle.kts 2 (clean/copyLicenseReport) dokka-conventions 1 (compileJava, compileTestJava) quasar-conventions 1 (installNode -> installQuasar) publishing-conventions 1 (javadocJar artifact) getByName forces the task to be created during configuration whether or not the build will run it; named() returns a provider and defers that. The quasar case changed shape rather than just its call: `tasks.getByName("installNode").finalizedBy(tasks.getByName("installQuasar"))` realized both tasks to express one wiring; it is now a configuration block on the provider. ./gradlew build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes the prerequisite trap BUILD.md documented: without a local JDK 21 the modules could not be built at all, because the foojay toolchain resolver was registered only in buildSrc/settings.gradle.kts. Reproduced first, by pointing the module toolchain at an uninstalled JDK 11: Cannot find a Java installation ... matching {languageVersion=11, ...}. Toolchain download repositories have not been configured. and verified fixed the same way, with --offline so nothing actually downloaded — the error becomes "Some toolchain resolvers had provisioning failures: foojay (... No cached resource ... available for offline mode)", i.e. the resolver is registered and tried. It was not the one-liner I predicted. Three attempts failed first, and the shape that works is worth recording because it looks wrong: - root `plugins { id(...) version "0.8.0" }` while buildSrc also declares it -> "already on the classpath with an unknown version" - root without a version -> "not found in any of the following sources" - registering the resolver class directly via toolchainManagement -> the class is not on the settings-script compilation classpath The working arrangement is BOTH, declared differently: the root **with** the version, buildSrc **without** one. Both are genuinely required — verified separately that buildSrc does NOT inherit the root's toolchain repositories (it fails with "download repositories have not been configured" when the root alone has the resolver). BUILD.md's trap becomes a statement of how it works, with the asymmetry and its two error messages spelled out; the same fact goes into CLAUDE.md's build-layout section so nobody "tidies" one of the two declarations away. The audit's L-2 is marked fixed, noting it cost more than the one line it was scoped at. ./gradlew clean build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>RED at this commit — the fix follows in the next one. Reported from a real pack. The Java-24 guard in setupForge only protects users who ALREADY had a suitable Java installed; a pack that installs its own sails straight past it: No suitable Java installation was found on your system. Proceeding to Java installation. Downloading and using Java temurin@25 ... Run Command: java ... -Djava.security.manager=allow -jar server.jar --installer-force --installer ...forge-1.20.1-47.4.22-installer.jar Error occurred during initialization of VM java.lang.Error: A command line option has attempted to allow or enable the Security Manager. Why: JAVA_VERSION starts as the literal "do_not_manually_edit" and is only filled in by getJavaVersion. None of the three installJava call-sites re-read it afterwards, and install_java.sh never sets it either — verified in all three shell templates. So setupForge evaluates [[ "${JAVA_VERSION}" =~ ^[0-9]+$ ]] && [[ ${JAVA_VERSION} -ge 24 ]] against the placeholder, the regex does not match, and the else branch hands SSJ the fatal flag. The pin asserts the FAIL-SAFE property rather than just "re-read the version": an unresolved Java version must never take the branch that passes a flag which is fatal on the JVMs it cannot rule out. Covers all three shapes the variable can carry when nothing resolved it — the placeholder, empty, and a non-numeric value. Observed failing for the right reason, reproducing the reported run command: with JAVA_VERSION='do_not_manually_edit' the security-manager flag was passed ... RESULT=@user_jvm_args.txt -Djava.security.manager=allow -jar server.jar ... Neither the grinder nor ScriptTemplateMatrixIT could have caught this: both pre-bake Java and never take the install path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Both guards are RED at this commit, on purpose; the fix follows in the next one. Observed against unmodified code: parallelMapDoesNotLeakAThreadPerInvocation parallelMap leaked 4 thread(s) still alive after 4 invocations expected: <0> but was: <4> parallelMapRunsElementsOnMoreThanOneThread every element ran on one thread (ids=[58]); the default context is not parallel `parallelMap`'s defaulted context is `newSingleThreadContext("parallelMap")`. That factory owns a dedicated thread and requires its creator to `close()` it, but a defaulted parameter has no owner, so every single invocation strands one thread for the life of the JVM. The same default also means a function named `parallelMap` confines all of its elements to one thread. Two deliberate choices in how the guards measure, both of which a more obvious version gets wrong: - the leak guard compares a COUNT, not a set of names. Every leaked thread carries the identical name `parallelMap`, and Kotlin's `List - Set` drops all occurrences of a duplicate, so a name-difference version silently reports zero leaks whenever the baseline is already non-empty — i.e. it stops guarding precisely when an earlier test in the same JVM has already leaked one, and JUnit guarantees no ordering between the two tests here. - the parallelism guard identifies threads by `Thread.threadId()`, never by name. Gradle enables assertions on test tasks, which flips kotlinx.coroutines' `auto` debug mode on, and that appends ` @coroutine#N` to the thread name. A name-based version collects one entry per *coroutine* and PASSES against the single-threaded context — it was written that way first and caught only by watching it fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Defaults `parallelMap`'s context to `Dispatchers.Default` instead of `newSingleThreadContext("parallelMap")`. Turns the previous commit's two guards green. `newSingleThreadContext` hands out a dispatcher that owns a dedicated thread and requires whoever built it to `close()` it. As a *defaulted parameter* it has no owner and nothing ever closed it, so every invocation stranded one thread for the life of the JVM — measured at 4 surviving threads for 4 calls. The same default also confined the whole list to one thread, so a function named `parallelMap` was not parallel: all 8 elements of the parallelism guard ran on thread id 58. This is labelled `fix:` and not `refactor:` because it changes behaviour twice over, and the second one is not merely a repair: - elements now run on the shared pool sized to the available processors rather than one confined thread. Callers relying on that confinement for safety — a lambda mutating shared state without synchronisation was previously serialised by accident — can now race. `parallelMap` is public, published API, so this reaches embedders and plugins even though it has zero call sites in this repo. Recorded in the API-compatibility table. - the `@OptIn(DelicateCoroutinesApi::class, ExperimentalCoroutinesApi::class)` is gone, because the delicate API was `newSingleThreadContext` itself. An opt-in to `DelicateCoroutinesApi` on a *default value* is the tell that the default is wrong, not a formality. The signature is unchanged, so a caller passing its own context is unaffected and nothing loses source compatibility. Also narrows this file's `import kotlinx.coroutines.*` to the four symbols it actually uses, now that removing the opt-in shrank that surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Version bumps only, no build logic. This commit is RED for -app, on purpose; the fix follows in the next one. kotlinLibs 2.3.21 -> 2.4.10 nightConfig 3.8.4 -> 3.9.0 coroutines 1.10.2 -> 1.11.0 jackson 2.22.0 -> 2.22.1 junitPlatform 6.1.0 -> 6.1.3 log4j 2.26.0 -> 2.26.1 mockk 1.14.6 -> 1.14.11 bouncycastle 1.84 -> 1.85 ktorfit 2.7.3 -> 2.7.5 playwright 1.60.0 -> 1.62.0 flatlaf 3.7.1 -> 3.7.2 springBoot 4.0.6 -> 4.1.0 Also folds the separate `jacksonDatabind` version entry into `jackson`; the two had drifted to 2.21.1 and 2.22.0 while naming artifacts from the same release train. Measured at THIS commit: :serverpackcreator-app:dependencyInsight --dependency kotlinx-coroutines-core testRuntimeClasspath 1.10.2 (catalog asks for 1.11.0) :serverpackcreator-app:test 108 tests, 16 failed java.lang.NoSuchMethodError: 'java.lang.Object kotlinx.coroutines.BuildersKt.runBlockingK(kotlin.coroutines.CoroutineContext, kotlin.jvm.functions.Function2)' The bump alone cannot be green, and separating it from its fix is the point. `io.spring.dependency-management` turns Boot's BOM into forced versions that beat every transitive request, and Boot's BOM manages kotlinx-coroutines — so -app is pinned to 1.10.2 while -api compiles against the catalog's 1.11.0, which renamed the Kotlin-facing `runBlocking` to JVM name `runBlockingK`. The result compiles green in every module and dies only when the code runs. Landing this separately is the same discipline as committing a failing test before its fix: it makes the breakage checkable. Squashing it into the build fix would leave a commit whose message claims a before/after measurement that nobody can reproduce from the tree it describes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Documentation only; no code changes. Root CLAUDE.md, API compatibility table — two new rows: - `parallelMap`'s new `Dispatchers.Default` default, spelling out that the interesting half for embedders is not the leak but the loss of accidental single-thread confinement, which can newly expose races in an unsynchronised caller lambda. - the coroutines 1.11.0 runtime floor. 1.11.0 renames the Kotlin-facing `runBlocking` to JVM name `runBlockingK`; `javap` confirms it is present in 1.11.0 and absent in 1.10.2, and our compiled `VersionMeta.class` emits it. An embedder that *pins* coroutines to 1.10.x gets `NoSuchMethodError` at runtime with nothing failing to compile. Root CLAUDE.md also gains the build-layout landmine for Boot's BOM — it must be a `platform()`, never io.spring.dependency-management — with the measured outcome (runtimeClasspath 13 -> 0 differing between -api and -app) and the two traps: the BOM coordinate must come from the catalog's `springBoot` and not from SpringBootPlugin.BOM_COORDINATES, and a platform only out-ranks what a module actually requests, so a purely transitive library still needs declaring. Corrects the stale claim that `kotlinLibs` is 2.3.21 — it is 2.4.10, a full minor above the 2.3.20 compiler rather than a patch, and records that the pairing was measured (metadata versions read off the jars with `javap -v`) rather than assumed, and that the measurement binds Gradle only: IntelliJ analyses with its own bundled Kotlin plugin. api test count 302 -> 309 (the table had already drifted; +2 is this branch). serverpackcreator-api/CLAUDE.md gains the reusable landmines: never default a parameter to a thread-owning dispatcher (an `@OptIn(DelicateCoroutinesApi)` on a default value is the tell), and the matched pair of ways to get a thread assertion wrong — identity by name is broken by coroutine-debug decoration, and set-difference over same-named threads silently stops guarding. The second was found by auditing a guard that was already green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Behaviour-preserving. Two coordinates that bypassed `gradle/libs.versions.toml` now read from it; no version changes and no classpath moves. - `-api` declared nekodetector as a hardcoded coordinate, `api("com.github.MCRcortex:nekodetector:Version-1.1-pre")`, while the catalog already carried both a `nekodetector` version and a library alias that nothing referenced. Now `api(libs.nekodetector)`. This was the only hardcoded coordinate left in the build and a direct violation of the rule in CLAUDE.md. - `spring-conventions` rebuilt the BOM coordinate as a string from `findVersion("springBoot")`, leaving the `springBootDependencies` alias declared but unused. It now resolves the alias with `findLibrary`, so the catalog entry is consulted rather than duplicated by concatenation. Verified: `./gradlew build` SUCCESSFUL; nekodetector still resolves com.github.MCRcortex:nekodetector:Version-1.1-pre. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Behaviour-preserving. Adds `[plugins]` with the two aliases a real build script can consume, and removes the three version literals that duplicated the catalog by hand. serverpackcreator-api id("de.comahe.i18n4k") version "0.11.2" serverpackcreator-plugin-example id("de.comahe.i18n4k") version "0.11.2" build.gradle.kts id("io.github.gradle-nexus.publish-plugin") version "2.0.0" all three now `alias(libs.plugins.…)`. The i18n4k literal sat in two modules and had to be kept in step with the catalog's `i18n4k` — which also versions the i18n4k libraries — by a comment rather than by a mechanism. buildSrc is untouched here and still takes its plugin versions from the `[libraries]` entries; moving it to `[plugins]` changes what it resolves, so it is the next commit rather than this one. Verified: `./gradlew build` SUCCESSFUL, twice. Same versions resolve; nothing moves on any classpath. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Moves buildSrc's plugin dependencies onto the catalog's `[plugins]` entries via the plugin MARKER (`<id>:<id>.gradle.plugin:<version>`), and deletes the ten `[libraries]` entries that superseded. This CHANGES what buildSrc resolves, which is why it is separate from the alias conversion in the previous commit. Precompiled script plugins CANNOT use `alias(libs.plugins.x)` — verified by trying it, which fails at :buildSrc:compilePluginsBlocks with `Unresolved reference: libs`. So the convention plugins keep applying a versionless `id("...")` and the version has to reach them through buildSrc's own compile classpath. Taking that from `[plugins]` removes the last place where one plugin was described by two unlinked strings: the id in the convention plugin, an unrelated implementation coordinate in the catalog. Measured, flattened :buildSrc:compileClasspath, before -> after: 23 -> 31 modules. The nine additions are marker POMs. The one REMOVAL is `org.jetbrains.dokka:javadoc-plugin:2.1.0`, which the `org.jetbrains.dokka-javadoc` marker does not depend on. Checked rather than assumed, because -api's javadoc jar is published to Maven Central and the task reports success either way: from a wiped build/dokka, :serverpackcreator-api:dokkaJavadocJar produces 467 files / 356 HTML pages including real class pages. Unnecessary artifact, not a silent loss. Verified: `./gradlew build` SUCCESSFUL, 91 tasks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Prerequisite for moving the Kotlin compiler off 2.3.20, landed separately so both commits stay green. Kover reads `compileKotlinTask` off the Kotlin compilation by reflection (`kotlinx.kover.gradle.plugin.util.DynamicBean.value`). KGP 2.4.10 no longer exposes that property on `KotlinWithJavaCompilation`, so Kover 0.9.1 fails the whole build at task-graph time, before anything compiles: Could not determine the dependencies of task ':serverpackcreator-api:koverGenerateArtifactJvm'. > Could not get unknown property 'compileKotlinTask' for compilation 'main' (target (jvm)) of type KotlinWithJavaCompilation_Decorated Since `kotlin-conventions` applies Kover to every module, that breaks the entire build, not one report. Measured at THIS commit, i.e. still on Kotlin 2.3.20: `./gradlew build` SUCCESSFUL. So 0.9.9 supports both the old and the new compiler, which is what lets it land first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Behaviour change: `kotlin`, `kotlinAllOpen` and `kotlinJpa` move 2.3.20 -> 2.4.10, matching `kotlinLibs`, which was already there. The four entries now hold the same value; collapsing them onto one ref is the next commit, so that "the compiler moved" and "the catalog was restructured" are separately bisectable. This is the decision CLAUDE.md deferred when the entries were split. Until now -api was compiled by a 2.3.20 compiler against a 2.4.10 stdlib. That worked, but it is the same shape as the coroutines 1.11.0 failure this branch already fixed: a compiler reading metadata from a newer library fails hard with "binary version of its metadata is X, expected Y", and nothing warns as the gap widens. Measured, because a compiler bump is exactly where silent regressions live: compiler warnings, all 5 modules, main + test, --rerun-tasks before 243 after 243 The ONLY difference is one warning the 2.4.10 compiler rewords in place, at the same ServerPackCreator.kt:164:95: - Right operand of elvis operator (?:) is useless if it is null. + Elvis operator (?:) is redundant if the right operand is always null. So: zero new warnings, zero suppressed ones. ./gradlew build BUILD SUCCESSFUL, 91 tasks — the whole graph: 741 JVM tests, every Kover report, bootJar, both dokka publications, sourcesJar, generateLicenseReport, and the frontend (installFrontend / assembleFrontend / checkFrontend, the last running the Vitest suite via `npm run test`). kapt in -plugin-example, allopen/jpa/spring in -app, dokka 2.1.0 and Kover were the four things most likely to object. Kover did, which is why it was bumped in the preceding commit; the other three build and test clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Documentation only, for the six build commits that precede it. Collected here rather than split across them so the branch has one documentation convention throughout — commits 6 and 7 already used dedicated `docs:` commits. Root CLAUDE.md: - `[plugins]` and the two consumption routes: a real build script uses `alias(libs.plugins.x)`; a precompiled script plugin CANNOT, failing with `Unresolved reference: libs` (verified by trying it), and instead takes its version from the plugin marker buildSrc puts on its own compile classpath. Either route reads the one catalog. - the buildSrc marker conversion's measured effect, including the dropped `org.jetbrains.dokka:javadoc-plugin` and the reason it is harmless — with a standing warning to check the javadoc jar when touching dokka wiring, because -api's is published to Maven Central and the task reports success either way. - `settings.gradle.kts` cannot use the catalog in its own `plugins {}` block, which is why the foojay resolver keeps a literal version. - one `kotlin` entry for everything JetBrains ships from the Kotlin release train, replacing the note that described the old four-entry split as deliberate. That note, and the paragraph justifying the 2.3.20-compiler / 2.4.10-library gap as verified-safe, are both obsolete now that the gap is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Started as "why is runBlocking(Dispatchers.IO) failing" and turned out to be two unrelated things: a stale IntelliJ project model, and a real runtime breakage nobody had hit yet. Fixes: * parallelMap leaked a dedicated thread on every call and was not parallel — its defaulted context was newSingleThreadContext, which nothing could close. * Spring Boot's BOM, applied through io.spring.dependency-management, forced versions over every transitive request and silently reverted -app's half of every catalog bump. It is now a Gradle platform(), so the catalog wins. This is what broke coroutines: -api compiled against 1.11.0 and emitted the renamed runBlockingK, while -app ran on the BOM's 1.10.2. * mockk had split 1.14.11/1.14.6 between -api and -app. * Kover 0.9.1 could not read KGP 2.4.10. * dokka's HTML publication had an undeclared dependency on the Java compilations — pre-existing, reproduced on develop before the fix. Build: * every plugin id and version now lives in gradle/libs.versions.toml, with a new [plugins] section; the last hardcoded coordinate is gone. * one `kotlin` entry for the whole Kotlin release train, at 2.4.10. This bumps the compiler from 2.3.20; measured at 243 compiler warnings before and after. * springGradle aligned with springBoot at 4.1.0. -api vs -app resolved runtimeClasspath: 13 differing coordinates before, 0 after. Two commits are deliberately red so the breakage they describe is checkable from the commit that causes it:e55ba8947(the parallelMap guards, before their fix) ande55ddfe8e(the catalog bump, before the platform switch). API note: ListUtilities.parallelMap keeps its signature but no longer confines elements to one thread, so an embedder relying on that accidental serialisation can now race. Recorded in the compatibility table in CLAUDE.md.The Boot 4.0.6 -> 4.1.0 bump moved `spring-boot-mongodb` 4.0.2 -> 4.1.0 and `mongodb-driver-core` 5.6.2 -> 5.8.0, leaving this landmine citing versions the build no longer resolves. Since the whole entry rests on a `javap` measurement, stale version numbers make it unverifiable rather than merely untidy. Re-measured at 4.1.0 / 5.8.0, both claims still hold exactly: PropertiesMongoConnectionDetails.getConnectionString() getfield properties / invokevirtual getUri / ifnull 25 new com/mongodb/ConnectionString ... areturn i.e. still `if (uri != null) return new ConnectionString(uri)`, with the host/port/username/password branch unreachable whenever a uri is set. com.mongodb.ConnectionString still declares MONGODB_PREFIX and MONGODB_SRV_PREFIX and accepts only `mongodb://` / `mongodb+srv://`. Documentation only; no behaviour change. Noticed because the driver version is printed in `WebServiceContextTest`'s startup log, which is where the mismatch with this file showed up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>New `NetworkConfig` settings-group, following the group pattern in serverpackcreator-api/CLAUDE.md, with thin `ApiProperties` facades: de.griefed.serverpackcreator.network.timeout.connect 5000 de.griefed.serverpackcreator.network.timeout.read 15000 de.griefed.serverpackcreator.network.timeout.download.read 60000 Read-timeout bounds a *single* read, not the whole transfer, so the download value is not a transfer budget -- it is separate because installers and mod jars are served by hosts that trickle bytes under load, where a stalled metadata endpoint is simply broken. Negative values fall back (`setConnectTimeout` throws on them, and a properties typo must not crash every call); `0` is honoured as the JDK's "wait forever", i.e. the documented escape hatch to the previous behaviour. Nothing reads these yet -- this commit only makes the values available, so the guard that follows can reference them and be red for the right reason. `NetworkConfig(store)` depends on nothing but the store, and nothing inside `ApiProperties` reads it, so the declaration-order landmine does not apply. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Both guards land RED against a loopback ServerSocket which accepts the connection and then never writes a byte -- what a black-holing firewall or a stalled upstream looks like from the client side: isReachableGivesUpOnAServerThatAcceptsButNeverResponds FAILED (>15s) downloadFileGivesUpOnAServerThatAcceptsButNeverResponds FAILED (>15s) Both block in sun.net.www.http.HttpClient.parseHTTPHeader, reached from HttpURLConnection.getInputStream -- the read of the status line, with no bound on it. No HTTP call in the codebase sets a connect- or read-timeout, so they inherit the JDK default of "wait forever". Why this matters beyond the two methods: twelve of these sit on the *blocking* GUI startup path, ServerPackCreator.kt:228 -> ApiWrapper.stageTwo() -> VersionMeta.init -> checkManifests(). A host that DROPs rather than REJECTs leaves the splash screen stuck at 20 % indefinitely, recoverable only by killing the process. The fixture stubs the timeout properties explicitly -- it does **not** use a bare `mockk(relaxed = true)`, which answers `0` for an `Int`, and `0` is the JDK's "wait forever". A relaxed mock would reproduce the very defect these guards exist to catch, so they would stay red against fixed code and prove nothing. That is why the settings group landed first: the guard needs the real property names to reference. The guards use a bounded wait on a daemon thread rather than a direct call, because the defect is an *infinite* wait -- calling inline would hang the whole suite instead of failing one test. The 15s threshold is orders of magnitude above the timeouts configured here, so only an unbounded wait can trip it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Verbatim move of `checkManifest` and both `updateManifest` overloads out of VersionMeta and into `versionmeta/ManifestUpdater`; VersionMeta keeps `checkManifest` as a one-line private facade and stays the class its collaborators use. Behaviour-preserving, deliberately to the letter: the version-counting `when`, its `var countOldFile/countNewFile` accumulators, the LegacyFabric equal-count-but-different-first-version nudge, the SAXException restore-from-jar branch and every log line moved across unchanged. No existing test's assertion, argument or expected value changed -- the whole api/app/clientside suites pass untouched. Two details worth recording: - `JarUtilities.copyFileFromJar` is now handed `ManifestUpdater::class.java` instead of `VersionMeta::class.java`. Identical by construction: it calls `identifierClass.getResourceAsStream("/$fileToCopy")` with an *absolute* resource path, which delegates to the classloader, and both classes share one. - Four imports in VersionMeta became unused with the move (JarUtilities, create, readText, org.w3c.dom.Document) and were dropped. Kotlin does not warn on those, so they would have lingered. Why extract at all: the next commit needs to pin *how many HTTP requests a manifest check costs*, and nothing about that was reachable from a test while the logic lived in VersionMeta -- it resolves its twelve URLs from VersionMetaConfig constants and performs the entire refresh inside its constructor, so there is no seam to point at a local server. This creates one. Measured: api 319 / app 108 / clientside 88, 1 skipped, all green before and after. Compiler warnings in -api: 21 before, 21 after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Six guards on ManifestUpdater against a local com.sun.net.httpserver.HttpServer (JDK built-in, no new dependency). Three land RED: aManifestCheckCostsOneRequest FAILED expected 1, was 2 anAbsentManifestIsDownloadedInOneRequest FAILED expected 1, was 2 anUnchangedManifestIsFetchedConditionally FAILED expected not <null> A check costs two requests, not one: a reachability pre-check whose response body is discarded, then the real fetch. Worse than doubling the count -- the pre-check calls disconnect() without draining the body, so the connection cannot be pooled and the real request pays a fresh TCP and TLS handshake. Twelve manifests therefore cost 24 requests and 24 handshakes across seven hosts before the splash screen moves past 20%. And nothing asks to be told only about changes: no If-Modified-Since, so every startup re-downloads all twelve manifests in full (~490 KB) and parses both the old and the new copy just to compare version counts, then discards the result -- which is the common case, because the manifests rarely change. The other three pin the behaviour that must survive the fix, and pass now: refresh when upstream has more versions, ignore upstream when it has fewer, and leave the local file alone otherwise. Note the honest caveat that aNotModifiedResponseLeavesTheLocalManifestAlone passes today for a *different* reason -- equal counts mean no refresh, since there is no 304 to short-circuit on yet. It becomes a guard on the 304 path in the next commit; it is not evidence of anything today. Pinning cost rather than wall-clock deliberately: a timing assertion would be flaky and would not say *why* startup is slow. Request counts do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the three guards from the previous commit green. Two changes to ManifestUpdater.checkManifest: 1. Both isReachable pre-checks are gone. Each was a full GET whose body was discarded, followed immediately by the real fetch -- and because it called disconnect() without draining, the connection could not be pooled, so the real request paid a fresh TCP and TLS handshake. Startup went from 24 requests to 12 across seven hosts. The absent-manifest branch now attempts the download and reports its failure, which is one request instead of two and names the actual error rather than "unreachable"; updateManifest returns Boolean to carry that. 2. The check sends If-Modified-Since from the local file's mtime, and returns immediately on 304 -- no body, no parse, no rewrite. Measured against the real upstreams (curl, both plain and conditional), because whether this pays off is a fact about their servers, not about our code: honour If-Modified-Since -> 304/0 bytes launchermeta.mojang.com version_manifest.json 206,986 -> 0 meta.fabricmc.net intermediary 56,270 -> 0 maven.fabricmc.net fabric-loader 9,381 -> 0 maven.fabricmc.net fabric-installer 2,516 -> 0 ignore it, still answer 200 with the full body files.minecraftforge.net, maven.neoforged.net (x2), maven.quiltmc.org (x2), meta.legacyfabric.net (x2), maven.legacyfabric.net So 275,153 of 488,038 bytes per startup, ~56%, plus the 12 requests already saved above. Not the whole set, and deliberately not special-cased per host: a server which ignores the header answers 200 and every line below runs exactly as before, which is what keeps this an optimisation rather than a new policy. Follow-up worth having -- files.minecraftforge.net ignores If-Modified-Since but DOES honour If-None-Match against its ETag (verified: 304/0), another 121,492 bytes, the second-largest manifest. That needs somewhere to persist an ETag per manifest, so it is its own change. Using the local file's mtime as If-Modified-Since is conservative-safe: it can only ever cause a redundant 200 (our write time is >= the server's Last-Modified at the time), never a missed update, and the version-count comparison still gates every replacement. One behaviour is deliberately preserved and guarded in the commit that follows: being offline stays a WARN. Dropping the pre-check moved that case onto the IOException path, which would otherwise have logged twelve ERRORs with stack traces on every networkless launch and buried any genuine failure. Reaching the host and understanding its answer are now caught separately -- connection failure warns (with the exception at DEBUG), an unparseable manifest still errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Definition-of-done paperwork for the two phases on this branch, plus three deferred items. Root CLAUDE.md: four rows in the API behaviour-change table (timeouts on every published HTTP call; why openTimedConnection returns URLConnection and must not be narrowed; the manifest-refresh request/byte reduction and its one visible log change). Refactor-state table: api 309 -> 326, status date to 2026-08-17. serverpackcreator-api/CLAUDE.md: NetworkConfig added to the settings-group notes (now 9 groups), and two landmines -- never open a connection outside WebUtilities.openTimedConnection/openTimedStream, and ManifestUpdater's one-request-per-check rule with the WARN-not-ERROR offline logging. REFACTOR-LOG.md: the narrative, including the four findings from the initial investigation that were **wrong or overweighted** and got corrected before any code was written. Recorded deliberately -- the wrong versions were stated out loud, so the correction belongs in the history rather than being quietly dropped. BACKLOG.md, three new items: B30 If-None-Match for the Forge manifest -- measured, another 121,492 B (57 % of what still transfers) but ~0 ms of startup, because the twelve checks run concurrently and the critical path is LegacyFabric at ~330 ms for 498 bytes. Only matters below ~4 Mbit/s, and only for one host. Deferred, not rejected: the bandwidth is real on a metered connection. B31 take the manifest refresh off the blocking startup path -- the larger prize the same measurement exposed (~392 ms -> ~0). Needs VersionMeta's construction contract weakened, so the grinder and the web version-schedule have to be checked against it first. B32 hasteBinPreChecks reading a whole file to measure its length. One correction not caused by this branch: the app row said 102 tests, the suite runs 108. Pre-existing drift, corrected to the measured number without trying to reconstruct which six were added when. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes audit findings F3 (documentation half) and F4. F4 — `ManifestUpdater` was extracted as a **public** class with a **public** `checkManifest`, in a commit labelled a verbatim move whose message did not mention the widening. `serverpackcreator-api` is published and its public surface is a stated plugin-compatibility constraint, so that added an exported type for what is an implementation detail of `VersionMeta`. Verified before narrowing: the only users are `VersionMeta` (same package) and `ManifestUpdaterTest` (same package, same module), so `internal` costs nothing. The root CLAUDE.md row that described it as "new exported" is corrected to "internal". F3 — the landmine claimed `url.openConnection()`/`openStream()` outside the opener "was every single call site" before 2026-08-17, while two genuinely unbounded network calls were still live. It now: - names both sanctioned routes and why there are two (a settings group cannot reach `WebUtilities` without closing a construction cycle; `-app`'s `VersionChecker` is abstract with a no-argument constructor); - lists the two remaining unrouted sites, `ServerPackCreatorPlugin.kt:68` and `ClassUtilities.kt:60`, as benign because they read a `jar:` URL rather than a socket; - says plainly that it once overstated its own coverage, and tells the next reader to grep rather than trust the list. Two testing traps recorded while there, both learned the hard way on this branch: - the first timeout pin does **not** go red -> green (its `mockk(relaxed = true)` fixture answered 0, the JDK's "wait forever", so it had to be edited); the later pins were written against existing signatures so they do, and are the ones to copy; - a hang guard's bound must not equal the timeout it measures, or it races between "gave up as configured" and "waited forever". Plus a new behaviour-change row for the two calls bounded in the preceding commits. No behaviour change here: narrowing visibility to a type nothing outside the module references, and documentation. `./gradlew build` green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Seven guards on ConfigEditorViewModel. Three land RED: aVersionTripleIsProbedOnlyOnce FAILED 20 probes, expected 1 anUnchangedModpackDirectoryIsReadOnce FAILED 20 reads, expected 1 aDirectoryWithoutManifestsFallsBackToItsName FAILED 5 reads, expected 1 Both costs are paid per debounce tick, per open tab, and the debounce is restarted by a document change in *any* field (ConfigEditor.kt:80 -> checkAll() -> TabbedConfigsTab.kt:229). So every time the user pauses typing for half a second, each open config tab sends an HTTP request to the modloader's maven for the installer URL, and re-parses the modpack's launcher manifest into a Jackson tree. Neither input has changed; both answers are pure functions of state that is sitting still. The manifest is the bigger of the two -- a real CurseForge minecraftinstance.json is multi-megabyte, and this repo's own fixture is 2.7 MB. The other four pass now and exist to constrain the fix rather than to demonstrate the defect: - eachDistinctVersionTripleIsProbedOnItsOwn -- the memo must key on all three versions, not collapse to "asked once, answered forever". - aFailedProbeIsRetried -- deliberately asymmetric. A published installer does not vanish, so a success is safe to keep; a failure may only mean the network blinked, and caching it would leave the editor stuck on "server unavailable" until restart. - aChangedManifestIsReadAgain -- a user editing their modpack while the editor is open must see the new name, so the memo has to invalidate on mtime. - aDirectoryWithoutManifestsFallsBackToItsName -- also covers the no-manifest fallback to the directory name. Pinning call counts, not wall-clock: the defect is redundant work, and a count says so exactly where a timing assertion would be flaky and silent about the cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the three guards from the previous commit green by memoizing both answers in ConfigEditorViewModel. Both were recomputed on every debounce tick, per open tab, from inputs that had not changed. The network probe. `serverDownloadable` is an HTTP request to the modloader's maven, and its only caller is the 500 ms debounce restarted by a document change in any field. Now cached per (minecraftVersion, modloader, modloaderVersion). Measured earlier on this branch, that request is ~234 ms against files.minecraftforge.net -- so a user pausing while typing was paying that per tab, repeatedly, to decide whether to show one warning label. **Successes are cached, failures are not:** a published installer does not vanish, but a `false` may only mean the network blinked, and remembering it would leave the editor insisting "server unavailable" until restart. Pinned both ways. The manifest parse. `checkManifests` builds a Jackson tree from the launcher manifest; now re-read only when the fingerprint of the six candidate files changes (existence, size, mtime). Measured against this repo's own CurseForge fixture: fixture 2,715,835 bytes full parse (before) 4.70 ms per tick 6-stat fingerprint 0.021 ms per tick ratio 221x Honest reading of that: the latency saved per tick is modest -- 4.70 ms, not the tens of milliseconds a 2.7 MB document intuitively suggests, because Jackson is fast. The garbage avoided is the real gain, since the old path allocated and discarded a tree of that whole document on every keystroke-pause, per tab. The network probe above is by far the larger of the two wins. The fingerprint reads `ConfigurationHandler.manifestCandidates` rather than listing the paths itself, so it cannot drift from the files actually consulted -- a drifted list would make the memo miss real edits, which is the failure `ManifestCandidatesTest` guards in `-api`. Both caches are `ConcurrentHashMap`-backed: the check walks the open tabs on a `parallelStream`, so several threads ask at once. Measured: api 329 / app 108 / clientside 88, all green. -app warnings 35 -> 35. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>anUnchangedSuggestionListIsParsedOnce FAILED "An unchanged property must not be re-parsed" Red on **identity**: two distinct TreeSet instances come back whose *contents are identical*. That is exactly why identity is the assertion and a value comparison would have guarded nothing. SuggestionProvider re-reads and re-parses its autocomplete property on every document event, on the EDT. For the clientside-mods field that property is the ~550-entry fallback list, so each character typed costs a property read, a split(",") and 550 sorted inserts into a fresh TreeSet before one suggestion can be shown -- and the answer cannot have changed unless the property did. Note what is deliberately *not* asserted: the property read count stays at 20, one per query. Reading it is a map lookup, and keying the memo on the raw value is what lets a saved suggestion-list be picked up with no change-listener. Rebuilding the set was the cost. An earlier draft of this guard demanded one read and was simply wrong -- it pinned a claim the design does not make, and no correct implementation could have satisfied it. Three guards pass already and constrain the fix: eachCallerGetsItsOwnMutableSet -- the load-bearing one. Every production caller mutates the returned set and persists it: ConfigEditor.saveSuggestions adds the current field value, InclusionsEditor.saveSuggestions adds and removeIfs. So caching must cache the *parse* and keep handing out a fresh owned copy; caching the instance would let those mutations corrupt the source and accumulate across calls. aChangedSuggestionListIsPickedUp -- saving suggestions writes the property back, so a memo keyed on its raw value must invalidate immediately. anAbsentPropertyYieldsNoSuggestions -- no suggestions, not the literal "null". Headless: only the suggestion source is exercised, no popup is shown. The provider is built against a real JTextArea because it registers listeners on one, which constructs without a display. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the previous pin green, unedited. Three changes to SuggestionProvider, all on the per-keystroke path: 1. `parsedSuggestions()` reuses its result until the property changes. The memo is keyed on the raw property value, not a change-listener: saving suggestions writes it back through storeGuiProperty, so comparing the string is both the cheapest check and the one that cannot miss an update. The property read itself stays per-call on purpose -- it is a map lookup; rebuilding a 550-entry sorted set was the cost. `allSuggestions()` still returns a fresh TreeSet, which is load-bearing: every production caller mutates the result and persists it. Caching the *instance* would let those mutations corrupt the source and accumulate. The hot path reads the parsed set directly, since it only reads and needs no copy. 2. showPopup no longer calls updateUI() on the list and the menu. updateUI() re-installs the look-and-feel delegate and exists for a LAF *change*; it ran on every keystroke. Replaced with revalidate/pack/repaint, which is what new content actually needs. 3. The `\W` word-boundary check is a companion constant instead of `"\\W".toRegex()` compiled per keystroke. Deliberately NOT done: a `TreeSet.tailSet(prefix)` prefix-walk instead of the linear startsWith. It looks like the obvious optimisation and it is a correctness trap -- the match is case-INsensitive while the set's ordering is case-sensitive, so matches are not contiguous and tailSet("op") would skip "OptiFine"; making the set case-insensitive instead would silently deduplicate entries differing only in case. A linear scan over a few hundred already-parsed strings is microseconds. Recorded in a comment so nobody "optimises" it into a case-folding bug. GUI-verified: the popup resizes with its content, 56x85 px at 5 matches -> 54x34 px at 2, correctly filtered, first row preselected, positioned at the caret. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Definition-of-done paperwork for Phase 2. serverpackcreator-app/CLAUDE.md: two new sections. The check timer is a 500 ms debounce on the typing path running for every open tab, so what it does per tick is what matters -- both memos, the success-cached/failure-retried asymmetry, and the rule that the manifest fingerprint must read ConfigurationHandler.manifestCandidates rather than its own copy of the paths. Also records that the timer's ten `launch { }` blocks inside `runBlocking { }` are *not* concurrent (single event-loop dispatcher), which is the only reason the shared errors list is safe -- making them concurrent would introduce a race. Second section covers SuggestionProvider: the parse cache keyed on the raw property value, why allSuggestions() must keep copying, the tailSet landmine (case-insensitive match over case-sensitive ordering, so matches are not contiguous and tailSet("op") skips OptiFine), and revalidate/pack/repaint instead of updateUI(). It also records **how** the popup was GUI-verified, because the technique is reusable: osascript has no Accessibility permission here, so no synthetic clicks or keystrokes. A throwaway JUnit harness drove Swing from inside the test JVM (-app tests are not headless), inserting characters on the EDT and logging each visible JList's row count and preferredSize while screencapture took stills. Evidence: the popup resizes with its content, 56x85 px at 5 matches -> 54x34 px at 2, correctly filtered, first row preselected, positioned at the caret. Focus must be re-asserted before each burst -- the popup only shows while the component isFocusOwner, and a first attempt looked like a failure when it was merely unfocused. Harness deleted. REFACTOR-LOG.md: the narrative, the measured table, and the guard that was wrong first -- it verified the property-read count, a claim the design deliberately does not make. Rewritten to assert reuse by identity, then deliberately re-broken to check it had teeth: it fails with two distinct TreeSet instances whose contents are identical, which is why identity is right and a value comparison would have guarded nothing. Refactor-state table: api 326 -> 329, app 108 -> 118. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Three guards, all RED: theExclusionFilterIsReadOncePerGeneration 7 reads, expected 1 regexEntriesStillClassifyEveryMod 5 reads, expected 1 aMalformedRegexEntryIsSkippedRatherThanAborting PatternSyntaxException escaped `compileModList` compares every mod against every clientside-list entry. The default list ships ~550 entries, so a 300-mod pack performs on the order of 165,000 comparisons per generation -- and each one re-reads `apiProperties.exclusionFilter`, whose getter calls `PropertyStore.acquire`, which reads `java.util.Properties` (a synchronized `Hashtable`) twice. Two synchronized map lookups per comparison, for a value that cannot change mid-generation. The read counts above match exactly: 1 for the log line plus one per comparison (3 mods x 2 entries, and 4 mods x 1 entry). Note this is the **default** START path, not just REGEX -- the filter default is START (GenerationConfig.kt:815), so the earlier claim that the per-comparison `entry.toRegex()` was the main cost had it backwards: that one only affects users who chose REGEX or EITHER, while this affects everyone. The third guard covers a real defect on the REGEX path: `entry.toRegex()` per comparison means one malformed user entry throws PatternSyntaxException out of compileModList and aborts generation, instead of reporting one unusable entry and applying the rest. Fixing that is a behaviour change, so it lands as `fix:`. Pinned by read count, not wall-clock: the count is the defect and it is deterministic. Separate class from ModListCompilerTest so the existing tests keep their real ApiWrapper graph. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the three guards from the previous commit green. `FilterMatcher` is built once per compileModList and reused for every comparison; the exclusion-filter setting is read once, and REGEX/EITHER patterns are compiled once per entry instead of once per comparison. **The primary value is the bug fix, not the speed — and the measurement says so.** Under REGEX/EITHER, `entry.toRegex()` ran per comparison, so a single malformed user entry threw PatternSyntaxException straight out of compileModList and aborted generation instead of reporting one unusable entry. Now the compile happens up front, a bad pattern is logged once and skipped, and every other entry still applies. Measured at realistic pack scale (300 mods x 550 default entries = 165,000 comparisons), because the earlier estimate was wrong and should be corrected on the record: 2 synchronized Properties lookups per comparison : 3 ms (default START path) one Pattern.compile per comparison : 20 ms (REGEX/EITHER only) So ~3 ms for most users and ~23 ms for regex users, not the substantial win the plan implied when it called this "the one that affects everyone". `Hashtable.get` is fast and its monitor is uncontended, so 330,000 of them simply do not cost much. The change is still worth keeping -- it is simpler, hoists genuinely invariant work, and fixes the abort -- but it should not be sold as a performance result. That also corrects the previous commit message, which framed the property read as the main cost. It is real and it is now gone; it was just never large. Behaviour changes, both deliberate: a malformed regex entry is skipped rather than fatal, and its error is logged once per generation rather than once per mod. Measured: api 329 -> 332, app 118, all green. Existing ModListCompilerTest untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Adds a defaulted `openZip: (File) -> ZipFile` constructor parameter, and routes all four `ZipFile(...)` sites through it. Behaviour-preserving: the default is exactly `{ ZipFile(it) }`, and every method still opens the archive the same number of times. It exists so the *next* commit's guard can count those opens. Without it the duplication is invisible -- every method returns the same answer whether it opened the archive once or four times, which is precisely how it survived unnoticed. Defaulted, so it is source-compatible: Kotlin still emits a no-argument constructor, and the one construction site (`ConfigurationHandler`) is unchanged. Its own commit rather than bundled with the guard, because it is production code and the guard is not -- a `test:` commit that ships a seam misrepresents its own diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>validatingAnArchiveReadsItOnce 2 opens, expected 1 listingEverythingReadsTheArchiveOnce 2 opens, expected 1 Reading the central directory is the expensive part of inspecting a modpack archive and it scales with the entry count -- measured 79.9 ms for a 10,000-entry archive, and a real export can hold far more. Two sites do it twice over: checkZipArchive opens once for isNotValidZipFile(), then again via getDirectoriesInModpackZipBaseDirectory, when the first open already has every header the check needs. getAllFilesAndDirectories.. delegates to one method for directories and another for files, each opening the archive, where a single pass over the headers partitions both. So ~80 ms wasted per validation and another ~80 ms per full listing, on that 10,000-entry archive. Two guards pass already and constrain the fix: the single pass must agree with the dedicated per-kind methods, and an invalid archive must still be rejected in one read (the error path must not be where a second parse creeps back). Fixture note, found the hard way: zip4j's `addFile` with a path-in-zip writes no explicit **directory** entries, so the first version of these tests saw zero directories and failed for the wrong reason. Built from a real tree with `addFolder` instead. Incidentally that documents why checkZipArchive works without them -- it derives `mods/` from a *file* entry's name, not from a directory entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the two open-count guards green. checkZipArchive validity check and base-directory scan now share one open. Extracted `baseDirectoriesOf(entryNames)` so the scan can work from headers already in hand; getDirectoriesInModpackZipBaseDirectory keeps its published signature and now delegates to it. getAllFilesAndDirectories.. one pass over the headers, partitioned, instead of delegating to the two per-kind methods and paying for two opens. Directories still come first, as they did when they were two calls. Measured on a 10,000-entry archive: one central-directory read is 79.9 ms, so this is ~80 ms saved per validation and ~80 ms per full listing -- and it scales with the archive, which is why it was worth doing properly. One behaviour change on the error path, which the compatibility table records: the two per-kind calls each had their own try/catch, so a failure fetching files still returned the directories and logged twice. It is now one pass, so a failure returns an empty list and logs once. Barely reachable -- both old calls opened the *same* archive -- but an embedder treating a partial list as usable now gets nothing instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>regexVariantListsAreNotSharedBetweenReads FAILED "expected: not same" `GenerationConfig.clientsideModsRegex` and `modsWhitelistRegex` have getters that `clear()` and refill one shared `TreeSet` field on every access, then hand it out. Two consequences, and the second is why this is a defect rather than untidiness: - a caller holding an earlier result has it emptied and rewritten underneath them; - the clear-then-refill is not atomic, so a concurrent reader can observe the set part-way through, i.e. a list that is briefly wrong rather than merely stale. Both are published via `ApiProperties.clientsideModsRegex` / `modsWhitelistRegex`, and the GUI reads settings from a `parallelStream` walk over its open tabs, so concurrent access is not hypothetical. Verified first that nothing depends on the current aliasing: the `private set` is never assigned anywhere in `-api` or `-app`, and the two readers (`clientSideMods()`, `ApiProperties.kt:1276`) both immediately `.toList()`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>anExistingHashIsFoundWithoutScanningTheCollection FAILED findAll() called anUnknownHashReportsNoDuplicate FAILED findAll() called aNullHashReportsNoDuplicate FAILED expected true, was false `existingUploadOf` loads every modpack and compares in memory, on every upload. That is heavier than a row count suggests: `ModPack.serverPacks` is an eager `@DBRef`, its targets eagerly resolve their `RunConfiguration`, and that resolves three more `@DBRef` lists -- so comparing one hash reads a four-collection graph and materialises the ~550 ClientMod documents behind each run-configuration on the default list. The third guard is about semantics rather than cost, and is honestly scoped: with the in-memory scan, `available.sha256 == sha256` is true when *both* are null, so a hash-less upload would be reported as a duplicate of any stored modpack that also has none. **Not reachable from the upload path today** -- `SavedFile.sha256` is a non-null String, so `saveUploadedFile` always passes a real hash. It is guarded because the parameter is nullable, stored documents genuinely can carry a null `sha256` (the no-arg constructor and the non-ZIP sources leave it unset), and Mongo's own `{sha256: null}` query would match those, so the fix has to say no explicitly rather than inherit the right answer. The unused `findAll()` stubs are intentional: without them mockk fails with a missing-answer exception instead of naming the redundant scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the three duplicate-check guards green. `existingUploadOf` calls `modpackRepository.findBySha256` against the newly-indexed field instead of loading the whole collection and comparing in memory. One indexed document lookup replaces a scan that also resolved, per document, the eager `@DBRef` chain ModPack.serverPacks -> ServerPack.runConfiguration -> start-args/clientside-mods/whitelist -- four collections, and the ~550 ClientMod documents behind every run-configuration on the default list, to compare one string. It runs on every upload. Behaviour change, deliberate and narrow: a **null** hash now returns empty instead of matching stored modpacks whose own sha256 is unset. The old `available.sha256 == sha256` comparison called that a duplicate, and a Mongo `{sha256: null}` query would too, so the short-circuit is explicit. Unreachable from the upload path today (`SavedFile.sha256` is non-null), so nothing observable changes for users; it is here because the parameter is nullable and stored documents genuinely carry null. Also unchanged on purpose: first-match-wins is now whichever-document-the-index-returns. Indistinguishable unless two stored modpacks share a hash, which is precisely what this check exists to prevent. Measured: app 120 -> 123, all green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`RunConfiguration.startArgs`, `clientMods` and `whitelistedMods` become `MutableList<String>` embedded in the document. `ClientMod`, `WhitelistedMod`, `StartArgument`, their three repositories and the shared `ModRepository` are deleted. The migration that converts existing databases ships **in this same commit**, deliberately: split apart, the intermediate state is an application whose mapped type cannot read its own stored run-configurations, so neither deploying nor bisecting to it is safe. Why the wrappers were pure overhead: each was a `@Document` whose *only* field was its `@MongoId`. A `ClientMod` document is literally `{_id: "OptiFine"}`, so the eager `@DBRef` resolved to the string it was already keyed by. Three collections and four repositories existed to store nothing. What that cost, per created run-configuration: each list was resolved entry by entry, one `findBy` plus a `save` on a miss. With the default clientside list that is ~550 sequential round-trips to build one configuration, and it is on the request path. Now two calls total -- the duplicate lookup and the save -- pinned by `buildingAConfigurationCostsTwoRepositoryCalls`. It also removes an eager join from every read that reaches a RunConfiguration, which is what made `findAll()` on the server packs or modpacks fan out across four collections. **A real bug goes with it.** The duplicate lookup was `…AndStartArgsInAndClientModsInAndWhitelistedModsIn`, and Spring Data's `In` means "contains any of", not "equals" -- so a configuration could be matched, and reused, because it shared a *single* mod with the one being created. It is now `…AndStartArgsAndClientModsAndWhitelistedMods`, an exact array match. **Four tests were deleted rather than adapted**, and that is the honest signal it looks like: `aKnownStartArgumentIsReplacedByTheStoredEntry`, `anUnknownStartArgumentIsSaved`, `aKnownClientModIsReplacedByTheStoredEntry` and `aKnownWhitelistedModIsReplacedByTheStoredEntry` describe resolution against collections that no longer exist. The two mod-list tests also covered comma-splitting, which is preserved as `clientModsAreSplitOnCommas` / `whitelistedModsAreSplitOnCommas` so no coverage is lost. Every surviving assertion is byte-identical apart from dropping the `.map { it.mod }` unwrapping. The migration, in `web/migration/`: - `RunConfigurationListMigration` is the per-document rewrite, and it is **join-free**: a DBRef's `$id` *is* the value, so `{$ref:"clientMod",$id:"OptiFine"}` becomes `"OptiFine"` without reading the referenced collection -- which also means it still works after those collections are dropped. Tested without a database. - `RunConfigurationListMigrationRunner` applies it on `ApplicationReadyEvent` rather than during context startup, so a slow or briefly unreachable database delays the migration instead of preventing the boot. Element-wise, so an interrupted run is *completed* rather than corrupting a half-rewritten document; idempotent, so a restart costs one read and no writes; failures logged and swallowed; the orphaned collections dropped only after a fully successful pass, because losing the referenced ids first would make the data unrecoverable. - `MongoTemplate`, not the repository, necessarily: the mapped type can no longer read the old shape, which is the entire problem. Frontend, same commit because it is one contract: `types/api.ts` declares `string[]`, and the `.map(entry => entry.mod)` unwrapping in `RunConfigurationCard.vue` and `SubmitModPackForm.vue` (both sites) is gone. The Vitest fixtures move to the new shape; **their expectations are untouched** and all 31 pass. Operators should back up before upgrading, as with any in-place data rewrite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Seven guards over the component that **mutates a user's persisted data** — the only substantial one on this branch that had no coverage, because its pure transformation was well tested and the untested half was the part that talks to the database. What they pin, each of which can lose data if wrong: everyRewriteHappensBeforeAnyDrop asserted on call *order*, since the end state looks identical either way aFailedRewriteLeavesTheOrphanedCollections a half-rewritten database must keep the ids it still needs anAlreadyMigratedDatabaseIsNotTouched no writes, and crucially no drops aFreshInstallDropsNothing an empty pass is not licence to delete oneFailedDropDoesNotStopTheRest an unused collection left behind is harmless; an aborted migration is not anUnreachableDatabaseDoesNotFailStartup it runs on ApplicationReadyEvent onlyTheDocumentsStillInTheOldShapeAreRewritten **Their teeth were verified rather than assumed**, since they were written after the code -- the lesson this project records twice already. Breaking the production code deliberately: drop before the rewrite -> 4 guards fail, incl. everyRewriteHappensBeforeAnyDrop drop when nothing rewritten -> 2 guards fail, incl. aFreshInstallDropsNothing Test-only, now that the seam it needs landed in the preceding commit. It was originally one commit with that seam -- the same `test:`-ships-production violation being fixed elsewhere on this branch, which is a worse failure when it is one's own work than when it is inherited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes audit finding X1, raised by the second pass over the remediation itself. `createHasteBinFromString` opened a bare connection and set the two timeouts by hand, because it needs `HttpsURLConnection` for its POST. It was bounded, so never a hang -- but the landmine had just been rewritten to say exactly two ways of applying a timeout exist, and this was a third. It now calls `openTimedConnection` and narrows the result, so `WebUtilities` contains a single `openConnection()`: the shared opener. Also completes REFACTOR-AUDIT.md with the second-pass results and a status table for all 22 findings across the four branches. Second-pass verification on the final tip: - 1,531 tracked source/doc files scanned for NUL bytes: none. (The first attempt used `grep -qU $'\000'`, which degrades to an empty pattern and "found" 539 matches including every PNG. A check that reports everything is broken, not thorough.) - No unbounded network call remains; the two surviving unrouted sites read `jar:` URLs and are named in the landmine as deliberate exceptions. - The new runner guards were verified to have teeth by breaking the code: dropping before the rewrite fails 4 of 7, dropping when nothing was rewritten fails 2. - Stack: 12 / 8 / 8 / 8 commits, no duplicate subjects, build green. Recorded honestly in the report: while rebasing the stack I used the wrong upstream and dropped seven of the web branch's eight commits, recovering them from the reflog. The correct form is `--onto <new-base> <old-base>` with the parent's *pre-rebase* tip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-3 audit findings Q1, Q2 and Q3 -- the most substantive of the three re-audits, and the only ones no structural check could have found. All three were written alongside their production code, so none ever had a red state: exactly the population this project's conventions single out as highest risk. Found by mutation -- break the production code, see whether the suite notices: Q1 aFailedProbeIsRetried stubbed a failure, then a success, and asserted `true`. Caching failures as well as successes makes the second call short-circuit to `return true`, satisfying that assertion **without probing at all** -- so the guard could not distinguish "re-probed" from "wrongly remembered", which is the entire asymmetric caching decision it claims to protect. Now also verifies the probe ran twice. Q2 theSinglePassAgreesWithTheDedicatedMethods compared both sides `.sorted()`, throwing away the directories-first order the production code promises in a comment and the commit message repeated. Inverting the partition changed nothing. The `.sorted()` calls are gone, so the order is now pinned rather than merely asserted in prose. Q3 aNullHashReportsNoDuplicate stubbed `findBySha256(null)` to return empty and asserted empty -- verifying the mock, not the code. Removing the short-circuit it exists to guard changed nothing. Now verifies the repository is never consulted, which is the actual point: Mongo's own `{sha256: null}` *would* match documents whose field is unset. **The fixes were mutation-tested rather than trusted.** Re-running the three mutations that previously slipped through: cache failures too -> 1 of 13 fails (was: all passed) partition inverted -> 1 of 7 fails (was: all passed) null short-circuit gone -> 1 of 3 fails (was: all passed) Every other new guard on the branch was checked the same way and does bite: the migration rewrite, the manifest candidate order, a shipped timeout default, the allSuggestions copy, the manifest fingerprint, and the runner's drop ordering. Recorded as a landmine in serverpackcreator-api/CLAUDE.md, with the method and the rule it implies: prefer asserting *that a collaborator was or was not called* over asserting a return value a wrong implementation could also produce. `./gradlew build` green; api 339 (1 skip), app 135, clientside 88, frontend 31. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Red pin for iteration-4 audit finding R1. Lands failing, on purpose: the fix is the next commit. `fix(app): look an upload's hash up by index instead of scanning every modpack` put `@Indexed` on `ModPack.sha256` and its repository KDoc states the consequence as fact -- "so this is a single indexed lookup rather than a scan". No index is ever created. Spring Data MongoDB stopped creating annotation-declared indexes automatically in 3.0. Verified from the resolved artifacts rather than from memory: javap -c MongoMappingContext (spring-data-mongodb 5.1.0) the no-arg constructor emits `iconst_0; putfield autoIndexCreation:Z` -- false javap -c DataMongoConfiguration.mongoMappingContext (spring-boot-data-mongodb 4.1.0) PropertyMapper.from(properties.isAutoIndexCreation()).to(context::setAutoIndexCreation), and PropertyMapper skips a null source, so an absent property leaves that default spring-configuration-metadata.json in the same jar spring.data.mongodb.auto-index-creation exists with no default value grep across the repo no property, no MongoMappingContext bean, no AbstractMongoClientConfiguration Two guards, because either alone is worthless. The switch half fails now (expected <true> but was <null>). The entity half passes now and is here to stay honest about *what* the switch will create: it runs Spring Data's own MongoPersistentEntityIndexResolver over the mapped type rather than asserting that an annotation is present, so removing `@Indexed` fails it too. Observed red: theShippedConfigurationCreatesDeclaredIndexes FAILED, anIndexOnTheUploadHashIsResolvedFromTheEntity PASSED -- 2 tests completed, 1 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Correction to the pin in `test(app): pin that the declared upload-hash index is actually created`, which is still red -- and red for the right reason but the wrong file. `getResourceAsStream("/application.properties")` resolves against the *test* classpath, where `serverpackcreator-app/src/test/resources/application.properties` shadows the shipped one. So the guard was asserting about a 720-byte test fixture, not the 1,159-byte file the running app reads, and adding the property to main resources left it failing. Every copy is now enumerated with `getResources` and the test one discarded, with the count asserted so a second shipped copy cannot appear unnoticed. Observed red against the still-unfixed main resource: theShippedConfigurationCreatesDeclaredIndexes FAILED (expected <true> but was <null>), anIndexOnTheUploadHashIsResolvedFromTheEntity PASSED. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-4 audit finding R5. `RunConfigurationListMigrationRunner.COLLECTION` is the literal `"runConfiguration"`, commented as "the collection Spring Data maps `RunConfiguration` to". It is that today. If it ever stops being -- a rename, or a `@Document("…")` naming the collection explicitly -- `findAll` reads a collection that does not exist, returns nothing, rewrites nothing, drops nothing, logs no failure, and the migration reports success. Persisted data silently stays in the old shape, which the mapped type can no longer read. Nothing existing catches that: all seven guards in `RunConfigurationListMigrationRunnerTest` drive the store through this same constant, so they agree with it however wrong it is. The new guard asks Spring Data instead of repeating its naming rule, via a standalone `MongoMappingContext` -- no database needed. **This one lands green, deliberately, and was verified by mutation instead of by a red state.** It guards a latent risk rather than fixing a present defect, so there is nothing to make it fail today; per the landmine in serverpackcreator-api/CLAUDE.md, teeth were checked by breaking the production code: COLLECTION -> "runConfigurations" new guard FAILED (+1 existing, incidentally) @Document("run_configurations") on entity new guard FAILED, all 7 existing PASSED The second is the realistic scenario, and it is caught by this guard alone -- which is the finding, stated as an experiment. The three `ORPHANED_COLLECTIONS` are deliberately not checked this way: their classes were deleted, which is the point of dropping them, so a literal is all that remains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-4 audit finding R3. `fix(app): embed the run-configuration mod lists, and migrate what is stored` changed `RunConfiguration.startArgs` / `clientMods` / `whitelistedMods` from `MutableList<StartArgument>` and friends to `MutableList<String>`, and deleted those three classes. `RunConfigurationController` returns the entity directly under `@RequestMapping("/api/v2/runconfigs")`, so the response body changed from "clientMods": [{"id": 1, "mod": "3dskinlayers-"}] to `"clientMods": ["3dskinlayers-"]` -- on a path carrying an explicit API version. Both published descriptions still documented the old shape: Writerside/api-docs.yaml the three properties `$ref`-ed #/components/schemas/StartArgument, /ClientMod and /WhitelistedMod, and defined all three -- schemas for classes that no longer exist. Now `items: {type: string}`, and the three orphaned definitions are gone (verified referenced from nowhere else; the file still parses as YAML). topics/*.md 30 sample arrays across Run-Configs.md, Server-Packs.md and Modpacks.md rewritten to string elements, keeping the `...` elision markers the samples already used. `RunConfiguration` is embedded in ServerPackView, which is why the other two files were affected. `serverpackcreator-app/CLAUDE.md` claimed "The JSON shape is part of this contract" but listed only `types/api.ts` and the two Vue consumers, so the obligation it stated was met while the documented external contract was not in the list. It now names the help module too, and says plainly that nothing in the build can catch a miss: `serverpackcreator-help` is not a Gradle module and `springdoc` is commented out, so that spec is a hand-maintained snapshot to be treated as source. Scoped honestly: the spec carries older drift of its own -- it types `id` as `integer/int32` where the entities use `@MongoId(FieldType.STRING)` -- which predates this branch and is left alone and recorded rather than silently "fixed" under an unrelated heading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Red pin for iteration-5 audit finding S1 -- a regression introduced by iteration 4's own fix, `fix(app): create the indexes the web module declares`. `spring.data.mongodb.auto-index-creation=true` does not merely permit index creation: it makes `MongoTemplate`'s bean creation perform it during context refresh, so the database must be reachable *then*. Measured against an absent MongoDB, one variable changed, same context: auto-index-creation=false context starts, ModPackService resolves auto-index-creation=true "Waiting for server to become available for operation createIndexes with ID 3. Remaining time: 29997 ms", then MongoTimeoutException -> mongoTemplate fails -> refresh CANCELLED, context dead So web mode went from "starts, logs a connection error, works once the database appears" to "hangs 30 s and dies if the database is not up yet". `docker/docker-compose.yml` starts the app alongside its `db` service, so losing that race is the normal first boot. It also contradicts a decision this module already documented, about the migration runner: "applies it on ApplicationReadyEvent, not during context startup, so an unreachable database delays the migration instead of blocking the boot." **Why iteration 4 missed it, which is the more useful half.** `WebServiceContextTest` boots the real context with no database and would have failed in one second -- but `src/test/resources/application.properties` shadows the shipped file, so the setting never reached the context under test. Iteration 4 even worked around that shadowing to *read* the shipped file, and treated it as plumbing rather than as the reason its change was unverified. A guard that reads a shipped file is not a test that runs with it. This pin closes that: the shipped file is read *and* its value is handed to a real context boot, so the combination that broke is the combination asserted. Observed red: theShippedConfigurationDoesNotCreateIndexesDuringRefresh FAILED (auto-index-creation=true), theContextStartsWithoutADatabase PASSED -- the boot half passes because the annotation overrides the property to `false`, which is exactly the measurement above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-6 audit finding T1. This branch adds five `@Component`s -- `RunConfigurationListMigration`, `RunConfigurationListMigrationRunner`, `MigrationStore`, `IndexStore` and `DeclaredIndexCreator`. All five are well unit-tested, and every one of those tests **constructs the class directly**, so they pass whether or not Spring ever creates the bean. Nothing asserted registration: `WebServiceContextTest` owns wiring and stopped at the controllers and four services. The failure mode is silent, and unequal in cost: DeclaredIndexCreator inert the index is never created -- iteration 4's R1 again, with its own guard still green RunConfigurationListMigrationRunner inert persisted data is never migrated, and the mapped type cannot read the old shape, so reads fail on real data while the whole suite is green Two guards, in the test that already owns wiring: the five beans resolve, and both deferred jobs listen for `ApplicationReadyEvent` specifically -- asserted on the *beans Spring holds*, not on the classes, because "registered" and "deferred" are the two halves that were missing. Running either during refresh is what made an unreachable database cancel the context earlier in this session. **Lands green, mutation-verified rather than red-first**, per the landmine in serverpackcreator-api/CLAUDE.md -- there is no present defect to fail against (the beans are genuinely wired; `WebService` is `@SpringBootApplication` in the parent package of both): drop @Component from DeclaredIndexCreator both new guards FAILED listener -> ContextRefreshedEvent deferral guard FAILED, naming both events Not a live defect. It is the guard gap that let R1 and S1 through twice in one session, which is the reason to close it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-6 audit finding T2, and lands the iteration-6 report. `RunConfigurationCard.test.ts` asserted `expect(wrapper.vm.clientMods).toEqual(['optifine'])` -- the value it had just passed in, through a card that assigns `this.clientMods = runConfig.clientMods` unchanged. That holds for any element type, so it could not see the one thing the DBRef-to-embedded change risked: the card consuming the wrong element shape. Mutation, before: revert both render sites to the pre-branch object shape, `clientMods.map(m => m.mod).join(', ')` -- **all 31 frontend tests passed** while the card rendered `undefined, undefined`. Mutation, after: the same mutation **fails** the new guard, which asserts the rendered text contains each list's values and contains neither `undefined` nor `[object Object]`. The old pass-through assertion still passes under it, which is the finding restated as evidence. `startArgs` already had a rendering assertion, which is why the mutation had to touch the other two lists to stay invisible. The test's doc comment was stale in the same direction -- it described the card as flattening "the nested `{argument}` / `{mod}` objects from the backend", which is the shape this branch removed. It now says the lists are plain strings and why that means every one of them needs a rendering assertion. **Scope corrected in the report rather than left overstated:** T2's first draft accused `SubmitModPackForm.test.ts` of the same weakness. It is not -- its `clientMods` is a *derived* join performed by `selectedRunConfiguration`, so the assertion is shape-sensitive, verified by mutating that line and watching the guard fail. Only the card was blind. Frontend suite 31 -> 32, green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-4 finding R3's stated caveat, and iteration-7 finding U1. **Pre-existing drift, not caused by the performance work** -- recorded as such when R3 was raised, and fixed now rather than left, because R3 corrected the run-configuration shape in this same file and a spec that still mistypes every id is no more usable than one that mistyped the mod-lists. Every persisted entity keys on `@MongoId(FieldType.STRING)` -- verified per class in `ZipResponse`, `RunConfiguration`, `ServerPack`, `ErrorEntry` and `QueueEvent` -- while the spec described ids as `integer`. Corrected: 10 path parameters {id}, {modPackId}, {runConfigurationId} -> string 10 properties ZipResponse.modPackId/.runConfigId/.serverPackId, RunConfiguration.id, ServerPack.id, ServerPack.fileID (int64, not int32 -- missed by the first pass), ErrorEntry.id, QueueEvent.id/.modPackId/.serverPackId **Deliberately untouched, because they are genuinely numbers:** `ServerPack.size`, `.downloads` and `.confirmedWorking` are `Int` in the entity. **Deliberately untouched, because they cannot be verified:** `ServerPackView.id` and `ModPackView.id`. Those two schemas describe classes that **no longer exist anywhere in the module** -- the same dangling-schema defect R3 fixed for `ClientMod` / `StartArgument` / `WhitelistedMod`, except these predate this branch by an unknown margin. Guessing their id type would be inventing a contract; they are reported instead. Whoever owns the web API should decide whether those schemas and the paths referencing them still describe anything real. Nothing in the build can verify any of this: `serverpackcreator-help` is not a Gradle module and `springdoc` is commented out in `serverpackcreator-app/build.gradle.kts`. The file still parses as YAML, which is the only mechanical check available. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-7 finding U2 by deciding it rather than half-doing it. The rule earned by iteration 6: assert what a component renders, not the prop you handed it. `RunConfigurationCard`'s old guard compared `wrapper.vm.clientMods` against the payload it had just passed in, through a card that assigns the array unchanged -- so it held for any element type, proven by mutation (the pre-embedding object shape rendered `undefined, undefined` with all 31 tests green). Recorded with the counter-example too: `SubmitModPackForm`'s equivalent assertion *is* shape-sensitive, because its value is derived by `.join(', ')` rather than passed through. `SubmitModPackForm`'s three tooltip render sites stay untested, by the same call already made for the three tables: they sit behind two layers of lazy Quasar rendering (`QBtnDropdown` renders on open, `QTooltip` on show), so reaching them means driving and stubbing both, for display-only duplicates of data already guarded where it is derived. The tempting cheap alternative -- asserting the shape of `runConfigurations` -- would only re-assert the axios mock, which is exactly the defect the entry above is about. Written down so the gap is a decision with a reason rather than an oversight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Iteration 7 asked the question the first six did not: does this branch still do what `develop` did? **Answer, by measurement: yes.** develop's unmodified test trees, run against HEAD's production code in a detached worktree -- api 309 (1 skip), clientside 88, app 93 -- **490 guards, zero failures**. Exactly two develop-era files could not compile, both at the branch's two deliberate shape changes: `ConfigEditorViewModelTest` was adapted by passing the two new constructor collaborators as relaxed mocks, with every assertion byte-identical (diffed to prove it) and all 7 guards green; and `RunConfigurationServiceTest`, which asserts the join behaviour the branch removed by design -- its three retired guards are enumerated in the report, and its replacement covers strictly more. Coverage was measured rather than assumed, including the correction that a class-name grep under-reports: `QuiltPackScanner`, the one genuinely algorithmic rewrite, is exercised only *indirectly* -- 30 `quilt` references in `ModScannerSidenessTest` -- and the `FilterMatcher` rewrite is covered across all five exclusion-filter modes, not just the common one. **And what only a real runtime can answer was asked of one.** The actual bootJar in `-web` mode against MongoDB 8.0.5 in Docker, seeded with pre-branch shaped documents: the `sha256` index really exists, the migration converted the legacy document to `["OptiFine","Sodium"]` while leaving the already-embedded one untouched (`Migrated 1 of 2`), all three orphan collections were dropped after the rewrite, and `GET /api/v2/runconfigs/all` returned the documented shape with a *string* id -- confirming the spec correction empirically. With no MongoDB at all, Tomcat and `Started ServerPackCreatorKt` come **first** and the failed index attempt after, which is S1's fix confirmed in production form. Both techniques are now conventions in CLAUDE.md, with the worktree recipe, because they are cheap, repeatable, and were missing. Two findings, filed as B33/B34 rather than fixed -- **neither caused by this branch**: B33 the web app appears to use MongoDB's default `test` database, not the configured one. HIGH and urgent-shaped: reproduced three times, `credential=null`, and a seeded `serverpackcreatordb` left untouched while `test` received everything. `WebserviceConfig.kt` has a zero diff against develop. Filed with full reproduction because it lives in config plumbing this branch never touched, and because it would make this migration a silent no-op on a correctly-configured instance. B34 the same URI does not reach the MongoClient in tests, which explains iteration 5's dead end -- the property demonstrably reaches the environment while the client keeps a 30 s timeout. Both attempts to shorten it are reverted; the ~30 s per no-database context boot stands as a stated cost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Red pin. Lands failing, on purpose: the fix is the next commit. Spring Boot **4.0.0 removed** `spring.data.mongodb.uri` -- the key ServerPackCreator writes. Its metadata carries `deprecation.level = "error"`, `replacement = "spring.mongodb.uri"`, and the connection properties moved from `DataMongoProperties` (`@ConfigurationProperties("spring.data.mongodb")`, which no longer has a `uri` at all) to `MongoProperties` (`@ConfigurationProperties("spring.mongodb")`). A removed key does not warn. It is simply not bound, so Boot falls back to `spring.mongodb.uri`'s own default, `mongodb://localhost/test`. Measured with the real bootJar, same URI, only the key differing: spring.data.mongodb.uri hosts=[localhost:27017] credential=null spring.mongodb.uri hosts=[127.0.0.1:27017] credential=MongoCredential{userName='spcuser'…} The host substitution is the tell -- `127.0.0.1` was configured, `localhost` is Boot's literal default. Consequences, all silent: writes land in `test` rather than the configured database, existing data is invisible, authentication is skipped, and every `SPC_DATABASE_*` container variable is ignored because `init-spc-config/run` writes the same dead key. This affects **shipped** versions -- `main` is on spring-boot-starter-web 4.0.3, develop/alpha on catalog springBoot 4.1.0. It is also the answer to the question `claude-docs/DOCKER-MONGO-INVESTIGATION.md` left open ("Still open: what makes the property absent entirely"), and therefore explains the reported symptom that SPC "appears to connect to localhost": from Spring's point of view the property genuinely is absent. Two guards, because one of them would have been useless: theConfiguredDatabaseUriReachesTheDriver registers the URI under `WebserviceConfig.DATABASE_URI_KEY` via @DynamicPropertySource -- keyed by the production constant, never a repeated literal -- and asserts host, credentials and database on Boot's own resolved MongoConnectionDetails. No database needed. Uses 127.0.0.1 deliberately: `localhost` is Boot's fallback, so only a different host proves binding. theKeyIsNotRetiredBySpringBoot reads Boot's own spring-configuration-metadata.json off the classpath and fails if the key SPC writes is absent or carries deprecation level "error". This one generalises: the next Boot release to retire a key we depend on fails here at build time instead of silently redirecting a production database. Observed red: host `[localhost]` vs expected `[127.0.0.1:27017]`, and "is retired by Spring Boot (deprecation level 'error'). Replacement(s): [spring.mongodb.uri]". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes the finding the previous commit's guards pin, turning both green. `DATABASE_URI_KEY` becomes `spring.mongodb.uri`. Spring Boot 4.0.0 retired `spring.data.mongodb.uri` -- metadata `deprecation.level = "error"`, replacement `spring.mongodb.uri` -- and a retired key is not bound at all, so Boot fell back to its own default `mongodb://localhost/test`. Every configured host, credential and database was ignored, silently, including every `SPC_DATABASE_*` container variable. **Existing installations keep working without anyone editing a file.** The getter reads the live key, falls back to `LEGACY_DATABASE_URI_KEY` when it is absent, and re-writes what it found under the live key -- so Spring never sees the retired name again. That fallback, not a `MigrationManager` step, is what protects people: migrations are skipped entirely on dev, alpha and beta builds, so a migration method alone would miss everyone not on a release. Grouped with it, because the fallback requires it: the URI check was `!startsWith("mongodb")`, which **accepts the degenerate `mongodb:`** a partially-configured container used to produce. With two keys in play a bad value must be rejected the same way whichever key carried it, so the check now enumerates `mongodb://` and `mongodb+srv://`. `mongodb+srv://` is pinned too -- it is what a hosted Atlas cluster hands out, and a prefix check would have kept accepting it by luck. Verified end-to-end against MongoDB 8.0.5, not just by unit test. Same jar, same URI value: before, legacy key hosts=[localhost:27017] credential=null after, live key hosts=[127.0.0.1:27017] credential=MongoCredential{userName='spcuser'…} after, legacy key hosts=[127.0.0.1:27017] file gains spring.mongodb.uri beside the old one and the conclusive one -- a document seeded into a **non-default** database `spc_e2e`, with the app configured through the **legacy** key only, came back from `GET /api/v2/runconfigs/all`. Before this it would have queried `localhost/test` and returned `[]`. Also updated, all of it part of the same contract: `init-spc-config/run` writes the live key (its harness `docker/tests/init-spc-config-test.sh` re-run in the production base image, 10/10 pass), `HELP.md`'s sample block and settings table, `README.md`'s setup step plus an upgrade note, and the test fixtures. **One fixture stays on the legacy key on purpose** -- `serverpackcreator-api/src/test/resources/serverpackcreator.properties`, commented as such -- so the fallback is exercised through the real `ApiProperties` chain rather than by unit tests alone. Two existing assertions in `WebserviceConfigTest` changed, which is the stop-and-flag signal and is deliberate here: they asserted the *written key literal*, and the written key is precisely what this fix changes. Both now assert through `WebserviceConfig.DATABASE_URI_KEY`, and the key's own value is pinned separately by `DatabaseUriPropertyTest` so nothing passes by construction. `./gradlew build` green, counts read from the JUnit XML rather than estimated: api 312 (1 skip), app 110, clientside 88, grinder 233 (19 skip), plugin-example 3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`claude-docs/DOCKER-MONGO-INVESTIGATION.md` ended with "Still open: what makes the property absent entirely", and listed three unexcluded candidates: the s6 service not running, `homeDirectory` resolving elsewhere, or SPC rewriting the file. None of them. **The key was retired.** `spring.data.mongodb.uri` is not a property Spring Boot 4 binds -- `deprecation.level = "error"`, replacement `spring.mongodb.uri`, since 4.0.0 -- so `MongoProperties.uri` stayed null and the else-branch that document already dissected produced the literal `localhost`. That closes its triage table's middle row: a `MongoSocketOpenException … localhost:27017` did **not** mean the reporter's `overrides.properties` was missing or their init service had not run. Their file was almost certainly correct. Nothing further is needed from them. The old "still open" candidates are kept under a *Superseded* heading rather than deleted -- they were excluded by evidence, and that reasoning is worth keeping next to the answer. Also recorded: - the compatibility row for `DATABASE_URI_KEY`, whose *value* changed. No signature moved, which is exactly why it belongs there: an embedder that hard-coded the old string now writes a key nothing reads. Reading stays backward-compatible. - **why there is no `MigrationManager` step**, since its absence looks like an omission: migrations run release->release only, so a version-keyed method would miss every dev/alpha/beta user and would need a release number that does not exist yet. The getter's re-write covers every build type on first read. The stale legacy line stays on purpose -- deleting it would strand a downgrade, and Spring ignores it. - a `REFACTOR-LOG.md` entry with the measurements, including how this was found: the performance branch's data migration reported `0 inspected` against a correctly-seeded database, which only made sense once the client's own log line showed `hosts=[localhost:27017]`, `credential=null`. Flagged for the merge: `claude-performance-improvements` carries B33/B34 in its `BACKLOG.md`, which described this defect from the outside. Both are resolved here and should be dropped, not carried forward. `./gradlew build` green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Red pin. Lands failing, on purpose. `FALLBACK_DATABASE_URI` is the Kotlin literal `"mongodb\\://user\\:password@localhost\\:27017/serverpackcreatordb"`, so its *value* contains literal backslashes: mongodb\://user\:password@localhost\:27017/serverpackcreatordb `com.mongodb.ConnectionString` accepts only `mongodb://` or `mongodb+srv://`, so that string is not a URI the driver will take -- and it is what every first-time web user starts from, before they have configured anything. The reason their first boot cannot connect is a constant in our source, not anything they did. The backslashes are a category error: escaping belongs to the `.properties` *file format*, and `Properties.store` already applies it on write while `Properties.load` reverses it on read. Carrying it in the value means it gets escaped twice -- which is exactly why a generated home reads `spring.mongodb.uri=mongodb\\\://…`, three backslashes for one colon. Pre-existing: the constant is byte-identical on `develop`. Surfaced now because the previous commit tightened the scheme check from `startsWith("mongodb")` -- which this value satisfies -- to enumerating `mongodb://` / `mongodb+srv://`, which it does not. So the tightening did not break anything; it made a value that was already unusable *visible*. The second assertion pins the cause rather than only the symptom, so a future edit cannot "fix" this by re-adding escaping. Observed red: "The fallback must be a URI the driver accepts, but it was: mongodb\://user\:password@localhost\:27017/serverpackcreatordb". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes the last open thread from this finding, and explains a prior dead end. Two earlier attempts to shorten the MongoDB driver's server-selection timeout in tests -- once via test resources, once via `@SpringBootTest(properties = …)` -- both appeared to do nothing, and the second was especially confusing because a probe showed the URI *did* reach the environment while the client kept reporting `serverSelectionTimeout='30000 ms'`. Same cause as everything else here: the URI was not reaching the client, so nothing inside it could take effect either. Measured after the fix, against a real MongoDB: mongodb://127.0.0.1:27017/spc_t?serverSelectionTimeoutMS=250 -> serverSelectionTimeout='250 ms' (no parameter) -> serverSelectionTimeout='30000 ms' So a web-context test that runs without a database can now cut ~30s per boot by putting the parameter in its URI. That matters for anything doing Mongo work on `ApplicationReadyEvent`. Verified alongside it, all four configuration paths against MongoDB 8.0.5: fresh install (nothing configured) hosts=[localhost:27017] credential=MongoCredential spring.mongodb.uri hosts=[127.0.0.1:27017] credential=MongoCredential spring.data.mongodb.uri (legacy) hosts=[127.0.0.1:27017] credential=MongoCredential The fresh-install row is the fallback fix confirmed end-to-end: `localhost` is the fallback's own host, and the *credentials* are what disambiguate it from Boot's default `mongodb://localhost/test`, which has none. Before the fix that URI carried literal backslashes and could not be parsed at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Red pin for the migration Griefed confirmed belongs in 9.0.0. Reading `WebserviceConfig.databaseUri` already normalises a legacy-key configuration on every build type, so the migration is not what makes the upgrade *work*. What only a migration can do is **report it**: the rename is otherwise invisible, and an operator whose own tooling, container environment or hand-written `overrides.properties` still writes `spring.data.mongodb.uri` needs to know Spring no longer reads it. That file is not SPC's to fix. Two guards, the second one being the half that keeps the feature honest: upgradingToNineZeroZeroReportsTheRenamedDatabaseProperty a URI stored under the legacy key is carried to the live key, and the change is reported naming the new key. upgradingToNineZeroZeroSaysNothingWhenTheOldKeyWasNeverUsed an installation that never used the old key gets no message and nothing written. A migration that announces itself to everyone is noise, and noise gets ignored. The harness gains `managerWithStore`, which hands the mocked ApiProperties a **real** WebserviceConfig over a real PropertyStore — so the assertion is about stored properties rather than about a stub. Observed red on the first (expected the carried URI, got null); the second passes already, which is what makes it worth keeping rather than writing after the fact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`serverpackcreator-help/Writerside/api-docs.yaml` was hand-maintained and had drifted badly: **25 documented paths against 44 real ones**, and two schemas -- `ModPackView` and `ServerPackView` -- described classes that no longer exist anywhere in the module. springdoc is wired back in, as `developmentOnly` so swagger-ui stays out of the shipped jar. The commented-out coordinate that was there pinned 2.2.0, which targets Spring Boot 3 and cannot resolve against Boot 4; **3.1.0** is the Boot 4 line -- verified, its parent POM is `spring-boot-starter-parent:4.1.0`, the same version this project pins, and it depends on Boot-4-only artifacts (`spring-boot-tomcat`, `spring-boot-health`). Version declared in the catalog, per the build conventions, not as a coordinate in the module file. Regenerated by running the real app and fetching `/v3/api-docs.yaml`; the command is recorded beside the dependency. What changed, measured: paths 25 -> 44 19 endpoints were undocumented (all of /stats/downloads/*, the *paginated variants, /modpacks/byserverpack/{id}, /versions/neoforge/{mcver}) schemas 12 -> 16 +AmountPerDate, AmountStatsData, DiskStatsData, ModPack, ModPackDownload, ServerPackDownload; -ModPackView, -ServerPackView **Nothing documented disappeared**, so the old file's drift was pure omission rather than staleness in the other direction -- and the two dangling schemas fell out on their own, which is the argument for regenerating over hand-patching. The generated types are also more precise than a hand-fix would be: ids come out as `['string','null']`, derived from the actual nullable Kotlin types. Every one of the 25 endpoints referenced by `<api-endpoint>` elements in the Writerside topics still resolves in the regenerated file, so no documentation page breaks. springdoc's placeholder header is replaced: `title: OpenAPI definition` / `version: v0` become `ServerPackCreator` / `v2` -- `v2` matching the stable `/api/v2` prefix rather than a build version that would go stale -- and the `servers:` block is dropped, since a published spec should not pin whichever host generated it. The description says the file is generated and points at the command. Root `CLAUDE.md` now states the file is generated, because it was hand-edited for long enough to drift by 19 endpoints. **`excludeGroups` for org.springdoc in the license report, which this change turned out to need.** `developmentOnly` keeps springdoc out of the jar -- verified by listing the bootJar, which contains no springdoc or swagger entry -- but it still reaches `compileClasspath`/`runtimeClasspath`, which is what `licenseReport` reads. So it had added itself to `licenses/LICENSE-AGREEMENT.txt` and the GUI copy, documents about what *ships*, taking the dependency count 44 -> 45 and churning 303 lines in two shipped files. Excluded, both files are byte-identical to develop again and the count is back to 44. **Note for whoever merges `claude-performance-improvements`:** that branch hand-edits this same file for its DBRef->embedded change, and deletes `ClientMod` / `StartArgument` / `WhitelistedMod`. On develop those entities still exist, so this regeneration correctly references them. After the merge, regenerate again rather than resolving the conflict by hand. `./gradlew build` green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Seven audit iterations of performance work: network/startup timeouts, manifest conditional GETs, the GUI typing path, generation throughput, and the web module's query shapes -- including flattening RunConfiguration's three @DBRef mod-lists to embedded string arrays, with a join-free migration for persisted data. Iteration 7 established equivalence with develop by measurement rather than assertion: develop's own test trees, run unmodified against this branch's production code, gave 490 pre-existing guards and zero failures (api 309, clientside 88, app 93). The two files that could not compile were the branch's two deliberate shape changes -- one adapted with no assertion edits, one legitimately superseded. The data migration, the declared index and the REST response shape were each verified against a real MongoDB. Two conflicts, both resolved deliberately: claude-docs/BACKLOG.md B33/B34 DROPPED. They described the Spring Boot 4 database-property defect from the outside -- "the web app appears to use MongoDB's default test database" -- and claude-mongo-boot4-property fixed the cause, so carrying them forward would list solved problems as open. B30-B32 kept; B35 kept. api-docs.yaml took the regenerated spec. The branch hand-edited this file for its embedded mod-lists, which was right at the time; it is now generated from the controllers, so it is regenerated in the next commit instead -- the entities this branch changed are exactly what it reads. Everything else auto-merged, and the high-value regions were checked rather than assumed: the compatibility table carries rows from both sides (32 total), both sets of app-module landmines survive (the auto-index-creation trap and the database-property rename), and both REFACTOR-LOG narratives are present. Suite counts in the refactor-state table still read this branch's figures and are corrected, from the JUnit XML, two commits from here -- the merge adds tests from the database work that were not in either side's count.Follow-up to the four merges, doing the two things a merge cannot do for itself. **The OpenAPI spec is regenerated against the merged code**, which is why the merge took the generated file rather than resolving that conflict by hand. `claude-performance-improvements` flattened `RunConfiguration`'s three `@DBRef` mod-lists into embedded string arrays and deleted `ClientMod`, `StartArgument` and `WhitelistedMod`; `claude-openapi-regenerate` had regenerated the spec while those entities still existed. Neither side's file was right for the merge. Regenerating produced the correct combined state on its own: RunConfiguration.startArgs/clientMods/whitelistedMods -> items: {type: string} ClientMod / StartArgument / WhitelistedMod schemas -> gone schemas 16 -> 13, paths 44 (unchanged) All 25 endpoints referenced by `<api-endpoint>` elements in the Writerside topics still resolve, so no documentation page breaks. This is the argument for generating rather than hand-maintaining, made concrete: a hand-merge of two divergent 1,300-line specs would have been guesswork, and the result here was mechanical. **Suite counts corrected from the JUnit XML**, api 339 -> 343 and app 145 -> 149. Neither branch's figures were wrong for that branch; the merge simply adds the database work's tests (`WebserviceConfigTest` +4 in `-api`, `DatabaseUriPropertyTest` and `MigrationManagerTest` +4 in `-app`) to counts neither side had measured together. Recorded here rather than left to drift, which is the defect three separate audits kept finding. Merged `./gradlew build` green: api 343 (1 skip), app 149, clientside 88, grinder 233 (19 skip), plugin-example 3, frontend 32. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes B30. After the conditional-GET work, four of twelve manifests answered `304` to `If-Modified-Since`. `files.minecraftforge.net` **ignores** that header but honours `If-None-Match` against its weak nginx ETag -- and it is the largest manifest still transferred in full on every startup, 121,492 B, 57 % of the 213,885 B that remained. Both headers are now sent. A host honouring either answers `304`; a host honouring neither answers `200` and everything downstream runs exactly as before, so this needs no per-host special-casing. **The ETag is validated, not trusted, and that is the whole safety of the feature.** An ETag describes one exact body. Recorded in a `<manifest>.etag` sidecar together with the manifest's byte length, and offered only when that length still matches what is on disk. Without that check, a manifest replaced by other means -- `ApiWrapper.setup()` re-seeding it from the jar is the real case, a restore or hand-edit are others -- would earn a `304` for content we do not hold and suppress a genuine update **permanently**. It is also recorded **only when the manifest is actually adopted**. The updater declines an upstream manifest with fewer versions than the local copy, and remembering that response's ETag would describe a file we chose not to keep. Both of those are pinned, and both pins were mutation-verified rather than trusted: trust the pairing (drop the length check) aStaleEtagPairingIsNotOffered FAILED remember the ETag when not adopting anEtagIsNotRememberedForAManifestThatWasNotAdopted FAILED Worth stating plainly: three of the four new guards passed *vacuously* before the implementation existed, because no ETag was being sent and `null == null`. The mutations above are what establish they assert anything. The fixtures also seed the manifest first, as `ApiWrapper.setup()` does in production -- the first draft did not, took the absent-manifest download path instead, and could never have observed an ETag at all. **A build-side hazard came with it.** `updateManifests` copies `tests/manifests` into the shipped resources with no filter, so sidecars the suite writes there would have been packaged into the jar and seeded into every user's home -- one machine's HTTP bookkeeping shipped to everyone. Now excluded. Measured: 12 manifests copied either way, 0 sidecars. Scoped honestly: this buys **~0 ms of startup**, as B30 always said. The twelve checks run concurrently, so wall-clock is the slowest host, and Forge is not it. What it buys is bandwidth on a metered or slow connection, where 121,492 B is the dominant cost rather than latency. `./gradlew build` green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes B31, the last of the deferred performance items. `VersionMeta`'s constructor blocked on checking all twelve manifests before it returned. Since `ApiWrapper.setup()` has already seeded every manifest from the jar, the metas have working data before any request is made -- so that wait bought nothing a moment later, and cost it on every launch. Measured with the same probe on the same machine, constructing a `VersionMeta` over the same home: develop (blocking) 399 / 390 / 835 ms median ~399 this branch 56 / 36 / 47 ms median ~47 ~352 ms off a GUI launch, and considerably more offline, where the old path waited out twelve connect timeouts before showing a window. The remaining ~47 ms is parsing the seeded manifests, which is the irreducible part. **The freshness contract is preserved where it is observable, which is the part B31's entry missed.** The GUI's version dropdowns are `DefaultComboBoxModel`s built once in `ConfigEditor` and **nothing anywhere repopulates them** -- so a naive background refresh would have hidden a freshly released Minecraft version until the next launch, breaking exactly the workflow ServerPackCreator exists for. Two awaits close that: ConfigEditor an `init` block placed *above* the version-list properties, because Kotlin runs initialisers in declaration order and that is the only point the wait can happen before they are built. checkConfiguration the single choke point every validating caller passes through -- CLI, interactive shell, web, embedders -- so a short-lived `--headless` run cannot reject a version upstream published minutes ago. One site instead of four. `awaitManifestRefresh` is idempotent, returns immediately once the refresh has landed, and is bounded (10 s default) so an unreachable host delays a dropdown rather than hanging the UI. The refresh re-parses after checking, because the metas are built from the manifest *files* and a refreshed file is invisible until read again -- the same two steps `update()` performs. Concurrent re-parse while another thread reads a meta is **not** a new hazard: the web backend's `VersionRefreshSchedule` has always called `update()` on a cron while requests read the metas. Failures are logged and swallowed; the seeded manifests remain perfectly usable. The scope is `CoroutineScope(Dispatchers.IO + SupervisorJob())`, deliberately not `GlobalScope`, which this project removed everywhere else. It owns no thread and the one job it runs completes. Pinned by `VersionMetaRefreshTest`: the seeded manifests are usable before any refresh -- the half the whole change rests on -- and the refresh is awaitable and reports completion, twice, so callers may await freely. The timing above is measurement rather than assertion, recorded here because no test can pin it without a network seam in an already fifteen-parameter constructor. Compatibility row added. `./gradlew build` green: api 354 (1 skip), app 149, clientside 88. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>All three deferred performance items have landed, so they leave the backlog and enter the log, per that file's own convention. Only the CI items (B26-B29) remain. The log entry keeps what was learned rather than only what was done: - B31's entry was **incomplete**. It described moving the manifest refresh off the blocking startup path and did not mention that the GUI's version dropdowns are built once and never repopulated -- which is what would have turned a 352 ms saving into "a freshly released Minecraft version needs a restart to appear". Written down so the next reader sees the constraint, not just the win. - B30's ETag is only safe because the pairing is validated. That reasoning, and the mutation results proving the guards bite, matter more than the 121,492 B saved -- and the entry records that three of the four guards passed vacuously before the implementation existed. - B32's characterization tests caught a mistake the change itself introduced: `File.length()` on a directory returns a small number, so the first version of the early return made the check *accept* a directory where it had always rejected one. Measurements carried over verbatim (~399 ms median -> ~47 ms, three runs each, same probe and machine) so nobody has to trust a remembered number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>None of B26-B29 can be closed from the repository. This makes the next pipeline able to close three of them in one shot instead of two pipelines, and records what was established so nobody re-derives it. **The diagnostic now prints `CI_RUNNER_ID` / `_DESCRIPTION` / `_TAGS`.** Not padding: this file declares **no `tags:` anywhere**, so nothing pins these jobs to one runner. Without knowing which runner produced the output, one pipeline cannot answer B27 -- if any runner lacks the `/var/run/docker.sock` mount that kills dind, then on that runner dind is the only daemon and dropping `.dockerized` breaks the release pipeline. That is the hole in reasoning about this from configuration alone, and it is why `.dockerized` was *not* deleted here. **Two facts verified locally, recorded in B27, eliminating one of its two branches:** - `.gitlab-ci.yml` sets neither `DOCKER_HOST` nor `DOCKER_TLS_CERTDIR` anywhere -- the only mentions in the whole file are the diagnostic echoing them. The CLI therefore uses its default `unix:///var/run/docker.sock`, so "the service is the intended endpoint" is excluded by configuration. - the `docker` alias the service publishes is never used as an endpoint by any job. So on the runner that produced the green 2026-08-04 pipeline, the jobs reach a daemon at the default socket path that the service did not create -- it died trying -- and removing a service nothing connects to cannot remove that daemon. Strong, but scoped to *that* runner. B27 warns that guessing wrong breaks the release pipeline's push jobs, and seven jobs still extend `.dockerized`, so the runner identity gets printed rather than the service deleted on inference. **B29:** nothing committed carries the Qodana count -- `qodana.yaml`, no SARIF, no baseline -- so it lives only in that job's artefact. Recorded, with the image the job uses. **B28** stays not-actionable, now cross-referenced: the useful question about the runner config is whether *every* runner accepting these untagged jobs carries the mount, which is exactly what B27 needs. Also recorded so nobody repeats it: reading the pipeline from here was attempted and failed -- `git.griefed.de`'s API answers `404` unauthenticated, and there is no `glab`, `gh` or token on this machine. `.gitlab-ci.yml` still parses as YAML. No source touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`git.griefed.de` is **Forgejo 16.0.3**, not GitLab -- verified against `/api/v1/version`. `.gitlab-ci.yml` and its 22 jobs are deleted. CI lives in `.forgejo/workflows` and Forgejo is the origin of every release. **This is a cutover, and it had to be one commit.** `.forgejo/workflows` is all-or-nothing: per a Forgejo maintainer on forgejo#9203, "If a project contains a `.forgejo` and a `.github` folder, then the `.github` folder is ignored." Forgejo had been running the `.github` workflows as a fallback -- that is where releases 9.0.0-alpha.2 through .6 came from -- so the moment this directory exists, those stop. Everything Forgejo must do therefore lands here at once; a staged port would have left a window with no releases. Nine workflows: `test.yml` and `docker-test.yml` (the GitLab Build Test / Docker Test role), `qodana.yml`, `release-generate.yml` (semantic-release), `release-build.yml` (assets, the Forgejo release, Maven, Docker, the outward mirror, VirusTotal), `devbuild.yml`, `docs.yml`, `update-readme.yml`. Decisions that are not obvious from the diff: - **`uses:` keeps the `actions/...@<github-sha>` lines the .github workflows used.** Forgejo's docs recommend `https://data.forgejo.org/...`, but these exact references are *proven* to resolve on this instance -- they built the existing alpha releases. Trading that for a mirror whose commit SHAs may differ would swap something known to work for something merely recommended. install4j is the one deliberate exception, fetched from GitHub by full URL. - **semantic-release now does version + changelog + tag only.** `@semantic-release/gitlab` and `gitlabUrl` are gone, `publish` is `false`; the release is created by the tag-triggered workflow, where the assets are. Same two-phase shape GitLab had, so the `releaseRules` that produce your version numbers are untouched. **The tag must be pushed with a real user token** -- Forgejo, like GitHub, does not trigger workflows from pushes made with the automatic per-run token, so an automatic-token push would tag a release nothing ever builds. Called out in the workflow itself. - **`GitGriefed` Maven repository retargeted, name deliberately kept.** It pointed at `/api/v4/projects/63/packages/maven` with a `Private-Token` header: a GitLab path and a GitLab auth scheme, neither of which exists on Forgejo. Now `/api/packages/Griefed/maven` over HTTP Basic. The repository *name* is unchanged so `publishMavenJavaPublicationToGitGriefedRepository` still exists -- verified, all four publish tasks are still generated. GitHub Packages, gitlab.com and OSSRH untouched: the move is off the self-hosted GitLab, not off gitlab.com. - **Release mirroring is explicit API calls.** Forgejo push-mirrors replicate refs but not releases. The mirror job runs last and only on success, so no downstream forge advertises a release Forgejo lacks. - **VirusTotal became a job in `release-build.yml`** instead of a release-triggered workflow: the assets are already there as an artifact, and `crazy-max/ghaction-virustotal` updates a *GitHub* release body, which is the wrong forge now. - **`docker-test.yml` does not push, and that is a change.** The GitLab job used `--push` and tagged every commit's image on ghcr.io *and* Docker Hub, publishing an image per branch push that nobody consumed. Restoring it is two lines, noted in the file. - **Two regressions avoided by reading the code being replaced rather than skimming it.** `update-readme` sent its token in an `Authorization` header *specifically* so it could not leak into logs -- the port keeps that rather than putting the token in the push URL. Qodana's JBR `chmod` existed because GitLab's cache drops the executable bit, which `actions/cache` does not; it survives as documented insurance rather than being blindly copied or silently dropped. GitHub keeps a **smoke test** plus the four issue-driven `clientside-*` workflows. `github_release.yml`, `github-prerelease.yml`, `devbuild.yml`, `update_readme.yml` and `virustotal.yml` are deleted from there. `devbuild` additionally clears the stale GitHub `continuous` *release* while leaving its *tag*, so the mirror recreates it from Forgejo. **B26-B29 dropped, not answered** -- they described GitLab dind and a GitLab-Pages-hosted Qodana report, and that infrastructure is gone. The backlog is now empty. All 13 workflow files parse as YAML. `./gradlew build` green: api 354 (1 skip), app 149, clientside 88, grinder 233 (19 skip). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-2 audit findings D1 and D2. Both were found by *executing* the workflow steps -- pulling each `run:` block out of the YAML, substituting the expressions and running it against this repository's real CHANGELOG.md and a synthetic build tree -- rather than by reading them. D1 in particular survived two readings. **D1: a final release's notes contained every prerelease's notes as well.** The changelog extraction's start pattern had an *optional* closing bracket, so a request for `8.1.1` also matched `## [8.1.1-beta.2](...)`. That rule ends in `next`, so the exit rule never saw those headings and collection ran straight through them. Measured against the real file: 8.1.1 extracted 5,685 chars true section 2,834 8.1.0 extracted 28,807 chars true section 14,377 8.1.1-beta.2 extracted 673 chars true section 672 (correct - nothing nests under a prerelease) So every full release would have published its changelog followed by the changelogs of the prereleases leading to it, roughly doubling the notes. Prereleases were unaffected -- which is exactly why it would have shipped unnoticed, since the first Forgejo-cut release will be an alpha. The bracket is now required and the version's dots are escaped. Verified by execution against five versions including one absent from the file: every extraction now matches the true section boundaries exactly, and a missing version still falls back to "Release <version>". **D2: a failed asset upload left an incomplete release and a green run.** Five steps calling `curl` in loops or sequence had no `set -e`, so `curl -sf` returning non-zero was ignored and the step exited with the status of its last command -- one asset failing to upload would produce a release missing a file while the run reported success. For a release pipeline that is the wrong failure mode: a loud failure can be re-run, a silently incomplete release gets downloaded. Added to *Create release and upload assets*, all three `mirror` steps, and Discord -- the last one after its missing-webhook guard, so an absent webhook still exits cleanly while a failing post does not pass silently. The three steps written for this migration in devbuild.yml, and both VirusTotal steps, already had `set -eu`; this was inconsistency rather than a considered choice. Also recorded in the audit as verified-by-execution rather than assumed: tag classification accepts the two release shapes and refuses `continuous`, `9.0`, `v9.0.0` and `9.0.0-rc.1`; docs image tags cover all three GitLab jobs' behaviour from one computation; asset collection yields the right 13 files with `-plain.jar` and `output.txt` excluded and `checksum.txt` not listing itself; and both release payloads are valid JSON with the body round-tripping byte-identically through shell quoting. No source touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes iteration-3 audit findings E1-E6. All six are about *when* jobs run rather than what they do -- the class neither YAML validation nor executing steps in isolation can see, because each one is a relationship between jobs. **E1, the one that would have done visible damage.** The GitHub mirror POSTed a release with `tag_name` and no `target_commitish`. GitHub creates the tag from `target_commitish` when it does not exist, defaulting to the **default branch** -- and git mirroring is asynchronous, so seconds after the Forgejo release the tag has usually not arrived. First release would have created a `9.0.0` tag on GitHub pointing at main's HEAD: disagreeing with Forgejo, and blocking the real tag once mirroring caught up. Now passes `target_commitish: github.sha`. **E2** is the same root cause from the other side: GitLab's release API answers `404 Tag Not Found` unless the tag exists or a `ref` is given. With the `set -eu` added in iteration 2, that became a hard failure of the mirror on essentially every release. Now passes `ref`. **E3: devbuild had no concurrency guard, and this migration is what made that dangerous.** The GitHub original had none either, but it updated the `continuous` release in place, which overlapping runs survive untidily. This port deletes-then-recreates -- deliberately, so stale assets cannot linger -- which is right for one run and wrong for two: concurrent runs interleave delete and create, and there is a window where the release the download page points at does not exist. The guard is not tidy-up being backfilled; the change raised the stakes and should have brought it along. **E4: a re-run could not repair a partly-failed release.** `release` created unconditionally, so re-running after a `maven` or `docker` failure -- the ordinary repair -- died at release creation under `set -eu`, skipping `mirror` and `virustotal`, the very jobs that needed retrying. It now looks the release up by tag, refreshes its notes and reuses its id, creating only when absent. **E5: two ordering problems, one fix.** `virustotal` PATCHes the Forgejo release body with the scan permalinks while `mirror` read that body in parallel, so the GitHub copy could never carry the section -- a capability the replaced workflow had via `update_release_body`. And `mirror` did not depend on `maven` or `docker`, so GitHub could advertise a release whose artifacts and images did not exist yet, or never would. The approved plan said the mirror "runs last and only on success"; the graph now says so too: prepare -> assets -> {release, maven, docker} -> virustotal -> mirror. **E6:** `update-readme` gets a guard so its schedule and a manual dispatch cannot both push to main. Re-validated against the final state, all six checks green: YAML and every needs/outputs/step-id reference resolve; no bare third-party actions and no `secrets.GITHUB_*` in executable positions; `mirror` depends on every other job; all eight workflows have a concurrency group; no curl or loop step lacks `set -e`; and the changelog extraction still returns exactly the true section for every version tested. No source touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The audit reports had drifted into two files that were never duplicates of each other, so neither could simply be deleted: claude-docs/REFACTOR-AUDIT.md 440 lines, 3 audits from 2026-06-25 (claude-workflow-audit, claude-clientside-verify, and a full-range Phase 0 -> HEAD pass) REFACTOR-AUDIT.md (root) 1802 lines, 15 audits covering the four perf branches, the seven claude-performance-improvements iterations, and the three Forgejo CI iterations The split is an artefact of the audit command writing to a root-level path by default, not a decision. Merged chronologically -- the June audits first, the August ones appended -- so the file reads in the order the work happened. Verified nothing was dropped: both halves diff byte-for-byte against their pre-merge blobs (440 and 1802 lines; 18 audit sections in the 2256-line result). Neither file was ever shipped -- serverpackcreator-api/build.gradle.kts limits shippedDocuments to an explicit seven-file allow-list -- so this is not a user-visible change and no build wiring refers to either path. Kept rather than deleted because the findings themselves are closed but the *evidence* is not reproducible from git log: the mutation-testing results, the before/after measurements the conventions require for build-logic changes, and the "verified clean, do not re-litigate" lists. REFACTOR-LOG.md records what was decided; this records what was measured. CLAUDE.md:298 cited the root path for iteration 7's 490-guard result and now points at claude-docs. REFACTOR-LOG.md:934 needed no change -- it is a sibling in claude-docs, so its bare reference resolves correctly for the first time. The remaining in-file mentions are historical prose about where the file used to live, and stay as written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`Bump install4j to 13` broke every task in the build. Not the media task, not the release -- everything, `./gradlew help` included: install4j-gradle-13.1.jar!/META-INF/...kotlin_module Module was compiled with an incompatible version of Kotlin. The binary version of its metadata is 2.3.0, expected version is 2.0.0. install4j-gradle:13.1 is built with Kotlin 2.3. Precompiled script plugins compile with GRADLE'S EMBEDDED Kotlin -- 2.0.x on the wrapper's 8.14.4 -- and buildSrc/build.gradle.kts had the plugin marker on that compile classpath, so :buildSrc:compilePluginsBlocks had to read metadata three minors ahead of itself and refused. The catalog's kotlin = 2.4.10 cannot help: it governs how the modules compile, never how build logic does. The obvious remedies are both bad. Reverting the plugin to 12.0.2 leaves a v12 plugin driving a v13 installation against a spc.install4j now stamped 13.1, which cannot be verified without install4j installed. Upgrading to Gradle 9.x (embedded Kotlin 2.3.10+) is the honest fix for the metadata gap but a major migration against a build with 20 configuration-cache problems and four third-party plugins to re-verify -- and the deprecation warning in this build's own output says it is not Gradle-9 ready today. Neither is necessary. Nothing in buildSrc/src/main/kotlin applies install4j -- verified by grepping every id(...) and kotlin(...) call in the precompiled script plugins -- so the marker existed solely to version the bare id("com.install4j.gradle") in the ROOT build script. That is a real build script, so it takes alias(libs.plugins.install4j) and the marker comes off entirely. The incompatible jar is then never on a classpath the embedded compiler reads. Measured, per the build-logic convention: before ./gradlew help FAILED in 3s at :buildSrc:compilePluginsBlocks after ./gradlew help SUCCESSFUL in 5s after ./gradlew build SUCCESSFUL in 6m 8s api 354 (1 skipped), app 149, clientside 88, grinder 233 (19 skipped), plugin-example 3 -- zero failures The plugin is genuinely applied, not merely absent: `tasks --all` still lists both `install4j` and `media`. install4j stays at 13.1, matching the tool and licence Griefed moved to, and Gradle stays at 8.14.4. CLAUDE.md's marker documentation gains the landmine, and with it a narrower rule: a plugin needs the marker only when a PRECOMPILED SCRIPT PLUGIN applies it. The ones that genuinely do are enumerated there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The root CLAUDE.md had reached 40,204 characters, past the ~40,000 floor where Claude Code warns that a single memory file is too large -- and it loads in full in every session, whatever the task. Today's additions (the marker landmine, the ordering mantra) are what pushed it over. Two sections move out into `.claude/rules/`, which loads a file only when the session touches paths its frontmatter names: build-layout.md the 149-line "where build declarations live" section -> **/build.gradle.kts, settings.gradle.kts, buildSrc/**, gradle/*.toml, gradle/wrapper/** ci-workflows.md the Forgejo all-or-nothing CI landmine -> .forgejo/**, .github/**, .releaserc.yml Root CLAUDE.md: 40,204 -> 27,243 chars, ~10,051 -> ~6,810 est. tokens resident per session. Nothing was dropped -- the three files together are 1,798 chars LARGER than the original, that being the frontmatter and the pointers left behind at both cut sites. Heading count is 12 before and after. Both frontmatter blocks parse, and their globs match real files (16 under buildSrc, 8 workflows, 8 build.gradle.kts, the catalog). Two judgement calls worth recording, because both cut against the obvious move: The CI bullet had five lines glued to its tail that were not about CI at all -- the note that api-docs.yaml is GENERATED, not hand-maintained. Moving that behind CI-scoped paths would have stopped it loading when someone edits a controller, which is exactly when it is needed and exactly how it drifted to 25 of 44 endpoints last time. It stays resident, promoted to its own bullet. Nothing was deleted. The obvious candidate was the build-layout section against BUILD.md, but they are complementary: BUILD.md is the contributor tour, this is the landmine set it does not carry (plugin markers, platform(), the retired io.spring.dependency-management). The ten module CLAUDE.md files were scanned too; their layout sections look like derivable class inventories but interleave the rationale inseparably -- dependency direction, which class is the seam, why GrinderApplication cannot move -- and they are lazy-loaded already. THE TRADE, STATED PLAINLY: these are landmines that now load conditionally. If a glob is wrong they silently stop appearing and someone re-adds io.spring.dependency-management or tidies away one of the two foojay declarations, which is what they exist to prevent. The globs above cover every build and workflow file in the tree today; a new build file outside them inherits no warning. Also dedents two lines in serverpackcreator-api/CLAUDE.md to 2 spaces, matching the surrounding bullet continuation -- whitespace collateral from this morning's B25 correction, where a 4-space continuation can render as a nested block instead of prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Three call sites, two APIs, found by configuring the build under Gradle 9.7.1: build.gradle.kts install4j block `properties["install4jHomeDir"]` -> providers.gradleProperty(...) build.gradle.kts examplePlugin `configurations.creating` delegate -> configurations.create("examplePlugin") -plugin-example pluginArtifact same delegate -> configurations.create("pluginArtifact") Both are deprecated in 9.7 and scheduled to FAIL in Gradle 10, so this is prerequisite work for the wrapper bump rather than tidying. The property fix also closes a trap that was one edit away from firing. `properties["x"]` on an ABSENT key returns null, and Kotlin renders that as the STRING "null", which is not blank — so the old `isNotBlank()` guard passed and installDir became a directory literally named `null`. It never fired only because gradle.properties declares `install4jHomeDir=` with an empty value, which made that empty declaration load-bearing and undocumented. Deleting that one line would have broken `media` at release time, where install4j is not part of the dev loop and nobody would have been watching. The provider form has no such edge. The two configuration renames are name-preserving on purpose: `pluginArtifact` is consumed by STRING name from the root build (`project(path = ..., configuration = "pluginArtifact")`), so a delegate-derived name silently becoming something else would break the example-plugin wiring rather than fail to compile. `create(name)` is what the deprecation message itself prescribes. Verified on the CURRENT Gradle (8.14.4) before the bump, since these land first: configuration succeeds, `install4j` and `media` are still registered, and the root build still resolves examplePlugin -> project :serverpackcreator-plugin-example. After the bump, a clean `build --warning-mode all` reports ZERO deprecation warnings naming any of our build scripts. Note on the count: this started as "two deprecations" because I read the tail of a log instead of the whole of it. `build.gradle.kts:69` used the same delegate and was invisible in the last 35 lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>docs.yml and qodana.yml both bind-mounted $PWD into a sibling container. Under docker-in-docker that path is resolved by the DIND daemon, whose filesystem holds no copy of the job's workspace, so the sibling is handed a freshly created empty directory instead. Measured 2026-08-22 on a docker:dind + data.forgejo.org/forgejo/runner:13 rig (Docker 29.7.2), running a probe workflow through `forgejo-runner exec`: docker run -v "$PWD:/x" alpine ls -A /x -> no output, exit 0 docker run --volumes-from <job> -w "$PWD" ... -> the file the job wrote, and a file placed by `docker cp`, which is how act injects the workspace Both failures are silent: Writerside would build nothing behind its `|| true`, and Qodana would scan nothing behind its own, which reads as "no problems found". - docs.yml, qodana.yml: `--volumes-from "$(cat /etc/hostname)"` plus `-w "$PWD"`, with the tool arguments pointed at workspace paths rather than mount points. - qodana.yml: assert a sibling can see the workspace before scanning, since the scan's `|| true` would otherwise hide broken propagation. Uses the Qodana image itself, so this adds no new image dependency. - Comments corrected: the docs.yml note claiming the workspace is a bind mount was false under DIND, and the qodana note recording a locally proven command line now says so about the *old* /data-mount form. Requires the runner's container.options to mount a volume at the workspace parent ("--add-host=dind.docker.internal:host-gateway -v /workspace"). Without it the workspace is ordinary container filesystem and nothing propagates -- which the new guard now reports loudly instead of silently. Not verified locally: Qodana's workspace-path argument form (the /data form was the one proven by a local run), and that act never overrides the job container's hostname. Both fail loudly, not silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Every artifact step in this directory failed with ::error::@actions/artifact v2.0.0+, upload-artifact@v4+ and download-artifact@v4+ are not currently supported on GHES. after the step had already found and staged its files -- 143 XMLs in test.yml, 2 files in docs.yml, 7 JARs in devbuild.yml. @actions/artifact v2 calls isGhes(), which treats any hostname that is not GITHUB.COM (or *.ghe.com / *.localhost) as GitHub Enterprise Server and throws. git.griefed.de is neither, so the refusal is unconditional. It is a client-side check, not a missing API on the instance, and no runner or instance setting disables it. Forgejo maintains patched forks for this. Verified 2026-08-22 by fetching the built bundles at the pinned commits and reading them: forgejo/upload-artifact v5 dist/upload/index.js -> function isGhes() { return false; } forgejo/download-artifact v7 dist/index.js -> function isGhes() { return false; } 11 upload and 8 download references across devbuild.yml, docs.yml, qodana.yml, release-build.yml and test.yml now point at those forks, pinned by commit like every other action in this directory. The directory-wide `uses:` note in test.yml records why these two are the exception to "the same actions/...@<github-sha> lines as .github". .github/workflows deliberately keeps the upstream actions: on GitHub the isGhes() check is correct and the forks buy nothing. The forks lag upstream (upload v5 vs v7, download v7 vs v8). Every input this directory passes -- name, path, if-no-files-found, retention-days -- exists in those versions, so no call site needed rewriting beyond the reference itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>test.yml uploaded both its artifacts with no `retention-days`, so each push fell back to the instance default. Verified against the Forgejo config cheat sheet: ARTIFACT_RETENTION_DAYS defaults to **90**, and per-artifact overrides are the documented mechanism ("Artifacts can have their own retention periods by setting the `retention-days` option in the `actions/upload-artifact` step"). Deletion is automatic and server-side once an expiry is set: `cron.cleanup_actions` ("Cleanup Expired Actions Assets") defaults to ENABLED true, RUN_AT_START true, SCHEDULE @midnight. No token, no extra job, and nothing for a workflow to sweep. - test-results: 7 days. Small XMLs, and the thing actually read after a failure -- including one noticed the following Monday. - build-artifacts: 1 day, matching devbuild.yml. Whole build trees (the run that surfaced the GHES refusal counted 5708 files) and a convenience copy of what any checkout rebuilds. Nothing downstream consumes it. docker-test.yml, the other test workflow, uploads no artifacts, so it needs nothing. Confirmed on the instance (Forgejo 16.0.3) while writing this: the repo currently holds zero artifacts, because every upload has been failing on the GHES refusal fixed in the preceding commit. So this policy has never been exercised here -- the check on the first green run is that `GET /api/v1/repos/Griefed/ServerPackCreator/ actions/artifacts` shows expires_at at created_at +1d and +7d rather than +90d. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The embed's url was the run page, so reading a report meant landing on the run, finding the artifact list and downloading from there. It now points straight at the artifact: <server>/<repo>/actions/runs/<run_number>/artifacts/qodana-report Verified against Forgejo's own routing table rather than assumed -- `m.Get("/artifacts/{artifact_name_or_id}", actions.ArtifactsDownloadView)` under `/{owner}/{repo}/actions/runs/{run}`. The artifact *name* is used because it is stable and readable. Deliberately NOT the upload action's `artifact-url` output: that is built as `${serverUrl}/${owner}/${repo}/actions/runs/${github.context.runId}/artifacts/${id}` -- read out of the pinned bundle -- and Forgejo's run URLs use the per-repo index, not the global run id. It is the same reason RUN_URL here has always used `github.run_number`. The artifact id is still consulted, as an existence check: the qodana job now exports `report_artifact_id`, and notify falls back to the run URL when it is empty, which is what happens when the scan dies before writing a report (`if-no-files-found: warn` makes that a warning, not a failure). The embed says which case it is and names the commit, so an old message stays unambiguous. retention-days: 7 on the report, per the same reasoning as test.yml: the link's lifetime is the artifact's, and 90 days of per-push reports is accumulation, not retention. Each message keeps pointing at its own run's artifact, so it goes dead rather than silently showing a newer scan. Both branches were executed, not eyeballed: the step's script was extracted, its workflow expressions substituted and the curl replaced with a dump, giving artifact present -> .../actions/runs/143/artifacts/qodana-report artifact absent -> .../actions/runs/143 and valid JSON in both. The no-webhook guard still exits 0 without posting. This does not render the report in a browser, and nothing available here can: Forgejo has no Pages, Qodana Cloud is out by design, and raw files come back as `text/plain` with `nosniff` (verified against the instance), so a report committed to a branch would show as source with its CSS and JS blocked. The link serves the zip; index.html inside it opens locally. Hosting it was offered and declined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The embed now carries `Expires at: <t:EPOCH:f>`, which Discord renders in each reader's own timezone and locale, so the message says when its own link dies. The epoch is Forgejo's `expires_at` for that artifact, read from `/api/v1/repos/{owner}/{repo}/actions/artifacts` and matched on the id the upload step already exports -- not retention-days re-added to "now". Two reasons: the instance is what actually deletes, so it is the thing worth quoting; and it keeps the message from being a second, drifting copy of the retention setting. The listing answers anonymously for this repo (verified against the instance), so no token is involved. `ActionArtifact` exposes `expires_at`, confirmed in the instance's own swagger rather than assumed from GitHub's shape. When the expiry cannot be determined the line is omitted rather than guessed -- no expiry beats a wrong one -- and the message still posts. Executed against fixtures, all five paths: Z-format expires_at -> <t:1787990819:f>, matches the expected epoch offset expires_at (+02:00) -> same epoch, so both RFC3339 spellings parse id absent from the listing -> no expiry line, still posts listing unreachable -> no expiry line, still posts no artifact uploaded -> run URL, no expiry line That fourth case is why `rm -f artifacts.json` precedes the fetch: the first run of the harness passed it only because the redirect truncates the file, and correctness should not rest on that. With the explicit removal the case passes on its own terms. Two shell details worth keeping: the expiry line is joined with $'\n' because an unindented continuation would dedent out of the YAML block scalar and parse as a mapping key, and the parser is a heredoc script rather than a `python3 -c` one-liner because it needs two format attempts and a loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>