feat(pikchr): configurable agent, model, and effort for diagram sub-sessions - #896
Merged
Conversation
…essions The generate_pikchr diagram sub-session always inherited the invoking session's agent and ran at that agent's default model/effort (its driver.run() was handed no config options). Let the user pin diagram generation to a specific agent — and that agent's model and effort — independent of the session that triggered the tool, via a new "Diagram generation" control in General settings. - acp_config: add DiagramSubsessionConfig and read_diagram_subsession_config(), reading the `diagram-subsession-config` preference key (mirroring how branch-prefix is read straight from preferences.json). Model/effort are applied only alongside a configured provider, since their value ids are provider-specific. - pikchr_mcp / pikchr_subsession: resolve the effective provider and thread the model/effort selections into every driver.run() turn (including resumed turns); fall back to the invoking session's agent at its defaults when unset, reproducing the prior behaviour. - frontend: persist the override through the preferences store and add an agent/model/effort picker to the General settings panel, rendered through the same AcpConfigPickerShell/Section chrome as the chat pickers (one trigger button with up to three columns) but with its own discovery and persistence, so the shared session pickers stay untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…e goes stale The diagram-generation override stored in `diagram-subsession-config` can drift out from under the preference: an agent update can drop the pinned model/effort value id, or the pinned agent can be uninstalled or lack the HTTP MCP support the render tool needs. Previously any of those failed the `generate_pikchr` call hard — and since the stale preference was re-read on every call and never recovered from, every subsequent diagram generation failed too, while the settings UI silently reconciled the stale id to a valid-looking default. Backend — degrade to the no-override behaviour instead of erroring: - pikchr_subsession: generate_pikchr_source now takes an optional DiagramFallback (the invoking session's agent at its default model/effort). When the override run fails with an error attributable to the override — a stale/missing config selection per acp-client's is_config_selection_unavailable_error, or the required-MCP-transport check — the run retries once on the fallback driver with no config options, and the child session row is moved to the fallback provider so the transcript names the agent that actually drew the diagram. Unrelated failures and cancellations still surface as before. - pikchr_mcp: override runs carry the invoking session's agent as that fallback; an override provider that no longer resolves at all falls back before the run even starts. The stored preference is deliberately left intact so a later agent update can revive it. Settings — show the stale state instead of masking it: - DiagramConfigSetting: detect a stored provider missing from discovery and stored model/effort ids the live options no longer include. The trigger keeps the stale stored label and gains a danger border, and the field description switches to an error explaining that diagrams fall back to the requesting session's own agent until the selection is updated. Tests cover the stale-selection fallback (fallback runs at defaults in a fresh agent session, session row reassigned), that unrelated failures do not reroute, and the override-unavailable error matcher. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…iagram fallback matcher The diagram-override fallback matcher in pikchr_subsession coupled its two halves asymmetrically: stale config selections were detected via acp-client's exported is_config_selection_unavailable_error (which shares CONFIG_SELECTION_* constants with its producer), but the missing-MCP-transport half hard-coded the literal "does not support required MCP transports" produced far away in acp-client's driver with no shared constant — so a rewording there would have silently downgraded the graceful fallback to a hard error. Hoist the fragment into a MISSING_MCP_TRANSPORT_PREFIX constant shared by the producer in setup_acp_session and a new exported is_missing_mcp_transport_error matcher, mirroring the existing config-selection producer/matcher contract, and route pikchr_subsession's transport check through it. The subsession test keeps asserting against the full literal message, so it now also guards that the constant still matches the real error shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ts tests Code review on e608a85 found the staged lib did not build: the fallback retry in generate_pikchr_source referenced grammar_reference, a name that does not exist (the parameter is grammar), and the three tests added for the stale-override fallback passed a bare "/tmp/grammar.md" where the grammar parameter is Option<&str> — so neither the library nor the test binary compiled, and the coverage the earlier commits described had never actually run. Pass grammar through to the fallback retry (the same inlined grammar body the original attempt used) and wrap the test literals in Some(...), as the existing run_generation_with_reason helper does. Verified with cargo check --lib and the full lib test suite: 522 passed, including the stale-fallback, unrelated-failure, and config-forwarding tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.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: 6b8e873803
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| configLoading = true; | ||
| configError = null; | ||
|
|
||
| discoverAcpConfig(providerId, null, modelValue ? { selectedModelValue: modelValue } : {}) |
There was a problem hiding this comment.
When a saved diagram model value has been removed by an agent update and there is no cached model-specific discovery, passing that stale selectedModelValue here makes discovery fail before returning the provider's model selector; the catch then clears config, so modelStale never becomes true and the UI falls through to “Diagrams will use at its default model and effort.” The backend still sends the stale config option and falls back to the requesting session's agent, so users see the wrong effective behavior and have no stale-selection warning to fix it.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a Diagram generation setting (General settings) that lets the user pick which agent — and that agent's model and reasoning effort — the
generate_pikchrdiagram sub-session runs under, independent of the session that invoked the tool. When unset, behaviour is unchanged: the sub-session inherits the invoking session's agent at its default model/effort.Frontend (
apps/staged)DiagramConfigSetting.sveltein General settings for choosing the diagram agent/model/effort.preferences.svelte.tspersists the override under a newdiagram-subsession-configkey (mirroringbranch-prefix, read directly frompreferences.jsonby the backend). Empty configs normalize tonullso the backend stays on its inherit path.Backend (
apps/staged/src-tauri)acp_config.rs: newDiagramSubsessionConfig— deserializes the stored preference; any read/parse failure yields the all-unset default. Model/effort selections are only applied when a provider is configured, since their value ids are provider-specific.pikchr_mcp.rs: resolves the override per tool call and threads the model/effort config options into the sub-session run.pikchr_subsession.rs: forwards the config options to the driver on every turn (including resumed ones).Stale-override fallback
The stored preference can drift stale between runs (agent uninstalled, model/effort id dropped by an agent update, agent without the HTTP MCP transport the render tool needs). Instead of failing every call until the setting is fixed, an override run carries the invoking session's agent as a fallback:
crates/acp-clientis_missing_mcp_transport_errormatcher, kept in lockstep with the producer error string via a shared constant, so the fallback path can distinguish "agent can't host the render tool's MCP server" from run-level failures.Testing
DiagramSubsessionConfigdeserialization, blank/missing provider handling, and no-provider ⇒ no config options.🤖 Generated with Claude Code