fix: source commit pushed status from git remote, not PR commits (#833)

* fix: source commit pushed status from git remote, not PR commits Commits showed as "not pushed" whenever the branch had no open PR, since the UI derived pushed status by matching local SHAs against the PR's commit list. Sourcing it from the upstream tracking ref in agentctl's GetLog keeps the answer correct without a PR, after rebases, and across multi-repo workspaces. * fix: treat PR-matched commits as pushed alongside the git remote signal mergeCommits now flags a commit as pushed when EITHER the backend's git-sourced flag is true OR a PR commit matches by SHA. The git signal fixes the original "no PR = unpushed" bug; the PR signal preserves the "PR exists ⇒ commit is on remote" assumption that the E2E mocks rely on (and that holds in practice, since GitHub PRs require pushed branches). Also corrects the validation-path comment in markPushedCommits to describe the actual non-branch fall-through. * perf: bound rev-list walk in markPushedCommits to result length Without -n the walk was unbounded — a branch with many local-only commits would traverse full unpushed history per GetLog call. Capping at len(commits) keeps the two walks proportional. rev-list walks newest-first like GetLog, so the cap covers every commit we mark. --------- Co-authored-by: Kandev Agent <agent@kandev.dev>
2026-05-05 22:09:02 +01:00
parent 5e3248a66b
commit 9fe2cbd874
6 changed files with 197 additions and 11 deletions
@@ -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.
@@ -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 `^<sha>`. 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) {
@@ -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 <merge-base>..HEAD"
@@ -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<string>();
  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) {
@@ -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);
@@ -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 = {