From 4acecaadffd7733723da464eabe5422d3efcd68c Mon Sep 17 00:00:00 2001 From: George Ng Date: Thu, 3 Sep 2026 13:46:04 -0700 Subject: [PATCH 1/4] fix(markdown): persist conflict-safe updates Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/agent/documentUpdatePersistence.ts | 123 +++ .../agents/markdown/src/agent/ipcTypes.ts | 24 +- .../src/agent/markdownActionHandler.ts | 776 ++++++++++-------- .../agents/markdown/src/agent/pathPolicy.ts | 8 + .../agents/markdown/src/view/route/service.ts | 411 +++++----- .../test/markdownActionHandler.spec.ts | 2 +- .../test/markdownUpdatePersistence.spec.ts | 157 ++++ 7 files changed, 971 insertions(+), 530 deletions(-) create mode 100644 ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts create mode 100644 ts/packages/agents/markdown/test/markdownUpdatePersistence.spec.ts diff --git a/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts b/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts new file mode 100644 index 0000000000..61bd69c26f --- /dev/null +++ b/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts @@ -0,0 +1,123 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import fs from "node:fs"; +import path from "node:path"; +import { createHash } from "node:crypto"; +import { applyDocumentOperations } from "./documentOperations.js"; +import type { DocumentOperation } from "./markdownOperationSchema.js"; +import { + isCanonicalDirectory, + resolveExistingFileWithinRoot, + resolveWritableFileWithinRoot, +} from "./pathPolicy.js"; + +export interface DocumentBinding { + token: string | undefined; + root: string; + relativePath: string; + filePath: string; +} + +export interface UpdateExpectations { + bindingToken: string | undefined; + root: string | undefined; + relativePath: string | undefined; + revision: string; + updatedRevision: string | undefined; +} + +export function computeContentRevision(content: string): string { + return createHash("sha256").update(content, "utf8").digest("hex"); +} + +function validateIdentity( + binding: DocumentBinding, + expected: UpdateExpectations, +): void { + if ( + expected.bindingToken !== undefined && + expected.bindingToken !== binding.token + ) { + throw new Error("Document binding token changed"); + } + if (expected.root !== undefined && expected.root !== binding.root) { + throw new Error("Document binding root changed"); + } + if ( + expected.relativePath !== undefined && + expected.relativePath !== binding.relativePath + ) { + throw new Error("Document binding path changed"); + } +} + +function resolveBoundFile(binding: DocumentBinding, write: boolean): string { + if (!isCanonicalDirectory(binding.root)) { + throw new Error("The authorized markdown workspace root changed"); + } + const resolved = write + ? resolveWritableFileWithinRoot(binding.root, binding.relativePath) + : resolveExistingFileWithinRoot(binding.root, binding.relativePath); + if ( + resolved === undefined || + path.relative(resolved, binding.filePath) !== "" + ) { + throw new Error( + "The markdown document binding changed or is outside its authorized workspace", + ); + } + return resolved; +} + +export function readBoundDocument(binding: DocumentBinding) { + const filePath = resolveBoundFile(binding, false); + const content = fs.readFileSync(filePath, "utf-8"); + return { content, revision: computeContentRevision(content), filePath }; +} + +export function persistDocumentOperations( + binding: DocumentBinding, + operations: DocumentOperation[], + expected: UpdateExpectations, +) { + validateIdentity(binding, expected); + let filePath = resolveBoundFile(binding, true); + const currentContent = fs.readFileSync(filePath, "utf-8"); + const currentRevision = computeContentRevision(currentContent); + if (expected.updatedRevision === currentRevision) { + return { + content: currentContent, + revision: currentRevision, + alreadyApplied: true, + filePath, + }; + } + if (currentRevision !== expected.revision) { + throw new Error( + "Document changed between read and apply (revision mismatch)", + ); + } + + const content = applyDocumentOperations(currentContent, operations); + const revision = computeContentRevision(content); + if ( + expected.updatedRevision !== undefined && + expected.updatedRevision !== revision + ) { + throw new Error("Updated document revision does not match operations"); + } + + validateIdentity(binding, expected); + filePath = resolveBoundFile(binding, true); + if ( + computeContentRevision(fs.readFileSync(filePath, "utf-8")) !== + currentRevision + ) { + throw new Error( + "Document changed between validation and write (revision mismatch)", + ); + } + fs.writeFileSync(filePath, content, "utf-8"); + return { content, revision, alreadyApplied: false, filePath }; +} diff --git a/ts/packages/agents/markdown/src/agent/ipcTypes.ts b/ts/packages/agents/markdown/src/agent/ipcTypes.ts index 34f538ce8b..fd3778960a 100644 --- a/ts/packages/agents/markdown/src/agent/ipcTypes.ts +++ b/ts/packages/agents/markdown/src/agent/ipcTypes.ts @@ -36,28 +36,50 @@ export interface UICommandResult { // Agent → View: Content requests export interface GetDocumentContentMessage { type: "getDocumentContent"; + requestId: string; + expectedBindingToken?: string; + expectedRoot?: string; + expectedRelativePath?: string; } export interface DocumentContentMessage { type: "documentContent"; + requestId: string; content: string; - source?: "client-serializer" | "yjs-fallback" | "error"; + source?: "file" | "error"; error?: string; timestamp: number; + bindingToken: string | null; + boundFilePath: string | null; + boundRoot: string | null; + boundRelativePath: string | null; + revision: string | null; + identityMismatch?: boolean; } // Agent → View: LLM operations export interface LLMOperationsMessage { type: "applyLLMOperations"; + requestId: string; operations: any[]; // DocumentOperation[] timestamp: number; + expectedBindingToken?: string; + expectedRoot?: string; + expectedRelativePath?: string; + expectedRevision: string; + expectedUpdatedRevision?: string; } export interface OperationsAppliedMessage { type: "operationsApplied"; + requestId: string; success: boolean; operationCount?: number; error?: string; + identityMismatch?: boolean; + revisionMismatch?: boolean; + bindingToken?: string | null; + revision?: string | null; } // View → Frontend: Auto-save notifications diff --git a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts index 863a2412b9..2217eaf135 100644 --- a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts +++ b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts @@ -30,6 +30,13 @@ import { resolveRealDirectory, resolveWritableFileWithinRoot, } from "./pathPolicy.js"; +import { + computeContentRevision, + persistDocumentOperations, + readBoundDocument, + type DocumentBinding, +} from "./documentUpdatePersistence.js"; +import { applyDocumentOperations } from "./documentOperations.js"; const debug = registerDebug("typeagent:markdown:agent"); @@ -67,6 +74,7 @@ type CurrentMarkdownDocument = type MarkdownActionContext = { currentDocument?: CurrentMarkdownDocument | undefined; + currentBindingToken?: string | undefined; viewProcess?: ChildProcess | undefined; localHostPort: number; // Handle returned by sessionContext.registerPort for the markdown @@ -355,7 +363,7 @@ async function updateMarkdownContext( fullPath, context.agentContext.localHostPort, ) - .then((result) => { + .then(async (result) => { if (!result) { return; } @@ -386,6 +394,22 @@ async function updateMarkdownContext( // view process exists (the earlier call below ran // before it was forked). setCurrentAgentContext(context.agentContext); + if ( + context.agentContext.currentDocument?.source === + "session" + ) { + const binding = await getCurrentDocumentBinding( + context.agentContext, + storage, + ); + if (!("storageKey" in binding)) { + viewProcess.send({ + type: "setFile", + workspaceRoot: binding.root, + relativePath: binding.relativePath, + }); + } + } }) .catch((e) => { console.warn( @@ -420,36 +444,11 @@ async function handleStreamingMarkdownAction( ); const agent = await createMarkdownAgent("GPT_4o"); - const storage = actionContext.sessionContext.sessionStorage; - const viewProcess = getCurrentDocumentViewProcess( - actionContext.sessionContext.agentContext, - ); - - // Get current document content - let markdownContent = ""; - - if (viewProcess) { - try { - markdownContent = await getDocumentContentFromView(viewProcess); - debug( - `Got content from view process for streaming: ${markdownContent?.length || 0} chars`, - ); - } catch (error) { - console.warn( - "[STREAMING] Failed to get content from view, falling back to storage:", - error, - ); - markdownContent = await getCurrentMarkdownContent( - actionContext.sessionContext.agentContext, - storage, - ); - } - } else { - markdownContent = await getCurrentMarkdownContent( - actionContext.sessionContext.agentContext, - storage, - ); - } + const { + content: markdownContent, + binding, + revision, + } = await readCurrentDocumentContent(actionContext); try { // Call agent with streaming callback @@ -498,6 +497,21 @@ async function handleStreamingMarkdownAction( updateResult.operations || [], actionContext, ); + if (updateResult.operations?.length) { + const operations = + updateResult.operations as DocumentOperation[]; + const updatedContent = applyDocumentOperations( + markdownContent, + operations, + ); + await applyOperationsForCurrentDocument( + actionContext, + operations, + binding, + revision, + computeContentRevision(updatedContent), + ); + } return createActionResult( updateResult.operationSummary || @@ -683,8 +697,8 @@ async function handleCreateDocument( if (fullPath) { agentContext.viewProcess.send({ type: "setFile", - filePath: path.basename(fullPath), - folderPath: path.dirname(fullPath), + workspaceRoot: fs.realpathSync(path.dirname(fullPath)), + relativePath: path.basename(fullPath), }); } } @@ -728,6 +742,7 @@ async function handleCreateDocument( }; } + agentContext.currentBindingToken = undefined; const actionLabel = documentExisted ? "opened" : "created"; const documentLocation = absoluteFilePath ?? relativeName; const result = createActionResult( @@ -776,7 +791,8 @@ async function handleOpenDocument( if (fullPath) { agentContext.viewProcess.send({ type: "setFile", - filePath: path.basename(fullPath), + workspaceRoot: fs.realpathSync(path.dirname(fullPath)), + relativePath: path.basename(fullPath), }); } } @@ -804,6 +820,7 @@ async function handleOpenDocument( documentLocation = absoluteFilePath; } + agentContext.currentBindingToken = undefined; const result = createActionResult(`Document opened at ${documentLocation}`); result.resultEntity = { name: relativeName, @@ -820,6 +837,251 @@ async function handleOpenDocument( return result; } +type DocumentUpdateAction = Extract< + MarkdownAction, + { actionName: "updateDocument" | "streamingUpdateDocument" } +>; + +type CurrentDocumentBinding = DocumentBinding | { storageKey: string }; + +async function getCurrentDocumentBinding( + agentContext: MarkdownActionContext, + storage: Storage | undefined, +): Promise { + const currentDocument = agentContext.currentDocument; + if (currentDocument === undefined) { + throw new Error( + "No markdown document is open. Use createDocument or openDocument first.", + ); + } + if (currentDocument.source === "session") { + if (storage === undefined) { + throw new Error("Session storage is unavailable"); + } + if (!getCurrentDocumentViewProcess(agentContext)) { + return { storageKey: currentDocument.storageKey }; + } + const fullPath = await getFullMarkdownFilePath( + currentDocument.storageKey, + storage, + ); + if (fullPath === undefined) { + throw new Error("Current session document has no local file"); + } + return { + token: agentContext.currentBindingToken, + root: fs.realpathSync(path.dirname(fullPath)), + relativePath: path.basename(fullPath), + filePath: fs.realpathSync(fullPath), + }; + } + return { + token: agentContext.currentBindingToken, + root: currentDocument.workspaceRoot, + relativePath: path.relative( + currentDocument.workspaceRoot, + currentDocument.filePath, + ), + filePath: currentDocument.filePath, + }; +} + +async function readCurrentDocumentContent( + actionContext: ActionContext, +): Promise<{ + content: string; + binding: CurrentDocumentBinding; + revision: string; +}> { + const agentContext = actionContext.sessionContext.agentContext; + const storage = actionContext.sessionContext.sessionStorage; + const binding = await getCurrentDocumentBinding(agentContext, storage); + if ("storageKey" in binding) { + const content = await getCurrentMarkdownContent(agentContext, storage); + return { content, binding, revision: computeContentRevision(content) }; + } + const viewProcess = getCurrentDocumentViewProcess(agentContext); + if (!viewProcess) { + const document = readBoundDocument(binding); + return { + content: document.content, + binding, + revision: document.revision, + }; + } + + const response = await getDocumentContentFromView( + viewProcess, + { + expectedBindingToken: binding.token, + expectedRoot: binding.root, + expectedRelativePath: binding.relativePath, + }, + ); + if (response.identityMismatch) { + throw new Error( + "Document identity changed while reading; refusing to update the wrong file", + ); + } + if (response.error) { + throw new Error(response.error); + } + if (typeof response.bindingToken === "string") { + agentContext.currentBindingToken = response.bindingToken; + } + return { + content: response.content, + binding: { + ...binding, + token: agentContext.currentBindingToken, + }, + revision: response.revision ?? computeContentRevision(response.content), + }; +} + +async function applyOperationsForCurrentDocument( + actionContext: ActionContext, + operations: DocumentOperation[], + binding: CurrentDocumentBinding, + revision: string, + expectedUpdatedRevision?: string, +): Promise { + const agentContext = actionContext.sessionContext.agentContext; + const storage = actionContext.sessionContext.sessionStorage; + const currentBinding = await getCurrentDocumentBinding( + agentContext, + storage, + ); + if ("storageKey" in binding) { + if ( + !("storageKey" in currentBinding) || + currentBinding.storageKey !== binding.storageKey + ) { + throw new Error("Document binding changed while generating update"); + } + if (storage === undefined) { + throw new Error("Session storage is unavailable"); + } + const content = await getCurrentMarkdownContent(agentContext, storage); + const currentRevision = computeContentRevision(content); + if (currentRevision === expectedUpdatedRevision) { + return; + } + if (currentRevision !== revision) { + throw new Error( + "Document changed between read and apply (revision mismatch)", + ); + } + const updatedContent = applyDocumentOperations(content, operations); + if ( + expectedUpdatedRevision !== undefined && + computeContentRevision(updatedContent) !== expectedUpdatedRevision + ) { + throw new Error("Updated document revision does not match operations"); + } + await storage.write(binding.storageKey, updatedContent); + return; + } + if ( + "storageKey" in currentBinding || + currentBinding.token !== binding.token || + currentBinding.root !== binding.root || + currentBinding.relativePath !== binding.relativePath || + currentBinding.filePath !== binding.filePath + ) { + throw new Error("Document binding changed while generating update"); + } + const expectations = { + expectedBindingToken: binding.token, + expectedRoot: binding.root, + expectedRelativePath: binding.relativePath, + expectedRevision: revision, + expectedUpdatedRevision, + }; + const viewProcess = getCurrentDocumentViewProcess(agentContext); + if (!viewProcess) { + persistDocumentOperations(binding, operations, { + bindingToken: binding.token, + root: binding.root, + relativePath: binding.relativePath, + revision, + updatedRevision: expectedUpdatedRevision, + }); + return; + } + + const applied = await sendOperationsToView( + viewProcess, + operations, + expectations, + ); + if (applied.success) { + return; + } + if (applied.identityMismatch) { + throw new Error( + "Document identity changed while applying operations; refusing to write to the wrong file", + ); + } + if (applied.revisionMismatch) { + throw new Error( + "Document changed between read and apply; refusing to overwrite (revision mismatch)", + ); + } + throw new Error( + applied.error ?? "Failed to apply operations in view process", + ); +} + +function parseEditorContext(serializedContext: string | undefined): unknown { + if (!serializedContext) { + return undefined; + } + try { + return JSON.parse(serializedContext); + } catch (error) { + debug( + `[AGENT] Failed to parse context JSON: ${ + error instanceof Error ? error.message : String(error) + }, using undefined`, + ); + return undefined; + } +} + +async function updateCurrentDocument( + action: DocumentUpdateAction, + actionContext: ActionContext, + agent: Awaited>, +): Promise { + const { content, binding, revision } = + await readCurrentDocumentContent(actionContext); + const response = await agent.updateDocument( + content, + action.parameters.originalRequest, + action.parameters.cursorPosition, + parseEditorContext(action.parameters.context), + ); + if (!response.success) { + const message = + (response as { message?: string }).message ?? + "Unknown error occurred"; + return createActionResult(`Failed to update document: ${message}`); + } + + if (response.data.operations?.length) { + await applyOperationsForCurrentDocument( + actionContext, + response.data.operations, + binding, + revision, + ); + } + return createActionResult( + response.data.operationSummary ?? "Updated document", + ); +} + async function handleMarkdownAction( action: MarkdownAction, actionContext: ActionContext, @@ -840,8 +1102,6 @@ async function handleMarkdownAction( return agent; }; - const storage = actionContext.sessionContext.sessionStorage; - switch (action.actionName) { case "createDocument": { result = await handleCreateDocument(action, actionContext); @@ -851,245 +1111,10 @@ async function handleMarkdownAction( result = await handleOpenDocument(action, actionContext); break; } - case "updateDocument": { - const agent = await createAgent(); - debug("Starting updateDocument action in agent process"); - result = createActionResult("Updating document ..."); - const viewProcess = getCurrentDocumentViewProcess( - actionContext.sessionContext.agentContext, - ); - - let markdownContent = ""; - - if (viewProcess) { - try { - markdownContent = - await getDocumentContentFromView(viewProcess); - debug( - `Got content from view process: ${markdownContent?.length || 0} chars`, - ); - debug( - `Content preview: ${markdownContent?.substring(0, 200)}...`, - ); - } catch (error) { - console.warn( - "Failed to get content from view, reading the current document directly:", - error, - ); - markdownContent = await getCurrentMarkdownContent( - actionContext.sessionContext.agentContext, - storage, - ); - } - } else { - markdownContent = await getCurrentMarkdownContent( - actionContext.sessionContext.agentContext, - storage, - ); - debug( - "No view process, read current document content:", - markdownContent.length, - "chars", - ); - } - - // Handle synchronous requests through the agent - const originalRequest = - "originalRequest" in action.parameters - ? action.parameters.originalRequest - : ""; - - const cursorPosition = - "cursorPosition" in action.parameters - ? action.parameters.cursorPosition - : undefined; - - const context = - "context" in action.parameters && action.parameters.context - ? (() => { - try { - return JSON.parse(action.parameters.context); - } catch (error) { - debug( - `[AGENT] Failed to parse context JSON: ${error}, using undefined`, - ); - return undefined; - } - })() - : undefined; - - debug( - `[AGENT] About to call LLM service with request: "${originalRequest}"`, - ); - debug( - `[AGENT] Document content length: ${markdownContent?.length || 0} chars`, - ); - - const response = await agent.updateDocument( - markdownContent, - originalRequest, - cursorPosition, - context, - ); - - debug(`[AGENT] LLM service returned, success: ${response.success}`); - - if (response.success) { - const updateResult = response.data; - debug( - `[AGENT] LLM processing successful, operations count: ${updateResult.operations?.length || 0}`, - ); - - // Apply operations to the document - if ( - updateResult.operations && - updateResult.operations.length > 0 - ) { - // Send operations to view process for application - if (viewProcess) { - debug( - "Agent sending operations to view process for Yjs application", - ); - - const success = await sendOperationsToView( - viewProcess, - updateResult.operations, - ); - - if (!success) { - throw new Error( - "Failed to apply operations in view process", - ); - } - - debug( - "Operations applied successfully via view process", - ); - } else { - console.warn( - "No view process available, operations not applied", - ); - } - } else { - debug("[AGENT] No operations returned from LLM"); - } - - if (updateResult.operationSummary) { - result = createActionResult(updateResult.operationSummary); - } else { - result = createActionResult("Updated document"); - } - - debug(`[AGENT] updateDocument case completed successfully`); - } else { - const errorMessage = - (response as any).message || "Unknown error occurred"; - console.error("Translation failed:", errorMessage); - result = createActionResult( - "Failed to update document: " + errorMessage, - ); - } - break; - } + case "updateDocument": case "streamingUpdateDocument": { const agent = await createAgent(); - // Handle streaming AI commands - now unified with regular updateDocument flow - debug( - "Starting streamingUpdateDocument action - using standard translator flow", - ); - result = createActionResult("Updating document ..."); - const viewProcess = getCurrentDocumentViewProcess( - actionContext.sessionContext.agentContext, - ); - - let markdownContent = ""; - - if (viewProcess) { - try { - markdownContent = - await getDocumentContentFromView(viewProcess); - debug( - `Got content from view process: ${markdownContent?.length || 0} chars`, - ); - debug( - `Content preview: ${markdownContent?.substring(0, 200)}...`, - ); - } catch (error) { - console.warn( - "Failed to get content from view, reading the current document directly:", - error, - ); - markdownContent = await getCurrentMarkdownContent( - actionContext.sessionContext.agentContext, - storage, - ); - } - } else { - markdownContent = await getCurrentMarkdownContent( - actionContext.sessionContext.agentContext, - storage, - ); - debug( - "No view process, read current document content:", - markdownContent.length, - "chars", - ); - } - - // Handle streaming requests through the standard agent (same as updateDocument) - const response = await agent.updateDocument( - markdownContent, - action.parameters.originalRequest, - ); - - if (response.success) { - const updateResult = response.data; - - // Apply operations to the document - if ( - updateResult.operations && - updateResult.operations.length > 0 - ) { - // Send operations to view process for application - if (viewProcess) { - debug( - "Agent sending operations to view process for Yjs application", - ); - - const success = await sendOperationsToView( - viewProcess, - updateResult.operations, - ); - - if (!success) { - throw new Error( - "Failed to apply operations in view process", - ); - } - - debug( - "Operations applied successfully via view process", - ); - } else { - console.warn( - "No view process available, operations not applied", - ); - } - } - - if (updateResult.operationSummary) { - result = createActionResult(updateResult.operationSummary); - } else { - result = createActionResult("Updated document"); - } - } else { - const errorMessage = - (response as any).message || "Unknown error occurred"; - console.error("Translation failed:", errorMessage); - result = createActionResult( - "Failed to update document: " + errorMessage, - ); - } + result = await updateCurrentDocument(action, actionContext, agent); break; } } @@ -1103,104 +1128,144 @@ async function handleMarkdownAction( return result; } -/** - * Send operations to view process for application (Flow 1 implementation) - */ -async function sendOperationsToView( +let applyRequestCounter = 0; + +type ApplyExpectations = { + expectedBindingToken: string | undefined; + expectedRoot: string; + expectedRelativePath: string; + expectedRevision: string; + expectedUpdatedRevision: string | undefined; +}; + +type ApplyResult = { + success: boolean; + identityMismatch: boolean; + revisionMismatch: boolean; + error: string | undefined; +}; + +export async function sendOperationsToView( viewProcess: ChildProcess | undefined, operations: DocumentOperation[], -): Promise { + expectations: ApplyExpectations, +): Promise { if (!viewProcess) { - return false; + return { + success: false, + identityMismatch: false, + revisionMismatch: false, + error: "No view process", + }; } + const requestId = `apply_${++applyRequestCounter}`; return new Promise((resolve) => { const timeout = setTimeout(() => { console.error("[AGENT] View process operation timeout"); - resolve(false); - }, 5000); - - // Listen for response - const responseHandler = (message: any) => { - if (message.type === "operationsApplied") { - clearTimeout(timeout); - viewProcess.off("message", responseHandler); - - if (message.success) { - resolve(true); - } else { - console.error( - "[AGENT] View failed to apply operations:", - message.error, - ); - resolve(false); - } + viewProcess.off("message", responseHandler); + resolve({ + success: false, + identityMismatch: false, + revisionMismatch: false, + error: "View process operation timeout", + }); + }, 15000); + + const responseHandler = (message: Record) => { + if ( + message.type !== "operationsApplied" || + message.requestId !== requestId + ) { + return; } + clearTimeout(timeout); + viewProcess.off("message", responseHandler); + resolve({ + success: message.success === true, + identityMismatch: message.identityMismatch === true, + revisionMismatch: message.revisionMismatch === true, + error: + typeof message.error === "string" + ? message.error + : undefined, + }); }; viewProcess.on("message", responseHandler); - - // Send operations viewProcess.send({ type: "applyLLMOperations", - operations: operations, + requestId, + operations, timestamp: Date.now(), + ...expectations, }); - - debug(`[AGENT] Sent ${operations.length} operations to view process`); }); } -/** - * Get document content from view process (Flow 1 implementation) - */ -async function getDocumentContentFromView( - viewProcess: ChildProcess, -): Promise { - return new Promise((resolve, reject) => { - const timeout = setTimeout(() => { - debug( - "[AGENT] Content request timeout, trying fallback to empty content", - ); - - // Use empty content as fallback when view process fails - // This allows the agent to continue processing even if content retrieval fails - console.warn( - "[AGENT] View process content request timed out, using empty content fallback", - ); - resolve(""); - }, 15000); // 15 second timeout +type ViewDocumentContentResponse = { + content: string; + bindingToken: string | null; + revision: string | null; + identityMismatch: boolean; + error: string | undefined; +}; - const responseHandler = (message: any) => { - if (message.type === "documentContent") { - clearTimeout(timeout); - viewProcess.off("message", responseHandler); +type ReadExpectations = { + expectedBindingToken: string | undefined; + expectedRoot: string | undefined; + expectedRelativePath: string | undefined; +}; - // Log the source of the content for debugging - const source = message.source || "unknown"; - debug( - `[AGENT] Received document content from ${source}: ${message.content?.length || 0} chars`, - ); +let getContentRequestCounter = 0; - if (message.error) { - debug( - `[AGENT] Content retrieval had error: ${message.error}`, - ); - // Still resolve with content even if there was an error - } +export async function getDocumentContentFromView( + viewProcess: ChildProcess, + expectations: ReadExpectations, +): Promise { + const requestId = `get_${++getContentRequestCounter}`; + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + viewProcess.off("message", responseHandler); + reject(new Error("View process content request timed out")); + }, 15000); - resolve(message.content || ""); + const responseHandler = (message: Record) => { + if ( + message.type !== "documentContent" || + message.requestId !== requestId + ) { + return; } + clearTimeout(timeout); + viewProcess.off("message", responseHandler); + resolve({ + content: + typeof message.content === "string" ? message.content : "", + bindingToken: + typeof message.bindingToken === "string" + ? message.bindingToken + : null, + revision: + typeof message.revision === "string" + ? message.revision + : null, + identityMismatch: message.identityMismatch === true, + error: + typeof message.error === "string" + ? message.error + : undefined, + }); }; viewProcess.on("message", responseHandler); - - debug("[AGENT] Sending getDocumentContent request to view process"); - viewProcess.send({ type: "getDocumentContent" }); + viewProcess.send({ + type: "getDocumentContent", + requestId, + ...expectations, + }); }); } -// NOTE: Function commented out per Flow 1 consolidation -// Collaboration server now managed by view process async function createViewServiceHost( filePath: string, @@ -1237,7 +1302,8 @@ async function createViewServiceHost( childProcess.send({ type: "setFile", - filePath: path.basename(filePath), + workspaceRoot: folderPath, + relativePath: path.basename(filePath), }); childProcess.on("message", function (message: any) { @@ -1265,6 +1331,7 @@ async function createViewServiceHost( // Global process message handler for UI commands let currentAgentContext: MarkdownActionContext | null = null; +const wiredViewProcesses = new WeakSet(); // Store agent context for UI command processing export function setCurrentAgentContext(context: MarkdownActionContext) { @@ -1272,9 +1339,28 @@ export function setCurrentAgentContext(context: MarkdownActionContext) { const viewProcess = context.viewProcess; - if (typeof viewProcess !== "undefined" && viewProcess.on) { + if ( + typeof viewProcess !== "undefined" && + viewProcess.on && + !wiredViewProcesses.has(viewProcess) + ) { + wiredViewProcesses.add(viewProcess); viewProcess.on("message", async (message: any) => { - if (message.type === "uiCommand" && currentAgentContext) { + if (message.type === "bindingUpdated" && currentAgentContext) { + if ( + message.boundRoot === + currentAgentContext.currentWorkspaceRoot && + message.boundRelativePath === + currentAgentContext.currentFileName && + message.boundFilePath === + currentAgentContext.currentFilePath + ) { + currentAgentContext.currentBindingToken = + typeof message.bindingToken === "string" + ? message.bindingToken + : undefined; + } + } else if (message.type === "uiCommand" && currentAgentContext) { debug( `[AGENT] Received UI command: ${message.command}, requestId: ${message.requestId}, cursorPosition: ${message.parameters?.cursorPosition}, context: ${message.parameters?.context ? "serialized" : "none"}`, ); diff --git a/ts/packages/agents/markdown/src/agent/pathPolicy.ts b/ts/packages/agents/markdown/src/agent/pathPolicy.ts index c93deaf3e6..0a31590e4a 100644 --- a/ts/packages/agents/markdown/src/agent/pathPolicy.ts +++ b/ts/packages/agents/markdown/src/agent/pathPolicy.ts @@ -79,6 +79,14 @@ export function resolveRealDirectory(absolutePath: string): string | undefined { } } +export function isCanonicalDirectory(absolutePath: string): boolean { + const canonicalPath = resolveRealDirectory(absolutePath); + return ( + canonicalPath !== undefined && + path.relative(path.resolve(absolutePath), canonicalPath) === "" + ); +} + export function resolveExistingFileWithinRoot( root: string, requestedPath: string, diff --git a/ts/packages/agents/markdown/src/view/route/service.ts b/ts/packages/agents/markdown/src/view/route/service.ts index 8e8f518e8c..33cd72972a 100644 --- a/ts/packages/agents/markdown/src/view/route/service.ts +++ b/ts/packages/agents/markdown/src/view/route/service.ts @@ -18,12 +18,22 @@ import * as encoding from "lib0/encoding"; import * as decoding from "lib0/decoding"; import registerDebug from "debug"; import sanitizeFilename from "sanitize-filename"; +import { randomUUID } from "node:crypto"; import { isAllowedViewOrigin } from "./originAllowlist.js"; +import { resolvePathWithinRoot } from "./pathPolicy.js"; import { + isCanonicalDirectory, + normalizeRelativeDocumentPath, resolveExistingFileWithinRoot, - resolvePathWithinRoot, + resolveRealDirectory, resolveWritableFileWithinRoot, -} from "./pathPolicy.js"; +} from "../../agent/pathPolicy.js"; +import { + persistDocumentOperations, + readBoundDocument, + type DocumentBinding, +} from "../../agent/documentUpdatePersistence.js"; +import type { DocumentOperation } from "../../agent/markdownOperationSchema.js"; const debug = registerDebug("typeagent:markdown:service"); @@ -107,7 +117,7 @@ app.post( // Construct and normalize file path const documentPath = resolvePathWithinRoot( - ROOT_DIR, + getValidatedCurrentRoot(), `${sanitizedDocumentName}.md`, ); @@ -121,7 +131,7 @@ app.post( } let safeDocumentPath = resolveWritableFileWithinRoot( - ROOT_DIR, + getValidatedCurrentRoot(), documentPath, ); if (safeDocumentPath === undefined) { @@ -142,6 +152,9 @@ app.post( } filePath = safeDocumentPath; + boundRelativePath = `${sanitizedDocumentName}.md`; + bindingToken = randomUUID(); + notifyBindingToParent(); // Initialize collaboration for new document const documentId = sanitizedDocumentName; @@ -231,7 +244,9 @@ app.post( ); let clients: any[] = []; -let filePath: string | null; +let filePath: string | null = null; +let boundRelativePath: string | null = null; +let bindingToken: string | null = null; let collaborationManager: CollaborationManager; // UI Command routing state @@ -242,8 +257,63 @@ const pendingCommands = new Map(); let markdownRequestCounter = 0; const pendingMarkdownRequests = new Map(); const userHomeDir = os.homedir(); -const ROOT_DIR = +const INITIAL_ROOT_DIR = process.env.TYPEAGENT_MARKDOWN_ROOT || path.join(userHomeDir, "Documents"); +let currentRoot = + resolveRealDirectory(INITIAL_ROOT_DIR) ?? path.resolve(INITIAL_ROOT_DIR); + +function getValidatedCurrentRoot(): string { + if (!isCanonicalDirectory(currentRoot)) { + throw new Error("The document root is no longer accessible"); + } + return currentRoot; +} + +type BindingSnapshot = { + bindingToken: string | null; + currentRoot: string; + filePath: string | null; + boundRelativePath: string | null; +}; + +function captureBindingSnapshot(): BindingSnapshot { + return { bindingToken, currentRoot, filePath, boundRelativePath }; +} + +function bindingError( + message: Record, + snapshot: BindingSnapshot, +): string | undefined { + if ( + typeof message.expectedBindingToken === "string" && + message.expectedBindingToken !== snapshot.bindingToken + ) { + return "Document binding token changed"; + } + if ( + typeof message.expectedRoot === "string" && + message.expectedRoot !== snapshot.currentRoot + ) { + return "Document binding root changed"; + } + if ( + typeof message.expectedRelativePath === "string" && + message.expectedRelativePath !== snapshot.boundRelativePath + ) { + return "Document binding path changed"; + } + return undefined; +} + +function notifyBindingToParent(): void { + process.send?.({ + type: "bindingUpdated", + bindingToken, + boundFilePath: filePath, + boundRoot: filePath ? currentRoot : null, + boundRelativePath, + }); +} // Streaming state for LLM responses const activeStreamingSessions = new Map< @@ -440,6 +510,8 @@ async function requestMarkdownFromClient(retryCount: number = 0): Promise<{ }); } +void requestMarkdownFromClient; + /** * Determine if a command should use streaming */ @@ -579,7 +651,7 @@ app.post("/document", express.json(), (req: Request, res: Response) => { try { const writableFilePath = resolveWritableFileWithinRoot( - ROOT_DIR, + getValidatedCurrentRoot(), filePath, ); if (writableFilePath === undefined) { @@ -691,7 +763,7 @@ app.post("/autosave", express.json(), (req: Request, res: Response) => { } const resolvedFilePath = resolvePathWithinRoot( - ROOT_DIR, + getValidatedCurrentRoot(), sanitizedFilePath, ); if (resolvedFilePath === undefined) { @@ -850,7 +922,7 @@ app.post("/file/load", express.json(), (req: Request, res: Response) => { } const resolvedPath = resolveExistingFileWithinRoot( - ROOT_DIR, + getValidatedCurrentRoot(), newFilePath, ); if (resolvedPath === undefined) { @@ -862,6 +934,12 @@ app.post("/file/load", express.json(), (req: Request, res: Response) => { // Set new file path filePath = resolvedPath; + boundRelativePath = path + .relative(currentRoot, resolvedPath) + .split(path.sep) + .join("/"); + bindingToken = randomUUID(); + notifyBindingToParent(); // Initialize collaboration for new document const documentId = path.basename(resolvedPath, ".md"); @@ -1501,11 +1579,22 @@ process.on("message", async (message: any) => { ); if (message.type == "setFile") { - if (message.filePath) { - // Resolve and validate the file path + if (message.relativePath) { + const nextRoot = + typeof message.workspaceRoot === "string" && + isCanonicalDirectory(message.workspaceRoot) + ? resolveRealDirectory(message.workspaceRoot) + : undefined; + const relativePath = normalizeRelativeDocumentPath( + message.relativePath, + ); + if (nextRoot === undefined || relativePath === undefined) { + debug("Invalid document binding provided in message"); + return; + } const resolvedFilePath = resolveWritableFileWithinRoot( - ROOT_DIR, - path.basename(message.filePath), + nextRoot, + relativePath, ); if (resolvedFilePath === undefined) { debug("Invalid file path provided in message"); @@ -1513,10 +1602,14 @@ process.on("message", async (message: any) => { } const oldFilePath = filePath; + currentRoot = nextRoot; filePath = resolvedFilePath; + boundRelativePath = relativePath; + bindingToken = randomUUID(); + notifyBindingToParent(); // Initialize collaboration for this document using authoritative document - const documentId = path.basename(message.filePath, ".md"); + const documentId = path.basename(relativePath, ".md"); // Get or create the authoritative Y.js document const ydoc = getAuthoritativeDocument(documentId); @@ -1531,7 +1624,7 @@ process.on("message", async (message: any) => { ytext.insert(0, content); // Insert file content debug( - `File loaded into authoritative document: ${documentId}, ${content.length} chars from ${message.filePath}`, + `File loaded into authoritative document: ${documentId}, ${content.length} chars from ${relativePath}`, ); } else { debug( @@ -1547,10 +1640,8 @@ process.on("message", async (message: any) => { `data: ${JSON.stringify({ type: "documentChanged", newDocumentId: documentId, - newDocumentName: path.basename( - message.filePath, - ".md", - ), + newDocumentName: path.basename(relativePath, ".md"), + bindingToken, timestamp: Date.now(), })}\n\n`, ); @@ -1559,6 +1650,9 @@ process.on("message", async (message: any) => { } else { // No file mode - initialize with default content using authoritative document filePath = null; + boundRelativePath = null; + bindingToken = null; + notifyBindingToParent(); debug("Running in memory-only mode (no file)"); const documentId = "default"; @@ -1635,198 +1729,149 @@ Start typing to see the editor in action! ); }); } else if (message.type === "applyLLMOperations") { - // PRODUCTION: Send operations to PRIMARY client only via SSE to prevent duplicates + const requestId = + typeof message.requestId === "string" ? message.requestId : ""; + const snapshot = captureBindingSnapshot(); try { - debug( - `[VIEW] Forwarding ${message.operations?.length || 0} operations to primary client via SSE`, - ); - - if (clients.length === 0) { - console.warn( - `[SSE] No clients connected to receive operations`, - ); + if ( + !Array.isArray(message.operations) || + !snapshot.filePath || + !snapshot.boundRelativePath || + typeof message.expectedRevision !== "string" + ) { + throw new Error("Invalid document update request"); + } + const identityError = bindingError(message, snapshot); + if (identityError) { process.send?.({ type: "operationsApplied", + requestId, success: false, - error: "No clients connected", - method: "sse-forwarded", + identityMismatch: true, + error: identityError, + bindingToken: snapshot.bindingToken, }); return; } - // Send operations to ONLY the first client to prevent duplicates - const primaryClient = clients[0]; - const operationsEvent = { - type: "llmOperations", - operations: message.operations, - timestamp: message.timestamp || Date.now(), - source: "agent", - clientRole: "primary", // Mark this client as the primary applier + const binding: DocumentBinding = { + token: snapshot.bindingToken ?? undefined, + root: snapshot.currentRoot, + relativePath: snapshot.boundRelativePath, + filePath: snapshot.filePath, }; + const persisted = persistDocumentOperations( + binding, + message.operations as DocumentOperation[], + { + bindingToken: + typeof message.expectedBindingToken === "string" + ? message.expectedBindingToken + : undefined, + root: + typeof message.expectedRoot === "string" + ? message.expectedRoot + : undefined, + relativePath: + typeof message.expectedRelativePath === "string" + ? message.expectedRelativePath + : undefined, + revision: message.expectedRevision, + updatedRevision: + typeof message.expectedUpdatedRevision === "string" + ? message.expectedUpdatedRevision + : undefined, + }, + ); - try { - primaryClient.write( - `data: ${JSON.stringify(operationsEvent)}\n\n`, - ); - debug( - `[SSE] Sent ${message.operations?.length || 0} operations to PRIMARY client (${clients.indexOf(primaryClient)} of ${clients.length} clients)`, - ); - - debug(`data: ${JSON.stringify(operationsEvent)}\n\n`); - - // Notify other clients that operations are being applied (optional) - if (clients.length > 1) { - const notificationEvent = { - type: "operationsBeingApplied", - timestamp: Date.now(), - operationCount: message.operations?.length || 0, - source: "agent", - }; - - clients.slice(1).forEach((client, index) => { - try { - client.write( - `data: ${JSON.stringify(notificationEvent)}\n\n`, - ); - debug( - `[SSE] Notified secondary client ${index + 1} of pending operations`, - ); - } catch (error) { - console.error( - `[SSE] Failed to notify secondary client ${index + 1}:`, - error, - ); - } - }); - } - } catch (error) { - console.error( - "[SSE] Failed to send operations to primary client:", - error, - ); - throw error; - } - - // Send success confirmation back to agent + const documentId = path.basename(snapshot.boundRelativePath, ".md"); + collaborationManager.setDocumentContent( + documentId, + persisted.content, + ); process.send?.({ type: "operationsApplied", + requestId, success: true, - operationCount: message.operations?.length || 0, - method: "sse-forwarded", - clientsNotified: clients.length, + operationCount: message.operations.length, + bindingToken: snapshot.bindingToken, + revision: persisted.revision, }); - - debug(`[VIEW] Operations forwarded to primary client successfully`); } catch (error) { - console.error( - "[VIEW] Failed to forward operations via SSE:", - error, - ); + const errorMessage = + error instanceof Error ? error.message : "Unknown error"; process.send?.({ type: "operationsApplied", + requestId, success: false, - error: error instanceof Error ? error.message : "Unknown error", - method: "sse-forwarded", + identityMismatch: /binding|workspace root/.test(errorMessage), + revisionMismatch: /revision mismatch/.test(errorMessage), + error: errorMessage, + bindingToken: snapshot.bindingToken, }); } } else if (message.type === "getDocumentContent") { - debug( - `[VIEW] Processing getDocumentContent request at ${new Date().toISOString()}`, - ); - // Handle content requests from agent - try client markdown first, fallback to Y.js - // Process this asynchronously to avoid blocking other messages - (async () => { - try { - let documentId = ""; - - if (!filePath) { - // Use default document ID for memory-only mode - documentId = "default"; - } else { - documentId = path.basename(filePath, ".md"); - } - - debug("Using documentID " + documentId); - - let content = ""; - let source = "unknown"; - - try { - // PRIMARY: Try to get proper markdown from connected client - if (clients.length > 0) { - debug( - `[VIEW] Attempting to get markdown from connected client...`, - ); - const markdownResponse = - await requestMarkdownFromClient(); - content = markdownResponse.markdown; - source = "client-serializer"; - debug( - `[VIEW] Retrieved markdown from client: ${content.length} chars`, - ); - } else { - throw new Error("No clients connected"); - } - } catch (clientError) { - const errorMessage = - clientError instanceof Error - ? clientError.message - : String(clientError); - debug( - `[VIEW] Failed to get markdown from client (${errorMessage}), falling back to Y.js`, - ); - - // FALLBACK: Get content from authoritative Y.js document - const ydoc = getAuthoritativeDocument(documentId); - const yText = ydoc.getText("content"); - content = yText.toString(); - source = "yjs-fallback"; - debug( - `[VIEW] Retrieved content from Y.js fallback: ${content.length} chars`, - ); - - // If Y.js is also empty, try reading from file as last resort - if (!content && filePath && fs.existsSync(filePath)) { - try { - content = fs.readFileSync(filePath, "utf-8"); - source = "file-fallback"; - debug( - `[VIEW] Retrieved content from file fallback: ${content.length} chars`, - ); - } catch (fileError) { - debug( - `[VIEW] File fallback also failed: ${fileError}`, - ); - } - } - } - - debug( - `[VIEW] Sending document content to agent (source: ${source}, ${content.length} chars)`, - ); - - process.send?.({ - type: "documentContent", - content: content, - source: source, - timestamp: Date.now(), - }); - - debug("[SENT] [VIEW] Sent document content to agent process"); - } catch (error) { - console.error("[VIEW] Failed to get document content:", error); + const requestId = + typeof message.requestId === "string" ? message.requestId : ""; + const snapshot = captureBindingSnapshot(); + try { + const identityError = bindingError(message, snapshot); + if (identityError) { process.send?.({ type: "documentContent", + requestId, content: "", source: "error", - error: - error instanceof Error - ? error.message - : "Unknown error", + error: identityError, + identityMismatch: true, + bindingToken: snapshot.bindingToken, + boundFilePath: snapshot.filePath, + boundRoot: snapshot.filePath ? snapshot.currentRoot : null, + boundRelativePath: snapshot.boundRelativePath, + revision: null, timestamp: Date.now(), }); + return; + } + if (!snapshot.filePath || !snapshot.boundRelativePath) { + throw new Error("No markdown document is bound"); } - })(); + const document = readBoundDocument({ + token: snapshot.bindingToken ?? undefined, + root: snapshot.currentRoot, + relativePath: snapshot.boundRelativePath, + filePath: snapshot.filePath, + }); + process.send?.({ + type: "documentContent", + requestId, + content: document.content, + source: "file", + bindingToken: snapshot.bindingToken, + boundFilePath: snapshot.filePath, + boundRoot: snapshot.currentRoot, + boundRelativePath: snapshot.boundRelativePath, + revision: document.revision, + timestamp: Date.now(), + }); + } catch (error) { + const errorMessage = + error instanceof Error ? error.message : "Unknown error"; + process.send?.({ + type: "documentContent", + requestId, + content: "", + source: "error", + error: errorMessage, + identityMismatch: /binding|workspace root/.test(errorMessage), + bindingToken: snapshot.bindingToken, + boundFilePath: snapshot.filePath, + boundRoot: snapshot.filePath ? snapshot.currentRoot : null, + boundRelativePath: snapshot.boundRelativePath, + revision: null, + timestamp: Date.now(), + }); + } } else if (message.type === "uiCommandResult") { // Handle UI command results from agent debug( diff --git a/ts/packages/agents/markdown/test/markdownActionHandler.spec.ts b/ts/packages/agents/markdown/test/markdownActionHandler.spec.ts index e8af7a07d3..4afe66f06e 100644 --- a/ts/packages/agents/markdown/test/markdownActionHandler.spec.ts +++ b/ts/packages/agents/markdown/test/markdownActionHandler.spec.ts @@ -23,7 +23,7 @@ type TestAgentContext = { }; describe("markdown document creation", () => { - let workspace: string; + let workspace = ""; beforeEach(() => { workspace = fs.mkdtempSync( diff --git a/ts/packages/agents/markdown/test/markdownUpdatePersistence.spec.ts b/ts/packages/agents/markdown/test/markdownUpdatePersistence.spec.ts new file mode 100644 index 0000000000..fb23d6a6e1 --- /dev/null +++ b/ts/packages/agents/markdown/test/markdownUpdatePersistence.spec.ts @@ -0,0 +1,157 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import type { ChildProcess } from "node:child_process"; +import { EventEmitter } from "node:events"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { + computeContentRevision, + persistDocumentOperations, + readBoundDocument, + type DocumentBinding, +} from "../src/agent/documentUpdatePersistence.js"; +import { + getDocumentContentFromView, + sendOperationsToView, +} from "../src/agent/markdownActionHandler.js"; + +describe("markdown update persistence", () => { + let temporaryDirectory: string; + let workspace: string; + let filePath: string; + let binding: DocumentBinding; + + beforeEach(() => { + temporaryDirectory = fs.mkdtempSync( + path.join(os.tmpdir(), "typeagent-markdown-update-"), + ); + workspace = path.join(temporaryDirectory, "workspace"); + fs.mkdirSync(path.join(workspace, "notes"), { recursive: true }); + workspace = fs.realpathSync(workspace); + filePath = path.join(workspace, "notes", "plan.md"); + fs.writeFileSync(filePath, "original", "utf-8"); + binding = { + token: "binding-1", + root: workspace, + relativePath: "notes/plan.md", + filePath, + }; + }); + + afterEach(() => { + fs.rmSync(temporaryDirectory, { recursive: true, force: true }); + }); + + test("persists operations and treats a repeated apply as complete", () => { + const operation = { + type: "insert" as const, + position: 8, + content: [{ type: "text" as const, text: " updated" }], + }; + const expected = { + bindingToken: binding.token, + root: binding.root, + relativePath: binding.relativePath, + revision: computeContentRevision("original"), + updatedRevision: computeContentRevision("original updated"), + }; + + expect( + persistDocumentOperations(binding, [operation], expected) + .alreadyApplied, + ).toBe(false); + expect( + persistDocumentOperations(binding, [operation], expected) + .alreadyApplied, + ).toBe(true); + expect(fs.readFileSync(filePath, "utf-8")).toBe("original updated"); + }); + + test("rejects stale revisions and binding identities", () => { + fs.writeFileSync(filePath, "changed", "utf-8"); + expect(() => + persistDocumentOperations(binding, [], { + bindingToken: binding.token, + root: binding.root, + relativePath: binding.relativePath, + revision: computeContentRevision("original"), + updatedRevision: undefined, + }), + ).toThrow(/revision mismatch/); + expect(() => + persistDocumentOperations(binding, [], { + bindingToken: "binding-2", + root: binding.root, + relativePath: binding.relativePath, + revision: computeContentRevision("changed"), + updatedRevision: undefined, + }), + ).toThrow(/binding token changed/); + }); + + test("rejects a workspace replaced by a junction", () => { + const movedWorkspace = path.join(temporaryDirectory, "moved-workspace"); + const outside = path.join(temporaryDirectory, "outside"); + fs.mkdirSync(outside); + fs.writeFileSync(path.join(outside, "plan.md"), "outside", "utf-8"); + fs.renameSync(workspace, movedWorkspace); + fs.symlinkSync(outside, workspace, "junction"); + + expect(() => readBoundDocument(binding)).toThrow( + /workspace root changed/, + ); + expect(fs.readFileSync(path.join(outside, "plan.md"), "utf-8")).toBe( + "outside", + ); + fs.unlinkSync(workspace); + }); + + test("correlates concurrent view reads and applies", async () => { + const view = new EventEmitter() as EventEmitter & { + send: (message: Record) => void; + }; + view.send = (message) => { + const requestId = message.requestId as string; + queueMicrotask(() => { + view.emit("message", { + type: + message.type === "getDocumentContent" + ? "documentContent" + : "operationsApplied", + requestId: "unrelated", + success: false, + }); + view.emit("message", { + type: + message.type === "getDocumentContent" + ? "documentContent" + : "operationsApplied", + requestId, + content: "original", + bindingToken: binding.token, + revision: computeContentRevision("original"), + success: true, + }); + }); + }; + const child = view as unknown as ChildProcess; + const identity = { + expectedBindingToken: binding.token, + expectedRoot: binding.root, + expectedRelativePath: binding.relativePath, + }; + + await expect( + getDocumentContentFromView(child, identity), + ).resolves.toMatchObject({ content: "original" }); + await expect( + sendOperationsToView(child, [], { + ...identity, + expectedRevision: computeContentRevision("original"), + expectedUpdatedRevision: undefined, + }), + ).resolves.toMatchObject({ success: true }); + }); +}); From 03089cfc1e3f2505e26556fbb04169799a007bb4 Mon Sep 17 00:00:00 2001 From: George Ng Date: Thu, 3 Sep 2026 13:50:52 -0700 Subject: [PATCH 2/4] refactor(markdown): keep persistence layer focused Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/agent/documentUpdatePersistence.ts | 20 +++--- .../src/agent/markdownActionHandler.ts | 71 ++++++++++++------- .../agents/markdown/src/agent/pathPolicy.ts | 8 --- .../agents/markdown/src/view/route/service.ts | 20 ++++-- 4 files changed, 69 insertions(+), 50 deletions(-) diff --git a/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts b/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts index 61bd69c26f..83806e9914 100644 --- a/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts +++ b/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts @@ -7,8 +7,7 @@ import { createHash } from "node:crypto"; import { applyDocumentOperations } from "./documentOperations.js"; import type { DocumentOperation } from "./markdownOperationSchema.js"; import { - isCanonicalDirectory, - resolveExistingFileWithinRoot, + resolveRealDirectory, resolveWritableFileWithinRoot, } from "./pathPolicy.js"; @@ -52,13 +51,14 @@ function validateIdentity( } } -function resolveBoundFile(binding: DocumentBinding, write: boolean): string { - if (!isCanonicalDirectory(binding.root)) { +function resolveBoundFile(binding: DocumentBinding): string { + if (resolveRealDirectory(binding.root) !== binding.root) { throw new Error("The authorized markdown workspace root changed"); } - const resolved = write - ? resolveWritableFileWithinRoot(binding.root, binding.relativePath) - : resolveExistingFileWithinRoot(binding.root, binding.relativePath); + const resolved = resolveWritableFileWithinRoot( + binding.root, + binding.relativePath, + ); if ( resolved === undefined || path.relative(resolved, binding.filePath) !== "" @@ -71,7 +71,7 @@ function resolveBoundFile(binding: DocumentBinding, write: boolean): string { } export function readBoundDocument(binding: DocumentBinding) { - const filePath = resolveBoundFile(binding, false); + const filePath = resolveBoundFile(binding); const content = fs.readFileSync(filePath, "utf-8"); return { content, revision: computeContentRevision(content), filePath }; } @@ -82,7 +82,7 @@ export function persistDocumentOperations( expected: UpdateExpectations, ) { validateIdentity(binding, expected); - let filePath = resolveBoundFile(binding, true); + let filePath = resolveBoundFile(binding); const currentContent = fs.readFileSync(filePath, "utf-8"); const currentRevision = computeContentRevision(currentContent); if (expected.updatedRevision === currentRevision) { @@ -109,7 +109,7 @@ export function persistDocumentOperations( } validateIdentity(binding, expected); - filePath = resolveBoundFile(binding, true); + filePath = resolveBoundFile(binding); if ( computeContentRevision(fs.readFileSync(filePath, "utf-8")) !== currentRevision diff --git a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts index 2217eaf135..5414e1e6f0 100644 --- a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts +++ b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts @@ -65,6 +65,7 @@ type CurrentMarkdownDocument = | { source: "session"; storageKey: string; + binding?: DocumentBinding; } | { source: "workspace"; @@ -351,6 +352,7 @@ async function updateMarkdownContext( storage, ); if (fullPath) { + currentDocument.binding = createSessionFileBinding(fullPath); process.env.MARKDOWN_FILE = fullPath; // Fork the express view service in the background instead of // blocking agent enable (and therefore agent-server startup) @@ -363,7 +365,7 @@ async function updateMarkdownContext( fullPath, context.agentContext.localHostPort, ) - .then(async (result) => { + .then((result) => { if (!result) { return; } @@ -398,11 +400,9 @@ async function updateMarkdownContext( context.agentContext.currentDocument?.source === "session" ) { - const binding = await getCurrentDocumentBinding( - context.agentContext, - storage, - ); - if (!("storageKey" in binding)) { + const binding = + context.agentContext.currentDocument.binding; + if (binding) { viewProcess.send({ type: "setFile", workspaceRoot: binding.root, @@ -695,6 +695,8 @@ async function handleCreateDocument( storage, ); if (fullPath) { + agentContext.currentDocument.binding = + createSessionFileBinding(fullPath); agentContext.viewProcess.send({ type: "setFile", workspaceRoot: fs.realpathSync(path.dirname(fullPath)), @@ -789,6 +791,8 @@ async function handleOpenDocument( storage, ); if (fullPath) { + agentContext.currentDocument.binding = + createSessionFileBinding(fullPath); agentContext.viewProcess.send({ type: "setFile", workspaceRoot: fs.realpathSync(path.dirname(fullPath)), @@ -844,6 +848,15 @@ type DocumentUpdateAction = Extract< type CurrentDocumentBinding = DocumentBinding | { storageKey: string }; +function createSessionFileBinding(fullPath: string): DocumentBinding { + return { + token: undefined, + root: fs.realpathSync(path.dirname(fullPath)), + relativePath: path.basename(fullPath), + filePath: fs.realpathSync(fullPath), + }; +} + async function getCurrentDocumentBinding( agentContext: MarkdownActionContext, storage: Storage | undefined, @@ -855,6 +868,15 @@ async function getCurrentDocumentBinding( ); } if (currentDocument.source === "session") { + if ( + getCurrentDocumentViewProcess(agentContext) && + currentDocument.binding + ) { + return { + ...currentDocument.binding, + token: agentContext.currentBindingToken, + }; + } if (storage === undefined) { throw new Error("Session storage is unavailable"); } @@ -869,10 +891,8 @@ async function getCurrentDocumentBinding( throw new Error("Current session document has no local file"); } return { + ...createSessionFileBinding(fullPath), token: agentContext.currentBindingToken, - root: fs.realpathSync(path.dirname(fullPath)), - relativePath: path.basename(fullPath), - filePath: fs.realpathSync(fullPath), }; } return { @@ -910,14 +930,11 @@ async function readCurrentDocumentContent( }; } - const response = await getDocumentContentFromView( - viewProcess, - { - expectedBindingToken: binding.token, - expectedRoot: binding.root, - expectedRelativePath: binding.relativePath, - }, - ); + const response = await getDocumentContentFromView(viewProcess, { + expectedBindingToken: binding.token, + expectedRoot: binding.root, + expectedRelativePath: binding.relativePath, + }); if (response.identityMismatch) { throw new Error( "Document identity changed while reading; refusing to update the wrong file", @@ -977,7 +994,9 @@ async function applyOperationsForCurrentDocument( expectedUpdatedRevision !== undefined && computeContentRevision(updatedContent) !== expectedUpdatedRevision ) { - throw new Error("Updated document revision does not match operations"); + throw new Error( + "Updated document revision does not match operations", + ); } await storage.write(binding.storageKey, updatedContent); return; @@ -1346,14 +1365,16 @@ export function setCurrentAgentContext(context: MarkdownActionContext) { ) { wiredViewProcesses.add(viewProcess); viewProcess.on("message", async (message: any) => { - if (message.type === "bindingUpdated" && currentAgentContext) { + if ( + message.type === "bindingUpdated" && + currentAgentContext?.currentDocument?.source === "session" + ) { + const binding = currentAgentContext.currentDocument.binding; if ( - message.boundRoot === - currentAgentContext.currentWorkspaceRoot && - message.boundRelativePath === - currentAgentContext.currentFileName && - message.boundFilePath === - currentAgentContext.currentFilePath + binding && + message.boundRoot === binding.root && + message.boundRelativePath === binding.relativePath && + message.boundFilePath === binding.filePath ) { currentAgentContext.currentBindingToken = typeof message.bindingToken === "string" diff --git a/ts/packages/agents/markdown/src/agent/pathPolicy.ts b/ts/packages/agents/markdown/src/agent/pathPolicy.ts index 0a31590e4a..c93deaf3e6 100644 --- a/ts/packages/agents/markdown/src/agent/pathPolicy.ts +++ b/ts/packages/agents/markdown/src/agent/pathPolicy.ts @@ -79,14 +79,6 @@ export function resolveRealDirectory(absolutePath: string): string | undefined { } } -export function isCanonicalDirectory(absolutePath: string): boolean { - const canonicalPath = resolveRealDirectory(absolutePath); - return ( - canonicalPath !== undefined && - path.relative(path.resolve(absolutePath), canonicalPath) === "" - ); -} - export function resolveExistingFileWithinRoot( root: string, requestedPath: string, diff --git a/ts/packages/agents/markdown/src/view/route/service.ts b/ts/packages/agents/markdown/src/view/route/service.ts index 33cd72972a..88d3c59492 100644 --- a/ts/packages/agents/markdown/src/view/route/service.ts +++ b/ts/packages/agents/markdown/src/view/route/service.ts @@ -22,9 +22,7 @@ import { randomUUID } from "node:crypto"; import { isAllowedViewOrigin } from "./originAllowlist.js"; import { resolvePathWithinRoot } from "./pathPolicy.js"; import { - isCanonicalDirectory, normalizeRelativeDocumentPath, - resolveExistingFileWithinRoot, resolveRealDirectory, resolveWritableFileWithinRoot, } from "../../agent/pathPolicy.js"; @@ -262,8 +260,16 @@ const INITIAL_ROOT_DIR = let currentRoot = resolveRealDirectory(INITIAL_ROOT_DIR) ?? path.resolve(INITIAL_ROOT_DIR); +function resolveCanonicalRoot(root: string): string | undefined { + const canonicalRoot = resolveRealDirectory(root); + return canonicalRoot !== undefined && + path.relative(path.resolve(root), canonicalRoot) === "" + ? canonicalRoot + : undefined; +} + function getValidatedCurrentRoot(): string { - if (!isCanonicalDirectory(currentRoot)) { + if (resolveCanonicalRoot(currentRoot) === undefined) { throw new Error("The document root is no longer accessible"); } return currentRoot; @@ -921,11 +927,11 @@ app.post("/file/load", express.json(), (req: Request, res: Response) => { return; } - const resolvedPath = resolveExistingFileWithinRoot( + const resolvedPath = resolveWritableFileWithinRoot( getValidatedCurrentRoot(), newFilePath, ); - if (resolvedPath === undefined) { + if (resolvedPath === undefined || !fs.existsSync(resolvedPath)) { res.status(403).json({ error: "Access to the file is forbidden or file not found", }); @@ -1582,8 +1588,8 @@ process.on("message", async (message: any) => { if (message.relativePath) { const nextRoot = typeof message.workspaceRoot === "string" && - isCanonicalDirectory(message.workspaceRoot) - ? resolveRealDirectory(message.workspaceRoot) + resolveCanonicalRoot(message.workspaceRoot) !== undefined + ? resolveCanonicalRoot(message.workspaceRoot) : undefined; const relativePath = normalizeRelativeDocumentPath( message.relativePath, From 6bdab85fa5f0c0422f57400a0d4db38f004dbd32 Mon Sep 17 00:00:00 2001 From: George Ng Date: Wed, 9 Sep 2026 16:51:29 -0700 Subject: [PATCH 3/4] fix(markdown): address persistence review feedback Clarify document path policy, preserve existing-only loads, synchronize changed Yjs spans, and read bound documents asynchronously with identity checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../{pathPolicy.ts => documentPathPolicy.ts} | 1 + .../src/agent/documentUpdatePersistence.ts | 58 +++++- .../src/agent/markdownActionHandler.ts | 4 +- .../src/view/route/collaborationManager.ts | 61 ++++++- .../agents/markdown/src/view/route/service.ts | 17 +- .../markdown/test/boundDocumentRead.spec.ts | 151 +++++++++++++++ .../test/collaborationManager.spec.ts | 83 +++++++++ .../markdown/test/creationPathPolicy.spec.ts | 2 +- .../markdown/test/markdownService.spec.ts | 172 ++++++++++++++++++ .../test/markdownUpdatePersistence.spec.ts | 4 +- 10 files changed, 536 insertions(+), 17 deletions(-) rename ts/packages/agents/markdown/src/agent/{pathPolicy.ts => documentPathPolicy.ts} (98%) create mode 100644 ts/packages/agents/markdown/test/boundDocumentRead.spec.ts create mode 100644 ts/packages/agents/markdown/test/collaborationManager.spec.ts create mode 100644 ts/packages/agents/markdown/test/markdownService.spec.ts diff --git a/ts/packages/agents/markdown/src/agent/pathPolicy.ts b/ts/packages/agents/markdown/src/agent/documentPathPolicy.ts similarity index 98% rename from ts/packages/agents/markdown/src/agent/pathPolicy.ts rename to ts/packages/agents/markdown/src/agent/documentPathPolicy.ts index c93deaf3e6..3a17a87f17 100644 --- a/ts/packages/agents/markdown/src/agent/pathPolicy.ts +++ b/ts/packages/agents/markdown/src/agent/documentPathPolicy.ts @@ -4,6 +4,7 @@ import fs from "node:fs"; import path from "node:path"; +// Filesystem policy for durable documents, including nested document creation. type RootPaths = { resolvedRoot: string; canonicalRoot: string; diff --git a/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts b/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts index 83806e9914..818f72cf09 100644 --- a/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts +++ b/ts/packages/agents/markdown/src/agent/documentUpdatePersistence.ts @@ -8,8 +8,8 @@ import { applyDocumentOperations } from "./documentOperations.js"; import type { DocumentOperation } from "./markdownOperationSchema.js"; import { resolveRealDirectory, - resolveWritableFileWithinRoot, -} from "./pathPolicy.js"; + resolveExistingFileWithinRoot, +} from "./documentPathPolicy.js"; export interface DocumentBinding { token: string | undefined; @@ -55,12 +55,16 @@ function resolveBoundFile(binding: DocumentBinding): string { if (resolveRealDirectory(binding.root) !== binding.root) { throw new Error("The authorized markdown workspace root changed"); } - const resolved = resolveWritableFileWithinRoot( + const resolved = resolveExistingFileWithinRoot( binding.root, binding.relativePath, ); if ( resolved === undefined || + path.relative( + resolved, + path.resolve(binding.root, binding.relativePath), + ) !== "" || path.relative(resolved, binding.filePath) !== "" ) { throw new Error( @@ -70,12 +74,56 @@ function resolveBoundFile(binding: DocumentBinding): string { return resolved; } -export function readBoundDocument(binding: DocumentBinding) { +export async function readBoundDocument(binding: DocumentBinding) { + binding = { ...binding }; const filePath = resolveBoundFile(binding); - const content = fs.readFileSync(filePath, "utf-8"); + const rootIdentity = fs.statSync(binding.root, { bigint: true }); + const fileIdentity = fs.statSync(filePath, { bigint: true }); + const file = await fs.promises.open(filePath, "r"); + let content: string; + try { + const openedIdentity = await file.stat({ bigint: true }); + if (!sameFileIdentity(fileIdentity, openedIdentity)) { + throw new Error("Document binding file changed while opening"); + } + // Check the opened handle before reading, not just its pathname. + resolveBoundFile(binding); + content = await file.readFile("utf-8"); + } finally { + await file.close(); + } + + // The root or file may have been replaced during any of the awaits above. + resolveBoundFile(binding); + if ( + !sameFileIdentity( + rootIdentity, + fs.statSync(binding.root, { bigint: true }), + ) + ) { + throw new Error("The authorized markdown workspace root changed"); + } + const currentIdentity = fs.statSync(filePath, { bigint: true }); + if (!sameFileIdentity(fileIdentity, currentIdentity)) { + throw new Error("Document binding file changed while reading"); + } + // ctime can change on a read on Windows; it is not a content revision. + if ( + fileIdentity.size !== currentIdentity.size || + fileIdentity.mtimeNs !== currentIdentity.mtimeNs + ) { + throw new Error("Document changed while reading (revision mismatch)"); + } return { content, revision: computeContentRevision(content), filePath }; } +function sameFileIdentity( + left: fs.BigIntStats, + right: fs.BigIntStats, +): boolean { + return left.dev === right.dev && left.ino === right.ino; +} + export function persistDocumentOperations( binding: DocumentBinding, operations: DocumentOperation[], diff --git a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts index 5414e1e6f0..39e013b870 100644 --- a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts +++ b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts @@ -29,7 +29,7 @@ import { resolveExistingFileWithinRoot, resolveRealDirectory, resolveWritableFileWithinRoot, -} from "./pathPolicy.js"; +} from "./documentPathPolicy.js"; import { computeContentRevision, persistDocumentOperations, @@ -922,7 +922,7 @@ async function readCurrentDocumentContent( } const viewProcess = getCurrentDocumentViewProcess(agentContext); if (!viewProcess) { - const document = readBoundDocument(binding); + const document = await readBoundDocument(binding); return { content: document.content, binding, diff --git a/ts/packages/agents/markdown/src/view/route/collaborationManager.ts b/ts/packages/agents/markdown/src/view/route/collaborationManager.ts index f75cd8e06d..42e68d463c 100644 --- a/ts/packages/agents/markdown/src/view/route/collaborationManager.ts +++ b/ts/packages/agents/markdown/src/view/route/collaborationManager.ts @@ -147,7 +147,7 @@ export class CollaborationManager { } /** - * Set document content from string + * Synchronize a snapshot while retaining unchanged Yjs text identities. */ setDocumentContent(documentId: string, content: string): void { let ydoc = this.documents.get(documentId); @@ -157,8 +157,52 @@ export class CollaborationManager { } const ytext = ydoc.getText("content"); - ytext.delete(0, ytext.length); - ytext.insert(0, content); + const current = ytext.toString(); + if (current === content) { + return; + } + + // Diff against live Y.Text, not operations whose offsets refer to disk. + let start = 0; + while ( + start < current.length && + start < content.length && + current[start] === content[start] + ) { + start++; + } + if ( + splitsSurrogatePair(current, start) || + splitsSurrogatePair(content, start) + ) { + start--; + } + let currentEnd = current.length; + let contentEnd = content.length; + while ( + currentEnd > start && + contentEnd > start && + current[currentEnd - 1] === content[contentEnd - 1] + ) { + currentEnd--; + contentEnd--; + } + if ( + splitsSurrogatePair(current, currentEnd) || + splitsSurrogatePair(content, contentEnd) + ) { + currentEnd++; + contentEnd++; + } + + ydoc.transact(() => { + if (currentEnd > start) { + ytext.delete(start, currentEnd - start); + } + if (contentEnd > start) { + ytext.insert(start, content.slice(start, contentEnd)); + } + }); } /** @@ -231,3 +275,14 @@ export class CollaborationManager { } } } + +function splitsSurrogatePair(text: string, offset: number): boolean { + const before = text.charCodeAt(offset - 1); + const after = text.charCodeAt(offset); + return ( + before >= 0xd800 && + before <= 0xdbff && + after >= 0xdc00 && + after <= 0xdfff + ); +} diff --git a/ts/packages/agents/markdown/src/view/route/service.ts b/ts/packages/agents/markdown/src/view/route/service.ts index 88d3c59492..5eead72f02 100644 --- a/ts/packages/agents/markdown/src/view/route/service.ts +++ b/ts/packages/agents/markdown/src/view/route/service.ts @@ -23,9 +23,10 @@ import { isAllowedViewOrigin } from "./originAllowlist.js"; import { resolvePathWithinRoot } from "./pathPolicy.js"; import { normalizeRelativeDocumentPath, + resolveExistingFileWithinRoot, resolveRealDirectory, resolveWritableFileWithinRoot, -} from "../../agent/pathPolicy.js"; +} from "../../agent/documentPathPolicy.js"; import { persistDocumentOperations, readBoundDocument, @@ -927,11 +928,11 @@ app.post("/file/load", express.json(), (req: Request, res: Response) => { return; } - const resolvedPath = resolveWritableFileWithinRoot( + const resolvedPath = resolveExistingFileWithinRoot( getValidatedCurrentRoot(), newFilePath, ); - if (resolvedPath === undefined || !fs.existsSync(resolvedPath)) { + if (resolvedPath === undefined) { res.status(403).json({ error: "Access to the file is forbidden or file not found", }); @@ -1842,12 +1843,20 @@ Start typing to see the editor in action! if (!snapshot.filePath || !snapshot.boundRelativePath) { throw new Error("No markdown document is bound"); } - const document = readBoundDocument({ + const document = await readBoundDocument({ token: snapshot.bindingToken ?? undefined, root: snapshot.currentRoot, relativePath: snapshot.boundRelativePath, filePath: snapshot.filePath, }); + if ( + bindingToken !== snapshot.bindingToken || + currentRoot !== snapshot.currentRoot || + filePath !== snapshot.filePath || + boundRelativePath !== snapshot.boundRelativePath + ) { + throw new Error("Document binding changed while reading"); + } process.send?.({ type: "documentContent", requestId, diff --git a/ts/packages/agents/markdown/test/boundDocumentRead.spec.ts b/ts/packages/agents/markdown/test/boundDocumentRead.spec.ts new file mode 100644 index 0000000000..6da8173cba --- /dev/null +++ b/ts/packages/agents/markdown/test/boundDocumentRead.spec.ts @@ -0,0 +1,151 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { jest } from "@jest/globals"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { + computeContentRevision, + readBoundDocument, + type DocumentBinding, +} from "../src/agent/documentUpdatePersistence.js"; + +describe("asynchronous bound document reads", () => { + let temporaryDirectory: string; + let binding: DocumentBinding; + + beforeEach(() => { + temporaryDirectory = fs.mkdtempSync( + path.join(os.tmpdir(), "typeagent-markdown-read-"), + ); + const root = path.join(temporaryDirectory, "workspace"); + fs.mkdirSync(root); + const filePath = path.join(fs.realpathSync(root), "plan.md"); + fs.writeFileSync(filePath, "original"); + binding = { + token: "binding", + root: fs.realpathSync(root), + relativePath: "plan.md", + filePath, + }; + }); + + afterEach(() => { + jest.restoreAllMocks(); + fs.rmSync(temporaryDirectory, { recursive: true, force: true }); + }); + + test("reads a large UTF-8 document without synchronous content I/O", async () => { + const content = "# Large 😀 document\n".repeat(200_000); + fs.writeFileSync(binding.filePath, content); + const syncRead = jest + .spyOn(fs, "readFileSync") + .mockImplementation(() => { + throw new Error("Synchronous content reads are forbidden"); + }); + const result = await readBoundDocument(binding); + expect(result).toEqual({ + content, + filePath: binding.filePath, + revision: computeContentRevision(content), + }); + expect(syncRead).not.toHaveBeenCalled(); + }); + + test("missing nested documents do not create directories", async () => { + binding.relativePath = "missing/nested/plan.md"; + binding.filePath = path.join(binding.root, binding.relativePath); + await expect(readBoundDocument(binding)).rejects.toThrow( + /binding changed/, + ); + expect(fs.existsSync(path.join(binding.root, "missing"))).toBe(false); + }); + + async function readWhileSuspended( + phase: "open" | "close", + mutate: () => void, + ) { + let pause!: () => void; + let resume!: () => void; + const paused = new Promise((resolve) => { + pause = resolve; + }); + const resumed = new Promise((resolve) => { + resume = resolve; + }); + const open = fs.promises.open; + jest.spyOn(fs.promises, "open").mockImplementationOnce( + async (...args) => { + const handle = await open(...args); + if (phase === "open") { + pause(); + await resumed; + } else { + const close = handle.close.bind(handle); + jest.spyOn(handle, "close").mockImplementationOnce( + async () => { + await close(); + pause(); + await resumed; + }, + ); + } + return handle; + }, + ); + const reading = readBoundDocument(binding); + await paused; + try { + mutate(); + } finally { + resume(); + } + return reading; + } + + test.each(["open", "close"] as const)( + "rejects a replaced file during %s", + async (phase) => { + await expect( + readWhileSuspended(phase, () => { + fs.renameSync(binding.filePath, binding.filePath + ".old"); + fs.writeFileSync(binding.filePath, "replacement"); + }), + ).rejects.toThrow(/binding file changed/); + }, + ); + + test("rejects in-place content changes while closing", async () => { + await expect( + readWhileSuspended("close", () => { + fs.writeFileSync(binding.filePath, "changed document"); + }), + ).rejects.toThrow(/revision mismatch/); + }); + + test("rejects a root rebound to a junction while closing", async () => { + const outside = path.join(temporaryDirectory, "outside"); + fs.mkdirSync(outside); + fs.writeFileSync(path.join(outside, "plan.md"), "outside"); + await expect( + readWhileSuspended("close", () => { + fs.renameSync(binding.root, binding.root + ".old"); + fs.symlinkSync(outside, binding.root, "junction"); + }), + ).rejects.toThrow(/workspace root changed/); + }); + + test("rejects a root replacement even if the same file is moved back", async () => { + await expect( + readWhileSuspended("close", () => { + fs.renameSync(binding.root, binding.root + ".old"); + fs.mkdirSync(binding.root); + fs.renameSync( + path.join(binding.root + ".old", "plan.md"), + binding.filePath, + ); + }), + ).rejects.toThrow(/workspace root changed/); + }); +}); diff --git a/ts/packages/agents/markdown/test/collaborationManager.spec.ts b/ts/packages/agents/markdown/test/collaborationManager.spec.ts new file mode 100644 index 0000000000..219ca115df --- /dev/null +++ b/ts/packages/agents/markdown/test/collaborationManager.spec.ts @@ -0,0 +1,83 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { jest } from "@jest/globals"; +import * as Y from "yjs"; +import { CollaborationManager } from "../src/view/route/collaborationManager.js"; + +describe("collaborative snapshot synchronization", () => { + test.each<[string, string, string, number, number, string]>([ + ["one word", "a red fox", "a blue fox", 2, 3, "blue"], + ["insert", "abcd", "abXYcd", 2, 0, "XY"], + ["delete", "abXYcd", "abcd", 2, 2, ""], + ["empty source", "", "new", 0, 0, "new"], + ["empty target", "old", "", 0, 3, ""], + ["no common text", "old", "new", 0, 3, "new"], + ["shared high surrogate", "a😀z", "a😁z", 1, 2, "😁"], + ["shared low surrogate", "a😀z", "a🨀z", 1, 2, "🨀"], + ["emoji insert", "az", "a😀z", 1, 0, "😀"], + ["emoji delete", "a😀z", "az", 1, 2, ""], + ["combining mark", "cafe\u0301!", "cafe!", 4, 1, ""], + [ + "divergent live text", + "prefix stale suffix", + "prefix saved suffix", + 8, + 4, + "aved", + ], + ])( + "%s changes only the differing span", + (_name, before, after, start, count, inserted) => { + const manager = new CollaborationManager(); + const doc = new Y.Doc(); + const text = doc.getText("content"); + text.insert(0, before); + manager.useExistingDocument("doc", doc, null); + const remove = jest.spyOn(text, "delete"); + const insert = jest.spyOn(text, "insert"); + const updated = jest.fn(); + doc.on("update", updated); + + manager.setDocumentContent("doc", after); + + expect(text.toString()).toBe(after); + expect(remove.mock.calls).toEqual(count ? [[start, count]] : []); + expect(insert.mock.calls).toEqual( + inserted ? [[start, inserted]] : [], + ); + expect(updated).toHaveBeenCalledTimes(1); + doc.destroy(); + }, + ); + + test("no-op emits no updates and leaves relative positions intact", () => { + const manager = new CollaborationManager(); + const doc = new Y.Doc(); + const text = doc.getText("content"); + text.insert(0, "a red fox"); + manager.useExistingDocument("doc", doc, null); + const prefix = Y.createRelativePositionFromTypeIndex(text, 1); + const suffix = Y.createRelativePositionFromTypeIndex(text, 6); + const updated = jest.fn(); + doc.on("update", updated); + + manager.setDocumentContent("doc", "a red fox"); + expect(updated).not.toHaveBeenCalled(); + manager.setDocumentContent("doc", "a blue fox"); + expect( + Y.createAbsolutePositionFromRelativePosition(prefix, doc)?.index, + ).toBe(1); + expect( + Y.createAbsolutePositionFromRelativePosition(suffix, doc)?.index, + ).toBe(7); + expect(updated).toHaveBeenCalledTimes(1); + doc.destroy(); + }); + + test("initializes an absent document", () => { + const manager = new CollaborationManager(); + manager.setDocumentContent("new", "content"); + expect(manager.getDocumentContent("new")).toBe("content"); + }); +}); diff --git a/ts/packages/agents/markdown/test/creationPathPolicy.spec.ts b/ts/packages/agents/markdown/test/creationPathPolicy.spec.ts index 5872c5bc7e..391f9f604a 100644 --- a/ts/packages/agents/markdown/test/creationPathPolicy.spec.ts +++ b/ts/packages/agents/markdown/test/creationPathPolicy.spec.ts @@ -9,7 +9,7 @@ import { resolveExistingFileWithinRoot, resolveRealDirectory, resolveWritableFileWithinRoot, -} from "../src/agent/pathPolicy.js"; +} from "../src/agent/documentPathPolicy.js"; describe("markdown creation path policy", () => { let temporaryDirectory: string; diff --git a/ts/packages/agents/markdown/test/markdownService.spec.ts b/ts/packages/agents/markdown/test/markdownService.spec.ts new file mode 100644 index 0000000000..3ceecae6c4 --- /dev/null +++ b/ts/packages/agents/markdown/test/markdownService.spec.ts @@ -0,0 +1,172 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { fork, type ChildProcess } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +// Suspend the real reader after opening its handle so rebinding is deterministic. +const readGatePreload = ` +import fs from "node:fs"; +const open = fs.promises.open; +fs.promises.open = async (...args) => { + const handle = await open(...args); + process.send?.({ type: "readPaused" }); + await new Promise(resolve => { + const release = message => { + if (message.type === "releaseRead") { + process.off("message", release); + resolve(); + } + }; + process.on("message", release); + }); + return handle; +}; +`; + +function nextMessage(child: ChildProcess, type: string) { + return new Promise>((resolve, reject) => { + const timer = setTimeout(() => { + cleanup(); + reject(new Error(`Timed out waiting for ${type}`)); + }, 10_000); + const receive = (message: Record) => { + if (message.type === type) { + cleanup(); + resolve(message); + } + }; + const failed = (error: Error) => { + cleanup(); + reject(error); + }; + const exited = () => failed(new Error(`Service exited before ${type}`)); + function cleanup() { + clearTimeout(timer); + child.off("message", receive); + child.off("error", failed); + child.off("exit", exited); + } + child.on("message", receive); + child.on("error", failed); + child.on("exit", exited); + }); +} + +describe("markdown service document reads", () => { + let temporaryDirectory: string; + let root: string; + let child: ChildProcess; + let port: number; + + beforeEach(async () => { + temporaryDirectory = fs.mkdtempSync( + path.join(os.tmpdir(), "typeagent-markdown-service-"), + ); + root = fs.realpathSync(temporaryDirectory); + fs.writeFileSync(path.join(root, "plan.md"), "original"); + fs.writeFileSync(path.join(root, "other.md"), "other"); + child = fork( + fileURLToPath(new URL("../view/route/service.js", import.meta.url)), + ["0"], + { + env: { ...process.env, TYPEAGENT_MARKDOWN_ROOT: root }, + execArgv: [ + "--import", + `data:text/javascript,${encodeURIComponent(readGatePreload)}`, + ], + stdio: ["ignore", "ignore", "inherit", "ipc"], + }, + ); + const ready = await nextMessage(child, "Success"); + port = ready.port as number; + }); + + afterEach(async () => { + if (child && child.exitCode === null && child.signalCode === null) { + await new Promise((resolve) => { + child.once("exit", () => resolve()); + child.kill(); + }); + } + fs.rmSync(temporaryDirectory, { recursive: true, force: true }); + }); + + async function bind(relativePath: string, workspaceRoot = root) { + const bound = nextMessage(child, "bindingUpdated"); + child.send({ type: "setFile", relativePath, workspaceRoot }); + return bound; + } + + async function load(filePath: string) { + return fetch(`http://127.0.0.1:${port}/file/load`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath }), + }); + } + + test("loading a missing nested document has no filesystem side effects", async () => { + const response = await load("missing/nested/document.md"); + expect(response.status).toBe(403); + expect(fs.existsSync(path.join(root, "missing"))).toBe(false); + }); + + test("loads only existing files under the current authorized root", async () => { + const workspace = path.join(root, "workspace"); + fs.mkdirSync(workspace); + fs.writeFileSync(path.join(workspace, "current.md"), "current"); + await bind("current.md", workspace); + + const response = await load("current.md"); + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ content: "current" }); + expect((await load("../plan.md")).status).toBe(403); + expect((await load(path.join(root, "plan.md"))).status).toBe(403); + expect((await load(".")).status).toBe(403); + + fs.symlinkSync(root, path.join(workspace, "escape"), "junction"); + expect((await load("escape/plan.md")).status).toBe(403); + }); + + test("returns the durable snapshot after an asynchronous read", async () => { + await bind("plan.md"); + const paused = nextMessage(child, "readPaused"); + const response = nextMessage(child, "documentContent"); + child.send({ type: "getDocumentContent", requestId: "read-1" }); + await paused; + child.send({ type: "releaseRead" }); + expect(await response).toMatchObject({ + requestId: "read-1", + content: "original", + source: "file", + }); + }); + + test.each(["other.md", "plan.md"])( + "rejects a read when the service rebinds to %s during suspension", + async (nextPath) => { + const binding = await bind("plan.md"); + const paused = nextMessage(child, "readPaused"); + const response = nextMessage(child, "documentContent"); + child.send({ + type: "getDocumentContent", + requestId: "read-1", + expectedBindingToken: binding.bindingToken, + }); + await paused; + await bind(nextPath); + child.send({ type: "releaseRead" }); + expect(await response).toMatchObject({ + requestId: "read-1", + source: "error", + content: "", + identityMismatch: true, + error: "Document binding changed while reading", + }); + }, + ); +}); diff --git a/ts/packages/agents/markdown/test/markdownUpdatePersistence.spec.ts b/ts/packages/agents/markdown/test/markdownUpdatePersistence.spec.ts index fb23d6a6e1..f2fee0a5da 100644 --- a/ts/packages/agents/markdown/test/markdownUpdatePersistence.spec.ts +++ b/ts/packages/agents/markdown/test/markdownUpdatePersistence.spec.ts @@ -91,7 +91,7 @@ describe("markdown update persistence", () => { ).toThrow(/binding token changed/); }); - test("rejects a workspace replaced by a junction", () => { + test("rejects a workspace replaced by a junction", async () => { const movedWorkspace = path.join(temporaryDirectory, "moved-workspace"); const outside = path.join(temporaryDirectory, "outside"); fs.mkdirSync(outside); @@ -99,7 +99,7 @@ describe("markdown update persistence", () => { fs.renameSync(workspace, movedWorkspace); fs.symlinkSync(outside, workspace, "junction"); - expect(() => readBoundDocument(binding)).toThrow( + await expect(readBoundDocument(binding)).rejects.toThrow( /workspace root changed/, ); expect(fs.readFileSync(path.join(outside, "plan.md"), "utf-8")).toBe( From a28a689684e3bc195609b66ade410876d3b7ee54 Mon Sep 17 00:00:00 2001 From: George Ng Date: Wed, 9 Sep 2026 17:06:49 -0700 Subject: [PATCH 4/4] test(markdown): validate asynchronous document reads Declare the Jest globals dependency and cover stale load tokens and external disk edits in the real service. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ts/packages/agents/markdown/package.json | 1 + .../markdown/test/markdownService.spec.ts | 42 +++++++++++++++++++ ts/pnpm-lock.yaml | 3 ++ 3 files changed, 46 insertions(+) diff --git a/ts/packages/agents/markdown/package.json b/ts/packages/agents/markdown/package.json index cf19f1fb47..923dae5357 100644 --- a/ts/packages/agents/markdown/package.json +++ b/ts/packages/agents/markdown/package.json @@ -70,6 +70,7 @@ "yjs": "^13.6.8" }, "devDependencies": { + "@jest/globals": "^29.7.0", "@milkdown/ctx": "^7.3.6", "@types/debug": "^4.1.12", "@types/express": "^4.17.17", diff --git a/ts/packages/agents/markdown/test/markdownService.spec.ts b/ts/packages/agents/markdown/test/markdownService.spec.ts index 3ceecae6c4..2eb11b8524 100644 --- a/ts/packages/agents/markdown/test/markdownService.spec.ts +++ b/ts/packages/agents/markdown/test/markdownService.spec.ts @@ -146,6 +146,48 @@ describe("markdown service document reads", () => { }); }); + test("rejects the previous HTTP load token after rebinding the same document", async () => { + const loaded = nextMessage(child, "bindingUpdated"); + expect((await load("plan.md")).status).toBe(200); + const previousBinding = await loaded; + const currentBinding = await bind("plan.md"); + expect(currentBinding.bindingToken).not.toBe( + previousBinding.bindingToken, + ); + + const response = nextMessage(child, "documentContent"); + child.send({ + type: "getDocumentContent", + requestId: "stale-load-token", + expectedBindingToken: previousBinding.bindingToken, + }); + expect(await response).toMatchObject({ + requestId: "stale-load-token", + source: "error", + content: "", + identityMismatch: true, + error: "Document binding token changed", + }); + expect(fs.readFileSync(path.join(root, "plan.md"), "utf-8")).toBe( + "original", + ); + }); + + test("reads external disk edits instead of the initialized collaborative snapshot", async () => { + await bind("plan.md"); + fs.writeFileSync(path.join(root, "plan.md"), "externally updated 😀"); + const paused = nextMessage(child, "readPaused"); + const response = nextMessage(child, "documentContent"); + child.send({ type: "getDocumentContent", requestId: "external-read" }); + await paused; + child.send({ type: "releaseRead" }); + expect(await response).toMatchObject({ + requestId: "external-read", + content: "externally updated 😀", + source: "file", + }); + }); + test.each(["other.md", "plan.md"])( "rejects a read when the service rebinds to %s during suspension", async (nextPath) => { diff --git a/ts/pnpm-lock.yaml b/ts/pnpm-lock.yaml index 91a39e5dd3..6d85f46fc0 100644 --- a/ts/pnpm-lock.yaml +++ b/ts/pnpm-lock.yaml @@ -2914,6 +2914,9 @@ importers: specifier: ^13.6.8 version: 13.6.27 devDependencies: + '@jest/globals': + specifier: ^29.7.0 + version: 29.7.0 '@milkdown/ctx': specifier: ^7.3.6 version: 7.13.1