smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent/session): resolved local:// files for streaming edit cache operations

Browse Source 
- 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.
This commit is contained in:
can1357
2026-04-30 03:48:36 +02:00
parent 90fabf6d4f
commit fd44728d6e
3 changed files with 80 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/modes/interactive-mode.ts
+1 -1
View File
@@ -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,
packages/coding-agent/src/session/agent-session.ts
+53 -26
View File
@@ -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
packages/coding-agent/test/streaming-edit-abort.test.ts
+26 -13
View File
@@ -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 {