Add file access escape hatch and role-agnostic enforcement

[james-in-a-box]

Summary

Adds the documented egg-orch request-file escape hatch so agents blocked by
file restrictions can request human approval via HITL decisions. Also makes
agent-role enforcement apply to all roles in pipeline sessions, not just coders.

This addresses review items #4 and #6 from PR #939:

• Escape hatch: egg-orch request-file create --path <file> --reason <why> creates a HITL decision; once approved, the file bypasses restrictions on subsequent pushes
• Role-agnostic enforcement: Pipeline sessions always enforce agent-role restrictions for all roles (coder, tester, documenter, integrator) without requiring EGG_AGENT_RESTRICTIONS_ENFORCE=true

Changes

• Session: file_exceptions field + add_file_exception() method for human-approved bypasses
• FileRequestManager: In-memory store for pending file access requests
• Gateway: POST/GET /sessions/request-file endpoints with orchestrator HITL integration
• Push handler: Excepted files filtered before both agent-role and phase restriction checks
• Post-agent commit: Respects file exceptions during auto-commit filtering
• Enforcement: enforce = bool(pipeline_id) or enforce_env — pipeline sessions always enforce
• CLI: egg-orch request-file create/status subcommands
• Docs: Updated orchestrator.md and environment.md with escape hatch instructions

Test plan

1. pytest gateway/tests/test_file_request.py — 28 new tests pass
2. pytest gateway/tests/test_agent_restrictions_enforce.py — 12 tests pass (4 new for pipeline enforcement)
3. pytest gateway/tests/ — full suite: 1637 passed, 5 skipped, 0 failures
4. make lint — passes cleanly

Authored-by: egg

[egg-reviewer]

No agent-mode design concerns.

This PR aligns well with the core principle ("the sandbox is the constraint"). The file access escape hatch correctly keeps enforcement in the gateway sidecar while providing agents a proper HITL mechanism to request exceptions — no prompt-level security, no pre-fetching, no post-processing pipelines.

— Authored by egg

[egg-reviewer]

Review: File Access Escape Hatch & Role-Agnostic Enforcement

Thorough review of all 10 changed files. The feature design is sound — HITL-gated file exceptions for agents blocked by restrictions is the right approach. Several issues need addressing before merge.

---

1. [Security] No path validation on file_path at the request endpoint

File: gateway/gateway.py — request_file_create() (around line 3990)

The file_path from the request body is .strip()-ed but never validated for path traversal before being stored and used. While check_phase_file_restrictions and check_agent_restrictions internally call _normalize_path() (which rejects ../ and absolute paths), the unvalidated path is:

1. Stored in FileAccessRequest in memory
2. Interpolated into the orchestrator decision question/context strings
3. Added verbatim to session.file_exceptions on approval

If a path like foo/../../../etc/passwd normalizes differently between the "is it blocked?" check and the push-time exception matching, this creates a bypass vector. The restriction check normalizes the path internally and may report it as blocked (or not) based on the normalized form, but the raw form is what gets stored in file_exceptions. At push time, the exception matching uses raw string equality (f in file_exceptions), so the paths must match exactly.

Fix: Normalize file_path at the top of request_file_create() using the same normalization the restriction checks use, and reject invalid paths early:

import posixpath
normalized = posixpath.normpath(file_path)
if normalized.startswith("../") or normalized.startswith("/"):
    return make_error("Invalid file path", status_code=400)
file_path = normalized

2. [Security] No validation on pipeline_id in URL construction

File: gateway/gateway.py — _orch_create_decision() and _orch_get_decision()

Both functions interpolate pipeline_id directly into URLs:

url = f"{orchestrator_url}/api/v1/pipelines/{pipeline_id}/decisions"

The pipeline_id comes from session.pipeline_id which is set during session creation by the orchestrator, so the attack surface is limited. However, this is still URL injection-vulnerable — a pipeline_id containing / or ? characters would alter the request path or query string. Add urllib.parse.quote(pipeline_id, safe='') or validate the format.

3. [Correctness] File exceptions bypass check_file_restrictions (role-based) but NOT check_agent_restrictions

File: gateway/gateway.py — push handler

The first file_exceptions block (line 720) filters changed_files before check_file_restrictions() at line 739 — correct. But the filtered changed_files list is then passed to check_agent_restrictions() at line 771, which is also correct because both checks operate on the same reduced list.

However, there's a subtle issue: the first check_file_restrictions (line 739) is a role-based restriction (e.g., implementer blocked from contract files), while check_agent_restrictions (line 771) is agent-scope restrictions (coder vs tester file scopes). These are semantically different restriction types. The file exception was granted because the file was blocked by one of these checks, but the exception silently bypasses both. This is probably intentional behavior, but it should be documented explicitly — a human approving an exception for a phase-blocked file also implicitly grants an agent-role exception for the same file.

4. [Correctness] Race condition in resolve_request — logging outside lock

File: gateway/file_request_manager.py — resolve_request() (lines 90-103)

with self._lock:
    req = self._requests.get(request_id)
    if not req or req.status != "pending":
        return False
    req.status = "approved" if approved else "denied"

logger.info(  # Outside the lock — reads req.status
    "File access request resolved",
    ...
    status=req.status,
    file_path=req.file_path,
)

After the lock is released, another thread could theoretically read req between the status update and the log call. In practice this is benign because resolve_request only transitions from "pending" to terminal state and the idempotency check prevents double-resolution. But the pattern is fragile — move the log inside the lock or capture the status value before releasing.

5. [Robustness] In-memory FileRequestManager state lost on gateway restart

File: gateway/file_request_manager.py

The FileRequestManager stores all requests in memory with a non-persistent sequential counter. On gateway restart:

• All pending requests are lost
• The counter resets to 0, producing duplicate request IDs
• If a human approves a decision in the orchestrator after restart, the gateway's status-poll endpoint will return 404 for the original request_id
• The session's file_exceptions list IS persisted (via SessionManager._save_to_disk()), so already-approved exceptions survive restart. But pending requests in flight are silently dropped.

This is acceptable for an initial implementation if documented. The agent can re-request after restart. But the duplicate request IDs across restarts could cause confusion in logs. Consider using a UUID or timestamp-based ID instead of a sequential counter.

6. [Correctness] Second file_exceptions block only runs in changed_files is None path

File: gateway/gateway.py — lines 855-875

The second file_exceptions block (for the phase restriction path) is inside the if changed_files is None: branch (line 831). This means it only executes when session_role was falsy (so the first code path that fetches changed_files was skipped). This is correct — if session_role was set, changed_files was already fetched and exceptions already applied at line 720.

However, when session_role IS set, the first exception block (line 720) removes excepted files before check_file_restrictions and check_agent_restrictions, but the same already-reduced changed_files is then passed to check_phase_file_restrictions at line 878. So phase restrictions also operate on the reduced list, which is correct. No issue here, just confirming the logic.

7. [Testing] No test for denied status polling

File: gateway/tests/test_file_request.py

TestRequestFileStatus has tests for not_found, pending_status, and approved_status, but no test for the denied case (where resolution is "Deny"). The deny path in request_file_status() does different things (no add_file_exception call, different audit event). Add a test that verifies:

• Status returns "denied"
• add_file_exception is NOT called
• Audit log records denial

8. [Testing] No test for session mismatch on status endpoint

File: gateway/tests/test_file_request.py

The request_file_status() endpoint checks req_obj.session_token_hash != session.session_token_hash and returns 404 if mismatched (line 4113). There's no test verifying that a session cannot poll another session's request. This is a security-relevant authorization check that should have test coverage.

9. [Minor] _orch_get_decision fetches ALL decisions to find one

File: gateway/gateway.py — _orch_get_decision() (lines 3940-3965)

This function fetches all decisions for a pipeline and iterates to find the matching one:

decisions = data.get("data", {}).get("decisions", [])
for d in decisions:
    did = d.get("id", d.get("decision_id"))
    if did == decision_id:
        ...

If the orchestrator has a GET /decisions/{id} endpoint, use that instead. If not, this is fine for now but will degrade with many decisions per pipeline.

10. [Minor] Import style inconsistency

File: gateway/gateway.py — request_file_create() and request_file_status()

Both endpoints use a try/except pattern for imports:

try:
    from file_request_manager import get_file_request_manager
except ImportError:
    from gateway.file_request_manager import get_file_request_manager

This dual-import pattern exists elsewhere in the codebase for the test vs production module loading path, so it's consistent. No action needed — noting for context.

---

Summary

Must fix before merge:

• #1 (path validation on file_path)
• #7 (denied status test)
• #8 (session mismatch test)

Should fix:

• #2 (pipeline_id URL injection)
• #4 (logging outside lock)
• #5 (document restart behavior, use UUIDs)

Non-blocking:

• #3, #6 (logic is correct, documentation suggestion)
• #9, #10 (minor improvements)

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

1 previous review(s) hidden.