From 8fb22fdfe24b66ca1df3218fee2a0ce77beb712a Mon Sep 17 00:00:00 2001 From: RenzoMXD <170978465+RenzoMXD@users.noreply.github.com> Date: Fri, 8 May 2026 12:07:47 +0200 Subject: [PATCH] fix(agents): compare file-target structurally not via fingerprint split Address clawsweeper P2 on PR #79067: the prior cross-tool recovery extracted the path target by splitting the joined fingerprint string on `|`, which is also a legal character in file paths. A failed edit on `/tmp/a|left` and a successful write to `/tmp/a|right` would both extract as `path=/tmp/a` and incorrectly clear the prior failure. Carry a structured `fileTarget: { path?, oldpath? }` alongside the existing `actionFingerprint` string and compare it directly. `extractFileTarget` reads args once at fingerprint-build time, with the same alias support as `buildToolActionFingerprint`. The fingerprint string is unchanged for diagnostics and the exact-equality match path; only the cross-tool fallback now compares structurally. Threaded through `ToolMutationState`, `ToolActionRef`, `ToolCallSummary`, and `ToolErrorSummary` so the existing handler code at `pi-embedded-subscribe.handlers.tools.ts:910-928` can populate and consume it without re-parsing. Adds delimiter-bearing-path regression test asserting that `/tmp/a|left` vs `/tmp/a|right` returns false, and that an identical delimiter-bearing path on both sides still matches. --- .../pi-embedded-subscribe.handlers.tools.ts | 3 + .../pi-embedded-subscribe.handlers.types.ts | 1 + src/agents/tool-error-summary.ts | 2 + src/agents/tool-mutation.test.ts | 123 ++++++++++++++++-- src/agents/tool-mutation.ts | 107 +++++++++------ 5 files changed, 183 insertions(+), 53 deletions(-) diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.ts b/src/agents/pi-embedded-subscribe.handlers.tools.ts index a434dbce12e..a795f02c168 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.ts @@ -120,6 +120,7 @@ function buildToolCallSummary(toolName: string, args: unknown, meta?: string): T meta, mutatingAction: mutation.mutatingAction, actionFingerprint: mutation.actionFingerprint, + fileTarget: mutation.fileTarget, }; } @@ -914,6 +915,7 @@ export async function handleToolExecutionEnd( timedOut: isToolResultTimedOut(sanitizedResult) || undefined, mutatingAction: callSummary?.mutatingAction, actionFingerprint: callSummary?.actionFingerprint, + fileTarget: callSummary?.fileTarget, }; } else if (ctx.state.lastToolError) { // Keep unresolved mutating failures until the same action succeeds. @@ -923,6 +925,7 @@ export async function handleToolExecutionEnd( toolName, meta, actionFingerprint: callSummary?.actionFingerprint, + fileTarget: callSummary?.fileTarget, }) ) { ctx.state.lastToolError = undefined; diff --git a/src/agents/pi-embedded-subscribe.handlers.types.ts b/src/agents/pi-embedded-subscribe.handlers.types.ts index 4556dcdadf6..3b62e97712c 100644 --- a/src/agents/pi-embedded-subscribe.handlers.types.ts +++ b/src/agents/pi-embedded-subscribe.handlers.types.ts @@ -25,6 +25,7 @@ export type ToolCallSummary = { meta?: string; mutatingAction: boolean; actionFingerprint?: string; + fileTarget?: import("./tool-mutation.js").FileTarget; }; export type EmbeddedPiSubscribeState = { diff --git a/src/agents/tool-error-summary.ts b/src/agents/tool-error-summary.ts index 39a9264ae7d..f38628d40eb 100644 --- a/src/agents/tool-error-summary.ts +++ b/src/agents/tool-error-summary.ts @@ -1,4 +1,5 @@ import { normalizeOptionalLowercaseString } from "../shared/string-coerce.js"; +import type { FileTarget } from "./tool-mutation.js"; export type ToolErrorSummary = { toolName: string; @@ -7,6 +8,7 @@ export type ToolErrorSummary = { timedOut?: boolean; mutatingAction?: boolean; actionFingerprint?: string; + fileTarget?: FileTarget; }; const EXEC_LIKE_TOOL_NAMES = new Set(["exec", "bash"]); diff --git a/src/agents/tool-mutation.test.ts b/src/agents/tool-mutation.test.ts index b3b39421cbc..84e47e651ad 100644 --- a/src/agents/tool-mutation.test.ts +++ b/src/agents/tool-mutation.test.ts @@ -88,27 +88,68 @@ describe("tool mutation helpers", () => { ).toBe(false); }); + it("populates structured fileTarget for file-mutating calls (#79024)", () => { + expect(buildToolMutationState("edit", { file_path: "/tmp/a" }).fileTarget).toEqual({ + path: "/tmp/a", + }); + expect(buildToolMutationState("write", { path: "/tmp/Foo|bar" }).fileTarget).toEqual({ + path: "/tmp/foo|bar", + }); + // Non-file-mutating tools never carry fileTarget, even with a path arg. + expect(buildToolMutationState("bash", { command: "rm /tmp/a" }).fileTarget).toBeUndefined(); + expect(buildToolMutationState("exec", { command: "touch /tmp/a" }).fileTarget).toBeUndefined(); + // apply_patch is excluded from file-mutating set, so no fileTarget even + // if a path-shaped arg is synthetically present. + expect( + buildToolMutationState("apply_patch", { input: "*** Update File: /tmp/a" }).fileTarget, + ).toBeUndefined(); + }); + it("recognizes cross-tool file-mutation recovery on the same target (#79024)", () => { expect( isSameToolMutationAction( - { toolName: "edit", actionFingerprint: "tool=edit|path=/tmp/a" }, - { toolName: "write", actionFingerprint: "tool=write|path=/tmp/a" }, + { + toolName: "edit", + actionFingerprint: "tool=edit|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, + { + toolName: "write", + actionFingerprint: "tool=write|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, ), ).toBe(true); expect( isSameToolMutationAction( - { toolName: "write", actionFingerprint: "tool=write|path=/tmp/a" }, - { toolName: "edit", actionFingerprint: "tool=edit|path=/tmp/a" }, + { + toolName: "write", + actionFingerprint: "tool=write|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, + { + toolName: "edit", + actionFingerprint: "tool=edit|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, ), ).toBe(true); // `apply_patch` is intentionally excluded from the file-mutating set // because production `apply_patch` calls only carry opaque `input` text, - // so real fingerprints never have a `path=` segment to compare. Even a - // synthetic path-bearing fingerprint must not unlock recovery. + // so `extractFileTarget` returns `undefined` and the fail-closed branch + // refuses cross-tool recovery. expect( isSameToolMutationAction( - { toolName: "edit", actionFingerprint: "tool=edit|path=/tmp/a" }, - { toolName: "apply_patch", actionFingerprint: "tool=apply_patch|path=/tmp/a" }, + { + toolName: "edit", + actionFingerprint: "tool=edit|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, + { + toolName: "apply_patch", + actionFingerprint: "tool=apply_patch|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, ), ).toBe(false); }); @@ -116,39 +157,93 @@ describe("tool mutation helpers", () => { it("does not cross-recover file mutations on different targets (#79024)", () => { expect( isSameToolMutationAction( - { toolName: "edit", actionFingerprint: "tool=edit|path=/tmp/a" }, - { toolName: "write", actionFingerprint: "tool=write|path=/tmp/b" }, + { + toolName: "edit", + actionFingerprint: "tool=edit|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, + { + toolName: "write", + actionFingerprint: "tool=write|path=/tmp/b", + fileTarget: { path: "/tmp/b" }, + }, ), ).toBe(false); }); + it("does not over-match paths containing the fingerprint delimiter (#79024)", () => { + // The fingerprint string carries raw paths separated by `|`. A naive + // `split("|")` parser would extract `path=/tmp/a` from both fingerprints + // and incorrectly clear the prior failure. Structural fileTarget + // comparison fails closed for these distinct paths. + expect( + isSameToolMutationAction( + { + toolName: "edit", + actionFingerprint: "tool=edit|path=/tmp/a|left", + fileTarget: { path: "/tmp/a|left" }, + }, + { + toolName: "write", + actionFingerprint: "tool=write|path=/tmp/a|right", + fileTarget: { path: "/tmp/a|right" }, + }, + ), + ).toBe(false); + // Same delimiter-bearing path on both sides still matches. + expect( + isSameToolMutationAction( + { + toolName: "edit", + actionFingerprint: "tool=edit|path=/tmp/a|shared", + fileTarget: { path: "/tmp/a|shared" }, + }, + { + toolName: "write", + actionFingerprint: "tool=write|path=/tmp/a|shared", + fileTarget: { path: "/tmp/a|shared" }, + }, + ), + ).toBe(true); + }); + it("does not cross-recover when the recovery tool is not file-mutating (#79024)", () => { expect( isSameToolMutationAction( - { toolName: "edit", actionFingerprint: "tool=edit|path=/tmp/a" }, + { + toolName: "edit", + actionFingerprint: "tool=edit|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, { toolName: "bash", actionFingerprint: "tool=bash|meta=cat /tmp/a" }, ), ).toBe(false); expect( isSameToolMutationAction( - { toolName: "edit", actionFingerprint: "tool=edit|path=/tmp/a" }, + { + toolName: "edit", + actionFingerprint: "tool=edit|path=/tmp/a", + fileTarget: { path: "/tmp/a" }, + }, { toolName: "exec", actionFingerprint: "tool=exec|meta=touch /tmp/a" }, ), ).toBe(false); }); it("ignores call-specific noise when comparing the cross-tool target (#79024)", () => { - // `id=...` and `meta=...` segments must not block recovery when the - // stable `path=...` target still matches. + // `id=...` and `meta=...` segments differ between calls; structural + // fileTarget comparison is unaffected. expect( isSameToolMutationAction( { toolName: "edit", actionFingerprint: "tool=edit|path=/tmp/a|id=42|meta=edit /tmp/a", + fileTarget: { path: "/tmp/a" }, }, { toolName: "write", actionFingerprint: "tool=write|path=/tmp/a|id=99|meta=write /tmp/a", + fileTarget: { path: "/tmp/a" }, }, ), ).toBe(true); diff --git a/src/agents/tool-mutation.ts b/src/agents/tool-mutation.ts index 2de00307c54..61dc5d2ce0c 100644 --- a/src/agents/tool-mutation.ts +++ b/src/agents/tool-mutation.ts @@ -32,11 +32,9 @@ const MUTATING_TOOL_NAMES = new Set([ // would only match handcrafted-fingerprint test inputs, not real recoveries. const FILE_MUTATING_TOOL_NAMES = new Set(["edit", "write"]); -// Stable target segment produced by `buildToolActionFingerprint` that -// identifies the file being mutated. Other segments (`tool=`, `action=`, -// `id=`, `meta=`) are call-specific and excluded from cross-tool target -// comparison. -const FILE_TARGET_FINGERPRINT_KEYS = new Set(["path"]); +// Args aliases that identify the file target on a file-mutating call. +const FILE_TARGET_PATH_ARG_KEYS = ["path", "file_path", "filePath", "filepath", "file"] as const; +const FILE_TARGET_OLDPATH_ARG_KEYS = ["oldPath", "old_path"] as const; const READ_ONLY_ACTIONS = new Set([ "get", @@ -69,15 +67,28 @@ const MESSAGE_MUTATING_ACTIONS = new Set([ "unpin", ]); +// Structured file-target identity for cross-tool same-target recovery. +// Carried alongside `actionFingerprint` so comparison does not have to +// re-parse the joined fingerprint string. Re-parsing was unsafe because +// `buildToolActionFingerprint` stores raw path values in a `|`-delimited +// string, so a path containing `|` could over-match (e.g. `/tmp/a|left` and +// `/tmp/a|right` would both extract as `path=/tmp/a`). +export type FileTarget = { + path?: string; + oldpath?: string; +}; + type ToolMutationState = { mutatingAction: boolean; actionFingerprint?: string; + fileTarget?: FileTarget; }; type ToolActionRef = { toolName: string; meta?: string; actionFingerprint?: string; + fileTarget?: FileTarget; }; function normalizeActionName(value: unknown): string | undefined { @@ -219,40 +230,60 @@ export function buildToolActionFingerprint( return parts.join("|"); } +function isFileMutatingToolName(rawName: string): boolean { + return FILE_MUTATING_TOOL_NAMES.has(normalizeLowercaseStringOrEmpty(rawName)); +} + +function readArgFingerprintValue( + record: Record | undefined, + keys: readonly string[], +): string | undefined { + if (!record) { + return undefined; + } + for (const key of keys) { + const normalized = normalizeFingerprintValue(record[key]); + if (normalized) { + return normalized; + } + } + return undefined; +} + +export function extractFileTarget(toolName: string, args: unknown): FileTarget | undefined { + if (!isFileMutatingToolName(toolName)) { + return undefined; + } + const record = asRecord(args); + const path = readArgFingerprintValue(record, FILE_TARGET_PATH_ARG_KEYS); + const oldpath = readArgFingerprintValue(record, FILE_TARGET_OLDPATH_ARG_KEYS); + if (!path && !oldpath) { + return undefined; + } + return { + ...(path !== undefined ? { path } : {}), + ...(oldpath !== undefined ? { oldpath } : {}), + }; +} + +function fileTargetsEqual(a: FileTarget, b: FileTarget): boolean { + return (a.path ?? "") === (b.path ?? "") && (a.oldpath ?? "") === (b.oldpath ?? ""); +} + export function buildToolMutationState( toolName: string, args: unknown, meta?: string, ): ToolMutationState { const actionFingerprint = buildToolActionFingerprint(toolName, args, meta); + const fileTarget = extractFileTarget(toolName, args); return { mutatingAction: actionFingerprint != null, actionFingerprint, + ...(fileTarget !== undefined ? { fileTarget } : {}), }; } -function isFileMutatingToolName(rawName: string): boolean { - return FILE_MUTATING_TOOL_NAMES.has(normalizeLowercaseStringOrEmpty(rawName)); -} - -function extractFileTargetFingerprint(fingerprint: string | undefined): string | undefined { - if (!fingerprint) { - return undefined; - } - const segments: string[] = []; - for (const segment of fingerprint.split("|")) { - const eqIndex = segment.indexOf("="); - if (eqIndex < 0) { - continue; - } - const key = segment.slice(0, eqIndex); - if (FILE_TARGET_FINGERPRINT_KEYS.has(key)) { - segments.push(segment); - } - } - return segments.length > 0 ? segments.join("|") : undefined; -} - export function isSameToolMutationAction(existing: ToolActionRef, next: ToolActionRef): boolean { if (existing.actionFingerprint != null || next.actionFingerprint != null) { // For mutating flows, fail closed: only clear when both fingerprints exist @@ -265,18 +296,16 @@ export function isSameToolMutationAction(existing: ToolActionRef, next: ToolActi } // Cross-tool recovery: a successful file-mutation on the same `path` // clears an unresolved file-mutation failure even when the tool name - // differs (e.g. edit→write self-heal). Different paths or - // non-file-mutating tools never qualify. - if (isFileMutatingToolName(existing.toolName) && isFileMutatingToolName(next.toolName)) { - const existingTarget = extractFileTargetFingerprint(existing.actionFingerprint); - const nextTarget = extractFileTargetFingerprint(next.actionFingerprint); - if ( - existingTarget !== undefined && - nextTarget !== undefined && - existingTarget === nextTarget - ) { - return true; - } + // differs (e.g. edit→write self-heal). Compared structurally on + // `fileTarget` so paths containing `|` cannot over-match. + if ( + isFileMutatingToolName(existing.toolName) && + isFileMutatingToolName(next.toolName) && + existing.fileTarget !== undefined && + next.fileTarget !== undefined && + fileTargetsEqual(existing.fileTarget, next.fileTarget) + ) { + return true; } return false; }