-
Notifications
You must be signed in to change notification settings - Fork 270
feat(task): per-task file observation registry (A2, #1375) #1394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
131d18d
13188d2
2c1582b
2965ad1
a00eef8
05c845d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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") | ||
| }) | ||
| }) | ||
| 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 | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,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 }, | ||
|
|
@@ -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) | ||
|
Comment on lines
+1541
to
+1555
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Cover version-token lookup failure after a successful read. This test rejects As per path instructions, “Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases.” 🤖 Prompt for AI AgentsSource: Path instructions |
||
| }) | ||
|
|
||
| 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") | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge Zoo-Code-Org/Zoo-Code /tmp/coderabbit-repo-knowledge/zoo-code-org-zoo-code-6665e61c/architecture /tmp/coderabbit-repo-knowledge/zoo-code-org-zoo-code-6665e61c/conventions /tmp/coderabbit-repo-knowledge/zoo-code-org-zoo-code-6665e61c/learningsLength of output: 41195
🏁 Script executed:
sed -n '1,180p' src/core/task/__tests__/observationRegistry.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 2454
🏁 Script executed:
Repository: Zoo-Code-Org/Zoo-Code
Length of output: 2958
🏁 Script executed:
#!/bin/sh cat -n src/core/task/__tests__/observationRegistry.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 2958
🏁 Script executed:
Repository: Zoo-Code-Org/Zoo-Code
Length of output: 20132
Restore fake timers in teardown.
If an assertion fails before
vi.useRealTimers(), subsequent tests can inherit Vitest’s fake timers and mockedDate. Move cleanup toafterEach, or usetry/finally.🤖 Prompt for AI Agents
Source: Path instructions