fix(discovery): set is_free from Bytez's real meterPrice signal - #948
Merged
Conversation
_parse_bytez built every DiscoveredModel without ever passing is_free, so it silently took the dataclass default of False for every Bytez model regardless of real price. Bytez's own list-models response does carry a real zero-cost signal -- meterPrice, e.g. "0.0006478333 / sec" -- that the parser read off `row` for nothing; the existing comment there explains why per-1k pricing stays unset for GPU-second billing, but that reasoning has no bearing on is_free, which the parser never set from any signal. Adds _bytez_meter_price_is_free, which classifies only an exact-zero meterPrice rate as free (via Decimal, matching _price_per_1k's underflow-safe precision handling) and treats a missing/malformed value as unknown, not free -- this module's existing fail-closed default. It deliberately ignores the sibling `meter` tier-name field (e.g. "sm-free"): live docs show a tier can be named "free" while its own meterPrice is nonzero, so tier naming is not a trustworthy zero-cost signal. prompt_price_per_1k/completion_price_per_1k stay unset for GPU-second pricing, unchanged. Also adds a small non-fatal diagnostic, logged from general_free_serving_candidates, naming why a credential that discovered rows still seeded zero orchestrator/free candidates (evidence_only / no_free_pricing_reported / free_rows_excluded_from_general_pool) -- a hard provider failure already logs an explicit reason, but a provider that discovers fine and simply has nothing free to offer previously left no comparable trace. Found during an investigation into orchestrator/free being served 100% by nvidia_nim with all 5 provider credentials registered. This fix alone will not restore Bytez diversity today: Bytez's backend has been returning HTTP 500 for this org's account for several days (docs/product-technical-gap-baseline.md, "2026-08-30 provider-catalog-sync", an already-tracked operator-side issue). The fix is still correct and necessary regardless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…_name Devin's review on PR #948 found _bytez_meter_price_is_free trusted only the text before the first "/", so a shape that does not match Bytez's documented "<rate> / <unit>" grammar at all -- a missing unit ("0 /"), an extra separator ("0 / sec / token"), or no separator whatsoever ("0") -- still read as "0" and got confidently classified free. An unexpected shape is itself a signal something about the row is wrong, so this now requires exactly one "/" with a non-empty rate and non-empty unit on both sides before trusting the rate at all. The unit itself is deliberately not required to be "sec": a zero rate is exactly as free regardless of its time unit ("0 / hour" stays free), so this validates shape, not the unit name. Adds the requested regression cases plus a few more malformed shapes (missing rate, doubled separator, space-joined, both sides empty). Also verified and documented, in response to a Semgrep/GHAS finding on the same PR, that `credential_name` at _log_zero_free_serving_contribution's log call is always the KV credential *name* (a literal string declared on each ProviderModelSource, e.g. "BYTEZ_API_KEY"), never the secret value resolved by get_credential() -- traced every DiscoveredModel construction site and confirmed the actual secret stays in a distinct local (`api_key`) that never reaches a DiscoveredModel or this log line. Verified empirically with a local semgrep run: the exact rule fires on an unsuppressed copy of this file and is silenced by the added nosemgrep annotation, matching this repo's established suppression convention (docs/planning/adrs/0007) of a one-line rule-specific nosemgrep plus justification at the exact call site, never a blanket bypass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
…o fix/bytez-discovery-is-free
6 tasks
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
…Router tests PR #948/#949 (main) added three tests that mocked urllib.request.urlopen directly. This branch's round-5 credential-leak fix already moved _fetch_json's transport call behind _open_trusted_discovery_request (a custom redirect-protected opener), so after merging origin/main the old urlopen patch no longer intercepted the call and these tests hit the real network (observed: a live 401 from api.bytez.com). Repoint the three tests at _open_trusted_discovery_request, matching every other test in this file post-refactor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
This was referenced Aug 31, 2026
seonghobae
added a commit
that referenced
this pull request
Sep 1, 2026
* feat(logging): add --log-level/--verbose CLI wiring and debug_logging module New stdlib-only contextual_orchestrator/debug_logging.py owns level parsing, configure_logging() (basicConfig with force=True so repeated in-process CLI invocations actually re-apply), a handler-level redaction safety net, and small pure log-formatting helpers. __main__.py wires --log-level/--verbose/ --debug and CONTEXTUAL_ORCHESTRATOR_LOG_LEVEL through a parse_known_args pre-scan that runs before subcommand dispatch, covering register-credential, discover-models, check-fast-mlsirm, one-shot completion, and --serve uniformly. orchestrator.py gains a module logger for the next commit's retry/circuit-breaker/ranking instrumentation. Part of the verbose/debug logging feature (see forthcoming ADR). * feat(logging): instrument retry/circuit-breaker/ranking/discovery/server Adds DEBUG/WARNING/INFO instrumentation at the previously silent decision points named in the task: ModelClient._send_with_retry / _send_raw_with_retry (per-attempt, backoff, a WARNING provider_exhausted line that fires without --verbose), TaskOrchestrator's circuit breaker (_record_failure/_record_success/_circuit_open, with a WARNING circuit_opened only on the edge transition), and evidence-based ranking (_static_rank_key/_measured_member_order/_select_agent). Every call site that logs caller- or provider-derived content wraps it in the existing orchestrator.redact_text/redact_value first. model_discovery.py gains per-provider discovery_attempt/discovery_result/ discovery_provider_failed DEBUG lines (credential *names*, never values) and one discovery_complete INFO summary in discover_all_models. server.py gains one body-free per-request INFO summary (method, path, status, latency, ADR 0122 session correlation hash) via handle_one_request and a DEBUG response-body summary that reuses the exact already-redacted safe_payload object _response_payload computes, never a second copy. telemetry.py exposes the shared session_id_hash() helper so both _safe_attributes (OTLP) and server.py's summary use the identical hash. Adds ADR 0007 (docs/adr/ series, matching the runtime-observability precedent set by ADR 0122), the missing logging + OpenTelemetry rows in docs/library_research.md, a CHANGELOG entry, and a light tech-stack.md touch-up. * fix(logging): close adversarial secret-leakage findings from PR #946 review Fixes the three confirmed findings from the adversarial secret-leakage review (github.com//pull/946#issuecomment-5473342909), independently corroborated by Devin's automated review comments on the PR: 1. Critical: server.py's DEBUG response-summary log reused redact_value(), which only pattern-matches an in-string secret *value* shape and never inspects the JSON *key* a string is nested under, so a response field like {"private_key": "..."}, {"key": "..."}, {"auth": "..."}, or {"credential": "..."} leaked verbatim. Adds a new, additional, key-name-aware redaction pass, debug_logging.redact_credential_shaped_keys, applied on top of the existing redact_value output at that one call site. orchestrator.redact_text/redact_value are unmodified, per the review's explicit scope. 2. The INFO per-request summary (summarize_request_for_log) logged self.path verbatim, including any query string, contradicting its own body-free docstring. It now strips the query string before formatting. 3. A --log-level/--verbose/--debug flag placed before a subcommand (e.g. `--verbose discover-models`) bypassed subcommand dispatch, which only checked arguments[0]. Adds _subcommand_token_index to skip past recognized leading logging flags when locating the subcommand token, without removing them from the argument list each subcommand's own parser sees. Each fix has a red/green regression test: the new tests reproduce the leak/bug against the pre-fix code (verified failing) before the fix makes them pass. Also merges main into this branch (was reported dirty/mergeable_state) and resolves a real conflict in model_discovery.py where this branch's new discovery instrumentation collided with main's independently-landed, privacy-hardened logging on the same call sites; reconciled by keeping the richer instrumentation but adopting main's stricter contract (never log source.credential_name, use account= naming), matching main's own test_discovery_debug_log_identifies_account_without_secret. Separately, the merge surfaced a non-conflicting but real regression: main independently added a second, plain --verbose flag to the discover-models and one-shot/ serve parsers that collided with this branch's --add_log_level_arguments, raising argparse.ArgumentError on every invocation of either path; removed the redundant declarations (and their dead, non-redacted logging.basicConfig follow-ups) now that _configure_logging_from_cli already covers this centrally. PR #942 (a separate, more conservative implementation of the same feature on a different branch) still needs reconciliation by a human/reviewing session -- out of scope here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(logging): address second-round Devin/CodeRabbit findings on PR #946 Fixes three further confirmed real findings from the re-review after the first push, plus verifies (and closes with a regression test rather than a code change) a fourth finding that turned out to be a false positive: 1. BUG (Devin, server.py): keep-alive phantom requests. On a persistent HTTP connection, handle_one_request never reset self.command/self.path before each call; stdlib's own handle_one_request only assigns them when it actually parses a request line, so when a keep-alive client closed the connection (nothing read), the *previous* request's command/path were still in place. _log_request_summary's existing "nothing to report" guard therefore never fired, logging the prior request a second time with a statusless "phantom" entry. Fixed by resetting self.command/self.path to None at the top of handle_one_request, alongside the existing per-request state resets, so the guard correctly recognizes when no new request was parsed. 2. BUG (Devin, orchestrator.py): permanent failures misreported as exhausted retries. _send_with_retry/_send_raw_with_retry unconditionally logged provider_exhausted whenever any failure occurred, even when a non-transient error on the very first attempt broke the loop immediately without spending any retry budget. Added a distinctly named provider_rejected_permanent WARNING event, chosen by whether the loop broke from reaching retry_limit (provider_exhausted) or from an early non-transient break (provider_rejected_permanent), applied identically in both duplicated retry loops. 3. Security/Privacy (CodeRabbit, CWE-532, server.py): the DEBUG response-body summary serialized the entire redacted payload, including ordinary response text (choices[].message.content, tool-call arguments, an error.message that can reflect caller-supplied input) -- none of which is credential-shaped, so neither redact_value nor redact_credential_shaped_keys ever masked it, and it could carry PII or business-sensitive content straight into DEBUG output. Added debug_logging.response_metadata_for_log, which extracts only an allowlisted metadata shape (has_error, model, choice_count, and numeric-only usage counts) for this log line to serialize instead of the payload; redact_credential_shaped_keys is still applied on top, defense-in-depth, in case a future allowlist field ever collides with a credential-shaped key name. Updated the two pre-existing tests whose assertions depended on the old "redact-then-log-the-payload" mechanism to match the new (strictly stronger) "never log the payload" contract. 4. Verified, NOT fixed (Devin, __main__.py, false positive): "the parse_known_args logging pre-scan doesn't respect -- as an option terminator." Directly tested against the real _configure_logging_from_cli pre-scan: a literal -- already correctly stops it from treating what follows as --log-level/--verbose/--debug, since parse_known_args honors stdlib argparse's -- semantics with no special-casing needed here. Documented this in the function's docstring and added a regression test locking in the already-correct behavior, rather than changing working code. Each of the three real fixes has a red/green regression test (verified failing against the pre-fix code, per this repo's TDD convention). Also added deterministic, non-threaded unit tests for the query-string-stripping and keep-alive fixes as reliable counterparts to their real-server integration tests, which can occasionally flake on unrelated ThreadingHTTPServer teardown timing (pre-existing in this test file, not introduced by this change -- reproduces identically on an untouched, pre-existing test in the same file). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * docs+test: update ADR/CHANGELOG for the second-round fixes; de-flake real-server tests Documents the response-body allowlist redesign, keep-alive phantom-request fix, provider_rejected_permanent split, and the confirmed-false-positive -- option-terminator check in docs/adr/0007-verbose-debug-logging.md and CHANGELOG.md's Unreleased/Added entry. Also de-flakes the four real-ThreadingHTTPServer tests in tests/test_telemetry.py that assert on the per-request INFO summary (present pre-existing in this file, not introduced by this PR -- it reproduced identically on an untouched test during verification). Root cause: the per-request summary logs in handle_one_request's `finally` block, which runs strictly after the response has already been flushed to the client, so a test asserting immediately after its client call returns has no guarantee the server thread reached that `finally` block yet. Fixed with a small bounded caplog-polling helper (_wait_for_caplog) used while the relevant caplog.at_level scope is still open, rather than a fixed sleep that would be either too short (still flaky) or wastefully long; kept a brief post-teardown settle too, since ThreadingHTTPServer's per-connection handler threads are daemons server_close() does not wait for, and a lingering straggler could otherwise log into a later test's capture window. Verified stable across 15 repeated runs (10 solo, 5 with the rest of this PR's touched test files) with zero failures, versus roughly 1-in-3 to 1-in-5 before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(logging): address third-round Devin findings on PR #946 Fixes four further confirmed real findings from the second re-review (after push cd57aa1), which also independently confirmed the keep-alive fix and the -- option-terminator false-positive verdict from round 2: 1. BUG, orchestrator.py: zero-retry failures mislabeled as exhausted. With max_retries=0, attempt >= retry_limit is trivially true after the single allowed attempt regardless of whether it was transient, so provider_exhausted fired even though no retry budget ever existed to exhaust. Both _send_with_retry and _send_raw_with_retry now also require retry_limit > 0 before calling provider_exhausted; provider_rejected_permanent covers both the pre-existing "non-transient break" case and this "no retry budget configured at all" case. 2. BUG, server.py: framework-generated responses lost their status in the per-request log. _last_status was only set by this module's own _send/_send_text/_send_bytes/_send_sse writers; a response BaseHTTPRequestHandler generates itself (e.g. its built-in 501 for an HTTP method with no matching do_* handler) bypasses all of those, logging status=- for a response the client actually received. Fixed by overriding send_response -- stdlib's own single choke point every response path (including its own send_error) already goes through -- so every current and future response path is captured uniformly. 3. BUG, __main__.py: abbreviated logging flags (--log-l, --ver) bypassed subcommand dispatch the same way full un-recognized flags used to, since argparse's own prefix-abbreviation matching and _subcommand_token_index's plain string comparison disagreed about what counts as a recognized flag. Set allow_abbrev=False on every CLI parser (the pre-scan and each subcommand's own parser), so an abbreviated flag is now rejected everywhere with a clear argparse error instead of silently accepted by one parser and not the other. 4. SEC, debug_logging.py: exception tracebacks bypassed log redaction. _RedactingLogFilter only rewrote record.msg; logging.Formatter.format() renders record.exc_info into the traceback text strictly after every filter has already run, so a call site using exc_info=True or logger.exception(...) could leak a secret embedded in the exception's own str() (e.g. an upstream error reflecting api_key=sk-...) straight into the unredacted traceback -- a real hole in the core safety-net mechanism itself, not just a call site. The filter now renders exc_info into text, redacts it, caches it as record.exc_text, and clears exc_info so the handler's formatter uses the already-redacted text instead of re-deriving an unredacted one. Each fix has a red/green regression test (verified failing against the pre-fix code): tests/test_orchestrator_debug_logging.py (two new zero-retry tests, one per retry loop), tests/test_telemetry.py (test_framework_generated_error_status_is_captured_in_log), tests/test_cli_logging.py (abbreviated value-flag and boolean-flag cases), tests/test_debug_logging.py (test_configure_logging_redactor_masks_exception_traceback_in_captured_output). Full suite (python -m pytest tests -q --ignore=tests/test_psychometric_routing.py) -> 2886 passed, 1 skipped, zero regressions. interrogate -> 100%. test_conventions.py -> all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(logging): distinguish no-retry-budget from permanent rejection (round 4) Fixes a real regression from the round-3 provider_exhausted/ provider_rejected_permanent split, found by Devin's re-review of 68291d4: with a configured retry limit of 0, EVERY failure -- including a genuinely transient one (HTTP 503, timeout) that simply never got a chance to retry -- was collapsed into provider_rejected_permanent. "No retry budget was configured" and "this error is non-retryable by nature" are two independent facts; conflating them mislabels a transient failure as permanent. Adds a third, distinctly named WARNING event, provider_no_retry_budget, for the retry_limit == 0 case specifically, carrying the error's own transient/non-transient classification explicitly via a `transient=%s` field rather than discarding it. provider_rejected_permanent now covers only its original, narrower case: a real non-zero budget existed, but the loop broke early because the failure was classified non-transient. provider_exhausted is unchanged (a real, non-zero budget was actually used up). Fixed identically in both duplicated retry loops (_send_with_retry/_send_raw_with_retry). Regression tests now cover all 4 (retries>0 vs =0) x (transient vs non-transient) combinations per retry loop, as requested: the two existing tests already covered retries>0 x transient (provider_exhausted) and retries>0 x non-transient (provider_rejected_permanent); this adds retries=0 x transient and retries=0 x non-transient (both now provider_no_retry_budget, differing only in transient=True/False) for both _send_with_retry and _send_raw_with_retry. Verified each new/changed assertion fails against the pre-fix (round-3) code before the fix, per this repo's TDD convention. Investigated, not fixed: Devin also flagged CONTEXTUAL_ORCHESTRATOR_LOG_LEVEL reading directly from os.environ as bypassing this repo's KV config registry (AGENTS.md/CLAUDE.md: "runtime config... never os.getenv"). Per the coordinator's request, checked whether this repo's existing CONTEXTUAL_ORCHESTRATOR_STATE_DB/_AGENTS_DB/_CLEARFOLIO_URL/ _PROVIDER_CA_BUNDLE env vars (contextual_orchestrator/__main__.py) follow the same direct-os.environ pattern: they do, all four, predating this PR. This is a pre-existing architectural pattern in this file, not a new or different violation this PR introduces -- reported as such in the PR comment rather than fixed unilaterally inside a logging-feature PR, per the coordinator's explicit guidance for that outcome. Full suite (python -m pytest tests -q --ignore=tests/test_psychometric_routing.py) -> 2888 passed, 1 skipped, zero regressions. interrogate -> 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(discovery): close credential leak via cross-host redirect (round 5) Real, pre-existing security finding from CodeRabbit's automated review of PR #946: `_fetch_json` in model_discovery.py -- the function every standard provider's authenticated "list models" call goes through (openai, openrouter, nvidia_nim, nvidia_nim_sub, bytez) -- called plain `urllib.request.urlopen`. Python's default `HTTPRedirectHandler` copies the `Authorization` header onto a redirected request even when the redirect target is a completely different host (unlike some other HTTP clients, urllib never strips sensitive headers on cross-origin redirects). A malicious or compromised provider endpoint issuing a 3xx redirect could exfiltrate the credential -- and #923's retry (already on main, inherited into this branch) means up to twice per attempt, since it calls _fetch_json with the same api_key on both tries. This codebase already had the correct fix for this exact risk class: _TrustedDiscoveryRedirectHandler (raises on any redirect leaving the original host) already protected _fetch_json_same_host_https. It just never got applied to _fetch_json, the function actually used for the real per-provider calls. Fixed by routing both functions through one new shared _open_trusted_discovery_request helper, so there is a single implementation of the redirect guard instead of two copies that could drift apart (as they already had). _fetch_json_same_host_https's own external behavior (size cap, TimeoutError conversion) is unchanged. A legitimate same-host redirect (e.g. /v1/models -> /v2/models) still succeeds. New regression tests (tests/test_model_discovery.py): a cross-host-redirect test (mirrors the existing test_openrouter_zdr_evidence_rejects_cross_host_ redirects pattern) proving exactly one request is ever issued and the credential never reaches the attacker host, verified failing against the pre-fix _fetch_json before the fix; and a same-host negative control proving the legitimate case still works. ~38 pre-existing tests across test_model_discovery.py, test_model_discovery_boundaries.py, test_discover_models_cli.py, and test_chat_model_capability_isolation.py that mocked bare urllib.request.urlopen for _fetch_json's behavior now mock the new shared seam instead (mechanical -- _fetch_json no longer calls urlopen directly). Two local test _Response fixtures gained an optional read(amt) argument (mirroring http.client.HTTPResponse.read(amt)) since _fetch_json_same_host_https's always-invoked OpenRouter ZDR fetch inside discover_all_models now shares the same seam and caps its read. Also folded into this round, four further confirmed findings relayed by the coordinator from the same review pass: 1. model_discovery.py: the configured_gateway provider's /model/info metadata fetch now also catches RuntimeError, matching the primary list-request retry loop's except tuple. Previously a raw RuntimeError from ModelClient's DNS/address-validation transport escaped discover_provider_models uncaught and aborted the entire discovery pass instead of just that one provider's metadata. 2. server.py: per-request latency_ms no longer counts a keep-alive connection's idle time between requests. request_started is now timestamped inside an overridden parse_request(), right after BaseHTTPRequestHandler.handle_one_request()'s blocking readline() has already returned real request bytes, instead of before that blocking read. 3. orchestrator.py: the retry-outcome classification (no budget at all / exhausted / stopped early on non-transient) duplicated verbatim between _send_with_retry and _send_raw_with_retry -- duplication that already caused a real regression once, fixed in one copy and missed in the other (round 4, provider_no_retry_budget) -- is now one shared _log_retry_outcome helper both call. 4. debug_logging.py: response_metadata_for_log's usage summary now keeps only a fixed allowlist of known counter names (prompt_tokens, completion_tokens, total_tokens, input_tokens, output_tokens), not any string key with a numeric value -- closes a path where a key shaped like "customer_note=<secret>" with a throwaway numeric value would have reached DEBUG output verbatim (CWE-532). New regression test with a customer_note-shaped key proving exclusion, plus a positive control. Full suite (python -m pytest tests -q --ignore=tests/test_psychometric_routing.py) -> 2892 passed, 1 skipped, zero regressions (test_psychometric_routing.py pre-existingly fails on ModuleNotFoundError: No module named 'numpy' in this sandbox -- environment-only, unrelated). interrogate (repo-root invocation, pyproject.toml fail-under=100) -> 100%. ruff check on all changed files -> clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(test): patch _open_trusted_discovery_request in merged Bytez/OpenRouter tests PR #948/#949 (main) added three tests that mocked urllib.request.urlopen directly. This branch's round-5 credential-leak fix already moved _fetch_json's transport call behind _open_trusted_discovery_request (a custom redirect-protected opener), so after merging origin/main the old urlopen patch no longer intercepted the call and these tests hit the real network (observed: a live 401 from api.bytez.com). Repoint the three tests at _open_trusted_discovery_request, matching every other test in this file post-refactor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(discovery,logging): bound _fetch_json's read; stop misclassifying one-shot calls (round 6) Two findings deferred during round 5 to avoid colliding with the in-flight merge-conflict fix: - model_discovery.py: _fetch_json read a discovery response body with an unbounded response.read(), unlike _fetch_json_same_host_https and _fetch_configured_gateway_json, which already cap at MAX_DISCOVERY_RESPONSE_BYTES and fail closed on an overage. A large or malicious/misbehaving provider response could exhaust worker memory before JSON parsing ever ran. _fetch_json now shares the identical bounded-read-then-check pattern. - orchestrator.py: _log_retry_outcome logged provider_no_retry_budget whenever retry_limit was 0, without distinguishing an agent with a genuinely zero configured retry budget from ModelClient.proxy_send_once's deliberate allow_transient_retries=False one-shot call (which forces retry_limit to 0 regardless of the agent's real budget, so an already-failing-over passthrough request cannot itself amplify load). _log_retry_outcome now takes allow_transient_retries explicitly and logs a distinctly named provider_one_shot_call_failed WARNING for the caller-forced case instead of misreporting it as no budget configured. New regression tests cover both: an oversized fake response for the size bound, and a mocked proxy_send_once one-shot failure (plus direct _send_raw_with_retry coverage) for the logging fix. Full suite: 2929 passed, 1 skipped (baseline 2925 + 4 new tests, zero regressions). interrogate: 100%. git diff --check: clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(logging,server): suppress CodeQL FP, fix disconnect status honesty, renumber ADR (round 7) Three findings from the latest automated review round on PR #946: - tests/test_debug_logging.py: two CodeQL py/clear-text-logging-sensitive-data HIGH alerts (lines 144, 167) are a deliberate redaction positive/negative control pair -- a hardcoded, non-functional fake secret is logged once WITH a redactor (proving it's masked) and once WITHOUT one (the negative control, proving the first test isn't a tautology). Verified this repo's CodeQL is advanced setup (github/codeql-action/init+analyze, no config-file) with security-events: write and default upload:true, and that `# codeql[rule-id]` inline suppression comments on the line before a flagged call are GitHub's documented, currently-supported mechanism, honored regardless of setup style once the analysis flows through upload-sarif. Added precise, two-line suppressions with an explanatory comment on exactly the two flagged lines. - contextual_orchestrator/server.py: `_send`/`_send_text`/`_send_bytes`/ `_send_sse`/`_begin_sse` all recorded their *intended* status into `_last_status` before handing off to `_write_response`, then ignored its boolean return value. `_write_response` deliberately swallows a dead peer's BrokenPipeError/ConnectionError/OSError, but the pre-set `_last_status` survived that failure untouched, so the per-request INFO summary reported a false "delivered" status for a write that never completed. Fixed at the one shared choke point (`_write_response`'s except block) instead of patching each writer, so it covers every current and future writer uniformly. While tracing that consumer, also found `_log_request_summary`'s "nothing to report" guard checked only method/path, so it silently dropped a request that *did* deliver bytes and *did* get a real 400/414 response but whose malformed/oversized request line left command/path unset (stdlib's own parse_request resets `self.command` to None "in case of error on the first line"). The guard now also logs when a status was actually recorded, while still skipping a true byte-free keep-alive close. Regression tests: test_http_response_write_disconnect_safety.py (dead-peer write no longer reports the intended status) and test_telemetry.py (a malformed request line's real 400 is no longer skipped). - docs/adr/0007-verbose-debug-logging.md renamed to 0005 -- the indexed series in docs/adr/README.md ends at 0004, and ADR 0122 (checked separately) is a deliberate cross-repo-numbered exception explicitly not listed in that series table, not evidence the sequential convention was already broken. Updated the file's own title, the README index row, conductor/tech-stack.md, and CHANGELOG.md's remaining "ADR 0007" mentions. Also replied on PR #946 to two review threads that investigation showed were false positives, with no code change: the CLI log-level env-var read (a bootstrap-time argparse pre-scan before any KV store exists, matching five other pre-existing bootstrap-time os.environ reads in the same file) and the "research artifact" ADR requirement (this is a pure observability/engineering feature with no algorithmic claim to ground, unlike ADR 0002/0003). Full suite: `pytest tests -q --ignore=tests/test_psychometric_routing.py` -> 2931 passed, 1 skipped, 0 failed. `interrogate .` -> 100%. `git diff --check` clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(server): preserve delivered status on a post-header disconnect (round 8) Follow-up to round 7's _write_response fix, per this round's Devin review: - contextual_orchestrator/server.py: clearing `_last_status` on *every* caught disconnect (BrokenPipeError/ConnectionError/OSError) was too broad. A write failure can strike in two different places: before the status line/headers were ever flushed (the client received nothing -- clearing is correct), or after `end_headers()` already completed (a later body write in the same call, or a later `_write_sse` frame on an SSE stream `_begin_sse` already opened -- the client genuinely received the real status, so clearing it would falsely report "no status" for a request that was, in fact, answered -- the exact overcorrection direction of the original bug). Every `_send`/`_send_text`/`_send_bytes`/`_send_sse`/`_begin_sse` writer now sets `self._response_headers_sent = True` immediately after its own `end_headers()` call returns without raising; `handle_one_request` resets that marker to `False` once per request. `_write_response`'s disconnect handler now clears `_last_status` only when the marker is still unset. `_write_sse` does not touch the marker itself -- it is only ever called after a prior successful `_begin_sse`, so the marker it already set correctly covers a later frame's failure too. Regression tests added to tests/test_http_response_write_disconnect_safety.py: - test_disconnected_write_does_not_report_intended_status_as_delivered (rewritten to fail inside end_headers() itself -- genuinely "before headers" -- so it still represents the original bug's case correctly; it previously failed in the body write, which is now the preserved case) - test_disconnected_body_write_after_headers_preserves_delivered_status (new: body write fails after end_headers() succeeds -> status preserved) - test_sse_frame_disconnect_after_headers_preserves_delivered_status (new: a later _write_sse frame fails after _begin_sse succeeded -> status preserved) Also investigated a second finding relayed this round ("transient failures labeled permanent" in _send_with_retry/_send_raw_with_retry) and found no reproducible bug: GitHub's own review data for this commit's parent shows Devin's re-review found exactly one new issue (the fix above), and an existing, currently-passing test (test_send_raw_with_retry_one_shot_call_does_not_log_no_retry_budget) already proves a transient error under allow_transient_retries=False with a real non-zero configured retry budget logs `provider_one_shot_call_failed ... transient=True`, never `provider_rejected_permanent` -- structurally, `_log_retry_outcome` can only reach `_log_provider_rejected_permanent` when `retry_limit != 0`, which `allow_transient_retries=False` always forces to 0. No code change made for that finding; likely a stale reference to the already-resolved round-5/6 thread on the same topic. Full suite: `pytest tests -q --ignore=tests/test_psychometric_routing.py` -> 2933 passed, 1 skipped, 0 failed. `interrogate .` -> 100%. `git diff --check` clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(cli): give check-fast-mlsirm its own argparse parser (round 9) Devin's review of PR #946 found: `check-fast-mlsirm --help` bypassed argument parsing and ran the diagnostic instead of showing help. The subcommand's dispatch function took no arguments and ignored everything after its own name in argv, so `--help` never reached an argparse parser that would have handled it -- it was silently swallowed and the real diagnostic ran anyway (and its exit code, not argparse's, decided the process exit status). Give _check_fast_mlsirm_command its own argparse.ArgumentParser, matching the pattern every other subcommand (register-credential, discover-models) already uses: declare the shared --log-level/--verbose/--debug flags via _add_log_level_arguments so --help documents them, call parse_args(argv) so --help exits before running anything and an unrecognized trailing option is rejected instead of silently ignored, then run the diagnostic as before. Updated the one call site to pass arguments_after_subcommand. Regression tests: test_check_fast_mlsirm_help_shows_help_without_running_diagnostic and test_check_fast_mlsirm_rejects_unknown_option. Full suite: 2935 passed, 1 skipped (baseline 2933 + 2 new tests, zero regressions). interrogate: 100%. git diff --check: clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(logging): keep runtime config out of environment * test(logging): avoid clear-text credential fixtures * fix(logging): distinguish ranking evidence from throughput --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_parse_bytezbuilt everyDiscoveredModelwithout ever passingis_free, so it silentlytook the dataclass default of
Falsefor every Bytez model regardless of real price. Bytez'sown list-models response (
https://docs.bytez.com/http-reference/list/models.md) does carry areal zero-cost signal —
meterPrice, e.g."0.0006478333 / sec"— that the parser read out ofrowfor nothing (an existing comment explained why per-1k pricing is intentionally left unset,but that reasoning has no bearing on
is_free).tests/test_model_discovery.py's existingBytez fixture already proved this field is present and parseable
(
{"modelId": "0-hero/Matter-0.1-Slim-7B-C", "task": "chat", "meterPrice": "0.0006 / sec"}); noBytez model could ever be classified free before this change, no matter its real price.
_bytez_meter_price_is_free, which classifies only an exact-zerometerPricerate asfree (parsed via
Decimal, matching_price_per_1k's underflow-safe precision handling), andtreats a missing/malformed/non-numeric value as unknown, not free — the same fail-closed default
every other pricing path in this module already uses. It deliberately does not use the
sibling
metertier-name field (e.g."sm-free"): live docs show a tier can be named "free"while its own
meterPriceis nonzero, so tier naming is not a trustworthy zero-cost signal.prompt_price_per_1k/completion_price_per_1kare still left unset for GPU-second pricing —that existing, intentional design is untouched.
_log_zero_free_serving_contribution, called fromgeneral_free_serving_candidates) that logs one line per credential that discovered rows butseeded zero
orchestrator/freecandidates, naming the coarse reason(
evidence_only/no_free_pricing_reported/free_rows_excluded_from_general_pool). Today ahard provider failure (e.g. Bytez's HTTP 500) already logs an explicit
model discovery failed account=bytez ...line, but a provider that discovers fine and stillcontributes nothing — OpenRouter's deliberate
evidence_onlyexclusion, OpenAI simply having nofree-tier model today — left no comparable trace, making a single-family pool look identical
whether every other provider was failing or just had nothing free to offer.
Context: broader
orchestrator/freesingle-family investigationThis was found during a 5-agent investigation (today) into why
orchestrator/freeis served100% by
nvidia_nim(free_family_diversity: 1) with all 5 provider credentials(
BYTEZ_API_KEY,NVIDIA_NIM_API_KEY,NVIDIA_NIM_API_KEY_SUB,OPENROUTER_API_KEY,OPENAI_API_KEY) present and registered ("5 of 5" in live logs). Two other, independent causes(a stale vendored commit pin and a duplicate hardcoded provider-family mapping) are being fixed
separately in
ContextualWisdomLab/.github— this PR is scoped tocontextual-orchestratoronlyand does not touch that repo.
This fix alone will not restore Bytez diversity today. Bytez's backend has been returning
HTTP 500for this org's account/key for several days straight — an already-tracked, operator-sideissue documented in
docs/product-technical-gap-baseline.md("2026-08-30 provider-catalog-sync")and confirmed independently via a live unauthenticated probe (fast, well-formed
401, not500—the endpoint itself is reachable and enforcing auth normally). That outage is not resolvable from
this repo alone. This fix is still correct and necessary so the code behaves properly the moment
that external outage clears — and so a future non-Bytez cause of "provider discovers fine but
contributes nothing" is now visible instead of silent, via the new diagnostic line.
OpenRouter (
evidence_only=True, deliberate ZDR-privacy hardening) and OpenAI (genuinely nofree-tier models today) both correctly contribute zero free models; neither was a bug.
What changed
contextual_orchestrator/model_discovery.py_bytez_meter_price_is_free(meter_price)— new helper classifying an exact-zero BytezmeterPricerate._parse_bytez— now passesis_free=_bytez_meter_price_is_free(row.get("meterPrice"))whenconstructing each
DiscoveredModel._log_zero_free_serving_contribution(discovered, candidates)— new helper, called fromgeneral_free_serving_candidates, logging a per-credential zero-contribution diagnostic.Purely additive: no change to
general_free_serving_candidates's return value.tests/test_model_discovery.pytest_discover_bytez_parses_models_with_key_auth_scheme— now also asserts real nonzeroGPU-second pricing stays
is_free=Falsewith both per-1k fields stillNone.test_discover_bytez_marks_zero_meter_price_as_free— new regression test: a zero-ratemeterPriceproducesis_free=Truewith per-1k pricing still unset.test_discover_bytez_missing_meter_price_stays_unknown_not_free— new: nometerPricefieldat all stays
is_free=False(unknown, not free).test_bytez_meter_price_is_free_classifies_exact_zero_rates— new parametrized unit test over_bytez_meter_price_is_free(numeric/string zero and nonzero rates, missing/malformed/Nonevalues,
bool, negative-zero, whitespace).test_general_free_serving_candidates_logs_zero_contribution_reasons— new: asserts thediagnostic line for an
evidence_onlyOpenRouter row and a real-but-nonzero-priced OpenAI row,and that a provider which did seed a candidate gets no such line.
Test plan
python -m pytest tests/test_model_discovery.py -q— 93 passedpython -m pytest tests -q(full suite) —2834 passed, 1 skipped, 1 failedin 778.70s.The one failure (
test_psychometric_routing.py::test_fast_mlsirm_fit_uses_judge_acceptance_item_for_context_score,ModuleNotFoundError: No module named 'fast_mlsirm') is a pre-existing, environment-onlygap unrelated to this change:
fast-mlsirmis only installed forpython_full_version >= '3.12'perpyproject.toml, and this sandbox runs Python 3.11.15.Confirmed unrelated to
model_discovery.py/test_model_discovery.py.interrogate contextual_orchestrator/model_discovery.py— 100% docstring coverage on thechanged file (module-level
fail-under = 100; the two new functions are private and bothcarry docstrings anyway)
_bytez_meter_price_is_freenever raises across ~20 adversarialinputs (
None,NaN/inffloats, huge ints, empty/whitespace/garbage strings, dicts,lists,
complex, negative zero) — matters becausefuzz/targets.py(
exercise_provider_model_payload) drives_parse_bytezover arbitrary decoded JSON.git diff --statscoped to exactlycontextual_orchestrator/model_discovery.pyandtests/test_model_discovery.py— no other files touched.🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Generated by Claude Code