fix(boot-test): exactly what a new server gets, and pinpoint settings it can't start without #20

Merged

greg merged 2 commits from feat/builder-pinpoint-empty-settings into main 2026-10-26 02:08:51 +00:00

Conversation 2 Commits 2 Files Changed 7

+146 -57

greg commented 2026-10-26 01:57:22 +00:00

Owner

Copy link

Copy source

Boot tests now run with exactly what a new server gets. CI's boot test (and the builder's, before #18) gave every blank password-like setting a throwaway value. A real deploy only fills x-generate fields, so a template whose server won't start without one passed CI and failed for the first owner who deployed it.

• boottest-payload fills only x-generate fields, for CI and the builder alike.
• The old behaviour remains as -fill-blank-passwords, used only by the builder's second boot: when a draft fails with its defaults and has blank password-like settings, it boots once more with them filled. If that passes, the model is told exactly which settings the server can't start without, and to give them x-generate or a working default.
• Messages and AUTHORING.md no longer say boot tests fill passwords; a server that needs a join password gets x-generate.

Existing templates with blank password settings (boot-tested with fake values until now): ARK ServerPassword, Palworld ServerPassword, RuneScape: Dragonwilds AdminPassword + WorldPassword. After merging, a boot test of all games shows whether they start for real.

🤖 Generated with Claude Code

**Boot tests now run with exactly what a new server gets.** CI's boot test (and the builder's, before #18) gave every blank password-like setting a throwaway value. A real deploy only fills `x-generate` fields, so a template whose server won't start without one passed CI and failed for the first owner who deployed it. - `boottest-payload` fills only `x-generate` fields, for CI and the builder alike. - The old behaviour remains as `-fill-blank-passwords`, used only by the builder's second boot: when a draft fails with its defaults and has blank password-like settings, it boots once more with them filled. If that passes, the model is told exactly which settings the server can't start without, and to give them `x-generate` or a working default. - Messages and `AUTHORING.md` no longer say boot tests fill passwords; a server that needs a join password gets `x-generate`. **Existing templates with blank password settings** (boot-tested with fake values until now): ARK `ServerPassword`, Palworld `ServerPassword`, RuneScape: Dragonwilds `AdminPassword` + `WorldPassword`. After merging, a boot test of all games shows whether they start for real. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

👀 1

greg added 1 commit 2026-10-26 01:57:22 +00:00

feat(builder): pinpoint settings a server can't start without ... 98ba043bec

Booting drafts as deployed catches a server that stops when a setting is
empty, but the server's own output rarely says why, so the model guessed
(more memory) for three rounds and the draft ended as WIP.

When a draft fails with its defaults and has blank password-like settings
without x-generate, the builder boots it once more with those filled. If
that boot passes, the cause is certain, and the model is told exactly that:
these settings stop the server when empty; give them x-generate (a password
per server) or a working default.

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

git_ai_bot reviewed 2026-10-26 01:58:30 +00:00

git_ai_bot left a comment

Member

Copy link

Copy source

🤖 AI Code Review

1. Blocking issues

None found. The logic is correct, the refactoring preserves existing behavior, and the new diagnostic path is sound.

2. Non-blocking suggestions

Grammar in emptySettingsProblem with multiple keys:

When two or more keys are blank, the message reads:

"the server stops when JoinPassword, ServerPassword is empty"

The verb should agree with the subject. A small fix:

verb := "is"
if len(keys) > 1 {
    verb = "are"
}
return fmt.Sprintf("the server stops when %s %s empty, ...", strings.Join(keys, ", "), verb, ...)

This is low-risk since the audience is an LLM, but it makes the message cleaner if a human ever reads it.

blankRequiredLooking silently swallows loadMap errors:

g, err := loadMap(filepath.Join(gamesDir, id, "game.yaml"))
if err != nil {
    return nil
}

If the YAML is malformed (e.g., a draft that the model just wrote with a syntax error), this returns nil and the diagnostic is silently skipped. The build will still fail and report the original boot output, so it's not a correctness bug—but a fmt.Printf (or log) here would make debugging easier if someone wonders why the blank-settings hint never appeared. This is purely a debugging-ergonomics note.

Consider a guard against the second agentBoot being the exitMissingSecret path:

if _, filledPassed, _, err := agentBoot(gamesDir, id, agentImage, false); err == nil && filledPassed {

If the second call hits the exitMissingSecret branch, err is nil and filledPassed is false, so the condition correctly falls through. But the problems slice returned by that call is discarded. If the template only fails because of a missing secret (and the blank-settings check was a red herring), the operator gets the generic bootFailure(output) instead of the more specific "Remove 'secrets' from test.yaml" message. In practice this is unlikely (the first call would have already hit that path), so it's a very minor point—just worth a one-line comment or an early check if you want to be defensive.

3. Tests

The new TestBlankRequiredLookingSettings covers the core filtering well (x-generate excluded, non-empty excluded, non-password key excluded). Two additions worth considering:

• Multi-key case: Assert that with two blank password-like keys, len(got) == 2 and both are sorted. This exercises the sort.Strings path and catches a regression in ordering.
• Empty / no-match case: A template where every password-like key either has x-generate or a non-empty default should return an empty (or nil) slice. This guards against the function accidentally including keys it shouldn't.

The "second boot also fails → fall through to original error" path is hard to unit-test without mocking agentBoot, and the existing integration-style flow covers it. That's acceptable.

4. Overall assessment

Clean, well-scoped change. The extraction of agentBoot is a good refactor that makes the retry-with-filled-settings logic readable in bootCheck rather than buried in a block of inline docker/exec code. The diagnostic message is specific and actionable for the model, and the fallback to the original failure path is correct. The test covers the key filtering logic. I'd merge after the optional multi-key/empty-case test additions; nothing here is a blocker.

---

Automated review by AI Git Bot

## 🤖 AI Code Review ## 1. Blocking issues None found. The logic is correct, the refactoring preserves existing behavior, and the new diagnostic path is sound. ## 2. Non-blocking suggestions **Grammar in `emptySettingsProblem` with multiple keys:** When two or more keys are blank, the message reads: > "the server stops when JoinPassword, ServerPassword is empty" The verb should agree with the subject. A small fix: ```go verb := "is" if len(keys) > 1 { verb = "are" } return fmt.Sprintf("the server stops when %s %s empty, ...", strings.Join(keys, ", "), verb, ...) ``` This is low-risk since the audience is an LLM, but it makes the message cleaner if a human ever reads it. **`blankRequiredLooking` silently swallows `loadMap` errors:** ```go g, err := loadMap(filepath.Join(gamesDir, id, "game.yaml")) if err != nil { return nil } ``` If the YAML is malformed (e.g., a draft that the model just wrote with a syntax error), this returns `nil` and the diagnostic is silently skipped. The build will still fail and report the original boot output, so it's not a correctness bug—but a `fmt.Printf` (or `log`) here would make debugging easier if someone wonders why the blank-settings hint never appeared. This is purely a debugging-ergonomics note. **Consider a guard against the second `agentBoot` being the `exitMissingSecret` path:** ```go if _, filledPassed, _, err := agentBoot(gamesDir, id, agentImage, false); err == nil && filledPassed { ``` If the second call hits the `exitMissingSecret` branch, `err` is `nil` and `filledPassed` is `false`, so the condition correctly falls through. But the `problems` slice returned by that call is discarded. If the template *only* fails because of a missing secret (and the blank-settings check was a red herring), the operator gets the generic `bootFailure(output)` instead of the more specific "Remove 'secrets' from test.yaml" message. In practice this is unlikely (the first call would have already hit that path), so it's a very minor point—just worth a one-line comment or an early check if you want to be defensive. ## 3. Tests The new `TestBlankRequiredLookingSettings` covers the core filtering well (x-generate excluded, non-empty excluded, non-password key excluded). Two additions worth considering: - **Multi-key case:** Assert that with two blank password-like keys, `len(got) == 2` and both are sorted. This exercises the `sort.Strings` path and catches a regression in ordering. - **Empty / no-match case:** A template where every password-like key either has `x-generate` or a non-empty default should return an empty (or nil) slice. This guards against the function accidentally including keys it shouldn't. The "second boot also fails → fall through to original error" path is hard to unit-test without mocking `agentBoot`, and the existing integration-style flow covers it. That's acceptable. ## 4. Overall assessment Clean, well-scoped change. The extraction of `agentBoot` is a good refactor that makes the retry-with-filled-settings logic readable in `bootCheck` rather than buried in a block of inline docker/exec code. The diagnostic message is specific and actionable for the model, and the fallback to the original failure path is correct. The test covers the key filtering logic. I'd merge after the optional multi-key/empty-case test additions; nothing here is a blocker. --- *Automated review by AI Git Bot*

greg added 1 commit 2026-10-26 02:01:18 +00:00

fix(boot-test): run every boot test with exactly what a new server gets ... fe790ad223

CI's boot test gave every blank password-like setting a throwaway value. A
real deploy only fills x-generate fields, so a template whose server won't
start without one passed CI and failed for the first owner who deployed it.

boottest-payload now fills only x-generate fields, for CI and the builder
alike. The old behaviour is kept as -fill-blank-passwords, used only by
the builder's second boot to tell whether an empty setting is why a draft
fails. Messages and AUTHORING.md no longer say boot tests fill passwords;
a server that needs a join password gets x-generate.

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

greg changed title from feat(builder): pinpoint settings a server can't start without to fix(boot-test): exactly what a new server gets, and pinpoint settings it can't start without 2026-10-26 02:01:22 +00:00

git_ai_bot reviewed 2026-10-26 02:02:42 +00:00

git_ai_bot left a comment

Member

Copy link

Copy source

🤖 AI Code Review

1. Blocking issues

Verify scripts/boot-test.sh doesn't pass -as-deployed.

The flag -as-deployed is gone from boottest.go. With flag.ExitOnError, any caller that still passes it will get a hard exit. The internal caller in agentBoot was correctly updated (it now omits the flag since the default is the safe behavior), but I don't have the content of scripts/boot-test.sh. If it invokes boottest-payload -as-deployed …, CI will break on the next run. Please confirm it's either updated or was never using the flag.

2. Non-blocking suggestions

Grammar in emptySettingsProblem with multiple keys (carried over from last review).

fmt.Sprintf("the server stops when %s is empty, …", strings.Join(keys, ", "), …)

With two or more keys this reads "stops when JoinPassword, ServerPassword is empty." A small conditional on len(keys) makes it correct:

verb := "is"
if len(keys) > 1 {
    verb = "are"
}

Low risk (the audience is an LLM), but it costs two lines and removes a small source of confusion if a human reads the log.

blankRequiredLooking silently swallows a loadMap error.

g, err := loadMap(filepath.Join(gamesDir, id, "game.yaml"))
if err != nil {
    return nil
}

If the YAML is malformed (a model just wrote a syntax error), the diagnostic is silently skipped and the generic bootFailure(output) is returned instead. The build still fails correctly, so this isn't a bug—but a one-line fmt.Printf (or log.Printf) here would save someone debugging "why didn't the blank-settings hint fire?" in the future.

CI behavior change is intentional but worth a release note.

Before this PR the default for boottest-payload was "fill every blank password-like field." Now the default is "only fill x-generate fields." If any existing template in games/ has a blank password-like setting without x-generate and the server actually requires it, CI will now fail where it previously passed. That's the desired outcome, but if it catches an existing game on the next pipeline run, it will look like a regression. Worth being ready for it (or adding x-generate to any such template in the same PR).

3. Tests

The existing tests cover the core paths well. Two small additions would round it out:

• Multi-key case in TestBlankRequiredLookingSettings: Add a second blank password-like key (e.g., AdminPassword: "" without x-generate) and assert len(got) == 2 and that both appear in the emptySettingsProblem message. This exercises sort.Strings and the multi-key formatting.
• No-match case: A template where every password-like key either has x-generate or a non-empty default should return an empty/nil slice. Guards against the function accidentally including keys it shouldn't.

The "second boot also fails → fall through" path in bootCheck requires mocking agentBoot (Docker, network) and isn't practical to unit-test here. The integration flow covers it; that's fine.

4. Overall assessment

The refactoring is clean. Extracting agentBoot makes bootCheck readable: first boot → on failure, check for blank settings → second boot with them filled → report the specific cause or fall back. Inverting the flag so the default is the safe behavior (what a new server gets) and -fill-blank-passwords is the opt-in diagnostic is the right direction—fewer flags to remember, and the safe path is the one you get by accident.

The message the model receives is specific and actionable: names the exact keys, says what was tried, and gives the two valid fixes (x-generate or a working default). This directly addresses the three-round guessing loop described in the PR description.

I'd merge after confirming the scripts/boot-test.sh question; everything else is optional polish.

---

Automated review by AI Git Bot

## 🤖 AI Code Review ## 1. Blocking issues **Verify `scripts/boot-test.sh` doesn't pass `-as-deployed`.** The flag `-as-deployed` is gone from `boottest.go`. With `flag.ExitOnError`, any caller that still passes it will get a hard exit. The internal caller in `agentBoot` was correctly updated (it now omits the flag since the default is the safe behavior), but I don't have the content of `scripts/boot-test.sh`. If it invokes `boottest-payload -as-deployed …`, CI will break on the next run. Please confirm it's either updated or was never using the flag. ## 2. Non-blocking suggestions **Grammar in `emptySettingsProblem` with multiple keys (carried over from last review).** ```go fmt.Sprintf("the server stops when %s is empty, …", strings.Join(keys, ", "), …) ``` With two or more keys this reads "stops when JoinPassword, ServerPassword is empty." A small conditional on `len(keys)` makes it correct: ```go verb := "is" if len(keys) > 1 { verb = "are" } ``` Low risk (the audience is an LLM), but it costs two lines and removes a small source of confusion if a human reads the log. **`blankRequiredLooking` silently swallows a `loadMap` error.** ```go g, err := loadMap(filepath.Join(gamesDir, id, "game.yaml")) if err != nil { return nil } ``` If the YAML is malformed (a model just wrote a syntax error), the diagnostic is silently skipped and the generic `bootFailure(output)` is returned instead. The build still fails correctly, so this isn't a bug—but a one-line `fmt.Printf` (or `log.Printf`) here would save someone debugging "why didn't the blank-settings hint fire?" in the future. **CI behavior change is intentional but worth a release note.** Before this PR the default for `boottest-payload` was "fill every blank password-like field." Now the default is "only fill `x-generate` fields." If any existing template in `games/` has a blank password-like setting without `x-generate` *and* the server actually requires it, CI will now fail where it previously passed. That's the desired outcome, but if it catches an existing game on the next pipeline run, it will look like a regression. Worth being ready for it (or adding `x-generate` to any such template in the same PR). ## 3. Tests The existing tests cover the core paths well. Two small additions would round it out: - **Multi-key case in `TestBlankRequiredLookingSettings`:** Add a second blank password-like key (e.g., `AdminPassword: ""` without `x-generate`) and assert `len(got) == 2` and that both appear in the `emptySettingsProblem` message. This exercises `sort.Strings` and the multi-key formatting. - **No-match case:** A template where every password-like key either has `x-generate` or a non-empty default should return an empty/nil slice. Guards against the function accidentally including keys it shouldn't. The "second boot also fails → fall through" path in `bootCheck` requires mocking `agentBoot` (Docker, network) and isn't practical to unit-test here. The integration flow covers it; that's fine. ## 4. Overall assessment The refactoring is clean. Extracting `agentBoot` makes `bootCheck` readable: first boot → on failure, check for blank settings → second boot with them filled → report the specific cause or fall back. Inverting the flag so the default is the safe behavior (what a new server gets) and `-fill-blank-passwords` is the opt-in diagnostic is the right direction—fewer flags to remember, and the safe path is the one you get by accident. The message the model receives is specific and actionable: names the exact keys, says what was tried, and gives the two valid fixes (`x-generate` or a working default). This directly addresses the three-round guessing loop described in the PR description. I'd merge after confirming the `scripts/boot-test.sh` question; everything else is optional polish. --- *Automated review by AI Git Bot*

greg merged commit 274ed68cc5 into main 2026-10-26 02:08:51 +00:00

greg deleted branch feat/builder-pinpoint-empty-settings 2026-10-26 02:08:51 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

git_ai_bot

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

gbr-bot git_ai_bot git_pipeline greg (Gregory Reed) openhands

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gbreed-public/gbr-games#20