Fix #2528: per-repo role-pattern overrides for non-Python conventions - #2561
Conversation
Adds a ``role_patterns:`` block under ``repo_settings`` in
``repositories.yaml`` so target repos can declare their test/code/docs
file conventions:
repo_settings:
owner/example-go-repo:
role_patterns:
tests_globs: ["**/*_test.go", "**/testdata/**"]
code_globs: ["**/*.go"]
docs_globs: ["**/*.md", "docs/"]
Refactors ``shared/egg_restrictions/patterns.py`` to extract the
language-specific globs (``DEFAULT_TESTS_GLOBS`` / ``DEFAULT_CODE_GLOBS``
/ ``DEFAULT_DOCS_GLOBS``) and slot them into per-role builders. Adds
``build_agent_patterns(repo)`` and ``get_agent_pattern_for_repo(role,
repo)`` with per-repo memoization invalidated by the existing
``reload_config`` SIGHUP path.
Threads ``repo`` through the four consumers that have it in scope:
- ``gateway/agent_restrictions.py::partition_files_by_role`` — push-time
enforcement
- ``shared/egg_agent/tool_interceptor.py`` — sandbox in-agent pre-check
(reads ``EGG_PIPELINE_REPO`` env var)
- ``shared/egg_contracts/plan_parser.py::validate_task_role_alignment``
(#2527) — plan-time validation now mirrors push-time on non-Python
repos
- ``orchestrator/routes/pipelines.py::_build_role_restrictions_section``
— planner prompt rendering reflects the repo's actual boundaries
Security: only language-convention glob lists are configurable. The
hard blocks on ``.egg-state/contracts/``, ``.github/``, and the other
pipeline-state directories are sourced from the per-role builders and
cannot be relaxed by a target repo's config. Unknown keys are silently
dropped at the validation layer in ``config/repo_config.py``; defense
in depth in the builders.
Scope: per the #2530 audit (the conftest line + test/code split are
correct as-is) and the #2528 issue comment, this ships only the narrow
knobs. The per-task ``includes_tests`` opt-in and any future per-repo
``pair_rule`` are deliberately out of scope; ``role_patterns`` is
schema-additive so a future ``role_patterns.pair_rule`` key slots in
without breaking the current schema.
Default-repo behaviour is unchanged: callers without a ``repo`` in
scope keep using ``AGENT_PATTERNS``, which is identical to
``build_agent_patterns(None)``. New regression tests in
``gateway/tests/test_per_repo_role_patterns.py`` cover the
default-registry parity, JS-convention and Go-convention overrides,
the security-blocklist guarantees, the YAML validation, and the cache
invalidation path.
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Test/Unit Tests": 2, "Lint/Python": 2} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The tester pattern was incorrectly granting write access to all source files via *code_globs (**/*.py, **/*.go, etc.) in allowed_patterns. This contradicted the legacy behavior and the existing tester-scope tests (tester is bounded to test files; source edits are the coder's or autofixer's job). Drop *code_globs from _build_tester_pattern allow-list. The code_globs parameter is kept for signature parity with the other builders so build_agent_patterns can keep passing it uniformly.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review of PR #2561 — per-repo role-pattern overrides
I traced the data flow of the per-repo override end-to-end against the gateway, orchestrator, and sandbox runtime environments. The PR is internally consistent and the unit tests pass, but the feature does not actually work in any production K8s deployment. The override file is unreachable at every call site that this PR threads repo through. Tests pass only because they run from the repo root, where the import path the PR uses happens to resolve as a Python namespace package — a property that does not hold inside any of the three runtime containers.
I also found two narrower issues: the agent's _build_file_boundary_section is not plumbed through to the new override, and operator misconfiguration is silently swallowed with no log signal.
Blocking — non-functional feature: from config.repo_config import … fails in every production runtime
shared/egg_restrictions/patterns.py:780-789 is the entry point for every override read:
try:
from config.repo_config import get_repo_role_patterns
override = get_repo_role_patterns(repo)
except Exception:
override = NoneThe except Exception swallows any failure and falls back to the default globs. That is exactly what happens in production:
-
Gateway sidecar.
gateway/DockerfilesetsWORKDIR /app,ENV PYTHONPATH=/app, and copiesconfig/repo_config.pyto two locations:/config/repo_config.py(NOT onsys.path) and/app/repo_config.py(top-level, noconfig/package).from config.repo_config import …raisesModuleNotFoundError: No module named 'config'. I reproduced this empirically with the same layout:$ PYTHONPATH=/tmp/testapp python3 -c "from config.repo_config import …" FAIL config.repo_config: ModuleNotFoundError No module named 'config' PASS repo_config (top-level)Note that
gateway/gateway.py:988-993already knows this — it uses a two-step import forreload_config(config.repo_config→ fall back torepo_config). The new code inbuild_agent_patternsdoes NOT have that fallback, so it always lands on theexceptbranch in the gateway runtime. -
Orchestrator.
orchestrator/Dockerfiledoes not copyconfig/repo_config.pyat all. Bothfrom config.repo_config import …andfrom repo_config import …raiseModuleNotFoundError.pipelines.py:76andpipelines.py:1492-1517already wrap their imports in try/except with stub fallbacks, confirming the import is known to fail. So_build_role_restrictions_section(repo=…)(the planner-prompt path the PR added) andvalidate_task_role_alignment(slices, repo=…)(the plan-time validator the PR added) both silently fall back to global defaults. -
Sandbox container.
sandbox/Dockerfile:296setsPYTHONPATH="/opt/egg-runtime/sandbox:/opt/egg-runtime/shared".config/repo_config.pylands at/opt/egg-runtime/config/repo_config.py—/opt/egg-runtime/is not onPYTHONPATH. Sotool_interceptor.py's newEGG_PIPELINE_REPOplumbing always falls back to defaults; the agent gets early warnings against Python conventions even when the repo configures Go/JS conventions. The sandbox also has norepositories.yamlmounted (only the gateway does, at/secrets/repositories.yamlperk8s/base/gateway-deployment.yaml), so even if the import succeeded the_get_config_path()lookup wouldFileNotFoundError.
Net effect: the entire PR is a no-op in production. A repo configures role_patterns: in repositories.yaml, the gateway loads the YAML when answering admin endpoints, the gateway's partition_files_by_role is given the right repo argument, but build_agent_patterns(repo) cannot read the YAML and returns the default registry. Every downstream consumer (gateway push enforcement, planner prompt, plan-time validator, sandbox tool interceptor) silently uses Python-language conventions.
This passes the new test suite because pytest runs from the repo root, where config/ is a namespace package on sys.path and the call chain works end-to-end.
Required fix:
build_agent_patterns(and the same import in_find_owning_roleintool_interceptor.py) needs the same two-step import the existing gateway uses:try: from config.repo_config import get_repo_role_patterns except ImportError: try: from repo_config import get_repo_role_patterns # type: ignore[no-redef] except ImportError: override = None ...
- The orchestrator Dockerfile needs to copy
config/repo_config.py(and at least the orchestrator deployment needs arepositories.yamlmount +EGG_REPO_CONFIGenv var) for_build_role_restrictions_section(repo=…)andvalidate_task_role_alignment(repo=…)to read the override. - The sandbox needs either
EGG_PIPELINE_REPO_OVERRIDESinjected into the env, or a config volume mount +EGG_REPO_CONFIG, for the tool interceptor's per-repo plumbing to work. - A test that exercises the import path in a setup that mirrors production (sys.path without the repo root) would have caught all three of these. The current
TestPerRepoCacheInvalidationtest passes a mock torepo_config.get_repo_role_patterns, which never trips the import that breaks in production.
This is a "false analogy" of the kind the review rules call out: the new code reuses the established from config.repo_config import … pattern, but every existing caller of that pattern has either an explicit fallback to repo_config or wraps the result in a try/except that meaningfully degrades. The new code's except Exception: override = None masks the import failure and pretends the feature is operating.
Blocking — agent prompt's "File Boundaries" section is not plumbed through
orchestrator/routes/pipelines.py:11214 correctly threads repo to _build_role_restrictions_section (the planner's prompt). But the analogous _build_file_boundary_section(role_value) call at orchestrator/routes/pipelines.py:10630 and :11326 — the section that tells every other agent (coder, tester, documenter, refiner, etc.) what files they can push — is unchanged.
_build_file_boundary_section reads from a different source entirely (egg_contracts.agent_roles.get_role_definition → FileAccessPattern.allowed_write/blocked_write, see shared/egg_contracts/agent_roles.py:103-145) which the PR does not touch. So in a Go repo:
- The planner is told "tester writes
**/*_test.go" (per the newrepo-aware section). - The coder/tester/documenter is told "tester writes
**/*_test.py" (legacyagent_roles.pypatterns). - The gateway enforces whatever
egg_restrictions.patternssays — which, per issue #1 above, is also the legacy default in production.
This third pattern source (egg_contracts.agent_roles.FileAccessPattern) is pre-existing tech debt — but the PR specifically promises that planner-prompt boundaries match push-time enforcement (see comment at pipelines.py:11212), which is now true for the planner and false for everybody else. A coder doing legitimate Go work will be told its boundaries are Python and will get a confusing prompt that contradicts whatever the gateway will actually do.
Required fix: either (a) plumb repo through _build_file_boundary_section and switch its source from egg_contracts.agent_roles to egg_restrictions.patterns.build_agent_patterns, or (b) document explicitly in this PR that the agent-prompt boundaries remain Python-only and the PR's claimed boundaries-in-sync invariant only applies to the planner. Option (a) is the only one that actually delivers what the PR description promises.
Blocking — get_repo_role_patterns silently drops misconfigured operator input
config/repo_config.py:413-427:
for key, value in raw.items():
if key not in _VALID_ROLE_PATTERN_KEYS:
# Defense-in-depth: a misconfigured repo cannot widen
# security boundaries by inventing keys. The pattern
# builders already ignore anything outside the three knobs,
# so dropping here is purely diagnostic.
continue
if not isinstance(value, list):
continue
cleaned = [str(item) for item in value if isinstance(item, str) and item]
if cleaned:
out[key] = cleanedThe function silently drops:
- Unknown keys (e.g. an operator typo:
tests_globinstead oftests_globs, or an attempt to setcontracts_blocklist). - Non-list values (
tests_globs: 42). - Non-string list entries (
tests_globs: [null]). - Empty strings.
The docstring at :402-407 claims "rejecting the unknown key here gives operators a clean signal in the logs" — but the function does not log anything. The comment is aspirational, not implemented.
This matches the "operator-facing misconfiguration produces no signal" pattern in the review rules. An operator who deliberately sets tests_globs: ["**/*_test.go", null] gets the first pattern and no signal that null was dropped; an operator who types tests_glob (missing s) gets the global default with no indication their config was discarded. Their tests will mis-route on every push and there is no audit trail.
Required fix: at minimum, log a warning at WARNING level for each dropped key/value (with repo, the dropped key, and the reason). Consider raising on type mismatches in non-prod / startup-validation paths so misconfig is surfaced loudly.
Non-blocking suggestions
-
reset_pattern_cacheis non-atomic._repo_pattern_cache.clear()followed by_repo_pattern_cache[None] = AGENT_PATTERNS(shared/egg_restrictions/patterns.py:858-859) leaves a brief window where a concurrent reader can rebuild theNoneentry viabuild_agent_patterns(None)— a benign rebuild but it produces a differentdictobject thanAGENT_PATTERNS. The testtest_get_agent_pattern_for_repo_returns_default_when_no_overrideassertscoder is AGENT_PATTERNS["coder"], which would flake under that race. Replace with_repo_pattern_cache = {None: AGENT_PATTERNS}(atomic rebind, since the dict variable is a module global). -
_find_owning_rolebypasses the cache.shared/egg_agent/tool_interceptor.py:118callsbuild_agent_patterns(repo)directly on every blocked write — rebuilding all 19 role patterns each time. Useget_agent_pattern_for_repo(role_name, repo)in a loop, or expose a memoizedget_all_patterns_for_repo(). Minor, but blocked-write paths can be hot in a noisy session. -
Cache key not normalized.
get_repo_setting(config/repo_config.py:259-263) is case-insensitive.get_agent_pattern_for_repo("coder", "Owner/Repo")andget_agent_pattern_for_repo("coder", "owner/repo")produce two distinct cache entries holding identical patterns. Normalize the key torepo.lower()before lookup. -
test_default_registry_*parity tests are tautologies.gateway/tests/test_per_repo_role_patterns.py:213-227comparesAGENT_PATTERNSagainstbuild_agent_patterns(None)— but in the new code,AGENT_PATTERNSis built from the same_build_*_patternfunctions with the sameDEFAULT_*_GLOBSinputs. The tests can't detect a regression in the constants vs the pre-PR hardcoded literals; they only verify internal consistency. To actually pin the parity claim in the PR description, either embed a snapshot of the pre-PRblocked_patternslists for coder/tester/documenter/autofixer/conflict_resolver, or rely entirely on the existingtest_partition_files_by_role.pyandtest_agent_restrictions*.pysuites and remove these tautological tests. -
CONFLICT_RESOLVER scope expansion is silent. Pre-PR
CONFLICT_RESOLVER_PATTERNS.allowed_patternsdid not include**/*_test.go,**/test_*.go, or**/conftest.py. Post-PR,*DEFAULT_TESTS_GLOBSadds them. This is probably correct (a conflict resolver should handle Go-test conflicts) but the PR description doesn't flag it. Mention the change so reviewers can audit it; same for**/README.mdbecoming an explicit (rather than**/*.md-implicit) entry inCONFLICT_RESOLVER_PATTERNS.allowed_patternsand**/*.shenteringDOCUMENTER_PATTERNS.blocked_patterns(both are no-ops in practice given the role's allowed_patterns, but worth calling out). -
No test for
EGG_PIPELINE_REPO→tool_interceptorplumbing. The whole sandbox plumbing path the PR adds (shared/egg_agent/tool_interceptor.py:71-82) is untested. A test that monkey-patchesEGG_PIPELINE_REPOand asserts the override flows through would catch the import failure described in issue #1. -
final_tests = list(tests_globs) if tests_globs is not None else DEFAULT_TESTS_GLOBS(patterns.py:791-793) does not copyDEFAULT_*_GLOBSwhen no override is provided. The default is shared by reference. The current builders splat into list literals (*tests_globs), so the shared list is read-only — but the safety relies on every future builder following that convention. Defensive copy or freeze the constants.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Reviewer found that the per-repo role-pattern feature is a no-op in production: ``from config.repo_config import get_repo_role_patterns`` in ``build_agent_patterns`` raises ``ModuleNotFoundError`` in every runtime container (gateway, orchestrator, sandbox), and the bare ``except Exception`` swallows it silently. This change makes the feature actually work end-to-end. Functional fixes: - ``shared/egg_restrictions/patterns.py``: extract a public ``load_repo_pattern_override`` helper that (a) reads the new ``EGG_PIPELINE_REPO_PATTERNS_JSON`` env var first (sandbox path — no filesystem access), then (b) attempts ``config.repo_config``, falling back to top-level ``repo_config`` (gateway / orchestrator containers). Mirrors the existing two-step import in ``gateway.gateway._reload_all_config``. - ``config/repo_config.py``: emit WARNING logs for every dropped unknown key, non-list value, or non-string entry in ``role_patterns`` so a misconfigured operator gets a clean signal instead of silently shipping the global default. - ``orchestrator/Dockerfile``: copy ``config/repo_config.py`` so the orchestrator runtime can read per-repo overrides for the planner prompt and plan-time validator. - ``k8s/base/orchestrator-deployment.yaml``: project the ``repositories.yaml`` key out of the gateway-secrets Secret as a read-only mount at ``/etc/egg-config/`` and set ``EGG_REPO_CONFIG`` so the orchestrator finds it. Distinct mount path from the gateway's ``/secrets`` keeps the local-dev overlay's full ``/secrets`` projection from colliding. - ``orchestrator/kubernetes_spawner.py``: pre-resolve the repo override at spawn time and pass it to the sandbox via ``EGG_PIPELINE_REPO_PATTERNS_JSON``. Sandbox containers don't mount ``repositories.yaml``; this snapshots the override into the Job env. - ``orchestrator/routes/pipelines.py``: switch ``_build_file_boundary_section`` from ``egg_contracts.agent_roles`` (Python-only patterns) to ``egg_restrictions.patterns.build_agent_patterns`` and plumb ``repo`` through both call sites. Coder/tester/documenter prompts now match what the gateway will actually enforce on push for non-Python repos. Quality fixes from non-blocking suggestions: - Atomic ``reset_pattern_cache`` via module-global rebind (was ``clear()`` + reseed, which had a benign race with concurrent readers). - Cache key normalization: ``Owner/Repo`` and ``owner/repo`` now share one cache entry (mirrors ``get_repo_setting`` casing). - ``_find_owning_role`` uses the cached ``get_agent_patterns_for_repo`` lookup instead of rebuilding all 19 role patterns on every blocked write. - New ``test_default_registry_preserves_canonical_partition`` pins ``can_write`` outcomes for load-bearing paths (Python / Go / JS), replacing the structural-equality tautologies the reviewer flagged. - New tests cover production-like import fallback, sandbox env-var injection, case-insensitive cache, atomic reset, and config-validation logging.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #2561 — addressing previous review
The new commits (f11dc260, e911adc8, d4a06b63) substantively address all three blocking issues from my previous review and most of the non-blocking suggestions. I traced the data flow end-to-end again and reproduced the production import paths empirically.
Blocking issues — all properly addressed
1. Import fallback (was: non-functional in every runtime container)
Resolved via the new load_repo_pattern_override helper in shared/egg_restrictions/patterns.py:831-902. The function now:
- Reads
EGG_PIPELINE_REPO_PATTERNS_JSONfirst (sandbox path — no filesystem dep), then - Falls back to the two-step import:
from config.repo_config→from repo_config(gateway / orchestrator), mirroring the existinggateway/gateway.py:988-993pattern, then - Differentiates
FileNotFoundError(silent — expected outside the gateway) from other exceptions (logger.exceptionso a real bug isn't swallowed).
I reproduced both production layouts in isolation:
Gateway-like (/app on PYTHONPATH, /config/ exists but no __init__.py):
config.repo_config: FAIL -> No module named 'config'
repo_config (top-level): SUCCESS
Orchestrator-like (/app on PYTHONPATH, no /config/ at all):
config.repo_config: FAIL -> No module named 'config'
repo_config (top-level): SUCCESS
The fallback chain works for both. The mypy no-redef fix in d4a06b63 (forward-declaring _loader: Callable[…] | None = None and tagging the second import with # type: ignore[no-redef]) is the standard pattern; the assertion assert _loader is not None after the import block is defensive but harmless.
2. _build_file_boundary_section not plumbed through (was: agent prompt mismatch)
Resolved at orchestrator/routes/pipelines.py:10495-10556. The function now reads from egg_restrictions.patterns.get_agent_pattern_for_repo(role_value, repo=repo) — the same source the gateway uses on push — and repo is threaded through both call sites at :10636 and :11334. Coder/tester/documenter prompts will now match push-time enforcement on non-Python repos.
The switch also drops the buggy except ValueError, KeyError, ImportError: from the legacy code path. (Note: that line was valid Python 3.14 syntax — PEP 758 made parens optional — but the new except ImportError: is unambiguous.)
The new code does not render block_exempt_patterns, which mirrors the prior behavior (the legacy egg_contracts.agent_roles.FileAccessPattern had no analogous concept) — not a regression. The .github-staging/ hint is preserved by the coder-specific block at :10542-10554.
3. Misconfig logging (was: silent input drop)
Resolved at config/repo_config.py:399-466. Each drop path now emits a WARNING with the offending key/value/repo:
- Unknown keys (e.g.
tests_globtypo) - Non-list values (
tests_globs: 42) - Non-string list entries (
tests_globs: [null]) - Non-dict
role_patternsblock
The warnings carry enough context (key name, repo, expected type) to diagnose without grepping the YAML.
Non-blocking suggestions — addressed
- ✅ Atomic
reset_pattern_cachevia module-global rebind (patterns.py:976-977). - ✅
_find_owning_roleuses cachedget_agent_patterns_for_repo(tool_interceptor.py:114-120). - ✅ Cache key normalization via
_normalize_repo_key(patterns.py:905-912). - ✅ Behavioral parity test
test_default_registry_preserves_canonical_partitionreplaces the structural-equality tautology. - ✅ Production-like import-path tests (
TestProductionLikeImportPaths). - ✅ Sandbox env-var injection tests (
TestSandboxEnvVarInjection,TestPipelineRepoOverrideintest_tool_interceptor.py).
Deployment plumbing — verified
orchestrator/Dockerfile:37 copies config/repo_config.py → ./repo_config.py (top-level). k8s/base/orchestrator-deployment.yaml:115-116, 177-205 mounts repositories.yaml from the existing gateway-secrets Secret at /etc/egg-config/ (read-only, optional projection of just the repositories.yaml key) and points EGG_REPO_CONFIG at it. optional: true on the volume means the orchestrator pod still rolls out cleanly when the Secret lacks the key — _get_config_path() raises FileNotFoundError, which load_repo_pattern_override catches and degrades to defaults.
orchestrator/kubernetes_spawner.py:746-774 pre-resolves the override at spawn time and emits EGG_PIPELINE_REPO_PATTERNS_JSON as a single-key JSON object alongside EGG_PIPELINE_REPO, both wrapped in a try/except Exception so spawn doesn't fail on a config glitch.
Non-blocking observations
These are minor; none warrant another round-trip.
-
Logger name differs between test and production. The test uses
caplog.at_level("WARNING", logger="config.repo_config"), which works because pytest imports the module asconfig.repo_config. In the gateway/orchestrator runtimes the module imports as top-levelrepo_config, sologging.getLogger(__name__)produces a logger namedrepo_config, notconfig.repo_config. Operators filtering production logs by the test's logger name would miss them. Not a correctness issue (the warnings still emit) but worth keeping in mind. -
_build_role_restrictions_sectioncallsbuild_agent_patterns(repo)directly atorchestrator/routes/pipelines.py:5690instead of the cachedget_agent_patterns_for_repo(repo). Single non-hot call site (one per planner-prompt build), so the impact is negligible — but for consistency with the rest of the per-repo plumbing the cached helper would be the obvious choice. -
Env-var lookup in
load_repo_pattern_overrideuses original-caserepo(patterns.py:865) rather than the normalized lowercase key. The cache key normalization elsewhere makesOwner/Repoandowner/reposhare a cache entry, but the env var'sdecoded.get(repo)is case-sensitive. The orchestrator/spawner/sandbox chain usespipeline_repoconsistently so production behavior is unaffected; a developer who hand-sets the env var with one case and queries with another would see misses. Cosmetic. -
.github-staging/is still surfaced only in the coder-specific block atorchestrator/routes/pipelines.py:10542-10554(correctly), andblock_exempt_patterns(e.g..egg-state/agent-outputs/,skills/) are still not rendered into the boundaries section. Both match prior behavior.
Approval
The previous blocking issues are all fixed and the supporting tests now exercise the production import paths properly. Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Stable logger name in config/repo_config.py (egg.repo_config) so production filtering by logger name works regardless of whether the module is imported as config.repo_config (tests) or top-level repo_config (gateway/orchestrator runtimes). - Switch _build_role_restrictions_section in the planner-prompt path to the cached get_agent_patterns_for_repo helper for consistency with the rest of the per-repo plumbing. - Make the EGG_PIPELINE_REPO_PATTERNS_JSON env-var lookup case-insensitive, mirroring get_repo_setting and the cache key normalization, so a hand-set snapshot with mixed casing still resolves under the canonical pipeline_repo string.
Response to non-blocking observations from re-reviewThe latest review approved the PR with four non-blocking observations. I addressed the three actionable items in commit
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #2561 — non-blocking observations addressed
The single new commit 06489863c addresses the three actionable non-blocking observations from the previous re-review. All three are clean, targeted fixes. I traced each change end-to-end and ran the relevant test suites.
Verified — observations correctly addressed
1. Stable logger name in config/repo_config.py
config/repo_config.py:42-43 introduces _LOGGER_NAME = "egg.repo_config" and replaces logging.getLogger(__name__) with logging.getLogger(_LOGGER_NAME). This resolves the prior concern that __name__ resolves to config.repo_config under pytest (where the repo root is on sys.path) but repo_config in the gateway/orchestrator runtimes (where only /app is on PYTHONPATH and repo_config.py is copied to the top level). Operators filtering by logger name will now reliably catch every WARNING regardless of import path.
The three caplog.at_level("WARNING", logger="egg.repo_config") updates in gateway/tests/test_per_repo_role_patterns.py:557, 571, 584 are consistent. Verified via grep: no other code depends on the old logger name.
2. _build_role_restrictions_section switched to cached helper
orchestrator/routes/pipelines.py:5679, 5690 now imports and calls get_agent_patterns_for_repo(repo) instead of build_agent_patterns(repo). The cached helper at shared/egg_restrictions/patterns.py:948-961 is functionally identical — same return type (dict[str, AgentFilePattern]), same construction via build_agent_patterns(repo) on cache miss — but reuses the per-repo cache shared with get_agent_pattern_for_repo. This is the last unmemoized caller; the per-repo plumbing is now uniformly cached.
3. Case-insensitive env var lookup in load_repo_pattern_override
shared/egg_restrictions/patterns.py:864-876 lowercases both the snapshot keys and the query repo before matching, mirroring _normalize_repo_key and get_repo_setting. The new test at gateway/tests/test_per_repo_role_patterns.py:472-487 correctly pins this with a side_effect=AssertionError patch on config.repo_config.get_repo_role_patterns so the env-var path must resolve without falling back to the filesystem loader. Production behaviour is unchanged (orchestrator/spawner/sandbox use the same pipeline_repo string end-to-end), but a developer who hand-sets the env var with mixed casing now hits.
The next(... for k, v in decoded.items() if isinstance(k, str) and ...) form correctly skips non-string keys defensively.
Test results
gateway/tests/test_per_repo_role_patterns.py— 40 / 40 passtests/config/test_repo_config.py— 51 / 51 passorchestrator/tests/test_pipeline_prompts.py— 357 / 357 pass- 3,143 / 3,143 gateway tests pass excluding one unrelated env failure (
test_worktree_manager.py::test_worktree_reuse_resets_to_safe_remote_ref— sandbox blocksgit init, not a regression from this PR)
Non-blocking observations
None worth a round-trip. Two minor notes for the record:
-
egg.repo_configis the onlyegg.*-namespaced logger in the repo. Every other module inshared/,orchestrator/,gateway/useslogging.getLogger(__name__). The hardcoded name is the right call here (precisely because__name__is unstable for this module across runtimes), but it's a one-off naming convention. If a future PR introduces more namespaced loggers it would be worth aligning, e.g.egg.gateway,egg.orchestrator. Not actionable in this PR. -
Pathological case in env-var lookup: duplicate keys differing only by case.
{"Owner/Repo": {...}, "owner/repo": {...}}inEGG_PIPELINE_REPO_PATTERNS_JSONwould causenext()to return whichever was iterated first (insertion order in Python 3.7+). This is operator misconfiguration (the orchestrator-spawned env always emits a single canonical key) and not worth handling.
Approval
The follow-up commit cleanly addresses the actionable non-blocking observations from the prior re-review. Approving.
— Authored by egg
|
egg review completed. View run logs 16 previous review(s) hidden. |
* docs: document per-repo role-pattern overrides (#2528) Update docs to reflect the role_patterns: configuration introduced in #2561: - sdlc-pipeline.md: add "Per-Repository Role Patterns" section with schema, key descriptions, and security boundary notes - agent-roles.md: note coder/tester/documenter file access is defaults-only and add override reference in the File Permission Enforcement section - harness-configuration.md: add EGG_PIPELINE_REPO_PATTERNS_JSON to the integration env vars table * docs: address review feedback on per-repo role-pattern docs - Document set-keys-replace-defaults behavior (not merge) with a Python+Go polyglot example showing the foot-gun. - Correct the validation-logic claim: the orchestrator/gateway path (config/repo_config.py) emits warnings for invalid root type, unknown keys, and non-list/non-string values; the env-var path (shared/egg_restrictions/patterns.py) only warns on invalid JSON and silently filters the rest. Note that operators still see warnings at orchestrator spawn time. - Add autofixer (blocked) to the docs_globs row of the affected-roles table and add a footnote about the conflict-resolver allow list being the union of all three globs. - Add a commented role_patterns: example block (Go + JS/TS) under repo_settings: in repositories.yaml.example, plus a description bullet matching the style of the surrounding settings. - Show the JSON object shape in the EGG_PIPELINE_REPO_PATTERNS_JSON description in harness-configuration.md. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Summary
role_patterns:block underrepo_settingsinrepositories.yamlso target repos can declare their test/code/docs file conventions (Go, JS/TS, etc.) without editing core code.shared/egg_restrictions/patterns.pyto extract default globs intoDEFAULT_TESTS_GLOBS/DEFAULT_CODE_GLOBS/DEFAULT_DOCS_GLOBSand slot them into per-role builders. Threadsrepothrough the four consumers that have it in scope: gateway push enforcement (partition_files_by_role), sandbox tool interceptor (EGG_PIPELINE_REPOenv var), plan-time validator (Validate task role↔file alignment at plan time, not push time #2527'svalidate_task_role_alignment), and the planner prompt renderer..egg-state/contracts/,.github/, draft/review state dirs) are NOT configurable — sourced from the builders and cannot be relaxed by a target repo's config. Unknown keys are silently dropped at the validation layer; defense-in-depth in the builders.Schema:
Default-repo behaviour is unchanged:
AGENT_PATTERNSis identical tobuild_agent_patterns(None). Tests pin this parity.Scope
Per the #2530 audit (the conftest line + test/code split are correct as-is) and the #2528 issue comment, this ships only the narrow knobs. The per-task
includes_testsopt-in and any future per-repopair_ruleare deliberately out of scope; the schema is additive so a futurerole_patterns.pair_rulekey slots in without breaking the current shape.Test plan
gateway/tests/test_per_repo_role_patterns.py(27 tests, all passing) covers:build_agent_patterns(None)== legacyAGENT_PATTERNS).__tests__/,*.test.ts) shifts coder/tester/documenter boundaries.*_test.gosame-dir,testdata/) ditto..egg-state/contracts//.github//.egg-state/drafts/bypass via knobs is denied.get_repo_role_patternsvalidation: missing block, non-dict, unknown keys, non-list values, non-string entries, empty dict.reset_pattern_cache(wired into the SIGHUP /reload_configpath).make testsuite — please run in CI.Closes #2528.