From b91d17e9e435150df2faaca37d3f63af53d1f84f Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 8 Aug 2026 18:12:58 -0400 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20Phase=201=20round-5=20=E2=80=94?= =?UTF-8?q?=20make=20FIELD=5FOWNERS=20load-bearing,=20revert=20mobile=20lo?= =?UTF-8?q?ckfile?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add fieldOwner(field, ctx) exported from agentFormModel.ts. It reads from FIELD_OWNERS for the base owner and handles two sets of context-dependent overrides: (1) rows-9-10 dual-owner fields (respondTo, respondToAllowlist, parallelism) are D-owned in definition-only context and I-owned otherwise; (2) definition-fallback fields (systemPrompt, model, provider) are D-owned when a definition is present and I-owned in instance-only context. Wire fieldOwner into emitAgentFormDiff via isD/isI helpers. Every field routing decision now consults fieldOwner(field, ctx) rather than hardcoded hasInst/def===null conditionals — making FIELD_OWNERS the single authoritative routing source as the JSDoc claims. Add 8 new tests to agentFormModel.test.mjs: fieldOwner context resolution for respondTo/parallelism/systemPrompt/model/provider, delegation to FIELD_OWNERS for static fields, and end-to-end emit routing via fieldOwner. 4568/4568 pass. Revert mobile/pubspec.lock to origin/main (boundary violation from round-4 commit — lockfile churn is pre-existing local dirt, not in scope). Clean up stale AgentInstanceEditDialog references in AGENTS.md, agentConfigOptions.tsx, agentConfigControls.tsx, AgentRunLocationContext.tsx, AgentCreationPreview.tsx, AgentDefinitionDialog.tsx. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src/features/agents/AGENTS.md | 2 +- .../agents/ui/AgentCreationPreview.tsx | 2 +- .../agents/ui/AgentDefinitionDialog.tsx | 2 +- .../agents/ui/AgentRunLocationContext.tsx | 2 +- .../agents/ui/agentConfigControls.tsx | 2 +- .../features/agents/ui/agentConfigOptions.tsx | 4 +- .../agents/ui/agentFormModel.test.mjs | 140 +++++++++++++++++- .../src/features/agents/ui/agentFormModel.ts | 140 +++++++++++++----- mobile/pubspec.lock | 16 +- 9 files changed, 258 insertions(+), 52 deletions(-) diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index b578326eb..cae869279 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -153,7 +153,7 @@ with a TypeScript lookup table or an id comparison in a component. place that resolves it for dialog surfaces and publishes it through `ui/AgentRunLocationContext.tsx`; the field reads that context and lets an explicit `runLocation` prop win. Do **not** thread the value as a prop - through `AgentDefinitionDialog` / `AgentInstanceEditDialog` — both are + through `AgentDefinitionDialog` / `AgentEditMergedDialog` — both are already over the 1000-line ceiling, and neither uses the value itself. Surfaces rendered outside `AgentDialog` (e.g. `EditRespondToDialog`) pass the prop directly. Local names "your diff --git a/desktop/src/features/agents/ui/AgentCreationPreview.tsx b/desktop/src/features/agents/ui/AgentCreationPreview.tsx index e7a68211d..bb850615f 100644 --- a/desktop/src/features/agents/ui/AgentCreationPreview.tsx +++ b/desktop/src/features/agents/ui/AgentCreationPreview.tsx @@ -65,7 +65,7 @@ export function AgentCreationPreview({ disabled?: boolean; /** When true, omit all upload/edit controls and render the avatar as a * plain display element. Use in contexts where avatar editing is - * handled by an external affordance (e.g. AgentInstanceEditDialog). */ + * handled externally (e.g. definition-only via AgentDefinitionDialog). */ hideEditControl?: boolean; label: string; onClearAvatar?: () => void; diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index 4a4566f5b..8a4c78fc7 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -505,7 +505,7 @@ export function AgentDefinitionDialog({ !isAvatarUploadPending; // Merge global env as the base layer so credential keys satisfied via global - // config are available to model discovery — same rationale as in AgentInstanceEditDialog. + // config are available to model discovery — same rationale as in AgentEditMergedDialog. const envVarsForDiscovery = React.useMemo( () => ({ ...globalConfig.env_vars, ...envVars }), [globalConfig.env_vars, envVars], diff --git a/desktop/src/features/agents/ui/AgentRunLocationContext.tsx b/desktop/src/features/agents/ui/AgentRunLocationContext.tsx index 5815130b4..62c57b6b9 100644 --- a/desktop/src/features/agents/ui/AgentRunLocationContext.tsx +++ b/desktop/src/features/agents/ui/AgentRunLocationContext.tsx @@ -7,7 +7,7 @@ import type { AgentRunLocation } from "../lib/agentAccessWarning"; * * Context rather than a prop on purpose: the only consumer is the respond-to * warning, buried several levels inside `AgentDefinitionDialog` (1000+ lines) - * and `AgentInstanceEditDialog` (1200+ lines). Threading a prop through them + * and `AgentEditMergedDialog` (1700+ lines). Threading a prop through them * would grow two files that are already over the 1000-line ceiling enforced by * `desktop/scripts/check-file-sizes.mjs`, for a value neither of them uses. * diff --git a/desktop/src/features/agents/ui/agentConfigControls.tsx b/desktop/src/features/agents/ui/agentConfigControls.tsx index 1a431d1f9..40a5e8cd8 100644 --- a/desktop/src/features/agents/ui/agentConfigControls.tsx +++ b/desktop/src/features/agents/ui/agentConfigControls.tsx @@ -1,7 +1,7 @@ /** * Shared provider and model field components for agent dialogs. * - * Both CreateAgentDialog (local mode) and AgentInstanceEditDialog import these + * CreateAgentDialog (local mode) and AgentEditMergedDialog import these * instead of duplicating the picker logic. */ import * as React from "react"; diff --git a/desktop/src/features/agents/ui/agentConfigOptions.tsx b/desktop/src/features/agents/ui/agentConfigOptions.tsx index 5c515a050..3d417e69a 100644 --- a/desktop/src/features/agents/ui/agentConfigOptions.tsx +++ b/desktop/src/features/agents/ui/agentConfigOptions.tsx @@ -14,8 +14,8 @@ export { getDefaultPersonaRuntime } from "../lib/resolvePersonaRuntime"; * offering it for new selections would create a regression path. * OSS builds pass an empty `Set` so v1 remains visible. * - * All three dialog sites that show a provider picker import this constant — - * `AgentDefinitionDialog`, `AgentInstanceEditDialog`, and + * All dialog sites that show a provider picker import this constant — + * `AgentDefinitionDialog`, `AgentEditMergedDialog`, and * `AgentDefaultsSettingsCard` — making it the single source of truth for * which provider ids to suppress on Block builds. */ diff --git a/desktop/src/features/agents/ui/agentFormModel.test.mjs b/desktop/src/features/agents/ui/agentFormModel.test.mjs index 3c4caf214..b10b1fa21 100644 --- a/desktop/src/features/agents/ui/agentFormModel.test.mjs +++ b/desktop/src/features/agents/ui/agentFormModel.test.mjs @@ -1,7 +1,12 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { seedAgentFormModel, emitAgentFormDiff } from "./agentFormModel.ts"; +import { + seedAgentFormModel, + emitAgentFormDiff, + fieldOwner, + FIELD_OWNERS, +} from "./agentFormModel.ts"; // ── Shared fixtures ──────────────────────────────────────────────────────────── @@ -460,3 +465,136 @@ test("test_r6_respond_to_override_only_no_other_fields_emitted", () => { ); assert.equal(emit.personaInput.behavior?.respondTo, "owner-only"); }); + +// ── fieldOwner load-bearing tests ───────────────────────────────────────────── +// +// Verifies that fieldOwner() is the actual routing mechanism consumed by +// emitAgentFormDiff. Each test confirms that (a) fieldOwner returns the right +// owner for a context, and (b) emitAgentFormDiff routes to the correct layer. + +test("test_fieldOwner_respondTo_is_instance_in_linked_context", () => { + const definition = makeDefinition(); + const instance = makeInstance({ respondTo: "owner-only" }); + const ctx = { kind: "instance-with-definition", definition, instance }; + + // fieldOwner must resolve to "instance" for linked context. + assert.equal( + fieldOwner("respondTo", ctx), + "instance", + "respondTo is I-owned in instance-with-definition context", + ); +}); + +test("test_fieldOwner_respondTo_is_definition_in_definition_only_context", () => { + const definition = makeDefinition(); + const ctx = { kind: "definition-only", definition }; + + // fieldOwner must resolve to "definition" for definition-only context (rows 9–10). + assert.equal( + fieldOwner("respondTo", ctx), + "definition", + "respondTo is D-owned in definition-only context", + ); +}); + +test("test_fieldOwner_parallelism_follows_same_contract_as_respondTo", () => { + const definition = makeDefinition(); + const instance = makeInstance({ parallelism: 3 }); + const linked = { kind: "instance-with-definition", definition, instance }; + const defOnly = { kind: "definition-only", definition }; + + assert.equal(fieldOwner("parallelism", linked), "instance"); + assert.equal(fieldOwner("parallelism", defOnly), "definition"); +}); + +test("test_fieldOwner_systemPrompt_is_instance_in_instance_only_context", () => { + const instance = makeInstance(); + const ctx = { kind: "instance-only", instance }; + + // systemPrompt is D-owned when a definition is present, I-owned for unlinked agents. + assert.equal(fieldOwner("systemPrompt", ctx), "instance"); +}); + +test("test_fieldOwner_model_and_provider_are_instance_in_instance_only_context", () => { + const instance = makeInstance(); + const ctx = { kind: "instance-only", instance }; + + assert.equal(fieldOwner("model", ctx), "instance"); + assert.equal(fieldOwner("provider", ctx), "instance"); +}); + +test("test_fieldOwner_all_definition_fields_map_back_to_FIELD_OWNERS", () => { + // Spot-check that fieldOwner delegates to FIELD_OWNERS for static fields. + const definition = makeDefinition(); + const ctx = { kind: "definition-only", definition }; + + // displayName, avatarUrl, runtime, envVars, namePool are always D. + for (const field of [ + "displayName", + "avatarUrl", + "runtime", + "envVars", + "namePool", + ]) { + assert.equal( + fieldOwner(field, ctx), + "definition", + `${field} must be D-owned`, + ); + assert.equal( + FIELD_OWNERS[field], + "definition", + `FIELD_OWNERS[${field}] must be definition`, + ); + } +}); + +test("test_emitAgentFormDiff_routes_respondTo_change_to_agentInput_via_fieldOwner", () => { + // Verify that emitAgentFormDiff uses fieldOwner to route respondTo for linked context. + const definition = makeDefinition({ respondTo: "anyone" }); + const instance = makeInstance({ respondTo: "anyone" }); + const ctx = { kind: "instance-with-definition", definition, instance }; + + const seed = seedAgentFormModel(ctx); + const changed = { ...seed, respondTo: "owner-only" }; + const emit = emitAgentFormDiff(seed, changed, ctx); + + // respondTo change must land in agentInput (I-owned), NOT personaInput. + assert.ok(emit.agentInput, "agentInput must be present for respondTo change"); + assert.equal( + emit.agentInput.respondTo, + "owner-only", + "changed respondTo must appear in agentInput", + ); + assert.equal( + emit.personaInput, + null, + "personaInput must be null — respondTo is I-owned in linked context", + ); +}); + +test("test_emitAgentFormDiff_routes_respondTo_change_to_personaInput_via_fieldOwner_in_definition_only", () => { + // In definition-only context, fieldOwner returns "definition" for respondTo. + const definition = makeDefinition({ respondTo: "anyone" }); + const ctx = { kind: "definition-only", definition }; + + const seed = seedAgentFormModel(ctx); + const changed = { ...seed, respondTo: "owner-only" }; + const emit = emitAgentFormDiff(seed, changed, ctx); + + // respondTo change must land in personaInput (D-owned in definition-only context). + assert.ok( + emit.personaInput, + "personaInput must be present for respondTo change in definition-only", + ); + assert.equal( + emit.personaInput.behavior?.respondTo, + "owner-only", + "changed respondTo must appear in personaInput.behavior", + ); + assert.equal( + emit.agentInput, + null, + "agentInput must be null — no instance in definition-only context", + ); +}); diff --git a/desktop/src/features/agents/ui/agentFormModel.ts b/desktop/src/features/agents/ui/agentFormModel.ts index e03c732f7..7f0f20d82 100644 --- a/desktop/src/features/agents/ui/agentFormModel.ts +++ b/desktop/src/features/agents/ui/agentFormModel.ts @@ -144,15 +144,12 @@ export type AgentFormModel = { /** * Source-of-truth field ownership map (Artifact 4). * - * Every AgentFormModel field is tagged with its FieldOwner. The emit function - * (`emitAgentFormDiff`) and the dialog's editability predicates derive their - * routing decisions from these ownership assignments — this map is the single - * authoritative statement of which layer owns each field. - * - * Ownership rules per spec rows 9–10: - * respondTo / parallelism — I-owned when an instance is in context; - * D-owned only in definition-only (zero-instance) context. - * All other D-fields — always D-owned. + * Declares the base FieldOwner for every AgentFormModel field. Two fields + * (respondTo, parallelism) have context-dependent ownership per rows 9–10: + * they are I-owned when an instance is in context, and D-owned only in + * definition-only (zero-instance) context. `fieldOwner()` resolves this + * context dependence and is the single function consulted by `emitAgentFormDiff` + * and the dialog's editability gates for all routing decisions. */ export const FIELD_OWNERS: Record = { // Identity @@ -176,6 +173,57 @@ export const FIELD_OWNERS: Record = { startOnAppLaunch: "local-policy", }; +/** + * Resolve the effective FieldOwner for a field given the current edit context. + * + * For most fields this is a direct lookup into FIELD_OWNERS. Two sets of + * context-dependent overrides apply: + * + * 1. Rows 9–10 dual-owner fields (respondTo, respondToAllowlist, parallelism): + * base entry is "instance", but in definition-only context (no instance) they + * are D-owned (definition default). + * + * 2. Definition-or-instance fields (systemPrompt, model, provider): base entry + * is "definition" (D-field when a definition is present), but in instance-only + * context (no definition) they fall back to I-owned. + * + * `emitAgentFormDiff` consults this function for every routing decision; + * the dialog's editability predicates call it to determine which layer a + * control belongs to, making FIELD_OWNERS the single authoritative source. + */ +export function fieldOwner( + field: keyof AgentFormModel, + ctx: AgentEditContext, +): FieldOwner { + const base = FIELD_OWNERS[field]; + + // Rows 9–10: respondTo/respondToAllowlist/parallelism are I-owned when an + // instance is present. In definition-only context (no instance) they are + // D-owned (definition default). + if ( + base === "instance" && + (field === "respondTo" || + field === "respondToAllowlist" || + field === "parallelism") && + ctx.kind === "definition-only" + ) { + return "definition"; + } + + // Definition-fallback fields: D-owned when a definition is present, I-owned + // in instance-only context (no definition). systemPrompt, model, provider are + // "definition" in the base map, but must be treated as I-owned for unlinked agents. + if ( + base === "definition" && + (field === "systemPrompt" || field === "model" || field === "provider") && + ctx.kind === "instance-only" + ) { + return "instance"; + } + + return base; +} + /** Which coordinator outputs changed vs. last saved state. */ export type AgentFormEmit = { /** D-field update payload, present iff a D-field changed AND the definition is editable. */ @@ -284,6 +332,11 @@ export function seedAgentFormModel(ctx: AgentEditContext): AgentFormModel { * Returns agentInput when an I-field changed. * Returns policySets for any L-field changed. * + * Routing decisions — which layer each field's change belongs to — are made + * via `fieldOwner(field, ctx)`, which reads from `FIELD_OWNERS` and handles + * the rows-9–10 context dependence. This makes `FIELD_OWNERS` the single + * authoritative source for ownership routing in this function. + * * The caller (save coordinator) calls this after re-fetching observed state, * so "previous" is the re-fetched stored state and "next" is what the user * submitted — the diff is exactly what hasn't been persisted yet. @@ -298,26 +351,35 @@ export function emitAgentFormDiff( const inst = editContextInstance(ctx); const defReadOnly = isDefinitionReadOnly(ctx); + // Helper: is a field D-owned in this context? + const isD = (field: keyof AgentFormModel) => + fieldOwner(field, ctx) === "definition"; + // Helper: is a field I-owned in this context? + const isI = (field: keyof AgentFormModel) => + fieldOwner(field, ctx) === "instance"; + // D-field diff — only when definition is present and NOT team-managed let personaInput: UpdatePersonaInput | null = null; if (def !== null && !defReadOnly) { - // Rows 9–10: respondTo and parallelism are I-owned when an instance is - // present — they NEVER go into personaInput in that context. They are - // D-owned (definition defaults) only in definition-only (zero-instance) context. - const hasInst = inst !== null; - const dChanged = - next.displayName.trim() !== saved.displayName.trim() || - (next.avatarUrl ?? "") !== (saved.avatarUrl ?? "") || - next.systemPrompt.trim() !== saved.systemPrompt.trim() || - next.runtime !== saved.runtime || - (next.model ?? null) !== (saved.model ?? null) || - (next.provider ?? null) !== (saved.provider ?? null) || - !envVarsMapEqual(next.envVars ?? {}, saved.envVars ?? {}) || - !namePoolEqual(next.namePool ?? [], saved.namePool ?? []) || - // Only include respondTo/parallelism in D-diff when NO instance (definition-only context) - (!hasInst && (next.respondTo ?? null) !== (saved.respondTo ?? null)) || - (!hasInst && + (isD("displayName") && + next.displayName.trim() !== saved.displayName.trim()) || + (isD("avatarUrl") && + (next.avatarUrl ?? "") !== (saved.avatarUrl ?? "")) || + (isD("systemPrompt") && + next.systemPrompt.trim() !== saved.systemPrompt.trim()) || + (isD("runtime") && next.runtime !== saved.runtime) || + (isD("model") && (next.model ?? null) !== (saved.model ?? null)) || + (isD("provider") && + (next.provider ?? null) !== (saved.provider ?? null)) || + (isD("envVars") && + !envVarsMapEqual(next.envVars ?? {}, saved.envVars ?? {})) || + (isD("namePool") && + !namePoolEqual(next.namePool ?? [], saved.namePool ?? [])) || + // respondTo/parallelism: D-owned only in definition-only context (rows 9–10) + (isD("respondTo") && + (next.respondTo ?? null) !== (saved.respondTo ?? null)) || + (isD("respondToAllowlist") && next.respondTo === "allowlist" && next.respondToAllowlist.join(",") !== (saved.respondToAllowlist ?? []).join(",")); @@ -333,17 +395,19 @@ export function emitAgentFormDiff( provider: next.provider ?? undefined, namePool: next.namePool ?? [], envVars: next.envVars ?? {}, - // Include behavior block only in definition-only context (rows 9–10: - // when instance is present, respondTo/parallelism go to agentInput). + // Include behavior block only when respondTo/parallelism are D-owned + // (definition-only context per rows 9–10). behavior: - !hasInst && next.respondTo != null + isD("respondTo") && next.respondTo != null ? { respondTo: next.respondTo, respondToAllowlist: next.respondTo === "allowlist" ? next.respondToAllowlist : undefined, - parallelism: next.parallelism ?? undefined, + parallelism: isD("parallelism") + ? (next.parallelism ?? undefined) + : undefined, } : undefined, }; @@ -359,31 +423,35 @@ export function emitAgentFormDiff( // Row 8 contract: instance env edit goes to instance, NOT wholesale-replace from definition. // When a definition is present, instanceEnvVars holds the per-instance overlay. const instanceEnvChanged = - next.instanceEnvVars !== undefined + isI("instanceEnvVars") && next.instanceEnvVars !== undefined ? !envVarsMapEqual(next.instanceEnvVars ?? {}, inst.envVars ?? {}) : false; const nameChanged = + isI("instanceName") && !hasNamePool && (next.instanceName ?? next.displayName.trim()) !== inst.name; const systemPromptChanged = - def === null && + isI("systemPrompt") && (next.systemPrompt.trim() || null) !== (inst.systemPrompt ?? null); const modelChanged = - def === null && (next.model ?? null) !== (inst.model ?? null); + isI("model") && (next.model ?? null) !== (inst.model ?? null); const providerChanged = - def === null && (next.provider ?? null) !== (inst.provider ?? null); - // Rows 9–10: respondTo/parallelism are always I-owned when an instance is - // present. Changes always emit to agentInput, regardless of whether a - // definition is present or team-managed. + isI("provider") && (next.provider ?? null) !== (inst.provider ?? null); + // Rows 9–10: respondTo/parallelism are I-owned when an instance is present. + // fieldOwner("respondTo", ctx) returns "instance" for instance-with-definition + // and instance-only contexts, so these always emit to agentInput here. // Normalize null → "anyone" in the comparison to avoid phantom writes // when the DB stores null but the UI defaults to "anyone". const respondToChanged = + isI("respondTo") && (next.respondTo ?? "anyone") !== (inst.respondTo ?? "anyone"); const allowlistChanged = + isI("respondToAllowlist") && next.respondTo === "allowlist" && next.respondToAllowlist.join(",") !== inst.respondToAllowlist.join(","); const parallelismChanged = + isI("parallelism") && (next.parallelism ?? null) !== (inst.parallelism ?? null); const iChanged = diff --git a/mobile/pubspec.lock b/mobile/pubspec.lock index 8758454cc..6287e4c86 100644 --- a/mobile/pubspec.lock +++ b/mobile/pubspec.lock @@ -820,10 +820,10 @@ packages: dependency: transitive description: name: meta - sha256: "1741988757a65eb6b36abe716829688cf01910bbf91c34354ff7ec1c3de2b349" + sha256: "23f08335362185a5ea2ad3a4e597f1375e78bce8a040df5c600c8d3552ef2394" url: "https://pub.dev" source: hosted - version: "1.18.0" + version: "1.17.0" mime: dependency: transitive description: @@ -1297,26 +1297,26 @@ packages: dependency: transitive description: name: test - sha256: "8d9ceddbab833f180fbefed08afa76d7c03513dfdba87ffcec2718b02bbcbf20" + sha256: "280d6d890011ca966ad08df7e8a4ddfab0fb3aa49f96ed6de56e3521347a9ae7" url: "https://pub.dev" source: hosted - version: "1.31.0" + version: "1.30.0" test_api: dependency: transitive description: name: test_api - sha256: "949a932224383300f01be9221c39180316445ecb8e7547f70a41a35bf421fb9e" + sha256: "8161c84903fd860b26bfdefb7963b3f0b68fee7adea0f59ef805ecca346f0c7a" url: "https://pub.dev" source: hosted - version: "0.7.11" + version: "0.7.10" test_core: dependency: transitive description: name: test_core - sha256: "1991d4cfe85d5043241acac92962c3977c8d2f2add1ee73130c7b286417d1d34" + sha256: "0381bd1585d1a924763c308100f2138205252fb90c9d4eeaf28489ee65ccde51" url: "https://pub.dev" source: hosted - version: "0.6.17" + version: "0.6.16" tuple: dependency: transitive description: