Skip to content

fix(pi): full PR review parity with Bun server#503

Merged
backnotprop merged 8 commits intomainfrom
fix/pi-pr-review-parity
Apr 7, 2026
Merged

fix(pi): full PR review parity with Bun server#503
backnotprop merged 8 commits intomainfrom
fix/pi-pr-review-parity

Conversation

@backnotprop
Copy link
Copy Markdown
Owner

@backnotprop backnotprop commented Apr 6, 2026

Summary

  • Pi PR review was completely broken — the plannotator-review command ignored PR URL arguments, always falling back to local git diffs
  • Implements full PR review flow: URL parsing, auth, PR fetch, local worktree creation (same-repo + cross-repo), cleanup on stop
  • Fixes 12+ server-level parity gaps found via exhaustive audit of every Bun review server feature against Pi
  • Fixes pre-existing bugs in both Bun and Pi: parseRemoteUrl for GitLab subgroups, shallow clone depth, --hostname flag on repo clone, Git Add button in PR mode
  • Adds Resolved/Outdated thread filters to PR comments tab

Changes

PR Review Flow (new)

  • plannotator-browser.ts: Full PR mode with worktree/clone support matching apps/hook/server/index.ts
  • index.ts: Command handler forwards PR URL argument, handles platform PR action feedback correctly
  • plannotator-events.ts: Event payload includes prUrl field

Server Parity Fixes (serverReview.ts)

  • Codex P0/P1 blocking findings override (was using raw verdict)
  • AI endpoints getCwd respects agentCwd for PR worktrees (uses shared resolveAgentCwd)
  • /api/pr-action + /api/pr-viewed: diagnostic logging
  • /api/diff: isWSL field, hasLocalAccess guard (was using isPRMode)
  • /api/diff/switch: guard on hasLocalAccess (was isPRMode)
  • /api/git-add: canStageFiles check with worktree composite diff type support
  • /api/file-content: local-first priority, "no file access" guard
  • isRemote in return structure, onReady callback
  • Error messages aligned to Bun server text
  • Export reviewRuntime — shared by browser and server, no duplication

Shared Fixes (both Bun and Pi)

  • parseRemoteUrl: full multi-segment GitLab path support, SSH+port handling, HTTPS non-standard port fix
  • Cross-repo clone: --hostname flag replaced with GH_HOST/GITLAB_HOST env vars (repo clone doesn't accept --hostname)
  • Cross-repo clone: shallow fetch depth increased from 50 to 200 commits
  • canStageFiles disabled in PR review mode (client-side fix in App.tsx)

PR Comments Tab

  • Resolved/Outdated toggle filters — click to hide resolved or outdated review threads
  • Uses same color tokens as existing badges (green/amber)
  • Integrates with existing Clear Filters and filter count

Test plan

  • TypeScript compiles clean (tsc --noEmit)
  • All 23 Pi extension tests pass
  • 3 rounds of automated code review with fixes
  • Full parity audit (27 feature areas) verified clean
  • Manual test: plannotator-review <PR-URL> in Pi opens PR diff (not local diff)
  • Manual test: local plannotator-review (no URL) still works as before
  • Manual test: Resolved/Outdated filters hide threads correctly
  • Manual test: Git Add button hidden in PR review mode

The Pi extension's plannotator-review command ignored PR URL arguments
entirely, always falling back to local git diffs. This brings Pi to
full parity with the Bun hook implementation across PR review, agent
jobs, server lifecycle, and all review endpoints.

PR review flow:
- Parse PR/MR URLs, authenticate, fetch PR metadata and diff
- Create local worktree (same-repo) or shallow clone (cross-repo)
  for agent file access, with cleanup on server stop
- Pass prMetadata, agentCwd, onCleanup to review server
- Forward prUrl through event payload and command handler

Server parity fixes:
- Add Codex P0/P1 blocking findings override to onJobComplete
- AI endpoints getCwd now respects agentCwd for PR worktrees
- /api/pr-action: add pre-submission, success, failure logging
- /api/pr-viewed: add error logging
- /api/diff: add isWSL field, use hasLocalAccess guard for
  diffType/gitContext conditionals (not isPRMode)
- /api/diff/switch: guard on hasLocalAccess with aligned error message
- /api/git-add: add canStageFiles check for unsupported diff types
- /api/file-content: reorder to local-first priority, add "no file
  access available" guard
- Add isRemote to ReviewServerResult return structure
- Add onReady callback option with (url, isRemote, port) signature
- Align all error messages to match Bun server text

For provenance purposes, this commit was AI assisted.
hasLocalAccess must be !!gitContext only (not agentCwd) — it gates
UI-facing features like diff switching and file content routing.
Agent-facing access uses hasAgentLocalAccess (agentCwd || gitContext)
inside buildCommand only. Without this fix, PR mode with a worktree
would incorrectly allow diff switching and return stale diffType.

For provenance purposes, this commit was AI assisted.
1. Exit handler missing cwd: worktree remove now passes cwd: repoDir
   so git can locate the parent repo on abnormal exit.

2. Exit listener accumulation: store handler ref, remove it in
   worktreeCleanup on normal stop. Prevents MaxListenersExceeded
   warnings after multiple PR reviews in a single Pi session.

3. Cross-repo clone --hostname flag: gh/glab repo clone doesn't
   accept --hostname. Use GH_HOST/GITLAB_HOST env var instead,
   fixing self-hosted GHE/GitLab cross-repo PR reviews.

4. AI endpoints getCwd duplication: replace verbatim copy of
   resolveAgentCwd closure with direct function reference.

5. Worktree staging broken: canStage check now parses composite
   "worktree:/path:uncommitted" diff types via parseWorktreeDiffType
   to extract the base subType before checking stageability.

For provenance purposes, this commit was AI assisted.
…eanup

1. Share reviewRuntime: export from serverReview.ts, import in
   plannotator-browser.ts. Remove duplicate nodeGitRuntime and
   runSync helper — all git calls now use shared runtime.

2. Fix parseRemoteUrl for GitLab subgroups: capture full multi-segment
   paths (group/subgroup/project) in SSH, HTTPS, and SSH+port formats.
   Fixes same-repo detection for nested GitLab groups (shared fix,
   affects both Bun and Pi).

3. Increase cross-repo shallow fetch depth from 50 to 200 commits
   in both Bun hook and Pi. Reduces "no merge base" failures for
   long-lived branches during agent reviews.

4. Fix Bun hook --hostname flag on gh/glab repo clone: replace with
   GH_HOST/GITLAB_HOST env vars (repo clone doesn't accept --hostname).
   Pi already had this fix from previous round.

5. Deregister exit handler in catch block: prevents dangling
   process.once("exit") listeners when worktree creation fails
   partway through.

For provenance purposes, this commit was AI assisted.
1. Fix parseRemoteUrl SSH regex matching HTTPS URLs with non-standard
   ports (e.g. https://gitlab.example.com:8443/...). Guard the SSH
   regex with !url.includes("://") so it only fires on actual SSH
   URLs, letting HTTPS+port fall through to the HTTPS regex.

2. Fix Pi PR approve sending "please address this feedback" to agent.
   Platform PR actions (approve/comment) return approved:false with a
   status message. Add isPRReview guard to skip the "please address"
   suffix, matching the Bun hook's if (!isPRMode) pattern.

3. Disable Git Add button in PR review mode. The server sends
   diffType: undefined in PR mode, but the client defaults to
   'uncommitted' which enables staging. Override canStageFiles to
   false when prMetadata is present. Fixes both Bun and Pi servers.

For provenance purposes, this commit was AI assisted.
1. Fix parseRemoteUrl SSH regex matching HTTPS URLs with non-standard
   ports (e.g. https://gitlab.example.com:8443/...). Guard SSH regex
   with !url.includes("://") so it only fires on actual SSH URLs.

2. Fix Pi PR approve sending "please address this feedback" to agent.
   Platform PR actions return approved:false with a status message —
   add isPRReview guard to skip the instruction suffix.

3. Disable Git Add button in PR review mode. Override canStageFiles
   to false when prMetadata is present. Fixes broken staging button
   for both Bun and Pi servers.

4. Add Resolved/Outdated toggle filters to PR comments tab. Click to
   hide resolved or outdated review threads. Uses same color tokens
   as the existing badges. Resets with Clear Filters.

For provenance purposes, this commit was AI assisted.
Change [^:]+? (one-or-more) to [^:]*? (zero-or-more) so that
git@host:x.git correctly captures 'x' instead of 'x.git'.
Without this, same-repo detection fails for short path segments.

For provenance purposes, this commit was AI assisted.
DiffViewer initialized pierreTheme with { type: 'dark', css: '' },
then computed the real theme asynchronously via requestAnimationFrame.
This caused a single frame of the diff library's default dark theme
(black background) before the actual theme CSS was applied.

Fix: compute initial bg/fg CSS synchronously in useState initializer
by reading --background/--foreground from computed styles. The full
theme (muted, primary, fonts) still applies via the existing useEffect
on the next frame, but the critical background color is correct from
the first render.

For provenance purposes, this commit was AI assisted.
@backnotprop backnotprop merged commit 181d2a1 into main Apr 7, 2026
5 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