diff --git a/package.json b/package.json index 5d4a7faf..a72e88ff 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,7 @@ "dev": "node scripts/dev-server.mjs", "postinstall": "node scripts/fix-node-pty-permissions.mjs", "start": "node dist/cli.js serve", - "test": "tsx src/config.test.ts && tsx src/request-meta.test.ts && tsx src/incoming-artifacts.test.ts && tsx src/artifact-download.test.ts && tsx src/ui/card-types.test.ts && tsx src/ui/patch-display.test.ts && tsx src/ui/tool-display.test.ts && tsx src/apply-patch.test.ts && tsx src/process-platform.test.ts && tsx src/process-sessions.test.ts && tsx src/mcp-sessions.test.ts && tsx src/server-shutdown.test.ts && tsx src/local-agent-runtime.test.ts && tsx src/local-agent-adapters.test.ts && tsx src/local-agent-availability.test.ts && tsx src/local-agent-profiles.test.ts && tsx src/local-agent-targets.test.ts && tsx src/local-agent-store.test.ts && tsx src/roots.test.ts && tsx src/skills.test.ts && tsx src/workspaces.test.ts && tsx src/workspace-conversation.test.ts && tsx src/review-checkpoints.test.ts && tsx src/server.test.ts && tsx src/oauth-store.test.ts && tsx src/cli.test.ts", + "test": "tsx src/config.test.ts && tsx src/request-meta.test.ts && tsx src/incoming-artifacts.test.ts && tsx src/artifact-download.test.ts && tsx src/ui/card-types.test.ts && tsx src/ui/patch-display.test.ts && tsx src/ui/tool-display.test.ts && tsx src/apply-patch.test.ts && tsx src/process-platform.test.ts && tsx src/process-sessions.test.ts && tsx src/mcp-sessions.test.ts && tsx src/server-shutdown.test.ts && tsx src/local-agent-runtime.test.ts && tsx src/local-agent-adapters.test.ts && tsx src/local-agent-availability.test.ts && tsx src/local-agent-profiles.test.ts && tsx src/local-agent-targets.test.ts && tsx src/local-agent-store.test.ts && tsx src/roots.test.ts && tsx src/skills.test.ts && tsx src/workspaces.test.ts && tsx src/workspace-conversation.test.ts && tsx src/review-checkpoints.test.ts && tsx src/review-change-journal.test.ts && tsx src/server.test.ts && tsx src/oauth-store.test.ts && tsx src/cli.test.ts", "typecheck": "tsc -p tsconfig.json --noEmit" }, "keywords": [], diff --git a/src/apply-patch.test.ts b/src/apply-patch.test.ts index 3a846f4f..5d8b9088 100644 --- a/src/apply-patch.test.ts +++ b/src/apply-patch.test.ts @@ -63,6 +63,7 @@ assert.equal(await readFile(join(root, "windows.txt"), "utf8"), "first\r\nupdate await assert.rejects(readFile(join(root, "remove.txt"), "utf8"), /ENOENT/); if (process.platform !== "win32") await chmod(join(root, "alpha.txt"), 0o755); +let movePreparedBeforeMutation = false; const moveResult = await applyPatch( root, `*** Begin Patch @@ -73,7 +74,22 @@ const moveResult = await applyPatch( +ONE changed *** End Patch`, + { + beforeApply: async ({ paths, files }) => { + assert.deepEqual(new Set(paths), new Set([ + join(root, "alpha.txt"), + join(root, "moved", "alpha.txt"), + ])); + assert.deepEqual(files, [ + { path: "moved/alpha.txt", previousPath: "alpha.txt", operation: "move" }, + ]); + assert.equal(await readFile(join(root, "alpha.txt"), "utf8"), "one\nchanged\nthree\n"); + await assert.rejects(readFile(join(root, "moved", "alpha.txt"), "utf8"), /ENOENT/); + movePreparedBeforeMutation = true; + }, + }, ); +assert.equal(movePreparedBeforeMutation, true); assert.deepEqual(moveResult.files, [ { path: "moved/alpha.txt", previousPath: "alpha.txt", operation: "move" }, ]); diff --git a/src/apply-patch.ts b/src/apply-patch.ts index 05a73de6..bc3dfc68 100644 --- a/src/apply-patch.ts +++ b/src/apply-patch.ts @@ -20,6 +20,13 @@ export interface ApplyPatchResult { removals: number; } +export interface ApplyPatchOptions { + beforeApply?: (input: { + paths: readonly string[]; + files: readonly AppliedPatchFile[]; + }) => Promise | void; +} + interface HunkLine { kind: "context" | "add" | "remove"; text: string; @@ -340,7 +347,11 @@ export async function isSamePatchFile( } } -export async function applyPatch(root: string, patch: string): Promise { +export async function applyPatch( + root: string, + patch: string, + options: ApplyPatchOptions = {}, +): Promise { const actions = parsePatch(patch); const results: AppliedPatchFile[] = []; const patches: string[] = []; @@ -396,6 +407,11 @@ export async function applyPatch(root: string, patch: string): Promise { + const root = await workspace(t); + const path = join(root, "file.txt"); + await writeFile(path, "A\n"); + const journal = createReviewChangeJournal(); + + const first = await journal.prepareMutation({ workspaceId: "ws_net", root, paths: [path] }); + await writeFile(path, "B\n"); + journal.commitMutation(first); + + const second = await journal.prepareMutation({ workspaceId: "ws_net", root, paths: [path] }); + await writeFile(path, "C\n"); + journal.commitMutation(second); + + const review = await journal.reviewChanges({ workspaceId: "ws_net", root }); + assert.deepEqual(review.files.map((file) => file.path), ["file.txt"]); + assert.match(review.patch, /-A/); + assert.match(review.patch, /\+C/); + assert.doesNotMatch(review.patch, /[+-]B/); +}); + +test("journal drops net-zero mutations and unrelated filesystem changes", async (t) => { + const root = await workspace(t); + const tracked = join(root, "tracked.txt"); + const unrelated = join(root, "unrelated.txt"); + await writeFile(tracked, "A\n"); + await writeFile(unrelated, "before\n"); + const journal = createReviewChangeJournal(); + + const mutation = await journal.prepareMutation({ + workspaceId: "ws_zero", + root, + paths: [tracked], + }); + await writeFile(tracked, "B\n"); + await writeFile(tracked, "A\n"); + await writeFile(unrelated, "after\n"); + journal.commitMutation(mutation); + + const review = await journal.reviewChanges({ workspaceId: "ws_zero", root }); + assert.equal(review.summary.files, 0); + assert.equal(review.patch, ""); +}); + +test("journal preserves a move across later edits", async (t) => { + const root = await workspace(t); + const before = join(root, "before.txt"); + const after = join(root, "after.txt"); + await writeFile(before, "before\n"); + const journal = createReviewChangeJournal(); + + const move = await journal.prepareMutation({ + workspaceId: "ws_move", + root, + paths: [before, after], + }); + await rename(before, after); + journal.commitMutation(move, [{ fromPath: "before.txt", toPath: "after.txt" }]); + + const edit = await journal.prepareMutation({ workspaceId: "ws_move", root, paths: [after] }); + await writeFile(after, "after\n"); + journal.commitMutation(edit); + + const review = await journal.reviewChanges({ workspaceId: "ws_move", root }); + assert.deepEqual(review.files, [ + { + path: "after.txt", + previousPath: "before.txt", + type: "rename-changed", + additions: 1, + removals: 1, + }, + ]); +}); + +test("markReviewed advances the journal without requiring Git", async (t) => { + const root = await workspace(t); + const path = join(root, "file.txt"); + await writeFile(path, "A\n"); + const journal = createReviewChangeJournal(); + const mutation = await journal.prepareMutation({ workspaceId: "ws_advance", root, paths: [path] }); + await writeFile(path, "B\n"); + journal.commitMutation(mutation); + + journal.markReviewed({ workspaceId: "ws_advance", root }); + assert.equal(journal.hasTrackedMutations("ws_advance"), false); + const review = await journal.reviewChanges({ workspaceId: "ws_advance", root }); + assert.equal(review.summary.files, 0); +}); + +test("journal preserves empty-file additions as additions", async (t) => { + const root = await workspace(t); + const path = join(root, "empty.txt"); + const journal = createReviewChangeJournal(); + const mutation = await journal.prepareMutation({ workspaceId: "ws_empty", root, paths: [path] }); + await writeFile(path, ""); + journal.commitMutation(mutation); + + const review = await journal.reviewChanges({ workspaceId: "ws_empty", root }); + assert.equal(review.files[0]?.type, "new"); + assert.match(review.patch, /new file mode/); +}); + +async function workspace(t: TestContext): Promise { + const root = await mkdtemp(join(tmpdir(), "devspace-review-journal-test-")); + t.after(() => rm(root, { recursive: true, force: true })); + return root; +} diff --git a/src/review-change-journal.ts b/src/review-change-journal.ts new file mode 100644 index 00000000..41180004 --- /dev/null +++ b/src/review-change-journal.ts @@ -0,0 +1,285 @@ +import { readFile } from "node:fs/promises"; +import { isAbsolute, relative, resolve, sep } from "node:path"; +import { TextDecoder } from "node:util"; +import { createTwoFilesPatch, FILE_HEADERS_ONLY } from "diff"; +import { + parseReviewFiles, + summarizeReviewFiles, + type ReviewChangesResult, + type ReviewFile, +} from "./review-diff.js"; + +type FileState = + | { kind: "missing" } + | { kind: "file"; bytes: Buffer } + | { kind: "unavailable" }; + +interface WorkspaceJournalState { + root: string; + baselines: Map; + moves: Map; +} + +export interface ReviewMutationCapture { + workspaceId: string; + root: string; + originals: Map; +} + +export interface ReviewMove { + fromPath: string; + toPath: string; +} + +export interface ReviewChangeJournal { + initializeWorkspace(input: { workspaceId: string; root: string }): void; + prepareMutation(input: { + workspaceId: string; + root: string; + paths: readonly string[]; + }): Promise; + commitMutation(capture: ReviewMutationCapture, moves?: readonly ReviewMove[]): void; + hasTrackedMutations(workspaceId: string): boolean; + reviewChanges(input: { workspaceId: string; root: string }): Promise; + markReviewed(input: { workspaceId: string; root: string }): void; +} + +export function createReviewChangeJournal(): ReviewChangeJournal { + const states = new Map(); + + const initializeWorkspace = ({ workspaceId, root }: { workspaceId: string; root: string }): void => { + const existing = states.get(workspaceId); + if (existing) { + assertWorkspaceRoot(existing.root, workspaceId, root); + return; + } + states.set(workspaceId, { + root, + baselines: new Map(), + moves: new Map(), + }); + }; + + return { + initializeWorkspace, + + async prepareMutation({ workspaceId, root, paths }) { + initializeWorkspace({ workspaceId, root }); + const state = states.get(workspaceId)!; + const originals = new Map(); + + for (const path of new Set(paths)) { + const relativePath = workspaceRelativePath(root, path); + if (state.baselines.has(relativePath) || originals.has(relativePath)) continue; + originals.set(relativePath, await readState(path)); + } + + return { workspaceId, root, originals }; + }, + + commitMutation(capture, moves = []) { + initializeWorkspace(capture); + const state = states.get(capture.workspaceId)!; + for (const [path, original] of capture.originals) { + if (!state.baselines.has(path)) state.baselines.set(path, original); + } + for (const move of moves) recordMove(state.moves, move.fromPath, move.toPath); + }, + + hasTrackedMutations(workspaceId) { + return (states.get(workspaceId)?.baselines.size ?? 0) > 0; + }, + + async reviewChanges({ workspaceId, root }) { + initializeWorkspace({ workspaceId, root }); + const state = states.get(workspaceId)!; + const files: ReviewFile[] = []; + const patches: string[] = []; + const consumed = new Set(); + + for (const [fromPath, toPath] of state.moves) { + const beforeSource = state.baselines.get(fromPath); + const beforeDestination = state.baselines.get(toPath); + if (!beforeSource || !beforeDestination) continue; + + const [afterSource, afterDestination] = await Promise.all([ + readState(resolve(root, fromPath)), + readState(resolve(root, toPath)), + ]); + if ( + beforeSource.kind !== "file" || + beforeDestination.kind !== "missing" || + afterSource.kind !== "missing" || + afterDestination.kind !== "file" + ) { + continue; + } + + const patch = filePatch(fromPath, toPath, beforeSource, afterDestination); + const stats = patchStats(patch); + files.push({ + path: toPath, + previousPath: fromPath, + type: beforeSource.bytes.equals(afterDestination.bytes) ? "rename-pure" : "rename-changed", + ...stats, + }); + patches.push(patch); + consumed.add(fromPath); + consumed.add(toPath); + } + + for (const [path, before] of state.baselines) { + if (consumed.has(path)) continue; + const after = await readState(resolve(root, path)); + if (sameState(before, after)) continue; + + const patch = filePatch(path, path, before, after); + const stats = patchStats(patch); + files.push({ + path, + type: fileChangeType(before, after), + ...stats, + }); + patches.push(patch); + } + + files.sort((left, right) => left.path.localeCompare(right.path)); + const summary = summarizeReviewFiles(files); + return { + result: + summary.files === 0 + ? "No changes since last shown changes." + : `Changed ${summary.files} ${summary.files === 1 ? "file" : "files"} (+${summary.additions} -${summary.removals}).`, + summary, + files, + patch: patches.filter(Boolean).join("\n"), + }; + }, + + markReviewed({ workspaceId, root }) { + initializeWorkspace({ workspaceId, root }); + const state = states.get(workspaceId)!; + state.baselines.clear(); + state.moves.clear(); + }, + }; +} + +async function readState(path: string): Promise { + try { + return { kind: "file", bytes: await readFile(path) }; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT" || code === "ENOTDIR") return { kind: "missing" }; + return { kind: "unavailable" }; + } +} + +function filePatch(oldPath: string, newPath: string, before: FileState, after: FileState): string { + const oldText = stateText(before); + const newText = stateText(after); + if (oldText !== undefined && newText !== undefined) { + const patch = createTwoFilesPatch( + before.kind === "missing" ? "/dev/null" : oldPath, + after.kind === "missing" ? "/dev/null" : newPath, + oldText, + newText, + "", + "", + { context: 3, headerOptions: FILE_HEADERS_ONLY }, + ); + return withFileModeHeader(oldPath, newPath, before, after, patch); + } + + const oldLabel = before.kind === "missing" ? "/dev/null" : `a/${oldPath}`; + const newLabel = after.kind === "missing" ? "/dev/null" : `b/${newPath}`; + return withFileModeHeader(oldPath, newPath, before, after, [ + `diff --git a/${oldPath} b/${newPath}`, + `Binary files ${oldLabel} and ${newLabel} differ`, + ].join("\n")); +} + +function withFileModeHeader( + oldPath: string, + newPath: string, + before: FileState, + after: FileState, + patch: string, +): string { + if (before.kind === "missing" && after.kind !== "missing") { + return `diff --git a/${newPath} b/${newPath}\nnew file mode 100644\n${patch}`; + } + if (before.kind !== "missing" && after.kind === "missing") { + return `diff --git a/${oldPath} b/${oldPath}\ndeleted file mode 100644\n${patch}`; + } + return patch; +} + +function stateText(state: FileState): string | undefined { + if (state.kind === "missing") return ""; + if (state.kind !== "file") return undefined; + if (state.bytes.includes(0)) return undefined; + + try { + return new TextDecoder("utf-8", { fatal: true }).decode(state.bytes); + } catch { + return undefined; + } +} + +function patchStats(patch: string): Pick { + const parsed = parseReviewFiles(patch)[0]; + return { + additions: parsed?.additions ?? 0, + removals: parsed?.removals ?? 0, + }; +} + +function fileChangeType(before: FileState, after: FileState): ReviewFile["type"] { + if (before.kind === "missing" && after.kind !== "missing") return "new"; + if (before.kind !== "missing" && after.kind === "missing") return "deleted"; + return "change"; +} + +function sameState(left: FileState, right: FileState): boolean { + if (left.kind !== right.kind) return false; + if (left.kind === "file" && right.kind === "file") return left.bytes.equals(right.bytes); + return true; +} + +function recordMove(moves: Map, fromPath: string, toPath: string): void { + let origin = normalizeRelativePath(fromPath); + const destination = normalizeRelativePath(toPath); + + for (const [candidate, currentDestination] of moves) { + if (currentDestination !== origin) continue; + moves.delete(candidate); + origin = candidate; + break; + } + + if (origin !== destination) moves.set(origin, destination); +} + +function workspaceRelativePath(root: string, path: string): string { + const relationship = relative(root, path); + if ( + relationship === "" || + isAbsolute(relationship) || + relationship === ".." || + relationship.startsWith(`..${sep}`) + ) { + throw new Error(`Review journal path is outside workspace root: ${path}`); + } + return normalizeRelativePath(relationship); +} + +function normalizeRelativePath(path: string): string { + return path.split(sep).join("/").replace(/^\.\//, ""); +} + +function assertWorkspaceRoot(existingRoot: string, workspaceId: string, root: string): void { + if (existingRoot !== root) { + throw new Error(`Review journal workspace root mismatch for ${workspaceId}.`); + } +} diff --git a/src/review-checkpoints.ts b/src/review-checkpoints.ts index ec998154..93a99750 100644 --- a/src/review-checkpoints.ts +++ b/src/review-checkpoints.ts @@ -1,31 +1,16 @@ import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { parsePatchFiles } from "@pierre/diffs"; import { git, getGitEligibility, safeWorkspaceRefSegment } from "./git.js"; +import { + parseReviewFiles, + summarizeReviewFiles, + type ReviewChangesResult, +} from "./review-diff.js"; -export type ReviewSince = "last_shown" | "workspace_open"; - -export interface ReviewSummary { - files: number; - additions: number; - removals: number; -} - -export interface ReviewFile { - path: string; - previousPath?: string; - type: "change" | "rename-pure" | "rename-changed" | "new" | "deleted"; - additions: number; - removals: number; -} +export type { ReviewChangesResult, ReviewFile, ReviewSummary } from "./review-diff.js"; -export interface ReviewChangesResult { - result: string; - summary: ReviewSummary; - files: ReviewFile[]; - patch: string; -} +export type ReviewSince = "last_shown" | "workspace_open"; interface WorkspaceReviewState { root: string; @@ -124,7 +109,7 @@ export function createReviewCheckpointManager(): ReviewCheckpointManager { maxBuffer: REVIEW_DIFF_MAX_BUFFER, })).stdout; const files = parseReviewFiles(patch); - const summary = summarizeFiles(files); + const summary = summarizeReviewFiles(files); if (markReviewed) { await git(state.gitRoot, ["update-ref", state.baselineRef, current]); @@ -148,47 +133,6 @@ export function createReviewCheckpointManager(): ReviewCheckpointManager { }; } -function parseReviewFiles(patch: string): ReviewFile[] { - if (patch.length === 0) return []; - - try { - return parsePatchFiles(patch, "review", true).flatMap((parsedPatch) => - parsedPatch.files.map((file) => { - const stats = file.hunks.reduce( - (total, hunk) => ({ - additions: total.additions + hunk.additionLines, - removals: total.removals + hunk.deletionLines, - }), - { additions: 0, removals: 0 }, - ); - - return { - path: file.name, - previousPath: file.prevName, - type: reviewFileType(file.type), - ...stats, - }; - }), - ); - } catch (error) { - const detail = error instanceof Error ? error.message : String(error); - throw new Error(`Review diff could not be rendered: ${detail}`); - } -} - -function reviewFileType(type: string): ReviewFile["type"] { - switch (type) { - case "rename-pure": - case "rename-changed": - case "new": - case "deleted": - case "change": - return type; - default: - return "change"; - } -} - function assertWorkspaceRoot( state: WorkspaceReviewState | undefined, workspaceId: string, @@ -291,14 +235,3 @@ function checkpointEnv(indexPath: string): NodeJS.ProcessEnv { GIT_COMMITTER_EMAIL: "devspace@users.noreply.local", }; } - -function summarizeFiles(files: ReviewFile[]): ReviewSummary { - return files.reduce( - (summary, file) => ({ - files: summary.files + 1, - additions: summary.additions + file.additions, - removals: summary.removals + file.removals, - }), - { files: 0, additions: 0, removals: 0 }, - ); -} diff --git a/src/review-diff.ts b/src/review-diff.ts new file mode 100644 index 00000000..712e3b28 --- /dev/null +++ b/src/review-diff.ts @@ -0,0 +1,74 @@ +import { parsePatchFiles } from "@pierre/diffs"; + +export interface ReviewSummary { + files: number; + additions: number; + removals: number; +} + +export interface ReviewFile { + path: string; + previousPath?: string; + type: "change" | "rename-pure" | "rename-changed" | "new" | "deleted"; + additions: number; + removals: number; +} + +export interface ReviewChangesResult { + result: string; + summary: ReviewSummary; + files: ReviewFile[]; + patch: string; +} + +export function parseReviewFiles(patch: string): ReviewFile[] { + if (patch.length === 0) return []; + + try { + return parsePatchFiles(patch, "review", true).flatMap((parsedPatch) => + parsedPatch.files.map((file) => { + const stats = file.hunks.reduce( + (total, hunk) => ({ + additions: total.additions + hunk.additionLines, + removals: total.removals + hunk.deletionLines, + }), + { additions: 0, removals: 0 }, + ); + + return { + path: file.name, + previousPath: file.prevName, + type: reviewFileType(file.type), + ...stats, + }; + }), + ); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error(`Review diff could not be rendered: ${detail}`); + } +} + +export function summarizeReviewFiles(files: ReviewFile[]): ReviewSummary { + return files.reduce( + (summary, file) => ({ + files: summary.files + 1, + additions: summary.additions + file.additions, + removals: summary.removals + file.removals, + }), + { files: 0, additions: 0, removals: 0 }, + ); +} + +function reviewFileType(type: string): ReviewFile["type"] { + switch (type) { + case "rename-pure": + case "rename-changed": + case "new": + case "deleted": + case "change": + return type; + default: + return "change"; + } +} diff --git a/src/server.test.ts b/src/server.test.ts index 73eaf03b..3ef43f67 100644 --- a/src/server.test.ts +++ b/src/server.test.ts @@ -125,6 +125,100 @@ test("a host without conversation metadata receives normal explicit-workspace be assert.doesNotMatch(responseText(second), /conversation metadata/i); }); +test("show_changes reviews successful writes outside Git repositories", async (t) => { + const context = await fixture(t, { widgets: "changes" }); + const opened = await callOpen(context.client, context.project, "chat-1"); + const workspaceId = structuredContent(opened).workspaceId as string; + + await context.client.callTool({ + name: "write", + arguments: { + workspaceId, + path: "journal.txt", + content: "journal change\n", + }, + }); + const review = await context.client.callTool({ + name: "show_changes", + arguments: { workspaceId }, + }); + + const card = responseCard(review); + const files = card.files as Array<{ path?: string }>; + const payload = card.payload as { patch?: string }; + assert.deepEqual(files.map((file) => file.path), ["journal.txt"]); + assert.match(payload.patch ?? "", /journal change/); +}); + +test("journal-owned reviews exclude unrelated working-tree changes and keep Git fallback synchronized", async (t) => { + const context = await fixture(t, { git: true, widgets: "changes" }); + const opened = await callOpen(context.client, context.project, "chat-1"); + const workspaceId = structuredContent(opened).workspaceId as string; + + await writeFile(join(context.project, "manual.txt"), "manual change\n"); + await context.client.callTool({ + name: "write", + arguments: { + workspaceId, + path: "agent.txt", + content: "agent change\n", + }, + }); + + const review = await context.client.callTool({ + name: "show_changes", + arguments: { workspaceId }, + }); + const card = responseCard(review); + const files = card.files as Array<{ path?: string }>; + const payload = card.payload as { patch?: string }; + assert.deepEqual(files.map((file) => file.path), ["agent.txt"]); + assert.match(payload.patch ?? "", /agent change/); + assert.doesNotMatch(payload.patch ?? "", /manual change/); + + const nextReview = await context.client.callTool({ + name: "show_changes", + arguments: { workspaceId }, + }); + assert.match(responseText(nextReview), /No changes since last shown changes/); +}); + +test("apply_patch moves are journaled as one net rename outside Git", async (t) => { + const context = await fixture(t, { toolMode: "codex", widgets: "changes" }); + await writeFile(join(context.project, "before.txt"), "before\n"); + const opened = await callOpen(context.client, context.project, "chat-1"); + const workspaceId = structuredContent(opened).workspaceId as string; + + await context.client.callTool({ + name: "apply_patch", + arguments: { + workspaceId, + patch: `*** Begin Patch +*** Update File: before.txt +*** Move to: after.txt +@@ +-before ++after +*** End Patch`, + }, + }); + const review = await context.client.callTool({ + name: "show_changes", + arguments: { workspaceId }, + }); + + const card = responseCard(review); + assert.deepEqual(card.files, [ + { + path: "after.txt", + previousPath: "before.txt", + type: "rename-changed", + additions: 1, + removals: 1, + }, + ]); +}); + test("checkout reuse and context suppression survive a registry restart", async (t) => { const context = await fixture(t); const first = await callOpen(context.client, context.project, "chat-1"); @@ -175,7 +269,14 @@ interface ServerFixture { close: () => Promise; } -async function fixture(t: TestContext, options: { git?: boolean } = {}): Promise { +async function fixture( + t: TestContext, + options: { + git?: boolean; + widgets?: "full" | "changes"; + toolMode?: "full" | "codex"; + } = {}, +): Promise { const root = await mkdtemp(join(tmpdir(), "devspace-server-test-")); const project = join(root, "project"); const agentDir = join(root, "agent"); @@ -208,8 +309,8 @@ async function fixture(t: TestContext, options: { git?: boolean } = {}): Promise DEVSPACE_ALLOWED_ROOTS: root, DEVSPACE_WORKTREE_ROOT: join(root, ".worktrees"), DEVSPACE_AGENT_DIR: agentDir, - DEVSPACE_WIDGETS: "full", - DEVSPACE_TOOL_MODE: "full", + DEVSPACE_WIDGETS: options.widgets ?? "full", + DEVSPACE_TOOL_MODE: options.toolMode ?? "full", DEVSPACE_OAUTH_OWNER_TOKEN: "test-owner-token-that-is-long-enough", PORT: "1", }); diff --git a/src/server.ts b/src/server.ts index 840594ab..2cd932fa 100644 --- a/src/server.ts +++ b/src/server.ts @@ -49,6 +49,11 @@ import { type McpSessionCloseResult, } from "./mcp-sessions.js"; import { ProcessSessionManager, type ProcessSnapshot } from "./process-sessions.js"; +import { + createReviewChangeJournal, + type ReviewMove, + type ReviewMutationCapture, +} from "./review-change-journal.js"; import { createReviewCheckpointManager } from "./review-checkpoints.js"; import { openAiConversationScopeId } from "./request-meta.js"; import { shutdownHttpServer } from "./server-shutdown.js"; @@ -704,6 +709,7 @@ export function createMcpServer( processSessions: ProcessSessionManager, localAgentProviders: LocalAgentProviderAvailability[], incomingArtifactAdapters: readonly IncomingArtifactAdapter[], + reviewJournal = createReviewChangeJournal(), ): McpServer { const server = new McpServer( { @@ -718,6 +724,22 @@ export function createMcpServer( }, ); + const prepareReviewMutation = async ( + workspaceId: string, + root: string, + paths: readonly string[], + ): Promise => { + if (config.widgets !== "changes") return undefined; + return reviewJournal.prepareMutation({ workspaceId, root, paths }); + }; + + const commitReviewMutation = ( + capture: ReviewMutationCapture | undefined, + moves: readonly ReviewMove[] = [], + ): void => { + if (capture) reviewJournal.commitMutation(capture, moves); + }; + registerAppResource( server, "DevSpace Diff Card", @@ -816,6 +838,10 @@ export function createMcpServer( workspaceId: workspace.id, root: workspace.root, }); + reviewJournal.initializeWorkspace({ + workspaceId: workspace.id, + root: workspace.root, + }); } const cardSkills = workspace.skills .filter((skill) => !skill.disableModelInvocation) @@ -1071,7 +1097,8 @@ export function createMcpServer( async ({ workspaceId, ...input }) => { const startedAt = performance.now(); const workspace = workspaces.getWorkspace(workspaceId); - workspaces.resolvePath(workspace, input.path); + const path = workspaces.resolvePath(workspace, input.path); + const reviewMutation = await prepareReviewMutation(workspaceId, workspace.root, [path]); const response = await writeFileTool(input, { cwd: workspace.root, root: workspace.root, @@ -1085,6 +1112,7 @@ export function createMcpServer( }, response.content, startedAt); return response; } + commitReviewMutation(reviewMutation); const patch = newFilePatch(input.path, input.content); const stats = countDiffStats(patch); @@ -1158,7 +1186,8 @@ export function createMcpServer( async ({ workspaceId, ...input }) => { const startedAt = performance.now(); const workspace = workspaces.getWorkspace(workspaceId); - workspaces.resolvePath(workspace, input.path); + const path = workspaces.resolvePath(workspace, input.path); + const reviewMutation = await prepareReviewMutation(workspaceId, workspace.root, [path]); const response = await editFileTool(input, { cwd: workspace.root, root: workspace.root, @@ -1172,6 +1201,7 @@ export function createMcpServer( }, response.content, startedAt); return response; } + commitReviewMutation(reviewMutation); const stats = countDiffStats( response.details?.patch ?? response.details?.diff, @@ -1246,7 +1276,20 @@ export function createMcpServer( async ({ workspaceId, patch }) => { const startedAt = performance.now(); const workspace = workspaces.getWorkspace(workspaceId); - const applied = await applyPatch(workspace.root, patch); + let reviewMutation: ReviewMutationCapture | undefined; + const applied = await applyPatch(workspace.root, patch, { + beforeApply: async ({ paths }) => { + reviewMutation = await prepareReviewMutation(workspaceId, workspace.root, paths); + }, + }); + commitReviewMutation( + reviewMutation, + applied.files.flatMap((file) => + file.operation === "move" && file.previousPath + ? [{ fromPath: file.previousPath, toPath: file.path }] + : [], + ), + ); const paths = applied.files.map((file) => file.path).join(", "); const result = `Applied patch to ${applied.files.length} file(s): ${paths}`; const content = [textBlock(result)]; @@ -1308,11 +1351,41 @@ export function createMcpServer( async ({ workspaceId }) => { const startedAt = performance.now(); const workspace = workspaces.getWorkspace(workspaceId); - const review = await reviewCheckpoints.reviewChanges({ - workspaceId, - root: workspace.root, - markReviewed: true, - }); + const review = reviewJournal.hasTrackedMutations(workspaceId) + ? await (async () => { + const journalReview = await reviewJournal.reviewChanges({ + workspaceId, + root: workspace.root, + }); + try { + const checkpointReview = await reviewCheckpoints.reviewChanges({ + workspaceId, + root: workspace.root, + markReviewed: true, + }); + const journalFiles = reviewFileKeys(journalReview.files); + const checkpointFiles = reviewFileKeys(checkpointReview.files); + if (journalFiles.join("\0") !== checkpointFiles.join("\0")) { + logEvent(config.logging, "debug", "review_source_mismatch", { + workspaceId, + journalFiles, + checkpointFiles, + }); + } + } catch (error) { + logEvent(config.logging, "debug", "review_checkpoint_comparison_unavailable", { + workspaceId, + error: error instanceof Error ? error.message : String(error), + }); + } + reviewJournal.markReviewed({ workspaceId, root: workspace.root }); + return journalReview; + })() + : await reviewCheckpoints.reviewChanges({ + workspaceId, + root: workspace.root, + markReviewed: true, + }); const content = [textBlock(review.result)]; logToolCall(config, { @@ -1690,6 +1763,7 @@ export function createServer( const workspaceStore = createWorkspaceStore(config.stateDir); const workspaces = new WorkspaceRegistry(config, workspaceStore); const reviewCheckpoints = createReviewCheckpointManager(); + const reviewJournal = createReviewChangeJournal(); const processSessions = new ProcessSessionManager(); const localAgentProviders = config.subagents ? getLocalAgentProviderAvailabilitySnapshot() @@ -1855,6 +1929,7 @@ export function createServer( processSessions, localAgentProviders, incomingArtifactAdapters, + reviewJournal, ); await server.connect(transport); } else { @@ -1893,6 +1968,14 @@ export function createServer( }; } +function reviewFileKeys( + files: ReadonlyArray<{ path: string; previousPath?: string }>, +): string[] { + return files + .map((file) => file.previousPath ? `${file.previousPath}->${file.path}` : file.path) + .sort(); +} + async function isMainModule(): Promise { if (!process.argv[1]) return false;