Skip to content

Unify review core across Bun and Pi#310

Merged
backnotprop merged 4 commits intomainfrom
fix/pi-cr-untracked
Mar 16, 2026
Merged

Unify review core across Bun and Pi#310
backnotprop merged 4 commits intomainfrom
fix/pi-cr-untracked

Conversation

@backnotprop
Copy link
Copy Markdown
Owner

Summary

  • extract a runtime-agnostic review core for diff assembly, file-content lookup, worktree handling, and stage/unstage operations
  • switch the Bun review path to the shared core and copy the same source into the Pi package at build time
  • bring the Pi review server up to the shared review API surface, including diff switching, file content, drafts, uploads/images, agents, and editor annotations
  • add regression coverage for the shared review core and the Pi review server endpoints

Fixes #307.

Testing

  • bun test packages/shared/review-core.test.ts apps/pi-extension/server.test.ts
  • bun test

Notes

  • The full test run still has one unrelated pre-existing failure in packages/ui/components/diagramLanguages.test.ts because @viz-js/viz is not installed in this environment.

@backnotprop
Copy link
Copy Markdown
Owner Author

Code review

Found 1 issue:

  1. Pi plannotator-review handler silently drops inline annotations. The review server's waitForDecision() returns { approved, feedback, annotations, agentSwitch } (line 693-699 of server.ts), but the command handler in index.ts only reads result.feedback and ignores result.annotations. Since this PR adds the full annotation API surface to the Pi review server, users can now create inline code annotations in the review UI that are accepted by the server but never forwarded to the agent — the annotations are silently lost.

const result = await runBrowserReview(server, ctx);
if (result.feedback) {
if (result.approved) {
pi.sendUserMessage(`# Code Review\n\nCode review completed — no changes requested.`);
} else {
pi.sendUserMessage(`# Code Review Feedback\n\n${result.feedback}\n\nPlease address this feedback.`);
}
} else {
ctx.ui.notify("Code review closed (no feedback).", "info");
}
},

The Bun hook server avoids this because it bakes annotations into the feedback string via exportAnnotations() before submission. The Pi handler needs equivalent logic — either call exportAnnotations() to merge annotations into the feedback text, or handle result.annotations separately.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

backnotprop and others added 3 commits March 16, 2026 07:52
- Handle unborn HEAD in runGitDiff("uncommitted") via rev-parse check
  instead of assertGitSuccess, so fresh repos fall through to untracked diffs
- Bind Pi server to 0.0.0.0 for remote sessions (regression from e47eda0)
- Update CLAUDE.md: packages/shared/ description, PLANNOTATOR_SHARE env var,
  /api/diff response shape

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@backnotprop backnotprop force-pushed the fix/pi-cr-untracked branch from 7b4f492 to be872b4 Compare March 16, 2026 14:53
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@backnotprop backnotprop merged commit ea689d9 into main Mar 16, 2026
backnotprop added a commit that referenced this pull request Apr 21, 2026
The memoized intro-card array was placed after the loading/error early
returns in TourDialogContent, so the first successful render called one
more hook than the initial loading render — React threw "Rendered more
hooks than during the previous render" (error #310) the moment a tour
finished loading. Move the useMemo above the early returns and use
optional chaining on `tour` so it's safe to run during loading.

Also move useTourData.ts into a hooks/tour/ subfolder to start a
standard for feature-scoped hooks (was flat among siblings).

For provenance purposes, this commit was AI assisted.
backnotprop added a commit that referenced this pull request Apr 21, 2026
* feat(server): add Code Tour agent as third review provider

Adds a "tour" provider alongside "claude" and "codex" that generates a
guided walkthrough of a changeset from a product-minded colleague's
perspective. Reuses the entire agent-jobs infrastructure (process
lifecycle, SSE broadcasting, live logs, kill support, capability
detection) so the only new server plumbing is the prompt/schema module,
the GET endpoint for the result, and checklist persistence.

- tour-review.ts: prompt, JSON schema, Claude (stream-json) and Codex
  (file output) command builders, and parsers for both. Prompt frames
  the agent as a colleague giving a casual tour, orders stops by reading
  flow (not impact), chunks by logical change (not per-file), and writes
  QA questions a human can answer by reading code or using the product.
- review.ts: tour buildCommand with per-engine model defaults (fixes a
  bug where Codex was defaulting to "sonnet", a Claude model), an
  onJobComplete handler that parses and stores the tour, plus GET
  /api/tour/:jobId and PUT /api/tour/:jobId/checklist endpoints.
- agent-jobs.ts: tour capability detection and config threading from the
  POST body through to the provider's buildCommand.
- shared/agent-jobs.ts: engine/model fields on AgentJobInfo so the UI
  can render tour jobs with their chosen provider + model.

For provenance purposes, this commit was AI assisted.

* feat(review-editor): Code Tour dialog with animated walkthrough

Adds the tour overlay surface: a full-screen dialog with three animated
pages (Overview, Walkthrough, Checklist) that renders the CodeTourOutput
from the server-side agent. Opens automatically when a tour job completes
and can be dismissed with Escape or backdrop click.

- components/tour/TourDialog.tsx: three-page animated dialog. Intro page
  uses a composition cascade (greeting, Intent/Before/After cards with
  one-shot color-dot pulse, key takeaways table) with a pinned Start
  Tour button and a bottom fade so content scrolls cleanly under it.
  Page slides between Overview/Walkthrough/Checklist are ease-out
  springs with direction auto-derived from tab index.
- components/tour/TourStopCard.tsx: per-stop accordion using motion's
  AnimatePresence for height + opacity springs with staggered children
  for detail text and anchor blocks. Anchors lazy-mount DiffHunkPreview
  on first open to keep mount costs low. Callout labels (Important,
  Warning, Note) are deterministic typography, no emoji.
- components/tour/QAChecklist.tsx: always-open list of verification
  questions with per-item spring entrance and persistent checkbox state
  that PUTs to the server.
- hooks/useTourData.ts: fetch + checklist persistence with a dev-mode
  short-circuit when jobId === DEMO_TOUR_ID so UI iteration doesn't
  require a live agent run.
- demoTour.ts: realistic demo data (multi-stop, multiple takeaways,
  real diff hunks) for dev iteration.
- App.tsx: tour dialog state, auto-open on job completion, and a
  dev-only "Demo tour" floating button + Cmd/Ctrl+Shift+T shortcut
  guarded by import.meta.env.DEV.
- index.css: all tour animations (dialog enter/exit, page slides, stop
  reveal cascade, intro exit) with reduced-motion fallbacks. Dark-mode
  contrast tuning across structural surfaces (borders, surface tints,
  callout backgrounds) so the design works in both themes.
- package.json: adds motion@12.38.0 for spring-driven accordion physics
  and the intro composition cascade.

For provenance purposes, this commit was AI assisted.

* feat(review-editor): wire Code Tour into agent jobs UI + polish DiffHunkPreview

Connects the tour provider to the existing agent jobs sidebar and detail
panel so the user can launch a tour the same way they launch Claude or
Codex reviews. Also tightens DiffHunkPreview (used inside tour anchors)
to render correctly on first mount and handle every diff hunk shape the
agent might produce.

- ui/components/AgentsTab.tsx: tour engine (Claude/Codex) + model select
  when launching a tour job. Resets model to blank when falling back to
  Codex-only so Codex uses its own default instead of inheriting
  "sonnet" (a Claude model).
- ui/hooks/useAgentJobs.ts: forward engine/model config from the UI
  through to the POST /api/agents/jobs body so the server's tour
  provider can pick the right command shape.
- dock/panels/ReviewAgentJobDetailPanel.tsx: tour-aware detail panel.
  Replaces the "Findings" tab with a "Status" card for tour jobs and
  surfaces an "Open Tour" button that opens the dialog overlay when
  the tab is already in the dock.
- components/DiffHunkPreview.tsx: synchronous pierre theme init via a
  useState lazy initializer so the first render (inside a tooltip) is
  already themed; robust hunk parser that handles bare @@ hunks, file-
  level --- hunks, and full git diffs; "Diff not available" fallback
  for broken hunks.
- components/ReviewSidebar.tsx, dock/ReviewStateContext.tsx: small
  wiring changes to expose openTourPanel from the review state context.

Also removes two dead components (TourHeader, DiffAnchorChip) that were
superseded by the inline title bar in TourDialog and the inline
AnchorBlock in TourStopCard.

For provenance purposes, this commit was AI assisted.

* refactor(server): extract Code Tour lifecycle into shared createTourSession factory + Pi parity

The route-parity test suite requires the Pi server (apps/pi-extension/)
to expose the same routes as the Bun server (packages/server/). After
Code Tour shipped in the prior 3 commits, Pi was missing /api/tour/:jobId
(GET) and /api/tour/:jobId/checklist (PUT).

A naive mirror would duplicate ~100 lines of provider-branch logic
(buildCommand, onJobComplete, in-memory maps) into Pi's serverReview.ts,
perpetuating the existing claude/codex duplication problem. Instead,
extract the pure runtime-agnostic tour lifecycle into a
createTourSession() factory that both servers consume. Route handlers
stay per-server (different HTTP primitives) but are ~5 lines each.

Net effect: Pi port is ~25 lines instead of ~100. Future providers that
adopt the same pattern cost ~15 lines per server.

- tour-review.ts: new createTourSession() at the bottom of the module.
  Encapsulates tourResults + tourChecklists maps, buildCommand (with
  the Claude-vs-Codex model-default fix baked in), onJobComplete
  (parse/store/summarize), plus getTour/saveChecklist lookup helpers
  for route handlers.
- review.ts (Bun): tour branch in buildCommand, tour branch in
  onJobComplete, and both route handlers collapse to one-line calls
  into the factory. Drops ~70 lines.
- vendor.sh: add tour-review to the review-agent loop so Pi regenerates
  generated/tour-review.ts on every build:pi.
- serverReview.ts (Pi): import createTourSession from
  ../generated/tour-review.js; add tour branch to buildCommand (one
  line), tour branch to onJobComplete (three lines), and GET/PUT route
  handlers using Pi's json() helper. ~25 lines added.
- agent-jobs.ts (Pi): extend buildCommand interface to accept config
  and return engine/model; thread config from POST body; extend
  spawnJob to persist engine/model on AgentJobInfo; add tour to
  capability list.

Claude and Codex branches are intentionally left in the old pattern;
they can migrate to the factory approach when next touched to keep
this change's blast radius contained.

Tests: 518/518 passing (previous 3 route-parity failures resolved,
plus 2 extra assertions passing since tour is now in both servers'
route tables).

For provenance purposes, this commit was AI assisted.

* refactor(tour): self-review cleanup — Pi route match parity + remove setOpen shim

Two small cleanups surfaced by a self-review pass:

- Pi's GET /api/tour/:jobId route used `endsWith("/checklist")` to block
  the checklist sub-route, while Bun uses `includes("/", ...)`. The two
  are not equivalent: a URL like /api/tour/abc/extra would be accepted
  by Pi (jobId becomes "abc/extra") but correctly rejected by Bun.
  Align Pi to Bun's pattern.
- TourStopCard had a leftover `setOpen` shim from when open state was
  local to the card. State is now lifted to TourDialog, so the shim
  just aliases onToggle and ignores its argument. Replace with a
  direct `onClick={onToggle}` on the trigger button.

520/0 tests still pass.

For provenance purposes, this commit was AI assisted.

* feat(review): per-provider model + effort controls across all review agents

Exposes model, effort/reasoning, and fast-mode (Codex) as per-job knobs
for Claude, Codex, and Code Tour via the Agents tab. Threads the config
from the UI through the POST body into each provider's buildCommand,
which emits the corresponding CLI flags (--model/--effort for Claude;
-m / -c model_reasoning_effort / -c service_tier for Codex). Bun and Pi
servers stay in parity.

UI: option catalogs (CLAUDE_MODELS, CLAUDE_EFFORT, CODEX_MODELS,
CODEX_REASONING, TOUR_CLAUDE_MODELS) are defined once and mapped into
every dropdown so there's a single source of truth for the choices.
Tour's Claude/Codex settings are kept separate from the standalone
provider's state so toggling the tour engine no longer overwrites the
provider's last choice.

Job badge now surfaces the model/reasoning/fast selection for both Tour
and the standalone Codex provider.

For provenance purposes, this commit was AI assisted.

* docs: add Prompts reference page

Documents the three-layer structure every review call travels through
(CLI system prompt, our user message of review-prompt + user-prompt,
output schema flag), names each constant + its file, and calls out
that the Claude/Codex review prompts are the upstream ones from those
projects (only Code Tour's prompt is original to Plannotator).

Linked from the Code Review command page.

For provenance purposes, this commit was AI assisted.

* feat(review): persist per-agent, per-model settings in a single cookie

Drops the hidden "Default" options from the agent dropdowns, locks in explicit
sensible defaults (Opus 4.7/High for Claude review, gpt-5.3-codex/High for Codex,
Sonnet/Medium for Tour Claude, gpt-5.3-codex/Medium for Tour Codex), and
remembers the last-used effort/reasoning/fast-mode per (agent job × model) so
switching models reveals the choices you made last time.

Backed by a single `plannotator.agents` cookie holding the whole settings tree —
one read on mount, one mirror write per change, all mutations funnel through a
single React state owner to avoid stale-read or lost-write races across rapid
successive updates.

For provenance purposes, this commit was AI assisted.

* fix(tour): unblock reduced-motion nav + flush pending checklist save on close

Two P2 issues surfaced in review:

- Under prefers-reduced-motion, tour page animations are suppressed, so
  onAnimationEnd never fires and exitingPage stuck on 'intro' kept the
  walkthrough/checklist gated out. navigate() now swaps pages directly when
  reduced motion is on, mirroring the pattern already used in the wrapper.

- Checklist toggles are debounced 500ms before the PUT, but unmount only
  cleared the timer — checking an item and closing within the window dropped
  the save. Cleanup now flushes the pending payload with keepalive: true.

For provenance purposes, this commit was AI assisted.

* feat(review): surface Claude model + effort in job badge, trim redundant labels

Claude effort was never persisted on AgentJobInfo, so the job badge had nothing
to render beyond "Claude". Plumbs `effort` through the shared type, both
server build-command pipelines (Bun + Pi), and spawnJob, then teaches the
ProviderBadge to display Claude and Tour Claude model + effort with the same
shape Codex already had. Labels resolve via the dropdown catalogs so the
badge shows "Opus 4.7" / "High" instead of raw ids.

Server labels for both Claude and Codex reviews collapse to plain "Code Review"
(matching tour's "Code Tour"), since the badge now carries the provider +
model + settings — the title only needs to name the action.

For provenance purposes, this commit was AI assisted.

* polish(review): prefix agent dropdown options with the action name

The Run launcher listed "Claude Code", "Codex CLI", and "Code Tour" side by
side — the CLI's detection name was doing double duty as the action label.
Prefixes the review entries with "Code Review · " so scanning three options
surfaces two reviews + one tour instead of three raw provider names.

For provenance purposes, this commit was AI assisted.

* fix(review): drop invalid codex reasoning option + fail tour on empty output

Two P2 issues from PR review:

- Tour jobs exited 0 but returned null output would still be marked "done",
  and the auto-open watcher would greet the user with a 404 "Tour not found"
  dialog. onJobComplete now flips the job to failed with an error message
  when nothing was stored, so the card reflects the real state.

- The codex reasoning dropdown offered "None", but codex-rs only accepts
  minimal/low/medium/high/xhigh. Picking None sent `-c model_reasoning_effort=none`
  and launched a broken job. Removed the option; added a one-shot cookie
  migration so users who already saved "none" don't keep shipping it.

For provenance purposes, this commit was AI assisted.

* fix(tour): format Claude-engine logs + allow reading linked issues

Two P2/P3 issues from PR review:

- The stdout formatter keyed only on provider === "claude", so Code Tour
  jobs running on the Claude engine streamed raw JSONL to the log panel
  while Claude review jobs got formatted text. Widened the check to also
  catch spawnOptions.engine === "claude" and mirrored the fix into Pi.

- The tour Claude allowlist permitted PR/MR commands but not issue reads,
  yet the prompt explicitly asks the agent to open `Fixes #123` / `Closes
  owner/repo#456` targets for deeper context. Claude was being denied
  those commands mid-tour. Added gh issue view + gh api issues + glab
  issue view to the allowlist.

For provenance purposes, this commit was AI assisted.

* cleanup(review): e2e punchlist — Pi parity + dedup + tests

- Pi now logs Claude parse failures with the same diagnostic as Bun
- Tour route match uses a regex instead of an includes-offset trick
- Drop `any` cast in Pi's Codex blocking-findings predicate
- Extract `patchClaude` twin of `patchCodex` in useAgentSettings
- Factor `resolveAgentCwd` helper in Bun review to match Pi
- AgentsTab launch payload becomes a per-provider dispatch table
- Wrap TourDialog in MotionConfig reducedMotion="user" so motion.*
  children honor prefers-reduced-motion alongside the CSS keyframes
- Tests for parseTourStreamOutput/parseTourFileOutput and the Codex
  perModel sanitizer

For provenance purposes, this commit was AI assisted.

* test(tour-review): use real TourStop shape in fixtures

The parsers don't validate stop fields so the original made-up shape
passed fine, but future readers would be misled. Use the real
CodeTourOutput / TourStop / TourDiffAnchor shapes from tour-review.ts.

For provenance purposes, this commit was AI assisted.

* cleanup(tour): hoist shared types + subfolder server module + dedup

- Hoist tour output types to packages/shared/tour.ts so server and UI
  share one source of truth (prevents silent drift when schema changes).
- Move tour-review into packages/server/tour/ subfolder alongside the
  existing packages/review-editor/components/tour/ convention.
- Extend tour.buildCommand to accept tour context directly (patch,
  diffType, options, prMetadata), so Bun and Pi no longer duplicate the
  buildTourUserMessage call site. Export TOUR_EMPTY_OUTPUT_ERROR from
  the tour module to prevent string drift between servers.
- Early-return the tour branch in buildCommand so the review userMessage
  is built once on the shared non-tour path instead of unconditionally
  (dead work on tour launches) or twice (codex+claude dup).
- Regex capture group for tour checklist jobId in both servers —
  replaces a second url.pathname.split("/")[3] fragile fallback.
- Drop existsSync guard in parseTourFileOutput (TOCTOU + duplicate
  syscall); the outer try/catch already handles ENOENT.
- Dedupe two matchMedia reads in TourDialog behind a prefersReducedMotion
  helper; memoize intro-page card array so unrelated state churn doesn't
  recompute it.
- Comment hygiene on tour files — drop WHAT/change-referencing comments
  added by this PR; keep non-obvious WHY.

For provenance purposes, this commit was AI assisted.

* fix(tour): hoist introCards useMemo above early returns + subfolder hook

The memoized intro-card array was placed after the loading/error early
returns in TourDialogContent, so the first successful render called one
more hook than the initial loading render — React threw "Rendered more
hooks than during the previous render" (error #310) the moment a tour
finished loading. Move the useMemo above the early returns and use
optional chaining on `tour` so it's safe to run during loading.

Also move useTourData.ts into a hooks/tour/ subfolder to start a
standard for feature-scoped hooks (was flat among siblings).

For provenance purposes, this commit was AI assisted.
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.

Bug: still missing untracked files in pi-extension

1 participant