Skip to content

Commit 3361738

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
u917 recut u9c-diffview-publish-and-edit-tools
1 parent f98fcf7 commit 3361738

10 files changed

Lines changed: 4443 additions & 223 deletions

‎src/core/tools/ApplyPatchTool.ts‎

Lines changed: 151 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
1111
import { fileExistsAtPath } from "../../utils/fs"
1212
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
1313
import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats"
14+
import { versionTokenOfStat } from "../../utils/versionToken"
15+
import { canonicalizeForApproval, GuardRejectedError, errorCode } from "./guardedWrite"
1416
import { BaseTool, ToolCallbacks } from "./BaseTool"
1517
import type { ToolUse } from "../../shared/tools"
1618
import { parsePatch, ParseError, processAllHunks } from "./apply-patch"
@@ -85,10 +87,38 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
8587
return
8688
}
8789

88-
// Process each hunk
90+
// Process each hunk. The read doubles as the S2 observation for the
91+
// guarded publish (ReadFileTool contract: stat before and after the
92+
// read, observe only when the on-disk version is unchanged between the
93+
// two stats). Without it, the in-place modify publish is an unobserved
94+
// write and the composed chat-diff default rejects it ("File already
95+
// exists ... and was not read before this write") even though this tool
96+
// just read the exact content the patch was applied to.
8997
const readFile = async (filePath: string): Promise<string> => {
9098
const absolutePath = path.resolve(task.cwd, filePath)
91-
return await fs.readFile(absolutePath, "utf8")
99+
const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined)
100+
const content: string = await fs.readFile(absolutePath, "utf8")
101+
const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined)
102+
if (preReadStats && postReadStats) {
103+
const preReadToken = versionTokenOfStat(preReadStats)
104+
if (preReadToken === versionTokenOfStat(postReadStats)) {
105+
// The tool's own hunk read, not a model read. With no prior observation the model has
106+
// seen only the patch's hunks, so the read stays incomplete: it must not become authority
107+
// for a later full-file replacement. A prior observation is carried - partial stays partial,
108+
// complete stays complete - only while it still describes the version that was read. An
109+
// OLDER observation is left exactly as the model earned it: refreshing it to the version this
110+
// read published would hand the guarded save a compare-and-swap token for content the model
111+
// never saw, so a patch built against v1 would overwrite an external v2 without the re-read
112+
// remediation. Same stale policy as ApplyDiffTool's read.
113+
const prior = task.observationRegistry.get(absolutePath)
114+
if (prior === undefined) {
115+
task.observationRegistry.observe(absolutePath, preReadToken, false)
116+
} else if (prior.version === preReadToken) {
117+
task.observationRegistry.observe(absolutePath, preReadToken, prior.complete === true)
118+
}
119+
}
120+
}
121+
return content
92122
}
93123

94124
let changes: ApplyPatchFileChange[]
@@ -160,6 +190,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
160190

161191
const newContent = change.newContent || ""
162192
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
193+
// Captured before the approval is asked: guardedWrite binds the publish to this
194+
// identity, so a name repointed between the approval and the publish is refused
195+
// instead of publishing to whatever the name points at by then.
196+
const approvedCanonicalTarget = isOutsideWorkspace
197+
? await canonicalizeForApproval(absolutePath, relPath)
198+
: undefined
163199

164200
// Initialize diff view for new file
165201
task.diffViewProvider.editType = "create"
@@ -214,9 +250,31 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
214250

215251
// Save the changes
216252
if (isPreventFocusDisruptionEnabled) {
217-
await task.diffViewProvider.saveDirectly(relPath, newContent, true, diagnosticsEnabled, writeDelayMs)
253+
// Guarded publish: the patch supplies the complete new content, so create-guard
254+
// semantics apply (an unobserved existing target is rejected, not overwritten).
255+
await task.diffViewProvider.saveDirectly(
256+
relPath,
257+
newContent,
258+
true,
259+
diagnosticsEnabled,
260+
writeDelayMs,
261+
"create",
262+
// Seventh parameter is completeOverride (this call claims no completeness of its
263+
// own); the eighth is the approval flag for a target outside every workspace root.
264+
undefined,
265+
isOutsideWorkspace,
266+
approvedCanonicalTarget,
267+
)
218268
} else {
219-
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
269+
// The add path publishes a whole new file, so create-guard semantics
270+
// apply here as well.
271+
await task.diffViewProvider.saveChanges(
272+
diagnosticsEnabled,
273+
writeDelayMs,
274+
"create",
275+
isOutsideWorkspace,
276+
approvedCanonicalTarget,
277+
)
220278
}
221279

222280
// Track file edit operation
@@ -310,6 +368,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
310368
const originalContent = change.originalContent || ""
311369
const newContent = change.newContent || ""
312370
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
371+
// Captured before the approval is asked: guardedWrite binds the publish to this
372+
// identity, so a name repointed between the approval and the publish is refused
373+
// instead of publishing to whatever the name points at by then.
374+
const approvedCanonicalTarget = isOutsideWorkspace
375+
? await canonicalizeForApproval(absolutePath, relPath)
376+
: undefined
313377

314378
// Initialize diff view
315379
task.diffViewProvider.editType = "modify"
@@ -396,6 +460,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
396460

397461
// Check if destination path is outside workspace
398462
const isMoveOutsideWorkspace = isPathOutsideWorkspace(moveAbsolutePath)
463+
// A destination outside every workspace root is refused right here, so the publish at the
464+
// end of this branch is always an inside-workspace one: it needs neither the approval flag
465+
// nor a canonical identity. Capturing the identity here instead - as this did - ran
466+
// canonicalizeForApproval for a destination that was about to be refused, so a destination
467+
// whose name could not be resolved surfaced an exception where the user should get the tool
468+
// error below.
399469
if (isMoveOutsideWorkspace) {
400470
task.consecutiveMistakeCount++
401471
task.recordToolError("apply_patch")
@@ -408,12 +478,63 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
408478

409479
// Save new content to the new path
410480
if (isPreventFocusDisruptionEnabled) {
481+
// The destination content is the source file plus one targeted hunk, so it can
482+
// only be as complete as the view the model had of the source. Carry that
483+
// completeness to the destination BEFORE the guarded publish: the guard decides
484+
// completeness at publish time, so a partial view must already be recorded when
485+
// the per-path chain runs. Downgrading only after saveDirectly leaves a window
486+
// in which a concurrent writer sees the destination as complete for content the
487+
// model never fully read.
488+
const sourceObs = task.observationRegistry.get(absolutePath)
489+
const sourceComplete = sourceObs !== undefined && sourceObs.complete === true
490+
if (!sourceComplete) {
491+
// Only carry completeness the model already had for the destination. An
492+
// unobserved destination stays unobserved so the guard's "read before a
493+
// full-file write" rule still applies; recording a partial observation here
494+
// would hand the model authority to edit a file it never read.
495+
const destObs = task.observationRegistry.get(moveAbsolutePath)
496+
if (destObs !== undefined) {
497+
let destAbsent = false
498+
try {
499+
await fs.access(moveAbsolutePath)
500+
} catch (error: unknown) {
501+
// Only an access error that means "not there" is an absence verdict. An
502+
// ELOOP or EACCES is a real I/O failure: reading it as absent would mark
503+
// the destination partial and let the publish run against a path the tool
504+
// never actually observed.
505+
if (errorCode(error) !== "ENOENT") throw error
506+
destAbsent = true
507+
}
508+
if (!destAbsent) {
509+
// Replacing an existing destination with content built from a partial
510+
// source view must be re-authorized by reading the source in full. Reject
511+
// before any state changes and name the source: a remediation that names
512+
// only the destination sends the model to re-read the wrong file, and a
513+
// downgrade that survives a rejected publish loses a full destination read.
514+
throw new GuardRejectedError(
515+
`Cannot move a partially read file onto ${change.movePath}: re-read the whole source (${change.path}) first, then retry.`,
516+
change.movePath,
517+
)
518+
}
519+
// The destination was read and then deleted: the create guard permits the
520+
// publish, and the content is still only as complete as the view the model
521+
// had of the source, so carry the source's completeness instead of the
522+
// destination's stale one.
523+
task.observationRegistry.observe(moveAbsolutePath, destObs.version, false)
524+
}
525+
}
411526
await task.diffViewProvider.saveDirectly(
412527
change.movePath,
413528
newContent,
414529
false,
415530
diagnosticsEnabled,
416531
writeDelayMs,
532+
"create",
533+
sourceComplete,
534+
// Guaranteed by the refusal above: this destination is inside a workspace root,
535+
// so the guard's own containment check is what applies and no approval is claimed.
536+
false,
537+
undefined,
417538
)
418539
} else {
419540
// Write to new path and delete old file
@@ -433,9 +554,33 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
433554
} else {
434555
// Save changes to the same file
435556
if (isPreventFocusDisruptionEnabled) {
436-
await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs)
557+
// Guarded publish: an update hunk is a targeted change, so it is guarded
558+
// as an edit -- a partial observation authorizes it and stays partial. A
559+
// stale observed version is still rejected with the re-read hint.
560+
await task.diffViewProvider.saveDirectly(
561+
relPath,
562+
newContent,
563+
false,
564+
diagnosticsEnabled,
565+
writeDelayMs,
566+
"edit",
567+
// Seventh parameter is completeOverride (this call claims no completeness of its
568+
// own); the eighth is the approval flag for a target outside every workspace root.
569+
undefined,
570+
isOutsideWorkspace,
571+
approvedCanonicalTarget,
572+
)
437573
} else {
438-
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
574+
// The diff-view save is the same targeted hunk as the guarded save above:
575+
// it must select the same edit guard, otherwise a partial read is
576+
// rejected and the approved patch is thrown away.
577+
await task.diffViewProvider.saveChanges(
578+
diagnosticsEnabled,
579+
writeDelayMs,
580+
"edit",
581+
isOutsideWorkspace,
582+
approvedCanonicalTarget,
583+
)
439584
}
440585

441586
await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource)

‎src/core/tools/EditFileTool.ts‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats"
1414
import type { ToolUse } from "../../shared/tools"
1515

1616
import { BaseTool, ToolCallbacks } from "./BaseTool"
17+
import { canonicalizeForApproval } from "./guardedWrite"
1718

1819
interface EditFileParams {
1920
file_path: string
@@ -400,6 +401,12 @@ export class EditFileTool extends BaseTool<"edit_file"> {
400401
const sanitizedDiff = sanitizeUnifiedDiff(diff || "")
401402
const diffStats = computeDiffStats(sanitizedDiff) || undefined
402403
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
404+
// Captured before the approval is asked: guardedWrite binds the publish to this
405+
// identity, so a name repointed between the approval and the publish is refused
406+
// instead of publishing to whatever the name points at by then.
407+
const approvedCanonicalTarget = isOutsideWorkspace
408+
? await canonicalizeForApproval(absolutePath, relPath)
409+
: undefined
403410

404411
const sharedMessageProps: ClineSayTool = {
405412
tool: isNewFile ? "newFileCreated" : "appliedDiff",
@@ -436,17 +443,31 @@ export class EditFileTool extends BaseTool<"edit_file"> {
436443

437444
// Save the changes
438445
if (isPreventFocusDisruptionEnabled) {
439-
// Direct file write without diff view or opening the file
446+
// Direct file write without diff view or opening the file. In-place edits
447+
// use edit-guard semantics (a prior read is required); new-file creation
448+
// keeps create-guard semantics.
440449
await task.diffViewProvider.saveDirectly(
441450
relPath,
442451
newContent,
443452
isNewFile,
444453
diagnosticsEnabled,
445454
writeDelayMs,
455+
isNewFile ? "create" : "edit",
456+
// Seventh parameter is completeOverride (this call claims no completeness of its
457+
// own); the eighth is the approval flag for a target outside every workspace root.
458+
undefined,
459+
isOutsideWorkspace,
460+
approvedCanonicalTarget,
446461
)
447462
} else {
448463
// Call saveChanges to update the DiffViewProvider properties
449-
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
464+
await task.diffViewProvider.saveChanges(
465+
diagnosticsEnabled,
466+
writeDelayMs,
467+
isNewFile ? "create" : "edit",
468+
isOutsideWorkspace,
469+
approvedCanonicalTarget,
470+
)
450471
}
451472

452473
// Track file edit operation

‎src/core/tools/EditTool.ts‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats"
1414
import type { ToolUse } from "../../shared/tools"
1515

1616
import { BaseTool, ToolCallbacks } from "./BaseTool"
17+
import { canonicalizeForApproval } from "./guardedWrite"
1718

1819
interface EditParams {
1920
file_path: string
@@ -175,6 +176,12 @@ export class EditTool extends BaseTool<"edit"> {
175176
const sanitizedDiff = sanitizeUnifiedDiff(diff)
176177
const diffStats = computeDiffStats(sanitizedDiff) || undefined
177178
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
179+
// Captured before the approval is asked: guardedWrite binds the publish to this
180+
// identity, so a name repointed between the approval and the publish is refused
181+
// instead of publishing to whatever the name points at by then.
182+
const approvedCanonicalTarget = isOutsideWorkspace
183+
? await canonicalizeForApproval(absolutePath, relPath)
184+
: undefined
178185

179186
const sharedMessageProps: ClineSayTool = {
180187
tool: "appliedDiff",
@@ -211,11 +218,30 @@ export class EditTool extends BaseTool<"edit"> {
211218

212219
// Save the changes
213220
if (isPreventFocusDisruptionEnabled) {
214-
// Direct file write without diff view or opening the file
215-
await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs)
221+
// Direct file write without diff view or opening the file. This tool only
222+
// edits existing files, so edit-guard semantics require a prior read.
223+
await task.diffViewProvider.saveDirectly(
224+
relPath,
225+
newContent,
226+
false,
227+
diagnosticsEnabled,
228+
writeDelayMs,
229+
"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,
235+
)
216236
} else {
217237
// Call saveChanges to update the DiffViewProvider properties
218-
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
238+
await task.diffViewProvider.saveChanges(
239+
diagnosticsEnabled,
240+
writeDelayMs,
241+
"edit",
242+
isOutsideWorkspace,
243+
approvedCanonicalTarget,
244+
)
219245
}
220246

221247
// Track file edit operation

‎src/core/tools/SearchReplaceTool.ts‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats"
1414
import type { ToolUse } from "../../shared/tools"
1515

1616
import { BaseTool, ToolCallbacks } from "./BaseTool"
17+
import { canonicalizeForApproval } from "./guardedWrite"
1718

1819
interface SearchReplaceParams {
1920
file_path: string
@@ -171,6 +172,12 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> {
171172
const sanitizedDiff = sanitizeUnifiedDiff(diff)
172173
const diffStats = computeDiffStats(sanitizedDiff) || undefined
173174
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
175+
// Captured before the approval is asked: guardedWrite binds the publish to this
176+
// identity, so a name repointed between the approval and the publish is refused
177+
// instead of publishing to whatever the name points at by then.
178+
const approvedCanonicalTarget = isOutsideWorkspace
179+
? await canonicalizeForApproval(absolutePath, relPath)
180+
: undefined
174181

175182
const sharedMessageProps: ClineSayTool = {
176183
tool: "appliedDiff",
@@ -207,11 +214,30 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> {
207214

208215
// Save the changes
209216
if (isPreventFocusDisruptionEnabled) {
210-
// Direct file write without diff view or opening the file
211-
await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs)
217+
// Direct file write without diff view or opening the file. This tool only
218+
// edits existing files, so edit-guard semantics require a prior read.
219+
await task.diffViewProvider.saveDirectly(
220+
relPath,
221+
newContent,
222+
false,
223+
diagnosticsEnabled,
224+
writeDelayMs,
225+
"edit",
226+
// Seventh parameter is completeOverride (this call claims no completeness of its
227+
// own); the eighth is the approval flag for a target outside every workspace root.
228+
undefined,
229+
isOutsideWorkspace,
230+
approvedCanonicalTarget,
231+
)
212232
} else {
213233
// Call saveChanges to update the DiffViewProvider properties
214-
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
234+
await task.diffViewProvider.saveChanges(
235+
diagnosticsEnabled,
236+
writeDelayMs,
237+
"edit",
238+
isOutsideWorkspace,
239+
approvedCanonicalTarget,
240+
)
215241
}
216242

217243
// Track file edit operation

0 commit comments

Comments
 (0)