From 04382bc9ba2e542d3795e11afb6d39a822cf9487 Mon Sep 17 00:00:00 2001 From: Joao Moura Date: Sat, 8 Aug 2026 21:17:02 -0700 Subject: [PATCH] fix(agents): close guard bypasses and false denies from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pip3.12 and python3.12 -m pip bypassed the direct-pip rule - git push -n is --dry-run, not skip-hooks, so it was denied wrongly - rm -rf docs/images without a trailing slash was not matched - path rules crossed shell separators, denying unrelated later segments - writes into docs/v*/ via redirection, cp, or sed -i were not covered - heredoc bodies were matched as commands, blocking commit messages that merely discuss a blocked command — found by the guard blocking this commit Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ASfWmW3RGy4qAQm6s8U9jH --- .claude/hooks/guard.py | 40 +++++++++++++++++++++++---- .claude/hooks/test_guard.py | 55 +++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/.claude/hooks/guard.py b/.claude/hooks/guard.py index 57da25dca..b1c9fd4a4 100644 --- a/.claude/hooks/guard.py +++ b/.claude/hooks/guard.py @@ -26,23 +26,35 @@ OVERRIDE_MARKER = "# policy-override:" #: (pattern, reason). Patterns match the raw Bash command string. BASH_RULES: tuple[tuple[str, str], ...] = ( ( - r"(?:^|[;&|(\n])\s*(?:sudo\s+)?(?:pip3?|python3?\s+-m\s+pip)\s+" + # Version-suffixed interpreters (pip3.12, python3.12 -m pip) bypass a bare + # pip3? match. `uv pip` is unaffected: the anchor requires command position. + r"(?:^|[;&|(\n])\s*(?:sudo\s+)?" + r"(?:pip(?:[23](?:\.\d+)?)?|python(?:[23](?:\.\d+)?)?\s+-m\s+pip)\s+" r"(?:install|uninstall)\b", "CONTRIBUTING.md (Dependency Management): do not use pip directly. " "Use `uv add --package `, `uv add --dev `, or `uv sync`.", ), ( - r"\bgit\b[^\n;&|]*\b(?:commit|push)\b[^\n;&|]*(?:--no-verify\b|\s-n(?=\s|$))", + # `-n` is only the skip-hooks flag for commit; on push it means --dry-run, + # which is safe and must stay allowed. + r"\bgit\b[^\n;&|]*\b(?:commit|push)\b[^\n;&|]*--no-verify\b" + r"|\bgit\b[^\n;&|]*\bcommit\b[^\n;&|]*\s-n(?=\s|$)", "CONTRIBUTING.md (Commits): do not use --no-verify to skip hooks. " "Fix what pre-commit reports instead.", ), ( - r"\b(?:rm|mv)\b[^\n]*\bdocs/images/", + # Stops at shell separators so a later segment merely naming the path does + # not trigger, and matches the directory with or without a trailing slash. + r"\b(?:rm|mv)\b[^\n;&|]*\bdocs/images(?![\w.-])", "AGENTS.md (Changing Docs, rule 3): do not delete or rename files under " "docs/images/ — frozen doc snapshots still reference them.", ), ( - r"\b(?:rm|mv|tee)\b[^\n]*\bdocs/v[0-9]", + # Write verbs and output redirection. Read-only access (cat, grep, less) + # is deliberately allowed. `cp` is matched in either direction: failing + # closed on a copy out of the tree is cheaper than missing a copy into it. + r"\b(?:rm|mv|cp|tee|sed\s+-i)\b[^\n;&|]*\bdocs/v[0-9]" + r"|>>?\s*[^\n;&|]*\bdocs/v[0-9]", "AGENTS.md (Changing Docs, rule 2): docs/v*/ are frozen release snapshots " "managed by devtools. Edit docs/edge/en/ instead.", ), @@ -74,12 +86,30 @@ def deny(reason: str) -> None: ) +def strip_heredocs(command: str) -> str: + """Replace heredoc bodies with a placeholder. + + A heredoc body is data — a commit message, a file being written, a PR body — + not a command. Matching rules against it produces false denials, such as + blocking a commit whose message merely discusses `rm -rf docs/images`. The + redirection and the delimiter stay on the command line, so a heredoc that + genuinely writes into a protected path is still caught. + """ + return re.sub( + r"(<<-?\s*[\"']?(\w+)[\"']?).*?^\s*\2\b", + r"\1 HEREDOC_BODY", + command, + flags=re.DOTALL | re.MULTILINE, + ) + + def bash_violation(command: str) -> str | None: """Return the reason this command is blocked, or None if it is allowed.""" if OVERRIDE_MARKER in command: return None + inspectable = strip_heredocs(command) for pattern, reason in BASH_RULES: - if re.search(pattern, command): + if re.search(pattern, inspectable): return reason return None diff --git a/.claude/hooks/test_guard.py b/.claude/hooks/test_guard.py index e7eb91eb4..52937a20b 100644 --- a/.claude/hooks/test_guard.py +++ b/.claude/hooks/test_guard.py @@ -46,6 +46,19 @@ BLOCKED_COMMANDS: list[tuple[str, str]] = [ ("rm docs/images/flow.png", "docs/images/"), ("mv docs/images/a.png docs/images/b.png", "docs/images/"), ("rm -rf docs/v1.15.0", "frozen release snapshots"), + # Version-suffixed interpreters must not bypass the pip rule. + ("pip3.12 install ruff", "do not use pip directly"), + ("pip2 install ruff", "do not use pip directly"), + ("python3.12 -m pip install ruff", "do not use pip directly"), + # The protected directory named without a trailing slash. + ("rm -rf docs/images", "docs/images/"), + ("mv docs/images docs/img", "docs/images/"), + # Writes into a frozen snapshot by redirection or copy, not just rm/mv. + ("echo x > docs/v1.15.0/index.mdx", "frozen release snapshots"), + ("cat tmp.mdx >> docs/v1.15.0/index.mdx", "frozen release snapshots"), + ("cp new.mdx docs/v1.15.0/index.mdx", "frozen release snapshots"), + ("sed -i 's/a/b/' docs/v1.15.0/index.mdx", "frozen release snapshots"), + ("tee docs/v1.15.0/index.mdx < new.mdx", "frozen release snapshots"), ] ALLOWED_COMMANDS: list[str] = [ @@ -63,7 +76,19 @@ ALLOWED_COMMANDS: list[str] = [ "uv run pytest -n auto lib/crewai/tests", "uv run pip-audit --skip-editable --ignore-vuln PYSEC-2024-277", "rm docs/edge/en/scratch.mdx", + # Read-only access to a frozen snapshot is allowed; only writes are blocked. "cat docs/v1.15.0/index.mdx", + "grep -rn 'agents' docs/v1.15.0/ > /tmp/hits.txt", + "less docs/v1.15.0/index.mdx", + "ls docs/images/", + # `-n` on push is --dry-run, not skip-hooks. + "git push -n origin main", + "git push --dry-run origin main", + # A protected path named in a later, unrelated command segment. + "rm /tmp/scratch.txt && ls docs/images/", + "rm /tmp/scratch.txt && cat docs/v1.15.0/index.mdx", + # Paths that merely start with the same prefix. + "rm docs/imagesets/old.png", ] @@ -88,6 +113,36 @@ def test_blocked_reasons_cite_a_committed_document() -> None: assert "CONTRIBUTING.md" in reason or "AGENTS.md" in reason +HEREDOC_COMMIT = """git commit -m "$(cat <<'EOF' +fix(agents): close guard bypasses found in review + +- rm -rf docs/images without a trailing slash was not matched +- pip install was reachable via pip3.12 +EOF +)" +""" + + +def test_heredoc_bodies_are_data_not_commands() -> None: + """A commit message discussing a blocked command must not itself be blocked.""" + assert guard.bash_violation(HEREDOC_COMMIT) is None + + +def test_a_heredoc_writing_into_a_frozen_snapshot_is_still_blocked() -> None: + """Stripping the body must not hide a redirection on the command line.""" + command = "cat > docs/v1.15.0/index.mdx <<'EOF'\nsome new content\nEOF\n" + reason = guard.bash_violation(command) + assert reason is not None + assert "frozen release snapshots" in reason + + +def test_a_command_after_a_heredoc_is_still_inspected() -> None: + command = "cat <<'EOF' > notes.txt\njust notes\nEOF\npip install ruff\n" + reason = guard.bash_violation(command) + assert reason is not None + assert "do not use pip directly" in reason + + def test_override_marker_allows_a_stated_exception() -> None: command = "pip install vendored.whl # policy-override: offline wheel, no index" assert guard.bash_violation(command) is None