fix(desktop): Phase 1 round-5 — make FIELD_OWNERS load-bearing, revert mobile lockfile

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 <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
Duncan
2026-08-08 18:12:58 -04:00
co-authored by Will Pfleger
parent 04c4c426e6
commit b91d17e9e4
9 changed files with 258 additions and 52 deletions
+1 -1
View File
@@ -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
@@ -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;
@@ -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],
@@ -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.
*
@@ -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";
@@ -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.
*/
@@ -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",
);
});
+104 -36
View File
@@ -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<keyof AgentFormModel, FieldOwner> = {
// Identity
@@ -176,6 +173,57 @@ export const FIELD_OWNERS: Record<keyof AgentFormModel, FieldOwner> = {
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 =
+8 -8
View File
@@ -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: