Repository navigation
feat(combo): O360 explicit-target-routing V1 - #1
Conversation
PR/release hygiene pass — commit classification, cleanup, separate issue filedPer request, closing out three housekeeping items before this PR is considered mergeable (still NOT merging or deploying — that stays a separate, explicit decision). 1. Commit classification (verified via
|
…arding them Fixes #5 Root cause: createStreamFailureAborter (open-sse/utils/streamFailureBoundary.ts) and emitClaudeEmptyStreamErrorAndAbort (open-sse/utils/stream.ts) both enqueue a formatted, sanitized terminal SSE payload (a translated response.failed event, or a Claude event: error frame) and then immediately call controller.error(). Per the WHATWG Streams spec, ReadableStreamDefaultController.error() resets the stream's internal queue -- confirmed via direct TransformStream reproduction (enqueue+error: reader rejects, queued chunk never delivered; enqueue+terminate: reader resolves with the queued chunk, then EOF). So the just-enqueued payload was silently discarded before any reader could consume it: a consumer reading the transform's own readable directly (as tests/unit/kiro-tool-call-validation.test.ts does) got a raw internal Error instead of the formatted, sanitized failure text. Fix: both call sites now call controller.terminate() instead of controller.error(). terminate() closes the readable side normally (draining the already-enqueued payload to the reader, then EOF) while still erroring the writable side, so no further upstream writes can succeed -- the exact same writable-side effect as before, just without discarding readable-side content already handed to it. Callers verified before this change: - open-sse/utils/streamHandler.ts's outer disconnect-aware wrapper (createDisconnectAwareStream): reads the inner transform's own reader: the newly-delivered response.failed/event:error frame reaches streamReadiness.ts's frameHasStructuredStreamError() (already recognizes "response.failed" and Claude "event: error"), so resolveSilentCloseOutcome() correctly stands down (no synthetic duplicate error/truncation frame gets appended) once the formatted failure is actually delivered. - streamHandler.ts's PENDING_REQUEST_CLEARED_MARKER / isPendingRequestClearedError guard: already unconditionally skips the outer onError failover/cooldown callback for these marked errors even under the old (rejecting) contract, so removing the marker (no longer meaningful once nothing rejects) changes nothing observable there. - open-sse/services/combo/validateQuality.ts's Claude empty-content-block retry (the actual "combo layer can retry" mechanism referenced by the historical cheaper-inference#3685 test comment): reads the RAW upstream provider response directly, entirely independent of this transform stream -- unaffected either way. - Two other controller.error() call sites in stream.ts (idle timeout at line ~1249, empty-choices-stream rejection at line ~2803) do NOT enqueue anything beforehand -- left untouched, since error() is the correct call there (nothing queued to lose). kiro-tool-call-validation.test.ts also needed a small update: writer.close() after the write that triggers the abort now races the abort's own write-side termination (terminate(), like the old error(), already errors the writable as part of handling that write) -- tolerate the resulting "WritableStream is closed" the same way isClientClosedPipelineError already treats it as expected elsewhere in this codebase, rather than asserting a specific teardown order. Test updates (all mechanical: assert.rejects(...) -> await the resolved, formatted text and assert.match against it -- no security assertion weakened, every doesNotMatch/secret-redaction check kept): - tests/unit/kiro-tool-call-validation.test.ts (Responses response.failed -- the originally-failing regression) - tests/unit/claude-empty-stream-error-3685.test.ts (Claude passthrough event: error, incl. the pending-request-counter regression guard, which still passes: clearPendingRequestFromStream() runs before terminate()) - tests/unit/stream-onfailure-callback-logging.test.ts (OpenAI passthrough response.failed, onFailure-throws and onFailure-returns-normally variants) - tests/unit/stream-utils.test.ts (5 tests: Responses rate-limit translate mode, Claude passthrough/translate empty-stream, Responses usage-limit passthrough, pending-request-counter passthrough) Security/sanitization: unchanged. No production sanitization, redaction, or SAFE_PUBLIC_ERROR_IDENTIFIERS logic touched -- this only changes how an already-sanitized, already-formatted payload reaches the reader once built. Not touched: PR #3, PR #1, PR #4, routing policy, GLM/combo, quality gates, migrations, provider code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MPKHh3DGo2bJBmXGQwtvSb
c8d4f2b to
4dbdf61
Compare
|
Reconciled onto the current release/v3.8.51 head (now includes PR #3 combo-audit, PR #4/#6 issue #5 fix, PR #7 Cloudflare Access identity) — clean merge (git merge-tree, zero conflicts), since this feature's diff is fully isolated to combo/explicit-target files none of those PRs touched. Cherry-picked only the 2 real feature commits (dropped the temp CI-workaround commits that were already reverted before the original merge). Fixed 2 real Fast Quality Gates findings surfaced against the new base: a complexity-ratchets regression in explicitTargetDetector.ts (extracted a helper) and a frozen file-size cap on combo.ts (rebaselined with justification, matching this file's own established precedent for irreducible dispatch-chokepoint wiring). All 46 explicit-target-routing unit tests pass. New head: 4dbdf6185. CI running. |
CI fixup: false-positive explicit-target detection + file-size baseline correctionRun 34261416326 surfaced two real issues after reconciliation against the moved release head ( 1. Fast Quality Gates (file-size) — fixed. The earlier rebaseline of 2. Unit Tests fast-path (1/4, 2/4, 3/4) — fixed, real regression. Root cause: every required positive test uses a capitalized alias ("Astra", "Claude"); every false positive uses a lowercase common noun. Fix requires the captured alias to start with an uppercase letter ( Also caught and fixed a real regex-engine trap along the way: the Verified locally: detector's own 32/32 tests pass; direct module invocation confirms Awaiting fresh CI on |
Semantic correction: hybrid /route + conservative natural language (replaces capitalization heuristic)Commit 1. 2. Natural language ("use
Test coverage added/updated
Local verification (this sandbox lacks a full CI-equivalent build; real CI will confirm)
Awaiting fresh CI on |
Real CI failure found and fixed: NL must never reach tier-4 literal-model-id resolutionCI on `1da6e4b97` (the hybrid /route + NL redesign) caught a genuine regression: "Use combo fallback" and "use FooModel para revisar" — both previously-regressed phrases — failed with a raw `400 "Unable to determine provider for model 'combo'"` instead of falling through to normal combo dispatch. Root cause (confirmed via a direct production stack trace, not assumed): `resolveExplicitTarget`'s tier 4 (literal model id, gated on `isModelAvailable`) was reachable for natural-language candidates too. The real `isModelAvailable` implementation (`checkModelAvailable`, `src/sse/handlers/chat.ts`) is written for a model string the caller already believes might be real — it calls `getModelInfo()`, which doesn't throw for a nonsense string like "combo" (it returns a result object with `errorType: "model_not_found"`), and that doesn't reliably propagate as "not available" back up the chain. Fed an arbitrary word extracted from ordinary prose, it can answer "yes" — letting the resolver treat "combo" as a genuine literal model id and dispatch straight to it. This was not reproducible with an isolated unit-test stub (which only returns what you tell it to) — it only surfaced going through the real production check end-to-end via `handleChat`, which is why I chased it down with an actual stack trace before concluding anything. Fix (`038ce3397`): natural language now only ever resolves through tiers 1-3 (settings synonym, exact combo name, exact target label) — all local, deterministic lookups with no fail-open risk. Tier 4 stays available for `/route `, where the user explicitly typed the string as a target and `/route cx/gpt-5.6-sol-high` needs it. Added a regression test that simulates `isModelAvailable` always answering "yes" (not "no") to prove natural language ignores tier 4 regardless of what that check says, rather than passing coincidentally. All 30 dispatch tests + 41 detector tests + 23 slash-command tests + 8 resolver tests pass locally, including a direct reproduction of the original failing scenario via a standalone script hitting the real `handleChat` path (confirmed 200, not 400, after the fix). Awaiting fresh CI on `038ce3397`. Still not merged. |
Fast Quality Gates fixup: max-lines-per-functionCI on `038ce3397` (the tier-4 fix) got all 4 Unit Test shards green (confirming the tier-4 fix itself) but failed Fast Quality Gates on `complexity-ratchets`. Root cause, confirmed via a scoped eslint run: cyclomatic complexity was never the issue (every function in the file measures well under the repo's `complexity>=15` threshold) — the `allowLiteralModelIdLookup` branching pushed `dispatchResolvedTarget` to 97 lines, over `max-lines-per-function>80`. Fixed in `85b383dde` by extracting the model-kind and combo-kind response branches into their own `dispatchToModel`/`dispatchToCombo` helpers — pure extraction, no behavior change (all 30 dispatch tests pass unchanged). `dispatchResolvedTarget` now just resolves, checks unknown/ambiguous, and delegates. Awaiting fresh CI on `85b383dde`. Still not merged. |
CI green — all checks passing on commit 85b383ddeAll 13 checks pass (Fast Quality Gates, all 4 Unit Test shards, semgrep, typecheck, docs, eslint). The one failure seen after the max-lines-per-function fix was a 27-minute hang in an unrelated "RTK strip-code-comments" test on shard 4/4, followed by a runner-level cancellation — confirmed transient by re-running just that job, which passed cleanly on retry with no code changes. READY_FOR_HUMAN_APPROVAL=true. Still not merged, awaiting your review. |
85b383d to
e382a4f
Compare
e382a4f to
5979ea0
Compare
Summary
Reconstructs O360 explicit-target routing directly on
release/v3.8.51commit7bd7c75f1ea2a7a08b2b7719a1e584e91d1ee567. The diff is limited to combo routing, its existing quality baseline, and focused tests; current Azure STT, MAI WebM normalization, auth/audit work, and production fixes remain untouched./route <target>handling, with task text accepted on the directive line or following lines.claude -> cc/claude-sonnet-5andastra -> cx/gpt-5.6-sol-high; operator-configured synonyms retain precedence.o360-codingando360-cheap-workerthrough the existing combo resolver/dispatch path.slash_commandornatural_languagein the existing combo decision trace.Acceptance coverage
/route claude,/route astra,/route o360-coding,/route o360-cheap-worker, and/route cx/gpt-5.6-sol-highUNKNOWN_TARGETandTARGET_UNAVAILABLEwith zero fallbackuse Claudenatural-language routinguse a tool,use combo fallback, andmanda a mensagemfalling through normallyo360-codingordering remainscx/gpt-5.6-sol-highthencc/claude-sonnet-5, with no GLM targetValidation
npm run typecheck:corenpm run check:file-sizenpm run check:complexity-ratchetsLive provider check
cx/gpt-5.6-sol-highinference returned HTTP 200 with the requested sentinel.cc/claude-sonnet-5did not infer and did not substitute another model. With this branch,/route claudereturnsTARGET_UNAVAILABLEwhile that state persists.