feat: surface command context in approval prompts - #22363
Conversation
Add optional purpose/effect/risk context fields to terminal tool calls and thread them through command approval requests. Gateway approval prompts keep the existing request message intact and send a follow-up context message when explanation data is available.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for improving the information available at a security-sensitive approval decision point. The idea remains relevant on current main, but this patch needs a safety-focused salvage.
Problems
gateway/run.py:14258-14281sends model-supplied purpose/effect/risk directly to the adapter. Current main explicitly redacts approval output atgateway/run.py:18503-18509; these new fields need the same forced-redaction boundary before delivery.tools/terminal_tool.py:322-327replaces the consolidated-guard wrapper without current main'shas_host_accesspropagation. Current main passes that value attools/terminal_tool.py:2282-2285to ensure host-mounted Docker commands cannot take the isolated-container approval bypass.
Suggested changes
- Preserve
has_host_accesswhen adding context and adapt the implementation to current main's notifier scheduling path. - Redact context at each approval egress and add delivery-level coverage, including the API/TUI payload paths.
Automated hermes-sweeper review.
| asyncio.run_coroutine_threadsafe( | ||
| _status_adapter.send( | ||
| _status_chat_id, | ||
| followup_msg, |
There was a problem hiding this comment.
This sends model-controlled context directly to the chat adapter. Approval output is a secret-egress boundary on current main (gateway/run.py:18503-18509 redacts the command with forced redaction); redact each context field before constructing this follow-up and cover the delivery path with a credential-shaped fixture.
| return _check_all_guards_impl(command, env_type, | ||
| approval_callback=_get_approval_callback()) | ||
| approval_callback=_get_approval_callback(), | ||
| approval_context=approval_context) |
There was a problem hiding this comment.
Please preserve current main's has_host_access propagation here. Current terminal_tool passes it into the consolidated guard for host-mounted Docker backends; removing it would reintroduce approval bypass for commands reaching bind-mounted host paths.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the missing purpose/effect/risk context in dangerous-command approvals. #22363 adds structured context as a separate follow-up with redaction across gateway egress paths, while #75795 rebuilds the feature for the current approval architecture as one sanitized, redacted, fail-closed approval message.
Related pull requests
- #22363
related— (+280/-8) — superseded implementation: The diff threads purpose/effect/risk through the terminal guard pipeline and adds forced redaction for chat, API, and TUI egress, but delivers gateway context as a separate follow-up message. Despite the keep_open review on #22363, #75795 covers the same user-visible need on the current single-message path and avoids the follow-up ordering and association problem. - #75795
related— (+983/-83) — keep open with a salvage path: The diff co-locates sanitized, credential-redacted model context with the actionable approval prompt and adds fail-closed delivery handling, directly addressing the reported lack of decision context. The keep_open review identifies a blocking sanitizer bypass because LF/CR are removed before line-based forged-command filtering, plus duplicate CLI rendering because both enhanced_desc and explanation contain the context.
Duplicates
#22363 and #75795 substantially overlap at the feature and guard-pipeline level, although #75795 is a current-architecture redesign rather than a line-for-line duplicate; for consolidation, #22363 can be treated as superseded by #75795.
Suggested consolidation
Keep #75795 open with a salvage path: normalize CRLF first, preserve LF while stripping other controls, filter every forged-command line, add embedded-line and CRLF regression coverage, and render purpose/effect/risk only once in the CLI fallback. Then close #22363 as duplicate of #75795 for issue-tracking purposes; this differs from the keep_open review on #22363 because the complete #75795 diff preserves its core context/schema work while replacing the separate follow-up with the current single-message approval design. Do not advance #75795 until its contributor-blocking sanitizer and duplicate-rendering findings are resolved.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 82 kB of PR diffs, 6 kB of issue/PR text, 3 kB of discussion (5 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
Thanks for the cross-PR triage.
For chronology and attribution, #22363 predates #75795, and #75795
explicitly describes itself as a rebuild of #22363. The follow-up commit in
#22363 (fe43d02) addressed the earlier review by preserving has_host_access
and adding forced redaction across the chat, API, and TUI approval egress
paths.
I agree that the remaining architectural difference is separate follow-up
delivery versus a single actionable approval message. In #22363, the
follow-up is sent only after the approval-message delivery completes, so
the send order is serialized, although I understand that two separate
messages still cannot provide atomic delivery or association.
Before closing #22363, could a maintainer confirm the preferred
consolidation path?
1.
Adapt #22363 to the current single-message approval path; or
2.
Continue with #75795 after its blocking sanitizer issue is resolved.
If #75795 is preferred, please retain clear attribution to the schema and
approval-context pipeline work originating in #22363.
…On Sat, Aug 1, 2026 at 12:51 PM Jan-Stefan Janetzky < ***@***.***> wrote:
***@***.**** commented on this pull request.
*This was generated by AI during triage.*
Summary
Two PRs address the missing purpose/effect/risk context in
dangerous-command approvals. #22363
<#22363> adds structured
context as a separate follow-up with redaction across gateway egress paths,
while #75795 <#75795>
rebuilds the feature for the current approval architecture as one
sanitized, redacted, fail-closed approval message.
Related pull requests
- #22363 <#22363>
related — (+280/-8) — superseded implementation: The diff threads
purpose/effect/risk through the terminal guard pipeline and adds forced
redaction for chat, API, and TUI egress, but delivers gateway context as a
separate follow-up message. Despite the keep_open review on #22363
<#22363>, #75795
<#75795> covers the
same user-visible need on the current single-message path and avoids the
follow-up ordering and association problem.
- #75795 <#75795>
related — (+983/-83) — keep open with a salvage path: The diff
co-locates sanitized, credential-redacted model context with the actionable
approval prompt and adds fail-closed delivery handling, directly addressing
the reported lack of decision context. The keep_open review identifies a
blocking sanitizer bypass because LF/CR are removed before line-based
forged-command filtering, plus duplicate CLI rendering because both
enhanced_desc and explanation contain the context.
Duplicates
#22363 <#22363> and
#75795 <#75795>
substantially overlap at the feature and guard-pipeline level, although
#75795 <#75795> is a
current-architecture redesign rather than a line-for-line duplicate; for
consolidation, #22363
<#22363> can be treated
as superseded by #75795
<#75795>.
Suggested consolidation
Keep #75795 <#75795>
open with a salvage path: normalize CRLF first, preserve LF while stripping
other controls, filter every forged-command line, add embedded-line and
CRLF regression coverage, and render purpose/effect/risk only once in the
CLI fallback. Then close #22363
<#22363> as duplicate of
#75795 <#75795> for
issue-tracking purposes; this differs from the keep_open review on #22363
<#22363> because the
complete #75795 <#75795>
diff preserves its core context/schema work while replacing the separate
follow-up with the current single-message approval design. Do not advance
#75795 <#75795> until
its contributor-blocking sanitizer and duplicate-rendering findings are
resolved.
*Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex.
Each diff was read against this issue; Assessment working set: 82 kB of PR
diffs, 6 kB of issue/PR text, 3 kB of discussion (5 comments), 0 verify
verdicts. verdicts reflect diff content, not PR titles. Part of an
automated triage batch.*
—
Reply to this email directly, view it on GitHub
<#22363?email_source=notifications&email_token=AUEHTDKC5V5H7NEIDHAYLJ35HVZMPA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBTGM3TANBQGAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4833704001>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AUEHTDNWYHADBIQF6QUEGUT5HVZMPAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMRUGU2TIMRWG45US43TOVSTWNBUGEYTMMRZGY4DBILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AUEHTDNXG4R4V36LKR7CIG35HVZMPA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBTGM3TANBQGAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/AUEHTDODH2TOY74UC545ASD5HVZMPA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBTGM3TANBQGAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Summary
This PR surfaces optional command context after dangerous-command approval requests.
The existing approval
Reasonexplains why Hermes flagged a command as requiring approval, e.g.delete in root path. This change adds optional user-facing context that explains:When this context is available, the gateway sends it as a follow-up message after the existing approval request:
Motivation
Dangerous-command approvals are a security-critical UX path, especially in messaging gateways where users may approve commands from a mobile chat UI. The raw command and detector reason are useful, but they do not always explain what approving the command means for the current task.
This keeps the existing approval prompt and approval flow intact while adding structured decision context when the model provides it.
Changes
approval_purposeapproval_effectapproval_riskterminal_toolintocheck_all_command_guards(...).Reasonfield.Notes
This PR does not auto-generate explanations with another model. It only surfaces context supplied by the tool call, avoiding extra latency, cost, and privacy considerations in the approval path.
Test Plan
python3 -m py_compile gateway/run.py tools/approval.py tools/terminal_tool.py tests/tools/test_command_guards.pypython3 -m pytest tests/tools/test_command_guards.py -q -o 'addopts='Local result: