From fd44728d6e4af7dfee50df142d210082cfade7ee Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 30 Apr 2026 03:48:36 +0200 Subject: [PATCH] fix(coding-agent/session): resolved local:// files for streaming edit cache operations - Added a shared session path resolver that maps local:// URLs through local-protocol options, skips other internal schemes, and returns an absolute filesystem path for real files. - Updated streaming-edit pre-cache and post-edit cache invalidation to use the shared resolver, preventing internal-scheme assertions while keeping filesystem-based flow for local plan files. - Extended streaming-edit tests to confirm local:// plan edits complete without panicking and that auto-generated checks receive resolved absolute paths. --- .../src/modes/interactive-mode.ts | 2 +- .../coding-agent/src/session/agent-session.ts | 79 +++++++++++++------ .../test/streaming-edit-abort.test.ts | 39 ++++++--- 3 files changed, 80 insertions(+), 40 deletions(-) diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index 1bf208e0e..3b499a004 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -15,8 +15,8 @@ import { } from "@oh-my-pi/pi-ai"; import type { Component, SlashCommand } from "@oh-my-pi/pi-tui"; import { - clearRenderCache, Container, + clearRenderCache, Loader, Markdown, ProcessTerminal, diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 72f462593..ec2a7c50d 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -104,7 +104,7 @@ import { ExtensionToolWrapper } from "../extensibility/extensions/wrapper"; import type { HookCommandContext } from "../extensibility/hooks/types"; import type { Skill, SkillWarning } from "../extensibility/skills"; import { expandSlashCommand, type FileSlashCommand } from "../extensibility/slash-commands"; -import { resolveLocalUrlToPath } from "../internal-urls"; +import { type LocalProtocolOptions, resolveLocalUrlToPath } from "../internal-urls"; import { disposeKernelSessionsByOwner, executePython as executePythonCommand, @@ -136,7 +136,7 @@ import { resolveThinkingLevelForModel, toReasoningEffort } from "../thinking"; import { assertEditableFile } from "../tools/auto-generated-guard"; import type { CheckpointState } from "../tools/checkpoint"; import { outputMeta } from "../tools/output-meta"; -import { isInternalUrlPath, normalizeLocalScheme, resolveToCwd } from "../tools/path-utils"; +import { normalizeLocalScheme, resolveToCwd } from "../tools/path-utils"; import { isAutoQaEnabled } from "../tools/report-tool-issue"; import { getLatestTodoPhasesFromEntries, type TodoItem, type TodoPhase } from "../tools/todo-write"; import { ToolError } from "../tools/tool-errors"; @@ -1522,16 +1522,20 @@ export class AgentSession { const path = typeof args.path === "string" ? args.path : undefined; if (!path) return undefined; - // Internal-scheme URLs (e.g. local://PLAN.md for plan-mode) don't have a - // stable filesystem path; the on-disk pre-cache cannot apply. The Edit - // tool itself dispatches these via the protocol-handler, so the actual - // edit still works — we just skip the streaming pre-cache machinery. - if (isInternalUrlPath(path)) return undefined; + + // `local://` URLs (e.g. local://PLAN.md for plan-mode) resolve to a real + // on-disk artifacts path; pre-caching works as long as we ask the + // local-protocol handler. Other internal-scheme URLs (agent://, skill://, + // rule://, mcp://, artifact://) have no stable filesystem representation; + // skip pre-cache entirely for those — the edit tool itself will reject + // them through its normal dispatch path. + const resolvedPath = this.#resolveSessionFsPath(path); + if (resolvedPath === undefined) return undefined; return { toolCall, path, - resolvedPath: resolveToCwd(path, this.sessionManager.getCwd()), + resolvedPath, diff: typeof args.diff === "string" ? args.diff : undefined, op: typeof args.op === "string" ? args.op : undefined, rename: typeof args.rename === "string" ? args.rename : undefined, @@ -1605,15 +1609,47 @@ export class AgentSession { } /** Invalidate cache for a file after an edit completes to prevent stale data */ - #invalidateFileCacheForPath(path: string): void { - // Internal-scheme URLs are never pre-cached (see #getStreamingEditToolCall), - // so there is nothing to invalidate. Skip before resolveToCwd, which would - // throw via assertNotInternalUrl. - if (isInternalUrlPath(path)) return; - const resolvedPath = resolveToCwd(path, this.sessionManager.getCwd()); + #invalidateFileCacheForPath(filePath: string): void { + const resolvedPath = this.#resolveSessionFsPath(filePath); + if (resolvedPath === undefined) return; this.#streamingEditFileCache.delete(resolvedPath); } + /** + * Resolve a path supplied to a tool to a real filesystem path. + * + * - `local://` URLs route through the local-protocol handler so they map + * onto the session's on-disk artifacts directory; pre-caching, ENOENT + * handling, and post-edit invalidation all work normally. + * - Other internal-scheme URLs (agent://, skill://, rule://, mcp://, + * artifact://) have no stable filesystem path; this returns `undefined` + * so callers skip filesystem-only operations. + * - Cwd-relative and absolute paths resolve via `resolveToCwd`. + */ + #resolveSessionFsPath(filePath: string): string | undefined { + const normalized = normalizeLocalScheme(filePath); + if (normalized.startsWith("local:")) { + return resolveLocalUrlToPath(normalized, this.#localProtocolOptions()); + } + if ( + normalized.startsWith("agent://") || + normalized.startsWith("skill://") || + normalized.startsWith("rule://") || + normalized.startsWith("mcp://") || + normalized.startsWith("artifact://") + ) { + return undefined; + } + return resolveToCwd(normalized, this.sessionManager.getCwd()); + } + + #localProtocolOptions(): LocalProtocolOptions { + return { + getArtifactsDir: () => this.sessionManager.getArtifactsDir(), + getSessionId: () => this.sessionManager.getSessionId(), + }; + } + #maybeAbortStreamingEdit(event: AgentEvent): void { if (!this.settings.get("edit.streamingAbort")) return; if (this.#streamingEditAbortTriggered) return; @@ -2475,10 +2511,7 @@ export class AgentSession { if (this.#planReferenceSent) return null; const planFilePath = this.#planReferencePath; - const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, { - getArtifactsDir: () => this.sessionManager.getArtifactsDir(), - getSessionId: () => this.sessionManager.getSessionId(), - }); + const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, this.#localProtocolOptions()); let planContent: string; try { planContent = await Bun.file(resolvedPlanPath).text(); @@ -2511,15 +2544,9 @@ export class AgentSession { if (!state?.enabled) return null; const sessionPlanUrl = "local://PLAN.md"; const resolvedPlanPath = state.planFilePath.startsWith("local:") - ? resolveLocalUrlToPath(normalizeLocalScheme(state.planFilePath), { - getArtifactsDir: () => this.sessionManager.getArtifactsDir(), - getSessionId: () => this.sessionManager.getSessionId(), - }) + ? resolveLocalUrlToPath(normalizeLocalScheme(state.planFilePath), this.#localProtocolOptions()) : resolveToCwd(state.planFilePath, this.sessionManager.getCwd()); - const resolvedSessionPlan = resolveLocalUrlToPath(sessionPlanUrl, { - getArtifactsDir: () => this.sessionManager.getArtifactsDir(), - getSessionId: () => this.sessionManager.getSessionId(), - }); + const resolvedSessionPlan = resolveLocalUrlToPath(sessionPlanUrl, this.#localProtocolOptions()); const displayPlanPath = state.planFilePath.startsWith("local:") || resolvedPlanPath !== resolvedSessionPlan ? state.planFilePath diff --git a/packages/coding-agent/test/streaming-edit-abort.test.ts b/packages/coding-agent/test/streaming-edit-abort.test.ts index 62ab1a22d..e9196d170 100644 --- a/packages/coding-agent/test/streaming-edit-abort.test.ts +++ b/packages/coding-agent/test/streaming-edit-abort.test.ts @@ -335,16 +335,22 @@ it("aborts when auto-generated check rejects with ToolError", async () => { } }); -it("does not panic when streaming edit targets a local:// (internal-scheme) path", async () => { +it("resolves local:// internal-scheme paths through the protocol handler instead of panicking", async () => { // Plan-mode persists the plan file under the synthetic local:// URL scheme. - // The streaming pre-cache (#preCacheStreamingEditFile → #getStreamingEditToolCall) - // previously called resolveToCwd() unconditionally on the path, which throws - // for internal-scheme URLs via assertNotInternalUrl(). The throw escaped the - // synchronous interceptor as an Unhandled Rejection, killing the session. - // The guard added in #getStreamingEditToolCall and #invalidateFileCacheForPath - // must skip pre-cache for these paths instead. The actual edit still runs - // (its tool dispatch handles internal URLs separately). - await Bun.write(path.join(tempDir, "fallback.txt"), "alpha\n"); // not used; just keeps tempDir consistent + // Earlier the streaming pre-cache (#preCacheStreamingEditFile → + // #getStreamingEditToolCall) called resolveToCwd() unconditionally on the + // path, which throws for internal-scheme URLs via assertNotInternalUrl(). + // The throw escaped the synchronous interceptor as an Unhandled Rejection + // and killed the session. + // + // The fix routes `local://` paths through resolveLocalUrlToPath() so they + // map onto the session's on-disk local-artifacts directory; pre-caching, + // auto-generated detection, and post-edit invalidation all run on the real + // file. Drive a streaming Edit toolcall with path: 'local://PLAN.md' and + // confirm the session completes without panicking and that + // assertEditableFile is invoked with a real (non-internal-scheme) absolute + // path so the auto-generated guard works for plan-mode edits too. + const checkSpy = vi.spyOn(autoGeneratedGuard, "assertEditableFile"); const diff = "@@\n-old\n+new\n"; const abortSignalRef: { current?: AbortSignal } = {}; const chunks = chunkStringRandomly(diff, 7); @@ -354,15 +360,22 @@ it("does not panic when streaming edit targets a local:// (internal-scheme) path try { await session.prompt("edit plan file"); - // Reaching here means neither #preCacheStreamingEditFile nor the post-edit - // #invalidateFileCacheForPath threw. Confirm the session ran the streaming - // edit through to completion (no aborted stop-reason) and the agent was - // not aborted by the synchronous panic. const lastAssistant = lastAssistantMessage(session.state.messages); expect(lastAssistant).toBeDefined(); expect(lastAssistant?.stopReason).not.toBe("aborted"); expect(abortSignalRef.current?.aborted ?? false).toBe(false); + + // Confirm pre-cache resolved the URL rather than skipping it: the + // auto-generated guard should have been invoked with a concrete fs path + // (absolute, no internal scheme) plus the original local:// display path. + expect(checkSpy).toHaveBeenCalled(); + const [absolutePath, displayPath] = checkSpy.mock.calls[0] ?? []; + expect(typeof absolutePath).toBe("string"); + expect(absolutePath).toMatch(/^(?:\/|[A-Za-z]:[\\/])/); + expect(absolutePath).not.toMatch(/^[a-z]+:\/\//); + expect(displayPath).toBe("local://PLAN.md"); } finally { + checkSpy.mockRestore(); try { await session.dispose(); } finally {