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/review/common/reviewStructuralDiff.test.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts index 0af2fd2a5..e83efa78e 100644 --- a/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewStructuralDiff.test.ts @@ -30,10 +30,10 @@ function leaf(id: number, start: number, end: number, extra: Partial(); const pairs = (left: StructuralRegion, right: StructuralRegion) => { diff --git a/apps/review-desktop/code-oss/src/vs/review/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/reviewStructuralDiffSession.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.test.ts index 16d6b83eb..b183fd5ee 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.test.ts @@ -1,13 +1,34 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { StructuralDiffSession } from './reviewStructuralDiffSession.js'; -import type { StructuralEvent } from '../common/reviewStructuralDiff.js'; +import { STRUCTURAL_WIRE_VERSION, type StructuralEvent } from '../common/reviewStructuralDiff.js'; const file = { rhs: { path: 'a.ts', oid: 'abc', mode: '100644' } }; -const start: StructuralEvent = { type: 'start', version: 4, lhs: { type: 'empty_tree' }, rhs: { type: 'revision', rev: 'head' }, files: [{ file, status: 'added' }] }; -const result: StructuralEvent = { type: 'file', file, diff: { type: 'text', structural_changes: { base: [], head: [[0, 1]] }, rhs: { text: 'a', regions: [{ kind: 'leaf', id: 1, fold_state_id: 2, alignment_id: 1, start: { line: 0, column: 0 }, end: { line: 0, column: 1 }, visibility: { collapsed: true } }] }, stats: { textual: { added: 1, removed: 0 }, visible: { added: 0, removed: 0 } } } }; +const start: StructuralEvent = { type: 'start', version: STRUCTURAL_WIRE_VERSION, lhs: { type: 'empty_tree' }, rhs: { type: 'revision', rev: 'head' }, files: [{ file, status: 'added' }] }; +const result: StructuralEvent = { type: 'file', file, diff: { type: 'text', structural_changes: { base: [], head: [[0, 1]] }, rhs: { text: 'a', root: { kind: 'leaf', id: 1, fold_state_id: 2, alignment_id: 1, start: { line: 0, column: 0 }, end: { line: 0, column: 1 }, visibility: { collapsed: true } } }, stats: { textual: { added: 1, removed: 0 }, visible: { added: 0, removed: 0 } } } }; const complete: StructuralEvent = { type: 'complete', succeeded: 1, failed: 0 }; +test('file results supply root visibility and summary labels before comparison completion', async () => { + let release!: () => void; + const gate = new Promise(resolve => release = resolve); + const labeled = structuredClone(result); + if (labeled.type !== 'file' || labeled.diff?.type !== 'text') throw new Error('Invalid fixture'); + labeled.diff.rhs!.root.visibility = { collapsed: true, label: 'Generated file' }; + const session = new StructuralDiffSession({ async *streamComparison() { yield start; yield labeled; await gate; yield complete; } }); + const label = session.regionLabel('a.ts', 1); + const task = session.start(); + try { + const fileResult = await session.fileResult('a.ts'); + assert.equal(fileResult.hidden, 'Generated file'); + assert.equal(label.get(), 'Generated file'); + assert.equal(session.complete, false); + session.setRegionCollapsed('a.ts', 2, false); + release(); await task; + assert.equal(session.isRegionCollapsed('a.ts', 2), false); + assert.equal(session.error, undefined); + } finally { release(); session.dispose(); await task; } +}); + test('views detach and reattach without restarting a comparison or losing folds', async () => { let requests = 0; const session = new StructuralDiffSession({ async *streamComparison() { requests++; yield start; yield result; yield complete; } }); @@ -61,117 +82,6 @@ test('truncation and missing manifest results are visible to later subscribers', } }); -test('deferred annotations retain source identity, counts and user fold choices', async () => { - let release!: () => void; - const gate = new Promise(resolve => release = resolve); - const session = new StructuralDiffSession({ - async *streamComparison() { - yield start; yield structuredClone(result); - await gate; - yield { type: 'annotations', file, annotations: [{ region_id: 1, label: 'summarized' }] } as StructuralEvent; - yield complete; - } - }); - const task = session.start(); - while (!session.getTextDiff('a.ts')) await new Promise(resolve => setTimeout(resolve, 0)); - try { - const initial = session.getTextDiff('a.ts')!; - assert.equal(session.complete, false); - session.setRegionCollapsed('a.ts', 2, false); - release(); await task; - assert.equal(session.getTextDiff('a.ts'), initial); - assert.equal(initial.rhs!.regions![0].visibility?.label, 'summarized'); - assert.equal(initial.rhs!.text, 'a'); - assert.deepEqual(initial.structural_changes, { base: [], head: [[0, 1]] }); - assert.equal(session.isRegionCollapsed('a.ts', 2), false); - assert.equal(session.error, undefined); - } finally { release(); session.dispose(); } -}); - -test('invalid annotation batches are atomic and enrichment failure keeps the initial diff usable', async () => { - for (const invalid of [false, true]) { - const session = new StructuralDiffSession({ - async *streamComparison() { - yield start; yield structuredClone(result); - yield { - type: 'annotations', file, - annotations: invalid ? [{ region_id: 1, label: 'must not apply' }, { region_id: 99, label: 'bad' }] : [], - error: invalid ? undefined : { code: 'enrichment_failed', message: 'offline' }, - } as StructuralEvent; - yield complete; - } - }); - try { - await session.start(); - assert.equal(session.getTextDiff('a.ts')!.rhs!.regions![0].visibility?.label, undefined); - assert.equal(session.isRegionCollapsed('a.ts', 2), true); - if (invalid) assert.match(session.error!, /Unknown annotation region/); - else { - assert.equal(session.getFileResult('a.ts')?.annotationError, 'offline'); - assert.equal(session.error, undefined); - } - } finally { session.dispose(); } - } -}); - -test('streamed labels update existing bands without invalidating either file provider', async () => { - const { StructuralDiffProvider } = await import('./reviewStructuralDiff.js'); - const { URI } = await import('../../base/common/uri.js'); - const { CancellationToken } = await import('../../base/common/cancellation.js'); - const other = { rhs: { path: 'b.ts', oid: 'def', mode: '100644' } }; - let release!: () => void; - const gate = new Promise(resolve => release = resolve); - const session = new StructuralDiffSession({ - async *streamComparison() { - yield { ...start, files: [...start.files, { file: other, status: 'added' as const }] }; - yield structuredClone(result); - yield { ...structuredClone(result), file: other }; - await gate; - yield { type: 'annotations', file, annotations: [{ region_id: 1, label: 'first' }] } as StructuralEvent; - yield { type: 'annotations', file, annotations: [{ region_id: 1, label: '// pseudocode\nfinal summary' }] } as StructuralEvent; - yield { ...complete, succeeded: 2 }; - } - }); - const task = session.start(); - const ready = new Promise(resolve => { - const sub = session.onDidChange(change => { if (change.files.has('b.ts')) { sub.dispose(); resolve(); } }); - }); - try { - await ready; - const models = (path: string) => [ - { uri: URI.parse(`test:/base/${path}`), getLinesContent: () => [''] }, - { uri: URI.parse(`test:/head/${path}`), getLinesContent: () => ['a'] }, - ] as const; - const a = models('a.ts'), b = models('b.ts'); - const pairs = new Map([[`${a[0].uri}\n${a[1].uri}`, 'a.ts'], [`${b[0].uri}\n${b[1].uri}`, 'b.ts']]); - const pa = new StructuralDiffProvider(session, pairs, new Set()); - const pb = new StructuralDiffProvider(session, pairs, new Set()); - const compute = (provider: typeof pa, pair: ReturnType) => provider.computeDiff( - pair[0] as Parameters[0], pair[1] as Parameters[1], - { ignoreTrimWhitespace: false, maxComputationTimeMs: 0, computeMoves: false }, CancellationToken.None, - ); - const diff = await compute(pa, a); - await compute(pb, b); - let invalidationsA = 0, invalidationsB = 0, labelBatches = 0; - const subscriptions = [pa.onDidChange(() => invalidationsA++), pb.onDidChange(() => invalidationsB++), - session.onDidChange(change => { if (change.labels.has('a.ts')) labelBatches++; })]; - const band = diff.contextGaps![0]; - release(); await task; - assert.equal(band.labelObservable!.get(), '// pseudocode\nfinal summary'); - assert.equal(invalidationsA, 0); - assert.equal(invalidationsB, 0); - assert.equal(labelBatches, 1, 'a burst publishes only the latest presentation state'); - const folded = new Promise(resolve => { - const sub = pa.onDidChange(() => { sub.dispose(); resolve(); }); - }); - session.setRegionCollapsed('a.ts', 2, false); - await folded; - assert.equal(invalidationsA, 1); - assert.equal(invalidationsB, 0, 'folding a file does not invalidate unrelated editors'); - subscriptions.forEach(sub => sub.dispose()); - } finally { release(); session.dispose(); await task; } -}); - test('structural editor models retain diffr bytes across a live file save', async () => { const { createStructuralDiffEditors, StructuralDiffProvider } = await import('./reviewStructuralDiff.js'); const { URI } = await import('../../base/common/uri.js'); @@ -188,7 +98,7 @@ test('structural editor models retain diffr bytes across a live file save', asyn yield { ...start, files: [{ file: changedFile, status: 'modified' }] } as StructuralEvent; await gate; yield { type: 'file', file: changedFile, diff: { - type: 'text', lhs: { text: 'base\n' }, rhs: { text: 'saved A\n' }, + type: 'text', lhs: { text: 'base\n', root: {kind:'leaf',id:3,fold_state_id:3,alignment_id:1,start:{line:0,column:0},end:{line:1,column:0}} }, rhs: { text: 'saved A\n', root: {kind:'leaf',id:4,fold_state_id:4,alignment_id:1,start:{line:0,column:0},end:{line:1,column:0}} }, structural_changes: { base: [[0, 1]], head: [[0, 1]] }, stats: { textual: { added: 1, removed: 1 }, visible: { added: 1, removed: 1 } }, } } as StructuralEvent; diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.ts index 77a58274c..b64888fa0 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiffSession.ts @@ -1,5 +1,5 @@ import { RunOnceScheduler } from "../../base/common/async.js"; -import { observableValue, transaction, type IObservable, type ISettableObservable } from "../../base/common/observable.js"; +import { observableValue, type IObservable, type ISettableObservable } from "../../base/common/observable.js"; import { Emitter } from "../../base/common/event.js"; import { Disposable } from "../../base/common/lifecycle.js"; import { CancellationError } from "../../base/common/errors.js"; @@ -11,14 +11,11 @@ export interface StructuralFileResult { diff?: StructuralDiff; error?: string; hidden?: string; - annotationError?: string; } export interface StructuralSessionChange { /** Files whose diff or fold state changed. */ readonly files: ReadonlySet; - /** Files with new summary labels or summary errors; their diff is unchanged. */ - readonly labels: ReadonlySet; readonly status: boolean; } @@ -27,11 +24,9 @@ export class StructuralDiffSession extends Disposable { private readonly changed = this._register(new Emitter()); readonly onDidChange = this.changed.event; private readonly pendingFiles = new Set(); - private readonly pendingLabels = new Set(); private pendingStatus = false; private readonly regions = new Map(); private readonly labels = new Map>(); - private readonly pendingLabelValues = new Map(); private readonly notification = this._register(new RunOnceScheduler(() => this.flushChanges(), 16)); private readonly abort = new AbortController(); private readonly results = new Map(); @@ -107,14 +102,10 @@ export class StructuralDiffSession extends Disposable { if (event.type !== "start" || event.version !== STRUCTURAL_WIRE_VERSION) throw new Error("Unsupported diffr stream protocol."); this.acceptManifest(event); } else if (event.type === "file") this.storeFileResult(event); - else if (event.type === "annotations") this.applyAnnotations(event); else if (event.type === "complete") this.finishLoading(event); else throw new Error(`Unexpected diffr event: ${event.type}`); if (event.type === "file") this.notify(structuralFilePath(event.file)); - else if (event.type === "annotations") { - this.pendingLabels.add(structuralFilePath(event.file)); - this.notify(); - } else this.notify(undefined, true); + else this.notify(undefined, true); } /** Stable presentation state, shared by all editors displaying this region. */ @@ -135,15 +126,11 @@ export class StructuralDiffSession extends Disposable { } private flushChanges(): void { - if (!this.pendingFiles.size && !this.pendingLabels.size && !this.pendingStatus) return; + if (!this.pendingFiles.size && !this.pendingStatus) return; const change: StructuralSessionChange = { - files: new Set(this.pendingFiles), labels: new Set(this.pendingLabels), status: this.pendingStatus, + files: new Set(this.pendingFiles), status: this.pendingStatus, }; - const labels = new Map(this.pendingLabelValues); - this.pendingFiles.clear(); this.pendingLabels.clear(); this.pendingLabelValues.clear(); this.pendingStatus = false; - transaction(tx => { - for (const [key, value] of labels) this.labels.get(key)?.set(value, tx); - }); + this.pendingFiles.clear(); this.pendingStatus = false; this.changed.fire(change); } @@ -160,33 +147,17 @@ export class StructuralDiffSession extends Disposable { if (diff.type === "text") { const seed = (region: StructuralRegion) => { this.regions.set(`${path}:${region.id}`, region); + this.labels.get(`${path}:${region.id}`)?.set(region.visibility?.label, undefined); const key = `${path}:${region.fold_state_id}`; if (!this.folds.has(key)) this.folds.set(key, region.visibility?.collapsed === true); if (region.kind === "fold") region.children.forEach(seed); }; - for (const side of [diff.lhs, diff.rhs]) side?.regions?.forEach(seed); - } - this.results.set(path, { - diff, - hidden: event.visibility?.collapsed ? event.visibility.label || "Hidden by default" : undefined - }); - } - - private applyAnnotations(event: Extract): void { - const path = structuralFilePath(event.file); - const result = this.results.get(path); - if (result?.diff?.type !== "text") throw new Error(`Annotations precede a text result: ${path}`); - // Validate the entire batch before applying it. Labels do not replace the - // source, correspondence, region identities, or the reader's fold choices. - for (const annotation of event.annotations) { - if (!this.regions.has(`${path}:${annotation.region_id}`)) throw new Error(`Unknown annotation region: ${annotation.region_id}`); - } - for (const annotation of event.annotations) { - const region = this.regions.get(`${path}:${annotation.region_id}`)!; - region.visibility = { ...region.visibility, label: annotation.label }; - this.pendingLabelValues.set(`${path}:${region.id}`, annotation.label); + for (const side of [diff.lhs, diff.rhs]) if (side) seed(side.root); } - result.annotationError = event.error?.message; + const visibility = diff.type === "text" + ? [diff.rhs, diff.lhs].find(side => side?.root.visibility?.collapsed)?.root.visibility + : undefined; + this.results.set(path, { diff, hidden: visibility ? visibility.label || "Hidden by default" : undefined }); } private finishLoading(event: Extract): void { @@ -203,8 +174,6 @@ export class StructuralDiffSession extends Disposable { this.labels.clear(); this.regions.clear(); this.pendingFiles.clear(); - this.pendingLabels.clear(); - this.pendingLabelValues.clear(); super.dispose(); } } diff --git a/apps/review-desktop/package.json b/apps/review-desktop/package.json index 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..fb05d9d60 100644 --- a/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/diffr-settings.mjs @@ -54,7 +54,7 @@ function testFolds(events) { return events .filter((event) => event.type === "file") - .flatMap((event) => walk(event.diff?.rhs?.regions)) + .flatMap((event) => walk(event.diff?.rhs ? [event.diff.rhs.root] : [])) .filter( (region) => region.tags?.includes("test-bodies:test") && @@ -117,8 +117,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 e8c60791d..9de0a7e23 100644 --- a/apps/review-desktop/scripts/linux/nixos/vm-test.nix +++ b/apps/review-desktop/scripts/linux/nixos/vm-test.nix @@ -15,7 +15,7 @@ let targetPkgs = p: package.runtimePackages p ++ [ p.unzip p.file p.binutils p.cargo p.rustc ]; runScript = pkgs.writeShellScript "probe-installed-whiteboard" '' set -eu - ${raw}/resources/app/review-runtime/bin/diffr --version + ${raw}/resources/app/review-runtime/node_modules/@dev.fast/diffr-linux-*/diffr --version mkdir -p "$HOME/rust-extension" unzip -qo ${rustVsix} -d "$HOME/rust-extension" chmod +x "$HOME/rust-extension/extension/server/rust-analyzer" diff --git a/apps/review-desktop/scripts/lsp-e2e.mjs b/apps/review-desktop/scripts/lsp-e2e.mjs index cb37f91b3..4798c0874 100644 --- a/apps/review-desktop/scripts/lsp-e2e.mjs +++ b/apps/review-desktop/scripts/lsp-e2e.mjs @@ -38,9 +38,11 @@ const { values } = parseArgs({ options: { app: { type: "string" }, keep: { type: "boolean" } }, }); +const diffrPackage = `node_modules/@dev.fast/diffr-${process.platform}-${process.arch}/diffr`; + const bundledDiffr = values.app - ? path.join(values.app, "Contents/Resources/app/review-runtime/bin/diffr") - : path.join(workspace, "packages/review/bin/diffr"); + ? path.join(values.app, "Contents/Resources/app/review-runtime", diffrPackage) + : path.join(workspace, diffrPackage); const structuralDiffAvailable = !!process.env.REVIEW_DIFFR_BINARY || diff --git a/apps/review-desktop/scripts/package-linux.sh b/apps/review-desktop/scripts/package-linux.sh index f00ec8ecc..c473fd2ab 100755 --- a/apps/review-desktop/scripts/package-linux.sh +++ b/apps/review-desktop/scripts/package-linux.sh @@ -42,7 +42,6 @@ node "$APP_DIR/scripts/curated-extensions.mjs" \ # The installed app embeds its own Review server runtime (server, CLI, and # docs) so it never reaches back into this checkout. pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard build -pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard ensure:diffr --required node "$APP_DIR/scripts/stage-review-runtime.mjs" --packaged-root "$PACKAGED_ROOT" node "$APP_DIR/scripts/stage-review-runtime.mjs" --verify --packaged-root "$PACKAGED_ROOT" diff --git a/apps/review-desktop/scripts/package-macos.sh b/apps/review-desktop/scripts/package-macos.sh index 82583d3fc..bf9af5f7c 100755 --- a/apps/review-desktop/scripts/package-macos.sh +++ b/apps/review-desktop/scripts/package-macos.sh @@ -79,8 +79,6 @@ node "$APP_DIR/scripts/curated-extensions.mjs" \ # # build.sh produces the server dist in local mode. In precompiled mode, the # server dist arrives in the darwin payload. -# Fetch on Darwin: precompiled payloads were built on Linux. -pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard ensure:diffr --required node "$APP_DIR/scripts/stage-review-runtime.mjs" --packaged-root "$PACKAGED_APP" # Installs Assets.car and rewrites CFBundleIconName, which invalidates any diff --git a/apps/review-desktop/scripts/package-windows.sh b/apps/review-desktop/scripts/package-windows.sh index 1f60a339a..4990f04ac 100644 --- a/apps/review-desktop/scripts/package-windows.sh +++ b/apps/review-desktop/scripts/package-windows.sh @@ -15,7 +15,6 @@ node "$APP_DIR/scripts/copy-canvas.mjs" --packaged-root "$PACKAGED_ROOT" node "$APP_DIR/scripts/curated-extensions.mjs" --target=win32-x64 --copy-to "$PACKAGED_ROOT/resources/app/extensions" # ty links the Visual C++ runtime dynamically and a clean Windows install has none, so ship it beside ty. cp "$(cygpath -u "$SYSTEMROOT")/System32/vcruntime140.dll" "$PACKAGED_ROOT/resources/app/extensions/astral-sh.ty/bundled/libs/bin/" -pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard ensure:diffr --required node "$APP_DIR/scripts/stage-review-runtime.mjs" --packaged-root "$PACKAGED_ROOT" node "$APP_DIR/scripts/stage-review-runtime.mjs" --verify --packaged-root "$PACKAGED_ROOT" npm --prefix "$CHECKOUT" run gulp -- vscode-win32-x64-inno-updater diff --git a/apps/review-desktop/scripts/run.sh b/apps/review-desktop/scripts/run.sh index 7bad815b0..b5e1fddf4 100755 --- a/apps/review-desktop/scripts/run.sh +++ b/apps/review-desktop/scripts/run.sh @@ -71,13 +71,6 @@ if [[ "$(cat "$EXTENSIONS_SELECTION_STAMP" 2>/dev/null)" != "$EXTENSIONS_SELECTI echo "$EXTENSIONS_SELECTION" >"$EXTENSIONS_SELECTION_STAMP" fi -# Development launches use the checkout runtime, not the staged release bundle. -# Check the pinned binary even when all compiled outputs are already current. -# An explicit developer override supplies its own executable. -if [[ -z "$PACKAGED_ROOT" && -z "${REVIEW_DIFFR_BINARY:-}" ]]; then - pnpm --dir "$MONOREPO_ROOT" --filter @dev.fast/whiteboard ensure:diffr --required -fi - rebuild_review_desktop_outputs "$MONOREPO_ROOT" "$REVIEW_PACKAGE" # Names this Desktop as the checkout's dev instance, packaged or not. export DEV_FAST_REVIEW_CHECKOUT="$MONOREPO_ROOT" diff --git a/apps/review-desktop/scripts/runtime-bundler.test.mjs b/apps/review-desktop/scripts/runtime-bundler.test.mjs index 20d9ba016..22106981b 100644 --- a/apps/review-desktop/scripts/runtime-bundler.test.mjs +++ b/apps/review-desktop/scripts/runtime-bundler.test.mjs @@ -13,13 +13,13 @@ import test from "node:test"; import { RUNTIME_CLI_ENTRY, + RUNTIME_DIFFR_ENTRY, assertNoRuntimeBundler, assertPackagedArtifacts, assertRuntimeClosure, assertRuntimeContents, requiredPackagedArtifacts, runtimeRootForPackagedRoot, - stageDiffrBinary, } from "./stage-review-runtime.mjs"; test("runtime closure rejects transitive esbuild packages and native binaries", async () => { @@ -131,7 +131,7 @@ test("final package verification requires the CLI and rechecks archives outside await mkdir(path.dirname(artifact), { recursive: true }); if (artifact === path.join(runtime, "node_modules")) { - await mkdir(artifact); + await mkdir(artifact, { recursive: true }); } else { await writeFile(artifact, artifact.endsWith(".json") ? "{}\n" : ""); } @@ -158,12 +158,12 @@ test("final package verification requires the CLI and rechecks archives outside await assertRuntimeClosure(runtime); await assertPackagedArtifacts(packaged); - const diffr = path.join(runtime, "bin", "diffr"); + const diffr = path.join(runtime, RUNTIME_DIFFR_ENTRY); await rm(diffr, { force: true }); - await assert.rejects(assertRuntimeClosure(runtime), /missing bin\/diffr/); + await assert.rejects(assertRuntimeClosure(runtime), /missing node_modules\/@dev\.fast\/diffr-/); await assert.rejects( assertPackagedArtifacts(packaged), - /missing .*bin\/diffr/, + /missing .*diffr-/, ); await writeFile(diffr, ""); @@ -188,31 +188,6 @@ test("final package verification requires the CLI and rechecks archives outside } }); -test("diffr staging rejects missing and stale downloads", async () => { - const root = await mkdtemp(path.join(os.tmpdir(), "review-diffr-stage-")); - const runtime = path.join(root, "review-runtime"); - const source = path.join(root, "diffr"); - - try { - await assert.rejects(stageDiffrBinary(runtime, source), /ensure:diffr/); - await writeFile(source, "#!/bin/sh\necho stale\n", { mode: 0o755 }); - await assert.rejects( - stageDiffrBinary(runtime, source), - /missing or not diffr/, - ); - await writeFile( - path.join(root, "diffr.stamp.json"), - JSON.stringify({ version: "0.0.0", target: "wrong-target" }), - ); - await assert.rejects( - stageDiffrBinary(runtime, source), - /missing or not diffr/, - ); - } finally { - await rm(root, { recursive: true, force: true }); - } -}); - function asarHeader(name) { const json = Buffer.from( JSON.stringify({ files: { [name]: { files: {} } } }), diff --git a/apps/review-desktop/scripts/smoke-windows.mjs b/apps/review-desktop/scripts/smoke-windows.mjs index e7d781f96..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..cdbb393ce 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,116 @@ 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("custom classifier controls write its own entry and keep its tags", async () => { + const { actions, current } = await mount(); + pluginSettings(current).classify = { + "team-classifier": { + path: "/plugins/team-classifier", + hide: ["team-generated"], + hide_deleted: true, + }, + }; + vi.mocked(actions.set).mockImplementation(async (key, value) => { + const [, , name, field] = key.split("."); + const classifiers = pluginSettings(current).classify as JsonObject; + const options = classifiers[name] as JsonObject; + + classifiers[name] = { ...options, [field]: value }; + + return { ...current, changed: true }; + }); + await open(); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(actions.set).toHaveBeenLastCalledWith( + "plugins.classify.team-classifier.hide", + [], + ); + await act(async () => page.getByLabelText("Hide files by tag").click()); + expect(actions.set).toHaveBeenLastCalledWith( + "plugins.classify.team-classifier.hide", + ["team-generated"], + ); + await act(async () => page.getByLabelText("Hide deleted files").click()); + expect(actions.set).toHaveBeenLastCalledWith( + "plugins.classify.team-classifier.hide_deleted", + false, + ); + expect(pluginSettings(current).classify).toEqual({ + "team-classifier": { + path: "/plugins/team-classifier", + hide: ["team-generated"], + hide_deleted: false, + }, + }); +}); + +test("custom classifiers only show the hide fields that they support", async () => { + const { actions, current } = await mount(); + pluginSettings(current).classify = { + "team-classifier": { path: "/plugins/team-classifier", hide_deleted: true }, + }; + await open(); + expect( + document.querySelector('input[aria-label="Hide files by tag"]'), + ).toBeNull(); + await act(async () => page.getByLabelText("Hide deleted files").click()); + expect(actions.set).toHaveBeenCalledWith( + "plugins.classify.team-classifier.hide_deleted", + false, + ); +}); + test("starts collapsed and reads only on first expansion", async () => { const { actions } = await mount(); expect(actions.read).not.toHaveBeenCalled(); @@ -144,7 +268,7 @@ test("writes the selected key and keeps reload visible when collapsed", async () await page.getByLabelText("Collapse test bodies").click(); }); expect(actions.set).toHaveBeenCalledWith( - "plugins.bundled.test-bodies.enabled", + "plugins.shape.bundled.test-bodies.enabled", false, ); await expect @@ -178,8 +302,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 +333,16 @@ test("tests draft settings without saving, then saves and clears the key", async provider: "gemini", model: "test-model", endpoint: "", - systemPrompt: "Default prompt.", + systemPrompt: "Keep my wording.", tests: true, apiKey: "test-secret", }); await expect .element(page.getByLabelText("API key", { exact: true })) .toHaveValue(""); + await expect + .element(page.getByLabelText("Prompt", { exact: true })) + .toHaveValue("Keep my wording."); }); test("confirms discarding unsaved summary edits before reloading", async () => { @@ -392,7 +525,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..edbb573ca 100644 --- a/packages/review/app/src/diffr-config-section.tsx +++ b/packages/review/app/src/diffr-config-section.tsx @@ -1,5 +1,6 @@ import { Button } from "@canvas/ui/button"; import { TextField, fieldStyles } from "@canvas/ui/text-field"; +import { isBooleanValue, isStringValue } from "@dev.fast/json"; import { type JsonValue, type ReviewDiffrConfig, @@ -13,30 +14,61 @@ import { type ReactNode, useEffect, useRef, useState } from "react"; import { Choice } from "./settings-choice"; import { settingsStyles as styles } from "./settings-styles"; -const displaySettings = [ - ["context.enabled", "Collapse unchanged lines"], - ["test-bodies.enabled", "Collapse test bodies"], - ["deleted-bodies.enabled", "Collapse deleted function bodies"], - ["removed-runs.enabled", "Collapse long removed stretches"], - ["group.enabled", "Group adjacent folds"], - ["hide-files.enabled", "Hide files by tag"], - ["hide-files.deleted", "Hide deleted files"], +const foldSettings = [ + ["plugins.shape.bundled.context.enabled", "Collapse unchanged lines"], + ["plugins.shape.bundled.test-bodies.enabled", "Collapse test bodies"], + [ + "plugins.shape.bundled.deleted-bodies.enabled", + "Collapse deleted function bodies", + ], + [ + "plugins.shape.bundled.removed-runs.enabled", + "Collapse long removed stretches", + ], ] as const; +type DisplaySetting = readonly [key: string, label: string]; + +function classifierPrefix(config: ReviewDiffrConfig): string | undefined { + const entries = setting(config, "plugins.classify"); + + if (!isJsonObject(entries)) return; + const names = Object.keys(entries); + + if (names.length === 1) return `plugins.classify.${names[0]}`; +} + +function displaySettings(config: ReviewDiffrConfig): DisplaySetting[] { + const settings: DisplaySetting[] = [...foldSettings]; + const prefix = classifierPrefix(config); + + if (!prefix) return settings; + + if (Array.isArray(setting(config, `${prefix}.hide`))) + settings.push([`${prefix}.hide`, "Hide files by tag"]); + + if (isBooleanValue(setting(config, `${prefix}.hide_deleted`))) + settings.push([`${prefix}.hide_deleted`, "Hide deleted files"]); + + return settings; +} + function setting( config: ReviewDiffrConfig, key: string, ): JsonValue | undefined { let value: JsonValue | undefined = config.values; - for (const part of `plugins.bundled.${key}`.split(".")) + for (const part of key.split(".")) value = isJsonObject(value) ? value[part] : undefined; return value; } function provider(config: ReviewDiffrConfig): string { - return String(setting(config, "summarize.provider") ?? ""); + return String( + setting(config, "plugins.shape.bundled.summarize.provider") ?? "", + ); } /** The provider diffr describes as `id`, if any. */ @@ -50,14 +82,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 +113,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 +125,7 @@ export function DiffrConfigSection({ actions.read().then( (result) => { if (cancelled) return; + rememberTags(result); setConfig(result); setDraft(summaryDraft(result)); }, @@ -121,7 +162,16 @@ export function DiffrConfigSection({ } } + function rememberTags(result: ReviewDiffrConfig) { + const prefix = classifierPrefix(result); + const tags = prefix ? setting(result, `${prefix}.hide`) : undefined; + + if (Array.isArray(tags) && tags.length) + rememberedTags.current = tags.filter(isStringValue); + } + function saved(result: ReviewDiffrConfig) { + rememberTags(result); setConfig(result); if (result.changed) setChanged(true); @@ -134,11 +184,15 @@ 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 classifier = config && classifierPrefix(config); + const hideKey = classifier ? `${classifier}.hide` : undefined; + + const hiddenTags = config && hideKey ? setting(config, hideKey) : undefined; return (
@@ -158,23 +212,30 @@ export function DiffrConfigSection({ {config && ( <>

Display

- {displaySettings.map(([key, label]) => ( + {displaySettings(config).map(([key, label]) => (
0 + : setting(config, key) === true + } disabled={busy || setting(config, key) === undefined} onChange={(event) => { - const value = event.target.checked; + const value = + key === hideKey + ? event.target.checked + ? (rememberedTags.current ?? + config.defaultHiddenTags ?? + []) + : [] + : event.target.checked; + void run(async () => - saved( - await actions.set( - `plugins.bundled.${key}`, - value, - ), - ), + saved(await actions.set(key, value)), ); }} /> @@ -184,16 +245,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 +266,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..8411ecae3 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", @@ -79,10 +79,10 @@ "semver": "7.8.5", "sharp": "0.35.4", "smol-toml": "1.8.0", + "toml-eslint-parser": "catalog:", "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..940d25fa7 100644 --- a/packages/review/scripts/bundled-diffr.integration.mjs +++ b/packages/review/scripts/bundled-diffr.integration.mjs @@ -1,15 +1,23 @@ import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; import { existsSync } from "node:fs"; -import { copyFile, mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { + copyFile, + cp, + mkdir, + readFile, + readdir, + 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 +28,43 @@ let structuralDiff, readDiffrConfig, setDiffrConfigValue, saveDiffrSummarizer, - testDiffrSummarizer; + testDiffrSummarizer, + ensureDiffrConfigMigrated, + diffrExecutable; + +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`, + ), +); + +// A local candidate lets this test run before its npm release is published. +const source = + process.env.REVIEW_DIFFR_TEST_BINARY || path.join(platformPackage, "diffr"); + +async function stageDiffr(runtime) { + for (const directory of [diffrPackage, platformPackage]) + await cp( + directory, + path.join(runtime, "node_modules/@dev.fast", path.basename(directory)), + { recursive: true, dereference: true }, + ); -const source = path.resolve(import.meta.dirname, "../bin/diffr"); + if (process.env.REVIEW_DIFFR_TEST_BINARY) + await copyFile( + source, + path.join( + runtime, + "node_modules/@dev.fast", + path.basename(platformPackage), + "diffr", + ), + ); +} async function collect(repositoryPath, base, head, paths, kind = "trees") { return Array.fromAsync( @@ -37,7 +79,7 @@ async function collect(repositoryPath, base, head, paths, kind = "trees") { function successfulFiles(events, count) { assert.equal(events[0].type, "start"); - assert.equal(events[0].version, 4); + assert.equal(events[0].version, 3); assert.deepEqual(events.at(-1), { type: "complete", succeeded: count, @@ -100,7 +142,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: { @@ -108,6 +150,10 @@ describe("Relocated runtime diffr integrates with Review streams and settings", import.meta.dirname, "../src/server/structural-diff.ts", ), + "diffr-config-migration": path.resolve( + import.meta.dirname, + "../src/server/diffr-config-migration.ts", + ), "diffr-config": path.resolve( import.meta.dirname, "../src/server/diffr-config.ts", @@ -127,7 +173,8 @@ describe("Relocated runtime diffr integrates with Review streams and settings", ).href ); - ({ structuralDiff } = await load("structural-diff")); + ({ structuralDiff, diffrExecutable } = await load("structural-diff")); + ({ ensureDiffrConfigMigrated } = await load("diffr-config-migration")); ({ readDiffrConfig, setDiffrConfigValue, @@ -187,25 +234,90 @@ describe("Relocated runtime diffr integrates with Review streams and settings", assert.deepEqual(events[0].files, []); }); + test("a saved UI config migrates through the staged binary with its overrides intact", async () => { + const previous = process.env.XDG_CONFIG_HOME; + process.env.XDG_CONFIG_HOME = path.join(root, "migration"); + const directory = path.join(process.env.XDG_CONFIG_HOME, "diffr"); + await mkdir(directory, { recursive: true }); + + const original = await readFile( + new URL( + "../src/server/test-fixtures/diffr-ui-overrides.toml", + import.meta.url, + ), + "utf8", + ); + + await writeFile(path.join(directory, "config.toml"), original, { + mode: 0o600, + }); + + try { + assert.equal(await ensureDiffrConfigMigrated(diffrExecutable()), true); + const config = await readDiffrConfig(repository); + assert.equal(config.values.version, 2); + assert.deepEqual(config.values.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 backup = (await readdir(directory)).find((name) => + name.includes("before-whiteboard-config-v2"), + ); + + assert.ok(backup); + assert.equal( + await readFile(path.join(directory, backup), "utf8"), + original, + ); + + const migrated = await readFile( + path.join(directory, "config.toml"), + "utf8", + ); + + assert.equal(await ensureDiffrConfigMigrated(diffrExecutable()), false); + await readDiffrConfig(repository); + assert.equal( + await readFile(path.join(directory, "config.toml"), "utf8"), + migrated, + ); + } finally { + process.env.XDG_CONFIG_HOME = previous; + } + }); + test("settings values and edits round-trip through the staged binary", async () => { const config = await readDiffrConfig(repository); - assert.ok(Number.isInteger(config.values.plugins.bundled.context.lines)); - assert.equal(config.values.plugins.bundled.summarize.api_key, undefined); + assert.ok( + Number.isInteger(config.values.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); @@ -214,7 +326,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("a provider switch saves through the binary, clears the old key and keeps the file sparse", async () => { delete process.env.ANTHROPIC_API_KEY; await setDiffrConfigValue( - "plugins.bundled.summarize.api_key", + "plugins.shape.bundled.summarize.api_key", "old-secret", repository, ); @@ -235,7 +347,10 @@ describe("Relocated runtime diffr integrates with Review streams and settings", ); assert.equal(saved.error, undefined); - assert.equal(saved.values.plugins.bundled.summarize.provider, "anthropic"); + assert.equal( + saved.values.plugins.shape.bundled.summarize.provider, + "anthropic", + ); assert.equal(saved.credentialSource, "missing"); const file = await readFile( @@ -260,19 +375,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 } }] })); @@ -318,7 +421,10 @@ describe("Relocated runtime diffr integrates with Review streams and settings", test("an unbundled installation falls back to PATH", async () => { delete process.env.REVIEW_DIFFR_BINARY; - await rm(path.join(runtime, "bin"), { recursive: true, force: true }); + await rm(path.join(runtime, "node_modules"), { + recursive: true, + force: true, + }); await assert.rejects( collect(repository, base, head), /diffr exited with 97/, @@ -329,7 +435,7 @@ describe("Relocated runtime diffr integrates with Review streams and settings", describe("a working host installation coexists with the Desktop bundle", () => { let called, launcher; beforeAll(async () => { - await stageDiffrBinary(runtime, source); + await stageDiffr(runtime); const host = path.join(root, "host"); const traced = path.join(root, "traced-host"); called = path.join(root, "host-called"); @@ -370,7 +476,10 @@ process.exit(result.status ?? 1); test("without a bundle the host binary runs real diffs and settings", async () => { delete process.env.REVIEW_DIFFR_BINARY; - await rm(path.join(runtime, "bin"), { recursive: true, force: true }); + await rm(path.join(runtime, "node_modules"), { + recursive: true, + force: true, + }); successfulFiles(await collect(repository, base, head), 3); assert.equal(existsSync(called), true); await rm(called); 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..907e133ca 100644 --- a/packages/review/src/server/desktop-server.ts +++ b/packages/review/src/server/desktop-server.ts @@ -62,7 +62,7 @@ import { setDiffrConfigValue, testDiffrSummarizer, } from "./diffr-config"; -import { installFullDiffr } from "./diffr-languages.js"; +import { ensureDiffrConfigMigrated } from "./diffr-config-migration.js"; import { GlobalReviewDesktopVerbRelay, type ReviewDesktopVerbRelay, @@ -74,6 +74,7 @@ import { reviewLifecycleTelemetry } from "./review-lifecycle-telemetry"; import { ReviewOpenWatchdog } from "./review-open-watchdog"; import { createWhiteboardCore, serverJson } from "./review-server-core"; import { invalidateStructuralComparisons } from "./structural-comparisons.js"; +import { diffrExecutable } from "./structural-diff.js"; import { createTutorialService } from "./tutorial-service"; import { captureSanitizedUiTelemetry } from "./ui-telemetry"; @@ -397,18 +398,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 +569,7 @@ export function createGlobalReviewServer( return urlForBoundPort(); }, listen: async () => { + await ensureDiffrConfigMigrated(diffrExecutable()); scratchpadEnabled = await readScratchpadEnabled(); boundPort = await listen(httpServer, input.port); discovery.url = urlForBoundPort(); diff --git a/packages/review/src/server/diffr-config-migration.test.ts b/packages/review/src/server/diffr-config-migration.test.ts new file mode 100644 index 000000000..e264a3a50 --- /dev/null +++ b/packages/review/src/server/diffr-config-migration.test.ts @@ -0,0 +1,692 @@ +import { + mkdir, + mkdtemp, + readFile, + readdir, + rm, + stat, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; + +import { parse } from "smol-toml"; +import { afterEach, expect, test } from "vitest"; +import { z } from "zod"; + +import { + ensureDiffrConfigMigrated, + migrateDiffrConfig, +} from "./diffr-config-migration"; +import { TomlEdits } from "./toml-edits"; + +const roots: string[] = []; + +test("rejects a change to a retained credential without exposing its value", () => { + const original = 'plugins.bundled.summarize.api_key = "saved-secret"\n'; + const edits = new TomlEdits(original); + edits.set("plugins.bundled.summarize.api_key", "different-secret"); + expect(() => + edits.assertUnchanged(original, [ + [ + "plugins.bundled.summarize.api_key", + "plugins.bundled.summarize.api_key", + ], + ]), + ).toThrow( + "diffr migration changed a retained setting: plugins.bundled.summarize.api_key", + ); +}); + +afterEach(async () => { + await Promise.all( + roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), + ); +}); + +test("converts a saved UI config without changing supported overrides or their comments", async () => { + const source = await readFile( + new URL("./test-fixtures/diffr-ui-overrides.toml", import.meta.url), + "utf8", + ); + + const migrated = migrateDiffrConfig(source); + const original = parse(source); + const expected = structuredClone(original); + + const plugins = z + .object({ + bundled: z.record( + z.string(), + z.record( + z.string(), + z.union([z.string(), z.boolean(), z.number(), z.array(z.string())]), + ), + ), + }) + .parse(expected.plugins); + + delete plugins.bundled.group; + delete plugins.bundled["hide-files"]; + delete plugins.bundled.summarize.max_concurrency; + expected.version = 2; + expected.plugins = { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { bundled: plugins.bundled }, + classify: { + bundled: { hide: ["test", "generated"], hide_deleted: false }, + }, + }; + expect(parse(migrated)).toEqual(expected); + + // Source spans for credentials, prompt, context and fold choices are not serialized. + for (const line of source + .split("\n") + .filter((line) => + /# (Collapse|Context|Enable|Provider|API key|Model|Endpoint|Include|Unrelated)/.test( + line, + ), + )) + expect(migrated).toContain(line); + expect(migrated).toContain( + "system_prompt = ''' Use my exact custom instructions.\nKeep # inside the prompt, and answer with JSON if I ask for it.\n''' # Prompt", + ); + expect(migrated).toContain("# Group adjacent folds; retired"); + expect(migrateDiffrConfig(migrated)).toBe(migrated); +}); + +test.each([ + [true, true, ["test", "generated"], true], + [true, false, ["test", "generated"], false], + [false, true, [], false], + [false, false, [], false], +])( + "preserves hiding for enabled=%s deleted=%s", + (enabled, deleted, hide, hideDeleted) => { + const migrated = migrateDiffrConfig( + `[plugins.bundled.hide-files]\nenabled = ${enabled}\ndeleted = ${deleted}\ntags = ["test", "generated"]\n`, + ); + + expect(parse(migrated).plugins).toEqual({ + classify: { + bundled: { + hide, + hide_deleted: hideDeleted, + }, + }, + }); + }, +); + +test.each([true, false])( + "removes retired group=%s without changing context", + (enabled) => { + const migrated = migrateDiffrConfig( + `[plugins.bundled.context]\nenabled = false\nlines = 9\n[plugins.bundled.group]\nenabled = ${enabled}\n`, + ); + + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { bundled: { context: { enabled: false, lines: 9 } } }, + }, + }); + }, +); + +test("handles quoted keys, inline tables and dotted keys without touching a custom prompt", () => { + const source = `version = 1\nplugins = { bundled = { group = { enabled = false }, "hide-files" = { enabled = false }, summarize = { system_prompt = "Keep me", max_concurrency = 2 } }, order = ["bundled.group", "bundled.hide-files", "bundled.summarize"] }\n`; + expect(parse(migrateDiffrConfig(source))).toEqual({ + version: 2, + plugins: { + classify: { bundled: { hide: [], hide_deleted: false } }, + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + bundled: { summarize: { system_prompt: "Keep me" } }, + order: ["bundled.summarize"], + }, + }, + }); + const dotted = `plugins.bundled.group.enabled = true\nplugins.bundled."hide-files".deleted = false\nclassifier = { hide = ["vendored"] }\n`; + expect(parse(migrateDiffrConfig(dotted)).plugins).toEqual({ + classify: { + bundled: { + hide: ["vendored"], + hide_deleted: false, + }, + }, + }); +}); + +test("retains explicit modern classifier values in a mixed config", () => { + const source = `[classifier]\nhide = ["custom"]\nhide_deleted = true\n[plugins.bundled.hide-files]\nenabled = false\n`; + expect(parse(migrateDiffrConfig(source))).toEqual({ + version: 2, + plugins: { + classify: { bundled: { hide: ["custom"], hide_deleted: true } }, + }, + }); +}); + +test("removes retired names from a custom order without sorting surviving plugins", () => { + const source = `[plugins]\norder = ["bundled.context", "bundled.group", "external.custom", "bundled.hide-files", "bundled.summarize"]\n`; + expect(parse(migrateDiffrConfig(source))).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + order: ["bundled.context", "custom", "bundled.summarize"], + }, + }, + }); +}); + +test("retains comments inside a rewritten plugin order", () => { + const source = `[plugins]\norder = [\n"bundled.context", # keep context first\n"bundled.group", # retired group\n"bundled.summarize",\n]\n`; + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { order: ["bundled.context", "bundled.summarize"] }, + }, + }); + expect(migrated.match(/# keep context first/g)).toHaveLength(1); + expect(migrated.match(/# retired group/g)).toHaveLength(1); +}); + +async function fixture() { + const root = await mkdtemp( + path.join(tmpdir(), "whiteboard-config-migration-"), + ); + + roots.push(root); + const file = path.join(root, "diffr", "config.toml"); + await mkdir(path.dirname(file)); + const executable = path.join(root, "diffr-cli"); + await writeFile( + executable, + `#!${process.execPath}\nconst fs = require('node:fs');\nconst path = require('node:path');\nif (process.argv[3] !== 'show') process.exit(3);\n{\n const candidate = fs.readFileSync(path.join(process.env.XDG_CONFIG_HOME,'diffr','config.toml'),'utf8');\n if (process.env.CAPTURE_CANDIDATE) fs.writeFileSync(process.env.CAPTURE_CANDIDATE, candidate);\n if (process.env.REJECT_CANDIDATE) { console.error('synthetic secret'); process.exit(2); }\n if (process.env.CONCURRENT_EDIT) fs.writeFileSync(${JSON.stringify(file)}, 'version = 1 # concurrent edit\\n');\n if (candidate.includes('plugins.bundled.group')) process.exit(2);\n console.log('{}');\n}\n`, + { mode: 0o755 }, + ); + + return { + root, + file, + executable, + env: { ...process.env, XDG_CONFIG_HOME: root }, + }; +} + +test("validates before writing, backs up privately and is a no-op on a second run", async () => { + const { root, file, executable, env } = await fixture(); + const source = `[plugins.bundled.group]\nenabled = false # retired\n`; + await writeFile(file, source); + expect(await ensureDiffrConfigMigrated(executable, env)).toBe(true); + const migrated = await readFile(file, "utf8"); + expect(parse(migrated)).toEqual({ version: 2 }); + + const backup = (await readdir(path.dirname(file))).find((name) => + name.includes(".before-whiteboard-config-v2."), + )!; + + expect(await readFile(path.join(path.dirname(file), backup), "utf8")).toBe( + source, + ); + + expect(await ensureDiffrConfigMigrated(executable, env)).toBe(false); + expect(await readFile(file, "utf8")).toBe(migrated); + expect( + (await readdir(path.join(root, "diffr"))).some((name) => + name.startsWith(".whiteboard-config-"), + ), + ).toBe(false); +}); + +test.skipIf(process.platform === "win32")( + "keeps backup credentials private", + async () => { + const { file, executable, env } = await fixture(); + await writeFile(file, `[plugins.bundled.group]\nenabled = false\n`); + await ensureDiffrConfigMigrated(executable, env); + + const backup = (await readdir(path.dirname(file))).find((name) => + name.includes(".before-whiteboard-config-v2."), + )!; + + expect( + (await stat(path.join(path.dirname(file), backup))).mode & 0o777, + ).toBe(0o600); + }, +); + +test("does not run the binary or change a version 2 file", async () => { + const { file, env } = await fixture(); + + const source = + "version = 2 # already migrated\n[plugins.shape.bundled.context]\nlines = 8\n"; + + await writeFile(file, source); + expect(await ensureDiffrConfigMigrated("/no/binary/needed", env)).toBe(false); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); + +test("keeps an unknown config version unchanged", async () => { + const { file, env } = await fixture(); + const source = "version = 3\n"; + await writeFile(file, source); + await expect( + ensureDiffrConfigMigrated("/no/binary/needed", env), + ).rejects.toThrow("Unsupported diffr config version: 3"); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); + +test("keeps the original and hides provider output when validation fails", async () => { + const { file, executable, env } = await fixture(); + const source = `[plugins.bundled.group]\nenabled = false\n`; + await writeFile(file, source); + await expect( + ensureDiffrConfigMigrated(executable, { ...env, REJECT_CANDIDATE: "1" }), + ).rejects.toThrow("The original file was kept"); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); + +test("does not replace an external edit made during validation", async () => { + const { file, executable, env } = await fixture(); + await writeFile(file, `[plugins.bundled.group]\nenabled = false\n`); + await expect( + ensureDiffrConfigMigrated(executable, { ...env, CONCURRENT_EDIT: "1" }), + ).rejects.toThrow("changed during migration"); + expect(await readFile(file, "utf8")).toBe("version = 1 # concurrent edit\n"); +}); + +test("uses the caller directory and preserves relative paths in the saved file", async () => { + const { root, file, env } = await fixture(); + const source = `[theme]\npath = "themes/custom.toml"\n[plugins.bundled.group]\nenabled = true\n`; + await writeFile(file, source); + const captured = path.join(root, "candidate.toml"); + expect( + await ensureDiffrConfigMigrated( + "./diffr-cli", + { + ...env, + XDG_CONFIG_HOME: ".", + CAPTURE_CANDIDATE: captured, + }, + undefined, + root, + ), + ).toBe(true); + expect(parse(await readFile(captured, "utf8")).theme).toEqual({ + path: path.join(path.dirname(file), "themes/custom.toml"), + }); + expect(parse(await readFile(file, "utf8")).theme).toEqual({ + path: "themes/custom.toml", + }); +}); + +test("migrates a version-only config and leaves v2 text unchanged", () => { + expect(parse(migrateDiffrConfig("version = 1 # format\n"))).toEqual({ + version: 2, + }); + const source = "version = 2 # format\n"; + expect(migrateDiffrConfig(source)).toBe(source); + expect(() => migrateDiffrConfig("version = 7\n")).toThrow( + "Unsupported diffr config version: 7", + ); +}); + +test("moves plugin options and external paths without rewriting value bytes", () => { + const source = `version = 1 +[plugins.bundled.context] +lines = 0x10 # keep spelling +extra = { option = 'custom', nested = [1, 2] } +[plugins.external.custom] +path = 'plugins/custom' # keep relative +threshold = 1_000 +[classifier] +hide = ['custom'] # keep tags +[diff] +byte_limit = 1_000_000 +`; + + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + bundled: { + context: { lines: 16, extra: { option: "custom", nested: [1, 2] } }, + }, + custom: { path: "plugins/custom", threshold: 1000 }, + }, + classify: { bundled: { hide: ["custom"] } }, + }, + diff: { byte_limit: 1000000 }, + }); + + for (const line of source + .split("\n") + .filter((line) => line.includes(" = ") && !line.startsWith("version"))) + expect(migrated).toContain(line); +}); + +test("preserves bundled and custom plugins with the same basename", () => { + const source = `[plugins.bundled.context] +enabled = false +lines = 9 +[plugins.external.context] +path = "custom/context" +`; + + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + bundled: { context: { enabled: false, lines: 9 } }, + context: { path: "custom/context" }, + }, + }, + }); + expect(migrated).toContain("lines = 9"); +}); + +test("moves a custom classifier under its manifest name and validates its original path", async () => { + const { root, file, executable, env } = await fixture(); + const directory = path.join(root, "diffr", "custom-classifier"); + await mkdir(directory); + await writeFile( + path.join(directory, "plugin.toml"), + 'name = "team.classifier"\n', + ); + + const source = + '[classifier]\npath = "custom-classifier" # local plugin\nhide = ["team"]\n'; + + await writeFile(file, source); + const captured = path.join(root, "candidate.toml"); + await ensureDiffrConfigMigrated(executable, { + ...env, + CAPTURE_CANDIDATE: captured, + }); + expect(parse(await readFile(file, "utf8"))).toEqual({ + version: 2, + plugins: { + classify: { + "team.classifier": { path: "custom-classifier", hide: ["team"] }, + }, + }, + }); + expect(parse(await readFile(captured, "utf8"))).toEqual({ + version: 2, + plugins: { + classify: { "team.classifier": { path: directory, hide: ["team"] } }, + }, + }); +}); + +test("keeps a custom classifier config when its manifest cannot be read", async () => { + const { file, executable, env } = await fixture(); + const source = '[classifier]\npath = "missing-plugin"\n'; + await writeFile(file, source); + await expect(ensureDiffrConfigMigrated(executable, env)).rejects.toThrow( + "could not read the custom classifier manifest", + ); + expect(await readFile(file, "utf8")).toBe(source); +}); + +test("redacts saved credentials from invalid TOML and schema errors", async () => { + const { file, executable, env } = await fixture(); + + for (const source of [ + 'plugins.bundled.summarize.api_key = "saved-secret" unexpected\n', + 'plugins.bundled.summarize = "saved-secret"\n', + ]) { + await writeFile(file, source); + const result = ensureDiffrConfigMigrated(executable, env); + await expect(result).rejects.toThrow("could not migrate the saved config"); + await expect(result).rejects.not.toThrow("saved-secret"); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); + } +}); + +test("serializes concurrent startup migrations and writes one backup", async () => { + const { file, executable, env } = await fixture(); + const source = "version = 1\n[plugins.bundled.context]\nlines = 11\n"; + await writeFile(file, source); + + const results = await Promise.all([ + ensureDiffrConfigMigrated(executable, env), + ensureDiffrConfigMigrated(executable, env), + ]); + + expect(results.sort()).toEqual([false, true]); + expect(parse(await readFile(file, "utf8"))).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { bundled: { context: { lines: 11 } } }, + }, + }); + + const backups = (await readdir(path.dirname(file))).filter((name) => + name.includes("before-whiteboard-config-v2"), + ); + + expect(backups).toHaveLength(1); + expect( + await readFile(path.join(path.dirname(file), backups[0]), "utf8"), + ).toBe(source); +}); + +test("migrates same-basename stock and custom entries on disk", async () => { + const { file, executable, env } = await fixture(); + + const source = + '[plugins.bundled.context]\nenabled = false\n[plugins.external.context]\npath = "custom"\n'; + + await writeFile(file, source); + expect(await ensureDiffrConfigMigrated(executable, env)).toBe(true); + expect(parse(await readFile(file, "utf8"))).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + bundled: { context: { enabled: false } }, + context: { path: "custom" }, + }, + }, + }); + + const backup = (await readdir(path.dirname(file))).find((name) => + name.includes("before-whiteboard-config-v2"), + )!; + + expect(await readFile(path.join(path.dirname(file), backup), "utf8")).toBe( + source, + ); +}); + +test.each([ + '[plugins.external."team.custom"]\npath = "plugins/custom"\n"option.name" = 0x10\n', + 'plugins.external."team.custom".path = "plugins/custom"\nplugins.external."team.custom"."option.name" = 0x10\n', + 'plugins = { external = { "team.custom" = { path = "plugins/custom", "option.name" = 0x10 } } }\n', +])("retains literal dots in external plugin and option names", (source) => { + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { "team.custom": { path: "plugins/custom", "option.name": 16 } }, + }, + }); + expect(migrated).toContain("0x10"); +}); + +test.each(["order", "bundled", "bundled.context"])( + "refuses reserved custom plugin name %s", + (name) => { + expect(() => + migrateDiffrConfig( + `[plugins.external.${JSON.stringify(name)}]\npath = "plugins/custom"\n`, + ), + ).toThrow(`custom plugin named ${name}`); + }, +); + +// Historical literals copied from diffr defaults and config/prune.rs. +test.each([ + 'For each listed fold, rewrite that function body as short python-flavored pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON array of {"id", "summary", "pseudocode"} objects, one per fold.', + 'For each listed fold, rewrite that function body as short pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON array of {"id", "summary", "pseudocode"} objects, one per fold.', + 'For each listed fold, rewrite that function body as short pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON object whose "summaries" array holds one {"id", "summary", "pseudocode"} object per fold.', +])("resets historical stock prompt %# but retains edits", (stockPrompt) => { + const source = `[plugins.bundled.summarize]\nsystem_prompt = ${JSON.stringify(stockPrompt)} # old default\n`; + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { bundled: { summarize: {} } }, + }, + }); + expect(migrated).toContain("# old default"); + + for (const custom of [ + " " + stockPrompt, + stockPrompt + " Keep my team naming.", + ]) { + const literal = JSON.stringify(custom); + + const migratedCustom = migrateDiffrConfig( + `[plugins.bundled.summarize]\nsystem_prompt = ${literal} # custom prompt\n`, + ); + + expect( + new TomlEdits(migratedCustom).value( + "plugins.shape.bundled.summarize.system_prompt", + ), + ).toBe(custom); + expect(migratedCustom).toContain( + `system_prompt = ${literal} # custom prompt`, + ); + } +}); + +test("moves the old default context-first order to the v2 context-last default", () => { + const source = + '[plugins]\norder = ["bundled.context", "bundled.hide-files", "bundled.deleted-bodies", "bundled.summarize", "bundled.test-bodies", "bundled.removed-runs", "bundled.group"]\n'; + + expect( + new TomlEdits(migrateDiffrConfig(source)).value("plugins.shape.order"), + ).toEqual([ + "bundled.deleted-bodies", + "bundled.summarize", + "bundled.test-bodies", + "bundled.removed-runs", + "bundled.context", + ]); +}); + +test("keeps new namespaces while removing only legacy parent tables", () => { + const source = `version = 1 +[plugins] # pipeline config +order = ["bundled.context", "external.team.custom"] +[plugins.bundled] # built-in plugins +context = { lines = 0x10 } +[plugins.external] # custom plugins +"team.custom" = { path = 'plugins/custom', threshold = 1_000 } +[classifier] # file tags +hide = ['team'] +`; + + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + order: ["bundled.context", "team.custom"], + bundled: { context: { lines: 16 } }, + "team.custom": { path: "plugins/custom", threshold: 1000 }, + }, + classify: { bundled: { hide: ["team"] } }, + }, + }); + expect(migrated).toContain("lines = 0x10"); + expect(migrated).toContain("threshold = 1_000"); + + for (const comment of [ + "pipeline config", + "built-in plugins", + "custom plugins", + "file tags", + ]) + expect(migrated).toContain(`# ${comment}`); +}); + +test("keeps a custom classifier whose name conflicts with bundled unchanged", async () => { + const { root, file, executable, env } = await fixture(); + const directory = path.join(root, "diffr", "custom-classifier"); + await mkdir(directory); + await writeFile(path.join(directory, "plugin.toml"), 'name = "bundled"\n'); + const source = '[classifier]\npath = "custom-classifier"\nhide = ["team"]\n'; + await writeFile(file, source); + await expect(ensureDiffrConfigMigrated(executable, env)).rejects.toThrow( + "custom classifier named bundled", + ); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual([ + "config.toml", + "custom-classifier", + ]); +}); + +test("retains order references for bundled and custom plugins with the same basename", () => { + const source = `[plugins] +order = ["external.context", "bundled.context"] +[plugins.bundled.context] +enabled = false +lines = 0x10 # stock override +[plugins.external.context] +path = 'custom/context' # custom path +lines = 1_000 # custom override +`; + + const migrated = migrateDiffrConfig(source); + expect(parse(migrated)).toEqual({ + version: 2, + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config namespace. + shape: { + order: ["context", "bundled.context"], + bundled: { context: { enabled: false, lines: 16 } }, + context: { path: "custom/context", lines: 1000 }, + }, + }, + }); + + for (const line of source.split("\n").filter((line) => line.includes("#"))) + expect(migrated).toContain(line); +}); + +test("keeps reserved custom plugin entries unchanged on disk", async () => { + const { file, executable, env } = await fixture(); + + const source = + '[plugins.external."bundled.context"]\npath = "custom/context"\n'; + + await writeFile(file, source); + await expect(ensureDiffrConfigMigrated(executable, env)).rejects.toThrow( + "custom plugin named bundled.context", + ); + expect(await readFile(file, "utf8")).toBe(source); + expect(await readdir(path.dirname(file))).toEqual(["config.toml"]); +}); diff --git a/packages/review/src/server/diffr-config-migration.ts b/packages/review/src/server/diffr-config-migration.ts new file mode 100644 index 000000000..0f6de340b --- /dev/null +++ b/packages/review/src/server/diffr-config-migration.ts @@ -0,0 +1,400 @@ +import { execFile } from "node:child_process"; +import { createHash } from "node:crypto"; +import { + mkdir, + mkdtemp, + readFile, + realpath, + rm, + writeFile, +} from "node:fs/promises"; +import { homedir } from "node:os"; +import path from "node:path"; +import { promisify } from "node:util"; + +import { withFileLock, writeFileAtomicAsync } from "@dev.fast/trace-core"; +import { parse } from "smol-toml"; +import { z } from "zod"; + +import { TomlEdits, tomlKey, tomlKeyParts } from "./toml-edits.js"; + +const execute = promisify(execFile); + +class ConfigMigrationError extends Error {} + +const oldOrder = [ + "bundled.context", + "bundled.hide-files", + "bundled.deleted-bodies", + "bundled.summarize", + "bundled.test-bodies", + "bundled.removed-runs", + "bundled.group", +]; + +const newOrder = [ + "bundled.deleted-bodies", + "bundled.summarize", + "bundled.test-bodies", + "bundled.removed-runs", + "bundled.context", +]; + +// Exact defaults written by older diffr releases; edited prompts stay intact. +const stockPrompts = new Set([ + 'For each listed fold, rewrite that function body as short python-flavored pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON array of {"id", "summary", "pseudocode"} objects, one per fold.', + 'For each listed fold, rewrite that function body as short pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON array of {"id", "summary", "pseudocode"} objects, one per fold.', + 'For each listed fold, rewrite that function body as short pseudocode. Keep the names. No prose, no comments, no code fences. Use as few lines as possible: about one pseudocode line per five source lines, and never more than a third of the body\'s lines. When a fold lists a doc, also set "summary" to one sentence copied verbatim from that doc; otherwise leave it empty. Answer with a JSON object whose "summaries" array holds one {"id", "summary", "pseudocode"} object per fold.', +]); + +const configVersionSchema = z.object({ version: z.number().int().optional() }); + +const pluginEntrySchema = z.record(z.string(), z.unknown()); + +const legacyConfigSchema = z.object({ + plugins: z + .object({ + order: z.array(z.string()).optional(), + bundled: z.record(z.string(), pluginEntrySchema).optional(), + external: z.record(z.string(), pluginEntrySchema).optional(), + }) + .optional(), + classifier: pluginEntrySchema.optional(), +}); + +const legacyHideSchema = z.object({ + enabled: z.boolean().default(true), + tags: z.array(z.string()).default(["generated", "vendored", "test"]), + deleted: z.boolean().default(true), +}); + +const legacySummarySchema = z.object({ system_prompt: z.string().optional() }); + +/** Convert v1 paths to v2 without serializing saved plugin option values. */ +export function migrateDiffrConfig( + source: string, + classifierName = "bundled", +): string { + const parsed = parse(source); + const version = configVersionSchema.parse(parsed).version ?? 1; + + if (version === 2) return source; + + if (version !== 1) + throw new ConfigMigrationError( + `Unsupported diffr config version: ${version}`, + ); + + const config = legacyConfigSchema.parse(parsed); + const bundled = config.plugins?.bundled; + const external = config.plugins?.external; + const edits = new TomlEdits(source); + + for (const name of Object.keys(external ?? {})) + if (name === "order" || name === "bundled" || name.startsWith("bundled.")) + throw new ConfigMigrationError( + `diffr config has a custom plugin named ${name}. Rename that plugin before migration; plugins.shape.${name} is reserved.`, + ); + + if (config.classifier?.path !== undefined && classifierName === "bundled") + throw new ConfigMigrationError( + "diffr config has a custom classifier named bundled. Rename that plugin before migration; plugins.classify.bundled is reserved.", + ); + + // The independent group switch is retired, for either saved boolean value. + edits.remove("plugins.bundled.group"); + + if (bundled?.["hide-files"] !== undefined) { + const hide = legacyHideSchema.parse(bundled["hide-files"]); + + // Explicit classifier settings in a mixed v1 file take precedence. + if (config.classifier?.hide === undefined) + edits.set("classifier.hide", hide.enabled ? hide.tags : []); + + if (config.classifier?.hide_deleted === undefined) + edits.set("classifier.hide_deleted", hide.enabled && hide.deleted); + edits.remove("plugins.bundled.hide-files"); + } + + if (config.plugins?.order) { + const order = config.plugins.order; + + const migrated = + JSON.stringify(order) === JSON.stringify(oldOrder) + ? newOrder + : order + .filter( + (name) => + name !== "bundled.group" && name !== "bundled.hide-files", + ) + .map((name) => name.replace(/^external\./, "")); + + edits.set("plugins.order", migrated); + edits.move("plugins.order", "plugins.shape.order"); + } + + // This old option was accepted but ignored. Do not turn it into --jobs. + edits.remove("plugins.bundled.summarize.max_concurrency"); + + const summary = + bundled?.summarize === undefined + ? undefined + : legacySummarySchema.parse(bundled.summarize); + + // Keep custom prompts exactly; reset only these historical stock defaults. + const resetPrompt = + summary?.system_prompt !== undefined && + stockPrompts.has(summary.system_prompt); + + if (resetPrompt) edits.remove("plugins.bundled.summarize.system_prompt"); + + for (const name of Object.keys(bundled ?? {})) + if (name !== "group" && name !== "hide-files") + edits.move( + tomlKey("plugins", "bundled", name), + tomlKey("plugins", "shape", "bundled", name), + ); + + for (const name of Object.keys(external ?? {})) + edits.move( + tomlKey("plugins", "external", name), + tomlKey("plugins", "shape", name), + ); + + edits.remove("plugins.bundled"); + edits.remove("plugins.external"); + edits.move("classifier", tomlKey("plugins", "classify", classifierName)); + edits.set("version", 2); + + const movedValues: [string, string][] = []; + + for (const key of new TomlEdits(source).valueKeys()) { + if ( + key.startsWith("plugins.bundled.group.") || + key.startsWith("plugins.bundled.hide-files.") || + key === "plugins.bundled.summarize.max_concurrency" || + (key === "plugins.bundled.summarize.system_prompt" && resetPrompt) + ) + continue; + + if (key.startsWith("plugins.bundled.")) + movedValues.push([ + key, + key.replace("plugins.bundled.", "plugins.shape.bundled."), + ]); + + if (key.startsWith("plugins.external.")) + movedValues.push([ + key, + key.replace("plugins.external.", "plugins.shape."), + ]); + + if (key.startsWith("classifier.")) + movedValues.push([ + key, + key.replace( + "classifier.", + `${tomlKey("plugins", "classify", classifierName)}.`, + ), + ]); + } + + edits.assertUnchanged(source, movedValues); + + return edits.source; +} + +/** Run once at startup; v2 files do not need binary probing or validation. */ +export async function ensureDiffrConfigMigrated( + executable: string, + env: NodeJS.ProcessEnv = process.env, + signal?: AbortSignal, + cwd?: string, +): Promise { + const file = path.resolve( + cwd ?? process.cwd(), + env.XDG_CONFIG_HOME || path.join(homedir(), ".config"), + "diffr", + "config.toml", + ); + + const target = await realpath(file).catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT") return undefined; + throw error; + }); + + if (!target) return false; + + const result = await withFileLock( + `${target}.whiteboard-migration.lock`, + { + retryMs: 25, + staleMs: 120_000, + timeoutMs: 30_000, + unownedGraceMs: 5_000, + }, + async () => { + signal?.throwIfAborted(); + const original = await readFile(target, "utf8"); + let candidate: string; + + try { + const version = configVersionSchema.parse(parse(original)).version ?? 1; + + if (version === 2) return false; + + if (version !== 1) + throw new ConfigMigrationError( + `Unsupported diffr config version: ${version}`, + ); + const classifierName = await legacyClassifierName(original, file); + candidate = migrateDiffrConfig(original, classifierName); + } catch (error) { + if (error instanceof ConfigMigrationError) throw error; + // TOML and schema errors can contain the source and saved credentials. + throw new Error( + "diffr could not migrate the saved config. The original file was kept.", + ); + } + + if (candidate === original) return false; + + // Validate from a sibling config directory, with relative paths made absolute. + await validateDiffrConfig(executable, candidate, file, env, signal, cwd); + + if ((await readFile(target, "utf8")) !== original) + throw new Error("diffr config changed during migration. Try again."); + + const hash = createHash("sha256") + .update(original) + .digest("hex") + .slice(0, 16); + + const backup = `${target}.before-whiteboard-config-v2.${hash}`; + + try { + await writeFile(backup, original, { flag: "wx", mode: 0o600 }); + } catch (error) { + if ( + !( + error instanceof Error && + "code" in error && + error.code === "EEXIST" + ) + ) + throw error; + + if ((await readFile(backup, "utf8")) !== original) + throw new Error( + "diffr migration backup already exists for different config. Keep it and move it before retrying.", + ); + } + + signal?.throwIfAborted(); + + if ((await readFile(target, "utf8")) !== original) + throw new Error("diffr config changed during migration. Try again."); + await writeFileAtomicAsync(target, candidate, { + encoding: "utf8", + mode: 0o600, + }); + + return true; + }, + ); + + if (!result.acquired) throw new Error("diffr config is busy. Try again."); + + return result.result; +} + +async function legacyClassifierName( + source: string, + file: string, +): Promise { + const classifier = legacyConfigSchema.parse(parse(source)).classifier; + + if (classifier?.path === undefined) return "bundled"; + + const directory = path.resolve( + path.dirname(file), + z.string().parse(classifier.path), + ); + + try { + const manifest = z + .object({ name: z.string().min(1) }) + .parse( + parse(await readFile(path.join(directory, "plugin.toml"), "utf8")), + ); + + return manifest.name; + } catch { + throw new ConfigMigrationError( + "diffr could not read the custom classifier manifest. The original file was kept.", + ); + } +} + +async function validateDiffrConfig( + executable: string, + source: string, + file: string, + env: NodeJS.ProcessEnv, + signal?: AbortSignal, + cwd?: string, +): Promise { + const directory = await mkdtemp( + path.join(path.dirname(file), ".whiteboard-config-"), + ); + + try { + const edits = new TomlEdits(source); + + // Moving the candidate for validation must not move theme or plugin paths. + for (const key of edits.keys()) { + const parts = tomlKeyParts(key); + + if ( + key === "theme.path" || + (parts.length === 4 && + parts[0] === "plugins" && + (parts[1] === "shape" || parts[1] === "classify") && + parts[3] === "path") || + (parts.length === 5 && + parts[0] === "plugins" && + parts[1] === "shape" && + parts[2] === "bundled" && + parts[4] === "path") + ) { + const value = z.string().parse(edits.value(key)); + + if (!path.isAbsolute(value)) + edits.set(key, path.resolve(path.dirname(file), value)); + } + } + + await mkdir(path.join(directory, "diffr"), { mode: 0o700 }); + await writeFile( + path.join(directory, "diffr", "config.toml"), + edits.source, + { mode: 0o600 }, + ); + + try { + await execute(executable, ["config", "show", "--json"], { + env: { ...env, XDG_CONFIG_HOME: directory }, + cwd, + signal: signal ?? AbortSignal.timeout(30_000), + maxBuffer: 16 * 1024 * 1024, + }); + } catch { + signal?.throwIfAborted(); + throw new Error( + "diffr could not validate the migrated config. The original file was kept.", + ); + } + } finally { + await rm(directory, { recursive: true, force: true }); + } +} diff --git a/packages/review/src/server/diffr-config.test.ts b/packages/review/src/server/diffr-config.test.ts index 4699803f7..cac9af5d2 100644 --- a/packages/review/src/server/diffr-config.test.ts +++ b/packages/review/src/server/diffr-config.test.ts @@ -114,7 +114,11 @@ const draft = { tests: true, }; -async function fakeDiffr(key = "", summarize: JsonObject = {}) { +async function fakeDiffr( + key = "", + summarize: JsonObject = {}, + classifier?: JsonObject, +) { const root = await mkdtemp(path.join(tmpdir(), "review-diffr-config-")); roots.push(root); @@ -125,18 +129,23 @@ async function fakeDiffr(key = "", summarize: JsonObject = {}) { await writeFile( state, JSON.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 }, }, }, }), @@ -153,23 +162,24 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { process.exit(2); } else if (args[0] === 'config' && args[1] === 'schema') { const summarize = ${JSON.stringify(SUMMARIZE_SCHEMA)}; - console.log(JSON.stringify({ properties: { plugins: { properties: { bundled: { properties: { summarize } } } } } })); + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + console.log(JSON.stringify({ properties: { plugins: { properties: { ${classifier ? 'classify: { properties: { bundled: { properties: { hide: { default: ["generated", "vendored"] } } } } },' : ""} shape: { properties: { bundled: { properties: { summarize } } } } } } } })); } else if (args[0] === 'config' && args[1] === 'show') { - if (!args.includes('--reveal') && config.plugins.bundled.summarize.api_key) config.plugins.bundled.summarize.api_key = ''; + if (!args.includes('--reveal') && config.plugins.shape.bundled.summarize.api_key) config.plugins.shape.bundled.summarize.api_key = ''; console.log(JSON.stringify(config)); } else if (args[0] === 'config' && args[1] === 'set') { if (args[2] === process.env.FAIL_KEY) { console.error('command leaked secret: ' + args.join(' ')); process.exit(2); } const keys = args[2].split('.'); let object = config; for (const part of keys.slice(0,-1)) object = object[part]; let value = args[3]; try { value = JSON.parse(value); } catch {} - // diffr 0.1.10 clears a provider's own settings when the provider changes. + // diffr clears a provider's own settings when the provider changes. if (process.env.FAKE_RESET && args[2].endsWith('.provider') && object.provider !== value) { for (const option of ['api_key', 'endpoint', 'model']) delete object[option]; } object[keys.at(-1)] = value; fs.writeFileSync(state, JSON.stringify(config)); } else if (!args.includes('--no-index')) { - console.log(JSON.stringify({type:'start',version:4,lhs:{type:'revision',rev:'base'},rhs:{type:'revision',rev:'head'},files:[]})); + console.log(JSON.stringify({type:'start',version:3,lhs:{type:'revision',rev:'base'},rhs:{type:'revision',rev:'head'},files:[]})); console.log(JSON.stringify({type:'complete',succeeded:0,failed:0})); } else { const temporary = require('node:path').join(process.env.XDG_CONFIG_HOME, 'diffr', 'config.toml'); @@ -181,7 +191,7 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { if (mode === 'hang') { setTimeout(() => {}, 10000); } else { const file = {rhs:{path:'after.rs',oid:'',mode:''}}; - console.log(JSON.stringify({type:'annotations',file,annotations:mode === 'empty' ? [] : [{region_id:1,label:'count positive values'}]})); + console.log(JSON.stringify({type:'file',file,diff:{type:'text',rhs:{text:'sample',root:{kind:'leaf',id:1,fold_state_id:1,alignment_id:1,start:{line:0,column:0},end:{line:0,column:6},visibility:mode === 'empty' ? undefined : {collapsed:true,label:'count positive values'}}},structural_changes:{base:[],head:[[0,1]]},stats:{textual:{added:1,removed:0},visible:{added:0,removed:0}}}})); console.log(JSON.stringify({type:'complete',succeeded:1,failed:0})); } } @@ -190,6 +200,8 @@ if (args[0] === 'config' && args[1] === 'schema' && process.env.FAIL_SCHEMA) { ); vi.stubEnv("REVIEW_DIFFR_BINARY", file); + vi.stubEnv("XDG_CONFIG_HOME", path.join(root, "xdg")); + return { root, state, @@ -211,6 +223,28 @@ test("reads resolved values and reports credentials without exposing keys", asyn ); }); +test("redacts credentials in bundled and custom shape entries", async () => { + const fake = await fakeDiffr("bundled-secret"); + const state = JSON.parse(await readFile(fake.state, "utf8")); + state.plugins.shape.custom = { + path: "plugins/custom", + api_key: "custom-secret", + enabled: true, + }; + await writeFile(fake.state, JSON.stringify(state)); + + const config = await readDiffrConfig(); + expect(JSON.stringify(config)).not.toMatch( + /bundled-secret|custom-secret|api_key/, + ); + expect(config.values).toMatchObject({ + plugins: { + // oxlint-disable-next-line anti-slop/no-shape-in-symbol-names -- Native diffr v2 config key. + shape: { custom: { path: "plugins/custom", enabled: true } }, + }, + }); +}); + test("detects host environment credentials and missing credentials", async () => { await fakeDiffr(); expect((await readDiffrConfig()).credentialSource).toBe("missing"); @@ -218,15 +252,88 @@ test("detects host environment credentials and missing credentials", async () => expect((await readDiffrConfig()).credentialSource).toBe("environment"); }); +test("reads classifier values and schema defaults without old plugin keys", async () => { + await fakeDiffr( + "saved-secret", + {}, + { hide: ["test", "custom"], hide_deleted: false }, + ); + const config = await readDiffrConfig(); + expect(config.values).toMatchObject({ + 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 fakeDiffr( + "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 = JSON.parse(await readFile(fake.state, "utf8")); + expect(disabled.plugins.classify.bundled).toEqual({ + hide: [], + hide_deleted: false, + }); + await setDiffrConfigValue("plugins.classify.bundled.hide", ["custom"]); + const enabled = JSON.parse(await readFile(fake.state, "utf8")); + 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 the native path of a custom classifier whose name contains spaces", async () => { + const fake = await fakeDiffr("saved-secret", {}, { hide: ["team"] }); + const state = JSON.parse(await readFile(fake.state, "utf8")); + state.plugins.classify = { + "team classifier": state.plugins.classify.bundled, + }; + await writeFile(fake.state, JSON.stringify(state)); + + const result = await setDiffrConfigValue( + "plugins.classify.team classifier.hide", + [], + ); + + expect(result.error).toBeUndefined(); + expect(result.values).toMatchObject({ + plugins: { classify: { "team classifier": { hide: [] } } }, + }); + const saved = JSON.parse(await readFile(fake.state, "utf8")); + expect(Object.keys(saved.plugins.classify)).toEqual(["team classifier"]); + expect(JSON.stringify(result)).not.toContain("saved-secret"); +}); + test("writes a setting, rereads it, and avoids invalidation for a no-op", async () => { await fakeDiffr(); expect( - (await setDiffrConfigValue("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 } } } } }, }); }); @@ -236,13 +343,13 @@ test("saves credentials and options before enabling and preserves blank keys", a const writes = (await fake.calls()).filter((args) => args[1] === "set"); expect(writes.map((args) => args[2])).toEqual( ["api_key", "model", "tests", "enabled"].map( - (key) => `plugins.bundled.summarize.${key}`, + (key) => `plugins.shape.bundled.summarize.${key}`, ), ); await saveDiffrSummarizer({ ...draft, apiKey: "" }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize - .api_key, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize.api_key, ).toBe("test-secret"); }); @@ -251,7 +358,7 @@ test("disables first and serializes concurrent saves", async () => { await saveDiffrSummarizer(draft); await Promise.all([ saveDiffrSummarizer({ ...draft, enabled: false, model: "next" }), - setDiffrConfigValue("plugins.bundled.context.lines", 9), + setDiffrConfigValue("plugins.shape.bundled.context.lines", 9), ]); const writes = (await fake.calls()) @@ -259,27 +366,30 @@ test("disables first and serializes concurrent saves", async () => { .slice(-3); expect(writes.map((args) => args[2])).toEqual([ - "plugins.bundled.summarize.enabled", - "plugins.bundled.summarize.model", - "plugins.bundled.context.lines", + "plugins.shape.bundled.summarize.enabled", + "plugins.shape.bundled.summarize.model", + "plugins.shape.bundled.context.lines", ]); }); test("partial failure returns current values and invalidates without leaking the key", async () => { await fakeDiffr(); - vi.stubEnv("FAIL_KEY", "plugins.bundled.summarize.model"); + vi.stubEnv("FAIL_KEY", "plugins.shape.bundled.summarize.model"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); expect(result.changed).toBe(true); expect(result.error).toContain("Some settings were saved"); expect(result.values).toMatchObject({ - 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"); + vi.stubEnv("FAIL_KEY", "plugins.shape.bundled.summarize.api_key"); const result = await saveDiffrSummarizer({ ...draft, apiKey: "test-secret" }); expect(result.changed).toBe(false); expect(result.error).toBeDefined(); @@ -380,12 +490,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"), @@ -414,8 +524,8 @@ test("saves a changed prompt and leaves an unchanged one alone", async () => { systemPrompt: "Be terse.", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize - .system_prompt, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize.system_prompt, ).toBe("Be terse."); }); @@ -423,7 +533,8 @@ test("switching providers clears the saved key unless a new one is entered", asy const fake = await fakeDiffr("gemini-secret"); const state = async () => - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize; + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize; await saveDiffrSummarizer({ ...draft, @@ -502,7 +613,7 @@ test("reads the environment key of the saved provider", async () => { test("a switch clears the saved key before naming the new provider", async () => { const fake = await fakeDiffr("gemini-secret"); - vi.stubEnv("FAIL_KEY", "plugins.bundled.summarize.provider"); + vi.stubEnv("FAIL_KEY", "plugins.shape.bundled.summarize.provider"); const result = await saveDiffrSummarizer({ ...draft, @@ -512,8 +623,8 @@ test("a switch clears the saved key before naming the new provider", async () => expect(result.error).toBeDefined(); - const state = JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled - .summarize; + const state = JSON.parse(await readFile(fake.state, "utf8")).plugins.shape + .bundled.summarize; // The failure left the old provider, and no key for it to send. expect(state).toMatchObject({ provider: "gemini", api_key: "" }); @@ -525,7 +636,8 @@ test("a new endpoint is treated like a new provider", async () => { await expect(testDiffrSummarizer(moved)).rejects.toThrow("Add an API key"); await saveDiffrSummarizer({ ...moved, enabled: false }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize, ).toMatchObject({ api_key: "", endpoint: "https://proxy.example/v1" }); }); @@ -619,7 +731,8 @@ test("a custom endpoint kept across a provider switch survives diffr clearing it apiKey: "openai-key", }); expect( - JSON.parse(await readFile(fake.state, "utf8")).plugins.bundled.summarize, + JSON.parse(await readFile(fake.state, "utf8")).plugins.shape.bundled + .summarize, ).toMatchObject({ provider: "openai", endpoint: "https://gateway.example/v1", diff --git a/packages/review/src/server/diffr-config.ts b/packages/review/src/server/diffr-config.ts index c7e137875..6538667bb 100644 --- a/packages/review/src/server/diffr-config.ts +++ b/packages/review/src/server/diffr-config.ts @@ -15,6 +15,7 @@ import { type ReviewDiffrConfig, type ReviewDiffrProvider, type ReviewDiffrSummarizerInput, + type StructuralRegion, decodeStructuralDiffEvent, reviewDiffrSummarizerInputSchema, } from "@dev.fast/review-protocol"; @@ -36,13 +37,39 @@ function serialized(operation: () => Promise): Promise { return next; } +type DiffrArguments = + | { command: "schema" } + | { command: "show"; reveal: boolean } + | { command: "set"; key: string; value: JsonValue } + | { command: "diff"; before: string; after: string }; + +function diffrArgv(args: DiffrArguments): string[] { + switch (args.command) { + case "schema": + return ["config", "schema"]; + case "show": + return ["config", "show", "--json", ...(args.reveal ? ["--reveal"] : [])]; + case "set": + return ["config", "set", args.key, diffrConfigValueText(args.value)]; + case "diff": + return [ + "--no-index", + "--format", + "ndjson", + "--", + args.before, + args.after, + ]; + } +} + async function diffr( - args: string[], + args: DiffrArguments, rootPath?: string, options: { signal?: AbortSignal; env?: NodeJS.ProcessEnv } = {}, ): Promise { try { - const { stdout } = await execFileAsync(diffrExecutable(), args, { + const { stdout } = await execFileAsync(diffrExecutable(), diffrArgv(args), { cwd: rootPath, maxBuffer: 16 * 1024 * 1024, signal: options.signal ?? AbortSignal.timeout(30_000), @@ -70,13 +97,7 @@ async function values( reveal = false, signal?: AbortSignal, ): Promise { - return json( - await diffr( - ["config", "show", "--json", ...(reveal ? ["--reveal"] : [])], - rootPath, - { signal }, - ), - ); + return json(await diffr({ command: "show", reveal }, rootPath, { signal })); } function json(output: string): JsonObject { @@ -100,7 +121,7 @@ function valueAt(object: JsonObject, key: string): JsonValue | undefined { return value; } -const prefix = "plugins.bundled.summarize"; +const prefix = "plugins.shape.bundled.summarize"; function savedProvider(config: JsonObject): string { const value = valueAt(config, `${prefix}.provider`); @@ -151,27 +172,35 @@ function strings(value: JsonValue | undefined): string[] { } /** - * What diffr's schema says about summaries: each provider's defaults, and - * the default prompt with the link its description gives. Nothing when - * diffr cannot describe its schema: the rest of Settings still works. + * Defaults supplied by diffr's schema for tag hiding and summary controls. + * If the schema cannot be read, resolved settings still work. */ -async function summaryDefaults( +async function configDefaults( rootPath?: string, -): Promise> { +): Promise< + Pick +> { let schema: JsonObject; try { - schema = json(await diffr(["config", "schema"], rootPath)); + schema = json(await diffr({ command: "schema" }, rootPath)); } catch { return {}; } const options = valueAt( schema, - "properties.plugins.properties.bundled.properties.summarize.properties", + "properties.plugins.properties.shape.properties.bundled.properties.summarize.properties", + ); + + const defaultHiddenTags = strings( + valueAt( + schema, + "properties.plugins.properties.classify.properties.bundled.properties.hide.default", + ), ); - if (!isJsonObject(options)) return {}; + if (!isJsonObject(options)) return { defaultHiddenTags }; const option = (name: string) => isJsonObject(options[name]) ? options[name] : {}; @@ -200,6 +229,7 @@ async function summaryDefaults( const prompt = option("system_prompt"); return { + defaultHiddenTags, providers, defaultPrompt: isStringValue(prompt.default) ? prompt.default : undefined, }; @@ -207,7 +237,7 @@ async function summaryDefaults( async function read(rootPath?: string): Promise { const config = await values(rootPath); - const defaults = await summaryDefaults(rootPath); + const defaults = await configDefaults(rootPath); const saved = valueAt(config, `${prefix}.api_key`); const credentialSource = @@ -217,16 +247,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 }; @@ -250,11 +279,9 @@ async function writeSettings( try { for (const [key, value] of entries) { - if (valueAt(current, key) === value) continue; - await diffr( - ["config", "set", key, diffrConfigValueText(value)], - rootPath, - ); + if (JSON.stringify(valueAt(current, key)) === JSON.stringify(value)) + continue; + await diffr({ command: "set", key, value }, rootPath); changed = true; // A write can clear others, such as a provider's endpoint and key. current = await values(rootPath); @@ -275,9 +302,7 @@ export function setDiffrConfigValue( value: JsonValue, rootPath?: string, ): Promise { - if ( - !/^[A-Za-z0-9_][A-Za-z0-9_-]*(\.[A-Za-z0-9_][A-Za-z0-9_-]*)*$/.test(key) - ) { + if (!/^[A-Za-z0-9_][A-Za-z0-9_-]*(\.[^.\r\n\0]+)*$/u.test(key)) { throw new Error("Invalid diffr config key."); } @@ -373,7 +398,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 +435,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 +453,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..c1a5a516a --- /dev/null +++ b/packages/review/src/server/diffr-startup.test.ts @@ -0,0 +1,109 @@ +import { readFileSync } from "node:fs"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; + +import { openLocalReviewStore } from "@review/review-api/local-data.js"; +import { parse } from "smol-toml"; +import { afterEach, expect, test, vi } from "vitest"; + +import { createGlobalReviewServer } from "./desktop-server.js"; +import { runHeadlessServer } from "./headless-host.js"; + +const roots: string[] = []; + +afterEach(async () => { + vi.unstubAllEnvs(); + await Promise.all( + roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), + ); +}); + +test.each(["desktop", "headless"] as const)( + "%s startup migrates saved settings before ready and skips migration on restart", + async (host) => { + const root = await mkdtemp(path.join(tmpdir(), "review-diffr-startup-")); + roots.push(root); + vi.stubEnv("DEV_REVIEW_HOME", root); + vi.stubEnv("XDG_CONFIG_HOME", root); + vi.stubEnv("DEV_FAST_REVIEW_TELEMETRY_DISABLED", "1"); + const file = path.join(root, "diffr", "config.toml"); + await mkdir(path.dirname(file)); + await writeFile( + file, + "version = 1\n[plugins.bundled.context]\nlines = 17\nenabled = false\n", + ); + const executable = path.join(root, "diffr-cli"); + const log = path.join(root, "validations"); + await writeFile( + executable, + `#!${process.execPath} +const fs = require('node:fs'); +const path = require('node:path'); +const source = fs.readFileSync(path.join(process.env.XDG_CONFIG_HOME, 'diffr', 'config.toml'), 'utf8'); +if (!/version = 2/.test(source)) process.exit(2); +fs.appendFileSync(${JSON.stringify(log)}, 'validated\\n'); +console.log('{}'); +`, + { mode: 0o755 }, + ); + vi.stubEnv("REVIEW_DIFFR_BINARY", executable); + + const ready = async () => { + expect(parse(await readFile(file, "utf8"))).toMatchObject({ + version: 2, + 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 }); + }; + + await start(); + await start(); + expect(await readFile(log, "utf8")).toBe("validated\n"); + }, +); diff --git a/packages/review/src/server/headless-host.ts b/packages/review/src/server/headless-host.ts index 34815d86b..045c1788a 100644 --- a/packages/review/src/server/headless-host.ts +++ b/packages/review/src/server/headless-host.ts @@ -12,6 +12,7 @@ import { reviewServerDiscoveryPath, } from "@review/server-discovery.js"; +import { ensureDiffrConfigMigrated } from "./diffr-config-migration.js"; import { GlobalReviewDesktopVerbRelay } from "./global-verb-relay.js"; import { createNodeRequestListener } from "./hono-http.js"; import { @@ -19,6 +20,7 @@ import { installProcessErrorTelemetry, } from "./process-error-telemetry.js"; import { createWhiteboardCore } from "./review-server-core.js"; +import { diffrExecutable } from "./structural-diff.js"; import type { ReviewTelemetryCapture } from "./ui-telemetry.js"; interface HeadlessServerInput { @@ -72,6 +74,8 @@ export function withHeadlessServerLock( async function serve(input: HeadlessServerInput) { if (input.signal.aborted) return; + await ensureDiffrConfigMigrated(diffrExecutable(), process.env, input.signal); + if (input.telemetry) await drainServerCrashReport(input.telemetry); const local = await openReviewProfile(input.stateDir, { diff --git a/packages/review/src/server/structural-diff-workspace.test.ts b/packages/review/src/server/structural-diff-workspace.test.ts index 8964486aa..9e7459a53 100644 --- a/packages/review/src/server/structural-diff-workspace.test.ts +++ b/packages/review/src/server/structural-diff-workspace.test.ts @@ -73,18 +73,22 @@ const {execFileSync} = require('node:child_process'); const args = process.argv.slice(2); const repo = args[args.indexOf('--repo') + 1]; const revisions = args - .slice(args.indexOf('ndjson') + 1, args.indexOf('--')) - .filter(arg => arg !== '--stream-annotations'); + .slice(args.indexOf('ndjson') + 1, args.indexOf('--')); const range = revisions.length === 1 ? revisions[0].split('...') : revisions; const file = args[args.indexOf('--') + 1]; -const read = rev => execFileSync('git', ['-C', repo, 'show', rev + ':' + file], {encoding:'utf8'}); -console.log(JSON.stringify({type:'start',version:4,lhs:{type:'revision',rev:range[0]},rhs:{type:'revision',rev:range[1]},files:[{file:{rhs:{path:file,oid:range[1],mode:'100644'}},status:'modified'}]})); -console.log(JSON.stringify({type:'file',file:{rhs:{path:file,oid:range[1],mode:'100644'}},diff:{type:'text',lhs:{text:read(range[0])},rhs:{text:read(range[1])},stats:{textual:{added:1,removed:1},visible:{added:1,removed:1}},structural_changes:{base:[[0,1]],head:[[0,1]]}}})); +const read = rev => { + const text = execFileSync('git', ['-C', repo, 'show', rev + ':' + file], {encoding:'utf8'}); + const lines = text.split('\\n'); + return {text,root:{kind:'leaf',id:1,fold_state_id:1,alignment_id:1,start:{line:0,column:0},end:{line:lines.length-1,column:lines.at(-1).length}}}; +}; +console.log(JSON.stringify({type:'start',version:3,lhs:{type:'revision',rev:range[0]},rhs:{type:'revision',rev:range[1]},files:[{file:{rhs:{path:file,oid:range[1],mode:'100644'}},status:'modified'}]})); +console.log(JSON.stringify({type:'file',file:{rhs:{path:file,oid:range[1],mode:'100644'}},diff:{type:'text',lhs:read(range[0]),rhs:read(range[1]),stats:{textual:{added:1,removed:1},visible:{added:1,removed:1}},structural_changes:{base:[[0,1]],head:[[0,1]]}}})); console.log(JSON.stringify({type:'complete',succeeded:1,failed:0})); `, { mode: 0o755 }, ); vi.stubEnv("REVIEW_DIFFR_BINARY", executable); + vi.stubEnv("XDG_CONFIG_HOME", path.join(root, "config")); const local = openLocalReviewStore(path.join(root, "reviews.db")); try { diff --git a/packages/review/src/server/structural-diff.test.ts b/packages/review/src/server/structural-diff.test.ts index 9ed12d9f4..5b22b20ae 100644 --- a/packages/review/src/server/structural-diff.test.ts +++ b/packages/review/src/server/structural-diff.test.ts @@ -1,15 +1,12 @@ -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; import type { JsonValue } from "@dev.fast/json"; +import { 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[] = []; @@ -27,13 +24,14 @@ async function executable(script: string) { const file = path.join(root, "diffr"); await writeFile(file, `#!${process.execPath}\n${script}`, { mode: 0o755 }); vi.stubEnv("REVIEW_DIFFR_BINARY", file); + vi.stubEnv("XDG_CONFIG_HOME", path.join(root, "config")); return root; } const START = { type: "start", - version: 4, + version: STRUCTURAL_DIFF_WIRE_VERSION, lhs: { type: "revision", rev: "base" }, rhs: { type: "revision", rev: "head" }, files: [ @@ -96,7 +94,6 @@ test.each(["trees", "merge-base"] as const)( root, "--format", "ndjson", - "--stream-annotations", ...(kind === "trees" ? ["base", "head"] : ["base...head"]), "--", "space name.ts", @@ -189,7 +186,7 @@ test("streams a large comparison without treating accumulated file bytes as one for (const { file } of files) await write({ type: 'file', file, diff: { - type: 'text', rhs: { text: 'x'.repeat(1024 * 1024), regions: [] }, + type: 'text', rhs: { text: 'x'.repeat(1024 * 1024), root: {kind:'leaf',id:1,fold_state_id:1,alignment_id:1,start:{line:0,column:0},end:{line:0,column:1024*1024}} }, structural_changes: { base: [], head: [[0, 1]] }, stats: { textual: { added: 1, removed: 0 }, visible: { added: 1, removed: 0 } }, }, @@ -322,70 +319,6 @@ test("rendering and coverage share a stream; cancelling one reader preserves the } }); -test("annotation failure is data, preserves files, and explains exit 2", async () => { - const annotation = { - type: "annotations", - file: FILE, - annotations: [], - error: { code: "enrichment_failed", message: "offline" }, - }; - - const root = await executable( - `${emit(START)} ${emit(BINARY)} ${emit(annotation)} ${emit(COMPLETE)} process.exitCode = 2;`, - ); - - expect(await collect(request(root))).toEqual([ - START, - BINARY, - annotation, - COMPLETE, - ]); -}); - -test("coverage can detach after initial files while summaries continue for later readers", async () => { - const { StructuralComparisons } = await import("./structural-comparisons.js"); - - const annotation = { - type: "annotations", - file: FILE, - annotations: [{ region_id: 1, label: "summary" }], - }; - - const root = await executable(` - require('node:fs').appendFileSync('runs', 'x'); - ${emit(START)} ${emit(BINARY)} - const timer = setInterval(() => { if(require('node:fs').existsSync('continue')) { clearInterval(timer); ${emit(annotation)} ${emit(COMPLETE)} } }, 10); - `); - - const cache = new StructuralComparisons(); - - try { - for await (const event of cache.stream(request(root))) - if (event.type === "file") break; - await writeFile(path.join(root, "continue"), ""); - const events = []; - - for await (const event of cache.stream(request(root))) events.push(event); - expect(events).toEqual([START, BINARY, annotation, COMPLETE]); - expect(await readFile(path.join(root, "runs"), "utf8")).toBe("x"); - } finally { - cache.close(); - } -}); - -test("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..e787323f1 100644 --- a/packages/review/src/server/structural-diff.ts +++ b/packages/review/src/server/structural-diff.ts @@ -1,17 +1,13 @@ import { spawn } from "node:child_process"; -import { existsSync } from "node:fs"; -import path from "node:path"; import { createInterface } from "node:readline"; +import { diffrBinaryPath } from "@dev.fast/diffr"; import { STRUCTURAL_DIFF_WIRE_VERSION, type StructuralDiffEvent, type StructuralProblem, decodeStructuralDiffEvent, } from "@dev.fast/review-protocol"; -import { findReviewPackageRoot } from "@review/package-paths"; - -import { installedFullDiffr } from "./diffr-languages.js"; export type DiffComparison = | { kind: "trees"; base: string; head: string } @@ -26,27 +22,13 @@ export interface StructuralDiffRequest { signal: AbortSignal; } -export function diffrExecutable( - packageRoot = findReviewPackageRoot(import.meta.url), -): string { - if (process.env.REVIEW_DIFFR_BINARY) return process.env.REVIEW_DIFFR_BINARY; - - const full = installedFullDiffr(packageRoot); - - if (full) return full; - - const bundled = path.join( - packageRoot, - "bin", - process.platform === "win32" ? "diffr.exe" : "diffr", - ); - - return existsSync(bundled) ? bundled : "diffr"; +export function diffrExecutable(): string { + return process.env.REVIEW_DIFFR_BINARY || diffrBinaryPath() || "diffr"; } export function diffrMissingError(): Error { return new Error( - `Cannot find diffr at ${diffrExecutable()}. Whiteboard Desktop bundles it at bin/diffr under its runtime; in a checkout, run \`pnpm --filter @dev.fast/whiteboard ensure:diffr\` or install diffr on PATH, or set REVIEW_DIFFR_BINARY to its executable.`, + `Cannot find diffr at ${diffrExecutable()}. Install diffr on PATH, or set REVIEW_DIFFR_BINARY to its executable.`, ); } @@ -61,13 +43,7 @@ export async function* structuralDiff( input.signal.throwIfAborted(); const comparison = input.comparison; - const args = [ - "--repo", - input.repositoryPath, - "--format", - "ndjson", - "--stream-annotations", - ]; + const args = ["--repo", input.repositoryPath, "--format", "ndjson"]; switch (comparison.kind) { case "worktree": @@ -120,7 +96,6 @@ export async function* structuralDiff( let completed = false; let aborted: StructuralProblem | undefined; let failed = 0; - let annotationFailed = false; const lines = createInterface({ input: child.stdout, crlfDelay: Infinity }); try { @@ -152,8 +127,6 @@ export async function* structuralDiff( failed = event.failed; aborted = event.aborted; - } else if (event.type === "annotations") { - annotationFailed ||= event.error !== undefined; } else if (event.type !== "file") { throw new Error(`Unexpected diffr event: ${event.type}`); } @@ -168,13 +141,10 @@ export async function* structuralDiff( throw new Error("diffr stream ended before completion."); } - // diffr exits 2 for file or enrichment failures already reported by the stream. + // diffr exits 2 for file failures already reported by the stream. const code = await exited; - if ( - code !== 0 && - !(code === 2 && (failed > 0 || annotationFailed || aborted !== undefined)) - ) + if (code !== 0 && !(code === 2 && (failed > 0 || aborted !== undefined))) throw exitError(code); } finally { clearTimeout(idle); diff --git a/packages/review/src/server/test-fixtures/diffr-ui-overrides.toml b/packages/review/src/server/test-fixtures/diffr-ui-overrides.toml new file mode 100644 index 000000000..ee58a441a --- /dev/null +++ b/packages/review/src/server/test-fixtures/diffr-ui-overrides.toml @@ -0,0 +1,38 @@ +# Overrides saved by Whiteboard 0.1.10. Migration must retain these comments. +version = 1 + +[plugins.bundled.context] +enabled = false # Collapse unchanged lines +lines = 17 # Context lines + +[plugins.bundled.test-bodies] +enabled = false # Collapse test bodies + +[plugins.bundled.deleted-bodies] +enabled = false # Collapse deleted function bodies + +[plugins.bundled.removed-runs] +enabled = false # Collapse long removed stretches + +[plugins.bundled.group] +enabled = false # Group adjacent folds; retired + +[plugins.bundled.hide-files] +enabled = true # Hide files by tag +tags = ["test", "generated"] +deleted = false # Hide deleted files + +[plugins.bundled.summarize] +enabled = true # Enable summaries +provider = "openai" # Provider +api_key = "synthetic-key" # API key +model = "custom-model" # Model +endpoint = "http://127.0.0.1:9/v1" # Endpoint URL +system_prompt = ''' Use my exact custom instructions. +Keep # inside the prompt, and answer with JSON if I ask for it. +''' # Prompt +tests = false # Include tests +max_concurrency = 2 # Old ignored option + +[diff] +byte_limit = 123456 # Unrelated file-only override diff --git a/packages/review/src/server/toml-edits.ts b/packages/review/src/server/toml-edits.ts new file mode 100644 index 000000000..c5012f8de --- /dev/null +++ b/packages/review/src/server/toml-edits.ts @@ -0,0 +1,266 @@ +import { type AST, getStaticTOMLValue, parseTOML } from "toml-eslint-parser"; + +export function tomlKey(...parts: string[]): string { + return parts + .map((part) => + /^[A-Za-z0-9_-]+$/.test(part) ? part : JSON.stringify(part), + ) + .join("."); +} + +export function tomlKeyParts(key: string): string[] { + const node = parseTOML(`${key} = 0`, { tomlVersion: "1.0" }).body[0].body[0]; + + if (node.type !== "TOMLKeyValue") + throw new Error("Expected a TOML config key."); + + return getStaticTOMLValue(node.key); +} + +/** Small source edits through TOML's AST; do not serialize unrelated user values. */ +export class TomlEdits { + constructor(public source: string) {} + + private index() { + const ast = parseTOML(this.source, { tomlVersion: "1.0" }); + const fields = new Map(); + const tables = new Map(); + + const field = (node: AST.TOMLKeyValue, prefix: string[]) => { + const key = [...prefix, ...getStaticTOMLValue(node.key)]; + fields.set(tomlKey(...key), node); + + if (node.value.type === "TOMLInlineTable") + for (const child of node.value.body) field(child, key); + }; + + for (const node of ast.body[0].body) { + if (node.type === "TOMLKeyValue") field(node, []); + else { + const key = node.resolvedKey.map(String); + tables.set(tomlKey(...key), node); + + for (const child of node.body) field(child, key); + } + } + + return { ast, fields, tables }; + } + + keys(): string[] { + return [...this.index().fields.keys()]; + } + + valueKeys(): string[] { + return [...this.index().fields].flatMap(([key, node]) => + node.value.type === "TOMLInlineTable" ? [] : [key], + ); + } + + value(key: string) { + const node = this.index().fields.get(key); + + return node && getStaticTOMLValue(node.value); + } + + /** Retained settings must keep their exact saved value, including prompt spacing. */ + assertUnchanged( + original: string, + keys: readonly (readonly [string, string])[], + ): void { + const before = new TomlEdits(original).index().fields; + const after = this.index().fields; + + for (const [from, to] of keys) { + const previous = before.get(from); + const current = after.get(to); + const previousText = previous && original.slice(...previous.value.range); + const currentText = current && this.source.slice(...current.value.range); + + if (previousText !== currentText) + throw new Error(`diffr migration changed a retained setting: ${from}`); + } + } + + /** Move a config subtree without serializing any of its saved values. */ + move(from: string, to: string): void { + const { fields, tables } = this.index(); + const matches = (key: string) => key === from || key.startsWith(`${from}.`); + + if ( + [...fields.keys(), ...tables.keys()].some( + (key) => key === to || key.startsWith(`${to}.`), + ) + ) + throw new Error(`diffr migration has conflicting config entries: ${to}`); + + const direct = fields.get(from); + + if (direct) { + const literal = this.source.slice(...direct.value.range); + this.remove(from, false); + this.setLiteral(to, literal); + + return; + } + + const moves: { start: number; end: number; text: string }[] = []; + const movedTables = [...tables].filter(([key]) => matches(key)); + + for (const [key, table] of movedTables) + moves.push({ + start: table.key.range[0], + end: table.key.range[1], + text: `${to}${key.slice(from.length)}`, + }); + + const dotted: [string, string][] = []; + + for (const [key, field] of fields) { + if (!matches(key) || field.parent.type === "TOMLInlineTable") continue; + + // Fields in a moved table keep their original spelling and comments. + if ( + field.parent.type === "TOMLTable" && + movedTables.some(([, table]) => table === field.parent) + ) + continue; + dotted.push([ + `${to}${key.slice(from.length)}`, + this.source.slice(...field.value.range), + ]); + moves.push({ start: field.range[0], end: field.range[1], text: "" }); + } + + for (const edit of moves.sort((a, b) => b.start - a.start)) + this.replace(edit.start, edit.end, edit.text); + + for (const [key, literal] of dotted) this.setLiteral(key, literal); + } + + remove(key: string, retainComments = true): void { + while (true) { + const { ast, fields, tables } = this.index(); + + const matches = (name: string) => + name === key || name.startsWith(`${key}.`); + + const node = + fields.get(key) ?? + [...tables].find(([name]) => matches(name))?.[1] ?? + [...fields].find(([name]) => matches(name))?.[1]; + + if (!node) return; + let [start, end] = node.range; + + if ( + node.type === "TOMLKeyValue" && + node.parent.type === "TOMLInlineTable" + ) { + const siblings = node.parent.body; + const position = siblings.indexOf(node); + const next = siblings[position + 1]; + const previous = siblings[position - 1]; + + if (next) { + const comma = ast.tokens.find( + (token) => + token.value === "," && + token.range[0] >= end && + token.range[0] < next.range[0], + ); + + if (comma) end = comma.range[1]; + } else if (previous) { + const comma = ast.tokens.find( + (token) => + token.value === "," && + token.range[0] >= previous.range[1] && + token.range[0] < start, + ); + + if (comma) start = comma.range[0]; + } + } + + const comments = retainComments + ? ast.comments + .filter( + (comment) => comment.range[0] >= start && comment.range[1] <= end, + ) + .map((comment) => this.source.slice(...comment.range)) + .join("\n") + : ""; + + this.replace(start, end, comments); + } + } + + set(key: string, value: boolean | number | string | string[]): void { + this.setLiteral(key, JSON.stringify(value), Array.isArray(value)); + } + + private setLiteral(key: string, input: string, retainComments = false): void { + const { ast, fields, tables } = this.index(); + let literal = input; + const existing = fields.get(key); + + if (existing) { + // Keep notes between entries when a plugin order is rewritten. + if (retainComments) { + const comments = ast.comments + .filter( + (comment) => + comment.range[0] >= existing.value.range[0] && + comment.range[1] <= existing.value.range[1], + ) + .map((comment) => this.source.slice(...comment.range)); + + if (comments.length) + literal = `[\n${comments.join("\n")}\n${literal.slice(1)}`; + } + + this.replace(...existing.value.range, literal); + + return; + } + + const parts = tomlKeyParts(key); + + for (let length = parts.length - 1; length > 0; length--) { + const parent = tomlKey(...parts.slice(0, length)); + const suffix = tomlKey(...parts.slice(length)); + const inline = fields.get(parent)?.value; + + if (inline?.type === "TOMLInlineTable") { + this.replace( + inline.range[1] - 1, + inline.range[1] - 1, + `${inline.body.length ? ", " : ""}${suffix} = ${literal}`, + ); + + return; + } + + const table = tables.get(parent); + + if (table) { + const newline = this.source.indexOf("\n", table.key.range[1]); + const at = newline < 0 ? this.source.length : newline + 1; + this.replace( + at, + at, + `${newline < 0 ? "\n" : ""}${suffix} = ${literal}\n`, + ); + + return; + } + } + + this.source = `${key} = ${literal}\n${this.source}`; + } + + private replace(start: number, end: number, text: string): void { + this.source = this.source.slice(0, start) + text + this.source.slice(end); + } +} diff --git a/packages/review/vitest.config.ts b/packages/review/vitest.config.ts index 2ea18dcf4..a34e006d4 100644 --- a/packages/review/vitest.config.ts +++ b/packages/review/vitest.config.ts @@ -12,6 +12,10 @@ export default defineConfig({ os.tmpdir(), `progressive-review-tests-${process.pid}`, ), + XDG_CONFIG_HOME: path.join( + os.tmpdir(), + `progressive-review-config-tests-${process.pid}`, + ), // GitHub Actions exports the repository slug, which the trace code // honors over a checkout's remote; scratch repositories in tests must // resolve to their own remotes. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index dbf356132..6e3d187b7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -6,6 +6,12 @@ settings: catalogs: default: + '@dev.fast/diffr': + specifier: 0.1.13 + version: 0.1.13 + toml-eslint-parser: + specifier: ^1.0.3 + version: 1.0.3 zod: specifier: 4.6.5 version: 4.6.5 @@ -51,8 +57,8 @@ importers: version: link:../../packages/review devDependencies: '@dev.fast/diffr': - specifier: 0.1.10 - version: 0.1.10(zod@4.6.5) + specifier: 'catalog:' + version: 0.1.13(zod@4.6.5) '@dev.fast/review-canvas': specifier: workspace:* version: link:../../packages/review/app @@ -119,6 +125,9 @@ importers: '@agentclientprotocol/sdk': specifier: 1.5.1 version: 1.5.1(zod@4.6.5) + '@dev.fast/diffr': + specifier: 'catalog:' + version: 0.1.13(zod@4.6.5) '@hono/node-server': specifier: 2.0.12 version: 2.0.12(hono@4.13.5) @@ -164,13 +173,13 @@ importers: smol-toml: specifier: 1.8.0 version: 1.8.0 + toml-eslint-parser: + specifier: 'catalog:' + version: 1.0.3 zod: specifier: 'catalog:' version: 4.6.5 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 +226,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.13(zod@4.6.5) '@dev.fast/json': specifier: workspace:* version: link:../json @@ -554,10 +563,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.13': + resolution: {integrity: sha512-xTXaa4PHjgbFgSuW+Ery5lvsNwIG9HbliqnmaUxrG4U9yxPtDSsi7hPpJnYfEB+0XO43s090ihiNtzLGfC3BNQ==} + cpu: [arm64] + os: [darwin] + + '@dev.fast/diffr-darwin-x64@0.1.13': + resolution: {integrity: sha512-XgZ22QRmK8vfRZHsHs9s0wZDkzS4IL1rQaVAizjuIGG+AubYkabqRzFqy/JE2M+MpXSS+fbw1W8r9e/9TulOnA==} + cpu: [x64] + os: [darwin] + + '@dev.fast/diffr-linux-arm64@0.1.13': + resolution: {integrity: sha512-tmTQLx9j3JW9mYh5YJ/JJES03c8j/BNJ5PnvEwSsd9FfS7K7+pHD+Lfk2jtzvaROzF2MZCEDI57TDq3b5zhvaQ==} + cpu: [arm64] + os: [linux] + libc: [glibc] + + '@dev.fast/diffr-linux-x64@0.1.13': + resolution: {integrity: sha512-xD60m46hDobsG+kJx3kulI/NEpGWw7B1OB4h9tAItjwOrPsh9mMcXlQfJYPGtlZha6CfE1+Ympc7Lexr/QTqfw==} + cpu: [x64] + os: [linux] + libc: [glibc] + + '@dev.fast/diffr-win32-x64@0.1.13': + resolution: {integrity: sha512-c9oaND+TyqrFVccOAmoWfg/25+pDKBnYNzoi4t5pdStKSFjcNSYgltoGIoS+GV1aFNdXOo/6qk3wdDDqKccR/g==} + cpu: [x64] + os: [win32] + + '@dev.fast/diffr@0.1.13': + resolution: {integrity: sha512-0awUVjvSWRv1oQu3prCrKvqPBH64Eqk+AtqUT67a3AxcKdvm4CrGeF+cPVy9D0PVhbXePPeqV0spjCwKShO0gw==} engines: {node: '>=20'} - hasBin: true peerDependencies: zod: ^4.0.0 @@ -2021,6 +2056,10 @@ packages: resolution: {integrity: sha512-/veY75JbMK4j1yjvuUxuVsiS/hr/4iHs9FTT6cgTexxdE0Ly/glccBAkloH/DofkjRbZU3bnoj38mOmhkZ0lHw==} engines: {node: '>=12'} + eslint-visitor-keys@5.0.1: + resolution: {integrity: sha512-tD40eHxA35h0PEIZNeIjkHoDR4YjjJp34biM0mDvplBe//mB+IHCqHDGV7pxF+7MklTvighcCPPZC7ynWyjdTA==} + engines: {node: ^20.19.0 || ^22.13.0 || >=24} + estree-walker@3.0.3: resolution: {integrity: sha512-7RUKfXgSMMkzt6ZuXmqapOurLGPPfgj6l9uRZ7lRGolvk0y2yocc35LdcxKC5PQZdn2DMqioAQ2NoWcrTKmm6g==} @@ -2894,6 +2933,10 @@ packages: resolution: {integrity: sha512-o5sSPKEkg/DIQNmH43V0/uerLrpzVedkUh8tGNvaeXpfpuwjKenlSox/2O/BTlZUtEe+JG7s5YhEz608PlAHRA==} engines: {node: '>=0.6'} + toml-eslint-parser@1.0.3: + resolution: {integrity: sha512-A5F0cM6+mDleacLIEUkmfpkBbnHJFV1d2rprHU2MXNk7mlxHq2zGojA+SRvQD1RoMo9gqjZPWEaKG4v1BQ48lw==} + engines: {node: ^20.19.0 || ^22.13.0 || >=24} + totalist@3.0.1: resolution: {integrity: sha512-sf4i37nQ2LBx4m3wB74y+ubopq6W/dIzXg0FDGjsYnZHVa1Da8FH853wlL2gtUhg+xJXjfk3kUZS3BRoQeoQBQ==} engines: {node: '>=6'} @@ -3426,9 +3469,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.13': + optional: true + + '@dev.fast/diffr-darwin-x64@0.1.13': + optional: true + + '@dev.fast/diffr-linux-arm64@0.1.13': + optional: true + + '@dev.fast/diffr-linux-x64@0.1.13': + optional: true + + '@dev.fast/diffr-win32-x64@0.1.13': + optional: true + + '@dev.fast/diffr@0.1.13(zod@4.6.5)': dependencies: zod: 4.6.5 + optionalDependencies: + '@dev.fast/diffr-darwin-arm64': 0.1.13 + '@dev.fast/diffr-darwin-x64': 0.1.13 + '@dev.fast/diffr-linux-arm64': 0.1.13 + '@dev.fast/diffr-linux-x64': 0.1.13 + '@dev.fast/diffr-win32-x64': 0.1.13 '@dual-bundle/import-meta-resolve@4.2.1': {} @@ -4478,6 +4542,8 @@ snapshots: escape-string-regexp@5.0.0: {} + eslint-visitor-keys@5.0.1: {} + estree-walker@3.0.3: dependencies: '@types/estree': 1.0.8 @@ -5572,6 +5638,10 @@ snapshots: toidentifier@1.0.1: {} + toml-eslint-parser@1.0.3: + dependencies: + eslint-visitor-keys: 5.0.1 + totalist@3.0.1: {} tough-cookie@6.0.2: diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index ed99b113c..4d86bd16b 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -5,6 +5,8 @@ packages: - "apps/*" catalog: + '@dev.fast/diffr': 0.1.13 + toml-eslint-parser: ^1.0.3 zod: "4.6.5" catalogMode: strict @@ -17,7 +19,12 @@ verifyDepsBeforeRun: false minimumReleaseAge: 10080 minimumReleaseAgeStrict: true minimumReleaseAgeExclude: - - "@dev.fast/diffr@0.1.10" + - "@dev.fast/diffr@0.1.13" + - "@dev.fast/diffr-darwin-arm64@0.1.13" + - "@dev.fast/diffr-darwin-x64@0.1.13" + - "@dev.fast/diffr-linux-arm64@0.1.13" + - "@dev.fast/diffr-linux-x64@0.1.13" + - "@dev.fast/diffr-win32-x64@0.1.13" # Ask tracks the latest ACP SDK and adapters, and the CLI packages they pin. - "@agentclientprotocol/sdk@1.5.1" - "@agentclientprotocol/claude-agent-acp@0.84.0"