Skip to content

fix(exec): keep framework verb out of backtick-wrapped failure message (#97319)#97359

Closed
ly-wang19 wants to merge 1 commit into
openclaw:mainfrom
ly-wang19:fix/exec-failure-verb-outside-command-97319
Closed

fix(exec): keep framework verb out of backtick-wrapped failure message (#97319)#97359
ly-wang19 wants to merge 1 commit into
openclaw:mainfrom
ly-wang19:fix/exec-failure-verb-outside-command-97319

Conversation

@ly-wang19

@ly-wang19 ly-wang19 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

When an exec/bash tool call fails, the chat and cron run history currently render the failure as:

⚠️ 🛠️ `run python3 /path/to/script.py` failed

The framework display verb run sits inside the backticks, adjacent to the actual command. Users debugging failures read this as "the agent typed run python3 /path" — the bug reporter burned ~45 minutes concluding the agent was prepending run to its commands when in fact the cause was an exec exit-code failure (issue #97319).

Why This Change Was Made

formatToolAggregate backtick-wraps the entire meta string for exec/bash tools. The meta string is the display label produced by summarizeKnownExec (src/agents/tool-display-exec.ts), which always starts with a known action verb (run, check, view, show, list, …). The wrapping has no concept of "this leading word is a framework label, not part of the literal command".

This PR adds stripLeadingExecDisplayVerb and uses it in the failure-warning builder (payloads.ts) so only the subject of the action is backtick-wrapped:

Before After
⚠️ 🛠️ \run python3 /path` failed` ⚠️ 🛠️ \python3 /path` failed`
⚠️ 🛠️ \show some-file (workspace)` failed` ⚠️ 🛠️ \some-file (workspace)` failed`

Scope is the failure-warning path only. Tool progress, streaming, and other formatToolAggregate callers are unchanged.

User Impact

  • Chat tool-failure cards and cron lastError lines now backtick-wrap exactly what was run. The framework action verb no longer leaks into the literal command.
  • Non-exec tools (filesystem, web_search, etc.) are unchanged.
  • Raw shell commands whose first token is not in the known exec-display verb set (e.g. cd ~/dir && ls, git status, npm install) pass through unchanged — the strip helper only fires on labels emitted by summarizeKnownExec.

Evidence

  • New unit tests: src/agents/tool-display.test.ts → stripLeadingExecDisplayVerb covers (a) known verbs (run, check, show, find), (b) non-verb first tokens (cd, git, npm), (c) single-token / empty input.
  • Existing regression test updated: src/agents/embedded-agent-runner/run/payloads.errors.test.ts — the markdown mutation-warning case now expects the verb-stripped shape.
  • Local: npx vitest run src/agents/tool-display.test.ts → 45/45 pass. npx vitest run src/agents/embedded-agent-runner/run/payloads.errors.test.ts → 54/54 pass. npx vitest run src/auto-reply/tool-meta.test.ts src/cron/isolated-agent/ → 406/406 pass.

Closes #97319.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S r: too-many-prs Auto-close: author has more than twenty active PRs. labels Jun 28, 2026
@clawsweeper

clawsweeper Bot commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 28, 2026, 12:31 AM ET / 04:31 UTC.

Summary
The PR changes exec/bash failure-warning formatting by stripping a leading display verb from exec metadata before markdown-wrapping the warning text.

PR surface: Source +59, Tests +28. Total +87 across 4 files.

Reproducibility: yes. Source inspection shows current main appends failed to formatToolAggregate(...), and exec summaries can produce command-shaped labels such as run python3 ...; I did not run tests because this review is read-only.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #97319
Summary: This PR is a candidate fix for the canonical exec failure-message ambiguity, with one sibling PR attempting a narrower formatter change.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Change the implementation so summaries like install dependencies and start app remain intelligible in failure warnings.
  • [P1] Add redacted real after-fix proof from a chat, cron, CLI, terminal, or log path showing the corrected warning text.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists unit-test runs only; it needs redacted real chat, cron, terminal, log, screenshot, recording, or linked artifact proof of the after-fix warning before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging as-is would make some common exec failures less intelligible, for example turning install dependencies into dependencies failed.
  • [P1] No after-fix chat, cron, terminal, log, screenshot, recording, or linked artifact proof shows the actual rendered warning in a real setup.

Maintainer options:

  1. Decide the mitigation before merge
    Revise the failure-warning presentation so the tool failure label is separate from exec metadata while preserving meaningful summaries and raw-command details, then add redacted real behavior proof.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Author or maintainer revision is needed because the code shape needs a small design correction and contributor-side real behavior proof cannot be supplied by an automated repair lane.

Security
Cleared: The diff only changes TypeScript display formatting and colocated tests; it does not touch secrets, dependency resolution, CI, package publishing, or code-execution setup.

Review findings

  • [P2] Preserve semantic exec summary verbs — src/agents/tool-display-exec.ts:316-317
Review details

Best possible solution:

Revise the failure-warning presentation so the tool failure label is separate from exec metadata while preserving meaningful summaries and raw-command details, then add redacted real behavior proof.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main appends failed to formatToolAggregate(...), and exec summaries can produce command-shaped labels such as run python3 ...; I did not run tests because this review is read-only.

Is this the best way to solve the issue?

No. The PR fixes one ambiguity by deleting leading words, but that is too broad for summaries where the first word is meaningful; preserving the summary and changing the failure rendering is the safer shape.

Full review comments:

  • [P2] Preserve semantic exec summary verbs — src/agents/tool-display-exec.ts:316-317
    This strips every listed leading word before the failure text is formatted. Existing summaries from summarizeKnownExec() include meaningful actions such as install dependencies and start app, so a failed npm install would render as ⚠️ 🛠️ dependencies failed, which is less accurate than current main. Keep those action words and change the failure-warning wrapper instead.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 07b934901a32.

Label changes

Label changes:

  • add P3: The PR addresses a low-severity diagnostic clarity bug with limited blast radius and no data-loss, security, availability, or delivery regression shown.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists unit-test runs only; it needs redacted real chat, cron, terminal, log, screenshot, recording, or linked artifact proof of the after-fix warning before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P3: The PR addresses a low-severity diagnostic clarity bug with limited blast radius and no data-loss, security, availability, or delivery regression shown.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists unit-test runs only; it needs redacted real chat, cron, terminal, log, screenshot, recording, or linked artifact proof of the after-fix warning before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +59, Tests +28. Total +87 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 60 1 +59
Tests 2 30 2 +28
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 90 3 +87

What I checked:

  • Repository policy read: Root AGENTS.md and scoped agent/embedded-runner AGENTS.md files were read fully; their PR review-depth and real-proof guidance apply to this review. (AGENTS.md:1, 07b934901a32)
  • Current failure-warning composition: Current main builds the visible warning as a formatted tool aggregate followed by failed, so this path controls the reported chat/cron failure string. (src/agents/embedded-agent-runner/run/payloads.ts:553, 07b934901a32)
  • Current exec summaries include semantic actions: Current main maps package-manager commands to summaries like install dependencies, run tests, and start app; these first words are meaningful summary actions, not disposable wrappers in every case. (src/agents/tool-display-exec.ts:226, 07b934901a32)
  • PR strips broad action verbs: The PR adds a helper whose verb set includes install and start, so an existing install dependencies or start app summary would lose the action word before formatToolAggregate renders the failure. (src/agents/tool-display-exec.ts:316, 5e72dc8a6b6b)
  • Linked canonical issue remains open: The PR uses closing syntax for the open exec failure-message ambiguity issue, so it is a candidate fix rather than a cleanup-close target.
  • Related open candidate exists: GitHub search found another open PR for the same linked issue; it is related context, not a safe superseding close target because it is still open and also missing real behavior proof.

Likely related people:

What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jun 28, 2026
@openclaw-barnacle

openclaw-barnacle Bot commented Jun 28, 2026

Copy link
Copy Markdown

Closing this PR because the author has more than 20 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

This was referenced Jun 28, 2026
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. r: too-many-prs Auto-close: author has more than twenty active PRs. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: exec tool failure message format makes framework labels indistinguishable from agent-typed commands

1 participant

@ly-wang19