Skip to content

Commit dd060a4

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(tools): take the read version token before the read, and pin the review-raised cases
Both read paths computed the observation token after reading the file and after awaiting trackFileContext, so a mutation in that window left the registry holding a token newer than the bytes this read returned and the write guard would accept a stale write. The token is now taken before the read and recorded only after the read succeeds, on the native and the legacy path alike, through one file-local helper so the two paths cannot drift apart - the two blocks were character-identical once comments are stripped, which I diffed line by line before folding them together. The direction is a deliberate choice: a mutation during the read now yields a token that mismatches and blocks the guarded write, which is the conservative answer for a guard. The pinning test mutates the file inside the read so the pre-read and post-read tokens differ, and names which one the tool may record; with the stat moved back after the read it reddens - expected '1:2:300:4000000000:5000000000', received '1:2:300:9000000000:5000000000' - and the implementation is restored from a saved copy (hash 7053a7861143cb42dd9a0dc0bd3f1a83115d0007 before and after the control). Also strengthens the assertions review flagged, with no behaviour change: observe assertions compare the resolved full path instead of a substring, so a call made with the relative path can no longer pass (native site and both legacy sites); the beforeEach stat mock now returns the BigIntStats shape the tool reads, because the partial double made computeVersionToken throw a TypeError that its .catch swallowed, laundering a mock gap into "no token"; the stale-entry test arms the stat failure on the call that passes { bigint: true }, the only call that does, instead of on call parity; and the registry test pins the exact observedAt under a fake clock instead of accepting any number, which both 0 and NaN satisfied. Measured: readFileTool.spec.ts + observationRegistry.spec.ts 102 passed, Task.spec.ts 201 passed, check-types 11/11 exit 0, root lint exit 0, eslint clean on the touched files, and test:solheim-smoke-ci fails identically with these files reverted to the merge commit, so its red is pre-existing on this merge and not caused here. Addresses review feedback on #1394. Related: #1375
1 parent 9fe680c commit dd060a4

4 files changed

Lines changed: 113 additions & 41 deletions

File tree

‎src/core/task/__tests__/observationRegistry.spec.ts‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,21 @@ import { describe, it, expect, vi } from "vitest"
33
import { ObservationRegistry } from "../observationRegistry"
44

55
describe("ObservationRegistry", () => {
6-
it("observe → get returns the recorded version and observedAt", () => {
7-
const reg = new ObservationRegistry()
8-
reg.observe("/a/b/c.ts", "1:2:300:4000000000:5000000000")
9-
10-
const obs = reg.get("/a/b/c.ts")
11-
expect(obs).toBeDefined()
12-
expect(obs!.version).toBe("1:2:300:4000000000:5000000000")
13-
expect(typeof obs!.observedAt).toBe("number")
6+
it("observe → get returns the recorded version and the clock time it observed at", () => {
7+
// typeof number accepts 0 and NaN, so pin the exact timestamp instead: a
8+
// broken clock that returns some other number must not pass.
9+
vi.useFakeTimers()
10+
vi.setSystemTime(new Date(Date.UTC(2025, 0, 1, 0, 0, 0, 0)))
11+
try {
12+
const reg = new ObservationRegistry()
13+
reg.observe("/a/b/c.ts", "1:2:300:4000000000:5000000000")
14+
const obs = reg.get("/a/b/c.ts")
15+
expect(obs).toBeDefined()
16+
expect(obs!.version).toBe("1:2:300:4000000000:5000000000")
17+
expect(obs!.observedAt).toBe(Date.UTC(2025, 0, 1, 0, 0, 0, 0))
18+
} finally {
19+
vi.useRealTimers()
20+
}
1421
})
1522

1623
it("re-observe replaces the entry with a fresh observedAt", () => {

‎src/core/tools/ReadFileTool.ts‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -216,22 +216,16 @@ export class ReadFileTool extends BaseTool<"read_file"> {
216216
// Read text file content with lossy UTF-8 conversion
217217
// Reading as Buffer first allows graceful handling of non-UTF8 bytes
218218
// (they become U+FFFD replacement characters instead of throwing)
219+
// Observe the on-disk version before reading so the token can never be newer than
220+
// the bytes this read returns; see observeReadVersion.
221+
const version = await computeVersionToken(fullPath).catch(() => undefined)
219222
const buffer = await fs.readFile(fullPath)
220223
const fileContent = buffer.toString("utf-8")
221224
const result = this.processTextFile(fileContent, entry)
222225

223226
await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource)
224227

225-
// A2 (plan #33 / epic #1375): record the observed on-disk version for the future write guard.
226-
// A stat failure leaves the target unobserved and never fails the read. It must also
227-
// drop any earlier entry, otherwise the guard still holds a token from a previous
228-
// read that no longer describes this one.
229-
const version = await computeVersionToken(fullPath).catch(() => undefined)
230-
if (version) {
231-
task.observationRegistry.observe(fullPath, version)
232-
} else {
233-
task.observationRegistry.forget(fullPath)
234-
}
228+
observeReadVersion(task, fullPath, version)
235229

236230
updateFileResult(relPath, {
237231
nativeContent: `File: ${relPath}\n${result}`,
@@ -782,6 +776,9 @@ export class ReadFileTool extends BaseTool<"read_file"> {
782776
}
783777

784778
// Read text file
779+
// Observe the on-disk version before reading so the token can never be newer than
780+
// the bytes this read returns; see observeReadVersion.
781+
const version = await computeVersionToken(fullPath).catch(() => undefined)
785782
const rawContent = await fs.readFile(fullPath, "utf8")
786783

787784
// Handle line ranges if specified
@@ -814,16 +811,7 @@ export class ReadFileTool extends BaseTool<"read_file"> {
814811
// Track file in context
815812
await task.fileContextTracker.trackFileContext(relPath, "read_tool")
816813

817-
// A2 (plan #33 / epic #1375): mirror the native path — record the observed
818-
// on-disk version so legacy-format reads also feed the future write guard.
819-
// A stat failure leaves the target unobserved and never fails the read, and drops any
820-
// entry from an earlier read so the guard cannot compare against a stale token.
821-
const version = await computeVersionToken(fullPath).catch(() => undefined)
822-
if (version) {
823-
task.observationRegistry.observe(fullPath, version)
824-
} else {
825-
task.observationRegistry.forget(fullPath)
826-
}
814+
observeReadVersion(task, fullPath, version)
827815
} catch (error) {
828816
const errorMsg = error instanceof Error ? error.message : String(error)
829817
results.push(`File: ${relPath}\nError: ${errorMsg}`)
@@ -839,3 +827,24 @@ export class ReadFileTool extends BaseTool<"read_file"> {
839827
}
840828

841829
export const readFileTool = new ReadFileTool()
830+
831+
/**
832+
* Record (or drop) the version of a file this tool just read, in one place so the
833+
* native and legacy read paths cannot drift apart.
834+
*
835+
* A present token is stored as observed. A missing token means the stat failed, so
836+
* the target is unobserved and any earlier entry must be dropped: that token came from
837+
* a previous read and no longer describes this file, and leaving it would let the write
838+
* guard compare against a stale version. The token itself is taken before the read by
839+
* both call sites, which is deliberate - a token taken afterwards could be newer than
840+
* the content the model saw, and a guard built on it would green-light a stale write.
841+
* The conservative direction is the one chosen here: a mutation during the read yields
842+
* a token that mismatches and blocks the guarded write.
843+
*/
844+
function observeReadVersion(task: Task, fullPath: string, version: string | undefined): void {
845+
if (version) {
846+
task.observationRegistry.observe(fullPath, version)
847+
} else {
848+
task.observationRegistry.forget(fullPath)
849+
}
850+
}

‎src/core/tools/__tests__/readFileTool.spec.ts‎

Lines changed: 68 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,17 @@ describe("ReadFileTool", () => {
196196
vi.clearAllMocks()
197197

198198
// Default mock implementations
199-
mockedFsStat.mockResolvedValue({ isDirectory: () => false } as any)
199+
// Full BigIntStats shape: a partial double makes computeVersionToken throw a
200+
// TypeError that its .catch swallows, which launders a mock gap into "no token"
201+
// and silently skips observation in every test that does not override this mock.
202+
mockedFsStat.mockResolvedValue({
203+
isDirectory: () => false,
204+
dev: BigInt(1),
205+
ino: BigInt(2),
206+
size: BigInt(300),
207+
mtimeNs: BigInt(4_000_000_000n),
208+
ctimeNs: BigInt(5_000_000_000n),
209+
} as unknown as Stats)
200210
mockedIsBinaryFile.mockResolvedValue(false)
201211
mockedFsReadFile.mockResolvedValue(Buffer.from("test content"))
202212
mockedReadWithSlice.mockReturnValue({
@@ -1726,14 +1736,56 @@ describe("ReadFileTool", () => {
17261736
// Verify the tool called observe exactly once with a valid token.
17271737
expect(observeSpy).toHaveBeenCalledTimes(1)
17281738
const [calledPath, calledVersion] = observeSpy.mock.calls[0]
1729-
expect(calledPath).toContain("existing.ts")
1739+
expect(calledPath).toBe(path.resolve("/test/workspace", "existing.ts"))
17301740
expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/)
17311741

17321742
// Verify get() returns the same data using the spy-captured key.
17331743
const obs = reg.get(calledPath)
17341744
expect(obs).toBeDefined()
17351745
expect(obs!.version).toBe(calledVersion)
17361746
})
1747+
it("observes the token taken before the read, not the one after it", async () => {
1748+
// A token computed after the read can describe bytes the model never saw: if the
1749+
// file changes between the read and the stat, the registry ends up holding a newer
1750+
// token than the content the tool returned, and the write guard would then accept a
1751+
// stale write. This fixture mutates the file inside the read itself, so the pre-read
1752+
// and post-read tokens differ and the assertion names which one may be recorded. The
1753+
// direction is chosen deliberately: a mid-read mutation must mismatch and block the
1754+
// guarded write, which is the conservative answer for a guard.
1755+
const mockTask = createMockTask({
1756+
observationRegistry: new ObservationRegistry(),
1757+
})
1758+
const callbacks = createMockCallbacks()
1759+
// The directory-check stat and the token stat share this mock. Only the token
1760+
// lookup passes { bigint: true }, so the mutation flag decides which token shape it
1761+
// returns; readStarted records whether the read had already happened.
1762+
let readStarted = false
1763+
mockedFsStat.mockImplementation(
1764+
async (_path: unknown, options?: { bigint?: boolean }) =>
1765+
({
1766+
isDirectory: () => false,
1767+
dev: BigInt(1),
1768+
ino: BigInt(2),
1769+
size: BigInt(300),
1770+
mtimeNs: BigInt(options?.bigint && readStarted ? 9_000_000_000n : 4_000_000_000n),
1771+
ctimeNs: BigInt(5_000_000_000n),
1772+
}) as unknown as Stats,
1773+
)
1774+
mockedIsBinaryFile.mockResolvedValue(false)
1775+
mockedFsReadFile.mockImplementation(async () => {
1776+
readStarted = true
1777+
return Buffer.from("content")
1778+
})
1779+
const reg = mockTask.observationRegistry!
1780+
const observeSpy = vi.spyOn(reg, "observe")
1781+
// Cast: the mock task only implements the members ReadFileTool.execute touches.
1782+
await readFileTool.execute({ path: "existing.ts" }, mockTask as unknown as Task, callbacks)
1783+
expect(observeSpy).toHaveBeenCalledTimes(1)
1784+
const [, calledVersion] = observeSpy.mock.calls[0]
1785+
// 4000000000 is the pre-read mtimeNs; 9000000000 is the mutated file.
1786+
expect(calledVersion).toBe("1:2:300:4000000000:5000000000")
1787+
expect(readStarted).toBe(true)
1788+
})
17371789

17381790
it("a failed read (absent path) leaves the registry size 0 and does not throw", async () => {
17391791
const mockTask = createMockTask({
@@ -1813,11 +1865,13 @@ describe("ReadFileTool", () => {
18131865

18141866
// Each read stats the path twice: once to rule out a directory and once for
18151867
// the version token. Only the token lookup is made to fail.
1816-
let statCalls = 0
1868+
// Only computeVersionToken asks for bigint stats, so arm the failure on that
1869+
// call rather than on call parity, which ties the test to how many stats a
1870+
// read happens to make.
18171871
let failTokenLookup = false
1818-
mockedFsStat.mockImplementation(async () => {
1819-
statCalls++
1820-
if (failTokenLookup && statCalls % 2 === 0) {
1872+
mockedFsStat.mockImplementation(async (_path: unknown, options?: { bigint?: boolean }) => {
1873+
// Cast: the mock only needs the option the real caller passes.
1874+
if (failTokenLookup && options?.bigint === true) {
18211875
throw new Error("EACCES")
18221876
}
18231877
return okStats
@@ -1928,11 +1982,13 @@ describe("ReadFileTool", () => {
19281982

19291983
// Each read stats the path twice: once to rule out a directory and once for
19301984
// the version token. Only the token lookup is made to fail.
1931-
let statCalls = 0
1985+
// Only computeVersionToken asks for bigint stats, so arm the failure on that
1986+
// call rather than on call parity, which ties the test to how many stats a
1987+
// read happens to make.
19321988
let failTokenLookup = false
1933-
mockedFsStat.mockImplementation(async () => {
1934-
statCalls++
1935-
if (failTokenLookup && statCalls % 2 === 0) {
1989+
mockedFsStat.mockImplementation(async (_path: unknown, options?: { bigint?: boolean }) => {
1990+
// Cast: the mock only needs the option the real caller passes.
1991+
if (failTokenLookup && options?.bigint === true) {
19361992
throw new Error("EACCES")
19371993
}
19381994
return okStats
@@ -1948,7 +2004,7 @@ describe("ReadFileTool", () => {
19482004
await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks)
19492005
expect(observeSpy).toHaveBeenCalledTimes(1)
19502006
const [observedPath, observedVersion] = observeSpy.mock.calls[0]
1951-
expect(observedPath).toContain("legacy.ts")
2007+
expect(observedPath).toBe(path.resolve("/test/workspace", "legacy.ts"))
19522008
expect(reg.get(observedPath)?.version).toBe(observedVersion)
19532009

19542010
failTokenLookup = true
@@ -1996,7 +2052,7 @@ describe("ReadFileTool", () => {
19962052

19972053
expect(observeSpy).toHaveBeenCalledTimes(1)
19982054
const [calledPath, calledVersion] = observeSpy.mock.calls[0]
1999-
expect(calledPath).toContain("legacy.ts")
2055+
expect(calledPath).toBe(path.resolve("/test/workspace", "legacy.ts"))
20002056
expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/)
20012057
})
20022058

‎src/eslint-suppressions.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -861,7 +861,7 @@
861861
},
862862
"core/tools/__tests__/readFileTool.spec.ts": {
863863
"@typescript-eslint/no-explicit-any": {
864-
"count": 96
864+
"count": 95
865865
}
866866
},
867867
"core/tools/__tests__/runSlashCommandTool.spec.ts": {

0 commit comments

Comments
 (0)