From 6b58f04d396b881d5d242eacac45bcfdfefe2d00 Mon Sep 17 00:00:00 2001 From: jcwalker3 Date: Tue, 21 Jul 2026 19:56:00 -0500 Subject: [PATCH 1/2] fix(guard): split on `&` and unwrap subshells so real daemon kills classify (Closes #787) `runtime_recovery_guard._SEGMENT_SPLIT_RE` claimed to split a compound command line "on shell separators", but omitted the background-job separator `&`, and no caller stripped a subshell wrapper before tokenising. `_analyse_kill_segment` only inspects the first command token of a segment, so in both forms the kill verb never reached the classifier. Measured against the previous implementation at 35e94e10: sleep 1 & pkill -f mcp_server.py -> process_kill False, contamination False (pkill -f mcp_server.py) -> process_kill False, contamination False Changes: - Add `&` to `_SEGMENT_SPLIT_RE` as `(?:\|\||&&|[|;&\n])`; the two-character logical forms stay first so `&&` and `||` are consumed whole. - Add `_strip_subshell()` and apply it in `_split_segments()`, removing leading `(` and trailing `)` (including nested wrappers) before tokenising. Both forms now classify as contamination with reason class `manual_daemon_kill`, and the record tool writes a durable marker for each. Direct-command behaviour is unchanged. The contamination model, gated-task set, marker lifecycle, and reconciler-only clearing path are untouched. Co-Authored-By: Claude Opus 4.8 (1M context) --- runtime_recovery_guard.py | 26 +++++- .../test_issue_630_runtime_recovery_guard.py | 80 +++++++++++++++++++ 2 files changed, 103 insertions(+), 3 deletions(-) diff --git a/runtime_recovery_guard.py b/runtime_recovery_guard.py index f941403..86012b8 100644 --- a/runtime_recovery_guard.py +++ b/runtime_recovery_guard.py @@ -86,8 +86,12 @@ REMEDIATION = ( # Split a compound command line into simple commands on shell separators so # ``ps aux | grep mcp_server`` is analysed segment by segment and its harmless -# inspection half never reaches the kill classifier. -_SEGMENT_SPLIT_RE = re.compile(r"(?:\|\||&&|\||;|\n)") +# inspection half never reaches the kill classifier. The background separator +# ``&`` is a separator too: without it ``sleep 1 & pkill -f mcp_server.py`` was +# a single segment whose command position held ``sleep``, so the kill was never +# classified (#787). The two-character logical forms are listed first so ``&&`` +# and ``||`` are consumed whole rather than split into single characters. +_SEGMENT_SPLIT_RE = re.compile(r"(?:\|\||&&|[|;&\n])") _KILL_VERBS = frozenset({"kill", "pkill", "killall"}) @@ -133,8 +137,24 @@ def _clean(value: str | None) -> str: return (value or "").strip() +def _strip_subshell(segment: str) -> str: + """Remove subshell wrappers so ``(pkill -f mcp_server.py)`` is classified. + + The parentheses are shell syntax, not part of the simple command, so a + wrapped kill otherwise put ``(pkill`` in command position and never + reached the kill classifier (#787). Nested wrappers are unwrapped too. + """ + stripped = segment.strip() + while stripped.startswith("("): + stripped = stripped[1:].strip() + while stripped.endswith(")"): + stripped = stripped[:-1].strip() + return stripped + + def _split_segments(command: str) -> list[str]: - return [seg for seg in _SEGMENT_SPLIT_RE.split(command) if seg.strip()] + segments = (_strip_subshell(seg) for seg in _SEGMENT_SPLIT_RE.split(command)) + return [seg for seg in segments if seg] def is_sanctioned_recovery(text: str | None) -> bool: diff --git a/tests/test_issue_630_runtime_recovery_guard.py b/tests/test_issue_630_runtime_recovery_guard.py index 4978328..c324f81 100644 --- a/tests/test_issue_630_runtime_recovery_guard.py +++ b/tests/test_issue_630_runtime_recovery_guard.py @@ -91,6 +91,44 @@ def test_compound_command_detects_the_kill_half(): assert result["contamination"] is True +# ── #787: background separator and subshell forms reach the classifier ─────── + +def test_background_separator_kill_is_contamination(): + result = guard.classify_recovery_command("sleep 1 & pkill -f mcp_server.py") + assert result["process_kill"] is True + assert result["contamination"] is True + assert result["reason_class"] == guard.REASON_MANUAL_DAEMON_KILL + assert result["ambiguous"] is False + + +def test_subshell_wrapped_kill_is_contamination(): + result = guard.classify_recovery_command("(pkill -f mcp_server.py)") + assert result["process_kill"] is True + assert result["contamination"] is True + assert result["reason_class"] == guard.REASON_MANUAL_DAEMON_KILL + assert result["ambiguous"] is False + + +def test_further_background_and_subshell_forms_are_contamination(): + for command in ( + "pkill -f mcp_server.py &", + "( sudo pkill -f mcp_server.py )", + "((pkill -f gitea_mcp_server))", + "sleep 1 & killall mcp_server", + "(ps aux | grep mcp_server) & pkill -f mcp_server.py", + ): + result = guard.classify_recovery_command(command) + assert result["contamination"] is True, command + assert result["reason_class"] == guard.REASON_MANUAL_DAEMON_KILL, command + + +def test_logical_operators_are_not_split_into_single_characters(): + # ``&&``/``||`` must still be consumed whole by the widened alternation. + assert guard._split_segments("a && b || c") == ["a", "b", "c"] + assert guard._split_segments("a & b") == ["a", "b"] + assert guard._split_segments("(a)") == ["a"] + + # ── no false positives ─────────────────────────────────────────────────────── def test_read_only_inspection_is_not_a_kill(): @@ -112,6 +150,22 @@ def test_unrelated_pkill_target_is_not_contamination(): assert result["ambiguous"] is False +def test_user_scoped_pkill_of_unrelated_app_is_not_contamination(): + # ``-u`` consumes ``mcpuser``; the surviving operand names no daemon (#787). + result = guard.classify_recovery_command("pkill -u mcpuser -f myapp") + assert result["process_kill"] is True + assert result["contamination"] is False + assert result["ambiguous"] is False + + +def test_commit_message_quoting_the_kill_string_is_not_a_kill(): + result = guard.classify_recovery_command( + 'git commit -m "block pkill -f mcp_server.py as workflow recovery"' + ) + assert result["process_kill"] is False + assert result["contamination"] is False + + def test_bare_kill_of_unknown_pid_is_ambiguous_not_contamination(): result = guard.classify_recovery_command("kill 31337") assert result["contamination"] is False @@ -333,6 +387,32 @@ def test_record_tool_marks_manual_daemon_kill(): assert "mcp_server.py" in loaded["command_summary"] +def test_record_tool_marks_background_separator_kill(): + _clear_marker() + res = srv.gitea_record_daemon_process_kill_attempt( + command="sleep 1 & pkill -f mcp_server.py", remote="prgs" + ) + assert res["contaminated"] is True + assert res["marked"] is True + assert res["marker"]["reason_class"] == guard.REASON_MANUAL_DAEMON_KILL + loaded = srv._load_runtime_recovery_marker("prgs") + assert loaded is not None + assert "mcp_server.py" in loaded["command_summary"] + + +def test_record_tool_marks_subshell_wrapped_kill(): + _clear_marker() + res = srv.gitea_record_daemon_process_kill_attempt( + command="(pkill -f mcp_server.py)", remote="prgs" + ) + assert res["contaminated"] is True + assert res["marked"] is True + assert res["marker"]["reason_class"] == guard.REASON_MANUAL_DAEMON_KILL + loaded = srv._load_runtime_recovery_marker("prgs") + assert loaded is not None + assert "mcp_server.py" in loaded["command_summary"] + + def test_record_tool_marks_broad_sweep(): _clear_marker() res = srv.gitea_record_daemon_process_kill_attempt( -- 2.43.7 From aa4fe1cc7bd2d6f07ea35775a5c5292d94a938e0 Mon Sep 17 00:00:00 2001 From: jcwalker3 Date: Tue, 21 Jul 2026 20:45:17 -0500 Subject: [PATCH 2/2] fix(guard): make shell segment splitting quote-aware (Closes #787) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review #497 on PR #789 found that adding `&` to the separator alternation created a new false-positive class: the splitter was quote-unaware, so any quoted text carrying an ampersand and a kill verb classified as a manual daemon kill. Three commands regressed against base 35e94e10, all of which merely mention the canonical kill string: git commit -m "block sleep 1 & pkill -f mcp_server.py as recovery" echo "docs: sleep 1 & pkill -f mcp_server.py is now detected" grep -rn "sleep 1 & pkill -f mcp_server.py" docs/ A false contamination marker fails review, merge, close and completion mutations closed and only a reconciler may clear it, so this is an operator-visible stall rather than a warning. F1: replace the `_SEGMENT_SPLIT_RE` alternation with a quote-aware scan. `_iter_active` yields only the character positions where a metacharacter is syntactically active — outside single and double quotes, not backslash escaped — and `_split_segments` separates only there. This fixes the class rather than the three named instances, and also retires the `;` and `|` false positives that predate #787. `&&` and `||` are still consumed whole, so logical-separator precedence is unchanged. F3: `_strip_subshell` now removes a trailing `)` only when it closes a leading `(` this call stripped, so `kill $(pgrep -f myapp)` is no longer mangled into `kill $(pgrep -f myapp`; and `_is_redirection` keeps `2>&1`, `>&2` and `&>log` from being read as background separators. Neither changed a verdict at the rejected head, but both are corrected here because the splitter was reworked for F1. Nine focused cases added: the three F1 commands, double-quoted, single-quoted and backslash-escaped ampersands, the POSIX rule that a backslash does not escape inside single quotes, the `;`/`|` class, the two F3 forms, and a record-tool case asserting no marker is written for a quoted mention. Focused suite 64 passed (47 at base 35e94e10, 55 at rejected head 6b58f04). Full suite 11 failed, 4190 passed, 6 skipped; the same 11 failures occur test-for-test at base and are unrelated to this branch. Co-Authored-By: Claude Opus 4.8 (1M context) --- runtime_recovery_guard.py | 136 ++++++++++++++++-- .../test_issue_630_runtime_recovery_guard.py | 101 ++++++++++++- 2 files changed, 227 insertions(+), 10 deletions(-) diff --git a/runtime_recovery_guard.py b/runtime_recovery_guard.py index 86012b8..aca1af5 100644 --- a/runtime_recovery_guard.py +++ b/runtime_recovery_guard.py @@ -49,7 +49,7 @@ from __future__ import annotations import os import re -from typing import Any, Iterable +from typing import Any, Iterable, Iterator # Single source of truth for both the redactor and the gated-mutation set: the # #671 guard already owns them, so the two contamination models can never drift @@ -89,9 +89,23 @@ REMEDIATION = ( # inspection half never reaches the kill classifier. The background separator # ``&`` is a separator too: without it ``sleep 1 & pkill -f mcp_server.py`` was # a single segment whose command position held ``sleep``, so the kill was never -# classified (#787). The two-character logical forms are listed first so ``&&`` -# and ``||`` are consumed whole rather than split into single characters. -_SEGMENT_SPLIT_RE = re.compile(r"(?:\|\||&&|[|;&\n])") +# classified (#787). +# +# Splitting is *quote-aware*, and a regex alternation cannot express that, so +# the scan below replaces the earlier ``_SEGMENT_SPLIT_RE`` pattern. A separator +# only separates where it is syntactically active: outside single and double +# quotes, and not backslash-escaped. Without that, adding ``&`` made every +# benign mention of the canonical kill string classify as a real kill — a commit +# message quoting ``sleep 1 & pkill -f mcp_server.py``, an ``echo`` of the same +# sentence, a ``grep`` for it — and a false contamination marker fails review, +# merge, close and completion mutations closed until a reconciler clears it (PR +# #789 review finding F1). Quote-awareness is not specific to ``&``: it also +# retires the same false-positive class that ``;`` and ``|`` carried before #787. +_SEPARATOR_CHARS = frozenset("|&;\n") + +#: Two-character logical separators, consumed whole so ``&&`` and ``||`` are +#: never split into single characters leaving a stray operator behind. +_LOGICAL_SEPARATORS = ("&&", "||") _KILL_VERBS = frozenset({"kill", "pkill", "killall"}) @@ -137,24 +151,128 @@ def _clean(value: str | None) -> str: return (value or "").strip() +def _iter_active(text: str) -> Iterator[tuple[int, str]]: + """Yield ``(index, char)`` for every *syntactically active* character. + + Active means outside single and double quotes and not backslash-escaped — + the positions where a shell metacharacter actually carries its meaning. + Quoted runs, the quote characters themselves, and escaped characters are + skipped, so a separator written inside a commit message or a ``grep`` + pattern is literal text rather than syntax. A backslash escapes nothing + inside single quotes, matching POSIX. + + An unterminated quote swallows the rest of the line, exactly as it does for + the shell — which would reject such a command as a syntax error rather than + run its tail, so nothing executable hides behind it. + """ + quote: str | None = None + index = 0 + end = len(text) + while index < end: + char = text[index] + if quote == "'": + if char == "'": + quote = None + index += 1 + elif quote == '"': + if char == "\\" and index + 1 < end: + index += 2 + else: + if char == '"': + quote = None + index += 1 + elif char == "\\" and index + 1 < end: + index += 2 + elif char in ("'", '"'): + quote = char + index += 1 + else: + yield index, char + index += 1 + + +def _is_redirection(command: str, index: int, active: frozenset[int]) -> bool: + """Is the ``&``/``|`` at *index* part of a redirection, not a separator? + + ``2>&1`` and ``>&2`` put the character immediately after a redirection + operator, and ``&>log`` immediately before one; in neither position does it + separate commands. Without this, ``a 2>&1`` split into ``['a 2>', '1']`` + (PR #789 review finding F3). + """ + previous = command[index - 1] if index else "" + if previous in ("<", ">") and (index - 1) in active: + return True + return ( + command[index] == "&" + and command[index + 1:index + 2] == ">" + and (index + 1) in active + ) + + +def _closes_leading_paren(body: str) -> bool: + """Does *body* end with the active ``)`` matching a stripped leading ``(``?""" + if not body.endswith(")"): + return False + depth = 0 + for index, char in _iter_active(body): + if char == "(": + depth += 1 + elif char == ")": + if depth == 0: + return index == len(body) - 1 + depth -= 1 + return False + + def _strip_subshell(segment: str) -> str: """Remove subshell wrappers so ``(pkill -f mcp_server.py)`` is classified. The parentheses are shell syntax, not part of the simple command, so a wrapped kill otherwise put ``(pkill`` in command position and never reached the kill classifier (#787). Nested wrappers are unwrapped too. + + A trailing ``)`` is removed only when it closes a leading ``(`` this call + stripped. Removing one unconditionally mangled balanced command + substitution — ``kill $(pgrep -f myapp)`` became ``kill $(pgrep -f myapp`` + (PR #789 review finding F3). An unmatched leading ``(`` is still dropped on + its own, because splitting a wrapped compound orphans the opening half. """ stripped = segment.strip() while stripped.startswith("("): - stripped = stripped[1:].strip() - while stripped.endswith(")"): - stripped = stripped[:-1].strip() + body = stripped[1:].strip() + if _closes_leading_paren(body): + body = body[:-1].strip() + stripped = body return stripped def _split_segments(command: str) -> list[str]: - segments = (_strip_subshell(seg) for seg in _SEGMENT_SPLIT_RE.split(command)) - return [seg for seg in segments if seg] + """Split *command* into simple commands on syntactically active separators.""" + active = frozenset(index for index, _ in _iter_active(command)) + segments: list[str] = [] + start = 0 + index = 0 + end = len(command) + while index < end: + char = command[index] + if ( + char not in _SEPARATOR_CHARS + or index not in active + or (char in "&|" and _is_redirection(command, index, active)) + ): + index += 1 + continue + width = ( + 2 + if command[index:index + 2] in _LOGICAL_SEPARATORS + and (index + 1) in active + else 1 + ) + segments.append(command[start:index]) + index += width + start = index + segments.append(command[start:]) + return [seg for seg in (_strip_subshell(seg) for seg in segments) if seg] def is_sanctioned_recovery(text: str | None) -> bool: diff --git a/tests/test_issue_630_runtime_recovery_guard.py b/tests/test_issue_630_runtime_recovery_guard.py index c324f81..1a2d0e1 100644 --- a/tests/test_issue_630_runtime_recovery_guard.py +++ b/tests/test_issue_630_runtime_recovery_guard.py @@ -123,10 +123,98 @@ def test_further_background_and_subshell_forms_are_contamination(): def test_logical_operators_are_not_split_into_single_characters(): - # ``&&``/``||`` must still be consumed whole by the widened alternation. + # ``&&``/``||`` must still be consumed whole by the separator scan. assert guard._split_segments("a && b || c") == ["a", "b", "c"] assert guard._split_segments("a & b") == ["a", "b"] assert guard._split_segments("(a)") == ["a"] + assert guard._split_segments("a; b\nc | d") == ["a", "b", "c", "d"] + + +# ── #789 F1: separators only separate outside quoted or escaped text ───────── + +# The three commands the PR #789 review measured as regressions at head +# 6b58f04: each merely *mentions* the canonical kill string inside quotes. +F1_QUOTED_COMMANDS = ( + 'git commit -m "block sleep 1 & pkill -f mcp_server.py as recovery"', + 'echo "docs: sleep 1 & pkill -f mcp_server.py is now detected"', + 'grep -rn "sleep 1 & pkill -f mcp_server.py" docs/', +) + + +def test_quoted_ampersand_examples_from_review_f1_are_not_kills(): + for command in F1_QUOTED_COMMANDS: + result = guard.classify_recovery_command(command) + assert result["process_kill"] is False, command + assert result["contamination"] is False, command + assert result["reason_class"] is None, command + + +def test_ampersand_inside_double_quotes_is_not_a_separator(): + assert guard._split_segments('echo "a & b"') == ['echo "a & b"'] + result = guard.classify_recovery_command( + 'echo "restart it: sleep 1 & pkill -f mcp_server.py"' + ) + assert result["process_kill"] is False + assert result["contamination"] is False + + +def test_ampersand_inside_single_quotes_is_not_a_separator(): + assert guard._split_segments("echo 'a & b'") == ["echo 'a & b'"] + result = guard.classify_recovery_command( + "git commit -m 'sleep 1 & pkill -f mcp_server.py stays quoted'" + ) + assert result["process_kill"] is False + assert result["contamination"] is False + + +def test_backslash_escaped_ampersand_is_not_a_separator(): + command = r"echo a \& pkill -f mcp_server.py" + assert guard._split_segments(command) == [command] + result = guard.classify_recovery_command(command) + assert result["process_kill"] is False + assert result["contamination"] is False + + +def test_backslash_does_not_escape_inside_single_quotes(): + # POSIX: a backslash is literal inside single quotes, so the closing quote + # still closes and the following ``&`` is a genuinely active separator. + command = r"echo 'a\' & pkill -f mcp_server.py" + assert guard._split_segments(command) == [r"echo 'a\'", "pkill -f mcp_server.py"] + assert guard.classify_recovery_command(command)["contamination"] is True + + +def test_quote_awareness_also_retires_the_pre_existing_semicolon_and_pipe_cases(): + # ``;`` and ``|`` misclassified quoted text before #787 as well. The fix is + # the quote-unawareness, not the ``&`` instance the issue happens to name. + for command in ( + 'git commit -m "fix; pkill -f mcp_server.py"', + 'git commit -m "fix | pkill -f mcp_server.py"', + ): + result = guard.classify_recovery_command(command) + assert result["process_kill"] is False, command + assert result["contamination"] is False, command + + +# ── #789 F3: subshell stripping and redirection stay syntactically honest ──── + +def test_command_substitution_is_not_mangled_by_subshell_stripping(): + # Only a wrapper this call opened may be unwrapped; a ``)`` closing ``$(`` + # must survive intact. + assert guard._strip_subshell("kill $(pgrep -f myapp)") == "kill $(pgrep -f myapp)" + result = guard.classify_recovery_command("kill $(pgrep -f myapp)") + assert result["contamination"] is False + assert result["ambiguous"] is True + + +def test_redirection_is_not_treated_as_a_background_separator(): + assert guard._split_segments("a 2>&1") == ["a 2>&1"] + assert guard._split_segments("a &> log") == ["a &> log"] + assert guard._split_segments("pkill -f mcp_server.py 2>&1") == [ + "pkill -f mcp_server.py 2>&1" + ] + result = guard.classify_recovery_command("pkill -f mcp_server.py 2>&1") + assert result["contamination"] is True + assert result["reason_class"] == guard.REASON_MANUAL_DAEMON_KILL # ── no false positives ─────────────────────────────────────────────────────── @@ -413,6 +501,17 @@ def test_record_tool_marks_subshell_wrapped_kill(): assert "mcp_server.py" in loaded["command_summary"] +def test_record_tool_does_not_mark_a_quoted_mention_of_the_kill_string(): + # The marker is what fails review/merge/close closed and only a reconciler + # may clear it, so a quoted mention must never create one (PR #789 F1). + for command in F1_QUOTED_COMMANDS: + _clear_marker() + res = srv.gitea_record_daemon_process_kill_attempt(command=command, remote="prgs") + assert res["contaminated"] is False, command + assert res["marked"] is False, command + assert srv._load_runtime_recovery_marker("prgs") is None, command + + def test_record_tool_marks_broad_sweep(): _clear_marker() res = srv.gitea_record_daemon_process_kill_attempt( -- 2.43.7