feat(worker): one draft per game; a rebuild replaces the earlier one #15

2026-10-25T22:13:30Z

greg commented

2026-10-25 22:13:30 +00:00

A game has one draft at a time. Rerunning a build used to push onto the earlier run's branch, which was rejected once it had review commits; separate branches per run would pile up open PRs instead.

Drafts live on draft/<id>.
Before pushing, the worker closes the game's open draft PRs (draft/<id>, and draft/<id>-<job> as earlier versions named them) and deletes their branches.
Then it force-pushes draft/<id> and opens the new PR, which says which PR it replaces.
Other games' drafts and anyone's non-draft branches are never touched; covered by a test against a fake Gitea.

Note: the next build of a game that has an open draft closes that draft, including any review commits on it.

🤖 Generated with Claude Code

A game has **one draft at a time**. Rerunning a build used to push onto the earlier run's branch, which was rejected once it had review commits; separate branches per run would pile up open PRs instead. - Drafts live on `draft/<id>`. - Before pushing, the worker closes the game's open draft PRs (`draft/<id>`, and `draft/<id>-<job>` as earlier versions named them) and deletes their branches. - Then it force-pushes `draft/<id>` and opens the new PR, which says which PR it replaces. - Other games' drafts and anyone's non-draft branches are never touched; covered by a test against a fake Gitea. Note: the next build of a game that has an open draft closes that draft, including any review commits on it. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

👀 1

git_ai_bot reviewed

2026-10-25 22:13:54 +00:00

git_ai_bot left a comment

🤖 AI Code Review

1. Blocking issues

None. The change is correct, minimal, and well-motivated.

2. Non-blocking suggestions

Same-second collision (extremely unlikely): If two runs of the same job somehow complete within the same second, time.Now() would produce the same branch name and the original bug (push rejected on a branch with prior commits) would resurface. In practice these are multi-minute AI builds so this is virtually impossible, but if you want to be fully bulletproof, appending time.Now().UnixNano()%1000 or a short random hex suffix would make the name unambiguous. Not worth blocking on.
Test coverage of the format string: The test asserts the full expected string for first ("draft/example-4b174b2d-20261025-213000"), which pins the format nicely. You might also want a quick assertion that the rerun string matches the expected value for its time ("draft/example-4b174b2d-20261025-220509"), so a future format-string typo is caught in both positions rather than only in the inequality check. Minor, but cheap to add:
```
if rerun != "draft/example-4b174b2d-20261025-220509" {
    t.Errorf("rerun = %q", rerun)
}
```

3. Tests

The added TestDraftBranchIsNewForEveryRun covers the core behavior (different times → different branches, correct format). That's appropriate for the scope of this change. No additional tests are needed; the branch-name logic is a pure one-liner.

4. Overall assessment

Clean, focused fix for a real operational bug. The extracted draftBranch helper improves testability and readability. The UTC conversion before formatting avoids ambiguity across time zones. Ready to merge.

Automated review by AI Git Bot

## 🤖 AI Code Review ## 1. Blocking issues None. The change is correct, minimal, and well-motivated. ## 2. Non-blocking suggestions - **Same-second collision (extremely unlikely):** If two runs of the same job somehow complete within the same second, `time.Now()` would produce the same branch name and the original bug (push rejected on a branch with prior commits) would resurface. In practice these are multi-minute AI builds so this is virtually impossible, but if you want to be fully bulletproof, appending `time.Now().UnixNano()%1000` or a short random hex suffix would make the name unambiguous. Not worth blocking on. - **Test coverage of the format string:** The test asserts the full expected string for `first` (`"draft/example-4b174b2d-20261025-213000"`), which pins the format nicely. You might also want a quick assertion that the rerun string matches the expected value for its time (`"draft/example-4b174b2d-20261025-220509"`), so a future format-string typo is caught in both positions rather than only in the inequality check. Minor, but cheap to add: ```go if rerun != "draft/example-4b174b2d-20261025-220509" { t.Errorf("rerun = %q", rerun) } ``` ## 3. Tests The added `TestDraftBranchIsNewForEveryRun` covers the core behavior (different times → different branches, correct format). That's appropriate for the scope of this change. No additional tests are needed; the branch-name logic is a pure one-liner. ## 4. Overall assessment Clean, focused fix for a real operational bug. The extracted `draftBranch` helper improves testability and readability. The UTC conversion before formatting avoids ambiguity across time zones. Ready to merge. --- *Automated review by AI Git Bot*

greg added 1 commit 2026-10-25 22:17:30 +00:00

feat(worker): one draft per game; a rebuild replaces the earlier one

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

Details

Validate game templates / boot-test (push) Skipped

Details

Validate game templates / publish (push) Skipped

Details

5d7dee3305

The draft branch was named after the job. Rerunning a build pushed onto the
earlier run's branch, which had review commits, so the push was rejected
and the finished build failed at its last step. Separate branches per run
would pile up open PRs for the same game instead.

A game now has one draft at a time, on draft/<id>. Before pushing, the
worker closes the game's open draft PRs (draft/<id>, and draft/<id>-<job>
as earlier versions named them) and deletes their branches, then
force-pushes draft/<id> and opens the new PR, which says which PR it
replaces. Other games' drafts and anyone's non-draft branches are never
touched.

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

greg force-pushed fix/worker-branch-per-run from fc006a6bcb to 5d7dee3305

2026-10-25 22:17:30 +00:00

Compare

greg changed title from ~~fix(worker): a new branch for every run, so reruns can open their PR~~ to feat(worker): one draft per game; a rebuild replaces the earlier one

2026-10-25 22:17:33 +00:00

openhands merged commit ea2c0b1eeb into main

2026-10-25 22:18:56 +00:00

openhands deleted branch fix/worker-branch-per-run

2026-10-25 22:18:56 +00:00

git_ai_bot reviewed

2026-10-25 22:18:59 +00:00

git_ai_bot left a comment

🤖 AI Code Review

1. Blocking issues

None. The logic is correct for the stated goal: one draft PR per game, with the previous one cleaned up before the new one is opened.

2. Non-blocking suggestions

Pagination on the PR list (limit=50).
replaceOldDrafts fetches open PRs with limit=50 and filters client-side. In a repo with more than 50 open PRs, the relevant draft PR could fall outside the window and be missed, leaving a stale PR open while the new one is created. Since the branch name is deterministic (draft/<gameID>), you can query precisely and avoid pagination entirely:

// Instead of listing all open PRs and filtering:
q := "/repos/" + c.repo + "/pulls?state=open&head=" + url.QueryEscape(c.botLogin+":"+draftBranch(gameID)) + "&limit=1"

This returns 0 or 1 results and sidesteps the limit concern. If you prefer to keep the current shape for simplicity (the repo is small), at minimum bump the limit or add a brief comment acknowledging the assumption.

Regex compiled on every call.
isDraftBranchOf calls regexp.MustCompile each time it's invoked. It's a handful of iterations so the cost is negligible, but hoisting it to a package-level var draftSuffixRe = regexp.MustCompile(...) is a one-line change that also documents intent.

Fragile 404 detection.

err != nil && !strings.HasPrefix(err.Error(), "HTTP 404")

This couples two functions through the error string format. If giteaCall ever changes its message (e.g., adds context), the check silently stops working. A small typed sentinel or a giteaDelete wrapper that swallows 404 internally would be more robust:

func (c *workerConfig) deleteBranch(name string) error {
    err := c.giteaCall(http.MethodDelete, "/repos/"+c.repo+"/branches/"+url.PathEscape(name), nil, nil)
    if err != nil && strings.HasPrefix(err.Error(), "HTTP 404") {
        return nil // already gone
    }
    return err
}

url.PathEscape on the branch name.
This encodes the / in draft/example to %2F. It works because Gitea (and Go's test server) percent-decode the path before routing, and the test confirms it. But it's slightly surprising to read—most Gitea API examples show the branch name with its slash intact. Either approach is fine; just be aware that if you ever add a strings.Contains assertion on the raw request URL (rather than r.URL.Path), the encoded form will trip you up.

3. Tests

The new test is solid: it verifies correct PRs are closed, correct branches are deleted, and other games'/other branches are untouched. The 404-tolerant delete path is also covered.

A few additions that would strengthen coverage:

isDraftBranchOf table test. The logic has three distinct cases (exact match, old-format suffix, non-match). A small table-driven test would make the boundary explicit:

cases := []struct{ ref, gameID string; want bool }{
    {"draft/example", "example", true},
    {"draft/example-4b174b2d", "example", true},
    {"draft/example-xyz", "example", false},   // not 8 hex
    {"draft/example-123456789", "example", false}, // 9 hex
    {"draft/other-4b174b2d", "example", false},
}

replaceOldDrafts with zero matching PRs. The current test always has matches. A quick case with an empty PR list would confirm the "no draft existed" path (branch delete 404 → no error).

4. Overall assessment

Clean, well-motivated fix. The "one draft per game" model is a good simplification over the previous per-run branches, and the migration path (recognising old-format branches via isDraftBranchOf) handles existing state gracefully. The extracted giteaCall helper eliminates the duplicated HTTP boilerplate and centralises auth/error handling. The --force push is correct and well-commented. Ready to merge; the pagination note is the only item I'd consider before the repo grows past a few dozen open PRs.

Automated review by AI Git Bot

## 🤖 AI Code Review ## 1. Blocking issues None. The logic is correct for the stated goal: one draft PR per game, with the previous one cleaned up before the new one is opened. ## 2. Non-blocking suggestions **Pagination on the PR list (`limit=50`).** `replaceOldDrafts` fetches open PRs with `limit=50` and filters client-side. In a repo with more than 50 open PRs, the relevant draft PR could fall outside the window and be missed, leaving a stale PR open while the new one is created. Since the branch name is deterministic (`draft/<gameID>`), you can query precisely and avoid pagination entirely: ```go // Instead of listing all open PRs and filtering: q := "/repos/" + c.repo + "/pulls?state=open&head=" + url.QueryEscape(c.botLogin+":"+draftBranch(gameID)) + "&limit=1" ``` This returns 0 or 1 results and sidesteps the limit concern. If you prefer to keep the current shape for simplicity (the repo is small), at minimum bump the limit or add a brief comment acknowledging the assumption. **Regex compiled on every call.** `isDraftBranchOf` calls `regexp.MustCompile` each time it's invoked. It's a handful of iterations so the cost is negligible, but hoisting it to a package-level `var draftSuffixRe = regexp.MustCompile(...)` is a one-line change that also documents intent. **Fragile 404 detection.** ```go err != nil && !strings.HasPrefix(err.Error(), "HTTP 404") ``` This couples two functions through the error string format. If `giteaCall` ever changes its message (e.g., adds context), the check silently stops working. A small typed sentinel or a `giteaDelete` wrapper that swallows 404 internally would be more robust: ```go func (c *workerConfig) deleteBranch(name string) error { err := c.giteaCall(http.MethodDelete, "/repos/"+c.repo+"/branches/"+url.PathEscape(name), nil, nil) if err != nil && strings.HasPrefix(err.Error(), "HTTP 404") { return nil // already gone } return err } ``` **`url.PathEscape` on the branch name.** This encodes the `/` in `draft/example` to `%2F`. It works because Gitea (and Go's test server) percent-decode the path before routing, and the test confirms it. But it's slightly surprising to read—most Gitea API examples show the branch name with its slash intact. Either approach is fine; just be aware that if you ever add a `strings.Contains` assertion on the raw request URL (rather than `r.URL.Path`), the encoded form will trip you up. ## 3. Tests The new test is solid: it verifies correct PRs are closed, correct branches are deleted, and other games'/other branches are untouched. The 404-tolerant delete path is also covered. A few additions that would strengthen coverage: - **`isDraftBranchOf` table test.** The logic has three distinct cases (exact match, old-format suffix, non-match). A small table-driven test would make the boundary explicit: ```go cases := []struct{ ref, gameID string; want bool }{ {"draft/example", "example", true}, {"draft/example-4b174b2d", "example", true}, {"draft/example-xyz", "example", false}, // not 8 hex {"draft/example-123456789", "example", false}, // 9 hex {"draft/other-4b174b2d", "example", false}, } ``` - **`replaceOldDrafts` with zero matching PRs.** The current test always has matches. A quick case with an empty PR list would confirm the "no draft existed" path (branch delete 404 → no error). ## 4. Overall assessment Clean, well-motivated fix. The "one draft per game" model is a good simplification over the previous per-run branches, and the migration path (recognising old-format branches via `isDraftBranchOf`) handles existing state gracefully. The extracted `giteaCall` helper eliminates the duplicated HTTP boilerplate and centralises auth/error handling. The `--force` push is correct and well-commented. Ready to merge; the pagination note is the only item I'd consider before the repo grows past a few dozen open PRs. --- *Automated review by AI Git Bot*

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gbreed-public/gbr-games#15