Bitti Bitti ein Mergi #678
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!678
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?
A minor bump and the largest of this run, and it is on the MARKER route -- buildSrc/build.gradle.kts puts its jar on buildSrc's own compile classpath -- so it carries the Kotlin-metadata landmine that .claude/rules/build-layout.md records. Checked first and cheaply: :buildSrc:compilePluginsBlocks SUCCESS in 14s, `./gradlew help` SUCCESS in 6s. THE PART THAT NEEDS A HUMAN'S EYE, and the reason the regenerated report is in this commit rather than a later one: 3.1.4 relabels two licences in the shipped LICENSE-AGREEMENT. Same 43 entries, same 6014 lines, only the labels move: Bouncy Castle: "Bouncy Castle Licence" -> "MIT License" bouncycastle.org/licence.html -> opensource.org/licenses/MIT LGPL-3.0: "GNU Lesser General Public License v3.0" -> "GNU LESSER GENERAL PUBLIC LICENSE, Version 3" and the URL drops its .txt suffix The LGPL change is cosmetic. The Bouncy Castle one is a factual claim in a document that ships to users: Bouncy Castle describes its own licence as an adaptation of the MIT X11 licence, so the new label is defensible rather than wrong -- but it is a legal notice, and whether SPC should assert it is Griefed's call, not this plugin's. Reverting just this bump restores the old wording. `./gradlew clean build`: SUCCESS in 1m52s, 2003 JVM tests, 0 failures, 30 deliberate skips, frontend 37. Both license copies stay byte-identical to each other, which `copyLicenseReport` requires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Refreshed with `:serverpackcreator-api:updateManifests`, which is the task that advances the snapshot: the api suite fetches into this module's test home and updateManifests copies that home into the shipped resources. A plain `build` does not do it -- verified, the task ran zero times across several full builds while the test home sat six Minecraft versions ahead of what shipped. minecraft latest.release 26.2 -> 26.3 latest.snapshot 26.3-pre-2 -> 26.4-snapshot-1 +6 ids: 26.3, 26.3-pre-3, 26.3-rc-1..3, 26.4-snapshot-1 fabric +6 intermediaries, matching those ids forge +3 entries on the 26.3 line, +1 on 1.21.5 neoforge +35 versions, through 26.3.0.9-beta mcserver +26.3.json, the per-version server manifest for the new release -- the file that keeps the api suite offline for it That last one is why the pair matters: 26.3.json was already sitting untracked from an earlier fetch while the parent manifest still advertised 26.2, so the snapshot had the server manifest for a release it did not admit existed. Both halves land together here. ShippedManifestSnapshotTest passes on a --rerun-tasks run, so it genuinely executed rather than reporting UP-TO-DATE: every advertised release still has a shipped server manifest. No `.etag` sidecars and no `.DS_Store` came along -- updateManifests excludes the former, and the latter was untouched. Checked, because shipping one machine's HTTP bookkeeping reaches every user's home through ApiWrapper.setup(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Both checks were effectively absent. The fish one skipped itself whenever `fish` was missing, which is this machine and the CI runner; PowerShell had no source-level check at all, only a copy inside ScriptTemplateMatrixIT behind GRINDER_TEMPLATE_IT=1, which no workflow in .forgejo/ or .github/ sets. So the guards for the two shells this repository has twice shipped silent bugs in were the guards that never ran. ShellTemplateSyntaxTest runs the real interpreter: local first, a container second, and a skip -- never a pass -- when neither is reachable. A setup marker separates "the container never got its interpreter" from "the template is broken", since both otherwise end non-zero, and a timeout is reported rather than folded into the exit code for the same reason. Containers rather than an installed interpreter because PowerShell is not in Ubuntu's default repositories; an image is one line and pins which interpreter answered. Its platform is named explicitly: no linux/arm64 tag is published, so an Apple-Silicon daemon picks arm/v7 and qemu dies with "uncaught target signal 11" and then hangs -- measured, that cost a 5-minute stall before the pin. Teeth checked by mutation, not assumed. With an unterminated `if` appended to default_template.fish and an unclosed block to default_template.ps1, both fail with the interpreter's own message: container alpine:latest rejected a shipped fish template: /templates/default_template.fish (line 745): Missing end to balance this if statement container mcr.microsoft.com/powershell:latest rejected a shipped PowerShell template: Missing closing '}' in statement block or type definition. Reverted, both green: 2 tests, 4.2s with the images present. The old fish test and ScriptTemplateMatrixIT.powerShellTemplatesParse are removed rather than left alongside -- they asked this exact question of these exact files, and a duplicate that never runs is the copy that drifts. What stays in the matrix IT is what needs more than a parser: booting a cell, and executing RunInstallerJavaCommand for the JAVA_INSTALLER fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The PowerShell parse check is flaky on an aarch64 host and I only caught it because a verification run went red and the next one green against identical code. Docker Desktop runs the amd64 image under Rosetta, and Rosetta mistranslates .NET's dynamic call sites: the process emits every correct line and then dies at teardown with assertion failed [block != nullptr]: BasicBlock requested for unrecognized address (BuilderBase.h:561 block_for_offset) -> exit 133 A trivial `Write-Output` exits 0 every time, so it is the heavier codegen that breaks rather than the image. An exit code therefore cannot decide this check on a developer machine, though it would on CI's native amd64 -- which is the worse failure mode: green where it is watched, red where it is worked on. Success is now proven positively. A script prints SETUP_MARKER once its interpreter is usable, `FAIL <name>` per rejected template, and DONE_MARKER as its last act. Missing markers mean the environment could not answer and skip; FAIL lines mean the template is wrong and fail. A parse error does not abort the loop, so a genuine rejection still reaches its DONE_MARKER and is never mistaken for a crash. The shared plumbing moves to TemplateInterpreterRunner in the same commit rather than a separate one, because that marker contract *is* the runner's contract -- extracting it around the exit-code rule and then replacing the rule would be two commits describing one decision, and the first would be a `refactor:` that knowingly published a broken contract. fish keeps the same shape for one definition of success on both paths; the local path synthesises the FAIL line from its exit code, which is trustworthy when nothing is emulated. Both checks green after the change; the fish one is unaffected either way (alpine runs native arm64). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Promoted out of ScriptTemplateMatrixIT, where it sat behind GRINDER_TEMPLATE_IT=1 and the built spc-grinder-templates image and so never ran. It needs neither: only pwsh and the shipped template, both of which the stock image supplies. Its remaining sibling keeps the gate because it downloads a Minecraft server per cell. What it guards is not reachable from a parse check. The Quilt installer needs Java 17+ even when the server runs on Java 8, so installers go through a JAVA_INSTALLER override that falls back to JAVA -- and the fallback is the branch every existing pack takes, because nothing writes JAVA_INSTALLER into a hand-made pack. A quoting slip there breaks installs for everyone while the syntax check stays green. The template is never run; it shells out to Windows CMD. The function is lifted from the parsed AST, defined alone, and CMD is stubbed to record its argument. The AST walk uses a Where-Object pipeline rather than Ast.FindAll. FindAll takes a ScriptBlock as a .NET delegate, and under Rosetta that call site dies with System.NullReferenceException at CallSite.Target before printing anything -- narrowed by bisecting a probe: parsing succeeded, the predicate call did not. The pipeline reaches the same node and survives. Teeth checked in both directions by mutating the template, not assumed: $InstallerJava = $Java -> "a set JAVA_INSTALLER must be used for the installer" $InstallerJava = $JavaInstaller -> "an unset JAVA_INSTALLER must fall back to the server's Java" Reverted, green in 8s. The two fixture paths deliberately share no substring, so neither assertion can be satisfied by the other's value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>runLocally looped over a staged directory while runLocalCommand ran a single invocation — the same "is it on the PATH, run it, synthesise a FAIL line, report a timeout" logic written twice because the callers differed only in how many commands they had. It now takes a label-to-command map, so one command and one per template are the same call. The labels were previously derived from the staged filenames, which is why the probe needed its own method at all: its one invocation inspects a directory rather than a file. Naming them at the call site removes that asymmetry and makes the synthesised failure line say what a reader would look for. Behaviour-preserving, and verified rather than assumed — the changed path is the *local* one, which executes nowhere on a machine without fish or pwsh, so the three green tests prove nothing about it. Driven directly with `sh` instead: RANWITH=local sh SETUP=true COMPLETED=true FAILURES=[FAIL rejected.fish (exit 3)] both commands ran, only the failing one is named runLocally("definitely-not-installed-xyz", …) -> null Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Found by scanning every @Test in the repository for one with no assertion reachable through its same-file helpers: 1983 tests, six candidates, three real after triage. **suggestInclusionsTest was the live defect.** It called `dirs.any { … }` six times and discarded every result -- expression statements that read exactly like assertions to a skimming eye. It passed against any return value at all. Demonstrated rather than asserted: with suggestInclusions mutated to `return ArrayList()`, the old body stayed green and the new one fails with 'config' must be suggested, got [] ==> expected: <true> but was: <false> Its last line was also the wrong question -- `any { it.source != "server_pack" }` is true the moment a second directory exists, where what matters is that the generated output is never suggested as an input. That is now `assertFalse(suggested.contains("server_pack"))`. **chunkyTest** called both chunked printers and asserted nothing, so chunk size, separator, prefix and index range were unguarded; it could only fail by throwing. The chunking is now asserted on the backend both printers share, including the trailing partial chunk, the no-indexes form and the empty list. Mutating the range to 0-based is caught: expected: <[> (1 to 3) a, b, c, …]> but was: <[> (0 to 3) a, b, c, …]> **printConfigModelTest** logs and returns nothing, so there is no value to check. Wrapped in assertDoesNotThrow, which is a weak guard but an honest one -- it now states that a fully populated model must be printable, instead of leaving it implicit. Two candidates were scanner false positives and left alone: encapsulateListElementsTest and aCorruptFileDegradesToEmpty both assert, and the scan mis-parsed their bodies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>StoreWriteBenchTest held three @Test methods that printed timings and asserted nothing, so they could not fail on the thing they exist to report. The fix is not to add timing assertions: this project pins I/O by request, read and open counts and never by wall-clock, and the behaviour these measurements motivated is already pinned properly by CoalescedVerdictWritesTest -- no write per record(), flush and close write what is buffered, the scheduled flusher fires, write-through still persists. A timing assertion here would duplicate that guard with a flakier one, and this repository's rule for a duplicate is to not create it. So it is named for what it is. The class is StoreWriteBenchmark, the methods say `measure…`, and the doc states outright that it asserts nothing about its numbers and why, pointing at the guard that does. What it now does assert is its own fixture -- the two `check()` calls become real assertions with messages, and the third method, which had no guard at all, gets one. A benchmark measuring the wrong thing is worse than none, so a run that mis-seeds fails loudly instead of reporting a confident number about nothing. Still gated behind SPC_GRINDER_BENCH=1. Verified with the gate on, all three green: [bench] 1000 rows -> 3.1 ms per record(), file 0.6 MiB [bench] 100000 rows -> 246.7 ms per record(), file 63.7 MiB Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Two lessons, both from scanning every @Test in the repo for one with no assertion reachable through its same-file helpers -- 2094 functions across develop and the servertest branch, plus 34 frontend specs, three real cases. The first is the Kotlin trap that hid the live defect: an expression statement reads exactly like an assertion. `dirs.any { … }` with its result dropped is valid, warning-free, and passes against any return value; the entry names the mutation that proves it and the `any { it != x }` / `none { it == x }` confusion in the same test. The second is that a benchmark which cannot be red is not a test, with the reason the fix was to rename rather than to assert the timings. api count 485 -> 488 from the three chunking guards added; full build green at 2011 tests, 18 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>A repository-wide audit scanned 2094 @Test functions and found three that asserted nothing. The worst called `dirs.any { … }` six times and discarded every result, so it passed against an empty list. All three are fixed; this exists so a fourth cannot arrive unnoticed, and it uses the analyser already running on every push rather than a custom scanner that would have to parse Kotlin correctly across 130 files with expression-bodied helpers. Runs 817 and 832 reported none of the three, which is the evidence that the rule is not part of qodana.recommended. That is what this entry tests -- it is a hypothesis, not an established fact, and the comment says so. Read like the exclude block it sits above: a wrong rule name matches nothing and reports nothing, so a clean run is not confirmation. Confirmation is the problem count CHANGING. The comment enumerates the three outcomes and what each implies, including the one where the rule turns out to be Java-only via UAST and blind to Kotlin -- the same shape as KDocUnresolvedReference naming 4 of 18 instances. No failThreshold paired with it: a rule whose false-positive rate here is unmeasured must not be able to fail a build on its first outing. Expect noise from mockk `verify` (16 files), MockMvc `andExpect` (7 files) and six custom assert* helpers, all named in the comment so the next reader does not mistake them for findings. Not run locally -- the Qodana image needs more memory than this machine's Docker has. Verified only as valid YAML parsing to the intended shape; the real check is the next CI run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>StoreWriteBenchmark measures; it cannot fail on what it measures, because this project pins I/O by request, read and open counts and never by wall-clock, and CoalescedVerdictWritesTest already pins the behaviour the numbers motivated. Living in src/test it was still collected by every build and reported as three skipped tests -- three phantom entries in a count people read to decide whether coverage gaps exist. It now lives in a `benchmark` source set, so `build` never sees it. Not being in `test` is the gate, so the three SPC_GRINDER_BENCH assumptions are gone. Keeping them would reproduce the phantom-skip confusion inside the new task, which is the thing this move removes. Landed as one commit rather than two: wiring the task without the sources leaves a Test task that fails when invoked, since Gradle 9 fails a Test task that discovers nothing. This is the first custom source set in the repository, and it is declared in the module's own build script rather than a convention plugin -- a real script can use the version catalog and reaches no other module. Three things did not come for free, each commented where it lives: - java-conventions configures `tasks.test` BY NAME, not withType<Test>().configureEach, so the new task inherits no useJUnitPlatform(), no agent jvmArgs, no test-home or Preferences isolation. Replicated locally. Broadening the convention would be tidier and would reach all seven modules plus -app's own re-declaration, so it stays a separate decision. - Kover instruments every Kotlin compilation, so the benchmark would have counted as production code and drifted the coverage number. Excluded via excludedSourceSets. - testLogging is likewise only on `tasks.test`. Found by running it: 3 tests, 0 skipped, 10.2s and *zero console output* -- the measurements were captured into the XML and never shown, which for a benchmark is the same as producing nothing. Now shown. Measured before and after, since build logic is verified by measurement here rather than by a test: grinder test task 542 tests / 18 skipped -> 539 tests / 15 skipped full build 2011 tests / 18 skipped -> 2008 tests / 15 skipped, SUCCESSFUL ./gradlew :serverpackcreator-grinder:benchmark -> 3 run, 0 skipped, no env var needed koverHtmlReport no StoreWriteBenchmark in the report dokkaGenerateJavadoc no StoreWriteBenchmark, no new undocumented warnings The numbers it now prints make B35's case plainly, at 100000 rows: write-through 299.8 ms per record() -> coalesced ~1 us per record() Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>testf7e1d0fc47cleanup()and a configuration-cache claim it was part of f7b6d6c8e1`cleanup()` became the compiled `TestHome.prepare(File)` in buildSrc, and four places still named it: serverpackcreator-api/CLAUDE.md and its build.gradle.kts, the root CLAUDE.md, and the grinder's. Three described current behaviour and were simply wrong; the grinder's is historical, so it now names both so a grep for either lands. The configuration-cache paragraph in the build-layout rule was stale in four ways at once, and only measuring showed how far: ./gradlew build --configuration-cache --dry-run claimed: 20 problems, 13 unique, ours = processTestResources' filter{} and test's doFirst{cleanup()}, plus -app's test.doFirst capturing projectDir measured: 8 problems, 2 unique, ALL of them :generateLicenseReport holding a Project ./gradlew :serverpackcreator-app:test --configuration-cache measured: "Configuration cache entry stored." — no problems at all So none of the problems are ours any more. TestHome.prepare closes over nothing but the File handed to it, and -app's duplicate doFirst is gone. The rule's conclusion inverted with it: the ceiling was "fewer problems, not zero" and is now exactly generateLicenseReport, so excluding that one task is the difference between 8 and zero. The paragraph says to re-run the command before believing it, because this is precisely the fact that goes stale the moment a task action captures a script again. Also: the rule stated Kotlin 2.4.10 as the catalog's current entry, which reads 2.4.20. The 2.4.10 in the POM measurement further down is left as the value that was measured, now labelled as such rather than read as today's. Full build green afterwards: 2008 tests, 15 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>checkThe previous commit moved StoreWriteBenchmark into its own source set, left it a JUnit Test task, and deliberately did not wire it into `check` -- on the assumption that an unwired Test task is not collected. It is. `./gradlew build` ran all three measurements, which is precisely what the move existed to prevent, and the pre-push build is where that showed up. **In this build every task of type `Test` is pulled into `check`** -- by type, not by name, not by group, and with nothing in any build file declaring the dependency. Probed three ways before accepting it: renaming the task changed nothing, moving it out of the `verification` group changed nothing, removing the Kover block changed nothing. So it stops being a Test task. StoreWriteBenchmark is now a plain `main` in an `object`, run by a JavaExec, which is what it should have been from the start: a benchmark is a program. Consequences, all improvements: - `check` no longer reaches it -- measured, 0 occurrences of `:benchmark` in a full build log. - No test framework at all. Fixture guards are Kotlin `check(...)`, which throws and fails the task. - No testLogging problem: JavaExec prints to the console, so the measurements are visible without replicating a block java-conventions only sets on `tasks.test`. - Its configurations extend `implementation`/`runtimeOnly` rather than the test ones, because it needs the module's own dependencies and no JUnit. Getting this wrong is what broke compilation on the first attempt at the conversion. Both docs from the previous commit are corrected rather than appended to: they described the Test-task shape and asserted it was not wired into `check`, which was the wrong fact. Full build after: SUCCESSFUL, 2008 tests, 15 skipped, no benchmark. ./gradlew :serverpackcreator-grinder:benchmark: all three measurements print, 100000 rows showing write-through 270.6 ms per record() against coalesced 1.0 us. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Run 646 (job 1650) failed on `ShellTemplateSyntaxTest > fishTemplatesAreSyntacticallyValid` with `FAIL` naming the glob `/templates/*.fish` itself — fish was installed and running, and the directory it was pointed at was empty. The cause is not the template and not fish: a `-v <hostPath>:/templates` bind is resolved on the **daemon's** filesystem, and the Forgejo runner's daemon is a sibling that holds no copy of the job container's `/tmp`. Docker creates the missing source directory and mounts that, so the probe sees nothing and says so in three different voices — a red fish check, a PowerShell parse check that matched zero files and **passed**, and `PowerShellInstallerJavaTest` skipping because its probe script was absent. This commit only pins it. Reproduced against a real remote daemon rather than argued: docker run -d --privileged -e DOCKER_TLS_CERTDIR= -p 12375:2375 docker:dind DOCKER_HOST=tcp://127.0.0.1:12375 ./gradlew :serverpackcreator-api:test \ --tests "de.griefed.serverpackcreator.api.TemplateInterpreterRunnerTest" TemplateInterpreterRunnerTest > theContainerSeesTheStagedTemplates() FAILED container busybox:latest could not see the staged 'default_template.fish' [...] SPC-SYNTAX-CHECK-READY SAW * SPC-PROBE-COMPLETE `SAW *` is the same unexpanded glob the CI log shows. Against the local Docker Desktop daemon both tests pass unchanged, which is exactly why this went unnoticed on every developer machine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`docker run -v <hostPath>:/templates` resolves the source on the daemon's filesystem. The Forgejo runner's job container talks to a sibling daemon that holds no copy of its `/tmp`, so Docker created the missing source directory and mounted an empty one — silently, with a zero exit. Every check built on this runner then answered about a directory with nothing in it. `docker cp` streams the files through the daemon API, so it works whether the daemon is local or remote, and it fails loudly when it cannot. The read-only flag goes with the bind: these are throwaway copies of classpath resources, and a container writing to them reaches nothing else. A `docker create` that produces no container id — no such image, no network for the pull — now returns `setUp = false`, so it reads as "not checked" rather than as a verdict on a template. Measured against `docker:dind` (`DOCKER_HOST=tcp://127.0.0.1:12375`), which is the same topology as the runner: before theContainerSeesTheStagedTemplates FAILED ("SAW *", the unexpanded glob) fishTemplatesAreSyntacticallyValid FAILED (FAIL, the glob itself) powerShellTemplatesParse PASSED (having parsed zero files) theInstallerJavaOverrideWins… SKIPPED (no probe script to run) after all five PASSED Against the local Docker Desktop daemon all five pass before and after, which is why three machines' worth of green runs never saw this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`foreach ($f in (Get-ChildItem -Path '/templates/*.ps1'))` over an empty match iterates zero times, prints no `FAIL` line, and the check reports success for having parsed nothing. That is what `powerShellTemplatesParse` did on run 646 while its fish sibling was failing on the same empty directory: one cause, and only the shell that treats an unexpanded glob as a filename was loud about it. A check that cannot run must skip or fail, never pass. The guard is pinned by `aPowerShellGlobThatMatchesNothingIsAFailure`, and its teeth were checked rather than assumed — deleting the new `if ($templates.Count -eq 0)` line from `parseCommandFor` turns that test red and nothing else: ShellTemplateSyntaxTest > aPowerShellGlobThatMatchesNothingIsAFailure() FAILED `-ErrorAction SilentlyContinue` is on the listing because a non-matching path is a non-terminating error there, and the count is the verdict, not the error record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The Discord message pointed at `/actions/runs/644/artifacts/qodana-report` and that 404s. `/actions/runs/{run}` resolves the run by its per-repo index in every handler Forgejo has under that path -- except `ArtifactsDownloadView`, which calls `getRunByID`. One path, two identifiers. Probed on the instance for the scan ofad5269302: /actions/runs/644/artifacts/qodana-report -> 404 /actions/runs/936/artifacts/qodana-report -> 200, 2515070 bytes /actions/runs/644/ -> 307 to the job view /actions/runs/936/ -> 404 So the run page keeps `github.run_number` (`run.Index`, services/actions/context.go) and the artifact link now uses `github.run_id` (`run.ID`). The comment that sent the artifact link to the wrong one is replaced: it rejected the upload action's `artifact-url` output *because* that output is built from `github.context.runId` -- which is precisely what this route wants. That action had logged `.../actions/runs/936/artifacts/879` in the same job, and it was right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Brings claude-fix-template-probe-transport onto develop. Fast-forwardable -- the branch was 8 ahead and 0 behind, so the merged tree is identical to the tip CI already ran green as run 941 (2011 tests, 15 skipped, 0 failed) and scanned as Qodana run 940 (0 problems, 0 sanity failures). Nothing to re-verify that those two runs did not already answer on the real runner. What it closes, all from run 646: - `ShellTemplateSyntaxTest > fishTemplatesAreSyntacticallyValid` failed on an unexpanded glob because `docker run -v <hostPath>:/templates` resolves its source on the DAEMON's filesystem, and the runner's job container talks to a sibling daemon holding no copy of its `/tmp`. Docker created the missing directory and mounted an empty one, silently. Now `docker cp`, which streams through the daemon API and fails loudly instead. - The same empty directory made `powerShellTemplatesParse` PASS having parsed nothing; a zero-match glob is a `FAIL` now, and that guard's teeth were checked by deleting it (red) and restoring it (green). - `PowerShellInstallerJavaTest` had been SKIPPING for want of its probe script -- so the `JAVA_INSTALLER` fallback that every hand-made pack takes has now actually executed in CI for the first time. - The Discord notification for a Qodana report linked a run URL that 404s: `/actions/runs/{run}` resolves by per-repo index in every Forgejo handler except `ArtifactsDownloadView`, which resolves by run id. Verified after the fix -- run 940's notify job posted an id-shaped URL, 200, 2509362 B. - Qodana's four findings onad5269302: two real (an unused import and a KDoc link the `benchmark` source set cannot resolve) and two analyser errors on `DatabaseStorageService.load`, where Spring's `@NonNullApi` hides that `findOne` really does return null for a miss. Run 940 reports zero, which is also what confirms the two suppression ids were the right ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The Fun Stuff chapter shipped two bash scripts, so the artifact could only be built on Linux/UNIX and only Linux/macOS could run the result -- which excludes most of the people who author modpacks at both ends. Replaced with a Java builder that runs anywhere a JDK does, emitting a `.bsx` for Linux/macOS and a `.cmd` for Windows 10 1803+ that carry a byte-identical tar.gz. `tar` is built into all three operating systems, which is why the payload is a tar.gz rather than the ZIP ServerPackCreator already makes. Verified before being written down, because the old chapter's Linux-only assumption is what happens otherwise. Against `serverpackcreator-api/tests/server-packs/forge_tests` with a 143-character path planted under `config/`, built on macOS: payload magic at the stub's own offset 1f 8b (off-by-one is correct) CR bytes in the stub 0 (a CRLF shebang is fatal) modes in the archive start.sh 0755, server.properties 0644, uid 0 extracted + started under bsdtar (macOS), GNU tar (debian:stable-slim) and busybox tar (alpine): 0755 scripts, long path intact, second run refuses, exit 1 stub parses under macOS sh, dash -n and busybox sh -n both artifacts' payloads: cmp -> identical the emitted PowerShell one-liner parses (Parser::ParseInput, powershell container) Four things the design did not survive contact with, all now in the chapter: - A zero-padded fixed-width offset -- the obvious answer to "the offset changes the length of the thing it measures" -- is rejected outright by BSD tail: `illegal offset -- +000000000639`. The builder iterates to a fixed point instead and writes a bare number. - commons-compress 1.28.0 needs commons-io and commons-lang3 at runtime. `java -cp commons-compress.jar` dies with NoClassDefFoundError on the first archive entry, so the chapter names all three jars. - Commons Compress does not read permission bits off disk on ANY platform, not just on Windows -- it assigns its own defaults. Every mode in the archive is therefore set explicitly, and the stub chmods again after extraction. - `TarArchiveEntry(File, name)` also copies creation time, which becomes a pax header GNU tar warns about on every extraction. Entries are built from the name instead. The Windows stub mirrors `default_template.bat`: cmd.exe cannot seek, so PowerShell does the work under `-ExecutionPolicy Bypass`, because a downloaded `.ps1` is blocked outright. It is strictly linear with no `goto` and no labels (cmd.exe resolves labels by scanning, and gzip output is full of nulls it cannot scan across), and its `-Command` string carries no `"`, no `%` and no `!` -- each of which cmd would eat before PowerShell ever saw it. That half is designed and parse-checked but NOT run: cmd.exe's own tolerance of the appended binary needs a real Windows machine, and the chapter says so rather than implying otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`ApiWrapper.stageOne()` stages `default_java_template.bat` from a resource that has never existed. `JarUtilities.copyFileFromJar` creates the destination *before* resolving the stream and then writes it with `it?.transferTo(out)`, so the safe call swallows the missing resource, the file exists, and the function reports success. Every launch, for every user, recreates a 0-byte template with nothing logged. Two pins, run red before committing: ShippedTemplateStagingTest > stagingWritesNoEmptyFile() FAILED these staged files are empty, so their jar resource could not be read: [default_java_template.bat] JarUtilitiesTest > copyingAResourceThatIsNotInTheJarFailsAndLeavesNoFileBehind() FAILED Expected JarAccessException to be thrown, but nothing was thrown. The staging guard asserts "nothing staged is empty" rather than naming that one file: everything there comes out of the jar and nothing in the jar is empty, so a zero-length file means a resource that could not be read, whichever one it turns out to be. It also asserts its own fixture first, so a run that stages nothing at all fails as a broken fixture instead of passing for having found no empty files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`default_template.bat` builds its command line as `-Command "& '%PSSCRIPTPATH%' %1"`, pasting the script's path into a SINGLE-QUOTED PowerShell string. An apostrophe in that path closes the string early, so every server pack under `C:\Users\O'Brien\...` fails to start with a PowerShell error naming neither the path nor the quote. Apostrophes in Windows user names are ordinary, and the pack author never sees it - it breaks on someone else's machine. The guard derives the invocation from the template instead of restating it: the `SET` lines are expanded the way cmd.exe expands them, textually and in order, and the result is handed to a real PowerShell. A rename of `PSSCRIPTPATH`, or a move back to `-Command`, is caught rather than sidestepped. Red, with the control beside it green, which is what makes the red mean anything: theWrapperStartsAScriptUnderAnOrdinaryPath() PASSED theWrapperStartsAScriptWhosePathContainsAnApostrophe() FAILED 'pwsh' '-NoProfile' '-ExecutionPolicy' 'Bypass' '-Command' '& '/probe/O'Brien/start.ps1' ' Value cannot be null. (Parameter 'key') Two harness details worth keeping, both of which produced a wrong red first: - cmd.exe and sh do NOT tokenise that line the same way. Running the derived line through `sh` splits the argument at the very apostrophe under test and hands PowerShell something cmd would never produce - a harness artefact wearing the bug's clothes. The line is tokenised as cmd does and re-quoted per argument for sh, so PowerShell gets the exact argv it gets on Windows. - `%~dp0` has no trailing `%`, so it is not a `%NAME%` variable and cannot go through the same expansion. Missing that left an unexpanded `%~dp0` in the command and a failure that had nothing to do with quoting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`start.bat` is the entry point for every Windows server pack - it exists so a user never has to touch their execution policy - and it built its command line as `-Command "& '%PSSCRIPTPATH%' %1"`. cmd expands that variable textually, so the pack's path landed inside a single-quoted PowerShell string: one apostrophe closed the string early and the server pack would not start. `C:\Users\O'Brien\...` is an ordinary Windows path, and the pack author never sees the failure, because it happens on someone else's machine. `-File "%PSSCRIPTPATH%" %1` hands PowerShell the path as one argument, which it never re-parses. It also propagates the script's exit code, which `-Command "& '...'"` did not. Both pins from the previous commit go green, against a real PowerShell: theWrapperStartsAScriptUnderAnOrdinaryPath() PASSED theWrapperStartsAScriptWhosePathContainsAnApostrophe() PASSED Full api suite: 495 tests, 0 skipped, 0 failed. Also gives the file the trailing newline it never had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`copyFileFromJar(fileToCopy, identifierClass, directory)` creates the destination before resolving the stream and writes it with `it?.transferTo(out)`, exactly like the overload fixed two commits ago. Fixing only the one `ApiWrapper` happens to call would have left the one the GUI's delete-watcher calls still handing back an empty file and a `true` - the fourth time in this repository that a fix written against one artifact missed its sibling. copyingAResourceThatIsNotInTheJarIntoADirectoryFailsAndLeavesNoFileBehind() FAILED copyingAResourceThatIsNotInTheJarFailsAndLeavesNoFileBehind() PASSED Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Three guards, red against the unused seam, so they fail on behaviour: eachEngineStampsItsOwnContainersAndCanFindThemAlone() FAILED closingOneEngineLeavesAnotherEnginesContainerRunning() FAILED reapingSparesTheReapersOwnContainers() FAILED `OWNER_LABEL` says "a grinder made this" and nothing more, so every question asked of the daemon through it is a question about the whole machine - fine for the shipped singleton service, false everywhere else, and CI is everywhere else. Two `test.yml` jobs share one runner and one daemon; on 2026-09-27 they reached this module's container suite seven seconds apart (runs 954 and 956) and each failed a *different* test of it. A defect fails the same test in both. That pattern is interference. The second guard pins something that already held - `close` sweeps an in-process set - because nothing said so, and the sweep is one edit away from being written as "every container with the owner label", which is the shape `reapOrphans` has and the reason this suite started failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`closingOneEngineLeavesAnotherEnginesContainerRunning` stayed red after the engine was fixed, and the engine was not the reason. `sh -c "echo …; sleep 120"` leaves `sh` as PID 1, which ignores SIGTERM and does not forward it, so `docker stop` waited out its whole grace window, `close` gave up and interrupted its own stopper mid-call, and the container was still there: Some containers did not stop within 15s; abandoning them so shutdown can finish. Container 853fdad… did not stop cleanly: java.nio.channels.ClosedByInterruptException A fixture failing the engine for doing exactly what it promises. The trap-and-one-second-loop idiom is the one `DockerJavaContainerEngineIT` already uses, for this reason. The engines also take a 5s shutdown grace now, which is what the parameter exists for: the class went from 1m26s to 17s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Every assertion and every wait in `DockerJavaContainerEngineIT` asked the *daemon* what was running - by owner label, or by counting `busybox … sleep 300` - and none of that is a question about what the test did. It is the reason runs 954 and 956 each failed a different test of this class within seven seconds of each other: two `test.yml` jobs, one runner, one daemon. They now ask by instance label, through `containersOf(engine)` / `runningContainersOf(engine)`. Reproduced the collision rather than reasoning about it - a container planted with the owner label and a foreign instance id, which is exactly what the other job had: with the foreign container running, scoped refusesToCreateAContainerOnceClosed PASSED closeSignalsAContainerBeforeKillingIt PASSED closeRemovesAContainerLeft… PASSED the same test with the old global assertion refusesToCreateAContainerOnceClosed FAILED `reapsALabelledOrphanLeftByAPreviousProcess` is the exception and says so in its own doc: reaping is global by definition, because an orphan is a container whose maker is gone and the daemon cannot say who is gone. Its assertions are scoped to the orphan it made; the side effect is not scopeable, which is what the next commit serialises the job for. Its `assertEquals(1, reaped)` becomes `>= 1` for the same reason - the count includes whatever else was orphaned on the host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Four fixes, one per finding, each verified the way the finding said it had not been. M-A, the assertion that could not fail. `reapOrphans()` returns a count, so `it >= 0` was always true and `assertTrue(reaped.get(), "the reap must have run")` asserted nothing - the exact class this repository already records, written two days after recording it. The test now starts a container on a *second* engine and pins both halves of the filter in one run: the foreign container is reaped, the reaper's own survives. It is no longer possible to pass it with a `reapOrphans` that removes nothing. M-B, the weakened assertion. `assertTrue(reaped >= 1)` in the orphan test is gone rather than loosened further: reaping is global, so the count there includes whatever else the machine had orphaned, and `>= 1` would pass against a reap that took fifty containers - the scenario that breaks a concurrent job. What that test owns is whether *its* orphan went, and it says so. The count is pinned in `ContainerOwnershipIT`, where a known foreign container makes it mean something. M-C needed no code, only the red the guard had never had for its own subject: rewriting `close()` as a sweep over every owner-labelled container - the shape `reapOrphans` has, and the one the guard forbids - turns it red with "and must leave the other engine's alone ==> expected: 1 but was 0", and restoring it turns it green. H-A, the silent skip. `grinder-container-it.yml` is now the only place `GRINDER_DOCKER_IT` is set, so if it ever stops reaching the JVM every test skips and the job passes having verified nothing. The job reads its own results and fails on a skip or on absent reports. **The first version of that step was itself the bug it guards against** - `grep -c` over a glob reported `ran=1 skipped=0` and exit 0 while both greps were failing with "No such file or directory". Rewritten against the suite attributes and run both ways: gate set container tests: 12, of which skipped: 0 exit 0 gate unset container tests: 12, of which skipped: 12 exit 1, ::error:: L-B, the time claim. "~40 s" is the tests; the job also provisions a JDK and compiles three modules, and what the lock costs on the runner cannot be known until one runs. Both places that quoted the number now say which it is and that the other is unmeasured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Two of the new ownership guards failed on the runner (run 677) while `docker-test.yml` was pulling an image beside them, and the log says exactly why: Stopping 1 container(s) abandoned by an interrupted run — 5s to exit on their own, then killed. Some containers did not stop within 5s; abandoning them so shutdown can finish. Could not remove abandoned container cec2ba2acf1c… `close` budgets the whole sweep - it asks the daemon to stop each container and gives the set one grace window, then interrupts its own stoppers so shutdown cannot hang. A busy daemon misses that window, the container is stopped a moment later anyway, and its *removal* is left to the next startup's reap. That is the documented design. The guard asserted the thing `close` does not promise (nothing left at all, via a `withShowAll(true)` query that counts stopped containers too) rather than the thing it does (nothing running), and so it failed the engine for behaving as specified. It now polls `withShowAll(false)` for up to 60s. The pre-existing `closeRemovesAContainerLeftRunningByAnAbandonedRun` already had this shape and passed in the same run, which is what pointed at the difference. The other failure was the same cause one step earlier: `startSleeper` waited 30s for a container to appear, did not get one, and reported "test setup: there must be one to reap ==> expected 1 but was 0" - a fixture timing out, disguised as a verdict about reaping. 60s now, and the message says which. Verified by reproducing the condition rather than by hoping: the whole container suite run against a deliberately loaded daemon - six concurrent image pulls and a 200 MB md5 in a container - passes 12/12 in 45 s. Not changed, and worth knowing: `stopThenRemove` passes the SAME number as both the per-container `docker stop` timeout and the sweep's overall budget, so the removal step is always racing the budget it shares. Production tolerates that by design (the containers keep the owner label and the next start reaps them), so this is a note, not a fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>