feat(research): one option per way to run a game, SteamCMD recommended #22

2026-10-26T03:03:08Z

greg commented

2026-10-26 03:03:08 +00:00

A draft built on a ready-made image whose files all live outside /server went round three times and ended as WIP (#21), although research had also verified the game's Steam app, which SteamCMD installs straight into /server. The build picked among the variant's images on its own.

SteamCMD recommended: research reads each Steam app's OS list. When it ships a Linux server, the SteamCMD route is the recommended option. For Windows-only servers, images that bring Proton or Wine come first. The research prompt says the same.
One option per way to run: a variant that can run on several images becomes one option per image ("… (SteamCMD)", "… (image x/y)"), so the admin picks exactly how the server runs.
The build sticks to the choice: it uses exactly the chosen option and never switches to another. If that one can't be made to work, the PR says so, and another option can be picked.

Checked: Steam reports the dedicated server from #21 as windows,linux, so its SteamCMD option would have been recommended.

🤖 Generated with Claude Code

A draft built on a ready-made image whose files all live outside `/server` went round three times and ended as WIP (#21), although research had also verified the game's Steam app, which SteamCMD installs straight into `/server`. The build picked among the variant's images on its own. - **SteamCMD recommended:** research reads each Steam app's OS list. When it ships a Linux server, the SteamCMD route is the recommended option. For Windows-only servers, images that bring Proton or Wine come first. The research prompt says the same. - **One option per way to run:** a variant that can run on several images becomes one option per image ("… (SteamCMD)", "… (image x/y)"), so the admin picks exactly how the server runs. - **The build sticks to the choice:** it uses exactly the chosen option and never switches to another. If that one can't be made to work, the PR says so, and another option can be picked. Checked: Steam reports the dedicated server from #21 as `windows,linux`, so its SteamCMD option would have been recommended. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

👀 1

git_ai_bot reviewed

2026-10-26 03:04:26 +00:00

git_ai_bot left a comment

🤖 AI Code Review

1. Blocking issues

os.Rename to firstboot-out/ will fail when the directory doesn't exist

In build, when a draft fails and the code tries to set it aside:

setAside := filepath.Join("firstboot-out", o.id+"-best-draft")
// ...
if _, err := os.Stat(filepath.Join(dir, "game.yaml")); err == nil {
    os.RemoveAll(setAside)
    if err := os.Rename(dir, setAside); err != nil {
        return res, err
    }
    best = res
}

This path is reachable when the SteamCMD image has no StartupHint (first boot is skipped entirely, so firstboot-out/ is never created). The drafter then writes games/<id>/, the boot check fails, and the rename to firstboot-out/<id>-best-draft fails with "no such file or directory" because the firstboot-out/ parent doesn't exist.

Fix: create the parent directory before the rename:

if _, err := os.Stat(filepath.Join(dir, "game.yaml")); err == nil {
    os.RemoveAll(setAside)
    if err := os.MkdirAll(filepath.Dir(setAside), 0o755); err != nil {
        return res, err
    }
    if err := os.Rename(dir, setAside); err != nil {
        return res, err
    }
    best = res
}

2. Non-blocking suggestions

Abort on any per-image error skips remaining candidates. In build, if buildWithImage returns a non-nil error (e.g., the model API is rate-limited, or newDrafter fails), the loop exits immediately and the remaining images are never tried. The PR description frames the fallback as "when no draft passes with one, the next image gets a fresh start," which implies resilience to transient failures. Consider distinguishing "this image is fundamentally unusable" (currently handled via imageUnusable → problems, not an error) from "something went wrong trying this image" (an error) and, in the latter case, log and continue to the next candidate rather than aborting the whole build. At minimum, a clear log line explaining why the fallback was cut short would help operators.

Shared probe directory across image attempts. Every call to firstBoot uses the same output directory firstboot-out/<id>-probe, so the second image's probe silently overwrites the first's. This is functionally correct (the report is only read immediately after), but if you ever need to debug a multi-image run, the earlier probe is gone. Consider including the image (or a counter) in the probe name, e.g. o.id + "-probe-" + strconv.Itoa(i).

attemptNote "did not boot" branch is likely unreachable. Every failure path in buildWithImage sets res.Problems before returning with Passed == false, so the fallback string res.Image + ": did not boot" probably never fires. If it's purely defensive, a brief comment would make the intent clear; otherwise it could be removed to avoid suggesting a code path that doesn't exist.

3. Tests

The new unit tests (TestParseAppInfoFindsLinuxServers, TestWithSteamCMDGoesFirstOnlyForLinuxServers, TestRecommendPrefersTheSteamCMDVariant) cover the research-side logic well.

Gaps worth filling:

attemptNote — trivial to unit-test the three branches (passed, problems, neither). Guards against a future refactor accidentally reordering the switch.
build candidate filtering + fallback — the loop that collects candidates, sets aside a failed draft, and restores the best one is the core of this PR. A table-driven test that stubs buildWithImage (or extracts the loop into a helper that accepts a per-image function) would catch regressions in the rename/restore logic, including the firstboot-out directory issue above.
buildSummary with Tried — verify the "Images tried, in order" section appears when len(Tried) > 1 and is absent when len(Tried) <= 1.

4. Overall assessment

The design is sound: prioritising SteamCMD for Linux servers (where everything lands in /server) and adding a per-image fallback with a "best draft" safety net directly addresses the failure mode seen in #21 (Satisfactory writing to /config under a ready-made image). The research prompt, withSteamCMD ordering, recommend priority chain, and the new tests are all consistent and well-scoped.

The one blocking issue (firstboot-out not existing on the rename path) is a straightforward fix. I'd address that and add at least a light test for the fallback/restore loop before merging.

Automated review by AI Git Bot

## 🤖 AI Code Review ## 1. Blocking issues **`os.Rename` to `firstboot-out/` will fail when the directory doesn't exist** In `build`, when a draft fails and the code tries to set it aside: ```go setAside := filepath.Join("firstboot-out", o.id+"-best-draft") // ... if _, err := os.Stat(filepath.Join(dir, "game.yaml")); err == nil { os.RemoveAll(setAside) if err := os.Rename(dir, setAside); err != nil { return res, err } best = res } ``` This path is reachable when the SteamCMD image has no `StartupHint` (first boot is skipped entirely, so `firstboot-out/` is never created). The drafter then writes `games/<id>/`, the boot check fails, and the rename to `firstboot-out/<id>-best-draft` fails with "no such file or directory" because the `firstboot-out/` parent doesn't exist. **Fix:** create the parent directory before the rename: ```go if _, err := os.Stat(filepath.Join(dir, "game.yaml")); err == nil { os.RemoveAll(setAside) if err := os.MkdirAll(filepath.Dir(setAside), 0o755); err != nil { return res, err } if err := os.Rename(dir, setAside); err != nil { return res, err } best = res } ``` --- ## 2. Non-blocking suggestions **Abort on any per-image error skips remaining candidates.** In `build`, if `buildWithImage` returns a non-nil error (e.g., the model API is rate-limited, or `newDrafter` fails), the loop exits immediately and the remaining images are never tried. The PR description frames the fallback as "when no draft *passes* with one, the next image gets a fresh start," which implies resilience to transient failures. Consider distinguishing "this image is fundamentally unusable" (currently handled via `imageUnusable` → problems, not an error) from "something went wrong trying this image" (an error) and, in the latter case, log and continue to the next candidate rather than aborting the whole build. At minimum, a clear log line explaining why the fallback was cut short would help operators. **Shared probe directory across image attempts.** Every call to `firstBoot` uses the same output directory `firstboot-out/<id>-probe`, so the second image's probe silently overwrites the first's. This is functionally correct (the report is only read immediately after), but if you ever need to debug a multi-image run, the earlier probe is gone. Consider including the image (or a counter) in the probe name, e.g. `o.id + "-probe-" + strconv.Itoa(i)`. **`attemptNote` "did not boot" branch is likely unreachable.** Every failure path in `buildWithImage` sets `res.Problems` before returning with `Passed == false`, so the fallback string `res.Image + ": did not boot"` probably never fires. If it's purely defensive, a brief comment would make the intent clear; otherwise it could be removed to avoid suggesting a code path that doesn't exist. --- ## 3. Tests The new unit tests (`TestParseAppInfoFindsLinuxServers`, `TestWithSteamCMDGoesFirstOnlyForLinuxServers`, `TestRecommendPrefersTheSteamCMDVariant`) cover the research-side logic well. Gaps worth filling: - **`attemptNote`** — trivial to unit-test the three branches (passed, problems, neither). Guards against a future refactor accidentally reordering the `switch`. - **`build` candidate filtering + fallback** — the loop that collects candidates, sets aside a failed draft, and restores the best one is the core of this PR. A table-driven test that stubs `buildWithImage` (or extracts the loop into a helper that accepts a per-image function) would catch regressions in the rename/restore logic, including the `firstboot-out` directory issue above. - **`buildSummary` with `Tried`** — verify the "Images tried, in order" section appears when `len(Tried) > 1` and is absent when `len(Tried) <= 1`. --- ## 4. Overall assessment The design is sound: prioritising SteamCMD for Linux servers (where everything lands in `/server`) and adding a per-image fallback with a "best draft" safety net directly addresses the failure mode seen in #21 (Satisfactory writing to `/config` under a ready-made image). The research prompt, `withSteamCMD` ordering, `recommend` priority chain, and the new tests are all consistent and well-scoped. The one blocking issue (`firstboot-out` not existing on the rename path) is a straightforward fix. I'd address that and add at least a light test for the fallback/restore loop before merging. --- *Automated review by AI Git Bot*

greg added 1 commit 2026-10-26 03:10:23 +00:00

feat(research): one option per way to run a game, SteamCMD recommended

Validate game templates / validate (push) Successful in 19s

Details

Validate game templates / boot-test (push) Skipped

Details

Validate game templates / publish (push) Skipped

Details

f2ffed5c98

A draft built on a ready-made image whose files all live outside /server
went round three times and ended as WIP, although research had also
verified the game's Steam app, which SteamCMD installs straight into
/server. The build picked among the variant's images on its own.

- research reads each Steam app's OS list. When it ships a Linux server,
  the SteamCMD route is the recommended option; for Windows-only servers,
  images that bring Proton or Wine come first. The research prompt says
  the same.
- A variant that can run on several images becomes one option per image
  ("... (SteamCMD)", "... (image x/y)"), so the admin picks exactly how the
  server runs.
- build uses exactly the chosen option and never switches to another; if it
  can't be made to work, the PR says so and another option can be picked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

greg force-pushed feat/builder-image-fallback from 9f6e26bc0c to f2ffed5c98

2026-10-26 03:10:23 +00:00

Compare

greg changed title from ~~feat(builder): SteamCMD first for Linux servers, and fall back to the next image~~ to feat(research): one option per way to run a game, SteamCMD recommended

2026-10-26 03:10:26 +00:00

git_ai_bot reviewed

2026-10-26 03:12:05 +00:00

git_ai_bot left a comment

🤖 AI Code Review

1. Blocking issues

None found. The previous firstboot-out rename bug is resolved because the multi-image fallback (and its rename/restore logic) has been removed entirely.

2. Non-blocking suggestions

PR description no longer matches the code. The description still says "build tries the variant's images in order. When no draft passes with one, the next image gets a fresh start. The first draft that boots wins; if none does, the PR gets the draft that got furthest." The new architecture is the opposite: oneOptionPerImage splits each multi-image variant into separate single-image options, the admin picks one, and build uses that one image exclusively ("It never switches to another image"). The fallback is now at the admin/UI level (pick a different option), not inside build. Worth updating the description and title ("fall back to the next image" is no longer what happens) so reviewers and operators aren't misled.

buildSummary is computed twice. buildWithImage sets res.Summary = buildSummary(o, res) at several return points, and then build immediately overwrites it with the same call:

res, err := buildWithImage(o, image)
if res != nil {
    res.Summary = buildSummary(o, res)   // ← always runs, clobbering the inner one
}

Since build is the sole caller and always sets the summary when res != nil, the inner calls in buildWithImage are dead writes. Removing them (or removing the one in build and keeping the inner ones) would avoid the confusion of "which one is authoritative?" Either way is fine; just pick one place.

The for loop in build is at most one iteration. After oneOptionPerImage, every variant has exactly one image, so the range loop either executes once (the only image) or skips once (SteamCMD without app ID) and hits the error return. A simple if/single-element check would make the intent clearer and prevent someone from adding continue logic later that looks like a multi-image fallback but isn't:

func build(o buildOptions) (*buildResult, error) {
    if len(o.variant.Images) == 0 {
        return nil, errors.New("the variant has no image to build from")
    }
    image := o.variant.Images[0]
    if isSteamCMDImage(image) && o.variant.SteamAppID == "" {
        return nil, errors.New("the SteamCMD image is listed but no steamAppId was set")
    }
    res, err := buildWithImage(o, image)
    if res != nil {
        res.Summary = buildSummary(o, res)
    }
    return res, err
}

This also gives a more specific error message for the SteamCMD-without-app-ID edge case instead of the generic "no image to build from."

recommend inner loop doesn't break early. Once rec is set, the if rec < 0 guard makes the remaining iterations no-ops. Adding a break would make the intent clearer and save a few comparisons on large variant lists (unlikely to matter, but the loop reads like it's "searching for the first match" and a break confirms that):

for i, v := range variants {
    if want(v) {
        rec = i
        break
    }
}

(And the outer for _, want := range ... loop can also break once rec >= 0, since lower-priority predicates can't change the answer.)

3. Tests

The new tests (TestParseAppInfoFindsLinuxServers, TestWithSteamCMDGoesFirstOnlyForLinuxServers, TestRecommendPrefersTheSteamCMDVariant, TestOneOptionPerImageThenSteamCMDRecommended) cover the key new logic well. Two gaps worth filling:

oneOptionPerImage with a SteamCMD image but no SteamAppID. This produces a "SteamCMD" option that build will skip. A test asserting the option is created (so the admin sees it) but carries

Automated review by AI Git Bot

## 🤖 AI Code Review ## 1. Blocking issues None found. The previous `firstboot-out` rename bug is resolved because the multi-image fallback (and its rename/restore logic) has been removed entirely. --- ## 2. Non-blocking suggestions **PR description no longer matches the code.** The description still says *"build tries the variant's images in order. When no draft passes with one, the next image gets a fresh start. The first draft that boots wins; if none does, the PR gets the draft that got furthest."* The new architecture is the opposite: `oneOptionPerImage` splits each multi-image variant into separate single-image options, the admin picks one, and `build` uses that one image exclusively ("It never switches to another image"). The fallback is now at the admin/UI level (pick a different option), not inside `build`. Worth updating the description and title ("fall back to the next image" is no longer what happens) so reviewers and operators aren't misled. **`buildSummary` is computed twice.** `buildWithImage` sets `res.Summary = buildSummary(o, res)` at several return points, and then `build` immediately overwrites it with the same call: ```go res, err := buildWithImage(o, image) if res != nil { res.Summary = buildSummary(o, res) // ← always runs, clobbering the inner one } ``` Since `build` is the sole caller and always sets the summary when `res != nil`, the inner calls in `buildWithImage` are dead writes. Removing them (or removing the one in `build` and keeping the inner ones) would avoid the confusion of "which one is authoritative?" Either way is fine; just pick one place. **The `for` loop in `build` is at most one iteration.** After `oneOptionPerImage`, every variant has exactly one image, so the range loop either executes once (the only image) or skips once (SteamCMD without app ID) and hits the error return. A simple `if`/single-element check would make the intent clearer and prevent someone from adding `continue` logic later that looks like a multi-image fallback but isn't: ```go func build(o buildOptions) (*buildResult, error) { if len(o.variant.Images) == 0 { return nil, errors.New("the variant has no image to build from") } image := o.variant.Images[0] if isSteamCMDImage(image) && o.variant.SteamAppID == "" { return nil, errors.New("the SteamCMD image is listed but no steamAppId was set") } res, err := buildWithImage(o, image) if res != nil { res.Summary = buildSummary(o, res) } return res, err } ``` This also gives a more specific error message for the SteamCMD-without-app-ID edge case instead of the generic "no image to build from." **`recommend` inner loop doesn't break early.** Once `rec` is set, the `if rec < 0` guard makes the remaining iterations no-ops. Adding a `break` would make the intent clearer and save a few comparisons on large variant lists (unlikely to matter, but the loop reads like it's "searching for the first match" and a `break` confirms that): ```go for i, v := range variants { if want(v) { rec = i break } } ``` (And the outer `for _, want := range ...` loop can also `break` once `rec >= 0`, since lower-priority predicates can't change the answer.) --- ## 3. Tests The new tests (`TestParseAppInfoFindsLinuxServers`, `TestWithSteamCMDGoesFirstOnlyForLinuxServers`, `TestRecommendPrefersTheSteamCMDVariant`, `TestOneOptionPerImageThenSteamCMDRecommended`) cover the key new logic well. Two gaps worth filling: - **`oneOptionPerImage` with a SteamCMD image but no `SteamAppID`.** This produces a "SteamCMD" option that `build` will skip. A test asserting the option is created (so the admin sees it) but carries --- *Automated review by AI Git Bot*

greg merged commit 4874868ce6 into main

2026-10-26 03:12:15 +00:00

greg deleted branch feat/builder-image-fallback

2026-10-26 03:12:15 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gbreed-public/gbr-games#22