diff --git a/apps/backend/internal/agentctl/client/git.go b/apps/backend/internal/agentctl/client/git.go index 99285313f4..59165ee10d 100644 --- a/apps/backend/internal/agentctl/client/git.go +++ b/apps/backend/internal/agentctl/client/git.go @@ -380,6 +380,10 @@ type GitCommitInfo struct { FilesChanged int `json:"files_changed"` Insertions int `json:"insertions"` Deletions int `json:"deletions"` + // Pushed mirrors process.GitCommitInfo.Pushed — true when the commit is + // reachable from the branch's upstream tracking ref. Sourced from git, so + // it stays correct without an open PR and across multi-repo workspaces. + Pushed bool `json:"pushed"` // RepositoryName tags commits when fetched from a multi-repo subpath. Empty // for single-repo workspaces. Stamped client-side after the per-repo call // so callers can fan out across repos and merge. diff --git a/apps/backend/internal/agentctl/server/process/git_log.go b/apps/backend/internal/agentctl/server/process/git_log.go index e6b7c19a12..7f1e75357d 100644 --- a/apps/backend/internal/agentctl/server/process/git_log.go +++ b/apps/backend/internal/agentctl/server/process/git_log.go @@ -37,6 +37,11 @@ type GitCommitInfo struct { FilesChanged int `json:"files_changed"` Insertions int `json:"insertions"` Deletions int `json:"deletions"` + // Pushed is true when the commit is reachable from the branch's upstream + // tracking ref (i.e. present on the remote). Sourced from git itself — + // not from any PR API — so it stays correct when no PR exists yet, after + // rebases, and across multi-repo workspaces. + Pushed bool `json:"pushed"` // RepositoryName tags commits returned from a multi-repo log fan-out. // Empty for single-repo workspaces. Set by the API layer after the call. RepositoryName string `json:"repository_name,omitempty"` @@ -151,10 +156,60 @@ func (g *GitOperator) GetLog(ctx context.Context, baseCommit string, limit int) }) } + g.markPushedCommits(ctx, result.Commits) + result.Success = true return result, nil } +// markPushedCommits sets Pushed on each commit by looking up which commits in +// HEAD's history are NOT reachable from the upstream tracking ref. A commit is +// pushed iff it is not in that "ahead" set. When the branch has no upstream +// (never been pushed) or the lookup fails, all commits stay Pushed=false — the +// safer default than falsely claiming a commit is on the remote. +func (g *GitOperator) markPushedCommits(ctx context.Context, commits []*GitCommitInfo) { + if len(commits) == 0 { + return + } + upstream := g.getUpstreamRef(ctx) + if upstream == "" { + return + } + // Resolve to a SHA so we can pass it to rev-list as `^`. The caret + // prefix means the arg doesn't match LooksLikeCommitSHA; it falls through + // to the non-branch catch-all in runGitCommand, which passes it unchanged. + // Safe: upstreamSHA is local rev-parse output, not user input. + upstreamSHA, err := g.runGitCommand(ctx, "rev-parse", upstream) + if err != nil { + return + } + upstreamSHA = strings.TrimSpace(upstreamSHA) + if upstreamSHA == "" { + return + } + // Cap the walk to the number of commits we're marking. Without this, a + // branch with many local-only commits would walk unbounded history per + // GetLog call. rev-list walks newest-first the same way GetLog does, so + // the N most recent unpushed SHAs cover the N commits in our result. + output, err := g.runGitCommand(ctx, "rev-list", + fmt.Sprintf("-n%d", len(commits)), "HEAD", "^"+upstreamSHA) + if err != nil { + return + } + unpushed := make(map[string]struct{}) + for _, line := range strings.Split(strings.TrimSpace(output), "\n") { + sha := strings.TrimSpace(line) + if sha != "" { + unpushed[sha] = struct{}{} + } + } + for _, c := range commits { + if _, isUnpushed := unpushed[c.CommitSHA]; !isUnpushed { + c.Pushed = true + } + } +} + // GetCumulativeDiff returns the cumulative diff from baseCommit to the working tree // (including uncommitted/unstaged changes). func (g *GitOperator) GetCumulativeDiff(ctx context.Context, baseCommit string) (*CumulativeDiffResult, error) { diff --git a/apps/backend/internal/agentctl/server/process/git_log_test.go b/apps/backend/internal/agentctl/server/process/git_log_test.go index dd46cc7224..71a954dbc8 100644 --- a/apps/backend/internal/agentctl/server/process/git_log_test.go +++ b/apps/backend/internal/agentctl/server/process/git_log_test.go @@ -125,6 +125,87 @@ func TestGetLog_StaleLocalBranchScenario(t *testing.T) { } } +// TestGetLog_PushedReflectsRemoteState verifies that GetLog tags each commit +// with Pushed = true iff it's reachable from the branch's upstream tracking +// ref. This is the fix for the bug where commits showed as "not pushed" when +// they were actually on the remote — sourcing the flag from PR commits broke +// any flow without an open PR. Sourcing it from git's own remote tracking ref +// keeps the answer correct regardless of PR state. +func TestGetLog_PushedReflectsRemoteState(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + defer cleanup() + + log := newTestLogger(t) + ctx := context.Background() + + // setupTestRepo leaves us on main with one initial commit already pushed. + // Branch off and create one pushed + one unpushed commit so we exercise + // both states inside the same log range. + runGit(t, repoDir, "checkout", "-b", "feature/x") + writeFile(t, repoDir, "a.txt", "a\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "feat: a") + pushedSHA := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + runGit(t, repoDir, "push", "-u", "origin", "feature/x") + + writeFile(t, repoDir, "b.txt", "b\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "feat: b") + unpushedSHA := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + + gitOp := NewGitOperator(repoDir, log, nil) + result, err := gitOp.GetLog(ctx, "", 0) + if err != nil { + t.Fatalf("GetLog returned error: %v", err) + } + if !result.Success { + t.Fatalf("GetLog failed: %s", result.Error) + } + + got := make(map[string]bool, len(result.Commits)) + for _, c := range result.Commits { + got[c.CommitSHA] = c.Pushed + } + if pushed, ok := got[pushedSHA]; !ok { + t.Fatalf("pushed commit %s missing from log", pushedSHA) + } else if !pushed { + t.Errorf("expected pushed commit %s to be Pushed=true, got false", pushedSHA) + } + if pushed, ok := got[unpushedSHA]; !ok { + t.Fatalf("unpushed commit %s missing from log", unpushedSHA) + } else if pushed { + t.Errorf("expected local-only commit %s to be Pushed=false, got true", unpushedSHA) + } +} + +// TestGetLog_PushedFalseWhenNoUpstream guards against falsely marking commits +// as pushed on branches that have never been pushed (no upstream configured). +// The "safer default" path must keep Pushed=false rather than guessing. +func TestGetLog_PushedFalseWhenNoUpstream(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + defer cleanup() + + log := newTestLogger(t) + ctx := context.Background() + + // Branch with no upstream — never pushed. + runGit(t, repoDir, "checkout", "-b", "feature/no-upstream") + writeFile(t, repoDir, "a.txt", "a\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "feat: a") + + gitOp := NewGitOperator(repoDir, log, nil) + result, err := gitOp.GetLog(ctx, "", 0) + if err != nil { + t.Fatalf("GetLog returned error: %v", err) + } + for _, c := range result.Commits { + if c.Pushed { + t.Errorf("commit %s on branch with no upstream should be Pushed=false, got true", c.CommitSHA) + } + } +} + // TestGetLog_FirstParentSkipsMergedInCommits verifies that GetLog uses // --first-parent so commits brought in via a merge from main do not appear in // the branch's commit list. Without --first-parent, "git log ..HEAD" diff --git a/apps/web/components/task/changes-panel-helpers.ts b/apps/web/components/task/changes-panel-helpers.ts index 56fb1a4392..f139fe2e11 100644 --- a/apps/web/components/task/changes-panel-helpers.ts +++ b/apps/web/components/task/changes-panel-helpers.ts @@ -149,10 +149,15 @@ type MergedCommit = { }; /** - * Merge local session commits and PR commits into a single list. Local - * commits matched to a PR commit are marked pushed; unmatched are unpushed. - * PR-only commits (e.g. from other contributors) are appended as pushed. - * Order: unpushed first, then pushed. + * Merge local session commits and PR commits into a single list. A commit is + * pushed when EITHER source confirms it: the backend's `pushed` field + * (computed from the upstream tracking ref) or a SHA match in `prCommits` (PR + * existence implies the commit is on the remote). The git signal fixes the + * original bug — pushed commits showing as unpushed when no PR exists — while + * keeping the PR signal so a commit visible only via a PR (e.g. authored by + * another contributor, or a rebased SHA the local repo doesn't track yet) + * still renders as pushed. PR commits not represented locally are appended + * as pushed. Order: unpushed first, then pushed. */ export function mergeCommits( localCommits: { @@ -162,24 +167,28 @@ export function mergeCommits( deletions: number; /** Multi-repo: name of the repo this commit was made in. */ repository_name?: string; + pushed?: boolean; }[], prCommits: { sha: string; message: string; additions: number; deletions: number }[], ): MergedCommit[] { - const matchesPR = (localSha: string) => - prCommits.some((pr) => pr.sha.startsWith(localSha) || localSha.startsWith(pr.sha)); + const shaMatches = (a: string, b: string) => a.startsWith(b) || b.startsWith(a); const unpushed: MergedCommit[] = []; const pushed: MergedCommit[] = []; const matchedPRShas = new Set(); for (const c of localCommits) { - if (matchesPR(c.commit_sha)) { + const matchesPR = prCommits.some((pr) => shaMatches(pr.sha, c.commit_sha)); + const isPushed = c.pushed === true || matchesPR; + if (isPushed) { pushed.push({ ...c, pushed: true }); + } else { + unpushed.push({ ...c, pushed: false }); + } + if (matchesPR) { for (const pr of prCommits) { - if (pr.sha.startsWith(c.commit_sha) || c.commit_sha.startsWith(pr.sha)) { + if (shaMatches(pr.sha, c.commit_sha)) { matchedPRShas.add(pr.sha); } } - } else { - unpushed.push({ ...c, pushed: false }); } } for (const pr of prCommits) { diff --git a/apps/web/components/task/changes-panel.test.ts b/apps/web/components/task/changes-panel.test.ts index 8b714a75c6..3eb01404de 100644 --- a/apps/web/components/task/changes-panel.test.ts +++ b/apps/web/components/task/changes-panel.test.ts @@ -71,7 +71,36 @@ describe("mergeCommits", () => { ]); }); - it("marks local commits as pushed when they match PR commits", () => { + it("trusts the backend pushed flag even with no PR (the bug fix)", () => { + // The original bug: local commits were marked unpushed whenever no PR + // existed, because pushed was derived from PR-SHA matching alone. + // Sourcing pushed from git on the backend means commits pushed to a + // branch without a PR show as pushed. + const local = [ + { ...makeLocal("aaa1111", "pushed"), pushed: true }, + { ...makeLocal("bbb2222", "local"), pushed: false }, + ]; + const result = mergeCommits(local, []); + expect(result[0]).toMatchObject({ commit_sha: "bbb2222", pushed: false }); + expect(result[1]).toMatchObject({ commit_sha: "aaa1111", pushed: true }); + }); + + it("backend pushed flag wins over PR-SHA mismatch (post-rebase scenario)", () => { + // After a rebase the local SHA no longer matches the PR's SHA, but the + // commit IS on the remote (force-pushed). The backend's pushed=true + // must override the missing PR match. + const local = [{ ...makeLocal("rebased1", "rebased"), pushed: true }]; + const pr = [makePRFull("oldsha9999", "rebased")]; + const result = mergeCommits(local, pr); + expect(result[0]).toMatchObject({ commit_sha: "rebased1", pushed: true }); + // PR-only SHA still appears, since it isn't matched. + expect(result).toHaveLength(2); + expect(result[1]).toMatchObject({ commit_sha: "oldsha9999", pushed: true }); + }); + + it("falls back to PR-SHA matching when backend pushed flag is absent", () => { + // Older commit_created notifications don't carry the pushed flag. + // PR matching kicks in as a backstop. const local = [makeLocal("aaa1111", "first")]; const pr = [makePRFull("aaa1111bbbccc", "first", "user")]; const result = mergeCommits(local, pr); diff --git a/apps/web/lib/state/slices/session-runtime/types.ts b/apps/web/lib/state/slices/session-runtime/types.ts index 77724932bd..4dc72ce667 100644 --- a/apps/web/lib/state/slices/session-runtime/types.ts +++ b/apps/web/lib/state/slices/session-runtime/types.ts @@ -112,6 +112,14 @@ export type SessionCommit = { created_at: string; /** Multi-repo: name of the repo this commit was made in. Empty for single-repo. */ repository_name?: string; + /** + * True when the commit is reachable from the branch's upstream tracking ref. + * Sourced from git on the backend so it stays correct without an open PR. + * Optional because incremental commit_created notifications don't carry it + * (newly-made commits are always unpushed); the next full refetch fills it + * in with the real value. + */ + pushed?: boolean; }; export type CumulativeDiff = {