kai ship --server: recover from a base commit the remote never saw - #104
Conversation
The ship names the commit the session was spawned from and the server fetches it from GitHub to build the branch on. That commit is the checkout's HEAD at spawn time, and a checkout's HEAD is often a commit GitHub has never seen — committed locally and not pushed, or the original of a squash-merged PR. The server failed with "not our ref" and the message told the person to re-ship from a fresh session base. That was the harness handing its own problem to the user. shipBaseFor resolves it before the request: if the spawn base is on the remote, it is the base; if not (after one fetch), the base moves to the merge-base with the remote's default branch, and every file's delta is re-derived against it — the base's content plus the agent's hunks — so the PR carries only the agent's change and not the unpushed commits it was working on top of. A hunk that cannot be separated ships the whole file and says so, as before. If the server still refuses the base (a remote the checkout does not track, a force-push underneath), the ship moves the base and goes again, once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
jschatz1 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
![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. 5 things worth your eyes before it merges. 👇
Review: kaicontext/kai-cli @ 9b7c066 — kai ship --server: recover from a base commit the remote never saw
The rest of the read-through
Scope: I read the full diff (cmd/kai/ship_base.go, cmd/kai/ship_base_test.go, cmd/kai/ship_server.go) and traced shipBaseFor, shipContentAgainst, and the buildFiles retry through runShipServer. The spawnpkg.Entry struct (RemoteName, BaseGitSHA, SourceRepo) lives in kai-engine/spawn, a sibling repo I cannot read here — I'm taking those fields on the word of how this repo's spawn.go already populates and reads them (e.g., spawn.go:279 sets ent.RemoteName, spawn.go:497-505 reads e.RemoteName).
What this change does: before queueing a server-ship, shipBaseFor checks whether the spawn base commit is reachable on a remote-tracking ref (fetching once if stale); if not, it moves the base to the merge-base with the remote's default branch and re-derives each shipped file against that commit so the PR carries the agent's hunks and not the unpushed local commits beneath. If the server later refuses the base anyway, the polling loop retries once with a freshly moved base. Overall this is a sound, well-scoped fix to a real "the harness handed its problem to the user" bug, and the delta re-derivation in shipContentAgainst is correctly factored out of shipContentFor.
Concerns
-
cmd/kai/ship_base_test.go:84— the "reason names unpushed" assertion gates itself on the very thing it's meant to verify, so it can pass without checking. The line isif !strings.Contains(shipBaseFor(spawn, e).Reason, "unpushed") && b.Moved. It callsshipBaseFora second time (a fresh result whoseReasonit inspects), but then ANDs that withb.Movedfrom the first call, and on failure reportsb.Reasonfrom the first call. If the first call did not move (b.Moved == false), the whole condition is false and thet.Errorfnever fires — regardless of what eitherReasonactually says. The test's own earlier assertion (if !b.Moved && b.SHA == pushed) already treatsb.Movedas the uncertain quantity; tying the reason check to it means the reason is only ever checked in the branch where the test already considers the move to have happened. To actually verify the reason, assert directly onb—if b.Moved && !strings.Contains(b.Reason, "unpushed") { t.Errorf(...) }— not on a re-invocation. -
cmd/kai/ship_base.go:35—shipBaseForrunsgit fetchagainste.SourceRepo(the user's checkout), mutating its remote-tracking state from insidekai ship.remoteHasis a read-onlygit branch -r --contains, but the fallbackgitOut(repo, "fetch", "-q", "--no-tags", remoteName(e))is a network write to the user's working repository that nobody askedkai shipto perform.repohere ise.SourceRepo, the person's source checkout — not the isolated spawn. Akai ship --servernow silently advances that repo'sorigin/*tracking refs (and pulls ref advertisement over the network) as a side effect of resolving the base. If the checkout is on a slow or restricted network, or the person deliberately keeps stale tracking refs, this is surprising. At minimum the note machinery should say it fetched to check the base; better, the fetch should be limited to the move-actually-needed case. This runs on the path of every server-ship, not just the unfetchable one. -
cmd/kai/ship_server.go:253— the belt path callsshipBaseForagain, which fetches a second time against the same checkout. When the server rejects a base that the proactive check passed (!base.Moved), the retry re-invokesshipBaseFor(cwd, entry), which will runremoteHasand, on a miss, anothergit fetch. Combined with the proactive call, that's two network round-trips into the user's repo for one ship, and the second fetch is redundant with the first if nothing changed in the interval. The retry would do better to reuse the already-resolved state rather than re-running the whole resolution from scratch. -
cmd/kai/ship_server.go:252— the!base.Movedguard means the retry only covers the narrow case where the proactive check thought the base was fetchable but the server disagreed. That's a real window (local tracking refs can be ahead of GitHub), so the belt is not useless. But the comment's framing ("a remote the checkout does not track, a force-push under the person's feet") overstates it: a force-push on the remote would make the proactiveremoteHasreturn false (the fetch updates tracking refs), so the proactive path would move and setMoved=true, and the!base.Movedguard would then suppress the retry the comment describes. The belt as written does not reach the force-push case it names. Not a defect — the proactive path handles force-push by moving — but the comment should describe what the guard actually lets through (local-tracking-says-yes, GitHub-says-no), not the case it doesn't. -
cmd/kai/ship_base.go:65—fmt.Sscanf(out, "%d", &n)ignores its error. Ifrev-listfails,nstays0and the user-facing reason reads "0 unpushed local commit(s)" while the base was in fact moved off real unpushed work. The reason is misleading in exactly the case the change exists to explain. Check the error, or drop the count from the reason when it can't be determined.
The refactor of shipContentFor → shipContentAgainst is clean: the only remaining caller of shipContentFor is the delta test, and buildFiles correctly calls shipContentAgainst with the (possibly moved) baseSHA. The retry correctly re-derives payload.Files against the moved base before re-queueing, and resets the poll deadline. The review_analyze.go baseSHA reference in the dependency data is a help-text example string, not a code dependency. No secret/token comparisons are touched.
Merge readiness: the core logic is correct and the factoring is good; the test's self-defeating assertion and the unannounced git fetch into the user's checkout are the two things I'd want addressed before this merges, but neither is a defect in the shipping path itself. Small fixes first.
Important files changed
| File | Change |
|---|---|
cmd/kai/ship_base.go |
modified · +113 −0 |
cmd/kai/ship_base_test.go |
modified · +90 −0 |
cmd/kai/ship_server.go |
modified · +78 −30 |
What I opened — 5 files, 20 turns, 1m30s
1 of the 3 changed files doesn't appear below: cmd/kai/ship_base.go.
cmd/kai/review_analyze.gocmd/kai/ship_base_test.gocmd/kai/ship_delta_test.gocmd/kai/ship_server.gocmd/kai/spawn.go
+281 −30 · 3 files · reaches 4 · 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".
| if want := "line1\nline2\nline3\nline4 AGENT\n"; string(got) != want || len(overlaps) != 0 { | ||
| t.Fatalf("content against the moved base = %q (overlaps %v)", got, overlaps) | ||
| } | ||
| if !strings.Contains(shipBaseFor(spawn, e).Reason, "unpushed") && b.Moved { |
There was a problem hiding this comment.
the "reason names unpushed" assertion ANDs its check on b.Moved from the first call while inspecting a fresh second call's Reason, so it silently skips when b.Moved is false and never verifies the reason in the uncertain branch.
| Spawn string // the spawn base (checkout HEAD at spawn), for the delta | ||
| Moved bool // SHA != Spawn: the spawn base was not on the remote | ||
| Unpushed int // local commits between the two, when moved | ||
| Reason string // one line for the person |
There was a problem hiding this comment.
shipBaseFor runs git fetch against e.SourceRepo (the user's working checkout), mutating its remote-tracking refs as an unannounced side effect of kai ship on every server-ship.
| // The server could not fetch the base after all. Move to the | ||
| // nearest commit it has and go again, once (ship_base.go). | ||
| if shipBaseUnfetchable(st.Error) && !base.Moved { | ||
| moved := shipBaseFor(cwd, entry) |
There was a problem hiding this comment.
the belt retry re-invokes shipBaseFor, fetching a second time into the same checkout; reuse the already-resolved state instead.
| case "failed": | ||
| // The server could not fetch the base after all. Move to the | ||
| // nearest commit it has and go again, once (ship_base.go). | ||
| if shipBaseUnfetchable(st.Error) && !base.Moved { |
There was a problem hiding this comment.
the !base.Moved guard means the retry does not cover the force-push case the comment names (the proactive path would already move and set Moved=true); the comment should describe the narrower window it actually lets through.
| } | ||
| n := 0 | ||
| if out, err := gitOut(repo, "rev-list", "--count", mb+".."+sha); err == nil { | ||
| fmt.Sscanf(out, "%d", &n) |
There was a problem hiding this comment.
fmt.Sscanf ignores its error, so a rev-list failure produces a misleading "0 unpushed local commit(s)" reason in exactly the case the change exists to explain.
1 participant
Summary
The base a ship names is the checkout's HEAD at spawn time, and that is often a commit GitHub has never seen (unpushed, or the original of a squash-merged PR). The server failed with "not our ref" and told the person to re-ship from a fresh session. Now:
Test plan
TestShipBaseFor_MovesOffAnUnpushedCommit: a bare remote, a pushed commit, a local-only commit on top, the spawn's agent hunk re-derived against the pushed commitTestShipContentFor_StripsCheckoutEdits;go test ./cmd/kai; execlint clean🤖 Generated with Claude Code