diff --git a/AGENTS.md b/AGENTS.md index f1a213f..18f8f02 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,12 +46,19 @@ permission on PRs from forks by default in a public repo. ### Entry point and Minecraft hooks -`BlueMapSignMarkersMod` (`DedicatedServerModInitializer`) wires three lifecycle hooks: -- `SERVER_STARTING` → `SignProvider.loadSigns(...)` reads the world's persisted `signs.json` +`BlueMapSignMarkersMod` (`DedicatedServerModInitializer`) wires four lifecycle hooks: +- `SERVER_STARTING` → `SignProvider.loadSigns(...)` reads the world's persisted, region-sharded sign storage + (migrating a pre-sharding single `signs.json` on first boot after an upgrade) - `SERVER_STOPPING` → `SignProvider.saveSigns(...)` then `SignManager.stop()` - `BLOCK_ENTITY_LOAD` → for any loaded `SignBlockEntity`, calls `SignManager.addOrUpdate(...)` +- `CHUNK_LOAD` → reconciles signs the mod's cache still knows about in a loading chunk against what's actually + there; if a tracked sign's block is gone (e.g. its region file was deleted/regenerated externally while + unloaded), calls `SignManager.remove(...)` for it. Addresses GitHub issue #110. -The markers file path is per-world: `config/bluemapsignmarkers//signs.json`. +Sign state is stored per-world, region-sharded: one JSON file per (dimension, 32x32-chunk region) under +`{server_root}/bluemapsignmarkers/{level}/{dimension_namespace}/{dimension_path}/r.{regionX}.{regionZ}.json`. A +pre-sharding single `config/bluemapsignmarkers//signs.json` is migrated in place on first boot +after upgrading (backed up, not deleted). Two Mixins (`src/main/resources/bluemapsignmarkers.mixins.json`) catch the events the lifecycle hooks above can't: - `SignBlockEntityInject` injects into `SignBlockEntity.updateSignText` (a player edits a sign) → `SignManager.addOrUpdate` @@ -62,13 +69,16 @@ Two Mixins (`src/main/resources/bluemapsignmarkers.mixins.json`) catch the event 1. **`SignHelper`** builds a `SignEntry` (immutable snapshot: position/dimension key, player id, parsed front/back text) from a `SignBlockEntity`, running sign text through a `SignLinesParser` configured with the current - `MarkerGroup`s. + `MarkerGroup`s. `SignHelper.reloadParser()` rebuilds that parser from the current config; it's called on every + config reload (see below) so a sign parsed after `/bluemap reload` picks up an edited prefix/matchType. 2. **`SignManager`** (singleton, holds a `ConcurrentMap` cache of all known signs) is the decision point: given a new/updated `SignEntry`, it figures out whether this is an add, update, remove, or prefix-change (remove-then-add into a different marker group) relative to what's cached, then dispatches the corresponding `MarkerAction` (`AddMarkerAction`/`UpdateMarkerAction`/`RemoveMarkerAction`, built via - `ActionFactory`) to the BlueMap connector. It also implements `IResetHandler.reset()` to replay the whole sign - cache when BlueMap resets its state. + `ActionFactory`) to the BlueMap connector. It also implements `IResetHandler.reset()`, which BlueMap fires on + `/bluemap reload`: reloads config (`ConfigManager.reload()`, `SignHelper.reloadParser()`, rebuilding its + prefix→group lookup and `ActionFactory`) then replays the whole sign cache through the same add/update/remove + logic, so an edited marker-group's icon/offset/visibility/prefix takes effect without a server restart. 3. **`BlueMapAPIConnector`** owns the `ReactiveQueue` and all actual BlueMap API calls. Because the BlueMap API is only available while BlueMap itself is enabled, actions are queued and only drained (`markerActionQueue.process()`) while `BlueMapAPI.getInstance().isPresent()`; `BlueMapAPI.onEnable`/`onDisable` @@ -90,12 +100,17 @@ line to match the pattern (unlike `STARTS_WITH`, a regex prefix can't share its ### Sign persistence and versioning -Sign state is persisted per-world as `signs.json`, wrapped in a `VersionedSignFile` envelope (`{version, data}`) so -the format can evolve without breaking old saves. `SignProvider.loadSigns` tries, in order: the versioned-file loader -(`VersionedFileSignEntryLoader`, handling V2→V3 migration via `Version3Converter`, and current V3 files directly), -then falls back to `Version1SignEntryLoader` for pre-versioning files. When adding a new persisted field, bump +Sign state is stored per-world, region-sharded (one file per dimension + 32x32-chunk region — see "Entry point" +above), with each region file wrapped in a `VersionedSignFile` envelope (`{version, data}`) so the format can evolve +without breaking old saves. `SignProvider.loadSigns` checks whether the storage root already has region files +(`RegionShardedSignEntryLoader.hasSignData`); if so, it loads every region file the same version-aware way as +before sharding — the versioned-file loader (`VersionedFileSignEntryLoader`, handling V2→V3 migration via +`Version3Converter`, and current V3 files directly), falling back to `Version1SignEntryLoader` for pre-versioning +files. If no region files exist yet, `LegacySignFileMigrator` reads a pre-sharding single `signs.json` (if present) +through that same version chain, writes it out region-sharded, and backs up the legacy file (renamed, not deleted) +only once every expected region file is confirmed on disk. When adding a new persisted field, bump `SignFileVersions` and add a loader/converter rather than changing an existing version's shape in place — old -`signs.json` files on live servers must keep loading. +region files (or a not-yet-migrated legacy `signs.json`) on live servers must keep loading. ### Adding a new marker/BlueMap action @@ -107,15 +122,17 @@ compile. ### Testable vs. game-coupled code When adding logic, prefer keeping it in plain Java classes with no Minecraft/Fabric/BlueMap API types in their -signature (like `SignLinesParser`, `SignEntryHelper`, `MarkerGroup`/`MarkerGroupMatchType`, `ConfigManager`/ -`ConfigProvider`, `ReactiveQueue`, the persistence loaders/converters, `ActionFactory`/`MarkerSetIdentifierCollection`) +signature (like `SignLinesParser`/`ParsingContext`, `SignEntry`/`SignEntryHelper`, `SignChunkKey`/`SignChunkIndex`, +`MarkerGroup`/`MarkerGroupMatchType`, `ConfigManager`/`ConfigProvider`, `ReactiveQueue`, `HtmlUtils`, `FileUtils`, +the persistence loaders/converters (including `Version1SignEntryLoader`), `ActionFactory`/`MarkerSetIdentifierCollection`) — these can be unit tested directly (see `src/test/java/.../core/signs/SignLinesParserTest.java` for the pattern). Code that must reference game types (`SignHelper`, the mixins, `BlueMapSignMarkersMod`, `BlueMapAPIConnector`) should stay thin glue around the testable core, since it can only be verified manually via `runServer`. -There is a documented, currently-unfixed gap here: `BlueMapAPIConnector` passes sign text into BlueMap's POI marker -`detail` field unescaped, and BlueMap renders `detail` as raw HTML (unlike `label`, which BlueMap escapes itself) — -see `plans/html-detail-escaping-plan.md` for the planned fix. +`BlueMapAPIConnector` escapes sign text (`HtmlUtils.toHtmlDetail`, in `common`) before it reaches BlueMap's POI +marker `detail` field — BlueMap renders `detail` as raw HTML (unlike `label`, which BlueMap escapes itself), and +sign text is player-controlled, so this closes a live XSS vector. See `plans/html-detail-escaping-plan.md` for the +design. Persisted sign data stays raw/unescaped; escaping happens only at this BlueMap API call site. ## Planning documents diff --git a/agent-context/README.md b/agent-context/README.md new file mode 100644 index 0000000..f710361 --- /dev/null +++ b/agent-context/README.md @@ -0,0 +1,36 @@ +# Agent Context — BlueMap Sign Markers + +BlueMap Sign Markers is a server-side Fabric mod (Java 25) for Minecraft. It watches in-game signs and, when a +sign's text matches a configured prefix (e.g. `[poi]`), creates/updates/removes a corresponding marker on a +[BlueMap](https://bluemap.bluecolored.de/) map. Signs are tracked persistently, sharded into one JSON file per +(dimension, 32x32-chunk region) under `{server_root}/bluemapsignmarkers/{level}/`, so markers survive restarts. +Multiple "marker groups" (prefix, match rule, icon, visibility) can be configured at once via +`config/bluemapsignmarkers/BMSM-Core.json`. + +Human-facing docs already exist: +- `README.md` (repo root) — end-user install/config instructions and the marker-group config format +- `AGENTS.md` / `CLAUDE.md` (repo root) — AI agent guidance (build commands, high-level architecture, conventions) + +This `agent-context/` directory goes deeper than `AGENTS.md` on implementation specifics — exact decision logic, +method-level behavior, data-shape history — so an agent doesn't have to re-derive it from source every time. + +## Documents + +| Document | What it covers | +|----------|----------------| +| `context/architecture.md` | Tech stack, directory map, package taxonomy, build/CI/publish tooling | +| `context/core-pipeline.md` | Sign text → marker action: parsing state machine, `SignManager` decision logic, chunk-load sign reconciliation (`SignChunkKey`/`SignChunkIndex`), `ReactiveQueue`/`BlueMapAPIConnector` mechanics, marker ID/set identity scheme | +| `context/config-and-persistence.md` | Marker-group config loading/migration (V1→V2), region-sharded sign persistence, per-region-file versioning (V1→V2→V3), legacy-file migration and backup-on-migrate behavior | +| `context/testing.md` | Test infra, testable-vs-game-coupled split, CI test-result summarization, current coverage and known gaps | + +## Document Scopes + +| Document | Codebase paths watched | +|----------|------------------------| +| `context/architecture.md` | `build.gradle`, `gradle.properties`, `settings.gradle`, top-level dirs, `.github/workflows/` | +| `context/core-pipeline.md` | `src/main/java/.../core/signs/` (excl. `persistence/`), `core/bluemap/`, `core/reactive/`, `core/markers/`, `mixin/`, `BlueMapSignMarkersMod.java` | +| `context/config-and-persistence.md` | `src/main/java/.../config/`, `core/signs/persistence/`, `README.md` (config format section) | +| `context/testing.md` | `src/test/java/`, `.github/workflows/build.yml`, `.github/workflows/publish.yml` | + +--- +*Last updated: 2026-07-21 | Verified against: 26.2-0.17.0 (72d4280)* diff --git a/agent-context/context/architecture.md b/agent-context/context/architecture.md new file mode 100644 index 0000000..69248c8 --- /dev/null +++ b/agent-context/context/architecture.md @@ -0,0 +1,75 @@ +# Architecture + +## Tech stack + +| Layer | Technology | +|-------|-----------| +| Language | Java 25 (`sourceCompatibility`/`targetCompatibility` = `VERSION_25` in `build.gradle`) | +| Mod platform | Fabric, `DedicatedServerModInitializer` (server-only, no client logic) | +| Build | Gradle wrapper + Fabric Loom `1.17-SNAPSHOT` | +| Target Minecraft | version pinned in `gradle.properties` (`minecraft_version`), currently `26.2` | +| Fabric loader/API | `loader_version` / `fabric_api_version` in `gradle.properties` | +| External integration | BlueMap API `blue_map_api_version` (`compileOnly` — provided by BlueMap itself at runtime), targeting BlueMap `blue_map_version` | +| JSON | Gson, `Strictness.LENIENT` | +| Logging | SLF4J, all loggers named via `Constants.MOD_ID` | +| Testing | JUnit 5 (Jupiter), via `junit-bom:5.11.4` | +| Publish target | Modrinth, via `com.modrinth.minotaur` Gradle plugin | +| CI | GitHub Actions | + +All version numbers live in `gradle.properties` — never hardcode a version in `build.gradle` or source. +`mod_version` follows `-` (e.g. `26.2-0.17.0`). + +## Top-level layout + +``` +src/main/java/com/tpwalke2/bluemapsignmarkers/ mod source +src/main/resources/ fabric.mod.json, mixins config, icon asset +src/test/java/com/tpwalke2/bluemapsignmarkers/ unit tests (plain-Java logic only) +plans/ design/implementation plans, written before larger changes +.github/workflows/ build.yml (CI), publish.yml (manual Modrinth publish) +build.gradle, gradle.properties build config and all version numbers +README.md end-user install + config-format docs +AGENTS.md / CLAUDE.md AI agent guidance (source of truth CLAUDE.md defers to AGENTS.md) +``` + +## Package taxonomy + +| Package | Contents | +|---------|----------| +| `com.tpwalke2.bluemapsignmarkers` (root) | `BlueMapSignMarkersMod` (entrypoint, lifecycle hooks; implements `ServerPathProvider`), `Constants` (`MOD_ID`), `ServerPathProvider` (interface: `getMarkerStorageRoot(MinecraftServer): Path`, resolves `{server_root}/bluemapsignmarkers/{level}`) | +| `common` | `FileUtils` — `createBackup`/`moveToBackup(originalPath, suffix, description)` (copy- vs. rename-based backup, used by config/sign migration code before overwriting or retiring an old-format file); `HtmlUtils` — `escape`/`toHtmlDetail`, escapes player-supplied sign text before it crosses into BlueMap's HTML-rendered marker `detail` field | +| `config` | `ConfigManager` (lazy `volatile` singleton over the loaded config, `reload()` on first null `get()`), `ConfigProvider` (load/save/migrate `BMSM-Core.json`) | +| `config.models` | `BMSMConfigV1` (legacy, single `poiPrefix` field), `BMSMConfigV2` (current runtime shape — `MarkerGroup[]`, ctor also seeds Gson defaults) | +| `config.persistence` | `LoadingBMSMConfigV2` / `LoadingMarkerGroupV2` — a *nullable-boxed-field* mirror of `BMSMConfigV2`/`MarkerGroup` used only for Gson deserialization, so partially-specified user JSON can be detected and defaulted field-by-field in `ConfigProvider.convertToLoadedMarkerGroup` rather than Gson silently zeroing missing primitives | +| `core` | `WorldMap` — just the `UNKNOWN` dimension-key sentinel used when a sign's `Level` is null | +| `core.bluemap` | `BlueMapAPIConnector` (the only class that touches the BlueMap API), `IResetHandler` (callback interface `SignManager` implements to replay its cache on BlueMap reset) | +| `core.bluemap.actions` | `MarkerAction` (abstract base, holds a `MarkerIdentifier`), `AddMarkerAction`/`UpdateMarkerAction`/`RemoveMarkerAction`, `ActionFactory` (builds actions, resolves `MarkerSetIdentifier`s) | +| `core.markers` | `MarkerGroup` (record: the config unit), `MarkerGroupMatchType` (`STARTS_WITH`/`REGEX`), `MarkerGroupType` (currently only `POI`), `MarkerIdentifier`/`MarkerSetIdentifier`/`MarkerSetIdentifierCollection` (marker identity + dedup) | +| `core.reactive` | `ReactiveQueue` (generic buffer-while-unavailable queue) + its three functional-interface callbacks (`ShouldRunCallback`, `MessageProcessorCallback`, `MessageProcessorErrorCallback`) — see `core-pipeline.md` | +| `core.signs` | `SignEntry`/`SignEntryKey` (immutable sign snapshot + position/dimension key), `SignHelper` (game-coupled: builds `SignEntry` from a `SignBlockEntity`), `SignEntryHelper` (plain-Java: derives label/detail/prefix from a `SignEntry`), `SignLinesParser`/`ParsingContext`/`SignLinesParseResult` (plain-Java parsing state machine), `SignManager` (singleton decision point — cache + dispatch), `SignChunkKey`/`SignChunkIndex` (plain-Java chunk-position lookup backing chunk-load sign reconciliation, see `core-pipeline.md` §4) | +| `core.signs.persistence` | `SignProvider` (load/save, dispatches to the region-sharded loader/writer or migration), `SignFileVersions` (`V1`/`V2`/`V3` — per-region-file schema version, unchanged by sharding), `VersionedSignFile` (`{version, data}` envelope), `SignRegionKey` (record: dimension + region coords; `forPosition` does the `floorDiv` region math, `relativeFilePath` builds the `{namespace}/{path}/r.{x}.{z}.json` path), `SignRegionPartitioner` (groups `SignEntry`s by `SignRegionKey`), `RegionShardedSignEntryWriter` (writes each region's file; region files that dropped to zero signs are quarantined with a `.stale` suffix, not deleted, unless any region write failed — in which case quarantine is skipped entirely, to avoid mistaking a load failure for a genuine "signs removed" case), `LegacySignFileMigrator` (one-shot: reads the pre-sharding single file through the existing V1/V2/V3 chain, writes it out sharded, backs up the legacy file only once every expected region file is confirmed written to disk) | +| `core.signs.persistence.loaders` | `VersionedFileSignEntryLoader` (tries versioned envelope; V2 → converts + backs up), `Version1SignEntryLoader` (pre-versioning fallback; also normalizes legacy `nether`/`end`/`overworld` map-id strings to real dimension identifiers — the identifiers are spelled out as literal strings rather than read from `net.minecraft.world.level.Level` statics, so the class has no Minecraft-type dependency and is fully unit-testable), `Version3Converter` (V2 model → current `SignEntry`), `RegionShardedSignEntryLoader` (walks a storage root's directory tree, loads each region file via `VersionedFileSignEntryLoader`, flattens to one list; `hasSignData(Path)` is the migration-trigger check) | +| `core.signs.persistence.models` | `SignEntryV2`, `SignLinesParseResultV2`, `MarkerTypeV2` — frozen shapes representing the on-disk V2 format, kept solely so `Version3Converter` can still read old files; do not evolve these, they must stay byte-compatible with historical `signs.json` | +| `mixin` | `SignBlockEntityInject` (injects `SignBlockEntity.updateSignText` TAIL), `AbstractBlockInject` (injects `BlockBehaviour.affectNeighborsAfterRemoval` HEAD) | + +## Import / module resolution + +Standard Java package imports. Fabric Loom remaps Minecraft/Fabric API artifacts at build time; `build.gradle`'s +`loom { mods { "bluemapsignmarkers" { sourceSet sourceSets.main } } }` registers the main source set as the mod's +dev source for `runServer`/`runClient`. + +## Build / CI / publish tooling + +- `./gradlew build` — compile, run tests, produce jar in `build/libs/`; fails on any test failure +- `./gradlew test` — JUnit 5 suite only (`src/test/java`) +- `./gradlew runServer` / `runClient` — dev Minecraft instance with the mod + BlueMap loaded +- `.github/workflows/build.yml` — runs on push/PR to `main` and `releases/**`: `./gradlew test` → summarize JUnit + XML results to `$GITHUB_STEP_SUMMARY` (plain shell `sed`, no third-party reporter action — see `testing.md` for + why) → `./gradlew build` → upload `build/libs/` as an artifact +- `.github/workflows/publish.yml` — manual `workflow_dispatch` only: runs tests first (failure blocks publish), then + `./gradlew modrinth -PbuildNumber=` (alpha, from a branch dispatch) or the same with `-PisRelease` + (release, from a `v*` tag dispatch) + +--- +*Last updated: 2026-07-21 | Verified against: 26.2-0.17.0 (72d4280)* + diff --git a/agent-context/context/config-and-persistence.md b/agent-context/context/config-and-persistence.md new file mode 100644 index 0000000..225acad --- /dev/null +++ b/agent-context/context/config-and-persistence.md @@ -0,0 +1,135 @@ +# Config loading and sign persistence + +Two independent versioned-JSON subsystems share the same pattern (detect version → migrate → back up old file → +save current format), but are separate code paths: marker-group **config** (`config/`) and sign **state** +(`core/signs/persistence/`). End-user config format is documented in repo-root `README.md`; this doc covers the +loading/migration mechanics, not the format itself. + +## Marker-group config (`config/`) + +File: `config/bluemapsignmarkers/BMSM-Core.json`. Path is fixed (not per-world) — +`ConfigProvider.getConfigPath()` = `Path.of("config", Constants.MOD_ID, "BMSM-Core.json")`. + +- `ConfigManager.get()` returns a `volatile` singleton reference, lazily populated by calling `reload()` on first + access if still `null` (not an eager static-field initializer — that would run the instant anything references + the class, including a test merely loading `ConfigManagerTest`, and write a real config file as a side effect). + **Config is hot-reloadable**: `ConfigManager.reload()` (`synchronized`, public) loads via `ConfigProvider.loadConfig()` + and swaps the reference; `volatile` alone is enough for safe publication to other threads since the new + `BMSMConfigV2` is fully built before the swap. Reload is wired to BlueMap's `/bluemap reload` via + `SignManager.reloadConfig()` — see `core-pipeline.md` §3. A package-private `reload(Path)` overload (and matching + package-private `ConfigProvider.loadConfig(Path)`/`saveConfig(BMSMConfigV2, Path)` overloads, which the public + no-arg methods now delegate to with `getConfigPath()`) exist solely so tests can point loading/saving at a temp + directory instead of the hardcoded `config//BMSM-Core.json` path — no behavior change for real usage. +- `ConfigProvider.loadConfig()`: + 1. If the file doesn't exist: create `new BMSMConfigV2()` (which self-populates the default single `[poi]` + group via its field initializer), save it, return it. + 2. If the raw file content contains the literal substring `"poiPrefix"` (a V1-only field), parse as `BMSMConfigV1`, + migrate via `loadV1Config` (builds a single `MarkerGroup` from `poiPrefix`, defaults for everything else), + back up the old file (`FileUtils.createBackup(path, ".v1.bak", "config file")`), save the migrated V2 config, + return it. + 3. Otherwise parse as `LoadingBMSMConfigV2` (nullable-boxed fields — `config.persistence.LoadingMarkerGroupV2`) + and convert each `LoadingMarkerGroupV2` to a runtime `MarkerGroup` via `convertToLoadedMarkerGroup`, which + applies defaults per-field (`matchType` → `STARTS_WITH`, `type` → `POI`, `offsetX`/`offsetY` → `0`, + `defaultHidden` → `false`, `minDistance` → `0.0`, `maxDistance` → `10000000.0`). This two-model split + (`LoadingMarkerGroupV2` boxed/nullable vs. `MarkerGroup` primitive) exists so a partially-specified group in + user JSON gets these explicit defaults rather than Gson silently zeroing missing primitive fields. + 4. Any exception during load (`Gson.fromJson` failure, I/O error) logs and returns `null`, and + `ConfigManager.loadCoreConfig` falls back to `new BMSMConfigV2()` defaults — a broken config file never + prevents server startup, it just silently reverts to a single default `[poi]` group. +- `saveConfig(config)` creates parent dirs if needed and writes pretty-printed Gson JSON. + +## Sign persistence (`core/signs/persistence/`) + +Storage root is **per-world, region-sharded**: `{server_root}/bluemapsignmarkers/{level}/{dimension_namespace}/ +{dimension_path}/r.{regionX}.{regionZ}.json` — one JSON file per (dimension, 32x32-chunk region), e.g. +`{server_root}/bluemapsignmarkers/world/minecraft/overworld/r.0.0.json`. Design rationale, rejected alternatives +(SQLite, H2, LMDB/MapDB, store-by-player, in-memory-only chunk index), and migration considerations are in +`plans/region-sharded-sign-persistence-plan.md`. This replaced the prior single-file-per-world layout +(`config/bluemapsignmarkers//signs.json`) so a future reconciliation feature (detecting signs whose chunk was +externally deleted/regenerated — GitHub issue #109) can query "signs known in this region" cheaply instead of +scanning every cached sign; that reconciliation logic itself is not yet implemented. + +`ServerPathProvider.getMarkerStorageRoot(server)` (implemented on `BlueMapSignMarkersMod`) resolves the root: +`levelDir = server.getWorldPath(LevelResource.ROOT).toAbsolutePath().normalize()`, server root = +`levelDir.getParent()`, level name = `levelDir.getFileName()`. The `.normalize()` is required because +`LevelResource.ROOT`'s relative path is literally `"."`, which `Path.resolve()` doesn't collapse on its own — +skipping it shifts `getParent()`/`getFileName()` by one level and lands the storage root inside the world save +folder instead of beside it. This also fixed a pre-existing bug where the old path formula's extra `.getParent()` +resolved to the *run directory's* name, not the level name (`plans/codebase-review-2026-07-11.md` finding #1). +`BlueMapSignMarkersMod.getLegacyMarkerFilePath` intentionally keeps the old (buggy) formula unchanged — migration +must locate files at the path they were actually written to, not the corrected one. + +Loaded on `SERVER_STARTING`, saved on `SERVER_STOPPING` (then `SignManager.stop()`). + +Format envelope per region file is unchanged: `VersionedSignFile(SignFileVersions version, String data)` where +`data` is itself a JSON-encoded string of that region's entry array (double-encoded — the envelope is parsed first, +then `data` is parsed again as the entry array). `SignFileVersions` = `V1, V2, V3` (current = `V3`, written by +`RegionShardedSignEntryWriter` unconditionally) — sharding changed how many files exist and where, not the schema +of an individual file. + +`SignRegionKey(dimension, regionX, regionZ)`: `forPosition(dimension, x, z)` computes region coordinates via +`Math.floorDiv(x, 512)`/`Math.floorDiv(z, 512)` (512 = 32 chunks x 16 blocks, matching Minecraft's own Anvil +`.mca` region size — `floorDiv`, not truncating division, so negative coordinates split correctly). +`relativeFilePath()` splits `dimension` (a string like `minecraft:overworld`, or the `WorldMap.UNKNOWN` sentinel +`"unknown"` with no colon) on the first `:` into namespace/path segments, appending `r.{regionX}.{regionZ}.json`. +It rejects (throws `IllegalArgumentException`) a blank/`.`/`..` namespace, or a resolved relative path that's +absolute, starts with `..`, or otherwise escapes the namespace directory after `.normalize()` — a defense against a +maliciously/accidentally crafted dimension id writing outside the storage root. +`SignRegionPartitioner.partition(List)` groups entries into `Map>` using +each entry's `key().parentMap()`/`x()`/`z()` — the shared grouping logic behind both save and migration. + +`SignProvider.loadSigns(storageRoot, legacyPath)`: +1. `RegionShardedSignEntryLoader.hasSignData(storageRoot)` — true if the storage root exists and contains at + least one region file. If true: `RegionShardedSignEntryLoader.loadSignEntries(...)` walks the directory tree + and loads every region file through `VersionedFileSignEntryLoader` (same per-file deserialization as before + sharding), flattening into one list. +2. If false (first boot after upgrading, or a fresh world): `LegacySignFileMigrator.migrate(legacyPath, storageRoot, + groups, gson)` — one-shot: + - If no file exists at `legacyPath`, returns an empty list (fresh install, nothing to migrate). + - Otherwise reads it and runs the **exact same V1/V2/V3 chain as before sharding**, unchanged: + `VersionedFileSignEntryLoader.loadSignEntries(...)`, falling back to `Version1SignEntryLoader.loadSignEntries(...)` + if that returns `null`. Region-sharding is layered *after* this existing version normalization, not a + replacement for it. + - Writes the resulting entries via `RegionShardedSignEntryWriter.write(...)` (see below). Backs up the legacy + file via `FileUtils.moveToBackup(legacyPath, ".migrated", ...)` — **renamed, not deleted** — only after + confirming every region file expected from `SignRegionPartitioner.partition(entryList)` actually exists on + disk (or the entry list was empty to begin with); if any expected region file is missing, the legacy file is + left in place and an error is logged, so a partial/failed migration doesn't lose the only remaining copy of + the data. A successful migration isn't re-attempted on future boots since step 1 will find the new storage + root non-empty from then on. +3. Every resulting `SignEntry` (from either path) is fed through `SignManager.addOrUpdate(...)` — same as before + sharding, loading signs at startup goes through the exact same decision logic as any other sign event (see + `core-pipeline.md` §3). Each entry is wrapped in its own try/catch, so one malformed entry logs an error and is + skipped rather than aborting the load of every other sign. + +`RegionShardedSignEntryLoader.hasSignData(storageRoot)` treats an `IOException` while walking the directory as +"assume it has data" (fails safe toward *not* triggering legacy migration) rather than "assume empty" — a directory +listing failure shouldn't be misread as a fresh install and cause the legacy file to be migrated again. + +`Version3Converter.convertToV3(SignEntryV2, MarkerGroup[])`: converts `SignEntryV2`/`SignLinesParseResultV2` (which +carried a `MarkerTypeV2` enum, not a raw prefix string) to current `SignEntry`/`SignLinesParseResult` by looking up +`Arrays.stream(markerGroups).filter(g -> g.type() == MarkerGroupType.POI).findFirst().orElseThrow()` and using +*that* group's `prefix()` — i.e. **the first configured POI-type group's prefix is assumed for every migrated V2 +entry**, regardless of what `MarkerTypeV2` value was actually stored. This is safe historically because V2-era +configs only ever had one POI group, but means migration would misattribute prefix if a server has multiple POI +groups configured differently than at V2 time. Unaffected by region-sharding — this conversion still runs once, +during `LegacySignFileMigrator`'s reuse of the V1/V2/V3 chain, before entries are ever partitioned by region. + +`SignProvider.saveSigns(storageRoot)`: gets all cached entries from `SignManager.getAll()`, delegates to +`RegionShardedSignEntryWriter.write(storageRoot, entries, gson)`, which partitions via `SignRegionPartitioner` and +writes each region's `VersionedSignFile(V3, ...)` (creating parent dirs as needed). Any region file already on disk +that isn't in this save's partition set is **quarantined, not deleted**: renamed in place with a `.stale` suffix, +since an empty region on this save could mean the signs were genuinely removed, or that the region failed to load +at startup (in which case deleting it would be data loss) — there's no way to tell which from here. Quarantining is +skipped entirely (leaving all pre-existing files, including genuinely stale ones, untouched) if *any* region file +failed to write during this save, so a partial-write failure can't be compounded by also discarding good data. + +### Adding a new persisted field + +Per `AGENTS.md`: bump `SignFileVersions`, add a new loader/converter — **never** change an existing version's shape +in place. Old region files (or a not-yet-migrated legacy `signs.json`) on live servers must keep loading through +the version they were written with. + +--- +*Last updated: 2026-07-21 | Verified against: 26.2-0.17.0 (72d4280)* + diff --git a/agent-context/context/core-pipeline.md b/agent-context/context/core-pipeline.md new file mode 100644 index 0000000..94c1d83 --- /dev/null +++ b/agent-context/context/core-pipeline.md @@ -0,0 +1,236 @@ +# Core pipeline: sign text → marker action + +Cross-ref: `architecture.md` for package locations. + +## 1. Entry points into the pipeline + +Three ways a `SignEntry` gets built and handed to `SignManager`, plus a fourth path that only ever removes (never +builds/adds) an entry: + +1. **Server startup** — `BlueMapSignMarkersMod.onServerStarting` → `SignProvider.loadSigns(storageRoot, legacyPath)` + reads the per-world, region-sharded storage (migrating a pre-sharding single `signs.json` on first boot if + found) and calls `SignManager.addOrUpdate(...)` for every stored entry (see `config-and-persistence.md`). +2. **Block entity load** — `BlueMapSignMarkersMod.onBlockEntityLoad` (registered on + `ServerBlockEntityEvents.BLOCK_ENTITY_LOAD`) fires for every loaded `SignBlockEntity` and calls + `SignHelper.createSignEntry(entity, "unknown")` → `SignManager.addOrUpdate(...)`. Player id is `"unknown"` + here because chunk load isn't attributable to a player. +3. **Mixins** (`src/main/resources/bluemapsignmarkers.mixins.json`, server-only, `JAVA_21` compat level): + - `SignBlockEntityInject` injects `SignBlockEntity.updateSignText` at `TAIL` → a player edited a sign → + `SignManager.addOrUpdate(SignHelper.createSignEntry(this, player.getStringUUID()))`. + - `AbstractBlockInject` injects `BlockBehaviour.affectNeighborsAfterRemoval` at `HEAD`, but only proceeds + `if (state.getBlock() instanceof SignBlock)` → `SignManager.remove(new SignEntryKey(...))`. +4. **Chunk-load reconciliation** — `BlueMapSignMarkersMod.onChunkLoad` (registered on + `ServerChunkEvents.CHUNK_LOAD`) doesn't build a `SignEntry`. It queries `SignManager.getKeysInChunk(...)` for + sign keys the cache already knows about in the loading chunk, and calls `SignManager.remove(key)` for any whose + `SignBlockEntity` is gone — see §4 below. + +`SignHelper.createSignEntry` builds a `SignEntry` from both the front and back `SignText`, running each through a +`volatile` module-level `SignLinesParser` instance (`buildParser()`, first populated from +`ConfigManager.get().getMarkerGroups()` at class-init). `SignHelper.reloadParser()` rebuilds it from the current +config — called from `SignManager.reloadConfig()` (see §3) on every BlueMap reset, so a sign parsed *after* +`/bluemap reload` picks up an edited prefix/matchType rather than a stale one. + +## 2. Parsing: `SignLinesParser` + +A 3-state machine (`START` → `HAS_MARKER_TYPE` → `INVALID`) driven by `ParsingContext`: + +- Every line is `.trim()`-ed before processing. +- In `START`: blank lines are skipped (state stays `START`). The first non-blank line is checked against every + configured `MarkerGroup` in order (`markerGroups.stream().filter(...).findFirst()`) — **first match wins** when + prefixes overlap (e.g. `[poi]` vs `[poi` — see `SignLinesParserTest.firstMatchingGroupWinsWhenMultipleConfigured`). + If no group matches, state goes to `INVALID` and the final result is `SignLinesParseResult(null, "", "")`. +- Match semantics differ by `MarkerGroupMatchType`: + - `STARTS_WITH` (default): `line.startsWith(prefix)`; label = `line.substring(prefix.length()).trim()`. + - `REGEX`: `line.matches(prefix)` — **whole-line match**, not `find()`. This means a regex prefix can't share its + line with label text (unlike `STARTS_WITH`, where `[poi] Town Hall` puts the label on the same line). Label + extraction uses `line.replaceAll(prefix, "").trim()`. +- Once in `HAS_MARKER_TYPE`: every non-blank line is appended to the detail buffer (`ParsingContext.appendDetail`, + which joins with `\n`); the *first* non-blank line becomes `label` if one hasn't been set yet. Blank lines + between content lines are skipped without breaking the state. +- `ParsingContext.buildResult()` trims the accumulated detail buffer and returns + `SignLinesParseResult(markerGroup.prefix(), label, detail)`. + +Result: a sign can put its label on the prefix line (`[poi] Town Hall`) or on the following line — both produce +the same label/detail. + +## 3. `SignManager` — the decision point + +Singleton (double-checked locking), holds: +- `ConcurrentMap signCache` — every known sign, keyed by position+dimension. +- `SignChunkIndex chunkIndex` — secondary lookup from chunk to the sign keys cached in it, kept in sync with + `signCache` (populated on add, cleared on remove, wiped and rebuilt in `reloadSigns()`) purely so chunk-load + reconciliation (§4) doesn't have to scan the whole cache. Never touched on the update branch — a `SignEntry`'s + position is immutable once cached, only ever a different key entirely. +- `volatile RuntimeConfig runtimeConfig` — a private record `RuntimeConfig(Map prefixGroupMap, + ActionFactory actionFactory)` built by static `buildRuntimeConfig()`/`buildPrefixGroupMap()` from + `ConfigManager.get().getMarkerGroups()` (duplicate prefixes are logged and skipped, first one wins). Bundling both + fields into one record swapped via a single `volatile` write means a reader always sees a `prefixGroupMap` paired + with the `actionFactory` (and its `MarkerSetIdentifierCollection`) built in the *same* reload — two separate + `volatile` fields could let one thread observe a freshly-rebuilt `prefixGroupMap` alongside the *previous* reload's + `actionFactory`, or vice versa. Every method that dispatches an action takes a local `var config = runtimeConfig` + snapshot first, then reads `config.prefixGroupMap()`/`config.actionFactory()` off that same snapshot rather than + re-reading the volatile field twice. +- One `BlueMapAPIConnector`; `SignManager` registers itself as an `IResetHandler` on it. + +`addOrUpdateSign(signEntry)` (called for every add/update event, from entry points 1-3 above — not §1.4's chunk-load +reconciliation, which only ever calls `removeByKey` directly): + +`newPrefix` (`SignEntryHelper.getPrefix`, preferring front-text prefix, falling back to back-text) `null` is handled +*before* the decision table below: if `existing != null`, dispatches a remove via `removeEntry` — this is how a sign +edited away from every configured prefix (e.g. `[poi] Town Hall` → `Town Hall`) gets its marker cleaned up, not the +table's `false` row. If `existing == null` too, it's a no-op (an unrecognized sign that was never tracked). Both +branches return immediately, skipping the table entirely. + +For every other case (`newPrefix != null`), decision table on `(existing entry in cache, whether the new entry is +currently a POI-type match)`: + +| existing | isPOIMarker | Action | +|----------|-------------|--------| +| `null` | `true` | **Add**: cache the entry, dispatch `AddMarkerAction` | +| non-null | `false` | **Remove**: dispatch `RemoveMarkerAction` via `removeEntry` | +| non-null | `true` | **Update** (see below) | +| `null` | `false` | no-op | + +`isPOIMarker` comes from `SignEntryHelper.isMarkerType(signEntry, prefixGroupMap, POI)`, which null-guards +`prefixGroupMap.get(prefix)` — returns `false` rather than throwing `NPE` if the entry's prefix isn't in the +current map at all (an operator removed/renamed that group's prefix in a config reload since this sign was last +dispatched). Every subsequent `prefixGroupMap.get(prefix)` call used to build the actual `MarkerGroup` for a +dispatch (in the Add branch, the same-prefix Update branch, and both sides of the prefix-changed +remove-then-add branch, plus `removeByKey`) is null-guarded the same way: if the prefix isn't in the map, that +specific dispatch is skipped and a warning logged, rather than passing a `null` `MarkerGroup` into `ActionFactory`. +In `reset()`'s replay (below), `existing` is always `null` post-clear, so an entry whose prefix maps to nothing +that reload falls into the `null`/`false` no-op row (or the Add branch's own null-guard) — its marker is neither +re-added nor removed, and stays orphaned in BlueMap until the physical sign is edited or removed. See the +config-reload known limitation in `plans/marker-group-config-reload-plan.md`. + +**Update branch detail:** the cache is refreshed unconditionally (note: if the incoming entry's `playerId` is +`"unknown"` — i.e. came from a chunk-load event, not a player edit — the *existing* cached `playerId` is preserved +rather than overwritten, so a chunk reload never erases the last player known to have edited a sign). Then: +- If the marker's prefix is unchanged (`existingPrefix.equals(newPrefix)`): only dispatch `UpdateMarkerAction` if + label or detail actually changed (`isTextDifferent`) — avoids redundant BlueMap API calls on every chunk reload. +- If the prefix changed (sign text edited to match a *different* marker group): dispatch `RemoveMarkerAction` for + the old group, then `AddMarkerAction` for the new group — a prefix change is a remove-then-add, not an in-place + update, because the marker lives in a different `MarkerSet`. + +`removeByKey(key)` looks up and removes from `signCache`; if nothing was cached for that key, or the removed +entry had no resolvable prefix, it logs and returns without dispatching anything. + +`reset()` (from `IResetHandler`, called when BlueMap fires a reset — i.e. `/bluemap reload`) calls +`reloadConfig()` then `reloadSigns()`, in that order: +- `reloadConfig()`: `ConfigManager.reload()` (re-reads `BMSM-Core.json` from disk), `SignHelper.reloadParser()`, + then replaces `runtimeConfig` wholesale via `buildRuntimeConfig()` — a freshly rebuilt `prefixGroupMap` paired + with a brand-new `ActionFactory` backed by a new `MarkerSetIdentifierCollection` — starting that cache clean on + every reload, paired with `BlueMapAPIConnector.resetQueue()` clearing `markerSetsCache` (see §6), means neither + identifier cache accumulates entries keyed on a `MarkerGroup` value from before the last reload (`MarkerSetIdentifier` + keys on the whole record by value, so a changed icon/offset/distance would otherwise be a new, never-evicted cache + entry). +- `reloadSigns()`: snapshots the current cache, clears it, then replays every entry back through `addOrUpdateSign` + — since `existing` is always `null` post-clear, every replayed entry takes the **Add** row above, re-dispatching + against the just-rebuilt `prefixGroupMap`/`actionFactory` so an edited icon/offset/visibility/distance/name takes + effect without a server restart. Full design and known limitations (group-rename leaves a duplicate marker under + the old `MarkerSet` name; a prefix rename alone, with no in-game re-edit, isn't reclassified) are in + `plans/marker-group-config-reload-plan.md` and `plans/marker-group-reload-followups-todo.md`. + +## 4. Chunk-load reconciliation — `SignChunkKey` / `SignChunkIndex` + +Addresses GitHub issue #110 (plan: `plans/chunk-load-sign-reconciliation-plan.md`). Nothing else detects a sign +that vanished while its chunk was unloaded (external region-file deletion/regen, backup restore, manual NBT +surgery) — the removal mixin (§1.3) only fires for an in-game block change on a *loaded* chunk. + +- **`SignChunkKey`** (`core.signs`, plain Java, record: `parentMap, chunkX, chunkZ`) — `forEntryKey(SignEntryKey)` + computes `Math.floorDiv(x, 16)`/`Math.floorDiv(z, 16)`, vanilla chunk granularity. Deliberately separate from + `core.signs.persistence.SignRegionKey`'s 512-block/32-chunk region math — that's on-disk file-layout, unrelated + to this in-memory runtime lookup. +- **`SignChunkIndex`** (`core.signs`, plain Java) — wraps `ConcurrentHashMap>`. + `add`/`remove` keep it in sync with a key's presence in `signCache`; `remove` also drops the chunk's map entry + once its key set empties, so long-emptied areas don't leak entries. `keysInChunk(parentMap, chunkX, chunkZ)` + returns a snapshot list (empty if nothing tracked there) — the query the reconciliation handler uses. `clear()` + is called alongside `signCache.clear()` in `reloadSigns()`. +- `SignManager.getKeysInChunk(parentMap, chunkX, chunkZ)` — static, delegates to `chunkIndex.keysInChunk(...)`. + Pure data query, but `SignManager` itself stays outside unit-test coverage regardless (constructs a + `BlueMapAPIConnector`); `SignChunkKey`/`SignChunkIndex` are unit-tested on their own + (`SignChunkKeyTest`/`SignChunkIndexTest`, see `testing.md`). +- **`BlueMapSignMarkersMod.onChunkLoad`** (registered on `ServerChunkEvents.CHUNK_LOAD`, game-coupled, no + automated coverage) — for each key `SignManager.getKeysInChunk` returns for the loading chunk, checks + `chunk.getBlockEntity(new BlockPos(key.x(), key.y(), key.z())) instanceof SignBlockEntity`; if not, logs at INFO + (an unattended removal is unusual enough to warrant visibility above the default log level) and calls + `SignManager.remove(key)` — the same removal path §1.3's mixin uses, no new dispatch logic. +- **No special case for `generated == true`** (a chunk Minecraft reports as newly generated, no saved data found). + That flag also covers "region file deleted externally, world regenerated it fresh" — exactly the scenario this + feature targets — so skipping reconciliation there would defeat the main use case. No performance reason to + skip it either: `keysInChunk` is one hashmap `get` returning empty for the overwhelming majority of chunk loads. + +## 5. Marker identity — `MarkerIdentifier` / `MarkerSetIdentifier` / `MarkerSetIdentifierCollection` + +- `MarkerIdentifier(x, y, z, parentSet)` — its `getId()` is `"x%d_y%d_z%d"`, the literal key used inside a BlueMap + `MarkerSet`'s marker map. **No dimension component** — uniqueness across dimensions is guaranteed only because + each dimension maps to a separate `MarkerSetIdentifier`/`MarkerSet`, not because the id string itself is unique. +- `MarkerSetIdentifier(mapId, markerGroup)` — one BlueMap marker-set per (map, marker-group) pair. +- `MarkerSetIdentifierCollection` is a per-`SignManager`-instance cache that guarantees the *same* + `MarkerSetIdentifier` object is returned for a given `(mapId, markerGroup)` pair (indexed both by map and by + marker group, intersected) — `ActionFactory` always goes through this rather than constructing + `MarkerSetIdentifier` directly, so repeated calls for the same map+group don't fragment the connector's + `markerSetsCache` (keyed by `MarkerSetIdentifier` equality/identity in `BlueMapAPIConnector`). + +## 6. `BlueMapAPIConnector` — the only class touching the BlueMap API + +- Holds a `ReactiveQueue` (see below) and a `markerSetsCache: Map>`. +- `BlueMapAPI.onEnable`/`onDisable` are registered in the constructor. `onDisable` shuts the queue down (actions + keep enqueuing but stop draining). `onEnable(api)`: assigns `this.blueMapAPI = api` **first**, then, if the queue + was shut down, calls `resetQueue()` (fresh queue + fresh `markerSetsCache`) and `fireReset()` (→ every registered + `IResetHandler`, i.e. `SignManager.reset()`) before resuming draining — this is why a BlueMap reload replays the + entire sign cache rather than assuming stale `MarkerSet` state is still valid. The `blueMapAPI` assignment must + come before `fireReset()`, not after: `fireReset()`'s replay dispatches `MarkerAction`s that `ReactiveQueue` + starts draining on background threads immediately (`enqueue()` calls `process()` synchronously, which submits + to the executor right away — it doesn't wait for the enqueuing loop, let alone `onEnable`, to finish), and those + threads read `this.blueMapAPI` in `getMaps()`. Assigning it after `fireReset()` (the pre-fix ordering) let replay + actions race ahead and read the *previous* cycle's `blueMapAPI` reference — root cause of a bug where editing a + marker group's config and running `/bluemap reload` made that group's markers (and its `MarkerSet` layer) vanish + instead of updating in place, recoverable only by reloading a second time. `SignManager.reloadConfig()`'s disk + read (see §3) made the race reliably reproducible by widening the window between replay-dispatch and the + now-corrected assignment point. This mitigates finding #12 in `plans/codebase-review-2026-07-11.md` (no + `volatile` on `blueMapAPI`, so the JMM still doesn't *guarantee* visibility across threads) but doesn't close it + outright, and doesn't touch findings #10/#11 (stale-executor-generation stragglers, unsynchronized field writes + racing `getMarkerSets()`'s lock) — those are a different race shape, still open. +- `getMarkerSets(identifier)` is `synchronized`; on cache miss it resolves `BlueMapAPI.getWorld(mapId)` → + `.getMaps()`, and for each map either fetches an existing `MarkerSet` by `markerGroup.name()` or builds+registers + one (`label`, `defaultHidden` from the `MarkerGroup`). One `MarkerSetIdentifier` can map to *multiple* + `MarkerSet`s if a world has multiple BlueMap maps rendered for it — every dispatched action applies to all of them. +- `processMarkerAction` dispatches on the concrete `MarkerAction` subtype via a `switch` pattern-match; **`MarkerAction` + is a plain abstract class, not `sealed`**, so adding a new subtype without adding a `case` here (and in + `logProcessingMessage`'s switch) silently falls through to `default` instead of failing to compile — see + `AGENTS.md`'s "Adding a new marker/BlueMap action" section. +- `addMarker` only actually builds a marker `if (markerGroup.type() == MarkerGroupType.POI)` — non-POI marker + groups are a no-op today (only `POI` exists in `MarkerGroupType`, so this is future-proofing, not a live branch). +- **HTML escaping (fixed)**: `addMarker`/`updateMarker` wrap `detail` with `HtmlUtils.toHtmlDetail(...)` (`common` + package) before it reaches `POIMarker.builder().detail(...)` / `poiMarker.setDetail(...)` — BlueMap renders + `detail` as raw HTML (unlike `label`, which BlueMap's own `Marker.setLabel()` escapes), and sign text is + player-controlled, so this closed a live XSS vector (`plans/html-detail-escaping-plan.md`). `toHtmlDetail` escapes + first, then converts `\n` to `
` so multi-line detail renders line breaks correctly — escaping before the `
` + substitution matters, otherwise the inserted tags would themselves get escaped. `SignEntry`/persisted `signs.json` + data stays raw/unescaped; escaping happens only at this BlueMap-API call site. + +## 7. `ReactiveQueue` — generic buffer-while-unavailable primitive + +Lives in `core.reactive`, not BlueMap-specific — reusable anywhere something needs to "queue while a dependency is +unavailable, drain once it's back." + +- `enqueue(message)`: offers to an internal `ConcurrentLinkedQueue`, then calls `process()`. +- `process()`: if `shouldRunCallback.shouldRun()` (for `BlueMapAPIConnector`, this is `BlueMapAPI.getInstance().isPresent()`), + submits `processMessages` to a fixed thread pool (`Executors.newFixedThreadPool(availableProcessors())`). +- `processMessages` loops while the queue is non-empty *and* `shouldRun()` still holds, polling one message at a + time and submitting **each individual message** as its own task to the same executor (so message processing + itself is also concurrent, not just the drain loop). +- `shutdown()`/`isShutdown()` wrap the executor; `getExecutor()` lazily recreates the executor if the current one + is null or shut down — this is what lets `BlueMapAPIConnector.onEnable` transparently get a fresh executor after + a prior `onDisable` shut the old one down. +- A package-private constructor overload accepts an `ExecutorService` directly (the public 3-arg constructor + delegates to it with `null`, same as before) — test-only seam so `ReactiveQueueTest` can inject a synchronous or + failure-simulating fake executor instead of the lazily-created fixed thread pool, with no change to real + behavior. See `testing.md` for what it covers, including a documented gap: an exception thrown by the processor + callback itself is swallowed (captured on an unawaited `Future`, never reaching `messageProcessorErrorCallback`) + — only a submission-time failure reaches that callback. + +--- +*Last updated: 2026-07-21 | Verified against: 26.2-0.17.0 (72d4280)* + diff --git a/agent-context/context/testing.md b/agent-context/context/testing.md new file mode 100644 index 0000000..929a456 --- /dev/null +++ b/agent-context/context/testing.md @@ -0,0 +1,137 @@ +# Testing + +## Framework and commands + +JUnit 5 (Jupiter), via `testImplementation platform("org.junit:junit-bom:5.11.4")` + +`org.junit.jupiter:junit-jupiter`, `testRuntimeOnly org.junit.platform:junit-platform-launcher`. `test { useJUnitPlatform() }` +in `build.gradle`. + +- `./gradlew test` — run the whole suite (`src/test/java`) +- `./gradlew test --tests "*.SignLinesParserTest"` — single class +- `./gradlew build` runs tests as part of the build and **fails the build on any test failure** + +## What's testable vs. what isn't + +Only plain-Java classes with **no Minecraft/Fabric/BlueMap API types in their method signatures** are unit tested. +Qualifying today: `SignLinesParser`/`ParsingContext`/`SignLinesParseResult`, `SignEntry`, `SignEntryHelper`, +`SignChunkKey`/`SignChunkIndex`, `MarkerGroup`/`MarkerGroupMatchType`/`MarkerGroupType`, `ConfigManager`/`ConfigProvider`, +`ReactiveQueue`, `HtmlUtils`, `FileUtils`, the sign-persistence loaders/converters/writer (`VersionedFileSignEntryLoader`, +`Version1SignEntryLoader`, `Version3Converter`, `RegionShardedSignEntryLoader`, `RegionShardedSignEntryWriter`, +`SignRegionKey`, `SignRegionPartitioner`, `LegacySignFileMigrator`), `ActionFactory`/`MarkerSetIdentifierCollection`. + +`Version1SignEntryLoader` used to be a partial exception — its legacy-shorthand (`"nether"`/`"end"`/`"overworld"`) +dimension normalization branch read `net.minecraft.world.level.Level`'s static constants, requiring a running +Minecraft bootstrap. That dependency was removed (the three identifiers are now spelled out as literal strings, +e.g. `"minecraft:the_nether"`) specifically so `Version1SignEntryLoaderTest` could exercise the shorthand-normalization +branches directly instead of only via an already-namespaced dimension string. + +Excluded — anything that must reference live game types (`SignHelper`, the two mixins, `BlueMapSignMarkersMod` +including its `ServerChunkEvents.CHUNK_LOAD` reconciliation handler, `BlueMapAPIConnector`, `SignProvider` itself, +since loading/saving calls the game-coupled `SignManager` singleton) — these are thin glue and can only be +verified manually: `./gradlew runServer` + placing/editing/breaking signs in-game (and, for chunk-load +reconciliation specifically, removing a sign block without going through the mod — e.g. deleting its chunk's +region file to force a regen — then reloading that chunk), watching the BlueMap web UI update. + +## Current coverage + +As of `26.2-0.17.0`, `src/test/java/com/tpwalke2/bluemapsignmarkers/`: +- `core/signs/SignLinesParserTest.java` — 10 `@Test` methods covering `SignLinesParser`: label-on-prefix-line vs. + label-on-following-line, multi-line detail joining/trimming, leading/interstitial blank-line handling, no-match + and all-blank sign results, `REGEX` match type's whole-line-match requirement (contrasted with `STARTS_WITH`), + first-matching-group-wins ordering, and whitespace tolerance. Test helper pattern: private static factory methods + (`startsWithGroup(prefix, name)`, `regexGroup(pattern, name)`) building a `MarkerGroup` with the remaining fields + fixed at reasonable defaults — follow this pattern for new test classes over hand-building full `MarkerGroup` + records inline. +- `common/HtmlUtilsTest.java` — escaping of individual metacharacters and a full script payload, plain text left + untouched, escape-before-`
`-substitution ordering (a sign literally containing `
` renders as escaped + entities, not a live tag), newline-to-`
` conversion including consecutive newlines and no-newline input. +- `core/signs/SignChunkKeyTest.java` — chunk assignment via `floorDiv` (origin, negative coordinates, the + 15/16 chunk-boundary blocks), mirroring `SignRegionKeyTest`'s pattern but at 16-block chunk granularity instead + of 512-block region granularity. +- `core/signs/SignChunkIndexTest.java` — add/query round-trip, multiple signs in one chunk, signs in different + chunks/dimensions staying isolated, `remove` dropping a chunk's map entry once its key set empties, `remove` of + an untracked key being a no-op, `clear` resetting everything. +- `core/signs/persistence/SignRegionKeyTest.java` — region assignment via `floorDiv` (including negative + coordinates and the exact region-boundary blocks 511/512), and `relativeFilePath` namespace/path splitting + (including the no-colon `unknown` dimension and a nested-path dimension). +- `core/signs/persistence/SignRegionPartitionerTest.java` — grouping entries by region and dimension, multiple + entries landing in the same region, empty input. +- `core/signs/persistence/RegionShardedSignEntryWriterTest.java` — one file per region; stale region files (signs + removed, or moved to a different region) quarantined with a `.stale` suffix on re-save, not deleted. +- `core/signs/persistence/RegionShardedSignEntryLoaderTest.java` — `hasSignData` true/false cases, round-trip + load of entries written across multiple regions and dimensions. +- `core/signs/persistence/LegacySignFileMigratorTest.java` — no-legacy-file case, migrating a V3-shaped legacy file + (entries land in the right region files, legacy file renamed to `.migrated` rather than deleted), migrating a + V1-shaped legacy file (prefix fabricated from the configured POI group, per `Version3Converter`'s existing + behavior). +- `core/signs/SignEntryHelperTest.java` — `getPrefix` (front-text preferred, back-text fallback, `null` when + neither side matches), `isMarkerType` (`true` on a matching POI prefix, `false` on no prefix, and `false` rather + than throwing when the prefix isn't in `prefixGroupMap` at all — the config-reload case from + `plans/marker-group-config-reload-plan.md`), `getLabel`/`getDetail` front/back precedence and combining. +- `core/signs/SignEntryTest.java` — standard `equals`/`hashCode` contract on the hand-written implementation + (reflexive, symmetric, per-field inequality, not equal to `null`/another type), `withKey` returning a new instance + with only the key changed; also documents a latent risk (`equalsAndHashCodeThrowNpeWhenThisEntrysKeyIsNull`) that + `equals`/`hashCode` NPE if the entry's own `key` (or `playerId`/`frontText`/`backText`) is `null` — harmless today + since no call site actually calls `SignEntry.equals()`/`hashCode()` (the sign cache keys on `SignEntryKey`). +- `core/signs/ParsingContextTest.java` — the `(null, "", "")` sentinel when no marker group is ever set, + `buildResult()` using the set group's `prefix()` plus the current label, multiple `appendDetail` calls joining + with `\n`, and that the final `trim()` only strips the outermost whitespace of the joined detail, not per-line + padding. +- `common/FileUtilsTest.java` — `createBackup` copies the original when no backup exists yet and leaves an existing + backup untouched; `moveToBackup` moves the original into place, no-ops when the source is missing, and no-ops when + a backup already exists; documents (`createBackupSwallowsACopyFailure...`) that a copy failure is caught and only + logged, never surfaced to the caller (review finding #13). +- `config/ConfigProviderTest.java` — `loadConfig` creating and persisting defaults when the file is absent; missing + optional V2 fields defaulted per-field in `convertToLoadedMarkerGroup`; malformed JSON returning `null`; V1→V2 + migration producing one POI group plus a `.v1.bak` backup; and a documented "current behavior" case (review + finding #9) where a well-formed V2 file that happens to contain the substring `poiPrefix` anywhere (e.g. inside a + group's `name`) is misdetected as V1 and collapsed to the single default POI group. +- `config/ConfigManagerTest.java` — `get()` returns the config from the most recent `reload`; falls back to + `new BMSMConfigV2()` defaults when the configured path fails to load; a second `reload()` replaces (not merges + with) what an earlier `reload` cached. +- `core/bluemap/actions/ActionFactoryTest.java` — each of `createAddPOIAction`/`createRemovePOIAction`/ + `createUpdatePOIAction` builds the right `MarkerIdentifier` and action-specific fields; repeated calls for the + same map/group (same or different action type) reuse the same `MarkerSetIdentifier` instance via + `MarkerSetIdentifierCollection`. +- `core/markers/MarkerSetIdentifierCollectionTest.java` — `getIdentifier` returns the same instance for a repeated + `(mapId, markerGroup)` pair (case-insensitive on `mapId`), distinct pairs get distinct identifiers. Also includes + `@Disabled` concurrent-first-callers stress test documenting review finding #16 (unsynchronized check-then-act + over the backing `TreeMap`/`HashMap`/`HashSet` lets concurrent first-time callers each construct their own + instance instead of converging on one) — confirmed failing when temporarily un-`@Disabled`d; re-enable once the + planned concurrency-hardening pass makes that sequence atomic. +- `core/reactive/ReactiveQueueTest.java` — enqueue → processor callback delivery; `shouldRun` gating (queued while + false, resumes once true, a mid-drain false leaves the rest queued); `shutdown()`/`isShutdown()`, including that + `getExecutor()` silently creates a fresh executor next time work is scheduled after a shutdown; a documented gap + where an exception thrown by the processor callback itself never reaches `messageProcessorErrorCallback` (it's + captured on an unawaited `Future` and dropped) versus a submission-time failure, which does reach it via a fake + executor. +- `core/signs/persistence/loaders/Version3ConverterTest.java` — basic V2→V3 conversion, plus three documented + "current behavior" gaps (review finding #6): a non-matching side (`markerType null`) still gets the POI group's + prefix fabricated onto it since `convertToV3` never reads `markerType`; with multiple POI-type groups configured, + the first one in array order always wins; with zero POI-type groups configured, `.orElseThrow()` throws + `NoSuchElementException` rather than failing gracefully. +- `core/signs/persistence/loaders/VersionedFileSignEntryLoaderTest.java` — V3-passthrough (no backup written); + V2 branch (converts via `Version3Converter`, backs up to `.v2.bak`); the catch-all fallback returning `null` + rather than throwing, for both malformed JSON and empty content (which parses to `null` and NPEs on + `.version()`, caught by the same generic `catch`). +- `core/signs/persistence/loaders/Version1SignEntryLoaderTest.java` — the three recognized legacy shorthand strings + normalizing to their canonical identifiers, case-insensitively; an already-namespaced dimension string passing + through unchanged; backup creation to `.v1.bak`; and a documented Low-severity finding that an unrecognized + dimension string is still silently lowercased on the `default` branch rather than preserved as-is. + +## CI integration + +`.github/workflows/build.yml` (push/PR to `main`, `releases/**`) and `.github/workflows/publish.yml` (manual +dispatch) both run `./gradlew test` before anything else — a test failure in `publish.yml` blocks the Modrinth +publish step entirely. + +Both workflows have a `summarize test results` step (`if: always()`, so it runs even when tests fail) +immediately after the test step: it sums the `tests`/`failures`/`errors`/`skipped` XML attributes out of +`build/test-results/test/*.xml` using plain shell (`sed`, looping the glob, `shopt -s nullglob`) and writes a +markdown pass/fail table to `$GITHUB_STEP_SUMMARY`. This was a **deliberate choice over a `checks: write`-based +JUnit reporter action** — those actions don't get `checks: write` permission on PRs from forks by default in a +public repo, so the summary step was written to need no extra permissions. + +--- +*Last updated: 2026-07-21 | Verified against: 26.2-0.17.0 (72d4280)* + diff --git a/plans/codebase-review-2026-07-11.md b/plans/codebase-review-2026-07-11.md index 8ee04f2..d567e70 100644 --- a/plans/codebase-review-2026-07-11.md +++ b/plans/codebase-review-2026-07-11.md @@ -69,6 +69,10 @@ next edited. Github issue #135 +**Resolved (`320fb47`, "#121 allow hot reloads on config").** `isMarkerType` now null-checks the +`prefixGroupMap.get(prefix)` lookup before calling `.type()`, returning `false` instead of throwing when a +persisted sign's prefix isn't in the current config. + ### 4. `SignProvider.loadSigns`'s load loop has no per-entry exception handling — one bad entry drops the whole file **`src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/persistence/SignProvider.java:59-61`** ```java @@ -236,11 +240,14 @@ a stale replayed value, or a sign removed during the race can be silently re-add (`BlueMapSignMarkersMod.java:42`, `SignManager.java:136`). Two independently-hardcoded literals that only happen to match today; editing one without the other silently breaks playerId-preservation logic on chunk load vs. player edit. -- **`ServerPathProvider.java` is dead code** — no callers anywhere in `src/`, and it's referenced elsewhere as - in-progress/unwired. Should be wired up or removed. -- **Charset mismatch between save and load**, both in `ConfigProvider.java` (`saveConfig` uses platform-default - `FileWriter`, `loadConfig` reads explicit UTF-8) and `SignProvider.java` (same pattern). Non-ASCII marker-group - names or sign text is at risk of encoding mismatch across a restart if the JVM default charset isn't UTF-8. +- ~~**`ServerPathProvider.java` is dead code**~~ — **Resolved.** `BlueMapSignMarkersMod` now `implements ... + ServerPathProvider`; no longer unwired. +- **Charset mismatch between save and load** in `ConfigProvider.java` (`saveConfig` uses platform-default + `FileWriter`, `loadConfig` reads explicit UTF-8) — still open. Non-ASCII marker-group names are at risk of + encoding mismatch across a restart if the JVM default charset isn't UTF-8. The sign-file half of this finding + (originally also flagged in `SignProvider.java`) is **resolved**: that class was replaced by the region-sharded + storage classes (`RegionShardedSignEntryWriter`/`Loader`, `LegacySignFileMigrator`, `#109`), which consistently + use `Files.writeString`/`readString` with explicit UTF-8 on both sides. - **No fail-fast config validation** — nothing checks marker-group prefixes are non-empty, that REGEX prefixes compile, or that prefixes are unique across groups at load time; bad data is only caught later, silently or by crashing (see finding #8). @@ -257,9 +264,13 @@ a stale replayed value, or a sign removed during the race can be silently re-add - **`logProcessingMessage` logs full raw sign text at INFO unconditionally**, sanitizing only `\n` (`BlueMapAPIConnector.java:97-102`) — other control characters (`\r`, ANSI escapes) aren't sanitized, a minor log-injection/log-noise vector. -- **Cache-key growth risk**: `MarkerSetIdentifier`/`markerSetsCache` keys are value-equality on the *entire* - `MarkerGroup` record, so any config field change (icon, offsets, distances) between reloads produces a new, - never-evicted cache entry. Speculative — depends on a config-reload path not covered in this review. +- **Cache-key growth risk, now confirmed rather than speculative**: `MarkerSetIdentifier`/`markerSetsCache` keys + are value-equality on the *entire* `MarkerGroup` record, so any config field change (icon, offsets, distances) + between reloads produces a new, never-evicted cache entry. Originally speculative, pending a config-reload path + not covered in this review — that path now exists (`SignManager.reloadConfig()`, added by `#121` hot-reload + work) and confirms the risk: it rebuilds the prefix map but never calls `blueMapAPIConnector.resetQueue()`, so + `markerSetsCache` is never cleared on a live config reload. Worth reconsidering as Medium severity rather than + Low. - **`SignManager`: dual-sided signs with different prefixes mix marker-group semantics** — the marker's group (icon/type/visibility) comes from whichever side `getPrefix` picks (front preferred), but `getDetail` merges *both* sides' text regardless of whether they matched different groups. @@ -271,8 +282,10 @@ a stale replayed value, or a sign removed during the race can be silently re-add ## Nitpick -- `ConfigManager.loadCoreConfig()` is `synchronized` despite being called exactly once from a `static final` field - initializer, which the JLS already makes thread-safe — the modifier is misleading, not harmful. +- ~~`ConfigManager.loadCoreConfig()` is `synchronized` despite being called exactly once from a `static final` + field initializer~~ — **stale**: the premise no longer holds. Config hot-reload work (`#121`) restructured + `ConfigManager`: `coreConfig` is now `volatile` and lazily loaded, and `reload()`/`reload(Path)` are + `synchronized` to guard concurrent hot-reloads for real, not a redundant guard on a one-time static initializer. - The default single-`[poi]`-group literal is duplicated verbatim across `BMSMConfigV2` and `LoadingBMSMConfigV2` with no shared constant. - `SignManager.isMarkerType` re-derives `getPrefix(signEntry)` that's already computed a few lines later —