Skip to content

fix(benchmark): require declared paired-bootstrap coverage - #1090

Closed
seonghobae wants to merge 6 commits into
mainfrom
codex/declared-paired-bootstrap-20260907
Closed

seonghobae wants to merge 6 commits into
mainfrom
codex/declared-paired-bootstrap-20260907

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Stack and current evidence

Draft successor of #1074 at 2157d702c4609cc51829d7423911cadf26cf83d0.
Normal merge 708fbb19733f1829ca3baddab68e4c0fdbcbe7a5 preserves the previous #1090 head 5e952933d4dd97057ba76b70ea17983314a4babe and current parent. Documentation-only follow-ups are 2f240363 and 2bf856cd; the latter adds the explicit ownership RED receipt. No production-source fix is added by the restack. #1074 is not closed.

The sole Gap conflict retained both complete sections. The parent's existing citation-only IRT-Router decision remains intact; the removed PDF is not resurrected. Bootstrap source, tests and workflow retain their previous #1090 bytes.

Problem and retained change

Paired NIM comparisons used a hidden 2,000-resample 95% percentile interval and a baked-in policy subset. Required resample_count, confidence_level, seed and comparison_pairs declarations replace those defaults. Invalid or degenerate declarations fail closed; unobserved/disjoint pairs are not imputed. Report schema 4.0.0 records declarations. ADR 0042 stays Proposed; Efron (1979) and Efron and Tibshirani (1993) remain cited without bundled PDFs.

Verification — not ready

At merge 708fbb19, benchmark, release-acceptance, workflow, paper and effort suites with warnings-as-errors: 235 passed, 3 failed in 9.66s, exit 1. Two HTTPError cleanup failures independently reproduce on parent 2157d702 (2 failed, 1.36s). The profile test receiving a delayed cleanup warning passes alone (1 passed, 0.68s). Later doc commits are not relabeled test executions.

At 2f240363, a separate local probe retains a BytesIO-backed HTTPError503 and calls run_policy_cell. Expected provider_http_error:503 and task_score=None remain correct, but response_error.closed fails before fallback cleanup, exit 1. This proves a consumed-response ownership defect. The local /tmp/co-nim-response-ownership-probe.py is not committed. Production repair belongs in a separately reviewed successor after ownership audit.

Historical old-head checks remain historical: 175 tests in 10.01s; 1328 NIM statements and 504 branches with zero misses; public docstrings100%. Independent old-head rerun passed175 in8.73s without warnings-as-errors. These do not establish current-head strict acceptance.

Visual inspection and remaining gates

At documentation head 2f240363, actual browser screenshots at1265x712, English/default, directly inspected the new Gap section, both preserved sections and runbook receipt. No text overlap or horizontal clipping was observed in those sections; this is not full product UI acceptance. Further receipt and GitHub body inspection are recorded separately.

Fresh hosted checks and independent review are required. No self-approval, protected merge, release, deployment, customer accuracy gain or decision-p95 gain is claimed. Existing workflow/token-budget follow-ups remain separate owners.

Remove the hidden 2,000-resample 95% interval and the baked-in
conduct/route/cheapest/hindsight comparison subset. Report schema
4.0.0 records operator declarations in provenance.
@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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 and others added 2 commits September 14, 2026 13:44
…-bootstrap-20260907

# Conflicts:
#	.github/workflows/security.yml
#	CHANGELOG.md
#	contextual_orchestrator/orchestrator.py
#	docs/architecture.md
#	docs/product-technical-gap-baseline.md
#	tests/test_distributed_cache_truth_and_isolation.py
#	tests/test_provider_embedding_batch_backend.py
#	tests/test_repository_security_metadata.py
- stream_route referenced the removed ranked_pool variable for its
  selection-design receipt after adopting main's candidate-selection
  structure during the main restack; use the equivalent candidates list.
- Update the hardcoded selected_deployment_id hash in
  test_selection_receipt_does_not_mix_catalog_revisions: main added
  model_timeout_seconds/model_timeout_revision to ModelAgent.to_config(),
  which legitimately changes the hash input for every agent.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@seonghobae

Copy link
Copy Markdown
Contributor Author

Restack onto main

New SHA: f93e046f (merged origin/main @ 767e67fb into codex/declared-paired-bootstrap-20260907 @ 2bf856cd, non-force push, plus one small follow-up commit fixing two post-merge regressions — see below).

Conflicts resolved (this branch and main had both independently evolved large, overlapping parts of orchestrator.py, so this was the most involved of the three restacks):

  • .github/workflows/security.yml, CHANGELOG.md, docs/product-technical-gap-baseline.md: additive/dated entries — kept both (ours first) or took main's addition where the PR side was empty.

  • docs/architecture.md: both sides rewrote the same reasoning_effort_profile paragraph. Took main's updated general description (psychometric θ̂/RMSE framing, production_default_change_allowed) and re-appended this PR's own "request-revision contract" paragraph describing its actual added behavior (see below) — dropping it entirely would have deleted documentation for real, shipped code.

  • contextual_orchestrator/orchestrator.py — the substantial one. This PR adds a per-request execution scope: a _REQUEST_EXECUTION_SNAPSHOT ContextVar, _request_execution_scope()/@_request_execution_scoped, a policy property override, and _effort_snapshot()/_role_effort_profile() that pin one catalog+policy revision for the life of a request (so a concurrent operator edit can't split one request across two effort-catalog revisions). Main independently renamed/simplified the pre-existing _with_effort_snapshot helper and added unrelated features (persist_structured_record DRY refactor, route echo-preservation, _last_assistant_message thread-local, _cache_key's resolved_mode parameter, EffortProfileError cache-key handling, ModelAgent.to_config() gaining model_timeout_seconds/model_timeout_revision from fix(gateway): remove implicit model request timeout #1053).

    Resolution: kept this PR's richer _with_execution_snapshot (the request-scoped version) as the single implementation, renamed the handful of already-auto-merged _with_effort_snapshot call sites to match, and adopted main's structural additions (persist_structured_record, route preservation, _last_assistant_message, _cache_key merging both the new resolved_mode key and this PR's policy_snapshot/reasoning_effort_snapshot_hash keys) everywhere they didn't conflict with this PR's actual behavior. One deliberate exception: run()'s workflow-record persistence keeps reusing result["policy_snapshot"]/result["reasoning_effort_snapshot"] captured inside complete()'s own request-scope (with copy.deepcopy) instead of recomputing via _with_execution_snapshot after that scope has already exited — recomputing there would reopen exactly the operator-mutation race this PR's request-scope feature exists to close. Also merged _invoke's endpoint-race return path: kept main's correct 4-tuple return contract (matching this PR's new _last_assistant_message thread-local side channel — the 4-tuple is what every caller unpacks) while preserving this PR's own selection_design_sink telemetry call.

  • tests/test_distributed_cache_truth_and_isolation.py, tests/test_provider_embedding_batch_backend.py, tests/test_repository_security_metadata.py: merged imports/tests additively; for the security-metadata test, kept main's version since it matches the merged security.yml (no branches: restriction on pull_request) rather than this PR's version (which asserted no types: line at all).

Two regressions found and fixed after the merge (separate follow-up commit f93e046f, not squashed into the merge commit):

  1. stream_route still referenced a ranked_pool variable for its _selection_design_receipt call that no longer existed after adopting main's candidate-selection structure — a NameError at runtime. Fixed to use the equivalent candidates list.
  2. tests/test_psychometric_routing.py::test_selection_receipt_does_not_mix_catalog_revisions hardcodes a SHA-256 selected_deployment_id. Main's ModelAgent.to_config() gained model_timeout_seconds/model_timeout_revision fields (from fix(gateway): remove implicit model request timeout #1053), which legitimately changes that hash input for every agent. Recomputed and updated the hardcoded value; the rest of the test's assertions are relative and were unaffected.

Tests

  • PR's own touched tests (14 files: test_distributed_cache_truth_and_isolation.py, test_endpoint_race.py, test_nim_benchmark*.py, test_openrouter_uptime.py, test_paper_contracts.py, test_provider_embedding_batch_backend.py, test_provider_reliability.py, test_psychometric_*.py, test_reasoning_effort_profile.py, test_request_effort_snapshot.py, test_request_policy_snapshot.py, test_true_streaming.py): 524 passed, 0 failed (after the two fixes above).
  • tests/test_api_contract.py tests/test_self_check.py: 9 passed.
  • Full suite (python -m pytest tests -q): 3922 passed, 1 skipped, 5 failed — all 5 are the known local-only failures: the openai SDK 2.54.0-pin mismatch (test_tool_execution_fallback.py x3, test_passthrough_provider_failover.py x1) and the mcp.Client privacy test.
  • python -m interrogate -v contextual_orchestrator/: 100.0% (PASSED).

Readiness review

The delta does what the title says: paired NIM benchmark comparisons now require explicit resample_count/confidence_level/seed/comparison_pairs declarations instead of a hidden 2,000-resample/95%-interval default and a baked-in policy subset, failing closed on invalid/degenerate input rather than imputing unobserved pairs. Report schema bumps to 4.0.0 to record the declarations. This is a real, currently-shipped feature — the request-revision-contract machinery it depends on for effort/policy consistency (see architecture.md note above) is genuinely new code, not documentation drift.

The PR's own description is explicit that it is not ready: at its last evidence head, the warnings-as-errors suite had 3 failures reproducing an HTTPError cleanup/resource-ownership defect ("consumed-response ownership defect... production repair belongs in a separately reviewed successor after ownership audit"), and it states plainly that "fresh hosted checks and independent review are required" with no protected-merge, release, or accuracy/latency claim. I did not re-run the strict (-W error) suite at the new head — only the PR-touched tests, the full default-warning suite, and the two required contract/self-check gates, all green (modulo the known local-only failures).

Verdict: NEEDS_WORK, per the PR's own admission plus the two regressions this restack surfaced and fixed:

  1. The HTTPError/resource-ownership defect the description calls out as requiring a separately-reviewed successor is unresolved and out of this PR's stated scope.
  2. Strict (-W error) full-suite status at the new restack head is unestablished (not re-run here).
  3. Hosted CI checks and independent review at the new head are outstanding.
  4. (Now fixed by this restack, flagging for visibility) the ranked_pool NameError and stale hash — these were merge-introduced, not present on either original branch tip in isolation, and are now corrected.

No defects found in the PR's own design beyond what it already discloses. The request-scope/effort-pinning mechanism is a genuine, carefully-reasoned addition (comments explain the ThreadingHTTPServer thread-local rationale and the race it closes) and merged cleanly in intent, if not textually, with main's independent refactor of the same area.

🤖 Addressed by Claude Code

Base automatically changed from codex/paired-policy-outcomes-20260905 to main September 17, 2026 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: medium Normal-priority or P2 work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant