feat(builder): diagnose a failed boot before changing the template #23

2026-10-26T03:15:38Z

greg commented

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

A failed boot went straight back to the model as "fix it", so it guessed. One draft raised its memory three rounds in a row, while the real cause, an empty setting the server can't run without, stayed untouched.

Now a failed boot is investigated first:

Evidence: the server's distinct error lines (timestamps and repeats removed), plus a web search on the most telling one, with the top answer read through the reader.
Diagnosis: the model states the cause, the evidence for it, its confidence and the exact fix. If unsure, it asks for up to 3 checks: a search, a page, the files the server writes (a first boot of the draft), or a boot test with settings or the startup command changed. Results go back to it, two rounds at most.
Fix: the model applies exactly the diagnosed fix, and nothing unrelated.
Attempt log: each cause, fix and outcome. A fix that changed nothing rules its cause out. If the diagnosis returns to a ruled-out cause, the build stops instead of guessing, and the PR's What the builder found section lists every attempt.
Boot rounds go up to 5, since rounds now end on a ruled-out cause rather than a fixed count.

Tested: unit tests cover the diagnosis loop with a fake model and a fake search server, the attempt log and ruled-out stop, error-line extraction, and the comparison-boot template copy. Not yet run end to end on a real build.

🤖 Generated with Claude Code

A failed boot went straight back to the model as "fix it", so it guessed. One draft raised its memory three rounds in a row, while the real cause, an empty setting the server can't run without, stayed untouched. **Now a failed boot is investigated first:** - **Evidence:** the server's distinct error lines (timestamps and repeats removed), plus a web search on the most telling one, with the top answer read through the reader. - **Diagnosis:** the model states the cause, the evidence for it, its confidence and the exact fix. If unsure, it asks for up to 3 checks: a search, a page, the files the server writes (a first boot of the draft), or a boot test with settings or the startup command changed. Results go back to it, two rounds at most. - **Fix:** the model applies exactly the diagnosed fix, and nothing unrelated. - **Attempt log:** each cause, fix and outcome. A fix that changed nothing rules its cause out. If the diagnosis returns to a ruled-out cause, the build stops instead of guessing, and the PR's **What the builder found** section lists every attempt. - Boot rounds go up to 5, since rounds now end on a ruled-out cause rather than a fixed count. **Tested:** unit tests cover the diagnosis loop with a fake model and a fake search server, the attempt log and ruled-out stop, error-line extraction, and the comparison-boot template copy. Not yet run end to end on a real build. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

👀 1

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

feat(builder): diagnose a failed boot before changing the template

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

Details

Validate game templates / boot-test (push) Skipped

Details

Validate game templates / publish (push) Skipped

Details

487dfbdf68

A failed boot went straight back to the model as "fix it", so it guessed:
one draft raised its memory three rounds in a row while the real cause, an
empty setting the server can't run without, stayed untouched.

Now a failed boot is investigated first:
- Evidence: the server's distinct error lines (timestamps and repeats
  removed), and a web search on the most telling one, with the top answer
  read through the reader.
- Diagnosis: the model states the cause, the evidence for it, its
  confidence and the exact fix. If unsure, it asks for up to 3 checks: a
  search, a page, the files the server writes (a first boot of the draft),
  or a boot test with settings or the startup command changed. Results go
  back to it, two rounds at most.
- Fix: the model applies exactly the diagnosed fix and nothing unrelated.
- Attempt log: each cause, fix and outcome. A fix that changed nothing
  rules its cause out; if the diagnosis returns to a ruled-out cause, the
  build stops instead of guessing, and the PR lists what the builder found.

Boot rounds go up to 5, since rounds now end on a ruled-out cause rather
than a fixed count.

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

git_ai_bot reviewed

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

git_ai_bot left a comment

🤖 AI Code Review

1. Blocking issues

None found. The logic is coherent and the guard rails (max 3 diagnosis rounds, max 2 boots per round, ruled-out stop, max boot rounds) are all bounded.

2. Non-blocking suggestions

a. read check doesn't guard against an unset READER_URL

In runChecks, the "read" case calls readPageLimit(iv.reader, c.URL, 8000) without first checking whether iv.reader is empty. The "search" case in the same function (and search itself) guards with if iv.searx == "". If READER_URL is unset, the model will get a confusing error message instead of a clean "not configured" note. Suggest adding:

case "read":
    if iv.reader == "" {
        b.WriteString("read: not configured\n")
        continue
    }
    // ...

b. bootWith only copies game.yaml and test.yaml

copyTemplateWith writes the (modified) game.yaml and, if present, test.yaml. If a template directory ever contains additional files the boot test needs (helper scripts, extra config files, a Dockerfile override), the boot_with check will fail for reasons unrelated to the setting being tested, and the model will receive a misleading "boot failed" signal. Consider copying the whole directory and then overwriting the modified fields, or at minimum logging which files were not copied.

c. ruledOut treats "a different failure" as ruled-out

The prompt and log text tell the model only an unchanged outcome rules a cause out, but ruledOut rejects any entry whose outcome doesn't start with "fixed":

if causeKey(a.Cause) == key && !strings.HasPrefix(a.Outcome, "fixed") {
    return true
}

So "a different failure: port 30001 in use" also rules out the cause "port 30000 in use". This is arguably correct (the fix for port-30000 was applied and the new failure is a different issue, so going back to port-30000 would be a loop), but it's stricter than the prompt promises. If you want to allow the model to revisit a cause whose fix produced a different error (perhaps the fix was incomplete), narrow the check to:

if causeKey(a.Cause) == key && strings.HasPrefix(a.Outcome, "unchanged") {
    return true
}

If the current behaviour is intentional, tighten the log text to match:

"A fix whose outcome is anything other than 'fixed' rules its cause out."

d. Silent errors on file reads

Both AUTHORING.md and the template game.yaml are read with _ for the error. A missing AUTHORING.md is recoverable (model just gets no house rules), but a missing game.yaml means the model is diagnosing with an empty template, which will almost certainly produce a wrong cause. A one-line fmt.Fprintf(os.Stderr, ...) warning would save debugging time.

e. External HTTP calls have no visible timeout in this file

searxSearch, readPageLimit, and the model's chat all make network calls. If they don't set a client-level timeout (likely they do, since they're defined elsewhere), a hung search server would block the build indefinitely. Worth verifying that a timeout is applied.

3. Tests

Coverage is good for the core unit logic. A few gaps worth filling when it's convenient:

Missing case	Why it matters
`outcome` → "a different failure" path + subsequent `ruledOut`	Confirms whether the build stops or continues when the symptom changes (see 2c).
`diagnose` fallback when `m.chat` returns an error	Exercises the `"unknown"` cause path and verifies `ruledOut` doesn't immediately stop on the second occurrence.
`ruledOut` returns false when the log is empty	Trivial guard, but prevents a future regression if someone reorders the calls.
`runChecks` with all three checks being `"files"`/`"boot_with"`	Verifies the "at most two boots" skip path.
`buildWithImage` loop hitting the `ruledOut` break (integration-level, could be a table-test with a scripted `fakeModel` + stubbed `bootCheck`)	The PR description notes this hasn't been exercised end-to-end; a lightweight integration test would catch wiring mistakes (e.g., `iv.outcome` called at the wrong point).

The existing tests are well-structured and the fakeModel pattern is clean.

4. Overall assessment

Solid, well-scoped improvement. The diagnosis-before-fix loop, the attempt log with ruled-out stopping, and the structured check system all address the real failure mode described in the PR (model guessing memory as the cause while an empty config was the actual issue). The code is readable, the abstractions are appropriate for the codebase, and the tests cover the core logic. The non-blocking items above are small polish items; I'd want 2a (the unset READER_URL guard) fixed before merge since it's a two-line change that prevents a confusing runtime error, but the rest can follow up.

Automated review by AI Git Bot

## 🤖 AI Code Review ## 1. Blocking issues None found. The logic is coherent and the guard rails (max 3 diagnosis rounds, max 2 boots per round, ruled-out stop, max boot rounds) are all bounded. ## 2. Non-blocking suggestions **a. `read` check doesn't guard against an unset `READER_URL`** In `runChecks`, the `"read"` case calls `readPageLimit(iv.reader, c.URL, 8000)` without first checking whether `iv.reader` is empty. The `"search"` case in the same function (and `search` itself) guards with `if iv.searx == ""`. If `READER_URL` is unset, the model will get a confusing error message instead of a clean "not configured" note. Suggest adding: ```go case "read": if iv.reader == "" { b.WriteString("read: not configured\n") continue } // ... ``` **b. `bootWith` only copies `game.yaml` and `test.yaml`** `copyTemplateWith` writes the (modified) `game.yaml` and, if present, `test.yaml`. If a template directory ever contains additional files the boot test needs (helper scripts, extra config files, a `Dockerfile` override), the `boot_with` check will fail for reasons unrelated to the setting being tested, and the model will receive a misleading "boot failed" signal. Consider copying the whole directory and then overwriting the modified fields, or at minimum logging which files were *not* copied. **c. `ruledOut` treats "a different failure" as ruled-out** The prompt and log text tell the model only an *unchanged* outcome rules a cause out, but `ruledOut` rejects any entry whose outcome doesn't start with `"fixed"`: ```go if causeKey(a.Cause) == key && !strings.HasPrefix(a.Outcome, "fixed") { return true } ``` So "a different failure: port 30001 in use" also rules out the cause "port 30000 in use". This is arguably correct (the fix for port-30000 was applied and the *new* failure is a different issue, so going back to port-30000 would be a loop), but it's stricter than the prompt promises. If you want to allow the model to revisit a cause whose fix produced a *different* error (perhaps the fix was incomplete), narrow the check to: ```go if causeKey(a.Cause) == key && strings.HasPrefix(a.Outcome, "unchanged") { return true } ``` If the current behaviour is intentional, tighten the log text to match: > "A fix whose outcome is anything other than 'fixed' rules its cause out." **d. Silent errors on file reads** Both `AUTHORING.md` and the template `game.yaml` are read with `_` for the error. A missing `AUTHORING.md` is recoverable (model just gets no house rules), but a missing `game.yaml` means the model is diagnosing with an empty template, which will almost certainly produce a wrong cause. A one-line `fmt.Fprintf(os.Stderr, ...)` warning would save debugging time. **e. External HTTP calls have no visible timeout in this file** `searxSearch`, `readPageLimit`, and the model's `chat` all make network calls. If they don't set a client-level timeout (likely they do, since they're defined elsewhere), a hung search server would block the build indefinitely. Worth verifying that a timeout is applied. ## 3. Tests Coverage is good for the core unit logic. A few gaps worth filling when it's convenient: | Missing case | Why it matters | |---|---| | `outcome` → "a different failure" path + subsequent `ruledOut` | Confirms whether the build stops or continues when the symptom changes (see 2c). | | `diagnose` fallback when `m.chat` returns an error | Exercises the `"unknown"` cause path and verifies `ruledOut` doesn't immediately stop on the second occurrence. | | `ruledOut` returns false when the log is empty | Trivial guard, but prevents a future regression if someone reorders the calls. | | `runChecks` with all three checks being `"files"`/`"boot_with"` | Verifies the "at most two boots" skip path. | | `buildWithImage` loop hitting the `ruledOut` break (integration-level, could be a table-test with a scripted `fakeModel` + stubbed `bootCheck`) | The PR description notes this hasn't been exercised end-to-end; a lightweight integration test would catch wiring mistakes (e.g., `iv.outcome` called at the wrong point). | The existing tests are well-structured and the `fakeModel` pattern is clean. ## 4. Overall assessment Solid, well-scoped improvement. The diagnosis-before-fix loop, the attempt log with ruled-out stopping, and the structured check system all address the real failure mode described in the PR (model guessing memory as the cause while an empty config was the actual issue). The code is readable, the abstractions are appropriate for the codebase, and the tests cover the core logic. The non-blocking items above are small polish items; I'd want 2a (the unset `READER_URL` guard) fixed before merge since it's a two-line change that prevents a confusing runtime error, but the rest can follow up. --- *Automated review by AI Git Bot*

greg merged commit e07025f03d into main

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

greg deleted branch feat/builder-diagnose

2026-10-26 03:21:37 +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#23