diff --git a/.changeset/vcs-extension-ownership.md b/.changeset/vcs-extension-ownership.md new file mode 100644 index 000000000..44282f5f8 --- /dev/null +++ b/.changeset/vcs-extension-ownership.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Decouple bundled VCS providers from core, add provider-neutral repository bootstrapping, and let extension source readers report files that exceed their safe read limit. diff --git a/AGENTS.md b/AGENTS.md index f15dcebd9..394c2b40e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,9 @@ CLI input - CLI entrypoints: `diff`, `show`, `stash show`, `patch`, `pager`, `difftool`. - All input sources normalize into one internal changeset model. +- Bundled VCS implementations live under `src/extensions/default/vcs//` and consume the + public extension contract; `src/app` composes their registrations into the provider-neutral + core VCS catalog. Do not add provider commands, spawning, or source readers under `src/core`. - Pager mode has two paths: full diff UI for patch-like stdin, plain-text fallback for non-diff pager content. - View defaults are layered through built-ins, user config, repo `.hunk/config.toml`, command sections, pager sections, and CLI flags. - `hunk daemon serve` runs one loopback daemon that brokers agent commands to many live Hunk sessions. Normal Hunk sessions should auto-start and register with that daemon when session brokering is enabled. Keep it local-only and session-brokered rather than opening per-TUI ports. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index ffde8f232..f0106beb7 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -17,11 +17,11 @@ object and registry collection (`src/extensions/runExtension.ts`): `src/extensions/trust.ts`. - **Bundled extensions** live in `src/extensions/default/` and are compiled into the binary. `default/vcs/{git,jujutsu,sapling}` is statically imported - and loaded synchronously _from VCS adapter resolution_ - (`default/vcs/index.ts`), so backends exist during config resolution — that - load path must stay renderer-free. `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. + 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. Git and the built-in file navigation use the public `registerVcsAdapter` and `registerPane` paths. The current-line lens remains an installable example. @@ -34,7 +34,7 @@ owns for commands (`.`), panes (`:`), and config (`[extension.]`). `host.ts` is the one place those ids are vetted — discovery stays a pure filesystem walk, and every way an id can be derived arrives there as `candidate.id`. It refuses -reserved ids (`hunk`, plus the bundled backends via `isVcsId`), ids outside +reserved ids (`hunk`, plus the base catalog's bundled backend ids), ids outside `/^[A-Za-z0-9][A-Za-z0-9_-]*$/` (a dot or colon would make the composed ids unsplittable), and the later of two sources claiming one id; each refusal is a load issue and costs only that extension. The rules themselves are stated in @@ -45,9 +45,13 @@ load issue and costs only that extension. The rules themselves are stated in Registrations (themes, file languages, VCS adapters, changeset transforms, panes, commands, lifecycle/UI events, and bus listeners) collect into one `ExtensionRegistry` (`src/extensions/types.ts`) and are resolved/applied -through `src/extensions/apply.ts` on both startup and reload. A factory that -throws is rolled back to its pre-run registration counts -(`runExtension.ts`); failures cost a warning, not the session. +through `src/extensions/apply.ts` on both startup and reload. Staged external-VCS +bootstrap retains the provisional candidate/config snapshot: a final pass that +only appends repo candidates extends the same registry, while a changed prefix +receives bounded `shutdown` before being rebuilt. Live registry replacement uses +the same shutdown/startup lifecycle. A factory that throws is rolled back to its +pre-run registration counts (`runExtension.ts`); failures cost a warning, not the +session. ## Host-served runtime modules @@ -200,16 +204,22 @@ none — which is why the visible menu list is derived from the menus record ## VCS adapters -`src/core/vcs/index.ts` is the single assembly point ordering bundled + user -adapters by `detectionPriority` (Git is the baseline at 0; jj 200 / sl 100 -sit above it for colocated checkouts — the constants in -`src/extension-api/types.ts` document the reasoning). Detection is uniform -across tiers: nearest checkout wins, priority breaks equal-distance ties, an -explicit `vcs` id a loaded backend owns beats detection -(`src/extensions/apply.ts`). `src/extensions/vcsPatchResult.ts` is the one -conversion boundary where a published `ExtensionVcsPatchResult` becomes -Hunk's internal diff model — anything a backend needs that cannot be -expressed publicly is a real gap in the contract. +`src/core/vcs/index.ts` owns provider-neutral catalog ordering, lookup, +detection, and operation dispatch. `src/app/vcsCatalog.ts` composes bundled +registrations, while `src/app/sessionBootstrap.ts` extends that catalog with +accepted user adapters and threads the same value through loading, reload, and +watch. Detection is uniform across tiers: nearest checkout wins, priority breaks +equal-distance ties, and an explicit `vcs` id owned by the catalog wins. + +Provider implementations — command construction, spawning, error translation, +and exact-source reading — live entirely under +`src/extensions/default/vcs//`. `src/extensions/vcsPatchResult.ts` is +the one conversion boundary where a published `ExtensionVcsPatchResult` +becomes Hunk's internal diff model, including structural `too-large` source +results. `src/core/projectRoot.ts` treats `.hunk` as a provider-independent +bootstrap marker and also consults the available catalog; startup performs a +second root/config pass when a global, config-path, or CLI adapter recognizes a +repository unavailable to the bundled catalog. ## Public contract rules diff --git a/docs/extensions.md b/docs/extensions.md index b0a962bed..e28f8bf2e 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -187,7 +187,10 @@ writes to the extension and asks for consent. The factory receives one object. Registration calls are only valid while the factory is running; Hunk seals the object afterwards so a deferred callback -cannot mutate the registry mid-session. +cannot mutate the registry mid-session. Keep the factory registration-only: +start watchers, processes, connections, and other long-lived resources from +`startup`, and release them from `shutdown`. Extension-registry reloads create +new instances and run that shutdown/startup pair around the replacement. ### `hunk.apiVersion` @@ -290,7 +293,7 @@ carries two sets of markers. | ------------------------ | -------------------------------------------- | | bundled `jj` | 200 | | bundled `sl` | 100 | -| bundled `git` | 0 (`HUNK_CORE_VCS_DETECTION_PRIORITY`) | +| bundled `git` | 0 (`HUNK_VCS_DETECTION_BASELINE_PRIORITY`) | | your adapter, by default | -100 (`HUNK_DEFAULT_VCS_DETECTION_PRIORITY`) | Higher is consulted first; equal priorities fall back to registration order. @@ -303,12 +306,12 @@ silently changes how an existing repository is reviewed. Set `detectionPriority` explicitly to outrank a shipped backend; it is your machine. ```ts -import { HUNK_CORE_VCS_DETECTION_PRIORITY } from "hunkdiff/extension"; +import { HUNK_VCS_DETECTION_BASELINE_PRIORITY } from "hunkdiff/extension"; hunk.registerVcsAdapter({ id: "hg", name: "Mercurial", - detectionPriority: HUNK_CORE_VCS_DETECTION_PRIORITY + 10, + detectionPriority: HUNK_VCS_DETECTION_BASELINE_PRIORITY + 10, detect, }); ``` @@ -322,7 +325,13 @@ adapter list — and that second answer is the one the session uses. What detection never overrides is an explicit choice: a `vcs = ""` in Hunk config naming a backend this session loaded is honored as-is, however near a -checkout some other adapter finds. +checkout some other adapter finds. A repository-local adapter can bootstrap a +provider Hunk has never seen because `.hunk` itself establishes the project root; +global, config-path, and `--extension` adapters also participate in a staged +root/config pass before the review loads. When the final root only adds repo +candidates, Hunk extends the provisional registry instead of executing its +already loaded factories again. If repo config changes an existing extension's +factory config, Hunk sends that provisional instance `shutdown` before rebuilding it. #### Watch support @@ -381,7 +390,10 @@ async load(input, ctx) { ``` 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. Hunk calls the reader +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. When equivalent reloads close over the same source base, return the same @@ -1345,20 +1357,20 @@ the UI waiting for one. Alongside `cwd` and `notify`, every handler receives That means a `changeset_loaded` handler can reveal its extension's pane when it finds something worth showing — no keypress required. -| Event | Payload | When | -| ---------------------- | ----------------------- | -------------------------------------------------------- | -| `startup` | `{ cwd }` | once, after the app mounts with its first changeset | -| `changeset_loaded` | `{ changeset }` | first load and every reload | -| `selection_changed` | `{ fileId, hunkIndex }` | when the review selection settles (debounced ~150ms) | -| `file_viewed` | `{ file, hunkIndex }` | when selection settles on a file or a reload replaces it | -| `filter_changed` | `{ filter }` | whenever the file-filter query changes | -| `theme_changed` | `{ themeId }` | when the user commits a new theme | -| `layout_changed` | `{ mode, layout }` | mode or responsive split/stack layout changes | -| `watch_reload_pending` | `{}` | watcher observed a change before its reload check | -| `note_created` | `{ note }` | a user saves an inline review note | -| `note_edited` | `{ note }` | an in-progress inline note's body changes | -| `session_reload` | `{ changeset, reason }` | on every session reload | -| `shutdown` | `{}` | on exit, best-effort within a short timeout | +| Event | Payload | When | +| ---------------------- | ----------------------- | --------------------------------------------------------- | +| `startup` | `{ cwd }` | once per loaded instance, after its review UI mounts | +| `changeset_loaded` | `{ changeset }` | first load and every reload | +| `selection_changed` | `{ fileId, hunkIndex }` | when the review selection settles (debounced ~150ms) | +| `file_viewed` | `{ file, hunkIndex }` | when selection settles on a file or a reload replaces it | +| `filter_changed` | `{ filter }` | whenever the file-filter query changes | +| `theme_changed` | `{ themeId }` | when the user commits a new theme | +| `layout_changed` | `{ mode, layout }` | mode or responsive split/stack layout changes | +| `watch_reload_pending` | `{}` | watcher observed a change before its reload check | +| `note_created` | `{ note }` | a user saves an inline review note | +| `note_edited` | `{ note }` | an in-progress inline note's body changes | +| `session_reload` | `{ changeset, reason }` | on every session reload | +| `shutdown` | `{}` | before instance replacement or exit, with a short timeout | `selection_changed` is trailing-debounced on purpose: holding `[`/`]` retargets the selection many times a second, and handlers only care where the user landed. @@ -1375,8 +1387,9 @@ these events, and a `session_reload` may remap or drop notes without one either. A list accumulated from these events is therefore "notes the user saved here this session", not a complete review record; present it as such. -`shutdown` handlers get a short window (250ms) to finish before Hunk exits -anyway, so treat it as best-effort flushing rather than guaranteed cleanup. +`shutdown` handlers get a short window (250ms) to finish before Hunk replaces +the extension registry or exits anyway, so make cleanup prompt and idempotent. +The replacement instance receives `startup` after its review is mounted. ### `hunk.events` diff --git a/docs/source-architecture.md b/docs/source-architecture.md index 1dd35e26e..b67d135b0 100644 --- a/docs/source-architecture.md +++ b/docs/source-architecture.md @@ -10,7 +10,7 @@ Use it when adding a new module or deciding where an existing responsibility bel src/app/ executable composition: startup plans and shared session bootstrap src/core/ normalized review model, loading, patch handling, VCS contracts, configuration, and runtime primitives -src/core/vcs/ VCS-specific helpers and adapter-facing support code +src/core/vcs/ provider-neutral VCS catalog, contracts, operation dispatch, and host support src/extensions/ extension host, registry, trust, lifecycle, and bundled extensions src/session/ shared session protocol, schemas, types, agent surface, app bridge, and broker transport src/session/client/ shared session-daemon HTTP and compatibility client support @@ -32,16 +32,17 @@ one owns its behaviour. - `app` may compose `core`, `extensions`, `session`, and `ui`. - `ui` may consume core models and the extension/session contracts; it owns terminal rendering. -- `extensions` may consume core model and VCS contracts, but must stay renderer-free except for - `extensions/default/ui/`, which is the explicit bundled-sidebar boundary. -- `core` must not import `ui`. Shared data needed by both belongs in `core`, not `ui/lib`. +- `extensions` may consume provider-neutral core models and contracts, but bundled VCS provider + implementations must depend only on `hunkdiff/extension`, local modules, and `src/lib` utilities. + Renderer access remains limited to `extensions/default/ui/`, the bundled-sidebar boundary. +- `core` must not import `ui` or `extensions`. Shared data needed by both belongs in core-owned + structural contracts or `src/lib`, never in a reverse dependency. - `extension-api/types.ts` stays import-free. It is a published declaration boundary, enforced by the package checks. - `opentui` and `extension-api` are public entrypoint directories, not general internal buckets. -The boundary test intentionally enforces the currently mechanical rule (`core` cannot import -`ui`). Broader direction is reviewed at feature boundaries until the session consolidation is -complete. +`scripts/source-boundaries.test.ts` mechanically enforces `core -> ui`, `core -> extensions`, +and bundled-provider -> core boundaries, including the public extension-barrel requirement. ## Bootstrap invariant @@ -63,5 +64,6 @@ This is an incremental migration, not a bulk rename: 4. Prefer a named ownership boundary over a generic `lib` folder. 5. Update this map and feature architecture docs when a boundary changes. -Current first cuts place startup composition in `app/`, the shared Shiki theme catalog in -`core/themeCatalog.ts`, and bundled VCS implementation helpers in `core/vcs/`. +Current composition lives in `app/`: `app/vcsCatalog.ts` assembles bundled registrations into a +provider-neutral catalog, and `app/sessionBootstrap.ts` extends that catalog with user adapters. +Provider commands, source readers, and tests live under `extensions/default/vcs//`. diff --git a/packages/session-broker-core/src/brokerState.test.ts b/packages/session-broker-core/src/brokerState.test.ts index a37d92344..abf198181 100644 --- a/packages/session-broker-core/src/brokerState.test.ts +++ b/packages/session-broker-core/src/brokerState.test.ts @@ -218,6 +218,54 @@ describe("session broker state", () => { ); }); + test("resolves repo subdirectories to the nearest eligible registered root", () => { + const outer = createListedSession({ sessionId: "outer", repoRoot: "/repo", cwd: "/repo" }); + const inner = createListedSession({ + sessionId: "inner", + repoRoot: "/repo/packages/app", + cwd: "/repo/packages/app", + }); + + expect( + resolveSessionTarget([outer, inner], { + repoRoot: "/repo/packages/app/src", + repoBoundary: "/repo/packages/app", + }).sessionId, + ).toBe("inner"); + expect( + resolveSessionTarget([outer], { + repoRoot: "/repo/other", + repoBoundary: "/repo", + }).sessionId, + ).toBe("outer"); + expect(() => + resolveSessionTarget([outer], { + repoRoot: "/repo/packages/app/src", + repoBoundary: "/repo/packages/app", + }), + ).toThrow("No active session matches repoRoot"); + + // An external adapter may own a nested root inside the nearest bundled + // boundary. Its active session remains eligible and wins by distance. + const custom = createListedSession({ + sessionId: "custom", + repoRoot: "/repo/custom", + cwd: "/repo/custom", + }); + expect( + resolveSessionTarget([outer, custom], { + repoRoot: "/repo/custom/src", + repoBoundary: "/repo", + }).sessionId, + ).toBe("custom"); + + // Older clients omit the boundary; containment fallback remains compatible. + expect(resolveSessionTarget([outer], { repoRoot: "/repo/packages/app/src" }).sessionId).toBe( + "outer", + ); + expect(resolveSessionTarget([outer], { repoRoot: "/repo/..cache" }).sessionId).toBe("outer"); + }); + test("keeps session-path matching tied to the live session cwd", () => { const sessions = [ createListedSession({ diff --git a/packages/session-broker-core/src/brokerState.ts b/packages/session-broker-core/src/brokerState.ts index 215a800d7..43c104f71 100644 --- a/packages/session-broker-core/src/brokerState.ts +++ b/packages/session-broker-core/src/brokerState.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import { matchesSessionSelector, type SelectableSession } from "./selectors"; +import { matchesSessionSelector, repoSelectorDistance, type SelectableSession } from "./selectors"; import type { SessionRegistration, SessionServerMessage, @@ -60,11 +60,7 @@ export interface SessionBrokerViewAdapter< export type UpdateSnapshotResult = "updated" | "invalid" | "not-found"; -export interface SessionTargetSelector { - sessionId?: string; - sessionPath?: string; - repoRoot?: string; -} +export type SessionTargetSelector = SessionTargetInput; function describeSessionChoices( sessions: ListedSession[], @@ -104,11 +100,22 @@ export function resolveSessionTarget matchesSessionSelector(session, selector)); - if (matches.length === 0) { + const candidates = sessions + .map((session) => ({ + session, + distance: repoSelectorDistance(session, selector.repoRoot!, selector.repoBoundary), + })) + .filter( + (entry): entry is { session: ListedSession; distance: number } => entry.distance !== null, + ); + if (candidates.length === 0) { throw new Error(`No active session matches repoRoot ${selector.repoRoot}.`); } + const nearestDistance = Math.min(...candidates.map((entry) => entry.distance)); + const matches = candidates + .filter((entry) => entry.distance === nearestDistance) + .map((entry) => entry.session); if (matches.length > 1) { throw new Error( `Multiple active sessions match repoRoot ${selector.repoRoot}; specify sessionId instead. ` + diff --git a/packages/session-broker-core/src/selectors.test.ts b/packages/session-broker-core/src/selectors.test.ts new file mode 100644 index 000000000..fa8ca5564 --- /dev/null +++ b/packages/session-broker-core/src/selectors.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, test } from "bun:test"; +import { resolve } from "node:path"; +import { normalizeSessionSelector, repoSelectorDistance } from "./selectors"; + +describe("session selector paths", () => { + test("normalizes the repo path and its optional boundary", () => { + expect(normalizeSessionSelector({ repoRoot: "repo/src", repoBoundary: "repo" })).toEqual({ + repoRoot: resolve("repo/src"), + repoBoundary: resolve("repo"), + sessionPath: undefined, + }); + }); + + test("rejects session roots outside a supplied repository boundary", () => { + const boundary = resolve("repo", "nested"); + const selectorPath = resolve(boundary, "src"); + + expect( + repoSelectorDistance( + { sessionId: "outer", cwd: resolve("repo"), repoRoot: resolve("repo") }, + selectorPath, + boundary, + ), + ).toBeNull(); + expect( + repoSelectorDistance( + { sessionId: "inner", cwd: boundary, repoRoot: boundary }, + selectorPath, + boundary, + ), + ).toBe(1); + }); +}); diff --git a/packages/session-broker-core/src/selectors.ts b/packages/session-broker-core/src/selectors.ts index 93601b577..f6d9650d0 100644 --- a/packages/session-broker-core/src/selectors.ts +++ b/packages/session-broker-core/src/selectors.ts @@ -1,4 +1,4 @@ -import { resolve } from "node:path"; +import { isAbsolute, relative, resolve, sep } from "node:path"; import type { SessionTargetInput } from "./types"; export interface SelectableSession { @@ -7,6 +7,33 @@ export interface SelectableSession { repoRoot?: string; } +/** Return one path's relative containment depth below a candidate root. */ +function containmentDistance(root: string, candidate: string): number | null { + const offset = relative(root, candidate); + if (offset === ".." || offset.startsWith(`..${sep}`) || isAbsolute(offset)) { + return null; + } + + return offset === "" ? 0 : offset.split(/[\\/]+/).filter(Boolean).length; +} + +/** Return containment distance when a repo selector belongs to one eligible session root. */ +export function repoSelectorDistance( + session: SelectableSession, + selectorPath: string, + repoBoundary?: string, +): number | null { + if (!session.repoRoot) { + return null; + } + + if (repoBoundary && containmentDistance(repoBoundary, session.repoRoot) === null) { + return null; + } + + return containmentDistance(session.repoRoot, selectorPath); +} + /** Return whether one session matches the selector precedence shared by the broker and CLI. */ export function matchesSessionSelector( session: SelectableSession, @@ -25,7 +52,7 @@ export function matchesSessionSelector( } if (selector.repoRoot) { - return session.repoRoot === selector.repoRoot; + return repoSelectorDistance(session, selector.repoRoot, selector.repoBoundary) !== null; } return true; @@ -37,6 +64,7 @@ export function normalizeSessionSelector(selector: SessionTargetInput): SessionT ...selector, sessionPath: selector.sessionPath ? resolve(selector.sessionPath) : undefined, repoRoot: selector.repoRoot ? resolve(selector.repoRoot) : undefined, + repoBoundary: selector.repoBoundary ? resolve(selector.repoBoundary) : undefined, }; } diff --git a/packages/session-broker-core/src/types.ts b/packages/session-broker-core/src/types.ts index 53d3f729e..af5ce74ed 100644 --- a/packages/session-broker-core/src/types.ts +++ b/packages/session-broker-core/src/types.ts @@ -2,6 +2,8 @@ export interface SessionTargetInput { sessionId?: string; sessionPath?: string; repoRoot?: string; + /** Nearest project boundary known to the client for a repo-path selector. */ + repoBoundary?: string; } export interface SessionTerminalLocation { diff --git a/scripts/build-npm.ts b/scripts/build-npm.ts index 42a682a08..4ad4d95f5 100644 --- a/scripts/build-npm.ts +++ b/scripts/build-npm.ts @@ -132,9 +132,9 @@ runBun([ runBun(["x", "tsc", "-p", path.join(repoRoot, "tsconfig.extension.json")]); -// The extension entry re-exports core façade types, so its declarations span several -// source directories. Ship the emitted tree as-is and point the subpath export at a -// one-line barrel so consumers still resolve `hunkdiff/extension` from a single file. +// The extension entry emits only the import-free public API declaration tree. Ship it +// as-is and point the subpath export at a one-line barrel so consumers still resolve +// `hunkdiff/extension` from a single file. // The specifier carries an explicit `.js` extension because `moduleResolution: // "nodenext"` consumers reject extensionless relative imports in ESM declarations. cpSync(extensionTypesOutdir, extensionOutdir, { recursive: true }); diff --git a/scripts/check-pack.ts b/scripts/check-pack.ts index b50474de5..8923e712f 100644 --- a/scripts/check-pack.ts +++ b/scripts/check-pack.ts @@ -217,7 +217,12 @@ export default function (hunk: HunkExtensionAPI) { title: "Mercurial working copy", patchText: "", untrackedPaths: [], - readFileSource: async ({ path, side }) => (side === "old" ? null : path), + readFileSource: async ({ path, side }) => + side === "old" + ? null + : path.endsWith(".generated") + ? { kind: "too-large", maxBytes: 1_000_000 } + : path, extraFiles: [ { kind: "patch", path: "notes.md", patchText: "", isUntracked: true }, { diff --git a/scripts/source-boundaries.test.ts b/scripts/source-boundaries.test.ts index c6a0d0a1d..ff9a12171 100644 --- a/scripts/source-boundaries.test.ts +++ b/scripts/source-boundaries.test.ts @@ -1,11 +1,14 @@ import { describe, expect, test } from "bun:test"; -import { readdirSync, readFileSync } from "node:fs"; -import { join, relative, resolve } from "node:path"; +import { existsSync, readdirSync, readFileSync } from "node:fs"; +import { dirname, isAbsolute, join, relative, resolve, sep } from "node:path"; const REPO_ROOT = resolve(import.meta.dir, ".."); -const CORE_ROOT = join(REPO_ROOT, "src", "core"); +const SRC_ROOT = join(REPO_ROOT, "src"); +const CORE_ROOT = join(SRC_ROOT, "core"); +const EXTENSIONS_ROOT = join(SRC_ROOT, "extensions"); +const BUNDLED_PROVIDER_ROOT = join(EXTENSIONS_ROOT, "default", "vcs"); -/** Return every TypeScript source file below one directory, excluding colocated tests. */ +/** Return every production TypeScript source file below one directory. */ function sourceFiles(directory: string): string[] { return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { const path = join(directory, entry.name); @@ -16,18 +19,70 @@ function sourceFiles(directory: string): string[] { }); } -/** Find core modules that reach into the terminal-rendering layer through relative imports. */ -function coreUiImports() { - return sourceFiles(CORE_ROOT).flatMap((path) => { - const source = readFileSync(path, "utf8"); - return /(?:from\s*|import\s*\()["'](?:\.\.\/)+ui\//.test(source) - ? [relative(REPO_ROOT, path)] - : []; - }); +/** Read static and dynamic module specifiers from one source file. */ +function importSpecifiers(path: string) { + const source = readFileSync(path, "utf8"); + return [...source.matchAll(/(?:from\s*|import\s*\()["']([^"']+)["']/g)].map((match) => match[1]!); +} + +/** Resolve one relative source import sufficiently for architectural containment checks. */ +function resolveImport(path: string, specifier: string) { + if (!specifier.startsWith(".")) { + return undefined; + } + const base = resolve(dirname(path), specifier); + for (const candidate of [base, `${base}.ts`, `${base}.tsx`, join(base, "index.ts")]) { + if (existsSync(candidate)) { + return candidate; + } + } + return base; +} + +/** Find imports from one tree that resolve beneath a forbidden tree. */ +function forbiddenImports(sourceRoot: string, forbiddenRoot: string) { + return sourceFiles(sourceRoot).flatMap((path) => + importSpecifiers(path).flatMap((specifier) => { + const target = resolveImport(path, specifier); + if (!target) { + return []; + } + const offset = relative(forbiddenRoot, target); + const isForbidden = + offset === "" || (offset !== ".." && !offset.startsWith(`..${sep}`) && !isAbsolute(offset)); + return isForbidden ? [`${relative(REPO_ROOT, path)} -> ${specifier}`] : []; + }), + ); +} + +/** Find bundled provider imports that bypass the published extension barrel. */ +function privateProviderApiImports() { + return ["git", "jujutsu", "sapling"].flatMap((provider) => + sourceFiles(join(BUNDLED_PROVIDER_ROOT, provider)).flatMap((path) => + importSpecifiers(path).some((specifier) => specifier.includes("extension-api")) + ? [relative(REPO_ROOT, path)] + : [], + ), + ); } describe("source architecture boundaries", () => { test("keeps UI rendering out of core", () => { - expect(coreUiImports()).toEqual([]); + expect(forbiddenImports(CORE_ROOT, join(SRC_ROOT, "ui"))).toEqual([]); + }); + + test("keeps extension composition out of core", () => { + expect(forbiddenImports(CORE_ROOT, EXTENSIONS_ROOT)).toEqual([]); + }); + + test("keeps bundled provider implementations out of core", () => { + for (const file of ["git.ts", "gitSource.ts", "jujutsu.ts", "sapling.ts"]) { + expect(existsSync(join(CORE_ROOT, "vcs", file))).toBe(false); + } + }); + + test("keeps bundled providers on their public host contract", () => { + expect(forbiddenImports(BUNDLED_PROVIDER_ROOT, CORE_ROOT)).toEqual([]); + expect(privateProviderApiImports()).toEqual([]); }); }); diff --git a/src/app/extensionBootstrap.ts b/src/app/extensionBootstrap.ts new file mode 100644 index 000000000..b9ec2e31a --- /dev/null +++ b/src/app/extensionBootstrap.ts @@ -0,0 +1,101 @@ +import { resolveConfiguredCliInput, type HunkConfigResolution } from "../core/config"; +import { findProjectRootCandidate } from "../core/projectRoot"; +import type { CliInput } from "../core/types"; +import { extendVcsCatalog } from "../core/vcs"; +import type { VcsCatalog } from "../core/vcs/types"; +import { resolveExtensionVcsAdapters } from "../extensions/apply"; +import { bindExtensionEventBus, retireExtensionLoadResult } from "../extensions/events"; +import { loadStartupExtensions } from "../extensions/startup"; +import type { ExtensionNotificationHub } from "../extensions/notifications"; +import type { ExtensionLoadResult } from "../extensions/types"; + +export interface ResolveConfiguredExtensionsOptions { + runtimeInput: CliInput; + cwd: string; + env?: NodeJS.ProcessEnv; + baseVcsCatalog: VcsCatalog; + /** Initial resolution already needed by a caller before extension loading begins. */ + configured?: HunkConfigResolution; + /** Adapters already known before this load, such as the current live-session catalog. */ + discoveryCatalog?: VcsCatalog; + notifications?: ExtensionNotificationHub; +} + +export interface ResolveConfiguredExtensionsDeps { + resolveConfiguredCliInputImpl?: typeof resolveConfiguredCliInput; + loadStartupExtensionsImpl?: typeof loadStartupExtensions; + findProjectRootCandidateImpl?: typeof findProjectRootCandidate; +} + +export interface ResolvedConfiguredExtensions { + configured: HunkConfigResolution; + extensions: ExtensionLoadResult; +} + +/** + * Resolve configuration and user extensions, repeating root discovery once when + * a newly loaded adapter recognizes a repository the starting catalog could not. + */ +export async function resolveConfiguredExtensions( + options: ResolveConfiguredExtensionsOptions, + deps: ResolveConfiguredExtensionsDeps = {}, +): Promise { + const resolveConfiguredCliInputImpl = + deps.resolveConfiguredCliInputImpl ?? resolveConfiguredCliInput; + const loadStartupExtensionsImpl = deps.loadStartupExtensionsImpl ?? loadStartupExtensions; + const findProjectRootCandidateImpl = + deps.findProjectRootCandidateImpl ?? findProjectRootCandidate; + let configured = + options.configured ?? + resolveConfiguredCliInputImpl(options.runtimeInput, { + cwd: options.cwd, + env: options.env, + vcsCatalog: options.discoveryCatalog ?? options.baseVcsCatalog, + }); + + let extensions: ExtensionLoadResult | undefined; + try { + extensions = await loadStartupExtensionsImpl({ + extensions: configured.extensions, + cwd: options.cwd, + env: options.env, + cliExtensionPaths: configured.input.options.extensionPaths, + projectRoot: configured.projectRoot, + reservedExtensionIds: options.baseVcsCatalog.reservedIds, + notifications: options.notifications, + deferEventBusBinding: true, + }); + + const provisionalAdapters = resolveExtensionVcsAdapters( + extensions.registry, + options.baseVcsCatalog, + ).adapters; + const provisionalCatalog = extendVcsCatalog(options.baseVcsCatalog, provisionalAdapters); + const extensionProjectRoot = findProjectRootCandidateImpl(options.cwd, provisionalCatalog); + + if (provisionalAdapters.length > 0 && extensionProjectRoot !== configured.projectRoot) { + configured = resolveConfiguredCliInputImpl(options.runtimeInput, { + cwd: options.cwd, + env: options.env, + vcsCatalog: provisionalCatalog, + }); + extensions = await loadStartupExtensionsImpl({ + extensions: configured.extensions, + cwd: options.cwd, + env: options.env, + cliExtensionPaths: configured.input.options.extensionPaths, + projectRoot: configured.projectRoot, + reservedExtensionIds: options.baseVcsCatalog.reservedIds, + notifications: extensions.notifications, + previousLoad: extensions, + }); + } else { + bindExtensionEventBus(extensions); + } + + return { configured, extensions }; + } catch (error) { + await retireExtensionLoadResult(extensions); + throw error; + } +} diff --git a/src/app/sessionBootstrap.ts b/src/app/sessionBootstrap.ts index 49c4694cb..947738752 100644 --- a/src/app/sessionBootstrap.ts +++ b/src/app/sessionBootstrap.ts @@ -1,7 +1,11 @@ import type { HunkConfigResolution } from "../core/config"; +import { isVcsReviewInput } from "../core/vcs"; +import type { VcsCatalog } from "../core/vcs/types"; +import { getBundledVcsCatalog } from "./vcsCatalog"; import { collectSessionCustomThemes } from "../core/customThemes"; import { loadAppBootstrap } from "../core/loaders"; -import type { AppBootstrap, CliInput } from "../core/types"; +import type { CliInput } from "../core/types"; +import type { AppBootstrap } from "./types"; import { applyExtensionChangesetTransforms, applyExtensionRegistrations, @@ -19,6 +23,8 @@ export interface SessionBootstrapOptions { /** Reloads can reopen another directory; initial launch relies on the loader's default cwd. */ loadAtCwd?: boolean; loadAppBootstrapImpl?: typeof loadAppBootstrap; + /** Base product adapters composed before user extensions are applied. */ + baseVcsCatalog?: VcsCatalog; } export interface SessionBootstrapResult { @@ -43,33 +49,32 @@ export async function loadConfiguredSessionBootstrap({ initialThemeMode, loadAtCwd = false, loadAppBootstrapImpl = loadAppBootstrap, + baseVcsCatalog = getBundledVcsCatalog(), }: SessionBootstrapOptions): Promise { const sessionThemes = collectSessionCustomThemes( configured.customThemes, extensions?.registry.themes, ); - const applied = applyExtensionRegistrations(extensions); - const sessionVcs = resolveSessionVcsId(configured.input.options.vcs, cwd, applied.vcsAdapters); + const applied = applyExtensionRegistrations(extensions, baseVcsCatalog); + const sessionVcs = resolveSessionVcsId(configured.input.options.vcs, cwd, applied.vcsCatalog); let input = configured.input; if (sessionVcs.vcsId !== input.options.vcs) { input = { ...input, options: { ...input.options, vcs: sessionVcs.vcsId } }; } - const detectedVcsId = resolveDetectedVcsIdWithExtensions( - cwd, - applied.vcsAdapters, - configured.explicitVcsId, - ); + const detectedVcsId = isVcsReviewInput(input) + ? resolveDetectedVcsIdWithExtensions(cwd, applied.vcsCatalog, configured.explicitVcsId) + : undefined; if (detectedVcsId !== undefined && detectedVcsId !== input.options.vcs) { input = { ...input, options: { ...input.options, vcs: detectedVcsId } }; } - const bootstrap = await loadAppBootstrapImpl(input, { + const bootstrap = (await loadAppBootstrapImpl(input, { ...(loadAtCwd ? { cwd } : {}), customThemes: sessionThemes.themes, - vcsAdapters: applied.vcsAdapters, - }); + vcsCatalog: applied.vcsCatalog, + })) as AppBootstrap; bootstrap.changeset = await applyExtensionChangesetTransforms(extensions, bootstrap.changeset); bootstrap.initialThemeMode = initialThemeMode ?? bootstrap.initialThemeMode; bootstrap.extensions = extensions; diff --git a/src/app/sessionSelector.test.ts b/src/app/sessionSelector.test.ts new file mode 100644 index 000000000..85fe32669 --- /dev/null +++ b/src/app/sessionSelector.test.ts @@ -0,0 +1,56 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdirSync, mkdtempSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { getBundledVcsCatalog } from "./vcsCatalog"; +import { resolveSessionSelectorBoundary } from "./sessionSelector"; + +const tempDirs: string[] = []; + +/** Create one portable temporary directory tracked for cleanup. */ +function createTempDir() { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "hunk-session-selector-"))); + tempDirs.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("resolveSessionSelectorBoundary", () => { + test("attaches the outer checkout root for an ordinary subdirectory", () => { + const repo = createTempDir(); + const nested = join(repo, "src", "deep"); + mkdirSync(join(repo, ".git")); + mkdirSync(nested, { recursive: true }); + + expect(resolveSessionSelectorBoundary({ repoRoot: nested }, getBundledVcsCatalog())).toEqual({ + repoRoot: nested, + repoBoundary: repo, + }); + }); + + test("attaches a recognized nested checkout instead of its outer checkout", () => { + const outer = createTempDir(); + const inner = join(outer, "vendor", "nested"); + const source = join(inner, "src"); + mkdirSync(join(outer, ".git")); + mkdirSync(join(inner, ".git"), { recursive: true }); + mkdirSync(source); + + expect(resolveSessionSelectorBoundary({ repoRoot: source }, getBundledVcsCatalog())).toEqual({ + repoRoot: source, + repoBoundary: inner, + }); + }); + + test("leaves selectors without a known project boundary unchanged", () => { + const directory = createTempDir(); + const selector = { repoRoot: directory }; + + expect(resolveSessionSelectorBoundary(selector, getBundledVcsCatalog())).toBe(selector); + }); +}); diff --git a/src/app/sessionSelector.ts b/src/app/sessionSelector.ts new file mode 100644 index 000000000..f590fecf2 --- /dev/null +++ b/src/app/sessionSelector.ts @@ -0,0 +1,16 @@ +import { findProjectRootCandidate } from "../core/projectRoot"; +import type { SessionSelectorInput } from "../core/types"; +import type { VcsCatalog } from "../core/vcs/types"; + +/** Attach the nearest known project boundary to one repo-path session selector. */ +export function resolveSessionSelectorBoundary( + selector: SessionSelectorInput, + catalog: Pick, +): SessionSelectorInput { + if (!selector.repoRoot) { + return selector; + } + + const repoBoundary = findProjectRootCandidate(selector.repoRoot, catalog); + return repoBoundary ? { ...selector, repoBoundary } : selector; +} diff --git a/src/app/startup.test.ts b/src/app/startup.test.ts index d2e4cfeca..bdf35d560 100644 --- a/src/app/startup.test.ts +++ b/src/app/startup.test.ts @@ -336,7 +336,8 @@ describe("startup planning", () => { resolveConfiguredCliInputImpl: (input) => createTestConfigResolution(input, { customThemes }), loadAppBootstrapImpl: async (input, options) => { expect(input).toBe(cliInput); - expect(options).toEqual({ customThemes, vcsAdapters: [] }); + expect(options).toMatchObject({ customThemes }); + expect(options?.vcsCatalog?.defaultAdapterId).toBe("git"); return { ...createBootstrap(input), customThemes, diff --git a/src/app/startup.ts b/src/app/startup.ts index 870b08650..2056eff2a 100644 --- a/src/app/startup.ts +++ b/src/app/startup.ts @@ -1,4 +1,6 @@ +import { resolveConfiguredExtensions } from "./extensionBootstrap"; import { loadConfiguredSessionBootstrap, type SessionBootstrapResult } from "./sessionBootstrap"; +import { getBundledVcsCatalog } from "./vcsCatalog"; import { createExtensionApplyNotices, createUnknownVcsNotice } from "../extensions/apply"; import { loadBundledExtensions } from "../extensions/default/vcs"; import { @@ -17,8 +19,8 @@ import { usesPipedPatchInput, type ControllingTerminal, } from "../core/terminal"; +import type { AppBootstrap } from "./types"; import type { - AppBootstrap, CliInput, MarkupRenderCommandInput, ParsedCliInput, @@ -26,6 +28,7 @@ import type { } from "../core/types"; import { canReloadInput } from "../core/inputReload"; import { parseCli } from "../core/cli"; +import { resolveSessionSelectorBoundary } from "./sessionSelector"; export type StartupPlan = | { @@ -115,6 +118,7 @@ export async function prepareStartupPlan( const stdoutIsTTY = deps.stdoutIsTTY ?? Boolean(process.stdout.isTTY); const stdout = deps.stdout ?? process.stdout; const env = deps.env ?? process.env; + const baseVcsCatalog = getBundledVcsCatalog(); let parsedCliInput = await parseCliImpl(argv); let controllingTerminal: ControllingTerminal | null = null; @@ -133,9 +137,16 @@ export async function prepareStartupPlan( } if (parsedCliInput.kind === "session") { + const sessionInput = + "selector" in parsedCliInput + ? { + ...parsedCliInput, + selector: resolveSessionSelectorBoundary(parsedCliInput.selector, baseVcsCatalog), + } + : parsedCliInput; return { kind: "session-command", - input: parsedCliInput, + input: sessionInput, }; } @@ -168,6 +179,9 @@ export async function prepareStartupPlan( }; const configuredStatic = resolveConfiguredCliInputImpl( resolveRuntimeCliInputImpl(staticPatchInput), + { + vcsCatalog: baseVcsCatalog, + }, ); const staticPlan = { kind: "static-diff-pager" as const, @@ -232,7 +246,12 @@ export async function prepareStartupPlan( } const runtimeCliInput = resolveRuntimeCliInputImpl(parsedCliInput); - const configured = resolveConfiguredCliInputImpl(runtimeCliInput); + const startupCwd = process.cwd(); + let configured = resolveConfiguredCliInputImpl(runtimeCliInput, { + cwd: startupCwd, + env, + vcsCatalog: baseVcsCatalog, + }); // Reassigned once below if an extension VCS backend claims this checkout. let cliInput = configured.input; @@ -263,15 +282,22 @@ export async function prepareStartupPlan( } // Extensions load before the changeset so later stages can hand their VCS adapters and - // changeset transforms to the loading pipeline. Failures never reach here: the host - // isolates them into issues that become startup notices below. - const startupCwd = process.cwd(); - const extensionResult = await loadStartupExtensionsImpl({ - extensions: configured.extensions, - cwd: startupCwd, - env, - cliExtensionPaths: cliInput.options.extensionPaths, - }); + // changeset transforms to the loading pipeline. External adapters may settle a root the + // bundled catalog could not; the shared resolver then appends newly discovered repo + // candidates without executing the provisional factory prefix twice. + const resolvedExtensions = await resolveConfiguredExtensions( + { + runtimeInput: runtimeCliInput, + configured, + cwd: startupCwd, + env, + baseVcsCatalog, + }, + { resolveConfiguredCliInputImpl, loadStartupExtensionsImpl }, + ); + configured = resolvedExtensions.configured; + cliInput = configured.input; + const extensionResult = resolvedExtensions.extensions; let preparedSession: SessionBootstrapResult; try { @@ -281,6 +307,7 @@ export async function prepareStartupPlan( extensions: extensionResult, initialThemeMode, loadAppBootstrapImpl, + baseVcsCatalog, }); } catch (error) { controllingTerminal?.close(); diff --git a/src/app/startup.vcsExtensions.test.ts b/src/app/startup.vcsExtensions.test.ts new file mode 100644 index 000000000..18bab27fa --- /dev/null +++ b/src/app/startup.vcsExtensions.test.ts @@ -0,0 +1,101 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { prepareStartupPlan } from "./startup"; +import { resolveConfiguredCliInput } from "../core/config"; +import type { CliInput, ParsedCliInput } from "../core/types"; + +const tempDirs: string[] = []; +const initialCwd = process.cwd(); + +afterEach(() => { + process.chdir(initialCwd); + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +function createTempDir(prefix: string) { + const dir = realpathSync(mkdtempSync(join(tmpdir(), prefix))); + tempDirs.push(dir); + return dir; +} + +describe("external VCS startup bootstrap", () => { + test("a CLI extension establishes a pure external-VCS project root before loading", async () => { + const repo = createTempDir("hunk-external-vcs-startup-"); + const nested = join(repo, "src", "nested"); + mkdirSync(join(repo, ".custom")); + mkdirSync(nested, { recursive: true }); + const extensionPath = join(repo, "custom-vcs.ts"); + const factoryLogPath = join(repo, "factory.log"); + writeFileSync( + extensionPath, + ` + import { dirname, join } from "node:path"; + import { appendFileSync, existsSync } from "node:fs"; + export default function (hunk) { + appendFileSync(${JSON.stringify(factoryLogPath)}, "factory\\n"); + hunk.registerVcsAdapter({ + id: "custom", + name: "Custom VCS", + detect(cwd) { + let current = cwd; + for (;;) { + if (existsSync(join(current, ".custom"))) { + return { id: "custom", repoRoot: current }; + } + const parent = dirname(current); + if (parent === current) return null; + current = parent; + } + }, + operations: { + "working-tree-diff": { + async load() { + return { + repoRoot: ${JSON.stringify(repo)}, + sourceLabel: ${JSON.stringify(repo)}, + title: "Custom working copy", + patchText: "", + }; + }, + }, + }, + }); + } + `, + ); + process.chdir(nested); + const input: CliInput = { + kind: "vcs", + staged: false, + options: { extensionPaths: [extensionPath] }, + }; + + const resolvedProjectRoots: Array = []; + const plan = await prepareStartupPlan(["bun", "hunk", "diff"], { + parseCliImpl: async () => input as ParsedCliInput, + resolveRuntimeCliInputImpl: (value) => value, + resolveConfiguredCliInputImpl: (value, options) => { + const result = resolveConfiguredCliInput(value, options); + resolvedProjectRoots.push(result.projectRoot); + return result; + }, + stdinIsTTY: true, + stdoutIsTTY: false, + env: { HOME: createTempDir("hunk-external-vcs-home-") }, + }); + + expect(plan.kind).toBe("app"); + if (plan.kind !== "app") { + return; + } + expect(resolvedProjectRoots).toEqual([undefined, repo]); + expect(plan.bootstrap.reloadContext.repoRoot).toBe(repo); + expect(plan.bootstrap.input.options.vcs).toBe("custom"); + expect(plan.bootstrap.changeset.title).toBe("Custom working copy"); + expect(readFileSync(factoryLogPath, "utf8")).toBe("factory\n"); + }); +}); diff --git a/src/app/types.ts b/src/app/types.ts new file mode 100644 index 000000000..da9e30cf8 --- /dev/null +++ b/src/app/types.ts @@ -0,0 +1,5 @@ +import type { AppBootstrap as CoreAppBootstrap } from "../core/types"; +import type { ExtensionLoadResult } from "../extensions/types"; + +/** Interactive app bootstrap specialized with the extension host's session state. */ +export type AppBootstrap = CoreAppBootstrap; diff --git a/src/app/vcsCatalog.test.ts b/src/app/vcsCatalog.test.ts new file mode 100644 index 000000000..e2e865d0e --- /dev/null +++ b/src/app/vcsCatalog.test.ts @@ -0,0 +1,12 @@ +import { describe, expect, test } from "bun:test"; +import { getBundledVcsCatalog } from "./vcsCatalog"; + +describe("app VCS catalog composition", () => { + test("owns bundled ordering, fallback, and reserved ids at the app boundary", () => { + const catalog = getBundledVcsCatalog(); + + expect(catalog.defaultAdapterId).toBe("git"); + expect(catalog.adapters.map((adapter) => adapter.id)).toEqual(["jj", "sl", "git"]); + expect(catalog.reservedIds).toEqual(new Set(["jj", "sl", "git"])); + }); +}); diff --git a/src/app/vcsCatalog.ts b/src/app/vcsCatalog.ts new file mode 100644 index 000000000..05324bb20 --- /dev/null +++ b/src/app/vcsCatalog.ts @@ -0,0 +1,14 @@ +import { createVcsCatalog } from "../core/vcs"; +import type { VcsCatalog } from "../core/vcs/types"; +import { getBundledVcsAdapters } from "../extensions/default/vcs"; + +/** Product fallback provider selected when config names no backend. */ +const DEFAULT_VCS_ID = "git"; + +let bundledCatalog: VcsCatalog | undefined; + +/** Compose Hunk's statically bundled adapters at the app boundary. */ +export function getBundledVcsCatalog(): VcsCatalog { + bundledCatalog ??= createVcsCatalog(getBundledVcsAdapters(), DEFAULT_VCS_ID); + return bundledCatalog; +} diff --git a/src/core/cli.test.ts b/src/core/cli.test.ts index 99240dd73..8c786ec12 100644 --- a/src/core/cli.test.ts +++ b/src/core/cli.test.ts @@ -369,7 +369,7 @@ describe("parseCli", () => { }); }); - test("resolves --repo from a subdirectory to the containing repo root", async () => { + test("keeps --repo provider-neutral while canonicalizing the selected subdirectory", async () => { const repoRoot = realpathSync.native(createTempDir("hunk-cli-repo-")); mkdirSync(join(repoRoot, ".git")); const subdir = join(repoRoot, "packages", "app"); @@ -380,7 +380,7 @@ describe("parseCli", () => { expect(parsed).toMatchObject({ kind: "session", action: "get", - selector: { repoRoot }, + selector: { repoRoot: realpathSync.native(subdir) }, }); }); @@ -544,7 +544,7 @@ describe("parseCli", () => { }); }); - test("resolves session reload --repo from a subdirectory to the containing repo root", async () => { + test("keeps session reload --repo provider-neutral for subdirectories", async () => { const repoRoot = realpathSync.native(createTempDir("hunk-cli-reload-")); mkdirSync(join(repoRoot, ".git")); const subdir = join(repoRoot, "packages", "app"); @@ -564,7 +564,7 @@ describe("parseCli", () => { expect(parsed).toMatchObject({ kind: "session", action: "reload", - selector: { repoRoot }, + selector: { repoRoot: realpathSync.native(subdir) }, }); }); diff --git a/src/core/cli.ts b/src/core/cli.ts index 24347c8ef..098f6c5d3 100644 --- a/src/core/cli.ts +++ b/src/core/cli.ts @@ -36,7 +36,6 @@ import { constraintViolationMessage, RELOAD_SEPARATOR_MESSAGE, } from "../session/agent/errors"; -import { detectVcs } from "./vcs"; import { DEFAULT_TAB_WIDTH, parseTabWidth } from "./tabWidth"; import { resolveCliVersion } from "./version"; @@ -555,21 +554,10 @@ function parseSessionCommentApplyPayload(raw: string): SessionCommentApplyItemIn }); } -/** - * Resolve a `--repo ` selector to the containing VCS toplevel. - * - * Sessions register under their repo root, so the selector must walk up from the - * given path to that root; otherwise a query run from a subdirectory would never - * match. The detected root is canonicalized through symlinks to mirror the - * session's registered repoRoot (git's `--show-toplevel` is realpath-canonical), - * so matching also holds under a symlinked ancestor like macOS `/tmp`. Falls back - * to the resolved path when it is not inside a known checkout. - */ +/** Canonicalize a `--repo` path without assuming which VCS owns it. */ function resolveRepoSelectorRoot(repoPath: string): string { const resolved = resolve(repoPath); - const repoRoot = detectVcs(resolved)?.repoRoot; - // The detected root always exists on disk, so realpath cannot throw here. - return repoRoot ? realpathSync.native(repoRoot) : resolved; + return existsSync(resolved) ? realpathSync.native(resolved) : resolved; } /** Normalize one explicit session selector from either session id or repo root. */ diff --git a/src/core/config.test.ts b/src/core/config.test.ts index 2dc1f6a2d..531b951ed 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; import type { CliInput } from "./types"; import { diffPersistedViewPreferences, @@ -945,24 +946,18 @@ describe("config resolution", () => { staged: false, options: {}, } satisfies CliInput; - - expect( - resolveConfiguredCliInput(input, { cwd: jjRepo, env: { HOME: home } }).input.options.vcs, - ).toBe("jj"); - expect( - resolveConfiguredCliInput(input, { cwd: colocatedRepo, env: { HOME: home } }).input.options - .vcs, - ).toBe("jj"); - expect( - resolveConfiguredCliInput(input, { cwd: gitRepo, env: { HOME: home } }).input.options.vcs, - ).toBe("git"); - expect( - resolveConfiguredCliInput(input, { cwd: gitRepoInsideParentJj, env: { HOME: home } }).input - .options.vcs, - ).toBe("git"); - expect( - resolveConfiguredCliInput(input, { cwd: plainDir, env: { HOME: home } }).input.options.vcs, - ).toBe("git"); + const resolveIn = (cwd: string) => + resolveConfiguredCliInput(input, { + cwd, + env: { HOME: home }, + vcsCatalog: getBundledVcsCatalog(), + }).input.options.vcs; + + expect(resolveIn(jjRepo)).toBe("jj"); + expect(resolveIn(colocatedRepo)).toBe("jj"); + expect(resolveIn(gitRepo)).toBe("git"); + expect(resolveIn(gitRepoInsideParentJj)).toBe("git"); + expect(resolveIn(plainDir)).toBe("git"); }); test("explicit config overrides auto-detected jj mode", () => { diff --git a/src/core/config.ts b/src/core/config.ts index 2f09f31ee..9c09d8723 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -16,7 +16,9 @@ import { LEGACY_CUSTOM_SYNTAX_COLOR_KEYS, resolveSyntaxScopeOverrides } from "./ import { resolveGlobalConfigPath } from "./paths"; import { LEGACY_CUSTOM_SYNTAX_NOTICES, type StartupNotice } from "./startupNotice"; import { DEFAULT_TAB_WIDTH, validateTabWidth } from "./tabWidth"; -import { detectVcs, findVcsRepoRootCandidate, getDefaultVcsAdapter } from "./vcs"; +import { findProjectRootCandidate } from "./projectRoot"; +import { createVcsCatalog, detectVcs } from "./vcs"; +import type { VcsCatalog } from "./vcs/types"; import type { CliInput, CommonOptions, @@ -62,11 +64,16 @@ const PERSISTED_VIEW_PREFERENCE_KEYS: Array<{ { configKey: "cursor_line", value: (preferences) => preferences.cursorLine }, ]; -interface ConfigResolutionOptions { +export interface ConfigResolutionOptions { cwd?: string; env?: NodeJS.ProcessEnv; + /** Base catalog available before user extensions load. */ + vcsCatalog?: VcsCatalog; } +const CONFIG_FALLBACK_VCS_ID = "git"; +const EMPTY_CONFIG_VCS_CATALOG = createVcsCatalog([], CONFIG_FALLBACK_VCS_ID, []); + export interface HunkConfigResolution { input: CliInput; /** Config-defined custom themes in declaration order, user layer before repo layer. */ @@ -91,6 +98,8 @@ export interface HunkConfigResolution { explicitVcsId?: string; startupNotices?: readonly StartupNotice[]; globalConfigPath?: string; + /** Project root selected from `.hunk` or the base VCS catalog. */ + projectRoot?: string; repoConfigPath?: string; viewPreferencesConfigPath?: string; } @@ -856,8 +865,8 @@ function readConfigPreferences(source: Record): CommonOptions { } /** Build concrete preference defaults from the same catalog rendered by generated docs. */ -function buildDefaultConfigPreferences(cwd: string): CommonOptions { - const defaults: CommonOptions = { vcs: detectRepoVcsMode(cwd) }; +function buildDefaultConfigPreferences(cwd: string, vcsCatalog: VcsCatalog): CommonOptions { + const defaults: CommonOptions = { vcs: detectRepoVcsMode(cwd, vcsCatalog) }; const mutable = defaults as Record; for (const option of CONFIG_REFERENCE_OPTIONS) { if (option.runtimeDefault !== undefined) { @@ -914,8 +923,8 @@ function resolveConfigLayer(source: Record, input: CliInput): C } /** Choose the VCS backend that best matches the discovered checkout. */ -function detectRepoVcsMode(cwd: string): VcsMode { - return detectVcs(cwd)?.id ?? getDefaultVcsAdapter().id; +function detectRepoVcsMode(cwd: string, vcsCatalog: VcsCatalog): VcsMode { + return detectVcs(cwd, vcsCatalog)?.id ?? vcsCatalog.defaultAdapterId; } /** Parse one TOML config file into a plain object. */ @@ -1031,9 +1040,13 @@ export function saveViewPreferencesPromptPreference( /** Resolve CLI input against global and repo-local config files. */ export function resolveConfiguredCliInput( input: CliInput, - { cwd = process.cwd(), env = process.env }: ConfigResolutionOptions = {}, + { + cwd = process.cwd(), + env = process.env, + vcsCatalog = EMPTY_CONFIG_VCS_CATALOG, + }: ConfigResolutionOptions = {}, ): HunkConfigResolution { - const repoRoot = findVcsRepoRootCandidate(cwd); + const repoRoot = findProjectRootCandidate(cwd, vcsCatalog); const repoConfigPath = repoRoot ? join(repoRoot, ".hunk", "config.toml") : undefined; const userConfigPath = resolveGlobalConfigPath(env); let resolvedCustomThemes: NamedCustomThemeConfig[] = []; @@ -1045,7 +1058,7 @@ export function resolveConfiguredCliInput( let keybindingsLayer: KeybindingsLayer = { bindings: {}, unusableIds: [] }; let resolvedOptions: CommonOptions = { - ...buildDefaultConfigPreferences(cwd), + ...buildDefaultConfigPreferences(cwd, vcsCatalog), agentContext: input.options.agentContext, pager: input.options.pager ?? false, experimental: false, @@ -1094,7 +1107,7 @@ export function resolveConfiguredCliInput( experimental: input.options.experimental ?? false, excludeUntracked: resolvedOptions.excludeUntracked ?? false, theme: resolvedOptions.theme, - vcs: resolvedOptions.vcs ?? getDefaultVcsAdapter().id, + vcs: resolvedOptions.vcs ?? vcsCatalog.defaultAdapterId, mode: resolvedOptions.mode ?? DEFAULT_VIEW_PREFERENCES.mode, lineNumbers: resolvedOptions.lineNumbers ?? DEFAULT_VIEW_PREFERENCES.showLineNumbers, tabWidth: resolvedOptions.tabWidth ?? DEFAULT_TAB_WIDTH, @@ -1153,6 +1166,7 @@ export function resolveConfiguredCliInput( ...keybindingNotices, ]), globalConfigPath: userConfigPath, + projectRoot: repoRoot, repoConfigPath, // Persist in the repo config only when the repo already has one; otherwise keep personal view // choices user-scoped so Hunk does not create project policy files from an interactive prompt. diff --git a/src/core/customThemes.test.ts b/src/core/customThemes.test.ts index a3f7573db..a903f1eb8 100644 --- a/src/core/customThemes.test.ts +++ b/src/core/customThemes.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import type { RegisteredTheme } from "../extensions/types"; +import type { RegisteredCustomTheme } from "./customThemes"; import { collectSessionCustomThemes, describeCustomThemeIdIssue } from "./customThemes"; import type { NamedCustomThemeConfig } from "./types"; @@ -7,7 +7,7 @@ import type { NamedCustomThemeConfig } from "./types"; function createTestRegisteredTheme( extensionId: string, theme: NamedCustomThemeConfig, -): RegisteredTheme { +): RegisteredCustomTheme { return { extensionId, theme }; } diff --git a/src/core/customThemes.ts b/src/core/customThemes.ts index b45849612..3d975e572 100644 --- a/src/core/customThemes.ts +++ b/src/core/customThemes.ts @@ -1,9 +1,14 @@ -import type { RegisteredTheme } from "../extensions/types"; import { BUNDLED_SHIKI_THEME_IDS, resolveBundledShikiThemeId } from "./themeCatalog"; import { LEGACY_CUSTOM_SYNTAX_COLOR_KEYS } from "./legacySyntaxScopes"; import type { StartupNotice } from "./startupNotice"; import type { NamedCustomThemeConfig } from "./types"; +/** Provider-neutral shape accepted from any custom-theme registration source. */ +export interface RegisteredCustomTheme { + extensionId: string; + theme: NamedCustomThemeConfig; +} + /** Id of the theme defined by the original single-slot `[custom_theme]` config table. */ export const LEGACY_CUSTOM_THEME_ID = "custom"; @@ -315,7 +320,7 @@ export interface SessionCustomThemes { */ export function collectSessionCustomThemes( configThemes: readonly NamedCustomThemeConfig[] = [], - extensionThemes: readonly RegisteredTheme[] = [], + extensionThemes: readonly RegisteredCustomTheme[] = [], ): SessionCustomThemes { const themes = [...configThemes]; const notices: StartupNotice[] = []; diff --git a/src/core/fileSource.test.ts b/src/core/fileSource.test.ts index 9d62a94e3..00424460d 100644 --- a/src/core/fileSource.test.ts +++ b/src/core/fileSource.test.ts @@ -3,7 +3,6 @@ import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { createFileSourceFetcher, SourceTextTooLargeError } from "./fileSource"; -import { readGitFileSource } from "./vcs/gitSource"; const tempDirs: string[] = []; @@ -13,48 +12,6 @@ function createTempDir(prefix: string) { return dir; } -function git(cwd: string, ...cmd: string[]) { - const proc = Bun.spawnSync(["git", ...cmd], { - cwd, - stdout: "pipe", - stderr: "pipe", - stdin: "ignore", - }); - - if (proc.exitCode !== 0) { - const stderr = Buffer.from(proc.stderr).toString("utf8"); - throw new Error(stderr.trim() || `git ${cmd.join(" ")} failed`); - } - - return Buffer.from(proc.stdout).toString("utf8"); -} - -function createTempRepo(prefix: string) { - const dir = createTempDir(prefix); - git(dir, "init"); - git(dir, "config", "user.name", "Test User"); - git(dir, "config", "user.email", "test@example.com"); - git(dir, "config", "commit.gpgSign", "false"); - return dir; -} - -/** Capture console.error calls while exercising diagnostic paths. */ -async function captureConsoleErrors(fn: () => Promise) { - const originalConsoleError = console.error; - const loggedErrors: unknown[][] = []; - console.error = (...args: unknown[]) => { - loggedErrors.push(args); - }; - - try { - await fn(); - } finally { - console.error = originalConsoleError; - } - - return loggedErrors; -} - afterEach(() => { while (tempDirs.length > 0) { const dir = tempDirs.pop(); @@ -117,179 +74,6 @@ describe("createFileSourceFetcher", () => { await expect(fetcher.getFullText("old")).rejects.toBeInstanceOf(SourceTextTooLargeError); }); - test("reads git blob contents for both sides via `git show`", async () => { - const repoRoot = createTempRepo("hunk-source-git-"); - const filePath = "note.txt"; - - writeFileSync(join(repoRoot, filePath), "first revision\n"); - git(repoRoot, "add", "."); - git(repoRoot, "commit", "-m", "first"); - writeFileSync(join(repoRoot, filePath), "second revision\n"); - git(repoRoot, "add", "."); - git(repoRoot, "commit", "-m", "second"); - - expect( - await readGitFileSource({ kind: "git-blob", repoRoot, ref: "HEAD~1", path: filePath }), - ).toBe("first revision\n"); - expect( - await readGitFileSource({ kind: "git-blob", repoRoot, ref: "HEAD", path: filePath }), - ).toBe("second revision\n"); - }); - - test("reads git index contents through an explicit index spec", async () => { - const repoRoot = createTempRepo("hunk-source-git-index-"); - const filePath = "note.txt"; - - writeFileSync(join(repoRoot, filePath), "committed\n"); - git(repoRoot, "add", "."); - git(repoRoot, "commit", "-m", "first"); - writeFileSync(join(repoRoot, filePath), "staged\n"); - git(repoRoot, "add", filePath); - writeFileSync(join(repoRoot, filePath), "working tree\n"); - - expect(await readGitFileSource({ kind: "git-index", repoRoot, path: filePath })).toBe( - "staged\n", - ); - expect(await readGitFileSource({ kind: "fs", absolutePath: join(repoRoot, filePath) })).toBe( - "working tree\n", - ); - }); - - test("rejects git blob and index source reads that exceed the configured byte cap", async () => { - const repoRoot = createTempRepo("hunk-source-git-large-"); - const filePath = "note.txt"; - - writeFileSync(join(repoRoot, filePath), "committed source\n"); - git(repoRoot, "add", filePath); - git(repoRoot, "commit", "-m", "first"); - writeFileSync(join(repoRoot, filePath), "staged source\n"); - git(repoRoot, "add", filePath); - - await expect( - readGitFileSource( - { kind: "git-blob", repoRoot, ref: "HEAD", path: filePath }, - { maxSourceBytes: 5 }, - ), - ).rejects.toBeInstanceOf(SourceTextTooLargeError); - await expect( - readGitFileSource({ kind: "git-index", repoRoot, path: filePath }, { maxSourceBytes: 5 }), - ).rejects.toBeInstanceOf(SourceTextTooLargeError); - }); - - test("treats oversized git stderr as a generic source failure", async () => { - const originalSpawn = Bun.spawn; - const mutableBun = Bun as unknown as { spawn: typeof Bun.spawn }; - - mutableBun.spawn = (() => - originalSpawn( - [ - process.execPath, - "--eval", - "process.stdout.write('small source\\n'); process.stderr.write('x'.repeat(70000));", - ], - { - stdin: "ignore", - stdout: "pipe", - stderr: "pipe", - }, - )) as typeof Bun.spawn; - - try { - const loggedErrors = await captureConsoleErrors(async () => { - await expect( - readGitFileSource({ - kind: "git-blob", - repoRoot: process.cwd(), - ref: "HEAD", - path: "note.txt", - }), - ).resolves.toBeNull(); - }); - - expect(String(loggedErrors[0]?.[0])).toContain("failed to collect Git source"); - expect(String(loggedErrors[0]?.[1])).toContain("diagnostics exceeded"); - } finally { - mutableBun.spawn = originalSpawn; - } - }); - - test("passes custom git executable through async git source reads", async () => { - const originalSpawn = Bun.spawn; - const mutableBun = Bun as unknown as { spawn: typeof Bun.spawn }; - const spawnCalls: string[][] = []; - - mutableBun.spawn = ((cmds: string[]) => { - spawnCalls.push(cmds); - return originalSpawn( - [ - process.execPath, - "--eval", - `process.stdout.write(${JSON.stringify(`read:${cmds[2]}\n`)})`, - ], - { - stdin: "ignore", - stdout: "pipe", - stderr: "pipe", - }, - ); - }) as typeof Bun.spawn; - - try { - expect( - await readGitFileSource( - { kind: "git-blob", repoRoot: process.cwd(), ref: "HEAD", path: "note.txt" }, - { gitExecutable: "custom-git" }, - ), - ).toBe("read:HEAD:note.txt\n"); - expect( - await readGitFileSource( - { kind: "git-index", repoRoot: process.cwd(), path: "note.txt" }, - { gitExecutable: "custom-git" }, - ), - ).toBe("read::note.txt\n"); - } finally { - mutableBun.spawn = originalSpawn; - } - - expect(spawnCalls).toEqual([ - ["custom-git", "show", "HEAD:note.txt"], - ["custom-git", "show", ":note.txt"], - ]); - }); - - test("returns null when a git blob cannot be resolved", async () => { - const repoRoot = createTempRepo("hunk-source-git-missing-"); - writeFileSync(join(repoRoot, "tracked.txt"), "x\n"); - git(repoRoot, "add", "."); - git(repoRoot, "commit", "-m", "first"); - - const loggedErrors = await captureConsoleErrors(async () => { - expect( - await readGitFileSource({ - kind: "git-blob", - repoRoot, - ref: "HEAD", - path: "missing-from-history.txt", - }), - ).toBeNull(); - }); - expect(loggedErrors).toHaveLength(0); - }); - - test("logs unexpected git source failures with object context", async () => { - const repoRoot = createTempDir("hunk-source-git-not-repo-"); - - const loggedErrors = await captureConsoleErrors(async () => { - expect( - await readGitFileSource({ kind: "git-blob", repoRoot, ref: "HEAD", path: "note.txt" }), - ).toBeNull(); - }); - - expect(loggedErrors).toHaveLength(1); - expect(String(loggedErrors[0]?.[0])).toContain("HEAD:note.txt"); - expect(String(loggedErrors[0]?.[0])).toContain(repoRoot); - }); - test("caches resolved text per side", async () => { const dir = createTempDir("hunk-source-cache-"); const target = join(dir, "value.txt"); diff --git a/src/core/fileSource.ts b/src/core/fileSource.ts index 8fbf7505e..88eebe674 100644 --- a/src/core/fileSource.ts +++ b/src/core/fileSource.ts @@ -1,3 +1,11 @@ +import { + DEFAULT_SOURCE_TEXT_MAX_BYTES, + readFileTextWithLimit, + readStreamTextWithLimit as readLimitedStreamText, +} from "../lib/sourceText"; + +export { DEFAULT_SOURCE_TEXT_MAX_BYTES } from "../lib/sourceText"; + /** * Generic full-file source fetcher primitives used by input loaders and VCS adapters. * @@ -23,8 +31,6 @@ export interface FileSourceFetcher { getFullText(side: FileSourceSide): Promise; } -export const DEFAULT_SOURCE_TEXT_MAX_BYTES = 1_000_000; - /** Raised when expanded-context source would require reading an unsafe amount of text. */ export class SourceTextTooLargeError extends Error { constructor(readonly maxBytes: number) { @@ -42,89 +48,24 @@ interface ResolvedSpecs { new: FileSourceSpec; } -/** Return the first useful diagnostic line from a failed source read. */ -function firstDiagnosticLine(text: string) { - return text - .split("\n") - .map((line) => line.trim()) - .find(Boolean); -} - -/** Keep source-load diagnostics terse enough to be useful in logs. */ -export function logSourceDiagnostic(message: string, detail?: unknown) { - if (detail instanceof Error) { - console.error(`hunk: ${message}: ${detail.message}`, detail); - return; - } - - const detailText = typeof detail === "string" ? firstDiagnosticLine(detail) : undefined; - console.error(detailText ? `hunk: ${message}: ${detailText}` : `hunk: ${message}`); -} - async function readFsSpec( spec: Extract, maxSourceBytes: number, ): Promise { - try { - const file = Bun.file(spec.absolutePath); - if (!(await file.exists())) { - return null; - } - - if (file.size > maxSourceBytes) { - throw new SourceTextTooLargeError(maxSourceBytes); - } - - return await file.text(); - } catch (error) { - if (error instanceof SourceTextTooLargeError) { - throw error; - } - - logSourceDiagnostic(`failed to read source file ${spec.absolutePath}`, error); - return null; + const result = await readFileTextWithLimit(spec.absolutePath, maxSourceBytes); + if (typeof result === "object" && result !== null) { + throw new SourceTextTooLargeError(result.maxBytes); } + return result; } -export async function readStreamTextWithLimit( +export function readStreamTextWithLimit( stream: ReadableStream | null, maxBytes: number, onTooLarge?: () => void, - createLimitError: (maxBytes: number) => Error = (maxBytes) => - new SourceTextTooLargeError(maxBytes), + createLimitError: (maxBytes: number) => Error = (limit) => new SourceTextTooLargeError(limit), ) { - if (!stream) { - return ""; - } - - const reader = stream.getReader(); - const chunks: Uint8Array[] = []; - let totalBytes = 0; - - for (;;) { - const { done, value } = await reader.read(); - if (done) { - break; - } - - totalBytes += value.byteLength; - if (totalBytes > maxBytes) { - onTooLarge?.(); - await reader.cancel().catch(() => undefined); - throw createLimitError(maxBytes); - } - - chunks.push(value); - } - - const combined = new Uint8Array(totalBytes); - let offset = 0; - for (const chunk of chunks) { - combined.set(chunk, offset); - offset += chunk.byteLength; - } - - return new TextDecoder().decode(combined); + return readLimitedStreamText(stream, maxBytes, onTooLarge, createLimitError); } /** Read the text one filesystem-backed source spec names, or null when there is none. */ diff --git a/src/core/loaders.test.ts b/src/core/loaders.test.ts index af27c0132..a58615b99 100644 --- a/src/core/loaders.test.ts +++ b/src/core/loaders.test.ts @@ -3,13 +3,22 @@ import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSyn import { platform, tmpdir } from "node:os"; import { join } from "node:path"; import { SourceTextTooLargeError } from "./fileSource"; -import { loadAppBootstrap } from "./loaders"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; +import { createGitVcsAdapter } from "../extensions/default/vcs/git"; +import { toInternalVcsAdapter } from "../extensions/runExtension"; +import { createVcsCatalog } from "./vcs"; +import { loadAppBootstrap as loadCoreAppBootstrap, type LoadAppBootstrapOptions } from "./loaders"; import type { CliInput } from "./types"; import type { VcsAdapter } from "./vcs/types"; import { computeWatchSignature } from "./watch"; const tempDirs: string[] = []; +/** Load through the same bundled catalog the app composes in production. */ +function loadAppBootstrap(input: CliInput, options: LoadAppBootstrapOptions = {}) { + return loadCoreAppBootstrap(input, { vcsCatalog: getBundledVcsCatalog(), ...options }); +} + // Jujutsu subprocess setup can exceed Bun's default 5s test timeout on Windows CI. const JjLoaderIntegrationTestTimeoutMs = 20_000; // Sapling subprocess setup can exceed Bun's default 5s test timeout on slower machines. @@ -200,14 +209,14 @@ describe("loadAppBootstrap", () => { const bootstrap = await loadAppBootstrap( { kind: "vcs", staged: false, options: { vcs: "demo" } }, - { cwd: dir, vcsAdapters: [adapter] }, + { cwd: dir, vcsCatalog: createVcsCatalog([adapter], "demo", []) }, ); expect(bootstrap.changeset.files.map((file) => file.path)).toEqual(["note.txt"]); expect(bootstrap.changeset.files[0]?.isUntracked).toBe(true); expect(bootstrap.changeset.files[0]?.patch).toContain("+hello"); // Watch planning has to resolve the same extension adapter the load used. - expect(bootstrap.reloadContext.vcsAdapters).toEqual([adapter]); + expect(bootstrap.reloadContext.vcsCatalog?.adapters).toEqual([adapter]); }); test("captures a watched signature before content loading", async () => { @@ -1809,7 +1818,13 @@ describe("loadAppBootstrap source fetcher attachment", () => { staged: false, options: { mode: "auto" }, }, - { cwd: dir, gitExecutable }, + { + cwd: dir, + vcsCatalog: createVcsCatalog( + [toInternalVcsAdapter(createGitVcsAdapter({ gitExecutable }))], + "git", + ), + }, ); const file = bootstrap.changeset.files[0]; diff --git a/src/core/loaders.ts b/src/core/loaders.ts index e630939bc..cb695b540 100644 --- a/src/core/loaders.ts +++ b/src/core/loaders.ts @@ -13,8 +13,13 @@ import { createFileSourceFetcher, type FileSourceSpec } from "./fileSource"; import { splitPatchIntoFileChunks, findPatchChunk } from "./patch/chunks"; import { normalizePatch, stripTerminalControl } from "./patch/normalize"; import { DEFAULT_TAB_WIDTH } from "./tabWidth"; -import { getConfiguredVcsAdapter, loadVcsReview, operationFromInput } from "./vcs"; -import type { VcsAdapter } from "./vcs/types"; +import { + getConfiguredVcsAdapter, + isVcsReviewInput, + loadVcsReview, + operationFromInput, +} from "./vcs"; +import type { VcsCatalog } from "./vcs/types"; import { buildFilesystemUntrackedDiffFile } from "./vcs/untracked"; import { computeWatchSignature } from "./watch"; import type { @@ -34,13 +39,12 @@ import type { VcsStashShowCommandInput, } from "./types"; -interface LoadAppBootstrapOptions { +export interface LoadAppBootstrapOptions { cwd?: string; /** Selectable custom themes for this session, already merged into menu order. */ customThemes?: readonly NamedCustomThemeConfig[]; - /** Extension-contributed VCS backends this session may load reviews through. */ - vcsAdapters?: readonly VcsAdapter[]; - gitExecutable?: string; + /** Complete adapter catalog composed by the app for this session. */ + vcsCatalog?: VcsCatalog; } /** Return the final path segment for display-oriented labels. */ @@ -385,13 +389,12 @@ async function loadFileDiffChangeset( async function loadVcsChangeset( input: VcsDiffCommandInput | VcsShowCommandInput | VcsStashShowCommandInput, agentContext: AgentContext | null, - cwd = process.cwd(), - gitExecutable = "git", - extensionVcsAdapters: readonly VcsAdapter[] = [], + cwd: string, + vcsCatalog: VcsCatalog, ) { - const adapter = getConfiguredVcsAdapter(input.options.vcs, extensionVcsAdapters); + const adapter = getConfiguredVcsAdapter(input.options.vcs, vcsCatalog); const operation = operationFromInput(input); - const result = await loadVcsReview(adapter, operation, { cwd, gitExecutable }); + const result = await loadVcsReview(adapter, operation, { cwd }, vcsCatalog); const parsedChangeset = normalizePatchChangeset( result.patchText, result.title, @@ -451,18 +454,15 @@ async function loadPatchChangeset( /** Resolve CLI input into the fully loaded app bootstrap state. */ export async function loadAppBootstrap( input: CliInput, - { - cwd = process.cwd(), - customThemes, - vcsAdapters, - gitExecutable = "git", - }: LoadAppBootstrapOptions = {}, + { cwd = process.cwd(), customThemes, vcsCatalog }: LoadAppBootstrapOptions = {}, ): Promise { // Capture before loading content so watch mode can detect mutations that race initial loading. let initialWatchSignature: string | undefined; if (input.options.watch) { try { - initialWatchSignature = computeWatchSignature(input, { cwd, gitExecutable, vcsAdapters }); + if (vcsCatalog || !isVcsReviewInput(input)) { + initialWatchSignature = computeWatchSignature(input, { cwd, vcsCatalog }); + } } catch { // A transient signature failure must not prevent an otherwise valid initial review. } @@ -478,7 +478,10 @@ export async function loadAppBootstrap( case "show": case "stash-show": { - const result = await loadVcsChangeset(input, agentContext, cwd, gitExecutable, vcsAdapters); + if (!vcsCatalog) { + throw new Error("VCS-backed reviews require a composed VCS catalog."); + } + const result = await loadVcsChangeset(input, agentContext, cwd, vcsCatalog); changeset = result.changeset; repoRoot = result.repoRoot; } @@ -501,7 +504,7 @@ export async function loadAppBootstrap( return { input, - reloadContext: { cwd, repoRoot, initialWatchSignature, vcsAdapters }, + reloadContext: { cwd, repoRoot, initialWatchSignature, vcsCatalog }, changeset, initialMode: input.options.mode ?? "auto", initialTheme: input.options.theme, diff --git a/src/core/patch/normalize.ts b/src/core/patch/normalize.ts index 28e797d4c..8e072ac5a 100644 --- a/src/core/patch/normalize.ts +++ b/src/core/patch/normalize.ts @@ -1,14 +1,7 @@ import { normalizeGitPatch, type NormalizedGitPatch } from "./gitFormat"; import { stripGitLogMetadata } from "./gitLog"; -/** Escape only path characters that break unified-diff header parsing. */ -export function escapeUntrackedPatchPath(path: string) { - return path - .replaceAll("\\", "\\\\") - .replaceAll("\t", "\\t") - .replaceAll("\n", "\\n") - .replaceAll("\r", "\\r"); -} +export { escapeUntrackedPatchPath } from "../../lib/patchPath"; /** Remove terminal escape sequences so Git-colored pager input still parses as plain patch text. */ export function stripTerminalControl(text: string) { diff --git a/src/core/projectRoot.test.ts b/src/core/projectRoot.test.ts new file mode 100644 index 000000000..d542b4402 --- /dev/null +++ b/src/core/projectRoot.test.ts @@ -0,0 +1,75 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { findProjectRootCandidate } from "./projectRoot"; +import { createVcsCatalog } from "./vcs"; +import type { VcsAdapter } from "./vcs/types"; + +const tempDirs: string[] = []; + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +function tempDir() { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "hunk-project-root-"))); + tempDirs.push(dir); + return dir; +} + +/** Build a marker adapter for project-root precedence tests. */ +function markerAdapter(marker: string): VcsAdapter { + return { + id: "custom", + name: "Custom", + operations: {}, + detect(cwd) { + let current = cwd; + for (;;) { + if (existsSync(join(current, marker))) { + return { id: "custom", repoRoot: current }; + } + const parent = dirname(current); + if (parent === current) { + return null; + } + current = parent; + } + }, + }; +} + +describe("findProjectRootCandidate", () => { + test("uses .hunk as a provider-independent bootstrap marker", () => { + const repo = tempDir(); + const nested = join(repo, "src", "deep"); + mkdirSync(join(repo, ".hunk")); + mkdirSync(nested, { recursive: true }); + + expect(findProjectRootCandidate(nested)).toBe(repo); + }); + + test("ignores a plain file named .hunk", () => { + const directory = tempDir(); + const nested = join(directory, "src"); + writeFileSync(join(directory, ".hunk"), "not a project directory\n"); + mkdirSync(nested); + + expect(findProjectRootCandidate(nested)).toBeUndefined(); + }); + + test("chooses the nearest .hunk or registered VCS root", () => { + const outer = tempDir(); + const inner = join(outer, "inner"); + const nested = join(inner, "src"); + mkdirSync(join(outer, ".custom")); + mkdirSync(join(inner, ".hunk"), { recursive: true }); + mkdirSync(nested, { recursive: true }); + const catalog = createVcsCatalog([markerAdapter(".custom")], "custom"); + + expect(findProjectRootCandidate(nested, catalog)).toBe(inner); + }); +}); diff --git a/src/core/projectRoot.ts b/src/core/projectRoot.ts new file mode 100644 index 000000000..ef301e454 --- /dev/null +++ b/src/core/projectRoot.ts @@ -0,0 +1,41 @@ +import fs from "node:fs"; +import { dirname, join, resolve } from "node:path"; +import type { VcsCatalog } from "./vcs/types"; + +/** Return whether one path is a `.hunk` project directory, following directory symlinks. */ +function isHunkProjectDirectory(path: string) { + try { + return fs.statSync(path).isDirectory(); + } catch { + return false; + } +} + +/** Find the nearest project root established by `.hunk` or a registered VCS adapter. */ +export function findProjectRootCandidate( + cwd: string, + catalog?: Pick, +): string | undefined { + let current = resolve(cwd); + + for (;;) { + if ( + isHunkProjectDirectory(join(current, ".hunk")) || + (catalog?.adapters ?? []).some((adapter) => { + try { + return adapter.detect(current)?.repoRoot === current; + } catch { + return false; + } + }) + ) { + return current; + } + + const parent = dirname(current); + if (parent === current) { + return undefined; + } + current = parent; + } +} diff --git a/src/core/types.ts b/src/core/types.ts index 8ebcd95de..a3b941a0e 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -1,7 +1,4 @@ import type { FileDiffMetadata } from "@pierre/diffs"; -// Type-only import; the extension types depend on this module in turn, and -// `import type` keeps that relationship out of the runtime module graph. -import type { ExtensionLoadResult } from "../extensions/types"; import type { AgentFileContext, ExtensionVcsDiffInput, @@ -11,7 +8,7 @@ import type { } from "../extension-api/types"; import type { FileSourceFetcher } from "./fileSource"; import type { StartupNotice } from "./startupNotice"; -import type { VcsAdapter } from "./vcs/types"; +import type { VcsCatalog } from "./vcs/types"; /** * Shapes that are simultaneously internal model types and part of the published @@ -172,6 +169,8 @@ export interface SessionSelectorInput { sessionId?: string; sessionPath?: string; repoRoot?: string; + /** Nearest project boundary known for this repo-path selector. */ + repoBoundary?: string; } export interface SessionListCommandInput { @@ -366,18 +365,11 @@ export interface ReloadContext { cwd: string; repoRoot?: string; initialWatchSignature?: string; - /** - * Extension-contributed VCS backends this session loaded its review through. - * - * Watch planning and signatures re-resolve the adapter from the input's - * configured VCS id, so they need the same adapter set the changeset came - * from — otherwise a review backed by an extension backend could not be - * watched at all. - */ - vcsAdapters?: readonly VcsAdapter[]; + /** Complete catalog used to load this review, retained for reload and watch. */ + vcsCatalog?: VcsCatalog; } -export interface AppBootstrap { +export interface AppBootstrap { input: CliInput; reloadContext: ReloadContext; changeset: Changeset; @@ -398,6 +390,6 @@ export interface AppBootstrap { viewPreferencesConfigPath?: string; /** The user's `[keybindings]` table, resolved against command defaults in App. */ keybindings?: Record; - /** Extensions loaded for this session, and any load failures worth surfacing. */ - extensions?: ExtensionLoadResult; + /** App-owned extension state carried without coupling core to the extension host. */ + extensions?: ExtensionState; } diff --git a/src/core/vcs/gitSource.test.ts b/src/core/vcs/gitSource.test.ts deleted file mode 100644 index af5fb1bde..000000000 --- a/src/core/vcs/gitSource.test.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { join } from "node:path"; -import { gitEndpointSourceSpec } from "./gitSource"; - -describe("gitEndpointSourceSpec", () => { - test("maps every endpoint kind to a source spec", () => { - expect(gitEndpointSourceSpec({ kind: "none" }, "/repo", "a.ts")).toEqual({ kind: "none" }); - expect(gitEndpointSourceSpec({ kind: "git-ref", ref: "HEAD" }, "/repo", "a.ts")).toEqual({ - kind: "git-blob", - repoRoot: "/repo", - ref: "HEAD", - path: "a.ts", - }); - expect(gitEndpointSourceSpec({ kind: "index" }, "/repo", "a.ts")).toEqual({ - kind: "git-index", - repoRoot: "/repo", - path: "a.ts", - }); - expect(gitEndpointSourceSpec({ kind: "worktree" }, "/repo", "a.ts")).toEqual({ - kind: "fs", - absolutePath: join("/repo", "a.ts"), - }); - }); -}); diff --git a/src/core/vcs/index.test.ts b/src/core/vcs/index.test.ts index 7434119a0..1a5179bab 100644 --- a/src/core/vcs/index.test.ts +++ b/src/core/vcs/index.test.ts @@ -1,287 +1,144 @@ -import { afterEach, describe, expect, test } from "bun:test"; -import { mkdtempSync, mkdirSync, rmSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { getBundledVcsAdapters } from "../../extensions/default/vcs"; +import { describe, expect, test } from "bun:test"; +import { HunkUserError } from "../errors"; import { createUnsupportedVcsOperationError, - createVcsWatchPlan, + createVcsCatalog, detectVcs, - findVcsRepoRootCandidate, - getBuiltInVcsAdapters, - getVcsAdapter, + extendVcsCatalog, + getConfiguredVcsAdapter, getDefaultVcsAdapter, - getVcsOperation, + getVcsAdapter, isVcsId, + isVcsReviewInput, loadVcsReview, operationFromInput, - resolveVcsAdapters, } from "."; -import type { VcsShowCommandInput, VcsStashShowCommandInput, VcsDiffCommandInput } from "../types"; import type { VcsAdapter } from "./types"; -const tempDirs: string[] = []; - -function createTempDir(prefix: string) { - const dir = mkdtempSync(join(tmpdir(), prefix)); - tempDirs.push(dir); - return dir; +function adapter( + id: string, + options: { + root?: string; + priority?: number; + operations?: VcsAdapter["operations"]; + } = {}, +): VcsAdapter { + return { + id, + name: id.toUpperCase(), + detectionPriority: options.priority, + detect: () => (options.root ? { id, repoRoot: options.root } : null), + operations: options.operations ?? {}, + }; } -afterEach(() => { - while (tempDirs.length > 0) { - const dir = tempDirs.pop(); - if (dir) { - rmSync(dir, { recursive: true, force: true }); - } - } -}); - -describe("VCS adapter registry", () => { - test("registers Git, Jujutsu, and Sapling operation maps", () => { - // Every one of these comes from the bundled extension tier: there are no - // core-registered adapters, so this list is purely an ordering of what the - // bundled factories registered through `hunk.registerVcsAdapter`. - expect(getBuiltInVcsAdapters().map((adapter) => adapter.id)).toEqual(["jj", "sl", "git"]); - expect(getBuiltInVcsAdapters()).toEqual( - [...getBundledVcsAdapters()].sort( - (left, right) => (right.detectionPriority ?? 0) - (left.detectionPriority ?? 0), - ), +describe("VCS catalog", () => { + test("orders detection by priority while preserving tie order", () => { + const catalog = createVcsCatalog( + [adapter("first", { priority: 10 }), adapter("low"), adapter("second", { priority: 10 })], + "first", ); - expect(getVcsAdapter("git").operations["working-tree-diff"]).toBeDefined(); - expect(getVcsAdapter("git").operations["revision-show"]).toBeDefined(); - expect(getVcsAdapter("git").operations["stash-show"]).toBeDefined(); - expect(getVcsAdapter("jj").operations["working-tree-diff"]).toBeDefined(); - expect(getVcsAdapter("jj").operations["revision-show"]).toBeDefined(); - expect(getVcsAdapter("jj").operations["stash-show"]).toBeUndefined(); - expect(getVcsAdapter("sl").operations["working-tree-diff"]).toBeDefined(); - expect(getVcsAdapter("sl").operations["revision-show"]).toBeDefined(); - expect(getVcsAdapter("sl").operations["stash-show"]).toBeUndefined(); + expect(catalog.adapters.map((entry) => entry.id)).toEqual(["first", "second", "low"]); }); - test("falls back to the bundled Git backend when config names none", () => { - expect(getDefaultVcsAdapter().id).toBe("git"); - expect(getDefaultVcsAdapter()).toBe(getVcsAdapter("git")); - }); + test("extends without replacing reserved or already claimed ids", () => { + const git = adapter("git"); + const base = createVcsCatalog([git], "git"); + const catalog = extendVcsCatalog(base, [adapter("git"), adapter("hg"), adapter("hg")]); - test("validates VCS ids from the registered adapter list", () => { - expect(isVcsId("git")).toBe(true); - expect(isVcsId("jj")).toBe(true); - expect(isVcsId("sl")).toBe(true); - expect(isVcsId("hg")).toBe(false); + expect(catalog.adapters.map((entry) => entry.id)).toEqual(["git", "hg"]); + expect(catalog.reservedIds).toEqual(new Set(["git"])); + expect(isVcsId("git", catalog)).toBe(true); + expect(isVcsId("hg", catalog)).toBe(false); }); - test("throws for an unregistered VCS id", () => { - expect(() => getVcsAdapter("hg" as VcsAdapter["id"])).toThrow("Unsupported VCS: hg"); + test("resolves configured and default adapters from the supplied catalog", () => { + const git = adapter("git"); + const hg = adapter("hg"); + const catalog = createVcsCatalog([git, hg], "git", ["git"]); + + expect(getDefaultVcsAdapter(catalog)).toBe(git); + expect(getConfiguredVcsAdapter(undefined, catalog)).toBe(git); + expect(getVcsAdapter("hg", catalog)).toBe(hg); + expect(() => getVcsAdapter("missing", catalog)).toThrow("Unsupported VCS: missing"); }); - test("orders built-ins by detection priority, jj and Sapling above the Git baseline", () => { - const priorities = getBuiltInVcsAdapters().map((adapter) => adapter.detectionPriority ?? 0); - expect(priorities).toEqual([...priorities].sort((left, right) => right - left)); - expect(getVcsAdapter("jj").detectionPriority).toBeGreaterThan( - getVcsAdapter("git").detectionPriority ?? 0, - ); - expect(getVcsAdapter("sl").detectionPriority).toBeGreaterThan( - getVcsAdapter("git").detectionPriority ?? 0, + test("detects the nearest root before consulting same-root priority", () => { + const catalog = createVcsCatalog( + [ + adapter("outer-high", { root: "/repo", priority: 100 }), + adapter("inner", { root: "/repo/nested", priority: 0 }), + ], + "outer-high", ); + expect(detectVcs("/repo/nested/src", catalog)?.id).toBe("inner"); }); - test("assembles built-ins ahead of unprioritized extension adapters", () => { - const createExtensionAdapter = (id: string, detectionPriority?: number): VcsAdapter => ({ - id, - name: id, - detect: () => null, + test("uses priority for colocated detections and isolates throwing adapters", () => { + const broken: VcsAdapter = { + id: "broken", + name: "Broken", + detectionPriority: 1_000, + detect: () => { + throw new Error("boom"); + }, operations: {}, - ...(detectionPriority === undefined ? {} : { detectionPriority }), - }); - - expect( - resolveVcsAdapters([ - createExtensionAdapter("hg"), - createExtensionAdapter("pijul"), - // Built-in ids stay reserved, whatever priority an extension claims. - createExtensionAdapter("git", 1_000), - ]).map((adapter) => adapter.id), - ).toEqual(["jj", "sl", "git", "hg", "pijul"]); - - // An extension that explicitly outranks Git is honored: the user's machine. - expect( - resolveVcsAdapters([createExtensionAdapter("hg", 500)]).map((adapter) => adapter.id), - ).toEqual(["hg", "jj", "sl", "git"]); - }); - - test("finds repo root candidates through bundled adapter detection", () => { - const repo = createTempDir("hunk-vcs-bundled-marker-"); - const nested = join(repo, "src", "nested"); - // `.jj` is only reachable through the bundled Jujutsu extension's detect(), - // so finding this root proves repo discovery consults the bundled tier. - mkdirSync(join(repo, ".jj"), { recursive: true }); - mkdirSync(nested, { recursive: true }); - - expect(findVcsRepoRootCandidate(nested)).toBe(repo); - }); - - test("detects a colocated jj repository as jj rather than git", () => { - const repo = createTempDir("hunk-vcs-colocated-jj-"); - const nested = join(repo, "src", "nested"); - // `jj git init --colocate` leaves both markers at the same root, so nothing - // but detection priority separates them. - mkdirSync(join(repo, ".jj")); - mkdirSync(join(repo, ".git")); - mkdirSync(nested, { recursive: true }); - - expect(detectVcs(repo)).toEqual({ id: "jj", repoRoot: repo }); - expect(detectVcs(nested)).toEqual({ id: "jj", repoRoot: repo }); - expect(findVcsRepoRootCandidate(nested)).toBe(repo); - }); - - test("detects a colocated Sapling repository as sl rather than git", () => { - const repo = createTempDir("hunk-vcs-colocated-sl-"); - mkdirSync(join(repo, ".sl")); - mkdirSync(join(repo, ".git")); - - expect(detectVcs(repo)).toEqual({ id: "sl", repoRoot: repo }); - }); - - test("detects repository roots by registered adapter priority", () => { - const repo = createTempDir("hunk-vcs-registry-"); - const nested = join(repo, "src", "nested"); - mkdirSync(nested, { recursive: true }); - mkdirSync(join(repo, ".git")); - - expect(detectVcs(nested)).toEqual({ id: "git", repoRoot: repo }); - expect(findVcsRepoRootCandidate(nested)).toBe(repo); - }); - - test("prefers the nearest checkout over a parent repository with higher adapter priority", () => { - const parent = createTempDir("hunk-vcs-parent-jj-"); - const repo = join(parent, "project"); - const nested = join(repo, "src", "nested"); - mkdirSync(join(parent, ".jj")); - mkdirSync(join(repo, ".git"), { recursive: true }); - mkdirSync(nested, { recursive: true }); - - expect(detectVcs(nested)).toEqual({ id: "git", repoRoot: repo }); - expect(findVcsRepoRootCandidate(nested)).toBe(repo); + }; + const catalog = createVcsCatalog( + [broken, adapter("low", { root: "/repo" }), adapter("high", { root: "/repo", priority: 5 })], + "low", + ); + expect(detectVcs("/repo/src", catalog)?.id).toBe("high"); }); +}); - test("maps CLI inputs to neutral review operations", () => { - const diffInput = { - kind: "vcs", - staged: false, - options: { vcs: "git" }, - } satisfies VcsDiffCommandInput; - const showInput = { - kind: "show", - ref: "HEAD", - options: { vcs: "git" }, - } satisfies VcsShowCommandInput; - const stashInput = { - kind: "stash-show", - options: { vcs: "git" }, - } satisfies VcsStashShowCommandInput; - - expect(operationFromInput(diffInput)).toEqual({ kind: "working-tree-diff", input: diffInput }); - expect(operationFromInput(showInput)).toEqual({ kind: "revision-show", input: showInput }); - expect(operationFromInput(stashInput)).toEqual({ kind: "stash-show", input: stashInput }); +describe("VCS operation dispatch", () => { + test("classifies exactly the adapter-backed review inputs", () => { + expect(isVcsReviewInput({ kind: "vcs", staged: false, options: {} })).toBe(true); + expect(isVcsReviewInput({ kind: "show", options: {} })).toBe(true); + expect(isVcsReviewInput({ kind: "stash-show", options: {} })).toBe(true); + expect(isVcsReviewInput({ kind: "patch", file: "change.patch", options: {} })).toBe(false); + expect(isVcsReviewInput({ kind: "diff", left: "a", right: "b", options: {} })).toBe(false); }); - test("creates friendly errors for unsupported adapter operations", async () => { - const adapter = getVcsAdapter("jj"); - const input = { - kind: "stash-show", - options: { vcs: "jj" }, - } satisfies VcsStashShowCommandInput; - - expect( - createUnsupportedVcsOperationError(adapter, operationFromInput(input).kind).message, - ).toBe("`hunk stash show` requires Git VCS mode."); - await expect( - loadVcsReview(adapter, operationFromInput(input), { cwd: process.cwd() }), - ).rejects.toThrow("`hunk stash show` requires Git VCS mode."); + test("maps review inputs to operation keys", () => { + expect(operationFromInput({ kind: "vcs", staged: false, options: {} }).kind).toBe( + "working-tree-diff", + ); + expect(operationFromInput({ kind: "show", options: {} }).kind).toBe("revision-show"); + expect(operationFromInput({ kind: "stash-show", options: {} }).kind).toBe("stash-show"); }); - test("dispatches watch plans and leaves adapters without one poll-only", () => { - const input = { - kind: "vcs", - staged: false, - options: { vcs: "custom" }, - } satisfies VcsDiffCommandInput; - const target = { - kind: "directory-tree" as const, - directory: "/repo", - ignoredRoots: [], - sources: ["worktree" as const], - }; - const adapter = { - id: "custom", - name: "Custom VCS", - detect: () => null, + test("loads through the selected operation", async () => { + const git = adapter("git", { operations: { "working-tree-diff": { - load: async () => ({ - repoRoot: "/repo", - sourceLabel: "/repo", - title: "x", - patchText: "", - }), - watchPlan: () => ({ coverage: "hybrid" as const, targets: [target] }), + async load(_input, { cwd }) { + return { repoRoot: cwd, sourceLabel: cwd, title: "review", patchText: "" }; + }, }, }, - } satisfies VcsAdapter; - - expect(createVcsWatchPlan(adapter, operationFromInput(input), { cwd: "/repo" })).toEqual({ - coverage: "hybrid", - targets: [target], }); - expect( - createVcsWatchPlan( - getVcsAdapter("jj"), - operationFromInput({ ...input, options: { vcs: "jj" } }), - { - cwd: "/repo", - }, - ), - ).toEqual({ coverage: "poll-only", targets: [] }); + const catalog = createVcsCatalog([git], "git"); + const input = { kind: "vcs", staged: false, options: {} } as const; + const result = await loadVcsReview(git, operationFromInput(input), { cwd: "/repo" }, catalog); + expect(result.repoRoot).toBe("/repo"); }); - test("treats a missing operation map as unsupported rather than crashing", async () => { - // Only reachable from an untyped extension, which is exactly the case that - // used to reach `adapter.operations[kind]` on undefined and throw a TypeError. - const adapter = { id: "bare", name: "Bare VCS", detect: () => null } as unknown as VcsAdapter; - const input = { - kind: "vcs", - staged: false, - options: { vcs: "bare" }, - } satisfies VcsDiffCommandInput; - - expect(getVcsOperation(adapter, operationFromInput(input))).toBeUndefined(); - expect(createUnsupportedVcsOperationError(adapter, "working-tree-diff").message).toBe( - "Bare VCS does not support working-tree-diff.", - ); - await expect( - loadVcsReview(adapter, operationFromInput(input), { cwd: process.cwd() }), - ).rejects.toThrow("Bare VCS does not support working-tree-diff."); - expect(() => createVcsWatchPlan(adapter, operationFromInput(input), { cwd: "/repo" })).toThrow( - "Bare VCS does not support working-tree-diff.", - ); - }); - - test("names the adapter and operation for non-stash unsupported operations", () => { - const adapter = { - id: "custom", - name: "Custom VCS", - detect: () => null, - operations: {}, - } satisfies VcsAdapter; - const input = { - kind: "vcs", - staged: false, - options: { vcs: "custom" }, - } satisfies VcsDiffCommandInput; - - expect( - createUnsupportedVcsOperationError(adapter, operationFromInput(input).kind).message, - ).toBe("Custom VCS does not support working-tree-diff."); + test("recommends a supporting adapter from the active catalog", () => { + const git = adapter("git", { + operations: { + "stash-show": { + async load() { + return { repoRoot: "/repo", sourceLabel: "/repo", title: "stash", patchText: "" }; + }, + }, + }, + }); + const jj = adapter("jj"); + const catalog = createVcsCatalog([jj, git], "git"); + const error = createUnsupportedVcsOperationError(jj, "stash-show", catalog); + expect(error).toBeInstanceOf(HunkUserError); + expect(error.message).toContain("requires GIT VCS mode"); }); }); diff --git a/src/core/vcs/index.ts b/src/core/vcs/index.ts index 36a5f339e..14f6f0c89 100644 --- a/src/core/vcs/index.ts +++ b/src/core/vcs/index.ts @@ -1,9 +1,10 @@ -import { dirname, relative, resolve } from "node:path"; +import { relative, resolve } from "node:path"; import { HUNK_DEFAULT_VCS_DETECTION_PRIORITY } from "../../extension-api/types"; -import { getBundledVcsAdapters } from "../../extensions/default/vcs"; import { HunkUserError } from "../errors"; +import type { CliInput } from "../types"; import type { VcsAdapter, + VcsCatalog, VcsDetection, VcsId, VcsLoadContext, @@ -14,16 +15,7 @@ import type { VcsReviewOperationKind, } from "./types"; -/** The backend a session falls back to when config names none. */ -const DEFAULT_VCS_ID = "git"; - -/** - * Order adapters the way detection consults them: highest priority first. - * - * The sort is stable, so adapters that declare the same priority keep the order - * they were assembled in — bundled extensions in load order, then user - * extensions in registration order. - */ +/** Order adapters by detection priority while preserving assembly order for ties. */ function orderByDetectionPriority(adapters: readonly VcsAdapter[]): VcsAdapter[] { return [...adapters].sort( (left, right) => @@ -32,104 +24,79 @@ function orderByDetectionPriority(adapters: readonly VcsAdapter[]): VcsAdapter[] ); } -/** Adapters that are part of the product, in detection order. */ -let builtInAdapters: VcsAdapter[] | undefined; - -/** - * Return the adapters Hunk ships with, assembled once per process. - * - * Every one of them — Git included — comes from the bundled extension tier, so - * this is a pure ordering step over what those factories registered. They are - * all product behavior, they all take part in first-class detection, and they - * all reserve their id against user extensions. Bundled loading is resolved - * lazily so this module can be imported from anywhere in the graph without - * depending on module evaluation order. - */ -export function getBuiltInVcsAdapters(): readonly VcsAdapter[] { - builtInAdapters ??= orderByDetectionPriority(getBundledVcsAdapters()); - return builtInAdapters; +/** Create the complete provider-neutral adapter catalog used by a composition root. */ +export function createVcsCatalog( + adapters: readonly VcsAdapter[], + defaultAdapterId: VcsId, + reservedIds: Iterable = adapters.map((adapter) => adapter.id), +): VcsCatalog { + return { + adapters: orderByDetectionPriority(adapters), + defaultAdapterId, + reservedIds: new Set(reservedIds), + }; } -/** - * Combine the built-in adapters with the session's user-extension ones. - * - * This is the one place adapter order is decided. Ids owned by a built-in - * backend are dropped here (callers report the skip once, at registration - * time), and everything else sorts by `detectionPriority` — which puts user - * adapters below Git unless they explicitly ask for more. - */ -export function resolveVcsAdapters(extraAdapters: readonly VcsAdapter[] = []): VcsAdapter[] { - const builtIns = getBuiltInVcsAdapters(); - if (extraAdapters.length === 0) { - return [...builtIns]; - } +/** Add user adapters without allowing them to replace ids owned by the base catalog. */ +export function extendVcsCatalog( + base: VcsCatalog, + extraAdapters: readonly VcsAdapter[], +): VcsCatalog { + const claimed = new Set(base.adapters.map((adapter) => adapter.id)); + const accepted = extraAdapters.filter((adapter) => { + if (base.reservedIds.has(adapter.id) || claimed.has(adapter.id)) { + return false; + } + claimed.add(adapter.id); + return true; + }); - return orderByDetectionPriority([ - ...builtIns, - ...extraAdapters.filter((adapter) => !isVcsId(adapter.id)), - ]); + return createVcsCatalog([...base.adapters, ...accepted], base.defaultAdapterId, base.reservedIds); } -/** Return the fallback adapter used when config has not selected a provider explicitly. */ -export function getDefaultVcsAdapter(): VcsAdapter { - const adapter = getBuiltInVcsAdapters().find((candidate) => candidate.id === DEFAULT_VCS_ID); +/** Return the catalog's fallback adapter. */ +export function getDefaultVcsAdapter(catalog: VcsCatalog): VcsAdapter { + const adapter = catalog.adapters.find((candidate) => candidate.id === catalog.defaultAdapterId); if (!adapter) { - // Only reachable if the bundled Git factory itself failed to load, which - // would leave Hunk with no default backend at all. - throw new HunkUserError("Hunk's bundled Git backend failed to load.", [ + throw new HunkUserError(`Hunk's default ${catalog.defaultAdapterId} backend failed to load.`, [ "Reinstall Hunk, or report this at https://github.com/modem-dev/hunk/issues.", ]); } - return adapter; } -/** Return the configured adapter, or the default adapter when no VCS id was supplied. */ -export function getConfiguredVcsAdapter( - id: VcsId | undefined, - extraAdapters: readonly VcsAdapter[] = [], -): VcsAdapter { - return id ? getVcsAdapter(id, extraAdapters) : getDefaultVcsAdapter(); +/** Return the configured adapter, or the catalog default when no id was supplied. */ +export function getConfiguredVcsAdapter(id: VcsId | undefined, catalog: VcsCatalog): VcsAdapter { + return id ? getVcsAdapter(id, catalog) : getDefaultVcsAdapter(catalog); } -export function getVcsAdapter(id: VcsId, extraAdapters: readonly VcsAdapter[] = []): VcsAdapter { - const adapter = resolveVcsAdapters(extraAdapters).find((candidate) => candidate.id === id); +/** Resolve one adapter id from the complete session catalog. */ +export function getVcsAdapter(id: VcsId, catalog: VcsCatalog): VcsAdapter { + const adapter = catalog.adapters.find((candidate) => candidate.id === id); if (!adapter) { throw new Error(`Unsupported VCS: ${id}`); } return adapter; } -/** Report whether one value names a backend Hunk ships with (core Git or a bundled one). */ -export function isVcsId(value: unknown): value is VcsId { - return getBuiltInVcsAdapters().some((adapter) => adapter.id === value); +/** Report whether a catalog reserves one adapter id against user extensions. */ +export function isVcsId(value: unknown, catalog: VcsCatalog): value is VcsId { + return typeof value === "string" && catalog.reservedIds.has(value); } -/** - * Detect the nearest containing VCS checkout. - * - * Distance decides first, so a Git checkout nested inside a jj workspace is - * reviewed as Git. Detection priority only breaks same-root ties — the - * colocated case, where one directory carries markers for two backends. - */ -export function detectVcs( - cwd: string, - extraAdapters: readonly VcsAdapter[] = [], -): VcsDetection | null { +/** Detect the nearest containing checkout across one complete catalog. */ +export function detectVcs(cwd: string, catalog: VcsCatalog): VcsDetection | null { const start = resolve(cwd); let bestDetection: VcsDetection | null = null; let bestDistance = Number.POSITIVE_INFINITY; - for (const adapter of resolveVcsAdapters(extraAdapters)) { - // Extension adapters run third-party detection code here; a throwing - // adapter must not stop the remaining adapters from being consulted. + for (const adapter of catalog.adapters) { let detected: VcsDetection | null; try { detected = adapter.detect(start); } catch { continue; } - if (!detected) { continue; } @@ -146,29 +113,12 @@ export function detectVcs( return bestDetection; } -/** - * Walk upward for the nearest directory a shipped backend calls a repo root. - * - * Config resolution and extension discovery both run before user extensions - * exist, so this deliberately consults built-ins only — which, since the whole - * bundled tier is loaded by then, still covers every backend Hunk ships with. - */ -export function findVcsRepoRootCandidate(cwd = process.cwd()) { - let current = resolve(cwd); - - for (;;) { - if (getBuiltInVcsAdapters().some((adapter) => adapter.detect(current)?.repoRoot === current)) { - return current; - } - - const parent = dirname(current); - if (parent === current) { - return undefined; - } - current = parent; - } +/** Return whether one CLI input loads a review through a VCS adapter. */ +export function isVcsReviewInput(input: CliInput): input is VcsReviewInput { + return input.kind === "vcs" || input.kind === "show" || input.kind === "stash-show"; } +/** Translate a CLI review input into the neutral operation map key. */ export function operationFromInput(input: VcsReviewInput): VcsReviewOperation { switch (input.kind) { case "vcs": @@ -180,14 +130,7 @@ export function operationFromInput(input: VcsReviewInput): VcsReviewOperation { } } -/** - * Return the adapter operation handler for one neutral review operation, if supported. - * - * The operation map is optional at the extension-authoring boundary and can be - * missing entirely on an adapter registered from JavaScript, so a missing map - * reads the same as a missing operation: unsupported, which callers turn into a - * `HunkUserError` instead of a TypeError. - */ +/** Return one operation handler from an adapter's optional operation map. */ export function getVcsOperation( adapter: VcsAdapter, operation: VcsReviewOperation, @@ -195,56 +138,58 @@ export function getVcsOperation( return adapter.operations?.[operation.kind] as VcsOperation | undefined; } -/** Load a review through the adapter operation map instead of adapter-local switch dispatch. */ +/** Load a review through a provider-neutral adapter operation. */ export async function loadVcsReview( adapter: VcsAdapter, operation: VcsReviewOperation, context: VcsLoadContext, + catalog: VcsCatalog, ): Promise { const handler = getVcsOperation(adapter, operation); if (!handler) { - throw createUnsupportedVcsOperationError(adapter, operation.kind); + throw createUnsupportedVcsOperationError(adapter, operation.kind, catalog); } - return await handler.load(operation.input, context); } -/** Build an adapter-backed event plan, falling back to signature polling when unsupported. */ +/** Build an adapter event plan, falling back to signature polling. */ export function createVcsWatchPlan( adapter: VcsAdapter, operation: VcsReviewOperation, context: VcsLoadContext, + catalog: VcsCatalog, ) { const handler = getVcsOperation(adapter, operation); if (!handler) { - throw createUnsupportedVcsOperationError(adapter, operation.kind); + throw createUnsupportedVcsOperationError(adapter, operation.kind, catalog); } - return handler.watchPlan?.(operation.input, context) ?? { coverage: "poll-only", targets: [] }; } -/** Build an adapter-backed watch signature when the selected operation supports it. */ +/** Build an adapter watch signature when the selected operation supports it. */ export function createVcsWatchSignature( adapter: VcsAdapter, operation: VcsReviewOperation, context: VcsLoadContext, + catalog: VcsCatalog, ) { const handler = getVcsOperation(adapter, operation); if (!handler) { - throw createUnsupportedVcsOperationError(adapter, operation.kind); + throw createUnsupportedVcsOperationError(adapter, operation.kind, catalog); } if (!handler.watchSignature) { throw new Error(`${adapter.name} does not support watch signatures for ${operation.kind}.`); } - return handler.watchSignature(operation.input, context); } +/** Build a user-facing unsupported-operation error using the active catalog. */ export function createUnsupportedVcsOperationError( adapter: VcsAdapter, operationKind: VcsReviewOperationKind, + catalog: VcsCatalog, ) { - const supportingAdapter = getBuiltInVcsAdapters().find( + const supportingAdapter = catalog.adapters.find( (candidate) => candidate.operations?.[operationKind], ); if (operationKind === "stash-show" && supportingAdapter) { diff --git a/src/core/vcs/types.ts b/src/core/vcs/types.ts index 1e4cf32be..d8293e78b 100644 --- a/src/core/vcs/types.ts +++ b/src/core/vcs/types.ts @@ -16,7 +16,6 @@ export interface VcsDetection { export interface VcsLoadContext { cwd: string; - gitExecutable?: string; } export type VcsReviewInput = VcsDiffCommandInput | VcsShowCommandInput | VcsStashShowCommandInput; @@ -61,6 +60,14 @@ export interface VcsPatchResult { extraFiles?: DiffFile[]; } +/** Complete ordered VCS capability set used throughout one session. */ +export interface VcsCatalog { + adapters: readonly VcsAdapter[]; + defaultAdapterId: VcsId; + /** Adapter ids owned by the base product and unavailable to user registrations. */ + reservedIds: ReadonlySet; +} + export interface VcsAdapter { id: VcsId; name: string; diff --git a/src/core/vcs/untracked.ts b/src/core/vcs/untracked.ts index 2bf6a0c53..45fcf3cae 100644 --- a/src/core/vcs/untracked.ts +++ b/src/core/vcs/untracked.ts @@ -3,10 +3,10 @@ import fs from "node:fs"; import { join } from "node:path"; import { createSkippedBinaryMetadata, isProbablyBinaryFile } from "../binary"; import { buildDiffFile, createSkippedLargeMetadata } from "../diffFile"; -import { inspectLargeUntrackedFile } from "./largeFile"; -import { escapeUntrackedPatchPath } from "../patch/normalize"; +import { inspectLargeUntrackedFile } from "../../lib/largeFile"; +import { escapeUntrackedPatchPath } from "../../lib/patchPath"; import { parseSingleFilePatch } from "../patch/singleFile"; -import type { LargeFileCheck } from "./largeFile"; +import type { LargeFileCheck } from "../../lib/largeFile"; /** * Host-side synthesis of untracked files into reviewable diffs. diff --git a/src/core/watch.test.ts b/src/core/watch.test.ts index abbe1eee9..24ab7e544 100644 --- a/src/core/watch.test.ts +++ b/src/core/watch.test.ts @@ -2,12 +2,25 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { computeWatchSignature } from "./watch"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; +import { createVcsCatalog } from "./vcs"; +import { + computeWatchSignature as computeCoreWatchSignature, + type WatchSignatureContext, +} from "./watch"; import type { CliInput } from "./types"; import type { VcsAdapter } from "./vcs/types"; const tempDirs: string[] = []; +/** Compute with the app's bundled catalog unless a test supplies another. */ +function computeWatchSignature(input: CliInput, context: WatchSignatureContext) { + return computeCoreWatchSignature(input, { + vcsCatalog: getBundledVcsCatalog(), + ...context, + }); +} + function cleanupTempDirs() { while (tempDirs.length > 0) { const dir = tempDirs.pop(); @@ -160,9 +173,12 @@ describe("computeWatchSignature", () => { options: { mode: "auto", vcs: "hg" }, } satisfies CliInput; - expect(computeWatchSignature(input, { cwd: process.cwd(), vcsAdapters: [adapter] })).toBe( - "vcs\n---\nhg:working-copy", - ); + expect( + computeWatchSignature(input, { + cwd: process.cwd(), + vcsCatalog: createVcsCatalog([adapter], "demo", []), + }), + ).toBe("vcs\n---\nhg:working-copy"); }); test("rejects unsupported watch operations before invoking adapter signatures", () => { diff --git a/src/core/watch.ts b/src/core/watch.ts index 1b3e78dc9..c056da5eb 100644 --- a/src/core/watch.ts +++ b/src/core/watch.ts @@ -2,7 +2,7 @@ import fs from "node:fs"; import { resolve } from "node:path"; import { createVcsWatchSignature, getConfiguredVcsAdapter, operationFromInput } from "./vcs"; import type { CliInput } from "./types"; -import type { VcsAdapter } from "./vcs/types"; +import type { VcsCatalog } from "./vcs/types"; /** Format one file stat into a stable signature fragment, or mark the path missing. */ function statSignature(path: string) { @@ -19,16 +19,18 @@ function vcsPatchSignature( input: Extract, context: WatchSignatureContext, ) { - const adapter = getConfiguredVcsAdapter(input.options.vcs, context.vcsAdapters); + if (!context.vcsCatalog) { + throw new Error("VCS-backed watch signatures require a composed VCS catalog."); + } + const adapter = getConfiguredVcsAdapter(input.options.vcs, context.vcsCatalog); const operation = operationFromInput(input); - return createVcsWatchSignature(adapter, operation, context); + return createVcsWatchSignature(adapter, operation, { cwd: context.cwd }, context.vcsCatalog); } export interface WatchSignatureContext { cwd: string; - gitExecutable?: string; - /** Extension-contributed adapters, so a watched review keeps its own backend. */ - vcsAdapters?: readonly VcsAdapter[]; + /** Complete catalog retained from the review load. */ + vcsCatalog?: VcsCatalog; } /** Compute a change-detection signature relative to the source's stable load context. */ diff --git a/src/core/watchPlan.test.ts b/src/core/watchPlan.test.ts index 237ae9762..c52e32259 100644 --- a/src/core/watchPlan.test.ts +++ b/src/core/watchPlan.test.ts @@ -1,11 +1,18 @@ import { describe, expect, test } from "bun:test"; import { posix, win32 } from "node:path"; -import { resolveWatchPlan } from "./watchPlan"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; +import { createVcsCatalog } from "./vcs"; +import { resolveWatchPlan as resolveCoreWatchPlan, type WatchPlanContext } from "./watchPlan"; import type { CliInput } from "./types"; import type { VcsAdapter } from "./vcs/types"; const cwd = posix.join("/", "workspace", "review"); +/** Resolve with the app's bundled catalog unless a test supplies another. */ +function resolveWatchPlan(input: CliInput, context: WatchPlanContext) { + return resolveCoreWatchPlan(input, { vcsCatalog: getBundledVcsCatalog(), ...context }); +} + /** Build one expected exact-entry target for a plan assertion. */ function entries( directory: string, @@ -232,7 +239,13 @@ describe("resolveWatchPlan", () => { }; const input = { kind: "vcs", staged: false, options: { vcs: "hg" } } satisfies CliInput; - expect(resolveWatchPlan(input, { cwd, platform: "linux", vcsAdapters: [adapter] })).toEqual({ + expect( + resolveWatchPlan(input, { + cwd, + platform: "linux", + vcsCatalog: createVcsCatalog([adapter], "hg", []), + }), + ).toEqual({ coverage: "hybrid", targets: [target], }); @@ -251,7 +264,13 @@ describe("resolveWatchPlan", () => { }; const input = { kind: "vcs", staged: false, options: { vcs: "hg" } } satisfies CliInput; - expect(resolveWatchPlan(input, { cwd, platform: "linux", vcsAdapters: [adapter] })).toEqual({ + expect( + resolveWatchPlan(input, { + cwd, + platform: "linux", + vcsCatalog: createVcsCatalog([adapter], "hg", []), + }), + ).toEqual({ coverage: "poll-only", targets: [], }); diff --git a/src/core/watchPlan.ts b/src/core/watchPlan.ts index 826c2b2d8..21134f4f0 100644 --- a/src/core/watchPlan.ts +++ b/src/core/watchPlan.ts @@ -8,7 +8,7 @@ import type { ExtensionVcsWatchTargetSource, } from "../extension-api/types"; import type { CliInput } from "./types"; -import type { VcsAdapter } from "./vcs/types"; +import type { VcsCatalog } from "./vcs/types"; import { createVcsWatchPlan, getConfiguredVcsAdapter, operationFromInput } from "./vcs"; /** @@ -25,15 +25,8 @@ export type WatchPlan = ExtensionVcsWatchPlan; export interface WatchPlanContext { cwd: string; platform?: NodeJS.Platform; - gitExecutable?: string; - /** - * Extension-contributed adapters this session resolved reviews through. - * - * Watch planning has to see the same adapter set the changeset was loaded - * with, or a review backed by an extension backend would fall back to - * polling — or fail to find its adapter at all. - */ - vcsAdapters?: readonly VcsAdapter[]; + /** Complete catalog retained from the review load. */ + vcsCatalog?: VcsCatalog; } interface FileTarget { @@ -124,11 +117,16 @@ export function resolveWatchPlan(input: CliInput, context: WatchPlanContext): Wa case "vcs": case "show": case "stash-show": { - const adapter = getConfiguredVcsAdapter(input.options.vcs, context.vcsAdapters); - const adapterPlan = createVcsWatchPlan(adapter, operationFromInput(input), { - cwd: context.cwd, - gitExecutable: context.gitExecutable, - }); + if (!context.vcsCatalog) { + throw new Error("VCS-backed watch plans require a composed VCS catalog."); + } + const adapter = getConfiguredVcsAdapter(input.options.vcs, context.vcsCatalog); + const adapterPlan = createVcsWatchPlan( + adapter, + operationFromInput(input), + { cwd: context.cwd }, + context.vcsCatalog, + ); coverage = adapterPlan.coverage; adapterTargets = adapterPlan.targets; break; diff --git a/src/extension-api/index.ts b/src/extension-api/index.ts index 3d8da1f6f..1fb183014 100644 --- a/src/extension-api/index.ts +++ b/src/extension-api/index.ts @@ -28,6 +28,7 @@ export { HUNK_CORE_VCS_DETECTION_PRIORITY, HUNK_DEFAULT_VCS_DETECTION_PRIORITY, HUNK_EXTENSION_API_VERSION, + HUNK_VCS_DETECTION_BASELINE_PRIORITY, HUNK_EXTENSION_USER_ERROR_NAME, HunkExtensionUserError, } from "./types.js"; @@ -116,6 +117,8 @@ export type { ExtensionVcsFileSide, ExtensionVcsFileSourceReader, ExtensionVcsFileSourceRequest, + ExtensionVcsFileSourceResult, + ExtensionVcsFileSourceTooLarge, ExtensionVcsFileStats, ExtensionVcsLoadContext, ExtensionVcsOperation, diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index ed9bc10c8..e024e6fe6 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -525,14 +525,16 @@ export type ExtensionThemeConfig = NamedCustomThemeConfig; /* -------------------------------------------------------------------------- */ /** - * Detection priority of Hunk's Git backend. + * Baseline detection priority used by Hunk's default bundled backend. * - * Adapters are consulted highest priority first, so this is the baseline every - * other backend positions itself around. Hunk's bundled Jujutsu and Sapling - * backends deliberately register above it: a colocated jj or Sapling checkout - * also contains a `.git` directory, and must not be reviewed as plain Git. + * Adapters are consulted highest priority first. Bundled providers that must + * win a same-root tie register above this value; user adapters can do the same + * when their repository metadata establishes the authoritative working copy. */ -export const HUNK_CORE_VCS_DETECTION_PRIORITY = 0; +export const HUNK_VCS_DETECTION_BASELINE_PRIORITY = 0; + +/** @deprecated Use `HUNK_VCS_DETECTION_BASELINE_PRIORITY`. */ +export const HUNK_CORE_VCS_DETECTION_PRIORITY = HUNK_VCS_DETECTION_BASELINE_PRIORITY; /** * Detection priority an adapter gets when it does not choose one. @@ -552,7 +554,6 @@ export interface ExtensionVcsDetection { /** Ambient information an operation may need to shell out. */ export interface ExtensionVcsLoadContext { cwd: string; - gitExecutable?: string; } /** @@ -639,7 +640,9 @@ export interface ExtensionVcsFileSourceRequest { * what lets Hunk expand context beyond the hunk, highlight against the real * file, and word-diff accurately. Return `null` when the side has no content — * a missing path, or the absent side of an added or deleted file — rather than - * throwing. + * throwing. Return `{ kind: "too-large", maxBytes }` when reading the source + * would exceed the adapter's safety limit; Hunk presents that as an unavailable + * expansion without treating it as an extension failure. * * Hunk calls this at most once per file and side and caches what it resolves, * so the reader does not need its own cache. It is never called for a file the @@ -647,9 +650,19 @@ export interface ExtensionVcsFileSourceRequest { * operation is loading and close over them: the request describes the file, not * the commits, because only the adapter knows how to name them. */ +/** A source side the adapter declined to read because it exceeded its safety limit. */ +export interface ExtensionVcsFileSourceTooLarge { + kind: "too-large"; + /** The byte ceiling the source exceeded, when useful to diagnostics. */ + maxBytes?: number; +} + +/** One exact-source read result returned through the public adapter boundary. */ +export type ExtensionVcsFileSourceResult = string | null | ExtensionVcsFileSourceTooLarge; + export type ExtensionVcsFileSourceReader = ( request: ExtensionVcsFileSourceRequest, -) => Promise; +) => Promise; /* -------------------------------------------------------------------------- */ /* Extra reviewed files */ diff --git a/src/extensions/apply.test.ts b/src/extensions/apply.test.ts index 76432df8d..bedcae65d 100644 --- a/src/extensions/apply.test.ts +++ b/src/extensions/apply.test.ts @@ -8,7 +8,9 @@ import { HUNK_DEFAULT_VCS_DETECTION_PRIORITY, } from "../extension-api/types"; import type { Changeset, DiffFile } from "../core/types"; +import { extendVcsCatalog } from "../core/vcs"; import type { VcsAdapter } from "../core/vcs/types"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; import { applyExtensionChangesetTransforms, applyExtensionFileLanguages, @@ -31,6 +33,12 @@ import { } from "./types"; const tempDirs: string[] = []; +const BASE_VCS_CATALOG = getBundledVcsCatalog(); + +/** Build a complete test catalog from bundled and extension adapters. */ +function catalogWith(adapters: readonly VcsAdapter[] = []) { + return extendVcsCatalog(BASE_VCS_CATALOG, adapters); +} afterEach(() => { for (const dir of tempDirs.splice(0)) { @@ -116,7 +124,7 @@ describe("extension VCS adapters", () => { { extensionId: "mercurial", adapter: createTestVcsAdapter("hg") }, ); - const { adapters, issues } = resolveExtensionVcsAdapters(result.registry); + const { adapters, issues } = resolveExtensionVcsAdapters(result.registry, BASE_VCS_CATALOG); expect(adapters.map((adapter) => adapter.id)).toEqual(["hg"]); expect(issues).toEqual([ @@ -135,7 +143,7 @@ describe("extension VCS adapters", () => { { extensionId: "second", adapter: createTestVcsAdapter("hg") }, ); - const { adapters, issues } = resolveExtensionVcsAdapters(result.registry); + const { adapters, issues } = resolveExtensionVcsAdapters(result.registry, BASE_VCS_CATALOG); expect(adapters.length).toBe(1); expect(issues[0]?.extensionId).toBe("second"); @@ -249,13 +257,7 @@ describe("extension commands", () => { }); describe("resolveDetectedVcsIdWithExtensions", () => { - /** - * A Mercurial-shaped extension adapter that walks upward for a `.hg` marker. - * - * Detection distance is the whole subject here, so a fixture that claims - * whatever directory it is handed would prove nothing: it has to report a real - * repo root the way a backend does. - */ + /** Build a Mercurial-shaped adapter that walks upward for a `.hg` marker. */ function createTestHgAdapter(detectionPriority?: number): VcsAdapter { return { id: "hg", @@ -267,7 +269,6 @@ describe("resolveDetectedVcsIdWithExtensions", () => { if (existsSync(join(current, ".hg"))) { return { id: "hg", repoRoot: current }; } - const parent = dirname(current); if (parent === current) { return null; @@ -280,86 +281,49 @@ describe("resolveDetectedVcsIdWithExtensions", () => { } test("prefers a nearer extension checkout over an outer built-in repository", () => { - // The verified-broken shape: `.hg` one level inside a Git repository. The - // extension root is nearer, so it is the repository the user is standing in. const repo = createTempDir("hunk-apply-nested-hg-"); const inner = join(repo, "inner-hg"); mkdirSync(join(repo, ".git")); mkdirSync(join(inner, ".hg"), { recursive: true }); - expect(resolveDetectedVcsIdWithExtensions(inner, [createTestHgAdapter()])).toBe("hg"); - // Without the adapter, the outer Git root is still all there is to find. - expect(resolveDetectedVcsIdWithExtensions(inner, [])).toBeUndefined(); - }); - - test("prefers a deeply nested extension checkout, dotfiles-home style", () => { - // A `.hg`-managed dotfiles directory several levels below a Git root — the - // farther root used to win purely because a built-in adapter owned it. - const repo = createTempDir("hunk-apply-dotfiles-"); - const dotfiles = join(repo, "home", "user", "dotfiles"); - mkdirSync(join(repo, ".git")); - mkdirSync(join(dotfiles, ".hg"), { recursive: true }); - - expect(resolveDetectedVcsIdWithExtensions(dotfiles, [createTestHgAdapter()])).toBe("hg"); - // One directory below the marker still resolves to the same nearest root. - expect( - resolveDetectedVcsIdWithExtensions(join(dotfiles, "nvim"), [createTestHgAdapter()]), - ).toBe("hg"); - }); - - test("keeps Git for a same-root tie with a default-priority extension adapter", () => { - // Colocated markers: only priority separates them, and the default puts a - // user adapter below Git so installing an extension changes nothing here. - const repo = createTempDir("hunk-apply-colocated-default-"); - mkdirSync(join(repo, ".git")); - mkdirSync(join(repo, ".hg")); - - expect(resolveDetectedVcsIdWithExtensions(repo, [createTestHgAdapter()])).toBe("git"); - expect( - resolveDetectedVcsIdWithExtensions(repo, [ - createTestHgAdapter(HUNK_DEFAULT_VCS_DETECTION_PRIORITY), - ]), - ).toBe("git"); + expect(resolveDetectedVcsIdWithExtensions(inner, catalogWith([createTestHgAdapter()]))).toBe( + "hg", + ); + expect(resolveDetectedVcsIdWithExtensions(inner, BASE_VCS_CATALOG)).toBe("git"); }); - test("lets an extension adapter outrank Git on a same-root tie when it asks to", () => { - const repo = createTempDir("hunk-apply-colocated-priority-"); + test("uses priority only for colocated roots", () => { + const repo = createTempDir("hunk-apply-colocated-"); mkdirSync(join(repo, ".git")); mkdirSync(join(repo, ".hg")); + expect(resolveDetectedVcsIdWithExtensions(repo, catalogWith([createTestHgAdapter()]))).toBe( + "git", + ); expect( - resolveDetectedVcsIdWithExtensions(repo, [ - createTestHgAdapter(HUNK_CORE_VCS_DETECTION_PRIORITY + 1), - ]), + resolveDetectedVcsIdWithExtensions( + repo, + catalogWith([createTestHgAdapter(HUNK_CORE_VCS_DETECTION_PRIORITY + 1)]), + ), ).toBe("hg"); - }); - - test("still resolves a colocated jj checkout as jj", () => { - const repo = createTempDir("hunk-apply-colocated-jj-"); - mkdirSync(join(repo, ".jj")); - mkdirSync(join(repo, ".git")); - mkdirSync(join(repo, ".hg")); - expect( - resolveDetectedVcsIdWithExtensions(repo, [ - createTestHgAdapter(HUNK_CORE_VCS_DETECTION_PRIORITY + 1), - ]), - ).toBe("jj"); + resolveDetectedVcsIdWithExtensions( + repo, + catalogWith([createTestHgAdapter(HUNK_DEFAULT_VCS_DETECTION_PRIORITY)]), + ), + ).toBe("git"); }); - test("never overrides an explicit vcs a loaded backend owns", () => { + test("never overrides an explicit vcs the complete catalog owns", () => { const repo = createTempDir("hunk-apply-explicit-"); const inner = join(repo, "inner-hg"); mkdirSync(join(repo, ".git")); mkdirSync(join(inner, ".hg"), { recursive: true }); - const adapters = [createTestHgAdapter()]; - - // `vcs = "git"` in config beats the nearer extension checkout. - expect(resolveDetectedVcsIdWithExtensions(inner, adapters, "git")).toBeUndefined(); - // So does naming the extension backend itself. - expect(resolveDetectedVcsIdWithExtensions(inner, adapters, "hg")).toBeUndefined(); - // An id nothing owns already fell back to detection, so detection decides. - expect(resolveDetectedVcsIdWithExtensions(inner, adapters, "bzr")).toBe("hg"); + const catalog = catalogWith([createTestHgAdapter()]); + + expect(resolveDetectedVcsIdWithExtensions(inner, catalog, "git")).toBeUndefined(); + expect(resolveDetectedVcsIdWithExtensions(inner, catalog, "hg")).toBeUndefined(); + expect(resolveDetectedVcsIdWithExtensions(inner, catalog, "bzr")).toBe("hg"); }); }); @@ -575,15 +539,17 @@ describe("resolveSessionVcsId", () => { test("honors a configured id a loaded extension backend owns", () => { // `vcs = "hg"` with a Mercurial extension installed is unambiguous intent. - expect(resolveSessionVcsId("hg", process.cwd(), [hgAdapter])).toEqual({ vcsId: "hg" }); + expect(resolveSessionVcsId("hg", process.cwd(), catalogWith([hgAdapter]))).toEqual({ + vcsId: "hg", + }); }); test("honors a configured id a built-in backend owns", () => { - expect(resolveSessionVcsId("git", process.cwd(), [])).toEqual({ vcsId: "git" }); + expect(resolveSessionVcsId("git", process.cwd(), BASE_VCS_CATALOG)).toEqual({ vcsId: "git" }); }); test("falls back to detection and reports an id nothing owns", () => { - const resolved = resolveSessionVcsId("hg", process.cwd(), []); + const resolved = resolveSessionVcsId("hg", process.cwd(), BASE_VCS_CATALOG); expect(resolved.unknownVcsId).toBe("hg"); // The repo Hunk lives in is a Git checkout, so detection lands there. @@ -591,7 +557,9 @@ describe("resolveSessionVcsId", () => { }); test("leaves an unset id alone", () => { - expect(resolveSessionVcsId(undefined, process.cwd(), [])).toEqual({ vcsId: undefined }); + expect(resolveSessionVcsId(undefined, process.cwd(), BASE_VCS_CATALOG)).toEqual({ + vcsId: undefined, + }); }); test("names the id and the fallback in the notice", () => { diff --git a/src/extensions/apply.ts b/src/extensions/apply.ts index a1718d350..77ba591e7 100644 --- a/src/extensions/apply.ts +++ b/src/extensions/apply.ts @@ -1,8 +1,8 @@ import { BUILT_IN_FILE_LANGUAGE_EXTENSIONS, registerFileLanguage } from "../core/fileLanguage"; import type { StartupNotice } from "../core/startupNotice"; import type { Changeset } from "../core/types"; -import { detectVcs, getDefaultVcsAdapter, isVcsId, resolveVcsAdapters } from "../core/vcs"; -import type { VcsAdapter } from "../core/vcs/types"; +import { detectVcs, extendVcsCatalog, getDefaultVcsAdapter } from "../core/vcs"; +import type { VcsAdapter, VcsCatalog } from "../core/vcs/types"; import { sanitizeTerminalLine } from "../lib/terminalText"; import type { ExtensionContext, @@ -78,13 +78,14 @@ export interface ResolvedExtensionVcsAdapters { */ export function resolveExtensionVcsAdapters( registry: ExtensionRegistry, + baseCatalog: VcsCatalog, ): ResolvedExtensionVcsAdapters { const adapters: VcsAdapter[] = []; const issues: ExtensionApplyIssue[] = []; const claimed = new Set(); for (const { extensionId, adapter } of registry.vcsAdapters) { - if (isVcsId(adapter.id)) { + if (baseCatalog.reservedIds.has(adapter.id)) { issues.push({ extensionId, message: `Skipped VCS adapter "${adapter.id}" from extension ${extensionId} • a built-in backend owns that id`, @@ -263,8 +264,10 @@ export function resolveExtensionCommands(registry: ExtensionRegistry): ResolvedE /** Everything one load pass contributes to the loading pipeline, plus refused registrations. */ export interface AppliedExtensionRegistrations { - /** Extension adapters to thread into `loadAppBootstrap`. */ + /** Accepted user adapters, retained for notices and extension-facing UI state. */ vcsAdapters: VcsAdapter[]; + /** Complete bundled plus user catalog used by loading, reload, and watch. */ + vcsCatalog: VcsCatalog; issues: ExtensionApplyIssue[]; } @@ -277,13 +280,14 @@ export interface AppliedExtensionRegistrations { */ export function applyExtensionRegistrations( result: ExtensionLoadResult | undefined, + baseCatalog: VcsCatalog, ): AppliedExtensionRegistrations { if (!result) { - return { vcsAdapters: [], issues: [] }; + return { vcsAdapters: [], vcsCatalog: baseCatalog, issues: [] }; } const languageIssues = applyExtensionFileLanguages(result.registry); - const vcs = resolveExtensionVcsAdapters(result.registry); + const vcs = resolveExtensionVcsAdapters(result.registry, baseCatalog); // Resolved again where the UI consumes them; consulted here so skipped // duplicate registrations surface through the same notice path as every // other refusal. @@ -293,6 +297,7 @@ export function applyExtensionRegistrations( const commands = resolveExtensionCommands(result.registry); return { vcsAdapters: vcs.adapters, + vcsCatalog: extendVcsCatalog(baseCatalog, vcs.adapters), issues: [ ...languageIssues, ...vcs.issues, @@ -305,8 +310,8 @@ export function applyExtensionRegistrations( } /** Report whether one id names a backend this session actually loaded. */ -function ownsVcsId(adapters: readonly VcsAdapter[], vcsId: string) { - return resolveVcsAdapters(adapters).some((adapter) => adapter.id === vcsId); +function ownsVcsId(catalog: VcsCatalog, vcsId: string) { + return catalog.adapters.some((adapter) => adapter.id === vcsId); } /** @@ -332,19 +337,14 @@ function ownsVcsId(adapters: readonly VcsAdapter[], vcsId: string) { */ export function resolveDetectedVcsIdWithExtensions( cwd: string, - adapters: readonly VcsAdapter[], + catalog: VcsCatalog, explicitVcsId?: string, ): string | undefined { - if (adapters.length === 0) { - // Config already detected across exactly this adapter list. + if (explicitVcsId !== undefined && ownsVcsId(catalog, explicitVcsId)) { return undefined; } - if (explicitVcsId !== undefined && ownsVcsId(adapters, explicitVcsId)) { - return undefined; - } - - return detectVcs(cwd, adapters)?.id; + return detectVcs(cwd, catalog)?.id; } /** The backend one session will load with, plus a configured id nothing owned. */ @@ -375,19 +375,19 @@ export interface ResolvedSessionVcsId { export function resolveSessionVcsId( configuredVcsId: string | undefined, cwd: string, - adapters: readonly VcsAdapter[], + catalog: VcsCatalog, ): ResolvedSessionVcsId { if (!configuredVcsId) { return { vcsId: configuredVcsId }; } - if (ownsVcsId(adapters, configuredVcsId)) { + if (ownsVcsId(catalog, configuredVcsId)) { return { vcsId: configuredVcsId }; } // Same fallback config itself would have produced had it dropped the id. return { - vcsId: detectVcs(cwd)?.id ?? getDefaultVcsAdapter().id, + vcsId: detectVcs(cwd, catalog)?.id ?? getDefaultVcsAdapter(catalog).id, unknownVcsId: configuredVcsId, }; } diff --git a/src/core/vcs/git.test.ts b/src/extensions/default/vcs/git/commands.test.ts similarity index 99% rename from src/core/vcs/git.test.ts rename to src/extensions/default/vcs/git/commands.test.ts index 837b82c33..ecadf4c0b 100644 --- a/src/core/vcs/git.test.ts +++ b/src/extensions/default/vcs/git/commands.test.ts @@ -14,8 +14,8 @@ import { resolveGitMetadata, runGitText, shouldSkipLargeTrackedDiff, -} from "./git"; -import type { VcsDiffCommandInput } from "../types"; +} from "./commands"; +import type { ExtensionVcsDiffInput as VcsDiffCommandInput } from "hunkdiff/extension"; const tempDirs: string[] = []; diff --git a/src/core/vcs/git.ts b/src/extensions/default/vcs/git/commands.ts similarity index 98% rename from src/core/vcs/git.ts rename to src/extensions/default/vcs/git/commands.ts index 38f2fcc88..2a2bf69d5 100644 --- a/src/core/vcs/git.ts +++ b/src/extensions/default/vcs/git/commands.ts @@ -5,17 +5,17 @@ import { type ExtensionVcsDiffInput, type ExtensionVcsShowInput, type ExtensionVcsStashShowInput, -} from "../../extension-api/types"; -import { LARGE_DIFF_FILE_MAX_BYTES, LARGE_DIFF_FILE_MAX_LINES } from "./largeFile"; -import { escapeUntrackedPatchPath } from "../patch/normalize"; -import { normalizePathForOS } from "../../lib/osPath"; +} from "hunkdiff/extension"; +import { LARGE_DIFF_FILE_MAX_BYTES, LARGE_DIFF_FILE_MAX_LINES } from "../../../../lib/largeFile"; +import { escapeUntrackedPatchPath } from "../../../../lib/patchPath"; +import { normalizePathForOS } from "../../../../lib/osPath"; /** * Every Git command Hunk runs, and the failures they translate into. * * This is the implementation layer behind the bundled Git backend - * (`src/extensions/default/vcs/git/`), so nothing here reaches into the diff - * engine or the adapter registry — user-facing failures are raised as the + * (`src/extensions/default/vcs/git/`), so nothing here reaches into core, the + * diff engine, or the adapter registry — user-facing failures are raised as the * published `HunkExtensionUserError`, which is exactly what a third-party * backend would throw. */ diff --git a/src/extensions/default/vcs/git/index.test.ts b/src/extensions/default/vcs/git/index.test.ts index ba38b1636..cbc50122f 100644 --- a/src/extensions/default/vcs/git/index.test.ts +++ b/src/extensions/default/vcs/git/index.test.ts @@ -8,7 +8,7 @@ import type { ExtensionVcsOperations, ExtensionVcsShowInput, ExtensionVcsStashShowInput, -} from "../../../../extension-api/types"; +} from "hunkdiff/extension"; // The adapter is written against the published contract, so the tests read it // through that contract too — including the capabilities Git is the only diff --git a/src/extensions/default/vcs/git/index.ts b/src/extensions/default/vcs/git/index.ts index 4ba9b63fb..668998886 100644 --- a/src/extensions/default/vcs/git/index.ts +++ b/src/extensions/default/vcs/git/index.ts @@ -20,11 +20,11 @@ import { shouldSkipLargeTrackedDiff, type GitBackedInput, type GitDiffEndpoints, -} from "../../../../core/vcs/git"; -import { gitEndpointSourceSpec, readGitFileSource } from "../../../../core/vcs/gitSource"; -import { inspectLargeUntrackedFile } from "../../../../core/vcs/largeFile"; +} from "./commands"; +import { gitEndpointSourceSpec, readGitFileSource } from "./source"; +import { inspectLargeUntrackedFile } from "../../../../lib/largeFile"; import { - HUNK_CORE_VCS_DETECTION_PRIORITY, + HUNK_VCS_DETECTION_BASELINE_PRIORITY, type ExtensionVcsAdapter, type ExtensionVcsDiffInput, type ExtensionVcsDirectoryTreeWatchTarget, @@ -32,7 +32,7 @@ import { type ExtensionVcsFileSourceReader, type ExtensionVcsWatchPlan, type HunkExtensionAPI, -} from "../../../../extension-api/types"; +} from "hunkdiff/extension"; /** * Hunk's Git backend, as a bundled extension. @@ -40,9 +40,9 @@ import { * Git is the backend that exercises every integration point there is — exact * file sources, skipped-too-large placeholders, untracked files, watch plans, * rich failures — so it is deliberately written the way a third-party backend - * would be: it sees only the published `hunkdiff/extension` contract plus its - * own implementation helpers in `src/core/vcs/git.ts`, `src/core/vcs/gitSource.ts`, and - * `src/core/vcs/largeFile.ts`. Nothing here reaches into the diff engine or the + * would be: it sees only the published `hunkdiff/extension` contract plus + * implementation helpers owned by this extension directory and generic `src/lib` + * utilities. Nothing here reaches into core, the diff engine, or the * adapter registry. If something Git needs cannot be said in these types, the * published contract is missing it, and that is the point of shipping it this * way. @@ -309,167 +309,183 @@ function buildGitWatchPlan( * Registered at the baseline detection priority: it is what every other * backend, bundled or installed, positions itself against. */ -export const GitVcsAdapter = { - id: "git", - name: "Git", - detect: detectGitRepo, - detectionPriority: HUNK_CORE_VCS_DETECTION_PRIORITY, - operations: { - "working-tree-diff": { - async load(input, { cwd, gitExecutable = "git" }) { - const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); - const repoName = basename(repoRoot); - const title = input.staged - ? `${repoName} staged changes` - : input.range - ? `${repoName} ${input.range}` - : `${repoName} working tree`; - // Ask for stats before the patch so files too large to render can be - // excluded from the diff instead of generating output nobody reads. - const largeTrackedFiles = parseGitNumstat( - runGitText({ input, args: buildGitDiffNumstatArgs(input), cwd, gitExecutable }), - ).filter((file) => shouldSkipLargeTrackedDiff(file, repoRoot)); - const colorMoved = resolveGitColorMovedOptions(input, { cwd, gitExecutable }); - const sourceCapability = createGitDiffSourceCapability(input, repoRoot, cwd, gitExecutable); +export interface GitVcsAdapterOptions { + gitExecutable?: string; +} - return { - repoRoot, - sourceLabel: repoRoot, - title, - patchText: runGitText({ +/** Create a Git adapter with provider-owned process dependencies. */ +export function createGitVcsAdapter({ + gitExecutable = "git", +}: Readonly = {}) { + return { + id: "git", + name: "Git", + detect: detectGitRepo, + detectionPriority: HUNK_VCS_DETECTION_BASELINE_PRIORITY, + operations: { + "working-tree-diff": { + async load(input, { cwd }) { + const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); + const repoName = basename(repoRoot); + const title = input.staged + ? `${repoName} staged changes` + : input.range + ? `${repoName} ${input.range}` + : `${repoName} working tree`; + // Ask for stats before the patch so files too large to render can be + // excluded from the diff instead of generating output nobody reads. + const largeTrackedFiles = parseGitNumstat( + runGitText({ input, args: buildGitDiffNumstatArgs(input), cwd, gitExecutable }), + ).filter((file) => shouldSkipLargeTrackedDiff(file, repoRoot)); + const colorMoved = resolveGitColorMovedOptions(input, { cwd, gitExecutable }); + const sourceCapability = createGitDiffSourceCapability( input, - args: buildGitDiffArgs( + repoRoot, + cwd, + gitExecutable, + ); + + return { + repoRoot, + sourceLabel: repoRoot, + title, + patchText: runGitText({ input, - largeTrackedFiles.map((file) => file.path), - colorMoved, - ), + args: buildGitDiffArgs( + input, + largeTrackedFiles.map((file) => file.path), + colorMoved, + ), + cwd, + gitExecutable, + }), + ...sourceCapability, + extraFiles: [ + ...largeTrackedFiles.map( + (file): ExtensionVcsExtraFile => ({ + kind: "skipped", + path: file.path, + reason: "too-large", + changeType: "change", + stats: { additions: file.additions, deletions: file.deletions }, + }), + ), + ...listGitUntrackedFiles(input, { cwd, repoRoot, gitExecutable }).map((filePath) => + buildUntrackedExtraFile(input, filePath, repoRoot, gitExecutable), + ), + ], + }; + }, + watchPlan(input, { cwd }) { + return buildGitWatchPlan(input, cwd, gitExecutable); + }, + watchSignature(input, { cwd }) { + const trackedPatch = runGitText({ + input, + args: buildGitDiffArgs(input), cwd, gitExecutable, - }), - ...sourceCapability, - extraFiles: [ - ...largeTrackedFiles.map( - (file): ExtensionVcsExtraFile => ({ - kind: "skipped", - path: file.path, - reason: "too-large", - changeType: "change", - stats: { additions: file.additions, deletions: file.deletions }, - }), - ), - ...listGitUntrackedFiles(input, { cwd, repoRoot, gitExecutable }).map((filePath) => - buildUntrackedExtraFile(input, filePath, repoRoot, gitExecutable), - ), - ], - }; - }, - watchPlan(input, { cwd, gitExecutable = "git" }) { - return buildGitWatchPlan(input, cwd, gitExecutable); - }, - watchSignature(input, { cwd, gitExecutable = "git" }) { - const trackedPatch = runGitText({ - input, - args: buildGitDiffArgs(input), - cwd, - gitExecutable, - preventOptionalLocks: true, - }); - const repoRoot = resolveGitRepoRoot(input, { - cwd, - gitExecutable, - preventOptionalLocks: true, - }); - const untrackedSignatures = listGitUntrackedFiles(input, { - cwd, - repoRoot, - gitExecutable, - preventOptionalLocks: true, - }).map((filePath) => `untracked:${statSignature(join(repoRoot, filePath))}`); - return [trackedPatch, ...untrackedSignatures].join("\n---\n"); + preventOptionalLocks: true, + }); + const repoRoot = resolveGitRepoRoot(input, { + cwd, + gitExecutable, + preventOptionalLocks: true, + }); + const untrackedSignatures = listGitUntrackedFiles(input, { + cwd, + repoRoot, + gitExecutable, + preventOptionalLocks: true, + }).map((filePath) => `untracked:${statSignature(join(repoRoot, filePath))}`); + return [trackedPatch, ...untrackedSignatures].join("\n---\n"); + }, }, - }, - "revision-show": { - async load(input, { cwd, gitExecutable = "git" }) { - const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); - const repoName = basename(repoRoot); - const sourceCapability = createGitRevisionSourceCapability( - input, - input.ref ?? "HEAD", - repoRoot, - gitExecutable, - ); - - return { - repoRoot, - sourceLabel: repoRoot, - title: input.ref ? `${repoName} show ${input.ref}` : `${repoName} show HEAD`, - patchText: runGitText({ + "revision-show": { + async load(input, { cwd }) { + const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); + const repoName = basename(repoRoot); + const sourceCapability = createGitRevisionSourceCapability( input, - args: buildGitShowArgs( + input.ref ?? "HEAD", + repoRoot, + gitExecutable, + ); + + return { + repoRoot, + sourceLabel: repoRoot, + title: input.ref ? `${repoName} show ${input.ref}` : `${repoName} show HEAD`, + patchText: runGitText({ input, - resolveGitColorMovedOptions(input, { cwd, gitExecutable }), - ), + args: buildGitShowArgs( + input, + resolveGitColorMovedOptions(input, { cwd, gitExecutable }), + ), + cwd, + gitExecutable, + }), + ...sourceCapability, + }; + }, + watchPlan(input, { cwd }) { + return buildGitWatchPlan(input, cwd, gitExecutable); + }, + watchSignature(input, { cwd }) { + return runGitText({ + input, + args: buildGitShowArgs(input), cwd, gitExecutable, - }), - ...sourceCapability, - }; - }, - watchPlan(input, { cwd, gitExecutable = "git" }) { - return buildGitWatchPlan(input, cwd, gitExecutable); - }, - watchSignature(input, { cwd, gitExecutable = "git" }) { - return runGitText({ - input, - args: buildGitShowArgs(input), - cwd, - gitExecutable, - preventOptionalLocks: true, - }); + preventOptionalLocks: true, + }); + }, }, - }, - "stash-show": { - async load(input, { cwd, gitExecutable = "git" }) { - const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); - const repoName = basename(repoRoot); - const sourceCapability = createGitRevisionSourceCapability( - input, - input.ref ?? "stash@{0}", - repoRoot, - gitExecutable, - ); - - return { - repoRoot, - sourceLabel: repoRoot, - title: input.ref ? `${repoName} stash ${input.ref}` : `${repoName} stash`, - patchText: runGitText({ + "stash-show": { + async load(input, { cwd }) { + const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); + const repoName = basename(repoRoot); + const sourceCapability = createGitRevisionSourceCapability( input, - args: buildGitStashShowArgs( + input.ref ?? "stash@{0}", + repoRoot, + gitExecutable, + ); + + return { + repoRoot, + sourceLabel: repoRoot, + title: input.ref ? `${repoName} stash ${input.ref}` : `${repoName} stash`, + patchText: runGitText({ input, - resolveGitColorMovedOptions(input, { cwd, gitExecutable }), - ), + args: buildGitStashShowArgs( + input, + resolveGitColorMovedOptions(input, { cwd, gitExecutable }), + ), + cwd, + gitExecutable, + }), + ...sourceCapability, + }; + }, + watchPlan(input, { cwd }) { + return buildGitWatchPlan(input, cwd, gitExecutable); + }, + watchSignature(input, { cwd }) { + return runGitText({ + input, + args: buildGitStashShowArgs(input), cwd, gitExecutable, - }), - ...sourceCapability, - }; - }, - watchPlan(input, { cwd, gitExecutable = "git" }) { - return buildGitWatchPlan(input, cwd, gitExecutable); - }, - watchSignature(input, { cwd, gitExecutable = "git" }) { - return runGitText({ - input, - args: buildGitStashShowArgs(input), - cwd, - gitExecutable, - preventOptionalLocks: true, - }); + preventOptionalLocks: true, + }); + }, }, }, - }, -} satisfies ExtensionVcsAdapter; + } satisfies ExtensionVcsAdapter; +} + +export const GitVcsAdapter = createGitVcsAdapter(); export default function (hunk: HunkExtensionAPI) { hunk.registerVcsAdapter(GitVcsAdapter); diff --git a/src/extensions/default/vcs/git/source.test.ts b/src/extensions/default/vcs/git/source.test.ts new file mode 100644 index 000000000..f38b00d2a --- /dev/null +++ b/src/extensions/default/vcs/git/source.test.ts @@ -0,0 +1,251 @@ +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"; + +const tempDirs: string[] = []; + +function createTempDir(prefix: string) { + const dir = mkdtempSync(join(tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +function git(cwd: string, ...cmd: string[]) { + const proc = Bun.spawnSync(["git", ...cmd], { + cwd, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + if (proc.exitCode !== 0) { + const stderr = Buffer.from(proc.stderr).toString("utf8"); + throw new Error(stderr.trim() || `git ${cmd.join(" ")} failed`); + } + return Buffer.from(proc.stdout).toString("utf8"); +} + +function createTempRepo(prefix: string) { + const dir = createTempDir(prefix); + git(dir, "init"); + git(dir, "config", "user.name", "Test User"); + git(dir, "config", "user.email", "test@example.com"); + git(dir, "config", "commit.gpgSign", "false"); + return dir; +} + +/** Capture console.error calls while exercising diagnostic paths. */ +async function captureConsoleErrors(fn: () => Promise) { + const originalConsoleError = console.error; + const loggedErrors: unknown[][] = []; + console.error = (...args: unknown[]) => loggedErrors.push(args); + try { + await fn(); + } finally { + console.error = originalConsoleError; + } + return loggedErrors; +} + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("gitEndpointSourceSpec", () => { + test("maps every endpoint kind to a source spec", () => { + expect(gitEndpointSourceSpec({ kind: "none" }, "/repo", "a.ts")).toEqual({ kind: "none" }); + expect(gitEndpointSourceSpec({ kind: "git-ref", ref: "HEAD" }, "/repo", "a.ts")).toEqual({ + kind: "git-blob", + repoRoot: "/repo", + ref: "HEAD", + path: "a.ts", + }); + expect(gitEndpointSourceSpec({ kind: "index" }, "/repo", "a.ts")).toEqual({ + kind: "git-index", + repoRoot: "/repo", + path: "a.ts", + }); + expect(gitEndpointSourceSpec({ kind: "worktree" }, "/repo", "a.ts")).toEqual({ + kind: "fs", + absolutePath: join("/repo", "a.ts"), + }); + }); +}); + +describe("Git source reading", () => { + test("reads git blob contents for both sides via `git show`", async () => { + const repoRoot = createTempRepo("hunk-source-git-"); + const filePath = "note.txt"; + + writeFileSync(join(repoRoot, filePath), "first revision\n"); + git(repoRoot, "add", "."); + git(repoRoot, "commit", "-m", "first"); + writeFileSync(join(repoRoot, filePath), "second revision\n"); + git(repoRoot, "add", "."); + git(repoRoot, "commit", "-m", "second"); + + expect( + await readGitFileSource({ kind: "git-blob", repoRoot, ref: "HEAD~1", path: filePath }), + ).toBe("first revision\n"); + expect( + await readGitFileSource({ kind: "git-blob", repoRoot, ref: "HEAD", path: filePath }), + ).toBe("second revision\n"); + }); + + test("reads git index contents through an explicit index spec", async () => { + const repoRoot = createTempRepo("hunk-source-git-index-"); + const filePath = "note.txt"; + + writeFileSync(join(repoRoot, filePath), "committed\n"); + git(repoRoot, "add", "."); + git(repoRoot, "commit", "-m", "first"); + writeFileSync(join(repoRoot, filePath), "staged\n"); + git(repoRoot, "add", filePath); + writeFileSync(join(repoRoot, filePath), "working tree\n"); + + expect(await readGitFileSource({ kind: "git-index", repoRoot, path: filePath })).toBe( + "staged\n", + ); + expect(await readGitFileSource({ kind: "fs", absolutePath: join(repoRoot, filePath) })).toBe( + "working tree\n", + ); + }); + + test("reports git blob and index source reads that exceed the configured byte cap", async () => { + const repoRoot = createTempRepo("hunk-source-git-large-"); + const filePath = "note.txt"; + + writeFileSync(join(repoRoot, filePath), "committed source\n"); + git(repoRoot, "add", filePath); + git(repoRoot, "commit", "-m", "first"); + writeFileSync(join(repoRoot, filePath), "staged source\n"); + git(repoRoot, "add", filePath); + + await expect( + readGitFileSource( + { kind: "git-blob", repoRoot, ref: "HEAD", path: filePath }, + { maxSourceBytes: 5 }, + ), + ).resolves.toEqual({ kind: "too-large", maxBytes: 5 }); + await expect( + readGitFileSource({ kind: "git-index", repoRoot, path: filePath }, { maxSourceBytes: 5 }), + ).resolves.toEqual({ kind: "too-large", maxBytes: 5 }); + }); + + test("treats oversized git stderr as a generic source failure", async () => { + const originalSpawn = Bun.spawn; + const mutableBun = Bun as unknown as { spawn: typeof Bun.spawn }; + + mutableBun.spawn = (() => + originalSpawn( + [ + process.execPath, + "--eval", + "process.stdout.write('small source\\n'); process.stderr.write('x'.repeat(70000));", + ], + { + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + }, + )) as typeof Bun.spawn; + + try { + const loggedErrors = await captureConsoleErrors(async () => { + await expect( + readGitFileSource({ + kind: "git-blob", + repoRoot: process.cwd(), + ref: "HEAD", + path: "note.txt", + }), + ).resolves.toBeNull(); + }); + + expect(String(loggedErrors[0]?.[0])).toContain("failed to collect Git source"); + expect(String(loggedErrors[0]?.[1])).toContain("diagnostics exceeded"); + } finally { + mutableBun.spawn = originalSpawn; + } + }); + + test("passes custom git executable through async git source reads", async () => { + const originalSpawn = Bun.spawn; + const mutableBun = Bun as unknown as { spawn: typeof Bun.spawn }; + const spawnCalls: string[][] = []; + + mutableBun.spawn = ((cmds: string[]) => { + spawnCalls.push(cmds); + return originalSpawn( + [ + process.execPath, + "--eval", + `process.stdout.write(${JSON.stringify(`read:${cmds[2]}\n`)})`, + ], + { + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + }, + ); + }) as typeof Bun.spawn; + + try { + expect( + await readGitFileSource( + { kind: "git-blob", repoRoot: process.cwd(), ref: "HEAD", path: "note.txt" }, + { gitExecutable: "custom-git" }, + ), + ).toBe("read:HEAD:note.txt\n"); + expect( + await readGitFileSource( + { kind: "git-index", repoRoot: process.cwd(), path: "note.txt" }, + { gitExecutable: "custom-git" }, + ), + ).toBe("read::note.txt\n"); + } finally { + mutableBun.spawn = originalSpawn; + } + + expect(spawnCalls).toEqual([ + ["custom-git", "show", "HEAD:note.txt"], + ["custom-git", "show", ":note.txt"], + ]); + }); + + test("returns null when a git blob cannot be resolved", async () => { + const repoRoot = createTempRepo("hunk-source-git-missing-"); + writeFileSync(join(repoRoot, "tracked.txt"), "x\n"); + git(repoRoot, "add", "."); + git(repoRoot, "commit", "-m", "first"); + + const loggedErrors = await captureConsoleErrors(async () => { + expect( + await readGitFileSource({ + kind: "git-blob", + repoRoot, + ref: "HEAD", + path: "missing-from-history.txt", + }), + ).toBeNull(); + }); + expect(loggedErrors).toHaveLength(0); + }); + + test("logs unexpected git source failures with object context", async () => { + const repoRoot = createTempDir("hunk-source-git-not-repo-"); + + const loggedErrors = await captureConsoleErrors(async () => { + expect( + await readGitFileSource({ kind: "git-blob", repoRoot, ref: "HEAD", path: "note.txt" }), + ).toBeNull(); + }); + + expect(loggedErrors).toHaveLength(1); + expect(String(loggedErrors[0]?.[0])).toContain("HEAD:note.txt"); + expect(String(loggedErrors[0]?.[0])).toContain(repoRoot); + }); +}); diff --git a/src/core/vcs/gitSource.ts b/src/extensions/default/vcs/git/source.ts similarity index 67% rename from src/core/vcs/gitSource.ts rename to src/extensions/default/vcs/git/source.ts index fabc16257..9e4bcb604 100644 --- a/src/core/vcs/gitSource.ts +++ b/src/extensions/default/vcs/git/source.ts @@ -1,25 +1,28 @@ import { join } from "node:path"; +import type { + ExtensionVcsFileSourceResult, + ExtensionVcsFileSourceTooLarge, +} from "hunkdiff/extension"; import { DEFAULT_SOURCE_TEXT_MAX_BYTES, - SourceTextTooLargeError, logSourceDiagnostic, - readFileSourceSpec, + readFileTextWithLimit, readStreamTextWithLimit, - type FileSourceSpec, -} from "../fileSource"; -import type { GitDiffEndpoint } from "./git"; +} from "../../../../lib/sourceText"; +import type { GitDiffEndpoint } from "./commands"; -/** - * Reading a reviewed file's full contents out of Git. - * - * Git can name the exact bytes on each side of a diff — a blob at a commit, the - * staged entry, the file on disk — which is what lets Hunk expand context and - * highlight against the real file instead of against the patch. Everything here - * answers one question: given a resolved source spec, what is that text? - */ +/** A provider-local signal converted to the public structural result at this boundary. */ +class GitSourceTooLargeError extends Error { + constructor(readonly maxBytes: number) { + super(`Source text exceeds ${maxBytes} bytes.`); + this.name = "GitSourceTooLargeError"; + } +} +/** Source locations Git can resolve without consulting Hunk core internals. */ export type GitFileSourceSpec = - | FileSourceSpec + | { kind: "none" } + | { kind: "fs"; absolutePath: string } | { kind: "git-blob"; repoRoot: string; ref: string; path: string } | { kind: "git-index"; repoRoot: string; path: string }; @@ -58,13 +61,18 @@ function isExpectedMissingGitSource(stderr: string) { ].some((fragment) => normalized.includes(fragment)); } +/** Represent an exceeded resource limit through the public extension contract. */ +function tooLarge(maxBytes: number): ExtensionVcsFileSourceTooLarge { + return { kind: "too-large", maxBytes }; +} + /** Read a blob-like Git object spec such as `HEAD:path` or `:path`. */ async function readGitObjectSpec( repoRoot: string, objectName: string, - gitExecutable = "git", + gitExecutable: string, maxSourceBytes: number, -): Promise { +): Promise { let proc: Bun.ReadableSubprocess; try { @@ -83,19 +91,24 @@ async function readGitObjectSpec( try { output = await Promise.all([ proc.exited, - readStreamTextWithLimit(proc.stdout, maxSourceBytes, () => proc.kill()), + readStreamTextWithLimit( + proc.stdout, + maxSourceBytes, + () => proc.kill(), + (limit) => new GitSourceTooLargeError(limit), + ), readStreamTextWithLimit( proc.stderr, 64 * 1024, undefined, - (maxBytes) => new Error(`Git source diagnostics exceeded ${maxBytes} bytes.`), + (limit) => new Error(`Git source diagnostics exceeded ${limit} bytes.`), ), ]); } catch (error) { - if (error instanceof SourceTextTooLargeError) { + if (error instanceof GitSourceTooLargeError) { proc.kill(); await proc.exited.catch(() => undefined); - throw error; + return tooLarge(error.maxBytes); } logSourceDiagnostic(`failed to collect Git source ${objectName}`, error); @@ -103,7 +116,6 @@ async function readGitObjectSpec( } const [exitCode, stdout, stderr] = output; - if (exitCode !== 0) { if (!isExpectedMissingGitSource(stderr)) { logSourceDiagnostic(`failed to read Git source ${objectName} in ${repoRoot}`, stderr); @@ -114,22 +126,19 @@ async function readGitObjectSpec( return stdout; } -/** - * Read the full text one resolved Git source spec names. - * - * Resolves to `null` rather than rejecting whenever a side simply is not there - * — the old side of an added file, a path the ref never contained — so callers - * can treat "no source" as an ordinary answer. Only a source too large to read - * safely rejects. - */ +/** Read the full text one resolved Git source spec names. */ export function readGitFileSource( spec: GitFileSourceSpec, { gitExecutable = "git", maxSourceBytes = DEFAULT_SOURCE_TEXT_MAX_BYTES, }: Readonly = {}, -): Promise { +): Promise { switch (spec.kind) { + case "none": + return Promise.resolve(null); + case "fs": + return readFileTextWithLimit(spec.absolutePath, maxSourceBytes); case "git-index": return readGitObjectSpec(spec.repoRoot, `:${spec.path}`, gitExecutable, maxSourceBytes); case "git-blob": @@ -139,7 +148,5 @@ export function readGitFileSource( gitExecutable, maxSourceBytes, ); - default: - return readFileSourceSpec(spec, { maxSourceBytes }); } } diff --git a/src/extensions/default/vcs/index.ts b/src/extensions/default/vcs/index.ts index 5d39981a0..7cdc221ce 100644 --- a/src/extensions/default/vcs/index.ts +++ b/src/extensions/default/vcs/index.ts @@ -9,7 +9,6 @@ import { type ExtensionMetadata, type ExtensionRegistry, } from "../../types"; -import type { VcsAdapter } from "../../../core/vcs/types"; /** * The bundled extension tier. @@ -36,8 +35,8 @@ import type { VcsAdapter } from "../../../core/vcs/types"; * rather than a crash — even though these factories are Hunk's own and that * path should be unreachable. * - * VCS backends are the only registration kind this tier uses today, and the - * only one consumed from here (`src/core/vcs` reads `getBundledVcsAdapters`). + * VCS backends are the only registration kind this tier uses today. The app + * composition root reads `getBundledVcsAdapters` and builds the core catalog. * A bundled extension that registered a theme or a changeset transform would * also have to be threaded through `applyExtensionRegistrations`, which today * only sees the user-extension load result. @@ -52,7 +51,7 @@ interface BundledExtensionDefinition { * Every bundled extension, in load order. * * Load order only breaks ties: detection order comes from each adapter's - * `detectionPriority`, assembled in `src/core/vcs`. + * `detectionPriority`, assembled by the app-owned catalog. */ const BUNDLED_EXTENSIONS: readonly BundledExtensionDefinition[] = [ { id: "jj", factory: jjExtension }, @@ -104,6 +103,6 @@ export function loadBundledExtensions(): BundledExtensionLoad { } /** Return the VCS backends the bundled tier registered, in registration order. */ -export function getBundledVcsAdapters(): readonly VcsAdapter[] { +export function getBundledVcsAdapters() { return loadBundledExtensions().registry.vcsAdapters.map((entry) => entry.adapter); } diff --git a/src/core/vcs/jujutsu.test.ts b/src/extensions/default/vcs/jujutsu/commands.test.ts similarity index 95% rename from src/core/vcs/jujutsu.test.ts rename to src/extensions/default/vcs/jujutsu/commands.test.ts index 9028a12c0..acd27c962 100644 --- a/src/core/vcs/jujutsu.test.ts +++ b/src/extensions/default/vcs/jujutsu/commands.test.ts @@ -2,8 +2,8 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { buildJjDiffArgs, runJjText } from "./jujutsu"; -import type { VcsDiffCommandInput } from "../types"; +import { buildJjDiffArgs, runJjText } from "./commands"; +import type { ExtensionVcsDiffInput as VcsDiffCommandInput } from "hunkdiff/extension"; const tempDirs: string[] = []; // Windows subprocess setup can exceed Bun's default 5s timeout while generating enough jj changes. @@ -20,7 +20,7 @@ function cleanupTempDirs() { /** Build one working-tree review input for the jj command helpers. */ function diffInput(overrides: Partial = {}): VcsDiffCommandInput { - return { kind: "vcs", staged: false, options: { mode: "auto", vcs: "jj" }, ...overrides }; + return { kind: "vcs", staged: false, options: {}, ...overrides }; } function createTempDir(prefix: string) { diff --git a/src/core/vcs/jujutsu.ts b/src/extensions/default/vcs/jujutsu/commands.ts similarity index 98% rename from src/core/vcs/jujutsu.ts rename to src/extensions/default/vcs/jujutsu/commands.ts index 145100718..945af555c 100644 --- a/src/core/vcs/jujutsu.ts +++ b/src/extensions/default/vcs/jujutsu/commands.ts @@ -2,8 +2,8 @@ import { HunkExtensionUserError, type ExtensionVcsDiffInput, type ExtensionVcsShowInput, -} from "../../extension-api/types"; -import { normalizePathForOS } from "../../lib/osPath"; +} from "hunkdiff/extension"; +import { normalizePathForOS } from "../../../../lib/osPath"; export type JjBackedInput = ExtensionVcsDiffInput | ExtensionVcsShowInput; diff --git a/src/extensions/default/vcs/jujutsu/index.test.ts b/src/extensions/default/vcs/jujutsu/index.test.ts index 6b9ede118..e1bea9792 100644 --- a/src/extensions/default/vcs/jujutsu/index.test.ts +++ b/src/extensions/default/vcs/jujutsu/index.test.ts @@ -7,7 +7,7 @@ import type { ExtensionVcsOperations, ExtensionVcsShowInput, ExtensionVcsDiffInput, -} from "../../../../extension-api/types"; +} from "hunkdiff/extension"; // The adapter is written against the published contract, so the tests read it // through that contract too — including the operations an adapter may omit. diff --git a/src/extensions/default/vcs/jujutsu/index.ts b/src/extensions/default/vcs/jujutsu/index.ts index dfa08fe11..c872c5e5c 100644 --- a/src/extensions/default/vcs/jujutsu/index.ts +++ b/src/extensions/default/vcs/jujutsu/index.ts @@ -6,20 +6,20 @@ import { createJjStagedError, resolveJjRepoRoot, runJjText, -} from "../../../../core/vcs/jujutsu"; +} from "./commands"; import { - HUNK_CORE_VCS_DETECTION_PRIORITY, + HUNK_VCS_DETECTION_BASELINE_PRIORITY, type ExtensionVcsAdapter, type HunkExtensionAPI, -} from "../../../../extension-api/types"; +} from "hunkdiff/extension"; /** * Hunk's Jujutsu backend, as a bundled extension. * * This file is written the way a third-party VCS extension would be: it sees - * only the published `hunkdiff/extension` contract plus its own implementation - * helpers in `src/core/vcs/jujutsu.ts`. If something here cannot be said in those types, - * the contract is missing something. + * only the published `hunkdiff/extension` contract plus implementation helpers + * owned by this extension directory. If something here cannot be said in those + * types, the contract is missing something. */ /** Return the last path segment for review titles. */ @@ -49,7 +49,7 @@ export const JjVcsAdapter = { detect: detectJjRepo, // Above Git: a colocated jj repository carries a `.git` directory too, and // reviewing it as plain Git would show the wrong working copy. - detectionPriority: HUNK_CORE_VCS_DETECTION_PRIORITY + 200, + detectionPriority: HUNK_VCS_DETECTION_BASELINE_PRIORITY + 200, operations: { "working-tree-diff": { async load(input, { cwd }) { diff --git a/src/core/vcs/sapling.test.ts b/src/extensions/default/vcs/sapling/commands.test.ts similarity index 92% rename from src/core/vcs/sapling.test.ts rename to src/extensions/default/vcs/sapling/commands.test.ts index 57cf5ad64..fe84c0cc8 100644 --- a/src/core/vcs/sapling.test.ts +++ b/src/extensions/default/vcs/sapling/commands.test.ts @@ -2,8 +2,8 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, realpathSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { buildSlDiffArgs, runSlText } from "./sapling"; -import type { VcsDiffCommandInput } from "../types"; +import { buildSlDiffArgs, runSlText } from "./commands"; +import type { ExtensionVcsDiffInput as VcsDiffCommandInput } from "hunkdiff/extension"; const slAvailable = (() => { try { @@ -28,7 +28,7 @@ function cleanupTempDirs() { /** Build one working-tree review input for the sl command helpers. */ function diffInput(overrides: Partial = {}): VcsDiffCommandInput { - return { kind: "vcs", staged: false, options: { mode: "auto", vcs: "sl" }, ...overrides }; + return { kind: "vcs", staged: false, options: {}, ...overrides }; } function createTempDir(prefix: string) { diff --git a/src/core/vcs/sapling.ts b/src/extensions/default/vcs/sapling/commands.ts similarity index 98% rename from src/core/vcs/sapling.ts rename to src/extensions/default/vcs/sapling/commands.ts index a5c26a609..3d6df5cd2 100644 --- a/src/core/vcs/sapling.ts +++ b/src/extensions/default/vcs/sapling/commands.ts @@ -4,8 +4,8 @@ import { HunkExtensionUserError, type ExtensionVcsDiffInput, type ExtensionVcsShowInput, -} from "../../extension-api/types"; -import { normalizePathForOS } from "../../lib/osPath"; +} from "hunkdiff/extension"; +import { normalizePathForOS } from "../../../../lib/osPath"; export type SlBackedInput = ExtensionVcsDiffInput | ExtensionVcsShowInput; diff --git a/src/extensions/default/vcs/sapling/index.test.ts b/src/extensions/default/vcs/sapling/index.test.ts index 0e03cb6d9..1ae2e3612 100644 --- a/src/extensions/default/vcs/sapling/index.test.ts +++ b/src/extensions/default/vcs/sapling/index.test.ts @@ -7,7 +7,7 @@ import type { ExtensionVcsOperations, ExtensionVcsShowInput, ExtensionVcsDiffInput, -} from "../../../../extension-api/types"; +} from "hunkdiff/extension"; // The adapter is written against the published contract, so the tests read it // through that contract too — including the operations an adapter may omit. diff --git a/src/extensions/default/vcs/sapling/index.ts b/src/extensions/default/vcs/sapling/index.ts index 9eefbe113..764e98c69 100644 --- a/src/extensions/default/vcs/sapling/index.ts +++ b/src/extensions/default/vcs/sapling/index.ts @@ -7,18 +7,18 @@ import { listSlUntrackedFiles, resolveSlRepoRoot, runSlText, -} from "../../../../core/vcs/sapling"; +} from "./commands"; import { - HUNK_CORE_VCS_DETECTION_PRIORITY, + HUNK_VCS_DETECTION_BASELINE_PRIORITY, type ExtensionVcsAdapter, type HunkExtensionAPI, -} from "../../../../extension-api/types"; +} from "hunkdiff/extension"; /** * Hunk's Sapling backend, as a bundled extension. * - * Like the Jujutsu one, this file sees only the published contract plus its own - * helpers in `src/core/vcs/sapling.ts`. + * Like the Jujutsu one, this file sees only the published contract plus helpers + * owned by this extension directory. */ /** Return the last path segment for review titles. */ @@ -72,7 +72,7 @@ export const SaplingVcsAdapter = { detect: detectSlRepo, // Above Git for the same reason Jujutsu is: `sl init --git` leaves Git // metadata behind, and the Sapling working copy is the one under review. - detectionPriority: HUNK_CORE_VCS_DETECTION_PRIORITY + 100, + detectionPriority: HUNK_VCS_DETECTION_BASELINE_PRIORITY + 100, operations: { "working-tree-diff": { async load(input, { cwd }) { diff --git a/src/extensions/discovery.test.ts b/src/extensions/discovery.test.ts index 7ff8f5e41..fb36d077c 100644 --- a/src/extensions/discovery.test.ts +++ b/src/extensions/discovery.test.ts @@ -104,6 +104,21 @@ describe("extension discovery", () => { expect(candidates).toEqual([{ id: "dual", path: typescriptIndex, origin: "flag" }]); }); + test("bootstraps repo-local extensions from .hunk without a bundled VCS marker", () => { + const repo = createTempDir("hunk-ext-provider-neutral-repo-"); + const nested = join(repo, "src", "nested"); + mkdirSync(nested, { recursive: true }); + const repoPath = writeExtensionFile(repo, ".hunk", "extensions", "custom-vcs.ts"); + + const candidates = discoverExtensions({ + cwd: nested, + globalExtensionsDir: undefined, + env: {}, + }); + + expect(candidates).toEqual([{ id: "custom-vcs", path: repoPath, origin: "repo" }]); + }); + test("orders flag, user config, global, then repo-local sources", () => { const repo = createRepo("hunk-ext-repo-"); const globalDir = join(repo, "global-extensions"); diff --git a/src/extensions/discovery.ts b/src/extensions/discovery.ts index 713976b6b..7c8987742 100644 --- a/src/extensions/discovery.ts +++ b/src/extensions/discovery.ts @@ -2,7 +2,7 @@ import fs from "node:fs"; import { homedir } from "node:os"; import { basename, isAbsolute, join, resolve } from "node:path"; import { resolveGlobalExtensionsDir } from "../core/paths"; -import { findVcsRepoRootCandidate } from "../core/vcs"; +import { findProjectRootCandidate } from "../core/projectRoot"; import { deriveExtensionId, type ExtensionCandidate, type ExtensionOrigin } from "./types"; /** Entry-file suffixes Hunk will import directly, in preference order. */ @@ -262,7 +262,7 @@ function expandExplicitPath(path: string, cwd: string): DiscoveredExtensionEntry export function discoverExtensions(options: DiscoverExtensionsOptions = {}): ExtensionCandidate[] { const cwd = options.cwd ?? process.cwd(); const env = options.env ?? process.env; - const repoRoot = options.repoRoot ?? findVcsRepoRootCandidate(cwd); + const repoRoot = options.repoRoot ?? findProjectRootCandidate(cwd); const globalExtensionsDir = options.globalExtensionsDir ?? resolveGlobalExtensionsDir(env); const groups: Array<{ origin: ExtensionOrigin; entries: DiscoveredExtensionEntry[] }> = [ diff --git a/src/extensions/events.test.ts b/src/extensions/events.test.ts index 1568aae7d..56faf9cd3 100644 --- a/src/extensions/events.test.ts +++ b/src/extensions/events.test.ts @@ -5,9 +5,9 @@ import { emitExtensionCustomEvent, emitExtensionEvent, emitExtensionEventBounded, - emitExtensionEventToExtensions, readMetadataHunkCount, readMetadataHunkSummaries, + retireExtensionLoadResult, toReadOnlyFileViews, } from "./events"; import { createExtensionNotificationHub } from "./notifications"; @@ -190,6 +190,42 @@ describe("extension event dispatch", () => { }); describe("extension event bus", () => { + test("revokes retained contexts before an asynchronous shutdown settles", async () => { + let retainedContext: Parameters>[1] | undefined; + let deliveries = 0; + const { result } = createTestLoadResult([ + { + extensionId: "sender", + event: "startup", + handler: (_payload, context) => { + retainedContext = context; + }, + }, + ]); + result.registry.customEventHandlers.push({ + extensionId: "receiver", + event: "probe", + handler: () => { + deliveries += 1; + }, + }); + let releaseShutdown!: () => void; + result.registry.eventHandlers.shutdown.push({ + extensionId: "sender", + handler: () => new Promise((resolve) => (releaseShutdown = resolve)), + }); + bindExtensionEventBus(result); + emitExtensionEvent(result, "startup", { cwd: "/repo" }); + + const retirement = retireExtensionLoadResult(result); + expect(result.registry.eventBusPhase).toBe("closed"); + retainedContext?.events.emit("probe", {}); + expect(deliveries).toBe(0); + + releaseShutdown(); + await retirement; + }); + test("delivers a namespaced event to every listener and isolates failures", async () => { const seen: string[] = []; const { result, notices } = createTestLoadResult(); @@ -507,62 +543,3 @@ describe("read-only file views", () => { expect(views[1]!.hunks).toBe(views[0]!.hunks); }); }); - -describe("emitExtensionEventToExtensions", () => { - test("delivers only to the named extensions", () => { - const seen: string[] = []; - const { result } = createTestLoadResult([ - { - extensionId: "already-started", - event: "startup", - handler: () => { - seen.push("already-started"); - }, - }, - { - extensionId: "newly-trusted", - event: "startup", - handler: () => { - seen.push("newly-trusted"); - }, - }, - ]); - - emitExtensionEventToExtensions(result, "startup", { cwd: "/repo" }, new Set(["newly-trusted"])); - - expect(seen).toEqual(["newly-trusted"]); - }); - - test("does nothing when the id set is empty", () => { - const seen: string[] = []; - const { result } = createTestLoadResult([ - { - extensionId: "any", - event: "startup", - handler: () => { - seen.push("any"); - }, - }, - ]); - - emitExtensionEventToExtensions(result, "startup", { cwd: "/repo" }, new Set()); - - expect(seen).toEqual([]); - }); - - test("still isolates a throwing handler", () => { - const { result, notices } = createTestLoadResult([ - { - extensionId: "broken", - event: "startup", - handler: () => { - throw new Error("boom"); - }, - }, - ]); - - emitExtensionEventToExtensions(result, "startup", { cwd: "/repo" }, new Set(["broken"])); - - expect(notices[0]).toContain("Extension broken failed handling startup • boom"); - }); -}); diff --git a/src/extensions/events.ts b/src/extensions/events.ts index d9cbf55e3..1d18bcfb1 100644 --- a/src/extensions/events.ts +++ b/src/extensions/events.ts @@ -343,13 +343,8 @@ function runExtensionEventHandlers( result: ExtensionLoadResult, event: Event, rawPayload: ExtensionEventPayloads[Event], - /** Restrict delivery to handlers owned by these extensions; all of them when omitted. */ - extensionIds?: ReadonlySet, ): Promise[] { - const registered = result.registry.eventHandlers[event]; - const handlers = extensionIds - ? registered.filter((entry) => extensionIds.has(entry.extensionId)) - : registered; + const handlers = result.registry.eventHandlers[event]; const settled: Promise[] = []; if (handlers.length === 0) { @@ -393,7 +388,7 @@ export function emitExtensionCustomEvent( event: string, rawPayload: unknown, ) { - if (!result) { + if (!result || result.registry.eventBusPhase === "closed") { return; } @@ -462,29 +457,6 @@ export function emitExtensionEvent( runExtensionEventHandlers(result, event, payload); } -/** - * Emit one lifecycle event to a named subset of the loaded extensions. - * - * This exists for `startup`, which is a per-extension promise ("once, after the - * app mounts with its first changeset") rather than a per-session one. Granting - * repo trust mid-session loads extensions that missed the mount emit entirely, - * and re-emitting to everyone would fire `startup` a second time for the - * extensions that already had it. Delivering to just the newly loaded ones - * keeps both halves of the promise. - */ -export function emitExtensionEventToExtensions( - result: ExtensionLoadResult | undefined, - event: Event, - payload: ExtensionEventPayloads[Event], - extensionIds: ReadonlySet, -) { - if (!result || extensionIds.size === 0) { - return; - } - - runExtensionEventHandlers(result, event, payload, extensionIds); -} - /** * Emit one lifecycle event and wait for its async handlers, up to a bound. * @@ -519,3 +491,26 @@ export async function emitExtensionEventBounded { + if (!revokeExtensionLoadResult(result)) { + return; + } + + await emitExtensionEventBounded(result, "shutdown", {}); +} diff --git a/src/extensions/host.test.ts b/src/extensions/host.test.ts index 915e73096..a26362514 100644 --- a/src/extensions/host.test.ts +++ b/src/extensions/host.test.ts @@ -326,7 +326,11 @@ export default function (hunk: { registerSidebarView: (view: unknown) => void }) const backend = createTestExtension(dir, "git.ts", source); const healthy = createTestExtension(dir, "healthy.ts", source); - const result = await loadExtensions({ candidates: [vendor, backend, healthy], cwd: dir }); + const result = await loadExtensions({ + candidates: [vendor, backend, healthy], + cwd: dir, + reservedExtensionIds: new Set(["git", "jj", "sl"]), + }); expect(result.loaded.map((entry) => entry.id)).toEqual(["healthy"]); expect(result.issues.map((issue) => issue.extensionId)).toEqual(["hunk", "git"]); diff --git a/src/extensions/host.ts b/src/extensions/host.ts index cff2d5992..aa2a16023 100644 --- a/src/extensions/host.ts +++ b/src/extensions/host.ts @@ -1,5 +1,5 @@ import { pathToFileURL } from "node:url"; -import { findVcsRepoRootCandidate, isVcsId } from "../core/vcs"; +import { findProjectRootCandidate } from "../core/projectRoot"; import { EXTENSION_ID_RULE, HUNK_VENDOR_EXTENSION_ID, isValidExtensionId } from "./extensionIds"; import { bindExtensionEventBus } from "./events"; import { registerHostRuntimeModules } from "./hostRuntimeModules"; @@ -18,6 +18,10 @@ import { export interface LoadExtensionsOptions { candidates: readonly ExtensionCandidate[]; + /** Full candidate order represented after this pass; defaults to `candidates`. */ + allCandidates?: readonly ExtensionCandidate[]; + /** Existing pass to extend when `candidates` contains only a newly discovered suffix. */ + previousLoad?: ExtensionLoadResult; cwd: string; /** Per-extension `[extension.]` config tables, keyed by extension id. */ extensionConfigs?: Record>; @@ -28,6 +32,10 @@ export interface LoadExtensionsOptions { notifications?: ExtensionNotificationHub; /** Repo root repo-local candidates belong to; discovered from `cwd` when omitted. */ repoRoot?: string; + /** Keep factory bus events queued until a staged caller finishes appending candidates. */ + deferEventBusBinding?: boolean; + /** Product-owned ids user extension modules may not claim. */ + reservedExtensionIds?: ReadonlySet; env?: NodeJS.ProcessEnv; /** Trust lookup seam so tests can drive gating without touching the state file. */ resolveRepoTrustImpl?: (repoRoot: string, options: ExtensionTrustOptions) => ExtensionTrustState; @@ -41,16 +49,9 @@ async function importExtensionModule(path: string): Promise { return await import(pathToFileURL(path).href); } -/** - * Report whether an id belongs to Hunk itself rather than to an extension. - * - * The bundled tier names each extension after the backend it registers, so - * `isVcsId` is the single source for `git`/`jj`/`sl` instead of a second list - * that could drift when a backend is added; `hunk` covers everything else Hunk - * owns — built-in commands and the bundled sidebar's views alike. - */ -function isReservedExtensionId(id: string) { - return id === HUNK_VENDOR_EXTENSION_ID || isVcsId(id); +/** Report whether an id belongs to Hunk rather than to a user extension. */ +function isReservedExtensionId(id: string, reservedIds: ReadonlySet) { + return id === HUNK_VENDOR_EXTENSION_ID || reservedIds.has(id); } /** @@ -64,8 +65,9 @@ function isReservedExtensionId(id: string) { function describeIdRefusal( candidate: ExtensionCandidate, claimedBy: ReadonlyMap, + reservedIds: ReadonlySet, ): string | undefined { - if (isReservedExtensionId(candidate.id)) { + if (isReservedExtensionId(candidate.id, reservedIds)) { return `"${candidate.id}" is reserved by Hunk and cannot be an extension id • rename ${candidate.path}`; } @@ -96,13 +98,17 @@ interface AcceptedCandidates { * uses everywhere else, with the loser reported rather than silently sharing * the winner's config table, command ids, and view keys. */ -function acceptCandidateIds(candidates: readonly ExtensionCandidate[]): AcceptedCandidates { +function acceptCandidateIds( + candidates: readonly ExtensionCandidate[], + reservedIds: ReadonlySet, + initialClaims: ReadonlyMap = new Map(), +): AcceptedCandidates { const accepted: ExtensionCandidate[] = []; const issues: ExtensionLoadIssue[] = []; - const claimedBy = new Map(); + const claimedBy = new Map(initialClaims); for (const candidate of candidates) { - const refusal = describeIdRefusal(candidate, claimedBy); + const refusal = describeIdRefusal(candidate, claimedBy, reservedIds); if (refusal !== undefined) { issues.push({ extensionId: candidate.id, @@ -120,6 +126,20 @@ function acceptCandidateIds(candidates: readonly ExtensionCandidate[]): Accepted return { accepted, issues }; } +/** Return the ids a completed candidate prefix owns, including candidates that failed later. */ +function collectCandidateClaims( + candidates: readonly ExtensionCandidate[], + reservedIds: ReadonlySet, +) { + const claimedBy = new Map(); + for (const candidate of candidates) { + if (describeIdRefusal(candidate, claimedBy, reservedIds) === undefined) { + claimedBy.set(candidate.id, candidate.path); + } + } + return claimedBy; +} + /** * Load every discovered extension into one registry. * @@ -132,22 +152,30 @@ function acceptCandidateIds(candidates: readonly ExtensionCandidate[]): Accepted export async function loadExtensions(options: LoadExtensionsOptions): Promise { // Ids are settled before anything is imported, so a refused candidate never // gets a loader hook, let alone an evaluated module. - const { accepted, issues } = acceptCandidateIds(options.candidates); + const reservedIds = options.reservedExtensionIds ?? new Set(); + const previousCandidates = options.previousLoad?.loadState.candidates ?? []; + const { accepted, issues: candidateIssues } = acceptCandidateIds( + options.candidates, + reservedIds, + collectCandidateClaims(previousCandidates, reservedIds), + ); + const issues = [...(options.previousLoad?.issues ?? []), ...candidateIssues]; // Before any candidate is imported, so its `react` (and `hunkdiff/extension`) // imports resolve to the host's own instances rather than the filesystem. registerHostRuntimeModules(accepted.map((candidate) => candidate.path)); - const registry = createEmptyExtensionRegistry(); + const registry = options.previousLoad?.registry ?? createEmptyExtensionRegistry(); + registry.eventBusPhase = "loading"; const importModule = options.importExtensionModuleImpl ?? importExtensionModule; const resolveTrust = options.resolveRepoTrustImpl ?? resolveRepoTrust; const trustOptions: ExtensionTrustOptions = { env: options.env }; let repoTrustState: ExtensionTrustState | undefined; let repoRoot = options.repoRoot; - let pendingTrustRepoRoot: string | undefined; + let pendingTrustRepoRoot = options.previousLoad?.pendingTrustRepoRoot; /** Resolve the repo trust state once per load pass, lazily. */ const resolveRepoTrustState = () => { - repoRoot ??= findVcsRepoRootCandidate(options.cwd); + repoRoot ??= findProjectRootCandidate(options.cwd); if (!repoRoot) { return "unknown" as const; } @@ -197,15 +225,24 @@ export async function loadExtensions(options: LoadExtensionsOptions): Promise { expect(detected.id).toBe("hg"); } - // Detection over the full adapter list never throws either. - expect(() => detectVcs("/repo", [adapter!])).not.toThrow(); - expect(() => resolveVcsAdapters([adapter!])).not.toThrow(); - expect(() => resolveDetectedVcsIdWithExtensions("/repo", [adapter!])).not.toThrow(); - expect(() => resolveSessionVcsId("hg", "/repo", [adapter!])).not.toThrow(); + // Detection over the complete catalog never throws either. + const catalog = extendVcsCatalog(BASE_VCS_CATALOG, [adapter!]); + expect(() => detectVcs("/repo", catalog)).not.toThrow(); + expect(() => resolveDetectedVcsIdWithExtensions("/repo", catalog)).not.toThrow(); + expect(() => resolveSessionVcsId("hg", "/repo", catalog)).not.toThrow(); } }); @@ -257,7 +260,7 @@ describe("registerVcsAdapter with junk", () => { const adapters = registry.vcsAdapters.map((entry) => entry.adapter); // Hunk's own repo is a Git checkout, so a throwing extension adapter must // not prevent Git from being detected. - expect(detectVcs(process.cwd(), adapters)?.id).toBe("git"); + expect(detectVcs(process.cwd(), extendVcsCatalog(BASE_VCS_CATALOG, adapters))?.id).toBe("git"); }); test("an adapter whose operations are unusable reports unsupported, not a TypeError", () => { @@ -290,10 +293,13 @@ describe("registerVcsAdapter with junk", () => { hunk.registerVcsAdapter({ id: "hg", name: "Mercurial", detect: () => null }); }); - const applied = applyExtensionRegistrations({ - ...createEmptyExtensionLoadResult("/repo"), - registry, - }); + const applied = applyExtensionRegistrations( + { + ...createEmptyExtensionLoadResult("/repo"), + registry, + }, + BASE_VCS_CATALOG, + ); expect(applied.vcsAdapters.map((adapter) => adapter.id)).toEqual(["hg"]); expect(applied.issues).toHaveLength(3); diff --git a/src/extensions/startup.test.ts b/src/extensions/startup.test.ts index 4a5004077..10fb9b205 100644 --- a/src/extensions/startup.test.ts +++ b/src/extensions/startup.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { ExtensionsConfig } from "../core/types"; @@ -80,6 +80,83 @@ describe("extension startup", () => { expect(result.registry.themes.map((entry) => entry.theme.id)).toEqual(["midnight"]); }); + test("extends a provisional pass without executing its unchanged factories again", async () => { + const root = createTempDir("hunk-startup-extend-"); + const configHome = join(root, "config"); + const repo = join(root, "repo"); + mkdirSync(repo); + const logPath = join(root, "factories.log"); + writeGlobalExtension( + configHome, + "global.ts", + `import { appendFileSync } from "node:fs"; +export default function (hunk) { + appendFileSync(${JSON.stringify(logPath)}, "global\\n"); + hunk.events.emit("global:ready", {}); +} +`, + ); + + const provisional = await loadStartupExtensions({ + extensions: createExtensionsConfig(), + cwd: repo, + env: { XDG_CONFIG_HOME: configHome } as NodeJS.ProcessEnv, + deferEventBusBinding: true, + }); + const repoExtensions = join(repo, ".hunk", "extensions"); + mkdirSync(repoExtensions, { recursive: true }); + writeFileSync( + join(repoExtensions, "local.ts"), + `import { appendFileSync } from "node:fs"; +export default function (hunk) { + appendFileSync(${JSON.stringify(logPath)}, "local\\n"); + hunk.events.on("global:ready", () => appendFileSync(${JSON.stringify(logPath)}, "event\\n")); +} +`, + ); + + const final = await loadStartupExtensions({ + extensions: createExtensionsConfig(), + cwd: repo, + env: { XDG_CONFIG_HOME: configHome } as NodeJS.ProcessEnv, + projectRoot: repo, + previousLoad: provisional, + hostOverrides: { resolveRepoTrustImpl: () => "trusted" }, + }); + + expect(readFileSync(logPath, "utf8")).toBe("global\nlocal\nevent\n"); + expect(final.loaded.map((extension) => extension.id)).toEqual(["global", "local"]); + }); + + test("shuts down a provisional pass before changed config requires rebuilding it", async () => { + const home = createTempDir("hunk-startup-rebuild-"); + const logPath = join(home, "lifecycle.log"); + writeGlobalExtension( + home, + "configured.ts", + `import { appendFileSync } from "node:fs"; +export default function (hunk) { + appendFileSync(${JSON.stringify(logPath)}, "factory:" + hunk.config.value + "\\n"); + hunk.on("shutdown", () => appendFileSync(${JSON.stringify(logPath)}, "shutdown\\n")); +} +`, + ); + + const provisional = await loadStartupExtensions({ + extensions: createExtensionsConfig({ extensionConfigs: { configured: { value: 1 } } }), + cwd: home, + env: { XDG_CONFIG_HOME: home } as NodeJS.ProcessEnv, + }); + await loadStartupExtensions({ + extensions: createExtensionsConfig({ extensionConfigs: { configured: { value: 2 } } }), + cwd: home, + env: { XDG_CONFIG_HOME: home } as NodeJS.ProcessEnv, + previousLoad: provisional, + }); + + expect(readFileSync(logPath, "utf8")).toBe("factory:1\nshutdown\nfactory:2\n"); + }); + test("maps load failures onto startup notices without dropping config notices", () => { const configNotice = { key: "deprecated:custom-theme-syntax", message: "legacy syntax" }; const failing = { diff --git a/src/extensions/startup.ts b/src/extensions/startup.ts index 23360d401..39c14f4f1 100644 --- a/src/extensions/startup.ts +++ b/src/extensions/startup.ts @@ -1,11 +1,14 @@ +import { isDeepStrictEqual } from "node:util"; import type { StartupNotice } from "../core/startupNotice"; import type { ExtensionsConfig } from "../core/types"; import { sanitizeTerminalText } from "../lib/terminalText"; import { discoverExtensions } from "./discovery"; +import { retireExtensionLoadResult } from "./events"; import { loadExtensions, type LoadExtensionsOptions } from "./host"; import { createExtensionNotificationHub, type ExtensionNotificationHub } from "./notifications"; import { createEmptyExtensionLoadResult, + type ExtensionCandidate, type ExtensionLoadIssue, type ExtensionLoadResult, } from "./types"; @@ -20,11 +23,19 @@ export interface LoadStartupExtensionsOptions { env?: NodeJS.ProcessEnv; /** Entry paths from repeated `--extension` flags. */ cliExtensionPaths?: readonly string[]; + /** Project root resolved before user extensions execute. */ + projectRoot?: string; + /** Product-owned ids user extension modules may not claim. */ + reservedExtensionIds?: ReadonlySet; /** * Sink extension `ctx.notify` calls land in. Pass the hub from an earlier * pass when reloading extensions so the mounted UI keeps receiving them. */ notifications?: ExtensionNotificationHub; + /** Provisional pass that may be extended when final discovery only appends candidates. */ + previousLoad?: ExtensionLoadResult; + /** Keep factory bus events queued for a possible staged continuation. */ + deferEventBusBinding?: boolean; /** Test seams forwarded to the host loader. */ hostOverrides?: Pick< LoadExtensionsOptions, @@ -32,11 +43,38 @@ export interface LoadStartupExtensionsOptions { >; } +/** Return whether final discovery can safely append to a provisional registry. */ +function canExtendPreviousLoad( + previous: ExtensionLoadResult, + candidates: readonly ExtensionCandidate[], + extensionConfigs: Record>, + cwd: string, +) { + const priorCandidates = previous.loadState.candidates; + if (previous.context.cwd !== cwd || priorCandidates.length > candidates.length) { + return false; + } + + for (let index = 0; index < priorCandidates.length; index += 1) { + if (!isDeepStrictEqual(priorCandidates[index], candidates[index])) { + return false; + } + } + + return priorCandidates.every((candidate) => + isDeepStrictEqual( + previous.loadState.extensionConfigs[candidate.id], + extensionConfigs[candidate.id], + ), + ); +} + /** * Run discovery and loading for one interactive session. * * Disabled extensions short-circuit to an empty registry so nothing on disk is - * read, let alone executed. + * read, let alone executed. A final staged pass extends an unchanged provisional + * prefix instead of executing those factories twice. */ export async function loadStartupExtensions( options: LoadStartupExtensionsOptions, @@ -45,31 +83,58 @@ export async function loadStartupExtensions( const env = options.env ?? process.env; // One hub per pass unless the caller supplies the session's existing one, so // `ctx.notify` always has somewhere to go even before the UI subscribes. - const notifications = options.notifications ?? createExtensionNotificationHub(); + const notifications = + options.notifications ?? + options.previousLoad?.notifications ?? + createExtensionNotificationHub(); if (!options.extensions.enabled) { + await retireExtensionLoadResult(options.previousLoad); return createEmptyExtensionLoadResult(cwd, notifications); } const candidates = discoverExtensions({ cwd, env, - repoRoot: options.hostOverrides?.repoRoot, + repoRoot: options.projectRoot ?? options.hostOverrides?.repoRoot, flagPaths: options.cliExtensionPaths, configPaths: options.extensions.paths, repoConfigPaths: options.extensions.repoPaths, }); if (candidates.length === 0) { + await retireExtensionLoadResult(options.previousLoad); return createEmptyExtensionLoadResult(cwd, notifications); } + const previousLoad = + options.previousLoad && + canExtendPreviousLoad( + options.previousLoad, + candidates, + options.extensions.extensionConfigs, + cwd, + ) + ? options.previousLoad + : undefined; + if (options.previousLoad && !previousLoad) { + await retireExtensionLoadResult(options.previousLoad); + } + const candidatesToLoad = previousLoad + ? candidates.slice(previousLoad.loadState.candidates.length) + : candidates; + return await loadExtensions({ - candidates, + candidates: candidatesToLoad, + allCandidates: candidates, + previousLoad, cwd, env, extensionConfigs: options.extensions.extensionConfigs, notifications, ...options.hostOverrides, + repoRoot: options.projectRoot ?? options.hostOverrides?.repoRoot, + reservedExtensionIds: options.reservedExtensionIds, + deferEventBusBinding: options.deferEventBusBinding, }); } diff --git a/src/extensions/types.ts b/src/extensions/types.ts index 56fa4e1c5..d67f0a9a1 100644 --- a/src/extensions/types.ts +++ b/src/extensions/types.ts @@ -226,6 +226,13 @@ export interface ExtensionLoadIssue { } /** Result of one extension load pass. */ +export interface ExtensionLoadState { + /** Full discovery order used to build this registry, including refused candidates. */ + candidates: readonly ExtensionCandidate[]; + /** Config snapshot factories in this registry were created against. */ + extensionConfigs: Record>; +} + export interface ExtensionLoadResult { registry: ExtensionRegistry; issues: ExtensionLoadIssue[]; @@ -245,6 +252,8 @@ export interface ExtensionLoadResult { * the same hub instead of orphaning the UI's subscription. */ notifications: ExtensionNotificationHub; + /** Internal inputs retained so a staged pass can append newly discovered candidates safely. */ + loadState: ExtensionLoadState; /** * Repo root holding repo-local extensions that have no trust decision yet. * Set only when such extensions exist and were therefore skipped, so the UI @@ -326,5 +335,6 @@ export function createEmptyExtensionLoadResult( loaded: [], context: createExtensionContext(cwd, notifications.notify), notifications, + loadState: { candidates: [], extensionConfigs: {} }, }; } diff --git a/src/extensions/vcsPatchResult.test.ts b/src/extensions/vcsPatchResult.test.ts index e02023f57..e28f25fd8 100644 --- a/src/extensions/vcsPatchResult.test.ts +++ b/src/extensions/vcsPatchResult.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { toInternalVcsPatchResult } from "./vcsPatchResult"; import { HunkExtensionUserError } from "../extension-api/types"; import { HunkUserError, toUserFacingError } from "../core/errors"; +import { SourceTextTooLargeError } from "../core/fileSource"; import { toInternalVcsAdapter } from "./runExtension"; import type { ExtensionVcsFileSourceRequest, @@ -120,6 +121,28 @@ describe("published source readers", () => { expect(await fetcher?.getFullText("new")).toBe("recovered"); }); + test("translate and cache public too-large results without re-reading the adapter", async () => { + let attempts = 0; + const result = toInternalVcsPatchResult( + baseResult({ + readFileSource: async () => { + attempts += 1; + return attempts === 1 ? { kind: "too-large", maxBytes: 42 } : "recovered"; + }, + }), + ); + const fetcher = result.sourceFetcherBuilder?.({ + path: "src/a.ts", + type: "change", + isUntracked: false, + isBinary: false, + }); + + await expect(fetcher?.getFullText("new")).rejects.toEqual(new SourceTextTooLargeError(42)); + await expect(fetcher?.getFullText("new")).rejects.toEqual(new SourceTextTooLargeError(42)); + expect(attempts).toBe(1); + }); + test("are absent when the result declares none", () => { expect(toInternalVcsPatchResult(baseResult()).sourceFetcherBuilder).toBeUndefined(); }); diff --git a/src/extensions/vcsPatchResult.ts b/src/extensions/vcsPatchResult.ts index f7fbb53ce..8ccae2c83 100644 --- a/src/extensions/vcsPatchResult.ts +++ b/src/extensions/vcsPatchResult.ts @@ -4,7 +4,11 @@ import { type BuildDiffFileOptions, } from "../core/diffFile"; import { parseSingleFilePatch } from "../core/patch/singleFile"; -import type { FileSourceSide } from "../core/fileSource"; +import { + DEFAULT_SOURCE_TEXT_MAX_BYTES, + SourceTextTooLargeError, + type FileSourceSide, +} from "../core/fileSource"; import type { DiffFile } from "../core/types"; import type { VcsPatchResult } from "../core/vcs/types"; import type { @@ -31,8 +35,8 @@ type SourceFetcherBuilder = NonNullable(); + const tooLargeCache = new Map(); return { cacheKey: sourceCacheKey, @@ -51,16 +56,34 @@ function toSourceFetcherBuilder( if (cache.has(side)) { return cache.get(side) ?? null; } + const cachedLimit = tooLargeCache.get(side); + if (cachedLimit !== undefined) { + throw new SourceTextTooLargeError(cachedLimit); + } - const text = await read({ + const result = await read({ path: file.path, previousPath: file.previousPath, changeType: file.type, isUntracked: file.isUntracked, side, }); - cache.set(side, text); - return text; + if (typeof result === "object" && result !== null) { + if (result.kind === "too-large") { + const maxBytes = + typeof result.maxBytes === "number" && + Number.isFinite(result.maxBytes) && + result.maxBytes > 0 + ? result.maxBytes + : DEFAULT_SOURCE_TEXT_MAX_BYTES; + tooLargeCache.set(side, maxBytes); + throw new SourceTextTooLargeError(maxBytes); + } + throw new Error("VCS source readers must return text, null, or a too-large result."); + } + + cache.set(side, result); + return result; }, }; }; diff --git a/src/core/vcs/largeFile.ts b/src/lib/largeFile.ts similarity index 100% rename from src/core/vcs/largeFile.ts rename to src/lib/largeFile.ts diff --git a/src/lib/patchPath.ts b/src/lib/patchPath.ts new file mode 100644 index 000000000..f03a69c16 --- /dev/null +++ b/src/lib/patchPath.ts @@ -0,0 +1,8 @@ +/** Escape only path characters that break unified-diff header parsing. */ +export function escapeUntrackedPatchPath(path: string) { + return path + .replaceAll("\\", "\\\\") + .replaceAll("\t", "\\t") + .replaceAll("\n", "\\n") + .replaceAll("\r", "\\r"); +} diff --git a/src/lib/sourceText.test.ts b/src/lib/sourceText.test.ts new file mode 100644 index 000000000..c9e4b8985 --- /dev/null +++ b/src/lib/sourceText.test.ts @@ -0,0 +1,32 @@ +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 { readFileTextWithLimit } from "./sourceText"; + +const tempDirs: string[] = []; + +/** Create one temporary source directory tracked for cleanup. */ +function createTempDir() { + const dir = mkdtempSync(join(tmpdir(), "hunk-source-text-")); + tempDirs.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("readFileTextWithLimit", () => { + test("returns text, missing, and structural too-large results", async () => { + const dir = createTempDir(); + const source = join(dir, "source.txt"); + writeFileSync(source, "source text\n"); + + expect(await readFileTextWithLimit(source, 100)).toBe("source text\n"); + expect(await readFileTextWithLimit(join(dir, "missing.txt"), 100)).toBeNull(); + expect(await readFileTextWithLimit(source, 5)).toEqual({ kind: "too-large", maxBytes: 5 }); + }); +}); diff --git a/src/lib/sourceText.ts b/src/lib/sourceText.ts new file mode 100644 index 000000000..64f986058 --- /dev/null +++ b/src/lib/sourceText.ts @@ -0,0 +1,84 @@ +/** Default byte ceiling for exact source text loaded for expanded context. */ +export const DEFAULT_SOURCE_TEXT_MAX_BYTES = 1_000_000; + +/** Structural result shared by bounded filesystem source readers. */ +export type LimitedSourceTextResult = string | null | { kind: "too-large"; maxBytes: number }; + +/** Keep source-load diagnostics terse enough to be useful in logs. */ +export function logSourceDiagnostic(message: string, detail?: unknown) { + if (detail instanceof Error) { + console.error(`hunk: ${message}: ${detail.message}`, detail); + return; + } + + const firstLine = + typeof detail === "string" + ? detail + .split("\n") + .map((line) => line.trim()) + .find(Boolean) + : undefined; + console.error(firstLine ? `hunk: ${message}: ${firstLine}` : `hunk: ${message}`); +} + +/** Read one filesystem source as text without exceeding the supplied byte ceiling. */ +export async function readFileTextWithLimit( + absolutePath: string, + maxBytes: number, +): Promise { + try { + const file = Bun.file(absolutePath); + if (!(await file.exists())) { + return null; + } + if (file.size > maxBytes) { + return { kind: "too-large", maxBytes }; + } + return await file.text(); + } catch (error) { + logSourceDiagnostic(`failed to read source file ${absolutePath}`, error); + return null; + } +} + +/** Read a byte stream as text while enforcing a caller-defined resource limit. */ +export async function readStreamTextWithLimit( + stream: ReadableStream | null, + maxBytes: number, + onTooLarge?: () => void, + createLimitError: (maxBytes: number) => Error = (limit) => + new Error(`Source text exceeds ${limit} bytes.`), +) { + if (!stream) { + return ""; + } + + const reader = stream.getReader(); + const chunks: Uint8Array[] = []; + let totalBytes = 0; + + for (;;) { + const { done, value } = await reader.read(); + if (done) { + break; + } + + totalBytes += value.byteLength; + if (totalBytes > maxBytes) { + onTooLarge?.(); + await reader.cancel().catch(() => undefined); + throw createLimitError(maxBytes); + } + + chunks.push(value); + } + + const combined = new Uint8Array(totalBytes); + let offset = 0; + for (const chunk of chunks) { + combined.set(chunk, offset); + offset += chunk.byteLength; + } + + return new TextDecoder().decode(combined); +} diff --git a/src/session/app/registration.ts b/src/session/app/registration.ts index db68deeaa..0f92d8ee3 100644 --- a/src/session/app/registration.ts +++ b/src/session/app/registration.ts @@ -1,6 +1,7 @@ import { randomUUID } from "node:crypto"; import { spawnSync } from "node:child_process"; import { resolveExperimentalFeatures } from "../../core/experimental"; +import { isVcsReviewInput } from "../../core/vcs"; import { summarizeHunk } from "../../core/hunkSummary"; import { hunkLineRange } from "../../core/liveComments"; import type { AppBootstrap } from "../../core/types"; @@ -27,11 +28,7 @@ function ttyname(): string | undefined { /** Infer the repo-root selector that remote session commands should match for this review input. */ function inferRepoRoot(bootstrap: AppBootstrap) { - return bootstrap.input.kind === "vcs" || - bootstrap.input.kind === "show" || - bootstrap.input.kind === "stash-show" - ? bootstrap.changeset.sourceLabel - : undefined; + return isVcsReviewInput(bootstrap.input) ? bootstrap.changeset.sourceLabel : undefined; } /** Convert the loaded changeset into the app-owned file-and-hunk review export model. */ diff --git a/src/session/app/reloadBounds.test.ts b/src/session/app/reloadBounds.test.ts index 469ffd16f..b1083d868 100644 --- a/src/session/app/reloadBounds.test.ts +++ b/src/session/app/reloadBounds.test.ts @@ -2,6 +2,7 @@ import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSyn import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { describe, expect, test } from "bun:test"; +import { getBundledVcsCatalog } from "../../app/vcsCatalog"; import type { AppBootstrap, CliInput } from "../../core/types"; import { createSessionReloadBounds, validateSessionReloadWithinBounds } from "./reloadBounds"; @@ -13,7 +14,7 @@ function realPath(path: string) { function bootstrapFor(input: CliInput, sourceLabel: string): AppBootstrap { return { input, - reloadContext: { cwd: sourceLabel }, + reloadContext: { cwd: sourceLabel, vcsCatalog: getBundledVcsCatalog() }, changeset: { id: "changeset:test", sourceLabel, diff --git a/src/session/app/reloadBounds.ts b/src/session/app/reloadBounds.ts index 52d12e9cc..23caaab8e 100644 --- a/src/session/app/reloadBounds.ts +++ b/src/session/app/reloadBounds.ts @@ -1,7 +1,8 @@ import { isAbsolute, relative, resolve } from "node:path"; import { resolveCanonicalPath } from "../../core/paths"; -import { findVcsRepoRootCandidate } from "../../core/vcs"; +import { findProjectRootCandidate } from "../../core/projectRoot"; import type { AppBootstrap, CliInput, CommonOptions } from "../../core/types"; +import type { VcsCatalog } from "../../core/vcs/types"; /** * Session reload filesystem policy: @@ -56,8 +57,8 @@ function normalizeRoots(roots: string[]) { } /** Return the initial repo root when every requested file is inside that checkout. */ -function resolveRepoReloadRoots(initialCwd: string, paths: string[]) { - const repoRoot = findVcsRepoRootCandidate(initialCwd); +function resolveRepoReloadRoots(initialCwd: string, paths: string[], vcsCatalog?: VcsCatalog) { + const repoRoot = findProjectRootCandidate(initialCwd, vcsCatalog); if (!repoRoot) { return []; } @@ -83,12 +84,20 @@ export function createSessionReloadBounds( break; case "diff": case "difftool": - roots = resolveRepoReloadRoots(initialCwd, [bootstrap.input.left, bootstrap.input.right]); + roots = resolveRepoReloadRoots( + initialCwd, + [bootstrap.input.left, bootstrap.input.right], + bootstrap.reloadContext.vcsCatalog, + ); break; case "patch": roots = bootstrap.input.file && bootstrap.input.file !== "-" - ? resolveRepoReloadRoots(initialCwd, [bootstrap.input.file]) + ? resolveRepoReloadRoots( + initialCwd, + [bootstrap.input.file], + bootstrap.reloadContext.vcsCatalog, + ) : []; break; } diff --git a/src/session/client/capabilities.test.ts b/src/session/client/capabilities.test.ts index 316c78d33..0685cd7e5 100644 --- a/src/session/client/capabilities.test.ts +++ b/src/session/client/capabilities.test.ts @@ -83,13 +83,13 @@ describe("readHunkSessionDaemonCapabilities", () => { await expect(readHunkSessionDaemonCapabilities(config)).resolves.toBeNull(); }); - test("rejects version 5 daemons that drop session experimental-feature fields", async () => { + test("rejects daemons from the previous selector wire version", async () => { const { config } = await listen((_request: IncomingMessage, response: ServerResponse) => { response.writeHead(200, { "content-type": "application/json" }); response.end( JSON.stringify({ version: HUNK_SESSION_API_VERSION, - daemonVersion: 5, + daemonVersion: HUNK_SESSION_DAEMON_VERSION - 1, actions: ["comment-add", "comment-apply"], }), ); diff --git a/src/session/protocol.ts b/src/session/protocol.ts index dc61cbf62..91c527458 100644 --- a/src/session/protocol.ts +++ b/src/session/protocol.ts @@ -32,7 +32,7 @@ export const HUNK_SESSION_API_VERSION = 1; * builds can refresh an older daemon even when it still exposes the same API endpoints. Bump this * when daemon-forwarded payloads change, even if the supported action names stay stable. */ -export const HUNK_SESSION_DAEMON_VERSION = 6; +export const HUNK_SESSION_DAEMON_VERSION = 7; export type SessionDaemonAction = | "list" diff --git a/src/session/protocolSchemas.test.ts b/src/session/protocolSchemas.test.ts index c77383a02..a85744335 100644 --- a/src/session/protocolSchemas.test.ts +++ b/src/session/protocolSchemas.test.ts @@ -20,7 +20,10 @@ describe("session daemon request validation", () => { const requests: unknown[] = [ { action: "list" }, { action: "get", selector: { sessionId: "s-1" } }, - { action: "context", selector: { repoRoot: "/repo" } }, + { + action: "context", + selector: { repoRoot: "/repo/nested", repoBoundary: "/repo" }, + }, { action: "review", selector: { sessionId: "s-1" } }, { action: "review", selector: { sessionId: "s-1" }, includePatch: true, includeNotes: true }, { action: "navigate", selector: { sessionId: "s-1" }, hunkNumber: 2 }, diff --git a/src/session/protocolSchemas.ts b/src/session/protocolSchemas.ts index 2fcf9a1b5..8f8d111fc 100644 --- a/src/session/protocolSchemas.ts +++ b/src/session/protocolSchemas.ts @@ -16,6 +16,7 @@ const selectorSchema = z.strictObject({ sessionId: z.string().optional(), sessionPath: z.string().optional(), repoRoot: z.string().optional(), + repoBoundary: z.string().optional(), }); const sideSchema = z.enum(["old", "new"]); diff --git a/src/ui/App.extension-command-controls.test.tsx b/src/ui/App.extension-command-controls.test.tsx index 95815e0e6..47f46cfc3 100644 --- a/src/ui/App.extension-command-controls.test.tsx +++ b/src/ui/App.extension-command-controls.test.tsx @@ -3,7 +3,7 @@ import { testRender } from "@opentui/react/test-utils"; import { act, StrictMode, useState } from "react"; import { createTestVcsAppBootstrap } from "../../test/helpers/app-bootstrap"; import { createTestDiffFile } from "../../test/helpers/diff-helpers"; -import type { AppBootstrap } from "../core/types"; +import type { AppBootstrap } from "../app/types"; import type { ExtensionCommandControls } from "../extension-api/types"; import { createEmptyExtensionLoadResult } from "../extensions/types"; import { App } from "./App"; diff --git a/src/ui/App.extension-trust.test.tsx b/src/ui/App.extension-trust.test.tsx index 858f8ceef..86ce9622a 100644 --- a/src/ui/App.extension-trust.test.tsx +++ b/src/ui/App.extension-trust.test.tsx @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import { testRender } from "@opentui/react/test-utils"; import { act, useState } from "react"; -import type { AppBootstrap } from "../core/types"; +import type { AppBootstrap } from "../app/types"; import { createEmptyExtensionLoadResult } from "../extensions/types"; import { createTestVcsAppBootstrap } from "../../test/helpers/app-bootstrap"; import { createTestDiffFile } from "../../test/helpers/diff-helpers"; diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 4dbc3540f..88d9714f6 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -22,6 +22,7 @@ import { } from "../core/config"; import { experimentalFeatureEnabled, resolveExperimentalDiffFiles } from "../core/experimental"; import { DEFAULT_TAB_WIDTH } from "../core/tabWidth"; +import { isVcsReviewInput } from "../core/vcs"; import type { AppBootstrap, CliInput, @@ -53,6 +54,7 @@ import type { ExtensionWorkspace, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, + ExtensionLoadResult, RegisteredCommand, RegisteredPane, } from "../extensions/types"; @@ -282,7 +284,7 @@ export function App({ } | null>(null); const [sessionNoticeText, setSessionNoticeText] = useState(null); const sessionNoticeTimeoutRef = useRef | null>(null); - const extensions = bootstrap.extensions; + const extensions = bootstrap.extensions as ExtensionLoadResult | undefined; const sessionPanes = useMemo(() => buildSessionPanes(extensions), [extensions]); const [paneOpenState, setPaneOpenState] = useState(() => initialPaneOpenState(sessionPanes)); useEffect( @@ -1443,12 +1445,7 @@ export function App({ await onReloadSession(nextInput, { ...options, resetApp: false, - sourcePath: - bootstrap.input.kind === "vcs" || - bootstrap.input.kind === "show" || - bootstrap.input.kind === "stash-show" - ? bootstrap.changeset.sourceLabel - : undefined, + sourcePath: isVcsReviewInput(bootstrap.input) ? bootstrap.changeset.sourceLabel : undefined, }); }, [ @@ -1566,12 +1563,9 @@ export function App({ }, [extensionTrustPromptRoot, showSessionNotice]); const triggerEditSelectedFile = useCallback(() => { - const basePath = - bootstrap.input.kind === "vcs" || - bootstrap.input.kind === "show" || - bootstrap.input.kind === "stash-show" - ? bootstrap.changeset.sourceLabel - : undefined; + const basePath = isVcsReviewInput(bootstrap.input) + ? bootstrap.changeset.sourceLabel + : undefined; const message = openSelectedFileInEditor({ basePath, file: selectedFile, diff --git a/src/ui/AppHost.extension-dialogs.test.tsx b/src/ui/AppHost.extension-dialogs.test.tsx index ef72efc33..ad536e072 100644 --- a/src/ui/AppHost.extension-dialogs.test.tsx +++ b/src/ui/AppHost.extension-dialogs.test.tsx @@ -6,12 +6,24 @@ import { afterEach, describe, expect, test } from "bun:test"; import { testRender } from "@opentui/react/test-utils"; import { act } from "react"; import { removeTestDirectory } from "../../test/helpers/filesystem"; -import { loadAppBootstrap } from "../core/loaders"; -import type { AppBootstrap, CliInput } from "../core/types"; +import { loadAppBootstrap as loadCoreAppBootstrap } from "../core/loaders"; + +import type { AppBootstrap } from "../app/types"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; +import type { CliInput } from "../core/types"; import type { HunkSessionBrokerClient } from "../session/types"; import { loadStartupExtensions } from "../extensions/startup"; import { AppHost } from "./AppHost"; +/** Specialize the core loader result with extension state assigned by these tests. */ +function loadAppBootstrap(...args: Parameters): Promise { + const [input, options] = args; + return loadCoreAppBootstrap(input, { + vcsCatalog: getBundledVcsCatalog(), + ...options, + }) as Promise; +} + /** * `ctx.dialogs`, driven through the real app: a fixture extension asks a * question from a command handler, the modal renders inside the mounted review, diff --git a/src/ui/AppHost.extension-navigation.test.tsx b/src/ui/AppHost.extension-navigation.test.tsx index 34b182897..e46427764 100644 --- a/src/ui/AppHost.extension-navigation.test.tsx +++ b/src/ui/AppHost.extension-navigation.test.tsx @@ -6,11 +6,22 @@ import { afterEach, describe, expect, test } from "bun:test"; import { testRender } from "@opentui/react/test-utils"; import { act } from "react"; import { removeTestDirectory } from "../../test/helpers/filesystem"; -import { loadAppBootstrap } from "../core/loaders"; -import type { AppBootstrap } from "../core/types"; +import { loadAppBootstrap as loadCoreAppBootstrap } from "../core/loaders"; + +import type { AppBootstrap } from "../app/types"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; import { loadStartupExtensions } from "../extensions/startup"; import { AppHost } from "./AppHost"; +/** Specialize the core loader result with extension state assigned by these tests. */ +function loadAppBootstrap(...args: Parameters): Promise { + const [input, options] = args; + return loadCoreAppBootstrap(input, { + vcsCatalog: getBundledVcsCatalog(), + ...options, + }) as Promise; +} + /** * `ctx.navigation`, driven through the real app: a fixture extension's command * jumps the review stream, and the `selection_changed` event that comes back is diff --git a/src/ui/AppHost.extension-sidebar.test.tsx b/src/ui/AppHost.extension-sidebar.test.tsx index b6b9259cc..aaacdfc4b 100644 --- a/src/ui/AppHost.extension-sidebar.test.tsx +++ b/src/ui/AppHost.extension-sidebar.test.tsx @@ -6,11 +6,22 @@ import { afterEach, describe, expect, test } from "bun:test"; import { testRender } from "@opentui/react/test-utils"; import { act } from "react"; import { removeTestDirectory } from "../../test/helpers/filesystem"; -import { loadAppBootstrap } from "../core/loaders"; -import type { AppBootstrap } from "../core/types"; +import { loadAppBootstrap as loadCoreAppBootstrap } from "../core/loaders"; + +import type { AppBootstrap } from "../app/types"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; import { loadStartupExtensions } from "../extensions/startup"; import { AppHost } from "./AppHost"; +/** Specialize the core loader result with extension state assigned by these tests. */ +function loadAppBootstrap(...args: Parameters): Promise { + const [input, options] = args; + return loadCoreAppBootstrap(input, { + vcsCatalog: getBundledVcsCatalog(), + ...options, + }) as Promise; +} + /** * Extension-contributed sidebar views, mounted through the real load path: a * fixture file on disk, dynamically imported, its `react` served by the host diff --git a/src/ui/AppHost.extensions.test.tsx b/src/ui/AppHost.extensions.test.tsx index 1f6d8f21d..ba265cf6c 100644 --- a/src/ui/AppHost.extensions.test.tsx +++ b/src/ui/AppHost.extensions.test.tsx @@ -6,8 +6,11 @@ import { afterEach, describe, expect, test } from "bun:test"; import { testRender } from "@opentui/react/test-utils"; import { act } from "react"; import { removeTestDirectory } from "../../test/helpers/filesystem"; -import { loadAppBootstrap } from "../core/loaders"; -import type { AppBootstrap, CliInput } from "../core/types"; +import type { AppBootstrap } from "../app/types"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; +import { loadAppBootstrap as loadCoreAppBootstrap } from "../core/loaders"; +import type { CliInput } from "../core/types"; + import type { HunkSessionBrokerClient } from "../session/types"; import { applyExtensionRegistrations, @@ -16,6 +19,15 @@ import { import { loadStartupExtensions } from "../extensions/startup"; import { AppHost } from "./AppHost"; +/** Specialize the core loader result with extension state assigned by these tests. */ +function loadAppBootstrap(...args: Parameters): Promise { + const [input, options] = args; + return loadCoreAppBootstrap(input, { + vcsCatalog: getBundledVcsCatalog(), + ...options, + }) as Promise; +} + /** * Extension behavior that only exists once a session is *running*. * @@ -120,6 +132,9 @@ function writeProbeExtension(path: string, logPath: string) { ` hunk.on("session_reload", () => {\n` + ` appendFileSync(${JSON.stringify(logPath)}, "session_reload\\n");\n` + ` });\n` + + ` hunk.on("shutdown", () => {\n` + + ` appendFileSync(${JSON.stringify(logPath)}, "shutdown\\n");\n` + + ` });\n` + `}\n`, ); } @@ -287,6 +302,93 @@ describe("reload keeps launch extension authority", () => { ); }); + test("a failed replacement keeps the visible extension instance running", async () => { + const repo = createTestRepo("hunk-apphost-failed-extension-reload-"); + const logPath = join(repo, "probe.log"); + const extPath = join(repo, "ext.ts"); + writeProbeExtension(extPath, logPath); + useTempConfigHome(); + + const bootstrap = await launchInSubdirectory(repo, { extensionPaths: [extPath] }); + bootstrap.extensions = await loadStartupExtensions({ + extensions: { enabled: true, paths: [], repoPaths: [], extensionConfigs: {} }, + cwd: join(repo, "sub"), + cliExtensionPaths: [extPath], + }); + + const broker = createTestBrokerClient(); + await withAppHost( + bootstrap, + async (setup) => { + await flushUntil( + setup, + () => readProbeLog(logPath).includes("startup"), + "the original extension instance to start", + ); + + await expect( + broker.reload({ kind: "vcs", staged: false, range: "missing-ref", options: {} }, repo), + ).rejects.toThrow("could not resolve Git revision or range"); + await pumpFrames(setup, 5); + + const events = readProbeLog(logPath); + expect(events.filter((line) => line === "factory")).toHaveLength(2); + expect(events.filter((line) => line === "shutdown")).toHaveLength(1); + // The failed replacement is cleaned up after its factory runs; the + // original instance was not shut down before the replacement proved valid. + expect(events.indexOf("shutdown")).toBeGreaterThan(events.lastIndexOf("factory")); + expect(events.filter((line) => line === "startup")).toHaveLength(1); + }, + broker.client, + ); + }); + + test("serializes concurrent reloads so every replacement receives a full lifecycle", async () => { + const repo = createTestRepo("hunk-apphost-concurrent-extension-reload-"); + const logPath = join(repo, "probe.log"); + const extPath = join(repo, "ext.ts"); + writeProbeExtension(extPath, logPath); + useTempConfigHome(); + + const bootstrap = await launchInSubdirectory(repo, { extensionPaths: [extPath] }); + bootstrap.extensions = await loadStartupExtensions({ + extensions: { enabled: true, paths: [], repoPaths: [], extensionConfigs: {} }, + cwd: join(repo, "sub"), + cliExtensionPaths: [extPath], + }); + const broker = createTestBrokerClient(); + + await withAppHost( + bootstrap, + async (setup) => { + await flushUntil( + setup, + () => readProbeLog(logPath).includes("startup"), + "the original extension instance to start", + ); + + const first = broker.reload({ kind: "vcs", staged: false, options: {} }, repo); + const second = broker.reload( + { kind: "vcs", staged: false, options: {} }, + join(repo, "sub"), + ); + await Promise.all([first, second]); + await flushUntil( + setup, + () => readProbeLog(logPath).filter((line) => line === "startup").length === 3, + "both serialized replacement instances to start", + ); + + const events = readProbeLog(logPath); + expect(events.filter((line) => line === "factory")).toHaveLength(3); + expect(events.filter((line) => line === "shutdown")).toHaveLength(2); + expect(events.filter((line) => line === "startup")).toHaveLength(3); + expect(events.filter((line) => line === "session_reload")).toHaveLength(2); + }, + broker.client, + ); + }); + test("--extension paths survive a reload that re-runs discovery", async () => { const repo = createTestRepo("hunk-apphost-extpath-"); const logPath = join(repo, "probe.log"); @@ -450,7 +552,76 @@ describe("startup for extensions loaded mid-session", () => { expect(events).toContain("startup"); }); - test("does not fire a second time for an extension that already had it", async () => { + test("starts a replacement only after its mounted sidebar controls are ready", async () => { + const repo = createTestRepo("hunk-apphost-mounted-startup-"); + const logPath = join(repo, "mounted.log"); + const extPath = join(repo, "mounted.ts"); + writeFileSync( + extPath, + `import { appendFileSync } from "node:fs"; +` + + `import { createElement } from "react"; +` + + `export default function (hunk) { +` + + ` appendFileSync(${JSON.stringify(logPath)}, "factory\\n"); +` + + ` hunk.registerSidebarView({ +` + + ` id: "probe", +` + + ` title: "Probe", +` + + ` component: () => createElement("text", { content: "MOUNTED STARTUP SIDEBAR" }), +` + + ` }); +` + + ` hunk.on("startup", (_payload, ctx) => { +` + + ` ctx.sidebars.open("probe"); +` + + ` appendFileSync(${JSON.stringify(logPath)}, "startup\\n"); +` + + ` }); +` + + ` hunk.on("shutdown", () => appendFileSync(${JSON.stringify(logPath)}, "shutdown\\n")); +` + + `} +`, + ); + useTempConfigHome(); + + const bootstrap = await launchInSubdirectory(repo, { extensionPaths: [extPath] }); + bootstrap.extensions = await loadStartupExtensions({ + extensions: { enabled: true, paths: [], repoPaths: [], extensionConfigs: {} }, + cwd: join(repo, "sub"), + cliExtensionPaths: [extPath], + }); + const broker = createTestBrokerClient(); + + await withAppHost( + bootstrap, + async (setup) => { + await flushUntil( + setup, + () => setup.captureCharFrame().includes("MOUNTED STARTUP SIDEBAR"), + "the initial startup handler to open its mounted sidebar", + ); + + await broker.reload({ kind: "vcs", staged: false, options: {} }, repo); + await flushUntil( + setup, + () => + readProbeLog(logPath).filter((line) => line === "startup").length === 2 && + setup.captureCharFrame().includes("MOUNTED STARTUP SIDEBAR"), + "the replacement startup handler to receive mounted sidebar controls", + ); + }, + broker.client, + ); + }); + + test("shuts down and starts each replacement extension instance", async () => { const repo = createTestRepo("hunk-apphost-startup-once-"); const logPath = join(repo, "probe.log"); const extPath = join(repo, "ext.ts"); @@ -481,11 +652,14 @@ describe("startup for extensions loaded mid-session", () => { "the refresh key to reload the session", ); - // The reload re-ran the factory, but `startup` is a once-per-extension - // promise: this id already had it, so it is not delivered again. + // The old instance remains live until the replacement review succeeds, + // then shuts down before the mounted replacement receives startup. const events = readProbeLog(logPath); expect(events.filter((line) => line === "factory")).toHaveLength(2); - expect(events.filter((line) => line === "startup")).toHaveLength(1); + expect(events.filter((line) => line === "startup")).toHaveLength(2); + expect(events.lastIndexOf("factory")).toBeLessThan(events.indexOf("shutdown")); + expect(events.indexOf("shutdown")).toBeLessThan(events.lastIndexOf("startup")); + expect(events.lastIndexOf("startup")).toBeLessThan(events.indexOf("session_reload")); }); }); }); @@ -536,6 +710,7 @@ function writeHgExtension(extPath: string) { } describe("reload re-runs extension VCS detection", () => { + const baseVcsCatalog = getBundledVcsCatalog(); test("an extension backend keeps a checkout no built-in recognizes", async () => { // A directory with only an `.hg` marker. No built-in backend detects it, so // config resolves `vcs` to the default Git backend on every pass — including @@ -555,7 +730,7 @@ describe("reload re-runs extension VCS detection", () => { cliExtensionPaths: [extPath], }); expect(extensions.issues).toEqual([]); - const { vcsAdapters } = applyExtensionRegistrations(extensions); + const { vcsCatalog } = applyExtensionRegistrations(extensions, baseVcsCatalog); // Launch the way `prepareStartupPlan` does: extension detection claims the // checkout, and the changeset loads through the extension backend. @@ -566,10 +741,10 @@ describe("reload re-runs extension VCS detection", () => { options: { mode: "stack", extensionPaths: [extPath], - vcs: resolveDetectedVcsIdWithExtensions(repo, vcsAdapters), + vcs: resolveDetectedVcsIdWithExtensions(repo, vcsCatalog), }, }, - { cwd: repo, vcsAdapters }, + { cwd: repo, vcsCatalog }, ); bootstrap.extensions = extensions; expect(bootstrap.changeset.title).toBe("Mercurial working copy"); @@ -616,10 +791,10 @@ describe("reload re-runs extension VCS detection", () => { cliExtensionPaths: [extPath], }); expect(extensions.issues).toEqual([]); - const { vcsAdapters } = applyExtensionRegistrations(extensions); + const { vcsCatalog } = applyExtensionRegistrations(extensions, baseVcsCatalog); // First launch: the nearer `.hg` root wins over the outer Git root. - expect(resolveDetectedVcsIdWithExtensions(inner, vcsAdapters)).toBe("hg"); + expect(resolveDetectedVcsIdWithExtensions(inner, vcsCatalog)).toBe("hg"); const bootstrap = await loadAppBootstrap( { kind: "vcs", @@ -627,10 +802,10 @@ describe("reload re-runs extension VCS detection", () => { options: { mode: "stack", extensionPaths: [extPath], - vcs: resolveDetectedVcsIdWithExtensions(inner, vcsAdapters), + vcs: resolveDetectedVcsIdWithExtensions(inner, vcsCatalog), }, }, - { cwd: inner, vcsAdapters }, + { cwd: inner, vcsCatalog }, ); bootstrap.extensions = extensions; expect(bootstrap.changeset.title).toBe("Mercurial working copy"); diff --git a/src/ui/AppHost.keybindings.test.tsx b/src/ui/AppHost.keybindings.test.tsx index a8abecf71..c8c9f3d01 100644 --- a/src/ui/AppHost.keybindings.test.tsx +++ b/src/ui/AppHost.keybindings.test.tsx @@ -7,6 +7,7 @@ import { testRender } from "@opentui/react/test-utils"; import { act } from "react"; import { removeTestDirectory } from "../../test/helpers/filesystem"; import { resolveConfiguredCliInput } from "../core/config"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; import { loadAppBootstrap } from "../core/loaders"; import type { AppBootstrap } from "../core/types"; import { AppHost } from "./AppHost"; @@ -75,8 +76,9 @@ async function launchWithConfig(repo: string, configToml: string): Promise { const bootstrap = await loadAppBootstrap( { kind: "vcs", staged: false, options: { mode: "stack", excludeUntracked: true } }, - { cwd: dir }, + { cwd: dir, vcsCatalog: getBundledVcsCatalog() }, ); const setup = await testRender(, { diff --git a/src/ui/AppHost.tsx b/src/ui/AppHost.tsx index c6f959960..2d504298a 100644 --- a/src/ui/AppHost.tsx +++ b/src/ui/AppHost.tsx @@ -1,16 +1,23 @@ import { useCallback, useEffect, useRef, useState } from "react"; +import { resolveConfiguredExtensions } from "../app/extensionBootstrap"; import { loadConfiguredSessionBootstrap } from "../app/sessionBootstrap"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; import { resolveConfiguredCliInput } from "../core/config"; import { resolveRuntimeCliInput } from "../core/terminal"; import type { StartupNotice } from "../core/startupNotice"; import type { AppBootstrap, CliInput } from "../core/types"; -import { createUnknownVcsNotice, reportExtensionApplyIssues } from "../extensions/apply"; +import type { ExtensionLoadResult } from "../extensions/types"; +import { + createUnknownVcsNotice, + reportExtensionApplyIssues, + resolveExtensionVcsAdapters, +} from "../extensions/apply"; import { emitExtensionEvent, emitExtensionEventBounded, - emitExtensionEventToExtensions, + retireExtensionLoadResult, } from "../extensions/events"; -import { loadStartupExtensions } from "../extensions/startup"; +import { extendVcsCatalog } from "../core/vcs"; import { createInitialSessionSnapshot, updateSessionRegistration, @@ -38,14 +45,23 @@ export function AppHost({ startupNoticeResolver?: () => Promise; watchRuntime?: WatchedInputRuntime; }) { - const [activeBootstrap, setActiveBootstrap] = useState(bootstrap); + const initialBootstrap = bootstrap.reloadContext.vcsCatalog + ? bootstrap + : { + ...bootstrap, + reloadContext: { + ...bootstrap.reloadContext, + vcsCatalog: getBundledVcsCatalog(), + }, + }; + const [activeBootstrap, setActiveBootstrap] = useState(initialBootstrap); const [appVersion, setAppVersion] = useState(0); // Extensions outlive App remounts, and a trust grant can replace the whole // load result mid-session, so the host owns them rather than the bootstrap. - const extensionsRef = useRef(bootstrap.extensions); + const extensionsRef = useRef(initialBootstrap.extensions as ExtensionLoadResult | undefined); // Experimental capabilities are launch authority: remote/watch reloads may replace content, // but opting in or out requires starting a new Hunk process. - const launchExperimental = bootstrap.input.options.experimental === true; + const launchExperimental = initialBootstrap.input.options.experimental === true; // Extension authority is launch authority for the same reason. A reload command // names *content* to reopen — `hunk session reload -- diff` — and is parsed // fresh, so it carries none of the extension flags the session was launched @@ -54,10 +70,10 @@ export function AppHost({ // `--extension` paths silently stop loading. Both are captured raw: `undefined` // means "no flag given", which must keep deferring to the config layers rather // than becoming an explicit choice. - const launchExtensionsEnabled = bootstrap.input.options.extensions; - const launchExtensionPaths = bootstrap.input.options.extensionPaths; + const launchExtensionsEnabled = initialBootstrap.input.options.extensions; + const launchExtensionPaths = initialBootstrap.input.options.extensionPaths; const [sessionFileBounds] = useState(() => - createSessionReloadBounds(bootstrap, { cwd: bootstrap.reloadContext.cwd }), + createSessionReloadBounds(initialBootstrap, { cwd: initialBootstrap.reloadContext.cwd }), ); // Which working directory the current extension set was discovered for. // Discovery is cwd-relative, so a reload that moves the session to another @@ -66,32 +82,46 @@ export function AppHost({ // from the bounds' cwd so it compares against the same resolved form reloads // produce, and a same-directory reload is not mistaken for a move. const extensionsCwdRef = useRef(sessionFileBounds.defaultCwd); + const initialExtensionStartupPendingRef = useRef(true); + const reloadTailRef = useRef>(Promise.resolve()); + const pendingReplacementLifecycleRef = useRef<{ + extensions: ExtensionLoadResult; + cwd: string; + changeset: AppBootstrap["changeset"]; + reason: NonNullable; + resolveMounted: () => void; + } | null>(null); const startupNoticeText = useStartupNotices({ enabled: !activeBootstrap.input.options.pager, notices: activeBootstrap.startupNotices, resolver: startupNoticeResolver, }); - // Extensions that have already received `startup`. The event is a per-extension - // promise, not a per-session one, so a pass that loads extensions later — the - // trust grant, or a reload into another repository — owes `startup` to exactly - // the ones that missed it, and owes nothing to the ones that already had it. - const startedExtensionIdsRef = useRef>(new Set()); - useEffect(() => { // Child effects run before the parent's, so by the time this fires the review - // UI is mounted with its first changeset — which is what `startup` promises. - const extensions = extensionsRef.current; - for (const { id } of extensions?.loaded ?? []) { - startedExtensionIdsRef.current.add(id); + // UI has rendered the matching bootstrap and installed live extension controls. + if (initialExtensionStartupPendingRef.current) { + initialExtensionStartupPendingRef.current = false; + emitExtensionEvent(extensionsRef.current, "startup", { + cwd: initialBootstrap.reloadContext.cwd, + }); + return; } - emitExtensionEvent(extensions, "startup", { - cwd: bootstrap.reloadContext.cwd, + const pending = pendingReplacementLifecycleRef.current; + if (!pending) { + return; + } + pendingReplacementLifecycleRef.current = null; + emitExtensionEvent(pending.extensions, "startup", { cwd: pending.cwd }); + emitExtensionEvent(pending.extensions, "session_reload", { + changeset: pending.changeset, + reason: pending.reason, }); - }, [bootstrap.reloadContext.cwd]); + pending.resolveMounted(); + }, [activeBootstrap, initialBootstrap.reloadContext.cwd]); - const reloadSession = useCallback( + const performReloadSession = useCallback( async (nextInput: CliInput, options?: ReloadSessionOptions) => { // Re-run the same startup normalization pipeline used on first launch so reloads honor // runtime defaults and config layering instead of assuming `nextInput` is already final. @@ -109,71 +139,97 @@ export function AppHost({ const { cwd } = validateSessionReloadWithinBounds(sessionFileBounds, runtimeInput, { sourcePath: options?.sourcePath, }); - const configured = resolveConfiguredCliInput(runtimeInput, { cwd }); - - // Extensions loaded before this pass; used below to tell newly loaded ones apart. - const previouslyLoadedIds = new Set( - (extensionsRef.current?.loaded ?? []).map((extension) => extension.id), - ); - let reloadedExtensions = false; + const baseVcsCatalog = getBundledVcsCatalog(); + const currentExtensions = extensionsRef.current; + const currentAdapters = currentExtensions + ? resolveExtensionVcsAdapters(currentExtensions.registry, baseVcsCatalog).adapters + : []; + const discoveryCatalog = extendVcsCatalog(baseVcsCatalog, currentAdapters); + let configured = resolveConfiguredCliInput(runtimeInput, { + cwd, + vcsCatalog: discoveryCatalog, + }); + let replacementExtensions: ExtensionLoadResult | undefined; if (options?.reloadExtensions || cwd !== extensionsCwdRef.current) { - // A reloaded extension set owns a fresh ephemeral bus. Detach the old - // registry first so delayed callbacks from a retired extension cannot - // keep publishing into listeners that no longer belong to this session. - if (extensionsRef.current) { - extensionsRef.current.registry.emitCustomEvent = undefined; - extensionsRef.current.registry.eventBusPhase = "closed"; - extensionsRef.current.registry.pendingCustomEvents.length = 0; - } - // Reuse the session's notification hub so the mounted toast surface keeps - // receiving `ctx.notify` from the extensions this pass loads. - extensionsRef.current = await loadStartupExtensions({ - extensions: configured.extensions, + const resolvedExtensions = await resolveConfiguredExtensions({ + runtimeInput, + configured, cwd, - cliExtensionPaths: configured.input.options.extensionPaths, - notifications: extensionsRef.current?.notifications, + baseVcsCatalog, + discoveryCatalog, + // Reuse the session hub so the mounted toast surface keeps receiving notifications. + notifications: currentExtensions?.notifications, }); - extensionsCwdRef.current = cwd; - reloadedExtensions = true; + configured = resolvedExtensions.configured; + replacementExtensions = resolvedExtensions.extensions; } - const extensions = extensionsRef.current; - const { - applied, - bootstrap: nextBootstrap, - input: reloadInput, - sessionVcs, - } = await loadConfiguredSessionBootstrap({ - configured, - cwd, - extensions, - loadAtCwd: true, - }); - if (extensions) { - reportExtensionApplyIssues(applied.issues, extensions.context); - } - nextBootstrap.startupNotices = - sessionVcs.unknownVcsId !== undefined - ? [ - ...(configured.startupNotices ?? []), - // Names the backend the reload really used, detection override included. - createUnknownVcsNotice(sessionVcs.unknownVcsId, String(reloadInput.options.vcs)), - ] - : configured.startupNotices; - const nextSnapshot = createInitialSessionSnapshot(nextBootstrap); + const extensions = replacementExtensions ?? currentExtensions; + const preparedReload = await (async () => { + try { + const { + applied, + bootstrap: nextBootstrap, + input: reloadInput, + sessionVcs, + } = await loadConfiguredSessionBootstrap({ + configured, + cwd, + extensions, + loadAtCwd: true, + baseVcsCatalog, + }); + if (extensions) { + reportExtensionApplyIssues(applied.issues, extensions.context); + } + nextBootstrap.startupNotices = + sessionVcs.unknownVcsId !== undefined + ? [ + ...(configured.startupNotices ?? []), + // Names the backend the reload really used, detection override included. + createUnknownVcsNotice(sessionVcs.unknownVcsId, String(reloadInput.options.vcs)), + ] + : configured.startupNotices; + const nextSnapshot = createInitialSessionSnapshot(nextBootstrap); - let sessionId = "local-session"; - if (hostClient) { - // Keep the daemon-facing session registration in sync with whatever the UI is about to - // show. Replacing both registration and snapshot here means external session commands see - // the new source, title, and selection baseline immediately after reload. - const nextRegistration = updateSessionRegistration( - hostClient.getRegistration(), - nextBootstrap, - ); - sessionId = nextRegistration.sessionId; - hostClient.replaceSession(nextRegistration, nextSnapshot); + let sessionId = "local-session"; + if (hostClient) { + // Keep the daemon-facing registration aligned with the review about to mount. + const nextRegistration = updateSessionRegistration( + hostClient.getRegistration(), + nextBootstrap, + ); + sessionId = nextRegistration.sessionId; + hostClient.replaceSession(nextRegistration, nextSnapshot); + } + return { nextBootstrap, nextSnapshot, sessionId }; + } catch (error) { + await retireExtensionLoadResult(replacementExtensions); + throw error; + } + })(); + const { nextBootstrap, nextSnapshot, sessionId } = preparedReload; + + let replacementMounted: Promise | undefined; + let currentExtensionsRetired: Promise | undefined; + if (replacementExtensions) { + // Only retire the visible runtime after its replacement review is known-good. + // Revocation is synchronous so mounted controls and modes become inert + // before shutdown starts; React then tears them down on this state update. + // `retireExtensionLoadResult` revokes synchronously before its first await. + currentExtensionsRetired = retireExtensionLoadResult(currentExtensions); + extensionsRef.current = replacementExtensions; + extensionsCwdRef.current = cwd; + replacementMounted = new Promise((resolveMounted) => { + pendingReplacementLifecycleRef.current = { + extensions: replacementExtensions, + cwd, + changeset: nextBootstrap.changeset, + reason: options?.reason ?? "daemon", + resolveMounted, + }; + }); } setActiveBootstrap(nextBootstrap); @@ -183,31 +239,17 @@ export function AppHost({ setAppVersion((current) => current + 1); } - if (reloadedExtensions) { - // Extensions this pass loaded for the first time — after a trust grant, or - // after moving into another repository — never saw the mount emit, so they - // get `startup` now that the review UI is showing their changeset. Ordered - // before `session_reload` so an extension's own lifecycle stays in sequence. - const newlyLoadedIds = new Set( - (extensions?.loaded ?? []) - .map((extension) => extension.id) - .filter( - (id) => !previouslyLoadedIds.has(id) && !startedExtensionIdsRef.current.has(id), - ), - ); - - for (const id of newlyLoadedIds) { - startedExtensionIdsRef.current.add(id); - } - - emitExtensionEventToExtensions(extensions, "startup", { cwd }, newlyLoadedIds); + if (!replacementExtensions) { + emitExtensionEvent(extensions, "session_reload", { + changeset: nextBootstrap.changeset, + reason: options?.reason ?? "daemon", + }); + } else { + // Keep the reload queue held until React commits the matching App and + // the replacement receives startup/session_reload through live controls. + await Promise.all([replacementMounted, currentExtensionsRetired]); } - emitExtensionEvent(extensions, "session_reload", { - changeset: nextBootstrap.changeset, - reason: options?.reason ?? "daemon", - }); - return { sessionId, inputKind: nextBootstrap.input.kind, @@ -227,6 +269,19 @@ export function AppHost({ ], ); + /** Serialize broker, watch, workspace, and manual reloads around extension replacement. */ + const reloadSession = useCallback( + (nextInput: CliInput, options?: ReloadSessionOptions) => { + const pending = reloadTailRef.current.then(() => performReloadSession(nextInput, options)); + reloadTailRef.current = pending.then( + () => undefined, + () => undefined, + ); + return pending; + }, + [performReloadSession], + ); + /** Give `shutdown` handlers a bounded window, then leave regardless. */ const quitAfterShutdownEvent = useCallback(() => { void emitExtensionEventBounded(extensionsRef.current, "shutdown", {}).finally(onQuit); diff --git a/src/ui/AppHost.workspace.test.tsx b/src/ui/AppHost.workspace.test.tsx index 51dcb9993..22a56ad32 100644 --- a/src/ui/AppHost.workspace.test.tsx +++ b/src/ui/AppHost.workspace.test.tsx @@ -13,11 +13,23 @@ import { afterEach, describe, expect, test } from "bun:test"; import { testRender } from "@opentui/react/test-utils"; import { act } from "react"; import { removeTestDirectory } from "../../test/helpers/filesystem"; -import { loadAppBootstrap } from "../core/loaders"; -import type { AppBootstrap, CliInput } from "../core/types"; +import { loadAppBootstrap as loadCoreAppBootstrap } from "../core/loaders"; + +import type { AppBootstrap } from "../app/types"; +import { getBundledVcsCatalog } from "../app/vcsCatalog"; +import type { CliInput } from "../core/types"; import { loadStartupExtensions } from "../extensions/startup"; import { AppHost } from "./AppHost"; +/** Specialize the core loader result with extension state assigned by these tests. */ +function loadAppBootstrap(...args: Parameters): Promise { + const [input, options] = args; + return loadCoreAppBootstrap(input, { + vcsCatalog: getBundledVcsCatalog(), + ...options, + }) as Promise; +} + /** * `ctx.workspace`, driven through the real app: a fixture extension reads a * reviewed file's document and asks to replace it, Hunk raises the confirm the diff --git a/website/src/content/docs/docs/extend/vcs-adapters.md b/website/src/content/docs/docs/extend/vcs-adapters.md index aff70f9fc..7a4522245 100644 --- a/website/src/content/docs/docs/extend/vcs-adapters.md +++ b/website/src/content/docs/docs/extend/vcs-adapters.md @@ -48,7 +48,7 @@ Detection prefers the **nearest** checkout: a Git repository nested inside a jj | ------------------------ | -------------------------------------------- | | bundled `jj` | 200 | | bundled `sl` | 100 | -| bundled `git` | 0 (`HUNK_CORE_VCS_DETECTION_PRIORITY`) | +| bundled `git` | 0 (`HUNK_VCS_DETECTION_BASELINE_PRIORITY`) | | your adapter, by default | -100 (`HUNK_DEFAULT_VCS_DETECTION_PRIORITY`) | Higher is consulted first; equal priorities fall back to registration order. jj and Sapling sit above Git because a colocated jj repository — or a Sapling repository created with `sl init --git` — also carries Git metadata, and the Git view is the wrong one. @@ -56,19 +56,19 @@ Higher is consulted first; equal priorities fall back to registration order. jj The default puts your adapter below Git, so installing an extension never silently changes how an existing repository is reviewed. Set `detectionPriority` explicitly to outrank a shipped backend; it is your machine. ```ts -import { HUNK_CORE_VCS_DETECTION_PRIORITY } from "hunkdiff/extension"; +import { HUNK_VCS_DETECTION_BASELINE_PRIORITY } from "hunkdiff/extension"; hunk.registerVcsAdapter({ id: "hg", name: "Mercurial", - detectionPriority: HUNK_CORE_VCS_DETECTION_PRIORITY + 10, + detectionPriority: HUNK_VCS_DETECTION_BASELINE_PRIORITY + 10, detect, }); ``` Detection runs the same way for every adapter, whichever tier registered it: the nearest checkout wins, `detectionPriority` breaks ties between adapters that recognize the same root, and equal priorities fall back to registration order. Config resolves the session's VCS before your extension has been imported, so detection runs again once extensions are loaded — with the full adapter list — and that second answer is the one the session uses. -What detection never overrides is an explicit choice: a `vcs = ""` in Hunk config naming a backend this session loaded is honored as-is, however near a checkout some other adapter finds. +What detection never overrides is an explicit choice: a `vcs = ""` in Hunk config naming a backend this session loaded is honored as-is, however near a checkout some other adapter finds. A repository-local adapter can bootstrap a provider Hunk has never seen because `.hunk` itself establishes the project root; global, config-path, and `--extension` adapters also participate in a staged root/config pass before the review loads. ## Watch support @@ -118,7 +118,7 @@ async load(input, ctx) { } ``` -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. 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. +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. ## Files outside the patch