fix: release-review hardening batch (#203 review) - #204
Merged
Conversation
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.
There was a problem hiding this comment.
🟡 Not ready to approve
There is a confirmed misconfiguration bypass in gameFromDetected (empty-but-non-nil sources map) and a potential overflow bypass of the new .EXMODZ total-size cap that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR hardens several pre-release code paths surfaced during the v1.28.0 whole-batch review, improving fail-loud validation, diagnosability, resource caps, and TUI parity with CLI behavior.
Changes:
- Add/propagate merged-pak staleness reasons through the TUI update model and rendering.
- Improve Icarus Firestore non-200 diagnostics (URL + capped response snippet) and add a combined-size cap for
.EXMODZbundled assets. - Make
lmm listsurface non-ErrProfileNotFoundprofile load errors, and make game detection reject known-games entries missing any source mapping.
File summaries
| File | Description |
|---|---|
| internal/tui/service_core.go | Threads core update staleness reason into TUI UpdateItem. |
| internal/tui/service_core_recompile_test.go | Adds integration + unit coverage for RecompileReason and VersionLabel() rendering. |
| internal/tui/actions_provider.go | Extends UpdateItem with RecompileReason and renders it in VersionLabel(). |
| internal/source/icarus/firestore_client.go | Enhances non-200 error messages with URL and capped response snippet while preserving connection reuse. |
| internal/source/icarus/firestore_client_test.go | Adds tests ensuring error messages include URL/snippet and are size-capped. |
| internal/source/icarus/exmodz.go | Adds total declared-size cap across bundled assets before reading content. |
| internal/source/icarus/exmodz_test.go | Adds fixture covering oversized combined declared asset sizes using raw zip headers. |
| cmd/lmm/list.go | Makes lmm list tolerate only ErrProfileNotFound for profile YAML; other errors now fail loud. |
| cmd/lmm/list_profile_error_test.go | Adds tests for fail-loud invalid link_method and tolerant missing profile YAML. |
| cmd/lmm/game.go | Makes gameFromDetected fail loud when known-games provides neither sources nor nexus_id. |
| cmd/lmm/game_detect_test.go | Adds coverage for the new sources/nexus_id validation and its success cases. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+82
to
+84
| assetFiles = append(assetFiles, f) | ||
| totalDeclaredSize += f.UncompressedSize64 | ||
| } |
Comment on lines
+105
to
109
| const errBodySnippetCap = 512 | ||
| snippet, _ := io.ReadAll(io.LimitReader(resp.Body, errBodySnippetCap)) | ||
| _, _ = io.Copy(io.Discard, resp.Body) | ||
| return fmt.Errorf("HTTP %d", resp.StatusCode) | ||
| return fmt.Errorf("icarus: GET %s: HTTP %d: %s", url, resp.StatusCode, strings.TrimSpace(string(snippet))) | ||
| } |
- 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.
There was a problem hiding this comment.
🟢 Ready to approve
The changes are well-scoped hardening improvements with targeted regression tests, and no functional blockers were found.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
cmd/lmm/game.go:275
- The new fail-loud error for missing sources/NexusID doesn’t mention that the misconfiguration is in the known-games schema (steam-games.yaml), unlike the deploy_mode validation just above. Including the origin makes the error more actionable when this bubbles out of
lmm game detect.
return nil, fmt.Errorf("game %q: known-games entry has no sources and no nexus_id - set at least one", g.Slug)
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
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
Five Copilot findings from PR #203's whole-batch release review (release/v1.28.0), fixed on
developsince the release branch has already cut and none of this is shipped yet:gameFromDetectedfails loud on missing source config (cmd/lmm/game.go): a known-games entry with neitherSourcesnor a non-emptyNexusIDsilently produced{"nexusmods": ""}— a garbage source mapping that would propagate intogames.yamlunnoticed. Now errors, naming the game, unless at least one is set. Thedeploy_modecheck still runs first (unchanged ordering).getJSONerrors name the URL and a capped body snippet (internal/source/icarus/firestore_client.go): a non-200 response was a bare"HTTP %d"— a 403 was undiagnosable (permission error, quota message, and malformed request all looked identical). Now includes the request URL and a 512-byte-capped snippet of the response body; connection-reuse draining is unchanged.ParseExmodzcaps total bundled-asset size (internal/source/icarus/exmodz.go): the existing per-entry cap (64 MiB) didn'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. Now sums every asset's declared size up front and refuses the archive, naming it, before reading any content, if the combined total exceeds a new 256 MiB cap.internal/tui/actions_provider.go,service_core.go): the TUI hardcoded"(base pak updated)"for every staleness row, even thoughcore.CheckMergedPakStalenessalready distinguishes two causes andlmm verifyalready shows the distinct reason.UpdateItemgainsRecompileReason, andVersionLabel()renders it instead of the hardcoded string.lmm listsurfacesLoadProfileerrors beyond "not found" (cmd/lmm/list.go): the lock-state lookup ignored everyLoadProfileerror, including Decide: unknown deploy_mode/link_method config values silently fall back to defaults #172's fail-loudlink_methodvalidation — an invalid profile YAML silently degraded the listing instead of erroring like every other command. Onlydomain.ErrProfileNotFoundis still tolerated.Test plan
go build ./...,go vet ./...,gofmt -l .— all cleango test ./...(fresh,go clean -testcache) — full suite green, 17 packagestrunk check— clean (0 new issues)zip.CreateRawto declare oversized sizes without needing real hundreds-of-MiB contentNo CHANGELOG changes — all five are fixes to code that hasn't shipped (still
[Unreleased]ondevelop), and none contradicts any existing CHANGELOG wording, so per the task's guidance nothing needed amending.Full report:
.superpowers/sdd/2026-08-01-prerelease-batch/report-203-fixes.md🤖 Generated with Claude Code