Skip to content

refactor: modularize 2.6K-line stream.ts (Issue #3594) - #3788

Closed
oyi77 wants to merge 19 commits into
diegosouzapw:release/v3.8.27from
oyi77:pr/3651-clean
Closed

oyi77 wants to merge 19 commits into
diegosouzapw:release/v3.8.27from
oyi77:pr/3651-clean

Conversation

@oyi77

@oyi77 oyi77 commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Clean re-cut of PR #3651 — modularizes open-sse/utils/stream.ts (2709→9 lines) into open-sse/utils/stream/ with 10 focused modules.

Cherry-picked only the modularization commit (b9ed863b9) onto current release/v3.8.24. No stacked dependencies, no unrelated changes.

New modules:

  • streamCore.ts — SSE parsing, streaming logic (1947 lines)
  • claudeLifecycle.ts — Claude message lifecycle handling
  • responsesLifecycle.ts — Responses API lifecycle
  • sseFormatters.ts — SSE event formatting
  • errors.ts — Stream error types
  • openaiChunks.ts — OpenAI chunk format
  • textualToolCalls.ts — Text-based tool call handling
  • types.ts — Stream types
  • utils.ts — Stream utilities
  • index.ts — Public API re-exports

@oyi77
oyi77 requested a review from diegosouzapw as a code owner June 13, 2026 20:36

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modularizes the SSE streaming utility code by splitting it into several smaller files under open-sse/utils/stream/. The review feedback highlights critical correctness issues across multiple files due to missing imports and undefined references (such as FORMATS, initState, and FETCH_BODY_TIMEOUT_MS) that will cause compilation and runtime failures. Additionally, the reviewer pointed out a repository style guide violation for not including tests with these production code changes, as well as several instances of unused imports that should be cleaned up.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread open-sse/utils/stream/streamCore.ts Outdated
* @param {object} options.body - Request body (for input token estimation)
* @param {function} options.onComplete - Callback when stream finishes: ({ status, usage }) => void
*/
export function createSSEStream(options: StreamOptions = {}) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Critical Correctness Issue: Missing Imports and Undefined References

During the modularization of stream.ts, several essential constants, helper functions, and external dependencies were not imported or defined in streamCore.ts. This will cause the file to fail compilation and crash at runtime.

Please ensure the following are properly imported or defined:

  • Constants/Enums: FORMATS, STREAM_IDLE_TIMEOUT_MS, HTTP_STATUS, OMIT_STREAMING_CHUNK_MARKER
  • Functions: initState, generateSessionId, consumeToolFinishTime, parseTextualToolCallCandidate, formatSSE, sanitizeStreamingChunk, extractThinkingFromContent, estimateUsage, filterUsageForFormat, addBufferToUsage, calculateCost, buildOmniRouteSseMetadataComment, trackPendingRequest, appendRequestLog, parseSSELine, extractUsage, fixInvalidId, recordToolLatency, markToolFinish, unwrapGeminiChunk, translateResponse, buildErrorBody, logUsage, buildStreamSummaryFromEvents

onFailure?: ((payload: StreamFailurePayload) => void | Promise<void>) | null;
};

export type TranslateState = ReturnType<typeof initState> & {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Correctness Issue: Undefined Reference to initState

The type helper initState is referenced here to define TranslateState but is not imported or defined in this file. This will cause a TypeScript compilation error.

import { JsonRecord, ToolCall } from "./types.ts";

export function parseTextualToolCallFromContent(text: unknown): { name: string; args: unknown } | null {
const candidate = parseTextualToolCallCandidate(text);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Correctness Issue: Undefined References

The functions parseTextualToolCallCandidate (used on lines 8 and 13) and isValidToolCallHeaderPrefix (used on line 29) are referenced but not imported or defined in this file. This will cause a TypeScript compilation error.

*/
export function withBodyTimeout<T>(
promise: Promise<T>,
timeoutMs: number = FETCH_BODY_TIMEOUT_MS

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Correctness Issue: Undefined Reference to FETCH_BODY_TIMEOUT_MS

The constant FETCH_BODY_TIMEOUT_MS is referenced as a default parameter value but is not imported or defined in this file. This will cause a TypeScript compilation error.

@@ -0,0 +1,9 @@
export * from "./types.ts";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Style Guide Violation: Missing Tests for Production Code Changes

This PR modularizes production code under open-sse/utils/stream/ but does not include any new or updated unit/integration tests. This violates Rule 9 of the Repository Style Guide.

Please add appropriate tests under the tests/ directory to cover the new modularized stream logic.

References
  1. Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)

Comment on lines +1 to +5
import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts";
import { v4 as uuidv4 } from "uuid";

import { asRecord } from "./utils.ts";
import { JsonRecord, ToolCall } from "./types.ts";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Maintainability: Unused Imports

The imports convertOpenAIToResponsesToolCall and uuidv4 are completely unused in this file and should be removed to keep the codebase clean.

Suggested change
import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts";
import { v4 as uuidv4 } from "uuid";
import { asRecord } from "./utils.ts";
import { JsonRecord, ToolCall } from "./types.ts";
import { asRecord } from "./utils.ts";
import { JsonRecord, ToolCall } from "./types.ts";

Comment on lines +1 to +5
import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts";
import { v4 as uuidv4 } from "uuid";

import { asRecord } from "./utils.ts";
import { JsonRecord, ToolCall } from "./types.ts";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Maintainability: Unused Imports

The imports convertOpenAIToResponsesToolCall and uuidv4 are completely unused in this file and should be removed to keep the codebase clean.

Suggested change
import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts";
import { v4 as uuidv4 } from "uuid";
import { asRecord } from "./utils.ts";
import { JsonRecord, ToolCall } from "./types.ts";
import { asRecord } from "./utils.ts";
import { JsonRecord, ToolCall } from "./types.ts";

Comment thread open-sse/utils/stream/types.ts Outdated
Comment on lines +1 to +4
import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts";
import { v4 as uuidv4 } from "uuid";


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Maintainability: Unused Imports

The imports convertOpenAIToResponsesToolCall and uuidv4 are completely unused in this file and should be removed to keep the codebase clean.

Comment on lines +1 to +5
import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts";
import { v4 as uuidv4 } from "uuid";

import { createSSEStream } from "./streamCore.ts";
import { JsonRecord } from "./types.ts";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Maintainability: Unused Imports

The imports convertOpenAIToResponsesToolCall, uuidv4, and createSSEStream are completely unused in this file and should be removed to keep the codebase clean.

Suggested change
import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts";
import { v4 as uuidv4 } from "uuid";
import { createSSEStream } from "./streamCore.ts";
import { JsonRecord } from "./types.ts";
import { JsonRecord } from "./types.ts";

Comment thread open-sse/utils/stream/streamCore.ts Outdated
Comment on lines +1 to +3
import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts";
import { v4 as uuidv4 } from "uuid";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Maintainability: Unused Imports

The imports convertOpenAIToResponsesToolCall and uuidv4 are completely unused in this file and should be removed to keep the codebase clean.

@diegosouzapw
diegosouzapw changed the base branch from release/v3.8.25 to release/v3.8.26 June 15, 2026 07:24
diegosouzapw and others added 15 commits June 15, 2026 09:11
…d docs audit report

- README advertised a stale '177 providers'; the canonical generator
  (gen-provider-reference.ts) reports 226 unique provider IDs. Updated the README
  badges/anchors/heading and regenerated docs/reference/PROVIDER_REFERENCE.md (223→226)
  to match the source of truth.
- Adds DOCUMENTATION_AUDIT_REPORT.md (docs/i18n sync audit + plan).

Docs-only; carried over from in-progress working-tree changes and verified against the
generator.
…iegosouzapw#3894)

Document the real pipeline (Session-Dedup, CCR, RTK, Headroom, Caveman,
LLMLingua-2, Lite, Aggressive, Ultra) that replaced the old RTK+Caveman
framing; presets table kept. '7 options' -> '9 engines'.
…root (diegosouzapw#3896)

Move the committed quality-gate state files out of the repo root into
config/quality/ and the v3.8.24 documentation audit into docs/ops/, then
re-point every gate script, test and .gitignore entry at the new paths.
Refresh docs/architecture/REPOSITORY_MAP.md (stale since v3.8.2) to match
the current layout.

Moved -> config/quality/:
  quality-baseline.json, complexity-baseline.json, duplication-baseline.json,
  file-size-baseline.json, test-discovery-baseline.json,
  dependency-allowlist.json, .license-allowlist.json
  (generated quality-metrics.json now written here too; still gitignored)

Moved -> docs/ops/:
  DOCUMENTATION_AUDIT_REPORT.md (+ meta.json entry + fabricated-docs skip)

Path updates: check-{complexity,duplication,file-size,test-discovery,deps,
licenses,dead-code,cognitive-complexity,type-coverage}.mjs, check-quality-
ratchet.mjs, collect-metrics.mjs, check-tracked-artifacts.mjs (+ its test and
check-deps test). Also gitignore /logs/ (was untracked-not-ignored).

Tracked root files: 56 -> 48. Tool configs left in root on purpose: most are
auto-discovered there, and the tsconfig variants have location-relative
files:[] arrays that would need 46 path rewrites for a 2-file gain.
diegosouzapw#3900)

The wiki has no native generator and drifts each release — it lacked SUPPLY_CHAIN
plus 24 other docs pages, and the cover counts went stale (212+/14/37 vs 226/15/87).

Adds:
  - scripts/docs/sync-wiki.mjs — adds docs/ pages missing from the wiki (curated;
    internal reports/plans/index excluded) and syncs the four Home.md cover counts.
    Matches the hand-curated, non-deterministic wiki page names by a normalized fuzzy
    key and writes the EXISTING name, so it never creates duplicate pages. Overwriting
    existing-page content is opt-in (--update-existing) and intentionally OFF by
    default: several docs sources still carry stale counts (e.g. ARCHITECTURE.md says
    "177 providers / 37 MCP tools" while the wiki cover is 226/87), so a blind overwrite
    would REGRESS the wiki. Full parity is gated on regenerating those sources — see
    docs/ops/DOCUMENTATION_AUDIT_REPORT.md.
  - .github/workflows/wiki-sync.yml — runs the sync on every push to main that touches
    docs/ (or a count source) + workflow_dispatch, pushing via GITHUB_TOKEN.
  - tests/unit/sync-wiki.test.ts — pure-function coverage (8 tests).

The first run already pushed the 25 missing pages to the wiki.
- extracted types, utils, responsesLifecycle, textualToolCalls, sseFormatters, errors, claudeLifecycle, openaiChunks, and streamCore to domain files
- stream.ts is now a facade re-exporting modules
… fix ctx.* object key prefixes in streamCore.ts
oyi77 added 3 commits June 16, 2026 02:53
…mCore.ts and stream.ts

- Restore working stream.ts from release/v3.8.26 (2584 lines)
- Keep modular stream/ directory for future incremental PRs
- Update file-size-baseline.json: streamCore.ts 2212, stream.ts 2584
- All tests pass: 49/49 stream-utils, 14/14 stream-handler
- Typecheck, lint, cycles, file-size: all clean
@diegosouzapw
diegosouzapw changed the base branch from release/v3.8.26 to release/v3.8.27 June 16, 2026 06:20
@oyi77

oyi77 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #3917–#3928 (10 independent stream module extractions off release/v3.8.27). Closing this monolithic PR per maintainer guidance.

@oyi77 oyi77 closed this Jun 16, 2026
@oyi77
oyi77 deleted the pr/3651-clean branch August 7, 2026 21:08
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.

7 participants