Griefed/ServerPackCreator
Griefed/ServerPackCreator
Watch
1
0
Fork
You've already forked ServerPackCreator
 0
Code Issues 49 Pull requests 1 Projects Releases 382 Packages Wiki Activity Actions

Eat the rich! #677

Merged
Griefed merged 68 commits from develop into beta 2026-09-24 20:53:04 +02:00
Conversation 0 Commits 68 Files changed 89 +3932 -234
Griefed commented 2026-09-23 20:33:27 +02:00
Owner
Copy link
No description provided. 
web/storage and web/scheduling had no test source set at all, so the upload
pipeline below ModPackService was executed by nothing: ModPackControllerTest
mocks the service, and git log on those packages shows only doc and Qodana
commits since the move to MongoDB.

Three new suites, all green against unmodified code, pinning what is there
before it is reworked:

- FileSystemStorageServiceTest (9): naming as <objectId>.zip, the SHA-256 of
  the stored bytes, the "-orig-" display-name recovery, load/delete/deleteAll,
  and the fact that `size` is truncated mebibytes rather than the bytes its
  KDoc claims -- a 1,048,575-byte file reports 0.
- StorageSystemTest (4): an upload leaves two files (the "<millis>-orig-<name>"
  landing copy is never removed), every stored file is also written into GridFS,
  and delete() removes the disk copy while no GridFS delete is even callable.
- FileCleanupScheduleTest (5) / DatabaseCleanupScheduleTest (4): the two
  opposite-direction nightly sweeps, including that the landing copy is what
  reclaims them and that a pack mid-generation survives.

cleanFiles and cleanDatabase are private -- Spring invokes them reflectively
through @Scheduled, and so do these.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED on purpose. Both guards fail against current code, and they fail for the
missing implementation rather than for a fixture fault -- the latch simply
never fires, because the worker thread is gone.

TaskExecutionServiceImpl drains a LinkedBlockingDeque from one `while (true)`
loop whose only catch is InterruptedException. checkModpack itself throws
(`throw StorageException("ModPack-file for ... not found.")` when the archive a
queued row names is absent), and checkConfiguration and ServerPackHandler.run
can throw anything. Such a throw escapes processTask, escapes the loop, and
terminates "GenerationThread" for the lifetime of the process: no supervisor,
no restart, no QueueEvent, no status change. Every later upload then sits in
QUEUED forever and the affected pack in CHECKING forever, and neither of them
is reaped -- DatabaseCleanupSchedule only removes ERROR rows and rows whose
file is gone.

The class had no test of any kind, which is why a defect this size was
invisible. Two asserts, both against the real thread:

- a task queued behind a throwing one is still processed
- a task that threw is reported as ERROR instead of vanishing

Measured before committing: both fail after the full 10 s latch timeout.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns the two guards from the previous commit green.

The worker loop caught only InterruptedException, so anything else a task threw
escaped processTask, escaped `while (true)`, and ended "GenerationThread" for
the lifetime of the process. Nothing restarted it, nothing logged that it was
gone, and nothing downstream could tell: the pack that threw stayed in CHECKING
and every later upload stayed in QUEUED, neither of which DatabaseCleanupSchedule
reaps. One missing archive was enough -- checkModpack throws StorageException
for exactly that.

Each task now runs through runTask, which catches Throwable, logs it, and hands
it to reportFailure: status ERROR on the modpack plus a QueueEvent naming the
failure, so the pack stops being indistinguishable from one still in flight.
reportFailure swallows its own failures on purpose -- an unreachable database
while recording an error must not kill the worker, which is the defect being
removed, and it is the same trade DeclaredIndexCreator and the migration runner
already make.

The loop had to be restructured for the catch to see the task it was running, so
it now blocks on take() instead of polling isEmpty() every second. That also
removes up to 1 s of idle latency per task and lets the thread exit on interrupt
instead of spinning forever.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Behaviour-preserving. Lands the boundary a guard needs before the guard can be
written, rather than committing a test that cannot compile.

What the scanner is pointed at is not observable from outside: it reports its
findings, never its target, so pointing it at the wrong path looks exactly like
scanning a clean modpack. Two changes make it observable without touching the
published signature:

- checkConfiguration(packConfig, configCheck, quietCheck) keeps its exact
  signature and now delegates to an internal four-argument overload carrying the
  scan as a parameter, defaulted to the real Nekodetector -- the same shape
  nekodetectorFindings already uses, and for the same reason.
- The scan block is extracted to scanModpackForInfections, which reads the
  modpack directory from the PackConfig at call time instead of from a value
  captured earlier.

Called from exactly the position it occupied, so nothing moves yet. Verified
against the api suite: 462 tests, 461 green, the single failure being the ZIP
guard added in the next commit; aDirectoryModpackIsStillScanned passes here,
which is what shows the extraction preserved the directory path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED on purpose, and red for the missing implementation: measured before
committing, aZipModpackIsScannedAfterItHasBeenExtracted fails with
"a ZIP upload was never handed to the malware scan ==> expected: <1> but was: <0>".
The companion case, aDirectoryModpackIsStillScanned, is green -- so the guard
discriminates rather than simply failing.

checkConfiguration captures `File(packConfig.modpackDir)` while modpackDir still
names the uploaded archive, then gates the Nekodetector scan on isDirectory --
false for every ZIP. isZip only extracts and repoints modpackDir some thirty
lines further down. Every upload the webservice accepts is a ZIP, so the scan
has been a no-op for the whole of web mode, while still logging "Performing
security scans" and "Performing Nekodetector scan" as though it ran. The only
remaining scan is ServerPackHandler's pass over the finished server pack, after
the output archive is already written and covering only what was copied in.

A scan that reports findings but never its target cannot be audited from its
output, which is why this asserts the target directly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns aZipModpackIsScannedAfterItHasBeenExtracted green.

The Nekodetector call sat above the modpack-source branch, gated on
`File(packConfig.modpackDir).isDirectory`. For a ZIP that is false -- modpackDir
names the archive until isZip extracts it and repoints the field some thirty
lines further down -- so the scan was skipped for every ZIP while still logging
"Performing security scans" and "Performing Nekodetector scan". Since every
upload the webservice accepts is a ZIP, the malware scan has been a no-op for
the whole of web mode, and the log said otherwise.

Moved below the branch, where modpackDir names something Nekodetector can
actually walk. The skip now logs a warning naming the path instead of passing
silently, so a scan that does not happen says so.

Behaviour change for embedders, recorded in claude-docs/API-BEHAVIOUR-CHANGES.md:
checkConfiguration on a ZIP can now return otherErrors it previously never
produced. Signatures are unchanged.

Verified: api 462 tests / 0 failures / 1 skipped, app 192 / 0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED on purpose. Measured before committing: app suite 195 tests, these 3 failed,
each with

  java.nio.file.NoSuchFileException:
    <root>/modpacks/1790107642029-orig-../../../escaped.zip

which is the mechanism itself, not a fixture fault.

StorageSystem.store(MultipartFile) builds its destination by concatenating
getOriginalFilename() -- whatever the client put in Content-Disposition, which
Spring hands over verbatim, separators included -- onto a "<millis>-orig-"
prefix, with no sanitisation and no containment check, while
FileSystemStorageService.store two calls downstream carries one on a path built
from a generated ObjectId and comments it "This is a security check". The guard
is on the internal path and absent from the external one.

Measured against a real Tomcat + Spring 7.0.8 stack before writing these: the
filename does arrive intact, and from three "../" up the resolved path does
leave the storage root -- but nothing is ever written there. "-orig-"
concatenates without a separator, so the first component is the literal name
"<millis>-orig-..", which is not an existing directory, and the OS resolves ".."
only through directories that exist. The open fails with ENOENT first. So this
is hardening, not a live arbitrary write, and it is worth saying plainly: the
only thing standing between client input and an arbitrary path is an accident
of string concatenation that stops holding the moment the prefix changes.

What *is* live is the third guard: that IOException is caught by nobody --
ModPackController catches StorageException only -- so any filename containing a
separator is an unhandled 500, and the shipped application.properties sets
server.error.include-stacktrace=ALWAYS on an endpoint with no auth and
@CrossOrigin("*").

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns the three guards from the previous commit green.

The client's filename is now reduced to its base name before it can reach a
File, and the destination is checked to sit directly under the storage root --
the same check FileSystemStorageService already applies to the internal path,
now also on the external one, which is where the untrusted string actually is.
That replaces a containment property that until now held only by accident: the
"-orig-" prefix concatenates without a separator, so a leading ".." became part
of a directory name that does not exist and the open failed with ENOENT. True
today, and true only until the prefix changes.

The live half of the defect is the error handling. transferTo throws IOException
(and IllegalStateException), ModPackController catches StorageException only, so
until now any filename carrying a separator produced an unhandled 500 with a
full stack trace and absolute server paths -- the shipped application.properties
sets server.error.include-stacktrace=ALWAYS, on an endpoint with no
authentication and @CrossOrigin("*"). Both are now caught and reported as an
empty result, and ModPackService turns that empty into a StorageException the
controller already answers as a 400, instead of calling Optional.get() on it and
raising NoSuchElementException -- another 500 by the same route.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Behaviour-preserving: nothing calls it yet. Lands the boundary so the guard for
the GridFS leak can compile and be committed red, instead of bundling guard and
change.

DatabaseStorageService had store and load and no delete at all, which is the
whole reason StorageSystem.delete removes only the filesystem copy and both
nightly cleanups walk only the filesystem. Every stored modpack and every
generated server pack therefore exists twice -- once on disk, once in GridFS --
and only one of the two is ever reclaimed.

Keyed on the same query shape as load, which the next commit pins: Spring Data's
query mapper converts a valid 24-hex id String to an ObjectId for _id, so the
String ids StorageSystem hands around do match the ObjectId GridFS stored.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two of the five are RED, and each fails for the missing implementation rather
than for a fixture fault -- measured before committing:

  loadingAnIdThatGridFsDoesNotHoldIsEmptyRatherThanAThrow
    -> java.lang.NullPointerException: findOne(...) must not be null
  deletingRemovesBothCopiesOfTheFile
    -> Verification failed: GridFsTemplate.delete(any<Query>()) was not called

The third guard is green and is the one the other two rest on. StorageSystem
stores a file under the ObjectId GridFS minted and then passes that id around as
a String, so every read and delete queries _id with a String against a field
holding an ObjectId. Asked directly of Spring Data's own QueryMapper -- no
database, the same approach this module already uses for its index and
collection-name declarations -- a valid 24-hex String is converted to an
ObjectId, so the lookup does match. Worth pinning precisely because the failure
mode if it ever stops holding is silence: a query that matches nothing is
indistinguishable from a file that is not there.

Note this corrects an assumption made while reading the code: the query is not
broken. What is broken is the miss, which returns Optional.of(Pair(null, ...))
and therefore an NPE -- reached whenever the filesystem copy is gone, which is
the ordinary state after cleanup, turning an intended 404 into a 500 with a
stack trace the shipped config sends to the client.

deletingRemovesTheArchiveFromDiskButLeavesTheGridFsCopy, added an hour ago to
characterize the leak, becomes deletingRemovesBothCopiesOfTheFile. Changing an
existing expectation is the stop-and-flag signal for a refactor; this is a fix,
and the expectation it asserted was the defect.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns both red guards from the previous commit green.

StorageSystem.delete now removes the GridFS copy alongside the filesystem one.
Every stored file is written to both -- DatabaseStorageService.store is a real
gridFsTemplate.store, called to mint the ObjectId the file is named after -- so
reclaiming only one of them meant fs.files and fs.chunks grew without bound for
the life of the installation, at roughly twice the storage of every modpack and
every generated server pack.

DatabaseStorageService.load returns Optional.empty() for an id GridFS does not
hold, instead of Optional.of(Pair(null, ...)) and therefore an NPE. This is not
an exotic path: StorageSystem.load falls back to GridFS precisely when the
filesystem copy is gone, so it is what the download route hits after a cleanup,
and the shipped application.properties sets include-stacktrace=ALWAYS, so the
500 carried a stack trace to an unauthenticated caller.

Per the decision to complete GridFS rather than retire it, this keeps the
database-backed copy and its ~2x storage as the deliberate trade, and makes the
lifecycle match: written by store, removed by delete.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All four RED, each for the missing implementation. Measured before committing:

  anUploadThatFailsValidationLeavesNothingUnderTheStorageRoot
    expected: <[]> but was: <[651f3c0e9a1b2c3d4e5f6071.zip, 1790108096710-orig-pack.zip]>
  anUploadThatFailsValidationIsNeverWrittenToGridFs
    Verification failed: GridFsTemplate.store(...) should not be called; Calls: 1)
  aDuplicateUploadIsNotStoredASecondTime
    Verification failed: GridFsTemplate.store(...) should not be called; Calls: 1)
  anAcceptedUploadLeavesExactlyOneArchiveAndNoLandingCopy
    expected: <[...zip]> but was: <[...zip, 1790108096912-orig-pack.zip]>

saveUploadedFile stores before it validates: the landing copy, the GridFS
document and the final archive are all written, and the file fully hashed,
before checkZipArchive is consulted and before the duplicate check runs. Both
rejection paths then throw with no cleanup. So a rejected upload costs exactly
what an accepted one costs, and an anonymous caller can repeat it.

The filesystem half is reclaimed at 00:30 by FileCleanupSchedule. The GridFS
half is not reclaimed at all, and cannot be: the sweep works back from ModPack
rows, and a rejected upload never gets one.

The fourth guard is the one that keeps the fix honest -- it pins that an
accepted upload leaves its archive and nothing else, so the landing copy stops
being something a nightly cron has to mop up.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns all four guards from the previous commit green. App suite 203 tests, 0
failures.

Storing is now two steps. StorageSystem.land writes the upload to the storage
root and nothing else; StorageSystem.store commits it to GridFS and to its final
name. saveUploadedFile lands, validates the archive, hashes it, checks for a
duplicate, and only then commits -- so a rejected or duplicate upload writes no
GridFS document and no archive, where before it wrote both and threw afterwards
with no cleanup. The GridFS half of that was unreclaimable by construction: the
nightly sweep works back from ModPack rows, and a rejected upload never gets one.

The landing copy is deleted in a finally block, on every path including the
accepted one. It was previously left behind on every single upload and reclaimed
only by the 00:30 cron, so each upload occupied twice its own size until then.
FileCleanupSchedule still sweeps such files; it is now the backstop for a crash
between landing and cleanup rather than the routine path, and its test says so.

Hashing moves to FileSystemStorageService.sha256Of, which streams the file a
buffer at a time instead of digest(file.readBytes()). With max-file-size at
5000MB the old form was an OutOfMemoryError outright -- a Java array cannot
exceed about 2 GB -- and it is on the accepted path of every upload. It also
takes a fresh MessageDigest per call, deriving only the algorithm from the
injected one: MessageDigest is stateful and not thread-safe, uploads run on
request threads, and two concurrent hashes sharing one instance interleave into
two wrong answers, which for the hash the duplicate-check keys on means a false
rejection or a missed duplicate. The bean was documented as "a fresh instance per
injection" but an unscoped @Bean is a singleton, so that guarantee did not exist.

saveUploadedFile's two rejections are extracted to rejectIfInvalid and
rejectIfDuplicate; store(MultipartFile) is replaced by land, which had no other
caller.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the GridFS lifecycle. delete() already reclaims both copies; the
nightly file sweep did not, so an orphaned archive it unlinked left its database
twin behind for good. It now routes any file named after a storage id -- 24 hex
characters, i.e. an ObjectId -- through ModPackService/ServerPackService
deleteStoredFile, and unlinks only what has no twin, such as a landing copy left
by a crash.

After the validate-before-store change this is a narrow window rather than the
routine path: a rejected upload no longer reaches GridFS at all, so what is left
is a crash between store() and save(). Narrow is not the same as closed, and a
half-managed second tier is what produced the original leak.

Also drops the non-null assertion on ModPack.fileID. The server-pack branch
three lines below already filtered nulls first; the modpack branch did not, so a
single row with a null fileID aborted the entire nightly pass with an NPE before
anything was deleted. Both branches now use mapNotNull.

The guards could not be committed red ahead of the change: expressing "the
GridFS twin was reclaimed" requires the services the constructor did not yet
take, and a guard that cannot compile is not a red pin. Teeth were instead
verified by mutation, both reproduced against this commit:

  deleteStored(storageId) -> file.deleteQuietly()
    => anOrphanedArchiveIsDeletedThroughStorageSoItsGridFsTwinGoesWithIt FAILED
  mapNotNull { it.fileID } -> map { it.fileID!! }
    => aModpackRowWithoutAFileIdDoesNotAbortTheSweep FAILED
    => aServerPackRowWithoutAFileIdIsSkippedRatherThanFailingTheSweep FAILED

The loop body is extracted to a private sweep() shared by both roots, which is
what let the two branches stop disagreeing about null handling.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three RED, each for the missing implementation. Measured before committing:

  downloadingAModpackStreamsTheArchiveInsteadOfCopyingItIntoTheHeap
    the whole archive was read into memory to serve it ==> expected: <false> but was: <true>
  downloadingAServerPackStreamsTheArchiveInsteadOfCopyingItIntoTheHeap
    the whole archive was read into memory to serve it ==> expected: <false> but was: <true>
  rejectingAnEmptyUploadDoesNotReadTheUploadIntoMemory
    java.lang.AssertionError: the controller read the whole upload into memory
      to check it was not empty

spring.servlet.multipart.max-file-size ships at 5000MB and a Java array cannot
hold more than about 2 GB, so every one of these is an OutOfMemoryError on a
large pack and on several concurrent medium ones. Both download handlers answer
with ByteArrayResource(archive.readBytes()); the upload guard calls
file.bytes.isEmpty() when file.size == 0L on the line above has already answered
that, and the || short-circuit means it runs on every non-empty upload.

The third guard works by overriding getBytes() to throw, so it fails if the
controller so much as asks -- which is the only way to observe "did not
materialise it", since a successful call looks identical either way. The
controllers are called directly rather than through MockMvc for the same reason:
the question is what the controller touches, not what the framework does around it.

The first run of the server-pack guard failed for a fixture fault instead -- a
justRun on updateDownloadStats, which returns Optional<ServerPack>, produced
"ClassCastException: kotlin.Unit cannot be cast to Optional". Fixed and re-run
before committing, because a guard that goes red for its own mistake pins nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns the three guards from the previous commit green; the existing controller
tests stay green unchanged.

Both download handlers now answer with a FileSystemResource and an explicit
content length, so the servlet container copies the file straight to the socket.
ByteArrayResource(archive.readBytes()) allocated the entire archive per request:
with max-file-size at 5000MB that is an OutOfMemoryError outright above ~2 GB --
a Java array cannot be larger -- and a handful of concurrent medium downloads
reach the same place.

The upload guard drops file.bytes.isEmpty(). file.size == 0L on the line above
already answers whether the upload is empty, and because || short-circuits, the
getBytes() call ran on every upload that was not empty -- that is, on all the
real ones. Reading a 5 GB upload into a ByteArray to learn it is not zero bytes
long was the most expensive line in the request.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three RED, each for the missing implementation. Measured before committing:

  DatabaseCleanupScheduleTest.aModpackRowWithoutAFileIdDoesNotAbortTheSweep
    -> java.lang.NullPointerException
  DatabaseCleanupScheduleTest.aServerPackRowWithoutAFileIdDoesNotAbortTheSweep
    -> Unexpected exception thrown: java.lang.reflect.InvocationTargetException
  FileCleanupScheduleTest.aRepositoryThatReturnsNoRowsAtAllDoesNotWipeTheDirectory
    -> Verification failed: ModPackService.deleteStoredFile(any<String>())
       should not be called; Calls: 1)

DatabaseCleanupSchedule dereferences modpack.fileID!! and serverpack.fileID!!.
A server pack has no fileID until its generation finishes, so any pack still in
flight at midnight ends the pass -- and it ends it partway through, after some
rows have already been deleted.

FileCleanupSchedule deletes every file no row refers to, so a repository that
returns nothing means every file is an orphan. Correct for a genuinely empty
installation; catastrophic for one pointed at the wrong database, which this
project has already shipped once -- Boot 4 retired spring.data.mongodb.uri and
the app silently used Mongo's default `test` database. A destructive nightly job
should not be how that gets discovered.

The empty-repository guard first passed for the wrong reason: deleteStoredFile is
mocked, so "the file is still there" was true whether or not the sweep had asked
for it to go. Rewritten to assert on the delete calls. Asking why a guard passed
is the same question as asking why it failed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns all three guards from the previous commit green.

DatabaseCleanupSchedule reads fileID through a local val instead of `!!`. A
modpack row with no fileID never had an archive and is treated as orphaned; a
server pack with no fileID is *skipped*, which is not merely null-safety --
a server pack has no fileID until its generation finishes, so deleting one would
remove a pack that is still being built. The modpack lookup behind the server-pack
branch is guarded with isPresent rather than calling get() on a possible empty.
Before, any one of these ended the pass midway, after other rows had already gone.

FileCleanupSchedule refuses to sweep a directory when its repository reports no
rows at all and files are present, logging what it found instead. "No rows" makes
every file an orphan, which is right for an empty installation and unrecoverable
for one reading the wrong database -- a state this project has already shipped,
when Spring Boot 4 retired spring.data.mongodb.uri and the app silently used
Mongo's default `test` database. The trade is deliberate and worth stating: an
installation whose packs were all legitimately deleted will keep its files until
someone removes them by hand. Reclaiming disk can wait for a human; deleting
every modpack and server pack cannot be undone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two RED, one green, and the green one is the point: it discriminates. Measured
before committing:

  aModpackCarryingAManifestStillHasItsServerIconFound
    -> the icon was not found; serverIconPath was '' ==> expected: <true> but was: <false>
  aModpackCarryingAManifestStillHasItsServerPropertiesFound
    -> server.properties was not found; path was '' ==> expected: <true> but was: <false>
  aModpackWithoutAManifestKeepsFindingItsServerIcon
    -> PASS

isZip ends by looking for server-icon.png and server.properties under `packName`,
which is whatever checkManifests returned -- a display string such as
"A Manifest Named Pack", not a path. updatePackName confirms it: the name comes
straight out of the JSON, and its fallback is File(modpackDir).name, i.e. the
bare directory name. So File(packName, "server-icon.png") resolves against the
JVM's working directory and cannot exist. Only the no-manifest fallback, where
packName is set to the extracted directory, actually works.

That inverts which modpacks get the feature: a plain zip keeps its icon, and the
CurseForge, GDLauncher and MultiMC exports -- the ones with manifests, i.e. most
real modpacks -- silently lose both files from their server pack. Silently is the
operative word, since an absent icon is indistinguishable from a modpack that
never had one.

Fixture is built in the test rather than added as a binary: the existing zips
carry manifests but no icon, which is exactly the combination that cannot show this.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns both manifest guards green; the no-manifest discriminator stays green.
api suite 465 tests, 0 failures, 1 skipped.

isZip looked for server-icon.png and server.properties under `packName` -- the
string checkManifests returns, which updatePackName takes straight out of the
manifest JSON. A display name, not a path. So the lookup resolved against the
process working directory and could never match for a modpack carrying a
manifest, while the no-manifest fallback, which sets packName to the extracted
directory, worked fine.

That inverted the feature: a plain ZIP kept its icon, and the CurseForge,
GDLauncher, ATLauncher and MultiMC exports -- most real modpacks -- silently lost
both files from their server pack. Silently, because an icon that was not found
looks exactly like a modpack that never had one.

Now read from the extracted modpack, which is the only place they can be. The
pack name is no longer used for anything after checkManifests, so it is no longer
kept; that also removes the pathSecureTextAlternative call on it, which was the
only thing between a crafted manifest name of "../../something" and a lookup
outside the modpacks tree -- and that method deliberately does not strip / or \.

Recorded in claude-docs/API-BEHAVIOUR-CHANGES.md: signatures are unchanged, but a
server pack generated from a manifest-carrying modpack now contains two files it
previously omitted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED for the missing implementation: "a modpack that could not be extracted
passed its own checks".

FileUtilities.unzipArchive wraps extractAll in a catch for IOException that only
logs, so isZip carries on against a directory that is empty or half-written --
reading manifests, hunting for an icon, and returning a ConfigCheck with no
modpack errors at all. zip4j's own zip-slip rejection is a ZipException, which
extends IOException, so a hostile archive takes the same silent path as a
truncated one.

The failure is produced by putting a regular file where the extraction directory
has to be created, rather than by hand-corrupting an archive: it does not depend
on zip4j internals or on whether CRCs are verified, so the guard cannot quietly
stop reproducing the condition it is meant to pin.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns anArchiveThatCannotBeExtractedFailsTheModpackChecks green; the existing
FileUtilitiesTest stays green unchanged.

unzipArchive caught every IOException around extractAll and only logged it, so
its caller could not distinguish a completed extraction from an empty or
half-written directory. isZip then read manifests, looked for a server-icon and
returned a ConfigCheck with no modpack errors -- from a directory that had never
been populated. zip4j reports a rejected zip-slip entry as a ZipException, an
IOException, so a hostile archive took exactly the same silent path as a
truncated one.

It now declares @Throws(IOException::class) and lets the failure out. Nothing
else in -api needs changing: isZip already declared IOException and
checkConfiguration already catches it around the isZip call, turning it into the
modpack error the user should have been getting all along.

Recorded in claude-docs/API-BEHAVIOUR-CHANGES.md -- an embedder calling
unzipArchive directly now has an exception to handle that the old signature
never threw, which is precisely the kind of change that breaks nothing at
compile time.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED for the missing implementation: "expected: <1.20.1> but was: <>". Nothing
read the index, so the Minecraft version was never set.

updateConfigModelFromModrinthManifest is complete, handles all four loaders, and
is exported through ConfigurationHandler -- and is called by nothing.
modrinth.index.json is absent from manifestCandidates, which checkManifests
documents as its single source of truth, so the dispatch never reaches it.

What a user sees is not "Modrinth is unsupported" but "Invalid modloader
specified": an undetected loader leaves PackConfig.modloader empty, and
ModloaderValidator then correctly rejects the empty value. The diagnosis points
at the modpack rather than at the missing branch, which is why a whole parser
could sit unreachable without anyone filing it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns aModrinthIndexSuppliesTheMinecraftVersionAndModloader green.
api suite 467 tests, 0 failures, 1 skipped.

modrinth.index.json is now the third manifest candidate, after the two
CurseForge ones, and checkManifests dispatches to
updateConfigModelFromModrinthManifest -- which was already complete, already
handled Fabric, Quilt, Forge and NeoForge, and was already exported through
ConfigurationHandler. The only thing missing was the entry in the candidate list
that checkManifests documents as its single source of truth.

The user-facing symptom was not "Modrinth is unsupported". PackConfig.modloader
silently ignores values it does not recognise, so an undetected loader stayed ""
and ModloaderValidator reported "Invalid modloader specified" -- pointing at the
modpack instead of at the missing branch, which is how a complete parser sat
unreachable without a bug report.

Placed after the CurseForge manifests deliberately: first hit wins, so a pack
carrying both keeps resolving to CurseForge exactly as before. manifestCandidates
now returns 7 entries rather than 6; ManifestCandidatesTest follows it, which is
expected for a fix rather than the stop-and-flag signal it would be for a
refactor.

New key configuration.log.error.zip.modrinth in Translations_en_GB.properties,
the locale i18n4k generates from. pt_BR and zn_GB fall back to English rather
than get a machine translation.

Recorded in claude-docs/API-BEHAVIOUR-CHANGES.md.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED: "expected: <1048575> but was: <0>".

FileSystemStorageService computes size().div(1048576.0).toInt() -- truncated
mebibytes -- while SavedFile, ModPack and ServerPack all document the field as
"size in bytes". Everything under a mebibyte therefore reports 0, and both
ModpacksTable.vue and ServerPacksTable.vue render the download button behind
`v-if="props.row.size > 0"`. A modpack or server pack under 1 MiB is not merely
mislabelled, it is undownloadable from the tables.

The expectation is a Long on purpose: the shipped
spring.servlet.multipart.max-file-size is 5000MB, which does not fit in an Int,
so storing bytes in the current Int field would overflow. This is a type change,
not only a unit change.

This flips the characterization committed in "characterize the storage seam and
both cleanup schedules", which recorded the truncation deliberately so the
correction would show up as a change rather than slip through.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns storeReportsSizeInBytesAsItsNameAndDocsSay green. app 211 tests / 0
failures, frontend 35 / 0 (was 32 -- the three new ones are the formatter).

size was computed as size().div(1048576.0).toInt() while SavedFile, ModPack and
ServerPack all documented it as bytes. Everything under a mebibyte therefore
reported 0, and both pack tables gated their download button on
`v-if="props.row.size > 0"` -- so a modpack or server pack under 1 MiB was not
merely mislabelled, it could not be downloaded from the tables at all. Server
packs are routinely that small.

The field widens to Long with the unit, necessarily: the shipped
spring.servlet.multipart.max-file-size is 5000MB, which does not fit in an Int,
so storing bytes in the old type would overflow.

Formatting moves to the SPA, where the unit is known: src/utils/format.ts renders
binary units and answers "unknown" for an absent size rather than
"NaN undefined". Both tables and both cards use it, and both `size > 0` gates are
gone rather than re-expressed -- the tables stay untested, per the standing
decision recorded in the frontend's CLAUDE.md, so the logic that could be tested
was moved out of them. Its guards were verified by mutation: scaling at 1048576
instead of 1024 fails two cases, dropping the non-finite guard fails the third.

Existing documents are deliberately not rewritten. A value-only migration cannot
tell a converted row from an unconverted one -- nothing marks them, the same trap
already recorded for supersededLegacyKey -- and it no longer matters: with the
size > 0 gate gone the field is cosmetic, so an old row simply renders as a few
hundred bytes. Stated in claude-docs/API-BEHAVIOUR-CHANGES.md rather than
silently accepted.

Response samples in Modpacks.md and Server-Packs.md restated in bytes.
api-docs.yaml still says int32 and is regenerated in the next commit, together
with the multipart request body it also gets wrong.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The published spec described POST /api/v2/modpacks/upload as taking an
application/json body containing a binary `file` property. It takes
multipart/form-data. That is what springdoc emitted, not hand-drift: the mapping
declared only `produces`, so springdoc had nothing to infer the request media
type from. The mapping now declares
consumes = MediaType.MULTIPART_FORM_DATA_VALUE, which also makes Spring answer a
wrong content type with 415 instead of accepting it by accident.

api-docs.yaml is regenerated rather than patched, by the command recorded beside
the springdoc dependency -- bootRun in web mode, then curl /v3/api-docs.yaml.
The diff against the committed copy is exactly three things, which is the useful
part: the multipart body above, and ModPack.size / ServerPack.size moving from
int32 to int64 after the previous commit. Nothing else had drifted.

The info block is still applied by hand and now says so instead of claiming the
whole file is generated. springdoc is a developmentOnly dependency, so it cannot
be configured by an annotation on a production class, and its defaults are
"OpenAPI definition", version v0 and a servers entry pointing at
http://localhost:8080 -- the last of which has no business in a published spec.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both RED, asked of Spring Data's mapping context rather than of a database.
Measured before committing:

  aRecordedDownloadIsNotIdentifiedByItsTimestamp
    -> ModPackDownload is identified by its timestamp, so same-millisecond
       downloads overwrite each other
  theDownloadStatsSortNamesAFieldTheDocumentActuallyHas
    -> sorted on [date], but ModPackDownload only has [downloadedAt, modPack]

Two defects, and neither reports itself. ModPackDownload and ServerPackDownload
carry @MongoId on `downloadedAt`, a millisecond Date -- a global id, not a
per-pack one, so any two downloads of any two packs in the same millisecond
collide and save() overwrites the earlier row. The loss is invisible because the
download *counters* live on the packs and are unaffected, so only the history
thins out.

DownloadStatsService then sorts all four of its queries on "date", which no
download document has. MongoDB does not reject a sort on an absent field; it
simply does not order, so the stats endpoints have been returning arbitrary order
while looking sorted. Same shape as EventService.loadAll's "dateCreated", which
QueueEvent also does not have.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED: "sorted on [dateCreated], but QueueEvent only has [id, modPackId,
serverPackId, status, message, timestamp, errors]".

EventService.loadAll's unpaginated overload sorts on "timestamp" correctly; the
paginated one defaults to "dateCreated", which QueueEvent does not have. Same
defect as DownloadStatsService's "date", found by the same question, so it is
pinned the same way -- capture what the service asks the repository for and check
the property names against the mapping context.

MongoDB does not reject a sort on an absent field, so /api/v2/events/allpaginated
has been returning arbitrary order while presenting itself as newest-first. There
is no error anywhere to notice.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns all three guards from the previous two commits green. api 467 tests / 0
failures / 1 skipped, app 214 / 0.

ModPackDownload and ServerPackDownload carried @MongoId on `downloadedAt`, a
millisecond Date. That id is global rather than per-pack, so two downloads of any
two packs in the same millisecond collided and save() overwrote the earlier row.
Both now have their own id, assigned by MongoDB, with downloadedAt demoted to an
ordinary field. The loss this caused was invisible from outside: the download
*counters* live on the packs and were never affected, so only the history thinned.

Existing rows keep their Date _id and are read back fine -- Mongo does not
require one type per collection, and nothing queries these by id.

DownloadStatsService sorted all four of its queries on "date", and
EventService.loadAll's paginated overload on "dateCreated". Neither field exists
on the document being sorted; MongoDB does not reject that, it simply does not
order. So the download-history and paginated-event endpoints have been returning
arbitrary order while presenting themselves as newest-first. Both now name the
timestamp field their document actually has, which the guards check against the
mapping context rather than against a database.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four bugs, one path, all of them about an id that is not what it claims to be.

Backend: the upload endpoint's catch used `ex.id.toString()`. StorageException's
id is null on the validation-failure path -- that branch uses the single-argument
constructor -- so the JSON carried the *string* "null". The SPA tests
`modPackId !== null`, which a non-empty string passes, so a failed upload told
the user "See Modpack ID: null" and offered to regenerate it.

Frontend, found while confirming the above:
- Both regeneration handlers read response.data.modPackID / runConfigID.
  ZipResponse spells them modPackId / runConfigId, so these were undefined and
  silently cleared the two pickers the user had just chosen from.
- The error handler read error.data rather than error.response.data. On an axios
  error `data` does not exist on the error itself, so this threw a TypeError from
  inside the catch block -- after the notify, so the user saw the message and
  then the form stopped responding. It now reads through optional chaining and
  keeps the current selection when the server said nothing useful, which is also
  correct for a network-level failure where there is no response at all.

Frontend suite 35 tests, 0 failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
/api/v2/modpacks/upload has no authentication, no CSRF and @CrossOrigin("*"), and
saveUploadedFile joins ConfigCheck's errors verbatim into the 400 body. Two of
those errors embedded the absolute path of the archive on the server, so a
caller learned the deployment's directory layout by uploading a file that is not
a ZIP. They now name the file instead; the full path is still logged, where it is
useful and not public.

server.error.include-stacktrace goes from ALWAYS to NEVER for the same reason --
any unhandled exception returned a full stack trace, with package structure,
versions and absolute paths, to whoever asked. include-message stays ALWAYS: those
messages are ours and are what the SPA shows the user.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three RED, one green discriminator. Measured before committing:

  aTrailingDotIsTrimmedWithoutTouchingTheOthers
    expected: <My Pack v1.2> but was: <My Pack v12>
  aTrailingSpaceIsTrimmedWithoutTouchingTheOthers
    expected: <My Pack v1> but was: <MyPackv1>
  severalTrailingOffendersAreAllTrimmed
    expected: <All the Mods 9> but was: <AlltheMods9>
  theIllegalCharactersAreStillRemovedAndSeparatorsStillAreNot -> PASS

StringUtilities.pathSecureTextAlternative strips a trailing "." or " " -- Windows
rejects a file name ending in either -- by taking the last character and calling
replace() with it, which removes *every* occurrence in the string. A trailing
space therefore deletes all spaces, and a trailing dot deletes every separator in
a version number.

Published API, and it now has no call site left inside this repo: its only one was
the server-icon lookup removed two commits ago. So an embedder is the only person
who can reach it, which is precisely why it needs a guard rather than a deletion.

The fourth case is there to keep the fix honest: this method deliberately does
NOT strip / or \, which its own KDoc states and which a "tidy it up with a path
sanitiser" fix would quietly change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns the three guards from the previous commit green.

Both pathSecureText and pathSecureTextAlternative stripped a trailing "." or " "
-- Windows rejects a file name ending in either -- by taking the last character
and calling replace() with it, which removes every occurrence in the string. A
trailing space therefore deleted all spaces and a trailing dot deleted every
separator in a version number: "My Pack v1 " became "MyPackv1", and
"All the Mods 9. . " became "AlltheMods9". Both now use trimEnd.

The second method was found by the fix landing on the wrong one: the two loops
were byte-identical, so a single-occurrence replace patched pathSecureText while
the guard was still red against the alternative. Worth saying because it changes
the severity -- pathSecureTextAlternative has no call site left in this repo, but
pathSecureText does: PackConfig runs serverPackSuffix through it when a
configuration is loaded, so this was live for anyone whose suffix ended in a dot
or a space.

The two pathSecureText cases were written after the fix, so their teeth were
verified by mutation rather than by a red commit -- restoring the replace-loop in
pathSecureText alone fails pathSecureTextCarriesTheSameDefectAndTheSameFix and
nothing else. A companion case pins that pathSecureText still strips / and \,
which is the documented difference between the two and what a shared fix could
quietly erase.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No behaviour change. Each of these was read during this pass and each was wrong
in the direction that matters -- they describe a protection or a property that
is not there, so a reader stops looking.

- ServerPack.sha256 claimed it was "Indexed, because it is what the
  de-duplication looks up". It carries no @Indexed and nothing looks a server
  pack up by hash; the de-duplication is modpack-only, on ModPack.sha256.
- ModPackRepository.findFirstBySha256 claimed application.properties enables
  index creation. It does not -- spring.data.mongodb.auto-index-creation is
  commented out there deliberately, because it makes a reachable MongoDB a
  condition of starting up, and DeclaredIndexCreator does the work on
  ApplicationReadyEvent instead. Following the old comment would re-add the line
  the module's CLAUDE.md records as a landmine.
- ModPackService.deleteModpack claimed to delete "the server packs generated from
  it". It deletes the modpack row and its archive; the server packs survive and
  stay downloadable.
- SecurityScans carried "Zip-slip and archive-safety checks applied before an
  upload is trusted" above a companion whose only member is a malware scanner.
  No such code has ever existed in that class. Removed rather than reworded: zip
  traversal is rejected by zip4j during extraction, which is a fact about a
  dependency and does not belong in this file's doc.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
serverpackcreator-app/CLAUDE.md said springdoc is commented out and the API spec
is a hand-maintained snapshot. It is not: springdoc is a developmentOnly
dependency with the regeneration command beside it, and regenerating it this pass
found the upload endpoint documented as application/json when it is multipart.
The entry now says regenerate rather than hand-edit, and names the one part that
genuinely is applied by hand.

Also records what the pass created: web/storage and web/scheduling have test
source sets for the first time, with the three patterns worth copying -- a real
FileSystemStorageService over a @TempDir, and two suites that ask Spring Data's
own QueryMapper and MongoMappingContext what the persistence layer will do
instead of needing a database. And a landmine that cost a full diagnosis here:
editing -api or -app sources while a test task runs produces NoClassDefFoundError
in unrelated tests, which reads exactly like a regression.

BACKLOG gains B41 (the shared parent directory that checkManifests reads,
latent and with both cheap fixes costing something real) and B42 (the public
surface: CORS, no auth, a state-mutating GET, unbounded /all routes and no
retention -- product decisions rather than defects, listed with what was fixed
regardless of how they are decided).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Refactor-state counts re-derived from build/test-results after a clean run:
api 460 -> 473, app 168 -> 214, frontend 32 -> 35.

REFACTOR-LOG gains the pass narrative, written around the two findings that
changed on contact with evidence rather than around the list of fixes -- the
upload filename was not an exploitable write, and the GridFS id query was not
broken -- because those are the ones a reader would otherwise re-derive from the
same wrong starting point. It also records the defect shape that recurred three
times (a name that refers to nothing: two Sort fields, one @MongoId) and the two
process mistakes that cost time.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured, before and after, on :serverpackcreator-app:test:

  WebServiceContextTest    60.37s -> 0.92s
  DatabaseUriPropertyTest  62.22s -> 5.71s
  wall across all classes  147.6s -> 29.1s   (Gradle task 2m37s -> 39s)
  214 tests, 0 failures, unchanged

Both @SpringBootTest classes boot the real context and need no database -- the
driver connects lazily, which is the point of them -- but each has two
ApplicationReadyEvent listeners that touch Mongo: DeclaredIndexCreator and the
migration runner. At the driver's default 30 s server-selection timeout that is
60 s per context, so 122.6 s of a 147.6 s suite was spent waiting for a server
nobody expects to be running.

This module's CLAUDE.md said shortening it in src/test/resources "does not work --
the effective URI comes from the generated test home". That is wrong, and the
entry is corrected with the measurement: the URI these tests bind comes from
src/test/resources/serverpackcreator.properties through application.properties'
own spring.config.import, and processTestResources rewrites only the Java
executable and the tomcat basedir.

DatabaseUriPropertyTest's own URI gets the same parameter so the two @SpringBootTest
property sets stay identical and Spring's context cache still reuses one boot. Its
assertions are unaffected -- a query parameter changes neither host, credentials nor
database, which is what it pins.

TestDatabaseTimeoutTest guards the value against silent removal, reading the
processed file under build/resources/test rather than src. Teeth verified by
mutation: deleting the parameter fails it with "the test URI sets no
serverSelectionTimeoutMS, so every context boot waits out the driver's 30 s default".

Note this makes the driver give up quickly; it does not remove the connection
error from the logs. That would need a real database, which is a separate call.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds de.flapdoodle.embed.mongo.spring4x, which starts a real mongod in the test
JVM. No Docker -- Docker's MongoDB images refuse to start on Linux kernels 6.19+
(SERVER-121912), which is what made a containerised database unusable here and
left the end-to-end verification outstanding.

H2 was asked about first and is not an option: the driver speaks the MongoDB wire
protocol and ConnectionString accepts only mongodb:// and mongodb+srv://. That is
precisely why the JPA-era `spring.data.mongodb.uri=jdbc:h2:mem:testdb` line this
module's CLAUDE.md records was a hard startup failure rather than a fallback.

WebPersistenceIT asks the five questions no mocked test can answer, all now
verified against a real database rather than against Spring Data's machinery:
  - the sha256 index really is created by DeclaredIndexCreator on
    ApplicationReadyEvent, so the upload duplicate-check is not a collection scan;
  - an upload round-trips through GridFS *and* the filesystem, reports its size as
    the archive's real byte count, and delete() reclaims BOTH copies -- the leak
    this pass closed, now proven rather than argued;
  - a refused duplicate writes no GridFS document, which is the validate-before-store
    change proven end to end;
  - the RunConfiguration migration flattens a legacy DBRef array and leaves an
    already-migrated document alone;
  - a modPackDownload row written before the @MongoId change -- timestamp as _id,
    no downloadedAt field at all -- still reads back. That one was a real risk this
    pass introduced and could not be settled any other way.

EmbeddedMongoAvailable skips a class, rather than failing it, where mongod cannot
start: CI runs on ubuntu-latest and the kernel is not ours to pin. Verified by
mutation -- forcing the probe to throw reports 11 skipped and a green build, not a
red one. The KDoc says plainly that a skipped guard proves nothing, because a CI
run that skips these has no database coverage and only the skip message says so.

LANDMINE, found by this commit breaking four tests: flapdoodle's
EmbeddedMongoAutoConfiguration activates for EVERY Spring context on the test
classpath and throws "Set the de.flapdoodle.mongodb.embedded.version property"
when it is absent. The dependency is not inert -- every @SpringBootTest must opt in
or opt out. DatabaseUriPropertyTest and DeclaredIndexStartupTest opt out, and must:
the first asserts that the *configured* URI reaches the driver, which an embedded
server overrides (measured -- mongod bound port 56242 while the configured URI still
said 27017 and the write went to 56242), and the second is literally named
theContextStartsWithoutADatabase.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WebServiceContextTest now starts with a real mongod rather than none. Measured on
:serverpackcreator-app:test:

  WebServiceContextTest  60.37s -> 0.99s   (and now WITH a database)
  app suite wall         147.6s -> 28.2s
  220 tests, 0 failures

Its bean-wiring coverage is unchanged; what is added is that the two
ApplicationReadyEvent listeners -- DeclaredIndexCreator and the migration runner --
are actually exercised instead of merely constructed, and that startup has to work
rather than merely survive a database that is not there. Its property set is kept
identical to WebPersistenceIT's so Spring's context cache serves both from one
boot: measured, WebPersistenceIT then runs in 0.37s, which is the cache hit.

Also drops two `.filter { it.downloadedAt != null }` calls that Kotlin now flags as
always-true. They were redundant once downloadedAt stopped being the @MongoId, and
the reason it is safe to remove them rather than make the field nullable is
measured, not assumed: WebPersistenceIT inserts a row in the old shape, with no
downloadedAt field at all, and it still materialises.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three things a reader needs before touching these tests: the dependency is not
inert (flapdoodle's autoconfiguration activates for every context and throws
without a version property, so each @SpringBootTest opts in or out); two classes
must stay opted out for reasons that are measured rather than stylistic; and a
skipped guard proves nothing, which matters because the CI gate is a skip.

Also replaces the H2 note with the short version of why it cannot work at all,
rather than why it is a bad idea.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
app 214 -> 220 (five integration tests against a real MongoDB, plus the timeout
guard), re-derived from build/test-results after a clean run.

REFACTOR-LOG gains the follow-up: why H2 cannot work, why the connection errors
turned out to be 122.6s of a 147.6s suite rather than noise, and the three things
worth carrying forward -- a dependency that changes every context that does not
use it, the fact that embedding a server voids any test whose subject IS the
connection, and the migration question that only a real database could settle.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deleting the false "zip-slip and archive-safety checks applied before an upload
is trusted" comment left the companion undocumented, which dokka reports and
which this project's own convention forbids. Caught by the full build:
`dokkaGeneratePublicationJavadoc` named it alongside two pre-existing ones.

Replaced with what the companion actually is, including why the old claim was
wrong -- archive traversal is rejected by zip4j during extraction, a fact about a
dependency rather than something this file does. The logger's own doc gets the
same treatment: it said "logger for rejected archives", and nothing here rejects
an archive.

ServerPackManifest.Companion and ServerPackProvisioner.Companion are still
undocumented. Both predate this work and neither file was touched by it, so they
are left alone rather than swept in.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dokkaGeneratePublicationJavadoc now reports no Undocumented items for -api.

Both companions are the same kind of thing, and the docs say which: the one place
a file name is spelled. ServerPackManifest's is load-bearing rather than tidy --
the manifest's presence is what tells ServerPackUpdater a generation is an update
rather than a first run, and that decision governs what may be overwritten and
what must be preserved. ServerPackProvisioner's holds the two run-files that have
no template to be named after, read back out through serverRunFileNames, which
the manifest and the ZIP exclusion both consume.

Neither restates its members; the per-member docs already do that.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
44 commits. Every fix has its guard committed red in the commit before it, with
the failure message quoted; where a guard could not be committed red -- because
expressing it needed a constructor the change itself introduced -- teeth were
verified by mutation and the mutation is quoted instead.

web/storage and web/scheduling had no test source at all and ModPackControllerTest
mocks its service, so everything below ModPackService was executed by nothing.
That is the single fact the pass turns on: the defects were not subtle, they were
unobserved.

What was fixed, worst first: the generation queue died permanently on the first
exception a task threw; the Nekodetector scan never ran for a ZIP, i.e. for the
entire web service, while logging that it did; uploads were stored before being
validated, so a rejected one cost the same three writes as an accepted one and
its GridFS copy could never be reclaimed; server-icon detection worked only on
modpacks WITHOUT a manifest, so every CurseForge and MultiMC export silently lost
it; a complete Modrinth parser was reachable from nothing; and any pack under
1 MiB was undownloadable, because size was truncated mebibytes and both tables
gated their download button on size > 0.

Two findings changed on contact with evidence and are recorded as such: the
upload filename was NOT an exploitable arbitrary write (the "-orig-" prefix makes
the first path component a directory that does not exist, so the open fails with
ENOENT -- measured against a real Tomcat), and the GridFS _id query was NOT broken
(MongoConverter.convertId converts a valid 24-hex String). Both had real defects
beside them, which is why they still needed fixing.

The suite then got a real database. H2 cannot work -- the driver speaks the
MongoDB wire protocol -- but flapdoodle runs a real mongod in-process, no Docker,
which closed the end-to-end verification Docker's kernel incompatibility had
blocked. Bounding the server-selection timeout took the app suite from 147.6s to
28.2s, and WebPersistenceIT now proves against a real server what was previously
only argued: the index exists, an upload round-trips both storage tiers, delete
reclaims both copies, and a refused duplicate writes none.

Full build green: 1986 JVM tests across six modules, 0 failures, plus 35 frontend
tests. Deferred with reasons in BACKLOG as B41 and B42.
Read-only audit of 4be89ab67..develop, appended as a dated section.

Two HIGH, both on published -api and both missed at the time:

- FileUtilities.unzipArchive gained @Throws(IOException::class), which javap
  confirms emits `throws java.io.IOException` -- source-INCOMPATIBLE for Java
  callers, against the "source-compatible within a major version" policy. Its own
  behaviour-change row claims the opposite.
- StringUtilities.pathSecureText feeds ServerPackHandler:189, which names the
  server pack DIRECTORY. Fixing its trailing-dot trim therefore renames the
  directory for any pack ending in a dot or space -- and ServerPackUpdater decides
  update-vs-first-run from a manifest inside that directory, so such a pack would
  be regenerated fresh instead of updated, with its world and ops.json not
  preserved. No behaviour row was written.

Five MEDIUM, the substantive one being that four of seventeen fixes shipped with
no guard, including three frontend defects in a module that has a Vitest harness.

Also records what was verified clean so it is not re-litigated: no new !!, no
GlobalScope, module boundaries intact, both refactor: commits genuinely
behaviour-preserving and touching no tests, characterization landed before the
rework, and thirteen of seventeen fixes have their red pin in the commit before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read-only companion to the same-day REFACTOR-AUDIT section.

Two HIGH. A guard added in this pass now asserts nothing: the C1 fix reduces an
upload filename to its base name, so the fixture "sub/dir/pack.zip" that used to
make transferTo fail now simply succeeds, and assertDoesNotThrow passes because
nothing went wrong. Second instance of that failure mode in one pass. And
ModPackController:71 interpolates the client-supplied filename into a quoted
Content-Disposition header unescaped -- on the line directly above one this pass
edited.

Four MEDIUM, the sharpest being that the embedded mongod version is spelled three
ways with the constant that looks authoritative being dead: the probe that decides
whether to skip starts Version.Main.V8_0 while the tests download "8.0.5" from
their annotations.

Records what is verified clean -- no unused imports across 40+ changed files, the
land/store split asserted on real directory contents rather than mock
interactions, and the QueryMapper guard that asks the framework rather than
asserting a belief about it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audit finding H-A. The extraction fix carried @Throws(IOException::class), and
javap showed what that actually emits:

  public final void unzipArchive(java.lang.String, java.lang.String) throws java.io.IOException;

Every existing Java caller of that published method would have stopped compiling
-- a source-incompatible change inside a major version, against the policy in the
root CLAUDE.md. The annotation is removed; javap now shows the original signature.
Kotlin has no checked exceptions, so the exception still propagates and the fix is
unchanged: ModpackExtractionFailureTest and FileUtilitiesTest both stay green.

Its behaviour-change row claimed "this is the one place a compile is unaffected",
which was true for Kotlin and false for Java. Corrected in place rather than
appended, with the javap output, since a wrong row is worse than none -- it is the
artefact a future reader trusts.

Two rows added for changes this pass made and did not record (findings H-B, M-D):
pathSecureText, which renames a generated server pack's DIRECTORY for any pack
whose name ends in a dot or space -- and ServerPackUpdater decides update-vs-first
-run from a manifest inside that directory, so such a pack is regenerated fresh
instead of updated, with its world and ops.json left behind in the old one; and
ModpackZipInspector's error text, which reaches API callers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED, and the failure shows the injection working:

  the name injected a second filename parameter:
    attachment; filename="evil".zip"; filename="other.exe"

ModPack.name is the upload's own filename, kept verbatim on purpose -- landingName
strips path separators and nothing else. Both download handlers interpolate it
into a quoted Content-Disposition value, so a `"` closes the string early and a
`;` appends a parameter. The route has no authentication and @CrossOrigin("*").
Tomcat rejects CR/LF, so this is header-parameter injection rather than response
splitting.

Analysis finding A-2, and notable because the line directly below it was edited by
this pass when the body switched to FileSystemResource; the header was read past.

The second guard is green and closes analysis finding A-4: ArchiveStreamingTest
asserted the body's TYPE but never that the bytes served are the archive's or that
contentLength is right, so a streaming change serving the wrong file would have
passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Analysis finding A-2. Both download handlers now build the header with
ContentDisposition, which escapes quotes in the value; ModPackController's name is
the upload's own filename and reached the header verbatim.

The guard took three attempts and the first two were wrong in the way this
codebase keeps recording, so the detail is worth keeping:

1. Counting occurrences of "filename" cannot work -- the escaped value legitimately
   contains the literal text `filename=` inside itself.
2. Parsing the header back and comparing to the original name ALSO cannot work.
   Measured: Spring's own ContentDisposition.parse is lenient and returns
   `evil".zip"; filename="other.exe` from BOTH the escaped and the unescaped
   header, so the assertion passed against the exact injection it existed to
   catch. Mutation caught it -- reverting the fix left the test green.
3. What actually differs is the bytes on the wire, so the assertion is now on the
   raw header: remove the escaped pairs and exactly two quote characters must
   remain, the opening and the closing.

Teeth re-verified after the rewrite: restoring the interpolation fails with
"the value carries unescaped quotes ... attachment; filename="evil".zip"; fi".

ServerPackController's name is generated rather than user-supplied, but it is
built the same way -- two headers should not differ in how carefully they are
made. The plain quoted form is used rather than the UTF-8 overload, which also
emits a filename* parameter and changes the published header for no benefit here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Analysis findings A-1, A-3, A-5.

anUploadThatCannotBeWrittenIsReportedAsAnEmptyResultRatherThanThrowing was a real
guard when written -- "sub/dir/pack.zip" made transferTo fail on the unsanitised
path, measured red. Then the fix it guards reduced that name to "pack.zip" and the
write started succeeding, so assertDoesNotThrow passed because nothing went wrong.
It asserted nothing while carrying a comment claiming it covered the unhandled-500
path, and a stale one at that: it cited include-stacktrace=ALWAYS, which a later
commit in the same pass set to NEVER.

The failure is now produced by making the storage ROOT a regular file, so it does
not depend on the filename at all, and the guard additionally asserts the result is
empty rather than merely non-throwing. Teeth verified by mutation: letting
IOException escape land() fails it.

The embedded mongod version was spelled three ways -- a dead MONGOD_VERSION
constant, "8.0.5" in two annotations, and Version.Main.V8_0 in the probe -- so the
server the probe validated and the server the tests ran were not tied together. Now
one const, referenced by both annotations through VERSION_PROPERTY and derived by
the probe ("V" + MONGOD_VERSION.replace('.', '_')), which is flapdoodle's own
spelling of the same number.

WebPersistenceIT still runs 5 tests, 0 failures, 0 skipped -- i.e. the probe still
starts a real mongod rather than silently skipping.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Analysis finding A-4, two of the four gaps.

The upload endpoint's `consumes = MULTIPART_FORM_DATA_VALUE` shipped without a
guard; a JSON body now has to be refused with 415, and one MockMvc case pins it.

The two nightly sweeps were the only destructive code in the module and were
covered by mocked repositories alone. WebPersistenceIT now runs the real file
sweep against real data: an orphaned pack loses BOTH tiers, and a repository that
reports nothing at all makes the sweep refuse rather than wipe the directory.

The first version of the orphan test failed, and the reason is the point of having
it. It deleted the only modPack row, which left the repository empty -- and the
empty-repository refusal then (correctly) stopped the sweep from doing anything,
so the GridFS twin survived and the assertion read as a leak. Two behaviours this
pass added interact, and nothing mocked could have shown it: the mocked schedule
tests stub findAll() per case and never see one case's premise invalidate the
other's. The guard now keeps a second pack alive so the repository is non-empty,
and asserts that pack is left alone.

WebPersistenceIT 7 tests, 0 failures, 0 skipped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audit finding M-A / analysis A-4: three frontend defects were fixed without a
guard in a module that has a Vitest harness.

Both guards mutation-verified, and the second one needed strengthening before it
bit. Reverting the field casing (`response.data.modPackId` back to `modPackID`)
fails the first immediately. Reverting `error.response?.data?.modPackId` back to
`error.data.modPackID` did NOT fail the second: the TypeError happens inside an
unawaited .catch(), so it becomes an unhandled rejection and simply leaves the
fields untouched -- which is exactly what the assertion was checking for. The
guard now also spies resetForm, which runs after the assignments and therefore
only happens if the handler got through them. Re-mutated: it fails.

That is the third guard in this session to pass for the wrong reason, all three
found by mutation rather than by reading.

Frontend suite 37 tests, 0 failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Analysis finding A-6. One stored id appears as two entries under the modpacks
root -- "<id>.zip" and the "<id>" directory it was extracted into -- and both
match the ObjectId pattern after removeSuffix(".zip"). deleteStored removes the
archive, the directory and the GridFS document in one call, so the second entry
repeated the whole thing, including a Mongo round-trip that could only match
nothing.

Harmless, since delete is idempotent, but it made the sweep's cost per-entry
rather than per-pack. A set of reclaimed ids fixes it.

Teeth verified by mutation: removing the set fails
anOrphanWithBothAnArchiveAndAnExtractedDirectoryIsReclaimedOnce.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last Undocumented item in the whole build; dokka now reports zero across both
-api and -app. Pre-existing rather than from this pass -- last touched by
"feat(app): implement ConsolePrompt" -- but it is the same one-line omission just
closed twice in -api, and leaving one behind makes the next reader assume the
warning is normal.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second audit round. The first round removed @Throws(IOException::class) to restore
Java source compatibility, but pinned nothing -- and that is the dangerous shape:
the behaviour the annotation was added for works without it, so every functional
guard stayed green while the published Java signature changed. A tidy-up re-adding
it would go unnoticed exactly as it did the first time.

Asserted through reflection on the emitted method rather than by reading the
source. Teeth verified by mutation: re-adding the annotation fails with
"unzipArchive declares [IOException] to Java; a checked exception here breaks
every existing Java caller of a published method".

Also re-measures the counts both CLAUDE.md files carry, rather than incrementing
them: api 473 -> 474, app 220 -> 226, web-frontend 35 -> 37, and the app module's
"all 214 tests still green" now says what it was at the time and what it is now.
Stale snapshots in prose are the defect class this repo has recorded three audits
running.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audit finding M-E. SavedFile, ModPack and ServerPack live in
serverpackcreator-app, which is not published to Maven Central, so the row sat in
a file whose own header scopes it to "what an exported serverpackcreator-api call
does". A reader scanning for plugin or embedder impact would have counted it.

Kept in this file rather than moved, because the field genuinely is part of a
published REST response shape and this is where a consumer-impact question gets
asked -- but the row now says which kind of consumer it means, in its first words
and again at the end.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs: record how each audit and analysis finding was closed
All checks were successful
Documentation / Writerside webhelp (push) Successful in 1m40s
Details
Continuous / Build JAR (push) Successful in 20m2s
Details
Docker Test / build image (push) Successful in 20m44s
Details
Qodana / scan (push) Successful in 21m34s
Details
Continuous / Build AppImage (x86_64) (push) Successful in 1m51s
Details
Continuous / Build AppImage (aarch64) (push) Successful in 1m57s
Details
Documentation / Help image (push) Successful in 6m39s
Details
Qodana / notify (push) Successful in 59s
Details
Continuous / Build Install4J Media (push) Successful in 7m46s
Details
Continuous / Continuous Pre-Release (push) Successful in 5m20s
Details
Test / build (push) Successful in 27m44s
Details
d2a2c46924
 
Both reports listed findings as open that are now fixed. Resolution tables added
in the format REFACTOR-AUDIT already uses, naming the commit and the evidence per
finding rather than just "done".

Four are deliberately NOT fixed and say why: two commits whose concerns are
bundled are already merged, so relabelling them means force-pushing a shared
branch; reportFailure's partial state save is not worth a second save path on a
row about to be reaped; and the concurrent-upload race is tolerated by design,
with findFirstBySha256's First existing for it.

The transferable part is recorded at the end of the analysis: three guards in this
pass passed for the wrong reason and mutation found all three, reading found none.
Fourth instance of that failure mode in this log.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A fresh compile surfaced a warning I introduced last round:
"Elvis operator (?:) always returns the left operand" on
DatabaseStorageService.load. Read literally it says the H4 fix -- returning an
empty Optional for an id GridFS does not hold -- is dead code.

Settled against a real mongod rather than against annotations, and the first
attempt at that guard was itself wrong: asking Spring's findOne directly from the
test failed with "findOne(...) must not be null", which is Kotlin's intrinsic
check on the TEST's own inferred non-null local, not evidence about load(). Asking
load() itself -- the method the download route actually calls when the filesystem
copy is gone -- returns empty and throws nothing.

So the fix works and the warning is a false positive: Spring does not annotate
findOne @Nullable while its own MongoIterable.first() is, so Kotlin infers
non-null. The local is now typed GridFSFile? explicitly, which is what the runtime
does, and the warning is gone. Leaving it would have left a warning asserting that
the only thing standing between an absent file and an NPE is dead.

WebPersistenceIT 8 tests, 0 failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Behaviour-preserving: scanFindings is populated with the same list errors already
carries, and success still reads errors, so nothing observable changes. Lands the
boundary the next commit's guard needs.

The two answer different questions -- "what is in the pack" versus "did building
it work" -- and today only one field exists for both, which is why a pack
containing an infected mod is indistinguishable from one whose files never got
copied. Defaulted to emptyList() so any caller constructing a generation keeps
compiling.

api suite green, 474 tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Behaviour-preserving; the published run(PackConfig) signature is unchanged and
delegates to an internal overload carrying the scan, defaulted to the real
Nekodetector. Same seam, for the same reason, as scanModpackForInfections in
ConfigurationHandler.

Needed because what run() does with a scan result is otherwise unobservable: every
fixture in the suite is clean, so the scan returns nothing and a test cannot tell
a finding routed into the wrong field from a finding that was never produced. That
is precisely the question the next commit's guard has to ask.

api suite green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED at the handler, and the failure names the defect:

  a scan finding was reported as a generation failure; errors was
    [Nekodetector infections found!, Stage 1 infections:, evil.jar]

ServerPackHandler.run populates `errors` from exactly one thing -- the
Nekodetector scan of the FINISHED pack -- and ServerPackGeneration.success is
errors.isEmpty(). So `success` means "no malware found", not "the generation
worked", and it is wrong in both directions:

- a pack that built perfectly and contains an infected mod reports failure;
- a copy that threw, a script that could not be written, a modloader server that
  did not install and a ZIP that was not created are all logged and nothing more,
  so they report success. Even serverPack.create swallows its IOException with a
  comment saying a real failure "would surface later when files are written" --
  it does not, because nothing checks.

Six call sites branch on it: the web queue (GENERATED vs ERROR), both CLI verbs,
the GUI control panel and the grinder's VanillaPackGenerator.

ServerPackGenerationOutcomeTest pins the contract at the type, which is what those
six rely on; it is green already, because the previous commit's seam made the type
capable of expressing it. The handler guard is the one that was red.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns aScanFindingDoesNotMakeAGoodGenerationReportFailure green.
api 478, app 227, grinder 544 -- all 0 failures.

ServerPackGeneration.errors was populated by exactly one thing: Nekodetector's
scan of the FINISHED pack. Since success is errors.isEmpty(), it meant "no malware
found", and it was wrong in both directions:

- a copy that threw, a script that could not be written, a modloader server that
  did not install and a ZIP that was requested and never produced are all logged
  and nothing more, so they reported SUCCESS;
- a pack that built perfectly and contained an infected mod reported FAILURE.

errors now carries generation failures and scanFindings carries the scan output.
run() additionally records two failures it previously swallowed: a server pack
directory it could not create -- whose comment claimed a real failure "would
surface later when files are written", which nothing checked -- and a ZIP asked
for and not delivered, previously indistinguishable from one never asked for.

All six consumers updated, because the fix alone would have SILENCED the malware
reporting rather than fixing it: the web queue records findings as a QueueEvent
against a still-GENERATED pack instead of flipping it to ERROR, both CLI verbs
print them under their own heading whether or not the build worked, the GUI logs
them at WARN so they reach the log window, and the grinder logs them.

RunHeadlessCommandTest's mock gains a scanFindings answer. An existing test
changing is the stop-and-flag signal for a refactor; this is a fix, and the mock
had to learn a question that did not exist before.

Recorded in claude-docs/API-BEHAVIOUR-CHANGES.md: an embedder printing errors as
"infections found" must move to scanFindings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RED, and the message is the defect:

  the exclusion-regex failure was reported as
    [Invalid inclusion-regex specified: [unclosed.], which names the wrong field

InclusionsValidator validates both filters and reported both with
configuration.log.error.checkcopydirs.inclusion, so a malformed exclusion-regex
told the user their INCLUSION filter was wrong -- the field they did not touch.
The dedicated configuration.log.error.checkcopydirs.exclusion key exists in all
three locale files and is referenced by nothing.

The inclusion case is green and stays, so the two discriminate: a fix that simply
swapped the keys would fail it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns aBadExclusionFilterIsReportedAsAnExclusionProblem green; the inclusion
discriminator stays green.

configuration.log.error.checkcopydirs.exclusion exists in Translations_en_GB,
_pt_BR and _zn_GB and was referenced by nothing, while a malformed exclusion-regex
was reported with the inclusion key -- telling the user the field they did not
touch was wrong.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
suggestInclusions guarded listFiles() with `assert(... != null)` and then `!!`,
inside a try that caught the resulting NullPointerException and logged a message
about "copy dirs should never be empty" -- which describes a different problem.

Two things were wrong with that, and the second is the interesting one:

- The caller received an empty list, and isZip merges that into the pack's
  inclusions, so a modpack directory that could not be read produced a server pack
  with no directories in it and said nothing useful about why.
- Java assertions are DISABLED at runtime by default and ENABLED by Gradle for
  test tasks, so the method behaved differently for users than for tests. Measured
  by mutation: restoring the old implementation fails both new guards with
  "java.lang.AssertionError: Assertion failed" -- an error no user would ever have
  seen, while the empty list they did get was invisible to any test.

Now an explicit null check, an accurate message naming the directory, and an early
return. The guards were written after the fix, so their teeth were verified by the
mutation above rather than by a red commit.

Also replaces an `indices` loop that called toList() on every iteration with a
direct iteration over the same collection.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs(api): say plainly that checkForInvalidPathCharacters returns true when CLEAN
Some checks failed
Documentation / Writerside webhelp (push) Successful in 1m46s
Details
Continuous / Build JAR (push) Successful in 14m16s
Details
Qodana / scan (push) Successful in 14m23s
Details
Continuous / Build AppImage (x86_64) (push) Has been cancelled
Details
Continuous / Build AppImage (aarch64) (push) Has been cancelled
Details
Continuous / Build Install4J Media (push) Has been cancelled
Details
Continuous / Continuous Pre-Release (push) Has been cancelled
Details
Docker Test / build image (push) Has been cancelled
Details
Qodana / notify (push) Has been cancelled
Details
Documentation / Help image (push) Has been cancelled
Details
Test / build (push) Has been cancelled
Details
9a088d559d
 
The name reads as a predicate for "has invalid characters" and means the exact
opposite; only the @return tag two dozen lines down said so. A reader reaching for
it, or a refactor tidying a call site, would invert the condition and the result
would be a plausible value rather than an error -- the failure mode this repo has
recorded for version parsing and shell templates.

Not renamed: it is published API and renaming inside a major version breaks
embedders. The in-repo caller (InclusionsValidator) negates it correctly and
StringUtilitiesTest pins both directions, so the risk is to future readers, which
is what a doc fixes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fix(build): publish kotlin-stdlib with a version, unbreaking the Maven release
All checks were successful
Documentation / Writerside webhelp (push) Successful in 1m45s
Details
Continuous / Build JAR (push) Successful in 18m42s
Details
Docker Test / build image (push) Successful in 18m6s
Details
Qodana / scan (push) Successful in 15m28s
Details
Docker Test / build image (pull_request) Successful in 15m36s
Details
Documentation / Help image (push) Successful in 2m48s
Details
Continuous / Build AppImage (x86_64) (push) Successful in 1m49s
Details
Continuous / Build AppImage (aarch64) (push) Successful in 2m22s
Details
Test / build (push) Successful in 28m34s
Details
Qodana / notify (push) Successful in 26s
Details
Continuous / Build Install4J Media (push) Successful in 9m27s
Details
Test / build (pull_request) Successful in 31m22s
Details
Continuous / Continuous Pre-Release (push) Successful in 3m50s
Details
43dc17400f
 
The 9.0.0-beta.2 release failed at :closeSonatypeStagingRepository. Every other
job succeeded -- signMavenJavaPublication and both repository publishes ran -- and
Sonatype then rejected the staging repository with HTTP 400:

  pkg:maven/de.griefed.serverpackcreator/serverpackcreator-api@9.0.0-beta.2
    - Dependency version information is missing for dependency: org.jetbrains.kotlin:kotlin-stdlib
    - Dependency management dependency version information is missing for dependency: org.jetbrains.kotlin:kotlin-bom

Reproduced locally from the generated POM, which carried exactly those two: a
dependencyManagement BOM import with no <version>, and a runtime kotlin-stdlib
with no <version>.

Both came from serverpackcreator.kotlin-conventions declaring them as bare
coordinates. A precompiled script plugin cannot read the version catalog, so bare
is the only thing that compiles there and the version was left to the BOM -- which
itself had none. They were redundant anyway: kotlin("jvm") adds a stdlib at the
plugin's own version (kotlin.stdlib.default.dependency is unset, so it defaults to
true), and a module wanting it explicitly uses libs.kotlinStdlib.

Measured on :serverpackcreator-api:generatePomFileForMavenJavaPublication, which is
how build logic gets verified here:

  dependencies without a <version>   2 -> 0
  <dependencyManagement> block       present -> absent
  kotlin-stdlib                      MISSING -> 2.4.10, scope runtime
  total <dependency> entries         20 -> 17

So consumers still get the stdlib; it simply has a version now. Full build green,
2003 JVM tests, 0 failures.

spring-conventions still declares versionless dependencies that resolve through
Boot's BOM. Left alone deliberately -- that is the documented pattern there and
-app is not published -- but recorded in .claude/rules/build-layout.md as the
thing to audit before any second module is ever published, together with the
one-command check that would have caught this before the release.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
accepted
Feature Ask which got accepted.

  
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
docker
Docker dependency updates

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature, request or improvement

  
github-actions
GitHub-Actions dependency updates

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
gradle
Gradle dependency updates

  
hacktoberfest-accepted

   
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
javascript
Pull requests that update Javascript code

  
not-an-issue
Not a problem, rather something for documentation purposes

  
npm
NPM dependency updates

  
question
Further information is requested

  
rejected
Feature Ask which got rejected.

  
wontfix
This will not be worked on

  
Working on it
Indicates that work is being done on this issue/pr

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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
Griefed/ServerPackCreator!677
No description provided.