From fe4329353d8bbee9a8ba7d1f7b3b4ba97b275237 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Fri, 4 Sep 2026 15:23:15 -0400 Subject: [PATCH 1/6] feat(extensions): carry delegated review metadata --- .changeset/calm-reviews-describe.md | 5 + docs/extension-architecture.md | 8 +- docs/extensions.md | 33 ++- src/app/delegatedReview.test.ts | 59 +++++ src/app/delegatedReview.ts | 30 +++ src/app/startup.test.ts | 91 ++++++++ src/app/startup.ts | 9 + src/core/bootstrap.ts | 4 +- src/extension-api/index.ts | 5 + src/extension-api/types.ts | 45 +++- src/extensions/cliCommandRuntime.test.ts | 114 ++++++++++ src/extensions/cliCommandRuntime.ts | 151 ++++++++++++- src/extensions/types.ts | 1 + src/ui/AppHost.review-metadata.test.tsx | 210 ++++++++++++++++++ src/ui/AppHost.tsx | 26 +++ .../content/docs/docs/extend/extension-api.md | 34 ++- 16 files changed, 810 insertions(+), 15 deletions(-) create mode 100644 .changeset/calm-reviews-describe.md create mode 100644 src/app/delegatedReview.test.ts create mode 100644 src/app/delegatedReview.ts create mode 100644 src/ui/AppHost.review-metadata.test.tsx diff --git a/.changeset/calm-reviews-describe.md b/.changeset/calm-reviews-describe.md new file mode 100644 index 000000000..129bf6e34 --- /dev/null +++ b/.changeset/calm-reviews-describe.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Let extension CLI commands attach validated provider-neutral review metadata when they delegate a patch into Hunk. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index e386a2ff4..78af21a02 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -67,8 +67,12 @@ the raw subtree and runs through leased process I/O. An exit result retires the registry before returning an exit plan; a one-time built-in delegation reparses through the ordinary planner. Delegated reviews reconcile the already loaded candidate/config prefix and hand the same registry to `AppBootstrap`, so factories -are not rerun merely for the handoff. Headless delegation retires before executing -the built-in plan. Terminal probing occurs only after the handler releases I/O. +are not rerun merely for the handoff. A delegated patch may attach a validated, bounded +provider-neutral review descriptor. Startup carries it beside the input on `AppBootstrap`; it does +not enter the changeset transform pipeline or `ReviewDocumentV1`. The host preserves it only while +the same file-backed patch identity reloads and clears it when a reload selects another input. +Headless delegation retires before executing the built-in plan. Terminal probing occurs only after +the handler releases I/O. ## Host-served runtime modules diff --git a/docs/extensions.md b/docs/extensions.md index f80dff034..3db67f617 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -280,8 +280,9 @@ new instances and run that shutdown/startup pair around the replacement. ### `hunk.apiVersion` -The API generation this Hunk speaks (currently `16`). Branch on it if you want -one file to support several Hunk versions. Version 16 adds pane-wide +The API generation this Hunk speaks (currently `17`). Branch on it if you want +one file to support several Hunk versions. Version 17 adds structured review metadata to delegated +patch commands; version 16 adds pane-wide `onActivate`; version 15 added `{ side, line }` to opted-in pane `currentLine` paint; version 14 added structured `rangeEndpoints` to two-revision VCS diff requests; version 13 added saved-note parent identities and @@ -312,7 +313,20 @@ hunk.registerCliCommand( } await ctx.stderr.write("Preparing review…\n"); - return { kind: "delegate", argv: ["diff", "--agent-context", "notes.json"] }; + return { + kind: "delegate", + argv: ["patch", "review.diff"], + review: { + kind: "change-request", + provider: "GitHub", + title: "Add structured review metadata", + url: "https://github.com/acme/project/pull/123", + id: "#123", + author: "octocat", + base: "main", + head: "review-metadata", + }, + }; }, ); ``` @@ -324,6 +338,19 @@ to access networks, processes, services, and files. Return `{ kind: "exit", code? }` with a status from 0 through 255, or delegate exactly once to a built-in Hunk command. +A delegated built-in `patch` command may include a provider-neutral `review` descriptor. Its +`kind` is `change-request`, `commit`, or `comparison`; each exact shape combines bounded display +strings with an optional credential-free HTTPS URL. Hunk rejects unknown fields, control +characters, invalid types, unsafe URLs, fields over their byte limits, and descriptors over 4 KiB, +then copies and freezes the accepted value. `provider` and change-request `id` allow 256 bytes; +`author`, `base`, `head`, and `revision` allow 512; `title` and `url` allow 2 KiB. Exit results and delegation to any built-in other than +`patch` cannot carry review metadata. An ordinary `hunk patch` has no descriptor. + +The descriptor describes the review source rather than its diff contents: it stays on the app +bootstrap and does not enter changeset transforms or `ReviewDocumentV1`. Refreshing the same +file-backed patch preserves it, including watch and manual refresh; an explicit reload to a +different patch path or input kind clears it. + Delegation cannot target another extension command or change extension bootstrap flags. Do not write stdout or read stdin before delegating; use stderr for progress. Reading stdin is an exit-only workflow because even a pending read can diff --git a/src/app/delegatedReview.test.ts b/src/app/delegatedReview.test.ts new file mode 100644 index 000000000..5d24260f1 --- /dev/null +++ b/src/app/delegatedReview.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, test } from "bun:test"; +import type { ExtensionReviewDescriptor } from "../extension-api/types"; +import { reviewDescriptorAfterReload } from "./delegatedReview"; + +const review: ExtensionReviewDescriptor = { + kind: "change-request", + provider: "GitHub", + title: "PR title", + id: "#123", +}; +const patch = (file?: string) => ({ kind: "patch" as const, file, options: {} }); + +describe("delegated review reload identity", () => { + test("preserves metadata while refreshing the same patch path", () => { + expect( + reviewDescriptorAfterReload( + patch("review.diff"), + "/tmp", + review, + patch("/tmp/review.diff"), + "/", + ), + ).toBe(review); + }); + + test("clears metadata for unrelated explicit and non-file reloads", () => { + expect( + reviewDescriptorAfterReload( + patch("/tmp/pr.diff"), + "/", + review, + patch("/tmp/other.diff"), + "/", + ), + ).toBeUndefined(); + expect( + reviewDescriptorAfterReload( + patch("/tmp/pr.diff"), + "/", + review, + { kind: "vcs", staged: false, options: {} }, + "/", + ), + ).toBeUndefined(); + expect(reviewDescriptorAfterReload(patch("-"), "/", review, patch("-"), "/")).toBeUndefined(); + }); + + test("does not invent metadata for ordinary patches", () => { + expect( + reviewDescriptorAfterReload( + patch("/tmp/pr.diff"), + "/", + undefined, + patch("/tmp/pr.diff"), + "/", + ), + ).toBeUndefined(); + }); +}); diff --git a/src/app/delegatedReview.ts b/src/app/delegatedReview.ts new file mode 100644 index 000000000..8311dd7ed --- /dev/null +++ b/src/app/delegatedReview.ts @@ -0,0 +1,30 @@ +import { resolve } from "node:path"; +import type { ExtensionReviewDescriptor } from "../extension-api/types"; +import { resolveCanonicalPath } from "../core/run/paths"; +import type { CliInput } from "../core/run/commandInputs"; + +/** Resolve the file identity already used by session reload bounds. */ +function patchFileIdentity(input: CliInput, cwd: string): string | undefined { + if (input.kind !== "patch" || !input.file || input.file === "-") return undefined; + return resolveCanonicalPath(resolve(cwd, input.file)); +} + +/** + * Preserve delegated review metadata only while reloading the same patch resource. + * + * The canonical patch path is the reload boundary's existing input identity: changes to the file + * refresh the same remote review, while a different or non-file input starts an unrelated review. + */ +export function reviewDescriptorAfterReload( + previousInput: CliInput, + previousCwd: string, + previousReview: ExtensionReviewDescriptor | undefined, + nextInput: CliInput, + nextCwd: string, +): ExtensionReviewDescriptor | undefined { + if (!previousReview) return undefined; + const previousIdentity = patchFileIdentity(previousInput, previousCwd); + return previousIdentity && previousIdentity === patchFileIdentity(nextInput, nextCwd) + ? previousReview + : undefined; +} diff --git a/src/app/startup.test.ts b/src/app/startup.test.ts index 2a346bcb5..df653c998 100644 --- a/src/app/startup.test.ts +++ b/src/app/startup.test.ts @@ -126,6 +126,97 @@ describe("startup planning", () => { expect(shutdowns).toBe(1); }); + test("carries validated review metadata only into a delegated patch bootstrap", async () => { + const invocation = { + kind: "extension-cli" as const, + commandName: "tools", + args: [], + extensionPaths: ["/tools.ts"], + extensionsEnabled: true, + }; + const patchInput: CliInput = { kind: "patch", file: "/tmp/review.diff", options: {} }; + const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "Add metadata", + url: "https://github.com/modem-dev/hunk/pull/123", + id: "#123", + author: "octocat", + base: "main", + head: "metadata", + }; + const extensions = createEmptyExtensionLoadResult(); + extensions.registry.extensions.push({ id: "tools", sourcePath: "/tools.ts", origin: "flag" }); + extensions.registry.cliCommands.push({ + extensionId: "tools", + command: { name: "tools", summary: "Tools" }, + handler: () => ({ kind: "delegate", argv: ["patch", "/tmp/review.diff"], review }), + }); + + const plan = await prepareStartupPlan(["bun", "hunk", "tools"], { + parseCliImpl: async (argv) => (argv.includes("patch") ? patchInput : invocation), + resolveRuntimeCliInputImpl: (input) => input, + resolveConfiguredCliInputImpl: (input) => + createTestConfigResolution(input, { + extensions: { enabled: true, paths: [], repoPaths: [], extensionConfigs: {} }, + }), + resolveExtensionCliBootstrapImpl: async ({ baseVcsCatalog }) => ({ + configured: { + extensions: { enabled: true, paths: [], repoPaths: [], extensionConfigs: {} }, + }, + extensions, + commands: resolveExtensionCliCommands(extensions.registry), + collisionIssues: [], + discoveryCatalog: baseVcsCatalog, + }), + loadStartupExtensionsImpl: async () => extensions, + loadAppBootstrapImpl: async (input) => createBootstrap(input), + usesPipedPatchInputImpl: () => false, + }); + + expect(plan.kind).toBe("app"); + if (plan.kind !== "app") throw new Error("Expected app plan."); + expect(plan.bootstrap.review).toEqual(review); + expect(Object.isFrozen(plan.bootstrap.review)).toBe(true); + }); + + test("rejects review metadata when delegation targets a non-patch built-in", async () => { + const invocation = { + kind: "extension-cli" as const, + commandName: "tools", + args: [], + extensionPaths: ["/tools.ts"], + extensionsEnabled: true, + }; + const extensions = createEmptyExtensionLoadResult(); + extensions.registry.extensions.push({ id: "tools", sourcePath: "/tools.ts", origin: "flag" }); + extensions.registry.cliCommands.push({ + extensionId: "tools", + command: { name: "tools", summary: "Tools" }, + handler: () => ({ + kind: "delegate", + argv: ["--version"], + review: { kind: "commit", provider: "GitHub", title: "Commit", revision: "abc" }, + }), + }); + + await expect( + prepareStartupPlan(["bun", "hunk", "tools"], { + parseCliImpl: async (argv) => + argv.includes("--version") ? { kind: "help", text: "1.2.3\n" } : invocation, + resolveExtensionCliBootstrapImpl: async ({ baseVcsCatalog }) => ({ + configured: { + extensions: { enabled: true, paths: [], repoPaths: [], extensionConfigs: {} }, + }, + extensions, + commands: resolveExtensionCliCommands(extensions.registry), + collisionIssues: [], + discoveryCatalog: baseVcsCatalog, + }), + }), + ).rejects.toThrow("only to a delegated patch command"); + }); + test("lists the loaded extension commands when the requested token is unclaimed", async () => { const invocation = { kind: "extension-cli" as const, diff --git a/src/app/startup.ts b/src/app/startup.ts index 79c05c50e..bea624496 100644 --- a/src/app/startup.ts +++ b/src/app/startup.ts @@ -28,6 +28,7 @@ import { assertReliableWatchRuntime } from "../core/watch/runtime"; import { parseCli } from "./cli"; import { resolveSessionSelectorBoundary } from "./sessionSelector"; import type { VcsCatalog } from "../core/vcs/types"; +import type { ExtensionReviewDescriptor } from "../extension-api/types"; /** * Load the bundled VCS catalog, memoized per call to `prepareStartupPlan`. @@ -175,6 +176,7 @@ export async function prepareStartupPlan( let controllingTerminal: ControllingTerminal | null = null; let preloadedExtensions: import("../extensions/types").ExtensionLoadResult | undefined; let delegatedDiscoveryCatalog: VcsCatalog | undefined; + let delegatedReview: ExtensionReviewDescriptor | undefined; /** Retire startup-owned extension state before returning a non-app plan. */ const retirePreloadedExtensions = async () => { @@ -296,6 +298,12 @@ export async function prepareStartupPlan( "Extension CLI commands may delegate only to built-in Hunk commands.", ); } + if (execution.result.review && delegated.kind !== "patch") { + throw new HunkUserError( + "Extension review metadata may be attached only to a delegated patch command.", + ); + } + delegatedReview = execution.result.review; parsedCliInput = applyDelegatedExtensionFlags(delegated, invocation); delegatedDiscoveryCatalog = resolved.discoveryCatalog; } catch (error) { @@ -550,6 +558,7 @@ export async function prepareStartupPlan( } const { applied, bootstrap, input: resolvedInput, sessionThemes, sessionVcs } = preparedSession; cliInput = resolvedInput; + if (delegatedReview) bootstrap.review = delegatedReview; // Built after adapter resolution so the notice names the backend the session really loads with. const unknownVcsNotices = diff --git a/src/core/bootstrap.ts b/src/core/bootstrap.ts index 76e47f73e..4860fa1b8 100644 --- a/src/core/bootstrap.ts +++ b/src/core/bootstrap.ts @@ -11,7 +11,7 @@ * boundary rules name: `changeset/loaders.ts` assembles this value in * `loadAppBootstrap`, so it has to name the shape it returns. */ -import type { NamedCustomThemeConfig } from "../extension-api/types"; +import type { ExtensionReviewDescriptor, NamedCustomThemeConfig } from "../extension-api/types"; import type { Changeset } from "./changeset/model"; import type { CliInput, CursorLine, LayoutMode, SidebarVisibility } from "./run/commandInputs"; import type { UserKeyBinding } from "./run/config"; @@ -55,6 +55,8 @@ export interface AppBootstrap { initialCopyDecorations?: boolean; initialCursorLine?: CursorLine; startupNotices?: readonly StartupNotice[]; + /** Validated metadata attached only by an extension-delegated patch review. */ + review?: ExtensionReviewDescriptor; viewPreferencesConfigPath?: string; /** The user's `[keybindings]` table, resolved against command defaults in App. */ keybindings?: Record; diff --git a/src/extension-api/index.ts b/src/extension-api/index.ts index dbc86e2fe..e75d4fb32 100644 --- a/src/extension-api/index.ts +++ b/src/extension-api/index.ts @@ -82,6 +82,11 @@ export type { ExtensionCliDelegateResult, ExtensionCliExitResult, ExtensionCliWriter, + ExtensionReviewDescriptor, + ExtensionReviewDescriptorBase, + ExtensionChangeRequestReviewDescriptor, + ExtensionCommitReviewDescriptor, + ExtensionComparisonReviewDescriptor, ExtensionCommand, ExtensionCommandContext, ExtensionCommandControls, diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index b162f6944..aa03f23ae 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -21,7 +21,7 @@ * Extensions can branch on `hunk.apiVersion` so a newer Hunk can keep loading * older extensions without guessing at their expectations. */ -export const HUNK_EXTENSION_API_VERSION = 16; +export const HUNK_EXTENSION_API_VERSION = 17; export type HunkExtensionApiVersion = typeof HUNK_EXTENSION_API_VERSION; export type ExtensionNotifyType = "info" | "warning" | "error"; @@ -1279,11 +1279,54 @@ export interface ExtensionCliExitResult { readonly code?: number; } +/** Shared display fields for one extension-described review source. */ +export interface ExtensionReviewDescriptorBase { + /** Provider name shown to the user, such as `GitHub` or `GitLab`. */ + readonly provider: string; + /** Human-readable review title. */ + readonly title: string; + /** Optional HTTPS page for the described review source. */ + readonly url?: string; +} + +/** Metadata for a provider change request such as a pull or merge request. */ +export interface ExtensionChangeRequestReviewDescriptor extends ExtensionReviewDescriptorBase { + readonly kind: "change-request"; + /** Provider-local identifier, such as `#123`. */ + readonly id: string; + readonly author?: string; + readonly base?: string; + readonly head?: string; +} + +/** Metadata for one reviewed commit. */ +export interface ExtensionCommitReviewDescriptor extends ExtensionReviewDescriptorBase { + readonly kind: "commit"; + /** Provider revision identifier. */ + readonly revision: string; + readonly author?: string; +} + +/** Metadata for one comparison between two provider refs. */ +export interface ExtensionComparisonReviewDescriptor extends ExtensionReviewDescriptorBase { + readonly kind: "comparison"; + readonly base: string; + readonly head: string; +} + +/** Bounded provider-neutral metadata attached to an extension-delegated patch review. */ +export type ExtensionReviewDescriptor = + | ExtensionChangeRequestReviewDescriptor + | ExtensionCommitReviewDescriptor + | ExtensionComparisonReviewDescriptor; + /** Hand terminal ownership to one built-in Hunk command. */ export interface ExtensionCliDelegateResult { readonly kind: "delegate"; /** Tokens after the `hunk` executable. Extension commands cannot be targets. */ readonly argv: readonly string[]; + /** Optional metadata for a delegated built-in `patch` review. */ + readonly review?: ExtensionReviewDescriptor; } export type ExtensionCliCommandResult = ExtensionCliExitResult | ExtensionCliDelegateResult; diff --git a/src/extensions/cliCommandRuntime.test.ts b/src/extensions/cliCommandRuntime.test.ts index 2d70025fd..dfa203926 100644 --- a/src/extensions/cliCommandRuntime.test.ts +++ b/src/extensions/cliCommandRuntime.test.ts @@ -259,6 +259,120 @@ describe("extension CLI command runtime", () => { ).rejects.toThrow("exit code must be a safe integer"); }); + test("copies and freezes exact bounded delegated review descriptors", async () => { + const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "Add review metadata", + url: "https://github.com/modem-dev/hunk/pull/123", + id: "#123", + author: "octocat", + base: "main", + head: "metadata", + }; + const execution = await runExtensionCliCommand({ + extensionId: "tools", + commandName: "tools", + args: [], + stdin: (async function* () {})(), + stdout: createTestWriter([]), + stderr: createTestWriter([]), + signals: new EventEmitter(), + handler: () => ({ kind: "delegate", argv: ["patch", "review.diff"], review }), + }); + + expect(execution.result).toEqual({ + kind: "delegate", + argv: ["patch", "review.diff"], + review, + }); + if (execution.result.kind !== "delegate" || !execution.result.review) { + throw new Error("Expected delegated review metadata."); + } + expect(execution.result.review).not.toBe(review); + expect(Object.isFrozen(execution.result.review)).toBe(true); + review.title = "mutated"; + expect(execution.result.review.title).toBe("Add review metadata"); + }); + + test("rejects malformed, unsafe, and oversized delegated review descriptors", async () => { + const execute = (review: unknown, kind: "delegate" | "exit" = "delegate") => + runExtensionCliCommand({ + extensionId: "tools", + commandName: "tools", + args: [], + stdin: (async function* () {})(), + stdout: createTestWriter([]), + stderr: createTestWriter([]), + signals: new EventEmitter(), + handler: () => + kind === "delegate" + ? ({ kind, argv: ["patch", "review.diff"], review } as never) + : ({ kind, review } as never), + }); + + const valid = { kind: "commit", provider: "GitHub", title: "Commit", revision: "abc" }; + await expect(execute({ ...valid, extra: true })).rejects.toThrow("unknown fields"); + await expect(execute({ ...valid, title: "bad\u001b[31m" })).rejects.toThrow( + "control characters", + ); + await expect(execute({ ...valid, url: "http://github.com/commit/abc" })).rejects.toThrow( + "credential-free HTTPS URL", + ); + await expect(execute({ ...valid, url: "https://user@example.com/commit/abc" })).rejects.toThrow( + "credential-free HTTPS URL", + ); + await expect(execute({ ...valid, title: "x".repeat(2049) })).rejects.toThrow("byte limit"); + await expect( + execute({ + kind: "change-request", + provider: "p".repeat(256), + title: "t".repeat(1800), + url: `https://example.com/${"u".repeat(1800)}`, + id: "i".repeat(256), + author: "a".repeat(100), + }), + ).rejects.toThrow("total byte limit"); + await expect(execute(valid, "exit")).rejects.toThrow("exit results cannot include"); + }); + + test("bounds the complete serialized delegated review descriptor at exactly 4 KiB", async () => { + const execute = (review: unknown) => + runExtensionCliCommand({ + extensionId: "tools", + commandName: "tools", + args: [], + stdin: (async function* () {})(), + stdout: createTestWriter([]), + stderr: createTestWriter([]), + signals: new EventEmitter(), + handler: () => ({ kind: "delegate", argv: ["patch", "review.diff"], review }) as never, + }); + const withoutHead = { + kind: "change-request" as const, + provider: "p".repeat(256), + title: "t".repeat(2048), + id: "i".repeat(256), + author: "a".repeat(512), + base: "b".repeat(512), + head: "", + }; + const serializedWithoutHeadBytes = new TextEncoder().encode(JSON.stringify(withoutHead)).length; + const multibytePrefix = "🚀"; + const exactHead = `${multibytePrefix}${"h".repeat( + 4 * 1024 - serializedWithoutHeadBytes - new TextEncoder().encode(multibytePrefix).length, + )}`; + const exact = { ...withoutHead, head: exactHead }; + expect(new TextEncoder().encode(JSON.stringify(exact))).toHaveLength(4 * 1024); + await expect(execute(exact)).resolves.toMatchObject({ + result: { kind: "delegate", review: exact }, + }); + + const oversized = { ...exact, head: `${exactHead}h` }; + expect(new TextEncoder().encode(JSON.stringify(oversized))).toHaveLength(4 * 1024 + 1); + await expect(execute(oversized)).rejects.toThrow("total byte limit"); + }); + test("aborts on host signals and removes listeners", async () => { const signals = new EventEmitter(); const execution = runExtensionCliCommand({ diff --git a/src/extensions/cliCommandRuntime.ts b/src/extensions/cliCommandRuntime.ts index e1caed86f..d07c6ae3d 100644 --- a/src/extensions/cliCommandRuntime.ts +++ b/src/extensions/cliCommandRuntime.ts @@ -3,6 +3,7 @@ import type { ExtensionCliCommandContext, ExtensionCliCommandHandler, ExtensionCliCommandResult, + ExtensionReviewDescriptor, ExtensionCliWriter, } from "./types"; import { describeError } from "./runExtension"; @@ -181,14 +182,154 @@ function createTrackedStdin( }; } +const REVIEW_DESCRIPTOR_TOTAL_BYTES = 4 * 1024; +const REVIEW_DESCRIPTOR_FIELD_LIMITS = Object.freeze({ + provider: 256, + title: 2 * 1024, + url: 2 * 1024, + id: 256, + author: 512, + base: 512, + head: 512, + revision: 512, +}); + +/** Measure a public descriptor string in transport bytes rather than UTF-16 code units. */ +function descriptorByteLength(value: string) { + return new TextEncoder().encode(value).byteLength; +} + +/** Validate one bounded terminal-safe descriptor string. */ +function validateDescriptorString( + candidate: Record, + field: keyof typeof REVIEW_DESCRIPTOR_FIELD_LIMITS, + required: boolean, +): string | undefined { + if (!Object.prototype.hasOwnProperty.call(candidate, field)) { + if (!required) return undefined; + throw new Error(`delegate review ${field} must be a non-empty string`); + } + const value = candidate[field]; + if (value === undefined && !required) return undefined; + if (typeof value !== "string" || value.length === 0) { + throw new Error(`delegate review ${field} must be a non-empty string`); + } + if (/[\u0000-\u001f\u007f-\u009f]/u.test(value)) { + throw new Error(`delegate review ${field} cannot contain control characters`); + } + if (descriptorByteLength(value) > REVIEW_DESCRIPTOR_FIELD_LIMITS[field]) { + throw new Error(`delegate review ${field} exceeds its byte limit`); + } + return value; +} + +/** Copy only present optional fields after applying their individual bounds. */ +function copyOptionalDescriptorFields( + candidate: Record, + fields: readonly (keyof typeof REVIEW_DESCRIPTOR_FIELD_LIMITS)[], +): Record { + const copied: Record = {}; + for (const field of fields) { + const value = validateDescriptorString(candidate, field, false); + if (value !== undefined) copied[field] = value; + } + return copied; +} + +/** Validate, copy, and deeply freeze provider-neutral delegated review metadata. */ +function validateReviewDescriptor(value: unknown): ExtensionReviewDescriptor { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + throw new Error("delegate review must be an object"); + } + const prototype = Object.getPrototypeOf(value); + if (prototype !== Object.prototype && prototype !== null) { + throw new Error("delegate review must be a plain object"); + } + const candidate = value as Record; + const kind = candidate.kind; + if ( + !Object.prototype.hasOwnProperty.call(candidate, "kind") || + (kind !== "change-request" && kind !== "commit" && kind !== "comparison") + ) { + throw new Error('delegate review kind must be "change-request", "commit", or "comparison"'); + } + + const common = ["kind", "provider", "title", "url"]; + const kindFields = + kind === "change-request" + ? ["id", "author", "base", "head"] + : kind === "commit" + ? ["revision", "author"] + : ["base", "head"]; + const allowed = new Set([...common, ...kindFields]); + const ownKeys = Reflect.ownKeys(value); + if (ownKeys.some((key) => typeof key !== "string" || !allowed.has(key))) { + throw new Error("delegate review contains unknown fields"); + } + + const provider = validateDescriptorString(candidate, "provider", true)!; + const title = validateDescriptorString(candidate, "title", true)!; + const url = validateDescriptorString(candidate, "url", false); + if (url !== undefined) { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + throw new Error("delegate review url must be a valid HTTPS URL"); + } + if (parsed.protocol !== "https:" || parsed.username || parsed.password) { + throw new Error("delegate review url must be a credential-free HTTPS URL"); + } + } + + let descriptor: ExtensionReviewDescriptor; + if (kind === "change-request") { + descriptor = { + kind, + provider, + title, + ...(url === undefined ? {} : { url }), + id: validateDescriptorString(candidate, "id", true)!, + ...copyOptionalDescriptorFields(candidate, ["author", "base", "head"]), + }; + } else if (kind === "commit") { + descriptor = { + kind, + provider, + title, + ...(url === undefined ? {} : { url }), + revision: validateDescriptorString(candidate, "revision", true)!, + ...copyOptionalDescriptorFields(candidate, ["author"]), + }; + } else { + descriptor = { + kind, + provider, + title, + ...(url === undefined ? {} : { url }), + base: validateDescriptorString(candidate, "base", true)!, + head: validateDescriptorString(candidate, "head", true)!, + }; + } + + const totalBytes = descriptorByteLength(JSON.stringify(descriptor)); + if (totalBytes > REVIEW_DESCRIPTOR_TOTAL_BYTES) { + throw new Error("delegate review exceeds the total byte limit"); + } + return Object.freeze(descriptor); +} + /** Validate and freeze the result returned by one extension CLI handler. */ function validateExtensionCliResult(result: unknown): ExtensionCliCommandResult { if (typeof result !== "object" || result === null || Array.isArray(result)) { throw new Error('must return an object with kind "exit" or "delegate"'); } - const candidate = result as { kind?: unknown; code?: unknown; argv?: unknown }; + const candidate = result as { kind?: unknown; code?: unknown; argv?: unknown; review?: unknown }; if (candidate.kind === "exit") { + if ("review" in candidate) { + throw new Error("exit results cannot include delegated review metadata"); + } const code = candidate.code ?? 0; if (!Number.isSafeInteger(code) || (code as number) < 0 || (code as number) > 255) { throw new Error("exit code must be a safe integer from 0 through 255"); @@ -215,7 +356,13 @@ function validateExtensionCliResult(result: unknown): ExtensionCliCommandResult ) { throw new Error("delegate argv cannot change extension bootstrap flags"); } - return Object.freeze({ kind: "delegate", argv: Object.freeze([...argv]) }); + const review = + candidate.review === undefined ? undefined : validateReviewDescriptor(candidate.review); + return Object.freeze({ + kind: "delegate", + argv: Object.freeze([...argv]), + ...(review === undefined ? {} : { review }), + }); } throw new Error('result kind must be "exit" or "delegate"'); diff --git a/src/extensions/types.ts b/src/extensions/types.ts index 1a399b57f..ca5d5f312 100644 --- a/src/extensions/types.ts +++ b/src/extensions/types.ts @@ -59,6 +59,7 @@ export type { ExtensionKeyboardMode, ExtensionKeyboardModeKeyResult, ExtensionLineHighlighter, + ExtensionReviewDescriptor, ExtensionReviewControls, ExtensionReviewNote, ExtensionReviewSnapshot, diff --git a/src/ui/AppHost.review-metadata.test.tsx b/src/ui/AppHost.review-metadata.test.tsx new file mode 100644 index 000000000..50810cf84 --- /dev/null +++ b/src/ui/AppHost.review-metadata.test.tsx @@ -0,0 +1,210 @@ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, test } from "bun:test"; +import { testRender } from "@opentui/react/test-utils"; +import { act } from "react"; +import { SESSION_BROKER_REGISTRATION_VERSION } from "@hunk/session-broker-core"; +import { createWatchTestRuntime } from "../../test/helpers/watchTest"; +import type { AppBootstrap } from "../core/bootstrap"; +import { loadAppBootstrap } from "../core/changeset/loaders"; +import type { HunkSessionBrokerClient } from "../session/broker/brokerClient"; +import type { + HunkSessionRegistration, + HunkSessionServerMessage, + HunkSessionSnapshot, +} from "../session/types"; +import { AppHost } from "./AppHost"; + +/** Stand in for the daemon so a mounted AppHost can receive unrelated reloads. */ +function createTestHostClient() { + type Bridge = Parameters[0]; + let bridge: Bridge = null; + let registration: HunkSessionRegistration = { + registrationVersion: SESSION_BROKER_REGISTRATION_VERSION, + sessionId: "session-1", + pid: process.pid, + cwd: process.cwd(), + repoRoot: process.cwd(), + launchedAt: "2026-09-04T00:00:00.000Z", + info: { inputKind: "patch", title: "Patch", sourceLabel: "Patch", files: [] }, + }; + + return { + hostClient: { + getRegistration: () => registration, + replaceSession: (nextRegistration: HunkSessionRegistration) => { + registration = nextRegistration; + }, + setBridge: (nextBridge: Bridge) => { + bridge = nextBridge; + }, + updateSnapshot: (_snapshot: HunkSessionSnapshot) => {}, + } as unknown as HunkSessionBrokerClient, + dispatchCommand: async (message: HunkSessionServerMessage) => { + if (!bridge) throw new Error("Expected AppHost to register its daemon bridge."); + return bridge.dispatchCommand(message); + }, + }; +} + +/** Write a minimal unified patch whose content marker changes across reloads. */ +function writeTestPatch(path: string, marker: string) { + writeFileSync( + path, + [ + "diff --git a/example.txt b/example.txt", + "--- a/example.txt", + "+++ b/example.txt", + "@@ -1 +1 @@", + "-before", + `+${marker}`, + "", + ].join("\n"), + ); +} + +/** Create a repository-backed patch fixture so AppHost may reload sibling inputs. */ +async function createTestBootstrap({ watch = false }: { watch?: boolean } = {}) { + const directory = mkdtempSync(join(tmpdir(), "hunk-review-metadata-host-")); + execFileSync("git", ["init", "-b", "main"], { cwd: directory, stdio: "ignore" }); + const firstPatch = join(directory, "first.diff"); + const secondPatch = join(directory, "second.diff"); + writeTestPatch(firstPatch, "first"); + writeTestPatch(secondPatch, "second"); + const bootstrap = await loadAppBootstrap( + { kind: "patch", file: firstPatch, options: { mode: "stack", watch } }, + { cwd: directory }, + ); + bootstrap.review = Object.freeze({ + kind: "change-request", + provider: "GitHub", + title: "PR #123 · Metadata", + id: "#123", + head: "abc123", + }); + return { bootstrap, directory, firstPatch, secondPatch }; +} + +/** Settle mounted host work until a committed bootstrap observation arrives. */ +async function flushUntil( + setup: Awaited>, + predicate: () => boolean, + description: string, +) { + for (let attempt = 0; attempt < 30 && !predicate(); attempt++) { + await act(async () => { + await setup.renderOnce(); + await Promise.resolve(); + await setup.renderOnce(); + }); + } + if (!predicate()) throw new Error(`Timed out waiting for ${description}.`); +} + +describe("delegated review metadata reloads", () => { + test("manual refresh preserves the same patch metadata and unrelated reloads clear it durably", async () => { + const fixture = await createTestBootstrap(); + const committed: AppBootstrap[] = []; + const broker = createTestHostClient(); + const setup = await testRender( + committed.push(bootstrap)} + />, + { width: 100, height: 12 }, + ); + + try { + await flushUntil(setup, () => committed.length === 1, "the delegated review to mount"); + + writeTestPatch(fixture.firstPatch, "manually refreshed"); + await act(async () => setup.mockInput.typeText("r")); + await flushUntil(setup, () => committed.length >= 2, "the manual refresh to commit"); + expect(committed.at(-1)?.review).toBe(fixture.bootstrap.review); + + await act(async () => { + await broker.dispatchCommand({ + type: "command", + requestId: "unrelated-patch", + command: "reload_session", + input: { + sessionId: "session-1", + nextInput: { + kind: "patch", + file: fixture.secondPatch, + options: { mode: "stack" }, + }, + }, + }); + }); + await flushUntil(setup, () => committed.length >= 3, "the unrelated review to commit"); + expect(committed.at(-1)?.review).toBeUndefined(); + + // Returning to the original resource compares against the latest identity, + // rather than resurrecting metadata retained from the initial delegated launch. + await act(async () => { + await broker.dispatchCommand({ + type: "command", + requestId: "return-to-original-patch", + command: "reload_session", + input: { + sessionId: "session-1", + nextInput: { + kind: "patch", + file: fixture.firstPatch, + options: { mode: "stack" }, + }, + }, + }); + }); + await flushUntil(setup, () => committed.length >= 4, "the original resource to remount"); + expect(committed.at(-1)?.review).toBeUndefined(); + } finally { + await act(async () => setup.renderer.destroy()); + rmSync(fixture.directory, { recursive: true, force: true }); + } + }); + + test("watch refresh preserves delegated metadata for the same patch resource", async () => { + const fixture = await createTestBootstrap({ watch: true }); + const committed: AppBootstrap[] = []; + const watch = createWatchTestRuntime(); + const setup = await testRender( + committed.push(bootstrap)} + watchRuntime={watch.runtime} + />, + { width: 100, height: 12 }, + ); + + try { + await flushUntil(setup, () => committed.length === 1, "the watched review to mount"); + expect(watch.sources).toHaveLength(1); + writeTestPatch(fixture.firstPatch, "watched refresh"); + watch.setSignature("signature:changed"); + watch.emit(); + await act(async () => { + watch.advanceBy(200); + await Promise.resolve(); + }); + await flushUntil(setup, () => committed.length >= 2, "the watch refresh to commit"); + for (let attempt = 0; attempt < 6; attempt++) { + await act(async () => { + await setup.renderOnce(); + await Promise.resolve(); + }); + } + + expect(committed.at(-1)?.review).toBe(fixture.bootstrap.review); + expect(watch.sources).toHaveLength(2); + expect(watch.sources[0]?.closeCount).toBe(1); + } finally { + await act(async () => setup.renderer.destroy()); + rmSync(fixture.directory, { recursive: true, force: true }); + } + }); +}); diff --git a/src/ui/AppHost.tsx b/src/ui/AppHost.tsx index 9e6fd2e78..246c3088c 100644 --- a/src/ui/AppHost.tsx +++ b/src/ui/AppHost.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, useLayoutEffect, useRef, useState } from "react"; import { resolveConfiguredExtensions } from "../app/extensionBootstrap"; import { ReviewProducer } from "../app/review/producer"; +import { reviewDescriptorAfterReload } from "../app/delegatedReview"; import { loadConfiguredSessionBootstrap } from "../app/sessionBootstrap"; import { getBundledVcsCatalog } from "../app/vcsCatalog"; import { restoreFileLanguageRegistrations } from "../core/changeset/fileLanguage"; @@ -53,6 +54,7 @@ export function AppHost({ externalQuitSignal, hostClient, onQuit = () => process.exit(0), + onActiveBootstrapChange, reviewProducer, startupNoticeResolver, watchRuntime, @@ -64,6 +66,8 @@ export function AppHost({ externalQuitSignal?: AbortSignal; hostClient?: HunkSessionBrokerClient; onQuit?: () => void; + /** Observe the bootstrap after its matching App commit; used by mounted host tests. */ + onActiveBootstrapChange?: (bootstrap: AppBootstrap) => void; /** * The producer whose generations this host publishes. Supplied by the process that * built the initial registration from its first publication; a host mounted without one @@ -86,6 +90,11 @@ export function AppHost({ }, }; const [activeBootstrap, setActiveBootstrap] = useState(initialBootstrap); + const reviewIdentityRef = useRef({ + input: initialBootstrap.input, + cwd: initialBootstrap.reloadContext.cwd, + review: initialBootstrap.review, + }); const [producer] = useState( () => reviewProducer ?? @@ -143,6 +152,10 @@ export function AppHost({ resolver: startupNoticeResolver, }); + useLayoutEffect(() => { + onActiveBootstrapChange?.(activeBootstrap); + }, [activeBootstrap, onActiveBootstrapChange]); + useLayoutEffect(() => { // Child layout effects run before the parent's, so controls and generation // leases are live here; passive UI events still wait until this order lands. @@ -334,6 +347,14 @@ export function AppHost({ try { const { applied, bootstrap, input: reloadInput, sessionVcs } = loaded; nextBootstrap = bootstrap; + const preservedReview = reviewDescriptorAfterReload( + reviewIdentityRef.current.input, + reviewIdentityRef.current.cwd, + reviewIdentityRef.current.review, + nextBootstrap.input, + cwd, + ); + if (preservedReview) nextBootstrap.review = preservedReview; if (extensions) { reportExtensionApplyIssues(applied.issues, extensions.context); } @@ -400,6 +421,11 @@ export function AppHost({ }) : undefined; + reviewIdentityRef.current = { + input: nextBootstrap.input, + cwd, + review: nextBootstrap.review, + }; setActiveBootstrap(nextBootstrap); if (options?.resetApp !== false) { // Bumping the key forces a full App remount. Callers that pass `resetApp: false` get a diff --git a/website/src/content/docs/docs/extend/extension-api.md b/website/src/content/docs/docs/extend/extension-api.md index df94e725d..3028a5ee6 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -7,8 +7,9 @@ The extension factory receives one API object. Registration calls are only valid ## `hunk.apiVersion` -The API generation this Hunk speaks (currently `16`). Branch on it if you want -one file to support several Hunk versions. Version 16 adds pane-wide +The API generation this Hunk speaks (currently `17`). Branch on it if you want +one file to support several Hunk versions. Version 17 adds structured review metadata to delegated +patch commands; version 16 adds pane-wide `onActivate`; version 15 added `{ side, line }` to opted-in pane `currentLine` paint; version 14 added structured two-revision VCS diff endpoints; version 13 added saved-note parent identities @@ -37,7 +38,20 @@ hunk.registerCliCommand( return { kind: "exit" }; } await ctx.stderr.write("Preparing review…\n"); - return { kind: "delegate", argv: ["diff"] }; + return { + kind: "delegate", + argv: ["patch", "review.diff"], + review: { + kind: "change-request", + provider: "GitHub", + title: "Add structured review metadata", + url: "https://github.com/acme/project/pull/123", + id: "#123", + author: "octocat", + base: "main", + head: "review-metadata", + }, + }; }, ); ``` @@ -45,9 +59,17 @@ hunk.registerCliCommand( Handlers receive `ctx.cwd`, cooperative `ctx.signal`, streaming `ctx.stdin`, and leased, backpressure-aware stdout/stderr writers. They may access networks, services, processes, and files. Return a validated exit status or delegate once -to a built-in Hunk command. Delegation cannot follow stdout output or any stdin -read, target another extension command, or change extension bootstrap flags. Built-ins and aliases cannot be shadowed; the first extension claim in -discovery order wins. +to a built-in Hunk command. A delegated `patch` command may also carry a provider-neutral +`review` descriptor whose exact shape is `change-request`, `commit`, or `comparison`. Hunk bounds +all strings and the 4 KiB payload, rejects control characters, unknown fields, and unsafe URLs, +then copies and freezes it. `provider` and change-request `id` allow 256 bytes; `author`, `base`, +`head`, and `revision` allow 512; `title` and `url` allow 2 KiB. The descriptor remains app-bootstrap metadata rather than entering +changeset transforms or `ReviewDocumentV1`; same-file refreshes preserve it, while unrelated +reloads clear it. Exit results and non-`patch` delegation cannot carry one. + +Delegation cannot follow stdout output or any stdin read, target another extension command, or +change extension bootstrap flags. Built-ins and aliases cannot be shadowed; the first extension +claim in discovery order wins. Use a leading explicit path while developing: From bf3731d38a541a7a4ff8c272b2edf9821b2c2d8f Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Fri, 4 Sep 2026 15:46:31 -0400 Subject: [PATCH 2/6] feat(extensions): show delegated review info pane --- .changeset/calm-reviews-describe.md | 2 +- docs/extension-architecture.md | 14 ++-- docs/extensions.md | 15 +++- skills/hunk-extensions/SKILL.md | 6 +- src/extension-api/types.ts | 8 +++ src/extensions/cliCommandRuntime.test.ts | 8 +++ src/extensions/cliCommandRuntime.ts | 23 +++++- src/extensions/default/ui/index.test.ts | 35 ++++++++- src/extensions/default/ui/index.ts | 6 +- .../default/ui/reviewInfo/index.tsx | 42 +++++++++++ .../ui/reviewInfo/presentation.test.ts | 51 +++++++++++++ .../default/ui/reviewInfo/presentation.ts | 50 +++++++++++++ .../default/ui/sidebar/FileSidebars.tsx | 7 +- src/ui/App.tsx | 2 + src/ui/AppHost.review-metadata.test.tsx | 47 +++++++++++- .../components/panes/ExtensionPane.test.tsx | 39 ++++++++++ src/ui/components/panes/ExtensionPane.tsx | 4 ++ .../hooks/useExtensionPaneController.test.tsx | 54 +++++++++++++- src/ui/hooks/useExtensionPaneController.ts | 1 + src/ui/lib/extensionPanes.test.ts | 71 +++++++++++++++++-- .../content/docs/docs/extend/extension-api.md | 12 ++-- 21 files changed, 467 insertions(+), 30 deletions(-) create mode 100644 src/extensions/default/ui/reviewInfo/index.tsx create mode 100644 src/extensions/default/ui/reviewInfo/presentation.test.ts create mode 100644 src/extensions/default/ui/reviewInfo/presentation.ts diff --git a/.changeset/calm-reviews-describe.md b/.changeset/calm-reviews-describe.md index 129bf6e34..bc5d25c41 100644 --- a/.changeset/calm-reviews-describe.md +++ b/.changeset/calm-reviews-describe.md @@ -2,4 +2,4 @@ "hunkdiff": minor --- -Let extension CLI commands attach validated provider-neutral review metadata when they delegate a patch into Hunk. +Let extension CLI commands attach validated provider-neutral review metadata when they delegate a patch into Hunk, expose it to extension panes, and show delegated change-request identity in a concise built-in top pane. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index 78af21a02..03fb4f6db 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -20,11 +20,11 @@ object and registry collection (`src/extensions/runExtension.ts`): by the app composition root (`app/vcsCatalog.ts`) and loaded synchronously before config resolution, so backends exist without making core import the extension host. `default/ui/index.ts` is deliberately not part of that list: - it synchronously loads the bundled files pane through `runExtensionFactory` - only where the app resolves UI panes. + it synchronously loads the bundled files and delegated review-info panes through + `runExtensionFactory` only where the app resolves UI panes. -Git and the built-in file navigation use the public `registerVcsAdapter` and -`registerPane` paths. The external [Hunk Lens](https://github.com/modem-dev/hunk-lens) +Git, built-in file navigation, and delegated change-request identity use the public +`registerVcsAdapter` and `registerPane` paths. The external [Hunk Lens](https://github.com/modem-dev/hunk-lens) extension exercises current-line pane paint through that same public contract. Bundled extensions are implicitly trusted and stay loaded under @@ -95,8 +95,10 @@ stream coordinates. Pane registrations may opt into a body-axis `fraction`; the planner resolves it to an integer target before applying bounds and lets a session-local divider drag override that automatic size. -`src/ui/components/panes/ExtensionPane.tsx` mounts panes with guarded actions and -failure containment. `DiffPane` exposes optional current-line paint — the row +`src/ui/components/panes/ExtensionPane.tsx` mounts panes with guarded actions, +immutable delegated review metadata, and failure containment. The fixed two-row +`hunk:review-info` top pane is available only for delegated change requests, so +ordinary reviews spend no geometry on it. `DiffPane` exposes optional current-line paint — the row painter plus the public `{ side, line }` address — without publishing Pierre rows, plans, cursor keys, or caches. Deprecated sidebar APIs normalize into this same registry and layout path. diff --git a/docs/extensions.md b/docs/extensions.md index 3db67f617..d4a2ac2b8 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -282,7 +282,7 @@ new instances and run that shutdown/startup pair around the replacement. The API generation this Hunk speaks (currently `17`). Branch on it if you want one file to support several Hunk versions. Version 17 adds structured review metadata to delegated -patch commands; version 16 adds pane-wide +patch commands and projects it into pane availability and component props; version 16 adds pane-wide `onActivate`; version 15 added `{ side, line }` to opted-in pane `currentLine` paint; version 14 added structured `rangeEndpoints` to two-revision VCS diff requests; version 13 added saved-note parent identities and @@ -322,9 +322,12 @@ hunk.registerCliCommand( title: "Add structured review metadata", url: "https://github.com/acme/project/pull/123", id: "#123", + repository: "acme/project", author: "octocat", base: "main", head: "review-metadata", + state: "open", + draft: false, }, }; }, @@ -343,7 +346,8 @@ A delegated built-in `patch` command may include a provider-neutral `review` des strings with an optional credential-free HTTPS URL. Hunk rejects unknown fields, control characters, invalid types, unsafe URLs, fields over their byte limits, and descriptors over 4 KiB, then copies and freezes the accepted value. `provider` and change-request `id` allow 256 bytes; -`author`, `base`, `head`, and `revision` allow 512; `title` and `url` allow 2 KiB. Exit results and delegation to any built-in other than +`repository`, `author`, `base`, `head`, and `revision` allow 512; `title` and `url` allow 2 KiB. +Change requests may also carry `state` (`open`, `closed`, or `merged`) and boolean `draft`. Exit results and delegation to any built-in other than `patch` cannot carry review metadata. An ordinary `hunk patch` has no descriptor. The descriptor describes the review source rather than its diff contents: it stays on the app @@ -773,6 +777,12 @@ hide it conditionally. One pane may replace each named target; the first registration owns that slot and later claims are skipped with a warning. `replaces` may also name another pane by its fully qualified `":"` key, and Hunk follows those replacement chains. +Both `available(context)` and the mounted component receive `review`: immutable +metadata supplied by a delegated patch command, or `null` for ordinary reviews. +The bundled `hunk:review-info` top pane uses this to show change-request identity +without taking any rows when no change-request descriptor exists. Pane extensions that read +`review` should declare `"hunk": { "apiVersion": 17 }` in their manifest so older Hunk versions +refuse them cleanly instead of mounting with an incomplete prop contract. `onActivate()` observes a primary mouse press anywhere in the pane's content, including content nested in a ``. Use it to focus an extension-owned @@ -808,6 +818,7 @@ The component receives fresh props as the app changes: | Prop | What it is | | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `review` | immutable delegated review metadata (`change-request`, `commit`, or `comparison`), or `null` for ordinary reviews | | `files` | the visible reviewed files, review-stream order, filtered, frozen views (each carries `changeType`, `statsTruncated`, and `hunks` summaries beside the usual file fields) | | `selectedFileId` | the selected file, or `null` | | `selectedHunkIndex` | the selected hunk within that file, or `null` | diff --git a/skills/hunk-extensions/SKILL.md b/skills/hunk-extensions/SKILL.md index 9bb72282d..813027c6b 100644 --- a/skills/hunk-extensions/SKILL.md +++ b/skills/hunk-extensions/SKILL.md @@ -163,8 +163,10 @@ transform — gets `ctx.cwd` and `ctx.notify(message, type?)`. A file view's `ctx.dialogs` (`confirm`/`select`/`input`, queued and attributed), and `ctx.workspace` (`readDocument`, `canWriteDocument`, `writeDocument` with consent). - **Pane components** get frozen `files`, selection, placement, exact dimensions, - optional `currentLine` paint (with `{ side, line }` when opted in), semantic `theme`, resolved `keybindings`, and - guarded navigation/notification `actions`. + nullable immutable delegated-source `review` metadata, optional `currentLine` paint + (with `{ side, line }` when opted in), semantic `theme`, resolved `keybindings`, and + guarded navigation/notification `actions`. Availability callbacks receive the same + `review` value, so a pane can consume no geometry for ordinary reviews. - **File-view `layout`** gets `file`, `width`, `signal`, `changes`, and a lazy `readDocument(side)`. - **File-view `mode` handlers** get `ctx.file` and `ctx.fileViews`. `onKey`, diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index aa03f23ae..2676ab0bd 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -1144,6 +1144,8 @@ export interface ExtensionCurrentLinePaint { /** Immutable state used to decide whether an open pane is meaningful this frame. */ export interface ExtensionPaneAvailabilityContext { readonly placement: ExtensionPanePlacement; + /** Immutable delegated review metadata, or null for ordinary reviews. */ + readonly review: ExtensionReviewDescriptor | null; readonly files: readonly ExtensionDiffFile[]; readonly selectedFileId: string | null; readonly selectedHunkIndex: number | null; @@ -1152,6 +1154,8 @@ export interface ExtensionPaneAvailabilityContext { /** Everything a custom pane component receives, refreshed as the app changes. */ export interface ExtensionPaneProps { + /** Immutable delegated review metadata, or null for ordinary reviews. */ + readonly review: ExtensionReviewDescriptor | null; readonly files: readonly ExtensionDiffFile[]; readonly selectedFileId: string | null; readonly selectedHunkIndex: number | null; @@ -1294,9 +1298,13 @@ export interface ExtensionChangeRequestReviewDescriptor extends ExtensionReviewD readonly kind: "change-request"; /** Provider-local identifier, such as `#123`. */ readonly id: string; + /** Provider repository slug, such as `owner/repo`. */ + readonly repository?: string; readonly author?: string; readonly base?: string; readonly head?: string; + readonly state?: "open" | "closed" | "merged"; + readonly draft?: boolean; } /** Metadata for one reviewed commit. */ diff --git a/src/extensions/cliCommandRuntime.test.ts b/src/extensions/cliCommandRuntime.test.ts index dfa203926..d124dade5 100644 --- a/src/extensions/cliCommandRuntime.test.ts +++ b/src/extensions/cliCommandRuntime.test.ts @@ -266,9 +266,12 @@ describe("extension CLI command runtime", () => { title: "Add review metadata", url: "https://github.com/modem-dev/hunk/pull/123", id: "#123", + repository: "modem-dev/hunk", author: "octocat", base: "main", head: "metadata", + state: "open" as const, + draft: false, }; const execution = await runExtensionCliCommand({ extensionId: "tools", @@ -323,6 +326,11 @@ describe("extension CLI command runtime", () => { "credential-free HTTPS URL", ); await expect(execute({ ...valid, title: "x".repeat(2049) })).rejects.toThrow("byte limit"); + const request = { kind: "change-request", provider: "GitHub", title: "PR", id: "#1" }; + await expect(execute({ ...request, state: "pending" })).rejects.toThrow( + 'state must be "open", "closed", or "merged"', + ); + await expect(execute({ ...request, draft: "yes" })).rejects.toThrow("draft must be a boolean"); await expect( execute({ kind: "change-request", diff --git a/src/extensions/cliCommandRuntime.ts b/src/extensions/cliCommandRuntime.ts index d07c6ae3d..87a449887 100644 --- a/src/extensions/cliCommandRuntime.ts +++ b/src/extensions/cliCommandRuntime.ts @@ -188,6 +188,7 @@ const REVIEW_DESCRIPTOR_FIELD_LIMITS = Object.freeze({ title: 2 * 1024, url: 2 * 1024, id: 256, + repository: 512, author: 512, base: 512, head: 512, @@ -236,6 +237,20 @@ function copyOptionalDescriptorFields( return copied; } +/** Validate optional provider change-request state. */ +function validateChangeRequestState(value: unknown): "open" | "closed" | "merged" | undefined { + if (value === undefined || value === "open" || value === "closed" || value === "merged") { + return value; + } + throw new Error('delegate review state must be "open", "closed", or "merged"'); +} + +/** Validate an optional boolean descriptor field. */ +function validateOptionalBoolean(value: unknown, field: string): boolean | undefined { + if (value === undefined || typeof value === "boolean") return value; + throw new Error(`delegate review ${field} must be a boolean`); +} + /** Validate, copy, and deeply freeze provider-neutral delegated review metadata. */ function validateReviewDescriptor(value: unknown): ExtensionReviewDescriptor { if (typeof value !== "object" || value === null || Array.isArray(value)) { @@ -257,7 +272,7 @@ function validateReviewDescriptor(value: unknown): ExtensionReviewDescriptor { const common = ["kind", "provider", "title", "url"]; const kindFields = kind === "change-request" - ? ["id", "author", "base", "head"] + ? ["id", "repository", "author", "base", "head", "state", "draft"] : kind === "commit" ? ["revision", "author"] : ["base", "head"]; @@ -284,13 +299,17 @@ function validateReviewDescriptor(value: unknown): ExtensionReviewDescriptor { let descriptor: ExtensionReviewDescriptor; if (kind === "change-request") { + const state = validateChangeRequestState(candidate.state); + const draft = validateOptionalBoolean(candidate.draft, "draft"); descriptor = { kind, provider, title, ...(url === undefined ? {} : { url }), id: validateDescriptorString(candidate, "id", true)!, - ...copyOptionalDescriptorFields(candidate, ["author", "base", "head"]), + ...copyOptionalDescriptorFields(candidate, ["repository", "author", "base", "head"]), + ...(state === undefined ? {} : { state }), + ...(draft === undefined ? {} : { draft }), }; } else if (kind === "commit") { descriptor = { diff --git a/src/extensions/default/ui/index.test.ts b/src/extensions/default/ui/index.test.ts index c3472717f..3bde0e67f 100644 --- a/src/extensions/default/ui/index.test.ts +++ b/src/extensions/default/ui/index.test.ts @@ -3,8 +3,39 @@ import { getBundledUIRegistry } from "."; import { paneKey } from "../../apply"; describe("bundled UI registry", () => { - test("registers only the built-in files pane", () => { + test("registers the built-in files and delegated review info panes", () => { const panes = getBundledUIRegistry().panes; - expect(panes.map(paneKey)).toEqual(["hunk:files"]); + expect(panes.map(paneKey)).toEqual(["hunk:files", "hunk:review-info"]); + const reviewInfo = panes[1]!.pane; + expect(reviewInfo).toMatchObject({ + placement: "top", + defaultOpen: true, + height: { preferred: 2, min: 2, max: 2 }, + }); + expect( + reviewInfo.available?.({ + review: { + kind: "change-request", + provider: "GitHub", + title: "Title", + id: "#1", + }, + placement: "top", + files: [], + selectedFileId: null, + selectedHunkIndex: null, + currentLine: null, + }), + ).toBeTrue(); + expect( + reviewInfo.available?.({ + review: null, + placement: "top", + files: [], + selectedFileId: null, + selectedHunkIndex: null, + currentLine: null, + }), + ).toBeFalse(); }); }); diff --git a/src/extensions/default/ui/index.ts b/src/extensions/default/ui/index.ts index b5575104b..228879c3e 100644 --- a/src/extensions/default/ui/index.ts +++ b/src/extensions/default/ui/index.ts @@ -6,9 +6,13 @@ import { type ExtensionLoadIssue, type ExtensionRegistry, } from "../../types"; +import registerBundledReviewInfo from "./reviewInfo"; import registerBundledSidebar from "./sidebar"; -const factories: readonly [string, ExtensionFactory][] = [["files", registerBundledSidebar]]; +const factories: readonly [string, ExtensionFactory][] = [ + ["files", registerBundledSidebar], + ["review-info", registerBundledReviewInfo], +]; let cachedRegistry: ExtensionRegistry | undefined; /** Load bundled UI registrations through the public factory path, once per process. */ diff --git a/src/extensions/default/ui/reviewInfo/index.tsx b/src/extensions/default/ui/reviewInfo/index.tsx new file mode 100644 index 000000000..92c2fe3e5 --- /dev/null +++ b/src/extensions/default/ui/reviewInfo/index.tsx @@ -0,0 +1,42 @@ +import type { ReactNode } from "react"; +import type { ExtensionFactory } from "../../../types"; +import type { ExtensionPaneProps } from "../../../../extension-api/types"; +import { reviewInfoLines } from "./presentation"; + +export const BUNDLED_REVIEW_INFO_VIEW_ID = "review-info"; + +/** Render delegated change-request identity above the review without duplicating diff facts. */ +export function ReviewInfoPane({ review, theme, width }: ExtensionPaneProps): ReactNode { + if (review?.kind !== "change-request") return null; + const [primary, secondary] = reviewInfoLines(review, Math.max(0, width - 2)); + return ( + + {primary} + {secondary} + + ); +} + +/** Register the provider-neutral delegated change-request summary pane. */ +const registerBundledReviewInfo: ExtensionFactory = (hunk) => { + hunk.registerPane({ + id: BUNDLED_REVIEW_INFO_VIEW_ID, + title: "Review info", + placement: "top", + height: { preferred: 2, min: 2, max: 2 }, + defaultOpen: true, + available: ({ review }) => review?.kind === "change-request", + component: ReviewInfoPane, + }); +}; + +export default registerBundledReviewInfo; diff --git a/src/extensions/default/ui/reviewInfo/presentation.test.ts b/src/extensions/default/ui/reviewInfo/presentation.test.ts new file mode 100644 index 000000000..d7e526c98 --- /dev/null +++ b/src/extensions/default/ui/reviewInfo/presentation.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, test } from "bun:test"; +import { fitReviewInfoText, reviewInfoLines, sanitizeReviewInfoText } from "./presentation"; + +const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "Add delegated review metadata", + id: "#123", + repository: "modem-dev/hunk", + author: "octocat", + base: "main", + head: "feature/review-info", + state: "open" as const, +}; + +describe("review info presentation", () => { + test("formats state, reference, identity, and refs into two lines", () => { + expect(reviewInfoLines(review, 200)).toEqual([ + "OPEN · #123 · Add delegated review metadata", + "octocat · GitHub · modem-dev/hunk · main ← feature/review-info", + ]); + expect(reviewInfoLines({ ...review, draft: true, state: "closed" }, 200)[0]).toStartWith( + "DRAFT · #123", + ); + }); + + test("omits unknown state while preserving explicit draft identity", () => { + const { state: _state, ...withoutState } = review; + expect(reviewInfoLines(withoutState, 200)[0]).toBe("#123 · Add delegated review metadata"); + expect(reviewInfoLines({ ...withoutState, draft: false }, 200)[0]).toBe( + "#123 · Add delegated review metadata", + ); + expect(reviewInfoLines({ ...withoutState, draft: true }, 200)[0]).toBe( + "DRAFT · #123 · Add delegated review metadata", + ); + }); + + test("sanitizes control characters and collapses layout-changing whitespace", () => { + expect(sanitizeReviewInfoText("hello\n\u001b[31m world")).toBe("hello [31m world"); + const lines = reviewInfoLines({ ...review, title: "unsafe\r\ntitle" }, 200); + expect(lines[0]).toBe("OPEN · #123 · unsafe title"); + }); + + test("fits narrow and wide-character text deterministically", () => { + expect(fitReviewInfoText("abcdef", 6)).toBe("abcdef"); + expect(fitReviewInfoText("abcdef", 5)).toBe("abcd…"); + expect(fitReviewInfoText("界界界", 5)).toBe("界界…"); + expect(fitReviewInfoText("abcdef", 1)).toBe("…"); + expect(fitReviewInfoText("abcdef", 0)).toBe(""); + }); +}); diff --git a/src/extensions/default/ui/reviewInfo/presentation.ts b/src/extensions/default/ui/reviewInfo/presentation.ts new file mode 100644 index 000000000..4845de90b --- /dev/null +++ b/src/extensions/default/ui/reviewInfo/presentation.ts @@ -0,0 +1,50 @@ +import type { ExtensionChangeRequestReviewDescriptor } from "../../../../extension-api/types"; +import { measureClusterWidth, textClusters } from "../../../../ui/lib/text"; + +/** Collapse unsafe or layout-changing provider text into one deterministic terminal line. */ +export function sanitizeReviewInfoText(value: string): string { + return value + .replace(/[\u0000-\u001f\u007f-\u009f]/gu, " ") + .replace(/\s+/gu, " ") + .trim(); +} + +/** Fit sanitized text to an exact terminal-cell budget with one ellipsis when clipped. */ +export function fitReviewInfoText(value: string, width: number): string { + const safe = sanitizeReviewInfoText(value); + if (width <= 0) return ""; + const clusters = textClusters(safe); + if (clusters.reduce((sum, cluster) => sum + measureClusterWidth(cluster), 0) <= width) + return safe; + if (width === 1) return "…"; + + let used = 0; + let fitted = ""; + for (const cluster of clusters) { + const clusterWidth = measureClusterWidth(cluster); + if (used + clusterWidth > width - 1) break; + fitted += cluster; + used += clusterWidth; + } + return `${fitted}…`; +} + +/** Derive the two concise rows rendered by the bundled change-request pane. */ +export function reviewInfoLines( + review: ExtensionChangeRequestReviewDescriptor, + width: number, +): readonly [string, string] { + const state = review.draft ? "DRAFT" : review.state?.toUpperCase(); + const first = [state, review.id, review.title] + .filter((value): value is string => typeof value === "string" && value.length > 0) + .map(sanitizeReviewInfoText) + .filter(Boolean) + .join(" · "); + const refs = review.base && review.head ? `${review.base} ← ${review.head}` : undefined; + const second = [review.author, review.provider, review.repository, refs] + .filter((value): value is string => typeof value === "string" && value.length > 0) + .map(sanitizeReviewInfoText) + .filter(Boolean) + .join(" · "); + return [fitReviewInfoText(first, width), fitReviewInfoText(second, width)]; +} diff --git a/src/extensions/default/ui/sidebar/FileSidebars.tsx b/src/extensions/default/ui/sidebar/FileSidebars.tsx index a091bbac7..44c2681a4 100644 --- a/src/extensions/default/ui/sidebar/FileSidebars.tsx +++ b/src/extensions/default/ui/sidebar/FileSidebars.tsx @@ -17,8 +17,11 @@ import { FileListItem, } from "../../../../ui/components/panes/FileListItem"; -export type BuiltInSidebarProps = Omit & - Partial>; +export type BuiltInSidebarProps = Omit< + ExtensionPaneProps, + "placement" | "height" | "currentLine" | "review" +> & + Partial>; type FileSidebarVariantProps = Pick< BuiltInSidebarProps, diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 9c5cdd1d1..4779c21d0 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -455,6 +455,7 @@ export function App({ updatePaneResize, } = useExtensionPaneController({ availabilityContext: { + review: bootstrap.review ?? null, files: getRenderExtensionFileViews(), selectedFileId, selectedHunkIndex, @@ -1192,6 +1193,7 @@ export function App({ > { + test("the bundled review pane occupies exactly two rows only for delegated change requests", async () => { + const delegated = await createTestBootstrap(); + const ordinary = await createTestBootstrap(); + delete ordinary.bootstrap.review; + + const renderFrame = async (bootstrap: AppBootstrap) => { + let committed = false; + const setup = await testRender( + (committed = true)} />, + { width: 100, height: 12 }, + ); + try { + await flushUntil(setup, () => committed, "the review to mount"); + return setup.captureCharFrame(); + } finally { + await act(async () => setup.renderer.destroy()); + } + }; + + try { + const delegatedFrame = await renderFrame(delegated.bootstrap); + const ordinaryFrame = await renderFrame(ordinary.bootstrap); + const firstFileRow = (frame: string) => + frame.split("\n").findIndex((line) => line.includes("example.txt")); + expect(delegatedFrame).toContain("OPEN · #123 · Metadata pane"); + expect(ordinaryFrame).not.toContain("OPEN · #123 · Metadata pane"); + expect(firstFileRow(delegatedFrame)).toBe(firstFileRow(ordinaryFrame) + 2); + } finally { + rmSync(delegated.directory, { recursive: true, force: true }); + rmSync(ordinary.directory, { recursive: true, force: true }); + } + }); + test("manual refresh preserves the same patch metadata and unrelated reloads clear it durably", async () => { const fixture = await createTestBootstrap(); const committed: AppBootstrap[] = []; @@ -119,6 +156,12 @@ describe("delegated review metadata reloads", () => { try { await flushUntil(setup, () => committed.length === 1, "the delegated review to mount"); + const initialFrame = setup.captureCharFrame(); + expect(initialFrame).toContain("OPEN · #123 · Metadata pane"); + expect(initialFrame).toContain( + "octocat · GitHub · modem-dev/hunk · main ← feature/review-info", + ); + expect(initialFrame.indexOf("OPEN · #123")).toBeLessThan(initialFrame.indexOf("example.txt")); writeTestPatch(fixture.firstPatch, "manually refreshed"); await act(async () => setup.mockInput.typeText("r")); diff --git a/src/ui/components/panes/ExtensionPane.test.tsx b/src/ui/components/panes/ExtensionPane.test.tsx index 71348a7ad..3afe6cbc6 100644 --- a/src/ui/components/panes/ExtensionPane.test.tsx +++ b/src/ui/components/panes/ExtensionPane.test.tsx @@ -62,6 +62,45 @@ async function withPane( } } +describe("ExtensionPaneHost props", () => { + test("passes the immutable delegated review descriptor to the component", async () => { + const files = createTestFiles(); + const review = Object.freeze({ + kind: "change-request" as const, + provider: "GitHub", + title: "Metadata", + id: "#1", + }); + let received: ExtensionPaneProps["review"] | undefined; + await withPane( + { + received = props.review; + return ; + })} + review={review} + files={files} + fileViews={toReadOnlyFileViews(files)} + selectedFileId={null} + selectedHunkIndex={null} + theme={resolveTheme("github-dark-default", null)} + width={30} + height={10} + placement="top" + currentLine={null} + keybindings={TEST_KEYBINDINGS} + notify={() => {}} + onSelectFile={() => {}} + onSelectHunk={() => {}} + onRevealLine={() => "line"} + />, + async () => { + expect(received).toBe(review); + }, + ); + }); +}); + describe("ExtensionPaneHost actions", () => { test("refuses garbage hunk indices and clamps the rest into the file's range", async () => { const files = createTestFiles(); diff --git a/src/ui/components/panes/ExtensionPane.tsx b/src/ui/components/panes/ExtensionPane.tsx index 178509c88..651996dc8 100644 --- a/src/ui/components/panes/ExtensionPane.tsx +++ b/src/ui/components/panes/ExtensionPane.tsx @@ -77,6 +77,7 @@ class ExtensionPaneErrorBoundary extends Component< export interface ExtensionPaneHostProps { registered: RegisteredPane; + review?: ExtensionPaneProps["review"]; files: DiffFile[]; fileViews: ExtensionDiffFile[]; selectedFileId: string | null; @@ -98,6 +99,7 @@ export interface ExtensionPaneHostProps { /** Mount a public pane component inside the exact rectangle planned by the host. */ function ExtensionPaneHostView({ registered, + review = null, files, fileViews, selectedFileId, @@ -143,6 +145,7 @@ function ExtensionPaneHostView({ ); const View = registered.pane.component as (props: ExtensionPaneProps) => ReactNode; const viewProps: ExtensionPaneProps = { + review, files: fileViews, selectedFileId, selectedHunkIndex, @@ -209,6 +212,7 @@ export const ExtensionPaneHost = memo( ExtensionPaneHostView, (previous, next) => previous.registered === next.registered && + previous.review === next.review && previous.files.length === next.files.length && previous.files.every((file, index) => file === next.files[index]) && previous.selectedFileId === next.selectedFileId && diff --git a/src/ui/hooks/useExtensionPaneController.test.tsx b/src/ui/hooks/useExtensionPaneController.test.tsx index e77913f16..31008bb05 100644 --- a/src/ui/hooks/useExtensionPaneController.test.tsx +++ b/src/ui/hooks/useExtensionPaneController.test.tsx @@ -4,6 +4,7 @@ import { act, StrictMode, useLayoutEffect, useState } from "react"; import type { ExtensionPaneAvailabilityContext, ExtensionPanePlacement, + ExtensionReviewDescriptor, ExtensionPaneSize, } from "../../extension-api/types"; import { HUNK_FILES_PANE_KEY } from "../../extensions/extensionIds"; @@ -88,6 +89,7 @@ async function renderController({ let setExtensions!: (value: ReturnType) => void; let setCurrentLineCursor!: (value: { fileId: string; stableKey: string } | null) => void; let setResponsiveShowsSidebar!: (value: boolean) => void; + let setReview!: (value: ExtensionReviewDescriptor | null) => void; let setSelectedFileId!: (value: string | null) => void; let setSize!: (value: { width: number; height: number }) => void; const committedFreshOpen: boolean[] = []; @@ -105,15 +107,22 @@ async function renderController({ fileId: string; stableKey: string; } | null>(null); + const [review, updateReview] = useState(null); const [selectedFileId, updateSelectedFileId] = useState(null); const [size, updateSize] = useState({ width: initialWidth, height: initialHeight }); setCurrentLineCursor = updateCurrentLineCursor; setExtensions = updateExtensions; setResponsiveShowsSidebar = updateResponsiveShowsSidebar; + setReview = updateReview; setSelectedFileId = updateSelectedFileId; setSize = updateSize; controller = useExtensionPaneController({ - availabilityContext: { files: emptyFiles, selectedFileId, selectedHunkIndex: null }, + availabilityContext: { + review, + files: emptyFiles, + selectedFileId, + selectedHunkIndex: null, + }, bodyHeight: size.height, bodyWidth: size.width, canForceShowSidebar: size.width >= 71, @@ -163,6 +172,7 @@ async function renderController({ setCurrentLineCursor, setExtensions, setResponsiveShowsSidebar, + setReview, setSelectedFileId, setSize, settle, @@ -213,6 +223,48 @@ describe("useExtensionPaneController", () => { } }); + test("re-probes availability when only delegated review metadata changes", async () => { + let calls = 0; + const pane = registeredPane("meta", "review", { + placement: "top", + height: { preferred: 2, min: 2, max: 2 }, + defaultOpen: true, + available: ({ review }) => { + calls += 1; + return review?.kind === "change-request"; + }, + }); + const harness = await renderController({ extensions: loadResultWith([pane]) }); + const descriptor: ExtensionReviewDescriptor = { + kind: "change-request", + provider: "GitHub", + title: "Review metadata", + id: "#123", + }; + try { + expect(calls).toBe(1); + expect( + harness.current().paneLayout.panes.some(({ pane }) => pane.key === "meta:review"), + ).toBeFalse(); + + await act(async () => harness.setReview(descriptor)); + await harness.settle(); + expect(calls).toBe(2); + expect( + harness.current().paneLayout.panes.some(({ pane }) => pane.key === "meta:review"), + ).toBeTrue(); + + await act(async () => harness.setReview(null)); + await harness.settle(); + expect(calls).toBe(3); + expect( + harness.current().paneLayout.panes.some(({ pane }) => pane.key === "meta:review"), + ).toBeFalse(); + } finally { + await destroy(harness.setup); + } + }); + test("quarantines one failed replacement, warns once, and restores a fresh registration", async () => { let calls = 0; const broken = registeredPane("meta", "files", { diff --git a/src/ui/hooks/useExtensionPaneController.ts b/src/ui/hooks/useExtensionPaneController.ts index d00be0c5c..2692202f8 100644 --- a/src/ui/hooks/useExtensionPaneController.ts +++ b/src/ui/hooks/useExtensionPaneController.ts @@ -354,6 +354,7 @@ export function useExtensionPaneController({ : {}), }), [ + availabilityContext.review, availabilityContext.files, availabilityContext.selectedFileId, availabilityContext.selectedHunkIndex, diff --git a/src/ui/lib/extensionPanes.test.ts b/src/ui/lib/extensionPanes.test.ts index a89433ae8..38217a29f 100644 --- a/src/ui/lib/extensionPanes.test.ts +++ b/src/ui/lib/extensionPanes.test.ts @@ -50,8 +50,8 @@ function loadResultWith(panes: RegisteredPane[]) { describe("extension panes", () => { test("offers the bundled files pane before user panes", () => { const panes = buildSessionPanes(undefined); - expect(panes.map((pane) => pane.key)).toEqual([HUNK_FILES_PANE_KEY]); - expect(panes.map((pane) => pane.defaultOpen)).toEqual([true]); + expect(panes.map((pane) => pane.key)).toEqual([HUNK_FILES_PANE_KEY, "hunk:review-info"]); + expect(panes.map((pane) => pane.defaultOpen)).toEqual([true, true]); }); test("a replacement changes only the initial bundled files default", () => { @@ -60,6 +60,7 @@ describe("extension panes", () => { ); expect(panes.map((pane) => [pane.key, pane.defaultOpen])).toEqual([ [HUNK_FILES_PANE_KEY, false], + ["hunk:review-info", true], ["meta:files", true], ]); }); @@ -141,6 +142,37 @@ describe("extension panes", () => { expect(resolvePaneKey(panes, "other", "meta:extra")).toBe("meta:extra"); }); + test("anchors delegated review info above the review and beside the files pane", () => { + const panes = buildSessionPanes(undefined); + const probe = probeExtensionPaneAvailability({ + panes, + context: { + review: { kind: "change-request", provider: "GitHub", title: "Title", id: "#1" }, + files: [], + selectedFileId: null, + selectedHunkIndex: null, + }, + currentLine: null, + }); + const layout = planExtensionPanes({ + panes, + openKeys: panes + .filter((pane) => probe.available.has(pane.registered)) + .map((pane) => pane.key), + sizes: {}, + bodyWidth: 240, + bodyHeight: 30, + minReviewWidth: 40, + minReviewHeight: 5, + }); + const files = layout.panes.find((pane) => pane.pane.key === HUNK_FILES_PANE_KEY)!; + const info = layout.panes.find((pane) => pane.pane.key === "hunk:review-info")!; + expect(files.bounds).toEqual({ x: 0, y: 0, width: 38, height: 30 }); + expect(info.bounds).toEqual({ x: 39, y: 0, width: 201, height: 2 }); + expect(info.divider).toBeUndefined(); + expect(layout.reviewBounds).toEqual({ x: 39, y: 2, width: 201, height: 28 }); + }); + test("plans all four edges around one review rectangle", () => { const session = ( key: string, @@ -195,7 +227,12 @@ describe("extension panes", () => { }, }); const panes = buildSessionPanes(loadResultWith([registered])); - const context = { files: [], selectedFileId: null, selectedHunkIndex: null } as const; + const context = { + review: null, + files: [], + selectedFileId: null, + selectedHunkIndex: null, + } as const; const geometry = { panes, sizes: {}, @@ -227,6 +264,30 @@ describe("extension panes", () => { expect(availabilityCalls).toBe(callsBeforePending); }); + test("passes delegated review metadata into pane availability", () => { + const review = Object.freeze({ + kind: "change-request" as const, + provider: "GitHub", + title: "Metadata", + id: "#1", + }); + let received: unknown; + const registered = registeredPane("a", "review", { + available: (context) => { + received = context.review; + return context.review?.kind === "change-request"; + }, + }); + const panes = buildSessionPanes(loadResultWith([registered])); + const probe = probeExtensionPaneAvailability({ + panes, + context: { review, files: [], selectedFileId: null, selectedHunkIndex: null }, + currentLine: null, + }); + expect(received).toBe(review); + expect(probe.available.has(registered)).toBeTrue(); + }); + test("does not retain a same-key replacement by stale registration identity", () => { const previous = registeredPane("a", "detail", { currentLine: true, @@ -243,7 +304,7 @@ describe("extension panes", () => { const panes = buildSessionPanes(loadResultWith([replacement])); const probe = probeExtensionPaneAvailability({ panes, - context: { files: [], selectedFileId: null, selectedHunkIndex: null }, + context: { review: null, files: [], selectedFileId: null, selectedHunkIndex: null }, currentLine: null, retainCurrentLineRegistrations: new Set([previous]), }); @@ -264,7 +325,7 @@ describe("extension panes", () => { const panes = buildSessionPanes(loadResultWith([throwing, asyncPane])); const probe = probeExtensionPaneAvailability({ panes, - context: { files: [], selectedFileId: null, selectedHunkIndex: null }, + context: { review: null, files: [], selectedFileId: null, selectedHunkIndex: null }, currentLine: null, }); diff --git a/website/src/content/docs/docs/extend/extension-api.md b/website/src/content/docs/docs/extend/extension-api.md index 3028a5ee6..c9728d0fc 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -9,7 +9,7 @@ The extension factory receives one API object. Registration calls are only valid The API generation this Hunk speaks (currently `17`). Branch on it if you want one file to support several Hunk versions. Version 17 adds structured review metadata to delegated -patch commands; version 16 adds pane-wide +patch commands and projects it into pane availability and component props; version 16 adds pane-wide `onActivate`; version 15 added `{ side, line }` to opted-in pane `currentLine` paint; version 14 added structured two-revision VCS diff endpoints; version 13 added saved-note parent identities @@ -47,9 +47,12 @@ hunk.registerCliCommand( title: "Add structured review metadata", url: "https://github.com/acme/project/pull/123", id: "#123", + repository: "acme/project", author: "octocat", base: "main", head: "review-metadata", + state: "open", + draft: false, }, }; }, @@ -62,8 +65,9 @@ services, processes, and files. Return a validated exit status or delegate once to a built-in Hunk command. A delegated `patch` command may also carry a provider-neutral `review` descriptor whose exact shape is `change-request`, `commit`, or `comparison`. Hunk bounds all strings and the 4 KiB payload, rejects control characters, unknown fields, and unsafe URLs, -then copies and freezes it. `provider` and change-request `id` allow 256 bytes; `author`, `base`, -`head`, and `revision` allow 512; `title` and `url` allow 2 KiB. The descriptor remains app-bootstrap metadata rather than entering +then copies and freezes it. `provider` and change-request `id` allow 256 bytes; `repository`, +`author`, `base`, `head`, and `revision` allow 512; `title` and `url` allow 2 KiB. Change requests +may also carry `state` (`open`, `closed`, or `merged`) and boolean `draft`. The descriptor remains app-bootstrap metadata rather than entering changeset transforms or `ReviewDocumentV1`; same-file refreshes preserve it, while unrelated reloads clear it. Exit results and non-`patch` delegation cannot carry one. @@ -154,7 +158,7 @@ Full contract: [VCS adapters](/docs/extend/vcs-adapters/). ## `hunk.registerPane(pane)` -Render a React component on the `left`, `right`, `top`, or `bottom` of the review. Panes receive their dimensions, review state, actions, keybindings, and optional current-line paint (including `{ side, line }` when opted in). `registerSidebarView` remains a deprecated alias. +Render a React component on the `left`, `right`, `top`, or `bottom` of the review. Panes receive their dimensions, review state, actions, keybindings, and optional current-line paint (including `{ side, line }` when opted in). `props.review` and `available(context).review` expose immutable metadata supplied by a delegated patch command, or `null` for ordinary reviews. `registerSidebarView` remains a deprecated alias. Full contract: [Custom panes](/docs/extend/custom-sidebars/). From f5c7c3dfb84a7d0684a1915b438ad80d129188b8 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Fri, 4 Sep 2026 16:08:31 -0400 Subject: [PATCH 3/6] feat(session): publish delegated review metadata --- .changeset/calm-reviews-describe.md | 2 +- docs/agent-workflows.md | 4 + docs/browser-review-rebuild.md | 4 +- docs/extension-architecture.md | 5 +- docs/extensions.md | 4 +- scripts/source-boundaries.test.ts | 27 +++ src/app/session/registration.test.ts | 22 +++ src/app/session/registration.ts | 2 + src/core/reviewDescriptor.test.ts | 60 +++++++ src/core/reviewDescriptor.ts | 166 ++++++++++++++++++ src/extensions/cliCommandRuntime.test.ts | 4 +- src/extensions/cliCommandRuntime.ts | 162 +---------------- src/session/broker/projections.test.ts | 21 +++ src/session/broker/projections.ts | 3 + src/session/broker/wire.test.ts | 26 +++ src/session/broker/wire.ts | 7 +- src/session/protocol.ts | 2 +- src/session/protocolSchemas.test.ts | 34 +++- src/session/protocolSchemas.ts | 8 + src/session/types.ts | 11 +- .../docs/docs/extend/custom-sidebars.md | 3 +- .../content/docs/docs/extend/extension-api.md | 4 +- 22 files changed, 412 insertions(+), 169 deletions(-) create mode 100644 src/core/reviewDescriptor.test.ts create mode 100644 src/core/reviewDescriptor.ts diff --git a/.changeset/calm-reviews-describe.md b/.changeset/calm-reviews-describe.md index bc5d25c41..eeb4812c0 100644 --- a/.changeset/calm-reviews-describe.md +++ b/.changeset/calm-reviews-describe.md @@ -2,4 +2,4 @@ "hunkdiff": minor --- -Let extension CLI commands attach validated provider-neutral review metadata when they delegate a patch into Hunk, expose it to extension panes, and show delegated change-request identity in a concise built-in top pane. +Let extension CLI commands attach validated provider-neutral review metadata when they delegate a patch into Hunk, expose it to extension panes and live-session snapshots, and show delegated change-request identity in a concise built-in top pane. diff --git a/docs/agent-workflows.md b/docs/agent-workflows.md index 09ce109f1..5020d45bc 100644 --- a/docs/agent-workflows.md +++ b/docs/agent-workflows.md @@ -43,6 +43,10 @@ hunk session review --repo . --json - `get --repo .` confirms which live session matches the current repo - `review --json` returns the loaded file and hunk structure without dumping the full raw patch +When a CLI extension delegated the review, JSON list, context, and review outputs may also include a +bounded `review` descriptor with provider, title, URL, and kind-specific identity. It is descriptive +context only and does not add remote provider or reload capabilities. + Only add `--include-patch` when an agent truly needs raw unified diff text: ```bash diff --git a/docs/browser-review-rebuild.md b/docs/browser-review-rebuild.md index 979c28212..7d8d9ecce 100644 --- a/docs/browser-review-rebuild.md +++ b/docs/browser-review-rebuild.md @@ -63,7 +63,9 @@ untouched (rung 4). `reviewProtocol.ts`, broker `wire.ts` validation, broker review mirror, `reviewResourceCache` (bounded in-flight budget). Patch reconstruction for `hunk session review --include-patch` uses bounded-parallel loads from day one. Valuable without any web UI: agents get chunked, -digest-verified, memory-bounded resource access. The wire vocabulary is derived from +digest-verified, memory-bounded resource access. Optional extension-delegated review identity rides +in bounded registration metadata and projects into list, context, and review snapshots; it never +enters `ReviewDocumentV1` or creates a remote reload/provider capability. The wire vocabulary is derived from `ReviewIntent` (B12) and carries `expandedLineProof` (B10) and actor identity (G2) from its first version so the browser never needs a schema break. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index 03fb4f6db..2eb0c8aa7 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -71,7 +71,10 @@ are not rerun merely for the handoff. A delegated patch may attach a validated, provider-neutral review descriptor. Startup carries it beside the input on `AppBootstrap`; it does not enter the changeset transform pipeline or `ReviewDocumentV1`. The host preserves it only while the same file-backed patch identity reloads and clears it when a reload selects another input. -Headless delegation retires before executing the built-in plan. Terminal probing occurs only after +Session registration projects the same optional bounded descriptor into list, selected-context, and +review exports through strict app-wire validation. It remains registration metadata rather than a +`ReviewDocumentV1` field and does not imply any provider or remote-reload capability. Headless +delegation retires before executing the built-in plan. Terminal probing occurs only after the handler releases I/O. ## Host-served runtime modules diff --git a/docs/extensions.md b/docs/extensions.md index d4a2ac2b8..893d0ccf1 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -353,7 +353,9 @@ Change requests may also carry `state` (`open`, `closed`, or `merged`) and boole The descriptor describes the review source rather than its diff contents: it stays on the app bootstrap and does not enter changeset transforms or `ReviewDocumentV1`. Refreshing the same file-backed patch preserves it, including watch and manual refresh; an explicit reload to a -different patch path or input kind clears it. +different patch path or input kind clears it. Live-session list, context, and review JSON snapshots +project the same optional descriptor from registration metadata; it remains outside the semantic +review document and grants no remote reload or provider capability. Delegation cannot target another extension command or change extension bootstrap flags. Do not write stdout or read stdin before delegating; use stderr for diff --git a/scripts/source-boundaries.test.ts b/scripts/source-boundaries.test.ts index a0835172e..f5561c311 100644 --- a/scripts/source-boundaries.test.ts +++ b/scripts/source-boundaries.test.ts @@ -15,6 +15,7 @@ const REVIEW_MODEL_ROOT = join(CORE_ROOT, "review"); // not the tree — `extension-api/index.ts` is the runtime boundary and imports freely. const EXTENSION_API_TYPES_PATH = join(SRC_ROOT, "extension-api", "types.ts"); const REVIEW_PROTOCOL_PATH = join(SRC_ROOT, "session", "reviewProtocol.ts"); +const REVIEW_DESCRIPTOR_PATH = join(CORE_ROOT, "reviewDescriptor.ts"); const WEB_CLIENT_ROOT = join(SRC_ROOT, "web"); // Session modules a browser bundle imports verbatim: the wire schema, the HTTP surface's @@ -200,6 +201,16 @@ const EXTRACTED_DUPLICATE_SYMBOLS: ReadonlyArray<{ { file: "src/ui/lib/agentAnnotations.ts", symbol: "annotationOverlapsHunk", finding: "B1" }, { file: "src/ui/lib/agentAnnotations.ts", symbol: "getAnnotatedHunkIndices", finding: "B1" }, { file: "src/ui/lib/reviewState.ts", symbol: "buildReviewAnnotationIndex", finding: "B1" }, + { + file: "src/extensions/cliCommandRuntime.ts", + symbol: "validateReviewDescriptor", + finding: "delegated-review-descriptor", + }, + { + file: "src/extensions/cliCommandRuntime.ts", + symbol: "validateDescriptorString", + finding: "delegated-review-descriptor", + }, ]; describe("source architecture boundaries", () => { @@ -306,6 +317,22 @@ describe("shared review primitives seam", () => { ).toEqual([]); }); + test("keeps delegated review descriptor validation browser-safe", () => { + const escapedImports = importSpecifiers(REVIEW_DESCRIPTOR_PATH).flatMap((specifier) => { + const target = resolveImport(REVIEW_DESCRIPTOR_PATH, specifier); + return target && !isWithin(EXTENSION_API_TYPES_PATH, target) + ? [`${repoPath(REVIEW_DESCRIPTOR_PATH)} -> ${specifier}`] + : []; + }); + expect(escapedImports).toEqual([]); + const violations = [...reachableSourceFiles([REVIEW_DESCRIPTOR_PATH])].flatMap((path) => + valueImportSpecifiers(path) + .filter((specifier) => specifier.startsWith("node:") || specifier.startsWith("bun:")) + .map((specifier) => `${repoPath(path)} -> ${specifier}`), + ); + expect(violations).toEqual([]); + }); + test("keeps the browser-safe session modules browser-safe", () => { const violations = BROWSER_SAFE_SESSION_MODULES.filter(existsSync).flatMap((path) => importSpecifiers(path).flatMap((specifier) => { diff --git a/src/app/session/registration.test.ts b/src/app/session/registration.test.ts index e5e607f08..1e119b635 100644 --- a/src/app/session/registration.test.ts +++ b/src/app/session/registration.test.ts @@ -134,6 +134,28 @@ describe("session registration", () => { expect(updated.info.reviewCapabilityDigest).toBe(current.info.reviewCapabilityDigest); }); + test("registration create and update project delegated review metadata atomically", () => { + const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "Add session metadata", + id: "#123", + repository: "modem-dev/hunk", + state: "open" as const, + }; + const bootstrap = createBootstrap({ review }); + const created = createSessionRegistration(bootstrap, publish(bootstrap)); + expect(created.info.review).toEqual(review); + + const preserved = updateSessionRegistration(created, bootstrap, publish(bootstrap)); + expect(preserved.info.review).toEqual(review); + + const unrelated = createBootstrap(); + const cleared = updateSessionRegistration(preserved, unrelated, publish(unrelated)); + expect(cleared.info.review).toBeUndefined(); + expect(cleared.info.files).toHaveLength(1); + }); + test("registration advertises STML only for opted-in launches", () => { const experimental = createBootstrap({ input: { kind: "vcs", staged: false, options: { experimental: true } }, diff --git a/src/app/session/registration.ts b/src/app/session/registration.ts index 1460d69a8..bacf67637 100644 --- a/src/app/session/registration.ts +++ b/src/app/session/registration.ts @@ -97,6 +97,7 @@ export function createSessionRegistration( title: bootstrap.changeset.title, sourceLabel: bootstrap.changeset.sourceLabel, experimentalFeatures: resolveExperimentalFeatures(bootstrap.input.options), + ...(bootstrap.review ? { review: bootstrap.review } : {}), files: buildSessionFiles(publication), reviewCatalog: buildReviewCatalog(publication), // The verifier, not the secret: the daemon can check a presented capability and can @@ -121,6 +122,7 @@ export function updateSessionRegistration( title: bootstrap.changeset.title, sourceLabel: bootstrap.changeset.sourceLabel, experimentalFeatures: resolveExperimentalFeatures(bootstrap.input.options), + ...(bootstrap.review ? { review: bootstrap.review } : {}), files: buildSessionFiles(publication), reviewCatalog: buildReviewCatalog(publication), // The verifier, not the secret: the daemon can check a presented capability and can diff --git a/src/core/reviewDescriptor.test.ts b/src/core/reviewDescriptor.test.ts new file mode 100644 index 000000000..3d46a51d3 --- /dev/null +++ b/src/core/reviewDescriptor.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, test } from "bun:test"; +import { + parseExtensionReviewDescriptor, + validateExtensionReviewDescriptor, +} from "./reviewDescriptor"; + +const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "Review metadata", + id: "#123", + repository: "modem-dev/hunk", + state: "open" as const, +}; + +describe("delegated review descriptor validation", () => { + test("copies, freezes, and accepts every descriptor kind", () => { + const parsed = validateExtensionReviewDescriptor(review); + expect(parsed).toEqual(review); + expect(Object.isFrozen(parsed)).toBe(true); + expect( + validateExtensionReviewDescriptor({ + kind: "commit", + provider: "GitHub", + title: "Commit", + revision: "abc1234", + }), + ).toMatchObject({ kind: "commit", revision: "abc1234" }); + expect( + validateExtensionReviewDescriptor({ + kind: "comparison", + provider: "GitHub", + title: "Comparison", + base: "main", + head: "feature", + }), + ).toMatchObject({ kind: "comparison", base: "main", head: "feature" }); + }); + + test("rejects unknown fields, controls, insecure URLs, and byte overflows", () => { + for (const value of [ + { ...review, unknown: true }, + { ...review, title: "bad\u001b[31m" }, + { ...review, url: "http://github.com/modem-dev/hunk/pull/123" }, + { ...review, title: "é".repeat(1025) }, + { ...review, repository: "x".repeat(513) }, + { + ...review, + provider: "p".repeat(256), + title: "t".repeat(2 * 1024), + repository: "r".repeat(512), + author: "a".repeat(512), + base: "b".repeat(512), + head: "h".repeat(512), + }, + ]) { + expect(parseExtensionReviewDescriptor(value)).toBeNull(); + } + }); +}); diff --git a/src/core/reviewDescriptor.ts b/src/core/reviewDescriptor.ts new file mode 100644 index 000000000..2f8c170ff --- /dev/null +++ b/src/core/reviewDescriptor.ts @@ -0,0 +1,166 @@ +import type { ExtensionReviewDescriptor } from "../extension-api/types"; + +const REVIEW_DESCRIPTOR_TOTAL_BYTES = 4 * 1024; +const REVIEW_DESCRIPTOR_FIELD_LIMITS = Object.freeze({ + provider: 256, + title: 2 * 1024, + url: 2 * 1024, + id: 256, + repository: 512, + author: 512, + base: 512, + head: 512, + revision: 512, +}); + +/** Measure a public descriptor string in transport bytes rather than UTF-16 code units. */ +function descriptorByteLength(value: string) { + return new TextEncoder().encode(value).byteLength; +} + +/** Validate one bounded terminal-safe descriptor string. */ +function validateDescriptorString( + candidate: Record, + field: keyof typeof REVIEW_DESCRIPTOR_FIELD_LIMITS, + required: boolean, +): string | undefined { + if (!Object.prototype.hasOwnProperty.call(candidate, field)) { + if (!required) return undefined; + throw new Error(`delegate review ${field} must be a non-empty string`); + } + const value = candidate[field]; + if (value === undefined && !required) return undefined; + if (typeof value !== "string" || value.length === 0) { + throw new Error(`delegate review ${field} must be a non-empty string`); + } + if (/[\u0000-\u001f\u007f-\u009f]/u.test(value)) { + throw new Error(`delegate review ${field} cannot contain control characters`); + } + if (descriptorByteLength(value) > REVIEW_DESCRIPTOR_FIELD_LIMITS[field]) { + throw new Error(`delegate review ${field} exceeds its byte limit`); + } + return value; +} + +/** Copy only present optional fields after applying their individual bounds. */ +function copyOptionalDescriptorFields( + candidate: Record, + fields: readonly (keyof typeof REVIEW_DESCRIPTOR_FIELD_LIMITS)[], +): Record { + const copied: Record = {}; + for (const field of fields) { + const value = validateDescriptorString(candidate, field, false); + if (value !== undefined) copied[field] = value; + } + return copied; +} + +/** Validate optional provider change-request state. */ +function validateChangeRequestState(value: unknown): "open" | "closed" | "merged" | undefined { + if (value === undefined || value === "open" || value === "closed" || value === "merged") { + return value; + } + throw new Error('delegate review state must be "open", "closed", or "merged"'); +} + +/** Validate an optional boolean descriptor field. */ +function validateOptionalBoolean(value: unknown, field: string): boolean | undefined { + if (value === undefined || typeof value === "boolean") return value; + throw new Error(`delegate review ${field} must be a boolean`); +} + +/** Validate, copy, and deeply freeze provider-neutral delegated review metadata. */ +export function validateExtensionReviewDescriptor(value: unknown): ExtensionReviewDescriptor { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + throw new Error("delegate review must be an object"); + } + const prototype = Object.getPrototypeOf(value); + if (prototype !== Object.prototype && prototype !== null) { + throw new Error("delegate review must be a plain object"); + } + const candidate = value as Record; + const kind = candidate.kind; + if ( + !Object.prototype.hasOwnProperty.call(candidate, "kind") || + (kind !== "change-request" && kind !== "commit" && kind !== "comparison") + ) { + throw new Error('delegate review kind must be "change-request", "commit", or "comparison"'); + } + + const common = ["kind", "provider", "title", "url"]; + const kindFields = + kind === "change-request" + ? ["id", "repository", "author", "base", "head", "state", "draft"] + : kind === "commit" + ? ["revision", "author"] + : ["base", "head"]; + const allowed = new Set([...common, ...kindFields]); + const ownKeys = Reflect.ownKeys(value); + if (ownKeys.some((key) => typeof key !== "string" || !allowed.has(key))) { + throw new Error("delegate review contains unknown fields"); + } + + const provider = validateDescriptorString(candidate, "provider", true)!; + const title = validateDescriptorString(candidate, "title", true)!; + const url = validateDescriptorString(candidate, "url", false); + if (url !== undefined) { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + throw new Error("delegate review url must be a valid HTTPS URL"); + } + if (parsed.protocol !== "https:" || parsed.username || parsed.password) { + throw new Error("delegate review url must be a credential-free HTTPS URL"); + } + } + + let descriptor: ExtensionReviewDescriptor; + if (kind === "change-request") { + const state = validateChangeRequestState(candidate.state); + const draft = validateOptionalBoolean(candidate.draft, "draft"); + descriptor = { + kind, + provider, + title, + ...(url === undefined ? {} : { url }), + id: validateDescriptorString(candidate, "id", true)!, + ...copyOptionalDescriptorFields(candidate, ["repository", "author", "base", "head"]), + ...(state === undefined ? {} : { state }), + ...(draft === undefined ? {} : { draft }), + }; + } else if (kind === "commit") { + descriptor = { + kind, + provider, + title, + ...(url === undefined ? {} : { url }), + revision: validateDescriptorString(candidate, "revision", true)!, + ...copyOptionalDescriptorFields(candidate, ["author"]), + }; + } else { + descriptor = { + kind, + provider, + title, + ...(url === undefined ? {} : { url }), + base: validateDescriptorString(candidate, "base", true)!, + head: validateDescriptorString(candidate, "head", true)!, + }; + } + + const totalBytes = descriptorByteLength(JSON.stringify(descriptor)); + if (totalBytes > REVIEW_DESCRIPTOR_TOTAL_BYTES) { + throw new Error("delegate review exceeds the total byte limit"); + } + return Object.freeze(descriptor); +} + +/** Parse untrusted review metadata without throwing at a wire boundary. */ +export function parseExtensionReviewDescriptor(value: unknown): ExtensionReviewDescriptor | null { + try { + return validateExtensionReviewDescriptor(value); + } catch { + return null; + } +} diff --git a/src/extensions/cliCommandRuntime.test.ts b/src/extensions/cliCommandRuntime.test.ts index d124dade5..cba206af2 100644 --- a/src/extensions/cliCommandRuntime.test.ts +++ b/src/extensions/cliCommandRuntime.test.ts @@ -315,7 +315,9 @@ describe("extension CLI command runtime", () => { }); const valid = { kind: "commit", provider: "GitHub", title: "Commit", revision: "abc" }; - await expect(execute({ ...valid, extra: true })).rejects.toThrow("unknown fields"); + await expect(execute({ ...valid, extra: true })).rejects.toThrow( + 'Extension tools CLI command "tools" failed: delegate review contains unknown fields', + ); await expect(execute({ ...valid, title: "bad\u001b[31m" })).rejects.toThrow( "control characters", ); diff --git a/src/extensions/cliCommandRuntime.ts b/src/extensions/cliCommandRuntime.ts index 87a449887..edbc69126 100644 --- a/src/extensions/cliCommandRuntime.ts +++ b/src/extensions/cliCommandRuntime.ts @@ -1,9 +1,9 @@ +import { validateExtensionReviewDescriptor } from "../core/reviewDescriptor"; import { HunkUserError, isUserFacingError, toUserFacingError } from "../core/run/errors"; import type { ExtensionCliCommandContext, ExtensionCliCommandHandler, ExtensionCliCommandResult, - ExtensionReviewDescriptor, ExtensionCliWriter, } from "./types"; import { describeError } from "./runExtension"; @@ -182,162 +182,6 @@ function createTrackedStdin( }; } -const REVIEW_DESCRIPTOR_TOTAL_BYTES = 4 * 1024; -const REVIEW_DESCRIPTOR_FIELD_LIMITS = Object.freeze({ - provider: 256, - title: 2 * 1024, - url: 2 * 1024, - id: 256, - repository: 512, - author: 512, - base: 512, - head: 512, - revision: 512, -}); - -/** Measure a public descriptor string in transport bytes rather than UTF-16 code units. */ -function descriptorByteLength(value: string) { - return new TextEncoder().encode(value).byteLength; -} - -/** Validate one bounded terminal-safe descriptor string. */ -function validateDescriptorString( - candidate: Record, - field: keyof typeof REVIEW_DESCRIPTOR_FIELD_LIMITS, - required: boolean, -): string | undefined { - if (!Object.prototype.hasOwnProperty.call(candidate, field)) { - if (!required) return undefined; - throw new Error(`delegate review ${field} must be a non-empty string`); - } - const value = candidate[field]; - if (value === undefined && !required) return undefined; - if (typeof value !== "string" || value.length === 0) { - throw new Error(`delegate review ${field} must be a non-empty string`); - } - if (/[\u0000-\u001f\u007f-\u009f]/u.test(value)) { - throw new Error(`delegate review ${field} cannot contain control characters`); - } - if (descriptorByteLength(value) > REVIEW_DESCRIPTOR_FIELD_LIMITS[field]) { - throw new Error(`delegate review ${field} exceeds its byte limit`); - } - return value; -} - -/** Copy only present optional fields after applying their individual bounds. */ -function copyOptionalDescriptorFields( - candidate: Record, - fields: readonly (keyof typeof REVIEW_DESCRIPTOR_FIELD_LIMITS)[], -): Record { - const copied: Record = {}; - for (const field of fields) { - const value = validateDescriptorString(candidate, field, false); - if (value !== undefined) copied[field] = value; - } - return copied; -} - -/** Validate optional provider change-request state. */ -function validateChangeRequestState(value: unknown): "open" | "closed" | "merged" | undefined { - if (value === undefined || value === "open" || value === "closed" || value === "merged") { - return value; - } - throw new Error('delegate review state must be "open", "closed", or "merged"'); -} - -/** Validate an optional boolean descriptor field. */ -function validateOptionalBoolean(value: unknown, field: string): boolean | undefined { - if (value === undefined || typeof value === "boolean") return value; - throw new Error(`delegate review ${field} must be a boolean`); -} - -/** Validate, copy, and deeply freeze provider-neutral delegated review metadata. */ -function validateReviewDescriptor(value: unknown): ExtensionReviewDescriptor { - if (typeof value !== "object" || value === null || Array.isArray(value)) { - throw new Error("delegate review must be an object"); - } - const prototype = Object.getPrototypeOf(value); - if (prototype !== Object.prototype && prototype !== null) { - throw new Error("delegate review must be a plain object"); - } - const candidate = value as Record; - const kind = candidate.kind; - if ( - !Object.prototype.hasOwnProperty.call(candidate, "kind") || - (kind !== "change-request" && kind !== "commit" && kind !== "comparison") - ) { - throw new Error('delegate review kind must be "change-request", "commit", or "comparison"'); - } - - const common = ["kind", "provider", "title", "url"]; - const kindFields = - kind === "change-request" - ? ["id", "repository", "author", "base", "head", "state", "draft"] - : kind === "commit" - ? ["revision", "author"] - : ["base", "head"]; - const allowed = new Set([...common, ...kindFields]); - const ownKeys = Reflect.ownKeys(value); - if (ownKeys.some((key) => typeof key !== "string" || !allowed.has(key))) { - throw new Error("delegate review contains unknown fields"); - } - - const provider = validateDescriptorString(candidate, "provider", true)!; - const title = validateDescriptorString(candidate, "title", true)!; - const url = validateDescriptorString(candidate, "url", false); - if (url !== undefined) { - let parsed: URL; - try { - parsed = new URL(url); - } catch { - throw new Error("delegate review url must be a valid HTTPS URL"); - } - if (parsed.protocol !== "https:" || parsed.username || parsed.password) { - throw new Error("delegate review url must be a credential-free HTTPS URL"); - } - } - - let descriptor: ExtensionReviewDescriptor; - if (kind === "change-request") { - const state = validateChangeRequestState(candidate.state); - const draft = validateOptionalBoolean(candidate.draft, "draft"); - descriptor = { - kind, - provider, - title, - ...(url === undefined ? {} : { url }), - id: validateDescriptorString(candidate, "id", true)!, - ...copyOptionalDescriptorFields(candidate, ["repository", "author", "base", "head"]), - ...(state === undefined ? {} : { state }), - ...(draft === undefined ? {} : { draft }), - }; - } else if (kind === "commit") { - descriptor = { - kind, - provider, - title, - ...(url === undefined ? {} : { url }), - revision: validateDescriptorString(candidate, "revision", true)!, - ...copyOptionalDescriptorFields(candidate, ["author"]), - }; - } else { - descriptor = { - kind, - provider, - title, - ...(url === undefined ? {} : { url }), - base: validateDescriptorString(candidate, "base", true)!, - head: validateDescriptorString(candidate, "head", true)!, - }; - } - - const totalBytes = descriptorByteLength(JSON.stringify(descriptor)); - if (totalBytes > REVIEW_DESCRIPTOR_TOTAL_BYTES) { - throw new Error("delegate review exceeds the total byte limit"); - } - return Object.freeze(descriptor); -} - /** Validate and freeze the result returned by one extension CLI handler. */ function validateExtensionCliResult(result: unknown): ExtensionCliCommandResult { if (typeof result !== "object" || result === null || Array.isArray(result)) { @@ -376,7 +220,9 @@ function validateExtensionCliResult(result: unknown): ExtensionCliCommandResult throw new Error("delegate argv cannot change extension bootstrap flags"); } const review = - candidate.review === undefined ? undefined : validateReviewDescriptor(candidate.review); + candidate.review === undefined + ? undefined + : validateExtensionReviewDescriptor(candidate.review); return Object.freeze({ kind: "delegate", argv: Object.freeze([...argv]), diff --git a/src/session/broker/projections.test.ts b/src/session/broker/projections.test.ts index 28d5b850e..6c1e16909 100644 --- a/src/session/broker/projections.test.ts +++ b/src/session/broker/projections.test.ts @@ -42,6 +42,27 @@ describe("hunk session projections", () => { ); }); + test("projects delegated review metadata through list, context, and review snapshots", () => { + const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "Review session metadata", + id: "#123", + }; + const entry = { + registration: createTestSessionRegistration({ info: { review } }), + snapshot: createTestSessionSnapshot(), + }; + + const listed = buildListedHunkSession(entry); + expect(listed.review).toEqual(review); + expect(buildSelectedHunkSessionContext(listed).review).toEqual(review); + expect(buildHunkSessionReview(entry).review).toEqual(review); + + const ordinary = buildListedHunkSession(createEntry()); + expect(ordinary).not.toHaveProperty("review"); + }); + test("buildSelectedHunkSessionContext projects the current file and selected ranges", () => { const session = buildListedHunkSession({ registration: createTestSessionRegistration({ experimentalFeatures: ["stml"] }), diff --git a/src/session/broker/projections.ts b/src/session/broker/projections.ts index ef149f950..4f451adbf 100644 --- a/src/session/broker/projections.ts +++ b/src/session/broker/projections.ts @@ -76,6 +76,7 @@ export function buildListedHunkSession(entry: HunkSessionEntryLike): ListedSessi title: entry.registration.info.title, sourceLabel: entry.registration.info.sourceLabel, experimentalFeatures: entry.registration.info.experimentalFeatures ?? [], + ...(entry.registration.info.review ? { review: entry.registration.info.review } : {}), fileCount: entry.registration.info.files.length, files: entry.registration.info.files.map(summarizeReviewFile), snapshot: entry.snapshot, @@ -94,6 +95,7 @@ export function buildSelectedHunkSessionContext(session: ListedSession): Selecte repoRoot: session.repoRoot, inputKind: session.inputKind, experimentalFeatures: session.experimentalFeatures, + ...(session.review ? { review: session.review } : {}), selectedFile, selectedHunk: selectedFile ? { @@ -124,6 +126,7 @@ export function buildHunkSessionReview( repoRoot: entry.registration.repoRoot, inputKind: entry.registration.info.inputKind, experimentalFeatures: entry.registration.info.experimentalFeatures ?? [], + ...(entry.registration.info.review ? { review: entry.registration.info.review } : {}), selectedFile: selectedFile ? serializeReviewFile(selectedFile, includePatch) : null, selectedHunk: selectedFile ? (selectedFile.hunks[entry.snapshot.state.selectedHunkIndex] ?? null) diff --git a/src/session/broker/wire.test.ts b/src/session/broker/wire.test.ts index ba4ab3428..729b6b8c8 100644 --- a/src/session/broker/wire.test.ts +++ b/src/session/broker/wire.test.ts @@ -113,6 +113,32 @@ describe("hunk session wire parsing", () => { }); }); + test("registration accepts absent metadata and exact bounded delegated review descriptors", () => { + const absent = parseSessionRegistration(createRegistration([])); + expect(absent?.info.review).toBeUndefined(); + + const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "Review broker metadata", + id: "#123", + repository: "modem-dev/hunk", + state: "open" as const, + }; + const registration = createRegistration([]); + const input = { ...registration, info: { ...registration.info, review } }; + expect(parseSessionRegistration(input)?.info.review).toEqual(review); + + for (const review of [ + { ...input.info.review, unknown: true }, + { ...input.info.review, title: "x".repeat(2 * 1024 + 1) }, + { ...input.info.review, title: "bad\u001b[31m" }, + { ...input.info.review, url: "http://github.com/modem-dev/hunk/pull/123" }, + ]) { + expect(parseSessionRegistration({ ...input, info: { ...input.info, review } })).toBeNull(); + } + }); + test("registration rejects malformed or unknown experimental feature ids", () => { const registration = parseSessionRegistration({ registrationVersion: SESSION_BROKER_REGISTRATION_VERSION, diff --git a/src/session/broker/wire.ts b/src/session/broker/wire.ts index 62837f1c4..9be467b4f 100644 --- a/src/session/broker/wire.ts +++ b/src/session/broker/wire.ts @@ -18,6 +18,7 @@ import { parseHunkReviewResourceCatalog, } from "../reviewProtocol"; import { isReviewSha256Digest } from "../../core/review/validation"; +import { parseExtensionReviewDescriptor } from "../../core/reviewDescriptor"; import type { HunkSessionRegistration, HunkSessionSnapshot } from "../types"; import type { HunkSessionInfo, @@ -249,7 +250,7 @@ function parseHunkSessionInfo(value: unknown): HunkSessionInfo | null { const record = exactRecord( value, ["inputKind", "title", "sourceLabel", "files"], - ["experimentalFeatures", "reviewCatalog", "reviewCapabilityDigest"], + ["experimentalFeatures", "review", "reviewCatalog", "reviewCapabilityDigest"], ); if (!Array.isArray(record.files) || record.files.length > MAX_REGISTRATION_FILES) return null; @@ -285,12 +286,16 @@ function parseHunkSessionInfo(value: unknown): HunkSessionInfo | null { if (reviewCapabilityDigest !== undefined && !isReviewSha256Digest(reviewCapabilityDigest)) { return null; } + const review = + record.review === undefined ? undefined : parseExtensionReviewDescriptor(record.review); + if (record.review !== undefined && review === null) return null; return { inputKind, title, sourceLabel, experimentalFeatures: parseExperimentalFeatures(record.experimentalFeatures), + ...(review ? { review } : {}), files: files as SessionReviewFile[], ...(reviewCatalog ? { reviewCatalog } : {}), ...(reviewCapabilityDigest ? { reviewCapabilityDigest } : {}), diff --git a/src/session/protocol.ts b/src/session/protocol.ts index ac099447b..fb49da725 100644 --- a/src/session/protocol.ts +++ b/src/session/protocol.ts @@ -36,7 +36,7 @@ export const HUNK_SESSION_API_VERSION = 1; * builds can refresh an older daemon even when it still exposes the same API endpoints. Bump this * when daemon-forwarded payloads change, even if the supported action names stay stable. */ -export const HUNK_SESSION_DAEMON_VERSION = 12; +export const HUNK_SESSION_DAEMON_VERSION = 13; export type SessionDaemonAction = | "list" diff --git a/src/session/protocolSchemas.test.ts b/src/session/protocolSchemas.test.ts index 8c7a72335..585279378 100644 --- a/src/session/protocolSchemas.test.ts +++ b/src/session/protocolSchemas.test.ts @@ -1,6 +1,11 @@ import { describe, expect, test } from "bun:test"; import type { z } from "zod"; import type { CliInput } from "../core/run/commandInputs"; +import { + createTestSessionRegistration, + createTestSessionSnapshot, +} from "../../test/helpers/session-daemon-fixtures"; +import { buildListedHunkSession } from "./broker/projections"; import { HUNK_SESSION_API_VERSION, HUNK_SESSION_DAEMON_VERSION, @@ -39,7 +44,7 @@ void _dualSelectorIsNotCliInput; describe("session daemon request validation", () => { test("uses the daemon revision for structured two-endpoint reload payloads", () => { - expect(HUNK_SESSION_DAEMON_VERSION).toBe(12); + expect(HUNK_SESSION_DAEMON_VERSION).toBe(13); }); test("strictly parses cross-process capabilities", () => { @@ -180,6 +185,33 @@ describe("session daemon request validation", () => { } }); + test("accepts bounded delegated review metadata and rejects malformed descriptors", () => { + const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "Protocol metadata", + id: "#123", + }; + const session = buildListedHunkSession({ + registration: createTestSessionRegistration({ info: { review } }), + snapshot: createTestSessionSnapshot(), + }); + expect(parseSessionDaemonResponse("list", { sessions: [session] })).toEqual({ + sessions: [session], + }); + + expect(() => + parseSessionDaemonResponse("list", { + sessions: [{ ...session, review: { ...review, unknown: true } }], + }), + ).toThrow("Invalid Hunk session daemon response for list."); + expect(() => + parseSessionDaemonResponse("list", { + sessions: [{ ...session, review: { ...review, title: "x".repeat(2 * 1024 + 1) } }], + }), + ).toThrow("Invalid Hunk session daemon response for list."); + }); + test("accepts zero-based ranges in navigation responses", () => { expect( parseSessionDaemonResponse("navigate", { diff --git a/src/session/protocolSchemas.ts b/src/session/protocolSchemas.ts index 67f8787f6..a0de52aaa 100644 --- a/src/session/protocolSchemas.ts +++ b/src/session/protocolSchemas.ts @@ -1,6 +1,8 @@ import { z } from "zod"; import type { CliInput } from "../core/run/commandInputs"; import { EXPERIMENTAL_FEATURES } from "../core/run/experimental"; +import { parseExtensionReviewDescriptor } from "../core/reviewDescriptor"; +import type { ExtensionReviewDescriptor } from "../extension-api/types"; import { HUNK_SESSION_API_VERSION, HUNK_SESSION_DAEMON_VERSION, @@ -228,6 +230,9 @@ const positive = z.int().positive(); const lineRangeSchema = z.tuple([nonnegative, nonnegative]); const inputKindSchema = z.enum(["vcs", "show", "stash-show", "diff", "patch", "difftool"]); const experimentalFeaturesSchema = z.array(z.enum(EXPERIMENTAL_FEATURES)); +const reviewDescriptorSchema = z.custom( + (value) => parseExtensionReviewDescriptor(value) !== null, +); const terminalLocationSchema = z.strictObject({ source: z.string(), tty: z.string().optional(), @@ -316,6 +321,7 @@ const listedSessionSchema = z.strictObject({ title: z.string(), sourceLabel: z.string(), experimentalFeatures: experimentalFeaturesSchema.optional(), + review: reviewDescriptorSchema.optional(), fileCount: nonnegative, files: z.array(fileSummarySchema), snapshot: snapshotSchema, @@ -328,6 +334,7 @@ const selectedContextSchema = z.strictObject({ repoRoot: z.string().optional(), inputKind: inputKindSchema, experimentalFeatures: experimentalFeaturesSchema.optional(), + review: reviewDescriptorSchema.optional(), selectedFile: fileSummarySchema.nullable(), selectedHunk: selectedHunkSchema.nullable(), showAgentNotes: z.boolean(), @@ -342,6 +349,7 @@ const reviewSchema = z.strictObject({ repoRoot: z.string().optional(), inputKind: inputKindSchema, experimentalFeatures: experimentalFeaturesSchema.optional(), + review: reviewDescriptorSchema.optional(), selectedFile: reviewFileSchema.nullable(), selectedHunk: reviewHunkSchema.nullable(), showAgentNotes: z.boolean(), diff --git a/src/session/types.ts b/src/session/types.ts index d46f810af..122b76103 100644 --- a/src/session/types.ts +++ b/src/session/types.ts @@ -1,5 +1,9 @@ import type { ExperimentalFeature } from "../core/run/experimental"; -import type { ExtensionLineHighlightTone, SessionReloadReason } from "../extension-api/types"; +import type { + ExtensionLineHighlightTone, + ExtensionReviewDescriptor, + SessionReloadReason, +} from "../extension-api/types"; import type { CommentTargetInput, DiffSide } from "../core/liveComments"; import type { ReviewPublicationAddress } from "../core/review/generationOrder"; import type { CliInput, ReviewNoteSource } from "../core/run/commandInputs"; @@ -62,6 +66,8 @@ export interface HunkSessionInfo { title: string; sourceLabel: string; experimentalFeatures?: ExperimentalFeature[]; + /** Provider-neutral metadata attached by the CLI extension that delegated this review. */ + review?: ExtensionReviewDescriptor; files: SessionReviewFile[]; /** * The generation this registration projects, and every resource it offers. @@ -296,6 +302,7 @@ export interface ListedSession { title: string; sourceLabel: string; experimentalFeatures?: ExperimentalFeature[]; + review?: ExtensionReviewDescriptor; fileCount: number; files: SessionFileSummary[]; snapshot: HunkSessionSnapshot; @@ -309,6 +316,7 @@ export interface SelectedSessionContext { repoRoot?: string; inputKind: CliInput["kind"]; experimentalFeatures?: ExperimentalFeature[]; + review?: ExtensionReviewDescriptor; selectedFile: SessionFileSummary | null; selectedHunk: SelectedHunkSummary | null; showAgentNotes: boolean; @@ -325,6 +333,7 @@ export interface SessionReview { repoRoot?: string; inputKind: CliInput["kind"]; experimentalFeatures?: ExperimentalFeature[]; + review?: ExtensionReviewDescriptor; selectedFile: SessionReviewFile | null; selectedHunk: SessionReviewHunk | null; showAgentNotes: boolean; diff --git a/website/src/content/docs/docs/extend/custom-sidebars.md b/website/src/content/docs/docs/extend/custom-sidebars.md index 1f474db3d..0c801d613 100644 --- a/website/src/content/docs/docs/extend/custom-sidebars.md +++ b/website/src/content/docs/docs/extend/custom-sidebars.md @@ -47,7 +47,7 @@ export default function (hunk: HunkExtensionAPI) { `fraction` opts into live responsive sizing until the user drags the divider. It must be greater than `0` and at most `1`; Hunk rounds that fraction of the full host body width or height to a terminal cell, then applies `min`, `max`, and the space required by the review. `preferred` remains the fixed-cell target when `fraction` is omitted. A divider drag establishes a session-local cell override: later terminal shrink may clamp it temporarily, and expanding restores it. Panes without `fraction` retain their fixed preferred startup size. Folder extensions that use `fraction` should declare `"hunk": { "apiVersion": 12 }` in their manifest. -Use `defaultOpen` to open a pane initially, `replaces: "hunk:files"` to replace it (and override `defaultOpen`), or `available(context)` to hide it conditionally. One pane may replace each named target; the first registration owns that slot and later claims are skipped with a warning. `replaces` may also name another pane by its fully qualified `":"` key, and Hunk follows those replacement chains. +Use `defaultOpen` to open a pane initially, `replaces: "hunk:files"` to replace it (and override `defaultOpen`), or `available(context)` to hide it conditionally. One pane may replace each named target; the first registration owns that slot and later claims are skipped with a warning. `replaces` may also name another pane by its fully qualified `":"` key, and Hunk follows those replacement chains. Both `available(context)` and the mounted component receive `review`: immutable delegated review metadata, or `null` for ordinary reviews. Hunk's bundled `hunk:review-info` top pane uses it for change requests and consumes no rows when absent. Pane extensions that read `review` should declare `"hunk": { "apiVersion": 17 }` in their manifest so older Hunk versions refuse them cleanly instead of mounting with an incomplete prop contract. `onActivate()` observes a primary mouse press anywhere in the pane's content, including content nested in a ``. Use it to focus an extension-owned editor or update pane-local active state without adding mouse handlers to every row. Hunk does not stop propagation or prevent the press, so extension-local mouse behavior can continue. Other mouse buttons do not activate the pane. A thrown or rejected callback is contained and reported as an attributed warning. @@ -65,6 +65,7 @@ The component receives fresh props as the app changes: | Prop | What it is | | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `review` | immutable delegated review metadata (`change-request`, `commit`, or `comparison`), or `null` for ordinary reviews | | `files` | the visible reviewed files, review-stream order, filtered, frozen views (each carries `changeType`, `statsTruncated`, and `hunks` summaries beside the usual file fields) | | `selectedFileId` | the selected file, or `null` | | `selectedHunkIndex` | the selected hunk within that file, or `null` | diff --git a/website/src/content/docs/docs/extend/extension-api.md b/website/src/content/docs/docs/extend/extension-api.md index c9728d0fc..1e55114a9 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -69,7 +69,9 @@ then copies and freezes it. `provider` and change-request `id` allow 256 bytes; `author`, `base`, `head`, and `revision` allow 512; `title` and `url` allow 2 KiB. Change requests may also carry `state` (`open`, `closed`, or `merged`) and boolean `draft`. The descriptor remains app-bootstrap metadata rather than entering changeset transforms or `ReviewDocumentV1`; same-file refreshes preserve it, while unrelated -reloads clear it. Exit results and non-`patch` delegation cannot carry one. +reloads clear it. Live-session list, context, and review JSON snapshots project the same optional +bounded descriptor without granting provider or remote-reload capabilities. Exit results and +non-`patch` delegation cannot carry one. Delegation cannot follow stdout output or any stdin read, target another extension command, or change extension bootstrap flags. Built-ins and aliases cannot be shadowed; the first extension From f9ac8ddd4ad0515efb61cf3c574ae2ea9cebc07e Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Fri, 4 Sep 2026 16:24:36 -0400 Subject: [PATCH 4/6] feat(examples): describe delegated GitHub reviews --- docs/extensions.md | 9 +- examples/extensions/github-pr/README.md | 6 +- examples/extensions/github-pr/index.test.ts | 267 +++++++++++++++++++- examples/extensions/github-pr/index.ts | 214 ++++++++++++++-- examples/extensions/github-pr/package.json | 2 +- test/helpers/session-daemon-fixtures.ts | 3 +- test/pty/extensions-integration.test.ts | 93 ++++++- 7 files changed, 554 insertions(+), 40 deletions(-) diff --git a/docs/extensions.md b/docs/extensions.md index 893d0ccf1..4cb9e9a80 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -386,10 +386,11 @@ Both fields are collapsed to one sanitized line, so an extension cannot forge host output with newlines or escape sequences. The dependency-free [`github-pr` example](../examples/extensions/github-pr/) -is a complete network workflow built on this contract. It fetches a GitHub PR -diff without the `gh` CLI, writes a temporary patch with restrictive POSIX -modes (and inherited temporary-directory ACLs on Windows), delegates to the -built-in `patch` command, and removes the patch on extension shutdown. Run it +is a complete network workflow built on this contract. It fetches bounded GitHub PR metadata and +the diff without the `gh` CLI, attaches a `change-request` descriptor so the bundled review-info +pane shows the provider facts above the diff, writes a temporary patch with restrictive POSIX modes +(and inherited temporary-directory ACLs on Windows), delegates to the built-in `patch` command, and +removes the patch on extension shutdown. Run it from this checkout with: ```bash diff --git a/examples/extensions/github-pr/README.md b/examples/extensions/github-pr/README.md index c33b2cc26..94d477fe1 100644 --- a/examples/extensions/github-pr/README.md +++ b/examples/extensions/github-pr/README.md @@ -6,7 +6,7 @@ Review a GitHub pull request in Hunk with a generic extension-provided CLI comma hunk gh 123 ``` -The extension fetches the PR diff directly from GitHub's API, writes it to a temporary patch, and delegates once to Hunk's built-in `patch` command. It has no npm dependencies and does not require the `gh` CLI. +The extension fetches the PR metadata and diff directly from GitHub's API, writes the diff to a temporary patch, and delegates once to Hunk's built-in `patch` command. Hunk shows the provided title, author, state, repository, and base/head refs in its bundled review-info pane above the diff. The extension has no npm dependencies and does not require the `gh` CLI. ## Try it from this checkout @@ -45,7 +45,9 @@ Public repositories work anonymously within GitHub's API rate limits. For privat 1. `GH_TOKEN` 2. `GITHUB_TOKEN` when `GH_TOKEN` is absent -The token needs access to the target repository and may require organization SSO authorization. The extension only accepts `github.com` PR URLs and only sends credentials to the fixed `https://api.github.com` endpoint. Redirects are refused, response bodies are not copied into errors, and fetched diffs are bounded to 64 MiB. +The token needs access to the target repository and may require organization SSO authorization. The extension only accepts `github.com` PR URLs and only sends credentials to the fixed `https://api.github.com` endpoint. Redirects are refused, response bodies are not copied into errors, fetched metadata is bounded to 256 KiB, and fetched diffs are bounded to 64 MiB. + +Metadata and diff are separate GitHub requests. Hunk intentionally shows only stable provider facts and ref names; it does not claim that the metadata attests the exact bytes returned by the following diff request. PR patches can contain private source. On POSIX systems, the extension creates a mode-`0700` temporary directory and a mode-`0600` patch. Windows does not enforce those POSIX mode bits; the directory and patch inherit the ACL of the user's system temporary directory. The extension retains the patch while the delegated review can reload, then removes it during extension shutdown. Abrupt process termination may leave cleanup to the operating system's temporary-file policy. diff --git a/examples/extensions/github-pr/index.test.ts b/examples/extensions/github-pr/index.test.ts index a658d46e3..4b90bb957 100644 --- a/examples/extensions/github-pr/index.test.ts +++ b/examples/extensions/github-pr/index.test.ts @@ -12,9 +12,11 @@ import type { import { createGitHubPrExtension, fetchGitHubPullRequestDiff, + fetchGitHubPullRequestMetadata, type GitHubFetch, parseGitHubPrInvocation, parseGitHubPullRequestLocator, + parseGitHubPullRequestMetadata, parseGitHubRemoteRepository, readGitOrigin, resolveGitHubPullRequest, @@ -22,6 +24,34 @@ import { const temporaryDirectories: string[] = []; +/** Build the exact GitHub metadata fields the extension consumes. */ +function createTestPullRequestMetadata(overrides: Record = {}) { + return { + title: "Describe delegated reviews", + html_url: "https://github.com/modem-dev/hunk/pull/123", + user: { login: "octocat" }, + state: "open", + draft: false, + merged: false, + base: { ref: "main" }, + head: { ref: "feature/review-info" }, + ...overrides, + }; +} + +/** Mock the metadata and diff representations returned by one GitHub PR endpoint. */ +function createTestPullRequestFetch( + patch: string, + metadata: Record = createTestPullRequestMetadata(), +): GitHubFetch { + return (async (_url, init) => { + const accept = new Headers(init?.headers).get("accept"); + if (accept === "application/vnd.github+json") return Response.json(metadata); + if (accept === "application/vnd.github.v3.diff") return new Response(patch); + throw new Error(`Unexpected Accept header: ${accept}`); + }) as GitHubFetch; +} + /** Create one test-owned temporary directory. */ function createTestDirectory() { const directory = mkdtempSync(join(tmpdir(), "hunk-github-pr-test-")); @@ -155,6 +185,214 @@ describe("GitHub repository resolution", () => { }); }); +describe("GitHub pull-request metadata", () => { + test("requests bounded JSON metadata with the same token and redirect policy", async () => { + let requestUrl = ""; + let requestInit: RequestInit | undefined; + const review = await fetchGitHubPullRequestMetadata( + { owner: "modem-dev", repo: "hunk", number: "123" }, + new AbortController().signal, + { GH_TOKEN: "preferred", GITHUB_TOKEN: "fallback" }, + (async (url, init) => { + requestUrl = String(url); + requestInit = init; + return Response.json(createTestPullRequestMetadata()); + }) as GitHubFetch, + ); + + expect(requestUrl).toBe("https://api.github.com/repos/modem-dev/hunk/pulls/123"); + expect(requestInit?.redirect).toBe("manual"); + expect(new Headers(requestInit?.headers).get("accept")).toBe("application/vnd.github+json"); + expect(new Headers(requestInit?.headers).get("authorization")).toBe("Bearer preferred"); + expect(review).toEqual({ + kind: "change-request", + provider: "GitHub", + title: "Describe delegated reviews", + url: "https://github.com/modem-dev/hunk/pull/123", + id: "#123", + repository: "modem-dev/hunk", + author: "octocat", + base: "main", + head: "feature/review-info", + state: "open", + draft: false, + }); + }); + + test("reports a merged PR only when GitHub explicitly attests merged true", () => { + const target = { owner: "modem-dev", repo: "hunk", number: "123" }; + expect( + parseGitHubPullRequestMetadata( + createTestPullRequestMetadata({ state: "closed", merged: true }), + target, + ).state, + ).toBe("merged"); + expect( + parseGitHubPullRequestMetadata( + createTestPullRequestMetadata({ state: "closed", merged: false }), + target, + ).state, + ).toBe("closed"); + expect( + parseGitHubPullRequestMetadata( + createTestPullRequestMetadata({ state: "closed", merged: undefined }), + target, + ).state, + ).toBe("closed"); + const withoutDraft = createTestPullRequestMetadata(); + Reflect.deleteProperty(withoutDraft, "draft"); + expect(parseGitHubPullRequestMetadata(withoutDraft, target)).not.toHaveProperty("draft"); + }); + + test("rejects malformed provider fields and untrusted PR URLs", async () => { + const target = { owner: "modem-dev", repo: "hunk", number: "123" }; + for (const metadata of [ + null, + createTestPullRequestMetadata({ title: null }), + createTestPullRequestMetadata({ title: "forged\u001b[2Jtitle" }), + createTestPullRequestMetadata({ title: "x".repeat(2 * 1024 + 1) }), + createTestPullRequestMetadata({ user: { login: null } }), + createTestPullRequestMetadata({ state: "merged" }), + createTestPullRequestMetadata({ draft: "false" }), + createTestPullRequestMetadata({ base: { ref: "" } }), + createTestPullRequestMetadata({ base: { ref: "forged\nref" } }), + createTestPullRequestMetadata({ head: null }), + createTestPullRequestMetadata({ merged: "yes" }), + createTestPullRequestMetadata({ + html_url: "https://attacker.invalid/modem-dev/hunk/pull/123", + }), + createTestPullRequestMetadata({ html_url: "https://github.com/modem-dev/hunk/pull/124" }), + createTestPullRequestMetadata({ + html_url: "https://github.com/modem-dev/hunk/pull/123?token=secret", + }), + ]) { + expect(() => parseGitHubPullRequestMetadata(metadata, target)).toThrow( + "malformed pull-request metadata", + ); + } + + await expect( + fetchGitHubPullRequestMetadata( + target, + new AbortController().signal, + {}, + (async () => new Response("{not json")) as GitHubFetch, + ), + ).rejects.toThrow("malformed pull-request metadata"); + let declaredBodyCancelled = false; + const declaredOversizedBody = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("{}")); + }, + cancel() { + declaredBodyCancelled = true; + }, + }); + await expect( + fetchGitHubPullRequestMetadata( + target, + new AbortController().signal, + {}, + (async () => + new Response(declaredOversizedBody, { + headers: { "content-length": String(256 * 1024 + 1) }, + })) as GitHubFetch, + ), + ).rejects.toThrow("256 KiB"); + expect(declaredBodyCancelled).toBe(true); + + let streamCancelled = false; + const oversizedBody = new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array(128 * 1024)); + controller.enqueue(new Uint8Array(128 * 1024)); + controller.enqueue(new Uint8Array(1)); + }, + cancel() { + streamCancelled = true; + }, + }); + await expect( + fetchGitHubPullRequestMetadata( + target, + new AbortController().signal, + {}, + (async () => new Response(oversizedBody)) as GitHubFetch, + ), + ).rejects.toThrow("256 KiB"); + expect(streamCancelled).toBe(true); + }); + + test("keeps metadata HTTP, body, network, and token failures credential-safe", async () => { + const target = { owner: "private", repo: "repo", number: "7" }; + let malformedTokenFetches = 0; + await expect( + fetchGitHubPullRequestMetadata( + target, + new AbortController().signal, + { GH_TOKEN: "top-secret\nvalue" }, + (async () => { + malformedTokenFetches += 1; + return Response.json({}); + }) as GitHubFetch, + ), + ).rejects.toThrow("cannot be sent in an HTTP header"); + expect(malformedTokenFetches).toBe(0); + + for (const response of [ + new Response("secret response body", { status: 401 }), + new Response("secret response body", { status: 404 }), + new Response(null, { status: 302, headers: { location: "https://attacker.invalid" } }), + ]) { + try { + await fetchGitHubPullRequestMetadata( + target, + new AbortController().signal, + { GH_TOKEN: "top-secret-token" }, + (async () => response) as GitHubFetch, + ); + throw new Error("Expected metadata loading to fail."); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + expect(message).not.toContain("top-secret-token"); + expect(message).not.toContain("secret response body"); + } + } + + await expect( + fetchGitHubPullRequestMetadata(target, new AbortController().signal, {}, (async () => { + throw new Error("network internals"); + }) as GitHubFetch), + ).rejects.toThrow("could not be reached"); + + const failedBody = new ReadableStream({ + start(controller) { + controller.error(new Error("secret stream internals")); + }, + }); + try { + await fetchGitHubPullRequestMetadata( + target, + new AbortController().signal, + {}, + (async () => new Response(failedBody)) as GitHubFetch, + ); + throw new Error("Expected metadata stream loading to fail."); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + expect(message).toContain("stopped sending the pull-request metadata"); + expect(message).not.toContain("secret stream internals"); + } + + const controller = new AbortController(); + controller.abort(); + await expect( + fetchGitHubPullRequestMetadata(target, controller.signal, {}, (async () => + Response.json(createTestPullRequestMetadata())) as GitHubFetch), + ).rejects.toThrow("cancelled"); + }); +}); + describe("GitHub diff fetching", () => { test("sends the exact diff request with GH_TOKEN precedence", async () => { let requestUrl = ""; @@ -245,7 +483,7 @@ describe("GitHub PR extension lifecycle", () => { const extension = createGitHubPrExtension({ temporaryRoot, env: {}, - fetchImpl: (async () => new Response(patch, { status: 200 })) as GitHubFetch, + fetchImpl: createTestPullRequestFetch(patch), resolveOrigin: async () => "git@github.com:modem-dev/hunk.git", }); extension({ @@ -299,6 +537,19 @@ describe("GitHub PR extension lifecycle", () => { if (result.kind !== "delegate") throw new Error("Expected patch delegation."); expect(result.argv[0]).toBe("patch"); expect(result.argv.slice(2)).toEqual(["--pager"]); + expect(result.review).toEqual({ + kind: "change-request", + provider: "GitHub", + title: "Describe delegated reviews", + url: "https://github.com/modem-dev/hunk/pull/123", + id: "#123", + repository: "modem-dev/hunk", + author: "octocat", + base: "main", + head: "feature/review-info", + state: "open", + draft: false, + }); const patchPath = result.argv[1]!; expect(readFileSync(patchPath, "utf8")).toBe(patch); expect(stdoutWrites).toBe(0); @@ -317,7 +568,10 @@ describe("GitHub PR extension lifecycle", () => { createGitHubPrExtension({ temporaryRoot, env: {}, - fetchImpl: (async () => new Response("diff --git a/a b/a\n", { status: 200 })) as GitHubFetch, + fetchImpl: createTestPullRequestFetch( + "diff --git a/a b/a\n", + createTestPullRequestMetadata({ html_url: "https://github.com/owner/repo/pull/1" }), + ), })({ registerCliCommand( _command: ExtensionCliCommand, @@ -352,7 +606,10 @@ describe("GitHub PR extension lifecycle", () => { const extension = createGitHubPrExtension({ temporaryRoot, env: {}, - fetchImpl: (async () => new Response("diff --git a/a b/a\n", { status: 200 })) as GitHubFetch, + fetchImpl: createTestPullRequestFetch( + "diff --git a/a b/a\n", + createTestPullRequestMetadata({ html_url: "https://github.com/owner/repo/pull/1" }), + ), }); const registrations: Array<{ handler?: ExtensionCliCommandHandler; @@ -439,13 +696,13 @@ describe("GitHub PR extension lifecycle", () => { expect(fetches).toBe(0); }); - test("declares a dependency-free API-v10 folder extension", () => { + test("declares a dependency-free API-v17 folder extension", () => { const manifest = JSON.parse(readFileSync(join(import.meta.dir, "package.json"), "utf8")) as { dependencies?: unknown; devDependencies?: unknown; hunk?: { apiVersion?: number; extensions?: string[] }; }; - expect(manifest.hunk).toEqual({ extensions: ["./index.ts"], apiVersion: 10 }); + expect(manifest.hunk).toEqual({ extensions: ["./index.ts"], apiVersion: 17 }); expect(manifest.dependencies).toBeUndefined(); expect(manifest.devDependencies).toBeUndefined(); }); diff --git a/examples/extensions/github-pr/index.ts b/examples/extensions/github-pr/index.ts index d0e711120..6328e640c 100644 --- a/examples/extensions/github-pr/index.ts +++ b/examples/extensions/github-pr/index.ts @@ -5,12 +5,14 @@ import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { HunkExtensionUserError, + type ExtensionChangeRequestReviewDescriptor, type ExtensionCliCommandHandler, type ExtensionFactory, } from "hunkdiff/extension"; const GITHUB_API_ORIGIN = "https://api.github.com"; const MAX_DIFF_BYTES = 64 * 1024 * 1024; +const MAX_METADATA_BYTES = 256 * 1024; const REPOSITORY_PART = /^[A-Za-z0-9_.-]+$/; export const GITHUB_PR_HELP = `Usage: hunk gh [--repo ] [-- ] @@ -312,13 +314,21 @@ export async function resolveGitHubPullRequest( } /** Read a bounded response body so a remote server cannot exhaust process memory. */ -async function readBoundedResponse(response: Response, signal: AbortSignal): Promise { +async function readBoundedResponse( + response: Response, + signal: AbortSignal, + maximumBytes = MAX_DIFF_BYTES, + description = "pull-request diff", +): Promise { const declaredLength = Number(response.headers.get("content-length")); - if (Number.isFinite(declaredLength) && declaredLength > MAX_DIFF_BYTES) { - throw new HunkExtensionUserError("The pull-request diff exceeds the 64 MiB safety limit."); + if (Number.isFinite(declaredLength) && declaredLength > maximumBytes) { + await response.body?.cancel().catch(() => undefined); + throw new HunkExtensionUserError( + `The ${description} exceeds the ${formatByteLimit(maximumBytes)} safety limit.`, + ); } if (!response.body) { - throw new HunkExtensionUserError("GitHub returned an empty pull-request response."); + throw new HunkExtensionUserError(`GitHub returned an empty ${description} response.`); } const reader = response.body.getReader(); @@ -332,9 +342,11 @@ async function readBoundedResponse(response: Response, signal: AbortSignal): Pro const next = await reader.read(); if (next.done) break; total += next.value.byteLength; - if (total > MAX_DIFF_BYTES) { + if (total > maximumBytes) { await reader.cancel(); - throw new HunkExtensionUserError("The pull-request diff exceeds the 64 MiB safety limit."); + throw new HunkExtensionUserError( + `The ${description} exceeds the ${formatByteLimit(maximumBytes)} safety limit.`, + ); } chunks.push(next.value); } @@ -342,7 +354,10 @@ async function readBoundedResponse(response: Response, signal: AbortSignal): Pro if (signal.aborted) { throw new HunkExtensionUserError("GitHub pull-request loading was cancelled."); } - throw error; + if (error instanceof HunkExtensionUserError) throw error; + throw new HunkExtensionUserError(`GitHub stopped sending the ${description}.`, { + suggestions: ["Check network access and retry."], + }); } finally { reader.releaseLock(); } @@ -354,11 +369,38 @@ async function readBoundedResponse(response: Response, signal: AbortSignal): Pro offset += chunk.byteLength; } if (bytes.byteLength === 0) { - throw new HunkExtensionUserError("GitHub returned an empty pull-request diff."); + throw new HunkExtensionUserError(`GitHub returned an empty ${description}.`); } return bytes; } +/** Format one binary byte limit for fixed, readable user errors. */ +function formatByteLimit(bytes: number): string { + if (bytes % (1024 * 1024) === 0) return `${bytes / (1024 * 1024)} MiB`; + return `${bytes / 1024} KiB`; +} + +/** Build fixed GitHub headers without letting malformed credentials reach fetch. */ +function githubHeaders(env: NodeJS.ProcessEnv, accept: string): Headers { + const headers = new Headers({ + Accept: accept, + "User-Agent": "hunk-github-pr-extension", + "X-GitHub-Api-Version": "2022-11-28", + }); + const token = env.GH_TOKEN || env.GITHUB_TOKEN; + if (token) { + try { + headers.set("Authorization", `Bearer ${token}`); + } catch { + throw new HunkExtensionUserError( + "The configured GitHub token contains characters that cannot be sent in an HTTP header.", + { suggestions: ["Set GH_TOKEN or GITHUB_TOKEN to the token value without line breaks."] }, + ); + } + } + return headers; +} + /** Convert a non-success GitHub response into a fixed, credential-safe error. */ function githubResponseError(response: Response, target: ResolvedGitHubPullRequest) { const name = `${target.owner}/${target.repo}#${target.number}`; @@ -397,30 +439,140 @@ function githubResponseError(response: Response, target: ResolvedGitHubPullReque return new HunkExtensionUserError(`GitHub returned HTTP ${response.status} for ${name}.`); } -/** Fetch one GitHub pull-request diff without invoking the gh CLI. */ -export async function fetchGitHubPullRequestDiff( +/** Parse the exact GitHub PR fields shown in Hunk's delegated review pane. */ +export function parseGitHubPullRequestMetadata( + value: unknown, + target: ResolvedGitHubPullRequest, +): ExtensionChangeRequestReviewDescriptor { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + throw new HunkExtensionUserError("GitHub returned malformed pull-request metadata."); + } + const candidate = value as Record; + const readString = (container: Record, field: string, maximumBytes: number) => { + const fieldValue = container[field]; + if ( + typeof fieldValue !== "string" || + fieldValue.length === 0 || + /[\u0000-\u001f\u007f-\u009f]/u.test(fieldValue) || + new TextEncoder().encode(fieldValue).byteLength > maximumBytes + ) { + throw new HunkExtensionUserError("GitHub returned malformed pull-request metadata."); + } + return fieldValue; + }; + const readObject = (field: string) => { + const fieldValue = candidate[field]; + if (typeof fieldValue !== "object" || fieldValue === null || Array.isArray(fieldValue)) { + throw new HunkExtensionUserError("GitHub returned malformed pull-request metadata."); + } + return fieldValue as Record; + }; + + const title = readString(candidate, "title", 2 * 1024); + const pageUrl = readString(candidate, "html_url", 2 * 1024); + const user = readObject("user"); + const author = readString(user, "login", 512); + const base = readString(readObject("base"), "ref", 512); + const head = readString(readObject("head"), "ref", 512); + const state = candidate.state; + const draft = candidate.draft; + if ( + (state !== "open" && state !== "closed") || + (draft !== undefined && typeof draft !== "boolean") + ) { + throw new HunkExtensionUserError("GitHub returned malformed pull-request metadata."); + } + if (candidate.merged !== undefined && typeof candidate.merged !== "boolean") { + throw new HunkExtensionUserError("GitHub returned malformed pull-request metadata."); + } + + let parsedUrl: URL; + try { + parsedUrl = new URL(pageUrl); + } catch { + throw new HunkExtensionUserError("GitHub returned malformed pull-request metadata."); + } + const expectedPath = `/${target.owner}/${target.repo}/pull/${target.number}`.toLowerCase(); + if ( + parsedUrl.protocol !== "https:" || + parsedUrl.hostname.toLowerCase() !== "github.com" || + parsedUrl.port || + parsedUrl.username || + parsedUrl.password || + parsedUrl.search || + parsedUrl.hash || + parsedUrl.pathname.toLowerCase() !== expectedPath + ) { + throw new HunkExtensionUserError("GitHub returned malformed pull-request metadata."); + } + + return { + kind: "change-request", + provider: "GitHub", + title, + url: pageUrl, + id: `#${target.number}`, + repository: `${target.owner}/${target.repo}`, + author, + base, + head, + state: candidate.merged === true ? "merged" : state, + ...(typeof draft === "boolean" ? { draft } : {}), + }; +} + +/** Fetch bounded GitHub pull-request metadata for delegated review chrome. */ +export async function fetchGitHubPullRequestMetadata( target: ResolvedGitHubPullRequest, signal: AbortSignal, env: NodeJS.ProcessEnv = process.env, fetchImpl: GitHubFetch = fetch, -): Promise { - const token = env.GH_TOKEN || env.GITHUB_TOKEN; - const headers = new Headers({ - Accept: "application/vnd.github.v3.diff", - "User-Agent": "hunk-github-pr-extension", - "X-GitHub-Api-Version": "2022-11-28", - }); - if (token) { - try { - headers.set("Authorization", `Bearer ${token}`); - } catch { - throw new HunkExtensionUserError( - "The configured GitHub token contains characters that cannot be sent in an HTTP header.", - { suggestions: ["Set GH_TOKEN or GITHUB_TOKEN to the token value without line breaks."] }, - ); +): Promise { + const url = + `${GITHUB_API_ORIGIN}/repos/${encodeURIComponent(target.owner)}/` + + `${encodeURIComponent(target.repo)}/pulls/${target.number}`; + const headers = githubHeaders(env, "application/vnd.github+json"); + let response: Response; + try { + response = await fetchImpl(url, { + headers, + redirect: "manual", + signal, + }); + } catch { + if (signal.aborted) { + throw new HunkExtensionUserError("GitHub pull-request loading was cancelled."); } + throw new HunkExtensionUserError( + "GitHub could not be reached while loading pull-request metadata.", + { suggestions: ["Check network access and retry."] }, + ); } + if (!response.ok) throw githubResponseError(response, target); + const bytes = await readBoundedResponse( + response, + signal, + MAX_METADATA_BYTES, + "pull-request metadata", + ); + let value: unknown; + try { + value = JSON.parse(new TextDecoder("utf-8", { fatal: true }).decode(bytes)); + } catch { + throw new HunkExtensionUserError("GitHub returned malformed pull-request metadata."); + } + return parseGitHubPullRequestMetadata(value, target); +} + +/** Fetch one GitHub pull-request diff without invoking the gh CLI. */ +export async function fetchGitHubPullRequestDiff( + target: ResolvedGitHubPullRequest, + signal: AbortSignal, + env: NodeJS.ProcessEnv = process.env, + fetchImpl: GitHubFetch = fetch, +): Promise { + const headers = githubHeaders(env, "application/vnd.github.v3.diff"); const url = `${GITHUB_API_ORIGIN}/repos/${encodeURIComponent(target.owner)}/` + `${encodeURIComponent(target.repo)}/pulls/${target.number}`; @@ -502,6 +654,12 @@ export function createGitHubPrExtension( await ctx.stderr.write( `Fetching GitHub pull request ${target.owner}/${target.repo}#${target.number}…\n`, ); + const review = await fetchGitHubPullRequestMetadata( + target, + ctx.signal, + runtime.env, + runtime.fetchImpl, + ); const diff = await fetchGitHubPullRequestDiff( target, ctx.signal, @@ -531,7 +689,11 @@ export function createGitHubPrExtension( await discardPatch(); throw new HunkExtensionUserError("GitHub pull-request loading was cancelled."); } - return { kind: "delegate", argv: ["patch", patchPath, ...invocation.patchArgs] }; + return { + kind: "delegate", + argv: ["patch", patchPath, ...invocation.patchArgs], + review, + }; }; hunk.registerCliCommand( diff --git a/examples/extensions/github-pr/package.json b/examples/extensions/github-pr/package.json index 6b04b88e2..d20c413c8 100644 --- a/examples/extensions/github-pr/package.json +++ b/examples/extensions/github-pr/package.json @@ -7,6 +7,6 @@ "extensions": [ "./index.ts" ], - "apiVersion": 10 + "apiVersion": 17 } } diff --git a/test/helpers/session-daemon-fixtures.ts b/test/helpers/session-daemon-fixtures.ts index 2d90a548f..ccfe79bce 100644 --- a/test/helpers/session-daemon-fixtures.ts +++ b/test/helpers/session-daemon-fixtures.ts @@ -67,7 +67,7 @@ export function createTestSessionSnapshot( } export function createTestSessionRegistration( - overrides: Partial & + overrides: Partial> & Partial< Pick< HunkSessionRegistration["info"], @@ -97,6 +97,7 @@ export function createTestSessionRegistration( launchedAt: "2026-03-22T00:00:00.000Z", ...registrationOverrides, info: { + ...infoOverrides, inputKind: inputKind ?? infoOverrides?.inputKind ?? "vcs", title: title ?? infoOverrides?.title ?? "repo working tree", sourceLabel: sourceLabel ?? infoOverrides?.sourceLabel ?? "/repo", diff --git a/test/pty/extensions-integration.test.ts b/test/pty/extensions-integration.test.ts index b5fb49338..9f1d85b2d 100644 --- a/test/pty/extensions-integration.test.ts +++ b/test/pty/extensions-integration.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; -import { existsSync, readFileSync } from "node:fs"; +import { existsSync, readFileSync, writeFileSync } from "node:fs"; import { join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { createPtyHarness, dragMouse, lineIndexOf } from "./harness"; @@ -17,6 +17,9 @@ const REVIEW_SNAPSHOT_EXPORT_EXTENSION = resolve( const VIM_NAVIGATION_EXTENSION = resolve( fileURLToPath(new URL("../../examples/extensions/vim-navigation", import.meta.url)), ); +const GITHUB_PR_EXTENSION_ENTRY = resolve( + fileURLToPath(new URL("../../examples/extensions/github-pr/index.ts", import.meta.url)), +); /** Give PTY-backed startup, reloads, and redraws headroom on slower CI machines. */ setDefaultTimeout(30_000); @@ -51,6 +54,38 @@ const TRANSFORM_EXTENSION_SOURCE = `export default function (hunk) { } `; +/** The real GitHub PR example with fixed network responses for an end-to-end pane proof. */ +const DELEGATED_REVIEW_EXTENSION_SOURCE = `import { createGitHubPrExtension } from ${JSON.stringify(GITHUB_PR_EXTENSION_ENTRY)}; +const metadata = { + title: "Delegated pane proof", + html_url: "https://github.com/modem-dev/hunk/pull/123", + user: { login: "octocat" }, + state: "open", + draft: false, + merged: false, + base: { ref: "main" }, + head: { ref: "feature/pane" }, +}; +const patch = [ + "diff --git a/probe.txt b/probe.txt", + "--- a/probe.txt", + "+++ b/probe.txt", + "@@ -1 +1 @@", + "-before", + "+after", + "", +].join("\\n"); +export default createGitHubPrExtension({ + env: {}, + fetchImpl: async (_url, init) => { + const accept = new Headers(init?.headers).get("accept"); + if (accept === "application/vnd.github+json") return Response.json(metadata); + if (accept === "application/vnd.github.v3.diff") return new Response(patch); + throw new Error("Unexpected Accept header: " + accept); + }, +}); +`; + /** A repo-local extension that only speaks through ctx.notify on startup. */ const NOTIFY_EXTENSION_SOURCE = `export default function (hunk) { hunk.on("startup", (_payload, ctx) => { @@ -279,6 +314,62 @@ const DIALOG_EXTENSION_SOURCE = `export default function (hunk) { `; describe("PTY extensions", () => { + test("shows delegated change-request info above the review beside the files pane", async () => { + const configHome = harness.createIsolatedConfigHome(); + const fixture = harness.createRepoExtensionFixture(DELEGATED_REVIEW_EXTENSION_SOURCE); + const session = await harness.launchHunk({ + args: [ + "--extension", + join(fixture.dir, ".hunk", "extensions", "fixture.ts"), + "gh", + "123", + "--repo", + "modem-dev/hunk", + ], + cwd: fixture.dir, + cols: 140, + rows: 24, + env: { XDG_CONFIG_HOME: configHome }, + }); + + try { + const frame = await harness.waitForSnapshot( + session, + (text) => text.includes("OPEN · #123 · Delegated pane proof") && text.includes("after"), + 20_000, + ); + expect(frame).toContain("octocat · GitHub · modem-dev/hunk · main ← feature/pane"); + expect(lineIndexOf(frame, "OPEN · #123")).toBeLessThan(lineIndexOf(frame, "after")); + } finally { + session.close(); + } + }); + + test("does not reserve review-info rows for an ordinary patch", async () => { + const configHome = harness.createIsolatedConfigHome(); + const fixture = harness.createRepoExtensionFixture(NOTIFY_EXTENSION_SOURCE); + const patch = join(fixture.dir, "ordinary.diff"); + writeFileSync( + patch, + "diff --git a/probe.txt b/probe.txt\\n--- a/probe.txt\\n+++ b/probe.txt\\n@@ -1 +1 @@\\n-before\\n+ordinary\\n", + ); + const session = await harness.launchHunk({ + args: ["patch", patch, "--mode", "stack"], + cwd: fixture.dir, + cols: 140, + rows: 24, + env: { XDG_CONFIG_HOME: configHome }, + }); + + try { + const frame = await session.waitForText(/ordinary/, { timeout: 20_000 }); + expect(frame).not.toContain("OPEN · #123"); + expect(frame).not.toContain("Review info"); + } finally { + session.close(); + } + }); + test("trust prompt runs repo extensions after the user trusts the repository", async () => { const configHome = harness.createIsolatedConfigHome(); const fixture = harness.createRepoExtensionFixture(TRANSFORM_EXTENSION_SOURCE); From ae1996d92929a3941d522a9c8f22828529bd4fbc Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Fri, 4 Sep 2026 20:54:14 -0400 Subject: [PATCH 5/6] style(extensions): distinguish review info chrome --- .../default/ui/reviewInfo/index.test.tsx | 80 +++++++++++++++++++ .../default/ui/reviewInfo/index.tsx | 24 ++++-- 2 files changed, 97 insertions(+), 7 deletions(-) create mode 100644 src/extensions/default/ui/reviewInfo/index.test.tsx diff --git a/src/extensions/default/ui/reviewInfo/index.test.tsx b/src/extensions/default/ui/reviewInfo/index.test.tsx new file mode 100644 index 000000000..f01a4badf --- /dev/null +++ b/src/extensions/default/ui/reviewInfo/index.test.tsx @@ -0,0 +1,80 @@ +import { describe, expect, test } from "bun:test"; +import { testRender } from "@opentui/react/test-utils"; +import { act } from "react"; +import { capturedTestColorToHex } from "../../../../../test/helpers/test-color-helpers"; +import type { ExtensionPaneProps } from "../../../../extension-api/types"; +import { resolveTheme } from "../../../../ui/themes"; +import { ReviewInfoPane } from "."; +import { reviewInfoLines } from "./presentation"; + +const review = { + kind: "change-request" as const, + provider: "GitHub", + title: "A deliberately long delegated review title", + id: "#123", + repository: "modem-dev/hunk", + author: "octocat", + base: "main", + head: "feature/review-info", + state: "open" as const, +}; + +/** Return the background painted at one terminal column on every captured row. */ +function backgroundsAtColumn( + setup: Awaited>, + column: number, +): Array { + return setup.captureSpans().lines.map((line) => { + let spanStart = 0; + for (const span of line.spans) { + const spanEnd = spanStart + span.width; + if (spanStart <= column && column < spanEnd) return capturedTestColorToHex(span.bg); + spanStart = spanEnd; + } + return null; + }); +} + +describe("ReviewInfoPane", () => { + test("separates review chrome with an accent rail and panel background", async () => { + const theme = resolveTheme("github-dark-default", null); + const width = 30; + const setup = await testRender( + , + { width, height: 2 }, + ); + + try { + await act(async () => { + await setup.renderOnce(); + }); + expect(backgroundsAtColumn(setup, 0)).toEqual([ + theme.accent.toLowerCase(), + theme.accent.toLowerCase(), + ]); + expect(backgroundsAtColumn(setup, 1)).toEqual([ + theme.panel.toLowerCase(), + theme.panel.toLowerCase(), + ]); + expect(backgroundsAtColumn(setup, width - 1)).toEqual([ + theme.panel.toLowerCase(), + theme.panel.toLowerCase(), + ]); + expect(backgroundsAtColumn(setup, 1)).not.toContain(theme.panelAlt.toLowerCase()); + + const [primary, secondary] = reviewInfoLines(review, width - 3); + const frame = setup.captureCharFrame(); + expect(frame).toContain(` ${primary}`); + expect(frame).toContain(` ${secondary}`); + } finally { + setup.renderer.destroy(); + } + }); +}); diff --git a/src/extensions/default/ui/reviewInfo/index.tsx b/src/extensions/default/ui/reviewInfo/index.tsx index 92c2fe3e5..1b26f8707 100644 --- a/src/extensions/default/ui/reviewInfo/index.tsx +++ b/src/extensions/default/ui/reviewInfo/index.tsx @@ -8,20 +8,30 @@ export const BUNDLED_REVIEW_INFO_VIEW_ID = "review-info"; /** Render delegated change-request identity above the review without duplicating diff facts. */ export function ReviewInfoPane({ review, theme, width }: ExtensionPaneProps): ReactNode { if (review?.kind !== "change-request") return null; - const [primary, secondary] = reviewInfoLines(review, Math.max(0, width - 2)); + const [primary, secondary] = reviewInfoLines(review, Math.max(0, width - 3)); return ( - {primary} - {secondary} + + + {primary} + {secondary} + ); } From a45ce31178e4f7993fd016f4328005f7d8c335f1 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Fri, 4 Sep 2026 21:03:51 -0400 Subject: [PATCH 6/6] style(extensions): separate review info with top rule --- docs/extension-architecture.md | 6 +-- src/extensions/default/ui/index.test.ts | 2 +- .../default/ui/reviewInfo/index.test.tsx | 34 ++++++++++++++++- .../default/ui/reviewInfo/index.tsx | 37 +++++++++++-------- src/ui/AppHost.review-metadata.test.tsx | 4 +- src/ui/lib/extensionPanes.test.ts | 4 +- test/pty/extensions-integration.test.ts | 4 +- 7 files changed, 64 insertions(+), 27 deletions(-) diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index 2eb0c8aa7..8e9b6e773 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -99,9 +99,9 @@ the planner resolves it to an integer target before applying bounds and lets a session-local divider drag override that automatic size. `src/ui/components/panes/ExtensionPane.tsx` mounts panes with guarded actions, -immutable delegated review metadata, and failure containment. The fixed two-row -`hunk:review-info` top pane is available only for delegated change requests, so -ordinary reviews spend no geometry on it. `DiffPane` exposes optional current-line paint — the row +immutable delegated review metadata, and failure containment. The fixed three-row +`hunk:review-info` top pane uses one border row above two metadata rows and is available only for +delegated change requests, so ordinary reviews spend no geometry on it. `DiffPane` exposes optional current-line paint — the row painter plus the public `{ side, line }` address — without publishing Pierre rows, plans, cursor keys, or caches. Deprecated sidebar APIs normalize into this same registry and layout path. diff --git a/src/extensions/default/ui/index.test.ts b/src/extensions/default/ui/index.test.ts index 3bde0e67f..1d925701d 100644 --- a/src/extensions/default/ui/index.test.ts +++ b/src/extensions/default/ui/index.test.ts @@ -10,7 +10,7 @@ describe("bundled UI registry", () => { expect(reviewInfo).toMatchObject({ placement: "top", defaultOpen: true, - height: { preferred: 2, min: 2, max: 2 }, + height: { preferred: 3, min: 3, max: 3 }, }); expect( reviewInfo.available?.({ diff --git a/src/extensions/default/ui/reviewInfo/index.test.tsx b/src/extensions/default/ui/reviewInfo/index.test.tsx index f01a4badf..7728fb99f 100644 --- a/src/extensions/default/ui/reviewInfo/index.test.tsx +++ b/src/extensions/default/ui/reviewInfo/index.test.tsx @@ -44,11 +44,11 @@ describe("ReviewInfoPane", () => { {...({ review, width, - height: 2, + height: 3, theme, } as unknown as ExtensionPaneProps)} />, - { width, height: 2 }, + { width, height: 3 }, ); try { @@ -56,25 +56,55 @@ describe("ReviewInfoPane", () => { await setup.renderOnce(); }); expect(backgroundsAtColumn(setup, 0)).toEqual([ + theme.panel.toLowerCase(), theme.accent.toLowerCase(), theme.accent.toLowerCase(), ]); expect(backgroundsAtColumn(setup, 1)).toEqual([ theme.panel.toLowerCase(), theme.panel.toLowerCase(), + theme.panel.toLowerCase(), ]); expect(backgroundsAtColumn(setup, width - 1)).toEqual([ theme.panel.toLowerCase(), theme.panel.toLowerCase(), + theme.panel.toLowerCase(), ]); expect(backgroundsAtColumn(setup, 1)).not.toContain(theme.panelAlt.toLowerCase()); const [primary, secondary] = reviewInfoLines(review, width - 3); const frame = setup.captureCharFrame(); + expect(frame.split("\n")[0]).toBe("─".repeat(width)); + const borderSpan = setup.captureSpans().lines[0]?.spans.find((span) => span.width > 0); + expect(capturedTestColorToHex(borderSpan?.fg)).toBe(theme.border.toLowerCase()); expect(frame).toContain(` ${primary}`); expect(frame).toContain(` ${secondary}`); } finally { setup.renderer.destroy(); } }); + + test("keeps the border deterministic when no metadata text fits", async () => { + const theme = resolveTheme("github-dark-default", null); + const setup = await testRender( + , + { width: 1, height: 3 }, + ); + + try { + await act(async () => { + await setup.renderOnce(); + }); + expect(setup.captureCharFrame().split("\n").slice(0, 3)).toEqual(["─", " ", " "]); + } finally { + setup.renderer.destroy(); + } + }); }); diff --git a/src/extensions/default/ui/reviewInfo/index.tsx b/src/extensions/default/ui/reviewInfo/index.tsx index 1b26f8707..5478bc762 100644 --- a/src/extensions/default/ui/reviewInfo/index.tsx +++ b/src/extensions/default/ui/reviewInfo/index.tsx @@ -13,24 +13,29 @@ export function ReviewInfoPane({ review, theme, width }: ExtensionPaneProps): Re - - - {primary} - {secondary} + + {"─".repeat(Math.max(0, width))} + + + + + {primary} + {secondary} + ); @@ -42,7 +47,7 @@ const registerBundledReviewInfo: ExtensionFactory = (hunk) => { id: BUNDLED_REVIEW_INFO_VIEW_ID, title: "Review info", placement: "top", - height: { preferred: 2, min: 2, max: 2 }, + height: { preferred: 3, min: 3, max: 3 }, defaultOpen: true, available: ({ review }) => review?.kind === "change-request", component: ReviewInfoPane, diff --git a/src/ui/AppHost.review-metadata.test.tsx b/src/ui/AppHost.review-metadata.test.tsx index 0c47c8c3a..74d4aa194 100644 --- a/src/ui/AppHost.review-metadata.test.tsx +++ b/src/ui/AppHost.review-metadata.test.tsx @@ -108,7 +108,7 @@ async function flushUntil( } describe("delegated review metadata reloads", () => { - test("the bundled review pane occupies exactly two rows only for delegated change requests", async () => { + test("the bundled review pane occupies exactly three rows only for delegated change requests", async () => { const delegated = await createTestBootstrap(); const ordinary = await createTestBootstrap(); delete ordinary.bootstrap.review; @@ -134,7 +134,7 @@ describe("delegated review metadata reloads", () => { frame.split("\n").findIndex((line) => line.includes("example.txt")); expect(delegatedFrame).toContain("OPEN · #123 · Metadata pane"); expect(ordinaryFrame).not.toContain("OPEN · #123 · Metadata pane"); - expect(firstFileRow(delegatedFrame)).toBe(firstFileRow(ordinaryFrame) + 2); + expect(firstFileRow(delegatedFrame)).toBe(firstFileRow(ordinaryFrame) + 3); } finally { rmSync(delegated.directory, { recursive: true, force: true }); rmSync(ordinary.directory, { recursive: true, force: true }); diff --git a/src/ui/lib/extensionPanes.test.ts b/src/ui/lib/extensionPanes.test.ts index 38217a29f..72ff1de7c 100644 --- a/src/ui/lib/extensionPanes.test.ts +++ b/src/ui/lib/extensionPanes.test.ts @@ -168,9 +168,9 @@ describe("extension panes", () => { const files = layout.panes.find((pane) => pane.pane.key === HUNK_FILES_PANE_KEY)!; const info = layout.panes.find((pane) => pane.pane.key === "hunk:review-info")!; expect(files.bounds).toEqual({ x: 0, y: 0, width: 38, height: 30 }); - expect(info.bounds).toEqual({ x: 39, y: 0, width: 201, height: 2 }); + expect(info.bounds).toEqual({ x: 39, y: 0, width: 201, height: 3 }); expect(info.divider).toBeUndefined(); - expect(layout.reviewBounds).toEqual({ x: 39, y: 2, width: 201, height: 28 }); + expect(layout.reviewBounds).toEqual({ x: 39, y: 3, width: 201, height: 27 }); }); test("plans all four edges around one review rectangle", () => { diff --git a/test/pty/extensions-integration.test.ts b/test/pty/extensions-integration.test.ts index 9f1d85b2d..f48fb62e3 100644 --- a/test/pty/extensions-integration.test.ts +++ b/test/pty/extensions-integration.test.ts @@ -339,7 +339,9 @@ describe("PTY extensions", () => { 20_000, ); expect(frame).toContain("octocat · GitHub · modem-dev/hunk · main ← feature/pane"); - expect(lineIndexOf(frame, "OPEN · #123")).toBeLessThan(lineIndexOf(frame, "after")); + const infoLine = lineIndexOf(frame, "OPEN · #123"); + expect(frame.split("\n")[infoLine - 1]).toContain("─"); + expect(infoLine).toBeLessThan(lineIndexOf(frame, "after")); } finally { session.close(); }