Skip to content

Commit 3b4bf28

Browse files
u917 recut u9b-task-history-store-provider-and-read-write-tools
1 parent 94206fb commit 3b4bf28

16 files changed

Lines changed: 3730 additions & 129 deletions

‎src/core/task-persistence/TaskHistoryStore.ts‎

Lines changed: 565 additions & 15 deletions
Large diffs are not rendered by default.

‎src/core/task-persistence/__tests__/TaskHistoryStore.deleteSemantics.spec.ts‎

Lines changed: 1348 additions & 37 deletions
Large diffs are not rendered by default.

‎src/core/task-persistence/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ export {
1313
} from "./taskMessages"
1414
export { taskMetadata } from "./taskMetadata"
1515
export { ensureMessageIdentifiers } from "./mergeMessageSnapshots"
16-
export { TaskHistoryStore } from "./TaskHistoryStore"
16+
export { TaskHistoryDeleteError, TaskHistoryStore } from "./TaskHistoryStore"
1717
export {
1818
abandonDelegatedChild,
1919
assertValidTransition,

‎src/core/tools/ApplyDiffTool.ts‎

Lines changed: 41 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,17 @@ import { TelemetryService } from "@roo-code/telemetry"
66

77
import { getReadablePath } from "../../utils/path"
88
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
9+
import { versionTokenOfStat } from "../../utils/versionToken"
910
import { Task } from "../task/Task"
1011
import { formatResponse } from "../prompts/responses"
1112
import { fileExistsAtPath } from "../../utils/fs"
1213
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
1314
import { unescapeHtmlEntities } from "../../utils/text-normalization"
1415
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
1516
import { computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats"
16-
import { versionTokenOfStat } from "../../utils/versionToken"
1717
import type { DiffResult, ToolUse } from "../../shared/tools"
1818

19+
import { canonicalizeForApproval } from "./guardedWrite"
1920
import { BaseTool, ToolCallbacks } from "./BaseTool"
2021

2122
interface ApplyDiffParams {
@@ -75,21 +76,24 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
7576
return
7677
}
7778

78-
// The tool's own read is what the diff hunks are matched against, and the guarded publish below
79-
// consumes an observation: leave it unrecorded and the approved publish is an unobserved write the
80-
// guard refuses, even though this tool just read the exact bytes the patch was applied to. Record
81-
// it stat-matched (S2) and incomplete -- a hunk read authorises only the lines the patch touched,
82-
// so it must not become authority for a later full-file replacement. A prior observation is
83-
// carried, partial stays partial and complete stays complete, only while it still describes the
84-
// version this read published; an older one is left exactly as the model earned it, since
85-
// refreshing it here would hand the guarded save a compare-and-swap token for content the model
86-
// never saw. Same policy as ApplyPatchTool's read.
79+
// The diff below is built from this exact read, so the save that follows must be
80+
// authorized against the version captured here - not against whatever version the
81+
// preview happens to stat afterwards. Same contract as ApplyPatchTool's hunk read:
82+
// stat around the read and observe only when the file did not change underneath it.
8783
const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined)
8884
const originalContent: string = await fs.readFile(absolutePath, "utf-8")
8985
const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined)
9086
if (preReadStats && postReadStats) {
9187
const preReadToken = versionTokenOfStat(preReadStats)
9288
if (preReadToken === versionTokenOfStat(postReadStats)) {
89+
// A tool read is not a model read. With no prior observation this stays a
90+
// partial observation of the version the diff was computed against - the only
91+
// authorization the save can have, since apply_diff computes its hunks from
92+
// this read. When the model already observed the file, keep the completeness it
93+
// earned, but only on the version it was earned on: refreshing an OLDER
94+
// observation to the current version would let content the model built from a
95+
// stale read pass the compare-and-swap, so an out-of-date observation is left
96+
// alone and the save fails with the re-read remediation.
9397
const prior = task.observationRegistry.get(absolutePath)
9498
if (prior === undefined) {
9599
task.observationRegistry.observe(absolutePath, preReadToken, false)
@@ -162,7 +166,19 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
162166

163167
// Check if file is write-protected
164168
const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false
169+
170+
// The guard in guardedWrite refuses a target outside every workspace root unless the tool
171+
// layer says the user approved this one, and it binds the publish to the identity that was
172+
// approved. apply_diff has both save paths behind an askApproval, so it has to carry both
173+
// through; without them an approved write outside the workspace is rejected by its own guard.
165174
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
175+
// Captured before the approval is asked: guardedWrite binds the publish to this identity, so
176+
// a name repointed between the approval and the publish is refused instead of publishing to
177+
// whatever the name points at by then. Inside the try so an unresolvable target goes through
178+
// the tool's normal error handling and the diff-view reset.
179+
const approvedCanonicalTarget = isOutsideWorkspace
180+
? await canonicalizeForApproval(absolutePath, relPath)
181+
: undefined
166182

167183
const sharedMessageProps: ClineSayTool = {
168184
tool: "appliedDiff",
@@ -200,7 +216,8 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
200216
return
201217
}
202218

203-
// Save directly without showing diff view or opening the file
219+
// Save directly without showing diff view or opening the file. The diff is
220+
// applied to an existing file, so edit-guard semantics require a prior read.
204221
task.diffViewProvider.editType = "modify"
205222
task.diffViewProvider.originalContent = originalContent
206223
await task.diffViewProvider.saveDirectly(
@@ -209,10 +226,12 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
209226
false,
210227
diagnosticsEnabled,
211228
writeDelayMs,
212-
// An apply_diff publish is a targeted hunk over content this tool read, so it is guarded as an
213-
// edit: a partial observation authorises it and stays partial. On the "create" default the guard
214-
// reads it as a fresh file and refuses the diff the user just approved.
215229
"edit",
230+
// Seventh parameter is completeOverride (this call claims no completeness of its
231+
// own); the eighth is the approval flag for a target outside every workspace root.
232+
undefined,
233+
isOutsideWorkspace,
234+
approvedCanonicalTarget,
216235
)
217236
} else {
218237
// Original behavior with diff view
@@ -251,12 +270,14 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
251270
return
252271
}
253272

254-
// Call saveChanges to update the DiffViewProvider properties. The save is the same targeted hunk
255-
// as the guarded save above, so it must select the same edit guard: on the "update" default the
256-
// guard reads the accepted preview as a full-file replacement and demands a complete observation
257-
// the model never earned, which throws the approved diff away behind a re-read-the-whole-file
258-
// remediation.
259-
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs, "edit")
273+
// Call saveChanges to update the DiffViewProvider properties
274+
await task.diffViewProvider.saveChanges(
275+
diagnosticsEnabled,
276+
writeDelayMs,
277+
"edit",
278+
isOutsideWorkspace,
279+
approvedCanonicalTarget,
280+
)
260281
}
261282

262283
// Track file edit operation

‎src/core/tools/ReadFileTool.ts‎

Lines changed: 93 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,14 @@ import type { ReadFileParams, ReadFileMode, ReadFileToolParams, FileEntry, LineR
1717
import { isLegacyReadFileParams, type ClineSayTool } from "@roo-code/types"
1818

1919
import { Task } from "../task/Task"
20+
import { versionTokenOfStat } from "../../utils/versionToken"
2021
import { formatResponse } from "../prompts/responses"
2122
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
2223
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
2324
import { getReadablePath } from "../../utils/path"
2425
import { extractTextFromFile, addLineNumbers, getSupportedBinaryFormats } from "../../integrations/misc/extract-text"
2526
import { readWithIndentation, readWithSlice } from "../../integrations/misc/indentation-reader"
26-
import { DEFAULT_LINE_LIMIT } from "../prompts/tools/native-tools/read_file"
27+
import { DEFAULT_LINE_LIMIT, MAX_LINE_LENGTH } from "../prompts/tools/native-tools/read_file"
2728
import type { ToolUse, PushToolResult } from "../../shared/tools"
2829

2930
import {
@@ -215,14 +216,36 @@ export class ReadFileTool extends BaseTool<"read_file"> {
215216
// Read text file content with lossy UTF-8 conversion
216217
// Reading as Buffer first allows graceful handling of non-UTF8 bytes
217218
// (they become U+FFFD replacement characters instead of throwing)
219+
// A2 (epic #1375): capture the on-disk token before the read so a mutation
220+
// landing mid-read is detected by the post-read stat below.
221+
const preReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined)
218222
const buffer = await fs.readFile(fullPath)
219223
const fileContent = buffer.toString("utf-8")
220-
const result = this.processTextFile(fileContent, entry)
224+
// A lossy decode is not the whole file: the model never saw those bytes.
225+
const lossyDecode = !Buffer.from(fileContent).equals(buffer)
226+
// S4b follow-up (#46 / epic #1375): processTextFile reports whether the
227+
// returned content is the whole file; the observation below records that
228+
// scope so the write guard can deny full-file updates built on a partial view.
229+
const processed = this.processTextFile(fileContent, entry)
221230

222231
await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource)
223232

233+
// A2 (plan #33 / epic #1375): record the observed on-disk version for the future write guard.
234+
// The token is captured before AND after the read; the target is observed only
235+
// when both match — a mutation between the two stats means the content the model
236+
// received is not the on-disk state, and observing it would let a later write
237+
// match a token the model never saw. A stat failure leaves the target
238+
// unobserved and never fails the read.
239+
const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined)
240+
if (preReadStats && postReadStats) {
241+
const preReadToken = versionTokenOfStat(preReadStats)
242+
if (preReadToken === versionTokenOfStat(postReadStats)) {
243+
task.observationRegistry.observe(fullPath, preReadToken, processed.complete && !lossyDecode)
244+
}
245+
}
246+
224247
updateFileResult(relPath, {
225-
nativeContent: `File: ${relPath}\n${result}`,
248+
nativeContent: `File: ${relPath}\n${processed.content}`,
226249
})
227250
} catch (error) {
228251
const errorMsg = error instanceof Error ? error.message : String(error)
@@ -266,8 +289,14 @@ export class ReadFileTool extends BaseTool<"read_file"> {
266289

267290
/**
268291
* Process a text file according to the requested mode.
292+
*
293+
* Returns the content string plus whether that content is the complete
294+
* file (S4b follow-up #46 / epic #1375): slice mode is complete only
295+
* when it starts at line 1, returns every line, and was not truncated;
296+
* indentation mode is never complete because it returns semantic blocks
297+
* of the file, not the file itself.
269298
*/
270-
private processTextFile(content: string, entry: InternalFileEntry): string {
299+
private processTextFile(content: string, entry: InternalFileEntry): { content: string; complete: boolean } {
271300
const mode = entry.mode || "slice"
272301

273302
if (mode === "indentation") {
@@ -300,7 +329,8 @@ export class ReadFileTool extends BaseTool<"read_file"> {
300329
output += `\n\nIncluded ranges: ${rangeStr} (total: ${result.totalLines} lines)`
301330
}
302331

303-
return output
332+
// Indentation mode returns semantic blocks: never a complete file view.
333+
return { content: output, complete: false }
304334
}
305335

306336
// Slice mode (default): simple offset/limit reading
@@ -323,11 +353,35 @@ export class ReadFileTool extends BaseTool<"read_file"> {
323353
To read more: Use the read_file tool with offset=${nextOffset} and limit=${limit}.
324354
325355
${result.content}`
356+
if (result.hasClippedLines) {
357+
// The slice cut lines off and also clipped long lines inside it, so both
358+
// notices belong to the response.
359+
output += `\nNote: Some lines in this view exceed ${MAX_LINE_LENGTH} characters and were clipped in this view.`
360+
}
361+
} else if (result.hasClippedLines) {
362+
// The slice reached the end of the file, so there is no later offset to read: report
363+
// the clipping without a next-offset hint, and keep the read incomplete so a full-file
364+
// replacement cannot be built from a clipped line. "Read in full" is only true from the
365+
// requested offset onward, and a model that reads it literally would think the lines
366+
// before that offset were included.
367+
const coverage =
368+
offset0 === 0 ? "The file was read in full" : `The file was read from line ${offset1} onward`
369+
output = `IMPORTANT: Some lines exceed ${MAX_LINE_LENGTH} characters and were clipped in this view. ${coverage}, but the clipped lines were not shown in full.
370+
${result.content}`
326371
} else if (result.returnedLines === 0 && result.totalLines === 0) {
372+
// Only a truly empty file is reported as empty. A slice that starts past the last line
373+
// returns the producer's beyond-EOF text (src/integrations/misc/indentation-reader.ts),
374+
// which must reach the model instead of being replaced by an empty-file note.
327375
output = "Note: File is empty"
328376
}
329377

330-
return output
378+
// Complete only when the slice starts at line 1, returned every line, and
379+
// showed every line in full (returnedLines === totalLines follows from the
380+
// first two conditions): a partial start, a truncated tail, or a clipped
381+
// line means the model did not see the whole file.
382+
const complete = offset0 === 0 && !result.wasTruncated && !result.hasClippedLines
383+
384+
return { content: output, complete }
331385
}
332386

333387
/**
@@ -770,9 +824,20 @@ export class ReadFileTool extends BaseTool<"read_file"> {
770824
}
771825

772826
// Read text file
773-
const rawContent = await fs.readFile(fullPath, "utf8")
827+
// A2 (epic #1375): capture the on-disk token before the read so a mutation
828+
// landing mid-read is detected by the post-read stat below.
829+
const preReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined)
830+
const rawBuffer = await fs.readFile(fullPath)
831+
const rawContent = rawBuffer.toString("utf-8")
832+
// Same contract: a lossy decode is a partial view.
833+
const lossyDecode = !Buffer.from(rawContent).equals(rawBuffer)
774834

775835
// Handle line ranges if specified
836+
// S4b follow-up (#46 / epic #1375): a line-range read returns only the requested
837+
// ranges, and a slice truncated to DEFAULT_LINE_LIMIT returns only the head of
838+
// the file — record such observations as partial so the write guard denies a
839+
// full-file update built on them.
840+
let readComplete = false
776841
let content: string
777842
if (entry.lineRanges && entry.lineRanges.length > 0) {
778843
const lines = rawContent.split("\n")
@@ -792,15 +857,36 @@ export class ReadFileTool extends BaseTool<"read_file"> {
792857
// Read with default limits using slice mode
793858
const result = readWithSlice(rawContent, 0, DEFAULT_LINE_LIMIT)
794859
content = result.content
860+
readComplete = !result.wasTruncated && !result.hasClippedLines
795861
if (result.wasTruncated) {
796862
content += `\n\n[File truncated: showing ${result.returnedLines} of ${result.totalLines} total lines]`
863+
if (result.hasClippedLines) {
864+
// Both notices: the slice was truncated and a line inside it was
865+
// clipped.
866+
content += `\n\n[Some lines exceed the per-line length cap and were clipped in this view]`
867+
}
868+
} else if (result.hasClippedLines) {
869+
content += `\n\n[Some lines exceed the per-line length cap and were clipped in this view]`
797870
}
798871
}
799872

800873
results.push(`File: ${relPath}\n${content}`)
801874

802875
// Track file in context
803876
await task.fileContextTracker.trackFileContext(relPath, "read_tool")
877+
878+
// A2 (plan #33 / epic #1375): mirror the native path — record the observed
879+
// on-disk version so legacy-format reads also feed the future write guard.
880+
// Observe only when the pre-read and post-read tokens match (a mutation between
881+
// them means the returned content is not the on-disk state). A stat failure
882+
// leaves the target unobserved and never fails the read.
883+
const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined)
884+
if (preReadStats && postReadStats) {
885+
const preReadToken = versionTokenOfStat(preReadStats)
886+
if (preReadToken === versionTokenOfStat(postReadStats)) {
887+
task.observationRegistry.observe(fullPath, preReadToken, readComplete && !lossyDecode)
888+
}
889+
}
804890
} catch (error) {
805891
const errorMsg = error instanceof Error ? error.message : String(error)
806892
results.push(`File: ${relPath}\nError: ${errorMsg}`)

‎src/core/tools/WriteToFileTool.ts‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } fr
1616
import type { ToolUse } from "../../shared/tools"
1717

1818
import { BaseTool, ToolCallbacks } from "./BaseTool"
19+
import { canonicalizeForApproval } from "./guardedWrite"
1920

2021
interface WriteToFileParams {
2122
path: string
@@ -86,7 +87,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
8687

8788
const fullPath = relPath ? path.resolve(task.cwd, relPath) : ""
8889
const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
89-
9090
const sharedMessageProps: ClineSayTool = {
9191
tool: fileExists ? "editedExistingFile" : "newFileCreated",
9292
path: getReadablePath(task.cwd, relPath),
@@ -96,6 +96,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
9696
}
9797

9898
try {
99+
// Captured before the approval is asked: guardedWrite binds the publish to this
100+
// identity, so a name repointed between the approval and the publish is refused
101+
// instead of publishing to whatever the name points at by then. Inside the try so a
102+
// target that cannot be resolved goes through the tool's normal error handling
103+
// (handleError and the diff-view reset) instead of escaping the write flow.
104+
const approvedCanonicalTarget = isOutsideWorkspace
105+
? await canonicalizeForApproval(absolutePath, relPath)
106+
: undefined
99107
task.consecutiveMistakeCount = 0
100108

101109
const provider = task.providerRef.deref()
@@ -132,7 +140,21 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
132140
return
133141
}
134142

135-
await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs)
143+
// Guarded publish: this write carries the complete file content, so it uses
144+
// create-guard semantics (unobserved targets may only be created when absent).
145+
await task.diffViewProvider.saveDirectly(
146+
relPath,
147+
newContent,
148+
false,
149+
diagnosticsEnabled,
150+
writeDelayMs,
151+
"create",
152+
// Seventh parameter is completeOverride (this call claims no completeness of its
153+
// own); the eighth is the approval flag for a target outside every workspace root.
154+
undefined,
155+
isOutsideWorkspace,
156+
approvedCanonicalTarget,
157+
)
136158
} else {
137159
if (!task.diffViewProvider.isEditing) {
138160
const partialMessage = JSON.stringify(sharedMessageProps)
@@ -164,7 +186,16 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
164186
return
165187
}
166188

167-
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
189+
await task.diffViewProvider.saveChanges(
190+
diagnosticsEnabled,
191+
writeDelayMs,
192+
// Name the guard explicitly: a whole-file write is an update, and
193+
// relying on the parameter's default would silently re-select a
194+
// different guard if the default ever moved.
195+
"update",
196+
isOutsideWorkspace,
197+
approvedCanonicalTarget,
198+
)
168199
}
169200

170201
if (relPath) {

0 commit comments

Comments
 (0)