Fix set config schema to work in gemini and few other tools #236
Conversation
WalkthroughRefines setConfigValue parsing for array-like settings in src/tools/config.ts, adding stricter JSON parsing and scalar-to-string coercion. Tightens input validation by narrowing SetConfigValueArgsSchema.value types in src/tools/schemas.ts from any to a specific union. No exported function signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Config as setConfigValue
participant JSON as JSON.parse
participant Log as Logger
participant Store as ConfigStore
Caller->>Config: setConfigValue(key, value)
alt value is array
Config->>Store: save array value
else value is string
Config->>JSON: parse(originalString)
alt parse succeeds
JSON-->>Config: parsed (array/object)
Config->>Store: save parsed
else parse fails
JSON-->>Config: error
Config->>Log: log parse error
alt originalString lacks '['
Config->>Store: save [originalString]
else
Config->>Store: save originalString (string)
end
end
else value is non-null, non-string scalar
Config->>Store: save [String(value)]
else value is null
Config->>Store: save null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/config.ts (1)
86-110: Array-key normalization can yield ["[object Object]"] and mishandle null; enforce string[]If a JSON string like "{...}" is parsed earlier, this branch converts the object to
["[object Object]"]. Also,nullbecomes["null"]. ForallowedDirectories/blockedCommands, normalize tostring[], treatnullas[], and only accept parsed arrays (fallback to single-item arrays otherwise).Apply:
- if ((parsed.data.key === 'allowedDirectories' || parsed.data.key === 'blockedCommands') && - !Array.isArray(valueToStore)) { - if (typeof valueToStore === 'string') { - const originalString = valueToStore; - try { - const parsedValue = JSON.parse(originalString); - valueToStore = parsedValue; - } catch (parseError) { - console.error(`Failed to parse string as array for ${parsed.data.key}: ${parseError}`); - // If parsing failed and it's a single value, convert to an array with one item - if (!originalString.includes('[')) { - valueToStore = [originalString]; - } - } - } else if (valueToStore !== null) { - // If not a string or array (and not null), convert to an array with one item - valueToStore = [String(valueToStore)]; - } - - // Ensure the value is an array after all our conversions - if (!Array.isArray(valueToStore)) { - console.error(`Value for ${parsed.data.key} is still not an array, converting to array`); - valueToStore = [String(valueToStore)]; - } - } + if (parsed.data.key === 'allowedDirectories' || parsed.data.key === 'blockedCommands') { + // Normalize strictly to string[] + if (valueToStore == null) { + valueToStore = []; + } else if (Array.isArray(valueToStore)) { + valueToStore = valueToStore.map(v => String(v)); + } else if (typeof valueToStore === 'object') { + // Avoid leaking "[object Object]" into config + console.error(`Invalid object for ${parsed.data.key}; expected string or array. Coercing to empty array.`); + valueToStore = []; + } else if (typeof valueToStore === 'string') { + const s = valueToStore.trim(); + if (s.startsWith('[')) { + try { + const parsedValue = JSON.parse(s); + valueToStore = Array.isArray(parsedValue) ? parsedValue.map(v => String(v)) : [s]; + } catch { + valueToStore = [s]; + } + } else { + valueToStore = [s]; + } + } else { + valueToStore = [String(valueToStore)]; + } + }
🧹 Nitpick comments (2)
src/tools/config.ts (2)
89-93: Redundant re-parse and lost original input
JSON.parsemay already have run earlier (Lines 75–83). Re-parsing here usingoriginalStringonly happens if the value is still a string; if it was already parsed to an object, you’ve lost the raw string. Capture the raw input once before any parsing and reuse it, or trim and parse in one place.Example:
- let valueToStore = parsed.data.value; + let valueToStore = parsed.data.value; + const rawInput = typeof parsed.data.value === 'string' ? parsed.data.value : undefined;Then prefer
rawInput?.trim()when parsing.
115-121: Avoid echoing full config (PII/paths) in success responseReturning
updatedConfigverbatim can leak sensitive paths/client info to tool logs. Prefer echoing only the changed key or redact sensitive fields.- text: `Successfully set ${parsed.data.key} to ${JSON.stringify(valueToStore, null, 2)}\n\nUpdated configuration:\n${JSON.stringify(updatedConfig, null, 2)}` + text: `Successfully set ${parsed.data.key} to ${JSON.stringify(valueToStore, null, 2)}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/tools/config.ts(1 hunks)src/tools/schemas.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/tools/config.ts (3)
test/test-allowed-directories.js (2)
testSpecificAllowedDirectory(139-167)testAllowedDirectories(364-386)test/test-blocked-commands.js (3)
testNonBlockedCommands(144-171)testEmptyBlockedCommands(231-250)runBlockedCommandsTests(255-271)src/config-manager.ts (1)
ServerConfig(9-19)
🔇 Additional comments (1)
src/tools/schemas.ts (1)
8-14: Validate external callers for new schema restrictions. No internal usages ofsetConfigValueor"set_config_value"passing complex objects or non-string arrays were found; confirm whether external clients rely on objects or other array types before narrowing this schema. Consider per-key schemas or expanding the union to include objects/non-string arrays.
Summary by CodeRabbit