From 5bae5a3d84aceb6e54ee71bb2839a2c802940278 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 06:55:46 +0000 Subject: [PATCH 1/2] fix(noema): surface real sidecar fail-closed diagnostics past redaction 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 --- docs/product-technical-gap-baseline.md | 101 ++++++++++++++++++ ..._contextual_orchestrator_sidecar_stream.py | 17 ++- ...l_orchestrator_review_runtime_preflight.py | 11 +- 3 files changed, 125 insertions(+), 4 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 010a025ace..08a8cd693b 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -455,6 +455,107 @@ flowchart LR way this pass found #1417's, and consider merging `main` into #1394's head to get it off its stale base. +## 2026-08-30 orchestrator/free pool exhausted by upstream ZDR hardening + +- **Root cause (verified by live, end-to-end local reproduction, not log + inference).** After #1422 bumped `ORCHESTRATOR_PIN_SHA` to + `5f2753ace756ddd81049a5221d55e8977572a416`, the first hosted `noema-review` + run on the new pin (`.github` PR #1423, head + `954d57b46fd8896ba0fb572a4fc662aa6a684c0a`) failed with `sidecar exited + before healthz (status 1); stderr: omitted_unstructured_lines=1` — a new + failure signature, distinct from the stale-pin HTTP 502/413 class the + 2026-08-30 entry above describes. Between the old pin + (`b21645116b352967e50fc497b87eb745b9cc8c61`) and the new one, upstream + `contextual-orchestrator` commit `952996ec` ("fix(discovery): keep + OpenRouter catalog evidence-only") deliberately set + `ProviderModelSource(provider_name="openrouter", ...).evidence_only=True` + (previously `False`) — an intentional, ZDR-privacy-motivated hardening + (OpenRouter routes to many third-party backends with varying retention + policies, so it may no longer be used as a *serving* agent, only as a + source of per-model ZDR evidence for other providers' matching canonical + ids). This is a correct fix on the orchestrator side and must not be + reverted or weakened. +- The org's sidecar (`scripts/ci/contextual_orchestrator_review_launcher.py`) + builds the `orchestrator/free` pool only from `is_free=True` routes among + the five credentialed providers (`BYTEZ_API_KEY`, `NVIDIA_NIM_API_KEY`, + `NVIDIA_NIM_API_KEY_SUB`, `OPENROUTER_API_KEY`, `OPENAI_API_KEY`). + `openrouter` was, and had always been, the *only* one of those five whose + discovery response carries genuine per-model pricing (`contextual_orchestrator/model_discovery.py`'s `_parse_openai_compatible` reads `row["pricing"]`, present only in OpenRouter's `/v1/models` + response shape). NVIDIA NIM, OpenAI, and Bytez publish no pricing via their + list-models endpoints at all — confirmed by an unauthenticated live probe + of `https://integrate.api.nvidia.com/v1/models` in this session, which + returns only `{id, object, created, owned_by}` per model, and by + `contextual_orchestrator`'s own `_parse_bytez` docstring ("Bytez prices by + GPU-second ... leaving per-1k pricing unset is more honest than a + misleading estimate"). `.github`'s own + `tests/test_contextual_orchestrator_review_live_discovery_contract.py` + already encoded this as `cost_evidence == "unknown"` for openai/nvidia_nim/ + nvidia_nim_sub/bytez in its live-shape fixture — this was a known, + pre-existing structural dependency on OpenRouter for the free pool, not a + new assumption. With `openrouter` now `evidence_only`, the launcher's + `_routable_discovered_models()` filter drops all 540 OpenRouter rows before + the free-pool selection ever runs, so `selected_models` is empty and + `main()` raises `SystemExit("review sidecar discovered no eligible models; + orchestrator/free would fail closed")` — exit 1, before `serve()`, hence + before `/healthz`. +- **Live reproduction** (this session, real network calls, fake-but-present + values for the five secrets, pinned commit `5f2753ac…` installed from its + own `requirements.lock`): `discover_all_models()` returned 682 models — + `openrouter`: 540 total, 60 genuinely free, but 540/540 `evidence_only`; + `nvidia_nim` and `nvidia_nim_sub`: 71 each, 0 free; `openai`/`bytez`: + `http_status_401` (fake key, but note neither provider's list endpoint + carries pricing regardless of auth outcome). Routable (non-evidence-only) + free models: **0**. Running + `scripts/ci/contextual_orchestrator_review_launcher.py` directly end-to-end + reproduced the exact hosted signature: raw stderr + `review sidecar discovered no eligible models; orchestrator/free would + fail closed`, exit 1. This is deterministic and structural, not a + transient provider/network fluke — every future `noema-review` run with + this exact five-secret credential set will fail identically until the free + pool gets a real, non-OpenRouter zero-cost source, so this blocks PR review + org-wide, not just PR #1423. +- **Independent bug found and fixed in this pass (safe, no policy + tradeoff):** `scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py`'s + `_PREFIX_SUMMARIES` allowlist still matched the launcher's *old* wording + ("no zero-cost models"), not the current "no eligible models" text, and had + no entry at all for the launcher's missing-auth-token or + missing-provider-credential `SystemExit` messages. All three fell through + to `omitted_unstructured_lines=N`, which is exactly why PR #1423's hosted + log showed only `omitted_unstructured_lines=1` instead of the actionable + cause above — the redaction was hiding a real, non-secret diagnostic, not + protecting a secret. Fixed the three prefixes/summaries and the matching + pinned assertions in + `tests/test_contextual_orchestrator_review_runtime_preflight.py`; full + `.github` suite (1875 passed, 1 skipped, 25 subtests), `coverage report` + (the changed file itself is 100%; the pre-existing repo-wide 99% is the + already-tracked `scripts/ci/pingora_edge_policy.py:274` gap owned by + #1398, not introduced here), and `interrogate` (100.0%) all pass on this + change alone. +- **What is intentionally NOT fixed by this pass, and needs a product/human + decision, not a unilateral code change:** restoring a non-empty + `orchestrator/free` pool. Two candidate paths, neither exercised or + authorized here: (a) accept real provider spend by pointing + `CONTEXTUAL_ORCHESTRATOR_POOL` at `auto` (already fully implemented in the + launcher as a priced fallback) — this trades away the "fail-closed + zero-cost" guarantee `docs/CWL-MASTER-CONTEXT.md`/`CLAUDE.md` describe for + every PR review org-wide, a budget-owner call; or (b) wire in a genuine + zero-cost provider — `contextual_orchestrator`'s `opencode_zen` source + already cross-references real Models.dev pricing (not a self-reported + flag) to compute `is_free` honestly, and its credential + (`OPENCODE_ZEN_API_KEY`) already exists as an org secret (used today only + by `opencode-review.yml`'s separate OpenCode Zen GitHub Models config, not + passed to this sidecar) — but wiring it in also needs a new + `scripts/ci/zdr_policy.py` `PROVIDER_ZDR_SCOPE["opencode_zen"]` attestation + entry (that table currently `KeyError`s on an unknown provider name by + design, so skipping this would crash every ZDR-required — i.e. + private/internal-repo — review instead of just noema-review's current + public-repo failure) and live verification, with a real key, that + opencode.ai/zen's discovered free models are actually + general-chat/tool-call-capable and pass the sidecar's runtime preflight — + none of which this pass could validate without provisioning real + credentials. Neither option is a small, obviously-safe patch, so it is + left open here rather than forced. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py b/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py index be16c49bef..a5d837d4a9 100644 --- a/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py +++ b/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py @@ -15,8 +15,21 @@ ("review sidecar preflight failed:", "review sidecar preflight failed"), ("review sidecar discovery failed:", "review sidecar discovery failed"), ( - "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", ), ) diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index a0fbac1f4a..868fd0e160 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -326,8 +326,15 @@ def test_sidecar_stream_sanitizer_allowlists_only_bounded_diagnostics() -> None: "review sidecar discovery failed: https://provider.invalid/?key=sk-secret" ) == "review sidecar discovery failed" assert sanitize_line( - "review sidecar discovered no zero-cost models; orchestrator/free would fail closed" - ) == "review sidecar discovered no zero-cost models" + "review sidecar discovered no eligible models; orchestrator/free would fail closed" + ) == "review sidecar discovered no eligible models" + assert sanitize_line( + "review sidecar requires an explicit --auth-token or the KV credential " + "'CONTEXTUAL_ORCHESTRATOR_TOKEN'" + ) == "review sidecar auth token unavailable" + assert sanitize_line( + "review sidecar requires at least one provider credential in the KV" + ) == "review sidecar requires at least one provider credential in the KV" assert sanitize_line("provider response sk-secret") is None From b124b316db22e024342dcab674e6d69b0dd2069c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 07:07:28 +0000 Subject: [PATCH 2/2] fix(ci): close sidecar-diagnostics sanitizer race flagged by Devin Review 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 --- .../contextual_orchestrator_review_sidecar.sh | 31 +++++++++++++++++-- ...al_orchestrator_review_sidecar_contract.py | 20 ++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index 1fb2b5aea3..fb9aca21e5 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -271,6 +271,22 @@ log "starting review sidecar on ${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}" cp "$ORCHESTRATOR_LAUNCHER" "$ORCHESTRATOR_WORK/launch_sidecar.py" export ORCHESTRATOR_CATALOG_LIMIT="$CATALOG_LIMIT" export ORCHESTRATOR_CATALOG_FAMILY_CAP="$CATALOG_FAMILY_CAP" +# Stream stdout/stderr through the redacting sanitizer as two named, awaitable +# processes (not bare `> >(...)` substitutions, whose PIDs bash never exposes) +# so a failure handler can wait for the sanitizer to finish flushing before it +# reads the sanitized file — otherwise the read can race the still-draining +# pipe and silently show an empty/truncated diagnostic (the exact class of bug +# this sanitizer exists to avoid: see the 2026-08-30 sidecar-diagnostics gap +# baseline entry). +exec {orchestrator_stdout_fd}> >("$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stdout") +stdout_sanitizer_pid=$! +exec {orchestrator_stderr_fd}> >("$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stderr") +stderr_sanitizer_pid=$! +wait_for_sidecar_sanitizers() { + wait "$stdout_sanitizer_pid" 2>/dev/null || true + wait "$stderr_sanitizer_pid" 2>/dev/null || true +} + PYTHONPATH="$ORCHESTRATOR_SOURCE:$ORG_REPO_ROOT" \ CONTEXTUAL_ORCHESTRATOR_TOKEN="$ORCHESTRATOR_TOKEN" \ "$sidecar_python" "$ORCHESTRATOR_WORK/launch_sidecar.py" \ @@ -283,9 +299,14 @@ PYTHONPATH="$ORCHESTRATOR_SOURCE:$ORG_REPO_ROOT" \ "${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}>&- cleanup_sidecar_on_error() { status=$? if [ "$status" -ne 0 ]; then @@ -293,6 +314,7 @@ cleanup_sidecar_on_error() { log "stopping failed sidecar (pid $sidecar_pid)" kill "$sidecar_pid" 2>/dev/null || true wait "$sidecar_pid" 2>/dev/null || true + wait_for_sidecar_sanitizers fi } trap cleanup_sidecar_on_error EXIT @@ -302,6 +324,11 @@ until curl -fsSL --max-time 2 "http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/ if ! kill -0 "$sidecar_pid" 2>/dev/null; then sidecar_status=0 wait "$sidecar_pid" || sidecar_status=$? + # The sidecar has fully exited (confirmed above), so its stderr pipe has + # 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 fail "sidecar exited before healthz (status ${sidecar_status}); stderr: $(sed -n '1,20p' "$sidecar_stderr")" fi i=$((i + 1)) diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index 3ddf8952d2..2b29a508b8 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -401,6 +401,26 @@ def test_sidecar_trap_keeps_the_gateway_alive_after_provisioning() -> None: assert 'trap \'log "stopping sidecar (pid $sidecar_pid)"; kill "$sidecar_pid"' not in text +def test_sidecar_waits_for_sanitizer_drain_before_reading_failure_diagnostics() -> None: + """A bare `2> >(sanitizer)` races the failure-path read and can hide the diagnostic; the drain must close that race.""" + text = _read(SIDECAR) + assert "exec {orchestrator_stdout_fd}> >(" in text + assert "stdout_sanitizer_pid=$!" in text + assert "exec {orchestrator_stderr_fd}> >(" in text + assert "stderr_sanitizer_pid=$!" in text + assert "exec {orchestrator_stdout_fd}>&- {orchestrator_stderr_fd}>&-" in text + assert "wait_for_sidecar_sanitizers" in text + # The old bare, unwaited process-substitution redirection must be gone. + assert '> >("$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stdout") \\' not in text + assert '2> >("$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stderr") &' not in text + # The drain must happen strictly before the failure-path read, only in the + # branch where the sidecar has already exited (not the healthz-timeout + # branch, where it may still be running and draining would hang). + exited_branch = text.index("sidecar exited before healthz") + drain_call = text.rindex("wait_for_sidecar_sanitizers", 0, exited_branch) + assert drain_call < exited_branch + + def test_noema_review_workflow_provisions_sidecar_with_all_five_secrets() -> None: """Required Noema review uses the gateway; the public NIM hardcode is gone.""" workflow = _read(NOEMA_WORKFLOW)