feat(approval): block agents from killing their own gateway/host process

[OmarB97]

Why

An agent session runs inside a host process — the desktop dashboard/gateway, or the CLI itself. Nothing stops a terminal() call from killing that host's PID. Reproduced on 2026-06-09: a session cleared "stale Python bytecode" by killing its own dashboard process; the turn died mid-flight, the session was orphaned (blank indicator, stop failed, session-not-found on the next prompt) and in-progress work was lost. A static blocklist pattern can't express "our own PID", so this adds a function guard alongside the existing sudo-stdin one.

What changed

• tools/approval.py: _check_self_host_kill extracts numeric kill targets plus the $$/$PPID shell self-tokens and blocks when they match os.getpid()/os.getppid(). Runs in check_all_command_guards in the unconditional section (with hardline + sudo-stdin), so --yolo, approvals.mode=off, and cron approve mode cannot bypass it. Containerized backends keep their existing early bypass — PIDs in a container namespace are not the host's.
• Block message points at supervisor-driven restarts (hermes gateway restart, the desktop's Gateway menu) instead of in-session kills.
• Review follow-up (2f05e2b): kill is anchored at command position via the shared _CMDPOS fragment, whose wrapper inventory now includes command [-p] and builtin — so wrapper spellings that still execute kill (command kill, builtin kill, sudo/env/exec/nohup/setsid/time prefixes, and chains of those) hit the guard. command -v/-V stays unmatched (resolves a name without executing). The _CMDPOS extension hardens the shutdown/reboot hardline patterns the same way.
• Out of scope by design: pkill name matching and negative-pgid forms (kill -1 is already on the hardline list).
• tests/tools/test_hardline_blocklist.py: 15 new cases (own/parent PID, $$/$PPID, chained commands, foreign-PID allow, pkill/pgid allow, end-to-end hardline shape, yolo no-bypass, container bypass).

Verification

tests/tools/test_hardline_blocklist.py — 115 passed (100 pre-existing + 15 new); full approval cluster (6 files) 370 passed on this branch.

Notes

Mirror of OmarB97#128 (merged on the fork after live reproduction). Cherry-picks cleanly onto upstream main; no other files touched.

[liuhao1024]

Code review: clean — well-designed guard

This is a solid security hardening addition. A few observations:

• Guard placement is correct: positioned after detect_hardline_command but before --yolo bypass, so it can't be overridden by session-level settings. The comment explicitly documents this rationale.
• Regex design: _KILL_CMD_RE correctly handles command separators (;, &&, ||, |, backtick, $() and the args capture group avoids signal flags. _KILL_SELF_TOKEN_RE anchors $$ and $PPID with word boundaries to avoid false positives.
• Container bypass is correct: container PIDs are in a separate namespace, so os.getpid() inside the container never matches the host's PID.
• Test coverage is thorough: own PID, parent PID, shell tokens, chained commands, foreign PIDs, pkill/pgid exclusions, yolo bypass attempt, container bypass — all critical paths covered.

No issues found. LGTM.

[egilewski]

Request changes. The new guard only recognizes kill when it is the immediate command at the start of a command segment, so shell wrappers that still execute kill can bypass it. On PR head a08fe96a5cfd70200eeca4ec01e9357ba1e52cfd, these both return approved:

import os
from tools.approval import _check_self_host_kill, check_all_command_guards
for cmd in [f"command kill {os.getpid()}", f"builtin kill {os.getpid()}"]:
    print(cmd, _check_self_host_kill(cmd), check_all_command_guards(cmd, "local"))

Observed result: _check_self_host_kill(...) == (False, None) and check_all_command_guards(...) == {"approved": True, "message": None} for both forms. command kill <pid> and builtin kill <pid> are valid shell ways to invoke the kill builtin/executable, so an agent can still kill its own host PID without approval. This leaves the core self-host kill bypass open despite the new hardline guard in tools/approval.py around _KILL_CMD_RE.

Validation I ran:

• git merge-tree --write-tree upstream/main refs/remotes/upstream/pr/43157 => f799ad298c61f4e64870ce67a40a515642a7ad5c
• git diff --check upstream/main...refs/remotes/upstream/pr/43157 passed
• pytest -q tests/tools/test_hardline_blocklist.py -p no:cacheprovider => 110 passed
• direct bypass probe above reproduced the remaining approval bypass

Signed: GPT-5.5-xhigh in Codex

[OmarB97]

Addressed in 2f05e2b9cd762a1423f0a38d6ff688b96a7e7672 (re: review 4472077377).

Fix. _KILL_CMD_RE no longer carries its own segment-start alternation; it anchors kill at command position via the shared _CMDPOS fragment, and _CMDPOS's wrapper inventory gains command [-p] and builtin. Wrapper spellings that still execute kill — command kill, builtin kill, sudo/env/exec/nohup/setsid/time prefixes, and chains of those (command builtin kill, nohup setsid kill) — now reach the guard. command -v/-V intentionally stays unmatched: it resolves a name without executing it. Because _CMDPOS is shared, the shutdown/reboot hardline patterns pick up the same wrapper coverage (command shutdown -h now was the same bypass class).

Your probe on the new head:

command kill <own-pid> (True, "kill targets this agent's own host process (<own-pid>)") {'approved': False, 'hardline': True}
builtin kill <own-pid> (True, "kill targets this agent's own host process (<own-pid>)") {'approved': False, 'hardline': True}

Tests. 5 new cases in tests/tools/test_hardline_blocklist.py: both reported probes end-to-end through check_all_command_guards, command -p kill $$, wrapper chains, foreign-PID allow via wrappers (command kill 999999999 stays approved), and command -v kill precision.

Validation:

• pytest -q tests/tools/test_hardline_blocklist.py -p no:cacheprovider => 115 passed
• approval cluster (test_approval.py, test_approval_heartbeat.py, test_approval_plugin_hooks.py, test_cron_approval_mode.py, test_execute_code_approval_cluster.py, test_hardline_blocklist.py) => 370 passed
• fork-side mirror of this follow-up: fix(approval): recognize wrapper-prefixed kill spellings in self-host guard OmarB97/hermes-agent#165

[egilewski]

Recommendation: request changes

I reviewed this against current GitHub main fa7f24e8980367c2ca849eb99e1eb2331c7d3699, PR base 72154ad879e2acebbbf46e27f77a3a4bde3f2ea2, and PR head 2f05e2b9cd762a1423f0a38d6ff688b96a7e7672.

Validation:

• git merge-tree --write-tree upstream/main 2f05e2b9cd762a1423f0a38d6ff688b96a7e7672: passed, produced tree 96d2c613c756f9034f414113c7bf05478a59cfdf.
• git diff --check upstream/main...2f05e2b9cd762a1423f0a38d6ff688b96a7e7672: passed.
• Direct guard probe on PR head: _check_self_host_kill() and check_all_command_guards(..., "local") still approve command -- kill <pid>, builtin -- kill <pid>, and env -- kill <pid>.

Finding:
The self-host kill guard still misses valid wrapper spellings that pass -- before the command name. command -- kill <pid>, builtin -- kill <pid>, and env -- kill <pid> all execute kill in normal shell usage, but the current _CMDPOS wrapper fragment only consumes command, command -p, builtin, or env assignments before looking for kill. As a result, the probe returned (False, None) from _check_self_host_kill() and {"approved": True, "message": None} from check_all_command_guards() for those self-PID forms, leaving the self-host kill bypass open.

Please cover the -- wrapper variants in the detector and add regression tests for them.

Signed: GPT-5.5-xhigh in Codex

[OmarB97]

Addressed the wrapper -- bypass in c19daf3f8.

Changes:

• _CMDPOS now consumes wrapper argument separators, so command -- kill <pid>, builtin -- kill <pid>, env -- kill <pid>, sudo -- kill <pid>, and chained wrapper forms route through the self-host kill detector.
• Added helper-level and check_all_command_guards regression coverage for the exact review probes.
• Preserved the command -v/-V behavior: lookup forms still do not execute the operand and are not treated as wrappers.

Verification:

• python3 -m pytest tests/tools/test_hardline_blocklist.py -q -> 115 passed
• Direct probe: command -- kill <self pid>, builtin -- kill <self pid>, and env -- kill <self pid> now return approved: false, hardline: true
• python3 -m py_compile tools/approval.py
• git diff --check

[egilewski]

Recommendation: request changes

I reviewed this against current GitHub main d62979a6f34f64f2ed840f159aac66e24d7cad78, PR base 72154ad879e2acebbbf46e27f77a3a4bde3f2ea2, and PR head c19daf3f85b7417947db2bdae1980487f77d7b56.

Validation:

• git merge-tree --write-tree upstream/main upstream/pr/43157: passed, produced fc8453ebaa847da553fe9fb1ee0d1da9b6c20387.
• git diff --check upstream/main...upstream/pr/43157: passed.
• /home/mac/hermes-agent/.venv/bin/python -B -m pytest -o addopts='' -p no:cacheprovider tests/tools/test_hardline_blocklist.py -q: failed test_self_host_kill_blocks_parent_pid in this review environment because os.getppid() is 1, while _self_host_pids() intentionally excludes PID 1.
• Direct probe: command -- kill <pid>, builtin -- kill <pid>, env -- kill <pid>, sudo -- kill <pid>, and command -- builtin -- kill <pid> now block.
• Direct probe: bash -c "kill <pid>" and sh -c "kill <pid>" still return {'approved': True, 'message': None} from check_all_command_guards(..., 'local').

Finding:
The wrapper fixes close the earlier -- variants, but the self-host kill guard still misses common shell-interpreter forms that execute the same kill <host-pid> command. A terminal command such as bash -c "kill <pid>" or sh -c "kill <pid>" is still approved even when <pid> is the current agent host process, so the self-host kill bypass remains reachable through a standard shell wrapper.

Please cover shell -c kill forms, or otherwise route them into the hardline/self-host guard, and make the parent-PID regression test robust when the pytest process parent is PID 1.

Signed: GPT-5.5-xhigh in Codex

[egilewski]

suggesting changes

I reviewed a run-owned patch replay of this PR onto current GitHub main 0800af0b8ae01fd808e54be53d2cf12eca1d0638 because the submitted branch currently has a content conflict in tools/approval.py; that setup validates the focused change against current main, but it does not prove the submitted branch itself merges cleanly. The replay still fails the focused hardline blocklist test suite:

PYTHONDONTWRITEBYTECODE=1 python -B -m pytest -q tests/tools/test_hardline_blocklist.py -p no:cacheprovider

Result: test_self_host_kill_blocks_parent_pid fails with assert False is True when the pytest process parent is PID 1. The implementation intentionally excludes PID 1 in _self_host_pids(), so the regression test still assumes every test runner has a meaningful non-1 parent PID. Please make that test robust for PID-1 parent environments while preserving the intended parent-process coverage.

Security evidence:

• trust boundary: terminal() command text is untrusted agent output that reaches tools.approval.check_all_command_guards().
• source/sink/invariant: self-host kill targets must be blocked before yolo/mode-off approval bypasses; the blocker here is the PR's own parent-PID regression coverage not matching the implemented PID-1 exclusion.
• current-main reproduction: on current main, direct probes imported the baseline tools/approval.py and approved kill <pid>, command -- kill <pid>, env -- kill <pid>, kill $$, and kill -TERM $PPID.
• PR-head or patch-replay validation: on the replay tree, direct probes imported the replayed tools/approval.py and those command-position probes block, but the focused pytest command above fails.
• positive/negative cases: wrapper self-PID and $$/$PPID cases block in the replay; command -v kill <pid>, foreign PIDs, pkill, and negative process-group forms remain allowed as expected.
• residual bypass search: bash -c "kill <pid>" remains approved in the replay, but command-carrying shell wrappers are a different parser class from the command-position wrapper prefixes changed here and are not the blocker for this comment.
• reviewer validation: CodeRabbit was skipped because this till first blocker security review found a decisive local focused-test failure before the clean-pass tooling stage.

Signed: GPT-5.5-xhigh in Codex

[OmarB97]

Addressed both open review points and refreshed the branch onto current main. The head was 118 commits behind (which is why it read CONFLICTING); it's MERGEABLE now, and the diff is just the three files below.

1. PID-1-robust parent test (this comment)

test_self_host_kill_blocks_parent_pid no longer assumes the runner has a meaningful non-1 parent. It pins a deterministic non-1 parent via monkeypatch.setattr(os, "getppid", …) and asserts the guard blocks it — so it holds in PID-1-parent environments (containers / CI) where os.getppid() is 1 and _self_host_pids() intentionally excludes PID 1. A companion test_self_host_kill_pid1_parent_is_excluded pins that exclusion (own-PID still blocks; kill -9 1 does not) so the coverage can't be "fixed" by dropping it.

2. bash -c / sh -c self-host bypass (from the CHANGES_REQUESTED review)

_check_self_host_kill now pulls shell -c payloads out with a POSIX tokenizer and re-scans them as standalone commands (recursively, for nested bash -c "sh -c '…'"). Covers path-qualified interpreters, bundled flags (-lc), and wrapper-nested forms. Now blocking:

bash -c "kill $$"            -> block
sh -c 'kill -9 $PPID'        -> block
sudo bash -c "kill $$"       -> block
bash -lc "kill $$"           -> block
/bin/sh -c 'kill $$'         -> block
bash -c "sh -c 'kill $$'"    -> block   (nested)

Still allowed, scope unchanged: bash -c "kill 999999999" (foreign PID), bash -c "echo kill it with fire", command -v kill <pid>, pkill, kill -- -12345. The wrapper anchor is a dedicated _KILL_CMDPOS (not the shared _CMDPOS) so widening the kill detector doesn't change the rm/shutdown floor's blast radius.

3. Bundled: systemic Run tests slice timeout flake

The slice 6/8 timeout in the CI you referenced isn't specific to this change — test_wait_for_process_kills_subprocess_on_keyboardinterrupt (on main) bounds its internal waits at join(15s) + _wait_for_pgid_exit(30s) ≈ up to 50s, against the suite's --timeout=30 --timeout-method=thread. On expiry the thread method hard-os._exit()s the per-file interpreter, so a slow reap under xdist load crashes the whole file ("1 file where no tests ran") instead of failing one assertion — and every PR inherits it. The real reap chain is ≤~3.2s, so I capped the internal budget (join→8s, _wait_for_pgid_exit→8s; total setup+settle+join+wait ≈ 21s < 30s) and added a 0.3s settle so the interrupt is injected only once the worker is parked in the poll loop. Product code (base.py/local.py reap path) is unchanged.

Verification (local, current-main base)

• pytest tests/tools/test_hardline_blocklist.py → 203 passed (incl. new self-host, shell--c, and PID-1 tests)
• pytest tests/tools/test_local_interrupt_cleanup.py → 3 passed in ~0.9s, 3/3 stable (was the shard-timeout culprit)
• No regressions across the approval suite: test_approval (298), test_command_guards (31), test_shell_bypass_denylist (38), test_yolo_mode (19), test_gnu_long_option_abbreviation_bypass (14)
• ruff check clean; ty adds no new diagnostics; CI ruff enforcement (blocking) already green on this push

Direct probe reproducing the guard on this head (self-PID / $$ / $PPID, wrapper, and shell--c forms all block; foreign PIDs and command -v stay allowed) is in the added regression tests.

[teknium1]

Thanks for addressing the earlier wrapper and PID-1 feedback. The numeric self-host-PID premise is still valid on current main: the hardline floor only recognizes kill ... -1 (tools/approval.py:395), and normal commands reach the later yolo path.

Problems

• tools/approval.py:603 marks any $$ as a host kill. Hermes executes local commands through eval (tools/environments/base.py:510) in a spawned bash -c (tools/environments/local.py:1044), so $$ identifies the terminal shell rather than the Python host. The tests at tests/tools/test_hardline_blocklist.py:699-701 assert this false-positive behavior.
• _extract_shell_c_scripts() scans all flat tokens (tools/approval.py:554-575) rather than shell command positions. Thus command data such as echo sh -c 'kill $PPID' is recursively treated as code and hard-blocked.

Suggested changes

• Retain numeric host-PID matching, but make shell-variable handling expansion-layer-aware and add direct/nested quoting regressions.
• Restrict -c extraction to actual executable command positions and add data/prose allow cases.

Automated hermes-sweeper review.

[teknium1]

$$ is not the agent host PID in Hermes's local execution path: the command is evaluated inside the spawned bash -c shell. Blocking every $$ therefore rejects kill $$, which only terminates that terminal shell. Resolve shell variables at their actual execution layer, or do not use $$ as a host-PID signal.

[teknium1]

This scans any token named sh/bash as an interpreter, including command arguments. echo sh -c 'kill $PPID' is parsed as a nested shell payload and becomes an unconditional block even though it only prints data. Restrict extraction to executable command positions and add an allow regression.

[egilewski]

suggesting changes

The PR blocks direct and several wrapped spellings of kill aimed at the agent process or its parent, but its new unconditional guard is bypassed by the standard path-qualified executable spellings /bin/kill and /usr/bin/kill. On the reviewed head, check_all_command_guards("/bin/kill <current-pid>", "local") returns approved while the same call with bare kill is blocked. Because the local backend passes approved commands to Bash and both path-qualified executables are installed in the reviewed environment, an agent can still terminate the protected host process without yolo or approval. The guard should recognize path-qualified kill command words and add regression cases across direct, wrapper, and shell--c forms before merge.

• [P2] Path-qualified kill executables bypass the self-host guard (tools/approval.py:570)
  The new _KILL_CMD_RE requires the executable token to begin exactly with kill. It therefore misses ordinary executable paths. A focused PR-head probe using the live process PID showed that bare kill <pid> returns a hardline denial, but /bin/kill <pid>, /usr/bin/kill -9 <pid>, env /bin/kill <pid>, and bash -c "/bin/kill <pid>" all return approved. /bin/kill and /usr/bin/kill are installed executable aliases in this checkout's host environment, and the local terminal backend executes approved strings through Bash. Thus the same untrusted terminal command can reach the process-signal sink and terminate the PID that this feature claims to protect simply by qualifying the executable path.
  Remediation: Recognize a path-qualified command word whose basename is kill (at minimum /bin/kill and /usr/bin/kill) in the same command positions, wrappers, deobfuscation variants, and extracted shell--c payloads. Add check_all_command_guards regression tests proving these spellings are hardline-blocked for own/parent PIDs while path-qualified kills of foreign PIDs remain allowed.

Security evidence:

• trust boundary: The source is the model-controlled command argument accepted by terminal_tool. For local, SSH, or container configurations with host access, that string crosses check_all_command_guards before reaching the environment's Bash/subprocess execution path, which is the process-signal sink. The new validator is intended to fail before yolo, approvals.mode=off, and cron approval bypasses; isolated container backends intentionally return before it because their PID namespace is separate.
• source/sink/invariant: The claimed invariant is that a kill command targeting os.getpid(), a meaningful os.getppid(), $$, or $PPID cannot be approved in a host-accessing environment, while foreign numeric PIDs and isolated container commands retain existing behavior. _self_host_pids supplies the protected numeric set, _check_self_host_kill extracts targets, and check_all_command_guards converts a match to a hardline denial before configurable bypasses. The executable recognizer at line 570 is the failed validator: it accepts only a bare kill token.
• current-main reproduction: I executed tools/approval.py directly from current-main SHA cff9728 and called its check_all_command_guards with kill <live os.getpid()> in the local environment. It returned {"approved": true, "message": null}, reproducing the original absence of a self-host-specific guard.
• PR-head or patch-replay validation: The leased checkout was exactly reviewed head ec1bf35. A read-only three-tree merge against current main and merge base 41a07f5 reported all three changed files as merged without conflict. On PR head, bare kill <live pid> was hardline-blocked, but /bin/kill <same pid> and the other path-qualified variants were approved, so the coherent current-main replay retains the bypass.
• positive/negative cases: Positive protection case: bare kill <live os.getpid()> was denied with hardline: true. Negative compatibility case: kill 999999999 was not classified as self-host kill. Adversarial negative cases: /bin/kill <live pid>, /usr/bin/kill -9 <live pid>, env /bin/kill <live pid>, command /bin/kill <live pid>, bash -c "/bin/kill <live pid>", and sh -c 'exec /bin/kill <live pid>' were all approved.
• residual bypass search: I searched command-position wrappers, interpreter payload extraction, deobfuscation, path qualification, multicall utilities, and argument-forwarding forms. Besides the published path-qualified finding, busybox kill <protected-pid> and echo <protected-pid> | xargs kill also passed the validator, illustrating that lexical enumeration cannot provide a complete no-signal invariant. The direct standard executable paths are sufficient to bypass the stated feature without those more indirect forms.
• reviewer validation: I traced the source through terminal_tool's _check_all_guards call and the post-approval Bash Popen sink, inspected the exact changed regex and target extraction, verified /bin/kill and /usr/bin/kill exist on the host, ran focused assertion probes for direct, foreign-PID, and path-qualified cases, executed the current-main implementation from its bound SHA, checked the read-only merge result, and ran git diff --check. The repository test files could not be run because no available Python environment has pytest installed.

Uncertainty: The changed pytest files were not executable in this lease because neither system Python nor an available project virtual environment provides pytest.; No destructive payload was executed against an actual gateway process; sink reachability is established from the exact approval result, installed executable paths, and the traced Bash execution path.; The complete universe of indirect signal-delivery spellings is not enumerable by this lexical guard; busybox and xargs were sampled as residual bypass classes rather than treated as exhaustive.

Signed: GPT-5.6-sol-xhigh in Codex