feat: evidence-grade NIM discovery + all-modality cost-quality benchmark - #90
feat: evidence-grade NIM discovery + all-modality cost-quality benchmark#90seonghobae wants to merge 200 commits into
Conversation
…ark (#86) Optional stdlib-only benchmark harness (contextual_orchestrator/nim_benchmark.py): - Dynamic catalog discovery from the OpenAI-compatible GET /v1/models — no hard-coded inventory; deduplicated, sorted (response-order-drift immune), with machine-readable duplicate/invalid hygiene lists. - Capability probes for every contract NIM can host: chat completions, text completions, Responses API, embeddings, image understanding, video understanding, omni-style audio understanding, audio transcription, and audio speech — omni_capable derived, skipped probes always carry a machine-readable reason, bounded concurrency under one shared hard request budget. - Fair policy comparison on a locked task split: per-worker direct baselines (source of best-single-worker-in-hindsight), route_once, conduct capped at five steps, cheapest-eligible-worker; identical scorers, caps, timeouts, and token budgets across systems. - Honest cost accounting: actual cost 0 while the hosted catalog is free; hypothetical paid cost only from an explicit versioned pricing scenario, "unknown" otherwise; the two never mix. - Paired-bootstrap CIs, quality-latency and quality-hypothetical-cost Pareto frontiers, full provenance (git SHA, run id, catalog/manifest/ pricing hashes, parameters), schema-validated JSON/CSV/Markdown artifacts with a secret-leak refusal guard. - Fail closed: missing KV credential (NVIDIA_NIM_API_KEY, bootstrap env->KV only, never argv), incomplete discovery, exceeded budget, missing provenance, schema violations. - Deterministic --dry-run drives the whole pipeline against an in-process synthetic provider covering every modality class — zero network, byte-identical artifacts. - Tests: 100% statement+branch coverage of the new module, adversarial cases (malformed catalogs, duplicate ids, non-finite tokens/costs, rate limits, timeouts, order drift, secret redaction); new fuzz seam (Hypothesis + Atheris) for the catalog parser. - CI: manual + conservative monthly scheduled workflow, single-flight concurrency, hard budgets, pinned actions, 90-day artifact retention. - Docs: docs/nim_benchmark.md, architecture/tracks pointers, HELM (arXiv:2211.09110) added to docs/papers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughNVIDIA NIM 비용·품질 벤치마크 하네스를 추가했습니다. 카탈로그 탐색, modality 검증, 정책 평가, 비용·증거 검증, 결정적 dry-run, live CI 실행, artifact 생성 및 품질 테스트를 포함합니다. ChangesNIM 벤치마크 기능과 검증
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant GitHubActions
participant NIMBenchmark
participant NIMProvider
participant ArtifactStore
Operator->>GitHubActions: dry-run 또는 live 실행 요청
GitHubActions->>NIMBenchmark: 고정된 예산·manifest·provenance 전달
NIMBenchmark->>NIMProvider: 카탈로그 조회 및 capability probe
NIMProvider-->>NIMBenchmark: 모델·probe 결과 반환
NIMBenchmark->>NIMProvider: 평가 task 요청 전송
NIMProvider-->>NIMBenchmark: 응답·usage 반환
NIMBenchmark->>ArtifactStore: JSON·CSV·Markdown artifact 업로드
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
|
Exact-head maintainer audit of
Do not mark this PR ready or resolve these findings from stale checks. Re-run all required workflows and independent review on the exact repaired head. |
seonghobae
left a comment
There was a problem hiding this comment.
Blocking security finding
CRITICAL — benchmark HTTPS transport reintroduces DNS-rebinding SSRF
require_public_https_endpoint() validates one DNS answer, but build_default_transport() then calls urllib.request.urlopen(), which resolves the hostname again. The validated_hosts cache widens the gap by skipping validation on later calls. A provider hostname can therefore pass public-address validation and later connect to loopback, private, RFC 6598, link-local, or otherwise non-global infrastructure. Redirects are also delegated to urllib's default handler, so credentials may be forwarded to an unvalidated destination.
This is the same TOCTOU class repaired by PR #76. Keep this PR Draft until #76 is in main, then reuse or generalize its DNS-pinned transport so the socket dials only validation-time public IPs while preserving the original hostname for HTTP authority, TLS SNI, and certificate verification. Reject redirects, bypass environment proxies, require is_global, clear stale pins before each validation, and deterministically close failed sockets/responses.
Required regression evidence:
- no transport-time DNS re-resolution;
- public IPv4/IPv6 pinning and hostname/SNI preservation;
- RFC 6598/private/loopback/link-local/multicast/reserved/unspecified rejection;
- redirect rejection with no authorization propagation;
- approved-IP fallback and all-address failure;
- exact-head statement/branch coverage and full repository checks.
Do not merge based on the current local 100% claim; it does not cover this security invariant.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review — still blocked
The latest head addresses the three audit themes, but the implementation is not yet mergeable.
1. HIGH — production security behavior is coupled to a test monkeypatch
nim_benchmark_hardening._build_secure_transport() deliberately falls back to urllib.request.urlopen() whenever that global has been replaced. This keeps the old offline tests green by changing production behavior instead of replacing the obsolete tests. It reintroduces the hostname-resolving, proxy-aware, redirect-capable path that this repair is supposed to eliminate, and requires a nosemgrep waiver on the exact sink.
Remove the compatibility branch entirely. Rewrite the transport tests to inject the pinned connection/resolver seam and prove the real direct transport. Do not let test instrumentation select a less-secure production path.
2. HIGH — the new 531-line hardening module has no direct regression suite
The current tests/test_nim_benchmark.py still asserts the old urlopen behavior and contains no evidence for EqualBudgetModelClient, expiry enforcement, actual-cost evidence validation, configured/observed budget fields, or the installed wrappers. A 100% repository claim cannot be accepted while the newly shipped module and its branches are not directly exercised.
Add behavior tests that fail without each contract, then prove exact-head statement and branch coverage at 100% for both nim_benchmark.py and nim_benchmark_hardening.py, plus 100% public docstrings.
3. HIGH — cited cost source does not support the recorded claim
ACTUAL_COST_EVIDENCE names https://docs.nvidia.com/nim/large-language-models/latest/faq.html, but the current NVIDIA FAQ location is different and the available FAQ content does not establish that the API Catalog hosted endpoint used by this run is free to the caller. Current build.nvidia.com model pages do label prototype endpoints as free, while NIM offering/licensing documentation distinguishes free exploratory offerings from NVIDIA AI Enterprise production support. Record the exact reviewed page/version or immutable evidence artifact that supports the hosted endpoint claim; do not cite a generic or moved FAQ as proof.
4. MEDIUM — optional adapter boundary is lost
contextual_orchestrator.__init__ now imports the entire benchmark and monkeypatch installer for every package import. Keep the NIM evaluator optional: integrate the fix directly into its module, or install it only from the benchmark entrypoint. Runtime gateway users should not import benchmark-only code or mutate benchmark globals as a side effect of import contextual_orchestrator.
5. MEDIUM — temporary privileged workflow remains in the PR tree
.github/workflows/temporary-nim-transport-fix.yml has top-level contents: write, persists checkout credentials, and triggers on every branch push. It must be removed from the final tree. Any one-shot repair must use read-only defaults, isolate credentials to the publication step, validate exact changed paths, and disappear before review.
Keep the PR Draft. Replace the compatibility installer with direct, test-first implementation; remove the temporary workflow; update the PR body to the actual head; then rerun all exact-head checks and independent review.
|
Exact-head maintainer follow-up for the new hardening commits:
Do not resolve the security thread or mark the PR ready until these are implemented and verified on the exact repaired head. |
test_reopened_nim_scaffold_is_superseded_without_false_closed_state correctly flags any TRACEABILITY.md line that mentions PR #115 (open, reopened, classified superseded) alongside "closed-unmerged" wording that actually describes predecessor PR #90's status, not #115's. Split the two facts onto lines that don't co-occur: the #86 backlog table row and prose paragraph now state PR #90's closed-unmerged predecessor status separately from PR #115's open/superseded status. No factual claim changes, only which physical line each claim sits on.
Purpose
Implement issue #86 as a provider-neutral, evidence-grade NVIDIA NIM discovery and benchmark harness while preserving standalone operation and the modular CWL MSA boundary.
Exact integration identity
26f8d8dc5634f0371fad0801056e9a3450c78bfffix/atheris-interpreter-lock3703d0da9823b8258a0be94f1801aa5d61bfad9f2df63e080737d5aa29d40871af99aa13411c3d76; it is not treated as the current stacked-base tip.The base branch is independently maintained and may move. Before every mutation and acceptance decision, live GitHub PR/ref metadata is authoritative. Any check, status, review, approval, mergeability result, or synthetic merge associated with another head/base pair is historical only.
Current-head verification state
For exact contributor head
26f8d8dc5634f0371fad0801056e9a3450c78bff:31182517889: success;31182517906: success;31182518012: success;Absent evidence is not success. The current GitHub review-thread inventory reports zero unresolved inline threads. The addressed transactional-publication finding still records test-first staging, complete-set validation, same-filesystem publication, rollback/recovery, and cleanup; its resolved state is not approval and does not substitute for post-reconciliation exact-head evidence.
The stacked base has advanced after this branch's earlier reconciliation, so no historical mergeability/check evidence is reused. After PR #96 integrates, this PR must be reconciled or retargeted from the exact integrated protected head and every required gate must be regenerated.
Implemented benchmark scope
GET /v1/modelscatalog without a hard-coded authoritative inventory;route_once, boundedconduct, and an optional reviewed cheapest-worker scenario under common total token and call envelopes;unknown;Strict scoring validity boundary
A test-first buyer-visible slice repairs construct-irrelevant score inflation in the locked task set.
337356117ede1b69ba1b47282038bfa0fcc9c360required complete-answer scorers that did not yet exist.exact_number_matchversion2accepts only one complete finite decimal response; prose, negation, units, multiple values,NaN, and infinities do not earn credit.exact_text_matchversion1compares the complete NFC-normalized, whitespace-normalized, case-folded response against explicit declared alternatives; substrings and undeclared aliases do not match.import contextual_orchestratordoes not import the benchmark, CSV adapter, or strict-scoring adapter;An isolated networkless harness exercised all 142 strict-scoring production statements and 54 branches and passed; that diagnostic validates the bounded module logic but is not repository CI, trusted central coverage, or merge evidence. Authoritative doctoring remains in
docs/doctoring/nim-benchmark-strict-answer-scoring.md.Provider and credential security boundary
NVIDIA_NIM_API_KEY; only the bounded live benchmark step receives the GitHub Secret;COPILOT_GITHUB_TOKENpath is introduced;Historical central dependencies — read-only
Protected central
.githubmainremains exact tip6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba.The live central continuation is
.github#937at exact head8726df151e64eecb89d91a4c029e809a785ee126and.github#939at exact headac5665148bb113f92e97d2fc49a729bca2f050b5. #937 has ten terminal-success workflows, zero unresolved threads, and COMMENTED-only reviews; #939 has nine terminal-success workflows, zero threads, and no formal review. Both are Ready but neither is protected-main authority or qualifying approval. Closed or superseded #888/#906/#929 evidence remains historical and does not transfer.PR #96 remains Draft at exact head
3703d0da9823b8258a0be94f1801aa5d61bfad9fon protected repositorymain@6841b71935e0b7cb98fb52bcb4709cc5100c8d87. This benchmark PR must reconcile only after the accepted protected #96 result exists and must regenerate every exact-head gate.This repository does not write central branches or dispatch repair agents there.
Evidence contracts
Permanent regressions cover complete-plan admission, one-request-short zero-partial egress, deterministic bounded concurrency, valid media fixtures, DNS rebinding and non-global egress, redirects, proxy and credential isolation, bounded provider bodies, equal token/call budgets, reviewed pricing expiry, strict scoring, complete assignment evidence, Pareto exclusion labels, schema completeness, transactional publication, rollback/crash recovery, package import isolation, exact contributor-head workflow checkout, fuzz instrumentation, and buyer-readable evidence-status semantics.
Stack order and merge acceptance
main; their current Ready state and green workflows are not protected-main authority;Queued, pending, skipped-required, cancelled, absent, stale-head, predecessor-head, failed, status-only, author-only, rate-limited, or synthetic-merge evidence is never success.
Refs #86.
Depends on #96 and the live protected-main central remediation path. Refetch its current successor before any acceptance decision; no historical PR number, head, or workflow evidence transfers.
Live dependency refresh (2026-08-12 05:07 UTC)
Read-only refetch superseding the historical dependency section above:
26f8d8dc5634f0371fad0801056e9a3450c78bffon branchclaude/nim-all-models-support-fecb0b.fix/atheris-interpreter-lockremains exact fix(security): pin provider egress and repair the Atheris lock #96 head3703d0da9823b8258a0be94f1801aa5d61bfad9f; protected repositorymainremains6841b71935e0b7cb98fb52bcb4709cc5100c8d87..github/mainremains6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba.67d834f510fe044dd9d53cd4f4b9783353e303bd, with 11 terminal-success exact-head workflows, zero unresolved threads, and no qualifying approval.ac5665148bb113f92e97d2fc49a729bca2f050b5, with nine terminal-success exact-head workflows, zero unresolved threads, and no formal review.601b254f3a8ea4cc593e7089d6baeadd9d8d3ee4, with nine terminal-success exact-head workflows, zero unresolved threads, and no formal review. It is the bounded trusted-download User-Agent repair and does not replace #939's separate cross-repository review-status semantics.Every dependency remains planning evidence only. No predecessor, status-only, synthetic-merge, queued, or unintegrated central evidence transfers to this contributor head.
Live central review-authority correction (2026-08-12 06:29 UTC)
Read-only dependency refetch: protected central
mainremains6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba. Central #937 remains open at67d834f510fe044dd9d53cd4f4b9783353e303bdwith eleven terminal-success workflows, zero unresolved threads, and one OpenCodeAPPROVEDformal model review. That model review is not a qualifying independent human approval and does not make the central branch protected authority. Central #939 remains open atac5665148bb113f92e97d2fc49a729bca2f050b5with nine terminal-success workflows, zero threads, and no formal review; #943 remains open at601b254f3a8ea4cc593e7089d6baeadd9d8d3ee4with the same nine-success/zero-thread/no-review classification.This repository loop did not mutate the central repository. This PR's unchanged source head and local workflow evidence remain
active_pr; protected central integration and qualifying independent non-author approval remain absent. Keep the PR Draft.