Fix #2066: collapse duplicate AgentRole into single source of truth

[jwbron]

Closes #2066.

shared/egg_restrictions/patterns.py defined a parallel AgentRole class that mirrored egg_contracts.agent_roles.AgentRole, with a doc comment asking readers to keep values in sync — enforced only by unit tests in tests/. PR #2061 (#1965) surfaced the failure mode: it added REVIEWER_SECURITY and REVIEWER_CONCURRENCY to the canonical enum but not the gateway-side mirror, and CI caught the drift on test_role_values_match_shared_library and test_all_canonical_roles_in_gateway.

(The issue framed the duplicate as living in gateway/agent_restrictions.py; that file actually re-exports AgentRole from egg_restrictions.patterns, so the real second copy is in shared/egg_restrictions/patterns.py. This PR fixes the actual file.)

Changes

• shared/egg_restrictions/patterns.py: drop the local class AgentRole; re-export the canonical StrEnum from egg_contracts.agent_roles so new roles propagate automatically.
• tests/gateway/test_agent_restrictions.py, tests/shared/egg_contracts/test_agent_roles.py: trim the role-value parity tests (now tautological) to a single identity assertion (assert AgentRole is SharedAgentRole) that trips if anyone reintroduces a parallel enum.

The chosen approach is option 1 from the issue. egg_restrictions and egg_contracts are co-installed in the same wheel and have no other dependency between them, so the new egg_restrictions → egg_contracts import direction is clean and introduces no cycle. All consumers of egg_restrictions.patterns.AgentRole use class-attribute access (AgentRole.CODER), which works identically for the StrEnum.

Test plan

• make test — 4 failures are all pre-existing flakes on main (3 event-loop config issues in tests/llm/claude/test_runner.py, 1 timing-flaky test_session_expires_exactly_at_boundary that passes in isolation). None touch AgentRole.
• Targeted run of tests/gateway/test_agent_restrictions.py, tests/shared/egg_contracts/test_agent_roles.py, shared/tests/test_egg_restrictions.py, gateway/tests/test_agent_restrictions_patterns.py, gateway/tests/test_agent_restrictions_gh.py — 529 passed, 1 skipped, plus 1 unrelated pre-existing failure (TestRoleSyncWithOrchestratorModels — bare-name from models import resolves only when orchestrator's conftest is loaded).

🤖 Generated with Claude Code

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflict with main:

File	Category	Resolution
shared/egg_restrictions/patterns.py	Semantic (clean)	Kept HEAD's re-export; deleted the local mirror class

Why this was clean

main (via #2061) added REVIEWER_SECURITY and REVIEWER_CONCURRENCY to the local AgentRole mirror class in shared/egg_restrictions/patterns.py. This PR deletes that mirror entirely and re-exports the canonical StrEnum from egg_contracts.agent_roles — which already carries both new roles. The two new roles still propagate, just through the canonical enum instead of the doomed copy.

Verification

• assert egg_restrictions.patterns.AgentRole is egg_contracts.agent_roles.AgentRole — passes
• AGENT_PATTERNS registry includes reviewer_security and reviewer_concurrency (already wired up in this PR's pattern definitions)
• Targeted run: tests/gateway/test_agent_restrictions.py, tests/shared/egg_contracts/test_agent_roles.py, shared/tests/test_egg_restrictions.py, gateway/tests/test_agent_restrictions_patterns.py, gateway/tests/test_agent_restrictions_gh.py → 530 passed, 1 skipped

Please review: Nothing surprising — the resolution preserves the PR's original intent exactly. The merge commit can be reverted cleanly if needed.

— Authored by egg

[egg-reviewer]

Review of PR #2082

Verified the change against the broader codebase. The PR cleanly collapses the duplicate AgentRole definition in shared/egg_restrictions/patterns.py into a re-export of the canonical egg_contracts.agent_roles.AgentRole StrEnum. The tripwire identity tests are well-targeted.

Verified safe

• No circular import: egg_contracts/agent_roles.py only imports from stdlib and .roles; egg_contracts/ has no references to egg_restrictions (grep -r egg_restrictions shared/egg_contracts returns nothing). The new egg_restrictions → egg_contracts edge is one-way.
• Class→StrEnum is semantically transparent for every existing call site I traced:
  • AGENT_PATTERNS.get(role.lower()) in shared/egg_restrictions/checker.py:25 — works because StrEnum members hash equal to their str values.
  • AGENT_GH_RESTRICTIONS.get(role_lower) in gateway/agent_restrictions.py:209 — same.
  • f"Agent role '{role}'..." strings in gateway/agent_restrictions.py:163,191,215 — Python 3.11+ StrEnum.__str__ returns the value, so error messages stay identical.
  • tool_interceptor.py:121 iteration over AGENT_PATTERNS.items() and the surrounding f"...'{owner_role}'..." — same.
  • Confirmed empirically: f"{AgentRole.CODER}" == "coder", isinstance(AgentRole.CODER, str) is True, hash(AgentRole.CODER) == hash("coder"), dict lookup with raw strings works.
• Gateway runtime: gateway/Dockerfile:71-77 already copies shared/egg_contracts/ into /app/ and pip installs pydantic. The new import works in the gateway image.
• Sandbox runtime: sandbox/Dockerfile ships egg_contracts via COPY . /opt/egg-runtime/ + PYTHONPATH=/opt/egg-runtime/shared, and pydantic is installed in stage 2. Fine.
• No remaining duplicates: grep -rn '^class AgentRole' shared orchestrator gateway shows only the canonical class in egg_contracts/agent_roles.py (models.py:460 is AgentRoleType, unrelated). The two tripwires are sufficient because there are only two re-export sites.
• Pre-existing failure called out in the test plan reproduces: TestRoleSyncWithOrchestratorModels.test_all_canonical_roles_in_orchestrator_models fails with ModuleNotFoundError: No module named 'models' — bare-name from models import only resolves when orchestrator's conftest is loaded. Unrelated to this PR.

Non-blocking observations

1. shared/pyproject.toml (egg-shared distribution) doesn't declare egg-contracts as a dependency. That file's dependencies = ["pyyaml>=6.0", "anthropic>=0.50,<1.0", "httpx>=0.25.0", "markdownify>=0.13.1"] does not include egg-contracts, but egg_restrictions.patterns now imports from it. After this PR, anyone who pip installs egg-shared standalone gets ImportError on first import of egg_restrictions.patterns. The monorepo install path (uv sync against root pyproject.toml, which lists both shared/egg_contracts and shared/egg_restrictions in tool.hatch.build.targets.wheel.packages) is unaffected, and I can't find any consumer that installs egg-shared in isolation — so this is latent rather than active. Consider either adding egg-contracts to the egg-shared deps for hygiene, or adding a comment in shared/pyproject.toml noting that egg_restrictions requires egg_contracts to be co-installed.

2. Import surface widens: from egg_contracts.agent_roles import AgentRole triggers egg_contracts/__init__.py, which eagerly imports pydantic, agent_recovery, audit, dependency_graph, feedback, hitl, loader, models, orchestration, orchestrator, phase_defaults, plan_parser, resilience, roles, validator. Every gateway and sandbox process that touches egg_restrictions.patterns now pays this cost on cold start. Not a correctness issue (those processes already import egg_contracts), but worth flagging if startup latency matters anywhere it didn't before. A from egg_contracts.agent_roles import AgentRole written as a deferred / lazy import would not avoid this — the package __init__ runs on the first sub-module import regardless. The clean fix is unrelated to this PR.

3. Tripwire scope: the assert AgentRole is SharedAgentRole identity check is good against re-introduced parallel classes, but does not catch a subtler regression where someone replaces the re-export with AgentRole = SharedAgentRole.__class__(...) or a Literal alias. That's a paranoid case; current tests are appropriate for the threat model.

Approving

The PR delivers exactly what #2066 asked for, with the right tests. Pre-existing failures called out in the PR body are not regressions. CI is green.

— Authored by egg

[james-in-a-box]

Thanks for the thorough verification. Addressing the observations:

1. shared/pyproject.toml missing egg-contracts dep — fixed in 101f39e. Added "egg-contracts" to the egg-shared dependencies list. Monorepo install is unaffected; this just makes the standalone-install failure mode loud at install time instead of silent until first import.

2. Wider import surface from egg_contracts/__init__.py — leaving as-is. As you noted, the gateway and sandbox processes already import egg_contracts, and a deferred import wouldn't help (package __init__ runs on first sub-module import regardless). The clean fix is in egg_contracts/__init__.py, which is out of scope here.

3. Tripwire scope — leaving as-is. Agreed it's appropriate for the threat model; the paranoid Literal-alias case isn't worth the test churn.

— Authored by egg

[egg-reviewer]

Re-review of PR #2082

Verified the delta since 7d0c45c — exactly one file changed:

shared/pyproject.toml | 1 +/-1

The commit (101f39e) adds "egg-contracts" to the egg-shared dependencies list, addressing non-blocking observation #1 from my previous review. CI is green.

Verification

• Diff is minimal: gh api repos/jwbron/egg/compare/7d0c45c...101f39e --jq '.files[] | .patch' confirms a single addition to the dependencies array. No Python code changed; the AgentRole consolidation work from the prior review is untouched.
• Dep declaration is correct: shared/egg_contracts/pyproject.toml:2 declares the distribution name as egg-contracts, matching the new entry.
• Monorepo install unaffected: Root pyproject.toml packages shared/egg_contracts directly via tool.hatch.build.targets.wheel.packages. Sandbox and gateway Dockerfiles COPY the module rather than installing through shared/pyproject.toml. Confirmed no install path exercises shared/pyproject.toml in isolation (grep -rn "pip install.*shared" finds nothing).
• Author's framing is accurate: This makes the standalone-install failure mode loud at install time instead of silent at first import. The shared/pyproject.toml file is latent in current usage, so this is hygiene rather than a live regression — exactly as the commit message states.
• Other two observations correctly deferred: The eager-import-surface point (#2) is genuinely out of scope (the fix lives in egg_contracts/__init__.py). The tripwire-scope point (#3) is the paranoid case I flagged as appropriate-for-threat-model in the original review. Both deferrals are well-reasoned.

Non-blocking observation (carried-forward, not introduced by this commit)

The new dep entry has no version constraint ("egg-contracts"), unlike its siblings (pyyaml>=6.0, anthropic>=0.50,<1.0, etc.). And there is no [tool.uv.sources] or workspace mapping pointing egg-contracts at shared/egg_contracts/ — egg-contracts is not on PyPI either. So if a hypothetical consumer ever did pip install egg-shared standalone, they would now get No matching distribution found for egg-contracts instead of a deferred ImportError. That's still broken — just broken differently. Since no consumer does that today, this remains latent. Mentioning for the record; not blocking.

Approving

The fix is minimal, targeted, and correctly addresses observation #1. Previous approval still stands.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Acknowledged. The carried-forward observation is correct — 101f39e traded a deferred ImportError for a No matching distribution found failure at install time, since egg-contracts isn't on PyPI and shared/pyproject.toml has no [tool.uv.sources] workspace mapping. Both states are broken for the standalone-install path, just at different points.

Leaving as-is in this PR for two reasons:

1. Out of scope. Properly resolving this requires either publishing egg-contracts to a registry or introducing [tool.uv.sources] / workspace plumbing at the monorepo level (pyproject.toml's root currently uses hatch packages = [...] paths with no [tool.uv] workspace setup). Either is a larger change than gateway/agent_restrictions.py AgentRole enum drifts silently from shared/egg_contracts AgentRole #2066 asked for.

2. Still latent. As both reviews confirmed, no consumer installs egg-shared standalone today (sandbox/gateway Dockerfiles COPY the module; the monorepo install goes through the root pyproject.toml). The change is hygiene-direction-of-travel rather than a live regression.

Filing this as a follow-up rather than expanding scope here. Thanks for the careful re-review.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

5 previous review(s) hidden.