patterns: drop sandbox/scripts wholesale block, enforce via reviewer_security (#2133)

.egg/phase-permissions.json

@@ -215,11 +215,10 @@
         "**/*.spec.js",
         "**/*.spec.jsx",
         "**/conftest.py",
-        ".github/",
-        "sandbox/scripts/"
+        ".github/"
       ],
-      "blocked_reason": "Coder cannot modify tests, docs, pipeline-state, CI workflows, or credential shims; see #1901.",
-      "description": "TODO(#1903): keep in sync with CODER_PATTERNS / TESTER_PATTERNS / DOCUMENTER_PATTERNS in shared/egg_restrictions/patterns.py. Coder: everything except tester, documenter, .egg-state/, .github/, and sandbox/scripts/ scopes."
+      "blocked_reason": "Coder cannot modify tests, docs, pipeline-state, or CI workflows; see #1901. sandbox/scripts/ is writable — credential-shim modifications are reviewed by reviewer_security (see shared/prompts/security-review-criteria.md), not blocked at this layer.",
+      "description": "TODO(#1903): keep in sync with CODER_PATTERNS / TESTER_PATTERNS / DOCUMENTER_PATTERNS in shared/egg_restrictions/patterns.py. Coder: everything except tester, documenter, .egg-state/, and .github/ scopes."
     },
     {
       "role": "tester",

docs/guides/sdlc-pipeline.md

@@ -588,7 +588,7 @@ Agents are organized into five categories (execution, analysis, review, utility,
 
 | Role | Category | Purpose | File Access |
 |------|----------|---------|-------------|
-| **Coder** | Execution | Implements code changes | All files except docs, tests, `.egg-state/`, `.github/`, `sandbox/scripts/` (blocklist-complement; see [Agent Roles Reference](../reference/agent-roles.md#coder)) |
+| **Coder** | Execution | Implements code changes | All files except docs, tests, `.egg-state/`, `.github/` (blocklist-complement; see [Agent Roles Reference](../reference/agent-roles.md#coder)) |
 | **Tester** | Execution | Finds gaps, writes tests, runs linters and reports issues to coder | Test files and infrastructure only: `tests/`, `test/`, `**/test_*.py`, `**/*_test.go`, `**/*.test.{ts,tsx,js,jsx}`, `**/*.spec.{ts,tsx,js,jsx}`, `**/conftest.py` (see [Agent Roles Reference](../reference/agent-roles.md#tester)) |
 | **Documenter** | Execution | Updates documentation | Documentation and markdown only: `docs/`, `**/*.md`, `**/README.md` (see [Agent Roles Reference](../reference/agent-roles.md#documenter)) |
 | **Autofixer** | Utility | Auto-fixes lint/format/type-check issues | Source and config files (no docs or contracts) |

docs/reference/agent-roles.md

@@ -161,10 +161,12 @@ each surface so reviewers know to keep them in sync.
   (`**/*_test.py`, `**/test_*.py`, `**/*_test.go`, `**/test_*.go`,
   `**/*.test.{ts,tsx,js,jsx}`, `**/*.spec.{ts,tsx,js,jsx}`),
   `**/conftest.py` (tester's scope); `.egg-state/` (pipeline state);
-  plus defense-in-depth blocks on `.github/` (CI workflows and
-  CODEOWNERS — preserves the branch-protection invariant) and
-  `sandbox/scripts/` (gateway credential shims — preserves the
-  credential-routing invariant).
+  plus a defense-in-depth block on `.github/` (CI workflows and
+  CODEOWNERS — preserves the branch-protection invariant).
+  `sandbox/scripts/` is **writable** — the gateway is the sole egress
+  chokepoint, so credential-shim modifications are reviewed by
+  `reviewer_security` rather than blocked at the role-pattern layer.
+  See [security-review-criteria.md](../../shared/prompts/security-review-criteria.md).
 - Block exemptions (always writable, overriding the blocks above):
   `.egg-state/agent-outputs/` (coder's handoff output),
   `.egg-state/agent-anchors/` (per-agent anchor state, scoped by

gateway/tests/test_agent_restrictions_comprehensive.py

@@ -573,6 +573,7 @@ class TestThreeRoleBehavior1901:
             ".egg-state/agent-outputs/coder.json",  # carved-back exempt
             "skills/my-skill/SKILL.md",  # skills exempt
             "sandbox/agent-config/rules/foo.md",  # rules exempt
+            "sandbox/scripts/gh",  # credential shim — gateway is the chokepoint
         ],
     )
     def test_coder_allowed_blocklist_complement(self, path):
@@ -592,7 +593,6 @@ def test_coder_allowed_blocklist_complement(self, path):
             ".egg-state/secrets/key",  # future subdir
             ".github/workflows/ci.yml",  # branch-protection invariant
             ".github/CODEOWNERS",  # branch-protection invariant
-            "sandbox/scripts/gh",  # credential-routing invariant
         ],
     )
     def test_coder_blocked_blocklist_complement(self, path):

gateway/tests/test_agent_restrictions_patterns.py

@@ -359,13 +359,15 @@ def test_allows_sandbox_egg(self, pattern):
     def test_allows_sandbox_bin_egg_health_inspect(self, pattern):
         assert pattern.can_write("sandbox/bin/egg-health-inspect") is True
 
-    def test_blocks_sandbox_scripts_gh(self, pattern):
-        """sandbox/scripts/ is blocked (credential-routing invariant)."""
-        assert pattern.can_write("sandbox/scripts/gh") is False
-
-    def test_blocks_sandbox_scripts_git_credential_helper(self, pattern):
-        """sandbox/scripts/ is blocked (credential-routing invariant)."""
-        assert pattern.can_write("sandbox/scripts/git-credential-github-token") is False
+    def test_allows_sandbox_scripts_gh(self, pattern):
+        """sandbox/scripts/ is writable; the gateway is the sole egress
+        chokepoint, so credential-shim modifications are reviewed by
+        reviewer_security rather than blocked at the role-pattern layer."""
+        assert pattern.can_write("sandbox/scripts/gh") is True
+
+    def test_allows_sandbox_scripts_git_credential_helper(self, pattern):
+        """sandbox/scripts/ is writable; see test_allows_sandbox_scripts_gh."""
+        assert pattern.can_write("sandbox/scripts/git-credential-github-token") is True
 
     def test_blocks_github_workflows(self, pattern):
         """.github/ is blocked (branch-protection invariant)."""

gateway/tests/test_phase_filter_restrictions.py

@@ -587,6 +587,19 @@ def test_coder_allowed_for_extensionless_script(self):
         result = check_file_restrictions("coder", ["bin/egg"])
         assert result.allowed is True
 
+    def test_coder_allowed_for_sandbox_scripts(self):
+        """sandbox/scripts/ is writable by the coder at the gateway
+        file_restrictions layer (#2133). Credential-shim modifications
+        are reviewed by reviewer_security rather than blocked at the
+        role-pattern layer; this test locks the synchronization between
+        .egg/phase-permissions.json and shared/egg_restrictions/patterns.py
+        so the two layers do not drift apart. Mirrors the patterns.py
+        layer coverage of both `gh` and `git-credential-github-token`."""
+        result = check_file_restrictions("coder", ["sandbox/scripts/gh"])
+        assert result.allowed is True
+        result = check_file_restrictions("coder", ["sandbox/scripts/git-credential-github-token"])
+        assert result.allowed is True
+
     # --- tester role ---
 
     def test_tester_blocked_from_contracts(self):

shared/egg_restrictions/patterns.py

@@ -231,9 +231,6 @@ def _matches_pattern(file_path: str, pattern: str) -> bool:
         # Defense-in-depth: CI workflows and CODEOWNERS — preserves the
         # branch-protection invariant.
         ".github/",
-        # Defense-in-depth: gateway credential shims — preserves the
-        # credential-routing invariant.
-        "sandbox/scripts/",
     ],
     block_exempt_patterns=[
         # Coder's handoff directory — the only .egg-state/ subdir the coder
@@ -257,14 +254,6 @@ def _matches_pattern(file_path: str, pattern: str) -> bool:
         "sandbox/agent-config/commands/*.md",
         # Top-level skills directory (skill definitions are functional code)
         "skills/",
-        # Jira sandbox wrapper — new in #1556.  ``sandbox/scripts/`` is
-        # blocked wholesale to keep agents from retargeting the existing
-        # ``gh`` / ``git`` credential shims; this exemption lets the coder
-        # land the NEW ``jira`` wrapper alongside them.  The file is
-        # functionally identical in shape (bash wrapper that POSTs to a
-        # gateway REST endpoint — never holds credentials locally) and is
-        # the only addition permitted under the broader block.
-        "sandbox/scripts/jira",
     ],
 )
 

shared/prompts/security-review-criteria.md

@@ -91,7 +91,59 @@ Verification recipe:
 3. Flag broken symlinks (`ls -l <link>` returns a missing target) as a
    blocking finding regardless of code quality.
 
-### 5. Secret leakage
+### 5. Credential-shim modifications under `sandbox/scripts/`
+
+The wrappers under `sandbox/scripts/` (`gh`, `git`, `jira`, …) are the
+sandbox's **only** egress path to credential-bearing services. The
+gateway sidecar is the actual security boundary — these wrappers hold
+no credentials and any request they emit is independently re-validated
+by the gateway against its policy. So a compromised wrapper cannot
+bypass the gateway. **But** a compromised wrapper can still:
+
+- **Mislead the agent calling it** — return fake success on a request
+  the gateway rejected, swallow error output, or print misleading
+  diagnostics that hide a failed operation.
+- **Smuggle data into request bodies** — append attacker-chosen
+  fields the gateway happens to forward verbatim (e.g. PR body text,
+  issue comments, commit messages) to an external system.
+- **Re-route to a different gateway endpoint** that has a more
+  permissive policy than the one the wrapper's name implies (e.g.
+  `gh` quietly POSTing to a `/jira/` route).
+- **Exfiltrate session-scoped state** the wrapper has access to
+  (`EGG_SESSION_TOKEN`, environment, stdin) by including it in a
+  request the gateway *would* allow.
+
+The role-level write filter does **not** block writes under
+`sandbox/scripts/` — the credential-routing invariant is enforced by
+this lens, not by `patterns.py`. Treat any diff that touches
+`sandbox/scripts/*` as a trust-boundary change.
+
+Verification recipe:
+
+1. Enumerate every changed file under `sandbox/scripts/`. For each,
+   confirm it is a thin bash/POSIX wrapper that POSTs to a
+   `/api/v1/<service>/*` gateway route — no inline secrets, no calls
+   to the real `gh`/`git`/`jira` binaries, no network calls outside
+   the gateway URL, no writes outside the wrapper's documented stdout.
+2. Confirm the wrapper's gateway route matches its name (a wrapper
+   named `gh` POSTs to `/api/v1/github/*`, not `/api/v1/jira/*`).
+3. Confirm output handling is faithful: a non-2xx gateway response
+   surfaces as a non-zero exit and an error on stderr; the wrapper
+   does not silently swallow errors or fabricate success output.
+4. For NEW wrappers, confirm a corresponding gateway route exists
+   (or is added by the same diff) and is itself reviewed for policy
+   correctness — a permissive new route is a real security finding
+   even if the wrapper looks innocuous.
+
+Any deviation from the documented wrapper shape is a **mandatory NACK**
+— do not silently approve a credential-shim diff that fails the recipe
+above. Note that the security lens is advisory today (see *Scope* at the
+top of this file), so the NACK is recorded as a finding on the approval
+matrix rather than deadlocking consensus; promotion to a critical
+(deadlock-capable) reviewer for `sandbox/scripts/*` diffs is tracked in
+[#1997](https://github.com/jwbron/egg/issues/1997).
+
+### 6. Secret leakage
 
 Any new code path that may emit secrets, tokens, credentials, or
 identity-bearing tokens to:
@@ -108,7 +160,7 @@ Pay special attention to redaction-bypass patterns: a regex that
 redacts `password=...` but not `passwd=`; a sanitiser applied to one
 log channel but not another.
 
-### 6. Cross-file OWASP top-10 patterns
+### 7. Cross-file OWASP top-10 patterns
 
 OWASP top-10 patterns where the source and the sink live in **different
 changed files**. Common shapes:
@@ -134,7 +186,8 @@ flag them.
    the reach, you have not found the bug.
 3. Trust-boundary changes get extra scrutiny: any new public endpoint,
    any change to a decorator stack, any new file in `gateway/` or
-   `auth/`, any change to allowlists or regex patterns.
+   `auth/`, any change to allowlists or regex patterns, and any change
+   under `sandbox/scripts/` (the credential-shim wrappers).
 4. Cross-reference [`code-review-criteria.md`](./code-review-criteria.md)
    for the base review rules — your verdict format, severity classification,
    and BRC ACK/NACK lifecycle inherit from there.

shared/tests/test_egg_restrictions.py

@@ -216,13 +216,15 @@ def test_allows_sandbox_egg_script(self):
     def test_allows_sandbox_bin_egg_health_inspect(self):
         assert CODER_PATTERNS.can_write("sandbox/bin/egg-health-inspect")
 
-    def test_blocks_sandbox_scripts_gh_shim(self):
-        """sandbox/scripts/ is blocked (credential-routing invariant)."""
-        assert not CODER_PATTERNS.can_write("sandbox/scripts/gh")
-
-    def test_blocks_sandbox_scripts_git_credential_helper(self):
-        """sandbox/scripts/ is blocked (credential-routing invariant)."""
-        assert not CODER_PATTERNS.can_write("sandbox/scripts/git-credential-github-token")
+    def test_allows_sandbox_scripts_gh_shim(self):
+        """sandbox/scripts/ is writable; the gateway is the sole egress
+        chokepoint, so credential-shim modifications are reviewed by
+        reviewer_security rather than blocked at the role-pattern layer."""
+        assert CODER_PATTERNS.can_write("sandbox/scripts/gh")
+
+    def test_allows_sandbox_scripts_git_credential_helper(self):
+        """sandbox/scripts/ is writable; see test_allows_sandbox_scripts_gh_shim."""
+        assert CODER_PATTERNS.can_write("sandbox/scripts/git-credential-github-token")
 
     def test_blocks_github_workflows(self):
         """.github/ is blocked (branch-protection invariant)."""