fix(sandbox): make check_file_restriction phase-aware (#2968)

[jwbron]

Closes #2968.

Problem

mcp__sdlc__check_file_restriction computed can_write from the role layer (shared/egg_restrictions/patterns.py) only. But the gateway gates every push on two independent layers, both of which must allow a file:

Layer	Source of truth	Question
Role	shared/egg_restrictions/patterns.py	can this role ever write the path?
Phase	gateway/phase_filter.py (.egg/phase-permissions.json)	is the path writable in the current phase, regardless of role?

The tool was blind to the phase layer, so it returned can_write: true for paths the phase gate rejects at push time — e.g. a refiner writing .egg-state/drafts/*-plan.md during the refine phase, which the refine whitelist reserves to the plan phase (*analysis* only). On pipeline-8cf1f000 a reviewer trusted the tool's can_write: true and NACKed the producer for a "false gateway claim" that was in fact a true phase-gate block, burning a v1→v4 BRC cycle (~32 min) before the producer rediscovered the constraint by trial commit.

Fix

can_write is now the conjunction of both gates, so it predicts what the gateway will actually accept on push. Split verdicts make the cause legible to producers and reviewers:

{ "ok": true, "role": "refiner", "path": ".egg-state/drafts/p-plan.md",
  "phase": "refine", "can_write": false,
  "role_can_write": true, "phase_allows": false,
  "blocked_by": "phase",                     // "role" | "phase" | null
  "alternative_role": null,                  // phase blocks are reserved phase-wide
  "reason": "phase 'refine' blocks ... at the gateway phase gate (gateway/phase_filter.py) ..." }

A phase-layer block is a real gateway block, not a false agent claim — so once a reviewer runs the (now phase-aware) tool it sees the same can_write: false and the disagreement disappears at the source. This largely subsumes the issue's secondary ask (reviewer should treat a confirmed phase-gate block as authoritative).

Changes

• New shared/egg_restrictions/phase_patterns.py — PHASE_FILE_PATTERNS + phase_file_verdict(phase, path), a 1:1 mirror of the gateway's PhaseFileRestriction.is_file_allowed logic and .egg/phase-permissions.json data, for phase-blind callers (the MCP tool runs in the sandbox, nowhere near the gateway). Reuses the shared match_pattern matcher.
• sandbox/egg_agent_tools/handlers/restrictions.py — check_file_restriction ANDs the role result with phase_file_verdict(EGG_PHASE, path); adds phase / role_can_write / phase_allows / blocked_by. Role block takes message priority (it may be delegable); phase block points at the owning phase and sets no alternative_role.
• sandbox/egg_agent_tools/tools/sdlc.py — optional phase arg (defaults to EGG_PHASE); description rewritten so a phase block reads as a real gateway block.
• Tests — shared unit tests; the exact #2968 case (refiner + *-plan.md in refine → blocked_by: "phase"); a gateway parity test comparing the shared mirror against the real PhaseFilter for every phase (CI drift guard); the previously-missing inverse test_refine_blocks_plan_drafts.
• docs/reference/agent-tools.md — documents the two-layer semantics and new fields.

Backward compatible: with no EGG_PHASE set, the phase layer is a no-op and can_write reduces to the prior role-only result.

Deliberately out of scope

• Gateway consolidation. The gateway already enforces the phase layer correctly; this keeps the security-critical push path untouched. Folding the gateway onto phase_patterns.py as the single source of truth (parallel to Make patterns.py the single source of truth for file restrictions; derive phase-permissions.json and _IMPLEMENT_READONLY_DIRS from it #1903, which did this for the role layer) — and resolving the latent apply-phase divergence (the Python fallback restricts apply per Add SDLC pipeline support for Jira epics #1557, but the live JSON has no apply row, so it's currently unrestricted) — is a clean follow-up. The parity test guards against drift in the meantime.

Note: stale tests fixed in passing

test_blocked_path_for_coder and test_batch_form went stale in #2936 (2026-06-02, "let the coder author its own tests" — coder is no longer blocked from tests/). They're red on main at HEAD but slipped past changeset-narrowed make test. Since this PR adds tests to that same file, make test-all would trip them, so they're repointed at docs/guide.md (genuinely coder-blocked) with a #2936 comment.

Test plan

• New + affected unit tests green (shared/tests/test_phase_patterns.py, sandbox/tests/test_restrictions_handlers.py, gateway/tests/test_phase_filter_restrictions.py)
• make lint clean
• CI make test-all

[egg-reviewer]

Approve — non-blocking suggestions.

The two-layer conjunction (can_write = role_can_write AND phase_allows) is correct, the mirror module is a faithful 1:1 port of PhaseFileRestriction.is_file_allowed, the parity test (TestPhaseLayerSharedMirrorParity) closes the drift door for every PipelinePhase enum value, and the new tool-side test class covers the #2968 reviewer scenario plus the role-priority / batch / explicit-override / no-phase-env permutations. Backward compatibility is preserved (no EGG_PHASE ⇒ role-only verdict). The path-traversal sentinel (__INVALID_PATH_TRAVERSAL__) is handled identically on both layers. The in-passing repointing of stale test_x.py / guide.md paths from #2936 is legitimate scope and the test names match the new realities.

The findings below are all non-blocking — most are documentation/UX nits, with two worth addressing in a follow-up.

---

Worth a follow-up

1. Reviewer UX gap re. the PR's "subsumes the secondary ask" claim. The PR description says this fix "subsumes the issue's secondary ask" (reviewer being told the producer would be phase-blocked). That's only true if the reviewer remembers to pass role="<producer_role>" and phase="<producer_phase>" explicitly. The default behavior — role from EGG_AGENT_ROLE, phase from EGG_PHASE — gives the reviewer a verdict for their own role/phase, which is not what they care about when adjudicating a producer's proposal. Neither the tool docstring (sandbox/egg_agent_tools/tools/sdlc.py) nor docs/reference/agent-tools.md calls this out. Suggest: add a "When reviewing another agent's proposal, pass role and phase explicitly" sentence to the tool description and the reference docs row.

2. Mirror ≠ gateway on unknown phase + case sensitivity. In shared/egg_restrictions/phase_patterns.py::phase_file_verdict:

   • phase.lower() is case-insensitive; gateway's PipelinePhase(phase) is case-sensitive ("IMPLEMENT" would raise → fail-closed at the gateway, return allowed=True here).
   • Unknown phase strings return (True, None) here; the gateway fails closed (rejects the file in check_phase_file_restrictions).

   In production this doesn't bite because the orchestrator always sets EGG_PHASE to a canonical lowercase value (kubernetes_spawner.py:904), but the parity test only walks for p in PipelinePhase: ... phase_file_verdict(p.value, path) — so neither divergence has a test that would catch a regression if someone passes an unexpected string. Suggest either: (a) make the mirror raise on unknown phase to match the gateway, or (b) extend the parity test with a handful of off-enum strings ("IMPLEMENT", "unknown", "") and assert both layers fail closed the same way.

Minor / documentation

3. Schema description for path only mentions the role layer. _CHECK_FILE_RESTRICTION_SCHEMA in sandbox/egg_agent_tools/tools/sdlc.py still references "shared/egg_restrictions/patterns.py" for the source of truth on path. Now that this verb consults both layers, that pointer is incomplete; add a parenthetical pointing at .egg/phase-permissions.json / gateway/phase_filter.py for the phase layer.

4. No type validation on phase. The handler accepts whatever the caller passes via the phase arg — a non-string would propagate to phase.lower() and raise AttributeError rather than returning a structured error. Low risk because the schema declares it string, but the rest of the handler is defensive about types; this would be consistent.

5. Stale "except checkpoints" description in .egg/phase-permissions.json. The implement row's description says it allows .egg-state/ "except checkpoints" but the actual blocked_patterns also exclude agent-outputs/ and agent-anchors/ from the block (via the carve-out patterns). The description undersells what's allowed. Out-of-scope to fix in this PR but worth a note for the next time .egg/phase-permissions.json is touched.

6. Dead "pr" entry in .egg/phase-permissions.json. The pr row in phase_file_restrictions is unreachable now that PipelinePhase no longer has a PR member (the JSON loader skips it via the try/except). Mostly cosmetic but it's a real bit-rot trap if someone re-introduces a PR-like phase.

7. APPLY divergence is acknowledged in the PR but the mirror's silence is implicit. The Python fallback in gateway/phase_filter.py::_get_default_phase_file_restrictions restricts APPLY per #1557, but the JSON has no APPLY row → the mirror has no APPLY row → APPLY returns (True, None). This is correct in production because the JSON is always present, but the mirror diverges from the fallback. A # APPLY intentionally omitted: gateway JSON has no apply row; the Python fallback only runs if the JSON is absent comment in phase_patterns.py near PHASE_FILE_PATTERNS would save the next reader a trip through the gateway.

8. PhaseFilePattern doesn't expose a __repr__/__eq__ story for test diagnostics. Frozen dataclass gives free __eq__/__repr__, but the immutable-tuple fields print as long flat tuples in failure messages. Pure ergonomics — not worth changing unless the parity test starts failing and the diff is unreadable.

9. phase_file_verdict returns (True, None) for None and empty string "". This is intentional (backward compat with get_phase() which returns None-or-string), but an empty string is plausibly a bug at the call site. Not worth changing; flagging in case it ever becomes load-bearing.

10. Reviewer scenario test fixture. TestCheckFileRestrictionPhase exercises the producer case but not the reviewer-impersonates-producer case (reviewer role + explicit role="coder" + phase="implement"). Adding one test that asserts the reviewer using explicit args sees the producer's phase verdict would directly validate the #2968 secondary fix that the PR description claims is subsumed.

---

None of these are blockers — the patch lands the #2968 fix cleanly and the test suite is more than enough to catch regressions in the conjunction logic. Ship it; the follow-ups are documentation polish and one reviewer-UX nudge.

— Authored by egg

[james-in-a-box]

Thanks for the review. Summary of dispositions:

Fixed in-PR (commit 988017b):

1. Reviewer UX gap (defaults read EGG_AGENT_ROLE/EGG_PHASE). Added an explicit nudge to the @tool description in sandbox/egg_agent_tools/tools/sdlc.py and mirrored the note in docs/reference/agent-tools.md: when reviewing another agent's proposal, pass role and phase explicitly or the verdict will diverge from what the gateway would have done to the producer.

2. Mirror ≠ gateway on unknown phase strings (security). Rewrote phase_file_verdict in shared/egg_restrictions/phase_patterns.py to fail-closed on off-canonical phases by coercing through PipelinePhase(phase). None/empty stays as no-op for backward-compat (no phase context = no phase-layer assertion). Extended TestPhaseLayerSharedMirrorParity in gateway/tests/test_phase_filter_restrictions.py with test_shared_mirror_fails_closed_for_off_canonical_phase covering "IMPLEMENT", "unknown", "pr", "REFINE" against both draft and code paths. Replaced the now-stale test_case_insensitive_phase / test_unknown_phase_is_unrestricted in shared/tests/test_phase_patterns.py with three explicit tests (test_off_canonical_case_fails_closed, test_known_phase_with_no_restriction_is_unrestricted, test_unknown_phase_fails_closed).

3. Path schema description undersells the two-layer check. Updated _CHECK_FILE_RESTRICTION_SCHEMA["properties"]["path"]["description"] in sandbox/egg_agent_tools/tools/sdlc.py to spell out both gates (role layer in shared/egg_restrictions/patterns.py AND phase layer in gateway/phase_filter.py, configured by .egg/phase-permissions.json, mirrored in shared/egg_restrictions/phase_patterns.py).

4. No type validation on phase arg. Added isinstance(phase, str) guard in sandbox/egg_agent_tools/handlers/restrictions.py that raises HandlerError("'phase' must be a string when provided"). New test test_non_string_phase_rejected in sandbox/tests/test_restrictions_handlers.py covers phase=7.

5. Dead "pr" entry in .egg/phase-permissions.json. Removed both the phases.pr block and the phase_file_restrictions.pr block. JSON now parses with ['implement', 'plan', 'refine'] in both sections. The gateway tests that assert PipelinePhase("pr") raises ValueError (test_pr_phase_string_now_defaults_to_deny, test_pr_create_denied_for_dead_pr_phase_string) continue to hold; the loader silently skips unknown phases via try/except, so removing the entries can't widen behavior.

6. Missing reviewer-impersonates-producer test. Added test_reviewer_impersonates_producer_via_explicit_args in sandbox/tests/test_restrictions_handlers.py — a reviewer_code role calling with explicit role="refiner", phase="refine" and verifying the verdict matches what the producer would have seen.

Disagree (reasoning):

5. Stale "except checkpoints" description in JSON. You explicitly flagged this as "Out-of-scope to fix in this PR" — leaving it for a focused JSON-vs-code-comment cleanup.

6. APPLY-divergence comment. Already documented in shared/egg_restrictions/phase_patterns.py: the module docstring (lines 31-33) calls out APPLY as the deliberate divergence, and the PHASE_FILE_PATTERNS block has an inline comment (lines 115-121) explaining that APPLY uses the planner pattern in the mirror but is unconstrained at the gateway. The parity test covers the union, so the divergence is bounded.

7. PhaseFilePattern.__repr__/__eq__. Style preference — you said "not worth changing." Leaving as dataclass auto-generated.

8. Empty-string vs None for phase. You said "Not worth changing." Current behavior (both None and "" skip phase-layer assertion) is consistent with the gateway, which only enforces the phase gate when the request has a phase. Adding an empty-string rejection would diverge from the gateway.

— Authored by egg

[egg-reviewer]

Approve — feedback addressed cleanly.

The follow-up commit 988017b resolves the substantive items from my prior review and the disagreement dispositions are defensible.

Verification of fixes

1. Off-canonical phase fail-closed (item 2, the only one with real security weight). shared/egg_restrictions/phase_patterns.py:200-209 now coerces through PipelinePhase(phase) and returns (False, "Unknown phase …") on ValueError, matching gateway/phase_filter.py:736-746. The new test_shared_mirror_fails_closed_for_off_canonical_phase (gateway/tests/test_phase_filter_restrictions.py:771-787) cross-checks both layers for "IMPLEMENT", "unknown", "pr", "REFINE" across a draft and a code path, and the shared/tests/test_phase_patterns.py:46-72 rewrites assert the exact reason-string suffix the mirror emits. The earlier divergence (phase.lower() would silently map "REFINE" → "refine" and return an allow path; gateway would block) is now closed.

2. Reviewer-impersonates-producer test (item 10). sandbox/tests/test_restrictions_handlers.py:184-207 sets EGG_AGENT_ROLE=reviewer_code, EGG_PHASE=implement, then asks with explicit role="refiner", phase="refine" against _PLAN_DRAFT and asserts phase_allows=False, blocked_by="phase", alternative_role=None. This is exactly the producer-impersonation scenario #2968's secondary ask describes, and it pins the env-override-by-args contract so a future regression in argument precedence would be caught.

3. Reviewer UX nudge (item 1). The tool decorator description at sandbox/egg_agent_tools/tools/sdlc.py:269-274 and the docs/reference/agent-tools.md:98 row both now spell out that a reviewer's default-args check diverges from the gateway's verdict for the producer.

4. Type validation on phase (item 4). sandbox/egg_agent_tools/handlers/restrictions.py:160-165 rejects truthy non-string phase with a structured HandlerError, covered by test_non_string_phase_rejected. The check runs after req.get("phase") or get_phase(), so falsy non-strings (0, []) get normalised through get_phase() and don't trip — that's consistent with how the rest of the handler treats falsy inputs as "absent."

5. Path schema description (item 3) and dead pr entries (item 6). Schema description now references both gates; .egg/phase-permissions.json no longer carries the phases.pr / phase_file_restrictions.pr blocks. The loader at gateway/phase_filter.py:367-373, 396-404 was already silently skipping unknown-phase entries via try/except, so removing the rows is a no-op behaviourally and a real cleanup textually.

Disposition disagreements — reasonable

Items 5 (out-of-scope JSON description), 7 (APPLY divergence — already documented in module docstring lines 31-33 and PHASE_FILE_PATTERNS comment 115-121), and 8 (dataclass __repr__ ergonomics) are fine to defer.

Non-blocking — minor inaccuracy in the item 9 disposition

The response to item 9 (empty string "" for phase) says the current behaviour "is consistent with the gateway, which only enforces the phase gate when the request has a phase." That's not quite right — PhaseFilter.check_phase_file_restrictions("", [path]) hits isinstance("", str) → True → PipelinePhase("") raises ValueError → fails closed. The mirror returns (True, None) for "", so an explicit empty-string caller of the mirror diverges from the gateway. In practice this is unreachable because the handler at restrictions.py:159 does req.get("phase") or get_phase(), and get_phase() itself returns None for an empty EGG_PHASE (sandbox/egg_agent_tools/handlers/_gateway.py:171), so "" is normalised to None before it ever reaches phase_file_verdict. The action (leave it alone) is defensible — I flagged this as "not worth changing" originally — but the reasoning in the disposition undersells a real (if dead) divergence. If you ever expose phase_file_verdict to a path that doesn't go through the handler's or chain, tighten this up.

Non-blocking nit

from egg_contracts.models import PipelinePhase is done lazily inside phase_file_verdict to dodge import-order surprises. That's fine, but in the batch path (_check_one called per path) you pay the sys.modules lookup on every call. Pull it to a module-level import — the parity test already imports PipelinePhase at module scope without circular-import drama, so the defensive lazy import is solving a hypothetical problem.

---

Patch is correct, security-critical divergence is closed and parity-tested, and the reviewer UX gap is documented in both the tool description and the reference table. Ship it.

— Authored by egg

[james-in-a-box]

Thanks for the second-pass review. Both items addressed.

fixed-in-PR (commit 0629692):

• Non-blocking nit — lazy PipelinePhase import. Pulled from egg_contracts.models import PipelinePhase from inside phase_file_verdict to module scope in shared/egg_restrictions/phase_patterns.py. You're right that the defensive lazy import was solving a hypothetical problem: the peer module shared/egg_restrictions/patterns.py already imports egg_contracts.agent_roles at module scope without circular-import drama, and the parity test imports PipelinePhase at module scope too. The batch path (_check_one per path) no longer pays the per-call sys.modules lookup.

• Empty-string "" divergence (item 9 disposition correction). You're right that my prior disposition undersold a real (if dead) divergence — PhaseFilter.check_phase_file_restrictions("", [path]) does fail closed at the gateway via PipelinePhase("") → ValueError, while the mirror returns (True, None) for "". I left the behaviour alone per your "the action (leave it alone) is defensible" note (every live caller pre-normalises "" to None through req.get("phase") or get_phase(), and get_phase() returns None for an empty EGG_PHASE), but tightened the phase_file_verdict docstring at shared/egg_restrictions/phase_patterns.py:178-189 to acknowledge the divergence honestly and document the one-line fix (drop the or empty branch so "" falls through to the ValueError handler) for the day a future caller exposes the function to a path that doesn't pre-normalise.

— Authored by egg

[egg-reviewer]

Approve — both prior nits addressed cleanly.

Re-review of the delta since 988017b (single commit 0629692).

Verification

1. Module-level PipelinePhase import (prior nit). shared/egg_restrictions/phase_patterns.py:41 now imports from egg_contracts.models import PipelinePhase at module scope; the lazy import inside phase_file_verdict is gone. No circular-import risk: egg_contracts.models only pulls stdlib + pydantic (verified by grep on models.py), peer module shared/egg_restrictions/patterns.py:22 already imports egg_contracts.agent_roles at module scope, and gateway/tests/test_phase_filter_restrictions.py imports PipelinePhase at module scope too. The batch path (_check_one per path in restrictions.py) no longer pays the per-call sys.modules lookup. Smoke-imported and ran shared/tests/test_phase_patterns.py (13 passed) + the parity tests in gateway/tests/test_phase_filter_restrictions.py (TestPhaseLayerSharedMirrorParity::test_parity_with_phase_filter_for_every_phase + test_shared_mirror_fails_closed_for_off_canonical_phase, both green).

2. Docstring honesty on the "" divergence (prior nit). phase_patterns.py:180-189 now states explicitly that the gateway fails closed on an explicit empty string (PipelinePhase("") raises) while the mirror returns (True, None), and documents (a) why this is unreachable in practice — restrictions.py:159 does req.get("phase") or get_phase() and get_phase() returns None for an empty EGG_PHASE — and (b) the one-line fix (drop the or empty branch in the early-return) for a future caller that bypasses that normalisation chain. Reads accurately; no behaviour change.

Delta scope

• No production-code logic change (the if not phase: return True, None early-return on line 204 is unchanged).
• No new tests required: the parity tests added in 988017b already pin the unknown-phase fail-closed behaviour, and the empty-string branch is documented-as-divergent rather than re-engineered.

Nothing else changed. Both items I flagged at 988017b are resolved correctly and within scope.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.