Skip to content

fix: isolate NIM-benchmark evidence-currency fixture and fix spend-analytics usage_source classification - #1070

Merged
seonghobae merged 8 commits into
mainfrom
fix/nim-benchmark-hardcoded-expiry-date
Sep 17, 2026
Merged

fix: isolate NIM-benchmark evidence-currency fixture and fix spend-analytics usage_source classification#1070
seonghobae merged 8 commits into
mainfrom
fix/nim-benchmark-hardcoded-expiry-date

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Identity correction, 2026-09-06

This PR's head became byte-identical to #1071 after both branches independently ported each other's fixes to pass against current main (three-dot diff against main is the same 385-line patch on both, confirmed independently via git diff/cmp/merge-tree, corroborating a peer session's adjudication comment below). This is the designated survivor per that adjudication (owner call, contextual-orchestrator/issues/1080); #1071 is being closed as a duplicate pointing here. The delta below is the exact current 6-file diff, not an approximation.

Current repair state

Exact head bcecfa16ea7910808605d8a16bba49c57ac28758. Effective base-to-head delta, verified against origin/main@2e414d15:

  • CHANGELOG.md, README.md — documentation for both fixes below;
  • tests/test_nim_benchmark.py — this PR's own fix (see "RED → causal GREEN");
  • tests/test_nim_benchmark_release_acceptance.py — a third instance of the same hardcoded-expiry-date bug class, found and fixed while validating this PR (commit 0eaca9f1; see PR comments);
  • contextual_orchestrator/orchestrator.py, tests/test_spend_analytics.py — the usage_source classification fix cherry-picked from #1071 (commits e4cda9cc/bcecfa16), a legitimately separate, pre-existing base-branch bug ported here so this PR's own tests pass against current main.

This PR remains Draft because fresh exact-head hosted/security evidence is non-terminal.

Production ACTUAL_COST_EVIDENCE is intentionally unchanged. Live benchmark execution must continue to fail closed after that human-reviewed window expires until NVIDIA hosted-endpoint terms are actually re-reviewed. Tests may not make that production fact relative to wall-clock time — this PR's own added regression test hit exactly that trap and was fixed (1d5e77f9) to monkeypatch a fixed-past date instead of relying on real-world drift relative to whatever production evidence currently says.

RED → causal GREEN (this PR's own fix)

The predecessor fix used an autouse fixture over all tests in tests/test_nim_benchmark.py, which silently replaced the expired production evidence for unrelated and future tests. The current descendant narrows the seam instead of changing production behavior:

  • current_actual_cost_evidence is now an explicit opt-in pytest fixture, not autouse;
  • only the five tests whose intended assertion is downstream of cost-evidence currency request it by name: missing credential, offline live path, default transport builder, evaluation-contract failure, and live CLI missing-secret;
  • an unmarked regression registers a valid credential but does not request the fixture, then requires the literal production evidence to fail closed as expired before another live-path assertion can run;
  • production source is untouched for this half of the diff.

Promotion gate

Keep Draft until the unchanged exact head has terminal focused/full/fuzz/docstring/security evidence and current review/thread state is clean. Do not extend the production review date, self-approve, bypass, weaken gates, force-update, destructively rebase, or add a no-op retrigger.

Generated by Claude Code

@claude
fix(tests): stop test_nim_benchmark.py from depending on wall-clock date
9c34928 
Five tests in tests/test_nim_benchmark.py exercise unrelated run_mode="live"
behavior (missing credential, transport wiring, a malformed-catalog contract
failure, CLI exit codes) that incidentally passes through
_require_current_actual_cost_evidence(). That check fails closed once
nim_benchmark.ACTUAL_COST_EVIDENCE["valid_until_date"] — a human-reviewed
date for NVIDIA's published hosted-endpoint terms — is in the past. Today
(2026-09-05) is now one day past the literal "2026-09-04" recorded there,
so all five started failing for a reason unrelated to what each actually
asserts: a classic ticking-time-bomb hard-coded date, not a code regression.

The production literal is deliberate and stays as-is: it is a real record of
when someone last verified NVIDIA's terms, and live runs are meant to fail
closed until that review happens again. tests/test_nim_benchmark_release_acceptance.py
already covers that review-cadence invariant directly via explicit `today`
injection into _require_current_actual_cost_evidence, and is unaffected.

Added an autouse fixture in test_nim_benchmark.py that monkeypatches
ACTUAL_COST_EVIDENCE's reviewed_at_date/valid_until_date to bracket real
"now" for the duration of this file's tests only, so they stay green
regardless of wall-clock date while still exercising the real evidence-gated
code path.

Verified: `python -m pytest tests/test_nim_benchmark.py -v` and
`python tests/test_nim_benchmark.py` both 98 passed (previously 5 failed);
tests/test_nim_benchmark_release_acceptance.py unaffected (28 passed);
interrogate scoped to contextual_orchestrator/ stays 100%; git diff --check
clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
@coderabbitai

coderabbitai Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 4 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 05231659-396d-4540-a3b8-720604407a5a

📥 Commits

Reviewing files that changed from the base of the PR and between fb782d3 and 232e3d4.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • README.md
  • contextual_orchestrator/orchestrator.py
  • tests/test_nim_benchmark.py
  • tests/test_nim_benchmark_release_acceptance.py
  • tests/test_spend_analytics.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as draft September 5, 2026 03:15
@seonghobae seonghobae changed the title fix(tests): stop test_nim_benchmark.py from depending on wall-clock date fix(tests): isolate live-path tests from hosted-cost review expiry Sep 5, 2026
@claude
fix(tests): narrow evidence-currency fixture to opt-in, not file-wide
b4cc6c6 
The prior fix used an autouse fixture across all 98 tests in
tests/test_nim_benchmark.py, silently giving every test synthetic
current cost evidence and deriving dates from date.today() — too
broad for a security/commercial fail-closed gate, since unrelated
tests (present and future) would stop observing the literal
production evidence dict.

Make the fixture opt-in (current_actual_cost_evidence, no autouse)
and request it explicitly only from the five tests that need to get
past the evidence gate to reach their own live-path assertion.
Production ACTUAL_COST_EVIDENCE is unchanged. Add a regression test
proving a test that does not request the fixture still observes the
literal production evidence and fails closed at the gate (verified
RED against the reverted autouse design, GREEN against this fix).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
@seonghobae Claude

seonghobae commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Pushed b4cc6c6a implementing the requested RED→GREEN fix:

  • Removed autouse=True from the evidence-currency fixture; renamed to current_actual_cost_evidence and made it opt-in.
  • Added it as an explicit parameter to only the five named tests (test_live_run_fails_closed_without_credential, test_live_run_end_to_end_offline, test_live_run_uses_default_transport_builder_when_none_given, test_evaluation_contract_failure_publishes_no_artifacts, test_cli_live_fails_closed_without_secret).
  • Production ACTUAL_COST_EVIDENCE literal (valid_until_date=2026-09-04) is unchanged.
  • Added test_live_run_without_evidence_fixture_still_fails_closed_on_expired_evidence: registers a credential, calls run_benchmark("live", ...) with no fixture requested, asserts BenchmarkContractError matching "expired". Verified this test is RED (fails, for a different reason — it gets past the gate and hits a downstream transport/auth error) when the fixture is reverted to file-wide autouse=True, and GREEN against this fix — confirming it actually exercises the boundary, not vacuously passing.

Validated locally: the five named tests + the new regression test all pass (pytest tests/test_nim_benchmark.py -k "..."), the full tests/test_nim_benchmark.py + tests/test_nim_benchmark_release_acceptance.py pair passes (127 passed), and a full tests -q run (excluding the pre-existing, unrelated tests/test_psychometric_routing.py::numpy import gap on this Python 3.11 sandbox — confirmed pre-existing on unmodified main too) was still completing at push time; will follow up here if it surfaces anything.

Left as Draft per your instructions — no self-approval, no ready-for-review flip. Over to you for the full-gate re-run and Ready decision.

Generated by Claude Code

This was referenced Sep 5, 2026
Draft
Merged
@claude
fix(tests): close third hardcoded-expiry-date gap in release-acceptan…
0eaca9f 
…ce tests

test_live_run_rejects_unreviewed_pricing_before_egress and
test_live_run_rejects_incomplete_or_expired_pricing_before_egress now request
the current_actual_cost_evidence fixture so they exercise
validate_live_pricing_scenario's own fail-closed branches (lines 1708/1718)
instead of incidentally matching _require_current_actual_cost_evidence's
"expired" error once ACTUAL_COST_EVIDENCE's literal valid_until_date lapsed.
Restores 100% branch coverage on nim_benchmark.py's pricing-scenario gate.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
@seonghobae seonghobae mentioned this pull request Sep 5, 2026
Closed
@seonghobae Claude

seonghobae commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 0eaca9f1: found and fixed a third instance of the same hardcoded-expiry-date bug this PR is about, this time in tests/test_nim_benchmark_release_acceptance.py.

What broke: that file's two pricing-scenario contract tests (test_live_run_rejects_unreviewed_pricing_before_egress, test_live_run_rejects_incomplete_or_expired_pricing_before_egress) are designed to exercise validate_live_pricing_scenario's own fail-closed branches (nim_benchmark.py:1708/:1718). Once the literal ACTUAL_COST_EVIDENCE["valid_until_date"] lapsed (today, 2026-09-05), run_benchmark started raising _require_current_actual_cost_evidence()'s "expired" error first, before ever reaching the pricing-scenario check. Both tests kept reporting green — their pytest.raises(match=...) substrings ("reviewed" / "expired") happened to also match that earlier exception's message — but the coverage gate caught what the green run hid: 0% coverage on the two lines it now silently skipped. Fixed the same way as this PR's existing test_nim_benchmark.py fix: added the identically-named, identically-scoped opt-in current_actual_cost_evidence fixture to this file too, requested from both tests.

What's still red on this required check, and why it's not this PR's: python -m coverage report --include=contextual_orchestrator/nim_benchmark.py --fail-under=100 now reports 99%, missing 434, 645, 671->682 — unrelated statement/branch gaps inside _BudgetedModelClient/_PolicyCellModelClient's proxy_send methods. I confirmed via a throwaway worktree that this exact 99%/434, 645, 671->682 result reproduces identically on unmodified origin/main, with zero relation to this PR's diff (this PR touches only tests/test_nim_benchmark.py, tests/test_nim_benchmark_release_acceptance.py, and CHANGELOG.md — never nim_benchmark.py itself). Filed #1075 to track and close that pre-existing gap with real tests rather than widening this PR's scope to cover it.

With 0eaca9f1, local reproduction of the full CI coverage step (coverage run --branch --source=contextual_orchestrator.nim_benchmark -m pytest tests/test_nim_benchmark.py tests/test_nim_benchmark_release_acceptance.py tests/test_nim_benchmark_workflow_contract.py -q + coverage report ... --fail-under=100 + interrogate -f 100) is back to parity with origin/main's pre-existing 99% (not a regression introduced by this PR), and all 135 tests in that scoped run pass.

Generated by Claude Code

claude and others added 3 commits September 5, 2026 12:41
@claude
fix(tests): stop relying on real-world evidence expiry timing
1d5e77f 
test_live_run_without_evidence_fixture_still_fails_closed_on_expired_evidence
asserted that a test not requesting the current_actual_cost_evidence fixture
observes real-world ACTUAL_COST_EVIDENCE as expired -- true only because
today happened to be past the literal valid_until_date at authoring time.
Once #1073 refreshed that production evidence (reviewed_at_date=2026-09-05,
valid_until_date=2026-10-05), this PR's own required "Tests and package
quality" check failed for real: the evidence gate no longer raised, so the
test fell through to a genuine network probe against NVIDIA's live API and
got HTTP 403 -- the exact same class of ticking-time-bomb bug this PR is
otherwise fixing, just introduced by this PR's own new regression test.

Fix: the test now deliberately monkeypatches ACTUAL_COST_EVIDENCE to a fixed
past window (2020) itself, proving the same opt-in-fixture-vs-file-wide
mechanism without depending on real wall-clock time relative to whatever the
production valid_until_date currently is. Verified locally against both the
unmodified branch and a merge of this branch with current origin/main
(post-#1073 evidence).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
@claude
fix(analytics): usage_source must reflect unmeasured prompt tokens
e4cda9c 
spend_analytics()["by_model"][].usage_source was classified purely from
each model's output-token source (reported/tokenizer/mixed/unavailable
across that model's steps this run), while completely ignoring whether
prompt tokens were ever available for that model. A model with exactly
one step, exact-tokenizer output, and zero reported prompt tokens was
labeled pure "tokenizer" — dishonestly implying full count-based
knowledge of that model's usage when the prompt side was entirely
unmeasured.

Add per-model prompt-token tracking (`bucket["prompt_reported_steps"]`)
alongside the existing per-model output-source counters, and require
full per-model prompt availability before classifying a row as
"reported" or "tokenizer". A model whose output is fully known but
whose prompt tokens are partially or entirely unmeasured now correctly
falls through to "mixed" — the same honest-composite label already used
for genuinely mixed output sources. The four-way usage_source vocabulary
(reported/tokenizer/mixed/unavailable) is unchanged, matching the
documented contract in README.md.

The existing report-wide `prompt_available` flag (used for
`totals.prompt_tokens`/`measurement_status`) is untouched: it already
forces "unavailable" whenever any step in the run lacks a valid reported
prompt count, so this fix only tightens per-model row classification,
not the report-level status.

Validated: tests/test_spend_analytics.py now passes (previously failed
with `'tokenizer' == 'mixed'`); full suite
`PYTHONPATH=. python -m pytest tests -q --ignore=tests/test_psychometric_routing.py`
now shows 3383 passed, 5 failed, 2 skipped — the 5 failures are
pre-existing, unrelated `test_nim_benchmark.py` date-evidence-expiry
failures (confirmed identical via `git stash` against unmodified
origin/main), not caused by or related to this change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
@seonghobae @claude
test(analytics): pin per-model prompt evidence scope
bcecfa1
@seonghobae Claude

seonghobae commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Tests and package quality failed again on this PR's prior head (0eaca9f1) — test_live_run_without_evidence_fixture_still_fails_closed_on_expired_evidence (added by this PR's own 0eaca9f1 commit) got a real BenchmarkAuthError: provider rejected the benchmark credential during probes (HTTP 403) instead of the expected BenchmarkContractError "expired".

Root cause, self-inflicted: that regression test asserted the evidence gate fires "expired" by relying on real wall-clock time being past whatever ACTUAL_COST_EVIDENCE["valid_until_date"] currently is in production — true when I wrote it (2026-09-05, literal was 2026-09-04), but PR checks run against the merge of this branch with current main, and #1073 merged to main in the meantime refreshing that evidence to valid_until_date=2026-10-05. So the merged target's evidence was no longer expired, the gate passed cleanly, and the test fell through to a real network probe against NVIDIA's live API — the exact ticking-time-bomb class this PR exists to fix, just newly introduced by this PR's own added test. Fixed (1d5e77f9): the test now deliberately monkeypatches ACTUAL_COST_EVIDENCE to a fixed 2020 window itself, so it no longer depends on real-world timing relative to whatever production evidence currently says.

Second failure in the same merge target, not caused by this PR: verifying against a real merge with current origin/main (not just this branch alone) also surfaces tests/test_spend_analytics.py::test_exact_output_without_prompt_usage_is_explicitly_unavailable ('tokenizer' == 'mixed') — confirmed identical on unmodified origin/main alone via a throwaway worktree, so it's pre-existing base-branch debt, not this PR's diff. A fix already exists and is actively being driven: #1071 (commits e645cffb/2bfb603a). Ported both into this branch (e4cda9cc/bcecfa16) rather than waiting on #1071 to merge first, since every currently-open PR merging against main hits this same failure until then — it no-ops once #1071 lands.

Verified locally: tests/test_nim_benchmark.py + tests/test_nim_benchmark_release_acceptance.py + tests/test_spend_analytics.py all pass (132/132) both on this branch alone and merged fresh with current origin/main. Benchmark coverage gate is back to the known pre-existing 99% (434, 645, 671->682, tracked as #1075, unrelated to this PR).

Generated by Claude Code

@seonghobae seonghobae mentioned this pull request Sep 5, 2026
Open
@seonghobae

seonghobae commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Adjudication evidence (host 1 session, 2026-09-06 KST; full report with commands in #1080). Nothing here closes, flips, or retargets anything — the decision is the opener's.

This head is byte-identical to #1071. git diff origin/main...refs/pr/1070 and ...refs/pr/1071 are the same 385-line patch (6 files, +193/−10; reverse-apply exit 0 both ways; merge-tree clean). The two bodies describe two different changes — this one the NIM-benchmark date fixture, #1071 the analytics usage_source — and #1071's body says the expiry defect is "tracked in #1070" as a separate item, but both heads now carry both changes. Two refuters failed to refute the duplicate verdict, and I re-ran the cmp myself.

Merging both would land the same diff twice; merging one makes the other empty. The owner (session_01KPm… per the lane-claim marker) picks the survivor; whichever survives needs its body (identity block says 4 files; head has 6) and title corrected. Draft hold and the "no ready-for-review flip" instruction in this thread are respected — nothing was flipped or closed.

@seonghobae seonghobae mentioned this pull request Sep 5, 2026
Closed
@seonghobae seonghobae changed the title fix(tests): isolate live-path tests from hosted-cost review expiry fix: isolate NIM-benchmark evidence-currency fixture and fix spend-analytics usage_source classification Sep 5, 2026
@seonghobae seonghobae added bug Something isn't working priority: medium Normal-priority or P2 work status: draft type: bug Defect or incorrect behavior labels Sep 7, 2026 — with ChatGPT Codex Connector
@opencode-agent

opencode-agent Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: bcecfa16ea7910808605d8a16bba49c57ac28758

@seonghobae @cursoragent
merge(main): restack NIM evidence-currency fixture onto current main
1d16a25 
Preserve opt-in current_actual_cost_evidence (not file-wide autouse) and
prompt-aware usage_source classification while absorbing #1074 / main.

Co-authored-by: Cursor <cursoragent@cursor.com>
@seonghobae
seonghobae marked this pull request as ready for review September 17, 2026 20:57
seonghobae pushed a commit that referenced this pull request Sep 17, 2026
@claude @seonghobae
docs: append cross-session agent know-how on nim_benchmark date gates…
fd631ed 
… and review sidecar gap

Append-only additions to AGENTS.md (two new sections), a one-line pointer in
CLAUDE.md, and a CHANGELOG.md entry, recording lessons independently
re-verified against this exact checkout and live GitHub state on 2026-09-05:
the nim_benchmark.py evidence-gate recurring bug class (hardcoded
valid_until_date literals causing silent branch-coverage loss once lapsed,
diagnosed/repaired three times this cycle across PR #1070, PR #1071, and
issue #1075), and the still-open central review sidecar/egress gap tracked
in issue #1041 comment 5550412102 and ContextualWisdomLab/.github issue
Documentation only: no code, test, or behavior change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
@seonghobae @cursoragent
merge(main): restack NIM evidence-currency fixture onto post-#1091 main
232e3d4 
Resolve test_nim_benchmark.py by keeping the opt-in current_actual_cost_evidence
fixture and fail-closed regression from #1070 while threading #1091 declared
workflow-budget kwargs through live/dry run_benchmark call sites.

Co-authored-by: Cursor <cursoragent@cursor.com>
@seonghobae
seonghobae merged commit dbd44fa into main Sep 17, 2026
17 of 21 checks passed
@seonghobae
seonghobae deleted the fix/nim-benchmark-hardcoded-expiry-date branch September 17, 2026 21:12
seonghobae added a commit that referenced this pull request Sep 17, 2026
@seonghobae @cursoragent
merge: restack declared held-out bootstrap onto origin/main
6e0e38d 
Resolve conflicts with #1091 workflow-budget declarations and #1070
evidence-currency fixtures while keeping required paired-bootstrap and
held-out coverage declarations fail-closed.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working priority: medium Normal-priority or P2 work status: draft type: bug Defect or incorrect behavior

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@seonghobae @claude