fix: use dynamic merge-base for git commits to filter main branch commits (#504)

* fix: use dynamic merge-base for git commits to filter main branch commits

When a feature branch is rebased on main, the stored base_commit_sha
becomes outdated. Using git log base_commit..HEAD would include main
branch commits that were merged after the stored base commit.

This fix adds dynamic merge-base calculation using the session's
base_branch (target branch like origin/main). When fetching commits:
- The backend computes git merge-base HEAD target_branch
- This returns only commits not yet merged into the target branch
- Works correctly even after rebases

Changes:
- Extended SessionReader interface with GetSessionBaseBranch method
- Added GetMergeBase to GitOperator for computing merge-base
- Updated git log API to accept target_branch parameter
- Modified wsGitCommits handler to pass target branch for filtering

* fix: update executor_test mock to match new GetGitLog signature

* fix: update remaining mocks to match new GetGitLog signature

* fix: update integration test mock for GetGitLog signature

* fix: address code review feedback

- Use GetOrEnsureExecution in wsGitCommits to recover workspaces after backend restarts
- Fix archive snapshot to use baseBranch for merge-base when baseCommit is empty
- Extended ExecutionLookup interface with GetOrEnsureExecution method

* fix: distinguish not-ready errors from unexpected errors in wsGitCommits

Address CodeRabbit review feedback:
- Use errors.Is() to detect ErrSessionWorkspaceNotReady sentinel error
- Return ready:false gracefully for workspace-not-ready and session-not-ready errors
- Return actual errors for unexpected failures (database errors, etc.)
- Add comprehensive tests for error handling paths
This commit is contained in:
João Salavisa
2026-04-15 21:29:59 +01:00
committed by GitHub
parent e8c34411ae
commit b3aaa804f1
13 changed files with 229 additions and 35 deletions
+3 -2
View File
@@ -341,7 +341,8 @@ func (a *lifecycleAdapter) ResolveAgentProfile(ctx context.Context, profileID st
}
// GetGitLog retrieves the git log for a session from baseCommit to HEAD.
func (a *lifecycleAdapter) GetGitLog(ctx context.Context, sessionID, baseCommit string, limit int) (*client.GitLogResult, error) {
// If targetBranch is provided, uses dynamic merge-base calculation for accurate filtering.
func (a *lifecycleAdapter) GetGitLog(ctx context.Context, sessionID, baseCommit string, limit int, targetBranch string) (*client.GitLogResult, error) {
execution, ok := a.mgr.GetExecutionBySessionID(sessionID)
if !ok {
return nil, nil // No execution, not an error
@@ -350,7 +351,7 @@ func (a *lifecycleAdapter) GetGitLog(ctx context.Context, sessionID, baseCommit
if agentClient == nil {
return nil, nil
}
return agentClient.GitLog(ctx, baseCommit, limit)
return agentClient.GitLog(ctx, baseCommit, limit, targetBranch)
}
// GetCumulativeDiff retrieves the cumulative diff for a session from baseCommit to HEAD.
+16
View File
@@ -67,6 +67,22 @@ func (a *sessionReaderAdapter) GetSessionBaseCommit(ctx context.Context, session
return session.BaseCommitSHA
}
func (a *sessionReaderAdapter) GetSessionBaseBranch(ctx context.Context, sessionID string) string {
session, err := a.repo.GetTaskSession(ctx, sessionID)
if err != nil {
if a.logger != nil {
a.logger.Warn("failed to load session for base branch lookup",
zap.String("session_id", sessionID),
zap.Error(err))
}
return ""
}
if session == nil {
return ""
}
return session.BaseBranch
}
func provideGateway(
ctx context.Context,
log *logger.Logger,
@@ -3,6 +3,7 @@ package handlers
import (
"context"
"errors"
"fmt"
"strings"
"time"
@@ -25,6 +26,9 @@ type GitOperationFailedCallback func(ctx context.Context, sessionID, taskID, ope
// ExecutionLookup provides access to running agent executions by session ID.
type ExecutionLookup interface {
GetExecutionBySessionID(sessionID string) (*lifecycle.AgentExecution, bool)
// GetOrEnsureExecution returns an existing execution or creates one on-demand.
// Use this for workspace-oriented operations that should survive backend restarts.
GetOrEnsureExecution(ctx context.Context, sessionID string) (*lifecycle.AgentExecution, error)
}
// SessionReader is a minimal interface for reading session metadata.
@@ -34,6 +38,11 @@ type SessionReader interface {
// GetSessionBaseCommit returns the base commit SHA for a session.
// Returns empty string if not set or on error.
GetSessionBaseCommit(ctx context.Context, sessionID string) string
// GetSessionBaseBranch returns the target branch for a session (e.g., "origin/main").
// Used for computing merge-base to filter commits accurately after rebases.
// Returns empty string if not set or on error.
GetSessionBaseBranch(ctx context.Context, sessionID string) string
}
// GitHandlers provides WebSocket handlers for git worktree operations.
@@ -635,6 +644,7 @@ type GitCommitsRequest struct {
// wsGitCommits handles session.git.commits action
// The base commit SHA is always looked up from the session metadata in the database.
// This ensures commits are filtered to only those made during the session.
// When a target branch is available, we use dynamic merge-base calculation for accuracy.
func (h *GitHandlers) wsGitCommits(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
var req GitCommitsRequest
if err := msg.ParsePayload(&req); err != nil {
@@ -645,21 +655,36 @@ func (h *GitHandlers) wsGitCommits(ctx context.Context, msg *ws.Message) (*ws.Me
return nil, fmt.Errorf("session_id is required")
}
agentClient, err := h.getAgentCtlClient(req.SessionID)
// Use GetOrEnsureExecution to recover workspace after backend restarts.
// This is a workspace-oriented operation that doesn't require a running agent process.
execution, err := h.lifecycleMgr.GetOrEnsureExecution(ctx, req.SessionID)
if err != nil {
if isSessionNotReadyError(err) {
// Check for specific "not ready" errors that indicate the session workspace
// is still being prepared. For these, return ready:false so the client can retry.
if errors.Is(err, lifecycle.ErrSessionWorkspaceNotReady) || isSessionNotReadyError(err) {
return ws.NewResponse(msg.ID, msg.Action, map[string]any{
"commits": []any{},
"ready": false,
})
}
return nil, err
// For unexpected errors (database failures, etc.), return the error
// so the client can display an appropriate error message.
return nil, fmt.Errorf("failed to get execution for session %s: %w", req.SessionID, err)
}
// Look up base commit SHA from the session metadata
var baseCommit string
agentClient := execution.GetAgentCtlClient()
if agentClient == nil {
return ws.NewResponse(msg.ID, msg.Action, map[string]any{
"commits": []any{},
"ready": false,
})
}
// Look up base commit SHA and target branch from the session metadata
var baseCommit, targetBranch string
if h.sessionReader != nil {
baseCommit = h.sessionReader.GetSessionBaseCommit(ctx, req.SessionID)
targetBranch = h.sessionReader.GetSessionBaseBranch(ctx, req.SessionID)
}
// Fallback: if base_commit_sha is not stored in session, use git merge-base
@@ -675,7 +700,9 @@ func (h *GitHandlers) wsGitCommits(ctx context.Context, msg *ws.Message) (*ws.Me
}
}
result, err := agentClient.GitLog(ctx, baseCommit, req.Limit)
// Use target branch for dynamic merge-base calculation.
// This ensures accurate commit filtering even after rebases.
result, err := agentClient.GitLog(ctx, baseCommit, req.Limit, targetBranch)
if err != nil {
return nil, fmt.Errorf("git log failed: %w", err)
}
@@ -3,6 +3,7 @@ package handlers
import (
"context"
"encoding/json"
"errors"
"fmt"
"sync"
"testing"
@@ -16,6 +17,8 @@ import (
// mockExecutionLookup implements ExecutionLookup for testing.
type mockExecutionLookup struct {
executions map[string]*lifecycle.AgentExecution
// ensureErr allows tests to inject specific errors from GetOrEnsureExecution
ensureErr error
}
func (m *mockExecutionLookup) GetExecutionBySessionID(sessionID string) (*lifecycle.AgentExecution, bool) {
@@ -26,9 +29,25 @@ func (m *mockExecutionLookup) GetExecutionBySessionID(sessionID string) (*lifecy
return exec, ok
}
func (m *mockExecutionLookup) GetOrEnsureExecution(_ context.Context, sessionID string) (*lifecycle.AgentExecution, error) {
// Return injected error if set
if m.ensureErr != nil {
return nil, m.ensureErr
}
if m.executions == nil {
return nil, fmt.Errorf("no execution for session %s", sessionID)
}
exec, ok := m.executions[sessionID]
if !ok {
return nil, fmt.Errorf("no execution for session %s", sessionID)
}
return exec, nil
}
// mockSessionReader implements SessionReader for testing.
type mockSessionReader struct {
baseCommits map[string]string
baseCommits map[string]string
baseBranches map[string]string
}
func (m *mockSessionReader) GetSessionBaseCommit(_ context.Context, sessionID string) string {
@@ -38,6 +57,13 @@ func (m *mockSessionReader) GetSessionBaseCommit(_ context.Context, sessionID st
return m.baseCommits[sessionID]
}
func (m *mockSessionReader) GetSessionBaseBranch(_ context.Context, sessionID string) string {
if m.baseBranches == nil {
return ""
}
return m.baseBranches[sessionID]
}
func TestNewGitHandlers(t *testing.T) {
log := newTestLogger()
lookup := &mockExecutionLookup{}
@@ -335,7 +361,10 @@ func TestWsPush_NoExecution(t *testing.T) {
func TestWsGitCommits_NotReady(t *testing.T) {
log := newTestLogger()
lookup := &mockExecutionLookup{} // no executions → "no agent running" error
// Use "no agent running for session" error which matches isSessionNotReadyError
lookup := &mockExecutionLookup{
ensureErr: fmt.Errorf("no agent running for session session-1"),
}
h := NewGitHandlers(lookup, nil, log)
msg, _ := ws.NewRequest("test-1", ws.ActionSessionGitCommits, GitCommitsRequest{SessionID: "session-1"})
@@ -411,3 +440,76 @@ func TestIsSessionNotReadyError(t *testing.T) {
})
}
}
func TestWsGitCommits_WorkspaceNotReadyError(t *testing.T) {
// Test that ErrSessionWorkspaceNotReady returns ready:false gracefully
log := newTestLogger()
lookup := &mockExecutionLookup{
ensureErr: lifecycle.ErrSessionWorkspaceNotReady,
}
h := NewGitHandlers(lookup, nil, log)
msg, _ := ws.NewRequest("test-1", ws.ActionSessionGitCommits, GitCommitsRequest{SessionID: "session-1"})
resp, err := h.wsGitCommits(context.Background(), msg)
if err != nil {
t.Fatalf("expected graceful response for workspace not ready, got error: %v", err)
}
if resp == nil {
t.Fatal("expected non-nil response")
}
var payload map[string]any
if err := json.Unmarshal(resp.Payload, &payload); err != nil {
t.Fatalf("failed to parse payload: %v", err)
}
if ready, ok := payload["ready"].(bool); !ok || ready {
t.Errorf("expected ready=false, got %v", payload["ready"])
}
}
func TestWsGitCommits_UnexpectedError(t *testing.T) {
// Test that unexpected errors (not workspace-not-ready) are returned as errors
log := newTestLogger()
lookup := &mockExecutionLookup{
ensureErr: errors.New("database connection failed"),
}
h := NewGitHandlers(lookup, nil, log)
msg, _ := ws.NewRequest("test-1", ws.ActionSessionGitCommits, GitCommitsRequest{SessionID: "session-1"})
_, err := h.wsGitCommits(context.Background(), msg)
if err == nil {
t.Fatal("expected error for unexpected failure")
}
if !errors.Is(err, lookup.ensureErr) && err.Error() != "failed to get execution for session session-1: database connection failed" {
t.Errorf("expected wrapped database error, got: %v", err)
}
}
func TestWsGitCommits_WrappedWorkspaceNotReadyError(t *testing.T) {
// Test that wrapped ErrSessionWorkspaceNotReady is still detected
log := newTestLogger()
lookup := &mockExecutionLookup{
ensureErr: fmt.Errorf("some context: %w", lifecycle.ErrSessionWorkspaceNotReady),
}
h := NewGitHandlers(lookup, nil, log)
msg, _ := ws.NewRequest("test-1", ws.ActionSessionGitCommits, GitCommitsRequest{SessionID: "session-1"})
resp, err := h.wsGitCommits(context.Background(), msg)
if err != nil {
t.Fatalf("expected graceful response for wrapped workspace not ready, got error: %v", err)
}
if resp == nil {
t.Fatal("expected non-nil response")
}
var payload map[string]any
if err := json.Unmarshal(resp.Payload, &payload); err != nil {
t.Fatalf("failed to parse payload: %v", err)
}
if ready, ok := payload["ready"].(bool); !ok || ready {
t.Errorf("expected ready=false, got %v", payload["ready"])
}
}
+7 -2
View File
@@ -326,8 +326,13 @@ type GitCommitInfo struct {
// GitLog gets the commit log from baseCommit to HEAD.
// If baseCommit is empty, returns recent commits (limited by limit).
func (c *Client) GitLog(ctx context.Context, baseCommit string, limit int) (*GitLogResult, error) {
reqURL := fmt.Sprintf("%s/api/v1/git/log?since=%s&limit=%d", c.baseURL, url.QueryEscape(baseCommit), limit)
// If targetBranch is provided, computes merge-base dynamically for accurate filtering.
func (c *Client) GitLog(ctx context.Context, baseCommit string, limit int, targetBranch string) (*GitLogResult, error) {
reqURL := fmt.Sprintf("%s/api/v1/git/log?since=%s&limit=%d",
c.baseURL, url.QueryEscape(baseCommit), limit)
if targetBranch != "" {
reqURL += "&target_branch=" + url.QueryEscape(targetBranch)
}
req, err := http.NewRequestWithContext(ctx, "GET", reqURL, nil)
if err != nil {
@@ -89,8 +89,9 @@ type GitResetRequest struct {
// GitLogRequest for GET /api/v1/git/log
type GitLogRequest struct {
Since string `form:"since"` // Base commit SHA (exclusive)
Limit int `form:"limit"` // Max commits to return
Since string `form:"since"` // Base commit SHA (exclusive)
TargetBranch string `form:"target_branch"` // Target branch for merge-base calculation (e.g., "origin/main")
Limit int `form:"limit"` // Max commits to return
}
// GitCumulativeDiffRequest for GET /api/v1/git/cumulative-diff
@@ -541,7 +542,24 @@ func (s *Server) handleGitLog(c *gin.Context) {
}
gitOp := s.procMgr.GitOperator()
result, err := gitOp.GetLog(c.Request.Context(), req.Since, limit)
// If target_branch is provided, compute merge-base dynamically.
// This ensures we only show commits not yet merged into the target branch,
// which is accurate even after rebases.
baseCommit := req.Since
if req.TargetBranch != "" {
mergeBase, err := gitOp.GetMergeBase(c.Request.Context(), "HEAD", req.TargetBranch)
if err != nil {
s.logger.Warn("failed to compute merge-base, falling back to since",
zap.String("target_branch", req.TargetBranch),
zap.Error(err))
// Fall back to the provided base commit
} else if mergeBase != "" {
baseCommit = mergeBase
}
}
result, err := gitOp.GetLog(c.Request.Context(), baseCommit, limit)
if err != nil {
s.logger.Error("git log failed", zap.Error(err))
c.JSON(http.StatusInternalServerError, process.GitLogResult{
@@ -342,3 +342,13 @@ func (g *GitOperator) parseCommitDiff(output string) map[string]interface{} {
return files
}
// GetMergeBase returns the merge-base commit SHA between two refs (e.g., HEAD and origin/main).
// This is used to determine the common ancestor for filtering commits.
func (g *GitOperator) GetMergeBase(ctx context.Context, ref1, ref2 string) (string, error) {
output, err := g.runGitCommand(ctx, "merge-base", ref1, ref2)
if err != nil {
return "", fmt.Errorf("failed to compute merge-base: %w", err)
}
return strings.TrimSpace(output), nil
}
@@ -452,7 +452,7 @@ func (s *SimulatedAgentManagerClient) GetExecutionIDForSession(_ context.Context
}
return "", fmt.Errorf("no execution found for session %s", sessionID)
}
func (s *SimulatedAgentManagerClient) GetGitLog(_ context.Context, _, _ string, _ int) (*client.GitLogResult, error) {
func (s *SimulatedAgentManagerClient) GetGitLog(_ context.Context, _, _ string, _ int, _ string) (*client.GitLogResult, error) {
return nil, nil
}
func (s *SimulatedAgentManagerClient) GetCumulativeDiff(_ context.Context, _, _ string) (*client.CumulativeDiffResult, error) {
@@ -219,7 +219,7 @@ func (m *mockAgentManager) EnsureWorkspaceExecutionForSession(_ context.Context,
func (m *mockAgentManager) GetExecutionIDForSession(_ context.Context, _ string) (string, error) {
return "", fmt.Errorf("no execution found")
}
func (m *mockAgentManager) GetGitLog(_ context.Context, _, _ string, _ int) (*client.GitLogResult, error) {
func (m *mockAgentManager) GetGitLog(_ context.Context, _, _ string, _ int, _ string) (*client.GitLogResult, error) {
return nil, nil
}
func (m *mockAgentManager) GetCumulativeDiff(_ context.Context, _, _ string) (*client.CumulativeDiffResult, error) {
@@ -165,8 +165,9 @@ type AgentManagerClient interface {
GetExecutionIDForSession(ctx context.Context, sessionID string) (string, error)
// GetGitLog retrieves the git log for a session from baseCommit to HEAD.
// If targetBranch is provided, uses dynamic merge-base calculation for accurate filtering.
// Used for archive snapshot capture. Returns nil, nil if no execution exists.
GetGitLog(ctx context.Context, sessionID, baseCommit string, limit int) (*client.GitLogResult, error)
GetGitLog(ctx context.Context, sessionID, baseCommit string, limit int, targetBranch string) (*client.GitLogResult, error)
// GetCumulativeDiff retrieves the cumulative diff for a session from baseCommit to the
// working tree (including uncommitted/unstaged changes). Used for archive snapshot capture.
@@ -136,7 +136,7 @@ func (m *mockAgentManager) ResolveAgentProfile(ctx context.Context, profileID st
}, nil
}
func (m *mockAgentManager) GetGitLog(ctx context.Context, sessionID, baseCommit string, limit int) (*client.GitLogResult, error) {
func (m *mockAgentManager) GetGitLog(ctx context.Context, sessionID, baseCommit string, limit int, targetBranch string) (*client.GitLogResult, error) {
return nil, nil
}
@@ -133,7 +133,7 @@ func (m *mockAgentManager) ResolveAgentProfile(ctx context.Context, profileID st
}, nil
}
func (m *mockAgentManager) GetGitLog(_ context.Context, _, _ string, _ int) (*client.GitLogResult, error) {
func (m *mockAgentManager) GetGitLog(_ context.Context, _, _ string, _ int, _ string) (*client.GitLogResult, error) {
return nil, nil
}
func (m *mockAgentManager) GetCumulativeDiff(_ context.Context, _, _ string) (*client.CumulativeDiffResult, error) {
@@ -1189,35 +1189,48 @@ func (s *Service) StopExecution(ctx context.Context, executionID string, reason
func (s *Service) CaptureArchiveSnapshot(ctx context.Context, sessionID string) error {
s.logger.Info("capturing archive snapshot", zap.String("session_id", sessionID))
baseCommit, err := s.resolveArchiveBaseCommit(ctx, sessionID)
baseCommit, baseBranch, err := s.resolveArchiveBaseCommitAndBranch(ctx, sessionID)
if err != nil {
return err
}
if baseCommit == "" {
s.logger.Debug("no base_commit available, skipping archive snapshot capture",
// Skip only if we have neither baseCommit nor baseBranch for merge-base calculation
if baseCommit == "" && baseBranch == "" {
s.logger.Debug("no base_commit or base_branch available, skipping archive snapshot capture",
zap.String("session_id", sessionID))
return nil
}
if !s.captureArchiveCommits(ctx, sessionID, baseCommit) {
// Capture commits - baseBranch can be used for merge-base even if baseCommit is empty
if !s.captureArchiveCommits(ctx, sessionID, baseCommit, baseBranch) {
// Agent not running, skip diff capture as well
return nil
}
// Diff capture requires baseCommit
if baseCommit == "" {
s.logger.Debug("no base_commit available, skipping archive diff capture",
zap.String("session_id", sessionID))
return nil
}
s.captureArchiveDiff(ctx, sessionID, baseCommit)
return nil
}
// resolveArchiveBaseCommit retrieves the base commit for archive snapshot capture.
// It first checks the session's stored base_commit_sha, falling back to git status if empty.
func (s *Service) resolveArchiveBaseCommit(ctx context.Context, sessionID string) (string, error) {
// resolveArchiveBaseCommitAndBranch retrieves the base commit and branch for archive snapshot capture.
// It first checks the session's stored values, falling back to git status if empty.
func (s *Service) resolveArchiveBaseCommitAndBranch(ctx context.Context, sessionID string) (string, string, error) {
session, err := s.repo.GetTaskSession(ctx, sessionID)
if err != nil {
return "", fmt.Errorf("failed to get session: %w", err)
return "", "", fmt.Errorf("failed to get session: %w", err)
}
if session.BaseCommitSHA != "" {
return session.BaseCommitSHA, nil
baseCommit := session.BaseCommitSHA
baseBranch := session.BaseBranch
if baseCommit != "" {
return baseCommit, baseBranch, nil
}
// Fallback: try to get base commit from git status for legacy sessions
@@ -1226,21 +1239,22 @@ func (s *Service) resolveArchiveBaseCommit(ctx context.Context, sessionID string
s.logger.Debug("failed to get git status for base commit fallback",
zap.String("session_id", sessionID),
zap.Error(err))
return "", nil
return "", baseBranch, nil
}
if status != nil && status.BaseCommit != "" {
s.logger.Debug("using git status base commit as fallback for archive",
zap.String("session_id", sessionID),
zap.String("base_commit", status.BaseCommit))
return status.BaseCommit, nil
return status.BaseCommit, baseBranch, nil
}
return "", nil
return "", baseBranch, nil
}
// captureArchiveCommits fetches and saves commits from baseCommit to HEAD.
// If targetBranch is provided, uses dynamic merge-base for accurate filtering.
// Returns false if the agent is not running (caller should skip remaining capture).
func (s *Service) captureArchiveCommits(ctx context.Context, sessionID, baseCommit string) bool {
logResult, err := s.agentManager.GetGitLog(ctx, sessionID, baseCommit, 0) // 0 = no limit
func (s *Service) captureArchiveCommits(ctx context.Context, sessionID, baseCommit, targetBranch string) bool {
logResult, err := s.agentManager.GetGitLog(ctx, sessionID, baseCommit, 0, targetBranch) // 0 = no limit
if err != nil {
s.logger.Warn("failed to capture git log for archive",
zap.String("session_id", sessionID),