fix(ci): group review catalog admission/diversity by outage domain, not account - #1474
fix(ci): group review catalog admission/diversity by outage domain, not account#1474seonghobae wants to merge 34 commits into
Conversation
…ot account model-catalog family (they are independent credentials that may expose different models), but in doing so also let the admission cap and free_account_diversity treat them as two fully independent outage domains. They are not: both resolve to the identical https://integrate.api.nvidia.com/v1 upstream (see PROVIDER_BASE_URLS in scripts/ci/zdr_policy.py, and that table's own nvidia_nim_sub ZDR-scope note, which already said as much). Two independent questions exist for this credential pair: 1. Model-catalog identity (may they expose different models?) -- yes, fixed correctly by #941/#945/#1468. 2. Outage-domain identity (would one physical outage take both down?) -- also yes, and #1468 flattened this axis to match axis 1. Concrete consequences fixed here: - free_account_diversity reported 2 for a discovery report whose only free routes were these two credentials -- falsely reassuring for exactly the decision this evidence exists to support (open PR #1437's Strix orchestrator/free eligibility gate: would a single outage empty the free catalog). - The admission cap (account_cap, sidecar default 8) let the pair jointly consume up to twice its intended per-endpoint budget, crowding out a genuinely independent provider's free routes even when it had capacity. Fix: contextual_orchestrator_review_policy.py gains _outage_domain(row), keyed on each row's own base_url evidence (not a second hand-maintained provider-name table, so it cannot go stale independently of the base_url evidence the catalog already serves from -- the exact failure mode that made the removed PROVIDER_FAMILIES mapping wrong). The admission cap now groups by outage domain; a new, additive free_outage_domain_diversity report field sits alongside the existing free_account_diversity (kept, not renamed, to avoid further naming churn right after #1468's own rename). contextual_orchestrator_review_launcher.py's _with_discovery_counts restores both fields from full discovery rows the same way. account_cap/DEFAULT_ACCOUNT_CAP/--account-cap/ORCHESTRATOR_CATALOG_ACCOUNT_CAP names are all left unchanged (still meaningful as "the cap value"; only its grouping was wrong) to minimize collision risk with .github#1469, which was concurrently advancing this same sidecar's pin. Tests: two dedicated regressions (semantic-conflation shape: 2 accounts, 1 domain; crowding-out shape: a shared-endpoint pair with many free rows vs. an independent provider with few) plus updated existing tests (test_build_catalog_applies_account_cap and friends, two launcher-facing tests in test_contextual_orchestrator_review_runtime_preflight.py). Full suite: 2095 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Not touched: open PR #1437's own gating logic -- its reviewer should read free_outage_domain_diversity, not free_account_diversity, for the >= 2 eligibility check. Does not revive closed PR #1470 (a different, now- superseded fix); this is a fresh, narrowly-scoped follow-up found by review against current main after #1468 merged. 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 38 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 (12)
📝 WalkthroughWalkthrough공유 upstream을 사용하는 계정의 admission cap과 장애 도메인 집계를 Changes장애 도메인 기반 무료 라우트 정책
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to The PR improves shared-upstream admission and outage-diversity reporting, but the current implementation can still prioritize priced or non-ZDR routes over free/ZDR routes under a small cap, reducing the intended free catalog. Documentation also reports an incorrect default cap, and IPv6 endpoint normalization can produce incorrect domain grouping, so merge should wait for these fixes. Sequence Diagram(s)sequenceDiagram
participant Discovery
participant contextual_orchestrator_review_launcher
participant build_zdr_prioritized_catalog
Discovery->>contextual_orchestrator_review_launcher: 무료 라우트와 base_url 제공
contextual_orchestrator_review_launcher->>build_zdr_prioritized_catalog: 계정 및 장애 도메인 집계 함수 전달
build_zdr_prioritized_catalog-->>contextual_orchestrator_review_launcher: catalog과 diversity 보고서 반환
contextual_orchestrator_review_launcher-->>Discovery: 전체 discovery 기준 다양성 결과 기록
🚥 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 31 functions across 4 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches📝 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 |
![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.
Devin Review finding on #1474: _outage_domain(row) compared raw base_url strings, so two rows for the identical physical endpoint but spelled differently (hostname case, an explicit default port like :443, a trailing slash) would be treated as two different outage domains -- directly undermining the fix free_outage_domain_diversity/the admission cap exist to provide. Verified against this codebase's actual code before acting: every DiscoveredModel.chat_base_url in contextual-orchestrator traces to one of a fixed set of hardcoded string literals (nvidia_nim/nvidia_nim_sub are byte-identical), and this repo's launcher copies that value verbatim, falling back only to zdr_policy.PROVIDER_BASE_URLS (confirmed byte-identical to the same literals). So this repo's one production caller (the sidecar/launcher) cannot produce inconsistent spellings today. It IS reachable through this script's own public --discovery-report CLI, which reads an arbitrary JSON file and isn't restricted to the launcher's exact generation path, and isn't wired into any current production workflow -- latent, not live, but real for that public surface. The fix is cheap and behavior-neutral on every input the sidecar produces today, so it's applied rather than left as an unstated assumption. _outage_domain now compares _normalize_base_url(row["base_url"]): lowercases scheme/host (case-insensitive per RFC 3986), drops an explicit port equal to the scheme's default, strips one trailing slash from the path. A different host, non-default port, path, or scheme still stays genuinely distinct. Falls back to a lowercased/stripped whole-string comparison (never raises) for anything unparseable into a scheme, host, and numeric port -- including a non-numeric port substring, which urlsplit(...).port raises ValueError on. Five new tests: the exact equivalent-spelling cases Devin named (case, default port, trailing slash), genuine distinctions still separate, no-raise on malformed/empty/bad-port input, and one end-to-end test through build_zdr_prioritized_catalog with two differently-spelled rows for the same endpoint (confirms the admission cap and diversity count both honor the normalization, not just the unit-level helper). Full suite: 2106 passed, 1 skipped, 21 subtests passed. 100% coverage (including the new fallback branch) and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Addressed the Devin Review finding on Verified it's real, but latent, not live, before fixing it. Traced every Given the fix is cheap and behavior-neutral on every input the sidecar produces today, applied it rather than leaving it as an unstated assumption: Five new tests: the exact equivalent-spelling cases named (hostname case, default port, trailing slash), genuine distinctions still separate, no-raise on malformed/empty/bad-port input, and one end-to-end test through Full suite: 2106 passed, 1 skipped, 21 subtests passed. 100% coverage (including the new fallback branch) and 100% docstring coverage on Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two Devin Review findings on this PR, one severe. Severe: shared-cap starvation within a domain. Grouping the admission cap by outage domain (this PR's own earlier fix) correctly stopped nvidia_nim/nvidia_nim_sub from jointly consuming 2x the intended budget across domains -- but the admission loop still walked rows in one strict sorted (cost-tier, ZDR, provider, model) order and admitted greedily until a domain's cap was reached. Since "nvidia_nim" < "nvidia_nim_sub" in every real fixture, nvidia_nim's rows always sort first, so nvidia_nim alone could consume the ENTIRE shared cap before a single nvidia_nim_sub row was ever considered. Verified concretely: 6 free nvidia_nim rows + 6 free nvidia_nim_sub rows, account_cap=4 -> nvidia_nim_sub got zero rows. Not "prevented from taking more than its share" (the bug already fixed), but "the alphabetically-first credential takes the whole shared budget, the other gets nothing" -- the same crowding-out problem, now within one domain instead of across domains. Fixed with a new _fair_admission_order() reordering step applied before the existing (otherwise unchanged) greedy admission loop: rows are partitioned by outage domain (each domain's whole block stays at the position of its first row's original appearance, so domain-vs-domain ordering is unaffected), and within any domain contributed to by more than one account, rows are taken in round-robin turns across those accounts -- one from each account's own priority-ordered queue per round -- instead of exhausting whichever account sorts first. A domain with only one contributing account (every provider except the shared NVIDIA pair, as of this writing) is returned completely untouched. Real: urlsplit() itself can raise, not only .port. _normalize_base_url's existing fallback wrapped only the .port property access; urlsplit() itself raises ValueError for an unmatched IPv6-literal bracket (e.g. https://[::1/v1, confirmed: "Invalid IPv6 URL"), before any scheme/host is even available to inspect -- an uncaught exception past this function's own "must never raise" contract. Fixed by wrapping the urlsplit() call itself in the same catch-and-fall-back pattern already used for .port. Noted, not chased further (info-level, optional per this session's coordinator): hostname canonicalization stops at lowercasing -- a trailing root-label dot, IDN Unicode-vs-punycode forms, and differently-compressed IPv6 literals aren't folded together. None of these shapes occur in any base_url this codebase produces today (every value traces to a fixed set of hardcoded, already-canonical HTTPS hostnames), so this is documented as a deliberate residual gap in _normalize_base_url's own docstring rather than implemented prophylactically. Tests: two existing tests whose assertions had encoded the starvation behavior were corrected to the fair-split expectation (test_build_catalog_applies_account_cap, test_build_catalog_prevents_shared_endpoint_from_crowding_out_independent_providers); added an end-to-end regression (test_build_catalog_shared_domain_cap_does_not_starve_second_account) and two unit-level tests directly against _fair_admission_order() (untouched single-account case; visible round-robin reordering with domain-block position preserved); added a regression for the IPv6 urlsplit() crash. Full suite: 2111 passed, 1 skipped, 21 subtests passed. 100% coverage (including the new reordering function and both new fallback branches) and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Addressed the two remaining Devin Review findings on this PR. 🔴 Severe — shared-cap starvation within a domain ( Fixed with a new 🟡 Real — malformed IPv6 URLs crash generation ( 🔍 Info #3 (hostname canonicalization) — left as a documented, deliberate residual gap rather than expanded further: root-label dots, IDN forms, and IPv6-canonical-form differences don't occur in any 📝 Info #4 — no action, as suggested; the "different path on one host counts as a separate domain" behavior is already documented in Full suite: 2111 passed, 1 skipped, 21 subtests passed. 100% coverage (including the new reordering function and both new fallback branches) and 100% docstring coverage on Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…llision Two Devin Review findings, one a real correctness regression the previous round's round-robin fix introduced. Real regression: fairness reordering could drop a free route for a paid one. _fair_admission_order grouped every row for one outage domain into a single block, emitted at the position of that domain's first appearance in the (already tier-sorted) input -- but a domain's rows can span multiple tiers (e.g. openai contributes both a free and a priced route, one single-account domain). Grouping by domain first, tier-blind, let a domain's lower-tier row get pulled into the same block as its higher-tier row, ahead of a different domain's higher-tier row that only sorted later because of the (provider, model) tie-break. Verified concretely: sorted [free openai, free openrouter, priced openai] reordered to [free openai, priced openai, free openrouter], and with limit=2 the genuinely free openrouter route got dropped for the priced openai one. Fixed by scoping the round-robin fairness pass strictly within one admission-priority tier at a time: eligible_rows and _fair_admission_order now share one _admission_priority_key() function (sort key and tier-boundary detector can no longer drift apart), the input is split into contiguous same-tier runs (safe -- already tier-sorted), and the existing domain/account round-robin logic (renamed _fair_order_within_tier) applies independently to each run, then the runs concatenate back in original order. Re-verified both the tier-priority scenario and the earlier starvation-fix scenario pass together. Real: IPv6 host normalization could collide two different endpoints. urlsplit().hostname strips IPv6 literal brackets ([::1] -> ::1); appending a port without re-adding them meant [::1]:8443 (host ::1, port 8443) and [::1:8443] (one IPv6 literal, no separate port) both normalized to the identical, syntactically-invalid ::1:8443. Fixed by re-wrapping a colon-bearing host in brackets before conditionally appending a port. Optional, applied since already in this code: round-robin queues switched from list.pop(0) (O(n)) to collections.deque.popleft() (O(1)). Tests: new unit-level and end-to-end regressions for tier-priority preservation; two new regressions for the IPv6 fix (distinct normalization, default-port-drop still works); existing _fair_admission_order tests updated for the now-required zdr_endpoints parameter. Full suite: 2115 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Addressed the round-4 Devin Review findings. 🟡 Real, most urgent — fairness reordering broke route priority. Confirmed concretely before fixing: sorted input Root cause: Redesigned per your guidance as two composable passes: 🟡 Real — IPv6 domains collide during normalization. Confirmed: 🔍 #3 (perf, optional) — applied since already in this code: round-robin queues switched from 📝 #4 — no action, as noted. Full suite: 2115 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
scripts/ci/contextual_orchestrator_review_launcher.py's _routable_discovered_models() unconditionally dropped every discovery row with evidence_only=True. contextual-orchestrator's OpenRouter ProviderModelSource hardcodes evidence_only=True for every discovered model unconditionally -- not computed per model from real evidence, even though genuine per-model ZDR evidence is fetched and parsed for OpenRouter in that same module. The upstream half of this bug is being fixed separately (a dispatched agent, PR forthcoming) -- not touched here. Consequence for this repo: with 100% of OpenRouter rows carrying evidence_only=True, this filter excluded ALL OpenRouter rows before scripts/ci/zdr_policy.py's own purpose-built, already-correct, already- wired per-route OpenRouter ZDR-feed check (is_zdr_model()'s openrouter_endpoints_feed branch) ever got a chance to evaluate a single one -- making that mechanism dead code for OpenRouter specifically, and leaving OpenRouter contributing zero routes to any pool despite genuinely offering ZDR-attested free models via its own documented feed. OpenRouter rows are now exempt from the evidence_only exclusion. A genuinely non-servable OpenRouter row is still excluded downstream by the same provider-agnostic chat-capability check every other provider's rows already go through (is_general_chat_agent_model_id + _has_text_output, in main()) -- this exemption relies on that existing, independent check, not on trusting evidence_only's current, wrong, blanket value for OpenRouter. Sequencing note, verified before writing this: this fix has real, immediate effect once merged, not only once contextual-orchestrator's own evidence_only fix and a matching ORCHESTRATOR_PIN_SHA bump also land. OpenRouter discovery already runs in this sidecar today, and for the general (non-private, require_zdr=False) pool -- what Noema/OpenCode/ default Strix use -- _zdr_admitted_rows() returns every row unfiltered regardless of ZDR status; is_zdr_model() only affects sort priority and tagging there, never admission. So genuinely chat-capable OpenRouter rows start reaching selection as soon as this merges. What remains gated on the upstream fix is OpenRouter rows being correctly excluded from evidence_only on a real per-model basis (e.g. a non-chat listing). Documented in the function's own docstring and this PR description. Tests: test_routable_discovered_models_excludes_evidence_only_rows (existing) corrected to use a non-OpenRouter provider for its evidence_only=True fixture; new test_routable_discovered_models_exempts_openrouter_from_evidence_only confirms both an evidence_only-tagged and untagged OpenRouter row pass through while a same-shaped row from a different provider does not; a contract-test assertion pins the exemption's presence in source. Full suite: 2093 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Devin Review on PR #1476 flagged that the blanket OpenRouter exemption in _routable_discovered_models() doesn't distinguish "vendored contextual-orchestrator still has the confirmed blanket evidence_only=True bug" from "vendored copy now computes evidence_only correctly per model" (the fix in contextual-orchestrator#950, open, not yet merged) -- so once #950 merges and ORCHESTRATOR_PIN_SHA bumps past it, this launcher would keep admitting genuinely evidence-only OpenRouter rows the corrected upstream code means to exclude. Considered gating on ORCHESTRATOR_PIN_SHA via git ancestry (merge-base --is-ancestor against #950's eventual merge commit, reachable from the sidecar's already-full vendored clone) but #950 has no merge commit yet, so there is no concrete threshold to gate on, and wiring the plumbing now (new CLI arg/env var, subprocess git call, sidecar/contract changes) would be built against a value that doesn't exist. Implemented instead: _openrouter_reports_per_model_evidence() reads this run's own discovered OpenRouter rows and turns the exemption off the moment any row reports evidence_only=False (real per-model evidence). While every row still reports True (today's exact bug signature), the exemption stays active. Self-corrects with no pin tracking and no manual conversion step once #950 merges. Known, accepted limitation documented in the docstring: a genuinely-fixed vendored copy that happens to report all-True in one run (feed-fetch failure, or zero attested models that run) is indistinguishable from the still-buggy signature by this check alone. Tests: split the previous mixed-fixture regression into test_routable_discovered_models_exempts_openrouter_when_every_row_reports_evidence_only (pre-fix blanket-True shape) and test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence (post-fix mixed shape). Full suite: 2094 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Follow-up recorded in docs/product-technical-gap-baseline.md with an explicit TODO referencing contextual-orchestrator#950 and this repo's #1476. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
…ts in outage-domain key Round-5 Devin Review found two real bugs left over in the outage-domain fairness logic: - _fair_order_within_tier collapsed every domain to one contiguous block positioned at that domain's first appearance, which silently displaced an unrelated domain's row whenever a shared domain's own rows were not already contiguous in priority order (e.g. [A1, B1, A2] became [A1, A2, B1], dropping B1 under a tight limit even though it outranked A2). Fixed by recording each domain's own global positions up front and only reordering which of a domain's own rows fills its own positions, never touching another domain's slot. - _normalize_base_url folded query and fragment into the outage-domain key together, so two identical endpoints differing only by a client-side-only #fragment reported as two domains with separate diversity counts and separate admission-cap budgets. The fragment is now stripped while the query string, which can be a real routing distinction, is still preserved. Both are covered by new regression tests exercising the internal reordering helper directly and the full build_zdr_prioritized_catalog path with a tight limit. 100% coverage/docstring gates re-verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
…finding
contextual-orchestrator#950 (cited by this PR as "the upstream half of this
bug, not yet merged") was closed as redundant/superseded. The fix actually
merged as contextual-orchestrator#949 ("fix(discovery): route OpenRouter by
model evidence", 8cd99f139915131ba0239bce12a5d6a5fd85394e); .github#1477
already advances ORCHESTRATOR_PIN_SHA to that commit. Corrects every stale
#950 reference in the PR's own docstrings and docs/product-technical-gap-
baseline.md, and folds in two things only visible from #949's actual diff:
- #949 also added DiscoveredModel.spend_admitted (default True, False for a
priced OpenRouter row when openrouter_paid_inference_available() cannot
confirm usable credit). orchestrator/free never considers priced rows, so
it was never exposed to this, but orchestrator/auto (real, reachable via
CONTEXTUAL_ORCHESTRATOR_POOL=auto, no other code change needed) does
consider priced rows and had no spend_admitted check anywhere in this
repo's pipeline. _routable_discovered_models() now excludes a
spend_admitted=False row the same way it excludes evidence_only=True,
with regression coverage including an end-to-end auto-pool composition.
- A fresh Devin Review red finding on this PR argued the OpenRouter
evidence_only exemption could let private/--require-zdr review content
reach ZDR-forbidden routes. Traced end to end: build_zdr_prioritized_
catalog() independently re-applies is_zdr_model()'s real OpenRouter ZDR-
feed check as its own admission gate whenever require_zdr=True, entirely
independent of evidence_only. Confirmed false alarm with a regression
test proving a non-ZDR-attested OpenRouter row is excluded from a
require_zdr=True catalog even while every discovered OpenRouter row still
carries the evidence_only=True bug signature; documented in the gap
baseline and replied/resolved on the GitHub review thread.
docs/product-technical-gap-baseline.md gets a new 2026-08-31 correction
subsection recording all of the above. Full suite: 2098 passed, 1 skipped,
21 subtests passed; 100% coverage on scripts/ci/; 100% docstring coverage.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
# Conflicts: # CHANGELOG.md
…utcome Devin Review (discussion r3891875665) on tests/test_contextual_orchestrator_ review_sidecar_contract.py:290 correctly noted the contract test's source check only requires the OpenRouter provider_name comparison to appear somewhere in source text -- a reversed (`!=` for `==`) or disconnected exemption would still satisfy that check. Verified by mutation: both mutations were applied locally and confirmed the old assertion alone would not have caught them (a "disconnect" mutation was crafted to leave the literal source fragment byte-for-byte intact while making the exemption a no-op). Add a direct behavioral assertion in the same test, against the already runpy-loaded launcher module's real `_routable_discovered_models`, that exercises one OpenRouter row that must be exempted and one same-shaped non-OpenRouter row that must not, asserting the actual filtered output. This fails under both the reversal and the disconnection mutation, closing the gap Devin identified without weakening the existing source-text checks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Ports the identical fix from #1506 into this branch. This PR's exact-head-path-policy check runs its own head-branch copy of scripts/ci/test_strix_quick_gate.sh (plain `pull_request` trigger in strix-changed-path-quality-ci.yml, not pull_request_target), so the pre-existing main-branch bug is not fixed here just by #1506 merging into main -- it needs porting into this branch directly. Root cause: assert_opencode_review_uses_codegraph_and_contextual_orchestrator extracted the required-workflow-bootstrap job block from opencode-review.yml with awk '/^ required-workflow-bootstrap:$/,/^[^ ]/'. Every job key in that workflow is indented 2 spaces (never column 0), so the end pattern never matched until EOF, sweeping an unrelated `if:` line from a later job (added by already-merged PR #1497) into the "block" and failing the assertion on unrelated content. Fixed by using an explicit state flag so the end pattern (`^ [A-Za-z0-9_-]+:`) is only tested starting on the line after the start match, correctly bounding the block to just its own lines. See #1506 for the full root-cause writeup and validation against origin/main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
grep -q exits on first match and closes its end of the pipe; if the upstream awk is still writing a large block, it gets SIGPIPE (141). Under `set -o pipefail` that non-zero awk status wins over grep's real 0, so `if pipeline; then` sees the pipeline as failed even though grep found a genuine match — silently missing e.g. a forbidden `if:` key or a fenced-diff marker that should have failed the check. Ports the same-file fix from PR #1506 to this branch's two call sites (required-workflow-bootstrap job-block check; opencode review REQUEST_CHANGES fenced-diff check). This branch already carried #1506's awk job-block-boundary correction, so only the grep -q removal was needed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Contextual-Orchestrator를 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. |
…alog-cap # Conflicts: # docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
Ports the identical fix from #1506 into this branch. This PR's exact-head-path-policy check runs its own head-branch copy of scripts/ci/test_strix_quick_gate.sh (plain `pull_request` trigger in strix-changed-path-quality-ci.yml, not pull_request_target), so the pre-existing main-branch bug is not fixed here just by #1506 merging into main -- it needs porting into this branch directly. Root cause: assert_opencode_review_uses_codegraph_and_contextual_orchestrator extracted the required-workflow-bootstrap job block from opencode-review.yml with awk '/^ required-workflow-bootstrap:$/,/^[^ ]/'. Every job key in that workflow is indented 2 spaces (never column 0), so the end pattern never matched until EOF, sweeping an unrelated `if:` line from a later job into the "block" and failing the assertion on unrelated content. Fixed by using an explicit state flag so the end pattern (`^ [A-Za-z0-9_-]+:`) is only tested starting on the line after the start match, correctly bounding the block to just its own lines. Confirmed FAIL before this fix, PASS after (bash scripts/ci/test_strix_quick_gate.sh). See #1506 for the full root-cause writeup. Co-Authored-By: Claude <noreply@anthropic.com>
… after rebase Same fix as #1444's identical rebase-time finding, applied here since this branch independently merged the same concurrent main PRs (#1532, #1533). Concurrent main PRs legitimately changed .github/workflows/opencode-review-dispatch.yml since this branch's last rebase, and this rebase's merge picked those changes up byte-for-byte (confirmed: `git diff origin/main -- .github/workflows/opencode-review-dispatch.yml` is empty). Two pre-existing contract tests were left pointing at stale expectations by that upstream change -- reproducible on origin/main's own tip, not introduced by this branch's diff: - REVIEW_DISPATCH_BLOB_SHA pinned the workflow's pre-#1532/#1533 blob SHA; updated to the current `git hash-object` value. - test_opencode_privileged_review_security_boundaries_are_fail_closed asserted the pre-#1533 strict `[ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ]` equality check. #1533 ("fix(opencode): proceed on head-only advance in review dispatch validation") deliberately removed head_sha from the fail-closed mismatch list -- a head advance between dispatch capture and this job is normal PR activity that every downstream job already re-validates independently (STALE_HEAD guards), so failing closed on it only starved the required review check of a verdict. Updated the assertion to check for the new warn-and-proceed behavior instead of the old fail-closed check it replaced. Co-Authored-By: Claude <noreply@anthropic.com>
Appends to this PR's own still-unmerged CHANGELOG bullet (matching this repo's convention of amending an unmerged PR's own entry in place rather than stacking a separate bullet for the same PR). Co-Authored-By: Claude <noreply@anthropic.com>
|
Rebase status update, folding in the review findings posted on this PR's pushed head during the rebase: Fixed: "Fallback remains single-domain" (Devin Review). Verified concretely: with both defaults at 4, Checked, false positive: CodeRabbit's "incorrect default cap" doc finding. The doc's "sidecar default 8" is correct — that's Checked, already correct: IPv6 endpoint normalization. Also, while rebasing across two rounds of concurrent Full suite green (2159 passed, 1 skipped), 100% coverage and 100% docstrings on Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review follow-up finding on this PR's pushed head: the previous
commit's _fallback_domain_aware_account_cap() shrank the per-domain
cap to fallback_limit // domain_count (floor) to guarantee every
outage domain a seat. That fixes starvation but wastes capacity
whenever fallback_limit does not divide evenly by domain_count --
concretely, limit=4 across 3 domains floored every domain to cap=1,
admitting only 3 routes even though a 4th eligible row existed in one
of those domains ("fallback quota wastes probe slots").
A single scalar per-domain cap cannot solve both problems at once (no
uniform cap value simultaneously guarantees every domain a seat *and*
leaves no capacity unused on an uneven split), so this replaces the
cap-shrinking helper with a new guarantee_domain_coverage flag on
build_zdr_prioritized_catalog itself: admission now runs in two passes
when set. The first pass admits at most one row per outage domain (in
the same priority order, bounded by account_cap and limit as before),
guaranteeing representation before any domain claims a second seat.
The second pass fills any remaining limit budget from the rows the
first pass did not pick, still respecting each domain's account_cap
ceiling (inclusive of the first pass's contribution) -- so the full
budget is used whenever enough eligible rows exist anywhere. The
picked order places every diversity (first-pass) row ahead of every
fill (second-pass) row, which is the more useful preflight try-order
for a pool whose entire purpose is outage-domain resilience, not
merely an implementation artifact.
_fallback_domain_aware_account_cap() is removed entirely rather than
kept alongside the new mechanism: it computed a value the new two-pass
admission no longer needs (both launcher call sites now pass
_catalog_account_cap(DEFAULT_ACCOUNT_CAP) directly again, unshrunk;
only the fallback call site additionally sets
guarantee_domain_coverage=True), and keeping an unused helper around
would just be a second, silently-driftable place answering the same
question.
Six new/replacement regression tests at the policy level, including
Devin's own two suggested non-divisible splits (limit=4 with 3
domains, and limit=8 with 3 domains) verifying both domain
representation and full use of available capacity, plus the
single-domain-unchanged case, the account_cap ceiling still holding,
and a domains-outnumber-limit edge case. Full suite green (2163
passed), 100% coverage and 100% docstrings on scripts/ci/.
Co-Authored-By: Claude <noreply@anthropic.com>
Updates the CHANGELOG bullet added two commits ago to describe the two-pass guarantee_domain_coverage mechanism instead of the now-removed cap-shrinking helper it originally described. Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review follow-up finding on this PR's pushed head: the guarantee_domain_coverage fix two commits ago only covered the priced-fallback stage. The identical single-scalar-cap-equals-limit coincidence is independently reachable through the *primary* auto-pool stage too, under the sidecar's real deployed configuration (not the DEFAULT_ACCOUNT_CAP=4 fixture value most of this file's tests use): contextual_orchestrator_review_sidecar.sh exports ORCHESTRATOR_CATALOG_ACCOUNT_CAP=8 by default, and this launcher's own REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT is also 8 for the auto pool's primary stage -- eight free routes from one dominant outage domain could exclude every independent free domain from the primary catalog entirely, at account_cap=limit=8 rather than the fallback stage's account_cap=limit=4. Wires guarantee_domain_coverage=True into main()'s primary build_zdr_prioritized_catalog call site as well. Full local suite (2163 passed before this change) confirmed no regressions from this extension -- guarantee_domain_coverage is a no-op whenever a stage's eligible rows only ever touch one outage domain, which covers every existing primary-stage test fixture. Adds a dedicated regression using the real deployed values (account_cap=8, limit=8, matching the sidecar's actual default rather than this file's usual account_cap=4 fixtures) reproducing Devin's exact scenario, plus extends the source-level wiring contract test to require guarantee_domain_coverage=True at both call sites. Full suite green (2164 passed), 100% coverage and 100% docstrings on scripts/ci/. Co-Authored-By: Claude <noreply@anthropic.com>
…1533 Main moved again mid-rebase: .github#1540 reverted #1533's warn-and-proceed head_sha check entirely (no rationale recorded beyond the revert itself), restoring the original strict fail-closed equality and, with it, the workflow file's original blob SHA (confirmed: git hash-object on origin/main's copy is exactly 2aa245e, byte-identical to what this file's REVIEW_DISPATCH_BLOB_SHA pinned before this whole detour started). Reverts this branch's own two prior commits' changes to these same two spots: REVIEW_DISPATCH_BLOB_SHA back to the original pin, and test_opencode_privileged_review_security_boundaries_are_fail_closed back to asserting the strict equality check instead of the now-reverted warn-and-proceed behavior. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 Head advances strand required reviews
When a pull request advances after dispatch, SUPPLIED_HEAD_SHA rejects the queued review although downstream work validates the live head. Active pull requests lose their required review verdict.
…ure-evidence-only-filter
Protected main regressed to 99% scripts/ci coverage after #1546 added live_head_matches, a no-active/no-stale fall-through in prepare_autofix_slot, and an "already queued or running" wait branch to pr_review_fix_scheduler.py without covering them, while the pre-existing inspect_pr conflicted-draft/conflicted-unauthorized returns and pr_review_merge_scheduler.py's fetch_workflow_names_by_check_suite_rest pagination/filtering/ permission-denied paths stayed untested. Every PR rebasing onto main inherits this via the coverage-evidence required check regardless of its own diff. Test-only change; no production code touched.
Adds a dated traceability entry for the coverage gap this PR closes: root cause (#1546's uncovered additions plus the older #1547/#1551/ #1554 gap, neither of which merged or transfers evidence here), the fix and its verification, the resolved Devin false-positive on sub-clause coverage, and the known pre-existing SIGPIPE test flake left unremediated as out of scope.
Raise scoped docstring coverage for the newly added scheduler REST regression helpers to 100% without changing test behavior or production code.
|
Converge the sidecar routing PRs into one non-conflicting stack instead of three parallel launcher writers. Canonical order: #1476 (OpenRouter |
…ure-evidence-only-filter # Conflicts: # CHANGELOG.md
…7) into fix/openrouter-premature-evidence-only-filter Interim stack: #1567 is not yet merged to main and closes the pre-existing post-#1546 scheduler coverage regression this PR would otherwise inherit via the merged-tree coverage-evidence gate. This merge is a no-op once #1567 lands on main and a future main-merge picks it up normally.
…e-only-filter' into fix/outage-domain-catalog-cap # Conflicts: # docs/product-technical-gap-baseline.md
|
Merged Validated on the new head ( @opencode-agent please review the new exact head Generated by Claude Code Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review found that build_zdr_prioritized_catalog's guarantee_domain_coverage two-pass admission ran across the full tier-sorted row list instead of within one admission-priority tier at a time, letting a worse-tier row win a first-pass "guaranteed representation" seat for its domain ahead of a better-tier row from an already-represented domain. Restructured both passes to iterate contiguous same-tier runs of ordered_rows in priority order, finishing a tier's own first and second pass before ever looking at the next, worse tier -- the same tier-boundary discipline _fair_admission_order already enforces for its own reordering. covered_domains/per_domain still accumulate across tiers so a domain already seated in a better tier does not claim a redundant guaranteed seat in a worse one. Added a regression proving the pre-fix behavior red (two free/ZDR openrouter routes plus one free/non-ZDR bytez route, limit=2: both admitted rows must stay free/ZDR) before confirming it green after.
# Conflicts: # CHANGELOG.md # docs/product-technical-gap-baseline.md # scripts/ci/contextual_orchestrator_review_launcher.py # scripts/ci/contextual_orchestrator_review_policy.py # tests/test_contextual_orchestrator_review_policy.py
Merging current main pulled in three overlapping changes to the same review-catalog code this PR's own outage-domain-cap-grouping fix touches: - #1587 added FREE_POOL_CREDENTIAL_NAMES source-authorization filtering (scripts/ci/contextual_orchestrator_review_policy.py and contextual_orchestrator_review_launcher.py's _with_discovery_counts, plus tests/test_contextual_orchestrator_free_pool_enrichment.py) and is independent of this PR's outage-domain grouping -- combined both additions in _with_discovery_counts (free_outage_domain_diversity alongside the free_pool_* fields) and in build_zdr_prioritized_catalog's report dict, dropping one duplicate free_account_diversity key the merge introduced. - #1592 fixed test_build_catalog_applies_account_cap's stale "openai" fixture (pre-#1587) to "openrouter" on protected main; this PR's own branch had already independently rewritten the same test's assertions for the outage-domain-grouped cap semantics (nvidia_nim/nvidia_nim_sub sharing one domain's cap) but still referenced the stale "openai" key in its own assertion -- corrected to "openrouter", matching the fixture's already-current provider name. - test_contextual_orchestrator_free_pool_enrichment.py's own new test called _with_discovery_counts without the outage_domain keyword this PR's fix requires; added a matching lambda and a free_outage_domain_diversity assertion, preserving the test's original free-pool-enrichment intent. - #1564 (merge-base-anchored deleted-file review evidence) left tests/test_noema_review_gate.py and tests/test_noema_removed_file_context.py broken against renamed/removed noema_review_gate.py functions; ported the same fix already opened as its own dedicated PR (#1598). Full suite: 2362 passed, 100% branch coverage, 100% docstrings.
| if guarantee_domain_coverage: | ||
| covered_domains: set[str] = set() | ||
| tier_start = 0 | ||
| total = len(ordered_rows) | ||
| while tier_start < total and len(picked) < limit: | ||
| tier = _admission_priority_key( | ||
| ordered_rows[tier_start], zdr_endpoints=zdr_endpoints | ||
| )[:2] | ||
| tier_end = tier_start + 1 | ||
| while ( | ||
| tier_end < total | ||
| and _admission_priority_key( | ||
| ordered_rows[tier_end], zdr_endpoints=zdr_endpoints | ||
| )[:2] | ||
| == tier | ||
| ): | ||
| tier_end += 1 | ||
| tier_rows = ordered_rows[tier_start:tier_end] | ||
| tier_start = tier_end | ||
|
|
||
| first_pass_ids: set[int] = set() | ||
| for row in tier_rows: | ||
| if len(picked) >= limit: | ||
| break | ||
| domain = _outage_domain(row) | ||
| if domain in covered_domains or per_domain[domain] >= account_cap: | ||
| continue | ||
| covered_domains.add(domain) | ||
| per_domain[domain] += 1 | ||
| picked.append(row) | ||
| first_pass_ids.add(id(row)) | ||
| for row in tier_rows: | ||
| if len(picked) >= limit: | ||
| break | ||
| if id(row) in first_pass_ids: | ||
| continue | ||
| domain = _outage_domain(row) | ||
| if per_domain[domain] >= account_cap: | ||
| continue | ||
| per_domain[domain] += 1 | ||
| picked.append(row) |
There was a problem hiding this comment.
📝 Info: Tier priority survives domain coverage
guarantee_domain_coverage finishes both passes within each cost/ZDR tier. Lower-tier routes cannot displace higher-tier routes, while caps remain catalog-wide.
Was this helpful? React with 👍 or 👎 to provide feedback.
| domain_positions: dict[str, list[int]] = {} | ||
| for index, row in enumerate(rows): | ||
| domain_positions.setdefault(_outage_domain(row), []).append(index) | ||
|
|
||
| ordered: list[Mapping[str, Any]] = list(rows) | ||
| for positions in domain_positions.values(): | ||
| bucket = [rows[index] for index in positions] | ||
| account_order: list[str] = [] | ||
| queues: dict[str, deque[Mapping[str, Any]]] = {} | ||
| for row in bucket: | ||
| account = provider_account(str(row["provider"])) | ||
| if account not in queues: | ||
| account_order.append(account) | ||
| queues[account] = deque() | ||
| queues[account].append(row) | ||
| if len(account_order) <= 1: | ||
| continue | ||
| reordered: list[Mapping[str, Any]] = [] | ||
| while any(queues[account] for account in account_order): | ||
| for account in account_order: | ||
| queue = queues[account] | ||
| if queue: | ||
| reordered.append(queue.popleft()) | ||
| for index, row in zip(positions, reordered): | ||
| ordered[index] = row |
There was a problem hiding this comment.
📝 Info: Fairness preserves independent positions
Round-robin replacement touches only slots already owned by one outage domain. Interleaved independent routes retain their original priority positions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| discovered = list(discovered or []) | ||
| openrouter_still_blanket_marked = not _openrouter_reports_per_model_evidence(discovered) | ||
| return [ | ||
| model | ||
| for model in discovered | ||
| if ( | ||
| not getattr(model, "evidence_only", False) | ||
| or ( | ||
| openrouter_still_blanket_marked | ||
| and getattr(model, "provider_name", None) == "openrouter" | ||
| ) | ||
| ) | ||
| and getattr(model, "spend_admitted", True) is not False |
There was a problem hiding this comment.
📝 Info: Private routing remains ZDR-gated
The OpenRouter workaround broadens discovery candidates only. Private-target candidates still pass the independent is_zdr_model filter before admission.
Was this helpful? React with 👍 or 👎 to provide feedback.
| text = base_url.strip() | ||
| try: | ||
| parsed = urlsplit(text) | ||
| except ValueError: | ||
| return text.casefold() | ||
| if not parsed.scheme or not parsed.hostname: | ||
| return text.casefold() | ||
| try: | ||
| port = parsed.port | ||
| except ValueError: | ||
| return text.casefold() | ||
| scheme = parsed.scheme.casefold() | ||
| host = parsed.hostname.casefold() | ||
| # urlsplit().hostname strips IPv6 literal brackets (``[::1]`` -> ``::1``). | ||
| # Re-adding them whenever the host itself contains a colon -- before ever | ||
| # conditionally appending a port -- is required for two reasons: without | ||
| # it, an explicit-port IPv6 URL (``[::1]:8443``) and a bracketless, | ||
| # colon-bearing literal address that merely *looks* like host:port when | ||
| # flattened (``[::1:8443]``, port None) collapse to the identical | ||
| # ``::1:8443`` string despite being different addresses; and the | ||
| # reassembled ``netloc`` must stay valid host:port syntax regardless. | ||
| bracketed_host = f"[{host}]" if ":" in host else host | ||
| netloc = ( | ||
| bracketed_host | ||
| if port is None or port == _DEFAULT_PORTS.get(scheme) | ||
| else f"{bracketed_host}:{port}" | ||
| ) | ||
| path = parsed.path.rstrip("/") | ||
| return urlunsplit((scheme, netloc, path, parsed.query, "")) |
There was a problem hiding this comment.
📝 Info: URL normalization avoids false domains
Normalization folds equivalent endpoint spellings while preserving meaningful distinctions. Malformed evidence falls back deterministically instead of aborting catalog construction.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Closing as superseded by the current no-heuristics review-admission owner lane #1629. This PR deliberately makes an outage-domain |
2 participants
Summary
Follow-up to already-merged
.github#1468("fix(ci): keep sidecar credential accounts independent"), found by review during this session (a Devin Review finding, checked directly againstmain's actual merged code before acting — not against the now-closed, superseded PR #1470).#1468correctly stopped treatingnvidia_nim/nvidia_nim_subas one model-catalog family (they are independent credentials that may expose different models — matchingcontextual-orchestratorPR #941/#945). But in doing so, it also let the catalog's admission cap and itsfree_account_diversityevidence field treat them as two fully independent outage domains. They are not: both resolve to the identicalhttps://integrate.api.nvidia.com/v1upstream (seePROVIDER_BASE_URLSinscripts/ci/zdr_policy.py, and that table's ownnvidia_nim_subZDR-scope note, which already said as much).Two genuinely different questions exist for this credential pair:
Concrete consequences fixed here
free_account_diversityreported2for a discovery report whose only free routes were these two credentials — falsely reassuring for exactly the decision this evidence exists to support (whether a single outage could empty the free catalog; seedocs/adr/0003-contextual-orchestrator-vendored-free-zdr.md's "Monitoring evidence" section, anddocs/product-goal-directive.md§8's note on the accepted single-outage-domain risk, both amended in this PR).account_cap, sidecar default 8) let the pair jointly consume up to twice its intended per-endpoint budget — a milder recurrence of the 2026-08-30orchestrator/freeexhaustion incident this cap exists to prevent (documented indocs/product-technical-gap-baseline.md): a shared endpoint's rows could crowd out a smaller, genuinely independent provider's free routes even when that provider had capacity available.Fix
scripts/ci/contextual_orchestrator_review_policy.pygains a second, distinct grouping,_outage_domain(row), keyed on each row's ownbase_urlevidence — not a second hand-maintained provider-name table, so it cannot silently go stale independently of thebase_urlevidence the catalog already serves from (the exact failure mode that made the removedPROVIDER_FAMILIESmapping wrong in the first place).free_outage_domain_diversity, is added alongside the existingfree_account_diversity(additive, not a rename — to avoid further naming churn immediately after fix(ci): keep sidecar credential accounts independent #1468's own rename and docs(product-goal-directive): rename free_family_diversity to free_account_diversity #1471's doc-reference fix).scripts/ci/contextual_orchestrator_review_launcher.py's_with_discovery_counts(which recomputes diversity from full discovery-wide rows, not the narrower per-stage set) restores both fields the same way.account_cap/DEFAULT_ACCOUNT_CAP/the CLI--account-capflag/the sidecar'sORCHESTRATOR_CATALOG_ACCOUNT_CAPenv var names are all left unchanged (still meaningful as "the cap value"; only its grouping was wrong) to minimize collision risk with.github#1469, which was concurrently advancing this same sidecar's pin in the same active window.docs/adr/0003-contextual-orchestrator-vendored-free-zdr.mdanddocs/product-goal-directive.md§8 both get a short correction pointing future readers atfree_outage_domain_diversityfor the single-outage-domain-risk question specifically.Developer experience
Two dedicated regressions reproduce the exact gaps:
test_build_catalog_counts_same_vendor_credentials_independently(updated):nvidia_nim+nvidia_nim_subalone reportfree_account_diversity == 2butfree_outage_domain_diversity == 1— the semantic-conflation bug.test_build_catalog_prevents_shared_endpoint_from_crowding_out_independent_providers(new): a shared-endpoint credential pair with far more free rows than an independent provider; the shared endpoint's admissions are correctly capped, leaving room for the independent provider (bytez/openrouterboth fully admitted, NVIDIA-domain capped toaccount_cap, not2 * account_cap).Existing tests updated to the corrected, domain-aware expectations:
test_build_catalog_applies_account_cap,test_build_catalog_reports_free_account_diversity, and two launcher-facing tests intest_contextual_orchestrator_review_runtime_preflight.py(including a new one,test_discovery_counts_distinguish_account_from_outage_domain_diversity).Full details in
docs/product-technical-gap-baseline.md's new 2026-08-31 entry.User experience
free_outage_domain_diversity, as reported by the sidecar's policy/preflight evidence, now correctly answers "would a single provider outage empty the free catalog" — the questiondocs/adr/0003-contextual-orchestrator-vendored-free-zdr.md's accepted-risk monitoring anddocs/product-goal-directive.md§8's note both actually need. The catalog's admission cap no longer lets two same-endpoint credentials jointly absorb twice their intended share of the bounded route budget, which is directly relevant to Strix'sorchestrator/freereliability now that it is hardcoded offorchestrator/auto(ADR-0003's 2026-08-30 amendment).Test plan
coverage run -m pytest tests -q(full suite) — 2095 passed, 1 skipped, 21 subtests passedcoverage report --show-missing— 100% onscripts/ci/interrogate— 100%Related
.github#1468(merged) — the fix this extends..github#1469/#1471/#1472(merged, concurrent) — pin advancement and a doc-reference fix; no file overlap with this PR's substantive changes beyond a clean rebase.free_outage_domain_diversity, notfree_account_diversity, if/when wiring an outage-domain-based eligibility check (note: perdocs/product-goal-directive.md§8, Strix is currently hardcoded toorchestrator/freeregardless of this evidence, so this is monitoring evidence, not presently a gate).PROVIDER_FAMILIESbug, already covered by fix(ci): keep sidecar credential accounts independent #1468). This PR does not revive it; it is a fresh, narrowly-scoped follow-up found by review against currentmainafter fix(ci): keep sidecar credential accounts independent #1468 merged.Generated by Claude Code
Summary by CodeRabbit
개선 사항
문서