Revert "fix(strix): disable SDK streaming for the contextual-orchestrator gateway" - #1463
Conversation
…ator gateway (#1448)" This reverts commit 702392a. contextual-orchestrator#925 (merged) fixes the actual root cause #1448 routed around: the gateway's server.py unconditionally rejected stream_options.include_usage=true combined with tools before any upstream call. That rejection is now narrowed to only the genuinely unmeasured response_format-only (conduct mode) case; single-agent tools passthrough -- exactly Strix's request shape -- succeeds with real, honestly-labeled usage (usage_source: "reported" when the provider returns it, "estimated" otherwise, never fabricated). #1448 was always documented as a temporary workaround, not the fix (see its own PR body and contextual-orchestrator's docs/product-technical-gap-baseline.md), trading away Strix's real-time SSE streaming to route around a bug that no longer exists. With #925 live, Strix's tools + stream_options.include_usage=true requests against orchestrator/free succeed on their own -- this workaround is now unnecessary and reverted to restore real streaming.
|
Warning Review limit reachedNext included review available in 36 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 (1)
📝 WalkthroughWalkthroughStrix의 contextual-orchestrator 스트리밍 비활성화 경로를 제거했습니다. 게이트웨이 핀 참조를 갱신했습니다. 자체 테스트에서 ChangesStrix 스트리밍 경로
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The PR resumes streaming for Strix, but the sidecar may still use an older gateway revision that rejects the request shape, causing scans or required checks to fail; the related streaming regression checks are also weakened. Synchronize the pinned revision and retain explicit assertions before merging. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 1 unsupported, 1 too large.) ✨ 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.
…ols fix Devin Review correctly caught that this revert branch's sidecar still vendored contextual-orchestrator at 30c6d716..., a commit predating contextual-orchestrator#925's merge (7944a3c) -- confirmed via git merge-base --is-ancestor. Removing the Strix-side LLM_DISABLE_STREAMING workaround while the vendored gateway still ran the old rejecting code would have restored the exact failure #1448 existed to route around. Bumps the pin to 7944a3c (the #925 merge commit itself, not contextual-orchestrator's later tip) in the three places this repo's convention keeps in sync: the sidecar script's default, the pinned-SHA contract test assertion, and ADR-0003's "today" reference. Recorded in docs/product-technical-gap-baseline.md.
|
Confirmed and fixed, pushed ( Bumped On the suggested runtime contract proving a live tools+stream_options request against the exact vendored revision: reasonable in principle, but out of scope here — this repo's existing pin-bump pattern (see the Verification: Generated by Claude Code Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
@opencode-agent review Please publish a substantive formal verdict for exact current head |
|
Status update for Strix actually succeeded on this PR's previous head ( The first Strix attempt on this new head was cancelled almost immediately (~24s, before any real work — an infra hiccup); I re-ran it and it made real progress (~12 minutes) before being cancelled again, cause not yet clear from the API. Given the prior head's full success on materially the same code, re-running once more now. Generated by Claude Code |
|
Cross-PR model-group correction: #1468 pins the shared Noema/Strix/OpenCode sidecar to contextual-orchestrator 0adca470 and removes the sidecar-local provider-family collapse. Each of BYTEZ_API_KEY, NVIDIA_NIM_API_KEY, NVIDIA_NIM_API_KEY_SUB, OPENROUTER_API_KEY, and OPENAI_API_KEY is an independent credential account; vendor/endpoint identity is not model equivalence. Only an explicit model_group in contextual-orchestrator may share routing evidence. Please rebase/stack on #1468 or treat its sidecar-policy portion as superseding overlapping family_cap/pin assumptions. |
…streaming # Conflicts: # docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md # scripts/ci/contextual_orchestrator_review_sidecar.sh # tests/test_contextual_orchestrator_review_sidecar_contract.py
|
Confirmed both parts of your diagnosis and merged main to pick up the fixes — both landed directly on
Full validation on the merged head: Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…n guards exact-head-path-policy was failing on this PR's head with "opencode required workflow bootstrap must not depend on required-workflow event payload fields" -- reproduced against unmodified main and confirmed unrelated to this PR's diff. Root cause and fix already exist in open PR #1506: assert_opencode_review_uses_codegraph_and_contextual_orchestrator extracted the required-workflow-bootstrap job block with an awk range pattern (/^ required-workflow-bootstrap:$/,/^[^ ]/) whose end pattern never matches (every job key is indented 2 spaces, never column 0), so it swept an unrelated if: from the separate opencode-review-target job (added legitimately by already-merged #1497) into the wrong job's block. Ported #1506's fix by hand: a state-flag-bounded awk extraction, a large-block regression test proving the bound survives a pipe-buffer- sized job block, and the same grep -Fq -> grep -F fix for a second, independently SIGPIPE-vulnerable assertion. Deliberately did not port #1506's LLM_DISABLE_STREAMING test content, since that would reinstate assertions for the exact workaround this PR reverts. Also addresses a real CodeRabbit finding on this PR's own revert commit: removing the LLM_DISABLE_STREAMING fake-log field/assertions left no regression guard against the reverted workaround silently reappearing in scripts/ci/strix_quick_gate.sh. Added assert_file_not_contains checks for both removed code paths (STRIX_CHILD_DISABLE_STREAMING threading and the child_env LLM_DISABLE_STREAMING assignment). test_strix_quick_gate.sh: PASS. Full suite: 2126 passed, 1 skipped, 21 subtests. Coverage and docstrings: 100%.
|
Verified: Generated by Claude Code |
|
Generated by Claude Code |
|
Re-running once now per this repo's CI-red protocol, since this is exactly the transient-infra class that classification exists for. Generated by Claude Code |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
…revert Main moved again mid-rebase: .github#1463 reverted #1448's LLM_DISABLE_STREAMING workaround (which the previous commit on this branch had just cited as the fix) now that the real root cause is fixed upstream -- contextual-orchestrator#925 (merge commit 7944a3c, "accept stream_options.include_usage=true for tools passthrough") makes the gateway itself stop rejecting the combination, and #1463 bumped ORCHESTRATOR_PIN_SHA to 7944a3c so the vendored sidecar actually carries that fix (verified: `git merge-base --is-ancestor 7944a3c <current pin>` is true). The conclusion is unchanged (the gap is fixed) but the mechanism is not what the previous commit said, so corrects both the gap-baseline entry and this PR's own CHANGELOG bullet to point at the current, real fix instead of the since-reverted workaround. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
Why
Reverts
#1448(702392a), a deliberate, documented temporary workaround for a bug that is now fixed at the source.#1448routed aroundcontextual-orchestrator's gateway unconditionally rejectingstream_options.include_usage=truecombined withtools— the exact shape Strix's always-streaming, always-tools agent loop sends on every turn — by disabling Strix's own SDK streaming (LLM_DISABLE_STREAMING=true) whenever it talks to that gateway. That traded away real-time SSE output for Strix scans to unblock the org's required Strix gate (which was itself transitively blocking every PR'sopencode-review, since that check requires completed Strix evidence before it will dispatch).ContextualWisdomLab/contextual-orchestrator#925(merged) fixes the actual root cause: the gateway's rejection is now narrowed to only the genuinely-unmeasuredresponse_format-only (conduct mode) case. Single-agenttoolspassthrough — exactly Strix's request shape — now succeeds normally, with honest usage labeling (usage_source: "reported"when the provider returns it,"estimated"otherwise, never fabricated as"reported"). Both#1448's own PR body andcontextual-orchestrator'sdocs/product-technical-gap-baseline.mdexplicitly tracked this as a workaround to revert once #925 landed — this PR is that follow-up.Effect
Strix scans through the
contextual-orchestratorgateway resume real SSE streaming instead of the single-non-streaming-call fallback. No other provider's behavior is affected —#1448's opt-in was already scoped narrowly to thecontextual-orchestratorloopback, so reverting it only removes that one narrow branch; every other provider in Strix's fallback chain was never touched.Verification
git revert --no-edit 702392a2fb80149d25734a4cae173ed5fecd6d83: clean, no conflicts.bash -non both touched scripts: clean.bash scripts/ci/test_strix_quick_gate.sh:test_strix_quick_gate: PASS(full suite, including the reverted assertions' own now-absent counterparts).PYTHONPATH=. coverage run -m pytest tests -q: 2023 passed, 1 skipped, 21 subtests passed.coverage report --show-missing:TOTAL 10250/10250 statements, 4084/4084 branches, 100%.interrogate .:PASSED (minimum: 100.0%, actual: 100.0%).Generated by Claude Code
Summary by CodeRabbit
변경 사항
테스트