feat(orchestrator): Agent OS runtime_profile (Codex backend, supersedes #488)#505
Conversation
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
b2aaf83for PR #505
Review record:
6ab89508-f4c7-417a-9619-242edf49addb
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/cli/commands/setup.py:233 | BLOCKING | _upsert_codex_worker_profile_section() rewrites [profiles.ouroboros-worker] to the bare managed header and drops every key already inside that table on each ouroboros setup --runtime codex rerun. That is a real config-loss bug because this PR’s user contract explicitly tells operators to put worker-specific overrides such as model, notify, and sandbox directly in that profile (docs/runtime-guides/codex.md:137). After the next setup/update, those overrides silently disappear and worker subprocesses stop using the intended isolation settings. |
Non-blocking Suggestions
| 1 | src/ouroboros/cli/commands/setup.py:414 | documentation | _print_codex_config_guidance() still says to use ~/.codex/config.toml “only for the Codex MCP/env hookup”, which now contradicts the new worker-profile feature and the runtime guide. |
| 2 | src/ouroboros/orchestrator/runtime_factory.py:77 | tests | The new get_runtime_profile() wiring into create_agent_runtime() is not covered in the changed tests. Existing runtime-factory tests assert permission/backend selection, but nothing verifies that a configured profile is actually propagated into CodexCliRuntime. |
| 3 | src/ouroboros/providers/factory.py:154 | tests | Likewise, the new runtime_profile propagation into CodexCliLLMAdapter has no factory-level regression test. The adapter unit tests cover _build_command(), but not the factory path that now reads config/env and injects the profile automatically. |
Design Notes
The runtime-profile mapping itself is small and sensible, and threading it through both the orchestrator runtime and Codex LLM adapter keeps the contract consistent. The main design problem is lifecycle ownership of the managed profile block: the setup command currently claims ownership of the same table the docs tell users to customize, which makes reruns destructive.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@Q00 — opened this as the `runtime_profile` replacement for #488, following your 4-step scope verbatim. Default-`None` regression-locked across both `_build_command` sites; ruff format/check, mypy (260 files), and the `tests/unit/{orchestrator,providers,config,cli}` slice (1727 passing) are all green locally. Ready for your final pass whenever — no rush. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
8d807edfor PR #505
Review record:
a67ea9bf-1b91-4bdb-a792-a3de976146d9
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/cli/commands/setup.py:235 | BLOCKING | _upsert_codex_worker_profile_section() deliberately replaces the entire [profiles.ouroboros-worker] block, including any existing keys or subtables. That is a merge blocker because the new Codex guide tells users to put their worker overrides directly in that section, so a routine re-run of ouroboros setup --runtime codex will silently wipe their configured model/sandbox/notify settings. This is user-config data loss in the primary setup path, and the new tests only cover an empty/idempotent block, not preservation of a populated worker profile. |
Non-blocking Suggestions
| 1 | src/ouroboros/cli/commands/setup.py:413 | Documentation | The CLI guidance still says to use ~/.codex/config.toml only for the MCP/env hookup, which now conflicts with the new worker-profile feature and the updated runtime guide. That messaging should be aligned so users know the worker profile also lives there. |
| 2 | src/ouroboros/providers/codex_cli_adapter.py:53 | Refactor | The runtime_profile -> codex_profile mapping now exists in both the orchestrator runtime and the LLM adapter. Centralizing it would reduce drift risk when new profile names are added. |
Design Notes
The change is scoped cleanly around one new orchestrator contract (runtime_profile) and threads it through the two Codex execution paths consistently. The weak point is config ownership: the docs present [profiles.ouroboros-worker] as user-customizable, but setup currently treats that same block as fully managed and destructive.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Q00
left a comment
Q00
left a comment
There was a problem hiding this comment.
Findings
- High:
src/ouroboros/cli/commands/setup.py:231replaces the entire[profiles.ouroboros-worker]table when setup runs. That conflicts with the docs telling users to customize that exact table indocs/runtime-guides/codex.md:137. Repro: addmodel,notify,sandboxunder[profiles.ouroboros-worker], rerun_register_codex_worker_profile(), and those keys are deleted. Since_setup_codex()calls this on every setup run atsrc/ouroboros/cli/commands/setup.py:465, upgrades/reconfiguration can silently wipe the worker profile and make--profile ouroboros-workereffectively empty.
Open Question
Is [profiles.ouroboros-worker] meant to be user-editable? If yes, _upsert_codex_worker_profile_section() should preserve existing keys and add a regression test for custom profile values. If no, the docs and inline comments need to point users to the supported customization path.
Verification
Ran targeted checks on the PR worktree: 43 passed. Also confirmed codex exec --help supports --profile on exec. No other blocking issue found in the runtime/adapter command wiring.
…ofile propagation Adds the missing regression coverage for the issues flagged on PR Q00#505 review: * setup.py: lock the new merge-preserving worker profile contract. - User-authored model/notify/sandbox keys plus a [profiles.ouroboros-worker.shell_environment_policy] subtable survive a setup rerun. - Multiple reruns after a user edit are byte-stable (no key loss AND no spurious additions on the first rerun, then idempotent afterwards). - The most adversarial layout -- a non-managed table abutting the worker header with no blank-line separator -- is handled without collapsing the predecessor and the worker block. - Malformed TOML emits print_error and leaves the file untouched. * runtime_factory.py / providers/factory.py: assert that get_runtime_profile() flows all the way into CodexCliRuntime and CodexCliLLMAdapter, both for the "worker" path (mapped to ouroboros-worker) and for the default-None path. This prevents a silent regression where someone drops the runtime_profile=... kwarg from the factory call and breaks worker-subprocess isolation without any test failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shaun0927
force-pushed
the
feat/orchestrator-runtime-profile
branch
from
8d807ed to
581761a
Compare
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
581761afor PR #505
Review record:
bf2e3911-06a5-4859-9b9b-6ad9b999d1ce
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
| 1 | src/ouroboros/cli/commands/setup.py:231 | Refactoring | _trim_managed_codex_worker_profile_comments() only recognizes the managed header when it is immediately adjacent to [profiles.ouroboros-worker]. If an operator inserts their own comment between the managed block and the table, each ouroboros setup --runtime codex rerun will prepend another managed header block. Consider tolerating that layout or adding a regression test for it. |
Design Notes
The slice is well-contained: config/env resolution, backend-local profile mapping, and setup-time Codex profile registration are kept separate, and both Codex command builders now share the same translation path. I did not find a blocking correctness issue in the changed files.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
cd601a4for PR #505
Review record:
da669856-e196-4c70-9de1-bea0d18dd674
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
The slice is internally consistent: config adds an orchestrator-level runtime_profile, both Codex execution paths map it through one shared resolver, and setup now provisions the matching Codex profile block without clobbering user-owned keys. Test coverage for the touched paths is strong. I could not execute the test suite here because pytest is not installed in this environment.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Autopilot verification for latest head The latest bot review approved @Q00 latest head should address the config-loss finding; please re-review when convenient. |
|
Autopilot status check: latest head remains CI-green and the latest ouroboros-agent review is APPROVED with no remaining bot findings. The PR still shows CHANGES_REQUESTED only because of the earlier human review state; I don't see a current code/design note requiring another commit. |
Q00
left a comment
Q00
left a comment
There was a problem hiding this comment.
Requesting changes at the stack contract level.
This PR introduces orchestrator.runtime_profile as a Codex backend profile name, for example worker -> --profile ouroboros-worker. PR #538 introduces the same public config key as a RuntimeProfileConfig object for stage routing, with default/stages fields. Those are incompatible meanings for the same config surface, and the two branches currently conflict in src/ouroboros/config/models.py.
Before merge, please coordinate one stable shape, for example runtime_profile.backend_profile plus runtime_profile.stages, or rename the Codex-specific field so the stage-routing object can own runtime_profile.
If #538 is meant to be dropped, or if the contract has moved 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>
Adds an opt-in `orchestrator.runtime_profile` setting that lets every runtime/adapter map a single abstract profile name to its native configuration mechanism. Phase 1 ships only the Codex mapping (`worker` → `codex exec --profile ouroboros-worker`); OpenCode, Hermes, Claude Code, and LiteLLM mappings are intentionally deferred to follow-up slices. This is the cooperative, multi-runtime alternative to PR Q00#488's blunt `--ignore-user-config`: setup-managed `[mcp_servers.ouroboros]` and the new `[profiles.ouroboros-worker]` section co-exist in `~/.codex/config.toml`, so isolation is selective rather than wholesale, and worker subprocesses no longer have to disable Ouroboros's own MCP/env hookup to escape interactive Codex defaults. Default `runtime_profile=None` preserves today's `_build_command` output exactly — covered by regression tests in both runtime and adapter — so existing installations see zero behavior change until they opt in. Closes the design discussion on Q00#488. Refs Agent OS RFC Q00#476, Q00#184. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The orchestrator runtime and the LLM provider adapter had identical
copies of the runtime_profile -> Codex --profile table plus its
warning helper, paired only by a "keep these in sync" comment. Move
both to ouroboros.codex.runtime_profile so the mapping is
single-sourced; future Phase 2 backends (OpenCode, Hermes, Claude
Code, LiteLLM) will provide their own backend-local mapping module.
The new module accepts the caller's logger instance plus a
log_namespace string so the existing
patch("module.log.warning") test seam still captures the
runtime_profile_unmapped warning at its original namespace.
The module lives under ouroboros.codex (rather than
ouroboros.orchestrator) to avoid a circular import:
ouroboros.orchestrator.__init__ pulls in the runner, which pulls in
the providers package -- exactly what imports this module from the
Codex LLM adapter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous _upsert_codex_worker_profile_section() deleted every line inside [profiles.ouroboros-worker] on each rerun, which was a silent config-loss path: the new runtime guide explicitly tells operators to add per-worker overrides (model, notify, sandbox, ...) directly under that section, but _setup_codex() invokes the upsert on every ouroboros setup --runtime codex run, so a routine setup/upgrade would quietly wipe those overrides and leave codex exec --profile ouroboros-worker resolving an empty profile -- the exact failure mode the runtime_profile design was supposed to avoid. Switch the upsert to a refresh-comment / pass-through-body strategy: * If the table does not exist, append the managed comment block plus an empty [profiles.ouroboros-worker] header so codex exec can resolve the profile. * If the table exists, only the managed comment block immediately preceding the primary header is rewritten. The body of [profiles.ouroboros-worker] and every [profiles.ouroboros-worker.<sub>] subtable is preserved verbatim. This trades aggressive ownership for safe upsert: setup guarantees the managed header + comment are present and current; the table body is operator-owned data and survives reruns byte-for-byte. Also route the new-file path through the same upsert helper so both branches produce identical output (trailing-newline normalization, comment-block placement) -- the previous direct-write produced a slightly different shape than the upsert. Refresh _print_codex_config_guidance() so it points operators to the new [profiles.ouroboros-worker] worker overrides as well as the managed MCP/env hookup, instead of telling them ~/.codex/config.toml is "only for the MCP/env hookup". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ofile propagation Adds the missing regression coverage for the issues flagged on PR Q00#505 review: * setup.py: lock the new merge-preserving worker profile contract. - User-authored model/notify/sandbox keys plus a [profiles.ouroboros-worker.shell_environment_policy] subtable survive a setup rerun. - Multiple reruns after a user edit are byte-stable (no key loss AND no spurious additions on the first rerun, then idempotent afterwards). - The most adversarial layout -- a non-managed table abutting the worker header with no blank-line separator -- is handled without collapsing the predecessor and the worker block. - Malformed TOML emits print_error and leaves the file untouched. * runtime_factory.py / providers/factory.py: assert that get_runtime_profile() flows all the way into CodexCliRuntime and CodexCliLLMAdapter, both for the "worker" path (mapped to ouroboros-worker) and for the default-None path. This prevents a silent regression where someone drops the runtime_profile=... kwarg from the factory call and breaks worker-subprocess isolation without any test failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The post-rebase reviewer bot flagged a residual stacking case in _trim_managed_codex_worker_profile_comments(): if an operator inserts their own comment between the managed block and [profiles.ouroboros-worker], the legacy "match only when adjacent to the header" trim misses the managed block, and each setup rerun prepends another fresh managed block above the operator's comment -- drifting the file with each invocation. Walk the full output_lines list and remove every contiguous run that exactly matches the managed comment lines (plus a single trailing blank line if present). The upsert can then re-emit one fresh block above the header without stacking, regardless of where the previous managed block sat relative to the table or how many stale copies a corrupt earlier rerun may have left behind. Adds a regression test that pins the behaviour for the exact layout the reviewer bot called out: managed block, blank, operator note, header. After the fix, two reruns produce a byte-identical file with exactly one managed block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
shaun0927
force-pushed
the
feat/orchestrator-runtime-profile
branch
from
cd601a4 to
96ea118
Compare
|
Follow-up for the latest stack-contract review: pushed |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
96ea118for PR #505
Review record:
d51b99c9-ac22-4467-a993-707a2b9fddc1
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
src/ouroboros/config/models.py:288[warning]RuntimeProfileConfig.backend_profileis hard-coded toLiteral["worker"], but the neworchestrator.runtime_profilekey is being presented as a shared public contract and the Codex runtime resolver already has an explicit "unknown profile => warn and run without --profile" fallback. With the schema locked to"worker", any config that contains a future profile name, a newer backend slice's value, or even a typo now makesload_config()fail for the entire app instead of taking the intended soft fallback path. This couples a backend-specific phase-1 value into the shared config model and makes the contract brittle across upgrades.
Non-blocking Suggestions
None.
Design Notes
The shared Codex mapping module and the factory wiring are clean, and the worker-profile upsert is much safer than the previous clobbering behavior. The main design issue is that the shared config object now exposes a backend-agnostic shape but still encodes a backend-specific enum in the core schema.
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
Loosen the shared runtime_profile.backend_profile schema so backend-local resolvers, not the core config object, decide whether a profile name maps to a native runtime profile. This preserves the Codex warning/fallback path and keeps the object contract open for future backend slices. Constraint: Latest bot design note flagged Literal["worker"] as a brittle backend-specific enum inside a shared public config shape. Rejected: Hard-coding worker in RuntimeProfileConfig | future profile names and unknown-profile fallback would fail at load_config before backend resolution. Confidence: high Scope-risk: narrow Directive: Keep backend_profile as a non-empty string in shared config; perform backend-specific vocabulary checks in backend-local resolvers. Tested: uv run ruff format src/ouroboros/config/models.py tests/unit/config/test_models.py tests/unit/config/test_loader.py Tested: uv run ruff check src/ouroboros/config/models.py tests/unit/config/test_models.py tests/unit/config/test_loader.py Tested: uv run pytest tests/unit/config/test_models.py tests/unit/config/test_loader.py tests/unit/orchestrator/test_codex_cli_runtime.py tests/unit/providers/test_codex_cli_adapter.py -q Tested: uv run ruff format --check src/ouroboros/config/models.py tests/unit/config/test_models.py tests/unit/config/test_loader.py Tested: uv run mypy src/ouroboros/config/models.py src/ouroboros/config/loader.py src/ouroboros/codex/runtime_profile.py src/ouroboros/orchestrator/codex_cli_runtime.py src/ouroboros/providers/codex_cli_adapter.py 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
8531671for PR #505
Review record:
2be88ba8-b347-4a5c-960d-7046e45327f5
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
The slice is coherent: the new runtime_profile contract is centralized in config/model parsing, translated once in a shared Codex resolver, and wired through both Codex execution paths with setup/docs updates to provision the corresponding managed profile. I did not find a scope-local regression or unresolved contract mismatch in the changed files.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@Q00 re-review ping for PR #505. I rechecked the latest maintainer/bot review history and pushed one final follow-up commit, What this PR does now:
Latest improvement after the bot design note:
Why I think this is merge-ready:
|
… 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>
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>
Resolve the PR Q00#505 conflict with the current main profile-stack changes by keeping both Codex task profile anchors and the backend_profile-based worker isolation profile. Constraint: PR Q00#505 had to remain compatible with the current llm_profiles/profile-anchor work already on main. Rejected: Drop worker profile setup during conflict resolution | That would reintroduce the runtime_profile contract gap this PR is meant to close. Rejected: Drop main Codex task profile anchors | They are now part of the current project direction and must coexist with backend_profile. Confidence: high Scope-risk: moderate Directive: Keep orchestrator.runtime_profile.backend_profile backend-local; use llm_profiles for portable task profile routing. Tested: uv run ruff check src tests Tested: uv run mypy src/ouroboros Tested: uv run pytest tests/unit/cli/test_setup.py tests/unit/config/test_loader.py tests/unit/config/test_models.py tests/unit/orchestrator/test_codex_cli_runtime.py tests/unit/orchestrator/test_runtime_factory.py tests/unit/providers/test_codex_cli_adapter.py tests/unit/providers/test_factory.py -q Tested: uv run pytest -q Not-tested: GitHub CI after push. Co-authored-by: OmX <omx@oh-my-codex.dev>
|
Conflict-resolution follow-up pushed in What changed:
Local validation on
ouroboros-agent re-review requested for latest head |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
8ae46ecfor PR #505
Review record:
c98af883-4257-4b06-836b-6810859afd19
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/orchestrator/codex_cli_runtime.py:638 | BLOCKING | CodexCliRuntime._build_command() now emits --profile ouroboros-worker up front and can emit a second --profile ... later from _resolve_runtime_codex_config() (line 657) when a role/profile mapping resolves. That means any profiled agent-runtime session can end up with two Codex profiles on one command. Since this code path has only one backend profile surface, one profile must override the other or the CLI will reject the duplicate flag, so the new worker-isolation contract is not actually preserved for role-routed runtime sessions. |
| 2 | src/ouroboros/providers/codex_cli_adapter.py:372 | BLOCKING | The same profile-collision exists in the LLM adapter: _build_command() adds the runtime-level worker profile first, then adds another --profile at line 393 whenever a Codex task profile resolves. Interview/QA/evaluation flows that already rely on llm_profiles/llm_role_profiles will therefore either lose the worker overrides or lose the task profile, depending on how Codex handles repeated flags. The new runtime-profile feature needs an explicit composition strategy here before it is safe to merge. |
Non-blocking Suggestions
None.
Design Notes
The shared config object and shared Codex profile resolver are a sensible direction, and the setup-side ownership boundaries are much cleaner than the earlier clobbering approach. The remaining architectural gap is that the new runtime-level profile and the existing Codex task-profile mechanism both target the same singular CLI flag, so they currently compete instead of composing.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Apply repository ruff formatting to the PR Q00#505 conflict-resolution files so CI and local validation use the same formatting gate. Constraint: GitHub Ruff Lint runs both ruff check and ruff format --check. Rejected: Leave formatting to CI | The PR must be merge-ready before final review and merge. Confidence: high Scope-risk: narrow Directive: Run ruff format --check alongside ruff check for PR conflict-resolution commits. Tested: uv run ruff check src tests Tested: uv run ruff format --check src tests Tested: uv run mypy src/ouroboros Tested: uv run pytest tests/unit/cli/test_setup.py tests/unit/config/test_loader.py tests/unit/config/test_models.py tests/unit/orchestrator/test_codex_cli_runtime.py tests/unit/orchestrator/test_runtime_factory.py tests/unit/providers/test_codex_cli_adapter.py tests/unit/providers/test_factory.py -q Tested: uv run pytest -q Co-authored-by: OmX <omx@oh-my-codex.dev>
|
Small CI follow-up pushed in Reason:
Validation now passed locally on latest head:
ouroboros-agent re-review requested for latest head |
Resolve the PR Q00#505 post-merge profile collision by letting runtime_profile worker isolation own Codex's singular --profile flag while role/task profile resolution may still provide model fallbacks. Constraint: Codex CLI exposes one --profile selector, and current main already uses Codex profiles for task-profile routing. Rejected: Emit both runtime and task --profile flags | The CLI can reject or ambiguously interpret repeated profile selectors. Rejected: Let task profiles override worker isolation | That would make the new runtime_profile boundary disappear on role-routed worker sessions. Confidence: high Scope-risk: moderate Directive: If Codex adds composable profile layering later, replace this precedence rule with explicit composition tests. Tested: uv run ruff check src tests Tested: uv run ruff format --check src tests Tested: uv run mypy src/ouroboros Tested: uv run pytest tests/unit/cli/test_setup.py tests/unit/config/test_loader.py tests/unit/config/test_models.py tests/unit/orchestrator/test_codex_cli_runtime.py tests/unit/orchestrator/test_runtime_factory.py tests/unit/providers/test_codex_cli_adapter.py tests/unit/providers/test_factory.py -q Tested: uv run pytest -q Co-authored-by: OmX <omx@oh-my-codex.dev>
|
Follow-up for the latest bot review has been pushed in What changed:
Validation on latest head:
ouroboros-agent re-review requested for latest head |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
9d1fb7efor PR #505
Review record:
28acd47b-d1cf-4419-8e89-6bb34cc6ea64
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/cli/commands/setup.py:610 | BLOCKING | _register_codex_worker_profile() only checks that the existing file is syntactically valid TOML before appending [profiles.ouroboros-worker], but it never validates that profiles is actually a table or that the post-write document still parses. A valid user config like profiles = "oops" currently passes tomllib.loads(raw), then this code writes a new table under profiles, producing invalid TOML (Cannot overwrite a value). That silently corrupts ~/.codex/config.toml on ouroboros setup --runtime codex, which is a production-breaking config write. _register_codex_default_profiles() already guards this case via _existing_codex_profile_names(); the worker-profile path needs equivalent validation or a post-update parse check before writing. |
Non-blocking Suggestions
None.
Design Notes
The runtime-profile wiring is otherwise coherent: the shared Codex resolver keeps the orchestrator/runtime and provider paths aligned, and the “worker profile owns the single Codex --profile flag” rule is consistently enforced in both command builders. The main weakness is setup-path robustness: the new worker-profile registrar is not yet as defensive as the existing default-profile registrar when mutating arbitrary user TOML.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Make Codex worker-profile registration use the same profiles-table validation as default profile anchors, and parse the generated TOML before touching user config. Constraint: Bot review found syntactically valid scalar profiles values could be corrupted by appending a nested profile table. Rejected: Rely on initial tomllib.loads only | That proves syntax, not that the profiles namespace can accept profile tables. Confidence: high Scope-risk: narrow Directive: Validate arbitrary user TOML structure before any setup-managed profile write. Tested: uv run ruff check src tests Tested: uv run ruff format --check src tests Tested: uv run mypy src/ouroboros Tested: uv run pytest tests/unit/cli/test_setup.py tests/unit/config/test_loader.py tests/unit/config/test_models.py tests/unit/orchestrator/test_codex_cli_runtime.py tests/unit/orchestrator/test_runtime_factory.py tests/unit/providers/test_codex_cli_adapter.py tests/unit/providers/test_factory.py -q Tested: uv run pytest -q Co-authored-by: OmX <omx@oh-my-codex.dev>
|
Follow-up for the latest bot setup-path finding has been pushed in What changed:
Validation on latest head:
ouroboros-agent re-review requested for latest head |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
e69e04ffor PR #505
Review record:
d5bf8736-1631-4c6f-9fd8-79facfe33394
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
The change is coherent: orchestrator.runtime_profile stays backend-agnostic at the config layer, Codex-specific mapping is isolated in a shared resolver, and both Codex execution paths now enforce a single-profile precedence rule consistently. The setup-side TOML mutation is also appropriately conservative and preserves user-owned profile content.
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>
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>
2 participants
Summary
Introduces an opt-in
orchestrator.runtime_profilesetting as themulti-runtime alternative to PR #488's blunt
--ignore-user-config.Phase 1 ships only the Codex mapping (
worker→codex exec --profile ouroboros-worker); OpenCode, Hermes, Claude Code, and LiteLLM mappingsare intentionally deferred to follow-up slices, matching the staging in
the design discussion on #488.
Default
runtime_profile=Nonepreserves today's_build_commandoutput exactly in both
CodexCliRuntimeandCodexCliLLMAdapter—no flag is added, so existing installations see zero behavior change
until they opt in. Setup-managed
[mcp_servers.ouroboros]and the new[profiles.ouroboros-worker]section co-exist in~/.codex/config.toml,so isolation becomes selective rather than wholesale and worker
subprocesses no longer need to disable Ouroboros's own MCP/env hookup
to escape interactive Codex defaults.
This implements the four-step slice you proposed on #488:
orchestrator.runtime_profile--profile <mapped-name>onlywhen a mapping exists
What changed
config/models.pyOrchestratorConfig.runtime_profile: Literal["worker"] | None = Noneconfig/loader.py+__init__.pyget_runtime_profile()(envOUROBOROS_RUNTIME_PROFILE→ config →None)orchestrator/codex_cli_runtime.py_RUNTIME_PROFILE_TO_CODEX_PROFILE = {"worker": "ouroboros-worker"};__init__acceptsruntime_profile;_build_commandinjects--profile <name>afterexeconly when mappedproviders/codex_cli_adapter.pyorchestrator/runtime_factory.py+providers/factory.pyruntime_profile=get_runtime_profile(); OpenCode/Hermes/Claude/LiteLLM branches untouchedcli/commands/setup.py_register_codex_worker_profile()upserts[profiles.ouroboros-worker]idempotently;_setup_codexinvokes it after the existing MCP block writerdocs/runtime-guides/codex.mdruntime_profile)" section with config + env-var examples13 files, +432/−10.
Why this instead of
--ignore-user-configouroboros setup --runtime codexregisters an Ouroboros MCP/env hookupin
~/.codex/config.toml. PR #488's--ignore-user-configwoulddisable that registration on every Ouroboros-spawned
codex exec,which is the boundary you and @shaun0927 flagged on #488. The
runtime_profiledesign preserves the MCP/env block intact andexpresses isolation as a selective overlay (
[profiles.ouroboros-worker])that only the runtime layer activates — exactly the "compose, don't
subtract" stance from RFC #476.
The
--profileflag also gives us a natural home for #184(reasoning-tier overrides) and for the OpenCode/Hermes/Claude/LiteLLM
mappings without touching the orchestrator core.
Testing
Unit coverage added:
runtime_profile=Nonekeeps the command shapeidentical to today (regression lock-in);
runtime_profile="worker"emits
--profile ouroboros-workerbetweenexecand--json;unmapped values fall back to no flag and log a
*.runtime_profile_unmappedwarning.
None.correct ordering, and re-running setup is idempotent.
_setup_codexend-to-end: now asserts_register_codex_worker_profileis invoked alongside the existing
_register_codex_mcp_server.Refs / closes
--ignore-user-configapproach)Replace per-handler mcp_manager plumbing with AgentRuntimeContext + ControlBus #474 ControlBus / feat(core): introduce Directive StrEnum for control-plane decisions #477 Directive / feat(events): add control.directive.emitted event factory #492 control.directive.emitted)
runtime_profileis the natural carrier forreasoning-tier overrides per backend)
Phase 2 follow-ups (separate PRs): OpenCode, Hermes, Claude Code,
LiteLLM mappings.
🤖 Generated with Claude Code