mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-09 03:10:42 +00:00
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.
This commit is contained in:
committed by
Peter Steinberger
parent
3f4c64163d
commit
8fb22fdfe2
@@ -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;
|
||||
|
||||
@@ -25,6 +25,7 @@ export type ToolCallSummary = {
|
||||
meta?: string;
|
||||
mutatingAction: boolean;
|
||||
actionFingerprint?: string;
|
||||
fileTarget?: import("./tool-mutation.js").FileTarget;
|
||||
};
|
||||
|
||||
export type EmbeddedPiSubscribeState = {
|
||||
|
||||
@@ -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"]);
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<string, unknown> | 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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user