fix: complete PR 765 review remediations - #810
Conversation
…github.com/ContextualWisdomLab/contextual-orchestrator into fix/auto-reasoning-effort-contract-rebased
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Runtime aggregate verification on exact source head 513a815 reproduced the configured provider-neutral gateway boundary without retaining credentials or response bodies: gpt-5.6-sol with temperature 0.2 returned HTTP 400, and the identical request with temperature omitted returned HTTP 200. This is the exact transition implemented by the bounded one-time capability negotiation and covered by the Azure diagnostic regression. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| raw, _served_id, _usage = self._invoke( | ||
| planner, | ||
| [ | ||
| {"role": "system", "content": system}, | ||
| {"role": "user", "content": task}, | ||
| ], | ||
| text=task, | ||
| role="thinker", | ||
| ) |
There was a problem hiding this comment.
📝 Info: BudgetExceededError caught by broad handler in generated planning
_plan_generated now calls _invoke, which can raise BudgetExceededError. conduct wraps planning in except Exception (orchestrator.py), so the hard-stop error is caught and it falls back to the template plan. Currently harmless: the first template step's _invoke re-checks the budget and raises before any provider call. Worth a narrow except BudgetExceededError: raise to keep the stop unambiguous.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Accepted. Local follow-up 7929f70 adds an explicit BudgetExceededError re-raise before the template fallback and a regression test. Focused verification passed 67 tests. The remote branch remains at 513a815 because the normal push was rejected by the active required-workflow ruleset; no bypass was used.
| restored = self.spend_analytics()["totals"] | ||
| self._budget_spent_output_tokens = max( | ||
| self._budget_spent_output_tokens, | ||
| restored["estimated_output_tokens"], | ||
| ) | ||
| self._budget_spent_cost_usd = max( | ||
| self._budget_spent_cost_usd, | ||
| restored["estimated_cost_usd"] or 0.0, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Budget ledger and run analytics intentionally diverge
spend_analytics reports budget.spent_output_tokens from the in-flight ledger, while totals.estimated_output_tokens still comes from persisted runs. They now diverge because the ledger counts planner, judge, and failover calls absent from any trace. Restart restore uses max(meter, run-estimate), which de-duplicates rather than sums, so no double counting; but when the char estimate exceeds reported tokens it over-states spend and can block sooner.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Reviewed against ADR 0014. The budget ledger intentionally includes in-flight provider calls that may not yet have persisted workflow rows; restart reconciliation uses the durable meter and avoids double counting. No source change is justified by this informational finding.
| with self._budget_spend_lock: | ||
| recorded_output_tokens = self._budget_spent_output_tokens - budget_before[0] | ||
| recorded_cost_usd = self._budget_spent_cost_usd - budget_before[1] | ||
| self._budget_spent_output_tokens += max( | ||
| 0, | ||
| workflow_output_tokens - recorded_output_tokens, | ||
| ) | ||
| self._budget_spent_cost_usd += max( | ||
| 0.0, | ||
| workflow_cost_usd - recorded_cost_usd, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Judge counted exactly once by reconciliation
The reconciliation adds max(0, workflow_output_tokens - recorded_output_tokens). When the judge records in-flight via the adapter, recorded >= workflow and it adds 0; when the judge bypasses in-flight recording, the reconciliation supplies judge_usage. Either way the judge is counted once, so this is a necessary safety net, not dead code.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Reviewed against ADR 0014. The reconciliation is the required safety net for judge usage when adapter metering is absent, while preventing duplicate counting when it is present. No source change is needed.
| if scheme == "local" and not _is_local_provider_url(self.base_url): | ||
| raise ValueError("local:// gateway URLs require a well-formed explicit loopback endpoint") |
There was a problem hiding this comment.
🔍 Existing host.docker.internal agents break on load
Removing host.docker.internal from LOCAL_PROVIDER_HOSTS plus the new __post_init__ check makes any local://host.docker.internal agent raise ValueError at construction. from_dict shares this path, so a stored agent-pool row or agents JSON using that host raises during load_all/load_agents on startup. Intended hardening, but a breaking migration for Docker-gateway configs.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Reviewed against ADR 0012 and the ADR 0004 decision recorded on 2026-08-21. local:// is intentionally authenticated and loopback-only; host.docker.internal is not a loopback guarantee and must remain rejected. The DNS resolution and rebinding check remains in the provider validator. No compatibility reintroduction is appropriate.
Maintainer audit — exact-head follow-up\n\n- Repository: ContextualWisdomLab/contextual-orchestrator\n- Pull request: #810\n- Remote exact head: \n- Base: ()\n- Remote state: OPEN, non-Draft, MERGEABLE, BLOCKED; formal approvals: 0.\n- Current hosted checks: 17 runs; 8 skipped/completed and 9 queued, including Close Empty, Strix, Dependency Review, Scorecard, Trivy FS, required-workflow bootstrap, OSV, scheduler sweep, and Semgrep. Queued runs are not success evidence.\n- Devin finding on generated planning: accepted. Local exact follow-up commit adds a narrow re-raise before template fallback and a regression test; focused verification: ................................................................... [100%]67 passed in 0.85s -> 67 passed.\n- Devin findings on budget/run analytics and judge reconciliation: reviewed against ADR 0014; no source change, retained as intentional ledger reconciliation behavior.\n- Devin finding on : reviewed against ADR 0012 and the 2026-08-21 ADR 0004 decision. The loopback-only contract and DNS rebinding check are intentional; no compatibility reintroduction is appropriate.\n- Normal push of the local follow-up was attempted against the unchanged remote head and rejected by the active ruleset because required workflows are not satisfied. No bypass, direct protected push, force push, fake status, or self-approval was used.\n- Decision: WAIT_AND_REMEDIATE. No merge or guarded force merge. |
|
Correction to the preceding maintainer audit: shell quoting stripped inline code markers from that comment. The authoritative details are: remote exact head 513a815; base 5379157; PR open, non-Draft, mergeable, blocked, with zero formal approvals. Hosted checks are 17 total, 8 skipped/completed and 9 queued. Local follow-up 7929f70 adds the narrow BudgetExceededError re-raise and regression test; focused verification passed 67 tests. The normal push was rejected by the active required-workflow ruleset. ADR 0012 and ADR 0004 support keeping host.docker.internal rejected. Decision remains WAIT_AND_REMEDIATE; no bypass or merge was used. |
|
Follow-up to the #810 maintainer audit: local commit 7929f70 was verified after the focused run with the full suite: 1646 passed in 552.26 seconds. Compileall, actionlint, git diff check, and Semgrep completed cleanly; pip-audit against requirements.lock reported no known vulnerabilities. These are local exact follow-up results only because the normal push remains rejected and the remote exact head is still 513a815. Hosted checks and formal approval remain outstanding. Decision remains WAIT_AND_REMEDIATE. |
|
At current parent head Validation: |
|
Superseded by merged #813. Exact tree comparison shows #813 merge |
1 participant
Customer outcome
The gateway now recovers from the reported Azure/LiteLLM default-only temperature rejection by retrying the same provider request once without the unsupported optional field, while preserving the original provider diagnostic and all normal fallback behavior.
Review remediation
Protected delivery
The organization ruleset began protecting every branch while this exact head was under validation, so direct update of PR #765s head was correctly rejected. This stacked fork PR is the normal non-bypass path into that protected branch.
Verification
Related parent: #765. Review and merge this stacked PR first; #765 then receives the fixes through its protected base branch.