From aac2afbf9319dfd6de4322ba6edbbc73504a8f11 Mon Sep 17 00:00:00 2001 From: Nicolas Lenepveu Date: Mon, 8 Jun 2026 18:49:34 +0200 Subject: [PATCH] fix(agent-settings): stop reseeding agent profiles the user deleted (#1305) --- .../lifecycle/profile_resolver_test.go | 8 ++ .../settings/controller/agent_discovery.go | 12 ++ .../agent/settings/controller/reconciler.go | 23 +++ .../settings/controller/reconciler_test.go | 134 +++++++++++++++++- .../internal/agent/settings/store/sqlite.go | 15 ++ .../settings/store/sqlite_soft_delete_test.go | 46 ++++++ .../internal/agent/settings/store/store.go | 6 + 7 files changed, 239 insertions(+), 5 deletions(-) diff --git a/apps/backend/internal/agent/runtime/lifecycle/profile_resolver_test.go b/apps/backend/internal/agent/runtime/lifecycle/profile_resolver_test.go index 7d5d7224a1..3e54e554c5 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/profile_resolver_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/profile_resolver_test.go @@ -23,6 +23,7 @@ type MockRepository struct { GetAgentProfileIncludingDeletedFn func(ctx context.Context, id string) (*models.AgentProfile, error) ListAgentsFn func(ctx context.Context) ([]*models.Agent, error) ListAgentProfilesFn func(ctx context.Context, agentID string) ([]*models.AgentProfile, error) + HasDeletedAgentProfilesFn func(ctx context.Context, agentID string) (bool, error) } var _ store.Repository = (*MockRepository)(nil) @@ -101,6 +102,13 @@ func (m *MockRepository) ListAgentProfiles(ctx context.Context, agentID string) return []*models.AgentProfile{}, nil } +func (m *MockRepository) HasDeletedAgentProfiles(ctx context.Context, agentID string) (bool, error) { + if m.HasDeletedAgentProfilesFn != nil { + return m.HasDeletedAgentProfilesFn(ctx, agentID) + } + return false, nil +} + func (m *MockRepository) ListTUIAgents(ctx context.Context) ([]*models.Agent, error) { return []*models.Agent{}, nil } diff --git a/apps/backend/internal/agent/settings/controller/agent_discovery.go b/apps/backend/internal/agent/settings/controller/agent_discovery.go index 0cb96678d7..d1f3d1159c 100644 --- a/apps/backend/internal/agent/settings/controller/agent_discovery.go +++ b/apps/backend/internal/agent/settings/controller/agent_discovery.go @@ -325,6 +325,18 @@ func (c *Controller) syncAgentFromDiscovery(ctx context.Context, result discover if len(profiles) > 0 { return c.updateExistingProfiles(ctx, profiles, p) } + // No live profiles. Only seed a default for an agent that has never been + // provisioned. If soft-deleted rows exist the user deliberately removed + // the profile(s); recreating one here would resurrect it on every restart. + // (A soft-deleted row implies a user deletion, not system orphan cleanup — + // see ProfileReconciler.reconcileAgent for the disjoint-enabled-set reasoning.) + hadProfiles, err := c.repo.HasDeletedAgentProfiles(ctx, agent.ID) + if err != nil { + return err + } + if hadProfiles { + return nil + } return c.createDefaultProfile(ctx, agent.ID, p) } diff --git a/apps/backend/internal/agent/settings/controller/reconciler.go b/apps/backend/internal/agent/settings/controller/reconciler.go index 04654f3936..ab3264ebfc 100644 --- a/apps/backend/internal/agent/settings/controller/reconciler.go +++ b/apps/backend/internal/agent/settings/controller/reconciler.go @@ -146,6 +146,29 @@ func (r *ProfileReconciler) reconcileAgent(ctx context.Context, ag agents.Agent) } if len(profiles) == 0 { + // Only seed for an agent that has never been provisioned. Soft-deleted + // rows mean the profile(s) were deliberately removed, so re-seeding + // would resurrect them on every boot (the bug this guards). + // + // A soft-deleted row here implies a *user* deletion, not system orphan + // cleanup: cleanupOrphans is the only system path that soft-deletes + // profiles, and it acts solely on agents absent from ListEnabled(), + // whereas reconcileAgent runs only for ListInferenceAgents(). Both gate + // on Enabled(), so the sets are disjoint — an enabled, reconciled agent + // is never orphan-cleaned. (If Enabled() ever becomes dynamic, revisit: + // a re-enabled agent reuses its DB id via ensureDBAgent and would then + // see its orphan-cleaned rows here.) + hadProfiles, err := r.store.HasDeletedAgentProfiles(ctx, dbAgent.ID) + if err != nil { + r.log.Warn("reconcile: check deleted profiles failed", + zap.String("agent_id", agentType), zap.Error(err)) + return + } + if hadProfiles { + r.log.Debug("skipping seed: agent has user-deleted profiles", + zap.String("agent_id", agentType)) + return + } r.seedDefaultProfile(ctx, ag, dbAgent, caps) return } diff --git a/apps/backend/internal/agent/settings/controller/reconciler_test.go b/apps/backend/internal/agent/settings/controller/reconciler_test.go index 645464d719..413258060e 100644 --- a/apps/backend/internal/agent/settings/controller/reconciler_test.go +++ b/apps/backend/internal/agent/settings/controller/reconciler_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/kandev/kandev/internal/agent/agents" + "github.com/kandev/kandev/internal/agent/discovery" "github.com/kandev/kandev/internal/agent/hostutility" "github.com/kandev/kandev/internal/agent/registry" "github.com/kandev/kandev/internal/agent/settings/models" @@ -27,7 +28,8 @@ func (f *fakeCapReader) Get(agentType string) (hostutility.AgentCapabilities, bo type fakeStore struct { agents map[string]*models.Agent // keyed by DB ID byName map[string]*models.Agent // keyed by Name - profiles map[string][]*models.AgentProfile // keyed by DB agent ID + profiles map[string][]*models.AgentProfile // keyed by DB agent ID (live) + deleted map[string][]*models.AgentProfile // keyed by DB agent ID (soft-deleted) created []*models.AgentProfile updated []*models.AgentProfile softDeleted []string @@ -41,6 +43,7 @@ func newFakeStore() *fakeStore { agents: map[string]*models.Agent{}, byName: map[string]*models.Agent{}, profiles: map[string][]*models.AgentProfile{}, + deleted: map[string][]*models.AgentProfile{}, } } @@ -103,9 +106,27 @@ func (f *fakeStore) UpdateAgentProfile(_ context.Context, p *models.AgentProfile func (f *fakeStore) DeleteAgentProfile(_ context.Context, id string) error { f.softDeleted = append(f.softDeleted, id) + // Faithfully model soft-delete: move the row out of the live list into the + // deleted list so HasDeletedAgentProfiles and GetAgentProfileIncludingDeleted + // stay consistent with the live ListAgentProfiles view. + for agentID, list := range f.profiles { + kept := list[:0:0] + for _, p := range list { + if p.ID == id { + f.deleted[agentID] = append(f.deleted[agentID], p) + continue + } + kept = append(kept, p) + } + f.profiles[agentID] = kept + } return nil } +func (f *fakeStore) HasDeletedAgentProfiles(_ context.Context, agentID string) (bool, error) { + return len(f.deleted[agentID]) > 0, nil +} + func (f *fakeStore) GetAgentProfile(_ context.Context, id string) (*models.AgentProfile, error) { for _, list := range f.profiles { for _, p := range list { @@ -118,10 +139,19 @@ func (f *fakeStore) GetAgentProfile(_ context.Context, id string) (*models.Agent } func (f *fakeStore) GetAgentProfileIncludingDeleted(ctx context.Context, id string) (*models.AgentProfile, error) { - // Delegate to the deleted_at-aware lookup; the fake does not model - // soft-delete state, so both methods see the same rows. This keeps - // nil dereferences out of any test that calls the new method. - return f.GetAgentProfile(ctx, id) + if p, err := f.GetAgentProfile(ctx, id); err == nil { + return p, nil + } + // Fall back to the soft-deleted set so this method, unlike GetAgentProfile, + // surfaces rows that DeleteAgentProfile moved out of the live list. + for _, list := range f.deleted { + for _, p := range list { + if p.ID == id { + return p, nil + } + } + } + return nil, sql.ErrNoRows } func (f *fakeStore) ListAgentProfiles(_ context.Context, agentID string) ([]*models.AgentProfile, error) { @@ -214,6 +244,43 @@ func TestProfileReconciler_SeedsDefaultProfile(t *testing.T) { } } +// TestProfileReconciler_DoesNotReseedAfterUserDelete pins the core fix: an +// agent whose only profile the user deleted (zero live rows, but soft-deleted +// rows present) must NOT have a default profile recreated on the next boot. +func TestProfileReconciler_DoesNotReseedAfterUserDelete(t *testing.T) { + st := newFakeStore() + // Agent exists and was provisioned before, but the user deleted every + // profile — modelled by creating a profile and soft-deleting it via the + // real delete path, leaving zero live rows and one soft-deleted row. + dbAgent := &models.Agent{Name: "claude-acp"} + _ = st.CreateAgent(context.Background(), dbAgent) + deletedProfile := &models.AgentProfile{AgentID: dbAgent.ID, Name: "Sonnet 4.6", Model: "claude-sonnet"} + _ = st.CreateAgentProfile(context.Background(), deletedProfile) + _ = st.DeleteAgentProfile(context.Background(), deletedProfile.ID) + st.created = nil // discard setup bookkeeping; measure only what Run() seeds + + ag := &mockInferenceAgent{id: "claude-acp", displayName: "Claude", enabled: true} + caps := &fakeCapReader{ + caps: map[string]hostutility.AgentCapabilities{ + "claude-acp": { + AgentType: "claude-acp", + Status: hostutility.StatusOK, + Models: []hostutility.Model{{ID: "claude-sonnet", Name: "Sonnet"}}, + CurrentModelID: "claude-sonnet", + CurrentModeID: "default", + }, + }, + } + r := newReconciler(t, st, caps, ag) + if err := r.Run(context.Background()); err != nil { + t.Fatalf("Run: %v", err) + } + if len(st.created) != 0 { + t.Fatalf("expected no profile to be recreated after user delete, got %d: %+v", + len(st.created), st.created) + } +} + func TestProfileReconciler_HealsStaleModel(t *testing.T) { st := newFakeStore() // Seed an existing DB agent and profile with a stale model. @@ -278,3 +345,60 @@ func TestProfileReconciler_CleansOrphanProfiles(t *testing.T) { t.Fatalf("expected orphan profile to be soft-deleted, got %v", st.softDeleted) } } + +// newDiscoveryController builds a Controller with just the dependencies that +// syncAgentFromDiscovery touches: the agent registry, the store, and a logger. +func newDiscoveryController(t *testing.T, st *fakeStore, ag agents.Agent) *Controller { + t.Helper() + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("logger: %v", err) + } + reg := registry.NewRegistry(log) + if err := reg.Register(ag); err != nil { + t.Fatalf("register: %v", err) + } + return &Controller{agentRegistry: reg, repo: st, logger: log} +} + +// TestSyncAgentFromDiscovery_DoesNotReseedAfterUserDelete pins the discovery +// seed path — the other half of the fix. An agent whose only profile the user +// deleted must NOT have a default recreated when discovery runs on boot. +func TestSyncAgentFromDiscovery_DoesNotReseedAfterUserDelete(t *testing.T) { + st := newFakeStore() + ag := &mockInferenceAgent{id: "claude-acp", displayName: "Claude", enabled: true} + + dbAgent := &models.Agent{Name: "claude-acp"} + _ = st.CreateAgent(context.Background(), dbAgent) + deletedProfile := &models.AgentProfile{AgentID: dbAgent.ID, Name: "Sonnet 4.6", Model: "claude-sonnet"} + _ = st.CreateAgentProfile(context.Background(), deletedProfile) + _ = st.DeleteAgentProfile(context.Background(), deletedProfile.ID) + st.created = nil // discard setup bookkeeping; measure only what sync seeds + + c := newDiscoveryController(t, st, ag) + result := discovery.Availability{Name: "claude-acp", Available: true} + if err := c.syncAgentFromDiscovery(context.Background(), result); err != nil { + t.Fatalf("syncAgentFromDiscovery: %v", err) + } + if len(st.created) != 0 { + t.Fatalf("expected no profile to be recreated after user delete, got %d: %+v", + len(st.created), st.created) + } +} + +// TestSyncAgentFromDiscovery_SeedsFreshAgent is the positive counterpart: an +// agent that has never been provisioned (no live and no deleted rows) still +// gets its default profile, so the guard does not break new-agent detection. +func TestSyncAgentFromDiscovery_SeedsFreshAgent(t *testing.T) { + st := newFakeStore() + ag := &mockInferenceAgent{id: "claude-acp", displayName: "Claude", enabled: true} + + c := newDiscoveryController(t, st, ag) + result := discovery.Availability{Name: "claude-acp", Available: true} + if err := c.syncAgentFromDiscovery(context.Background(), result); err != nil { + t.Fatalf("syncAgentFromDiscovery: %v", err) + } + if len(st.created) != 1 { + t.Fatalf("expected fresh agent to be seeded, got %d created", len(st.created)) + } +} diff --git a/apps/backend/internal/agent/settings/store/sqlite.go b/apps/backend/internal/agent/settings/store/sqlite.go index 20315040a2..325197fe5d 100644 --- a/apps/backend/internal/agent/settings/store/sqlite.go +++ b/apps/backend/internal/agent/settings/store/sqlite.go @@ -799,6 +799,21 @@ func (r *sqliteRepository) ListAgentProfiles(ctx context.Context, agentID string return result, rows.Err() } +// HasDeletedAgentProfiles reports whether the agent has any soft-deleted +// profile rows (deleted_at IS NOT NULL). It is the "has been provisioned +// before" signal the boot-time seeders consult before recreating a default +// profile, so a profile the user deleted is not silently resurrected. +func (r *sqliteRepository) HasDeletedAgentProfiles(ctx context.Context, agentID string) (bool, error) { + var exists int + err := r.ro.QueryRowContext(ctx, + r.ro.Rebind(`SELECT EXISTS(SELECT 1 FROM agent_profiles WHERE agent_id = ? AND deleted_at IS NOT NULL)`), + agentID).Scan(&exists) + if err != nil { + return false, err + } + return exists == 1, nil +} + // applyLegacyBackfill returns the profile with CLIFlags populated from the // legacy allow_indexing column for Auggie rows that predate the cli_flags // column. The backfill is scoped to auggie only so a legacy Claude/Codex/ diff --git a/apps/backend/internal/agent/settings/store/sqlite_soft_delete_test.go b/apps/backend/internal/agent/settings/store/sqlite_soft_delete_test.go index e637a02708..265f0e2f3c 100644 --- a/apps/backend/internal/agent/settings/store/sqlite_soft_delete_test.go +++ b/apps/backend/internal/agent/settings/store/sqlite_soft_delete_test.go @@ -69,6 +69,52 @@ func TestGetAgentProfileIncludingDeleted_MissingRowStillErrors(t *testing.T) { } } +// TestHasDeletedAgentProfiles pins the "has been provisioned before" signal the +// boot-time seeders rely on: false while a profile is live (or never existed), +// true once the user soft-deletes it. This is what stops a deleted profile from +// being resurrected on the next restart. +func TestHasDeletedAgentProfiles(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + id := seedAgentProfile(t, repo, "kilo-default", "kilocode-acp") + + live, err := repo.GetAgentProfileIncludingDeleted(ctx, id) + if err != nil { + t.Fatalf("lookup profile: %v", err) + } + agentID := live.AgentID + + // Fresh agent with no deleted rows -> false. + has, err := repo.HasDeletedAgentProfiles(ctx, agentID) + if err != nil { + t.Fatalf("HasDeletedAgentProfiles (live): %v", err) + } + if has { + t.Fatal("expected false while the only profile is still live") + } + + // Unknown agent -> false (not an error). + has, err = repo.HasDeletedAgentProfiles(ctx, "agent-that-never-existed") + if err != nil { + t.Fatalf("HasDeletedAgentProfiles (unknown): %v", err) + } + if has { + t.Fatal("expected false for an agent with no profile rows at all") + } + + if err := repo.DeleteAgentProfile(ctx, id); err != nil { + t.Fatalf("soft-delete failed: %v", err) + } + + has, err = repo.HasDeletedAgentProfiles(ctx, agentID) + if err != nil { + t.Fatalf("HasDeletedAgentProfiles (deleted): %v", err) + } + if !has { + t.Fatal("expected true once the agent has a soft-deleted profile") + } +} + // seedAgentProfile creates a parent agent + a profile referencing it and // returns the profile id. Centralised so the table+FK setup stays in one // place even if the schema grows new required columns. diff --git a/apps/backend/internal/agent/settings/store/store.go b/apps/backend/internal/agent/settings/store/store.go index ff974f2b36..7e7cf8af04 100644 --- a/apps/backend/internal/agent/settings/store/store.go +++ b/apps/backend/internal/agent/settings/store/store.go @@ -28,6 +28,12 @@ type Repository interface { // only used by callers of ProfileResolver, which wraps this method. GetAgentProfileIncludingDeleted(ctx context.Context, id string) (*models.AgentProfile, error) ListAgentProfiles(ctx context.Context, agentID string) ([]*models.AgentProfile, error) + // HasDeletedAgentProfiles reports whether the agent has any soft-deleted + // profile rows. Seeding paths use this to distinguish a fresh agent that + // has never been provisioned (no rows at all -> seed a default) from one + // whose profiles the user deliberately deleted (deleted rows present -> + // do not resurrect them on the next boot). + HasDeletedAgentProfiles(ctx context.Context, agentID string) (bool, error) Close() error }