ci(kai-ci): make the pin job re-runnable - #93
Conversation
The branch name is derived from the digest, so a re-run of the same build computes the same name. Attempt 1 pushed the branch and then died opening the PR (the App had no pull_requests permission yet); attempt 2 rebuilt an identical commit on a fresh checkout and was rejected non-fast-forward. The job was only ever correct on a clean first run. Same digest means the same edit, so a branch that already exists is already the right branch: reuse it and go on to the PR. Not a force-push — overwriting a branch someone may be reviewing, to replace it with identical content, is all cost and no benefit. The PR step had the mirror of the problem: a re-run whose branch already carries an open PR would fail trying to open a second one. It checks first. This is the same class of bug as the double-charge fixed in kai-server#184: an operation that is not idempotent, re-run after a partial success. Worth naming, because "it worked when nothing went wrong" is how both of them passed review. Both paths verified against the real remote before committing — the existing branch takes the reuse path, an unseen digest takes the push path.
|
Warning Review limit reachedNext included review available in 10 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| .github/workflows/build-kai-ci.yml | Adds branch and PR reuse for reruns, but digest-only reuse can silently ignore a changed pin target. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Build image and obtain digest] --> B[Derive digest-only branch name]
B --> C{Branch already exists?}
C -- No --> D[Apply edits selected by PIN]
D --> E[Commit and push branch]
C -- Yes --> F[Reuse branch without validating requested PIN]
E --> G{Open PR exists?}
F --> G
G -- Yes --> H[Exit successfully]
G -- No --> I[Create pin PR]
Prompt To Fix All With AI
### Issue 1
.github/workflows/build-kai-ci.yml:296-299
**Digest-only reuse drops pin changes**
When a later dispatch produces the same digest with a different `pin` selection, this branch-existence check skips the new edit and then reuses the existing PR, causing requested targets such as `defaultReviewImage` to remain stale.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "ci(kai-ci): make the pin job re-runnable" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if git ls-remote --exit-code --heads origin "$BR" >/dev/null 2>&1; then | ||
| echo "$BR already exists — an earlier attempt pushed it; reusing" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
When a later dispatch produces the same digest with a different pin selection, this branch-existence check skips the new edit and then reuses the existing PR, causing requested targets such as defaultReviewImage to remain stale.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/build-kai-ci.yml
Line: 296-299
Comment:
**Digest-only reuse drops pin changes**
When a later dispatch produces the same digest with a different `pin` selection, this branch-existence check skips the new edit and then reuses the existing PR, causing requested targets such as `defaultReviewImage` to remain stale.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![kaicontext[bot]](https://avatars.githubusercontent.com/in/4190771?s=60&v=4){kind=small}
There was a problem hiding this comment.
Kai review
Kai Summary
Read through this one. 3 things worth your eyes before it merges. 👇
Where I'd land: 4/5 — your call, then merge.
The --jq '.\[0\].number // empty' correctly emits nothing when the list is empty (.\[0\] on \[\] is null, // empty yields empty). That's sound. This is standard, well-formed gh/jq — no need to invoke; the concern here isn't a JSON-field-shape question.
The rest of the read-through
I have enough to write the review. Let me note one remaining edge: the early-exit in the bump step means that if an earlier attempt pushed the branch but the commit content differs (e.g. the target files changed on main between attempts), the reuse path silently ships a stale branch. Let me confirm the branch content is purely digest-derived.
The branch content edits const defaultAgentImage/defaultReviewImage to IMAGE@DIGEST. The branch name is derived from the digest only. So two runs with the same digest but a different base main (e.g. someone merged an unrelated change) would produce the same branch name but the reuse path keeps the old branch built on the old base. That's a real, if narrow, consideration — but for identical content it converges as claimed. Worth naming as a caveat, not a defect, since a stale pin branch would still be reviewed by a human before merge and rebased if needed.
I have what I need.
I reviewed kai-cli at 7822986, the .github/workflows/build-kai-ci.yml change only. What I can't see from here: the actual gh CLI version on the ubuntu-latest runner and its exact pr create --head resolution behavior, and the kaicontext/kai-server remote itself — both matter to the reuse path, and I say where below.
What the change does: it makes the pin job re-runnable after a partial failure. The bump step now short-circuits (exit 0) when the digest-derived branch already exists on the remote instead of trying a non-fast-forward push; the PR step now checks for an already-open PR on that branch and exits 0 instead of trying to open a duplicate. The reasoning is sound and the idempotency framing is right — same digest means same edit means the existing branch is already correct. The gh pr list --jq '.\[0\].number // empty' guard is well-formed: it emits nothing on an empty list, so \[ -n "$EXISTING" \] behaves. This is a clean, well-commented fix and I'd take it.
Two things worth naming.
The dead output. Line 274 adds id: bump and line 284 writes branch=$BR to $GITHUB_OUTPUT, but nothing anywhere reads steps.bump.outputs.branch — I searched the workflows and there are no consumers. The PR step recomputes BR itself at line 343 from the same ${DIGEST:7:12} expression. This is harmless, but it's plumbing that implies a wiring that doesn't exist, and the two independent computations of the branch name are a place a future edit can drift (change the slice in one spot, forget the other). I'd either delete the id/output and keep both recomputing, or make the PR step consume steps.bump.outputs.branch so there's a single source of truth. Not blocking.
The reuse path assumes the existing branch's content is still correct, and that's the assumption I'd want you to confirm holds. When the bump step takes the early exit (line 296–299), it does no local checkout, no bump, no commit — it trusts that the branch already on the remote is the right branch. That's true when the earlier attempt pushed identical content, which is the case you're fixing. But the branch content is base-main plus a digest edit, while the branch name is derived from the digest alone. If main moved between the failed attempt and the re-run, the pre-existing branch carries the old base, and the reuse path ships it unchanged — the branch name can't distinguish "same digest, same base" from "same digest, newer base." In practice a human reviews and can rebase the pin PR, so this doesn't corrupt anything silently, but it's the one way "converge to the same end state as a clean first run" isn't quite literal. Worth a line in the comment so the next person doesn't assume reuse re-derives content.
One thing I could not confirm from here and you should know rests on external behavior: after the bump step early-exits, the local checkout is on the default branch with no local $BR, and the PR step calls gh pr create --head "$BR". For a same-repo PR where $BR exists on the remote, gh resolves the head from the remote, so this should work — but I can't exercise the runner's gh version to prove it, and if that resolution ever needed a local branch, the reuse path would fail at PR creation for exactly the re-run case it's meant to fix. If you've seen this path go green end-to-end on a real re-run, that closes it; if not, it's the one line I'd want a green run to confirm before trusting it.
Merge-wise: this is sound and I'd merge it after you glance at the dead branch output — the fix itself is correct and the two caveats are things to confirm, not defects to repair.
Important files changed
| File | Change |
|---|---|
.github/workflows/build-kai-ci.yml |
modified · +30 −1 |
Decisions
Correct as written, but somebody should say yes to these:
- The PR step's
gh pr create --head "$BR"runs with no local$BRbranch on the reuse path and depends onghresolving the head from the remote — confirm this goes green on a real re-run, since it can't be exercised from the repo.
+30 −1 · 1 file · reaches 0 · the full analysis
💬 Reply to any of my comments and I'll answer, or say @kaicontext anywhere on this PR — a question, or "take another look at the retry logic".
| set -euo pipefail | ||
| NEW="${IMAGE}@${DIGEST}" | ||
| BR="kai-ci/pin-${DIGEST:7:12}" | ||
| echo "branch=$BR" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
id: bump and the branch=$BR output are never read anywhere, and the branch name is recomputed independently in the PR step, so the two can silently drift.
| # | ||
| # Not a force-push. Overwriting a branch someone may have reviewed, to | ||
| # replace it with identical content, is all cost and no benefit. | ||
| if git ls-remote --exit-code --heads origin "$BR" >/dev/null 2>&1; then |
There was a problem hiding this comment.
the reuse early-exit trusts the pre-existing remote branch's content without re-deriving it, so a same-digest re-run after main moved would ship a branch built on the stale base (mitigated by human PR review, not a corruption).
1 participant
The pin job was only ever correct on a clean first run.
The branch name is derived from the digest, so a re-run of the same build computes the same name. In run 34209744781: attempt 1 pushed
kai-ci/pin-cf951c677866and then died opening the PR (the App had nopull_requestspermission yet); attempt 2 rebuilt an identical commit on a fresh checkout and was rejected non-fast-forward.The fix
Same digest means the same edit, so a branch that already exists is already the right branch. Reuse it and go on to the PR.
Deliberately not a force-push: overwriting a branch someone may be reviewing, in order to replace it with identical content, is all cost and no benefit.
The PR step had the mirror of the problem — a re-run whose branch already carries an open PR would fail trying to open a second one — so it checks first.
Worth naming
This is the same class of bug as the double-charge fixed in kaicontext/kai-server#184: an operation that isn't idempotent, re-run after a partial success. There, a redelivered job completion charged the org's Cloud Agent minutes twice. Here, a re-run after a half-finished pin can't proceed at all.
Both passed review because both are correct when nothing goes wrong. That's the tell: any step with a side effect needs an answer for "what happens when this runs twice", and neither had one.
Verified
Both paths checked against the real remote before committing: the existing
kai-ci/pin-cf951c677866takes the reuse path, an unseen digest takes the push path.