fix(cli): wire the role-effort catalog into the CLI/server on-ramp - #958
Conversation
TaskOrchestrator has accepted role_effort_catalog (ADR 0021 / issue #568) since it was added, but __main__.py never passed a non-None value: there was no --role-effort-catalog flag, and the TaskOrchestrator(...) constructed in main() never set the kwarg. apply_effort_profile therefore always ran its profile=None no-op branch in the shipped CLI/server, leaving the whole reasoning_effort_profile subsystem (temperature/top_p/seed/reasoning_effort injection, fail-closed provider-support handling, the replayable reasoning_effort_snapshot) unreachable in production despite being correct, unit-tested, and fuzzed. Add --role-effort-catalog default, which loads default_role_effort_catalog() and passes it into the constructed TaskOrchestrator. Omitting the flag (the default) keeps role_effort_catalog=None, so every existing payload is unchanged and the locked route/conduct selection defaults stay locked -- this only wires the catalog's own additive, already-opt-in fields. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthrough
Changes역할별 effort catalog CLI
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to The change preserves existing behavior unless operators explicitly enable the built-in catalog, but enabled server processes may reject affected requests when provider capability metadata is missing; the CLI help also needs a minor field-name correction. The PR is mergeable with explicit owner awareness. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…r support Devin Review flagged that default_role_effort_catalog() uses unsupported_provider_fallback="abstain" for every role, while ordinary real-provider agent configs (examples/agents.openai.json) and every auto-discovered agent never set reasoning_effort_supported. Confirmed by direct repro against ModelClient.apply_effort_profile: with such a pool, --role-effort-catalog default constructed successfully and then raised EffortProfileError on every subsequent request -- the CLI's own new test only exercised the mock:// pool, which auto-passes the support check. Per ADR 0021's fail-closed design and the doctoring note that the catalog is for "an evaluation or an explicitly configured deployment," reject the flag at startup instead of loosening the catalog's default: extract the duplicated support check into agent_proves_reasoning_effort_support() and use it in a new _require_eligible_role_effort_agents() gate, run right after the agent pool (including --auto-discover-model-agents) is finalized, before any request or server bind. Covers both the one-shot CLI prompt and --serve. Also fixes a CodeRabbit nit: the flag's help text said "max_tokens" where it meant ReasoningEffortProfile's actual field name, max_output_tokens. Adds three regression tests (non-mock unknown-support pool rejected in CLI and --serve mode; non-mock pool with explicit reasoning_effort_supported: true allowed) and points the CHANGELOG's existing entry at docs/architecture.md's existing Fugu/TRINITY/Conductor citation for reasoning_effort_profile. Full suite: 2837 passed, 1 skipped (pre-existing fast_mlsirm sandbox gap, same as PR #958 baseline), 100% interrogate, ruff clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Likely load contention from the burst of PRs across this org marked ready for review in the last ~45 minutes. Re-running the failed job once. Generated by Claude Code |
Devin's round-2 finding on PR #958: _require_eligible_role_effort_agents only proves *some* agent in the pool supports reasoning_effort. Ordinary role-based selection (route_once, conduct, stream_route, batch_route, structured-synthesis passthrough) could still rank or select an *unsupported* agent from a mixed pool ahead of a supported one. route_once/conduct recovered by accident via _invoke's generic tool-failure failover; stream_route/batch_route call the provider directly with no such recovery and would raise EffortProfileError outright. TaskOrchestrator._ranked_agents now narrows role-based candidates to agents that prove reasoning_effort support whenever the role's role_effort_catalog entry fails closed, via a new shared _eligible_role_effort_candidates helper (falls back to the unfiltered set when none prove support, so that edge case still gets a clear attempt instead of a silently emptied list). This mirrors, and now shares code with, the equivalent filter proxy_completion already applied for its own caller-supplied effort profile. Also documents (CHANGELOG + docstrings on remove_agent/patch_agent) Devin's informational finding: runtime pool mutations are real (POST/PATCH/DELETE .../worker_agents) and are not revalidated against an active catalog -- confirmed real, left as a documented known limitation per this PR's own narrow scope rather than a full runtime-revalidation fix. Adds tests/test_mixed_pool_role_effort_selection.py: a mixed pool (one unsupported agent ranked first by priority, one supported) never dispatches to the unsupported agent across route, conduct, stream, and batch, plus a control test proving an "omit"-fallback role is left unfiltered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
…effort profile PR #958 round-3 Devin findings: - _require_eligible_role_effort_agents now proves eligibility per active fail-closed role using the same general-chat (_is_general_chat_agent) and provider_exclusions rules TaskOrchestrator._ranked_agents/_select_agent already apply at request time, reusing agent_proves_reasoning_effort_support and _eligible_role_effort_candidates from round 2 rather than new logic. A pool whose only proving agent is non-chat (e.g. embedding-only) or excluded from every active role is now rejected at startup instead of passing and failing every request later. - The startup rejection path now closes the already-constructed TaskOrchestrator before calling parser.error() (which raises SystemExit), so an embedded caller of main() gets clean resource teardown instead of relying on process exit. - TaskOrchestrator.proxy_completion's single-agent passthrough (the server's tool_loop call site, and any other caller that omits effort_profile) now defaults an unset effort_profile to the opted-in role_effort_catalog's "worker" entry -- the role every selection/failover call in that method already uses -- mirroring the existing `effort_profile or self._role_effort_profile("synthesizer")` pattern _orchestrated_provider_completion already applies for its own role. A caller that passes its own effort_profile, or runs with no catalog configured, is unaffected. Adds two startup regression tests (non-chat-only prover; sole prover excluded from every role) and an end-to-end HTTP test driving a real tool-loop request through the server and asserting the resolved upstream payload carries the catalog's worker-role sampling/token/seed/reasoning fields, plus a companion test proving an unconfigured catalog stays a no-op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Round-3 Devin findings addressed in 1. 🔴 "Startup accepts unusable provider pools" ( 2. 🔍 "Rejected startup relies on process cleanup" (lines 683-684) — Fixed. 3. 🟡 "Tool requests bypass effort profiles" (lines 676-678) — Investigated first, per your framing: confirmed this is the local fix you suggested, not a multi-call-site change. Validation: full Generated by Claude Code |
|
Pausing further fixes on this PR here — this is round 4 of Devin review findings in the same widening area (role-effort-catalog integration across startup validation, routing, failover, and now passthrough/synthesis), and each round has kept surfacing new issues rather than converging (round 1: 1 bug; round 2: 1 bug; round 3: 2 bugs; round 4: 1 bug + 2 informational). That's the standing signal to stop iterating and report rather than keep expanding this PR's scope. Status of round 4's findings:
What's already fixed and validated across rounds 1-3 (all confirmed resolved by Devin's own follow-up passes): the original hollow-path finding (CLI wiring for Recommend a narrowly-scoped follow-up PR for the explicit-override-bypass edge case rather than extending this one further, since it requires threading the per-call profile into candidate ranking — a small but distinct change from what this PR is about. Leaving this PR as-is for review/merge on its actual scope (the CLI wiring + the request-time bugs that would affect every default-catalog user, not just explicit-override callers). Generated by Claude Code |
… selection Devin review on PR #958 (contextual_orchestrator/orchestrator.py:5883-5884): _orchestrated_provider_completion resolved its caller-supplied effort_profile only for the final apply_effort_profile payload call, not for the synthesizer's own selection (_select_agent), replica lookup (_ranked_agents), or failover list (_failover_candidates) -- those three call sites omitted effort_profile entirely and silently fell back to the raw role_effort_catalog entry inside _ranked_agents. A fail-closed override (unsupported_provider_fallback other than "omit") could rank/select an unproven-support agent ahead of a proven one, and apply_effort_profile then raised EffortProfileError outright with no failover -- unlike the identical scenario on the plain passthrough path, which already threads its override through every selection call site. Resolve effort_profile or self._role_effort_profile("synthesizer") once, up front, and pass it to all three call sites, matching the passthrough path's existing pattern. Adds a regression test (test_structured_synthesis_honors_explicit_effort_profile_override_for_selection) that reproduces the bug against the pre-fix code (EffortProfileError instead of failover) and passes against the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Required
|
|
Cross-PR integration contract: routing identity is provider-neutral |
|
Re: the cross-PR Not a doc/text collision like the other flagged PRs — this is the same method being reshaped two different ways by two in-flight PRs. Whoever lands second will need to manually reconcile both reshapes rather than take a mechanical rebase; flagging rather than resolving it myself since it's a real design choice (which reshape of Generated by Claude Code |
|
Contextual-Orchestrator를 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Strix가 6시간 이상 동작해서 취약점 잡는 것도 본 일이 있습니다. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
|
A fix already exists but hasn't merged yet: Generated by Claude Code |
|
Generated by Claude Code |
|
This is Re-ran the failed job once (run Generated by Claude Code Generated by Claude Code |
| updated_agents = [agent for agent in updated_candidates if not agent.disabled] | ||
| if not updated_agents: | ||
| raise ValueError("cannot disable the last enabled agent") | ||
| self._require_role_effort_pool(updated_candidates) |
There was a problem hiding this comment.
🟡 Live supporting-provider rotation is blocked
Admin-managed providers cannot declare reasoning_effort_supported because the request allowlists reject it. patch_agent then blocks disabling the final seeded supporter, preventing live provider rotation.
Prompt for agents
Complete the runtime agent-management contract for reasoning-effort support. The HTTP create and patch allowlists in contextual_orchestrator/server.py currently reject reasoning_effort_supported, patch_agent in contextual_orchestrator/orchestrator.py does not apply it, and the API schema omits it. Allow a validated true/false/null value through create and patch operations, persist it through the existing agent-pool storage, and retain the new role-effort invariant check so an operator can add or update a replacement supporter before disabling or removing the old one. Add API-level tests for this rotation sequence.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <<<<<<< HEAD | ||
| - (Devin review on #958) `_orchestrated_provider_completion`'s structured/ | ||
| Responses synthesis path (the `single_agent=False` branch of | ||
| `proxy_completion`) resolved its caller-supplied `effort_profile` override | ||
| only for the final `apply_effort_profile` payload call, not for the | ||
| synthesizer's own selection (`_select_agent`), replica lookup | ||
| (`_ranked_agents`), or failover list (`_failover_candidates`) — those three | ||
| call sites omitted `effort_profile` entirely and so silently fell back to | ||
| the raw `role_effort_catalog` entry inside `_ranked_agents`. A fail-closed | ||
| override (`unsupported_provider_fallback` other than `"omit"`) could still | ||
| rank/select an unproven-support agent ahead of a proven one, and | ||
| `apply_effort_profile` then raised `EffortProfileError` outright — with no | ||
| failover, unlike the identical scenario on the plain passthrough path, | ||
| which already threads its override through every selection call site. Now | ||
| resolves `effort_profile or self._role_effort_profile("synthesizer")` once, | ||
| up front, and passes it to all three call sites, matching the passthrough | ||
| path's existing pattern and `_ranked_agents`' own documented intent that | ||
| every role-based selection path -- "structured synthesis" included -- stay | ||
| consistent with the effort catalog's eligibility guard. | ||
| ======= |
Summary
TaskOrchestratorhas acceptedrole_effort_catalog(ADR 0021 / issue [Product Gap] Provider-neutral role reasoning-effort profiles with equal-budget ablation #568) since it was added, but no caller incontextual_orchestrator/__main__.pyever passed a non-Nonevalue — the CLI/server entrypoint had no--role-effort-catalogflag and itsTaskOrchestrator(...)construction never set the kwarg.apply_effort_profiletherefore always ran itsprofile=Noneno-op branch in production, and the wholereasoning_effort_profilesubsystem (temperature/top_p/seed/reasoning_effortinjection, fail-closed provider-support handling, the replayablereasoning_effort_snapshot) was correct, unit-tested, and Atheris-fuzzed but structurally unreachable from the shipped CLI/server.--role-effort-catalog defaultloadsdefault_role_effort_catalog()and passes it into theTaskOrchestratorconstructed bymain(). Omitting the flag (the default) keepsrole_effort_catalog=None, so every existing payload is byte-for-byte unchanged and the locked route/conduct selection defaults (production_default_change_allowed) are untouched — this only wires the catalog's own additive per-role sampling/effort/snapshot fields, exactly the "opt-in" ADR 0021 already claims exists.Why this direction over the alternative
The finding's other suggested direction — downgrading ADR 0021 / the
orchestrator.py:3427"Operator next action" comment to admit the subsystem is unreachable — would have been the wrong fix here:docs/doctoring/reasoning-effort-profile.mdalready framesdefault_role_effort_catalog()as usable "for an evaluation or an explicitly configured deployment." A CLI flag is that explicitly-configured-deployment on-ramp, so adding it makes the existing docs/ADR true instead of rewriting them to describe a permanent gap.Changes
contextual_orchestrator/__main__.py: importdefault_role_effort_catalog; add--role-effort-catalog {default}(defaultNone); passrole_effort_catalog=default_role_effort_catalog() if args.role_effort_catalog == "default" else Noneinto theTaskOrchestrator(...)construction inmain().tests/test_cli_role_effort_catalog.py(new): exercisesmain()end-to-end against the realTaskOrchestratorandexamples/agents.mock.json— confirms--role-effort-catalog defaultreaches the constructor and produces areasoning_effort_snapshotin the CLI's JSON output; confirms omitting the flag keepsrole_effort_catalog=Noneand the payload snapshot-free; confirms an unknown catalog name fails closed (SystemExit(2)).CHANGELOG.md:### Fixedentry under[0.2.0] - Unreleased.Developer experience
python -m contextual_orchestrator "hi" --role-effort-catalog defaultnow returns areasoning_effort_snapshot(profile_version + snapshot_hash + role_profiles) alongside the normal route/conduct payload; omitting the flag reproduces today's exact output.User experience
No default-path behavior change for existing operators. Operators who want the issue #568 per-role reasoning-effort catalog (temperature/top_p/seed/max_tokens per workflow role, native
reasoning_effortonly where a provider proves support, fail-closed on unproven support) now have a documented, testable way to turn it on, instead of a constructor kwarg nothing ever set.Test plan
ruff check contextual_orchestrator/__main__.py tests/test_cli_role_effort_catalog.pypython -m pytest -q tests/test_cli_role_effort_catalog.py tests/test_cli_auth.py(new tests + existing__main__.pyCLI coverage)python -m interrogate -v .(100%, unchanged)python -m pytest -q(Python 3.12, matching CI):2834 passed, 1 skipped, 1 failed. Theone failure,
tests/test_psychometric_routing.py::test_fast_mlsirm_fit_uses_judge_acceptance_item_for_context_score,is the pre-existing
ModuleNotFoundError: No module named 'fast_mlsirm'sandbox gapdocumented in
docs/product-technical-gap-baseline.md("unreachable in this sandbox, samedocumented blocker as PR fix(rater): preserve generic v1 and add criterion-bound wrapper #917, not a regression") — the private
fast-mlsirmarchive isn'tfetchable from this sandbox; unrelated to this change.
Generated by Claude Code
Summary by CodeRabbit
새 기능
--role-effort-catalog default옵션을 추가했습니다.버그 수정
문서