From 27396517c94c7c6c6ecc34d86361ddeda28ef28d Mon Sep 17 00:00:00 2001 From: soundvibe Date: Wed, 16 Sep 2026 22:42:42 +0300 Subject: [PATCH 1/9] feat(review): support static patch files (PR #1331 base) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies backnotprop/plannotator#1331 onto 0.27.15: review-args gains --patch-file with parse once-required path semantics folded into the errors[] contract (hosts must surface rather than throw); the direct `plannotator review` CLI reads a static unified diff from a file or `-` (stdin) and opens a workspace-less session (diffType "static-patch"). Harness surfaces (pi command, opencode bridge) keep the PR's explicit reject for now — generalized in the follow-up commit. Co-Authored-By: Kimchi --- README.md | 1 + apps/hook/server/cli.test.ts | 3 ++- apps/hook/server/cli.ts | 8 +++++-- apps/hook/server/index.ts | 19 ++++++++++++++-- apps/pi-extension/index.ts | 4 ++++ packages/server/agent-review-message.test.ts | 12 ++++++++++ packages/shared/review-args.test.ts | 23 ++++++++++++++++++++ packages/shared/review-args.ts | 18 +++++++++++++++ packages/shared/review-core.ts | 1 + 9 files changed, 84 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index d78d9fb24..3305b4d25 100644 --- a/README.md +++ b/README.md @@ -127,6 +127,7 @@ Need a realistic document to try? Copy the [product requirements document templa /plannotator-review # Review a GitHub pull request /plannotator-review # Review a GitLab merge request plannotator review --gitbutler # Review an active GitButler workspace +plannotator review --patch-file reading.diff # Review a static caller-supplied unified diff ``` GitButler users can review the whole workspace, one stack, or one branch layer. See the [GitButler workflow guide](https://docs.plannotator.ai/open-source/workflows/gitbutler). diff --git a/apps/hook/server/cli.test.ts b/apps/hook/server/cli.test.ts index 80c9e657d..24c9e254f 100644 --- a/apps/hook/server/cli.test.ts +++ b/apps/hook/server/cli.test.ts @@ -32,7 +32,7 @@ describe("CLI top-level help", () => { expect(output).toContain("plannotator [--browser ]"); // Deliberate literal: the review usage line is API surface for agents // probing --help (and for the knowledge-skill freshness guard). - expect(output).toContain("plannotator review [--git | --gitbutler] [--base ] [--diff-type ] [--tailscale] [PR_URL]"); + expect(output).toContain("plannotator review [--git | --gitbutler] [--base ] [--diff-type ] [--patch-file ] [--tailscale] [PR_URL]"); expect(output).toContain("plannotator annotate "); expect(output).toContain("[--markdown] [--no-jina]"); expect(output).toContain("plannotator annotate-last [--stdin]"); @@ -113,6 +113,7 @@ describe("CLI subcommand help", () => { // Deliberate literals: the open-state flag tokens are API surface. expect(formatSubcommandHelp("review")).toContain("--base "); expect(formatSubcommandHelp("review")).toContain("--diff-type "); + expect(formatSubcommandHelp("review")).toContain("--patch-file "); expect(formatSubcommandHelp("review")).toContain("PR_URL"); expect(formatSubcommandHelp("annotate")).toContain("--no-jina"); expect(formatSubcommandHelp("annotate")).toContain("--require-approval"); diff --git a/apps/hook/server/cli.ts b/apps/hook/server/cli.ts index 1a9fe6848..a91589761 100644 --- a/apps/hook/server/cli.ts +++ b/apps/hook/server/cli.ts @@ -141,7 +141,7 @@ export function formatTopLevelHelp(): string { " plannotator --help", " plannotator --version, -v", " plannotator [--browser ]", - " plannotator review [--git | --gitbutler] [--base ] [--diff-type ] [--tailscale] [PR_URL]", + " plannotator review [--git | --gitbutler] [--base ] [--diff-type ] [--patch-file ] [--tailscale] [PR_URL]", " plannotator annotate [--markdown] [--no-jina] [--tailscale] [--gate] [--json] [--hook] [--require-approval] [--result-file ]", " plannotator annotate-last [--stdin] [--tailscale] [--gate] [--json] [--hook]", " plannotator copilot-last [--gate] [--json] [--hook]", @@ -175,7 +175,7 @@ export function formatTopLevelHelp(): string { export const SUBCOMMAND_HELP: Record = { review: [ "Usage:", - " plannotator review [--git | --gitbutler] [--base ] [--diff-type ] [--local | --no-local] [--tailscale] [--json] [PR_URL]", + " plannotator review [--git | --gitbutler] [--base ] [--diff-type ] [--local | --no-local] [--patch-file ] [--tailscale] [--json] [PR_URL]", "", "Review local VCS changes or a GitHub/GitLab pull request in the browser.", "", @@ -190,10 +190,13 @@ export const SUBCOMMAND_HELP: Record = { " Session-only; never changes your saved defaults. Git only.", " --local For PR review, prepare a local checkout for full file access (default)", " --no-local For PR review, skip the local checkout (diff only)", + " --patch-file Display a static unified diff from a file, or use - for stdin", " --tailscale Publish the loopback session over your tailnet via tailscale serve (HTTPS)", " --json Emit one decision/message JSON record instead of plaintext", " PR_URL GitHub PR or GitLab MR URL to review", "", + " --patch-file cannot be combined with PR_URL.", + "", "JSON output:", ' { "decision": "approved" | "annotated" | "dismissed", "message": string }', " message is the rendered plaintext output without its final console newline:", @@ -206,6 +209,7 @@ export const SUBCOMMAND_HELP: Record = { " plannotator review --git", " plannotator review --gitbutler", " plannotator review --base feature/part-1 # review one layer of a stacked branch", + " plannotator review --patch-file reading.diff", " plannotator review https://github.com/owner/repo/pull/123", ].join("\n"), annotate: [ diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 6c832847e..ddb13f200 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -815,6 +815,10 @@ if (args[0] === "sessions") { process.exit(1); } const urlArg = reviewArgs.prUrl; + if (reviewArgs.patchFile && urlArg) { + console.error("--patch-file cannot be combined with a PR/MR URL"); + process.exit(1); + } const isPRMode = urlArg !== undefined; const useLocal = isPRMode && reviewArgs.useLocal; // Caller-pinned open state: `--base` / `--diff-type` seed this session only @@ -836,7 +840,18 @@ if (args[0] === "sessions") { let worktreeCleanup: (() => void | Promise) | undefined; let workspace: Awaited> | undefined; - if (isPRMode) { + if (reviewArgs.patchFile) { + try { + rawPatch = reviewArgs.patchFile === "-" + ? await Bun.stdin.text() + : await Bun.file(reviewArgs.patchFile).text(); + gitRef = reviewArgs.patchFile === "-" ? "stdin patch" : reviewArgs.patchFile; + initialDiffType = "static-patch"; + } catch (err) { + console.error(`Failed to read patch file: ${err instanceof Error ? err.message : String(err)}`); + process.exit(1); + } + } else if (isPRMode) { // --- PR Review Mode --- // The base comes from the pull request — the open-state flags always // error here (validated before any auth check or platform fetch). @@ -1154,7 +1169,7 @@ if (args[0] === "sessions") { error: diffError, origin: detectedOrigin, project: reviewProject, - diffType: workspace ? (initialDiffType ?? workspace.diffType) : gitContext ? (initialDiffType ?? "unstaged") : undefined, + diffType: workspace ? (initialDiffType ?? workspace.diffType) : gitContext ? (initialDiffType ?? "unstaged") : initialDiffType, gitContext, initialBase: initialBaseFromFlags, initialBaseExplicit: initialBaseFromFlags !== undefined, diff --git a/apps/pi-extension/index.ts b/apps/pi-extension/index.ts index d2524aba7..499abcd65 100644 --- a/apps/pi-extension/index.ts +++ b/apps/pi-extension/index.ts @@ -689,6 +689,10 @@ export default function plannotator(pi: ExtensionAPI): void { ctx.ui.notify(`Plannotator: ${reviewArgs.errors.join("; ")}`, "error"); return; } + if (reviewArgs.patchFile) { + ctx.ui.notify("--patch-file is only supported by the direct plannotator review CLI", "error"); + return; + } const session = await startCodeReviewBrowserSession(ctx, { prUrl: reviewArgs.prUrl, vcsType: reviewArgs.vcsType, diff --git a/packages/server/agent-review-message.test.ts b/packages/server/agent-review-message.test.ts index 5bd0ace62..214143049 100644 --- a/packages/server/agent-review-message.test.ts +++ b/packages/server/agent-review-message.test.ts @@ -70,6 +70,18 @@ describe("buildAgentReviewUserMessage", () => { expect(message).toContain(patch); }); + test("uses the inline patch as Ask AI context for static patch reviews", () => { + // given + const diffType = "static-patch"; + + // when + const message = buildAgentReviewUserMessage(patch, diffType, undefined, undefined, true); + + // then + expect(message).toContain(patch); + expect(message).not.toContain("working tree"); + }); + test("treats the inline GitButler patch as authoritative", () => { const message = buildAgentReviewUserMessage( patch, diff --git a/packages/shared/review-args.test.ts b/packages/shared/review-args.test.ts index 4f000d342..a7aaa5837 100644 --- a/packages/shared/review-args.test.ts +++ b/packages/shared/review-args.test.ts @@ -170,4 +170,27 @@ describe("parseReviewArgs", () => { expect(parsed.errors).toEqual(["Unknown review option: --bse"]); expect(parsed.prUrl).toBe("https://github.com/acme/repo/pull/12"); }); + + test("parses one external patch file", () => { + // given + const input = ["--patch-file", "reading.diff"]; + + // when + const result = parseReviewArgs(input); + + // then + expect(result.patchFile).toBe("reading.diff"); + expect(result.prUrl).toBeUndefined(); + expect(result.errors).toEqual([]); + }); + + test("rejects a missing or duplicate patch file", () => { + // given + const missingPath = ["--patch-file"]; + const duplicatePath = ["--patch-file", "one.diff", "--patch-file", "two.diff"]; + + // when / then + expect(parseReviewArgs(missingPath).errors).toEqual(["--patch-file requires a path or -"]); + expect(parseReviewArgs(duplicatePath).errors).toEqual(["--patch-file may only be specified once"]); + }); }); diff --git a/packages/shared/review-args.ts b/packages/shared/review-args.ts index e1decb587..a5404b5a8 100644 --- a/packages/shared/review-args.ts +++ b/packages/shared/review-args.ts @@ -26,6 +26,7 @@ export type ReviewOpenDiffType = (typeof REVIEW_OPEN_DIFF_TYPES)[number]; export interface ParsedReviewArgs { prUrl?: string; + patchFile?: string; vcsType?: VcsSelection; useLocal: boolean; /** Compare target the session opens against (`--base `). */ @@ -53,6 +54,8 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { const errors: string[] = []; const positional: string[] = []; + let patchFile: string | undefined; + // Index-based so value-taking flags consume their value token before the // positional collector sees it — otherwise `--base main ` would put // "main" in positional[0] and lose the URL. @@ -65,6 +68,20 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { case "--gitbutler": vcsType = "gitbutler"; break; + case "--patch-file": { + const value = tokens[i + 1]; + if (value === undefined || value.startsWith("--")) { + errors.push("--patch-file requires a path or -"); + break; + } + i++; + if (patchFile !== undefined) { + errors.push("--patch-file may only be specified once"); + break; + } + patchFile = value; + break; + } case "--local": useLocal = true; break; @@ -131,6 +148,7 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { const target = positional[0]; return { prUrl: target && isReviewUrl(target) ? target : undefined, + patchFile, vcsType, useLocal, base, diff --git a/packages/shared/review-core.ts b/packages/shared/review-core.ts index 6d42051b9..fc2cdacc7 100644 --- a/packages/shared/review-core.ts +++ b/packages/shared/review-core.ts @@ -43,6 +43,7 @@ export type DiffType = | `commit:${string}` | `worktree:${string}` | `gitbutler:${string}` + | "static-patch" | "p4-default" | `p4-changelist:${string}`; From be16d430ee90c6922f29b9b9a37ee00587ded62c Mon Sep 17 00:00:00 2001 From: soundvibe Date: Wed, 16 Sep 2026 22:46:47 +0300 Subject: [PATCH 2/9] feat(review): static patch review across harnesses (pi, opencode) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalizes #1331's CLI-only --patch-file to the in-process harnesses: - pi: openCodeReview / startCodeReviewBrowserSession accept `patch` (inline unified diff), `patchFile` (path read at call time, resolved against the caller cwd), and `patchLabel`; static sessions open the same review server with diffType "static-patch" and no workspace / gitContext, so no repo is required. The shared `plannotator:request` bus forwards the three fields on the `code-review` action, and `/plannotator-review --patch-file ` now works inside pi instead of erroring. patch* and prUrl remain mutually exclusive. - opencode: the OpenCode bridge (opencode-review entrypoint) reads `--patch-file ` itself — relative to PLANNOTATOR_CWD or the process cwd — and starts the review in static-patch mode. `-` (stdin) is rejected with guidance: the bridge's stdin carries the input JSON. - docs: pi README documents the new code-review payload fields and the command flag. Tests: `bun test apps/pi-extension` (276 pass; the same 2 config tests fail on a clean tree — env-dependent baseline), `bun test apps/opencode-plugin`, `bun test apps/hook/server`, `tsc -p apps/pi-extension/tsconfig.json` all green. Co-Authored-By: Kimchi --- apps/hook/server/index.ts | 25 +++++++++++++++++++- apps/pi-extension/README.md | 10 ++++++-- apps/pi-extension/index.ts | 8 +++++-- apps/pi-extension/plannotator-browser.ts | 30 ++++++++++++++++++++++++ apps/pi-extension/plannotator-events.ts | 11 +++++++++ 5 files changed, 79 insertions(+), 5 deletions(-) diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index ddb13f200..071cea203 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -1878,7 +1878,30 @@ if (args[0] === "sessions") { let workspace: Awaited> | undefined; let agentCwd: string | undefined; - if (isPRMode) { + if (reviewArgs.patchFile) { + if (urlArg) { + console.error("--patch-file cannot be combined with a PR/MR URL"); + process.exit(1); + } + if (reviewArgs.patchFile === "-") { + // The bridge's stdin carries the input JSON; a stdin patch has no + // channel. Direct `plannotator review --patch-file -` remains the way. + console.error("--patch-file - (stdin) is not available through the OpenCode bridge; pass a file path"); + process.exit(1); + } + try { + const bridgeCwd = process.env.PLANNOTATOR_CWD || process.cwd(); + const patchPath = reviewArgs.patchFile.startsWith("/") + ? reviewArgs.patchFile + : `${bridgeCwd}/${reviewArgs.patchFile}`; + rawPatch = await Bun.file(patchPath).text(); + gitRef = reviewArgs.patchFile; + userDiffType = "static-patch"; + } catch (err) { + console.error(`Failed to read patch file: ${err instanceof Error ? err.message : String(err)}`); + process.exit(1); + } + } else if (isPRMode) { await resolveCliReviewOpenState(reviewArgs, { isPRMode: true, isWorkspace: false, diff --git a/apps/pi-extension/README.md b/apps/pi-extension/README.md index 707663a3f..5fd057d81 100644 --- a/apps/pi-extension/README.md +++ b/apps/pi-extension/README.md @@ -203,7 +203,7 @@ Use these inside `instructions` strings. They render once, when the phase is ent ### Code review -Run `/plannotator-review` to open your current VCS changes in the code review UI. Annotate specific lines, switch between the modes supported by the detected Git, GitButler, or JJ provider, and submit feedback that gets sent to the agent. Pass `--git` or `--gitbutler` to force that provider; GitButler requires `but` 0.21.0 or newer on `PATH`. +Run `/plannotator-review` to open your current VCS changes in the code review UI. Annotate specific lines, switch between the modes supported by the detected Git, GitButler, or JJ provider, and submit feedback that gets sent to the agent. Pass `--git` or `--gitbutler` to force that provider; GitButler requires `but` 0.21.0 or newer on `PATH`. Pass `--patch-file ` to review a static caller-supplied unified diff without a repository. ### Shared Plannotator event API @@ -213,7 +213,13 @@ Supported actions and payloads: - `plan-review`: `{ planContent, planFilePath? }` - `review-status`: `{ reviewId }` -- `code-review`: `{ cwd?, defaultBranch?, diffType? }` +- `code-review`: `{ cwd?, defaultBranch?, diffType?, vcsType?, useLocal?, prUrl?, patch?, patchFile?, patchLabel? }` + + Pass `patch` (inline unified diff) or `patchFile` (path read at request time, + resolved against `cwd`) to review a caller-supplied patch without a local + repository — the review opens in static-patch mode with no file-system + affordances that would need the worktree. `patchLabel` sets the header + label. `patch`/`patchFile` are mutually exclusive with `prUrl`. - `annotate`: `{ filePath, markdown?, mode?, folderPath? }` - `annotate-last`: `{ markdown? }` - `archive`: `{ customPlanPath? }` diff --git a/apps/pi-extension/index.ts b/apps/pi-extension/index.ts index 499abcd65..77048b4b8 100644 --- a/apps/pi-extension/index.ts +++ b/apps/pi-extension/index.ts @@ -689,12 +689,16 @@ export default function plannotator(pi: ExtensionAPI): void { ctx.ui.notify(`Plannotator: ${reviewArgs.errors.join("; ")}`, "error"); return; } - if (reviewArgs.patchFile) { - ctx.ui.notify("--patch-file is only supported by the direct plannotator review CLI", "error"); + if (reviewArgs.patchFile && reviewArgs.prUrl) { + ctx.ui.notify("--patch-file cannot be combined with a PR/MR URL", "error"); return; } const session = await startCodeReviewBrowserSession(ctx, { prUrl: reviewArgs.prUrl, + // --patch-file: static patch mode — read at session open, + // resolved against the session cwd (fs resolve in + // createCodeReviewBrowserSession). + patchFile: reviewArgs.patchFile, vcsType: reviewArgs.vcsType, useLocal: reviewArgs.useLocal, // --base / --diff-type: session-only open state from user flags. diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index b1c80a079..fd72fe42e 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -75,6 +75,20 @@ type CodeReviewOptions = { prUrl?: string; vcsType?: VcsSelection; useLocal?: boolean; + /** + * Inline unified-diff content to review without a repo (static patch mode). + * Mutually exclusive with `prUrl`, local/VCS modes — mirrors the direct CLI's + * `--patch-file` contract. Wins over the caller's cwd/git detection entirely. + */ + patch?: string; + /** + * Path to a unified-diff file to review without a repo — the file is read + * once at call time (resolved relative to the caller's cwd, not ctx.cwd). + */ + patchFile?: string; + /** Display label for a static patch (header + share title); defaults to + * the patchFile path or "inline patch". */ + patchLabel?: string; /** * `defaultBranch` / `diffType` came from user CLI flags (`--base` / * `--diff-type` on /plannotator-review): validate strictly (provider @@ -583,6 +597,22 @@ async function createCodeReviewBrowserSession( worktreeCleanup = undefined; } } + } else if (options.patch !== undefined || options.patchFile !== undefined) { + // --- Static Patch Mode --- + // Caller-supplied unified diff, reviewed without any repository: the + // server serves rawPatch as-is, workspace undefined, gitContext undefined, + // diffType "static-patch" — identical to the direct CLI's --patch-file + // path. No refresh: there is no live tree to recomputed against; the + // initial patch is the session's whole content. + if (options.prUrl) { + throw new Error("patch/patchFile cannot be combined with prUrl"); + } + rawPatch = options.patch ?? readFileSync(resolve(options.cwd ?? ctx.cwd, options.patchFile!), "utf-8"); + if (!rawPatch.trim()) { + throw new Error("Static patch review requires non-empty unified-diff content."); + } + gitRef = options.patchLabel ?? options.patchFile ?? "inline patch"; + diffType = "static-patch"; } else { // --- Local Review Mode --- const cwd = options.cwd ?? ctx.cwd; diff --git a/apps/pi-extension/plannotator-events.ts b/apps/pi-extension/plannotator-events.ts index 3e3012b9a..0359b30f5 100644 --- a/apps/pi-extension/plannotator-events.ts +++ b/apps/pi-extension/plannotator-events.ts @@ -171,6 +171,14 @@ export interface PlannotatorCodeReviewPayload { useLocal?: boolean; cwd?: string; prUrl?: string; + /** Inline unified-diff content — static patch mode (no repo required). + * Mutually exclusive with `prUrl`. */ + patch?: string; + /** Path to a unified-diff file — static patch mode. Read by the host at + * request time, resolved against payload.cwd (or the session cwd). */ + patchFile?: string; + /** Display label for the static patch view. */ + patchLabel?: string; } export interface PlannotatorCodeReviewResult { @@ -376,6 +384,9 @@ export function registerPlannotatorEventListeners( vcsType: request.payload?.vcsType, useLocal: request.payload?.useLocal, prUrl: request.payload?.prUrl, + patch: request.payload?.patch, + patchFile: request.payload?.patchFile, + patchLabel: request.payload?.patchLabel, }); request.respond({ status: "handled", result }); return; From ebd17acb2a0e777f789c2877ef0a93b1380b6875 Mon Sep 17 00:00:00 2001 From: soundvibe Date: Wed, 16 Sep 2026 23:20:30 +0300 Subject: [PATCH 3/9] Tighten static patch review: single parser validator, shared reader, drop unused options - Move --patch-file conflict validation into parseReviewArgs (PR URL, --base, --diff-type, --git/--gitbutler, explicit --local) so all three runtimes reject in one place; delete the duplicated guards in the direct CLI and OpenCode bridge branches. - Extract readStaticPatch() shared by both Bun review entry paths; reject empty patch content on the Bun path to match Pi. - Remove the uncalled inline `patch` and `patchLabel` options from the Pi session/event API. - Document --patch-file in the plannotator knowledge skill. Co-Authored-By: Kimchi --- apps/hook/server/index.ts | 57 +++++++++++------------- apps/pi-extension/README.md | 11 +++-- apps/pi-extension/index.ts | 7 --- apps/pi-extension/plannotator-browser.ts | 21 +++------ apps/pi-extension/plannotator-events.ts | 12 ++--- apps/skills/core/plannotator/SKILL.md | 3 +- packages/shared/review-args.test.ts | 14 ++++++ packages/shared/review-args.ts | 16 +++++++ 8 files changed, 74 insertions(+), 67 deletions(-) diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 071cea203..6032188d6 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -455,6 +455,25 @@ if (helpSubcommand) { exitOnUnknownSubcommand(args); +// Read a caller-supplied unified diff for static patch mode (`--patch-file`). +// "-" means stdin; file paths resolve against the given cwd. A read failure +// is a startup failure: exit 1 like every other review startup failure. +async function readStaticPatch(patchFile: string, cwd: string): Promise<{ rawPatch: string; gitRef: string }> { + try { + const rawPatch = patchFile === "-" + ? await Bun.stdin.text() + : await Bun.file(path.resolve(cwd, patchFile)).text(); + if (!rawPatch.trim()) { + console.error("Static patch review requires non-empty unified-diff content."); + process.exit(1); + } + return { rawPatch, gitRef: patchFile === "-" ? "stdin patch" : patchFile }; + } catch (err) { + console.error(`Failed to read patch file: ${err instanceof Error ? err.message : String(err)}`); + process.exit(1); + } +} + if (args[0] === "uninstall") { let options: ReturnType; try { @@ -815,10 +834,6 @@ if (args[0] === "sessions") { process.exit(1); } const urlArg = reviewArgs.prUrl; - if (reviewArgs.patchFile && urlArg) { - console.error("--patch-file cannot be combined with a PR/MR URL"); - process.exit(1); - } const isPRMode = urlArg !== undefined; const useLocal = isPRMode && reviewArgs.useLocal; // Caller-pinned open state: `--base` / `--diff-type` seed this session only @@ -841,16 +856,10 @@ if (args[0] === "sessions") { let workspace: Awaited> | undefined; if (reviewArgs.patchFile) { - try { - rawPatch = reviewArgs.patchFile === "-" - ? await Bun.stdin.text() - : await Bun.file(reviewArgs.patchFile).text(); - gitRef = reviewArgs.patchFile === "-" ? "stdin patch" : reviewArgs.patchFile; - initialDiffType = "static-patch"; - } catch (err) { - console.error(`Failed to read patch file: ${err instanceof Error ? err.message : String(err)}`); - process.exit(1); - } + const patch = await readStaticPatch(reviewArgs.patchFile, process.env.PLANNOTATOR_CWD || process.cwd()); + rawPatch = patch.rawPatch; + gitRef = patch.gitRef; + initialDiffType = "static-patch"; } else if (isPRMode) { // --- PR Review Mode --- // The base comes from the pull request — the open-state flags always @@ -1879,28 +1888,16 @@ if (args[0] === "sessions") { let agentCwd: string | undefined; if (reviewArgs.patchFile) { - if (urlArg) { - console.error("--patch-file cannot be combined with a PR/MR URL"); - process.exit(1); - } if (reviewArgs.patchFile === "-") { // The bridge's stdin carries the input JSON; a stdin patch has no // channel. Direct `plannotator review --patch-file -` remains the way. console.error("--patch-file - (stdin) is not available through the OpenCode bridge; pass a file path"); process.exit(1); } - try { - const bridgeCwd = process.env.PLANNOTATOR_CWD || process.cwd(); - const patchPath = reviewArgs.patchFile.startsWith("/") - ? reviewArgs.patchFile - : `${bridgeCwd}/${reviewArgs.patchFile}`; - rawPatch = await Bun.file(patchPath).text(); - gitRef = reviewArgs.patchFile; - userDiffType = "static-patch"; - } catch (err) { - console.error(`Failed to read patch file: ${err instanceof Error ? err.message : String(err)}`); - process.exit(1); - } + const patch = await readStaticPatch(reviewArgs.patchFile, process.env.PLANNOTATOR_CWD || process.cwd()); + rawPatch = patch.rawPatch; + gitRef = patch.gitRef; + userDiffType = "static-patch"; } else if (isPRMode) { await resolveCliReviewOpenState(reviewArgs, { isPRMode: true, diff --git a/apps/pi-extension/README.md b/apps/pi-extension/README.md index 5fd057d81..7a7b294c2 100644 --- a/apps/pi-extension/README.md +++ b/apps/pi-extension/README.md @@ -213,13 +213,12 @@ Supported actions and payloads: - `plan-review`: `{ planContent, planFilePath? }` - `review-status`: `{ reviewId }` -- `code-review`: `{ cwd?, defaultBranch?, diffType?, vcsType?, useLocal?, prUrl?, patch?, patchFile?, patchLabel? }` +- `code-review`: `{ cwd?, defaultBranch?, diffType?, vcsType?, useLocal?, prUrl?, patchFile? }` - Pass `patch` (inline unified diff) or `patchFile` (path read at request time, - resolved against `cwd`) to review a caller-supplied patch without a local - repository — the review opens in static-patch mode with no file-system - affordances that would need the worktree. `patchLabel` sets the header - label. `patch`/`patchFile` are mutually exclusive with `prUrl`. + Pass `patchFile` (path read at request time, resolved against `cwd`) to + review a caller-supplied patch without a local repository — the review opens + in static-patch mode with no file-system affordances that would need the + worktree. `patchFile` is mutually exclusive with `prUrl`. - `annotate`: `{ filePath, markdown?, mode?, folderPath? }` - `annotate-last`: `{ markdown? }` - `archive`: `{ customPlanPath? }` diff --git a/apps/pi-extension/index.ts b/apps/pi-extension/index.ts index 77048b4b8..166a901ef 100644 --- a/apps/pi-extension/index.ts +++ b/apps/pi-extension/index.ts @@ -689,15 +689,8 @@ export default function plannotator(pi: ExtensionAPI): void { ctx.ui.notify(`Plannotator: ${reviewArgs.errors.join("; ")}`, "error"); return; } - if (reviewArgs.patchFile && reviewArgs.prUrl) { - ctx.ui.notify("--patch-file cannot be combined with a PR/MR URL", "error"); - return; - } const session = await startCodeReviewBrowserSession(ctx, { prUrl: reviewArgs.prUrl, - // --patch-file: static patch mode — read at session open, - // resolved against the session cwd (fs resolve in - // createCodeReviewBrowserSession). patchFile: reviewArgs.patchFile, vcsType: reviewArgs.vcsType, useLocal: reviewArgs.useLocal, diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index fd72fe42e..d60b1126a 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -75,20 +75,13 @@ type CodeReviewOptions = { prUrl?: string; vcsType?: VcsSelection; useLocal?: boolean; - /** - * Inline unified-diff content to review without a repo (static patch mode). - * Mutually exclusive with `prUrl`, local/VCS modes — mirrors the direct CLI's - * `--patch-file` contract. Wins over the caller's cwd/git detection entirely. - */ - patch?: string; /** * Path to a unified-diff file to review without a repo — the file is read * once at call time (resolved relative to the caller's cwd, not ctx.cwd). + * Mutually exclusive with `prUrl` and local/VCS modes. The path doubles as + * the display label in the review header. */ patchFile?: string; - /** Display label for a static patch (header + share title); defaults to - * the patchFile path or "inline patch". */ - patchLabel?: string; /** * `defaultBranch` / `diffType` came from user CLI flags (`--base` / * `--diff-type` on /plannotator-review): validate strictly (provider @@ -597,21 +590,21 @@ async function createCodeReviewBrowserSession( worktreeCleanup = undefined; } } - } else if (options.patch !== undefined || options.patchFile !== undefined) { + } else if (options.patchFile !== undefined) { // --- Static Patch Mode --- // Caller-supplied unified diff, reviewed without any repository: the // server serves rawPatch as-is, workspace undefined, gitContext undefined, // diffType "static-patch" — identical to the direct CLI's --patch-file - // path. No refresh: there is no live tree to recomputed against; the + // path. No refresh: there is no live tree to recompute against; the // initial patch is the session's whole content. if (options.prUrl) { - throw new Error("patch/patchFile cannot be combined with prUrl"); + throw new Error("--patch-file cannot be combined with a PR/MR URL"); } - rawPatch = options.patch ?? readFileSync(resolve(options.cwd ?? ctx.cwd, options.patchFile!), "utf-8"); + rawPatch = readFileSync(resolve(options.cwd ?? ctx.cwd, options.patchFile), "utf-8"); if (!rawPatch.trim()) { throw new Error("Static patch review requires non-empty unified-diff content."); } - gitRef = options.patchLabel ?? options.patchFile ?? "inline patch"; + gitRef = options.patchFile; diffType = "static-patch"; } else { // --- Local Review Mode --- diff --git a/apps/pi-extension/plannotator-events.ts b/apps/pi-extension/plannotator-events.ts index 0359b30f5..4a84b3e7e 100644 --- a/apps/pi-extension/plannotator-events.ts +++ b/apps/pi-extension/plannotator-events.ts @@ -171,14 +171,10 @@ export interface PlannotatorCodeReviewPayload { useLocal?: boolean; cwd?: string; prUrl?: string; - /** Inline unified-diff content — static patch mode (no repo required). - * Mutually exclusive with `prUrl`. */ - patch?: string; - /** Path to a unified-diff file — static patch mode. Read by the host at - * request time, resolved against payload.cwd (or the session cwd). */ + /** Path to a unified-diff file — static patch mode (no repo required). + * Mutually exclusive with `prUrl`. Read by the host at request time, + * resolved against payload.cwd (or the session cwd). */ patchFile?: string; - /** Display label for the static patch view. */ - patchLabel?: string; } export interface PlannotatorCodeReviewResult { @@ -384,9 +380,7 @@ export function registerPlannotatorEventListeners( vcsType: request.payload?.vcsType, useLocal: request.payload?.useLocal, prUrl: request.payload?.prUrl, - patch: request.payload?.patch, patchFile: request.payload?.patchFile, - patchLabel: request.payload?.patchLabel, }); request.respond({ status: "handled", result }); return; diff --git a/apps/skills/core/plannotator/SKILL.md b/apps/skills/core/plannotator/SKILL.md index c4f618b41..e6ae5faa7 100644 --- a/apps/skills/core/plannotator/SKILL.md +++ b/apps/skills/core/plannotator/SKILL.md @@ -41,7 +41,7 @@ Stdout is the interface, but its contract is command-specific. For `annotate` an ## plannotator review ```bash -plannotator review [--git | --gitbutler] [--base ] [--diff-type ] [--local | --no-local] [--tailscale] [--json] [PR_URL] +plannotator review [--git | --gitbutler] [--base ] [--diff-type ] [--local | --no-local] [--patch-file ] [--tailscale] [--json] [PR_URL] ``` Reviews local VCS changes, or a pull request when a URL is given. Default stdout stays plaintext: the existing close message, approval prompt, or feedback. @@ -54,6 +54,7 @@ Classify the outcome only by `decision`, never by `message` text. Notes on an `a - The default diff is "everything a PR would show now": merge-base of the trunk vs the working tree plus untracked files. `--base ` opens the session against a different compare target (branch, `origin/`, tag, or commit) and `--diff-type ` opens it in a different mode (`since-base`, `merge-base`, `branch`, `uncommitted`, `staged`, `unstaged`, `last-commit`, `local-vs-remote`, `all`). Both are **session-only**: the reviewer can change either in the UI, and neither writes the user's saved defaults. - **Reviewing one layer of a stacked branch? Pass `--base `** — `plannotator review --base feature/part-1` shows only what this layer adds, instead of everything since `main`. - Both flags are git-only: they error on jj, GitButler, Perforce, multi-repo workspace reviews, and PR URLs (a PR's base comes from the pull request). A `--base` ref that does not resolve is a startup error naming near-match branches, never a silently wrong diff. +- `--patch-file ` reviews a static caller-supplied unified diff with no repository at all (use `-` to read it from stdin): the session serves the patch as-is with no file-system affordances that need a worktree. It cannot be combined with a PR/MR URL, `--base`, `--diff-type`, `--git`/`--gitbutler`, or `--local`. - PR review (`plannotator review https://github.com/owner/repo/pull/123`, GitLab MR URLs too) needs an authenticated `gh` or `glab` CLI. `--local` (the default) builds a local checkout of the PR head in the background for full file access; `--no-local` skips it and reviews the platform diff only. - `--tailscale` publishes the loopback session over the user's tailnet via `tailscale serve` (HTTPS, never public) and prints the URL with a QR code. A publish failure exits nonzero instead of leaving the server hanging. diff --git a/packages/shared/review-args.test.ts b/packages/shared/review-args.test.ts index a7aaa5837..cbcd22e11 100644 --- a/packages/shared/review-args.test.ts +++ b/packages/shared/review-args.test.ts @@ -184,6 +184,20 @@ describe("parseReviewArgs", () => { expect(result.errors).toEqual([]); }); + test("rejects patch file combined with VCS/PR selectors", () => { + // given + const withPrUrl = ["https://github.com/acme/repo/pull/12", "--patch-file", "reading.diff"]; + const withBase = ["--patch-file", "reading.diff", "--base", "main"]; + const withDiffType = ["--patch-file", "reading.diff", "--diff-type", "staged"]; + const withProvider = ["--patch-file", "reading.diff", "--git"]; + + // when / then + expect(parseReviewArgs(withPrUrl).errors).toContain("--patch-file cannot be combined with a PR/MR URL"); + expect(parseReviewArgs(withBase).errors).toContain("--patch-file cannot be combined with --base"); + expect(parseReviewArgs(withDiffType).errors).toContain("--patch-file cannot be combined with --diff-type"); + expect(parseReviewArgs(withProvider).errors).toContain("--patch-file cannot be combined with --git/--gitbutler"); + }); + test("rejects a missing or duplicate patch file", () => { // given const missingPath = ["--patch-file"]; diff --git a/packages/shared/review-args.ts b/packages/shared/review-args.ts index a5404b5a8..c91c3d3ed 100644 --- a/packages/shared/review-args.ts +++ b/packages/shared/review-args.ts @@ -55,6 +55,9 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { const positional: string[] = []; let patchFile: string | undefined; + // --local conflicts with --patch-file only when the user typed it: its + // value defaults to true, so check the flag's presence, not the value. + let localFlagSeen = false; // Index-based so value-taking flags consume their value token before the // positional collector sees it — otherwise `--base main ` would put @@ -84,6 +87,7 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { } case "--local": useLocal = true; + localFlagSeen = true; break; case "--no-local": useLocal = false; @@ -146,6 +150,18 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { } const target = positional[0]; + // Static patch mode wins over VCS detection entirely, so every VCS/PR + // selector combined with it is a usage error — fail loudly in one place + // rather than silently ignoring the flag in each runtime. + if (patchFile !== undefined) { + if (target && isReviewUrl(target)) { + errors.push("--patch-file cannot be combined with a PR/MR URL"); + } + if (base) errors.push("--patch-file cannot be combined with --base"); + if (diffType) errors.push("--patch-file cannot be combined with --diff-type"); + if (vcsType) errors.push("--patch-file cannot be combined with --git/--gitbutler"); + if (localFlagSeen) errors.push("--patch-file cannot be combined with --local"); + } return { prUrl: target && isReviewUrl(target) ? target : undefined, patchFile, From af96dfb66d41cafc60e2953614db4aef180a2ea1 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 00:21:22 -0700 Subject: [PATCH 4/9] feat(review): advertise static-patch mode and refuse working-tree endpoints A `--patch-file` session has no repo, no worktree and no VCS, but the payload never said so: `diffType` was withheld (it is gated on local access), so the browser stayed on its `uncommitted` default and rendered Git Add buttons and "No uncommitted changes"; `repoInfo` advertised whatever repo the server process happened to sit in. Both runtimes now carry `sourceKind: "patch"` beside `approvalNotesSupported` on every diff payload (absent reads as "vcs", so an ordinary review's payload is byte-identical), serve `diffType` in patch mode, and omit `repoInfo`. `/api/git-add`, `/api/file-content` and `/api/open-in` answer 400, and `/api/open-in/apps` advertises none: the patch's paths belong to whatever tree produced it, so resolving them against this process's cwd would stage, read, or open an unrelated same-named file. --- apps/pi-extension/server/serverReview.ts | 50 ++++++++++++++++++++- packages/server/review.ts | 57 ++++++++++++++++++++++-- packages/shared/review-core.ts | 21 +++++++++ packages/shared/types.ts | 1 + 4 files changed, 124 insertions(+), 5 deletions(-) diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index 0f710ae58..c4702d8d5 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -49,6 +49,7 @@ import { isBinaryPatchFile, isSameCwdCommitSwitch, listPatchFiles, + STATIC_PATCH_DIFF_TYPE, parseCommitDiffType, parseWorktreeDiffType, resolveBaseBranch, @@ -430,7 +431,12 @@ export async function startReviewServer(options: { // Non-fatal: viewed state is best-effort } } - let repoInfo = prMeta + // Static patch: the session has no repository. Whatever repo the process + // happens to sit in is NOT the patch's origin, and advertising it would put + // an unrelated repo and branch in the review header. + let repoInfo = options.diffType === STATIC_PATCH_DIFF_TYPE + ? undefined + : prMeta ? { display: getDisplayRepo(prMeta), branch: `${getMRLabel(prMeta)} ${getMRNumberLabel(prMeta)}`, @@ -1724,6 +1730,15 @@ export async function startReviewServer(options: { // Session-constant capability advert; rides every diff payload (see the // option's doc). Absent option = false, so old callers advertise honestly. const approvalNotesSupported = options.approvalNotesSupported === true; + // Static patch mode (`--patch-file`): caller-supplied diff bytes, no repo, + // no working tree. Advertised as `sourceKind: "patch"` on every diff payload + // (absent reads as "vcs") and enforced by 400ing the endpoints that would + // otherwise resolve the patch's paths against an unrelated cwd. Mirrors + // packages/server/review.ts. + const isStaticPatchMode = options.diffType === STATIC_PATCH_DIFF_TYPE; + const sourceKindAdvert = isStaticPatchMode + ? ({ sourceKind: "patch" } as const) + : ({} as Record); const shareBaseUrl = (options.shareBaseUrl ?? process.env.PLANNOTATOR_SHARE_URL) || undefined; const pasteApiUrl = @@ -2086,7 +2101,7 @@ export async function startReviewServer(options: { snapshotId: servedSnapshotId, origin: options.origin ?? "pi", mode: isWorkspaceMode ? "workspace" : undefined, - diffType: hasLocalAccess || isWorkspaceMode ? servedDiffType : undefined, + diffType: hasLocalAccess || isWorkspaceMode || isStaticPatchMode ? servedDiffType : undefined, // Echo the active base so page refresh/reconnect rehydrates the // picker to what the server is actually using, not the detected default. base: hasLocalAccess ? servedBase : undefined, @@ -2095,6 +2110,7 @@ export async function startReviewServer(options: { gitContext: hasLocalAccess ? servedGitContext : undefined, sharingEnabled, approvalNotesSupported, + ...sourceKindAdvert, // Mount is the only place the pin matters, so it rides /api/diff // alone (not the switch endpoints). ...(options.openStatePinned && { openStatePinned: true }), @@ -2398,6 +2414,7 @@ export async function startReviewServer(options: { gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, diffType: currentDiffType, diffOptions: workspace.diffOptions, hideWhitespace: currentHideWhitespace, @@ -2541,6 +2558,7 @@ export async function startReviewServer(options: { gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, diffType: currentDiffType, // Echo the base the server actually used. resolveBaseBranch // trusts the caller verbatim; this echo lets the client @@ -2603,6 +2621,7 @@ export async function startReviewServer(options: { gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, prDiffScope: currentPRDiffScope, ...(layerPatchIncomplete ? { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable } : {}), ...(currentError ? { error: currentError } : {}), @@ -2669,6 +2688,7 @@ export async function startReviewServer(options: { gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, prDiffScope: currentPRDiffScope, ...(layerPatchIncomplete ? { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable } : {}), ...((currentError ?? upgradeError) ? { error: currentError ?? upgradeError } : {}), @@ -2709,6 +2729,7 @@ export async function startReviewServer(options: { gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, prDiffScope: currentPRDiffScope, semanticDiff: await getSemanticDiffAdvert(), callFlow: await getCallFlowAdvert(), @@ -2794,6 +2815,7 @@ export async function startReviewServer(options: { gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, prMetadata: pr.metadata, // The new PR's checkout (null while warming) so Open-in re-roots // immediately on switch instead of waiting for the 5s probe. @@ -2984,6 +3006,12 @@ export async function startReviewServer(options: { json(res, { error: message }, 500); } } else if (url.pathname === "/api/file-content" && req.method === "GET") { + // No working tree behind a static patch: the patch IS the whole content + // of the session, so there is nothing to expand into. + if (isStaticPatchMode) { + json(res, { error: "File content is unavailable for a static patch review" }, 400); + return; + } const filePath = url.searchParams.get("path"); if (!filePath) { json(res, { error: "Missing path" }, 400); @@ -3311,6 +3339,10 @@ export async function startReviewServer(options: { } json(res, { instructions: writeGuideInstructions(instructions) }); } else if (url.pathname === "/api/git-add" && req.method === "POST") { + if (isStaticPatchMode) { + json(res, { error: "Staging is unavailable for a static patch review" }, 400); + return; + } try { const body = await parseBody(req); const filePath = body.filePath as string | undefined; @@ -3362,6 +3394,13 @@ export async function startReviewServer(options: { json(res, { error: message }, 500); } } else if (url.pathname === "/api/open-in/apps" && req.method === "GET") { + // Static patch mode has no tree the patch's paths belong to, so + // advertise no apps: the client hides the control rather than offering + // to open a same-named file from an unrelated checkout. + if (isStaticPatchMode) { + json(res, { available: false, apps: [] }); + return; + } // Remote/headless sessions can't open apps on the user's machine — // report unavailable so the UI hides the control entirely. if (isRemote) { @@ -3374,6 +3413,13 @@ export async function startReviewServer(options: { json(res, { error: "Open in app is unavailable for committed GitButler views" }, 400); return; } + // A static patch's paths belong to whatever tree produced the patch, + // which this process cannot know — resolving them against the session + // cwd would open an unrelated file with the same name. + if (isStaticPatchMode) { + json(res, { ok: false, error: "Open in app is unavailable for a static patch review" }, 400); + return; + } if (isRemote) { json(res, { ok: false, error: "Open in app is unavailable in remote sessions" }, 400); return; diff --git a/packages/server/review.ts b/packages/server/review.ts index 1ea03532c..ff09408c9 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -24,6 +24,7 @@ import { detectRemoteDefaultInfo, isBinaryPatchFile, listPatchFiles, + STATIC_PATCH_DIFF_TYPE, type RemoteDefaultInfo, type SinceBaseSections, } from "@plannotator/shared/review-core"; @@ -278,6 +279,16 @@ export async function startReviewServer( // Session-constant capability advert; rides every diff payload (see the // option's doc). Absent option = false, so old callers advertise honestly. const approvalNotesSupported = options.approvalNotesSupported === true; + // Static patch mode (`plannotator review --patch-file`): the diff is + // caller-supplied bytes, so there is no repo, no working tree and no VCS + // behind it. Advertised to the client as `sourceKind: "patch"` on every diff + // payload (absent reads as "vcs"), and enforced here by 400ing the endpoints + // that would otherwise resolve patch paths against whatever cwd the server + // happens to run in. + const isStaticPatchMode = options.diffType === STATIC_PATCH_DIFF_TYPE; + const sourceKindAdvert = isStaticPatchMode + ? ({ sourceKind: "patch" } as const) + : ({} as Record); const submitPlatformReview = options.prReviewSubmitter ?? submitPRReview; const aiEnabled = resolveAIEnabled(); @@ -1708,12 +1719,17 @@ export async function startReviewServer( // Detect repo info (cached for this session) // In PR mode, derive from metadata instead of local git - let repoInfo = isPRMode && prMetadata + // Static patch: the session has no repository. Whatever repo the process + // happens to sit in is NOT the patch's origin, and advertising it would put + // an unrelated repo and branch in the review header. + let repoInfo = isStaticPatchMode + ? undefined + : isPRMode && prMetadata ? { display: getDisplayRepo(prMetadata), branch: `${getMRLabel(prMetadata)} ${getMRNumberLabel(prMetadata)}` } : workspace ? { display: basename(workspace.root), branch: "Workspace" } : await getRepoInfo(); - if (gitContext?.repository?.displayFallback) { + if (!isStaticPatchMode && gitContext?.repository?.displayFallback) { repoInfo = { ...repoInfo, display: repoInfo?.display || gitContext.repository.displayFallback, @@ -2070,7 +2086,7 @@ export async function startReviewServer( snapshotId: servedSnapshotId, origin, mode: isWorkspaceMode ? "workspace" : undefined, - diffType: hasLocalAccess || isWorkspaceMode ? servedDiffType : undefined, + diffType: hasLocalAccess || isWorkspaceMode || isStaticPatchMode ? servedDiffType : undefined, // Echo the active base so a page refresh or reconnect rehydrates // the picker to what the server is actually using — not the // detected default. @@ -2080,6 +2096,7 @@ export async function startReviewServer( gitContext: hasLocalAccess ? servedGitContext : undefined, sharingEnabled, approvalNotesSupported, + ...sourceKindAdvert, // Mount is the only place the pin matters, so it rides /api/diff // alone (not the switch endpoints). ...(options.openStatePinned && { openStatePinned: true }), @@ -2119,6 +2136,10 @@ export async function startReviewServer( // API: List apps the host can open a file in (Open in App control). if (url.pathname === "/api/open-in/apps" && req.method === "GET") { + // Static patch mode has no tree the patch's paths belong to, so + // advertise no apps: the client hides the control rather than + // offering to open a same-named file from an unrelated checkout. + if (isStaticPatchMode) return Response.json({ available: false, apps: [] }); return handleOpenInApps(); } @@ -2134,6 +2155,16 @@ export async function startReviewServer( { status: 400 }, ); } + // A static patch's paths are relative to whatever tree produced + // the patch, which this process cannot know — resolving them + // against process.cwd() would open an unrelated file with the + // same name. Refuse instead of guessing. + if (isStaticPatchMode) { + return Response.json( + { error: "Open in app is unavailable for a static patch review" }, + { status: 400 }, + ); + } return handleOpenIn(req, { resolveRoot: resolveOpenInRoot }); } @@ -2440,6 +2471,7 @@ export async function startReviewServer( gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, diffType: currentDiffType, diffOptions: workspace.diffOptions, hideWhitespace: currentHideWhitespace, @@ -2600,6 +2632,7 @@ export async function startReviewServer( gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, diffType: currentDiffType, // Echo the base the server actually used. resolveBaseBranch // trusts the caller verbatim; this echo lets the client @@ -2665,6 +2698,7 @@ export async function startReviewServer( gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, prDiffScope: currentPRDiffScope, ...(layerPatchIncomplete && { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable }), ...(currentError && { error: currentError }), @@ -2722,6 +2756,7 @@ export async function startReviewServer( gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, prDiffScope: currentPRDiffScope, ...(layerPatchIncomplete && { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable }), ...((currentError ?? upgradeError) && { error: currentError ?? upgradeError }), @@ -2770,6 +2805,7 @@ export async function startReviewServer( gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, prDiffScope: currentPRDiffScope, semanticDiff: await getSemanticDiffAdvert(), callFlow: await getCallFlowAdvert(), @@ -2899,6 +2935,7 @@ export async function startReviewServer( gitRef: currentGitRef, snapshotId: currentSnapshotId(), approvalNotesSupported, + ...sourceKindAdvert, prMetadata: pr.metadata, // The new PR's checkout (null while warming) so Open-in re-roots // immediately on switch instead of waiting for the 5s probe. @@ -3016,6 +3053,14 @@ export async function startReviewServer( // API: Get file content for expandable diff context if (url.pathname === "/api/file-content" && req.method === "GET") { + // No working tree behind a static patch: the patch IS the whole + // content of the session, so there is nothing to expand into. + if (isStaticPatchMode) { + return Response.json( + { error: "File content is unavailable for a static patch review" }, + { status: 400 }, + ); + } const filePath = url.searchParams.get("path"); if (!filePath) { return Response.json({ error: "Missing path" }, { status: 400 }); @@ -3230,6 +3275,12 @@ export async function startReviewServer( // API: Stage / unstage a file (disabled when VCS doesn't support it) if (url.pathname === "/api/git-add" && req.method === "POST") { + if (isStaticPatchMode) { + return Response.json( + { error: "Staging is unavailable for a static patch review" }, + { status: 400 }, + ); + } try { const body = (await req.json()) as { filePath?: unknown; undo?: boolean }; if (typeof body.filePath !== "string" || !body.filePath) { diff --git a/packages/shared/review-core.ts b/packages/shared/review-core.ts index fc2cdacc7..7b22a4c94 100644 --- a/packages/shared/review-core.ts +++ b/packages/shared/review-core.ts @@ -2476,3 +2476,24 @@ export function isBinaryPatchFile(patch: string, filePath: string): boolean { } return false; } + +/** + * The `static-patch` diff type: the session's content is caller-supplied + * unified-diff bytes (`plannotator review --patch-file`), not something a VCS + * computed. Nothing in the session may read the working tree. + */ +export const STATIC_PATCH_DIFF_TYPE = "static-patch"; + +/** + * Where the session's diff came from, advertised on every diff payload + * (`/api/diff` and the switch/PR endpoints) beside `approvalNotesSupported`. + * ABSENT reads as `"vcs"`, so an old server is unchanged and an old client + * ignoring the field behaves exactly as it always has. + * + * `"patch"` means static-patch mode: there is no repository, no working tree + * and no VCS behind the diff, so every affordance that would touch one + * (staging, hunk-context expansion, open-in-app, code navigation, diff-type / + * base switching, commit history, baseline freshness) is unavailable and the + * corresponding endpoints answer 400. + */ +export type ReviewSourceKind = "vcs" | "patch"; diff --git a/packages/shared/types.ts b/packages/shared/types.ts index a513ff973..1d890e9ff 100644 --- a/packages/shared/types.ts +++ b/packages/shared/types.ts @@ -13,6 +13,7 @@ export type { RepositoryContext, SinceBaseSectionEntry, SinceBaseSections, + ReviewSourceKind, } from "./review-core"; export type { From b19355c723a2e3a7316917e7f34f6861b54f5b8c Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 00:21:29 -0700 Subject: [PATCH 5/9] feat(review-editor): render static-patch sessions honestly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reads the `sourceKind` advert and turns off every affordance that assumes a repository: - `canUseLiveWorkspaceActions` is false, which already gates open-in-app, code navigation, token hover cards and editor annotations. - Hunk-context expansion is skipped rather than firing `/api/file-content` requests the server now 400s (`contextExpansionAvailable` on the review state, threaded to both diff surfaces). - Edit Mode is off: it reads and writes the file on disk. - The freshness probe is off: the patch bytes cannot go stale. - The header names the patch ("sample.diff · Patch") instead of falling through to a bare "Review" beside an unrelated repo. - The empty state says "The patch contains no changes." — or "could not be parsed as a unified diff" when the bytes carry no diff headers — instead of "No uncommitted changes to review." Staging, the diff-type/base pickers, the Git status and Commits panels and the fetch-base banner already self-hide once `diffType` is `static-patch` and `gitContext` is absent. --- packages/review-editor/App.tsx | 58 +++++++++++++++++-- .../components/AllFilesCodeView.tsx | 9 ++- .../review-editor/components/DiffViewer.tsx | 9 ++- .../review-editor/dock/ReviewStateContext.tsx | 4 ++ .../dock/panels/ReviewAllFilesDiffPanel.tsx | 1 + .../dock/panels/ReviewDiffPanel.tsx | 1 + 6 files changed, 75 insertions(+), 7 deletions(-) diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index a343e8c07..af1684a0a 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -151,7 +151,7 @@ import { } from './dock/reviewPanelTypes'; import type { DiffFile, AnnotationScrollTarget } from './types'; import { annotationMatchesPrScope, proseAnnotationMatchesPr } from './utils/annotationScope'; -import type { DiffOption, WorktreeInfo, GitContext, SinceBaseSections, CommitDiffInfo } from '@plannotator/shared/types'; +import type { DiffOption, WorktreeInfo, GitContext, SinceBaseSections, CommitDiffInfo, ReviewSourceKind } from '@plannotator/shared/types'; import { SectionsPanel } from './components/SectionsPanel'; import { CommitsPanel } from './components/CommitsPanel'; import { useCommitsView } from './hooks/useCommitsView'; @@ -651,6 +651,12 @@ const ReviewApp: React.FC = () => { // that never sends the field renders no approve-carrying items (PR3 // behavior); read off every diff payload that carries it. const [approvalNotesSupported, setApprovalNotesSupported] = useState(false); + // Session-constant capability advert from `/api/diff`: 'patch' means the + // diff is caller-supplied bytes (`plannotator review --patch-file`) with no + // repository behind it. ABSENT reads as 'vcs', so an old server keeps every + // affordance exactly as before. + const [sourceKind, setSourceKind] = useState('vcs'); + const isStaticPatch = sourceKind === 'patch'; const [repoInfo, setRepoInfo] = useState<{ display: string; branch?: string } | null>(null); useEffect(() => { @@ -1498,7 +1504,11 @@ const ReviewApp: React.FC = () => { snapshotId, }; }, [activeDiffBase, diffData?.gitRef, committedBase, snapshotId]); - const canUseLiveWorkspaceActions = !activeDiffBase.startsWith('gitbutler:stack:') && + // A static patch has no working tree at all, so everything that reads one — + // open-in-app, code navigation, token hover cards, editor annotations — + // is off for the same reason a committed GitButler layer turns them off. + const canUseLiveWorkspaceActions = !isStaticPatch && + !activeDiffBase.startsWith('gitbutler:stack:') && !activeDiffBase.startsWith('gitbutler:branch:'); const visibleEditorAnnotations = useMemo( () => canUseLiveWorkspaceActions ? editorAnnotations : [], @@ -2025,6 +2035,7 @@ const ReviewApp: React.FC = () => { agentCwd?: string | null; sharingEnabled?: boolean; approvalNotesSupported?: boolean; + sourceKind?: ReviewSourceKind; repoInfo?: { display: string; branch?: string }; prMetadata?: PRMetadata; prStackInfo?: PRStackInfo | null; @@ -2085,6 +2096,9 @@ const ReviewApp: React.FC = () => { if (data.agentCwd !== undefined) setAgentCwd(data.agentCwd); if (data.sharingEnabled !== undefined) setSharingEnabled(data.sharingEnabled); setApprovalNotesSupported(readApprovalNotesAdvert(data.approvalNotesSupported)); + // Session-constant: a static patch session has no diff-type switch to + // re-advertise it on, so `/api/diff` is the only place it can arrive. + setSourceKind(data.sourceKind === 'patch' ? 'patch' : 'vcs'); if (data.repoInfo) setRepoInfo(data.repoInfo); updatePRSession({ ...(data.prMetadata && { prMetadata: data.prMetadata }), @@ -3203,7 +3217,9 @@ const ReviewApp: React.FC = () => { // user refreshes when THEY are ready — never automatically (annotations are // line-anchored; rug-pulling the diff under them is worse than staleness). const diffFreshness = useDiffFreshness({ - enabled: !!origin, + // Static patch: the bytes are the session. Nothing can go stale, so the + // probe would only poll an endpoint with no fingerprint to compare. + enabled: !!origin && !isStaticPatch, resetKey: diffData?.rawPatch ?? '', snapshotId, onAgentCwd: setAgentCwd, @@ -3547,6 +3563,7 @@ const ReviewApp: React.FC = () => { prDiffScope, agentCwd, canUseLiveWorkspaceActions, + contextExpansionAvailable: !isStaticPatch, allAnnotations, externalAnnotations, selectedAnnotationId, @@ -3557,7 +3574,8 @@ const ReviewApp: React.FC = () => { onAddCallFlowAnnotation: handleAddCallFlowAnnotation, onAddAnnotation: handleAddAnnotation, onAddAnnotationForFile: handleAddAnnotationForFile, - editSuggestionsEnabled, + // Edit Mode reads and writes the file on disk; a static patch has none. + editSuggestionsEnabled: editSuggestionsEnabled && !isStaticPatch, onAddSuggestionsForFile: handleAddSuggestionsForFile, onAddEditorCommentForFile: handleAddEditorCommentForFile, onAddFileComment: handleAddFileComment, @@ -3656,7 +3674,7 @@ const ReviewApp: React.FC = () => { }), [ files, diffData?.rawPatch, activeFileIndex, guideOpen, effectiveDiffStyle, handleDiffStyleChange, isCompactTouchLayout, diffOverflow, diffIndicators, diffLineDiffType, diffShowLineNumbers, diffShowBackground, - diffExpandUnchanged, diffFontFamily, diffFontSize, activeDiffBase, committedBase, feedbackDiffContext, prReviewScopeLabel, prDiffScope, agentCwd, canUseLiveWorkspaceActions, + diffExpandUnchanged, diffFontFamily, diffFontSize, activeDiffBase, committedBase, feedbackDiffContext, prReviewScopeLabel, prDiffScope, agentCwd, canUseLiveWorkspaceActions, isStaticPatch, allAnnotations, externalAnnotations, visibleDescriptionAnnotations, selectedDescriptionAnnotationId, handleAddDescriptionAnnotation, handleSelectDescriptionAnnotation, handleDeleteDescriptionAnnotation, handleAskAIForDescription, @@ -4388,6 +4406,16 @@ const ReviewApp: React.FC = () => { const compactActionBusy = isSendingFeedback || isApproving || isExiting || isPlatformActioning; const showsLocalVsRemoteEmptyState = activeDiffBase === 'local-vs-remote'; + // Static patch header identity: the patch path the caller passed (or + // "stdin patch"), which the server echoes as gitRef. Never a branch or repo. + const staticPatchSource = (diffData?.gitRef ?? '').trim(); + const staticPatchLabel = staticPatchSource + ? staticPatchSource.split(/[\\/]/).pop() || staticPatchSource + : 'Patch'; + // Distinguishes "valid patch, nothing in it" from "these bytes are not a + // unified diff" for the empty state. The server already refuses a + // whitespace-only patch at startup, so anything reaching here has content. + const patchHasDiffHeaders = /^(diff --git |--- |\+\+\+ |@@ |Index: )/m.test(diffData?.rawPatch ?? ''); const compactReviewActions: CompactReviewAction[] = !isCompactTouchLayout ? [] : !origin @@ -4598,6 +4626,23 @@ const ReviewApp: React.FC = () => { {repoInfo.display} + ) : isStaticPatch ? ( + // Honest label for a repo-less session: name the patch that IS + // the review, never a branch or repo this process happens to + // sit in. +
+ + {staticPatchLabel} + + Patch +
) : ( { {activeDiffBase === 'branch' && `No changes vs ${selectedBase || gitContext?.defaultBranch || 'main'}${activeWorktreePath ? ' in this worktree' : ''}.`} {activeDiffBase === 'merge-base' && `No changes vs ${selectedBase || gitContext?.defaultBranch || 'main'}${activeWorktreePath ? ' in this worktree' : ''}.`} {activeDiffBase === 'all' && `No tracked files${activeWorktreePath ? ' in this worktree' : ' in this repository'}.`} + {isStaticPatch && (patchHasDiffHeaders + ? 'The patch contains no changes.' + : 'The patch could not be parsed as a unified diff.')}

)} diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index 1d6fd28a6..0218a6202 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -185,6 +185,10 @@ export interface AllFilesCodeViewProps { pendingSelection: SelectedLineRange | null; reviewBase?: string; reviewSnapshotId?: string; + /** False when there is no source behind the diff to expand into (static + * patch review): the augmentation stage completes as a no-op instead of + * firing a /api/file-content request the server answers 400. */ + contextExpansionAvailable?: boolean; /** Compact coarse-pointer shell. Adjusts custom-header chrome and Pierre's * matching virtualization metric without changing desktop geometry. */ compactTouchLayout?: boolean; @@ -561,6 +565,7 @@ export const AllFilesCodeView: React.FC = ({ pendingSelection, reviewBase, reviewSnapshotId, + contextExpansionAvailable = true, compactTouchLayout, onLineSelection, onAddAnnotationForFile, @@ -1233,6 +1238,8 @@ export const AllFilesCodeView: React.FC = ({ reviewBaseRef.current = reviewBase; const reviewSnapshotIdRef = useRef(reviewSnapshotId); reviewSnapshotIdRef.current = reviewSnapshotId; + const contextExpansionAvailableRef = useRef(contextExpansionAvailable); + contextExpansionAvailableRef.current = contextExpansionAvailable; const itemIdToFileRef = useRef(itemIdToFile); itemIdToFileRef.current = itemIdToFile; const fileSetKeyRef = useRef(fileSetKey); @@ -1348,7 +1355,7 @@ export const AllFilesCodeView: React.FC = ({ // Read-only hosts have no review server: leave the raw-patch context in // place and mark the item done so it never re-fires (no dead requests, // no console noise from a CSP that blocks connect-src). - if (readOnlyRef.current) { + if (readOnlyRef.current || !contextExpansionAvailableRef.current) { augmentState.set(itemId, { status: 'done', controller, generation }); return; } diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index ab3eef688..63ade6842 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -162,6 +162,10 @@ interface DiffViewerProps { status?: import('../types').DiffFileStatus; /** Base branch override used for file-content lookups (branch / merge-base modes only). */ reviewBase?: string; + /** False when there is no source behind the diff to expand into (static + * patch review): skip the /api/file-content fetch entirely rather than + * firing a request the server answers 400. Absent means available. */ + contextExpansionAvailable?: boolean; /** Opaque diff snapshot used to reject mutable file-content lookups from another view. */ reviewSnapshotId?: string; /** Current PR url + diff scope — used to namespace file-comment drafts so they don't leak across in-place PR switches. */ @@ -240,6 +244,7 @@ export const DiffViewer: React.FC = ({ status, reviewBase, reviewSnapshotId, + contextExpansionAvailable = true, prUrl, prDiffScope, isFocused = false, @@ -386,6 +391,8 @@ export const DiffViewer: React.FC = ({ useEffect(() => { const controller = new AbortController(); setFileContents(null); + // Nothing to expand into: the patch is the whole content of the session. + if (!contextExpansionAvailable) return; const params = new URLSearchParams({ path: filePath }); if (oldPath) params.set('oldPath', oldPath); if (reviewBase) params.set('base', reviewBase); @@ -399,7 +406,7 @@ export const DiffViewer: React.FC = ({ }) .catch(() => {}); // Silent fallback — no expansion in demo mode return () => controller.abort(); - }, [filePath, oldPath, reviewBase, reviewSnapshotId]); + }, [filePath, oldPath, reviewBase, reviewSnapshotId, contextExpansionAvailable]); // Re-parse the patch with full file contents so hunk indices are computed // against the complete file (isPartial: false), enabling expansion. diff --git a/packages/review-editor/dock/ReviewStateContext.tsx b/packages/review-editor/dock/ReviewStateContext.tsx index 7d5dbc1c8..43b8d6e33 100644 --- a/packages/review-editor/dock/ReviewStateContext.tsx +++ b/packages/review-editor/dock/ReviewStateContext.tsx @@ -65,6 +65,10 @@ export interface ReviewState { agentCwd?: string | null; /** Whether live-working-tree actions match the snapshot currently shown. */ canUseLiveWorkspaceActions?: boolean; + /** False when hunk-context expansion has no source to expand from — a static + * patch review has no working tree, so `/api/file-content` answers 400 and + * the diff views must not ask. Absent means available (every VCS session). */ + contextExpansionAvailable?: boolean; // Annotations allAnnotations: CodeAnnotation[]; diff --git a/packages/review-editor/dock/panels/ReviewAllFilesDiffPanel.tsx b/packages/review-editor/dock/panels/ReviewAllFilesDiffPanel.tsx index 04ca00f6a..df1f61ffd 100644 --- a/packages/review-editor/dock/panels/ReviewAllFilesDiffPanel.tsx +++ b/packages/review-editor/dock/panels/ReviewAllFilesDiffPanel.tsx @@ -38,6 +38,7 @@ export const ReviewAllFilesDiffPanel: React.FC = () => { pendingSelection={state.pendingSelection} reviewBase={state.reviewBase} reviewSnapshotId={state.feedbackDiffContext?.snapshotId} + contextExpansionAvailable={state.contextExpansionAvailable} compactTouchLayout={state.isCompactTouchLayout} onLineSelection={state.onLineSelection} onAddAnnotationForFile={state.onAddAnnotationForFile} diff --git a/packages/review-editor/dock/panels/ReviewDiffPanel.tsx b/packages/review-editor/dock/panels/ReviewDiffPanel.tsx index ed6d8f71d..4667359a5 100644 --- a/packages/review-editor/dock/panels/ReviewDiffPanel.tsx +++ b/packages/review-editor/dock/panels/ReviewDiffPanel.tsx @@ -73,6 +73,7 @@ export const ReviewDiffPanel: React.FC = (props) => { status={file.status} reviewBase={state.reviewBase} reviewSnapshotId={state.feedbackDiffContext?.snapshotId} + contextExpansionAvailable={state.contextExpansionAvailable} prUrl={state.prMetadata?.url} prDiffScope={state.prDiffScope} isFocused={isFocusedFile} From 1cd044ecf9c585344826094f77cc5b0e91c9c43a Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 00:21:36 -0700 Subject: [PATCH 6/9] fix(review): treat --no-local as a --patch-file conflict; reject stdin on Pi MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--no-local` is a PR-review selector exactly like `--local`, and `useLocal` defaults to true, so only flag PRESENCE can decide the conflict — it was silently ignored next to `--patch-file`. The Pi host has no stdin of its own (it is reached through an extension event), so `--patch-file -` read a file literally named "-"; it now refuses with a clear message. Drops the unreachable `if (options.prUrl) throw` in the same branch: the enclosing `else if` already means prUrl is falsy. --- apps/pi-extension/plannotator-browser.ts | 9 +++++++-- packages/shared/review-args.test.ts | 15 +++++++++++++++ packages/shared/review-args.ts | 8 +++++--- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index d60b1126a..1d6236f35 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -597,8 +597,13 @@ async function createCodeReviewBrowserSession( // diffType "static-patch" — identical to the direct CLI's --patch-file // path. No refresh: there is no live tree to recompute against; the // initial patch is the session's whole content. - if (options.prUrl) { - throw new Error("--patch-file cannot be combined with a PR/MR URL"); + // `-` means stdin, which only the direct CLI has: this host reaches the + // review through an extension event with no stdin of its own, so refuse + // rather than reading a file literally named "-". + if (options.patchFile === "-") { + throw new Error( + "--patch-file - (stdin) is not available here; pass a file path instead", + ); } rawPatch = readFileSync(resolve(options.cwd ?? ctx.cwd, options.patchFile), "utf-8"); if (!rawPatch.trim()) { diff --git a/packages/shared/review-args.test.ts b/packages/shared/review-args.test.ts index cbcd22e11..12fb8ac0f 100644 --- a/packages/shared/review-args.test.ts +++ b/packages/shared/review-args.test.ts @@ -190,12 +190,27 @@ describe("parseReviewArgs", () => { const withBase = ["--patch-file", "reading.diff", "--base", "main"]; const withDiffType = ["--patch-file", "reading.diff", "--diff-type", "staged"]; const withProvider = ["--patch-file", "reading.diff", "--git"]; + const withLocal = ["--patch-file", "reading.diff", "--local"]; + // --no-local is a PR-review selector exactly like --local, and useLocal + // defaults to true — so presence, not value, decides the conflict. + const withNoLocal = ["--patch-file", "reading.diff", "--no-local"]; // when / then expect(parseReviewArgs(withPrUrl).errors).toContain("--patch-file cannot be combined with a PR/MR URL"); expect(parseReviewArgs(withBase).errors).toContain("--patch-file cannot be combined with --base"); expect(parseReviewArgs(withDiffType).errors).toContain("--patch-file cannot be combined with --diff-type"); expect(parseReviewArgs(withProvider).errors).toContain("--patch-file cannot be combined with --git/--gitbutler"); + expect(parseReviewArgs(withLocal).errors).toContain("--patch-file cannot be combined with --local/--no-local"); + expect(parseReviewArgs(withNoLocal).errors).toContain("--patch-file cannot be combined with --local/--no-local"); + }); + + test("leaves --no-local alone without a patch file", () => { + // given / when + const result = parseReviewArgs(["https://github.com/acme/repo/pull/12", "--no-local"]); + + // then + expect(result.errors).toEqual([]); + expect(result.useLocal).toBe(false); }); test("rejects a missing or duplicate patch file", () => { diff --git a/packages/shared/review-args.ts b/packages/shared/review-args.ts index c91c3d3ed..f2e6116e2 100644 --- a/packages/shared/review-args.ts +++ b/packages/shared/review-args.ts @@ -55,8 +55,9 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { const positional: string[] = []; let patchFile: string | undefined; - // --local conflicts with --patch-file only when the user typed it: its - // value defaults to true, so check the flag's presence, not the value. + // --local / --no-local conflict with --patch-file only when the user typed + // one: useLocal defaults to true, so check the flag's presence, not the + // value. Both spellings are PR-review selectors, so both are usage errors. let localFlagSeen = false; // Index-based so value-taking flags consume their value token before the @@ -91,6 +92,7 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { break; case "--no-local": useLocal = false; + localFlagSeen = true; break; case "--base": { const value = tokens[i + 1]; @@ -160,7 +162,7 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { if (base) errors.push("--patch-file cannot be combined with --base"); if (diffType) errors.push("--patch-file cannot be combined with --diff-type"); if (vcsType) errors.push("--patch-file cannot be combined with --git/--gitbutler"); - if (localFlagSeen) errors.push("--patch-file cannot be combined with --local"); + if (localFlagSeen) errors.push("--patch-file cannot be combined with --local/--no-local"); } return { prUrl: target && isReviewUrl(target) ? target : undefined, From 9b48df04ba26dbf6a1194001bb3372482a29327d Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 00:21:36 -0700 Subject: [PATCH 7/9] test(review): pin static-patch adverts and endpoint refusals in both runtimes Boots a Bun and a Pi review server on a patch and asserts the payload carries `sourceKind`/`diffType` and no `repoInfo`/`gitContext`, and that git-add, file-content and open-in answer 400. A VCS session is checked alongside so the guards stay keyed on patch mode and the advert stays absent for an ordinary review. --- packages/server/review-static-patch.test.ts | 186 ++++++++++++++++++++ 1 file changed, 186 insertions(+) create mode 100644 packages/server/review-static-patch.test.ts diff --git a/packages/server/review-static-patch.test.ts b/packages/server/review-static-patch.test.ts new file mode 100644 index 000000000..1ad7286f9 --- /dev/null +++ b/packages/server/review-static-patch.test.ts @@ -0,0 +1,186 @@ +/** + * `plannotator review --patch-file` — server half, dual-runtime (Bun + Pi). + * + * A static patch review has no repository, no working tree and no VCS behind + * it, so the server has to say so and act like it. Three regressions: + * + * 1. The client cannot tell patch mode from a repo-less VCS session unless + * the server advertises it. Without `sourceKind: "patch"` the browser + * falls back to its `uncommitted` default and renders Git Add buttons, + * hunk-expansion arrows and "No uncommitted changes" — all lies. + * 2. `diffType` must ride the payload in patch mode. It used to be withheld + * (it is gated on local access), which is what left the client on its + * `uncommitted` default in the first place. + * 3. The endpoints that read a working tree must answer 400 rather than + * resolving the patch's paths against whatever cwd the server runs in — + * `/api/git-add`, `/api/file-content` and `/api/open-in` would otherwise + * stage, read, or OPEN an unrelated same-named file. + * + * A VCS session is checked alongside each case: the advert is absent there + * (absent reads as "vcs"), so nothing about an ordinary review changed. + */ +import { afterEach, describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { createServer } from "node:net"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { startReviewServer as startBunReviewServer } from "./review"; +import { startReviewServer as startPiReviewServer } from "../../apps/pi-extension/server"; +import { getVcsContext } from "./vcs"; + +const MINIMAL_HTML = "Plannotator"; +const PATCH = "diff --git a/src/parse.ts b/src/parse.ts\n@@ -1 +1 @@\n-a\n+b\n"; + +const tempDirs: string[] = []; +const savedEnv: Record = {}; + +function saveEnv(key: string) { + if (!(key in savedEnv)) savedEnv[key] = process.env[key]; +} + +function makeTempDir(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +function useTempDataDir(): void { + saveEnv("PLANNOTATOR_DATA_DIR"); + process.env.PLANNOTATOR_DATA_DIR = makeTempDir("plannotator-static-patch-"); +} + +async function reservePiPort() { + const server = createServer(); + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", () => resolve()); + }); + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + await new Promise((resolve) => server.close(() => resolve())); + saveEnv("PLANNOTATOR_PORT"); + process.env.PLANNOTATOR_PORT = String(port); +} + +function git(cwd: string, args: string[]): void { + const result = spawnSync("git", args, { cwd, encoding: "utf-8" }); + if (result.status !== 0) { + throw new Error(result.stderr || `git ${args.join(" ")} failed`); + } +} + +function initRepo(): string { + const repoDir = makeTempDir("plannotator-static-patch-repo-"); + git(repoDir, ["init", "-q"]); + git(repoDir, ["branch", "-M", "main"]); + git(repoDir, ["config", "user.email", "test@example.com"]); + git(repoDir, ["config", "user.name", "Test"]); + writeFileSync(join(repoDir, "README.md"), "# repo\n"); + git(repoDir, ["add", "README.md"]); + git(repoDir, ["commit", "-q", "-m", "initial"]); + return repoDir; +} + +afterEach(() => { + for (const [key, value] of Object.entries(savedEnv)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + delete savedEnv[key]; + } + for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +for (const [runtime, startServer] of [ + ["Bun", startBunReviewServer], + ["Pi", startPiReviewServer], +] as const) { + describe(`static patch review server (${runtime})`, () => { + test("advertises sourceKind + diffType and 400s every working-tree endpoint", async () => { + useTempDataDir(); + if (runtime === "Pi") await reservePiPort(); + const server = await startServer({ + rawPatch: PATCH, + gitRef: "reading.diff", + diffType: "static-patch", + origin: runtime === "Pi" ? "pi" : "claude-code", + htmlContent: MINIMAL_HTML, + }); + try { + const diff = (await fetch(`${server.url}/api/diff`).then((r) => r.json())) as { + sourceKind?: string; + diffType?: string; + gitContext?: unknown; + repoInfo?: unknown; + rawPatch?: string; + }; + expect(diff.sourceKind).toBe("patch"); + expect(diff.diffType).toBe("static-patch"); + expect(diff.gitContext).toBeUndefined(); + // Whatever repo the server process sits in is not the patch's origin: + // advertising it would put an unrelated repo and branch in the header. + expect(diff.repoInfo).toBeUndefined(); + expect(diff.rawPatch).toBe(PATCH); + + const gitAdd = await fetch(`${server.url}/api/git-add`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath: "src/parse.ts" }), + }); + expect(gitAdd.status).toBe(400); + + const fileContent = await fetch( + `${server.url}/api/file-content?path=src/parse.ts`, + ); + expect(fileContent.status).toBe(400); + + // The path in the patch may well exist under the server's cwd by + // coincidence; opening it would show a file from an unrelated tree. + const openIn = await fetch(`${server.url}/api/open-in`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath: "src/parse.ts", base: null, appId: "reveal" }), + }); + expect(openIn.status).toBe(400); + + const apps = (await fetch(`${server.url}/api/open-in/apps`).then((r) => + r.json(), + )) as { available?: boolean; apps?: unknown[] }; + expect(apps.available).toBe(false); + expect(apps.apps).toEqual([]); + } finally { + server.stop(); + } + }); + + test("an ordinary VCS review carries no advert and keeps file access", async () => { + useTempDataDir(); + const repoDir = initRepo(); + const gitContext = await getVcsContext(repoDir, "git"); + if (runtime === "Pi") await reservePiPort(); + const server = await startServer({ + rawPatch: PATCH, + gitRef: "Working tree", + diffType: "uncommitted", + gitContext, + origin: runtime === "Pi" ? "pi" : "claude-code", + htmlContent: MINIMAL_HTML, + }); + try { + const diff = (await fetch(`${server.url}/api/diff`).then((r) => r.json())) as { + sourceKind?: string; + }; + // Absent, not "vcs": the advert is add-only, so an old client that + // never reads the field sees a byte-identical payload. + expect(diff.sourceKind).toBeUndefined(); + + // The guards above must be keyed on patch mode, not merely present: + // a real repo still resolves file content (README.md is committed). + const fileContent = await fetch(`${server.url}/api/file-content?path=README.md`); + expect(fileContent.status).toBe(200); + } finally { + server.stop(); + } + }); + }); +} From 1d591984700be1d5e35fcff88109be29c25ed714 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 00:21:41 -0700 Subject: [PATCH 8/9] docs(review): document --patch-file and what it switches off Adds the usage section to the code-review reference and keeps the freshness-guarded plannotator skill in step, including the corrected --local/--no-local conflict wording. --- .../src/content/docs/commands/code-review.md | 22 +++++++++++++++++++ apps/skills/core/plannotator/SKILL.md | 2 +- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/apps/marketing/src/content/docs/commands/code-review.md b/apps/marketing/src/content/docs/commands/code-review.md index c95b8e614..7a49122f3 100644 --- a/apps/marketing/src/content/docs/commands/code-review.md +++ b/apps/marketing/src/content/docs/commands/code-review.md @@ -26,6 +26,28 @@ PR review uses the `gh` CLI for authentication, so private repos work automatica GitLab merge request URLs are also supported when the `glab` CLI is installed and authenticated. +**Review a patch file, with no repository:** + +``` +plannotator review --patch-file reading.diff +curl -s https://example.com/change.diff | plannotator review --patch-file - +``` + +`--patch-file` opens the review UI against a caller-supplied unified diff — a +patch from an email, a paste, a CI artifact, or a remote agent — with no Git +repo, no worktree and no VCS detection. Use `-` to read the patch from stdin. + +The patch is the whole session, so everything that would read a working tree is +switched off: no staging, no hunk-context expansion, no "Open in editor" or code +navigation, no diff-type or base switching, no Git status or commit panels, and +no diff-staleness refresh. Annotating, Ask AI, Guided Review and submitting +feedback all work as usual, and the header names the patch instead of a branch. + +Because it replaces VCS detection entirely, `--patch-file` cannot be combined +with a PR/MR URL, `--base`, `--diff-type`, `--git`/`--gitbutler`, or +`--local`/`--no-local`; each combination is a startup error naming the conflict, +as is an empty or unreadable patch. + ## How it works **Local review:** diff --git a/apps/skills/core/plannotator/SKILL.md b/apps/skills/core/plannotator/SKILL.md index e6ae5faa7..5190ccd77 100644 --- a/apps/skills/core/plannotator/SKILL.md +++ b/apps/skills/core/plannotator/SKILL.md @@ -54,7 +54,7 @@ Classify the outcome only by `decision`, never by `message` text. Notes on an `a - The default diff is "everything a PR would show now": merge-base of the trunk vs the working tree plus untracked files. `--base ` opens the session against a different compare target (branch, `origin/`, tag, or commit) and `--diff-type ` opens it in a different mode (`since-base`, `merge-base`, `branch`, `uncommitted`, `staged`, `unstaged`, `last-commit`, `local-vs-remote`, `all`). Both are **session-only**: the reviewer can change either in the UI, and neither writes the user's saved defaults. - **Reviewing one layer of a stacked branch? Pass `--base `** — `plannotator review --base feature/part-1` shows only what this layer adds, instead of everything since `main`. - Both flags are git-only: they error on jj, GitButler, Perforce, multi-repo workspace reviews, and PR URLs (a PR's base comes from the pull request). A `--base` ref that does not resolve is a startup error naming near-match branches, never a silently wrong diff. -- `--patch-file ` reviews a static caller-supplied unified diff with no repository at all (use `-` to read it from stdin): the session serves the patch as-is with no file-system affordances that need a worktree. It cannot be combined with a PR/MR URL, `--base`, `--diff-type`, `--git`/`--gitbutler`, or `--local`. +- `--patch-file ` reviews a static caller-supplied unified diff with no repository at all (use `-` to read it from stdin): the session serves the patch as-is with no file-system affordances that need a worktree. It cannot be combined with a PR/MR URL, `--base`, `--diff-type`, `--git`/`--gitbutler`, or `--local`/`--no-local`. Every working-tree affordance is off in that session (staging, hunk-context expansion, open-in-editor, code navigation, diff-type/base switching), and the endpoints behind them answer 400. - PR review (`plannotator review https://github.com/owner/repo/pull/123`, GitLab MR URLs too) needs an authenticated `gh` or `glab` CLI. `--local` (the default) builds a local checkout of the PR head in the background for full file access; `--no-local` skips it and reviews the platform diff only. - `--tailscale` publishes the loopback session over the user's tailnet via `tailscale serve` (HTTPS, never public) and prints the URL with a QR code. A publish failure exits nonzero instead of leaving the server hanging. From 58896f4b0d997628967ceb438a21621eb0226391 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 00:21:41 -0700 Subject: [PATCH 9/9] chore(guides-show): repin the viewer manifest AllFilesCodeView gained the context-expansion seam, and the portable viewer bundles it. --- packages/core/guide-viewer-manifest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/guide-viewer-manifest.ts b/packages/core/guide-viewer-manifest.ts index 4445ebc92..3cc89a636 100644 --- a/packages/core/guide-viewer-manifest.ts +++ b/packages/core/guide-viewer-manifest.ts @@ -5,9 +5,9 @@ import type { GuideViewerAssets } from "./guide-format"; export const GUIDE_VIEWER_MANIFEST: Omit = { - js: "viewer.KTNT-M2b.js", + js: "viewer.BnD4aEPi.js", css: "viewer.NkTIi4sR.css", - jsIntegrity: "sha384-UGxkmDjeL0LMAKSAnleY0ewq4d4vtotFlHvHYWaK6UGIWmV30DyT5wSKQEz2NHdV", + jsIntegrity: "sha384-LfSwy9OFdH7mo2/oM1ccvWbm2oqGV1CdiACxZvUgvbpw5UeUDnPJ6Gxf+FBTswDZ", cssIntegrity: "sha384-2tINtoWgdpcbwUZudhmxJiiW7Tu+29vXj6P12fLLkRtc2sWERGoHl71K35L+UR9f", langs: { "astro": "chunks/astro.Ts5EKq2l.js",