feat(orchestrator): Stage enum + runtime_profile.stages config (slice 1 of #519)#538
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
5addcf2for PR #538
Review record:
5b91d748-80df-46e9-b926-a7e202a87c34
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/config/models.py:298 | BLOCKING | RuntimeProfileConfig.default and the stages values are unconstrained strs, so invalid backends like "cluade" or empty strings pass startup validation even though this feature is supposed to fail bad routing config early. In practice that means a malformed runtime_profile loads successfully and only fails later when some stage is dispatched, or silently falls through because resolve_runtime_for_stage() treats falsy strings as “unset”. These fields need the same backend validation/normalization as orchestrator.runtime_backend, and the tests should cover bad backend values instead of only bad stage keys. |
Non-blocking Suggestions
None.
Design Notes
The split between a closed Stage enum and a small resolver is a sensible primitive for later wiring. The weak spot is the new config contract: stage names are validated, but backend names are not, so the abstraction currently exposes an unsafe configuration surface.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Follow-up architectural decisions for #519 slices 2–8The post-merge checklist on this PR lists the follow-up slices but does not enumerate the architectural decisions each slice depends on. Spelling those out here so reviewers and the next contributor see the open questions explicitly. This PR (slice 1) is intentionally non-controversial — Slice 2 —
|
| Slice | Hard dependency | Open architectural decisions |
|---|---|---|
| 2 (mcp doctor Medium) | Slice 1 | (1) per-runtime check pattern, (2) failure mode, (3) output format |
| 3 (--probe Deep) | Slice 2 | (1) probe shape, (2) timeout, (3) network probe gating |
| 4–7 (per-runtime mappings) | Slice 1 + #505 merged | (1) profile naming convention, (2) mapping location, (3) validation hook |
| 8 (docs + acceptance) | Slices 1–7 + #505 | (1) runtime_id event shape, (2) acceptance assertion, (3) docs file location |
This PR (slice 1) does not require any of the above to be decided yet. It is the foundation; the decisions land at their slice boundary.
Why this matters
If this PR is approved without explicit acknowledgement of the open decisions above, the next contributor (or me, in a follow-up session) will pick some answer for each by default. Putting the questions on the record now means:
- Reviewers can pre-empt the bad answers ("don't use Option A because…").
- The decisions become explicit choices rather than emergent defaults.
- Subsequent slice PRs can reference this comment for "this is why we picked X" rather than re-litigating.
Happy to fold this into the PR body if maintainers prefer it on the body itself.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
5fc2ff6for PR #538
Review record:
d8b49ef2-fd20-41a9-baf7-d4df8ba1b003
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/config/models.py:279 | BLOCKING | RuntimeProfileConfig hard-rejects claude_code by limiting VALID_RUNTIME_BACKENDS to {"claude","codex","opencode","hermes","gemini"}. That makes the new runtime_profile surface unable to express the documented evaluate -> Claude Code stage mapping from this PR’s own stage contract, so a valid-looking config will fail at startup instead of routing evaluation through the intended runtime. The new tests also cement the narrower behavior by only accepting "claude" in stage mappings. |
Non-blocking Suggestions
None.
Design Notes
The slice is appropriately narrow: enum + config schema + pure resolution helper. The main issue is the config contract is stricter than the runtime vocabulary the design text points contributors toward, so the foundation is not yet safe to build later slices on.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Autopilot follow-up: pushed review-addressing commits and confirmed the GitHub check suite is green on the latest head. Current merge state is clean; remaining CHANGES_REQUESTED status appears to be awaiting fresh bot/human re-review rather than failing CI. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
01e9039for PR #538
Review record:
47dee817-d541-47ac-ac16-fa9c80c21aa3
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/config/models.py:320 | BLOCKING | RuntimeProfileConfig._validate_stage_keys() imports ouroboros.orchestrator.stage, but in Python that executes ouroboros/orchestrator/__init__.py first. This validator therefore pulls in the full orchestrator package graph just to validate YAML keys, despite the comment claiming it avoids cycles. Because orchestrator/__init__.py eagerly imports many runtime modules, config parsing for runtime_profile can now fail with unrelated orchestrator/optional-dependency import errors before validation even runs. Given the package already has explicit “partial installs” handling in orchestrator/__init__.py, this is a real startup regression for config-only code paths. The stage constants/error should live in an import-light module, or the validator should avoid package-level orchestrator imports entirely. |
Non-blocking Suggestions
None.
Design Notes
The slice boundary is reasonable: enum + config schema + pure resolution helper. The main architectural problem is that the config layer is no longer isolated; stage validation currently depends on importing the orchestrator package instead of a minimal stage-definition module.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
015fb2cfor PR #538
Review record:
ce73c031-3a5b-4d5c-8433-e16698c76fca
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
src/ouroboros/config/models.py:279[warning]RuntimeProfileConfigvalidates stage backends against a hard-coded set that excludes backend spellings the orchestrator already accepts (codex_cli,opencode_cli,hermes_cli,gemini_cliin runtime_factory.py). That makes the new config surface reject values the runtime layer can resolve today, so a valid existing backend identifier copied intoruntime_profile.defaultorruntime_profile.stages.*will fail startup validation instead of running. The validator should share the same accepted backend vocabulary as runtime resolution, or normalize through that resolver.
Non-blocking Suggestions
| 1 | src/ouroboros/config/models.py:282 | refactoring | VALID_RUNTIME_PROFILE_STAGE_KEYS duplicates the stage vocabulary already defined in stage.py. Reusing Stage/VALID_STAGE_KEYS would remove one drift point between the schema and resolver. |
| 2 | src/ouroboros/config/models.py:299 | documentation | The docstring says unknown stage keys raise UnknownStageError, but this validator currently raises ValueError directly and never calls parse_stage(). The rendered validation message is fine; the exception type claim is what is inaccurate. |
Design Notes
The slice is intentionally narrow and the resolution helper is simple enough, but the config schema should not invent its own backend contract. This foundation is only safe if stage/backend validation and runtime resolution stay sourced from the same vocabulary.
Policy Notes
- No in-scope blocking findings remained after policy filtering; downgraded verdict accordingly.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Q00
left a comment
There was a problem hiding this comment.
The stage enum and per-stage runtime resolution look coherent in isolation, but I am requesting changes because this PR collides with PR #505 on the public orchestrator.runtime_profile contract.
Here runtime_profile is an object used for stage routing. In #505, the same key is a backend profile string used to select the Codex --profile mapping. Merging both as-is creates an incompatible config contract and a real conflict in src/ouroboros/config/models.py.
Please coordinate the stack shape before merge, for example runtime_profile.backend_profile plus runtime_profile.stages, or choose a distinct key for the Codex backend profile.
If #505 is being abandoned or renamed elsewhere, please reply and I will re-check.
Coordinate PR Q00#538's stage-routing table with PR Q00#505's backend-native profile selection by reserving runtime_profile.backend_profile and keeping default/stages for stage routing. Move the stage vocabulary to an import-light module so config validation can share it without importing the full orchestrator package graph. Constraint: PR Q00#538 must coexist with PR Q00#505's Codex backend profile contract while preserving stage routing. Rejected: Keeping runtime_profile as stage-routing-only object | conflicts with PR Q00#505's string contract. Rejected: Duplicating stage keys inside config models | drifts from the canonical stage enum. Confidence: high Scope-risk: moderate Directive: Keep backend-native profile selection under runtime_profile.backend_profile and stage routing under runtime_profile.default/stages; import stage vocabulary from ouroboros.orchestrator_stage for config validation. Tested: uv run ruff format --check src/ouroboros/config/models.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py tests/unit/orchestrator/test_stage_resolution.py Tested: uv run ruff check src/ouroboros/config/models.py src/ouroboros/config/__init__.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py tests/unit/orchestrator/test_stage_resolution.py tests/unit/config/ Tested: uv run mypy src/ouroboros/config/models.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py Tested: uv run pytest tests/unit/orchestrator/test_stage_resolution.py tests/unit/config/ -q Not-tested: full repository test suite locally Co-authored-by: OmX <omx@oh-my-codex.dev>
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
82043ebfor PR #538
Review record:
b4b9e4ea-183a-4cca-a920-f2da7c9460d1
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
The slice is narrowly scoped and internally coherent: stage vocabulary is isolated in a low-dependency module, config validation fails early for bad stage/backend values, and the compatibility re-export preserves the intended import surface. I did not find a blocking contract mismatch within the changed files.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Move the Codex worker selector under runtime_profile.backend_profile while preserving the old string shorthand and reserving the stage-routing object fields used by the parallel runtime_profile stack. Constraint: maintainer review found PR Q00#505 and PR Q00#538 assigned incompatible meanings to orchestrator.runtime_profile. Rejected: keeping runtime_profile as a Codex-only string | that blocks the stage-routing object contract from sharing the public config key. Confidence: high Scope-risk: moderate Directive: backend-native profile selectors must live under runtime_profile.backend_profile; stage routing owns runtime_profile.default and runtime_profile.stages. Tested: uv run ruff check targeted runtime-profile files; uv run ruff format --check targeted runtime-profile files; uv run pytest tests/unit/config/test_loader.py tests/unit/cli/test_setup.py tests/unit/orchestrator/test_runtime_factory.py tests/unit/orchestrator/test_codex_cli_runtime.py tests/unit/providers/test_factory.py tests/unit/providers/test_codex_cli_adapter.py -q; uv run mypy targeted runtime-profile files
|
@Q00 re-review ping: I re-checked PR #538 against the latest head What this PR does now:
Why the previous review concerns appear addressed:
Fresh verification I ran locally on this worktree:
GitHub status at re-check time:
Based on the above, I think this PR is merge-ready after maintainer re-review/dismissal of the stale requested-changes review. |
Coordinate PR Q00#538's stage-routing table with PR Q00#505's backend-native profile selection by reserving runtime_profile.backend_profile and keeping default/stages for stage routing. Move the stage vocabulary to an import-light module so config validation can share it without importing the full orchestrator package graph. Constraint: PR Q00#538 must coexist with PR Q00#505's Codex backend profile contract while preserving stage routing. Rejected: Keeping runtime_profile as stage-routing-only object | conflicts with PR Q00#505's string contract. Rejected: Duplicating stage keys inside config models | drifts from the canonical stage enum. Confidence: high Scope-risk: moderate Directive: Keep backend-native profile selection under runtime_profile.backend_profile and stage routing under runtime_profile.default/stages; import stage vocabulary from ouroboros.orchestrator_stage for config validation. Tested: uv run ruff format --check src/ouroboros/config/models.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py tests/unit/orchestrator/test_stage_resolution.py Tested: uv run ruff check src/ouroboros/config/models.py src/ouroboros/config/__init__.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py tests/unit/orchestrator/test_stage_resolution.py tests/unit/config/ Tested: uv run mypy src/ouroboros/config/models.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py Tested: uv run pytest tests/unit/orchestrator/test_stage_resolution.py tests/unit/config/ -q Not-tested: full repository test suite locally Co-authored-by: OmX <omx@oh-my-codex.dev>
82043eb to
240bf55
Compare
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
240bf55for PR #538
Review record:
012f42ec-cc2e-4cdc-8adf-11c4e4ac3c7b
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
The slice stays narrow and coherent: the stage vocabulary, config schema, and resolution helper are separated cleanly, and the lightweight ouroboros.orchestrator_stage module avoids pulling config validation into the full orchestrator import graph.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
… 1 of Q00#519) Adds the per-stage runtime selection primitive the Agent OS diagram agreed in Q00#476 calls for: each pipeline stage (interview / execute / evaluate / reflect) can be served by a different harness. This first slice ships the binding-table primitive plus the resolution helper; subsequent slices wire mcp doctor (5.2/5.3) and the per-runtime mappings (5.4-5.7). What's added: - src/ouroboros/orchestrator/stage.py * ``Stage`` StrEnum with the four agreed members. Adding a new stage requires an explicit, justified PR (the same narrow- membership rule the maintainer alignment in Q00#476 Q1 applied to AgentRuntimeContext). * ``parse_stage(value)`` raises ``UnknownStageError`` on unknown keys; the message names the offending key plus the valid set so operators see typos at startup, not mid-workflow. * ``resolve_runtime_for_stage(stage, *, stages, default, fallback)`` resolves in the order locked by the Q00#519 sub-thread: 1. stages[stage] explicit per-stage mapping 2. default — runtime_profile.default 3. fallback — orchestrator.runtime_backend (today's behaviour) - src/ouroboros/config/models.py * ``RuntimeProfileConfig`` Pydantic model with two fields: - ``default``: Optional[str] - ``stages``: dict[str, str] (validated against ``VALID_STAGE_KEYS``) * Field validator raises pydantic ValidationError on unknown stage keys at startup so ``[orchestrator.runtime_profile.stages] interveiw = "codex"`` fails fast. * ``OrchestratorConfig.runtime_profile`` added as ``RuntimeProfileConfig | None = None``. Default ``None`` is byte-for-byte today's behaviour — that is the BC commitment carried forward from PR Q00#505. - src/ouroboros/config/__init__.py * Re-exports ``RuntimeProfileConfig`` alongside the existing ``OrchestratorConfig`` re-export. - tests/unit/orchestrator/test_stage_resolution.py (13 cases) * Stage enum has exactly four members; VALID_STAGE_KEYS matches. * parse_stage rejects "interveiw" with a helpful message that enumerates the valid set. * resolve_runtime_for_stage picks stages → default → fallback in order. * runtime_profile=None preserves byte-for-byte the orchestrator's runtime_backend. * RuntimeProfileConfig rejects unknown stage keys at validation via pydantic ValidationError that surfaces the typo + valid set. * Legacy OrchestratorConfig construction (no runtime_profile) remains valid; runtime resolution falls through to runtime_backend. Verification: - uv run ruff check (clean after one --fix iteration) - uv run ruff format (2 files reformatted, no logic change) - uv run pytest tests/unit/orchestrator/test_stage_resolution.py (13 passed) - uv run pytest tests/unit/config/ regression (145 passed) Stack note: This PR is independent of Sprints 1/2/3/4 work. It branches off main and lands on its own track. Subsequent Q00#519 slices (mcp doctor extensions, per-runtime mappings, docs/acceptance) build on the ``RuntimeProfileConfig`` and ``resolve_runtime_for_stage`` introduced here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Constraint: Addressed current blocking PR review findings without widening the feature scope. Confidence: high Scope-risk: narrow Directive: Preserve the new regression tests when modifying this contract. Tested: uv run ruff check src/ouroboros/config/models.py tests/unit/orchestrator/test_stage_resolution.py; uv run pytest tests/unit/orchestrator/test_stage_resolution.py Not-tested: Full repository matrix; CI remains authoritative.
Constraint: Stage routing validation must reject typos without excluding documented runtime aliases. Confidence: high Scope-risk: narrow Directive: Keep runtime_profile backend validation aligned with the stage contract docs. Tested: uv run ruff check src/ouroboros/config/models.py tests/unit/orchestrator/test_stage_resolution.py; uv run pytest tests/unit/orchestrator/test_stage_resolution.py Not-tested: Full repository matrix; CI remains authoritative.
Coordinate PR Q00#538's stage-routing table with PR Q00#505's backend-native profile selection by reserving runtime_profile.backend_profile and keeping default/stages for stage routing. Move the stage vocabulary to an import-light module so config validation can share it without importing the full orchestrator package graph. Constraint: PR Q00#538 must coexist with PR Q00#505's Codex backend profile contract while preserving stage routing. Rejected: Keeping runtime_profile as stage-routing-only object | conflicts with PR Q00#505's string contract. Rejected: Duplicating stage keys inside config models | drifts from the canonical stage enum. Confidence: high Scope-risk: moderate Directive: Keep backend-native profile selection under runtime_profile.backend_profile and stage routing under runtime_profile.default/stages; import stage vocabulary from ouroboros.orchestrator_stage for config validation. Tested: uv run ruff format --check src/ouroboros/config/models.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py tests/unit/orchestrator/test_stage_resolution.py Tested: uv run ruff check src/ouroboros/config/models.py src/ouroboros/config/__init__.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py tests/unit/orchestrator/test_stage_resolution.py tests/unit/config/ Tested: uv run mypy src/ouroboros/config/models.py src/ouroboros/orchestrator/stage.py src/ouroboros/orchestrator_stage.py Tested: uv run pytest tests/unit/orchestrator/test_stage_resolution.py tests/unit/config/ -q Not-tested: full repository test suite locally Co-authored-by: OmX <omx@oh-my-codex.dev>
240bf55 to
6168447
Compare
Summary
First slice of #519 (Stage-Runtime mapping). Adds the per-stage runtime selection primitive the Agent OS diagram agreed in #476 calls for: each pipeline stage (
interview/execute/evaluate/reflect) can be served by a different harness.This PR ships only the binding-table primitive — the
Stageenum, theRuntimeProfileConfigmodel, and the resolution helper. Subsequent slices wire mcp doctor extensions (5.2/5.3), the per-runtime mappings (5.4–5.7), and docs/acceptance (5.8).Decisions captured (from the #519 sub-thread sign-off)
interview,execute,evaluate,reflect. Adding a member requires an explicit justified PR.stages[stage]→default→fallback(orchestrator.runtime_backend)runtime_profile=Nonepydantic.ValidationErrorat startup, surfacing the typo + valid set. Mid-workflow surprises ruled out.Changes
src/ouroboros/orchestrator/stage.py(new) —Stageenum,parse_stage,resolve_runtime_for_stage,UnknownStageError,VALID_STAGE_KEYS.src/ouroboros/config/models.py—RuntimeProfileConfigPydantic model with field validator;runtime_profile: RuntimeProfileConfig | None = NoneonOrchestratorConfig.src/ouroboros/config/__init__.py— re-exportRuntimeProfileConfig.tests/unit/orchestrator/test_stage_resolution.py— 13 cases.Verification
uv run ruff checkuv run ruff formatuv run pytest tests/unit/orchestrator/test_stage_resolution.pyuv run pytest tests/unit/config/(regression)Pre-merge checklist
Stageenum with exactly 4 membersparse_stagerejects unknown values with helpful messageresolve_runtime_for_stagepicksstages → default → fallbackin orderruntime_profile=Nonepreserves byte-for-byte today's behaviourstageskey rejected at config validationOrchestratorConfiglegacy construction unchangedPost-merge checklist
ouroboros mcp doctorwith Medium-tier validation that prints the resolved stage→runtime table and rejects configs whose runtimes are not on$PATH--probeflag for Deep validation (per-runtime ping smoke)Rollback
Pure additive: a new module, a new config block, two new re-exports.
runtime_profile=Nonedefault means existing installs see no behaviour change. Rollback removes the kwarg + helpers; legacy code path is unchanged.