Skip to content

Fix MCP stdio protocol violation during startup (Docker/langchain compatibility) #285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 2, 2025
Merged

Fix MCP stdio protocol violation during startup (Docker/langchain compatibility) #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/custom-stdio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,13 +277,25 @@ export class FilteredStdioServerTransport extends StdioServerTransport {

/**
* Public method to send log notifications from anywhere in the application
* Now properly buffers messages before MCP initialization to avoid breaking stdio protocol
*/
public sendLog(level: "emergency" | "alert" | "critical" | "error" | "warning" | "notice" | "info" | "debug", message: string, data?: any) {
// Skip if notifications are disabled (e.g., for Cline)
if (this.disableNotifications) {
return;
}

// Buffer messages before initialization to avoid breaking MCP protocol
// MCP requires client to send first message - server cannot write to stdout before that
if (!this.isInitialized) {
this.messageBuffer.push({
level,
args: [data ? { message, ...data } : message],
timestamp: Date.now()
});
return;
}

try {
const notification: LogNotification = {
jsonrpc: "2.0",
Expand Down Expand Up @@ -315,6 +327,11 @@ export class FilteredStdioServerTransport extends StdioServerTransport {
* Send a progress notification (useful for long-running operations)
*/
public sendProgress(token: string, value: number, total?: number) {
// Don't send progress before initialization - would break MCP protocol
if (!this.isInitialized) {
Copy link
Contributor

@codeant-ai codeant-ai bot Dec 1, 2025

Choose a reason for hiding this comment

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

Suggestion: The progress notification method currently ignores the disableNotifications flag, so clients explicitly configured to have notifications disabled (e.g., Cline/vscode/claude-dev) will still receive progress JSON-RPC messages, which contradicts the intent of configureForClient and can break those clients; you should also short‑circuit when notifications are disabled. [logic error]

Severity Level: Minor ⚠️

Suggested change
if (!this.isInitialized) {
// Also respect clients that have notifications disabled entirely
if (!this.isInitialized || this.disableNotifications) {
Why it matters? ⭐

This is a real behavioral bug, not cosmetic. The class has an explicit
disableNotifications flag set in configureForClient for Cline/vscode/claude-dev,
and both sendLog and sendLogNotification already short-circuit on that flag,
meaning "notifications disabled" is intended to apply globally. However,
sendProgress only checks isInitialized and still emits JSON-RPC progress
notifications even when disableNotifications is true, which contradicts the
stated behavior and can still break those same clients. Adding the
this.disableNotifications check aligns sendProgress with the rest of the
class' behavior and prevents unwanted notifications from being sent.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** src/custom-stdio.ts
**Line:** 331:331
**Comment:**
	*Logic Error: The progress notification method currently ignores the `disableNotifications` flag, so clients explicitly configured to have notifications disabled (e.g., Cline/vscode/claude-dev) will still receive progress JSON-RPC messages, which contradicts the intent of `configureForClient` and can break those clients; you should also short‑circuit when notifications are disabled.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

return;
}

try {
const notification = {
jsonrpc: "2.0" as const,
Expand Down Expand Up @@ -346,6 +363,11 @@ export class FilteredStdioServerTransport extends StdioServerTransport {
* Send a custom notification with any method name
*/
public sendCustomNotification(method: string, params: any) {
// Don't send custom notifications before initialization - would break MCP protocol
if (!this.isInitialized) {
Copy link
Contributor

@codeant-ai codeant-ai bot Dec 1, 2025

Choose a reason for hiding this comment

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

Suggestion: The custom notification method does not check the disableNotifications flag, so even when a client has been configured to disable all notifications, custom JSON-RPC notifications will still be sent, leading to inconsistent behavior and potential protocol issues for those clients; this should also bail out when notifications are disabled. [logic error]

Severity Level: Minor ⚠️

Suggested change
if (!this.isInitialized) {
// Also respect clients that have notifications disabled entirely
if (!this.isInitialized || this.disableNotifications) {
Why it matters? ⭐

Same story here: configureForClient explicitly sets disableNotifications
for certain clients and logs "Notifications disabled", and both sendLog and
sendLogNotification honor that flag. sendCustomNotification only checks
isInitialized and will still push arbitrary JSON-RPC notifications over
stdout even when disableNotifications is true, which is inconsistent and
undermines the purpose of the flag. Adding a this.disableNotifications
short-circuit fixes a real logic inconsistency and prevents custom notifications
from leaking to clients that are supposed to receive none.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** src/custom-stdio.ts
**Line:** 367:367
**Comment:**
	*Logic Error: The custom notification method does not check the `disableNotifications` flag, so even when a client has been configured to disable all notifications, custom JSON-RPC notifications will still be sent, leading to inconsistent behavior and potential protocol issues for those clients; this should also bail out when notifications are disabled.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

return;
}

try {
const notification = {
jsonrpc: "2.0" as const,
Expand Down
11 changes: 7 additions & 4 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ async function runServer() {
// Set global flag for onboarding control
(global as any).disableOnboarding = DISABLE_ONBOARDING;

// Create transport FIRST so all logging gets properly buffered
// This must happen before any code that might use logger.*
const transport = new FilteredStdioServerTransport();

// Export transport for use throughout the application
global.mcpTransport = transport;

try {
deferLog('info', 'Loading configuration...');
await configManager.loadConfig();
Expand All @@ -56,10 +63,6 @@ async function runServer() {
// Continue anyway - we'll use an in-memory config
}

const transport = new FilteredStdioServerTransport();

// Export transport for use throughout the application
global.mcpTransport = transport;
// Handle uncaught exceptions
process.on('uncaughtException', async (error) => {
const errorMessage = error instanceof Error ? error.message : String(error);
Expand Down
13 changes: 8 additions & 5 deletions src/utils/feature-flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ class FeatureFlagManager {
});
}, this.cacheMaxAge);

// Allow process to exit even if interval is pending
// This is critical for proper cleanup when MCP client disconnects
this.refreshInterval.unref();

logger.info(`Feature flags initialized (refresh every ${this.cacheMaxAge / 1000}s)`);
} catch (error) {
logger.warning('Failed to initialize feature flags:', error);
Expand Down Expand Up @@ -107,7 +111,7 @@ class FeatureFlagManager {
*/
private async fetchFlags(): Promise<void> {
try {
logger.debug('Fetching feature flags from:', this.flagUrl);
// Don't log here - runs async and can interfere with MCP clients

const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 5000);
Expand All @@ -132,10 +136,9 @@ class FeatureFlagManager {
this.flags = config.flags;
this.lastFetch = Date.now();

// Save to cache
// Save to cache (silently - don't log during async operations
// as it can interfere with MCP clients that close quickly)
await this.saveToCache(config);

logger.info(`Feature flags updated: ${Object.keys(this.flags).length} flags`);
}
} catch (error: any) {
logger.debug('Failed to fetch feature flags:', error.message);
Expand All @@ -154,7 +157,7 @@ class FeatureFlagManager {
}

await fs.writeFile(this.cachePath, JSON.stringify(config, null, 2), 'utf8');
logger.debug('Saved feature flags to cache');
// Don't log here - this runs async and can cause issues with MCP clients
} catch (error) {
logger.warning('Failed to save feature flags to cache:', error);
}
Expand Down