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
@@ -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:

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(