fix(guard): make shell segment splitting quote-aware (Closes #787)

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) <[email protected]>

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:

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(