diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 349d9c51d3..8977b60830 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -103,6 +103,7 @@ import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector" import { restoreTodoListForTask } from "../tools/UpdateTodoListTool" import { FileContextTracker } from "../context-tracking/FileContextTracker" import { RooIgnoreController } from "../ignore/RooIgnoreController" +import { ObservationRegistry } from "./observationRegistry" import { RooProtectedController } from "../protect/RooProtectedController" import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message" import { NativeToolCallParser } from "../assistant-message/NativeToolCallParser" @@ -181,6 +182,7 @@ export class Task extends EventEmitter implements TaskLike { readonly parentTask: Task | undefined = undefined readonly taskNumber: number readonly workspacePath: string + readonly observationRegistry = new ObservationRegistry() /** * The mode associated with this task. Persisted across sessions diff --git a/src/core/task/__tests__/observationRegistry.spec.ts b/src/core/task/__tests__/observationRegistry.spec.ts new file mode 100644 index 0000000000..51b73aabde --- /dev/null +++ b/src/core/task/__tests__/observationRegistry.spec.ts @@ -0,0 +1,72 @@ +import { describe, it, expect, vi } from "vitest" + +import { ObservationRegistry } from "../observationRegistry" + +describe("ObservationRegistry", () => { + it("observe → get returns the recorded version and observedAt", () => { + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "1:2:300:4000000000:5000000000") + + const obs = reg.get("/a/b/c.ts") + expect(obs).toBeDefined() + expect(obs!.version).toBe("1:2:300:4000000000:5000000000") + expect(typeof obs!.observedAt).toBe("number") + }) + + it("re-observe replaces the entry with a fresh observedAt", () => { + vi.useFakeTimers() + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "v1") + const first = reg.get("/a/b/c.ts")! + expect(first.version).toBe("v1") + + vi.advanceTimersByTime(50) + reg.observe("/a/b/c.ts", "v2") + const second = reg.get("/a/b/c.ts")! + expect(second.version).toBe("v2") + expect(second.observedAt).toBeGreaterThan(first.observedAt) + + vi.useRealTimers() + }) + + it("has returns true for observed paths, false otherwise", () => { + const reg = new ObservationRegistry() + reg.observe("/x.ts", "t1") + expect(reg.has("/x.ts")).toBe(true) + expect(reg.has("/y.ts")).toBe(false) + }) + + it("size reflects the number of observed entries", () => { + const reg = new ObservationRegistry() + expect(reg.size).toBe(0) + reg.observe("/a.ts", "t1") + reg.observe("/b.ts", "t2") + expect(reg.size).toBe(2) + }) + + it("clear removes all entries and resets size to 0", () => { + const reg = new ObservationRegistry() + reg.observe("/a.ts", "t1") + reg.observe("/b.ts", "t2") + reg.clear() + expect(reg.size).toBe(0) + expect(reg.get("/a.ts")).toBeUndefined() + expect(reg.has("/b.ts")).toBe(false) + }) + + it("get on empty registry returns undefined", () => { + const reg = new ObservationRegistry() + expect(reg.get("/any.ts")).toBeUndefined() + }) + + it("separate instances are independent — observing in one does not appear in the other", () => { + const regA = new ObservationRegistry() + const regB = new ObservationRegistry() + regA.observe("/shared.ts", "v1") + expect(regA.get("/shared.ts")).toBeDefined() + expect(regB.get("/shared.ts")).toBeUndefined() + regB.observe("/shared.ts", "v2") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")!.version).toBe("v2") + }) +}) diff --git a/src/core/task/observationRegistry.ts b/src/core/task/observationRegistry.ts new file mode 100644 index 0000000000..871f80225b --- /dev/null +++ b/src/core/task/observationRegistry.ts @@ -0,0 +1,47 @@ +/** + * Per-task file observation registry (upstream epic #1375, phase A2). + * + * Each Task owns its own instance so parent and subtask observations are + * independent. The S4 guarded-write will compare these versions against the + * token recomputed pre-write to detect stale reads or file replacement. + * + * Pure in-memory — zero I/O, no dependencies. No behavior change in this PR: + * observations are recorded but not consulted. + */ + +export interface FileObservation { + /** Version token derived from on-disk fs.stat (bigint mode). */ + version: string + /** Millisecond timestamp when the observation was recorded. */ + observedAt: number +} + +export class ObservationRegistry { + private readonly entries = new Map() + + /** + * Record an observation for a file at its absolute path. + * + * Re-observing replaces the entry with a fresh observedAt timestamp and + * the new version token. + */ + observe(absolutePath: string, version: string): void { + this.entries.set(absolutePath, { version, observedAt: Date.now() }) + } + + get(absolutePath: string): FileObservation | undefined { + return this.entries.get(absolutePath) + } + + has(absolutePath: string): boolean { + return this.entries.has(absolutePath) + } + + clear(): void { + this.entries.clear() + } + + get size(): number { + return this.entries.size + } +} diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index 2107cfe21b..6e222e1309 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -16,6 +16,7 @@ import type { ReadFileParams, ReadFileMode, ReadFileToolParams, FileEntry, LineR import { isLegacyReadFileParams, type ClineSayTool } from "@roo-code/types" import { Task } from "../task/Task" +import { computeVersionToken } from "../../utils/versionToken" import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { isPathOutsideWorkspace } from "../../utils/pathUtils" @@ -220,6 +221,11 @@ export class ReadFileTool extends BaseTool<"read_file"> { await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) + // A2 (plan #33 / epic #1375): record the observed on-disk version for the future write guard. + // A stat failure leaves the target unobserved and never fails the read. + const version = await computeVersionToken(fullPath).catch(() => undefined) + if (version) task.observationRegistry.observe(fullPath, version) + updateFileResult(relPath, { nativeContent: `File: ${relPath}\n${result}`, }) @@ -799,6 +805,12 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Track file in context await task.fileContextTracker.trackFileContext(relPath, "read_tool") + + // A2 (plan #33 / epic #1375): mirror the native path — record the observed + // on-disk version so legacy-format reads also feed the future write guard. + // A stat failure leaves the target unobserved and never fails the read. + const version = await computeVersionToken(fullPath).catch(() => undefined) + if (version) task.observationRegistry.observe(fullPath, version) } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error) results.push(`File: ${relPath}\nError: ${errorMsg}`) diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 6c9e177d38..6108e78151 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -13,10 +13,16 @@ */ import path from "path" +import type { Stats } from "fs" + +import type { LegacyReadFileParams } from "@roo-code/types" import { isBinaryFile } from "isbinaryfile" import { readFileTool, ReadFileTool } from "../ReadFileTool" +import type { Task } from "../../task/Task" +import { ObservationRegistry } from "../../task/observationRegistry" +import { computeVersionToken } from "../../../utils/versionToken" import { formatResponse } from "../../prompts/responses" import { validateImageForProcessing, @@ -136,6 +142,7 @@ interface MockTaskOptions { rooIgnoreAllowed?: boolean maxImageFileSize?: number maxTotalImageSize?: number + observationRegistry?: ObservationRegistry } function createMockTask(options: MockTaskOptions = {}) { @@ -143,6 +150,9 @@ function createMockTask(options: MockTaskOptions = {}) { return { cwd: "/test/workspace", + // Mirror Task: every task always owns an observation registry (A2, #1375). + // Tests asserting on observations pass their own instance via options. + observationRegistry: options.observationRegistry ?? new ObservationRegistry(), api: { getModel: vi.fn().mockReturnValue({ info: { supportsImages }, @@ -1489,5 +1499,108 @@ describe("ReadFileTool", () => { expect(mockTask.didToolFailInCurrentTurn).toBe(true) }) + + describe("observation registry", () => { + it("records an observation on successful read of an existing file", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // Override the beforeEach default stat mock with proper BigIntStats. + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + // Spy on observe to capture the exact key used (Windows path.resolve may use backslashes). + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "existing.ts" }, mockTask as unknown as Task, callbacks) + + // Verify the tool called observe exactly once with a valid token. + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion] = observeSpy.mock.calls[0] + expect(calledPath).toContain("existing.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + + // Verify get() returns the same data using the spy-captured key. + const obs = reg.get(calledPath) + expect(obs).toBeDefined() + expect(obs!.version).toBe(calledVersion) + }) + + it("a failed read (absent path) leaves the registry size 0 and does not throw", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsReadFile.mockRejectedValue(new Error("ENOENT")) + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "missing.ts" }, mockTask as unknown as Task, callbacks) + + // observationRegistry is guaranteed present because we passed it in createMockTask. + const reg = mockTask.observationRegistry + expect(reg).toBeDefined() + expect(reg!.size).toBe(0) + }) + + it("records an observation for legacy-format reads of existing files", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Typed legacy (pre-refactor) params: the multi-file format with the + // _legacyFormat discriminant (see LegacyReadFileParams). + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion] = observeSpy.mock.calls[0] + expect(calledPath).toContain("legacy.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + }) + + it("two separate Task-owned registries are independent", async () => { + const regA = new ObservationRegistry() + const regB = new ObservationRegistry() + regA.observe("/shared.ts", "v1") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")).toBeUndefined() + regB.observe("/shared.ts", "v2") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")!.version).toBe("v2") + }) + }) }) }) diff --git a/src/utils/__tests__/versionToken.spec.ts b/src/utils/__tests__/versionToken.spec.ts new file mode 100644 index 0000000000..3e2ca26b5c --- /dev/null +++ b/src/utils/__tests__/versionToken.spec.ts @@ -0,0 +1,105 @@ +import * as fs from "fs/promises" +import * as os from "os" +import * as path from "path" +import type { BigIntStats } from "fs" +import { afterEach, beforeEach, describe, expect, it } from "vitest" + +import { computeVersionToken, versionTokenOfStat } from "../versionToken" + +// BigIntStats is a class-backed interface without a public constructor, so a +// plain-object test double is the only practical way to pin the token format +// without real files. Last-resort double assertion (test-local, per AGENTS.md). +function makeStats(overrides: Partial = {}): BigIntStats { + // This repo's @types/node models every StatsBase field (including the *Ms + // fields) as the parameter type T, so all values here are bigint literals; + // the token only reads the *Ns fields. Single-step downcast from Partial to + // the full type (BigIntStats has no public constructor). + const base: Partial = { + dev: 7n, + ino: 4242n, + size: 1234n, + atimeMs: 1_700_000_000_000n, + mtimeMs: 1_700_000_000_123n, + ctimeMs: 1_700_000_000_789n, + birthtimeMs: 1_700_000_000_000n, + atimeNs: 1_700_000_000_000_000_000n, + mtimeNs: 1_700_000_000_123_456_789n, + ctimeNs: 1_700_000_000_789_999_999n, + birthtimeNs: 1_700_000_000_000_000_000n, + } + return { ...base, ...overrides } as BigIntStats +} + +describe("versionTokenOfStat (A1, epic #1375)", () => { + it("is deterministic for an identical stat", () => { + expect(versionTokenOfStat(makeStats())).toBe(versionTokenOfStat(makeStats())) + }) + + it("matches the documented dev:ino:size:mtimeNs:ctimeNs format with exact decimal fields", () => { + expect(versionTokenOfStat(makeStats())).toBe("7:4242:1234:1700000000123456789:1700000000789999999") + }) + + it("distinguishes size changes at identical timestamps", () => { + expect(versionTokenOfStat(makeStats({ size: 1235n }))).not.toBe(versionTokenOfStat(makeStats())) + }) + + it("distinguishes a one-nanosecond mtime change", () => { + expect(versionTokenOfStat(makeStats({ mtimeNs: 1_700_000_000_123_456_790n }))).not.toBe( + versionTokenOfStat(makeStats()), + ) + }) + + it("distinguishes a replaced file (dev/ino change) with identical content state", () => { + const replaced = makeStats({ dev: 8n, ino: 999n }) + expect(versionTokenOfStat(replaced)).not.toBe(versionTokenOfStat(makeStats())) + }) + + it("renders nanosecond resolution exactly (no float quantization)", () => { + const base = versionTokenOfStat(makeStats()) + const plusOneMicrosecond = versionTokenOfStat(makeStats({ mtimeNs: 1_700_000_000_123_457_789n })) + // 1_000 ns apart — the BigInt derivation must keep the delta exact. + const baseNs = BigInt(base.split(":")[3]) + const microNs = BigInt(plusOneMicrosecond.split(":")[3]) + expect(microNs - baseNs).toBe(1_000n) + }) + + it("handles sizes beyond Number.MAX_SAFE_INTEGER without precision loss", () => { + const size = 10_000_000_000_000_001n // 10^16 + 1 > 2^53 + const token = versionTokenOfStat(makeStats({ size })) + expect(token).toBe(`7:4242:${size}:1700000000123456789:1700000000789999999`) + }) +}) + +describe("computeVersionToken (A1, epic #1375)", () => { + let tmpDir: string + let file: string + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "version-token-")) + file = path.join(tmpDir, "seed.txt") + await fs.writeFile(file, "seed content", "utf8") + }) + + afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }) + }) + + it("derives the token from the on-disk state (single bigint stat)", async () => { + const token = await computeVersionToken(file) + expect(token).toBe(versionTokenOfStat(await fs.stat(file, { bigint: true }))) + }) + + it("changes when the file content changes", async () => { + const before = await computeVersionToken(file) + // Different size + a new mtime — both must move the token. + await fs.writeFile(file, "seed content, extended", "utf8") + await new Promise((resolve) => setTimeout(resolve, 5)) + expect(await computeVersionToken(file)).not.toBe(before) + }) + + it("rejects with ENOENT for an absent file", async () => { + await expect(computeVersionToken(path.join(tmpDir, "absent.txt"))).rejects.toMatchObject({ + code: "ENOENT", + }) + }) +}) diff --git a/src/utils/versionToken.ts b/src/utils/versionToken.ts new file mode 100644 index 0000000000..1738280087 --- /dev/null +++ b/src/utils/versionToken.ts @@ -0,0 +1,48 @@ +import { stat } from "fs/promises" +import type { BigIntStats } from "fs" + +/** + * Version token for the compare-and-swap write guard (upstream epic #1375, phase A1). + * + * A token is a pure function of a file's on-disk state, derived from a single + * `fs.stat(path, { bigint: true })`, so every process that observes the same file + * state (a second VS Code window, the CLI, the user's own editor tooling) computes + * the same token. The downstream guard phases (A2/A3) compare the token observed at + * read time with the token recomputed just before a write to detect "the file + * changed since the read" (stale) or "the file was replaced by a different file" + * (dev/ino change). + * + * Format: `dev:ino:size:mtimeNs:ctimeNs` + * + * Precision: the stat is fetched in `bigint` mode, so all five fields are exact + * `BigInt` values rendered as decimal strings — no float is involved anywhere. + * There is therefore no precision loss for large sizes or inodes (a Windows file ID + * exceeds 2^53 and is still exact), and the ns timestamps are the kernel's exact + * nanosecond values rather than a ms→ns derivation (no ~256 ns double-precision + * quantum). Guarantee: same disk state → same token, deterministic across + * processes; any change to size, file identity, or mtime/ctime → a different token. + * + * Platform note: on POSIX `ctime` is the last file-status change; on Windows it is + * the file creation time. The token only requires it to move when the file's + * metadata is replaced, which holds on both. + */ + +/** + * Build the version token from an already-fetched `BigIntStats` — no I/O. + * + * Exported separately from {@link computeVersionToken} so tests can pin the exact + * format against synthetic stats. + */ +export function versionTokenOfStat(stats: BigIntStats): string { + return [stats.dev, stats.ino, stats.size, stats.mtimeNs, stats.ctimeNs].map((value) => value.toString()).join(":") +} + +/** + * Compute the version token for a file (one `fs.stat` in bigint mode). + * + * Rejects with the underlying ENOENT (or equivalent) error when the file is absent; + * how an unobservable target is treated is decided by the guard layer (A3). + */ +export async function computeVersionToken(filePath: string): Promise { + return versionTokenOfStat(await stat(filePath, { bigint: true })) +}