feat(routing): bind provider-neutral reasoning effort profiles - #785
Conversation
|
Warning Review limit reachedNext included review available in 55 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: Pro Plus Run ID: 📒 Files selected for processing (23)
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 |
|
Exact current head |
|
Current HEAD review/fix: Fixed review findings:
Verification at this exact HEAD:
@opencode-agent please review exact current HEAD |
|
|
2 similar comments
|
|
|
|
|
Current-head proof for
The production default remains locked until measured held-out buyer evidence clears the gate. Protected auto-merge is enabled; independent approval and terminal hosted Checks remain required. No self-approval, admin merge, or force-push was used. |
|
Current-head refresh for
Protected auto-merge remains enabled; independent approval and terminal hosted Checks are still required. No self-approval, admin merge, or force-push was used. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Fixed Devin's exact-head finding on
@devin-ai-integration please re-review this exact current HEAD. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact-head verification completed on
@devin-ai-integration please review this exact current HEAD. |
|
Final exact-head verification completed on
@devin-ai-integration please review this exact current HEAD. |
|
Current-head refresh for
The production default remains locked until measured held-out evidence clears the gate. Protected auto-merge remains enabled; independent approval and terminal hosted Checks are required. No self-approval, admin merge, or force-push was used. |
|
Exact-head review pass for ec609fa:
No new local defect was found. Independent protected approval remains required. @opencode-agent review exact current HEAD ec609fa against main; publish a formal verdict for role effort application, passthrough omission honesty, and judge failover exclusions. |
|
Current-head validation for
Local diagnostic note: this checkout has no Ruff configuration matching the PR description; default @opencode-agent Review only exact current HEAD |
|
Exact current HEAD ec609fa was revalidated for provider-neutral reasoning effort profiles.
Please review and run protected Checks for this exact HEAD only. |
Exact-head validation — PR #785
@opencode-agent please review only exact current HEAD |
opencode-agent
Bot
added
priority: medium
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headec609fa7b526a995346c34434e277eb12f5a0246. -
Head SHA:
ec609fa7b526a995346c34434e277eb12f5a0246 -
Workflow run: 32702051427
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: fuzz.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["actionlint plus required checks"]
Evidence --> S2["Changed file (11 files)"]
S2 --> I2["repository behavior"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["required checks"]
Evidence --> S3["Docs (7 files)"]
S3 --> I3["operator or user guidance"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["docs review"]
Evidence --> S4["Test (8 files)"]
S4 --> I4["regression suite"]
I4 --> Conflict["Merge conflict blocks this path"]
Conflict --> V4["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: fuzz.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["actionlint plus required checks"]
Evidence --> S2["Changed file (11 files)"]
S2 --> I2["repository behavior"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["required checks"]
Evidence --> S3["Docs (7 files)"]
S3 --> I3["operator or user guidance"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["docs review"]
Evidence --> S4["Test (8 files)"]
S4 --> I4["regression suite"]
I4 --> Conflict["Merge conflict blocks this path"]
Conflict --> V4["targeted test run"]
Merge Conflict Guidance
gh pr checkout 785 --repo ContextualWisdomLab/contextual-orchestrator
git fetch origin main
git merge --no-ff origin/main # or: git rebase origin/main
git status --short
# resolve files, then git add <resolved-files>
# merge path: git commit
# rebase path: git rebase --continue
git push origin HEAD:feat/issue-568-role-effort
# rebase path only: git push --force-with-lease origin HEAD:feat/issue-568-role-effort |
…lation Issue #568 first slice: versioned per-role reasoning_effort_profile, fail-closed parse, replayable snapshot, and true-θ RMSE ablation. Temperature is not effort. Production route/conduct defaults stay locked. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
…onduct Same snapshot_hash on both paths when the operator passes a catalog. Default constructor still omits the key so production payloads stay put. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Buyer next action: pass role_effort_catalog to attach the same snapshot on route and conduct, or omit it to keep today's payload. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Issue #568 honesty slice: θ̂ is (1-λ)θ and RMSE uses θ̂-θ, not a rank constant. Parse/hash fail closed on missing profiles, blank versions, fractional seeds, and extra roles. The same snapshot survives run, stream_route, and batch_route. Production defaults stay locked while measurement_status is estimated. Buyer next action: python tests/test_reasoning_effort_profile.py Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
ec609fa to
3653968
Compare
There was a problem hiding this comment.
📝 Info: Enabling catalog with undeclared real providers fails all requests closed
With a bound catalog and a real (https) provider whose reasoning_effort_supported is left at the default None, apply_effort_profile computes supports=False, and since the default catalog profiles use unsupported_provider_fallback="abstain", apply_request_profile raises EffortProfileError. Inside _invoke this is caught, recorded as a failure, and every candidate is exhausted, raising 'all N candidate agents failed'. This is the documented fail-closed behavior (native effort only when support is proven), so it is not a bug, but operators enabling the catalog against real providers must set reasoning_effort_supported=true or choose the omit fallback or all traffic will fail.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
📝 Info: Native reasoning_effort never validated/applied on mock:// path
chat and stream_chat return early for mock:// agents before apply_effort_profile is ever called (orchestrator.py, 1073-1077), so a profile passed through the mock path is not validated or applied. apply_effort_profile also treats reasoning_effort_supported is None mock agents as supported (orchestrator.py:851-854), which is dead for the mock chat path but would matter only for non-mock local providers (which are correctly treated as unproven). The snapshot function still re-parses/validates every profile, so misconfigured profiles are caught at snapshot time. Net effect: mock-based tests exercise snapshot attachment but not the native-effort egress branch; that branch is covered by test_request_profile_separates_native_effort_and_sampling_controls. No bug, but worth noting the mock path gives no coverage of profile application.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| "body": self.apply_effort_profile(agent, { | ||
| "model": agent.model, | ||
| "messages": messages, | ||
| "temperature": self.temperature if temperature is None else temperature, | ||
| "max_tokens": self.max_output_tokens, | ||
| }, | ||
| }, effort_profile), |
There was a problem hiding this comment.
📝 Info: Default payloads unchanged despite always-on profile application
chat, stream_chat, and _batch_run now call apply_effort_profile unconditionally. With no profile, apply_request_profile only runs payload.setdefault("max_tokens", default_max_output_tokens). chat/stream_chat already set max_tokens, so it is a no-op; _batch_run dropped its explicit max_tokens line but setdefault restores the same value. The no-catalog path yields identical provider payloads. proxy_completion applies the profile only when non-None, matching the new max_tokens-absent assertions in the passthrough tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
| payload["max_tokens"] = validated.max_output_tokens | ||
| payload["temperature"] = validated.temperature | ||
| payload["top_p"] = validated.top_p | ||
| if validated.seed is not None: | ||
| payload["seed"] = validated.seed | ||
| if supports_reasoning_effort: | ||
| payload["reasoning_effort"] = validated.reasoning_effort |
There was a problem hiding this comment.
📝 Info: Effort profile overrides request-scoped temperature/top_p on real providers
When an operator opts into role_effort_catalog, apply_request_profile unconditionally sets payload["temperature"], payload["top_p"], and payload["max_tokens"] from the profile (reasoning_effort_profile.py). On the real-provider chat/stream_chat/batch paths this overwrites the effective sampling values computed earlier in ModelClient.chat (e.g. self._local.last_temperature), so a request-scoped temperature would be silently replaced by the profile's temperature (default 0.2). This is consistent with the stated design (profile carries sampling controls and the catalog is opt-in/off by default), so it is not flagged as a bug, but reviewers should confirm this is the intended precedence for deployments that both enable the catalog and pass per-request sampling.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _role_effort_profile(self, role: str) -> ReasoningEffortProfile | None: | ||
| """Return the opt-in profile bound to one workflow role.""" | ||
| if self.role_effort_catalog is None: | ||
| return None | ||
| return self.role_effort_catalog.get(role) | ||
|
|
||
| def _with_effort_snapshot(self, result: dict[str, Any]) -> dict[str, Any]: | ||
| """Attach a replayable role-effort snapshot when the operator opted in. | ||
|
|
||
| Buyer next action: compare ``reasoning_effort_snapshot.snapshot_hash`` | ||
| on ``complete``, ``run``, ``stream_route``, and ``batch_route``. Omit | ||
| the constructor catalog to keep today's payload. | ||
| """ | ||
| if self.role_effort_catalog is None: | ||
| return result | ||
| snapshot = snapshot_role_effort_catalog(self.role_effort_catalog) |
There was a problem hiding this comment.
📝 Info: Snapshot requires the full role set while role lookup tolerates a partial catalog
_role_effort_profile uses self.role_effort_catalog.get(role) (orchestrator.py), which returns None for a missing role, but _with_effort_snapshot calls snapshot_role_effort_catalog (orchestrator.py:2529) which raises EffortProfileError unless the catalog binds exactly WORKFLOW_ROLES (reasoning_effort_profile.py:275-278). If an operator passes a partial role_effort_catalog to TaskOrchestrator, per-role application would degrade gracefully but every route_once/conduct/run/stream_route/batch_route call would raise when attaching the snapshot. Only the complete default_role_effort_catalog() is exercised by tests, so partial-catalog usage is an unguarded failure mode.
Was this helpful? React with 👍 or 👎 to provide feedback.
| candidates = self._failover_candidates(primary, text, eligibility_role or role) | ||
| retry_limit = min(self.tool_retry_attempts, MAX_TOOL_RETRY_ATTEMPTS) | ||
| for agent in candidates: | ||
| retry_attempt = 0 | ||
| while True: | ||
| try: | ||
| output = self.client.chat(agent, messages) | ||
| effort_profile = self._role_effort_profile(role) | ||
| output = ( | ||
| self.client.chat(agent, messages, effort_profile=effort_profile) | ||
| if effort_profile is not None | ||
| else self.client.chat(agent, messages) | ||
| ) |
There was a problem hiding this comment.
📝 Info: judge role uses judge effort profile but verifier-based failover selection
_FastMLSIJudgeAdapter.complete now calls _invoke(..., role="judge", eligibility_role="verifier") (orchestrator.py). Failover candidate selection uses eligibility_role ("verifier") so operator verifier-exclusions still apply (verified by the new test_fast_mlsirm_judge_failover_honors_verifier_exclusions), while the effort profile lookup inside _invoke uses the actual role ("judge"). This split is intentional and correct; the effort_profile is recomputed each failover iteration but that is harmless. Confirmed no other caller passes eligibility_role, so the new default keeps existing behavior for non-judge roles.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if self.reasoning_effort_supported not in (None, True, False): | ||
| raise TypeError("reasoning_effort_supported must be true, false, or null") |
There was a problem hiding this comment.
📝 Info: reasoning_effort_supported accepts integer 0/1 due to equality-based membership check
__post_init__ validates with if self.reasoning_effort_supported not in (None, True, False) (orchestrator.py). Because 1 == True and 0 == False in Python, integer values 1/0 (and 1.0/0.0) pass this check without raising, so a JSON config value of 1 from from_dict would be stored as the int 1. Downstream apply_effort_profile uses identity checks (is True / is None), so a stored 1 is treated as unsupported (fail-closed), meaning the impact is limited to a non-bool sneaking past the type guard rather than a behavioral hazard. Worth tightening the validation to isinstance/identity if strict typing is desired.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "body": self.apply_effort_profile(agent, { | ||
| "model": agent.model, | ||
| "messages": messages, | ||
| "temperature": self.temperature if temperature is None else temperature, | ||
| "max_tokens": self.max_output_tokens, | ||
| }, | ||
| }, effort_profile), |
There was a problem hiding this comment.
📝 Info: Batch body max_tokens restored via effort profile setdefault
_batch_run removed the explicit "max_tokens": self.max_output_tokens from each JSONL body and now wraps the body in self.apply_effort_profile(agent, {...}, effort_profile) (orchestrator.py). For effort_profile=None the setdefault in apply_request_profile re-adds max_tokens, so the emitted batch line is unchanged from before. This is behavior-preserving for the batch path (unlike proxy_completion, which previously had no max_tokens at all), so it is intentional and correct here.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Merge-gate evidence (2026-08-24): Deep diff review completed (verdict: merge-ready); all required checks green on current head except strix, which fails closed on org-wide NVIDIA NIM quota exhaustion (external provider-capacity blocker; serialization fix in ContextualWisdomLab/.github#1297). Full local suite green. |
2 participants
Buyer-visible outcome
Operators can assign a versioned, provider-neutral reasoning-effort profile to thinker, worker, verifier, synthesizer, planner, and judge work, then replay the exact catalog snapshot across route, conduct, stream, batch, planning, judging, and persisted runs.
Safety contract
reasoning_effortis separate from temperature, top-p, and seed.ModelAgent.reasoning_effort_supported=true; unknown support abstains unlessunsupported_provider_fallback=omitis explicit.Evidence
1455 passed in 550.47s34 passed100%(178 statements,66 branches)uv run ruff check ., compileall, andgit diff --check: passedDocumentation
docs/doctoring/reasoning-effort-profile.mddocs/planning/adrs/0021-reasoning-effort-profiles.mdvsZMd8WAv42HDRgcZuNcWk; Storybook deferred because this backend repository has no frontend package.Closes #568