refactor: push Unreal/pak knowledge behind the MergeCompiler seam (#256)

[dyoung522]

Closes #256 — well, fixes; per repo convention the issue is closed manually at develop-merge.

What

internal/core knew what an Unreal .pak is. This PR pushes each of the issue's five enumerated leaks behind the source.MergeCompiler seam so core asks the source rather than knowing:

#	Leak	Now
1	resolveBasePak hardcoded <InstallPath>/Icarus/Content/Data/data.pak	mc.ResolveBaseArtifact(game) (icarus impl keeps the path + the locating base pak for %q wrap)
2	basePakIndexHash imported go-unrealpak in core	mc.FingerprintBase(path) — core holds an opaque string; unrealpak import gone from core non-test code
3	isConvertEligiblePakFile tested .pak suffix in core	split: core keeps the DeployCompile && ConvertPaks policy half (isConvertEligibleArtifact); the format half is mc.IsConvertibleArtifact
4	MergeSourcePak/MergeSourceExmodz constants in shared source package	moved into the icarus package; core classifies via mc.ClassifyMergeSource(id) (kind, convertible) and round-trips kinds as opaque strings (incl. stored fingerprint kinds and the legacy empty Kind)
5	mergedPakFileName = "zzz_LMM_Merged_P.pak" (UE sort-order convention) in core	mc.MergedArtifactName(); the "Icarus Merged Pak" display name likewise via mc.MergedArtifactLabel()

source.MergeCompiler widens from 2 to 8 methods and its doc block now states the complete contract a second DeployCompile game must implement — a new source package plus one registration line, zero core changes.

Behavior preservation

No user-visible change. Kind wire values ("pak"/"exmodz") and the merged filename are unchanged, so stored fingerprints, cache entries, and deployed artifacts all remain valid. Core resolves the compile source lazily on merge paths (only once a retained file actually needs classifying), so every no-compiler/no-retained flow that succeeded before still succeeds — verified by the existing no-compiler fall-through tests passing untouched.

Two deliberate structural choices worth reviewer eyes:

• mergeCompilerForGame(game) (struct-based) joins the existing mergeCompilerSourceForGame(gameID) — merged-pak paths always receive their game as a parameter and previously never consulted s.games, so resolving off the struct preserves that.
• ModHasPakMergeSource gains a *domain.Game parameter — classification now needs the game's compile source. Callers in cmd/lmm (×3), internal/tui (×1), and moddetail.go updated.

Test changes (all called out, none assert different behavior)

• New: internal/source/icarus/format_test.go covers the six seam methods (written first, TDD); TestMergeSourceKind's table lives on there as TestClassifyMergeSource.
• Fakes: compile-source fakes gain an embedded fakeMergeFormat (per test package: core_test, cmd/lmm, tui_test) mirroring the icarus conventions the fixtures already encode — wrappers (recordingMergeCompilerSource, pakOutcomeCompilerSource, healthE2ESource, failingValidateCompilerSource) inherit it for free.
• Fixtures: DeployCompile-game fixtures that previously relied on core's static classification now map/register a MergeCompiler source (moddetail_test, merged_pak_test's TestEnabledMergeSources…, mod_convert_test, both tui fixtures).
• Moved-symbol edits: source.MergeSource* constant references became literals/icarus constants; mergedFingerprintsEqual calls in merged_pak_internal_test.go pass the classifier; TestModHasPakMergeSource rewired to a registry-backed service and gains a no-compiler⇒false case.

The protected suites — service_icarus_compile_test.go, pak_convert_e2e_test.go, merged_pak_*_test.go — pass with only the accommodations above (embed lines, fixture source registration, moved-symbol references); every behavioral assertion is untouched.

Acceptance (from the issue)

• rg -l unrealpak internal/core/ → only _test.go files ✅
• core non-test sources: no .pak string literal, no Icarus path segment (remaining matches are prose comments) ✅
• the icarus package is the only place knowing base-pak location, pak format, .pak extension, merge-source kinds, merged filename ✅
• go build ./... && go vet ./... && gofmt -l . && go test -count=1 ./... → clean, 18/18 packages ok ✅

Round 2 (post-review follow-up)

• Merged origin/develop (fix(tui): auto-open a warnings overlay when an outcome collapses to (N warnings) #258 TUI fix, fix(cache): keep a converted pak's raw-fallback copy through prune; heal pruned entries (#250) #261 prune/heal fix). fix(cache): keep a converted pak's raw-fallback copy through prune; heal pruned entries (#250) #261's raw-fallback restore and prune exemption survive intact on top of the refactor; note its rawPakRestoreName reintroduces .pak/_P.pak naming knowledge into core — flagged to the coordinator as a candidate for a follow-up seam method, deliberately not restructured here.
• Sixth leak closed (coordinator-requested): core's static isExmodzFile is gone. A new IsNativeMergeSource(fileName) method (exact mirror of IsConvertibleArtifact) joins MergeCompiler — chosen over widening ClassifyMergeSource, whose unknown-id→native-kind default (legacy fingerprint compat) cannot distinguish a genuinely native archive from unknown junk. All four call sites route through Service.isNativeMergeFile, which falls back to the game's sole compile source so the mixed-source download hard error stays reachable. Two behavior deltas, both confined to games with no resolvable compile source: a native-archive import there still fails loud and caches nothing but as unsupported archive format: .exmodz (one test updated — the old compiler-specific message required core to know the extension); a standalone core.NewImporter now fails every DeployCompile import up front (original error verbatim; production never constructs one). The mixed-selection message is reworded to drop exmodz vocabulary (asserted substring unchanged).

Round 3

• rawPakRestoreName behind the seam (was flagged in round 2; acceptance criterion 2 required it): the .pak extension test reuses IsConvertibleArtifact (behaviorally identical to the EqualFold-on-Ext form for every filename), and the synthesized <mod-id>_P.pak heal name becomes the tenth method, RestoredArtifactName(modID) — the per-mod analogue of MergedArtifactName, byte-identical output so healed installs keep their on-disk names. The import-vs-download provenance split stays in core (it follows from ingest's fileID keying, uniform across games). fix(cache): keep a converted pak's raw-fallback copy through prune; heal pruned entries (#250) #261's heal/prune tests pass unmodified.
• Mixed-selection message now names the colliding files: Mod_P.pak and Mod.exmodz are alternate forms of the same mod - select one. Format-agnostic (vocabulary comes from the selection itself) and strictly more useful; trigger unchanged (booleans, not name-emptiness). Variant-exclusivity expectations updated deliberately; cmd/lmm/install_test.go's local fake updated to mirror the production shape it stands in for. CHANGELOG amended (this is the PR's one user-visible change).
• Copilot triage: round-5's three doc/rendering nits fixed; round-6's ModHasPakMergeSource note split — nil-game guard taken, the suggested DeployCompile short-circuit rejected with evidence (it breaks lmm mod convert's pinned advisory behavior on non-compile games, caught by the suite). Final round on a05274a: zero findings, zero suppressed.

Not done (deliberately)

• The issue's optional naming follow-on (merged_pak.go, MergedFingerprint, SyncMergedPak, …) — skipped per its own guidance; public-API churn for cosmetics.
• No plugin system, no second game (non-goals).
• installer.go untouched (sibling work on syncMergedPak's uninstall-to-zero branch errors when the merged-pak cache entry is absent (steady state after disabling the last merge source) #260 in flight).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR refactors the DeployCompile / merged-artifact pipeline so that Unreal .pak / Icarus-specific format knowledge no longer lives in internal/core, and is instead provided by the game’s source.MergeCompiler implementation (Icarus today) via an expanded interface contract.

Changes:

• Expand source.MergeCompiler to include base-artifact resolution, fingerprinting, convertibility checks, merge-source kind classification, and merged output naming/labeling; update core to depend on these methods instead of hardcoded .pak logic/strings.
• Add Icarus implementations for the new “format vocabulary” methods (internal/source/icarus/format.go) with dedicated unit tests.
• Update CLI/TUI/core tests and fixtures to register/implement the widened MergeCompiler seam (via embedded fakeMergeFormat helpers) and to pass *domain.Game where classification now requires it.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/tui/service_core.go	Updates TUI overview mapping to call ModHasPakMergeSource(game, mod) with the game context now needed for classification.
internal/tui/service_core_test.go	Adjusts TUI details fixture to map/register a merge-compiler source for ConvertPaks classification.
internal/tui/service_core_recompile_test.go	Embeds fakeMergeFormat in the TUI MergeCompiler fake to satisfy the widened interface.
internal/tui/service_core_convert_test.go	Registers a MergeCompiler source in the convert fixture so HasPakSource classification can resolve the compiler.
internal/tui/merge_format_helpers_test.go	Adds a TUI test helper implementing the MergeCompiler “format vocabulary” methods for fakes.
internal/source/source.go	Widens MergeCompiler interface and updates documentation to describe the full DeployCompile contract.
internal/source/icarus/merge.go	Switches pak-kind checks to Icarus-local merge-source kind constants.
internal/source/icarus/merge_test.go	Updates tests to use Icarus-local kind constants.
internal/source/icarus/format.go	Introduces Icarus’s MergeCompiler “format vocabulary” implementation (base artifact path, fingerprinting, convertibility, kind classification, merged name/label).
internal/source/icarus/format_test.go	Adds unit tests covering the new Icarus format-vocabulary methods.
internal/core/service.go	Removes unrealpak from core, adds mergeCompilerForGame(*Game) resolver, and routes convert-eligibility through mc.IsConvertibleArtifact.
internal/core/service_test.go	Updates compiler-related mocks to satisfy the widened MergeCompiler interface.
internal/core/service_icarus_compile_test.go	Updates core compile fakes to satisfy the widened MergeCompiler interface.
internal/core/pak_convert_e2e_test.go	Converts kind assertions from source.MergeSource* constants to literal kind strings round-tripped through the seam.
internal/core/moddetail.go	Updates ModDetail to call ModHasPakMergeSource(game, installed) with game context.
internal/core/moddetail_test.go	Registers/maps a MergeCompiler source for DeployCompile subtests now relying on compiler classification.
internal/core/merged_pak.go	Replaces core-owned .pak/kind/name logic with MergeCompiler classification and naming; adjusts fingerprint helpers to take a classifier function.
internal/core/merged_pak_test.go	Updates merged-pak fixtures to register/map a MergeCompiler source for classification.
internal/core/merged_pak_internal_test.go	Refactors internal tests to pass a classifier function; adds a minimal MergeCompiler fake for ModHasPakMergeSource tests.
internal/core/merged_pak_import_flow_test.go	Embeds fakeMergeFormat in the import-flow compiler fake to satisfy the widened interface.
internal/core/merge_format_helpers_test.go	Adds a core test helper implementing the MergeCompiler “format vocabulary” methods for fakes.
internal/core/importer.go	Routes convertible-artifact detection through the compiler seam (mc.IsConvertibleArtifact) and adjusts when compiler resolution is attempted.
internal/core/flows.go	Uses isConvertEligibleArtifact(game, mc, ...) rather than core’s former .pak-suffix predicate.
internal/core/flows_variant_exclusivity_test.go	Embeds fakeMergeFormat in the test source to satisfy the widened MergeCompiler interface.
cmd/lmm/mod.go	Updates mod convert to pass game context to ModHasPakMergeSource.
cmd/lmm/mod_convert_test.go	Registers a MergeCompiler-capable wrapper source so convert-flag surfaces can classify pak-kind fileIDs via the seam.
cmd/lmm/merge_format_helpers_test.go	Adds a CLI test helper implementing the MergeCompiler “format vocabulary” methods for fakes.
cmd/lmm/list.go	Updates list output to use ModHasPakMergeSource(game, mod) with game context.
cmd/lmm/install_compile_test.go	Embeds fakeMergeFormat in the CLI compile-source fake to satisfy the widened MergeCompiler interface.
CHANGELOG.md	Adds an Unreleased entry documenting the internal refactor and “no user-visible change” intent.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

internal/source/icarus/format.go:54

• Typo in comment: "week-correct" reads like a time period; likely meant "correct" (or similar).

// 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.

[Copilot]

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.

Suppressed comments (3)

internal/source/source.go:160

• The MergeCompiler doc comment enumerating the “format vocabulary” methods omits IsNativeMergeSource, even though it’s now part of the interface and required for DeployCompile imports/selection validation. This makes the interface contract description incomplete/inaccurate for implementers.

// 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

internal/core/service.go:141

• This error message still hard-codes “pak”, but the MergeCompiler seam is now intended to support compile-mode games whose raw artifact format may not be a pak. Consider making the message format-agnostic so it doesn’t leak Unreal vocabulary and remains correct for future DeployCompile sources.

	if native && other {
		return fmt.Errorf("raw pak and native merge archive are alternate forms of the same mod - select one")
	}

internal/core/service.go:1086

• isNativeMergeFile silently returns false when mergeCompilerForGame(game) errors. That can hide actionable configuration problems (e.g. ambiguous/multiple MergeCompiler sources) and cause native archives to fall back to the legacy extract/copy path, producing a generic “unsupported archive format” error instead of surfacing the real misconfiguration.

	if mc == nil {
		gmc, err := s.mergeCompilerForGame(game)
		if err != nil {
			return false
		}

[dyoung522]

Copilot round-2 triage (3 suppressed notes, none posted inline):

1. Interface doc omits IsNativeMergeSource — valid, fixed in 9b17f6a: the MergeCompiler preamble now enumerates it with the rest of the format vocabulary.
2. raw pak in the mixed-selection message — valid in spirit, fixed in 9b17f6a: now "raw artifact and native merge archive are alternate forms of the same mod - select one". It was the one remaining core string a second DeployCompile game would have had to edit; better to break the wording once, in the PR that already reworded it.
3. isNativeMergeFile swallows mergeCompilerForGame errors — declined. The nil-return is the designed degradation for "nothing can define native" (zero compilers). The other failure mode, ambiguous multiple compilers, cannot stay hidden: the very next merge-execution step — syncMergedPak, import's resolver, CheckMergedPakStaleness — performs the same resolution and fails loud with the ambiguity message. Propagating an error here would force the download/planning call sites to invent error policy for a doubly-misconfigured state those paths already report. Happy to revisit as a follow-up if a sentinel-error split (zero vs. ambiguous) is preferred.

[Copilot]

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.

[dyoung522]

Confirmed real — good catch, and it invalidated the round-2 design's premise: detectFormatFromPath does content-sniff zip magic after the extension miss, so the fall-through would have silently extracted a real zip-backed .exmodz. Fixed in 48012c2: an unresolvable MergeCompiler 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 message names the fix). This restored TestImportMod_DeployCompile_NoCompilerSourceFailsLoud to its original assertions and supersedes the import-side half of #221 I1 — the pak fall-through test is rewritten as …PakNoCompilerSourceFailsLoud with the history documented.

[dyoung522]

Real but bounded, and deliberately accepted on this path (documented on isNativeMergeFile in 0088253): hard-erroring here would break #221 I1's protected download contract — a non-MergeCompiler source's paks/zips must fall through to legacy (pinned by TestDownloadPak_NonMergeCompilerSource_FallsThroughToLegacyPath, whose game maps no compiler at all). The residual requires a hand-broken SourceIDs map (icarus is a built-in and always registered) and a foreign source serving real native archives; whenever the game maps its compiler, the pre-existing hard error still fires via the game-level fallback. The import path — which has no per-archive source contract — now hard-errors instead (48012c2).

[Copilot]

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/source/icarus/format_test.go:29

• Use the repo’s preferred Go octal literal style (0o644) instead of legacy 0644 for file mode constants.

	if err := os.WriteFile(wantPath, []byte("stub"), 0644); err != nil {

internal/source/icarus/format_test.go:25

• Use the repo’s preferred Go octal literal style (0o755) instead of legacy 0755 for file mode constants (this package already uses 0o644, etc.).

This issue also appears on line 29 of the same file.

	if err := os.MkdirAll(basePakDir, 0755); err != nil {

[Copilot]

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (3)

CHANGELOG.md:20

• The changelog bullet wraps a backticked example across a line break and the continuation line isn’t indented, which breaks Markdown rendering for the bullet and the inline-code span. Keep the example in a single code span and ensure all continuation lines are indented under the list item.

  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.

internal/source/source.go:163

• The MergeCompiler contract docblock lists the required format-vocabulary methods but omits RestoredArtifactName, which is also part of the required interface and matters for #250 healing behavior. Please include it so the “complete contract” claim stays accurate.

// 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

internal/core/service.go:212

• The doc comment block mentions "compilerSourceForGame", but the function is named mergeCompilerSourceForGame (and it resolves MergeCompiler sources). This mismatch can make code search/navigation confusing.

// 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 — at most one of a

[Copilot]

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (1)

internal/core/merged_pak.go:84

• ModHasPakMergeSource now always tries to resolve the game’s MergeCompiler, even for non-DeployCompile games. This can add repeated registry lookups (and construction of the “requires DeployCompile…” error) in callers that don’t guard on DeployCompile (e.g. the TUI Overview builds rows for all games). Since pak-conversion is only meaningful for DeployCompile games, this should short-circuit to false when game is nil or not DeployCompile (and also avoids a nil-game panic).

func (s *Service) ModHasPakMergeSource(game *domain.Game, mod *domain.InstalledMod) bool {
	if mod == nil {
		return false
	}
	mc, err := s.mergeCompilerForGame(game)

[Copilot]

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.