Skip to content

docs: update advisor CLI config resolution docs for bytes cap [doc-updater] - #2301

Merged
jwbron merged 1 commit into
mainfrom
egg/doc-update-advisor-bytes-cap-config-resolution
Apr 29, 2026
Merged

docs: update advisor CLI config resolution docs for bytes cap [doc-updater]#2301
jwbron merged 1 commit into
mainfrom
egg/doc-update-advisor-bytes-cap-config-resolution

Conversation

@james-in-a-box

Copy link
Copy Markdown
Contributor

Update documentation to reflect changes from 3a97c85 (PR #2296):

  • cmd_overseer_consult_advisor now reads both PipelineConfig.overseer_advisor_model and PipelineConfig.overseer_advisor_recent_log_bytes_cap via the orchestrator status endpoint
  • docs/reference/orchestrator-cli.md was already updated in Fix #2170: wire overseer_advisor_recent_log_bytes_cap through CLI consult-advisor path #2296, but docs/guides/pipeline-health-monitoring.md had two paragraphs (lines 434 and 537) that still only mentioned the model field
  • Both paragraphs now describe the full config resolution: model + 256 KiB byte cap, with fallback defaults when the pipeline ID is absent or lookup fails

Issue: none

Test Plan

  • Automated: no code changes; doc-only update
  • Manual: verify the two updated sentences match orchestrator-cli.md table entry for consult-advisor

Triggered by: #2296

Authored-by: egg

…dater]

Update documentation to reflect changes from 3a97c85:
- Both PipelineConfig fields (overseer_advisor_model and
  overseer_advisor_recent_log_bytes_cap) are now read via the CLI
  consult-advisor path; two spots in pipeline-health-monitoring.md
  only mentioned the model field.

Triggered by: #2296

Authored-by: egg

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Doc-only update propagating the overseer_advisor_recent_log_bytes_cap config-resolution wording from PR #2296 (which updated docs/reference/orchestrator-cli.md) to the two stale paragraphs in docs/guides/pipeline-health-monitoring.md.

Verified against the implementation:

  • sandbox/egg_lib/orch_cli.py::cmd_overseer_consult_advisor (line 1888) calls OrchClient().get_pipeline_status(...) and reads both overseer_advisor_model (line 1987) and overseer_advisor_recent_log_bytes_cap (line 1993) from the returned config dict, builds a SimpleNamespace, and hands it to consult_advisor. Matches the new wording.
  • The cap is not None check at line 1993 correctly preserves the 0 (disable) sentinel — relevant aside, but not something the doc needs to spell out at this level of detail.
  • Fallbacks fire when (a) pipeline-id is absent, (b) pipeline-id fails the safe-id regex, (c) egg_lib.orch_client import fails, (d) OrchClient().get_pipeline_status raises OrchestratorError. The doc's "absent or the lookup fails" covers all of these.
  • Default values match the claims: overseer_advisor_model="opus" (orchestrator/models.py:517) and overseer_advisor_recent_log_bytes_cap=256_000 (line 528).

Non-blocking

Minor unit inconsistency, pre-existing. The default cap is 256_000 bytes — that's 256 KB (decimal) or ≈250 KiB, not 256 KiB (which would be 262,144 bytes). This wording originates in orchestrator/models.py:535 ("256 KiB default") and docs/reference/orchestrator-cli.md:43, and PR #2296 propagated it to those locations. This PR is faithfully matching that wording, so flagging it here would be out of scope. Worth fixing in a follow-up — either bump the default to 262_144 or change all three sites to "256 KB" / "256_000 bytes".

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box james-in-a-box Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — out of scope here, follow-up. Confirming the reviewer's read: this PR is intentionally propagating the exact wording from #2296 to keep the three sites consistent (orchestrator/models.py:535, docs/reference/orchestrator-cli.md:43, and now the two paragraphs in docs/guides/pipeline-health-monitoring.md). Changing "256 KiB" → "256 KB" / "256_000 bytes" — or alternatively bumping the default to 262_144 — is a one-line decision that should land in its own PR touching all three sites together. Filing as a follow-up rather than expanding this PR's scope.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author
egg is addressing review feedback...

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg feedback addressed. View run logs

3 previous review(s) hidden.

@jwbron
jwbron merged commit f351e90 into main Apr 29, 2026
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant