feat(staged): add generate_pikchr MCP tool for note-writing sessions#841
Open
matt2e wants to merge 2 commits into
Open
feat(staged): add generate_pikchr MCP tool for note-writing sessions#841matt2e wants to merge 2 commits into
matt2e wants to merge 2 commits into
Conversation
Add a `generate_pikchr` MCP tool alongside `preview_pikchr` that turns a natural-language description into validated Pikchr. It runs a focused internal ACP sub-session that emits a diagram, renders it through the same engine as `preview_pikchr`, and re-prompts itself to fix parse errors and box overlaps before returning the final source plus a PNG preview. Revisions pass the existing diagram's source in via `previous_pikchr` so the sub-agent edits real Pikchr rather than redrawing from scratch — no id-addressable storage; the note's ```pikchr fence remains the store. How: - Expose `run_preview` + `PreviewOutcome` as pub(crate) so the loop reuses the render/overlap path directly (no MCP round-trip). - New `pikchr_subsession` module: in-memory Store/MessageWriter stubs (the sub-session transcript never touches the user's session), prompt templates, and a bounded `generate_pikchr_source` loop written against the `AgentDriver` trait. Parse errors always block; overlaps are advisory and accepted after a couple of retries. The store stub captures the agent session id so repair turns resume in-context. - `generate_pikchr` drives the loop on a dedicated LocalSet thread (the ACP driver needs `spawn_local`), bounded by a wall-clock timeout via the sub-session's cancel token. - Carry the parent session's provider id + AppHandle on the pikchr handler; `start_pikchr_mcp_server` now takes both, and `session_runner` threads the resolved provider through so the sub-agent matches the agent the user chose. - Update `pikchr_note_guidance` to point the note agent at `generate_pikchr` as the primary path, keeping `preview_pikchr` for checks and hand-tweaks. Unit-tested with a fake driver: bad -> overlapping -> clean converges, persistent overlaps return best-effort, and never-rendering errors out — all within the attempt cap, with resumption asserted. Signed-off-by: Matt Toohey <contact@matttoohey.com>
The generate_pikchr worker ran on a detached thread that minted its own CancellationToken, so the parent MCP request had no way to signal it. If the note agent dropped (or retried) the tool call, only the receiving end of the result channel was dropped — the worker kept driving the provider subprocess until it finished or hit the 180s wall-clock cap. With no cap on concurrent invocations, a note agent that fired several of these could leave multiple orphaned provider subprocesses alive for up to three minutes each. Move the token into the parent future and hand the worker a clone. The parent holds a DropGuard, so when the future is dropped (the client abandons the call) the token is cancelled and the sub-session's provider subprocess is torn down promptly. The worker still arms this same token on timeout, so both the client-cancel and wall-clock paths converge on one token. 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: 41246c6c52
ℹ️ 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".
Comment on lines
+100
to
+101
| if let Some(id) = store_capture.agent_session_id() { | ||
| agent_session_id = Some(id); |
There was a problem hiding this comment.
When a first candidate needs a parse/overlap correction, this saves the ACP session id and the next loop passes it back into driver.run, which forces AcpDriver down its load_session path before the repair prompt is sent. For any selected provider that supports the parent HTTP MCP tool but advertises load_session = false, generate_pikchr will fail on the first retry instead of doing the promised repair, even though a fresh one-shot session with the failure details would still work.
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
generate_pikchrMCP tool to the Pikchr MCP server used by note-writing sessions (project notes and local branch notes). It turns a natural-language description into validated Pikchr source so agents no longer have to hand-write Pikchr and iterate blindly.What it does
generate_pikchrtool — takes a freeformdescription(boxes, arrows, labels, layout, relationships), an optionalprevious_pikchr(the current diagram's source, for revisions so intent is edited rather than redrawn), and an optionalscale. Returns the validated Pikchr source plus a rendered PNG preview. If overlaps or intent can't be fully resolved, it returns the best diagram reached with a note.pikchr_subsessionmodule — a focused internal ACP sub-agent writes the diagram, renders it through the same engine aspreview_pikchr, and re-prompts itself to repair parse errors and box overlaps. The loop is bounded (parse errors always block; overlaps are advisory and accepted after a couple of retries). The sub-session is not persisted — in-memoryStore/MessageWriterstubs keep its transcript out of the user's session messages.session_runnernow tracks the provider id the driver actually resolved to and threads it into the MCP server, so the sub-agent runs under the same agent the user chose without re-running login-shell discovery. An empty id is tolerated:generate_pikchrfails per-call whilepreview_pikchrkeeps working.generate_pikchrfor creating/revising diagrams, withpreview_pikchrreserved for checking hand-written source.Cancellation
The sub-session's
CancellationTokenis owned by the parent MCP request future (held via aDropGuard). If the MCP client abandons the tool call, the future is dropped, the token is cancelled, and the provider subprocess is torn down promptly instead of running detached. A 180s wall-clock cap arms the same token, so client-cancel and timeout converge on one path — preventing orphaned provider subprocesses.