Skip to content

fix(tests): close #1831's stale served_model test, missing docstring, and coverage gap - #1835

Merged
seonghobae merged 1 commit into
mainfrom
fix/served-model-annotation-safe-oracle
Sep 4, 2026
Merged

fix(tests): close #1831's stale served_model test, missing docstring, and coverage gap#1835
seonghobae merged 1 commit into
mainfrom
fix/served-model-annotation-safe-oracle

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Summary

#1831 ("ground verdicts and classify gateway errors") replaced _extract_served_model's scrub-and-keep sanitization with a delegate to the new, stricter _safe_model_identifier (reject outright on any character outside a conservative allowlist), but left three gaps that broke the required test/coverage/docstring suite for every .github PR:

  1. test_served_model_is_annotation_safe still asserted the old scrub-based behavior (a sanitized non-None value) — now fails since the function returns None for the same malicious input.
  2. The new nested render closure inside _bounded_allowed_locations_json had no docstring (interrogate 99.9%).
  3. The new _extract_http_error_served_model function had zero test coverage (99% branch coverage — lines 1331-1332, 1340, 1343, 1346 uncovered).

Third and independent instance of the same "PR changes behavior, sibling test/coverage/docstring not updated" class found this tick, following #1829 (fixed #1826/#1823's gaps) and #1832 (fixed #1828's gap) — all from the same rapid merge burst as the org-wide Actions queue drained.

Is the behavior change itself a regression?

Verified it is not. Real provider model ids seen in this session's own gateway logs (deepseek-ai/deepseek-v4-pro-0813, meta/llama-3.2-11b-vision-instruct, google/gemma-3-12b-it, orchestrator/free) all still pass the new regex unaffected — only genuinely malformed/malicious input is now rejected outright instead of sanitized-and-kept. served_model is embedded directly into ::warning::/::notice:: GitHub Actions workflow commands (scripts/ci/noema_review_gate.py lines ~1600/1605/1621), so reject-and-report-unknown is strictly safer against annotation injection than the old scrub approach, never weaker.

Fix

  1. Updated test_served_model_is_annotation_safe to assert the safe outcome (None) instead of a specific sanitized string, with a docstring explaining why None is the correct safe outcome. Added a companion test confirming a real provider id still passes through unchanged.
  2. Added the missing docstring to render.
  3. Added 7 tests in test_noema_model_output_edge_coverage.py covering _extract_http_error_served_model's success path and all six fail-closed branches (unreadable body, oversized body, invalid JSON, non-dict payload, non-dict error, non-dict detail).

Verification

  • Full suite: 2780 passed, 1 skipped, 21 subtests passed (reproduced the original 1-failure state on fresh main before this fix).
  • coverage report --fail-under=100: 100%.
  • interrogate: 100%.
  • Independently reproduced by a peer session on a separate branch before this fix landed, confirming it is not environment-specific.

Developer experience

Closes the last of three known gaps (alongside #1829 and #1832) blocking the required test/coverage/docstring suite for every .github PR.

User experience

None (test-infrastructure-only change; no product-facing behavior change).

🤖 Generated with Claude Code

… and coverage gap

#1831 ("ground verdicts and classify gateway errors") replaced
_extract_served_model's scrub-and-keep sanitization with a delegate
to the new, stricter _safe_model_identifier (reject outright on any
character outside a conservative allowlist), but left three gaps
that broke the required test/coverage/docstring suite for every
.github PR:

1. test_served_model_is_annotation_safe still asserted the old
   scrub-based behavior (a sanitized non-None value). Verified the
   new behavior is not a regression: real provider model ids
   (deepseek-ai/deepseek-v4-pro-0813, meta/llama-3.2-11b-vision-instruct,
   etc., seen in this session's own gateway logs) all still pass the
   new regex unaffected; only genuinely malformed/malicious input is
   now rejected outright. served_model is embedded directly into
   `::warning::`/`::notice::` GitHub Actions workflow commands, so
   reject-and-report-unknown is strictly safer against annotation
   injection than the old scrub approach, not weaker. Updated the
   test to assert the safe outcome (None) instead of a specific
   sanitized string, and added a companion test confirming a real
   provider id still passes through unchanged.

2. The new nested `render` closure inside
   _bounded_allowed_locations_json had no docstring (interrogate
   99.9%). Added one.

3. The new _extract_http_error_served_model function had zero test
   coverage (99% branch coverage, scripts/ci/noema_review_gate.py
   lines 1331-1332, 1340, 1343, 1346 uncovered). Added 7 tests in
   test_noema_model_output_edge_coverage.py covering the success path
   and all six fail-closed branches (unreadable body, oversized body,
   invalid JSON, non-dict payload, non-dict error, non-dict detail).

Independently reproduced by a peer session on a separate branch
before this fix landed, confirming it is not environment-specific.

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

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 32 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: Team

Run ID: ed9f099e-f584-450d-b9e0-65b8a0ff30ba

📥 Commits

Reviewing files that changed from the base of the PR and between 816e3e4 and f3b1fbb.

📒 Files selected for processing (3)
  • scripts/ci/noema_review_gate.py
  • tests/test_noema_model_output_edge_coverage.py
  • tests/test_noema_repair_attempt_telemetry.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 merged commit fd5cec4 into main Sep 4, 2026
5 of 17 checks passed
@seonghobae
seonghobae deleted the fix/served-model-annotation-safe-oracle branch September 4, 2026 05:43
seonghobae added a commit that referenced this pull request Sep 4, 2026
…g after merging main

Merging origin/main (now including #1832/#1835) surfaced a failure in
test_transport_failure_reports_requested_model_and_not_literal_connecting:
expected phase=awaiting_response, got phase=response_error.

Not a base-branch bug -- ruled out by testing fresh main alone (passes
cleanly) and by a peer session's independent clean run. This is a
silent merge-combination bug specific to this branch: this branch's
own unreleased DNS-pinning/TOCTOU hardening also built a careful
connecting-vs-awaiting_response phase distinction (three prior PRs:
bebd7c7, e7b29f2, 5c9d30e) via a `reported_phase` ternary gated on
`active_phase == "connecting"`. Main's #1831 separately added
`active_phase = "response_error"` for the HTTPError case (to also
extract served_model from HTTP error bodies) on a non-overlapping
line, so the merge produced zero conflict markers -- but by the time
this branch's ternary runs, active_phase has already been overwritten,
silently defeating the phase distinction for exactly the HTTPError
case.

Verified "response_error" is not a regression: it is strictly more
specific than the old "awaiting_response" fallback (an HTTPError
proves a real response arrived, unlike a genuinely ambiguous timeout),
and still satisfies the test's real safety property (never mislabel a
real HTTP-error response as "connecting"). The two sibling tests
covering the non-HTTPError paths are untouched, confirming the fix
scope is exactly this one test's stale string expectation, not a
deeper production-code issue. Updated the assertion and docstring; no
production code change needed.

Full verification after fix: 2811 passed, coverage 100%, interrogate
100%.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae added a commit that referenced this pull request Sep 5, 2026
…works (#1868)

The .github-side follow-up this entry named as still-needed (call_llm not
reading the HTTPError response body, so it couldn't surface served_model)
shipped in #1831 (merged), hardened by #1835 and #1850.

Found incidentally while handling an unrelated Autofix event on PR #1757: a
fresh gateway failure now logs phase=response_error and a real model name
(served_model=google/gemma-4-31b-it) instead of the old unknown/connecting
pair. The underlying gateway instability (502 after 284.7s) is still a
separate, open, recurring problem -- but the telemetry gap that made every
prior instance of it undiagnosable is closed.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
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