shell: accept command as alias for cmd and improve empty-arg error#2481
shell: accept command as alias for cmd and improve empty-arg error#2481dgageot merged 2 commits intodocker:mainfrom
command as alias for cmd and improve empty-arg error#2481Conversation
Some models (notably Claude, biased by Anthropic's built-in bash tool and
other ecosystems using `command`) occasionally emit {"command": "..."}
instead of the schema's canonical {"cmd": "..."}. The tool then
rejected the call with "Error: empty command", wasting a turn before
the model retried with the right key.
This change:
- Adds a custom UnmarshalJSON on RunShellArgs and RunShellBackgroundArgs
that accepts both "cmd" (canonical) and "command" (alias). "cmd"
wins when both are present. The advertised JSON schema is unchanged.
- Rewrites the empty-command error to name the expected parameter so the
model can self-correct without guessing.
- Also validates params.Cmd in RunShellBackground for parity with RunShell.
- Adds unit tests covering the alias path (both struct-level and
end-to-end through the handler) and the actionable error message.
Refs docker#2478
|
/review |
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Note: Verification step returned an inconclusive response. The drafter's unverified findings are surfaced below for author evaluation.
Summary: The PR correctly handles the cmd/command alias via cmp.Or and improves the empty-arg error. Two medium-severity edge cases were identified in the new unmarshalling logic.
| if err := json.Unmarshal(data, &raw); err != nil { | ||
| return err | ||
| } | ||
| a.Cmd = cmp.Or(raw.Cmd, raw.Command) |
There was a problem hiding this comment.
[MEDIUM] Whitespace-only cmd silently shadows a valid command fallback (unverified)
cmp.Or treats any non-empty string—including whitespace-only strings—as non-zero. If a model emits {"cmd": " ", "command": "real-cmd"}, cmp.Or(raw.Cmd, raw.Command) will return " " (the whitespace cmd), which then fails the strings.TrimSpace guard at line 175 with a misleading error, silently discarding the valid command value.
Consider normalising before selecting:
a.Cmd = cmp.Or(strings.TrimSpace(raw.Cmd), strings.TrimSpace(raw.Command))or using a small helper that skips blank strings.
There was a problem hiding this comment.
Good catch — fixed in 56a1857. Replaced cmp.Or with a small preferNonBlank helper that falls back to the alias when cmd is empty or whitespace-only, and added test cases covering {"cmd":" ","command":"..."} and {"cmd":"","command":"..."}. I kept the chosen value unmodified (no implicit TrimSpace on the winner) to preserve any intentional whitespace inside a legitimate command (e.g. heredoc trailing newlines).
| if err := json.Unmarshal(data, &raw); err != nil { | ||
| return err | ||
| } | ||
| a.Cmd = cmp.Or(raw.Cmd, raw.Command) |
There was a problem hiding this comment.
[MEDIUM] Same whitespace-shadowing issue in RunShellBackgroundArgs.UnmarshalJSON (unverified)
Identical root cause to line 124: cmp.Or(raw.Cmd, raw.Command) picks a whitespace-only cmd string over a valid command value, causing the strings.TrimSpace guard at line 255 to fire an error instead of using the alias.
The fix is the same as suggested for line 124 — normalise inputs to cmp.Or with strings.TrimSpace.
There was a problem hiding this comment.
Fixed together with the RunShellArgs case in 56a1857 — both UnmarshalJSON methods now go through the shared preferNonBlank helper, and there is a dedicated background-args test for {"cmd":" ","command":"sleep 1"}.
cmp.Or treats any non-empty string as non-zero, so a whitespace-only "cmd" would win over a valid "command" alias and trip the empty-arg guard with a misleading error. Select the alias when "cmd" is blank. Addresses PR docker#2481 review comments. Assisted-By: docker-agent
2 participants
Addressed #2478.
Some models (notably Claude, biased by Anthropic's built-in bash tool and other ecosystems that use
command) occasionally emit{"command": "..."}instead of the schema's canonical{"cmd": "..."}. Theshelltool then rejected the call withError: empty command, wasting a turn before the model retried with the right key.This PR supports both cmd and command.