This commit is contained in:
@@ -42,6 +42,7 @@ var scenarioRegistry = map[string]func(e *emitter){
|
||||
"symlink-file-setup": scenarioSymlinkFileSetup,
|
||||
"markdown-table": scenarioMarkdownTable,
|
||||
"empty-turn": scenarioEmptyTurn,
|
||||
"push-current-branch": scenarioPushCurrentBranch,
|
||||
}
|
||||
|
||||
// scenarioEmptyTurn emits no content and no tool calls, so the turn ends
|
||||
@@ -452,6 +453,66 @@ func scenarioDiffUpdateModify(e *emitter) {
|
||||
e.text("diff-update-modify complete: diff_update_test.txt now has SECOND_MODIFICATION")
|
||||
}
|
||||
|
||||
// scenarioPushCurrentBranch pushes the worktree's current branch to origin
|
||||
// directly, with no Kandev "Create MR"/"Create PR" action involved. Used by
|
||||
// push-detection auto-link e2e tests to reproduce an agent running a raw
|
||||
// `git push` on its own — the same shape as a real user/agent pushing and
|
||||
// then opening the MR/PR through `glab mr create`, `gh pr create`, or the
|
||||
// code host's web UI, rather than through Kandev's own runtime action.
|
||||
// Expects a prior turn (e.g. diff-update-setup) to have already committed
|
||||
// something to push.
|
||||
//
|
||||
// Explicitly writes the remote-tracking ref and upstream config after
|
||||
// pushing rather than relying on `git push -u` alone: the e2e fixture's mock
|
||||
// git remote only implements the receive-pack (push) side of the smart-HTTP
|
||||
// protocol, not upload-pack/info-refs (fetch/clone) — confirmed by
|
||||
// worktree-manager's own "git fetch failed before worktree creation;
|
||||
// continuing with fallback ref" warning during task setup, and reproducible
|
||||
// directly (`git fetch` against it fails with "not valid: is this a git
|
||||
// repository?"). A real code host's push response updates
|
||||
// refs/remotes/origin/<branch> as a side effect of the exchange; here that
|
||||
// has to be reproduced by hand with `update-ref`, using the local HEAD this
|
||||
// process just successfully pushed as the known-good value — no fetch
|
||||
// required, since the pushed content is exactly what's already local.
|
||||
func scenarioPushCurrentBranch(e *emitter) {
|
||||
fixedDelay(50)
|
||||
|
||||
wd, err := os.Getwd()
|
||||
if err != nil {
|
||||
e.text("push-current-branch: getwd failed: " + err.Error())
|
||||
return
|
||||
}
|
||||
branchNameOut, err := exec.Command("git", "-C", wd, "rev-parse", "--abbrev-ref", "HEAD").Output()
|
||||
if err != nil {
|
||||
e.text("push-current-branch: resolve branch name failed: " + err.Error())
|
||||
return
|
||||
}
|
||||
branchName := strings.TrimSpace(string(branchNameOut))
|
||||
headSHAOut, err := exec.Command("git", "-C", wd, "rev-parse", "HEAD").Output()
|
||||
if err != nil {
|
||||
e.text("push-current-branch: resolve HEAD sha failed: " + err.Error())
|
||||
return
|
||||
}
|
||||
headSHA := strings.TrimSpace(string(headSHAOut))
|
||||
|
||||
runGitCmd := makeGitRunner(wd)
|
||||
if err := runGitCmd("push", "origin", branchName); err != nil {
|
||||
e.text("push-current-branch: git push failed")
|
||||
return
|
||||
}
|
||||
if err := runGitCmd("update-ref", "refs/remotes/origin/"+branchName, headSHA); err != nil {
|
||||
e.text("push-current-branch: git update-ref failed")
|
||||
return
|
||||
}
|
||||
if err := runGitCmd("branch", "--set-upstream-to=origin/"+branchName, branchName); err != nil {
|
||||
e.text("push-current-branch: git branch --set-upstream-to failed")
|
||||
return
|
||||
}
|
||||
|
||||
fixedDelay(100)
|
||||
e.text("push-current-branch complete: pushed directly, no Create MR/PR action used")
|
||||
}
|
||||
|
||||
// scenarioDiffUpdateStreaming modifies the file mid-turn, emitting text both
|
||||
// before and after the write so the agent's turn stays active for several
|
||||
// seconds. Used to assert that open file editor / diff panels auto-update
|
||||
|
||||
@@ -264,17 +264,23 @@ func (p GitEventPayload) GetSessionID() string {
|
||||
}
|
||||
|
||||
type GitStatusData struct {
|
||||
Branch string `json:"branch"`
|
||||
RemoteBranch string `json:"remote_branch,omitempty"`
|
||||
HeadCommit string `json:"head_commit,omitempty"`
|
||||
BaseCommit string `json:"base_commit,omitempty"`
|
||||
Modified []string `json:"modified"`
|
||||
Added []string `json:"added"`
|
||||
Deleted []string `json:"deleted"`
|
||||
Untracked []string `json:"untracked"`
|
||||
Renamed []string `json:"renamed"`
|
||||
Ahead int `json:"ahead"`
|
||||
Behind int `json:"behind"`
|
||||
Branch string `json:"branch"`
|
||||
RemoteBranch string `json:"remote_branch,omitempty"`
|
||||
HeadCommit string `json:"head_commit,omitempty"`
|
||||
BaseCommit string `json:"base_commit,omitempty"`
|
||||
Modified []string `json:"modified"`
|
||||
Added []string `json:"added"`
|
||||
Deleted []string `json:"deleted"`
|
||||
Untracked []string `json:"untracked"`
|
||||
Renamed []string `json:"renamed"`
|
||||
Ahead int `json:"ahead"`
|
||||
Behind int `json:"behind"`
|
||||
// RemoteAhead/RemoteBehind compare against this branch's own upstream
|
||||
// (@{upstream}), unlike Ahead/Behind which are relative to the base
|
||||
// branch and never reach zero just because the branch was pushed. Push
|
||||
// detection (event_handlers_git.go) reads RemoteAhead, not Ahead.
|
||||
RemoteAhead int `json:"remote_ahead"`
|
||||
RemoteBehind int `json:"remote_behind"`
|
||||
Files interface{} `json:"files,omitempty"`
|
||||
BranchAdditions int `json:"branch_additions,omitempty"`
|
||||
BranchDeletions int `json:"branch_deletions,omitempty"`
|
||||
|
||||
@@ -296,6 +296,8 @@ func (p *EventPublisher) PublishGitStatus(execution *AgentExecution, update *age
|
||||
Renamed: update.Renamed,
|
||||
Ahead: update.Ahead,
|
||||
Behind: update.Behind,
|
||||
RemoteAhead: update.RemoteAhead,
|
||||
RemoteBehind: update.RemoteBehind,
|
||||
Files: update.Files,
|
||||
BranchAdditions: update.BranchAdditions,
|
||||
BranchDeletions: update.BranchDeletions,
|
||||
|
||||
@@ -36,13 +36,15 @@ func (wt *WorkspaceTracker) pollGitChanges(ctx context.Context) {
|
||||
}
|
||||
defer timer.Stop()
|
||||
|
||||
// Prime cached HEAD SHA, branch name, and index hash. A read failure here
|
||||
// is non-fatal: the caches stay empty and the first successful tick fills
|
||||
// them, which is what the previous per-value getters did on error too.
|
||||
// Prime cached HEAD SHA, branch name, upstream SHA, and index hash. A read
|
||||
// failure here is non-fatal: the caches stay empty and the first
|
||||
// successful tick fills them, which is what the previous per-value
|
||||
// getters did on error too.
|
||||
snap, _ := wt.readGitPollSnapshot(ctx)
|
||||
wt.gitStateMu.Lock()
|
||||
wt.cachedHeadSHA = snap.headSHA
|
||||
wt.cachedBranchName = snap.branch
|
||||
wt.cachedUpstreamSHA = snap.upstreamSHA
|
||||
wt.cachedIndexHash = snap.indexHash
|
||||
wt.gitStateMu.Unlock()
|
||||
|
||||
@@ -163,12 +165,14 @@ func (wt *WorkspaceTracker) handleGitPollFailure(ctx context.Context, consecutiv
|
||||
return false
|
||||
}
|
||||
|
||||
// gitPollSnapshot is one tick's worth of git state: HEAD, branch, and a hash
|
||||
// standing in for the index contents.
|
||||
// gitPollSnapshot is one tick's worth of git state: HEAD, branch, upstream
|
||||
// branch name, and a hash standing in for the index contents.
|
||||
type gitPollSnapshot struct {
|
||||
headSHA string
|
||||
branch string
|
||||
indexHash string
|
||||
headSHA string
|
||||
branch string
|
||||
upstream string
|
||||
upstreamSHA string
|
||||
indexHash string
|
||||
}
|
||||
|
||||
// Porcelain v2 header prefixes, and the sentinel values git prints when there
|
||||
@@ -176,11 +180,12 @@ type gitPollSnapshot struct {
|
||||
// sentinels map to the empty string, matching what the rev-parse and
|
||||
// symbolic-ref probes this read replaces returned in those same states.
|
||||
const (
|
||||
gitStatusHeaderPrefix = "# "
|
||||
gitStatusBranchOIDPrefix = "# branch.oid "
|
||||
gitStatusBranchHeadPrefix = "# branch.head "
|
||||
gitStatusInitialOID = "(initial)"
|
||||
gitStatusDetachedHead = "(detached)"
|
||||
gitStatusHeaderPrefix = "# "
|
||||
gitStatusBranchOIDPrefix = "# branch.oid "
|
||||
gitStatusBranchHeadPrefix = "# branch.head "
|
||||
gitStatusBranchUpstreamPrefix = "# branch.upstream "
|
||||
gitStatusInitialOID = "(initial)"
|
||||
gitStatusDetachedHead = "(detached)"
|
||||
)
|
||||
|
||||
// readGitPollSnapshot reads HEAD, branch name and index state in a single git
|
||||
@@ -210,12 +215,43 @@ const (
|
||||
// - --untracked-files=no, preserved from the old status call: untracked files
|
||||
// are already monitored by monitorLoop via git ls-files, and =all would pay
|
||||
// for a full directory traversal on every tick.
|
||||
//
|
||||
// branch.upstream carries the upstream's *name*, which the status call reports
|
||||
// for free, but not its SHA — --no-ahead-behind only suppresses branch.ab, the
|
||||
// walk that counts commits between the two, not the upstream header itself.
|
||||
// Detecting a push needs the SHA (the name is stable across a push; only what
|
||||
// it points at moves), so when an upstream is configured this issues one more
|
||||
// `git rev-parse` for it. That's a plain ref resolution, not a graph walk — the
|
||||
// same class of cheap call as the isAncestor/merge-base spawns already on this
|
||||
// path — so it doesn't reintroduce the walk cost --no-ahead-behind avoids.
|
||||
func (wt *WorkspaceTracker) readGitPollSnapshot(ctx context.Context) (gitPollSnapshot, error) {
|
||||
out, err := wt.runPollingGitOutput(ctx, "status", "--porcelain=v2", "--branch", "--no-ahead-behind", "--untracked-files=no")
|
||||
if err != nil {
|
||||
return gitPollSnapshot{}, err
|
||||
}
|
||||
return parseGitPollSnapshot(out), nil
|
||||
snap := parseGitPollSnapshot(out)
|
||||
if snap.upstream != "" {
|
||||
sha, err := wt.getUpstreamSHA(ctx)
|
||||
if err != nil {
|
||||
// Propagate rather than coerce to "", which would be
|
||||
// indistinguishable from "no upstream" and could mask a real
|
||||
// transition. Returning the error routes through the same
|
||||
// gitPollTick failure/retry path as the status read above, so
|
||||
// the next tick just tries the snapshot again.
|
||||
return gitPollSnapshot{}, err
|
||||
}
|
||||
snap.upstreamSHA = sha
|
||||
}
|
||||
return snap, nil
|
||||
}
|
||||
|
||||
// getUpstreamSHA resolves the current branch's upstream ref to a SHA.
|
||||
func (wt *WorkspaceTracker) getUpstreamSHA(ctx context.Context) (string, error) {
|
||||
out, err := wt.runPollingGitOutput(ctx, "rev-parse", "@{upstream}")
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
return strings.TrimSpace(string(out)), nil
|
||||
}
|
||||
|
||||
// parseGitPollSnapshot splits porcelain v2 output into the branch headers and
|
||||
@@ -237,8 +273,10 @@ func parseGitPollSnapshot(out []byte) gitPollSnapshot {
|
||||
if head := strings.TrimPrefix(line, gitStatusBranchHeadPrefix); head != gitStatusDetachedHead {
|
||||
snap.branch = head
|
||||
}
|
||||
case strings.HasPrefix(line, gitStatusBranchUpstreamPrefix):
|
||||
snap.upstream = strings.TrimPrefix(line, gitStatusBranchUpstreamPrefix)
|
||||
case line == "" || strings.HasPrefix(line, gitStatusHeaderPrefix):
|
||||
// Other headers (branch.upstream, branch.ab) are not poll state.
|
||||
// branch.ab is not poll state (suppressed by --no-ahead-behind anyway).
|
||||
default:
|
||||
entries = append(entries, line...)
|
||||
entries = append(entries, '\n')
|
||||
@@ -251,22 +289,24 @@ func parseGitPollSnapshot(out []byte) gitPollSnapshot {
|
||||
}
|
||||
|
||||
// gitPollDelta is a snapshot compared against the cached state: what git says
|
||||
// now, what it said last tick, and which of the three tracked values moved.
|
||||
// now, what it said last tick, and which of the four tracked values moved.
|
||||
type gitPollDelta struct {
|
||||
snap gitPollSnapshot
|
||||
previousHead string
|
||||
previousBranch string
|
||||
headChanged bool
|
||||
branchChanged bool
|
||||
indexChanged bool
|
||||
snap gitPollSnapshot
|
||||
previousHead string
|
||||
previousBranch string
|
||||
headChanged bool
|
||||
branchChanged bool
|
||||
indexChanged bool
|
||||
upstreamChanged bool
|
||||
}
|
||||
|
||||
// compareToCachedState reads the cached HEAD, branch and index hash once and
|
||||
// diffs the snapshot against them.
|
||||
// compareToCachedState reads the cached HEAD, branch, upstream SHA and index
|
||||
// hash once and diffs the snapshot against them.
|
||||
func (wt *WorkspaceTracker) compareToCachedState(snap gitPollSnapshot) gitPollDelta {
|
||||
wt.gitStateMu.RLock()
|
||||
previousHead := wt.cachedHeadSHA
|
||||
previousBranch := wt.cachedBranchName
|
||||
previousUpstreamSHA := wt.cachedUpstreamSHA
|
||||
previousIndexHash := wt.cachedIndexHash
|
||||
wt.gitStateMu.RUnlock()
|
||||
|
||||
@@ -278,17 +318,26 @@ func (wt *WorkspaceTracker) compareToCachedState(snap gitPollSnapshot) gitPollDe
|
||||
// Track all transitions including to/from detached HEAD.
|
||||
branchChanged: snap.branch != previousBranch,
|
||||
indexChanged: snap.indexHash != "" && snap.indexHash != previousIndexHash,
|
||||
// A push or fetch moves the upstream ref without touching HEAD, the
|
||||
// local branch name, or the index — none of the other three signals
|
||||
// observe it. Compared as a plain value change (not gated on
|
||||
// non-empty like headChanged) so losing the upstream entirely, e.g. a
|
||||
// deleted remote branch, is also observed.
|
||||
upstreamChanged: snap.upstreamSHA != previousUpstreamSHA,
|
||||
}
|
||||
}
|
||||
|
||||
// checkGitChanges checks if HEAD or git index has changed and processes changes.
|
||||
// It only routes: each case below owns its own cache update and notifications.
|
||||
// checkGitChanges checks if HEAD, git index, or the upstream ref has changed
|
||||
// and processes changes. It only routes: each case below owns its own cache
|
||||
// update and notifications.
|
||||
func (wt *WorkspaceTracker) checkGitChanges(ctx context.Context, snap gitPollSnapshot) {
|
||||
delta := wt.compareToCachedState(snap)
|
||||
|
||||
switch {
|
||||
case !delta.headChanged && !delta.branchChanged && !delta.indexChanged:
|
||||
case !delta.headChanged && !delta.branchChanged && !delta.indexChanged && !delta.upstreamChanged:
|
||||
// Nothing moved.
|
||||
case delta.upstreamChanged && !delta.headChanged && !delta.branchChanged && !delta.indexChanged:
|
||||
wt.handleUpstreamOnlyChange(ctx, delta)
|
||||
case delta.indexChanged && !delta.headChanged && !delta.branchChanged:
|
||||
wt.handleIndexOnlyChange(ctx, delta)
|
||||
case delta.branchChanged && !delta.headChanged:
|
||||
@@ -298,11 +347,35 @@ func (wt *WorkspaceTracker) checkGitChanges(ctx context.Context, snap gitPollSna
|
||||
}
|
||||
}
|
||||
|
||||
// handleUpstreamOnlyChange records a push or fetch that moved the upstream
|
||||
// ref without changing HEAD, the branch, or the index — the case the other
|
||||
// three signals can't see on their own.
|
||||
//
|
||||
// Unlike the other three handlers, the cache write happens only after
|
||||
// tryUpdateGitStatus confirms it actually published a status. If it was
|
||||
// skipped (updateMu held by a concurrent RefreshGitStatus) or getGitStatus
|
||||
// itself failed, cachedUpstreamSHA is left at its old value on purpose: the
|
||||
// next tick's compareToCachedState will see the same "changed" delta again
|
||||
// and retry, instead of the event being silently marked handled while no
|
||||
// refreshed RemoteAhead/RemoteBranch was ever published — which push
|
||||
// detection depends on to fire (event_handlers_git.go).
|
||||
func (wt *WorkspaceTracker) handleUpstreamOnlyChange(ctx context.Context, delta gitPollDelta) {
|
||||
wt.logger.Debug("git upstream ref changed (push or fetch detected)")
|
||||
if !wt.tryUpdateGitStatus(ctx) {
|
||||
return
|
||||
}
|
||||
|
||||
wt.gitStateMu.Lock()
|
||||
wt.cachedUpstreamSHA = delta.snap.upstreamSHA
|
||||
wt.gitStateMu.Unlock()
|
||||
}
|
||||
|
||||
// handleIndexOnlyChange records staging/unstaging that left HEAD and the branch
|
||||
// alone.
|
||||
func (wt *WorkspaceTracker) handleIndexOnlyChange(ctx context.Context, delta gitPollDelta) {
|
||||
wt.gitStateMu.Lock()
|
||||
wt.cachedIndexHash = delta.snap.indexHash
|
||||
wt.cachedUpstreamSHA = delta.snap.upstreamSHA
|
||||
wt.gitStateMu.Unlock()
|
||||
|
||||
wt.logger.Debug("git index changed (staging/unstaging detected)")
|
||||
@@ -317,6 +390,7 @@ func (wt *WorkspaceTracker) handleBranchChangeWithoutHead(ctx context.Context, d
|
||||
wt.gitStateMu.Lock()
|
||||
wt.cachedBranchName = delta.snap.branch
|
||||
wt.cachedIndexHash = delta.snap.indexHash
|
||||
wt.cachedUpstreamSHA = delta.snap.upstreamSHA
|
||||
wt.gitStateMu.Unlock()
|
||||
|
||||
// In detached HEAD state there is no branch to report a switch to, so just
|
||||
@@ -340,6 +414,7 @@ func (wt *WorkspaceTracker) handleHeadChange(ctx context.Context, delta gitPollD
|
||||
wt.cachedHeadSHA = delta.snap.headSHA
|
||||
wt.cachedBranchName = delta.snap.branch
|
||||
wt.cachedIndexHash = delta.snap.indexHash
|
||||
wt.cachedUpstreamSHA = delta.snap.upstreamSHA
|
||||
wt.gitStateMu.Unlock()
|
||||
|
||||
// Branch switches need special handling to update the base commit. Skipped
|
||||
|
||||
@@ -0,0 +1,197 @@
|
||||
package process
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestCheckGitChanges_DetectsPushWithNoOtherChange pins the structural fix
|
||||
// for a production bug: pollGitChanges' checkGitChanges only re-published
|
||||
// git status on a HEAD, branch, or working-tree-index change. A push moves
|
||||
// none of those three — the upstream ref is the only thing that changes —
|
||||
// so a push-only event was previously invisible to change detection and
|
||||
// tryUpdateGitStatus never re-fired, meaning the refreshed status carrying
|
||||
// the new RemoteAhead/RemoteBranch values (which push-detection in
|
||||
// event_handlers_git.go depends on) was never published at all. This test
|
||||
// primes the tracker's cache to a pre-push state, pushes for real, and
|
||||
// asserts a single checkGitChanges call republishes status with the
|
||||
// post-push RemoteAhead/RemoteBranch, on a HEAD that never moves after the
|
||||
// prime (matching an already-committed, not-yet-pushed branch).
|
||||
func TestCheckGitChanges_DetectsPushWithNoOtherChange(t *testing.T) {
|
||||
repoDir, cleanup := setupTestRepo(t)
|
||||
defer cleanup()
|
||||
|
||||
runGit(t, repoDir, "checkout", "-b", "feature/push-only")
|
||||
writeFile(t, repoDir, "feature.txt", "local change")
|
||||
runGit(t, repoDir, "add", ".")
|
||||
runGit(t, repoDir, "commit", "-m", "local change")
|
||||
|
||||
log := newTestLogger(t)
|
||||
wt := NewWorkspaceTracker(repoDir, log)
|
||||
ctx := context.Background()
|
||||
|
||||
// Prime the cache exactly as pollGitChanges' startup block does, so this
|
||||
// test exercises checkGitChanges' own change-detection rather than the
|
||||
// unconditional first-ever-status-computation path.
|
||||
primeSnap, err := wt.readGitPollSnapshot(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("readGitPollSnapshot failed: %v", err)
|
||||
}
|
||||
wt.gitStateMu.Lock()
|
||||
wt.cachedHeadSHA = primeSnap.headSHA
|
||||
wt.cachedBranchName = primeSnap.branch
|
||||
wt.cachedIndexHash = primeSnap.indexHash
|
||||
wt.cachedUpstreamSHA = primeSnap.upstreamSHA
|
||||
wt.gitStateMu.Unlock()
|
||||
if wt.cachedUpstreamSHA != "" {
|
||||
t.Fatalf("expected no upstream before the push, got %q", wt.cachedUpstreamSHA)
|
||||
}
|
||||
|
||||
// A no-op tick: nothing changed since priming, so status must not have
|
||||
// been computed yet (currentStatus stays at its zero value).
|
||||
noopSnap, err := wt.readGitPollSnapshot(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("readGitPollSnapshot failed: %v", err)
|
||||
}
|
||||
wt.checkGitChanges(ctx, noopSnap)
|
||||
wt.mu.RLock()
|
||||
stillEmpty := wt.currentStatus.HeadCommit == ""
|
||||
wt.mu.RUnlock()
|
||||
if !stillEmpty {
|
||||
t.Fatal("expected no status update on a tick with nothing changed since priming")
|
||||
}
|
||||
|
||||
runGit(t, repoDir, "push", "-u", "origin", "feature/push-only")
|
||||
|
||||
// HEAD, branch name, and the working tree are all unchanged by the push
|
||||
// — only the upstream ref moved. This is the exact case that used to be
|
||||
// invisible to checkGitChanges.
|
||||
pushSnap, err := wt.readGitPollSnapshot(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("readGitPollSnapshot failed: %v", err)
|
||||
}
|
||||
wt.checkGitChanges(ctx, pushSnap)
|
||||
|
||||
wt.mu.RLock()
|
||||
status := wt.currentStatus
|
||||
wt.mu.RUnlock()
|
||||
if status.RemoteBranch == "" {
|
||||
t.Fatal("expected RemoteBranch to be populated after checkGitChanges observed the push")
|
||||
}
|
||||
if status.RemoteAhead != 0 {
|
||||
t.Fatalf("RemoteAhead = %d, want 0 after push", status.RemoteAhead)
|
||||
}
|
||||
}
|
||||
|
||||
// TestHandleUpstreamOnlyChange_RetriesWhenUpdateSkipped pins the fix for a
|
||||
// race in handleUpstreamOnlyChange: if updateMu is held by a concurrent
|
||||
// RefreshGitStatus (e.g. triggered by a git operation elsewhere) at the exact
|
||||
// tick that observes an upstream ref move, tryUpdateGitStatus is skipped and
|
||||
// no refreshed RemoteAhead/RemoteBranch is published. The handler must leave
|
||||
// cachedUpstreamSHA at its old value in that case, so the next tick's
|
||||
// compareToCachedState sees the same "changed" delta and retries — instead of
|
||||
// marking the push observed while nothing was ever published, which would
|
||||
// permanently starve push detection of the status update it needs.
|
||||
func TestHandleUpstreamOnlyChange_RetriesWhenUpdateSkipped(t *testing.T) {
|
||||
repoDir, cleanup := setupTestRepo(t)
|
||||
defer cleanup()
|
||||
|
||||
runGit(t, repoDir, "checkout", "-b", "feature/skipped-update")
|
||||
writeFile(t, repoDir, "feature.txt", "local change")
|
||||
runGit(t, repoDir, "add", ".")
|
||||
runGit(t, repoDir, "commit", "-m", "local change")
|
||||
|
||||
log := newTestLogger(t)
|
||||
wt := NewWorkspaceTracker(repoDir, log)
|
||||
ctx := context.Background()
|
||||
|
||||
primeSnap, err := wt.readGitPollSnapshot(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("readGitPollSnapshot failed: %v", err)
|
||||
}
|
||||
wt.gitStateMu.Lock()
|
||||
wt.cachedHeadSHA = primeSnap.headSHA
|
||||
wt.cachedBranchName = primeSnap.branch
|
||||
wt.cachedIndexHash = primeSnap.indexHash
|
||||
wt.cachedUpstreamSHA = primeSnap.upstreamSHA
|
||||
wt.gitStateMu.Unlock()
|
||||
|
||||
runGit(t, repoDir, "push", "-u", "origin", "feature/skipped-update")
|
||||
pushSnap, err := wt.readGitPollSnapshot(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("readGitPollSnapshot failed: %v", err)
|
||||
}
|
||||
if pushSnap.upstreamSHA == "" {
|
||||
t.Fatal("expected a non-empty upstream SHA after the push")
|
||||
}
|
||||
|
||||
// Simulate a concurrent RefreshGitStatus already in progress.
|
||||
wt.updateMu.Lock()
|
||||
wt.checkGitChanges(ctx, pushSnap)
|
||||
wt.updateMu.Unlock()
|
||||
|
||||
wt.gitStateMu.RLock()
|
||||
cachedAfterSkip := wt.cachedUpstreamSHA
|
||||
wt.gitStateMu.RUnlock()
|
||||
if cachedAfterSkip != "" {
|
||||
t.Fatalf("cachedUpstreamSHA = %q, want empty (unchanged) after a skipped update — the push event must not be marked observed", cachedAfterSkip)
|
||||
}
|
||||
wt.mu.RLock()
|
||||
stillEmpty := wt.currentStatus.HeadCommit == ""
|
||||
wt.mu.RUnlock()
|
||||
if !stillEmpty {
|
||||
t.Fatal("expected no status to have been published while updateMu was held")
|
||||
}
|
||||
|
||||
// Next tick, lock is free: the same delta must be retried and succeed.
|
||||
wt.checkGitChanges(ctx, pushSnap)
|
||||
|
||||
wt.gitStateMu.RLock()
|
||||
cachedAfterRetry := wt.cachedUpstreamSHA
|
||||
wt.gitStateMu.RUnlock()
|
||||
if cachedAfterRetry != pushSnap.upstreamSHA {
|
||||
t.Fatalf("cachedUpstreamSHA = %q, want %q after the retried tick", cachedAfterRetry, pushSnap.upstreamSHA)
|
||||
}
|
||||
wt.mu.RLock()
|
||||
status := wt.currentStatus
|
||||
wt.mu.RUnlock()
|
||||
if status.RemoteBranch == "" {
|
||||
t.Fatal("expected RemoteBranch to be populated after the retried tick")
|
||||
}
|
||||
if status.RemoteAhead != 0 {
|
||||
t.Fatalf("RemoteAhead = %d, want 0 after the retried tick", status.RemoteAhead)
|
||||
}
|
||||
}
|
||||
|
||||
// TestReadGitPollSnapshot_UpstreamLookupErrorPropagates pins the fix for a
|
||||
// silent-failure mode in getUpstreamSHA: git can report `branch.upstream` in
|
||||
// --porcelain=v2 output (from branch.<name>.remote/merge config) while the
|
||||
// remote-tracking ref itself is gone, so `git rev-parse @{upstream}` fails.
|
||||
// Coercing that failure to "" would be indistinguishable from "no upstream
|
||||
// configured" and could mask a real transition; readGitPollSnapshot must
|
||||
// instead fail the whole snapshot so gitPollTick's existing retry path (via
|
||||
// handleGitPollFailure) picks it up on the next tick.
|
||||
func TestReadGitPollSnapshot_UpstreamLookupErrorPropagates(t *testing.T) {
|
||||
repoDir, cleanup := setupTestRepo(t)
|
||||
defer cleanup()
|
||||
|
||||
runGit(t, repoDir, "checkout", "-b", "feature/dangling-upstream")
|
||||
runGit(t, repoDir, "push", "-u", "origin", "feature/dangling-upstream")
|
||||
|
||||
log := newTestLogger(t)
|
||||
wt := NewWorkspaceTracker(repoDir, log)
|
||||
ctx := context.Background()
|
||||
|
||||
if _, err := wt.readGitPollSnapshot(ctx); err != nil {
|
||||
t.Fatalf("readGitPollSnapshot failed before breaking the upstream ref: %v", err)
|
||||
}
|
||||
|
||||
// branch.<name>.remote/merge config stays intact; only the
|
||||
// remote-tracking ref is deleted, so git status still reports
|
||||
// branch.upstream while `@{upstream}` no longer resolves.
|
||||
runGit(t, repoDir, "update-ref", "-d", "refs/remotes/origin/feature/dangling-upstream")
|
||||
|
||||
if _, err := wt.readGitPollSnapshot(ctx); err == nil {
|
||||
t.Fatal("expected readGitPollSnapshot to fail when @{upstream} can't be resolved, got nil error")
|
||||
}
|
||||
}
|
||||
@@ -23,15 +23,18 @@ var safeBranchRefPattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._/-]*$`)
|
||||
|
||||
// updateGitStatus updates the git status. Callers must coordinate access
|
||||
// via updateMu — use tryUpdateGitStatus for polling loops, RefreshGitStatus
|
||||
// for user-triggered operations.
|
||||
func (wt *WorkspaceTracker) updateGitStatus(ctx context.Context) {
|
||||
// for user-triggered operations. Returns whether a new status was computed
|
||||
// and published; false on a getGitStatus failure or a context already
|
||||
// cancelled, so callers that must not lose the event they're reacting to
|
||||
// (see tryUpdateGitStatus) know a retry is needed.
|
||||
func (wt *WorkspaceTracker) updateGitStatus(ctx context.Context) bool {
|
||||
status, err := wt.getGitStatus(ctx)
|
||||
if err != nil {
|
||||
wt.logger.Warn("updateGitStatus: getGitStatus failed", zap.Error(err))
|
||||
return
|
||||
return false
|
||||
}
|
||||
if ctx.Err() != nil || wt.cancelCtx.Err() != nil {
|
||||
return
|
||||
return false
|
||||
}
|
||||
|
||||
wt.mu.Lock()
|
||||
@@ -40,17 +43,23 @@ func (wt *WorkspaceTracker) updateGitStatus(ctx context.Context) {
|
||||
|
||||
// Notify workspace stream subscribers
|
||||
wt.notifyWorkspaceStreamGitStatus(status)
|
||||
return true
|
||||
}
|
||||
|
||||
// tryUpdateGitStatus attempts a non-blocking git status update. If another
|
||||
// update is already in progress (from the other polling loop or an explicit
|
||||
// refresh), the call is skipped — the running update will produce the same result.
|
||||
func (wt *WorkspaceTracker) tryUpdateGitStatus(ctx context.Context) {
|
||||
// refresh), the call is skipped — the running update will produce the same
|
||||
// result. Returns whether it actually published a new status: false either
|
||||
// because it was skipped (lock contention) or because updateGitStatus itself
|
||||
// failed. Callers reacting to a specific change (e.g. an upstream ref move)
|
||||
// must check this before treating that change as observed — see
|
||||
// handleUpstreamOnlyChange in workspace_git_poll.go.
|
||||
func (wt *WorkspaceTracker) tryUpdateGitStatus(ctx context.Context) bool {
|
||||
if !wt.updateMu.TryLock() {
|
||||
return
|
||||
return false
|
||||
}
|
||||
defer wt.updateMu.Unlock()
|
||||
wt.updateGitStatus(ctx)
|
||||
return wt.updateGitStatus(ctx)
|
||||
}
|
||||
|
||||
// RefreshGitStatus forces a git status refresh and notifies subscribers.
|
||||
@@ -216,6 +225,11 @@ func (wt *WorkspaceTracker) computeGitStatus(ctx context.Context) (types.GitStat
|
||||
return update, err
|
||||
}
|
||||
|
||||
wt.getRemoteAheadBehindCounts(ctx, &update, prior)
|
||||
if err := ctx.Err(); err != nil {
|
||||
return update, err
|
||||
}
|
||||
|
||||
if err := wt.parseGitStatusOutput(ctx, &update); err != nil {
|
||||
return update, err
|
||||
}
|
||||
@@ -253,7 +267,9 @@ func (wt *WorkspaceTracker) getGitBranchInfo(ctx context.Context, update *types.
|
||||
}
|
||||
update.Branch = strings.TrimSpace(string(branchOut))
|
||||
|
||||
// Get remote branch
|
||||
// Get remote branch. A non-nil error here is the normal, expected case
|
||||
// for a branch with no upstream yet (not logged — this fires on every
|
||||
// poll of every unpushed branch).
|
||||
if remoteOut, err := wt.runGitOutput(ctx, "rev-parse", "--abbrev-ref", "@{upstream}"); err == nil {
|
||||
update.RemoteBranch = strings.TrimSpace(string(remoteOut))
|
||||
}
|
||||
@@ -393,6 +409,37 @@ func (wt *WorkspaceTracker) getAheadBehindCounts(ctx context.Context, update *ty
|
||||
update.Behind, _ = strconv.Atoi(parts[1])
|
||||
}
|
||||
|
||||
// getRemoteAheadBehindCounts populates RemoteAhead/RemoteBehind relative to
|
||||
// this branch's own upstream (@{upstream}) — the counts push-detection needs
|
||||
// to tell "has this branch been pushed" apart from Ahead/Behind, which are
|
||||
// deliberately base-branch-relative (see getAheadBehindCounts) and never
|
||||
// reach zero just because a push happened. Left at 0/0 when RemoteBranch is
|
||||
// empty (no upstream configured yet — nothing to compare against).
|
||||
//
|
||||
// Same carry-forward-on-failure treatment as getAheadBehindCounts: a
|
||||
// transient rev-list failure keeps the prior counts instead of flashing 0/0
|
||||
// (which would look like "just pushed" to a push-detection consumer).
|
||||
func (wt *WorkspaceTracker) getRemoteAheadBehindCounts(ctx context.Context, update *types.GitStatusUpdate, prior types.GitStatusUpdate) {
|
||||
if update.RemoteBranch == "" {
|
||||
update.RemoteAhead = 0
|
||||
update.RemoteBehind = 0
|
||||
return
|
||||
}
|
||||
countOut, err := wt.runGitOutput(ctx, "rev-list", "--left-right", "--count", "HEAD..."+update.RemoteBranch)
|
||||
if err != nil {
|
||||
wt.logger.Debug("getRemoteAheadBehindCounts: rev-list failed, carrying forward", zap.Error(err))
|
||||
carryRemoteAheadBehind(update, prior)
|
||||
return
|
||||
}
|
||||
parts := strings.Fields(string(countOut))
|
||||
if len(parts) != 2 {
|
||||
carryRemoteAheadBehind(update, prior)
|
||||
return
|
||||
}
|
||||
update.RemoteAhead, _ = strconv.Atoi(parts[0])
|
||||
update.RemoteBehind, _ = strconv.Atoi(parts[1])
|
||||
}
|
||||
|
||||
// branchDiffCandidates is the integration-branch priority list used when the
|
||||
// task has no recorded base_branch (legacy tasks / external branches). Kept
|
||||
// in sync between base-commit and ahead/behind resolution.
|
||||
@@ -503,6 +550,15 @@ func carryAheadBehind(update *types.GitStatusUpdate, prior types.GitStatusUpdate
|
||||
update.Behind = prior.Behind
|
||||
}
|
||||
|
||||
// carryRemoteAheadBehind mirrors carryAheadBehind for RemoteAhead/RemoteBehind.
|
||||
func carryRemoteAheadBehind(update *types.GitStatusUpdate, prior types.GitStatusUpdate) {
|
||||
if prior.HeadCommit == "" || prior.HeadCommit != update.HeadCommit {
|
||||
return
|
||||
}
|
||||
update.RemoteAhead = prior.RemoteAhead
|
||||
update.RemoteBehind = prior.RemoteBehind
|
||||
}
|
||||
|
||||
// parseGitStatusOutput runs git status --porcelain and populates the file lists and map.
|
||||
func (wt *WorkspaceTracker) parseGitStatusOutput(ctx context.Context, update *types.GitStatusUpdate) error {
|
||||
// --untracked-files=all shows all files in untracked directories, not just the directory name.
|
||||
|
||||
@@ -235,6 +235,79 @@ func mapKeys[V any](m map[string]V) []string {
|
||||
// ahead/behind git command fails (timeout, missing upstream). The contract:
|
||||
// preserve prior counts when HEAD is unchanged, leave them zero when HEAD
|
||||
// moved (the prior counts are stale by definition) or when prior is empty.
|
||||
// TestGetGitStatus_RemoteAheadTracksUpstreamNotBaseBranch pins the fix for a
|
||||
// production bug: push-detection (event_handlers_git.go) needs a signal that
|
||||
// reaches zero once a branch is pushed. Ahead/Behind are deliberately
|
||||
// base-branch-relative (see getAheadBehindCounts) and stay elevated for a
|
||||
// real feature branch's entire life — pushing the branch doesn't move the
|
||||
// base branch, so Ahead never reflects push state. RemoteAhead/RemoteBehind
|
||||
// (relative to this branch's own @{upstream}) do reach zero on push, which
|
||||
// is what this test proves end-to-end against a real git repo and remote.
|
||||
func TestGetGitStatus_RemoteAheadTracksUpstreamNotBaseBranch(t *testing.T) {
|
||||
repoDir, cleanup := setupTestRepo(t)
|
||||
defer cleanup()
|
||||
|
||||
log := newTestLogger(t)
|
||||
wt := NewWorkspaceTracker(repoDir, log)
|
||||
ctx := context.Background()
|
||||
|
||||
runGit(t, repoDir, "checkout", "-b", "feature/x")
|
||||
writeFile(t, repoDir, "feature.txt", "first change")
|
||||
runGit(t, repoDir, "add", ".")
|
||||
runGit(t, repoDir, "commit", "-m", "first change")
|
||||
|
||||
// No upstream yet for feature/x — RemoteBranch empty, RemoteAhead is
|
||||
// defined as 0 (nothing to compare against), even though there's a real
|
||||
// unpushed commit sitting locally.
|
||||
status, err := wt.getGitStatus(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("getGitStatus (pre-push): %v", err)
|
||||
}
|
||||
if status.RemoteBranch != "" {
|
||||
t.Fatalf("expected no upstream before first push, got %q", status.RemoteBranch)
|
||||
}
|
||||
if status.RemoteAhead != 0 {
|
||||
t.Fatalf("RemoteAhead = %d, want 0 (no upstream to compare against)", status.RemoteAhead)
|
||||
}
|
||||
|
||||
runGit(t, repoDir, "push", "-u", "origin", "feature/x")
|
||||
|
||||
writeFile(t, repoDir, "feature.txt", "second change")
|
||||
runGit(t, repoDir, "add", ".")
|
||||
runGit(t, repoDir, "commit", "-m", "second change")
|
||||
|
||||
// One commit ahead of main from the first push, plus this second
|
||||
// unpushed commit: Ahead (vs main) = 2, RemoteAhead (vs upstream) = 1 —
|
||||
// the two diverge exactly as documented, proving Ahead alone could never
|
||||
// signal "just pushed" for a real feature branch.
|
||||
status, err = wt.getGitStatus(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("getGitStatus (post-first-push, one unpushed commit): %v", err)
|
||||
}
|
||||
if status.Ahead != 2 {
|
||||
t.Fatalf("Ahead = %d, want 2 (both commits ahead of main)", status.Ahead)
|
||||
}
|
||||
if status.RemoteAhead != 1 {
|
||||
t.Fatalf("RemoteAhead = %d, want 1 (only the second commit is unpushed)", status.RemoteAhead)
|
||||
}
|
||||
|
||||
runGit(t, repoDir, "push", "origin", "feature/x")
|
||||
|
||||
// After the second push: RemoteAhead drops to 0 (the push-detection
|
||||
// signal), while Ahead stays at 2 (still 2 commits ahead of main —
|
||||
// pushing a feature branch never reduces its distance from main).
|
||||
status, err = wt.getGitStatus(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("getGitStatus (post-second-push): %v", err)
|
||||
}
|
||||
if status.RemoteAhead != 0 {
|
||||
t.Fatalf("RemoteAhead = %d, want 0 after push", status.RemoteAhead)
|
||||
}
|
||||
if status.Ahead != 2 {
|
||||
t.Fatalf("Ahead = %d, want 2 (unchanged by push — still ahead of main)", status.Ahead)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCarryAheadBehind(t *testing.T) {
|
||||
head := "abc123"
|
||||
tests := []struct {
|
||||
@@ -276,3 +349,45 @@ func TestCarryAheadBehind(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestCarryRemoteAheadBehind(t *testing.T) {
|
||||
head := "abc123"
|
||||
tests := []struct {
|
||||
name string
|
||||
prior types.GitStatusUpdate
|
||||
updateHead string
|
||||
wantAhead int
|
||||
wantBehind int
|
||||
}{
|
||||
{
|
||||
name: "same head preserves counts",
|
||||
prior: types.GitStatusUpdate{HeadCommit: head, RemoteAhead: 1, RemoteBehind: 3},
|
||||
updateHead: head,
|
||||
wantAhead: 1,
|
||||
wantBehind: 3,
|
||||
},
|
||||
{
|
||||
name: "different head drops counts",
|
||||
prior: types.GitStatusUpdate{HeadCommit: head, RemoteAhead: 1, RemoteBehind: 3},
|
||||
updateHead: "def456",
|
||||
wantAhead: 0,
|
||||
wantBehind: 0,
|
||||
},
|
||||
{
|
||||
name: "empty prior head no-op",
|
||||
prior: types.GitStatusUpdate{RemoteAhead: 9, RemoteBehind: 9},
|
||||
updateHead: head,
|
||||
wantAhead: 0,
|
||||
wantBehind: 0,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
update := &types.GitStatusUpdate{HeadCommit: tt.updateHead}
|
||||
carryRemoteAheadBehind(update, tt.prior)
|
||||
if update.RemoteAhead != tt.wantAhead || update.RemoteBehind != tt.wantBehind {
|
||||
t.Errorf("remote ahead/behind = %d/%d, want %d/%d", update.RemoteAhead, update.RemoteBehind, tt.wantAhead, tt.wantBehind)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -68,10 +68,11 @@ type WorkspaceTracker struct {
|
||||
mu sync.RWMutex
|
||||
|
||||
// Cached git state for detecting manual operations
|
||||
cachedHeadSHA string
|
||||
cachedBranchName string // Current branch name for detecting branch switches
|
||||
cachedIndexHash string // Hash of git status porcelain output to detect staging changes
|
||||
gitStateMu sync.RWMutex
|
||||
cachedHeadSHA string
|
||||
cachedBranchName string // Current branch name for detecting branch switches
|
||||
cachedIndexHash string // Hash of git status porcelain output to detect staging changes
|
||||
cachedUpstreamSHA string // SHA of @{upstream}, "" when no upstream — detects push/fetch
|
||||
gitStateMu sync.RWMutex
|
||||
|
||||
// Unified workspace stream subscribers
|
||||
workspaceStreamSubscribers map[types.WorkspaceStreamSubscriber]struct{}
|
||||
|
||||
@@ -32,6 +32,10 @@ type GitStatusUpdate struct {
|
||||
Renamed []string `json:"renamed"`
|
||||
|
||||
// Ahead is the number of commits ahead of the base branch (origin/main).
|
||||
// This is a "how much unreviewed work is on this branch" measure (the
|
||||
// Changes panel's Push/Pull badge), not a push-state signal: it does not
|
||||
// reach zero just because the branch was pushed, since pushing doesn't
|
||||
// move the base branch. Push-state logic must use RemoteAhead instead.
|
||||
Ahead int `json:"ahead"`
|
||||
|
||||
// Behind is the number of commits behind the base branch (origin/main).
|
||||
@@ -43,6 +47,18 @@ type GitStatusUpdate struct {
|
||||
// RemoteBranch is the tracked remote branch (e.g., "origin/main").
|
||||
RemoteBranch string `json:"remote_branch,omitempty"`
|
||||
|
||||
// RemoteAhead is the number of local commits not yet present on this
|
||||
// branch's own upstream (@{upstream}) — what `git push` would send.
|
||||
// Unlike Ahead (relative to the base branch), this reaches zero once a
|
||||
// push settles, which is what push-detection logic needs to observe a
|
||||
// push. Always 0 when RemoteBranch is empty (no upstream configured yet).
|
||||
RemoteAhead int `json:"remote_ahead"`
|
||||
|
||||
// RemoteBehind is the number of commits on the upstream not yet present
|
||||
// locally — what `git pull` would fetch. Always 0 when RemoteBranch is
|
||||
// empty.
|
||||
RemoteBehind int `json:"remote_behind"`
|
||||
|
||||
// HeadCommit is the current HEAD commit SHA.
|
||||
HeadCommit string `json:"head_commit,omitempty"`
|
||||
|
||||
|
||||
@@ -213,17 +213,30 @@ func (s *Service) persistGitStatusSnapshot(ctx context.Context, data watcher.Git
|
||||
}
|
||||
}
|
||||
|
||||
// trackPushAndAssociatePR detects git pushes by tracking the "ahead" count.
|
||||
// Two cases trigger detection:
|
||||
// pushTrackerUnsynced is the pushTracker sentinel for "no upstream
|
||||
// configured yet" (RemoteBranch == ""). Deliberately distinct from 0 (a real
|
||||
// RemoteAhead of zero, meaning "has an upstream, nothing left to push") —
|
||||
// collapsing both into 0 would make "never pushed" and "just pushed"
|
||||
// indistinguishable to the transition check below, since a task's very
|
||||
// first status observation (no upstream, no commits yet) would itself read
|
||||
// as "already synced" and silently consume the one-time first-observation
|
||||
// fast path before any real push ever happens.
|
||||
const pushTrackerUnsynced = -1
|
||||
|
||||
// trackPushAndAssociatePR detects git pushes by tracking the "remote ahead"
|
||||
// count — commits not yet present on the branch's own upstream (see
|
||||
// lifecycle.GitStatusData.RemoteAhead). Two cases trigger detection:
|
||||
//
|
||||
// 1. Transition: ahead went from >0 to 0 with a remote branch set — the
|
||||
// normal in-session push, observed across two status events.
|
||||
// 1. Transition: the branch was unsynced (no upstream yet, or remote-ahead
|
||||
// >0 with a remote branch set) on the previous observation, and this one
|
||||
// shows remote-ahead=0 with a remote branch — the normal in-session
|
||||
// push, observed across two status events.
|
||||
// 2. First-observation sync: the very first status event for this
|
||||
// (session, repo) already shows ahead=0 with a remote branch. This means
|
||||
// a push happened before agentctl's poller saw the ahead>0 phase (the
|
||||
// poll cadence missed it, or the session resumed after a restart). For a
|
||||
// fresh task branch, RemoteBranch is only populated after `git push -u`,
|
||||
// so seeing it pre-synced is itself a push signal.
|
||||
// (session, repo) already shows remote-ahead=0 with a remote branch.
|
||||
// This means a push happened before agentctl's poller saw the unsynced
|
||||
// phase (the poll cadence missed it, or the session resumed after a
|
||||
// restart). For a fresh task branch, RemoteBranch is only populated
|
||||
// after `git push -u`, so seeing it pre-synced is itself a push signal.
|
||||
//
|
||||
// Without (2), multi-repo tasks routinely lose PR associations for any repo
|
||||
// whose first poll happens to land after the push completes — the
|
||||
@@ -231,13 +244,23 @@ func (s *Service) persistGitStatusSnapshot(ctx context.Context, data watcher.Git
|
||||
//
|
||||
// Multi-repo: keyed per (session, repository_name) so each repo's
|
||||
// transitions are tracked independently. Without this, agentctl's per-repo
|
||||
// status events race-overwrote each other's ahead counts and only one
|
||||
// status events race-overwrote each other's remote-ahead counts and only one
|
||||
// repo's push got detected.
|
||||
//
|
||||
// Deliberately keys off RemoteAhead, not the base-branch-relative Ahead:
|
||||
// Ahead reflects "commits ahead of origin/main" for the Push/Pull UI badge
|
||||
// and stays > 0 for the entire life of a normal feature branch — it never
|
||||
// drops to 0 just because the branch itself was pushed, so it could never
|
||||
// signal "a push just happened" for real feature-branch work.
|
||||
func (s *Service) trackPushAndAssociatePR(ctx context.Context, data watcher.GitEventData) {
|
||||
key := pushTrackerKey(data.SessionID, data.Status.RepositoryName)
|
||||
prevAheadVal, loaded := s.pushTracker.Swap(key, data.Status.Ahead)
|
||||
prevAhead, _ := prevAheadVal.(int)
|
||||
if !shouldFirePushDetection(loaded, prevAhead, data.Status) {
|
||||
trackedValue := data.Status.RemoteAhead
|
||||
if data.Status.RemoteBranch == "" {
|
||||
trackedValue = pushTrackerUnsynced
|
||||
}
|
||||
prevValueVal, loaded := s.pushTracker.Swap(key, trackedValue)
|
||||
prevValue, _ := prevValueVal.(int)
|
||||
if !shouldFirePushDetection(loaded, prevValue, data.Status) {
|
||||
return
|
||||
}
|
||||
s.logger.Info("git push detected, starting PR association",
|
||||
@@ -325,25 +348,40 @@ func (s *Service) resolvePushRepositoryProvider(ctx context.Context, sessionID,
|
||||
// shouldFirePushDetection decides whether to kick off PR association for one
|
||||
// status event. It fires in two cases (see trackPushAndAssociatePR doc):
|
||||
//
|
||||
// - Transition: the previous observation had ahead>0 and this one has ahead=0
|
||||
// with a remote branch set.
|
||||
// - First observation: no previous entry, this one has ahead=0 with a
|
||||
// remote branch set — meaning a push happened before agentctl's poller
|
||||
// observed the ahead>0 phase.
|
||||
// - Transition: the previous observation was unsynced — either no upstream
|
||||
// yet (pushTrackerUnsynced) or a remote branch with remote-ahead>0 — and
|
||||
// this one has remote-ahead=0 with a remote branch set.
|
||||
// - First observation: no previous entry, this one has remote-ahead=0 with
|
||||
// a remote branch set — meaning a push happened before agentctl's poller
|
||||
// observed the unsynced phase.
|
||||
//
|
||||
// Reads RemoteAhead (commits not yet on this branch's own upstream), not the
|
||||
// base-branch-relative Ahead — see lifecycle.GitStatusData.RemoteAhead and
|
||||
// trackPushAndAssociatePR's doc comment for why Ahead can never signal this.
|
||||
//
|
||||
// prevValue must come from pushTracker's stored value, not a raw prior
|
||||
// RemoteAhead: comparing against pushTrackerUnsynced (not 0) for "was this
|
||||
// unsynced before" is what lets a task's first-ever observation (no
|
||||
// upstream, no commits) correctly NOT count as "already synced" — otherwise
|
||||
// it would consume the first-observation fast path before any real push
|
||||
// happens, and the genuine no-upstream-to-synced transition on the next
|
||||
// observation would never fire (prevValue would already read 0 == "was
|
||||
// already synced", identical to "had nothing to push", not ">0" or
|
||||
// "unsynced").
|
||||
//
|
||||
// Pulled out as a pure function so the decision logic can be tested without
|
||||
// spawning the goroutine that calls the GitHub API.
|
||||
func shouldFirePushDetection(loaded bool, prevAhead int, status *lifecycle.GitStatusData) bool {
|
||||
func shouldFirePushDetection(loaded bool, prevValue int, status *lifecycle.GitStatusData) bool {
|
||||
if status == nil {
|
||||
return false
|
||||
}
|
||||
if status.RemoteBranch == "" || status.Ahead != 0 {
|
||||
if status.RemoteBranch == "" || status.RemoteAhead != 0 {
|
||||
return false
|
||||
}
|
||||
if !loaded {
|
||||
return true
|
||||
}
|
||||
return prevAhead > 0
|
||||
return prevValue != 0
|
||||
}
|
||||
|
||||
// pushTrackerKey builds the per-(session, repo) key used by pushTracker.
|
||||
|
||||
@@ -130,6 +130,23 @@ func TestHandleBranchSwitched_ResetsPRWatch(t *testing.T) {
|
||||
// after the push had completed would silently never get its PR associated,
|
||||
// because the >0→0 transition was never observed. See task
|
||||
// 4fdff41b-095a-4158-a311-4a1a23abe064 for the original failure mode.
|
||||
//
|
||||
// Fixtures set RemoteAhead (commits ahead of this branch's own upstream),
|
||||
// not Ahead (commits ahead of the base branch) — Ahead never reaches zero
|
||||
// just because a branch got pushed, since pushing doesn't move the base
|
||||
// branch, so it can't signal push state. This was itself a production bug
|
||||
// (push detection silently never fired for any real feature branch) found
|
||||
// via an e2e regression test and fixed alongside this predicate.
|
||||
//
|
||||
// `prev` fixtures use pushTrackerUnsynced (not 0) to represent "no upstream
|
||||
// yet" — trackPushAndAssociatePR's own tracked value, not a raw prior
|
||||
// RemoteAhead. Collapsing "no upstream" and "upstream, nothing to push"
|
||||
// into the same 0 value was itself a production bug: a task's very first
|
||||
// observation (before any push) reads as remote-ahead=0-with-no-branch,
|
||||
// which would consume the one-time first-observation fast path, and the
|
||||
// real no-upstream-to-synced transition on the next observation would then
|
||||
// compare prevValue(0) > 0 and never fire — found via an e2e regression
|
||||
// test and fixed alongside this predicate.
|
||||
func TestShouldFirePushDetection(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
@@ -140,47 +157,60 @@ func TestShouldFirePushDetection(t *testing.T) {
|
||||
wantWhy string
|
||||
}{
|
||||
{
|
||||
name: "first observation, ahead=0 with remote branch fires (push happened pre-poll)",
|
||||
name: "first observation, remote-ahead=0 with remote branch fires (push happened pre-poll)",
|
||||
loaded: false,
|
||||
prev: 0,
|
||||
status: &lifecycle.GitStatusData{Ahead: 0, RemoteBranch: "origin/feature/x"},
|
||||
prev: pushTrackerUnsynced,
|
||||
status: &lifecycle.GitStatusData{RemoteAhead: 0, RemoteBranch: "origin/feature/x"},
|
||||
wantOn: true,
|
||||
wantWhy: "first-observation sync — pre-existing remote branch in synced state means a push completed before we started watching",
|
||||
},
|
||||
{
|
||||
name: "first observation, no remote branch does not fire",
|
||||
loaded: false,
|
||||
prev: 0,
|
||||
prev: pushTrackerUnsynced,
|
||||
// Branch never been pushed — RemoteBranch is empty.
|
||||
status: &lifecycle.GitStatusData{Ahead: 0, RemoteBranch: ""},
|
||||
status: &lifecycle.GitStatusData{RemoteAhead: 0, RemoteBranch: ""},
|
||||
wantOn: false,
|
||||
},
|
||||
{
|
||||
name: "first observation, ahead>0 does not fire (waiting for transition)",
|
||||
name: "first observation, remote-ahead>0 does not fire (waiting for transition)",
|
||||
loaded: false,
|
||||
prev: 0,
|
||||
status: &lifecycle.GitStatusData{Ahead: 3, RemoteBranch: "origin/feature/x"},
|
||||
prev: pushTrackerUnsynced,
|
||||
status: &lifecycle.GitStatusData{RemoteAhead: 3, RemoteBranch: "origin/feature/x"},
|
||||
wantOn: false,
|
||||
},
|
||||
{
|
||||
name: "transition >0 to 0 with remote fires (legacy in-session push)",
|
||||
loaded: true,
|
||||
prev: 2,
|
||||
status: &lifecycle.GitStatusData{Ahead: 0, RemoteBranch: "origin/feature/x"},
|
||||
status: &lifecycle.GitStatusData{RemoteAhead: 0, RemoteBranch: "origin/feature/x"},
|
||||
wantOn: true,
|
||||
},
|
||||
{
|
||||
name: "no-upstream-to-synced transition fires (the previously-missed case)",
|
||||
// This is the exact bug: a task's first-ever status observation
|
||||
// (no commits, no upstream yet) burns the !loaded fast path with
|
||||
// a negative result, so the *next* observation — the real push,
|
||||
// going from "never had an upstream" to "synced" — must still be
|
||||
// recognized as a transition even though it's not a literal
|
||||
// N>0-to-0 drop.
|
||||
loaded: true,
|
||||
prev: pushTrackerUnsynced,
|
||||
status: &lifecycle.GitStatusData{RemoteAhead: 0, RemoteBranch: "origin/feature/x"},
|
||||
wantOn: true,
|
||||
},
|
||||
{
|
||||
name: "stays at 0 after first fire does not refire",
|
||||
loaded: true,
|
||||
prev: 0,
|
||||
status: &lifecycle.GitStatusData{Ahead: 0, RemoteBranch: "origin/feature/x"},
|
||||
status: &lifecycle.GitStatusData{RemoteAhead: 0, RemoteBranch: "origin/feature/x"},
|
||||
wantOn: false,
|
||||
},
|
||||
{
|
||||
name: "transition with no remote does not fire (local-only commit was undone)",
|
||||
loaded: true,
|
||||
prev: 1,
|
||||
status: &lifecycle.GitStatusData{Ahead: 0, RemoteBranch: ""},
|
||||
status: &lifecycle.GitStatusData{RemoteAhead: 0, RemoteBranch: ""},
|
||||
wantOn: false,
|
||||
},
|
||||
{
|
||||
@@ -190,6 +220,17 @@ func TestShouldFirePushDetection(t *testing.T) {
|
||||
status: nil,
|
||||
wantOn: false,
|
||||
},
|
||||
{
|
||||
name: "base-branch-ahead is irrelevant: real feature branch, just pushed, still fires",
|
||||
// This is the exact production bug: a branch with real commits
|
||||
// ahead of main (Ahead > 0, as any normal feature branch has)
|
||||
// must still be detected as "just pushed" once RemoteAhead drops
|
||||
// to 0 — Ahead staying high must not suppress detection.
|
||||
loaded: false,
|
||||
prev: pushTrackerUnsynced,
|
||||
status: &lifecycle.GitStatusData{Ahead: 12, RemoteAhead: 0, RemoteBranch: "origin/feature/x"},
|
||||
wantOn: true,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
|
||||
@@ -0,0 +1,102 @@
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { GITLAB_PROJECT, gitLabMR } from "../../helpers/gitlab";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
|
||||
// This spec covers push-detection auto-link — the feature that discovers a
|
||||
// merge request opened OUTSIDE Kandev's own "Create MR" action (e.g. `glab mr
|
||||
// create` run as a raw shell command, or an MR opened manually via the
|
||||
// GitLab web UI) purely by observing a `git push`, and links it to the task
|
||||
// without any Kandev UI action creating or naming the MR. This is distinct
|
||||
// from gitlab-mr-creation.spec.ts, which exercises Kandev's own Create-MR
|
||||
// button (a different code path — AssociateExistingMRByURL — that already
|
||||
// worked before push-detection auto-link existed).
|
||||
test.describe("GitLab MR push-detection auto-link", () => {
|
||||
test("links an externally-opened MR after a raw git push, without using Create MR", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
backend,
|
||||
}) => {
|
||||
// Sequential assertion budget: 45s + 45s + 150s + 30s = 270s. Give it
|
||||
// headroom above that so the documented ~90s push-detection retry window
|
||||
// can actually run to completion instead of the test timing out first.
|
||||
test.setTimeout(330_000);
|
||||
const remoteURL = `${backend.baseUrl}/${GITLAB_PROJECT}.git`;
|
||||
await apiClient.configureGitLab(seedData.workspaceId, backend.baseUrl);
|
||||
await apiClient.configureGitLabRepositoryRemote(seedData.repositoryId, remoteURL);
|
||||
await apiClient.updateRepository(seedData.repositoryId, {
|
||||
provider: "gitlab",
|
||||
provider_host: backend.baseUrl,
|
||||
provider_owner: "platform",
|
||||
provider_name: "kandev",
|
||||
});
|
||||
|
||||
// First turn: commit a change but do NOT push yet, so the branch has no
|
||||
// upstream and no MR could exist for it — mirrors the ordinary "agent
|
||||
// made a change" state before any push happens.
|
||||
const task = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Push-detection GitLab auto-link",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:diff-update-setup",
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
executor_profile_id: seedData.worktreeExecutorProfileId,
|
||||
},
|
||||
);
|
||||
if (!task.session_id) throw new Error("Push-detection auto-link task did not return a session");
|
||||
|
||||
await testPage.goto(`/t/${task.id}`);
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
await expect(
|
||||
session.chat.getByText("diff-update-setup complete", { exact: false }),
|
||||
).toBeVisible({ timeout: 45_000 });
|
||||
|
||||
// Learn the worktree's actual branch, then seed an MR for that branch as
|
||||
// already open on GitLab — simulating someone opening it outside Kandev
|
||||
// (`glab mr create`, or the GitLab web UI) before the push lands. Seeded
|
||||
// ahead of the push so the very first FindMRByBranch search after push
|
||||
// detection fires can find it, the common real-world case.
|
||||
const sessions = await apiClient.listTaskSessions(task.id);
|
||||
const branch = sessions.sessions.find((s) => s.id === task.session_id)?.worktree_branch;
|
||||
if (!branch) throw new Error("Could not resolve the session's worktree branch");
|
||||
|
||||
const mrIID = 55;
|
||||
await apiClient.mockGitLabAddMRs(seedData.workspaceId, GITLAB_PROJECT, [
|
||||
gitLabMR(mrIID, "Externally opened GitLab MR", {
|
||||
head_branch: branch,
|
||||
// Seeded MRs are stored verbatim by the mock (no GitLab "opened" ->
|
||||
// "open" normalization the real client applies), and FindMRByBranch
|
||||
// only matches the normalized "open" value.
|
||||
state: "open",
|
||||
url: `${backend.baseUrl}/${GITLAB_PROJECT}/-/merge_requests/${mrIID}`,
|
||||
web_url: `${backend.baseUrl}/${GITLAB_PROJECT}/-/merge_requests/${mrIID}`,
|
||||
}),
|
||||
]);
|
||||
|
||||
// Second turn: a raw `git push`, with no Kandev "Create MR" dialog or
|
||||
// button ever used. This is the exact trigger push-detection auto-link
|
||||
// depends on.
|
||||
await session.clickSessionChatTab();
|
||||
await session.sendMessage("/e2e:push-current-branch");
|
||||
await expect(
|
||||
session.chat.getByText("push-current-branch complete", { exact: false }),
|
||||
).toBeVisible({ timeout: 45_000 });
|
||||
|
||||
// Push detection has to observe the push (agentctl git-status polling),
|
||||
// then search for and link the MR — give it the full retry window
|
||||
// (up to ~90s) plus polling latency.
|
||||
const mrButton = testPage.getByTestId("mr-topbar-button");
|
||||
await expect(mrButton).toHaveAttribute("data-mr-iid", String(mrIID), { timeout: 150_000 });
|
||||
|
||||
await testPage.reload();
|
||||
await expect(testPage.getByTestId("mr-topbar-button")).toHaveAttribute(
|
||||
"data-mr-iid",
|
||||
String(mrIID),
|
||||
{ timeout: 30_000 },
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,115 @@
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
|
||||
// GitHub twin of gitlab-mr-push-autolink.spec.ts. Covers push-detection
|
||||
// auto-link — the feature that discovers a PR opened OUTSIDE Kandev's own
|
||||
// "Create PR" action (e.g. `gh pr create` run as a raw shell command, or a
|
||||
// PR opened manually via the GitHub web UI) purely by observing a `git
|
||||
// push`, and links it to the task without any Kandev UI action creating or
|
||||
// naming the PR. Existing coverage in pr-detection.spec.ts only exercises
|
||||
// the background poller and direct mock association (mockGitHubAssociateTaskPR)
|
||||
// — neither goes through detectPushAndAssociatePR, the code path this
|
||||
// covers. trackPushAndAssociatePR/shouldFirePushDetection/checkGitChanges
|
||||
// are shared between GitHub and GitLab, so this pins the same fix the
|
||||
// GitLab spec does, from the other provider's entry point.
|
||||
test.describe("GitHub PR push-detection auto-link", () => {
|
||||
test("links an externally-opened PR after a raw git push, without using Create PR", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
backend,
|
||||
}) => {
|
||||
// Sequential assertion budget: 45s + 45s + 150s + 30s = 270s. Give it
|
||||
// headroom above that so the documented ~90s push-detection retry window
|
||||
// can actually run to completion instead of the test timing out first.
|
||||
test.setTimeout(330_000);
|
||||
// The default seeded repository has no git "origin" configured — nothing
|
||||
// in the existing PR-detection coverage ever pushes for real. The path
|
||||
// segment here isn't provider-specific: the test-harness endpoint checks
|
||||
// it against KANDEV_E2E_GITLAB_REMOTE_URL verbatim regardless of which
|
||||
// provider's test uses it, so this reuses the same fixed E2E remote path
|
||||
// gitlab-mr-push-autolink.spec.ts configures.
|
||||
await apiClient.configureGitLabRepositoryRemote(
|
||||
seedData.repositoryId,
|
||||
`${backend.baseUrl}/platform/kandev.git`,
|
||||
);
|
||||
await apiClient.updateRepository(seedData.repositoryId, {
|
||||
provider: "github",
|
||||
provider_owner: "testorg",
|
||||
provider_name: "testrepo",
|
||||
});
|
||||
await apiClient.mockGitHubReset();
|
||||
await apiClient.mockGitHubSetUser("test-user");
|
||||
|
||||
// First turn: commit a change but do NOT push yet, so the branch has no
|
||||
// upstream and no PR could exist for it — mirrors the ordinary "agent
|
||||
// made a change" state before any push happens.
|
||||
const task = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Push-detection GitHub auto-link",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:diff-update-setup",
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
executor_profile_id: seedData.worktreeExecutorProfileId,
|
||||
},
|
||||
);
|
||||
if (!task.session_id) throw new Error("Push-detection auto-link task did not return a session");
|
||||
|
||||
await testPage.goto(`/t/${task.id}`);
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
await expect(
|
||||
session.chat.getByText("diff-update-setup complete", { exact: false }),
|
||||
).toBeVisible({ timeout: 45_000 });
|
||||
|
||||
// Learn the worktree's actual branch, then seed a PR for that branch as
|
||||
// already open on GitHub — simulating someone opening it outside Kandev
|
||||
// (`gh pr create`, or the GitHub web UI) before the push lands. Seeded
|
||||
// ahead of the push so the very first FindPRByBranch search after push
|
||||
// detection fires can find it, the common real-world case.
|
||||
const sessions = await apiClient.listTaskSessions(task.id);
|
||||
const branch = sessions.sessions.find((s) => s.id === task.session_id)?.worktree_branch;
|
||||
if (!branch) throw new Error("Could not resolve the session's worktree branch");
|
||||
|
||||
const prNumber = 77;
|
||||
await apiClient.mockGitHubAddPRs([
|
||||
{
|
||||
number: prNumber,
|
||||
title: "Externally opened GitHub PR",
|
||||
state: "open",
|
||||
head_branch: branch,
|
||||
base_branch: "main",
|
||||
author_login: "test-user",
|
||||
repo_owner: "testorg",
|
||||
repo_name: "testrepo",
|
||||
additions: 5,
|
||||
deletions: 1,
|
||||
},
|
||||
]);
|
||||
|
||||
// Second turn: a raw `git push`, with no Kandev "Create PR" dialog or
|
||||
// button ever used. This is the exact trigger push-detection auto-link
|
||||
// depends on.
|
||||
await session.clickSessionChatTab();
|
||||
await session.sendMessage("/e2e:push-current-branch");
|
||||
await expect(
|
||||
session.chat.getByText("push-current-branch complete", { exact: false }),
|
||||
).toBeVisible({ timeout: 45_000 });
|
||||
|
||||
// Push detection has to observe the push (agentctl git-status polling),
|
||||
// then search for and link the PR — give it the full retry window
|
||||
// (up to ~90s) plus polling latency.
|
||||
const prButton = session.prTopbarButton();
|
||||
await expect(prButton).toHaveAttribute("data-pr-number", String(prNumber), {
|
||||
timeout: 150_000,
|
||||
});
|
||||
|
||||
await testPage.reload();
|
||||
await expect(session.prTopbarButton()).toHaveAttribute("data-pr-number", String(prNumber), {
|
||||
timeout: 30_000,
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user