diff --git a/apps/backend/internal/agent/runtime/lifecycle/base_branches_metadata_test.go b/apps/backend/internal/agent/runtime/lifecycle/base_branches_metadata_test.go index 32eb6f2853..3ff57ecc0a 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/base_branches_metadata_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/base_branches_metadata_test.go @@ -1,8 +1,12 @@ package lifecycle import ( + "context" + "errors" "reflect" "testing" + + "github.com/kandev/kandev/internal/common/logger" ) // TestCollectBaseBranches_MultiRepo verifies the per-repo map populated for @@ -180,3 +184,113 @@ func TestBuildLaunchMetadata_IncludesBaseBranches(t *testing.T) { t.Errorf("got %v, want %v", got, want) } } + +// fakeBaseBranchSetter records the maps pushed to one agentctl endpoint. +type fakeBaseBranchSetter struct { + calls []map[string]string + err error +} + +func (f *fakeBaseBranchSetter) SetBaseBranches(_ context.Context, branches map[string]string) error { + f.calls = append(f.calls, branches) + return f.err +} + +// Regression: the per-repo base-branch map only ever reached agentctl through +// LaunchRequest metadata (full launch) or an explicit user edit. Workspaces +// created by startAgentOnExistingWorkspace or post-restart lazy recovery got +// nothing, so WorkspaceTracker.BaseBranch() stayed empty, resolveBaseBranch +// skipped the stored ref, and the branch diff stat was computed against an +// integration candidate (origin/master) — producing whole-tree-scale numbers. +// Pushing at the agentctl-ready chokepoint covers every creation path. +func TestPushTaskBaseBranches(t *testing.T) { + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + ctx := context.Background() + + t.Run("pushes the hydrated map", func(t *testing.T) { + want := map[string]string{"alpha": "features/x", "": "features/x"} + m := &Manager{logger: log} + m.SetBaseBranchProvider(func(context.Context, string) (map[string]string, error) { + return want, nil + }) + + setter := &fakeBaseBranchSetter{} + m.pushTaskBaseBranches(ctx, "task-1", "exec-1", setter) + + if len(setter.calls) != 1 { + t.Fatalf("expected 1 SetBaseBranches call, got %d", len(setter.calls)) + } + if !reflect.DeepEqual(setter.calls[0], want) { + t.Errorf("pushed %v, want %v", setter.calls[0], want) + } + }) + + // SetBaseBranches *replaces* the map. The full-launch path already seeded + // it from LaunchRequest metadata, so pushing an empty hydration on top + // would wipe a correct map and reintroduce the bug on the one path that + // never had it. + t.Run("empty hydration does not clobber launch metadata", func(t *testing.T) { + m := &Manager{logger: log} + m.SetBaseBranchProvider(func(context.Context, string) (map[string]string, error) { + return nil, nil + }) + + setter := &fakeBaseBranchSetter{} + m.pushTaskBaseBranches(ctx, "task-1", "exec-1", setter) + + if len(setter.calls) != 0 { + t.Fatalf("expected no push for an empty map, got %d calls", len(setter.calls)) + } + }) + + t.Run("provider error is non-fatal", func(t *testing.T) { + m := &Manager{logger: log} + m.SetBaseBranchProvider(func(context.Context, string) (map[string]string, error) { + return nil, errors.New("db unavailable") + }) + + setter := &fakeBaseBranchSetter{} + m.pushTaskBaseBranches(ctx, "task-1", "exec-1", setter) + + if len(setter.calls) != 0 { + t.Fatalf("expected no push after a provider error, got %d calls", len(setter.calls)) + } + }) + + t.Run("setter error is non-fatal", func(t *testing.T) { + m := &Manager{logger: log} + m.SetBaseBranchProvider(func(context.Context, string) (map[string]string, error) { + return map[string]string{"alpha": "main"}, nil + }) + + setter := &fakeBaseBranchSetter{err: errors.New("agentctl down")} + m.pushTaskBaseBranches(ctx, "task-1", "exec-1", setter) + + if len(setter.calls) != 1 { + t.Fatalf("expected the push to be attempted once, got %d", len(setter.calls)) + } + }) + + t.Run("no provider or no task is a no-op", func(t *testing.T) { + setter := &fakeBaseBranchSetter{} + (&Manager{logger: log}).pushTaskBaseBranches(ctx, "task-1", "exec-1", setter) + + m := &Manager{logger: log} + m.SetBaseBranchProvider(func(context.Context, string) (map[string]string, error) { + return map[string]string{"alpha": "main"}, nil + }) + m.pushTaskBaseBranches(ctx, "", "exec-1", setter) + + if len(setter.calls) != 0 { + t.Fatalf("expected no pushes, got %d", len(setter.calls)) + } + }) + + t.Run("nil setter is a no-op", func(t *testing.T) { + m := &Manager{logger: log} + m.SetBaseBranchProvider(func(context.Context, string) (map[string]string, error) { + return map[string]string{"alpha": "main"}, nil + }) + m.pushTaskBaseBranches(ctx, "task-1", "exec-1", nil) + }) +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager.go b/apps/backend/internal/agent/runtime/lifecycle/manager.go index 81e4761209..cabe83f0b4 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager.go @@ -110,6 +110,11 @@ type Manager struct { // manager_subscription.go. pollAggregator *workspacePollAggregator + // baseBranchProvider hydrates a task's stored per-repo base-branch map so + // every workspace can be seeded at agentctl-ready time, not just full + // launches. See manager_base_branches.go. + baseBranchProvider BaseBranchProvider + // secretStore encrypts/decrypts runtime auth tokens (e.g., agentctl handshake tokens). // Used to persist tokens across backend restarts for remote executor recovery. secretStore secrets.SecretStore diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_base_branches.go b/apps/backend/internal/agent/runtime/lifecycle/manager_base_branches.go index 8358e453a9..3319f8e35c 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_base_branches.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_base_branches.go @@ -6,6 +6,61 @@ import ( "go.uber.org/zap" ) +// baseBranchSetter is the agentctl surface used to replace a workspace's +// per-repo base-branch map. *agentctl.Client satisfies it; tests supply a fake. +type baseBranchSetter interface { + SetBaseBranches(ctx context.Context, branches map[string]string) error +} + +// BaseBranchProvider hydrates the stored per-repo {RepositoryName → base_branch} +// map for a task. Wired to the task service, which reads task_repositories. +type BaseBranchProvider func(ctx context.Context, taskID string) (map[string]string, error) + +// SetBaseBranchProvider wires the DB-backed hydrator used to seed a workspace's +// base-branch map at agentctl-ready time. +// +// Without it, the map reaches agentctl only through LaunchRequest metadata (the +// full launch path) or an explicit user edit. Workspaces created any other way +// — an agent starting on an already-prepared workspace, or lazy recovery after +// a backend restart — got nothing, leaving WorkspaceTracker.BaseBranch() empty +// so its diff stat fell back to an integration branch. +func (m *Manager) SetBaseBranchProvider(fn BaseBranchProvider) { + m.baseBranchProvider = fn +} + +// pushTaskBaseBranches hydrates taskID's stored base-branch map and pushes it to +// one agentctl endpoint. Called from waitForAgentctlReady so every workspace +// gets the map regardless of how it was created. +// +// Best-effort throughout: a missing provider, a hydration failure, or a push +// failure is logged and never blocks the workspace. The persisted +// task_repositories.base_branch remains the source of truth. +func (m *Manager) pushTaskBaseBranches(ctx context.Context, taskID, executionID string, setter baseBranchSetter) { + if taskID == "" || setter == nil || m.baseBranchProvider == nil { + return + } + branches, err := m.baseBranchProvider(ctx, taskID) + if err != nil { + m.logger.Warn("failed to hydrate base branches for workspace", + zap.String("task_id", taskID), + zap.String("execution_id", executionID), + zap.Error(err)) + return + } + // SetBaseBranches *replaces* the stored map, so an empty hydration must not + // be pushed — on the full-launch path it would wipe the map LaunchRequest + // metadata already seeded. + if len(branches) == 0 { + return + } + if err := setter.SetBaseBranches(ctx, branches); err != nil { + m.logger.Warn("failed to seed base branches on agentctl", + zap.String("task_id", taskID), + zap.String("execution_id", executionID), + zap.Error(err)) + } +} + // PushBaseBranchesForTask sends an updated per-repo base-branch map to every // running execution of taskID. Implements // service.AgentBaseBranchPusher — called from diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle.go b/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle.go index fcd6b08b0e..ef73805aa9 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle.go @@ -102,6 +102,15 @@ func (m *Manager) Start(ctx context.Context) error { // truth — overwrite the row to match. No-op if already in sync. m.persistExecutorRunning(ctx, execution) + // Re-seed the base-branch map before reconnecting. A surviving + // agentctl instance never passes through waitForAgentctlReady, so + // without this its trackers keep whatever map they had — or none, + // if it was created by a path that could not supply one — and + // their diff stats stay pinned to an integration-branch fallback. + if client := execution.GetAgentCtlClient(); client != nil { + m.pushTaskBaseBranches(ctx, execution.TaskID, execution.ID, client) + } + // Reconnect to workspace streams (shell, git, file changes) in background // This is needed so shell.input, git status, etc. work after backend restart go m.streamManager.ReconnectAll(execution) diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle_test.go index d84830e068..85dcd4c4f3 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle_test.go @@ -2,10 +2,14 @@ package lifecycle import ( "context" + "encoding/json" + "net" + "net/http" "testing" "time" "github.com/kandev/kandev/internal/agent/executor" + agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" "github.com/kandev/kandev/internal/agentctl/server/process" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -278,3 +282,65 @@ func TestManager_StartStop(t *testing.T) { t.Fatalf("unexpected error stopping manager: %v", err) } } + +// TestManager_StartSeedsRecoveredExecution verifies the recovery path itself, +// not only the readiness helper. A recovered agentctl never passes through +// waitForAgentctlReady, so startup must seed its base-branch map before stream +// reconnection begins. +func TestManager_StartSeedsRecoveredExecution(t *testing.T) { + log := newTestRegistryLogger() + branchesCh := make(chan map[string]string, 1) + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + server := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/workspace/base-branches" { + http.NotFound(w, r) + return + } + var body struct { + BaseBranches map[string]string `json:"base_branches"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + branchesCh <- body.BaseBranches + w.WriteHeader(http.StatusOK) + })} + go func() { _ = server.Serve(listener) }() + t.Cleanup(func() { + _ = server.Close() + }) + + port := listener.Addr().(*net.TCPAddr).Port + client := agentctl.NewClient("127.0.0.1", port, log) + execRegistry := NewExecutorRegistry(log) + execRegistry.Register(&MockExecutor{ + name: executor.NameStandalone, + recoverInstances: []*ExecutorInstance{{ + InstanceID: "exec-recovered", + TaskID: "task-recovered", + Client: client, + }}, + }) + mgr := NewManager(newTestRegistry(), &MockEventBus{}, execRegistry, &MockCredentialsManager{}, &MockProfileResolver{}, nil, ExecutorFallbackWarn, "", log) + t.Cleanup(func() { _ = mgr.Stop() }) + mgr.SetBaseBranchProvider(func(context.Context, string) (map[string]string, error) { + return map[string]string{"frontend": "develop"}, nil + }) + + if err := mgr.Start(context.Background()); err != nil { + t.Fatalf("Start: %v", err) + } + + select { + case got := <-branchesCh: + if got["frontend"] != "develop" { + t.Fatalf("recovered base branch = %q, want develop; map=%v", got["frontend"], got) + } + case <-time.After(time.Second): + t.Fatal("timed out waiting for recovered execution base-branch seed") + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go b/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go index cee5f7e750..ff923017b8 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go @@ -406,6 +406,12 @@ func (m *Manager) waitForAgentctlReady(execution *AgentExecution) { // default slow poll mode even though the frontend already sent focus, // and git state updates take up to 30s to reach the UI. m.flushCachedPollMode(execution.SessionID) + // Seed the workspace's per-repo base-branch map. LaunchRequest metadata + // only carries it on the full launch path, so workspaces created by an + // agent starting on an already-prepared workspace, or by lazy recovery + // after a restart, would otherwise have none — and their branch diff stat + // would silently fall back to an integration branch. + m.pushTaskBaseBranches(ctx, execution.TaskID, execution.ID, execution.GetAgentCtlClient()) // Use the timeout context for event publishing instead of a fresh Background context m.eventPublisher.PublishAgentctlEvent(ctx, events.AgentctlReady, execution, "") } 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 978b60e2fc..0a13a3aa9f 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status.go @@ -3,6 +3,7 @@ package process import ( "context" "fmt" + "path/filepath" "regexp" "strconv" "strings" @@ -479,17 +480,93 @@ var aheadBehindFallbackCandidates = integrationBranchRefs(false) // stats/commits mismatch even though both sides are computing correctly // for the ref name they happened to resolve first. func (wt *WorkspaceTracker) resolveBaseBranch(ctx context.Context) string { - if stored := wt.BaseBranch(); stored != "" { + resolution := wt.resolveBaseBranchWithReason(ctx) + resolution.log(wt) + return resolution.ref +} + +// baseBranchReason records why resolveBaseBranch landed on the ref it did. +// The two fallback reasons have different fixes — one means the task's base +// branch never reached this tracker, the other means it did but no longer +// exists in git — so a wrong diff stat must be attributable to one of them +// without re-deriving the resolution by hand. +type baseBranchReason int + +const ( + baseBranchStored baseBranchReason = iota + baseBranchFallbackNoStored + baseBranchFallbackStoredUnresolved + baseBranchUnresolved +) + +// baseBranchResolution is resolveBaseBranch's decision plus the evidence +// needed to explain it. +type baseBranchResolution struct { + ref string + stored string + reason baseBranchReason +} + +// log reports a fallback resolution once per change. The stored case is the +// norm and stays silent. +// +// Two constraints shape this. resolveBaseBranch runs on every status poll — as +// often as every couple of seconds, per repository — so logging unconditionally +// would bury the signal in its own repetition; only a *changed* outcome is +// reported. And a recorded base branch that does not resolve is an anomaly the +// operator needs to see without turning on debug logging, so it warns, while a +// task that simply has no recorded base is ordinary and stays at debug. +func (r baseBranchResolution) log(wt *WorkspaceTracker) { + key := strconv.Itoa(int(r.reason)) + "|" + r.stored + "|" + r.ref + wt.baseBranchLogMu.Lock() + repeat := wt.lastBaseBranchLog == key + wt.lastBaseBranchLog = key + wt.baseBranchLogMu.Unlock() + if repeat || r.reason == baseBranchStored { + return + } + repository := wt.repositoryName + if repository == "" && wt.workDir != "" { + repository = filepath.Base(filepath.Clean(wt.workDir)) + } + + switch r.reason { + case baseBranchFallbackNoStored: + wt.logger.Debug("no base branch recorded for workspace, using integration fallback for diff stats", + zap.String("repository", repository), + zap.String("candidate", r.ref)) + case baseBranchFallbackStoredUnresolved: + wt.logger.Warn("recorded base branch does not resolve in git, diff stats fall back to an integration branch", + zap.String("repository", repository), + zap.String("stored_base_branch", r.stored), + zap.String("candidate", r.ref)) + case baseBranchUnresolved: + wt.logger.Warn("no base branch or integration candidate resolved, diff stats unavailable", + zap.String("repository", repository), + zap.String("stored_base_branch", r.stored)) + case baseBranchStored: + } +} + +// resolveBaseBranchWithReason is resolveBaseBranch's decision, separated so the +// outcome is assertable in tests rather than only observable through logs. +func (wt *WorkspaceTracker) resolveBaseBranchWithReason(ctx context.Context) baseBranchResolution { + stored := wt.BaseBranch() + if stored != "" { if ref := wt.resolveStoredRef(ctx, stored); ref != "" { - return ref + return baseBranchResolution{ref: ref, stored: stored, reason: baseBranchStored} } } + fallbackReason := baseBranchFallbackNoStored + if stored != "" { + fallbackReason = baseBranchFallbackStoredUnresolved + } for _, candidate := range branchDiffCandidates { if err := wt.runGit(ctx, "rev-parse", "--verify", candidate); err == nil { - return candidate + return baseBranchResolution{ref: candidate, stored: stored, reason: fallbackReason} } } - return "" + return baseBranchResolution{stored: stored, reason: baseBranchUnresolved} } // resolveAheadBehindRef is the ahead/behind variant of resolveBaseBranch. diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go index cc5df8a1af..4d31ea8980 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go @@ -2,9 +2,12 @@ package process import ( "context" + "path/filepath" "strings" "testing" + "go.uber.org/zap/zapcore" + "github.com/kandev/kandev/internal/agentctl/types" ) @@ -293,3 +296,249 @@ func TestLookupBaseBranch_FallbackToEmptyKey(t *testing.T) { }) } } + +// Regression: resolveBaseBranch fell through to the integration-branch list +// silently. When a task's base branch never reached the tracker, the diff stat +// was computed against origin/master — a wrong-but-plausible number with +// nothing in the logs to attribute it. The two fallback reasons need different +// fixes (propagation vs a branch that no longer exists in git), so they must be +// distinguishable without re-running git by hand. +func TestResolveBaseBranchWithReason(t *testing.T) { + ctx := context.Background() + + t.Run("stored branch resolves", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + runGit(t, repoDir, "checkout", "-b", "develop") + runGit(t, repoDir, "checkout", "main") + + wt := NewWorkspaceTracker(repoDir, newTestLogger(t)) + wt.SetBaseBranch("develop") + + got := wt.resolveBaseBranchWithReason(ctx) + if got.ref != "develop" { + t.Errorf("ref = %q, want %q", got.ref, "develop") + } + if got.reason != baseBranchStored { + t.Errorf("reason = %v, want baseBranchStored", got.reason) + } + }) + + t.Run("no stored branch falls back", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + wt := NewWorkspaceTracker(repoDir, newTestLogger(t)) + + got := wt.resolveBaseBranchWithReason(ctx) + if got.ref != "origin/main" { + t.Errorf("ref = %q, want %q", got.ref, "origin/main") + } + if got.reason != baseBranchFallbackNoStored { + t.Errorf("reason = %v, want baseBranchFallbackNoStored", got.reason) + } + if got.stored != "" { + t.Errorf("stored = %q, want empty", got.stored) + } + }) + + // A base branch that is recorded but missing from git is a different + // failure than one that never arrived — this is the case where the branch + // was deleted or never fetched, not a propagation gap. + t.Run("stored branch that does not exist falls back and is attributable", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + wt := NewWorkspaceTracker(repoDir, newTestLogger(t)) + wt.SetBaseBranch("features/never-fetched") + + got := wt.resolveBaseBranchWithReason(ctx) + if got.ref != "origin/main" { + t.Errorf("ref = %q, want %q", got.ref, "origin/main") + } + if got.reason != baseBranchFallbackStoredUnresolved { + t.Errorf("reason = %v, want baseBranchFallbackStoredUnresolved", got.reason) + } + if got.stored != "features/never-fetched" { + t.Errorf("stored = %q, want the recorded branch", got.stored) + } + }) + + // resolveBaseBranch keeps its exact contract — only observability is added. + t.Run("resolveBaseBranch returns the same ref", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + wt := NewWorkspaceTracker(repoDir, newTestLogger(t)) + wt.SetBaseBranch("features/never-fetched") + + if got := wt.resolveBaseBranch(ctx); got != wt.resolveBaseBranchWithReason(ctx).ref { + t.Errorf("resolveBaseBranch diverged from resolveBaseBranchWithReason") + } + }) +} + +// The reason enum only pays off if it reaches the operator, and only if it does +// so at a level and with fields that identify *which* repository fell back to +// *which* candidate. resolveBaseBranch also runs on every status poll, so an +// unsuppressed log would bury the one event that matters under thousands of +// identical repeats — the suppression is part of the contract, not an +// optimisation. +func TestResolveBaseBranchLogsFallback(t *testing.T) { + ctx := context.Background() + + t.Run("single-repo fallback names the repository", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + log, observed := newObservedTestLogger(t) + wt := NewWorkspaceTracker(repoDir, log) + + wt.resolveBaseBranch(ctx) + + entries := observed.FilterMessage( + "no base branch recorded for workspace, using integration fallback for diff stats").All() + if len(entries) != 1 { + t.Fatalf("got %d fallback log entries, want 1", len(entries)) + } + if got := entries[0].ContextMap()["repository"]; got != filepath.Base(repoDir) { + t.Errorf("repository field = %v, want %q", got, filepath.Base(repoDir)) + } + }) + + // A missing base branch is ordinary (legacy tasks, external branches), so it + // stays at debug and must not cry wolf at warn. + t.Run("no stored branch logs at debug with repository and candidate", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + log, observed := newObservedTestLogger(t) + wt := NewWorkspaceTrackerForRepo(repoDir, "frontend", log) + + wt.resolveBaseBranch(ctx) + + entries := observed.FilterMessage( + "no base branch recorded for workspace, using integration fallback for diff stats").All() + if len(entries) != 1 { + t.Fatalf("got %d fallback log entries, want 1", len(entries)) + } + if entries[0].Level != zapcore.DebugLevel { + t.Errorf("level = %v, want debug", entries[0].Level) + } + fields := entries[0].ContextMap() + if fields["repository"] != "frontend" { + t.Errorf("repository field = %v, want frontend", fields["repository"]) + } + if fields["candidate"] != "origin/main" { + t.Errorf("candidate field = %v, want origin/main", fields["candidate"]) + } + }) + + // A base branch that was recorded but does not resolve is an anomaly the + // operator has to see without turning on debug logging. + t.Run("stored branch that does not resolve warns and names the branch", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + log, observed := newObservedTestLogger(t) + wt := NewWorkspaceTrackerForRepo(repoDir, "frontend", log) + wt.SetBaseBranch("features/never-fetched") + + wt.resolveBaseBranch(ctx) + + entries := observed.FilterMessage( + "recorded base branch does not resolve in git, diff stats fall back to an integration branch").All() + if len(entries) != 1 { + t.Fatalf("got %d fallback log entries, want 1", len(entries)) + } + if entries[0].Level != zapcore.WarnLevel { + t.Errorf("level = %v, want warn", entries[0].Level) + } + fields := entries[0].ContextMap() + if fields["repository"] != "frontend" { + t.Errorf("repository field = %v, want frontend", fields["repository"]) + } + if fields["stored_base_branch"] != "features/never-fetched" { + t.Errorf("stored_base_branch field = %v, want features/never-fetched", fields["stored_base_branch"]) + } + if fields["candidate"] != "origin/main" { + t.Errorf("candidate field = %v, want origin/main", fields["candidate"]) + } + }) + + t.Run("an unchanged outcome is logged once", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + log, observed := newObservedTestLogger(t) + wt := NewWorkspaceTrackerForRepo(repoDir, "frontend", log) + wt.SetBaseBranch("features/never-fetched") + + for range 5 { + wt.resolveBaseBranch(ctx) + } + + if got := observed.Len(); got != 1 { + t.Fatalf("got %d log entries across 5 identical resolutions, want 1: %+v", got, observed.All()) + } + }) + + // Suppression keys off the outcome, not "have I ever logged" — a tracker + // whose resolution changes has to report the new one, or the log stops + // tracking reality after the first fallback. + t.Run("a changed outcome is logged again", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + log, observed := newObservedTestLogger(t) + wt := NewWorkspaceTrackerForRepo(repoDir, "frontend", log) + + wt.resolveBaseBranch(ctx) + wt.SetBaseBranch("features/never-fetched") + wt.resolveBaseBranch(ctx) + + if got := observed.Len(); got != 2 { + t.Fatalf("got %d log entries, want 2 (one per distinct outcome): %+v", got, observed.All()) + } + }) + + t.Run("different unresolved stored branches are logged separately", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + log, observed := newObservedTestLogger(t) + wt := NewWorkspaceTrackerForRepo(repoDir, "frontend", log) + + wt.SetBaseBranch("features/first-missing") + wt.resolveBaseBranch(ctx) + wt.SetBaseBranch("features/second-missing") + wt.resolveBaseBranch(ctx) + + if got := observed.Len(); got != 2 { + t.Fatalf("got %d log entries, want 2 for distinct stored branches: %+v", got, observed.All()) + } + }) + + t.Run("fallback after a successful stored resolution is logged again", func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + runGit(t, repoDir, "checkout", "-b", "develop") + runGit(t, repoDir, "checkout", "main") + + log, observed := newObservedTestLogger(t) + wt := NewWorkspaceTrackerForRepo(repoDir, "frontend", log) + + wt.SetBaseBranch("features/never-fetched") + wt.resolveBaseBranch(ctx) + wt.SetBaseBranch("develop") + wt.resolveBaseBranch(ctx) + wt.SetBaseBranch("features/never-fetched") + wt.resolveBaseBranch(ctx) + + if got := observed.Len(); got != 2 { + t.Fatalf("got %d log entries, want 2 across fallback, stored, fallback: %+v", got, observed.All()) + } + }) +} diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker.go b/apps/backend/internal/agentctl/server/process/workspace_tracker.go index 18b6aaa0c3..7afac41089 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker.go @@ -62,6 +62,13 @@ type WorkspaceTracker struct { // Empty for legacy tasks or external branches with no recorded base. baseBranch string + // lastBaseBranchLog dedupes the diff-base resolution log. resolveBaseBranch + // runs on every status poll (as often as every couple of seconds), so + // logging each resolution would bury the signal it exists to provide. + // Holds the previously logged "reason|stored|ref" so only a *change* is reported. + lastBaseBranchLog string + baseBranchLogMu sync.Mutex + // Current state currentStatus types.GitStatusUpdate currentFiles types.FileListUpdate diff --git a/apps/backend/internal/backendapp/agents.go b/apps/backend/internal/backendapp/agents.go index 2e640c2db9..91a3aee424 100644 --- a/apps/backend/internal/backendapp/agents.go +++ b/apps/backend/internal/backendapp/agents.go @@ -28,6 +28,7 @@ func provideLifecycleManager( agentSettingsRepo settingsstore.Repository, agentRegistry *registry.Registry, secretStore secrets.SecretStore, + baseBranchProvider lifecycle.BaseBranchProvider, ) (*lifecycle.Manager, error) { log.Info("Initializing Agent Manager...") @@ -131,6 +132,9 @@ func provideLifecycleManager( // MCP handler is set later in main.go after MCP handlers are registered // via lifecycleMgr.SetMCPHandler(gateway.Dispatcher) + // Wire the base-branch provider before Start so recovered executions are + // seeded during startup as well as newly-created executions. + lifecycleMgr.SetBaseBranchProvider(baseBranchProvider) if err := lifecycleMgr.Start(ctx); err != nil { return nil, err diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index ff46fe0eab..007dd4c58b 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -412,7 +412,7 @@ func startAgentInfrastructure( // ============================================ // AGENT MANAGER // ============================================ - lifecycleMgr, err := provideLifecycleManager(ctx, cfg, log, eventBus, repos.AgentSettings, agentRegistry, userSecretStore) + lifecycleMgr, err := provideLifecycleManager(ctx, cfg, log, eventBus, repos.AgentSettings, agentRegistry, userSecretStore, services.Task.TaskBaseBranches) if err != nil { log.Error("Failed to initialize agent manager", zap.Error(err)) return false diff --git a/apps/backend/internal/task/service/service_branch_update.go b/apps/backend/internal/task/service/service_branch_update.go index 5ee2878816..a652b21c3f 100644 --- a/apps/backend/internal/task/service/service_branch_update.go +++ b/apps/backend/internal/task/service/service_branch_update.go @@ -11,6 +11,7 @@ import ( "github.com/kandev/kandev/internal/common/securityutil" "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/worktree" ) // UpdateRepositoryBaseBranchRequest carries the parameters for the @@ -167,30 +168,69 @@ func isSafeBaseBranchRef(ref string) bool { return securityutil.IsValidBranchName(ref) } -// collectTaskBaseBranches builds the per-repo {RepositoryName → base_branch} -// map the agentctl WorkspaceTracker reads. Mirrors lifecycle.collectBaseBranches -// but at update time the LaunchRequest is gone, so we hydrate from the DB: -// list task_repositories, resolve each Repository to recover its Name (which -// matches the worktree dir basename and therefore the tracker's -// repositoryName), and key the map with both Name and the empty fallback for -// single-repo tasks. +// TaskBaseBranches exposes the stored per-repo base-branch map so the agent +// runtime can seed a workspace at agentctl-ready time. Wired as +// lifecycle.BaseBranchProvider; the launch path builds the same shape from the +// LaunchRequest, and this is the DB-backed equivalent for every other path. +func (s *Service) TaskBaseBranches(ctx context.Context, taskID string) (map[string]string, error) { + if taskID == "" { + return nil, nil + } + return s.collectTaskBaseBranches(ctx, taskID) +} + +// collectTaskBaseBranches builds the per-worktree {trackerName → base_branch} +// map the agentctl WorkspaceTracker reads. Mirrors +// lifecycle.baseBranchMetadataKey but at update time the LaunchRequest is gone, +// so we hydrate from the DB: list task_repositories, resolve each Repository to +// recover its Name, and derive the same branch-identity path slug the launch +// path derives, so the key equals the worktree directory basename — which is +// what the tracker reports as its repositoryName. +// +// Keying by the bare repository name is NOT sufficient. A task may attach the +// same repository on several branches (task_repositories is unique on +// task+repo+base+checkout), and those siblings live in +// `{RepoName}-{BranchSlug}` directories. Collapsing them under `{RepoName}` +// leaves every sibling tracker without a key, and because SetBaseBranches +// *replaces* the stored map, pushing that collapsed map actively overwrites the +// correctly-keyed map the launch path already seeded — sending the siblings +// back to the origin/main fallback. func (s *Service) collectTaskBaseBranches(ctx context.Context, taskID string) (map[string]string, error) { taskRepos, err := s.taskRepos.ListTaskRepositories(ctx, taskID) if err != nil { return nil, fmt.Errorf("list task repositories: %w", err) } + repos, err := s.resolveBaseBranchRepositories(ctx, taskRepos) + if err != nil { + return nil, err + } + // Plans are computed over *every* row, including rows without a base + // branch: BuildBranchIdentityPlans groups by repository and picks which + // member of a group keeps the flat legacy path. Filtering first would + // change that choice and desync these keys from the launch path's. + inputs := make([]worktree.BranchIdentityInput, len(taskRepos)) + for i, tr := range taskRepos { + defaultBranch := "" + if repos[i] != nil { + defaultBranch = repos[i].DefaultBranch + } + inputs[i] = worktree.BranchIdentityInput{ + RepositoryID: tr.RepositoryID, + BaseBranch: tr.BaseBranch, + CheckoutBranch: tr.CheckoutBranch, + DefaultBranch: defaultBranch, + PRNumber: taskRepositoryPRNumber(tr.Metadata), + Position: tr.Position, + } + } + plans := worktree.BuildBranchIdentityPlans(inputs) + out := make(map[string]string, len(taskRepos)+1) - for _, tr := range taskRepos { + for i, tr := range taskRepos { if tr.BaseBranch == "" { continue } - repo, err := s.repoEntities.GetRepository(ctx, tr.RepositoryID) - if err != nil || repo == nil { - continue - } - if repo.Name != "" { - out[repo.Name] = tr.BaseBranch - } + out[baseBranchTrackerKey(repos[i].Name, plans[i].PathSlug)] = tr.BaseBranch } // Single-repo legacy fallback: when only one row, duplicate under the // empty key so the root WorkspaceTracker (repositoryName == "") picks it @@ -206,3 +246,57 @@ func (s *Service) collectTaskBaseBranches(ctx context.Context, taskID string) (m } return out, nil } + +// resolveBaseBranchRepositories resolves the Repository entity for each row, +// positionally aligned with taskRepos so the branch-identity plans can be +// indexed alongside. +// +// A row that records a base branch but cannot resolve its repository makes the +// map INCOMPLETE, not merely smaller. agentctl's SetBaseBranches replaces the +// stored map wholesale, so pushing a partial map silently drops the base branch +// of every repository that failed to resolve — and the caller's len(map) > 0 +// guard cannot detect it, because the map is non-empty. Fail instead: callers +// already skip the push on error, leaving the previously-correct map in place. +// +// A row *without* a base branch contributes no key, so an unresolvable one is +// tolerated (nil entry) rather than failing the whole map; it still occupies +// its index so the plans stay aligned. +func (s *Service) resolveBaseBranchRepositories( + ctx context.Context, + taskRepos []*models.TaskRepository, +) ([]*models.Repository, error) { + repos := make([]*models.Repository, len(taskRepos)) + for i, tr := range taskRepos { + repo, err := s.repoEntities.GetRepository(ctx, tr.RepositoryID) + if err != nil { + if tr.BaseBranch == "" { + continue + } + return nil, fmt.Errorf("resolve repository %s for base-branch map: %w", tr.RepositoryID, err) + } + if repo == nil || repo.Name == "" { + if tr.BaseBranch == "" { + continue + } + return nil, fmt.Errorf("repository %s for base-branch map is missing or unnamed", tr.RepositoryID) + } + repos[i] = repo + } + return repos, nil +} + +// baseBranchTrackerKey builds the map key a WorkspaceTracker looks itself up +// under: the worktree directory basename. Mirrors +// lifecycle.baseBranchMetadataKey exactly — same sanitiser, same raw-name +// fallback, same `-`-joined path slug — so the DB-hydrated map and the +// launch-time map agree on every key. +func baseBranchTrackerKey(repoName, pathSlug string) string { + key := worktree.SanitizeRepoDirName(repoName) + if key == "" { + key = repoName + } + if pathSlug == "" { + return key + } + return key + "-" + pathSlug +} diff --git a/apps/backend/internal/task/service/service_branch_update_test.go b/apps/backend/internal/task/service/service_branch_update_test.go index 02dc65a46f..d31e5bb124 100644 --- a/apps/backend/internal/task/service/service_branch_update_test.go +++ b/apps/backend/internal/task/service/service_branch_update_test.go @@ -306,3 +306,189 @@ func TestUpdateRepositoryBaseBranch_NotFound(t *testing.T) { } }) } + +// Regression: collectTaskBaseBranches skipped any task_repositories row whose +// Repository could not be resolved. For a multi-repo task that yields a +// non-empty but INCOMPLETE map — and since agentctl's SetBaseBranches replaces +// the stored map wholesale, pushing it silently drops the base branch of every +// repository that was skipped. The len>0 guard does not catch this, because the +// map is not empty; it is just missing entries. A partial hydration must be +// treated as a failure, not pushed. +func TestUpdateRepositoryBaseBranch_PartialHydrationIsNotPushed(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + setBranchUpdateWorkflowStep(svc) + pusher := &fakeBaseBranchPusher{} + svc.SetAgentBaseBranchPusher(pusher) + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-2", WorkspaceID: "ws-1", Name: "backend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Multi repo", + Repositories: []TaskRepositoryInput{ + {RepositoryID: "repo-1", BaseBranch: "main", CheckoutBranch: "feature/a"}, + {RepositoryID: "repo-2", BaseBranch: "main", CheckoutBranch: "feature/b"}, + }, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + + rows, err := repo.ListTaskRepositories(ctx, task.ID) + if err != nil || len(rows) != 2 { + t.Fatalf("ListTaskRepositories: %v, rows=%d", err, len(rows)) + } + + // Delete one repository so its task_repositories row can no longer resolve + // a Name — the shape a failed GetRepository produces. + if err := repo.DeleteRepository(ctx, "repo-2"); err != nil { + t.Fatalf("DeleteRepository: %v", err) + } + + var target string + for _, r := range rows { + if r.RepositoryID == "repo-1" { + target = r.ID + } + } + + if _, err := svc.UpdateRepositoryBaseBranch(ctx, UpdateRepositoryBaseBranchRequest{ + TaskID: task.ID, + TaskRepositoryID: target, + BaseBranch: "staging", + }); err != nil { + t.Fatalf("UpdateRepositoryBaseBranch: %v", err) + } + + // The push must be skipped entirely. Pushing {frontend: staging} alone + // would replace the map and wipe backend's recorded base branch. + if calls := pusher.snapshot(); len(calls) != 0 { + t.Fatalf("expected no push for an incomplete hydration, got %d calls: %+v", len(calls), calls) + } +} + +// Regression: collectTaskBaseBranches keyed the map by the bare Repository.Name. +// A task may attach the same repository on several branches, and those siblings +// live in `{RepoName}-{BranchSlug}` worktree directories — which is the name +// their WorkspaceTracker reports. Keying by name alone collapsed every sibling +// onto one entry, so the non-flat trackers found no key and fell back to +// origin/main. Worse, because SetBaseBranches *replaces* the stored map, this +// push overwrote the correctly-keyed map the launch path had already seeded. +// +// The keys must match lifecycle.baseBranchMetadataKey: the lowest-positioned +// branch of a repeated repository keeps the flat legacy path, the rest are +// suffixed with their branch-identity path slug. +func TestUpdateRepositoryBaseBranch_MultiBranchKeysPerWorktree(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + setBranchUpdateWorkflowStep(svc) + pusher := &fakeBaseBranchPusher{} + svc.SetAgentBaseBranchPusher(pusher) + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "frontend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Title: "Same repo, two branches", + Repositories: []TaskRepositoryInput{ + {RepositoryID: "repo-1", BaseBranch: "main", CheckoutBranch: "feature/a"}, + {RepositoryID: "repo-1", BaseBranch: "main", CheckoutBranch: "feature/b"}, + }, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + + rows, err := repo.ListTaskRepositories(ctx, task.ID) + if err != nil || len(rows) != 2 { + t.Fatalf("ListTaskRepositories: %v, rows=%d", err, len(rows)) + } + var sibling string + for _, r := range rows { + if r.CheckoutBranch == "feature/b" { + sibling = r.ID + } + } + if sibling == "" { + t.Fatal("no task_repositories row for feature/b") + } + + if _, err := svc.UpdateRepositoryBaseBranch(ctx, UpdateRepositoryBaseBranchRequest{ + TaskID: task.ID, + TaskRepositoryID: sibling, + BaseBranch: "staging", + }); err != nil { + t.Fatalf("UpdateRepositoryBaseBranch: %v", err) + } + + calls := pusher.snapshot() + if len(calls) != 1 { + t.Fatalf("expected 1 pusher call, got %d", len(calls)) + } + got := calls[0].branches + + // feature/a is position 0, so it keeps the flat directory. + if got["frontend"] != "main" { + t.Errorf("branches[frontend] = %q, want main (flat sibling keeps its base)", got["frontend"]) + } + // feature/b is the suffixed sibling and is the row we just updated. + if got["frontend-feature-b"] != "staging" { + t.Errorf("branches[frontend-feature-b] = %q, want staging", got["frontend-feature-b"]) + } + // Multi-row tasks must not publish the empty-key fallback: lookupBaseBranch + // falls back to it for any unmatched tracker, which would hand one sibling's + // base branch to the other. + if _, ok := got[""]; ok { + t.Errorf("branches[\"\"] present for a multi-row task: %+v", got) + } +} + +// The map keys are read by WorkspaceTracker, whose repositoryName is the +// worktree *directory* basename — a sanitised repo name, not the raw +// Repository.Name. A name with characters the directory sanitiser rewrites +// (e.g. a space) produced a key no tracker could ever match. +func TestCollectTaskBaseBranches_SanitizesRepositoryName(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + setBranchUpdateWorkflowStep(svc) + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-1", WorkspaceID: "ws-1", Name: "my repo", DefaultBranch: "main"}) + _ = repo.CreateRepository(ctx, &models.Repository{ID: "repo-2", WorkspaceID: "ws-1", Name: "backend", DefaultBranch: "main"}) + + task, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", WorkflowID: "wf-1", WorkflowStepID: "step-1", Title: "Unsanitised name", + Repositories: []TaskRepositoryInput{ + {RepositoryID: "repo-1", BaseBranch: "main", CheckoutBranch: "feature/a"}, + {RepositoryID: "repo-2", BaseBranch: "develop", CheckoutBranch: "feature/b"}, + }, + }) + if err != nil { + t.Fatalf("CreateTask: %v", err) + } + + branches, err := svc.collectTaskBaseBranches(ctx, task.ID) + if err != nil { + t.Fatalf("collectTaskBaseBranches: %v", err) + } + if branches["my-repo"] != "main" { + t.Errorf("branches[my-repo] = %q, want main; got map %+v", branches["my-repo"], branches) + } + if _, ok := branches["my repo"]; ok { + t.Errorf("raw repository name leaked as a key: %+v", branches) + } + if branches["backend"] != "develop" { + t.Errorf("branches[backend] = %q, want develop", branches["backend"]) + } +} diff --git a/docs/plans/workspace-base-branch-propagation/plan.md b/docs/plans/workspace-base-branch-propagation/plan.md new file mode 100644 index 0000000000..51c7674521 --- /dev/null +++ b/docs/plans/workspace-base-branch-propagation/plan.md @@ -0,0 +1,140 @@ +--- +spec: docs/specs/workspace-base-branch-propagation/spec.md +created: 2026-08-05 +status: complete +--- + +# Implementation Plan: Workspace Base-Branch Propagation + +## Overview + +Two backend changes. Task 01 makes the stored base-branch map reach agentctl on +every workspace, not only full launches, by pushing it from a single chokepoint +once the agentctl client is available. Task 02 makes the integration-branch +fallback observable so this class of defect cannot recur silently. + +No schema, API, or persistence change. Backend only; no frontend work — the card +renders whatever stat the backend reports. + +## Confirmed root cause + +`MetadataKeyBaseBranches` is produced in exactly one place — +`collectBaseBranches` at +`internal/agent/runtime/lifecycle/manager_launch.go:173`, on the full +`LaunchAgent` path. The only other producer is +`Service.UpdateRepositoryBaseBranch` → +`Manager.PushBaseBranchesForTask`, which fires only when a user edits the base +branch. + +`startAgentOnExistingWorkspace` (`internal/orchestrator/executor/executor_execute.go`) +and the workspace-only / lazy-recovery execution creation that runs after a +backend restart both bypass `LaunchAgent`. Their agentctl instances therefore +receive no base-branch map, so `WorkspaceTracker.BaseBranch()` is empty, +`resolveBaseBranch` skips `resolveStoredRef`, and the tracker falls through to +`branchDiffCandidates` (`origin/main`, `origin/master`, `main`, `master`). +`computeBaseCommit` then returns the merge-base against whichever candidate +resolves first. + +Reproduced exactly against a repository whose integration line is neither `main` +nor `master`: + +| Quantity | Value | +|---|---| +| Configured base / true merge-base | the task's recorded feature base → correct SHA | +| True diff | `+66 −1` | +| `git merge-base HEAD origin/master` | an ancient commit | +| `git diff --shortstat ` | `2156 files changed, 440466 insertions, 28354 deletions` | +| `+` untracked additions (2 files, 7 + 72 lines) | **+440545** | +| Displayed on the card | **+440545 −28354** | + +A sibling task in the same repository whose workspace *was* created through the +full launch path displayed correctly (`+542 −32`), and a second affected task +shared the identical `−28354` because it resolved to the same fallback base. + +## Backend + +### Task 01 — push the stored base branches on every workspace + +The pieces already exist and are wired: + +- `Service.collectTaskBaseBranches(ctx, taskID)` + (`internal/task/service/service_branch_update.go`) hydrates the per-repo map + from `task_repositories` + `repositories`, including the single-repo empty-key + fallback. It is the DB-time mirror of `collectBaseBranches`. +- `Manager.PushBaseBranchesForTask(ctx, taskID, branches)` + (`internal/agent/runtime/lifecycle/manager_base_branches.go`) pushes to every + running execution of the task and is already idempotent. +- `services.Task.SetAgentBaseBranchPusher(lifecycleMgr)` + (`internal/backendapp/main.go:439`) already connects them. + +What is missing is a caller on the non-launch paths. Add a service entry point +that pairs hydrate + push, and invoke it from the chokepoint where an agentctl +client becomes available for an execution, so every creation path is covered by +one call rather than each path remembering. + +Prefer the single chokepoint in the lifecycle manager (where the agentctl client +is attached / the workspace stream connects) over sprinkling calls into +`startAgentOnExistingWorkspace` and each recovery site. If the lifecycle manager +cannot reach the task service directly, follow the existing callback style in +this codebase (`SetOnAgentStartFailed`, `SetMCPIdentityScoper`, +`SetAgentBaseBranchPusher`) and inject a base-branch **provider** the manager +calls at attach time. + +Keep the push best-effort: a failure is logged at warn and never fails the +workspace, matching `PushBaseBranchesForTask`'s existing contract. + +### Task 02 — make the fallback observable + +`resolveBaseBranch` currently falls through silently. Log the fallback once per +resolution with the repository name and the candidate chosen, so a wrong base +shows up in the logs instead of only as a suspicious number on a card. + +Keep it cheap — this runs on a status poll. A single log line at the point of +fallback is sufficient; do not add a DB read to determine whether a base branch +"should" have been present. + +## Frontend + +None. The card renders the backend-reported stat. + +## Waves + +| Wave | Task | Parallel-safe | +|---|---|---| +| 1 | 01 — push stored base branches on every workspace | no | +| 2 | 02 — log integration-branch fallback | no | + +`parallelism: sequential` on both. Task 02 is independent in principle but +touches the same resolution path Task 01's test exercises, so keep it ordered. + +## Validation + +```bash +(cd apps/backend && go test ./internal/agent/runtime/lifecycle/... ./internal/agentctl/server/process/... ./internal/task/service/... -race) +``` + +```bash +(cd apps/backend && golangci-lint run ./... --new-from-rev=origin/main --timeout=5m) +``` + +## Risks + +- `PushBaseBranchesForTask` iterates every execution of the task. Calling it + from a per-execution attach point means O(executions) pushes per attach. For + the common one-or-two-execution task this is negligible, but prefer a + per-execution push if the chokepoint makes that natural. +- The tracker's map is keyed by repository name matching the worktree directory + basename. `collectTaskBaseBranches` already encodes that mapping including the + single-repo empty-key fallback — reuse it rather than re-deriving the key, or + multi-repo tasks will silently miss. +- Existing coverage lives in + `internal/agentctl/server/process/workspace_git_status_base_branch_test.go` + and `internal/agent/runtime/lifecycle/base_branches_metadata_test.go`; the new + regression test should join those rather than starting a third pattern. + +## Out of scope + +Carried from the spec: the integration candidate list stays as-is, untracked-file +folding is unchanged, `correctStaleComparisonBase` is unchanged, and no +backfill/recompute of existing workspaces (the value is not persisted and +refreshes on the next poll). diff --git a/docs/plans/workspace-base-branch-propagation/task-01-push-base-branches-every-workspace.md b/docs/plans/workspace-base-branch-propagation/task-01-push-base-branches-every-workspace.md new file mode 100644 index 0000000000..bfb32f3baf --- /dev/null +++ b/docs/plans/workspace-base-branch-propagation/task-01-push-base-branches-every-workspace.md @@ -0,0 +1,95 @@ +--- +id: "01-push-base-branches-every-workspace" +title: "Push stored base branches to every workspace" +status: done +wave: 1 +parallelism: sequential +depends_on: [] +plan: "plan.md" +spec: "../../specs/workspace-base-branch-propagation/spec.md" +--- + +# Task 01: Push stored base branches to every workspace + +## Acceptance + +- The stored per-repo base-branch map reaches agentctl's `WorkspaceTracker` for + workspaces created outside the full `LaunchAgent` path — specifically + `startAgentOnExistingWorkspace` and workspace-only / lazy-recovery execution + creation after a backend restart. +- The map is hydrated with `Service.collectTaskBaseBranches`, so the repository + key derivation (including the single-repo empty-key fallback) is shared with + the edit-time path rather than re-derived. +- The push happens at one chokepoint covering every creation path, not as a + separate call bolted onto each site. +- The push is best-effort: failure is logged at warn, never fails workspace + creation or agent start. +- Re-pushing an unchanged map is a no-op (`SetBaseBranches` is already + idempotent). +- The full-launch path keeps working unchanged — `collectBaseBranches` in + `manager_launch.go` is not removed or bypassed. + +## Regression test + +Pin the **propagation**, not the git arithmetic — asserting on diff numbers +would require a fixture repo and would not fail for the right reason. + +- **Red first.** Drive the existing-workspace / recovery execution path with a + task whose `task_repositories.base_branch` is set, and assert that the + agentctl client received a `SetBaseBranches` call carrying that map. Before + the fix, zero calls are made. +- Cover the multi-repo key shape and the single-repo empty-key fallback, since + a wrong key silently reproduces the bug for multi-repo tasks. +- Assert the push is not fatal when the client returns an error. +- Join the existing patterns in + `internal/agent/runtime/lifecycle/base_branches_metadata_test.go`; do not + start a third harness. + +## Verification + +```bash +(cd apps/backend && go test ./internal/agent/runtime/lifecycle/... ./internal/task/service/... -race) +``` + +PASS — `ok .../internal/task/service 40.100s`, `ok .../internal/agent/runtime/lifecycle 22.957s`, +`ok .../internal/agent/runtime/lifecycle/skill 3.889s`. + +```bash +(cd apps/backend && go test ./internal/agentctl/server/process/... ./internal/orchestrator/... -race) +``` + +PASS — `ok .../internal/agentctl/server/process 90.732s`; every `./internal/orchestrator/...` +package reported `ok` or had no test files. + +## Files Likely Touched + +- `apps/backend/internal/agent/runtime/lifecycle/manager_base_branches.go` +- `apps/backend/internal/task/service/service_branch_update.go` +- `apps/backend/internal/agent/runtime/lifecycle/base_branches_metadata_test.go` +- one of `internal/agent/runtime/lifecycle/` (attach chokepoint) or + `internal/orchestrator/executor/executor_execute.go`, depending on where the + chokepoint lands +- `apps/backend/internal/backendapp/main.go` if a provider callback is wired + +## Dependencies + +None. + +## Inputs + +- Spec scenarios 1, 2, 4 and the multi-repo failure mode. +- `collectBaseBranches` (`manager_launch.go:173`) — the launch-time shape to + mirror. +- `Service.collectTaskBaseBranches` — the DB-time hydrator to reuse. +- `Manager.PushBaseBranchesForTask` and `Client.SetBaseBranches` — the existing + push, already idempotent and best-effort. +- Existing wiring at `internal/backendapp/main.go:439`. + +## Output Contract + +The provider is wired before lifecycle startup, so recovered executions and +agentctl-ready executions are seeded from the task's recorded base-branch map. +For workspaces created outside the full launch path, agentctl may publish one +fallback-based status update before the readiness-time push lands; the push +then refreshes the tracker and subsequent stats use the recorded base. No +signature change to `PushBaseBranchesForTask` or `SetBaseBranches`. diff --git a/docs/plans/workspace-base-branch-propagation/task-02-log-base-branch-fallback.md b/docs/plans/workspace-base-branch-propagation/task-02-log-base-branch-fallback.md new file mode 100644 index 0000000000..f5d8cc1366 --- /dev/null +++ b/docs/plans/workspace-base-branch-propagation/task-02-log-base-branch-fallback.md @@ -0,0 +1,76 @@ +--- +id: "02-log-base-branch-fallback" +title: "Make the integration-branch fallback observable" +status: done +wave: 2 +parallelism: sequential +depends_on: ["01-push-base-branches-every-workspace"] +plan: "plan.md" +spec: "../../specs/workspace-base-branch-propagation/spec.md" +--- + +# Task 02: Make the integration-branch fallback observable + +## Acceptance + +- When `resolveBaseBranch` falls through to `branchDiffCandidates`, it emits one + log line naming the repository (tracker's repository name) and the candidate + it selected. +- The line distinguishes the two reasons it can fall through: no stored base + branch was present, versus a stored base branch that failed to verify in git. + Those have different fixes, so they must be told apart from the log alone. +- No DB read is added to decide whether a base branch "should" have been + present — the tracker stays a pure git/state component. +- The line is cheap enough for a status poll and does not spam once per second + at info level; pick a level and/or de-duplication consistent with the + surrounding poll logging. +- No behavior change: the resolution result is identical, only observability is + added. + +## Regression test + +- Assert the fallback path is reached and reports the selected candidate when no + stored base branch is set. +- Assert the stored-but-unverifiable case is distinguishable from the + never-stored case. +- Extend `internal/agentctl/server/process/workspace_git_status_base_branch_test.go`, + which already covers stored-vs-fallback resolution. + +## Verification + +Recorded against `4b52037` (the merge-base with `main` at the time of the run), +which is the revision passed to `--new-from-rev`. + +```bash +(cd apps/backend && go test ./internal/agentctl/server/process/... -race) +``` + +PASS — `ok github.com/kandev/kandev/internal/agentctl/server/process 90.732s`. + +```bash +(cd apps/backend && golangci-lint run ./... --new-from-rev=4b52037 --timeout=5m) +``` + +PASS — `0 issues.` + +## Files Likely Touched + +- `apps/backend/internal/agentctl/server/process/workspace_git_status.go` +- `apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go` + +## Dependencies + +Task 01 — same resolution path; keep the edits ordered. + +## Inputs + +- Spec bullet: a silent fallback that yields a wrong-but-plausible number is not + acceptable. +- `resolveBaseBranch` / `resolveStoredRef` + (`workspace_git_status.go`), including the existing `Debug` line in + `computeBaseCommit`'s neighbours for level/tone consistency. + +## Output Contract + +`resolveBaseBranch` behavior is unchanged; a fallback is now attributable from +logs alone. diff --git a/docs/specs/workspace-base-branch-propagation/spec.md b/docs/specs/workspace-base-branch-propagation/spec.md new file mode 100644 index 0000000000..087e4274f1 --- /dev/null +++ b/docs/specs/workspace-base-branch-propagation/spec.md @@ -0,0 +1,120 @@ +--- +status: draft +created: 2026-08-05 +owner: Kandev +--- + +# Workspace Base-Branch Propagation + +## Why + +The task card's branch diff stat (`+N −M`) is meant to show what the task's +branch changed relative to its configured base branch. For workspaces that were +not created through a full agent launch, it instead reports a diff against +`origin/master`, producing numbers that can be four orders of magnitude too +large — and plausible-looking enough that a user may believe the branch really +touched thousands of files. + +## Broken behavior + +The per-repo base-branch map reaches agentctl's `WorkspaceTracker` through +exactly two paths: + +1. `LaunchRequest` metadata, assembled by `collectBaseBranches` and stored under + `MetadataKeyBaseBranches` — the **full launch** path only; and +2. `Manager.PushBaseBranchesForTask`, which fires only from + `Service.UpdateRepositoryBaseBranch` when a user **edits** the base branch. + +Every other way a workspace comes to exist skips both — notably +`startAgentOnExistingWorkspace` (the agent starting on a workspace that was +already prepared) and the workspace-only / lazy-recovery execution creation that +runs after a backend restart. + +`WorkspaceTracker.BaseBranch()` is then empty, so `resolveBaseBranch` never +consults the stored value and falls through to `branchDiffCandidates` +(`origin/main`, `origin/master`, `main`, `master`). In any repository whose +integration line is not `main`/`master`, that resolves to an unrelated branch, +and `computeBaseCommit` returns a merge-base far back in history. The failure is +silent: no error, no warning, just a wrong number. + +Observed in a local run against a repository whose integration branch is neither +`main` nor `master`. The task's configured base was correct in the database, its true diff +was `+66 −1`, and the card showed `+440545 −28354` — reproduced exactly as the +merge-base diff against `origin/master` (`+440466 −28354`) plus the two +untracked files' 79 lines. A sibling task in the same repository, whose +workspace *was* created through the full launch path, displayed correctly. + +## What + +- The stored per-repo base branch reaches the `WorkspaceTracker` for **every** + workspace, regardless of how the workspace came to exist — full launch, agent + start on an already-prepared workspace, or post-restart recovery. +- The branch diff stat is computed against the configured base branch whenever + one is recorded for the task's repository. +- The integration-branch fallback (`origin/main` → `origin/master` → `main` → + `master`) applies only when no base branch is recorded, which remains its + intended purpose. +- When the tracker falls back to an integration candidate, that decision is + observable in the logs, naming the repository and the candidate chosen. A + silent fallback that yields a wrong-but-plausible number is not acceptable. +- Pushing the map is idempotent: re-sending an unchanged map is a no-op, and a + workspace that already has the correct base is not disturbed. + +## Failure modes + +- The push fails (agentctl unreachable, transient error): logged at warn; the + persisted `task_repositories.base_branch` remains the source of truth and the + next workspace creation retries. Stats degrade to the existing fallback rather + than breaking the workspace. +- No base branch is recorded for the repository: unchanged — the existing + integration-branch fallback applies, now with a log line. +- The recorded base branch no longer exists in git: unchanged — `resolveStoredRef` + fails to verify it and the fallback applies, now observable. +- A multi-repo task where only some repositories have a base branch: each + repository resolves independently; the ones with a recorded base use it. + +## Scenarios + +- **GIVEN** a task whose repository has a recorded base branch, **WHEN** the + agent starts on an already-prepared workspace, **THEN** the tracker resolves + the diff base from that recorded branch, not from an integration candidate. +- **GIVEN** the same task, **WHEN** the backend restarts and the execution is + recreated by lazy recovery, **THEN** the recreated workspace still resolves + the recorded base branch. +- **GIVEN** a task whose repository has no recorded base branch, **WHEN** its + workspace is created, **THEN** the integration-branch fallback applies as + before and the chosen candidate is logged. +- **GIVEN** a workspace that already holds the correct base-branch map, + **WHEN** the map is pushed again, **THEN** nothing changes and no error is + raised. +- **GIVEN** a repository whose integration line is neither `main` nor `master`, + **WHEN** a base branch is recorded, **THEN** the reported stat matches + `git diff --numstat ` plus untracked-file + additions. + +## Known gap + +agentctl starts its workspace trackers during **instance creation**, and each +tracker runs one unconditional scan immediately — before the HTTP server the +backend waits on is reachable. The seeding push therefore cannot land before the +very first status computation, so a workspace created outside the launch path +can publish one fallback-based stat before its base branch arrives. + +The window is bounded and self-healing: `SetBaseBranches` stamps the new ref +synchronously and kicks a background `RefreshGitStatus`, which emits a corrected +`GitStatusUpdate` over the same stream. So the wrong value is transient rather +than the indefinite one this repair fixes. + +Closing it entirely means supplying the map at instance-creation time, or gating +tracker polling until hydration completes — a change to the instance-creation +contract, deliberately not bundled here. + +## Out of scope + +- Changing the integration-branch candidate list or making it configurable. The + fallback is correct for its purpose; the defect is that it was reached at all. +- Changing how untracked-file additions are folded into the totals. +- The `correctStaleComparisonBase` re-pointing rule for merged/deleted stacked + parents, which is a separate, intentional behavior. +- Backfilling or recomputing stats for workspaces that already exist; the value + is not persisted and refreshes on the next poll.