fix(approval): extend gateway-lifecycle guard to launchctl and pidof-based kills - #33084
Closed
briandevans wants to merge 1 commit into
Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Expands dangerous-command detection to close bypasses that can terminate Hermes agents by using pidof substitutions and direct launchctl service control on macOS.
Changes:
- Extend
kill $(...)and backtick expansion detection to includepidofalongsidepgrep. - Add detection for
launchctlsubcommands that can stop/restart/unload Hermes launchd services. - Add unit tests covering the new
pidofandlaunchctldetection behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/approval.py | Adds new dangerous-command regex patterns for pidof expansions and launchctl lifecycle operations. |
| tests/tools/test_approval.py | Adds tests validating detection of the new pidof and launchctl patterns plus non-flagged cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+386
to
+387
| # Catch the operations that stop, restart, or unload it. | ||
| (r'\blaunchctl\s+(stop|kickstart|bootout|unload|kill|disable|remove)\b.*\b(hermes|ai\.hermes)\b', "stop/restart hermes launchd service (kills running agents)"), |
| # the `hermes gateway stop|restart` pattern above by driving launchd | ||
| # directly against the service label (commonly `ai.hermes.gateway`). | ||
| # Catch the operations that stop, restart, or unload it. | ||
| (r'\blaunchctl\s+(stop|kickstart|bootout|unload|kill|disable|remove)\b.*\b(hermes|ai\.hermes)\b', "stop/restart hermes launchd service (kills running agents)"), |
Comment on lines
+833
to
+836
| dangerous, _, desc = detect_dangerous_command(cmd) | ||
| assert dangerous is True | ||
| assert "pidof" in desc.lower() or "pgrep" in desc.lower() | ||
|
|
alt-glitch
added
type/security
briandevans
force-pushed
the
fix/approval-gateway-launchctl-pidof-33071
branch
6 times, most recently
from
June 1, 2026 16:14
5b6864d to
f24a93c
Compare
…based kills The dangerous-command approval layer already blocks `hermes gateway (stop|restart)`, `pkill/killall hermes|gateway`, and `kill ... $(pgrep ...)`. A reporter noted on NousResearch#33071 that the agent can still achieve the same effect by driving launchd directly against the gateway's service label (`launchctl stop ai.hermes.gateway`, `launchctl kickstart -k system/ai.hermes.gateway`, etc.) or by substituting `pidof` for `pgrep` in the kill-expansion form. This widens the "Gateway lifecycle protection" block in `tools/approval.py` to cover both vectors: - `launchctl (stop|kickstart|bootout|unload|kill|disable|remove)` scoped to commands that target a Hermes label (`hermes`, `ai.hermes`). Read-only inspection (`launchctl print …`, `launchctl list`) and operations against unrelated labels remain unflagged. - `kill ... $(pidof …)` and the backtick form, alongside the existing `pgrep` expansion. `pidof` is the BSD/Linux equivalent and is equally opaque to the `(pkill|killall) … hermes` name pattern. Intentionally left out of scope: plain `kill -TERM <numeric_pid>` with a PID looked up out-of-band. Catching that would require runtime PID state and would break the existing `TestPgrepKillExpansion::test_safe_kill_pid_not_flagged` contract, which guarantees that a plain literal-PID `kill 12345` stays safe.
briandevans
force-pushed
the
fix/approval-gateway-launchctl-pidof-33071
branch
from
June 2, 2026 00:13
f24a93c to
9f3db37
Compare
19 tasks
Contributor
|
Merged via #53216 — cherry-picked onto current main with your authorship preserved in git log (commit 3c8d3ec). Thanks @briandevans, and especially for documenting the literal-PID exclusion + rationale in the commit message; that framed the whole review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
The dangerous-command approval layer in
tools/approval.pyalready blockshermes gateway (stop|restart),pkill/killall hermes|gateway, andkill ... $(pgrep ...). Issue #33071 reports a distinct vector confirmed by alt-glitch as not covered by the existing related security PRs (#30882, #22557, #33057, #29159): the agent can still achieve a gateway restart by driving launchd directly against the service label (launchctl stop ai.hermes.gateway,launchctl kickstart -k system/ai.hermes.gateway) or by substitutingpidofforpgrepin the kill-expansion form.This widens the existing "Gateway lifecycle protection" block to cover both vectors with explicit, narrowly-scoped patterns:
launchctl (stop|kickstart|bootout|unload|kill|disable|remove)is flagged only when the target includes a Hermes label (hermesorai.hermes). Read-only inspection (launchctl print …,launchctl list) and operations against unrelated labels stay unflagged — both verified by regression tests.kill ... $(pidof …)and the backtick form are added alongside the existingpgrepexpansion.pidofis the BSD/Linux equivalent and is equally opaque to the(pkill|killall) … hermesname pattern.Mirrors the existing block's precedence: explicit hermes-named target > generic command shape > literal-PID kills stay safe. The literal-PID case (
kill -TERM <numeric_pid>looked up out-of-band) is intentionally still allowed — catching it would require runtime PID state and would break the existingTestPgrepKillExpansion::test_safe_kill_pid_not_flaggedcontract.Related Issue
Fixes #33071
Type of Change
Changes Made
tools/approval.py— extended thepgrepkill-expansion pattern to also matchpidof, and added alaunchctl (stop|kickstart|bootout|unload|kill|disable|remove)pattern scoped to Hermes labels in the existing "Gateway lifecycle protection" block.tests/tools/test_approval.py— addedTestLaunchctlGatewayLifecycle(6 cases: stop/kickstart/bootout/unload all flagged;launchctl printagainst an unrelated label andlaunchctl stop com.example.unrelatedstay safe) and two newTestPgrepKillExpansioncases for$(pidof …)and`pidof …`. Existingtest_safe_kill_pid_not_flaggedpreserved as the explicit safety boundary.How to Test
uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/tools/test_approval.py -v -k "Pgrep or Launchctl"— 13 pass (5 pre-existing + 8 new).uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/tools/test_approval.py— 196 pass.from tools.approval import detect_dangerous_command; print(detect_dangerous_command('launchctl stop ai.hermes.gateway'))→(True, …, 'stop/restart hermes launchd service (kills running agents)').Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) — N/A; existing# Self-termination protectionand# Gateway lifecycle protectioncomments updated inlinecli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Alaunchctlis macOS-only; Linux/systemctlis already covered at line 338 (systemctl … (stop|restart|disable|mask)). Windows service-manager equivalents (sc.exe stop,taskkill /F /IM) are not currently covered by any pattern in this block — could be a follow-up if Windows gateway-as-a-service usage grows.Screenshots / Logs