docs: update consult-advisor docs for configurable model [doc-updater] - #2171
Conversation
Document that egg-orch overseer consult-advisor now reads PipelineConfig.overseer_advisor_model from the orchestrator status endpoint when a pipeline ID is provided, rather than always using the Opus default. Triggered by: ab840a0 (overseer: wire overseer_advisor_model through consult-advisor CLI (#2113) (#2158))
There was a problem hiding this comment.
Review
Doc-only PR updating docs/guides/pipeline-health-monitoring.md and docs/reference/orchestrator-cli.md to reflect the runtime wiring landed in ab840a0. Verified the technical claims against the implementation:
- ✅
PipelineConfig.overseer_advisor_modelexists atorchestrator/models.py:420withdefault="opus". - ✅ The status endpoint at
orchestrator/routes/pipelines.py:2722-2723exposesconfig.overseer_advisor_modelexactly as the CLI handler expects. - ✅
sandbox/egg_lib/orch_cli.py:1813-1862resolvespipeline_idfrom the positional arg orEGG_PIPELINE_ID, callsOrchClient().get_pipeline_status(...), readsstatus["config"]["overseer_advisor_model"], and constructs a duck-typedSimpleNamespaceforconsult_advisor. Falls back toconfig=None(→"opus"inshared/egg_overseer/advisor.py:238) on missing/malformed pipeline-id,OrchestratorError, orImportError. - ✅ The CLI table entry's wording ("falls back to the
opusdefault if the pipeline ID is absent or the lookup fails") matches the three explicit fallback branches in code. - ✅ Removing the bare "Opus" qualifier in the two call sites is correct — those sentences described the underlying
run_agent_asyncinvocation, which is now model-agnostic.
Non-blocking observations
docs/guides/pipeline-health-monitoring.md:420still says "an Opus 4.6 advisor is invoked only when both ..." This is technically still true for the default config (the"opus"alias resolves to Opus 4.6), but now misleading for callers who overrideoverseer_advisor_model(e.g., toclaude-opus-4-7as one of the tests does). Since the PR is already in this file rephrasing the same concept, "Opus-tier advisor" or "configured advisor model" would have been a natural one-line follow-on.- The other "Opus" mentions in the file (lines 29, 245, 286, 294-296, 339-340, 425) are tier-class descriptions ("Sonnet/Opus decision-maker") or accurate
default="opus"references — fine to leave.
No blocking issues.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address reviewer's non-blocking observation: replace 'an Opus 4.6 advisor' with 'the configured advisor model (PipelineConfig.overseer_advisor_model, defaulting to the opus alias)' to match the configurable model wiring this PR is documenting elsewhere in the same file.
|
Agreed. Pushed cdd9222 generalizing line 420 from "an Opus 4.6 advisor" to "the configured advisor model ( — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… docs/reference/orchestrator-cli.md Both sides modified the consult-advisor row: - HEAD: generalize 'Opus advisor' wording and add note about PipelineConfig.overseer_advisor_model resolution - origin/main: add --recent-log-bytes-cap flag documentation Resolution: include both — kept generalized 'advisor' wording, added the new flag to the signature, and merged both explanatory paragraphs.
Conflict Resolution SummaryResolved merge conflicts with
Auto-merged (no manual resolution): Please review: the merged — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conflict Resolution SummaryNo action needed — this branch is already up to date with
The previous merge commit ( — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-Review
The delta since c5612a06 is two commits: cdd9222 (addresses my prior non-blocking note on line 421) and 5c3fc342 (merge against main resolving docs/reference/orchestrator-cli.md).
Prior feedback resolved. Line 421 in docs/guides/pipeline-health-monitoring.md now reads "the configured advisor model (PipelineConfig.overseer_advisor_model, defaulting to the opus alias)" — matches the rephrasing this PR already does at lines 434 and 537.
Merge resolution verified. The consult-advisor row in docs/reference/orchestrator-cli.md correctly preserves both:
- This PR's generalized "advisor" wording + the model-resolution paragraph (
When [<id>] (or EGG_PIPELINE_ID) is set, the verb reads PipelineConfig.overseer_advisor_model ...). - #2168's
[--recent-log-bytes-cap <n>]flag in the signature + the byte-cap paragraph (--recent-log-bytes-cap overrides the PipelineConfig.overseer_advisor_recent_log_bytes_cap value ...).
The two paragraphs read in the right order (model resolution → byte cap), so the row stays coherent.
Technical claims re-verified against the code:
orchestrator/models.py:420—overseer_advisor_model: str = Field(default="opus")✅orchestrator/routes/pipelines.py:2723— status endpoint exposesoverseer_advisor_model✅sandbox/egg_lib/orch_cli.py:1813–1862— pipeline-id gated; resolves viaOrchClient().get_pipeline_status(...)readingstatus["config"]["overseer_advisor_model"]; duck-types viaSimpleNamespace; three explicit fallback branches (ImportError, malformed pid,OrchestratorError) ✅shared/egg_overseer/advisor.py:238—config=None → "opus"✅docs/guides/pipeline-health-monitoring.md:245—overseer_advisor_modeltable entry preserved by the merge ✅
Non-blocking nit (carried over)
Line 537's fallback list ("falls back to opus when absent or the orchestrator is unreachable") doesn't mention the ImportError and malformed-pipeline-id branches that line 42 and line 434 do mention. Doesn't matter in practice — those are pathological paths — but worth noting for future doc passes if anyone is doing a final wording sweep.
No blocking issues. The doc-update is accurate, the prior feedback was addressed substantively (not superficially), and the merge resolution is clean.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Reviewer's carry-over non-blocking nit: line 537 said 'falls back to opus when absent or the orchestrator is unreachable' — narrower than lines 42 and 434 which both use 'lookup fails', encompassing all three fallback branches (ImportError, malformed pipeline ID, OrchestratorError).
|
Agreed. Pushed 53efb9f harmonizing line 537's fallback wording to "falls back to — Authored by egg |
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
The delta since 5c3fc342 is a single commit (53efb9f) touching one line of docs/guides/pipeline-health-monitoring.md.
Prior non-blocking nit resolved. Line 537 now reads "falls back to opus when absent or the lookup fails" — matching line 434 in the same file and line 42 of docs/reference/orchestrator-cli.md. The previous wording ("the orchestrator is unreachable") only described the OrchestratorError branch; the new wording correctly covers all four fallback paths in sandbox/egg_lib/orch_cli.py:1813–1862:
ImportErroron theOrchClientimport (line 1833)- Malformed pipeline-id (
_SAFE_ID_PATTERNmiss on line 1854) OrchestratorErrorfromget_pipeline_status(line 1848)- Status response missing
config.overseer_advisor_model(lines 1842–1846 —cfg_dictis None ormodelis falsy)
"Lookup fails" is the correct umbrella term for all four.
Scope and risk. Doc-only, one-line change, no semantic shift in machine-readable artifacts. No CI/runtime impact.
Verified no regressions in the surrounding text. Line 245's overseer_advisor_model table entry is unchanged; line 421's "configured advisor model" rephrasing from cdd9222 is preserved; the consult-advisor row in docs/reference/orchestrator-cli.md retains both the model-resolution paragraph and #2168's --recent-log-bytes-cap paragraph in the right order.
No blocking issues.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
No actionable feedback in the latest review ( — Authored by egg |
|
egg feedback addressed. View run logs 12 previous review(s) hidden. |
#2171) * docs: update consult-advisor docs for configurable model Document that egg-orch overseer consult-advisor now reads PipelineConfig.overseer_advisor_model from the orchestrator status endpoint when a pipeline ID is provided, rather than always using the Opus default. Triggered by: ab840a0 (overseer: wire overseer_advisor_model through consult-advisor CLI (#2113) (#2158)) * docs: generalize advisor model reference at line 420 Address reviewer's non-blocking observation: replace 'an Opus 4.6 advisor' with 'the configured advisor model (PipelineConfig.overseer_advisor_model, defaulting to the opus alias)' to match the configurable model wiring this PR is documenting elsewhere in the same file. * Harmonize line 537 fallback wording to 'lookup fails' Reviewer's carry-over non-blocking nit: line 537 said 'falls back to opus when absent or the orchestrator is unreachable' — narrower than lines 42 and 434 which both use 'lookup fails', encompassing all three fallback branches (ImportError, malformed pipeline ID, OrchestratorError). --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Update documentation to reflect changes from ab840a0 (overseer: wire overseer_advisor_model through consult-advisor CLI (#2113) (#2158)):
docs/reference/orchestrator-cli.md: note that[<id>]/EGG_PIPELINE_IDtriggers resolution ofPipelineConfig.overseer_advisor_model; falls back toopusdefaultdocs/guides/pipeline-health-monitoring.md: update two places that called the advisor call an "Opus call" — it now uses the configured model aliasTriggered by: ab840a084
Test Plan
egg-orch overseer consult-advisor --helpAuthored-by: egg