Tequila! #676
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!676
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?
Three guards, all red against this commit's production code, each for the defect it names rather than a fixture fault: configModeCapturesTheConfigFileItWasGiven -config <nonexistent> discards the path (isPresent false), which is what leaves ServerPackCreator.run() calling get() on an empty Optional. pathArgumentsWithoutAValueAreReportedNotThrown IndexOutOfBoundsException: Index: 1, Size: 1 at CommandlineParser.kt:218 homeArgumentWithoutAValueIsReportedNotThrown IndexOutOfBoundsException: Index: 1, Size: 1 at CommandlineParser.kt:108 Run before committing, per the convention that a guard committed red is only evidence when the red is the missing implementation: 16 tests, 3 failed, and the three stacks above are the parser indexing past the end of its own argument list. The existing assertion that a non-existent -config path leaves serverPackConfig empty is inverted here rather than kept. That is a deliberate contract change, not a refactor, which is why the fix commit that follows is labelled fix: -- parsing should record what the user typed so the run can name it, and deciding whether the path is usable belongs to the command that runs it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>-config with a path that is not a file left serverPackConfig empty, and the CONFIG dispatch then called get() on it -- a bare NoSuchElementException, with the path the user mistyped already discarded so nothing could name it. -config with no path at all never got that far: reading argsList[indexOf(flag) + 1] unguarded threw IndexOutOfBoundsException while still parsing. Both halves are one defect -- the parser deciding whether a path is usable and throwing away the evidence either way -- so the whole class is fixed rather than the one spelling that was reported. pathAfter() records the path as given and returns null only when the flag is absent or is the last word on the commandline; -cgen, -feelinglucky, --setup, --destination and --home all read through it, because all five indexed past the end of the argument list in exactly the same way. Validation moves to whoever runs the thing, which is where the path can be named: runHeadless already warns "<path> not found...", feelingLucky already logs a modpack-directory that does not exist, and --setup/--home now say which path they rejected. Only "no path given at all" needs a guard at the dispatch, because there is nothing to report. Verified against the real application, not just the suite -- :serverpackcreator-app:bootRun: -config /definitely/not/here.conf WARN (RunHeadlessCommand.kt:131) - /definitely/not/here.conf not found... -config ERROR (ServerPackCreator.kt:162) - -config requires the path to a server pack config, e.g. -config "/path/to/serverpackcreator.conf". Both previously aborted with a stack trace. Suite: 151 tests, 0 failures. Residual risk stated rather than assumed: the two dispatch guards in ServerPackCreator.run() are not unit-tested, because run() boots the API and there is no seam to drive it from a test. They are what the two bootRun transcripts above exercise. The parser half is pinned by CommandlineParserTest. Note for anyone scripting this: a mistyped path used to exit non-zero by virtue of crashing, and now exits 0 like every other headless failure. That inconsistency is pre-existing and deliberately not changed here -- the headless verbs signal nothing through their exit code, and giving one of them an exit code is a separate decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>runwith no subcommand actually does 8a6fa73ddePure move. Same question, same Windows escaping hint in the same position (it is appended to the question so it still lands between the question and the first "Path: "), same "Directory '<path>' does not exist.", same restart notice. The file's @Suppress("DuplicatedCode") goes with it -- there is no longer a duplicate to suppress. One thing a reviewer should check rather than take on trust: the stored value is now the File's path instead of the raw typed line, which differs only by a redundant trailing separator. It is not observable, because the preference is only ever read back through File(...) (PathsConfig.homeDirectory, line 127), and File("/a/b/") and File("/a/b") resolve to the same absoluteFile. No assertion changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`run withSpecificConfig` with no -c prompted "Enter the full path to the new ServerPackCreator home-directory.", then validated isFile and returned it as the server pack config. A user who answered the question as asked -- with a directory -- was told "File '<dir>' does not exist.", which is both wrong and unhelpful about what it actually wanted. It is a copy of HomeDirCommand's loop with the validation swapped and the question left behind. Both files carried @Suppress("DuplicatedCode"), so the annotation was sitting on the copy that held the bug. The question now says "Enter the full path to the server pack config." and the loop is ConsolePrompt.readExistingFile. This is the defect that justifies the extraction: it is not a bug anyone would write on purpose, it is what duplication does over time, and it survived because the two copies were marked as known duplicates rather than merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Written against the current constructor, so the guard compiles and the red is the defect rather than a missing seam. langAcceptsAndStoresTheLocaleItDisplayed "lang must accept the locale it just displayed" expected: <false> but was: <true> -- "Unsupported locale en_GB." really is printed for the value the command itself just listed. langRefusesALocaleItNeverOffered "must go on to accept a listed locale" expected: <pt_BR> but was: <null> -- pt_BR is offered and refused too, so nothing is ever stored. Two defects, one contract, so one pin. printAvailableLanguages prints the locale (en_GB) while the loop compares locale.language (en), so every offered value is refused; and having validated scanner.next(), the command passes scanner.nextLine() -- the rest of that line, which is "" -- to changeLocale, so even an accepted answer stores the wrong locale. The second is invisible until the first is fixed, because nothing gets that far. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the two guards from the previous commit green. lang listed en_GB / pt_BR / zn_GB and matched against locale.language -- en / pt / zn -- so it refused every value it had just offered. There was no way to use the command as displayed. And once past that, it validated scanner.next() but handed scanner.nextLine() to changeLocale: the remainder of the same line, which is "", so an accepted answer stored Locale("") rather than the locale chosen. Both go away together, because readChoice is given one map: its keys are what gets listed and what gets matched, and its values are what gets returned. A locale that is offered can be chosen, by construction rather than by agreement between two pieces of code. printAvailableLanguages is gone -- listing the choices is part of asking for one. One existing fixture changed, which is the signal this is a behaviour change and not a refactor: InteractivePromptStdinTest.langPromptLeavesSystemInOpen fed "en" and now feeds "en_GB". It failed with NoSuchElementException: No line found -- "en" is no longer accepted, so the command kept asking until the fed input ran out. That test asserts stdin handling, not locale handling, so only the answer changed; its assertion is untouched. Suite: 162 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Two halves of the same failure. ReportServer now selects through VerdictSnapshotCache rather than store.all(), and its thread count is SPC_GRINDER_HTTP_THREADS, default 4 where it was a hardcoded 2. Why the pool matters as much as the cost: the JDK's HTTP server hands every request to this pool, so a request that blocks costs a whole thread and a pool with nothing free stops answering *everything* -- including /status and /dashboard, which do no store work. From outside that is indistinguishable from a dead host. The socket still accepts, nothing ever replies, and the proxy returns a 502; measured against the public instance at 131.3 s, the same for /, /status, /dashboard and a path that does not exist, while port 80 answered a redirect in 0.18 s. Raising the pool alone would only have bought a bigger window, which is why the derivation cost went first. Documented in three places because a knob missing from any of them is invisible to a guard: KNOBS (else neither documentation test sees it), the README table plus a new "Report responsiveness" section carrying the measurements, and the systemd unit. The reader call is deliberately on one line -- everyVariableReadIsDeclaredAsAKnob matches `reader("NAME"` and a wrapped call is invisible to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Six concerns, each pinned before it was changed. -config with a path that was not a file discarded the path and then called get() on the empty Optional -- a bare NoSuchElementException naming nothing -- and with no path at all indexed past the end of the argument list. pathAfter() records what it was given; deciding whether it is usable belongs to whoever runs it, which is the only place that can name it. The same crash lived in -feelinglucky, -cgen, --setup, --home, --destination and, found a pass later, in the four scan and clientside branches. All four interactive prompts closed the Scanner wrapping System.in. Scanner.close() closes its source, System.in cannot be reopened, and JLine's POSIX terminal holds that same descriptor as its pty slave, so the next readLine died with ioctl(TIOCGWINSZ) = -1 inside a java.io.IOError -- an Error, which the shell's per-line catch missed. One prompt per session, then the session ended. Reported against 8.1.2 and never fixed, only re-documented. The four prompt loops were copies and had drifted into three defects: `run withSpecificConfig` asked for a home-directory while validating a config file, cgen said "File" about a directory, and lang listed en_GB/pt_BR/zn_GB while matching en/pt/zn -- refusing every value it offered -- then stored Locale("") from the remainder of the accepted line. They are now one ConsolePrompt, whose readChoice takes a single map so that listing what it will not accept is unrepresentable rather than merely fixed. The headless verbs signalled nothing through their exit code. run() returns one and main exits with it -- but only on failure, because GUI and WEB return as soon as they hand off to the Swing EDT and the embedded server, and exiting on success would kill them. Verified against the jar, -gui included. The grinder's report re-derived the whole verdict store per request: 251 ms to select against 3 ms to render the 250 rows it sent, at the deployed row count. Pagination bounds only the render. With a 2-thread pool that is how the public instance stopped answering *everything*, including endpoints that do no store work -- measured at a 502 after 131.3 s while port 80 answered in 0.18 s. The derivations are now cached per store change (329 ms to 5.4 ms) and the pool is SPC_GRINDER_HTTP_THREADS, default 4. Full build green: 1871 tests, 0 failures, 30 skipped, across all six modules. Two guards were caught asserting nothing and rewritten rather than trusted: the report's equivalence test compared two overloads after one began delegating to the other, and stayed green with filtering disabled outright. Both mechanisms were mutation-checked afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Found while adding updateProtectedPaths. Every collection-valued property in GenerationConfig is declared `var x = fallbackX`, which makes the property and its fallback the same TreeSet, and every setter does `field.clear(); field.addAll(value)`. Configuring one therefore overwrites the constant it is supposed to be able to fall back to. Reproduced: fallbackZipExclusions reads [libraries/net/minecraft/server/.../server-MINECRAFT_VERSION.jar, minecraft_server.MINECRAFT_VERSION.jar, server.jar] on a fresh GenerationConfig, and [only-this.jar] after `zipArchiveExclusions = TreeSet(listOf("only-this.jar"))`. It is user-visible: GlobalSettings.kt's four reset-to-default buttons (zipReset, inclusionsReset, preInstallFilesReset, postInstallFilesReset) each read a fallback, so pressing reset after a save restores the user's own values rather than the shipped ones. Two guards, both red: the fallback must survive configuring its setting, across all six list-settings, and a setting must not be the same object as its fallback in the first place. updateProtectedPaths is in both and already green -- it builds a fresh set per read -- so the guards fail only on the five that alias. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Found by the analysis pass on develop, recorded as H1 in claude-docs/ANALYSIS-AUDIT.md. ServerPackHandler.run writes manifest.json, and ServerPackUpdater.isUpdateRun decides whether a run is an update by asking whether that file exists -- so isUpdateRun flips from false to true in the middle of the run, before either createServerRunFiles call. On a first generation with updating enabled and zip creation desired: 1. the zipped-variant call writes variables.txt, because it does not exist yet 2. the local-variant call, which is the only one carrying the user's SPC_JAVA_SPC path, now sees a file that exists, is protected, and belongs to a run isUpdateRun calls an update -- so it preserves this run's own output and never writes the local variant The pack keeps the archive's copy, where SPC_JAVA_SPC was replaced with the literal "java". Reachable in the feature's default configuration: overwrite on, zip on, user ticks Update Server Packs. Measured before writing the guard, same config generated twice into temporary directories and differing only in the toggle, with SPC_JAVA_SPC=/opt/my-very-distinctive-jdk/bin/java -- updating enabled: variables.txt does NOT contain the path; updating disabled: it does. No existing guard covers it: anUpdateKeepsTheOperatorsVariables exercises the second run only, and nothing asserted anything about a first generation while updating was enabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The six items left open as M4 in claude-docs/ANALYSIS-AUDIT.md. None of them guards a defect that is currently present -- every one passes against today's code -- so each was mutation-checked instead, and each failed exactly the guard written for it and no other. - aManifestWrittenOnTheOtherPlatformStillPrunes: a manifest whose entries are `mods\alpha.jar`, as a pack generated on Windows carries, pruned on this platform. protects() was already pinned against backslashes; prune() never was, and it is the half that deletes. Mutation: normalize() stops converting separators -> fails (with protectionIgnoresSeparatorStyleAndCase). - anAbsolutePathInAnOldManifestCannotReachOutsideThePack: File(pack, "/abs/path") resolves under the pack on Unix, and the guard pins that the pack stays the only thing a prune can touch whatever a manifest claims. Mutation: prune resolves entries with File(relative) -> fails (with four others). - protectionStopsAtTheSegmentBoundaryWhenPruning: `world` must protect world/ but not worlds/, world_backup/ or worldly.jar, or a modpack with a directory named after the save becomes unprunable. Mutation: startsWith("$protected/") relaxed to startsWith(protected) -> fails (with protectionCoversAnEntryAndItsContents). - anUnreadableManifestPrunesNothingEither: the permission path, distinct from the unparseable one already covered. Guarded by Assumptions so a filesystem that ignores setReadable(false), or a run as root, skips rather than lies. Mutation: readManifest stops catching -> fails (with anUnreadableManifestPrunesNothing). - aDerivedDestinationIsUpdatedInPlaceJustLikeAGivenOne: every other update guard redirects the pack with customDestination, so the path a normal GUI or CLI run takes -- the destination derived from the server-packs directory and the pack name -- was never exercised with updating on. Also asserts the second run updates the first pack rather than generating a second beside it. - theCallersExclusionAppliesEvenWithTheZipExclusionPreferenceOff: zipBuilder now installs its ExcludeFileFilter unconditionally, because an update must keep a running server's data out of a shareable archive whatever the preference says -- while the configured list must still be ignored when the preference is off, that being what the preference means. Neither half was covered. Mutation: filter installed only when the preference is on -> fails. api 459 tests (was 453), 1910 across the repository, no failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes audit finding M1. Eighteen orphaned doc blocks were fixed on 2026-09-20 with nothing left to stop the nineteenth: Qodana reports one only when it happens to contain a [link] that no longer resolves, which was 4 of the 18, and the scan that found the rest was a throwaway script in a chat log. **This lands red, and the red is real.** It fails on two instances that script missed, because it only looked at blocks spanning several lines and both of these are one-liners superseded by a longer block written next to them: serverpackcreator-api/.../legacyfabric/LegacyFabricInstaller.kt:47 serverpackcreator-grinder/.../GrindLoopTest.kt:185 Run before committing, per the rule that a guard committed red is only evidence when the red is the missing fix rather than the guard's own fault: the failure message lists exactly those two paths, and the scan reached 500+ Kotlin files, which the size assertion above it states independently so a scan that finds nothing cannot pass by vacuity. Lives in -api and reads every module because the convention is repository-wide; a test source set adds no compile dependency, so the module graph is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`release` and `mirror` both assembled the whole release JSON -- changelog section included -- into a single `-d "{...}"` shell word. Linux caps ONE argv entry at MAX_ARG_STRLEN (32 * PAGE_SIZE = 131,072 bytes) regardless of ARG_MAX, so past that size execve returns E2BIG and the shell reports /var/run/act/workflow/create.sh: line 21: /usr/bin/curl: Argument list too long which is exactly how 9.0.0-beta.1 died (run 536, job `Forgejo release`). The downstream JSONDecodeError in that log is the consequence, not the cause: curl never ran, so python read an empty pipe. Measured, since build logic is verified by measurement: 9.0.0-alpha.8 changelog section 74,454 B -d argument ~75,9xx B released 9.0.0-beta.1 changelog section 200,185 B -d argument 201,610 B E2BIG The jump is structural, not bad luck: a beta or final release aggregates every prerelease section since the last stable tag, so 9.0.0 final will be at least as large. Reproduced both shapes against the real notes in alpine:3.20 -- inlined gives exit 126 "Argument list too long", `-d @file` execs curl and reaches the network with a 201,620-byte payload. The PATCH branch four lines below had always written a file and used `-d @patch.json`; only the POST branches inlined. Both now build the payload with python (values via env, so nothing is spliced into shell or JSON) and pass `-d @<file>`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The `release` step's own comment names re-running this workflow as "the ordinary way to repair a release whose `maven` or `docker` job failed", and the reuse branch above was written so a second POST would not die. The asset loop below it was not: it uploaded all twelve unconditionally. Forgejo does not save it. `CreateReleaseAttachment` in routers/api/v1/repo/release_attachment.go passes the name straight to `UploadAttachment` with no existence check -- and nothing else enforces uniqueness either; `Release` truncates only `Title`, at 255 characters. So the re-run does not fail on a duplicate name, which would at least be visible. It attaches a second copy of every asset and reports success. The `mirror` job has guarded its loop with a `have.txt` name list since it was written, for exactly this reason on the GitHub side. The job that publishes the canonical release now does the same, against `GET /releases/{id}/assets` (verified against release 1734: 12 names, HTTP 200). Exercised the loop against that real name list with a mixed asset directory: three existing names skipped, one new name uploaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`.claude/rules/ci-workflows.md` already recorded this as outstanding: "the `release`, `virustotal` and release-body-update steps still use `curl -sf` and have the same blindness waiting for them". `-f` sets exit 22 and `-s` discards the body, which is the only place these APIs say what is wrong -- and the body is what distinguishes a rejected token from a rejected payload. 9.0.0-beta.1 is a fresh example of the cost one layer down: the job reported a python JSONDecodeError traceback, which points at the parser, while the actual line -- `/usr/bin/curl: Argument list too long` -- was a consequence of curl never running at all. Converted to the `-o file -w '%{http_code}'` shape the `mirror` job has used since it was written: the `release` job's create, notes-refresh and asset upload; the VirusTotal release-body read and PATCH; and the Discord post. Each prints `::error::` naming the status and the operation, then the body. Two `-sf` calls remain on purpose, both VirusTotal submissions: their failures are explicitly tolerated (`|| true`, then a guard on the empty id) because a scan that does not come back must not fail a published release. The rule file now says that instead of listing work already done. Every `run:` script in the workflow passes `bash -n` with the `${{ }}` expressions stubbed (16 steps, 0 errors). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The step escaped the version's dots in the shell -- `sed 's/\./\\./g'` -- and handed the result to `awk -v ver=...`. awk processes escape sequences in a `-v` assignment, so `9\.0\.0-beta\.1` arrives inside the program as `9.0.0-beta.1` and the dots have been wildcards the whole time. Every release log says so: awk: warning: escape sequence `\.' treated as plain `.' The comment above the line claimed the dots were escaped, which is the failure mode this repo's conventions single out -- a shell template fails silently, producing a plausible value rather than an error. Nothing broke in practice because the `\]` anchor does the real work, but the guard was not the guard it was documented to be. Escaped inside awk instead, as `[.]` rather than `\.`: POSIX leaves a backslash in a gsub replacement undefined for anything but `&` and `\\`, and gawk bears that out -- a program-text replacement of `"\\\\."` yields `9\\.0\\.0-beta\\.1`, two backslashes, and the regex then matches nothing. A bracket expression has no such ambiguity. Verified under gawk in alpine:3.20 against six real CHANGELOG.md revisions (8.1.0, 8.1.1, 8.1.2, 9.0.0-alpha.8, 9.0.0-alpha.9, 9.0.0-beta.1), running the step's own script as the runner renders it: - byte-identical sections to the old shape in all six (14,396 / 2,844 / 8,111 / 74,454 / 18,747 / 200,185 bytes) - stderr 56 B -> 0 B; the warning is gone - and the escaping now bites: given a heading `## [9X0Y0-betaZ1]`, the old shape collects its body, the new one collects nothing Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`CHANGELOG.md` is semantic-release's and must not be hand-edited, so without this file the only copy of these notes is the release body on two forges. Provenance. Run 536 never created a release, so both were built by hand from CI artifact 712 (897,155,382 B, 12 entries, all 11 SHA-256s verified against its own checksum.txt before upload). This text is the body of Forgejo release 1735 and GitHub release 391643508, which are byte-identical at 18,245 characters. The file differs from them in one way: it omits the `## VirusTotal` section, because those nine permalinks are generated per release by the pipeline's own step and belong on the release page, not in the repository. Written rather than generated. The raw section for this tag is 1,121 entries, of which 297 are docs commits and 277 tests; the audience for a release body is modpack creators, so the prose covers breaking changes, features and fixes and links the full list. Four claims were checked and left out because they are false or unverifiable: - an aarch64 AppImage in the release -- spc.install4j defines exactly three media (154 Windows, 159 Unix, 158 macOS) and the AppImage jobs are in devbuild.yml, confirmed against the artifact's 12 entries - a download-size reduction -- the 274.7 -> 77.8 MB jar shrink happened within the 9.0.0 alphas; against 8.1.2 every download grew (Linux installer 164,164,895 -> 260,126,652 B) - a Java version bump -- JavaLanguageVersion.of(21) at both 8.1.2 and HEAD, and updates.xml bundles 21.0.4 - a downloadable grinder or grinder plugin -- neither is a release asset Numbers that did survive were measured here, not quoted: the shipped fallback clientside list goes 496 -> 548 entries between 8.1.2 and this tag, and 8.1.2's `configured-` line really was missing its trailing comma, welding it to `connectedness-` so that neither mod could ever match. One correction after publishing, caught by Griefed: the intro presented the boot-verification tooling as something 9.0.0 gives you, without saying it is command-line only. The engine does ship -- BOOT-INF/lib/serverpackcreator- clientside-9.0.0-beta.1.jar, with ScanCommand, ClientsideReportCommand, VerifyClientsideCommand and ClientsideApplyCommand -- but there is no GUI surface at all; the only scanner in the gui package is LarsonScanner, the progress widget. The notes now separate the two halves by how they reach a user, and say plainly that the second has no GUI. Both release bodies were patched; this file is the corrected text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`serverpackcreator-api` has never been usable from Maven Central. The `mavenJava` publication declared exactly one thing -- `artifact(tasks.named("javadocJar"))` -- and no `from(components["java"])`, so it had no main artifact and no dependencies. Gradle writes `<packaging>pom</packaging>` for such a publication, which is precisely what is on Central: 7.3.0 8.0.0 8.1.0 8.1.2 9.0.0-alpha.6 9.0.0-alpha.9 9.0.0-beta.1 -> every one ships `-javadoc.jar` + `.pom`, and nothing else An embedder writing `de.griefed.serverpackcreator:serverpackcreator-api` resolved a POM and a javadoc jar and no classes, with no transitive dependencies. The root CLAUDE.md calls this module's public surface "a compatibility constraint -- plugins compile against it"; from Central they could not. The javadoc half compounded it. `withJavadocJar()` registers Gradle's stock `javadocJar`, which zips the stock `javadoc` task -- and this module is pure Kotlin, so `javadoc` documents nothing and that jar is a 261-byte manifest. Worse, `dokka-conventions` registers `dokkaJavadocJar` with the SAME `javadoc` classifier, so both wrote `build/libs/<name>-<version>-javadoc.jar` and whichever ran last won. That is the whole explanation for a contradiction that looked impossible: the release ASSET javadoc jar is 261 bytes (the `assets` job runs `build`, which ran the stock task) while Central's is 2.4 MB (the `maven` job runs `:serverpackcreator-api:dokkaJavadocJar` first). `-app` never applied publishing-conventions, has no stock task, and so has always shipped a 5 MB javadoc jar -- the contrast that located this. So: `from(components["java"])` for the main jar, the sources jar and the dependency list; `withJavadocJar()` removed so the empty task cannot exist or collide; and the Dokka jar attached from `dokka-conventions`, which is the plugin that owns it -- publishing-conventions is applied first, so a `tasks.named("dokkaJavadocJar")` there would resolve a task that does not exist yet. Measured with `publishMavenJavaPublicationToMavenLocal` (signing temporarily neutralised for the probe, reverted before this commit): before -javadoc.jar 261 B / 2 entries, .pom 1,306 B, packaging=pom, 0 <dependency>, no .module after .jar 4,314,699 B / 197 classes (ApiWrapper, ApiProperties, ApiPlugins ...), -sources.jar 4,036,536 B / 848 entries, -javadoc.jar 2,449,329 B / 473 entries, .module 9,176 B, .pom 5,430 B, 20 <dependency> No source API changed, so nothing moves for a plugin that already compiles against a local build. What changes is that resolving the coordinate now yields something that works. Recorded in claude-docs/API-BEHAVIOUR-CHANGES.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`tasks.build { finalizedBy(tasks.dokkaGeneratePublicationJavadoc) }` ran the generator and stopped there: Dokka wrote 208 files into `build/dokka` and nothing ever packaged them. The release's `assets` job takes its javadoc asset from `build/libs` (`cp serverpackcreator-api/build/libs/*.jar`), so the asset was whatever else had written that path -- the stock empty jar, before the previous commit removed it. `-app` has always finalised on `dokkaJavadocJar` rather than the generator, and that is exactly why its javadoc asset is 5 MB while `-api`'s was 261 bytes. This matches it. Measured on `./gradlew -Pversion=9.9.9-repro :serverpackcreator-api:build`, build/libs and build/dokka cleaned first: before -javadoc.jar 261 B / 2 entries, 208 files stranded in build/dokka after -javadoc.jar 2,449,323 B / 473 entries The release `maven` job's explicit `:serverpackcreator-api:dokkaJavadocJar` is now redundant rather than load-bearing, and is left alone: it is harmless, and removing it would couple this fix to a workflow change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>serverpackcreator-api has never been usable from Maven Central. The mavenJava publication declared one artifact -- the javadoc jar -- and no from(components["java"]), so it had no main artifact and no dependencies, and Gradle wrote <packaging>pom</packaging> for it. Verified against Maven Central itself before merging, rather than taken from the branch's notes: repo1.maven.org's directory for 9.0.0-beta.1 holds exactly serverpackcreator-api-9.0.0-beta.1.pom and -javadoc.jar, and that POM carries <packaging>pom</packaging> and zero <dependency> entries. An embedder resolving the coordinate the API compatibility policy exists to protect got a POM and a javadoc jar and no classes. The second commit fixes the other half: -api finalised build on the Dokka *generator*, which wrote 208 files into build/dokka and never packaged them, so the release assets job -- which copies from build/libs -- shipped whatever else had written that path. That was Gradle's stock javadocJar, an empty 261-byte manifest colliding with Dokka's on the same classifier. withJavadocJar() is gone so the collision cannot recur, and both landmines are written down where the next person will hit them. The branch's measurements were re-run on this tree rather than trusted, since it was cut on 2026-09-13 and -api has grown since: POM 5,430 B, 20 <dependency>, no <packaging> -- identical to the branch's figure main jar 4,337,778 B / 201 classes (branch: 4,314,699 / 197), carrying ApiWrapper, ApiProperties and ApiPlugins sources jar 4,051,432 B / 849 entries (branch: 4,036,536 / 848) javadoc jar 2,473,615 B / 477 entries (branch: 2,449,329 / 473) -- and not the 261-byte stub Signing blocks publishMavenJavaPublicationToMavenLocal without a key and cannot simply be excluded, as the .asc artifacts stay declared; generatePomFileForMavenJavaPublication plus the three jar tasks answer the same question without one. Conflict in API-BEHAVIOUR-CHANGES.md was both sides appending a row to the same table; both kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Eight commits fixing what killed 9.0.0-beta.1 (run 536, job `Forgejo release`): the release JSON, changelog section included, was assembled into a single `-d "{...}"` shell word, and Linux caps one argv entry at MAX_ARG_STRLEN = 131,072 bytes whatever ARG_MAX says. The section had grown from 74,454 B at 9.0.0-alpha.8 to 200,185 B, because a beta or final aggregates every prerelease since the last stable tag -- so 9.0.0 final will be at least as large, and this was not bad luck. With the body out of argv the mirror job hits GitHub's own 125,000-character cap, so the copy bound for GitHub is truncated on a line boundary with a pointer to the Forgejo release that keeps every character. Re-running the workflow to repair a failed release no longer attaches a second copy of all twelve assets, because Forgejo's CreateReleaseAttachment enforces no uniqueness and reports success. And `curl -sf` is gone from the steps that must explain themselves: the JSONDecodeError traceback in run 536 pointed at a parser, while the cause was a line above it saying curl could not be exec'd. Verified on the merged result rather than on the branch, since develop's AppImage work landed in this same file after the branch was cut: merge clean; `Build AppImages` and the ServerPackCreator-* globs both survive YAML parses; 8 jobs bash -n 17 run: scripts with the ${{ }} expressions stubbed, 0 errors awk extraction re-run under gawk in alpine:3.20 against this repository's CHANGELOG. For 9.0.0-alpha.6 the old and new forms produce byte-identical output (40,347 chars), but the old writes `gawk: warning: escape sequence \. treated as plain .` to stderr and the new writes nothing -- which is the finding: the dots were wildcards all along and only the ] anchor was doing the work. Also keeps the hand-written 9.0.0-beta.1 notes in the repository, since CHANGELOG.md is semantic-release's and must not be hand-edited, and records both limits in the CI rules file so the next person to touch the release job reads them before 9.0.0 final crosses the same thresholds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes B39. The scan ran against a raw checkout, so i18n4k's generated `Translations` and `Example` objects did not exist during analysis. 85 source files reference one of them, 79 of those in -app, and a file with an unresolved core symbol is analysed with its inspections DEGRADED -- so the low finding count for -app was not evidence that -app was clean, it was evidence that the scanner could not read it. Measured, per the build-logic rule in the root CLAUDE.md: sanity failures, run 817: 35 (30 Translations + 5 Example) sanity failures, run 832: 35 (30 Translations + 5 Example) -app 24 / -api 6 / -plugin-example 5 sanity failures, after: to be read off the next run; the target is 0 codegen cost, clean checkout, --rerun-tasks --no-build-cache: 23.0 s codegen cost, clean checkout, build cache warm: 16.4 s Run 832 scanned revisiond97a6e188-- this branch's base -- so the 35 is a measurement of exactly the tree this step is being added to, not of an older one. If the next run does not move it, revert this rather than keep a slower job, which is what B39 asked for. `generateI18n4kFiles` resolves in -api and -plugin-example and `kaptKotlin` in both plugin modules: exactly the three modules the failures came from. Not `build` -- the scan needs the generated sources, not the artifacts, and a full build would cost the frontend, the license report and the suites for nothing. kaptKotlin does drag in :serverpackcreator-api:compileKotlin, which is a feature rather than a cost: -app is then analysed against a compiled -api. The scan step's landmine said this could not be seen; it now says the opposite and names what breaks if the step is removed or reordered. The count step reports the sanity failures beside the problem count, because the problem count alone cannot tell a clean scan from a broken one -- degradation LOWERS it, so a broken scan reads as a clean report. An absent sanity.json is reported as unknown rather than as zero, since those are opposite states. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>B38, first half. Twelve of run 832's 28 problems are decisions already taken and re-taken: they were triaged from run 817, and every one of them came back unchanged. Until now those verdicts lived only in a chat log and in a backlog table, so the tool kept reporting them and every reader re-litigated them. UnusedSymbol 2 false positive -- @Component beans injected by interface; QDJVM Community has no Spring plugin and cannot see the wiring. WebServiceContextTest asserts both resolve from the context. UnstableApiUsage 4 won't fix -- FAIL_ON_PROJECT_REPOS is @Incubating with no stable alternative. ConvertLongToDuration 1 MUST NOT FIX -- changes an exported parameter type on a Maven Central artifact, against the API compatibility policy. RedundantIf 5 won't fix -- five explained early-return guards. Each entry is scoped to the exact files it was decided for, never to a whole rule. An over-broad exclusion silences a rule everywhere and is worse than the noise it removes, so a new occurrence of any of these four rules in any other file still reports. Verified before committing, since a name or path that matches nothing fails silently and looks identical to a working exclusion: all nine paths exist in the tree, and replaying the nine (ruleId, uri) pairs against run 832's SARIF covers exactly 12 of the 28 findings and leaves exactly the 16 style notes. The confirmation on the next run is therefore the count going 28 -> 16 precisely, not merely going down. The baseline, B38's other half, deliberately does NOT land here. It would suppress all 28 at once and so destroy the only evidence that these twelve exclusions work; and a baseline taken now would be built from a scan that could not fully read -app, which the preceding commit is what fixes. It is worth taking against the first run that is both trustworthy and confirmed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Committed RED, and run before committing so the red is the missing implementation rather than the guard's own fault: manyFetchersCollapseToOneClient FAILED 25 default-constructed fetchers produced 25 distinct HttpClients ==> expected: <1> but was: <25> defaultFetchersShareOneClient FAILED ==> expected: <HttpClientImpl@4c29f8aa(27)> but was: <HttpClientImpl@122b07f1(28)> anExplicitlySuppliedClientIsStillHonoured PASSED Note the `(27)`/`(28)` in that failure: it is the JDK's own per-client counter, the same one that names the `HttpClient-N-SelectorManager` threads. The third test passes already and must keep passing -- the fix must not swallow a caller-supplied client, which every test faking the transport relies on. WHY THIS IS A DEFECT AND NOT A STYLE NOTE. The JDK documents the alternative as the anti-pattern in as many words: "Creating a new client for each operation, though possible, will usually prevent reusing such connections." Each client carries its own connection pool and its own SelectorManager thread, so a fetcher per candidate re-does the TLS handshake to Modrinth/CurseForge that the previous candidate had already paid for. MEASURED, on the live daemon 2026-09-21: 384 clients created in 2.9 hours, 28 still alive in the thread dump. The path is ContainerCandidateVerifier.verifyStaged -> supportedPlatforms() per candidate -> a ModrinthPlatform and a CurseForgePlatform -> a default JdkHttpFetcher each. Sharing is safe, checked rather than assumed: the CurseForge key travels as an `x-api-key` REQUEST header through HttpFetcher.get(url, headers), never as client state, and HttpClient is documented immutable and thread-safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the previous commit's pin green. `JdkHttpFetcher`'s default client is now a single `SHARED` instance rather than a fresh build per construction. before: 25 default-constructed fetchers -> 25 distinct HttpClients after: 25 default-constructed fetchers -> 1 WHAT IT COST IN PRODUCTION. `ContainerCandidateVerifier.verifyStaged` calls `supportedPlatforms()` per candidate; each call builds a ModrinthPlatform and a CurseForgePlatform, each default-constructing a fetcher. On the live daemon on 2026-09-21 that was 384 clients in 2.9 hours, 28 still alive in the thread dump. Each one is a private connection pool -- so every candidate re-paid the TLS handshake to Modrinth and CurseForge that the previous candidate had already done -- plus a SelectorManager thread that lives until the client is collected. The JDK names this exact shape as the anti-pattern: "Creating a new client for each operation, though possible, will usually prevent reusing such connections." WHY SHARING IS SAFE, verified rather than assumed: - HttpClient is documented immutable and thread-safe. - Neither platform puts credentials on the client. The CurseForge key travels as an `x-api-key` REQUEST header through `HttpFetcher.get(url, headers)`, so two platforms sharing a client cannot leak one's key into the other's calls. - A caller-supplied client still wins -- `anExplicitlySuppliedClientIsStillHonoured` was green before this change and stays green, which is what keeps every test that fakes the transport working. `SHARED` is deliberately never closed: it is process-wide with no lifecycle shorter than the JVM's, closing it would race whatever is mid-request, and there is now exactly one of it rather than hundreds. Suites: clientside 671 (668 + this commit's 3), grinder 537 (29 skipped), app 168 -- 0 failures across all three, no new compiler warnings. NOT claimed as the cause of the report outage. That is still unproven; this is a defect found while looking, fixed on its own evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Run before committing. THREE fail, four pass, and which is which matters: RED statusReportsTheCountWithoutCopyingTheStore /status copied the entire store to read a count ==> expected <0> was <1> RED asPropertiesReadsThroughTheSnapshotCache 3 scans for 3 requests against an unchanged store ==> expected <true> was <false> RED theDefaultWindowCoalescesRatherThanRebuildingPerVerdict a default-constructed cache rebuilt on the very next verdict GREEN aVerdictRecordedInsideTheWindowDoesNotForceARebuild GREEN theWindowExpiringLetsTheNextRequestSeeTheNewData GREEN anUnchangedStoreIsNeverRebuiltHoweverLongPasses GREEN aZeroWindowRebuildsOnEveryChangeAsBefore THE FOUR GREEN ONES ARE NOT RED PINS AND ARE NOT CLAIMED AS SUCH. The preceding seam commit carried the window mechanism, inert behind a zero default, so they characterize what that commit added rather than demanding anything new. The only behaviour left to change is the DEFAULT, which is what the third red pin is for. Saying so here because a green test in a `test(...)` commit otherwise reads as a pin that was never checked. WHAT THE THREE RED ONES ARE ABOUT. All three are costs that grow with a long-running grind while the page stays 250 rows, and the store has passed the high six figures: - `/status` read `store.all().size` -- a full list copy to look at one integer, on the endpoint the dashboard polls on a timer. - `/as-properties` called `store.all()` directly, the one endpoint the snapshot cache never covered, and the one polled unattended by every SPC instance in the wild. - the cache rebuilt whenever `VerdictStore.version` moved, and `record()` moves it on every verdict -- so with several workers recording continuously it was very nearly a no-op exactly when the daemon is busiest. They assert a COUNT OF SCANS, never a duration: a timing assertion is flaky on CI and says nothing about why it got slow, whereas "this endpoint copied every verdict" is the defect itself and is exactly reproducible. `CountingStore` delegates to a real `InMemoryVerdictStore` so what is observed is the production store's traffic rather than a reimplementation of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Turns the three red pins green. Three separate costs, all of which grow with the store while the page stays 250 rows, and the store has passed the high six figures: 1. /status read `store.all().size` -- a full list copy to look at one integer, on the endpoint the dashboard polls on a timer. `VerdictStore` gains `count`, defaulted to `all().size` so no implementation is forced to care, overridden by both real stores to read the backing map directly. 2. /as-properties called `store.all()` directly. It was the ONLY endpoint the snapshot cache never covered, and it is the one polled unattended by every SPC instance in the wild -- a whole-store copy per poll by the endpoint least able to afford one. It now reads `snapshots.current().verdicts`, the same rows the table serves. 3. The cache rebuilt whenever `VerdictStore.version` moved, and `record()` moves it on EVERY verdict. With several workers recording continuously there is a new version between almost any two requests, so during a grind the cache was very nearly a no-op -- it stopped working exactly when the daemon is busiest. `VerdictSnapshotCache` now coalesces for `SPC_GRINDER_REPORT_CACHE_SECONDS` (default 5). BEHAVIOUR CHANGE, hence `perf(` and not `refactor(`. The report may now lag by up to one window. That is the same bargain SPC_GRINDER_STORE_FLUSH_SECONDS already makes for writes and for the same reason: whole-store work belongs on a clock, not on an event that fires thousands of times an hour. `0` restores the old strictly-live behaviour exactly, and is pinned as such. TWO TEST EDITS, both disclosed rather than buried: - `theSnapshotIsRebuiltWhenAVerdictIsRecorded` now passes `Duration.ZERO`. Every assertion is byte-identical; it pins what it always pinned -- that a changed store invalidates the derivation -- and now names the window it assumes. The default-window behaviour is pinned separately. - `CountingStore` had to override `count` to delegate. `VerdictStore.count` DEFAULTS to `all().size`, so a double that inherited it counted a scan per count and reported the defect regardless of what production did. The double was incomplete; the assertion did not move. The knob is declared in all three places a knob has to be or the documentation guards cannot see it: `GrinderConfiguration.KNOBS`, the README table, and the systemd unit. The reader call is on one line, like httpThreads, because `everyVariableReadIsDeclaredAsAKnob` matches `reader("NAME"` and a wrapped call is invisible to it. Suite: 544 tests (537 + 7), 0 failures, 29 skipped. No new compiler warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Two defects found while chasing the outage, fixed on their own evidence. Neither is claimed as the outage's cause -- that was the host firewall, merged just before this. ONE HTTP CLIENT. `JdkHttpFetcher`'s default parameter built a fresh `HttpClient` per instance, and `ContainerCandidateVerifier.verifyStaged` calls `supportedPlatforms()` once PER CANDIDATE, which constructs a ModrinthPlatform and a CurseForgePlatform that each default-construct a fetcher. Two clients per candidate, each a private connection pool and a SelectorManager thread, none ever closed. Measured on the live daemon 2026-09-21: 384 clients in 2.9 hours, 28 still alive in the thread dump. The JDK names the shape as the anti-pattern in as many words -- "Creating a new client for each operation, though possible, will usually prevent reusing such connections" -- so every candidate re-paid the TLS handshake the previous one had already done. Red pin: 25 fetchers -> 25 distinct clients; after: 1. Sharing is safe for a checkable reason rather than by assumption -- the CurseForge key is an `x-api-key` REQUEST header through `HttpFetcher.get(url, headers)`, never client state, and a caller-supplied client still wins, which is what every test faking the transport depends on. A REPORT THAT STOPS WALKING THE WHOLE STORE. Three separate costs, all growing with the store while the page stays 250 rows, and the store has passed the high six figures: - `/status` read `store.all().size` -- a full list copy to read one integer, on the document the dashboard polls on a timer. `VerdictStore.count` now answers from the backing map. - `/as-properties` called `store.all()` directly. It was the ONLY endpoint the snapshot cache never covered, and it is the one polled unattended by every SPC instance in the wild. - The cache rebuilt whenever `VerdictStore.version` moved, and `record()` moves it on EVERY verdict -- so with several workers there is a new version between almost any two requests and the cache degraded to nearly nothing exactly when the daemon is busiest. That is the part that answers "it worked a week ago": the cache never scaled with an ACTIVE grind, which did not matter at 38k rows. BEHAVIOUR CHANGE, hence `perf(` rather than `refactor(`: the report may now lag by up to `SPC_GRINDER_REPORT_CACHE_SECONDS` (default 5). Same bargain SPC_GRINDER_STORE_FLUSH_SECONDS already makes for writes. `0` restores the old strictly-live behaviour exactly and is pinned as such. Two test edits are disclosed in their commits rather than buried: `theSnapshotIsRebuiltWhenAVerdictIsRecorded` now passes `Duration.ZERO` with every assertion byte-identical, and `CountingStore` had to delegate `count`, because `VerdictStore.count` DEFAULTS to `all().size` and an inheriting double reports the defect whatever production does. Four of the seven new tests were green when committed and the commit says so -- the seam carried the window mechanism inert behind a zero default, so only three were real red pins. Verified on the merged result, not on the branch, since the outage docs landed in the same README section after this branch was cut: merge one conflict, in the README's "Report responsiveness" prose; both edits kept, the host-first triage box ahead of the cost table clientside 671 tests, 0 failures, 0 skipped grinder 544 tests, 0 failures, 29 skipped app 168 tests, 0 failures, 0 skipped plugin-grinder green documentation SystemdUnitConfigurationTest + ReadmeConfigurationTest green, which is what keeps the new knob declared in all three places Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # serverpackcreator-grinder/README.mdOf the sixteen notes the scan reported, these five are cases where the tool was right and the change costs nothing: ModListCompiler TreeSet<File>(...) -> TreeSet(...) x2 GenerationConfig "${updateProtectedPaths}" -> "$updateProtectedPaths" DockerJavaContainerEngine "${OWNER_LABEL}" -> "$OWNER_LABEL" SelectionAttribution when { entry in X -> } -> when (entry) { in X -> } Behaviour-preserving, hence `refactor:`. No test assertion, argument or expected value changed anywhere; the string templates render identical bytes, the TreeSet element type is inferred to the same File, and the `when` subject form is the same five branches in the same order with the same comments between them. The other eleven were read at their call sites and refused rather than applied -- they are excluded in the following commit, each with the reason. Not applying a lint is a decision, so it is recorded where the tool will look rather than in a commit message nobody re-reads. Suites, all re-run: api 460 (1 skip), clientside 671, grinder 544 (29 skip), app 168, plugin-grinder 75. 0 failures, no new compiler warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closes B40, and not by the means B40 proposed. Its goal was "a run reports what is NEW rather than everything"; reaching **zero** achieves that better than a baseline, because then anything reported is new by definition and there is no file to maintain. WHY NOT THE BASELINE. Measured on run 785's SARIF: the file is 3.9 MB, of which 2.78 MB is JetBrains' inspection catalog and 22 KB the actual findings. That is 99.4% vendor metadata committed to the repository to suppress sixteen style notes, re-churned on every re-baseline. A hand-trimmed SARIF might work -- the results do carry partialFingerprints -- but it could not be verified here (Docker Desktop is still capped at 1.9 GiB and the linter is OOM-killed), and an unverifiable config change is exactly the trap B38 existed to avoid. Each of these was read at its call site. The five the tool was right about are fixed in the preceding commit; these eleven are refused, scoped to the exact file each was decided for: UsePropertyAccessSyntax 1 FALSE POSITIVE, and the code already said so -- g2d.renderingHints is read-only in Kotlin, so the suggestion DOES NOT COMPILE. Someone tried before. RemoveExplicitTypeArguments 2 both pin a type inference would narrow. The two that pinned nothing were fixed instead. RedundantInterpolationPrefix 1 escaping-heavy regex, deduplicated because two copies had drifted and lost a migration. UnnecessaryVariable 1 `annotating` is read 11x and means what `fired` no longer does -- the rule stopped deciding. ConvertToStringTemplate 1 key construction; its sibling verdictKey spells the marker the same way. Change both or neither. ConvertCallChainIntoSequence 2 an unmeasured perf change over tens of elements, where Sequence is a pessimisation. B30 is the receipt. DestructuringDeclaration 2 binds a domain object's fields POSITIONALLY, so re-ordering the data class would silently rebind rather than fail to compile. JoinDeclarationAndAssignment 1 one of three metas assigned in an ordered init, in -api's central published type. Verified before committing, the same way B38 was: all 19 inspection-scoped paths exist, and replaying every (ruleId, uri) against run 785's SARIF accounts for all 16 findings -- 11 excluded, 5 fixed in code, 0 unaccounted. Projected next run: 0 problems, 0 sanity failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>