Skip to content

fix(parallel): honor configured worker caps#489

Open
andrew-adamson wants to merge 1 commit intoQ00:mainfrom
andrew-adamson:fix/parallel-workers-config
Open

fix(parallel): honor configured worker caps#489
andrew-adamson wants to merge 1 commit intoQ00:mainfrom
andrew-adamson:fix/parallel-workers-config

Conversation

@andrew-adamson
Copy link
Copy Markdown

Summary

  • honor configured max parallel worker limits when the environment override is not set
  • thread the runtime lookup through the relevant CLI and execution paths
  • add focused config and CLI coverage for the worker-cap resolution behavior

Testing

  • uv run pytest tests/unit/cli/test_run_qa.py tests/unit/config/test_loader.py::TestRuntimeHelperLookups::test_get_max_parallel_workers_prefers_env tests/unit/config/test_loader.py::TestRuntimeHelperLookups::test_get_max_parallel_workers_falls_back_to_config

Copy link
Copy Markdown
Contributor

@ouroboros-agent ouroboros-agent Bot left a comment

Choose a reason for hiding this comment

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

Review — ouroboros-agent[bot]

Verdict: APPROVE

Reviewing commit ab54999 for PR #489

Review record: f21c4524-e6d7-4e98-894d-fa6f745a9a8c

Blocking Findings

No in-scope blocking findings remained after policy filtering.

Follow-up Findings

  • src/ouroboros/config/loader.py:483 [warning] get_max_parallel_workers() silently falls back to 3 when the env var is non-integer or when load_config() rejects orchestrator.max_parallel_workers. That defeats the new Field(..., ge=1) validation and turns typos like 0 or five into an unnoticed behavior change instead of a config error, so users can believe their configured parallelism is in effect when the runner is actually using the default.

Non-blocking Suggestions

| 1 | tests/unit/cli/test_run_qa.py:123 | Tests | The new CLI fallback test stubs get_max_parallel_workers() directly, so it cannot catch regressions in the actual config/env parsing path. A test that exercises the real helper with invalid values would have exposed the silent-fallback bug above. |

Design Notes

The change is directionally correct: centralizing the worker-cap lookup avoids duplicated defaults and threads the config through both CLI and MCP execution. The main issue is that the shared helper currently treats invalid configuration as “missing”, which weakens the contract introduced by the new config field.

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

@shaun0927
Copy link
Copy Markdown
Collaborator

This is directionally right.

Recent work in this area has been about making runtime behavior more explicit and less surprising, and centralizing the worker-cap lookup fits that well. Threading the configured cap through both CLI and MCP execution paths is the right move.

I do want one follow-up before we treat this as fully done: please avoid silently swallowing invalid config/env values.

If get_max_parallel_workers() quietly falls back to 3 for malformed env/config input, users can believe their configured cap is in effect when the runner is actually using the default. In this codebase we've generally been moving toward explicit contracts rather than silent fallback.

So please tighten that behavior — either by surfacing the invalid setting more clearly or by locking the intended fallback semantics down with stronger tests.

Overall though, this PR is in the right shape.

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.

3 participants