From 0e068194aded2c5d86cc7d6b31b5fc11262c116a Mon Sep 17 00:00:00 2001 From: Taras Lukavyi Date: Sat, 21 Feb 2026 03:33:32 +0100 Subject: [PATCH] =?UTF-8?q?fix(tool-display):=20`cd=20~/dir=20&&=20npm=20i?= =?UTF-8?q?nstall`=20shows=20as=20`run=20cd`=20=E2=80=94=20compound=20comm?= =?UTF-8?q?ands=20truncated=20to=20first=20stage=20(#21925)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merged via /review-pr -> /prepare-pr -> /merge-pr. Prepared head SHA: 4728bfe8e75dfcdf21f9ac22e7a26d081dc95d93 Co-authored-by: Lukavyi <1013690+Lukavyi@users.noreply.github.com> Co-authored-by: obviyus <22031114+obviyus@users.noreply.github.com> Reviewed-by: @obviyus --- CHANGELOG.md | 1 + src/agents/tool-display-common.ts | 224 +++++++++++++++++++++++----- src/agents/tool-display.e2e.test.ts | 157 +++++++++++++++++++ 3 files changed, 348 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00998bdf6..0944b1d62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ Docs: https://docs.openclaw.ai - Gateway/Auth: require `gateway.trustedProxies` to include a loopback proxy address when `auth.mode="trusted-proxy"` and `bind="loopback"`, preventing same-host proxy misconfiguration from silently blocking auth. (#22082, follow-up to #20097) thanks @mbelinky. - Security/OpenClawKit/UI: prevent injected inbound user context metadata blocks from leaking into chat history in TUI, webchat, and macOS surfaces by stripping all untrusted metadata prefixes at display boundaries. (#22142) Thanks @Mellowambience, @vincentkoc. - Agents/System Prompt: label allowlisted senders as authorized senders to avoid implying ownership. Thanks @thewilloftheshadow. +- Agents/Tool display: fix exec cwd suffix inference so `pushd ... && popd ... && ` does not keep stale `(in )` context in summaries. (#21925) thanks @Lukavyi. - Gateway/Auth: allow trusted-proxy mode with loopback bind for same-host reverse-proxy deployments, while still requiring configured `gateway.trustedProxies`. (#20097) thanks @xinhuagu. - Gateway/Auth: allow authenticated clients across roles/scopes to call `health` while preserving role and scope enforcement for non-health methods. (#19699) thanks @Nachx639. - Gateway/Security: remove shared-IP fallback for canvas endpoints and require token or session capability for canvas access. Thanks @thewilloftheshadow. diff --git a/src/agents/tool-display-common.ts b/src/agents/tool-display-common.ts index 28fcb0045..35551530b 100644 --- a/src/agents/tool-display-common.ts +++ b/src/agents/tool-display-common.ts @@ -520,20 +520,26 @@ function scanTopLevelChars( } } -function firstTopLevelStage(command: string): string { - let splitIndex = -1; +function splitTopLevelStages(command: string): string[] { + const parts: string[] = []; + let start = 0; + scanTopLevelChars(command, (char, index) => { if (char === ";") { - splitIndex = index; - return false; + parts.push(command.slice(start, index)); + start = index + 1; + return true; } if ((char === "&" || char === "|") && command[index + 1] === char) { - splitIndex = index; - return false; + parts.push(command.slice(start, index)); + start = index + 2; + return true; } return true; }); - return splitIndex >= 0 ? command.slice(0, splitIndex) : command; + + parts.push(command.slice(start)); + return parts.map((part) => part.trim()).filter((part) => part.length > 0); } function splitTopLevelPipes(command: string): string[] { @@ -552,38 +558,79 @@ function splitTopLevelPipes(command: string): string[] { return parts.map((part) => part.trim()).filter((part) => part.length > 0); } -function stripShellPreamble(command: string): string { +function parseChdirTarget(head: string): string | undefined { + const words = splitShellWords(head, 3); + const bin = binaryName(words[0]); + if (bin === "cd" || bin === "pushd") { + return words[1] || undefined; + } + return undefined; +} + +function isChdirCommand(head: string): boolean { + const bin = binaryName(splitShellWords(head, 2)[0]); + return bin === "cd" || bin === "pushd" || bin === "popd"; +} + +function isPopdCommand(head: string): boolean { + return binaryName(splitShellWords(head, 2)[0]) === "popd"; +} + +type PreambleResult = { + command: string; + chdirPath?: string; +}; + +function stripShellPreamble(command: string): PreambleResult { let rest = command.trim(); + let chdirPath: string | undefined; for (let i = 0; i < 4; i += 1) { - const andIndex = rest.indexOf("&&"); - const semicolonIndex = rest.indexOf(";"); - const newlineIndex = rest.indexOf("\n"); - - const candidates = [ - { index: andIndex, length: 2 }, - { index: semicolonIndex, length: 1 }, - { index: newlineIndex, length: 1 }, - ] - .filter((candidate) => candidate.index >= 0) - .toSorted((a, b) => a.index - b.index); - - const first = candidates[0]; + // Find the first top-level separator (&&, ||, ;, \n) respecting quotes/escaping. + let first: { index: number; length: number; isOr?: boolean } | undefined; + scanTopLevelChars(rest, (char, idx) => { + if (char === "&" && rest[idx + 1] === "&") { + first = { index: idx, length: 2 }; + return false; + } + if (char === "|" && rest[idx + 1] === "|") { + first = { index: idx, length: 2, isOr: true }; + return false; + } + if (char === ";" || char === "\n") { + first = { index: idx, length: 1 }; + return false; + } + }); const head = (first ? rest.slice(0, first.index) : rest).trim(); + // cd/pushd/popd is preamble when followed by && / ; / \n, or when we already + // stripped at least one preamble segment (handles chained cd's like `cd /tmp && cd /app`). + // NOT for || — `cd /app || npm install` means npm runs when cd *fails*, so (in /app) is wrong. + const isChdir = (first ? !first.isOr : i > 0) && isChdirCommand(head); const isPreamble = - head.startsWith("set ") || head.startsWith("export ") || head.startsWith("unset "); + head.startsWith("set ") || head.startsWith("export ") || head.startsWith("unset ") || isChdir; if (!isPreamble) { break; } + if (isChdir) { + // popd returns to the previous directory, so inferred cwd from earlier + // preamble steps is no longer reliable. + if (isPopdCommand(head)) { + chdirPath = undefined; + } else { + chdirPath = parseChdirTarget(head) ?? chdirPath; + } + } + rest = first ? rest.slice(first.index + first.length).trimStart() : ""; if (!rest) { break; } } - return rest.trim(); + return { command: rest.trim(), chdirPath }; } function summarizeKnownExec(words: string[]): string { @@ -853,13 +900,7 @@ function summarizeKnownExec(words: string[]): string { return /^[A-Za-z0-9._/-]+$/.test(arg) ? `run ${bin} ${arg}` : `run ${bin}`; } -function summarizeExecCommand(command: string): string | undefined { - const cleaned = stripShellPreamble(command); - const stage = firstTopLevelStage(cleaned).trim(); - if (!stage) { - return cleaned ? summarizeKnownExec(trimLeadingEnv(splitShellWords(cleaned))) : undefined; - } - +function summarizePipeline(stage: string): string { const pipeline = splitTopLevelPipes(stage); if (pipeline.length > 1) { const first = summarizeKnownExec(trimLeadingEnv(splitShellWords(pipeline[0]))); @@ -867,10 +908,108 @@ function summarizeExecCommand(command: string): string | undefined { const extra = pipeline.length > 2 ? ` (+${pipeline.length - 2} steps)` : ""; return `${first} -> ${last}${extra}`; } - return summarizeKnownExec(trimLeadingEnv(splitShellWords(stage))); } +type ExecSummary = { + text: string; + chdirPath?: string; + allGeneric?: boolean; +}; + +function summarizeExecCommand(command: string): ExecSummary | undefined { + const { command: cleaned, chdirPath } = stripShellPreamble(command); + if (!cleaned) { + // All segments were preamble (e.g. `cd /tmp && cd /app`) — preserve chdirPath for context. + return chdirPath ? { text: "", chdirPath } : undefined; + } + + const stages = splitTopLevelStages(cleaned); + if (stages.length === 0) { + return undefined; + } + + const summaries = stages.map((stage) => summarizePipeline(stage)); + const text = summaries.length === 1 ? summaries[0] : summaries.join(" → "); + const allGeneric = summaries.every((s) => isGenericSummary(s)); + + return { text, chdirPath, allGeneric }; +} + +/** Known summarizer prefixes that indicate a recognized command with useful context. */ +const KNOWN_SUMMARY_PREFIXES = [ + "check git", + "view git", + "show git", + "list git", + "switch git", + "create git", + "pull git", + "push git", + "fetch git", + "merge git", + "rebase git", + "stage git", + "restore git", + "reset git", + "stash git", + "search ", + "find files", + "list files", + "show first", + "show last", + "print line", + "print text", + "copy ", + "move ", + "remove ", + "create folder", + "create file", + "fetch http", + "install dependencies", + "run tests", + "run build", + "start app", + "run lint", + "run openclaw", + "run node script", + "run node ", + "run python", + "run ruby", + "run php", + "run sed", + "run git ", + "run npm ", + "run pnpm ", + "run yarn ", + "run bun ", + "check js syntax", +]; + +/** True when the summary is generic and the raw command would be more informative. */ +function isGenericSummary(summary: string): boolean { + if (summary === "run command") { + return true; + } + // "run " or "run " without useful context + if (summary.startsWith("run ")) { + return !KNOWN_SUMMARY_PREFIXES.some((prefix) => summary.startsWith(prefix)); + } + return false; +} + +/** Compact the raw command for display: collapse whitespace, trim long strings. */ +function compactRawCommand(raw: string, maxLength = 120): string { + const oneLine = raw + .replace(/\s*\n\s*/g, " ") + .replace(/\s{2,}/g, " ") + .trim(); + if (oneLine.length <= maxLength) { + return oneLine; + } + return `${oneLine.slice(0, Math.max(0, maxLength - 1))}…`; +} + export function resolveExecDetail(args: unknown): string | undefined { const record = asRecord(args); if (!record) { @@ -883,7 +1022,8 @@ export function resolveExecDetail(args: unknown): string | undefined { } const unwrapped = unwrapShellWrapper(raw); - const summary = summarizeExecCommand(unwrapped) ?? summarizeExecCommand(raw) ?? "run command"; + const result = summarizeExecCommand(unwrapped) ?? summarizeExecCommand(raw); + const summary = result?.text || "run command"; const cwdRaw = typeof record.workdir === "string" @@ -891,9 +1031,25 @@ export function resolveExecDetail(args: unknown): string | undefined { : typeof record.cwd === "string" ? record.cwd : undefined; - const cwd = cwdRaw?.trim(); + // Explicit workdir takes priority; fall back to cd path extracted from the command. + const cwd = cwdRaw?.trim() || result?.chdirPath || undefined; - return cwd ? `${summary} (in ${cwd})` : summary; + const compact = compactRawCommand(unwrapped); + + // When ALL stages are generic (e.g. "run jj"), use the compact raw command instead. + // For mixed stages like "run cargo build → run tests", keep the summary since some parts are useful. + if (result?.allGeneric !== false && isGenericSummary(summary)) { + return cwd ? `${compact} (in ${cwd})` : compact; + } + + const displaySummary = cwd ? `${summary} (in ${cwd})` : summary; + + // Append the raw command when the summary differs meaningfully from the command itself. + if (compact && compact !== displaySummary && compact !== summary) { + return `${displaySummary}\n\n\`${compact}\``; + } + + return displaySummary; } export function resolveActionSpec( diff --git a/src/agents/tool-display.e2e.test.ts b/src/agents/tool-display.e2e.test.ts index b50f88c8e..b41db4d05 100644 --- a/src/agents/tool-display.e2e.test.ts +++ b/src/agents/tool-display.e2e.test.ts @@ -104,6 +104,163 @@ describe("tool display details", () => { expect(detail).toContain(".openclaw/workspace)"); }); + it("moves cd path to context suffix and appends raw command", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "cd ~/my-project && npm install" }, + }), + ); + + expect(detail).toBe( + "install dependencies (in ~/my-project)\n\n`cd ~/my-project && npm install`", + ); + }); + + it("moves cd path to context suffix with multiple stages and raw command", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "cd ~/my-project && npm install && npm test" }, + }), + ); + + expect(detail).toBe( + "install dependencies → run tests (in ~/my-project)\n\n`cd ~/my-project && npm install && npm test`", + ); + }); + + it("moves pushd path to context suffix and appends raw command", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "pushd /tmp && git status" }, + }), + ); + + expect(detail).toBe("check git status (in /tmp)\n\n`pushd /tmp && git status`"); + }); + + it("clears inferred cwd when popd is stripped from preamble", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "pushd /tmp && popd && npm install" }, + }), + ); + + expect(detail).toBe("install dependencies\n\n`pushd /tmp && popd && npm install`"); + }); + + it("moves cd path to context suffix with || separator", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "cd /app || npm install" }, + }), + ); + + // || means npm install runs when cd FAILS — cd should NOT be stripped as preamble. + // Both stages are summarized; cd is not treated as context prefix. + expect(detail).toMatch(/^run cd \/app → install dependencies/); + }); + + it("explicit workdir takes priority over cd path", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "cd /tmp && npm install", workdir: "/app" }, + }), + ); + + expect(detail).toBe("install dependencies (in /app)\n\n`cd /tmp && npm install`"); + }); + + it("summarizes all stages and appends raw command", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "git fetch && git rebase origin/main" }, + }), + ); + + expect(detail).toBe( + "fetch git changes → rebase git branch\n\n`git fetch && git rebase origin/main`", + ); + }); + + it("falls back to raw command for unknown binaries", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "jj rebase -s abc -d main" }, + }), + ); + + expect(detail).toBe("jj rebase -s abc -d main"); + }); + + it("falls back to raw command for unknown binary with cwd", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "mycli deploy --prod", workdir: "/app" }, + }), + ); + + expect(detail).toBe("mycli deploy --prod (in /app)"); + }); + + it("keeps multi-stage summary when only some stages are generic", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "cargo build && npm test" }, + }), + ); + + // "run cargo build" is generic, but "run tests" is known — keep joined summary + expect(detail).toMatch(/^run cargo build → run tests/); + }); + + it("handles standalone cd as raw command", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "cd /tmp" }, + }), + ); + + // standalone cd (no following command) — treated as raw since it's generic + expect(detail).toBe("cd /tmp"); + }); + + it("handles chained cd commands using last path", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: "cd /tmp && cd /app" }, + }), + ); + + // both cd's are preamble; last path wins + expect(detail).toBe("cd /tmp && cd /app (in /app)"); + }); + + it("respects quotes when splitting preamble separators", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "exec", + args: { command: 'export MSG="foo && bar" && echo test' }, + }), + ); + + // The && inside quotes must not be treated as a separator — + // summary line should be "print text", not "run export" (which would happen + // if the quoted && was mistaken for a real separator). + expect(detail).toMatch(/^print text/); + }); + it("recognizes heredoc/inline script exec details", () => { const pyDetail = formatToolDetail( resolveToolDisplay({