-
Notifications
You must be signed in to change notification settings - Fork 6.6k
Add 'displayInput' to bash tool schema #5321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
/review |
|
/review |
|
idk if that was worth it |
|
should be good to go tmr if i forget just @ me or dm |
|
/review |
|
lgtm |
|
/review |
|
|
||
| case "tool-call": { | ||
| const match = toolcalls[value.toolCallId] | ||
| if (match) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: This line uses as any which the style guide recommends avoiding. Consider adding a proper type for the tool input that includes the optional displayInput property.
| if (match) { | |
| const { displayInput, ...execArgs } = value.input as Record<string, unknown> & { displayInput?: unknown } |
| toolCallId: part.callID, | ||
| input: part.state.input, | ||
| output: part.state.time.compacted ? "[Old tool result content cleared]" : part.state.output, | ||
| toolCallId: part.callID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: There appears to be inconsistent whitespace here - the new lines have an extra leading space compared to the surrounding code. This looks like a formatting issue.
| toolCallId: part.callID, | |
| toolCallId: part.callID, | |
| input: part.state.displayInput ?? part.state.input, | |
| output: part.state.time.compacted ? "[Old tool result content cleared]" : part.state.output, |
| toolCallId: part.callID, | ||
| input: part.state.input, | ||
| errorText: part.state.error, | ||
| toolCallId: part.callID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Same inconsistent whitespace here - extra leading space on the new lines.
| toolCallId: part.callID, | |
| toolCallId: part.callID, | |
| input: part.state.displayInput ?? part.state.input, | |
| errorText: part.state.error, |
| }, | ||
| ) | ||
| const result = await item.execute(args, { | ||
| const displayArgs = args.displayInput ?? args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Using delete on args mutates the original object passed in, which could lead to unexpected side effects. Consider destructuring instead to avoid mutation:
| const displayArgs = args.displayInput ?? args | |
| const { displayInput, ...execArgs } = args | |
| const displayArgs = displayInput ?? args | |
| const result = await item.execute(execArgs, { |
| if (!execute) continue | ||
|
|
||
| // Wrap execute to add plugin hooks and format output | ||
| const schema = (item.inputSchema as any).jsonSchema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: This uses as any which the style guide recommends avoiding. Consider using a more specific type or type guard.
| const schema = (item.inputSchema as any).jsonSchema | |
| const schema = (item.inputSchema as { jsonSchema?: { type?: string; properties?: Record<string, unknown> } }).jsonSchema |
| }, | ||
| { | ||
| args, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Similar to above, using delete mutates the original args object. Since the MCP execute wrapper is invoked with caller-provided args, mutating them could cause issues. Consider using destructuring instead:
| }, | |
| const { displayInput, ...execArgs } = args | |
| const result = await execute(execArgs, opts) |
And update the Plugin.trigger call above to use the original args if needed for the hook.
- Avoid mutating tool arguments by using destructuring instead of delete - Replace 'any' type assertions with more specific types for tool schemas and inputs - Fix inconsistent leading whitespace in MessageV2 tool part mapping
This keeps tool metadata callbacks in sync with the in-memory toolcall state so display inputs can be passed through to completion.
processor.tsnow prefers a metadata-updated input when finishing a tool call, allowing modified displays of commands without changing the actual execution.packages/opencode/src/session/prompt.ts: metadata callback updates the live toolcall entry (input/metadata/title/time/displayInput) before persisting, keeping the processor’s map aligned.packages/opencode/src/session/processor.ts: completion/error handling usesmatch.state.displayInput(from metadata), so the UI shows the plugin-provided display args.Use case:
nix developbash wrapper pluginA plugin can wrap the
bashtool and modify the final displayed command viadisplayInput. This is shown to the user through the UI and also modifies the agent history:Before:
$ nix develop --quiet -c bash -c "echo \"hello\""After:
$ echo "hello"This has many use cases other than just nix, e.g. sandbox-esque wrappers where the full wrapped command is not relevant enough to be constantly showing to the user or agent, and would otherwise clutter history.