feat(pikchr): persist diagram sub-sessions with in-session render iteration#888
Merged
Conversation
Persist generate_pikchr specialist runs as child sessions using the real store and DB-backed message writer, including prompt/assistant transcript rows, ACP agent ids, and terminal status updates. Return the child session id in MCP structured content while preserving the existing text output for preview paths and Pikchr source. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Match generate_pikchr ACP tool calls when providers qualify the name with dot or underscore delimiters, including single- and double-underscore variants. Signed-off-by: Matt Toohey <contact@matttoohey.com>
When the sub-session generates a diagram successfully but the terminal status update fails, log the bookkeeping failure and return the outcome instead of discarding it. The child session stays Running until the next launch's dead-session recovery, which is preferable to losing a good diagram over a status row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The repairs_out_of_bounds_render_before_returning test was not updated when generate_pikchr_source gained store and session id parameters, so cargo test --lib failed to compile with E0061. Create a child session like the sibling tests and pass Arc::clone(&store), &session_id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Collapse the two-branch fork in richToolCard into a single tool-card that swaps just the header row for the open-diagram-session button, instead of duplicating the card wrapper and re-indenting the entire details block on the else side. Factor the status-dot span into the toolStatusDot snippet (absorbing toolStatusIcon) so its tone-class logic lives in one place, and reuse it from the regular header, the pikchr button, and the grouped-tools header. The expandable details stay hidden while the button is shown, preserving the existing behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Replace the host-driven render/repair loop with a single render_pikchr MCP tool served per-call to the specialist sub-session. Each successful render returns the rendered image plus a layout analysis to the specialist and overwrites a shared last-render slot; the specialist accepts by ending its turn with the AcceptLastRender sentinel, and the host returns the slot contents — so only source that passed the render gate can ever reach the caller. The host loop now just checks for the sentinel and re-prompts on protocol misses (no sentinel, or acceptance before anything rendered), with MAX_ATTEMPTS down from 5 to 3 since it no longer bounds design iteration. Fence extraction and the host-side repair prompts are deleted with the behavior. Layout warnings no longer gate the loop: a flagged render still lands in the slot with its report shown to the specialist, and accepting it is a deliberate act. The accepted render's warnings propagate to the calling agent as a warnings text line and a renderWarnings structured field on the generate_pikchr result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The sentinel was detected with a case-insensitive contains-check, but
the token appears verbatim in the prompts and every render_pikchr
result, so a model echoing the instructions mid-prose ("I'll end with
AcceptLastRender once the overlap is fixed") would accept whatever
intermediate render was in the slot.
Tell the specialist its whole message must end with AcceptLastRender as
its own line, and parse accordingly: the reply's final line must equal
the sentinel, forgiving case, surrounding backticks, and trailing
sentence punctuation but not extra words. Update the initial prompt,
both re-prompts, the render_pikchr tool description, and the standing
accept instruction to teach the final-line form.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
The child session's terminal status was written with unconditional update_session_status calls, bypassing transition_from_running — the store's documented safe path for background threads. The race is reachable: this session is driven by the pikchr worker thread and never registered in the SessionRegistry, so cancel_session's fallback writes Cancelled directly to the store, and a late Completed (or Error) write from the worker would silently overwrite it. Route the post-generation status match in generate_pikchr_source and mark_pikchr_child_session_error through transition_from_running so a status the session already reached wins. A skipped completion is logged and the generated diagram still goes back to the caller. This also means a preview temp-file failure after a successful generation no longer flips the child session from Completed to Error. Add a regression test that lands the fallback-path cancel mid-turn and asserts Cancelled survives the worker's completion write. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The temp-file write for the preview PNG happens after generate_pikchr_source has already settled the child session's terminal status (Completed, a surviving concurrent Cancel, or deliberately left Running when the completion write failed). Marking the child session Error/Crashed from this parent-side failure produced a session row claiming a crash over a transcript ending in a clean acceptance — and in the still-Running case the bogus Error write would actually land. Drop the mark_pikchr_child_session_error call from that path: the child session's status describes the specialist run, and the error on the parent tool result already explains the preview failure to the caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
generate_pikchr_source and its _inner helper grew to eight parameters across the store/session-id and render-slot changes, tripping clippy's too_many_arguments at -D warnings. Apply the crate's established per-function allow, as in session_commands and lib. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The "Open diagram session" button on a generate_pikchr tool card only appeared after the tool finished, because the child session id travels in the tool result's structured content. While the specialist ran, the chat showed a bare "Running mcp__pikchr__generate_pikchr" row with nothing to click. Announce the child session the moment generate_pikchr creates it: the pikchr MCP handler now carries its parent session id and writes a hidden pikchr_session_started metadata row (naming the innerSessionId) into the parent transcript, which the chat pane's poll already picks up live. The transcript builder pairs unclaimed announcements FIFO with pikchr tools lacking an output-derived id — preferring tools recorded before the announcement row — and applies them only while the tool is still pending. The button therefore appears mid-run with the running status dot; the tool result stays authoritative on completion, and a failed call falls back to the plain header so its error details remain visible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
In a diagram child session, the specialist's render_pikchr iterations showed as bare collapsible tool cards, so reading the transcript meant expanding each call and squinting at raw Pikchr source. Render each successful call's source through the existing markdown ```pikchr pipeline instead, as if the specialist had posted it as a message — the shared diagram styles, source fallback while the WASM renderer loads, and fullscreen zoom viewer all apply for free. The transcript builder surfaces a pikchrRenderSource on completed render_pikchr tool items (matching delimiter-qualified names like mcp__pikchr-preview__render_pikchr, reading the source from the ACP raw input or the recorded call content); failed and still-running calls keep the plain tool card so error details stay visible. The chat pane swaps the card header for the rendered diagram, keeps diagram items out of collapsed verb groups, and counts tool sources toward the pikchr renderer's lazy load and its retry key. Fence construction lives in a shared fencedDiagramMarkdown helper that grows the fence past any backtick run in the source, so source content cannot terminate the block early. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
A failed generate_pikchr call (e.g. a timeout) rendered as a bare "Ran mcp__pikchr__generate_pikchr" header: the error result carries no structured content, and the transcript builder only used the start-of-run announcement while the tool was still pending, so the diagram session — the place with the real story — was unreachable from the chat. Keep the announced child-session link on failed calls: the tool card now shows an "Open failed diagram session" button, tinted with the danger status dot, instead of dropping back to the plain header. The full error then appears at the bottom of the inner session's chat: - Hard failures already land there (Error status + message drives the existing bottom-of-chat alert). - Timeout and abandonment cancellations previously ended Cancelled with no explanation. The worker's wall-clock timer now records its reason in a CancelReason slot before arming the cancellation token, and generate_pikchr_source resolves it — an unexplained token cancel reads as the caller abandoning the MCP request (parent stopped, crashed, or client-side timeout) — writing it as the cancelled child session's message and returning it as the tool error, so a timeout reads as a timeout instead of a generic cancel. transition_from_running now persists the message for Cancelled as well as Error (a direct user cancel passes no message and stays clean), and the chat pane's destructive alert also covers cancelled sessions that carry one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
assignAnnouncedPikchrSessions matched each pikchr_session_started row to the OLDEST unmatched generate_pikchr tool recorded before it. Any tool with neither an output-derived id nor its own announcement — a transcript recorded before announcements existed, or a call whose announcement write failed (the backend treats that as non-fatal) — therefore sat in the unmatched pool and stole the next call's announcement: the stale card linked to the wrong diagram session while the new call showed no link mid-run (and never gained one if it then failed, since a failed result carries no id). Pair each announcement with the MOST RECENT unmatched tool before its row instead. The backend records the tool_call row before writing the announcement, so the nearest preceding tool is the announcement's own call, and stale legacy tools stay unclaimed. An announcement racing ahead of its tool_call row still falls back to the earliest unmatched tool, as before. Add a regression test covering the stale-tool scenario. 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: 3c5aaac081
ℹ️ 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".
| } | ||
|
|
||
| fn create_pikchr_child_session(store: &Store, provider_id: &str) -> Result<Session, String> { | ||
| let mut session = Session::new_running(PIKCHR_CHILD_SESSION_PROMPT, &std::env::temp_dir()); |
There was a problem hiding this comment.
When a user opens the announced diagram session while generate_pikchr is still running and presses Stop, this child row is only created in the store and is never registered with SessionRegistry; cancel_session therefore takes its fallback path and just marks the row Cancelled. The worker's cancellation token is private to this tool call, so it keeps appending transcript/tool output and can still return a diagram to the parent; transition_from_running later preserves the user's Cancelled status instead of stopping the work. Please wire the child session to the worker token (or poll/store-cancel before continuing) so the exposed Stop control actually terminates the sub-session.
Useful? React with 👍 / 👎.
The announced diagram session could be opened mid-run, but its Stop control was a dud: the child row was only ever created in the store, never registered in the SessionRegistry, so cancel_session took its fallback path and just wrote Cancelled to the row. The worker's cancellation token was private to the generate_pikchr call, so the specialist kept running, kept appending transcript rows, and could still hand a diagram back to the parent — transition_from_running merely preserved the user's Cancelled status over the completion write without ever stopping the work. Register the child session in the SessionRegistry for the duration of the call, via a new register_external API that hands back a guard (deregistered on drop) exposing a session-scoped token. The worker watches that token next to its existing wall-clock timer and forwards a fire onto its own token, recording a user-stop CancelReason first — so a Stop now terminates the provider subprocess, marks the session Cancelled with a message saying the stop was deliberate, and fails the parent tool call with the same story instead of the caller-abandonment text. Cancels landing in the sliver before registration still take the store-write fallback that transition_from_running already respects. Covered by a registry test for register_external's cancel-until-drop contract, a forwarder unit test, and an end-to-end test driving a registry Stop mid-turn through generate_pikchr_source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
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
Turns
generate_pikchrspecialist runs into first-class, persisted child sessions and reworks how diagrams are produced and surfaced in chat.Persisted, openable diagram sessions
mcp__pikchr__generate_pikchr, dot/underscore variants) are recognized.In-session render iteration
render_pikchrMCP tool served to the specialist: each successful render returns the image plus layout analysis and lands in a last-render slot, so only source that passed the render gate can reach the caller.AcceptLastRenderfinal line — parsed strictly (final line only) so echoing the sentinel mid-prose can't accept an intermediate render. Layout warnings no longer gate the loop; accepted-render warnings propagate to the calling agent.Chat rendering
render_pikchrcalls render inline as diagrams through the existing markdown```pikchrpipeline (source fallback, fullscreen zoom included) instead of bare collapsible tool cards; failed/running calls keep the plain card so errors stay visible.Robustness fixes
transition_from_running, so a late worker write can't clobber a concurrent cancel (regression test included).🤖 Generated with Claude Code