overseer: wire overseer_advisor_model through consult-advisor CLI (#2113)

[jwbron]

Summary

• Plumbs PipelineConfig.overseer_advisor_model through the egg-orch overseer consult-advisor CLI verb so the configured advisor alias actually reaches consult_advisor. Previously the verb passed config=None, silently defaulting to "opus" regardless of pipeline config — flagged in the #2096 re-review.
• Adds an optional positional pipeline_id to the verb (auto-resolved from EGG_PIPELINE_ID, matching every other overseer subcommand). When provided, the verb reads data.config.overseer_advisor_model from the orchestrator status endpoint (already surfaced by overseer: advisor-gated escalation + auto-issue filing (#1962) #2096) and passes a duck-typed config to consult_advisor.
• Status-endpoint failures fall back to config=None with a warning, preserving the historic "opus" default for unreachable orchestrators or unconfigured pipelines. No interface break — the existing --inputs-file-only invocation still works.

Test plan

• pytest sandbox/tests/test_egg_orch_overseer_consult_advisor.py shared/tests/test_overseer_advisor.py (39 passed; four new regressions)
• make lint-python (ruff + mypy clean)
• New regression covers: positional pipeline_id flows alias → consult_advisor; EGG_PIPELINE_ID resolution; orchestrator failure fallback; parser accepts the positional arg
• Reviewer to confirm the duck-typed SimpleNamespace config is acceptable vs. fabricating a real PipelineConfig (kept minimal because the advisor only reads .overseer_advisor_model and the unit tests at shared/tests/test_overseer_advisor.py:230-247 already exercise this shape)

Closes #2113.

[egg-reviewer]

No agent-mode design concerns. This is a focused config-plumbing fix that wires an existing alias through an existing CLI verb — it does not introduce pre-fetching, structured output for humans, post-processing pipelines, or direct LLM API calls. The model alias resolution itself reinforces the alias-over-pinned-identifier convention (the whole point is to let PipelineConfig.overseer_advisor_model flow through).

— Authored by egg

[egg-reviewer]

Review of PR #2158

The core fix is correct: consult-advisor now reads PipelineConfig.overseer_advisor_model from the orchestrator status endpoint and threads it into consult_advisor, instead of hardcoding config=None (which silently defaulted to "opus"). The duck-typed SimpleNamespace is acceptable — consult_advisor (shared/egg_overseer/advisor.py:181) only reads .overseer_advisor_model, and the unit tests at shared/tests/test_overseer_advisor.py:230-247 already exercise this shape. Test coverage for the happy path, env-resolved path, fallback path, and parser registration is solid; the autouse _clear_egg_pipeline_id_env fixture is a clean way to insulate prior tests.

Approving — findings below are non-blocking but worth folding in.

Non-blocking

1. validate_id crashes the verb instead of falling back when EGG_PIPELINE_ID is malformed. (sandbox/egg_lib/orch_cli.py:1826)

validate_id (sandbox/egg_lib/orch_cli.py:87-102) calls sys.exit(1) directly on an id that fails _SAFE_ID_PATTERN. The surrounding try only catches OrchestratorError, so the SystemExit escapes. If EGG_PIPELINE_ID happens to contain a stray space, slash, or other character outside [a-zA-Z0-9_\-\.], the verb hard-exits with code 1 (which collides with the AdvisorParseError exit-code semantics documented in the docstring) instead of warning + falling back to the "opus" default.

This is a behavioural regression: callers that previously worked because the verb didn't read EGG_PIPELINE_ID will now crash on a corrupted env. The PR description says the fallback "preserves the historic 'opus' default for unreachable orchestrators or unconfigured pipelines"; an invalid env value isn't covered by that.

Realistically, pipeline IDs in this codebase are UUID-shaped and pass the pattern, so the blast radius is small — but this is the wrong code path to die loudly on. Suggested fix: either a manual _SAFE_ID_PATTERN.match check that silently skips the lookup, or except (OrchestratorError, SystemExit) (with the existing warning).

2. ImportError from the lazy import is not handled. (sandbox/egg_lib/orch_cli.py:1824)

from egg_lib.orch_client import OrchClient, OrchestratorError is inside the try block but only OrchestratorError is caught. In any environment where egg_lib.orch_client isn't importable (currently impossible in the sandbox, but the lazy-import placement implies defensiveness), the verb crashes. Either move the import to module level (the rest of the file imports OrchestratorError at top via the egg_lib.orch_cli's own usage pattern) or add ImportError to the except clause.

3. Test gap: invalid EGG_PIPELINE_ID env path is unexercised. The new tests cover orchestrator-unreachable and well-formed env, but none cover the validate_id-fail case. Adding one would have caught finding #1.

4. Doc example shows a redundant positional. (sandbox/agent-config/rules/overseer.md:79)

The example writes consult-advisor "$EGG_PIPELINE_ID" even though the line directly below explains the verb auto-resolves from EGG_PIPELINE_ID. Since the overseer is the canonical caller and EGG_PIPELINE_ID is always set in that context, the positional is noise — drop it from the example, or invert the wording to "passing the positional explicitly when calling outside an env-scoped context."

5. Forward-looking nit on the duck-typed config. SimpleNamespace(overseer_advisor_model=model) is fine today because consult_advisor reads exactly one attribute. If the advisor ever grows another config.* read, the duck-typed surface will silently fall back to AttributeError or implicit defaults. Worth a one-line comment noting "extend SimpleNamespace here when consult_advisor reads more config.* attributes," or fabricating a real PipelineConfig so any new field is backed by the model's defaults rather than missing.

— Authored by egg

[egg-reviewer]

Re-review of delta 3aba7b4..026502c: the single follow-up commit lands the fixes from my prior advisory feedback (graceful fallback on malformed EGG_PIPELINE_ID instead of validate_id's sys.exit(1), ImportError added to the except, doc example deduped to drop the redundant positional, duck-type extension note, and a new test for the malformed-id path). No agent-mode design concerns — still a focused config-plumbing fix that threads an existing alias through an existing CLI verb without introducing pre-fetching, structured-for-humans output, post-processing, or direct LLM calls.

— Authored by egg

[egg-reviewer]

Re-review of PR #2158 (commits 3aba7b4 → 026502c)

The new commit 026502c addresses four of the five non-blocking findings from the prior review correctly:

• Malformed-id handling (sandbox/egg_lib/orch_cli.py:1826,1841-1849) — the explicit _SAFE_ID_PATTERN.match pre-check + elif warning branch is the right fix; replacing validate_id(pid, "pipeline_id") with quote(pid, safe="") preserves the original behaviour now that the regex check has been hoisted. ✓
• Test coverage for malformed id (sandbox/tests/test_egg_orch_overseer_consult_advisor.py:414-448) — the _explode patch is solid defence-in-depth: it asserts OrchClient is never instantiated on the bad-id path, so any future regression that drops the regex pre-check fails loudly. ✓
• Doc example (sandbox/agent-config/rules/overseer.md:79-84) — redundant positional dropped, prose now correctly describes auto-resolution + the optional positional + the unreachable-orchestrator fallback. ✓
• Forward-looking SimpleNamespace comment (sandbox/egg_lib/orch_cli.py:1821-1823) — clear, points the next editor at the right line. ✓

Blocking-ish: the ImportError "fix" doesn't actually work

sandbox/egg_lib/orch_cli.py:1827-1840:

try:
    from egg_lib.orch_client import OrchClient, OrchestratorError
    status = OrchClient().get_pipeline_status(quote(pid, safe=""))
    ...
except (OrchestratorError, ImportError) as exc:
    print(...)

If the from egg_lib.orch_client import … line raises ImportError, neither OrchClient nor OrchestratorError is bound. Python then evaluates the except-clause tuple (OrchestratorError, ImportError), can't resolve OrchestratorError, and raises NameError — which escapes the handler entirely. The verb crashes with NameError: name 'OrchestratorError' is not defined, not the warning + fallback the commit message promises. Confirmed locally:

$ python3 -c "
try:
    from nonexistent import Foo, Bar
except (Bar, ImportError) as exc:
    print('caught:', exc)"
NameError: name 'Bar' is not defined

The malformed-id fix is well-tested, but this branch has no test, which is why the bug slipped through. Two clean fixes:

# Option A: nested try, so ImportError binds nothing and is handled separately
try:
    from egg_lib.orch_client import OrchClient, OrchestratorError
except ImportError as exc:
    print(f"Warning: cannot import egg_lib.orch_client ({exc}); falling back to default advisor model", file=sys.stderr)
else:
    try:
        status = OrchClient().get_pipeline_status(quote(pid, safe=""))
        ...
    except OrchestratorError as exc:
        print(...)

# Option B: hoist the import to module-level (matching the rest of orch_cli.py)
# at line ~58 — no try/except needed at the call site.

Add a regression test that patches builtins.__import__ (or uses sys.meta_path finder) to make egg_lib.orch_client unimportable and asserts rc == 0 + the warning. Without that, any future re-arrangement of the lazy import will silently re-break this branch.

Severity

I'm flagging this as a non-blocking nit because the failure mode is unreachable in the sandbox today (the previous review noted as much). But the commit message explicitly claims "Catch ImportError alongside OrchestratorError … so an environment that cannot resolve the client module degrades to the default rather than crashing" — and that claim is false. If you want to honour the stated intent, please apply Option A or B before merge.

Other items reviewed

• The quote(pid, safe="") call is functionally identical to the previous validate_id(pid, "pipeline_id") (which itself returns quote(value, safe="") after the same regex). No behavioural drift.
• The _explode patch in the new test is intentionally a no-arg callable because the production path uses OrchClient(); consistent with the existing lambda: _StubClient() pattern in test_pipeline_id_from_env_resolves_advisor_model.
• Doc prose at line 84 correctly notes both fallback triggers (env+positional absent OR orchestrator unreachable). The malformed-id fallback isn't called out, but that's a corner case unlikely to hit overseer callers; non-blocking.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Lint/Python": 1}

[egg-reviewer]

Re-review of delta 026502c..9cbb273 (commits 3861dc5 + formatting fix 9cbb273): the follow-up addresses my prior advisory finding by replacing the broken combined except (OrchestratorError, ImportError) with a nested try (sandbox/egg_lib/orch_cli.py:1827-1853) and adds a regression test that patches builtins.__import__ to make egg_lib.orch_client unimportable (sandbox/tests/test_egg_orch_overseer_consult_advisor.py:450-491). No agent-mode design concerns — still a focused config-plumbing fix that threads an existing alias through an existing CLI verb without introducing pre-fetching, structured-for-humans output, post-processing pipelines, or direct LLM calls.

— Authored by egg

[egg-reviewer]

Re-review of PR #2158 (delta 026502c..9cbb273)

The two new commits (3861dc5 ImportError fix + 9cbb273 formatting) correctly address the blocking-ish concern from my prior review.

Fix verification

sandbox/egg_lib/orch_cli.py:1827-1853 — restructured into a nested try: the import is in its own try/except ImportError, with the OrchClient().get_pipeline_status(...) + OrchestratorError handling moved to the else: branch. This is option A from my prior review and resolves the NameError: name 'OrchestratorError' is not defined bug. Confirmed locally:

# OLD (broken):
try:
    from egg_lib.orch_client import OrchClient, OrchestratorError
    ...
except (OrchestratorError, ImportError) as exc:  # raises NameError on ImportError
    ...

# NEW (correct):
try:
    from egg_lib.orch_client import OrchClient, OrchestratorError
except ImportError as exc:
    print(...)
else:
    try:
        ...
    except OrchestratorError as exc:
        print(...)

I reproduced both failure and success modes against the actual import statement to confirm the bug was real and the fix works.

Test verification

sandbox/tests/test_egg_orch_overseer_consult_advisor.py:450-490 — the new test_orch_client_import_failure_falls_back_without_crashing patches builtins.__import__ with a delegating stub that raises ImportError only for name == "egg_lib.orch_client" and forwards everything else to the real __import__. Asserts rc == 0, config is None, and both warning fragments appear in stderr. The pass-through is the right pattern — cmd_overseer_consult_advisor does several other lazy imports (asyncio, SimpleNamespace, egg_overseer.advisor) that must succeed for the assertion path to be reached. Without the production fix, this test would fail with NameError, so it locks in the regression.

Comment quality

The block comment at orch_cli.py:1827-1830 explaining why nested try is required is appropriate — the structural choice is non-obvious and would invite a "simplification" back to the broken combined-except form without it. The Any = None annotation on advisor_config (line 1824) and the comment block at lines 1813-1823 (extension note for SimpleNamespace) carry over correctly from the prior commit.

Other items reviewed in delta

• Formatting-only commit 9cbb273 — touches only line wrapping in the new cmd_overseer_consult_advisor block; semantics unchanged.
• Warning message text matches the docstring contract ("falling back to default advisor model"); both stderr branches are now consistent in phrasing.
• No drift in exit-code semantics: import failure stays on the rc == 0 warn-and-fall-back path alongside the OrchestratorError and malformed-id paths, matching the verb's documented failure semantics.

No new findings. Approving.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
sandbox/egg_lib/orch_cli.py	Additive (semantic)	Combined PR's overseer_advisor_model PipelineConfig resolution (#2113) with main's recent_log_bytes_cap forwarding (#2120). Both kwargs now flow into consult_advisor.
sandbox/tests/test_egg_orch_overseer_consult_advisor.py	Additive	Kept all regression tests from both sides: the four #2113 tests (pipeline-id config resolution, env-var fallback, status-failure fallback, malformed-id fallback, import-failure fallback, positional-arg parser) and main's recent_log_bytes_cap flag-forwarded test. Extended _make_args to accept both pipeline_id and recent_log_bytes_cap kwargs.

Verification:

• pytest sandbox/tests/test_egg_orch_overseer_consult_advisor.py shared/tests/test_overseer_advisor.py — 69 passed
• ruff check — clean on both files

Please review: The two changes were genuinely additive — #2113 wires config=advisor_config and #2120 wires recent_log_bytes_cap=recent_log_bytes_cap into the same consult_advisor call. The merged call passes both. No semantic overlap between the changes.

— Authored by egg

[egg-reviewer]

Re-review of delta since 9cbb273 (the conflict-resolution merge f652971, since squashed into ab840a0): the only change is an additive merge with main that combines this PR's overseer_advisor_model PipelineConfig resolution (sandbox/egg_lib/orch_cli.py:1813-1862) with main's recent_log_bytes_cap forwarding (#2120) — both kwargs now thread through to consult_advisor (sandbox/egg_lib/orch_cli.py:1865-1875). The nested try / else import-failure structure I approved last round is preserved verbatim, and the test file additively keeps regression coverage from both sides. No agent-mode design concerns — still a focused config-plumbing fix with no pre-fetching, no structured-for-humans output, no post-processing pipelines, no direct LLM calls, and no hardcoded model IDs (the advisor model is resolved from a config alias).

— Authored by egg

[egg-reviewer]

Re-review of PR #2158 (delta 9cbb273..f652971)

The only change since my last approval is the conflict-resolution merge with main (commit f652971), which combines this PR's overseer_advisor_model PipelineConfig resolution with main's recent_log_bytes_cap forwarding (#2120).

Merge resolution verified

sandbox/egg_lib/orch_cli.py:1864-1875 — both kwargs now flow into the same consult_advisor call:

recent_log_bytes_cap = getattr(args, "recent_log_bytes_cap", None)
verdict = asyncio.run(
    consult_advisor(
        ...
        config=advisor_config,                 # this PR (#2113)
        recent_log_bytes_cap=recent_log_bytes_cap,  # main (#2120)
    )
)

The nested try / else import-failure structure I approved last round (orch_cli.py:1827-1853) is preserved verbatim, the _SAFE_ID_PATTERN malformed-id pre-check is intact, and the duck-typed SimpleNamespace(overseer_advisor_model=model) resolution still runs only on the safe-id path.

sandbox/tests/test_egg_orch_overseer_consult_advisor.py:125-140 — _make_args correctly extended to accept both pipeline_id and recent_log_bytes_cap. Both sides' regression tests are kept (parser tests for --recent-log-bytes-cap, test_recent_log_bytes_cap_flag_forwarded, plus all six pipeline_id tests). Local run: python3 -m pytest sandbox/tests/test_egg_orch_overseer_consult_advisor.py shared/tests/test_overseer_advisor.py → 69 passed.

The merge claim ("genuinely additive — no semantic overlap") holds: the two changes hit orthogonal kwargs, the SimpleNamespace branch is untouched by #2120, and the CLI-flag branch is untouched by this PR.

Non-blocking: SimpleNamespace not extended to the new byte-cap field

#2120 made consult_advisor (shared/egg_overseer/advisor.py:239-244) read a second config.* attribute as a fallback when the explicit recent_log_bytes_cap arg is None:

if recent_log_bytes_cap is None:
    recent_log_bytes_cap = (
        getattr(config, "overseer_advisor_recent_log_bytes_cap", None)
        if config is not None
        else None
    )

The orchestrator's status endpoint already exposes overseer_advisor_recent_log_bytes_cap (orchestrator/routes/pipelines.py:2724-2726), so the value is on the wire. But the SimpleNamespace this PR builds at orch_cli.py:1846 only sets overseer_advisor_model, so when a pipeline-id path hits with no --recent-log-bytes-cap flag, the PipelineConfig byte-cap value is silently ignored and consult_advisor falls through to the 256 KiB module default.

This isn't a regression from main — main passed config=None, so the byte-cap PipelineConfig field was already dead through this CLI verb — and the merge resolution is faithful to both sides. But it's exactly the situation the comment block at orch_cli.py:1820-1823 (added by this PR) was warning about:

NOTE: extend SimpleNamespace below if consult_advisor ever reads more config.* attributes; the duck-typed surface silently falls back to AttributeError today.

Worth a one-line tweak in the merge to extend the namespace, e.g.:

cfg_dict = status.get("config") if isinstance(status, dict) else None
if isinstance(cfg_dict, dict):
    model = cfg_dict.get("overseer_advisor_model")
    cap = cfg_dict.get("overseer_advisor_recent_log_bytes_cap")
    if model or cap is not None:
        advisor_config = SimpleNamespace(
            overseer_advisor_model=model or "opus",
            overseer_advisor_recent_log_bytes_cap=cap,
        )

Plus a regression test along the lines of test_pipeline_id_resolves_advisor_model_from_config that returns {"overseer_advisor_recent_log_bytes_cap": 65536} from the stub status payload and asserts captured["recent_log_bytes_cap"] falls through to that value when the CLI flag is omitted. Without it, the merged contract has a quiet asymmetry: pipeline-config tuning works for the model knob but not for the byte-cap knob.

Non-blocking because the field was already dead through this path on main; a follow-up issue is fine.

Other items reviewed in delta

• The _explode patch in test_malformed_pipeline_id_falls_back_without_crashing still passes after the _make_args extension — neither pipeline_id nor recent_log_bytes_cap were added in a way that breaks the existing _StubClient / _explode patterns.
• No exit-code drift: all three documented failure modes (parse, I/O, runtime) keep their codes; the new _non_negative_int validator (from main) raises argparse.ArgumentTypeError which exits via argparse's own path, not through this verb's exit-code semantics.
• --recent-log-bytes-cap parser registration at orch_cli.py:3188-3199 matches the docstring contract (0 disables, negatives rejected, omitted defers to PipelineConfig / module default).

Approving — no blocking findings on the merge. Please consider folding the SimpleNamespace extension and matching test in before merge so the PipelineConfig byte-cap knob isn't shipped as silently-ignored dead code.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

15 previous review(s) hidden.