feat(staged): add generate_pikchr MCP tool for note-writing sessions - #841
Merged
Conversation
![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 👍 / 👎.
matt2e
force-pushed
the
pikchar-grammar-corrections
branch
from
July 2, 2026 05:52
a2cf4a8 to
eda0536
Compare
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>
…sions `generate_pikchr` now fully supersedes `preview_pikchr` as the single Pikchr MCP tool exposed to local note-writing sessions. The manual preview tool is removed: the agent no longer renders hand-written source itself, it describes the diagram (optionally passing existing source via `previous_pikchr`) and gets back validated, overlap-checked Pikchr plus a preview. As before, the pikchr MCP server is loopback-only and attached solely to local sessions (`workspace_name.is_none()`) — remote sessions can't reach it — so the surviving tool keeps the same local-only gating the preview tool had. How: - Drop the `preview_pikchr` `#[tool]` method and its `PreviewPikchrParams` from `PikchrToolsHandler`; `generate_pikchr` is now the only exposed tool. The internal `run_preview`/`PreviewOutcome` render+overlap path stays `pub(crate)` — the `generate_pikchr` sub-session still uses it to validate and repair candidate diagrams. - Rename the plumbing that gates the server so it no longer advertises a "preview" tool: `SessionConfig::expose_pikchr_preview` → `expose_pikchr_tools`, `local_note_pikchr_preview_available` → `local_note_pikchr_tools_available`, and the guidance flag `preview_available` → `pikchr_tools_available`. Behavior is unchanged; the gate still resolves to note-session && local. - Point `pikchr_note_guidance` at `generate_pikchr` only, folding the old "check/hand-tweak with preview_pikchr" advice into "pass current source as `previous_pikchr` to have it validated or repaired." - Refresh module/doc comments and log lines that named the removed tool. Tests updated to assert `generate_pikchr` in the note guidance; all pikchr unit tests pass and the crate builds clean. Signed-off-by: Matt Toohey <contact@matttoohey.com>
The 180s wall-clock cap on a single generate_pikchr call was too tight: the sub-session spins up a provider subprocess and runs several diagram + repair turns, and slower providers could hit the cap before converging on a clean, overlap-free diagram — returning a best-effort result or timing out mid-repair. Raise GENERATE_PIKCHR_TIMEOUT from 180s to 600s so the sub-agent has room to finish. The cap still fires via the sub-session's cancel token, so a genuinely stuck call is torn down promptly. Signed-off-by: Matt Toohey <contact@matttoohey.com>
…lf-repairing
The generate_pikchr sub-session used to loop internally on box overlaps:
render, detect overlaps, re-prompt itself to separate the shapes, and only
accept a still-overlapping diagram as a best-effort fallback after a couple
of retries. But overlaps are frequently intentional (touching/nested
shapes), the geometry check is box-vs-box only, and the reliable signal for
layout problems is the rendered PNG the calling note agent already sees.
Self-repairing burned provider turns fighting overlaps the caller may have
wanted.
Shrink the sub-session's job to: draw, guarantee it renders, hand back the
image plus warnings. The calling note agent reviews the preview and summary
and decides for itself — keep the diagram, or call generate_pikchr again
with the returned source as previous_pikchr and an adjusted description.
Parse errors are still repaired internally (a diagram that doesn't render is
useless to hand back); overlaps become pure advisory warnings.
How:
- pikchr_subsession.rs: drop GenOutcome::gave_up, MAX_OVERLAP_RETRIES, the
best/overlap_retries locals, overlap_prompt, and summary_reports_overlaps.
The loop now returns the first renderable diagram as-is (overlaps and all);
MAX_ATTEMPTS bounds only parse-error/empty-reply retries, and the final Err
still fires if nothing ever renders.
- pikchr_mcp.rs: always append the render summary to the tool result (no more
gave_up branch), reframe build_summary's overlap hint to address the
deciding agent ('call generate_pikchr again with previous_pikchr … otherwise
keep'), and fix the tool description's stale 'repairs … box overlaps on its
own' claim to 'repairs syntax errors … reports overlapping shapes; re-call
with previous_pikchr if the layout needs work'.
- session_commands.rs: update pikchr_note_guidance's matching stale promise.
Tests: replace the two loop-behavior tests with
returns_immediately_on_overlapping_render (returns after 1 call, keeps the
overlapping source, summary warns) and
repairs_parse_error_then_returns_overlapping_render (parse error repaired on
turn 2, overlap kept, resumption asserted). errors_when_nothing_ever_renders
is unchanged. All 34 pikchr unit tests pass; fmt and clippy clean.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
… boxes generate_pikchr's overlap check was box-vs-box only: it regex-scanned the rendered SVG for closed <path>/<rect> shapes and kept just the anchor point of each <text>, so labels never had an extent. It couldn't see a label colliding with another label or straying onto a shape it doesn't belong to — the box-only limitation 4d15e1b cited when it stopped self-repairing overlaps. Read geometry off the usvg tree instead. usvg already shapes every <text> into positioned glyphs at parse time and exposes the measured rectangle via abs_bounding_box(), using the same fonts and engine that rasterize the preview. So the real, shaped label rectangle is one method call away on the tree we were already building only to rasterize and then throw away. How: - Parse the rendered SVG once in run_preview and reuse the tree for both overlap analysis and rasterization (split parse_svg_tree out of the old rasterize_svg_to_png; rasterize_tree_to_png now takes a &Tree). If usvg can't parse Pikchr's own output (rare), degrade to dimensions-only rather than failing the whole call. - Replace the regex/path-tokenizer geometry extraction (tokenize_path, path_bounds, extract_shapes, extract_labels, attr_f64, decode_xml_entities, label_shapes) with a recursive walk of the usvg tree. Box-like shapes are closed, stroked paths — arrow shafts are open and arrowheads are fill-only polygons, so both fall out — and text labels take their true rectangle from abs_bounding_box. - Model boxes and text labels uniformly as Elements and extend find_overlaps to every pair: box↔box (as before), label↔label (colliding labels), and label↔box (a label straying onto a foreign shape). A label resting inside its own or an enclosing box is skipped (box contains the label's centre), and the separate lines of one multi-line label — which share a "home" box — don't flag each other. - Keep it advisory: overlaps stay warnings the calling note agent reviews, with no self-repair loop. Label-involving overlaps use a more forgiving threshold (MIN_TEXT_OVERLAP_PX) because usvg lays text out with native system fonts rather than the frontend's browser metrics. - Refresh the tool description, module docs, and note guidance to say the summary reports overlapping shapes or labels. Tests: usvg canvas coords line up 1:1 with raw path coords (asserted); arrowheads/shafts aren't counted as boxes; a label inside its box isn't flagged; colliding free labels are; the known cascade still reports overlaps and the corrected diagram still reports none. All pikchr unit tests pass; fmt and clippy clean. Signed-off-by: Matt Toohey <contact@matttoohey.com>
`colliding_free_labels_are_flagged` failed in CI ("expected a text-vs-text
overlap, found []") while passing on a dev macOS box. Root cause: the test
rendered two labels, parsed the SVG with usvg, and asserted an overlap — but
the label rectangles it compared come from usvg shaping the text with a
*system* font. usvg silently drops any text it can't find a matching font
for, so on a lean CI host with a different font set the text nodes vanish
entirely: `analyze_overlaps` finds zero labels and reports no overlap. The
sibling `label_inside_its_box_is_not_flagged` shared the same fragility — it
only passed on CI vacuously, because with no labels present there was
nothing left to (correctly) not flag.
Confirmed by parsing the same render with an empty fontdb: the two labels
that shape to overlapping ~72×13 / ~86×13 rects with system fonts produce no
text elements at all, and `overlaps` comes back empty — exactly the CI
failure.
The detection logic under test — `home_boxes` + `find_overlaps`, the
text↔text and text↔box branches and their thresholds — is pure geometry and
doesn't need usvg in the loop. Feed both tests synthetic `Element`s with
explicit rectangles (via a small `element` builder) instead of round-tripping
through render/parse, so they exercise the exact branches deterministically
regardless of which fonts the host happens to have. Box-overlap coverage
already runs end-to-end and is font-independent (box paths, not shaped text),
so `analyze_overlaps` wiring stays covered.
All 409 staged lib tests pass; fmt and clippy clean (the two remaining clippy
warnings are pre-existing and in unrelated files).
Signed-off-by: Matt Toohey <contact@matttoohey.com>
matt2e
force-pushed
the
pikchar-grammar-corrections
branch
from
July 2, 2026 13:34
5f3a2d6 to
5ea8e6c
Compare
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.