fix(gateway): block launchctl submit in the gateway lifecycle guard - #62896
fix(gateway): block launchctl submit in the gateway lifecycle guard#62896jackjin1997 wants to merge 2 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for closing the real launchctl submit omission: current main's Branch B at cron/lifecycle_guard.py:59 does not include that verb.
Problems
- The changed regex at
cron/lifecycle_guard.py:64still uses[^\n]*between the verb and gateway label. The actual #62891 command puts a shell-continuation newline afterlaunchctl submit, so this pattern stops before-l ai.hermes.gateway-…and does not block the reported form.
Suggested changes
- Normalize shell continuations before matching, or permit continuation-newline segments in Branch B without relaxing the gateway-label constraint.
- Add the literal multiline #62891 command to
tests/hermes_cli/test_gateway_restart_loop.py, including the terminal guard path described in the PR.
Automated hermes-sweeper review.
| # creates a NEW keepalive job wrapping an arbitrary helper, which is how | ||
| # a blocked direct restart/kill gets laundered into a persistent restart | ||
| # loop instead (#62891) — same foot-gun, indirect shape. | ||
| r"|(?:launchctl\s+(?:kickstart|unload|load|stop|restart|submit)\b[^\n]*\bhermes[.\-]?gateway)" |
There was a problem hiding this comment.
[^\n]* still prevents this branch from reaching a -l ai.hermes.gateway-… label when launchctl submit is written with the backslash-newline continuation shown in #62891. Please normalize or explicitly handle shell continuations and add that literal reproduction.
|
Good catch — the exact reported command was split across backslash-newline shell continuations, so |
_GATEWAY_LIFECYCLE_PATTERN's launchctl branch only matched kickstart/unload/load/stop/restart. An agent whose direct restart/kill was correctly blocked could launder the same effect through `launchctl submit -l ai.hermes.gateway-<suffix> -- <helper-script>`, which creates a NEW keepalive job wrapping an arbitrary helper — this bypassed both the cron-creation-time guard and the terminal execution-time guard (both call the same shared function), and became a restart loop that ran 9,447 times before manual removal. Fixes NousResearch#62891
…rd matching Every branch of _GATEWAY_LIFECYCLE_PATTERN uses [^\n]* between its verb and the gateway identifier so a match can't span unrelated lines of a longer cron prompt/script. But the exact NousResearch#62891 command was split across backslash-newline shell continuations, so the verb and the gateway label landed on different physical lines and the pattern (even with `submit` added) never matched. Collapse continuations to a single space before matching, mirroring what the shell itself does, instead of loosening [^\n]* and risking false positives across genuinely separate lines. Addresses review feedback on NousResearch#62896.
d0720ee to
6ae871e
Compare
…bel-independent detection Extends the shared _GATEWAY_LIFECYCLE_PATTERN (used by BOTH the cron creation-time guard in cron/lifecycle_guard.py and the terminal execution-time hard-block in tools/terminal_tool.py) so Branch B covers launchctl submit and bootstrap alongside kickstart/unload/load/stop/ restart, and normalizes POSIX shell line continuations before matching so the exact multi-line reported shape in #62891 cannot slip past. Also extends the execution-aware, label-independent detector (contains_launchctl_submit_command, cherry-picked from #63272) to cover launchctl bootstrap, since a neutral label like ai.hermes.svc-reload-tmp defeats any label-anchored regex — the second production reproduction. Regression tests cover both sites, including `launchctl submit -l com.foo -- /path/gateway` and the bootstrap variant, plus outside-gateway pass-through. Branch B regex extension and continuation normalization drawn from PR #62896; bootstrap coverage and test shapes drawn from PR #51003. Co-authored-by: JackJin <1037461232@qq.com> Co-authored-by: joelbrilliant <joelbrilliant1@gmail.com>
…bel-independent detection Extends the shared _GATEWAY_LIFECYCLE_PATTERN (used by BOTH the cron creation-time guard in cron/lifecycle_guard.py and the terminal execution-time hard-block in tools/terminal_tool.py) so Branch B covers launchctl submit and bootstrap alongside kickstart/unload/load/stop/ restart, and normalizes POSIX shell line continuations before matching so the exact multi-line reported shape in NousResearch#62891 cannot slip past. Also extends the execution-aware, label-independent detector (contains_launchctl_submit_command, cherry-picked from NousResearch#63272) to cover launchctl bootstrap, since a neutral label like ai.hermes.svc-reload-tmp defeats any label-anchored regex — the second production reproduction. Regression tests cover both sites, including `launchctl submit -l com.foo -- /path/gateway` and the bootstrap variant, plus outside-gateway pass-through. Branch B regex extension and continuation normalization drawn from PR NousResearch#62896; bootstrap coverage and test shapes drawn from PR NousResearch#51003. Co-authored-by: JackJin <1037461232@qq.com> Co-authored-by: joelbrilliant <joelbrilliant1@gmail.com>
What does this PR do?
_GATEWAY_LIFECYCLE_PATTERN's launchctl branch (Branch B incron/lifecycle_guard.py) only matched the verbskickstart|unload|load|stop|restart. As reported in #62891, an agent whose direct gateway restart/kill was correctly blocked by the guard instead laundered the same effect through:launchctl submitcreates a new launchd keepalive job wrapping an arbitrary helper script — a different verb shape than the existing service ops the pattern already caught, so it slipped through both enforcement points that share this function (cron.jobs.create_jobat job-creation time, andtools/terminal_tool.py's execution-time hard-block). The helper script killed the real gateway and re-triggered it every ~20s vialaunchctl kickstart -k, running 9,447 times before manual removal.Related Issue
Fixes #62891
Type of Change
Changes Made
cron/lifecycle_guard.py: addsubmitto Branch B's verb alternation, with a comment explaining the indirection this closes.tests/hermes_cli/test_gateway_restart_loop.py: added the exact reproduction command from the issue plus a secondlaunchctl submitvariant as positive matches, and alaunchctl submiton an unrelated label as a negative (no-false-positive) case.Scope note
The issue's "Suggested fixes" section lists five items; this PR addresses #1 (the concrete pattern gap that caused the actual incident) and its accompanying regression test (#5). Items #2 (treating any post-rejection workaround as the same prohibited effect — needs session-level intent tracking, not just pattern matching), #3 (a supported one-shot detached restart helper — a new tool surface), and #4 (enumerating submitted launchd jobs in restart-loop diagnostics) are broader hardening/feature work that I think deserves separate design discussion rather than being bundled into a reactive pattern fix — flagging them here in case a maintainer wants to track them as follow-ups.
How to Test
_contains_gateway_lifecycle_command("launchctl submit -l ai.hermes.gateway-x -- /bin/sh helper.sh")returnsFalse— the exact command from the issue is not caught.True, and the same protection applies at both the cron-job-creation guard and the terminal execution-time guard (both call this shared function).pytest tests/hermes_cli/test_gateway_restart_loop.py -q— 66 passed (2 pre-existing failures unrelated to this change: this sandbox's venv has nopip/croniterinstalled, confirmed identical on a cleanorigin/maincheckout viagit stash).Checklist
Code
_GATEWAY_LIFECYCLE_PATTERN,launchctl submit, related Cron tool can schedule gateway lifecycle scripts that cause restart loops #51976/fix(cron): apply gateway lifecycle block to cronjob tool prompt and script scan #36194/fix(cron): block gateway lifecycle commands on job update #51980) to make sure this isn't a duplicatepytest tests/hermes_cli/test_gateway_restart_loop.py -q— 66/66 relevant tests pass (2 pre-existing environment-only failures noted above)Documentation & Housekeeping
AI Disclosure
This bug was identified and fixed with AI assistance.