...or you live long enough to see... #713
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!713
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?
`Mirror release outward` polled GitHub for five minutes for a release commit that a Forgejo push-mirror delivers on its own schedule. `Sync when new commits are pushed` is an opt-in checkbox, and without it that schedule defaults to 8h -- up to ninety-six times the poll's own patience. The probe diagnosed this correctly every time and failed the release anyway. Measured over the 12 release builds from 9.0.0-alpha.6 to 9.2.1: the job reached its steps 8 times and failed 6, and 5 of those 6 are this one stall -- alpha.7 (run 311), alpha.9 (755), beta.3 (901), 9.0.0 (1063) and 9.2.1 (1411), each 20/20 attempts of `422 No commit found for SHA`. The sixth is alpha.6's since-deleted GitLab step. That only timing was ever missing is proven by the 9.2.0 tag, which GitHub holds today -- delivered by the mirror's own interval, hours after the workflow had given up on it. So ask Forgejo to push rather than waiting to see whether it has. `POST /repos/{owner}/{repo}/push_mirrors-sync` is synchronous: the handler loops the repository's mirrors calling SyncPushMirror inline rather than queueing it, and that runs `git push --mirror --force`. A 200 means every mirror holds the release commit and its tag by the time the call returns. One call covers every mirror the repository has, so a second outward forge needs no workflow change -- which is why this is preferred over probing each target. The step is best-effort by design: the handler returns 500 on the first mirror whose push fails and stops iterating, and the API has no per-mirror sync to route around a dead one, so the per-forge probe stays the verdict. It prints each mirror's sync_on_commit, interval, last_update and last_error either way -- last_error being the exact text the web UI shows under Settings -> Mirror Settings, which the old failure message sent an operator to go and read by hand. The poll that follows is resized for what it now measures -- GitHub indexing a push it has already received -- from 20 attempts to 8, and its error points at the mirror table rather than at the web UI. Verified by extracting both steps' scripts through a YAML parser and executing them against a stubbed API: sync 200 with a healthy mirror, sync 500 with a broken one, and the probe at 200, 401 and exhaustion. That is also what surfaced the four output defects fixed here -- Forgejo's error body carries no trailing newline, Python printed `True` where the API says `true`, a never-synced mirror printed `None`, and `::error::` is only an annotation when it starts a line, which until now depended on GitHub choosing to end its response with a newline. misc/ci-duplication-check.py: 88 -> 89 distinct step bodies, 3 duplicated and unexplained and 4 allowlisted both before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`licenses/LICENSE-AGREEMENT.txt` and the app resource copied from it are written by `generateLicenseReport`/`copyLicenseReport` on every build, and both were tracked. `.gitignore` already carried `licenses/*` -- it simply does not apply to a file git is already tracking, which is why the sibling `licenses/LICENSE-AGREEMENT` was correctly ignored while the `.txt` was not. The asymmetry was accidental. Two ways that cost something: - Any build with a non-default `-Pversion` dirties the tree. The renderer writes `ServerPackCreator ${project.version} license:`, so the documented verification command `./gradlew -Pversion="develop" build` rewrites `dev` -> `develop` in two tracked files every run. Easy to commit by accident. - Worse, running `./gradlew generateLicenseReport` ALONE truncates the notice. The report covers whatever projects that invocation configured, so a narrow request produces a legally empty document. Measured: committed 300,472 bytes listing 43 dependencies, standalone run 25,154 bytes listing ZERO. Nothing warned, and the result was commitable. So the fix is to generate them and track neither. `.gitignore` gains the app resource; `licenses/*` already covers the other. That removes the checked-in copy that was masking a second defect, so it is fixed here rather than left behind. `processResources` depends on `:copyLicenseReport` directly, and `copyLicenseReport` was wired only as `generateLicenseReport.finalizedBy(...)` -- a finalizer runs the copy after the report but does not make the copy REQUIRE it. With nothing committed, that packaged a jar with no notice at all. Measured before the fix: source deleted, `:copyLicenseReport` -> `NO-SOURCE`, BUILD SUCCESSFUL, destination absent, and the About dialog falls back to "Could not read resource". `copyLicenseReport` now `dependsOn` its producer. The guard for the remaining path took three attempts, and the first two are worth recording because both LOOKED like guards: doLast inside copyLicenseReport never runs -- a Copy with an empty source set is NO-SOURCE, and a skipped task runs none of its actions. BUILD SUCCESSFUL. inputs.file inside copyLicenseReport same. NO-SOURCE is decided before the declared input is snapshotted. doFirst on processResources FAILS, with the diagnostic. Both dead versions were run against a deleted source before being replaced, which is the only reason they were caught; each would otherwise have read as coverage. Verified: red source deleted + producer excluded -> BUILD FAILED naming the file green nothing on disk, `:serverpackcreator-app:processResources` alone -> generateLicenseReport, copyLicenseReport, notice present clean `./gradlew -Pversion="develop" build` BUILD SUCCESSFUL, 43 dependencies, 300,476 bytes -- byte-identical to the committed file but for the 4-character version string -- and `git status` free of licence churn Known and unchanged: the report lists 43 dependencies under a full `build` and 40 under `:serverpackcreator-app:processResources` alone, because the plugin reports on the configurations that invocation resolved. Releases run the full build, so the shipped artifact is the complete one, as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Three shapes the conventions name explicitly, all mechanical enough to be sure about. **Commented-out code, 14 lines across 12 files.** Eleven are `//preferredSize = dimension` in the Swing file-choosers, the rest a disabled `//checkForUpdateWithApi()`, two `//javaSetting.file = ...` lines bracketing a settings load/save, a dead `//unzippedModpack = unzipDestination(...)`, a `//source = "de/griefed/resources/manifests"` inside an isExe() branch and a stray `// Font("Arial Unicode MS", ...)`. Each was read in context first; none is referenced by the code around it. **Two comments whose subject was an earlier comment.** The convention names this one as its own anti-pattern. SecurityScans explained at length that an older doc "promised zip-slip and archive-safety checks ... which never existed in this class" — the fact a reader needs is that archive safety is zip4j's during extraction and not this class's, which is what it now says in one sentence. BootVerifier's "the same early return this made for itself when it owned the scan" described a function that no longer owns the scan; the first clause stated the whole rule by itself. **The two sentences commit8c7ac2719deliberately left.** Both characterization-test docs said the tests pin behaviour "before its decomposition" so "the upcoming restructuring" can be checked — a decomposition that happened and a restructuring that is not upcoming. They now state the property that is still true and still load-bearing: these assert what the code does rather than what it should, so an assertion changing here means behaviour moved rather than that a test needs updating. InclusionSpecificationTest's "Previously the class used reference-equality" becomes what reference-equality *would* cost, which is the reason not to reintroduce it. Compiles: api test, app main, clientside main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The three methods a plugin author MUST implement had no prose at all -- their KDoc opened straight into `@param`, so the published Javadoc showed a parameter table and no sentence saying what the method is for or when the host calls it. This is the first thing a third party reads. Each now states what it does, who calls it and what the contract is, verified against the call sites rather than inferred: ConfigCheckExtension.runCheck runs AFTER the built-in validators, on the same PackConfig instance; returning true fails the configuration, so the reason has to go into configCheck as well or the user is told only that something is wrong. TabExtension.getTab fires once at GUI startup, not per generation, so the returned tab is long-lived and owns its state. ServerPackHandlerBase.run sealed into Pre/PreZip/PostGen, which decide WHEN it fires, not what it is handed; a PreZip hook writing to `destination` still lands inside the archive, a PostGen hook does not. Two claims were corrected while writing them, which is the reason to check rather than describe. `ApiPlugins.runPreGenExtensions` catches `ExtensionException`, `Exception` AND `Error`, so a hook cannot fail a generation at all -- the doc says that rather than naming only the declared exception. And the pre-generation directory is only empty on a FIRST run: regenerating over an existing pack leaves that pack's files there until the copy overwrites them, which `ServerPackUpdater` exists to manage. BaseInformation's four members were "Get the name of this plugin. @return The name of this plugin." four times over. They now say where each value surfaces -- `name` is what `plugins.log` blames when an extension misbehaves, `version` is displayed as-is because SPC compares nothing against it. serverpackcreator-plugin-example moves in the same commit, as its own CLAUDE.md requires ("documentation-by-example: it must always reflect current API idiom"). 24 identical "Get the X of this addon" blocks collapse to one line each, and the overrides stop restating a contract the base now carries. Its first draft linked [BaseInformation.name]; that type is not imported there, so the links would not have resolved and they are gone. The four SPCListeners now say *when* they fire and what is on disk at that moment, copying the treatment SPCGenericListener already had. Verified: `:serverpackcreator-api:dokkaGenerateHtml` generates with no unresolved reference, api and plugin-example compile, and the measurement is 4549 doc blocks before and after with 2903 documented declarations unchanged -- nothing lost its documentation. Cross-referencing links 1351 -> 1362, doc lines 23343 -> 23203. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>RED ON PURPOSE -- the fix is the next commit, per the pin-first convention. `orphansIn` looks ahead from the end of a doc block for another doc block, and skipped blank lines only. A doc block, an annotation, then a second doc block is the same defect wearing a disguise: dokka still drops the first one, and the lookahead stopping at the annotation is what hid it. Exactly one instance exists, and this run names it: VersionMeta.kt:384 -- `update()` carries TWO KDoc blocks with `@Throws` between them. The older one ("Update the Minecraft, Forge and Fabric metas") is dropped; the one dokka keeps is the newer block documenting why the method is `@Synchronized`. The guard is widened here and watched red against that instance first, because a guard whose teeth were never checked has repeatedly turned out to assert nothing in this repository. It fails with exactly one path, which is also the evidence that the widening finds the real defect and not a class of false positives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Answers part three of the ask: how far a call goes from something an implementor can call. `misc/call-depth.py` reads the compiled classes and reports the distribution; `claude-docs/CALL-DEPTH.md` is the result. **The hypothesis holds.** 87% of 5,421 public methods bottom out within three hops and 2.5% go past six. Every deep chain is one of three named shapes -- the settings read (`ApiProperties -> <Area>Config -> PropertyStore`), a manifest read that may fetch, and the Strangler-Fig facades -- rather than accidental nesting. No chain from -api points outward, so the inward-dependency rule holds in the bytecode and not merely in prose. **Measure from bytecode, not from source.** A name-keyed graph over the Kotlin sources was tried first and is worthless: it merges every function sharing a name and produced `checkConfiguration -> setInclusions -> validate -> updateUI -> updateFont`, an API config check reaching Swing, at a fake depth of 54. Four things the tool does that a naive version gets wrong, each found by a measurement disagreeing with a hand-trace rather than by inspection: self-calls a call on `this` prints with NO owner class. Missing it hid every overload delegation -- ServerPackHandler.run reported depth 0 against a hand-trace of 9, and the graph was short by 2,808 of its 7,634 edges. descriptors keyed by name alone, Kotlin's default-argument overload chain looks like a cycle and truncates to zero. invokedynamic Kotlin 2.x compiles lambdas and SAM conversions to it. subtypes only resolving an abstract call by name+descriptor alone makes every data class's hashCode() an implementation of every other's; that fabricated 23-hop chains of unrelated value types on the first run. Two limits are stated in the report rather than hidden: pass-through hops are counted, so a one-line facade reads as nesting and the raw integer mildly punishes the extraction work that made this shallower -- read the chains, not only the number. And a chain through a host-supplied `fun interface`, or through reflection or pf4j dispatch, is not reachable at all. **B46 filed, not fixed.** `checkConfiguration` reaches `WebUtilities.downloadFile` through `getAndSetIcon` -- a call whose contract is "check this configuration" fetches a CurseForge thumbnail. Timeout-bounded, so a leak rather than a defect, and separating it is a behaviour change on published API because the check currently populates the icon as a side effect. **The Swing contradiction is fixed in writing.** The root CLAUDE.md forbade -api compile dependencies on Swing outright while -api imports `javax.swing` in five files, and -api/CLAUDE.md carved out `java.awt` only. The code is right -- a plugin must be able to ship a GUI tab and only -api is published -- so the rule now carries the same carve-out, names the five files, and gives the grep that re-derives them. Every audit so far has rediscovered this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`docker-test.yml` and `test.yml` had identical triggers (`push`, `workflow_dispatch`) and identical concurrency semantics (`<name>-${ref}`, cancel-in-progress). They were one workflow wearing two names, and a reader asking whether a commit is green had two places to look. The image check is now `test.yml`'s `docker` job, moved verbatim with its comments -- including the one explaining why it deliberately does not push. **`grinder-container-it.yml` is NOT folded in, and that is structural rather than a preference.** Its concurrency group has no ref in it and `cancel-in-progress: false`, so exactly one run happens at a time across the whole repository; `test.yml`'s is per-ref and cancellable. A workflow can hold only one concurrency block, so merging means picking one of those -- and **Forgejo has no job-level concurrency**. That was read rather than assumed: `ConfigureActionRunConcurrency` evaluates `workflow.RawConcurrency` onto the `ActionRun`, and `models/actions/run_job.go` does not mention concurrency once. Both outcomes of merging it anyway are already on this repository's record. Dropping the repo-wide lock re-opens the interference that failed runs 947, 954 and 956 on a *different* test of the same class each time -- which is the signature of interference rather than of a defect. Taking the lock for all of `test.yml` serialises every push, which `.claude/rules/ci-workflows.md` already says was considered and rejected at the cost of a second full 35-minute run in series. So it stays until Forgejo grows job-level concurrency, and the rules file now says so in its own section rather than leaving the next person to rediscover it. Documentation that stated present fact was corrected rather than left to rot: the allowlist entries naming `docker-test.yml`'s retry steps, CI-SECRETS' table row, `release-build.yml`'s comment citing the retry, RUNNER-REGISTRY-CACHE's cache-probe sentence, and the "creating alpha/beta starts five workflows" count, now four. The dated measurements that name the old file -- the saturation table, the three ghcr 429 runs, the 2026-10-02 `pull_request:` removal -- are history and keep its name. misc/ci-duplication-check.py: 89 distinct step bodies, 3 duplicated and unexplained, 4 allowlisted, all unchanged -- the move preserved every step body byte for byte, which is why the allowlist's content hashes still resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`serverpackcreator-homepage` is a subtree of a separate repository, and the mode it is pulled with decides whether SPC's next changelog is about SPC. A plain `git subtree pull` merges the website's tip into this history and makes all of its commits ancestors of `develop`; `.releaserc.yml` uses a bare `tagFormat: '${version}'`, so semantic-release analyses every one of them and reads their conventional-commit subjects as SPC's own. The original `git subtree add` did precisely that, and `breaking: Move ServerPackCreator from griefed.de to its own website` (2023-05-01) alone would have released 10.0.0 instead of 9.3.0. Nothing enforces the mode, so the rule goes where it is read: the root module map for anyone scanning the repository, and a module `CLAUDE.md` that Claude Code lazy-loads when work is happening in that directory. The module file records only what the directory's own files cannot answer -- its provenance, the pull command, and that `git fetch`'s tag auto-following is how the website's `1.0.0`-`1.3.1` tags got into SPC's release namespace. How the image is built, how the versioned documentation is assembled and which versions the site serves are already stated in `Dockerfile`, `docs/assemble-docs.sh` and `docs/versions.json`, so the file points at those rather than keeping a second copy to go stale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>---or you live long enough to see...to ...or you live long enough to see...`spc.install4j` reads `licenses/LICENSE-AGREEMENT.txt` for the installer's licence screen, and that file is written by `generateLicenseReport`. Nothing in `media`'s task graph ran it: `media` dependsOn `tasks.build`, which is the ROOT project's build -- that project has no sources and reaches no subproject, so `-app`'s own `build { dependsOn(":generateLicenseReport") }` never applies here. It stayed invisible while the file was tracked, because the checkout supplied it. `stop tracking the generated third-party licence notice` untracked it -- rightly; it is generated and `licenses/*` was already in `.gitignore` -- and the first `media` run after that failed: install4j: compilation failed. Reason: java.io.FileNotFoundException: .../licenses/LICENSE-AGREEMENT.txt (No such file or directory) Measured, `./gradlew media --dry-run`: before 9 tasks, every one in the root project; no :generateLicenseReport after :generateLicenseReport and :copyLicenseReport in the graph A `dependsOn` that merely makes the file EXIST is not sufficient, and that is the half worth checking rather than assuming: the commit above recorded that a narrow invocation of `generateLicenseReport` silently writes a legally empty notice -- 25,154 bytes listing ZERO dependencies -- so a plausible fix ships installers with a blank licence screen and reports success. Verified by deleting both files and running exactly what CI runs: baseline (1 of 43), 300,476 bytes regenerated (1 of 43), 300,476 bytes, byte-identical End-to-end against install4j 13.1: BUILD SUCCESSFUL in 7m 4s, all three installers written. The red half is Forgejo run 1056's re-run, whose `install4j media` job is this code without this edge and fails as quoted above; every other job in that re-run is green. 9.2.1 does not carry the untracking commit, so no release shipped this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Reported from the grinder running under systemd. A truncated forge-manifest.json ended every start with com.fasterxml.jackson.core.io.JsonEOFException: Unexpected end-of-input: was expecting closing quote for a string value [line: 4477, column: 11] at ...JsonUtilities.getJson(JsonUtilities.kt:219) at ...ForgeLoader.update(ForgeLoader.kt:99) at ...VersionMeta.<init>(VersionMeta.kt:313) at ...ApiWrapper.setup(ApiWrapper.kt:381) at ...GrinderApplication.main(GrinderApplication.kt:146) and Restart=on-failure turned that into a loop. The host has 1.8 TB free, so this was a kill mid-write rather than ENOSPC -- the background manifest refresh runs at startup, which is exactly the window a `systemctl restart` lands in. Two defects, and the second is why it was fatal rather than annoying. The write was truncate-then-write: updateManifest did create() then writeText(content) in place, so a killed JVM leaves exactly as many bytes as it managed. And nothing could ever repair the result. ApiWrapper.setup() seeds the manifests from the jar through JarUtilities.copyFolderFromJar, whose inner loop is guarded by `if (!destination.exists())` -- correct for "do not clobber a refreshed manifest", fatal for "this file is a half-write". The refresh could not undo it either: checkManifest decides on *version counts*, so it parses the local copy too, threw before the comparison, and discarded the working download already in memory. The manifest a host could not read was therefore the one manifest it could never replace, and deleting the file by hand was the only exit. Prevention. File.writeTextAtomically stages into a sibling and ATOMIC_MOVEs it into place, so a killed or failed write leaves the previous manifest rather than part of the new one. ManifestUpdater is its only caller today. Its temporaryFile parameter exists only so the failure can be executed -- the same reason ModpackZipInspector takes an openZip; a partial write cannot be provoked from a test. Repair, in three places, because each covers what the others cannot: - ManifestUpdater adopts the upstream copy over a local one it cannot parse, so an online host heals to current data rather than to the shipped snapshot. An upstream answer that cannot be parsed is still never adopted -- taking it would turn a transient truncated response into the same permanent failure. - ManifestIntegrity.restoreIfUnreadable restores the jar's copy, or deletes a file the jar does not carry (the mcserver/<version>.json files), and clears both the .etag sidecar and the modification time. The sidecar earns a 304, which is a promise that the local copy IS the upstream one; the modification time would otherwise tell the four upstreams that honour If-Modified-Since that "now" is newer than anything they have, stranding the host on the snapshot until upstream happened to publish. - VersionMeta.updateOrRepair is the wiring: every meta's update() runs through it, and a failure repairs that meta's manifests and retries once. A failure that repaired nothing is RETHROWN unchanged, deliberately -- the alternative is an empty meta, and this module has the receipts for what an empty serverReleases() costs (every candidate refused with "No bootable combination", i.e. the engine's own state published as a statement about a mod). Deliberately NOT a proactive scan of the manifests directory in setup(): it would parse ~500 KB on every start for a case that is rare, and the metas parse the same files seconds later anyway. The repair belongs on the failure path. Same shape one level down, found while reading the path and fixed here because it shares the reason: MinecraftServer.readServerJson discards an mcserver/<version>.json it cannot parse. It never crashed anything -- both callers catch -- which is why it was worse. Every lookup re-read the same broken file and returned Optional.empty(), which every consumer reads as "this version declares no required Java": REQUIREMENT_UNKNOWN masquerading as JDK_NOT_BUNDLED, permanently. Behaviour change on published API, row appended to claude-docs/API-BEHAVIOUR-CHANGES.md. Also: checkManifest's SAX catch no longer restores from the jar. Only the upstream answer can reach it now -- an unreadable local copy is handled where it is counted -- and overwriting a working local manifest because upstream sent rubbish is the opposite of a repair. Pins written and observed RED first, in one commit because they share one reason. The reds, and what produced them: - VersionMetaManifestRepairTest reproduced the host's own message verbatim, from a forge-manifest.json truncated to 90,000 bytes: "JsonEOFException: Unexpected end-of-input: was expecting closing quote for a string value". The XML half went red with 'SAXParseException: Elementtyp "version" muss mit dem entsprechenden Endtag "</version>" beendet werden' from a neoforge-manifest-new.xml truncated to 40,000 bytes. - ManifestIntegrityTest: 5 of 6 red against a stub returning false. - ManifestUpdaterTest.anUnreadableLocalManifestIsReplacedByTheUpstreamCopy, red against the old count comparison -- the local manifest kept its 40 truncated bytes. - AtomicTextWriteTest.aFailedWriteLeavesThePreviousContentsIntact, red against create()+writeText(). - MinecraftServerManifestCooldownTest.anUnparseableCachedManifestIsDiscardedSoItCanBeFetchedAgain, red against the unguarded getJson. Two seams (File.writeTextAtomically, ManifestIntegrity) were landed stubbed so the red is the missing behaviour and not a missing symbol -- the third sanctioned route for new code. The first run of VersionMetaManifestRepairTest went red for the WRONG reason (UnrecognizedPropertyException: the fixture built a bare ObjectMapper where ApiWrapper disables FAIL_ON_UNKNOWN_PROPERTIES), which is why the convention says to read why a pin failed before trusting it. Three of the new guards are GREEN against the old code by design. They are there to stop this change breaking what already worked, so name them rather than let them be mistaken for pins: anUnreadableUpstreamManifestIsNotAdopted, aRefreshLeavesNoStagingFileBehind, and aManifestWithNoModificationTimeIsFetchedUnconditionally -- which pins the JDK behaviour the modification-time reset depends on (HttpURLConnection omits If-Modified-Since entirely when it is zero), load-bearing and non-obvious enough not to leave assumed. Suites, all green: api 520 (from 504), grinder 543 with its 27 gated skips, clientside 680, app 234. The app suite's first run had 25 failures, every one of them the documented flapdoodle-and-stale-jar flakiness of three concurrent Gradle builds; clean on a re-run of the same commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Follow-up to the manifest repair, same failure one layer down and reached by every artifact SPC fetches rather than only by the twelve manifests. `downloadFile` called `file.create()` -- which truncates -- then streamed into the destination, and `deleteQuietly()`d it when the stream threw. Two consequences, and only the first is obvious. A download that merely FAILED destroyed the copy the host already had. The destination was truncated before the connection was even opened, so an unreachable host, a 404 or a dropped connection left nothing where a working file had been -- an installer, a pack icon, a per-version Minecraft manifest. And a KILLED download left a partial file, which satisfies `isFile`, which is exactly what `return file.isFile` reported as success. Every caller reads a `true` as "I now have this artifact": `ForgeMeta.installerFor` and its NeoForge, Fabric, Quilt and LegacyFabric siblings hand the destination straight back as an installer and never re-download it (`if (!destination.isFile)`), `HttpJarDownloader` returns it as a mod jar to be staged, `MinecraftServer` parses it, and `ModpackManifestParser` adopts it as a pack icon. A truncated installer therefore cached as a complete one for ever -- the same shape as the truncated manifest, except it is served from cache rather than parsed on startup, so nothing ever reports it. `File.writeTextAtomically` generalises into `File.writeAtomically`, which hands a staging sibling to a writer and `ATOMIC_MOVE`s it into place; the string overload delegates to it and `downloadFile` streams into it. The return value had to stop being derived from `isFile` in the same change: once the destination survives a failure, "a file is there" and "this call wrote it" are different questions, and only the second is the one every caller is asking. The default staging name is **unique per call**, not derived from the destination. Shared, two writers interleave their bytes into it and then move the mixture into place -- the corruption this helper exists to prevent, one path along -- and it is reachable rather than theoretical: the grinder runs N workers against one shared installer cache and the five `installerFor` methods take no lock. Given one each, both stage a complete copy and whichever move lands last wins. Behaviour change on published API; row appended to claude-docs/API-BEHAVIOUR-CHANGES.md. The part worth reading before upgrading is the preserved file: code that used the deletion as a signal ("the file is gone, therefore the download failed") must read the return value instead. `downloadAndReplaceFile` is untouched and still deletes first, by its own documented contract. Pins written and observed RED first: - `aFailedDownloadLeavesAnExistingFileIntact` failed with `FileNotFoundException` on the destination -- the failed download had deleted the file the test wrote before it. - `aFailedDownloadReportsFailureEvenThoughTheOldFileSurvives` failed on its second assertion, the old file being gone. - `eachWriteStagesIntoAFileOfItsOwn` failed against the shared `"$name.tmp"`, two writes to one destination yielding one staging path. Two of the five new guards are GREEN against the old code by design, and guard this change rather than pinning the defect -- `aSuccessfulDownloadLeavesNoStagingFileBehind` and `aFailedDownloadLeavesNoStagingFileBehind`, which is what stops staging accumulating one file per attempt beside the artifact it failed to become. NOT covered by a test, stated rather than implied: the killed-mid-download case itself. A kill cannot be staged from a test, so it is covered by construction -- partial bytes can only ever land in the staging file -- and the two staging guards are what prove the mechanism is wired. The same limit applies to `writeAtomically`'s own contract, which is why its `temporaryFile` is injectable at all. Suites, all green in one clean sequential run: api 525, clientside 680, grinder 543 with its 27 gated skips, app 234 — 1,982 tests, zero failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Both ends of the same problem: how a server pack's text files are written, and what the pack does when one of them comes back changed. They are one commit because the second half is what makes the first half worth anything -- writing variables.txt correctly does not keep it correct, since the file travels. WRITING: a generated run-file's endings are chosen by its reader Every file createServerRunFiles wrote ended its lines by deleting carriage returns, at four copies of `.replace("\r", "")`. Right for CRLF input, wrong for everything else, in two separate ways. A template separated by LONE carriage returns lost its separators instead of having them converted, collapsing into one line. Measured against the guard: `#!/usr/bin/env bash\recho one\recho two\r` came out as `#!/usr/bin/env bashecho oneecho two` -- no shebang, no commands, and still a file that parses. Nothing reports it. And start.bat shipped LF-only, because `bat` is in defaultStartScriptTemplates() and the deletion ran over every template alike. cmd.exe resolves a goto/call label by scanning the file and misses one crossing a 512-byte boundary when no carriage return has reset its line counter (dostips.com/forum/viewtopic.php?t=8988, Dave Benham, reproduced there on Windows 10). The shipped default_template.bat has no label, so nothing was broken -- but the templates are operator-editable, which is what makes it worth closing rather than noting. serverpackcreator-plugin-selfextract's Stubs.batch had already reached the same conclusion for its own .cmd and joins with \r\n. String.withLineEndings is now the one place that decides. It normalises to LF before expanding, so a carriage return is replaced and never deleted, and an already-CRLF template is not doubled into \r\r\n. ServerPackProvisioner.lineEndingFor picks the convention from the written file's extension: CRLF for bat/cmd, LF for everything else. The four writes go through writeTextAtomically now, so a killed generation leaves the previous pack's file rather than a truncated one, and variables.txt is made writable first like its three siblings already were -- without that, the atomic move would succeed on POSIX (rename needs the directory's permission, not the file's) and fail on Windows. A template's own endings no longer reach a user, which makes .gitattributes' comment about the server_files templates wrong; corrected rather than deleted, since keeping that directory LF in the working tree still costs nothing. READING: the pack survives a variables.txt that arrived changed ServerPackCreator writes that file LF-only and without a byte-order mark and cannot keep it that way. An FTP client in ASCII or Auto mode rewrites .txt on upload, a hosting panel's web editor saves what its browser gave it, an editor on Windows adds what it likes. The start script ships with the pack, so it is the last point of control. Carriage returns: sourced as-is, one is attached to every value. "$JAVA" -version fails as `java\r: command not found`, with a message a terminal mangles by rewinding its cursor over it, and WAIT_FOR_USER_INPUT is never equal to "true" however the operator sets it. A byte-order mark: three invisible bytes that stick to the FIRST setting's name, so JAVA becomes a name nothing reads and the operator's Java choice is ignored while every setting below it still works. In bash the line stops being an assignment and parses as a command. The two shells failed differently, so one fix was not the fix. Measured against the shipped functions beforehand: bash broke on CRLF, on lone CR and on the mark; fish already survived CRLF -- `string trim` counts a carriage return as whitespace -- and broke on lone CR, which its `read` hands over as one single line, and on the mark, which it does not count as whitespace. Three choices in there that look arbitrary and are not: eval "$(tr ...)", not source <(tr ...). bash 3.2, which is what macOS ships, sets NO variable at all from a process substitution handed to source, and reports nothing: every value comes back empty and the pack starts with its settings gone. Verified on 3.2.57 (silent, all empty) against 5.2.15 (works). That was the first spelling tried and would have shipped a worse bug than the one being fixed. tr '\r' '\n', not tr -d '\r'. Same rule as the writing side: deleting the separator collapses a lone-CR file into one line, which still parses. The mark is stripped anchored to the start of the file, never deleted globally -- those bytes are a legal lead byte inside ordinary UTF-8. Four templates, not two: default_java_template.sh and .fish guard and read variables.txt themselves, because they run standalone as well as being sourced by the start script. Found by grepping for the other readers before calling it done, which is what the "a fix written against one artifact is a fix for one artifact" entry in CLAUDE.md asks for. The comments in those four are written for the operator rather than for us -- Griefed's call. The scripts ship inside every pack and are write-protected but editable, so whoever opens one has to be able to follow it: what goes wrong in plain words, then what the line does about it. The mechanism notes behind the awkward spellings stay, because those are the parts someone would otherwise tidy back into a bug. PINS Each guard was written and run before its fix, and each red was read rather than assumed. Grouped here, so the message carries what the boundaries would have: RunFileLineEndingsTest: 3 of 4 red on content mismatch, never on a missing symbol. Reverting `.withLineEndings(lineEndingFor(name))` to `.replace("\r", "")` at either script loop reproduces it. The fourth guard, theFilesAPosixShellReadsCarryNoCarriageReturn, was green from the start on purpose: it pins the half that was already correct so this cannot regress it. VariablesFileLoadingTest: the bash half got a behaviour-preserving seam first -- loadVariables() { source "$1"; } plus both call sites switched to it, which does exactly what the two `source` lines did -- so the guard fails on the logic and not on a missing function. Before each fix, bash failed first on crlf, fish only on cr, and exactly the three bom_* cases failed on JAVA alone across all four templates. Its fixture puts JAVA first deliberately: a variables.txt whose first line is a comment is read correctly by a loader that handles nothing at all, so that fixture could not fail. Teeth verified by mutation, not by green. Removing the mark strip from all four templates names all four; removing it from default_java_template.sh alone names that one -- which matters, because the bash guard stops at its first failed assertion, so "it went red" does not prove the second template is covered. One guard had no teeth and was fixed: the fish probe passed against a deliberately corrupted installer template, because fish keeps a function defined when a later `source` fails, so the first template's working copy was silently grading the second's. It now erases the function between templates and fails loudly if the source or the definition does not take. VERIFICATION api 538 tests green, app 234 green, plugin-servertest 112 green (1 deliberate skip), no new compiler warnings, fish and PowerShell still parse. Checked against really generated packs rather than only fixtures -- all five under serverpackcreator-api/tests/server-packs/ after a suite run: start.bat 38 of 38 lines CRLF; start.sh, start.fish, start.ps1, install_java.sh, variables.txt and HOW-TO-RUN.md with 0 lines carrying a carriage return; no staging .tmp left behind; the zips carry the same files. ON THE REPORT THAT PROMPTED THIS It does not explain itself. 8.1.0 already stripped carriage returns at all four write sites and File.writeText does no platform translation, so a variables.txt 8.1.0 wrote was LF-only by construction. Whatever that user's file picked up, it picked up after generation -- which is the half this commit's reading side now survives. Still open: nothing reads start.sh itself if IT arrives with carriage returns. That fails loudly (`bad interpreter`) rather than quietly, and a script cannot repair itself before being run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Every Renovate PR on Forgejo failed in `:serverpackcreator-app:bootJar`: Entry BOOT-INF/lib/babel-monorepo.jar is a duplicate but no duplicate handling strategy has been set. `test.yml` passes `-Pversion="${{ github.ref_name }}"`, which on a Renovate branch is `renovate/babel-monorepo`. `version` is a FILE NAME COMPONENT -- every archive is `<module>-<version>.jar` -- and Gradle does not check that it is a legal one, so the slash was read as a path separator and the jar landed in `build/libs/serverpackcreator-api-renovate/babel-monorepo.jar`. Boot copies each dependency into `BOOT-INF/lib/` under `file.getName()`, which for `-api` and `-clientside` alike was now `babel-monorepo.jar`. Two jars, one entry name. Reproduced locally on develop, and the directories are the proof: $ ./gradlew :serverpackcreator-app:bootJar -Pversion="renovate/babel-monorepo" > Entry BOOT-INF/lib/babel-monorepo.jar is a duplicate ... BUILD FAILED in 8s $ find serverpackcreator-*/build/libs -maxdepth 1 -type d -name '*renovate*' serverpackcreator-api/build/libs/serverpackcreator-api-renovate serverpackcreator-app/build/libs/serverpackcreator-app-renovate serverpackcreator-clientside/build/libs/serverpackcreator-clientside-renovate That is also why only PR #713 was green: its head is `develop`, every other open PR is `renovate/<slug>`. `.github/workflows/test.yml` has the identical defect on every pull request, where `github.ref_name` is `<number>/merge`. NOT fixed with `duplicatesStrategy` on `bootJar`, which the error message invites: that is green by dropping one of the two modules out of the shipped fat jar. Fixed in the build, because the invariant is a build one and the alternative is the sanitising living in each workflow that passes a ref-derived version, where the next one written will not have it. `BuildVersion.sanitize` replaces every run of file-name-illegal characters with a single `-`, and `BuildVersion.of` says so at lifecycle level, so a rewritten version is auditable from a CI log. It is applied in `java-conventions`, which every jar-producing module reaches, and in the root script, which applies no convention plugin and feeds install4j. A release is untouched: `resolve-version`'s `tag` mode already refuses anything that is not `X.Y.Z` or `X.Y.Z-(alpha|beta).N`, and `-Pversion=9.3.0` logs nothing and produces the same names as before. Measured, per the build-logic standard: ./gradlew :serverpackcreator-app:bootJar -Pversion="renovate/babel-monorepo" before: FAILED, Entry BOOT-INF/lib/babel-monorepo.jar is a duplicate after: BUILD SUCCESSFUL in 36s serverpackcreator-app-renovate-babel-monorepo.jar, and inside it both BOOT-INF/lib/serverpackcreator-api-renovate-babel-monorepo.jar BOOT-INF/lib/serverpackcreator-clientside-renovate-babel-monorepo.jar plugin jars, which the release globs as `*_experimental.jar`: before: build/libs/<module>-renovate/babel-monorepo_experimental.jar (glob misses it) after: build/libs/<module>-renovate-babel-monorepo_experimental.jar (glob matches) THE CI HALF, same defect one layer up. `resolve-version`'s `branch` mode ends at `VERSION="${REF_NAME}-$(git rev-parse --short HEAD)"`, so a slashed ref produced a slashed version there too. It was latent -- `devbuild.yml` triggers on `push: develop` plus dispatch -- and the build-side sanitiser only keeps such a run GREEN, it does not keep the two in AGREEMENT: `inputs.version` labels the release assets, the installer media, the dokka zip and the `continuous` tag while the build names the jars inside them. Normalised once after the `case`, covering all four branch-mode paths and `tag` mode, where it cannot fire. Executed, not reasoned about: branch renovate/babel-monorepo -> renovate-babel-monorepo-444f32dc8 (announced) branch develop -> develop-444f32dc8 (untouched, silent) override "my/custom ver" -> my-custom-ver (announced) tag 9.3.0 / 9.0.0-alpha.6 -> unchanged, prerelease false / true tag not-a-version -> still refuses PIN FIRST. `BuildVersionTest` was written and run RED against a stubbed `sanitize` returning its input unchanged -- 4 of 7 failed, and the three that passed were vacuous against that stub and are named here so they are not mistaken for pins: `a release version passes through untouched`, `a branch mode version passes through untouched`, `legal punctuation survives`. The red: expected: <renovate-babel-monorepo> but was: <renovate/babel-monorepo> expected: <713-merge> but was: <713/merge> expected: <feature-windows> but was: <feature\windows> expected: <feature-a-b> but was: <feature/ a : b> The bash and Kotlin normalisations are a FIFTH instance of duplicated knowledge -- the two cannot share a regex -- so the duplication is pinned rather than commented: `BuildVersionTest` extracts the `sed -E` program OUT OF THE ACTION and runs it over every case the class asserts, and a cheaper string guard catches a divergence on a host with no `sed` instead of skipping silently. TWO WIRING FINDINGS, both of which would have left a guard that never runs. A buildSrc test source set reverses a recorded decision -- Griefed's call, 2026-10-16 -- and is for PURE FUNCTIONS; task wiring still gets a commit-message measurement. Gradle 8.0 stopped the main build running buildSrc's lifecycle tasks (gradle#25439, closed as intended), and measured on 9.8.1 `:buildSrc:check` is inert, `gradle.includedBuild("buildSrc")` does not resolve, and `-p buildSrc check` dies in buildSrc's own settings, which declare the foojay resolver with no version because only the root build puts it on the classpath. The jar IS always built, so `tasks.jar { finalizedBy(tasks.test) }` is the edge -- `finalizedBy` and not `dependsOn`, because kotlin-dsl puts that jar on the test compile classpath and `dependsOn` is a cycle that fails `./gradlew help` outright. And mutating the action's expression left the suite GREEN, because `.forgejo/actions/resolve-version/action.yml` was not a declared input of `:buildSrc:test` -- Gradle reported UP-TO-DATE and the guard could never have fired on the single edit it exists to catch. With `inputs.file(...)` declared the same mutation turns both guards red: the action and the build disagree about 'feature/ a : b' ==> expected: <feature-a-b> but was: <feature--a---b> Recorded as a landmine, because it applies to any guard reaching across a project boundary: ask what makes a test re-run, not only what it asserts. Deferred as B48 -- moving buildSrc to a `build-logic` included build, which would make root `check` the one honest gate instead of the jar finalizer above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>typecould cost the mod its whole descriptor 4de775286d