fix(desktop): stop the instance form offering AI-config edits it discards - #4206
fix(desktop): stop the instance form offering AI-config edits it discards#4206fitz2882 wants to merge 1 commit into
Conversation
…ards ## Summary block#1968 (`8c0e8cb16`) made the linked definition authoritative for model, LLM provider, and system prompt: `resolve_effective_config` reads all three from the definition and never consults the instance record. The write path moved; the controls did not. `AgentInstanceEditDialog` kept rendering Model and LLM provider as live dropdowns, prefilled from the definition, while `handleSubmit` omitted them and `update_managed_agent` dropped them again server-side (`record.persona_id.is_some()` early return). Editing either reported a successful save and changed nothing, so the value had to be set a second time in the definition dialog — the "you have to edit both places" report. - `personaRuntimeModel.ts`: add `definitionOwnsAiConfig`, the single predicate for "the definition, not this instance, owns these fields". The submit omissions and the control state now derive from it instead of two independent `linkedPersona != null` checks, which is exactly how they drifted apart. - `AgentInstanceEditDialog.tsx`: Model and LLM provider (and the custom- value inputs behind both) go read-only when a definition owns them. They stay visible — the value answers "what does this agent run on" — but cannot be edited where the edit would be dropped. - `AgentAiDefaults.tsx`: `AgentAiDefaultsNotice` takes an optional `onEditDefinition`. When set it names the definition as the source and adds an "Edit agent definition" link, the same hand-off "Edit avatar" already makes. A disabled control with no route to the real one is the same dead end in a different costume. - A definition-owned provider no longer gates Save. Switching harness re-derives the provider draft and can blank it, which would otherwise leave Save disabled on a read-only field the user cannot repair, over a value this form never sends. - The Model field moves to `EditAgentModelField`, mirroring `PersonaModelField` on the definition form. `AgentInstanceEditDialog` is already 228 lines over the repo's 1000-line cap, and the file-size ratchet forbids growth. Definition-level respond-to and parallelism are a separate gap (they reach an instance at mint time only) and are not touched here. ## Validation At this commit, hermit toolchain: - `just desktop-check` — clean; the 2 `useTemplate` infos are pre-existing in `personaCatalogRelay.test.mjs`, untouched here, and present at base `ac4fa13b8` - `just desktop-typecheck` — clean - `just desktop-test` — 3914 passed, 0 failed (8 new) - `just desktop-build` — succeeds; new copy and the `edit-agent-definition` testid confirmed present in `desktop/dist` - `desktop/scripts/check-file-sizes.mjs` passes: 1228 -> 1211 lines New contract tests (`definitionOwnedAiConfig.test.mjs`) pin the predicate and both seams, so a future change that re-derives either one on its own fails rather than silently re-opening the same hole. Not verified: the rendered dialog. No screenshot — the desktop E2E mock bridge cannot seed a linked managed agent for this surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: David Fitzsimmons <dave_fitz@icloud.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0a9982aa1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : null, | ||
| [agent.personaId, personasQuery.data], | ||
| ); | ||
| const aiConfigIsDefinitionOwned = definitionOwnsAiConfig(linkedPersona); |
There was a problem hiding this comment.
When the persona query has no data—because it is still loading, failed, or the instance is orphaned—linkedPersona is null even though agent.personaId remains set. The backend write guard keys solely on record.persona_id.is_some(), so this predicate enables model/provider controls and includes their changes in a save that silently discards them, preserving the exact dead-control behavior this change intends to remove. Derive ownership from agent.personaId and use the resolved persona separately for display/navigation.
Useful? React with 👍 / 👎.
| disabled={aiFieldsDisabled} | ||
| discoveryLoading={modelDiscoveryLoading} | ||
| isRequired={modelRequired} | ||
| model={model} |
There was a problem hiding this comment.
When a definition's model or provider changes after an instance was created, the instance record intentionally retains stale create-time snapshots, but these newly read-only controls still receive model/provider state initialized from agent.model and agent.provider. Consequently the dialog claims to show what the agent runs on while displaying the old snapshot—and the summary below uses the same stale submission values—rather than linkedPersona.model and linkedPersona.provider that the backend actually resolves.
Useful? React with 👍 / 👎.
| export function definitionOwnsAiConfig( | ||
| linkedPersona: unknown | null | undefined, | ||
| ): boolean { | ||
| return linkedPersona != null; |
There was a problem hiding this comment.
This introduces a new ownership predicate and changes how model/provider configuration is rendered and persisted, but the scoped contributor guide is unchanged. It explicitly requires configuration behavior changes to update that guide in the same PR and directs reviewers to treat a config-behavior diff without the update (or an explicit no-rules-changed note) as incomplete; document the new definition-ownership invariant and its query-state behavior.
AGENTS.md reference: desktop/src/features/agents/AGENTS.md:L177-L184
Useful? React with 👍 / 👎.
1 participant
Summary
#1968 (
8c0e8cb16) made the linked definition authoritative for model, LLM provider, and system prompt:resolve_effective_configreads all three from the definition and never consults the instance record. The write path moved; the controls did not.AgentInstanceEditDialogstill renders Model and LLM provider as live dropdowns, prefilled from the definition, whilehandleSubmitomits them for a linked instance andupdate_managed_agentdrops them again server-side (apply_model_provider_prompt_updatereturns early onrecord.persona_id.is_some()). Editing either one reports a successful save and changes nothing, so the value has to be set a second time in the definition dialog.Reported as "there are two places to edit an agent and you have to edit both."
personaRuntimeModel.ts: adddefinitionOwnsAiConfig, one predicate for "the definition, not this instance, owns these fields". The submit omissions and the control state now both derive from it instead of two independentlinkedPersona != nullchecks — which is exactly how they drifted apart.AgentInstanceEditDialog.tsx: Model and LLM provider, and the custom-value inputs behind both, go read-only when a definition owns them. They stay visible — the value answers "what does this agent run on" — but cannot be edited where the edit would be discarded.AgentAiDefaults.tsx:AgentAiDefaultsNoticetakes an optionalonEditDefinition. When set it names the definition as the source and adds an Edit agent definition link — the same hand-off "Edit avatar" already makes. A disabled control with no route to the real one is the same dead end in a different costume.EditAgentModelField, mirroringPersonaModelFieldon the definition form.AgentInstanceEditDialogis already 228 lines over the 1000-line cap and the file-size ratchet forbids growth; 1228 → 1211.Definition-level respond-to and parallelism are a separate gap — they reach an instance at mint time only — and are not touched here.
Validation
Hermit toolchain, at
a0a9982a:just desktop-check— clean. The 2useTemplateinfos are pre-existing inpersonaCatalogRelay.test.mjs, untouched here and present at baseac4fa13b8.just desktop-typecheck— cleanjust desktop-test— 3914 passed, 0 failed (8 new)just desktop-build— succeeds; the new copy and theedit-agent-definitiontestid are present indesktop/distdesktop/scripts/check-file-sizes.mjspassesNew contract tests (
definitionOwnedAiConfig.test.mjs) pin the predicate and both seams, so a future change that re-derives either one independently fails rather than silently reopening the same hole.Not verified: the rendered dialog. No screenshot — the desktop E2E mock bridge cannot seed a linked managed agent for this surface, so verification stops at the built bundle.
Test plan