diff --git a/apps/backend/cmd/mock-agent/scenarios.go b/apps/backend/cmd/mock-agent/scenarios.go index fae471ee3b..072695da24 100644 --- a/apps/backend/cmd/mock-agent/scenarios.go +++ b/apps/backend/cmd/mock-agent/scenarios.go @@ -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/ 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 diff --git a/apps/backend/internal/agent/runtime/lifecycle/event_types.go b/apps/backend/internal/agent/runtime/lifecycle/event_types.go index 9a921bd29e..70f6f28b4f 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/event_types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/event_types.go @@ -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"` diff --git a/apps/backend/internal/agent/runtime/lifecycle/events.go b/apps/backend/internal/agent/runtime/lifecycle/events.go index 8afdf1d43f..4822eaafdc 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/events.go +++ b/apps/backend/internal/agent/runtime/lifecycle/events.go @@ -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, diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_poll.go b/apps/backend/internal/agentctl/server/process/workspace_git_poll.go index 14dca6fdc4..f853838b87 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_poll.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_poll.go @@ -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 diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_poll_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_poll_test.go new file mode 100644 index 0000000000..6cf84a1d48 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/workspace_git_poll_test.go @@ -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..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..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") + } +} diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status.go b/apps/backend/internal/agentctl/server/process/workspace_git_status.go index 9158944fc1..446eb35378 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status.go @@ -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. diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go index 18d8e5bf08..169f5c1692 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go @@ -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) + } + }) + } +} diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker.go b/apps/backend/internal/agentctl/server/process/workspace_tracker.go index 4463c530c8..b715a26250 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker.go @@ -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{} diff --git a/apps/backend/internal/agentctl/types/streams/git.go b/apps/backend/internal/agentctl/types/streams/git.go index ed6284ea13..a302f21a8c 100644 --- a/apps/backend/internal/agentctl/types/streams/git.go +++ b/apps/backend/internal/agentctl/types/streams/git.go @@ -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"` diff --git a/apps/backend/internal/orchestrator/event_handlers_git.go b/apps/backend/internal/orchestrator/event_handlers_git.go index a8ab69c48c..4940ea933d 100644 --- a/apps/backend/internal/orchestrator/event_handlers_git.go +++ b/apps/backend/internal/orchestrator/event_handlers_git.go @@ -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. diff --git a/apps/backend/internal/orchestrator/event_handlers_git_test.go b/apps/backend/internal/orchestrator/event_handlers_git_test.go index aaf18f90a0..cf2d2acafd 100644 --- a/apps/backend/internal/orchestrator/event_handlers_git_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_git_test.go @@ -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) { diff --git a/apps/web/e2e/tests/gitlab/gitlab-mr-push-autolink.spec.ts b/apps/web/e2e/tests/gitlab/gitlab-mr-push-autolink.spec.ts new file mode 100644 index 0000000000..9a567193c9 --- /dev/null +++ b/apps/web/e2e/tests/gitlab/gitlab-mr-push-autolink.spec.ts @@ -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 }, + ); + }); +}); diff --git a/apps/web/e2e/tests/pr/pr-push-autolink.spec.ts b/apps/web/e2e/tests/pr/pr-push-autolink.spec.ts new file mode 100644 index 0000000000..ed27903020 --- /dev/null +++ b/apps/web/e2e/tests/pr/pr-push-autolink.spec.ts @@ -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, + }); + }); +});