release: v1.28.0 - #203
Merged
Merged
Conversation
Wires icarus.New into builtinSourceFactories using the real Project Daedalus Firestore project ID (projectdaedalus-fb09f), discovered via the daedalus-static-poc/AgentKush firebase config and live-verified read-only (538 mods, pagination, GetMod, GetModFiles). Documents the manual games.yaml entry until Steam auto-detection learns App ID 1149460. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…teDump (#136) validateDump previously treated any pak.ReadFile error identically via a blanket continue, conflating the expected Oodle-compression skip with genuinely unexpected errors (corruption, truncation, I/O failure) and silently narrowing what the gate verified. Now only errors.Is(err, unrealpak.ErrUnsupportedFormat) is skipped; any other error returns a wrapped, actionable error naming the unreadable table.
Adds Compile(ctx, dumps, basePakPath, localDumpDir, exmodzPath, outputPakPath), wiring unrealpak, ParseExmodz/ApplyRowPatch (Task 10/11) and DumpForBuild (Task 12a) into the end-to-end .exmodz -> _P.pak orchestration. Two corrections to the brief, both empirically grounded against a real install + a real Bear_Mount.EXMODZ (coordinator-approved, see task-12-report.md plan delta): - resolveCurrentFile reconstructs the base pak mount path by converting every '-' in CurrentFile back to '/', not by suffix-matching a literal hyphenated filename -- the brief's literal-suffix algorithm matched 0/14 real rows against the real base pak. - Compile skips the real-world 'EndOfMod' sentinel/terminator row and fails loudly on any other row missing File_Items, rather than trying to resolve a sentinel as a data table. Also adds sanitizeAssetPath (coordinator-approved) to reject path-traversal and absolute bundled-asset entry names from a .EXMODZ before they reach the output pak's index.
Compile now uses a named return + deferred cleanup: once unrealpak.Create(outputPakPath) succeeds, any later error (unresolvable row, missing dump table, patch failure, unsafe asset path, AddFile/Close failure) removes the partial output file before returning, instead of leaving a stray incomplete _P.pak on disk. A removal failure is joined into the returned error rather than masking it; the success path is untouched. unrealpak.Writer has no abort-without-finalizing method, so the underlying file descriptor is only reclaimed on GC in this case -- os.Remove still eliminates the on-disk deploy hazard the review flagged. Adds TestCompile_MidCompileFailure_LeavesNoOutputFile and extends TestCompile_UnsafeAssetPath_Errors to assert no file exists at outputPakPath after a mid-compile failure. Fix round 1 for the Task 12 review's one Important finding (task-12-review.md).
Adds domain.DeployCompile, source.Compiler, and Icarus.Compile, and wires a DeployCompile branch into Service.DownloadModToCache: after download, a source implementing Compiler transforms the file (Icarus's .exmodz -> _P.pak) before it's committed to cache, so everything downstream treats it like a DeployCopy file. Adds the per-game data_dump_path config (games.yaml -> GameConfig -> domain.Game.BaseDataPath), and closes the Task 9 review gap where ParseDeployMode didn't yet recognize "compile". Icarus.Compile needs a real dumps cache directory in production, but New(httpClient, projectID) was frozen at those two params by an earlier task with a dependent call site. Wires it instead via an optional SetDataDir(string) setter, mirroring the existing SetAPIKey optional-setter pattern in cmd/lmm/root.go's registerSource; Compile fails loudly rather than panicking if SetDataDir was never called. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review fix round 1: a DeployCompile game's compile branch ran on every downloaded file, but icarus.GetModFiles can also serve a prebuilt .pak alongside the .exmodz diff - routing a .pak through Compile fails loudly since it isn't a zip, making plain-pak Icarus mods permanently uninstallable. Gate the branch on file.FileName ending in ".exmodz" (case-insensitive) as well as DeployMode; any other file (notably .pak) falls through to the existing extract/copy path unchanged. No new config. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Final-review finding I1: trunk check reported 6 new errcheck findings for deferred Close() calls whose error is intentionally ignored (the repo idiom elsewhere, e.g. datadump.go/service.go, already appends //nolint:errcheck to these). Annotated the flagged sites in compile.go and compile_test.go. Re-running trunk check after that fix surfaced 2 more unannotated defers of the same class in reader_test.go:222 and roundtrip_test.go:36 (both files are wholly new to this branch, so every finding in them counts as branch- introduced, not pre-existing debt) -- annotated those too so the branch carries zero new lint issues, not just the six named in final-review.md. trunk check: 0 new issues (14 pre-existing, out of scope, unchanged).
…ew) (#136) PR #171 Copilot review fixes: - unrealpak: readRegion and Reader.ReadFile now validate a size field (non-negative, <= the pak file's own size) before using it to size an allocation, via a shared validateAllocSize helper. Reader gains a fileSize field, threaded through Open/parseIndex/readRegion. Closes the Task 2/3 deferred-minor 'unvalidated size pre-check allocation'. - unrealpak: cursor.take's already-err-latched early-return path now clamps n to >=0 before make(), matching the fresh-error branch a few lines below -- a corrupted length field could otherwise panic there. - icarus: firestore_client.listCollection now url.QueryEscape()s nextPageToken before appending it to the request URL, instead of concatenating the server-issued token raw. Closes Task 7's deferred minor. - icarus: Compile's error-path cleanup defer now closes the unrealpak.Writer (error ignored -- the partial output is about to be deleted anyway) before os.Remove, so the fd never leaks and the remove works on platforms that refuse to delete a still-open file. Success path unchanged. New tests: TestValidateAllocSize, TestReadRegion_RejectsInvalidSizeOrOffsetBeforeReading, TestReader_ReadFile_RejectsInvalidSizeField, TestCursor_Take_ClampsNegativeLengthOnErrLatchedPath, TestFirestoreClient_ListCollection_EscapesPageToken.
…llback (review) (#136) PR #171 Copilot round-2 review fixes: - unrealpak: Writer.Close now rejects, via a new checkEncodedLocationFits helper, an encoded-index length exceeding math.MaxInt32 before storing it as an int32 location -- previously a >2 GiB index would silently wrap/go-negative there, corrupting every location recorded from that point on. Consistent with the existing >4GiB offset/size error style. - icarus: fetchTree now rejects a tar entry whose declared header size exceeds a 64 MiB per-table cap (the largest real table is 7.3 MB) before reading it, and reads the body through io.LimitReader bounded by the declared size -- a network-fetched, third-party archive's size field is not trusted for allocation. - icarus: fileNameFromURL's fallback now returns a dotted name ('mod.<ext>') instead of a bare extension ('exmodz'/'pak'), which silently defeated isExmodzFile's '.exmodz' suffix check and compiledFileName's filepath.Ext-based rename. A parsed basename that exists but has no extension of its own gets the expected extension appended rather than being discarded. Also guards a '..' basename into the fallback path (closes the final review's fileNameFromURL '..' minor). New tests: TestCheckEncodedLocationFits, TestDumpStore_DumpForBuild_RejectsOversizedTarEntry, TestFileNameFromURL (table-driven).
…idth (review) (#136) PR #171 Copilot round-3 review fixes (suppressed-comment items): - unrealpak: validateAllocSize also rejects size > math.MaxInt before the caller's int(size) cast -- a no-op on a 64-bit build (where int is 64 bits, same as size's type) but load-bearing on a 32-bit one, where a size that already passed the fileSize check could still overflow int and wrap negative on the cast. - icarus: DumpStore's dead cacheDir field is removed (it was never read -- every call re-fetches over the network) along with newDumpStore's now-pointless cacheDir parameter and SetDataDir's filepath.Join plumbing that built it; DumpStore's doc comment now says what it actually does. SetDataDir keeps its dataDir parameter (required by the shared interface{ SetDataDir(string) } duck-typed contract root.go's registerSource calls uniformly across sources) but no longer uses its value. - icarus: ParseExmodz now normalizes zip entry backslashes to forward slashes and matches the manifest's 'Extracted Mods/' prefix + '.EXMOD' suffix, and an asset's .uasset/.uexp extension, case-insensitively, so a Windows-built .EXMODZ's differently-cased or backslash-separated entries are no longer silently dropped. Multiple candidate manifests is now a loud, named error instead of silently keeping whichever the zip directory listed last. Asset keys are stored under their normalized (forward-slash, original-case) form; compile.go's sanitizeAssetPath still composes correctly against this -- its own backslash normalization becomes a harmless no-op on an already-normalized string, not a double-handling bug (noted in the report). New/extended tests: TestValidateAllocSize (math.MaxInt boundary), TestParseExmodz_NormalizesBackslashNames, TestParseExmodz_MatchesCaseInsensitively, TestParseExmodz_MultipleManifests_Errors.
PR #171 Copilot round-4 review fix: Search() sliced filtered mods for pagination straight off Firestore's listCollection order, which is not guaranteed stable across runs -- the same page could return different mods between requests. Sort deterministically (Name, then ID as a tiebreak for same-named mods) before slicing, matching the custom api/manifest/directory sources' name-based ordering convention (internal/source/custom/search.go). Extended TestIcarus_Search_FiltersClientSide with a deliberately non-alphabetical mock response order and an empty-query search asserting the returned order is alphabetical by Name -- verified this fails without the fix (stashed icarus.go, reran) and passes with it.
registerSource's comment still said SetDataDir was needed because Icarus's Compile 'needs a cache directory for the base-table dump store' -- stale since round 3 (e18b0bd) removed DumpStore's cacheDir entirely (it was never read; the store fetches on demand). Reworded to describe what SetDataDir actually does today: gate Compile on having been called at all (it constructs the dump store then), with dataDir's value itself unused -- SetDataDir exists only to satisfy the shared interface{ SetDataDir(string) } contract. Comment-only change.
feat: Icarus mod support — Firestore source + .exmodz PAK compilation (#136)
M1: amend the untouched #136/#175 CHANGELOG bullet, which still claimed a .exmodz compiles into a deployable _P.pak at download time - false under the merged-only model landed by this same unreleased section. M3: fix 3 stale comments referencing retired #196 surfaces (source.Compiler, compiledFileName, Service.ApplyRecompile) that no longer exist - no dead code, just misleading doc comments. M4: applyRecompile (cmd/lmm/update.go) discarded ApplyMergedPakRegen's result.Warnings and watched for UpdateWarning/UpdateNote progress phases the function never emits (only UpdateDownloadDone) - a merge's asset-collision warnings, 'a loud warning' per the CHANGELOG, silently never printed via lmm update's single-mod apply path (TUI and DeployProfile were unaffected).
feat: merged-pak compilation for Icarus exmod mods (#197)
…#197) Root cause of the postsmoke bug: cmd/lmm/install.go's batchInstallMods (reached from doInstall when a search returns multiple mods, installMultipleMods -> batchInstallMods) is a bespoke reimplementation of install/deploy that never went through Service.ApplyInstall - the only seam that synced the merged pak. A DeployCompile mod deploys zero files of its own (validate+retain only), so a multi-select install of 2+ exmodz mods generated the merged pak in cache but never deployed it, with nothing to warn the user - exactly the user-reported bug. Sync failures print unconditionally (not --verbose-gated) so a future failure is loud, not silent. Regression test drives the real production batchInstallMods (not a reimplementation) with two different exmodz mods and confirms the deployed merged pak contains both mods' content.
#197) Completes the seam audit started by the batchInstallMods fix. Each of these is a bespoke cmd-layer reimplementation (or repair path) that mutates a merge input with no seam that used to catch it: - doProfileApply: disable/enable/install-missing loops, none synced. - doProfileSync: toAdd/toRemove change profile.Mods MEMBERSHIP directly, which GetInstalledModsInProfileOrder (and so enabledExmodzSources) depends on independent of the DB Enabled flag. - doModEdit: --version is a direct regeneration trigger; a --source/--source-id relink changes the identity enabledExmodzSources keys off. - doVerify --fix: repairModVersion (moves the cache dir + recorded version) and redownloadModFile both change merge-fingerprint inputs. Each regression test drives the real production cmd-layer function (not a reimplementation) and confirms the merged pak's deployed state actually changes as a result.
Task item 2 ("check the error path plumbing") surfaced a systemic
issue beyond the postsmoke bug itself: syncMergedPak failures were
folded into result.Notes (this codebase's --verbose-gated diagnostic
channel) or appended to result.Warnings WITHOUT a corresponding live
progress event, on nearly every flow that syncs - including
ApplyInstall, the already-fixed single-mod install path. Several
callers (doProfileSwitch, applyUpdate, doProfileImport) also simply
discarded the result struct that would have carried the warning.
- EnableResult/DisableResult/SwitchResult gain a Warnings field
(additive) - Notes alone had no unconditional display channel.
- EnableMod/DisableMod/UninstallMod/ApplyProfileSwitch/ApplyRollback/
PurgeProfile route sync failures through Warnings instead of Notes.
- ApplyInstall/ApplyUpdate/ApplyRollback/PurgeProfile additionally
emit a live *Warning progress event, so a caller driving purely off
progress (like doInstall/applyUpdate) sees it without needing to
read the result struct at all.
- cmd/lmm/mod.go (enable/disable), cmd/lmm/profile.go (switch/import)
now print result.Warnings unconditionally to stderr.
- internal/tui/service_core.go folds the new/now-populated Warnings
fields into ActionOutcome.Warnings for enable/disable/switch (the
TUI's own equivalent of loud), so nothing silently regressed there.
ReorderProfileMods (bare-error signature, discards sync warnings) is
deliberately left as-is - its own doc comment already reasons about
this tradeoff explicitly, and lmm update/verify remain the safety net.
Regression test drives a real single-mod install through a forced
merge failure and confirms it reaches stderr unconditionally - the
exact scenario the task named ("if install's sync deploy had failed
loudly the user would have seen it").
…ak (#197) Closes out the remaining postsmoke UX corrections beyond the sync-plumbing fixes: "0 files" read as a failure for a validate+retain-only exmodz mod, `mod files` gave false "may need to be redeployed" guidance for the same mods, and `verify`'s RECOMPILE NEEDED row always blamed "base pak updated" even when the real cause was a missing artifact, then pointed at a bare `lmm update` that (being notify-policy by construction) only reports the row again instead of fixing it. - doInstall/doInstallBatch/batchInstallMods/doImport now print "Installed (merged pak updated)" instead of "(0 files)"/"Files deployed: 0" when a DeployCompile mod deploys zero files by design. - `lmm mod files` explains that a zero-file DeployCompile mod participates in the profile's merged pak, reusing verify.go's hasRetainedSource to distinguish that case from a genuinely broken record. - domain.Update gains RecompileReason ("base pak updated" | "not deployed"), set by CheckMergedPakStaleness from the same fingerprint/ artifact-existence check that already distinguished the two internally (#197 I5). `lmm verify` surfaces the real reason in both text and --json (the `note` field), and the fix hint now says `lmm update --all`. Regression tests added/extended for every site: install/import output text, mod files' two branches (exmodz vs a genuinely broken record), and both CheckMergedPakStaleness reason values at the core and CLI layers. docs/man/man1/lmm-verify.1 regenerated (`make man`) for the corrected RECOMPILE NEEDED help text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…docs (#200 review) Two Copilot review findings on PR #200: 1. install.go:657 (+732, 1213) - the "Installed (merged pak updated)" success line printed unconditionally for a DeployCompile zero-file mod, even when the sync that actually updates the merged pak is non-fatal and had just failed (with a Warning already on stderr) - stdout and stderr told contradictory stories. - doInstall (STRICT path): InstallResult gains MergedPakSyncFailed, set by ApplyInstall's own end-of-call sync attempt; the success line threads it directly, since the sync has already happened by the time it prints. - doInstallBatch (dependency/BATCH path) and batchInstallMods (multi-select path): the sync is deferred to ONCE per batch, AFTER every per-mod "Installed" line would otherwise print live - so the outcome literally isn't knowable yet at that point. Both now buffer the affected mods' names as they're installed and print their completion lines only after the batch's one sync attempt actually runs, positioned after its stderr Warning ("...see warning above" is now literally true, not just illustrative). 2. verify.go:663 - doVerify --fix has synced the merged pak since the prior #197 fix wave, but the command's --help text (and generated man page) still claimed "--fix does not repair" compile/merged-pak staleness. Corrected to describe what --fix actually does; `make man` regenerated docs/man/man1/lmm-verify.1 (enforced by TestGenManTree_MatchesCommittedPages). RED->GREEN verified for all three install.go sites (temporarily reverted each fix, confirmed the false-success-line symptom, restored, confirmed pass) plus a fixture fix (TestDoInstall_DeployCompile_AnnouncesRetaining was missing svc.AddGame, silently sync-failing and printing the wrong branch once the wording was made accurate). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fix: merged pak never deployed on multi-select install (#197 postsmoke)
…201) lmm list showed mods in DB install order (installed_at), which has no relationship to what actually decides merge precedence for a deploy_mode: compile game (Icarus). Switched to core.OrderByProfile - the same seam the TUI's mod list already uses (Overview, service_core.go) - rather than the deploy-only GetInstalledModsInProfileOrder: that helper deliberately OMITS a mod absent from the profile's load order (correct for deploy, where an untracked mod must never silently deploy), which would have made such a mod vanish from a listing instead of just showing up first (lowest priority, since it has no claim to "final say"). Using OrderByProfile gives list.go the exact same order the TUI already shows, so CLI and TUI now genuinely agree - true parity, confirmed with the coordinator - rather than inventing a third, list-only convention. Added a "Merge precedence" paragraph to README and docs/configuration.md, verified against the actual merge implementation (internal/source/icarus/merge.go): later-in-load-order mods win conflicting table-row fields via a per-field upsert (untouched fields from earlier mods survive); bundled assets are whole-file last-wins with a warning (install/update surface it; reorder itself regenerates the pak silently); the bottom of the load order has final say; lmm profile reorder regenerates the merged pak immediately. list's help text now names the load-order behavior explicitly (ran `make man` to regenerate the stale committed man page the genman test caught).
- cmd/lmm/list.go help text said a later mod "wins conflicting rows" - the actual merge semantics are field-level (a per-field upsert on a shared row, not a whole-row overwrite), matching the README/ configuration.md wording already written for #201. Ran `make man` to regenerate the now-stale lmm-list.1 page. - list_order_test.go's assertion messages labeled Mod B as "later in load order" when the test's own ReorderMods call ([B, A]) actually makes Mod A last (final say) and Mod B first (lowest priority) - a failure would have pointed at the wrong mod. Reworded to state the actual array order and which mod has final say.
feat: document merge precedence; display list in profile load order (#201)
There was a problem hiding this comment.
Pull request overview
Release-prep PR for v1.28.0, updating docs/manpages and landing the feature/fix batch centered on Icarus compile-deploy support (merged pak, staleness detection), fail-loud config validation, Steam detection updates, and richer CLI/TUI color output.
Changes:
- Add/extend Icarus + compile-deploy support (merged pak flow, cache retention markers, staleness/update/verify plumbing, and supporting unrealpak/icarus source code + tests).
- Enforce fail-loud validation for
link_method/deploy_modeacross config loaders, and propagate new Steam known-game metadata (deploy_mode/sources). - Expand CLI/TUI UX: richer color output and updated docs/man pages for v1.28.0.
Reviewed changes
Copilot reviewed 158 out of 160 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Document Icarus entry, merge precedence, CLI color defaults, and new custom-source placeholders |
| internal/unrealpak/writer_test.go | Add Writer unit tests (footer, determinism, mount point, bounds) |
| internal/unrealpak/roundtrip_test.go | Add writer↔reader roundtrip + structural byte-shape assertions |
| internal/tui/service_core_internal_test.go | Ensure InstallCompiling progress renders in TUI |
| internal/tui/mutations.go | Use UpdateItem.VersionLabel() in update detail/result rendering |
| internal/tui/actions.go | Use UpdateItem.VersionLabel() in changelog picker/overlay labels |
| internal/tui/actions_provider.go | Add RecompileNeeded + VersionLabel() helper for update rows |
| internal/storage/config/profiles.go | Fail-loud validation of profile link_method |
| internal/storage/config/profiles_test.go | Add tests for rejecting unknown profile link_method on import |
| internal/storage/config/games.go | Fail-loud validation for games.yaml link_method/deploy_mode |
| internal/storage/config/config.go | Fail-loud validation for config.yaml default_link_method |
| internal/storage/config/config_test.go | Add tests for rejecting invalid config/games/profile methods/modes |
| internal/storage/cache/cache.go | Add retained source naming + merge fingerprint marker + empty-dir cleanup on delete |
| internal/storage/cache/cache_test.go | Tests for cache delete cleanup + retained source/fingerprint reserved behavior |
| internal/source/steam/steam.go | Dedup symlinked Steam roots; carry deploy_mode/sources through detection |
| internal/source/steam/steam_test.go | Tests for symlink dedup + Icarus detection fields |
| internal/source/steam/games.go | Extend known-games schema with optional nexus_id/deploy_mode/sources |
| internal/source/steam/games_test.go | Tests for Icarus entry + override round-trip of new fields |
| internal/source/steam/data/steam-games.yaml | Add schema notes and Icarus (AppID 1149460) entry |
| internal/source/source.go | Introduce MergeCompiler + MergeSource for merged compile model |
| internal/source/icarus/helpers_test.go | Shared base pak test fixture builder for Icarus compile tests |
| internal/source/icarus/firestore_value.go | Firestore typed-value unwrapping helpers |
| internal/source/icarus/firestore_value_test.go | Tests for Firestore field/value decoding |
| internal/source/icarus/firestore_client.go | Firestore REST client (list/get document, pagination) |
| internal/source/icarus/exmodz.go | Parse .EXMODZ: find manifest + extract uasset/uexp assets with size caps |
| internal/source/icarus/exmod.go | Parse/apply .EXMOD diff against UE DataTable JSON Rows array |
| internal/source/custom/api.go | Add {category} / {tags} placeholders for API custom sources |
| internal/source/custom/api_test.go | Tests for category/tags placeholder substitution + escaping |
| internal/domain/mod.go | Add SourceMerged and update fields for recompile/staleness reporting |
| internal/domain/game.go | Make ParseLinkMethod/ParseDeployMode return (value, ok) + valid-options constants; add DeployCompile |
| internal/domain/game_test.go | Update parser tests to assert fail-loud (ok=false) behavior |
| internal/domain/errors.go | Add ErrInvalidLinkMethod / ErrInvalidDeployMode sentinels |
| internal/core/updater.go | Add Service.CheckGameUpdates to unify remote updates + merged-pak staleness |
| internal/core/service_download_traversal_test.go | Tests ensuring filename/fileID traversal is sanitized during download/compile ingest |
| internal/core/service_download_local_test.go | Test traversal sanitization for local ingest copy-mode filenames |
| internal/core/merged_pak_locked_test.go | Tests covering merged pak behavior with locked mods present |
| internal/core/merged_pak_internal_test.go | Tests for merged fingerprint determinism/equality conditions |
| internal/core/merged_pak_import_flow_test.go | Ensure ApplyImport syncs merged pak for DeployCompile .exmodz mods |
| internal/core/importer.go | Add DeployCompile import validation/retention + compiler resolver + retained file ID plumbing |
| docs/plans/archive/2026-08-01-icarus-quickbms-fallback-design.md | Archive QuickBMS fallback design doc |
| docs/man/man1/lmm.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-verify.1 | Document stale-compile checks + JSON status addition |
| docs/man/man1/lmm-update.1 | Document recompile-needed behavior and JSON/status extensions |
| docs/man/man1/lmm-update-rollback.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-uninstall.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-tui.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-status.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-source.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-source-validate.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-source-list.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-search.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-purge.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-sync.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-switch.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-reorder.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-list.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-import.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-export.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-delete.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-create.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-profile-apply.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod-unlock.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod-show.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod-set-update.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod-lock.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod-files.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod-enable.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod-edit.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-mod-disable.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-list.1 | Document list load-order semantics in man page |
| docs/man/man1/lmm-install.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-import.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-game.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-game-show-default.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-game-set-default.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-game-detect.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-game-clear-default.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-game-add.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-deploy.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-conflicts.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-completion.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-completion-zsh.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-completion-powershell.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-completion-fish.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-completion-bash.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-auth.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-auth-status.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-auth-logout.1 | Regenerate man page version header to 1.28.0 |
| docs/man/man1/lmm-auth-login.1 | Regenerate man page version header to 1.28.0 |
| docs/configuration.md | Document fail-loud validation, compile deploy_mode semantics, merge precedence, steam-games schema |
| cmd/lmm/update_color_test.go | Tests for update command ANSI coloring + alignment stability |
| cmd/lmm/uninstall_cache_test.go | Tests ensuring uninstall removes empty per-mod cache container dir |
| cmd/lmm/status.go | Route tabwriter output through printTable; add richer color accents; propagate GetEffectiveLinkMethod errors |
| cmd/lmm/status_color_test.go | Tests for status output color gating + alignment stability |
| cmd/lmm/search.go | Route tabwriter output through printTable; row-tint installed results |
| cmd/lmm/search_color_test.go | Tests for search row tinting + ANSI gating/alignment |
| cmd/lmm/root_test.go | Ensure builtin source factories include Icarus |
| cmd/lmm/root_color_test.go | Tests for reportError ANSI gating |
| cmd/lmm/profile.go | Ensure merged pak sync is invoked at mutation seams; print warnings; pass ctx to sync |
| cmd/lmm/profile_compile_test.go | CLI regression tests for merged pak sync on apply/sync flows |
| cmd/lmm/mod_show_color_test.go | Tests for mod show richer color accents |
| cmd/lmm/mod_files_compile_test.go | UX regression tests for DeployCompile mod files messaging |
| cmd/lmm/mod_edit.go | Ensure mod edit triggers merged pak sync + prints warnings |
| cmd/lmm/mod_edit_compile_test.go | Regression test: version edit syncs merged pak |
| cmd/lmm/mod_color_test.go | Tests for mod lock/set-update success checkmark coloring |
| cmd/lmm/list.go | List in profile load order; route table through printTable; apply row tinting |
| cmd/lmm/list_test.go | Test that list help text mentions load order |
| cmd/lmm/list_order_test.go | Tests for list ordering semantics (load order vs install order; untracked still shown) |
| cmd/lmm/import.go | Import: ensure retained file ID included; propagate GetEffectiveLinkMethod errors; sync merged pak; better UX message for compile imports |
| cmd/lmm/import_compile_test.go | Regression test: imported exmodz participates in merge + UX message |
| cmd/lmm/game.go | Convert detected Steam games via helper supporting deploy_mode/sources and strict validation |
| cmd/lmm/game_detect_test.go | Tests for detected game conversion, strict deploy_mode validation, README-equivalent Icarus values |
| cmd/lmm/deploy.go | Propagate GetEffectiveLinkMethod errors instead of ignoring |
| cmd/lmm/conflicts.go | Colorize stale conflict winner suffix |
| cmd/lmm/conflicts_color_test.go | Test for stale suffix coloring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+263
to
+266
| sources := g.Sources | ||
| if sources == nil { | ||
| sources = map[string]string{"nexusmods": g.NexusID} | ||
| } |
Comment on lines
+95
to
+99
| if resp.StatusCode != http.StatusOK { | ||
| // Drain before Close so the underlying connection stays eligible for | ||
| // reuse — net/http only pools an HTTP/1.x connection once its body | ||
| // has been read to EOF; returning immediately here left whatever the | ||
| // server sent (however small) unread, forcing the transport to |
Comment on lines
+67
to
+71
| for _, f := range zr.File { | ||
| if f.Name == manifestPath || f.FileInfo().IsDir() { | ||
| continue | ||
| } | ||
| normalized := normalizeZipName(f.Name) |
Comment on lines
+283
to
+287
| // RecompileNeeded marks a #197 merged-pak staleness row (generalizing | ||
| // #196's per-mod version): the profile's merged pak no longer matches | ||
| // its recorded fingerprint (enabled-mod set, load order, a mod's | ||
| // version, or the base pak changed). ToVersion equals FromVersion in | ||
| // this case - u itself is the SYNTHETIC merged-pak row, not a real |
Comment on lines
+113
to
+117
| // #201: display the profile's load order - the order that actually | ||
| // decides merge precedence (later = merged later = wins) - not | ||
| // installed_at (GetInstalledMods' own DB order), which has no | ||
| // relationship to it. core.OrderByProfile, not the deploy-only | ||
| // GetInstalledModsInProfileOrder seam: that one deliberately OMITS a |
A known-games entry with neither Sources nor a non-empty NexusID
silently produced {"nexusmods": ""} - a garbage source mapping that
would propagate into games.yaml unnoticed. Every legitimate entry sets
at least one of the two, so this is a misconfigured entry and now
fails loud, naming the game, instead. The deploy_mode check still runs
first (unchanged ordering), so an entry with both problems reports the
deploy_mode error, matching the existing test's fixture.
…eview) A non-200 response from Firestore produced a bare "HTTP %d" - a 403 was otherwise undiagnosable, since a permission error, a quota message, and a malformed request all look identical without knowing which request failed and what the server actually said. The error now includes the request URL and a 512-byte-capped snippet of the response body, read via io.LimitReader before continuing to drain the rest of the body to EOF (unchanged connection-reuse behavior - Close still happens on an already-drained body).
The per-entry cap (maxZipEntrySize, 64 MiB) doesn't bound an archive with many entries each individually under it - five 60 MiB entries sum to 300 MiB despite none tripping the per-entry check. ParseExmodz now sums every asset entry's DECLARED uncompressed size up front and refuses the whole archive, naming it, before reading any asset content at all, if the combined total exceeds a new 256 MiB cap (maxZipTotalAssetsSize). The per-entry cap and the lying-declared-size read guard are unchanged.
…iew) The TUI hardcoded "(base pak updated)" for every RecompileNeeded row, even though core.CheckMergedPakStaleness already distinguishes two causes (RecompileReason: "base pak updated" - the fingerprint changed - or "not deployed" - the fingerprint matches but the artifact is missing) and lmm verify already shows the distinct reason. UpdateItem gains a RecompileReason field, threaded through from domain.Update in coreProvider.CheckUpdates; VersionLabel renders "(<reason>)" instead of the hardcoded string, falling back to "base pak updated" only if RecompileReason is ever left empty (defensive - core always sets one).
… review) The lock-state lookup ignored EVERY config.LoadProfile error (`profileYAML, _ := ...`), including #172's fail-loud link_method validation - an invalid profile YAML silently degraded `lmm list` (no lock info, and per #201 every mod reading as "absent from the load order") instead of surfacing the same error every other command honors. Only domain.ErrProfileNotFound (a profile with no YAML on disk yet) is still tolerated; any other error, including validation, now aborts the listing.
6 tasks
- gameFromDetected: treat an empty-but-non-nil Sources map the same as
nil (len(sources) == 0), since YAML's `sources: {}` unmarshals to an
empty map, not nil.
- ParseExmodz: make the bundled-assets total-size accumulation
overflow-safe by checking each declared size against the remaining
cap headroom before adding, instead of summing first and comparing
after - a naive sum could wrap a uint64 given attacker-controlled
UncompressedSize64 values.
- firestore_client.getJSON: omit the trailing body clause entirely
when the trimmed error snippet is empty, avoiding a dangling colon.
…-fixes fix: release-review hardening batch (#203 review)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 159 out of 161 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/source/icarus/firestore_client.go:81
- In getDocument, the local variable name
urlshadows the importednet/urlpackage name used elsewhere in this file (e.g. QueryEscape). Renaming it to something likereqURLavoids confusion and prevents accidental misuse if this function grows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Release prep for v1.28.0 — MINOR, new functionality plus a batch of fixes since v1.27.1:
internal/source/icarus, newdeploy_mode: compile+internal/unrealpak): a built-in, unauthenticated Firestore-backed catalog;.exmodzmods are validated and their table-row diffs retained,lmm game detectrecognizes Icarus and generates a workinggames.yamlentry (EPIC: Icarus mod support — .exmod/.exmodz PAK compilation on Linux #136, Icarus: read base tables directly from data.pak (Zlib) and remove the dump subsystem #175, Add Icarus to Steam game auto-detection (lmm game detect) #177).exmodzmod's diffs compose into onezzz_LMM_Merged_P.pakper profile instead of silently shadowing each other, self-heals when the base pak or enabled-mod set changes, and now syncs from every mutation seam (install, profile apply/sync, mod edit, verify --fix) with loud, non-silent failures--no-color/NO_COLOR-respecting colorization acrosslist/status/search/update/conflicts/mod showlink_method/deploy_modeis now a load-time error instead of a silent fallback (breaking only for configs that were already silently misbehaving).exmodzimports now route through the same compile step as a download{category}/{tags}in a declarativeapisource'ssearchpathlmm listload-order display (Document merge precedence; display lmm list in profile load order #201): shows mods in profile load order (matching merge precedence and TUI parity) instead of DB install orderGetEffectiveLinkMethodno longer silently swallowing an invalid profilelink_method(GetEffectiveLinkMethod swallows profile load errors, bypassing #172's fail-loud validation on the deploy path #189);mod disable/enable'sdeployedflag not tracking undeploy/redeploy (mod disable leaves the Deployed flag stale (list -v shows DEPLOYED yes after files are removed) #183); a Copilot-review UX/wording pass on the merged-pak sync work (Document merge precedence; display lmm list in profile load order #201's own review fixups included)Plan docs for the completed Icarus epic (#136, #175, #197 — research, pak-format findings, quickbms spike/fallback exploration, zlib pivot, and the final merged-pak design) are archived to
docs/plans/archive/in the first commit.Test plan
go build ./...go vet ./...gofmt -l .(clean)go test ./... -count=1(all packages green, includes the genman version-consistency test)trunk check --no-fixon all touched files (no new issues)make man(all man pages regenerated for the 1.28.0 version bump)[Unreleased]moved to a dated[1.28.0]section with comparison link, following the existing pattern🤖 Generated with Claude Code