Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -181,6 +182,7 @@ export class Task extends EventEmitter<TaskEvents> 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
Expand Down
72 changes: 72 additions & 0 deletions src/core/task/__tests__/observationRegistry.spec.ts
Original file line number Diff line number Diff line change
@@ -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")
})
})
47 changes: 47 additions & 0 deletions src/core/task/observationRegistry.ts
Original file line number Diff line number Diff line change
@@ -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<string, FileObservation>()

/**
* 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
}
}
12 changes: 12 additions & 0 deletions src/core/tools/ReadFileTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

updateFileResult(relPath, {
nativeContent: `File: ${relPath}\n${result}`,
})
Expand Down Expand Up @@ -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}`)
Expand Down
113 changes: 113 additions & 0 deletions src/core/tools/__tests__/readFileTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -136,13 +142,17 @@ interface MockTaskOptions {
rooIgnoreAllowed?: boolean
maxImageFileSize?: number
maxTotalImageSize?: number
observationRegistry?: ObservationRegistry
}

function createMockTask(options: MockTaskOptions = {}) {
const { supportsImages = false, rooIgnoreAllowed = true, maxImageFileSize = 5, maxTotalImageSize = 20 } = options

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 },
Expand Down Expand Up @@ -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")
})
})
})
})
Loading
Loading