fix(run): report where the screenshot actually landed - #334
Conversation
`labelle run --screenshot=<path>` printed "screenshot will be written to '<path>'" before the run and never checked the result. The CLI only forwards LABELLE_SCREENSHOT_PATH; the backend owns the real filename. bgfx writes TGA and appends its own `.tga` (labelle-bgfx#57), so `--screenshot=shot.png` lands at `shot.png.tga` — the announced path never exists and the capture reads as a silent failure. It isn't: the file is there, under another name. A second trap compounds it: a RELATIVE path is resolved by the game against its own cwd, `.labelle/<target>/`, not the user's shell cwd, so `--screenshot=shot.png` doesn't appear where it was typed either. Report the truth after the run instead of promising it beforehand: - Pre-run line is now "screenshot requested:", not "will be written to". - After the game exits, resolve the path against the cwd the game actually ran in, then print "screenshot written to '<real path>'". - If the exact path is absent, probe the extensions a backend may append (.tga/.png/.bmp) and name the file that exists, plus a note that the requested path does not. - If nothing landed at all, warn and list every path checked, with a hint about backends that need a native surface. Help text now states that the backend owns the final filename and that a relative path resolves against the game's cwd. Verified against flying-platform-labelle on bgfx/Metal — absolute path with appended .tga, relative path resolving into .labelle/bgfx_desktop/, and an unwritable destination each report correctly. Claude-Session: https://claude.ai/code/session_01328ogbqzrsy2LN6PRz21gc
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe CLI probes requested screenshot paths before and after Docker or non-Docker runs, reports the actual output location or missing result, and documents backend filename behavior and progress JSON guarantees. ChangesScreenshot reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RunPipeline
participant GameBackend
participant Filesystem
RunPipeline->>Filesystem: fingerprint requested and suffixed paths
RunPipeline->>GameBackend: launch game with screenshot request
GameBackend->>Filesystem: write exact or suffixed screenshot file
GameBackend-->>RunPipeline: game exits
RunPipeline->>Filesystem: compare post-run file stamps
Filesystem-->>RunPipeline: changed screenshot path or no matching output
RunPipeline-->>RunPipeline: report screenshot outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae68e943dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (util.fileExists(resolved)) { | ||
| std.debug.print("labelle: screenshot written to '{s}'\n", .{resolved}); |
There was a problem hiding this comment.
When the requested path already exists from an earlier run, this check reports that stale file as the screenshot produced now, even if the current capture failed; it can also hide a newly created suffixed file such as shot.png.tga because the old exact path wins first. The suffix loop has the same stale-file problem. Record each candidate's pre-run state or remove/compare its metadata before claiming screenshot written to after the child exits.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/help.zig`:
- Line 16: Update the run command help text near the relative screenshot path
description to document Docker’s exception: when --docker is used, relative
paths resolve from project_dir as the game’s working directory instead of
.labelle/<target>/. Preserve the existing guidance to trust the post-run
“screenshot written to” path.
In `@src/cli/pipeline.zig`:
- Around line 905-915: Update the screenshot verification flow around the
requested path and screenshot_suffixes checks so it snapshots candidate file
metadata before spawning the game, then reports success only when the file is
newly created or its metadata changes afterward. Keep unchanged pre-existing
requested and suffixed files on the warning path, using the existing screenshot
reporting symbols and allocator flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63539166-2a9c-4bee-ba9b-162e8b0ae69d
📒 Files selected for processing (2)
src/cli/help.zigsrc/cli/pipeline.zig
Review findings on #334 (codex P2, CodeRabbit Major): the existence checks treated ANY file at a candidate path as this run's capture. Two failures followed, both of them the same misreporting bug this PR set out to fix: - A file left by an earlier run was reported as "screenshot written to" even when the current capture produced nothing. - A stale file at the exact requested path won the check and MASKED a newly written suffixed file, so `shot.png.tga` went unreported. Fingerprint every candidate (size + mtime) BEFORE the game spawns and treat only a created-or-changed file as this run's output. A pre-existing exact path that stayed untouched is now called out as leftover instead of being reported as success. Also addresses the second CodeRabbit finding: the help text claimed a relative path resolves against `.labelle/<target>/`, which is wrong under --docker (the game's cwd is the project dir there). The code already handled both — the probe resolves against the same cwd the run uses — but the docs named only one. Verified on flying-platform-labelle / bgfx: - stale file + failing capture -> warns, names the file as leftover - stale .png + fresh .tga -> reports the .tga, not the stale .png - absolute / relative / unwritable paths unchanged from before Claude-Session: https://claude.ai/code/session_01328ogbqzrsy2LN6PRz21gc
|
Round 1 findings addressed in e776160. codex P2 + CodeRabbit Major — stale files reported as this run's capture. Both valid, and pointing at the same defect from two angles. Existence alone proved nothing: a file from an earlier run was reported as freshly written, and a stale file at the exact requested path won the check and masked a newly written Fixed by fingerprinting every candidate (size + mtime) before the game spawns and treating only a created-or-changed file as this run's output. A pre-existing exact path that stayed untouched is now named as leftover rather than reported as success. Verified on flying-platform-labelle / bgfx: CodeRabbit Minor — Docker's different cwd. Valid as a docs gap. The code already handled it (the probe resolves against the same cwd the run uses, project dir under
|
1 participant
labelle run --screenshotannounces a path it doesn't necessarily write, and never verifies the result. This session I read that as "screenshot capture is silently broken on bgfx" and reported it as such — it wasn't. The file existed the whole time atshot.png.tga. Making the CLI tell the truth so that misreading isn't available.The two traps
1. The backend owns the filename. The CLI only forwards
LABELLE_SCREENSHOT_PATH. bgfx writes TGA and appends its own.tga(labelle-bgfx#57), so--screenshot=shot.pngproducesshot.png.tga. The announced path never exists;labelle runstill exits 0.2. A relative path resolves against the game's cwd, not yours. The game runs in
.labelle/<target>/so its saves land wherezig build runput them.--screenshot=shot.pngtherefore lands in.labelle/bgfx_desktop/, not the directory you typed it in.Together: the announced path is wrong, and the obvious place to look is also wrong.
Change
screenshot requested:rather thanwill be written to— it's a request, not a promise.--dockerand normal paths) and printscreenshot written to '<real path>'..tga/.png/.bmp), name the file that does exist, and note that the requested one doesn't.Exit codes are unchanged — worth a follow-up discussion whether a requested-but-absent capture should exit non-zero, but that's a behavior change for CI consumers and isn't bundled here.
Verification
zig buildandzig build testboth exit 0. Exercised end-to-end against flying-platform-labelle on bgfx/Metal:Confirmed in each case that the reported file exists and the unreported one doesn't.
Related
labelle-bgfx#57 is the underlying path bug. This PR doesn't fix that — it stops the CLI from misreporting it. If #57 lands and bgfx honors the requested path, this code degrades to simply printing the exact path.
https://claude.ai/code/session_01328ogbqzrsy2LN6PRz21gc
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.