From 60fe459313618e5eda9e269c95f1a250ce040305 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Sun, 30 Aug 2026 22:52:58 -0400 Subject: [PATCH 1/4] feat(extensions): move editor workflow into bundled extension --- .changeset/fuzzy-editors-dock.md | 5 + docs/extension-architecture.md | 15 ++- docs/extensions.md | 41 ++++-- skills/hunk-extensions/SKILL.md | 19 +-- src/extension-api/index.ts | 3 + src/extension-api/types.ts | 37 +++++- .../default/ui/editor/index.test.ts | 67 ++++++++++ src/extensions/default/ui/editor/index.ts | 32 +++++ src/extensions/default/ui/index.test.ts | 11 +- src/extensions/default/ui/index.ts | 42 +++--- src/extensions/types.ts | 3 + src/ui/App.tsx | 55 +++----- src/ui/AppHost.edit-in-editor.test.tsx | 44 ++++--- .../useExtensionWorkspaceControls.test.tsx | 121 +++++++++++++++++- src/ui/hooks/useExtensionWorkspaceControls.ts | 104 ++++++++++++++- src/ui/lib/extensionWorkspace.test.ts | 36 ++++++ src/ui/lib/extensionWorkspace.ts | 39 ++++++ src/ui/lib/openInEditor.test.ts | 28 ++-- src/ui/lib/openInEditor.ts | 34 +++-- .../content/docs/docs/extend/extension-api.md | 24 ++-- 20 files changed, 624 insertions(+), 136 deletions(-) create mode 100644 .changeset/fuzzy-editors-dock.md create mode 100644 src/extensions/default/ui/editor/index.test.ts create mode 100644 src/extensions/default/ui/editor/index.ts diff --git a/.changeset/fuzzy-editors-dock.md b/.changeset/fuzzy-editors-dock.md new file mode 100644 index 000000000..56c0c1058 --- /dev/null +++ b/.changeset/fuzzy-editors-dock.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Expose host-mediated editor launches to extensions and run Hunk's open-in-editor workflow as a bundled extension. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index e386a2ff4..79baacd19 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -20,11 +20,12 @@ 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 pane and editor command through + `runExtensionFactory` only where the interactive app resolves UI contributions. -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 open-in-editor workflow use the public +`registerVcsAdapter`, `registerPane`, and `registerCommand` 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 @@ -277,6 +278,12 @@ resolve reviewed file ids through the existing source fetcher, which retains ownership of caching and size limits. Missing or unreadable sources become `null`. +Editor requests also name reviewed file ids. The host resolves the current +working-tree path and source line and retains renderer suspend/resume and +process ownership in `openInEditor.ts`; reloadable inputs reconcile the review +after success. Hunk's own editor command is a bundled extension handler over +that same capability. + Writes are limited to reloadable working-tree reviews and reviewed paths inside the review root. App supplies the current input, unfiltered changeset, and root through refs so soft reloads update the policy inputs. The host verifies the diff --git a/docs/extensions.md b/docs/extensions.md index 549810a68..120e3ddb2 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -280,9 +280,10 @@ new instances and run that shutdown/startup pair around the replacement. ### `hunk.apiVersion` -The API generation this Hunk speaks (currently `15`). Branch on it if you want -one file to support several Hunk versions. Version 15 adds `{ side, line }` to -opted-in pane `currentLine` paint; version 14 added structured `rangeEndpoints` +The API generation this Hunk speaks (currently `16`). Branch on it if you want +one file to support several Hunk versions. Version 16 adds host-mediated editor +launches for reviewed files; 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 committed note-edit events; version 12 adds responsive fractional pane sizing; version 11 added the `"dim"` line-highlight tone; version 10 added generic top-level CLI commands; version 9 @@ -1681,14 +1682,16 @@ failure, that surfaces as a warning naming your extension. #### Workspace documents -`ctx.workspace` reads full documents from the current review and can replace an -eligible working-tree file. +`ctx.workspace` reads full documents from the current review, opens reviewed +files through Hunk's editor lifecycle, and can replace an eligible working-tree +file. -| Method | Result | -| -------------------------------------- | ------------------------------------------------- | -| `readDocument(fileId, "old" \| "new")` | The reviewed source text, or `null` | -| `canWriteDocument(fileId)` | Whether the review and file allow writes | -| `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | +| Method | Result | +| --------------------------------------------- | ------------------------------------------------- | +| `readDocument(fileId, "old" \| "new")` | The reviewed source text, or `null` | +| `openInEditor({ fileId, hunkIndex?, line? })` | `{ ok: true }` or `{ ok: false, reason, detail }` | +| `canWriteDocument(fileId)` | Whether the review and file allow writes | +| `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | A command can read, transform, and write a selected file: @@ -1711,6 +1714,16 @@ hunk.registerCommand({ id: "shout-headings", title: "Shout headings", key: "f7" }); ``` +`openInEditor` accepts only a reviewed file id, plus an optional zero-based +`hunkIndex` and one-based `{ side, line }` source address. Hunk resolves the +working-tree path and editor command itself. It maps old-side lines onto the +file on disk, suspends and resumes terminal editors, and queues a review reload +after success when the current input is reloadable. Missing `$EDITOR` +configuration or a missing reviewed file returns `unavailable`; process +failures and non-zero exits return `failed`. Malformed source addresses reject. +No consent prompt is shown because opening the user's configured editor does +not itself modify a file. + `readDocument` returns the exact source represented by the review, not the file's patch. It works for every review kind. For example, the `"new"` side in `hunk show HEAD` is the file at that commit, not the working-tree file. It @@ -1792,10 +1805,10 @@ showing — no keypress required. Dialog calls made before the mounted app is ready resolve to their cancel value with a warning rather than opening later. Controls retained across a review or extension-registry replacement expire: navigation and pane mutations warn and do nothing, dialogs resolve to their -normal cancel value, and workspace reads or not-yet-started writes return -`null`/`unavailable` instead of acting on replacement content. Once a consented -filesystem write starts, it reports its actual outcome and success reconciles -the review then active. +normal cancel value, and workspace reads, editor launches, or not-yet-started +writes return `null`/`unavailable` instead of acting on replacement content. +Once a consented filesystem write starts, it reports its actual outcome and +success reconciles the review then active. | Event | Payload | When | | ---------------------- | ----------------------- | --------------------------------------------------------- | diff --git a/skills/hunk-extensions/SKILL.md b/skills/hunk-extensions/SKILL.md index 9bb72282d..a8f2188b1 100644 --- a/skills/hunk-extensions/SKILL.md +++ b/skills/hunk-extensions/SKILL.md @@ -110,7 +110,7 @@ bad or duplicate id is skipped with a startup notice. | Coordinate with another loaded extension | `hunk.events.emit` / `hunk.events.on` | | Read user-supplied settings | `hunk.config` (`[extension.]` table) | | Snapshot stable files and every saved review note | `ctx.review.snapshot()` in a command | -| Branch on the API generation (currently `15`) | `hunk.apiVersion` | +| Branch on the API generation (currently `16`) | `hunk.apiVersion` | Registration is only valid while the factory runs — Hunk seals the API object afterwards. @@ -161,7 +161,8 @@ transform — gets `ctx.cwd` and `ctx.notify(message, type?)`. A file view's `ctx.keyboardModes` (enter/exit/probe this extension's session modes), `ctx.review` (deeply immutable snapshots of stable files and complete saved store notes), `ctx.dialogs` (`confirm`/`select`/`input`, queued and attributed), and - `ctx.workspace` (`readDocument`, `canWriteDocument`, `writeDocument` with consent). + `ctx.workspace` (`readDocument`, host-mediated `openInEditor`, + `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`. @@ -215,7 +216,8 @@ Most extension bugs are one of these: navigation after awaiting a selector; file filters can still refuse hidden targets. - **Retained review controls expire on reload.** An old handler cannot control replacement content: pane/navigation calls become inert, dialogs cancel, and - workspace reads or not-yet-started writes return `null`/`unavailable`. A + workspace reads, editor launches, or not-yet-started writes return + `null`/`unavailable`. A consented write already in progress reports its real outcome, holds graceful exit until it settles, and reconciles the active review on success. `shutdown` runs after revocation, so use it only @@ -252,11 +254,12 @@ Most extension bugs are one of these: - **Failures are contained, not sandboxed.** A throwing factory is rolled back to zero registrations and a throwing handler is a warning naming the extension — containment against bugs, not against code that should not have been loaded. -- **The API touches nothing outside the review.** No clipboard, no filesystem, no - process surface beyond `ctx.workspace` — an extension is ordinary code, so shell - out for the rest. Never write to stdout: the renderer owns it. For the same - reason `hunk.log` is collected as diagnostics and printed nowhere; `ctx.notify` - is how a user hears from you. +- **The API touches nothing outside the review.** No clipboard, no arbitrary + filesystem path, and no arbitrary process surface: `ctx.workspace` reads, + writes, or opens only reviewed file ids. An extension is ordinary code, so + shell out for unsupported integrations. Never write to stdout: the renderer + owns it. For the same reason `hunk.log` is collected as diagnostics and printed + nowhere; `ctx.notify` is how a user hears from you. - **`HunkExtensionUserError`** (detected structurally by `name`) buys the full treatment — message plus `suggestions`, no stack trace — only from a VCS adapter operation, which is where Hunk formats it for the CLI. From a command or event diff --git a/src/extension-api/index.ts b/src/extension-api/index.ts index dbc86e2fe..b588f18e6 100644 --- a/src/extension-api/index.ts +++ b/src/extension-api/index.ts @@ -155,6 +155,9 @@ export type { ExtensionVcsWatchTarget, ExtensionVcsWatchTargetSource, ExtensionWorkspace, + ExtensionWorkspaceEditorLine, + ExtensionWorkspaceOpenInEditorRequest, + ExtensionWorkspaceOpenInEditorResult, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, HunkExtensionAPI, diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index 345d29820..5ac231f00 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 = 15; +export const HUNK_EXTENSION_API_VERSION = 16; export type HunkExtensionApiVersion = typeof HUNK_EXTENSION_API_VERSION; export type ExtensionNotifyType = "info" | "warning" | "error"; @@ -1675,6 +1675,28 @@ export type ExtensionWorkspaceWriteResult = | { ok: true } | { ok: false; reason: "unavailable" | "cancelled" | "failed"; detail: string }; +/** One reviewed source line an editor should reveal. */ +export interface ExtensionWorkspaceEditorLine { + side: ExtensionFileSide; + /** One-based source line number on `side`. */ + line: number; +} + +/** A reviewed file and optional source location an extension asks Hunk to open. */ +export interface ExtensionWorkspaceOpenInEditorRequest { + /** The reviewed file to open, by its `ExtensionDiffFile.id`. */ + fileId: string; + /** Hunk index used to map an old-side line onto the working-tree file. */ + hunkIndex?: number; + /** Exact source line to prefer over the hunk's first line. */ + line?: ExtensionWorkspaceEditorLine; +} + +/** How a host-mediated editor launch settled. */ +export type ExtensionWorkspaceOpenInEditorResult = + | { ok: true } + | { ok: false; reason: "unavailable" | "failed"; detail: string }; + /** * The reviewed files as whole documents, read and written through the host. * @@ -1729,6 +1751,19 @@ export interface ExtensionWorkspace { * the pairing this exists for. */ readDocument(fileId: string, side: ExtensionFileSide): Promise; + /** + * Open a reviewed file in the user's `$EDITOR` through Hunk's terminal lifecycle. + * + * The extension names a reviewed file id and source location, never a filesystem + * path or process. Hunk resolves the working-tree path, maps old-side lines onto + * the file on disk, suspends and resumes terminal editors, and reloads a reloadable + * review after a successful launch. Missing files or editor configuration resolve + * `"unavailable"`; launch and non-zero-exit failures resolve `"failed"`. Malformed + * ids, hunk indexes, sides, or line numbers reject as programming errors. + */ + openInEditor( + request: ExtensionWorkspaceOpenInEditorRequest, + ): Promise; /** * Whether `writeDocument` could currently succeed for this reviewed file. * diff --git a/src/extensions/default/ui/editor/index.test.ts b/src/extensions/default/ui/editor/index.test.ts new file mode 100644 index 000000000..a06b93fc2 --- /dev/null +++ b/src/extensions/default/ui/editor/index.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, mock, test } from "bun:test"; +import type { ExtensionCommandContext } from "hunkdiff/extension"; +import { getBundledUIRegistry } from ".."; +import { BUNDLED_EDITOR_COMMAND_FULL_ID } from "."; + +/** Return the editor registration from the process-static bundled UI registry. */ +function getBundledEditorCommand() { + const registered = getBundledUIRegistry().commands.find( + ({ extensionId, command }) => `${extensionId}.${command.id}` === BUNDLED_EDITOR_COMMAND_FULL_ID, + ); + if (!registered) throw new Error("Bundled editor command is missing."); + return registered; +} + +describe("bundled editor extension", () => { + test("registers the shared Hunk command identity without owning its host key shell", () => { + const registered = getBundledEditorCommand(); + + expect(registered.extensionId).toBe("hunk"); + expect(registered.command).toEqual({ + id: "review.editSelectedFile", + title: "Open the selected file in your editor", + }); + }); + + test("forwards the frozen review selection to the host editor capability", async () => { + const openInEditor = mock(async () => ({ ok: true as const })); + const notify = mock(() => {}); + const context = { + notify, + selection: { + file: { id: "alpha" }, + hunkIndex: 2, + currentLine: { side: "old", line: 17 }, + }, + workspace: { openInEditor }, + } as unknown as ExtensionCommandContext; + + await getBundledEditorCommand().handler(context); + + expect(openInEditor).toHaveBeenCalledWith({ + fileId: "alpha", + hunkIndex: 2, + line: { side: "old", line: 17 }, + }); + expect(notify).not.toHaveBeenCalled(); + }); + + test("surfaces host refusals without attempting its own process or path handling", async () => { + const notify = mock(() => {}); + const context = { + notify, + selection: { file: { id: "alpha" }, hunkIndex: null, currentLine: null }, + workspace: { + openInEditor: async () => ({ + ok: false as const, + reason: "unavailable" as const, + detail: "$EDITOR is not set.", + }), + }, + } as unknown as ExtensionCommandContext; + + await getBundledEditorCommand().handler(context); + + expect(notify).toHaveBeenCalledWith("$EDITOR is not set.", "warning"); + }); +}); diff --git a/src/extensions/default/ui/editor/index.ts b/src/extensions/default/ui/editor/index.ts new file mode 100644 index 000000000..436db65c9 --- /dev/null +++ b/src/extensions/default/ui/editor/index.ts @@ -0,0 +1,32 @@ +import type { ExtensionFactory } from "hunkdiff/extension"; + +export const BUNDLED_EDITOR_COMMAND_ID = "review.editSelectedFile"; +export const BUNDLED_EDITOR_COMMAND_FULL_ID = `hunk.${BUNDLED_EDITOR_COMMAND_ID}`; + +/** Register Hunk's host-mediated editor workflow through the public command contract. */ +const registerBundledEditor: ExtensionFactory = (hunk) => { + hunk.registerCommand( + { + id: BUNDLED_EDITOR_COMMAND_ID, + title: "Open the selected file in your editor", + }, + async (ctx) => { + const file = ctx.selection.file; + if (!file) { + ctx.notify("No file selected.", "warning"); + return; + } + + const result = await ctx.workspace.openInEditor({ + fileId: file.id, + ...(ctx.selection.hunkIndex === null ? {} : { hunkIndex: ctx.selection.hunkIndex }), + ...(ctx.selection.currentLine === null ? {} : { line: ctx.selection.currentLine }), + }); + if (!result.ok) { + ctx.notify(result.detail, result.reason === "failed" ? "error" : "warning"); + } + }, + ); +}; + +export default registerBundledEditor; diff --git a/src/extensions/default/ui/index.test.ts b/src/extensions/default/ui/index.test.ts index c3472717f..f9cced6b4 100644 --- a/src/extensions/default/ui/index.test.ts +++ b/src/extensions/default/ui/index.test.ts @@ -1,10 +1,17 @@ import { describe, expect, test } from "bun:test"; import { getBundledUIRegistry } from "."; import { paneKey } from "../../apply"; +import { BUNDLED_EDITOR_COMMAND_FULL_ID } from "./editor"; describe("bundled UI registry", () => { - test("registers only the built-in files pane", () => { - const panes = getBundledUIRegistry().panes; + test("registers the built-in files pane and editor command", () => { + const registry = getBundledUIRegistry(); + const panes = registry.panes; expect(panes.map(paneKey)).toEqual(["hunk:files"]); + expect( + registry.commands.map(({ extensionId, command }) => `${extensionId}.${command.id}`), + ).toEqual([BUNDLED_EDITOR_COMMAND_FULL_ID]); + expect(registry.extensions).toHaveLength(1); + expect(registry.extensions[0]?.origin).toBe("bundled"); }); }); diff --git a/src/extensions/default/ui/index.ts b/src/extensions/default/ui/index.ts index b5575104b..8e86df792 100644 --- a/src/extensions/default/ui/index.ts +++ b/src/extensions/default/ui/index.ts @@ -6,30 +6,42 @@ import { type ExtensionLoadIssue, type ExtensionRegistry, } from "../../types"; +import registerBundledEditor, { BUNDLED_EDITOR_COMMAND_FULL_ID } from "./editor"; import registerBundledSidebar from "./sidebar"; -const factories: readonly [string, ExtensionFactory][] = [["files", registerBundledSidebar]]; let cachedRegistry: ExtensionRegistry | undefined; +/** Register Hunk's default terminal surfaces through one bundled extension identity. */ +const registerBundledUI: ExtensionFactory = (hunk) => { + registerBundledSidebar(hunk); + registerBundledEditor(hunk); +}; + /** Load bundled UI registrations through the public factory path, once per process. */ export function getBundledUIRegistry(): ExtensionRegistry { if (cachedRegistry) return cachedRegistry; const registry = createEmptyExtensionRegistry(); const issues: ExtensionLoadIssue[] = []; - for (const [id, factory] of factories) { - runExtensionFactory({ - metadata: { - id: HUNK_VENDOR_EXTENSION_ID, - sourcePath: `hunk:bundled/ui/${id}`, - origin: "bundled", - }, - registry, - issues, - factory, - }); - } - if (issues.length > 0 || registry.panes.length !== factories.length) { - throw new Error(`Bundled UI failed to register: ${issues[0]?.message ?? "missing pane"}`); + runExtensionFactory({ + metadata: { + id: HUNK_VENDOR_EXTENSION_ID, + sourcePath: "hunk:bundled/ui", + origin: "bundled", + }, + registry, + issues, + factory: registerBundledUI, + }); + const filesPaneRegistered = registry.panes.some( + ({ extensionId, pane }) => extensionId === HUNK_VENDOR_EXTENSION_ID && pane.id === "files", + ); + const editorCommandRegistered = registry.commands.some( + ({ extensionId, command }) => `${extensionId}.${command.id}` === BUNDLED_EDITOR_COMMAND_FULL_ID, + ); + if (issues.length > 0 || !filesPaneRegistered || !editorCommandRegistered) { + throw new Error( + `Bundled UI failed to register: ${issues[0]?.message ?? "missing required contribution"}`, + ); } cachedRegistry = registry; return registry; diff --git a/src/extensions/types.ts b/src/extensions/types.ts index 1a399b57f..ab07601d2 100644 --- a/src/extensions/types.ts +++ b/src/extensions/types.ts @@ -74,6 +74,9 @@ export type { ExtensionThemeConfig, ExtensionVcsAdapter, ExtensionWorkspace, + ExtensionWorkspaceEditorLine, + ExtensionWorkspaceOpenInEditorRequest, + ExtensionWorkspaceOpenInEditorResult, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, HunkExtensionAPI, diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 9c5cdd1d1..e7ded3d47 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -16,9 +16,9 @@ import { } from "react"; import type { PersistedViewPreferences } from "../core/run/config"; import { experimentalFeatureEnabled, resolveExperimentalDiffFiles } from "../core/run/experimental"; +import { isVcsReviewInput } from "../core/vcs"; import { DEFAULT_FILE_GAP, DEFAULT_HUNK_GAP } from "../core/run/reviewGap"; import { DEFAULT_TAB_WIDTH } from "../core/run/tabWidth"; -import { isVcsReviewInput } from "../core/vcs"; import type { AppBootstrap } from "../core/bootstrap"; import { selectActiveEditableReviewNoteId, @@ -35,6 +35,8 @@ import { } from "../extensions/apply"; import { projectExtensionReviewNotes } from "../extensions/reviewSnapshot"; import type { ExtensionNotifyType, ExtensionLoadResult } from "../extensions/types"; +import { getBundledUIRegistry } from "../extensions/default/ui"; +import { BUNDLED_EDITOR_COMMAND_FULL_ID } from "../extensions/default/ui/editor"; import type { ReviewProducer } from "../app/review/producer"; import type { HunkSessionBrokerClient } from "../session/broker/brokerClient"; import type { ReloadedSessionResult, ReloadSessionOptions } from "../session/types"; @@ -103,7 +105,6 @@ import { import { HUNK_FILES_PANE_KEY } from "../extensions/extensionIds"; import { maxFileHeaderStatsWidth } from "./lib/fileHeader"; import { setMouseCapture } from "./lib/mouseCapture"; -import { openSelectedFileInEditor } from "./lib/openInEditor"; import { resolveResponsiveLayout } from "./lib/responsive"; import type { WorkspaceRefreshRequest } from "./currentReviewRefresh"; @@ -509,6 +510,10 @@ export function App({ const extensionWorkspaceController = useExtensionWorkspaceControls({ createExtensionDialogs, createReviewCapabilityLease, + editorBasePath: isVcsReviewInput(bootstrap.input) + ? (bootstrap.reloadContext.repoRoot ?? bootstrap.changeset.sourceLabel) + : undefined, + editorRenderer: renderer, files: reviewFiles, input: bootstrap.input, onWorkspaceWriteCompleted, @@ -538,6 +543,20 @@ export function App({ getSelection: getExtensionSelection, }); + const bundledEditorCommand = useMemo(() => { + const command = resolveExtensionCommands(getBundledUIRegistry()).commands.find( + ({ extensionId, command: registration }) => + `${extensionId}.${registration.id}` === BUNDLED_EDITOR_COMMAND_FULL_ID, + ); + if (!command) throw new Error("Bundled editor command is not registered."); + return command; + }, []); + + /** Delegate the shared host command shell to the bundled editor extension. */ + const triggerEditSelectedFile = useCallback(() => { + runExtensionCommand(bundledEditorCommand); + }, [bundledEditorCommand, runExtensionCommand]); + const registeredExtensionCommands = useMemo( () => (extensions ? resolveExtensionCommands(extensions.registry).commands : []), [extensions], @@ -884,38 +903,6 @@ export function App({ showNotice: showSessionNotice, }); - const triggerEditSelectedFile = useCallback(() => { - const basePath = isVcsReviewInput(bootstrap.input) - ? bootstrap.changeset.sourceLabel - : undefined; - const message = openSelectedFileInEditor({ - basePath, - file: selectedFile, - lineCursor: activeLineCursor, - renderer, - selectedHunk: review.selectedHunk, - }); - - if (message) { - showSessionNotice(message); - return; - } - - if (canRefreshCurrentInput) { - triggerRefreshCurrentInput(); - } - }, [ - activeLineCursor, - bootstrap.changeset.sourceLabel, - bootstrap.input.kind, - canRefreshCurrentInput, - renderer, - review.selectedHunk, - selectedFile, - showSessionNotice, - triggerRefreshCurrentInput, - ]); - /** Close the agent skill setup overlay. */ const closeAgentSkill = useCallback(() => { setShowAgentSkill(false); diff --git a/src/ui/AppHost.edit-in-editor.test.tsx b/src/ui/AppHost.edit-in-editor.test.tsx index d4f5a3d91..a6702dead 100644 --- a/src/ui/AppHost.edit-in-editor.test.tsx +++ b/src/ui/AppHost.edit-in-editor.test.tsx @@ -5,6 +5,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { act } from "react"; import type { AppBootstrap } from "../core/bootstrap"; +import { createEmptyExtensionLoadResult } from "../extensions/types"; import { createTestVcsAppBootstrap } from "../../test/helpers/app-bootstrap"; import { createTestDiffFile, lines } from "../../test/helpers/diff-helpers"; @@ -46,22 +47,26 @@ function mockSpawnSync(implementation: typeof Bun.spawnSync) { } /** Bootstrap one working-tree review whose file really exists under `sourceLabel`. */ -function createEditorBootstrap(sourceLabel: string): AppBootstrap { - return createTestVcsAppBootstrap({ - changesetId: "changeset:edit-in-editor", - initialMode: "stack", - sourceLabel, - files: [ - createTestDiffFile({ - after: AFTER, - agent: false, - before: BEFORE, - context: 3, - id: "sample", - path: "sample.ts", - }), - ], - }); +function createEditorBootstrap(sourceLabel: string, repoRoot = sourceLabel): AppBootstrap { + return { + ...createTestVcsAppBootstrap({ + changesetId: "changeset:edit-in-editor", + initialMode: "stack", + sourceLabel, + files: [ + createTestDiffFile({ + after: AFTER, + agent: false, + before: BEFORE, + context: 3, + id: "sample", + path: "sample.ts", + }), + ], + }), + extensions: createEmptyExtensionLoadResult(repoRoot), + reloadContext: { cwd: repoRoot, repoRoot }, + }; } async function flush(target: Awaited>) { @@ -119,7 +124,7 @@ describe("AppHost edit-selected-file shortcut", () => { await pressKeys(setup, "e"); - // openSelectedFileInEditor returns "$EDITOR is not set." which shows as a session notice. + // The bundled editor extension reports the host capability's refusal. expect(setup.captureCharFrame()).toContain("EDITOR is not set"); }); @@ -133,7 +138,10 @@ describe("AppHost edit-selected-file shortcut", () => { return { exitCode: 1 }; }) as unknown as typeof Bun.spawnSync); - setup = await testRender(, WIDE); + setup = await testRender( + , + WIDE, + ); await flush(setup); // The hunk starts at line 1; step down onto the changed line, then one line past it. diff --git a/src/ui/hooks/useExtensionWorkspaceControls.test.tsx b/src/ui/hooks/useExtensionWorkspaceControls.test.tsx index 68ed4158d..1a7f93382 100644 --- a/src/ui/hooks/useExtensionWorkspaceControls.test.tsx +++ b/src/ui/hooks/useExtensionWorkspaceControls.test.tsx @@ -7,6 +7,7 @@ import { act, useState } from "react"; import type { CliInput } from "../../core/run/commandInputs"; import type { ExtensionConfirmOptions } from "../../extension-api/types"; import type { WorkspaceFileSource } from "../lib/extensionWorkspace"; +import { createTestDiffFile } from "../../../test/helpers/diff-helpers"; import { useExtensionWorkspaceControls, type WorkspaceFileWriter, @@ -20,8 +21,13 @@ const EXPIRED = { } as const; const WRITABLE_INPUT: CliInput = { kind: "vcs", staged: false, options: {} }; const tempDirs: string[] = []; +const originalEditor = process.env.EDITOR; +const originalSpawnSync = Bun.spawnSync; afterEach(() => { + if (originalEditor === undefined) delete process.env.EDITOR; + else process.env.EDITOR = originalEditor; + (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = originalSpawnSync; for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); @@ -35,10 +41,9 @@ function createTestRoot() { /** Build one reviewed file carrying an optional full-source reader. */ function createTestFile(overrides: Partial = {}): WorkspaceFileSource { + const diff = createTestDiffFile({ id: "alpha", path: "alpha.txt" }); return { - id: "alpha", - path: "alpha.txt", - metadata: { type: "change" }, + ...diff, sourceFetcher: { getFullText: async (side) => `${side} alpha` }, ...overrides, }; @@ -56,6 +61,11 @@ async function renderController({ return true; }, workspaceFileWriter, + editorRenderer = { + isDestroyed: false, + resume: () => {}, + suspend: () => {}, + }, }: { confirm?: (options: ExtensionConfirmOptions, extensionId: string) => Promise; files?: readonly WorkspaceFileSource[]; @@ -64,6 +74,11 @@ async function renderController({ root?: string; runWorkspaceWrite?: WorkspaceWriteRunner; workspaceFileWriter?: WorkspaceFileWriter; + editorRenderer?: { + isDestroyed: boolean; + resume(): void; + suspend(): void; + }; } = {}) { let live = true; let controller!: ReturnType; @@ -86,6 +101,8 @@ async function renderController({ controller = useExtensionWorkspaceControls({ createExtensionDialogs, createReviewCapabilityLease, + editorRenderer, + editorBasePath: liveInputs.root, ...liveInputs, onWorkspaceWriteCompleted, runWorkspaceWrite, @@ -225,6 +242,104 @@ describe("useExtensionWorkspaceControls reads", () => { }); }); +describe("useExtensionWorkspaceControls editor launches", () => { + test("opens only a reviewed file and reconciles after success", async () => { + const root = createTestRoot(); + process.env.EDITOR = "code"; + const spawnCalls: string[][] = []; + (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = ((commands: string[]) => { + spawnCalls.push(commands); + return { exitCode: 0 }; + }) as unknown as typeof Bun.spawnSync; + let reconciliations = 0; + const harness = await renderController({ + root, + onWorkspaceWriteCompleted: () => { + reconciliations += 1; + }, + }); + const workspace = harness.controller().createWorkspaceControls("probe"); + + try { + await expect( + workspace.openInEditor({ + fileId: "alpha", + hunkIndex: 0, + line: { side: "new", line: 3 }, + }), + ).resolves.toEqual({ ok: true }); + expect(spawnCalls).toEqual([["code", "--goto", `${join(root, "alpha.txt")}:3`]]); + expect(reconciliations).toBe(1); + + await expect(workspace.openInEditor({ fileId: "missing" })).resolves.toMatchObject({ + ok: false, + reason: "unavailable", + }); + expect(spawnCalls).toHaveLength(1); + } finally { + await destroy(harness.setup); + } + }); + + test("keeps retained editor controls inert after their review retires", async () => { + process.env.EDITOR = "code"; + let spawns = 0; + (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = (() => { + spawns += 1; + return { exitCode: 0 }; + }) as unknown as typeof Bun.spawnSync; + const harness = await renderController(); + const workspace = harness.controller().createWorkspaceControls("probe"); + harness.retire(); + + try { + await expect(workspace.openInEditor({ fileId: "alpha" })).resolves.toEqual(EXPIRED); + expect(spawns).toBe(0); + } finally { + await destroy(harness.setup); + } + }); + + test("derives the owning hunk for an old-side line and rejects a mismatched hunk", async () => { + const root = createTestRoot(); + process.env.EDITOR = "vim"; + const spawnCalls: string[][] = []; + (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = ((commands: string[]) => { + spawnCalls.push(commands); + return { exitCode: 1 }; + }) as unknown as typeof Bun.spawnSync; + const diff = createTestDiffFile({ + id: "alpha", + path: "alpha.txt", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + }); + const harness = await renderController({ + root, + files: [{ ...diff, sourceFetcher: { getFullText: async () => null } }], + }); + const workspace = harness.controller().createWorkspaceControls("probe"); + + try { + await expect( + workspace.openInEditor({ fileId: "alpha", line: { side: "old", line: 3 } }), + ).resolves.toMatchObject({ ok: false, reason: "failed" }); + expect(spawnCalls).toEqual([["vim", "+2", join(root, "alpha.txt")]]); + + await expect( + workspace.openInEditor({ + fileId: "alpha", + hunkIndex: 0, + line: { side: "old", line: 99 }, + }), + ).resolves.toMatchObject({ ok: false, reason: "unavailable" }); + expect(spawnCalls).toHaveLength(1); + } finally { + await destroy(harness.setup); + } + }); +}); + describe("useExtensionWorkspaceControls lifecycle", () => { test("keeps stable identities while reading live inputs and retiring minted controls", async () => { const initialRoot = createTestRoot(); diff --git a/src/ui/hooks/useExtensionWorkspaceControls.ts b/src/ui/hooks/useExtensionWorkspaceControls.ts index a3e1e7624..2b3631d69 100644 --- a/src/ui/hooks/useExtensionWorkspaceControls.ts +++ b/src/ui/hooks/useExtensionWorkspaceControls.ts @@ -10,16 +10,25 @@ import type { ExtensionDialogs, ExtensionFileSide, ExtensionWorkspace, + ExtensionWorkspaceOpenInEditorRequest, + ExtensionWorkspaceOpenInEditorResult, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, } from "../../extension-api/types"; import type { ExtensionCapabilityLease } from "../lib/extensionCapabilityLease"; import { normalizeWorkspaceWriteRequest, + normalizeWorkspaceOpenInEditorRequest, resolveExtensionWorkspaceRead, resolveExtensionWorkspaceWriteTarget, type WorkspaceFileSource, } from "../lib/extensionWorkspace"; +import { + openSelectedFileInEditor, + type EditorDiffFile, + type EditorDiffHunk, +} from "../lib/openInEditor"; +import type { CliRenderer } from "@opentui/core"; import { verifyWorkspaceWriteTarget } from "../lib/workspaceWriteGuard"; /** Filesystem write implementation used by the host-mediated extension workspace. */ @@ -48,11 +57,22 @@ function expiredWorkspaceWrite(): ExtensionWorkspaceWriteResult { }; } +/** Describe an editor request retired before Hunk starts its host operation. */ +function expiredWorkspaceEditor(): ExtensionWorkspaceOpenInEditorResult { + return { + ok: false, + reason: "unavailable", + detail: "The review reloaded before this extension operation could finish.", + }; +} + /** Own live reviewed-document inputs and host-mediated extension workspace operations. */ export function useExtensionWorkspaceControls({ createExtensionDialogs, createReviewCapabilityLease, files, + editorBasePath, + editorRenderer, input, onWorkspaceWriteCompleted, root, @@ -65,6 +85,10 @@ export function useExtensionWorkspaceControls({ createReviewCapabilityLease: () => ExtensionCapabilityLease; /** Every current reviewed file, including files hidden by filtering. */ files: readonly WorkspaceFileSource[]; + /** Base path used to resolve reviewed paths to their working-tree counterparts. */ + editorBasePath?: string; + /** Renderer lifecycle retained by the host while terminal editors run. */ + editorRenderer: Pick; /** The current CLI review input that decides whether writes are meaningful. */ input: CliInput; /** Reconcile the review currently mounted by the host after a successful write. */ @@ -75,8 +99,8 @@ export function useExtensionWorkspaceControls({ runWorkspaceWrite: WorkspaceWriteRunner; workspaceFileWriter?: WorkspaceFileWriter; }): ExtensionWorkspaceControlsController { - const liveInputsRef = useRef({ files, input, root }); - liveInputsRef.current = { files, input, root }; + const liveInputsRef = useRef({ editorBasePath, files, input, root }); + liveInputsRef.current = { editorBasePath, files, input, root }; const createWorkspaceControls = useCallback( (extensionId: string): ExtensionWorkspace => { @@ -98,6 +122,81 @@ export function useExtensionWorkspaceControls({ const document = read ? await read().catch(() => null) : null; return lease.isLive() ? document : null; }, + async openInEditor( + request: ExtensionWorkspaceOpenInEditorRequest, + ): Promise { + const { fileId, hunkIndex, line } = normalizeWorkspaceOpenInEditorRequest(request); + if (!lease.isLive()) return expiredWorkspaceEditor(); + + const file = liveInputsRef.current.files.find((candidate) => candidate.id === fileId); + if (!file) { + return { + ok: false, + reason: "unavailable", + detail: `No reviewed file has the id "${fileId}".`, + }; + } + + const metadata = file.metadata as Partial | undefined; + if (!metadata || !Array.isArray(metadata.hunks) || typeof metadata.type !== "string") { + return { + ok: false, + reason: "unavailable", + detail: `${file.path} has no editable diff metadata.`, + }; + } + const editorFile = file as WorkspaceFileSource & EditorDiffFile; + let resolvedHunkIndex = hunkIndex; + if (line?.side === "old" && editorFile.metadata.type !== "deleted") { + resolvedHunkIndex ??= editorFile.metadata.hunks.findIndex( + (hunk) => + hunk.deletionCount > 0 && + line.line >= hunk.deletionStart && + line.line < hunk.deletionStart + hunk.deletionCount, + ); + if (resolvedHunkIndex < 0) resolvedHunkIndex = undefined; + } + const selectedHunk = + resolvedHunkIndex === undefined + ? undefined + : editorFile.metadata.hunks[resolvedHunkIndex]; + if (resolvedHunkIndex !== undefined && !selectedHunk) { + return { + ok: false, + reason: "unavailable", + detail: `${file.path} has no hunk at index ${resolvedHunkIndex}.`, + }; + } + if ( + line?.side === "old" && + editorFile.metadata.type !== "deleted" && + (!selectedHunk || + line.line < selectedHunk.deletionStart || + line.line >= selectedHunk.deletionStart + selectedHunk.deletionCount) + ) { + return { + ok: false, + reason: "unavailable", + detail: `${file.path} old line ${line.line} does not belong to the requested hunk.`, + }; + } + + const result = openSelectedFileInEditor({ + basePath: liveInputsRef.current.editorBasePath, + file: editorFile, + lineCursor: line + ? { + fileId, + hunkIndex: resolvedHunkIndex ?? 0, + target: line, + } + : undefined, + renderer: editorRenderer, + selectedHunk: selectedHunk as EditorDiffHunk | undefined, + }); + if (result.ok) onWorkspaceWriteCompleted(); + return result; + }, canWriteDocument(fileId: string) { // An affordance probe answers false rather than throwing for malformed ids. return lease.isLive() && typeof fileId === "string" && resolveTarget(fileId).writable; @@ -174,6 +273,7 @@ export function useExtensionWorkspaceControls({ [ createExtensionDialogs, createReviewCapabilityLease, + editorRenderer, onWorkspaceWriteCompleted, runWorkspaceWrite, workspaceFileWriter, diff --git a/src/ui/lib/extensionWorkspace.test.ts b/src/ui/lib/extensionWorkspace.test.ts index 587e84130..1a3415b64 100644 --- a/src/ui/lib/extensionWorkspace.test.ts +++ b/src/ui/lib/extensionWorkspace.test.ts @@ -2,6 +2,7 @@ import { join, resolve, sep } from "node:path"; import { describe, expect, test } from "bun:test"; import type { CliInput, CommonOptions } from "../../core/run/commandInputs"; import { + normalizeWorkspaceOpenInEditorRequest, normalizeWorkspaceWriteRequest, resolveExtensionWorkspaceRead, resolveExtensionWorkspaceWriteTarget, @@ -240,3 +241,38 @@ describe("extension workspace write requests", () => { ); }); }); + +describe("extension workspace editor requests", () => { + test("copies a well-formed reviewed source address", () => { + expect( + normalizeWorkspaceOpenInEditorRequest({ + fileId: "alpha", + hunkIndex: 2, + line: { side: "old", line: 17 }, + }), + ).toEqual({ + fileId: "alpha", + hunkIndex: 2, + line: { side: "old", line: 17 }, + }); + }); + + test("rejects malformed ids, indexes, and source lines", () => { + expect(() => normalizeWorkspaceOpenInEditorRequest(undefined)).toThrow("non-empty fileId"); + expect(() => normalizeWorkspaceOpenInEditorRequest({ fileId: "alpha", hunkIndex: -1 })).toThrow( + "non-negative integer", + ); + expect(() => + normalizeWorkspaceOpenInEditorRequest({ + fileId: "alpha", + line: { side: "both", line: 1 }, + }), + ).toThrow('line.side must be "old" or "new"'); + expect(() => + normalizeWorkspaceOpenInEditorRequest({ + fileId: "alpha", + line: { side: "new", line: 0 }, + }), + ).toThrow("positive integer"); + }); +}); diff --git a/src/ui/lib/extensionWorkspace.ts b/src/ui/lib/extensionWorkspace.ts index 1d1e76cd9..f9ed14bfe 100644 --- a/src/ui/lib/extensionWorkspace.ts +++ b/src/ui/lib/extensionWorkspace.ts @@ -66,6 +66,45 @@ export interface WorkspaceWriteRequestFields { text: string; } +/** A normalized editor request, once its source address is known to be well-formed. */ +export interface WorkspaceOpenInEditorRequestFields { + fileId: string; + hunkIndex?: number; + line?: { side: FileSourceSide; line: number }; +} + +/** Reject malformed editor requests before they reach renderer or process ownership. */ +export function normalizeWorkspaceOpenInEditorRequest( + request: unknown, +): WorkspaceOpenInEditorRequestFields { + const fields = request as Partial | null | undefined; + if (typeof fields?.fileId !== "string" || fields.fileId.length === 0) { + throw new Error("workspace.openInEditor requires a non-empty fileId."); + } + if ( + fields.hunkIndex !== undefined && + (!Number.isInteger(fields.hunkIndex) || fields.hunkIndex < 0) + ) { + throw new Error("workspace.openInEditor hunkIndex must be a non-negative integer."); + } + if (fields.line !== undefined) { + if (fields.line.side !== "old" && fields.line.side !== "new") { + throw new Error('workspace.openInEditor line.side must be "old" or "new".'); + } + if (!Number.isInteger(fields.line.line) || fields.line.line < 1) { + throw new Error("workspace.openInEditor line.line must be a positive integer."); + } + } + + return { + fileId: fields.fileId, + ...(fields.hunkIndex === undefined ? {} : { hunkIndex: fields.hunkIndex }), + ...(fields.line === undefined + ? {} + : { line: { side: fields.line.side, line: fields.line.line } }), + }; +} + /** * Name what this session is reviewing when it is not the working tree. * diff --git a/src/ui/lib/openInEditor.test.ts b/src/ui/lib/openInEditor.test.ts index 16d099fb5..6c3b29803 100644 --- a/src/ui/lib/openInEditor.test.ts +++ b/src/ui/lib/openInEditor.test.ts @@ -134,7 +134,7 @@ describe("open in editor helpers", () => { renderer, selectedHunk: undefined, }), - ).toBe("No file selected."); + ).toEqual({ ok: false, reason: "unavailable", detail: "No file selected." }); expect(spawnCalls).toEqual([]); expect(renderer.suspend).not.toHaveBeenCalled(); @@ -156,7 +156,7 @@ describe("open in editor helpers", () => { renderer, selectedHunk: undefined, }), - ).toBe("$EDITOR is not set."); + ).toEqual({ ok: false, reason: "unavailable", detail: "$EDITOR is not set." }); expect(spawnCalls).toEqual([]); expect(renderer.suspend).not.toHaveBeenCalled(); @@ -179,7 +179,11 @@ describe("open in editor helpers", () => { renderer, selectedHunk: undefined, }), - ).toBe("Cannot edit missing-on-disk.ts: file does not exist on disk."); + ).toEqual({ + ok: false, + reason: "unavailable", + detail: "Cannot edit missing-on-disk.ts: file does not exist on disk.", + }); expect(spawnCalls).toEqual([]); expect(renderer.suspend).not.toHaveBeenCalled(); @@ -210,7 +214,7 @@ describe("open in editor helpers", () => { renderer, selectedHunk: undefined, }), - ).toBeNull(); + ).toEqual({ ok: true }); expect(spawnCalls).toEqual([ { @@ -247,7 +251,7 @@ describe("open in editor helpers", () => { renderer: createRenderer(), selectedHunk: file.metadata.hunks[0], }), - ).toBeNull(); + ).toEqual({ ok: true }); expect(spawnCalls).toEqual([["vim", "+3", join(basePath, "example.ts")]]); }); @@ -281,7 +285,7 @@ describe("open in editor helpers", () => { renderer: createRenderer(), selectedHunk: file.metadata.hunks[0], }), - ).toBeNull(); + ).toEqual({ ok: true }); expect(spawnCalls).toEqual([["vim", "+2", join(basePath, "example.ts")]]); }); @@ -316,7 +320,7 @@ describe("open in editor helpers", () => { renderer: createRenderer(), selectedHunk: file.metadata.hunks[0], }), - ).toBeNull(); + ).toEqual({ ok: true }); // Old line 3 ("three") was removed, so the editor lands on the line that now follows "one". expect(spawnCalls).toEqual([["vim", "+2", join(basePath, "example.ts")]]); @@ -351,7 +355,7 @@ describe("open in editor helpers", () => { renderer: createRenderer(), selectedHunk: file.metadata.hunks[0], }), - ).toBeNull(); + ).toEqual({ ok: true }); // Old line 3 ("three") is the second of two replaced lines, so the editor // lands on the second replacement line ("THREE") rather than the first. @@ -383,7 +387,7 @@ describe("open in editor helpers", () => { renderer: createRenderer(), selectedHunk: file.metadata.hunks[1], }), - ).toBeNull(); + ).toEqual({ ok: true }); expect(spawnCalls).toEqual([ ["vim", `+${file.metadata.hunks[1]!.additionStart}`, join(basePath, "example.ts")], @@ -422,7 +426,7 @@ describe("open in editor helpers", () => { renderer: createRenderer(), selectedHunk, }), - ).toBeNull(); + ).toEqual({ ok: true }); expect(spawnCalls).toEqual([["vim", "+9", join(basePath, "deleted.ts")]]); }); @@ -448,7 +452,7 @@ describe("open in editor helpers", () => { renderer, selectedHunk: file.metadata.hunks[0], }), - ).toBe("Editor exited with status 2."); + ).toEqual({ ok: false, reason: "failed", detail: "Editor exited with status 2." }); expect(spawnCalls).toEqual([["code", "--wait", "--goto", `${join(basePath, "example.ts")}:1`]]); expect(renderer.suspend).not.toHaveBeenCalled(); @@ -474,7 +478,7 @@ describe("open in editor helpers", () => { renderer, selectedHunk: file.metadata.hunks[0], }), - ).toBe("Failed to launch editor: boom"); + ).toEqual({ ok: false, reason: "failed", detail: "Failed to launch editor: boom" }); expect(renderer.suspend).toHaveBeenCalledTimes(1); expect(renderer.resume).toHaveBeenCalledTimes(1); diff --git a/src/ui/lib/openInEditor.ts b/src/ui/lib/openInEditor.ts index 21f0ed2dc..8235e5699 100644 --- a/src/ui/lib/openInEditor.ts +++ b/src/ui/lib/openInEditor.ts @@ -3,13 +3,17 @@ import { basename, resolve, win32 } from "node:path"; import type { CliRenderer } from "@opentui/core"; import type { DiffFile } from "../../core/changeset/model"; import type { LineCursor } from "./lineCursors"; +import type { ExtensionWorkspaceOpenInEditorResult } from "../../extension-api/types"; export interface EditorCommand { command: string; args: string[]; } -type DiffHunk = DiffFile["metadata"]["hunks"][number]; +export type EditorDiffHunk = DiffFile["metadata"]["hunks"][number]; +export type EditorDiffFile = Pick & { + metadata: Pick; +}; /** The review stream's current line, minus the geometry fields this module never reads. */ export type EditorLineCursor = Pick; @@ -20,7 +24,7 @@ export type EditorLineCursor = Pick; - selectedHunk: DiffHunk | undefined; -}) { + selectedHunk: EditorDiffHunk | undefined; +}): ExtensionWorkspaceOpenInEditorResult { if (!file) { - return "No file selected."; + return { ok: false, reason: "unavailable", detail: "No file selected." }; } const editor = process.env.EDITOR?.trim(); if (!editor) { - return "$EDITOR is not set."; + return { ok: false, reason: "unavailable", detail: "$EDITOR is not set." }; } const absolutePath = resolveEditableFilePath(file.path, basePath); if (!existsSync(absolutePath)) { - return `Cannot edit ${file.path}: file does not exist on disk.`; + return { + ok: false, + reason: "unavailable", + detail: `Cannot edit ${file.path}: file does not exist on disk.`, + }; } const line = Math.max(1, selectedLine(file, selectedHunk, lineCursor)); @@ -197,12 +205,12 @@ export function openSelectedFileInEditor({ } if (failureMessage) { - return `Failed to launch editor: ${failureMessage}`; + return { ok: false, reason: "failed", detail: `Failed to launch editor: ${failureMessage}` }; } if (exitCode !== 0) { - return `Editor exited with status ${exitCode}.`; + return { ok: false, reason: "failed", detail: `Editor exited with status ${exitCode}.` }; } - return null; + return { ok: true }; } diff --git a/website/src/content/docs/docs/extend/extension-api.md b/website/src/content/docs/docs/extend/extension-api.md index c376bbc25..1d17e1427 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -7,9 +7,10 @@ The extension factory receives one API object. Registration calls are only valid ## `hunk.apiVersion` -The API generation this Hunk speaks (currently `15`). Branch on it if you want -one file to support several Hunk versions. Version 15 adds `{ side, line }` to -opted-in pane `currentLine` paint; version 14 added structured two-revision +The API generation this Hunk speaks (currently `16`). Branch on it if you want +one file to support several Hunk versions. Version 16 adds host-mediated editor +launches for reviewed files; 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 and committed note-edit events; version 12 added responsive fractional pane sizing; version 11 added the `dim` line-highlight tone; version 10 added @@ -335,13 +336,14 @@ One dialog shows at a time; concurrent requests queue in call order, across exte ### Workspace documents -`ctx.workspace` reads full documents from the current review and writes eligible working-tree files. +`ctx.workspace` reads full documents from the current review, opens reviewed files through Hunk's editor lifecycle, and writes eligible working-tree files. -| Method | Result | -| -------------------------------------- | ------------------------------------------------- | -| `readDocument(fileId, "old" \| "new")` | Reviewed source text or `null` | -| `canWriteDocument(fileId)` | Whether review policy allows a write | -| `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | +| Method | Result | +| --------------------------------------------- | ------------------------------------------------- | +| `readDocument(fileId, "old" \| "new")` | Reviewed source text or `null` | +| `openInEditor({ fileId, hunkIndex?, line? })` | `{ ok: true }` or `{ ok: false, reason, detail }` | +| `canWriteDocument(fileId)` | Whether review policy allows a write | +| `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | ```ts const file = ctx.selection.file; @@ -355,13 +357,15 @@ if (file && ctx.workspace.canWriteDocument(file.id)) { Reads return the source represented by the review, including historical content in revision and stash reviews. Missing, unreadable, or oversized sources return `null`; reads never prompt. +Editor requests name a reviewed file id and optional source address, never a path or process. Hunk resolves `$EDITOR`, maps old-side lines onto the working-tree file, owns terminal suspension, and reloads reloadable inputs after success. Missing configuration or files return `unavailable`; launch failures return `failed`. + Writes require a reloadable, unstaged working-tree review and a writable reviewed-file id. Hunk verifies the target, asks for attributed consent, verifies it again, writes it, and reloads the review. Other review kinds and deleted, binary, oversized, missing, symlinked, or root-escaping targets return `unavailable`. Cancellation returns `cancelled`; an attempted write failure returns `failed` with a displayable `detail`. `canWriteDocument` does not inspect the filesystem, so `writeDocument` can still refuse a changed target. See the [full workspace guide](https://github.com/modem-dev/hunk/blob/main/docs/extensions.md#workspace-documents) for lifecycle and error details. ## `hunk.on(event, handler)` -Subscribe to a lifecycle or UI event. Handlers may be async; Hunk never blocks the UI waiting for one. Every handler receives `ctx.panes`, live `ctx.navigation`, and attributed `ctx.dialogs` alongside `cwd` and `notify`, so a `startup` handler can present one focused welcome dialog and navigate to its first example without a keypress. `ctx.sidebars` is deprecated. Controls retained across a review or extension-registry replacement expire instead of controlling the replacement UI; workspace reads and writes that have not started return `null`/`unavailable`. Once a consented filesystem write starts, it reports its actual outcome, graceful shutdown waits for it, and success reconciles the review then active. +Subscribe to a lifecycle or UI event. Handlers may be async; Hunk never blocks the UI waiting for one. Every handler receives `ctx.panes`, live `ctx.navigation`, and attributed `ctx.dialogs` alongside `cwd` and `notify`, so a `startup` handler can present one focused welcome dialog and navigate to its first example without a keypress. `ctx.sidebars` is deprecated. Controls retained across a review or extension-registry replacement expire instead of controlling the replacement UI; workspace reads, editor launches, and writes that have not started return `null`/`unavailable`. Once a consented filesystem write starts, it reports its actual outcome, graceful shutdown waits for it, and success reconciles the review then active. | Event | Payload | When | | ---------------------- | ----------------------- | -------------------------------------------------------- | From 640b4515747f89aaba0a1b3cad80415f4fcb595c Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Mon, 31 Aug 2026 11:23:56 -0400 Subject: [PATCH 2/4] refactor(extensions): generalize editor app handoff --- .changeset/fuzzy-editors-dock.md | 2 +- docs/extension-architecture.md | 16 +- docs/extensions.md | 84 ++- skills/hunk-extensions/SKILL.md | 25 +- src/extension-api/index.ts | 5 +- src/extension-api/types.ts | 53 +- .../default/ui/editor/editorApp.test.ts | 32 ++ src/extensions/default/ui/editor/editorApp.ts | 87 ++++ .../default/ui/editor/index.test.ts | 113 ++-- src/extensions/default/ui/editor/index.ts | 50 +- src/extensions/types.ts | 5 +- src/ui/App.tsx | 11 +- src/ui/AppHost.edit-in-editor.test.tsx | 2 +- src/ui/currentReviewRefresh.ts | 4 +- .../useCurrentReviewRefreshController.ts | 4 +- .../hooks/useExtensionAppController.test.tsx | 191 +++++++ src/ui/hooks/useExtensionAppController.ts | 54 ++ .../hooks/useExtensionCommandRunner.test.tsx | 3 + src/ui/hooks/useExtensionCommandRunner.ts | 5 + .../useExtensionWorkspaceControls.test.tsx | 112 +--- src/ui/hooks/useExtensionWorkspaceControls.ts | 113 +--- src/ui/lib/extensionWorkspace.test.ts | 138 ++++- src/ui/lib/extensionWorkspace.ts | 128 ++++- src/ui/lib/openInEditor.test.ts | 486 ------------------ src/ui/lib/openInEditor.ts | 216 -------- test/pty/extensions-integration.test.ts | 64 ++- .../content/docs/docs/extend/extension-api.md | 31 +- 27 files changed, 960 insertions(+), 1074 deletions(-) create mode 100644 src/extensions/default/ui/editor/editorApp.test.ts create mode 100644 src/extensions/default/ui/editor/editorApp.ts create mode 100644 src/ui/hooks/useExtensionAppController.test.tsx create mode 100644 src/ui/hooks/useExtensionAppController.ts delete mode 100644 src/ui/lib/openInEditor.test.ts delete mode 100644 src/ui/lib/openInEditor.ts diff --git a/.changeset/fuzzy-editors-dock.md b/.changeset/fuzzy-editors-dock.md index 56c0c1058..e05add4af 100644 --- a/.changeset/fuzzy-editors-dock.md +++ b/.changeset/fuzzy-editors-dock.md @@ -2,4 +2,4 @@ "hunkdiff": minor --- -Expose host-mediated editor launches to extensions and run Hunk's open-in-editor workflow as a bundled extension. +Let extension commands temporarily hand Hunk's terminal to an application and run Hunk's open-in-editor workflow as a bundled extension. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index 79baacd19..5e3b99a93 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -273,16 +273,18 @@ inert before shutdown begins. Session behavior requests are registry data too: presentation view changes ephemeral without teaching `App` about an extension id. +`src/ui/hooks/useExtensionAppController.ts` owns `ctx.openInApp`. Command-scoped +leases refuse stale handoffs, one shared lock prevents overlapping applications, +and renderer suspension always resumes in `finally` unless the renderer was +destroyed. The extension owns execution and application-specific metadata; +Hunk's bundled editor command consumes the same public callback and explicitly +refreshes after a successful edit. + `src/ui/lib/extensionWorkspace.ts` owns the policy for `ctx.workspace`. Reads resolve reviewed file ids through the existing source fetcher, which retains ownership of caching and size limits. Missing or unreadable sources become -`null`. - -Editor requests also name reviewed file ids. The host resolves the current -working-tree path and source line and retains renderer suspend/resume and -process ownership in `openInEditor.ts`; reloadable inputs reconcile the review -after success. Hunk's own editor command is a bundled extension handler over -that same capability. +`null`. Location resolution maps reviewed file ids and source addresses onto +attested on-disk paths and lines using input provenance and the authoritative parsed hunk. Writes are limited to reloadable working-tree reviews and reviewed paths inside the review root. App supplies the current input, unfiltered changeset, and root diff --git a/docs/extensions.md b/docs/extensions.md index 120e3ddb2..8ecbf504b 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -281,8 +281,8 @@ 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 host-mediated editor -launches for reviewed files; version 15 added `{ side, line }` to opted-in pane +one file to support several Hunk versions. Version 16 adds temporary application +handoffs from command handlers; 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 committed note-edit events; version 12 adds responsive fractional pane sizing; version 11 added @@ -1680,18 +1680,55 @@ the same way, and a request made after that point cancels immediately. A blank answer from the user, so the promise **rejects**; like any other handler failure, that surfaces as a warning naming your extension. +#### Temporary applications + +`ctx.openInApp(callback)` temporarily replaces Hunk with an application your +extension runs. Hunk suspends its renderer before calling you and restores the +review in `finally` after your callback returns or throws: + +```ts +async function runProjectTool(metadata: { file: string | undefined; line: number | undefined }) { + // Spawn an interactive process with inherited stdio and encode metadata however the app expects. + return { exitCode: 0, metadata }; +} + +hunk.registerCommand({ id: "open-tool", title: "Open project tool", key: "f8" }, async (ctx) => { + const file = ctx.selection.file; + const location = file + ? ctx.workspace.resolveLocation({ + fileId: file.id, + ...(ctx.selection.hunkIndex === null ? {} : { hunkIndex: ctx.selection.hunkIndex }), + ...(ctx.selection.currentLine === null ? {} : { line: ctx.selection.currentLine }), + }) + : null; + const result = await ctx.openInApp(() => + runProjectTool({ + file: location?.path, + line: location?.line, + }), + ); + if (result.exitCode !== 0) ctx.notify(`Tool exited with status ${result.exitCode}`, "error"); +}); +``` + +The extension owns process execution and decides how to pass file, line, hunk, +or extension state through arguments, environment, files, or an application-specific +protocol. Hunk only owns terminal suspension and restoration. One application +may own the terminal at a time; concurrent calls and controls retained past a +review reload reject without invoking the callback. The callback's value and +error pass through unchanged. + #### Workspace documents -`ctx.workspace` reads full documents from the current review, opens reviewed -files through Hunk's editor lifecycle, and can replace an eligible working-tree -file. +`ctx.workspace` reads full documents from the current review and can replace an +eligible working-tree file. -| Method | Result | -| --------------------------------------------- | ------------------------------------------------- | -| `readDocument(fileId, "old" \| "new")` | The reviewed source text, or `null` | -| `openInEditor({ fileId, hunkIndex?, line? })` | `{ ok: true }` or `{ ok: false, reason, detail }` | -| `canWriteDocument(fileId)` | Whether the review and file allow writes | -| `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | +| Method | Result | +| -------------------------------------- | ------------------------------------------------- | +| `readDocument(fileId, "old" \| "new")` | The reviewed source text, or `null` | +| `resolveLocation({ fileId, ... })` | Absolute on-disk `{ path, line }`, or `null` | +| `canWriteDocument(fileId)` | Whether the review and file allow writes | +| `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | A command can read, transform, and write a selected file: @@ -1714,16 +1751,6 @@ hunk.registerCommand({ id: "shout-headings", title: "Shout headings", key: "f7" }); ``` -`openInEditor` accepts only a reviewed file id, plus an optional zero-based -`hunkIndex` and one-based `{ side, line }` source address. Hunk resolves the -working-tree path and editor command itself. It maps old-side lines onto the -file on disk, suspends and resumes terminal editors, and queues a review reload -after success when the current input is reloadable. Missing `$EDITOR` -configuration or a missing reviewed file returns `unavailable`; process -failures and non-zero exits return `failed`. Malformed source addresses reject. -No consent prompt is shown because opening the user's configured editor does -not itself modify a file. - `readDocument` returns the exact source represented by the review, not the file's patch. It works for every review kind. For example, the `"new"` side in `hunk show HEAD` is the file at that commit, not the working-tree file. It @@ -1731,6 +1758,16 @@ returns `null` when the file or side is absent, no source is available, reading fails, or the document exceeds Hunk's size limit. Reads never prompt. An invalid side rejects the promise. +`resolveLocation` turns a reviewed file id and optional `hunkIndex` and +`{ side, line }` into the corresponding absolute path and one-based line on +disk. VCS reviews resolve against the repository; direct file comparisons retain +the concrete compared path, including the old path for a deleted-file comparison. +Hunk uses parsed hunk metadata to map old-side deletions onto their on-disk +position, so extensions can pass accurate locations to editors, debuggers, +browsers, or other applications without interpreting opaque diff metadata. Raw +patch reviews have no attested filesystem path and return `null`. Missing hunks +and stale controls also return `null`; malformed source addresses reject. + Writes require all of the following: - an unstaged working-tree review (`hunk diff` with no revision range) @@ -1805,8 +1842,9 @@ showing — no keypress required. Dialog calls made before the mounted app is ready resolve to their cancel value with a warning rather than opening later. Controls retained across a review or extension-registry replacement expire: navigation and pane mutations warn and do nothing, dialogs resolve to their -normal cancel value, and workspace reads, editor launches, or not-yet-started -writes return `null`/`unavailable` instead of acting on replacement content. +normal cancel value, and workspace reads or not-yet-started writes return +`null`/`unavailable` instead of acting on replacement content. A stale +`openInApp` callback rejects before taking terminal ownership. Once a consented filesystem write starts, it reports its actual outcome and success reconciles the review then active. diff --git a/skills/hunk-extensions/SKILL.md b/skills/hunk-extensions/SKILL.md index a8f2188b1..90eeb29ad 100644 --- a/skills/hunk-extensions/SKILL.md +++ b/skills/hunk-extensions/SKILL.md @@ -160,9 +160,10 @@ transform — gets `ctx.cwd` and `ctx.notify(message, type?)`. A file view's (`isEnabled`/`execute` for public semantic `hunk.*` commands), `ctx.keyboardModes` (enter/exit/probe this extension's session modes), `ctx.review` (deeply immutable snapshots of stable files and complete saved store notes), - `ctx.dialogs` (`confirm`/`select`/`input`, queued and attributed), and - `ctx.workspace` (`readDocument`, host-mediated `openInEditor`, - `canWriteDocument`, `writeDocument` with consent). + `ctx.dialogs` (`confirm`/`select`/`input`, queued and attributed), + `ctx.openInApp` (temporary terminal ownership around extension-run applications), + and `ctx.workspace` (`readDocument`, `resolveLocation`, `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`. @@ -215,9 +216,9 @@ Most extension bugs are one of these: `review-note-navigator` shows how to join stable note ids and file keys back to guarded navigation after awaiting a selector; file filters can still refuse hidden targets. - **Retained review controls expire on reload.** An old handler cannot control - replacement content: pane/navigation calls become inert, dialogs cancel, and - workspace reads, editor launches, or not-yet-started writes return - `null`/`unavailable`. A + replacement content: pane/navigation calls become inert, dialogs cancel, + stale app handoffs reject, and workspace reads or not-yet-started writes + return `null`/`unavailable`. A consented write already in progress reports its real outcome, holds graceful exit until it settles, and reconciles the active review on success. `shutdown` runs after revocation, so use it only @@ -254,12 +255,12 @@ Most extension bugs are one of these: - **Failures are contained, not sandboxed.** A throwing factory is rolled back to zero registrations and a throwing handler is a warning naming the extension — containment against bugs, not against code that should not have been loaded. -- **The API touches nothing outside the review.** No clipboard, no arbitrary - filesystem path, and no arbitrary process surface: `ctx.workspace` reads, - writes, or opens only reviewed file ids. An extension is ordinary code, so - shell out for unsupported integrations. Never write to stdout: the renderer - owns it. For the same reason `hunk.log` is collected as diagnostics and printed - nowhere; `ctx.notify` is how a user hears from you. +- **Application execution stays extension-owned.** Extensions are ordinary + trusted code and may spawn processes; use `ctx.openInApp` when one needs the + terminal so Hunk suspends and restores its renderer. `ctx.workspace.resolveLocation` + maps reviewed ids to app-ready paths and lines. Never write to stdout while + Hunk owns the terminal; `hunk.log` is collected as diagnostics and `ctx.notify` + is how a user hears from you. - **`HunkExtensionUserError`** (detected structurally by `name`) buys the full treatment — message plus `suggestions`, no stack trace — only from a VCS adapter operation, which is where Hunk formats it for the CLI. From a command or event diff --git a/src/extension-api/index.ts b/src/extension-api/index.ts index b588f18e6..06c7b6aab 100644 --- a/src/extension-api/index.ts +++ b/src/extension-api/index.ts @@ -155,9 +155,8 @@ export type { ExtensionVcsWatchTarget, ExtensionVcsWatchTargetSource, ExtensionWorkspace, - ExtensionWorkspaceEditorLine, - ExtensionWorkspaceOpenInEditorRequest, - ExtensionWorkspaceOpenInEditorResult, + ExtensionWorkspaceLocation, + ExtensionWorkspaceLocationRequest, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, HunkExtensionAPI, diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index 5ac231f00..414d6e510 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -1675,27 +1675,23 @@ export type ExtensionWorkspaceWriteResult = | { ok: true } | { ok: false; reason: "unavailable" | "cancelled" | "failed"; detail: string }; -/** One reviewed source line an editor should reveal. */ -export interface ExtensionWorkspaceEditorLine { - side: ExtensionFileSide; - /** One-based source line number on `side`. */ - line: number; -} - -/** A reviewed file and optional source location an extension asks Hunk to open. */ -export interface ExtensionWorkspaceOpenInEditorRequest { - /** The reviewed file to open, by its `ExtensionDiffFile.id`. */ +/** A reviewed source address an extension wants to pass to an application. */ +export interface ExtensionWorkspaceLocationRequest { + /** The reviewed file, by its `ExtensionDiffFile.id`. */ fileId: string; - /** Hunk index used to map an old-side line onto the working-tree file. */ + /** Hunk used to map an old-side line onto the corresponding on-disk file. */ hunkIndex?: number; /** Exact source line to prefer over the hunk's first line. */ - line?: ExtensionWorkspaceEditorLine; + line?: { side: ExtensionFileSide; line: number }; } -/** How a host-mediated editor launch settled. */ -export type ExtensionWorkspaceOpenInEditorResult = - | { ok: true } - | { ok: false; reason: "unavailable" | "failed"; detail: string }; +/** The on-disk path and line represented by a reviewed source address. */ +export interface ExtensionWorkspaceLocation { + /** Absolute on-disk path corresponding to the reviewed source. */ + path: string; + /** One-based line in the file on disk. */ + line: number; +} /** * The reviewed files as whole documents, read and written through the host. @@ -1752,18 +1748,12 @@ export interface ExtensionWorkspace { */ readDocument(fileId: string, side: ExtensionFileSide): Promise; /** - * Open a reviewed file in the user's `$EDITOR` through Hunk's terminal lifecycle. - * - * The extension names a reviewed file id and source location, never a filesystem - * path or process. Hunk resolves the working-tree path, maps old-side lines onto - * the file on disk, suspends and resumes terminal editors, and reloads a reloadable - * review after a successful launch. Missing files or editor configuration resolve - * `"unavailable"`; launch and non-zero-exit failures resolve `"failed"`. Malformed - * ids, hunk indexes, sides, or line numbers reject as programming errors. + * Resolve review metadata into the corresponding path and line on disk. + * + * Returns `null` when the input has no attested path, the file or hunk is + * unavailable, or the review generation expires. Malformed source addresses reject. */ - openInEditor( - request: ExtensionWorkspaceOpenInEditorRequest, - ): Promise; + resolveLocation(request: ExtensionWorkspaceLocationRequest): ExtensionWorkspaceLocation | null; /** * Whether `writeDocument` could currently succeed for this reviewed file. * @@ -1835,6 +1825,15 @@ export interface ExtensionSessionOptions { export interface ExtensionCommandContext extends ExtensionContext { /** Live access to the public built-in command table. */ readonly commands: ExtensionCommandControls; + /** + * Temporarily hand Hunk's terminal to an application run by this extension. + * + * Hunk suspends its renderer before calling `run` and restores the review in + * `finally` after `run` settles. The extension owns execution, metadata, + * arguments, environment, and exit handling. Calls reject after this review + * generation expires or while another application already owns the terminal. + */ + openInApp(run: () => Result | PromiseLike): Promise; /** Session keyboard modes registered by this command's owning extension. */ readonly keyboardModes: ExtensionKeyboardModeControls; /** Session panes registered by this command's owning extension. */ diff --git a/src/extensions/default/ui/editor/editorApp.test.ts b/src/extensions/default/ui/editor/editorApp.test.ts new file mode 100644 index 000000000..45fdddf6e --- /dev/null +++ b/src/extensions/default/ui/editor/editorApp.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, test } from "bun:test"; +import { buildEditorCommand, editorUsesTerminal } from "./editorApp"; + +describe("bundled editor app", () => { + test("builds editor-specific line arguments without a shell", () => { + expect( + buildEditorCommand({ + editor: '"C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd" --wait', + filePath: "C:\\repo\\file with spaces.ts", + line: 7, + }), + ).toEqual({ + command: "C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd", + args: ["--wait", "--goto", "C:\\repo\\file with spaces.ts:7"], + }); + expect( + buildEditorCommand({ editor: "nvim --clean", filePath: "/repo/a.ts", line: 12 }), + ).toEqual({ command: "nvim", args: ["--clean", "+12", "/repo/a.ts"] }); + expect( + buildEditorCommand({ editor: "code --reuse-window", filePath: "/repo/a.ts", line: 9 }), + ).toEqual({ + command: "code", + args: ["--reuse-window", "--wait", "--goto", "/repo/a.ts:9"], + }); + }); + + test("hands terminal editors to Hunk's app lifecycle but leaves GUI editors visible", () => { + expect(editorUsesTerminal("nvim --clean")).toBe(true); + expect(editorUsesTerminal('"C:\\Program Files\\Cursor\\cursor.exe" --wait')).toBe(false); + expect(editorUsesTerminal("code-insiders --wait")).toBe(false); + }); +}); diff --git a/src/extensions/default/ui/editor/editorApp.ts b/src/extensions/default/ui/editor/editorApp.ts new file mode 100644 index 000000000..53ff7ba68 --- /dev/null +++ b/src/extensions/default/ui/editor/editorApp.ts @@ -0,0 +1,87 @@ +import { existsSync } from "node:fs"; +import { basename, win32 } from "node:path"; + +export interface EditorCommand { + command: string; + args: string[]; +} + +/** Split the user's editor command without involving a shell. */ +function splitEditorCommand(editor: string) { + return ( + editor + .match(/(?:[^\s"']+|"(?:\\.|[^"])*"|'(?:\\.|[^'])*')+/g) + ?.map((token) => token.replace(/^(["'])(.*)\1$/, "$2")) ?? [] + ); +} + +/** Return the executable basename used to select an editor's line-address syntax. */ +function editorProgram(editor: string) { + const [firstToken = ""] = splitEditorCommand(editor); + return basename(win32.basename(firstToken)) + .replace(/\.(?:cmd|exe)$/i, "") + .toLowerCase(); +} + +const VI_STYLE_EDITORS = ["vim", "nvim", "vi"]; +const CODE_STYLE_EDITORS = ["code", "code-insiders", "cursor"]; + +/** Report whether this editor expects to own the current terminal. */ +export function editorUsesTerminal(editor: string) { + return !CODE_STYLE_EDITORS.includes(editorProgram(editor)); +} + +/** Build an editor process invocation without shell quoting. */ +export function buildEditorCommand({ + editor, + filePath, + line, +}: { + editor: string; + filePath: string; + line: number; +}): EditorCommand { + const [command = "", ...editorArgs] = splitEditorCommand(editor); + const program = editorProgram(editor); + + if (VI_STYLE_EDITORS.includes(program)) { + return { command, args: [...editorArgs, `+${line}`, filePath] }; + } + if (CODE_STYLE_EDITORS.includes(program)) { + const waitArgs = editorArgs.includes("--wait") || editorArgs.includes("-w") ? [] : ["--wait"]; + return { + command, + args: [...editorArgs, ...waitArgs, "--goto", `${filePath}:${line}`], + }; + } + if (program === "hx") { + return { command, args: [...editorArgs, `${filePath}:${line}`] }; + } + return { command, args: [...editorArgs, filePath] }; +} + +/** Validate one resolved location and turn it into an editor invocation. */ +export function editorCommandForLocation({ + editor, + line, + path, + reviewPath, +}: { + editor: string; + line: number; + path: string; + reviewPath: string; +}): { ok: true; command: EditorCommand } | { ok: false; detail: string } { + if (!existsSync(path)) { + return { ok: false, detail: `Cannot edit ${reviewPath}: file does not exist on disk.` }; + } + + return { + ok: true, + command: buildEditorCommand({ + editor, + filePath: path, + line, + }), + }; +} diff --git a/src/extensions/default/ui/editor/index.test.ts b/src/extensions/default/ui/editor/index.test.ts index a06b93fc2..8b2674592 100644 --- a/src/extensions/default/ui/editor/index.test.ts +++ b/src/extensions/default/ui/editor/index.test.ts @@ -1,8 +1,22 @@ -import { describe, expect, mock, test } from "bun:test"; +import { afterEach, describe, expect, mock, test } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import type { ExtensionCommandContext } from "hunkdiff/extension"; import { getBundledUIRegistry } from ".."; import { BUNDLED_EDITOR_COMMAND_FULL_ID } from "."; +const originalEditor = process.env.EDITOR; +const originalSpawnSync = Bun.spawnSync; +const tempDirs: string[] = []; + +afterEach(() => { + if (originalEditor === undefined) delete process.env.EDITOR; + else process.env.EDITOR = originalEditor; + (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = originalSpawnSync; + for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + /** Return the editor registration from the process-static bundled UI registry. */ function getBundledEditorCommand() { const registered = getBundledUIRegistry().commands.find( @@ -12,6 +26,36 @@ function getBundledEditorCommand() { return registered; } +/** Build a frozen public selection for one file that exists in a temporary workspace. */ +function createEditorContext() { + const cwd = mkdtempSync(join(tmpdir(), "hunk-bundled-editor-")); + tempDirs.push(cwd); + writeFileSync(join(cwd, "alpha.ts"), "one\ntwo\nthree\n"); + const execute = mock(() => true); + const notify = mock(() => {}); + const openInApp = mock(async (run: () => Result | PromiseLike) => await run()); + const context = { + commands: { execute }, + cwd, + notify, + openInApp, + selection: { + file: { + id: "alpha", + path: "alpha.ts", + changeType: "change", + hunks: [{ index: 0, header: "@@", oldRange: [1, 3], newRange: [1, 3] }], + }, + hunkIndex: 0, + currentLine: { side: "new", line: 2 }, + }, + workspace: { + resolveLocation: () => ({ path: join(cwd, "alpha.ts"), line: 2 }), + }, + } as unknown as ExtensionCommandContext; + return { context, cwd, execute, notify, openInApp }; +} + describe("bundled editor extension", () => { test("registers the shared Hunk command identity without owning its host key shell", () => { const registered = getBundledEditorCommand(); @@ -23,45 +67,50 @@ describe("bundled editor extension", () => { }); }); - test("forwards the frozen review selection to the host editor capability", async () => { - const openInEditor = mock(async () => ({ ok: true as const })); - const notify = mock(() => {}); - const context = { - notify, - selection: { - file: { id: "alpha" }, - hunkIndex: 2, - currentLine: { side: "old", line: 17 }, - }, - workspace: { openInEditor }, - } as unknown as ExtensionCommandContext; + test("runs the configured editor inside a generic app handoff and refreshes", async () => { + const { context, cwd, execute, notify, openInApp } = createEditorContext(); + process.env.EDITOR = "vim --clean"; + const spawnCalls: string[][] = []; + (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = ((command: string[]) => { + spawnCalls.push(command); + return { exitCode: 0 }; + }) as unknown as typeof Bun.spawnSync; await getBundledEditorCommand().handler(context); - expect(openInEditor).toHaveBeenCalledWith({ - fileId: "alpha", - hunkIndex: 2, - line: { side: "old", line: 17 }, - }); + expect(openInApp).toHaveBeenCalledTimes(1); + expect(spawnCalls).toEqual([["vim", "--clean", "+2", join(cwd, "alpha.ts")]]); + expect(execute).toHaveBeenCalledWith("hunk.app.refresh"); expect(notify).not.toHaveBeenCalled(); }); - test("surfaces host refusals without attempting its own process or path handling", async () => { - const notify = mock(() => {}); - const context = { - notify, - selection: { file: { id: "alpha" }, hunkIndex: null, currentLine: null }, - workspace: { - openInEditor: async () => ({ - ok: false as const, - reason: "unavailable" as const, - detail: "$EDITOR is not set.", - }), - }, - } as unknown as ExtensionCommandContext; + test("reports editor failures after Hunk restores its view", async () => { + const { context, notify } = createEditorContext(); + process.env.EDITOR = "vim"; + (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = (() => ({ + exitCode: 2, + })) as unknown as typeof Bun.spawnSync; + + await getBundledEditorCommand().handler(context); + + expect(notify).toHaveBeenCalledWith("Editor exited with status 2.", "error"); + }); + + test("keeps GUI editors visible and waits for them before refreshing", async () => { + const { context, cwd, execute, openInApp } = createEditorContext(); + process.env.EDITOR = "code --reuse-window"; + const spawnCalls: string[][] = []; + (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = ((command: string[]) => { + spawnCalls.push(command); + return { exitCode: 0 }; + }) as unknown as typeof Bun.spawnSync; await getBundledEditorCommand().handler(context); - expect(notify).toHaveBeenCalledWith("$EDITOR is not set.", "warning"); + expect(openInApp).not.toHaveBeenCalled(); + expect(spawnCalls).toEqual([ + ["code", "--reuse-window", "--wait", "--goto", `${join(cwd, "alpha.ts")}:2`], + ]); + expect(execute).toHaveBeenCalledWith("hunk.app.refresh"); }); }); diff --git a/src/extensions/default/ui/editor/index.ts b/src/extensions/default/ui/editor/index.ts index 436db65c9..506f24175 100644 --- a/src/extensions/default/ui/editor/index.ts +++ b/src/extensions/default/ui/editor/index.ts @@ -1,9 +1,10 @@ import type { ExtensionFactory } from "hunkdiff/extension"; +import { editorCommandForLocation, editorUsesTerminal } from "./editorApp"; export const BUNDLED_EDITOR_COMMAND_ID = "review.editSelectedFile"; export const BUNDLED_EDITOR_COMMAND_FULL_ID = `hunk.${BUNDLED_EDITOR_COMMAND_ID}`; -/** Register Hunk's host-mediated editor workflow through the public command contract. */ +/** Register Hunk's editor workflow through the public app-handoff contract. */ const registerBundledEditor: ExtensionFactory = (hunk) => { hunk.registerCommand( { @@ -11,20 +12,59 @@ const registerBundledEditor: ExtensionFactory = (hunk) => { title: "Open the selected file in your editor", }, async (ctx) => { + const editor = process.env.EDITOR?.trim(); + if (!editor) { + ctx.notify("$EDITOR is not set.", "warning"); + return; + } + const file = ctx.selection.file; if (!file) { ctx.notify("No file selected.", "warning"); return; } - - const result = await ctx.workspace.openInEditor({ + const location = ctx.workspace.resolveLocation({ fileId: file.id, ...(ctx.selection.hunkIndex === null ? {} : { hunkIndex: ctx.selection.hunkIndex }), ...(ctx.selection.currentLine === null ? {} : { line: ctx.selection.currentLine }), }); - if (!result.ok) { - ctx.notify(result.detail, result.reason === "failed" ? "error" : "warning"); + if (!location) { + ctx.notify(`Cannot resolve ${file.path} on disk.`, "warning"); + return; + } + const selected = editorCommandForLocation({ + editor, + ...location, + reviewPath: file.path, + }); + if (!selected.ok) { + ctx.notify(selected.detail, "warning"); + return; + } + + let exitCode: number; + try { + const runEditor = () => + Bun.spawnSync([selected.command.command, ...selected.command.args], { + stdin: "inherit", + stdout: "inherit", + stderr: "inherit", + }); + const result = editorUsesTerminal(editor) ? await ctx.openInApp(runEditor) : runEditor(); + exitCode = result.exitCode; + } catch (error) { + ctx.notify( + `Failed to launch editor: ${error instanceof Error ? error.message : String(error)}`, + "error", + ); + return; + } + + if (exitCode !== 0) { + ctx.notify(`Editor exited with status ${exitCode}.`, "error"); + return; } + ctx.commands.execute("hunk.app.refresh"); }, ); }; diff --git a/src/extensions/types.ts b/src/extensions/types.ts index ab07601d2..9f9523288 100644 --- a/src/extensions/types.ts +++ b/src/extensions/types.ts @@ -74,9 +74,8 @@ export type { ExtensionThemeConfig, ExtensionVcsAdapter, ExtensionWorkspace, - ExtensionWorkspaceEditorLine, - ExtensionWorkspaceOpenInEditorRequest, - ExtensionWorkspaceOpenInEditorResult, + ExtensionWorkspaceLocation, + ExtensionWorkspaceLocationRequest, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, HunkExtensionAPI, diff --git a/src/ui/App.tsx b/src/ui/App.tsx index e7ded3d47..f6ee0751c 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -16,7 +16,6 @@ import { } from "react"; import type { PersistedViewPreferences } from "../core/run/config"; import { experimentalFeatureEnabled, resolveExperimentalDiffFiles } from "../core/run/experimental"; -import { isVcsReviewInput } from "../core/vcs"; import { DEFAULT_FILE_GAP, DEFAULT_HUNK_GAP } from "../core/run/reviewGap"; import { DEFAULT_TAB_WIDTH } from "../core/run/tabWidth"; import type { AppBootstrap } from "../core/bootstrap"; @@ -56,6 +55,7 @@ import { import { useAppKeyboardShortcuts } from "./hooks/useAppKeyboardShortcuts"; import { useCurrentReviewRefreshController } from "./hooks/useCurrentReviewRefreshController"; import { useExtensionCommandRunner } from "./hooks/useExtensionCommandRunner"; +import { useExtensionAppController } from "./hooks/useExtensionAppController"; import { useExtensionDialogController } from "./hooks/useExtensionDialogController"; import { useExtensionEventContextProvider } from "./hooks/useExtensionEventContextProvider"; import { useExtensionNotifications } from "./hooks/useExtensionNotifications"; @@ -510,10 +510,6 @@ export function App({ const extensionWorkspaceController = useExtensionWorkspaceControls({ createExtensionDialogs, createReviewCapabilityLease, - editorBasePath: isVcsReviewInput(bootstrap.input) - ? (bootstrap.reloadContext.repoRoot ?? bootstrap.changeset.sourceLabel) - : undefined, - editorRenderer: renderer, files: reviewFiles, input: bootstrap.input, onWorkspaceWriteCompleted, @@ -521,6 +517,10 @@ export function App({ runWorkspaceWrite, workspaceFileWriter, }); + const extensionAppController = useExtensionAppController({ + createReviewCapabilityLease, + renderer, + }); useExtensionEventContextProvider({ createDialogs: createExtensionDialogs, @@ -536,6 +536,7 @@ export function App({ createKeyboardModeControls, createLineHighlightControls, createNavigation: createExtensionNavigation, + createOpenInApp: extensionAppController.createOpenInApp, createPaneControls, createReviewControls: createExtensionReviewControls, createWorkspaceControls: extensionWorkspaceController.createWorkspaceControls, diff --git a/src/ui/AppHost.edit-in-editor.test.tsx b/src/ui/AppHost.edit-in-editor.test.tsx index a6702dead..f950323ec 100644 --- a/src/ui/AppHost.edit-in-editor.test.tsx +++ b/src/ui/AppHost.edit-in-editor.test.tsx @@ -124,7 +124,7 @@ describe("AppHost edit-selected-file shortcut", () => { await pressKeys(setup, "e"); - // The bundled editor extension reports the host capability's refusal. + // The bundled editor extension owns editor configuration and reports its refusal. expect(setup.captureCharFrame()).toContain("EDITOR is not set"); }); diff --git a/src/ui/currentReviewRefresh.ts b/src/ui/currentReviewRefresh.ts index debdf5ba9..96b82bd28 100644 --- a/src/ui/currentReviewRefresh.ts +++ b/src/ui/currentReviewRefresh.ts @@ -1,8 +1,8 @@ /** * Describes how the currently mounted review can be rebuilt from its original input. * - * Manual refresh, watch mode, editor return, extension trust reloads, and completed workspace - * writes all reuse this descriptor. It reapplies live view options so a soft reload does not + * Manual refresh, watch mode, extension trust reloads, and completed workspace writes all reuse + * this descriptor. It reapplies live view options so a soft reload does not * fall back to launch-time settings, and it supplies a source path only for VCS-backed reviews. * * Stdin-backed inputs remain non-reloadable because refreshing must not attempt to reread diff --git a/src/ui/hooks/useCurrentReviewRefreshController.ts b/src/ui/hooks/useCurrentReviewRefreshController.ts index 05c78046a..1afa12717 100644 --- a/src/ui/hooks/useCurrentReviewRefreshController.ts +++ b/src/ui/hooks/useCurrentReviewRefreshController.ts @@ -1,8 +1,8 @@ /** * Coordinates every in-session refresh of the currently mounted review. * - * Watch changes, manual commands, editor return, extension trust grants, and completed workspace - * writes converge on the same reloadable review descriptor. This hook derives and registers that + * Watch changes, manual commands, extension trust grants, and completed workspace writes converge + * on the same reloadable review descriptor. This hook derives and registers that * descriptor, connects watch notifications to refreshes, and exposes stable refresh callbacks to * App. * diff --git a/src/ui/hooks/useExtensionAppController.test.tsx b/src/ui/hooks/useExtensionAppController.test.tsx new file mode 100644 index 000000000..ac112752f --- /dev/null +++ b/src/ui/hooks/useExtensionAppController.test.tsx @@ -0,0 +1,191 @@ +import { describe, expect, mock, test } from "bun:test"; +import { testRender } from "@opentui/react/test-utils"; +import { act } from "react"; +import { useExtensionAppController } from "./useExtensionAppController"; + +/** Build one renderer identity whose terminal ownership can outlive hook mounts. */ +function createTestAppRenderer() { + return { + destroyed: false, + resume: mock(() => {}), + suspend: mock(() => {}), + renderer: null as unknown as { + readonly isDestroyed: boolean; + resume: () => void; + suspend: () => void; + }, + }; +} + +/** Mount app controls with mutable review authority and a traced renderer. */ +async function renderController(appRenderer = createTestAppRenderer()) { + let live = true; + let controller!: ReturnType; + appRenderer.renderer ||= { + get isDestroyed() { + return appRenderer.destroyed; + }, + suspend: appRenderer.suspend, + resume: appRenderer.resume, + }; + + function Harness() { + controller = useExtensionAppController({ + createReviewCapabilityLease: () => ({ isLive: () => live }), + renderer: appRenderer.renderer, + }); + return null; + } + + const setup = await testRender(, { width: 20, height: 2 }); + await act(async () => setup.renderOnce()); + return { + controller: () => controller, + destroyRenderer: () => { + appRenderer.destroyed = true; + }, + resume: appRenderer.resume, + retire: () => { + live = false; + }, + setup, + suspend: appRenderer.suspend, + }; +} + +describe("useExtensionAppController", () => { + test("suspends around extension work and passes its result through", async () => { + const harness = await renderController(); + const openInApp = harness.controller().createOpenInApp(); + const calls: string[] = []; + + try { + await expect( + openInApp(async () => { + calls.push("app"); + return 42; + }), + ).resolves.toBe(42); + expect(calls).toEqual(["app"]); + expect(harness.suspend).toHaveBeenCalledTimes(1); + expect(harness.resume).toHaveBeenCalledTimes(1); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("restores after application failures without replacing their error", async () => { + const harness = await renderController(); + const openInApp = harness.controller().createOpenInApp(); + const failure = new Error("app failed"); + + try { + await expect( + openInApp(() => { + throw failure; + }), + ).rejects.toBe(failure); + expect(harness.resume).toHaveBeenCalledTimes(1); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("refuses stale and concurrent handoffs before invoking extension code", async () => { + const harness = await renderController(); + const stale = harness.controller().createOpenInApp(); + harness.retire(); + + try { + let staleRuns = 0; + await expect( + stale(() => { + staleRuns += 1; + }), + ).rejects.toThrow("after the review reloads"); + expect(staleRuns).toBe(0); + + const currentHarness = await renderController(); + try { + let finish!: () => void; + const waiting = new Promise((resolve) => { + finish = resolve; + }); + const first = currentHarness.controller().createOpenInApp(); + const second = currentHarness.controller().createOpenInApp(); + const active = first(async () => await waiting); + await expect(second(() => "never")).rejects.toThrow("another application owns"); + finish(); + await active; + } finally { + await act(async () => currentHarness.setup.renderer.destroy()); + } + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("keeps terminal ownership across controller remounts", async () => { + const renderer = createTestAppRenderer(); + const firstHarness = await renderController(renderer); + const secondHarness = await renderController(renderer); + let finish!: () => void; + const waiting = new Promise((resolve) => { + finish = resolve; + }); + + try { + const active = firstHarness.controller().createOpenInApp()(async () => await waiting); + await expect(secondHarness.controller().createOpenInApp()(() => "never")).rejects.toThrow( + "another application owns", + ); + finish(); + await active; + expect(renderer.suspend).toHaveBeenCalledTimes(1); + expect(renderer.resume).toHaveBeenCalledTimes(1); + } finally { + await act(async () => firstHarness.setup.renderer.destroy()); + await act(async () => secondHarness.setup.renderer.destroy()); + } + }); + + test("does not resume a renderer destroyed while the app owns the terminal", async () => { + const harness = await renderController(); + const openInApp = harness.controller().createOpenInApp(); + + try { + await openInApp(() => harness.destroyRenderer()); + expect(harness.resume).not.toHaveBeenCalled(); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("does not replace the application's result when renderer restoration fails", async () => { + let controller!: ReturnType; + function Harness() { + controller = useExtensionAppController({ + createReviewCapabilityLease: () => ({ isLive: () => true }), + renderer: { + isDestroyed: false, + suspend: () => {}, + resume: () => { + throw new Error("resume failed"); + }, + }, + }); + return null; + } + const setup = await testRender(, { width: 20, height: 2 }); + await act(async () => setup.renderOnce()); + const originalError = console.error; + console.error = mock(() => {}); + try { + await expect(controller.createOpenInApp()(() => "app result")).resolves.toBe("app result"); + expect(console.error).toHaveBeenCalled(); + } finally { + console.error = originalError; + await act(async () => setup.renderer.destroy()); + } + }); +}); diff --git a/src/ui/hooks/useExtensionAppController.ts b/src/ui/hooks/useExtensionAppController.ts new file mode 100644 index 000000000..f089465a9 --- /dev/null +++ b/src/ui/hooks/useExtensionAppController.ts @@ -0,0 +1,54 @@ +import type { CliRenderer } from "@opentui/core"; +import { useCallback, useMemo } from "react"; +import type { ExtensionCommandContext } from "../../extension-api/types"; +import type { ExtensionCapabilityLease } from "../lib/extensionCapabilityLease"; + +/** The terminal handoff capability installed on one extension command context. */ +export type ExtensionOpenInApp = ExtensionCommandContext["openInApp"]; + +const activeAppByRenderer = new WeakMap(); + +/** Build command-scoped app handoffs around one renderer's terminal ownership. */ +export function useExtensionAppController({ + createReviewCapabilityLease, + renderer, +}: { + createReviewCapabilityLease: () => ExtensionCapabilityLease; + renderer: Pick; +}) { + const createOpenInApp = useCallback((): ExtensionOpenInApp => { + const lease = createReviewCapabilityLease(); + return async (run: () => Result | PromiseLike): Promise => { + if (typeof run !== "function") { + throw new Error("openInApp requires an application callback."); + } + if (!lease.isLive()) { + throw new Error("openInApp is unavailable after the review reloads."); + } + if (activeAppByRenderer.has(renderer)) { + throw new Error("openInApp is unavailable while another application owns the terminal."); + } + + const ownership = {}; + activeAppByRenderer.set(renderer, ownership); + let suspended = false; + try { + renderer.suspend(); + suspended = true; + return await run(); + } finally { + const stillOwnsTerminal = activeAppByRenderer.get(renderer) === ownership; + if (stillOwnsTerminal) activeAppByRenderer.delete(renderer); + if (stillOwnsTerminal && suspended && !renderer.isDestroyed) { + try { + renderer.resume(); + } catch (error) { + console.error("Failed to restore Hunk after an extension application.", error); + } + } + } + }; + }, [createReviewCapabilityLease, renderer]); + + return useMemo(() => ({ createOpenInApp }), [createOpenInApp]); +} diff --git a/src/ui/hooks/useExtensionCommandRunner.test.tsx b/src/ui/hooks/useExtensionCommandRunner.test.tsx index 39949918c..85c530d2d 100644 --- a/src/ui/hooks/useExtensionCommandRunner.test.tsx +++ b/src/ui/hooks/useExtensionCommandRunner.test.tsx @@ -31,6 +31,7 @@ const selection = Object.freeze({ hunkIndex: null, currentLine: null, }) as ExtensionReviewSelection; +const openInApp = async (run: () => Result | PromiseLike) => await run(); /** Mount the command runner and expose its stable invocation callback. */ async function renderRunner({ @@ -50,6 +51,7 @@ async function renderRunner({ createKeyboardModeControls: () => keyboardModes, createLineHighlightControls: () => highlights, createNavigation: () => navigation, + createOpenInApp: () => openInApp, createPaneControls: createPanes, createReviewControls: () => review, createWorkspaceControls: () => workspace, @@ -92,6 +94,7 @@ describe("useExtensionCommandRunner", () => { highlights, keyboardModes, navigation, + openInApp, panes, review, selection, diff --git a/src/ui/hooks/useExtensionCommandRunner.ts b/src/ui/hooks/useExtensionCommandRunner.ts index f1812d1ca..14014c1ec 100644 --- a/src/ui/hooks/useExtensionCommandRunner.ts +++ b/src/ui/hooks/useExtensionCommandRunner.ts @@ -22,6 +22,7 @@ import type { ExtensionWorkspace, } from "../../extension-api/types"; import type { ExtensionLoadResult, RegisteredCommand } from "../../extensions/types"; +import type { ExtensionOpenInApp } from "./useExtensionAppController"; /** Describe an extension command failure without assuming an Error instance. */ function commandFailureMessage(registered: RegisteredCommand, error: unknown) { @@ -39,6 +40,7 @@ export function useExtensionCommandRunner({ createKeyboardModeControls, createLineHighlightControls, createNavigation, + createOpenInApp, createPaneControls, createReviewControls, createWorkspaceControls, @@ -54,6 +56,7 @@ export function useExtensionCommandRunner({ ) => ExtensionKeyboardModeControls; createLineHighlightControls: (extensionId: string) => ExtensionLineHighlightControls; createNavigation: (extensionId: string) => ExtensionReviewNavigation; + createOpenInApp: () => ExtensionOpenInApp; createPaneControls: (extensionId: string) => ExtensionPaneControls; createReviewControls: () => ExtensionReviewControls; createWorkspaceControls: (extensionId: string) => ExtensionWorkspace; @@ -74,6 +77,7 @@ export function useExtensionCommandRunner({ commands: commandControls, keyboardModes: createKeyboardModeControls(registered.extensionId, extensions?.registry), notify: (message, type) => extensions?.context.notify(message, type), + openInApp: createOpenInApp(), panes, sidebars: panes, fileViews: createFileViewControls(registered.extensionId), @@ -101,6 +105,7 @@ export function useExtensionCommandRunner({ createKeyboardModeControls, createLineHighlightControls, createNavigation, + createOpenInApp, createPaneControls, createReviewControls, createWorkspaceControls, diff --git a/src/ui/hooks/useExtensionWorkspaceControls.test.tsx b/src/ui/hooks/useExtensionWorkspaceControls.test.tsx index 1a7f93382..142043940 100644 --- a/src/ui/hooks/useExtensionWorkspaceControls.test.tsx +++ b/src/ui/hooks/useExtensionWorkspaceControls.test.tsx @@ -21,13 +21,8 @@ const EXPIRED = { } as const; const WRITABLE_INPUT: CliInput = { kind: "vcs", staged: false, options: {} }; const tempDirs: string[] = []; -const originalEditor = process.env.EDITOR; -const originalSpawnSync = Bun.spawnSync; afterEach(() => { - if (originalEditor === undefined) delete process.env.EDITOR; - else process.env.EDITOR = originalEditor; - (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = originalSpawnSync; for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); @@ -61,11 +56,6 @@ async function renderController({ return true; }, workspaceFileWriter, - editorRenderer = { - isDestroyed: false, - resume: () => {}, - suspend: () => {}, - }, }: { confirm?: (options: ExtensionConfirmOptions, extensionId: string) => Promise; files?: readonly WorkspaceFileSource[]; @@ -74,11 +64,6 @@ async function renderController({ root?: string; runWorkspaceWrite?: WorkspaceWriteRunner; workspaceFileWriter?: WorkspaceFileWriter; - editorRenderer?: { - isDestroyed: boolean; - resume(): void; - suspend(): void; - }; } = {}) { let live = true; let controller!: ReturnType; @@ -101,8 +86,6 @@ async function renderController({ controller = useExtensionWorkspaceControls({ createExtensionDialogs, createReviewCapabilityLease, - editorRenderer, - editorBasePath: liveInputs.root, ...liveInputs, onWorkspaceWriteCompleted, runWorkspaceWrite, @@ -240,100 +223,19 @@ describe("useExtensionWorkspaceControls reads", () => { await destroy(harness.setup); } }); -}); -describe("useExtensionWorkspaceControls editor launches", () => { - test("opens only a reviewed file and reconciles after success", async () => { + test("resolves live app locations and makes retained resolvers inert", async () => { const root = createTestRoot(); - process.env.EDITOR = "code"; - const spawnCalls: string[][] = []; - (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = ((commands: string[]) => { - spawnCalls.push(commands); - return { exitCode: 0 }; - }) as unknown as typeof Bun.spawnSync; - let reconciliations = 0; - const harness = await renderController({ - root, - onWorkspaceWriteCompleted: () => { - reconciliations += 1; - }, - }); + const harness = await renderController({ root }); const workspace = harness.controller().createWorkspaceControls("probe"); try { - await expect( - workspace.openInEditor({ - fileId: "alpha", - hunkIndex: 0, - line: { side: "new", line: 3 }, - }), - ).resolves.toEqual({ ok: true }); - expect(spawnCalls).toEqual([["code", "--goto", `${join(root, "alpha.txt")}:3`]]); - expect(reconciliations).toBe(1); - - await expect(workspace.openInEditor({ fileId: "missing" })).resolves.toMatchObject({ - ok: false, - reason: "unavailable", + expect(workspace.resolveLocation({ fileId: "alpha" })).toEqual({ + path: join(root, "alpha.txt"), + line: 1, }); - expect(spawnCalls).toHaveLength(1); - } finally { - await destroy(harness.setup); - } - }); - - test("keeps retained editor controls inert after their review retires", async () => { - process.env.EDITOR = "code"; - let spawns = 0; - (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = (() => { - spawns += 1; - return { exitCode: 0 }; - }) as unknown as typeof Bun.spawnSync; - const harness = await renderController(); - const workspace = harness.controller().createWorkspaceControls("probe"); - harness.retire(); - - try { - await expect(workspace.openInEditor({ fileId: "alpha" })).resolves.toEqual(EXPIRED); - expect(spawns).toBe(0); - } finally { - await destroy(harness.setup); - } - }); - - test("derives the owning hunk for an old-side line and rejects a mismatched hunk", async () => { - const root = createTestRoot(); - process.env.EDITOR = "vim"; - const spawnCalls: string[][] = []; - (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = ((commands: string[]) => { - spawnCalls.push(commands); - return { exitCode: 1 }; - }) as unknown as typeof Bun.spawnSync; - const diff = createTestDiffFile({ - id: "alpha", - path: "alpha.txt", - before: "one\ntwo\nthree\nfour\n", - after: "one\nfour\n", - }); - const harness = await renderController({ - root, - files: [{ ...diff, sourceFetcher: { getFullText: async () => null } }], - }); - const workspace = harness.controller().createWorkspaceControls("probe"); - - try { - await expect( - workspace.openInEditor({ fileId: "alpha", line: { side: "old", line: 3 } }), - ).resolves.toMatchObject({ ok: false, reason: "failed" }); - expect(spawnCalls).toEqual([["vim", "+2", join(root, "alpha.txt")]]); - - await expect( - workspace.openInEditor({ - fileId: "alpha", - hunkIndex: 0, - line: { side: "old", line: 99 }, - }), - ).resolves.toMatchObject({ ok: false, reason: "unavailable" }); - expect(spawnCalls).toHaveLength(1); + harness.retire(); + expect(workspace.resolveLocation({ fileId: "alpha" })).toBeNull(); } finally { await destroy(harness.setup); } diff --git a/src/ui/hooks/useExtensionWorkspaceControls.ts b/src/ui/hooks/useExtensionWorkspaceControls.ts index 2b3631d69..40b8c0d89 100644 --- a/src/ui/hooks/useExtensionWorkspaceControls.ts +++ b/src/ui/hooks/useExtensionWorkspaceControls.ts @@ -10,25 +10,19 @@ import type { ExtensionDialogs, ExtensionFileSide, ExtensionWorkspace, - ExtensionWorkspaceOpenInEditorRequest, - ExtensionWorkspaceOpenInEditorResult, + ExtensionWorkspaceLocationRequest, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, } from "../../extension-api/types"; import type { ExtensionCapabilityLease } from "../lib/extensionCapabilityLease"; import { + normalizeWorkspaceLocationRequest, normalizeWorkspaceWriteRequest, - normalizeWorkspaceOpenInEditorRequest, resolveExtensionWorkspaceRead, + resolveExtensionWorkspaceLocation, resolveExtensionWorkspaceWriteTarget, type WorkspaceFileSource, } from "../lib/extensionWorkspace"; -import { - openSelectedFileInEditor, - type EditorDiffFile, - type EditorDiffHunk, -} from "../lib/openInEditor"; -import type { CliRenderer } from "@opentui/core"; import { verifyWorkspaceWriteTarget } from "../lib/workspaceWriteGuard"; /** Filesystem write implementation used by the host-mediated extension workspace. */ @@ -57,22 +51,11 @@ function expiredWorkspaceWrite(): ExtensionWorkspaceWriteResult { }; } -/** Describe an editor request retired before Hunk starts its host operation. */ -function expiredWorkspaceEditor(): ExtensionWorkspaceOpenInEditorResult { - return { - ok: false, - reason: "unavailable", - detail: "The review reloaded before this extension operation could finish.", - }; -} - /** Own live reviewed-document inputs and host-mediated extension workspace operations. */ export function useExtensionWorkspaceControls({ createExtensionDialogs, createReviewCapabilityLease, files, - editorBasePath, - editorRenderer, input, onWorkspaceWriteCompleted, root, @@ -85,10 +68,6 @@ export function useExtensionWorkspaceControls({ createReviewCapabilityLease: () => ExtensionCapabilityLease; /** Every current reviewed file, including files hidden by filtering. */ files: readonly WorkspaceFileSource[]; - /** Base path used to resolve reviewed paths to their working-tree counterparts. */ - editorBasePath?: string; - /** Renderer lifecycle retained by the host while terminal editors run. */ - editorRenderer: Pick; /** The current CLI review input that decides whether writes are meaningful. */ input: CliInput; /** Reconcile the review currently mounted by the host after a successful write. */ @@ -99,8 +78,8 @@ export function useExtensionWorkspaceControls({ runWorkspaceWrite: WorkspaceWriteRunner; workspaceFileWriter?: WorkspaceFileWriter; }): ExtensionWorkspaceControlsController { - const liveInputsRef = useRef({ editorBasePath, files, input, root }); - liveInputsRef.current = { editorBasePath, files, input, root }; + const liveInputsRef = useRef({ files, input, root }); + liveInputsRef.current = { files, input, root }; const createWorkspaceControls = useCallback( (extensionId: string): ExtensionWorkspace => { @@ -122,80 +101,15 @@ export function useExtensionWorkspaceControls({ const document = read ? await read().catch(() => null) : null; return lease.isLive() ? document : null; }, - async openInEditor( - request: ExtensionWorkspaceOpenInEditorRequest, - ): Promise { - const { fileId, hunkIndex, line } = normalizeWorkspaceOpenInEditorRequest(request); - if (!lease.isLive()) return expiredWorkspaceEditor(); - - const file = liveInputsRef.current.files.find((candidate) => candidate.id === fileId); - if (!file) { - return { - ok: false, - reason: "unavailable", - detail: `No reviewed file has the id "${fileId}".`, - }; - } - - const metadata = file.metadata as Partial | undefined; - if (!metadata || !Array.isArray(metadata.hunks) || typeof metadata.type !== "string") { - return { - ok: false, - reason: "unavailable", - detail: `${file.path} has no editable diff metadata.`, - }; - } - const editorFile = file as WorkspaceFileSource & EditorDiffFile; - let resolvedHunkIndex = hunkIndex; - if (line?.side === "old" && editorFile.metadata.type !== "deleted") { - resolvedHunkIndex ??= editorFile.metadata.hunks.findIndex( - (hunk) => - hunk.deletionCount > 0 && - line.line >= hunk.deletionStart && - line.line < hunk.deletionStart + hunk.deletionCount, - ); - if (resolvedHunkIndex < 0) resolvedHunkIndex = undefined; - } - const selectedHunk = - resolvedHunkIndex === undefined - ? undefined - : editorFile.metadata.hunks[resolvedHunkIndex]; - if (resolvedHunkIndex !== undefined && !selectedHunk) { - return { - ok: false, - reason: "unavailable", - detail: `${file.path} has no hunk at index ${resolvedHunkIndex}.`, - }; - } - if ( - line?.side === "old" && - editorFile.metadata.type !== "deleted" && - (!selectedHunk || - line.line < selectedHunk.deletionStart || - line.line >= selectedHunk.deletionStart + selectedHunk.deletionCount) - ) { - return { - ok: false, - reason: "unavailable", - detail: `${file.path} old line ${line.line} does not belong to the requested hunk.`, - }; - } - - const result = openSelectedFileInEditor({ - basePath: liveInputsRef.current.editorBasePath, - file: editorFile, - lineCursor: line - ? { - fileId, - hunkIndex: resolvedHunkIndex ?? 0, - target: line, - } - : undefined, - renderer: editorRenderer, - selectedHunk: selectedHunk as EditorDiffHunk | undefined, + resolveLocation(request: ExtensionWorkspaceLocationRequest) { + const normalized = normalizeWorkspaceLocationRequest(request); + if (!lease.isLive()) return null; + return resolveExtensionWorkspaceLocation({ + files: liveInputsRef.current.files, + input: liveInputsRef.current.input, + request: normalized, + root: liveInputsRef.current.root, }); - if (result.ok) onWorkspaceWriteCompleted(); - return result; }, canWriteDocument(fileId: string) { // An affordance probe answers false rather than throwing for malformed ids. @@ -273,7 +187,6 @@ export function useExtensionWorkspaceControls({ [ createExtensionDialogs, createReviewCapabilityLease, - editorRenderer, onWorkspaceWriteCompleted, runWorkspaceWrite, workspaceFileWriter, diff --git a/src/ui/lib/extensionWorkspace.test.ts b/src/ui/lib/extensionWorkspace.test.ts index 1a3415b64..0f6f4a6b1 100644 --- a/src/ui/lib/extensionWorkspace.test.ts +++ b/src/ui/lib/extensionWorkspace.test.ts @@ -1,16 +1,23 @@ import { join, resolve, sep } from "node:path"; import { describe, expect, test } from "bun:test"; +import { createTestDiffFile } from "../../../test/helpers/diff-helpers"; import type { CliInput, CommonOptions } from "../../core/run/commandInputs"; import { - normalizeWorkspaceOpenInEditorRequest, + normalizeWorkspaceLocationRequest, normalizeWorkspaceWriteRequest, resolveExtensionWorkspaceRead, + resolveExtensionWorkspaceLocation, resolveExtensionWorkspaceWriteTarget, type WorkspaceFileSource, } from "./extensionWorkspace"; const ROOT = resolve(sep, "repo"); const NO_OPTIONS: CommonOptions = {}; +const WORKING_TREE_INPUT = { + kind: "vcs", + staged: false, + options: NO_OPTIONS, +} satisfies CliInput; /** One reviewed file as the workspace policy sees it, changed unless told otherwise. */ function createTestWorkspaceFile( @@ -242,37 +249,126 @@ describe("extension workspace write requests", () => { }); }); -describe("extension workspace editor requests", () => { - test("copies a well-formed reviewed source address", () => { +describe("extension workspace locations", () => { + test("resolves the repository path and maps old-side lines from parsed hunk metadata", () => { + const file = createTestDiffFile({ + id: "alpha", + path: "packages/app/alpha.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + }); + expect( - normalizeWorkspaceOpenInEditorRequest({ - fileId: "alpha", - hunkIndex: 2, - line: { side: "old", line: 17 }, + resolveExtensionWorkspaceLocation({ + files: [file], + input: WORKING_TREE_INPUT, + request: { fileId: "alpha", hunkIndex: 0, line: { side: "old", line: 3 } }, + root: ROOT, }), - ).toEqual({ - fileId: "alpha", - hunkIndex: 2, - line: { side: "old", line: 17 }, - }); + ).toEqual({ path: join(ROOT, "packages", "app", "alpha.ts"), line: 2 }); }); - test("rejects malformed ids, indexes, and source lines", () => { - expect(() => normalizeWorkspaceOpenInEditorRequest(undefined)).toThrow("non-empty fileId"); - expect(() => normalizeWorkspaceOpenInEditorRequest({ fileId: "alpha", hunkIndex: -1 })).toThrow( + test("rejects malformed source addresses and returns null for unavailable metadata", () => { + expect(() => normalizeWorkspaceLocationRequest(undefined)).toThrow("non-empty fileId"); + expect(() => normalizeWorkspaceLocationRequest({ fileId: "alpha", hunkIndex: -1 })).toThrow( "non-negative integer", ); expect(() => - normalizeWorkspaceOpenInEditorRequest({ + normalizeWorkspaceLocationRequest({ fileId: "alpha", line: { side: "both", line: 1 }, }), ).toThrow('line.side must be "old" or "new"'); - expect(() => - normalizeWorkspaceOpenInEditorRequest({ - fileId: "alpha", - line: { side: "new", line: 0 }, + expect( + resolveExtensionWorkspaceLocation({ + files: [createTestWorkspaceFile({ metadata: undefined })], + input: WORKING_TREE_INPUT, + request: { fileId: "alpha" }, + root: ROOT, + }), + ).toBeNull(); + }); + + test("preserves old-side offsets through context and multi-line replacements", () => { + const removed = createTestDiffFile({ + id: "removed", + path: "removed.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + context: 1, + }); + const replaced = createTestDiffFile({ + id: "replaced", + path: "replaced.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nTWO\nTHREE\nfour\n", + }); + + expect( + resolveExtensionWorkspaceLocation({ + files: [removed], + input: WORKING_TREE_INPUT, + request: { fileId: "removed", hunkIndex: 0, line: { side: "old", line: 3 } }, + root: ROOT, + })?.line, + ).toBe(2); + expect( + resolveExtensionWorkspaceLocation({ + files: [replaced], + input: WORKING_TREE_INPUT, + request: { fileId: "replaced", hunkIndex: 0, line: { side: "old", line: 3 } }, + root: ROOT, + })?.line, + ).toBe(3); + }); + + test("uses direct comparison provenance and refuses unattested patch paths", () => { + const file = createTestDiffFile({ id: "alpha", path: "after.ts" }); + const directInput = { + kind: "diff", + left: "nested/before.ts", + right: "nested/after.ts", + options: NO_OPTIONS, + } satisfies CliInput; + + expect( + resolveExtensionWorkspaceLocation({ + files: [file], + input: directInput, + request: { fileId: "alpha", line: { side: "new", line: 2 } }, + root: ROOT, + }), + ).toEqual({ path: join(ROOT, "nested", "after.ts"), line: 2 }); + expect( + resolveExtensionWorkspaceLocation({ + files: [file], + input: { kind: "patch", text: file.patch, options: NO_OPTIONS }, + request: { fileId: "alpha" }, + root: ROOT, + }), + ).toBeNull(); + }); + + test("resolves deleted direct comparisons to their old-side source", () => { + const file = createTestDiffFile({ + id: "deleted", + path: "deleted.ts", + before: "one\ntwo\n", + after: "", + }); + + expect( + resolveExtensionWorkspaceLocation({ + files: [file], + input: { + kind: "diff", + left: "archive/deleted.ts", + right: "/dev/null", + options: NO_OPTIONS, + }, + request: { fileId: "deleted", hunkIndex: 0, line: { side: "old", line: 2 } }, + root: ROOT, }), - ).toThrow("positive integer"); + ).toEqual({ path: join(ROOT, "archive", "deleted.ts"), line: 2 }); }); }); diff --git a/src/ui/lib/extensionWorkspace.ts b/src/ui/lib/extensionWorkspace.ts index f9ed14bfe..18dfcf9eb 100644 --- a/src/ui/lib/extensionWorkspace.ts +++ b/src/ui/lib/extensionWorkspace.ts @@ -23,6 +23,7 @@ import type { FileSourceSide } from "../../core/changeset/fileSource"; import { canReloadInput } from "../../core/run/inputReload"; import type { CliInput } from "../../core/run/commandInputs"; import { readMetadataChangeType } from "../../extensions/events"; +import type { ExtensionWorkspaceLocation } from "../../extension-api/types"; /** * The slice of one reviewed file the workspace policy inspects. @@ -66,36 +67,50 @@ export interface WorkspaceWriteRequestFields { text: string; } -/** A normalized editor request, once its source address is known to be well-formed. */ -export interface WorkspaceOpenInEditorRequestFields { +/** A validated reviewed source address ready for workspace resolution. */ +export interface WorkspaceLocationRequestFields { fileId: string; hunkIndex?: number; line?: { side: FileSourceSide; line: number }; } -/** Reject malformed editor requests before they reach renderer or process ownership. */ -export function normalizeWorkspaceOpenInEditorRequest( +interface WorkspaceLocationHunk { + deletionStart: number; + deletionCount: number; + additionStart: number; + additionCount: number; + hunkContent: Array< + { type: "context"; lines: number } | { type: "change"; deletions: number; additions: number } + >; +} + +interface WorkspaceLocationMetadata { + type: string; + hunks: WorkspaceLocationHunk[]; +} + +/** Reject malformed app-location metadata before it reaches workspace state. */ +export function normalizeWorkspaceLocationRequest( request: unknown, -): WorkspaceOpenInEditorRequestFields { - const fields = request as Partial | null | undefined; +): WorkspaceLocationRequestFields { + const fields = request as Partial | null | undefined; if (typeof fields?.fileId !== "string" || fields.fileId.length === 0) { - throw new Error("workspace.openInEditor requires a non-empty fileId."); + throw new Error("workspace.resolveLocation requires a non-empty fileId."); } if ( fields.hunkIndex !== undefined && (!Number.isInteger(fields.hunkIndex) || fields.hunkIndex < 0) ) { - throw new Error("workspace.openInEditor hunkIndex must be a non-negative integer."); + throw new Error("workspace.resolveLocation hunkIndex must be a non-negative integer."); } if (fields.line !== undefined) { if (fields.line.side !== "old" && fields.line.side !== "new") { - throw new Error('workspace.openInEditor line.side must be "old" or "new".'); + throw new Error('workspace.resolveLocation line.side must be "old" or "new".'); } if (!Number.isInteger(fields.line.line) || fields.line.line < 1) { - throw new Error("workspace.openInEditor line.line must be a positive integer."); + throw new Error("workspace.resolveLocation line.line must be a positive integer."); } } - return { fileId: fields.fileId, ...(fields.hunkIndex === undefined ? {} : { hunkIndex: fields.hunkIndex }), @@ -105,6 +120,97 @@ export function normalizeWorkspaceOpenInEditorRequest( }; } +/** Translate one old-side line to its corresponding working-tree line. */ +function lineOnDisk(hunk: WorkspaceLocationHunk, deletionLine: number) { + let deletionCursor = hunk.deletionStart; + let additionCursor = hunk.additionCount === 0 ? hunk.additionStart + 1 : hunk.additionStart; + + for (const content of hunk.hunkContent) { + if (content.type === "context") { + if (deletionLine < deletionCursor + content.lines) { + return additionCursor + (deletionLine - deletionCursor); + } + deletionCursor += content.lines; + additionCursor += content.lines; + continue; + } + if (deletionLine < deletionCursor + content.deletions) { + const offset = Math.min(deletionLine - deletionCursor, Math.max(content.additions - 1, 0)); + return additionCursor + offset; + } + deletionCursor += content.deletions; + additionCursor += content.additions; + } + return additionCursor; +} + +/** Resolve a reviewed source address against the authoritative parsed diff. */ +export function resolveExtensionWorkspaceLocation({ + files, + input, + request, + root, +}: { + files: readonly WorkspaceFileSource[]; + input: CliInput; + request: WorkspaceLocationRequestFields; + root: string; +}): ExtensionWorkspaceLocation | null { + const file = files.find((candidate) => candidate.id === request.fileId); + if (!file) return null; + const metadata = file.metadata as Partial | undefined; + if (!metadata || typeof metadata.type !== "string" || !Array.isArray(metadata.hunks)) return null; + + let hunkIndex = request.hunkIndex; + if (request.line?.side === "old" && metadata.type !== "deleted") { + hunkIndex ??= metadata.hunks.findIndex( + (hunk) => + request.line!.line >= hunk.deletionStart && + request.line!.line < hunk.deletionStart + hunk.deletionCount, + ); + if (hunkIndex < 0) hunkIndex = undefined; + } + const hunk = hunkIndex === undefined ? undefined : metadata.hunks[hunkIndex]; + if (hunkIndex !== undefined && !hunk) return null; + + const deleted = metadata.type === "deleted"; + let line: number; + if (request.line?.side === (deleted ? "old" : "new")) { + line = request.line.line; + } else if (request.line && !deleted) { + if ( + !hunk || + request.line.line < hunk.deletionStart || + request.line.line >= hunk.deletionStart + hunk.deletionCount + ) { + return null; + } + line = lineOnDisk(hunk, request.line.line); + } else { + line = deleted ? (hunk?.deletionStart ?? 1) : (hunk?.additionStart ?? 1); + } + + let filePath: string; + if (input.kind === "patch") { + return null; + } else if (input.kind === "diff") { + const sourcePath = deleted ? input.left : input.right; + if (sourcePath === "/dev/null") return null; + filePath = resolve(root, sourcePath); + } else if (input.kind === "difftool") { + const sourcePath = input.path ?? (deleted ? input.left : input.right); + if (sourcePath === "/dev/null") return null; + filePath = resolve(root, sourcePath); + } else { + filePath = resolve(root, normalizeDiffPath(file.path) ?? file.path); + } + + return { + path: filePath, + line: Math.max(1, line), + }; +} + /** * Name what this session is reviewing when it is not the working tree. * diff --git a/src/ui/lib/openInEditor.test.ts b/src/ui/lib/openInEditor.test.ts deleted file mode 100644 index 6c3b29803..000000000 --- a/src/ui/lib/openInEditor.test.ts +++ /dev/null @@ -1,486 +0,0 @@ -import { afterEach, describe, expect, mock, test } from "bun:test"; -import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join, resolve } from "node:path"; -import { createTestDiffFile } from "../../../test/helpers/diff-helpers"; -import { - buildEditorCommand, - openSelectedFileInEditor, - resolveEditableFilePath, - shouldSuspendForEditor, -} from "./openInEditor"; - -const originalEditor = process.env.EDITOR; -const originalSpawnSync = Bun.spawnSync; -const tempDirs: string[] = []; - -function createTempDir() { - const dir = realpathSync(mkdtempSync(join(tmpdir(), "hunk-open-editor-"))); - tempDirs.push(dir); - return dir; -} - -function restoreEditorEnv() { - if (originalEditor === undefined) { - delete process.env.EDITOR; - } else { - process.env.EDITOR = originalEditor; - } -} - -function mockSpawnSync( - implementation: (cmds: string[], options?: Parameters[1]) => unknown, -) { - const mutableBun = Bun as unknown as { spawnSync: typeof Bun.spawnSync }; - mutableBun.spawnSync = implementation as typeof Bun.spawnSync; -} - -function createRenderer() { - return { - isDestroyed: false, - resume: mock(() => {}), - suspend: mock(() => {}), - }; -} - -afterEach(() => { - restoreEditorEnv(); - mockSpawnSync(originalSpawnSync); - - while (tempDirs.length > 0) { - const dir = tempDirs.pop(); - if (dir) { - rmSync(dir, { recursive: true, force: true }); - } - } -}); - -describe("open in editor helpers", () => { - test("builds vi-style editor args without shell quoting", () => { - expect( - buildEditorCommand({ - editor: "nvim", - filePath: "/tmp/project/file with spaces's.ts", - line: 12, - }), - ).toEqual({ - command: "nvim", - args: ["+12", "/tmp/project/file with spaces's.ts"], - }); - }); - - test("preserves editor flags before appending the target file", () => { - expect( - buildEditorCommand({ - editor: "code --reuse-window", - filePath: "/tmp/project/example.ts", - line: 4, - }), - ).toEqual({ - command: "code", - args: ["--reuse-window", "--goto", "/tmp/project/example.ts:4"], - }); - }); - - test("handles quoted editor commands and Windows executable paths", () => { - expect( - buildEditorCommand({ - editor: '"C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd" --wait', - filePath: "C:\\Users\\Duarte\\repo\\file with spaces.ts", - line: 7, - }), - ).toEqual({ - command: "C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd", - args: ["--wait", "--goto", "C:\\Users\\Duarte\\repo\\file with spaces.ts:7"], - }); - }); - - test("defaults unknown editors to opening the file path only", () => { - expect( - buildEditorCommand({ - editor: "zed --new-window", - filePath: "/tmp/project/example.ts", - line: 4, - }), - ).toEqual({ - command: "zed", - args: ["--new-window", "/tmp/project/example.ts"], - }); - }); - - test("does not suspend for code-style GUI editors", () => { - expect(shouldSuspendForEditor("code --reuse-window")).toBe(false); - expect(shouldSuspendForEditor('"C:\\Program Files\\Cursor\\cursor.exe"')).toBe(false); - expect(shouldSuspendForEditor("nvim")).toBe(true); - }); - - test("resolves repo-relative diff paths from the diff source path", () => { - expect(resolveEditableFilePath("src/main.tsx", "/tmp/project")).toBe( - resolve("/tmp/project", "src/main.tsx"), - ); - }); - - test("returns an error when no file is selected", () => { - const renderer = createRenderer(); - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - expect( - openSelectedFileInEditor({ - file: undefined, - renderer, - selectedHunk: undefined, - }), - ).toEqual({ ok: false, reason: "unavailable", detail: "No file selected." }); - - expect(spawnCalls).toEqual([]); - expect(renderer.suspend).not.toHaveBeenCalled(); - expect(renderer.resume).not.toHaveBeenCalled(); - }); - - test("returns an error when $EDITOR is unset", () => { - const renderer = createRenderer(); - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - delete process.env.EDITOR; - - expect( - openSelectedFileInEditor({ - file: createTestDiffFile({ path: "missing-editor.ts" }), - renderer, - selectedHunk: undefined, - }), - ).toEqual({ ok: false, reason: "unavailable", detail: "$EDITOR is not set." }); - - expect(spawnCalls).toEqual([]); - expect(renderer.suspend).not.toHaveBeenCalled(); - expect(renderer.resume).not.toHaveBeenCalled(); - }); - - test("returns an error when the file does not exist on disk", () => { - const renderer = createRenderer(); - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - process.env.EDITOR = "nvim"; - - expect( - openSelectedFileInEditor({ - basePath: createTempDir(), - file: createTestDiffFile({ path: "missing-on-disk.ts" }), - renderer, - selectedHunk: undefined, - }), - ).toEqual({ - ok: false, - reason: "unavailable", - detail: "Cannot edit missing-on-disk.ts: file does not exist on disk.", - }); - - expect(spawnCalls).toEqual([]); - expect(renderer.suspend).not.toHaveBeenCalled(); - expect(renderer.resume).not.toHaveBeenCalled(); - }); - - test("spawns terminal editors with suspend and resume around a successful edit", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "nvim --clean"; - - const spawnCalls: Array<{ - cmds: string[]; - options: Parameters[1] | undefined; - }> = []; - mockSpawnSync((cmds, options) => { - spawnCalls.push({ cmds, options }); - return { exitCode: 0 }; - }); - - const renderer = createRenderer(); - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - renderer, - selectedHunk: undefined, - }), - ).toEqual({ ok: true }); - - expect(spawnCalls).toEqual([ - { - cmds: ["nvim", "--clean", "+1", join(basePath, "example.ts")], - options: { stdin: "inherit", stdout: "inherit", stderr: "inherit" }, - }, - ]); - expect(renderer.suspend).toHaveBeenCalledTimes(1); - expect(renderer.resume).toHaveBeenCalledTimes(1); - }); - - test("opens the current line instead of the selected hunk start", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: file.id, - hunkIndex: 1, - target: { side: "new", line: 3 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[0], - }), - ).toEqual({ ok: true }); - - expect(spawnCalls).toEqual([["vim", "+3", join(basePath, "example.ts")]]); - }); - - test("maps an old-side current line onto the line on disk", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "one\nfour\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ - path: "example.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nfour\n", - }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: file.id, - hunkIndex: 0, - target: { side: "old", line: 3 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[0], - }), - ).toEqual({ ok: true }); - - expect(spawnCalls).toEqual([["vim", "+2", join(basePath, "example.ts")]]); - }); - - test("walks leading context when mapping an old-side current line", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "one\nfour\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ - path: "example.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nfour\n", - context: 1, - }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: file.id, - hunkIndex: 0, - target: { side: "old", line: 3 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[0], - }), - ).toEqual({ ok: true }); - - // Old line 3 ("three") was removed, so the editor lands on the line that now follows "one". - expect(spawnCalls).toEqual([["vim", "+2", join(basePath, "example.ts")]]); - }); - - test("preserves the deleted line's offset within a multi-line replacement", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "one\nTWO\nTHREE\nfour\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ - path: "example.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nTWO\nTHREE\nfour\n", - }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: file.id, - hunkIndex: 0, - target: { side: "old", line: 3 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[0], - }), - ).toEqual({ ok: true }); - - // Old line 3 ("three") is the second of two replaced lines, so the editor - // lands on the second replacement line ("THREE") rather than the first. - expect(spawnCalls).toEqual([["vim", "+3", join(basePath, "example.ts")]]); - }); - - test("falls back to the selected hunk when the cursor is in another file", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: "other-file", - hunkIndex: 0, - target: { side: "new", line: 42 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[1], - }), - ).toEqual({ ok: true }); - - expect(spawnCalls).toEqual([ - ["vim", `+${file.metadata.hunks[1]!.additionStart}`, join(basePath, "example.ts")], - ]); - }); - - test("uses deletion line numbers for deleted files", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "deleted.ts"), "const old = true;\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const baseFile = createTestDiffFile({ path: "deleted.ts" }); - const file = { - ...baseFile, - metadata: { - ...baseFile.metadata, - type: "deleted" as const, - }, - }; - const selectedHunk = { - ...file.metadata.hunks[0]!, - additionStart: 2, - deletionStart: 9, - }; - - expect( - openSelectedFileInEditor({ - basePath, - file, - renderer: createRenderer(), - selectedHunk, - }), - ).toEqual({ ok: true }); - - expect(spawnCalls).toEqual([["vim", "+9", join(basePath, "deleted.ts")]]); - }); - - test("does not suspend GUI editors and reports non-zero exits", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "code --wait"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 2 }; - }); - - const renderer = createRenderer(); - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - renderer, - selectedHunk: file.metadata.hunks[0], - }), - ).toEqual({ ok: false, reason: "failed", detail: "Editor exited with status 2." }); - - expect(spawnCalls).toEqual([["code", "--wait", "--goto", `${join(basePath, "example.ts")}:1`]]); - expect(renderer.suspend).not.toHaveBeenCalled(); - expect(renderer.resume).not.toHaveBeenCalled(); - }); - - test("resumes after spawn failures and reports launch errors", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "vi"; - - mockSpawnSync(() => { - throw new Error("boom"); - }); - - const renderer = createRenderer(); - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - renderer, - selectedHunk: file.metadata.hunks[0], - }), - ).toEqual({ ok: false, reason: "failed", detail: "Failed to launch editor: boom" }); - - expect(renderer.suspend).toHaveBeenCalledTimes(1); - expect(renderer.resume).toHaveBeenCalledTimes(1); - }); -}); diff --git a/src/ui/lib/openInEditor.ts b/src/ui/lib/openInEditor.ts deleted file mode 100644 index 8235e5699..000000000 --- a/src/ui/lib/openInEditor.ts +++ /dev/null @@ -1,216 +0,0 @@ -import { existsSync } from "node:fs"; -import { basename, resolve, win32 } from "node:path"; -import type { CliRenderer } from "@opentui/core"; -import type { DiffFile } from "../../core/changeset/model"; -import type { LineCursor } from "./lineCursors"; -import type { ExtensionWorkspaceOpenInEditorResult } from "../../extension-api/types"; - -export interface EditorCommand { - command: string; - args: string[]; -} - -export type EditorDiffHunk = DiffFile["metadata"]["hunks"][number]; -export type EditorDiffFile = Pick & { - metadata: Pick; -}; - -/** The review stream's current line, minus the geometry fields this module never reads. */ -export type EditorLineCursor = Pick; - -/** - * Translate an old-side line to the line it maps to in the file on disk. - * - * Deleted lines have no on-disk counterpart, so they resolve to the position their - * replacement occupies, which is where the reader expects the editor to land. - */ -function deletionLineToFileLine(hunk: EditorDiffHunk, deletionLine: number) { - let deletionCursor = hunk.deletionStart; - // A zero-count side names the line before the change, so step past it to land inside the file. - let additionCursor = hunk.additionCount === 0 ? hunk.additionStart + 1 : hunk.additionStart; - - for (const content of hunk.hunkContent) { - if (content.type === "context") { - if (deletionLine < deletionCursor + content.lines) { - return additionCursor + (deletionLine - deletionCursor); - } - - deletionCursor += content.lines; - additionCursor += content.lines; - continue; - } - - if (deletionLine < deletionCursor + content.deletions) { - // Land on the corresponding replacement line, clamped to the last one this block adds. - const offset = Math.min(deletionLine - deletionCursor, Math.max(content.additions - 1, 0)); - return additionCursor + offset; - } - - deletionCursor += content.deletions; - additionCursor += content.additions; - } - - return additionCursor; -} - -/** Prefer the current line over the selected hunk's first line. */ -function selectedLine( - file: EditorDiffFile, - selectedHunk: EditorDiffHunk | undefined, - lineCursor: EditorLineCursor | null | undefined, -) { - // Deleted files are opened against their pre-change content, every other file against its new one. - const isDeleted = file.metadata.type === "deleted"; - const diskSide = isDeleted ? "old" : "new"; - const cursor = lineCursor?.fileId === file.id ? lineCursor : undefined; - - if (cursor) { - if (cursor.target.side === diskSide) { - return cursor.target.line; - } - - const cursorHunk = file.metadata.hunks[cursor.hunkIndex]; - if (!isDeleted && cursorHunk) { - return deletionLineToFileLine(cursorHunk, cursor.target.line); - } - } - - if (isDeleted) { - return selectedHunk?.deletionStart ?? 1; - } - - return selectedHunk?.additionStart ?? 1; -} - -function splitEditorCommand(editor: string) { - return ( - editor - .match(/(?:[^\s"']+|"(?:\\.|[^"])*"|'(?:\\.|[^'])*')+/g) - ?.map((token) => token.replace(/^(["'])(.*)\1$/, "$2")) ?? [] - ); -} - -function editorProgram(editor: string) { - const [firstToken = ""] = splitEditorCommand(editor); - return basename(win32.basename(firstToken)) - .replace(/\.(?:cmd|exe)$/i, "") - .toLowerCase(); -} - -const VI_STYLE_EDITORS = ["vim", "nvim", "vi"]; -const CODE_STYLE_EDITORS = ["code", "code-insiders", "cursor"]; - -/** Suspend for terminal editors. */ -export function shouldSuspendForEditor(editor: string) { - const program = editorProgram(editor); - if (CODE_STYLE_EDITORS.includes(program)) { - return false; - } - - return true; -} - -/** Build an editor process invocation without shell quoting so paths stay cross-platform. */ -export function buildEditorCommand({ - editor, - filePath, - line, -}: { - editor: string; - filePath: string; - line: number; -}): EditorCommand { - const [command = "", ...editorArgs] = splitEditorCommand(editor); - const program = editorProgram(editor); - - if (VI_STYLE_EDITORS.includes(program)) { - return { command, args: [...editorArgs, `+${line}`, filePath] }; - } - - if (CODE_STYLE_EDITORS.includes(program)) { - return { command, args: [...editorArgs, "--goto", `${filePath}:${line}`] }; - } - - if (program == "hx") { - return { command, args: [...editorArgs, `${filePath}:${line}`] }; - } - - return { command, args: [...editorArgs, filePath] }; -} - -/** Resolve diff paths relative to their source repo instead of the launch cwd. */ -export function resolveEditableFilePath(filePath: string, basePath = process.cwd()) { - return resolve(basePath, filePath); -} - -/** Open the selected file in $EDITOR, suspending TUI for terminal editors. */ -export function openSelectedFileInEditor({ - basePath, - file, - lineCursor, - renderer, - selectedHunk, -}: { - basePath?: string; - file: EditorDiffFile | undefined; - lineCursor?: EditorLineCursor | null; - renderer: Pick; - selectedHunk: EditorDiffHunk | undefined; -}): ExtensionWorkspaceOpenInEditorResult { - if (!file) { - return { ok: false, reason: "unavailable", detail: "No file selected." }; - } - - const editor = process.env.EDITOR?.trim(); - if (!editor) { - return { ok: false, reason: "unavailable", detail: "$EDITOR is not set." }; - } - - const absolutePath = resolveEditableFilePath(file.path, basePath); - if (!existsSync(absolutePath)) { - return { - ok: false, - reason: "unavailable", - detail: `Cannot edit ${file.path}: file does not exist on disk.`, - }; - } - - const line = Math.max(1, selectedLine(file, selectedHunk, lineCursor)); - const command = buildEditorCommand({ - editor, - filePath: absolutePath, - line, - }); - - const shouldSuspend = shouldSuspendForEditor(editor); - if (shouldSuspend) { - renderer.suspend(); - } - - let exitCode = 0; - let failureMessage: string | null = null; - try { - const result = Bun.spawnSync([command.command, ...command.args], { - stdin: "inherit", - stdout: "inherit", - stderr: "inherit", - }); - exitCode = result.exitCode; - } catch (error) { - failureMessage = error instanceof Error ? error.message : String(error); - } - - if (shouldSuspend && !renderer.isDestroyed) { - renderer.resume(); - } - - if (failureMessage) { - return { ok: false, reason: "failed", detail: `Failed to launch editor: ${failureMessage}` }; - } - - if (exitCode !== 0) { - return { ok: false, reason: "failed", detail: `Editor exited with status ${exitCode}.` }; - } - - return { ok: true }; -} diff --git a/test/pty/extensions-integration.test.ts b/test/pty/extensions-integration.test.ts index ac4467832..8bf421078 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"; @@ -256,6 +256,27 @@ const DIALOG_EXTENSION_SOURCE = `export default function (hunk) { } `; +const APP_HANDOFF_CHILD_SOURCE = ` +process.stdout.write("\\x1b[2J\\x1b[HCHILD APP ACTIVE\\nreturning to Hunk\\n"); +while (!(await Bun.file(".hunk-child-release").exists())) await Bun.sleep(25); +`; + +/** An extension that gives a child process exclusive use of the real terminal. */ +const APP_HANDOFF_EXTENSION_SOURCE = `export default function (hunk) { + hunk.registerCommand({ id: "open-child", title: "Open child app", key: "y" }, async (ctx) => { + const exitCode = await ctx.openInApp(() => { + const child = Bun.spawnSync([process.execPath, "-e", ${JSON.stringify(APP_HANDOFF_CHILD_SOURCE)}], { + stdin: "inherit", + stdout: "inherit", + stderr: "inherit", + }); + return child.exitCode; + }); + ctx.notify("CHILD APP EXITED " + exitCode); + }); +} +`; + describe("PTY extensions", () => { test("trust prompt runs repo extensions after the user trusts the repository", async () => { const configHome = harness.createIsolatedConfigHome(); @@ -632,6 +653,47 @@ describe("PTY extensions", () => { } }); + test("an extension app takes over the terminal and returns to the review", async () => { + const configHome = harness.createIsolatedConfigHome(); + const fixture = harness.createRepoExtensionFixture(APP_HANDOFF_EXTENSION_SOURCE); + const session = await harness.launchHunk({ + args: [ + "diff", + "--mode", + "stack", + "--extension", + join(fixture.dir, ".hunk", "extensions", "fixture.ts"), + ], + cwd: fixture.dir, + cols: 80, + rows: 20, + env: { XDG_CONFIG_HOME: configHome }, + }); + + try { + await harness.waitForSnapshot(session, (text) => text.includes("alpha.ts"), 20_000); + await harness.ensureKeyboardIsLive(session); + + await session.press("y"); + const childFrame = await harness.waitForSnapshot( + session, + (text) => text.includes("CHILD APP ACTIVE") && text.includes("returning to Hunk"), + 20_000, + ); + expect(childFrame).not.toContain("alpha.ts"); + writeFileSync(join(fixture.dir, ".hunk-child-release"), "release"); + + const restoredFrame = await harness.waitForSnapshot( + session, + (text) => text.includes("alpha.ts") && text.includes("CHILD APP EXITED 0"), + 20_000, + ); + expect(restoredFrame).not.toContain("CHILD APP ACTIVE"); + } finally { + session.close(); + } + }); + test("the real review note navigator inventories and reveals a saved user note", async () => { const configHome = harness.createIsolatedConfigHome(); const fixture = harness.createBottomClampedRepoFixture(); diff --git a/website/src/content/docs/docs/extend/extension-api.md b/website/src/content/docs/docs/extend/extension-api.md index 1d17e1427..d47206b7d 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -8,8 +8,8 @@ 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 host-mediated editor -launches for reviewed files; version 15 added `{ side, line }` to opted-in pane +one file to support several Hunk versions. Version 16 adds temporary application +handoffs from command handlers; 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 and committed note-edit events; version 12 added responsive fractional pane @@ -334,16 +334,25 @@ Hunk draws the dialog; your text fills the title, body, and choices. Dialogs fro One dialog shows at a time; concurrent requests queue in call order, across extensions. Escape cancels (`false` or `null`), Enter accepts; confirm dialogs also answer to `y`/`n`, select dialogs to `↑`/`↓`, and everything is clickable. A session reload cancels open and queued dialogs, and a dialog pending at shutdown resolves its cancel value. +### Temporary applications + +`ctx.openInApp(callback)` temporarily hands Hunk's terminal to application code +owned by your command. Hunk suspends its renderer, awaits the callback, and +restores the review in `finally`. Your extension owns execution and how file, +line, hunk, or application state reaches the child process. One app can own the +terminal at a time, and stale or concurrent handoffs reject before the callback +runs. + ### Workspace documents -`ctx.workspace` reads full documents from the current review, opens reviewed files through Hunk's editor lifecycle, and writes eligible working-tree files. +`ctx.workspace` reads full documents from the current review and writes eligible working-tree files. -| Method | Result | -| --------------------------------------------- | ------------------------------------------------- | -| `readDocument(fileId, "old" \| "new")` | Reviewed source text or `null` | -| `openInEditor({ fileId, hunkIndex?, line? })` | `{ ok: true }` or `{ ok: false, reason, detail }` | -| `canWriteDocument(fileId)` | Whether review policy allows a write | -| `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | +| Method | Result | +| -------------------------------------- | ------------------------------------------------- | +| `readDocument(fileId, "old" \| "new")` | Reviewed source text or `null` | +| `resolveLocation({ fileId, ... })` | Absolute on-disk `{ path, line }` or `null` | +| `canWriteDocument(fileId)` | Whether review policy allows a write | +| `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | ```ts const file = ctx.selection.file; @@ -357,7 +366,7 @@ if (file && ctx.workspace.canWriteDocument(file.id)) { Reads return the source represented by the review, including historical content in revision and stash reviews. Missing, unreadable, or oversized sources return `null`; reads never prompt. -Editor requests name a reviewed file id and optional source address, never a path or process. Hunk resolves `$EDITOR`, maps old-side lines onto the working-tree file, owns terminal suspension, and reloads reloadable inputs after success. Missing configuration or files return `unavailable`; launch failures return `failed`. +`resolveLocation` maps a reviewed file id and optional hunk/source line onto an attested absolute path and line on disk. VCS reviews resolve against the repository and direct comparisons retain their concrete file path. Hunk uses parsed hunk metadata for old-side mapping; raw patches, missing hunks, and stale locations return `null`. Writes require a reloadable, unstaged working-tree review and a writable reviewed-file id. Hunk verifies the target, asks for attributed consent, verifies it again, writes it, and reloads the review. Other review kinds and deleted, binary, oversized, missing, symlinked, or root-escaping targets return `unavailable`. Cancellation returns `cancelled`; an attempted write failure returns `failed` with a displayable `detail`. @@ -365,7 +374,7 @@ Writes require a reloadable, unstaged working-tree review and a writable reviewe ## `hunk.on(event, handler)` -Subscribe to a lifecycle or UI event. Handlers may be async; Hunk never blocks the UI waiting for one. Every handler receives `ctx.panes`, live `ctx.navigation`, and attributed `ctx.dialogs` alongside `cwd` and `notify`, so a `startup` handler can present one focused welcome dialog and navigate to its first example without a keypress. `ctx.sidebars` is deprecated. Controls retained across a review or extension-registry replacement expire instead of controlling the replacement UI; workspace reads, editor launches, and writes that have not started return `null`/`unavailable`. Once a consented filesystem write starts, it reports its actual outcome, graceful shutdown waits for it, and success reconciles the review then active. +Subscribe to a lifecycle or UI event. Handlers may be async; Hunk never blocks the UI waiting for one. Every handler receives `ctx.panes`, live `ctx.navigation`, and attributed `ctx.dialogs` alongside `cwd` and `notify`, so a `startup` handler can present one focused welcome dialog and navigate to its first example without a keypress. `ctx.sidebars` is deprecated. Controls retained across a review or extension-registry replacement expire instead of controlling the replacement UI; workspace reads and writes that have not started return `null`/`unavailable`. A stale `openInApp` callback rejects before taking terminal ownership. Once a consented filesystem write starts, it reports its actual outcome, graceful shutdown waits for it, and success reconciles the review then active. | Event | Payload | When | | ---------------------- | ----------------------- | -------------------------------------------------------- | From b8320513e426e2ffd91fba92bc3e407d1a9038f6 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Mon, 31 Aug 2026 16:38:27 -0400 Subject: [PATCH 3/4] fix(extensions): harden app location handoffs --- .changeset/fuzzy-editors-dock.md | 2 +- docs/extension-architecture.md | 6 +- docs/extensions.md | 51 ++++-- skills/hunk-extensions/SKILL.md | 4 +- src/core/changeset/diffFile.test.ts | 17 ++ src/core/changeset/diffFile.ts | 11 +- src/core/changeset/fileSource.test.ts | 15 +- src/core/changeset/fileSource.ts | 27 ++- src/core/changeset/fromPatch.ts | 2 +- src/core/changeset/loaders.test.ts | 64 +++++++ src/core/changeset/loaders.ts | 58 +++++-- src/core/changeset/model.ts | 4 +- src/core/vcs/types.ts | 2 + src/core/vcs/untracked.test.ts | 1 + src/core/vcs/untracked.ts | 18 +- src/extension-api/index.ts | 1 + src/extension-api/types.ts | 12 ++ .../default/ui/editor/editorApp.test.ts | 4 + .../default/ui/editor/index.test.ts | 78 +++++++-- src/extensions/default/ui/editor/index.ts | 35 ++-- src/extensions/default/vcs/git/index.test.ts | 16 ++ src/extensions/default/vcs/git/index.ts | 55 +++--- src/extensions/default/vcs/git/source.test.ts | 13 +- src/extensions/default/vcs/git/source.ts | 5 + .../default/vcs/jujutsu/index.test.ts | 6 + src/extensions/default/vcs/jujutsu/index.ts | 4 + .../default/vcs/sapling/index.test.ts | 11 ++ src/extensions/default/vcs/sapling/index.ts | 4 + .../default/vcs/workingTreeSource.test.ts | 14 ++ .../default/vcs/workingTreeSource.ts | 15 ++ src/extensions/vcsPatchResult.test.ts | 61 +++++++ src/extensions/vcsPatchResult.ts | 48 +++++- src/ui/App.tsx | 17 +- src/ui/AppHost.edit-in-editor.test.tsx | 35 ++-- src/ui/AppHost.extension-dialogs.test.tsx | 42 +++++ .../hooks/useExtensionAppController.test.tsx | 72 ++++++-- src/ui/hooks/useExtensionAppController.ts | 13 +- src/ui/hooks/useExtensionDialogController.ts | 3 + .../useExtensionWorkspaceControls.test.tsx | 158 +++++++++++++++++- src/ui/hooks/useExtensionWorkspaceControls.ts | 43 ++++- src/ui/lib/extensionWorkspace.test.ts | 153 ++++++++++------- src/ui/lib/extensionWorkspace.ts | 35 ++-- .../content/docs/docs/extend/extension-api.md | 9 +- .../content/docs/docs/extend/vcs-adapters.md | 17 +- 44 files changed, 1021 insertions(+), 240 deletions(-) create mode 100644 src/extensions/default/vcs/workingTreeSource.test.ts create mode 100644 src/extensions/default/vcs/workingTreeSource.ts diff --git a/.changeset/fuzzy-editors-dock.md b/.changeset/fuzzy-editors-dock.md index e05add4af..1ce88f96b 100644 --- a/.changeset/fuzzy-editors-dock.md +++ b/.changeset/fuzzy-editors-dock.md @@ -2,4 +2,4 @@ "hunkdiff": minor --- -Let extension commands temporarily hand Hunk's terminal to an application and run Hunk's open-in-editor workflow as a bundled extension. +Let extension commands temporarily hand Hunk's terminal to an application, resolve filesystem-attested review locations, and run Hunk's responsive open-in-editor workflow as a bundled extension. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index 5e3b99a93..edd7c28d4 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -278,13 +278,15 @@ leases refuse stale handoffs, one shared lock prevents overlapping applications, and renderer suspension always resumes in `finally` unless the renderer was destroyed. The extension owns execution and application-specific metadata; Hunk's bundled editor command consumes the same public callback and explicitly -refreshes after a successful edit. +refreshes after a successful edit. Dialog admission and workspace writes consult +the same ownership state so host UI cannot deadlock behind a suspended renderer. `src/ui/lib/extensionWorkspace.ts` owns the policy for `ctx.workspace`. Reads resolve reviewed file ids through the existing source fetcher, which retains ownership of caching and size limits. Missing or unreadable sources become `null`. Location resolution maps reviewed file ids and source addresses onto -attested on-disk paths and lines using input provenance and the authoritative parsed hunk. +attested on-disk paths and lines using per-side provenance supplied by loaders +and VCS adapters plus the authoritative parsed hunk. Writes are limited to reloadable working-tree reviews and reviewed paths inside the review root. App supplies the current input, unfiltered changeset, and root diff --git a/docs/extensions.md b/docs/extensions.md index 8ecbf504b..1c74845d6 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -282,7 +282,8 @@ new instances and run that shutdown/startup pair around the replacement. The API generation this Hunk speaks (currently `16`). Branch on it if you want one file to support several Hunk versions. Version 16 adds temporary application -handoffs from command handlers; version 15 added `{ side, line }` to opted-in pane +handoffs and on-disk location resolution to command handlers; 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 committed note-edit events; version 12 adds responsive fractional pane sizing; version 11 added @@ -468,12 +469,13 @@ instead of a crash. A `load` result is patch text plus how to label it. Everything else on it is optional, and each optional field buys one thing: -| Field | What it adds | -| ---------------- | ------------------------------------------------------------------ | -| `untrackedPaths` | files your VCS calls unknown, synthesized into added-file diffs | -| `readFileSource` | exact whole-file contents, for context expansion and highlighting | -| `sourceCacheKey` | stable source-snapshot identity for highlight reuse across reloads | -| `extraFiles` | files reviewed outside the patch, including skipped placeholders | +| Field | What it adds | +| ----------------------- | ------------------------------------------------------------------ | +| `untrackedPaths` | files your VCS calls unknown, synthesized into added-file diffs | +| `readFileSource` | exact whole-file contents, for context expansion and highlighting | +| `resolveFileSourcePath` | exact filesystem provenance for application location handoff | +| `sourceCacheKey` | stable source-snapshot identity for highlight reuse across reloads | +| `extraFiles` | files reviewed outside the patch, including skipped placeholders | `untrackedPaths` is the shorthand: list the repo-root-relative paths your VCS reports as unknown and Hunk synthesizes the added-file diffs for you, skipping @@ -586,6 +588,10 @@ async load(input, ctx) { } return changeType === "deleted" ? null : hgCat(newRev, path); }, + resolveFileSourcePath: ({ path, changeType, side }) => { + if (side !== "new" || changeType === "deleted" || input.range) return null; + return join(ctx.cwd, path); + }, }; } ``` @@ -605,6 +611,15 @@ stable identity and Hunk will invalidate conservatively. Leaving `readFileSource` off is fine: Hunk falls back to the content the patch itself carries, which renders the same diff with less context available. +`resolveFileSourcePath` is separate from source reads because a binary or skipped +file can still have a real path. Return an absolute path only when that exact +reviewed side is backed by the filesystem. Return `null` for absent sides and +for index, revision, stash, patch, merged, or other virtual sources, even when a +same-named working-tree file exists. Hunk uses this provenance for +`ctx.workspace.resolveLocation`; it never invents a checkout path for historical +content. Direct file and difftool comparisons retain their concrete input paths +independently of their display names. + #### Files outside the patch `extraFiles` lists files to review that your `patchText` does not contain, in @@ -1716,7 +1731,10 @@ or extension state through arguments, environment, files, or an application-spec protocol. Hunk only owns terminal suspension and restoration. One application may own the terminal at a time; concurrent calls and controls retained past a review reload reject without invoking the callback. The callback's value and -error pass through unchanged. +error pass through unchanged. Host-presented dialogs cancel immediately and +workspace writes return `unavailable` while the callback owns the terminal, so +do not await Hunk UI from inside it. Non-interactive workspace reads and location +resolution remain available. #### Workspace documents @@ -1759,14 +1777,15 @@ fails, or the document exceeds Hunk's size limit. Reads never prompt. An invalid side rejects the promise. `resolveLocation` turns a reviewed file id and optional `hunkIndex` and -`{ side, line }` into the corresponding absolute path and one-based line on -disk. VCS reviews resolve against the repository; direct file comparisons retain -the concrete compared path, including the old path for a deleted-file comparison. -Hunk uses parsed hunk metadata to map old-side deletions onto their on-disk -position, so extensions can pass accurate locations to editors, debuggers, -browsers, or other applications without interpreting opaque diff metadata. Raw -patch reviews have no attested filesystem path and return `null`. Missing hunks -and stale controls also return `null`; malformed source addresses reject. +`{ side, line }` into an attested absolute path and one-based line on disk. Hunk +uses parsed hunk metadata to map old-side deletions onto a filesystem-backed new +side, so extensions can pass accurate locations to editors, debuggers, browsers, +or other applications without interpreting opaque diff metadata. Direct file +comparisons retain their concrete input paths, including the old path for a +deleted-file comparison. Index, revision, stash, patch, merged, absent, and +other virtual sides return `null` instead of borrowing a same-named checkout +file. Missing hunks and stale controls also return `null`; malformed source +addresses reject. Writes require all of the following: diff --git a/skills/hunk-extensions/SKILL.md b/skills/hunk-extensions/SKILL.md index 90eeb29ad..a024daf5e 100644 --- a/skills/hunk-extensions/SKILL.md +++ b/skills/hunk-extensions/SKILL.md @@ -258,7 +258,9 @@ Most extension bugs are one of these: - **Application execution stays extension-owned.** Extensions are ordinary trusted code and may spawn processes; use `ctx.openInApp` when one needs the terminal so Hunk suspends and restores its renderer. `ctx.workspace.resolveLocation` - maps reviewed ids to app-ready paths and lines. Never write to stdout while + maps only filesystem-attested reviewed sides to app-ready paths and lines. + Dialogs cancel and writes refuse while an app owns the terminal, so do not + await host UI inside the callback. Never write to stdout while Hunk owns the terminal; `hunk.log` is collected as diagnostics and `ctx.notify` is how a user hears from you. - **`HunkExtensionUserError`** (detected structurally by `name`) buys the full diff --git a/src/core/changeset/diffFile.test.ts b/src/core/changeset/diffFile.test.ts index d137fd25e..2fe9fb59c 100644 --- a/src/core/changeset/diffFile.test.ts +++ b/src/core/changeset/diffFile.test.ts @@ -105,6 +105,23 @@ describe("buildDiffFile", () => { isBinary: false, }); }); + + test("retains source paths for binary files independently of source fetching", () => { + let fetched = false; + const file = buildDiffFile(metadata, "Binary files a/x and b/x differ\n", 0, "src", null, { + sourceFetcherBuilder: () => { + fetched = true; + return undefined; + }, + sourcePathBuilder: (context) => { + expect(context.isBinary).toBe(true); + return { old: "/repo/old.png", new: "/repo/new.png" }; + }, + }); + + expect(fetched).toBe(true); + expect(file.sourcePaths).toEqual({ old: "/repo/old.png", new: "/repo/new.png" }); + }); }); describe("change-block line pairing", () => { diff --git a/src/core/changeset/diffFile.ts b/src/core/changeset/diffFile.ts index dfb603df1..97049c5ba 100644 --- a/src/core/changeset/diffFile.ts +++ b/src/core/changeset/diffFile.ts @@ -3,7 +3,7 @@ import { findSidecarFileContext } from "./sidecar"; import { patchLooksBinary } from "./binary"; import { fileLanguageForPath } from "./fileLanguageLookup"; import { normalizeDiffMetadataPaths, normalizeDiffPath } from "./diffPaths"; -import type { FileSourceFetcher } from "./fileSource"; +import type { FileSourceFetcher, FileSourcePaths } from "./fileSource"; import type { DiffFile, DiffLineMoveKinds, SidecarContext } from "./model"; /** Count visible additions and deletions from parsed diff metadata. */ @@ -36,6 +36,7 @@ export interface BuildDiffFileOptions { previousPath?: string; isBinary?: boolean; sourceFetcherBuilder?: (file: DiffFileSourceContext) => FileSourceFetcher | undefined; + sourcePathBuilder?: (file: DiffFileSourceContext) => FileSourcePaths | undefined; isTooLarge?: boolean; stats?: DiffFile["stats"]; statsTruncated?: boolean; @@ -55,6 +56,7 @@ export function buildDiffFile( previousPath, isBinary, sourceFetcherBuilder, + sourcePathBuilder, isTooLarge, stats, statsTruncated, @@ -69,13 +71,15 @@ export function buildDiffFile( : (normalizeDiffPath(previousPath) ?? normalizedMetadata.prevName); const resolvedIsBinary = isBinary ?? patchLooksBinary(patch); const language = fileLanguageForPath(path); - const sourceFetcher = sourceFetcherBuilder?.({ + const sourceContext = { path, previousPath: resolvedPreviousPath, type: normalizedMetadata.type, isUntracked: Boolean(isUntracked), isBinary: resolvedIsBinary, - }); + } satisfies DiffFileSourceContext; + const sourceFetcher = sourceFetcherBuilder?.(sourceContext); + const sourcePaths = sourcePathBuilder?.(sourceContext); return { id: `${sourcePrefix}:${index}:${path}`, @@ -93,6 +97,7 @@ export function buildDiffFile( isTooLarge, statsTruncated, sourceFetcher, + sourcePaths, }; } diff --git a/src/core/changeset/fileSource.test.ts b/src/core/changeset/fileSource.test.ts index 00424460d..8b2302aee 100644 --- a/src/core/changeset/fileSource.test.ts +++ b/src/core/changeset/fileSource.test.ts @@ -2,7 +2,11 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { createFileSourceFetcher, SourceTextTooLargeError } from "./fileSource"; +import { + createFileSourceFetcher, + fileSourcePathsForSpecs, + SourceTextTooLargeError, +} from "./fileSource"; const tempDirs: string[] = []; @@ -22,6 +26,15 @@ afterEach(() => { }); describe("createFileSourceFetcher", () => { + test("projects only filesystem-backed specs to source paths", () => { + expect( + fileSourcePathsForSpecs({ + old: { kind: "none" }, + new: { kind: "fs", absolutePath: join("/repo", "after.txt") }, + }), + ).toEqual({ old: null, new: join("/repo", "after.txt") }); + }); + test("reads fs paths for old and new sides", async () => { const dir = createTempDir("hunk-source-fs-"); const left = join(dir, "before.txt"); diff --git a/src/core/changeset/fileSource.ts b/src/core/changeset/fileSource.ts index b654fdfd8..b45131892 100644 --- a/src/core/changeset/fileSource.ts +++ b/src/core/changeset/fileSource.ts @@ -15,6 +15,18 @@ export type FileSourceSpec = { kind: "none" } | { kind: "fs"; absolutePath: stri export type FileSourceSide = "old" | "new"; +/** Exact filesystem paths for the reviewed sides, or null when a side is not filesystem-backed. */ +export interface FileSourcePaths { + readonly old: string | null; + readonly new: string | null; +} + +/** Generic source specs for both sides of one reviewed file. */ +export interface FileSourceSpecs { + old: FileSourceSpec; + new: FileSourceSpec; +} + export interface FileSourceFetcher { /** Stable identity for source state not already represented by the file's patch. */ readonly cacheKey?: string; @@ -39,11 +51,6 @@ export interface FileSourceFetcherOptions { maxSourceBytes?: number; } -interface ResolvedSpecs { - old: FileSourceSpec; - new: FileSourceSpec; -} - async function readFsSpec( spec: Extract, maxSourceBytes: number, @@ -67,9 +74,17 @@ export async function readFileSourceSpec( return readFsSpec(spec, maxSourceBytes); } +/** Project source specs to the exact paths of only their filesystem-backed sides. */ +export function fileSourcePathsForSpecs(specs: FileSourceSpecs): FileSourcePaths { + return { + old: specs.old.kind === "fs" ? specs.old.absolutePath : null, + new: specs.new.kind === "fs" ? specs.new.absolutePath : null, + }; +} + /** Build a per-file source fetcher that caches each side's resolved text. */ export function createFileSourceFetcher( - specs: ResolvedSpecs, + specs: FileSourceSpecs, { maxSourceBytes = DEFAULT_SOURCE_TEXT_MAX_BYTES }: Readonly = {}, ): FileSourceFetcher { const cache = new Map(); diff --git a/src/core/changeset/fromPatch.ts b/src/core/changeset/fromPatch.ts index b07a1c354..a88a5afea 100644 --- a/src/core/changeset/fromPatch.ts +++ b/src/core/changeset/fromPatch.ts @@ -128,7 +128,7 @@ export function changesetFromPatch( title: string, sourceLabel: string, sidecar: SidecarContext | null, - perFileOptions?: Pick, + perFileOptions?: Pick, ): Changeset { const lineMoveKinds = collectLineMoveKinds(patchText); const sanitizedPatch = sanitizePatch(patchText); diff --git a/src/core/changeset/loaders.test.ts b/src/core/changeset/loaders.test.ts index 9950f19da..ad5a861a5 100644 --- a/src/core/changeset/loaders.test.ts +++ b/src/core/changeset/loaders.test.ts @@ -398,6 +398,8 @@ describe("loadAppBootstrap", () => { expect(bootstrap.changeset.files[0]?.previousPath).toBe("before.png"); expect(bootstrap.changeset.files[0]?.isBinary).toBe(true); expect(bootstrap.changeset.files[0]?.metadata.hunks).toHaveLength(0); + expect(bootstrap.changeset.files[0]?.sourceFetcher).toBeUndefined(); + expect(bootstrap.changeset.files[0]?.sourcePaths).toEqual({ old: left, new: right }); }); test("marks git binary diffs as skipped binary content", async () => { @@ -513,6 +515,10 @@ describe("loadAppBootstrap", () => { }); expect(bootstrap.changeset.files[0]?.metadata.hunks).toHaveLength(0); expect(bootstrap.changeset.files[0]?.sourceFetcher).toBeUndefined(); + expect(bootstrap.changeset.files[0]?.sourcePaths).toEqual({ + old: null, + new: join(dir, "large.txt"), + }); }); test("keeps generated large untracked files as skipped placeholders", async () => { @@ -539,6 +545,10 @@ describe("loadAppBootstrap", () => { expect(bootstrap.changeset.files[0]?.statsTruncated).toBe(false); expect(bootstrap.changeset.files[0]?.metadata.hunks).toHaveLength(0); expect(bootstrap.changeset.files[0]?.sourceFetcher).toBeUndefined(); + expect(bootstrap.changeset.files[0]?.sourcePaths).toEqual({ + old: null, + new: join(dir, "large.txt"), + }); }); test("caps skipped untracked-file stats when byte-size detection would require a full huge read", async () => { @@ -1853,8 +1863,59 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(file?.sourceFetcher).toBeDefined(); expect(await file?.sourceFetcher?.getFullText("old")).toBe("old\n"); expect(await file?.sourceFetcher?.getFullText("new")).toBe("new\n"); + expect(file?.sourcePaths).toEqual({ old: left, new: right }); + }); + + test("difftool keeps concrete side paths instead of its display path", async () => { + const dir = createTempDir("hunk-source-difftool-"); + const left = join(dir, "before.ts"); + const right = join(dir, "after.ts"); + writeFileSync(left, "old\n"); + writeFileSync(right, "new\n"); + + const bootstrap = await loadAppBootstrap({ + kind: "difftool", + left, + right, + path: "display/renamed.ts", + options: {}, + }); + + expect(bootstrap.changeset.files[0]?.path).toBe("display/renamed.ts"); + expect(bootstrap.changeset.files[0]?.sourcePaths).toEqual({ old: left, new: right }); }); + test.skipIf(platform() === "win32")( + "marks /dev/null as an absent direct-comparison side", + async () => { + const dir = createTempDir("hunk-source-dev-null-"); + const right = join(dir, "added.ts"); + writeFileSync(right, "new\n"); + + const bootstrap = await loadAppBootstrap({ + kind: "diff", + left: "/dev/null", + right, + options: {}, + }); + const file = bootstrap.changeset.files[0]; + + expect(file?.metadata.type).toBe("new"); + expect(file?.sourcePaths).toEqual({ old: null, new: right }); + expect(await file?.sourceFetcher?.getFullText("old")).toBeNull(); + + const deleted = await loadAppBootstrap({ + kind: "diff", + left: right, + right: "/dev/null", + options: {}, + }); + expect(deleted.changeset.files[0]?.metadata.type).toBe("deleted"); + expect(deleted.changeset.files[0]?.sourcePaths).toEqual({ old: right, new: null }); + expect(await deleted.changeset.files[0]?.sourceFetcher?.getFullText("new")).toBeNull(); + }, + ); + test("git working-tree diffs read the new side from the working tree and the old side from the index", async () => { const dir = createTempRepo("hunk-source-git-wt-"); writeFileSync(join(dir, "value.txt"), "first\n"); @@ -1873,6 +1934,7 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(file?.sourceFetcher).toBeDefined(); expect(await file?.sourceFetcher?.getFullText("new")).toBe("second\n"); expect(await file?.sourceFetcher?.getFullText("old")).toBe("first\n"); + expect(file?.sourcePaths).toEqual({ old: null, new: join(dir, "value.txt") }); }); test("git source fetchers use the custom git executable from bootstrap loading", async () => { @@ -1979,6 +2041,7 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(file?.sourceFetcher).toBeDefined(); expect(await file?.sourceFetcher?.getFullText("new")).toBe("second\n"); expect(await file?.sourceFetcher?.getFullText("old")).toBe("first\n"); + expect(file?.sourcePaths).toBeUndefined(); }); test("`hunk show ` refuses to expand source blobs above the source cap", async () => { @@ -2072,6 +2135,7 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(untracked?.sourceFetcher).toBeDefined(); expect(await untracked?.sourceFetcher?.getFullText("new")).toBe("added contents\n"); expect(await untracked?.sourceFetcher?.getFullText("old")).toBeNull(); + expect(untracked?.sourcePaths).toEqual({ old: null, new: join(dir, "added.txt") }); }); test("deleted Unicode files attach a fetcher with new=null and old source", async () => { diff --git a/src/core/changeset/loaders.ts b/src/core/changeset/loaders.ts index aece09bc4..2e919bd0b 100644 --- a/src/core/changeset/loaders.ts +++ b/src/core/changeset/loaders.ts @@ -12,7 +12,12 @@ import { resolve as resolvePath } from "node:path"; import { findSidecarFileContext, loadSidecarContext } from "./sidecar"; import { createSkippedBinaryMetadata, isProbablyBinaryFile } from "./binary"; import { buildDiffFile, type BuildDiffFileOptions, type DiffFileSourceContext } from "./diffFile"; -import { createFileSourceFetcher, type FileSourceSpec } from "./fileSource"; +import { + createFileSourceFetcher, + fileSourcePathsForSpecs, + type FileSourceSpec, + type FileSourceSpecs, +} from "./fileSource"; import { changesetFromPatch } from "./fromPatch"; import { DEFAULT_FILE_GAP, DEFAULT_HUNK_GAP } from "../run/reviewGap"; @@ -52,14 +57,9 @@ function basename(path: string) { return path.split(/[\\/]/).filter(Boolean).pop() ?? path; } -interface ResolvedFileSourceSpecs { - old: FileSourceSpec; - new: FileSourceSpec; -} - /** Build a binary-aware source-fetcher factory from per-file source specs. */ function createSourceFetcherBuilder( - resolveSpecs: (file: DiffFileSourceContext) => ResolvedFileSourceSpecs | undefined, + resolveSpecs: (file: DiffFileSourceContext) => FileSourceSpecs | undefined, ): NonNullable { return (file) => { if (file.isBinary) { @@ -71,6 +71,21 @@ function createSourceFetcherBuilder( }; } +/** Build exact filesystem provenance from per-file source specs. */ +function createSourcePathBuilder( + resolveSpecs: (file: DiffFileSourceContext) => FileSourceSpecs | undefined, +): NonNullable { + return (file) => { + const specs = resolveSpecs(file); + return specs ? fileSourcePathsForSpecs(specs) : undefined; + }; +} + +/** Represent `/dev/null` as an absent side and every other resolved path as filesystem-backed. */ +function directFileSourceSpec(absolutePath: string): FileSourceSpec { + return absolutePath === "/dev/null" ? { kind: "none" } : { kind: "fs", absolutePath }; +} + /** Reorder files to follow agent-context narrative order when a sidecar provides one. */ export function orderDiffFiles(files: DiffFile[], sidecar: SidecarContext | null) { if (!sidecar || sidecar.files.length === 0) { @@ -132,6 +147,7 @@ function buildBinaryFileDiffChangeset( leftPath: string, rightPath: string, sidecar: SidecarContext | null, + sourceSpecs: FileSourceSpecs, ) { return { id: `pair:${displayPath}`, @@ -148,6 +164,7 @@ function buildBinaryFileDiffChangeset( { previousPath: basename(input.left), isBinary: true, + sourcePathBuilder: createSourcePathBuilder(() => sourceSpecs), }, ), ], @@ -164,6 +181,10 @@ async function loadFileDiffChangeset( const rightPath = resolvePath(cwd, input.right); const displayPath = input.kind === "difftool" ? (input.path ?? basename(input.right)) : basename(input.right); + const sourceSpecs = { + old: directFileSourceSpec(leftPath), + new: directFileSourceSpec(rightPath), + } satisfies FileSourceSpecs; const title = input.kind === "difftool" ? `git difftool: ${displayPath}` @@ -172,7 +193,15 @@ async function loadFileDiffChangeset( : `${basename(input.left)} ↔ ${basename(input.right)}`; if (isProbablyBinaryFile(leftPath) || isProbablyBinaryFile(rightPath)) { - return buildBinaryFileDiffChangeset(input, displayPath, title, leftPath, rightPath, sidecar); + return buildBinaryFileDiffChangeset( + input, + displayPath, + title, + leftPath, + rightPath, + sidecar, + sourceSpecs, + ); } const leftText = await Bun.file(leftPath).text(); @@ -201,10 +230,8 @@ async function loadFileDiffChangeset( files: [ buildDiffFile(metadata, patch, 0, displayPath, sidecar, { previousPath: basename(input.left), - sourceFetcherBuilder: createSourceFetcherBuilder(() => ({ - old: { kind: "fs", absolutePath: leftPath }, - new: { kind: "fs", absolutePath: rightPath }, - })), + sourceFetcherBuilder: createSourceFetcherBuilder(() => sourceSpecs), + sourcePathBuilder: createSourcePathBuilder(() => sourceSpecs), }), ], } satisfies Changeset; @@ -225,7 +252,12 @@ async function loadVcsChangeset( result.title, result.sourceLabel, sidecar, - result.sourceFetcherBuilder ? { sourceFetcherBuilder: result.sourceFetcherBuilder } : undefined, + result.sourceFetcherBuilder || result.sourcePathBuilder + ? { + sourceFetcherBuilder: result.sourceFetcherBuilder, + sourcePathBuilder: result.sourcePathBuilder, + } + : undefined, ); // Two published ways to review a file the patch does not contain, and both // land here: `untrackedPaths`, where an adapter names what its VCS considers diff --git a/src/core/changeset/model.ts b/src/core/changeset/model.ts index 2fa4c8750..4ade602ac 100644 --- a/src/core/changeset/model.ts +++ b/src/core/changeset/model.ts @@ -9,7 +9,7 @@ */ import type { FileDiffMetadata } from "@pierre/diffs"; import type { AgentFileContext } from "../../extension-api/types"; -import type { FileSourceFetcher } from "./fileSource"; +import type { FileSourceFetcher, FileSourcePaths } from "./fileSource"; /** One loaded review sidecar: the changeset summary plus every annotated file it names. */ export interface SidecarContext { @@ -38,6 +38,8 @@ export interface DiffFile { // Optional capability for fetching the file's full text on either side. // Loaders attach this when source content is reachable; absent when not. sourceFetcher?: FileSourceFetcher; + // Exact on-disk provenance for filesystem-backed reviewed sides. + sourcePaths?: FileSourcePaths; } export type DiffLineMoveKind = "moved"; diff --git a/src/core/vcs/types.ts b/src/core/vcs/types.ts index bb056911c..15cbfe108 100644 --- a/src/core/vcs/types.ts +++ b/src/core/vcs/types.ts @@ -56,6 +56,8 @@ export interface VcsPatchResult { untrackedPaths?: string[]; /** Exact old/new content lookups, built from the result's `readFileSource`. */ sourceFetcherBuilder?: BuildDiffFileOptions["sourceFetcherBuilder"]; + /** Exact filesystem paths, built from the result's `resolveFileSourcePath`. */ + sourcePathBuilder?: BuildDiffFileOptions["sourcePathBuilder"]; /** Diff files built from the result's declarative `extraFiles` entries. */ extraFiles?: DiffFile[]; } diff --git a/src/core/vcs/untracked.test.ts b/src/core/vcs/untracked.test.ts index a8032ec42..d7bbbf5ac 100644 --- a/src/core/vcs/untracked.test.ts +++ b/src/core/vcs/untracked.test.ts @@ -28,6 +28,7 @@ describe("buildFilesystemUntrackedDiffFile", () => { expect(file.metadata.type).toBe("new"); expect(file.metadata.hunks).toHaveLength(0); + expect(file.sourcePaths).toEqual({ old: null, new: join(repoRoot, "empty.txt") }); expect(file.patch).toBe( [ "diff --git a/empty.txt b/empty.txt", diff --git a/src/core/vcs/untracked.ts b/src/core/vcs/untracked.ts index ea5187aa7..f50e92b67 100644 --- a/src/core/vcs/untracked.ts +++ b/src/core/vcs/untracked.ts @@ -22,12 +22,14 @@ export function buildSkippedLargeUntrackedDiffFile( index: number, sourcePrefix: string, largeFileCheck: LargeFileCheck, + absolutePath: string, ) { return buildDiffFile(createSkippedLargeMetadata(filePath, "new"), "", index, sourcePrefix, null, { isTooLarge: true, isUntracked: true, stats: largeFileCheck.stats, statsTruncated: largeFileCheck.statsTruncated, + sourcePathBuilder: () => ({ old: null, new: absolutePath }), }); } @@ -86,12 +88,19 @@ export function buildFilesystemUntrackedDiffFile( // patch already carries the only content a symlink has. return buildDiffFile(parseSingleFilePatch(patch, filePath), patch, index, sourcePrefix, null, { isUntracked: true, + sourcePathBuilder: () => ({ old: null, new: absolutePath }), }); } const largeFileCheck = inspectLargeUntrackedFile(repoRoot, filePath); if (largeFileCheck.shouldSkip) { - return buildSkippedLargeUntrackedDiffFile(filePath, index, sourcePrefix, largeFileCheck); + return buildSkippedLargeUntrackedDiffFile( + filePath, + index, + sourcePrefix, + largeFileCheck, + absolutePath, + ); } if (isProbablyBinaryFile(absolutePath)) { @@ -101,7 +110,11 @@ export function buildFilesystemUntrackedDiffFile( index, sourcePrefix, null, - { isBinary: true, isUntracked: true }, + { + isBinary: true, + isUntracked: true, + sourcePathBuilder: () => ({ old: null, new: absolutePath }), + }, ); } @@ -128,5 +141,6 @@ export function buildFilesystemUntrackedDiffFile( old: { kind: "none" }, new: { kind: "fs", absolutePath }, }), + sourcePathBuilder: () => ({ old: null, new: absolutePath }), }); } diff --git a/src/extension-api/index.ts b/src/extension-api/index.ts index 06c7b6aab..f3f6d11a0 100644 --- a/src/extension-api/index.ts +++ b/src/extension-api/index.ts @@ -137,6 +137,7 @@ export type { ExtensionVcsFileChangeType, ExtensionVcsFileSide, ExtensionVcsFileSourceReader, + ExtensionVcsFileSourcePathResolver, ExtensionVcsFileSourceRequest, ExtensionVcsFileSourceResult, ExtensionVcsFileSourceTooLarge, diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index 414d6e510..3268f97d6 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -797,6 +797,11 @@ export type ExtensionVcsFileSourceReader = ( request: ExtensionVcsFileSourceRequest, ) => Promise; +/** Resolve one reviewed side to its exact absolute path when it is filesystem-backed. */ +export type ExtensionVcsFileSourcePathResolver = ( + request: ExtensionVcsFileSourceRequest, +) => string | null; + /* -------------------------------------------------------------------------- */ /* Extra reviewed files */ /* -------------------------------------------------------------------------- */ @@ -875,6 +880,11 @@ export interface ExtensionVcsPatchResult { * carries, which renders the same diff with less context available. */ readFileSource?: ExtensionVcsFileSourceReader; + /** + * Return the exact absolute path for a filesystem-backed side, or `null` for + * absent, index, historical, patch, and other virtual sources. + */ + resolveFileSourcePath?: ExtensionVcsFileSourcePathResolver; /** * Opaque stable identity for source state not already represented by each file's patch. * @@ -1832,6 +1842,8 @@ export interface ExtensionCommandContext extends ExtensionContext { * `finally` after `run` settles. The extension owns execution, metadata, * arguments, environment, and exit handling. Calls reject after this review * generation expires or while another application already owns the terminal. + * Host-presented dialogs cancel and workspace writes are unavailable while + * `run` owns the terminal; document reads and location resolution remain available. */ openInApp(run: () => Result | PromiseLike): Promise; /** Session keyboard modes registered by this command's owning extension. */ diff --git a/src/extensions/default/ui/editor/editorApp.test.ts b/src/extensions/default/ui/editor/editorApp.test.ts index 45fdddf6e..ac5e625e8 100644 --- a/src/extensions/default/ui/editor/editorApp.test.ts +++ b/src/extensions/default/ui/editor/editorApp.test.ts @@ -22,6 +22,10 @@ describe("bundled editor app", () => { command: "code", args: ["--reuse-window", "--wait", "--goto", "/repo/a.ts:9"], }); + expect(buildEditorCommand({ editor: "cursor -w", filePath: "/repo/a.ts", line: 9 })).toEqual({ + command: "cursor", + args: ["-w", "--goto", "/repo/a.ts:9"], + }); }); test("hands terminal editors to Hunk's app lifecycle but leaves GUI editors visible", () => { diff --git a/src/extensions/default/ui/editor/index.test.ts b/src/extensions/default/ui/editor/index.test.ts index 8b2674592..831ff19c3 100644 --- a/src/extensions/default/ui/editor/index.test.ts +++ b/src/extensions/default/ui/editor/index.test.ts @@ -7,16 +7,31 @@ import { getBundledUIRegistry } from ".."; import { BUNDLED_EDITOR_COMMAND_FULL_ID } from "."; const originalEditor = process.env.EDITOR; -const originalSpawnSync = Bun.spawnSync; +const originalSpawn = Bun.spawn; const tempDirs: string[] = []; afterEach(() => { if (originalEditor === undefined) delete process.env.EDITOR; else process.env.EDITOR = originalEditor; - (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = originalSpawnSync; + (Bun as unknown as { spawn: typeof Bun.spawn }).spawn = originalSpawn; for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); +/** Replace Bun's asynchronous process launcher through one narrowly typed test seam. */ +function mockSpawn(implementation: (command: string[]) => { exited: Promise }) { + (Bun as unknown as { spawn: typeof Bun.spawn }).spawn = + implementation as unknown as typeof Bun.spawn; +} + +/** Create a promise whose completion one editor test controls. */ +function createDeferredExit() { + let resolve!: (exitCode: number) => void; + const exited = new Promise((settle) => { + resolve = settle; + }); + return { exited, resolve }; +} + /** Return the editor registration from the process-static bundled UI registry. */ function getBundledEditorCommand() { const registered = getBundledUIRegistry().commands.find( @@ -67,19 +82,25 @@ describe("bundled editor extension", () => { }); }); - test("runs the configured editor inside a generic app handoff and refreshes", async () => { + test("awaits a terminal editor asynchronously inside a generic app handoff", async () => { const { context, cwd, execute, notify, openInApp } = createEditorContext(); process.env.EDITOR = "vim --clean"; const spawnCalls: string[][] = []; - (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = ((command: string[]) => { + const exit = createDeferredExit(); + mockSpawn((command) => { spawnCalls.push(command); - return { exitCode: 0 }; - }) as unknown as typeof Bun.spawnSync; + return { exited: exit.exited }; + }); - await getBundledEditorCommand().handler(context); + const pending = getBundledEditorCommand().handler(context); expect(openInApp).toHaveBeenCalledTimes(1); expect(spawnCalls).toEqual([["vim", "--clean", "+2", join(cwd, "alpha.ts")]]); + expect(execute).not.toHaveBeenCalled(); + + exit.resolve(0); + await pending; + expect(execute).toHaveBeenCalledWith("hunk.app.refresh"); expect(notify).not.toHaveBeenCalled(); }); @@ -87,30 +108,55 @@ describe("bundled editor extension", () => { test("reports editor failures after Hunk restores its view", async () => { const { context, notify } = createEditorContext(); process.env.EDITOR = "vim"; - (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = (() => ({ - exitCode: 2, - })) as unknown as typeof Bun.spawnSync; + mockSpawn(() => ({ exited: Promise.resolve(2) })); await getBundledEditorCommand().handler(context); expect(notify).toHaveBeenCalledWith("Editor exited with status 2.", "error"); }); - test("keeps GUI editors visible and waits for them before refreshing", async () => { - const { context, cwd, execute, openInApp } = createEditorContext(); + test("keeps GUI editors responsive, waits before refreshing, and refuses overlap", async () => { + const { context, cwd, execute, notify, openInApp } = createEditorContext(); process.env.EDITOR = "code --reuse-window"; const spawnCalls: string[][] = []; - (Bun as unknown as { spawnSync: typeof Bun.spawnSync }).spawnSync = ((command: string[]) => { + const exit = createDeferredExit(); + mockSpawn((command) => { spawnCalls.push(command); - return { exitCode: 0 }; - }) as unknown as typeof Bun.spawnSync; + return { exited: exit.exited }; + }); - await getBundledEditorCommand().handler(context); + const pending = getBundledEditorCommand().handler(context); expect(openInApp).not.toHaveBeenCalled(); expect(spawnCalls).toEqual([ ["code", "--reuse-window", "--wait", "--goto", `${join(cwd, "alpha.ts")}:2`], ]); + expect(execute).not.toHaveBeenCalled(); + + // The first asynchronous child remains pending without blocking a second command dispatch. + await getBundledEditorCommand().handler(context); + expect(spawnCalls).toHaveLength(1); + expect(notify).toHaveBeenCalledWith("An editor is already open.", "warning"); + + exit.resolve(0); + await pending; + expect(execute).toHaveBeenCalledWith("hunk.app.refresh"); }); + + test("releases bundled editor ownership when asynchronous process launch fails", async () => { + const { context, notify } = createEditorContext(); + process.env.EDITOR = "code"; + let launches = 0; + mockSpawn(() => { + launches += 1; + throw new Error("missing executable"); + }); + + await getBundledEditorCommand().handler(context); + await getBundledEditorCommand().handler(context); + + expect(launches).toBe(2); + expect(notify).toHaveBeenCalledWith("Failed to launch editor: missing executable", "error"); + }); }); diff --git a/src/extensions/default/ui/editor/index.ts b/src/extensions/default/ui/editor/index.ts index 506f24175..a2a572b25 100644 --- a/src/extensions/default/ui/editor/index.ts +++ b/src/extensions/default/ui/editor/index.ts @@ -6,6 +6,8 @@ export const BUNDLED_EDITOR_COMMAND_FULL_ID = `hunk.${BUNDLED_EDITOR_COMMAND_ID} /** Register Hunk's editor workflow through the public app-handoff contract. */ const registerBundledEditor: ExtensionFactory = (hunk) => { + let editorOpen = false; + hunk.registerCommand( { id: BUNDLED_EDITOR_COMMAND_ID, @@ -42,29 +44,38 @@ const registerBundledEditor: ExtensionFactory = (hunk) => { return; } - let exitCode: number; + if (editorOpen) { + ctx.notify("An editor is already open.", "warning"); + return; + } + + editorOpen = true; try { - const runEditor = () => - Bun.spawnSync([selected.command.command, ...selected.command.args], { + const runEditor = async () => { + const child = Bun.spawn([selected.command.command, ...selected.command.args], { stdin: "inherit", stdout: "inherit", stderr: "inherit", }); - const result = editorUsesTerminal(editor) ? await ctx.openInApp(runEditor) : runEditor(); - exitCode = result.exitCode; + return await child.exited; + }; + const exitCode = editorUsesTerminal(editor) + ? await ctx.openInApp(runEditor) + : await runEditor(); + + if (exitCode !== 0) { + ctx.notify(`Editor exited with status ${exitCode}.`, "error"); + return; + } + ctx.commands.execute("hunk.app.refresh"); } catch (error) { ctx.notify( `Failed to launch editor: ${error instanceof Error ? error.message : String(error)}`, "error", ); - return; - } - - if (exitCode !== 0) { - ctx.notify(`Editor exited with status ${exitCode}.`, "error"); - return; + } finally { + editorOpen = false; } - ctx.commands.execute("hunk.app.refresh"); }, ); }; diff --git a/src/extensions/default/vcs/git/index.test.ts b/src/extensions/default/vcs/git/index.test.ts index fc43fe21c..b56198f60 100644 --- a/src/extensions/default/vcs/git/index.test.ts +++ b/src/extensions/default/vcs/git/index.test.ts @@ -132,6 +132,18 @@ describe("GitVcsAdapter", () => { const trackedFile = { path: "tracked.txt", changeType: "change", isUntracked: false } as const; expect(await readSource?.({ ...trackedFile, side: "old" })).toBe("old\n"); expect(await readSource?.({ ...trackedFile, side: "new" })).toBe("new\n"); + expect(result.resolveFileSourcePath?.({ ...trackedFile, side: "old" })).toBeNull(); + expect(result.resolveFileSourcePath?.({ ...trackedFile, side: "new" })).toBe( + join(repo, "tracked.txt"), + ); + expect( + result.resolveFileSourcePath?.({ ...trackedFile, changeType: "new", side: "old" }), + ).toBeNull(); + expect( + result.resolveFileSourcePath?.({ ...trackedFile, changeType: "deleted", side: "new" }), + ).toBeNull(); + rmSync(join(repo, "tracked.txt")); + expect(result.resolveFileSourcePath?.({ ...trackedFile, side: "new" })).toBeNull(); git(repo, "add", "tracked.txt"); const changedIndexResult = await GitVcsAdapter.operations["working-tree-diff"]!.load(input, { @@ -169,6 +181,8 @@ describe("GitVcsAdapter", () => { expect(result.untrackedPaths).toEqual([]); expect(await result.readFileSource?.({ ...file, side: "old" })).toBe("old\ncontext\n"); expect(await result.readFileSource?.({ ...file, side: "new" })).toBe("new\ncontext\n"); + expect(result.resolveFileSourcePath?.({ ...file, side: "old" })).toBeNull(); + expect(result.resolveFileSourcePath?.({ ...file, side: "new" })).toBeNull(); }); test("loads revision and stash patches through adapter operations", async () => { @@ -196,6 +210,8 @@ describe("GitVcsAdapter", () => { const showFile = { path: "file.txt", changeType: "change", isUntracked: false } as const; expect(await showResult.readFileSource?.({ ...showFile, side: "old" })).toBe("one\n"); expect(await showResult.readFileSource?.({ ...showFile, side: "new" })).toBe("two\n"); + expect(showResult.resolveFileSourcePath?.({ ...showFile, side: "old" })).toBeNull(); + expect(showResult.resolveFileSourcePath?.({ ...showFile, side: "new" })).toBeNull(); writeFileSync(join(repo, "file.txt"), "three\n"); git(repo, "stash", "push", "-m", "adapter stash"); diff --git a/src/extensions/default/vcs/git/index.ts b/src/extensions/default/vcs/git/index.ts index d3a1b27ff..d2ee70395 100644 --- a/src/extensions/default/vcs/git/index.ts +++ b/src/extensions/default/vcs/git/index.ts @@ -19,7 +19,7 @@ import { type GitBackedInput, type GitDiffEndpoints, } from "./commands"; -import { gitEndpointSourceSpec, readGitFileSource } from "./source"; +import { gitEndpointSourceSpec, gitFileSourcePath, readGitFileSource } from "./source"; import { describeDiffRange } from "../diffRange"; import { HUNK_VCS_DETECTION_BASELINE_PRIORITY, @@ -28,6 +28,8 @@ import { type ExtensionVcsDirectoryTreeWatchTarget, type ExtensionVcsExtraFile, type ExtensionVcsFileSourceReader, + type ExtensionVcsFileSourcePathResolver, + type ExtensionVcsFileSourceRequest, type ExtensionVcsWatchPlan, type HunkExtensionAPI, } from "hunkdiff/extension"; @@ -83,9 +85,34 @@ export function statSignature(path: string) { /** Exact source reader plus a stable identity for its complete old/new snapshot. */ interface GitSourceCapability { readFileSource: ExtensionVcsFileSourceReader; + resolveFileSourcePath: ExtensionVcsFileSourcePathResolver; sourceCacheKey: string; } +/** Resolve one file side to the exact Git source spec used for both reads and paths. */ +function gitFileSourceSpecForRequest( + request: ExtensionVcsFileSourceRequest, + repoRoot: string, + endpoints: GitDiffEndpoints, +) { + const spec = + request.side === "old" + ? request.changeType === "new" + ? ({ kind: "none" } as const) + : gitEndpointSourceSpec(endpoints.old, repoRoot, request.previousPath ?? request.path) + : request.changeType === "deleted" + ? ({ kind: "none" } as const) + : gitEndpointSourceSpec(endpoints.new, repoRoot, request.path); + + if (spec.kind !== "fs") return spec; + try { + fs.lstatSync(spec.absolutePath); + return spec; + } catch { + return { kind: "none" } as const; + } +} + /** Hash semantic index entries so filesystem-stat refreshes do not defeat cache reuse. */ function gitIndexCacheKey(input: GitBackedInput, repoRoot: string, gitExecutable: string) { const entries = runGitText({ @@ -130,26 +157,12 @@ function createGitSourceCapability( gitEndpointCacheKey(endpoints.old, indexCacheKey), gitEndpointCacheKey(endpoints.new, indexCacheKey), ].join(":"), - readFileSource: ({ path, previousPath, changeType, side }) => { - // An added file has no old side and a deleted one has no new side; asking - // Git for either would just be a failed lookup. - if (side === "old") { - return changeType === "new" - ? Promise.resolve(null) - : readGitFileSource( - gitEndpointSourceSpec(endpoints.old, repoRoot, previousPath ?? path), - { - gitExecutable, - }, - ); - } - - return changeType === "deleted" - ? Promise.resolve(null) - : readGitFileSource(gitEndpointSourceSpec(endpoints.new, repoRoot, path), { - gitExecutable, - }); - }, + readFileSource: (request) => + readGitFileSource(gitFileSourceSpecForRequest(request, repoRoot, endpoints), { + gitExecutable, + }), + resolveFileSourcePath: (request) => + gitFileSourcePath(gitFileSourceSpecForRequest(request, repoRoot, endpoints)), }; } diff --git a/src/extensions/default/vcs/git/source.test.ts b/src/extensions/default/vcs/git/source.test.ts index f97eb2029..f07166fa3 100644 --- a/src/extensions/default/vcs/git/source.test.ts +++ b/src/extensions/default/vcs/git/source.test.ts @@ -2,7 +2,7 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { gitEndpointSourceSpec, readGitFileSource } from "./source"; +import { gitEndpointSourceSpec, gitFileSourcePath, readGitFileSource } from "./source"; const tempDirs: string[] = []; @@ -73,6 +73,17 @@ describe("gitEndpointSourceSpec", () => { absolutePath: join("/repo", "a.ts"), }); }); + + test("exposes paths only for filesystem specs", () => { + expect(gitFileSourcePath({ kind: "fs", absolutePath: join("/repo", "a.ts") })).toBe( + join("/repo", "a.ts"), + ); + expect(gitFileSourcePath({ kind: "git-index", repoRoot: "/repo", path: "a.ts" })).toBeNull(); + expect( + gitFileSourcePath({ kind: "git-blob", repoRoot: "/repo", ref: "HEAD", path: "a.ts" }), + ).toBeNull(); + expect(gitFileSourcePath({ kind: "none" })).toBeNull(); + }); }); describe("Git source reading", () => { diff --git a/src/extensions/default/vcs/git/source.ts b/src/extensions/default/vcs/git/source.ts index b5a767291..23e5632a2 100644 --- a/src/extensions/default/vcs/git/source.ts +++ b/src/extensions/default/vcs/git/source.ts @@ -32,6 +32,11 @@ export interface GitFileSourceOptions { maxSourceBytes?: number; } +/** Return the exact path only when one Git source spec names the live filesystem. */ +export function gitFileSourcePath(spec: GitFileSourceSpec) { + return spec.kind === "fs" ? spec.absolutePath : null; +} + /** Convert one Git diff endpoint into the corresponding source lookup. */ export function gitEndpointSourceSpec( endpoint: GitDiffEndpoint, diff --git a/src/extensions/default/vcs/jujutsu/index.test.ts b/src/extensions/default/vcs/jujutsu/index.test.ts index 0cf270698..76c825113 100644 --- a/src/extensions/default/vcs/jujutsu/index.test.ts +++ b/src/extensions/default/vcs/jujutsu/index.test.ts @@ -130,6 +130,10 @@ describe("JjVcsAdapter", () => { } as const; expect(await diffResult.readFileSource?.({ ...reviewedFile, side: "old" })).toBe("one\n"); expect(await diffResult.readFileSource?.({ ...reviewedFile, side: "new" })).toBe("two\n"); + expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "old" })).toBeNull(); + expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "new" })).toBe( + join(repo, "file.txt"), + ); const equivalentDiffResult = await JjVcsAdapter.operations["working-tree-diff"]!.load( diffInput, { cwd: repo }, @@ -150,6 +154,7 @@ describe("JjVcsAdapter", () => { expect(showResult.sourceCacheKey).toContain("jj-source-v1"); expect(await showResult.readFileSource?.({ ...reviewedFile, side: "old" })).toBe("one\n"); expect(await showResult.readFileSource?.({ ...reviewedFile, side: "new" })).toBe("two\n"); + expect("resolveFileSourcePath" in showResult).toBe(false); // Lazy source reads stay attached to the revision that produced the patch, // even after `@` is resnapshotted with different working-copy contents. @@ -195,6 +200,7 @@ describe("JjVcsAdapter", () => { expect(result.patchText).toContain("+two"); expect(await result.readFileSource?.({ ...file, side: "old" })).toBe("one\ncontext\n"); expect(await result.readFileSource?.({ ...file, side: "new" })).toBe("two\ncontext\n"); + expect(result.resolveFileSourcePath).toBeUndefined(); writeFileSync(join(repo, "file.txt"), "three\ncontext\n"); expect( diff --git a/src/extensions/default/vcs/jujutsu/index.ts b/src/extensions/default/vcs/jujutsu/index.ts index 48018f053..10036b4fe 100644 --- a/src/extensions/default/vcs/jujutsu/index.ts +++ b/src/extensions/default/vcs/jujutsu/index.ts @@ -12,6 +12,7 @@ import { } from "./commands"; import { readJjFileSource } from "./source"; import { describeDiffRange } from "../diffRange"; +import { createWorkingTreeSourcePathResolver } from "../workingTreeSource"; import { HUNK_VCS_DETECTION_BASELINE_PRIORITY, type ExtensionVcsAdapter, @@ -165,6 +166,9 @@ export function createJjVcsAdapter({ jjExecutable = "jj" }: Readonly { expect(diffResult.title).toContain("working copy"); expect(diffResult.patchText).toContain("diff --git a/file.txt b/file.txt"); expect(diffResult.patchText).toContain("+two"); + const reviewedFile = { + path: "file.txt", + changeType: "change", + isUntracked: false, + } as const; + expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "old" })).toBeNull(); + expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "new" })).toBe( + join(repo, "file.txt"), + ); const showInput = { kind: "show", @@ -130,6 +139,7 @@ describe("SaplingVcsAdapter", () => { expect(showResult.title).toContain("show ."); expect(showResult.patchText).toContain("diff --git a/file.txt b/file.txt"); + expect("resolveFileSourcePath" in showResult).toBe(false); expect( SaplingVcsAdapter.operations["working-tree-diff"]!.watchSignature!(diffInput, { cwd: repo, @@ -186,6 +196,7 @@ describe("SaplingVcsAdapter without the sl binary", () => { }); expect(result.untrackedPaths).toEqual([]); + expect(result.resolveFileSourcePath).toBeUndefined(); expect(commands.some((command) => command.includes("status"))).toBe(false); expect( commands.some((command) => command.includes("main") && command.includes("feature")), diff --git a/src/extensions/default/vcs/sapling/index.ts b/src/extensions/default/vcs/sapling/index.ts index 37eacb1df..28f31defe 100644 --- a/src/extensions/default/vcs/sapling/index.ts +++ b/src/extensions/default/vcs/sapling/index.ts @@ -9,6 +9,7 @@ import { runSlText, } from "./commands"; import { describeDiffRange } from "../diffRange"; +import { createWorkingTreeSourcePathResolver } from "../workingTreeSource"; import { HUNK_VCS_DETECTION_BASELINE_PRIORITY, type ExtensionVcsAdapter, @@ -90,6 +91,9 @@ export const SaplingVcsAdapter = { title: range ? `${repoName} ${range}` : `${repoName} working copy`, patchText: runSlText({ input, args: diffArgs, cwd }), untrackedPaths: listSlUntrackedFiles(input, { cwd, repoRoot }), + ...(!input.range && !input.rangeEndpoints + ? { resolveFileSourcePath: createWorkingTreeSourcePathResolver(repoRoot) } + : {}), }; }, watchSignature(input, { cwd }) { diff --git a/src/extensions/default/vcs/workingTreeSource.test.ts b/src/extensions/default/vcs/workingTreeSource.test.ts new file mode 100644 index 000000000..d1a3bf3d2 --- /dev/null +++ b/src/extensions/default/vcs/workingTreeSource.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, test } from "bun:test"; +import { join } from "node:path"; +import { createWorkingTreeSourcePathResolver } from "./workingTreeSource"; + +describe("createWorkingTreeSourcePathResolver", () => { + const resolvePath = createWorkingTreeSourcePathResolver("/repo"); + const file = { path: "src/a.ts", changeType: "change", isUntracked: false } as const; + + test("resolves only present new sides", () => { + expect(resolvePath({ ...file, side: "new" })).toBe(join("/repo", "src/a.ts")); + expect(resolvePath({ ...file, side: "old" })).toBeNull(); + expect(resolvePath({ ...file, changeType: "deleted", side: "new" })).toBeNull(); + }); +}); diff --git a/src/extensions/default/vcs/workingTreeSource.ts b/src/extensions/default/vcs/workingTreeSource.ts new file mode 100644 index 000000000..223713731 --- /dev/null +++ b/src/extensions/default/vcs/workingTreeSource.ts @@ -0,0 +1,15 @@ +import { join } from "node:path"; +import type { + ExtensionVcsFileSourcePathResolver, + ExtensionVcsFileSourceRequest, +} from "hunkdiff/extension"; + +/** Resolve only a present new side to its path in a provider's live working tree. */ +export function createWorkingTreeSourcePathResolver( + repoRoot: string, +): ExtensionVcsFileSourcePathResolver { + return (request: ExtensionVcsFileSourceRequest) => + request.side === "new" && request.changeType !== "deleted" + ? join(repoRoot, request.path) + : null; +} diff --git a/src/extensions/vcsPatchResult.test.ts b/src/extensions/vcsPatchResult.test.ts index fa22cc56a..b901a39da 100644 --- a/src/extensions/vcsPatchResult.test.ts +++ b/src/extensions/vcsPatchResult.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { join, resolve } from "node:path"; import { toInternalVcsPatchResult } from "./vcsPatchResult"; import { HunkExtensionUserError } from "../extension-api/types"; import { HunkUserError, toUserFacingError } from "../core/run/errors"; @@ -148,6 +149,63 @@ describe("published source readers", () => { }); }); +describe("published source path resolvers", () => { + test("become per-file absolute source paths without requiring a text reader", () => { + const requests: ExtensionVcsFileSourceRequest[] = []; + const root = resolve("repo"); + const result = toInternalVcsPatchResult( + baseResult({ + resolveFileSourcePath: (request) => { + requests.push(request); + return request.side === "new" ? join(root, request.path) : null; + }, + }), + ); + const sourcePaths = result.sourcePathBuilder?.({ + path: "src/a.ts", + previousPath: "src/old.ts", + type: "rename-changed", + isUntracked: false, + isBinary: true, + }); + + expect(result.sourceFetcherBuilder).toBeUndefined(); + expect(sourcePaths).toEqual({ old: null, new: join(root, "src/a.ts") }); + expect(requests).toEqual([ + { + path: "src/a.ts", + previousPath: "src/old.ts", + changeType: "rename-changed", + isUntracked: false, + side: "old", + }, + { + path: "src/a.ts", + previousPath: "src/old.ts", + changeType: "rename-changed", + isUntracked: false, + side: "new", + }, + ]); + }); + + test("drops relative and wholly unavailable resolver answers", () => { + const relative = toInternalVcsPatchResult( + baseResult({ resolveFileSourcePath: () => "relative/a.ts" }), + ); + const unavailable = toInternalVcsPatchResult(baseResult({ resolveFileSourcePath: () => null })); + const file = { + path: "a.ts", + type: "change", + isUntracked: false, + isBinary: false, + } as const; + + expect(relative.sourcePathBuilder?.(file)).toBeUndefined(); + expect(unavailable.sourcePathBuilder?.(file)).toBeUndefined(); + }); +}); + describe("published extra files", () => { test("build a diff file from a one-file patch, labeled with the declared path", () => { const result = toInternalVcsPatchResult( @@ -170,6 +228,7 @@ describe("published extra files", () => { }); test("build a placeholder for a skipped file with no content to read", () => { + const sourcePath = resolve("repo", "generated.txt"); const result = toInternalVcsPatchResult( baseResult({ extraFiles: [ @@ -184,6 +243,7 @@ describe("published extra files", () => { }, ], readFileSource: async () => "unreachable", + resolveFileSourcePath: ({ side }) => (side === "new" ? sourcePath : null), }), ); @@ -195,6 +255,7 @@ describe("published extra files", () => { expect(file?.statsTruncated).toBe(true); expect(file?.metadata.hunks).toHaveLength(0); expect(file?.sourceFetcher).toBeUndefined(); + expect(file?.sourcePaths).toEqual({ old: null, new: sourcePath }); }); test("defaults a skipped file to a modification with no counted lines", () => { diff --git a/src/extensions/vcsPatchResult.ts b/src/extensions/vcsPatchResult.ts index 540697358..03a0cbe35 100644 --- a/src/extensions/vcsPatchResult.ts +++ b/src/extensions/vcsPatchResult.ts @@ -3,6 +3,7 @@ import { createSkippedLargeMetadata, type BuildDiffFileOptions, } from "../core/changeset/diffFile"; +import { isAbsolute } from "node:path"; import { parseSingleFilePatch } from "../core/patch/singleFile"; import { DEFAULT_SOURCE_TEXT_MAX_BYTES, @@ -14,6 +15,8 @@ import type { VcsPatchResult } from "../core/vcs/types"; import type { ExtensionVcsExtraFile, ExtensionVcsFileSourceReader, + ExtensionVcsFileSourcePathResolver, + ExtensionVcsFileSourceRequest, ExtensionVcsPatchResult, } from "../extension-api/types"; @@ -27,6 +30,21 @@ import type { */ type SourceFetcherBuilder = NonNullable; +type SourcePathBuilder = NonNullable; + +/** Build one public source request from normalized per-file context. */ +function sourceRequest( + file: Parameters[0], + side: FileSourceSide, +): ExtensionVcsFileSourceRequest { + return { + path: file.path, + previousPath: file.previousPath, + changeType: file.type, + isUntracked: file.isUntracked, + side, + }; +} /** * Adapt a published per-file source reader to the internal per-file fetcher. @@ -61,13 +79,7 @@ function toSourceFetcherBuilder( throw new SourceTextTooLargeError(cachedLimit); } - const result = await read({ - path: file.path, - previousPath: file.previousPath, - changeType: file.type, - isUntracked: file.isUntracked, - side, - }); + const result = await read(sourceRequest(file, side)); if (typeof result === "object" && result !== null) { if (result.kind === "too-large") { const maxBytes = @@ -89,6 +101,19 @@ function toSourceFetcherBuilder( }; } +/** Adapt a published path resolver to normalized per-file filesystem provenance. */ +function toSourcePathBuilder(resolvePath: ExtensionVcsFileSourcePathResolver): SourcePathBuilder { + return (file) => { + const resolveSide = (side: FileSourceSide) => { + const path = resolvePath(sourceRequest(file, side)); + return path !== null && isAbsolute(path) ? path : null; + }; + const old = resolveSide("old"); + const next = resolveSide("new"); + return old === null && next === null ? undefined : { old, new: next }; + }; +} + /** * Build the diff model for one file an adapter reported outside its patch text. * @@ -101,6 +126,7 @@ function toInternalExtraFile( index: number, sourcePrefix: string, sourceFetcherBuilder: SourceFetcherBuilder | undefined, + sourcePathBuilder: SourcePathBuilder | undefined, ): DiffFile { if (entry.kind === "skipped") { return buildDiffFile( @@ -115,6 +141,7 @@ function toInternalExtraFile( isTooLarge: true, stats: entry.stats, statsTruncated: entry.statsTruncated, + sourcePathBuilder, }, ); } @@ -129,6 +156,7 @@ function toInternalExtraFile( previousPath: entry.previousPath, isUntracked: entry.isUntracked, sourceFetcherBuilder, + sourcePathBuilder, }, ); } @@ -138,6 +166,9 @@ export function toInternalVcsPatchResult(result: ExtensionVcsPatchResult): VcsPa const sourceFetcherBuilder = result.readFileSource ? toSourceFetcherBuilder(result.readFileSource, result.sourceCacheKey) : undefined; + const sourcePathBuilder = result.resolveFileSourcePath + ? toSourcePathBuilder(result.resolveFileSourcePath) + : undefined; return { repoRoot: result.repoRoot, @@ -146,8 +177,9 @@ export function toInternalVcsPatchResult(result: ExtensionVcsPatchResult): VcsPa patchText: result.patchText, untrackedPaths: result.untrackedPaths, sourceFetcherBuilder, + sourcePathBuilder, extraFiles: result.extraFiles?.map((entry, index) => - toInternalExtraFile(entry, index, result.repoRoot, sourceFetcherBuilder), + toInternalExtraFile(entry, index, result.repoRoot, sourceFetcherBuilder, sourcePathBuilder), ), }; } diff --git a/src/ui/App.tsx b/src/ui/App.tsx index f6ee0751c..b1076cfdb 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -483,6 +483,7 @@ export function App({ const { accept: acceptExtensionDialog, cancel: cancelExtensionDialog, + cancelAll: cancelAllExtensionDialogs, createDialogs: createQueuedExtensionDialogs, inputValue: extensionDialogInputValue, moveSelection: moveExtensionDialogSelection, @@ -492,6 +493,12 @@ export function App({ updateInput: setExtensionDialogInputValue, } = useExtensionDialogController({ reviewGeneration: bootstrap }); + const extensionAppController = useExtensionAppController({ + createReviewCapabilityLease, + onOwnershipStarted: cancelAllExtensionDialogs, + renderer, + }); + /** Keep third-party dialog attribution while presenting bundled extensions as native Hunk UI. */ const createExtensionDialogs = useCallback( (extensionId: string) => { @@ -500,11 +507,11 @@ export function App({ (metadata) => metadata.id === extensionId && metadata.origin === "bundled", ); return createQueuedExtensionDialogs(extensionId, { - isLive: lease.isLive, + isLive: () => lease.isLive() && !extensionAppController.isAppActive(), showAttribution: !bundled, }); }, - [createQueuedExtensionDialogs, createReviewCapabilityLease, extensions], + [createQueuedExtensionDialogs, createReviewCapabilityLease, extensionAppController, extensions], ); const extensionWorkspaceController = useExtensionWorkspaceControls({ @@ -512,16 +519,12 @@ export function App({ createReviewCapabilityLease, files: reviewFiles, input: bootstrap.input, + isAppActive: extensionAppController.isAppActive, onWorkspaceWriteCompleted, root: bootstrap.reloadContext.repoRoot ?? bootstrap.reloadContext.cwd, runWorkspaceWrite, workspaceFileWriter, }); - const extensionAppController = useExtensionAppController({ - createReviewCapabilityLease, - renderer, - }); - useExtensionEventContextProvider({ createDialogs: createExtensionDialogs, createNavigation: createExtensionNavigation, diff --git a/src/ui/AppHost.edit-in-editor.test.tsx b/src/ui/AppHost.edit-in-editor.test.tsx index f950323ec..d983c4740 100644 --- a/src/ui/AppHost.edit-in-editor.test.tsx +++ b/src/ui/AppHost.edit-in-editor.test.tsx @@ -29,7 +29,7 @@ const AFTER = lines( ); const originalEditor = process.env.EDITOR; -const originalSpawnSync = Bun.spawnSync; +const originalSpawn = Bun.spawn; const tempDirs: string[] = []; let setup: Awaited> | undefined; @@ -41,9 +41,9 @@ function createTempWorkspace() { return dir; } -function mockSpawnSync(implementation: typeof Bun.spawnSync) { - const mutableBun = Bun as unknown as { spawnSync: typeof Bun.spawnSync }; - mutableBun.spawnSync = implementation; +function mockSpawn(implementation: (commands: string[]) => { exited: Promise }) { + const mutableBun = Bun as unknown as { spawn: typeof Bun.spawn }; + mutableBun.spawn = implementation as unknown as typeof Bun.spawn; } /** Bootstrap one working-tree review whose file really exists under `sourceLabel`. */ @@ -54,14 +54,17 @@ function createEditorBootstrap(sourceLabel: string, repoRoot = sourceLabel): App initialMode: "stack", sourceLabel, files: [ - createTestDiffFile({ - after: AFTER, - agent: false, - before: BEFORE, - context: 3, - id: "sample", - path: "sample.ts", - }), + { + ...createTestDiffFile({ + after: AFTER, + agent: false, + before: BEFORE, + context: 3, + id: "sample", + path: "sample.ts", + }), + sourcePaths: { old: null, new: join(repoRoot, "sample.ts") }, + }, ], }), extensions: createEmptyExtensionLoadResult(repoRoot), @@ -104,7 +107,7 @@ afterEach(async () => { } else { process.env.EDITOR = originalEditor; } - mockSpawnSync(originalSpawnSync); + (Bun as unknown as { spawn: typeof Bun.spawn }).spawn = originalSpawn; while (tempDirs.length > 0) { const dir = tempDirs.pop(); @@ -133,10 +136,10 @@ describe("AppHost edit-selected-file shortcut", () => { process.env.EDITOR = "vim"; const spawnCalls: string[][] = []; - mockSpawnSync(((cmds: string[]) => { + mockSpawn((cmds) => { spawnCalls.push(cmds); - return { exitCode: 1 }; - }) as unknown as typeof Bun.spawnSync); + return { exited: Promise.resolve(1) }; + }); setup = await testRender( , diff --git a/src/ui/AppHost.extension-dialogs.test.tsx b/src/ui/AppHost.extension-dialogs.test.tsx index 7df60410d..84da7f025 100644 --- a/src/ui/AppHost.extension-dialogs.test.tsx +++ b/src/ui/AppHost.extension-dialogs.test.tsx @@ -214,6 +214,48 @@ function writeDialogFixture(extPath: string, logPath: string, askSource: string) } describe("extension dialogs", () => { + test("dialogs cancel immediately instead of queueing while an application owns the terminal", async () => { + const repo = createTestRepo("hunk-ext-dialog-app-owner-"); + const extDir = createTempDir("hunk-ext-dialog-app-owner-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeFileSync( + extPath, + `import { appendFileSync } from "node:fs";\n` + + `export default function (hunk) {\n` + + ` hunk.registerCommand({ id: "ask-in-app", title: "Ask in app", key: "y" }, async (ctx) => {\n` + + ` const pending = ctx.dialogs.confirm({ title: "Already queued confirm" });\n` + + ` const answers = await ctx.openInApp(async () => await Promise.all([\n` + + ` pending,\n` + + ` ctx.dialogs.confirm({ title: "Invisible confirm" }),\n` + + ` ctx.dialogs.select({ title: "Invisible select", options: ["one"] }),\n` + + ` ctx.dialogs.input({ title: "Invisible input" }),\n` + + ` ]));\n` + + ` appendFileSync(${JSON.stringify(logPath)}, JSON.stringify(answers) + "\\n");\n` + + ` });\n` + + `}\n`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("[false,false,null,null]"), + "the app-owned dialogs to resolve their cancel values", + ); + + const frame = setup.captureCharFrame(); + expect(frame).toContain("alpha.txt"); + expect(frame).not.toContain("Already queued confirm"); + expect(frame).not.toContain("Invisible confirm"); + expect(frame).not.toContain("Invisible select"); + expect(frame).not.toContain("Invisible input"); + }); + }); + test("a confirm dialog renders with attribution and resolves true on enter", async () => { const repo = createTestRepo("hunk-ext-dialog-confirm-"); // Outside the repo, so the fixture and its log never join the review. diff --git a/src/ui/hooks/useExtensionAppController.test.tsx b/src/ui/hooks/useExtensionAppController.test.tsx index ac112752f..6fccb4e63 100644 --- a/src/ui/hooks/useExtensionAppController.test.tsx +++ b/src/ui/hooks/useExtensionAppController.test.tsx @@ -4,11 +4,11 @@ import { act } from "react"; import { useExtensionAppController } from "./useExtensionAppController"; /** Build one renderer identity whose terminal ownership can outlive hook mounts. */ -function createTestAppRenderer() { +function createTestAppRenderer(suspend: () => void = () => {}) { return { destroyed: false, resume: mock(() => {}), - suspend: mock(() => {}), + suspend: mock(suspend), renderer: null as unknown as { readonly isDestroyed: boolean; resume: () => void; @@ -18,7 +18,10 @@ function createTestAppRenderer() { } /** Mount app controls with mutable review authority and a traced renderer. */ -async function renderController(appRenderer = createTestAppRenderer()) { +async function renderController( + appRenderer = createTestAppRenderer(), + onOwnershipStarted = mock(() => {}), +) { let live = true; let controller!: ReturnType; appRenderer.renderer ||= { @@ -32,6 +35,7 @@ async function renderController(appRenderer = createTestAppRenderer()) { function Harness() { controller = useExtensionAppController({ createReviewCapabilityLease: () => ({ isLive: () => live }), + onOwnershipStarted, renderer: appRenderer.renderer, }); return null; @@ -44,6 +48,7 @@ async function renderController(appRenderer = createTestAppRenderer()) { destroyRenderer: () => { appRenderer.destroyed = true; }, + onOwnershipStarted, resume: appRenderer.resume, retire: () => { live = false; @@ -54,20 +59,33 @@ async function renderController(appRenderer = createTestAppRenderer()) { } describe("useExtensionAppController", () => { - test("suspends around extension work and passes its result through", async () => { + test("exposes renderer ownership while suspended and passes the application result through", async () => { const harness = await renderController(); const openInApp = harness.controller().createOpenInApp(); const calls: string[] = []; + let finish!: (value: number) => void; + const result = new Promise((resolve) => { + finish = resolve; + }); try { - await expect( - openInApp(async () => { - calls.push("app"); - return 42; - }), - ).resolves.toBe(42); + expect(harness.controller().isAppActive()).toBe(false); + const active = openInApp(async () => { + calls.push("app"); + expect(harness.controller().isAppActive()).toBe(true); + return await result; + }); + + expect(harness.controller().isAppActive()).toBe(true); expect(calls).toEqual(["app"]); + expect(harness.onOwnershipStarted).toHaveBeenCalledTimes(1); expect(harness.suspend).toHaveBeenCalledTimes(1); + expect(harness.resume).not.toHaveBeenCalled(); + + finish(42); + await expect(active).resolves.toBe(42); + + expect(harness.controller().isAppActive()).toBe(false); expect(harness.resume).toHaveBeenCalledTimes(1); } finally { await act(async () => harness.setup.renderer.destroy()); @@ -85,6 +103,7 @@ describe("useExtensionAppController", () => { throw failure; }), ).rejects.toBe(failure); + expect(harness.controller().isAppActive()).toBe(false); expect(harness.resume).toHaveBeenCalledTimes(1); } finally { await act(async () => harness.setup.renderer.destroy()); @@ -136,11 +155,15 @@ describe("useExtensionAppController", () => { try { const active = firstHarness.controller().createOpenInApp()(async () => await waiting); + expect(firstHarness.controller().isAppActive()).toBe(true); + expect(secondHarness.controller().isAppActive()).toBe(true); await expect(secondHarness.controller().createOpenInApp()(() => "never")).rejects.toThrow( "another application owns", ); finish(); await active; + expect(firstHarness.controller().isAppActive()).toBe(false); + expect(secondHarness.controller().isAppActive()).toBe(false); expect(renderer.suspend).toHaveBeenCalledTimes(1); expect(renderer.resume).toHaveBeenCalledTimes(1); } finally { @@ -161,11 +184,40 @@ describe("useExtensionAppController", () => { } }); + test("releases renderer ownership when suspension fails before the callback runs", async () => { + let suspensionAttempts = 0; + const renderer = createTestAppRenderer(() => { + suspensionAttempts += 1; + if (suspensionAttempts === 1) throw new Error("suspend failed"); + }); + const harness = await renderController(renderer); + let runs = 0; + + try { + await expect( + harness.controller().createOpenInApp()(() => { + runs += 1; + }), + ).rejects.toThrow("suspend failed"); + expect(runs).toBe(0); + expect(harness.controller().isAppActive()).toBe(false); + + await expect(harness.controller().createOpenInApp()(() => "restored")).resolves.toBe( + "restored", + ); + expect(harness.controller().isAppActive()).toBe(false); + expect(harness.resume).toHaveBeenCalledTimes(1); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + test("does not replace the application's result when renderer restoration fails", async () => { let controller!: ReturnType; function Harness() { controller = useExtensionAppController({ createReviewCapabilityLease: () => ({ isLive: () => true }), + onOwnershipStarted: () => {}, renderer: { isDestroyed: false, suspend: () => {}, diff --git a/src/ui/hooks/useExtensionAppController.ts b/src/ui/hooks/useExtensionAppController.ts index f089465a9..843886f7b 100644 --- a/src/ui/hooks/useExtensionAppController.ts +++ b/src/ui/hooks/useExtensionAppController.ts @@ -11,11 +11,17 @@ const activeAppByRenderer = new WeakMap(); /** Build command-scoped app handoffs around one renderer's terminal ownership. */ export function useExtensionAppController({ createReviewCapabilityLease, + onOwnershipStarted, renderer, }: { createReviewCapabilityLease: () => ExtensionCapabilityLease; + /** Settle host UI before the renderer gives the terminal away. */ + onOwnershipStarted: () => void; renderer: Pick; }) { + /** Report whether an extension application currently owns this renderer's terminal. */ + const isAppActive = useCallback(() => activeAppByRenderer.has(renderer), [renderer]); + const createOpenInApp = useCallback((): ExtensionOpenInApp => { const lease = createReviewCapabilityLease(); return async (run: () => Result | PromiseLike): Promise => { @@ -25,7 +31,7 @@ export function useExtensionAppController({ if (!lease.isLive()) { throw new Error("openInApp is unavailable after the review reloads."); } - if (activeAppByRenderer.has(renderer)) { + if (isAppActive()) { throw new Error("openInApp is unavailable while another application owns the terminal."); } @@ -33,6 +39,7 @@ export function useExtensionAppController({ activeAppByRenderer.set(renderer, ownership); let suspended = false; try { + onOwnershipStarted(); renderer.suspend(); suspended = true; return await run(); @@ -48,7 +55,7 @@ export function useExtensionAppController({ } } }; - }, [createReviewCapabilityLease, renderer]); + }, [createReviewCapabilityLease, isAppActive, onOwnershipStarted, renderer]); - return useMemo(() => ({ createOpenInApp }), [createOpenInApp]); + return useMemo(() => ({ createOpenInApp, isAppActive }), [createOpenInApp, isAppActive]); } diff --git a/src/ui/hooks/useExtensionDialogController.ts b/src/ui/hooks/useExtensionDialogController.ts index 0ac595b72..6fd1b925c 100644 --- a/src/ui/hooks/useExtensionDialogController.ts +++ b/src/ui/hooks/useExtensionDialogController.ts @@ -14,6 +14,8 @@ export interface ExtensionDialogController { inputValue: string; accept: (selectedIndexOverride?: number) => void; cancel: () => void; + /** Cancel the visible request and every queued request with their kind-specific values. */ + cancelAll: () => void; moveSelection: (delta: number) => void; pickOption: (index: number) => void; updateInput: (value: string) => void; @@ -87,6 +89,7 @@ export function useExtensionDialogController({ inputValue, accept, cancel, + cancelAll: queue.cancelAll, moveSelection, pickOption: setSelectedIndex, updateInput: setInputValue, diff --git a/src/ui/hooks/useExtensionWorkspaceControls.test.tsx b/src/ui/hooks/useExtensionWorkspaceControls.test.tsx index 142043940..f8878eacb 100644 --- a/src/ui/hooks/useExtensionWorkspaceControls.test.tsx +++ b/src/ui/hooks/useExtensionWorkspaceControls.test.tsx @@ -19,6 +19,11 @@ const EXPIRED = { reason: "unavailable", detail: "The review reloaded before this extension operation could finish.", } as const; +const APP_ACTIVE = { + ok: false, + reason: "unavailable", + detail: "Workspace writes are unavailable while another application owns the terminal.", +} as const; const WRITABLE_INPUT: CliInput = { kind: "vcs", staged: false, options: {} }; const tempDirs: string[] = []; @@ -66,6 +71,7 @@ async function renderController({ workspaceFileWriter?: WorkspaceFileWriter; } = {}) { let live = true; + let appActive = false; let controller!: ReturnType; let replaceInputs!: (next: { files: readonly WorkspaceFileSource[]; @@ -77,6 +83,7 @@ async function renderController({ confirm: (options: ExtensionConfirmOptions) => confirm(options, extensionId), }); const createReviewCapabilityLease = () => ({ isLive: () => live }); + const isAppActive = () => appActive; function Harness() { const [liveInputs, setLiveInputs] = useState({ files, input, root }); @@ -87,6 +94,7 @@ async function renderController({ createExtensionDialogs, createReviewCapabilityLease, ...liveInputs, + isAppActive, onWorkspaceWriteCompleted, runWorkspaceWrite, workspaceFileWriter, @@ -97,7 +105,13 @@ async function renderController({ const setup = await testRender(, { width: 20, height: 4 }); await act(async () => setup.renderOnce()); return { + claimApp: () => { + appActive = true; + }, controller: () => controller, + releaseApp: () => { + appActive = false; + }, replaceInputs: async (next: { files: readonly WorkspaceFileSource[]; input: CliInput; @@ -226,7 +240,14 @@ describe("useExtensionWorkspaceControls reads", () => { test("resolves live app locations and makes retained resolvers inert", async () => { const root = createTestRoot(); - const harness = await renderController({ root }); + const harness = await renderController({ + files: [ + createTestFile({ + sourcePaths: { old: null, new: join(root, "alpha.txt") }, + }), + ], + root, + }); const workspace = harness.controller().createWorkspaceControls("probe"); try { @@ -299,6 +320,140 @@ describe("useExtensionWorkspaceControls lifecycle", () => { }); describe("useExtensionWorkspaceControls writes", () => { + test("refuses writes during app ownership without disabling reads or retained controls", async () => { + const root = createTestRoot(); + let prompts = 0; + let writes = 0; + const harness = await renderController({ + files: [ + createTestFile({ + sourcePaths: { old: null, new: join(root, "alpha.txt") }, + }), + ], + root, + confirm: async () => { + prompts += 1; + return true; + }, + workspaceFileWriter: async () => { + writes += 1; + }, + }); + const workspace = harness.controller().createWorkspaceControls("probe"); + harness.claimApp(); + + try { + expect(workspace.canWriteDocument("alpha")).toBe(false); + await expect( + workspace.writeDocument({ fileId: "alpha", text: "replacement" }), + ).resolves.toEqual(APP_ACTIVE); + await expect(workspace.writeDocument({ fileId: "", text: "replacement" })).rejects.toThrow( + "non-empty fileId", + ); + await expect(workspace.readDocument("alpha", "new")).resolves.toBe("new alpha"); + expect(workspace.resolveLocation({ fileId: "alpha" })).toEqual({ + path: join(root, "alpha.txt"), + line: 1, + }); + expect(prompts).toBe(0); + expect(writes).toBe(0); + + harness.releaseApp(); + expect(workspace.canWriteDocument("alpha")).toBe(true); + await expect( + workspace.writeDocument({ fileId: "alpha", text: "replacement" }), + ).resolves.toEqual({ ok: true }); + expect(prompts).toBe(1); + expect(writes).toBe(1); + } finally { + await destroy(harness.setup); + } + }); + + test("refuses ownership acquired during verification or consent and preserves stale precedence", async () => { + let prompts = 0; + const verifyingHarness = await renderController({ + confirm: async () => { + prompts += 1; + return true; + }, + }); + + try { + const pending = verifyingHarness + .controller() + .createWorkspaceControls("probe") + .writeDocument({ fileId: "alpha", text: "replacement" }); + verifyingHarness.claimApp(); + await expect(pending).resolves.toEqual(APP_ACTIVE); + expect(prompts).toBe(0); + } finally { + await destroy(verifyingHarness.setup); + } + + let confirmStarted!: () => void; + const started = new Promise((resolve) => { + confirmStarted = resolve; + }); + let resolveConfirm!: (confirmed: boolean) => void; + const confirmation = new Promise((resolve) => { + resolveConfirm = resolve; + }); + let writes = 0; + const consentHarness = await renderController({ + confirm: async () => { + confirmStarted(); + return await confirmation; + }, + workspaceFileWriter: async () => { + writes += 1; + }, + }); + + try { + const pending = consentHarness + .controller() + .createWorkspaceControls("probe") + .writeDocument({ fileId: "alpha", text: "replacement" }); + await started; + consentHarness.claimApp(); + resolveConfirm(true); + await expect(pending).resolves.toEqual(APP_ACTIVE); + expect(writes).toBe(0); + } finally { + await destroy(consentHarness.setup); + } + + let staleConfirmStarted!: () => void; + const staleStarted = new Promise((resolve) => { + staleConfirmStarted = resolve; + }); + let resolveStaleConfirm!: (confirmed: boolean) => void; + const staleConfirmation = new Promise((resolve) => { + resolveStaleConfirm = resolve; + }); + const staleHarness = await renderController({ + confirm: async () => { + staleConfirmStarted(); + return await staleConfirmation; + }, + }); + + try { + const pending = staleHarness + .controller() + .createWorkspaceControls("probe") + .writeDocument({ fileId: "alpha", text: "replacement" }); + await staleStarted; + staleHarness.claimApp(); + staleHarness.retire(); + resolveStaleConfirm(true); + await expect(pending).resolves.toEqual(EXPIRED); + } finally { + await destroy(staleHarness.setup); + } + }); + test("throws for malformed requests and refuses unwritable reviews without prompting", async () => { let prompts = 0; const harness = await renderController({ @@ -574,6 +729,7 @@ describe("useExtensionWorkspaceControls writes", () => { .writeDocument({ fileId: "alpha", text: "replacement\n" }); await started; harness.retire(); + harness.claimApp(); finishWrite(); await expect(pending).resolves.toEqual({ ok: true }); diff --git a/src/ui/hooks/useExtensionWorkspaceControls.ts b/src/ui/hooks/useExtensionWorkspaceControls.ts index 40b8c0d89..caafb8600 100644 --- a/src/ui/hooks/useExtensionWorkspaceControls.ts +++ b/src/ui/hooks/useExtensionWorkspaceControls.ts @@ -51,12 +51,22 @@ function expiredWorkspaceWrite(): ExtensionWorkspaceWriteResult { }; } +/** Describe a write that cannot ask for consent while another application owns the terminal. */ +function appActiveWorkspaceWrite(): ExtensionWorkspaceWriteResult { + return { + ok: false, + reason: "unavailable", + detail: "Workspace writes are unavailable while another application owns the terminal.", + }; +} + /** Own live reviewed-document inputs and host-mediated extension workspace operations. */ export function useExtensionWorkspaceControls({ createExtensionDialogs, createReviewCapabilityLease, files, input, + isAppActive, onWorkspaceWriteCompleted, root, runWorkspaceWrite, @@ -70,6 +80,8 @@ export function useExtensionWorkspaceControls({ files: readonly WorkspaceFileSource[]; /** The current CLI review input that decides whether writes are meaningful. */ input: CliInput; + /** Whether an extension application currently owns the terminal renderer. */ + isAppActive: () => boolean; /** Reconcile the review currently mounted by the host after a successful write. */ onWorkspaceWriteCompleted: () => void; /** The current repository root, or the review's working directory. */ @@ -86,6 +98,12 @@ export function useExtensionWorkspaceControls({ const lease = createReviewCapabilityLease(); const resolveTarget = (fileId: string) => resolveExtensionWorkspaceWriteTarget({ fileId, ...liveInputsRef.current }); + const writeAuthorityRefusal = () => { + // Stale review authority remains the more fundamental refusal when both conditions apply. + if (!lease.isLive()) return expiredWorkspaceWrite(); + if (isAppActive()) return appActiveWorkspaceWrite(); + return null; + }; return { async readDocument(fileId: string, side: ExtensionFileSide) { @@ -106,21 +124,25 @@ export function useExtensionWorkspaceControls({ if (!lease.isLive()) return null; return resolveExtensionWorkspaceLocation({ files: liveInputsRef.current.files, - input: liveInputsRef.current.input, request: normalized, - root: liveInputsRef.current.root, }); }, canWriteDocument(fileId: string) { // An affordance probe answers false rather than throwing for malformed ids. - return lease.isLive() && typeof fileId === "string" && resolveTarget(fileId).writable; + return ( + lease.isLive() && + !isAppActive() && + typeof fileId === "string" && + resolveTarget(fileId).writable + ); }, async writeDocument( request: ExtensionWorkspaceWriteRequest, ): Promise { // Malformed requests are extension bugs, including after authority expires. const { fileId, text } = normalizeWorkspaceWriteRequest(request); - if (!lease.isLive()) return expiredWorkspaceWrite(); + const initialRefusal = writeAuthorityRefusal(); + if (initialRefusal) return initialRefusal; const target = resolveTarget(fileId); if (!target.writable) { @@ -136,7 +158,8 @@ export function useExtensionWorkspaceControls({ root, }); const refusal = await verifyTarget(); - if (!lease.isLive()) return expiredWorkspaceWrite(); + const verifiedRefusal = writeAuthorityRefusal(); + if (verifiedRefusal) return verifiedRefusal; if (refusal) { return { ok: false, reason: "unavailable", detail: refusal }; } @@ -146,7 +169,8 @@ export function useExtensionWorkspaceControls({ body: `Extension ${extensionId} will replace this file's contents on disk.`, confirmLabel: "write", }); - if (!lease.isLive()) return expiredWorkspaceWrite(); + const consentRefusal = writeAuthorityRefusal(); + if (consentRefusal) return consentRefusal; if (!confirmed) { return { ok: false, @@ -156,13 +180,15 @@ export function useExtensionWorkspaceControls({ } const changedTargetRefusal = await verifyTarget(); - if (!lease.isLive()) return expiredWorkspaceWrite(); + const reverifiedRefusal = writeAuthorityRefusal(); + if (reverifiedRefusal) return reverifiedRefusal; if (changedTargetRefusal) { return { ok: false, reason: "unavailable", detail: changedTargetRefusal }; } // Authority remains revocable until the host atomically starts the filesystem write. - if (!lease.isLive()) return expiredWorkspaceWrite(); + const writeBoundaryRefusal = writeAuthorityRefusal(); + if (writeBoundaryRefusal) return writeBoundaryRefusal; try { const started = await runWorkspaceWrite(() => workspaceFileWriter(target.absolutePath, text), @@ -187,6 +213,7 @@ export function useExtensionWorkspaceControls({ [ createExtensionDialogs, createReviewCapabilityLease, + isAppActive, onWorkspaceWriteCompleted, runWorkspaceWrite, workspaceFileWriter, diff --git a/src/ui/lib/extensionWorkspace.test.ts b/src/ui/lib/extensionWorkspace.test.ts index 0f6f4a6b1..f10832432 100644 --- a/src/ui/lib/extensionWorkspace.test.ts +++ b/src/ui/lib/extensionWorkspace.test.ts @@ -13,12 +13,6 @@ import { const ROOT = resolve(sep, "repo"); const NO_OPTIONS: CommonOptions = {}; -const WORKING_TREE_INPUT = { - kind: "vcs", - staged: false, - options: NO_OPTIONS, -} satisfies CliInput; - /** One reviewed file as the workspace policy sees it, changed unless told otherwise. */ function createTestWorkspaceFile( overrides: Partial = {}, @@ -250,20 +244,24 @@ describe("extension workspace write requests", () => { }); describe("extension workspace locations", () => { - test("resolves the repository path and maps old-side lines from parsed hunk metadata", () => { - const file = createTestDiffFile({ - id: "alpha", - path: "packages/app/alpha.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nfour\n", - }); + test("uses new-side provenance and maps old-side lines from parsed hunk metadata", () => { + const file = { + ...createTestDiffFile({ + id: "alpha", + path: "packages/app/alpha.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + }), + sourcePaths: { + old: join(ROOT, "archive", "alpha.ts"), + new: join(ROOT, "packages", "app", "alpha.ts"), + }, + }; expect( resolveExtensionWorkspaceLocation({ files: [file], - input: WORKING_TREE_INPUT, request: { fileId: "alpha", hunkIndex: 0, line: { side: "old", line: 3 } }, - root: ROOT, }), ).toEqual({ path: join(ROOT, "packages", "app", "alpha.ts"), line: 2 }); }); @@ -282,93 +280,126 @@ describe("extension workspace locations", () => { expect( resolveExtensionWorkspaceLocation({ files: [createTestWorkspaceFile({ metadata: undefined })], - input: WORKING_TREE_INPUT, request: { fileId: "alpha" }, - root: ROOT, }), ).toBeNull(); }); test("preserves old-side offsets through context and multi-line replacements", () => { - const removed = createTestDiffFile({ - id: "removed", - path: "removed.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nfour\n", - context: 1, - }); - const replaced = createTestDiffFile({ - id: "replaced", - path: "replaced.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nTWO\nTHREE\nfour\n", - }); + const removed = { + ...createTestDiffFile({ + id: "removed", + path: "removed.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + context: 1, + }), + sourcePaths: { old: null, new: join(ROOT, "removed.ts") }, + }; + const replaced = { + ...createTestDiffFile({ + id: "replaced", + path: "replaced.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nTWO\nTHREE\nfour\n", + }), + sourcePaths: { old: null, new: join(ROOT, "replaced.ts") }, + }; expect( resolveExtensionWorkspaceLocation({ files: [removed], - input: WORKING_TREE_INPUT, request: { fileId: "removed", hunkIndex: 0, line: { side: "old", line: 3 } }, - root: ROOT, })?.line, ).toBe(2); expect( resolveExtensionWorkspaceLocation({ files: [replaced], - input: WORKING_TREE_INPUT, request: { fileId: "replaced", hunkIndex: 0, line: { side: "old", line: 3 } }, - root: ROOT, })?.line, ).toBe(3); }); - test("uses direct comparison provenance and refuses unattested patch paths", () => { - const file = createTestDiffFile({ id: "alpha", path: "after.ts" }); - const directInput = { - kind: "diff", - left: "nested/before.ts", - right: "nested/after.ts", - options: NO_OPTIONS, - } satisfies CliInput; + test("uses concrete provenance instead of reviewed display paths", () => { + const file = { + ...createTestDiffFile({ id: "alpha", path: "display/after.ts" }), + sourcePaths: { + old: join(ROOT, "concrete", "before.ts"), + new: join(ROOT, "concrete", "after.ts"), + }, + }; expect( resolveExtensionWorkspaceLocation({ files: [file], - input: directInput, request: { fileId: "alpha", line: { side: "new", line: 2 } }, - root: ROOT, }), - ).toEqual({ path: join(ROOT, "nested", "after.ts"), line: 2 }); + ).toEqual({ path: join(ROOT, "concrete", "after.ts"), line: 2 }); expect( resolveExtensionWorkspaceLocation({ - files: [file], - input: { kind: "patch", text: file.patch, options: NO_OPTIONS }, + files: [{ ...file, sourcePaths: undefined }], request: { fileId: "alpha" }, - root: ROOT, }), ).toBeNull(); }); test("resolves deleted direct comparisons to their old-side source", () => { - const file = createTestDiffFile({ - id: "deleted", - path: "deleted.ts", - before: "one\ntwo\n", - after: "", - }); + const file = { + ...createTestDiffFile({ + id: "deleted", + path: "deleted.ts", + before: "one\ntwo\n", + after: "", + }), + sourcePaths: { old: join(ROOT, "archive", "deleted.ts"), new: null }, + }; expect( resolveExtensionWorkspaceLocation({ files: [file], - input: { - kind: "diff", - left: "archive/deleted.ts", - right: "/dev/null", - options: NO_OPTIONS, - }, request: { fileId: "deleted", hunkIndex: 0, line: { side: "old", line: 2 } }, - root: ROOT, }), ).toEqual({ path: join(ROOT, "archive", "deleted.ts"), line: 2 }); }); + + test("rejects requested absent sides before considering opposite-side provenance", () => { + const added = { + ...createTestDiffFile({ id: "added", before: "", after: "new\n" }), + sourcePaths: { old: null, new: join(ROOT, "added.ts") }, + }; + const deleted = { + ...createTestDiffFile({ id: "deleted", before: "old\n", after: "" }), + sourcePaths: { old: join(ROOT, "deleted.ts"), new: null }, + }; + + expect( + resolveExtensionWorkspaceLocation({ + files: [added], + request: { fileId: "added", line: { side: "old", line: 1 } }, + }), + ).toBeNull(); + expect( + resolveExtensionWorkspaceLocation({ + files: [deleted], + request: { fileId: "deleted", line: { side: "new", line: 1 } }, + }), + ).toBeNull(); + }); + + test("returns null when the relevant side is virtual or its path is not absolute", () => { + const file = createTestDiffFile({ id: "alpha" }); + + expect( + resolveExtensionWorkspaceLocation({ + files: [{ ...file, sourcePaths: { old: join(ROOT, "old.ts"), new: null } }], + request: { fileId: "alpha", hunkIndex: 0, line: { side: "old", line: 1 } }, + }), + ).toBeNull(); + expect( + resolveExtensionWorkspaceLocation({ + files: [{ ...file, sourcePaths: { old: null, new: "relative/alpha.ts" } }], + request: { fileId: "alpha" }, + }), + ).toBeNull(); + }); }); diff --git a/src/ui/lib/extensionWorkspace.ts b/src/ui/lib/extensionWorkspace.ts index 18dfcf9eb..9d555abbb 100644 --- a/src/ui/lib/extensionWorkspace.ts +++ b/src/ui/lib/extensionWorkspace.ts @@ -19,7 +19,7 @@ import { isAbsolute, relative, resolve, sep } from "node:path"; import { normalizeDiffPath } from "../../core/changeset/diffPaths"; -import type { FileSourceSide } from "../../core/changeset/fileSource"; +import type { FileSourcePaths, FileSourceSide } from "../../core/changeset/fileSource"; import { canReloadInput } from "../../core/run/inputReload"; import type { CliInput } from "../../core/run/commandInputs"; import { readMetadataChangeType } from "../../extensions/events"; @@ -41,6 +41,8 @@ export interface WorkspaceFileSource { isTooLarge?: boolean; /** Absent when the loader had no reachable source for this file. */ sourceFetcher?: { getFullText(side: FileSourceSide): Promise }; + /** Exact absolute paths for only the sides backed by the live filesystem. */ + sourcePaths?: FileSourcePaths; } /** One reviewed document side's read, already bound to the file that answers it. */ @@ -120,7 +122,7 @@ export function normalizeWorkspaceLocationRequest( }; } -/** Translate one old-side line to its corresponding working-tree line. */ +/** Translate one old-side line to its corresponding filesystem-backed new-side line. */ function lineOnDisk(hunk: WorkspaceLocationHunk, deletionLine: number) { let deletionCursor = hunk.deletionStart; let additionCursor = hunk.additionCount === 0 ? hunk.additionStart + 1 : hunk.additionStart; @@ -147,20 +149,24 @@ function lineOnDisk(hunk: WorkspaceLocationHunk, deletionLine: number) { /** Resolve a reviewed source address against the authoritative parsed diff. */ export function resolveExtensionWorkspaceLocation({ files, - input, request, - root, }: { files: readonly WorkspaceFileSource[]; - input: CliInput; request: WorkspaceLocationRequestFields; - root: string; }): ExtensionWorkspaceLocation | null { const file = files.find((candidate) => candidate.id === request.fileId); if (!file) return null; const metadata = file.metadata as Partial | undefined; if (!metadata || typeof metadata.type !== "string" || !Array.isArray(metadata.hunks)) return null; + const deleted = metadata.type === "deleted"; + if ( + (metadata.type === "new" && request.line?.side === "old") || + (deleted && request.line?.side === "new") + ) { + return null; + } + let hunkIndex = request.hunkIndex; if (request.line?.side === "old" && metadata.type !== "deleted") { hunkIndex ??= metadata.hunks.findIndex( @@ -173,7 +179,6 @@ export function resolveExtensionWorkspaceLocation({ const hunk = hunkIndex === undefined ? undefined : metadata.hunks[hunkIndex]; if (hunkIndex !== undefined && !hunk) return null; - const deleted = metadata.type === "deleted"; let line: number; if (request.line?.side === (deleted ? "old" : "new")) { line = request.line.line; @@ -190,20 +195,8 @@ export function resolveExtensionWorkspaceLocation({ line = deleted ? (hunk?.deletionStart ?? 1) : (hunk?.additionStart ?? 1); } - let filePath: string; - if (input.kind === "patch") { - return null; - } else if (input.kind === "diff") { - const sourcePath = deleted ? input.left : input.right; - if (sourcePath === "/dev/null") return null; - filePath = resolve(root, sourcePath); - } else if (input.kind === "difftool") { - const sourcePath = input.path ?? (deleted ? input.left : input.right); - if (sourcePath === "/dev/null") return null; - filePath = resolve(root, sourcePath); - } else { - filePath = resolve(root, normalizeDiffPath(file.path) ?? file.path); - } + const filePath = file.sourcePaths?.[deleted ? "old" : "new"]; + if (!filePath || !isAbsolute(filePath)) return null; return { path: filePath, diff --git a/website/src/content/docs/docs/extend/extension-api.md b/website/src/content/docs/docs/extend/extension-api.md index d47206b7d..f37846ef1 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -9,7 +9,8 @@ The extension factory receives one API object. Registration calls are only valid The API generation this Hunk speaks (currently `16`). Branch on it if you want one file to support several Hunk versions. Version 16 adds temporary application -handoffs from command handlers; version 15 added `{ side, line }` to opted-in pane +handoffs and on-disk location resolution to command handlers; 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 and committed note-edit events; version 12 added responsive fractional pane @@ -341,7 +342,9 @@ owned by your command. Hunk suspends its renderer, awaits the callback, and restores the review in `finally`. Your extension owns execution and how file, line, hunk, or application state reaches the child process. One app can own the terminal at a time, and stale or concurrent handoffs reject before the callback -runs. +runs. Host-presented dialogs cancel immediately and workspace writes are +unavailable while the callback owns the terminal; reads and location resolution +remain available. ### Workspace documents @@ -366,7 +369,7 @@ if (file && ctx.workspace.canWriteDocument(file.id)) { Reads return the source represented by the review, including historical content in revision and stash reviews. Missing, unreadable, or oversized sources return `null`; reads never prompt. -`resolveLocation` maps a reviewed file id and optional hunk/source line onto an attested absolute path and line on disk. VCS reviews resolve against the repository and direct comparisons retain their concrete file path. Hunk uses parsed hunk metadata for old-side mapping; raw patches, missing hunks, and stale locations return `null`. +`resolveLocation` maps a reviewed file id and optional hunk/source line onto an attested absolute path and line on disk. Direct comparisons retain their concrete input path. Index, revision, stash, patch, merged, absent, and other virtual sides return `null` rather than borrowing a same-named checkout file. Hunk uses parsed hunk metadata for old-side mapping; missing hunks and stale locations also return `null`. Writes require a reloadable, unstaged working-tree review and a writable reviewed-file id. Hunk verifies the target, asks for attributed consent, verifies it again, writes it, and reloads the review. Other review kinds and deleted, binary, oversized, missing, symlinked, or root-escaping targets return `unavailable`. Cancellation returns `cancelled`; an attempted write failure returns `failed` with a displayable `detail`. diff --git a/website/src/content/docs/docs/extend/vcs-adapters.md b/website/src/content/docs/docs/extend/vcs-adapters.md index 7a4522245..8313eef57 100644 --- a/website/src/content/docs/docs/extend/vcs-adapters.md +++ b/website/src/content/docs/docs/extend/vcs-adapters.md @@ -32,11 +32,12 @@ The ids Hunk ships with — `git`, `jj`, and `sl` — are reserved. An adapter t A `load` result is patch text plus how to label it. Everything else on it is optional, and each optional field buys one thing: -| Field | What it adds | -| ---------------- | ----------------------------------------------------------------- | -| `untrackedPaths` | files your VCS calls unknown, synthesized into added-file diffs | -| `readFileSource` | exact whole-file contents, for context expansion and highlighting | -| `extraFiles` | files reviewed outside the patch, including skipped placeholders | +| Field | What it adds | +| ----------------------- | ----------------------------------------------------------------- | +| `untrackedPaths` | files your VCS calls unknown, synthesized into added-file diffs | +| `readFileSource` | exact whole-file contents, for context expansion and highlighting | +| `resolveFileSourcePath` | exact filesystem provenance for application location handoff | +| `extraFiles` | files reviewed outside the patch, including skipped placeholders | `untrackedPaths` is the shorthand: list the repo-root-relative paths your VCS reports as unknown and Hunk synthesizes the added-file diffs for you, skipping binaries and files too large to render. Honor `input.options.excludeUntracked` when you do, so `--exclude-untracked` still means what it says. The other two are covered below. @@ -114,12 +115,18 @@ async load(input, ctx) { } return changeType === "deleted" ? null : hgCat(newRev, path); }, + resolveFileSourcePath: ({ path, changeType, side }) => { + if (side !== "new" || changeType === "deleted" || input.range) return null; + return join(ctx.cwd, path); + }, }; } ``` Return `null` for a side that has no content — the old side of an added file, a path the revision never contained — rather than throwing. Return `{ kind: "too-large", maxBytes }` when fetching the source would exceed your resource limit; Hunk shows expansion as unavailable without treating the result as an extension failure. Hunk calls the reader **at most once per file and side** and caches what it resolves, so you do not need your own cache, and it never calls it for a file the diff reports as binary. Leaving `readFileSource` off is fine: Hunk falls back to the content the patch itself carries, which renders the same diff with less context available. +`resolveFileSourcePath` is independent of source reads because binary and skipped files can still have real paths. Return an absolute path only when that exact reviewed side is filesystem-backed. Return `null` for index, revision, stash, patch, merged, absent, and other virtual sides even if a same-named checkout file exists. Hunk uses this provenance for `ctx.workspace.resolveLocation` and never derives historical paths from display names. + ## Files outside the patch `extraFiles` lists files to review that your `patchText` does not contain, in the order they should appear. Each entry is one of two kinds, and Hunk builds the diff model for both — you describe files, you never assemble them. From 5c85c55711e0051297953657a03bb88ea971a76f Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Mon, 31 Aug 2026 17:52:33 -0400 Subject: [PATCH 4/4] test(windows): normalize filesystem source paths --- src/core/changeset/loaders.test.ts | 26 ++++++++++++------- src/extensions/default/vcs/git/index.test.ts | 6 ++--- .../default/vcs/jujutsu/index.test.ts | 8 +++--- .../default/vcs/sapling/index.test.ts | 8 +++--- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/core/changeset/loaders.test.ts b/src/core/changeset/loaders.test.ts index ad5a861a5..b9e008d78 100644 --- a/src/core/changeset/loaders.test.ts +++ b/src/core/changeset/loaders.test.ts @@ -515,10 +515,10 @@ describe("loadAppBootstrap", () => { }); expect(bootstrap.changeset.files[0]?.metadata.hunks).toHaveLength(0); expect(bootstrap.changeset.files[0]?.sourceFetcher).toBeUndefined(); - expect(bootstrap.changeset.files[0]?.sourcePaths).toEqual({ - old: null, - new: join(dir, "large.txt"), - }); + expect(bootstrap.changeset.files[0]?.sourcePaths?.old).toBeNull(); + expect(normalizeComparablePath(bootstrap.changeset.files[0]!.sourcePaths!.new!)).toBe( + normalizeComparablePath(join(dir, "large.txt")), + ); }); test("keeps generated large untracked files as skipped placeholders", async () => { @@ -545,10 +545,10 @@ describe("loadAppBootstrap", () => { expect(bootstrap.changeset.files[0]?.statsTruncated).toBe(false); expect(bootstrap.changeset.files[0]?.metadata.hunks).toHaveLength(0); expect(bootstrap.changeset.files[0]?.sourceFetcher).toBeUndefined(); - expect(bootstrap.changeset.files[0]?.sourcePaths).toEqual({ - old: null, - new: join(dir, "large.txt"), - }); + expect(bootstrap.changeset.files[0]?.sourcePaths?.old).toBeNull(); + expect(normalizeComparablePath(bootstrap.changeset.files[0]!.sourcePaths!.new!)).toBe( + normalizeComparablePath(join(dir, "large.txt")), + ); }); test("caps skipped untracked-file stats when byte-size detection would require a full huge read", async () => { @@ -1934,7 +1934,10 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(file?.sourceFetcher).toBeDefined(); expect(await file?.sourceFetcher?.getFullText("new")).toBe("second\n"); expect(await file?.sourceFetcher?.getFullText("old")).toBe("first\n"); - expect(file?.sourcePaths).toEqual({ old: null, new: join(dir, "value.txt") }); + expect(file?.sourcePaths?.old).toBeNull(); + expect(normalizeComparablePath(file!.sourcePaths!.new!)).toBe( + normalizeComparablePath(join(dir, "value.txt")), + ); }); test("git source fetchers use the custom git executable from bootstrap loading", async () => { @@ -2135,7 +2138,10 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(untracked?.sourceFetcher).toBeDefined(); expect(await untracked?.sourceFetcher?.getFullText("new")).toBe("added contents\n"); expect(await untracked?.sourceFetcher?.getFullText("old")).toBeNull(); - expect(untracked?.sourcePaths).toEqual({ old: null, new: join(dir, "added.txt") }); + expect(untracked?.sourcePaths?.old).toBeNull(); + expect(normalizeComparablePath(untracked!.sourcePaths!.new!)).toBe( + normalizeComparablePath(join(dir, "added.txt")), + ); }); test("deleted Unicode files attach a fetcher with new=null and old source", async () => { diff --git a/src/extensions/default/vcs/git/index.test.ts b/src/extensions/default/vcs/git/index.test.ts index b56198f60..e34101ef7 100644 --- a/src/extensions/default/vcs/git/index.test.ts +++ b/src/extensions/default/vcs/git/index.test.ts @@ -133,9 +133,9 @@ describe("GitVcsAdapter", () => { expect(await readSource?.({ ...trackedFile, side: "old" })).toBe("old\n"); expect(await readSource?.({ ...trackedFile, side: "new" })).toBe("new\n"); expect(result.resolveFileSourcePath?.({ ...trackedFile, side: "old" })).toBeNull(); - expect(result.resolveFileSourcePath?.({ ...trackedFile, side: "new" })).toBe( - join(repo, "tracked.txt"), - ); + expect( + normalizeComparablePath(result.resolveFileSourcePath!({ ...trackedFile, side: "new" })!), + ).toBe(normalizeComparablePath(join(repo, "tracked.txt"))); expect( result.resolveFileSourcePath?.({ ...trackedFile, changeType: "new", side: "old" }), ).toBeNull(); diff --git a/src/extensions/default/vcs/jujutsu/index.test.ts b/src/extensions/default/vcs/jujutsu/index.test.ts index 76c825113..231dddbe5 100644 --- a/src/extensions/default/vcs/jujutsu/index.test.ts +++ b/src/extensions/default/vcs/jujutsu/index.test.ts @@ -131,9 +131,11 @@ describe("JjVcsAdapter", () => { expect(await diffResult.readFileSource?.({ ...reviewedFile, side: "old" })).toBe("one\n"); expect(await diffResult.readFileSource?.({ ...reviewedFile, side: "new" })).toBe("two\n"); expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "old" })).toBeNull(); - expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "new" })).toBe( - join(repo, "file.txt"), - ); + expect( + normalizeComparablePath( + diffResult.resolveFileSourcePath!({ ...reviewedFile, side: "new" })!, + ), + ).toBe(normalizeComparablePath(join(repo, "file.txt"))); const equivalentDiffResult = await JjVcsAdapter.operations["working-tree-diff"]!.load( diffInput, { cwd: repo }, diff --git a/src/extensions/default/vcs/sapling/index.test.ts b/src/extensions/default/vcs/sapling/index.test.ts index fe0b4102c..ca53df285 100644 --- a/src/extensions/default/vcs/sapling/index.test.ts +++ b/src/extensions/default/vcs/sapling/index.test.ts @@ -124,9 +124,11 @@ describe("SaplingVcsAdapter", () => { isUntracked: false, } as const; expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "old" })).toBeNull(); - expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "new" })).toBe( - join(repo, "file.txt"), - ); + expect( + normalizeComparablePath( + diffResult.resolveFileSourcePath!({ ...reviewedFile, side: "new" })!, + ), + ).toBe(normalizeComparablePath(join(repo, "file.txt"))); const showInput = { kind: "show",