fix(workflow-guard): only guard commands that actually push

The parser failed closed on any execute command containing shell
metacharacters, even ones with no `git push` at all (a bundled
clone+commit heredoc, `echo x && ls`). That blocked the E2E full-flow
implement step before it could push, so the opened PR carried an empty
file list and Playwright failed — and it would break most real agent
shell usage (pipes, redirects, heredocs, env vars).

Engage the guard only when the command genuinely invokes `git push`,
detected precisely on the shlex-normalized token stream. Push the E2E
branch as a standalone, inspectable `git push` rather than bundling it
in the heredoc.

Hardening (from an adversarial security-review pass): a `git push` the
shell would run can hide from a literal token scan via fused separators
(`true;git push`), subshell/grouping (`(git push …)`), value-option
splicing (`git -c ; git push`), or expansion/quoting (`git${IFS}push`,
`git $(printf push)`, `git $'push'`, `git $'\x70ush'`). Fail closed on
those via a quote-aware command skeleton (ANSI-C `$'…'` decoded) while
still allowing legitimate metacharacter commands — chained commits,
pipes, redirects, `$VAR`, and `$'…\t…'` format strings.

agent/middleware/workflow_push_guard.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import asyncio
+import codecs
 import hashlib
 import json
 import logging
@@ -44,10 +45,19 @@ _SHELL_OPERATORS = {";", "|", "||", "&"}
 _REF_NAME = re.compile(r"^[A-Za-z0-9._/@+-]+$")
 _GIT_OBJECT_ID = re.compile(r"^[0-9a-fA-F]{40,64}$")
 _UNSAFE_RAW_COMMAND = re.compile(r"[;|`$<>\n\r]")
+# Unquoted shell expansion/substitution (`$VAR`, `${...}`, `$(...)`, backticks) can produce
+# a `git push` whose text we cannot see; its presence alongside a push forces fail-closed.
+_EXPANSION = re.compile(r"[$`]")
+# Unquoted command separators and grouping. Splitting the unquoted command skeleton on these
+# yields the individual simple-commands the shell would run, each checked for a `git push`.
+_SEGMENT_SPLIT = re.compile(r"[;|&(){}\n]+")
 # Command wrappers that can prefix a `git` invocation (e.g. `command git push`,
 # `env FOO=bar git push`, `/usr/bin/git push`). Recognized so a wrapped or path-qualified
 # git push cannot slip past the guard as "not a git command".
 _GIT_WRAPPERS = {"command", "env", "nice", "sudo", "stdbuf", "nohup", "time", "ionice", "setsid"}
+# Git global options that consume the following token as a separate value; skipped when
+# locating the git subcommand so `git -c k=v push` is still recognized as a push.
+_GIT_VALUE_OPTIONS = {"-C", "-c", "--namespace", "--git-dir", "--work-tree", "--exec-path"}
 
 
 class _BlockedGitPush:
@@ -160,14 +170,145 @@ def _response_ok(response: Any) -> bool:
     return True
 
 
+def _tokens_invoke_git_push(tokens: list[str]) -> bool:
+    """True if the token stream runs `git push` as a git subcommand (not merely mentions it).
+
+    Scans each `git` invocation (bareword or path-qualified, after any wrapper prefix),
+    skips `-C <dir>` and option flags, and checks whether the first positional argument is
+    `push`. This ignores the word "push" appearing inside a commit message or another
+    command, so `git commit -m "push it" && ls` is not treated as a push, while a genuine
+    `git ... push` anywhere in a chain is.
+    """
+    i = 0
+    while i < len(tokens):
+        if tokens[i].rsplit("/", 1)[-1] != "git":
+            i += 1
+            continue
+        j = i + 1
+        while j < len(tokens):
+            tok = tokens[j]
+            if tok in _SHELL_OPERATORS or tok == "&&":
+                break
+            if (
+                tok in _GIT_VALUE_OPTIONS
+                and j + 1 < len(tokens)
+                and tokens[j + 1] not in _SHELL_OPERATORS
+                and tokens[j + 1] != "&&"
+            ):
+                j += 2  # option consumes the next token as its value
+                continue
+            if tok.startswith("-"):
+                j += 1
+                continue
+            if tok == "push":
+                return True
+            break  # first positional is a non-push subcommand
+        i = j + 1
+    return False
+
+
+def _decode_ansi_c(body: str) -> str:
+    """Best-effort ANSI-C (`$'...'`) escape decoding, so `$'\\x70ush'` / `$'\\160ush'` reveal
+    the literal `push` the shell would run. Falls back to the raw body if decoding fails."""
+    if "\\" not in body:
+        return body
+    try:
+        return codecs.decode(body, "unicode_escape")
+    except Exception:
+        return body
+
+
+def _strip_quoted(command: str) -> str:
+    """Return the command with quoted spans and escaped characters blanked to spaces.
+
+    Leaves only the unquoted shell structure, so metacharacters that are literal (inside
+    quotes or backslash-escaped — e.g. a `;` or `&` inside a commit message) do not look
+    like command separators. Plain `'...'`/`"..."` spans wrap arguments and are blanked;
+    `$'...'`/`$"..."` (ANSI-C / locale quoting) form a literal *word* (e.g. `git $'push'` runs
+    `git push`), so their content is kept — otherwise the push word would vanish. Unbalanced
+    quotes blank the remainder (fail-closed shape).
+    """
+    out: list[str] = []
+    i, n = 0, len(command)
+    while i < n:
+        c = command[i]
+        if c in "'\"":
+            # `$'...'` / `$"..."` — the preceding `$` marks a literal word, so keep content
+            # and drop the `$` so adjacent pieces concatenate (`$'pus'$'h'` -> `push`).
+            word_quote = bool(out) and out[-1] == "$"
+            if word_quote:
+                out.pop()
+            if c == "'":
+                j = command.find("'", i + 1)
+                if j == -1:
+                    return "".join(out) + " "
+                out.append(_decode_ansi_c(command[i + 1 : j]) if word_quote else " ")
+                i = j + 1
+            else:
+                k = i + 1
+                buf: list[str] = []
+                while k < n and command[k] != '"':
+                    if command[k] == "\\" and k + 1 < n:
+                        buf.append(command[k + 1])
+                        k += 2
+                    else:
+                        buf.append(command[k])
+                        k += 1
+                if k >= n:
+                    return "".join(out) + " "
+                out.append("".join(buf) if word_quote else " ")
+                i = k + 1
+        elif c == "\\" and i + 1 < n:
+            out.append(" ")
+            i += 2
+        else:
+            out.append(c)
+            i += 1
+    return "".join(out)
+
+
+def _maybe_obfuscated_git_push(command: str) -> bool:
+    """True if the shell would run a `git push` that `_tokens_invoke_git_push` could not see.
+
+    Only reached when the precise token scan found no clean push. `shlex` splits on
+    whitespace and performs no expansion, so a push can hide behind a fused separator
+    (`true;git push`), subshell/grouping (`(git push ...)`), or expansion (`git${IFS}push`,
+    `git $(printf push)`). We work on the unquoted command skeleton so literal metacharacters
+    inside a commit message are ignored (no false positive on `git commit -m "push it" && ls`
+    or `git log | grep push`), then fail closed if any unquoted simple-command is a `git push`
+    or if unquoted expansion could produce one.
+    """
+    skeleton = _strip_quoted(command)
+    if "push" not in skeleton:
+        # The only `push` text is inside quotes (a literal argument), so it cannot be a
+        # command word. (A push spelled purely by expansion with no literal `push` anywhere
+        # is left to the GitHub proxy token scope, which lacks workflows:write until approval.)
+        return False
+    if _EXPANSION.search(skeleton):
+        return True
+    return any(
+        _tokens_invoke_git_push(segment.split()) for segment in _SEGMENT_SPLIT.split(skeleton)
+    )
+
+
 def _parse_git_push(command: str) -> ParsedGitPush | _BlockedGitPush | None:
     stripped = command.strip()
+    try:
+        probe = shlex.split(stripped)
+    except ValueError:
+        probe = None
+    if probe is None or not _tokens_invoke_git_push(probe):
+        # No push the token scan can normalize. Fail closed if one is hidden behind shell
+        # metacharacters/expansion; otherwise leave the command untouched.
+        if _maybe_obfuscated_git_push(stripped):
+            return _BlockedGitPush(
+                "possible git push obscured by shell metacharacters; issue it as a plain "
+                "`git push origin <branch>`"
+            )
+        return None
     if _UNSAFE_RAW_COMMAND.search(stripped) or "&" in stripped.replace("&&", ""):
         return _BlockedGitPush("unsafe shell characters in git push command")
-    try:
+    tokens = probe
-        tokens = shlex.split(stripped)
-    except ValueError:
-        return _BlockedGitPush("unparseable git push command")
     if not tokens:
         return None
 

tests/e2e/fake_llm.py

@@ -28,8 +28,11 @@ from langchain_core.language_models import BaseChatModel
 from langchain_core.messages import AIMessage, BaseMessage, HumanMessage, ToolMessage
 from langchain_core.outputs import ChatGeneration, ChatResult
 
-# One shell command that does the whole git workflow. Each execute() runs in a
+# One shell command that clones, implements, and commits. Each execute() runs in a
-# fresh shell rooted at the sandbox dir, so the clone+commit+push is bundled.
+# fresh shell rooted at the sandbox dir, and the `repo` checkout persists on disk
+# between calls. The push is a separate, standalone step: the workflow-push guard only
+# inspects (and gates) pushes issued as a plain `git push origin <branch>`, so bundling
+# the push inside this heredoc would trip its fail-closed check.
 _IMPLEMENT_SCRIPT = f"""
 set -e
 rm -rf repo
@@ -44,10 +47,12 @@ def greet(name):
 EOF
 git add -A
 git commit -m "{PR_TITLE}"
-git push origin {FEATURE_BRANCH}
+echo IMPLEMENTED_OK
-echo PUSHED_OK
 """.strip()
 
+# Standalone push in the inspectable form the workflow-push guard expects.
+_PUSH_SCRIPT = f"cd repo && git push origin {FEATURE_BRANCH}"
+
 _PLAN_URL_RE = re.compile(r"https?://[^\s\"'<>)\]|]+/plan\b")
 _ATTRIBUTION_RE = re.compile(r"@([A-Za-z0-9-]+):")
 
@@ -281,6 +286,12 @@ SCRIPT_LIBRARY: dict[str, tuple[StepSpec, ...]] = {
             {"command": _IMPLEMENT_SCRIPT},
             "call-impl",
         ),
+        _tool_step(
+            "Pushing the branch.",
+            "execute",
+            {"command": _PUSH_SCRIPT},
+            "call-push",
+        ),
         _tool_step(
             "Opening a pull request.",
             "open_pull_request",

tests/test_workflow_push_guard.py

@@ -169,6 +169,82 @@ def test_parse_git_push_blocks_unsafe_and_unrecognized_forms() -> None:
     assert guard._parse_git_push("git status") is None
 
 
+def test_parse_git_push_ignores_commands_without_a_push() -> None:
+    # Chained or heredoc commands that never `git push` are not our concern — the
+    # fail-closed blocking is reserved for commands that actually push.
+    assert guard._parse_git_push("echo planning && ls") is None
+    assert guard._parse_git_push("git add -A && git commit -m 'x'") is None
+    assert guard._parse_git_push("cat > f <<'EOF'\nhi\nEOF") is None
+    # "push" inside a commit message is not a push subcommand.
+    assert guard._parse_git_push("git commit -m 'push it real good' && ls") is None
+    # A standalone `cd <dir> && git push origin <branch>` stays inspectable, not blocked.
+    parsed = guard._parse_git_push("cd repo && git push origin feature")
+    assert isinstance(parsed, guard.ParsedGitPush)
+    assert parsed.repo_dir == "repo"
+    assert parsed.remote_ref == "feature"
+
+
+def test_parse_git_push_still_catches_quote_obfuscated_push() -> None:
+    # Shell-quoting that the shell would run as a real `git push` must not slip past the
+    # guard just because the raw string lacks a literal "push" token: it is detected and
+    # inspected, never passed through as None.
+    parsed = guard._parse_git_push('git "pu""sh" origin feature')
+    assert isinstance(parsed, guard.ParsedGitPush)
+    assert parsed.remote_ref == "feature"
+    assert guard._tokens_invoke_git_push(["git", "status", "&&", "git", "push"]) is True
+    assert guard._tokens_invoke_git_push(["command", "git", "push", "origin", "x"]) is True
+    assert guard._tokens_invoke_git_push(["git", "-c", "protocol.version=2", "push"]) is True
+    assert guard._tokens_invoke_git_push(["git", "commit", "-m", "push"]) is False
+    # A value-option must not swallow a shell operator as its "value" and thereby hide the
+    # real `git push` that follows the operator.
+    assert guard._tokens_invoke_git_push(["git", "-c", ";", "git", "push"]) is True
+
+
+def test_parse_git_push_fails_closed_on_metachar_obfuscated_push() -> None:
+    # `shlex` splits only on whitespace and performs no expansion, so a push can hide behind
+    # a fused separator or a variable/command expansion. Each of these is a real push the
+    # sandbox shell would run, and must fail closed rather than pass through unguarded.
+    for command in (
+        "true;git push origin HEAD",
+        "echo hi|git push origin HEAD",
+        "git${IFS}push origin HEAD",
+        "$(echo git) push origin HEAD",
+        "git $(printf push) origin main",
+        "cmd=push; git ${cmd} origin main",
+        "git -c ; git push origin main",
+        "git -c | git push origin main",
+        "(git push origin main)",
+        "(git push -u origin HEAD)",
+        "{ git push origin main; }",
+        "git $'push' origin main",
+        "git $'pus'$'h' origin main",
+        r"git $'\x70ush' origin main",
+        r"git $'\160ush' origin main",
+    ):
+        assert isinstance(guard._parse_git_push(command), guard._BlockedGitPush), command
+
+
+def test_parse_git_push_allows_legitimate_metachar_commands() -> None:
+    # Standalone operators / redirects around a non-push git command (or a push mentioned in
+    # a commit message) are common and must not be over-blocked. Metacharacters that are
+    # literal because they sit inside a quoted commit message must not read as separators.
+    assert guard._parse_git_push("git commit -m 'push it real good' && ls") is None
+    assert guard._parse_git_push('git commit -m "push & shove (v2)" && ls') is None
+    assert guard._parse_git_push('git commit -m "$MSG about push" && npm test') is None
+    assert guard._parse_git_push("git log | grep push") is None
+    assert guard._parse_git_push("git diff > push.txt") is None
+    # A non-push git command with an unquoted variable must not be blocked just for the `$`.
+    assert guard._parse_git_push("git checkout $BRANCH") is None
+    assert guard._parse_git_push("git log --grep=$PATTERN | head") is None
+    # Legitimate ANSI-C quoting (a tab/newline in a git format or message) is not a push.
+    assert guard._parse_git_push(r"git log --pretty=$'%h\t%s'") is None
+    assert guard._parse_git_push(r"git commit -m $'line1\nline2'") is None
+    # The standard `cd <dir> && git push` form is recognized, not blocked as obfuscation.
+    parsed = guard._parse_git_push("cd repo && git push origin feature")
+    assert isinstance(parsed, guard.ParsedGitPush)
+    assert parsed.remote_ref == "feature"
+
+
 def test_workflow_change_for_push_fingerprints_head_workflow_tree() -> None:
     backend = _Backend()
     change = guard._workflow_change_for_push(