feat(mister): add RBF disambiguation cache and load_path config override#709
Merged
Merged
Conversation
When multiple RBF files share the same short name (e.g. three N64 cores in _Console/, _LLAPI/, _YCConsole/), the cache now stores all candidates per short name and uses the canonical directory from the system definition to pick the correct one. Adds a load_path field to LaunchersDefault config, allowing users to override which RBF is used for a given launcher, e.g.: [[launchers.default]] launcher = "Nintendo64" load_path = "_LLAPI/N64_LLAPI" This is resolved live at launch time via LookupLauncherDefaults, so a config reload is sufficient — no service restart required.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd optional launcher Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MGL as MGL Generator
participant Config as Config Instance
participant Cache as RBF Cache
participant Core as Core/System
User->>MGL: request GenerateMgl(core, rbfPath, path, override)
MGL->>Config: LookupLauncherDefaults(core.LauncherID / groups)
Config-->>MGL: LaunchersDefault (LoadPath?)
alt LoadPath configured
MGL->>Cache: GetByMglPath(load_path)
Cache->>Cache: parse dir + short name, select candidate by canonical dir
Cache-->>MGL: RBFInfo (from configured path)
else No LoadPath
MGL->>Cache: Resolve(cfg, core)
Cache->>Cache: consider launcher ID, system ID, select by canonical dir
Cache-->>MGL: RBFInfo or error
end
MGL->>Core: build MGL using resolved RBFInfo.MglName / Path
MGL-->>User: generated MGL content or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/config/configlaunchers_test.go (1)
370-403: Add a TOML/round-trip case forload_path.These tests only exercise
LookupLauncherDefaultswith in-memory structs. They will not catch a brokentoml:"load_path,omitempty"tag or a save/load regression, which is how this setting is actually consumed.As per coding guidelines "Write tests for all new code — use testify/mock, sqlmock, afero, and patterns from pkg/testing/".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/configlaunchers_test.go` around lines 370 - 403, The tests only exercise LookupLauncherDefaults with in-memory structs and miss a TOML round-trip for the load_path tag; add a unit test that marshals an Instance/Launchers containing a LaunchersDefault with LoadPath (the struct using the toml:"load_path,omitempty" tag) to TOML and unmarshals it back, then assert the decoded Instance/LaunchersDefault.LoadPath equals the original value (and optionally call LookupLauncherDefaults to verify behavior after round-trip); target symbols: LaunchersDefault, Instance, LookupLauncherDefaults and the toml struct tag to catch serialization/deserialization regressions.pkg/platforms/mister/cores/rbf_cache_test.go (1)
548-550: Usefilepath.Joinfor the fullPathfixtures.The new
RBFInfo.Pathtest data hardcodes/media/fat/...strings. Please build the filesystem paths withfilepath.Joinand keep the POSIX-only literals forMglNamevalues only.As per coding guidelines "Use
filepath.Joinfor path construction everywhere, including test files — never hardcode POSIX-style paths like/roms/snes/game.sfcas string literals".Also applies to: 625-633, 671-679, 711-719
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/mister/cores/rbf_cache_test.go` around lines 548 - 550, Replace hardcoded POSIX path literals in the RBFInfo test fixtures with platform-safe joins using filepath.Join: for the RBFInfo instances (console, unstable, rootLevel and the other fixtures around the noted ranges) set Path by calling filepath.Join("/media", "fat", ... , "<filename>") while keeping MglName and ShortName unchanged; import "path/filepath" in the test file if not already present and update the test variable initializations that reference RBFInfo.Path to use filepath.Join instead of embedding "/media/fat/..." strings.pkg/platforms/mister/mgls/mgls_test.go (1)
410-500: Build the new media-path fixtures withfilepath.Join.The added cases hardcode POSIX-style filesystem paths. Please build those with
filepath.Joinso the tests stay aligned with the repo's path-construction rule.As per coding guidelines "Use
filepath.Joinfor path construction everywhere, including test files — never hardcode POSIX-style paths like/roms/snes/game.sfcas string literals".Also applies to: 521-522
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/mister/mgls/mgls_test.go` around lines 410 - 500, The tests TestGenerateMgl_LoadPathOverride, TestGenerateMgl_LoadPathOverride_UsesLauncherID and TestGenerateMgl_NoLoadPath_FallsBackToCoreRBF currently pass hardcoded POSIX paths (e.g. "/media/fat/games/...") to GenerateMgl; replace those string literals with platform-safe path construction using filepath.Join when creating the game path argument (and any other added fixtures referenced in the same tests), so use filepath.Join("media", "fat", "games", "SNES", "game.sfc") etc. to build the paths and keep tests consistent across OSes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/platforms/mister/mgls/mgls.go`:
- Around line 319-336: The code uses system.ID when falling back to cache if no
load_path override exists, causing launcher-specific cores to be missed when
system.LauncherID is set; update the else branch in the block using coreKey
(computed from system.LauncherID or system.ID) so it calls
cores.GlobalRBFCache.GetBySystemID(coreKey) instead of GetBySystemID(system.ID),
and adjust the error/log messages to reference coreKey or include both system.ID
and coreKey for clarity; relevant symbols: system.LauncherID, coreKey,
cfg.LookupLauncherDefaults, cores.GlobalRBFCache.GetByMglPath,
cores.GlobalRBFCache.GetBySystemID.
---
Nitpick comments:
In `@pkg/config/configlaunchers_test.go`:
- Around line 370-403: The tests only exercise LookupLauncherDefaults with
in-memory structs and miss a TOML round-trip for the load_path tag; add a unit
test that marshals an Instance/Launchers containing a LaunchersDefault with
LoadPath (the struct using the toml:"load_path,omitempty" tag) to TOML and
unmarshals it back, then assert the decoded Instance/LaunchersDefault.LoadPath
equals the original value (and optionally call LookupLauncherDefaults to verify
behavior after round-trip); target symbols: LaunchersDefault, Instance,
LookupLauncherDefaults and the toml struct tag to catch
serialization/deserialization regressions.
In `@pkg/platforms/mister/cores/rbf_cache_test.go`:
- Around line 548-550: Replace hardcoded POSIX path literals in the RBFInfo test
fixtures with platform-safe joins using filepath.Join: for the RBFInfo instances
(console, unstable, rootLevel and the other fixtures around the noted ranges)
set Path by calling filepath.Join("/media", "fat", ... , "<filename>") while
keeping MglName and ShortName unchanged; import "path/filepath" in the test file
if not already present and update the test variable initializations that
reference RBFInfo.Path to use filepath.Join instead of embedding
"/media/fat/..." strings.
In `@pkg/platforms/mister/mgls/mgls_test.go`:
- Around line 410-500: The tests TestGenerateMgl_LoadPathOverride,
TestGenerateMgl_LoadPathOverride_UsesLauncherID and
TestGenerateMgl_NoLoadPath_FallsBackToCoreRBF currently pass hardcoded POSIX
paths (e.g. "/media/fat/games/...") to GenerateMgl; replace those string
literals with platform-safe path construction using filepath.Join when creating
the game path argument (and any other added fixtures referenced in the same
tests), so use filepath.Join("media", "fat", "games", "SNES", "game.sfc") etc.
to build the paths and keep tests consistent across OSes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42a87a96-9cf8-4ffa-a51a-973e5d398e55
📒 Files selected for processing (7)
pkg/config/configlaunchers.gopkg/config/configlaunchers_test.gopkg/platforms/mister/cores/rbf_cache.gopkg/platforms/mister/cores/rbf_cache_test.gopkg/platforms/mister/mgls/mgls.gopkg/platforms/mister/mgls/mgls_test.gopkg/platforms/shared/kodi/client_test.go
…load_path TOML round-trip test selectByCanonicalDir compared directory strings case-sensitively, so a user-supplied load_path with wrong casing (e.g. "_console/SNES" vs "_Console/SNES") would silently fall back to the first RBF candidate instead of the intended one. Switch to strings.EqualFold to match the already case-insensitive short-name lookup. Also adds a TOML round-trip test for the load_path field in LookupLauncherDefaults to catch serialization regressions.
…olve method Replace three separate resolution paths (resolveCoreRBF in mgls, inline load_path logic in GenerateMgl, and ResolveRBFPathForLauncher in cores) with a single (*RBFCache).Resolve method that handles load_path validation, alt core LauncherID lookup, and system ID fallback in one place. - Add RBFCache.Resolve(cfg, core): strict lookup, errors on cache miss, validates load_path against scanned RBFs - Remove ResolveRBFPath and ResolveRBFPathForLauncher free functions - Make GenerateMgl pure: drop cfg param, accept resolved rbfPath string - launchTempMgl now resolves via cache before calling GenerateMgl - LaunchCore simplified: remove duplicate load_path validation in SetName branch; SetName=="" branch uses Resolve directly for CmdInterface path - Remove resolveCoreRBF wrapper from mgls package - Move Resolve test coverage to cores package; all tests parallel, no global cache mutation
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/platforms/mister/cores/rbf_cache.go (1)
173-208: Consider adding nil-check forcoreparameter.The
Resolvemethod accessescore.LauncherIDandcore.IDwithout checking ifcoreis nil. While callers may always pass a valid core, a defensive check would prevent panics from programming errors.Proposed fix
func (c *RBFCache) Resolve(cfg *config.Instance, core *Core) (RBFInfo, error) { + if core == nil { + return RBFInfo{}, fmt.Errorf("core cannot be nil") + } key := core.LauncherID if key == "" { key = core.ID }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/mister/cores/rbf_cache.go` around lines 173 - 208, Resolve currently dereferences core (accessing core.LauncherID and core.ID) without checking for nil; add a defensive nil-check at the start of RBFCache.Resolve to return a clear error if core == nil (e.g. fmt.Errorf("nil core") or similar) before any use of core, preserving existing behavior for GetByMglPath, GetByLauncherID and GetBySystemID lookups when core is non-nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/platforms/mister/cores/rbf_cache.go`:
- Around line 173-208: Resolve currently dereferences core (accessing
core.LauncherID and core.ID) without checking for nil; add a defensive nil-check
at the start of RBFCache.Resolve to return a clear error if core == nil (e.g.
fmt.Errorf("nil core") or similar) before any use of core, preserving existing
behavior for GetByMglPath, GetByLauncherID and GetBySystemID lookups when core
is non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc1787b9-d238-40d3-90bb-2567cb9ce24a
📒 Files selected for processing (4)
pkg/platforms/mister/cores/rbf_cache.gopkg/platforms/mister/cores/rbf_cache_test.gopkg/platforms/mister/mgls/mgls.gopkg/platforms/mister/mgls/mgls_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/platforms/mister/mgls/mgls_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/platforms/mister/cores/rbf_cache.go (1)
94-113: Enforce locking invariants forBuildFromRBFsto prevent accidental races.This exported method mutates shared maps without taking
c.mu. It’s safe in the current refresh path, but a concurrent caller can race against readers/writers. Consider a locked wrapper plus a privatebuildFromRBFsLockedhelper.Refactor sketch
-func (c *RBFCache) BuildFromRBFs(rbfFiles []RBFInfo) { +func (c *RBFCache) BuildFromRBFs(rbfFiles []RBFInfo) { + c.mu.Lock() + defer c.mu.Unlock() + c.buildFromRBFsLocked(rbfFiles) +} + +func (c *RBFCache) buildFromRBFsLocked(rbfFiles []RBFInfo) { c.bySystemID = make(map[string]RBFInfo) c.byShortName = make(map[string][]RBFInfo) for _, rbf := range rbfFiles { key := strings.ToLower(rbf.ShortName) c.byShortName[key] = append(c.byShortName[key], rbf) } @@ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/mister/cores/rbf_cache.go` around lines 94 - 113, BuildFromRBFs currently mutates shared maps c.bySystemID and c.byShortName without taking c.mu, risking data races; wrap the mutation with the cache mutex by making BuildFromRBFs acquire c.mu (e.g., c.mu.Lock()/Unlock()) and move the current logic into an unexported helper buildFromRBFsLocked that assumes the lock is held, then have BuildFromRBFs call that helper while holding the lock; reference RBFCache, BuildFromRBFs, c.mu and buildFromRBFsLocked when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/platforms/mister/cores/rbf_cache.go`:
- Around line 140-146: GetByMglPath currently calls
selectByCanonicalDir(c.byShortName[strings.ToLower(shortName)], canonicalDir)
which silently falls back to the first candidate when canonicalDir matching
fails; change the logic so resolution is strict: if canonicalDir is non-empty
and no entry in the candidates matches canonicalDir, return (RBFInfo{}, false)
instead of returning the first candidate. Update selectByCanonicalDir (or the
caller GetByMglPath) to check candidates for a canonicalDir match and only
return the first candidate when canonicalDir is empty; apply the same
strict-match change to the other identical lookup code paths noted (the similar
block around lines 187-195) so a mistyped load_path does not resolve to an
unrelated core.
---
Nitpick comments:
In `@pkg/platforms/mister/cores/rbf_cache.go`:
- Around line 94-113: BuildFromRBFs currently mutates shared maps c.bySystemID
and c.byShortName without taking c.mu, risking data races; wrap the mutation
with the cache mutex by making BuildFromRBFs acquire c.mu (e.g.,
c.mu.Lock()/Unlock()) and move the current logic into an unexported helper
buildFromRBFsLocked that assumes the lock is held, then have BuildFromRBFs call
that helper while holding the lock; reference RBFCache, BuildFromRBFs, c.mu and
buildFromRBFsLocked when applying the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4d6c19f-a96e-435f-acdd-59f33f2decbc
📒 Files selected for processing (1)
pkg/platforms/mister/cores/rbf_cache.go
…herID
A mistyped load_path like _Unstable/SNES previously resolved silently to
_Console/SNES (the first candidate). Now returns (RBFInfo{}, false) when the
directory portion of the path doesn't match any cached entry, so Resolve
surfaces a clear error instead of loading the wrong core.
GetByLauncherID is similarly strict: a registered alt core path that names
a specific directory will only match an RBF in that directory. BuildFromRBFs
and selectByCanonicalDir retain the lenient fallback used for system ID
resolution.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/platforms/mister/cores/rbf_cache.go (2)
186-200: Consider extracting shared strict-match logic.
GetByLauncherID(lines 186-200) andGetByMglPath(lines 145-159) share nearly identical directory-matching logic. A small helper likestrictMatchByDir(candidates []RBFInfo, canonicalDir string) (RBFInfo, bool)would reduce duplication.♻️ Optional refactor to reduce duplication
+// strictMatchByDir returns a candidate matching canonicalDir exactly, or (RBFInfo{}, false). +// When canonicalDir is empty, returns the first candidate. +func strictMatchByDir(candidates []RBFInfo, canonicalDir string) (RBFInfo, bool) { + if len(candidates) == 0 { + return RBFInfo{}, false + } + if canonicalDir == "" { + return candidates[0], true + } + for _, c := range candidates { + dir, _ := splitRBFPath(c.MglName) + if strings.EqualFold(dir, canonicalDir) { + return c, true + } + } + return RBFInfo{}, false +}Then both
GetByMglPathandGetByLauncherIDcan delegate to it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/mister/cores/rbf_cache.go` around lines 186 - 200, Get rid of the duplicated directory-match loop in GetByLauncherID and GetByMglPath by extracting the logic into a helper like strictMatchByDir(candidates []RBFInfo, canonicalDir string) (RBFInfo, bool); have both GetByLauncherID and GetByMglPath call splitRBFPath to get canonicalDir/shortName as before, then if canonicalDir != "" delegate to strictMatchByDir which runs the for loop calling splitRBFPath(candidate.MglName) and strings.EqualFold on dirs, returning the matched RBFInfo or false, otherwise preserve the current short-name fallback behavior.
234-240: Minor: clarify error message when LauncherID is empty.When
core.LauncherIDis empty,keyequalscore.ID, but the error message reads "launcher %s" which could confuse debugging. Consider making it clearer:- return RBFInfo{}, fmt.Errorf( - "no core found for system %s (launcher %s, not in cache)", core.ID, key, - ) + return RBFInfo{}, fmt.Errorf( + "no core found for system %s (key=%s, not in cache)", core.ID, key, + )Or include both explicitly:
"system=%s launcherID=%s".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/mister/cores/rbf_cache.go` around lines 234 - 240, The error message returned when GetBySystemID(core.ID) fails uses "launcher %s" with variable key which may equal core.ID when core.LauncherID is empty; update the fmt.Errorf in the failure branch to explicitly include both the system ID and the launcher ID (e.g., "system=%s launcherID=%s") so callers can distinguish an empty launcherID from a different key value; change the fmt.Errorf in the function containing the call to c.GetBySystemID(core.ID) (the block that returns RBFInfo{}, fmt.Errorf(...)) to interpolate core.ID and core.LauncherID (or key and core.LauncherID) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/platforms/mister/cores/rbf_cache.go`:
- Around line 186-200: Get rid of the duplicated directory-match loop in
GetByLauncherID and GetByMglPath by extracting the logic into a helper like
strictMatchByDir(candidates []RBFInfo, canonicalDir string) (RBFInfo, bool);
have both GetByLauncherID and GetByMglPath call splitRBFPath to get
canonicalDir/shortName as before, then if canonicalDir != "" delegate to
strictMatchByDir which runs the for loop calling splitRBFPath(candidate.MglName)
and strings.EqualFold on dirs, returning the matched RBFInfo or false, otherwise
preserve the current short-name fallback behavior.
- Around line 234-240: The error message returned when GetBySystemID(core.ID)
fails uses "launcher %s" with variable key which may equal core.ID when
core.LauncherID is empty; update the fmt.Errorf in the failure branch to
explicitly include both the system ID and the launcher ID (e.g., "system=%s
launcherID=%s") so callers can distinguish an empty launcherID from a different
key value; change the fmt.Errorf in the function containing the call to
c.GetBySystemID(core.ID) (the block that returns RBFInfo{}, fmt.Errorf(...)) to
interpolate core.ID and core.LauncherID (or key and core.LauncherID)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38ea3926-34eb-45d3-974b-2a8210bd34ec
📒 Files selected for processing (2)
pkg/platforms/mister/cores/rbf_cache.gopkg/platforms/mister/cores/rbf_cache_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/platforms/mister/cores/rbf_cache_test.go
This was referenced May 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
byShortNamein the RBF cache changed frommap[string]RBFInfotomap[string][]RBFInfoto handle multiple RBF files sharing the same short name (e.g. three N64 cores across_Console/,_LLAPI/,_YCConsole/)selectByCanonicalDirpicks the correct candidate by matching the directory from the system definition, with a fallback to the first candidateload_pathfield toLaunchersDefaultconfig, allowing users to pin a specific RBF for any launcher without editing system definitionsGenerateMglaccepts*config.Instanceand checksload_pathbefore falling back to cache resolutionLaunchCoreuses the sameload_pathlogic viaGetByMglPathfor core-only launchesload_pathchanges — no restart requiredSummary by CodeRabbit
New Features
Improvements
Tests
Documentation