fix(backend): seed every workspace with its stored base branch (#2270)
This commit is contained in:
nova28
@@ -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)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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, "")
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
+249
@@ -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())
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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"])
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 <that>` | `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).
|
||||
+95
@@ -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`.
|
||||
@@ -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.
|
||||
@@ -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 <merge-base with the recorded base>` 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.
|
||||
Reference in New Issue
Block a user