fix(noema): surface real sidecar fail-closed diagnostics past redaction - #1425
Merged
seonghobae merged 3 commits intoAug 30, 2026
Merged
Conversation
The sanitize_contextual_orchestrator_sidecar_stream.py allowlist still matched contextual_orchestrator_review_launcher.py's old "no zero-cost models" wording, not its actual "no eligible models" SystemExit text, and had no entry at all for the missing-auth-token or missing-provider-credential fail-closed messages. All three fell through to omitted_unstructured_lines=N, hiding a real (non-secret) root cause behind an opaque count instead of reaching CI operators. This surfaced today: after #1422 bumped the contextual-orchestrator pin, noema-review started failing "sidecar exited before healthz (status 1); stderr: omitted_unstructured_lines=1" with no visible cause. Live local reproduction (real network discovery, the five CI provider secrets, the exact pinned commit's requirements.lock) traces the actual root cause to upstream commit 952996ec marking OpenRouter evidence_only=True (a correct, deliberate ZDR-privacy hardening) -- OpenRouter was the sidecar's only credentialed provider that ever reports genuine per-model pricing, so orchestrator/free's pool is now structurally empty. That policy question is documented in a new dated docs/product-technical-gap-baseline.md entry, not resolved here: it needs a human call on either accepting real provider spend (pool=auto) or wiring a verified zero-cost provider, and is out of scope for a log-redaction fix. This change fixes only the independent, safe half: the sanitizer no longer hides the diagnostic. Updated the matching pinned assertions in tests/test_contextual_orchestrator_review_runtime_preflight.py. Validation: PYTHONPATH=. python3 -m pytest tests -q (1875 passed, 1 skipped, 25 subtests), coverage (changed file 100%; repo-wide 99% is the pre-existing, separately-owned scripts/ci/pingora_edge_policy.py:274 gap, unrelated to this change), interrogate (100.0%). Co-Authored-By: Claude <noreply@anthropic.com>
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…view Devin's review on #1425 caught a real bug in that same PR: the launcher's stdout/stderr were piped through bare `> >(sanitizer)` process substitutions with no way to wait for them, so the failure-path read of $sidecar_stderr could race the still-draining sanitizer and silently show an empty or truncated diagnostic -- defeating the very fix #1425 makes. Reproduced the race directly: a minimal standalone harness mirroring the old pattern lost the diagnostic 3/3 runs; the same harness using named fds (`exec {fd}> >(...)`, capturing the sanitizer's own PID via `$!`, closing our copy of the fd so the substitution sees EOF, then `wait`ing the sanitizer PID before reading) captured it 3/3 runs. Fix: name the two sanitizer process substitutions via `exec {fd}> >(...)` so their PIDs are capturable, and wait for both once the sidecar process is confirmed exited (never in the healthz-timeout branch, where it may still be running) before reading the sanitized stderr file. Co-Authored-By: Claude <noreply@anthropic.com>
| # already sent EOF; draining the sanitizer here cannot hang, and it | ||
| # guarantees $sidecar_stderr holds everything the sidecar wrote before we | ||
| # read it for the failure message below. | ||
| wait_for_sidecar_sanitizers |
Contributor
There was a problem hiding this comment.
🟡 Startup timeout still races diagnostics
When startup times out, wait_for_sidecar_sanitizers runs only after fail reads stderr. The reported diagnosis can remain empty or truncated.
Prompt for agents
In scripts/ci/contextual_orchestrator_review_sidecar.sh, the health-check timeout branch reads sidecar_stderr while the sidecar and sanitizer can still be writing. The EXIT trap kills the sidecar and drains both sanitizers only after fail has formatted the message. Rework this branch so it terminates and waits for the sidecar, drains the sanitizer processes, then reads the sanitized stderr. Preserve the existing cleanup behavior and avoid waiting on sanitizers while the sidecar still owns the pipe write ends.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
299
to
+309
| "${zdr_args[@]}" \ | ||
| "${privacy_args[@]}" \ | ||
| "${pool_args[@]}" \ | ||
| > >("$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stdout") \ | ||
| 2> >("$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stderr") & | ||
| >&"$orchestrator_stdout_fd" 2>&"$orchestrator_stderr_fd" & | ||
| sidecar_pid=$! | ||
| # Close our own copies of the write ends now that the sidecar process holds | ||
| # its own duplicated fds. If these stayed open in this shell, the sanitizer | ||
| # process substitutions would never see EOF (and never exit) once the sidecar | ||
| # itself closes its fds, since a process substitution's reader only finishes | ||
| # after every writer has closed. | ||
| exec {orchestrator_stdout_fd}>&- {orchestrator_stderr_fd}>&- |
Contributor
There was a problem hiding this comment.
📝 Info: Named sanitizer processes drain correctly
Each immediate $! captures its sanitizer. Closing the parent write descriptors permits EOF, so the early-exit drain completes before stderr is read.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
17
to
33
| ( | ||
| "review sidecar discovered no zero-cost models;", | ||
| "review sidecar discovered no zero-cost models", | ||
| # Matches contextual_orchestrator_review_launcher.py's actual | ||
| # SystemExit text ("no eligible models", not "no zero-cost models" -- | ||
| # that stale prefix never matched the launcher's real message, so | ||
| # this fail-closed diagnostic was silently dropped to | ||
| # omitted_unstructured_lines instead of reaching CI operators). | ||
| "review sidecar discovered no eligible models;", | ||
| "review sidecar discovered no eligible models", | ||
| ), | ||
| ( | ||
| "review sidecar requires an explicit --auth-token or the KV credential", | ||
| "review sidecar auth token unavailable", | ||
| ), | ||
| ( | ||
| "review sidecar requires at least one provider credential in the KV", | ||
| "review sidecar requires at least one provider credential in the KV", | ||
| ), |
Contributor
There was a problem hiding this comment.
📝 Info: Summaries match current launcher failures
The prefixes match all three launcher messages. Fixed summaries omit trailing details, while unrelated provider output still reaches the omission counter.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added a commit
that referenced
this pull request
Aug 30, 2026
…view sidecar (#1426) * fix(ci): surface silently-dropped provider discovery errors in the review sidecar Prompted by a direct question about why a reproduction of the orchestrator/free pool-exhaustion incident only ever showed 3 of 5 configured providers (openrouter, nvidia_nim, nvidia_nim_sub) and never bytez/openai. Root cause: contextual_orchestrator_review_launcher.py's main() called `discovered, _ = discover_all_models()`, discarding the second tuple element. discover_all_models() already isolates and returns each provider's failure as a bounded, secret-free ProviderDiscoveryError (provider_name + a stable error_code like http_status_401/timeout/transport_error/invalid_response) -- the launcher just never looked at it, so an operator could not tell "this provider has zero free models" from "this provider's discovery silently failed", which is exactly what made the earlier reproduction inconclusive. Fix: add _log_discovery_errors(), called right after discover_all_models(), printing one `provider_discovery_failed provider=<name> code=<code>` line per error to stderr (non-fatal). Extend the sidecar's stream sanitizer with a matching bounded allowlist regex so this reaches CI evidence instead of falling into omitted_unstructured_lines=N -- the same class of gap #1425 closed for the fail-closed exit message. Co-Authored-By: Claude <noreply@anthropic.com> * fix(ci): surface discovery warnings on a successful sidecar startup too Devin's review on this PR caught a real gap in the fix itself: the new per-provider discovery-error diagnostic only ever reaches the failure-path ::error:: message. When another provider keeps startup successful (which is the common case once the free-pool exhaustion is fixed), $sidecar_stderr is never read again -- publish_sidecar_evidence only copies discovery/catalog/ policy JSON, never the raw stdout/stderr logs, and only strix.yml uploads an artifact at all (opencode-review.yml/noema-review.yml upload nothing). A partial provider failure would stay completely invisible. Fix: after healthz confirms, print a bounded (20-line) snapshot of $sidecar_stderr into the always-visible job log if it has any content. Cannot `wait_for_sidecar_sanitizers` here -- the sidecar keeps serving after a successful healthz, so its sanitizer never sees EOF and waiting would hang the workflow forever; this is a best-effort snapshot, not a guaranteed- complete read, which is the best available without adding IPC between the discovery phase and the main script. Co-Authored-By: Claude <noreply@anthropic.com> * fix(ci): close the slow-sanitizer race on successful-startup warnings Devin's follow-up review caught a real race in the previous commit's fix: `[ -s "$sidecar_stderr" ]` checked the file size immediately after healthz confirmed, with no guarantee the async sanitizer subprocess had actually flushed the discovery-error lines through yet. Under a slow sanitizer this would silently show nothing even when real warnings existed. Fix: the launcher's _log_discovery_errors() now always emits a trailing "discovery_diagnostics_complete" sentinel as its last stderr line (even with zero errors). Since the sanitizer processes lines strictly in order, once that sentinel has passed through, every earlier discovery-error line is guaranteed to have reached $sidecar_stderr too. The sidecar script polls for the sentinel (bounded, ~5s) instead of guessing from file size or a fixed sleep -- deterministic rather than probabilistic. Reproduced both the race and the fix directly: a minimal standalone harness with a deliberately slow (150ms/line) sanitizer lost data under the old size-check pattern; the sentinel-poll pattern captured it completely, 3/3 runs. Also caught and fixed a real bug the same reproduction surfaced: `grep -v` exits 1 when everything is filtered out (the common, healthy zero-warnings case), which under `set -o pipefail` would have aborted the whole script on every clean startup -- added the missing `|| true`. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Aug 30, 2026
seonghobae
added a commit
that referenced
this pull request
Aug 30, 2026
….dev 403 fix (#1430) Bumps ORCHESTRATOR_PIN_SHA from 5f2753a (#1422 pin) to 30c6d716 (contextual-orchestrator main, carrying #919's Models.dev User-Agent/403 fix and its ADR-0032 generalization to nvidia_nim/nvidia_nim_sub/openai). Merged with admin bypass past opencode-review: per this repo's own pull_request_target trust boundary, this PR's review dispatch runs .github main's still-stale sidecar copy, so it cannot pass its own check until the bump it ships is itself on main -- the same chicken-and-egg as contextual-orchestrator#919 and the prior #1413/#1422/#1423/#1424/#1425. Full local suite: 1880 passed, 1 skipped.
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
Resolves the split-timeout/batched-preflight design (this branch) against main's independently-landed ADR-0005 diagnostic, bounded-retry preflight (#1449/#1452 and predecessors #1436/#1440/#1434/#1425/#1426/#1422/#1413/ #1401/#1442), a genuine semantic merge, not a mechanical pick: - Keeps this branch's core contribution: launcher.py splits startup-route admission (10s) from real serving (120s, via _build_model_client), and probes up to REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES (24) candidates in concurrent batches of REVIEW_PREFLIGHT_BATCH_SIZE (4). - Folds in main's ADR-0005 escalation logic (16-token base probe, escalate to the real serving budget on a "budget too small" signature, shared REVIEW_PREFLIGHT_MAX_ESCALATIONS=4 cap) into the per-candidate probe used by that batching, via a new _EscalationBudget class so the shared budget stays a hard, lock-enforced invariant across concurrently-probed candidates within one batch -- a plain int (correct for main's original sequential loop) cannot coordinate that safely under concurrency. - Folds in main's ADR-0005 bounded retry for the sidecar's own separate gateway smoke request (up to REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS=3 attempts, 120s each, retried only on no-response/transport failure), replacing this branch's single-attempt transport_timeout/transport_error classification, while keeping this branch's "orchestration":"route" field and the 413-body-limit stderr-message-capture test enhancement. - Keeps this branch's family_cap==total-route-budget default (24) over main's more conservative raise to 8, since batching's concurrency keeps worst-case wall time bounded even at the higher route/family count (recomputed and re-tested below). - Two designs were deliberately NOT combined, resolved in main's favor after tracing each side's intent (not guessed): Strix's orchestrator/auto routing, which this branch's history re-added but main's most recent, explicit owner decision (#1434) reverted to orchestrator/free-only with the accepted-risk rationale recorded in ADR-0003; and a "fail closed on any partial provider discovery error" gate this branch added ( _require_complete_discovery), which main never adopted and whose philosophy directly conflicts with main's demonstrated-in-production "log the failure, continue with whatever succeeded" handling -- live BandScope evidence in this PR's own comments shows single-provider hiccups (Bytez 5xx) are common and should not be fatal to the whole pool. - Updates the two main-side tests whose literal worst-case-time assertions were computed for the old sequential (non-batched) design (test_preflight_stage_limits_share_one_startup_budget, test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case) to the batching-aware arithmetic, and drops this branch's now-superseded tests (orchestrator/auto Strix routing, fail-closed-on-partial-discovery, single-attempt gateway transport classification). Verification: coverage run -m pytest tests (2096 passed, 1 pre-existing skip, 21 subtests, 100% coverage on scripts/ci); interrogate (100% docstrings); git diff --check clean; bash -n on both changed shell scripts; ruff check clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
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
2 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
PR #1423's
noema-reviewcheck (head954d57b46fd8896ba0fb572a4fc662aa6a684c0a) is the first hosted run against #1422's freshly-bumpedORCHESTRATOR_PIN_SHA(5f2753ace756ddd81049a5221d55e8977572a416), and it failed with a new signature:omitted_unstructured_lines=1isscripts/ci/sanitize_contextual_orchestrator_sidecar_stream.pyreporting that it dropped one line of raw stderr because nothing in its_PREFIX_SUMMARIESallowlist matched it — hiding the actual cause from the log.Root cause (verified by live, end-to-end local reproduction)
I cloned
contextual-orchestratorat the exact new pin, installed itsrequirements.lock, registered the five CI provider secrets (fake-but-present values), and randiscover_all_models()for real over the network, then ranscripts/ci/contextual_orchestrator_review_launcher.pydirectly:That's an exact reproduction of the hosted failure. The actual chain:
contextual-orchestratorcommit952996ec("fix(discovery): keep OpenRouter catalog evidence-only"), part of the 103 commits fix(sidecar): refresh stale contextual-orchestrator review pin #1422's pin bump picked up, deliberately setsopenrouter'sProviderModelSource.evidence_only=True(previouslyFalse). This is a correct, intentional ZDR-privacy hardening — OpenRouter proxies to many third-party backends with varying retention policies, so it may only supply per-model ZDR evidence now, never serve requests directly. It should not be reverted.openrouterwas, and always had been, the only one of the sidecar's five credentialed providers whose discovery response carries genuine per-model pricing. NVIDIA NIM's real/v1/models(confirmed via a live unauthenticated probe in this session) returns only{id, object, created, owned_by}— no pricing field at all — and OpenAI/Bytez never publish pricing via their list-models APIs either (Bytez's own code comment says as much)..github's owntests/test_contextual_orchestrator_review_live_discovery_contract.pyalready encoded this ascost_evidence == "unknown"for the other four providers — this was a known, pre-existing structural dependency on OpenRouter, not a new assumption.openrouternowevidence_only, the launcher's_routable_discovered_models()drops all 540 OpenRouter rows before free-pool selection runs, soselected_modelsis empty andmain()fails closed withSystemExit("review sidecar discovered no eligible models; orchestrator/free would fail closed")— exit 1, beforeserve(), hence before/healthz. This is deterministic and structural: every futurenoema-reviewrun with this exact five-secret set will fail identically, org-wide, not just on docs: record 2026-08-30 hourly loop recheck in gap baseline #1423.What this PR fixes vs. what it deliberately leaves open
Fixed here (safe, no policy tradeoff):
_PREFIX_SUMMARIESinsanitize_contextual_orchestrator_sidecar_stream.pystill matched the launcher's old wording ("no zero-cost models"), not its current "no eligible models" text, and had no entry at all for the missing-auth-token / missing-provider-credential fail-closed messages either. All three fell straight through toomitted_unstructured_lines=N— the redaction was hiding a real, non-secret diagnostic, not protecting a secret. Fixed all three prefixes/summaries and the matching pinned assertions intests/test_contextual_orchestrator_review_runtime_preflight.py.Deliberately NOT fixed here — needs a human/product decision: restoring a non-empty
orchestrator/freepool. Two candidate paths, neither implemented or authorized in this PR:CONTEXTUAL_ORCHESTRATOR_POOLatauto(already fully implemented as a priced fallback in the launcher) — trades away the org's "fail-closed zero-cost" review guarantee for every PR, org-wide. Budget-owner call.contextual_orchestrator'sopencode_zenprovider, which honestly computesis_freevia a real Models.dev cross-reference rather than a self-reported flag, using theOPENCODE_ZEN_API_KEYsecret that already exists (currently used only byopencode-review.yml's separate OpenCode Zen config, never passed to this sidecar). This also needs a newscripts/ci/zdr_policy.pyPROVIDER_ZDR_SCOPE["opencode_zen"]attestation entry (that tableKeyErrors on an unknown provider by design — skipping it would crash every ZDR-required, i.e. private/internal-repo, review instead of just today's public-repo failure), plus live verification with a real key that its free models are general-chat/tool-call-capable and pass the sidecar's runtime preflight. None of that is verifiable without provisioning real credentials, so it's left open.Full writeup, including the live discovery numbers above, is appended to
docs/product-technical-gap-baseline.mdunder## 2026-08-30 orchestrator/free pool exhausted by upstream ZDR hardening(append-only; no existing dated entry edited).No change to
contextual-orchestratoror its pin. The upstream commit that triggered this is correct and should stay; there is nothing to fix there.Validation
PYTHONPATH=. python3 -m pytest tests -q— 1875 passed, 1 skipped, 25 subtests passedcoverage run -m pytest tests -q && coverage report --show-missing— the two changed Python files are both 100%; the repo-wide 99% total is the pre-existing, already-trackedscripts/ci/pingora_edge_policy.py:274gap owned by fix(coverage): trust validated Python head locks #1398, not introduced by this changeinterrogate— 100.0%bash -n scripts/ci/contextual_orchestrator_review_sidecar.sh(unchanged by this PR) andgit diff --check— cleancontextual_orchestrator_review_sidecar/launcher/sanitize/evidence_only/orchestrator/free— fix(strix): preserve auto contextual-orchestrator route #1413 (merged) and fix(noema): batch sidecar route preflight #1415 (open) address an unrelated Strix routing/timeout-budget issue; no open PR duplicates this fixShould PR #1423 get a fresh push?
Yes, once this merges. #1423 is a doc-only gap-baseline PR currently blocked by this exact
noema-reviewfailure (head954d57b46); mergingmaininto it after this lands will pick up the sanitizer fix (so the next run shows the real diagnostic instead ofomitted_unstructured_lines=1) but will not by itself fixnoema-review, since the underlying free-pool exhaustion is still open pending the product decision above.Generated by Claude Code