Skip to content

fix: redirect Winston logger to stderr to prevent IPC stream corruption + fix binary tests#11914

Merged
sestinj merged 4 commits intomainfrom
jb-binary-tests
Mar 27, 2026
Merged

fix: redirect Winston logger to stderr to prevent IPC stream corruption + fix binary tests#11914
sestinj merged 4 commits intomainfrom
jb-binary-tests

Conversation

@RomneyDa
Copy link
Copy Markdown
Contributor

@RomneyDa RomneyDa commented Mar 27, 2026

Summary

  • Fix Winston stdout corruption: Winston's Console transport writes directly to process.stdout, which corrupts the \r\n-delimited JSON IPC stream between the binary and JetBrains plugin. Redirected all Winston log levels to stderr instead.
  • Fix binary tests: Replaced ReverseMessageIde with BinaryIdeHandler that responds with plain data matching the Kotlin CoreMessenger format, added response unwrapping for test assertions, and updated stale test expectations (modelsmodelsByRole, expected files).

Test plan

  • Binary tests pass locally (6/6)
  • Binary tests pass in CI across all platforms

…al requests

The binary tests were failing because Core sends getIdeInfo, getIdeSettings,
and getControlPlaneSessionInfo on startup but the test had no handlers for
these messages (ReverseMessageIde was commented out), causing the binary to
crash immediately.
@RomneyDa RomneyDa requested a review from a team as a code owner March 27, 2026 00:53
@RomneyDa RomneyDa requested review from Patrick-Erichsen and removed request for a team March 27, 2026 00:53
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 27, 2026
@continue
Copy link
Copy Markdown
Contributor

continue bot commented Mar 27, 2026

No documentation updates needed for this PR.

This change fixes internal test infrastructure by wiring up ReverseMessageIde in binary tests and adjusting Jest timeouts. These modifications are confined to the test suite (binary/test/binary.test.ts) and don't affect any user-facing APIs, configuration options, or documented workflows.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

@RomneyDa RomneyDa marked this pull request as draft March 27, 2026 19:59
…t corruption

- Replace ReverseMessageIde with BinaryIdeHandler that responds with plain
  data (matching Kotlin CoreMessenger format) instead of the JS _handleLine
  auto-wrapper
- Unwrap binary responses in test assertions (binary wraps in {done, content, status})
- Redirect Winston Console transport to stderr so it never corrupts the
  stdout IPC JSON stream
- Update stale test expectations (models → modelsByRole, expected files)
- Add stderr capture for debugging binary crashes
- Set jest timeout to 30s for binary startup
@RomneyDa RomneyDa changed the title fix: wire up ReverseMessageIde in binary tests fix: redirect Winston logger to stderr to prevent IPC stream corruption + fix binary tests Mar 27, 2026
@RomneyDa RomneyDa marked this pull request as ready for review March 27, 2026 20:30
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 27, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

2 issues found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="binary/test/binary.test.ts">

<violation number="1" location="binary/test/binary.test.ts:85">
P2: Avoid per-chunk `Buffer.toString()` for stdout IPC parsing; use stream UTF-8 decoding (`setEncoding("utf8")`) so split multibyte characters are handled correctly.

(Based on your team's feedback about Node.js child process stream decoding with UTF-8 StringDecoder.) [FEEDBACK_USED]</violation>

<violation number="2" location="binary/test/binary.test.ts:246">
P2: `BinaryIdeHandler` is created even in TCP mode where `subprocess` is undefined, which will crash test setup when `USE_TCP` is enabled.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@sestinj sestinj merged commit 294ba93 into main Mar 27, 2026
62 of 66 checks passed
@sestinj sestinj deleted the jb-binary-tests branch March 27, 2026 21:03
@github-project-automation github-project-automation bot moved this from Todo to Done in Issues and PRs Mar 27, 2026
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants