Skip to content

fix(sidecar): let the stream sanitizer pass orchestrator route and circuit events - #1945

Merged
seonghobae merged 3 commits into
mainfrom
fix/sidecar-sanitizer-admit-orchestrator-route-events
Sep 5, 2026
Merged

fix(sidecar): let the stream sanitizer pass orchestrator route and circuit events#1945
seonghobae merged 3 commits into
mainfrom
fix/sidecar-sanitizer-admit-orchestrator-route-events

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Third piece of the observability lane behind #1935 / #1939: #1943 turns on DEBUG logging in the sidecar process, #1944 ships the sanitized stderr when a Noema verdict fails, and this PR makes the sanitizer let the orchestrator's route and circuit events through. Any order works: without this PR the artifact only grows omitted_unstructured_lines; without #1943 only the WARNING-level lines (provider_exhausted and its two siblings, which already fire today) appear.

What changes

scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py

  • _LOG_PREFIX: strips either Python's default LEVEL:name: prefix or the fix(sidecar): record the orchestrator's per-attempt trace in the review sidecar's stderr #1943 formatter's asctime LEVEL name prefix; the timestamp (digits and punctuation only) is kept in front of the summary so per-route durations can be read as differences between lines.
  • _ORCHESTRATOR_EVENTS: ten anchored patterns copied from contextual_orchestrator/orchestrator.py at the vendored pin 2e414d15 (:1337 provider_attempt, :1351 provider_attempt_failed, :1365 provider_backoff, :1389 provider_exhausted, :1458 provider_rejected_permanent, :1397 provider_no_retry_budget, :7988/:8003/:8010/:8021 circuit_reset|failure|opened|cleared). Every field is matched against a bounded charset (agent_id [a-z][a-z0-9_]*, model [A-Za-z0-9_./:-]+, error types [A-Za-z_][A-Za-z0-9_.]*, numbers \d+(\.\d+)?); provider_attempt_failed is cut before its free-text error_message= and re-emitted with error_message=<omitted>.
  • _sanitize_orchestrator_event() + one hook in sanitize_line() after the existing structured patterns and before the fixed prefixes. Nothing else in the allowlist changes.

tests/test_contextual_orchestrator_review_runtime_preflight.py: test_sidecar_stream_sanitizer_admits_orchestrator_route_events — all ten templates, both prefixes, timestamp retention, the error_message cut with a planted secret asserted absent, and four rejections (uppercase agent id, trailing free text after a complete template, a failed-attempt line without the error_message boundary, a known prefix with an unknown message).

CHANGELOG.md entry.

Why

Every one of these lines was being folded into omitted_unstructured_lines. That includes provider_exhausted, which is a WARNING and therefore already in today's stderr: a route that burns its full retry budget (≈ 548 s at the current pin: orchestrator retry × client retry × 90 s per-recv timeout) leaves no trace in any artifact. Run 33981136873 walked six ready routes for 3122 s and ended in HTTP 502; the only diagnosis was the caller's one-line summary. With #1943 + #1944 + this, the failed-run artifact reads as a per-route timeline.

Not claimed

  • No new log lines are added anywhere; the orchestrator pin is untouched. A richer single-line route_attempt … outcome= duration_ms= event stays a later CO improvement.
  • Strix already ships the same file; it benefits from this change without any workflow edit.

Verification

Full gate on aae077ac with GITHUB_ACTIONS=true: 2897 passed / 1 skipped, coverage report --fail-under=100 → 100% (the sanitizer itself: 67 statements, 32 branches, 0 missed), interrogate → 100%; the preflight/sanitizer suite (62 tests) passes; negative control: with main's sanitizer swapped in the new test fails (1 failed), restored it passes.

🤖 Generated with Claude Code

…rcuit events

Every provider_*/circuit_* line from the orchestrator was folded into
omitted_unstructured_lines, so even the provider_exhausted WARNING that
fires today never reached an artifact. Admit those templates field by
field against bounded charsets, cut provider_attempt_failed before its
free-text error_message, and accept both the default and the sidecar
formatter log prefixes (keeping the timestamp for durations).

Companion to #1943 and #1944. Refs #1935, #1939

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: a2201b93-744c-442c-b85b-c521b316a691

📥 Commits

Reviewing files that changed from the base of the PR and between d9eb9f7 and 560e17d.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py
  • tests/test_contextual_orchestrator_review_runtime_preflight.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Integration check (host 1): I formatted real LogRecords for all ten templates with #1943's SIDECAR_LOG_FORMAT and with Python's default %(levelname)s:%(name)s:%(message)s, and fed them through this head's sanitize_line (aae077ac). Eight of ten pass in both prefix forms, the timestamp is preserved, a planted Authorization: Bearer … inside error_message= is cut, and the four adversarial lines (uppercase agent id, trailing free text after a complete template, provider_attempt_failed without an error_message= boundary, known prefix + unknown message) are all dropped. Two templates are dropped when they should pass:

circuit_failure agent_id=nvidia_nim_a_b failures=2.0 threshold=3                       -> None
circuit_opened  agent_id=nvidia_nim_a_b failures=3.0 threshold=3 reset_seconds=30.0    -> None

Cause: the orchestrator's circuit counter is a float. self._circuit.setdefault(agent_id, {"failures": 0.0, "opened_at": 0.0}) (orchestrator.py:7995) and state["failures"] += 1.0 (:7996), so %s renders 2.0/3.0, and circuit_opened also prints reset_seconds=%s from circuit_reset_seconds = 30.0 (:3959) → 30.0. The event regexes here expect an integer for those fields:

47:        rf"^circuit_failure agent_id={_AGENT_ID} failures=\d+ threshold=\d+$",
48:        rf"^circuit_opened agent_id={_AGENT_ID} failures=\d+ threshold=\d+ reset_seconds={_NUMBER}$",

Fix is one character class: failures=\d+(?:\.\d+)? and reset_seconds=\d+(?:\.\d+)? (threshold=\d+ is a real int, circuit_failure_threshold = 3 at :3958). The tests in this PR use integer fixtures for these two events, which is why they stayed green — worth switching the fixtures to failures=2.0 / reset_seconds=30.0 so the contract pins the shape the orchestrator actually emits. Everything else in the allow-list matched the real formatter output byte-for-byte; no change needed on the #1943 side.

…r output

The orchestrator's circuit counters are floats (failures 0.0 += 1.0,
circuit_reset_seconds 30.0), so the lines that reach stderr say
failures=2.0 / reset_seconds=30.0; the integer-only pattern rejected both
circuit_failure and circuit_opened. Found by rendering the templates through
the sidecar's logging.Formatter, which the new test now does for all ten.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@seonghobae

Copy link
Copy Markdown
Contributor Author

Fixed in 0840bdac — the circuit counters are floats at the pin (orchestrator.py:7995 "failures": 0.0, :7996 += 1.0, :3959 circuit_reset_seconds = 30.0), so the real lines are circuit_failure … failures=2.0 threshold=3 and circuit_opened … failures=3.0 threshold=3 reset_seconds=30.0; failures= now takes the number charset (threshold stays an int). The hand-typed fixtures were the defect: they were written from the %s templates, not from what the formatter emits. test_sidecar_stream_sanitizer_matches_real_formatter_output now renders all ten templates through logging.Formatter("%(asctime)s %(levelname)s %(name)s %(message)s") with runtime value types and requires every one to pass; against the previous head's sanitizer it fails (1 failed), which is the regression guard for exactly this class. Gate on 0840bdac: 2898 passed / 1 skipped, coverage 100%, interrogate 100%; the preflight/sanitizer suite is 63 tests.

@seonghobae
seonghobae merged commit 972b74b into main Sep 5, 2026
3 of 15 checks passed
@seonghobae
seonghobae deleted the fix/sidecar-sanitizer-admit-orchestrator-route-events branch September 5, 2026 22:09
@seonghobae

Copy link
Copy Markdown
Contributor Author

Merged as 972b74be (squash, bypass over REST) — author session peer1, merger a separate session.

Verified by the merging session's own run, not relayed. Because main had moved since this PR's head was verified, the merged tree was gated, not just the head: git merge-tree of d9eb9f79 × 560e17d3 — no conflicts, no markers; vs its base: CHANGELOG, scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py (+58), its test module (+112) — the test module is also touched by #1943, hence the merged-tree gate; head 560e17d3 is the author's merge of main@d9eb9f79 with the changelog entry placed above #1943's; full gate on that merged tree: 2903 passed, 1 skipped, 21 subtests passed in 113.86s (0:01:53); coverage 100%; RESULT: PASSED (minimum: 100.0%, actual: 100.0%). After the squash, main's tree 8350b20d5c28 is byte-identical to the gated merge tree 8350b20d5c28. Negative control on the head: on 0840bdac (sanitizer and tests byte-identical to this head): with main's sanitizer and with the previous head aae077ac's (\\d+ counters) exactly the two float-fixture tests fail — …admits_orchestrator_route_events and …matches_real_formatter_output; with the PR's sanitizer all 63 pass. The stderr sanitizer's allowlist now admits ten fully-anchored orchestrator templates (provider_attempt, provider_attempt_failed, provider_backoff, provider_exhausted, provider_rejected_permanent, circuit_*) under charset limits, with an optional asctime prefix and both LEVEL:name:/LEVEL name forms; numbers match \\d+(?:\\.\\d+)? so the orchestrator's float counters (failures=2.0, reset_seconds=30.0) pass; test_sidecar_stream_sanitizer_matches_real_formatter_output renders all ten through a real logging.Formatter.

Authorization, cited at the act from the standing directive: "60-job ceiling에 막혀 있을 거라 일을 하기 어렵다면 60-job ceiling을 만드는 workflow issues를 추적해서 해소하세요. 이 상황은 Chicken-and-eggs 상황이므로 Bypass merge가 허용됩니다." This PR is the tracing half of that grant — it makes the ≈548 s-per-candidate slot burn behind today's noema/strix failures readable as a per-route timeline in the run artifact — and it structurally cannot validate itself (its own required run executes main's copy of the file it changes). Review-pipeline files under scripts/ci/ or .github/workflows/ only; no pinned or policy file; not dirty. Of the day's bypasses this is the weakest case (instrumentation, not a fix), stated as such.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Precision on the negative-control sentence above: between the verified head 0840bdac and the merged head 560e17d3, scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py is byte-identical (so the control's conclusion stands), but the test module is not — it gained #1943's five sidecar-logging tests through the main merge (+63 lines). "Sanitizer and tests byte-identical" should read "sanitizer byte-identical; this PR's own tests unchanged; the module additionally carries #1943's tests". The merged-tree gate (2903 passed) covered the combined file.

seonghobae added a commit that referenced this pull request Sep 5, 2026
…integration

Conflict: tests/test_noema_orchestrator_workflow_contract.py -- both sides
appended a test at end of file (this branch: the cancel-superseded-noema-runs
job rationale pin; main: #1944's sidecar-evidence upload step contract).
Kept both. noema-review.yml auto-merged.

Gate after merge: 2941 passed, coverage 100%, interrogate 100%.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@seonghobae

Copy link
Copy Markdown
Contributor Author

Verified in production: the first three post-fe827e13 Noema failures (runs 33997442892, 33997237253, 33996977334) each shipped noema-sidecar-evidence with a per-route timeline — 12 agent ids, timestamps, zero omitted_unstructured_lines. Three failures turned out to have three different causes (gateway walk with permanent rejections of two OpenRouter free models; a verdict failing local schema validation; the gh pr diff 300-file limit on #1555). Table on #1935.

seonghobae pushed a commit that referenced this pull request Sep 6, 2026
… size contract)

Signature 3: the first post-#1939 noema-review runs split 1/1; the
failing run's policy report shows the diversified pool #1939 promised
and still ends in a 502 after a ~548 s-per-route walk (host 1's
arithmetic from source), so a base-merge push recovers pre-#1939 heads
but does not shorten the post-#1939 walk; #1943/#1944/#1945 make the
per-route timeline readable from the noema-sidecar-evidence artifact;
the remaining lever is inside contextual-orchestrator.

Signature 12: required-workflow-bootstrap exit 2 in ~5 s on
"exceeds the size contract" -- the Contents API's 1 MiB inline ceiling
on a patchless text file, fixed by #1946's Git Blobs API route.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae pushed a commit that referenced this pull request Sep 6, 2026
Binds the current review workflows (#1943/#1944/#1945: sidecar trace and
noema-sidecar-evidence artifact on failure) to this head's required runs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae pushed a commit that referenced this pull request Sep 6, 2026
Binds the sidecar pin bump to contextual-orchestrator@414f2297
(contextual-orchestrator#1081's retry-stacking fix, .github efb8926) and the
#1943/#1944/#1945 workflows to this head's required runs; the previous head's
Noema/Strix runs failed on the stalled-route loop that fix removes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae pushed a commit that referenced this pull request Sep 6, 2026
Binds the sidecar pin bump to contextual-orchestrator@414f2297
(contextual-orchestrator#1081's retry-stacking fix, .github efb8926) and the
#1943/#1944/#1945 workflows to this head's required runs; the previous head's
Noema/Strix runs failed on the stalled-route loop that fix removes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae pushed a commit that referenced this pull request Sep 6, 2026
…letion

Binds the sidecar pin bump to contextual-orchestrator@414f2297
(contextual-orchestrator#1081's retry-stacking fix, .github efb8926) and the
#1943/#1944/#1945 workflows to this head's required runs; the previous head's
Noema/Strix runs failed on the stalled-route loop that fix removes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae pushed a commit that referenced this pull request Sep 6, 2026
…ence

CHANGELOG.md: both new top entries kept, main's pin-bump entry first.

Binds the sidecar pin bump to contextual-orchestrator@414f2297
(contextual-orchestrator#1081's retry-stacking fix, .github efb8926) and the
#1943/#1944/#1945 workflows to this head's required runs; the previous head's
Noema/Strix runs failed on the stalled-route loop that fix removes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant