[issue-2769][merge-gate] Add per-agent non-Claude model support via...

[james-in-a-box]

Program-level umbrella PR — terminal slice of pipeline issue-2769.
Roll-up of the slice-PR chain; this PR is the merge gate for the program. Pre-merge obligations (when present) live here.

Today every egg SDLC agent runs on Claude through the Claude Code
harness, with the gateway hard-wiring api.anthropic.com as the
only /v1/messages upstream. The orchestrator's consensus wrapper
hardcodes --model opus for every non-overseer role, so per-agent
model selection does not exist. We need to let any agent run on a
non-Claude backend (Qwen is the first target, primarily for cost)
while every Claude-bound agent stays byte-identically on the
existing path — and we need the integration to be safe to ship
before a live non-Claude endpoint is available.

This change lands the buildable, no-op-by-default seam in two
stacked PRs:

1. Gateway upstream router + LiteLLM Deployment (slice 1).
   Introduces a small UpstreamRegistry abstraction in the
   gateway that keys per-request httpx.Client + credential by
   upstream name, with proxy_anthropic_messages /
   proxy_count_tokens resolving the upstream per request via
   the existing IP-keyed session lookup that already drives
   session_mode. Adds Session.upstream (default "anthropic")
   and Session.upstream_model (default None), wired through
   /api/v1/sessions/create, SessionManager.register_session,
   and GatewayClient.register_session. The LiteLLM proxy itself
   ships as a separate Deployment + Service + ConfigMap in
   egg-system, reachable only by the gateway. The Claude path
   is structurally untouched.
2. Per-agent model config + spawn-side plumbing (slice 2).
   Adds PipelineConfig.agent_models: dict[str, str] and a
   default_agent_model repository-level setting, plus a
   resolution function that the orchestrator's spawner calls to
   (a) thread the right --model to build_consensus_wrapped_command
   and (b) tell the gateway the per-agent upstream and
   upstream_model at session-create time. The gateway, on a
   LiteLLM-routed request, rewrites the body's model field
   from the Claude alias presented to Claude Code to the
   upstream-side model name — keeping Claude Code's compaction
   math sane (cq-5).

With agent_models empty (the default everywhere), no LiteLLM
request fires. Every existing pipeline keeps running on Claude
with byte-identical gateway behavior. The empirical
Claude-Code-compaction smoke test (cq-4) is an operator-driven
follow-up once a live non-Claude endpoint is configured; it is
explicitly out of scope here.

This slice

Per-agent model config + spawn-side plumbing + body rewrite

Files affected:

• orchestrator/models.py
• config/repo_config.py
• config/repositories.yaml.example
• orchestrator/agent_model_resolution.py
• orchestrator/concurrent_executor.py
• orchestrator/kubernetes_spawner.py
• orchestrator/routes/pipelines.py
• gateway/gateway.py
• orchestrator/tests/test_agent_model_resolution.py
• orchestrator/tests/test_concurrent_executor.py
• tests/gateway/test_anthropic_proxy.py
• docs/guides/per-agent-models.md
• docs/index.md
• docs/architecture/upstream-routing.md

Tasks:

• task-2-1: Add agent_models: dict[str, str] = Field(default_factory=dict, ...) to PipelineConfig (orchestrator/models.py:405). Validate keys against the AgentRole enum at shared/egg_contracts/agent_roles.py:46 via a Pydantic validator: unknown roles raise a typed config error at construction time. Values are free-form strings (validated downstream by the resolver in TASK-2-3).
  • Acceptance criteria: - PipelineConfig(agent_models={"refiner": "qwen3-coder-30b"}) constructs successfully. - PipelineConfig(agent_models={"bogus_role": "x"}) raises a Pydantic validation error citing the unknown role. - Default-constructed PipelineConfig.agent_models is an empty dict (no behavioral change for existing pipelines).
• task-2-2: Add a default_agent_model: str | None field to the repositories.yaml schema (documented in config/repositories.yaml.example) and expose it via a new get_default_agent_model(repo) helper in config/repo_config.py (mirroring the get_repo_setting(repo, key, default) pattern at config/repo_config.py:248).
  • Acceptance criteria: - get_default_agent_model("owner/repo") returns the configured value when set in repositories.yaml, or None when absent. - config/repositories.yaml.example shows the new field in context with an inline comment naming the precedence rule (per-pipeline agent_models > this default > built-in "opus").
• task-2-3: New module orchestrator/agent_model_resolution.py exporting resolve_agent_model(role: AgentRole, pipeline_config: PipelineConfig, repo: str | None) -> AgentModelDecision, where AgentModelDecision is a small dataclass with fields (claude_code_alias: str, upstream: str, upstream_model: str | None). Precedence: pipeline_config.agent_models.get(role.value) → get_default_agent_model(repo) → built-in "opus". Classifier: model strings matching opus, opus[1m], sonnet, haiku, or claude-* map to upstream="anthropic", claude_code_alias=<model>, upstream_model=None. Every other string maps to upstream="litellm", claude_code_alias="opus" (cq-5 mitigation), upstream_model=<model>.
  • Acceptance criteria: - resolve_agent_model(AgentRole.CODER, default_config, None) returns (claude_code_alias="opus", upstream="anthropic", upstream_model=None). - resolve_agent_model(AgentRole.REFINER, PipelineConfig(agent_models={"refiner": "qwen3-coder-30b"}), None) returns (claude_code_alias="opus", upstream="litellm", upstream_model="qwen3-coder-30b"). - resolve_agent_model(...) with only default_agent_model="sonnet" set on the repo returns (claude_code_alias="sonnet", upstream="anthropic", upstream_model=None). - Per-pipeline agent_models entry overrides repo-level default_agent_model.
• task-2-4: Thread the resolved decision through the initial spawn path: orchestrator/concurrent_executor.py:454 calls resolve_agent_model(role, ...) and passes model=decision.claude_code_alias to build_consensus_wrapped_command. The same site passes upstream=decision.upstream and upstream_model=decision.upstream_model to the downstream spawn helper that ultimately reaches GatewayClient.register_session (the existing call at orchestrator/kubernetes_spawner.py:735). When the decision is the default-Anthropic case, the new register_session kwargs are omitted (no wire change vs today).
  • Acceptance criteria: - With PipelineConfig.agent_models == {}, the spawn path produces the same build_consensus_wrapped_command args and the same register_session payload as before this slice (regression guard). - With agent_models={"refiner": "qwen3-coder-30b"}, the refiner spawn passes --model opus to the wrapper and upstream="litellm", upstream_model="qwen3-coder-30b" to the gateway.
• task-2-5: Thread the resolved decision through the restart path at orchestrator/routes/pipelines.py:2704. Same shape as TASK-2-4 — resolve, pass model= to build_consensus_wrapped_command, ensure the surrounding restart code reuses the existing session (already registered with the right upstream).
  • Acceptance criteria: - Restarting an agent whose pipeline has a non-default agent_models entry uses the resolved Claude alias for the --model flag. - Restarting an agent on the default Claude path is byte-identical to today.
• task-2-6: Add _rewrite_upstream_model(request_body, upstream_model) next to _filter_blocked_tools in gateway/gateway.py. On LiteLLM-routed requests with session.upstream_model set, the helper parses the JSON body, replaces the top-level "model" field with session.upstream_model, and returns the re-serialized body. On parse error or when upstream_model is None, the body is returned unchanged. Call it in proxy_anthropic_messages and proxy_count_tokens AFTER _filter_blocked_tools and BEFORE building the upstream request.
  • Acceptance criteria: - With upstream == "litellm" and upstream_model == "qwen3-coder-30b", the body forwarded upstream has "model": "qwen3-coder-30b" regardless of the incoming "model" value. - With upstream == "anthropic", the body is byte-identical to the incoming body (regression guard). - Invalid JSON returns the original body unchanged (does not crash the proxy).
• task-2-7: Unit tests for the resolver and the spawn-side wiring: orchestrator/tests/test_agent_model_resolution.py (new) covering precedence + classifier; extensions to the existing concurrent-executor and restart-path test modules (orchestrator/tests/test_concurrent_executor.py or its current equivalent, plus a new or extended pipeline-restart test) that mock the spawner and assert the resolved --model and the register_session kwargs.
  • Acceptance criteria: - All new and extended tests pass under make test. - Tests assert the cq-5 mitigation explicitly: the Claude-Code-facing alias for a LiteLLM-routed agent is always "opus", never the upstream model name. - Default-agent_models path is exercised as the regression guard (no register_session kwargs added; no --model change).
• task-2-8: Extend tests/gateway/test_anthropic_proxy.py with the body-rewrite branch: with a LiteLLM session whose upstream_model is set, the request body forwarded upstream has the rewritten model field; with an Anthropic session, the body is byte-identical. Also test the invalid-JSON path through _rewrite_upstream_model.
  • Acceptance criteria: - Tests pass under make test. - The byte-identical-Claude-path assertion uses a non-default incoming model value (e.g. "opus") and confirms it survives unchanged when upstream == "anthropic".
• task-2-9: Write a how-to doc docs/guides/per-agent-models.md covering: setting agent_models per pipeline; setting default_agent_model per repository in repositories.yaml; the precedence rule; the cq-5 recognised-alias presented-to-Claude-Code mitigation; the operator smoke test (live LiteLLM endpoint, the cq-4-deferred validation). Cross-link from docs/index.md and the new docs/architecture/upstream-routing.md.
  • Acceptance criteria: - The guide names every primitive added in slice 2 with a file:line cite (resolver, config field, repo helper, body-rewrite helper). - It walks an operator through enabling Qwen for the refiner role end-to-end without modifying source code. - docs/index.md and docs/architecture/upstream-routing.md link to it.

Test Plan

Automated:

• make test from the repo root catches both slices' reachable
  suites given the changeset.
• Slice 1: tests/gateway/test_upstream_registry.py (new),
  extensions to tests/gateway/test_anthropic_proxy.py,
  tests/gateway/test_anthropic_credentials.py,
  gateway/tests/test_session_manager.py,
  orchestrator/tests/test_gateway_client.py.
• Slice 2: orchestrator/tests/test_agent_model_resolution.py
  (new), extensions to the concurrent-executor and
  pipeline-spawn tests, extensions to the gateway proxy tests
  covering the body-rewrite branch.

Manual (reviewer):

• Confirm make test and make lint are green.
• kubectl apply --dry-run=client -k k8s/base/ succeeds with
  the new LiteLLM manifests included.
• Spot-check that with agent_models={} and no
  LITELLM_MASTER_KEY in secrets, gateway request flow is
  byte-identical to today's Claude path (no new headers, no
  upstream change, same SSE behavior).

Manual (operator, post-merge — not gating merge):

• Populate LITELLM_MASTER_KEY in secrets.env, configure
  LiteLLM model_list with a hosted Qwen provider (cq-6), set
  agent_models={"refiner": "qwen3-coder-30b"} on a pipeline,
  and exercise a tool-heavy multi-turn loop plus a long session
  crossing the auto-compaction boundary (the cq-4-deferred
  empirical compatibility check).

Manual Steps

Pre-merge: none.

Post-merge (only required when operator wants to actually run an
agent on a non-Claude backend):

1. Add LITELLM_MASTER_KEY=<random> to ~/.config/egg/secrets.env.
2. Populate the LiteLLM ConfigMap model_list with at least one
   backend (hosted Qwen provider first, per cq-6). The
   provider-side API key goes in LiteLLM's standard env-var slot,
   not in secrets.env.
3. Either set default_agent_model in
   ~/.config/egg/repositories.yaml for the target repo, or
   pass agent_models={"<role>": "<model>"} on pipeline submit
   to override per pipeline.

Stack

• Position: merge-gate (slice 2 of 2) in pipeline issue-2769
• Stacked on top of egg/issue-2769/slice-1

Slice slice-2 of pipeline issue-2769. Stacked on top of egg/issue-2769/slice-1.

[james-in-a-box]

Autofix tracking

{"Lint/Custom Checks": 1}

[egg-reviewer]

No agent-mode design concerns.

This PR is upstream-routing infrastructure: a pure model-resolution function, a gateway request-body rewrite, a PipelineConfig field, and spawn/restart plumbing. It touches no agent prompts, so the five agent-mode guidelines (pre-fetching, human-facing output formats, post-processing pipelines, rigid procedures, prompt-level security) don't apply here.

Spot-checks against the enforced architectural conventions, all clean:

• EGG200 (no direct LLM API calls outside the sandbox): The LiteLLM path keeps the gateway-as-proxy boundary intact — the agent's /v1/messages traffic is still proxied by the gateway sidecar, just to a different in-cluster upstream. agent_model_resolution.py is pure data resolution with no API calls.
• EGG201 (model aliases over pinned IDs): The built-in default is the "opus" alias, and agent_models/default_agent_model are operator config rather than hardcoded identifiers. The classifier recognizes versioned claude-* IDs as valid operator input — appropriate flexibility, not a hardcoded use — and the lone docstring example carries a correct # noqa: EGG201.
• cq-5 mitigation: Presenting the "opus" alias to Claude Code for LiteLLM-routed agents preserves the harness's compaction calibration. This supports the agent rather than constraining it — not an anti-pattern.

Design is aligned.

— Authored by egg

[egg-reviewer]

Code Review — PR #2774: Per-agent non-Claude model support (slice 2)

Thorough review of the slice-2 changeset. I traced the full data flow (resolver → spawn/restart → register_session → gateway session → proxy_anthropic_messages/proxy_count_tokens body rewrite), read the slice-1 primitives this builds on, and ran the three affected suites locally: 159 passed, 2 skipped (test_agent_model_resolution.py 30, test_concurrent_executor.py 53, test_anthropic_proxy.py 76).

The production wiring is sound — the no-op-by-default invariant holds, the gateway body rewrite is correctly resilient, and the tests genuinely exercise the production code path (the gateway tests drive the real Flask route). But I found two blocking issues, the first of which breaks the guide's primary recommended workflow.

---

BLOCKING 1 — per-agent-models.md uses the wrong YAML top-level key; the "recommended" repo-default workflow is broken

docs/guides/per-agent-models.md tells operators to put default_agent_model under a repositories: top-level key, in two snippets:

• Line 74 ("Repository-level default" section)
• Line 273 — "3a. Per-repository default (recommended for stable rollouts)"

# docs/guides/per-agent-models.md:273  — WRONG
repositories:
  acme-corp/widgets:
    default_agent_model: qwen3-coder-30b

But repositories.yaml has no repositories: key. Per-repo settings live under repo_settings: — see config/repositories.yaml.example:123 and get_repo_setting itself:

# config/repo_config.py:260-261
config = _load_config()
repo_settings = config.get("repo_settings", {})

An operator who copy-pastes the guide's 3a block writes config under repositories:. _load_config() parses it, config.get("repo_settings", {}) returns {}, get_repo_setting returns the default (None), get_default_agent_model returns None, and the repo-level default is silently dropped — every agent stays on Claude with no error and no log line. This is a silent no-op on deliberately-set operator config, and it sinks the workflow the guide explicitly labels "recommended for stable rollouts."

Fix: change repositories: → repo_settings: in both snippets (lines 74 and 273). Note config/repositories.yaml.example itself is correct, so the example file and the guide currently disagree.

---

BLOCKING 2 — agent_models validator accepts role keys the feature never honors

PipelineConfig._validate_agent_models_roles (orchestrator/models.py:772-792) validates keys against the entire AgentRole enum:

valid = {role.value for role in AgentRole}

That enum (shared/egg_contracts/agent_roles.py:46-94) includes overseer, autofixer, conflict_resolver, and inspector. But resolve_agent_model is threaded through only two spawn sites — concurrent_executor._spawn_agent and routes/pipelines.py:restart_agent — which cover the phase + reviewer roles. The overseer spawns via the separate spawn_overseer_job; autofixer/conflict_resolver/inspector spawn on-demand. None of those consult the resolver.

So PipelineConfig(agent_models={"overseer": "qwen3-coder-30b"}) constructs successfully, and the override is then silently dropped — no error, no warning, no effect. This directly defeats the validator's own stated purpose:

"Catches typos at PipelineConfig construction time so misconfigured overrides surface immediately instead of silently being ignored at spawn time."

The guide reinforces the false expectation: it says the feature flips "any single SDLC agent role (refiner, coder, tester, …)" and "Each agent role independently resolves."

Fix (pick one): restrict the validator's accepted set to the roles actually threaded through the resolver (phase + reviewer roles); or keep the full-enum validation but emit a loud warning at spawn when an agent_models entry names a role that never reaches resolve_agent_model, and document the honored-role set in the guide. A validated-but-ignored key is exactly the silent-ignore trap the validator was added to prevent.

---

Non-blocking

N1 — Doc contradicts code: restart does re-register the session. The "How the resolved decision threads through spawn" table (per-agent-models.md:125) says the restart path uses the "Same session … no second register_session call". That is false: restart_agent → restart_agent_container/restart_agent_job → spawn_agent_job → register_session registers a new session. The PR's own kubernetes_spawner.py comment confirms it ("the gateway session is otherwise rebuilt against the anthropic default"). The code is correct; the doc row is wrong — update it.

N2 — sonnet[1m] is undocumented. _CLAUDE_EXACT_ALIASES (agent_model_resolution.py) contains "sonnet[1m]", but every doc surface lists only opus, opus[1m], sonnet, haiku, claude-* — the module docstring, classify_model's context, PipelineConfig.agent_models' field description, get_default_agent_model's docstring, repositories.yaml.example, and per-agent-models.md's classifier table. Even _is_claude_alias's own docstring lists it while the module docstring doesn't. Code accepts one more alias than it documents — add sonnet[1m] to the doc surfaces (or drop it from the set).

N3 — Restart fallback import skips the dual-import pattern. The resolver-failure fallback at routes/pipelines.py:2697 does a bare from agent_model_resolution import UPSTREAM_ANTHROPIC, classify_model, without the except ImportError → from ..agent_model_resolution fallback used by the primary import 20 lines above (2671-2680). In the (narrow) topology where agent_model_resolution is importable only via the relative path, a resolver failure would convert into an uncaught ImportError → HTTP 500 on restart. Mirror the dual-import for consistency, or reuse the already-bound symbols.

N4 — restart_agent_job docstring is stale. kubernetes_spawner.py:1243 adds upstream/upstream_model parameters but the Args section stops at wait_for_gateway. Add the two new entries.

N5 — Weak assertion. test_default_config_passes_opus_to_consensus_wrapper captures kwargs.get("model", "opus") with a default of "opus", so it cannot distinguish "model='opus' was passed" from "model was not passed at all." Use kwargs.get("model") (as the sibling override tests do) so the regression guard is real.

---

What's good

• The no-op-by-default path is genuinely preserved: default agent_models={} adds no register_session kwargs and no --model change, and the gateway forwards the Anthropic body byte-identically (verified by the regression tests).
• _rewrite_upstream_model correctly mirrors _filter_blocked_tools' bytes-in/bytes-out, parse-tolerant contract, runs after tool filtering on both proxy routes, and applies to streaming and non-streaming paths.
• Tests exercise the real production code path rather than hand-built fixtures — the gateway tests drive the actual Flask route with a faked upstream registry.

Please address the two blocking items (the YAML key fix is a one-word change in two places; the validator gap needs a small decision on restrict-vs-warn). Happy to re-review.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with egg/issue-2769/slice-1 (merge commit, no rebase):

File	Category	Resolution
docs/architecture/upstream-routing.md	Semantic (doc)	Kept slice-2's text on both conflicting blocks
tests/gateway/test_anthropic_proxy.py	Semantic + additive	Took slice-1's tightened assertion; kept all of slice-2's new test section

Details

docs/architecture/upstream-routing.md (2 conflict blocks) — Both sides describe what slice 2 adds. Slice-1's text said the operator guide "does not exist until slice 2 lands" and referenced it as a plain path. This PR is slice 2 and ships docs/guides/per-agent-models.md, so slice-2's text (with the working markdown link) is correct post-merge. Kept HEAD on both blocks.

tests/gateway/test_anthropic_proxy.py (1 conflict block) — Two independent changes overlapped:

• Both sides modified the final assertion of test_known_upstreams_pass_validation. Slice-1's review feedback tightened it from assert status != 400 or ... to assert status == 200, which matches that test's docstring ("reach a clean 200"). Slice-2's branch carried the older, weaker form only because it branched off an earlier slice-1. Took slice-1's == 200 assertion — it is the deliberately-tightened, newer version, and slice-2 has no stake in this particular check.
• Slice-2 additionally appended a large new body-rewrite test section (TestRewriteUpstreamModelHelper, TestRewriteUpstreamModelOnProxyRoute, TestRewriteUpstreamModelMalformedBodyResilience) after that assertion. Kept all of it verbatim. The trailing ) shared by both sides now closes slice-2's malformed-JSON != 500 assertion.

Verification

• tests/gateway/test_anthropic_proxy.py — 79 passed
• Full gateway + slice-2 suites (tests/gateway/, test_agent_model_resolution.py, test_gateway_client.py, test_session_manager.py) — 442 passed
• orchestrator/tests/test_concurrent_executor.py — 53 passed
• ruff check + ruff format --check on both resolved files — clean

Please review

• The test_known_upstreams_pass_validation assertion: I chose slice-1's stricter == 200 over slice-2's weaker != 400 or .... This is the only behavioral pick in the resolution — confirm the strict form is intended (it matches the test docstring).

— Authored by egg

[egg-reviewer]

No agent-mode design concerns (re-review).

Delta since my last review (66723b73): a single merge commit (4b77867) integrating egg/issue-2769/slice-1. The only PR-authored content in that merge is conflict resolution in two files:

• docs/architecture/upstream-routing.md — markdown prose; kept slice-2's text (working link to the operator guide this PR ships). No agent-design surface.
• tests/gateway/test_anthropic_proxy.py — test code; took slice-1's tightened == 200 assertion and kept slice-2's body-rewrite test section. Test fixtures reference model names (opus, qwen3-coder-30b, claude-3-5-sonnet-…) as assertion data, not production model selection — not an EGG201 concern.

The slice-2 source code I assessed previously (the pure resolve_agent_model resolver, the gateway _rewrite_upstream_model body rewrite, the PipelineConfig.agent_models field, and the spawn/restart plumbing) is unchanged by this delta. The gateway-as-proxy boundary still holds and the no-op-by-default path is preserved. My prior approval stands.

The two request-changes blockers on this PR are from the general code-review bot (YAML key, validator scope) — those are correctness/doc-accuracy issues outside the agent-design scope and not re-litigated here.

— Authored by egg

[egg-reviewer]

Re-review — PR #2774 (slice 2): Per-agent non-Claude model support

Re-reviewed at 4b77867c. The only PR-authored change since my last review (66723b73) is the merge commit 4b77867c, which merged egg/issue-2769/slice-1 into the branch and resolved conflicts in two files.

Conflict resolution — correct

I verified both resolutions against each parent and ran the affected suites:

• docs/architecture/upstream-routing.md — kept slice-2's text on both conflicting blocks (working markdown link to the guide this PR ships) and preserved slice-1's new cq-9 table row. Correct.
• tests/gateway/test_anthropic_proxy.py — took slice-1's tightened assert status == 200 in test_known_upstreams_pass_validation, kept slice-2's TestRewriteUpstreamModelHelper / TestRewriteUpstreamModelOnProxyRoute / TestRewriteUpstreamModelMalformedBodyResilience sections. Correct.
• No leftover conflict markers. test_anthropic_proxy.py 79 passed; test_agent_model_resolution.py + test_concurrent_executor.py + test_session_manager.py 217 passed.

Both blocking issues from the last review remain unaddressed

The merge commit does not touch docs/guides/per-agent-models.md or orchestrator/models.py, so neither blocking item was fixed. I re-verified both against the current tree (4b77867c):

---

BLOCKING 1 — per-agent-models.md still uses the wrong YAML top-level key

docs/guides/per-agent-models.md still tells operators to nest default_agent_model under a repositories: key, in both snippets:

• "Repository-level default" section (~line 74)
• "3a. Per-repository default (recommended for stable rollouts)" (~line 272)

# per-agent-models.md ~line 272 — WRONG
repositories:
  acme-corp/widgets:
    default_agent_model: qwen3-coder-30b

repositories.yaml has no repositories: key — per-repo settings live under repo_settings:. Confirmed against the current tree:

• config/repo_config.py:261 — repo_settings = config.get("repo_settings", {})
• config/repositories.yaml.example:123 — top-level key is repo_settings:

An operator who copy-pastes the guide's 3a block writes config under repositories:, get_repo_setting returns the default (None), get_default_agent_model returns None, and the repo-level default is silently dropped — every agent stays on Claude with no error. This is a silent no-op on deliberately-set operator config, and it sinks the workflow the guide explicitly labels "recommended for stable rollouts." The example file and the guide currently contradict each other.

Fix: repositories: → repo_settings: in both snippets.

---

BLOCKING 2 — agent_models validator still accepts role keys the feature never honors

PipelineConfig._validate_agent_models_roles (orchestrator/models.py:772-792) still validates keys against the entire AgentRole enum:

valid = {role.value for role in AgentRole}

That enum (shared/egg_contracts/agent_roles.py) includes overseer, autofixer, conflict_resolver, inspector, and applier. resolve_agent_model is threaded only through concurrent_executor._spawn_agent and routes/pipelines.py:restart_agent (phase + reviewer roles). The overseer/autofixer/conflict_resolver/inspector/applier spawn paths never consult the resolver.

So PipelineConfig(agent_models={"overseer": "qwen3-coder-30b"}) constructs successfully and the override is then silently dropped at spawn — defeating the validator's own stated purpose ("misconfigured overrides surface immediately instead of silently being ignored at spawn time"). A validated-but-ignored key is exactly the silent-ignore trap the validator was added to prevent.

Fix (pick one): restrict the validator's accepted set to the roles actually threaded through the resolver; or keep full-enum validation but emit a loud warning at spawn when an agent_models entry names a role the resolver never reaches, and document the honored-role set in the guide.

---

Non-blocking items from the last review — also untouched

N1 (restart-path doc row says no second register_session — it does re-register), N2 (sonnet[1m] undocumented), N3 (restart fallback import skips the dual-import pattern), N4 (stale restart_agent_job docstring), N5 (weak kwargs.get("model", "opus") assertion) all remain. None block merge; fold them in with the blocking fixes.

---

The conflict resolution in this delta is sound. But the two blocking issues stand — the YAML key is a one-word change in two places; the validator gap needs the restrict-vs-warn decision. Happy to re-review once both land.

— Authored by egg

[james-in-a-box]

Review feedback addressed — all items fixed in commit 3b1f845

Thanks for the thorough re-review. Every item is fixed in-PR; per-item disposition below.

Blocking

BLOCKING 1 — wrong YAML top-level key — fixed-in-PR (commit 3b1f845)
Both snippets in per-agent-models.md (the "Repository-level default" section and "3a. Per-repository default") now nest default_agent_model under repo_settings:, matching config/repositories.yaml.example and get_repo_setting's config.get("repo_settings", {}).

BLOCKING 2 — validator accepts unhonored role keys — fixed-in-PR (commit 3b1f845)
Took the restrict-the-validator option. Added agent_roles.MODEL_OVERRIDE_ROLES — a frozenset derived from _PHASE_ROLES + _PHASE_REVIEWERS (the roles resolve_agent_model is actually threaded through). PipelineConfig._validate_agent_models_roles now validates keys against that set, so agent_models={"overseer": ...} raises at construction time instead of silently no-op-ing at spawn. The honored set resolves to the 16 phase producer/reviewer roles; the rejected set is exactly autofixer, conflict_resolver, inspector, overseer. New regression test test_unhonored_real_role_raises_validation_error covers all four. Guide and field-description text updated to describe the honored set.

One small correction: applier is honored — it's the producer for the apply phase (_PHASE_ROLES["apply"]), so it reaches the resolver via get_roles_for_phase("apply", …). The re-review listed it among the unhonored roles; the validator correctly keeps it valid.

Non-blocking

N1 — restart-path doc row contradicts code — fixed-in-PR (commit 3b1f845)
The "threads through spawn" table row now states the restart respawns the Job and registers a new gateway session (restart_agent_job → spawn_agent_job → register_session), rather than reusing the session.

N2 — sonnet[1m] undocumented — fixed-in-PR (commit 3b1f845)
Added sonnet[1m] to every classifier doc surface: the agent_model_resolution.py module docstring, PipelineConfig.agent_models field description, get_default_agent_model's docstring, repositories.yaml.example, and the guide's classifier table.

N3 — restart fallback import skips the dual-import pattern — fixed-in-PR (commit 3b1f845)
The resolver-failure fallback in routes/pipelines.py now uses the same try: from agent_model_resolution … except ImportError: from ..agent_model_resolution … dual-import as the primary import.

N4 — stale restart_agent_job docstring — fixed-in-PR (commit 3b1f845)
Added upstream / upstream_model Args entries to restart_agent_job, and to spawn_agent_job as well (the PR added the same params there and left them undocumented too).

N5 — weak assertion — fixed-in-PR (commit 3b1f845)
test_default_config_passes_opus_to_consensus_wrapper now captures kwargs.get("model") with no default, so it distinguishes "model='opus' was passed" from "model was not passed at all".

Verification

make lint clean; tests/test_agent_model_resolution.py + tests/test_concurrent_executor.py — 84 passed.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns (re-review).

Re-reviewed the delta since my last approval (4b77867c → 3b1f8452, one commit: "Address review feedback on per-agent model support"). The delta is 10 files, +123/-44 — entirely feedback fixes from the general code-review bot's two blocking items plus the five non-blocking notes. None of it touches agent prompts, the sandbox, or model-selection logic.

Per-change assessment against the agent-mode guidelines:

• Doc/docstring fixes (per-agent-models.md, repo_config.py, repositories.yaml.example, agent_model_resolution.py module docstring, kubernetes_spawner.py param docstrings) — the repositories: → repo_settings: YAML-key correction, the sonnet[1m] alias documentation, and the restart-path table row are all prose accuracy fixes. No agent-design surface.
• Validator tightening (models.py + new agent_roles.MODEL_OVERRIDE_ROLES) — PipelineConfig.agent_models now validates keys against the phase producer/reviewer set the resolver actually threads through, rejecting overseer/autofixer/conflict_resolver/inspector at construction time. This is code-enforced config validation surfacing a misconfiguration at the boundary — the correct place for a constraint, not a prompt-level one. It restricts which operator-config keys are honored, not any agent's runtime exploration.
• Import resilience + test improvements (routes/pipelines.py dual-import fallback, test_agent_model_resolution.py new regression test, test_concurrent_executor.py strengthened assertion) — no agent-design surface.

No EGG201 concern in the delta: the alias strings (opus, sonnet, sonnet[1m], haiku, claude-*, qwen3-coder-30b) appear only as docstring/comment documentation of the classifier and as test-fixture data — no hardcoded full Claude model ID is used for selection. The cq-5 mitigation (presenting the "opus" alias to Claude Code for LiteLLM-routed agents to preserve compaction calibration) is unchanged from what I assessed previously and still supports the agent rather than constraining it.

My prior approval stands.

— Authored by egg

[egg-reviewer]

Re-review — both blocking items fixed; approving with non-blocking nits

Re-reviewed the delta since commit 4b77867. (Note: the PR branch was squashed to a single commit 3b1f8452 and 4b77867 is no longer fetchable through the gateway, so this pass re-verifies the full PR diff against the base with focus on the previously-flagged items.)

Blocking items — both verified fixed

BLOCKING 1 — wrong YAML top-level key — FIXED. Both snippets in per-agent-models.md now nest default_agent_model under repo_settings: — the "Repository-level default" section (line 80) and "3a. Per-repository default" (line 279). Matches config/repositories.yaml.example:68 and get_repo_setting's config.get("repo_settings", {}).

BLOCKING 2 — validator accepts unhonored role keys — FIXED. agent_roles.MODEL_OVERRIDE_ROLES is derived from _PHASE_ROLES.values() + _PHASE_REVIEWERS.values(), and _validate_agent_models_roles now validates against it. I traced the spawn path to confirm the derivation is correct: _run_pipeline routes every phase (including apply) through _run_concurrent_phase → ConcurrentPhaseExecutor._spawn_agent → resolve_agent_model (routes/pipelines.py:19600 docstring + :21188 dispatch). So every role in those two maps genuinely reaches the resolver, and the four rejected roles (overseer, autofixer, conflict_resolver, inspector) genuinely don't. The author's correction is right — applier is honored (_PHASE_ROLES["apply"]). test_unhonored_real_role_raises_validation_error covers all four rejected roles.

N1–N5 are all addressed (restart-path doc row, sonnet[1m] doc surfaces, restart dual-import, spawn_agent_job/restart_agent_job Args entries, the kwargs.get("model") assertion).

New-code scrutiny

Traced the primary use case (agent_models={"refiner": "qwen3-coder-30b"}) end-to-end: validator accepts → _spawn_agent resolves to (opus, litellm, qwen3-coder-30b) → build_consensus_wrapped_command(model="opus") + spawn_kwargs carries upstream/upstream_model → spawn_agent_job → register_session → gateway proxy_anthropic_messages rewrites the body's model to qwen3-coder-30b. The gateway helper and both call sites are correct (upstream_name/session in scope in both proxy_anthropic_messages and proxy_count_tokens; rewrite guarded by upstream_name != "anthropic"; byte-identical on the Anthropic path). Resolution tests (31) and concurrent-executor tests (53) pass locally; tests exercise the production functions directly (no self-seeding goldens, no path-bypassing fixtures).

Non-blocking suggestions

1. per-agent-models.md line reference is stale. The guide says agent_models "lives next to the existing overseer-tier model fields around orchestrator/models.py:405" (and the primitives table repeats :405). The field is actually at orchestrator/models.py:757. Two occurrences — worth correcting since the guide is otherwise precise about line numbers.

2. "register_session kwargs omitted entirely" is slightly inaccurate. The guide (the "threads through spawn" section) says on the default Claude path "the new register_session kwargs are omitted entirely." kubernetes_spawner.py:770-771 always passes upstream=upstream, upstream_model=upstream_model to register_session (as None on the default path). The conditional omission happens one layer up, at the _spawn_agent → spawn_fn boundary (concurrent_executor.py builds spawn_kwargs conditionally). The regression guard is real; the guide just attributes it to the wrong layer.

3. sonnet[1m] still missing from the test file. The N2 fix added sonnet[1m] to the doc surfaces, but test_agent_model_resolution.py was not updated: the module docstring's classifier list and test_short_claude_alias_is_anthropic's parametrize list (["opus", "opus[1m]", "sonnet", "haiku"]) both omit it. _CLAUDE_EXACT_ALIASES includes sonnet[1m], so it's handled — just untested.

4. No positive test for applier as an accepted key. test_unhonored_real_role_raises_validation_error proves the four rejected roles raise, but nothing asserts applier — the role specifically corrected as honored — is accepted. test_multiple_known_roles_accepted covers only refiner/coder/tester. A one-line addition would lock in the correction.

5. Non-string default_agent_model degrades silently. get_default_agent_model returns the raw YAML value (cast(str, value) does not convert). A non-string value (default_agent_model: 4) reaches classify_model, where _CLAUDE_VERSIONED_RE.match(non_str) raises TypeError, which the spawn-path except Exception swallows into the opus fallback with only a log line. The realistic misconfigs are handled loudly (bad role key → ValidationError; bad model name → LiteLLM 502 fail-closed; agent_models values are Pydantic-checked via dict[str, str]), and YAML scalars are naturally strings, so this is a thin edge case — but an isinstance(value, str) guard in get_default_agent_model would close it.

— Authored by egg

[james-in-a-box]

Re-review suggestions addressed — all five fixed in commit c22dc8e

Thanks for the thorough re-review and for verifying both blocking items. All five non-blocking suggestions are fixed in-PR; per-item disposition below.

1 — stale orchestrator/models.py:405 line reference — fixed-in-PR (commit c22dc8e)
per-agent-models.md now references orchestrator/models.py:757 in both places (the prose under "Per-pipeline override" and the primitives table row). The field is at line 757.

2 — "register_session kwargs omitted entirely" is inaccurate — fixed-in-PR (commit c22dc8e)
The "threads through spawn" paragraph now attributes the omission to the correct layer: ConcurrentPhaseExecutor._spawn_agent omits the upstream / upstream_model kwargs from its spawn_fn call (concurrent_executor.py:501-503), while spawn_agent_job still passes both to register_session as None on the default path (kubernetes_spawner.py:770-771) and register_session drops None values from the request body (gateway_client.py:707-712). The byte-identical wire shape claim stands; the doc now names the right layer.

3 — sonnet[1m] missing from the test file — fixed-in-PR (commit c22dc8e)
Added sonnet[1m] to the test_agent_model_resolution.py module docstring classifier list and to test_short_claude_alias_is_anthropic's parametrize set, so the alias _CLAUDE_EXACT_ALIASES already accepts is now exercised.

4 — no positive test for applier as an accepted key — fixed-in-PR (commit c22dc8e)
test_multiple_known_roles_accepted now includes "applier": "haiku" with an assertion, plus a docstring note explaining applier is the honored apply-phase producer. This locks in the correction that applier is a valid agent_models key.

5 — non-string default_agent_model degrades silently — fixed-in-PR (commit c22dc8e)
get_default_agent_model now has an isinstance(value, str) guard: a non-string value (e.g. default_agent_model: 4) raises a clear ValueError naming the bad value, instead of reaching classify_model and raising an opaque TypeError from the regex internals that the spawn-path except Exception swallows into the opus fallback. New regression test test_non_string_repo_default_raises_value_error covers it; the docstring gained a Raises: section.

Verification

make lint clean; test_agent_model_resolution.py (33) + test_concurrent_executor.py (53) + tests/config/test_repo_config.py (51) — 137 passed.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns (re-review).

Re-reviewed the delta since my last approval (3b1f8452 → c22dc8e, one commit: "Address re-review suggestions on per-agent model support"). The delta is 3 files, +59/-11 — entirely the five non-blocking nits from the general code-review bot's re-review. None of it touches agent prompts, the sandbox, or model-selection logic.

Per-change assessment against the agent-mode guidelines:

• Doc accuracy fixes (per-agent-models.md) — the stale orchestrator/models.py:405 → :757 line reference (two occurrences) and the corrected "threads through spawn" paragraph (attributing the conditional kwarg omission to the _spawn_agent → spawn_fn boundary rather than register_session) are prose accuracy fixes. No agent-design surface.
• Test coverage additions (test_agent_model_resolution.py) — adding sonnet[1m] to the classifier docstring + test_short_claude_alias_is_anthropic parametrize set, and applier to test_multiple_known_roles_accepted, both lock in already-implemented behavior. No agent-design surface. No EGG201 concern: the alias strings appear only as test-fixture data and docstring documentation of the classifier, not as hardcoded selection identifiers.
• Config-boundary validation (get_default_agent_model ValueError guard) — a non-string default_agent_model in repositories.yaml now raises an explicit, operator-readable ValueError instead of reaching classify_model and producing an opaque TypeError the spawn-path except Exception swallows into the opus fallback. This is code-enforced validation of operator config surfaced loudly at the boundary — the correct place for the constraint. It does not constrain any agent's runtime exploration.

My prior approval stands.

— Authored by egg

[egg-reviewer]

Re-review — all five non-blocking suggestions correctly addressed; approving

Re-reviewed the delta since my last approval (3b1f8452 → c22dc8ed, one commit: "Address re-review suggestions on per-agent model support"). The delta is 3 files (+59/-11) and contains only doc/test/guard fixes — no change to the feature's logic. Verified each item against the actual code, not just the diff.

Per-suggestion verification

1 — stale orchestrator/models.py:405 → :757. Confirmed: agent_models: dict[str, str] = Field(...) is at orchestrator/models.py:757. Both occurrences in per-agent-models.md (the prose and the primitives table) now point to :757. Correct.

2 — "register_session kwargs omitted entirely" rewrite. Every line reference in the rewritten paragraph checks out:

• concurrent_executor.py:501-503 — if decision.upstream != UPSTREAM_ANTHROPIC or decision.upstream_model is not None: then sets spawn_kwargs["upstream"] / ["upstream_model"]. The conditional omission is indeed at the _spawn_agent → spawn_fn boundary.
• kubernetes_spawner.py:770-771 — register_session(..., upstream=upstream, upstream_model=upstream_model) unconditionally (as None on the default path).
• gateway_client.py:707-712 — register_session drops None: if upstream is not None: request_data["upstream"] = upstream and the same for upstream_model.
  The doc now attributes the byte-identical wire shape to the correct layer.

3 — sonnet[1m] added to the test module. Confirmed _CLAUDE_EXACT_ALIASES includes "sonnet[1m]" (agent_model_resolution.py:61). It is now in the module-docstring classifier list and test_short_claude_alias_is_anthropic's parametrize set — 33 passed.

4 — positive applier test. Confirmed MODEL_OVERRIDE_ROLES is derived from _PHASE_ROLES.values(), and _PHASE_ROLES["apply"] = [AgentRole.APPLIER], so "applier" is an honored key. test_multiple_known_roles_accepted now asserts {"applier": "haiku"} is accepted, with a docstring explaining why.

5 — non-string default_agent_model guard. get_default_agent_model now raises a clear ValueError naming the bad value before a non-string can reach classify_model. test_non_string_repo_default_raises_value_error exercises the production function directly (patches get_repo_setting to return 4, asserts ValueError match). Tests pass.

Note (not blocking, no action needed): the ValueError is still caught by the spawn-path except Exception at concurrent_executor.py:472-485 (and the analogous restart path), so a misconfigured repo still degrades to the built-in opus decision rather than hard-failing. That degrade-with-log behavior is the deliberate design (the defensive wrap comment notes a resolver crash would "bring down agent spawn for every pipeline"). The fix's real benefit is that the swallowed exception now carries an actionable message in the logger.warning (error=str(resolve_err)) instead of an opaque TypeError from the regex internals — which is exactly the right scope for what suggestion 5 asked for.

Verification

test_agent_model_resolution.py — 33 passed. tests/config/test_repo_config.py — 51 passed. No new issues; the delta does not touch the feature logic, only docs/tests/guard.

My prior approval stands.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

15 previous review(s) hidden.