feat(edit/read): enabled canonical snapshot keys across read/write paths

- Canonicalized snapshot key resolution with realpath, parent fallback, and raw fallback.
- Updated snapshot, hashline, read, and write flows to use canonical snapshot keys consistently.
- Fixed converted-content line-range reads to apply selectors via in-memory range/text builders.
- Updated read tool docs to clarify one-line and multi-range selector context bounds.
This commit is contained in:
can1357
2026-06-08 02:04:59 +02:00
parent 088fb7fb75
commit 68c454c0fc
10 changed files with 262 additions and 35 deletions
@@ -8,6 +8,8 @@
* from `@oh-my-pi/hashline`; the only coding-agent-specific concern here
* is wiring it onto the per-session owner object.
*/
import * as fs from "node:fs";
import * as path from "node:path";
import { InMemorySnapshotStore } from "@oh-my-pi/hashline";
import { normalizeToLF } from "./normalize";
@@ -33,6 +35,36 @@ export function getFileSnapshotStore(session: FileSnapshotStoreOwner): InMemoryS
return session.fileSnapshotStore;
}
/**
* Canonicalize an absolute path into the stable key the snapshot store uses.
*
* Different code paths reach the snapshot store via different path forms:
* `read local://foo.md` records under the file's `fs.realpath` (the local
* protocol handler resolves symlinks); a subsequent `edit` may address the
* same artifact via `local://foo.md`, whose resolver does NOT realpath, or
* via the absolute path returned in the `[path#tag]` header. macOS adds the
* same hazard at the working-tree level (`/tmp/...` vs `/private/tmp/...`).
* Collapsing every key through `realpath` makes those forms fuse onto one
* snapshot entry, so a freshly-minted tag is never rejected as stale just
* because the lookup spelled the same file differently.
*
* Non-existent paths (new-file writes) fall back to a realpath of the parent
* directory + basename, then to the input. This keeps creates and updates on
* the same canonical key.
*/
export function canonicalSnapshotKey(absolutePath: string): string {
try {
return fs.realpathSync.native(absolutePath);
} catch {
try {
const parent = fs.realpathSync.native(path.dirname(absolutePath));
return path.join(parent, path.basename(absolutePath));
} catch {
return absolutePath;
}
}
}
/**
* Read the full text of `absolutePath` (within {@link SNAPSHOT_MAX_BYTES}),
* record it as a version snapshot, and return its content-hash tag. Returns
@@ -52,7 +84,7 @@ export async function recordFileSnapshot(
const file = Bun.file(absolutePath);
if (file.size > SNAPSHOT_MAX_BYTES) return undefined;
const normalized = normalizeToLF(await file.text());
return getFileSnapshotStore(session).record(absolutePath, normalized);
return getFileSnapshotStore(session).record(canonicalSnapshotKey(absolutePath), normalized);
} catch {
return undefined;
}
@@ -23,6 +23,7 @@ import type { ToolSession } from "../../tools";
import { assertEditableFileContent } from "../../tools/auto-generated-guard";
import { invalidateFsScanAfterWrite } from "../../tools/fs-cache-invalidation";
import { enforcePlanModeWrite, resolvePlanPath } from "../../tools/plan-mode-guard";
import { canonicalSnapshotKey } from "../file-snapshot-store";
import { readEditFileText, serializeEditFileText } from "../read-file";
import type { LspBatchRequest } from "../renderer";
@@ -81,7 +82,7 @@ export class HashlineFilesystem extends Filesystem {
}
canonicalPath(relativePath: string): string {
return this.resolveAbsolute(relativePath);
return canonicalSnapshotKey(this.resolveAbsolute(relativePath));
}
async readText(relativePath: string): Promise<string> {
@@ -18,8 +18,8 @@ Append `:<sel>` to `path`. The bare path falls back to the default mode.
- `:50` / `:50-` — read from line 50 onward.
- `:50-200` — lines 50–200 inclusive.
- `:50+150` — 150 lines starting at line 50.
- `:20+1` — exactly one line.
- `:5-16,960-973` — multiple ranges in one call (sorted, overlaps merged).
- `:20+1` — anchor on line 20 (single-range reads expand by ≤1 leading and ≤3 trailing context lines).
- `:5-16,960-973` — multiple ranges in one call (sorted, overlaps merged). Multi-range mode returns exact bounds with no context padding.
- `:raw` — verbatim text; no anchors, no summary, no line prefixes.
- `:2-4:raw` or `:raw:2-4` — range AND verbatim; the two compose in either order.
- `:conflicts` — one-line-per-block index of every unresolved git merge conflict.
+25 -12
View File
@@ -9,7 +9,7 @@ import type { Component } from "@oh-my-pi/pi-tui";
import { Text } from "@oh-my-pi/pi-tui";
import { getRemoteDir, logger, prompt, readImageMetadata, untilAborted } from "@oh-my-pi/pi-utils";
import * as z from "zod/v4";
import { getFileSnapshotStore, recordFileSnapshot } from "../edit/file-snapshot-store";
import { canonicalSnapshotKey, getFileSnapshotStore, recordFileSnapshot } from "../edit/file-snapshot-store";
import { normalizeToLF } from "../edit/normalize";
import { isNotebookPath, readEditableNotebookText } from "../edit/notebook";
import type { RenderResultOptions } from "../extensibility/custom-tools/types";
@@ -131,7 +131,7 @@ function recordFullHashlineContext(
): HashlineHeaderContext | undefined {
if (!absolutePath || !path.isAbsolute(absolutePath)) return undefined;
const normalized = normalizeToLF(fullText);
const tag = getFileSnapshotStore(session).record(absolutePath, normalized);
const tag = getFileSnapshotStore(session).record(canonicalSnapshotKey(absolutePath), normalized);
return {
header: formatHashlineHeader(displayPath, tag),
tag,
@@ -1750,15 +1750,25 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
// Convert document via markit.
const result = await convertFileWithMarkit(absolutePath, signal);
if (result.ok) {
// Apply truncation to converted content
const truncation = truncateHead(result.content);
const outputText = truncation.content;
details = { truncation };
sourcePath = absolutePath;
truncationInfo = { result: truncation, options: { direction: "head", startLine: 1 } };
content = [{ type: "text", text: outputText }];
// Route the converted markdown through the in-memory text builder
// so line-range selectors (`file.pdf:50-100`, `:5-16,40-80`) and
// raw mode apply against the converted output. Without this,
// `file.pdf:50-100` silently returned the head of the document
// because only `truncateHead` was being applied.
if (isMultiRange(parsed) && parsed.kind === "lines") {
return this.#buildInMemoryMultiRangeResult(result.content, parsed.ranges, {
details: { resolvedPath: absolutePath },
sourcePath: absolutePath,
entityLabel: "document",
});
}
const { offset, limit } = selToOffsetLimit(parsed);
return this.#buildInMemoryTextResult(result.content, offset, limit, {
details: { resolvedPath: absolutePath },
sourcePath: absolutePath,
entityLabel: "document",
raw: isRawSelector(parsed),
});
} else if (result.error) {
content = [{ type: "text", text: `[Cannot read ${ext} file: ${result.error || "conversion failed"}]` }];
} else {
@@ -1944,7 +1954,10 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
// full file and any anchor validates while the file is unchanged.
const isWholeFile = offset === undefined && limit === undefined && !wasTruncated;
const tag = isWholeFile
? getFileSnapshotStore(this.session).record(absolutePath, normalizeToLF(collectedLines.join("\n")))
? getFileSnapshotStore(this.session).record(
canonicalSnapshotKey(absolutePath),
normalizeToLF(collectedLines.join("\n")),
)
: await recordFileSnapshot(this.session, absolutePath);
if (tag) {
hashContext = hashlineHeaderContext(formatPathRelativeToCwd(absolutePath, this.session.cwd), tag);
+2 -2
View File
@@ -8,7 +8,7 @@ import type { Component } from "@oh-my-pi/pi-tui";
import { isEnoent, isRecord, prompt, untilAborted } from "@oh-my-pi/pi-utils";
import * as z from "zod/v4";
import { getFileSnapshotStore } from "../edit/file-snapshot-store";
import { canonicalSnapshotKey, getFileSnapshotStore } from "../edit/file-snapshot-store";
import { normalizeToLF } from "../edit/normalize";
import type { RenderResultOptions } from "../extensibility/custom-tools/types";
import { InternalUrlRouter } from "../internal-urls";
@@ -132,7 +132,7 @@ function stripWriteContent(session: ToolSession, content: string): { text: strin
function maybeWriteSnapshotHeader(session: ToolSession, absolutePath: string, content: string): string | undefined {
if (!resolveFileDisplayMode(session).hashLines) return undefined;
const normalized = normalizeToLF(content);
const tag = getFileSnapshotStore(session).record(absolutePath, normalized);
const tag = getFileSnapshotStore(session).record(canonicalSnapshotKey(absolutePath), normalized);
return formatHashlineHeader(formatPathRelativeToCwd(absolutePath, session.cwd), tag);
}
@@ -22,6 +22,7 @@ import {
} from "@oh-my-pi/hashline";
import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import {
canonicalSnapshotKey,
type ExecuteHashlineSingleOptions,
executeHashlineSingle,
generateDiffString,
@@ -94,9 +95,19 @@ const outputSepRe = ":";
function tag(line: number, _content: string): string {
return `${line}`;
}
function recordFullSnapshot(cache: FileReadCache, filePath: string, fullText: string): string {
return cache.record(filePath, fullText);
// Mirror the production read/write recorders: collapse symlink-equivalent
// path spellings (e.g. macOS `/tmp/...` vs `/private/tmp/...`) so the patcher
// looks up snapshots under the same canonical key it just recorded.
return cache.record(canonicalSnapshotKey(filePath), fullText);
}
/** Snapshot-cache lookup that mirrors {@link recordFullSnapshot}'s canonical key. */
function snapshotHead(cache: FileReadCache, filePath: string) {
return cache.head(canonicalSnapshotKey(filePath));
}
function snapshotByHash(cache: FileReadCache, filePath: string, hash: string) {
return cache.byHash(canonicalSnapshotKey(filePath), hash);
}
function header(filePath: string, tag: string): string {
@@ -959,7 +970,7 @@ describe("hashline — anchor-stale recovery via read snapshot cache", () => {
const v1Text = `${v1Lines.join("\n")}\n`;
expect(await Bun.file(filePath).text()).toBe(v1Text);
const v1Tag = recordFullSnapshot(getFileReadCache(session), filePath, v1Text);
const snap = getFileReadCache(session).head(filePath);
const snap = snapshotHead(getFileReadCache(session), filePath);
expect(snap?.text).toBe(v1Text);
// External actor insert heads 7 lines after the edit. Anchors authored
@@ -1024,7 +1035,7 @@ describe("hashline — anchor-stale recovery via read snapshot cache", () => {
const recovered = tryRecoverHashlineWithCache({
cache,
absolutePath: fakePath,
absolutePath: canonicalSnapshotKey(fakePath),
currentText,
tag: v0Tag,
edits: parseHashline(`replace 10..10:\n${repl("L10-EDITED")}`).edits,
@@ -1040,9 +1051,9 @@ describe("hashline — anchor-stale recovery via read snapshot cache", () => {
const oneTag = recordFullSnapshot(cache, fakePath, "one\n");
const twoTag = recordFullSnapshot(cache, fakePath, "two\n");
recordFullSnapshot(cache, fakePath, "three\n");
expect(cache.head(fakePath)?.text).toBe("three\n");
expect(cache.byHash(fakePath, oneTag)?.text).toBe("one\n");
expect(cache.byHash(fakePath, twoTag)?.text).toBe("two\n");
expect(snapshotHead(cache, fakePath)?.text).toBe("three\n");
expect(snapshotByHash(cache, fakePath, oneTag)?.text).toBe("one\n");
expect(snapshotByHash(cache, fakePath, twoTag)?.text).toBe("two\n");
});
it("evicts the least-recently-used path beyond the LRU cap", () => {
const cache = new FileReadCache({ maxPaths: 4 });
@@ -1050,10 +1061,10 @@ describe("hashline — anchor-stale recovery via read snapshot cache", () => {
recordFullSnapshot(cache, `/tmp/file-${i}.ts`, `x${i}\n`);
}
// The two oldest paths aged out; the four most-recent survive.
expect(cache.head("/tmp/file-0.ts")).toBeNull();
expect(cache.head("/tmp/file-1.ts")).toBeNull();
expect(cache.head("/tmp/file-2.ts")?.text).toBe("x2\n");
expect(cache.head("/tmp/file-5.ts")?.text).toBe("x5\n");
expect(snapshotHead(cache, "/tmp/file-0.ts")).toBeNull();
expect(snapshotHead(cache, "/tmp/file-1.ts")).toBeNull();
expect(snapshotHead(cache, "/tmp/file-2.ts")?.text).toBe("x2\n");
expect(snapshotHead(cache, "/tmp/file-5.ts")?.text).toBe("x5\n");
});
});
@@ -0,0 +1,67 @@
import { describe, expect, it } from "bun:test";
import * as fs from "node:fs/promises";
import * as os from "node:os";
import * as path from "node:path";
import type { InMemorySnapshotStore } from "@oh-my-pi/hashline";
import { canonicalSnapshotKey, getFileSnapshotStore } from "../../src/edit/file-snapshot-store";
interface SessionOwner {
fileSnapshotStore?: InMemorySnapshotStore;
}
describe("canonicalSnapshotKey", () => {
it("collapses symlink-equivalent forms (macOS /tmp ↔ /private/tmp) onto one key", async () => {
// `os.tmpdir()` returns the realpath on macOS; mkdtemp under it gives us a
// real directory that we can address via both /tmp/... and /private/tmp/...
// when the platform has that symlink. Skip the assertion when it doesn't.
const realDir = await fs.mkdtemp(path.join(os.tmpdir(), "snap-key-"));
const filePath = path.join(realDir, "a.txt");
await Bun.write(filePath, "x\n");
const k1 = canonicalSnapshotKey(filePath);
// If realDir already starts at the symlink target form, k1 === filePath
// — that's also valid behavior. Either way both spellings MUST round-trip
// to the same canonical key.
expect(canonicalSnapshotKey(k1)).toBe(k1);
// Construct the alternate spelling for tmpdir if /tmp -> /private/tmp.
if (filePath.startsWith("/private/")) {
const alt = filePath.slice("/private".length);
expect(canonicalSnapshotKey(alt)).toBe(k1);
}
});
it("falls back to parent realpath + basename for non-existent paths", async () => {
const realDir = await fs.mkdtemp(path.join(os.tmpdir(), "snap-key-"));
const missing = path.join(realDir, "does-not-exist.txt");
// Snapshot key is still computable (used for write-then-snapshot flow).
const key = canonicalSnapshotKey(missing);
expect(key).toBe(path.join(canonicalSnapshotKey(realDir), "does-not-exist.txt"));
});
it("returns the input unchanged when nothing in the chain exists", () => {
const key = canonicalSnapshotKey("/__definitely-not-a-real-path__/x/y/z.txt");
expect(key).toBe("/__definitely-not-a-real-path__/x/y/z.txt");
});
});
describe("snapshot store fusion via canonical keys", () => {
it("records and looks up the same snapshot regardless of /tmp vs /private/tmp spelling", async () => {
const realDir = await fs.mkdtemp(path.join(os.tmpdir(), "snap-fuse-"));
const filePath = path.join(realDir, "a.txt");
await Bun.write(filePath, "x\n");
const session: SessionOwner = {};
const store = getFileSnapshotStore(session);
const hash = store.record(canonicalSnapshotKey(filePath), "x\n");
// The hash MUST be retrievable via every path spelling that points at
// the same file content (covers the patcher looking up a tag the read
// tool minted under a different spelling).
expect(store.byHash(canonicalSnapshotKey(filePath), hash)?.text).toBe("x\n");
if (filePath.startsWith("/private/")) {
const alt = filePath.slice("/private".length);
expect(store.byHash(canonicalSnapshotKey(alt), hash)?.text).toBe("x\n");
}
});
});
@@ -16,7 +16,7 @@ import * as path from "node:path";
import { Patch, Patcher } from "@oh-my-pi/hashline";
import type { AgentToolResult } from "@oh-my-pi/pi-agent-core";
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { getFileSnapshotStore } from "@oh-my-pi/pi-coding-agent/edit/file-snapshot-store";
import { canonicalSnapshotKey, getFileSnapshotStore } from "@oh-my-pi/pi-coding-agent/edit/file-snapshot-store";
import { HashlineFilesystem } from "@oh-my-pi/pi-coding-agent/edit/hashline/filesystem";
import { writethroughNoop } from "@oh-my-pi/pi-coding-agent/lsp";
import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
@@ -111,7 +111,7 @@ describe("read tool column truncation vs hashline snapshot", () => {
expect(text).not.toContain(longLine);
const { tag } = extractHeader(text);
const snapshot = getFileSnapshotStore(session).byHash(filePath, tag);
const snapshot = getFileSnapshotStore(session).byHash(canonicalSnapshotKey(filePath), tag);
expect(snapshot).not.toBeNull();
// The snapshot MUST hold the on-disk text, not the display-truncated version.
@@ -132,7 +132,7 @@ describe("read tool column truncation vs hashline snapshot", () => {
expect(text).toContain("…");
const { tag } = extractHeader(text);
const snapshot = getFileSnapshotStore(session).byHash(filePath, tag);
const snapshot = getFileSnapshotStore(session).byHash(canonicalSnapshotKey(filePath), tag);
expect(snapshot?.text.split("\n")[1]).toBe(longLine);
});
@@ -149,7 +149,7 @@ describe("read tool column truncation vs hashline snapshot", () => {
expect(text).toContain("…");
const { tag } = extractHeader(text);
const snapshot = getFileSnapshotStore(session).byHash(filePath, tag);
const snapshot = getFileSnapshotStore(session).byHash(canonicalSnapshotKey(filePath), tag);
expect(snapshot?.text.split("\n")[1]).toBe(longLine);
expect(snapshot?.text.split("\n")[5]).toBe(longLine);
});
@@ -0,0 +1,103 @@
/**
* Regression test for cluster 51: line-range selectors on PDF/document
* reads silently returned the head of the converted document. The fix
* routes the converted markdown through the same in-memory builders that
* notebook reads use.
*/
import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test";
import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read";
import * as markit from "@oh-my-pi/pi-coding-agent/utils/markit";
import { Snowflake } from "@oh-my-pi/pi-utils";
function makeSession(testDir: string): ToolSession {
const sessionFile = path.join(testDir, "session.jsonl");
const artifactsDir = sessionFile.slice(0, -6);
let nextArtifactId = 0;
return {
cwd: testDir,
hasUI: false,
getSessionFile: () => sessionFile,
getArtifactsDir: () => artifactsDir,
getSessionSpawns: () => null,
allocateOutputArtifact: async toolType => {
const id = String(nextArtifactId++);
return { id, path: path.join(artifactsDir, `${id}.${toolType}.log`) };
},
settings: Settings.isolated(),
};
}
describe("read PDF with a line-range selector", () => {
let testDir: string;
let pdfPath: string;
beforeEach(() => {
testDir = path.join(os.tmpdir(), `read-pdf-${Snowflake.next()}`);
fs.mkdirSync(testDir, { recursive: true });
pdfPath = path.join(testDir, "doc.pdf");
fs.writeFileSync(pdfPath, "%PDF-stub");
});
afterEach(() => {
vi.restoreAllMocks();
fs.rmSync(testDir, { recursive: true, force: true });
});
it("honours `:N-M` against the converted markdown body", async () => {
const converted = Array.from({ length: 200 }, (_, i) => `pdf line ${i + 1}`).join("\n");
vi.spyOn(markit, "convertFileWithMarkit").mockResolvedValue({ ok: true, content: converted });
const session = makeSession(testDir);
const tool = new ReadTool(session);
const result = await tool.execute("call", { path: `${pdfPath}:120-122` });
const text = result.content
.filter(c => c.type === "text")
.map(c => c.text)
.join("\n");
// The requested window must surface. Pre-fix the read silently returned
// the head of the document (lines 1-200 head-truncated) instead.
expect(text).toContain("pdf line 120");
expect(text).toContain("pdf line 122");
expect(text).not.toContain("pdf line 1\n");
expect(text).not.toContain("pdf line 5");
});
it("honours `:A-B,C-D` multi-range against the converted markdown body", async () => {
const converted = Array.from({ length: 200 }, (_, i) => `pdf line ${i + 1}`).join("\n");
vi.spyOn(markit, "convertFileWithMarkit").mockResolvedValue({ ok: true, content: converted });
const session = makeSession(testDir);
const tool = new ReadTool(session);
const result = await tool.execute("call", { path: `${pdfPath}:50-52,160-162` });
const text = result.content
.filter(c => c.type === "text")
.map(c => c.text)
.join("\n");
expect(text).toContain("pdf line 50");
expect(text).toContain("pdf line 52");
expect(text).toContain("pdf line 160");
expect(text).toContain("pdf line 162");
expect(text).not.toContain("pdf line 100");
});
it("falls back to the full converted body when no selector is provided", async () => {
const converted = "pdf line 1\npdf line 2\npdf line 3\n";
vi.spyOn(markit, "convertFileWithMarkit").mockResolvedValue({ ok: true, content: converted });
const session = makeSession(testDir);
const tool = new ReadTool(session);
const result = await tool.execute("call", { path: pdfPath });
const text = result.content
.filter(c => c.type === "text")
.map(c => c.text)
.join("\n");
expect(text).toContain("pdf line 1");
expect(text).toContain("pdf line 3");
});
});
@@ -4,7 +4,7 @@ import * as os from "node:os";
import * as path from "node:path";
import { Patch, Patcher } from "@oh-my-pi/hashline";
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { getFileSnapshotStore } from "@oh-my-pi/pi-coding-agent/edit/file-snapshot-store";
import { canonicalSnapshotKey, getFileSnapshotStore } from "@oh-my-pi/pi-coding-agent/edit/file-snapshot-store";
import { HashlineFilesystem } from "@oh-my-pi/pi-coding-agent/edit/hashline/filesystem";
import { writethroughNoop } from "@oh-my-pi/pi-coding-agent/lsp";
import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
@@ -65,7 +65,7 @@ describe("write tool hashline header", () => {
// The tag must address a snapshot whose content matches what we wrote so a
// follow-up edit can land without an extra `read` round-trip.
const snapshot = getFileSnapshotStore(session).byHash(filePath, tag!);
const snapshot = getFileSnapshotStore(session).byHash(canonicalSnapshotKey(filePath), tag!);
expect(snapshot).not.toBeNull();
expect(snapshot?.text).toBe(content);
});