From 78b41f04fdab3a2ff277a287ace88fced69e1f4a Mon Sep 17 00:00:00 2001 From: siddseethepalli Date: Thu, 16 Apr 2026 21:32:25 +0000 Subject: [PATCH 1/3] macos(settings): add SettingsStore APIs for per-call-site overrides --- .../Features/Settings/CallSiteOverride.swift | 154 +++++++ .../Features/Settings/SettingsStore.swift | 191 ++++++++ .../SettingsStoreCallSiteOverrideTests.swift | 431 ++++++++++++++++++ 3 files changed, 776 insertions(+) create mode 100644 clients/macos/vellum-assistant/Features/Settings/CallSiteOverride.swift create mode 100644 clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift diff --git a/clients/macos/vellum-assistant/Features/Settings/CallSiteOverride.swift b/clients/macos/vellum-assistant/Features/Settings/CallSiteOverride.swift new file mode 100644 index 00000000000..7fc7543e16e --- /dev/null +++ b/clients/macos/vellum-assistant/Features/Settings/CallSiteOverride.swift @@ -0,0 +1,154 @@ +import Foundation + +/// Logical grouping for an LLM call site, used to organize the per-call-site +/// overrides view introduced in PRs 22-24. +/// +/// Mirrors the structure documented in the unify-llm-callsites plan: each +/// call-site ID belongs to exactly one domain so the UI can render them in +/// a stable, user-friendly order. +public enum CallSiteDomain: String, CaseIterable, Identifiable, Hashable { + case agentLoop + case memory + case workspace + case ui + case notifications + case voice + case utility + + public var id: String { rawValue } + + /// User-facing label for this domain. Shown as a section header in the + /// per-call-site override picker. + public var displayName: String { + switch self { + case .agentLoop: return "Agent loop" + case .memory: return "Memory" + case .workspace: return "Workspace" + case .ui: return "UI" + case .notifications: return "Notifications" + case .voice: return "Voice" + case .utility: return "Utility" + } + } + + /// Stable display order for sections in the override picker. Lower values + /// appear first. + public var sortOrder: Int { + switch self { + case .agentLoop: return 0 + case .memory: return 1 + case .workspace: return 2 + case .ui: return 3 + case .notifications: return 4 + case .voice: return 5 + case .utility: return 6 + } + } +} + +/// A user-editable override entry for a single LLM call site. +/// +/// Mirrors the wire shape of `llm.callSites.` in the assistant config: +/// any combination of `provider`, `model`, and `profile` may be set; an +/// entry where all three are `nil` represents "follows the default". +public struct CallSiteOverride: Identifiable, Equatable, Hashable { + /// Stable call-site identifier matching the backend `LLMCallSiteEnum` + /// (e.g. `"memoryRetrieval"`). + public let id: String + + /// User-facing label shown in the override picker + /// (e.g. `"Memory · Retrieval"`). + public let displayName: String + + /// Logical grouping for sectioning in the picker. + public let domain: CallSiteDomain + + /// Provider override; `nil` means "follows the default". + public var provider: String? + + /// Model override; `nil` means "follows the default" (or the profile, + /// when one is selected). + public var model: String? + + /// Profile override referencing a key in `llm.profiles`; `nil` means + /// "no profile selected". + public var profile: String? + + public init( + id: String, + displayName: String, + domain: CallSiteDomain, + provider: String? = nil, + model: String? = nil, + profile: String? = nil + ) { + self.id = id + self.displayName = displayName + self.domain = domain + self.provider = provider + self.model = model + self.profile = profile + } + + /// True when this entry has at least one explicit override + /// (`provider`, `model`, or `profile`). + public var hasOverride: Bool { + provider != nil || model != nil || profile != nil + } +} + +/// Static catalog of every call site the assistant exposes. +/// +/// Mirrors the backend `LLMCallSiteEnum` in +/// `assistant/src/config/schemas/llm.ts`. When the backend enum changes, +/// update this catalog in lockstep so the macOS UI can render every site +/// without depending on a runtime fetch. +public enum CallSiteCatalog { + /// All known call sites, paired with their display name and domain. + /// Order matches the backend enum so the UI is deterministic. + public static let all: [CallSiteOverride] = [ + // Agent loop + CallSiteOverride(id: "mainAgent", displayName: "Main agent", domain: .agentLoop), + CallSiteOverride(id: "subagentSpawn", displayName: "Subagent spawn", domain: .agentLoop), + CallSiteOverride(id: "heartbeatAgent", displayName: "Heartbeat agent", domain: .agentLoop), + CallSiteOverride(id: "filingAgent", displayName: "Filing agent", domain: .agentLoop), + CallSiteOverride(id: "analyzeConversation", displayName: "Analyze conversation", domain: .agentLoop), + CallSiteOverride(id: "callAgent", displayName: "Call agent", domain: .agentLoop), + // Memory + CallSiteOverride(id: "memoryExtraction", displayName: "Memory · Extraction", domain: .memory), + CallSiteOverride(id: "memoryConsolidation", displayName: "Memory · Consolidation", domain: .memory), + CallSiteOverride(id: "memoryRetrieval", displayName: "Memory · Retrieval", domain: .memory), + CallSiteOverride(id: "narrativeRefinement", displayName: "Narrative refinement", domain: .memory), + CallSiteOverride(id: "patternScan", displayName: "Pattern scan", domain: .memory), + CallSiteOverride(id: "conversationSummarization", displayName: "Conversation summarization", domain: .memory), + CallSiteOverride(id: "conversationStarters", displayName: "Conversation starters", domain: .memory), + // Workspace + CallSiteOverride(id: "conversationTitle", displayName: "Conversation title", domain: .workspace), + CallSiteOverride(id: "commitMessage", displayName: "Commit message generator", domain: .workspace), + // UI + CallSiteOverride(id: "identityIntro", displayName: "Identity intro", domain: .ui), + CallSiteOverride(id: "emptyStateGreeting", displayName: "Empty-state greeting", domain: .ui), + // Notifications + CallSiteOverride(id: "notificationDecision", displayName: "Notification decision", domain: .notifications), + CallSiteOverride(id: "preferenceExtraction", displayName: "Preference extraction", domain: .notifications), + // Voice + CallSiteOverride(id: "guardianQuestionCopy", displayName: "Guardian question copy", domain: .voice), + CallSiteOverride(id: "watchCommentary", displayName: "Watch commentary", domain: .voice), + CallSiteOverride(id: "watchSummary", displayName: "Watch summary", domain: .voice), + // Utility + CallSiteOverride(id: "interactionClassifier", displayName: "Interaction classifier", domain: .utility), + CallSiteOverride(id: "styleAnalyzer", displayName: "Style analyzer", domain: .utility), + CallSiteOverride(id: "inviteInstructionGenerator", displayName: "Invite instruction generator", domain: .utility), + CallSiteOverride(id: "skillCategoryInference", displayName: "Skill category inference", domain: .utility), + ] + + /// Lookup table from call-site ID to its catalog entry. Constructed + /// once at first access for O(1) lookup during config sync. + public static let byId: [String: CallSiteOverride] = { + Dictionary(uniqueKeysWithValues: all.map { ($0.id, $0) }) + }() + + /// Set of valid call-site IDs, used to validate / filter raw config + /// payloads coming back from the daemon. + public static let validIds: Set = Set(all.map { $0.id }) +} diff --git a/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift b/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift index 1bf55b99e99..3c48979fe7c 100644 --- a/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift +++ b/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift @@ -58,6 +58,19 @@ public final class SettingsStore: ObservableObject { /// Full provider catalog from daemon. Seeded with inline defaults for pre-fetch rendering. @Published var providerCatalog: [ProviderCatalogEntry] = [] + // MARK: - Per-Call-Site LLM Overrides + + /// Catalog of every LLM call site, merged with whatever overrides the + /// user has configured under `llm.callSites.` in the workspace + /// config. Order matches `CallSiteCatalog.all` so the UI renders a + /// stable list grouped by `CallSiteDomain`. + /// + /// Seeded from the static catalog so the picker has every row available + /// before the first daemon fetch completes. Replaced by + /// `loadCallSiteOverrides(config:)` once the daemon reports the + /// authoritative config. + @Published var callSiteOverrides: [CallSiteOverride] = CallSiteCatalog.all + static let availableImageGenModels: [String] = [ "gemini-3.1-flash-image-preview", "gemini-3-pro-image-preview", @@ -3023,6 +3036,183 @@ public final class SettingsStore: ObservableObject { return task } + // MARK: - Per-Call-Site Override Read / Write + + /// Number of entries in `callSiteOverrides` that have at least one + /// explicit override (`provider`, `model`, or `profile`). Useful for + /// rendering a badge on the overrides settings entry. + var overridesCount: Int { + callSiteOverrides.lazy.filter { $0.hasOverride }.count + } + + /// Reads `llm.callSites.` from the workspace config dictionary, + /// merges every entry against `CallSiteCatalog.all`, and replaces + /// `callSiteOverrides`. Catalog entries missing from the config map to + /// "no override" (all fields `nil`), preserving display order. + /// + /// Unknown call-site IDs in the config (e.g. ones added on a newer + /// daemon) are silently ignored — the catalog is the source of truth + /// for what the UI can render. + func loadCallSiteOverrides(config: [String: Any]) { + let llm = config["llm"] as? [String: Any] + let callSitesRaw = (llm?["callSites"] as? [String: Any]) ?? [:] + var byId: [String: (provider: String?, model: String?, profile: String?)] = [:] + for (id, raw) in callSitesRaw { + guard CallSiteCatalog.validIds.contains(id), + let entry = raw as? [String: Any] else { continue } + let provider = (entry["provider"] as? String).flatMap { $0.isEmpty ? nil : $0 } + let model = (entry["model"] as? String).flatMap { $0.isEmpty ? nil : $0 } + let profile = (entry["profile"] as? String).flatMap { $0.isEmpty ? nil : $0 } + byId[id] = (provider: provider, model: model, profile: profile) + } + self.callSiteOverrides = CallSiteCatalog.all.map { entry in + var merged = entry + if let raw = byId[entry.id] { + merged.provider = raw.provider + merged.model = raw.model + merged.profile = raw.profile + } else { + merged.provider = nil + merged.model = nil + merged.profile = nil + } + return merged + } + } + + /// Persists an override for a single call site at + /// `llm.callSites..{provider,model,profile}`. Nil arguments are + /// omitted from the patch payload — passing `provider: nil` does + /// **not** clear an existing provider override; use + /// `clearCallSiteOverride(_:)` for that. + /// + /// The local `callSiteOverrides` cache is updated optimistically so + /// SwiftUI views reflect the change immediately. + @discardableResult + func setCallSiteOverride( + _ id: String, + provider: String? = nil, + model: String? = nil, + profile: String? = nil + ) -> Task { + guard CallSiteCatalog.validIds.contains(id) else { + log.error("setCallSiteOverride: unknown call-site id \(id, privacy: .public)") + return Task { false } + } + if let index = callSiteOverrides.firstIndex(where: { $0.id == id }) { + if let provider { callSiteOverrides[index].provider = provider } + if let model { callSiteOverrides[index].model = model } + if let profile { callSiteOverrides[index].profile = profile } + } + var entry: [String: Any] = [:] + if let provider { entry["provider"] = provider } + if let model { entry["model"] = model } + if let profile { entry["profile"] = profile } + let payload: [String: Any] = ["llm": ["callSites": [id: entry]]] + let task = Task { + let success = await settingsClient.patchConfig(payload) + if !success { + log.error("Failed to patch config for llm.callSites.\(id, privacy: .public)") + } + return success + } + return task + } + + /// Clears the override for a single call site by writing + /// `{ provider: null, model: null, profile: null }` to + /// `llm.callSites.`. The daemon's PATCH handler uses + /// `deepMergeOverwrite`, which assigns `null` leaves rather than + /// deleting the key, so the on-disk shape becomes + /// `{ "llm": { "callSites": { "": { "provider": null, ... } } } }`. + /// + /// The Zod fragment schema treats `null` as equivalent to "absent" for + /// these optional fields (parsed as `undefined`), which is what the + /// resolver consumes. If a true key-deletion semantic is needed in + /// the future, a dedicated DELETE endpoint should be added — see + /// the PR body for the proposed follow-up. + @discardableResult + func clearCallSiteOverride(_ id: String) -> Task { + guard CallSiteCatalog.validIds.contains(id) else { + log.error("clearCallSiteOverride: unknown call-site id \(id, privacy: .public)") + return Task { false } + } + if let index = callSiteOverrides.firstIndex(where: { $0.id == id }) { + callSiteOverrides[index].provider = nil + callSiteOverrides[index].model = nil + callSiteOverrides[index].profile = nil + } + let entry: [String: Any] = [ + "provider": NSNull(), + "model": NSNull(), + "profile": NSNull(), + ] + let payload: [String: Any] = ["llm": ["callSites": [id: entry]]] + let task = Task { + let success = await settingsClient.patchConfig(payload) + if !success { + log.error("Failed to patch config to clear llm.callSites.\(id, privacy: .public)") + } + return success + } + return task + } + + /// Batch update of every entry in `overrides`. Each entry's + /// `provider`/`model`/`profile` is written verbatim; `nil` fields are + /// emitted as JSON null so the daemon clears them via the same + /// deep-merge mechanism as `clearCallSiteOverride(_:)`. + /// + /// Useful for "reset all overrides" or "apply preset" actions that + /// touch many call sites in a single round trip. The local + /// `callSiteOverrides` cache is replaced so SwiftUI views reflect + /// the new state immediately. + @discardableResult + func setCallSiteOverrides(_ overrides: [CallSiteOverride]) -> Task { + let validOverrides = overrides.filter { CallSiteCatalog.validIds.contains($0.id) } + // Preserve catalog order in the local cache so SwiftUI lists stay stable. + let overrideById = Dictionary(uniqueKeysWithValues: validOverrides.map { ($0.id, $0) }) + callSiteOverrides = CallSiteCatalog.all.map { entry in + var merged = entry + if let provided = overrideById[entry.id] { + merged.provider = provided.provider + merged.model = provided.model + merged.profile = provided.profile + } else { + merged.provider = nil + merged.model = nil + merged.profile = nil + } + return merged + } + var callSitesPayload: [String: Any] = [:] + for entry in validOverrides { + // Emit explicit JSON null for absent fields so the daemon's + // deep-merge clears them rather than leaving stale values in + // place. Build the dict with NSNull placeholders, then + // overwrite with the real string values when present — this + // avoids the Optional-to-Any nil-flattening trap. + var rawEntry: [String: Any] = [ + "provider": NSNull(), + "model": NSNull(), + "profile": NSNull(), + ] + if let provider = entry.provider { rawEntry["provider"] = provider } + if let model = entry.model { rawEntry["model"] = model } + if let profile = entry.profile { rawEntry["profile"] = profile } + callSitesPayload[entry.id] = rawEntry + } + let payload: [String: Any] = ["llm": ["callSites": callSitesPayload]] + let task = Task { + let success = await settingsClient.patchConfig(payload) + if !success { + log.error("Failed to patch config for batch llm.callSites update (\(validOverrides.count, privacy: .public) entries)") + } + return success + } + return task + } + /// Persists the selected TTS provider to the daemon config so synthesis /// routes through the correct backend. The canonical config path is /// `services.tts.provider`. @@ -3812,6 +4002,7 @@ public final class SettingsStore: ObservableObject { Self.applyHostBrowserCdpInspectConfig(config, into: self) loadServiceModes(config: config) + loadCallSiteOverrides(config: config) // Persist enabledSince when it was defaulted so subsequent loads // produce a deterministic timestamp. diff --git a/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift b/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift new file mode 100644 index 00000000000..af079823521 --- /dev/null +++ b/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift @@ -0,0 +1,431 @@ +import XCTest +@testable import VellumAssistantLib +@testable import VellumAssistantShared + +/// Verifies the per-call-site LLM override APIs on `SettingsStore`: +/// catalog seeding, config sync, single-entry write/clear, batch update, +/// and the `overridesCount` derivation that drives UI badges. +@MainActor +final class SettingsStoreCallSiteOverrideTests: XCTestCase { + + private var mockSettingsClient: MockSettingsClient! + private var store: SettingsStore! + + override func setUp() { + super.setUp() + mockSettingsClient = MockSettingsClient() + mockSettingsClient.patchConfigResponse = true + store = SettingsStore(settingsClient: mockSettingsClient) + } + + override func tearDown() { + store = nil + mockSettingsClient = nil + super.tearDown() + } + + // MARK: - Helpers + + /// Returns the most recent `llm.callSites` patch payload captured by + /// the mock client, or `nil` if no such patch has been emitted. + private func lastCallSitesPatch() -> [String: Any]? { + for payload in mockSettingsClient.patchConfigCalls.reversed() { + if let llm = payload["llm"] as? [String: Any], + let sites = llm["callSites"] as? [String: Any] { + return sites + } + } + return nil + } + + /// Waits for the background `Task` started by a store helper to flush + /// its patch into the mock client. + private func waitForPatchCount(_ expected: Int, timeout: TimeInterval = 2.0) { + let predicate = NSPredicate { _, _ in + self.mockSettingsClient.patchConfigCalls.count >= expected + } + let expectation = XCTNSPredicateExpectation(predicate: predicate, object: nil) + wait(for: [expectation], timeout: timeout) + } + + // MARK: - Catalog + + func testCatalogCoversEveryCallSite() { + // The plan calls out 26 sites grouped across 7 domains. If this + // count drifts, the catalog and the backend `LLMCallSiteEnum` + // have diverged. + XCTAssertEqual(CallSiteCatalog.all.count, 26) + XCTAssertEqual(CallSiteCatalog.byId.count, 26) + XCTAssertEqual(CallSiteCatalog.validIds.count, 26) + } + + func testCatalogHasUniqueIdsAndNonEmptyDisplayNames() { + let ids = CallSiteCatalog.all.map(\.id) + XCTAssertEqual(Set(ids).count, ids.count, "CallSiteCatalog must not contain duplicate IDs") + for entry in CallSiteCatalog.all { + XCTAssertFalse(entry.id.isEmpty, "Catalog entry must have non-empty ID") + XCTAssertFalse(entry.displayName.isEmpty, "Catalog entry must have non-empty displayName") + XCTAssertFalse(entry.hasOverride, "Catalog seed entry must start with no override") + } + } + + func testCatalogCoversEveryDomain() { + let representedDomains = Set(CallSiteCatalog.all.map(\.domain)) + XCTAssertEqual(representedDomains, Set(CallSiteDomain.allCases)) + } + + func testStoreSeedsCallSiteOverridesFromCatalog() { + XCTAssertEqual(store.callSiteOverrides.count, CallSiteCatalog.all.count) + XCTAssertEqual(store.overridesCount, 0) + } + + // MARK: - Sync from raw config + + func testLoadCallSiteOverridesPopulatesEntriesFromRawConfig() { + let config: [String: Any] = [ + "llm": [ + "callSites": [ + "memoryRetrieval": [ + "provider": "openai", + "model": "gpt-4.1" + ], + "mainAgent": [ + "profile": "fast" + ] + ] + ] + ] + + store.loadCallSiteOverrides(config: config) + + let memory = store.callSiteOverrides.first(where: { $0.id == "memoryRetrieval" }) + XCTAssertEqual(memory?.provider, "openai") + XCTAssertEqual(memory?.model, "gpt-4.1") + XCTAssertNil(memory?.profile) + XCTAssertTrue(memory?.hasOverride ?? false) + + let main = store.callSiteOverrides.first(where: { $0.id == "mainAgent" }) + XCTAssertNil(main?.provider) + XCTAssertNil(main?.model) + XCTAssertEqual(main?.profile, "fast") + XCTAssertTrue(main?.hasOverride ?? false) + + // An entry that has no override in the config must surface as + // "no override" with all fields nil. + let untouched = store.callSiteOverrides.first(where: { $0.id == "watchSummary" }) + XCTAssertNil(untouched?.provider) + XCTAssertNil(untouched?.model) + XCTAssertNil(untouched?.profile) + XCTAssertFalse(untouched?.hasOverride ?? true) + } + + func testLoadCallSiteOverridesIgnoresUnknownIds() { + let config: [String: Any] = [ + "llm": [ + "callSites": [ + "totallyMadeUpId": [ + "provider": "anthropic" + ], + "memoryRetrieval": [ + "provider": "openai" + ] + ] + ] + ] + + store.loadCallSiteOverrides(config: config) + + XCTAssertEqual(store.callSiteOverrides.count, CallSiteCatalog.all.count) + XCTAssertFalse(store.callSiteOverrides.contains(where: { $0.id == "totallyMadeUpId" })) + let memory = store.callSiteOverrides.first(where: { $0.id == "memoryRetrieval" }) + XCTAssertEqual(memory?.provider, "openai") + } + + func testLoadCallSiteOverridesResetsPriorOverridesWhenConfigEmpty() { + // Seed with an override, then re-load against an empty config to + // confirm `loadCallSiteOverrides` produces a fresh catalog snapshot + // rather than merging on top of stale local state. + store.loadCallSiteOverrides(config: [ + "llm": ["callSites": ["memoryRetrieval": ["provider": "openai"]]] + ]) + XCTAssertEqual(store.overridesCount, 1) + + store.loadCallSiteOverrides(config: [:]) + XCTAssertEqual(store.overridesCount, 0) + XCTAssertEqual(store.callSiteOverrides.count, CallSiteCatalog.all.count) + for entry in store.callSiteOverrides { + XCTAssertFalse(entry.hasOverride, "Expected \(entry.id) to be reset to no override") + } + } + + func testLoadCallSiteOverridesTreatsEmptyStringsAsNil() { + let config: [String: Any] = [ + "llm": [ + "callSites": [ + "memoryRetrieval": [ + "provider": "", + "model": "", + "profile": "" + ] + ] + ] + ] + + store.loadCallSiteOverrides(config: config) + + let memory = store.callSiteOverrides.first(where: { $0.id == "memoryRetrieval" }) + XCTAssertNil(memory?.provider) + XCTAssertNil(memory?.model) + XCTAssertNil(memory?.profile) + XCTAssertFalse(memory?.hasOverride ?? true) + } + + // MARK: - Single-entry write + + func testSetCallSiteOverrideEmitsExpectedPatch() { + _ = store.setCallSiteOverride( + "memoryRetrieval", + provider: "openai", + model: "gpt-4.1" + ) + waitForPatchCount(1) + + let sites = lastCallSitesPatch() + XCTAssertNotNil(sites) + let memory = sites?["memoryRetrieval"] as? [String: Any] + XCTAssertEqual(memory?["provider"] as? String, "openai") + XCTAssertEqual(memory?["model"] as? String, "gpt-4.1") + XCTAssertNil( + memory?["profile"], + "setCallSiteOverride must omit nil keys from the patch payload" + ) + } + + func testSetCallSiteOverrideUpdatesLocalCacheOptimistically() { + _ = store.setCallSiteOverride("mainAgent", profile: "fast") + let main = store.callSiteOverrides.first(where: { $0.id == "mainAgent" }) + XCTAssertEqual(main?.profile, "fast") + XCTAssertEqual(store.overridesCount, 1) + } + + func testSetCallSiteOverrideRejectsUnknownId() { + let task = store.setCallSiteOverride("notARealCallSite", provider: "openai") + let result = waitForResult(task) + XCTAssertFalse(result) + XCTAssertEqual( + mockSettingsClient.patchConfigCalls.count, + 0, + "Unknown call-site IDs must not produce a network call" + ) + } + + // MARK: - Single-entry clear + + func testClearCallSiteOverrideEmitsNullLeavesAndClearsLocalCache() { + _ = store.setCallSiteOverride( + "memoryRetrieval", + provider: "openai", + model: "gpt-4.1" + ) + waitForPatchCount(1) + + _ = store.clearCallSiteOverride("memoryRetrieval") + waitForPatchCount(2) + + let sites = lastCallSitesPatch() + let memory = sites?["memoryRetrieval"] as? [String: Any] + XCTAssertNotNil(memory) + XCTAssertTrue(memory?["provider"] is NSNull) + XCTAssertTrue(memory?["model"] is NSNull) + XCTAssertTrue(memory?["profile"] is NSNull) + + let cached = store.callSiteOverrides.first(where: { $0.id == "memoryRetrieval" }) + XCTAssertNil(cached?.provider) + XCTAssertNil(cached?.model) + XCTAssertNil(cached?.profile) + XCTAssertFalse(cached?.hasOverride ?? true) + XCTAssertEqual(store.overridesCount, 0) + } + + // MARK: - Round-trip + + /// End-to-end round trip: write three overrides, confirm + /// `overridesCount` reflects them, then re-load from a synthetic + /// config that mirrors what would be persisted on disk and verify + /// the store's view matches the original write set. + func testRoundTripWriteThenReloadFromConfig() { + _ = store.setCallSiteOverride("memoryRetrieval", provider: "openai", model: "gpt-4.1") + _ = store.setCallSiteOverride("mainAgent", profile: "fast") + _ = store.setCallSiteOverride("commitMessage", provider: "anthropic") + waitForPatchCount(3) + + XCTAssertEqual(store.overridesCount, 3) + + let synthetic: [String: Any] = [ + "llm": [ + "callSites": [ + "memoryRetrieval": ["provider": "openai", "model": "gpt-4.1"], + "mainAgent": ["profile": "fast"], + "commitMessage": ["provider": "anthropic"], + ] + ] + ] + store.loadCallSiteOverrides(config: synthetic) + + XCTAssertEqual(store.overridesCount, 3) + let memory = store.callSiteOverrides.first(where: { $0.id == "memoryRetrieval" }) + XCTAssertEqual(memory?.provider, "openai") + XCTAssertEqual(memory?.model, "gpt-4.1") + let main = store.callSiteOverrides.first(where: { $0.id == "mainAgent" }) + XCTAssertEqual(main?.profile, "fast") + let commit = store.callSiteOverrides.first(where: { $0.id == "commitMessage" }) + XCTAssertEqual(commit?.provider, "anthropic") + } + + // MARK: - Batch update + + func testSetCallSiteOverridesBatchEmitsAllEntriesInOnePatch() { + let updates: [CallSiteOverride] = [ + CallSiteOverride( + id: "memoryRetrieval", + displayName: "Memory · Retrieval", + domain: .memory, + provider: "openai", + model: "gpt-4.1" + ), + CallSiteOverride( + id: "mainAgent", + displayName: "Main agent", + domain: .agentLoop, + profile: "fast" + ), + CallSiteOverride( + id: "watchSummary", + displayName: "Watch summary", + domain: .voice + ), // no overrides — should emit explicit nulls to clear + ] + + _ = store.setCallSiteOverrides(updates) + waitForPatchCount(1) + + let sites = lastCallSitesPatch() + XCTAssertNotNil(sites) + XCTAssertEqual(sites?.count, updates.count) + + let memory = sites?["memoryRetrieval"] as? [String: Any] + XCTAssertEqual(memory?["provider"] as? String, "openai") + XCTAssertEqual(memory?["model"] as? String, "gpt-4.1") + XCTAssertTrue(memory?["profile"] is NSNull) + + let main = sites?["mainAgent"] as? [String: Any] + XCTAssertTrue(main?["provider"] is NSNull) + XCTAssertTrue(main?["model"] is NSNull) + XCTAssertEqual(main?["profile"] as? String, "fast") + + let watch = sites?["watchSummary"] as? [String: Any] + XCTAssertTrue(watch?["provider"] is NSNull) + XCTAssertTrue(watch?["model"] is NSNull) + XCTAssertTrue(watch?["profile"] is NSNull) + } + + func testSetCallSiteOverridesUpdatesLocalCacheInCatalogOrder() { + let updates: [CallSiteOverride] = [ + CallSiteOverride( + id: "watchSummary", + displayName: "Watch summary", + domain: .voice, + provider: "openai" + ), + CallSiteOverride( + id: "mainAgent", + displayName: "Main agent", + domain: .agentLoop, + provider: "anthropic" + ), + ] + + _ = store.setCallSiteOverrides(updates) + // Local cache must follow CallSiteCatalog.all order, regardless + // of the order the caller passed in. + let mainIndex = store.callSiteOverrides.firstIndex(where: { $0.id == "mainAgent" }) ?? -1 + let watchIndex = store.callSiteOverrides.firstIndex(where: { $0.id == "watchSummary" }) ?? -1 + XCTAssertLessThan(mainIndex, watchIndex, + "callSiteOverrides must preserve CallSiteCatalog order") + XCTAssertEqual(store.overridesCount, 2) + } + + func testSetCallSiteOverridesIgnoresUnknownEntries() { + let updates: [CallSiteOverride] = [ + CallSiteOverride( + id: "totallyMadeUpId", + displayName: "ghost", + domain: .utility, + provider: "openai" + ), + CallSiteOverride( + id: "memoryRetrieval", + displayName: "Memory · Retrieval", + domain: .memory, + provider: "anthropic" + ), + ] + + _ = store.setCallSiteOverrides(updates) + waitForPatchCount(1) + + let sites = lastCallSitesPatch() + XCTAssertEqual(sites?.count, 1, "Unknown call-site IDs must be filtered out of the patch") + XCTAssertNotNil(sites?["memoryRetrieval"]) + XCTAssertNil(sites?["totallyMadeUpId"]) + } + + // MARK: - overridesCount derivation + + func testOverridesCountReflectsPartialOverrides() { + XCTAssertEqual(store.overridesCount, 0) + + store.loadCallSiteOverrides(config: [ + "llm": [ + "callSites": [ + "memoryRetrieval": ["provider": "openai"], + "mainAgent": ["profile": "fast"], + "commitMessage": ["model": "claude-haiku-4"], + ] + ] + ]) + XCTAssertEqual(store.overridesCount, 3) + + // Catalog entries present in raw config but with all fields nil + // do not count as overrides — guards against the empty-string + // sanitization in `loadCallSiteOverrides`. + store.loadCallSiteOverrides(config: [ + "llm": [ + "callSites": [ + "memoryRetrieval": ["provider": "", "model": "", "profile": ""] + ] + ] + ]) + XCTAssertEqual(store.overridesCount, 0) + } + + // MARK: - Test utilities + + private func waitForResult( + _ task: Task, + timeout: TimeInterval = 2.0 + ) -> T { + let expectation = XCTestExpectation(description: "Task completes") + let box = ResultBox() + Task { + box.value = await task.value + expectation.fulfill() + } + wait(for: [expectation], timeout: timeout) + return box.value! + } +} + +private final class ResultBox: @unchecked Sendable { + var value: T? +} From f32910993215d6b9f41fea9d337a2f8679f160bf Mon Sep 17 00:00:00 2001 From: siddseethepalli Date: Thu, 16 Apr 2026 21:41:28 +0000 Subject: [PATCH 2/3] fix(callsite-overrides): harden setCallSiteOverrides against dup-id crash and batch divergence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devin and Codex flagged two issues: - Dictionary(uniqueKeysWithValues:) crashes if callers pass duplicate CallSiteOverride.id values (external input — must be tolerant). Switch to Dictionary(_:uniquingKeysWith:) with last-write-wins. - Batch updates locally cleared entries omitted from the input but only PATCHed entries that were present, so omitted entries appeared cleared in the UI but reappeared on next sync. Now the PATCH payload includes NSNull clears for every catalog entry not in the batch, aligning remote with local. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Features/Settings/SettingsStore.swift | 22 +++- .../SettingsStoreCallSiteOverrideTests.swift | 122 +++++++++++++++++- 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift b/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift index 3c48979fe7c..eedfdd82168 100644 --- a/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift +++ b/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift @@ -3171,7 +3171,13 @@ public final class SettingsStore: ObservableObject { func setCallSiteOverrides(_ overrides: [CallSiteOverride]) -> Task { let validOverrides = overrides.filter { CallSiteCatalog.validIds.contains($0.id) } // Preserve catalog order in the local cache so SwiftUI lists stay stable. - let overrideById = Dictionary(uniqueKeysWithValues: validOverrides.map { ($0.id, $0) }) + // Use `uniquingKeysWith:` (last-write-wins) instead of + // `uniqueKeysWithValues:` to tolerate duplicate IDs from external + // input — the latter traps at runtime on collisions. + let overrideById = Dictionary( + validOverrides.map { ($0.id, $0) }, + uniquingKeysWith: { _, new in new } + ) callSiteOverrides = CallSiteCatalog.all.map { entry in var merged = entry if let provided = overrideById[entry.id] { @@ -3202,6 +3208,20 @@ public final class SettingsStore: ObservableObject { if let profile = entry.profile { rawEntry["profile"] = profile } callSitesPayload[entry.id] = rawEntry } + // Align remote with local: any catalog entry NOT in `validOverrides` + // is locally cleared above (provider/model/profile -> nil), so the + // PATCH must explicitly clear those entries on the daemon as well. + // Without this, omitted entries would appear cleared in the UI but + // the daemon would retain their previous values, and the stale + // values would "reappear" on the next config sync. + let nullClear: [String: Any] = [ + "provider": NSNull(), + "model": NSNull(), + "profile": NSNull(), + ] + for entry in CallSiteCatalog.all where callSitesPayload[entry.id] == nil { + callSitesPayload[entry.id] = nullClear + } let payload: [String: Any] = ["llm": ["callSites": callSitesPayload]] let task = Task { let success = await settingsClient.patchConfig(payload) diff --git a/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift b/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift index af079823521..9422cae10e3 100644 --- a/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift +++ b/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift @@ -311,7 +311,11 @@ final class SettingsStoreCallSiteOverrideTests: XCTestCase { let sites = lastCallSitesPatch() XCTAssertNotNil(sites) - XCTAssertEqual(sites?.count, updates.count) + // Batch PATCH must include every catalog entry: the three caller- + // provided entries plus null-clears for every other catalog ID, so + // the daemon's view stays aligned with the local cache (which + // resets all omitted entries to nil). + XCTAssertEqual(sites?.count, CallSiteCatalog.all.count) let memory = sites?["memoryRetrieval"] as? [String: Any] XCTAssertEqual(memory?["provider"] as? String, "openai") @@ -329,6 +333,112 @@ final class SettingsStoreCallSiteOverrideTests: XCTestCase { XCTAssertTrue(watch?["profile"] is NSNull) } + /// Regression for Devin's review on PR #26128 (`SettingsStore.swift:3174`): + /// `Dictionary(uniqueKeysWithValues:)` traps at runtime when the input + /// contains duplicate keys. `setCallSiteOverrides` accepts external + /// input, so it must be tolerant of duplicates — last-write-wins is the + /// chosen contract. + func testSetCallSiteOverridesToleratesDuplicateIdsLastWriteWins() { + let duplicates: [CallSiteOverride] = [ + CallSiteOverride( + id: "memoryRetrieval", + displayName: "Memory · Retrieval (first)", + domain: .memory, + provider: "openai", + model: "gpt-4.1" + ), + CallSiteOverride( + id: "memoryRetrieval", + displayName: "Memory · Retrieval (second)", + domain: .memory, + provider: "anthropic", + model: "claude-haiku-4" + ), + ] + + // Must not crash. + _ = store.setCallSiteOverrides(duplicates) + waitForPatchCount(1) + + // Last-write-wins in the local cache. + let memory = store.callSiteOverrides.first(where: { $0.id == "memoryRetrieval" }) + XCTAssertEqual(memory?.provider, "anthropic") + XCTAssertEqual(memory?.model, "claude-haiku-4") + XCTAssertNil(memory?.profile) + + // And in the PATCH payload. + let sites = lastCallSitesPatch() + let memoryEntry = sites?["memoryRetrieval"] as? [String: Any] + XCTAssertEqual(memoryEntry?["provider"] as? String, "anthropic") + XCTAssertEqual(memoryEntry?["model"] as? String, "claude-haiku-4") + XCTAssertTrue(memoryEntry?["profile"] is NSNull) + } + + /// Regression for Codex P1 + Devin on PR #26128: prior to the fix, + /// `setCallSiteOverrides` cleared local cache entries omitted from the + /// input but only PATCHed entries that were present. Result: omitted + /// entries appeared cleared in the UI but the daemon retained their + /// previous values, and on the next config sync the stale persisted + /// values would "reappear." The fix aligns remote with local by + /// emitting NSNull clears for every catalog entry not in the input + /// batch. + func testSetCallSiteOverridesBatchClearsOmittedCatalogEntriesOnRemote() { + // Pre-populate two unrelated entries via single-entry writes. + _ = store.setCallSiteOverride("memoryRetrieval", provider: "openai", model: "gpt-4.1") + _ = store.setCallSiteOverride("commitMessage", provider: "anthropic") + waitForPatchCount(2) + + // Now batch-update a SINGLE entry that is neither of the above. + let updates: [CallSiteOverride] = [ + CallSiteOverride( + id: "watchSummary", + displayName: "Watch summary", + domain: .voice, + provider: "openai" + ), + ] + _ = store.setCallSiteOverrides(updates) + waitForPatchCount(3) + + let sites = lastCallSitesPatch() + XCTAssertNotNil(sites) + + // The PATCH must include the new entry verbatim. + let watch = sites?["watchSummary"] as? [String: Any] + XCTAssertEqual(watch?["provider"] as? String, "openai") + XCTAssertTrue(watch?["model"] is NSNull) + XCTAssertTrue(watch?["profile"] is NSNull) + + // And it must include null-clears for the two pre-populated entries + // so the daemon's view matches the (now-cleared) local cache. + let memory = sites?["memoryRetrieval"] as? [String: Any] + XCTAssertNotNil(memory, "PATCH must include null-clear for memoryRetrieval") + XCTAssertTrue(memory?["provider"] is NSNull) + XCTAssertTrue(memory?["model"] is NSNull) + XCTAssertTrue(memory?["profile"] is NSNull) + + let commit = sites?["commitMessage"] as? [String: Any] + XCTAssertNotNil(commit, "PATCH must include null-clear for commitMessage") + XCTAssertTrue(commit?["provider"] is NSNull) + XCTAssertTrue(commit?["model"] is NSNull) + XCTAssertTrue(commit?["profile"] is NSNull) + + // Stronger invariant: the set of IDs PATCHed must equal the full + // catalog. Anything less re-creates the divergence bug. + XCTAssertEqual( + Set(sites?.keys.map { String($0) } ?? []), + CallSiteCatalog.validIds, + "Batch PATCH must cover every catalog entry to keep remote/local aligned" + ) + + // Local cache also reflects the cleared state for the omitted entries. + let cachedMemory = store.callSiteOverrides.first(where: { $0.id == "memoryRetrieval" }) + XCTAssertFalse(cachedMemory?.hasOverride ?? true) + let cachedCommit = store.callSiteOverrides.first(where: { $0.id == "commitMessage" }) + XCTAssertFalse(cachedCommit?.hasOverride ?? true) + XCTAssertEqual(store.overridesCount, 1) + } + func testSetCallSiteOverridesUpdatesLocalCacheInCatalogOrder() { let updates: [CallSiteOverride] = [ CallSiteOverride( @@ -375,9 +485,15 @@ final class SettingsStoreCallSiteOverrideTests: XCTestCase { waitForPatchCount(1) let sites = lastCallSitesPatch() - XCTAssertEqual(sites?.count, 1, "Unknown call-site IDs must be filtered out of the patch") + // The PATCH covers every catalog entry (valid input + null-clears + // for everything else). Unknown IDs from the input must NOT appear. + XCTAssertEqual(sites?.count, CallSiteCatalog.all.count) XCTAssertNotNil(sites?["memoryRetrieval"]) - XCTAssertNil(sites?["totallyMadeUpId"]) + XCTAssertNil(sites?["totallyMadeUpId"], "Unknown call-site IDs must be filtered out of the patch") + + // The valid input is written verbatim. + let memory = sites?["memoryRetrieval"] as? [String: Any] + XCTAssertEqual(memory?["provider"] as? String, "anthropic") } // MARK: - overridesCount derivation From b8cba8cd5f7f5e5cb8665d02c29766bb7aedd1a7 Mon Sep 17 00:00:00 2001 From: siddseethepalli Date: Thu, 16 Apr 2026 21:49:40 +0000 Subject: [PATCH 3/3] fix(callsite-overrides): null entire entry on clear so non-UI leaves get cleared too Codex P2 (PR #26128 cycle 2): clearCallSiteOverride only nulled provider/model/profile, but call-site config supports additional leaves (maxTokens, effort, speed, thinking, contextWindow). If those were set via manual edits, the UI would report cleared while the daemon kept applying hidden overrides. Switch the PATCH payload from { provider: null, model: null, profile: null } to a single null on the entry itself. The Zod fragment treats null as absent, so the resolver falls back to llm.default. Same fix applies to the omitted-catalog-entry clears in setCallSiteOverrides batch. Tests updated to assert the new shape. --- .../Features/Settings/SettingsStore.swift | 40 +++++++++---------- .../SettingsStoreCallSiteOverrideTests.swift | 33 +++++++-------- 2 files changed, 33 insertions(+), 40 deletions(-) diff --git a/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift b/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift index eedfdd82168..99e27384596 100644 --- a/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift +++ b/clients/macos/vellum-assistant/Features/Settings/SettingsStore.swift @@ -3119,18 +3119,17 @@ public final class SettingsStore: ObservableObject { return task } - /// Clears the override for a single call site by writing - /// `{ provider: null, model: null, profile: null }` to - /// `llm.callSites.`. The daemon's PATCH handler uses - /// `deepMergeOverwrite`, which assigns `null` leaves rather than - /// deleting the key, so the on-disk shape becomes - /// `{ "llm": { "callSites": { "": { "provider": null, ... } } } }`. + /// Clears the override for a single call site by writing `null` to + /// `llm.callSites.` itself, which the Zod fragment treats as + /// "absent" and the resolver falls back to `llm.default`. /// - /// The Zod fragment schema treats `null` as equivalent to "absent" for - /// these optional fields (parsed as `undefined`), which is what the - /// resolver consumes. If a true key-deletion semantic is needed in - /// the future, a dedicated DELETE endpoint should be added — see - /// the PR body for the proposed follow-up. + /// We null the entire entry rather than just `provider`/`model`/`profile` + /// because `LLMCallSiteConfig` supports additional leaves + /// (`maxTokens`, `effort`, `speed`, `temperature`, `thinking`, + /// `contextWindow`) that may have been set via manual config edits or + /// other clients. Clearing only the three Settings-UI-managed fields + /// would leave hidden overrides applied — the user couldn't truly + /// reset the call site. @discardableResult func clearCallSiteOverride(_ id: String) -> Task { guard CallSiteCatalog.validIds.contains(id) else { @@ -3142,12 +3141,9 @@ public final class SettingsStore: ObservableObject { callSiteOverrides[index].model = nil callSiteOverrides[index].profile = nil } - let entry: [String: Any] = [ - "provider": NSNull(), - "model": NSNull(), - "profile": NSNull(), + let payload: [String: Any] = [ + "llm": ["callSites": [id: NSNull()]], ] - let payload: [String: Any] = ["llm": ["callSites": [id: entry]]] let task = Task { let success = await settingsClient.patchConfig(payload) if !success { @@ -3214,13 +3210,13 @@ public final class SettingsStore: ObservableObject { // Without this, omitted entries would appear cleared in the UI but // the daemon would retain their previous values, and the stale // values would "reappear" on the next config sync. - let nullClear: [String: Any] = [ - "provider": NSNull(), - "model": NSNull(), - "profile": NSNull(), - ] + // + // Null the entire `callSites.` entry (rather than just the three + // Settings-managed fields) so any other leaves an entry might have + // — `maxTokens`, `effort`, `speed`, `thinking`, `contextWindow` — + // are cleared too. Same rationale as `clearCallSiteOverride`. for entry in CallSiteCatalog.all where callSitesPayload[entry.id] == nil { - callSitesPayload[entry.id] = nullClear + callSitesPayload[entry.id] = NSNull() } let payload: [String: Any] = ["llm": ["callSites": callSitesPayload]] let task = Task { diff --git a/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift b/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift index 9422cae10e3..d8cbffe4436 100644 --- a/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift +++ b/clients/macos/vellum-assistantTests/SettingsStoreCallSiteOverrideTests.swift @@ -221,7 +221,7 @@ final class SettingsStoreCallSiteOverrideTests: XCTestCase { // MARK: - Single-entry clear - func testClearCallSiteOverrideEmitsNullLeavesAndClearsLocalCache() { + func testClearCallSiteOverrideNullsEntireEntryAndClearsLocalCache() { _ = store.setCallSiteOverride( "memoryRetrieval", provider: "openai", @@ -232,12 +232,13 @@ final class SettingsStoreCallSiteOverrideTests: XCTestCase { _ = store.clearCallSiteOverride("memoryRetrieval") waitForPatchCount(2) + // The whole `callSites.` entry is nulled (not just provider/ + // model/profile leaves) so any other config leaves the entry might + // have — maxTokens, effort, speed, thinking, contextWindow — get + // cleared too. Per Codex PR #26128 cycle 2 P2. let sites = lastCallSitesPatch() - let memory = sites?["memoryRetrieval"] as? [String: Any] - XCTAssertNotNil(memory) - XCTAssertTrue(memory?["provider"] is NSNull) - XCTAssertTrue(memory?["model"] is NSNull) - XCTAssertTrue(memory?["profile"] is NSNull) + XCTAssertNotNil(sites?["memoryRetrieval"]) + XCTAssertTrue(sites?["memoryRetrieval"] is NSNull) let cached = store.callSiteOverrides.first(where: { $0.id == "memoryRetrieval" }) XCTAssertNil(cached?.provider) @@ -410,18 +411,14 @@ final class SettingsStoreCallSiteOverrideTests: XCTestCase { XCTAssertTrue(watch?["profile"] is NSNull) // And it must include null-clears for the two pre-populated entries - // so the daemon's view matches the (now-cleared) local cache. - let memory = sites?["memoryRetrieval"] as? [String: Any] - XCTAssertNotNil(memory, "PATCH must include null-clear for memoryRetrieval") - XCTAssertTrue(memory?["provider"] is NSNull) - XCTAssertTrue(memory?["model"] is NSNull) - XCTAssertTrue(memory?["profile"] is NSNull) - - let commit = sites?["commitMessage"] as? [String: Any] - XCTAssertNotNil(commit, "PATCH must include null-clear for commitMessage") - XCTAssertTrue(commit?["provider"] is NSNull) - XCTAssertTrue(commit?["model"] is NSNull) - XCTAssertTrue(commit?["profile"] is NSNull) + // so the daemon's view matches the (now-cleared) local cache. The + // whole entry is nulled (not just provider/model/profile leaves) per + // PR #26128 cycle 2 fix — clears any other leaves the entry may have. + XCTAssertNotNil(sites?["memoryRetrieval"], "PATCH must include null-clear for memoryRetrieval") + XCTAssertTrue(sites?["memoryRetrieval"] is NSNull) + + XCTAssertNotNil(sites?["commitMessage"], "PATCH must include null-clear for commitMessage") + XCTAssertTrue(sites?["commitMessage"] is NSNull) // Stronger invariant: the set of IDs PATCHed must equal the full // catalog. Anything less re-creates the divergence bug.