fix(core): record the installed file's version, not the mod's latest (#94) - #128
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ersion (#94) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…o match (#94) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…version (#94) Code review flagged that the A2 stamp in applyInstallPrimary also changes what install.before_each hooks receive as ModVersion (the effective selected-file version, not the mod-level version) with no test pinning that behavior. Adds a focused test reusing the existing old-file fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nks (#94) Task A7: --fix now repairs Task A6's version_mismatch detection. Re-keys the cache entry from the recorded version to the effective (source- reported) one, corrects the DB row and the active profile's record, and re-runs the installer for symlink deployments (whose targets lived inside the just-renamed cache dir and would otherwise dangle). A rename blocked by a pre-existing destination leaves the cache alone and emits a note but still fixes DB/profile; a rename failure aborts before any DB write so the row stays version_mismatch instead of creating a new disagreement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… working symlinks (#94) Code review on the version-record repair found two correctness gaps and a doc-accuracy gap: - A re-link failure (step 4) after the cache/DB/profile correction (steps 1-3) already succeeded left the row with Deployed still true and no way for a later `verify --fix` to detect anything wrong, since effective now equals the recorded version. --fix now clears Deployed (and saves) before returning that error, so the DB doesn't keep claiming a working deployment that's actually dangling. - When the cache rename is blocked by a pre-existing effective-version dir, the existing symlink deployment still points at the intact recorded-version dir - re-linking in that case would repoint a working deployment into the unvetted pre-existing one. The re-link step is now skipped whenever the rename was blocked. The blocked-rename note is also now exposed in --json (new verifyFileJSON.Note field, additive to the JSON contract) instead of only appearing in text output. - Updated three stale/incomplete user-facing strings: the post-run --fix hint, the --fix example, and the Long doc's --json paragraph, to reflect that VERSION MISMATCH is now repairable and that its note is visible in JSON too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
README and the CHANGELOG's new 1.23.0 entry cover the version_mismatch/ version_unverifiable statuses, that verify now contacts each mod's source for the check, and what --fix repairs. Regenerates docs/man from the updated verify --help text (make man). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every man page's .TH header embeds the CLI version string, so the 1.22.0 -> 1.23.0 bump touches all of them (make man). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g the cache (#94) The mod cache has no profile segment (internal/storage/cache/cache.go:29) but installed_mods rows are per-profile. When `verify --fix -p P1` renamed the shared cache dir for a version_mismatch repair, a sibling profile holding the same mod at the same stale recorded version was left pointing at the now-missing dir until it was independently verified. repairModVersion now runs a new repairSiblingProfiles step after a successful cache rename (never after a blocked/skipped one - nothing was orphaned in that case): every other profile's row for the same mod whose Version still equals the pre-repair recorded version gets corrected (DB + profile YAML), and re-linked (with the same Deployed-clear-on-failure handling as the primary row) if deployed via symlink. Rows recording a different version are left alone for their own verify run. This runs even if the primary row's own re-link failed, since a sibling's orphaning is caused by the rename, not by the primary's deploy outcome. Also adds the test-coverage gap flagged separately: the "os.Rename itself fails" branch (as opposed to the already-tested "blocked because the destination exists" case) had no direct test, verified only by inspection per the original task report. Covered via a read-only cache parent directory, asserting no DB write happens and the row stays version_mismatch. CHANGELOG's 1.23.0 --fix bullet and the verify Long/--json doc text (regenerated man page) now describe the sibling-repair behavior and the "note" field's additive use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#94) hookCtx.ModVersion is deliberately stamped from mod.Version before file selection here, unlike core.applyInstallPrimary where the #94 effective-file stamp precedes its own hook. One-line comment only - not a behavior change in either direction, per final-review Finding 3(b). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on primary relink failure (#94) Re-review of the sibling-profile repair (previous commit) found two new Important gaps: 1. repairSiblingProfiles silently `continue`d past a failed SaveInstalledMod, UpsertMod, or pm.List call, contradicting DEV.md's "never swallow errors without logging/context" - a sibling DB-write failure right after the shared cache rename left that profile orphaned, indistinguishable from "was never a candidate". Every per-sibling failure now prints a "Warning: could not repair profile <name>: <err>" line in text mode and folds into the returned summary as "repair FAILED in profile(s): ..."; a pm.List failure surfaces once the same way. The best-effort loop still continues past a single sibling's failure - only the silence was the defect. 2. repairModVersion can return a non-empty note (a successful sibling repair) alongside a non-nil err (the PRIMARY row's own re-link failed), but doVerify's --json branch only ever assigned Note when repairErr == nil, so a real sibling repair was invisible to a --json caller in that combination. The repairErr != nil branch now attaches note to the JSON row when present, without touching Status (stays version_mismatch) or issue accounting - only the primary row's own deploy state is what actually failed. Also simplified the now-dead note+siblingNote concatenation branch in repairModVersion: note is provably "" whenever a rename actually happened (it's only ever set in the mutually-exclusive "blocked" branch), so the sibling summary is now a plain assignment with a comment explaining the invariant, instead of an unreachable else-branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a core correctness bug where installs that select a specific downloadable file (e.g., archived/older) were recorded and cached using the mod-level “latest” version instead of the selected file’s version (#94). It also adds lmm verify detection + --fix repair for pre-existing version-record mismatches, and bumps docs/version to 1.23.0 as part of the EPIC #98 foundational work.
Changes:
- Add
domain.EffectiveInstalledVersionand stamp the effective (file-level) version into all install-recording flows so DB/profile/cache all reflect the bytes actually installed. - Extend
lmm verifywith version-record validation against upstream file IDs, plus--fixrepair (cache re-key, DB/profile correction, symlink re-linking, sibling-profile repair notes). - Update README/manpages/CHANGELOG and bump CLI version to 1.23.0.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents verify’s new version-record checks/repairs. |
| internal/domain/mod.go | Adds EffectiveInstalledVersion helper. |
| internal/domain/mod_test.go | Unit tests for EffectiveInstalledVersion. |
| internal/core/flows.go | Stamps effective version in core install/switch/import flows. |
| internal/core/flows_test.go | Adds regression coverage for ApplyProfileSwitch stamping. |
| internal/core/flows_install_test.go | Adds regression tests for primary + batch/dependency stamping and hook visibility. |
| internal/core/flows_import_test.go | Adds regression coverage for ApplyImport stamping. |
| docs/plans/2026-07-28-epic98-foundational-fixes.md | Adds implementation plan doc for #94/#95 sequencing. |
| docs/man/man1/lmm.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-verify.1 | Documents verify’s new statuses, JSON note, and repair behavior. |
| docs/man/man1/lmm-update.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-update-rollback.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-uninstall.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-tui.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-status.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-source.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-source-validate.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-source-list.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-search.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-purge.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-sync.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-switch.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-reorder.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-list.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-import.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-export.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-delete.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-create.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-profile-apply.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-mod.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-mod-show.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-mod-set-update.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-mod-files.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-mod-enable.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-mod-edit.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-mod-disable.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-list.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-install.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-import.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-game.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-game-show-default.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-game-set-default.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-game-detect.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-game-clear-default.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-game-add.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-deploy.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-conflicts.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-completion.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-completion-zsh.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-completion-powershell.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-completion-fish.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-completion-bash.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-auth.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-auth-status.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-auth-logout.1 | Version bump in generated manpage header. |
| docs/man/man1/lmm-auth-login.1 | Version bump in generated manpage header. |
| cmd/lmm/verify.go | Implements version-record checking + --fix repair logic and JSON note. |
| cmd/lmm/verify_test.go | Adds extensive behavioral coverage for new verify functionality. |
| cmd/lmm/root.go | Bumps CLI version to 1.23.0. |
| cmd/lmm/profile.go | Stamps effective version in profile apply install path. |
| cmd/lmm/profile_test.go | Regression test for profile apply stamping. |
| cmd/lmm/install.go | Stamps effective version in CLI batch install save path. |
| cmd/lmm/install_test.go | Regression test for CLI batch install stamping. |
| CHANGELOG.md | Adds 1.23.0 release notes and link updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+232
to
+240
| // Nothing to check against: local imports and manual downloads have | ||
| // no source to query, and a mod with no recorded file IDs predates | ||
| // even the buggy stamping this check exists to catch. | ||
| if mod.SourceID == domain.SourceLocal || mod.ManualDownload || len(mod.FileIDs) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| sourceFiles, err := svc.GetModFiles(ctx, mod.SourceID, &mod.Mod) | ||
| if err != nil { |
Collaborator
Author
There was a problem hiding this comment.
Verified against the code and this doesn't reproduce — refuting rather than fixing.
mod.FileIDs (the gate at the top of the pre-pass — len(mod.FileIDs) == 0 skips before any of the mismatch/unverifiable/unreachable branches can run) and the files slice the main per-file loop iterates (from svc.GetFilesWithChecksums) are both sourced from the exact same installed_mod_files table with no additional filtering beyond game/profile:
internal/storage/db/mods.go'sreplaceModFileIDsTx(called from everySaveInstalledMod) writes it.getModFileIDsBatch/GetModFileIDs(what populatesmod.FileIDsforGetInstalledMods/GetInstalledMod) read it.GetFilesWithChecksums(what populatesfiles) reads it with the identicalWHERE game_id = ? AND profile_name = ?predicate.
So whenever mod.FileIDs is non-empty for a filtered mod — the only way the pre-pass can reach MISMATCH/UNVERIFIABLE/source-unreachable at all — files filtered to that same mod ID is provably non-empty too, and the main loop's unconditional checked++ (which runs before any per-file status logic) fires for it at least once. The "no checksum rows while FileIDs is non-empty" state this claim needs can't occur under the current schema.
Added TestDoVerify_VersionUnverifiable_WithModFilter_ChecksumRowsAlwaysExist (cmd/lmm/verify_test.go) as empirical evidence: constructs exactly the scenario described (modFilter set, VERSION UNVERIFIABLE, nothing else installed) and asserts the misleading "No files found for mod ..." message never appears. It passes without any production change — commit cc7e4b2.
No code change made for this claim.
Comment on lines
+623
to
+633
| if sibling.Deployed && sibling.LinkMethod == domain.LinkSymlink { | ||
| if err := svc.GetInstaller(game).Install(cmd.Context(), game, &sibling.Mod, p.Name); err != nil { | ||
| sibling.Deployed = false | ||
| _ = svc.SaveInstalledMod(sibling) //nolint:errcheck // best-effort: the version correction above already stands regardless | ||
| } | ||
| } | ||
|
|
||
| repaired = append(repaired, p.Name) | ||
| if !jsonOutput { | ||
| fmt.Printf(" %s\n", colorGreen(fmt.Sprintf("Repaired (profile %s): %s → %s", p.Name, recorded, effective))) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Verified against the code and this is real — the re-link failure branch genuinely slipped through the previous round's error-surfacing fix (which covered pm.List/SaveInstalledMod/UpsertMod but not this one).
Before: on Install failure, sibling.Deployed was cleared and saved, but the code then fell through unconditionally into repaired = append(repaired, p.Name) and printed the green "Repaired (profile %s): ..." line — reporting a broken deployment as a clean success, with nothing in the warning output or the --json note.
Fixed (cmd/lmm/verify.go, repairSiblingProfiles): a re-link failure is now tracked the same way as the other two failure points — appended to failed (including the flag-clear error too if that also fails), a "Warning: could not repair profile <name>: <err>" line in text mode, and continue instead of falling into the success path, so it shows up in the JSON note's "repair FAILED in profile(s): ..." and the "Repaired" line is never printed for that profile. The DB/profile version correction from the earlier steps still stands (matches the primary row's own re-link-failure semantics) — only the deployment itself is reported broken.
TDD: extended TestDoVerify_Fix_VersionMismatch_SiblingProfile_Deployed_RelinkFails_ClearsDeployedFlag with stdout assertions and added TestDoVerify_Fix_VersionMismatch_SiblingProfile_Deployed_RelinkFails_JSONNotesFailure — both RED against the old code (literally printed "Repaired (profile second): 1.5 → 1.0" despite the forced re-link failure), GREEN after the fix.
Commit cc7e4b2.
…as repaired (#94) PR #128 Copilot review round 1, Claim 2 (comment 3670171495): verified against the code and confirmed real. repairSiblingProfiles's re-link branch cleared a failed sibling's Deployed flag but then fell through into the success path anyway - appending the profile to `repaired` and printing the green "Repaired (profile ...)" line - even though the game-dir symlink was left dangling. This slipped through the previous round's error-surfacing fix, which covered SaveInstalledMod/UpsertMod/ pm.List but not this branch. A re-link failure is now tracked the same way as the other two per-sibling failure points: appended to `failed` (with the underlying error, and the flag-clear error too if that ALSO fails), a "Warning: could not repair profile <name>: <err>" line in text mode, and `continue` instead of falling into the "repaired" success path - so the JSON note's "repair FAILED in profile(s): ..." now includes it and the success line is never printed for that profile. The DB/profile version correction from the earlier steps still stands (matching the primary row's own re-link-failure semantics) - only the deployment itself is reported broken, not the whole repair. Claim 1 (comment 3670171464, `checked` under-incremented in the version-record pre-pass) was investigated and refuted: mod.FileIDs (the gate into the pre-pass's mismatch/unverifiable/unreachable branches) and the `files` slice the main per-file loop iterates are both sourced from the same `installed_mod_files` table with no additional filtering, so whenever the pre-pass can reach any of those branches, the main loop's unconditional per-file `checked++` has already fired for that mod - the "no checksum rows" premise cannot coexist with a populated FileIDs set under the current schema. Added TestDoVerify_VersionUnverifiable_WithModFilter_ChecksumRowsAlwaysExist as evidence: constructs exactly the scenario described (modFilter set, VERSION UNVERIFIABLE, nothing else installed) and asserts the misleading message never appears - it passes without any production change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cmd/lmm/verify.go:602
- In
repairSiblingProfiles, errors fromsvc.GetInstalledModare currently treated the same as "mod not installed" and silently skipped.GetInstalledModcan also fail for real DB/query problems; swallowing those makes sibling repairs look complete while actually skipping profiles due to an error, and those failures won’t be reflected in the returnednote/warnings.
sibling, err := svc.GetInstalledMod(mod.SourceID, mod.ID, game.ID, p.Name)
if err != nil || sibling.Version != recorded {
continue
}
…lure (#94) PR #128 Copilot round 2 surfaced a suppressed, low-confidence finding with no inline thread (verify.go:602 at review time): repairSiblingProfiles treated `svc.GetInstalledMod` returning ANY error identically to "this profile never had the mod" - a genuine DB/query failure was silently `continue`d past, indistinguishable from a normal not-a-candidate skip, with nothing in the warnings or the returned note. Ruled real: it's the exact error-swallowing class this PR keeps fixing, in the same loop the previous two rounds already had to patch for SaveInstalledMod, UpsertMod, pm.List, and the re-link step. internal/storage/db/mods.go's GetInstalledMod returns the sentinel domain.ErrModNotFound on sql.ErrNoRows and wraps anything else ("querying installed mod: %w"). The sibling lookup now branches on errors.Is(err, domain.ErrModNotFound): a not-installed profile stays a silent skip (correct, unchanged), but any other error is appended to `failed` and printed as "Warning: could not repair profile <name>: <err>" - exactly like the other three failure points already handle it. Test: added TestDoVerify_Fix_VersionMismatch_SiblingProfile_NotInstalled_SkippedSilently, pinning that a profile which genuinely never had the mod (a new "fourth" profile with no InstalledMod row at all, distinct from "third"'s different-recorded-version case) stays silent - no warning, no mention. A genuine DB-error-per-sibling case has no clean injection point in this test harness without either corrupting shared state the primary path also depends on, or adding a test-only seam to production code neither asked for nor otherwise justified - verified by inspection instead, mirroring the SaveInstalledMod-failure precedent from the previous round (see final-fix-wave-report.md). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Copilot round 2 follow-up — one suppressed, low-confidence finding (no inline thread, so no discussion comment reply) at Ruled real — it's the same error-swallowing class this branch has already fixed twice in this exact loop ( Added Commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cmd/lmm/verify.go:240
doVerifycallssvc.GetModFiles(ctx, mod.SourceID, &mod.Mod)using the InstalledMod’s embeddedmod.GameID. InstalledMod rows are persisted with the LMM-normalized game ID (see core regression tests likeTestService_ApplyProfileSwitch_InstallLoop_SavesWithNormalizedGameID), but several sources (e.g. NexusMods) expect a source-specific game ID inmod.GameIDwhen listing files. This can causelmm verifyto incorrectly treat the source as unreachable / skip version checks for games wheregame.SourceIDs[sourceID]differs fromgame.ID.
Compute a source-mapped copy of the mod (same mapping logic as Service.GetMod) before calling GetModFiles.
sourceFiles, err := svc.GetModFiles(ctx, mod.SourceID, &mod.Mod)
…ion-record check (#94) PR #128 Copilot round 3 surfaced a suppressed finding at cmd/lmm/verify.go:240: the version-record pre-pass called svc.GetModFiles(ctx, mod.SourceID, &mod.Mod), where mod.GameID is the LMM game ID (installed rows persist normalized IDs). Service.GetModFiles forwards straight to the source with no game-ID translation - unlike Service.GetMod, which maps through game.SourceIDs[sourceID] first (internal/core/service.go), and unlike the core Updater, which does the same translation before CheckUpdates (internal/core/updater.go). Sources like NexusMods address games by their own domain (e.g. "skyrimspecialedition" vs an LMM ID like "skyrim-se"), so on any game where the mapping differs, the version check would call the source with the wrong ID for every mod - almost certainly manifesting as "could not check version (source unreachable)" across the board, silently defeating the whole check in the common case. Added sourceMappedMod(game, mod) in cmd/lmm/verify.go: returns a copy of mod with GameID translated through game.SourceIDs, using the exact same rule as Service.GetMod (map only when the mapping exists AND is non-empty - an empty mapping means "this source applies to any game" and must not blank out the LMM ID). The version-record pre-pass now calls svc.GetModFiles(ctx, mod.SourceID, sourceMappedMod(game, &mod.Mod)). Confirmed repairModVersion and repairSiblingProfiles have no equivalent issue: neither calls into a ModSource at all - repairModVersion re-keys the local cache, writes the DB row and profile YAML, and re-links from cache via the installer; repairSiblingProfiles does the same DB/YAML/ re-link work for sibling profiles. Deploys read from the already-cached files, not the source API, so no GameID ever reaches a source call in either function. Test: added fakeInstallSource.receivedGameFileIDs (cmd/lmm/install_test.go), capturing the GameID GetModFiles is actually called with, mirroring internal/core/updater_test.go's gameIDCapturingSource. Two new tests in cmd/lmm/verify_test.go: TestDoVerify_VersionCheck_MapsGameIDPerSourceMapping (RED against the unfixed code - received the LMM ID "g1" instead of the mapped "mapped-domain" - then GREEN) and TestDoVerify_VersionCheck_EmptySourceMapping_KeepsLMMGameID (the empty-mapping case, which already passed pre-fix since mod.GameID was always the LMM ID unconditionally - pins the "must not blank out" rule going forward). setupDoVerifyVersionTest now also returns the fakeInstallSource so tests can inspect it; its three other call sites were updated to ignore the new return value. Note: setupDoInstallTest's game fixture maps SourceIDs["test-src"] to "g1" - the SAME as game.ID - which is exactly why no earlier test caught this: the bug is invisible whenever the mapped and unmapped IDs happen to coincide. Separately spotted but explicitly OUT OF SCOPE for this fix: redownloadModFile (cmd/lmm/verify.go, used by the MISSING/NO CHECKSUM --fix repairs) has the identical unmapped svc.GetModFiles(ctx, mod.SourceID, &mod.Mod) call. Flagged for a follow-up rather than folded in here, since the coordinator's task scoped this round to the version-record check plus confirming repairModVersion/repairSiblingProfiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 68 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
cmd/lmm/verify_test.go:1159
- Same as above: using svc.SaveInstalledMod here can wipe installed_mod_files checksums and create unrelated NO CHECKSUM behavior during doVerify. Prefer SetModDeployed/SetModLinkMethod for this setup change.
secondMod, err := svc.GetInstalledMod("test-src", "mod1", game.ID, "second")
require.NoError(t, err)
secondMod.Deployed = true
secondMod.LinkMethod = domain.LinkSymlink
require.NoError(t, svc.SaveInstalledMod(secondMod))
cmd/lmm/verify_test.go:1207
- Same pattern again: svc.SaveInstalledMod in test setup can drop file checksums via installed_mod_files replacement. Use the dedicated setters to flip Deployed/LinkMethod without touching file rows.
secondMod, err := svc.GetInstalledMod("test-src", "mod1", game.ID, "second")
require.NoError(t, err)
secondMod.Deployed = true
secondMod.LinkMethod = domain.LinkSymlink
require.NoError(t, svc.SaveInstalledMod(secondMod))
cmd/lmm/verify_test.go:1116
- This test flips Deployed/LinkMethod by mutating the struct and calling svc.SaveInstalledMod, but SaveInstalledMod replaces installed_mod_files and can wipe checksums (the behavior this PR is trying to prevent). Use the targeted setters instead so the test doesn’t accidentally introduce NO CHECKSUM warnings or hide checksum-related regressions.
This issue also appears in the following locations of the same file:
- line 1155
- line 1203
secondMod, err := svc.GetInstalledMod("test-src", "mod1", game.ID, "second")
require.NoError(t, err)
secondMod.Deployed = true
secondMod.LinkMethod = domain.LinkSymlink
require.NoError(t, svc.SaveInstalledMod(secondMod))
…n in verify_test.go (#94) PR #128 Copilot round 8, three suppressed findings - all one pattern, the same fixture hazard already fixed in setupDoVerifyFixTest during the seventh round's F8 (audit wave), but three more sibling-test setup blocks used it too: fetch the sibling's row, mutate Deployed/LinkMethod on the struct, and call svc.SaveInstalledMod - whose full-row upsert always replaces installed_mod_files (DELETE + a checksum-less re-INSERT), silently wiping any seeded checksum before the test under test even runs. Fixed the three cited sites (TestDoVerify_Fix_VersionMismatch_SiblingProfile_Deployed_Relinks, ..._Deployed_RelinkFails_ClearsDeployedFlag, ..._Deployed_RelinkFails_JSONNotesFailure) with the targeted setters (svc.SetModDeployed / svc.SetModLinkMethod), exactly as the shared fixture already does. Grepped the whole file for other stragglers per the coordinator's instruction and found one more not in the three cited sites: TestDoVerify_Fix_VersionMismatch_PrimaryRelinkFails_SiblingRepaired_JSONNoteVisible did the identical mutate-then-save on the PRIMARY ("default") row. Fixed the same way. All other svc.SaveInstalledMod calls remaining in the file are genuine fresh-row seeding (composite literals for brand-new InstalledMod rows), which is exactly what SaveInstalledMod is for - left untouched. Discovered while adding verification: setupDoVerifyFixSiblingTest never seeded a checksum for "second"/"third" at all (only the "default" profile's fixture, setupDoVerifyVersionTest, called SaveFileChecksum) - so a naive "checksum survived" assertion on the sibling rows would have passed vacuously (empty before, empty after) regardless of whether the fixture bug existed. Fixed the fixture to seed a checksum for both siblings too, matching "default" and making the new assertions load-bearing. Confirmed load-bearing by temporarily reverting the production-fixture fix and re-running: the checksum-survival assertions failed exactly as expected before being restored. Also confirmed doVerify only reports on the ACTIVE profile being verified, so the three sibling-focused tests' captured output (text/JSON) can't observe a wiped SIBLING checksum at all - replaced initial no-op "no NO CHECKSUM in this run's own output" checks with direct svc.GetFilesWithChecksums(gameID, "second") assertions, which actually observe the row regardless of which profile doVerify is checking. The one test mutating the PRIMARY row instead (PrimaryRelinkFails_SiblingRepaired_JSONNoteVisible) keeps its own-output check, since that row IS the one being verified in that run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dFile (#94) The follow-up d1c0e0f explicitly flagged as out of scope: redownloadModFile (cmd/lmm/verify.go, the --fix repair for MISSING and NO CHECKSUM rows) had the identical unmapped svc.GetModFiles(ctx, mod.SourceID, &mod.Mod) call the version-record pre-pass was fixed for in that commit. mod.GameID is the LMM game ID (installed rows persist normalized IDs), but Service.GetModFiles forwards straight to the source with no game-ID translation - unlike Service.GetMod, which maps through game.SourceIDs[sourceID] first (internal/core/service.go). Sources like NexusMods address games by their own domain (e.g. "skyrimspecialedition" vs an LMM ID like "skyrim-se"), so on any game where the mapping differs, every MISSING redownload and NO CHECKSUM repopulation queried the source with the wrong game ID. Fix reuses d1c0e0f's sourceMappedMod(game, mod) helper: the call becomes svc.GetModFiles(ctx, mod.SourceID, sourceMappedMod(game, &mod.Mod)). No signature change needed - redownloadModFile already receives game (it uses it for DownloadMod and SaveFileChecksum). Test: TestDoVerify_Fix_Redownload_MapsGameIDPerSourceMapping (cmd/lmm/verify_test.go), same receivedGameFileIDs capture pattern as d1c0e0f's sibling test, on setupDoVerifyRedownloadTest's MISSING fixture with the per-source mapping overridden to "mapped-domain" (the fixture's default maps to game.ID itself, which is exactly what hid the bug). RED against the unfixed code - the redownload lookup received the LMM ID "g1" instead of "mapped-domain" - then GREEN. The NO CHECKSUM branch routes through the same redownloadModFile call, so the one scenario pins both; the empty-mapping ("applies to any game") rule is already pinned at the helper level by TestDoVerify_VersionCheck_EmptySourceMapping_KeepsLMMGameID. PR #128. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 68 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cmd/lmm/verify.go:335
- The inline comment here is misleading:
repairModVersioncan return a non-nil error after it has already renamed the cache and/or corrected the DB/profile version (e.g., if the later symlink re-link step fails). In that case, the primary row has been partially repaired even though we keep reportingversion_mismatchfor this run. Please update the comment to reflect that partial-success behavior so future readers don’t assume the DB row is unchanged on every error path.
// The PRIMARY row itself wasn't fixed (status/issues
// stay as already recorded above). The failure
// reason itself (audit Finding 7) and note can
// still carry a successful SIBLING repair - see
// repairModVersion's doc comment - and dropping
// either here would make them invisible to a
// --json caller even though they genuinely
// happened/occurred.
…ess (#94) Copilot triage on PR #128 (review 4803429266, suppressed finding): the call-site comment in doVerify's repair-failed branch claimed "The PRIMARY row itself wasn't fixed", but repairModVersion's own doc documents the opposite for its step-4 path - a re-link failure returns an error AFTER the cache rename and the DB/profile version correction have landed, and those are deliberately not rolled back (mod.Version is already mutated by then). Only the reporting claim was wrong; behavior is unchanged. Comment-only change - reworded to say the row keeps reporting version_mismatch even though the repair may have partially succeeded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 68 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cmd/lmm/verify.go:279
- The comment says warnings are unaffected in the source-unreachable branch, but the code increments
warnings++immediately after. This makes the comment misleading for future maintenance and for anyone auditing the warning counts.
// Copilot round 6 (PR #128): the source-unreachable reason
// must reach --json too, not just the text-mode line below
// - status stays "skipped" and warnings is unaffected,
// only the note is new.
…fected (#94) Copilot triage on PR #128 (review 4803446755, suppressed finding): the round-6 comment in the version pre-pass's source-unreachable branch said "warnings is unaffected", meaning the round-6 note addition didn't change the counting - but read on its own it claims the branch doesn't count as a warning, and warnings++ fires for this exact branch four lines below. Reworded to state the actual behavior: the shared warnings++ fires for both output modes, exactly as it did before the note existed. Comment-only change; behavior unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 68 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
cmd/lmm/verify.go:310
- The version-record check treats a mod as "OK" when the matched upstream file entries have empty Version fields, because EffectiveInstalledVersion falls back to the recorded mod.Version (so effective == recorded). For sources that legitimately omit file versions (e.g. custom sources via internal/source/custom/mapping.go), this produces a false negative instead of surfacing that the version is unverifiable.
effective := domain.EffectiveInstalledVersion(mod.Version, matched)
if effective != mod.Version {
cmd/lmm/verify.go:176
- This comment about
checkedis inaccurate: the version-record pre-pass does not incrementcheckedon skip paths (and only increments it on the "recorded version matches" path). Sincecheckedis used for the "No files found" mod-filter message later, the comment should reflect what is actually counted to avoid misleading future changes.
// checked mixes two counters: per-FILE increments from the main loop
// below and per-MOD skip/ok increments from the version-record pre-pass
// above it - both feed the same "no files found" zero-check, so neither
// needed its own variable.
… counts (#94) Copilot triage on PR #128 (review 4803471832, suppressed finding): the comment above the checked counter said the version-record pre-pass adds "per-MOD skip/ok increments", but the pre-pass only increments checked on its quiet-OK path (recorded version matches, verify.go's final line of the pre-pass loop); every skip/mismatch path continues without counting. Reworded to say exactly that. The same review's other suppressed finding (versionless sources pass the version check vacuously) is deliberately NOT addressed here: for sources with no per-file versions the install-time stamping rule falls back the same way, so the mis-stamp this check targets cannot occur - whether the vacuous pass should surface as version_unverifiable instead of quiet OK is a reporting/noise product decision, spun off for separate discussion. Comment-only change; behavior unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dyoung522
added a commit
that referenced
this pull request
Jul 29, 2026
Conflict resolution and reconciliation: - version 1.22.1 -> 1.23.1 (root.go, CHANGELOG entry and compare links) since #128 merged first as 1.23.0 - SetModVersion (from #128) adopts the sweep's per-operation error wording ("setting mod version: checking rows affected: %w") and comment style - the two overlapping mods_test.go inspection notes consolidated into one covering all seven RowsAffected sites - man pages regenerated for the 1.23.1 version string (make man) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 29, 2026
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.
Closes #94. Part of EPIC #98 (version locking) — foundational-fixes tier.
Problem
When a specific file was installed (
--file+--show-archived, or any path where the selected file's version differs from the mod-level latest), the recorded version was the mod-level latest, not the installed file's version. The cache directory was keyed by the same wrong value. Consequence: an archived 1.0 install was recorded and cached as "1.5",lmm listlied, andCheckUpdatessilently suppressed the mod's own update notifications.What changed
Recording fix (forward-only):
domain.EffectiveInstalledVersion— primary selected file's version, else first non-empty file version, else the mod-level version.applyInstallPrimary,applyInstallBatchMod,ApplyProfileSwitch,ApplyImport; CLI:batchInstallMods,doProfileApply) — one stamp per flow fixes the DB row, the profileModReference.Version, and the cache key together.install.before_eachhooks in the core install path now see the effective file version (pinned by test); the update-apply path deliberately still recordsUpdate.NewVersion(loop-breaker for sloppy upstream metadata — revisit under Version→file resolution; make ModReference.Version authoritative #96).Repair for pre-existing bad rows (
lmm verify):FileIDs; reportsversion_mismatch(fixable) andversion_unverifiable(file gone upstream — reinstall hint). Verify now contacts sources for this check; unreachable sources degrade to a warning.--fixrepairs a mismatch: re-keys the shared cache dir, corrects the DB row (full-row save — neverUpdateModVersion, which would poison rollback), updates the profile ref, and re-links symlink deployments. Failure ordering is strict: rename failure → no DB write; re-link failure →Deployedcleared so the state stays detectable; blocked rename (destination dir already exists) → cache and working symlinks left untouched, DB still corrected, surfaced via a new additivenotefield in--json.--json, never swallowed.Docs: README verify section, CHANGELOG 1.23.0, regenerated man pages, version bump to 1.23.0 (MINOR: new verify capability + additive JSON contract).
Notes for review
--fix— nothing touches deploy-time reads.trunk checkwas unavailable in the dev environment; gofmt/vet/full test suite are clean (verified repeatedly, including the man-page drift test).🤖 Generated with Claude Code