From 46678d670348d0425bb14fa0c7758098c9d9e8a6 Mon Sep 17 00:00:00 2001 From: nova28 Date: Mon, 17 Aug 2026 20:32:06 +0000 Subject: [PATCH] fix(workflow): stop agent profile duplication from rebinding synced steps (#2740) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 Co-authored-by: Carlos Florencio --- .../backendapp/agent_profile_matcher_test.go | 247 +++++++++++++++++ apps/backend/internal/backendapp/services.go | 104 ++++++-- .../internal/workflow/models/export.go | 11 +- .../workflow/models/export_review_test.go | 6 +- .../internal/workflow/service/service.go | 25 +- .../internal/workflow/service/service_test.go | 24 +- .../internal/workflow/service/sync_apply.go | 167 +++++++++++- .../workflow/service/sync_apply_test.go | 250 ++++++++++++++++++ .../service/sync_review_profile_test.go | 106 ++++++++ 9 files changed, 900 insertions(+), 40 deletions(-) create mode 100644 apps/backend/internal/backendapp/agent_profile_matcher_test.go create mode 100644 apps/backend/internal/workflow/service/sync_review_profile_test.go diff --git a/apps/backend/internal/backendapp/agent_profile_matcher_test.go b/apps/backend/internal/backendapp/agent_profile_matcher_test.go new file mode 100644 index 0000000000..9d133d87e1 --- /dev/null +++ b/apps/backend/internal/backendapp/agent_profile_matcher_test.go @@ -0,0 +1,247 @@ +package backendapp + +import ( + "context" + "path/filepath" + "testing" + + "github.com/google/uuid" + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + agentsettingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" + "github.com/kandev/kandev/internal/common/logger" +) + +// newMatcherTestRepos opens an isolated SQLite-backed agent settings store for +// buildAgentProfileMatcher tests, along with the raw *sqlx.DB handle so a test +// can force specific CreatedAt values that the store API always overwrites. +func newMatcherTestRepos(t *testing.T) (*Repositories, *sqlx.DB) { + t.Helper() + db, err := sqlx.Open("sqlite3", filepath.Join(t.TempDir(), "matcher.db")) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + if err != nil { + t.Fatalf("new logger: %v", err) + } + repo, cleanup, err := settingsstore.Provide(db, db, log) + if err != nil { + t.Fatalf("settings store: %v", err) + } + t.Cleanup(func() { _ = cleanup() }) + return &Repositories{AgentSettings: repo}, db +} + +func createMatcherTestAgent(t *testing.T, repos *Repositories) string { + t.Helper() + agent := &agentsettingsmodels.Agent{Name: "claude-" + uuid.New().String()} + if err := repos.AgentSettings.CreateAgent(context.Background(), agent); err != nil { + t.Fatalf("CreateAgent: %v", err) + } + return agent.ID +} + +// createMatcherTestProfile creates a profile with the given (agent display +// name, model, mode) triple, applying any overrides after creation. +func createMatcherTestProfile(t *testing.T, repos *Repositories, agentID, displayName, model, mode string) *agentsettingsmodels.AgentProfile { + t.Helper() + p := &agentsettingsmodels.AgentProfile{ + AgentID: agentID, + Name: "Profile " + uuid.New().String(), + AgentDisplayName: displayName, + Model: model, + Mode: mode, + } + if err := repos.AgentSettings.CreateAgentProfile(context.Background(), p); err != nil { + t.Fatalf("CreateAgentProfile: %v", err) + } + return p +} + +// TestBuildAgentProfileMatcher_DuplicateDoesNotStealBinding is the reported +// regression: duplicating a profile produces a byte-identical +// (agent_display_name, model, mode) triple, and the copy's CreatedAt is +// always later than its source. The matcher must keep resolving to the +// original (oldest) profile, not the newer copy. +func TestBuildAgentProfileMatcher_DuplicateDoesNotStealBinding(t *testing.T) { + repos, _ := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + + source := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + copyProfile := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") // the "Copy" + require.True(t, source.CreatedAt.Before(copyProfile.CreatedAt), + "precondition: the copy must be strictly newer than the source, or this test stops proving the regression") + + match := buildAgentProfileMatcher(repos, testLogger(t)) + got := match("Claude", "opus[1m]", "auto", "") + if got != source.ID { + t.Fatalf("matcher returned %q, want source profile %q", got, source.ID) + } +} + +func TestBuildAgentProfileMatcher_SkipsDisabledProfile(t *testing.T) { + repos, _ := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + + p := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + if _, err := repos.AgentSettings.UpdateAgentProfileEnabled(context.Background(), p.ID, false); err != nil { + t.Fatalf("disable profile: %v", err) + } + + match := buildAgentProfileMatcher(repos, testLogger(t)) + got := match("Claude", "opus[1m]", "auto", "") + if got != "" { + t.Fatalf("matcher returned %q, want empty (disabled profile must never match)", got) + } +} + +// TestBuildAgentProfileMatcher_PreservesDisabledExistingBinding is the +// reconciliation regression: a workflow step already bound to a profile that +// gets disabled after the fact must keep that binding on the next sync, per +// docs/specs/agents/profile-disable.md ("existing sessions/workflows keep +// their profile"). Excluding disabled profiles from *candidate selection* +// must not also strip a binding that was already in place. +func TestBuildAgentProfileMatcher_PreservesDisabledExistingBinding(t *testing.T) { + repos, _ := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + + p := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + if _, err := repos.AgentSettings.UpdateAgentProfileEnabled(context.Background(), p.ID, false); err != nil { + t.Fatalf("disable profile: %v", err) + } + + match := buildAgentProfileMatcher(repos, testLogger(t)) + got := match("Claude", "opus[1m]", "auto", p.ID) + if got != p.ID { + t.Fatalf("matcher returned %q, want existing binding %q preserved despite disablement", got, p.ID) + } +} + +// TestBuildAgentProfileMatcher_CurrentIDDriftFallsBackToCandidateSelection +// covers the case where the currently-bound profile no longer matches the +// descriptor (e.g. the profile itself was edited) - the matcher must not +// blindly trust currentID, it must fall through to normal candidate +// selection. +func TestBuildAgentProfileMatcher_CurrentIDDriftFallsBackToCandidateSelection(t *testing.T) { + repos, _ := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + + stale := createMatcherTestProfile(t, repos, agentID, "Claude", "sonnet[1m]", "auto") + fresh := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + + match := buildAgentProfileMatcher(repos, testLogger(t)) + got := match("Claude", "opus[1m]", "auto", stale.ID) + if got != fresh.ID { + t.Fatalf("matcher returned %q, want %q (currentID no longer matches the descriptor)", got, fresh.ID) + } +} + +func TestBuildAgentProfileMatcher_SkipsWorkspaceScopedProfile(t *testing.T) { + repos, db := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + + p := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + if _, err := db.Exec(`UPDATE agent_profiles SET workspace_id = ? WHERE id = ?`, "workspace-1", p.ID); err != nil { + t.Fatalf("set workspace_id: %v", err) + } + + match := buildAgentProfileMatcher(repos, testLogger(t)) + got := match("Claude", "opus[1m]", "auto", "") + if got != "" { + t.Fatalf("matcher returned %q, want empty (workspace-scoped profile must never match)", got) + } +} + +func TestBuildAgentProfileMatcher_SingleMatchUnchanged(t *testing.T) { + repos, _ := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + + p := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + + match := buildAgentProfileMatcher(repos, testLogger(t)) + got := match("Claude", "opus[1m]", "auto", "") + if got != p.ID { + t.Fatalf("matcher returned %q, want %q", got, p.ID) + } +} + +func TestBuildAgentProfileMatcher_NoMatch(t *testing.T) { + repos, _ := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + + match := buildAgentProfileMatcher(repos, testLogger(t)) + got := match("Claude", "sonnet[1m]", "auto", "") + if got != "" { + t.Fatalf("matcher returned %q, want empty", got) + } +} + +// TestBuildAgentProfileMatcher_EqualCreatedAtIsStable forces two candidates +// to share an identical CreatedAt (which the store API's time.Now() calls +// would otherwise make astronomically unlikely) and asserts the ID tiebreak +// makes the result deterministic across repeated calls. +func TestBuildAgentProfileMatcher_EqualCreatedAtIsStable(t *testing.T) { + repos, db := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + + a := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + b := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + if _, err := db.Exec(`UPDATE agent_profiles SET created_at = (SELECT created_at FROM agent_profiles WHERE id = ?) WHERE id = ?`, a.ID, b.ID); err != nil { + t.Fatalf("force equal created_at: %v", err) + } + + want := a.ID + if b.ID < a.ID { + want = b.ID + } + + match := buildAgentProfileMatcher(repos, testLogger(t)) + for i := 0; i < 5; i++ { + got := match("Claude", "opus[1m]", "auto", "") + if got != want { + t.Fatalf("call %d: matcher returned %q, want stable %q", i, got, want) + } + } +} + +// TestBuildAgentProfileMatcher_MultipleCandidatesLogsDebugWithFields proves +// the ambiguity signal described in buildAgentProfileMatcher's doc comment: +// when more than one candidate matches, it logs at Debug with the triple, +// the candidate count, and the profile it actually selected. +func TestBuildAgentProfileMatcher_MultipleCandidatesLogsDebugWithFields(t *testing.T) { + repos, _ := newMatcherTestRepos(t) + agentID := createMatcherTestAgent(t, repos) + + a := createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") + createMatcherTestProfile(t, repos, agentID, "Claude", "opus[1m]", "auto") // newer candidate, must not be selected + + core, logs := observer.New(zapcore.DebugLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + match := buildAgentProfileMatcher(repos, log) + got := match("Claude", "opus[1m]", "auto", "") + if got != a.ID { + t.Fatalf("matcher returned %q, want oldest profile %q", got, a.ID) + } + + entries := logs.FilterMessage("agent profile matcher: multiple candidates for workflow step sync").All() + require.Len(t, entries, 1, "an ambiguous match must log exactly one Debug entry") + fields := entries[0].ContextMap() + assert.Equal(t, "Claude", fields["agent_display_name"]) + assert.Equal(t, "opus[1m]", fields["model"]) + assert.Equal(t, "auto", fields["mode"]) + assert.Equal(t, int64(2), fields["candidates"]) + assert.Equal(t, a.ID, fields["selected_profile_id"]) +} diff --git a/apps/backend/internal/backendapp/services.go b/apps/backend/internal/backendapp/services.go index 09a35b202d..806d76592a 100644 --- a/apps/backend/internal/backendapp/services.go +++ b/apps/backend/internal/backendapp/services.go @@ -17,6 +17,7 @@ import ( "github.com/kandev/kandev/internal/agent/managedruntime" "github.com/kandev/kandev/internal/agent/registry" agentsettingscontroller "github.com/kandev/kandev/internal/agent/settings/controller" + agentsettingsmodels "github.com/kandev/kandev/internal/agent/settings/models" analyticsservice "github.com/kandev/kandev/internal/analytics/service" "github.com/kandev/kandev/internal/automation" "github.com/kandev/kandev/internal/azuredevops" @@ -153,7 +154,7 @@ func provideServices(cfg *config.Config, log *logger.Logger, repos *Repositories // Wire agent profile resolver/matcher for workflow export/import workflowSvc.SetAgentProfileFuncs( buildAgentProfileResolver(repos), - buildAgentProfileMatcher(repos), + buildAgentProfileMatcher(repos, log), ) githubSvc := initGitHubService(cfg, dbPool, eventBus, repos.Secrets, log) @@ -1301,26 +1302,93 @@ func buildAgentProfileResolver(repos *Repositories) wfmodels.AgentProfileResolve } } -// buildAgentProfileMatcher creates a matcher that finds profiles by agent name, model, and mode for import. -func buildAgentProfileMatcher(repos *Repositories) wfmodels.AgentProfileMatcher { - return func(agentName, model, mode string) string { - agents, err := repos.AgentSettings.ListAgents(context.Background()) - if err != nil { - return "" - } - for _, agent := range agents { - profiles, pErr := repos.AgentSettings.ListAgentProfiles(context.Background(), agent.ID) - if pErr != nil { - continue - } - for _, p := range profiles { - if p.AgentDisplayName == agentName && p.Model == model && p.Mode == mode { - return p.ID - } - } +// agentProfileStillMatches reports whether the profile with id still has the +// exact (agent_display_name, model, mode) triple, ignoring Enabled and +// WorkspaceID - those only gate candidate *selection*, not a binding that +// already exists. +func agentProfileStillMatches(repos *Repositories, id, agentName, model, mode string) bool { + p, err := repos.AgentSettings.GetAgentProfile(context.Background(), id) + if err != nil || p == nil { + return false + } + return p.AgentDisplayName == agentName && p.Model == model && p.Mode == mode +} + +// buildAgentProfileMatcher creates a matcher that finds profiles by agent +// name, model, and mode for import. +// +// The (agent_display_name, model, mode) triple is not unique: duplicating a +// profile through the UI produces a byte-identical triple for the copy. +// Candidates are filtered to enabled, global (non-workspace-scoped) profiles, +// and ties are broken by oldest CreatedAt (then ID for a total order). Oldest +// wins so that duplicating a profile can never steal an existing synced +// workflow step's binding - a copy is always newer than its source. +// +// currentID, when non-empty, is checked first: if that profile still has the +// exact descriptor, it's kept as-is without re-running candidate selection - +// even when it's disabled or workspace-scoped. Reconciliation must not treat +// "profile got disabled" as "profile needs a new binding" (profile-disable.md +// promises existing bindings survive disabling); candidate selection only +// applies when picking a profile for new work. +func buildAgentProfileMatcher(repos *Repositories, log *logger.Logger) wfmodels.AgentProfileMatcher { + return func(agentName, model, mode, currentID string) string { + if currentID != "" && agentProfileStillMatches(repos, currentID, agentName, model, mode) { + return currentID } + return selectAgentProfileCandidate(repos, log, agentName, model, mode) + } +} + +// selectAgentProfileCandidate scans enabled, global profiles for an exact +// (agent_display_name, model, mode) match and returns the oldest one (see +// buildAgentProfileMatcher), logging when the triple was ambiguous. +func selectAgentProfileCandidate(repos *Repositories, log *logger.Logger, agentName, model, mode string) string { + agents, err := repos.AgentSettings.ListAgents(context.Background()) + if err != nil { return "" } + var best *agentsettingsmodels.AgentProfile + matches := 0 + for _, agent := range agents { + profiles, pErr := repos.AgentSettings.ListAgentProfiles(context.Background(), agent.ID) + if pErr != nil { + continue + } + for _, p := range profiles { + if p.AgentDisplayName != agentName || p.Model != model || p.Mode != mode { + continue + } + if !p.Enabled || p.WorkspaceID != "" { + continue + } + matches++ + if best == nil || isOlderAgentProfileMatch(p, best) { + best = p + } + } + } + if best == nil { + return "" + } + if matches > 1 { + log.Debug("agent profile matcher: multiple candidates for workflow step sync", + zap.String("agent_display_name", agentName), + zap.String("model", model), + zap.String("mode", mode), + zap.Int("candidates", matches), + zap.String("selected_profile_id", best.ID)) + } + return best.ID +} + +// isOlderAgentProfileMatch reports whether candidate should replace current +// as the matcher's selection: earlier CreatedAt wins, ties broken by ID so +// the result is stable across repeated calls. +func isOlderAgentProfileMatch(candidate, current *agentsettingsmodels.AgentProfile) bool { + if !candidate.CreatedAt.Equal(current.CreatedAt) { + return candidate.CreatedAt.Before(current.CreatedAt) + } + return candidate.ID < current.ID } // codeHostBranchListerAdapter routes first-party GitHub repositories directly diff --git a/apps/backend/internal/workflow/models/export.go b/apps/backend/internal/workflow/models/export.go index a9e0f27a99..a18b151e4f 100644 --- a/apps/backend/internal/workflow/models/export.go +++ b/apps/backend/internal/workflow/models/export.go @@ -29,8 +29,13 @@ type AgentProfilePortable struct { // AgentProfileResolver resolves an agent profile ID to its portable representation. type AgentProfileResolver func(profileID string) *AgentProfilePortable -// AgentProfileMatcher finds a matching agent profile ID by agent name, model, and mode. -type AgentProfileMatcher func(agentName, model, mode string) string +// AgentProfileMatcher finds a matching agent profile ID by agent name, model, +// and mode. currentID, when non-empty, is the profile already bound (e.g. +// during workflow-sync reconciliation); implementations should keep it when +// it still matches the descriptor even if the profile was since disabled - +// disabling only hides a profile from new selection, it doesn't touch +// existing bindings (docs/specs/agents/profile-disable.md). +type AgentProfileMatcher func(agentName, model, mode, currentID string) string // WorkflowPortable is a workflow without instance-specific fields (IDs, timestamps). type WorkflowPortable struct { @@ -322,7 +327,7 @@ func ConvertReviewProfileToID(events StepEvents, matchProfile AgentProfileMatche agentName, _ := descriptor["agent_name"].(string) model, _ := descriptor["model"].(string) mode, _ := descriptor["mode"].(string) - profileID := matchProfile(agentName, model, mode) + profileID := matchProfile(agentName, model, mode, "") if profileID == "" { return nil, false } diff --git a/apps/backend/internal/workflow/models/export_review_test.go b/apps/backend/internal/workflow/models/export_review_test.go index 1e6c1ff157..2553be9b9f 100644 --- a/apps/backend/internal/workflow/models/export_review_test.go +++ b/apps/backend/internal/workflow/models/export_review_test.go @@ -81,7 +81,7 @@ func TestConvertReviewProfileToPortable_NilResolverAndEmptyConfig(t *testing.T) } func TestConvertReviewProfileToID_MatchesLocalProfile(t *testing.T) { - match := func(agentName, model, mode string) string { + match := func(agentName, model, mode, _ string) string { if agentName == "codex" && model == "gpt-5.3" && mode == "review" { return "profile-imported" } @@ -106,7 +106,7 @@ func TestConvertReviewProfileToID_MatchesLocalProfile(t *testing.T) { func TestConvertReviewProfileToID_DropsUnmatchedDescriptor(t *testing.T) { events := ConvertReviewProfileToID(reviewStepEvents(map[string]any{ ReviewAgentProfilePortableKey: map[string]any{"agent_name": "unknown"}, - }), func(string, string, string) string { return "" }) + }), func(string, string, string, string) string { return "" }) config := reviewActionConfig(t, events) if _, present := config[ReviewAgentProfileConfigKey]; present { @@ -121,7 +121,7 @@ func TestReviewProfileRoundTripAcrossWorkspaces(t *testing.T) { resolve := func(string) *AgentProfilePortable { return &AgentProfilePortable{AgentName: "claude", Model: "haiku"} } - match := func(agentName, model, _ string) string { + match := func(agentName, model, _, _ string) string { if agentName == "claude" && model == "haiku" { return "profile-in-target-workspace" } diff --git a/apps/backend/internal/workflow/service/service.go b/apps/backend/internal/workflow/service/service.go index 521b817d42..17c66e37e8 100644 --- a/apps/backend/internal/workflow/service/service.go +++ b/apps/backend/internal/workflow/service/service.go @@ -706,7 +706,7 @@ func (s *Service) importSingleWorkflow(ctx context.Context, workspaceID string, // any step. This keeps imports atomic with respect to validation failures. steps := make([]*models.WorkflowStep, 0, len(pw.Steps)) for _, sp := range pw.Steps { - step := s.stepFromPortable("pending-workflow", sp, posToID) + step := s.stepFromPortable("pending-workflow", sp, posToID, "") if err := models.ValidateWorkflowStep(step); err != nil { return nil, fmt.Errorf("validate step %q: %w", sp.Name, err) } @@ -721,7 +721,7 @@ func (s *Service) importSingleWorkflow(ctx context.Context, workspaceID string, needsUpdate := false // Match workflow-level agent profile if present. if pw.AgentProfile != nil && s.matchProfile != nil { - if profileID := s.matchProfile(pw.AgentProfile.AgentName, pw.AgentProfile.Model, pw.AgentProfile.Mode); profileID != "" { + if profileID := s.matchProfile(pw.AgentProfile.AgentName, pw.AgentProfile.Model, pw.AgentProfile.Mode, ""); profileID != "" { wf.AgentProfileID = profileID needsUpdate = true } @@ -747,8 +747,19 @@ func (s *Service) importSingleWorkflow(ctx context.Context, workspaceID string, // stepFromPortable builds a WorkflowStep from its portable form, remapping // position-based references to the step IDs in posToID and matching the -// step-level agent profile when a matcher is wired. -func (s *Service) stepFromPortable(workflowID string, sp models.StepPortable, posToID map[int]string) *models.WorkflowStep { +// step-level agent profile when a matcher is wired. existingProfileID is the +// profile already bound to this step (by name) before sync, or "" for a step +// with no prior binding (including at import time) - it lets the matcher +// preserve a disabled-but-still-matching binding instead of treating +// disablement as a rebind. +func (s *Service) stepFromPortable(workflowID string, sp models.StepPortable, posToID map[int]string, existingProfileID string) *models.WorkflowStep { + return s.stepFromPortableWithMatcher(workflowID, sp, posToID, s.matchProfile, existingProfileID) +} + +// stepFromPortableWithMatcher lets workflow sync use a matcher that preserves +// profile IDs embedded in existing review actions while imports use the normal +// matcher directly. +func (s *Service) stepFromPortableWithMatcher(workflowID string, sp models.StepPortable, posToID map[int]string, matchProfile models.AgentProfileMatcher, existingProfileID string) *models.WorkflowStep { step := &models.WorkflowStep{ ID: posToID[sp.Position], WorkflowID: workflowID, @@ -756,7 +767,7 @@ func (s *Service) stepFromPortable(workflowID string, sp models.StepPortable, po Position: sp.Position, Color: sp.Color, Prompt: sp.Prompt, - Events: models.ConvertReviewProfileToID(models.ConvertPositionToStepID(sp.Events, posToID), s.matchProfile), + Events: models.ConvertReviewProfileToID(models.ConvertPositionToStepID(sp.Events, posToID), matchProfile), IsStartStep: sp.IsStartStep, ShowInCommandPanel: sp.ShowInCommandPanel, AllowManualMove: sp.AllowManualMove, @@ -766,8 +777,8 @@ func (s *Service) stepFromPortable(workflowID string, sp models.StepPortable, po WIPLimit: sp.WIPLimit, PullFromStepID: sp.PullFromStepID(posToID), } - if sp.AgentProfile != nil && s.matchProfile != nil { - step.AgentProfileID = s.matchProfile(sp.AgentProfile.AgentName, sp.AgentProfile.Model, sp.AgentProfile.Mode) + if sp.AgentProfile != nil && matchProfile != nil { + step.AgentProfileID = matchProfile(sp.AgentProfile.AgentName, sp.AgentProfile.Model, sp.AgentProfile.Mode, existingProfileID) } return step } diff --git a/apps/backend/internal/workflow/service/service_test.go b/apps/backend/internal/workflow/service/service_test.go index 16eada5ec6..3a734380b4 100644 --- a/apps/backend/internal/workflow/service/service_test.go +++ b/apps/backend/internal/workflow/service/service_test.go @@ -19,7 +19,10 @@ import ( "github.com/kandev/kandev/internal/workflow/repository" ) -func setupTestService(t *testing.T) (*Service, *sqlx.DB) { +// setupTestService builds a Service against an in-memory SQLite DB. Callers +// needing to observe log output (e.g. via a zaptest observer core) may pass a +// logOverride; otherwise a quiet default logger is used. +func setupTestService(t *testing.T, logOverride ...*logger.Logger) (*Service, *sqlx.DB) { rawDB, err := sql.Open("sqlite3", ":memory:") require.NoError(t, err) // Pin the pool to one connection: every connection to an in-memory SQLite @@ -41,8 +44,14 @@ func setupTestService(t *testing.T) (*Service, *sqlx.DB) { repo, err := repository.NewWithDB(sqlxDB, sqlxDB, nil) require.NoError(t, err) - log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) - svc := NewService(repo, log) + log := logOverride + var svcLogger *logger.Logger + if len(log) > 0 { + svcLogger = log[0] + } else { + svcLogger, _ = logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + } + svc := NewService(repo, svcLogger) t.Cleanup(func() { _ = svc.Close() }) return svc, sqlxDB } @@ -57,6 +66,10 @@ func insertWorkflow(t *testing.T, db *sqlx.DB, id, name string) { type mockWorkflowProvider struct { workflows []*taskmodels.Workflow getWorkflowCalls int + // forceUpdateWorkflowErr, when set, makes UpdateWorkflow fail without + // mutating state - used to test that a rebind Warn never fires for a + // change that was not actually persisted. + forceUpdateWorkflowErr error } func (m *mockWorkflowProvider) ListWorkflows(_ context.Context, workspaceID string, includeHidden bool) ([]*taskmodels.Workflow, error) { @@ -98,6 +111,9 @@ func (m *mockWorkflowProvider) CreateWorkflow(_ context.Context, workspaceID, na } func (m *mockWorkflowProvider) UpdateWorkflow(_ context.Context, workflow *taskmodels.Workflow) error { + if m.forceUpdateWorkflowErr != nil { + return m.forceUpdateWorkflowErr + } for i, wf := range m.workflows { if wf.ID == workflow.ID { m.workflows[i] = workflow @@ -919,7 +935,7 @@ func TestImportWorkflows(t *testing.T) { svc, _, _ := setupTestServiceWithProvider(t) ctx := context.Background() - matcher := func(agentName, model, mode string) string { + matcher := func(agentName, model, mode, _ string) string { if agentName == "Claude Code" && model == "opus" { return "matched-prof-1" } diff --git a/apps/backend/internal/workflow/service/sync_apply.go b/apps/backend/internal/workflow/service/sync_apply.go index a38d6ed3d4..934acacaff 100644 --- a/apps/backend/internal/workflow/service/sync_apply.go +++ b/apps/backend/internal/workflow/service/sync_apply.go @@ -160,7 +160,7 @@ func (s *Service) updateSyncedWorkflow(ctx context.Context, wf *taskmodels.Workf // persisting any step. A malformed later step must leave the synced workflow // untouched so the next sync can retry after the source is fixed. for _, sp := range pw.Steps { - step := s.stepFromPortable(wf.ID, sp, posToID) + step := s.stepFromPortableForSync(wf.ID, sp, posToID, existingByName[sp.Name]) if err := models.ValidateWorkflowStep(step); err != nil { result.Warnings = append(result.Warnings, fmt.Sprintf("workflow %q: invalid step %q: %v", wf.Name, sp.Name, err)) return @@ -173,13 +173,21 @@ func (s *Service) updateSyncedWorkflow(ctx context.Context, wf *taskmodels.Workf return } for _, sp := range pw.Steps { - step := s.stepFromPortable(wf.ID, sp, posToID) - if existing, ok := existingByName[sp.Name]; ok { + existing := existingByName[sp.Name] + step := s.stepFromPortableForSync(wf.ID, sp, posToID, existing) + var rebinding bool + var oldProfileID, existingID string + var reviewRebindings []profileRebinding + if existing != nil { step.CreatedAt = existing.CreatedAt step.StageType = existing.StageType // not carried by the portable format if stepMatchesDefinition(existing, step) { continue } + rebinding = existing.AgentProfileID != "" && existing.AgentProfileID != step.AgentProfileID + oldProfileID = existing.AgentProfileID + existingID = existing.ID + reviewRebindings = findReviewProfileRebindings(existing.Events, step.Events) err = s.repo.UpdateStep(ctx, step) } else { err = s.repo.CreateStep(ctx, step) @@ -188,6 +196,7 @@ func (s *Service) updateSyncedWorkflow(ctx context.Context, wf *taskmodels.Workf result.Warnings = append(result.Warnings, fmt.Sprintf("workflow %q: failed to apply step %q: %v", wf.Name, sp.Name, err)) return } + s.logStepProfileRebindings(wf, sp.Name, existingID, rebinding, oldProfileID, step.AgentProfileID, reviewRebindings) changed = true } for _, st := range toDelete { @@ -202,6 +211,142 @@ func (s *Service) updateSyncedWorkflow(ctx context.Context, wf *taskmodels.Workf } } +func (s *Service) logStepProfileRebindings(wf *taskmodels.Workflow, stepName, stepID string, rebinding bool, oldProfileID, newProfileID string, reviewRebindings []profileRebinding) { + if rebinding { + s.logger.Warn("workflow sync rebinding step's agent profile", + zap.String("workflow_id", wf.ID), + zap.String("workflow_name", wf.Name), + zap.String("step_id", stepID), + zap.String("step_name", stepName), + zap.String("old_agent_profile_id", oldProfileID), + zap.String("new_agent_profile_id", newProfileID)) + } + for _, reviewRebinding := range reviewRebindings { + s.logger.Warn("workflow sync rebinding step's review agent profile", + zap.String("workflow_id", wf.ID), + zap.String("workflow_name", wf.Name), + zap.String("step_id", stepID), + zap.String("step_name", stepName), + zap.String("old_agent_profile_id", reviewRebinding.oldID), + zap.String("new_agent_profile_id", reviewRebinding.newID)) + } +} + +type profileRebinding struct { + oldID string + newID string +} + +func (s *Service) stepFromPortableForSync(workflowID string, sp models.StepPortable, posToID map[int]string, existing *models.WorkflowStep) *models.WorkflowStep { + existingProfileID := "" + if existing != nil { + existingProfileID = existing.AgentProfileID + } + var matchProfile models.AgentProfileMatcher + if s.matchProfile != nil { + matchProfile = s.reviewProfileMatcherForSync(existing) + } + return s.stepFromPortableWithMatcher(workflowID, sp, posToID, matchProfile, existingProfileID) +} + +func (s *Service) reviewProfileMatcherForSync(existing *models.WorkflowStep) models.AgentProfileMatcher { + preserved := make(map[string][]string) + if existing != nil && s.resolveProfile != nil { + for _, action := range existing.Events.OnEnter { + if action.Type != models.OnEnterRunCodeReview || action.Config == nil { + continue + } + profileID, ok := action.Config[models.ReviewAgentProfileConfigKey].(string) + if !ok || profileID == "" { + continue + } + if portable := s.resolveProfile(profileID); portable != nil { + key := portableProfileKey(portable.AgentName, portable.Model, portable.Mode) + preserved[key] = append(preserved[key], profileID) + } + } + } + return func(agentName, model, mode, currentID string) string { + if currentID != "" { + return s.matchProfile(agentName, model, mode, currentID) + } + key := portableProfileKey(agentName, model, mode) + if candidates := preserved[key]; len(candidates) > 0 { + profileID := candidates[0] + preserved[key] = candidates[1:] + return profileID + } + if s.matchProfile == nil { + return "" + } + return s.matchProfile(agentName, model, mode, "") + } +} + +func portableProfileKey(agentName, model, mode string) string { + return agentName + "\x00" + model + "\x00" + mode +} + +func findReviewProfileRebindings(existing, desired models.StepEvents) []profileRebinding { + oldIDs := reviewProfileIDs(existing) + newIDs := reviewProfileIDs(desired) + newCounts := make(map[string]int, len(newIDs)) + for _, profileID := range newIDs { + if profileID != "" { + newCounts[profileID]++ + } + } + var unmatchedOld []string + for _, profileID := range oldIDs { + if profileID == "" { + continue + } + if newCounts[profileID] > 0 { + newCounts[profileID]-- + continue + } + unmatchedOld = append(unmatchedOld, profileID) + } + oldCounts := make(map[string]int, len(oldIDs)) + for _, profileID := range oldIDs { + if profileID != "" { + oldCounts[profileID]++ + } + } + var unmatchedNew []string + for _, profileID := range newIDs { + if profileID == "" { + continue + } + if oldCounts[profileID] > 0 { + oldCounts[profileID]-- + continue + } + unmatchedNew = append(unmatchedNew, profileID) + } + var rebindings []profileRebinding + for i, oldID := range unmatchedOld { + newID := "" + if i < len(unmatchedNew) { + newID = unmatchedNew[i] + } + rebindings = append(rebindings, profileRebinding{oldID: oldID, newID: newID}) + } + return rebindings +} + +func reviewProfileIDs(events models.StepEvents) []string { + var profileIDs []string + for _, action := range events.OnEnter { + if action.Type != models.OnEnterRunCodeReview || action.Config == nil { + continue + } + profileID, _ := action.Config[models.ReviewAgentProfileConfigKey].(string) + profileIDs = append(profileIDs, profileID) + } + return profileIDs +} + // stepMatchesDefinition reports whether an existing step already equals its // desired definition (timestamps and stage type aside — stage type is copied // from the existing step). Events are compared via their JSON encoding to @@ -268,15 +413,27 @@ func (s *Service) planStepChanges(ctx context.Context, wf *taskmodels.Workflow, func (s *Service) applyWorkflowFields(ctx context.Context, wf *taskmodels.Workflow, pw models.WorkflowPortable) (bool, error) { profileID := "" if pw.AgentProfile != nil && s.matchProfile != nil { - profileID = s.matchProfile(pw.AgentProfile.AgentName, pw.AgentProfile.Model, pw.AgentProfile.Mode) + profileID = s.matchProfile(pw.AgentProfile.AgentName, pw.AgentProfile.Model, pw.AgentProfile.Mode, wf.AgentProfileID) } if wf.Description == pw.Description && wf.Prompt == pw.Prompt && wf.AgentProfileID == profileID { return false, nil } + oldProfileID := wf.AgentProfileID + rebinding := oldProfileID != "" && oldProfileID != profileID wf.Description = pw.Description wf.Prompt = pw.Prompt wf.AgentProfileID = profileID - return true, s.workflowProvider.UpdateWorkflow(ctx, wf) + if err := s.workflowProvider.UpdateWorkflow(ctx, wf); err != nil { + return true, err + } + if rebinding { + s.logger.Warn("workflow sync rebinding workflow's agent profile", + zap.String("workflow_id", wf.ID), + zap.String("workflow_name", wf.Name), + zap.String("old_agent_profile_id", oldProfileID), + zap.String("new_agent_profile_id", profileID)) + } + return true, nil } // ReleaseSyncedWorkflows converts every GitHub-sourced workflow in the diff --git a/apps/backend/internal/workflow/service/sync_apply_test.go b/apps/backend/internal/workflow/service/sync_apply_test.go index 1884dda982..63bfb54f05 100644 --- a/apps/backend/internal/workflow/service/sync_apply_test.go +++ b/apps/backend/internal/workflow/service/sync_apply_test.go @@ -7,7 +7,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + "github.com/kandev/kandev/internal/common/logger" taskmodels "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/workflow/models" ) @@ -66,6 +70,17 @@ func setupSyncService(t *testing.T) (*Service, *mockWorkflowProvider, *mockSyncO return svc, provider, ops } +// setupSyncServiceWithLogger is a variant of setupSyncService for tests that +// need to observe log output (e.g. via a zaptest observer core). +func setupSyncServiceWithLogger(t *testing.T, log *logger.Logger) (*Service, *mockWorkflowProvider, *mockSyncOps) { + svc, _ := setupTestService(t, log) + provider := &mockWorkflowProvider{} + svc.SetWorkflowProvider(provider) + ops := newMockSyncOps(provider) + svc.SetSyncWorkflowOps(ops) + return svc, provider, ops +} + func portableWorkflow(name string, stepNames ...string) models.WorkflowPortable { steps := make([]models.StepPortable, 0, len(stepNames)) for i, sn := range stepNames { @@ -492,3 +507,238 @@ func TestReleaseSyncedWorkflows(t *testing.T) { assert.NoError(t, svc.EnsureWorkflowMutable(ctx, id), "released workflows are editable again") } } + +// TestApplySyncedWorkflows_RebindingStepAgentProfileLogsWarning covers the +// visibility half of the profile-rebinding defect: a sync round that changes +// an existing step's AgentProfileID must warn (naming the workflow, step, +// and both profile IDs) since the workflow is read-only to the user and the +// change would otherwise be silent. A second sync that resolves to the same +// profile must not add another warning. +func TestApplySyncedWorkflows_RebindingStepAgentProfileLogsWarning(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + svc, provider, _ := setupSyncServiceWithLogger(t, log) + ctx := context.Background() + wf := addSyncedWorkflow(provider, "wf-1", "ws-1", "Dev Flow", "flows/dev.yml") + createStep(t, svc, &models.WorkflowStep{ + ID: "step-todo", WorkflowID: wf.ID, Name: "Todo", Position: 0, IsStartStep: true, + AgentProfileID: "profile-old", + }) + + pw := portableWorkflow("Dev Flow", "Todo") + pw.Steps[0].AgentProfile = &models.AgentProfilePortable{AgentName: "Claude", Model: "opus[1m]", Mode: "auto"} + files := []SyncFileExport{{Path: "flows/dev.yml", Export: exportOf(pw)}} + + svc.SetAgentProfileFuncs(nil, func(string, string, string, string) string { return "profile-new" }) + result, err := svc.ApplySyncedWorkflows(ctx, "ws-1", files) + require.NoError(t, err) + assert.Equal(t, []string{"Dev Flow"}, result.Updated) + + entries := logs.FilterMessage("workflow sync rebinding step's agent profile").All() + require.Len(t, entries, 1, "rebind must be logged exactly once") + fields := entries[0].ContextMap() + assert.Equal(t, "step-todo", fields["step_id"]) + assert.Equal(t, "Todo", fields["step_name"]) + assert.Equal(t, "profile-old", fields["old_agent_profile_id"]) + assert.Equal(t, "profile-new", fields["new_agent_profile_id"]) + + // A second sync resolving to the same profile must not warn again. + result, err = svc.ApplySyncedWorkflows(ctx, "ws-1", files) + require.NoError(t, err) + assert.Empty(t, result.Updated, "no drift means nothing is rewritten") + assert.Len(t, logs.FilterMessage("workflow sync rebinding step's agent profile").All(), 1, + "an unchanged profile must not log another warning") +} + +// TestApplySyncedWorkflows_RebindWarningDoesNotFireOnFailedUpdate covers +// R1-F1: the rebind Warn must only fire once the change is actually +// persisted. It forces the step's UpdateStep write to fail (via PRAGMA +// query_only, which makes every write on this connection fail while reads — +// like the ListStepsByWorkflow that already ran — keep working) and asserts +// no Warn is logged for a rebind that never landed. +func TestApplySyncedWorkflows_RebindWarningDoesNotFireOnFailedUpdate(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + svc, db := setupTestService(t, log) + provider := &mockWorkflowProvider{} + svc.SetWorkflowProvider(provider) + ops := newMockSyncOps(provider) + svc.SetSyncWorkflowOps(ops) + + ctx := context.Background() + wf := addSyncedWorkflow(provider, "wf-1", "ws-1", "Dev Flow", "flows/dev.yml") + createStep(t, svc, &models.WorkflowStep{ + ID: "step-todo", WorkflowID: wf.ID, Name: "Todo", Position: 0, IsStartStep: true, + AgentProfileID: "profile-old", + }) + + pw := portableWorkflow("Dev Flow", "Todo") + pw.Steps[0].AgentProfile = &models.AgentProfilePortable{AgentName: "Claude", Model: "opus[1m]", Mode: "auto"} + files := []SyncFileExport{{Path: "flows/dev.yml", Export: exportOf(pw)}} + svc.SetAgentProfileFuncs(nil, func(string, string, string, string) string { return "profile-new" }) + + _, err = db.Exec("PRAGMA query_only = ON") + t.Cleanup(func() { _, _ = db.Exec("PRAGMA query_only = OFF") }) + require.NoError(t, err) + + result, err := svc.ApplySyncedWorkflows(ctx, "ws-1", files) + require.NoError(t, err) + require.Len(t, result.Warnings, 1, "the failed step write surfaces a warning") + assert.Contains(t, result.Warnings[0], "Todo") + assert.Empty(t, result.Updated, "a step update that failed to persist must not be reported as applied") + assert.Empty(t, logs.FilterMessage("workflow sync rebinding step's agent profile").All(), + "a rebind Warn must never fire for a change that was not actually persisted") +} + +// TestApplySyncedWorkflows_RebindingWorkflowAgentProfileLogsWarning covers +// R1-F2: the workflow-level AgentProfileID (the fallback steps with no +// profile of their own resolve to) is rebound by the same matcher and must +// be logged just like a step-level rebind. +func TestApplySyncedWorkflows_RebindingWorkflowAgentProfileLogsWarning(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + svc, provider, _ := setupSyncServiceWithLogger(t, log) + ctx := context.Background() + wf := addSyncedWorkflow(provider, "wf-1", "ws-1", "Dev Flow", "flows/dev.yml") + wf.AgentProfileID = "profile-old" + createStep(t, svc, &models.WorkflowStep{ID: "step-todo", WorkflowID: wf.ID, Name: "Todo", Position: 0, IsStartStep: true}) + + pw := portableWorkflow("Dev Flow", "Todo") + pw.AgentProfile = &models.AgentProfilePortable{AgentName: "Claude", Model: "opus[1m]", Mode: "auto"} + files := []SyncFileExport{{Path: "flows/dev.yml", Export: exportOf(pw)}} + + svc.SetAgentProfileFuncs(nil, func(string, string, string, string) string { return "profile-new" }) + result, err := svc.ApplySyncedWorkflows(ctx, "ws-1", files) + require.NoError(t, err) + assert.Equal(t, []string{"Dev Flow"}, result.Updated) + + got, err := provider.GetWorkflow(ctx, wf.ID) + require.NoError(t, err) + assert.Equal(t, "profile-new", got.AgentProfileID) + + entries := logs.FilterMessage("workflow sync rebinding workflow's agent profile").All() + require.Len(t, entries, 1, "workflow-level rebind must be logged exactly once") + fields := entries[0].ContextMap() + assert.Equal(t, "wf-1", fields["workflow_id"]) + assert.Equal(t, "Dev Flow", fields["workflow_name"]) + assert.Equal(t, "profile-old", fields["old_agent_profile_id"]) + assert.Equal(t, "profile-new", fields["new_agent_profile_id"]) + + // A second sync resolving to the same profile must not warn again. + result, err = svc.ApplySyncedWorkflows(ctx, "ws-1", files) + require.NoError(t, err) + assert.Empty(t, result.Updated, "no drift means nothing is rewritten") + assert.Len(t, logs.FilterMessage("workflow sync rebinding workflow's agent profile").All(), 1, + "an unchanged profile must not log another warning") +} + +// TestApplySyncedWorkflows_WorkflowRebindWarningDoesNotFireOnFailedUpdate is +// the workflow-level counterpart to +// TestApplySyncedWorkflows_RebindWarningDoesNotFireOnFailedUpdate (R1-F1): a +// rebind Warn must never fire for a workflow-level profile change that failed +// to persist. +func TestApplySyncedWorkflows_WorkflowRebindWarningDoesNotFireOnFailedUpdate(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + svc, provider, _ := setupSyncServiceWithLogger(t, log) + ctx := context.Background() + wf := addSyncedWorkflow(provider, "wf-1", "ws-1", "Dev Flow", "flows/dev.yml") + wf.AgentProfileID = "profile-old" + createStep(t, svc, &models.WorkflowStep{ID: "step-todo", WorkflowID: wf.ID, Name: "Todo", Position: 0, IsStartStep: true}) + + pw := portableWorkflow("Dev Flow", "Todo") + pw.AgentProfile = &models.AgentProfilePortable{AgentName: "Claude", Model: "opus[1m]", Mode: "auto"} + files := []SyncFileExport{{Path: "flows/dev.yml", Export: exportOf(pw)}} + svc.SetAgentProfileFuncs(nil, func(string, string, string, string) string { return "profile-new" }) + + provider.forceUpdateWorkflowErr = fmt.Errorf("simulated UpdateWorkflow failure") + + result, err := svc.ApplySyncedWorkflows(ctx, "ws-1", files) + require.NoError(t, err) + require.Len(t, result.Warnings, 1, "the failed workflow write surfaces a warning") + assert.Contains(t, result.Warnings[0], "Dev Flow") + assert.Empty(t, result.Updated, "a workflow update that failed to persist must not be reported as applied") + assert.Empty(t, logs.FilterMessage("workflow sync rebinding workflow's agent profile").All(), + "a rebind Warn must never fire for a workflow change that was not actually persisted") +} + +// TestApplySyncedWorkflows_StepMatcherReceivesCurrentBinding proves +// updateSyncedWorkflow passes the step's existing AgentProfileID to the +// matcher as currentID. A real matcher (buildAgentProfileMatcher) uses that +// to preserve a binding that was disabled after the fact instead of treating +// disablement as a rebind (docs/specs/agents/profile-disable.md). This test +// stubs that same contract: the matcher echoes currentID back when given +// one, standing in for "the bound profile still matches, keep it". +func TestApplySyncedWorkflows_StepMatcherReceivesCurrentBinding(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + svc, provider, _ := setupSyncServiceWithLogger(t, log) + ctx := context.Background() + wf := addSyncedWorkflow(provider, "wf-1", "ws-1", "Dev Flow", "flows/dev.yml") + wf.Description = "synced Dev Flow" // must already match pw's description below, or the workflow itself counts as changed + createStep(t, svc, &models.WorkflowStep{ + ID: "step-todo", WorkflowID: wf.ID, Name: "Todo", Position: 0, Color: "#aabbcc", IsStartStep: true, + AgentProfileID: "profile-disabled", + }) + + pw := portableWorkflow("Dev Flow", "Todo") + pw.Steps[0].AgentProfile = &models.AgentProfilePortable{AgentName: "Claude", Model: "opus[1m]", Mode: "auto"} + files := []SyncFileExport{{Path: "flows/dev.yml", Export: exportOf(pw)}} + + svc.SetAgentProfileFuncs(nil, func(_, _, _, currentID string) string { + if currentID != "" { + return currentID + } + return "profile-new" + }) + result, err := svc.ApplySyncedWorkflows(ctx, "ws-1", files) + require.NoError(t, err) + assert.Empty(t, result.Updated, "a preserved binding must not be reported as a change") + assert.Empty(t, logs.FilterMessage("workflow sync rebinding step's agent profile").All(), + "a preserved binding must not log a rebind") +} + +// TestApplySyncedWorkflows_StepProfileUnbindWarnsWithEmptyNewID covers a +// genuine unbind: the matcher finds no candidate at all (not the +// still-matches-but-disabled case above), so the step's AgentProfileID +// resolves to "". The rebind Warn must still fire and its +// new_agent_profile_id field must be observably empty, so a reader of the +// log can tell the step lost its profile rather than gained a different one. +func TestApplySyncedWorkflows_StepProfileUnbindWarnsWithEmptyNewID(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + svc, provider, _ := setupSyncServiceWithLogger(t, log) + ctx := context.Background() + wf := addSyncedWorkflow(provider, "wf-1", "ws-1", "Dev Flow", "flows/dev.yml") + createStep(t, svc, &models.WorkflowStep{ + ID: "step-todo", WorkflowID: wf.ID, Name: "Todo", Position: 0, IsStartStep: true, + AgentProfileID: "profile-old", + }) + + pw := portableWorkflow("Dev Flow", "Todo") + pw.Steps[0].AgentProfile = &models.AgentProfilePortable{AgentName: "Claude", Model: "opus[1m]", Mode: "auto"} + files := []SyncFileExport{{Path: "flows/dev.yml", Export: exportOf(pw)}} + + svc.SetAgentProfileFuncs(nil, func(string, string, string, string) string { return "" }) + result, err := svc.ApplySyncedWorkflows(ctx, "ws-1", files) + require.NoError(t, err) + assert.Equal(t, []string{"Dev Flow"}, result.Updated) + + entries := logs.FilterMessage("workflow sync rebinding step's agent profile").All() + require.Len(t, entries, 1, "an unbind must still be logged") + fields := entries[0].ContextMap() + assert.Equal(t, "profile-old", fields["old_agent_profile_id"]) + assert.Equal(t, "", fields["new_agent_profile_id"]) +} diff --git a/apps/backend/internal/workflow/service/sync_review_profile_test.go b/apps/backend/internal/workflow/service/sync_review_profile_test.go new file mode 100644 index 0000000000..d622ecdc59 --- /dev/null +++ b/apps/backend/internal/workflow/service/sync_review_profile_test.go @@ -0,0 +1,106 @@ +package service + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/workflow/models" +) + +func TestApplySyncedWorkflows_PreservesCurrentReviewAgentProfileBinding(t *testing.T) { + svc, provider, _ := setupSyncService(t) + ctx := context.Background() + wf := addSyncedWorkflow(provider, "wf-1", "ws-1", "Dev Flow", "flows/dev.yml") + wf.Description = "synced Dev Flow" + createStep(t, svc, &models.WorkflowStep{ + ID: "step-todo", WorkflowID: wf.ID, Name: "Todo", Position: 0, Color: "#aabbcc", IsStartStep: true, + Events: models.StepEvents{OnEnter: []models.OnEnterAction{{ + Type: models.OnEnterRunCodeReview, + Config: map[string]interface{}{models.ReviewAgentProfileConfigKey: "profile-current"}, + }}}, + }) + + pw := portableWorkflow("Dev Flow", "Todo") + pw.Steps[0].Events = models.StepEvents{OnEnter: []models.OnEnterAction{{ + Type: models.OnEnterRunCodeReview, + Config: map[string]interface{}{models.ReviewAgentProfilePortableKey: map[string]interface{}{ + "agent_name": "Claude", "model": "opus[1m]", "mode": "auto", + }}, + }}} + svc.SetAgentProfileFuncs(func(id string) *models.AgentProfilePortable { + if id == "profile-current" { + return &models.AgentProfilePortable{AgentName: "Claude", Model: "opus[1m]", Mode: "auto"} + } + return nil + }, func(string, string, string, string) string { return "profile-oldest" }) + + result, err := svc.ApplySyncedWorkflows(ctx, "ws-1", []SyncFileExport{{ + Path: "flows/dev.yml", Export: exportOf(pw), + }}) + require.NoError(t, err) + assert.Empty(t, result.Updated, "an existing exact review binding must win over a global candidate") + + steps, err := svc.ListStepsByWorkflow(ctx, wf.ID) + require.NoError(t, err) + require.Len(t, steps, 1) + require.Len(t, steps[0].Events.OnEnter, 1) + assert.Equal(t, "profile-current", steps[0].Events.OnEnter[0].Config[models.ReviewAgentProfileConfigKey]) +} + +func TestApplySyncedWorkflows_LogsReviewAgentProfileRebinding(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + svc, provider, _ := setupSyncServiceWithLogger(t, log) + ctx := context.Background() + wf := addSyncedWorkflow(provider, "wf-1", "ws-1", "Dev Flow", "flows/dev.yml") + wf.Description = "synced Dev Flow" + createStep(t, svc, &models.WorkflowStep{ + ID: "step-todo", WorkflowID: wf.ID, Name: "Todo", Position: 0, Color: "#aabbcc", IsStartStep: true, + Events: models.StepEvents{OnEnter: []models.OnEnterAction{{ + Type: models.OnEnterRunCodeReview, + Config: map[string]interface{}{models.ReviewAgentProfileConfigKey: "profile-old"}, + }}}, + }) + + pw := portableWorkflow("Dev Flow", "Todo") + pw.Steps[0].Events = models.StepEvents{OnEnter: []models.OnEnterAction{{ + Type: models.OnEnterRunCodeReview, + Config: map[string]interface{}{models.ReviewAgentProfilePortableKey: map[string]interface{}{ + "agent_name": "Claude", "model": "opus[1m]", "mode": "auto", + }}, + }}} + svc.SetAgentProfileFuncs(nil, func(string, string, string, string) string { return "profile-new" }) + + result, err := svc.ApplySyncedWorkflows(ctx, "ws-1", []SyncFileExport{{ + Path: "flows/dev.yml", Export: exportOf(pw), + }}) + require.NoError(t, err) + assert.Equal(t, []string{"Dev Flow"}, result.Updated) + + entries := logs.FilterMessage("workflow sync rebinding step's review agent profile").All() + require.Len(t, entries, 1) + assert.Equal(t, "profile-old", entries[0].ContextMap()["old_agent_profile_id"]) + assert.Equal(t, "profile-new", entries[0].ContextMap()["new_agent_profile_id"]) +} + +func TestFindReviewProfileRebindingsIgnoresActionOrder(t *testing.T) { + existing := models.StepEvents{OnEnter: []models.OnEnterAction{ + {Type: models.OnEnterRunCodeReview, Config: map[string]interface{}{models.ReviewAgentProfileConfigKey: "profile-a"}}, + {Type: models.OnEnterRunCodeReview, Config: map[string]interface{}{models.ReviewAgentProfileConfigKey: "profile-b"}}, + }} + desired := models.StepEvents{OnEnter: []models.OnEnterAction{ + {Type: models.OnEnterRunCodeReview, Config: map[string]interface{}{models.ReviewAgentProfileConfigKey: "profile-b"}}, + {Type: models.OnEnterRunCodeReview, Config: map[string]interface{}{models.ReviewAgentProfileConfigKey: "profile-a"}}, + }} + + assert.Empty(t, findReviewProfileRebindings(existing, desired)) +}