Skip to content

fix: log recovered stdio frames at WARN without a stack trace - #984

Merged
devcrocod merged 3 commits into
modelcontextprotocol:mainfrom
kkkhs:agent/fix-stdio-bom-956
Oct 28, 2026
Merged

devcrocod merged 3 commits into
modelcontextprotocol:mainfrom
kkkhs:agent/fix-stdio-bom-956

Conversation

@kkkhs

@kkkhs kkkhs commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

What

Fixes #956. A BOM-prefixed line was already delivered through ReadBuffer's recovery path, but readMessage first logged an ERROR with a stack trace and the whole line.

Fix

  • A recovered line is logged once at WARN, without a stack trace.
  • A dropped line is logged once at ERROR (previously up to two ERRORs).
  • Logs carry a 160-char line prefix instead of the whole line.

Test

Added a ReadBuffer regression for BOM-prefixed messages.

Copilot AI lite review requested due to automatic review settings September 7, 2026 08:27

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kkkhs
kkkhs force-pushed the agent/fix-stdio-bom-956 branch from 8f2311a to 0a04ce7 Compare October 26, 2026 07:18
Copilot AI lite review requested due to automatic review settings October 26, 2026 07:18

Copilot AI 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.

🟢 Approval recommended

The BOM handling change is covered by a regression test, and no unresolved review issues remain.

No open issues 🎉

Review effort: Lite

@devcrocod devcrocod 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.

Thanks for the PR!
A BOM prefix line is already delivered now via tryRecover. What #956 actually shows is the error with a stack trace that readMessage logs before recovery even runs.
Rather than adding another parsing rule, could you rework the logging:
WARN without a stack trace when recovery succeeds,
ERROR only when the line is dropped?
And please log a truncated prefix instead of the whole line.

Copilot AI lite review requested due to automatic review settings October 28, 2026 07:49

Copilot AI 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.

🟡 Changes recommended

BOM-prefixed valid frames still take the malformed-input recovery path and emit a warning, so the initial deserialization must strip the BOM and regression coverage should verify that behavior.

1 open issue

Review effort: Lite

Copilot AI lite review requested due to automatic review settings October 28, 2026 13:13

Copilot AI 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.

🟡 Changes recommended

The BOM-prefixed line is still deserialized before normalization, causing affected frames to fail initially and rely on recovery while emitting warnings; leading BOM characters should be stripped before the first parse attempt.

2 open issues

Review effort: Lite

} catch (e: Exception) {
logger.error(e) { "Failed to deserialize message from line: $line\nAttempting to recover..." }
tryRecover(line)
val recovered = tryRecover(line)
@devcrocod
devcrocod marked this pull request as ready for review October 28, 2026 13:56

@devcrocod devcrocod 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.

lgtm

@devcrocod devcrocod changed the title fix: handle UTF-8 BOM in JSON-RPC frames fix: log recovered stdio frames at WARN without a stack trace Oct 28, 2026
@devcrocod
devcrocod merged commit 0d180d2 into modelcontextprotocol:main Oct 28, 2026
19 checks passed
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.

StdioServerTransport fails to deserialize initial message when stdin starts with UTF-8 BOM

3 participants