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(