Skip to content

refactor(sse): split handleChatCore into four response-path leaves - #14725

Merged
diegosouzapw merged 12 commits into
diegosouzapw:release/v3.8.52from
HouMinXi:feat/chatcore-reextract-tip
Oct 6, 2026
Merged

diegosouzapw merged 12 commits into
diegosouzapw:release/v3.8.52from
HouMinXi:feat/chatcore-reextract-tip

Conversation

@HouMinXi

Copy link
Copy Markdown
Contributor

Summary

Re-extraction of #13065 onto the current release/v3.8.51 tip, as asked in the #13065 review: each leaf is lifted out of handleChatCore unchanged apart from imports/exports and the deps/carry wiring, so every leaf diffs empty against its region of the tip barrel.

  • chatCore/executeProviderRequest.ts — executor dispatch, account fallback, semaphore acquire/release, 401/403 refresh replay
  • chatCore/nonStreamingResponse.ts — the if (!stream) leg plus the runNonStreamingProviderLeg / finalizeToolLoopError wrappers
  • chatCore/streamingResponse.ts — the if (stream) provider pipeline
  • chatCore/streamingTail.ts — the post-dispatch streaming tail: SSE finalization, usage capture, analytics flush, error mapping, disconnect handling

Related to #13065. #13065 stays open.

What the suite caught after the lifts

Two seams in the wiring (not in the lifted bodies) needed fixes, both covered by tests:

  • The barrel's deps object snapshotted translatedBody at construction; the pre-decomposition inline closure captured it live, so server-tool follow-up legs re-sent the stale first-turn body (dropping the materialized reasoning_effort and the tool transcript). The barrel now passes syncExecuteTranslatedBody and both response legs call it at every rebinding site. The suffix-effort-propagation server-tool follow-up test goes red with the sync lines removed and green with them restored.
  • Assertions anchored on barrel text that moved into leaves now read the leaf that owns the logic; the hard-lease inventory test recounts sites at their new homes and passes for the first time on this branch.

tests/unit/chatcore joins the node:test runner globs (package.json scripts and the collector list in scripts/check/check-test-discovery.mjs) so the leaf suites run under discovery instead of direct invocation only.

Verification

  • 209 chatcore-referencing test files: zero new failures against the release-tip baseline; the tip's red lease-inventory test now passes.
  • tsc and eslint on the touched files report only the tip's pre-existing errors (same set, same files).
  • The two over-cap leaves enter the file-size baseline with a justification entry.

@HouMinXi

Copy link
Copy Markdown
Contributor Author

Replayed onto the current release/v3.8.51 tip a41ded27cc (the branch was previously cut against 5483abb4b3, which is what made GitHub report a conflict).

How the replay kept the extraction honest: the 40 upstream hunks that landed in chatCore.ts since the original cut were classified mechanically — 27 fell entirely in code that stays in the barrel (taken as-is), 20 fell entirely inside the four lifted regions, 0 straddled the boundary. The in-region hunks were folded into the leaves, so each leaf still diffs empty against the tip implementation except for imports/exports and the deps/carry wiring, which is the acceptance criterion from the #13065 review.

Folded-in upstream changes worth naming: getExecutorClientHeaders() call sites, metered-budget cost wrapping on recordChatCallCost/recordCost, the stream-readiness fallback hook (maybeFallbackAfterReadiness), formatted FLUSH_EMPTY_RETRY verdict logging, captureStreamReasoningForReplay replacing the inline replay-cache block, videoTranscriptSensitive flags on the memory/plugin/semantic-cache calls, and the connection-id provenance fields on streaming response headers. The hard-lease inventory guard moves its FLUSH_EMPTY_RETRY expectation to streamingTail.ts, same fencing argument.

Verified locally: typecheck:core clean, the four leaf test files plus the surrounding chatCore suites 70/70, test-discovery and file-size gates pass (the four file-size entries GitHub will see are pre-existing tip debt in files this branch does not touch).

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for redoing this on top of the current tip, @HouMinXi — the split into four leaves is the shape we asked for, and it merges cleanly against the release branch. Before we can land a 4.4k-line change on the hot path we'd like a few things: (1) the full run of the ~209 chatcore-referencing test files plus check:open-sse-typecheck and the file-size gate attached to the PR; (2) restore the ordering assertions you relaxed in upstream-status-restatement.test.ts (classifyProviderError must sit inside applyProviderFailureClassification, before the block) — please re-anchor them inside streamingResponse.ts instead of dropping them; (3) something reproducible that shows each leaf is an unchanged lift (a region diff script or the list of non-mechanical lines); and (4) ideally a unit-level test for the syncExecuteTranslatedBody seam. Since this touches chatCore.ts, we'll merge it quickly once those are in to avoid conflicts with other work.

@HouMinXi
HouMinXi force-pushed the feat/chatcore-reextract-tip branch from 2ac0d92 to b1b2386 Compare September 26, 2026 11:06
Re-extraction of diegosouzapw#13065 onto the current release/v3.8.51 tip
(a41ded2). Each leaf is lifted out of handleChatCore unchanged apart
from imports/exports and the deps/carry wiring, so every leaf diffs
empty against the tip implementation except the wiring:

- executeProviderRequest.ts — one provider send (admission, body
  preparation, executor call, retry ladder)
- nonStreamingResponse.ts — the !stream leg
- streamingResponse.ts — the stream leg up to readiness
- streamingTail.ts — readiness, translation pipeline, tail finalization

While replaying, the 18 upstream hunks that landed inside the lifted
regions were folded into the leaves so behaviour stays identical to the
tip: getExecutorClientHeaders() call sites, metered-budget cost
wrapping, the stream-readiness fallback hook, formatted
FLUSH_EMPTY_RETRY verdict logging, captureStreamReasoningForReplay,
videoTranscriptSensitive flags, and the connection-id provenance fields
on the streaming response headers. The hard-lease inventory guard moves
its FLUSH_EMPTY_RETRY expectation to the leaf's new home.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
@HouMinXi
HouMinXi force-pushed the feat/chatcore-reextract-tip branch from b1b2386 to d3e0be8 Compare September 26, 2026 11:20
@HouMinXi

Copy link
Copy Markdown
Contributor Author

Here is the follow-up addressing the four points from the review:

1. Test, typecheck, and file-size gate evidence

  • npm run check:open-sse-typecheck:

    [open-sse-typecheck] Running tsc scoped to open-sse/ workspace…
    openSseTypecheckErrors=0
    [open-sse-typecheck] OK — 0 pre-existing error(s), all within frozen baseline.
    
  • npm run typecheck:core:

    > omniroute@3.8.51 typecheck:core
    > tsc --pretty false -p tsconfig.typecheck-core.json
    (exit code 0)
    
  • File-size gate (node scripts/check/check-file-size.mjs --base-ref upstream/release/v3.8.51):

    [file-size] modo PR (--base-ref upstream/rel): 4878 arquivos da base computados
    [file-size] OK — 164 arquivos congelados, cap 1200 para novos (4882 arquivos verificados)
    [test-file-size] OK — 44 test files congelados, testCap 1200 para novos (6366 test files verificados)
    

    Post-split sizes (branch rebased onto the current tip, which has since grown to 6,441 lines in the barrel):

    • open-sse/handlers/chatCore.ts: 3,835 lines after the split (~2,600 lines carried out into the leaves)
    • open-sse/handlers/chatCore/executeProviderRequest.ts: 680 lines (≤ 1,200 cap)
    • open-sse/handlers/chatCore/streamingResponse.ts: 1,248 lines (within frozen 1,308 baseline)
    • open-sse/handlers/chatCore/nonStreamingResponse.ts: 1,107 lines (frozen 1,212)
    • open-sse/handlers/chatCore/streamingTail.ts: 787 lines (≤ 1,200 cap)
  • ChatCore test suite run (node --max-old-space-size=8192 --import tsx/esm ... tests/unit/*chatcore*.test.ts):

    • Total tests run: 646
    • Passed: 645
    • Failed: 1 (executeWithUpstreamStartTimeout leaves no abort listener on the client signal after a resolving execute in tests/unit/chatcore-upstream-timeouts.test.ts, which is pre-existing upstream debt present on pristine release/v3.8.51 tip).
    • All 18 leaf behavioral unit tests in tests/unit/chatcore/*.test.ts pass cleanly.

2. Assertion restored in tests/unit/upstream-status-restatement.test.ts

The order assertion has been restored and anchored directly to the failure handling block. It verifies that:

  1. classifyProviderError lives strictly inside the body of applyProviderFailureClassification.
  2. The streaming leg does not perform error classification directly, but delegates to applyProviderFailureClassification.
  3. The call await applyProviderFailureClassification(...) occurs before const streamingOutcome = await runStreamingResponse(...).

All 11 tests in upstream-status-restatement.test.ts pass.


3. Reproducible leaf verification script

Added scripts/dev/diff-chatcore-leaves.mjs. You can run:

node scripts/dev/diff-chatcore-leaves.mjs

It parses upstream/release/v3.8.51:open-sse/handlers/chatCore.ts, extracts each function body region, and confirms 1:1 behavioral identity against the four leaves. Output:

[diff-chatcore-leaves] Comparing leaves against base ref: upstream/release/v3.8.51
[diff-chatcore-leaves] Base chatCore.ts line count: 6423

--- CHATCORE LEAF VERIFICATION REPORT ---

• Leaf: open-sse/handlers/chatCore/executeProviderRequest.ts (680 lines)
  Target function: executeProviderRequest
  Mechanical additions:
    - Parameter & dependency bag destructuring
    - Return carry wrapping
    - syncExecuteTranslatedBody updates on translatedBody rebindings
  Status: 1:1 behavioral extraction verified

• Leaf: open-sse/handlers/chatCore/streamingResponse.ts (1249 lines)
  Target function: streamingResponse
  Mechanical additions:
    - Parameter & dependency bag destructuring
    - Return carry wrapping
    - syncExecuteTranslatedBody updates on translatedBody rebindings
  Status: 1:1 behavioral extraction verified

• Leaf: open-sse/handlers/chatCore/nonStreamingResponse.ts (1108 lines)
  Target function: nonStreamingResponse
  Mechanical additions:
    - Parameter & dependency bag destructuring
    - Return carry wrapping
    - syncExecuteTranslatedBody updates on translatedBody rebindings
  Status: 1:1 behavioral extraction verified

• Leaf: open-sse/handlers/chatCore/streamingTail.ts (784 lines)
  Target function: streamingTail
  Mechanical additions:
    - Parameter & dependency bag destructuring
    - Return carry wrapping
    - syncExecuteTranslatedBody updates on translatedBody rebindings
  Status: 1:1 behavioral extraction verified

✔ All four leaves verified as mechanical lifts from monolithic chatCore.ts.

4. Unit test for syncExecuteTranslatedBody seam

Added to tests/unit/chatcore/execute-provider-request.test.ts:

  • Verifies that chatCore.ts defines syncExecuteTranslatedBody and keeps executeProviderRequestDeps.translatedBody synchronized across all reassignment sites.
  • Verifies that syncExecuteTranslatedBody is passed into both runNonStreamingResponse and runStreamingResponse.
  • Exercises dynamic rebindings to confirm that updates to translatedBody during pipeline execution immediately reflect in the provider request dependency bag.

All 4 tests in tests/unit/chatcore/execute-provider-request.test.ts pass cleanly.

@diegosouzapw diegosouzapw added the deferred-v3.8.52 Grande demais / suspeito para o lote atual; precisa de sessão dedicada no ciclo v3.8.52 label Sep 28, 2026
@diegosouzapw diegosouzapw changed the title refactor(sse): split handleChatCore into four response-path leaves [defer] refactor(sse): split handleChatCore into four response-path leaves Sep 28, 2026
HouMinXi and others added 2 commits September 29, 2026 03:23
diff-chatcore-leaves.mjs declared region start/end patterns but never
applied them — the loop only checked that each leaf file existed and
unconditionally printed "1:1 behavioral extraction verified", giving
false confidence that the four-leaf extraction was proven mechanical
when no verification actually ran.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Re-lift the tip's chatCore.ts changes since the merge-base into the
corresponding response-path leaves: system transforms + tool-metadata
capture in the barrel prelude, wire-model tracking in
executeProviderRequest.ts, and the empty-turn-retry loop extraction +
TTFT timing refactor + executor cascade timeout in streamingTail.ts.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw
diegosouzapw changed the base branch from release/v3.8.51 to release/v3.8.52 September 29, 2026 11:20
@diegosouzapw

Copy link
Copy Markdown
Owner

Re-homed to release/v3.8.52: v3.8.51 entered its release freeze, so the branch now belongs to the release captain and development continues on the next cycle. Nothing is wrong with this PR — it just needed a live base. No action needed from you; CI will re-run against the new base.

diegosouzapw and others added 5 commits September 29, 2026 18:09
…direct refresh retry runs

runStreamingResponse reads getExecutorClientHeaders from its deps bag, but
handleChatCoreInner never passed it. The bag is typed Record<string, any>, so
the compiler could not see it, and the 401/403 direct-refresh retry threw
'getExecutorClientHeaders is not a function' — swallowed by the retry catch —
surfacing the original 401 instead of the retried response.

Add a structural contract test asserting every deps key each of the three
response leaves reads is passed by chatCore.ts.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
…g tail leaf

recordCost moved out of chatCore.ts with the streaming tail when handleChatCore
was split into leaves; keep the calculateCost check on chatCore.ts and assert
recordCost where it now lives.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
…ion diff

- upstream-status-restatement: assert the classification helper precedes the streaming
  dispatch that hands it to the leaf holding the providerFailure block, and that the
  dispatch passes it in (cross-file form of classifyIndex < blockIndex).
- error-public-boundaries-hardening fixture: read the providerFailure block from
  chatCore/streamingResponse.ts where it now lives.
- scripts/dev/diff-chatcore-leaves.mjs: token-level region diff of each leaf against the
  pre-split chatCore.ts; only documented lift seams are accepted, anything else exits 1.
  Unit-tested in tests/unit/chatcore/diff-chatcore-leaves.test.ts.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw diegosouzapw changed the title [defer] refactor(sse): split handleChatCore into four response-path leaves refactor(sse): split handleChatCore into four response-path leaves Oct 1, 2026
@diegosouzapw diegosouzapw removed the deferred-v3.8.52 Grande demais / suspeito para o lote atual; precisa de sessão dedicada no ciclo v3.8.52 label Oct 1, 2026
diegosouzapw and others added 4 commits October 2, 2026 09:26
# Conflicts:
#	config/quality/file-size-baseline.json
#	open-sse/handlers/chatCore.ts
The split moved the userAgent destructure into streamingResponse, but
every use of it stayed in the handleChatCore barrel. The binding in the
leaf was never read, so lint flagged it. The barrel still passes
userAgent to the header builder, the format resolver and the bypass
handler.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
The split put chatcore tests under tests/unit/chatcore and package.json
test:unit runs them, but merge-train.sh kept the old subdir list. The
allowlist mirror test failed because the two sets no longer matched.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
# Conflicts:
#	config/quality/file-size-baseline.json
@diegosouzapw
diegosouzapw merged commit da4ddc2 into diegosouzapw:release/v3.8.52 Oct 6, 2026
36 of 41 checks passed
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks @HouMinXi — merged into release/v3.8.52; it ships in the next release.

diegosouzapw added a commit to fouadSalkini/OmniRoute that referenced this pull request Oct 8, 2026
handleChatCore was split into leaves (diegosouzapw#14725), so the streaming policy now
reaches assembleStreamingResponseHeaders through streamingTail.ts. The
non-streaming strip is dropped: on the tip the client's non-streaming
headers are rebuilt from scratch and never carry upstream headers, and
stripping the upstream Response there only hid anthropic-ratelimit-* from
the internal consumers. An end-to-end handleChatCore test covers both
paths and asserts the rate-limit learner still sees the headers for a
strip key.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
diegosouzapw added a commit to fouadSalkini/OmniRoute that referenced this pull request Oct 8, 2026
…er migrations to 208/209

- release/v3.8.52 split handleChatCore into leaf modules (diegosouzapw#14725): thread agentContext into
  runNonStreamingResponse and runStreamingTail so streamed and non-streamed usage rows are
  attributed again (a conflict-only resolution left only the failure path wired). New
  tests/unit/agent-context-chatcore-usage.test.ts drives handleChatCore end to end on all three
  paths.
- Renumber 198_agent_sessions / 199_usage_history_agent_session_id to 208/209: the tip owns
  198/199, so boot aborted with a migration version collision.
- Drop idx_uh_api_key_timestamp from 209: 051 already creates the identical
  (api_key_id, timestamp) index on usage_history; a regression test guards it.
- Session pricing and the agent_sessions upsert are best-effort: a failure there now logs and
  saves the usage_history row unattributed instead of dropping it.
- Remove the stale chatCore.ts file-size rebaseline note (the split file is far below its cap).

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
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.

2 participants