Skip to content

Plannotator Inbox: guided reviews from agents (step 5) - #1760

Merged
backnotprop merged 1 commit into
mainfrom
feat/inbox-guides
Oct 7, 2026
Merged

backnotprop merged 1 commit into
mainfrom
feat/inbox-guides

Conversation

@backnotprop

@backnotprop backnotprop commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Plannotator Inbox, step 5: guided reviews from agents

An agent sends the person a guided review of a code change through two new MCP tools, get_guide_brief and submit_guide. The Inbox checks it with Plannotator's own guide checks, keeps the snapshot as a blob on the message, marks the thread's row "Guided review" and shows the guide at the foot of its message. Open renders it in Plannotator's guide viewer over the whole window, read-only, with the Inbox as its GuideHost.

Base: main with the window (PR 1755), notifications (1756), decisions (1757) and attachments (1759) and the Claude Code connection (1758, 4bbc34c) merged. The branch is rebased onto 4bbc34c with both sides kept in every conflict, and is one squashed commit. git merge-tree --write-tree origin/main HEAD shows no conflict.

Review fixes (REVIEW-PLANNOTATOR-INBOX-guides-2026-10-07)

  • B1, an idempotent retry. buildAuthoredGuideSnapshot stamps the build time, so a retried guide plus patch built a different snapshot. Idempotency now compares guide.input_sha256, the hash of what the agent sent as given, so a retry answers the first message and its guide. The blob is written only after the message lands, so a refused send leaves no orphan (item I4) and a retry writes nothing. Proved through the SDK client in the guides e2e: the second call returns replayed: true with the first message_id and the first guide's sha256.
  • B2, ticks kept with the thread. The ticks live on the message (guide_reviewed) and are saved through POST /api/inbox/messages/:id/guide/reviewed. The route is same-origin and serverSession guarded like the other writing routes; 422 means not a list of booleans, 404 means no message or no guide. Proved end to end: tick, then read the stored record. Then the Inbox is stopped, its last port is held by another listener, and it restarts on another port, which is a new origin. The guide opens there with both ticks. The localStorage copy is gone.
  • I1, the lazy wording. It is corrected in GuidePane, GuideReader, the entry-assets test and below. In the single-file build the reader's bytes ship inline in inbox.html and its modules (the guide chain, review-editor's renderer) run at page load. Only the mount, the diff render and the highlighter wait for Open. A truly lazy load is not cheap here, because apps/inbox builds with inlineDynamicImports: true like every Plannotator single-file app. The +390 kB stands as the cost.
  • I5. The row mark is hidden on a Sent row: that row replaces the subject with the delivery line, and the mark goes with the subject.
  • One blob writer (I2). Attachments (PR 1759) landed first, so guides now write through its writeBlob (exported from packages/shared/inbox/attachments.ts) and read through inboxBlobPath. packages/shared/inbox/blobs.ts is deleted. The blob is still written only after the message lands. A guide's blob joins the thread's attachment blobs in threadBlobs and in the delete paths' used-set, so delete thread or delete project never removes a blob a guide still uses, and Settings counts it once. "In use" means: some message still in the store names the hash, as an attachment's sent_sha256 or a guide's sha256. The delete census collects those from every remaining message and removes only blobs outside that set. Proved in the guides e2e: two threads with the same guide blob; deleting one keeps the blob, deleting the second removes it, and the other guides' blobs stay. Removing the guide from the used-set makes that proof fail.
  • send_message's attachments ride the shared sendAgentMessage path; submit_guide takes none.

What changed

  • MCP (packages/server/inbox-guides.ts, new; registered from packages/server/inbox-mcp.ts):
    • get_guide_brief returns the same shape as Workspaces' tool of that name: methodology (GUIDE_REVIEW_PROMPT), output_schema (GUIDE_SCHEMA_JSON), diff_steps, rules, and example. The example is a submit_guide call that works as it is: the AUTHORED fixture of guide-cli.test.ts and its patch.
    • submit_guide takes either guide plus patch, or snapshot (an object or its JSON). It also takes body, which defaults to the guide's intent, and send_message's routing fields.
    • Checks:
      • parseGuideSnapshot / parseGuideSnapshotJson for a snapshot.
      • buildAuthoredGuideSnapshot's strict placement checks for both forms. They are called, never re-implemented, with the project root as the cwd.
      • A refusal is isError invalid_guide: … and names the file.
    • Tool descriptions: get_guide_brief 318 characters, submit_guide 717. The server instructions gain one line and are 1,444 characters.
    • send_message's routing fields and send path moved into SEND_ROUTING_FIELDS, sendProject and sendAgentMessage. Both tools use them, and send_message behaves as before.
  • Store:
    • The snapshot is written to inbox/blobs/<sha256> through the attachments' writeBlob.
    • Messages gain an additive guide: { sha256, input_sha256, bytes, title, sections, files, additions, deletions } and guide_reviewed. Rows gain guide: boolean (packages/core/inbox-types.ts, packages/shared/inbox/store.ts).
    • Idempotency also compares the guide's input hash (B1).
    • Settings counts a thread's guide blobs in its size.
  • Server route: GET /api/inbox/messages/:id/guide returns the stored snapshot. It serves by message only, never a blob by hash. The handler is in inbox-guides.ts and is one block in inbox.ts.
  • Window (packages/inbox):
    • Wide rows show the "Guided review" mark (InboxList.tsx).
    • GuideCard.tsx is the card at the foot of the message.
    • GuidePane.tsx is the guide over the window: header "← Thread · Guided review · from in , sent " and a close button. Route: #thread=…&guide=<message id>. Escape and "Thread" go back to the thread.
    • guide/GuideReader.tsx holds GuideView behind GuideHostProvider, over review-editor's AllFilesCodeView in readOnly mode. That is the renderer guides.show uses.
    • The reader mounts only through lazy(() => import('#guide-reader')) (package.json imports). Its bytes are inline and its modules run at page load (I1).
    • Reviewed ticks are saved on the message through the new POST route (B2).
    • CSS: inbox.css has its own block. apps/inbox/index.css scans packages/guide-viewer and packages/review-editor/components.
    • apps/inbox/vite.config.ts gains apps/review's worker block: Pierre's inline worker needs the ES format.
  • Docs: the Inbox section of AGENTS.md (= CLAUDE.md) covers guided reviews, the route table and the proofs.

Record sections this makes real

.product/approved/plannotator-inbox-window-2026-10-07/:

  • 4.1: the guide sits in the thread at the foot of its message, with "1 attachment", the book mark, the name, "N sections, N files, +A -D" and Open.
  • 4.2: the guide takes the window under a header naming who sent it and when. Plannotator's guide viewer shows sections on the left with their files and the diff on the right. Reviewed ticks are kept with the thread.
  • 1.1: a thread that carries a guide shows the row mark "Guided review" after its subject.

Proofs

  • tests/e2e/inbox-guides.spec.ts runs with bun run test:e2e:inbox in the existing inbox-e2e.yml job, after the binary is built. It is Playwright against the compiled binary under a temp data dir and HOME. Agents go through plannotator inbox mcp with the SDK's stdio client (scripts/inbox-sim.ts gains guideBrief and submitGuide). It proves:
    • get_guide_brief returns the brief.
    • The brief's own example (the AUTHORED guide plus patch, from Pi) and the shipped fixture FIXTURE_V1_LOCAL as a snapshot (from Claude Code) land as two rows with the mark.
    • The thread's card shows the counts.
    • Open renders the title, intent, sections, provenance line, files and the diff text.
    • A retry with the same idempotency_key answers the first message.
    • A reviewed tick is kept on the message and survives a restart on another port.
    • Thread and Escape go back.
    • The authored guide shows its left-out file under "Everything else" and "generated by Claude Code · claude-opus-5".
    • Refused by name, with no row landing:
      • a file not in the patch (src/nope.ts);
      • a file placed twice (src/auth.ts is placed twice);
      • a snapshot that names a file outside its own patch;
      • guide without patch.
    • No page or CSP errors.
  • PNGs, light and dark at 1440 by 900, are in .local/proof/guides/ with a contact sheet: 1.1-guide-rows, 4.1-guide-in-thread, 4.2-guide-open, 4.2-guide-reviewed-tick, 4.2-authored-guide-open. CI uploads them with the window's artifact.
  • tests/entry-assets.test.ts gains one test. No Inbox module except the reader may statically import @plannotator/guide-viewer, @plannotator/review-editor or #guide-reader, and GuidePane must load the reader lazily. I checked that it fails when App.tsx imports the guide viewer statically.
  • packages/server/inbox.test.ts now also checks that both guide tool descriptions stay under 2,048 characters. The tool list check covers the two new tools through INBOX_MCP_TOOLS.

Gates run

  • bun run typecheck: green. It includes packages/inbox/tsconfig.json and apps/inbox/tsconfig.json.
  • bun test packages apps/hook tests scripts under a temp PLANNOTATOR_DATA_DIR and HOME: packages 4,956 pass / 0 fail; tests 61 / 0; scripts 198 / 0; apps/hook 566 pass / 1 fail. The one failure is annotate-cli.test.ts "exits 1 with one line naming the port", and it fails the same way on a clean origin/main worktree on this machine, so it is not from this change. The shim test inbox-cli.test.ts now reads the tool list from INBOX_MCP_TOOLS instead of a literal four-tool list.
  • bun test tests, which includes tests/entry-assets.test.ts and tests/favicon-surfaces.test.ts: green.
  • bun run test:e2e:inbox after the rebase onto 4bbc34c: 51 of 51 green (window, notifications, decisions, attachments, guides).
  • bun run --cwd apps/guides-show build:viewer && bun run --cwd apps/guides-show check:manifest: "guide-viewer-manifest.ts is in sync".
  • bun run --cwd packages/ui smoke:package: green (ui 0.52.1 with core 0.25.12). Core changed, but only inbox-types.ts, which ui does not import.
  • bun run check:release-version: consistent.
  • No DOM test files were added, so the DOM_TESTS=1 allowlist is unchanged.

What it leaves out

  • Annotations on guides (not in this step).
  • Document walkthroughs: diffs only, per owner Q6.
  • Any size bound beyond what the snapshot parser enforces (owner, 2026-10-06). plannotator-ops's 25 MiB suggestion is not applied.
  • The mark on narrow rows: 4.1 draws none there.

Deviations from the record and the plan, with reasons

  1. The viewer header meta is GuideView's own. 4.2's sheet draws "4 sections · from Pi". GuideView draws "2 sections · 1/2 reviewed · generated by <engine · model>" plus the provenance line guides.show passes (repo · branch · range). The record says to use Plannotator's components as they are, so the viewer is drawn as it is, and who sent the guide is named in the header above it.
  2. The card's name is "Guided review: <the guide's title>". The sheet's sample name is a shorter phrase than its guide's title, and the snapshot has no second name to use.
  3. Withdrawn: the ticks are now kept on the message in the store (B2).
  4. The header's close button closes the guide and the thread; "← Thread" goes back to the thread. The sheet draws both controls without saying what each does.
  5. Default body and subject. When the agent sends no body, the message body is the guide's intent and a new thread's subject is "Guided review: <title>". sendMessage needs a body, and the card needs a message to sit under.
  6. Snapshots go through the strict checks too. plannotator-ops's item 6 asks for this. The authored form also requires a non-empty intent and overviews, so a snapshot without them is refused. A file reference with no summary, which the snapshot format allows and the shipped FIXTURE_V1_WORKSPACE uses, is checked as "" instead of refused.
  7. The Inbox now depends on @plannotator/review-editor, for AllFilesCodeView only. plannotator-ops's binary-size note said to avoid importing review-editor. The guide viewer has no diff renderer of its own, and guides.show uses exactly this one in readOnly mode. Without it the guide is not Plannotator's guide viewer. Cost: inbox.html grows from 20,145 kB to 20,535 kB (+390 kB; Pierre was already in it). Only the mount is behind the dynamic import; the bytes and the module evaluation are not (I1).
  8. The reader's typecheck is not a CI gate. review-editor carries pre-existing type errors under every config in the repo, as apps/guides-show/tsconfig.viewer.json says. The window's strict typecheck reads the reader's shape from guide/reader-module.d.ts through a paths entry. The reader itself is checked by packages/inbox/tsconfig.guide.json for local use, and its own files report no errors.
  9. engineLabel is now exported from packages/guide-viewer/GuideViewer.tsx, so the reader names the generator exactly as the viewer does (for example claude-code becomes "Claude Code") instead of copying its table. This is JS only: no class was added, and the guides.show manifest check is unchanged.
  10. Two values from the review stylesheet, scoped to the guide pane: the code-review text-xs of 13px and Pierre's theme variables on diffs-container. guides.show and the in-app guide get both from packages/review-editor/index.css. Importing that file whole would restyle the Inbox.
  11. .ib-shell gets isolation: isolate so the message body's own layers never draw over the open guide. Without it, a frame showed the thread's text through the guide.
  12. The blob layout is inbox/blobs/<sha256>, as the step brief names it and as attachments (1759) writes it, through the same writer. RESEARCH.md section 8 had sketched blobs/sha256/<2 hex>/<rest>.
  13. A fix to the window's own e2e (tests/e2e/inbox.spec.ts, "agents write three threads"). It failed once in my run. When the first send's refresh lands before the other two, the list correctly holds them behind "N new", and the test assumed it never would. The test now clicks the notice when it shows. A second race in the same spec hit after the rebase: the Settings test read config.json right after the switch moved, but the switch moves before its POST answers. The test now polls the file. The window spec also clears only its own PNGs, so the guides' frames in proof/guides/ survive.
  14. CI runner time: inbox-e2e.yml gains paths (packages/guide-viewer/**, packages/review-editor/**, packages/server/guide/**, packages/core/guide*), and the guides spec adds about 10 s to the existing job. There is no new job.

Depends on: nothing beyond main.

@backnotprop
backnotprop force-pushed the feat/inbox-guides branch 4 times, most recently from 2487929 to 4ffeaa3 Compare October 7, 2026 16:11
get_guide_brief and submit_guide on the Inbox MCP; guides validated by
parseGuideSnapshot and buildAuthoredGuideSnapshot's strict placement checks,
stored as inbox/blobs/<sha256> after the message lands, carried on the
message (idempotency on the input as sent); the row's Guided review mark,
the card in the thread, and Plannotator's guide viewer over the window
(GuideView with the Inbox as GuideHost, AllFilesCodeView read-only, mounted
through a dynamic import); reviewed ticks kept on the message. Proofs:
tests/e2e/inbox-guides.spec.ts. Also two timing fixes in the window's e2e.
@backnotprop
backnotprop merged commit dfd7475 into main Oct 7, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant