diff --git a/runtime_recovery_guard.py b/runtime_recovery_guard.py index f941403..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 @@ -86,8 +86,26 @@ 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). +# +# 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"}) @@ -133,8 +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("("): + body = stripped[1:].strip() + if _closes_leading_paren(body): + body = body[:-1].strip() + stripped = body + return stripped + + def _split_segments(command: str) -> list[str]: - return [seg for seg in _SEGMENT_SPLIT_RE.split(command) if seg.strip()] + """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 4978328..1a2d0e1 100644 --- a/tests/test_issue_630_runtime_recovery_guard.py +++ b/tests/test_issue_630_runtime_recovery_guard.py @@ -91,6 +91,132 @@ 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 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 ─────────────────────────────────────────────────────── def test_read_only_inspection_is_not_a_kill(): @@ -112,6 +238,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 +475,43 @@ 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_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(