diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 3c59089ee6..8ea7a81621 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -465,6 +465,106 @@ recurrence" section below out of the file entirely; both are restored here.) 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. ## 2026-08-30 sidecar pin staleness recurrence - Same class of defect as the 2026-08-29 entry above recurred within one day: 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/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 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)