Skip to content

fix(nim): close consumed benchmark error responses - #1155

Merged
seonghobae merged 8 commits into
mainfrom
codex/nim-response-ownership-20260913
Sep 17, 2026
Merged

seonghobae merged 8 commits into
mainfrom
codex/nim-response-ownership-20260913

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Scope and dependency

Draft stack on #1090. This is not independently merge-ready: #1140 separately owns inherited ModelClient.chat final-classification closure. Its implementation is neither copied nor amended here; no predecessor is closed.

Repair

RED e2c06a3 adds two retained HTTPError closure cases, both failing. Initial source dda57de classifies consumed errors before closing at run_policy_cell; follow-up 7ff2c8e generalizes protection. Cleanup Exception cannot replace provider_http_error:503 or the null raw score; BaseException propagates. Propagated contract/budget/auth errors retain caller ownership. The direct proxy test closes its caller-owned response.

Evidence

Hosted follow-up for current head dfd9d5e4b69b7b517460245ce4858677f5602ccf:
Security run 34705849987
completed all three jobs successfully. Quality job 103585663660 checked out
merge 492b480 of this head into 2bf856cdaa3756ba6e729479b0cbdc6109d065d9:
3,637 passed, 2 skipped in 761.18s; separate benchmark coverage invocation
178 passed in 16.05s. Built core wheel SHA-256
3502786a41c6faa3d48a7bd8fc508dbe45e2f2be7c7bc776c03a7c61f03906ca.
The hosted full suite does not enable warnings-as-errors, so this does not
resolve the separately recorded strict sibling failure. No GitHub review or
protected merge is established; PR remains Draft with the #1140 dependency.

Final exact head dfd9d5e4b69b7b517460245ce4858677f5602ccf: focused strict benchmark module 138 passed in 9.07s, terminal exit 0. No source changed during this run.

Follow-up review identified general cleanup exception masking. RED f67db9c2: RuntimeError case failed (1 failed, 2 passed, 135 deselected, 0.64s). Source 7ff2c8e7 catches Exception, not BaseException, at the cleanup boundary; 138 passed in 5.30s. Intermediate 46ccd25e failed collection because of indentation, corrected in 7ff2c8e7. No #1140 implementation is copied or changed.

  • Source strict benchmark module: 137 passed, 9.24s, exit 0.
  • Five-module strict: 239 passed, 1 failed, 14.56s, exit 1.
  • Sibling-only strict: 102 passed, 1 inherited delayed cleanup failure, 1.96s, exit 1. The remaining release-acceptance HTTP500 failure reproduces independently and belongs to fix(transport): preserve errors while closing stream resources #1140.
  • Initial independent static review of dda57de found no actionable issue; subsequent coordinator review found the general-exception gap repaired above. Neither is GitHub approval of the final head.
  • Cleanup injection covers OSError and RuntimeError after underlying close; inability to close is unproven. BaseException is deliberately not caught.

AGENTS, CLAUDE, owner doctoring and Gap retain historical exact-head RED/GREEN boundaries. Hosted acceptance above supplements those local records. No strict full-suite acceptance, protected merge, release, deployment, observed accuracy gain or decision-latency gain is claimed.

Visual receipt: actual browser PR body and production source diff were captured and directly inspected at final head dfd9d5e, 1265×712, English, desktop. The Exception cleanup handler and preserved outcome fields were readable with no observed text overlap. This is limited PR/document rendering inspection, not full product UI, mobile or locale acceptance.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 24 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: 0b4075a7-96db-439e-8c4b-0072a01b7296

📥 Commits

Reviewing files that changed from the base of the PR and between e973631 and b4cc678.

📒 Files selected for processing (6)
  • AGENTS.md
  • CLAUDE.md
  • contextual_orchestrator/nim_benchmark.py
  • docs/doctoring/nim-benchmark-evidence-grade.md
  • docs/product-technical-gap-baseline.md
  • tests/test_nim_benchmark.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

Copy link
Copy Markdown
Contributor Author

Restacked on the updated base branch (26e6c7ac; merged codex/declared-paired-bootstrap-20260907 after #1090 took current main — clean, no conflicts despite the base moving 258 commits). Local: tests/test_nim_benchmark.py 143 passed, api-contract/self-check 9 passed, interrogate 100%. Still draft.

🤖 Addressed by Claude Code

@seonghobae
seonghobae force-pushed the codex/nim-response-ownership-20260913 branch from 26e6c7a to 51366a6 Compare September 17, 2026 22:57
@seonghobae
seonghobae changed the base branch from codex/declared-paired-bootstrap-20260907 to main September 17, 2026 22:57
@seonghobae
seonghobae marked this pull request as ready for review September 17, 2026 22:57
@seonghobae
seonghobae merged commit 72eb52d into main Sep 17, 2026
17 of 21 checks passed
@seonghobae
seonghobae deleted the codex/nim-response-ownership-20260913 branch September 17, 2026 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant