Skip to content

fix(sse): remove duplicate const settings in handleChatCore (release-blocker) - #2989

Merged
diegosouzapw merged 1 commit into
release/v3.8.8from
fix/chatcore-duplicate-settings
May 31, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.8from
fix/chatcore-duplicate-settings

Conversation

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Problem (release-blocker)

handleChatCore (open-sse/handlers/chatCore.ts) declared const settings = cachedSettings ?? (await getCachedSettings()) twice in the same function scope — the consolidated "fetch once, reuse" const (~line 1921) plus a duplicate (~line 2154) added alongside the per-key stream-default-mode feature (commit 99d673fe5).

esbuild/tsx rejects the same-scope redeclaration:

chatCore.ts:2154:8: ERROR: The symbol "settings" has already been declared

This turned every unit test that imports chatCore red (e.g. system-role-extraction, claude-passthrough-thinking-2454) and breaks the production build (a duplicate block-scoped const is a hard JS error).

Same commit (99d673fe5) that introduced the migration-077 collision fixed in #2966.

Fix

Remove the duplicate declaration; reuse the earlier consolidated settings.

Verification

  • chatcore-imports-cleanly (new import guard): 1/1
  • system-role-extraction: 10/10 (was red — TransformError)
  • claude-passthrough-thinking-2454: 4/4 (was red)
  • typecheck:core: no settings redeclare error; lint clean.

🤖 Generated with Claude Code

handleChatCore declared `const settings = cachedSettings ?? await
getCachedSettings()` twice in the same function scope — the consolidated
"fetch once, reuse" const near the top, plus a duplicate added alongside the
per-key stream-default-mode feature. esbuild/tsx rejected the same-scope
redeclaration ("The symbol 'settings' has already been declared"), which made
every unit test that imports chatCore fail to transform and broke the
production build.

Remove the duplicate; reuse the earlier consolidated `settings`. Add an
import-guard regression test.

@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 resolves a duplicate const settings declaration in open-sse/handlers/chatCore.ts that was causing build and test failures, and adds a regression test to prevent future occurrences. Feedback on these changes suggests removing the verbose comment documenting the bug in the source code, and refactoring the new test file to use vitest instead of Node's native test runner to align with the repository's testing framework.

Comment on lines +13 to +23
import test from "node:test";
import assert from "node:assert/strict";

test("chatCore.ts imports without a duplicate-declaration transform error", async () => {
const mod = await import("../../open-sse/handlers/chatCore.ts");
assert.equal(
typeof mod.handleChatCore,
"function",
"handleChatCore must be importable (no duplicate `const settings` in scope)"
);
});

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

The project uses Vitest as its primary test runner (as indicated by the presence of vitest.config.ts in the repository configuration). For consistency across the test suite and proper integration with Vitest's test runner, reporters, and coverage tools, please use vitest instead of the Node.js native node:test and node:assert modules.

import { test, expect } from "vitest";

test("chatCore.ts imports without a duplicate-declaration transform error", async () => {
  const mod = await import("../../open-sse/handlers/chatCore.ts");
  expect(typeof mod.handleChatCore).toBe("function");
});
References
  1. Rule 1 of the Repository Style Guide mentions vitest.config.ts as a standard configuration file and refers to test files as 'Vitest files', indicating Vitest is the standard testing framework for the repository. (link)

Comment on lines +2154 to +2157
// `settings` is already consolidated once near the top of handleChatCore
// (the "fetch once, reuse" const). A second `const settings` here was a
// duplicate same-scope declaration that broke the esbuild/tsx transform
// ("settings has already been declared") and the production build. Reuse it.

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

Avoid committing verbose comments that document past bugs, git history, or PR context directly in the source code. This information is already captured in the git history and PR description, and keeping it in the code adds unnecessary noise and can quickly become outdated. The code is self-explanatory without this comment.

@diegosouzapw
diegosouzapw merged commit 923b8fe into release/v3.8.8 May 31, 2026
3 checks passed
@diegosouzapw
diegosouzapw deleted the fix/chatcore-duplicate-settings branch May 31, 2026 12:03
@kilo-code-bot

kilo-code-bot Bot commented May 31, 2026 •

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge (already merged)

Files Reviewed (3 files)
  • CHANGELOG.md - Change logged
  • open-sse/handlers/chatCore.ts - Duplicate const settings removed
  • tests/unit/chatcore-imports-cleanly.test.ts - Regression guard added

Reviewed by step-3.7-flash-20260528 · 363,616 tokens

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