feat(replayos): add RePlayOS platform integration#692
Merged
Conversation
- Add ReplayOS platform implementation with game tracking via /proc maps polling and inotify on _recent/ for menu-launched games - Add screenshot support using keyboard hotkey sequence with Caps Lock toggle to handle Keyboard Real Mode - Add systemd service with scheduling hardening (Nice=10, CPUWeight=10, CPUAffinity=0-2, IOSchedulingClass=best-effort, OOMScoreAdjust=500) to yield resources to replay - Add GOMAXPROCS(2) cap in daemon mode to leave CPU headroom for replay - Add ReplayOS build and deploy tasks - Refactor screenshot completeness checks (IEND/BMP header) into shared package used by both MiSTer and ReplayOS - Add platform ID constant and input mode support for ReplayOS - Add install.sh support for ReplayOS
📝 WalkthroughWalkthroughAdds a Linux ReplayOS platform: platform implementation (storage, autostart, launch), game tracker, screenshot support, system map, systemd unit and installer/CI/deploy tasks, shared screenshot helpers, tests, and input default adjustments for ReplayOS. Changes
Sequence DiagramsequenceDiagram
participant User
participant TUI
participant Platform
participant Autostart as Autostart File
participant Replay as replay.service
participant Tracker
participant Core as Emulator Core
participant RecFile as .rec File
User->>TUI: select ROM to launch
TUI->>Platform: LaunchMedia(romPath)
Platform->>Platform: validate ROM under active storage
Platform->>Autostart: write relative /roms/... autostart
Platform->>Replay: systemctl restart replay.service
Note over Replay: service boots and loads emulator core
Replay->>Core: load ROM
par Monitoring & recent-file
Tracker->>Replay: query replay MainPID
Tracker->>Tracker: inspect /proc/... -> loaded core
Tracker->>RecFile: watch for .rec creation
RecFile-->>Tracker: .rec created (contains ROM path)
end
Tracker->>Tracker: map ROM folder/core to SystemID
Tracker-->>TUI: report ActiveMedia (system, ROM, core)
TUI->>User: display running game
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 4
🧹 Nitpick comments (4)
pkg/platforms/shared/screenshot.go (1)
35-44: Consider adding JPEG support for forward compatibility.The dispatcher currently returns an error for unsupported formats. If RePlayOS or future platforms emit JPEG screenshots, this would fail unexpectedly. Consider either:
- Adding a basic JPEG completion check (SOI/EOI markers)
- Returning
(true, nil)for unknown formats with a log warning, allowing the caller to proceedThis is optional given the current PNG/BMP scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/shared/screenshot.go` around lines 35 - 44, The ScreenshotFileComplete function currently only handles PNG and BMP; add basic JPEG support by handling ".jpg" and ".jpeg" in ScreenshotFileComplete and calling a new helper JPEGFileComplete(path) that opens the file and checks JPEG SOI (0xFF 0xD8) at the start and EOI (0xFF 0xD9) at the end, returning (true,nil) if both markers are present and (false,error) on I/O or marker mismatch; ensure JPEGFileComplete's errors are propagated and update imports if needed (e.g., os/io) so callers won't fail for JPEG screenshots.scripts/install.sh (1)
500-541: Consider adding error handling formkdir -p.Line 524's
mkdir -p /media/sd/zaparoocould fail silently on permission issues before thecpfails with a less clear error.Proposed fix
# Install binary info "Installing to /media/sd/zaparoo/..." - mkdir -p /media/sd/zaparoo + if ! mkdir -p /media/sd/zaparoo; then + abort "Failed to create /media/sd/zaparoo directory" + fi if ! cp "${ZAPAROO_BIN}" /media/sd/zaparoo/zaparoo; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 500 - 541, In install_replayos, add error handling around the directory creation step: after calling mkdir -p /media/sd/zaparoo check its exit status and abort with a clear message (including the command error or $?) if it fails, so that mkdir failures (e.g., permissions) are reported before the subsequent cp; update the mkdir invocation in install_replayos to perform this check and call abort "Failed to create /media/sd/zaparoo" (or similar) on failure.pkg/platforms/replayos/screenshot.go (1)
83-107: Consider logging the best-effort Caps Lock restoration attempt.Line 94 silently discards the error from the best-effort Caps Lock restoration. While this is intentional (as indicated by the comment), adding a trace-level log would aid debugging if users report issues with keyboard state after failed screenshots.
🔧 Optional: Add trace logging for best-effort restoration
if err := p.KeyboardPress("s"); err != nil { if p.keyboardRealMode { // Best-effort: try to restore Real Mode even after the screenshot key failed. - _ = p.KeyboardPress("{capslock}") + if restoreErr := p.KeyboardPress("{capslock}"); restoreErr != nil { + log.Trace().Err(restoreErr).Msg("best-effort capslock restoration also failed") + } } return fmt.Errorf("send screenshot key: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/replayos/screenshot.go` around lines 83 - 107, In triggerScreenshot on Platform, replace the silent discard of the best‑effort Caps Lock restoration (the `_ = p.KeyboardPress("{capslock}")` inside the error branch after sending the "s" key) with a trace‑level log that records the error; call the existing logger (e.g. log.Trace().Err(err).Msg("best-effort restore keyboard real mode after failed screenshot") ) so the attempt remains non‑fatal but the failure is visible for debugging.pkg/platforms/replayos/platform_test.go (1)
301-351: Add test for successful launch path inTestLaunchGame.The
TestLaunchGameonly tests error cases. A test verifying the happy path (ROM on active storage successfully launching) would improve coverage, though it requires mockingrestartReplayService()or restructuring for testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/replayos/platform_test.go` around lines 301 - 351, Add a "happy path" subtest inside TestLaunchGame that sets up a temp storage with mkRoms, sets p := newTestPlatform(), p.activeStorage and p.storagePaths to that storage, and uses a way to stub/mock restartReplayService on Platform so it does not actually restart (e.g., set a replaceable method or inject a no-op before calling launchGame); call p.launchGame(nil, romPath, nil) where romPath is under the active storage and assert no error and expected return values (e.g., non-nil launch info or correct resolved path). Ensure the test cleans up with defer p.cancel() and runs t.Parallel() like the others.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/replayos/main.go`:
- Around line 179-220: The two user-facing prints in installService and
uninstallService (the fmt.Println calls) violate the guideline to use zerolog;
replace them with zerolog info-level logs (e.g., log.Info().Msg("...")) and
remove the fmt dependency from this file, ensuring you import
github.com/rs/zerolog/log; apply the same change pattern to other platform
entrypoints mentioned (functions in cmd/mistex/main.go and cmd/batocera/main.go)
so all CLI confirmation output uses zerolog rather than fmt.Println, or if you
intentionally allow prints, add an explicit documented exemption to the coding
guidelines and reference it in these functions.
In `@pkg/platforms/replayos/platform.go`:
- Around line 257-284: The Generic `.sh` launcher in the Launchers method uses
context.Background(), so launched commands won't be cancelled with the platform;
update the closure to capture the receiver and call exec.CommandContext with the
platform's context (e.g., use p.ctx or the platform's context field/method)
instead of context.Background(), keeping the rest of the closure (error wrap and
nil process return) intact; locate this in the Launchers method where the
Generic launcher closure is defined and replace context.Background() with the
platform context.
- Around line 379-419: detectStorages can return an active mount (mount) that
wasn’t detected in all (no roms/), causing active not to be in storagePaths;
after resolving mount via tokenMap[token] in detectStorages, validate that mount
is present in the detected list (all) and if it is not, treat it like an unknown
token: if all has entries, warn and return all[0] as fallback, otherwise return
an error—use symbols token, mount, all, tokenMap, and the function
detectStorages/readStorageToken to locate and implement this check.
In `@pkg/platforms/replayos/tracker.go`:
- Around line 187-207: The file lacks tests for findReplayChild because it
directly calls os.ReadFile and isReplayBinary, making it hard to stub; refactor
to introduce package-level variables (e.g., var readFile = os.ReadFile and var
isReplayBinaryFunc = isReplayBinary) and update findReplayChild to call
readFile(...) and isReplayBinaryFunc(...), then add a _test.go that replaces
readFile and isReplayBinaryFunc with test stubs to simulate: (1) a children file
containing a replay PID and isReplayBinaryFunc returning true for that PID
(assert returned PID), and (2) children with no replay PID or malformed fields
(assert error). Ensure tests restore the originals after each case.
---
Nitpick comments:
In `@pkg/platforms/replayos/platform_test.go`:
- Around line 301-351: Add a "happy path" subtest inside TestLaunchGame that
sets up a temp storage with mkRoms, sets p := newTestPlatform(), p.activeStorage
and p.storagePaths to that storage, and uses a way to stub/mock
restartReplayService on Platform so it does not actually restart (e.g., set a
replaceable method or inject a no-op before calling launchGame); call
p.launchGame(nil, romPath, nil) where romPath is under the active storage and
assert no error and expected return values (e.g., non-nil launch info or correct
resolved path). Ensure the test cleans up with defer p.cancel() and runs
t.Parallel() like the others.
In `@pkg/platforms/replayos/screenshot.go`:
- Around line 83-107: In triggerScreenshot on Platform, replace the silent
discard of the best‑effort Caps Lock restoration (the `_ =
p.KeyboardPress("{capslock}")` inside the error branch after sending the "s"
key) with a trace‑level log that records the error; call the existing logger
(e.g. log.Trace().Err(err).Msg("best-effort restore keyboard real mode after
failed screenshot") ) so the attempt remains non‑fatal but the failure is
visible for debugging.
In `@pkg/platforms/shared/screenshot.go`:
- Around line 35-44: The ScreenshotFileComplete function currently only handles
PNG and BMP; add basic JPEG support by handling ".jpg" and ".jpeg" in
ScreenshotFileComplete and calling a new helper JPEGFileComplete(path) that
opens the file and checks JPEG SOI (0xFF 0xD8) at the start and EOI (0xFF 0xD9)
at the end, returning (true,nil) if both markers are present and (false,error)
on I/O or marker mismatch; ensure JPEGFileComplete's errors are propagated and
update imports if needed (e.g., os/io) so callers won't fail for JPEG
screenshots.
In `@scripts/install.sh`:
- Around line 500-541: In install_replayos, add error handling around the
directory creation step: after calling mkdir -p /media/sd/zaparoo check its exit
status and abort with a clear message (including the command error or $?) if it
fails, so that mkdir failures (e.g., permissions) are reported before the
subsequent cp; update the mkdir invocation in install_replayos to perform this
check and call abort "Failed to create /media/sd/zaparoo" (or similar) on
failure.
🪄 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: f7331f6b-8a6f-412b-8ea6-0cadf95021e3
📒 Files selected for processing (20)
.github/workflows/build.ymlTaskfile.dist.ymlcmd/replayos/conf/zaparoo.servicecmd/replayos/main.gopkg/platforms/ids/ids.gopkg/platforms/mister/screenshot.gopkg/platforms/replayos/platform.gopkg/platforms/replayos/platform_test.gopkg/platforms/replayos/screenshot.gopkg/platforms/replayos/screenshot_test.gopkg/platforms/replayos/systemmap.gopkg/platforms/replayos/tracker.gopkg/platforms/replayos/tracker_test.gopkg/platforms/shared/screenshot.gopkg/platforms/shared/screenshot_test.gopkg/zapscript/input.gopkg/zapscript/input_test.goscripts/install.shscripts/tasks/replayos.ymlscripts/tasks/utils/makezip/main.go
… active mount detection - deleteAutostart: restructure to only log "deleted autostart file" when os.Remove succeeds; ErrNotExist is silently ignored, other errors warn. Both log entries now include the file path. - detectStorages: after resolving a valid system_storage token, verify the resolved mount is actually in the detected list (has a roms/ directory). If it is not, warn and fall back to the first detected mount, matching the behaviour of the unknown-token and missing-config paths. - screenshot: log best-effort Caps Lock restoration failures at trace level instead of discarding the error silently. - install.sh: abort with a clear message if mkdir -p /media/sd/zaparoo fails, so permissions errors are not masked by the subsequent cp failure.
…and screenshot Add command.Executor, procPath, and clockwork.Clock injection fields to Platform to enable hermetic testing of systemctl calls, /proc reads, and time-dependent key sequences. Convert restartReplayService, getReplayPID, isReplayBinary, findReplayChild, getLoadedCore, and healthCheck to methods that use these fields. Add tests for SystemMap.GetLauncherID, Platform trivial returns, RootDirs, Launchers, deleteAutostart, StopActiveLauncher, restartReplayService, launchGame success path, isReplayBinary, findReplayChild, getLoadedCore, getReplayPID, checkAndUpdateRunningGame, startGameTracker shutdown, startRecentWatcher, waitForScreenshot, and triggerScreenshot key sequences including capslock-wrap and error recovery paths. Add PNGFileComplete and BMPFileComplete missing-file error tests in pkg/platforms/shared.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/platforms/replayos/platform.go (1)
407-529: Thread the ReplayOS filesystem helpers throughafero.Fs.
detectStorages,readStorageToken,readRealMode,writeAutostart, anddeleteAutostartare all new testable helpers, but they still hard-wireos.*calls. Please move them behind an injectedafero.Fslike the other new seams in this platform so they can be exercised hermetically.As per coding guidelines, "pkg/**/*.go: Use afero for filesystem operations in testable code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/replayos/platform.go` around lines 407 - 529, The helper functions detectStorages, readStorageToken, readRealMode, writeAutostart, and deleteAutostart currently call os.* directly; change them to accept an afero.Fs (or use a struct field that holds afero.Fs) so all filesystem ops use afero (replace os.Stat, os.ReadFile, os.MkdirAll, os.WriteFile, os.Remove, filepath.Rel remains) and update callers to pass the injected fs; update function signatures (e.g., detectStorages(fs afero.Fs, cfgPath string, ...), readStorageToken(fs afero.Fs, cfgPath string), readRealMode(fs afero.Fs, cfgPath string), writeAutostart(fs afero.Fs, storagePath, romPath string), deleteAutostart(fs afero.Fs, storagePath string)) and ensure error handling/log messages remain unchanged.
🤖 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/replayos/platform.go`:
- Around line 533-539: Change restartReplayService to return an error (func (p
*Platform) restartReplayService() error): call systemctl and if it fails return
the error instead of only logging it. Update every caller of
restartReplayService to check the returned error and abort/surface the failure
before mutating platform state (do not clear media/tracker state or report
success on failure); propagate or handle the error appropriately so the
operation fails fast. Ensure callers that previously ignored errors now either
return the error up the stack or log and stop further state changes.
In `@pkg/platforms/replayos/screenshot.go`:
- Around line 72-77: The current code records a raw wall-clock timestamp
(triggerTime := time.Now()) and then waitForScreenshot filters files by
ModTime() >= that timestamp, which fails on filesystems with coarse/truncated
mtimes; instead capture the newest existing screenshot file/path and its ModTime
immediately before calling p.triggerScreenshot(), then call waitForScreenshot
with that baseline (or a small filesystem-resolution margin) and update
waitForScreenshot to accept any file whose path differs from the pre-trigger
path OR whose ModTime is newer than the pre-trigger ModTime minus a small
resolution margin; refer to triggerTime, triggerScreenshot(),
waitForScreenshot(), and the pre-trigger newest file/path when making the
change.
---
Nitpick comments:
In `@pkg/platforms/replayos/platform.go`:
- Around line 407-529: The helper functions detectStorages, readStorageToken,
readRealMode, writeAutostart, and deleteAutostart currently call os.* directly;
change them to accept an afero.Fs (or use a struct field that holds afero.Fs) so
all filesystem ops use afero (replace os.Stat, os.ReadFile, os.MkdirAll,
os.WriteFile, os.Remove, filepath.Rel remains) and update callers to pass the
injected fs; update function signatures (e.g., detectStorages(fs afero.Fs,
cfgPath string, ...), readStorageToken(fs afero.Fs, cfgPath string),
readRealMode(fs afero.Fs, cfgPath string), writeAutostart(fs afero.Fs,
storagePath, romPath string), deleteAutostart(fs afero.Fs, storagePath string))
and ensure error handling/log messages remain unchanged.
🪄 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: 116cb4d8-b5ee-4fc3-9515-a29a98885cab
📒 Files selected for processing (10)
.github/workflows/build.ymlpkg/platforms/replayos/platform.gopkg/platforms/replayos/platform_test.gopkg/platforms/replayos/screenshot.gopkg/platforms/replayos/screenshot_test.gopkg/platforms/replayos/systemmap_test.gopkg/platforms/replayos/tracker.gopkg/platforms/replayos/tracker_test.gopkg/platforms/shared/screenshot_test.goscripts/install.sh
✅ Files skipped from review due to trivial changes (1)
- pkg/platforms/replayos/screenshot_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/platforms/shared/screenshot_test.go
- scripts/install.sh
- pkg/platforms/replayos/tracker.go
- pkg/platforms/replayos/tracker_test.go
…t mtime baseline restartReplayService now returns an error instead of logging and continuing. StopActiveLauncher and launchGame check the error and abort before mutating platform state, so a failed systemctl restart no longer clears active media or reports success to callers. Screenshot captures the newest pre-existing PNG path and mtime before triggering, then waitForScreenshot accepts any file whose path differs from the baseline or whose mtime is strictly after it. This avoids false negatives on exFAT/ext4 filesystems where coarse mtime resolution can cause a newly written file to compare as older than a wall-clock triggerTime. findNewestPNG (time-filtered) is replaced by newestPNG (returns newest unconditionally); callers now own the baseline comparison. Tests updated to match the new signatures and to cover the error propagation and coarse-mtime paths.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/platforms/replayos/platform.go (1)
302-303:⚠️ Potential issue | 🟡 MinorGeneric
.shlauncher still bypasses the platform context.Line 303 starts the script with
context.Background(), sop.cancel()will not stop it during platform shutdown. Reuse the platform context here instead of detaching the process from the platform lifecycle.#!/bin/bash # Verify that the platform owns a cancellable context, but the Generic launcher does not use it. rg -n -C2 'context\.WithCancel|Start\(context\.Background\(' pkg/platforms/replayos/platform.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/replayos/platform.go` around lines 302 - 303, The Launch function is starting the generic .sh launcher with context.Background(), which detaches it from the platform lifecycle; change the call in Launch (the p.cmdExec().Start invocation) to use the platform's cancellable context (e.g., p.ctx or the context returned/owned by the platform instance) so that p.cancel() will stop the process on shutdown; update any callers or struct fields if needed to ensure Launch can access the platform context rather than using context.Background().
🤖 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/replayos/platform.go`:
- Around line 219-225: StopActiveLauncher currently ignores failures from
deleteAutostart(p.activeStorage) and proceeds to call p.restartReplayService(),
which can relaunch the previous game; change the flow so that deleteAutostart's
error is checked and returned immediately (do not proceed to
restartReplayService) when non-nil. Update the same pattern in the delayed
cleanup path (the other StopActiveLauncher occurrence around the 524-534
region). Look for the symbols StopActiveLauncher, p.activeStorage,
deleteAutostart, and restartReplayService to locate the code and make the
deleteAutostart error cause an early return instead of continuing.
- Around line 362-372: The code writes the autostart (autostart.auto) and sets
p.pendingROMPath under p.trackerMu, but if p.restartReplayService() fails it
returns early leaving a stale autostart file and p.pendingROMPath set; update
the error path in the function that calls writeAutostart and
restartReplayService so that on restart error you (1) remove or revert the
autostart file (the writeAutostart side-effect) and (2) clear p.pendingROMPath
under p.trackerMu before returning the error, ensuring the mutex is
locked/unlocked correctly; use the existing symbols writeAutostart, p.trackerMu,
p.pendingROMPath and p.restartReplayService to locate and implement the
rollback.
- Around line 417-418: The filesystem helpers in platform.go currently call os.*
(e.g., os.Stat, os.ReadDir) directly (see the os.Stat usage around the check
that appends path to all), which prevents hermetic tests; change the Platform
type or the helper function signatures to accept an afero.Fs (or embed afero.Fs
on Platform) and replace direct os.* calls with the afero.Fs equivalents (Stat,
ReadDir, ReadFile, etc.) in all helper functions referenced (including the
blocks at ~457-480 and ~504-526). Ensure all places that construct Platform or
call these helpers provide an afero.NewOsFs() in production and a memMapFs in
tests so behavior stays the same while enabling test isolation.
---
Duplicate comments:
In `@pkg/platforms/replayos/platform.go`:
- Around line 302-303: The Launch function is starting the generic .sh launcher
with context.Background(), which detaches it from the platform lifecycle; change
the call in Launch (the p.cmdExec().Start invocation) to use the platform's
cancellable context (e.g., p.ctx or the context returned/owned by the platform
instance) so that p.cancel() will stop the process on shutdown; update any
callers or struct fields if needed to ensure Launch can access the platform
context rather than using context.Background().
🪄 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: 0749e402-ff9b-40a5-b50e-4f9aaa20dd93
📒 Files selected for processing (4)
pkg/platforms/replayos/platform.gopkg/platforms/replayos/platform_test.gopkg/platforms/replayos/screenshot.gopkg/platforms/replayos/screenshot_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/platforms/replayos/platform_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/platforms/replayos/screenshot.go
- pkg/platforms/replayos/screenshot_test.go
Comment on lines
+417
to
+418
| if info, statErr := os.Stat(romsPath); statErr == nil && info.IsDir() { | ||
| all = append(all, path) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inject an afero.Fs for these filesystem helpers.
These helpers are all testable platform logic, but they hard-code os.* filesystem access. Threading an afero.Fs through Platform or these helper methods would keep ReplayOS tests hermetic and align this package with the repo rule for pkg/**.
As per coding guidelines, pkg/**/*.go: Use afero for filesystem operations in testable code.
Also applies to: 457-480, 504-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/platforms/replayos/platform.go` around lines 417 - 418, The filesystem
helpers in platform.go currently call os.* (e.g., os.Stat, os.ReadDir) directly
(see the os.Stat usage around the check that appends path to all), which
prevents hermetic tests; change the Platform type or the helper function
signatures to accept an afero.Fs (or embed afero.Fs on Platform) and replace
direct os.* calls with the afero.Fs equivalents (Stat, ReadDir, ReadFile, etc.)
in all helper functions referenced (including the blocks at ~457-480 and
~504-526). Ensure all places that construct Platform or call these helpers
provide an afero.NewOsFs() in production and a memMapFs in tests so behavior
stays the same while enabling test isolation.
…eleteAutostart errors - deleteAutostart now returns error; StopActiveLauncher bails if deletion fails before calling restartReplayService - launchGame rolls back autostart file and clears pendingROMPath when restartReplayService fails, so a later manual restart does not autolaunch a ROM the caller never successfully started - launchGame health-check goroutine treats deleteAutostart as best-effort (logs warning instead of discarding the error silently) Tests: assert rollback in LaunchGame_RestartFails; cover deleteAutostart error path by placing a file where the directory should be.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/platforms/replayos/platform.go (2)
421-428: 🛠️ Refactor suggestion | 🟠 MajorBack these helpers with
afero.Fsinstead of directos.*calls.
detectStorages, the config readers, and the autostart helpers are all testable platform logic, but they still hit the host filesystem directly. Thread anafero.FsthroughPlatform/these helpers so ReplayOS tests can stay hermetic.As per coding guidelines,
pkg/**/*.go: Use afero for filesystem operations in testable code.Also applies to: 466-490, 512-546
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/replayos/platform.go` around lines 421 - 428, The helpers (notably detectStorages) currently call os.* directly; change them to use an injected afero.Fs so tests can provide an in-memory FS: add an afero.Fs field to the Platform struct (or pass afero.Fs into detectStorages and the config reader/autostart helper functions), replace os.Stat/os.ReadFile/os.MkdirAll/etc. with the afero.Fs equivalents (and keep filepath.Join for path composition), and update callers to pass Platform.fs (or the new fs argument); apply the same pattern to the other mentioned helpers around lines 466-490 and 512-546 so all filesystem operations use the injected afero.Fs.
304-306:⚠️ Potential issue | 🟡 MinorUse the platform context for Generic launcher processes.
context.Background()detaches the script from platform shutdown/cancellation, so a launched.shcan outlive ReplayOS lifecycle management. Please start it with the platform context instead.🛠️ Proposed fix
launchers = append(launchers, platforms.Launcher{ ID: "Generic", Extensions: []string{".sh"}, AllowListOnly: true, Launch: func(_ *config.Instance, path string, _ *platforms.LaunchOptions) (*os.Process, error) { - if err := p.cmdExec().Start(context.Background(), path); err != nil { + ctx := p.ctx + if ctx == nil { + ctx = context.Background() + } + if err := p.cmdExec().Start(ctx, path); err != nil { return nil, fmt.Errorf("failed to start command: %w", err) } return nil, nil //nolint:nilnil // Command launches don't return a process handle }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platforms/replayos/platform.go` around lines 304 - 306, The Launch callback uses context.Background() which detaches launched processes from platform shutdown; change the call in the Launch function so p.cmdExec().Start uses the platform's context instead of context.Background() (e.g., use p.ctx or the appropriate platform context accessor on the receiver) so launched scripts are canceled when the platform is shut down; update the single call site where p.cmdExec().Start is invoked in Launch to pass the platform context.
🤖 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/replayos/platform.go`:
- Around line 229-231: The code clears p.lastKnownCore while holding p.trackerMu
but leaves p.pendingROMPath intact, which can cause the next core transition to
be attributed to a stale ROM; under the same lock where p.lastKnownCore is reset
(guarded by p.trackerMu) also set p.pendingROMPath = "" (or nil as appropriate)
to clear any queued ROM path so the tracker won't use an old value—update the
block that currently calls p.trackerMu.Lock()/Unlock() to reset both
p.lastKnownCore and p.pendingROMPath together.
- Around line 384-394: The goroutine currently returns immediately on ctx.Done
and skips calling deleteAutostart, leaving stale autostart.auto; modify the
goroutine so deleteAutostart(activeStorage) is called in a best-effort path even
if ctx is cancelled (i.e., don’t return on the ctx.Done case before cleanup).
Concretely, restructure the select around the timer/ctx (or use a defer) so that
after the wait you still invoke deleteAutostart(activeStorage) and only skip the
subsequent healthCheck when ctx is cancelled; ensure you reference the existing
healthCheckDelay timer and call deleteAutostart(activeStorage) in the
cancellation path or defer block to guarantee cleanup.
---
Duplicate comments:
In `@pkg/platforms/replayos/platform.go`:
- Around line 421-428: The helpers (notably detectStorages) currently call os.*
directly; change them to use an injected afero.Fs so tests can provide an
in-memory FS: add an afero.Fs field to the Platform struct (or pass afero.Fs
into detectStorages and the config reader/autostart helper functions), replace
os.Stat/os.ReadFile/os.MkdirAll/etc. with the afero.Fs equivalents (and keep
filepath.Join for path composition), and update callers to pass Platform.fs (or
the new fs argument); apply the same pattern to the other mentioned helpers
around lines 466-490 and 512-546 so all filesystem operations use the injected
afero.Fs.
- Around line 304-306: The Launch callback uses context.Background() which
detaches launched processes from platform shutdown; change the call in the
Launch function so p.cmdExec().Start uses the platform's context instead of
context.Background() (e.g., use p.ctx or the appropriate platform context
accessor on the receiver) so launched scripts are canceled when the platform is
shut down; update the single call site where p.cmdExec().Start is invoked in
Launch to pass the platform context.
🪄 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: b0cc1a36-357e-4d51-88a7-ba655210cf28
📒 Files selected for processing (2)
pkg/platforms/replayos/platform.gopkg/platforms/replayos/platform_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/platforms/replayos/platform_test.go
Comment on lines
+229
to
+231
| p.trackerMu.Lock() | ||
| p.lastKnownCore = "" | ||
| p.trackerMu.Unlock() |
There was a problem hiding this comment.
Clear pendingROMPath when aborting back to the menu.
This resets lastKnownCore but leaves any queued ROM path behind. If the tracker hasn't consumed it yet, the next core transition can be attributed to the old ROM. Clear p.pendingROMPath under the same lock here.
🛠️ Proposed fix
p.trackerMu.Lock()
p.lastKnownCore = ""
+ p.pendingROMPath = ""
p.trackerMu.Unlock()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| p.trackerMu.Lock() | |
| p.lastKnownCore = "" | |
| p.trackerMu.Unlock() | |
| p.trackerMu.Lock() | |
| p.lastKnownCore = "" | |
| p.pendingROMPath = "" | |
| p.trackerMu.Unlock() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/platforms/replayos/platform.go` around lines 229 - 231, The code clears
p.lastKnownCore while holding p.trackerMu but leaves p.pendingROMPath intact,
which can cause the next core transition to be attributed to a stale ROM; under
the same lock where p.lastKnownCore is reset (guarded by p.trackerMu) also set
p.pendingROMPath = "" (or nil as appropriate) to clear any queued ROM path so
the tracker won't use an old value—update the block that currently calls
p.trackerMu.Lock()/Unlock() to reset both p.lastKnownCore and p.pendingROMPath
together.
Comment on lines
+384
to
+394
| go func() { | ||
| t1 := time.NewTimer(healthCheckDelay) | ||
| defer t1.Stop() | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-t1.C: | ||
| } | ||
| if err := deleteAutostart(activeStorage); err != nil { | ||
| log.Warn().Err(err).Msg("failed to clear autostart after launch") | ||
| } |
There was a problem hiding this comment.
Don't skip autostart cleanup on context cancellation.
If p.ctx is cancelled before the first timer fires, this goroutine exits without deleting autostart.auto. That leaves a stale boot target behind for the next replay.service restart. The cancellation path should still attempt best-effort cleanup and only skip healthCheck.
🛠️ Proposed fix
go func() {
t1 := time.NewTimer(healthCheckDelay)
defer t1.Stop()
select {
case <-ctx.Done():
- return
+ if err := deleteAutostart(activeStorage); err != nil {
+ log.Warn().Err(err).Msg("failed to clear autostart during shutdown")
+ }
+ return
case <-t1.C:
}
if err := deleteAutostart(activeStorage); err != nil {
log.Warn().Err(err).Msg("failed to clear autostart after launch")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| go func() { | |
| t1 := time.NewTimer(healthCheckDelay) | |
| defer t1.Stop() | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-t1.C: | |
| } | |
| if err := deleteAutostart(activeStorage); err != nil { | |
| log.Warn().Err(err).Msg("failed to clear autostart after launch") | |
| } | |
| go func() { | |
| t1 := time.NewTimer(healthCheckDelay) | |
| defer t1.Stop() | |
| select { | |
| case <-ctx.Done(): | |
| if err := deleteAutostart(activeStorage); err != nil { | |
| log.Warn().Err(err).Msg("failed to clear autostart during shutdown") | |
| } | |
| return | |
| case <-t1.C: | |
| } | |
| if err := deleteAutostart(activeStorage); err != nil { | |
| log.Warn().Err(err).Msg("failed to clear autostart after launch") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/platforms/replayos/platform.go` around lines 384 - 394, The goroutine
currently returns immediately on ctx.Done and skips calling deleteAutostart,
leaving stale autostart.auto; modify the goroutine so
deleteAutostart(activeStorage) is called in a best-effort path even if ctx is
cancelled (i.e., don’t return on the ctx.Done case before cleanup). Concretely,
restructure the select around the timer/ctx (or use a defer) so that after the
wait you still invoke deleteAutostart(activeStorage) and only skip the
subsequent healthCheck when ctx is cancelled; ensure you reference the existing
healthCheckDelay timer and call deleteAutostart(activeStorage) in the
cancellation path or defer block to guarantee cleanup.
This was referenced May 5, 2026
This was referenced Jul 11, 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.
pkg/platforms/replayosplatform implementation targeting RePlayOS (Raspberry Pi 4, Debian 13, libretro). Game tracking polls/proc/{pid}/mapsfor loaded cores every 2s and watches_recent/via inotify for games launched from the RePlayOS menu.s), with Caps Lock toggling to temporarily disable Keyboard Real Mode when needed.replay:Nice=10,CPUWeight=10,CPUAffinity=0-2,IOSchedulingClass=best-effortprio 7,IOWeight=10,OOMScoreAdjust=500. Also setsGOMAXPROCS(2)in the daemon.cmd/replayosentry point with TUI, daemon, install, and uninstall modes matching other platform binaries.scripts/tasks/replayos.ymlwithbuild-arm64anddeploytasks.pkg/platforms/shared/screenshot.go, shared by MiSTer and RePlayOS.ReplayOSplatform ID constant and unrestricted input mode default.scripts/install.sh.Summary by CodeRabbit
New Features
Refactor
Docs
Tests