Skip to content

Commit 05c1a12

Browse files
author
Eric Oliver
committed
fix: use nullish coalescing to preserve zero values in MCP config defaults
- Replace logical OR (||) with nullish coalescing (??) in CLIMcpService - Fixes bug where intentional zero values (timeout: 0, retryAttempts: 0, etc.) were overridden with defaults - Add comprehensive test to verify zero values are preserved while undefined values get defaults - Addresses code reviewer feedback about proper default value handling
1 parent b126407 commit 05c1a12

File tree

2 files changed

+53
-4
lines changed

2 files changed

+53
-4
lines changed

src/cli/services/CLIMcpService.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -273,10 +273,10 @@ export class CLIMcpService implements ICLIMcpService {
273273

274274
return configFile.servers.map((server) => ({
275275
...server,
276-
timeout: server.timeout || this.defaults.timeout,
277-
retryAttempts: server.retryAttempts || this.defaults.retryAttempts,
278-
retryDelay: server.retryDelay || this.defaults.retryDelay,
279-
healthCheckInterval: server.healthCheckInterval || this.defaults.healthCheckInterval,
276+
timeout: server.timeout ?? this.defaults.timeout,
277+
retryAttempts: server.retryAttempts ?? this.defaults.retryAttempts,
278+
retryDelay: server.retryDelay ?? this.defaults.retryDelay,
279+
healthCheckInterval: server.healthCheckInterval ?? this.defaults.healthCheckInterval,
280280
}))
281281
} catch (error) {
282282
if (error.code === "ENOENT") {

src/cli/services/__tests__/CLIMcpService.test.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,55 @@ describe("CLIMcpService", () => {
112112
// Should try home directory config
113113
expect(mockFs.readFile).toHaveBeenCalledWith(path.join(os.homedir(), ".roo", "mcp-config.json"), "utf-8")
114114
})
115+
it("should preserve zero values and not replace them with defaults", async () => {
116+
const mockConfig = {
117+
version: "1.0.0",
118+
servers: [
119+
{
120+
id: "test-server-zero-values",
121+
name: "Test Server with Zero Values",
122+
type: "stdio" as const,
123+
enabled: true,
124+
command: "test-command",
125+
timeout: 0, // Intentionally set to 0
126+
retryAttempts: 0, // Intentionally set to 0
127+
retryDelay: 0, // Intentionally set to 0
128+
healthCheckInterval: 0, // Intentionally set to 0
129+
},
130+
{
131+
id: "test-server-undefined-values",
132+
name: "Test Server with Undefined Values",
133+
type: "stdio" as const,
134+
enabled: true,
135+
command: "test-command",
136+
// No timeout, retryAttempts, retryDelay, healthCheckInterval - should use defaults
137+
},
138+
],
139+
defaults: DEFAULT_MCP_CONFIG,
140+
}
141+
142+
mockFs.readFile.mockResolvedValue(JSON.stringify(mockConfig))
143+
144+
const configs = await service.loadServerConfigs("/test/config.json")
145+
146+
expect(configs).toHaveLength(2)
147+
148+
// First server should preserve zero values
149+
const zeroValueServer = configs[0]
150+
expect(zeroValueServer.id).toBe("test-server-zero-values")
151+
expect(zeroValueServer.timeout).toBe(0)
152+
expect(zeroValueServer.retryAttempts).toBe(0)
153+
expect(zeroValueServer.retryDelay).toBe(0)
154+
expect(zeroValueServer.healthCheckInterval).toBe(0)
155+
156+
// Second server should use defaults for undefined values
157+
const undefinedValueServer = configs[1]
158+
expect(undefinedValueServer.id).toBe("test-server-undefined-values")
159+
expect(undefinedValueServer.timeout).toBe(DEFAULT_MCP_CONFIG.timeout)
160+
expect(undefinedValueServer.retryAttempts).toBe(DEFAULT_MCP_CONFIG.retryAttempts)
161+
expect(undefinedValueServer.retryDelay).toBe(DEFAULT_MCP_CONFIG.retryDelay)
162+
expect(undefinedValueServer.healthCheckInterval).toBe(DEFAULT_MCP_CONFIG.healthCheckInterval)
163+
})
115164
})
116165

117166
describe("validateServerConfig", () => {

0 commit comments

Comments
 (0)