Small readability fix, raised during review of #224 as a recurrence note after the reviewer reached a wrong conclusion from this code and had to withdraw it.
Problem
ChatRocmCommandAction::Approval is constructed with a short pending_title (for example "Install driver"). Read at that site alone, it looks as though the human approving a privileged command sees only that title.
That is not what happens — the same construction attaches display_command: Some(format_structured_tool_call("rocm", &args)), and the approval body renders the full argv. But the code that consumes it and renders the approval sits several hundred lines away, so following the value takes real effort.
A reviewer did read it that way during #224, filed a blocking finding on the strength of it, and had to withdraw the claim after tracing the value properly. That is a cheap mistake to make and it will be made again.
Suggested fix
One line at the construction site saying the full command line travels with the approval via display_command and is rendered in the approval body. No behaviour change.
Out of scope
The separate, genuine observation from that review still stands and is not covered here: the download URL and the fact that the package is installed as root are not surfaced in the approval body. That is existing behaviour and deserves its own issue if anyone wants to change it.
Small readability fix, raised during review of #224 as a recurrence note after the reviewer reached a wrong conclusion from this code and had to withdraw it.
Problem
ChatRocmCommandAction::Approvalis constructed with a shortpending_title(for example"Install driver"). Read at that site alone, it looks as though the human approving a privileged command sees only that title.That is not what happens — the same construction attaches
display_command: Some(format_structured_tool_call("rocm", &args)), and the approval body renders the full argv. But the code that consumes it and renders the approval sits several hundred lines away, so following the value takes real effort.A reviewer did read it that way during #224, filed a blocking finding on the strength of it, and had to withdraw the claim after tracing the value properly. That is a cheap mistake to make and it will be made again.
Suggested fix
One line at the construction site saying the full command line travels with the approval via
display_commandand is rendered in the approval body. No behaviour change.Out of scope
The separate, genuine observation from that review still stands and is not covered here: the download URL and the fact that the package is installed as root are not surfaced in the approval body. That is existing behaviour and deserves its own issue if anyone wants to change it.