fix(zdr): cite NVIDIA's own Trial ToS for the nvidia_nim not-ZDR classification - #1434
Merged
Conversation
…sification scripts/ci/zdr_policy.py already treated nvidia_nim/nvidia_nim_sub as not zero-data-retention, but only on an indirect third-party citation. Fetched NVIDIA's own current NVIDIA API Trial Terms of Service (v. September 19, 2025, the terms actually governing this org's free/trial integrate.api.nvidia.com key) directly. Section 3.3(iv) states NVIDIA collects "User Content and Generated Content to improve NVIDIA products and services, including AI models" -- affirmative evidence against ZDR, not merely an absence of attestation. zero_data_retention stays False; only the source/note/as_of fields change to cite and quote this primary source. Also records this session's broader ZDR/NIM-routing architecture review (repo-visibility check across the org, why free+ZDR is structurally near- empty for private targets, an audit of the three flagged direct-NIM call sites, and why a drafted Strix orchestrator/auto -> orchestrator/free change was reverted rather than merged -- it would reproduce the exact single-outage-domain incident ADR-0003 already fixed by switching Strix to auto), plus the pingora_edge_policy.py binary-evidence gap between the two already-open, partially-conflicting #1420/#1427 fixes, and a consolidated, evidence-based account of the still-open sidecar-preflight outage blocking noema-review/opencode-review/strix org-wide, in docs/product-technical-gap-baseline.md. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 6 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 (18)
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.
… free-catalog family cap Two changes, both part of the same investigation: 1. Switch Strix from orchestrator/auto to orchestrator/free, matching OpenCode and Noema. This supersedes ADR-0003's original orchestrator/auto decision by explicit, informed owner override: the trade-off ADR-0003 recorded (four free routes sharing one outage domain, so Strix -- which has no external fallback -- used the paid-inclusive auto pool for resilience) was surfaced back to the owner, including a live 2026-08-30 reproduction of that exact pattern recovered from a strix run artifact. The owner's decision, having seen both: implement free-only as originally instructed. docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md carries a dated amendment recording this as a superseding decision with the accepted risk spelled out, not a silent contradiction. 2. Root-caused the live "no provider route passed the Strix plain-chat preflight" outage that has been failing noema-review/opencode-review/ strix org-wide even after #1430's fix: build_zdr_prioritized_catalog's family_cap (default 4) groups nvidia_nim/nvidia_nim_sub as one family, and 100% of orchestrator/free's current candidates belong to that one family (23 distinct model ids). Selection sorts purely alphabetically with no reliability signal, so every run deterministically admitted the same 4 candidates -- 2 of which are confirmed NVIDIA-retired model ids (HTTP 404, forever) -- while ~19 healthy candidates in the same discovery report never got a chance. Raised ORCHESTRATOR_CATALOG_FAMILY_CAP's default from 4 to 8, a deliberately moderate, bounded mitigation (not a full fix -- see the dated comment at that line and the gap-baseline entry for the full reasoning, the latency trade-off, and the more complete live-catalog-freshness fix if this proves insufficient). Not verified against a live hosted run: this session has no provider credentials. Updated 8 test files, the reviewed-workflow blob-SHA pin in tests/test_pr_review_autofix_nvidia_nim_contract.py, scripts/ci/ strix_required_workflow_smoke.sh, and AGENTS.md to match. Full local suite: 1880 passed, 1 skipped, 100% interrogate; pingora_edge_policy.py's single coverage miss is pre-existing on main and untouched here. 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 finding on the prior commit: the comment/gap-baseline text said raising ORCHESTRATOR_CATALOG_FAMILY_CAP from 4 to 8 adds "up to ~80s more" worst-case preflight time versus the previous ceiling, but 4 candidates at REVIEW_PREFLIGHT_TIMEOUT_SECONDS=10s each was already up to ~40s worst case -- 8 candidates is up to ~80s total, so the actual delta is ~40s more, not ~80s more. Comment-accuracy fix only; the family_cap value itself (8) and the underlying root-cause finding are unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…-route limit Devin Review follow-up: the 80s figure only holds while a single provider family fills the free catalog (today's actual state). The picking loop independently stops at CATALOG_LIMIT (12) regardless of family_cap, so the absolute worst case across any number of distinct families was already REVIEW_PREFLIGHT_TIMEOUT_SECONDS x 12 = 120s before this session's family_cap raise (reached once family_cap x distinct families >= 12) and is unchanged by it. Spell out both the unchanged 120s ceiling and the shifted per-scenario figures (40s->80s at one family; 80s->120s at two) in both the sidecar comment and the matching gap-baseline entry. Co-Authored-By: Claude <noreply@anthropic.com>
…itation-20260830
…-pass Merging main into this branch picked up #1436, which independently fixes a different bug in the same sidecar script: the post-healthz gateway smoke request's max_tokens was desynchronized from REVIEW_MAX_OUTPUT_TOKENS, so a reasoning-capable route the launcher's own preflight had already proved ready could still exhaust its budget on internal reasoning before visible output, failing the separate smoke request with 502 invalid_structured_output. This is the precise mechanism behind the earlier PR #1433 "healthz reached, then 502" signature this doc described without yet knowing the cause -- a genuinely different bug from this session's own family-cap/stale-model finding, not a duplicate. Recorded so the two fixes aren't mistaken for redundant work. Co-Authored-By: Claude <noreply@anthropic.com>
…self A strix repository_dispatch run against PR #1434 was reported as a possible live test of the auto->free switch. It is not: the job failed at its "Self-test Strix required workflow contract" step, before provisioning the sidecar or running any scan. That step deliberately materializes the PR head's strix.yml and checks it against the trusted-base (current main, not yet carrying this PR's changes) strix_required_workflow_smoke.sh, which still asserts orchestrator/auto and explicitly rejects orchestrator/free -- the same pull_request_target-style trust boundary #1430 hit, now for this PR's own strix.yml + smoke script pair. Documents this plus a secondary, separate finding (a target-app-token 403 in the follow-up status-publish job, correctly fail-closed given the non-success scan result, surfacing a real but likely edge-case token-scoping gap). Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
Noting a structural, expected This PR edits Per this session's standing instruction for this exact class of self-editing trust-boundary PR: not forcing it, not bypassing it myself. Leaving it for the repo owner to bypass if warranted, same as #1430. Generated by Claude Code |
This was referenced Aug 30, 2026
Open
seonghobae
added a commit
that referenced
this pull request
Aug 30, 2026
seonghobae
pushed a commit
that referenced
this pull request
Aug 30, 2026
- Use owner/repo#num convention only for cross-repo references; same-repo mentions of #1433/#1434/#1437 use bare #num. - Point the sidecar's CATALOG_FAMILY_CAP comment at a durable commit SHA instead of the removing branch's name, which disappears once the branch is deleted after merge. - Correct the doctoring doc's grep-audit claim: one intentional code comment still names select_nvidia_nim_model.py on purpose as a searchable history pointer, so "zero references" was inaccurate.
seonghobae
pushed a commit
that referenced
this pull request
Aug 30, 2026
…xqzc Reconciles this branch's evidence-gated Strix pool-gating work with three bypass-merged main commits that landed while this PR was under review (#1434: unconditional Strix free-only flip with the single-outage-domain risk explicitly accepted, plus a family_cap 4->8 mitigation and an NVIDIA ZDR citation strengthening; #1436/#1440/#1439: sidecar preflight max_tokens and gateway-timeout fixes, unrelated to pool selection). Per the exact-head governance review on this PR (quoted in docs/adr/0003's new Amendment): an administrator bypass merge -- even one made with the risk explicitly surfaced and consciously accepted -- is not, by itself, operational acceptance of the underlying approach once a real review happens. This merge keeps #1434's orthogonal mitigations (family_cap raise, ZDR citation) and #1436/#1440/#1439's sidecar fixes, but replaces #1434's unconditional STRIX_MODEL/CONTEXTUAL_ORCHESTRATOR_POOL flip to orchestrator/free with this branch's evidence-gated conditional (free_family_diversity >= 2), restores strix_quick_gate.sh's is_contextual_orchestrator_model acceptance of orchestrator/auto (which #1434 had narrowed to free-only, which would have misrouted Strix's own safe fallback), and fixes several silent regressions where git's line-level merge took main's since-superseded literal because this branch's own text happened to match the merge-base after its earlier revert (not real conflicts, so no marker -- caught by re-running the full test suite and grepping for every orchestrator/free vs orchestrator/auto assertion post-merge). Also implements, in response to a live root-cause investigation surfaced during this merge: a bounded (1-retry) probe in the launcher's own preflight loop (already committed pre-merge) and a new opt-in --require-minimum-serving-diversity runtime floor in contextual_orchestrator_review_launcher.py -- a defense-in-depth backstop, distinct from strix.yml's decision-time free_family_diversity gate, that refuses to serve() a post-preflight pool spanning fewer than 2 independent provider families. Off by default (not yet enabled for any caller, including Strix): analysis found today's documented single-family-dominated free-route catalog means enabling it unconditionally would make even the safe orchestrator/auto fallback refuse to boot, a worse availability regression than what this change exists to prevent. See docs/adr/0020-strix-orchestrator-free-pool.md. Full local suite: 1871 passed, 1 skipped, 36 subtests passed. interrogate 100%. Coverage 99% (single pre-existing miss in pingora_edge_policy.py:274, unrelated and untouched by this branch, tracked separately). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae
pushed a commit
that referenced
this pull request
Aug 30, 2026
- Recompute free_family_diversity in _with_discovery_counts from the full discovery-wide row set rather than trusting whichever narrower rows build_zdr_prioritized_catalog was called with: the auto-pool primary stage only sees ZDR-admitted free rows (undercounting under --require-zdr) and the priced-fallback stage sees no free rows at all (always reading zero), both contradicting the field's documented "among all discovered free routes" contract. - Clarify free_family_diversity's docstring: it measures discovery-time route diversity, not preflight-confirmed readiness. - Correct docs/product-goal-directive.md's now-stale note: Strix already moved to orchestrator/free on main (merged #1434), it did not stay on orchestrator/auto pending a future evidence-gated follow-up. - Fix a stale test comment describing a malformed error_type as printing unchanged, when _log_preflight_rejections actually replaces it with the bounded "UnknownError" placeholder; add a positive assertion for that replacement. Full suite: 1903 passed, 1 skipped. Coverage 99% (single pre-existing, separately tracked gap in scripts/ci/pingora_edge_policy.py:345, unrelated to this change). interrogate: 100%.
seonghobae
added a commit
that referenced
this pull request
Aug 31, 2026
Bypass-merged under explicit user authorization. Removes scripts/ci/select_nvidia_nim_model.py, a leftover direct-to-NVIDIA-API resolver superseded by the contextual-orchestrator gateway's own auto-discovery (ADR-0003) — confirmed dead by two independent audits (this PR and #1434's ZDR/pool-migration review). 4 review threads, all resolved. Full suite: 1850 passed, 1 skipped, 99% coverage (pre-existing, unrelated gap only), 100% docstrings. noema-review confirms clean on this head. Only failing check is opencode-review's known-benign async-verdict pattern.
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
Resolves the split-timeout/batched-preflight design (this branch) against main's independently-landed ADR-0005 diagnostic, bounded-retry preflight (#1449/#1452 and predecessors #1436/#1440/#1434/#1425/#1426/#1422/#1413/ #1401/#1442), a genuine semantic merge, not a mechanical pick: - Keeps this branch's core contribution: launcher.py splits startup-route admission (10s) from real serving (120s, via _build_model_client), and probes up to REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES (24) candidates in concurrent batches of REVIEW_PREFLIGHT_BATCH_SIZE (4). - Folds in main's ADR-0005 escalation logic (16-token base probe, escalate to the real serving budget on a "budget too small" signature, shared REVIEW_PREFLIGHT_MAX_ESCALATIONS=4 cap) into the per-candidate probe used by that batching, via a new _EscalationBudget class so the shared budget stays a hard, lock-enforced invariant across concurrently-probed candidates within one batch -- a plain int (correct for main's original sequential loop) cannot coordinate that safely under concurrency. - Folds in main's ADR-0005 bounded retry for the sidecar's own separate gateway smoke request (up to REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS=3 attempts, 120s each, retried only on no-response/transport failure), replacing this branch's single-attempt transport_timeout/transport_error classification, while keeping this branch's "orchestration":"route" field and the 413-body-limit stderr-message-capture test enhancement. - Keeps this branch's family_cap==total-route-budget default (24) over main's more conservative raise to 8, since batching's concurrency keeps worst-case wall time bounded even at the higher route/family count (recomputed and re-tested below). - Two designs were deliberately NOT combined, resolved in main's favor after tracing each side's intent (not guessed): Strix's orchestrator/auto routing, which this branch's history re-added but main's most recent, explicit owner decision (#1434) reverted to orchestrator/free-only with the accepted-risk rationale recorded in ADR-0003; and a "fail closed on any partial provider discovery error" gate this branch added ( _require_complete_discovery), which main never adopted and whose philosophy directly conflicts with main's demonstrated-in-production "log the failure, continue with whatever succeeded" handling -- live BandScope evidence in this PR's own comments shows single-provider hiccups (Bytez 5xx) are common and should not be fatal to the whole pool. - Updates the two main-side tests whose literal worst-case-time assertions were computed for the old sequential (non-batched) design (test_preflight_stage_limits_share_one_startup_budget, test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case) to the batching-aware arithmetic, and drops this branch's now-superseded tests (orchestrator/auto Strix routing, fail-closed-on-partial-discovery, single-attempt gateway transport classification). Verification: coverage run -m pytest tests (2096 passed, 1 pre-existing skip, 21 subtests, 100% coverage on scripts/ci); interrogate (100% docstrings); git diff --check clean; bash -n on both changed shell scripts; ruff check clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Owner-directed architecture review, now with two implemented fixes plus the investigation itself.
1. ZDR citation (
scripts/ci/zdr_policy.py) — strengthens the existingnvidia_nim/nvidia_nim_subnot-ZDR classification with a direct primary-source citation (NVIDIA's own current NVIDIA API Trial Terms of Service, v. Sept 19, 2025, §3.3(iv): prompts/completions are used for training).zero_data_retentionstaysFalse. Full citation moved to a code comment above the entries;notetrimmed to one sentence per its own documented one-sentence contract (Devin finding, addressed).2. Strix
orchestrator/auto→orchestrator/free, per the owner's explicit, informed decision. This was drafted, then reverted on discoveringdocs/adr/0003-contextual-orchestrator-vendored-free-zdr.md's original, evidence-based rationale forauto(a 2026-08-29 incident: free routes collapsing to one outage domain, so Strix — no external fallback — used the paid-inclusive pool for resilience) and a live 2026-08-30 reproduction of that exact pattern. Surfaced back to the owner; their decision, having seen both, was to implement free-only as originally instructed. ADR-0003 carries a dated amendment recording this as a superseding decision with the accepted risk spelled out.3. Root-caused and mitigated the live sidecar-preflight outage that's been failing
noema-review/opencode-review/strixorg-wide even after #1430:build_zdr_prioritized_catalog'sfamily_cap(default 4) groupsnvidia_nim/nvidia_nim_subas one family, and currently 100% oforchestrator/free's candidates belong to that one family (23 distinct model ids). Selection sorts purely alphabetically with no reliability signal, so every run deterministically admitted the same 4 candidates — 2 of which are confirmed NVIDIA-retired model ids (HTTP 404, forever) — while ~19 healthy candidates in the same discovery report never got a chance. RaisedORCHESTRATOR_CATALOG_FAMILY_CAP's default from 4 to 8 — a deliberately moderate, bounded mitigation, not a full fix (see the dated comment at that line and the gap-baseline entry for the full reasoning, the latency trade-off, and the more complete live-catalog-freshness fix if this proves insufficient). Not verified against a live hosted run — this session has no provider credentials.4.
docs/product-technical-gap-baseline.md: consolidated, evidence-based account of all of the above, plus repo-visibility findings, the direct-NIM call-site audit, and apingora_edge_policy.pybinary-evidence gap between two already-open, partially-conflicting fixes (#1420, #1427 — commented on both).Test plan
coverage run -m pytest tests -q— 1880 passed, 1 skipped, 21 subtests (unchanged frommain)interrogate— 100.0% (unchanged)coverage report— 99% total, single pre-existing miss inscripts/ci/pingora_edge_policy.py:274(untouched by this PR)bash -non all touched shell scripts;python3 -c "yaml.safe_load(...)"on both touched workflow YAML filestests/test_pr_review_autofix_nvidia_nim_contract.py) to matchopencode-review-dispatch.yml's new contentorchestrator/auto-pinning test files updated toorchestrator/free, verified individually and in the full suiteNote: this PR's own
noema-review/opencode-review/strixchecks are expected to hit the currently-live sidecar-preflight outage documented in this same PR — items 3 above is this session's attempted mitigation for exactly that, not yet confirmed on a hosted run.🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw