From f6b3d282de05101701a400dc391803ae9bd9aec2 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Mon, 5 Oct 2026 18:25:22 -0400 Subject: [PATCH 1/7] Resolve diffr from @dev.fast/diffr's platform package @dev.fast/diffr 0.1.11 ships the binary as optional per-platform packages, so the server finds diffr with diffrBinaryPath() instead of a bundled bin/diffr or a fetched copy. It becomes a runtime dependency so installs carry the platform package. Agent-Session: 459ae7ec-7302-42c5-b473-4e315a7e4e19 Agent-Session: 96eed70a-4af6-4249-a91d-8064746610b6 Agent-Session: a5134983-fbca-42d2-ac6c-fe14cf3ce20e Agent-Session: 01a0fd55-7d25-70c3-bf87-46cdb881d91e Agent-Session: 9c8a2d13-4198-4d4b-bf1d-4de6fccd0f19 Agent-Session: 99df4638-b570-4eeb-b115-0311294ee04d Agent-Session: 2cfc4ef0-4d24-4442-8eb4-6d35cd498eeb Agent-Session: d9b59ba5-fdb1-49e5-85d8-9ad33d77e66a Agent-Session: 4d2db452-9ddb-42e0-ae76-22080016d1f8 Agent-Session: 095327a8-9eac-4f8f-9008-f88d1feb914e Agent-Session: c155cff4-ccd6-416d-a2e3-f65113f2d163 Agent-Session: ffd763f2-8a62-453f-8087-cb0da647b5fb Agent-Session: 5c0cda00-e828-48d5-822b-821b7a47abe5 Agent-Session: edffabb0-c704-439a-bf69-7b366f2f332d --- apps/review-desktop/package.json | 2 +- packages/review-protocol/package.json | 2 +- packages/review/package.json | 2 +- .../review/src/server/structural-diff.test.ts | 21 +----- packages/review/src/server/structural-diff.ts | 26 ++----- pnpm-lock.yaml | 69 ++++++++++++++++--- pnpm-workspace.yaml | 7 +- 7 files changed, 73 insertions(+), 56 deletions(-) diff --git a/apps/review-desktop/package.json b/apps/review-desktop/package.json index 58f467ca9..0554b815c 100644 --- a/apps/review-desktop/package.json +++ b/apps/review-desktop/package.json @@ -29,7 +29,7 @@ "@dev.fast/whiteboard": "workspace:*" }, "devDependencies": { - "@dev.fast/diffr": "0.1.10", + "@dev.fast/diffr": "0.1.11", "@dev.fast/review-canvas": "workspace:*", "@types/node": "24.12.2", "jsdom": "29.1.1", diff --git a/packages/review-protocol/package.json b/packages/review-protocol/package.json index 1f9ecfe16..bba0229c2 100644 --- a/packages/review-protocol/package.json +++ b/packages/review-protocol/package.json @@ -20,7 +20,7 @@ "typecheck": "pnpm -w exec tsc -p packages/review-protocol/tsconfig.json --noEmit" }, "dependencies": { - "@dev.fast/diffr": "0.1.10", + "@dev.fast/diffr": "0.1.11", "@dev.fast/json": "workspace:*", "@dev.fast/trace-protocol": "workspace:*", "zod": "catalog:" diff --git a/packages/review/package.json b/packages/review/package.json index 6f0d6a2aa..dc1ef9c39 100644 --- a/packages/review/package.json +++ b/packages/review/package.json @@ -64,6 +64,7 @@ "@agentclientprotocol/claude-agent-acp": "0.84.0", "@agentclientprotocol/codex-acp": "2.0.0", "@agentclientprotocol/sdk": "1.5.1", + "@dev.fast/diffr": "0.1.11", "@hono/node-server": "2.0.12", "@img/sharp-wasm32": "0.35.4", "@modelcontextprotocol/sdk": "1.30.0", @@ -82,7 +83,6 @@ "zod": "catalog:" }, "devDependencies": { - "@dev.fast/diffr": "0.1.10", "@dev.fast/json": "workspace:*", "@dev.fast/local-vcs": "^0.1.0", "@dev.fast/review-protocol": "workspace:*", diff --git a/packages/review/src/server/structural-diff.test.ts b/packages/review/src/server/structural-diff.test.ts index 9ed12d9f4..471d85e8b 100644 --- a/packages/review/src/server/structural-diff.test.ts +++ b/packages/review/src/server/structural-diff.test.ts @@ -1,15 +1,11 @@ -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; import type { JsonValue } from "@dev.fast/json"; import { afterEach, expect, test, vi } from "vitest"; -import { - type StructuralDiffRequest, - diffrExecutable, - structuralDiff, -} from "./structural-diff"; +import { type StructuralDiffRequest, structuralDiff } from "./structural-diff"; const roots: string[] = []; @@ -373,19 +369,6 @@ test("coverage can detach after initial files while summaries continue for later } }); -test("uses the bundled binary only when present and no override is set", async () => { - const root = await mkdtemp(path.join(tmpdir(), "review-bundled-diffr-")); - roots.push(root); - vi.stubEnv("REVIEW_DIFFR_BINARY", ""); - expect(diffrExecutable(root)).toBe("diffr"); - await mkdir(path.join(root, "bin")); - const binary = path.join(root, "bin", "diffr"); - await writeFile(binary, "#!/bin/sh\n", { mode: 0o755 }); - expect(diffrExecutable(root)).toBe(binary); - vi.stubEnv("REVIEW_DIFFR_BINARY", "/elsewhere/diffr"); - expect(diffrExecutable(root)).toBe("/elsewhere/diffr"); -}); - test("keeps the ten most recently read idle comparisons", async () => { const { StructuralComparisons } = await import("./structural-comparisons.js"); diff --git a/packages/review/src/server/structural-diff.ts b/packages/review/src/server/structural-diff.ts index 586d55225..e2596c3ee 100644 --- a/packages/review/src/server/structural-diff.ts +++ b/packages/review/src/server/structural-diff.ts @@ -1,17 +1,13 @@ import { spawn } from "node:child_process"; -import { existsSync } from "node:fs"; -import path from "node:path"; import { createInterface } from "node:readline"; +import { diffrBinaryPath } from "@dev.fast/diffr"; import { STRUCTURAL_DIFF_WIRE_VERSION, type StructuralDiffEvent, type StructuralProblem, decodeStructuralDiffEvent, } from "@dev.fast/review-protocol"; -import { findReviewPackageRoot } from "@review/package-paths"; - -import { installedFullDiffr } from "./diffr-languages.js"; export type DiffComparison = | { kind: "trees"; base: string; head: string } @@ -26,27 +22,13 @@ export interface StructuralDiffRequest { signal: AbortSignal; } -export function diffrExecutable( - packageRoot = findReviewPackageRoot(import.meta.url), -): string { - if (process.env.REVIEW_DIFFR_BINARY) return process.env.REVIEW_DIFFR_BINARY; - - const full = installedFullDiffr(packageRoot); - - if (full) return full; - - const bundled = path.join( - packageRoot, - "bin", - process.platform === "win32" ? "diffr.exe" : "diffr", - ); - - return existsSync(bundled) ? bundled : "diffr"; +export function diffrExecutable(): string { + return process.env.REVIEW_DIFFR_BINARY || diffrBinaryPath() || "diffr"; } export function diffrMissingError(): Error { return new Error( - `Cannot find diffr at ${diffrExecutable()}. Whiteboard Desktop bundles it at bin/diffr under its runtime; in a checkout, run \`pnpm --filter @dev.fast/whiteboard ensure:diffr\` or install diffr on PATH, or set REVIEW_DIFFR_BINARY to its executable.`, + `Cannot find diffr at ${diffrExecutable()}. Install diffr on PATH, or set REVIEW_DIFFR_BINARY to its executable.`, ); } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index dbf356132..ef0a11c94 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -51,8 +51,8 @@ importers: version: link:../../packages/review devDependencies: '@dev.fast/diffr': - specifier: 0.1.10 - version: 0.1.10(zod@4.6.5) + specifier: 0.1.11 + version: 0.1.11(zod@4.6.5) '@dev.fast/review-canvas': specifier: workspace:* version: link:../../packages/review/app @@ -119,6 +119,9 @@ importers: '@agentclientprotocol/sdk': specifier: 1.5.1 version: 1.5.1(zod@4.6.5) + '@dev.fast/diffr': + specifier: 0.1.11 + version: 0.1.11(zod@4.6.5) '@hono/node-server': specifier: 2.0.12 version: 2.0.12(hono@4.13.5) @@ -168,9 +171,6 @@ importers: specifier: 'catalog:' version: 4.6.5 devDependencies: - '@dev.fast/diffr': - specifier: 0.1.10 - version: 0.1.10(zod@4.6.5) '@dev.fast/json': specifier: workspace:* version: link:../json @@ -217,8 +217,8 @@ importers: packages/review-protocol: dependencies: '@dev.fast/diffr': - specifier: 0.1.10 - version: 0.1.10(zod@4.6.5) + specifier: 0.1.11 + version: 0.1.11(zod@4.6.5) '@dev.fast/json': specifier: workspace:* version: link:../json @@ -554,10 +554,36 @@ packages: resolution: {integrity: sha512-QxULHAm7cNu72w97JUNCBFODFaXpbDg+dP8b/oWFAZ2MTRppA3U00Y2L1HqaS4J6yBqxwa/Y3nMBaxVKbB/NsA==} engines: {node: '>=20.19.0'} - '@dev.fast/diffr@0.1.10': - resolution: {integrity: sha512-C5bU+lmZ5LRWt3HsgW37lp4cXU+7RVERGmD+50Rwf9+Vi368fYfiP9Brx1o8O13TQui5uTp6fsP8FrpgpOQ/Vw==} + '@dev.fast/diffr-darwin-arm64@0.1.11': + resolution: {integrity: sha512-GcjoOlroyk1gRnHorpxHERooHq41YUBsr84ZHj71yCdQmih6WRGgFBJsSfmXoiie4GRO/kbRgL5tP1ubJhFFUQ==} + cpu: [arm64] + os: [darwin] + + '@dev.fast/diffr-darwin-x64@0.1.11': + resolution: {integrity: sha512-nQ9kj7fsy8U7Xj5QH2ZBUB9ie/htfzi0X6nVh/6/VlQUJqFWe0CnsyAx/R5k/oNH129n9BnOvzJXMDtv1gTbCg==} + cpu: [x64] + os: [darwin] + + '@dev.fast/diffr-linux-arm64@0.1.11': + resolution: {integrity: sha512-+K8nTmVkc2wVY4lKy6UoH+Sv7rIqbh43w5YqVyiXThHYPd6qY1/jqbtcNhAELeUfPb4S7u1PCAQDK0/u+Pluow==} + cpu: [arm64] + os: [linux] + libc: [glibc] + + '@dev.fast/diffr-linux-x64@0.1.11': + resolution: {integrity: sha512-gZrI3C8BubT/q/SnMF85SjQUJU5dXLiLkNZsYy/q4xilOT+muv7i7eRw808771bQ8lgWanBfDNLx0cq3XY/5BA==} + cpu: [x64] + os: [linux] + libc: [glibc] + + '@dev.fast/diffr-win32-x64@0.1.11': + resolution: {integrity: sha512-WHE1daqKnF7Uo/NqvevSEB7IiWWIsFHvYehujwaMWM+5GpJNtz/rI8o9aWGrf1mDcICQwTAaAw3QhZHDY31faw==} + cpu: [x64] + os: [win32] + + '@dev.fast/diffr@0.1.11': + resolution: {integrity: sha512-bG7zvvv5MaWIisUarPuOrTb8KI87HGemhci8sMdsVMJ5bCFN5Ml6D57iOMqzwsGMwZqbWJ8ViHVZzb9f6RAA1g==} engines: {node: '>=20'} - hasBin: true peerDependencies: zod: ^4.0.0 @@ -3426,9 +3452,30 @@ snapshots: '@csstools/css-tokenizer@4.0.0': {} - '@dev.fast/diffr@0.1.10(zod@4.6.5)': + '@dev.fast/diffr-darwin-arm64@0.1.11': + optional: true + + '@dev.fast/diffr-darwin-x64@0.1.11': + optional: true + + '@dev.fast/diffr-linux-arm64@0.1.11': + optional: true + + '@dev.fast/diffr-linux-x64@0.1.11': + optional: true + + '@dev.fast/diffr-win32-x64@0.1.11': + optional: true + + '@dev.fast/diffr@0.1.11(zod@4.6.5)': dependencies: zod: 4.6.5 + optionalDependencies: + '@dev.fast/diffr-darwin-arm64': 0.1.11 + '@dev.fast/diffr-darwin-x64': 0.1.11 + '@dev.fast/diffr-linux-arm64': 0.1.11 + '@dev.fast/diffr-linux-x64': 0.1.11 + '@dev.fast/diffr-win32-x64': 0.1.11 '@dual-bundle/import-meta-resolve@4.2.1': {} diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index ed99b113c..f17b8d77f 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -17,7 +17,12 @@ verifyDepsBeforeRun: false minimumReleaseAge: 10080 minimumReleaseAgeStrict: true minimumReleaseAgeExclude: - - "@dev.fast/diffr@0.1.10" + - "@dev.fast/diffr@0.1.11" + - "@dev.fast/diffr-darwin-arm64@0.1.11" + - "@dev.fast/diffr-darwin-x64@0.1.11" + - "@dev.fast/diffr-linux-arm64@0.1.11" + - "@dev.fast/diffr-linux-x64@0.1.11" + - "@dev.fast/diffr-win32-x64@0.1.11" # Ask tracks the latest ACP SDK and adapters, and the CLI packages they pin. - "@agentclientprotocol/sdk@1.5.1" - "@agentclientprotocol/claude-agent-acp@0.84.0" From 0257247d3bfb4d093001ad698e96389226513f57 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Mon, 5 Oct 2026 18:25:38 -0400 Subject: [PATCH 2/7] Ship Desktop's diffr from the runtime's node_modules pnpm deploy already installs the platform package into the runtime, so staging stops copying bin/diffr and ensure:diffr goes from packaging, run.sh and CI. Signing, the closure check and the smoke scripts use the new path. Agent-Session: 459ae7ec-7302-42c5-b473-4e315a7e4e19 Agent-Session: 96eed70a-4af6-4249-a91d-8064746610b6 Agent-Session: a5134983-fbca-42d2-ac6c-fe14cf3ce20e Agent-Session: 01a0fd55-7d25-70c3-bf87-46cdb881d91e Agent-Session: 9c8a2d13-4198-4d4b-bf1d-4de6fccd0f19 Agent-Session: 99df4638-b570-4eeb-b115-0311294ee04d Agent-Session: 2cfc4ef0-4d24-4442-8eb4-6d35cd498eeb Agent-Session: d9b59ba5-fdb1-49e5-85d8-9ad33d77e66a Agent-Session: 4d2db452-9ddb-42e0-ae76-22080016d1f8 Agent-Session: 095327a8-9eac-4f8f-9008-f88d1feb914e Agent-Session: c155cff4-ccd6-416d-a2e3-f65113f2d163 Agent-Session: ffd763f2-8a62-453f-8087-cb0da647b5fb Agent-Session: 5c0cda00-e828-48d5-822b-821b7a47abe5 Agent-Session: edffabb0-c704-439a-bf69-7b366f2f332d --- .github/workflows/review-desktop-ci.yml | 1 - .gitignore | 1 - .../code-oss/build/darwin/sign.ts | 2 +- .../scripts/linux/nixos/vm-test.nix | 2 +- apps/review-desktop/scripts/lsp-e2e.mjs | 6 ++- apps/review-desktop/scripts/package-linux.sh | 1 - apps/review-desktop/scripts/package-macos.sh | 2 - .../review-desktop/scripts/package-windows.sh | 1 - apps/review-desktop/scripts/run.sh | 7 --- .../scripts/runtime-bundler.test.mjs | 35 ++----------- apps/review-desktop/scripts/smoke-windows.mjs | 4 +- .../scripts/stage-review-runtime.mjs | 52 +------------------ packages/review/package.json | 3 +- .../scripts/bundled-diffr.integration.mjs | 33 +++++++++--- 14 files changed, 42 insertions(+), 108 deletions(-) diff --git a/.github/workflows/review-desktop-ci.yml b/.github/workflows/review-desktop-ci.yml index 80d85b45b..aeb4426bf 100644 --- a/.github/workflows/review-desktop-ci.yml +++ b/.github/workflows/review-desktop-ci.yml @@ -161,7 +161,6 @@ jobs: - name: Bundled diffr integration run: | - pnpm --filter @dev.fast/whiteboard ensure:diffr --required pnpm --filter @dev.fast/whiteboard exec vitest run --config vitest.integration.config.ts - name: Upload browser test diagnostics diff --git a/.gitignore b/.gitignore index 4eefcb39c..c54fa9203 100644 --- a/.gitignore +++ b/.gitignore @@ -91,4 +91,3 @@ artifacts/devbox/ scripts/acceptance/evidence/ packages/review/tutorial/pins.json -packages/review/bin/ diff --git a/apps/review-desktop/code-oss/build/darwin/sign.ts b/apps/review-desktop/code-oss/build/darwin/sign.ts index 23aee9992..742895bc1 100644 --- a/apps/review-desktop/code-oss/build/darwin/sign.ts +++ b/apps/review-desktop/code-oss/build/darwin/sign.ts @@ -19,7 +19,7 @@ function getElectronVersion(): string { } function getEntitlementsForFile(filePath: string): string { - if (filePath.endsWith('/Contents/Resources/app/review-runtime/bin/diffr')) { + if (/\/Contents\/Resources\/app\/review-runtime\/node_modules\/@dev\.fast\/diffr-darwin-(arm64|x64)\/diffr$/.test(filePath)) { return path.join(entitlementsDir, 'diffr.plist'); } if (filePath.includes(' Helper (GPU).app')) { diff --git a/apps/review-desktop/scripts/linux/nixos/vm-test.nix b/apps/review-desktop/scripts/linux/nixos/vm-test.nix index e8c60791d..9de0a7e23 100644 --- a/apps/review-desktop/scripts/linux/nixos/vm-test.nix +++ b/apps/review-desktop/scripts/linux/nixos/vm-test.nix @@ -15,7 +15,7 @@ let targetPkgs = p: package.runtimePackages p ++ [ p.unzip p.file p.binutils p.cargo p.rustc ]; runScript = pkgs.writeShellScript "probe-installed-whiteboard" '' set -eu - ${raw}/resources/app/review-runtime/bin/diffr --version + ${raw}/resources/app/review-runtime/node_modules/@dev.fast/diffr-linux-*/diffr --version mkdir -p "$HOME/rust-extension" unzip -qo ${rustVsix} -d "$HOME/rust-extension" chmod +x "$HOME/rust-extension/extension/server/rust-analyzer" diff --git a/apps/review-desktop/scripts/lsp-e2e.mjs b/apps/review-desktop/scripts/lsp-e2e.mjs index cb37f91b3..4798c0874 100644 --- a/apps/review-desktop/scripts/lsp-e2e.mjs +++ b/apps/review-desktop/scripts/lsp-e2e.mjs @@ -38,9 +38,11 @@ const { values } = parseArgs({ options: { app: { type: "string" }, keep: { type: "boolean" } }, }); +const diffrPackage = `node_modules/@dev.fast/diffr-${process.platform}-${process.arch}/diffr`; + const bundledDiffr = values.app - ? path.join(values.app, "Contents/Resources/app/review-runtime/bin/diffr") - : path.join(workspace, "packages/review/bin/diffr"); + ? path.join(values.app, "Contents/Resources/app/review-runtime", diffrPackage) + : path.join(workspace, diffrPackage); const structuralDiffAvailable = !!process.env.REVIEW_DIFFR_BINARY || diff --git a/apps/review-desktop/scripts/package-linux.sh b/apps/review-desktop/scripts/package-linux.sh index f00ec8ecc..c473fd2ab 100755 --- a/apps/review-desktop/scripts/package-linux.sh +++ b/apps/review-desktop/scripts/package-linux.sh @@ -42,7 +42,6 @@ node "$APP_DIR/scripts/curated-extensions.mjs" \ # The installed app embeds its own Review server runtime (server, CLI, and # docs) so it never reaches back into this checkout. pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard build -pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard ensure:diffr --required node "$APP_DIR/scripts/stage-review-runtime.mjs" --packaged-root "$PACKAGED_ROOT" node "$APP_DIR/scripts/stage-review-runtime.mjs" --verify --packaged-root "$PACKAGED_ROOT" diff --git a/apps/review-desktop/scripts/package-macos.sh b/apps/review-desktop/scripts/package-macos.sh index 82583d3fc..bf9af5f7c 100755 --- a/apps/review-desktop/scripts/package-macos.sh +++ b/apps/review-desktop/scripts/package-macos.sh @@ -79,8 +79,6 @@ node "$APP_DIR/scripts/curated-extensions.mjs" \ # # build.sh produces the server dist in local mode. In precompiled mode, the # server dist arrives in the darwin payload. -# Fetch on Darwin: precompiled payloads were built on Linux. -pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard ensure:diffr --required node "$APP_DIR/scripts/stage-review-runtime.mjs" --packaged-root "$PACKAGED_APP" # Installs Assets.car and rewrites CFBundleIconName, which invalidates any diff --git a/apps/review-desktop/scripts/package-windows.sh b/apps/review-desktop/scripts/package-windows.sh index 1f60a339a..4990f04ac 100644 --- a/apps/review-desktop/scripts/package-windows.sh +++ b/apps/review-desktop/scripts/package-windows.sh @@ -15,7 +15,6 @@ node "$APP_DIR/scripts/copy-canvas.mjs" --packaged-root "$PACKAGED_ROOT" node "$APP_DIR/scripts/curated-extensions.mjs" --target=win32-x64 --copy-to "$PACKAGED_ROOT/resources/app/extensions" # ty links the Visual C++ runtime dynamically and a clean Windows install has none, so ship it beside ty. cp "$(cygpath -u "$SYSTEMROOT")/System32/vcruntime140.dll" "$PACKAGED_ROOT/resources/app/extensions/astral-sh.ty/bundled/libs/bin/" -pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard ensure:diffr --required node "$APP_DIR/scripts/stage-review-runtime.mjs" --packaged-root "$PACKAGED_ROOT" node "$APP_DIR/scripts/stage-review-runtime.mjs" --verify --packaged-root "$PACKAGED_ROOT" npm --prefix "$CHECKOUT" run gulp -- vscode-win32-x64-inno-updater diff --git a/apps/review-desktop/scripts/run.sh b/apps/review-desktop/scripts/run.sh index 7bad815b0..b5e1fddf4 100755 --- a/apps/review-desktop/scripts/run.sh +++ b/apps/review-desktop/scripts/run.sh @@ -71,13 +71,6 @@ if [[ "$(cat "$EXTENSIONS_SELECTION_STAMP" 2>/dev/null)" != "$EXTENSIONS_SELECTI echo "$EXTENSIONS_SELECTION" >"$EXTENSIONS_SELECTION_STAMP" fi -# Development launches use the checkout runtime, not the staged release bundle. -# Check the pinned binary even when all compiled outputs are already current. -# An explicit developer override supplies its own executable. -if [[ -z "$PACKAGED_ROOT" && -z "${REVIEW_DIFFR_BINARY:-}" ]]; then - pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard ensure:diffr --required -fi - rebuild_review_desktop_outputs "$MONOREPO_ROOT" "$REVIEW_PACKAGE" # Names this Desktop as the checkout's dev instance, packaged or not. export DEV_FAST_REVIEW_CHECKOUT="$MONOREPO_ROOT" diff --git a/apps/review-desktop/scripts/runtime-bundler.test.mjs b/apps/review-desktop/scripts/runtime-bundler.test.mjs index 20d9ba016..22106981b 100644 --- a/apps/review-desktop/scripts/runtime-bundler.test.mjs +++ b/apps/review-desktop/scripts/runtime-bundler.test.mjs @@ -13,13 +13,13 @@ import test from "node:test"; import { RUNTIME_CLI_ENTRY, + RUNTIME_DIFFR_ENTRY, assertNoRuntimeBundler, assertPackagedArtifacts, assertRuntimeClosure, assertRuntimeContents, requiredPackagedArtifacts, runtimeRootForPackagedRoot, - stageDiffrBinary, } from "./stage-review-runtime.mjs"; test("runtime closure rejects transitive esbuild packages and native binaries", async () => { @@ -131,7 +131,7 @@ test("final package verification requires the CLI and rechecks archives outside await mkdir(path.dirname(artifact), { recursive: true }); if (artifact === path.join(runtime, "node_modules")) { - await mkdir(artifact); + await mkdir(artifact, { recursive: true }); } else { await writeFile(artifact, artifact.endsWith(".json") ? "{}\n" : ""); } @@ -158,12 +158,12 @@ test("final package verification requires the CLI and rechecks archives outside await assertRuntimeClosure(runtime); await assertPackagedArtifacts(packaged); - const diffr = path.join(runtime, "bin", "diffr"); + const diffr = path.join(runtime, RUNTIME_DIFFR_ENTRY); await rm(diffr, { force: true }); - await assert.rejects(assertRuntimeClosure(runtime), /missing bin\/diffr/); + await assert.rejects(assertRuntimeClosure(runtime), /missing node_modules\/@dev\.fast\/diffr-/); await assert.rejects( assertPackagedArtifacts(packaged), - /missing .*bin\/diffr/, + /missing .*diffr-/, ); await writeFile(diffr, ""); @@ -188,31 +188,6 @@ test("final package verification requires the CLI and rechecks archives outside } }); -test("diffr staging rejects missing and stale downloads", async () => { - const root = await mkdtemp(path.join(os.tmpdir(), "review-diffr-stage-")); - const runtime = path.join(root, "review-runtime"); - const source = path.join(root, "diffr"); - - try { - await assert.rejects(stageDiffrBinary(runtime, source), /ensure:diffr/); - await writeFile(source, "#!/bin/sh\necho stale\n", { mode: 0o755 }); - await assert.rejects( - stageDiffrBinary(runtime, source), - /missing or not diffr/, - ); - await writeFile( - path.join(root, "diffr.stamp.json"), - JSON.stringify({ version: "0.0.0", target: "wrong-target" }), - ); - await assert.rejects( - stageDiffrBinary(runtime, source), - /missing or not diffr/, - ); - } finally { - await rm(root, { recursive: true, force: true }); - } -}); - function asarHeader(name) { const json = Buffer.from( JSON.stringify({ files: { [name]: { files: {} } } }), diff --git a/apps/review-desktop/scripts/smoke-windows.mjs b/apps/review-desktop/scripts/smoke-windows.mjs index e7d781f96..986c0be1f 100644 --- a/apps/review-desktop/scripts/smoke-windows.mjs +++ b/apps/review-desktop/scripts/smoke-windows.mjs @@ -207,7 +207,7 @@ export async function smokeWindows(app, evidence) { assert.match(version, /\d+\.\d+\.\d+/); const diffr = execFileSync( - path.join(runtime, "bin/diffr.exe"), + path.join(runtime, "node_modules/@dev.fast/diffr-win32-x64/diffr.exe"), ["--version"], { encoding: "utf8", timeout: 30000 }, ); @@ -251,7 +251,7 @@ export async function smokeWindows(app, evidence) { const head = git("rev-parse", "HEAD").trim(); const stream = execFileSync( - path.join(runtime, "bin/diffr.exe"), + path.join(runtime, "node_modules/@dev.fast/diffr-win32-x64/diffr.exe"), [ "--repo", repository, diff --git a/apps/review-desktop/scripts/stage-review-runtime.mjs b/apps/review-desktop/scripts/stage-review-runtime.mjs index a02fe8543..675b38772 100644 --- a/apps/review-desktop/scripts/stage-review-runtime.mjs +++ b/apps/review-desktop/scripts/stage-review-runtime.mjs @@ -2,9 +2,7 @@ import { execFile } from "node:child_process"; import { access, chmod, - copyFile, cp, - mkdir, open, readFile, readdir, @@ -12,7 +10,6 @@ import { rm, stat, } from "node:fs/promises"; -import { createRequire } from "node:module"; import path from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; import { promisify } from "node:util"; @@ -21,7 +18,7 @@ import { pruneReviewRuntime } from "./prune-review-runtime.mjs"; const execFileAsync = promisify(execFile); -const diffrName = process.platform === "win32" ? "diffr.exe" : "diffr"; +export const RUNTIME_DIFFR_ENTRY = `node_modules/@dev.fast/diffr-${process.platform}-${process.arch}/${process.platform === "win32" ? "diffr.exe" : "diffr"}`; const appDirectory = path.resolve( path.dirname(fileURLToPath(import.meta.url)), @@ -48,10 +45,7 @@ export const REQUIRED_RUNTIME_ENTRIES = [ "THIRD_PARTY_NOTICES.md", RUNTIME_SERVER_ENTRY, RUNTIME_CLI_ENTRY, - `bin/${diffrName}`, - "bin/diffr-package/bin/fetch.mjs", - "bin/diffr-package/package.json", - "bin/diffr-package/pins.json", + RUNTIME_DIFFR_ENTRY, "dist/cli.js", // The build's commit; without it the server reports `commit: null`. "dist/build-info.json", @@ -147,7 +141,6 @@ export async function stageReviewRuntime(packagedRoot) { ); await stageReviewDocs(runtimeRoot); - await stageDiffrBinary(runtimeRoot); await makeTreeOwnerWritable(path.join(runtimeRoot, "tutorial", "git-stub")); console.log("[runtime pruning]", await pruneReviewRuntime(runtimeRoot)); await assertRuntimeClosure(runtimeRoot); @@ -173,47 +166,6 @@ export async function stageReviewDocs( return destination; } -export async function stageDiffrBinary( - runtimeRoot, - source = path.join(monorepoRoot, "packages", "review", "bin", diffrName), -) { - if (!(await stat(source).catch(() => null))?.isFile()) { - throw new Error( - `Missing ${source}. Run pnpm --filter @dev.fast/whiteboard ensure:diffr before packaging.`, - ); - } - - const require = createRequire( - path.join(monorepoRoot, "packages/review/package.json"), - ); - - const packageRoot = path.dirname( - require.resolve("@dev.fast/diffr/package.json"), - ); - - await execFileAsync(process.execPath, [ - path.join(packageRoot, "bin/fetch.mjs"), - "--check", - "--into", - path.dirname(source), - ]); - - const destination = path.join(runtimeRoot, "bin", diffrName); - await mkdir(path.dirname(destination), { recursive: true }); - await copyFile(source, destination); - const installerRoot = path.join(runtimeRoot, "bin/diffr-package"); - await mkdir(path.join(installerRoot, "bin"), { recursive: true }); - - for (const file of ["package.json", "pins.json", "bin/fetch.mjs"]) { - await copyFile( - path.join(packageRoot, file), - path.join(installerRoot, file), - ); - } - - await chmod(destination, 0o755); -} - async function assertMatchingFileTrees(sourceRoot, destinationRoot) { const [sourceFiles, destinationFiles] = await Promise.all([ listRelativeFiles(sourceRoot), diff --git a/packages/review/package.json b/packages/review/package.json index dc1ef9c39..d844b03bf 100644 --- a/packages/review/package.json +++ b/packages/review/package.json @@ -41,7 +41,6 @@ }, "scripts": { "prebuild": "pnpm run build:workspace-deps", - "ensure:diffr": "diffr-fetch --into bin", "build": "node --max-old-space-size=8192 node_modules/tsdown/dist/run.mjs", "build:tutorial-assets": "tsx scripts/build-tutorial-assets.ts", "ensure:tutorial-assets": "tsx scripts/ensure-tutorial-assets.ts", @@ -57,7 +56,7 @@ "test:node": "vitest run --config vitest.config.ts --project shared-module-graph", "test:telemetry-privacy": "vitest run --config vitest.config.ts src/ui-telemetry-events.test.ts src/error-telemetry.test.ts", "typecheck": "pnpm -w exec tsc --noEmit -p packages/review", - "test:integration:diffr": "pnpm run build:workspace-deps && pnpm run ensure:diffr --required && vitest run --config vitest.integration.config.ts", + "test:integration:diffr": "pnpm run build:workspace-deps && vitest run --config vitest.integration.config.ts", "whiteboard": "tsx src/cli.ts" }, "dependencies": { diff --git a/packages/review/scripts/bundled-diffr.integration.mjs b/packages/review/scripts/bundled-diffr.integration.mjs index cc1ff0fee..12042441f 100644 --- a/packages/review/scripts/bundled-diffr.integration.mjs +++ b/packages/review/scripts/bundled-diffr.integration.mjs @@ -1,15 +1,15 @@ import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; import { existsSync } from "node:fs"; -import { copyFile, mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { copyFile, cp, mkdir, readFile, rm, writeFile } from "node:fs/promises"; import { createServer } from "node:http"; +import { createRequire } from "node:module"; import path from "node:path"; import { pathToFileURL } from "node:url"; import { build } from "tsdown"; import { afterAll, beforeAll, describe, test } from "vitest"; -import { stageDiffrBinary } from "../../../apps/review-desktop/scripts/stage-review-runtime.mjs"; import { cleanupTempDirs, gitRepository, @@ -22,7 +22,26 @@ let structuralDiff, saveDiffrSummarizer, testDiffrSummarizer; -const source = path.resolve(import.meta.dirname, "../bin/diffr"); +const diffrPackage = path.dirname( + createRequire(import.meta.url).resolve("@dev.fast/diffr/package.json"), +); + +const platformPackage = path.dirname( + createRequire(path.join(diffrPackage, "package.json")).resolve( + `@dev.fast/diffr-${process.platform}-${process.arch}/package.json`, + ), +); + +const source = path.join(platformPackage, "diffr"); + +async function stageDiffr(runtime) { + for (const directory of [diffrPackage, platformPackage]) + await cp( + directory, + path.join(runtime, "node_modules/@dev.fast", path.basename(directory)), + { recursive: true, dereference: true }, + ); +} async function collect(repositoryPath, base, head, paths, kind = "trees") { return Array.fromAsync( @@ -100,7 +119,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", git("add", "-A"); git("commit", "-qm", "head"); head = git("rev-parse", "HEAD"); - await stageDiffrBinary(runtime, source); + await stageDiffr(runtime); await build({ config: false, entry: { @@ -318,7 +337,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("an unbundled installation falls back to PATH", async () => { delete process.env.REVIEW_DIFFR_BINARY; - await rm(path.join(runtime, "bin"), { recursive: true, force: true }); + await rm(path.join(runtime, "node_modules"), { recursive: true, force: true }); await assert.rejects( collect(repository, base, head), /diffr exited with 97/, @@ -329,7 +348,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", describe("a working host installation coexists with the Desktop bundle", () => { let called, launcher; beforeAll(async () => { - await stageDiffrBinary(runtime, source); + await stageDiffr(runtime); const host = path.join(root, "host"); const traced = path.join(root, "traced-host"); called = path.join(root, "host-called"); @@ -370,7 +389,7 @@ process.exit(result.status ?? 1); test("without a bundle the host binary runs real diffs and settings", async () => { delete process.env.REVIEW_DIFFR_BINARY; - await rm(path.join(runtime, "bin"), { recursive: true, force: true }); + await rm(path.join(runtime, "node_modules"), { recursive: true, force: true }); successfulFiles(await collect(repository, base, head), 3); assert.equal(existsSync(called), true); await rm(called); From a7135eaa47e600a0c664dbb2860a7f461f38fd7f Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Mon, 5 Oct 2026 18:25:44 -0400 Subject: [PATCH 3/7] Remove the full-edition diffr install The platform package is the full edition, so the command, its endpoint and journey, and the separate install under ~/.cache/diffr are no longer needed. Agent-Session: 459ae7ec-7302-42c5-b473-4e315a7e4e19 Agent-Session: 96eed70a-4af6-4249-a91d-8064746610b6 Agent-Session: a5134983-fbca-42d2-ac6c-fe14cf3ce20e Agent-Session: 01a0fd55-7d25-70c3-bf87-46cdb881d91e Agent-Session: 9c8a2d13-4198-4d4b-bf1d-4de6fccd0f19 Agent-Session: 99df4638-b570-4eeb-b115-0311294ee04d Agent-Session: 2cfc4ef0-4d24-4442-8eb4-6d35cd498eeb Agent-Session: d9b59ba5-fdb1-49e5-85d8-9ad33d77e66a Agent-Session: 4d2db452-9ddb-42e0-ae76-22080016d1f8 Agent-Session: 095327a8-9eac-4f8f-9008-f88d1feb914e Agent-Session: c155cff4-ccd6-416d-a2e3-f65113f2d163 Agent-Session: ffd763f2-8a62-453f-8087-cb0da647b5fb Agent-Session: 5c0cda00-e828-48d5-822b-821b7a47abe5 Agent-Session: edffabb0-c704-439a-bf69-7b366f2f332d --- .../reviewCuratedExtensions.contribution.ts | 22 --- .../e2e/journeys/extra-diff-languages.mjs | 146 ------------------ packages/review/src/server/desktop-server.ts | 13 -- .../review/src/server/diffr-languages.test.ts | 54 ------- packages/review/src/server/diffr-languages.ts | 97 ------------ 5 files changed, 332 deletions(-) delete mode 100644 apps/review-desktop/scripts/e2e/journeys/extra-diff-languages.mjs delete mode 100644 packages/review/src/server/diffr-languages.test.ts delete mode 100644 packages/review/src/server/diffr-languages.ts diff --git a/apps/review-desktop/code-oss/src/vs/review/contrib/extensions/reviewCuratedExtensions.contribution.ts b/apps/review-desktop/code-oss/src/vs/review/contrib/extensions/reviewCuratedExtensions.contribution.ts index eae3f8686..2368f2768 100644 --- a/apps/review-desktop/code-oss/src/vs/review/contrib/extensions/reviewCuratedExtensions.contribution.ts +++ b/apps/review-desktop/code-oss/src/vs/review/contrib/extensions/reviewCuratedExtensions.contribution.ts @@ -30,9 +30,6 @@ import { IProgressService, ProgressLocation } from '../../../platform/progress/c import { IQuickInputService, type IQuickPickItem } from '../../../platform/quickinput/common/quickInput.js'; import { Registry } from '../../../platform/registry/common/platform.js'; import { IStorageService, StorageScope, StorageTarget } from '../../../platform/storage/common/storage.js'; -import { IReviewDesktopConnectionService } from '../../services/reviewDesktopConnectionService.js'; -import { IEditorService } from '../../../workbench/services/editor/common/editorService.js'; -import { ReviewCanvasEditorPane } from '../../browser/parts/canvas/reviewCanvasPart.js'; import { LifecyclePhase } from '../../../workbench/services/lifecycle/common/lifecycle.js'; import { Extensions as WorkbenchExtensions, @@ -740,22 +737,3 @@ Registry.as(WorkbenchExtensions.Workbench).regi OptionalExtensionPinUpgrades, LifecyclePhase.Eventually ); - -class InstallExtraDiffLanguagesAction extends Action2 { - constructor() { - super({ id: 'review.installExtraDiffLanguages', title: localize2('review.diff.installLanguages', "Install extra diffr languages"), f1: true }); - } - override async run(accessor: ServicesAccessor): Promise { - const connection = accessor.get(IReviewDesktopConnectionService); - const progress = accessor.get(IProgressService); - const editors = accessor.get(IEditorService); - await progress.withProgress({ location: ProgressLocation.Notification, title: localize('review.diff.installingLanguages', "Installing extra diffr languages..."), delay: 0 }, async () => { - const { serverUrl, token } = await connection.getConnection(); - const response = await fetch(`${serverUrl}/diffr-languages/install`, { method: 'POST', headers: { 'x-review-token': token } }); - if (!response.ok) throw new Error((await response.json()).error); - const pane = editors.activeEditorPane; - if (pane instanceof ReviewCanvasEditorPane) await pane.refreshComparison(); - }); - } -} -registerAction2(InstallExtraDiffLanguagesAction); diff --git a/apps/review-desktop/scripts/e2e/journeys/extra-diff-languages.mjs b/apps/review-desktop/scripts/e2e/journeys/extra-diff-languages.mjs deleted file mode 100644 index d89c3f684..000000000 --- a/apps/review-desktop/scripts/e2e/journeys/extra-diff-languages.mjs +++ /dev/null @@ -1,146 +0,0 @@ -import assert from "node:assert/strict"; -import { writeFile } from "node:fs/promises"; -import path from "node:path"; - -import { createReview } from "../harness.mjs"; - -export const name = "extra-diff-languages"; - -export const phase = 1; - -export const options = { - settings: { "review.experimental.structuralDiff.enabled": true }, - env: { - REVIEW_DIFFR_BINARY: "", - GEMINI_API_KEY: "", - GOOGLE_API_KEY: "", - OPENAI_API_KEY: "", - ANTHROPIC_API_KEY: "", - }, - async beforeLaunch(ctx) { - ctx.env.XDG_CACHE_HOME = path.join(ctx.root, "cache"); - const file = path.join(ctx.repo, "example.jl"); - await writeFile(file, "function example()\n value = 1\nend\n"); - await ctx.git("add", "."); - await ctx.git( - "-c", - "commit.gpgsign=false", - "commit", - "-qm", - "Before optional grammar", - ); - ctx.base = await ctx.git("rev-parse", "HEAD"); - await writeFile(file, "function example()\n value = 2\nend\n"); - await ctx.git( - "-c", - "commit.gpgsign=false", - "commit", - "-qam", - "After optional grammar", - ); - ctx.head = await ctx.git("rev-parse", "HEAD"); - }, -}; - -export async function run(ctx) { - const review = await createReview(ctx, { - title: "Extra diffr languages", - blocks: [ - { - type: "code_peek", - source: { - file: "example.jl", - start: { side: "head", line: 1 }, - end: { side: "head", line: 1 }, - }, - }, - ], - }); - - const before = await ctx.apiOk(`/reviews-api/${review.reviewId}?full=true`); - - const compare = async () => { - const response = await fetch( - new URL( - `/reviews-api/${review.reviewId}/structural-diff`, - ctx.discovery.url, - ), - { headers: { "x-review-token": ctx.discovery.token } }, - ); - - assert.equal(response.status, 200); - - return (await response.text()) - .trim() - .split("\n") - .map((line) => JSON.parse(line)) - .find( - (event) => - event.type === "file" && - (event.file.rhs ?? event.file.lhs).path === "example.jl", - ).diff; - }; - - assert.ok((await compare()).stats.fallback); - - await review.canvas - .getByRole("button", { name: "Diff", exact: true }) - .click(); - - let navigations = 0; - let installed = false; - - const installationResponse = (response) => { - if (response.url().endsWith("/diffr-languages/install") && response.ok()) - installed = true; - }; - - ctx.page.on("response", installationResponse); - - const navigated = (frame) => { - if (frame === ctx.page.mainFrame()) navigations++; - }; - - ctx.page.on("framenavigated", navigated); - - const refreshed = ctx.page.waitForResponse( - (response) => - installed && - response - .url() - .includes(`/reviews-api/${review.reviewId}/structural-diff`), - { timeout: 180000 }, - ); - - await ctx.page.keyboard.press("F1"); - const palette = ctx.page.locator(".quick-input-widget input").first(); - await palette.fill(">Install extra diffr languages"); - await ctx.page - .locator(".quick-input-list .monaco-list-row") - .filter({ hasText: "Install extra diffr languages" }) - .first() - .click(); - const response = await refreshed; - assert.equal(response.status(), 200); - - const structural = await compare(); - - assert.equal(structural.stats.fallback, undefined); - assert.equal(structural.rhs.text, "function example()\n value = 2\nend\n"); - await review.canvas - .getByText("example.jl", { exact: true }) - .first() - .waitFor(); - assert.equal(navigations, 0, "installation must not reload the window"); - ctx.page.off("framenavigated", navigated); - ctx.page.off("response", installationResponse); - await ctx.page.screenshot({ - path: path.join(ctx.root, "extra-diff-languages-ready.png"), - }); - const after = await ctx.apiOk(`/reviews-api/${review.reviewId}?full=true`); - assert.deepEqual(after, before); - ctx.check( - "extra languages: command palette installs the full edition and refreshes the current review without reloading the window", - "extra languages: pinned review identity and content remain unchanged", - ); -} diff --git a/packages/review/src/server/desktop-server.ts b/packages/review/src/server/desktop-server.ts index 9c62c0887..7c8446ba9 100644 --- a/packages/review/src/server/desktop-server.ts +++ b/packages/review/src/server/desktop-server.ts @@ -62,7 +62,6 @@ import { setDiffrConfigValue, testDiffrSummarizer, } from "./diffr-config"; -import { installFullDiffr } from "./diffr-languages.js"; import { GlobalReviewDesktopVerbRelay, type ReviewDesktopVerbRelay, @@ -397,18 +396,6 @@ export function createGlobalReviewServer( return serverJson(200, { ok: true }); }); - app.post("/diffr-languages/install", async () => { - try { - await installFullDiffr(); - invalidateStructuralComparisons(); - - return serverJson(200, { installed: true }); - } catch (error) { - return serverJson(503, { - error: error instanceof Error ? error.message : String(error), - }); - } - }); app.get("/diffr-config", async () => serverJson(200, await readDiffrConfig()), diff --git a/packages/review/src/server/diffr-languages.test.ts b/packages/review/src/server/diffr-languages.test.ts deleted file mode 100644 index ac3efe9c9..000000000 --- a/packages/review/src/server/diffr-languages.test.ts +++ /dev/null @@ -1,54 +0,0 @@ -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; -import os from "node:os"; -import path from "node:path"; - -import { afterEach, expect, test, vi } from "vitest"; - -import { installFullDiffr } from "./diffr-languages.js"; -import { diffrExecutable } from "./structural-diff.js"; - -let temporary: string; - -afterEach(async () => { - vi.unstubAllEnvs(); - - if (temporary) await rm(temporary, { recursive: true, force: true }); -}); - -test("delegates full installation once and selects its cache outside the runtime", async () => { - temporary = await mkdtemp(path.join(os.tmpdir(), "whiteboard-diffr-")); - vi.stubEnv("XDG_CACHE_HOME", path.join(temporary, "cache")); - vi.stubEnv("REVIEW_DIFFR_BINARY", ""); - const root = path.join(temporary, "runtime"); - const installer = path.join(root, "bin/diffr-package"); - await mkdir(path.join(installer, "bin"), { recursive: true }); - await writeFile( - path.join(installer, "package.json"), - JSON.stringify({ version: "0.1.10" }), - ); - const calls = path.join(temporary, "calls"); - await writeFile( - path.join(installer, "bin/fetch.mjs"), - ` - import { appendFileSync, chmodSync, mkdirSync, writeFileSync } from "node:fs"; - import path from "node:path"; - if (!process.argv.includes("--full") || !process.argv.includes("--required")) throw new Error("full verified installation required"); - appendFileSync(${JSON.stringify(calls)}, "install\\n"); - const directory = process.argv[process.argv.indexOf("--into") + 1]; - mkdirSync(directory, { recursive: true }); - const binary = path.join(directory, process.platform === "win32" ? "diffr.exe" : "diffr"); - writeFileSync(binary, "installed"); - chmodSync(binary, 0o755); - writeFileSync(path.join(directory, "diffr.stamp.json"), JSON.stringify({ version: "0.1.10", full: true })); - `, - ); - expect(diffrExecutable(root)).toBe("diffr"); - await Promise.all([installFullDiffr(root), installFullDiffr(root)]); - expect(await readFile(calls, "utf8")).toBe("install\n"); - const binary = diffrExecutable(root); - expect(binary.startsWith(root)).toBe(false); - expect(await readFile(binary, "utf8")).toBe("installed"); - vi.stubEnv("REVIEW_DIFFR_BINARY", "/explicit/diffr"); - expect(diffrExecutable(root)).toBe("/explicit/diffr"); - await expect(installFullDiffr(root)).rejects.toThrow("explicitly selected"); -}); diff --git a/packages/review/src/server/diffr-languages.ts b/packages/review/src/server/diffr-languages.ts deleted file mode 100644 index dc9a992f5..000000000 --- a/packages/review/src/server/diffr-languages.ts +++ /dev/null @@ -1,97 +0,0 @@ -import { execFile } from "node:child_process"; -import { lstatSync, readFileSync } from "node:fs"; -import os from "node:os"; -import path from "node:path"; -import { promisify } from "node:util"; - -import { findReviewPackageRoot } from "@review/package-paths"; -import { z } from "zod"; - -const exec = promisify(execFile); - -const versionSchema = z.object({ version: z.string() }); - -const stampSchema = z.object({ version: z.string(), full: z.boolean() }); - -const pending = new Map>(); - -function configuration(root: string) { - const installer = path.join(root, "bin/diffr-package/bin/fetch.mjs"); - - const { version } = versionSchema.parse( - JSON.parse( - readFileSync(path.join(root, "bin/diffr-package/package.json"), "utf8"), - ), - ); - - const directory = path.join( - process.env.XDG_CACHE_HOME || path.join(os.homedir(), ".cache"), - "diffr", - version, - `${process.platform}-${process.arch}`, - "full", - ); - - return { - installer, - version, - directory, - binary: path.join( - directory, - process.platform === "win32" ? "diffr.exe" : "diffr", - ), - }; -} - -export function installedFullDiffr(root: string): string | undefined { - try { - const { binary, directory, version } = configuration(root); - const stat = lstatSync(binary); - - const stamp = stampSchema.parse( - JSON.parse( - readFileSync(path.join(directory, "diffr.stamp.json"), "utf8"), - ), - ); - - if ( - stat.isFile() && - (process.platform === "win32" || stat.mode & 0o111) && - stamp.full && - stamp.version === version - ) - return binary; - } catch {} - - return undefined; -} - -export function installFullDiffr( - root = findReviewPackageRoot(import.meta.url), -): Promise { - if (process.env.REVIEW_DIFFR_BINARY) - return Promise.reject( - new Error( - "An explicitly selected diffr executable is in use. Install its full edition separately.", - ), - ); - const { installer, directory } = configuration(root); - const current = pending.get(directory); - - if (current) return current; - - const task = exec( - process.execPath, - [installer, "--full", "--required", "--into", directory], - { - env: { ...process.env, ELECTRON_RUN_AS_NODE: "1" }, - timeout: 180_000, - }, - ) - .then(() => {}) - .finally(() => pending.delete(directory)); - - pending.set(directory, task); - - return task; -} From 7434f1c6892a5409758f581821bf788f82346951 Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Tue, 6 Oct 2026 16:31:23 -0700 Subject: [PATCH 4/7] Integrate diffr v2 settings and migrate saved config at startup Move settings to native shape/classify keys and consume the current tree-based wire protocol. Preserve supported saved overrides and custom prompts with a validated, backed-up migration of the shared config file. The final 0.1.13 npm pin remains blocked on review and publication of devdotfast/diffr#92. Keep this PR in draft until published package validation completes. AI assistance: implemented and validated with OpenAI Codex. Agent-Session: 01a10ff2-f5c2-7ec1-852f-8ebe516d8af9 --- .../common/reviewStructuralDiff.test.ts | 4 +- .../vs/review/common/reviewStructuralDiff.ts | 8 +- .../review/services/reviewDiffViewService.ts | 11 +- .../reviewStructuralDiffSession.test.ts | 140 +---- .../services/reviewStructuralDiffSession.ts | 53 +- apps/review-desktop/package.json | 2 +- .../scripts/e2e/journeys/diffr-settings.mjs | 6 +- apps/review-desktop/scripts/smoke-windows.mjs | 1 - packages/review-protocol/package.json | 2 +- packages/review-protocol/src/contracts.ts | 2 + .../review-protocol/src/source-alignment.ts | 10 +- .../src/diffr-config-section.browser.test.tsx | 178 ++++-- .../review/app/src/diffr-config-section.tsx | 119 ++-- packages/review/package.json | 3 +- .../scripts/bundled-diffr.integration.mjs | 135 ++++- .../src/review-api/comparison-coverage.ts | 17 +- .../src/review-api/folded-progress.test.ts | 92 +-- packages/review/src/review-api/lenses.test.ts | 26 +- .../review/src/review-api/review-api.test.ts | 58 +- packages/review/src/server/desktop-server.ts | 3 + .../src/server/diffr-config-migration.test.ts | 533 ++++++++++++++++++ .../src/server/diffr-config-migration.ts | 382 +++++++++++++ .../review/src/server/diffr-config.test.ts | 145 +++-- packages/review/src/server/diffr-config.ts | 165 +++--- .../review/src/server/diffr-startup.test.ts | 107 ++++ packages/review/src/server/headless-host.ts | 4 + .../server/structural-diff-workspace.test.ts | 14 +- .../review/src/server/structural-diff.test.ts | 58 +- packages/review/src/server/structural-diff.ts | 18 +- .../test-fixtures/diffr-ui-overrides.toml | 38 ++ packages/review/src/server/toml-edits.ts | 266 +++++++++ packages/review/vitest.config.ts | 4 + pnpm-lock.yaml | 81 ++- pnpm-workspace.yaml | 14 +- 34 files changed, 2123 insertions(+), 576 deletions(-) create mode 100644 packages/review/src/server/diffr-config-migration.test.ts create mode 100644 packages/review/src/server/diffr-config-migration.ts create mode 100644 packages/review/src/server/diffr-startup.test.ts create mode 100644 packages/review/src/server/test-fixtures/diffr-ui-overrides.toml create mode 100644 packages/review/src/server/toml-edits.ts diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts index 0af2fd2a5..e83efa78e 100644 --- a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts @@ -30,10 +30,10 @@ function leaf(id: number, start: number, end: number, extra: Partial(); const pairs = (left: StructuralRegion, right: StructuralRegion) => { diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts index 7b1136bdb..025931f4c 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts @@ -37,7 +37,7 @@ import { ReviewFilesDiffView, ReviewFilesEditorInput, type ReviewFilesEditorEntr import { ReviewEmbeddedEditors } from "./reviewEmbeddedEditors.js"; import { createStructuralDiffEditors } from "./reviewStructuralDiff.js"; -import { StructuralDiffSession } from "./reviewStructuralDiffSession.js"; +import { StructuralDiffSession, type StructuralSessionChange } from "./reviewStructuralDiffSession.js"; import type { StructuralDiffStream } from "./reviewStructuralDiffClient.js"; export interface ReviewDiffViewSource { @@ -360,17 +360,12 @@ class DiffViewController extends Disposable implements ReviewDiffViewHandle { private observeSession(session: StructuralDiffSession, entries: readonly ReviewFilesEditorEntry[], view: ReviewFilesDiffView, store: DisposableStore, structuralEnabled: boolean): void { const rendered = new Set(); - const annotationWarnings = new Set(); - const renderCurrentState = (change?: import("./reviewStructuralDiffSession.js").StructuralSessionChange) => { + const renderCurrentState = (change?: StructuralSessionChange) => { if (this.disposed) return; for (const entry of entries) { const path = entry.file.path; - if (change && !change.files.has(path) && !change.labels.has(path)) continue; + if (change && !change.files.has(path)) continue; const result = entry.file.status === "unchanged" ? {} : session.getFileResult(path); - if (result?.annotationError && !annotationWarnings.has(path)) { - annotationWarnings.add(path); - this._onDidError.fire(`Summary unavailable for ${path}: ${result.annotationError}`); - } if (!result || rendered.has(path)) continue; rendered.add(path); if (structuralEnabled && result.hidden !== undefined) view.hideFile(path, result.hidden); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.test.ts index 16d6b83eb..b183fd5ee 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.test.ts @@ -1,13 +1,34 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { StructuralDiffSession } from './reviewStructuralDiffSession.js'; -import type { StructuralEvent } from '../common/reviewStructuralDiff.js'; +import { STRUCTURAL_WIRE_VERSION, type StructuralEvent } from '../common/reviewStructuralDiff.js'; const file = { rhs: { path: 'a.ts', oid: 'abc', mode: '100644' } }; -const start: StructuralEvent = { type: 'start', version: 4, lhs: { type: 'empty_tree' }, rhs: { type: 'revision', rev: 'head' }, files: [{ file, status: 'added' }] }; -const result: StructuralEvent = { type: 'file', file, diff: { type: 'text', structural_changes: { base: [], head: [[0, 1]] }, rhs: { text: 'a', regions: [{ kind: 'leaf', id: 1, fold_state_id: 2, alignment_id: 1, start: { line: 0, column: 0 }, end: { line: 0, column: 1 }, visibility: { collapsed: true } }] }, stats: { textual: { added: 1, removed: 0 }, visible: { added: 0, removed: 0 } } } }; +const start: StructuralEvent = { type: 'start', version: STRUCTURAL_WIRE_VERSION, lhs: { type: 'empty_tree' }, rhs: { type: 'revision', rev: 'head' }, files: [{ file, status: 'added' }] }; +const result: StructuralEvent = { type: 'file', file, diff: { type: 'text', structural_changes: { base: [], head: [[0, 1]] }, rhs: { text: 'a', root: { kind: 'leaf', id: 1, fold_state_id: 2, alignment_id: 1, start: { line: 0, column: 0 }, end: { line: 0, column: 1 }, visibility: { collapsed: true } } }, stats: { textual: { added: 1, removed: 0 }, visible: { added: 0, removed: 0 } } } }; const complete: StructuralEvent = { type: 'complete', succeeded: 1, failed: 0 }; +test('file results supply root visibility and summary labels before comparison completion', async () => { + let release!: () => void; + const gate = new Promise(resolve => release = resolve); + const labeled = structuredClone(result); + if (labeled.type !== 'file' || labeled.diff?.type !== 'text') throw new Error('Invalid fixture'); + labeled.diff.rhs!.root.visibility = { collapsed: true, label: 'Generated file' }; + const session = new StructuralDiffSession({ async *streamComparison() { yield start; yield labeled; await gate; yield complete; } }); + const label = session.regionLabel('a.ts', 1); + const task = session.start(); + try { + const fileResult = await session.fileResult('a.ts'); + assert.equal(fileResult.hidden, 'Generated file'); + assert.equal(label.get(), 'Generated file'); + assert.equal(session.complete, false); + session.setRegionCollapsed('a.ts', 2, false); + release(); await task; + assert.equal(session.isRegionCollapsed('a.ts', 2), false); + assert.equal(session.error, undefined); + } finally { release(); session.dispose(); await task; } +}); + test('views detach and reattach without restarting a comparison or losing folds', async () => { let requests = 0; const session = new StructuralDiffSession({ async *streamComparison() { requests++; yield start; yield result; yield complete; } }); @@ -61,117 +82,6 @@ test('truncation and missing manifest results are visible to later subscribers', } }); -test('deferred annotations retain source identity, counts and user fold choices', async () => { - let release!: () => void; - const gate = new Promise(resolve => release = resolve); - const session = new StructuralDiffSession({ - async *streamComparison() { - yield start; yield structuredClone(result); - await gate; - yield { type: 'annotations', file, annotations: [{ region_id: 1, label: 'summarized' }] } as StructuralEvent; - yield complete; - } - }); - const task = session.start(); - while (!session.getTextDiff('a.ts')) await new Promise(resolve => setTimeout(resolve, 0)); - try { - const initial = session.getTextDiff('a.ts')!; - assert.equal(session.complete, false); - session.setRegionCollapsed('a.ts', 2, false); - release(); await task; - assert.equal(session.getTextDiff('a.ts'), initial); - assert.equal(initial.rhs!.regions![0].visibility?.label, 'summarized'); - assert.equal(initial.rhs!.text, 'a'); - assert.deepEqual(initial.structural_changes, { base: [], head: [[0, 1]] }); - assert.equal(session.isRegionCollapsed('a.ts', 2), false); - assert.equal(session.error, undefined); - } finally { release(); session.dispose(); } -}); - -test('invalid annotation batches are atomic and enrichment failure keeps the initial diff usable', async () => { - for (const invalid of [false, true]) { - const session = new StructuralDiffSession({ - async *streamComparison() { - yield start; yield structuredClone(result); - yield { - type: 'annotations', file, - annotations: invalid ? [{ region_id: 1, label: 'must not apply' }, { region_id: 99, label: 'bad' }] : [], - error: invalid ? undefined : { code: 'enrichment_failed', message: 'offline' }, - } as StructuralEvent; - yield complete; - } - }); - try { - await session.start(); - assert.equal(session.getTextDiff('a.ts')!.rhs!.regions![0].visibility?.label, undefined); - assert.equal(session.isRegionCollapsed('a.ts', 2), true); - if (invalid) assert.match(session.error!, /Unknown annotation region/); - else { - assert.equal(session.getFileResult('a.ts')?.annotationError, 'offline'); - assert.equal(session.error, undefined); - } - } finally { session.dispose(); } - } -}); - -test('streamed labels update existing bands without invalidating either file provider', async () => { - const { StructuralDiffProvider } = await import('./reviewStructuralDiff.js'); - const { URI } = await import('../../base/common/uri.js'); - const { CancellationToken } = await import('../../base/common/cancellation.js'); - const other = { rhs: { path: 'b.ts', oid: 'def', mode: '100644' } }; - let release!: () => void; - const gate = new Promise(resolve => release = resolve); - const session = new StructuralDiffSession({ - async *streamComparison() { - yield { ...start, files: [...start.files, { file: other, status: 'added' as const }] }; - yield structuredClone(result); - yield { ...structuredClone(result), file: other }; - await gate; - yield { type: 'annotations', file, annotations: [{ region_id: 1, label: 'first' }] } as StructuralEvent; - yield { type: 'annotations', file, annotations: [{ region_id: 1, label: '// pseudocode\nfinal summary' }] } as StructuralEvent; - yield { ...complete, succeeded: 2 }; - } - }); - const task = session.start(); - const ready = new Promise(resolve => { - const sub = session.onDidChange(change => { if (change.files.has('b.ts')) { sub.dispose(); resolve(); } }); - }); - try { - await ready; - const models = (path: string) => [ - { uri: URI.parse(`test:/base/${path}`), getLinesContent: () => [''] }, - { uri: URI.parse(`test:/head/${path}`), getLinesContent: () => ['a'] }, - ] as const; - const a = models('a.ts'), b = models('b.ts'); - const pairs = new Map([[`${a[0].uri}\n${a[1].uri}`, 'a.ts'], [`${b[0].uri}\n${b[1].uri}`, 'b.ts']]); - const pa = new StructuralDiffProvider(session, pairs, new Set()); - const pb = new StructuralDiffProvider(session, pairs, new Set()); - const compute = (provider: typeof pa, pair: ReturnType) => provider.computeDiff( - pair[0] as Parameters[0], pair[1] as Parameters[1], - { ignoreTrimWhitespace: false, maxComputationTimeMs: 0, computeMoves: false }, CancellationToken.None, - ); - const diff = await compute(pa, a); - await compute(pb, b); - let invalidationsA = 0, invalidationsB = 0, labelBatches = 0; - const subscriptions = [pa.onDidChange(() => invalidationsA++), pb.onDidChange(() => invalidationsB++), - session.onDidChange(change => { if (change.labels.has('a.ts')) labelBatches++; })]; - const band = diff.contextGaps![0]; - release(); await task; - assert.equal(band.labelObservable!.get(), '// pseudocode\nfinal summary'); - assert.equal(invalidationsA, 0); - assert.equal(invalidationsB, 0); - assert.equal(labelBatches, 1, 'a burst publishes only the latest presentation state'); - const folded = new Promise(resolve => { - const sub = pa.onDidChange(() => { sub.dispose(); resolve(); }); - }); - session.setRegionCollapsed('a.ts', 2, false); - await folded; - assert.equal(invalidationsA, 1); - assert.equal(invalidationsB, 0, 'folding a file does not invalidate unrelated editors'); - subscriptions.forEach(sub => sub.dispose()); - } finally { release(); session.dispose(); await task; } -}); - test('structural editor models retain diffr bytes across a live file save', async () => { const { createStructuralDiffEditors, StructuralDiffProvider } = await import('./reviewStructuralDiff.js'); const { URI } = await import('../../base/common/uri.js'); @@ -188,7 +98,7 @@ test('structural editor models retain diffr bytes across a live file save', asyn yield { ...start, files: [{ file: changedFile, status: 'modified' }] } as StructuralEvent; await gate; yield { type: 'file', file: changedFile, diff: { - type: 'text', lhs: { text: 'base\n' }, rhs: { text: 'saved A\n' }, + type: 'text', lhs: { text: 'base\n', root: {kind:'leaf',id:3,fold_state_id:3,alignment_id:1,start:{line:0,column:0},end:{line:1,column:0}} }, rhs: { text: 'saved A\n', root: {kind:'leaf',id:4,fold_state_id:4,alignment_id:1,start:{line:0,column:0},end:{line:1,column:0}} }, structural_changes: { base: [[0, 1]], head: [[0, 1]] }, stats: { textual: { added: 1, removed: 1 }, visible: { added: 1, removed: 1 } }, } } as StructuralEvent; diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.ts index 77a58274c..b64888fa0 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.ts @@ -1,5 +1,5 @@ import { RunOnceScheduler } from "../../base/common/async.js"; -import { observableValue, transaction, type IObservable, type ISettableObservable } from "../../base/common/observable.js"; +import { observableValue, type IObservable, type ISettableObservable } from "../../base/common/observable.js"; import { Emitter } from "../../base/common/event.js"; import { Disposable } from "../../base/common/lifecycle.js"; import { CancellationError } from "../../base/common/errors.js"; @@ -11,14 +11,11 @@ export interface StructuralFileResult { diff?: StructuralDiff; error?: string; hidden?: string; - annotationError?: string; } export interface StructuralSessionChange { /** Files whose diff or fold state changed. */ readonly files: ReadonlySet; - /** Files with new summary labels or summary errors; their diff is unchanged. */ - readonly labels: ReadonlySet; readonly status: boolean; } @@ -27,11 +24,9 @@ export class StructuralDiffSession extends Disposable { private readonly changed = this._register(new Emitter()); readonly onDidChange = this.changed.event; private readonly pendingFiles = new Set(); - private readonly pendingLabels = new Set(); private pendingStatus = false; private readonly regions = new Map(); private readonly labels = new Map>(); - private readonly pendingLabelValues = new Map(); private readonly notification = this._register(new RunOnceScheduler(() => this.flushChanges(), 16)); private readonly abort = new AbortController(); private readonly results = new Map(); @@ -107,14 +102,10 @@ export class StructuralDiffSession extends Disposable { if (event.type !== "start" || event.version !== STRUCTURAL_WIRE_VERSION) throw new Error("Unsupported diffr stream protocol."); this.acceptManifest(event); } else if (event.type === "file") this.storeFileResult(event); - else if (event.type === "annotations") this.applyAnnotations(event); else if (event.type === "complete") this.finishLoading(event); else throw new Error(`Unexpected diffr event: ${event.type}`); if (event.type === "file") this.notify(structuralFilePath(event.file)); - else if (event.type === "annotations") { - this.pendingLabels.add(structuralFilePath(event.file)); - this.notify(); - } else this.notify(undefined, true); + else this.notify(undefined, true); } /** Stable presentation state, shared by all editors displaying this region. */ @@ -135,15 +126,11 @@ export class StructuralDiffSession extends Disposable { } private flushChanges(): void { - if (!this.pendingFiles.size && !this.pendingLabels.size && !this.pendingStatus) return; + if (!this.pendingFiles.size && !this.pendingStatus) return; const change: StructuralSessionChange = { - files: new Set(this.pendingFiles), labels: new Set(this.pendingLabels), status: this.pendingStatus, + files: new Set(this.pendingFiles), status: this.pendingStatus, }; - const labels = new Map(this.pendingLabelValues); - this.pendingFiles.clear(); this.pendingLabels.clear(); this.pendingLabelValues.clear(); this.pendingStatus = false; - transaction(tx => { - for (const [key, value] of labels) this.labels.get(key)?.set(value, tx); - }); + this.pendingFiles.clear(); this.pendingStatus = false; this.changed.fire(change); } @@ -160,33 +147,17 @@ export class StructuralDiffSession extends Disposable { if (diff.type === "text") { const seed = (region: StructuralRegion) => { this.regions.set(`${path}:${region.id}`, region); + this.labels.get(`${path}:${region.id}`)?.set(region.visibility?.label, undefined); const key = `${path}:${region.fold_state_id}`; if (!this.folds.has(key)) this.folds.set(key, region.visibility?.collapsed === true); if (region.kind === "fold") region.children.forEach(seed); }; - for (const side of [diff.lhs, diff.rhs]) side?.regions?.forEach(seed); - } - this.results.set(path, { - diff, - hidden: event.visibility?.collapsed ? event.visibility.label || "Hidden by default" : undefined - }); - } - - private applyAnnotations(event: Extract): void { - const path = structuralFilePath(event.file); - const result = this.results.get(path); - if (result?.diff?.type !== "text") throw new Error(`Annotations precede a text result: ${path}`); - // Validate the entire batch before applying it. Labels do not replace the - // source, correspondence, region identities, or the reader's fold choices. - for (const annotation of event.annotations) { - if (!this.regions.has(`${path}:${annotation.region_id}`)) throw new Error(`Unknown annotation region: ${annotation.region_id}`); - } - for (const annotation of event.annotations) { - const region = this.regions.get(`${path}:${annotation.region_id}`)!; - region.visibility = { ...region.visibility, label: annotation.label }; - this.pendingLabelValues.set(`${path}:${region.id}`, annotation.label); + for (const side of [diff.lhs, diff.rhs]) if (side) seed(side.root); } - result.annotationError = event.error?.message; + const visibility = diff.type === "text" + ? [diff.rhs, diff.lhs].find(side => side?.root.visibility?.collapsed)?.root.visibility + : undefined; + this.results.set(path, { diff, hidden: visibility ? visibility.label || "Hidden by default" : undefined }); } private finishLoading(event: Extract): void { @@ -203,8 +174,6 @@ export class StructuralDiffSession extends Disposable { this.labels.clear(); this.regions.clear(); this.pendingFiles.clear(); - this.pendingLabels.clear(); - this.pendingLabelValues.clear(); super.dispose(); } } diff --git a/apps/review-desktop/package.json b/apps/review-desktop/package.json index 0554b815c..37690dc7a 100644 --- a/apps/review-desktop/package.json +++ b/apps/review-desktop/package.json @@ -29,7 +29,7 @@ "@dev.fast/whiteboard": "workspace:*" }, "devDependencies": { - "@dev.fast/diffr": "0.1.11", + "@dev.fast/diffr": "catalog:", "@dev.fast/review-canvas": "workspace:*", "@types/node": "24.12.2", "jsdom": "29.1.1", diff --git a/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs b/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs index 7e5f60d92..a2cae9c75 100644 --- a/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs @@ -54,7 +54,7 @@ function testFolds(events) { return events .filter((event) => event.type === "file") - .flatMap((event) => walk(event.diff?.rhs?.regions)) + .flatMap((event) => walk(event.diff?.rhs ? [event.diff.rhs.root] : [])) .filter( (region) => region.tags?.includes("test-bodies:test") && @@ -117,8 +117,8 @@ export async function run(ctx) { await toggle.click(); await ctx.until( async () => - (await ctx.apiOk("/diffr-config")).values.plugins.bundled["test-bodies"] - .enabled === false, + (await ctx.apiOk("/diffr-config")).values.shape["test-bodies"].enabled === + false, "diffr persisted the setting", ); await settings diff --git a/apps/review-desktop/scripts/smoke-windows.mjs b/apps/review-desktop/scripts/smoke-windows.mjs index 986c0be1f..3d1d02572 100644 --- a/apps/review-desktop/scripts/smoke-windows.mjs +++ b/apps/review-desktop/scripts/smoke-windows.mjs @@ -257,7 +257,6 @@ export async function smokeWindows(app, evidence) { repository, "--format", "ndjson", - "--stream-annotations", base, head, "--", diff --git a/packages/review-protocol/package.json b/packages/review-protocol/package.json index bba0229c2..e80336a18 100644 --- a/packages/review-protocol/package.json +++ b/packages/review-protocol/package.json @@ -20,7 +20,7 @@ "typecheck": "pnpm -w exec tsc -p packages/review-protocol/tsconfig.json --noEmit" }, "dependencies": { - "@dev.fast/diffr": "0.1.11", + "@dev.fast/diffr": "catalog:", "@dev.fast/json": "workspace:*", "@dev.fast/trace-protocol": "workspace:*", "zod": "catalog:" diff --git a/packages/review-protocol/src/contracts.ts b/packages/review-protocol/src/contracts.ts index 67e486393..e169540df 100644 --- a/packages/review-protocol/src/contracts.ts +++ b/packages/review-protocol/src/contracts.ts @@ -487,6 +487,7 @@ export interface ReviewDiffrConfig { credentialSource: "config" | "environment" | "missing"; providers?: ReviewDiffrProvider[]; defaultPrompt?: string; + defaultHiddenTags?: string[]; changed?: boolean; error?: string; } @@ -522,6 +523,7 @@ const reviewDiffrConfigSchema = z.object({ ) .optional(), defaultPrompt: z.string().optional(), + defaultHiddenTags: z.array(z.string()).optional(), changed: z.boolean().optional(), error: z.string().optional(), }); diff --git a/packages/review-protocol/src/source-alignment.ts b/packages/review-protocol/src/source-alignment.ts index 572a81d3f..26f987fcb 100644 --- a/packages/review-protocol/src/source-alignment.ts +++ b/packages/review-protocol/src/source-alignment.ts @@ -14,9 +14,7 @@ function alignmentRegionLines(region: StructuralRegion) { }; } -function alignmentLeaves( - regions: readonly StructuralRegion[] | undefined, -): AlignmentLeaf[] { +function alignmentLeaves(root: StructuralRegion | undefined): AlignmentLeaf[] { const leaves: AlignmentLeaf[] = []; const walk = (region: StructuralRegion) => { @@ -24,7 +22,7 @@ function alignmentLeaves( else for (const child of region.children) walk(child); }; - for (const region of regions ?? []) walk(region); + if (root) walk(root); return leaves; } @@ -43,8 +41,8 @@ function monacoLineCount(source: StructuralSource | undefined): number { export function structuralRows( diff: Extract, ): [number | null, number | null][] { - const lhsLeaves = alignmentLeaves(diff.lhs?.regions); - const rhsLeaves = alignmentLeaves(diff.rhs?.regions); + const lhsLeaves = alignmentLeaves(diff.lhs?.root); + const rhsLeaves = alignmentLeaves(diff.rhs?.root); const leftCount = monacoLineCount(diff.lhs); const rightCount = monacoLineCount(diff.rhs); const rows: [number | null, number | null][] = []; diff --git a/packages/review/app/src/diffr-config-section.browser.test.tsx b/packages/review/app/src/diffr-config-section.browser.test.tsx index ed7820a79..905f94b22 100644 --- a/packages/review/app/src/diffr-config-section.browser.test.tsx +++ b/packages/review/app/src/diffr-config-section.browser.test.tsx @@ -44,22 +44,22 @@ function config(): ReviewDiffrConfig { keylessCustomEndpoint: id === "openai", })), defaultPrompt: "Default prompt.", + defaultHiddenTags: ["generated", "vendored", "test"], values: { - plugins: { - bundled: { - context: { enabled: true, lines: 3 }, - "test-bodies": { enabled: true }, - "deleted-bodies": { enabled: true }, - "removed-runs": { enabled: true }, - group: { enabled: true }, - "hide-files": { enabled: true, deleted: true, tags: ["test"] }, - summarize: { - enabled: false, - provider: "gemini", - model: "test-model", - tests: true, - system_prompt: "Default prompt.", - }, + version: 2, + classify: { classify: { hide: ["test"], hide_deleted: true } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + context: { enabled: true, lines: 3 }, + "test-bodies": { enabled: true }, + "deleted-bodies": { enabled: true }, + "removed-runs": { enabled: true }, + summarize: { + enabled: false, + provider: "gemini", + model: "test-model", + tests: true, + system_prompt: "Default prompt.", }, }, }, @@ -79,17 +79,19 @@ async function mount(overrides: Partial = {}) { async (input) => ({ ...current, changed: true, + defaultHiddenTags: ["generated", "vendored", "test"], values: { - plugins: { - bundled: { - summarize: { - enabled: input.enabled, - provider: input.provider, - model: input.model, - endpoint: input.endpoint, - system_prompt: input.systemPrompt, - tests: input.tests, - }, + version: 2, + classify: { classify: { hide: ["test"], hide_deleted: true } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + summarize: { + enabled: input.enabled, + provider: input.provider, + model: input.model, + endpoint: input.endpoint, + system_prompt: input.systemPrompt, + tests: input.tests, }, }, }, @@ -122,6 +124,112 @@ async function open() { .toBeVisible(); } +test("file-hiding controls use classify fields and retain custom tags across off and on", async () => { + const { actions, current } = await mount(); + current.values.classify = { + classify: { hide: ["custom"], hide_deleted: true }, + }; + vi.mocked(actions.set).mockImplementation(async (key, value) => { + const field = key.split(".").at(-1)!; + const classify = current.values.classify as JsonObject; + classify.classify = { + ...(classify.classify as JsonObject), + [field]: value, + }; + + return { ...current, changed: true }; + }); + await open(); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(current.values.classify).toMatchObject({ classify: { hide: [] } }); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(current.values.classify).toMatchObject({ + classify: { hide: ["custom"] }, + }); + await act(async () => page.getByLabelText("Hide deleted files").click()); + expect(current.values.classify).toMatchObject({ + classify: { hide_deleted: false }, + }); + expect(actions.set).toHaveBeenCalledWith("classify.classify.hide", []); + expect(actions.set).toHaveBeenCalledWith("classify.classify.hide", [ + "custom", + ]); + expect(actions.set).toHaveBeenCalledWith( + "classify.classify.hide_deleted", + false, + ); +}); + +test("enabling tag hiding from an empty list uses the schema defaults", async () => { + const { actions, current } = await mount(); + current.values.classify = { classify: { hide: [], hide_deleted: false } }; + await open(); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(actions.set).toHaveBeenCalledWith( + "classify.classify.hide", + current.defaultHiddenTags, + ); +}); + +test("custom classifier controls write its own entry and keep its tags", async () => { + const { actions, current } = await mount(); + current.values.classify = { + "team-classifier": { + path: "/plugins/team-classifier", + hide: ["team-generated"], + hide_deleted: true, + }, + }; + vi.mocked(actions.set).mockImplementation(async (key, value) => { + const [, name, field] = key.split("."); + const classifiers = current.values.classify as JsonObject; + const options = classifiers[name] as JsonObject; + + classifiers[name] = { ...options, [field]: value }; + + return { ...current, changed: true }; + }); + await open(); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(actions.set).toHaveBeenLastCalledWith( + "classify.team-classifier.hide", + [], + ); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(actions.set).toHaveBeenLastCalledWith( + "classify.team-classifier.hide", + ["team-generated"], + ); + await act(async () => page.getByLabelText("Hide deleted files").click()); + expect(actions.set).toHaveBeenLastCalledWith( + "classify.team-classifier.hide_deleted", + false, + ); + expect(current.values.classify).toEqual({ + "team-classifier": { + path: "/plugins/team-classifier", + hide: ["team-generated"], + hide_deleted: false, + }, + }); +}); + +test("custom classifiers only show the hide fields that they support", async () => { + const { actions, current } = await mount(); + current.values.classify = { + "team-classifier": { path: "/plugins/team-classifier", hide_deleted: true }, + }; + await open(); + expect( + document.querySelector('input[aria-label="Hide files by tag"]'), + ).toBeNull(); + await act(async () => page.getByLabelText("Hide deleted files").click()); + expect(actions.set).toHaveBeenCalledWith( + "classify.team-classifier.hide_deleted", + false, + ); +}); + test("starts collapsed and reads only on first expansion", async () => { const { actions } = await mount(); expect(actions.read).not.toHaveBeenCalled(); @@ -143,10 +251,7 @@ test("writes the selected key and keeps reload visible when collapsed", async () await act(async () => { await page.getByLabelText("Collapse test bodies").click(); }); - expect(actions.set).toHaveBeenCalledWith( - "plugins.bundled.test-bodies.enabled", - false, - ); + expect(actions.set).toHaveBeenCalledWith("shape.test-bodies.enabled", false); await expect .element(page.getByRole("button", { name: "Reload window", exact: true })) .toBeVisible(); @@ -178,8 +283,11 @@ test("rejects invalid context lines without writing", async () => { expect(actions.set).not.toHaveBeenCalled(); }); -test("tests draft settings without saving, then saves and clears the key", async () => { - const { actions } = await mount(); +test("tests draft settings without saving, then saves the custom prompt and clears the key", async () => { + const saved = config(); + const foldPlugins = saved.values.shape as JsonObject; + (foldPlugins.summarize as JsonObject).system_prompt = "Keep my wording."; + const { actions } = await mount(saved); await open(); expect( (document.querySelector("input[type=password]") as HTMLInputElement).value, @@ -203,13 +311,16 @@ test("tests draft settings without saving, then saves and clears the key", async provider: "gemini", model: "test-model", endpoint: "", - systemPrompt: "Default prompt.", + systemPrompt: "Keep my wording.", tests: true, apiKey: "test-secret", }); await expect .element(page.getByLabelText("API key", { exact: true })) .toHaveValue(""); + await expect + .element(page.getByLabelText("Prompt", { exact: true })) + .toHaveValue("Keep my wording."); }); test("confirms discarding unsaved summary edits before reloading", async () => { @@ -392,7 +503,8 @@ test("switching provider clears a custom endpoint, and a new endpoint warns abou system_prompt: "Default prompt.", }; - await mount({ values: { ...values, plugins: { bundled: { summarize } } } }); + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + await mount({ values: { ...values, shape: { summarize } } }); await open(); const endpoint = page.getByLabelText("Endpoint URL", { exact: true }); await expect.element(endpoint).toHaveValue("https://openrouter.ai/api/v1"); diff --git a/packages/review/app/src/diffr-config-section.tsx b/packages/review/app/src/diffr-config-section.tsx index 0d2931bdc..d93769dfd 100644 --- a/packages/review/app/src/diffr-config-section.tsx +++ b/packages/review/app/src/diffr-config-section.tsx @@ -1,5 +1,6 @@ import { Button } from "@canvas/ui/button"; import { TextField, fieldStyles } from "@canvas/ui/text-field"; +import { isBooleanValue, isStringValue } from "@dev.fast/json"; import { type JsonValue, type ReviewDiffrConfig, @@ -13,30 +14,53 @@ import { type ReactNode, useEffect, useRef, useState } from "react"; import { Choice } from "./settings-choice"; import { settingsStyles as styles } from "./settings-styles"; -const displaySettings = [ - ["context.enabled", "Collapse unchanged lines"], - ["test-bodies.enabled", "Collapse test bodies"], - ["deleted-bodies.enabled", "Collapse deleted function bodies"], - ["removed-runs.enabled", "Collapse long removed stretches"], - ["group.enabled", "Group adjacent folds"], - ["hide-files.enabled", "Hide files by tag"], - ["hide-files.deleted", "Hide deleted files"], +const foldSettings = [ + ["shape.context.enabled", "Collapse unchanged lines"], + ["shape.test-bodies.enabled", "Collapse test bodies"], + ["shape.deleted-bodies.enabled", "Collapse deleted function bodies"], + ["shape.removed-runs.enabled", "Collapse long removed stretches"], ] as const; +type DisplaySetting = readonly [key: string, label: string]; + +function classifierPrefix(config: ReviewDiffrConfig): string | undefined { + const entries = config.values.classify; + + if (!isJsonObject(entries)) return; + const names = Object.keys(entries); + + if (names.length === 1) return `classify.${names[0]}`; +} + +function displaySettings(config: ReviewDiffrConfig): DisplaySetting[] { + const settings: DisplaySetting[] = [...foldSettings]; + const prefix = classifierPrefix(config); + + if (!prefix) return settings; + + if (Array.isArray(setting(config, `${prefix}.hide`))) + settings.push([`${prefix}.hide`, "Hide files by tag"]); + + if (isBooleanValue(setting(config, `${prefix}.hide_deleted`))) + settings.push([`${prefix}.hide_deleted`, "Hide deleted files"]); + + return settings; +} + function setting( config: ReviewDiffrConfig, key: string, ): JsonValue | undefined { let value: JsonValue | undefined = config.values; - for (const part of `plugins.bundled.${key}`.split(".")) + for (const part of key.split(".")) value = isJsonObject(value) ? value[part] : undefined; return value; } function provider(config: ReviewDiffrConfig): string { - return String(setting(config, "summarize.provider") ?? ""); + return String(setting(config, "shape.summarize.provider") ?? ""); } /** The provider diffr describes as `id`, if any. */ @@ -50,14 +74,16 @@ function title(config: ReviewDiffrConfig, id: string): string { function summaryDraft(config: ReviewDiffrConfig): ReviewDiffrSummarizerInput { return { - enabled: setting(config, "summarize.enabled") === true, + enabled: setting(config, "shape.summarize.enabled") === true, provider: provider(config), - model: String(setting(config, "summarize.model") ?? ""), - endpoint: String(setting(config, "summarize.endpoint") ?? ""), + model: String(setting(config, "shape.summarize.model") ?? ""), + endpoint: String(setting(config, "shape.summarize.endpoint") ?? ""), systemPrompt: String( - setting(config, "summarize.system_prompt") ?? config.defaultPrompt ?? "", + setting(config, "shape.summarize.system_prompt") ?? + config.defaultPrompt ?? + "", ), - tests: setting(config, "summarize.tests") === true, + tests: setting(config, "shape.summarize.tests") === true, apiKey: "", }; } @@ -74,6 +100,7 @@ export function DiffrConfigSection({ const [draft, setDraft] = useState(); const [busy, setBusy] = useState(false); const pending = useRef(false); + const rememberedTags = useRef(undefined); const [error, setError] = useState(); const [summary, setSummary] = useState(); const [changed, setChanged] = useState(false); @@ -85,6 +112,7 @@ export function DiffrConfigSection({ actions.read().then( (result) => { if (cancelled) return; + rememberTags(result); setConfig(result); setDraft(summaryDraft(result)); }, @@ -121,7 +149,16 @@ export function DiffrConfigSection({ } } + function rememberTags(result: ReviewDiffrConfig) { + const prefix = classifierPrefix(result); + const tags = prefix ? setting(result, `${prefix}.hide`) : undefined; + + if (Array.isArray(tags) && tags.length) + rememberedTags.current = tags.filter(isStringValue); + } + function saved(result: ReviewDiffrConfig) { + rememberTags(result); setConfig(result); if (result.changed) setChanged(true); @@ -134,11 +171,14 @@ export function DiffrConfigSection({ JSON.stringify(draft) !== JSON.stringify(summaryDraft(config)); const unavailable = - !config || setting(config, "summarize.enabled") === undefined; + !config || setting(config, "shape.summarize.enabled") === undefined; const summaryValid = !!draft?.model.trim(); const savedDraft = config && summaryDraft(config); - const hiddenTags = config ? setting(config, "hide-files.tags") : undefined; + const classifier = config && classifierPrefix(config); + const hideKey = classifier ? `${classifier}.hide` : undefined; + + const hiddenTags = config && hideKey ? setting(config, hideKey) : undefined; return (
@@ -158,23 +198,30 @@ export function DiffrConfigSection({ {config && ( <>

Display

- {displaySettings.map(([key, label]) => ( + {displaySettings(config).map(([key, label]) => (
0 + : setting(config, key) === true + } disabled={busy || setting(config, key) === undefined} onChange={(event) => { - const value = event.target.checked; + const value = + key === hideKey + ? event.target.checked + ? (rememberedTags.current ?? + config.defaultHiddenTags ?? + []) + : [] + : event.target.checked; + void run(async () => - saved( - await actions.set( - `plugins.bundled.${key}`, - value, - ), - ), + saved(await actions.set(key, value)), ); }} /> @@ -184,30 +231,26 @@ export function DiffrConfigSection({ Not available in this configuration.

)} - {key === "context.enabled" && ( + {key === "shape.context.enabled" && ( void run(async () => saved( - await actions.set( - "plugins.bundled.context.lines", - value, - ), + await actions.set("shape.context.lines", value), ), ) } /> )} - {key === "hide-files.enabled" && - Array.isArray(hiddenTags) && ( -

- Tags: {hiddenTags.join(", ")} -

- )} + {key === hideKey && Array.isArray(hiddenTags) && ( +

+ Tags: {hiddenTags.join(", ")} +

+ )}
))}

AI summaries

diff --git a/packages/review/package.json b/packages/review/package.json index d844b03bf..8411ecae3 100644 --- a/packages/review/package.json +++ b/packages/review/package.json @@ -63,7 +63,7 @@ "@agentclientprotocol/claude-agent-acp": "0.84.0", "@agentclientprotocol/codex-acp": "2.0.0", "@agentclientprotocol/sdk": "1.5.1", - "@dev.fast/diffr": "0.1.11", + "@dev.fast/diffr": "catalog:", "@hono/node-server": "2.0.12", "@img/sharp-wasm32": "0.35.4", "@modelcontextprotocol/sdk": "1.30.0", @@ -79,6 +79,7 @@ "semver": "7.8.5", "sharp": "0.35.4", "smol-toml": "1.8.0", + "toml-eslint-parser": "catalog:", "zod": "catalog:" }, "devDependencies": { diff --git a/packages/review/scripts/bundled-diffr.integration.mjs b/packages/review/scripts/bundled-diffr.integration.mjs index 12042441f..2dc8e5b6a 100644 --- a/packages/review/scripts/bundled-diffr.integration.mjs +++ b/packages/review/scripts/bundled-diffr.integration.mjs @@ -1,7 +1,15 @@ import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; import { existsSync } from "node:fs"; -import { copyFile, cp, mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { + copyFile, + cp, + mkdir, + readFile, + readdir, + rm, + writeFile, +} from "node:fs/promises"; import { createServer } from "node:http"; import { createRequire } from "node:module"; import path from "node:path"; @@ -20,7 +28,9 @@ let structuralDiff, readDiffrConfig, setDiffrConfigValue, saveDiffrSummarizer, - testDiffrSummarizer; + testDiffrSummarizer, + ensureDiffrConfigMigrated, + diffrExecutable; const diffrPackage = path.dirname( createRequire(import.meta.url).resolve("@dev.fast/diffr/package.json"), @@ -32,7 +42,9 @@ const platformPackage = path.dirname( ), ); -const source = path.join(platformPackage, "diffr"); +// A local candidate lets this test run before its npm release is published. +const source = + process.env.REVIEW_DIFFR_TEST_BINARY || path.join(platformPackage, "diffr"); async function stageDiffr(runtime) { for (const directory of [diffrPackage, platformPackage]) @@ -41,6 +53,17 @@ async function stageDiffr(runtime) { path.join(runtime, "node_modules/@dev.fast", path.basename(directory)), { recursive: true, dereference: true }, ); + + if (process.env.REVIEW_DIFFR_TEST_BINARY) + await copyFile( + source, + path.join( + runtime, + "node_modules/@dev.fast", + path.basename(platformPackage), + "diffr", + ), + ); } async function collect(repositoryPath, base, head, paths, kind = "trees") { @@ -56,7 +79,7 @@ async function collect(repositoryPath, base, head, paths, kind = "trees") { function successfulFiles(events, count) { assert.equal(events[0].type, "start"); - assert.equal(events[0].version, 4); + assert.equal(events[0].version, 3); assert.deepEqual(events.at(-1), { type: "complete", succeeded: count, @@ -127,6 +150,10 @@ describe("Relocated runtime diffr integrates with Review streams and settings", import.meta.dirname, "../src/server/structural-diff.ts", ), + "diffr-config-migration": path.resolve( + import.meta.dirname, + "../src/server/diffr-config-migration.ts", + ), "diffr-config": path.resolve( import.meta.dirname, "../src/server/diffr-config.ts", @@ -146,7 +173,8 @@ describe("Relocated runtime diffr integrates with Review streams and settings", ).href ); - ({ structuralDiff } = await load("structural-diff")); + ({ structuralDiff, diffrExecutable } = await load("structural-diff")); + ({ ensureDiffrConfigMigrated } = await load("diffr-config-migration")); ({ readDiffrConfig, setDiffrConfigValue, @@ -206,25 +234,84 @@ describe("Relocated runtime diffr integrates with Review streams and settings", assert.deepEqual(events[0].files, []); }); + test("a saved UI config migrates through the staged binary with its overrides intact", async () => { + const previous = process.env.XDG_CONFIG_HOME; + process.env.XDG_CONFIG_HOME = path.join(root, "migration"); + const directory = path.join(process.env.XDG_CONFIG_HOME, "diffr"); + await mkdir(directory, { recursive: true }); + + const original = await readFile( + new URL( + "../src/server/test-fixtures/diffr-ui-overrides.toml", + import.meta.url, + ), + "utf8", + ); + + await writeFile(path.join(directory, "config.toml"), original, { + mode: 0o600, + }); + + try { + assert.equal(await ensureDiffrConfigMigrated(diffrExecutable()), true); + const config = await readDiffrConfig(repository); + assert.equal(config.values.version, 2); + assert.deepEqual(config.values.classify.classify.hide, [ + "test", + "generated", + ]); + assert.equal(config.values.classify.classify.hide_deleted, false); + assert.equal(config.values.shape.context.lines, 17); + assert.equal( + config.values.shape.summarize.system_prompt, + " Use my exact custom instructions.\nKeep # inside the prompt, and answer with JSON if I ask for it.\n", + ); + + const backup = (await readdir(directory)).find((name) => + name.includes("before-whiteboard-config-v2"), + ); + + assert.ok(backup); + assert.equal( + await readFile(path.join(directory, backup), "utf8"), + original, + ); + + const migrated = await readFile( + path.join(directory, "config.toml"), + "utf8", + ); + + assert.equal(await ensureDiffrConfigMigrated(diffrExecutable()), false); + await readDiffrConfig(repository); + assert.equal( + await readFile(path.join(directory, "config.toml"), "utf8"), + migrated, + ); + } finally { + process.env.XDG_CONFIG_HOME = previous; + } + }); + test("settings values and edits round-trip through the staged binary", async () => { const config = await readDiffrConfig(repository); - assert.ok(Number.isInteger(config.values.plugins.bundled.context.lines)); - assert.equal(config.values.plugins.bundled.summarize.api_key, undefined); + assert.ok(Number.isInteger(config.values.shape.context.lines)); + assert.equal(config.values.shape.summarize.api_key, undefined); assert.ok( ["config", "environment", "missing"].includes(config.credentialSource), ); const updated = await setDiffrConfigValue( - "plugins.bundled.context.lines", + "shape.context.lines", 7, repository, ); - assert.equal(updated.values.plugins.bundled.context.lines, 7); + assert.equal(updated.values.shape.context.lines, 7); assert.equal(updated.changed, true); assert.equal(updated.error, undefined); assert.equal( - (await readDiffrConfig(repository)).values.plugins.bundled.context.lines, + (await readDiffrConfig(repository)).values.shape.context.lines, 7, ); assert.equal(existsSync(sentinel), false); @@ -233,7 +320,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("a provider switch saves through the binary, clears the old key and keeps the file sparse", async () => { delete process.env.ANTHROPIC_API_KEY; await setDiffrConfigValue( - "plugins.bundled.summarize.api_key", + "shape.summarize.api_key", "old-secret", repository, ); @@ -254,7 +341,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", ); assert.equal(saved.error, undefined); - assert.equal(saved.values.plugins.bundled.summarize.provider, "anthropic"); + assert.equal(saved.values.shape.summarize.provider, "anthropic"); assert.equal(saved.credentialSource, "missing"); const file = await readFile( @@ -279,19 +366,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", body: JSON.parse(body), }; - const id = Number( - /fold (\d+):/.exec(received.body.messages[1].content)[1], - ); - - const content = JSON.stringify({ - summaries: [ - { - id, - summary: "", - pseudocode: "count and average positive values", - }, - ], - }); + const content = "count and average positive values"; response.setHeader("content-type", "application/json"); response.end(JSON.stringify({ choices: [{ message: { content } }] })); @@ -337,7 +412,10 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("an unbundled installation falls back to PATH", async () => { delete process.env.REVIEW_DIFFR_BINARY; - await rm(path.join(runtime, "node_modules"), { recursive: true, force: true }); + await rm(path.join(runtime, "node_modules"), { + recursive: true, + force: true, + }); await assert.rejects( collect(repository, base, head), /diffr exited with 97/, @@ -389,7 +467,10 @@ process.exit(result.status ?? 1); test("without a bundle the host binary runs real diffs and settings", async () => { delete process.env.REVIEW_DIFFR_BINARY; - await rm(path.join(runtime, "node_modules"), { recursive: true, force: true }); + await rm(path.join(runtime, "node_modules"), { + recursive: true, + force: true, + }); successfulFiles(await collect(repository, base, head), 3); assert.equal(existsSync(called), true); await rm(called); diff --git a/packages/review/src/review-api/comparison-coverage.ts b/packages/review/src/review-api/comparison-coverage.ts index 12595f89a..ca2ded9cc 100644 --- a/packages/review/src/review-api/comparison-coverage.ts +++ b/packages/review/src/review-api/comparison-coverage.ts @@ -4,7 +4,6 @@ import { type StructuralDiff, type StructuralRegion, type StructuralSource, - type StructuralVisibility, structuralRows, } from "@dev.fast/review-protocol"; import type { AlignmentRow } from "@review/lens-selection.js"; @@ -217,7 +216,7 @@ export async function comparisonCoverage( fingerprint, changed: diff.type === "text" ? diff.structural_changes : emptyCoverage(), - folded: foldedChanges(event.visibility, diff), + folded: foldedChanges(diff), viewed: emptyCoverage(), }); publish?.({ @@ -244,21 +243,17 @@ export async function comparisonCoverage( * the lines, not just the counts, are known. A paired leaf contributes its * changed spans' lines; an unpaired leaf, all of its lines. */ -export function foldedChanges( - visibility: StructuralVisibility | undefined, - diff: StructuralDiff, -): Coverage { +export function foldedChanges(diff: StructuralDiff): Coverage { if (diff.type !== "text") return emptyCoverage(); const all = diff.structural_changes; - if (visibility?.collapsed) - return { base: unionIntervals(all.base), head: unionIntervals(all.head) }; - const side = ( changed: readonly LineInterval[], source: StructuralSource | undefined, other: StructuralSource | undefined, ): LineInterval[] => { + if (source?.root.visibility?.collapsed) return unionIntervals(changed); + const paired = new Set(); const pair = (region: StructuralRegion) => { @@ -266,7 +261,7 @@ export function foldedChanges( else region.children.forEach(pair); }; - other?.regions?.forEach(pair); + if (other) pair(other.root); const hidden: LineInterval[] = [], visible: LineInterval[] = []; @@ -292,7 +287,7 @@ export function foldedChanges( ]); }; - for (const region of source?.regions ?? []) collect(region, false); + if (source) collect(source.root, false); return subtractIntervals(intersectIntervals(changed, hidden), visible); }; diff --git a/packages/review/src/review-api/folded-progress.test.ts b/packages/review/src/review-api/folded-progress.test.ts index ef56ad88f..45dc80455 100644 --- a/packages/review/src/review-api/folded-progress.test.ts +++ b/packages/review/src/review-api/folded-progress.test.ts @@ -2,10 +2,11 @@ import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; -import type { - StructuralDiff, - StructuralDiffEvent, - StructuralRegion, +import { + STRUCTURAL_DIFF_WIRE_VERSION, + type StructuralDiff, + type StructuralDiffEvent, + type StructuralRegion, } from "@dev.fast/review-protocol"; import { coverageProgress, coverageSources } from "@review/viewed-coverage.js"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; @@ -18,6 +19,8 @@ import { LocalReviewData } from "./local-data.js"; import type { ReviewProgress } from "./review-progress.js"; import { type ReviewProviders, ReviewStore } from "./store.js"; +let nextRoot = 100; + const pins = { repositoryId: "repo", base: "base-commit", head: "head-commit" }; const leaf = ( @@ -46,6 +49,7 @@ const fold = ( id, fold_state_id: id, kind: "fold", + indent: { line: from, column: 0 }, start: { line: from, column: 0 }, end: { line: to, column: 0 }, visibility: { collapsed }, @@ -57,15 +61,15 @@ const text = (lines: number) => /** One changed line in a function, and a test module diffr folds whose three * added lines pair with nothing. */ -const sourceDiff: StructuralDiff = { +const sourceDiff: Extract = { type: "text", - lhs: { text: text(2), regions: [leaf(1, 1, [0, 1], [0])] }, + lhs: { text: text(2), root: root([leaf(1, 1, [0, 1], [0])]) }, rhs: { text: text(6), - regions: [ + root: root([ leaf(2, 1, [0, 1], [0]), fold(3, [2, 5], true, [fold(4, [2, 5], false, [leaf(5, 5, [2, 5])])]), - ], + ]), }, structural_changes: { base: [[0, 1]], @@ -82,8 +86,8 @@ const sourceDiff: StructuralDiff = { const plainDiff = (lines: number): StructuralDiff => ({ type: "text", - lhs: { text: text(lines), regions: [leaf(1, 1, [0, lines], [0])] }, - rhs: { text: text(lines), regions: [leaf(2, 1, [0, lines], [0])] }, + lhs: { text: text(lines), root: root([leaf(1, 1, [0, lines], [0])]) }, + rhs: { text: text(lines), root: root([leaf(2, 1, [0, lines], [0])]) }, structural_changes: { base: [[0, 1]], head: [[0, 1]] }, stats: { textual: { added: 1, removed: 1 }, @@ -92,30 +96,30 @@ const plainDiff = (lines: number): StructuralDiff => ({ }); it("folds what diffr folds: hidden regions' changed lines, or a hidden file's", () => { - expect(foldedChanges(undefined, sourceDiff)).toEqual({ + expect(foldedChanges(sourceDiff)).toEqual({ base: [], head: [[2, 5]], }); // What stays unfolded is exactly what diffr counts in stats.visible. - expect( - foldedChanges({ collapsed: true, label: "Test file" }, sourceDiff), - ).toEqual(sourceDiff.structural_changes); - expect(foldedChanges(undefined, plainDiff(3))).toEqual({ + expect(foldedChanges(hideFile(sourceDiff))).toEqual( + sourceDiff.structural_changes, + ); + expect(foldedChanges(plainDiff(3))).toEqual({ base: [], head: [], }); }); it("a paired leaf folds only its changed lines, and a visible leaf wins a shared line", () => { - const diff: StructuralDiff = { + const diff: Extract = { type: "text", - lhs: { text: text(4), regions: [leaf(1, 7, [0, 4], [1, 3])] }, + lhs: { text: text(4), root: root([leaf(1, 7, [0, 4], [1, 3])]) }, rhs: { text: text(4), - regions: [ + root: root([ fold(2, [0, 3], true, [leaf(3, 7, [0, 3], [0, 2])]), leaf(4, 8, [2, 4]), - ], + ]), }, structural_changes: { base: [ @@ -133,7 +137,7 @@ it("a paired leaf folds only its changed lines, and a visible leaf wins a shared }, }; - expect(foldedChanges(undefined, diff)).toEqual({ + expect(foldedChanges(diff)).toEqual({ base: [], head: [[0, 1]], }); @@ -147,19 +151,19 @@ it("any region diffr collapses folds its changed lines, whatever its kind: a doc tags: ["summarize:docstring"], }; - const diff: StructuralDiff = { + const diff: Extract = { type: "text", lhs: { text: text(5), - regions: [ + root: root([ leaf(1, 1, [0, 1]), leaf(4, 3, [1, 4], [2]), leaf(5, 5, [4, 5], [4]), - ], + ]), }, rhs: { text: text(5), - regions: [leaf(6, 1, [0, 1]), docstring, leaf(7, 5, [4, 5], [4])], + root: root([leaf(6, 1, [0, 1]), docstring, leaf(7, 5, [4, 5], [4])]), }, structural_changes: { base: [ @@ -177,7 +181,7 @@ it("any region diffr collapses folds its changed lines, whatever its kind: a doc }, }; - const folded = foldedChanges(undefined, diff); + const folded = foldedChanges(diff); expect(folded).toEqual({ base: [], head: [[2, 3]] }); expect( coverageProgress([ @@ -233,17 +237,12 @@ const ref = (path: string) => ({ const records: { path: string; diff: StructuralDiff; - visibility?: { collapsed: boolean; label: string }; }[] = [ { path: "src/api.rs", diff: sourceDiff }, { path: "src/main.rs", diff: plainDiff(2) }, { path: "Cargo.lock", - diff: plainDiff(2), - visibility: { - collapsed: true, - label: "Generated file · hidden by default", - }, + diff: hideFile(plainDiff(2)), }, { path: "docs/readme.md", diff: plainDiff(2) }, ]; @@ -275,7 +274,7 @@ async function progressApi() { vi.spyOn(data, "structuralChanges").mockImplementation(async function* () { yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: records.map(({ path }) => ({ @@ -284,8 +283,8 @@ async function progressApi() { })), } satisfies StructuralDiffEvent; - for (const { path, diff, visibility } of records) - yield { type: "file", file: ref(path), diff, visibility }; + for (const { path, diff } of records) + yield { type: "file", file: ref(path), diff }; yield { type: "complete", succeeded: records.length, failed: 0 }; }); @@ -354,3 +353,28 @@ it("progress counts folded changes as done, overall, per lens and uncategorized" ).state, ).toBe("viewed"); }); + +function root(children: StructuralRegion[]): StructuralRegion { + const id = nextRoot++; + + return { + kind: "fold", + id, + fold_state_id: id, + start: children[0].start, + end: children.at(-1)!.end, + indent: children[0].start, + children, + }; +} + +function hideFile(diff: StructuralDiff): StructuralDiff { + const hidden = structuredClone(diff); + + if (hidden.type === "text") + for (const source of [hidden.lhs, hidden.rhs]) + if (source) + source.root.visibility = { collapsed: true, label: "Test file" }; + + return hidden; +} diff --git a/packages/review/src/review-api/lenses.test.ts b/packages/review/src/review-api/lenses.test.ts index 3ab82a4fe..3ce6e661d 100644 --- a/packages/review/src/review-api/lenses.test.ts +++ b/packages/review/src/review-api/lenses.test.ts @@ -3,6 +3,10 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { DatabaseSync } from "node:sqlite"; +import { + STRUCTURAL_DIFF_WIRE_VERSION, + type StructuralSource, +} from "@dev.fast/review-protocol"; import { rangeAnchor } from "@review/lens-selection.js"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; @@ -302,7 +306,7 @@ it("reports the changed lines no lens selects after each lens write", async () = vi.spyOn(data, "structuralChanges").mockImplementation(async function* () { yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: [api, readme].map(({ file }) => ({ file, status: "modified" })), @@ -314,8 +318,8 @@ it("reports the changed lines no lens selects after each lens write", async () = file, diff: { type: "text", - lhs: { text }, - rhs: { text }, + lhs: structuralSource(text), + rhs: structuralSource(text), structural_changes: { base: [[0, text.split("\n").length]], head: [[0, text.split("\n").length]], @@ -391,3 +395,19 @@ it("reports the changed lines no lens selects after each lens write", async () = await data.close(); } }); + +function structuralSource(text: string): StructuralSource { + const lines = text.split("\n"); + + return { + text, + root: { + kind: "leaf", + id: 1, + fold_state_id: 1, + alignment_id: 1, + start: { line: 0, column: 0 }, + end: { line: lines.length - 1, column: lines.at(-1)!.length }, + }, + }; +} diff --git a/packages/review/src/review-api/review-api.test.ts b/packages/review/src/review-api/review-api.test.ts index 71b662765..64eb82a94 100644 --- a/packages/review/src/review-api/review-api.test.ts +++ b/packages/review/src/review-api/review-api.test.ts @@ -4,6 +4,10 @@ import path from "node:path"; import { DatabaseSync } from "node:sqlite"; import { fileURLToPath } from "node:url"; +import { + STRUCTURAL_DIFF_WIRE_VERSION, + type StructuralSource, +} from "@dev.fast/review-protocol"; import { anchorSelection, rangeAnchor } from "@review/lens-selection.js"; import { createGlobalReviewServer } from "@review/server/desktop-server.js"; import { GlobalReviewDesktopVerbRelay } from "@review/server/global-verb-relay.js"; @@ -2088,7 +2092,7 @@ it("keeps reference coverage apart by pins: one path, changed under one comparis yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: at.base }, rhs: { type: "revision", rev: at.head }, files, @@ -2100,8 +2104,8 @@ it("keeps reference coverage apart by pins: one path, changed under one comparis file: file.file, diff: { type: "text", - lhs: { text: text }, - rhs: { text: text }, + lhs: structuralSource(text), + rhs: structuralSource(text), structural_changes: { base: [], head: [[0, 3]] }, stats: { textual: { added: 3, removed: 0 }, @@ -2176,7 +2180,7 @@ it("preserves unchanged partial file coverage across pins and rejects stale writ vi.spyOn(data, "structuralChanges").mockImplementation(async function* () { yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: [ @@ -2197,8 +2201,8 @@ it("preserves unchanged partial file coverage across pins and rejects stale writ }, diff: { type: "text", - lhs: { text: base }, - rhs: { text: head }, + lhs: structuralSource(base), + rhs: structuralSource(head), structural_changes: { base: [[1, 2]], head: [[1, 2]] }, stats: { textual: { added: 99, removed: 99 }, @@ -2307,7 +2311,7 @@ it("still reports structural read failures", async () => { yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: pins.base }, rhs: { type: "revision", rev: pins.head }, files: [{ file, status: "added" }], @@ -2377,7 +2381,7 @@ it("textual coverage uses Git ranges without launching diffr", async () => { yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: [{ file, status: "modified" }], @@ -2387,8 +2391,8 @@ it("textual coverage uses Git ranges without launching diffr", async () => { file, diff: { type: "text", - lhs: { text: "const x=1;" }, - rhs: { text: "const x = 1;" }, + lhs: structuralSource("const x=1;"), + rhs: structuralSource("const x = 1;"), structural_changes: { base: [], head: [] }, stats: { textual: { added: 1, removed: 1 }, @@ -2447,7 +2451,7 @@ it("resolves file lenses to whole changed files, preserves empty groups, and sha vi.spyOn(data, "structuralChanges").mockImplementation(async function* () { yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: [ @@ -2468,8 +2472,8 @@ it("resolves file lenses to whole changed files, preserves empty groups, and sha }, diff: { type: "text", - lhs: { text: "base\ncontext\nmore context" }, - rhs: { text: "head\ncontext\nmore context" }, + lhs: structuralSource("base\ncontext\nmore context"), + rhs: structuralSource("head\ncontext\nmore context"), structural_changes: { base: [[0, 1]], head: [[0, 1]] }, stats: { textual: { added: 99, removed: 99 }, @@ -2567,7 +2571,7 @@ it("validates range lens evidence and scopes progress and Uncategorized to disti vi.spyOn(data, "structuralChanges").mockImplementation(async function* () { yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: [ @@ -2588,8 +2592,8 @@ it("validates range lens evidence and scopes progress and Uncategorized to disti }, diff: { type: "text", - lhs: { text: "base1\nbase2\nbase3" }, - rhs: { text: "head1\nhead2\nhead3" }, + lhs: structuralSource("base1\nbase2\nbase3"), + rhs: structuralSource("head1\nhead2\nhead3"), structural_changes: { base: [[0, 3]], head: [[0, 3]] }, stats: { textual: { added: 99, removed: 99 }, @@ -2693,7 +2697,7 @@ it("returns coverage and lenses after initial files without requesting summary e vi.spyOn(data, "structuralChanges").mockImplementation(async function* () { yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "empty_tree" }, rhs: { type: "revision", rev: "head" }, files: [{ file, status: "added" }], @@ -2703,7 +2707,7 @@ it("returns coverage and lenses after initial files without requesting summary e file, diff: { type: "text", - rhs: { text: "added" }, + rhs: structuralSource("added"), structural_changes: { base: [], head: [[0, 1]] }, stats: { textual: { added: 1, removed: 0 }, @@ -2955,7 +2959,7 @@ it("shares pending comparison work even when more than 32 reviews are opened", a await gate; yield { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: [], @@ -3029,3 +3033,19 @@ it("reports a created review with the origin its headers claim", async () => { { reviewId: expect.any(String), kind: "review", blocks: 0, via: "other" }, ]); }); + +function structuralSource(text: string): StructuralSource { + const lines = text.split("\n"); + + return { + text, + root: { + kind: "leaf", + id: 1, + fold_state_id: 1, + alignment_id: 1, + start: { line: 0, column: 0 }, + end: { line: lines.length - 1, column: lines.at(-1)!.length }, + }, + }; +} diff --git a/packages/review/src/server/desktop-server.ts b/packages/review/src/server/desktop-server.ts index 7c8446ba9..907e133ca 100644 --- a/packages/review/src/server/desktop-server.ts +++ b/packages/review/src/server/desktop-server.ts @@ -62,6 +62,7 @@ import { setDiffrConfigValue, testDiffrSummarizer, } from "./diffr-config"; +import { ensureDiffrConfigMigrated } from "./diffr-config-migration.js"; import { GlobalReviewDesktopVerbRelay, type ReviewDesktopVerbRelay, @@ -73,6 +74,7 @@ import { reviewLifecycleTelemetry } from "./review-lifecycle-telemetry"; import { ReviewOpenWatchdog } from "./review-open-watchdog"; import { createWhiteboardCore, serverJson } from "./review-server-core"; import { invalidateStructuralComparisons } from "./structural-comparisons.js"; +import { diffrExecutable } from "./structural-diff.js"; import { createTutorialService } from "./tutorial-service"; import { captureSanitizedUiTelemetry } from "./ui-telemetry"; @@ -567,6 +569,7 @@ export function createGlobalReviewServer( return urlForBoundPort(); }, listen: async () => { + await ensureDiffrConfigMigrated(diffrExecutable()); scratchpadEnabled = await readScratchpadEnabled(); boundPort = await listen(httpServer, input.port); discovery.url = urlForBoundPort(); diff --git a/packages/review/src/server/diffr-config-migration.test.ts b/packages/review/src/server/diffr-config-migration.test.ts new file mode 100644 index 000000000..a9c228410 --- /dev/null +++ b/packages/review/src/server/diffr-config-migration.test.ts @@ -0,0 +1,533 @@ +import { + mkdir, + mkdtemp, + readFile, + readdir, + rm, + stat, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; + +import { parse } from "smol-toml"; +import { afterEach, expect, test } from "vitest"; +import { z } from "zod"; + +import { + ensureDiffrConfigMigrated, + migrateDiffrConfig, +} from "./diffr-config-migration"; +import { TomlEdits } from "./toml-edits"; + +const roots: string[] = []; + +test("rejects a change to a retained credential without exposing its value", () => { + const original = 'plugins.bundled.summarize.api_key = "saved-secret"\n'; + const edits = new TomlEdits(original); + edits.set("plugins.bundled.summarize.api_key", "different-secret"); + expect(() => + edits.assertUnchanged(original, [ + [ + "plugins.bundled.summarize.api_key", + "plugins.bundled.summarize.api_key", + ], + ]), + ).toThrow( + "diffr migration changed a retained setting: plugins.bundled.summarize.api_key", + ); +}); + +afterEach(async () => { + await Promise.all( + roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), + ); +}); + +test("converts a saved UI config without changing supported overrides or their comments", async () => { + const source = await readFile( + new URL("./test-fixtures/diffr-ui-overrides.toml", import.meta.url), + "utf8", + ); + + const migrated = migrateDiffrConfig(source); + const original = parse(source); + const expected = structuredClone(original); + + const plugins = z + .object({ + bundled: z.record( + z.string(), + z.record( + z.string(), + z.union([z.string(), z.boolean(), z.number(), z.array(z.string())]), + ), + ), + }) + .parse(expected.plugins); + + delete plugins.bundled.group; + delete plugins.bundled["hide-files"]; + delete plugins.bundled.summarize.max_concurrency; + delete expected.plugins; + expected.version = 2; + expected.shape = plugins.bundled; + expected.classify = { + classify: { hide: ["test", "generated"], hide_deleted: false }, + }; + expect(parse(migrated)).toEqual(expected); + + // Source spans for credentials, prompt, context and fold choices are not serialized. + for (const line of source + .split("\n") + .filter((line) => + /# (Collapse|Context|Enable|Provider|API key|Model|Endpoint|Include|Unrelated)/.test( + line, + ), + )) + expect(migrated).toContain(line); + expect(migrated).toContain( + "system_prompt = ''' Use my exact custom instructions.\nKeep # inside the prompt, and answer with JSON if I ask for it.\n''' # Prompt", + ); + expect(migrated).toContain("# Group adjacent folds; retired"); + expect(migrateDiffrConfig(migrated)).toBe(migrated); +}); + +test.each([ + [true, true, ["test", "generated"], true], + [true, false, ["test", "generated"], false], + [false, true, [], false], + [false, false, [], false], +])( + "preserves hiding for enabled=%s deleted=%s", + (enabled, deleted, hide, hideDeleted) => { + const migrated = migrateDiffrConfig( + `[plugins.bundled.hide-files]\nenabled = ${enabled}\ndeleted = ${deleted}\ntags = ["test", "generated"]\n`, + ); + + expect(parse(migrated).classify).toEqual({ + classify: { + hide, + hide_deleted: hideDeleted, + }, + }); + }, +); + +test.each([true, false])( + "removes retired group=%s without changing context", + (enabled) => { + const migrated = migrateDiffrConfig( + `[plugins.bundled.context]\nenabled = false\nlines = 9\n[plugins.bundled.group]\nenabled = ${enabled}\n`, + ); + + expect(parse(migrated)).toEqual({ + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { context: { enabled: false, lines: 9 } }, + }); + }, +); + +test("handles quoted keys, inline tables and dotted keys without touching a custom prompt", () => { + const source = `version = 1\nplugins = { bundled = { group = { enabled = false }, "hide-files" = { enabled = false }, summarize = { system_prompt = "Keep me", max_concurrency = 2 } }, order = ["bundled.group", "bundled.hide-files", "bundled.summarize"] }\n`; + expect(parse(migrateDiffrConfig(source))).toEqual({ + version: 2, + classify: { classify: { hide: [], hide_deleted: false } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + summarize: { system_prompt: "Keep me" }, + order: ["summarize"], + }, + }); + const dotted = `plugins.bundled.group.enabled = true\nplugins.bundled."hide-files".deleted = false\nclassifier = { hide = ["vendored"] }\n`; + expect(parse(migrateDiffrConfig(dotted)).classify).toEqual({ + classify: { + hide: ["vendored"], + hide_deleted: false, + }, + }); +}); + +test("retains explicit modern classifier values in a mixed config", () => { + const source = `[classifier]\nhide = ["custom"]\nhide_deleted = true\n[plugins.bundled.hide-files]\nenabled = false\n`; + expect(parse(migrateDiffrConfig(source))).toEqual({ + version: 2, + classify: { classify: { hide: ["custom"], hide_deleted: true } }, + }); +}); + +test("removes retired names from a custom order without sorting surviving plugins", () => { + const source = `[plugins]\norder = ["bundled.context", "bundled.group", "external.custom", "bundled.hide-files", "bundled.summarize"]\n`; + expect(parse(migrateDiffrConfig(source))).toEqual({ + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + order: ["context", "custom", "summarize"], + }, + }); +}); + +test("retains comments inside a rewritten plugin order", () => { + const source = `[plugins]\norder = [\n"bundled.context", # keep context first\n"bundled.group", # retired group\n"bundled.summarize",\n]\n`; + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { order: ["context", "summarize"] }, + }); + expect(migrated.match(/# keep context first/g)).toHaveLength(1); + expect(migrated.match(/# retired group/g)).toHaveLength(1); +}); + +async function fixture() { + const root = await mkdtemp( + path.join(tmpdir(), "whiteboard-config-migration-"), + ); + + roots.push(root); + const file = path.join(root, "diffr", "config.toml"); + await mkdir(path.dirname(file)); + const executable = path.join(root, "diffr-cli"); + await writeFile( + executable, + `#!${process.execPath}\nconst fs = require('node:fs');\nconst path = require('node:path');\nif (process.argv[3] !== 'show') process.exit(3);\n{\n const candidate = fs.readFileSync(path.join(process.env.XDG_CONFIG_HOME,'diffr','config.toml'),'utf8');\n if (process.env.CAPTURE_CANDIDATE) fs.writeFileSync(process.env.CAPTURE_CANDIDATE, candidate);\n if (process.env.REJECT_CANDIDATE) { console.error('synthetic secret'); process.exit(2); }\n if (process.env.CONCURRENT_EDIT) fs.writeFileSync(${JSON.stringify(file)}, 'version = 1 # concurrent edit\\n');\n if (candidate.includes('plugins.bundled.group')) process.exit(2);\n console.log('{}');\n}\n`, + { mode: 0o755 }, + ); + + return { + root, + file, + executable, + env: { ...process.env, XDG_CONFIG_HOME: root }, + }; +} + +test("validates before writing, backs up privately and is a no-op on a second run", async () => { + const { root, file, executable, env } = await fixture(); + const source = `[plugins.bundled.group]\nenabled = false # retired\n`; + await writeFile(file, source); + expect(await ensureDiffrConfigMigrated(executable, env)).toBe(true); + const migrated = await readFile(file, "utf8"); + expect(parse(migrated)).toEqual({ version: 2 }); + + const backup = (await readdir(path.dirname(file))).find((name) => + name.includes(".before-whiteboard-config-v2."), + )!; + + expect(await readFile(path.join(path.dirname(file), backup), "utf8")).toBe( + source, + ); + + expect(await ensureDiffrConfigMigrated(executable, env)).toBe(false); + expect(await readFile(file, "utf8")).toBe(migrated); + expect( + (await readdir(path.join(root, "diffr"))).some((name) => + name.startsWith(".whiteboard-config-"), + ), + ).toBe(false); +}); + +test.skipIf(process.platform === "win32")( + "keeps backup credentials private", + async () => { + const { file, executable, env } = await fixture(); + await writeFile(file, `[plugins.bundled.group]\nenabled = false\n`); + await ensureDiffrConfigMigrated(executable, env); + + const backup = (await readdir(path.dirname(file))).find((name) => + name.includes(".before-whiteboard-config-v2."), + )!; + + expect( + (await stat(path.join(path.dirname(file), backup))).mode & 0o777, + ).toBe(0o600); + }, +); + +test("does not run the binary or change a version 2 file", async () => { + const { file, env } = await fixture(); + const source = "version = 2 # already migrated\n[shape.context]\nlines = 8\n"; + await writeFile(file, source); + expect(await ensureDiffrConfigMigrated("/no/binary/needed", env)).toBe(false); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); + +test("keeps an unknown config version unchanged", async () => { + const { file, env } = await fixture(); + const source = "version = 3\n"; + await writeFile(file, source); + await expect( + ensureDiffrConfigMigrated("/no/binary/needed", env), + ).rejects.toThrow("Unsupported diffr config version: 3"); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); + +test("keeps the original and hides provider output when validation fails", async () => { + const { file, executable, env } = await fixture(); + const source = `[plugins.bundled.group]\nenabled = false\n`; + await writeFile(file, source); + await expect( + ensureDiffrConfigMigrated(executable, { ...env, REJECT_CANDIDATE: "1" }), + ).rejects.toThrow("The original file was kept"); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); + +test("does not replace an external edit made during validation", async () => { + const { file, executable, env } = await fixture(); + await writeFile(file, `[plugins.bundled.group]\nenabled = false\n`); + await expect( + ensureDiffrConfigMigrated(executable, { ...env, CONCURRENT_EDIT: "1" }), + ).rejects.toThrow("changed during migration"); + expect(await readFile(file, "utf8")).toBe("version = 1 # concurrent edit\n"); +}); + +test("uses the caller directory and preserves relative paths in the saved file", async () => { + const { root, file, env } = await fixture(); + const source = `[theme]\npath = "themes/custom.toml"\n[plugins.bundled.group]\nenabled = true\n`; + await writeFile(file, source); + const captured = path.join(root, "candidate.toml"); + expect( + await ensureDiffrConfigMigrated( + "./diffr-cli", + { + ...env, + XDG_CONFIG_HOME: ".", + CAPTURE_CANDIDATE: captured, + }, + undefined, + root, + ), + ).toBe(true); + expect(parse(await readFile(captured, "utf8")).theme).toEqual({ + path: path.join(path.dirname(file), "themes/custom.toml"), + }); + expect(parse(await readFile(file, "utf8")).theme).toEqual({ + path: "themes/custom.toml", + }); +}); + +test("migrates a version-only config and leaves v2 text unchanged", () => { + expect(parse(migrateDiffrConfig("version = 1 # format\n"))).toEqual({ + version: 2, + }); + const source = "version = 2 # format\n"; + expect(migrateDiffrConfig(source)).toBe(source); + expect(() => migrateDiffrConfig("version = 7\n")).toThrow( + "Unsupported diffr config version: 7", + ); +}); + +test("moves plugin options and external paths without rewriting value bytes", () => { + const source = `version = 1 +[plugins.bundled.context] +lines = 0x10 # keep spelling +extra = { option = 'custom', nested = [1, 2] } +[plugins.external.custom] +path = 'plugins/custom' # keep relative +threshold = 1_000 +[classifier] +hide = ['custom'] # keep tags +[diff] +byte_limit = 1_000_000 +`; + + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + context: { lines: 16, extra: { option: "custom", nested: [1, 2] } }, + custom: { path: "plugins/custom", threshold: 1000 }, + }, + classify: { classify: { hide: ["custom"] } }, + diff: { byte_limit: 1000000 }, + }); + + for (const line of source + .split("\n") + .filter((line) => line.includes(" = ") && !line.startsWith("version"))) + expect(migrated).toContain(line); +}); + +test("refuses ambiguous bundled and external plugins without dropping overrides", () => { + const source = `[plugins.bundled.context] +enabled = false +lines = 9 +[plugins.external.context] +path = "custom/context" +`; + + expect(() => migrateDiffrConfig(source)).toThrow( + "both bundled and external entries for context", + ); +}); + +test("moves a custom classifier under its manifest name and validates its original path", async () => { + const { root, file, executable, env } = await fixture(); + const directory = path.join(root, "diffr", "custom-classifier"); + await mkdir(directory); + await writeFile( + path.join(directory, "plugin.toml"), + 'name = "team.classifier"\n', + ); + + const source = + '[classifier]\npath = "custom-classifier" # local plugin\nhide = ["team"]\n'; + + await writeFile(file, source); + const captured = path.join(root, "candidate.toml"); + await ensureDiffrConfigMigrated(executable, { + ...env, + CAPTURE_CANDIDATE: captured, + }); + expect(parse(await readFile(file, "utf8"))).toEqual({ + version: 2, + classify: { + "team.classifier": { path: "custom-classifier", hide: ["team"] }, + }, + }); + expect(parse(await readFile(captured, "utf8"))).toEqual({ + version: 2, + classify: { "team.classifier": { path: directory, hide: ["team"] } }, + }); +}); + +test("keeps a custom classifier config when its manifest cannot be read", async () => { + const { file, executable, env } = await fixture(); + const source = '[classifier]\npath = "missing-plugin"\n'; + await writeFile(file, source); + await expect(ensureDiffrConfigMigrated(executable, env)).rejects.toThrow( + "could not read the custom classifier manifest", + ); + expect(await readFile(file, "utf8")).toBe(source); +}); + +test("redacts saved credentials from invalid TOML and schema errors", async () => { + const { file, executable, env } = await fixture(); + + for (const source of [ + 'plugins.bundled.summarize.api_key = "saved-secret" unexpected\n', + 'plugins.bundled.summarize = "saved-secret"\n', + ]) { + await writeFile(file, source); + const result = ensureDiffrConfigMigrated(executable, env); + await expect(result).rejects.toThrow("could not migrate the saved config"); + await expect(result).rejects.not.toThrow("saved-secret"); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); + } +}); + +test("serializes concurrent startup migrations and writes one backup", async () => { + const { file, executable, env } = await fixture(); + const source = "version = 1\n[plugins.bundled.context]\nlines = 11\n"; + await writeFile(file, source); + + const results = await Promise.all([ + ensureDiffrConfigMigrated(executable, env), + ensureDiffrConfigMigrated(executable, env), + ]); + + expect(results.sort()).toEqual([false, true]); + expect(parse(await readFile(file, "utf8"))).toEqual({ + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { context: { lines: 11 } }, + }); + + const backups = (await readdir(path.dirname(file))).filter((name) => + name.includes("before-whiteboard-config-v2"), + ); + + expect(backups).toHaveLength(1); + expect( + await readFile(path.join(path.dirname(file), backups[0]), "utf8"), + ).toBe(source); +}); + +test("keeps ambiguous plugin entries unchanged on disk", async () => { + const { file, executable, env } = await fixture(); + + const source = + '[plugins.bundled.context]\nenabled = false\n[plugins.external.context]\npath = "custom"\n'; + + await writeFile(file, source); + await expect(ensureDiffrConfigMigrated(executable, env)).rejects.toThrow( + "both bundled and external entries for context", + ); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); + +test.each([ + '[plugins.external."team.custom"]\npath = "plugins/custom"\n"option.name" = 0x10\n', + 'plugins.external."team.custom".path = "plugins/custom"\nplugins.external."team.custom"."option.name" = 0x10\n', + 'plugins = { external = { "team.custom" = { path = "plugins/custom", "option.name" = 0x10 } } }\n', +])("retains literal dots in external plugin and option names", (source) => { + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { "team.custom": { path: "plugins/custom", "option.name": 16 } }, + }); + expect(migrated).toContain("0x10"); +}); + +test("refuses a plugin named order because the v2 key is reserved", () => { + expect(() => + migrateDiffrConfig('[plugins.external.order]\npath = "plugins/custom"\n'), + ).toThrow("plugin named order"); +}); + +// Historical literals copied from diffr defaults and config/prune.rs. +test.each([ + 'For each listed fold, rewrite that function body as short python-flavored pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON array of {"id", "summary", "pseudocode"} objects, one per fold.', + 'For each listed fold, rewrite that function body as short pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON array of {"id", "summary", "pseudocode"} objects, one per fold.', + 'For each listed fold, rewrite that function body as short pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON object whose "summaries" array holds one {"id", "summary", "pseudocode"} object per fold.', +])("resets historical stock prompt %# but retains edits", (stockPrompt) => { + const source = `[plugins.bundled.summarize]\nsystem_prompt = ${JSON.stringify(stockPrompt)} # old default\n`; + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { summarize: {} }, + }); + expect(migrated).toContain("# old default"); + + for (const custom of [ + " " + stockPrompt, + stockPrompt + " Keep my team naming.", + ]) { + const literal = JSON.stringify(custom); + + const migratedCustom = migrateDiffrConfig( + `[plugins.bundled.summarize]\nsystem_prompt = ${literal} # custom prompt\n`, + ); + + expect( + new TomlEdits(migratedCustom).value("shape.summarize.system_prompt"), + ).toBe(custom); + expect(migratedCustom).toContain( + `system_prompt = ${literal} # custom prompt`, + ); + } +}); + +test("moves the old default context-first order to the v2 context-last default", () => { + const source = + '[plugins]\norder = ["bundled.context", "bundled.hide-files", "bundled.deleted-bodies", "bundled.summarize", "bundled.test-bodies", "bundled.removed-runs", "bundled.group"]\n'; + + expect( + new TomlEdits(migrateDiffrConfig(source)).value("shape.order"), + ).toEqual([ + "deleted-bodies", + "summarize", + "test-bodies", + "removed-runs", + "context", + ]); +}); diff --git a/packages/review/src/server/diffr-config-migration.ts b/packages/review/src/server/diffr-config-migration.ts new file mode 100644 index 000000000..37322f566 --- /dev/null +++ b/packages/review/src/server/diffr-config-migration.ts @@ -0,0 +1,382 @@ +import { execFile } from "node:child_process"; +import { createHash } from "node:crypto"; +import { + mkdir, + mkdtemp, + readFile, + realpath, + rm, + writeFile, +} from "node:fs/promises"; +import { homedir } from "node:os"; +import path from "node:path"; +import { promisify } from "node:util"; + +import { withFileLock, writeFileAtomicAsync } from "@dev.fast/trace-core"; +import { parse } from "smol-toml"; +import { z } from "zod"; + +import { TomlEdits, tomlKey, tomlKeyParts } from "./toml-edits.js"; + +const execute = promisify(execFile); + +class ConfigMigrationError extends Error {} + +const oldOrder = [ + "bundled.context", + "bundled.hide-files", + "bundled.deleted-bodies", + "bundled.summarize", + "bundled.test-bodies", + "bundled.removed-runs", + "bundled.group", +]; + +const newOrder = [ + "deleted-bodies", + "summarize", + "test-bodies", + "removed-runs", + "context", +]; + +// Exact defaults written by older diffr releases; edited prompts stay intact. +const stockPrompts = new Set([ + 'For each listed fold, rewrite that function body as short python-flavored pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON array of {"id", "summary", "pseudocode"} objects, one per fold.', + 'For each listed fold, rewrite that function body as short pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON array of {"id", "summary", "pseudocode"} objects, one per fold.', + 'For each listed fold, rewrite that function body as short pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON object whose "summaries" array holds one {"id", "summary", "pseudocode"} object per fold.', +]); + +const configVersionSchema = z.object({ version: z.number().int().optional() }); + +const pluginEntrySchema = z.record(z.string(), z.unknown()); + +const legacyConfigSchema = z.object({ + plugins: z + .object({ + order: z.array(z.string()).optional(), + bundled: z.record(z.string(), pluginEntrySchema).optional(), + external: z.record(z.string(), pluginEntrySchema).optional(), + }) + .optional(), + classifier: pluginEntrySchema.optional(), +}); + +const legacyHideSchema = z.object({ + enabled: z.boolean().default(true), + tags: z.array(z.string()).default(["generated", "vendored", "test"]), + deleted: z.boolean().default(true), +}); + +const legacySummarySchema = z.object({ system_prompt: z.string().optional() }); + +/** Convert v1 paths to v2 without serializing saved plugin option values. */ +export function migrateDiffrConfig( + source: string, + classifierName = "classify", +): string { + const parsed = parse(source); + const version = configVersionSchema.parse(parsed).version ?? 1; + + if (version === 2) return source; + + if (version !== 1) + throw new ConfigMigrationError( + `Unsupported diffr config version: ${version}`, + ); + + const config = legacyConfigSchema.parse(parsed); + const bundled = config.plugins?.bundled; + const external = config.plugins?.external; + const edits = new TomlEdits(source); + + if (bundled?.order !== undefined || external?.order !== undefined) + throw new ConfigMigrationError( + "diffr config has a plugin named order. Rename that plugin before migration; shape.order is reserved.", + ); + + for (const name of Object.keys(external ?? {})) + if ( + bundled?.[name] !== undefined && + name !== "group" && + name !== "hide-files" + ) + throw new ConfigMigrationError( + `diffr config has both bundled and external entries for ${name}. Choose one before migration.`, + ); + + // The independent group switch is retired, for either saved boolean value. + edits.remove("plugins.bundled.group"); + + if (bundled?.["hide-files"] !== undefined) { + const hide = legacyHideSchema.parse(bundled["hide-files"]); + + // Explicit classifier settings in a mixed v1 file take precedence. + if (config.classifier?.hide === undefined) + edits.set("classifier.hide", hide.enabled ? hide.tags : []); + + if (config.classifier?.hide_deleted === undefined) + edits.set("classifier.hide_deleted", hide.enabled && hide.deleted); + edits.remove("plugins.bundled.hide-files"); + } + + if (config.plugins?.order) { + const order = config.plugins.order; + + const migrated = + JSON.stringify(order) === JSON.stringify(oldOrder) + ? newOrder + : order + .filter( + (name) => + name !== "bundled.group" && name !== "bundled.hide-files", + ) + .map((name) => name.replace(/^(bundled|external)\./, "")); + + edits.set("plugins.order", migrated); + edits.move("plugins.order", "shape.order"); + } + + // This old option was accepted but ignored. Do not turn it into --jobs. + edits.remove("plugins.bundled.summarize.max_concurrency"); + + const summary = + bundled?.summarize === undefined + ? undefined + : legacySummarySchema.parse(bundled.summarize); + + // Keep custom prompts exactly; reset only these historical stock defaults. + const resetPrompt = + summary?.system_prompt !== undefined && + stockPrompts.has(summary.system_prompt); + + if (resetPrompt) edits.remove("plugins.bundled.summarize.system_prompt"); + + for (const name of Object.keys(bundled ?? {})) + if (name !== "group" && name !== "hide-files") + edits.move(tomlKey("plugins", "bundled", name), tomlKey("shape", name)); + + for (const name of Object.keys(external ?? {})) + edits.move(tomlKey("plugins", "external", name), tomlKey("shape", name)); + + edits.remove("plugins"); + edits.move("classifier", tomlKey("classify", classifierName)); + edits.set("version", 2); + + const movedValues: [string, string][] = []; + + for (const key of new TomlEdits(source).valueKeys()) { + if ( + key.startsWith("plugins.bundled.group.") || + key.startsWith("plugins.bundled.hide-files.") || + key === "plugins.bundled.summarize.max_concurrency" || + (key === "plugins.bundled.summarize.system_prompt" && resetPrompt) + ) + continue; + + if (/^plugins\.(bundled|external)\./.test(key)) + movedValues.push([ + key, + key.replace(/^plugins\.(bundled|external)\./, "shape."), + ]); + + if (key.startsWith("classifier.")) + movedValues.push([ + key, + key.replace("classifier.", `${tomlKey("classify", classifierName)}.`), + ]); + } + + edits.assertUnchanged(source, movedValues); + + return edits.source; +} + +/** Run once at startup; v2 files do not need binary probing or validation. */ +export async function ensureDiffrConfigMigrated( + executable: string, + env: NodeJS.ProcessEnv = process.env, + signal?: AbortSignal, + cwd?: string, +): Promise { + const file = path.resolve( + cwd ?? process.cwd(), + env.XDG_CONFIG_HOME || path.join(homedir(), ".config"), + "diffr", + "config.toml", + ); + + const target = await realpath(file).catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT") return undefined; + throw error; + }); + + if (!target) return false; + + const result = await withFileLock( + `${target}.whiteboard-migration.lock`, + { + retryMs: 25, + staleMs: 120_000, + timeoutMs: 30_000, + unownedGraceMs: 5_000, + }, + async () => { + signal?.throwIfAborted(); + const original = await readFile(target, "utf8"); + let candidate: string; + + try { + const version = configVersionSchema.parse(parse(original)).version ?? 1; + + if (version === 2) return false; + + if (version !== 1) + throw new ConfigMigrationError( + `Unsupported diffr config version: ${version}`, + ); + const classifierName = await legacyClassifierName(original, file); + candidate = migrateDiffrConfig(original, classifierName); + } catch (error) { + if (error instanceof ConfigMigrationError) throw error; + // TOML and schema errors can contain the source and saved credentials. + throw new Error( + "diffr could not migrate the saved config. The original file was kept.", + ); + } + + if (candidate === original) return false; + + // Validate from a sibling config directory, with relative paths made absolute. + await validateDiffrConfig(executable, candidate, file, env, signal, cwd); + + if ((await readFile(target, "utf8")) !== original) + throw new Error("diffr config changed during migration. Try again."); + + const hash = createHash("sha256") + .update(original) + .digest("hex") + .slice(0, 16); + + const backup = `${target}.before-whiteboard-config-v2.${hash}`; + + try { + await writeFile(backup, original, { flag: "wx", mode: 0o600 }); + } catch (error) { + if ( + !( + error instanceof Error && + "code" in error && + error.code === "EEXIST" + ) + ) + throw error; + + if ((await readFile(backup, "utf8")) !== original) + throw new Error( + "diffr migration backup already exists for different config. Keep it and move it before retrying.", + ); + } + + signal?.throwIfAborted(); + + if ((await readFile(target, "utf8")) !== original) + throw new Error("diffr config changed during migration. Try again."); + await writeFileAtomicAsync(target, candidate, { + encoding: "utf8", + mode: 0o600, + }); + + return true; + }, + ); + + if (!result.acquired) throw new Error("diffr config is busy. Try again."); + + return result.result; +} + +async function legacyClassifierName( + source: string, + file: string, +): Promise { + const classifier = legacyConfigSchema.parse(parse(source)).classifier; + + if (classifier?.path === undefined) return "classify"; + + const directory = path.resolve( + path.dirname(file), + z.string().parse(classifier.path), + ); + + try { + const manifest = z + .object({ name: z.string().min(1) }) + .parse( + parse(await readFile(path.join(directory, "plugin.toml"), "utf8")), + ); + + return manifest.name; + } catch { + throw new ConfigMigrationError( + "diffr could not read the custom classifier manifest. The original file was kept.", + ); + } +} + +async function validateDiffrConfig( + executable: string, + source: string, + file: string, + env: NodeJS.ProcessEnv, + signal?: AbortSignal, + cwd?: string, +): Promise { + const directory = await mkdtemp( + path.join(path.dirname(file), ".whiteboard-config-"), + ); + + try { + const edits = new TomlEdits(source); + + // Moving the candidate for validation must not move theme or plugin paths. + for (const key of edits.keys()) { + const parts = tomlKeyParts(key); + + if ( + key === "theme.path" || + (parts.length === 3 && + (parts[0] === "shape" || parts[0] === "classify") && + parts[2] === "path") + ) { + const value = z.string().parse(edits.value(key)); + + if (!path.isAbsolute(value)) + edits.set(key, path.resolve(path.dirname(file), value)); + } + } + + await mkdir(path.join(directory, "diffr"), { mode: 0o700 }); + await writeFile( + path.join(directory, "diffr", "config.toml"), + edits.source, + { mode: 0o600 }, + ); + + try { + await execute(executable, ["config", "show", "--json"], { + env: { ...env, XDG_CONFIG_HOME: directory }, + cwd, + signal: signal ?? AbortSignal.timeout(30_000), + maxBuffer: 16 * 1024 * 1024, + }); + } catch { + signal?.throwIfAborted(); + throw new Error( + "diffr could not validate the migrated config. The original file was kept.", + ); + } + } finally { + await rm(directory, { recursive: true, force: true }); + } +} diff --git a/packages/review/src/server/diffr-config.test.ts b/packages/review/src/server/diffr-config.test.ts index 4699803f7..ebca7b2e2 100644 --- a/packages/review/src/server/diffr-config.test.ts +++ b/packages/review/src/server/diffr-config.test.ts @@ -114,7 +114,11 @@ const draft = { tests: true, }; -async function fakeDiffr(key = "", summarize: JsonObject = {}) { +async function fakeDiffr( + key = "", + summarize: JsonObject = {}, + classifier?: JsonObject, +) { const root = await mkdtemp(path.join(tmpdir(), "review-diffr-config-")); roots.push(root); @@ -125,19 +129,20 @@ async function fakeDiffr(key = "", summarize: JsonObject = {}) { await writeFile( state, JSON.stringify({ - plugins: { - bundled: { - summarize: { - enabled: false, - provider: "gemini", - model: "old", - tests: false, - system_prompt: DEFAULT_PROMPT, - ...(key && { api_key: key }), - ...summarize, - }, - context: { lines: 3, enabled: true }, + version: 2, + ...(classifier && { classify: { classify: classifier } }), + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + summarize: { + enabled: false, + provider: "gemini", + model: "old", + tests: false, + system_prompt: DEFAULT_PROMPT, + ...(key && { api_key: key }), + ...summarize, }, + context: { lines: 3, enabled: true }, }, }), ); @@ -153,23 +158,24 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { process.exit(2); } else if (args[0] === 'config' && args[1] === 'schema') { const summarize = ${JSON.stringify(SUMMARIZE_SCHEMA)}; - console.log(JSON.stringify({ properties: { plugins: { properties: { bundled: { properties: { summarize } } } } } })); + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + console.log(JSON.stringify({ properties: { ${classifier ? 'classify: { properties: { classify: { properties: { hide: { default: ["generated", "vendored"] } } } } },' : ""} shape: { properties: { summarize } } } })); } else if (args[0] === 'config' && args[1] === 'show') { - if (!args.includes('--reveal') && config.plugins.bundled.summarize.api_key) config.plugins.bundled.summarize.api_key = ''; + if (!args.includes('--reveal') && config.shape.summarize.api_key) config.shape.summarize.api_key = ''; console.log(JSON.stringify(config)); } else if (args[0] === 'config' && args[1] === 'set') { if (args[2] === process.env.FAIL_KEY) { console.error('command leaked secret: ' + args.join(' ')); process.exit(2); } const keys = args[2].split('.'); let object = config; for (const part of keys.slice(0,-1)) object = object[part]; let value = args[3]; try { value = JSON.parse(value); } catch {} - // diffr 0.1.10 clears a provider's own settings when the provider changes. + // diffr clears a provider's own settings when the provider changes. if (process.env.FAKE_RESET && args[2].endsWith('.provider') && object.provider !== value) { for (const option of ['api_key', 'endpoint', 'model']) delete object[option]; } object[keys.at(-1)] = value; fs.writeFileSync(state, JSON.stringify(config)); } else if (!args.includes('--no-index')) { - console.log(JSON.stringify({type:'start',version:4,lhs:{type:'revision',rev:'base'},rhs:{type:'revision',rev:'head'},files:[]})); + console.log(JSON.stringify({type:'start',version:3,lhs:{type:'revision',rev:'base'},rhs:{type:'revision',rev:'head'},files:[]})); console.log(JSON.stringify({type:'complete',succeeded:0,failed:0})); } else { const temporary = require('node:path').join(process.env.XDG_CONFIG_HOME, 'diffr', 'config.toml'); @@ -181,7 +187,7 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { if (mode === 'hang') { setTimeout(() => {}, 10000); } else { const file = {rhs:{path:'after.rs',oid:'',mode:''}}; - console.log(JSON.stringify({type:'annotations',file,annotations:mode === 'empty' ? [] : [{region_id:1,label:'count positive values'}]})); + console.log(JSON.stringify({type:'file',file,diff:{type:'text',rhs:{text:'sample',root:{kind:'leaf',id:1,fold_state_id:1,alignment_id:1,start:{line:0,column:0},end:{line:0,column:6},visibility:mode === 'empty' ? undefined : {collapsed:true,label:'count positive values'}}},structural_changes:{base:[],head:[[0,1]]},stats:{textual:{added:1,removed:0},visible:{added:0,removed:0}}}})); console.log(JSON.stringify({type:'complete',succeeded:1,failed:0})); } } @@ -190,6 +196,8 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { ); vi.stubEnv("REVIEW_DIFFR_BINARY", file); + vi.stubEnv("XDG_CONFIG_HOME", path.join(root, "xdg")); + return { root, state, @@ -218,15 +226,69 @@ test("detects host environment credentials and missing credentials", async () => expect((await readDiffrConfig()).credentialSource).toBe("environment"); }); -test("writes a setting, rereads it, and avoids invalidation for a no-op", async () => { - await fakeDiffr(); +test("reads classifier values and schema defaults without old plugin keys", async () => { + await fakeDiffr( + "saved-secret", + {}, + { hide: ["test", "custom"], hide_deleted: false }, + ); + const config = await readDiffrConfig(); + expect(config.values).toMatchObject({ + classify: { classify: { hide: ["test", "custom"], hide_deleted: false } }, + }); + expect(config.defaultHiddenTags).toEqual(["generated", "vendored"]); + expect(JSON.stringify(config)).not.toContain("saved-secret"); +}); + +test("writes classifier values without changing credentials", async () => { + const fake = await fakeDiffr( + "saved-secret", + {}, + { hide: ["custom"], hide_deleted: true }, + ); + expect( - (await setDiffrConfigValue("plugins.bundled.context.lines", 3)).changed, + (await setDiffrConfigValue("classify.classify.hide", ["custom"])).changed, ).toBe(false); - const result = await setDiffrConfigValue("plugins.bundled.context.lines", 8); + await setDiffrConfigValue("classify.classify.hide", []); + await setDiffrConfigValue("classify.classify.hide_deleted", false); + const disabled = JSON.parse(await readFile(fake.state, "utf8")); + expect(disabled.classify.classify).toEqual({ hide: [], hide_deleted: false }); + await setDiffrConfigValue("classify.classify.hide", ["custom"]); + const enabled = JSON.parse(await readFile(fake.state, "utf8")); + expect(enabled.classify.classify).toEqual({ + hide: ["custom"], + hide_deleted: false, + }); + expect(enabled.shape.summarize.api_key).toBe("saved-secret"); + expect(enabled.shape["hide-files"]).toBeUndefined(); +}); + +test("writes the native path of a custom classifier whose name contains spaces", async () => { + const fake = await fakeDiffr("saved-secret", {}, { hide: ["team"] }); + const state = JSON.parse(await readFile(fake.state, "utf8")); + state.classify = { "team classifier": state.classify.classify }; + await writeFile(fake.state, JSON.stringify(state)); + + const result = await setDiffrConfigValue("classify.team classifier.hide", []); + expect(result.error).toBeUndefined(); + expect(result.values).toMatchObject({ + classify: { "team classifier": { hide: [] } }, + }); + expect(Object.keys(result.values.classify!)).toEqual(["team classifier"]); + expect(JSON.stringify(result)).not.toContain("saved-secret"); +}); + +test("writes a setting, rereads it, and avoids invalidation for a no-op", async () => { + await fakeDiffr(); + expect((await setDiffrConfigValue("shape.context.lines", 3)).changed).toBe( + false, + ); + const result = await setDiffrConfigValue("shape.context.lines", 8); expect(result).toMatchObject({ changed: true, - values: { plugins: { bundled: { context: { lines: 8 } } } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + values: { shape: { context: { lines: 8 } } }, }); }); @@ -236,13 +298,12 @@ test("saves credentials and options before enabling and preserves blank keys", a const writes = (await fake.calls()).filter((args) => args[1] === "set"); expect(writes.map((args) => args[2])).toEqual( ["api_key", "model", "tests", "enabled"].map( - (key) => `plugins.bundled.summarize.${key}`, + (key) => `shape.summarize.${key}`, ), ); await saveDiffrSummarizer({ ...draft, apiKey: "" }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize - .api_key, + JSON.parse(await readFile(fake.state, "utf8")).shape.summarize.api_key, ).toBe("test-secret"); }); @@ -251,7 +312,7 @@ test("disables first and serializes concurrent saves", async () => { await saveDiffrSummarizer(draft); await Promise.all([ saveDiffrSummarizer({ ...draft, enabled: false, model: "next" }), - setDiffrConfigValue("plugins.bundled.context.lines", 9), + setDiffrConfigValue("shape.context.lines", 9), ]); const writes = (await fake.calls()) @@ -259,27 +320,28 @@ test("disables first and serializes concurrent saves", async () => { .slice(-3); expect(writes.map((args) => args[2])).toEqual([ - "plugins.bundled.summarize.enabled", - "plugins.bundled.summarize.model", - "plugins.bundled.context.lines", + "shape.summarize.enabled", + "shape.summarize.model", + "shape.context.lines", ]); }); test("partial failure returns current values and invalidates without leaking the key", async () => { await fakeDiffr(); - vi.stubEnv("FAIL_KEY", "plugins.bundled.summarize.model"); + vi.stubEnv("FAIL_KEY", "shape.summarize.model"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); expect(result.changed).toBe(true); expect(result.error).toContain("Some settings were saved"); expect(result.values).toMatchObject({ - plugins: { bundled: { summarize: { enabled: false, model: "old" } } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { summarize: { enabled: false, model: "old" } }, }); expect(JSON.stringify(result)).not.toContain("test-secret"); }); test("failed key saves do not leak args or stderr and do not invalidate", async () => { await fakeDiffr(); - vi.stubEnv("FAIL_KEY", "plugins.bundled.summarize.api_key"); + vi.stubEnv("FAIL_KEY", "shape.summarize.api_key"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); expect(result.changed).toBe(false); expect(result.error).toBeDefined(); @@ -380,12 +442,12 @@ test("saved changes invalidate cached comparisons while no-op saves reuse them", try { await consume(); - await setDiffrConfigValue("plugins.bundled.context.lines", 3); + await setDiffrConfigValue("shape.context.lines", 3); await consume(); expect( (await fake.calls()).filter((args) => args[0] === "--repo"), ).toHaveLength(1); - await setDiffrConfigValue("plugins.bundled.context.lines", 8); + await setDiffrConfigValue("shape.context.lines", 8); await consume(); expect( (await fake.calls()).filter((args) => args[0] === "--repo"), @@ -414,7 +476,7 @@ test("saves a changed prompt and leaves an unchanged one alone", async () => { systemPrompt: "Be terse.", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize + JSON.parse(await readFile(fake.state, "utf8")).shape.summarize .system_prompt, ).toBe("Be terse."); }); @@ -423,7 +485,7 @@ test("switching providers clears the saved key unless a new one is entered", asy const fake = await fakeDiffr("gemini-secret"); const state = async () => - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize; + JSON.parse(await readFile(fake.state, "utf8")).shape.summarize; await saveDiffrSummarizer({ ...draft, @@ -502,7 +564,7 @@ test("reads the environment key of the saved provider", async () => { test("a switch clears the saved key before naming the new provider", async () => { const fake = await fakeDiffr("gemini-secret"); - vi.stubEnv("FAIL_KEY", "plugins.bundled.summarize.provider"); + vi.stubEnv("FAIL_KEY", "shape.summarize.provider"); const result = await saveDiffrSummarizer({ ...draft, @@ -512,8 +574,7 @@ test("a switch clears the saved key before naming the new provider", async () => expect(result.error).toBeDefined(); - const state = JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled - .summarize; + const state = JSON.parse(await readFile(fake.state, "utf8")).shape.summarize; // The failure left the old provider, and no key for it to send. expect(state).toMatchObject({ provider: "gemini", api_key: "" }); @@ -525,7 +586,7 @@ test("a new endpoint is treated like a new provider", async () => { await expect(testDiffrSummarizer(moved)).rejects.toThrow("Add an API key"); await saveDiffrSummarizer({ ...moved, enabled: false }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize, + JSON.parse(await readFile(fake.state, "utf8")).shape.summarize, ).toMatchObject({ api_key: "", endpoint: "https://proxy.example/v1" }); }); @@ -619,7 +680,7 @@ test("a custom endpoint kept across a provider switch survives diffr clearing it apiKey: "openai-key", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize, + JSON.parse(await readFile(fake.state, "utf8")).shape.summarize, ).toMatchObject({ provider: "openai", endpoint: "https://gateway.example/v1", diff --git a/packages/review/src/server/diffr-config.ts b/packages/review/src/server/diffr-config.ts index c7e137875..75bfbba92 100644 --- a/packages/review/src/server/diffr-config.ts +++ b/packages/review/src/server/diffr-config.ts @@ -15,6 +15,7 @@ import { type ReviewDiffrConfig, type ReviewDiffrProvider, type ReviewDiffrSummarizerInput, + type StructuralRegion, decodeStructuralDiffEvent, reviewDiffrSummarizerInputSchema, } from "@dev.fast/review-protocol"; @@ -36,13 +37,39 @@ function serialized(operation: () => Promise): Promise { return next; } +type DiffrArguments = + | { command: "schema" } + | { command: "show"; reveal: boolean } + | { command: "set"; key: string; value: JsonValue } + | { command: "diff"; before: string; after: string }; + +function diffrArgv(args: DiffrArguments): string[] { + switch (args.command) { + case "schema": + return ["config", "schema"]; + case "show": + return ["config", "show", "--json", ...(args.reveal ? ["--reveal"] : [])]; + case "set": + return ["config", "set", args.key, diffrConfigValueText(args.value)]; + case "diff": + return [ + "--no-index", + "--format", + "ndjson", + "--", + args.before, + args.after, + ]; + } +} + async function diffr( - args: string[], + args: DiffrArguments, rootPath?: string, options: { signal?: AbortSignal; env?: NodeJS.ProcessEnv } = {}, ): Promise { try { - const { stdout } = await execFileAsync(diffrExecutable(), args, { + const { stdout } = await execFileAsync(diffrExecutable(), diffrArgv(args), { cwd: rootPath, maxBuffer: 16 * 1024 * 1024, signal: options.signal ?? AbortSignal.timeout(30_000), @@ -70,13 +97,7 @@ async function values( reveal = false, signal?: AbortSignal, ): Promise { - return json( - await diffr( - ["config", "show", "--json", ...(reveal ? ["--reveal"] : [])], - rootPath, - { signal }, - ), - ); + return json(await diffr({ command: "show", reveal }, rootPath, { signal })); } function json(output: string): JsonObject { @@ -100,7 +121,7 @@ function valueAt(object: JsonObject, key: string): JsonValue | undefined { return value; } -const prefix = "plugins.bundled.summarize"; +const prefix = "shape.summarize"; function savedProvider(config: JsonObject): string { const value = valueAt(config, `${prefix}.provider`); @@ -151,27 +172,35 @@ function strings(value: JsonValue | undefined): string[] { } /** - * What diffr's schema says about summaries: each provider's defaults, and - * the default prompt with the link its description gives. Nothing when - * diffr cannot describe its schema: the rest of Settings still works. + * Defaults supplied by diffr's schema for tag hiding and summary controls. + * If the schema cannot be read, resolved settings still work. */ -async function summaryDefaults( +async function configDefaults( rootPath?: string, -): Promise> { +): Promise< + Pick +> { let schema: JsonObject; try { - schema = json(await diffr(["config", "schema"], rootPath)); + schema = json(await diffr({ command: "schema" }, rootPath)); } catch { return {}; } const options = valueAt( schema, - "properties.plugins.properties.bundled.properties.summarize.properties", + "properties.shape.properties.summarize.properties", + ); + + const defaultHiddenTags = strings( + valueAt( + schema, + "properties.classify.properties.classify.properties.hide.default", + ), ); - if (!isJsonObject(options)) return {}; + if (!isJsonObject(options)) return { defaultHiddenTags }; const option = (name: string) => isJsonObject(options[name]) ? options[name] : {}; @@ -200,6 +229,7 @@ async function summaryDefaults( const prompt = option("system_prompt"); return { + defaultHiddenTags, providers, defaultPrompt: isStringValue(prompt.default) ? prompt.default : undefined, }; @@ -207,7 +237,7 @@ async function summaryDefaults( async function read(rootPath?: string): Promise { const config = await values(rootPath); - const defaults = await summaryDefaults(rootPath); + const defaults = await configDefaults(rootPath); const saved = valueAt(config, `${prefix}.api_key`); const credentialSource = @@ -217,16 +247,11 @@ async function read(rootPath?: string): Promise { ? "environment" : "missing"; - const plugins = config.plugins; + const plugins = config.shape; if (isJsonObject(plugins)) { - for (const namespace of Object.values(plugins)) { - if (!isJsonObject(namespace)) continue; - - for (const plugin of Object.values(namespace)) { - if (isJsonObject(plugin)) delete plugin.api_key; - } - } + for (const plugin of Object.values(plugins)) + if (isJsonObject(plugin)) delete plugin.api_key; } return { values: config, credentialSource, ...defaults }; @@ -250,11 +275,9 @@ async function writeSettings( try { for (const [key, value] of entries) { - if (valueAt(current, key) === value) continue; - await diffr( - ["config", "set", key, diffrConfigValueText(value)], - rootPath, - ); + if (JSON.stringify(valueAt(current, key)) === JSON.stringify(value)) + continue; + await diffr({ command: "set", key, value }, rootPath); changed = true; // A write can clear others, such as a provider's endpoint and key. current = await values(rootPath); @@ -275,9 +298,7 @@ export function setDiffrConfigValue( value: JsonValue, rootPath?: string, ): Promise { - if ( - !/^[A-Za-z0-9_][A-Za-z0-9_-]*(\.[A-Za-z0-9_][A-Za-z0-9_-]*)*$/.test(key) - ) { + if (!/^[A-Za-z0-9_][A-Za-z0-9_-]*(\.[^.\r\n\0]+)*$/u.test(key)) { throw new Error("Invalid diffr config key."); } @@ -373,7 +394,7 @@ export async function testDiffrSummarizer( throw new Error("The summarizer is not available in this configuration."); const { provider, endpoint, model, systemPrompt } = parsed.data; - const providers = (await summaryDefaults(rootPath)).providers ?? []; + const providers = (await configDefaults(rootPath)).providers ?? []; const variables = providers.find((known) => known.id === provider)?.keyVariables ?? []; @@ -410,10 +431,11 @@ export async function testDiffrSummarizer( await writeFile( join(directory, "diffr", "config.toml"), stringify({ - version: 1, - plugins: { - order: ["bundled.summarize"], - bundled: { summarize: options }, + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + order: ["summarize"], + summarize: options, }, }), { mode: 0o600 }, @@ -425,56 +447,47 @@ export async function testDiffrSummarizer( await writeFile(before, ""); await writeFile(after, SUMMARY_SAMPLE); - const output = await diffr( - [ - "--no-index", - "--format", - "ndjson", - "--stream-annotations", - "--", - before, - after, - ], - directory, - { - env: { - ...process.env, - ...Object.fromEntries( - providers - .flatMap((known) => known.keyVariables) - .map((name) => [name, ""]), - ), - ...(apiKey && variables[0] && { [variables[0]]: apiKey }), - XDG_CONFIG_HOME: directory, - }, - signal: deadline, + const output = await diffr({ command: "diff", before, after }, directory, { + env: { + ...process.env, + ...Object.fromEntries( + providers + .flatMap((known) => known.keyVariables) + .map((name) => [name, ""]), + ), + ...(apiKey && variables[0] && { [variables[0]]: apiKey }), + XDG_CONFIG_HOME: directory, }, - ); + signal: deadline, + }); try { const events = output.trim().split("\n").map(decodeStructuralDiffEvent); const complete = events.at(-1); - const annotation = events + const labels = (region: StructuralRegion): string[] => [ + ...(region.visibility?.label ? [region.visibility.label] : []), + ...(region.kind === "fold" ? region.children.flatMap(labels) : []), + ]; + + const summary = events .flatMap((event) => - event.type === "annotations" ? event.annotations : [], + event.type === "file" && event.diff?.type === "text" + ? [event.diff.rhs, event.diff.lhs].flatMap((source) => + source ? labels(source.root) : [], + ) + : [], ) - .find((item) => item.label.trim()); + .find((label) => label.trim()); if ( complete?.type === "complete" && complete.failed === 0 && !complete.aborted && - !events.some( - (event) => - (event.type === "annotations" || event.type === "file") && - event.error, - ) && - annotation + !events.some((event) => event.type === "file" && event.error) && + summary ) { - return apiKey - ? annotation.label.split(apiKey).join("[redacted]") - : annotation.label; + return apiKey ? summary.split(apiKey).join("[redacted]") : summary; } } catch { /* Never return provider output or parser causes. */ diff --git a/packages/review/src/server/diffr-startup.test.ts b/packages/review/src/server/diffr-startup.test.ts new file mode 100644 index 000000000..4a7882298 --- /dev/null +++ b/packages/review/src/server/diffr-startup.test.ts @@ -0,0 +1,107 @@ +import { readFileSync } from "node:fs"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; + +import { openLocalReviewStore } from "@review/review-api/local-data.js"; +import { parse } from "smol-toml"; +import { afterEach, expect, test, vi } from "vitest"; + +import { createGlobalReviewServer } from "./desktop-server.js"; +import { runHeadlessServer } from "./headless-host.js"; + +const roots: string[] = []; + +afterEach(async () => { + vi.unstubAllEnvs(); + await Promise.all( + roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), + ); +}); + +test.each(["desktop", "headless"] as const)( + "%s startup migrates saved settings before ready and skips migration on restart", + async (host) => { + const root = await mkdtemp(path.join(tmpdir(), "review-diffr-startup-")); + roots.push(root); + vi.stubEnv("DEV_REVIEW_HOME", root); + vi.stubEnv("XDG_CONFIG_HOME", root); + vi.stubEnv("DEV_FAST_REVIEW_TELEMETRY_DISABLED", "1"); + const file = path.join(root, "diffr", "config.toml"); + await mkdir(path.dirname(file)); + await writeFile( + file, + "version = 1\n[plugins.bundled.context]\nlines = 17\nenabled = false\n", + ); + const executable = path.join(root, "diffr-cli"); + const log = path.join(root, "validations"); + await writeFile( + executable, + `#!${process.execPath} +const fs = require('node:fs'); +const path = require('node:path'); +const source = fs.readFileSync(path.join(process.env.XDG_CONFIG_HOME, 'diffr', 'config.toml'), 'utf8'); +if (!/version = 2/.test(source)) process.exit(2); +fs.appendFileSync(${JSON.stringify(log)}, 'validated\\n'); +console.log('{}'); +`, + { mode: 0o755 }, + ); + vi.stubEnv("REVIEW_DIFFR_BINARY", executable); + + const ready = async () => { + expect(parse(await readFile(file, "utf8"))).toMatchObject({ + version: 2, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { context: { lines: 17, enabled: false } }, + }); + }; + + const start = async () => { + let configAtReady: string | undefined; + + if (host === "headless") { + const controller = new AbortController(); + + const running = runHeadlessServer({ + stateDir: path.join(root, "server"), + signal: controller.signal, + onReady: () => { + configAtReady = readFileSync(file, "utf8"); + controller.abort(); + }, + }); + + await running; + await ready(); + } else { + const local = openLocalReviewStore(path.join(root, "reviews.db")); + + const server = createGlobalReviewServer({ + reviewStore: local.store, + reviewData: local.data, + appPid: process.pid, + packageRoot: root, + toolingRoot: root, + port: 0, + }); + + try { + await server.listen(); + configAtReady = readFileSync(file, "utf8"); + await ready(); + } finally { + await server.close(); + await local.data.close(); + await local.store.close(); + } + } + + expect(parse(configAtReady!)).toMatchObject({ version: 2 }); + }; + + await start(); + await start(); + expect(await readFile(log, "utf8")).toBe("validated\n"); + }, +); diff --git a/packages/review/src/server/headless-host.ts b/packages/review/src/server/headless-host.ts index 34815d86b..045c1788a 100644 --- a/packages/review/src/server/headless-host.ts +++ b/packages/review/src/server/headless-host.ts @@ -12,6 +12,7 @@ import { reviewServerDiscoveryPath, } from "@review/server-discovery.js"; +import { ensureDiffrConfigMigrated } from "./diffr-config-migration.js"; import { GlobalReviewDesktopVerbRelay } from "./global-verb-relay.js"; import { createNodeRequestListener } from "./hono-http.js"; import { @@ -19,6 +20,7 @@ import { installProcessErrorTelemetry, } from "./process-error-telemetry.js"; import { createWhiteboardCore } from "./review-server-core.js"; +import { diffrExecutable } from "./structural-diff.js"; import type { ReviewTelemetryCapture } from "./ui-telemetry.js"; interface HeadlessServerInput { @@ -72,6 +74,8 @@ export function withHeadlessServerLock( async function serve(input: HeadlessServerInput) { if (input.signal.aborted) return; + await ensureDiffrConfigMigrated(diffrExecutable(), process.env, input.signal); + if (input.telemetry) await drainServerCrashReport(input.telemetry); const local = await openReviewProfile(input.stateDir, { diff --git a/packages/review/src/server/structural-diff-workspace.test.ts b/packages/review/src/server/structural-diff-workspace.test.ts index 8964486aa..9e7459a53 100644 --- a/packages/review/src/server/structural-diff-workspace.test.ts +++ b/packages/review/src/server/structural-diff-workspace.test.ts @@ -73,18 +73,22 @@ const {execFileSync} = require('node:child_process'); const args = process.argv.slice(2); const repo = args[args.indexOf('--repo') + 1]; const revisions = args - .slice(args.indexOf('ndjson') + 1, args.indexOf('--')) - .filter(arg => arg !== '--stream-annotations'); + .slice(args.indexOf('ndjson') + 1, args.indexOf('--')); const range = revisions.length === 1 ? revisions[0].split('...') : revisions; const file = args[args.indexOf('--') + 1]; -const read = rev => execFileSync('git', ['-C', repo, 'show', rev + ':' + file], {encoding:'utf8'}); -console.log(JSON.stringify({type:'start',version:4,lhs:{type:'revision',rev:range[0]},rhs:{type:'revision',rev:range[1]},files:[{file:{rhs:{path:file,oid:range[1],mode:'100644'}},status:'modified'}]})); -console.log(JSON.stringify({type:'file',file:{rhs:{path:file,oid:range[1],mode:'100644'}},diff:{type:'text',lhs:{text:read(range[0])},rhs:{text:read(range[1])},stats:{textual:{added:1,removed:1},visible:{added:1,removed:1}},structural_changes:{base:[[0,1]],head:[[0,1]]}}})); +const read = rev => { + const text = execFileSync('git', ['-C', repo, 'show', rev + ':' + file], {encoding:'utf8'}); + const lines = text.split('\\n'); + return {text,root:{kind:'leaf',id:1,fold_state_id:1,alignment_id:1,start:{line:0,column:0},end:{line:lines.length-1,column:lines.at(-1).length}}}; +}; +console.log(JSON.stringify({type:'start',version:3,lhs:{type:'revision',rev:range[0]},rhs:{type:'revision',rev:range[1]},files:[{file:{rhs:{path:file,oid:range[1],mode:'100644'}},status:'modified'}]})); +console.log(JSON.stringify({type:'file',file:{rhs:{path:file,oid:range[1],mode:'100644'}},diff:{type:'text',lhs:read(range[0]),rhs:read(range[1]),stats:{textual:{added:1,removed:1},visible:{added:1,removed:1}},structural_changes:{base:[[0,1]],head:[[0,1]]}}})); console.log(JSON.stringify({type:'complete',succeeded:1,failed:0})); `, { mode: 0o755 }, ); vi.stubEnv("REVIEW_DIFFR_BINARY", executable); + vi.stubEnv("XDG_CONFIG_HOME", path.join(root, "config")); const local = openLocalReviewStore(path.join(root, "reviews.db")); try { diff --git a/packages/review/src/server/structural-diff.test.ts b/packages/review/src/server/structural-diff.test.ts index 471d85e8b..5b22b20ae 100644 --- a/packages/review/src/server/structural-diff.test.ts +++ b/packages/review/src/server/structural-diff.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from "node:os"; import path from "node:path"; import type { JsonValue } from "@dev.fast/json"; +import { STRUCTURAL_DIFF_WIRE_VERSION } from "@dev.fast/review-protocol"; import { afterEach, expect, test, vi } from "vitest"; import { type StructuralDiffRequest, structuralDiff } from "./structural-diff"; @@ -23,13 +24,14 @@ async function executable(script: string) { const file = path.join(root, "diffr"); await writeFile(file, `#!${process.execPath}\n${script}`, { mode: 0o755 }); vi.stubEnv("REVIEW_DIFFR_BINARY", file); + vi.stubEnv("XDG_CONFIG_HOME", path.join(root, "config")); return root; } const START = { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: [ @@ -92,7 +94,6 @@ test.each(["trees", "merge-base"] as const)( root, "--format", "ndjson", - "--stream-annotations", ...(kind === "trees" ? ["base", "head"] : ["base...head"]), "--", "space name.ts", @@ -185,7 +186,7 @@ test("streams a large comparison without treating accumulated file bytes as one for (const { file } of files) await write({ type: 'file', file, diff: { - type: 'text', rhs: { text: 'x'.repeat(1024 * 1024), regions: [] }, + type: 'text', rhs: { text: 'x'.repeat(1024 * 1024), root: {kind:'leaf',id:1,fold_state_id:1,alignment_id:1,start:{line:0,column:0},end:{line:0,column:1024*1024}} }, structural_changes: { base: [], head: [[0, 1]] }, stats: { textual: { added: 1, removed: 0 }, visible: { added: 1, removed: 0 } }, }, @@ -318,57 +319,6 @@ test("rendering and coverage share a stream; cancelling one reader preserves the } }); -test("annotation failure is data, preserves files, and explains exit 2", async () => { - const annotation = { - type: "annotations", - file: FILE, - annotations: [], - error: { code: "enrichment_failed", message: "offline" }, - }; - - const root = await executable( - `${emit(START)} ${emit(BINARY)} ${emit(annotation)} ${emit(COMPLETE)} process.exitCode = 2;`, - ); - - expect(await collect(request(root))).toEqual([ - START, - BINARY, - annotation, - COMPLETE, - ]); -}); - -test("coverage can detach after initial files while summaries continue for later readers", async () => { - const { StructuralComparisons } = await import("./structural-comparisons.js"); - - const annotation = { - type: "annotations", - file: FILE, - annotations: [{ region_id: 1, label: "summary" }], - }; - - const root = await executable(` - require('node:fs').appendFileSync('runs', 'x'); - ${emit(START)} ${emit(BINARY)} - const timer = setInterval(() => { if(require('node:fs').existsSync('continue')) { clearInterval(timer); ${emit(annotation)} ${emit(COMPLETE)} } }, 10); - `); - - const cache = new StructuralComparisons(); - - try { - for await (const event of cache.stream(request(root))) - if (event.type === "file") break; - await writeFile(path.join(root, "continue"), ""); - const events = []; - - for await (const event of cache.stream(request(root))) events.push(event); - expect(events).toEqual([START, BINARY, annotation, COMPLETE]); - expect(await readFile(path.join(root, "runs"), "utf8")).toBe("x"); - } finally { - cache.close(); - } -}); - test("keeps the ten most recently read idle comparisons", async () => { const { StructuralComparisons } = await import("./structural-comparisons.js"); diff --git a/packages/review/src/server/structural-diff.ts b/packages/review/src/server/structural-diff.ts index e2596c3ee..e787323f1 100644 --- a/packages/review/src/server/structural-diff.ts +++ b/packages/review/src/server/structural-diff.ts @@ -43,13 +43,7 @@ export async function* structuralDiff( input.signal.throwIfAborted(); const comparison = input.comparison; - const args = [ - "--repo", - input.repositoryPath, - "--format", - "ndjson", - "--stream-annotations", - ]; + const args = ["--repo", input.repositoryPath, "--format", "ndjson"]; switch (comparison.kind) { case "worktree": @@ -102,7 +96,6 @@ export async function* structuralDiff( let completed = false; let aborted: StructuralProblem | undefined; let failed = 0; - let annotationFailed = false; const lines = createInterface({ input: child.stdout, crlfDelay: Infinity }); try { @@ -134,8 +127,6 @@ export async function* structuralDiff( failed = event.failed; aborted = event.aborted; - } else if (event.type === "annotations") { - annotationFailed ||= event.error !== undefined; } else if (event.type !== "file") { throw new Error(`Unexpected diffr event: ${event.type}`); } @@ -150,13 +141,10 @@ export async function* structuralDiff( throw new Error("diffr stream ended before completion."); } - // diffr exits 2 for file or enrichment failures already reported by the stream. + // diffr exits 2 for file failures already reported by the stream. const code = await exited; - if ( - code !== 0 && - !(code === 2 && (failed > 0 || annotationFailed || aborted !== undefined)) - ) + if (code !== 0 && !(code === 2 && (failed > 0 || aborted !== undefined))) throw exitError(code); } finally { clearTimeout(idle); diff --git a/packages/review/src/server/test-fixtures/diffr-ui-overrides.toml b/packages/review/src/server/test-fixtures/diffr-ui-overrides.toml new file mode 100644 index 000000000..ee58a441a --- /dev/null +++ b/packages/review/src/server/test-fixtures/diffr-ui-overrides.toml @@ -0,0 +1,38 @@ +# Overrides saved by Whiteboard 0.1.10. Migration must retain these comments. +version = 1 + +[plugins.bundled.context] +enabled = false # Collapse unchanged lines +lines = 17 # Context lines + +[plugins.bundled.test-bodies] +enabled = false # Collapse test bodies + +[plugins.bundled.deleted-bodies] +enabled = false # Collapse deleted function bodies + +[plugins.bundled.removed-runs] +enabled = false # Collapse long removed stretches + +[plugins.bundled.group] +enabled = false # Group adjacent folds; retired + +[plugins.bundled.hide-files] +enabled = true # Hide files by tag +tags = ["test", "generated"] +deleted = false # Hide deleted files + +[plugins.bundled.summarize] +enabled = true # Enable summaries +provider = "openai" # Provider +api_key = "synthetic-key" # API key +model = "custom-model" # Model +endpoint = "http://127.0.0.1:9/v1" # Endpoint URL +system_prompt = ''' Use my exact custom instructions. +Keep # inside the prompt, and answer with JSON if I ask for it. +''' # Prompt +tests = false # Include tests +max_concurrency = 2 # Old ignored option + +[diff] +byte_limit = 123456 # Unrelated file-only override diff --git a/packages/review/src/server/toml-edits.ts b/packages/review/src/server/toml-edits.ts new file mode 100644 index 000000000..c5012f8de --- /dev/null +++ b/packages/review/src/server/toml-edits.ts @@ -0,0 +1,266 @@ +import { type AST, getStaticTOMLValue, parseTOML } from "toml-eslint-parser"; + +export function tomlKey(...parts: string[]): string { + return parts + .map((part) => + /^[A-Za-z0-9_-]+$/.test(part) ? part : JSON.stringify(part), + ) + .join("."); +} + +export function tomlKeyParts(key: string): string[] { + const node = parseTOML(`${key} = 0`, { tomlVersion: "1.0" }).body[0].body[0]; + + if (node.type !== "TOMLKeyValue") + throw new Error("Expected a TOML config key."); + + return getStaticTOMLValue(node.key); +} + +/** Small source edits through TOML's AST; do not serialize unrelated user values. */ +export class TomlEdits { + constructor(public source: string) {} + + private index() { + const ast = parseTOML(this.source, { tomlVersion: "1.0" }); + const fields = new Map(); + const tables = new Map(); + + const field = (node: AST.TOMLKeyValue, prefix: string[]) => { + const key = [...prefix, ...getStaticTOMLValue(node.key)]; + fields.set(tomlKey(...key), node); + + if (node.value.type === "TOMLInlineTable") + for (const child of node.value.body) field(child, key); + }; + + for (const node of ast.body[0].body) { + if (node.type === "TOMLKeyValue") field(node, []); + else { + const key = node.resolvedKey.map(String); + tables.set(tomlKey(...key), node); + + for (const child of node.body) field(child, key); + } + } + + return { ast, fields, tables }; + } + + keys(): string[] { + return [...this.index().fields.keys()]; + } + + valueKeys(): string[] { + return [...this.index().fields].flatMap(([key, node]) => + node.value.type === "TOMLInlineTable" ? [] : [key], + ); + } + + value(key: string) { + const node = this.index().fields.get(key); + + return node && getStaticTOMLValue(node.value); + } + + /** Retained settings must keep their exact saved value, including prompt spacing. */ + assertUnchanged( + original: string, + keys: readonly (readonly [string, string])[], + ): void { + const before = new TomlEdits(original).index().fields; + const after = this.index().fields; + + for (const [from, to] of keys) { + const previous = before.get(from); + const current = after.get(to); + const previousText = previous && original.slice(...previous.value.range); + const currentText = current && this.source.slice(...current.value.range); + + if (previousText !== currentText) + throw new Error(`diffr migration changed a retained setting: ${from}`); + } + } + + /** Move a config subtree without serializing any of its saved values. */ + move(from: string, to: string): void { + const { fields, tables } = this.index(); + const matches = (key: string) => key === from || key.startsWith(`${from}.`); + + if ( + [...fields.keys(), ...tables.keys()].some( + (key) => key === to || key.startsWith(`${to}.`), + ) + ) + throw new Error(`diffr migration has conflicting config entries: ${to}`); + + const direct = fields.get(from); + + if (direct) { + const literal = this.source.slice(...direct.value.range); + this.remove(from, false); + this.setLiteral(to, literal); + + return; + } + + const moves: { start: number; end: number; text: string }[] = []; + const movedTables = [...tables].filter(([key]) => matches(key)); + + for (const [key, table] of movedTables) + moves.push({ + start: table.key.range[0], + end: table.key.range[1], + text: `${to}${key.slice(from.length)}`, + }); + + const dotted: [string, string][] = []; + + for (const [key, field] of fields) { + if (!matches(key) || field.parent.type === "TOMLInlineTable") continue; + + // Fields in a moved table keep their original spelling and comments. + if ( + field.parent.type === "TOMLTable" && + movedTables.some(([, table]) => table === field.parent) + ) + continue; + dotted.push([ + `${to}${key.slice(from.length)}`, + this.source.slice(...field.value.range), + ]); + moves.push({ start: field.range[0], end: field.range[1], text: "" }); + } + + for (const edit of moves.sort((a, b) => b.start - a.start)) + this.replace(edit.start, edit.end, edit.text); + + for (const [key, literal] of dotted) this.setLiteral(key, literal); + } + + remove(key: string, retainComments = true): void { + while (true) { + const { ast, fields, tables } = this.index(); + + const matches = (name: string) => + name === key || name.startsWith(`${key}.`); + + const node = + fields.get(key) ?? + [...tables].find(([name]) => matches(name))?.[1] ?? + [...fields].find(([name]) => matches(name))?.[1]; + + if (!node) return; + let [start, end] = node.range; + + if ( + node.type === "TOMLKeyValue" && + node.parent.type === "TOMLInlineTable" + ) { + const siblings = node.parent.body; + const position = siblings.indexOf(node); + const next = siblings[position + 1]; + const previous = siblings[position - 1]; + + if (next) { + const comma = ast.tokens.find( + (token) => + token.value === "," && + token.range[0] >= end && + token.range[0] < next.range[0], + ); + + if (comma) end = comma.range[1]; + } else if (previous) { + const comma = ast.tokens.find( + (token) => + token.value === "," && + token.range[0] >= previous.range[1] && + token.range[0] < start, + ); + + if (comma) start = comma.range[0]; + } + } + + const comments = retainComments + ? ast.comments + .filter( + (comment) => comment.range[0] >= start && comment.range[1] <= end, + ) + .map((comment) => this.source.slice(...comment.range)) + .join("\n") + : ""; + + this.replace(start, end, comments); + } + } + + set(key: string, value: boolean | number | string | string[]): void { + this.setLiteral(key, JSON.stringify(value), Array.isArray(value)); + } + + private setLiteral(key: string, input: string, retainComments = false): void { + const { ast, fields, tables } = this.index(); + let literal = input; + const existing = fields.get(key); + + if (existing) { + // Keep notes between entries when a plugin order is rewritten. + if (retainComments) { + const comments = ast.comments + .filter( + (comment) => + comment.range[0] >= existing.value.range[0] && + comment.range[1] <= existing.value.range[1], + ) + .map((comment) => this.source.slice(...comment.range)); + + if (comments.length) + literal = `[\n${comments.join("\n")}\n${literal.slice(1)}`; + } + + this.replace(...existing.value.range, literal); + + return; + } + + const parts = tomlKeyParts(key); + + for (let length = parts.length - 1; length > 0; length--) { + const parent = tomlKey(...parts.slice(0, length)); + const suffix = tomlKey(...parts.slice(length)); + const inline = fields.get(parent)?.value; + + if (inline?.type === "TOMLInlineTable") { + this.replace( + inline.range[1] - 1, + inline.range[1] - 1, + `${inline.body.length ? ", " : ""}${suffix} = ${literal}`, + ); + + return; + } + + const table = tables.get(parent); + + if (table) { + const newline = this.source.indexOf("\n", table.key.range[1]); + const at = newline < 0 ? this.source.length : newline + 1; + this.replace( + at, + at, + `${newline < 0 ? "\n" : ""}${suffix} = ${literal}\n`, + ); + + return; + } + } + + this.source = `${key} = ${literal}\n${this.source}`; + } + + private replace(start: number, end: number, text: string): void { + this.source = this.source.slice(0, start) + text + this.source.slice(end); + } +} diff --git a/packages/review/vitest.config.ts b/packages/review/vitest.config.ts index 2ea18dcf4..a34e006d4 100644 --- a/packages/review/vitest.config.ts +++ b/packages/review/vitest.config.ts @@ -12,6 +12,10 @@ export default defineConfig({ os.tmpdir(), `progressive-review-tests-${process.pid}`, ), + XDG_CONFIG_HOME: path.join( + os.tmpdir(), + `progressive-review-config-tests-${process.pid}`, + ), // GitHub Actions exports the repository slug, which the trace code // honors over a checkout's remote; scratch repositories in tests must // resolve to their own remotes. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ef0a11c94..5b321f368 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -6,6 +6,12 @@ settings: catalogs: default: + '@dev.fast/diffr': + specifier: 0.1.12 + version: 0.1.12 + toml-eslint-parser: + specifier: ^1.0.3 + version: 1.0.3 zod: specifier: 4.6.5 version: 4.6.5 @@ -51,8 +57,8 @@ importers: version: link:../../packages/review devDependencies: '@dev.fast/diffr': - specifier: 0.1.11 - version: 0.1.11(zod@4.6.5) + specifier: 'catalog:' + version: 0.1.12(zod@4.6.5) '@dev.fast/review-canvas': specifier: workspace:* version: link:../../packages/review/app @@ -120,8 +126,8 @@ importers: specifier: 1.5.1 version: 1.5.1(zod@4.6.5) '@dev.fast/diffr': - specifier: 0.1.11 - version: 0.1.11(zod@4.6.5) + specifier: 'catalog:' + version: 0.1.12(zod@4.6.5) '@hono/node-server': specifier: 2.0.12 version: 2.0.12(hono@4.13.5) @@ -167,6 +173,9 @@ importers: smol-toml: specifier: 1.8.0 version: 1.8.0 + toml-eslint-parser: + specifier: 'catalog:' + version: 1.0.3 zod: specifier: 'catalog:' version: 4.6.5 @@ -217,8 +226,8 @@ importers: packages/review-protocol: dependencies: '@dev.fast/diffr': - specifier: 0.1.11 - version: 0.1.11(zod@4.6.5) + specifier: 'catalog:' + version: 0.1.12(zod@4.6.5) '@dev.fast/json': specifier: workspace:* version: link:../json @@ -554,35 +563,35 @@ packages: resolution: {integrity: sha512-QxULHAm7cNu72w97JUNCBFODFaXpbDg+dP8b/oWFAZ2MTRppA3U00Y2L1HqaS4J6yBqxwa/Y3nMBaxVKbB/NsA==} engines: {node: '>=20.19.0'} - '@dev.fast/diffr-darwin-arm64@0.1.11': - resolution: {integrity: sha512-GcjoOlroyk1gRnHorpxHERooHq41YUBsr84ZHj71yCdQmih6WRGgFBJsSfmXoiie4GRO/kbRgL5tP1ubJhFFUQ==} + '@dev.fast/diffr-darwin-arm64@0.1.12': + resolution: {integrity: sha512-x7MlUSQ467hE2u19shRZjFonxK1oXat/SRcakJ/hNIM/w5YH0+Sb8UqB2GUwcGXXkqoVQCjN25Kr52O/qmG3hQ==} cpu: [arm64] os: [darwin] - '@dev.fast/diffr-darwin-x64@0.1.11': - resolution: {integrity: sha512-nQ9kj7fsy8U7Xj5QH2ZBUB9ie/htfzi0X6nVh/6/VlQUJqFWe0CnsyAx/R5k/oNH129n9BnOvzJXMDtv1gTbCg==} + '@dev.fast/diffr-darwin-x64@0.1.12': + resolution: {integrity: sha512-CuH9D/ETnHA5nVPDgqTxk2nC1Esgn7aNqlLkHdAPkaaTUCV+kwxGcIhr7ldIFAE86f2MWiQ7rDFf49avfV+RmQ==} cpu: [x64] os: [darwin] - '@dev.fast/diffr-linux-arm64@0.1.11': - resolution: {integrity: sha512-+K8nTmVkc2wVY4lKy6UoH+Sv7rIqbh43w5YqVyiXThHYPd6qY1/jqbtcNhAELeUfPb4S7u1PCAQDK0/u+Pluow==} + '@dev.fast/diffr-linux-arm64@0.1.12': + resolution: {integrity: sha512-3aNF+7mygPJAPmrWiUP6V8oJ6VBLG9f6Dyk/79Q6+y7e8qh4K1LyCZi2tspIGnQ1ZKw19Q7OUZPBFKv6MOYyyQ==} cpu: [arm64] os: [linux] libc: [glibc] - '@dev.fast/diffr-linux-x64@0.1.11': - resolution: {integrity: sha512-gZrI3C8BubT/q/SnMF85SjQUJU5dXLiLkNZsYy/q4xilOT+muv7i7eRw808771bQ8lgWanBfDNLx0cq3XY/5BA==} + '@dev.fast/diffr-linux-x64@0.1.12': + resolution: {integrity: sha512-J/jcMBLXLTUvITYoXf3IZqjmknUKJp0aDk0xh5txsYXp5IdPhzX8uSbViXuIE/oMpFx6u0ElayBoilEaioFSmg==} cpu: [x64] os: [linux] libc: [glibc] - '@dev.fast/diffr-win32-x64@0.1.11': - resolution: {integrity: sha512-WHE1daqKnF7Uo/NqvevSEB7IiWWIsFHvYehujwaMWM+5GpJNtz/rI8o9aWGrf1mDcICQwTAaAw3QhZHDY31faw==} + '@dev.fast/diffr-win32-x64@0.1.12': + resolution: {integrity: sha512-kzhjzEouKwg44UCfWZ1cD2bCvyhKc3uPB8UdgvGJiNburQWBiSnuJR010zIYUk1lVO2T4ruoRKSChF+ha4/igQ==} cpu: [x64] os: [win32] - '@dev.fast/diffr@0.1.11': - resolution: {integrity: sha512-bG7zvvv5MaWIisUarPuOrTb8KI87HGemhci8sMdsVMJ5bCFN5Ml6D57iOMqzwsGMwZqbWJ8ViHVZzb9f6RAA1g==} + '@dev.fast/diffr@0.1.12': + resolution: {integrity: sha512-k5eeAhx1roIXS28j0pxMsEwVNWlm0OUAKfEFNeFp96DazYJ0yCM2MY8/1M4s6mkiyzNtEpRV/aYl9qmuLi8M8Q==} engines: {node: '>=20'} peerDependencies: zod: ^4.0.0 @@ -2047,6 +2056,10 @@ packages: resolution: {integrity: sha512-/veY75JbMK4j1yjvuUxuVsiS/hr/4iHs9FTT6cgTexxdE0Ly/glccBAkloH/DofkjRbZU3bnoj38mOmhkZ0lHw==} engines: {node: '>=12'} + eslint-visitor-keys@5.0.1: + resolution: {integrity: sha512-tD40eHxA35h0PEIZNeIjkHoDR4YjjJp34biM0mDvplBe//mB+IHCqHDGV7pxF+7MklTvighcCPPZC7ynWyjdTA==} + engines: {node: ^20.19.0 || ^22.13.0 || >=24} + estree-walker@3.0.3: resolution: {integrity: sha512-7RUKfXgSMMkzt6ZuXmqapOurLGPPfgj6l9uRZ7lRGolvk0y2yocc35LdcxKC5PQZdn2DMqioAQ2NoWcrTKmm6g==} @@ -2920,6 +2933,10 @@ packages: resolution: {integrity: sha512-o5sSPKEkg/DIQNmH43V0/uerLrpzVedkUh8tGNvaeXpfpuwjKenlSox/2O/BTlZUtEe+JG7s5YhEz608PlAHRA==} engines: {node: '>=0.6'} + toml-eslint-parser@1.0.3: + resolution: {integrity: sha512-A5F0cM6+mDleacLIEUkmfpkBbnHJFV1d2rprHU2MXNk7mlxHq2zGojA+SRvQD1RoMo9gqjZPWEaKG4v1BQ48lw==} + engines: {node: ^20.19.0 || ^22.13.0 || >=24} + totalist@3.0.1: resolution: {integrity: sha512-sf4i37nQ2LBx4m3wB74y+ubopq6W/dIzXg0FDGjsYnZHVa1Da8FH853wlL2gtUhg+xJXjfk3kUZS3BRoQeoQBQ==} engines: {node: '>=6'} @@ -3452,30 +3469,30 @@ snapshots: '@csstools/css-tokenizer@4.0.0': {} - '@dev.fast/diffr-darwin-arm64@0.1.11': + '@dev.fast/diffr-darwin-arm64@0.1.12': optional: true - '@dev.fast/diffr-darwin-x64@0.1.11': + '@dev.fast/diffr-darwin-x64@0.1.12': optional: true - '@dev.fast/diffr-linux-arm64@0.1.11': + '@dev.fast/diffr-linux-arm64@0.1.12': optional: true - '@dev.fast/diffr-linux-x64@0.1.11': + '@dev.fast/diffr-linux-x64@0.1.12': optional: true - '@dev.fast/diffr-win32-x64@0.1.11': + '@dev.fast/diffr-win32-x64@0.1.12': optional: true - '@dev.fast/diffr@0.1.11(zod@4.6.5)': + '@dev.fast/diffr@0.1.12(zod@4.6.5)': dependencies: zod: 4.6.5 optionalDependencies: - '@dev.fast/diffr-darwin-arm64': 0.1.11 - '@dev.fast/diffr-darwin-x64': 0.1.11 - '@dev.fast/diffr-linux-arm64': 0.1.11 - '@dev.fast/diffr-linux-x64': 0.1.11 - '@dev.fast/diffr-win32-x64': 0.1.11 + '@dev.fast/diffr-darwin-arm64': 0.1.12 + '@dev.fast/diffr-darwin-x64': 0.1.12 + '@dev.fast/diffr-linux-arm64': 0.1.12 + '@dev.fast/diffr-linux-x64': 0.1.12 + '@dev.fast/diffr-win32-x64': 0.1.12 '@dual-bundle/import-meta-resolve@4.2.1': {} @@ -4525,6 +4542,8 @@ snapshots: escape-string-regexp@5.0.0: {} + eslint-visitor-keys@5.0.1: {} + estree-walker@3.0.3: dependencies: '@types/estree': 1.0.8 @@ -5619,6 +5638,10 @@ snapshots: toidentifier@1.0.1: {} + toml-eslint-parser@1.0.3: + dependencies: + eslint-visitor-keys: 5.0.1 + totalist@3.0.1: {} tough-cookie@6.0.2: diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index f17b8d77f..30136bfb1 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -5,6 +5,8 @@ packages: - "apps/*" catalog: + '@dev.fast/diffr': 0.1.12 + toml-eslint-parser: ^1.0.3 zod: "4.6.5" catalogMode: strict @@ -17,12 +19,12 @@ verifyDepsBeforeRun: false minimumReleaseAge: 10080 minimumReleaseAgeStrict: true minimumReleaseAgeExclude: - - "@dev.fast/diffr@0.1.11" - - "@dev.fast/diffr-darwin-arm64@0.1.11" - - "@dev.fast/diffr-darwin-x64@0.1.11" - - "@dev.fast/diffr-linux-arm64@0.1.11" - - "@dev.fast/diffr-linux-x64@0.1.11" - - "@dev.fast/diffr-win32-x64@0.1.11" + - "@dev.fast/diffr@0.1.12" + - "@dev.fast/diffr-darwin-arm64@0.1.12" + - "@dev.fast/diffr-darwin-x64@0.1.12" + - "@dev.fast/diffr-linux-arm64@0.1.12" + - "@dev.fast/diffr-linux-x64@0.1.12" + - "@dev.fast/diffr-win32-x64@0.1.12" # Ask tracks the latest ACP SDK and adapters, and the CLI packages they pin. - "@agentclientprotocol/sdk@1.5.1" - "@agentclientprotocol/claude-agent-acp@0.84.0" From 6d082a93e7384d108387e9d3c12b40c71695f2a9 Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Tue, 6 Oct 2026 17:51:46 -0700 Subject: [PATCH 5/7] Nest diffr v2 settings under plugins.shape and plugins.classify Update startup migration, native config writes, schema defaults, and settings controls together. Preserve the new plugins groups while removing only the legacy bundled/external tables. Validated with the local diffr 0.1.13 candidate: 12 runtime integration tests, 68 server tests, and 16 browser settings tests pass. Server/app typechecks, lint, and formatting pass. AI assistance: implemented and validated with OpenAI Codex. Agent-Session: 01a10ff2-f5c2-7ec1-852f-8ebe516d8af9 --- .../scripts/e2e/journeys/diffr-settings.mjs | 4 +- .../src/diffr-config-section.browser.test.tsx | 118 ++++++++------ .../review/app/src/diffr-config-section.tsx | 35 ++-- .../scripts/bundled-diffr.integration.mjs | 22 +-- .../src/server/diffr-config-migration.test.ts | 150 +++++++++++++----- .../src/server/diffr-config-migration.ts | 33 ++-- .../review/src/server/diffr-config.test.ts | 115 ++++++++------ packages/review/src/server/diffr-config.ts | 18 ++- .../review/src/server/diffr-startup.test.ts | 6 +- 9 files changed, 316 insertions(+), 185 deletions(-) diff --git a/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs b/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs index a2cae9c75..9559f3d95 100644 --- a/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs @@ -117,8 +117,8 @@ export async function run(ctx) { await toggle.click(); await ctx.until( async () => - (await ctx.apiOk("/diffr-config")).values.shape["test-bodies"].enabled === - false, + (await ctx.apiOk("/diffr-config")).values.plugins.shape["test-bodies"] + .enabled === false, "diffr persisted the setting", ); await settings diff --git a/packages/review/app/src/diffr-config-section.browser.test.tsx b/packages/review/app/src/diffr-config-section.browser.test.tsx index 905f94b22..b993164e4 100644 --- a/packages/review/app/src/diffr-config-section.browser.test.tsx +++ b/packages/review/app/src/diffr-config-section.browser.test.tsx @@ -47,25 +47,31 @@ function config(): ReviewDiffrConfig { defaultHiddenTags: ["generated", "vendored", "test"], values: { version: 2, - classify: { classify: { hide: ["test"], hide_deleted: true } }, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - shape: { - context: { enabled: true, lines: 3 }, - "test-bodies": { enabled: true }, - "deleted-bodies": { enabled: true }, - "removed-runs": { enabled: true }, - summarize: { - enabled: false, - provider: "gemini", - model: "test-model", - tests: true, - system_prompt: "Default prompt.", + plugins: { + classify: { classify: { hide: ["test"], hide_deleted: true } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + context: { enabled: true, lines: 3 }, + "test-bodies": { enabled: true }, + "deleted-bodies": { enabled: true }, + "removed-runs": { enabled: true }, + summarize: { + enabled: false, + provider: "gemini", + model: "test-model", + tests: true, + system_prompt: "Default prompt.", + }, }, }, }, }; } +function pluginSettings(config: ReviewDiffrConfig): JsonObject { + return config.values.plugins as JsonObject; +} + async function mount(overrides: Partial = {}) { const current = { ...config(), ...overrides }; @@ -82,16 +88,18 @@ async function mount(overrides: Partial = {}) { defaultHiddenTags: ["generated", "vendored", "test"], values: { version: 2, - classify: { classify: { hide: ["test"], hide_deleted: true } }, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - shape: { - summarize: { - enabled: input.enabled, - provider: input.provider, - model: input.model, - endpoint: input.endpoint, - system_prompt: input.systemPrompt, - tests: input.tests, + plugins: { + classify: { classify: { hide: ["test"], hide_deleted: true } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + summarize: { + enabled: input.enabled, + provider: input.provider, + model: input.model, + endpoint: input.endpoint, + system_prompt: input.systemPrompt, + tests: input.tests, + }, }, }, }, @@ -126,12 +134,12 @@ async function open() { test("file-hiding controls use classify fields and retain custom tags across off and on", async () => { const { actions, current } = await mount(); - current.values.classify = { + pluginSettings(current).classify = { classify: { hide: ["custom"], hide_deleted: true }, }; vi.mocked(actions.set).mockImplementation(async (key, value) => { const field = key.split(".").at(-1)!; - const classify = current.values.classify as JsonObject; + const classify = pluginSettings(current).classify as JsonObject; classify.classify = { ...(classify.classify as JsonObject), [field]: value, @@ -141,39 +149,46 @@ test("file-hiding controls use classify fields and retain custom tags across off }); await open(); await act(async () => page.getByLabelText("Hide files by tag").click()); - expect(current.values.classify).toMatchObject({ classify: { hide: [] } }); + expect(pluginSettings(current).classify).toMatchObject({ + classify: { hide: [] }, + }); await act(async () => page.getByLabelText("Hide files by tag").click()); - expect(current.values.classify).toMatchObject({ + expect(pluginSettings(current).classify).toMatchObject({ classify: { hide: ["custom"] }, }); await act(async () => page.getByLabelText("Hide deleted files").click()); - expect(current.values.classify).toMatchObject({ + expect(pluginSettings(current).classify).toMatchObject({ classify: { hide_deleted: false }, }); - expect(actions.set).toHaveBeenCalledWith("classify.classify.hide", []); - expect(actions.set).toHaveBeenCalledWith("classify.classify.hide", [ + expect(actions.set).toHaveBeenCalledWith( + "plugins.classify.classify.hide", + [], + ); + expect(actions.set).toHaveBeenCalledWith("plugins.classify.classify.hide", [ "custom", ]); expect(actions.set).toHaveBeenCalledWith( - "classify.classify.hide_deleted", + "plugins.classify.classify.hide_deleted", false, ); }); test("enabling tag hiding from an empty list uses the schema defaults", async () => { const { actions, current } = await mount(); - current.values.classify = { classify: { hide: [], hide_deleted: false } }; + pluginSettings(current).classify = { + classify: { hide: [], hide_deleted: false }, + }; await open(); await act(async () => page.getByLabelText("Hide files by tag").click()); expect(actions.set).toHaveBeenCalledWith( - "classify.classify.hide", + "plugins.classify.classify.hide", current.defaultHiddenTags, ); }); test("custom classifier controls write its own entry and keep its tags", async () => { const { actions, current } = await mount(); - current.values.classify = { + pluginSettings(current).classify = { "team-classifier": { path: "/plugins/team-classifier", hide: ["team-generated"], @@ -181,8 +196,8 @@ test("custom classifier controls write its own entry and keep its tags", async ( }, }; vi.mocked(actions.set).mockImplementation(async (key, value) => { - const [, name, field] = key.split("."); - const classifiers = current.values.classify as JsonObject; + const [, , name, field] = key.split("."); + const classifiers = pluginSettings(current).classify as JsonObject; const options = classifiers[name] as JsonObject; classifiers[name] = { ...options, [field]: value }; @@ -192,20 +207,20 @@ test("custom classifier controls write its own entry and keep its tags", async ( await open(); await act(async () => page.getByLabelText("Hide files by tag").click()); expect(actions.set).toHaveBeenLastCalledWith( - "classify.team-classifier.hide", + "plugins.classify.team-classifier.hide", [], ); await act(async () => page.getByLabelText("Hide files by tag").click()); expect(actions.set).toHaveBeenLastCalledWith( - "classify.team-classifier.hide", + "plugins.classify.team-classifier.hide", ["team-generated"], ); await act(async () => page.getByLabelText("Hide deleted files").click()); expect(actions.set).toHaveBeenLastCalledWith( - "classify.team-classifier.hide_deleted", + "plugins.classify.team-classifier.hide_deleted", false, ); - expect(current.values.classify).toEqual({ + expect(pluginSettings(current).classify).toEqual({ "team-classifier": { path: "/plugins/team-classifier", hide: ["team-generated"], @@ -216,7 +231,7 @@ test("custom classifier controls write its own entry and keep its tags", async ( test("custom classifiers only show the hide fields that they support", async () => { const { actions, current } = await mount(); - current.values.classify = { + pluginSettings(current).classify = { "team-classifier": { path: "/plugins/team-classifier", hide_deleted: true }, }; await open(); @@ -225,7 +240,7 @@ test("custom classifiers only show the hide fields that they support", async () ).toBeNull(); await act(async () => page.getByLabelText("Hide deleted files").click()); expect(actions.set).toHaveBeenCalledWith( - "classify.team-classifier.hide_deleted", + "plugins.classify.team-classifier.hide_deleted", false, ); }); @@ -251,7 +266,10 @@ test("writes the selected key and keeps reload visible when collapsed", async () await act(async () => { await page.getByLabelText("Collapse test bodies").click(); }); - expect(actions.set).toHaveBeenCalledWith("shape.test-bodies.enabled", false); + expect(actions.set).toHaveBeenCalledWith( + "plugins.shape.test-bodies.enabled", + false, + ); await expect .element(page.getByRole("button", { name: "Reload window", exact: true })) .toBeVisible(); @@ -285,7 +303,7 @@ test("rejects invalid context lines without writing", async () => { test("tests draft settings without saving, then saves the custom prompt and clears the key", async () => { const saved = config(); - const foldPlugins = saved.values.shape as JsonObject; + const foldPlugins = pluginSettings(saved).shape as JsonObject; (foldPlugins.summarize as JsonObject).system_prompt = "Keep my wording."; const { actions } = await mount(saved); await open(); @@ -503,8 +521,16 @@ test("switching provider clears a custom endpoint, and a new endpoint warns abou system_prompt: "Default prompt.", }; - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - await mount({ values: { ...values, shape: { summarize } } }); + await mount({ + values: { + ...values, + plugins: { + ...(values.plugins as JsonObject), + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { summarize }, + }, + }, + }); await open(); const endpoint = page.getByLabelText("Endpoint URL", { exact: true }); await expect.element(endpoint).toHaveValue("https://openrouter.ai/api/v1"); diff --git a/packages/review/app/src/diffr-config-section.tsx b/packages/review/app/src/diffr-config-section.tsx index d93769dfd..38b5de723 100644 --- a/packages/review/app/src/diffr-config-section.tsx +++ b/packages/review/app/src/diffr-config-section.tsx @@ -15,21 +15,21 @@ import { Choice } from "./settings-choice"; import { settingsStyles as styles } from "./settings-styles"; const foldSettings = [ - ["shape.context.enabled", "Collapse unchanged lines"], - ["shape.test-bodies.enabled", "Collapse test bodies"], - ["shape.deleted-bodies.enabled", "Collapse deleted function bodies"], - ["shape.removed-runs.enabled", "Collapse long removed stretches"], + ["plugins.shape.context.enabled", "Collapse unchanged lines"], + ["plugins.shape.test-bodies.enabled", "Collapse test bodies"], + ["plugins.shape.deleted-bodies.enabled", "Collapse deleted function bodies"], + ["plugins.shape.removed-runs.enabled", "Collapse long removed stretches"], ] as const; type DisplaySetting = readonly [key: string, label: string]; function classifierPrefix(config: ReviewDiffrConfig): string | undefined { - const entries = config.values.classify; + const entries = setting(config, "plugins.classify"); if (!isJsonObject(entries)) return; const names = Object.keys(entries); - if (names.length === 1) return `classify.${names[0]}`; + if (names.length === 1) return `plugins.classify.${names[0]}`; } function displaySettings(config: ReviewDiffrConfig): DisplaySetting[] { @@ -60,7 +60,7 @@ function setting( } function provider(config: ReviewDiffrConfig): string { - return String(setting(config, "shape.summarize.provider") ?? ""); + return String(setting(config, "plugins.shape.summarize.provider") ?? ""); } /** The provider diffr describes as `id`, if any. */ @@ -74,16 +74,16 @@ function title(config: ReviewDiffrConfig, id: string): string { function summaryDraft(config: ReviewDiffrConfig): ReviewDiffrSummarizerInput { return { - enabled: setting(config, "shape.summarize.enabled") === true, + enabled: setting(config, "plugins.shape.summarize.enabled") === true, provider: provider(config), - model: String(setting(config, "shape.summarize.model") ?? ""), - endpoint: String(setting(config, "shape.summarize.endpoint") ?? ""), + model: String(setting(config, "plugins.shape.summarize.model") ?? ""), + endpoint: String(setting(config, "plugins.shape.summarize.endpoint") ?? ""), systemPrompt: String( - setting(config, "shape.summarize.system_prompt") ?? + setting(config, "plugins.shape.summarize.system_prompt") ?? config.defaultPrompt ?? "", ), - tests: setting(config, "shape.summarize.tests") === true, + tests: setting(config, "plugins.shape.summarize.tests") === true, apiKey: "", }; } @@ -171,7 +171,7 @@ export function DiffrConfigSection({ JSON.stringify(draft) !== JSON.stringify(summaryDraft(config)); const unavailable = - !config || setting(config, "shape.summarize.enabled") === undefined; + !config || setting(config, "plugins.shape.summarize.enabled") === undefined; const summaryValid = !!draft?.model.trim(); const savedDraft = config && summaryDraft(config); @@ -231,15 +231,18 @@ export function DiffrConfigSection({ Not available in this configuration.

)} - {key === "shape.context.enabled" && ( + {key === "plugins.shape.context.enabled" && ( void run(async () => saved( - await actions.set("shape.context.lines", value), + await actions.set( + "plugins.shape.context.lines", + value, + ), ), ) } diff --git a/packages/review/scripts/bundled-diffr.integration.mjs b/packages/review/scripts/bundled-diffr.integration.mjs index 2dc8e5b6a..2a5627cb6 100644 --- a/packages/review/scripts/bundled-diffr.integration.mjs +++ b/packages/review/scripts/bundled-diffr.integration.mjs @@ -256,14 +256,14 @@ describe("Relocated runtime diffr integrates with Review streams and settings", assert.equal(await ensureDiffrConfigMigrated(diffrExecutable()), true); const config = await readDiffrConfig(repository); assert.equal(config.values.version, 2); - assert.deepEqual(config.values.classify.classify.hide, [ + assert.deepEqual(config.values.plugins.classify.classify.hide, [ "test", "generated", ]); - assert.equal(config.values.classify.classify.hide_deleted, false); - assert.equal(config.values.shape.context.lines, 17); + assert.equal(config.values.plugins.classify.classify.hide_deleted, false); + assert.equal(config.values.plugins.shape.context.lines, 17); assert.equal( - config.values.shape.summarize.system_prompt, + config.values.plugins.shape.summarize.system_prompt, " Use my exact custom instructions.\nKeep # inside the prompt, and answer with JSON if I ask for it.\n", ); @@ -295,23 +295,23 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("settings values and edits round-trip through the staged binary", async () => { const config = await readDiffrConfig(repository); - assert.ok(Number.isInteger(config.values.shape.context.lines)); - assert.equal(config.values.shape.summarize.api_key, undefined); + assert.ok(Number.isInteger(config.values.plugins.shape.context.lines)); + assert.equal(config.values.plugins.shape.summarize.api_key, undefined); assert.ok( ["config", "environment", "missing"].includes(config.credentialSource), ); const updated = await setDiffrConfigValue( - "shape.context.lines", + "plugins.shape.context.lines", 7, repository, ); - assert.equal(updated.values.shape.context.lines, 7); + assert.equal(updated.values.plugins.shape.context.lines, 7); assert.equal(updated.changed, true); assert.equal(updated.error, undefined); assert.equal( - (await readDiffrConfig(repository)).values.shape.context.lines, + (await readDiffrConfig(repository)).values.plugins.shape.context.lines, 7, ); assert.equal(existsSync(sentinel), false); @@ -320,7 +320,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("a provider switch saves through the binary, clears the old key and keeps the file sparse", async () => { delete process.env.ANTHROPIC_API_KEY; await setDiffrConfigValue( - "shape.summarize.api_key", + "plugins.shape.summarize.api_key", "old-secret", repository, ); @@ -341,7 +341,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", ); assert.equal(saved.error, undefined); - assert.equal(saved.values.shape.summarize.provider, "anthropic"); + assert.equal(saved.values.plugins.shape.summarize.provider, "anthropic"); assert.equal(saved.credentialSource, "missing"); const file = await readFile( diff --git a/packages/review/src/server/diffr-config-migration.test.ts b/packages/review/src/server/diffr-config-migration.test.ts index a9c228410..0ea1aabe8 100644 --- a/packages/review/src/server/diffr-config-migration.test.ts +++ b/packages/review/src/server/diffr-config-migration.test.ts @@ -69,11 +69,13 @@ test("converts a saved UI config without changing supported overrides or their c delete plugins.bundled.group; delete plugins.bundled["hide-files"]; delete plugins.bundled.summarize.max_concurrency; - delete expected.plugins; expected.version = 2; - expected.shape = plugins.bundled; - expected.classify = { - classify: { hide: ["test", "generated"], hide_deleted: false }, + expected.plugins = { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: plugins.bundled, + classify: { + classify: { hide: ["test", "generated"], hide_deleted: false }, + }, }; expect(parse(migrated)).toEqual(expected); @@ -105,10 +107,12 @@ test.each([ `[plugins.bundled.hide-files]\nenabled = ${enabled}\ndeleted = ${deleted}\ntags = ["test", "generated"]\n`, ); - expect(parse(migrated).classify).toEqual({ + expect(parse(migrated).plugins).toEqual({ classify: { - hide, - hide_deleted: hideDeleted, + classify: { + hide, + hide_deleted: hideDeleted, + }, }, }); }, @@ -123,8 +127,10 @@ test.each([true, false])( expect(parse(migrated)).toEqual({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { context: { enabled: false, lines: 9 } }, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { context: { enabled: false, lines: 9 } }, + }, }); }, ); @@ -133,18 +139,22 @@ test("handles quoted keys, inline tables and dotted keys without touching a cust const source = `version = 1\nplugins = { bundled = { group = { enabled = false }, "hide-files" = { enabled = false }, summarize = { system_prompt = "Keep me", max_concurrency = 2 } }, order = ["bundled.group", "bundled.hide-files", "bundled.summarize"] }\n`; expect(parse(migrateDiffrConfig(source))).toEqual({ version: 2, - classify: { classify: { hide: [], hide_deleted: false } }, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { - summarize: { system_prompt: "Keep me" }, - order: ["summarize"], + plugins: { + classify: { classify: { hide: [], hide_deleted: false } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + summarize: { system_prompt: "Keep me" }, + order: ["summarize"], + }, }, }); const dotted = `plugins.bundled.group.enabled = true\nplugins.bundled."hide-files".deleted = false\nclassifier = { hide = ["vendored"] }\n`; - expect(parse(migrateDiffrConfig(dotted)).classify).toEqual({ + expect(parse(migrateDiffrConfig(dotted)).plugins).toEqual({ classify: { - hide: ["vendored"], - hide_deleted: false, + classify: { + hide: ["vendored"], + hide_deleted: false, + }, }, }); }); @@ -153,7 +163,9 @@ test("retains explicit modern classifier values in a mixed config", () => { const source = `[classifier]\nhide = ["custom"]\nhide_deleted = true\n[plugins.bundled.hide-files]\nenabled = false\n`; expect(parse(migrateDiffrConfig(source))).toEqual({ version: 2, - classify: { classify: { hide: ["custom"], hide_deleted: true } }, + plugins: { + classify: { classify: { hide: ["custom"], hide_deleted: true } }, + }, }); }); @@ -161,9 +173,11 @@ test("removes retired names from a custom order without sorting surviving plugin const source = `[plugins]\norder = ["bundled.context", "bundled.group", "external.custom", "bundled.hide-files", "bundled.summarize"]\n`; expect(parse(migrateDiffrConfig(source))).toEqual({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { - order: ["context", "custom", "summarize"], + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + order: ["context", "custom", "summarize"], + }, }, }); }); @@ -173,8 +187,10 @@ test("retains comments inside a rewritten plugin order", () => { const migrated = migrateDiffrConfig(source); expect(parse(migrated)).toEqual({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { order: ["context", "summarize"] }, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { order: ["context", "summarize"] }, + }, }); expect(migrated.match(/# keep context first/g)).toHaveLength(1); expect(migrated.match(/# retired group/g)).toHaveLength(1); @@ -247,7 +263,10 @@ test.skipIf(process.platform === "win32")( test("does not run the binary or change a version 2 file", async () => { const { file, env } = await fixture(); - const source = "version = 2 # already migrated\n[shape.context]\nlines = 8\n"; + + const source = + "version = 2 # already migrated\n[plugins.shape.context]\nlines = 8\n"; + await writeFile(file, source); expect(await ensureDiffrConfigMigrated("/no/binary/needed", env)).toBe(false); expect(await readFile(file, "utf8")).toBe(source); @@ -338,12 +357,14 @@ byte_limit = 1_000_000 const migrated = migrateDiffrConfig(source); expect(parse(migrated)).toEqual({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { - context: { lines: 16, extra: { option: "custom", nested: [1, 2] } }, - custom: { path: "plugins/custom", threshold: 1000 }, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + context: { lines: 16, extra: { option: "custom", nested: [1, 2] } }, + custom: { path: "plugins/custom", threshold: 1000 }, + }, + classify: { classify: { hide: ["custom"] } }, }, - classify: { classify: { hide: ["custom"] } }, diff: { byte_limit: 1000000 }, }); @@ -386,13 +407,17 @@ test("moves a custom classifier under its manifest name and validates its origin }); expect(parse(await readFile(file, "utf8"))).toEqual({ version: 2, - classify: { - "team.classifier": { path: "custom-classifier", hide: ["team"] }, + plugins: { + classify: { + "team.classifier": { path: "custom-classifier", hide: ["team"] }, + }, }, }); expect(parse(await readFile(captured, "utf8"))).toEqual({ version: 2, - classify: { "team.classifier": { path: directory, hide: ["team"] } }, + plugins: { + classify: { "team.classifier": { path: directory, hide: ["team"] } }, + }, }); }); @@ -435,8 +460,10 @@ test("serializes concurrent startup migrations and writes one backup", async () expect(results.sort()).toEqual([false, true]); expect(parse(await readFile(file, "utf8"))).toEqual({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { context: { lines: 11 } }, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { context: { lines: 11 } }, + }, }); const backups = (await readdir(path.dirname(file))).filter((name) => @@ -471,8 +498,10 @@ test.each([ const migrated = migrateDiffrConfig(source); expect(parse(migrated)).toEqual({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { "team.custom": { path: "plugins/custom", "option.name": 16 } }, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { "team.custom": { path: "plugins/custom", "option.name": 16 } }, + }, }); expect(migrated).toContain("0x10"); }); @@ -493,8 +522,10 @@ test.each([ const migrated = migrateDiffrConfig(source); expect(parse(migrated)).toEqual({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { summarize: {} }, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { summarize: {} }, + }, }); expect(migrated).toContain("# old default"); @@ -509,7 +540,9 @@ test.each([ ); expect( - new TomlEdits(migratedCustom).value("shape.summarize.system_prompt"), + new TomlEdits(migratedCustom).value( + "plugins.shape.summarize.system_prompt", + ), ).toBe(custom); expect(migratedCustom).toContain( `system_prompt = ${literal} # custom prompt`, @@ -522,7 +555,7 @@ test("moves the old default context-first order to the v2 context-last default", '[plugins]\norder = ["bundled.context", "bundled.hide-files", "bundled.deleted-bodies", "bundled.summarize", "bundled.test-bodies", "bundled.removed-runs", "bundled.group"]\n'; expect( - new TomlEdits(migrateDiffrConfig(source)).value("shape.order"), + new TomlEdits(migrateDiffrConfig(source)).value("plugins.shape.order"), ).toEqual([ "deleted-bodies", "summarize", @@ -531,3 +564,40 @@ test("moves the old default context-first order to the v2 context-last default", "context", ]); }); + +test("keeps new namespaces while removing only legacy parent tables", () => { + const source = `version = 1 +[plugins] # pipeline config +order = ["bundled.context", "external.team.custom"] +[plugins.bundled] # built-in plugins +context = { lines = 0x10 } +[plugins.external] # custom plugins +"team.custom" = { path = 'plugins/custom', threshold = 1_000 } +[classifier] # file tags +hide = ['team'] +`; + + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + order: ["context", "team.custom"], + context: { lines: 16 }, + "team.custom": { path: "plugins/custom", threshold: 1000 }, + }, + classify: { classify: { hide: ["team"] } }, + }, + }); + expect(migrated).toContain("lines = 0x10"); + expect(migrated).toContain("threshold = 1_000"); + + for (const comment of [ + "pipeline config", + "built-in plugins", + "custom plugins", + "file tags", + ]) + expect(migrated).toContain(`# ${comment}`); +}); diff --git a/packages/review/src/server/diffr-config-migration.ts b/packages/review/src/server/diffr-config-migration.ts index 37322f566..090872026 100644 --- a/packages/review/src/server/diffr-config-migration.ts +++ b/packages/review/src/server/diffr-config-migration.ts @@ -92,7 +92,7 @@ export function migrateDiffrConfig( if (bundled?.order !== undefined || external?.order !== undefined) throw new ConfigMigrationError( - "diffr config has a plugin named order. Rename that plugin before migration; shape.order is reserved.", + "diffr config has a plugin named order. Rename that plugin before migration; plugins.shape.order is reserved.", ); for (const name of Object.keys(external ?? {})) @@ -134,7 +134,7 @@ export function migrateDiffrConfig( .map((name) => name.replace(/^(bundled|external)\./, "")); edits.set("plugins.order", migrated); - edits.move("plugins.order", "shape.order"); + edits.move("plugins.order", "plugins.shape.order"); } // This old option was accepted but ignored. Do not turn it into --jobs. @@ -154,13 +154,20 @@ export function migrateDiffrConfig( for (const name of Object.keys(bundled ?? {})) if (name !== "group" && name !== "hide-files") - edits.move(tomlKey("plugins", "bundled", name), tomlKey("shape", name)); + edits.move( + tomlKey("plugins", "bundled", name), + tomlKey("plugins", "shape", name), + ); for (const name of Object.keys(external ?? {})) - edits.move(tomlKey("plugins", "external", name), tomlKey("shape", name)); + edits.move( + tomlKey("plugins", "external", name), + tomlKey("plugins", "shape", name), + ); - edits.remove("plugins"); - edits.move("classifier", tomlKey("classify", classifierName)); + edits.remove("plugins.bundled"); + edits.remove("plugins.external"); + edits.move("classifier", tomlKey("plugins", "classify", classifierName)); edits.set("version", 2); const movedValues: [string, string][] = []; @@ -177,13 +184,16 @@ export function migrateDiffrConfig( if (/^plugins\.(bundled|external)\./.test(key)) movedValues.push([ key, - key.replace(/^plugins\.(bundled|external)\./, "shape."), + key.replace(/^plugins\.(bundled|external)\./, "plugins.shape."), ]); if (key.startsWith("classifier.")) movedValues.push([ key, - key.replace("classifier.", `${tomlKey("classify", classifierName)}.`), + key.replace( + "classifier.", + `${tomlKey("plugins", "classify", classifierName)}.`, + ), ]); } @@ -345,9 +355,10 @@ async function validateDiffrConfig( if ( key === "theme.path" || - (parts.length === 3 && - (parts[0] === "shape" || parts[0] === "classify") && - parts[2] === "path") + (parts.length === 4 && + parts[0] === "plugins" && + (parts[1] === "shape" || parts[1] === "classify") && + parts[3] === "path") ) { const value = z.string().parse(edits.value(key)); diff --git a/packages/review/src/server/diffr-config.test.ts b/packages/review/src/server/diffr-config.test.ts index ebca7b2e2..b8ec20b12 100644 --- a/packages/review/src/server/diffr-config.test.ts +++ b/packages/review/src/server/diffr-config.test.ts @@ -130,19 +130,21 @@ async function fakeDiffr( state, JSON.stringify({ version: 2, - ...(classifier && { classify: { classify: classifier } }), - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - shape: { - summarize: { - enabled: false, - provider: "gemini", - model: "old", - tests: false, - system_prompt: DEFAULT_PROMPT, - ...(key && { api_key: key }), - ...summarize, + plugins: { + ...(classifier && { classify: { classify: classifier } }), + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + summarize: { + enabled: false, + provider: "gemini", + model: "old", + tests: false, + system_prompt: DEFAULT_PROMPT, + ...(key && { api_key: key }), + ...summarize, + }, + context: { lines: 3, enabled: true }, }, - context: { lines: 3, enabled: true }, }, }), ); @@ -159,9 +161,9 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { } else if (args[0] === 'config' && args[1] === 'schema') { const summarize = ${JSON.stringify(SUMMARIZE_SCHEMA)}; // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - console.log(JSON.stringify({ properties: { ${classifier ? 'classify: { properties: { classify: { properties: { hide: { default: ["generated", "vendored"] } } } } },' : ""} shape: { properties: { summarize } } } })); + console.log(JSON.stringify({ properties: { plugins: { properties: { ${classifier ? 'classify: { properties: { classify: { properties: { hide: { default: ["generated", "vendored"] } } } } },' : ""} shape: { properties: { summarize } } } } } })); } else if (args[0] === 'config' && args[1] === 'show') { - if (!args.includes('--reveal') && config.shape.summarize.api_key) config.shape.summarize.api_key = ''; + if (!args.includes('--reveal') && config.plugins.shape.summarize.api_key) config.plugins.shape.summarize.api_key = ''; console.log(JSON.stringify(config)); } else if (args[0] === 'config' && args[1] === 'set') { if (args[2] === process.env.FAIL_KEY) { console.error('command leaked secret: ' + args.join(' ')); process.exit(2); } @@ -234,7 +236,9 @@ test("reads classifier values and schema defaults without old plugin keys", asyn ); const config = await readDiffrConfig(); expect(config.values).toMatchObject({ - classify: { classify: { hide: ["test", "custom"], hide_deleted: false } }, + plugins: { + classify: { classify: { hide: ["test", "custom"], hide_deleted: false } }, + }, }); expect(config.defaultHiddenTags).toEqual(["generated", "vendored"]); expect(JSON.stringify(config)).not.toContain("saved-secret"); @@ -248,47 +252,58 @@ test("writes classifier values without changing credentials", async () => { ); expect( - (await setDiffrConfigValue("classify.classify.hide", ["custom"])).changed, + (await setDiffrConfigValue("plugins.classify.classify.hide", ["custom"])) + .changed, ).toBe(false); - await setDiffrConfigValue("classify.classify.hide", []); - await setDiffrConfigValue("classify.classify.hide_deleted", false); + await setDiffrConfigValue("plugins.classify.classify.hide", []); + await setDiffrConfigValue("plugins.classify.classify.hide_deleted", false); const disabled = JSON.parse(await readFile(fake.state, "utf8")); - expect(disabled.classify.classify).toEqual({ hide: [], hide_deleted: false }); - await setDiffrConfigValue("classify.classify.hide", ["custom"]); + expect(disabled.plugins.classify.classify).toEqual({ + hide: [], + hide_deleted: false, + }); + await setDiffrConfigValue("plugins.classify.classify.hide", ["custom"]); const enabled = JSON.parse(await readFile(fake.state, "utf8")); - expect(enabled.classify.classify).toEqual({ + expect(enabled.plugins.classify.classify).toEqual({ hide: ["custom"], hide_deleted: false, }); - expect(enabled.shape.summarize.api_key).toBe("saved-secret"); - expect(enabled.shape["hide-files"]).toBeUndefined(); + expect(enabled.plugins.shape.summarize.api_key).toBe("saved-secret"); + expect(enabled.plugins.shape["hide-files"]).toBeUndefined(); }); test("writes the native path of a custom classifier whose name contains spaces", async () => { const fake = await fakeDiffr("saved-secret", {}, { hide: ["team"] }); const state = JSON.parse(await readFile(fake.state, "utf8")); - state.classify = { "team classifier": state.classify.classify }; + state.plugins.classify = { + "team classifier": state.plugins.classify.classify, + }; await writeFile(fake.state, JSON.stringify(state)); - const result = await setDiffrConfigValue("classify.team classifier.hide", []); + const result = await setDiffrConfigValue( + "plugins.classify.team classifier.hide", + [], + ); + expect(result.error).toBeUndefined(); expect(result.values).toMatchObject({ - classify: { "team classifier": { hide: [] } }, + plugins: { classify: { "team classifier": { hide: [] } } }, }); - expect(Object.keys(result.values.classify!)).toEqual(["team classifier"]); + const saved = JSON.parse(await readFile(fake.state, "utf8")); + expect(Object.keys(saved.plugins.classify)).toEqual(["team classifier"]); expect(JSON.stringify(result)).not.toContain("saved-secret"); }); test("writes a setting, rereads it, and avoids invalidation for a no-op", async () => { await fakeDiffr(); - expect((await setDiffrConfigValue("shape.context.lines", 3)).changed).toBe( - false, - ); - const result = await setDiffrConfigValue("shape.context.lines", 8); + expect( + (await setDiffrConfigValue("plugins.shape.context.lines", 3)).changed, + ).toBe(false); + const result = await setDiffrConfigValue("plugins.shape.context.lines", 8); expect(result).toMatchObject({ changed: true, // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - values: { shape: { context: { lines: 8 } } }, + values: { plugins: { shape: { context: { lines: 8 } } } }, }); }); @@ -298,12 +313,13 @@ test("saves credentials and options before enabling and preserves blank keys", a const writes = (await fake.calls()).filter((args) => args[1] === "set"); expect(writes.map((args) => args[2])).toEqual( ["api_key", "model", "tests", "enabled"].map( - (key) => `shape.summarize.${key}`, + (key) => `plugins.shape.summarize.${key}`, ), ); await saveDiffrSummarizer({ ...draft, apiKey: "" }); expect( - JSON.parse(await readFile(fake.state, "utf8")).shape.summarize.api_key, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize + .api_key, ).toBe("test-secret"); }); @@ -312,7 +328,7 @@ test("disables first and serializes concurrent saves", async () => { await saveDiffrSummarizer(draft); await Promise.all([ saveDiffrSummarizer({ ...draft, enabled: false, model: "next" }), - setDiffrConfigValue("shape.context.lines", 9), + setDiffrConfigValue("plugins.shape.context.lines", 9), ]); const writes = (await fake.calls()) @@ -320,28 +336,28 @@ test("disables first and serializes concurrent saves", async () => { .slice(-3); expect(writes.map((args) => args[2])).toEqual([ - "shape.summarize.enabled", - "shape.summarize.model", - "shape.context.lines", + "plugins.shape.summarize.enabled", + "plugins.shape.summarize.model", + "plugins.shape.context.lines", ]); }); test("partial failure returns current values and invalidates without leaking the key", async () => { await fakeDiffr(); - vi.stubEnv("FAIL_KEY", "shape.summarize.model"); + vi.stubEnv("FAIL_KEY", "plugins.shape.summarize.model"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); expect(result.changed).toBe(true); expect(result.error).toContain("Some settings were saved"); expect(result.values).toMatchObject({ // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - shape: { summarize: { enabled: false, model: "old" } }, + plugins: { shape: { summarize: { enabled: false, model: "old" } } }, }); expect(JSON.stringify(result)).not.toContain("test-secret"); }); test("failed key saves do not leak args or stderr and do not invalidate", async () => { await fakeDiffr(); - vi.stubEnv("FAIL_KEY", "shape.summarize.api_key"); + vi.stubEnv("FAIL_KEY", "plugins.shape.summarize.api_key"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); expect(result.changed).toBe(false); expect(result.error).toBeDefined(); @@ -442,12 +458,12 @@ test("saved changes invalidate cached comparisons while no-op saves reuse them", try { await consume(); - await setDiffrConfigValue("shape.context.lines", 3); + await setDiffrConfigValue("plugins.shape.context.lines", 3); await consume(); expect( (await fake.calls()).filter((args) => args[0] === "--repo"), ).toHaveLength(1); - await setDiffrConfigValue("shape.context.lines", 8); + await setDiffrConfigValue("plugins.shape.context.lines", 8); await consume(); expect( (await fake.calls()).filter((args) => args[0] === "--repo"), @@ -476,7 +492,7 @@ test("saves a changed prompt and leaves an unchanged one alone", async () => { systemPrompt: "Be terse.", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).shape.summarize + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize .system_prompt, ).toBe("Be terse."); }); @@ -485,7 +501,7 @@ test("switching providers clears the saved key unless a new one is entered", asy const fake = await fakeDiffr("gemini-secret"); const state = async () => - JSON.parse(await readFile(fake.state, "utf8")).shape.summarize; + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize; await saveDiffrSummarizer({ ...draft, @@ -564,7 +580,7 @@ test("reads the environment key of the saved provider", async () => { test("a switch clears the saved key before naming the new provider", async () => { const fake = await fakeDiffr("gemini-secret"); - vi.stubEnv("FAIL_KEY", "shape.summarize.provider"); + vi.stubEnv("FAIL_KEY", "plugins.shape.summarize.provider"); const result = await saveDiffrSummarizer({ ...draft, @@ -574,7 +590,8 @@ test("a switch clears the saved key before naming the new provider", async () => expect(result.error).toBeDefined(); - const state = JSON.parse(await readFile(fake.state, "utf8")).shape.summarize; + const state = JSON.parse(await readFile(fake.state, "utf8")).plugins.shape + .summarize; // The failure left the old provider, and no key for it to send. expect(state).toMatchObject({ provider: "gemini", api_key: "" }); @@ -586,7 +603,7 @@ test("a new endpoint is treated like a new provider", async () => { await expect(testDiffrSummarizer(moved)).rejects.toThrow("Add an API key"); await saveDiffrSummarizer({ ...moved, enabled: false }); expect( - JSON.parse(await readFile(fake.state, "utf8")).shape.summarize, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize, ).toMatchObject({ api_key: "", endpoint: "https://proxy.example/v1" }); }); @@ -680,7 +697,7 @@ test("a custom endpoint kept across a provider switch survives diffr clearing it apiKey: "openai-key", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).shape.summarize, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize, ).toMatchObject({ provider: "openai", endpoint: "https://gateway.example/v1", diff --git a/packages/review/src/server/diffr-config.ts b/packages/review/src/server/diffr-config.ts index 75bfbba92..19f22d823 100644 --- a/packages/review/src/server/diffr-config.ts +++ b/packages/review/src/server/diffr-config.ts @@ -121,7 +121,7 @@ function valueAt(object: JsonObject, key: string): JsonValue | undefined { return value; } -const prefix = "shape.summarize"; +const prefix = "plugins.shape.summarize"; function savedProvider(config: JsonObject): string { const value = valueAt(config, `${prefix}.provider`); @@ -190,13 +190,13 @@ async function configDefaults( const options = valueAt( schema, - "properties.shape.properties.summarize.properties", + "properties.plugins.properties.shape.properties.summarize.properties", ); const defaultHiddenTags = strings( valueAt( schema, - "properties.classify.properties.classify.properties.hide.default", + "properties.plugins.properties.classify.properties.classify.properties.hide.default", ), ); @@ -247,7 +247,7 @@ async function read(rootPath?: string): Promise { ? "environment" : "missing"; - const plugins = config.shape; + const plugins = valueAt(config, "plugins.shape"); if (isJsonObject(plugins)) { for (const plugin of Object.values(plugins)) @@ -432,10 +432,12 @@ export async function testDiffrSummarizer( join(directory, "diffr", "config.toml"), stringify({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - shape: { - order: ["summarize"], - summarize: options, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + order: ["summarize"], + summarize: options, + }, }, }), { mode: 0o600 }, diff --git a/packages/review/src/server/diffr-startup.test.ts b/packages/review/src/server/diffr-startup.test.ts index 4a7882298..9d91cb848 100644 --- a/packages/review/src/server/diffr-startup.test.ts +++ b/packages/review/src/server/diffr-startup.test.ts @@ -52,8 +52,10 @@ console.log('{}'); const ready = async () => { expect(parse(await readFile(file, "utf8"))).toMatchObject({ version: 2, - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - shape: { context: { lines: 17, enabled: false } }, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { context: { lines: 17, enabled: false } }, + }, }); }; From 4db016342f984820efa6ace6dc4f9f464934e40f Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Tue, 6 Oct 2026 18:20:53 -0700 Subject: [PATCH 6/7] Migrate stock and custom diffr plugins to their v2 entries Keep bundled stock settings separate and move external settings directly under their plugin kind. Preserve saved values and order references, allow matching basenames, and update settings, summary configuration, credential redaction, and integration checks. Agent-Session: 01a10ff2-f5c2-7ec1-852f-8ebe516d8af9 --- .../scripts/e2e/journeys/diffr-settings.mjs | 5 +- .../src/diffr-config-section.browser.test.tsx | 76 ++++---- .../review/app/src/diffr-config-section.tsx | 45 +++-- .../scripts/bundled-diffr.integration.mjs | 31 ++-- .../src/server/diffr-config-migration.test.ts | 167 ++++++++++++++---- .../src/server/diffr-config-migration.ts | 53 +++--- .../review/src/server/diffr-config.test.ts | 125 ++++++++----- packages/review/src/server/diffr-config.ts | 16 +- .../review/src/server/diffr-startup.test.ts | 2 +- 9 files changed, 343 insertions(+), 177 deletions(-) diff --git a/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs b/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs index 9559f3d95..fb05d9d60 100644 --- a/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs @@ -117,8 +117,9 @@ export async function run(ctx) { await toggle.click(); await ctx.until( async () => - (await ctx.apiOk("/diffr-config")).values.plugins.shape["test-bodies"] - .enabled === false, + (await ctx.apiOk("/diffr-config")).values.plugins.shape.bundled[ + "test-bodies" + ].enabled === false, "diffr persisted the setting", ); await settings diff --git a/packages/review/app/src/diffr-config-section.browser.test.tsx b/packages/review/app/src/diffr-config-section.browser.test.tsx index b993164e4..cdbb393ce 100644 --- a/packages/review/app/src/diffr-config-section.browser.test.tsx +++ b/packages/review/app/src/diffr-config-section.browser.test.tsx @@ -48,19 +48,21 @@ function config(): ReviewDiffrConfig { values: { version: 2, plugins: { - classify: { classify: { hide: ["test"], hide_deleted: true } }, + classify: { bundled: { hide: ["test"], hide_deleted: true } }, // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. shape: { - context: { enabled: true, lines: 3 }, - "test-bodies": { enabled: true }, - "deleted-bodies": { enabled: true }, - "removed-runs": { enabled: true }, - summarize: { - enabled: false, - provider: "gemini", - model: "test-model", - tests: true, - system_prompt: "Default prompt.", + bundled: { + context: { enabled: true, lines: 3 }, + "test-bodies": { enabled: true }, + "deleted-bodies": { enabled: true }, + "removed-runs": { enabled: true }, + summarize: { + enabled: false, + provider: "gemini", + model: "test-model", + tests: true, + system_prompt: "Default prompt.", + }, }, }, }, @@ -89,16 +91,18 @@ async function mount(overrides: Partial = {}) { values: { version: 2, plugins: { - classify: { classify: { hide: ["test"], hide_deleted: true } }, + classify: { bundled: { hide: ["test"], hide_deleted: true } }, // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. shape: { - summarize: { - enabled: input.enabled, - provider: input.provider, - model: input.model, - endpoint: input.endpoint, - system_prompt: input.systemPrompt, - tests: input.tests, + bundled: { + summarize: { + enabled: input.enabled, + provider: input.provider, + model: input.model, + endpoint: input.endpoint, + system_prompt: input.systemPrompt, + tests: input.tests, + }, }, }, }, @@ -135,13 +139,13 @@ async function open() { test("file-hiding controls use classify fields and retain custom tags across off and on", async () => { const { actions, current } = await mount(); pluginSettings(current).classify = { - classify: { hide: ["custom"], hide_deleted: true }, + bundled: { hide: ["custom"], hide_deleted: true }, }; vi.mocked(actions.set).mockImplementation(async (key, value) => { const field = key.split(".").at(-1)!; const classify = pluginSettings(current).classify as JsonObject; - classify.classify = { - ...(classify.classify as JsonObject), + classify.bundled = { + ...(classify.bundled as JsonObject), [field]: value, }; @@ -150,25 +154,22 @@ test("file-hiding controls use classify fields and retain custom tags across off await open(); await act(async () => page.getByLabelText("Hide files by tag").click()); expect(pluginSettings(current).classify).toMatchObject({ - classify: { hide: [] }, + bundled: { hide: [] }, }); await act(async () => page.getByLabelText("Hide files by tag").click()); expect(pluginSettings(current).classify).toMatchObject({ - classify: { hide: ["custom"] }, + bundled: { hide: ["custom"] }, }); await act(async () => page.getByLabelText("Hide deleted files").click()); expect(pluginSettings(current).classify).toMatchObject({ - classify: { hide_deleted: false }, + bundled: { hide_deleted: false }, }); - expect(actions.set).toHaveBeenCalledWith( - "plugins.classify.classify.hide", - [], - ); - expect(actions.set).toHaveBeenCalledWith("plugins.classify.classify.hide", [ + expect(actions.set).toHaveBeenCalledWith("plugins.classify.bundled.hide", []); + expect(actions.set).toHaveBeenCalledWith("plugins.classify.bundled.hide", [ "custom", ]); expect(actions.set).toHaveBeenCalledWith( - "plugins.classify.classify.hide_deleted", + "plugins.classify.bundled.hide_deleted", false, ); }); @@ -176,12 +177,12 @@ test("file-hiding controls use classify fields and retain custom tags across off test("enabling tag hiding from an empty list uses the schema defaults", async () => { const { actions, current } = await mount(); pluginSettings(current).classify = { - classify: { hide: [], hide_deleted: false }, + bundled: { hide: [], hide_deleted: false }, }; await open(); await act(async () => page.getByLabelText("Hide files by tag").click()); expect(actions.set).toHaveBeenCalledWith( - "plugins.classify.classify.hide", + "plugins.classify.bundled.hide", current.defaultHiddenTags, ); }); @@ -267,7 +268,7 @@ test("writes the selected key and keeps reload visible when collapsed", async () await page.getByLabelText("Collapse test bodies").click(); }); expect(actions.set).toHaveBeenCalledWith( - "plugins.shape.test-bodies.enabled", + "plugins.shape.bundled.test-bodies.enabled", false, ); await expect @@ -303,7 +304,10 @@ test("rejects invalid context lines without writing", async () => { test("tests draft settings without saving, then saves the custom prompt and clears the key", async () => { const saved = config(); - const foldPlugins = pluginSettings(saved).shape as JsonObject; + + const foldPlugins = (pluginSettings(saved).shape as JsonObject) + .bundled as JsonObject; + (foldPlugins.summarize as JsonObject).system_prompt = "Keep my wording."; const { actions } = await mount(saved); await open(); @@ -527,7 +531,7 @@ test("switching provider clears a custom endpoint, and a new endpoint warns abou plugins: { ...(values.plugins as JsonObject), // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - shape: { summarize }, + shape: { bundled: { summarize } }, }, }, }); diff --git a/packages/review/app/src/diffr-config-section.tsx b/packages/review/app/src/diffr-config-section.tsx index 38b5de723..edbb573ca 100644 --- a/packages/review/app/src/diffr-config-section.tsx +++ b/packages/review/app/src/diffr-config-section.tsx @@ -15,10 +15,16 @@ import { Choice } from "./settings-choice"; import { settingsStyles as styles } from "./settings-styles"; const foldSettings = [ - ["plugins.shape.context.enabled", "Collapse unchanged lines"], - ["plugins.shape.test-bodies.enabled", "Collapse test bodies"], - ["plugins.shape.deleted-bodies.enabled", "Collapse deleted function bodies"], - ["plugins.shape.removed-runs.enabled", "Collapse long removed stretches"], + ["plugins.shape.bundled.context.enabled", "Collapse unchanged lines"], + ["plugins.shape.bundled.test-bodies.enabled", "Collapse test bodies"], + [ + "plugins.shape.bundled.deleted-bodies.enabled", + "Collapse deleted function bodies", + ], + [ + "plugins.shape.bundled.removed-runs.enabled", + "Collapse long removed stretches", + ], ] as const; type DisplaySetting = readonly [key: string, label: string]; @@ -60,7 +66,9 @@ function setting( } function provider(config: ReviewDiffrConfig): string { - return String(setting(config, "plugins.shape.summarize.provider") ?? ""); + return String( + setting(config, "plugins.shape.bundled.summarize.provider") ?? "", + ); } /** The provider diffr describes as `id`, if any. */ @@ -74,16 +82,21 @@ function title(config: ReviewDiffrConfig, id: string): string { function summaryDraft(config: ReviewDiffrConfig): ReviewDiffrSummarizerInput { return { - enabled: setting(config, "plugins.shape.summarize.enabled") === true, + enabled: + setting(config, "plugins.shape.bundled.summarize.enabled") === true, provider: provider(config), - model: String(setting(config, "plugins.shape.summarize.model") ?? ""), - endpoint: String(setting(config, "plugins.shape.summarize.endpoint") ?? ""), + model: String( + setting(config, "plugins.shape.bundled.summarize.model") ?? "", + ), + endpoint: String( + setting(config, "plugins.shape.bundled.summarize.endpoint") ?? "", + ), systemPrompt: String( - setting(config, "plugins.shape.summarize.system_prompt") ?? + setting(config, "plugins.shape.bundled.summarize.system_prompt") ?? config.defaultPrompt ?? "", ), - tests: setting(config, "plugins.shape.summarize.tests") === true, + tests: setting(config, "plugins.shape.bundled.summarize.tests") === true, apiKey: "", }; } @@ -171,7 +184,8 @@ export function DiffrConfigSection({ JSON.stringify(draft) !== JSON.stringify(summaryDraft(config)); const unavailable = - !config || setting(config, "plugins.shape.summarize.enabled") === undefined; + !config || + setting(config, "plugins.shape.bundled.summarize.enabled") === undefined; const summaryValid = !!draft?.model.trim(); const savedDraft = config && summaryDraft(config); @@ -231,16 +245,19 @@ export function DiffrConfigSection({ Not available in this configuration.

)} - {key === "plugins.shape.context.enabled" && ( + {key === "plugins.shape.bundled.context.enabled" && ( void run(async () => saved( await actions.set( - "plugins.shape.context.lines", + "plugins.shape.bundled.context.lines", value, ), ), diff --git a/packages/review/scripts/bundled-diffr.integration.mjs b/packages/review/scripts/bundled-diffr.integration.mjs index 2a5627cb6..940d25fa7 100644 --- a/packages/review/scripts/bundled-diffr.integration.mjs +++ b/packages/review/scripts/bundled-diffr.integration.mjs @@ -256,14 +256,14 @@ describe("Relocated runtime diffr integrates with Review streams and settings", assert.equal(await ensureDiffrConfigMigrated(diffrExecutable()), true); const config = await readDiffrConfig(repository); assert.equal(config.values.version, 2); - assert.deepEqual(config.values.plugins.classify.classify.hide, [ + assert.deepEqual(config.values.plugins.classify.bundled.hide, [ "test", "generated", ]); - assert.equal(config.values.plugins.classify.classify.hide_deleted, false); - assert.equal(config.values.plugins.shape.context.lines, 17); + assert.equal(config.values.plugins.classify.bundled.hide_deleted, false); + assert.equal(config.values.plugins.shape.bundled.context.lines, 17); assert.equal( - config.values.plugins.shape.summarize.system_prompt, + config.values.plugins.shape.bundled.summarize.system_prompt, " Use my exact custom instructions.\nKeep # inside the prompt, and answer with JSON if I ask for it.\n", ); @@ -295,23 +295,29 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("settings values and edits round-trip through the staged binary", async () => { const config = await readDiffrConfig(repository); - assert.ok(Number.isInteger(config.values.plugins.shape.context.lines)); - assert.equal(config.values.plugins.shape.summarize.api_key, undefined); + assert.ok( + Number.isInteger(config.values.plugins.shape.bundled.context.lines), + ); + assert.equal( + config.values.plugins.shape.bundled.summarize.api_key, + undefined, + ); assert.ok( ["config", "environment", "missing"].includes(config.credentialSource), ); const updated = await setDiffrConfigValue( - "plugins.shape.context.lines", + "plugins.shape.bundled.context.lines", 7, repository, ); - assert.equal(updated.values.plugins.shape.context.lines, 7); + assert.equal(updated.values.plugins.shape.bundled.context.lines, 7); assert.equal(updated.changed, true); assert.equal(updated.error, undefined); assert.equal( - (await readDiffrConfig(repository)).values.plugins.shape.context.lines, + (await readDiffrConfig(repository)).values.plugins.shape.bundled.context + .lines, 7, ); assert.equal(existsSync(sentinel), false); @@ -320,7 +326,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("a provider switch saves through the binary, clears the old key and keeps the file sparse", async () => { delete process.env.ANTHROPIC_API_KEY; await setDiffrConfigValue( - "plugins.shape.summarize.api_key", + "plugins.shape.bundled.summarize.api_key", "old-secret", repository, ); @@ -341,7 +347,10 @@ describe("Relocated runtime diffr integrates with Review streams and settings", ); assert.equal(saved.error, undefined); - assert.equal(saved.values.plugins.shape.summarize.provider, "anthropic"); + assert.equal( + saved.values.plugins.shape.bundled.summarize.provider, + "anthropic", + ); assert.equal(saved.credentialSource, "missing"); const file = await readFile( diff --git a/packages/review/src/server/diffr-config-migration.test.ts b/packages/review/src/server/diffr-config-migration.test.ts index 0ea1aabe8..e264a3a50 100644 --- a/packages/review/src/server/diffr-config-migration.test.ts +++ b/packages/review/src/server/diffr-config-migration.test.ts @@ -72,9 +72,9 @@ test("converts a saved UI config without changing supported overrides or their c expected.version = 2; expected.plugins = { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: plugins.bundled, + shape: { bundled: plugins.bundled }, classify: { - classify: { hide: ["test", "generated"], hide_deleted: false }, + bundled: { hide: ["test", "generated"], hide_deleted: false }, }, }; expect(parse(migrated)).toEqual(expected); @@ -109,7 +109,7 @@ test.each([ expect(parse(migrated).plugins).toEqual({ classify: { - classify: { + bundled: { hide, hide_deleted: hideDeleted, }, @@ -129,7 +129,7 @@ test.each([true, false])( version: 2, plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { context: { enabled: false, lines: 9 } }, + shape: { bundled: { context: { enabled: false, lines: 9 } } }, }, }); }, @@ -140,18 +140,18 @@ test("handles quoted keys, inline tables and dotted keys without touching a cust expect(parse(migrateDiffrConfig(source))).toEqual({ version: 2, plugins: { - classify: { classify: { hide: [], hide_deleted: false } }, + classify: { bundled: { hide: [], hide_deleted: false } }, // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. shape: { - summarize: { system_prompt: "Keep me" }, - order: ["summarize"], + bundled: { summarize: { system_prompt: "Keep me" } }, + order: ["bundled.summarize"], }, }, }); const dotted = `plugins.bundled.group.enabled = true\nplugins.bundled."hide-files".deleted = false\nclassifier = { hide = ["vendored"] }\n`; expect(parse(migrateDiffrConfig(dotted)).plugins).toEqual({ classify: { - classify: { + bundled: { hide: ["vendored"], hide_deleted: false, }, @@ -164,7 +164,7 @@ test("retains explicit modern classifier values in a mixed config", () => { expect(parse(migrateDiffrConfig(source))).toEqual({ version: 2, plugins: { - classify: { classify: { hide: ["custom"], hide_deleted: true } }, + classify: { bundled: { hide: ["custom"], hide_deleted: true } }, }, }); }); @@ -176,7 +176,7 @@ test("removes retired names from a custom order without sorting surviving plugin plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. shape: { - order: ["context", "custom", "summarize"], + order: ["bundled.context", "custom", "bundled.summarize"], }, }, }); @@ -189,7 +189,7 @@ test("retains comments inside a rewritten plugin order", () => { version: 2, plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { order: ["context", "summarize"] }, + shape: { order: ["bundled.context", "bundled.summarize"] }, }, }); expect(migrated.match(/# keep context first/g)).toHaveLength(1); @@ -265,7 +265,7 @@ test("does not run the binary or change a version 2 file", async () => { const { file, env } = await fixture(); const source = - "version = 2 # already migrated\n[plugins.shape.context]\nlines = 8\n"; + "version = 2 # already migrated\n[plugins.shape.bundled.context]\nlines = 8\n"; await writeFile(file, source); expect(await ensureDiffrConfigMigrated("/no/binary/needed", env)).toBe(false); @@ -360,10 +360,12 @@ byte_limit = 1_000_000 plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. shape: { - context: { lines: 16, extra: { option: "custom", nested: [1, 2] } }, + bundled: { + context: { lines: 16, extra: { option: "custom", nested: [1, 2] } }, + }, custom: { path: "plugins/custom", threshold: 1000 }, }, - classify: { classify: { hide: ["custom"] } }, + classify: { bundled: { hide: ["custom"] } }, }, diff: { byte_limit: 1000000 }, }); @@ -374,7 +376,7 @@ byte_limit = 1_000_000 expect(migrated).toContain(line); }); -test("refuses ambiguous bundled and external plugins without dropping overrides", () => { +test("preserves bundled and custom plugins with the same basename", () => { const source = `[plugins.bundled.context] enabled = false lines = 9 @@ -382,9 +384,18 @@ lines = 9 path = "custom/context" `; - expect(() => migrateDiffrConfig(source)).toThrow( - "both bundled and external entries for context", - ); + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + bundled: { context: { enabled: false, lines: 9 } }, + context: { path: "custom/context" }, + }, + }, + }); + expect(migrated).toContain("lines = 9"); }); test("moves a custom classifier under its manifest name and validates its original path", async () => { @@ -462,7 +473,7 @@ test("serializes concurrent startup migrations and writes one backup", async () version: 2, plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { context: { lines: 11 } }, + shape: { bundled: { context: { lines: 11 } } }, }, }); @@ -476,18 +487,32 @@ test("serializes concurrent startup migrations and writes one backup", async () ).toBe(source); }); -test("keeps ambiguous plugin entries unchanged on disk", async () => { +test("migrates same-basename stock and custom entries on disk", async () => { const { file, executable, env } = await fixture(); const source = '[plugins.bundled.context]\nenabled = false\n[plugins.external.context]\npath = "custom"\n'; await writeFile(file, source); - await expect(ensureDiffrConfigMigrated(executable, env)).rejects.toThrow( - "both bundled and external entries for context", + expect(await ensureDiffrConfigMigrated(executable, env)).toBe(true); + expect(parse(await readFile(file, "utf8"))).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + bundled: { context: { enabled: false } }, + context: { path: "custom" }, + }, + }, + }); + + const backup = (await readdir(path.dirname(file))).find((name) => + name.includes("before-whiteboard-config-v2"), + )!; + + expect(await readFile(path.join(path.dirname(file), backup), "utf8")).toBe( + source, ); - expect(await readFile(file, "utf8")).toBe(source); - expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); }); test.each([ @@ -506,11 +531,16 @@ test.each([ expect(migrated).toContain("0x10"); }); -test("refuses a plugin named order because the v2 key is reserved", () => { - expect(() => - migrateDiffrConfig('[plugins.external.order]\npath = "plugins/custom"\n'), - ).toThrow("plugin named order"); -}); +test.each(["order", "bundled", "bundled.context"])( + "refuses reserved custom plugin name %s", + (name) => { + expect(() => + migrateDiffrConfig( + `[plugins.external.${JSON.stringify(name)}]\npath = "plugins/custom"\n`, + ), + ).toThrow(`custom plugin named ${name}`); + }, +); // Historical literals copied from diffr defaults and config/prune.rs. test.each([ @@ -524,7 +554,7 @@ test.each([ version: 2, plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. - shape: { summarize: {} }, + shape: { bundled: { summarize: {} } }, }, }); expect(migrated).toContain("# old default"); @@ -541,7 +571,7 @@ test.each([ expect( new TomlEdits(migratedCustom).value( - "plugins.shape.summarize.system_prompt", + "plugins.shape.bundled.summarize.system_prompt", ), ).toBe(custom); expect(migratedCustom).toContain( @@ -557,11 +587,11 @@ test("moves the old default context-first order to the v2 context-last default", expect( new TomlEdits(migrateDiffrConfig(source)).value("plugins.shape.order"), ).toEqual([ - "deleted-bodies", - "summarize", - "test-bodies", - "removed-runs", - "context", + "bundled.deleted-bodies", + "bundled.summarize", + "bundled.test-bodies", + "bundled.removed-runs", + "bundled.context", ]); }); @@ -583,11 +613,11 @@ hide = ['team'] plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. shape: { - order: ["context", "team.custom"], - context: { lines: 16 }, + order: ["bundled.context", "team.custom"], + bundled: { context: { lines: 16 } }, "team.custom": { path: "plugins/custom", threshold: 1000 }, }, - classify: { classify: { hide: ["team"] } }, + classify: { bundled: { hide: ["team"] } }, }, }); expect(migrated).toContain("lines = 0x10"); @@ -601,3 +631,62 @@ hide = ['team'] ]) expect(migrated).toContain(`# ${comment}`); }); + +test("keeps a custom classifier whose name conflicts with bundled unchanged", async () => { + const { root, file, executable, env } = await fixture(); + const directory = path.join(root, "diffr", "custom-classifier"); + await mkdir(directory); + await writeFile(path.join(directory, "plugin.toml"), 'name = "bundled"\n'); + const source = '[classifier]\npath = "custom-classifier"\nhide = ["team"]\n'; + await writeFile(file, source); + await expect(ensureDiffrConfigMigrated(executable, env)).rejects.toThrow( + "custom classifier named bundled", + ); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual([ + "config.toml", + "custom-classifier", + ]); +}); + +test("retains order references for bundled and custom plugins with the same basename", () => { + const source = `[plugins] +order = ["external.context", "bundled.context"] +[plugins.bundled.context] +enabled = false +lines = 0x10 # stock override +[plugins.external.context] +path = 'custom/context' # custom path +lines = 1_000 # custom override +`; + + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + order: ["context", "bundled.context"], + bundled: { context: { enabled: false, lines: 16 } }, + context: { path: "custom/context", lines: 1000 }, + }, + }, + }); + + for (const line of source.split("\n").filter((line) => line.includes("#"))) + expect(migrated).toContain(line); +}); + +test("keeps reserved custom plugin entries unchanged on disk", async () => { + const { file, executable, env } = await fixture(); + + const source = + '[plugins.external."bundled.context"]\npath = "custom/context"\n'; + + await writeFile(file, source); + await expect(ensureDiffrConfigMigrated(executable, env)).rejects.toThrow( + "custom plugin named bundled.context", + ); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); diff --git a/packages/review/src/server/diffr-config-migration.ts b/packages/review/src/server/diffr-config-migration.ts index 090872026..0f6de340b 100644 --- a/packages/review/src/server/diffr-config-migration.ts +++ b/packages/review/src/server/diffr-config-migration.ts @@ -33,11 +33,11 @@ const oldOrder = [ ]; const newOrder = [ - "deleted-bodies", - "summarize", - "test-bodies", - "removed-runs", - "context", + "bundled.deleted-bodies", + "bundled.summarize", + "bundled.test-bodies", + "bundled.removed-runs", + "bundled.context", ]; // Exact defaults written by older diffr releases; edited prompts stay intact. @@ -73,7 +73,7 @@ const legacySummarySchema = z.object({ system_prompt: z.string().optional() }); /** Convert v1 paths to v2 without serializing saved plugin option values. */ export function migrateDiffrConfig( source: string, - classifierName = "classify", + classifierName = "bundled", ): string { const parsed = parse(source); const version = configVersionSchema.parse(parsed).version ?? 1; @@ -90,21 +90,17 @@ export function migrateDiffrConfig( const external = config.plugins?.external; const edits = new TomlEdits(source); - if (bundled?.order !== undefined || external?.order !== undefined) - throw new ConfigMigrationError( - "diffr config has a plugin named order. Rename that plugin before migration; plugins.shape.order is reserved.", - ); - for (const name of Object.keys(external ?? {})) - if ( - bundled?.[name] !== undefined && - name !== "group" && - name !== "hide-files" - ) + if (name === "order" || name === "bundled" || name.startsWith("bundled.")) throw new ConfigMigrationError( - `diffr config has both bundled and external entries for ${name}. Choose one before migration.`, + `diffr config has a custom plugin named ${name}. Rename that plugin before migration; plugins.shape.${name} is reserved.`, ); + if (config.classifier?.path !== undefined && classifierName === "bundled") + throw new ConfigMigrationError( + "diffr config has a custom classifier named bundled. Rename that plugin before migration; plugins.classify.bundled is reserved.", + ); + // The independent group switch is retired, for either saved boolean value. edits.remove("plugins.bundled.group"); @@ -131,7 +127,7 @@ export function migrateDiffrConfig( (name) => name !== "bundled.group" && name !== "bundled.hide-files", ) - .map((name) => name.replace(/^(bundled|external)\./, "")); + .map((name) => name.replace(/^external\./, "")); edits.set("plugins.order", migrated); edits.move("plugins.order", "plugins.shape.order"); @@ -156,7 +152,7 @@ export function migrateDiffrConfig( if (name !== "group" && name !== "hide-files") edits.move( tomlKey("plugins", "bundled", name), - tomlKey("plugins", "shape", name), + tomlKey("plugins", "shape", "bundled", name), ); for (const name of Object.keys(external ?? {})) @@ -181,10 +177,16 @@ export function migrateDiffrConfig( ) continue; - if (/^plugins\.(bundled|external)\./.test(key)) + if (key.startsWith("plugins.bundled.")) movedValues.push([ key, - key.replace(/^plugins\.(bundled|external)\./, "plugins.shape."), + key.replace("plugins.bundled.", "plugins.shape.bundled."), + ]); + + if (key.startsWith("plugins.external.")) + movedValues.push([ + key, + key.replace("plugins.external.", "plugins.shape."), ]); if (key.startsWith("classifier.")) @@ -312,7 +314,7 @@ async function legacyClassifierName( ): Promise { const classifier = legacyConfigSchema.parse(parse(source)).classifier; - if (classifier?.path === undefined) return "classify"; + if (classifier?.path === undefined) return "bundled"; const directory = path.resolve( path.dirname(file), @@ -358,7 +360,12 @@ async function validateDiffrConfig( (parts.length === 4 && parts[0] === "plugins" && (parts[1] === "shape" || parts[1] === "classify") && - parts[3] === "path") + parts[3] === "path") || + (parts.length === 5 && + parts[0] === "plugins" && + parts[1] === "shape" && + parts[2] === "bundled" && + parts[4] === "path") ) { const value = z.string().parse(edits.value(key)); diff --git a/packages/review/src/server/diffr-config.test.ts b/packages/review/src/server/diffr-config.test.ts index b8ec20b12..cac9af5d2 100644 --- a/packages/review/src/server/diffr-config.test.ts +++ b/packages/review/src/server/diffr-config.test.ts @@ -131,19 +131,21 @@ async function fakeDiffr( JSON.stringify({ version: 2, plugins: { - ...(classifier && { classify: { classify: classifier } }), + ...(classifier && { classify: { bundled: classifier } }), // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. shape: { - summarize: { - enabled: false, - provider: "gemini", - model: "old", - tests: false, - system_prompt: DEFAULT_PROMPT, - ...(key && { api_key: key }), - ...summarize, + bundled: { + summarize: { + enabled: false, + provider: "gemini", + model: "old", + tests: false, + system_prompt: DEFAULT_PROMPT, + ...(key && { api_key: key }), + ...summarize, + }, + context: { lines: 3, enabled: true }, }, - context: { lines: 3, enabled: true }, }, }, }), @@ -161,9 +163,9 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { } else if (args[0] === 'config' && args[1] === 'schema') { const summarize = ${JSON.stringify(SUMMARIZE_SCHEMA)}; // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - console.log(JSON.stringify({ properties: { plugins: { properties: { ${classifier ? 'classify: { properties: { classify: { properties: { hide: { default: ["generated", "vendored"] } } } } },' : ""} shape: { properties: { summarize } } } } } })); + console.log(JSON.stringify({ properties: { plugins: { properties: { ${classifier ? 'classify: { properties: { bundled: { properties: { hide: { default: ["generated", "vendored"] } } } } },' : ""} shape: { properties: { bundled: { properties: { summarize } } } } } } } })); } else if (args[0] === 'config' && args[1] === 'show') { - if (!args.includes('--reveal') && config.plugins.shape.summarize.api_key) config.plugins.shape.summarize.api_key = ''; + if (!args.includes('--reveal') && config.plugins.shape.bundled.summarize.api_key) config.plugins.shape.bundled.summarize.api_key = ''; console.log(JSON.stringify(config)); } else if (args[0] === 'config' && args[1] === 'set') { if (args[2] === process.env.FAIL_KEY) { console.error('command leaked secret: ' + args.join(' ')); process.exit(2); } @@ -221,6 +223,28 @@ test("reads resolved values and reports credentials without exposing keys", asyn ); }); +test("redacts credentials in bundled and custom shape entries", async () => { + const fake = await fakeDiffr("bundled-secret"); + const state = JSON.parse(await readFile(fake.state, "utf8")); + state.plugins.shape.custom = { + path: "plugins/custom", + api_key: "custom-secret", + enabled: true, + }; + await writeFile(fake.state, JSON.stringify(state)); + + const config = await readDiffrConfig(); + expect(JSON.stringify(config)).not.toMatch( + /bundled-secret|custom-secret|api_key/, + ); + expect(config.values).toMatchObject({ + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { custom: { path: "plugins/custom", enabled: true } }, + }, + }); +}); + test("detects host environment credentials and missing credentials", async () => { await fakeDiffr(); expect((await readDiffrConfig()).credentialSource).toBe("missing"); @@ -237,7 +261,7 @@ test("reads classifier values and schema defaults without old plugin keys", asyn const config = await readDiffrConfig(); expect(config.values).toMatchObject({ plugins: { - classify: { classify: { hide: ["test", "custom"], hide_deleted: false } }, + classify: { bundled: { hide: ["test", "custom"], hide_deleted: false } }, }, }); expect(config.defaultHiddenTags).toEqual(["generated", "vendored"]); @@ -252,31 +276,31 @@ test("writes classifier values without changing credentials", async () => { ); expect( - (await setDiffrConfigValue("plugins.classify.classify.hide", ["custom"])) + (await setDiffrConfigValue("plugins.classify.bundled.hide", ["custom"])) .changed, ).toBe(false); - await setDiffrConfigValue("plugins.classify.classify.hide", []); - await setDiffrConfigValue("plugins.classify.classify.hide_deleted", false); + await setDiffrConfigValue("plugins.classify.bundled.hide", []); + await setDiffrConfigValue("plugins.classify.bundled.hide_deleted", false); const disabled = JSON.parse(await readFile(fake.state, "utf8")); - expect(disabled.plugins.classify.classify).toEqual({ + expect(disabled.plugins.classify.bundled).toEqual({ hide: [], hide_deleted: false, }); - await setDiffrConfigValue("plugins.classify.classify.hide", ["custom"]); + await setDiffrConfigValue("plugins.classify.bundled.hide", ["custom"]); const enabled = JSON.parse(await readFile(fake.state, "utf8")); - expect(enabled.plugins.classify.classify).toEqual({ + expect(enabled.plugins.classify.bundled).toEqual({ hide: ["custom"], hide_deleted: false, }); - expect(enabled.plugins.shape.summarize.api_key).toBe("saved-secret"); - expect(enabled.plugins.shape["hide-files"]).toBeUndefined(); + expect(enabled.plugins.shape.bundled.summarize.api_key).toBe("saved-secret"); + expect(enabled.plugins.shape.bundled["hide-files"]).toBeUndefined(); }); test("writes the native path of a custom classifier whose name contains spaces", async () => { const fake = await fakeDiffr("saved-secret", {}, { hide: ["team"] }); const state = JSON.parse(await readFile(fake.state, "utf8")); state.plugins.classify = { - "team classifier": state.plugins.classify.classify, + "team classifier": state.plugins.classify.bundled, }; await writeFile(fake.state, JSON.stringify(state)); @@ -297,13 +321,19 @@ test("writes the native path of a custom classifier whose name contains spaces", test("writes a setting, rereads it, and avoids invalidation for a no-op", async () => { await fakeDiffr(); expect( - (await setDiffrConfigValue("plugins.shape.context.lines", 3)).changed, + (await setDiffrConfigValue("plugins.shape.bundled.context.lines", 3)) + .changed, ).toBe(false); - const result = await setDiffrConfigValue("plugins.shape.context.lines", 8); + + const result = await setDiffrConfigValue( + "plugins.shape.bundled.context.lines", + 8, + ); + expect(result).toMatchObject({ changed: true, // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - values: { plugins: { shape: { context: { lines: 8 } } } }, + values: { plugins: { shape: { bundled: { context: { lines: 8 } } } } }, }); }); @@ -313,13 +343,13 @@ test("saves credentials and options before enabling and preserves blank keys", a const writes = (await fake.calls()).filter((args) => args[1] === "set"); expect(writes.map((args) => args[2])).toEqual( ["api_key", "model", "tests", "enabled"].map( - (key) => `plugins.shape.summarize.${key}`, + (key) => `plugins.shape.bundled.summarize.${key}`, ), ); await saveDiffrSummarizer({ ...draft, apiKey: "" }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize - .api_key, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize.api_key, ).toBe("test-secret"); }); @@ -328,7 +358,7 @@ test("disables first and serializes concurrent saves", async () => { await saveDiffrSummarizer(draft); await Promise.all([ saveDiffrSummarizer({ ...draft, enabled: false, model: "next" }), - setDiffrConfigValue("plugins.shape.context.lines", 9), + setDiffrConfigValue("plugins.shape.bundled.context.lines", 9), ]); const writes = (await fake.calls()) @@ -336,28 +366,30 @@ test("disables first and serializes concurrent saves", async () => { .slice(-3); expect(writes.map((args) => args[2])).toEqual([ - "plugins.shape.summarize.enabled", - "plugins.shape.summarize.model", - "plugins.shape.context.lines", + "plugins.shape.bundled.summarize.enabled", + "plugins.shape.bundled.summarize.model", + "plugins.shape.bundled.context.lines", ]); }); test("partial failure returns current values and invalidates without leaking the key", async () => { await fakeDiffr(); - vi.stubEnv("FAIL_KEY", "plugins.shape.summarize.model"); + vi.stubEnv("FAIL_KEY", "plugins.shape.bundled.summarize.model"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); expect(result.changed).toBe(true); expect(result.error).toContain("Some settings were saved"); expect(result.values).toMatchObject({ - // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - plugins: { shape: { summarize: { enabled: false, model: "old" } } }, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { bundled: { summarize: { enabled: false, model: "old" } } }, + }, }); expect(JSON.stringify(result)).not.toContain("test-secret"); }); test("failed key saves do not leak args or stderr and do not invalidate", async () => { await fakeDiffr(); - vi.stubEnv("FAIL_KEY", "plugins.shape.summarize.api_key"); + vi.stubEnv("FAIL_KEY", "plugins.shape.bundled.summarize.api_key"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); expect(result.changed).toBe(false); expect(result.error).toBeDefined(); @@ -458,12 +490,12 @@ test("saved changes invalidate cached comparisons while no-op saves reuse them", try { await consume(); - await setDiffrConfigValue("plugins.shape.context.lines", 3); + await setDiffrConfigValue("plugins.shape.bundled.context.lines", 3); await consume(); expect( (await fake.calls()).filter((args) => args[0] === "--repo"), ).toHaveLength(1); - await setDiffrConfigValue("plugins.shape.context.lines", 8); + await setDiffrConfigValue("plugins.shape.bundled.context.lines", 8); await consume(); expect( (await fake.calls()).filter((args) => args[0] === "--repo"), @@ -492,8 +524,8 @@ test("saves a changed prompt and leaves an unchanged one alone", async () => { systemPrompt: "Be terse.", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize - .system_prompt, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize.system_prompt, ).toBe("Be terse."); }); @@ -501,7 +533,8 @@ test("switching providers clears the saved key unless a new one is entered", asy const fake = await fakeDiffr("gemini-secret"); const state = async () => - JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize; + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize; await saveDiffrSummarizer({ ...draft, @@ -580,7 +613,7 @@ test("reads the environment key of the saved provider", async () => { test("a switch clears the saved key before naming the new provider", async () => { const fake = await fakeDiffr("gemini-secret"); - vi.stubEnv("FAIL_KEY", "plugins.shape.summarize.provider"); + vi.stubEnv("FAIL_KEY", "plugins.shape.bundled.summarize.provider"); const result = await saveDiffrSummarizer({ ...draft, @@ -591,7 +624,7 @@ test("a switch clears the saved key before naming the new provider", async () => expect(result.error).toBeDefined(); const state = JSON.parse(await readFile(fake.state, "utf8")).plugins.shape - .summarize; + .bundled.summarize; // The failure left the old provider, and no key for it to send. expect(state).toMatchObject({ provider: "gemini", api_key: "" }); @@ -603,7 +636,8 @@ test("a new endpoint is treated like a new provider", async () => { await expect(testDiffrSummarizer(moved)).rejects.toThrow("Add an API key"); await saveDiffrSummarizer({ ...moved, enabled: false }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize, ).toMatchObject({ api_key: "", endpoint: "https://proxy.example/v1" }); }); @@ -697,7 +731,8 @@ test("a custom endpoint kept across a provider switch survives diffr clearing it apiKey: "openai-key", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.summarize, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize, ).toMatchObject({ provider: "openai", endpoint: "https://gateway.example/v1", diff --git a/packages/review/src/server/diffr-config.ts b/packages/review/src/server/diffr-config.ts index 19f22d823..6538667bb 100644 --- a/packages/review/src/server/diffr-config.ts +++ b/packages/review/src/server/diffr-config.ts @@ -121,7 +121,7 @@ function valueAt(object: JsonObject, key: string): JsonValue | undefined { return value; } -const prefix = "plugins.shape.summarize"; +const prefix = "plugins.shape.bundled.summarize"; function savedProvider(config: JsonObject): string { const value = valueAt(config, `${prefix}.provider`); @@ -190,13 +190,13 @@ async function configDefaults( const options = valueAt( schema, - "properties.plugins.properties.shape.properties.summarize.properties", + "properties.plugins.properties.shape.properties.bundled.properties.summarize.properties", ); const defaultHiddenTags = strings( valueAt( schema, - "properties.plugins.properties.classify.properties.classify.properties.hide.default", + "properties.plugins.properties.classify.properties.bundled.properties.hide.default", ), ); @@ -250,7 +250,11 @@ async function read(rootPath?: string): Promise { const plugins = valueAt(config, "plugins.shape"); if (isJsonObject(plugins)) { - for (const plugin of Object.values(plugins)) + const entries = isJsonObject(plugins.bundled) + ? [...Object.values(plugins), ...Object.values(plugins.bundled)] + : Object.values(plugins); + + for (const plugin of entries) if (isJsonObject(plugin)) delete plugin.api_key; } @@ -435,8 +439,8 @@ export async function testDiffrSummarizer( plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. shape: { - order: ["summarize"], - summarize: options, + order: ["bundled.summarize"], + bundled: { summarize: options }, }, }, }), diff --git a/packages/review/src/server/diffr-startup.test.ts b/packages/review/src/server/diffr-startup.test.ts index 9d91cb848..c1a5a516a 100644 --- a/packages/review/src/server/diffr-startup.test.ts +++ b/packages/review/src/server/diffr-startup.test.ts @@ -54,7 +54,7 @@ console.log('{}'); version: 2, plugins: { // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. - shape: { context: { lines: 17, enabled: false } }, + shape: { bundled: { context: { lines: 17, enabled: false } } }, }, }); }; From 77822ea0b5894ab48f6e18d51282aad0f8335e0c Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Tue, 6 Oct 2026 19:32:26 -0700 Subject: [PATCH 7/7] Use released diffr 0.1.13 AI-assisted: Codex updated the package pins and registry lock records, then verified the installed npm release with the Whiteboard diffr integration suite. --- pnpm-lock.yaml | 56 ++++++++++++++++++++++----------------------- pnpm-workspace.yaml | 14 ++++++------ 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5b321f368..6e3d187b7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -7,8 +7,8 @@ settings: catalogs: default: '@dev.fast/diffr': - specifier: 0.1.12 - version: 0.1.12 + specifier: 0.1.13 + version: 0.1.13 toml-eslint-parser: specifier: ^1.0.3 version: 1.0.3 @@ -58,7 +58,7 @@ importers: devDependencies: '@dev.fast/diffr': specifier: 'catalog:' - version: 0.1.12(zod@4.6.5) + version: 0.1.13(zod@4.6.5) '@dev.fast/review-canvas': specifier: workspace:* version: link:../../packages/review/app @@ -127,7 +127,7 @@ importers: version: 1.5.1(zod@4.6.5) '@dev.fast/diffr': specifier: 'catalog:' - version: 0.1.12(zod@4.6.5) + version: 0.1.13(zod@4.6.5) '@hono/node-server': specifier: 2.0.12 version: 2.0.12(hono@4.13.5) @@ -227,7 +227,7 @@ importers: dependencies: '@dev.fast/diffr': specifier: 'catalog:' - version: 0.1.12(zod@4.6.5) + version: 0.1.13(zod@4.6.5) '@dev.fast/json': specifier: workspace:* version: link:../json @@ -563,35 +563,35 @@ packages: resolution: {integrity: sha512-QxULHAm7cNu72w97JUNCBFODFaXpbDg+dP8b/oWFAZ2MTRppA3U00Y2L1HqaS4J6yBqxwa/Y3nMBaxVKbB/NsA==} engines: {node: '>=20.19.0'} - '@dev.fast/diffr-darwin-arm64@0.1.12': - resolution: {integrity: sha512-x7MlUSQ467hE2u19shRZjFonxK1oXat/SRcakJ/hNIM/w5YH0+Sb8UqB2GUwcGXXkqoVQCjN25Kr52O/qmG3hQ==} + '@dev.fast/diffr-darwin-arm64@0.1.13': + resolution: {integrity: sha512-xTXaa4PHjgbFgSuW+Ery5lvsNwIG9HbliqnmaUxrG4U9yxPtDSsi7hPpJnYfEB+0XO43s090ihiNtzLGfC3BNQ==} cpu: [arm64] os: [darwin] - '@dev.fast/diffr-darwin-x64@0.1.12': - resolution: {integrity: sha512-CuH9D/ETnHA5nVPDgqTxk2nC1Esgn7aNqlLkHdAPkaaTUCV+kwxGcIhr7ldIFAE86f2MWiQ7rDFf49avfV+RmQ==} + '@dev.fast/diffr-darwin-x64@0.1.13': + resolution: {integrity: sha512-XgZ22QRmK8vfRZHsHs9s0wZDkzS4IL1rQaVAizjuIGG+AubYkabqRzFqy/JE2M+MpXSS+fbw1W8r9e/9TulOnA==} cpu: [x64] os: [darwin] - '@dev.fast/diffr-linux-arm64@0.1.12': - resolution: {integrity: sha512-3aNF+7mygPJAPmrWiUP6V8oJ6VBLG9f6Dyk/79Q6+y7e8qh4K1LyCZi2tspIGnQ1ZKw19Q7OUZPBFKv6MOYyyQ==} + '@dev.fast/diffr-linux-arm64@0.1.13': + resolution: {integrity: sha512-tmTQLx9j3JW9mYh5YJ/JJES03c8j/BNJ5PnvEwSsd9FfS7K7+pHD+Lfk2jtzvaROzF2MZCEDI57TDq3b5zhvaQ==} cpu: [arm64] os: [linux] libc: [glibc] - '@dev.fast/diffr-linux-x64@0.1.12': - resolution: {integrity: sha512-J/jcMBLXLTUvITYoXf3IZqjmknUKJp0aDk0xh5txsYXp5IdPhzX8uSbViXuIE/oMpFx6u0ElayBoilEaioFSmg==} + '@dev.fast/diffr-linux-x64@0.1.13': + resolution: {integrity: sha512-xD60m46hDobsG+kJx3kulI/NEpGWw7B1OB4h9tAItjwOrPsh9mMcXlQfJYPGtlZha6CfE1+Ympc7Lexr/QTqfw==} cpu: [x64] os: [linux] libc: [glibc] - '@dev.fast/diffr-win32-x64@0.1.12': - resolution: {integrity: sha512-kzhjzEouKwg44UCfWZ1cD2bCvyhKc3uPB8UdgvGJiNburQWBiSnuJR010zIYUk1lVO2T4ruoRKSChF+ha4/igQ==} + '@dev.fast/diffr-win32-x64@0.1.13': + resolution: {integrity: sha512-c9oaND+TyqrFVccOAmoWfg/25+pDKBnYNzoi4t5pdStKSFjcNSYgltoGIoS+GV1aFNdXOo/6qk3wdDDqKccR/g==} cpu: [x64] os: [win32] - '@dev.fast/diffr@0.1.12': - resolution: {integrity: sha512-k5eeAhx1roIXS28j0pxMsEwVNWlm0OUAKfEFNeFp96DazYJ0yCM2MY8/1M4s6mkiyzNtEpRV/aYl9qmuLi8M8Q==} + '@dev.fast/diffr@0.1.13': + resolution: {integrity: sha512-0awUVjvSWRv1oQu3prCrKvqPBH64Eqk+AtqUT67a3AxcKdvm4CrGeF+cPVy9D0PVhbXePPeqV0spjCwKShO0gw==} engines: {node: '>=20'} peerDependencies: zod: ^4.0.0 @@ -3469,30 +3469,30 @@ snapshots: '@csstools/css-tokenizer@4.0.0': {} - '@dev.fast/diffr-darwin-arm64@0.1.12': + '@dev.fast/diffr-darwin-arm64@0.1.13': optional: true - '@dev.fast/diffr-darwin-x64@0.1.12': + '@dev.fast/diffr-darwin-x64@0.1.13': optional: true - '@dev.fast/diffr-linux-arm64@0.1.12': + '@dev.fast/diffr-linux-arm64@0.1.13': optional: true - '@dev.fast/diffr-linux-x64@0.1.12': + '@dev.fast/diffr-linux-x64@0.1.13': optional: true - '@dev.fast/diffr-win32-x64@0.1.12': + '@dev.fast/diffr-win32-x64@0.1.13': optional: true - '@dev.fast/diffr@0.1.12(zod@4.6.5)': + '@dev.fast/diffr@0.1.13(zod@4.6.5)': dependencies: zod: 4.6.5 optionalDependencies: - '@dev.fast/diffr-darwin-arm64': 0.1.12 - '@dev.fast/diffr-darwin-x64': 0.1.12 - '@dev.fast/diffr-linux-arm64': 0.1.12 - '@dev.fast/diffr-linux-x64': 0.1.12 - '@dev.fast/diffr-win32-x64': 0.1.12 + '@dev.fast/diffr-darwin-arm64': 0.1.13 + '@dev.fast/diffr-darwin-x64': 0.1.13 + '@dev.fast/diffr-linux-arm64': 0.1.13 + '@dev.fast/diffr-linux-x64': 0.1.13 + '@dev.fast/diffr-win32-x64': 0.1.13 '@dual-bundle/import-meta-resolve@4.2.1': {} diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 30136bfb1..4d86bd16b 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -5,7 +5,7 @@ packages: - "apps/*" catalog: - '@dev.fast/diffr': 0.1.12 + '@dev.fast/diffr': 0.1.13 toml-eslint-parser: ^1.0.3 zod: "4.6.5" @@ -19,12 +19,12 @@ verifyDepsBeforeRun: false minimumReleaseAge: 10080 minimumReleaseAgeStrict: true minimumReleaseAgeExclude: - - "@dev.fast/diffr@0.1.12" - - "@dev.fast/diffr-darwin-arm64@0.1.12" - - "@dev.fast/diffr-darwin-x64@0.1.12" - - "@dev.fast/diffr-linux-arm64@0.1.12" - - "@dev.fast/diffr-linux-x64@0.1.12" - - "@dev.fast/diffr-win32-x64@0.1.12" + - "@dev.fast/diffr@0.1.13" + - "@dev.fast/diffr-darwin-arm64@0.1.13" + - "@dev.fast/diffr-darwin-x64@0.1.13" + - "@dev.fast/diffr-linux-arm64@0.1.13" + - "@dev.fast/diffr-linux-x64@0.1.13" + - "@dev.fast/diffr-win32-x64@0.1.13" # Ask tracks the latest ACP SDK and adapters, and the CLI packages they pin. - "@agentclientprotocol/sdk@1.5.1" - "@agentclientprotocol/claude-agent-acp@0.84.0"