From 26f2f7dc361499f24edb7bc01da3b8292e84e5ee Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 14:27:38 -0400 Subject: [PATCH 01/11] feat(icarus): implement MergeCompiler format-vocabulary methods (#256) Adds the five pieces of Unreal/Icarus format knowledge core currently holds - base-pak location, pak fingerprinting, the .pak convert test, merge-source kind classification, and the merged artifact's name/label - as methods on *Icarus, with the kind constants moved into this package. Additive step: internal/source's MergeCompiler interface widens in the next commit, which swings internal/core over to these methods. Co-Authored-By: Claude Fable 5 --- internal/source/icarus/format.go | 110 ++++++++++++++++++++ internal/source/icarus/format_test.go | 142 ++++++++++++++++++++++++++ 2 files changed, 252 insertions(+) create mode 100644 internal/source/icarus/format.go create mode 100644 internal/source/icarus/format_test.go diff --git a/internal/source/icarus/format.go b/internal/source/icarus/format.go new file mode 100644 index 0000000..7a96bf8 --- /dev/null +++ b/internal/source/icarus/format.go @@ -0,0 +1,110 @@ +package icarus + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/DonovanMods/go-unrealpak" + "github.com/DonovanMods/linux-mod-manager/internal/domain" +) + +// This file implements source.MergeCompiler's format-vocabulary methods +// (#256): everything lmm needs to know about Unreal paks and Icarus's disk +// layout to orchestrate merges, moved here from internal/core so that core +// asks the source instead of knowing the format itself. A second +// DeployCompile game supplies its own answers to these questions in its own +// source package; core never changes. + +// Merge-source kinds (#221; moved here from internal/source in #256 - they +// are Icarus vocabulary, and core now treats kinds as opaque strings it +// obtains from ClassifyMergeSource). An empty Kind means MergeSourceExmodz: +// every pre-#221 constructor built exmodz-only sources and never set a kind, +// and stored merge fingerprints from that era carry no Kind field. +const ( + MergeSourceExmodz = "exmodz" + MergeSourcePak = "pak" +) + +// mergedPakFileName names the single merged output pak. It sorts LAST among +// files UE mounts from a profile's mods directory: paks mount in +// filename-sort order and a later mount wins same-path conflicts (this +// package's own icarusContentMountPoint doc comment, and #197's issue body, +// both note this) - "zzz" is a long-standing UE-modding convention for +// "load last, highest priority", so the merged pak's authoritative combined +// table state can never be silently shadowed by a plain prebuilt .pak mod +// that happens to also carry a table override. "LMM" makes the file +// greppable as lmm-owned; "_P" is UE's override-pak suffix convention, +// carried by the real prebuilt Icarus mod paks this package was built +// against (#178). +const mergedPakFileName = "zzz_LMM_Merged_P.pak" + +// ResolveBaseArtifact locates the currently-installed game's base pak - the +// artifact every merge applies against. One known pak filename pattern: the +// relative path below is Task 1's empirically-confirmed finding +// (docs/plans/icarus-pak-format-findings.md), recorded before this function +// was written, not an assumption made here: the JSON data tables live in +// Content/Data/data.pak, NOT in the Content/Paks pakchunks, which carry +// only cooked .uasset/.uexp assets and no JSON at all. +// +// This pak is also the direct source of base table *content* (#175): +// MergeCompile reads each patched table straight out of it, so a compile is +// always week-correct by construction (there's no separate dump to go stale +// relative to the install) and works entirely offline. +func (s *Icarus) ResolveBaseArtifact(game *domain.Game) (string, error) { + candidate := filepath.Join(game.InstallPath, "Icarus", "Content", "Data", "data.pak") + if _, err := os.Stat(candidate); err != nil { + return "", fmt.Errorf("locating base pak for %q: %w", game.ID, err) + } + return candidate, nil +} + +// FingerprintBase opens the base pak and returns its footer IndexHash +// (#196) as an opaque fingerprint - cheap (footer + primary-index region +// only; unrealpak.Open never reads a pak's actual file payloads), matching +// the base pak MergeCompile itself already opens to read patched tables +// from, so this adds no new I/O pattern to the compile path. +func (s *Icarus) FingerprintBase(baseArtifactPath string) (string, error) { + r, err := unrealpak.Open(baseArtifactPath) + if err != nil { + return "", fmt.Errorf("reading base pak for compile fingerprint: %w", err) + } + defer r.Close() //nolint:errcheck + return r.IndexHash(), nil +} + +// IsConvertibleArtifact reports whether fileName names a prebuilt .pak this +// source can convert into a merge source (#221). Pure format test +// (case-insensitive ".pak" suffix, mirroring pre-#256 core's +// isConvertEligiblePakFile) - core owns the DeployCompile/ConvertPaks +// policy gates that decide whether a convertible file actually enters the +// merge-convert pipeline. +func (s *Icarus) IsConvertibleArtifact(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".pak") +} + +// ClassifyMergeSource maps a retained-source identity to its merge-source +// kind (#221) plus whether that kind is convertible (a raw prebuilt pak, as +// opposed to a native .exmodz diff). id is a retained-source fileID - +// download-path icarus fileIDs are literally "pak"/"exmodz", import-path +// fileIDs are the archive's own filename - or a Kind string previously +// recorded on a merge fingerprint (kind strings classify as themselves, and +// a legacy pre-#221 entry's empty Kind classifies as exmodz, the only kind +// that existed then). Unknown ids default to exmodz for the same reason. +func (s *Icarus) ClassifyMergeSource(id string) (kind string, convertible bool) { + lower := strings.ToLower(id) + if lower == "pak" || strings.HasSuffix(lower, ".pak") { + return MergeSourcePak, true + } + return MergeSourceExmodz, false +} + +// MergedArtifactName names the single merged output artifact deployed into +// the game's mods directory - see mergedPakFileName for why this exact name +// is a deploy contract. +func (s *Icarus) MergedArtifactName() string { return mergedPakFileName } + +// MergedArtifactLabel is the user-facing display name for the merged +// artifact's synthetic mod row (verify/update output). +func (s *Icarus) MergedArtifactLabel() string { return "Icarus Merged Pak" } diff --git a/internal/source/icarus/format_test.go b/internal/source/icarus/format_test.go new file mode 100644 index 0000000..7cf2238 --- /dev/null +++ b/internal/source/icarus/format_test.go @@ -0,0 +1,142 @@ +package icarus + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/DonovanMods/go-unrealpak" + "github.com/DonovanMods/linux-mod-manager/internal/domain" +) + +// newFormatTestSource returns an *Icarus suitable for exercising the format +// methods (#256) - none of them touch Firestore, so a nil HTTP client and a +// dummy project ID are fine. +func newFormatTestSource() *Icarus { return New(nil, "test-project") } + +func testGame(id, installPath string) *domain.Game { + return &domain.Game{ID: id, InstallPath: installPath} +} + +func TestResolveBaseArtifact_FindsIcarusDataPak(t *testing.T) { + installDir := t.TempDir() + basePakDir := filepath.Join(installDir, "Icarus", "Content", "Data") + if err := os.MkdirAll(basePakDir, 0755); err != nil { + t.Fatal(err) + } + wantPath := filepath.Join(basePakDir, "data.pak") + if err := os.WriteFile(wantPath, []byte("stub"), 0644); err != nil { + t.Fatal(err) + } + + got, err := newFormatTestSource().ResolveBaseArtifact(testGame("icarus", installDir)) + if err != nil { + t.Fatalf("ResolveBaseArtifact: %v", err) + } + if got != wantPath { + t.Errorf("ResolveBaseArtifact = %q, want %q", got, wantPath) + } +} + +func TestResolveBaseArtifact_MissingPakErrors(t *testing.T) { + _, err := newFormatTestSource().ResolveBaseArtifact(testGame("icarus", t.TempDir())) + if err == nil { + t.Fatal("ResolveBaseArtifact must error when the base pak is absent") + } + // The error names the game, matching core's pre-#256 resolveBasePak + // wrapping ("locating base pak for %q: ...") which callers' messages + // were built around. + if want := `locating base pak for "icarus"`; !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not contain %q", err, want) + } +} + +func TestFingerprintBase_MatchesPakIndexHash(t *testing.T) { + pakPath := writeTestBasePak(t, map[string][]byte{"Data/D_Fixture.json": []byte(`{"fixture":true}`)}) + + got, err := newFormatTestSource().FingerprintBase(pakPath) + if err != nil { + t.Fatalf("FingerprintBase: %v", err) + } + + r, err := unrealpak.Open(pakPath) + if err != nil { + t.Fatal(err) + } + defer r.Close() //nolint:errcheck + if want := r.IndexHash(); got != want { + t.Errorf("FingerprintBase = %q, want the pak's IndexHash %q", got, want) + } + if got == "" { + t.Error("FingerprintBase must not be empty for a valid pak") + } +} + +func TestFingerprintBase_InvalidPakErrors(t *testing.T) { + notAPak := filepath.Join(t.TempDir(), "data.pak") + if err := os.WriteFile(notAPak, []byte("not a pak"), 0644); err != nil { + t.Fatal(err) + } + if _, err := newFormatTestSource().FingerprintBase(notAPak); err == nil { + t.Fatal("FingerprintBase must error on an unparseable pak") + } +} + +func TestIsConvertibleArtifact(t *testing.T) { + tests := map[string]bool{ + "MyMod.pak": true, + "MyMod.PAK": true, // case-insensitive + "MyMod.exmodz": false, + "MyMod.zip": false, + // A bare "pak" is a download-path fileID, not a filename - the + // ingest predicate this backs (pre-#256 isConvertEligiblePakFile) + // was suffix-only, and stays that way. + "pak": false, + } + s := newFormatTestSource() + for fileName, want := range tests { + if got := s.IsConvertibleArtifact(fileName); got != want { + t.Errorf("IsConvertibleArtifact(%q) = %v, want %v", fileName, got, want) + } + } +} + +func TestClassifyMergeSource(t *testing.T) { + tests := map[string]struct { + kind string + convertible bool + }{ + "pak": {MergeSourcePak, true}, // download-path fileID + "MyMod.PAK": {MergeSourcePak, true}, // import-path filename + "exmodz": {MergeSourceExmodz, false}, + "MyMod.exmodz": {MergeSourceExmodz, false}, + "weird.zip": {MergeSourceExmodz, false}, // unknown: exmodz, the only pre-#221 kind + "": {MergeSourceExmodz, false}, // legacy fingerprint entries carry no Kind + } + s := newFormatTestSource() + for id, want := range tests { + kind, convertible := s.ClassifyMergeSource(id) + if kind != want.kind || convertible != want.convertible { + t.Errorf("ClassifyMergeSource(%q) = (%q, %v), want (%q, %v)", + id, kind, convertible, want.kind, want.convertible) + } + } +} + +func TestMergedArtifactName(t *testing.T) { + // The exact name is a deploy contract (#197): it must sort last among + // mounted paks ("zzz"), be greppable as lmm-owned, and keep the "_P" + // override suffix - changing it would orphan already-deployed files. + if got := newFormatTestSource().MergedArtifactName(); got != "zzz_LMM_Merged_P.pak" { + t.Errorf("MergedArtifactName = %q, want %q", got, "zzz_LMM_Merged_P.pak") + } +} + +func TestMergedArtifactLabel(t *testing.T) { + // User-facing display name for the merged artifact's synthetic mod row + // (verify/update output) - pre-#256 core hardcoded this string. + if got := newFormatTestSource().MergedArtifactLabel(); got != "Icarus Merged Pak" { + t.Errorf("MergedArtifactLabel = %q, want %q", got, "Icarus Merged Pak") + } +} From fcba12f5bea25837601abed40a8a5e896ec9e1c4 Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 14:40:20 -0400 Subject: [PATCH 02/11] refactor(core): push Unreal/pak knowledge behind the MergeCompiler seam (#256) Closes all five format leaks the issue enumerates: 1. resolveBasePak (Icarus/Content/Data/data.pak path) -> mc.ResolveBaseArtifact 2. basePakIndexHash (go-unrealpak import in core) -> mc.FingerprintBase 3. isConvertEligiblePakFile's .pak suffix test -> mc.IsConvertibleArtifact (core keeps only the DeployCompile/ConvertPaks policy half) 4. MergeSourceExmodz/MergeSourcePak constants -> moved into the icarus package; core classifies via mc.ClassifyMergeSource and round-trips kinds as opaque strings 5. mergedPakFileName (zzz UE sort-order convention) and the 'Icarus Merged Pak' display name -> mc.MergedArtifactName/MergedArtifactLabel source.MergeCompiler widens from 2 to 8 methods and now states the full contract a second DeployCompile game must implement. Core resolves the compile source lazily on merge paths (only once something retained needs classifying), so no-compiler/no-retained flows keep succeeding exactly as before. ModHasPakMergeSource gains the *domain.Game parameter its classification now requires; callers in cmd/lmm, internal/tui, and moddetail updated. Test accommodations (no behavioral assertions changed): compile-source fakes gain an embedded fakeMergeFormat mirroring the icarus conventions; fixtures for DeployCompile games now map/register a MergeCompiler source (required by the seam where core previously classified statically); moved-symbol references (source.MergeSource* constants, mergedFingerprintsEqual's new classifier arg) updated in place. TestMergeSourceKind's table moved to icarus TestClassifyMergeSource. Co-Authored-By: Claude Fable 5 --- cmd/lmm/install_compile_test.go | 1 + cmd/lmm/list.go | 4 +- cmd/lmm/merge_format_helpers_test.go | 52 ++++ cmd/lmm/mod.go | 2 +- cmd/lmm/mod_convert_test.go | 6 +- internal/core/flows.go | 8 +- .../core/flows_variant_exclusivity_test.go | 1 + internal/core/importer.go | 22 +- internal/core/merge_format_helpers_test.go | 52 ++++ internal/core/merged_pak.go | 223 +++++++++++------- internal/core/merged_pak_import_flow_test.go | 2 + internal/core/merged_pak_internal_test.go | 121 +++++++--- internal/core/merged_pak_test.go | 7 +- internal/core/moddetail.go | 2 +- internal/core/moddetail_test.go | 7 +- internal/core/pak_convert_e2e_test.go | 8 +- internal/core/service.go | 101 ++++---- internal/core/service_icarus_compile_test.go | 2 + internal/core/service_test.go | 1 + internal/source/icarus/merge.go | 2 +- internal/source/icarus/merge_test.go | 10 +- internal/source/source.go | 77 ++++-- internal/tui/merge_format_helpers_test.go | 52 ++++ internal/tui/service_core.go | 2 +- internal/tui/service_core_convert_test.go | 12 +- internal/tui/service_core_recompile_test.go | 2 + internal/tui/service_core_test.go | 4 + 27 files changed, 567 insertions(+), 216 deletions(-) create mode 100644 cmd/lmm/merge_format_helpers_test.go create mode 100644 internal/core/merge_format_helpers_test.go create mode 100644 internal/tui/merge_format_helpers_test.go diff --git a/cmd/lmm/install_compile_test.go b/cmd/lmm/install_compile_test.go index 1491f9d..5973c4f 100644 --- a/cmd/lmm/install_compile_test.go +++ b/cmd/lmm/install_compile_test.go @@ -32,6 +32,7 @@ func writeFakeBasePak(t *testing.T, path string) { // internal/core/service_icarus_compile_test.go's fakeCompilerSource, at the // CLI layer instead of core's). type compilerInstallSource struct { + fakeMergeFormat // #256: the format-vocabulary half of source.MergeCompiler *fakeInstallSource validateCalls int compileCalls int diff --git a/cmd/lmm/list.go b/cmd/lmm/list.go index 9be337b..f09cbd6 100644 --- a/cmd/lmm/list.go +++ b/cmd/lmm/list.go @@ -162,7 +162,7 @@ func doList(cmd *cobra.Command, service *core.Service, game *domain.Game) error LockedVersion: lockedVersion, } // Populate ConvertPaks only for merge-compile games with pak merge source - if game.DeployMode == domain.DeployCompile && service.ModHasPakMergeSource(&mod) { + if game.DeployMode == domain.DeployCompile && service.ModHasPakMergeSource(game, &mod) { v := mod.ConvertPaks row.ConvertPaks = &v } @@ -227,7 +227,7 @@ func doList(cmd *cobra.Command, service *core.Service, game *domain.Game) error locked = lockedRef.Version } convert := "-" - if game.DeployMode == domain.DeployCompile && service.ModHasPakMergeSource(&mod) { + if game.DeployMode == domain.DeployCompile && service.ModHasPakMergeSource(game, &mod) { if mod.ConvertPaks { convert = "on" } else { diff --git a/cmd/lmm/merge_format_helpers_test.go b/cmd/lmm/merge_format_helpers_test.go new file mode 100644 index 0000000..ffeafe5 --- /dev/null +++ b/cmd/lmm/merge_format_helpers_test.go @@ -0,0 +1,52 @@ +package main + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/DonovanMods/go-unrealpak" + "github.com/DonovanMods/linux-mod-manager/internal/domain" +) + +// fakeMergeFormat supplies source.MergeCompiler's format-vocabulary methods +// (#256) for this package's compile-source fakes, mirroring the icarus +// conventions the fixtures already encode (writeFakeBasePak's +// Icarus/Content/Data/data.pak path, "pak"/"exmodz" fileIDs, the +// zzz_LMM_Merged_P.pak artifact name). Embed it in any fake that needs to +// satisfy source.MergeCompiler. Duplicated per test package by design - +// mirrors internal/core's identical helper. +type fakeMergeFormat struct{} + +func (fakeMergeFormat) ResolveBaseArtifact(game *domain.Game) (string, error) { + candidate := filepath.Join(game.InstallPath, "Icarus", "Content", "Data", "data.pak") + if _, err := os.Stat(candidate); err != nil { + return "", fmt.Errorf("locating base pak for %q: %w", game.ID, err) + } + return candidate, nil +} + +func (fakeMergeFormat) FingerprintBase(basePakPath string) (string, error) { + r, err := unrealpak.Open(basePakPath) + if err != nil { + return "", fmt.Errorf("reading base pak for compile fingerprint: %w", err) + } + defer r.Close() //nolint:errcheck + return r.IndexHash(), nil +} + +func (fakeMergeFormat) IsConvertibleArtifact(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".pak") +} + +func (fakeMergeFormat) ClassifyMergeSource(id string) (string, bool) { + lower := strings.ToLower(id) + if lower == "pak" || strings.HasSuffix(lower, ".pak") { + return "pak", true + } + return "exmodz", false +} + +func (fakeMergeFormat) MergedArtifactName() string { return "zzz_LMM_Merged_P.pak" } +func (fakeMergeFormat) MergedArtifactLabel() string { return "Icarus Merged Pak" } diff --git a/cmd/lmm/mod.go b/cmd/lmm/mod.go index 509cf10..5429a9a 100644 --- a/cmd/lmm/mod.go +++ b/cmd/lmm/mod.go @@ -787,7 +787,7 @@ func doModConvert(service *core.Service, game *domain.Game, modID string, conver // Pak conversion only applies to mods with a pak-kind merge source. // Exmodz-only mods have no pak to convert or leave raw, so reject the request. - if !service.ModHasPakMergeSource(mod) { + if !service.ModHasPakMergeSource(game, mod) { return fmt.Errorf("mod %s has no pak merge source: pak conversion does not apply", modID) } diff --git a/cmd/lmm/mod_convert_test.go b/cmd/lmm/mod_convert_test.go index 2501311..3cc6d9f 100644 --- a/cmd/lmm/mod_convert_test.go +++ b/cmd/lmm/mod_convert_test.go @@ -31,7 +31,11 @@ func setupDoModConvertTest(t *testing.T) (*core.Service, *domain.Game, *fakeInst src := newFakeInstallSource("src") t.Cleanup(src.Close) - svc.RegisterSource(src) + // #256: convert-flag surfaces (list/mod show/mod convert) classify + // pak-kind fileIDs via the game's MergeCompiler source, so the + // registered "src" must implement it - wrapping the plain fake keeps + // callers stubbing through the returned inner fake (shared pointer). + svc.RegisterSource(&compilerInstallSource{fakeInstallSource: src}) game := &domain.Game{ ID: "g1", diff --git a/internal/core/flows.go b/internal/core/flows.go index 7905103..5054627 100644 --- a/internal/core/flows.go +++ b/internal/core/flows.go @@ -4157,7 +4157,7 @@ func (s *Service) applyInstallPrimary(ctx context.Context, game *domain.Game, pl if err != nil { return nil, fmt.Errorf("resolving source %q: %w", plan.SourceID, err) } - _, isMergeCompiler := src.(source.MergeCompiler) + mc, isMergeCompiler := src.(source.MergeCompiler) // compiledFiles accumulates every file this loop actually compiled (game // DeployCompile + a ".exmodz" file - the same condition @@ -4211,15 +4211,15 @@ func (s *Service) applyInstallPrimary(ctx context.Context, game *domain.Game, pl downloadedFileIDs = append(downloadedFileIDs, file.ID) // Copilot round 1 (PR #222): compiledFiles collects BOTH kinds - - // .exmodz files and convert-eligible .pak files alike - so the + // .exmodz files and convert-eligible raw files alike - so the // InstallCompiling/"Retaining ... for merge" messaging below (and // in cmd/lmm/install.go's InstallCompiling case) fires for a // convert-eligible raw pak exactly as it does for a native // .exmodz; len(compiledFiles) > 0 doesn't care which kind matched. - // #221: gate .pak files on MergeCompiler capability, matching the + // #221: gate raw files on MergeCompiler capability, matching the // ingest path's predicate. .exmodz files are still included because // they hard-error later if the source lacks MergeCompiler. - if game.DeployMode == domain.DeployCompile && (isExmodzFile(file.FileName) || (isMergeCompiler && isConvertEligiblePakFile(game, file.FileName))) { + if game.DeployMode == domain.DeployCompile && (isExmodzFile(file.FileName) || (isMergeCompiler && isConvertEligibleArtifact(game, mc, file.FileName))) { compiledFiles = append(compiledFiles, file) } } diff --git a/internal/core/flows_variant_exclusivity_test.go b/internal/core/flows_variant_exclusivity_test.go index 2668021..dc6e039 100644 --- a/internal/core/flows_variant_exclusivity_test.go +++ b/internal/core/flows_variant_exclusivity_test.go @@ -32,6 +32,7 @@ import ( // because a single mod must offer BOTH a pak and an exmodz file so // PlanInstall/ApplyInstall can drive a real mixed selection end to end. type variantExclusivitySource struct { + fakeMergeFormat // #256: the format-vocabulary half of source.MergeCompiler *mockSourceWithDownloads files map[string][]domain.DownloadableFile // mod.ID -> served files, verbatim } diff --git a/internal/core/importer.go b/internal/core/importer.go index 4d435fb..352c84d 100644 --- a/internal/core/importer.go +++ b/internal/core/importer.go @@ -120,25 +120,33 @@ func (i *Importer) Import(ctx context.Context, archivePath string, game *domain. // mergeEligible (.exmodz) has no other valid interpretation for a // DeployCompile game - an unresolvable MergeCompiler is a hard error. - // convertEligiblePak (.pak) DOES have one - the legacy extract/copy path - // below - so a resolver failure there falls through instead of erroring - // the whole import (#221 I1 fix, mirrors DownloadModToCache's identical - // fix): resolveMergeCompiler is a GAME-level lookup (Import has no - // per-archive source pinned the way a download does), so "no + // convertEligiblePak (a raw pak) DOES have one - the legacy extract/copy + // path below - so a resolver failure there falls through instead of + // erroring the whole import (#221 I1 fix, mirrors DownloadModToCache's + // identical fix): resolveMergeCompiler is a GAME-level lookup (Import + // has no per-archive source pinned the way a download does), so "no // MergeCompiler-capable source configured for this game" is exactly the // same "fall through for a pak, still hard-error for an exmodz" case as // the download path's "this specific source lacks MergeCompiler". + // + // #256: whether filename IS a convertible artifact is now the compile + // source's call (mc.IsConvertibleArtifact), so resolution is attempted + // for any non-exmodz import into a convert_paks game - the pre-#256 + // static ".pak" pre-filter no longer exists in core. Resolution is a + // pure registry lookup, so the wider trigger changes nothing + // observable: a non-convertible filename still ends up with + // convertEligiblePak == false and the exact same fall-through. mergeEligible := isExmodzFile(filename) var mc source.MergeCompiler var mcErr error - if game.DeployMode == domain.DeployCompile && (mergeEligible || isConvertEligiblePakFile(game, filename)) { + if game.DeployMode == domain.DeployCompile && (mergeEligible || game.ConvertPaks) { if i.resolveMergeCompiler == nil { mcErr = fmt.Errorf("game %q requires DeployCompile to import %q, but this Importer was constructed without service context (via core.NewImporter, not Service.NewImporter) and has no compiler resolver to consult - import via the service-backed importer instead", game.ID, filename) } else { mc, mcErr = i.resolveMergeCompiler(game.ID) } } - convertEligiblePak := mcErr == nil && isConvertEligiblePakFile(game, filename) + convertEligiblePak := mcErr == nil && mc != nil && isConvertEligibleArtifact(game, mc, filename) // Handle based on game's deploy mode if game.DeployMode == domain.DeployCompile && (mergeEligible || convertEligiblePak) { diff --git a/internal/core/merge_format_helpers_test.go b/internal/core/merge_format_helpers_test.go new file mode 100644 index 0000000..7305bed --- /dev/null +++ b/internal/core/merge_format_helpers_test.go @@ -0,0 +1,52 @@ +package core_test + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/DonovanMods/go-unrealpak" + "github.com/DonovanMods/linux-mod-manager/internal/domain" +) + +// fakeMergeFormat supplies source.MergeCompiler's format-vocabulary methods +// (#256) for this package's compile-source fakes, mirroring the icarus +// conventions the fixtures already encode (writeFakeBasePak's +// Icarus/Content/Data/data.pak path, "pak"/"exmodz" fileIDs, the +// zzz_LMM_Merged_P.pak artifact name) without pulling the icarus package +// into internal/core's tests (fakeCompilerSource's own precedent). Embed it +// in any fake that needs to satisfy source.MergeCompiler. +type fakeMergeFormat struct{} + +func (fakeMergeFormat) ResolveBaseArtifact(game *domain.Game) (string, error) { + candidate := filepath.Join(game.InstallPath, "Icarus", "Content", "Data", "data.pak") + if _, err := os.Stat(candidate); err != nil { + return "", fmt.Errorf("locating base pak for %q: %w", game.ID, err) + } + return candidate, nil +} + +func (fakeMergeFormat) FingerprintBase(basePakPath string) (string, error) { + r, err := unrealpak.Open(basePakPath) + if err != nil { + return "", fmt.Errorf("reading base pak for compile fingerprint: %w", err) + } + defer r.Close() //nolint:errcheck + return r.IndexHash(), nil +} + +func (fakeMergeFormat) IsConvertibleArtifact(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".pak") +} + +func (fakeMergeFormat) ClassifyMergeSource(id string) (string, bool) { + lower := strings.ToLower(id) + if lower == "pak" || strings.HasSuffix(lower, ".pak") { + return "pak", true + } + return "exmodz", false +} + +func (fakeMergeFormat) MergedArtifactName() string { return "zzz_LMM_Merged_P.pak" } +func (fakeMergeFormat) MergedArtifactLabel() string { return "Icarus Merged Pak" } diff --git a/internal/core/merged_pak.go b/internal/core/merged_pak.go index b7d7dd2..7c81fcb 100644 --- a/internal/core/merged_pak.go +++ b/internal/core/merged_pak.go @@ -14,13 +14,14 @@ import ( "github.com/DonovanMods/linux-mod-manager/internal/storage/cache" ) -// mergedPakModID/mergedPakVersion/mergedPakFileName identify the merged pak -// as a synthetic, singleton "mod" per (game, profile) - domain.SourceMerged -// is the matching sourceID. This reuses Installer.Install/Uninstall and -// cache.Cache verbatim (#197 design decision 2) rather than a parallel -// deploy/tracking mechanism: zero schema changes, and the SAME -// deployed_files ownership (and #168-class residue risk) as every other -// deployed file. +// mergedPakModID/mergedPakVersion identify the merged pak as a synthetic, +// singleton "mod" per (game, profile) - domain.SourceMerged is the matching +// sourceID. This reuses Installer.Install/Uninstall and cache.Cache +// verbatim (#197 design decision 2) rather than a parallel deploy/tracking +// mechanism: zero schema changes, and the SAME deployed_files ownership +// (and #168-class residue risk) as every other deployed file. The merged +// artifact's on-disk FILENAME is the compile source's business, not core's +// (#256): mc.MergedArtifactName() supplies it wherever it's needed. const ( mergedPakModID = "merged-pak" // mergedPakVersion is fixed ("merged", not a real upstream version) - @@ -28,17 +29,6 @@ const ( // every regeneration REPLACES it outright (mirrors #166's directory- // source "replace, don't overlay" precedent) rather than versioning it. mergedPakVersion = "merged" - // mergedPakFileName sorts LAST among files UE mounts from a profile's - // mods directory: paks mount in filename-sort order and a later mount - // wins same-path conflicts (this repo's own icarusContentMountPoint doc - // comment, and #197's issue body, both note this) - "zzz" is a - // long-standing UE-modding convention for "load last, highest - // priority", so the merged pak's authoritative combined table state can - // never be silently shadowed by a plain prebuilt .pak mod that happens - // to also carry a table override. "LMM" makes the file greppable as - // lmm-owned; "_P" matches this codebase's existing override-pak suffix - // convention (compiledFileName). - mergedPakFileName = "zzz_LMM_Merged_P.pak" ) // MergedFingerprint captures everything a merged pak was built from (#197): @@ -68,33 +58,35 @@ type MergedFingerprintEntry struct { FailReason string `json:",omitempty"` } -// mergeSourceKind classifies a retained-source fileID (#221). Download-path -// icarus fileIDs are literally "pak"/"exmodz"; import-path fileIDs are the -// archive's own filename. Unknown kinds default to exmodz - the only kind -// that existed before #221. -func mergeSourceKind(fileID string) string { - lower := strings.ToLower(fileID) - if lower == "pak" || strings.HasSuffix(lower, ".pak") { - return source.MergeSourcePak - } - return source.MergeSourceExmodz -} - -// ModHasPakMergeSource reports whether mod carries at least one pak-kind -// (source.MergeSourcePak) merge-source fileID, as opposed to being -// exmodz-only (#221 round-4 fix). Pure classification over mod.FileIDs via -// mergeSourceKind - no cache lookups or retained-source disk checks (unlike -// enabledMergeSources, which additionally confirms ingest actually RETAINED -// something). Callers that only need "does pak-conversion state have any -// effect on this mod at all" - e.g. the TUI deciding whether to show the -// "raw" flag or honor the convert-toggle key - want this cheaper check, not -// enabledMergeSources' full retained-file resolution. -func (s *Service) ModHasPakMergeSource(mod *domain.InstalledMod) bool { +// mergeSourceClassifier is the one sliver of source.MergeCompiler the +// package-level fingerprint helpers need: ClassifyMergeSource as a function +// value (a method value like mc.ClassifyMergeSource assigns directly). +// Kept narrow so the helpers stay pure and tests can exercise fingerprint +// semantics without constructing a full source (#256). +type mergeSourceClassifier func(id string) (kind string, convertible bool) + +// ModHasPakMergeSource reports whether mod carries at least one +// convertible-kind (raw pak) merge-source fileID, as opposed to being +// native/exmodz-only (#221 round-4 fix). Classification over mod.FileIDs +// via the game's compile source (#256) - no cache lookups or +// retained-source disk checks (unlike enabledMergeSources, which +// additionally confirms ingest actually RETAINED something). Callers that +// only need "does pak-conversion state have any effect on this mod at all" +// - e.g. the TUI deciding whether to show the "raw" flag or honor the +// convert-toggle key - want this cheaper check, not enabledMergeSources' +// full retained-file resolution. A game with no (or an ambiguous) +// merge-compiler source has no merge sources of any kind, so resolution +// failure is simply false, not an error. +func (s *Service) ModHasPakMergeSource(game *domain.Game, mod *domain.InstalledMod) bool { if mod == nil { return false } + mc, err := s.mergeCompilerForGame(game) + if err != nil { + return false + } for _, fileID := range mod.FileIDs { - if mergeSourceKind(fileID) == source.MergeSourcePak { + if _, convertible := mc.ClassifyMergeSource(fileID); convertible { return true } } @@ -102,14 +94,15 @@ func (s *Service) ModHasPakMergeSource(mod *domain.InstalledMod) bool { } // fingerprintInputs strips outcome fields and normalizes Kind so equality -// judges inputs only. Kind "" and "exmodz" are the same input (pre-#221 -// markers wrote no Kind). -func fingerprintInputs(f MergedFingerprint) MergedFingerprint { +// judges inputs only. A legacy pre-#221 marker entry's empty Kind and the +// source's own default kind are the same input - classify("") returns that +// default (icarus: "exmodz"), per the ClassifyMergeSource contract. +func fingerprintInputs(f MergedFingerprint, classify mergeSourceClassifier) MergedFingerprint { out := MergedFingerprint{BaseIndexHash: f.BaseIndexHash, Mods: make([]MergedFingerprintEntry, len(f.Mods))} for i, m := range f.Mods { kind := m.Kind if kind == "" { - kind = source.MergeSourceExmodz + kind, _ = classify(kind) } out.Mods[i] = MergedFingerprintEntry{SourceID: m.SourceID, ModID: m.ModID, Version: m.Version, Checksum: m.Checksum, Kind: kind} } @@ -159,12 +152,12 @@ func marshalMergedFingerprint(f MergedFingerprint) ([]byte, error) { // inputs, by comparing their marshaled bytes - exactly what "compare // against the stored marker" needs, since the marker itself IS the // marshaled form. -func mergedFingerprintsEqual(a, b MergedFingerprint) (bool, error) { - aBytes, err := marshalMergedFingerprint(fingerprintInputs(a)) +func mergedFingerprintsEqual(a, b MergedFingerprint, classify mergeSourceClassifier) (bool, error) { + aBytes, err := marshalMergedFingerprint(fingerprintInputs(a, classify)) if err != nil { return false, err } - bBytes, err := marshalMergedFingerprint(fingerprintInputs(b)) + bBytes, err := marshalMergedFingerprint(fingerprintInputs(b, classify)) if err != nil { return false, err } @@ -189,6 +182,13 @@ func (s *Service) enabledMergeSources(game *domain.Game, profileName string) ([] } gameCache := s.GetGameCache(game) + // The compile source is resolved lazily, on the first retained file + // found (#256): classification is its business now, but a profile with + // nothing retained has nothing to classify, and must keep working - + // exactly as it did pre-#256 - even for a game whose MergeCompiler + // source isn't configured (syncMergedPak's uninstall-to-zero path runs + // unconditionally from every mutation flow). + var mc source.MergeCompiler var sources []source.MergeSource for _, mod := range mods { if !mod.Enabled { @@ -199,8 +199,14 @@ func (s *Service) enabledMergeSources(game *domain.Game, profileName string) ([] if _, statErr := os.Stat(retainedPath); statErr != nil { continue // not a retained merge source (nothing ingested for this fileID - a legacy-ingest pak, or a non-convert-eligible one) } - kind := mergeSourceKind(fileID) - if kind == source.MergeSourcePak && (!game.ConvertPaks || !mod.ConvertPaks) { + if mc == nil { + var mcErr error + if mc, mcErr = s.mergeCompilerForGame(game); mcErr != nil { + return nil, mcErr + } + } + kind, convertible := mc.ClassifyMergeSource(fileID) + if convertible && (!game.ConvertPaks || !mod.ConvertPaks) { continue // opted out (game- or mod-level): stays raw-deployed (#221) } sources = append(sources, source.MergeSource{ @@ -225,8 +231,8 @@ func (s *Service) EnabledMergeSourcesForTest(game *domain.Game, profileName stri // syncMergedPak regenerates game+profileName's merged pak if its recorded // fingerprint no longer matches the CURRENT enabled-mod set/order/versions/ // base pak (#197). Cheap when nothing changed: the fast path is one -// directory read (enabledMergeSources), one base-pak footer read -// (basePakIndexHash - never the pak's full content), and N small MD5s +// directory read (enabledMergeSources), one base-artifact fingerprint read +// (mc.FingerprintBase - for Icarus a pak footer, never the full content), and N small MD5s // (md5File over each retained .exmodz - real files here are small, see // #175's own research on real base-table sizes), then a byte comparison. // Safe to call unconditionally from ANY mutation flow regardless of game @@ -275,15 +281,24 @@ func (s *Service) syncMergedPak(ctx context.Context, game *domain.Game, profileN return reconWarnings, nil } - basePakPath, err := resolveBasePak(game) + // Non-empty sources imply a resolvable compile source (enabledMergeSources + // already consulted it to classify them), so resolving here - earlier + // than pre-#256, which only needed the source on the slow path below - + // cannot newly fail a flow that used to succeed. + mc, err := s.mergeCompilerForGame(game) + if err != nil { + return nil, err + } + + basePakPath, err := mc.ResolveBaseArtifact(game) if err != nil { return nil, err } cachePath := gameCache.ModPath(game.ID, domain.SourceMerged, mergedPakModID, mergedPakVersion) - deployedPath := filepath.Join(game.ModPath, mergedPakFileName) + deployedPath := filepath.Join(game.ModPath, mc.MergedArtifactName()) if stored, ok := readMergedFingerprint(cachePath); ok { - if eq, eqErr := mergedFingerprintsEqual(current, stored); eqErr == nil && eq { + if eq, eqErr := mergedFingerprintsEqual(current, stored, mc.ClassifyMergeSource); eqErr == nil && eq { // #197 I5 fix: an unchanged fingerprint alone doesn't guarantee // the pak is actually deployed - a PRIOR call's Install could // have failed AFTER the fingerprint was already committed @@ -324,11 +339,6 @@ func (s *Service) syncMergedPak(ctx context.Context, game *domain.Game, profileN } } - mc, err := s.mergeCompilerSourceForGame(game.ID) - if err != nil { - return nil, err - } - stagePath := cachePath + ".staging" if err := os.RemoveAll(stagePath); err != nil { return nil, fmt.Errorf("clearing merged pak staging: %w", err) @@ -338,7 +348,7 @@ func (s *Service) syncMergedPak(ctx context.Context, game *domain.Game, profileN } defer os.RemoveAll(stagePath) //nolint:errcheck - outputPath := filepath.Join(stagePath, mergedPakFileName) + outputPath := filepath.Join(stagePath, mc.MergedArtifactName()) mergeWarnings, mergeFailed, err := mc.MergeCompile(ctx, basePakPath, sources, outputPath) if err != nil { return nil, fmt.Errorf("merging %d merge source(s): %w", len(sources), err) @@ -405,20 +415,33 @@ func (s *Service) reconcilePakManifests(ctx context.Context, game *domain.Game, return nil, fmt.Errorf("loading profile mods: %w", err) } gameCache := s.GetGameCache(game) + // Lazily resolved, like enabledMergeSources (#256): only a mod with a + // retained file has anything to classify, so a profile with nothing + // retained never needs (and pre-#256 never consulted) the compile + // source - the retained-stat therefore runs BEFORE classification, + // flipping the pre-#256 order of two independent, side-effect-free + // filters. + var mc source.MergeCompiler for i := range mods { mod := &mods[i] if !mod.Enabled { continue } for _, fileID := range mod.FileIDs { - if mergeSourceKind(fileID) != source.MergeSourcePak { - continue - } versionDir := gameCache.ModPath(game.ID, mod.SourceID, mod.ID, mod.Version) retained := filepath.Join(versionDir, cache.RetainedSourceName(fileID)) if _, statErr := os.Stat(retained); statErr != nil { continue // nothing retained (legacy ingest): Task 11's needs_reingest covers it } + if mc == nil { + var mcErr error + if mc, mcErr = s.mergeCompilerForGame(game); mcErr != nil { + return warnings, mcErr + } + } + if _, convertible := mc.ClassifyMergeSource(fileID); !convertible { + continue + } ref := mod.SourceID + ":" + mod.ID _, failed := failedByRef[ref] participating := game.ConvertPaks && mod.ConvertPaks && !failed @@ -617,6 +640,9 @@ func (s *Service) ReconcilePakManifestsForTest(ctx context.Context, game *domain // MergedPakOutcomes returns the stored merge fingerprint's per-mod entries // (with #221 conversion outcomes), if a merged pak exists for game+profile. +// The game's compile source interprets the stored Kind strings (#256); a +// stored fingerprint implies a source produced it, so failing to resolve +// one now (unconfigured since) reads as "no outcomes available". func (s *Service) MergedPakOutcomes(game *domain.Game, profileName string) ([]MergedFingerprintEntry, bool) { gameCache := s.GetGameCache(game) cachePath := gameCache.ModPath(game.ID, domain.SourceMerged, mergedPakModID, mergedPakVersion) @@ -624,27 +650,31 @@ func (s *Service) MergedPakOutcomes(game *domain.Game, profileName string) ([]Me if !ok { return nil, false } - return normalizeOutcomes(fp.Mods), true + mc, err := s.mergeCompilerForGame(game) + if err != nil { + return nil, false + } + return normalizeOutcomes(fp.Mods, mc.ClassifyMergeSource), true } -// normalizeOutcomes forces a trivially-successful outcome on every non-pak -// entry (#221 C1 fix): conversion failure is definitionally a pak-kind -// concern (mergeSourceKind(fileID) == source.MergeSourcePak), but a -// pre-#221 fingerprint marker unmarshals its (exmodz-only, at the time) -// entries as Kind:"", Converted:false - those fields didn't exist yet, and -// fingerprintInputs/mergedFingerprintsEqual deliberately never regenerate -// them for an unchanged profile (input equality ignores outcomes). Without -// this normalization, every consumer of MergedPakOutcomes (verify's +// normalizeOutcomes forces a trivially-successful outcome on every +// non-convertible entry (#221 C1 fix): conversion failure is definitionally +// a convertible-kind concern, but a pre-#221 fingerprint marker unmarshals +// its (exmodz-only, at the time) entries as Kind:"", Converted:false - +// those fields didn't exist yet, and fingerprintInputs/ +// mergedFingerprintsEqual deliberately never regenerate them for an +// unchanged profile (input equality ignores outcomes). Without this +// normalization, every consumer of MergedPakOutcomes (verify's // conversion_failed rows, status's conversion-failure counts) would report // a spurious, permanent "CONVERSION FAILED" for every exmodz mod on any // profile that predates #221 - forever, since nothing ever rewrites the // stored marker's outcome fields for inputs that haven't changed. Kind=="" -// is the legacy shape; Kind==MergeSourceExmodz is the current one - both -// are non-pak and therefore always trivially "converted". -func normalizeOutcomes(mods []MergedFingerprintEntry) []MergedFingerprintEntry { +// is the legacy shape and the current native kind is the modern one - the +// classifier maps both to non-convertible, therefore trivially "converted". +func normalizeOutcomes(mods []MergedFingerprintEntry, classify mergeSourceClassifier) []MergedFingerprintEntry { out := make([]MergedFingerprintEntry, len(mods)) for i, m := range mods { - if m.Kind != source.MergeSourcePak { + if _, convertible := classify(m.Kind); !convertible { m.Converted = true m.FailReason = "" } @@ -664,7 +694,11 @@ func (s *Service) PakNeedsReingest(game *domain.Game, mod *domain.InstalledMod, if game.DeployMode != domain.DeployCompile || !game.ConvertPaks || !mod.ConvertPaks { return false, nil } - if mergeSourceKind(fileID) != source.MergeSourcePak { + mc, err := s.mergeCompilerForGame(game) + if err != nil { + return false, err + } + if _, convertible := mc.ClassifyMergeSource(fileID); !convertible { return false, nil } gameCache := s.GetGameCache(game) @@ -715,11 +749,17 @@ func (s *Service) currentMergedFingerprint(game *domain.Game, profileName string return MergedFingerprint{}, sources, nil } - basePakPath, err := resolveBasePak(game) + // Non-empty sources imply enabledMergeSources already resolved the + // compile source, so this cannot newly fail (#256). + mc, err := s.mergeCompilerForGame(game) if err != nil { return MergedFingerprint{}, sources, err } - liveHash, err := basePakIndexHash(basePakPath) + basePakPath, err := mc.ResolveBaseArtifact(game) + if err != nil { + return MergedFingerprint{}, sources, err + } + liveHash, err := mc.FingerprintBase(basePakPath) if err != nil { return MergedFingerprint{}, sources, fmt.Errorf("reading base pak for merge fingerprint: %w", err) } @@ -768,6 +808,13 @@ func (s *Service) CheckMergedPakStaleness(game *domain.Game, profileName string) return nil, nil } + // Non-empty sources imply currentMergedFingerprint already resolved the + // compile source, so this cannot newly fail (#256). + mc, err := s.mergeCompilerForGame(game) + if err != nil { + return nil, err + } + gameCache := s.GetGameCache(game) cachePath := gameCache.ModPath(game.ID, domain.SourceMerged, mergedPakModID, mergedPakVersion) stored, ok := readMergedFingerprint(cachePath) @@ -778,7 +825,7 @@ func (s *Service) CheckMergedPakStaleness(game *domain.Game, profileName string) // the real cause is a missing artifact. reason := "base pak updated" if ok { - if eq, eqErr := mergedFingerprintsEqual(current, stored); eqErr == nil && eq { + if eq, eqErr := mergedFingerprintsEqual(current, stored, mc.ClassifyMergeSource); eqErr == nil && eq { // #197 I5 fix: mirrors syncMergedPak's identical fast-path // check - a matching fingerprint alone doesn't prove the pak // is actually deployed (a prior failed Install, or a purge @@ -786,7 +833,7 @@ func (s *Service) CheckMergedPakStaleness(game *domain.Game, profileName string) // this, `lmm update`/`lmm verify` would report "up to date" // for a profile whose game directory doesn't actually hold // the merged pak at all - the exact wedge this fix closes. - if _, statErr := os.Stat(filepath.Join(game.ModPath, mergedPakFileName)); statErr == nil { + if _, statErr := os.Stat(filepath.Join(game.ModPath, mc.MergedArtifactName())); statErr == nil { return nil, nil } reason = "not deployed" @@ -797,7 +844,7 @@ func (s *Service) CheckMergedPakStaleness(game *domain.Game, profileName string) InstalledMod: domain.InstalledMod{ Mod: domain.Mod{ ID: mergedPakModID, SourceID: domain.SourceMerged, - Name: "Icarus Merged Pak", Version: mergedPakVersion, GameID: game.ID, + Name: mc.MergedArtifactLabel(), Version: mergedPakVersion, GameID: game.ID, }, }, NewVersion: mergedPakVersion, @@ -815,12 +862,20 @@ func (s *Service) CheckMergedPakStaleness(game *domain.Game, profileName string) // protects against). func (s *Service) ApplyMergedPakRegen(ctx context.Context, game *domain.Game, profileName string, progress func(DeployProgress)) (*UpdateApplyResult, error) { result := &UpdateApplyResult{} + // Resolved up front: the Applied entry below reports the merged + // artifact by the name only the compile source knows (#256), and a + // regen request for a game without one is a misconfiguration worth + // failing loud on before touching anything. + mc, err := s.mergeCompilerForGame(game) + if err != nil { + return result, err + } warnings, err := s.syncMergedPak(ctx, game, profileName) if err != nil { return result, err } result.Warnings = warnings - result.Applied = []string{mergedPakFileName} + result.Applied = []string{mc.MergedArtifactName()} if progress != nil { progress(DeployProgress{Phase: UpdateDownloadDone}) } diff --git a/internal/core/merged_pak_import_flow_test.go b/internal/core/merged_pak_import_flow_test.go index 28eeb34..32e023c 100644 --- a/internal/core/merged_pak_import_flow_test.go +++ b/internal/core/merged_pak_import_flow_test.go @@ -21,6 +21,8 @@ import ( // GetMod/GetModFiles as source.ErrNotSupported, ApplyImport's download // loop needs a working GetMod->GetModFiles->DownloadMod chain end to end. type importFlowCompilerSource struct { + fakeMergeFormat // #256: the format-vocabulary half of source.MergeCompiler + mod *domain.Mod fileName string server *httptest.Server diff --git a/internal/core/merged_pak_internal_test.go b/internal/core/merged_pak_internal_test.go index 840fa91..3306c96 100644 --- a/internal/core/merged_pak_internal_test.go +++ b/internal/core/merged_pak_internal_test.go @@ -1,7 +1,9 @@ package core import ( + "context" "os" + "strings" "testing" "github.com/DonovanMods/linux-mod-manager/internal/domain" @@ -9,6 +11,67 @@ import ( "github.com/DonovanMods/linux-mod-manager/internal/storage/cache" ) +// testClassify mirrors the icarus classifier for the fingerprint-helper +// tests below: core no longer owns a merge-source classification of its own +// (#256 - ClassifyMergeSource moved behind the MergeCompiler seam), so the +// package-level helpers take the classifier as a parameter and these tests +// supply the canonical one. +func testClassify(id string) (kind string, convertible bool) { + lower := strings.ToLower(id) + if lower == "pak" || strings.HasSuffix(lower, ".pak") { + return "pak", true + } + return "exmodz", false +} + +// formatOnlyCompilerSource is the minimal source.ModSource + +// source.MergeCompiler TestModHasPakMergeSource needs (#256): +// classification moved behind the MergeCompiler seam, so even the cheap +// FileIDs check resolves the game's compile source now. Only ID and the +// format methods matter; everything else is inert. +type formatOnlyCompilerSource struct{} + +func (formatOnlyCompilerSource) ID() string { return "fake-compiler" } +func (formatOnlyCompilerSource) Name() string { return "Fake Compiler" } +func (formatOnlyCompilerSource) AuthURL() string { return "" } +func (formatOnlyCompilerSource) ExchangeToken(context.Context, string) (*source.Token, error) { + return nil, source.ErrNotSupported +} +func (formatOnlyCompilerSource) Search(context.Context, source.SearchQuery) (source.SearchResult, error) { + return source.SearchResult{}, source.ErrNotSupported +} +func (formatOnlyCompilerSource) GetMod(context.Context, string, string) (*domain.Mod, error) { + return nil, source.ErrNotSupported +} +func (formatOnlyCompilerSource) GetDependencies(context.Context, *domain.Mod) ([]domain.ModReference, error) { + return nil, source.ErrNotSupported +} +func (formatOnlyCompilerSource) GetModFiles(context.Context, *domain.Mod) ([]domain.DownloadableFile, error) { + return nil, source.ErrNotSupported +} +func (formatOnlyCompilerSource) GetDownloadURL(context.Context, *domain.Mod, string) (string, error) { + return "", source.ErrNotSupported +} +func (formatOnlyCompilerSource) CheckUpdates(context.Context, []domain.InstalledMod) ([]domain.Update, error) { + return nil, source.ErrNotSupported +} +func (formatOnlyCompilerSource) ValidateSource(string) error { return nil } +func (formatOnlyCompilerSource) MergeCompile(context.Context, string, []source.MergeSource, string) ([]string, []source.MergeFailure, error) { + return nil, nil, nil +} +func (formatOnlyCompilerSource) ResolveBaseArtifact(*domain.Game) (string, error) { + return "", os.ErrNotExist +} +func (formatOnlyCompilerSource) FingerprintBase(string) (string, error) { return "", os.ErrNotExist } +func (formatOnlyCompilerSource) IsConvertibleArtifact(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".pak") +} +func (formatOnlyCompilerSource) ClassifyMergeSource(id string) (string, bool) { + return testClassify(id) +} +func (formatOnlyCompilerSource) MergedArtifactName() string { return "zzz_LMM_Merged_P.pak" } +func (formatOnlyCompilerSource) MergedArtifactLabel() string { return "Icarus Merged Pak" } + func TestMergedFingerprint_Deterministic(t *testing.T) { f := MergedFingerprint{ BaseIndexHash: "abc123", @@ -35,7 +98,7 @@ func TestMergedFingerprintsEqual_IdenticalInputs(t *testing.T) { BaseIndexHash: "abc123", Mods: []MergedFingerprintEntry{{SourceID: "icarus", ModID: "bear-mount", Version: "1.0", Checksum: "deadbeef"}}, } - eq, err := mergedFingerprintsEqual(f, f) + eq, err := mergedFingerprintsEqual(f, f, testClassify) if err != nil { t.Fatalf("mergedFingerprintsEqual: %v", err) } @@ -48,7 +111,7 @@ func TestMergedFingerprintsEqual_BaseHashChanged(t *testing.T) { a := MergedFingerprint{BaseIndexHash: "abc123", Mods: []MergedFingerprintEntry{{SourceID: "icarus", ModID: "m1", Version: "1.0", Checksum: "x"}}} b := a b.BaseIndexHash = "def456" - eq, err := mergedFingerprintsEqual(a, b) + eq, err := mergedFingerprintsEqual(a, b, testClassify) if err != nil { t.Fatalf("mergedFingerprintsEqual: %v", err) } @@ -63,7 +126,7 @@ func TestMergedFingerprintsEqual_ModSetChanged(t *testing.T) { {SourceID: "icarus", ModID: "m1", Version: "1.0", Checksum: "x"}, {SourceID: "icarus", ModID: "m2", Version: "1.0", Checksum: "y"}, }} - eq, err := mergedFingerprintsEqual(a, b) + eq, err := mergedFingerprintsEqual(a, b, testClassify) if err != nil { t.Fatalf("mergedFingerprintsEqual: %v", err) } @@ -81,7 +144,7 @@ func TestMergedFingerprintsEqual_LoadOrderChanged(t *testing.T) { {SourceID: "icarus", ModID: "m2", Version: "1.0", Checksum: "y"}, {SourceID: "icarus", ModID: "m1", Version: "1.0", Checksum: "x"}, }} - eq, err := mergedFingerprintsEqual(a, b) + eq, err := mergedFingerprintsEqual(a, b, testClassify) if err != nil { t.Fatalf("mergedFingerprintsEqual: %v", err) } @@ -93,7 +156,7 @@ func TestMergedFingerprintsEqual_LoadOrderChanged(t *testing.T) { func TestMergedFingerprintsEqual_VersionChanged(t *testing.T) { a := MergedFingerprint{BaseIndexHash: "abc", Mods: []MergedFingerprintEntry{{SourceID: "icarus", ModID: "m1", Version: "1.0", Checksum: "x"}}} b := MergedFingerprint{BaseIndexHash: "abc", Mods: []MergedFingerprintEntry{{SourceID: "icarus", ModID: "m1", Version: "2.0", Checksum: "x2"}}} - eq, err := mergedFingerprintsEqual(a, b) + eq, err := mergedFingerprintsEqual(a, b, testClassify) if err != nil { t.Fatalf("mergedFingerprintsEqual: %v", err) } @@ -105,7 +168,7 @@ func TestMergedFingerprintsEqual_VersionChanged(t *testing.T) { func TestMergedFingerprintsEqual_EmptyModsBothSides(t *testing.T) { a := MergedFingerprint{BaseIndexHash: "abc", Mods: nil} b := MergedFingerprint{BaseIndexHash: "abc", Mods: []MergedFingerprintEntry{}} - eq, err := mergedFingerprintsEqual(a, b) + eq, err := mergedFingerprintsEqual(a, b, testClassify) if err != nil { t.Fatalf("mergedFingerprintsEqual: %v", err) } @@ -114,22 +177,16 @@ func TestMergedFingerprintsEqual_EmptyModsBothSides(t *testing.T) { } } -func TestMergeSourceKind(t *testing.T) { - tests := map[string]string{ - "pak": source.MergeSourcePak, - "MyMod.PAK": source.MergeSourcePak, - "exmodz": source.MergeSourceExmodz, - "MyMod.exmodz": source.MergeSourceExmodz, - "weird.zip": source.MergeSourceExmodz, // unknown retained kind: today's behavior - } - for fileID, want := range tests { - if got := mergeSourceKind(fileID); got != want { - t.Errorf("mergeSourceKind(%q) = %q, want %q", fileID, got, want) - } - } -} +// The fileID->kind classification itself moved to the icarus package in +// #256 (ClassifyMergeSource) - internal/source/icarus/format_test.go's +// TestClassifyMergeSource carries the old TestMergeSourceKind table. func TestModHasPakMergeSource(t *testing.T) { + reg := source.NewRegistry() + reg.Register(formatOnlyCompilerSource{}) + svc := &Service{registry: reg} + game := &domain.Game{ID: "g", SourceIDs: map[string]string{"fake-compiler": "g"}} + tests := []struct { name string mod *domain.InstalledMod @@ -142,24 +199,30 @@ func TestModHasPakMergeSource(t *testing.T) { {"import-path .pak filename", &domain.InstalledMod{FileIDs: []string{"MyMod.PAK"}}, true}, {"mixed FileIDs, one pak", &domain.InstalledMod{FileIDs: []string{"exmodz", "pak"}}, true}, } - svc := &Service{} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := svc.ModHasPakMergeSource(tt.mod); got != tt.want { + if got := svc.ModHasPakMergeSource(game, tt.mod); got != tt.want { t.Errorf("ModHasPakMergeSource(%+v) = %v, want %v", tt.mod, got, tt.want) } }) } + + // #256: with no merge-compiler-capable source mapped, there is nothing + // to classify against - never true, regardless of FileIDs. + bare := &domain.Game{ID: "bare"} + if svc.ModHasPakMergeSource(bare, &domain.InstalledMod{FileIDs: []string{"pak"}}) { + t.Error("ModHasPakMergeSource must be false for a game with no MergeCompiler source") + } } func TestFingerprintEqualityIgnoresOutcomes(t *testing.T) { a := MergedFingerprint{BaseIndexHash: "h", Mods: []MergedFingerprintEntry{ - {SourceID: "icarus", ModID: "m", Version: "1", Checksum: "c", Kind: source.MergeSourcePak, Converted: true}, + {SourceID: "icarus", ModID: "m", Version: "1", Checksum: "c", Kind: "pak", Converted: true}, }} b := MergedFingerprint{BaseIndexHash: "h", Mods: []MergedFingerprintEntry{ - {SourceID: "icarus", ModID: "m", Version: "1", Checksum: "c", Kind: source.MergeSourcePak, Converted: false, FailReason: "x"}, + {SourceID: "icarus", ModID: "m", Version: "1", Checksum: "c", Kind: "pak", Converted: false, FailReason: "x"}, }} - eq, err := mergedFingerprintsEqual(a, b) + eq, err := mergedFingerprintsEqual(a, b, testClassify) if err != nil { t.Fatal(err) } @@ -168,8 +231,8 @@ func TestFingerprintEqualityIgnoresOutcomes(t *testing.T) { } c := b - c.Mods = []MergedFingerprintEntry{{SourceID: "icarus", ModID: "m", Version: "2", Checksum: "c", Kind: source.MergeSourcePak}} - eq, err = mergedFingerprintsEqual(a, c) + c.Mods = []MergedFingerprintEntry{{SourceID: "icarus", ModID: "m", Version: "2", Checksum: "c", Kind: "pak"}} + eq, err = mergedFingerprintsEqual(a, c, testClassify) if err != nil { t.Fatal(err) } @@ -192,9 +255,9 @@ func TestReadOldFingerprintMarkerCompat(t *testing.T) { t.Fatal("old marker unreadable") } current := MergedFingerprint{BaseIndexHash: "h", Mods: []MergedFingerprintEntry{ - {SourceID: "icarus", ModID: "m", Version: "1", Checksum: "c", Kind: source.MergeSourceExmodz, Converted: true}, + {SourceID: "icarus", ModID: "m", Version: "1", Checksum: "c", Kind: "exmodz", Converted: true}, }} - eq, err := mergedFingerprintsEqual(current, stored) + eq, err := mergedFingerprintsEqual(current, stored, testClassify) if err != nil { t.Fatal(err) } diff --git a/internal/core/merged_pak_test.go b/internal/core/merged_pak_test.go index 7a6da2a..b06f42e 100644 --- a/internal/core/merged_pak_test.go +++ b/internal/core/merged_pak_test.go @@ -21,8 +21,13 @@ import ( // skips a mod's fileIDs that have no retained source (a plain .pak). func TestEnabledMergeSources_OrderMatchesProfileLoadOrderAndSkipsDisabled(t *testing.T) { svc := newFlowsTestService(t) - game := &domain.Game{ID: "icarus", ModPath: t.TempDir(), DeployMode: domain.DeployCompile} + // #256: enabledMergeSources classifies retained fileIDs via the game's + // MergeCompiler source instead of a static suffix test in core, so this + // fixture now maps and registers one. + game := &domain.Game{ID: "icarus", ModPath: t.TempDir(), DeployMode: domain.DeployCompile, + SourceIDs: map[string]string{"fake-compiler": "icarus"}} require.NoError(t, svc.AddGame(game)) + svc.RegisterSource(&fakeCompilerSource{}) gameCache := svc.GetGameCache(game) diff --git a/internal/core/moddetail.go b/internal/core/moddetail.go index eb8691c..c928e46 100644 --- a/internal/core/moddetail.go +++ b/internal/core/moddetail.go @@ -63,7 +63,7 @@ func (s *Service) ModDetail(ctx context.Context, game *domain.Game, profile, sou Profile: profile, UpdatePolicy: installed.UpdatePolicy, } - if game.DeployMode == domain.DeployCompile && s.ModHasPakMergeSource(installed) { + if game.DeployMode == domain.DeployCompile && s.ModHasPakMergeSource(game, installed) { v := installed.ConvertPaks info.ConvertPaks = &v } diff --git a/internal/core/moddetail_test.go b/internal/core/moddetail_test.go index 691eb3e..b8f80eb 100644 --- a/internal/core/moddetail_test.go +++ b/internal/core/moddetail_test.go @@ -20,9 +20,14 @@ import ( func newModDetailTestService(t *testing.T) (*core.Service, *domain.Game, *mockSource) { t.Helper() svc := newFlowsTestService(t) - game := &domain.Game{ID: "testgame", Name: "Test Game", ModPath: t.TempDir(), LinkMethod: domain.LinkSymlink} + game := &domain.Game{ID: "testgame", Name: "Test Game", ModPath: t.TempDir(), LinkMethod: domain.LinkSymlink, + // #256: ModHasPakMergeSource classifies fileIDs via the game's + // MergeCompiler source instead of a static suffix test in core, so + // the DeployCompile subtests need one mapped and registered. + SourceIDs: map[string]string{"fake-compiler": "testgame"}} src := newMockSource("src") svc.RegisterSource(src) + svc.RegisterSource(&fakeCompilerSource{}) return svc, game, src } diff --git a/internal/core/pak_convert_e2e_test.go b/internal/core/pak_convert_e2e_test.go index bc9cc38..bab2721 100644 --- a/internal/core/pak_convert_e2e_test.go +++ b/internal/core/pak_convert_e2e_test.go @@ -80,9 +80,9 @@ func TestPakConvertEndToEnd(t *testing.T) { require.Len(t, rec.lastSources, 2, "MergeCompile must receive both mods") require.Equal(t, "fake-compiler:exmod", rec.lastSources[0].ModRef, "profile load order: exmod first") - require.Equal(t, source.MergeSourceExmodz, rec.lastSources[0].Kind) + require.Equal(t, "exmodz", rec.lastSources[0].Kind) require.Equal(t, "fake-compiler:pakmod", rec.lastSources[1].ModRef, "profile load order: pakmod second") - require.Equal(t, source.MergeSourcePak, rec.lastSources[1].Kind) + require.Equal(t, "pak", rec.lastSources[1].Kind) manifests, err := gameCache.FileManifests(game.ID, "fake-compiler", "pakmod", "1.0") require.NoError(t, err) @@ -202,12 +202,12 @@ func TestNoPakModsByteIdentical(t *testing.T) { require.Equal(t, 1, rec.compileCalls) require.Len(t, rec.lastSources, 1) - require.Equal(t, source.MergeSourceExmodz, rec.lastSources[0].Kind, "MergeCompile must receive Kind set even for a pure-exmodz profile (#221 regression net)") + require.Equal(t, "exmodz", rec.lastSources[0].Kind, "MergeCompile must receive Kind set even for a pure-exmodz profile (#221 regression net)") outcomes, ok := svc.MergedPakOutcomes(game, "default") require.True(t, ok) require.Len(t, outcomes, 1) - require.Equal(t, source.MergeSourceExmodz, outcomes[0].Kind) + require.Equal(t, "exmodz", outcomes[0].Kind) require.True(t, outcomes[0].Converted) require.Empty(t, outcomes[0].FailReason) diff --git a/internal/core/service.go b/internal/core/service.go index 72b58cd..1784adc 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -14,7 +14,6 @@ import ( "strings" "time" - "github.com/DonovanMods/go-unrealpak" "github.com/DonovanMods/linux-mod-manager/internal/domain" "github.com/DonovanMods/linux-mod-manager/internal/linker" "github.com/DonovanMods/linux-mod-manager/internal/source" @@ -204,9 +203,8 @@ func (s *Service) SourcesForGame(gameID string) ([]source.ModSource, error) { // check to the specific source a file was downloaded from // (DownloadModToCache's src.(source.MergeCompiler) check); Importer.Import // has no such per-archive source to key off of, so it resolves against -// every source the game maps in its registry instead — matching -// resolveBasePak's v1 scope of "Icarus only", at most one of a game's -// configured sources implements MergeCompiler today. Zero is the expected +// every source the game maps in its registry instead — at most one of a +// game's configured sources implements MergeCompiler today. Zero is the expected // failure when the game (or its MergeCompiler source) isn't configured; // more than one is treated as ambiguous rather than picking arbitrarily — // both fail loud instead of letting an .exmodz import silently skip @@ -222,6 +220,35 @@ func (s *Service) mergeCompilerSourceForGame(gameID string) (source.MergeCompile compilers = append(compilers, c) } } + return soleMergeCompiler(gameID, compilers) +} + +// mergeCompilerForGame is mergeCompilerSourceForGame for callers that +// already hold the *domain.Game (#256): it resolves against the game +// struct's own source map instead of re-looking the game up in s.games, so +// merged-pak paths that always received their game as a parameter keep +// working for a game value that was never registered with the service (a +// distinction only tests exercise today). Same 0/1/many contract. +func (s *Service) mergeCompilerForGame(game *domain.Game) (source.MergeCompiler, error) { + var compilers []source.MergeCompiler + for id := range game.SourceIDs { + src, err := s.registry.Get(id) + if err != nil { + continue // unregistered: silently skipped, matching SourcesForGame + } + if c, ok := src.(source.MergeCompiler); ok { + compilers = append(compilers, c) + } + } + return soleMergeCompiler(game.ID, compilers) +} + +// soleMergeCompiler enforces the "exactly one compile-capable source per +// game" contract shared by both resolvers above: zero is the expected +// failure when the game's MergeCompiler source isn't configured; more than +// one is treated as ambiguous rather than picking arbitrarily - both fail +// loud instead of letting a compile-path operation silently skip. +func soleMergeCompiler(gameID string, compilers []source.MergeCompiler) (source.MergeCompiler, error) { switch len(compilers) { case 0: return nil, fmt.Errorf("game %q requires DeployCompile but has no merge-compiler-capable source configured (map a source implementing source.MergeCompiler in the game's sources)", gameID) @@ -576,15 +603,15 @@ func (s *Service) DownloadModToCache(ctx context.Context, gameCache *cache.Cache // convertEligiblePak requires BOTH the game's own eligibility (deploy // mode + ConvertPaks) AND this specific src implementing MergeCompiler - // (#221 I1 fix): isConvertEligiblePakFile alone only checks game flags, - // so a .pak served by a source that does NOT implement MergeCompiler + // (#221 I1 fix): the game flags alone don't decide, so a raw pak served + // by a source that does NOT implement MergeCompiler // (a mixed-source game, or a misconfigured/non-icarus source) must fall // through to the legacy extract/copy path below - exactly as it did // before #221 - rather than hard-erroring the whole download. Unlike a // .exmodz file, which has no other valid interpretation and so still // hard-errors when src lacks MergeCompiler (see the !ok check below). mc, isMergeCompiler := src.(source.MergeCompiler) - convertEligiblePak := isMergeCompiler && isConvertEligiblePakFile(game, safeFileName) + convertEligiblePak := isMergeCompiler && isConvertEligibleArtifact(game, mc, safeFileName) if game.DeployMode == domain.DeployCompile && (isExmodzFile(safeFileName) || convertEligiblePak) { if !isMergeCompiler { return nil, fmt.Errorf("source %q: game %q requires DeployCompile but source does not implement MergeCompiler", src.ID(), game.ID) @@ -1048,53 +1075,21 @@ func isExmodzFile(fileName string) bool { return strings.HasSuffix(strings.ToLower(fileName), ".exmodz") } -// isConvertEligiblePakFile reports whether fileName is a prebuilt .pak that -// should enter the merge-convert pipeline (#221): DeployCompile game with -// convert_paks enabled. The per-MOD opt-out is consulted at merge-membership -// time (enabledMergeSources), not here - ingest state is identical either -// way (retained + raw-deployable), only participation differs. This checks -// only game-level flags - callers (DownloadModToCache, Importer.Import) must -// ALSO confirm the actual source/resolver implements source.MergeCompiler -// before treating a pak as convert-eligible; a source that doesn't falls -// through to the legacy extract/copy path instead (#221 I1 fix). -func isConvertEligiblePakFile(game *domain.Game, fileName string) bool { +// isConvertEligibleArtifact reports whether fileName is a prebuilt raw +// artifact that should enter the merge-convert pipeline (#221): DeployCompile +// game with convert_paks enabled, and a file the game's compile-capable +// source says it can convert (mc.IsConvertibleArtifact - the format half of +// the pre-#256 isConvertEligiblePakFile, now behind the MergeCompiler seam; +// the policy half stays here). The per-MOD opt-out is consulted at +// merge-membership time (enabledMergeSources), not here - ingest state is +// identical either way (retained + raw-deployable), only participation +// differs. mc must be the source/resolver actually serving the file: a +// source that does not implement source.MergeCompiler falls through to the +// legacy extract/copy path instead (#221 I1 fix), which callers express by +// never reaching this check without one. +func isConvertEligibleArtifact(game *domain.Game, mc source.MergeCompiler, fileName string) bool { return game.DeployMode == domain.DeployCompile && game.ConvertPaks && - strings.HasSuffix(strings.ToLower(fileName), ".pak") -} - -// resolveBasePak locates the currently-installed game's base pak for -// DeployCompile sources. v1 scope: Icarus only, one known pak filename -// pattern — extend this if a second DeployCompile-using game is ever added -// rather than generalizing speculatively now. The relative path below is -// Task 1's empirically-confirmed finding (docs/plans/icarus-pak-format-findings.md), -// recorded before this function was written, not an assumption made here: the -// JSON data tables live in Content/Data/data.pak, NOT in the Content/Paks -// pakchunks, which carry only cooked .uasset/.uexp assets and no JSON at all. -// -// This pak is also the direct source of base table *content* (#175): Compile -// reads each patched table straight out of it via go-unrealpak, so a -// compile is always week-correct by construction (there's no separate dump -// to go stale relative to the install) and works entirely offline. -func resolveBasePak(game *domain.Game) (string, error) { - candidate := filepath.Join(game.InstallPath, "Icarus", "Content", "Data", "data.pak") - if _, err := os.Stat(candidate); err != nil { - return "", fmt.Errorf("locating base pak for %q: %w", game.ID, err) - } - return candidate, nil -} - -// basePakIndexHash opens basePakPath and returns its footer IndexHash -// (#196) - cheap (footer + primary-index region only; unrealpak.Open never -// reads a pak's actual file payloads), matching the base pak Compile itself -// already opens to read patched tables from, so this adds no new I/O -// pattern to the compile path. -func basePakIndexHash(basePakPath string) (string, error) { - r, err := unrealpak.Open(basePakPath) - if err != nil { - return "", fmt.Errorf("reading base pak for compile fingerprint: %w", err) - } - defer r.Close() //nolint:errcheck - return r.IndexHash(), nil + mc.IsConvertibleArtifact(fileName) } // GetGame retrieves a game by ID diff --git a/internal/core/service_icarus_compile_test.go b/internal/core/service_icarus_compile_test.go index 6f840d0..7b9608d 100644 --- a/internal/core/service_icarus_compile_test.go +++ b/internal/core/service_icarus_compile_test.go @@ -37,6 +37,8 @@ func writeFakeBasePak(t *testing.T, path string) { // needs to prove Service validates and retains (never compiles a per-mod // pak) when DeployMode is DeployCompile (#197: merged-only). type fakeCompilerSource struct { + fakeMergeFormat // #256: the format-vocabulary half of source.MergeCompiler + downloadURL string compileCalls int validateCalls int diff --git a/internal/core/service_test.go b/internal/core/service_test.go index 756c521..ca8de7c 100644 --- a/internal/core/service_test.go +++ b/internal/core/service_test.go @@ -1000,6 +1000,7 @@ func (m *mockSourceWithDownloads) Close() { // takes DownloadModToCache's ordinary copy path, and a later .exmodz // re-download that takes its validate+retain (#197) path. type compilerMockSource struct { + fakeMergeFormat // #256: the format-vocabulary half of source.MergeCompiler *mockSourceWithDownloads } diff --git a/internal/source/icarus/merge.go b/internal/source/icarus/merge.go index 18368bf..9e066f5 100644 --- a/internal/source/icarus/merge.go +++ b/internal/source/icarus/merge.go @@ -89,7 +89,7 @@ func MergeCompile(ctx context.Context, basePakPath string, sources []MergeSource if label == "" { label = src.ModRef } - if src.Kind == source.MergeSourcePak { + if src.Kind == MergeSourcePak { bundle, convWarnings, cerr := convertPakToBundle(src.SourcePath, base, baseFold) if cerr != nil { failed = append(failed, source.MergeFailure{ModRef: src.ModRef, Reason: cerr.Error()}) diff --git a/internal/source/icarus/merge_test.go b/internal/source/icarus/merge_test.go index 5278cad..4742e1f 100644 --- a/internal/source/icarus/merge_test.go +++ b/internal/source/icarus/merge_test.go @@ -322,7 +322,7 @@ func TestMergeCompilePakSource(t *testing.T) { out := filepath.Join(dir, "merged.pak") warnings, failed, err := MergeCompile(context.Background(), basePath, []MergeSource{ - {ModRef: "icarus:pakmod", SourcePath: pakPath, Kind: source.MergeSourcePak}, + {ModRef: "icarus:pakmod", SourcePath: pakPath, Kind: MergeSourcePak}, }, out) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -360,8 +360,8 @@ func TestMergeCompilePakFailureSkipsModOnly(t *testing.T) { out := filepath.Join(dir, "merged.pak") warnings, failed, err := MergeCompile(context.Background(), basePath, []MergeSource{ - {ModRef: "icarus:bad", SourcePath: badPak, Kind: source.MergeSourcePak}, - {ModRef: "icarus:good", SourcePath: goodPak, Kind: source.MergeSourcePak}, + {ModRef: "icarus:bad", SourcePath: badPak, Kind: MergeSourcePak}, + {ModRef: "icarus:good", SourcePath: goodPak, Kind: MergeSourcePak}, }, out) if err != nil { t.Fatalf("per-mod failure must not be fatal: %v", err) @@ -433,8 +433,8 @@ func TestMergeCompileWarningsPreferModName(t *testing.T) { out := filepath.Join(dir, "merged.pak") warnings, failed, err := MergeCompile(context.Background(), basePath, []MergeSource{ - {ModRef: "icarus:pakmod", ModName: "Combined QOL", SourcePath: pakPath, Kind: source.MergeSourcePak}, - {ModRef: "icarus:badmod", ModName: "Broken Mod", SourcePath: badPak, Kind: source.MergeSourcePak}, + {ModRef: "icarus:pakmod", ModName: "Combined QOL", SourcePath: pakPath, Kind: MergeSourcePak}, + {ModRef: "icarus:badmod", ModName: "Broken Mod", SourcePath: badPak, Kind: MergeSourcePak}, }, out) if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/internal/source/source.go b/internal/source/source.go index 9b44782..8249b3e 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -150,6 +150,17 @@ type DownloadHeaderProvider interface { // mods patch the same table). Replaces #196's Compiler interface, which // this source no longer implements: there is no more per-mod compiled // artifact to produce. +// +// This interface is the complete contract a DeployCompile game must +// implement (#256): the merge operations (ValidateSource, MergeCompile) +// plus the format vocabulary core needs to orchestrate them without knowing +// the game's artifact format itself - where the base artifact lives +// (ResolveBaseArtifact), how to fingerprint it (FingerprintBase), which +// files are convertible raw artifacts (IsConvertibleArtifact, +// ClassifyMergeSource), and what the merged output is called +// (MergedArtifactName, MergedArtifactLabel). A second compile-mode game is +// a new source package implementing these methods plus one registration +// line; internal/core never changes. type MergeCompiler interface { // ValidateSource parses/validates sourceFilePath (the retained, // not-yet-merged source archive) without compiling anything - called at @@ -158,29 +169,63 @@ type MergeCompiler interface { ValidateSource(sourceFilePath string) error // MergeCompile applies every entry in sources, in order (profile load - // order), against basePakPath's tables, and writes the merged result to - // outputPakPath. Returns non-fatal warnings (e.g. same-path asset - // collisions - last-applied wins) alongside a nil error; a nil error - // with warnings is still a fully-written, deployable pak. Pak-kind - // sources that cannot be converted are skipped per-mod and reported in - // failed (#221) - only exmodz-source errors and I/O failures are fatal. - MergeCompile(ctx context.Context, basePakPath string, sources []MergeSource, outputPakPath string) (warnings []string, failed []MergeFailure, err error) + // order), against the base artifact's tables, and writes the merged + // result to outputPath. Returns non-fatal warnings (e.g. same-path + // asset collisions - last-applied wins) alongside a nil error; a nil + // error with warnings is still a fully-written, deployable artifact. + // Convertible-kind sources that cannot be converted are skipped per-mod + // and reported in failed (#221) - only native-source errors and I/O + // failures are fatal. + MergeCompile(ctx context.Context, baseArtifactPath string, sources []MergeSource, outputPath string) (warnings []string, failed []MergeFailure, err error) + + // ResolveBaseArtifact locates the installed game's base artifact - the + // input every merge applies against (Icarus: the game's own + // Content/Data/data.pak). Errors when the artifact cannot be found + // under the game's install path. + ResolveBaseArtifact(game *domain.Game) (string, error) + + // FingerprintBase returns an opaque fingerprint of the base artifact at + // baseArtifactPath: cheap to compute, changing exactly when the base + // content changes. Core stores it in merge fingerprints to detect a + // game-update-invalidated merge; it never interprets the value. + FingerprintBase(baseArtifactPath string) (string, error) + + // IsConvertibleArtifact reports whether fileName names a raw, prebuilt + // game artifact this source can convert into a merge source (#221; + // Icarus: a ".pak" file). Pure format test - core owns the + // DeployCompile/ConvertPaks policy gates that decide whether such a + // file actually enters the merge-convert pipeline. + IsConvertibleArtifact(fileName string) bool + + // ClassifyMergeSource maps a retained-source identity - a fileID, an + // imported archive's filename, or a Kind string previously recorded on + // a merge fingerprint - to the source-defined kind string core + // round-trips (MergeSource.Kind, fingerprint entries) and whether that + // kind is a convertible raw artifact (subject to the ConvertPaks + // opt-out and per-mod conversion outcomes) as opposed to the source's + // native mergeable format. Must accept the empty string (legacy + // fingerprints recorded no Kind) and classify it as the native kind. + ClassifyMergeSource(id string) (kind string, convertible bool) + + // MergedArtifactName names the single merged output artifact core + // deploys into the game's mod directory. The name is a deploy contract: + // it must stay stable across merges (core stats/replaces it by name), + // and any load-order significance it carries (Icarus: sorts last so it + // wins) is entirely the source's concern. + MergedArtifactName() string + + // MergedArtifactLabel is the user-facing display name for the merged + // artifact's synthetic mod row (verify/update output). + MergedArtifactLabel() string } -// Merge-source kinds (#221). An empty Kind means MergeSourceExmodz - every -// pre-#221 constructor built exmodz-only sources and never set a kind. -const ( - MergeSourceExmodz = "exmodz" - MergeSourcePak = "pak" -) - // MergeSource identifies one mod's contribution to a merge, in the order it // must be applied (profile load order). type MergeSource struct { ModRef string // "sourceID:modID" - machine identity (MergeFailure, ownership tracking) ModName string // display name preferred over ModRef in user-facing warnings; may be empty - SourcePath string // the retained source archive to read (.exmodz, or a raw .pak eligible for conversion - #221) - Kind string // MergeSourceExmodz (default when empty) or MergeSourcePak + SourcePath string // the retained source archive to read (native diff, or a convertible raw artifact - #221) + Kind string // source-defined kind from ClassifyMergeSource; empty means the source's native kind (#256) } // MergeFailure records one source that could not participate in a merge diff --git a/internal/tui/merge_format_helpers_test.go b/internal/tui/merge_format_helpers_test.go new file mode 100644 index 0000000..1a87f92 --- /dev/null +++ b/internal/tui/merge_format_helpers_test.go @@ -0,0 +1,52 @@ +package tui_test + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/DonovanMods/go-unrealpak" + "github.com/DonovanMods/linux-mod-manager/internal/domain" +) + +// fakeMergeFormat supplies source.MergeCompiler's format-vocabulary methods +// (#256) for this package's compile-source fakes, mirroring the icarus +// conventions the fixtures already encode (the Icarus/Content/Data/data.pak +// base path, "pak"/"exmodz" fileIDs, the zzz_LMM_Merged_P.pak artifact +// name). Embed it in any fake that needs to satisfy source.MergeCompiler. +// Duplicated per test package by design - mirrors internal/core's identical +// helper. +type fakeMergeFormat struct{} + +func (fakeMergeFormat) ResolveBaseArtifact(game *domain.Game) (string, error) { + candidate := filepath.Join(game.InstallPath, "Icarus", "Content", "Data", "data.pak") + if _, err := os.Stat(candidate); err != nil { + return "", fmt.Errorf("locating base pak for %q: %w", game.ID, err) + } + return candidate, nil +} + +func (fakeMergeFormat) FingerprintBase(basePakPath string) (string, error) { + r, err := unrealpak.Open(basePakPath) + if err != nil { + return "", fmt.Errorf("reading base pak for compile fingerprint: %w", err) + } + defer r.Close() //nolint:errcheck + return r.IndexHash(), nil +} + +func (fakeMergeFormat) IsConvertibleArtifact(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".pak") +} + +func (fakeMergeFormat) ClassifyMergeSource(id string) (string, bool) { + lower := strings.ToLower(id) + if lower == "pak" || strings.HasSuffix(lower, ".pak") { + return "pak", true + } + return "exmodz", false +} + +func (fakeMergeFormat) MergedArtifactName() string { return "zzz_LMM_Merged_P.pak" } +func (fakeMergeFormat) MergedArtifactLabel() string { return "Icarus Merged Pak" } diff --git a/internal/tui/service_core.go b/internal/tui/service_core.go index 70a11a9..605839e 100644 --- a/internal/tui/service_core.go +++ b/internal/tui/service_core.go @@ -166,7 +166,7 @@ func (p *coreProvider) Overview(_ context.Context) (Summary, []ModItem, error) { ConvertPaks: mod.ConvertPaks, CompileGame: game.DeployMode == domain.DeployCompile, GameConvertPaks: game.ConvertPaks, - HasPakSource: p.svc.ModHasPakMergeSource(&mod), + HasPakSource: p.svc.ModHasPakMergeSource(game, &mod), // This row came from the installed-mods list, so its // install-state fields above (Version/UpdatePolicy, plus // Locked/LockedVersion set below) are genuine local install diff --git a/internal/tui/service_core_convert_test.go b/internal/tui/service_core_convert_test.go index f94ffb3..d8cad8c 100644 --- a/internal/tui/service_core_convert_test.go +++ b/internal/tui/service_core_convert_test.go @@ -23,11 +23,11 @@ import ( // the two independently constructed instances always observe the same // underlying DB/filesystem truth - letting this test mutate via one and // re-read via the other. Unlike newRecompileActionsFixture -// (service_core_recompile_test.go), no merge-compiler source is registered -// and no base pak is written: this test never deploys or merges anything, -// so that machinery would be pure overhead - mirrors cmd/lmm/mod_convert_ -// test.go's setupDoModConvertTest, the CLI's own lightweight convert -// fixture. +// (service_core_recompile_test.go), no base pak is written: this test never +// deploys or merges anything. A merge-compiler source IS registered (#256: +// HasPakSource classification resolves the game's MergeCompiler source), +// but stays otherwise inert - mirrors cmd/lmm/mod_convert_test.go's +// setupDoModConvertTest, the CLI's own lightweight convert fixture. func newConvertActionsFixture(t *testing.T) (tui.ActionProvider, tui.DataProvider, *core.Service, *domain.Game) { t.Helper() @@ -47,8 +47,10 @@ func newConvertActionsFixture(t *testing.T) (tui.ActionProvider, tui.DataProvide LinkMethod: domain.LinkSymlink, DeployMode: domain.DeployCompile, ConvertPaks: true, + SourceIDs: map[string]string{"fake-compiler": "icarus"}, } require.NoError(t, svc.AddGame(game)) + svc.RegisterSource(&recompileFakeSource{}) pm := svc.NewProfileManager() _, err = pm.Create(game.ID, "default") diff --git a/internal/tui/service_core_recompile_test.go b/internal/tui/service_core_recompile_test.go index 03ce9a4..01ac919 100644 --- a/internal/tui/service_core_recompile_test.go +++ b/internal/tui/service_core_recompile_test.go @@ -20,6 +20,8 @@ import ( // internal/core/service_icarus_compile_test.go's fakeCompilerSource at the // TUI layer. type recompileFakeSource struct { + fakeMergeFormat // #256: the format-vocabulary half of source.MergeCompiler + validateCalls int compileCalls int } diff --git a/internal/tui/service_core_test.go b/internal/tui/service_core_test.go index 3601dbe..53d7e3b 100644 --- a/internal/tui/service_core_test.go +++ b/internal/tui/service_core_test.go @@ -2642,6 +2642,10 @@ func TestCoreProviderGetModDetails_NotInstalled(t *testing.T) { func TestCoreProviderGetModDetails_InstalledUsesPolicyToString(t *testing.T) { provider, svc, game, netSrc := newCoreDetailsFixture(t) game.DeployMode = domain.DeployCompile + // #256: the ConvertPaks detail field classifies pak-kind fileIDs via + // the game's MergeCompiler source, so this compile-game test maps one. + game.SourceIDs = map[string]string{"fake-compiler": game.ID} + svc.RegisterSource(&recompileFakeSource{}) netSrc.addMod(game.ID, &domain.Mod{ID: "modA", SourceID: "src", GameID: game.ID, Name: "Mod A", Version: "1.5"}) require.NoError(t, svc.SaveInstalledMod(&domain.InstalledMod{ Mod: domain.Mod{ID: "modA", SourceID: "src", GameID: game.ID, Name: "Mod A", Version: "1.5"}, From 73726897b70cabfe606f7e7c54d86a7dc096fcbb Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 14:40:42 -0400 Subject: [PATCH 03/11] docs: CHANGELOG entry for #256 Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd31251..fa3379e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Internal: all Unreal/Icarus format knowledge (base-pak location, pak + fingerprinting, the `.pak` convert test, merge-source kinds, and the + merged artifact's filename) moved out of `internal/core` and behind the + `source.MergeCompiler` interface, which now states the complete contract + a second compile-mode game would implement (#256). No user-visible + change. + ## [1.30.0] - 2026-08-08 ### Added From e7400e992c46273e38007bdfbd574a29e8596814 Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 14:41:45 -0400 Subject: [PATCH 04/11] docs(core): fix stale isConvertEligiblePakFile comment references Co-Authored-By: Claude Fable 5 --- internal/core/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/core/service.go b/internal/core/service.go index 1784adc..8f2f291 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -1064,10 +1064,10 @@ func commitStagedCache(cachePath, stagePath string) error { // before "exmodz") - those must NOT be routed through this function's own // validate+retain branch as an exmodz, since MergeCompile expects an exmodz // diff, not a whole pak (#136 review, Task 13 fix round 1). A prebuilt pak -// gets its OWN eligibility check instead - isConvertEligiblePakFile (#221) - +// gets its OWN eligibility check instead - isConvertEligibleArtifact (#221) - // which the same validate+retain branch also widens for: a convert-eligible // pak still enters ingest's validate+retain machinery (a different -// isExmodzFile-vs-isConvertEligiblePakFile Kind, not a different branch), +// isExmodzFile-vs-isConvertEligibleArtifact Kind, not a different branch), // while a non-eligible pak (ConvertPaks off, or a non-DeployCompile game) // falls through to the pre-compile extract/copy logic unchanged, exactly as // if DeployMode were not DeployCompile at all. From 09b8c6e0bc4e28c3b8904676b2c99cc9382798ed Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 16:02:59 -0400 Subject: [PATCH 05/11] refactor(core): route the native .exmodz test through the MergeCompiler seam (#256) Closes the sixth leak: core's static isExmodzFile hardcoded Icarus's NATIVE merge-source format, so a second DeployCompile game's native files would silently never reach the validate+retain branch - the exact core-edit-per-game outcome #256 exists to prevent. A new IsNativeMergeSource(fileName) method (the exact mirror of IsConvertibleArtifact) joins source.MergeCompiler rather than widening ClassifyMergeSource: ClassifyMergeSource must default UNKNOWN ids to the native kind (legacy fingerprint compat), so its result cannot distinguish 'genuinely native archive' from 'unknown junk' - using it for ingest routing would send every unrecognized file into validate+retain. All four call sites now go through the seam, via a new Service.isNativeMergeFile helper that prefers the file's own source's view and falls back to the game's sole compile source - keeping the 'native archive from a non-compile-capable source' download hard error reachable on mixed-source games. Two deliberate behavior deltas, both confined to games with NO resolvable compile source (broken config), where nothing is left to define 'native': - Importing a native archive into such a game still fails loud and still caches nothing, but with the legacy path's 'unsupported archive format: .exmodz' (the Extractor is extension-keyed) instead of the old compiler-specific message - that message required core itself to know the extension. TestImportMod_DeployCompile_NoCompilerSourceFailsLoud updated accordingly; pak/zip no-compiler imports are byte-identical. - A standalone core.NewImporter (nil resolver) now fails loud for EVERY DeployCompile import up front, preserving its original error verbatim; production only ever constructs the service-backed importer. The mixed-selection message loses its exmodz vocabulary: 'raw pak and native merge archive are alternate forms of the same mod - select one' (the 'alternate forms of the same mod' substring callers match on is unchanged). Co-Authored-By: Claude Fable 5 --- cmd/lmm/merge_format_helpers_test.go | 4 ++ internal/core/flows.go | 8 ++- .../core/flows_variant_exclusivity_test.go | 6 +- internal/core/importer.go | 64 +++++++++---------- internal/core/merge_format_helpers_test.go | 4 ++ internal/core/merged_pak.go | 2 +- internal/core/merged_pak_internal_test.go | 3 + internal/core/service.go | 58 ++++++++++------- internal/core/service_import_compile_test.go | 18 +++++- internal/source/icarus/format.go | 10 +++ internal/source/icarus/format_test.go | 19 ++++++ internal/source/icarus/icarus.go | 2 +- internal/source/source.go | 8 +++ internal/tui/merge_format_helpers_test.go | 4 ++ 14 files changed, 145 insertions(+), 65 deletions(-) diff --git a/cmd/lmm/merge_format_helpers_test.go b/cmd/lmm/merge_format_helpers_test.go index ffeafe5..56bff99 100644 --- a/cmd/lmm/merge_format_helpers_test.go +++ b/cmd/lmm/merge_format_helpers_test.go @@ -36,6 +36,10 @@ func (fakeMergeFormat) FingerprintBase(basePakPath string) (string, error) { return r.IndexHash(), nil } +func (fakeMergeFormat) IsNativeMergeSource(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".exmodz") +} + func (fakeMergeFormat) IsConvertibleArtifact(fileName string) bool { return strings.HasSuffix(strings.ToLower(fileName), ".pak") } diff --git a/internal/core/flows.go b/internal/core/flows.go index 5054627..0acf827 100644 --- a/internal/core/flows.go +++ b/internal/core/flows.go @@ -4217,9 +4217,11 @@ func (s *Service) applyInstallPrimary(ctx context.Context, game *domain.Game, pl // convert-eligible raw pak exactly as it does for a native // .exmodz; len(compiledFiles) > 0 doesn't care which kind matched. // #221: gate raw files on MergeCompiler capability, matching the - // ingest path's predicate. .exmodz files are still included because - // they hard-error later if the source lacks MergeCompiler. - if game.DeployMode == domain.DeployCompile && (isExmodzFile(file.FileName) || (isMergeCompiler && isConvertEligibleArtifact(game, mc, file.FileName))) { + // ingest path's predicate. Native merge archives are still included + // when only the GAME's compile source (not this file's own source) + // recognizes them - isNativeMergeFile's fallback - because ingest + // hard-errors on exactly that mismatch later (#256). + if game.DeployMode == domain.DeployCompile && (s.isNativeMergeFile(game, mc, file.FileName) || (isMergeCompiler && isConvertEligibleArtifact(game, mc, file.FileName))) { compiledFiles = append(compiledFiles, file) } } diff --git a/internal/core/flows_variant_exclusivity_test.go b/internal/core/flows_variant_exclusivity_test.go index dc6e039..e9f7224 100644 --- a/internal/core/flows_variant_exclusivity_test.go +++ b/internal/core/flows_variant_exclusivity_test.go @@ -142,7 +142,7 @@ func TestApplyInstall_StrictPath_TargetFileIDs_MixedVariants_Rejected(t *testing opts := core.InstallOptions{TargetFileIDs: []string{"pak", "exmodz"}} _, err = svc.ApplyInstall(context.Background(), game, plan, opts, nil) - require.ErrorContains(t, err, "pak and exmodz are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "raw pak and native merge archive are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) @@ -167,7 +167,7 @@ func TestApplyInstall_StrictPath_CallerSuppliedMixedVariantFiles_Rejected(t *tes plan.Files = mc.files["mod1"] _, err = svc.ApplyInstall(context.Background(), game, plan, core.InstallOptions{}, nil) - require.ErrorContains(t, err, "pak and exmodz are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "raw pak and native merge archive are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) @@ -204,7 +204,7 @@ func TestApplyInstall_BatchPath_TargetFileIDs_MixedVariants_Rejected(t *testing. opts := core.InstallOptions{TargetFileIDs: []string{"pak", "exmodz"}} _, err = svc.ApplyInstall(context.Background(), game, plan, opts, nil) - require.ErrorContains(t, err, "pak and exmodz are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "raw pak and native merge archive are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) diff --git a/internal/core/importer.go b/internal/core/importer.go index 352c84d..c262b98 100644 --- a/internal/core/importer.go +++ b/internal/core/importer.go @@ -49,13 +49,14 @@ type Importer struct { // cache. Empty means fall back to $TMPDIR — see newStagingDir. stagingRoot string // resolveMergeCompiler resolves the MergeCompiler-capable source mapped - // to a DeployCompile game's registry entry (#197), consulted only when - // importing a ".exmodz" archive for such a game — Import has no - // per-archive source pinned the way DownloadModToCache does, so it must - // look up the game's configured sources instead. nil when the Importer - // was built via the standalone NewImporter (no Service context): - // importing an .exmodz through such an Importer fails loud rather than - // silently caching an unvalidated archive. + // to a DeployCompile game's registry entry (#197), consulted for every + // import into such a game (#256: the compile source now answers the + // native/convertible format questions too) — Import has no per-archive + // source pinned the way DownloadModToCache does, so it must look up the + // game's configured sources instead. nil when the Importer was built + // via the standalone NewImporter (no Service context): a DeployCompile + // import through such an Importer fails loud rather than silently + // caching an unvalidated archive. resolveMergeCompiler func(gameID string) (source.MergeCompiler, error) } @@ -118,34 +119,36 @@ func (i *Importer) Import(ctx context.Context, archivePath string, game *domain. var fileCount int var retainedFileID string - // mergeEligible (.exmodz) has no other valid interpretation for a - // DeployCompile game - an unresolvable MergeCompiler is a hard error. - // convertEligiblePak (a raw pak) DOES have one - the legacy extract/copy - // path below - so a resolver failure there falls through instead of - // erroring the whole import (#221 I1 fix, mirrors DownloadModToCache's - // identical fix): resolveMergeCompiler is a GAME-level lookup (Import - // has no per-archive source pinned the way a download does), so "no - // MergeCompiler-capable source configured for this game" is exactly the - // same "fall through for a pak, still hard-error for an exmodz" case as - // the download path's "this specific source lacks MergeCompiler". + // #256: whether filename is the game's NATIVE merge format + // (mc.IsNativeMergeSource - the seam-routed successor to core's static + // ".exmodz" test) or a convertible artifact (mc.IsConvertibleArtifact) + // is the compile source's call, so the compiler is resolved up front + // for every DeployCompile import. Resolution is a pure registry + // lookup; when it fails, nothing can define either format, so BOTH + // predicates are false and the import falls through to the legacy + // extract/copy path: a raw pak or plain zip behaves exactly as before + // (#221 I1 fix - "unsupported archive format" for the pak, a normal + // extract for the zip), and a native archive - unextractable, since + // the Extractor is extension-keyed - still fails loud with + // "unsupported archive format" rather than the old compiler-specific + // message, and still caches nothing. // - // #256: whether filename IS a convertible artifact is now the compile - // source's call (mc.IsConvertibleArtifact), so resolution is attempted - // for any non-exmodz import into a convert_paks game - the pre-#256 - // static ".pak" pre-filter no longer exists in core. Resolution is a - // pure registry lookup, so the wider trigger changes nothing - // observable: a non-convertible filename still ends up with - // convertEligiblePak == false and the exact same fall-through. - mergeEligible := isExmodzFile(filename) + // A NIL RESOLVER is different: core.NewImporter (no Service context) + // cannot answer the format question for ANY file, and unlike a + // misconfigured game there is a correct importer to use instead - so a + // DeployCompile import through a standalone Importer fails loud + // unconditionally. Production only ever constructs the service-backed + // importer (Service.NewImporter); this path is reachable only by + // direct core.NewImporter use. var mc source.MergeCompiler var mcErr error - if game.DeployMode == domain.DeployCompile && (mergeEligible || game.ConvertPaks) { + if game.DeployMode == domain.DeployCompile { if i.resolveMergeCompiler == nil { - mcErr = fmt.Errorf("game %q requires DeployCompile to import %q, but this Importer was constructed without service context (via core.NewImporter, not Service.NewImporter) and has no compiler resolver to consult - import via the service-backed importer instead", game.ID, filename) - } else { - mc, mcErr = i.resolveMergeCompiler(game.ID) + return nil, fmt.Errorf("game %q requires DeployCompile to import %q, but this Importer was constructed without service context (via core.NewImporter, not Service.NewImporter) and has no compiler resolver to consult - import via the service-backed importer instead", game.ID, filename) } + mc, mcErr = i.resolveMergeCompiler(game.ID) } + mergeEligible := mcErr == nil && mc != nil && mc.IsNativeMergeSource(filename) convertEligiblePak := mcErr == nil && mc != nil && isConvertEligibleArtifact(game, mc, filename) // Handle based on game's deploy mode @@ -156,9 +159,6 @@ func (i *Importer) Import(ctx context.Context, archivePath string, game *domain. // keyed by the archive's own filename instead - stable across // re-imports of the same name, and the ONLY identity Import ever // has for this content. - if mcErr != nil { - return nil, mcErr - } if err := mc.ValidateSource(archivePath); err != nil { return nil, fmt.Errorf("validating %s: %w", filename, err) } diff --git a/internal/core/merge_format_helpers_test.go b/internal/core/merge_format_helpers_test.go index 7305bed..a0997ab 100644 --- a/internal/core/merge_format_helpers_test.go +++ b/internal/core/merge_format_helpers_test.go @@ -36,6 +36,10 @@ func (fakeMergeFormat) FingerprintBase(basePakPath string) (string, error) { return r.IndexHash(), nil } +func (fakeMergeFormat) IsNativeMergeSource(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".exmodz") +} + func (fakeMergeFormat) IsConvertibleArtifact(fileName string) bool { return strings.HasSuffix(strings.ToLower(fileName), ".pak") } diff --git a/internal/core/merged_pak.go b/internal/core/merged_pak.go index bd32a28..cc3808c 100644 --- a/internal/core/merged_pak.go +++ b/internal/core/merged_pak.go @@ -49,7 +49,7 @@ type MergedFingerprintEntry struct { ModID string Version string Checksum string // MD5 of the retained source bytes (md5File) - Kind string `json:",omitempty"` // source.MergeSourcePak for retained paks; empty/exmodz otherwise (#221) + Kind string `json:",omitempty"` // source-defined convertible kind for retained paks; empty/native otherwise (#221, opaque to core since #256) // Outcome fields (#221): recorded AFTER the merge, ignored by input // equality - a failed conversion retries only when an INPUT changes diff --git a/internal/core/merged_pak_internal_test.go b/internal/core/merged_pak_internal_test.go index 3306c96..82574a3 100644 --- a/internal/core/merged_pak_internal_test.go +++ b/internal/core/merged_pak_internal_test.go @@ -63,6 +63,9 @@ func (formatOnlyCompilerSource) ResolveBaseArtifact(*domain.Game) (string, error return "", os.ErrNotExist } func (formatOnlyCompilerSource) FingerprintBase(string) (string, error) { return "", os.ErrNotExist } +func (formatOnlyCompilerSource) IsNativeMergeSource(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".exmodz") +} func (formatOnlyCompilerSource) IsConvertibleArtifact(fileName string) bool { return strings.HasSuffix(strings.ToLower(fileName), ".pak") } diff --git a/internal/core/service.go b/internal/core/service.go index 8f2f291..b66443d 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -124,19 +124,20 @@ func (s *Service) ValidateInstallFileSelection(sourceID string, files []domain.D if err != nil { return nil } - if _, ok := src.(source.MergeCompiler); !ok { + mc, ok := src.(source.MergeCompiler) + if !ok { return nil } - var exmodz, other bool + var native, other bool for _, f := range files { - if isExmodzFile(f.FileName) { - exmodz = true + if mc.IsNativeMergeSource(f.FileName) { + native = true } else { other = true } } - if exmodz && other { - return fmt.Errorf("pak and exmodz are alternate forms of the same mod - select one") + if native && other { + return fmt.Errorf("raw pak and native merge archive are alternate forms of the same mod - select one") } return nil } @@ -612,7 +613,7 @@ func (s *Service) DownloadModToCache(ctx context.Context, gameCache *cache.Cache // hard-errors when src lacks MergeCompiler (see the !ok check below). mc, isMergeCompiler := src.(source.MergeCompiler) convertEligiblePak := isMergeCompiler && isConvertEligibleArtifact(game, mc, safeFileName) - if game.DeployMode == domain.DeployCompile && (isExmodzFile(safeFileName) || convertEligiblePak) { + if game.DeployMode == domain.DeployCompile && (s.isNativeMergeFile(game, mc, safeFileName) || convertEligiblePak) { if !isMergeCompiler { return nil, fmt.Errorf("source %q: game %q requires DeployCompile but source does not implement MergeCompiler", src.ID(), game.ID) } @@ -1058,21 +1059,34 @@ func commitStagedCache(cachePath, stagePath string) error { return nil } -// isExmodzFile reports whether fileName is a compile-eligible archive -// (case-insensitive ".exmodz" suffix). DeployCompile games can also serve -// plain, already-built ".pak" files (icarus.GetModFiles enumerates "pak" -// before "exmodz") - those must NOT be routed through this function's own -// validate+retain branch as an exmodz, since MergeCompile expects an exmodz -// diff, not a whole pak (#136 review, Task 13 fix round 1). A prebuilt pak -// gets its OWN eligibility check instead - isConvertEligibleArtifact (#221) - -// which the same validate+retain branch also widens for: a convert-eligible -// pak still enters ingest's validate+retain machinery (a different -// isExmodzFile-vs-isConvertEligibleArtifact Kind, not a different branch), -// while a non-eligible pak (ConvertPaks off, or a non-DeployCompile game) -// falls through to the pre-compile extract/copy logic unchanged, exactly as -// if DeployMode were not DeployCompile at all. -func isExmodzFile(fileName string) bool { - return strings.HasSuffix(strings.ToLower(fileName), ".exmodz") +// isNativeMergeFile reports whether fileName is the game's compile source's +// NATIVE merge-source format (#256 - the seam-routed successor to the old +// static isExmodzFile ".exmodz" test). DeployCompile games can also serve +// plain, already-built raw paks (icarus.GetModFiles enumerates "pak" +// before "exmodz") - those must NOT be routed through ingest's +// validate+retain branch as a native archive, since MergeCompile expects a +// native diff, not a whole pak (#136 review, Task 13 fix round 1); a +// prebuilt pak gets its OWN eligibility check instead, +// isConvertEligibleArtifact (#221), a different Kind through the same +// validate+retain branch. +// +// mc is the file's own source's MergeCompiler view (nil when that source +// doesn't implement it). When mc is nil, the GAME's sole compile source is +// consulted instead, so the "native archive served by a non-compile-capable +// source" hard error in DownloadModToCache stays reachable on mixed-source +// games. With no compiler resolvable anywhere, nothing can define "native" +// and this returns false - such files take the legacy extract/copy path, +// where an unextractable native archive still fails loud +// ("unsupported archive format"), just without the compile-specific message. +func (s *Service) isNativeMergeFile(game *domain.Game, mc source.MergeCompiler, fileName string) bool { + if mc == nil { + gmc, err := s.mergeCompilerForGame(game) + if err != nil { + return false + } + mc = gmc + } + return mc.IsNativeMergeSource(fileName) } // isConvertEligibleArtifact reports whether fileName is a prebuilt raw diff --git a/internal/core/service_import_compile_test.go b/internal/core/service_import_compile_test.go index 74f92e4..bc4da3f 100644 --- a/internal/core/service_import_compile_test.go +++ b/internal/core/service_import_compile_test.go @@ -176,8 +176,20 @@ func TestImportMod_DeployCompile_ZipPassthroughUnaffected(t *testing.T) { // TestImportMod_DeployCompile_NoCompilerSourceFailsLoud pins the "never // silently cache an unvalidated .exmodz" requirement (#173/#197): a // DeployCompile game with no MergeCompiler-capable source mapped in its -// SourceIDs must fail loud with an actionable error instead of falling -// through to extract/copy. +// SourceIDs must fail loud, creating no cache entry. +// +// #256 amended WHICH loud failure this is: with the ".exmodz" test moved +// behind the MergeCompiler seam (IsNativeMergeSource), a game whose +// compiler cannot be resolved has nothing left that can define "native", +// so the import falls through to the legacy path - where the +// extension-keyed Extractor rejects the unknown ".exmodz" suffix as +// "unsupported archive format" before anything is staged or cached. The +// pre-#256 compiler-specific message required core itself to know the +// extension, which is the leak #256 closes; the invariant that matters - +// loud failure, nothing cached - is unchanged. (The resolver-nil variant +// below, TestImportMod_DeployCompile_StandaloneImporterFailsLoud, keeps +// its original message: a standalone Importer fails every DeployCompile +// import up front, no format question needed.) func TestImportMod_DeployCompile_NoCompilerSourceFailsLoud(t *testing.T) { installDir := t.TempDir() basePak := filepath.Join(installDir, "Icarus", "Content", "Data", "data.pak") @@ -202,7 +214,7 @@ func TestImportMod_DeployCompile_NoCompilerSourceFailsLoud(t *testing.T) { result, err := importer.Import(context.Background(), archivePath, game, core.ImportOptions{}) require.Error(t, err) require.Nil(t, result) - require.Contains(t, err.Error(), "compiler") + require.Contains(t, err.Error(), "unsupported archive format") _, statErr := os.Stat(filepath.Join(cfg.CacheDir, game.ID)) require.True(t, os.IsNotExist(statErr), "no cache entry should have been created") diff --git a/internal/source/icarus/format.go b/internal/source/icarus/format.go index 7a96bf8..2476522 100644 --- a/internal/source/icarus/format.go +++ b/internal/source/icarus/format.go @@ -74,6 +74,16 @@ func (s *Icarus) FingerprintBase(baseArtifactPath string) (string, error) { return r.IndexHash(), nil } +// IsNativeMergeSource reports whether fileName names this source's NATIVE +// merge-source format - a ".exmodz" archive (case-insensitive), the diff +// format MergeCompile consumes directly without conversion. Pure format +// test, the exact mirror of IsConvertibleArtifact (pre-#256 core's +// isExmodzFile); core owns the DeployCompile policy gates and the +// "native archive from a non-compile-capable source" hard error. +func (s *Icarus) IsNativeMergeSource(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".exmodz") +} + // IsConvertibleArtifact reports whether fileName names a prebuilt .pak this // source can convert into a merge source (#221). Pure format test // (case-insensitive ".pak" suffix, mirroring pre-#256 core's diff --git a/internal/source/icarus/format_test.go b/internal/source/icarus/format_test.go index 7cf2238..d366ce7 100644 --- a/internal/source/icarus/format_test.go +++ b/internal/source/icarus/format_test.go @@ -102,6 +102,25 @@ func TestIsConvertibleArtifact(t *testing.T) { } } +func TestIsNativeMergeSource(t *testing.T) { + tests := map[string]bool{ + "Bear_Mount.exmodz": true, + "Bear_Mount.EXMODZ": true, // case-insensitive + "MyMod.pak": false, + "MyMod.zip": false, + // Bare "exmodz" is a download-path fileID, not a filename - this + // predicate backs ingest routing on FILENAMES (pre-#256 core's + // isExmodzFile), which was suffix-only, and stays that way. + "exmodz": false, + } + s := newFormatTestSource() + for fileName, want := range tests { + if got := s.IsNativeMergeSource(fileName); got != want { + t.Errorf("IsNativeMergeSource(%q) = %v, want %v", fileName, got, want) + } + } +} + func TestClassifyMergeSource(t *testing.T) { tests := map[string]struct { kind string diff --git a/internal/source/icarus/icarus.go b/internal/source/icarus/icarus.go index 9a2b229..7676259 100644 --- a/internal/source/icarus/icarus.go +++ b/internal/source/icarus/icarus.go @@ -260,7 +260,7 @@ func mapDoc(d firestoreDoc) domain.Mod { // fileNameFromURL derives a download's file name from its URL, falling back // to a synthesized "mod." name (never a bare, dot-less // fallbackExt) when the URL yields nothing usable. A dot-less fallback would -// silently defeat isExmodzFile's case-insensitive ".exmodz" suffix check — +// silently defeat IsNativeMergeSource's case-insensitive ".exmodz" suffix check — // a downloaded file named e.g. "exmodz" would never route through the // DeployCompile ingest branch (validate+retain, #197). A parsed basename // that exists but carries no extension of its own gets fallbackExt diff --git a/internal/source/source.go b/internal/source/source.go index 8249b3e..738c1cb 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -190,6 +190,14 @@ type MergeCompiler interface { // game-update-invalidated merge; it never interprets the value. FingerprintBase(baseArtifactPath string) (string, error) + // IsNativeMergeSource reports whether fileName names this source's + // NATIVE merge-source format (Icarus: a ".exmodz" archive) - the diff + // format MergeCompile consumes directly, with no other valid + // interpretation at ingest. Pure format test, the mirror of + // IsConvertibleArtifact - core owns the DeployCompile policy gates and + // routes native files into validate+retain instead of extract/copy. + IsNativeMergeSource(fileName string) bool + // IsConvertibleArtifact reports whether fileName names a raw, prebuilt // game artifact this source can convert into a merge source (#221; // Icarus: a ".pak" file). Pure format test - core owns the diff --git a/internal/tui/merge_format_helpers_test.go b/internal/tui/merge_format_helpers_test.go index 1a87f92..8b31114 100644 --- a/internal/tui/merge_format_helpers_test.go +++ b/internal/tui/merge_format_helpers_test.go @@ -36,6 +36,10 @@ func (fakeMergeFormat) FingerprintBase(basePakPath string) (string, error) { return r.IndexHash(), nil } +func (fakeMergeFormat) IsNativeMergeSource(fileName string) bool { + return strings.HasSuffix(strings.ToLower(fileName), ".exmodz") +} + func (fakeMergeFormat) IsConvertibleArtifact(fileName string) bool { return strings.HasSuffix(strings.ToLower(fileName), ".pak") } From 9b17f6aa4f0d66e61ba626428e5a89a489b06e6a Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 16:07:54 -0400 Subject: [PATCH 06/11] docs+ux(core): Copilot round-2 triage - complete the seam doc, neutralize the last format word (#256) - The MergeCompiler preamble now enumerates IsNativeMergeSource with the rest of the format vocabulary (its omission made the contract description incomplete for implementers). - The mixed-selection rejection drops 'pak': 'raw artifact and native merge archive are alternate forms of the same mod - select one' - the message was this PR's one remaining core string a second DeployCompile game would have had to edit. Declined (with reasoning on the PR): isNativeMergeFile deliberately treats an unresolvable compile source as 'nothing defines native' rather than propagating - an ambiguous-compilers config cannot stay hidden, as every merge-execution path (sync, import, staleness) still fails loud on the same resolution. Co-Authored-By: Claude Fable 5 --- internal/core/flows_variant_exclusivity_test.go | 6 +++--- internal/core/service.go | 2 +- internal/source/source.go | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/core/flows_variant_exclusivity_test.go b/internal/core/flows_variant_exclusivity_test.go index e9f7224..ccc6c45 100644 --- a/internal/core/flows_variant_exclusivity_test.go +++ b/internal/core/flows_variant_exclusivity_test.go @@ -142,7 +142,7 @@ func TestApplyInstall_StrictPath_TargetFileIDs_MixedVariants_Rejected(t *testing opts := core.InstallOptions{TargetFileIDs: []string{"pak", "exmodz"}} _, err = svc.ApplyInstall(context.Background(), game, plan, opts, nil) - require.ErrorContains(t, err, "raw pak and native merge archive are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "raw artifact and native merge archive are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) @@ -167,7 +167,7 @@ func TestApplyInstall_StrictPath_CallerSuppliedMixedVariantFiles_Rejected(t *tes plan.Files = mc.files["mod1"] _, err = svc.ApplyInstall(context.Background(), game, plan, core.InstallOptions{}, nil) - require.ErrorContains(t, err, "raw pak and native merge archive are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "raw artifact and native merge archive are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) @@ -204,7 +204,7 @@ func TestApplyInstall_BatchPath_TargetFileIDs_MixedVariants_Rejected(t *testing. opts := core.InstallOptions{TargetFileIDs: []string{"pak", "exmodz"}} _, err = svc.ApplyInstall(context.Background(), game, plan, opts, nil) - require.ErrorContains(t, err, "raw pak and native merge archive are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "raw artifact and native merge archive are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) diff --git a/internal/core/service.go b/internal/core/service.go index b66443d..2bc63ab 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -137,7 +137,7 @@ func (s *Service) ValidateInstallFileSelection(sourceID string, files []domain.D } } if native && other { - return fmt.Errorf("raw pak and native merge archive are alternate forms of the same mod - select one") + return fmt.Errorf("raw artifact and native merge archive are alternate forms of the same mod - select one") } return nil } diff --git a/internal/source/source.go b/internal/source/source.go index 738c1cb..2c89912 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -156,7 +156,8 @@ type DownloadHeaderProvider interface { // plus the format vocabulary core needs to orchestrate them without knowing // the game's artifact format itself - where the base artifact lives // (ResolveBaseArtifact), how to fingerprint it (FingerprintBase), which -// files are convertible raw artifacts (IsConvertibleArtifact, +// files are the source's native merge format (IsNativeMergeSource), which +// are convertible raw artifacts (IsConvertibleArtifact, // ClassifyMergeSource), and what the merged output is called // (MergedArtifactName, MergedArtifactLabel). A second compile-mode game is // a new source package implementing these methods plus one registration From 48012c21e0d776713c66d85aedc5c245553d4e8e Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 16:19:39 -0400 Subject: [PATCH 07/11] fix(core): fail loud on unresolvable compiler for DeployCompile imports (#256) Copilot round-3 catch, confirmed real: the legacy extract path content-sniffs zip magic (detectFormatFromPath's header fallback - NOT extension-only, as the round-2 design assumed), so letting a DeployCompile import fall through when the game's MergeCompiler cannot be resolved would silently extract and cache a real, zip-backed native archive without ValidateSource. Import path: an unresolvable compiler is now a hard error for EVERY import into a DeployCompile game - with no compiler, core cannot tell a native archive from anything else, and the resolver's own message names the fix. This restores TestImportMod_DeployCompile_NoCompilerSourceFailsLoud to its ORIGINAL assertions (the round-2 edit is reverted), and supersedes the import-side half of #221 I1: the pak fall-through test becomes TestImportMod_DeployCompile_PakNoCompilerSourceFailsLoud, doc'd with the history. The download-side I1 fall-through (per-source pin) is unchanged and its test passes untouched. Download path: unchanged - the non-compiler-source fall-through is a protected #221 contract. The residual (native archive from a non-compiler source on a game with NO resolvable compiler anywhere gets sniff-ingested) requires a hand-broken SourceIDs map plus a foreign source serving native archives; documented on isNativeMergeFile. Co-Authored-By: Claude Fable 5 --- internal/core/importer.go | 48 ++++++++------ internal/core/service_import_compile_test.go | 66 ++++++++------------ 2 files changed, 54 insertions(+), 60 deletions(-) diff --git a/internal/core/importer.go b/internal/core/importer.go index c262b98..5a354c4 100644 --- a/internal/core/importer.go +++ b/internal/core/importer.go @@ -123,33 +123,41 @@ func (i *Importer) Import(ctx context.Context, archivePath string, game *domain. // (mc.IsNativeMergeSource - the seam-routed successor to core's static // ".exmodz" test) or a convertible artifact (mc.IsConvertibleArtifact) // is the compile source's call, so the compiler is resolved up front - // for every DeployCompile import. Resolution is a pure registry - // lookup; when it fails, nothing can define either format, so BOTH - // predicates are false and the import falls through to the legacy - // extract/copy path: a raw pak or plain zip behaves exactly as before - // (#221 I1 fix - "unsupported archive format" for the pak, a normal - // extract for the zip), and a native archive - unextractable, since - // the Extractor is extension-keyed - still fails loud with - // "unsupported archive format" rather than the old compiler-specific - // message, and still caches nothing. + // for EVERY DeployCompile import, and a resolution failure is a hard + // error for every one of them - not just the native case the old + // static test could single out. Falling through instead would be + // unsafe: with no compiler, core cannot tell a native merge archive + // from anything else, and the legacy extract path CONTENT-SNIFFS zip + // magic (detectFormatFromPath), so a real, zip-backed native archive + // would be silently extracted and cached without ValidateSource - + // exactly the "never silently cache an unvalidated native archive" + // invariant the resolver error protects. This supersedes #221 I1's + // import-side pak fall-through: that was safe only while core itself + // knew which files were native. (The DOWNLOAD path's I1 fall-through + // stands - it pins eligibility to the file's own source, a per-archive + // signal Import does not have.) The resolver error names the fix + // ("map a source implementing source.MergeCompiler"), which is + // accurate for any import into a compile game whose compiler is + // missing or ambiguous. // - // A NIL RESOLVER is different: core.NewImporter (no Service context) - // cannot answer the format question for ANY file, and unlike a - // misconfigured game there is a correct importer to use instead - so a - // DeployCompile import through a standalone Importer fails loud - // unconditionally. Production only ever constructs the service-backed - // importer (Service.NewImporter); this path is reachable only by - // direct core.NewImporter use. + // A NIL RESOLVER fails the same way for the same reason, with its own + // message: core.NewImporter (no Service context) cannot answer the + // format question for ANY file, and there is a correct importer to use + // instead. Production only ever constructs the service-backed importer + // (Service.NewImporter); this path is reachable only by direct + // core.NewImporter use. var mc source.MergeCompiler - var mcErr error if game.DeployMode == domain.DeployCompile { if i.resolveMergeCompiler == nil { return nil, fmt.Errorf("game %q requires DeployCompile to import %q, but this Importer was constructed without service context (via core.NewImporter, not Service.NewImporter) and has no compiler resolver to consult - import via the service-backed importer instead", game.ID, filename) } - mc, mcErr = i.resolveMergeCompiler(game.ID) + var mcErr error + if mc, mcErr = i.resolveMergeCompiler(game.ID); mcErr != nil { + return nil, mcErr + } } - mergeEligible := mcErr == nil && mc != nil && mc.IsNativeMergeSource(filename) - convertEligiblePak := mcErr == nil && mc != nil && isConvertEligibleArtifact(game, mc, filename) + mergeEligible := mc != nil && mc.IsNativeMergeSource(filename) + convertEligiblePak := mc != nil && isConvertEligibleArtifact(game, mc, filename) // Handle based on game's deploy mode if game.DeployMode == domain.DeployCompile && (mergeEligible || convertEligiblePak) { diff --git a/internal/core/service_import_compile_test.go b/internal/core/service_import_compile_test.go index bc4da3f..b9ab2dd 100644 --- a/internal/core/service_import_compile_test.go +++ b/internal/core/service_import_compile_test.go @@ -176,20 +176,11 @@ func TestImportMod_DeployCompile_ZipPassthroughUnaffected(t *testing.T) { // TestImportMod_DeployCompile_NoCompilerSourceFailsLoud pins the "never // silently cache an unvalidated .exmodz" requirement (#173/#197): a // DeployCompile game with no MergeCompiler-capable source mapped in its -// SourceIDs must fail loud, creating no cache entry. -// -// #256 amended WHICH loud failure this is: with the ".exmodz" test moved -// behind the MergeCompiler seam (IsNativeMergeSource), a game whose -// compiler cannot be resolved has nothing left that can define "native", -// so the import falls through to the legacy path - where the -// extension-keyed Extractor rejects the unknown ".exmodz" suffix as -// "unsupported archive format" before anything is staged or cached. The -// pre-#256 compiler-specific message required core itself to know the -// extension, which is the leak #256 closes; the invariant that matters - -// loud failure, nothing cached - is unchanged. (The resolver-nil variant -// below, TestImportMod_DeployCompile_StandaloneImporterFailsLoud, keeps -// its original message: a standalone Importer fails every DeployCompile -// import up front, no format question needed.) +// SourceIDs must fail loud with an actionable error instead of falling +// through to extract/copy. (#256: the failure now happens up front - the +// compiler is resolved for every DeployCompile import, since only it can +// say which files are native - but the message still names the compiler +// gap and nothing is ever cached, same as always.) func TestImportMod_DeployCompile_NoCompilerSourceFailsLoud(t *testing.T) { installDir := t.TempDir() basePak := filepath.Join(installDir, "Icarus", "Content", "Data", "data.pak") @@ -214,37 +205,33 @@ func TestImportMod_DeployCompile_NoCompilerSourceFailsLoud(t *testing.T) { result, err := importer.Import(context.Background(), archivePath, game, core.ImportOptions{}) require.Error(t, err) require.Nil(t, result) - require.Contains(t, err.Error(), "unsupported archive format") + require.Contains(t, err.Error(), "compiler") _, statErr := os.Stat(filepath.Join(cfg.CacheDir, game.ID)) require.True(t, os.IsNotExist(statErr), "no cache entry should have been created") } -// TestImportMod_DeployCompile_PakNoCompilerSourceFallsThrough is the I1 fix -// (final whole-branch review of #221) for the import path, mirroring +// TestImportMod_DeployCompile_PakNoCompilerSourceFailsLoud mirrors // TestImportMod_DeployCompile_NoCompilerSourceFailsLoud's setup (a -// DeployCompile game with NO MergeCompiler-capable source mapped) but for a -// .pak filename instead of .exmodz: isConvertEligiblePakFile only checked -// game flags (DeployMode + ConvertPaks), so this scenario used to enter the -// validate+retain branch and hard-error on resolveMergeCompiler's failure - -// exactly like the exmodz case above, even though a .pak (unlike an -// .exmodz) has another valid interpretation once eligibility is properly -// gated on actual resolver success. +// DeployCompile game with NO MergeCompiler-capable source mapped) for a +// .pak filename: like every other import into such a game, it must fail +// loud on the compiler-resolution error, caching nothing. // -// Unlike the download path (DownloadModToCache's copy fallback triggers -// unconditionally whenever the file isn't a recognized archive, regardless -// of deploy mode), Import has no such unconditional fallback outside -// DeployCopy - a DeployCompile game importing any unrecognized, non-archive -// format (a raw .pak included) has always ended in "unsupported archive -// format" once it misses the compile branch, pre-#221 included (a .pak -// import for Icarus simply wasn't a supported scenario before #221 existed -// at all). So the observable fix here is not "the import now succeeds" (it -// can't - Import genuinely has nothing else to do with a raw .pak) but "the -// import fails with the SAME, accurate, pre-#221-consistent 'unsupported -// archive format' error instead of a MISLEADING 'no merge-compiler-capable -// source configured' one" - and, just as importantly, no cache entry or -// retained source is created for a mod that was never actually ingested. -func TestImportMod_DeployCompile_PakNoCompilerSourceFallsThrough(t *testing.T) { +// History: #221 I1 made this exact case fall through to the legacy path +// (whose "unsupported archive format" error it then reported), because the +// resolver message was misleading while core could statically single out +// .exmodz as the only file kind that truly REQUIRED the compiler. #256 +// moved that knowledge behind the seam (IsNativeMergeSource), which +// removes the basis for the carve-out: with no resolvable compiler, core +// cannot tell a raw pak from a native merge archive, and falling through +// would let the legacy path's zip-content-sniffing extractor silently +// ingest a real native archive unvalidated. Failing every import of a +// compiler-less compile game with the actionable resolver message ("map a +// source implementing source.MergeCompiler") is now both the safe and the +// accurate behavior. The download path's I1 fall-through is unchanged - +// it keys on the file's own source, a per-archive signal Import lacks +// (TestDownloadPak_NonMergeCompilerSource_FallsThroughToLegacyPath). +func TestImportMod_DeployCompile_PakNoCompilerSourceFailsLoud(t *testing.T) { installDir := t.TempDir() basePak := filepath.Join(installDir, "Icarus", "Content", "Data", "data.pak") require.NoError(t, os.MkdirAll(filepath.Dir(basePak), 0o755)) @@ -268,8 +255,7 @@ func TestImportMod_DeployCompile_PakNoCompilerSourceFallsThrough(t *testing.T) { result, err := importer.Import(context.Background(), archivePath, game, core.ImportOptions{}) require.Error(t, err) require.Nil(t, result) - require.Contains(t, err.Error(), "unsupported archive format", "must fall through to the legacy path's own error, not the misleading MergeCompiler-resolution one") - require.NotContains(t, err.Error(), "compiler", "must NOT report a MergeCompiler-resolution failure once eligibility correctly fell through") + require.Contains(t, err.Error(), "merge-compiler-capable source", "must fail loud on the actionable compiler-resolution error, never reach the sniffing legacy path") _, statErr := os.Stat(filepath.Join(cfg.CacheDir, game.ID)) require.True(t, os.IsNotExist(statErr), "no cache entry (and no retained source) should have been created") From 008825310cea77a1d7b63c4821d1a5e0f0b4b509 Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 16:20:01 -0400 Subject: [PATCH 08/11] docs(core): document the accepted download-path sniffing residual on isNativeMergeFile (#256) Co-Authored-By: Claude Fable 5 --- internal/core/service.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/core/service.go b/internal/core/service.go index 2bc63ab..7b3315f 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -1076,8 +1076,15 @@ func commitStagedCache(cachePath, stagePath string) error { // source" hard error in DownloadModToCache stays reachable on mixed-source // games. With no compiler resolvable anywhere, nothing can define "native" // and this returns false - such files take the legacy extract/copy path, -// where an unextractable native archive still fails loud -// ("unsupported archive format"), just without the compile-specific message. +// preserving #221 I1's protected download fall-through for the non-compile +// source's paks and zips. Known residual, accepted deliberately: because +// the legacy Extractor content-sniffs zip magic, a REAL native archive +// downloaded in that doubly-broken state (a game whose SourceIDs map no +// compiler at all - icarus is always registered, so only a hand-edited map +// gets here - AND a foreign source serving native archives) is ingested as +// a plain archive without ValidateSource. The import path has no such +// residual: Importer.Import hard-errors on an unresolvable compiler, since +// it has no per-archive source contract forcing a fall-through. func (s *Service) isNativeMergeFile(game *domain.Game, mc source.MergeCompiler, fileName string) bool { if mc == nil { gmc, err := s.mergeCompilerForGame(game) From c1e839608dcbf6e437d7649a6ecf32d497a27017 Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 16:47:52 -0400 Subject: [PATCH 09/11] refactor(core): finish the seam - restore-name behind MergeCompiler, colliding files named in the rejection (#256) Item 1: #261's rawPakRestoreName held the last live format knowledge in core - a '.pak' extension test and the synthesized '_P.pak' Icarus override name. The extension test was already expressed by the seam (IsConvertibleArtifact: EqualFold-on-Ext and suffix-on-lowered are equivalent for every filename), so it reuses that; the synthesized name is genuinely new vocabulary and becomes the tenth method, RestoredArtifactName(modID) - the per-mod analogue of MergedArtifactName, byte-identical output ('_P.pak', core Base's the input) so healed installs keep their on-disk names. The import-vs-download provenance split STAYS in core: it follows from how ingest keys fileIDs, which is uniform across games - only the two format questions inside it moved. #261's heal/prune tests pass unmodified. Item 2: the mixed-selection rejection now names the actual colliding files ('Mod_P.pak and Mod.exmodz are alternate forms of the same mod - select one') - format-agnostic because the vocabulary comes from the selection itself, and strictly more useful than both prior wordings. The trigger is unchanged (booleans, not name-emptiness, so a file with an empty FileName still trips it). Variant-exclusivity expectations updated deliberately; cmd/lmm/install_test.go's local fake updated to mirror the production message shape it stands in for. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 11 ++++--- cmd/lmm/install_test.go | 14 ++++---- cmd/lmm/merge_format_helpers_test.go | 2 ++ .../core/flows_variant_exclusivity_test.go | 6 ++-- internal/core/merge_format_helpers_test.go | 2 ++ internal/core/merged_pak.go | 33 +++++++++++-------- internal/core/merged_pak_internal_test.go | 3 ++ internal/core/service.go | 11 +++++-- internal/source/icarus/format.go | 10 ++++++ internal/source/icarus/format_test.go | 10 ++++++ internal/source/source.go | 9 +++++ internal/tui/merge_format_helpers_test.go | 2 ++ 12 files changed, 83 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a17c6c5..be5099b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,10 +11,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Internal: all Unreal/Icarus format knowledge (base-pak location, pak fingerprinting, the `.pak` convert test, the native `.exmodz` test, - merge-source kinds, and the merged artifact's filename) moved out of - `internal/core` and behind the `source.MergeCompiler` interface, which - now states the complete contract a second compile-mode game would - implement (#256). No user-visible change. + merge-source kinds, and the merged/restored artifact filenames) moved + out of `internal/core` and behind the `source.MergeCompiler` interface, + which now states the complete contract a second compile-mode game would + implement (#256). The one user-visible change: rejecting a mixed + pak+exmodz install selection now names the two colliding files + (`Mod_P.pak and Mod.exmodz are alternate forms of the same mod - select +one`) instead of the generic wording. ### Fixed diff --git a/cmd/lmm/install_test.go b/cmd/lmm/install_test.go index eb2c433..392fcc1 100644 --- a/cmd/lmm/install_test.go +++ b/cmd/lmm/install_test.go @@ -381,19 +381,21 @@ func TestPromptMultiSelection_Range(t *testing.T) { } // mixedPakExmodzValidate mirrors the shape of the real -// Service.ValidateInstallFileSelection closure (#211): reject a selection -// that mixes an exmodz file with any other file. +// Service.ValidateInstallFileSelection closure (#211, message shape #256): +// reject a selection that mixes an exmodz file with any other file, naming +// the two colliding files the way production does. func mixedPakExmodzValidate(sel []domain.DownloadableFile) error { var ex, other bool + var exName, otherName string for _, f := range sel { if strings.HasSuffix(strings.ToLower(f.FileName), ".exmodz") { - ex = true + ex, exName = true, f.FileName } else { - other = true + other, otherName = true, f.FileName } } if ex && other { - return fmt.Errorf("pak and exmodz are alternate forms of the same mod - select one") + return fmt.Errorf("%s and %s are alternate forms of the same mod - select one", otherName, exName) } return nil } @@ -430,7 +432,7 @@ func TestSelectInstallFiles_FileFlagMixedRejected(t *testing.T) { selected, err := selectInstallFilesFrom(strings.NewReader(""), files, mixedPakExmodzValidate) require.Error(t, err) assert.Nil(t, selected) - assert.Contains(t, err.Error(), "pak and exmodz are alternate forms of the same mod - select one") + assert.Contains(t, err.Error(), "Mod_P.pak and Mod.exmodz are alternate forms of the same mod - select one") } // TestSelectInstallFiles_EOFAfterRejectedSelection guards against a hang: diff --git a/cmd/lmm/merge_format_helpers_test.go b/cmd/lmm/merge_format_helpers_test.go index 56bff99..f683ed7 100644 --- a/cmd/lmm/merge_format_helpers_test.go +++ b/cmd/lmm/merge_format_helpers_test.go @@ -54,3 +54,5 @@ func (fakeMergeFormat) ClassifyMergeSource(id string) (string, bool) { func (fakeMergeFormat) MergedArtifactName() string { return "zzz_LMM_Merged_P.pak" } func (fakeMergeFormat) MergedArtifactLabel() string { return "Icarus Merged Pak" } + +func (fakeMergeFormat) RestoredArtifactName(modID string) string { return modID + "_P.pak" } diff --git a/internal/core/flows_variant_exclusivity_test.go b/internal/core/flows_variant_exclusivity_test.go index ccc6c45..45ff950 100644 --- a/internal/core/flows_variant_exclusivity_test.go +++ b/internal/core/flows_variant_exclusivity_test.go @@ -142,7 +142,7 @@ func TestApplyInstall_StrictPath_TargetFileIDs_MixedVariants_Rejected(t *testing opts := core.InstallOptions{TargetFileIDs: []string{"pak", "exmodz"}} _, err = svc.ApplyInstall(context.Background(), game, plan, opts, nil) - require.ErrorContains(t, err, "raw artifact and native merge archive are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "Mod_P.pak and Mod.exmodz are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) @@ -167,7 +167,7 @@ func TestApplyInstall_StrictPath_CallerSuppliedMixedVariantFiles_Rejected(t *tes plan.Files = mc.files["mod1"] _, err = svc.ApplyInstall(context.Background(), game, plan, core.InstallOptions{}, nil) - require.ErrorContains(t, err, "raw artifact and native merge archive are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "Mod_P.pak and Mod.exmodz are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) @@ -204,7 +204,7 @@ func TestApplyInstall_BatchPath_TargetFileIDs_MixedVariants_Rejected(t *testing. opts := core.InstallOptions{TargetFileIDs: []string{"pak", "exmodz"}} _, err = svc.ApplyInstall(context.Background(), game, plan, opts, nil) - require.ErrorContains(t, err, "raw artifact and native merge archive are alternate forms of the same mod - select one") + require.ErrorContains(t, err, "Mod_P.pak and Mod.exmodz are alternate forms of the same mod - select one") require.Equal(t, 0, mc.DownloadCount(), "no download may happen for a rejected mixed selection") gameCache := svc.GetGameCache(game) diff --git a/internal/core/merge_format_helpers_test.go b/internal/core/merge_format_helpers_test.go index a0997ab..d1f94fd 100644 --- a/internal/core/merge_format_helpers_test.go +++ b/internal/core/merge_format_helpers_test.go @@ -54,3 +54,5 @@ func (fakeMergeFormat) ClassifyMergeSource(id string) (string, bool) { func (fakeMergeFormat) MergedArtifactName() string { return "zzz_LMM_Merged_P.pak" } func (fakeMergeFormat) MergedArtifactLabel() string { return "Icarus Merged Pak" } + +func (fakeMergeFormat) RestoredArtifactName(modID string) string { return modID + "_P.pak" } diff --git a/internal/core/merged_pak.go b/internal/core/merged_pak.go index cc3808c..cbb117c 100644 --- a/internal/core/merged_pak.go +++ b/internal/core/merged_pak.go @@ -518,7 +518,7 @@ func (s *Service) reconcilePakManifests(ctx context.Context, game *domain.Game, // it. Without this, the mark below would record an EMPTY // member set and Install would deploy nothing, silently // - the raw fallback's whole purpose defeated. - restoredName, herr := restoreRawPakCopy(versionDir, retained, fileID, mod.ID) + restoredName, herr := restoreRawPakCopy(mc, versionDir, retained, fileID, mod.ID) if herr != nil { return warnings, fmt.Errorf("restoring pruned raw pak for %s: %w", ref, herr) } @@ -624,20 +624,25 @@ func rawPakMembers(versionDir, retainedPath string, candidates []string) ([]stri // rawPakRestoreName picks the on-disk name restoreRawPakCopy publishes a // healed deployable pak copy under (#250). An import-path fileID IS the // original archive filename (Importer.Import stages the deployable copy -// under exactly that name), so the restore is name-exact. A download-path -// fileID (the literal icarus "pak") never carried the deployable name: it -// came from the download URL's basename at ingest time, the convert flip -// erased the manifest that recorded it, and nothing else durably stores it -// - so a deterministic mod-scoped name in Icarus's "_P.pak" override -// convention (the old compiledFileName convention) is synthesized instead. -// Both inputs are source-controlled, so both are Base'd before use as a -// path component. -func rawPakRestoreName(fileID, modID string) string { +// under exactly that name), so the restore is name-exact - detected by +// asking the compile source whether the fileID names one of its +// convertible artifacts (#256: the format test lives behind the seam). A +// download-path fileID (the literal icarus "pak") never carried the +// deployable name: it came from the download URL's basename at ingest +// time, the convert flip erased the manifest that recorded it, and +// nothing else durably stores it - so the source synthesizes its +// deterministic mod-scoped fallback name instead +// (mc.RestoredArtifactName; Icarus's "_P.pak" override convention). The +// import-vs-download provenance SPLIT stays in core - it follows from how +// ingest keys fileIDs, which is uniform across games - while both format +// questions inside it are the source's. Both inputs are +// source-controlled, so both are Base'd before use as a path component. +func rawPakRestoreName(mc source.MergeCompiler, fileID, modID string) string { base := filepath.Base(fileID) - if strings.EqualFold(filepath.Ext(base), ".pak") { + if mc.IsConvertibleArtifact(base) { return base } - return filepath.Base(modID) + "_P.pak" + return mc.RestoredArtifactName(filepath.Base(modID)) } // restoreRawPakCopy re-creates fileID's deployable pak copy in versionDir @@ -649,8 +654,8 @@ func rawPakRestoreName(fileID, modID string) string { // (rawPakMembers just matched nothing), and it could be a sibling fileID's // claimed member - failing loudly beats corrupting it, and the next // reconcile pass retries. -func restoreRawPakCopy(versionDir, retainedPath, fileID, modID string) (string, error) { - name := rawPakRestoreName(fileID, modID) +func restoreRawPakCopy(mc source.MergeCompiler, versionDir, retainedPath, fileID, modID string) (string, error) { + name := rawPakRestoreName(mc, fileID, modID) target := filepath.Join(versionDir, name) if _, err := os.Stat(target); err == nil { return "", fmt.Errorf("restore target %s already exists with content not matching the retained source; refusing to overwrite", name) diff --git a/internal/core/merged_pak_internal_test.go b/internal/core/merged_pak_internal_test.go index 82574a3..bd16070 100644 --- a/internal/core/merged_pak_internal_test.go +++ b/internal/core/merged_pak_internal_test.go @@ -74,6 +74,9 @@ func (formatOnlyCompilerSource) ClassifyMergeSource(id string) (string, bool) { } func (formatOnlyCompilerSource) MergedArtifactName() string { return "zzz_LMM_Merged_P.pak" } func (formatOnlyCompilerSource) MergedArtifactLabel() string { return "Icarus Merged Pak" } +func (formatOnlyCompilerSource) RestoredArtifactName(modID string) string { + return modID + "_P.pak" +} func TestMergedFingerprint_Deterministic(t *testing.T) { f := MergedFingerprint{ diff --git a/internal/core/service.go b/internal/core/service.go index 7b3315f..e3482f1 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -128,16 +128,21 @@ func (s *Service) ValidateInstallFileSelection(sourceID string, files []domain.D if !ok { return nil } + // The colliding filenames are captured for the error: naming the actual + // files keeps the message format-agnostic (the vocabulary comes from + // the selection itself, not a hardcoded format list) and tells the user + // WHICH two files collided, not just that a collision happened (#256). var native, other bool + var nativeName, otherName string for _, f := range files { if mc.IsNativeMergeSource(f.FileName) { - native = true + native, nativeName = true, f.FileName } else { - other = true + other, otherName = true, f.FileName } } if native && other { - return fmt.Errorf("raw artifact and native merge archive are alternate forms of the same mod - select one") + return fmt.Errorf("%s and %s are alternate forms of the same mod - select one", otherName, nativeName) } return nil } diff --git a/internal/source/icarus/format.go b/internal/source/icarus/format.go index 2476522..96b4b89 100644 --- a/internal/source/icarus/format.go +++ b/internal/source/icarus/format.go @@ -118,3 +118,13 @@ func (s *Icarus) MergedArtifactName() string { return mergedPakFileName } // MergedArtifactLabel is the user-facing display name for the merged // artifact's synthetic mod row (verify/update output). func (s *Icarus) MergedArtifactLabel() string { return "Icarus Merged Pak" } + +// RestoredArtifactName names the deployable raw-fallback copy synthesized +// when core heals a prune-damaged cache entry whose original artifact name +// is unrecoverable (#250: download-path fileIDs never carried it - the +// name came from the download URL's basename at ingest, and the convert +// flip erased the manifest that recorded it). A deterministic mod-scoped +// name in the "_P.pak" override convention (see mergedPakFileName's doc) +// keeps healed installs stable: the same mod always restores to the same +// name, which is on-disk state existing installs already depend on. +func (s *Icarus) RestoredArtifactName(modID string) string { return modID + "_P.pak" } diff --git a/internal/source/icarus/format_test.go b/internal/source/icarus/format_test.go index d366ce7..164a51c 100644 --- a/internal/source/icarus/format_test.go +++ b/internal/source/icarus/format_test.go @@ -143,6 +143,16 @@ func TestClassifyMergeSource(t *testing.T) { } } +func TestRestoredArtifactName(t *testing.T) { + // The exact shape is on-disk state (#250): a heal-restored raw-fallback + // copy for existing installs is published under _P.pak, "_P" + // being UE's override-pak suffix convention - byte-identical names must + // come out of the seam or already-healed caches would orphan. + if got := newFormatTestSource().RestoredArtifactName("cool-mod"); got != "cool-mod_P.pak" { + t.Errorf("RestoredArtifactName = %q, want %q", got, "cool-mod_P.pak") + } +} + func TestMergedArtifactName(t *testing.T) { // The exact name is a deploy contract (#197): it must sort last among // mounted paks ("zzz"), be greppable as lmm-owned, and keep the "_P" diff --git a/internal/source/source.go b/internal/source/source.go index 2c89912..f065126 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -226,6 +226,15 @@ type MergeCompiler interface { // MergedArtifactLabel is the user-facing display name for the merged // artifact's synthetic mod row (verify/update output). MergedArtifactLabel() string + + // RestoredArtifactName names the deployable raw-fallback copy core + // synthesizes when healing a prune-damaged cache entry whose original + // artifact name is unrecoverable (#250; Icarus: "_P.pak"). + // Deterministic per mod - the same mod must always restore to the same + // name, since the name is on-disk state existing installs depend on. + // Core passes a path-safe (Base'd) modID and uses the result as a + // filename within the mod's own cache entry. + RestoredArtifactName(modID string) string } // MergeSource identifies one mod's contribution to a merge, in the order it diff --git a/internal/tui/merge_format_helpers_test.go b/internal/tui/merge_format_helpers_test.go index 8b31114..60a60e2 100644 --- a/internal/tui/merge_format_helpers_test.go +++ b/internal/tui/merge_format_helpers_test.go @@ -54,3 +54,5 @@ func (fakeMergeFormat) ClassifyMergeSource(id string) (string, bool) { func (fakeMergeFormat) MergedArtifactName() string { return "zzz_LMM_Merged_P.pak" } func (fakeMergeFormat) MergedArtifactLabel() string { return "Icarus Merged Pak" } + +func (fakeMergeFormat) RestoredArtifactName(modID string) string { return modID + "_P.pak" } From beffdaecf6d1619ac5b183cb0ac59defeb46338c Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 16:52:11 -0400 Subject: [PATCH 10/11] docs: Copilot round-5 triage - CHANGELOG wrap, complete-contract enumeration, stale doc name (#256) Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 3 +-- internal/core/service.go | 2 +- internal/source/source.go | 5 +++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be5099b..085b94f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,8 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 which now states the complete contract a second compile-mode game would implement (#256). The one user-visible change: rejecting a mixed pak+exmodz install selection now names the two colliding files - (`Mod_P.pak and Mod.exmodz are alternate forms of the same mod - select -one`) instead of the generic wording. + (e.g. `Mod_P.pak and Mod.exmodz`) instead of the generic wording. ### Fixed diff --git a/internal/core/service.go b/internal/core/service.go index e3482f1..0caa518 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -204,7 +204,7 @@ func (s *Service) SourcesForGame(gameID string) ([]source.ModSource, error) { return srcs, nil } -// compilerSourceForGame resolves the sole Compiler-capable source +// mergeCompilerSourceForGame resolves the sole MergeCompiler-capable source // registered for gameID (#173). The download path pins its MergeCompiler // check to the specific source a file was downloaded from // (DownloadModToCache's src.(source.MergeCompiler) check); Importer.Import diff --git a/internal/source/source.go b/internal/source/source.go index f065126..34d6e32 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -158,8 +158,9 @@ type DownloadHeaderProvider interface { // (ResolveBaseArtifact), how to fingerprint it (FingerprintBase), which // files are the source's native merge format (IsNativeMergeSource), which // are convertible raw artifacts (IsConvertibleArtifact, -// ClassifyMergeSource), and what the merged output is called -// (MergedArtifactName, MergedArtifactLabel). A second compile-mode game is +// ClassifyMergeSource), what the merged output is called +// (MergedArtifactName, MergedArtifactLabel), and what a healed raw-fallback +// copy is called (RestoredArtifactName). A second compile-mode game is // a new source package implementing these methods plus one registration // line; internal/core never changes. type MergeCompiler interface { From a05274a962d73690b53783455be2cef3b9677e4b Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 16:58:33 -0400 Subject: [PATCH 11/11] fix(core): nil-game guard on ModHasPakMergeSource; keep it deploy-mode-agnostic (#256) Copilot round-6 triage: the nil guard is taken; the suggested DeployCompile short-circuit is NOT - it breaks a pinned CLI behavior: 'lmm mod convert' persists the per-mod flag on non-compile games with an advisory note (TestModConvertCommand_NonCompileGame), which requires classification to answer whenever the game's compile source resolves, exactly as the pre-#256 static test did. Documented on the method; the internal test now covers the non-compile-with-compiler true case and the nil-game case. Co-Authored-By: Claude Fable 5 --- internal/core/merged_pak.go | 10 ++++++++-- internal/core/merged_pak_internal_test.go | 21 ++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/internal/core/merged_pak.go b/internal/core/merged_pak.go index cbb117c..772b688 100644 --- a/internal/core/merged_pak.go +++ b/internal/core/merged_pak.go @@ -76,9 +76,15 @@ type mergeSourceClassifier func(id string) (kind string, convertible bool) // convert-toggle key - want this cheaper check, not enabledMergeSources' // full retained-file resolution. A game with no (or an ambiguous) // merge-compiler source has no merge sources of any kind, so resolution -// failure is simply false, not an error. +// failure is simply false, not an error. Deliberately NOT gated on +// game.DeployMode: `lmm mod convert` persists the per-mod flag on +// non-compile games too (with an advisory that it has no effect there, +// TestModConvertCommand_NonCompileGame), so classification must answer +// for any game whose compile source resolves - matching the pre-#256 +// static behavior. Callers that only care about compile games gate on +// DeployMode themselves. func (s *Service) ModHasPakMergeSource(game *domain.Game, mod *domain.InstalledMod) bool { - if mod == nil { + if game == nil || mod == nil { return false } mc, err := s.mergeCompilerForGame(game) diff --git a/internal/core/merged_pak_internal_test.go b/internal/core/merged_pak_internal_test.go index bd16070..a30dce3 100644 --- a/internal/core/merged_pak_internal_test.go +++ b/internal/core/merged_pak_internal_test.go @@ -191,7 +191,7 @@ func TestModHasPakMergeSource(t *testing.T) { reg := source.NewRegistry() reg.Register(formatOnlyCompilerSource{}) svc := &Service{registry: reg} - game := &domain.Game{ID: "g", SourceIDs: map[string]string{"fake-compiler": "g"}} + game := &domain.Game{ID: "g", DeployMode: domain.DeployCompile, SourceIDs: map[string]string{"fake-compiler": "g"}} tests := []struct { name string @@ -214,11 +214,26 @@ func TestModHasPakMergeSource(t *testing.T) { } // #256: with no merge-compiler-capable source mapped, there is nothing - // to classify against - never true, regardless of FileIDs. - bare := &domain.Game{ID: "bare"} + // to classify against - never true, regardless of FileIDs. DeployCompile + // is set so this exercises the resolution-failure path, not the + // deploy-mode short-circuit. + bare := &domain.Game{ID: "bare", DeployMode: domain.DeployCompile} if svc.ModHasPakMergeSource(bare, &domain.InstalledMod{FileIDs: []string{"pak"}}) { t.Error("ModHasPakMergeSource must be false for a game with no MergeCompiler source") } + + // NOT gated on DeployMode: `lmm mod convert` persists the flag on + // non-compile games (advisory-only there), so a resolvable compiler + // still classifies - pre-#256 static behavior. + nonCompile := &domain.Game{ID: "g3", SourceIDs: map[string]string{"fake-compiler": "g3"}} + if !svc.ModHasPakMergeSource(nonCompile, &domain.InstalledMod{FileIDs: []string{"pak"}}) { + t.Error("ModHasPakMergeSource must classify for a non-DeployCompile game with a resolvable compiler") + } + + // A nil game is false, never a panic. + if svc.ModHasPakMergeSource(nil, &domain.InstalledMod{FileIDs: []string{"pak"}}) { + t.Error("ModHasPakMergeSource must be false for a nil game") + } } func TestFingerprintEqualityIgnoresOutcomes(t *testing.T) {