diff --git a/apps/backend/internal/utility/service/service.go b/apps/backend/internal/utility/service/service.go index b5baa0b35f..828ee9f88d 100644 --- a/apps/backend/internal/utility/service/service.go +++ b/apps/backend/internal/utility/service/service.go @@ -76,8 +76,8 @@ func (s *Service) ClearAgentProfileBindings(ctx context.Context, profileID strin // MigrateLegacyBindings upgrades old agent/model selections after profile // reconciliation. The operation is idempotent and leaves explicit bindings -// untouched. An empty unconfigured built-in is also left untouched because an -// older release could have erased the ID of a deleted explicit binding. +// untouched. An empty unconfigured built-in is normalized to inherit because +// no concrete override remains to preserve. func (s *Service) MigrateLegacyBindings(ctx context.Context) (int, error) { if s.profileResolver == nil { return 0, nil @@ -88,9 +88,24 @@ func (s *Service) MigrateLegacyBindings(ctx context.Context) (int, error) { } updated := 0 for _, agent := range agents { + // Skip already-inherited rows and any row that still carries a concrete + // profile ID. The latter includes stale/unconfigured overrides whose + // original intent must remain available for explicit user repair. if agent == nil || agent.AgentProfileID != "" || - agent.ProfileBindingState == models.ProfileBindingInherit || - agent.ProfileBindingState == models.ProfileBindingUnconfigured { + agent.ProfileBindingState == models.ProfileBindingInherit { + continue + } + if agent.ProfileBindingState == models.ProfileBindingUnconfigured { + if !agent.Builtin { + continue + } + changed, err := s.repo.NormalizeEmptyBuiltinBinding(ctx, agent.ID) + if err != nil { + return updated, err + } + if changed { + updated++ + } continue } profile, matchErr := s.profileResolver.MatchLegacy(ctx, agent.AgentID, agent.Model) diff --git a/apps/backend/internal/utility/service/service_test.go b/apps/backend/internal/utility/service/service_test.go index 7f82c2a90a..8a0f2deea2 100644 --- a/apps/backend/internal/utility/service/service_test.go +++ b/apps/backend/internal/utility/service/service_test.go @@ -43,7 +43,7 @@ func TestMigrateLegacyBindingsUpdatesOnlyUnambiguousRows(t *testing.T) { } } -func TestMigrateLegacyBindingsPreservesEmptyUnconfiguredBuiltin(t *testing.T) { +func TestMigrateLegacyBindingsNormalizesEmptyUnconfiguredBuiltin(t *testing.T) { repo := &fakeRepository{agents: map[string]*models.UtilityAgent{ "builtin": { ID: "builtin", @@ -58,11 +58,74 @@ func TestMigrateLegacyBindingsPreservesEmptyUnconfiguredBuiltin(t *testing.T) { if err != nil { t.Fatalf("MigrateLegacyBindings() error = %v", err) } - if updated != 0 { - t.Fatalf("MigrateLegacyBindings() updated = %d, want 0", updated) + if updated != 1 { + t.Fatalf("MigrateLegacyBindings() updated = %d, want 1", updated) } - if got := repo.agents["builtin"].ProfileBindingState; got != models.ProfileBindingUnconfigured { - t.Fatalf("profile binding state = %q, want %q", got, models.ProfileBindingUnconfigured) + if got := repo.agents["builtin"].ProfileBindingState; got != models.ProfileBindingInherit { + t.Fatalf("profile binding state = %q, want %q", got, models.ProfileBindingInherit) + } + if repo.agents["builtin"].Enabled { + t.Fatal("migration enabled the built-in action") + } + + updated, err = svc.MigrateLegacyBindings(context.Background()) + if err != nil || updated != 0 { + t.Fatalf("second MigrateLegacyBindings() = (%d, %v), want (0, nil)", updated, err) + } +} + +func TestMigrateLegacyBindingsDoesNotOverwriteConcurrentRepair(t *testing.T) { + repo := &fakeRepository{agents: map[string]*models.UtilityAgent{ + "builtin": { + ID: "builtin", + Builtin: true, + ProfileBindingState: models.ProfileBindingUnconfigured, + }, + }} + repo.normalizeEmptyBuiltinBindingHook = func(agent *models.UtilityAgent) { + agent.AgentProfileID = "profile-1" + agent.ProfileBindingState = models.ProfileBindingExplicit + } + svc := NewService(repo) + svc.SetProfileResolver(fakeProfileResolver{}) + + updated, err := svc.MigrateLegacyBindings(context.Background()) + if err != nil || updated != 0 { + t.Fatalf("MigrateLegacyBindings() = (%d, %v), want (0, nil)", updated, err) + } + got := repo.agents["builtin"] + if got.AgentProfileID != "profile-1" || got.ProfileBindingState != models.ProfileBindingExplicit { + t.Fatalf("concurrent repair was overwritten: %#v", got) + } +} + +func TestMigrateLegacyBindingsPreservesConcreteAndCustomUnconfiguredBindings(t *testing.T) { + repo := &fakeRepository{agents: map[string]*models.UtilityAgent{ + "builtin": { + ID: "builtin", + Builtin: true, + AgentProfileID: "deleted-profile", + ProfileBindingState: models.ProfileBindingUnconfigured, + }, + "custom": { + ID: "custom", + ProfileBindingState: models.ProfileBindingUnconfigured, + }, + }} + svc := NewService(repo) + svc.SetProfileResolver(fakeProfileResolver{}) + + updated, err := svc.MigrateLegacyBindings(context.Background()) + if err != nil || updated != 0 { + t.Fatalf("MigrateLegacyBindings() = (%d, %v), want (0, nil)", updated, err) + } + for id, agent := range repo.agents { + if agent.ProfileBindingState != models.ProfileBindingUnconfigured { + t.Fatalf("%s binding state = %q, want %q", id, agent.ProfileBindingState, models.ProfileBindingUnconfigured) + } + } + if got := repo.agents["builtin"].AgentProfileID; got != "deleted-profile" { + t.Fatalf("builtin profile ID = %q, want %q", got, "deleted-profile") } } @@ -155,7 +218,8 @@ func TestClearAgentProfileBindingsRetainsStaleProfileID(t *testing.T) { } type fakeRepository struct { - agents map[string]*models.UtilityAgent + agents map[string]*models.UtilityAgent + normalizeEmptyBuiltinBindingHook func(*models.UtilityAgent) } func (r *fakeRepository) ListAgents(context.Context) ([]*models.UtilityAgent, error) { @@ -193,6 +257,19 @@ func (r *fakeRepository) UpdateAgent(_ context.Context, agent *models.UtilityAge return nil } +func (r *fakeRepository) NormalizeEmptyBuiltinBinding(_ context.Context, id string) (bool, error) { + agent := r.agents[id] + if r.normalizeEmptyBuiltinBindingHook != nil { + r.normalizeEmptyBuiltinBindingHook(agent) + } + if agent == nil || !agent.Builtin || agent.AgentProfileID != "" || + agent.ProfileBindingState != models.ProfileBindingUnconfigured { + return false, nil + } + agent.ProfileBindingState = models.ProfileBindingInherit + return true, nil +} + func (r *fakeRepository) DeleteAgent(_ context.Context, id string) error { delete(r.agents, id) return nil diff --git a/apps/backend/internal/utility/store/interface.go b/apps/backend/internal/utility/store/interface.go index 1516471938..901e0032d8 100644 --- a/apps/backend/internal/utility/store/interface.go +++ b/apps/backend/internal/utility/store/interface.go @@ -14,6 +14,9 @@ type Repository interface { GetAgentByName(ctx context.Context, name string) (*models.UtilityAgent, error) CreateAgent(ctx context.Context, agent *models.UtilityAgent) error UpdateAgent(ctx context.Context, agent *models.UtilityAgent) error + // NormalizeEmptyBuiltinBinding changes an empty, unconfigured built-in to + // inherit only while the row still has that exact stale binding. + NormalizeEmptyBuiltinBinding(ctx context.Context, id string) (bool, error) DeleteAgent(ctx context.Context, id string) error // Calls (history) diff --git a/apps/backend/internal/utility/store/sqlite.go b/apps/backend/internal/utility/store/sqlite.go index cfe9bbbc78..6411a491f8 100644 --- a/apps/backend/internal/utility/store/sqlite.go +++ b/apps/backend/internal/utility/store/sqlite.go @@ -277,6 +277,22 @@ func (r *sqliteRepository) UpdateAgent(ctx context.Context, agent *models.Utilit return err } +func (r *sqliteRepository) NormalizeEmptyBuiltinBinding(ctx context.Context, id string) (bool, error) { + result, err := r.db.ExecContext(ctx, r.db.Rebind(` + UPDATE utility_agents + SET profile_binding_state = ?, updated_at = ? + WHERE id = ? AND builtin = 1 AND agent_profile_id = '' AND profile_binding_state = ? + `), models.ProfileBindingInherit, time.Now().UTC(), id, models.ProfileBindingUnconfigured) + if err != nil { + return false, err + } + rows, err := result.RowsAffected() + if err != nil { + return false, err + } + return rows == 1, nil +} + func (r *sqliteRepository) DeleteAgent(ctx context.Context, id string) error { _, err := r.db.ExecContext(ctx, r.db.Rebind(`DELETE FROM utility_agents WHERE id = ? AND builtin = 0`), id) return err diff --git a/apps/backend/internal/utility/store/sqlite_migration_test.go b/apps/backend/internal/utility/store/sqlite_migration_test.go index 21e8a135a1..35704fc493 100644 --- a/apps/backend/internal/utility/store/sqlite_migration_test.go +++ b/apps/backend/internal/utility/store/sqlite_migration_test.go @@ -117,3 +117,42 @@ func TestSeedBuiltinAgents_UsesClaudeACP(t *testing.T) { t.Fatalf("expected at least one built-in row, found 0") } } + +func TestNormalizeEmptyBuiltinBindingUsesStalePredicate(t *testing.T) { + sqlxDB := openTestDB(t) + repo, err := newSQLiteRepositoryWithDB(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("schema init: %v", err) + } + + now := time.Now().UTC() + if _, err := sqlxDB.Exec(`INSERT OR REPLACE INTO utility_agents + (id, name, description, prompt, agent_id, model, agent_profile_id, profile_binding_state, builtin, enabled, created_at, updated_at) + VALUES (?, ?, '', '', 'claude-acp', '', '', 'unconfigured', 1, 0, ?, ?)`, + "stale", "stale", now, now); err != nil { + t.Fatalf("seed stale builtin: %v", err) + } + + changed, err := repo.NormalizeEmptyBuiltinBinding(context.Background(), "stale") + if err != nil || !changed { + t.Fatalf("NormalizeEmptyBuiltinBinding() = (%v, %v), want (true, nil)", changed, err) + } + + if _, err := sqlxDB.Exec(`UPDATE utility_agents + SET agent_profile_id = 'profile-1', profile_binding_state = 'explicit' + WHERE id = ?`, "stale"); err != nil { + t.Fatalf("simulate concurrent repair: %v", err) + } + changed, err = repo.NormalizeEmptyBuiltinBinding(context.Background(), "stale") + if err != nil || changed { + t.Fatalf("NormalizeEmptyBuiltinBinding() after repair = (%v, %v), want (false, nil)", changed, err) + } + + var profileID, bindingState string + if err := sqlxDB.QueryRowx(`SELECT agent_profile_id, profile_binding_state FROM utility_agents WHERE id = ?`, "stale").Scan(&profileID, &bindingState); err != nil { + t.Fatalf("read repaired builtin: %v", err) + } + if profileID != "profile-1" || bindingState != "explicit" { + t.Fatalf("concurrent repair was overwritten: profile=%q state=%q", profileID, bindingState) + } +} diff --git a/apps/web/components/settings/utility-agents-section.test.ts b/apps/web/components/settings/utility-agents-section.test.ts index 7eb5f67acb..70400435be 100644 --- a/apps/web/components/settings/utility-agents-section.test.ts +++ b/apps/web/components/settings/utility-agents-section.test.ts @@ -1,9 +1,11 @@ import { describe, expect, it } from "vitest"; import type { UtilityAgent } from "@/lib/api/domains/utility-api"; +import { USE_DEFAULT } from "./utility-sections"; import { isUtilityAgentDirty, mergeRefreshedUtilityAgents, replaceCustomUtilityAgents, + updateBuiltinProfileDraft, } from "./utility-agents-section"; const DRAFT_MODEL = "draft-model"; @@ -23,7 +25,7 @@ function agent(id: string, builtin: boolean, model: string): UtilityAgent { }; } -describe("replaceCustomUtilityAgents", () => { +describe("utility agent helpers", () => { it("compares only draftable built-in fields", () => { const baseline = agent("commit", true, "saved-model"); @@ -54,3 +56,30 @@ describe("replaceCustomUtilityAgents", () => { ]); }); }); + +describe("updateBuiltinProfileDraft", () => { + it("treats the picker fallback value as an inherited binding", () => { + const base = { + ...agent("commit", true, ""), + enabled: false, + agent_profile_id: "deleted-profile", + profile_binding_state: "unconfigured", + }; + + for (const pickerValue of ["", USE_DEFAULT]) { + const draft = updateBuiltinProfileDraft(base, pickerValue); + + expect(draft.agent_profile_id).toBe(""); + expect(draft.profile_binding_state).toBe("inherit"); + expect(draft.enabled).toBe(true); + } + }); + + it("keeps a concrete profile selection explicit", () => { + const draft = updateBuiltinProfileDraft(agent("commit", true, ""), "profile-1"); + + expect(draft.agent_profile_id).toBe("profile-1"); + expect(draft.profile_binding_state).toBe("explicit"); + expect(draft.enabled).toBe(true); + }); +}); diff --git a/apps/web/components/settings/utility-agents-section.tsx b/apps/web/components/settings/utility-agents-section.tsx index bc234766d9..f930a9a00f 100644 --- a/apps/web/components/settings/utility-agents-section.tsx +++ b/apps/web/components/settings/utility-agents-section.tsx @@ -57,23 +57,23 @@ export function mergeRefreshedUtilityAgents( }); } +export function updateBuiltinProfileDraft(agent: UtilityAgent, value: string): UtilityAgent { + const inherit = value === "" || value === USE_DEFAULT; + return { + ...agent, + agent_profile_id: inherit ? "" : value, + profile_binding_state: inherit ? "inherit" : "explicit", + enabled: true, + }; +} + function updateBuiltinDraft( agent: UtilityAgent, value: string, setAgents: React.Dispatch>, ) { - const inherit = value === USE_DEFAULT; setAgents((prev) => - prev.map((item) => - item.id === agent.id - ? { - ...item, - agent_profile_id: inherit ? "" : value, - profile_binding_state: inherit ? "inherit" : "explicit", - enabled: true, - } - : item, - ), + prev.map((item) => (item.id === agent.id ? updateBuiltinProfileDraft(item, value) : item)), ); } diff --git a/apps/web/e2e/tests/settings/mobile-utility-agents.spec.ts b/apps/web/e2e/tests/settings/mobile-utility-agents.spec.ts index 8d4b868e26..1002ead8bf 100644 --- a/apps/web/e2e/tests/settings/mobile-utility-agents.spec.ts +++ b/apps/web/e2e/tests/settings/mobile-utility-agents.spec.ts @@ -8,6 +8,88 @@ import { test, expect } from "../../fixtures/test-base"; * stacked. See PR #1654 review discussion. */ test.describe("Mobile utility agents action rows", () => { + test("repairs an unavailable action with the default by touch", async ({ + testPage, + apiClient, + seedData, + prCapture, + }) => { + const agentId = "builtin-commit-message"; + const { agents } = await apiClient.listAgents(); + const profile = agents + .flatMap((agent) => agent.profiles ?? []) + .find((candidate) => candidate.id === seedData.agentProfileId); + if (!profile) throw new Error(`seed profile ${seedData.agentProfileId} was not found`); + let savedBinding: Record | undefined; + const unavailable = { + id: agentId, + name: "commit-message", + description: "Generate a commit message.", + prompt: "Generate a commit message.", + builtin: true, + enabled: false, + agent_id: "", + model: "", + agent_profile_id: "", + profile_binding_state: "unconfigured", + }; + + await testPage.route("**/api/v1/user/settings", (route) => { + if (route.request().method() !== "GET") return route.continue(); + return route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ + settings: { default_utility_agent_profile_id: seedData.agentProfileId }, + }), + }); + }); + await testPage.route(`**/api/v1/utility/agents/${agentId}`, async (route) => { + if (route.request().method() !== "PATCH") return route.continue(); + savedBinding = route.request().postDataJSON() as Record; + return route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ ...unavailable, ...savedBinding }), + }); + }); + await testPage.route("**/api/v1/utility/agents", (route) => { + if (route.request().method() !== "GET") return route.continue(); + return route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ agents: [unavailable] }), + }); + }); + + await testPage.goto("/settings/utility-agents"); + await expect( + testPage.getByRole("heading", { name: "Utility Agents", exact: true }), + ).toBeVisible({ timeout: 15_000 }); + + const card = testPage.getByTestId("utility-actions-card"); + const row = testPage.getByTestId(`utility-action-row-${agentId}`); + const picker = row.getByTestId(`utility-profile-picker-action-${agentId}`); + await picker.tap(); + const listbox = testPage.getByRole("listbox"); + await listbox.locator('[data-value="__USE_DEFAULT__"]').tap(); + + const floatingSave = testPage.getByTestId("settings-floating-save"); + await floatingSave.getByRole("button", { name: "Save changes" }).tap(); + await expect(floatingSave).not.toBeVisible({ timeout: 10_000 }); + expect(savedBinding).toMatchObject({ + agent_profile_id: "", + profile_binding_state: "inherit", + enabled: true, + }); + expect(await card.evaluate((element) => element.scrollWidth > element.clientWidth + 1)).toBe( + false, + ); + await prCapture.screenshot("utility-action-inherits-default-mobile", { + caption: "Default inheritance remains reachable on a narrow touch viewport.", + }); + }); + test("profile select and edit button stay reachable on a narrow viewport", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/settings/utility-agents.spec.ts b/apps/web/e2e/tests/settings/utility-agents.spec.ts index bbd7c20cfa..b901a21123 100644 --- a/apps/web/e2e/tests/settings/utility-agents.spec.ts +++ b/apps/web/e2e/tests/settings/utility-agents.spec.ts @@ -10,6 +10,111 @@ import { test, expect } from "../../fixtures/test-base"; * interactions (open the page, inspect sections, open the create dialog). */ test.describe("Utility Agents settings page", () => { + test("repairs an unavailable action with the default", async ({ + testPage, + apiClient, + seedData, + prCapture, + }) => { + const agentId = "builtin-commit-message"; + const { agents } = await apiClient.listAgents(); + const profile = agents + .flatMap((agent) => agent.profiles ?? []) + .find((candidate) => candidate.id === seedData.agentProfileId); + if (!profile) throw new Error(`seed profile ${seedData.agentProfileId} was not found`); + const profileLabel = `${profile.agentDisplayName} • ${profile.name}`; + const baselineResponse = await apiClient.rawRequest("GET", `/api/v1/utility/agents/${agentId}`); + expect(baselineResponse.ok).toBe(true); + const baseline = await baselineResponse.json(); + + await testPage.route("**/api/v1/user/settings", (route) => { + if (route.request().method() !== "GET") return route.continue(); + return route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ + settings: { default_utility_agent_profile_id: seedData.agentProfileId }, + }), + }); + }); + await testPage.route("**/api/v1/utility/agents", (route) => { + if (route.request().method() !== "GET") return route.continue(); + return route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ + agents: [ + { + ...baseline, + agent_profile_id: "", + profile_binding_state: "unconfigured", + enabled: false, + }, + ], + }), + }); + }); + + try { + await testPage.goto("/settings/utility-agents"); + await expect( + testPage.getByRole("heading", { name: "Utility Agents", exact: true }), + ).toBeVisible({ timeout: 15_000 }); + + const row = testPage.getByTestId(`utility-action-row-${agentId}`); + const picker = row.getByTestId(`utility-profile-picker-action-${agentId}`); + await expect( + row.getByText("This profile is unavailable. Select an enabled inference profile.", { + exact: true, + }), + ).toBeVisible(); + + await picker.click(); + const listbox = testPage.getByRole("listbox"); + await listbox.locator('[data-value="__USE_DEFAULT__"]').click(); + + const floatingSave = testPage.getByTestId("settings-floating-save"); + await expect(floatingSave).toBeVisible(); + await floatingSave.getByRole("button", { name: "Save changes" }).click(); + await expect(floatingSave).not.toBeVisible({ timeout: 10_000 }); + + await testPage.unroute("**/api/v1/utility/agents"); + await testPage.reload(); + await expect(picker).toContainText(profileLabel, { timeout: 15_000 }); + await expect( + row.getByText("This profile is unavailable. Select an enabled inference profile.", { + exact: true, + }), + ).toHaveCount(0); + await prCapture.screenshot("utility-action-inherits-default-desktop", { + caption: "The repaired utility action inherits the selected default profile.", + }); + + const persistedResponse = await apiClient.rawRequest( + "GET", + `/api/v1/utility/agents/${agentId}`, + ); + expect(persistedResponse.ok).toBe(true); + const persisted = await persistedResponse.json(); + expect(persisted).toMatchObject({ + agent_profile_id: "", + profile_binding_state: "inherit", + enabled: true, + }); + } finally { + const restoreResponse = await apiClient.rawRequest( + "PATCH", + `/api/v1/utility/agents/${agentId}`, + { + agent_profile_id: baseline.agent_profile_id, + profile_binding_state: baseline.profile_binding_state, + enabled: baseline.enabled, + }, + ); + expect(restoreResponse.ok).toBe(true); + } + }); + test("renders the default for an inherited built-in action", async ({ testPage, apiClient, diff --git a/docs/decisions/2026-08-12-built-in-utility-default-inheritance.md b/docs/decisions/2026-08-12-built-in-utility-default-inheritance.md index 2bd9c5b215..d11d6613a0 100644 --- a/docs/decisions/2026-08-12-built-in-utility-default-inheritance.md +++ b/docs/decisions/2026-08-12-built-in-utility-default-inheritance.md @@ -1,6 +1,6 @@ # ADR-2026-08-12-built-in-utility-default-inheritance: Built-in Utility Actions Inherit the Global Profile by Default -**Status:** accepted +**Status:** superseded by 2026-08-12-empty-utility-bindings-inherit-default **Date:** 2026-08-12 **Area:** backend, frontend diff --git a/docs/decisions/2026-08-12-empty-utility-bindings-inherit-default.md b/docs/decisions/2026-08-12-empty-utility-bindings-inherit-default.md new file mode 100644 index 0000000000..40f0baefa9 --- /dev/null +++ b/docs/decisions/2026-08-12-empty-utility-bindings-inherit-default.md @@ -0,0 +1,47 @@ +# ADR-2026-08-12-empty-utility-bindings-inherit-default: Empty Built-in Utility Bindings Inherit the Default + +**Status:** accepted +**Date:** 2026-08-12 +**Area:** backend, frontend + +## Context + +Older profile migration code persisted built-in utility actions with an empty profile ID and an +`unconfigured` binding. The previous default-inheritance decision preserved that state because it +did not distinguish a deleted explicit binding from a failed legacy migration. In practice, this +leaves every built-in action unavailable when the user has selected a valid global default. The +settings repair flow can also send an invalid explicit binding when the user chooses Default. + +## Decision + +A built-in utility action with an empty profile ID always means that no concrete override is +recoverable. The backend normalizes an empty `unconfigured` built-in binding to `inherit`, and the +action resolves through the user's saved global default profile. This normalization is idempotent +and persists the inherited state so the database does not continue to expose the action as +unavailable. + +The Utility Agents picker treats its fallback callback value, the empty string, as an inherited +selection. Saving Default sends an empty `agent_profile_id` with `profile_binding_state: "inherit"`. +It never sends an empty explicit binding. + +A non-empty stale profile ID still represents a recoverable explicit choice. Such a binding remains +`unconfigured` and fails closed until the user selects Default or another eligible profile. Custom +utility agents still require a concrete eligible profile. + +## Consequences + +- Existing installations with empty unavailable built-in actions recover on the next binding + migration and use the selected global default. +- Selecting Default repairs the action through the normal Settings save flow. +- The system no longer preserves hypothetical explicit intent when no profile identity remains. +- Concrete stale overrides remain diagnosable and do not silently change model, permissions, + credentials, or launch policy. + +## Alternatives Considered + +1. **Keep every empty `unconfigured` row fail-closed.** Rejected because no concrete identity can be + repaired or audited, while the product contract already gives every built-in action a default. +2. **Copy the current default profile ID into every repaired row.** Rejected because built-in + actions then stop following later changes to the global default. +3. **Convert every stale binding to `inherit`.** Rejected because a non-empty stale ID preserves a + real explicit user choice and its security-relevant launch configuration. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index f4dbf60f8e..b7b4a13bbc 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -158,5 +158,6 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-10-remote-contribution-head-drift | [Separate Current Contribution and Local Checkout Histories](2026-08-10-remote-contribution-head-drift.md) | accepted (remote-action policy superseded) | backend, frontend, protocol, GitHub, GitLab | 2026-08-10 | | 2026-08-12-local-first-contribution-replacement | [Keep Remote Contribution Work Local-First](2026-08-12-local-first-contribution-replacement.md) | accepted | backend, frontend, protocol, security, GitHub, GitLab | 2026-08-12 | | 2026-08-11-user-owned-status-bar-visibility | [Make Status Bar Visibility a Portable User Preference](2026-08-11-user-owned-status-bar-visibility.md) | accepted | backend, frontend, protocol | 2026-08-11 | -| 2026-08-12-built-in-utility-default-inheritance | [Built-in Utility Actions Inherit the Global Profile by Default](2026-08-12-built-in-utility-default-inheritance.md) | accepted | backend, frontend | 2026-08-12 | +| 2026-08-12-built-in-utility-default-inheritance | [Built-in Utility Actions Inherit the Global Profile by Default](2026-08-12-built-in-utility-default-inheritance.md) | superseded by 2026-08-12-empty-utility-bindings-inherit-default | backend, frontend | 2026-08-12 | +| 2026-08-12-empty-utility-bindings-inherit-default | [Empty Built-in Utility Bindings Inherit the Default](2026-08-12-empty-utility-bindings-inherit-default.md) | accepted | backend, frontend | 2026-08-12 | | 2026-08-10-no-em-dash-public-copy | [Keep Em Dashes Out of Public Copy](2026-08-10-no-em-dash-public-copy.md) | accepted | frontend, infra | 2026-08-10 | diff --git a/docs/plans/utility-agent-unavailable-repair/plan.md b/docs/plans/utility-agent-unavailable-repair/plan.md new file mode 100644 index 0000000000..efa34ac8b2 --- /dev/null +++ b/docs/plans/utility-agent-unavailable-repair/plan.md @@ -0,0 +1,101 @@ +--- +spec: docs/specs/agents/utility-agent-profiles.md +created: 2026-08-12 +status: complete +--- + +# Implementation Plan: Repair Unavailable Utility Actions + +## Overview + +The live database contains all eight built-in utility actions as empty `unconfigured` rows. A valid +default profile exists. The settings picker translates its Default option to an empty callback +value. The draft code treats only `__USE_DEFAULT__` as inherited and submits an invalid empty +explicit binding. The repair first normalizes legacy empty built-ins to `inherit`. Then it fixes the +shared desktop and mobile save path and proves persistence through the real backend. + +--- + +## Backend + +### Legacy binding normalization + +- `apps/backend/internal/utility/service/service.go`: update `MigrateLegacyBindings` so a built-in + with `profile_binding_state = 'unconfigured'` and an empty `agent_profile_id` is persisted as + `inherit` through a conditional repository update. Preserve non-empty stale IDs and every custom + `unconfigured` row, including when a concurrent settings save changes the stale row first. +- Keep the operation idempotent and preserve the existing `enabled` value. This repair changes only + binding ownership. It does not silently enable an action or copy a concrete profile ID. + +## Frontend + +### Default action selection + +- `apps/web/components/settings/utility-agents-section.tsx`: extract the built-in profile draft + transition into a small pure helper. Treat the profile picker's empty fallback callback as Default, + producing an empty profile ID, `inherit`, and `enabled: true`. Keep concrete values explicit. +- Do not change `UtilityAgentProfilePicker`'s global callback contract because the default-profile + card intentionally uses the empty value to mean no saved default. + +### Mobile design contract + +The existing stacked action row remains the desktop and phone entry point. The picker, save bar, +single page scroll owner, touch targets, and responsive composition do not change. The state +transition is shared across viewports. The existing mobile action-row spec is the nearest shipped +mobile surface and will be extended to prove that choosing Default by touch submits an inherited +binding. + +## Tests + +- **What:** Empty unavailable built-ins normalize to inherited state, while concrete stale and custom + rows remain unconfigured. + **File:** `apps/backend/internal/utility/service/service_test.go`. + **How:** Replace the preservation regression with table-driven service migration tests that assert + repository updates and idempotent second-run behavior. +- **What:** The fallback value from the shared picker creates a valid inherited draft, and a concrete + profile creates an explicit draft. + **File:** `apps/web/components/settings/utility-agents-section.test.ts`. + **How:** Pure helper tests, including the exact empty-string callback that caused the failure. +- **What:** Selecting Default traverses the handler, service, and SQLite repository without an empty + explicit binding. + **File:** `apps/web/e2e/tests/settings/utility-agents.spec.ts`. + **How:** Serve one unavailable GET state, continue the PATCH to the real isolated backend, wait for + Saved, reload from the real backend, and assert the inherited default remains selected. + +## E2E Tests + +- **Scenario:** GIVEN an empty unavailable built-in action and a valid global default, WHEN the user + selects Default and saves, THEN the real API save succeeds and reload shows the inherited default. + **File:** `apps/web/e2e/tests/settings/utility-agents.spec.ts`. +- **Scenario:** GIVEN the same action on a Pixel 5 viewport, WHEN the user selects Default with the + touch picker and saves, THEN the outgoing binding is `inherit` and all controls remain reachable. + **File:** `apps/web/e2e/tests/settings/mobile-utility-agents.spec.ts`. + +## Verification Results + +- Backend utility and backend-app suites: `cd apps/backend && go test ./internal/utility/... ./internal/backendapp/...` + passed 378 tests; `go vet ./internal/utility/...` passed. The SQLite stale-predicate regression is + covered by the utility store migration suite. +- Frontend utility suites: `cd apps/web && pnpm exec vitest run + components/settings/utility-agents-section.test.ts + components/settings/utility-sections.test.ts` passed 8 tests. +- Frontend type check: `cd apps/web && pnpm run typecheck` passed. +- Internationalization checks: `cd apps/web && pnpm run i18n:check && pnpm run i18n:ratchet` + passed. Existing translation parity notices remained advisory. +- Desktop E2E: the unavailable action saved through the real backend, reloaded as `inherit`, and + restored its isolated fixture baseline. +- Mobile E2E: both Utility Agents scenarios passed. The touch flow sent the inherited binding and + the actions card had no horizontal overflow. + +## Implementation Waves And Parallel Candidates + +Wave 1: + +- [x] [task-01-normalize-empty-bindings](task-01-normalize-empty-bindings.md) + +Wave 2: + +- [x] [task-02-save-default-selection](task-02-save-default-selection.md) + +The tasks are sequential because the UI persistence scenario verifies the backend binding contract. +No subagent delegation is authorized by this plan. diff --git a/docs/plans/utility-agent-unavailable-repair/task-01-normalize-empty-bindings.md b/docs/plans/utility-agent-unavailable-repair/task-01-normalize-empty-bindings.md new file mode 100644 index 0000000000..1ebe10198c --- /dev/null +++ b/docs/plans/utility-agent-unavailable-repair/task-01-normalize-empty-bindings.md @@ -0,0 +1,64 @@ +--- +id: "01-normalize-empty-bindings" +title: "Normalize empty utility bindings" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/agents/utility-agent-profiles.md" +--- + +# Task 01: Normalize empty utility bindings + +Repair legacy built-in rows before changing the settings save path. + +- **Acceptance:** A built-in with an empty profile ID and `unconfigured` state is persisted as + `inherit`, and a second migration run makes no further update. +- **Acceptance:** A concrete stale built-in binding and every custom unconfigured binding remain + unchanged and fail closed. +- **Acceptance:** Migration preserves the row's existing `enabled` value and does not copy a concrete + default profile ID. +- **Acceptance:** A concurrent concrete repair cannot be overwritten by the startup normalization. +- **Verification:** Add the failing migration tests first, then run: + + ```bash + cd apps/backend + go test -run 'TestMigrateLegacyBindings' ./internal/utility/service/... + ``` + +- **Files likely touched:** `apps/backend/internal/utility/service/service.go`, + `apps/backend/internal/utility/service/service_test.go`, + `apps/backend/internal/utility/store/interface.go`, `apps/backend/internal/utility/store/sqlite.go`, + and `apps/backend/internal/utility/store/sqlite_migration_test.go`. +- **Dependencies:** None. +- **Parallelism:** sequential. +- **Inputs:** The legacy migration, failure-mode, persistence, and scenario sections in + `docs/specs/agents/utility-agent-profiles.md`, plus + ADR-2026-08-12-empty-utility-bindings-inherit-default. +- **Output contract:** Report files changed, the RED and GREEN test commands and results, idempotency + evidence, preserved stale/custom behavior, and synchronized task/plan status. + +## Results + +Implemented idempotent, conditional normalization for empty `unconfigured` built-in rows. The +migration now persists only the binding-state field to `inherit` without changing `enabled`, and a +stale-row predicate prevents a concurrent concrete repair from being overwritten. Concrete stale +built-in bindings and custom unconfigured bindings remain unchanged. + +- RED: `cd apps/backend && go test -run 'TestMigrateLegacyBindingsNormalizesEmptyUnconfiguredBuiltin' ./internal/utility/service/...` + (failed as expected: `updated = 0, want 1`) +- GREEN: `cd apps/backend && go test -run 'TestMigrateLegacyBindingsNormalizesEmptyUnconfiguredBuiltin' ./internal/utility/service/...` + (pass: 1 test) +- REFACTOR: `cd apps/backend && go test -run 'TestMigrateLegacyBindings' ./internal/utility/service/...` + (pass: 5 tests) +- Concurrency guard: `cd apps/backend && go test -run 'TestMigrateLegacyBindingsDoesNotOverwriteConcurrentRepair' ./internal/utility/service/...` + (pass: conditional repository fake preserves the concurrent explicit repair) +- SQLite predicate: `cd apps/backend && go test -run 'TestNormalizeEmptyBuiltinBindingUsesStalePredicate' ./internal/utility/store/...` + (pass: first stale row updates, second repaired row is not changed) +- Package verification: `cd apps/backend && go test ./internal/utility/... ./internal/backendapp/...` + (pass: 378 tests) and `go vet ./internal/utility/...` (pass). +- Fixup review: preserved the stale concrete profile ID assertion and added a conditional SQLite + update so concurrent settings repairs cannot be overwritten. +- Generated artifacts: None. +- Cleanup: No temporary artifacts. +- Security or external side effects: None. Tests use the in-memory fake repository. diff --git a/docs/plans/utility-agent-unavailable-repair/task-02-save-default-selection.md b/docs/plans/utility-agent-unavailable-repair/task-02-save-default-selection.md new file mode 100644 index 0000000000..e16722856a --- /dev/null +++ b/docs/plans/utility-agent-unavailable-repair/task-02-save-default-selection.md @@ -0,0 +1,75 @@ +--- +id: "02-save-default-selection" +title: "Save inherited utility selection" +status: done +wave: 2 +depends_on: ["01-normalize-empty-bindings"] +plan: "plan.md" +spec: "../../specs/agents/utility-agent-profiles.md" +--- + +# Task 02: Save inherited utility selection + +Fix the shared Utility Agents draft transition and prove the real persistence path on desktop and +phone viewports. + +- **Acceptance:** The picker's empty fallback callback creates `agent_profile_id: ""`, + `profile_binding_state: "inherit"`, and `enabled: true`. A concrete profile remains explicit. +- **Acceptance:** Selecting Default from an unavailable built-in action saves through the real + backend and remains selected after reload. +- **Acceptance:** The existing stacked mobile action row remains reachable and the touch flow sends + the same inherited binding without horizontal overflow. +- **Verification:** Bootstrap dependencies if needed, add the failing unit and E2E regressions first, + then run: + + ```bash + cd apps + pnpm install --frozen-lockfile + pnpm --filter @kandev/web test -- --run components/settings/utility-agents-section.test.ts + cd web + pnpm run typecheck + pnpm run i18n:check + pnpm run i18n:ratchet + pnpm e2e:run --project chromium tests/settings/utility-agents.spec.ts -- --grep "repairs an unavailable action with the default" + pnpm e2e:run --project mobile-chrome tests/settings/mobile-utility-agents.spec.ts + ``` + +- **Files likely touched:** `apps/web/components/settings/utility-agents-section.tsx`, + `apps/web/components/settings/utility-agents-section.test.ts`, + `apps/web/e2e/tests/settings/utility-agents.spec.ts`, and + `apps/web/e2e/tests/settings/mobile-utility-agents.spec.ts`. +- **Dependencies:** Task 01. +- **Parallelism:** sequential. +- **Inputs:** The Utility Agents save and persistence scenarios in + `docs/specs/agents/utility-agent-profiles.md`, the current shared picker empty-value contract, the + existing desktop Utility Agents spec, and the existing stacked mobile action-row spec. +- **Output contract:** Report files changed, RED and GREEN test commands and results, saved PATCH + payload and reload evidence, desktop/mobile outcomes, cleanup, and synchronized task/plan status. + +## Results + +Implemented a shared draft transition that treats both the Default sentinel and the picker's empty +callback as inherited state. Concrete profile IDs remain explicit. The desktop regression serves a +synthetic unavailable GET response, lets the PATCH reach the isolated Go backend, reloads from that +backend, and restores the original fixture row. The mobile regression selects Default by touch, +captures the inherited PATCH body, and verifies that the actions card does not overflow. + +- RED: `cd apps/web && pnpm exec vitest run + components/settings/utility-agents-section.test.ts` failed as expected because the empty callback + produced `explicit` instead of `inherit`. +- GREEN: the same command passed 5 tests after the draft transition repair. +- Component regression: `cd apps/web && pnpm exec vitest run + components/settings/utility-agents-section.test.ts + components/settings/utility-sections.test.ts` passed 8 tests. +- Desktop E2E: `cd apps/web && pnpm exec playwright test --config=e2e/playwright.config.ts + e2e/tests/settings/utility-agents.spec.ts --project=chromium --grep "repairs an unavailable + action with the default"` passed 1 test. +- Mobile E2E: `cd apps/web && pnpm exec playwright test --config=e2e/playwright.config.ts + e2e/tests/settings/mobile-utility-agents.spec.ts --project=mobile-chrome` passed 2 tests. +- Saved mobile PATCH: `agent_profile_id: ""`, `profile_binding_state: "inherit"`, and + `enabled: true`. +- Fixup review: both desktop and mobile option selection now scope through the page-level listbox + role, while the picker test ID remains available for the trigger and dropdown semantics. +- Generated artifacts: None. +- Cleanup: The desktop E2E restores the original built-in row in a `finally` block. +- Security or external side effects: None. Both browser projects use an isolated E2E backend. diff --git a/docs/specs/agents/utility-agent-profiles.md b/docs/specs/agents/utility-agent-profiles.md index 9c1d33b7bf..039a6bea5a 100644 --- a/docs/specs/agents/utility-agent-profiles.md +++ b/docs/specs/agents/utility-agent-profiles.md @@ -10,11 +10,11 @@ Decision: [ADR-2026-08-08-utility-agent-profile-execution](../../decisions/2026- Safety decision: [ADR-2026-08-08-utility-profile-dependency-safety](../../decisions/2026-08-08-utility-profile-dependency-safety.md) -Default inheritance repair: [ADR-2026-08-12-built-in-utility-default-inheritance](../../decisions/2026-08-12-built-in-utility-default-inheritance.md) +Default inheritance repair: [ADR-2026-08-12-empty-utility-bindings-inherit-default](../../decisions/2026-08-12-empty-utility-bindings-inherit-default.md) Implementation plan: [utility-agent-profiles](../../plans/utility-agent-profiles/plan.md) -Repair plan: [utility-action-default-profile](../../plans/utility-action-default-profile/plan.md) +Latest repair plan: [utility-agent-unavailable-repair](../../plans/utility-agent-unavailable-repair/plan.md) ## Why @@ -34,6 +34,8 @@ permission choice that its caller cannot answer. - A built-in utility action without a concrete profile override inherits the default utility profile. A stale concrete override remains **unconfigured** after its profile is deleted or disabled and cannot run until the user repairs it. +- An empty `unconfigured` built-in binding is normalized to `inherit`. Selecting Default in an + action picker persists the same inherited state and never submits an empty explicit binding. - Eligible choices are enabled, non-deleted, global profiles for ACP inference-capable agents. CLI-passthrough profiles and workspace-scoped Office profiles are not eligible. - A utility invocation resolves its effective profile at the start of the call. It uses that @@ -67,10 +69,9 @@ permission choice that its caller cannot answer. `inherit` is valid only for built-in rows. `explicit` requires an eligible profile. `unconfigured` is used for a custom row without a profile and for a built-in row whose concrete profile binding is stale or unavailable. Only a built-in row whose persisted state is `inherit` uses the selected -default. A legacy built-in row still in the migration state's `explicit` form with no concrete -profile ID is normalized to `inherit`. An empty `unconfigured` row remains fail-closed because an -older release could have erased the ID of a deleted explicit binding, making that row's original -intent impossible to recover safely. +default. A built-in row with an empty profile ID in either the legacy `explicit` or `unconfigured` +state is normalized to `inherit`. A concrete stale profile ID remains `unconfigured` so the user can +repair or replace the known override explicitly. The legacy `agent_id` and `model` columns may remain temporarily as migration inputs, but they are not execution inputs after this feature ships and are not writable through the utility-agent API. @@ -91,14 +92,13 @@ non-deleted, non-passthrough global profiles whose parent agent matches the lega and whose configured model matches the legacy model (including an explicit empty model). Exactly one match is copied to `agent_profile_id` and sets the row state to `explicit`. -Zero matches or multiple matches set a custom row to `unconfigured`. A built-in row in the legacy -`explicit` migration state with no concrete profile ID becomes `inherit`. Existing `unconfigured` -rows are not remigrated: an older release could have erased the ID of a deleted explicit binding, -so an empty `unconfigured` row remains fail-closed until the user repairs it. The backend does not -pick the first profile, infer from a display name, or silently use a provider default. Custom utility -agents that cannot be migrated remain stored and editable but are not executable until the user -selects a profile. The legacy values remain available to a later retry or diagnostic report, but the -new state is authoritative after the migration. +Zero matches or multiple matches set a custom row to `unconfigured`. A built-in row with no concrete +profile ID becomes `inherit`, including an empty `unconfigured` row written by an older release. +The backend does not pick the first profile, infer from a display name, or silently use a provider +default. Concrete stale built-in IDs remain `unconfigured`. Custom utility agents that cannot be +migrated remain stored and editable but are not executable until the user selects a profile. The +legacy values remain available to a later retry or diagnostic report, but the new state is +authoritative after the migration. ## API surface @@ -133,10 +133,9 @@ new state is authoritative after the migration. - With no effective profile, invocation fails before a call is dispatched and tells the user to select a profile in Settings > Utility Agents. - A missing, deleted, disabled, CLI-passthrough, workspace-scoped, or non-inference-capable - concrete profile binding fails closed. An unconfigured binding also fails closed when its profile - ID is empty because its origin may be an older deleted override. Only an inherited built-in action - uses the saved global default. The backend never falls back to another profile, the active task - profile, a raw agent/model pair, or the first available agent. + concrete profile binding fails closed. An empty built-in binding is normalized to `inherit` and + uses the saved global default. The backend never falls back to another concrete profile, the active + task profile, a raw agent/model pair, or the first available agent. - A profile launch-policy error (invalid command prefix, unresolved required secret, invalid config option, or unavailable agent runtime) is surfaced as the utility call failure. The runner does not retry without that setting. @@ -158,9 +157,8 @@ new state is authoritative after the migration. The warning dialog is confirmation only; it does not perform reassignment. Built-in actions that inherit the default remain inherited when the default profile is deleted, so selecting a new default repairs them without editing each action. -- Built-in rows left as empty `unconfigured` by an older release remain fail-closed because their - original explicit-versus-inherited intent cannot be reconstructed safely. Selecting the default - in the action picker repairs the row by persisting `inherit`. +- Built-in rows left as empty `unconfigured` by an older release are repaired idempotently to + `inherit`. Selecting Default in the action picker also persists `inherit` and survives reload. - Utility call history retains the effective profile ID and resolved model even if the profile is later edited or deleted. @@ -202,10 +200,12 @@ new state is authoritative after the migration. backend upgrades, **THEN** the row state becomes `inherit` and the action uses the selected default utility profile. A custom utility agent with the same migration result becomes `unconfigured` and remains unavailable until the user chooses a profile. -- **GIVEN** an older release left a built-in action `unconfigured` with an empty profile ID after - deleting or clearing a binding, **WHEN** the backend upgrades, **THEN** the row remains - `unconfigured` and fails closed until the user repairs the action because its original intent is - ambiguous. +- **GIVEN** an older release left a built-in action `unconfigured` with an empty profile ID, + **WHEN** the backend runs utility binding migration, **THEN** the row is persisted as `inherit` and + resolves through the saved global default profile. +- **GIVEN** an unavailable built-in action and a valid global default, **WHEN** the user selects + Default and saves, **THEN** the save succeeds, the row persists `inherit` with an empty profile ID, + and the default profile remains selected after reload. - **GIVEN** an inherited built-in action and a deleted default profile, **WHEN** the user selects a new eligible default profile, **THEN** the action uses the new default without an action-level edit.