feat(tool-display): add intent-first details and exec summaries#18592
Conversation
- add human-readable read/write/edit/attach details with path alias support\n- add explicit web_search/web_fetch phrasing (quoted query, mode/limit)\n- make detail text title-first by returning detail-only in formatters\n- add deterministic exec summarizer (wrappers, pipelines, heredoc, git/node/python heuristics, preamble stripping)\n- extend e2e coverage for file/web/exec cases
openclaw-barnacle
bot
added
app: web-ui
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/agents/tool-display-common.ts
Outdated
| return `run ${bin} inline script`; | ||
| } | ||
|
|
||
| const script = firstPositional(words, 1, ["-c", "-e", "--eval", "-m"]); |
There was a problem hiding this comment.
node -c misclassified as value-taking option
For Node.js, -c means --check (syntax check) and does not consume the next argument as its value — the filename is a positional argument. However, -c is listed in optionsWithValue here, which causes firstPositional to skip the filename (treating it as -c's value).
Result: node -c /tmp/test.js produces "run node" instead of "check js syntax for /tmp/test.js".
The existing test passes only because it uses --check rather than -c, avoiding the buggy code path. Consider handling node separately before calling firstPositional, or using different optionsWithValue lists per language:
| const script = firstPositional(words, 1, ["-c", "-e", "--eval", "-m"]); | |
| const nodeOptsWithValue = ["-e", "--eval", "-m"]; | |
| const otherOptsWithValue = ["-c", "-e", "--eval", "-m"]; | |
| const script = firstPositional(words, 1, bin === "node" ? nodeOptsWithValue : otherOptsWithValue); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/tool-display-common.ts
Line: 794:794
Comment:
**`node -c` misclassified as value-taking option**
For Node.js, `-c` means `--check` (syntax check) and does *not* consume the next argument as its value — the filename is a positional argument. However, `-c` is listed in `optionsWithValue` here, which causes `firstPositional` to skip the filename (treating it as `-c`'s value).
Result: `node -c /tmp/test.js` produces `"run node"` instead of `"check js syntax for /tmp/test.js"`.
The existing test passes only because it uses `--check` rather than `-c`, avoiding the buggy code path. Consider handling node separately before calling `firstPositional`, or using different `optionsWithValue` lists per language:
```suggestion
const nodeOptsWithValue = ["-e", "--eval", "-m"];
const otherOptsWithValue = ["-c", "-e", "--eval", "-m"];
const script = firstPositional(words, 1, bin === "node" ? nodeOptsWithValue : otherOptsWithValue);
```
How can I resolve this? If you propose a fix, please make it concise.
src/agents/tool-display-common.ts
Outdated
| const subcommands = new Set([ | ||
| "status", | ||
| "diff", | ||
| "log", | ||
| "show", | ||
| "branch", | ||
| "checkout", | ||
| "switch", | ||
| "commit", | ||
| "pull", | ||
| "push", | ||
| "fetch", | ||
| "merge", | ||
| "rebase", | ||
| "add", | ||
| "restore", | ||
| "reset", | ||
| "stash", | ||
| ]); | ||
| const globalWithValue = new Set([ |
There was a problem hiding this comment.
Unused subcommands set is dead code
The subcommands Set is declared but never referenced. The map Record on line 612 duplicates the same subcommand list for the human-readable mapping and is the one actually used. Consider removing subcommands to avoid confusion.
| const subcommands = new Set([ | |
| "status", | |
| "diff", | |
| "log", | |
| "show", | |
| "branch", | |
| "checkout", | |
| "switch", | |
| "commit", | |
| "pull", | |
| "push", | |
| "fetch", | |
| "merge", | |
| "rebase", | |
| "add", | |
| "restore", | |
| "reset", | |
| "stash", | |
| ]); | |
| const globalWithValue = new Set([ | |
| const globalWithValue = new Set([ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/tool-display-common.ts
Line: 553:572
Comment:
**Unused `subcommands` set is dead code**
The `subcommands` Set is declared but never referenced. The `map` Record on line 612 duplicates the same subcommand list for the human-readable mapping and is the one actually used. Consider removing `subcommands` to avoid confusion.
```suggestion
const globalWithValue = new Set([
```
How can I resolve this? If you propose a fix, please make it concise.
src/agents/tool-display.ts
Outdated
| if (!detail && key === "web_search" && params.args && typeof params.args === "object") { | ||
| const record = params.args as Record<string, unknown>; | ||
| const query = typeof record.query === "string" ? record.query.trim() : undefined; | ||
| const count = | ||
| typeof record.count === "number" && Number.isFinite(record.count) && record.count > 0 | ||
| ? Math.floor(record.count) | ||
| : undefined; | ||
| if (query) { | ||
| detail = count !== undefined ? `for "${query}" (top ${count})` : `for "${query}"`; | ||
| } | ||
| } | ||
|
|
||
| if (!detail && key === "web_fetch" && params.args && typeof params.args === "object") { | ||
| const record = params.args as Record<string, unknown>; | ||
| const url = typeof record.url === "string" ? record.url.trim() : undefined; | ||
| const mode = | ||
| typeof record.extractMode === "string" ? record.extractMode.trim() : undefined; | ||
| const maxChars = | ||
| typeof record.maxChars === "number" && Number.isFinite(record.maxChars) && record.maxChars > 0 | ||
| ? Math.floor(record.maxChars) | ||
| : undefined; | ||
| if (url) { | ||
| const suffix = [ | ||
| mode ? `mode ${mode}` : undefined, | ||
| maxChars !== undefined ? `max ${maxChars} chars` : undefined, | ||
| ] | ||
| .filter((value): value is string => Boolean(value)) | ||
| .join(", "); | ||
| detail = suffix ? `from ${url} (${suffix})` : `from ${url}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
web_search/web_fetch detail logic duplicated across files
These blocks (lines 95–125) are character-for-character identical to the same blocks in ui/src/ui/tool-display.ts (lines 95–125). The other tool-specific detail resolvers (resolveReadDetail, resolveWriteDetail, resolveExecDetail) were correctly extracted into tool-display-common.ts. Consider extracting resolveWebSearchDetail and resolveWebFetchDetail into the common module to follow the same pattern and prevent future drift between the two copies.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/tool-display.ts
Line: 95:125
Comment:
**`web_search`/`web_fetch` detail logic duplicated across files**
These blocks (lines 95–125) are character-for-character identical to the same blocks in `ui/src/ui/tool-display.ts` (lines 95–125). The other tool-specific detail resolvers (`resolveReadDetail`, `resolveWriteDetail`, `resolveExecDetail`) were correctly extracted into `tool-display-common.ts`. Consider extracting `resolveWebSearchDetail` and `resolveWebFetchDetail` into the common module to follow the same pattern and prevent future drift between the two copies.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.- extract web_search/web_fetch detail resolvers into common module\n- fix node -c classification so file path remains positional\n- remove dead git subcommands set\n- keep exec summary refinements (heredoc/node check/git -C/preamble strip)\n- make tests cover node -c syntax-check path\n- run format:check, tsgo, lint, and focused e2e tests
|
Follow-up pushed in efa76d4 to address CI + review feedback:\n\n- fixed node -c handling in exec summarization (syntax-check path now remains positional)\n- removed dead git subcommands set\n- extracted duplicated web_search/web_fetch detail logic into tool-display-common\n- verified locally: format:check, tsgo, lint, and tool-display e2e test all pass\n\nThanks for the review. |
2 participants
Summary
Improve verbose tool-call readability by switching to intent-first detail text in both runtime/TUI and Control UI tool cards.
What changed
read/write/edit/attach) now render human-readable phrases:lines X-Y from ...,first N lines of ...,to ... (N chars),in ...,from ...path,file_path, andfilePathaliasesweb_search:for "<query>" (top N)web_fetch:from <url> (mode <extractMode>, max <maxChars> chars)execsummarization heuristics:bash/sh/zsh/fish -c/-lc/-ic)set/export/unset)A -> B (+N steps))git,grep/rg,find,head/tail,sed,cp/mv,node/python, etc.)python3 <<PY,node <<NODE) andnode --checkFiles
src/agents/tool-display-common.tssrc/agents/tool-display.tsui/src/ui/tool-display.tssrc/agents/tool-display.e2e.test.tsValidation
Ran:
Result: all tests in
tool-display.e2e.test.tspassed.Greptile Summary
This PR improves tool-call display readability by adding intent-first detail text for file tools (
read/write/edit/attach), web tools (web_search/web_fetch), and a comprehensiveexeccommand summarization engine. The exec summarizer handles shell wrapper unwrapping, preamble stripping, pipeline detection, and recognizes ~20 common CLI tools (git, grep, find, node, python, etc.).node -c file.js(short flag for--check) is misidentified because-cis listed as a value-consuming option infirstPositional. The file argument gets swallowed, producing"run node"instead of"check js syntax for file.js". The test only covers--check(long flag), masking this issue.subcommandsSet in the git handler (line 553) is declared but never used — themapRecord duplicates the same list and is the one actually referenced.web_searchandweb_fetchdetail resolution is copy-pasted identically into bothsrc/agents/tool-display.tsandui/src/ui/tool-display.tsrather than being extracted intotool-display-common.tsalongside the other resolvers (resolveReadDetail,resolveWriteDetail,resolveExecDetail).Confidence Score: 3/5
Last reviewed commit: 973c7e7
(2/5) Greptile learns from your feedback when you react with thumbs up/down!