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/code-oss/src/vs/editor/browser/widget/diffEditor/diffEditorViewModel.ts b/apps/review-desktop/code-oss/src/vs/editor/browser/widget/diffEditor/diffEditorViewModel.ts index 4e3e59537..72e3aa891 100644 --- a/apps/review-desktop/code-oss/src/vs/editor/browser/widget/diffEditor/diffEditorViewModel.ts +++ b/apps/review-desktop/code-oss/src/vs/editor/browser/widget/diffEditor/diffEditorViewModel.ts @@ -689,7 +689,7 @@ class SuppliedContextGap extends UnchangedRegion { constructor(private readonly gap: NonNullable[number]) { super(gap.originalStart, gap.modifiedStart, Math.max(gap.originalCount, gap.modifiedCount), 0, 0); } - override readLabel(reader: IReader | undefined): string | undefined { return this.gap.labelObservable?.read(reader) ?? this.gap.label; } + override readLabel(_reader: IReader | undefined): string | undefined { return this.gap.label; } override get foldStateId(): number | undefined { return this.gap.foldStateId; } override get owner(): 'base' | 'head' | 'both' { return this.gap.owner ?? 'both'; } override get change(): 'unchanged' | 'inserted' | 'removed' | 'modified' { return this.gap.change ?? 'unchanged'; } diff --git a/apps/review-desktop/code-oss/src/vs/editor/common/diff/documentDiffProvider.ts b/apps/review-desktop/code-oss/src/vs/editor/common/diff/documentDiffProvider.ts index 6bf2d744a..f06e525c0 100644 --- a/apps/review-desktop/code-oss/src/vs/editor/common/diff/documentDiffProvider.ts +++ b/apps/review-desktop/code-oss/src/vs/editor/common/diff/documentDiffProvider.ts @@ -3,7 +3,6 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import type { IObservable } from "../../../base/common/observable.js"; import { CancellationToken } from '../../../base/common/cancellation.js'; import { IRange } from '../core/range.js'; import { Event } from '../../../base/common/event.js'; @@ -67,8 +66,6 @@ export interface IDocumentContextGap { readonly originalCount: number; readonly modifiedCount: number; readonly label?: string; - /** Presentation updates do not invalidate the diff or its fold state. */ - readonly labelObservable?: IObservable; readonly foldStateId?: number; /** Where the fold control originates; independent of the enclosed code's change status. */ readonly owner?: 'base' | 'head' | 'both'; 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..0d0104cd4 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 @@ -7,8 +7,6 @@ import assert from "node:assert/strict"; import test from "node:test"; import { projectInlineSourceAlignment, projectSplitSourceAlignment } from "../../editor/common/diff/sourceLineAlignment.js"; import { - collapsedRegions, - hiddenLinesOf, structuralContextGaps, bandDetail, structuralHighlights, @@ -30,10 +28,10 @@ function leaf(id: number, start: number, end: number, extra: Partial { }); -test("a collapsed region hides every line it covers, fold or leaf; nested collapses are subsumed", () => { - const collapsed = { collapsed: true, label: "x" }; - const inner = fold(3, [leaf(4, 3, 4), leaf(5, 4, 6)]); - inner.visibility = collapsed; - const body = fold(2, [leaf(6, 2, 3), inner, leaf(7, 6, 8)]); - body.visibility = collapsed; - assert.deepEqual(hiddenLinesOf(body), { start: 2, end: 8 }); - assert.deepEqual(hiddenLinesOf(leaf(1, 0, 2, { visibility: collapsed })), { start: 0, end: 2 }); - assert.deepEqual(collapsedRegions([leaf(1, 0, 2, { visibility: collapsed }), body], (id) => id === 1 || id === 2 || id === 3).map((r) => r.fold_state_id), [1, 2]); - assert.deepEqual(collapsedRegions([leaf(1, 0, 2), body], (id) => id === 3).map((r) => r.fold_state_id), [3]); -}); - test("every collapsed region becomes a labelled band: paired across sides, or one-sided at the aligned line", () => { const pseudocode = "// pseudocode\nif not key: return None\nreturn call(key)"; const gapL = leaf(1, 0, 40, { visibility: { collapsed: true, label: "40 unchanged lines" } }); @@ -188,10 +174,10 @@ test("every collapsed region becomes a labelled band: paired across sides, or on }; const gaps = structuralContextGaps(diff, (id) => id === 1 || id === 2 || id === 5); assert.deepEqual(gaps, [ - { originalStart: 1, originalCount: 40, modifiedStart: 1, modifiedCount: 40, label: "40 unchanged lines", owner: "both", change: "unchanged", collapsed: true, foldStateId: 1, regionIds: [11, 1], breadcrumbs: true }, + { originalStart: 1, originalCount: 40, modifiedStart: 1, modifiedCount: 40, label: "40 unchanged lines", owner: "both", change: "unchanged", collapsed: true, foldStateId: 1, breadcrumbs: true }, // The removed body hides lines 42..60 on the left; its rows precede the added body's, so it anchors before them on the right. - { originalStart: 42, originalCount: 19, modifiedStart: 41, modifiedCount: 0, label: "19 lines removed", owner: "base", change: "removed", collapsed: true, foldStateId: 2, regionIds: [2], breadcrumbs: true }, - { originalStart: 61, originalCount: 0, modifiedStart: 42, modifiedCount: 29, label: pseudocode, owner: "head", change: "inserted", collapsed: true, foldStateId: 5, regionIds: [5], breadcrumbs: true }, + { originalStart: 42, originalCount: 19, modifiedStart: 41, modifiedCount: 0, label: "19 lines removed", owner: "base", change: "removed", collapsed: true, foldStateId: 2, breadcrumbs: true }, + { originalStart: 61, originalCount: 0, modifiedStart: 42, modifiedCount: 29, label: pseudocode, owner: "head", change: "inserted", collapsed: true, foldStateId: 5, breadcrumbs: true }, ]); // A region the reader revealed stays a band, marked open, so the editor keeps a fold control on it. const revealed = structuralContextGaps(diff, (id) => id === 2 || id === 5, (id) => (id === 1 ? false : id === 2 || id === 5 ? true : undefined)); @@ -222,7 +208,7 @@ test("a one-sided band also hides the opposite lines the zip aligned with it", ( const [gap] = structuralContextGaps(diff, (id) => id === 2); assert.deepEqual(gap, { originalStart: 2, originalCount: 5, modifiedStart: 2, modifiedCount: 5, - label: "5 unchanged lines", owner: "base", change: "unchanged", collapsed: true, foldStateId: 2, regionIds: [2], breadcrumbs: true, + label: "5 unchanged lines", owner: "base", change: "unchanged", collapsed: true, foldStateId: 2, breadcrumbs: true, }); }); @@ -242,7 +228,7 @@ test("a one-sided band starts after the row before its fold and hides only the o ]); assert.deepEqual(structuralContextGaps(inserted, (id) => id === 20), [{ originalStart: 3, originalCount: 2, modifiedStart: 4, modifiedCount: 6, - label: "// pseudocode\nreturn rows", owner: "head", change: "inserted", collapsed: true, foldStateId: 20, regionIds: [20], breadcrumbs: true, + label: "// pseudocode\nreturn rows", owner: "head", change: "inserted", collapsed: true, foldStateId: 20, breadcrumbs: true, }]); // The mirror: a left-only function whose rows are all filler on the right, followed by a right-only line. const removed = fold(20, [leaf(22, 3, 6)], ["deleted-bodies:function"]); @@ -256,7 +242,7 @@ test("a one-sided band starts after the row before its fold and hides only the o assert.deepEqual(structuralRows(deleted).slice(0, 8), [[0, 0], [1, 1], [2, null], [3, null], [4, null], [5, null], [null, 2], [6, 3]]); assert.deepEqual(structuralContextGaps(deleted, (id) => id === 20), [{ originalStart: 4, originalCount: 3, modifiedStart: 3, modifiedCount: 0, - label: "3 lines removed", owner: "base", change: "removed", collapsed: true, foldStateId: 20, regionIds: [20], breadcrumbs: true, + label: "3 lines removed", owner: "base", change: "removed", collapsed: true, foldStateId: 20, breadcrumbs: true, }]); }); @@ -277,7 +263,7 @@ test("alignment and fold state are separate: the zip follows one, collapse follo assert.deepEqual(gaps.map((g) => [g.change, g.foldStateId]).sort(), [["inserted", 7], ["unchanged", 7]]); assert.deepEqual(gaps.find((g) => g.change === "unchanged"), { originalStart: 2, originalCount: 3, modifiedStart: 4, modifiedCount: 3, - label: "3 hidden lines", owner: "both", change: "unchanged", collapsed: true, foldStateId: 7, regionIds: [5, 2], breadcrumbs: true, + label: "3 hidden lines", owner: "both", change: "unchanged", collapsed: true, foldStateId: 7, breadcrumbs: true, }); // With fold state 7 open, neither the docstring nor the function is a band. assert.deepEqual(structuralContextGaps(diff, () => false), []); diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts index 10a628287..9fb83ea9c 100644 --- a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.ts @@ -79,19 +79,19 @@ export function structuralContextScopes(diff: StructuralTextDiff) { } region.children.forEach(visit); }; - source?.regions?.forEach(visit); + if (source) visit(source.root); return result; }; return { original: scopes(diff.lhs), modified: scopes(diff.rhs) }; } -export function structuralLeaves(regions: readonly StructuralRegion[] | undefined): StructuralLeaf[] { +function structuralLeaves(root: StructuralRegion): StructuralLeaf[] { const leaves: StructuralLeaf[] = []; const walk = (region: StructuralRegion) => { if (region.kind === "leaf") leaves.push(region); else for (const child of region.children) walk(child); }; - for (const region of regions ?? []) walk(region); + walk(root); return leaves; } @@ -120,7 +120,7 @@ export function structuralHighlights(diff: StructuralTextDiff) { for (let line = start; line < end; line++) changedLines.push(line + 1); } const spans = []; - for (const leaf of structuralLeaves(source.regions)) { + for (const leaf of structuralLeaves(source.root)) { for (const span of leaf.changed ?? []) { spans.push({ startLineNumber: span.line + 1, @@ -177,8 +177,6 @@ export interface StructuralGap { collapsed: boolean; /** The fold-state id of the region(s) this band hides; toggling the band toggles it. */ foldStateId: number; - /** Label precedence: head first, then base. */ - regionIds: readonly number[]; /** False for a bundled docstring: its band shows the bare count, no symbol names. */ breadcrumbs: boolean; } @@ -195,37 +193,20 @@ export function bandDetail(label: string): string { return body.join("\n"); } -/** - * The lines a collapsed region hides on its side, zero-based half-open: every - * line it covers, fold or leaf. A fold covers its body alone — the line that - * opens the construct is the last line of the leaf before it — so a band - * hides exactly the fold's range, and the signature above it stays a row. - */ -export function hiddenLinesOf(region: StructuralRegion): { start: number; end: number } { - return regionLines(region); -} - -/** Collapsed regions of one side, outermost first; a collapsed descendant of a collapsed region is subsumed. `isCollapsed` answers for a fold-state id. */ -export function collapsedRegions( - regions: readonly StructuralRegion[] | undefined, - isCollapsed: (foldStateId: number) => boolean, -): StructuralRegion[] { - return knownRegions(regions, (id) => (isCollapsed(id) ? true : undefined)).map((r) => r.region); -} - /** * Regions of one side the state knows about, outermost first: collapsed ones * and ones a reader revealed. A collapsed region subsumes its descendants; a * revealed one still lists them, since a child may be collapsed on its own. */ -export function knownRegions( - regions: readonly StructuralRegion[] | undefined, +function knownRegions( + root: StructuralRegion | undefined, state: (foldStateId: number) => boolean | undefined, ): { region: StructuralRegion; collapsed: boolean }[] { const result: { region: StructuralRegion; collapsed: boolean }[] = []; const walk = (region: StructuralRegion) => { const known = state(region.fold_state_id); - const hides = hiddenLinesOf(region).end > hiddenLinesOf(region).start; + const { start, end } = regionLines(region); + const hides = end > start; if (known === true) { if (hides) result.push({ region, collapsed: true }); return; @@ -234,7 +215,7 @@ export function knownRegions( if (known === false && hides && region.visibility?.collapsed === true) result.push({ region, collapsed: false }); if (region.kind === "fold") for (const child of region.children) walk(child); }; - for (const region of regions ?? []) walk(region); + if (root) walk(root); return result; } @@ -270,8 +251,8 @@ export function structuralContextGaps( for (let index = first; index <= last; index++) if (rows[index][other] !== null) count++; return { start: before + 2, count }; }; - const lhs = knownRegions(diff.lhs?.regions, state); - const rhs = knownRegions(diff.rhs?.regions, state); + const lhs = knownRegions(diff.lhs?.root, state); + const rhs = knownRegions(diff.rhs?.root, state); // Leaves pair by alignment; folds pair by fold state, the only identity they share across sides. const usedRhs = new Set(); const pairs = (left: StructuralRegion, right: StructuralRegion) => { @@ -282,18 +263,18 @@ export function structuralContextGaps( rhs.find(({ region: right }) => !usedRhs.has(right) && pairs(left, right)); const gaps: StructuralGap[] = []; for (const { region: left, collapsed } of lhs) { - const hidden = hiddenLinesOf(left); + const hidden = regionLines(left); const partner = counterpart(left); if (partner) { usedRhs.add(partner.region); - const right = hiddenLinesOf(partner.region); + const right = regionLines(partner.region); gaps.push({ originalStart: hidden.start + 1, originalCount: hidden.end - hidden.start, modifiedStart: right.start + 1, modifiedCount: right.end - right.start, label: partner.region.visibility?.label || left.visibility?.label || "", owner: "both", change: "unchanged", collapsed: collapsed && partner.collapsed, - foldStateId: left.fold_state_id, regionIds: [partner.region.id, left.id], + foldStateId: left.fold_state_id, breadcrumbs: !isDocstring(left) && !isDocstring(partner.region), }); continue; @@ -302,18 +283,18 @@ export function structuralContextGaps( gaps.push({ originalStart: hidden.start + 1, originalCount: hidden.end - hidden.start, modifiedStart: opposite.start, modifiedCount: opposite.count, - label: left.visibility?.label || "", owner: "base", change: "unchanged", collapsed, foldStateId: left.fold_state_id, regionIds: [left.id], + label: left.visibility?.label || "", owner: "base", change: "unchanged", collapsed, foldStateId: left.fold_state_id, breadcrumbs: !isDocstring(left), }); } for (const { region: right, collapsed } of rhs) { if (usedRhs.has(right)) continue; - const hidden = hiddenLinesOf(right); + const hidden = regionLines(right); const opposite = oppositeSpan(1, hidden); gaps.push({ originalStart: opposite.start, originalCount: opposite.count, modifiedStart: hidden.start + 1, modifiedCount: hidden.end - hidden.start, - label: right.visibility?.label || "", owner: "head", change: "unchanged", collapsed, foldStateId: right.fold_state_id, regionIds: [right.id], + label: right.visibility?.label || "", owner: "head", change: "unchanged", collapsed, foldStateId: right.fold_state_id, breadcrumbs: !isDocstring(right), }); } 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/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/reviewStructuralDiff.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts index 639860b7c..519d95efa 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts @@ -19,7 +19,7 @@ import { ICodeEditorService } from "../../editor/browser/services/codeEditorServ import type { IDiffEditor } from "../../editor/browser/editorBrowser.js"; import { LineRange } from "../../editor/common/core/ranges/lineRange.js"; import { DetailedLineRangeMapping } from "../../editor/common/diff/rangeMapping.js"; -import { autorun, derived, type IObservable } from "../../base/common/observable.js"; +import { autorun, type IObservable } from "../../base/common/observable.js"; import type { UnchangedRegion } from "../../editor/browser/widget/diffEditor/diffEditorViewModel.js"; import { structuralContextGaps, @@ -170,16 +170,7 @@ export class StructuralDiffProvider implements IDocumentDiffProvider { diff, (id) => this.session.isRegionCollapsed(path!, id) === true, (id) => this.session.isRegionCollapsed(path!, id), - ).map(gap => ({ - ...gap, labelObservable: derived(reader => { - for (const id of gap.regionIds) { - const label = this.session.regionLabel(path!, id).read(reader); - if (label) return label; - } - const count = Math.max(gap.originalCount, gap.modifiedCount); - return `${count} hidden line${count === 1 ? "" : "s"}`; - }) - })), + ), changeHighlights: highlights, }; } 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..50c232938 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,32 @@ 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 task = session.start(); + try { + const fileResult = await session.fileResult('a.ts'); + assert.equal(fileResult.hidden, '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 +80,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 +96,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..1a8ccb826 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,4 @@ import { RunOnceScheduler } from "../../base/common/async.js"; -import { observableValue, transaction, 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 +10,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 +23,7 @@ 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,25 +99,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); - } - - /** Stable presentation state, shared by all editors displaying this region. */ - regionLabel(path: string, id: number): IObservable { - const key = `${path}:${id}`; - let label = this.labels.get(key); - if (!label) { - label = observableValue(this, this.regions.get(key)?.visibility?.label); - this.labels.set(key, label); - } - return label; + else this.notify(undefined, true); } private notify(path?: string, status = false): void { @@ -135,15 +112,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); } @@ -159,34 +132,16 @@ export class StructuralDiffSession extends Disposable { const diff = event.diff; if (diff.type === "text") { const seed = (region: StructuralRegion) => { - this.regions.set(`${path}:${region.id}`, region); 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 { @@ -200,11 +155,7 @@ export class StructuralDiffSession extends Disposable { this.abort.abort(); this.results.clear(); this.folds.clear(); - 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 58f467ca9..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.10", + "@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..73399b808 100644 --- a/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs @@ -11,7 +11,6 @@ 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: "", @@ -54,7 +53,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 +116,9 @@ 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.plugins.shape.bundled[ + "test-bodies" + ].enabled === false, "diffr persisted the setting", ); await settings 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/apps/review-desktop/scripts/linux/nixos/vm-test.nix b/apps/review-desktop/scripts/linux/nixos/vm-test.nix index 3c94ea698..40bca16d2 100644 --- a/apps/review-desktop/scripts/linux/nixos/vm-test.nix +++ b/apps/review-desktop/scripts/linux/nixos/vm-test.nix @@ -13,7 +13,7 @@ let targetPkgs = p: package.runtimePackages p ++ [ p.file p.binutils ]; 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 while IFS= read -r -d $'\0' binary; do if file -b "$binary" | grep -q '^ELF .*dynamically linked'; then if ldd "$binary" 2>&1 | grep -q 'not found'; then diff --git a/apps/review-desktop/scripts/lsp-e2e.mjs b/apps/review-desktop/scripts/lsp-e2e.mjs index cb37f91b3..80a7cb4d9 100644 --- a/apps/review-desktop/scripts/lsp-e2e.mjs +++ b/apps/review-desktop/scripts/lsp-e2e.mjs @@ -38,17 +38,13 @@ 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"); - -const structuralDiffAvailable = - !!process.env.REVIEW_DIFFR_BINARY || - existsSync(bundledDiffr) || - (await exec("which", ["diffr"]).then( - () => true, - () => false, - )); + ? path.join(values.app, "Contents/Resources/app/review-runtime", diffrPackage) + : path.join(workspace, diffrPackage); + +const structuralDiffAvailable = existsSync(bundledDiffr); const root = await realpath( await mkdtemp( 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..3d1d02572 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,13 +251,12 @@ 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, "--format", "ndjson", - "--stream-annotations", base, head, "--", 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-protocol/package.json b/packages/review-protocol/package.json index 1f9ecfe16..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.10", + "@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..5b0abbf03 100644 --- a/packages/review/app/src/diffr-config-section.browser.test.tsx +++ b/packages/review/app/src/diffr-config-section.browser.test.tsx @@ -44,21 +44,25 @@ function config(): ReviewDiffrConfig { keylessCustomEndpoint: id === "openai", })), defaultPrompt: "Default prompt.", + defaultHiddenTags: ["generated", "vendored", "test"], values: { + version: 2, 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.", + classify: { bundled: { hide: ["test"], hide_deleted: true } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + 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.", + }, }, }, }, @@ -66,6 +70,10 @@ function config(): ReviewDiffrConfig { }; } +function pluginSettings(config: ReviewDiffrConfig): JsonObject { + return config.values.plugins as JsonObject; +} + async function mount(overrides: Partial = {}) { const current = { ...config(), ...overrides }; @@ -79,16 +87,22 @@ async function mount(overrides: Partial = {}) { async (input) => ({ ...current, changed: true, + defaultHiddenTags: ["generated", "vendored", "test"], values: { + version: 2, plugins: { - bundled: { - summarize: { - enabled: input.enabled, - provider: input.provider, - model: input.model, - endpoint: input.endpoint, - system_prompt: input.systemPrompt, - tests: input.tests, + classify: { bundled: { hide: ["test"], hide_deleted: true } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + bundled: { + summarize: { + enabled: input.enabled, + provider: input.provider, + model: input.model, + endpoint: input.endpoint, + system_prompt: input.systemPrompt, + tests: input.tests, + }, }, }, }, @@ -122,6 +136,57 @@ 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(); + pluginSettings(current).classify = { + 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.bundled = { + ...(classify.bundled as JsonObject), + [field]: value, + }; + + return { ...current, changed: true }; + }); + await open(); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(pluginSettings(current).classify).toMatchObject({ + bundled: { hide: [] }, + }); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(pluginSettings(current).classify).toMatchObject({ + bundled: { hide: ["custom"] }, + }); + await act(async () => page.getByLabelText("Hide deleted files").click()); + expect(pluginSettings(current).classify).toMatchObject({ + bundled: { hide_deleted: false }, + }); + expect(actions.set).toHaveBeenCalledWith("plugins.classify.bundled.hide", []); + expect(actions.set).toHaveBeenCalledWith("plugins.classify.bundled.hide", [ + "custom", + ]); + expect(actions.set).toHaveBeenCalledWith( + "plugins.classify.bundled.hide_deleted", + false, + ); +}); + +test("enabling tag hiding from an empty list uses the schema defaults", async () => { + const { actions, current } = await mount(); + pluginSettings(current).classify = { + bundled: { hide: [], hide_deleted: false }, + }; + await open(); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(actions.set).toHaveBeenCalledWith( + "plugins.classify.bundled.hide", + current.defaultHiddenTags, + ); +}); + test("starts collapsed and reads only on first expansion", async () => { const { actions } = await mount(); expect(actions.read).not.toHaveBeenCalled(); @@ -144,7 +209,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.bundled.test-bodies.enabled", + "plugins.shape.bundled.test-bodies.enabled", false, ); await expect @@ -178,8 +243,14 @@ 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 = (pluginSettings(saved).shape as JsonObject) + .bundled 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 +274,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 () => { @@ -270,31 +344,6 @@ test("does not prompt for reload after a no-op and disables reload while testing await act(async () => finish("summary")); }); -test("shows partial save errors with authoritative values and a reload prompt", async () => { - const { actions, current } = await mount(); - await open(); - vi.mocked(actions.saveSummarizer).mockResolvedValue({ - ...current, - changed: true, - error: "Some settings were saved.", - }); - await act(async () => { - await page.getByLabelText("Model", { exact: true }).fill("new-model"); - }); - await act(async () => { - await page.getByRole("button", { name: "Save summaries" }).click(); - }); - await expect - .element(page.getByRole("alert")) - .toHaveTextContent("Some settings were saved."); - await expect - .element(page.getByLabelText("Model", { exact: true })) - .toHaveValue("test-model"); - await expect - .element(page.getByRole("button", { name: "Reload window", exact: true })) - .toBeVisible(); -}); - test("defaults to Gemini and switching provider resets the model and key", async () => { const { actions } = await mount(); await open(); @@ -392,7 +441,16 @@ test("switching provider clears a custom endpoint, and a new endpoint warns abou system_prompt: "Default prompt.", }; - await mount({ values: { ...values, plugins: { bundled: { 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: { bundled: { 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..d15e228ad 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 { isStringValue } from "@dev.fast/json"; import { type JsonValue, type ReviewDiffrConfig, @@ -13,14 +14,21 @@ import { type ReactNode, useEffect, useRef, useState } from "react"; import { Choice } from "./settings-choice"; import { settingsStyles as styles } from "./settings-styles"; +const hideKey = "plugins.classify.bundled.hide"; + 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"], + ["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", + ], + [hideKey, "Hide files by tag"], + ["plugins.classify.bundled.hide_deleted", "Hide deleted files"], ] as const; function setting( @@ -29,14 +37,16 @@ function setting( ): 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, "plugins.shape.bundled.summarize.provider") ?? "", + ); } /** The provider diffr describes as `id`, if any. */ @@ -50,14 +60,21 @@ function title(config: ReviewDiffrConfig, id: string): string { function summaryDraft(config: ReviewDiffrConfig): ReviewDiffrSummarizerInput { return { - enabled: setting(config, "summarize.enabled") === true, + enabled: + setting(config, "plugins.shape.bundled.summarize.enabled") === true, provider: provider(config), - model: String(setting(config, "summarize.model") ?? ""), - endpoint: String(setting(config, "summarize.endpoint") ?? ""), + model: String( + setting(config, "plugins.shape.bundled.summarize.model") ?? "", + ), + endpoint: String( + setting(config, "plugins.shape.bundled.summarize.endpoint") ?? "", + ), systemPrompt: String( - setting(config, "summarize.system_prompt") ?? config.defaultPrompt ?? "", + setting(config, "plugins.shape.bundled.summarize.system_prompt") ?? + config.defaultPrompt ?? + "", ), - tests: setting(config, "summarize.tests") === true, + tests: setting(config, "plugins.shape.bundled.summarize.tests") === true, apiKey: "", }; } @@ -74,6 +91,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 +103,7 @@ export function DiffrConfigSection({ actions.read().then( (result) => { if (cancelled) return; + rememberTags(result); setConfig(result); setDraft(summaryDraft(result)); }, @@ -121,7 +140,15 @@ export function DiffrConfigSection({ } } + function rememberTags(result: ReviewDiffrConfig) { + const tags = setting(result, hideKey); + + 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 +161,12 @@ export function DiffrConfigSection({ JSON.stringify(draft) !== JSON.stringify(summaryDraft(config)); const unavailable = - !config || setting(config, "summarize.enabled") === undefined; + !config || + setting(config, "plugins.shape.bundled.summarize.enabled") === undefined; const summaryValid = !!draft?.model.trim(); const savedDraft = config && summaryDraft(config); - const hiddenTags = config ? setting(config, "hide-files.tags") : undefined; + const hiddenTags = config && setting(config, hideKey); return (
@@ -164,17 +192,24 @@ export function DiffrConfigSection({ 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,16 +219,19 @@ export function DiffrConfigSection({ Not available in this configuration.

)} - {key === "context.enabled" && ( + {key === "plugins.shape.bundled.context.enabled" && ( void run(async () => saved( await actions.set( - "plugins.bundled.context.lines", + "plugins.shape.bundled.context.lines", value, ), ), @@ -202,12 +240,11 @@ export function DiffrConfigSection({ /> )} - {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 6f0d6a2aa..5f3b31460 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,13 +56,14 @@ "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": { "@agentclientprotocol/claude-agent-acp": "0.84.0", "@agentclientprotocol/codex-acp": "2.0.0", "@agentclientprotocol/sdk": "1.5.1", + "@dev.fast/diffr": "catalog:", "@hono/node-server": "2.0.12", "@img/sharp-wasm32": "0.35.4", "@modelcontextprotocol/sdk": "1.30.0", @@ -82,7 +82,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/scripts/bundled-diffr.integration.mjs b/packages/review/scripts/bundled-diffr.integration.mjs index cc1ff0fee..47b310c21 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 { 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, @@ -20,9 +20,27 @@ let structuralDiff, readDiffrConfig, setDiffrConfigValue, saveDiffrSummarizer, - testDiffrSummarizer; - -const source = path.resolve(import.meta.dirname, "../bin/diffr"); + testDiffrSummarizer, + migrateDiffrConfig; + +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`, + ), +); + +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( @@ -37,7 +55,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, @@ -100,7 +118,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: { @@ -129,6 +147,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", ({ structuralDiff } = await load("structural-diff")); ({ + migrateDiffrConfig, readDiffrConfig, setDiffrConfigValue, saveDiffrSummarizer, @@ -140,10 +159,10 @@ describe("Relocated runtime diffr integrates with Review streams and settings", { mode: 0o755 }, ); process.env.PATH = `${trap}${path.delimiter}${savedEnv.PATH}`; - delete process.env.REVIEW_DIFFR_BINARY; + process.env.REVIEW_DIFFR_BINARY = path.join(trap, "diffr"); }); - test("bundled binary streams added, modified and deleted files without using PATH", async () => { + test("bundled binary streams files and ignores host overrides", async () => { const events = await collect(repository, base, head); const files = successfulFiles(events, 3); assert.deepEqual(events[0].files.map(({ status }) => status).sort(), [ @@ -187,34 +206,89 @@ 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 migrateDiffrConfig(), true); + const config = await readDiffrConfig(repository); + assert.equal(config.values.version, 2); + assert.deepEqual(config.values.plugins.classify.bundled.hide, [ + "test", + "generated", + ]); + 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.bundled.summarize.system_prompt, + " Use my exact custom instructions.\nKeep # inside the prompt, and answer with JSON if I ask for it.\n", + ); + + const migrated = await readFile( + path.join(directory, "config.toml"), + "utf8", + ); + + assert.equal(await migrateDiffrConfig(), 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.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.bundled.context.lines", + "plugins.shape.bundled.context.lines", 7, repository, ); - assert.equal(updated.values.plugins.bundled.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.bundled.context.lines, + (await readDiffrConfig(repository)).values.plugins.shape.bundled.context + .lines, 7, ); assert.equal(existsSync(sentinel), false); }); - test("a provider switch saves through the binary, clears the old key and keeps the file sparse", async () => { + test("a provider switch saves through the binary and clears the old key", async () => { delete process.env.ANTHROPIC_API_KEY; await setDiffrConfigValue( - "plugins.bundled.summarize.api_key", + "plugins.shape.bundled.summarize.api_key", "old-secret", repository, ); @@ -235,15 +309,11 @@ 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.credentialSource, "missing"); - - const file = await readFile( - path.join(process.env.XDG_CONFIG_HOME, "diffr", "config.toml"), - "utf8", + assert.equal( + saved.values.plugins.shape.bundled.summarize.provider, + "anthropic", ); - - assert.doesNotMatch(file, /system_prompt/); + assert.equal(saved.credentialSource, "missing"); }); test("setup summarizes through a keyless OpenAI-compatible server", async () => { @@ -260,19 +330,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 } }] })); @@ -304,78 +362,19 @@ describe("Relocated runtime diffr integrates with Review streams and settings", server.close(); } }); - test("an explicit executable override takes precedence over the bundle", async () => { - const override = path.join(root, "override"); - await writeFile( - override, - `#!/bin/sh\necho explicit-override >&2\nexit 93\n`, - { mode: 0o755 }, - ); - process.env.REVIEW_DIFFR_BINARY = override; - await assert.rejects(collect(repository, base, head), /explicit-override/); - assert.equal(existsSync(sentinel), false); - }); - - 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 }); + test("a missing bundle fails without using PATH", async () => { + await rm(path.join(runtime, "node_modules"), { + recursive: true, + force: true, + }); await assert.rejects( collect(repository, base, head), - /diffr exited with 97/, + /The bundled diffr executable is missing/, ); - assert.equal(existsSync(sentinel), true); - }); - - describe("a working host installation coexists with the Desktop bundle", () => { - let called, launcher; - beforeAll(async () => { - await stageDiffrBinary(runtime, source); - const host = path.join(root, "host"); - const traced = path.join(root, "traced-host"); - called = path.join(root, "host-called"); - await Promise.all([mkdir(host), mkdir(traced)]); - const binary = path.join(host, "diffr"); - await copyFile(source, binary); - launcher = path.join(traced, "diffr"); - await writeFile( - launcher, - `#!${process.execPath} -const { appendFileSync } = require("node:fs"); -const { spawnSync } = require("node:child_process"); -appendFileSync(${JSON.stringify(called)}, "called\\n"); -const result = spawnSync(${JSON.stringify(binary)}, process.argv.slice(2), { stdio: "inherit" }); -process.exit(result.status ?? 1); -`, - { mode: 0o755 }, - ); - process.env.PATH = `${traced}${path.delimiter}${host}${path.delimiter}${savedEnv.PATH}`; - }); - - test("prefers the bundle over a working host binary", async () => { - delete process.env.REVIEW_DIFFR_BINARY; - successfulFiles(await collect(repository, base, head), 3); - await readDiffrConfig(repository); - assert.equal(existsSync(called), false); - }); - - test("an explicit host override runs real diffs and settings", async () => { - process.env.REVIEW_DIFFR_BINARY = launcher; - successfulFiles(await collect(repository, base, head), 3); - assert.equal(existsSync(called), true); - await rm(called); - await readDiffrConfig(repository); - assert.equal(existsSync(called), true); - await rm(called); - }); - - 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 }); - successfulFiles(await collect(repository, base, head), 3); - assert.equal(existsSync(called), true); - await rm(called); - await readDiffrConfig(repository); - assert.equal(existsSync(called), true); - }); + await assert.rejects( + readDiffrConfig(repository), + /The bundled diffr executable is missing/, + ); + assert.equal(existsSync(sentinel), false); }); }); 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 9c62c0887..70dcdd4ab 100644 --- a/packages/review/src/server/desktop-server.ts +++ b/packages/review/src/server/desktop-server.ts @@ -57,12 +57,12 @@ import { z } from "zod"; import { aliasInstallationToAccount } from "./account-alias"; import { CrashReportRequestSchema, reportCrashDump } from "./crash-report"; import { + migrateDiffrConfig, readDiffrConfig, saveDiffrSummarizer, setDiffrConfigValue, testDiffrSummarizer, } from "./diffr-config"; -import { installFullDiffr } from "./diffr-languages.js"; import { GlobalReviewDesktopVerbRelay, type ReviewDesktopVerbRelay, @@ -397,18 +397,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()), @@ -580,6 +568,7 @@ export function createGlobalReviewServer( return urlForBoundPort(); }, listen: async () => { + await migrateDiffrConfig(); scratchpadEnabled = await readScratchpadEnabled(); boundPort = await listen(httpServer, input.port); discovery.url = urlForBoundPort(); diff --git a/packages/review/src/server/diffr-config.test.ts b/packages/review/src/server/diffr-config.test.ts index 4699803f7..92dac60c5 100644 --- a/packages/review/src/server/diffr-config.test.ts +++ b/packages/review/src/server/diffr-config.test.ts @@ -1,8 +1,10 @@ -import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; +import * as diffr from "@dev.fast/diffr"; import type { JsonObject } from "@dev.fast/json"; +import { parse, stringify } from "smol-toml"; import { afterEach, beforeEach, expect, test, vi } from "vitest"; import { @@ -20,12 +22,12 @@ beforeEach(() => { vi.stubEnv("GOOGLE_API_KEY", ""); vi.stubEnv("OPENAI_API_KEY", ""); vi.stubEnv("ANTHROPIC_API_KEY", ""); - vi.stubEnv("MISTRAL_API_KEY", ""); vi.clearAllMocks(); }); afterEach(async () => { vi.unstubAllEnvs(); + vi.restoreAllMocks(); await Promise.all( roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), ); @@ -33,78 +35,6 @@ afterEach(async () => { const DEFAULT_PROMPT = "Summarize each fold."; -// What diffr's schema says about each provider. "mistral" exists only here: -// Whiteboard must handle a provider it has never heard of. -const PROVIDERS = [ - [ - "gemini", - "Gemini", - "gemini-3.8-flash", - "https://generativelanguage.googleapis.com", - ["GEMINI_API_KEY", "GOOGLE_API_KEY"], - false, - ], - [ - "openai", - "OpenAI", - "gpt-6-luna", - "https://api.openai.com/v1", - ["OPENAI_API_KEY"], - true, - ], - [ - "anthropic", - "Anthropic", - "claude-haiku-4-5", - "https://api.anthropic.com", - ["ANTHROPIC_API_KEY"], - false, - ], - [ - "mistral", - "Mistral", - "mistral-small", - "https://api.mistral.ai/v1", - ["MISTRAL_API_KEY"], - false, - ], -] as const; - -const SUMMARIZE_SCHEMA = { - type: "object", - properties: { - provider: { - enum: PROVIDERS.map(([id]) => id), - "x-enum-titles": PROVIDERS.map(([, title]) => title), - }, - model: { - type: "string", - "x-default-by": { - key: "provider", - values: Object.fromEntries( - PROVIDERS.map(([id, , model]) => [id, model]), - ), - }, - }, - provider_details: { - type: "object", - "x-default-by": { - key: "provider", - values: Object.fromEntries( - PROVIDERS.map(([id, , , endpoint, keys, keyless]) => [ - id, - { endpoint, key_variables: keys, keyless_custom_endpoint: keyless }, - ]), - ), - }, - }, - system_prompt: { - type: "string", - default: DEFAULT_PROMPT, - }, - }, -}; - const draft = { enabled: true, provider: "gemini" as const, @@ -114,29 +44,41 @@ const draft = { tests: true, }; -async function fakeDiffr(key = "", summarize: JsonObject = {}) { +async function diffrTestProcess( + key = "", + summarize: JsonObject = {}, + classifier?: JsonObject, +) { const root = await mkdtemp(path.join(tmpdir(), "review-diffr-config-")); roots.push(root); const log = path.join(root, "calls.jsonl"), - state = path.join(root, "state.json"), + state = path.join(root, "xdg", "diffr", "config.toml"), file = path.join(root, "diffr"); + const binary = diffr.diffrBinaryPath(); + await mkdir(path.dirname(state), { recursive: true }); + await writeFile( state, - JSON.stringify({ + stringify({ + version: 2, plugins: { - bundled: { - summarize: { - enabled: false, - provider: "gemini", - model: "old", - tests: false, - system_prompt: DEFAULT_PROMPT, - ...(key && { api_key: key }), - ...summarize, + ...(classifier && { classify: { bundled: classifier } }), + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + 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 }, }, }, }), @@ -146,30 +88,23 @@ async function fakeDiffr(key = "", summarize: JsonObject = {}) { `#!${process.execPath} const fs = require('node:fs'); const args = process.argv.slice(2); -const state = ${JSON.stringify(state)}, log = ${JSON.stringify(log)}; +const log = ${JSON.stringify(log)}; fs.appendFileSync(log, JSON.stringify(args) + '\\n'); -const config = JSON.parse(fs.readFileSync(state, 'utf8')); -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 } } } } } })); -} else if (args[0] === 'config' && args[1] === 'show') { - if (!args.includes('--reveal') && config.plugins.bundled.summarize.api_key) config.plugins.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); } - 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. - if (process.env.FAKE_RESET && args[2].endsWith('.provider') && object.provider !== value) { - for (const option of ['api_key', 'endpoint', 'model']) delete object[option]; +if (args[0] === 'config') { + if (args[1] === 'schema' && process.env.FAIL_SCHEMA) process.exit(2); + if (args[1] === 'show' && process.env.TEST_VALUES) { + console.log(fs.readFileSync(process.env.TEST_VALUES, 'utf8')); + process.exit(0); } - object[keys.at(-1)] = value; - fs.writeFileSync(state, JSON.stringify(config)); + if (args[1] === 'set' && process.env.FAIL_SET) { + console.error(fs.readFileSync(0, 'utf8')); + console.log(JSON.stringify({error:{message:'diffr rejected the change'}})); + process.exit(2); + } + const result = require('node:child_process').spawnSync(${JSON.stringify(binary)}, args, {stdio:'inherit'}); + process.exit(result.status ?? 1); } 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,14 +116,16 @@ 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})); } } `, { mode: 0o755 }, ); - vi.stubEnv("REVIEW_DIFFR_BINARY", file); + vi.spyOn(diffr, "diffrBinaryPath").mockReturnValue(file); + + vi.stubEnv("XDG_CONFIG_HOME", path.join(root, "xdg")); return { root, @@ -201,8 +138,18 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { }; } +async function savedConfig(file: string) { + return parse(await readFile(file, "utf8")) as { + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr config key. + shape: { bundled: Record }; + classify: { bundled: JsonObject }; + }; + }; +} + test("reads resolved values and reports credentials without exposing keys", async () => { - await fakeDiffr("saved-secret"); + await diffrTestProcess("saved-secret"); vi.stubEnv("GEMINI_API_KEY", "env-secret"); const config = await readDiffrConfig(); expect(config.credentialSource).toBe("config"); @@ -211,83 +158,147 @@ test("reads resolved values and reports credentials without exposing keys", asyn ); }); +test("redacts credentials in bundled and nested plugin values", async () => { + const fixture = await diffrTestProcess("bundled-secret"); + const values = path.join(fixture.root, "values.json"); + await writeFile( + values, + JSON.stringify({ + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr config key. + shape: { + bundled: { summarize: { api_key: "bundled-secret" } }, + custom: { api_key: "custom-secret", enabled: true }, + }, + }, + }), + ); + vi.stubEnv("TEST_VALUES", values); + + const config = await readDiffrConfig(); + expect(JSON.stringify(config)).not.toMatch( + /bundled-secret|custom-secret|api_key/, + ); + expect(config.values).toMatchObject({ + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr config key. + plugins: { shape: { custom: { enabled: true } } }, + }); +}); + test("detects host environment credentials and missing credentials", async () => { - await fakeDiffr(); + await diffrTestProcess(); expect((await readDiffrConfig()).credentialSource).toBe("missing"); vi.stubEnv("GOOGLE_API_KEY", "environment-secret"); expect((await readDiffrConfig()).credentialSource).toBe("environment"); }); +test("reads classifier values and schema defaults without old plugin keys", async () => { + await diffrTestProcess( + "saved-secret", + {}, + { hide: ["test", "custom"], hide_deleted: false }, + ); + const config = await readDiffrConfig(); + expect(config.values).toMatchObject({ + plugins: { + classify: { bundled: { 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 diffrTestProcess( + "saved-secret", + {}, + { hide: ["custom"], hide_deleted: true }, + ); + + expect( + (await setDiffrConfigValue("plugins.classify.bundled.hide", ["custom"])) + .changed, + ).toBe(false); + await setDiffrConfigValue("plugins.classify.bundled.hide", []); + await setDiffrConfigValue("plugins.classify.bundled.hide_deleted", false); + const disabled = await savedConfig(fake.state); + expect(disabled.plugins.classify.bundled).toEqual({ + hide: [], + hide_deleted: false, + }); + await setDiffrConfigValue("plugins.classify.bundled.hide", ["custom"]); + const enabled = await savedConfig(fake.state); + expect(enabled.plugins.classify.bundled).toEqual({ + hide: ["custom"], + hide_deleted: false, + }); + expect(enabled.plugins.shape.bundled.summarize.api_key).toBe("saved-secret"); + expect(enabled.plugins.shape.bundled["hide-files"]).toBeUndefined(); +}); + test("writes a setting, rereads it, and avoids invalidation for a no-op", async () => { - await fakeDiffr(); + await diffrTestProcess(); expect( - (await setDiffrConfigValue("plugins.bundled.context.lines", 3)).changed, + (await setDiffrConfigValue("plugins.shape.bundled.context.lines", 3)) + .changed, ).toBe(false); - const result = await setDiffrConfigValue("plugins.bundled.context.lines", 8); + + const result = await setDiffrConfigValue( + "plugins.shape.bundled.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: { plugins: { shape: { bundled: { context: { lines: 8 } } } } }, }); }); -test("saves credentials and options before enabling and preserves blank keys", async () => { - const fake = await fakeDiffr(); +test("saves credentials and options together and preserves blank keys", async () => { + const fake = await diffrTestProcess(); await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); - 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}`, - ), - ); await saveDiffrSummarizer({ ...draft, apiKey: "" }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize - .api_key, + (await savedConfig(fake.state)).plugins.shape.bundled.summarize.api_key, ).toBe("test-secret"); }); -test("disables first and serializes concurrent saves", async () => { - const fake = await fakeDiffr("test-secret"); +test("serializes concurrent batch saves", async () => { + const fake = await diffrTestProcess("test-secret"); await saveDiffrSummarizer(draft); await Promise.all([ saveDiffrSummarizer({ ...draft, enabled: false, model: "next" }), - setDiffrConfigValue("plugins.bundled.context.lines", 9), + setDiffrConfigValue("plugins.shape.bundled.context.lines", 9), ]); - const writes = (await fake.calls()) - .filter((args) => args[1] === "set") - .slice(-3); - - expect(writes.map((args) => args[2])).toEqual([ - "plugins.bundled.summarize.enabled", - "plugins.bundled.summarize.model", - "plugins.bundled.context.lines", - ]); + expect((await savedConfig(fake.state)).plugins.shape.bundled).toMatchObject({ + summarize: { model: "next", api_key: "test-secret" }, + context: { lines: 9 }, + }); + expect((await readDiffrConfig()).values).toMatchObject({ + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr config key. + plugins: { shape: { bundled: { summarize: { enabled: false } } } }, + }); }); -test("partial failure returns current values and invalidates without leaking the key", async () => { - await fakeDiffr(); - vi.stubEnv("FAIL_KEY", "plugins.bundled.summarize.model"); +test("a rejected batch preserves current values without leaking the key", async () => { + await diffrTestProcess(); + vi.stubEnv("FAIL_SET", "1"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); - expect(result.changed).toBe(true); - expect(result.error).toContain("Some settings were saved"); + expect(result.changed).toBe(false); + expect(result.error).toContain("diffr rejected the change"); expect(result.values).toMatchObject({ - plugins: { bundled: { 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.bundled.summarize.api_key"); - const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); - expect(result.changed).toBe(false); - expect(result.error).toBeDefined(); - expect(JSON.stringify(result)).not.toMatch(/test-secret|command leaked/); -}); - test("cannot enable summaries without credentials", async () => { - await fakeDiffr(); + await diffrTestProcess(); expect(await saveDiffrSummarizer(draft)).toMatchObject({ changed: false, error: expect.stringContaining("API key"), @@ -295,7 +306,7 @@ test("cannot enable summaries without credentials", async () => { }); test("synthetic test returns a summary without saving and cleans its config", async () => { - const fake = await fakeDiffr("saved-secret"); + const fake = await diffrTestProcess("saved-secret"); const before = await readFile(fake.state, "utf8"); expect(await testDiffrSummarizer(draft)).toBe("count positive values"); expect(await readFile(fake.state, "utf8")).toBe(before); @@ -308,7 +319,7 @@ test("synthetic test returns a summary without saving and cleans its config", as }); test("rejected credentials and empty summaries are safe failures with cleanup", async () => { - const fake = await fakeDiffr(); + const fake = await diffrTestProcess(); vi.stubEnv("TEST_MODE", "reject"); await expect( testDiffrSummarizer({ ...draft, apiKey: "test-secret" }), @@ -323,7 +334,7 @@ test("rejected credentials and empty summaries are safe failures with cleanup", }); test("a cancelled test cleans temporary files and releases the single-test guard", async () => { - const fake = await fakeDiffr("test-secret"); + const fake = await diffrTestProcess("test-secret"); vi.stubEnv("TEST_MODE", "hang"); const controller = new AbortController(); @@ -359,12 +370,14 @@ test("rejects malformed keys and reports missing executables", async () => { await expect(async () => setDiffrConfigValue("--flag", true)).rejects.toThrow( "Invalid diffr config key", ); - vi.stubEnv("REVIEW_DIFFR_BINARY", "/nonexistent/diffr"); - await expect(readDiffrConfig()).rejects.toThrow("Cannot find diffr"); + vi.spyOn(diffr, "diffrBinaryPath").mockReturnValue("/nonexistent/diffr"); + await expect(readDiffrConfig()).rejects.toThrow( + "The bundled diffr executable is missing", + ); }); test("saved changes invalidate cached comparisons while no-op saves reuse them", async () => { - const fake = await fakeDiffr(); + const fake = await diffrTestProcess(); const cache = new StructuralComparisons(); const input = { @@ -380,12 +393,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("plugins.shape.bundled.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("plugins.shape.bundled.context.lines", 8); await consume(); expect( (await fake.calls()).filter((args) => args[0] === "--repo"), @@ -395,35 +408,29 @@ test("saved changes invalidate cached comparisons while no-op saves reuse them", } }); -test("reads the default prompt from diffr's schema", async () => { - await fakeDiffr(); - expect(await readDiffrConfig()).toMatchObject({ - defaultPrompt: DEFAULT_PROMPT, - }); -}); - test("saves a changed prompt and leaves an unchanged one alone", async () => { - const fake = await fakeDiffr("test-secret"); + const fake = await diffrTestProcess("test-secret"); await saveDiffrSummarizer({ ...draft, enabled: false }); expect( - (await fake.calls()).some((args) => args[2]?.endsWith(".system_prompt")), - ).toBe(false); + (await savedConfig(fake.state)).plugins.shape.bundled.summarize + .system_prompt, + ).toBe(DEFAULT_PROMPT); await saveDiffrSummarizer({ ...draft, enabled: false, systemPrompt: "Be terse.", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize + (await savedConfig(fake.state)).plugins.shape.bundled.summarize .system_prompt, ).toBe("Be terse."); }); test("switching providers clears the saved key unless a new one is entered", async () => { - const fake = await fakeDiffr("gemini-secret"); + const fake = await diffrTestProcess("gemini-secret"); const state = async () => - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize; + (await savedConfig(fake.state)).plugins.shape.bundled.summarize; await saveDiffrSummarizer({ ...draft, @@ -444,7 +451,7 @@ test("switching providers clears the saved key unless a new one is entered", asy }); test("credentials are checked against the draft provider", async () => { - await fakeDiffr("gemini-secret"); + await diffrTestProcess("gemini-secret"); expect( await saveDiffrSummarizer({ ...draft, provider: "anthropic" }), ).toMatchObject({ @@ -467,7 +474,7 @@ test("credentials are checked against the draft provider", async () => { }); test("the synthetic test never sends a saved key to another provider and uses the draft prompt", async () => { - const fake = await fakeDiffr("gemini-secret"); + const fake = await diffrTestProcess("gemini-secret"); await expect( testDiffrSummarizer({ ...draft, provider: "openai" }), ).rejects.toThrow("Add an API key"); @@ -488,7 +495,7 @@ test("the synthetic test never sends a saved key to another provider and uses th }); test("reads the environment key of the saved provider", async () => { - await fakeDiffr(); + await diffrTestProcess(); await saveDiffrSummarizer({ ...draft, enabled: false, @@ -500,49 +507,18 @@ test("reads the environment key of the saved provider", async () => { expect((await readDiffrConfig()).credentialSource).toBe("environment"); }); -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"); - - const result = await saveDiffrSummarizer({ - ...draft, - provider: "openai", - apiKey: "openai-secret", - }); - - expect(result.error).toBeDefined(); - - const state = JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled - .summarize; - - // The failure left the old provider, and no key for it to send. - expect(state).toMatchObject({ provider: "gemini", api_key: "" }); -}); - test("a new endpoint is treated like a new provider", async () => { - const fake = await fakeDiffr("saved-secret"); + const fake = await diffrTestProcess("saved-secret"); const moved = { ...draft, endpoint: "https://proxy.example/v1" }; 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, + (await savedConfig(fake.state)).plugins.shape.bundled.summarize, ).toMatchObject({ api_key: "", endpoint: "https://proxy.example/v1" }); }); -test("a switch with no saved key writes no empty key", async () => { - const fake = await fakeDiffr(); - await saveDiffrSummarizer({ - ...draft, - enabled: false, - provider: "anthropic", - }); - expect( - (await fake.calls()).some((args) => args[2]?.endsWith(".api_key")), - ).toBe(false); -}); - test("settings still read when diffr describes no schema", async () => { - await fakeDiffr(); + await diffrTestProcess(); vi.stubEnv("FAIL_SCHEMA", "1"); const config = await readDiffrConfig(); expect(config.defaultPrompt).toBeUndefined(); @@ -550,7 +526,7 @@ test("settings still read when diffr describes no schema", async () => { }); test("a saved prompt is compared as written and a blank one is left alone", async () => { - const fake = await fakeDiffr("", { system_prompt: "Custom.\n" }); + const fake = await diffrTestProcess("", { system_prompt: "Custom.\n" }); await saveDiffrSummarizer({ ...draft, enabled: false, @@ -558,71 +534,7 @@ test("a saved prompt is compared as written and a blank one is left alone", asyn }); await saveDiffrSummarizer({ ...draft, enabled: false, systemPrompt: "" }); expect( - (await fake.calls()).some((args) => args[2]?.endsWith(".system_prompt")), - ).toBe(false); -}); - -test("providers, their titles, models and keys all come from diffr's schema", async () => { - await fakeDiffr(); - const config = await readDiffrConfig(); - expect(config.providers).toContainEqual({ - id: "mistral", - title: "Mistral", - model: "mistral-small", - endpoint: "https://api.mistral.ai/v1", - keyVariables: ["MISTRAL_API_KEY"], - keylessCustomEndpoint: false, - }); - expect( - (await saveDiffrSummarizer({ ...draft, provider: "mistral" })).error, - ).toContain("API key"); - vi.stubEnv("MISTRAL_API_KEY", "mistral-env"); - expect( - (await saveDiffrSummarizer({ ...draft, provider: "mistral" })).error, - ).toBeUndefined(); -}); - -test("the synthetic test lets diffr derive the draft provider's details", async () => { - const fake = await fakeDiffr("", { - provider_details: { - endpoint: "https://generativelanguage.googleapis.com", - key_variables: ["GEMINI_API_KEY"], - keyless_custom_endpoint: false, - }, - }); - - vi.stubEnv("ANTHROPIC_API_KEY", "env-anthropic"); - await testDiffrSummarizer({ ...draft, provider: "anthropic" }); - expect( - await readFile(path.join(fake.root, "test-config"), "utf8"), - ).not.toContain("provider_details"); -}); - -test("a custom endpoint kept across a provider switch survives diffr clearing it", async () => { - const fake = await fakeDiffr(); - vi.stubEnv("FAKE_RESET", "1"); - - const gateway = { - ...draft, - enabled: false, - endpoint: "https://gateway.example/v1", - }; - - await saveDiffrSummarizer({ - ...gateway, - provider: "anthropic", - apiKey: "anthropic-key", - }); - await saveDiffrSummarizer({ - ...gateway, - provider: "openai", - apiKey: "openai-key", - }); - expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize, - ).toMatchObject({ - provider: "openai", - endpoint: "https://gateway.example/v1", - api_key: "openai-key", - }); + (await savedConfig(fake.state)).plugins.shape.bundled.summarize + .system_prompt, + ).toBe("Custom.\n"); }); diff --git a/packages/review/src/server/diffr-config.ts b/packages/review/src/server/diffr-config.ts index c7e137875..d0cd1af77 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,29 +37,91 @@ function serialized(operation: () => Promise): Promise { return next; } +type DiffrArguments = + | { command: "schema" } + | { command: "show"; reveal: boolean } + | { command: "set"; patch: JsonObject } + | { command: "migrate" } + | { 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", "--stdin", "--json"]; + case "migrate": + return ["config", "migrate", "--json"]; + 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 { + const executable = diffrExecutable(); + try { - const { stdout } = await execFileAsync(diffrExecutable(), args, { + const operation = execFileAsync(executable, diffrArgv(args), { cwd: rootPath, maxBuffer: 16 * 1024 * 1024, signal: options.signal ?? AbortSignal.timeout(30_000), env: options.env ?? process.env, }); + if (args.command === "set") + operation.child.stdin?.end(JSON.stringify(args.patch)); + const { stdout } = await operation; + return stdout; } catch (error) { - // SAFETY: execFile rejects with an Error carrying its exit or spawn code. + // SAFETY: execFile adds the exit or spawn code and stdout to its Error. // Do not forward its message: it includes command arguments. - const failure = error as NodeJS.ErrnoException; + const failure = error as NodeJS.ErrnoException & { stdout?: string }; if (failure.code === "ENOENT") throw diffrMissingError(); if (failure.name === "AbortError") throw new Error("diffr timed out or was cancelled."); + + if ( + (args.command === "set" || args.command === "migrate") && + failure.stdout + ) { + let details: JsonValue | undefined; + + try { + details = parseJsonText(failure.stdout); + } catch { + /* Do not echo raw output in parse errors. */ + } + + if ( + isJsonObject(details) && + isJsonObject(details.error) && + isStringValue(details.error.message) + ) { + const { message, repair_prompt: prompt } = details.error; + throw new Error( + isStringValue(prompt) + ? `${message}\n\nCopy this as a prompt to your coding agent:\n${prompt}` + : message, + ); + } + } + throw new Error( "diffr could not complete the operation. Check its configuration and credentials.", ); @@ -70,13 +133,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 +157,7 @@ function valueAt(object: JsonObject, key: string): JsonValue | undefined { return value; } -const prefix = "plugins.bundled.summarize"; +const prefix = "plugins.shape.bundled.summarize"; function savedProvider(config: JsonObject): string { const value = valueAt(config, `${prefix}.provider`); @@ -108,20 +165,16 @@ function savedProvider(config: JsonObject): string { return isStringValue(value) ? value : ""; } -function savedEndpoint(config: JsonObject): string { - const value = valueAt(config, `${prefix}.endpoint`); - - return isStringValue(value) ? value : ""; -} - /** Whether a saved key belongs to a different destination than the draft's. */ function movesKey( config: JsonObject, draft: Pick, ): boolean { + const endpoint = valueAt(config, `${prefix}.endpoint`); + return ( draft.provider !== savedProvider(config) || - draft.endpoint !== savedEndpoint(config) + draft.endpoint !== (isStringValue(endpoint) ? endpoint : "") ); } @@ -151,27 +204,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.plugins.properties.shape.properties.bundled.properties.summarize.properties", ); - if (!isJsonObject(options)) return {}; + const defaultHiddenTags = strings( + valueAt( + schema, + "properties.plugins.properties.classify.properties.bundled.properties.hide.default", + ), + ); + + if (!isJsonObject(options)) return { defaultHiddenTags }; const option = (name: string) => isJsonObject(options[name]) ? options[name] : {}; @@ -200,6 +261,7 @@ async function summaryDefaults( const prompt = option("system_prompt"); return { + defaultHiddenTags, providers, defaultPrompt: isStringValue(prompt.default) ? prompt.default : undefined, }; @@ -207,7 +269,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 +279,15 @@ async function read(rootPath?: string): Promise { ? "environment" : "missing"; - const plugins = config.plugins; + const plugins = valueAt(config, "plugins.shape"); if (isJsonObject(plugins)) { - for (const namespace of Object.values(plugins)) { - if (!isJsonObject(namespace)) continue; + const entries = isJsonObject(plugins.bundled) + ? [...Object.values(plugins), ...Object.values(plugins.bundled)] + : Object.values(plugins); - for (const plugin of Object.values(namespace)) { - if (isJsonObject(plugin)) delete plugin.api_key; - } - } + for (const plugin of entries) + if (isJsonObject(plugin)) delete plugin.api_key; } return { values: config, credentialSource, ...defaults }; @@ -236,35 +297,35 @@ export function readDiffrConfig(rootPath?: string): Promise { return serialized(() => read(rootPath)); } -export function diffrConfigValueText(value: JsonValue): string { - return isStringValue(value) ? value : JSON.stringify(value); +export async function migrateDiffrConfig( + signal?: AbortSignal, +): Promise { + const result = json( + await diffr({ command: "migrate" }, undefined, { signal }), + ); + + if (result.changed === true) invalidateStructuralComparisons(); + + return result.changed === true; } async function writeSettings( - entries: [string, JsonValue][], + patch: JsonObject, rootPath?: string, ): Promise { let changed = false; let error: string | undefined; - let current = await values(rootPath); try { - for (const [key, value] of entries) { - if (valueAt(current, key) === value) continue; - await diffr( - ["config", "set", key, diffrConfigValueText(value)], - rootPath, - ); - changed = true; - // A write can clear others, such as a provider's endpoint and key. - current = await values(rootPath); - } - } catch { - error = changed - ? "Some settings were saved before diffr rejected a change. Check the current values and try again." - : "diffr could not save the setting. Check its configuration and the entered value."; - } finally { + const result = json(await diffr({ command: "set", patch }, rootPath)); + changed = result.changed === true; + if (changed) invalidateStructuralComparisons(); + } catch (cause) { + error = + cause instanceof Error + ? cause.message + : "diffr could not save the settings."; } return { ...(await read(rootPath)), changed, error }; @@ -275,13 +336,16 @@ 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."); } - return serialized(() => writeSettings([[key, value]], rootPath)); + const parts = key.split("."); + let patch: JsonObject = { [parts.pop()!]: value }; + + for (const part of parts.reverse()) patch = { [part]: patch }; + + return serialized(() => writeSettings(patch, rootPath)); } export function saveDiffrSummarizer( @@ -313,40 +377,24 @@ export function saveDiffrSummarizer( }; } - const entries: [string, JsonValue][] = []; - - if (!draft.enabled) entries.push([`${prefix}.enabled`, false]); - - // Clear a key before its destination changes and write a new one only - // after, so a failure part way never pairs a key with another vendor. - if (moving && savedKey) entries.push([`${prefix}.api_key`, ""]); - entries.push([`${prefix}.provider`, draft.provider]); + const summarize: JsonObject = { + enabled: draft.enabled, + provider: draft.provider, + endpoint: draft.endpoint, + model: draft.model, + tests: draft.tests, + }; - // diffr clears a provider's endpoint when the provider changes, so a - // custom one kept across a switch is written again. - if ( - draft.endpoint !== savedEndpoint(current.values) || - (draft.endpoint && draft.provider !== savedProvider(current.values)) - ) - entries.push([`${prefix}.endpoint`, draft.endpoint]); + if (draft.apiKey) summarize.api_key = draft.apiKey; + else if (moving && savedKey) summarize.api_key = ""; - if (draft.apiKey) entries.push([`${prefix}.api_key`, draft.apiKey]); + if (draft.systemPrompt.trim()) summarize.system_prompt = draft.systemPrompt; - // A blank prompt leaves diffr's own in place. - if ( - draft.systemPrompt.trim() && - draft.systemPrompt !== valueAt(current.values, `${prefix}.system_prompt`) - ) - entries.push([`${prefix}.system_prompt`, draft.systemPrompt]); - - entries.push( - [`${prefix}.model`, draft.model], - [`${prefix}.tests`, draft.tests], + return writeSettings( + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr config key. + { plugins: { shape: { bundled: { summarize } } } }, + rootPath, ); - - if (draft.enabled) entries.push([`${prefix}.enabled`, true]); - - return writeSettings(entries, rootPath); }); } @@ -373,7 +421,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 +458,13 @@ export async function testDiffrSummarizer( await writeFile( join(directory, "diffr", "config.toml"), stringify({ - version: 1, + version: 2, plugins: { - order: ["bundled.summarize"], - bundled: { summarize: options }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { + order: ["bundled.summarize"], + bundled: { summarize: options }, + }, }, }), { mode: 0o600 }, @@ -425,56 +476,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-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; -} 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..8b0005201 --- /dev/null +++ b/packages/review/src/server/diffr-startup.test.ts @@ -0,0 +1,100 @@ +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 keeps invalid config, migrates before ready, and does not rewrite 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)); + + const original = + "version = 1\n[plugins.bundled.context]\nlines = 17\nenabled = false\n"; + + const ready = async () => { + expect(parse(await readFile(file, "utf8"))).toMatchObject({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { bundled: { 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 }); + }; + + const invalid = "[invalid TOML"; + await writeFile(file, invalid); + await expect(start()).rejects.toThrow(/TOML/); + expect(await readFile(file, "utf8")).toBe(invalid); + + await writeFile(file, original); + await start(); + const migrated = await readFile(file, "utf8"); + await start(); + expect(await readFile(file, "utf8")).toBe(migrated); + }, +); diff --git a/packages/review/src/server/headless-host.ts b/packages/review/src/server/headless-host.ts index 34815d86b..ba98d88ff 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 { migrateDiffrConfig } from "./diffr-config.js"; import { GlobalReviewDesktopVerbRelay } from "./global-verb-relay.js"; import { createNodeRequestListener } from "./hono-http.js"; import { @@ -72,6 +73,8 @@ export function withHeadlessServerLock( async function serve(input: HeadlessServerInput) { if (input.signal.aborted) return; + await migrateDiffrConfig(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..eed45ece2 100644 --- a/packages/review/src/server/structural-diff-workspace.test.ts +++ b/packages/review/src/server/structural-diff-workspace.test.ts @@ -3,6 +3,7 @@ import { mkdir, mkdtemp, realpath, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; +import * as diffr from "@dev.fast/diffr"; import { createReviewApi as createJsonReviewApi } from "@review/review-api/http.js"; import { openLocalReviewStore } from "@review/review-api/local-data.js"; import { afterEach, expect, test, vi } from "vitest"; @@ -11,6 +12,7 @@ const roots: string[] = []; afterEach(async () => { vi.unstubAllEnvs(); + vi.restoreAllMocks(); await Promise.all( roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), ); @@ -73,18 +75,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.spyOn(diffr, "diffrBinaryPath").mockReturnValue(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 9ed12d9f4..5771603e5 100644 --- a/packages/review/src/server/structural-diff.test.ts +++ b/packages/review/src/server/structural-diff.test.ts @@ -1,21 +1,20 @@ -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 * as diffr from "@dev.fast/diffr"; 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, - diffrExecutable, - structuralDiff, -} from "./structural-diff"; +import { type StructuralDiffRequest, structuralDiff } from "./structural-diff"; const roots: string[] = []; afterEach(async () => { vi.useRealTimers(); vi.unstubAllEnvs(); + vi.restoreAllMocks(); await Promise.all( roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), ); @@ -26,14 +25,15 @@ async function executable(script: string) { roots.push(root); const file = path.join(root, "diffr"); await writeFile(file, `#!${process.execPath}\n${script}`, { mode: 0o755 }); - vi.stubEnv("REVIEW_DIFFR_BINARY", file); + vi.spyOn(diffr, "diffrBinaryPath").mockReturnValue(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: [ @@ -96,7 +96,6 @@ test.each(["trees", "merge-base"] as const)( root, "--format", "ndjson", - "--stream-annotations", ...(kind === "trees" ? ["base", "head"] : ["base...head"]), "--", "space name.ts", @@ -169,8 +168,12 @@ test.each([ test("reports launch failures", async () => { const root = await executable(""); - vi.stubEnv("REVIEW_DIFFR_BINARY", path.join(root, "missing")); - await expect(collect(request(root))).rejects.toThrow("Cannot find diffr"); + vi.spyOn(diffr, "diffrBinaryPath").mockReturnValue( + path.join(root, "missing"), + ); + await expect(collect(request(root))).rejects.toThrow( + "The bundled diffr executable is missing", + ); }); test("streams a large comparison without treating accumulated file bytes as one record", async () => { @@ -189,7 +192,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 } }, }, @@ -322,70 +325,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("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..482300968 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,17 @@ 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); +export function diffrExecutable(): string { + const executable = diffrBinaryPath(); - if (full) return full; + if (!executable) throw diffrMissingError(); - const bundled = path.join( - packageRoot, - "bin", - process.platform === "win32" ? "diffr.exe" : "diffr", - ); - - return existsSync(bundled) ? bundled : "diffr"; + return executable; } 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.`, + "The bundled diffr executable is missing. Reinstall Whiteboard.", ); } @@ -59,15 +45,10 @@ export async function* structuralDiff( input: StructuralDiffRequest, ): AsyncGenerator { input.signal.throwIfAborted(); + const executable = diffrExecutable(); 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": @@ -88,11 +69,9 @@ export async function* structuralDiff( // The host inherits its own environment and runs from the repository, so // diffr reads the user's config and keys exactly as it would from a shell. - console.info( - `[Review] structural diff: ${diffrExecutable()} ${args.join(" ")}`, - ); + console.info(`[Review] structural diff: ${executable} ${args.join(" ")}`); - const child = spawn(diffrExecutable(), args, { + const child = spawn(executable, args, { cwd: input.repositoryPath, stdio: ["ignore", "pipe", "pipe"], signal, @@ -120,7 +99,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 { @@ -152,8 +130,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}`); } @@ -168,13 +144,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/vitest.config.ts b/packages/review/vitest.config.ts index 2ea18dcf4..4b05f0f81 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. @@ -27,6 +31,7 @@ export default defineConfig({ name: "shared-module-graph", environment: "node", isolate: false, + server: { deps: { inline: ["@dev.fast/diffr"] } }, exclude: [...configDefaults.exclude, "app/**"], // Integration cases can exceed Vitest's // 5 second default while sharing a two-core hosted runner. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index dbf356132..a997ed7c8 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -6,6 +6,9 @@ settings: catalogs: default: + '@dev.fast/diffr': + specifier: 0.1.14 + version: 0.1.14 zod: specifier: 4.6.5 version: 4.6.5 @@ -51,8 +54,8 @@ importers: version: link:../../packages/review devDependencies: '@dev.fast/diffr': - specifier: 0.1.10 - version: 0.1.10(zod@4.6.5) + specifier: 'catalog:' + version: 0.1.14(zod@4.6.5) '@dev.fast/review-canvas': specifier: workspace:* version: link:../../packages/review/app @@ -119,6 +122,9 @@ importers: '@agentclientprotocol/sdk': specifier: 1.5.1 version: 1.5.1(zod@4.6.5) + '@dev.fast/diffr': + specifier: 'catalog:' + version: 0.1.14(zod@4.6.5) '@hono/node-server': specifier: 2.0.12 version: 2.0.12(hono@4.13.5) @@ -168,9 +174,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 +220,8 @@ importers: packages/review-protocol: dependencies: '@dev.fast/diffr': - specifier: 0.1.10 - version: 0.1.10(zod@4.6.5) + specifier: 'catalog:' + version: 0.1.14(zod@4.6.5) '@dev.fast/json': specifier: workspace:* version: link:../json @@ -554,10 +557,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.14': + resolution: {integrity: sha512-BTCbL50aBYDxwdngcSdSY04YpUQfWdCDTRHXSdKFaPMN0Qxmgo7NW9ioVKIQtF8+WZ9H3b9uJqKsl/vrrObggA==} + cpu: [arm64] + os: [darwin] + + '@dev.fast/diffr-darwin-x64@0.1.14': + resolution: {integrity: sha512-IZ9/mXCjXcQIgBaOkofTVI35a1CnsUhwwls8h9yE5cmRRbSdTw/kW2Md6tH8B5qAHbFRvvNw42I9FGrAhvooAw==} + cpu: [x64] + os: [darwin] + + '@dev.fast/diffr-linux-arm64@0.1.14': + resolution: {integrity: sha512-xX/DyUB/VnjA5I1j8ZASypDaNUry7clr3F+wg4yTXiW68GgTSAvmkJHlM+vaeyTLzsorNftclAsgzetwd0Ppvw==} + cpu: [arm64] + os: [linux] + libc: [glibc] + + '@dev.fast/diffr-linux-x64@0.1.14': + resolution: {integrity: sha512-DOMo2qQ8P9qIl6w3PaUaTTgGEtRzZtYmimr4R5m66jV9mASLpTZhSj4VIwft+occgBgy1Pg1ADILUYUhKZdMhw==} + cpu: [x64] + os: [linux] + libc: [glibc] + + '@dev.fast/diffr-win32-x64@0.1.14': + resolution: {integrity: sha512-VlLq9ro8oJKG/vb9L5gM+Ktq/YKZH+OJFB6+ulAJm9+HtrijMKTIgz+czXU4nR7+zcifA77DHxq1HJnmveMgqQ==} + cpu: [x64] + os: [win32] + + '@dev.fast/diffr@0.1.14': + resolution: {integrity: sha512-sD+Yo2Y1/subZm1t8JT/vZ4Spxti8tqXDFiT6mzKDlXZv+xZHG8Pt7McRdnv52Ptn5SBKH5f0FSlJYxFXXV+4A==} engines: {node: '>=20'} - hasBin: true peerDependencies: zod: ^4.0.0 @@ -3426,9 +3455,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.14': + optional: true + + '@dev.fast/diffr-darwin-x64@0.1.14': + optional: true + + '@dev.fast/diffr-linux-arm64@0.1.14': + optional: true + + '@dev.fast/diffr-linux-x64@0.1.14': + optional: true + + '@dev.fast/diffr-win32-x64@0.1.14': + optional: true + + '@dev.fast/diffr@0.1.14(zod@4.6.5)': dependencies: zod: 4.6.5 + optionalDependencies: + '@dev.fast/diffr-darwin-arm64': 0.1.14 + '@dev.fast/diffr-darwin-x64': 0.1.14 + '@dev.fast/diffr-linux-arm64': 0.1.14 + '@dev.fast/diffr-linux-x64': 0.1.14 + '@dev.fast/diffr-win32-x64': 0.1.14 '@dual-bundle/import-meta-resolve@4.2.1': {} diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index ed99b113c..3655b76e1 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -5,6 +5,7 @@ packages: - "apps/*" catalog: + '@dev.fast/diffr': 0.1.14 zod: "4.6.5" catalogMode: strict @@ -17,7 +18,12 @@ verifyDepsBeforeRun: false minimumReleaseAge: 10080 minimumReleaseAgeStrict: true minimumReleaseAgeExclude: - - "@dev.fast/diffr@0.1.10" + - "@dev.fast/diffr@0.1.14" + - "@dev.fast/diffr-darwin-arm64@0.1.14" + - "@dev.fast/diffr-darwin-x64@0.1.14" + - "@dev.fast/diffr-linux-arm64@0.1.14" + - "@dev.fast/diffr-linux-x64@0.1.14" + - "@dev.fast/diffr-win32-x64@0.1.14" # 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"