fix: handle JSON-encoded string for list parameters in MCP tools#76
Open
ej31 wants to merge 6 commits intomakeplane:canaryfrom
Open
fix: handle JSON-encoded string for list parameters in MCP tools#76ej31 wants to merge 6 commits intomakeplane:canaryfrom
ej31 wants to merge 6 commits intomakeplane:canaryfrom
Conversation
Some MCP clients (e.g. Claude Code) serialize list parameters as JSON-encoded strings when invoking tools. This caused a Pydantic validation error in tools that accept `list[str]` parameters: Input should be a valid list [type=list_type, input_value='["uuid-..."]', input_type=str] Affected tools: - add_work_items_to_module (modules.py) - add_work_items_to_cycle (cycles.py) - add_work_items_to_milestone (milestones.py) - remove_work_items_from_milestone (milestones.py) Fix: add a runtime check at the start of each affected function that deserializes the value with json.loads() when it arrives as a string, preserving the original list[str] type annotation for schema generation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds JSON-string parsing for multiple list-like parameters across plane_mcp tools so callers may pass either native lists or JSON-encoded strings; invalid JSON or wrong types now raise explicit ValueError exceptions. Public function signatures remain unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Covers the case where MCP clients pass list parameters as JSON strings. Tests verify both the string-input and native-list-input paths for: - add_work_items_to_module - add_work_items_to_cycle - add_work_items_to_milestone - remove_work_items_from_milestone
Extends the fix from PR makeplane#76 to cover all remaining tool parameters that accept list[str] and may receive JSON-encoded strings from MCP clients. Previously fixed (PR makeplane#76): - add_work_items_to_module (modules.py) - add_work_items_to_cycle (cycles.py) - add_work_items_to_milestone (milestones.py) - remove_work_items_from_milestone (milestones.py) Newly fixed parameters: - create_work_item / update_work_item: assignees, labels (work_items.py) - create_module / update_module: members (modules.py) - create_work_item_relation: issues (work_item_relations.py) - create_epic / update_epic: assignees, labels (epics.py) - create_work_item_property / update_work_item_property: default_value - create_work_item_type / update_work_item_type: project_ids Also backfills the four PR makeplane#76 guards with proper json.JSONDecodeError handling (raise ValueError with chained exception) for consistency. Adds import json to: work_items.py, work_item_relations.py, epics.py, work_item_properties.py, work_item_types.py. Extends test_list_param_fix.py from 5 to 18 tests covering all newly fixed parameters (JSON-string input and native-list regression cases).
Author
|
Following up on this fix — I noticed that the same serialization issue affects several more parameters beyond the four covered here. Additional parameters that were missing the guard:
I also backfilled the four guards introduced here with proper The changes (including 18 unit tests, up from 5) are on the same branch: |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
plane_mcp/tools/modules.py (1)
85-92: Consider extracting a shared coercion helper for JSON-list params.This exact try/except block is duplicated multiple times in this file and across the tool modules. A helper will reduce drift and keep validation/error wording consistent.
♻️ Refactor sketch
+def _coerce_str_list_param(value: list[str] | None, param_name: str) -> list[str] | None: + if isinstance(value, str): + try: + value = json.loads(value) + except json.JSONDecodeError as e: + raise ValueError( + f"{param_name} must be a JSON array string or a list, got: {value!r}" + ) from e + if value is not None and ( + not isinstance(value, list) or any(not isinstance(item, str) for item in value) + ): + raise ValueError(f"{param_name} must be a list[str] or a JSON array string of strings") + return valueAlso applies to: 169-176, 246-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/tools/modules.py` around lines 85 - 92, Extract the duplicated try/except JSON-array coercion into a reusable helper like parse_json_list(value, param_name) that accepts the raw value and parameter name, returns a list when given a list or a JSON array string, and raises ValueError with the same message format ("{param_name} must be a JSON array string or a list, got: {value!r}") when decoding fails; replace each inline block that checks isinstance(members, str) (and the other duplicated blocks) with calls to this helper (e.g., members = parse_json_list(members, "members")) to centralize behavior and error wording across plane_mcp/tools/modules.py.tests/test_list_param_fix.py (2)
4-7: Native-list coverage is inconsistent with the suite description.Line 4-Line 7 says both JSON-string and native-list paths are verified, but only
add_work_items_to_modulehas a native-list test (Line 64-Line 87). Consider parameterizing key tests to run both input forms consistently.Also applies to: 64-87, 89-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_list_param_fix.py` around lines 4 - 7, The test suite claims to verify both JSON-string and native-list inputs but only `add_work_items_to_module` currently covers native lists; update tests in tests/test_list_param_fix.py so the primary list-accepting test cases (including `add_work_items_to_module` and the other tests between the noted ranges) are parametrized to run twice: once with a native Python list and once with a JSON-encoded string of that list. Implement this by adding a pytest parameterization or a small helper/fixture that yields both representations for each list-like argument, then reuse that helper in the test functions to ensure assertions and mock expectations accept both forms (so conversion logic is exercised consistently across all tests).
37-423: Add explicit malformed-JSON tests for the newValueErrorcontract.The implementation now maps decode failures to
ValueError, but this suite only tests valid JSON strings. Please add at least one invalid-input assertion per parsing surface (or a parametrized matrix) so the new error behavior can’t regress silently.Suggested test additions (example pattern)
+import pytest + +@patch("plane_mcp.tools.modules.get_plane_client_context") +def test_add_work_items_to_module_invalid_json_raises_value_error(mock_ctx): + mock_client = make_mock_client() + mock_ctx.return_value = (mock_client, "test-workspace") + + mcp = FastMCP("test") + register_module_tools(mcp) + tool_fn = mcp._tool_manager._tools["add_work_items_to_module"].fn + + with pytest.raises(ValueError, match="issue_ids must be a JSON array string or a list"): + tool_fn(project_id="proj-1", module_id="mod-1", issue_ids='["bad-json"')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_list_param_fix.py` around lines 37 - 423, The test suite currently exercises valid JSON strings but omits negative tests for the new decode-to-ValueError contract; add tests that pass malformed JSON (e.g. '{"bad":') for each parsing surface and assert a ValueError is raised and the underlying mock client method is NOT invoked. For each existing test pattern use the same setup (mock get_plane_client_context, create FastMCP, register_*_tools) and retrieve the tool function via mcp._tool_manager._tools["<tool_name>"].fn (e.g. "add_work_items_to_module", "create_work_item", "update_module", "create_work_item_relation", "create_epic", "create_work_item_property", "create_work_item_type", etc.), wrap the call in pytest.raises(ValueError), and assert the corresponding mock_client.*.<method>.assert_not_called() to ensure malformed input short-circuits before calling the client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plane_mcp/tools/work_item_relations.py`:
- Around line 77-84: After decoding issues, validate its shape: ensure issues is
a list and every element is a string, otherwise raise a ValueError with a clear
message; do this right after the json.loads branch (and also when issues is
already a list) so the rest of the function and SDK models receive a guaranteed
list[str] (refer to the issues variable and the json.loads handling in
work_item_relations.py).
In `@tests/test_list_param_fix.py`:
- Around line 299-337: Add tests in tests/test_list_param_fix.py mirroring
test_create_epic_assignees_json_string and
test_update_epic_assignees_json_string but for the labels field: call
create_epic and update_epic with labels=json.dumps(LABEL_IDS) (or appropriate
constant), assert mock_client.work_items.create.call_args.kwargs["data"].labels
== LABEL_IDS and mock_client.work_items.update.call_args.kwargs["data"].labels
== LABEL_IDS; reuse the same mocking pattern (mock_client, mock_ctx return,
FastMCP/register_epic_tools, and retrieving tool functions
create_epic/update_epic) so the new tests validate JSON-encoded labels are
parsed the same way as assignees.
---
Nitpick comments:
In `@plane_mcp/tools/modules.py`:
- Around line 85-92: Extract the duplicated try/except JSON-array coercion into
a reusable helper like parse_json_list(value, param_name) that accepts the raw
value and parameter name, returns a list when given a list or a JSON array
string, and raises ValueError with the same message format ("{param_name} must
be a JSON array string or a list, got: {value!r}") when decoding fails; replace
each inline block that checks isinstance(members, str) (and the other duplicated
blocks) with calls to this helper (e.g., members = parse_json_list(members,
"members")) to centralize behavior and error wording across
plane_mcp/tools/modules.py.
In `@tests/test_list_param_fix.py`:
- Around line 4-7: The test suite claims to verify both JSON-string and
native-list inputs but only `add_work_items_to_module` currently covers native
lists; update tests in tests/test_list_param_fix.py so the primary
list-accepting test cases (including `add_work_items_to_module` and the other
tests between the noted ranges) are parametrized to run twice: once with a
native Python list and once with a JSON-encoded string of that list. Implement
this by adding a pytest parameterization or a small helper/fixture that yields
both representations for each list-like argument, then reuse that helper in the
test functions to ensure assertions and mock expectations accept both forms (so
conversion logic is exercised consistently across all tests).
- Around line 37-423: The test suite currently exercises valid JSON strings but
omits negative tests for the new decode-to-ValueError contract; add tests that
pass malformed JSON (e.g. '{"bad":') for each parsing surface and assert a
ValueError is raised and the underlying mock client method is NOT invoked. For
each existing test pattern use the same setup (mock get_plane_client_context,
create FastMCP, register_*_tools) and retrieve the tool function via
mcp._tool_manager._tools["<tool_name>"].fn (e.g. "add_work_items_to_module",
"create_work_item", "update_module", "create_work_item_relation", "create_epic",
"create_work_item_property", "create_work_item_type", etc.), wrap the call in
pytest.raises(ValueError), and assert the corresponding
mock_client.*.<method>.assert_not_called() to ensure malformed input
short-circuits before calling the client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54b7f291-b116-42ee-a879-f619b2c9f79d
📒 Files selected for processing (9)
plane_mcp/tools/cycles.pyplane_mcp/tools/epics.pyplane_mcp/tools/milestones.pyplane_mcp/tools/modules.pyplane_mcp/tools/work_item_properties.pyplane_mcp/tools/work_item_relations.pyplane_mcp/tools/work_item_types.pyplane_mcp/tools/work_items.pytests/test_list_param_fix.py
After each json.loads() guard, validate that the decoded value is
actually a list of strings. Valid non-array JSON (e.g. "{}", "42",
"\"id\"") would otherwise pass the JSONDecodeError check silently and
fail later with an unclear error from the SDK or Pydantic.
Required params (issue_ids, issues): raise ValueError immediately if
the decoded value is not a list[str].
Optional params (assignees, labels, members, project_ids,
default_value): raise ValueError when the param is not None and not
a valid list[str].
Also adds two missing tests for create_epic/update_epic labels
parameter (JSON-encoded string), and fixes two pre-existing lint
issues in tests/:
- tests/test_integration.py: bare except -> except Exception
- tests/test_oauth_security.py: unsorted import block
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plane_mcp/tools/modules.py (1)
85-97: Consider extracting shared JSON-list coercion to a helper.The same parsing/validation block is repeated several times here (and in sibling tool modules), which makes future behavior drift likely.
♻️ Refactor sketch
+def _coerce_list_str_param(value: list[str] | str | None, param_name: str) -> list[str] | None: + if isinstance(value, str): + try: + value = json.loads(value) + except json.JSONDecodeError as e: + raise ValueError( + f"{param_name} must be a JSON array string or a list, got: {value!r}" + ) from e + if value is not None and (not isinstance(value, list) or any(not isinstance(i, str) for i in value)): + raise ValueError(f"{param_name} must be a list[str] or a JSON array string of strings") + return value + - if isinstance(members, str): - try: - members = json.loads(members) - except json.JSONDecodeError as e: - raise ValueError( - f"members must be a JSON array string or a list, got: {members!r}" - ) from e - if members is not None and ( - not isinstance(members, list) or any(not isinstance(i, str) for i in members) - ): - raise ValueError("members must be a list[str] or a JSON array string of strings") + members = _coerce_list_str_param(members, "members")Also applies to: 173-185, 254-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/tools/modules.py` around lines 85 - 97, Extract the repeated JSON-string-to-list coercion/validation into a single helper function (e.g., parse_str_list or coerce_members_to_list) in plane_mcp.tools.modules that accepts the raw members value, returns a list[str] or raises ValueError, and encapsulates the try/json.loads + type checks; replace the inline blocks that inspect/JSON-decode `members` (the occurrences around the shown diff and the similar blocks at the other occurrences) to call this helper instead; update any sibling tool modules to import and reuse this helper so all callers use the same parsing/validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_integration.py`:
- Around line 47-48: The except block currently catches all exceptions when
parsing JSON (around the json.loads call handling content.text) which is too
broad; change the error handling to catch only json.JSONDecodeError and
TypeError (i.e., replace "except Exception:" with "except (json.JSONDecodeError,
TypeError):") so only expected parsing/typing errors fall back to returning
{"raw": content.text}; ensure json is imported and the handling is applied in
the function that calls json.loads with content.text.
---
Nitpick comments:
In `@plane_mcp/tools/modules.py`:
- Around line 85-97: Extract the repeated JSON-string-to-list
coercion/validation into a single helper function (e.g., parse_str_list or
coerce_members_to_list) in plane_mcp.tools.modules that accepts the raw members
value, returns a list[str] or raises ValueError, and encapsulates the
try/json.loads + type checks; replace the inline blocks that inspect/JSON-decode
`members` (the occurrences around the shown diff and the similar blocks at the
other occurrences) to call this helper instead; update any sibling tool modules
to import and reuse this helper so all callers use the same parsing/validation
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53ba0fd6-2a98-4e47-a73e-d4c228865061
📒 Files selected for processing (11)
plane_mcp/tools/cycles.pyplane_mcp/tools/epics.pyplane_mcp/tools/milestones.pyplane_mcp/tools/modules.pyplane_mcp/tools/work_item_properties.pyplane_mcp/tools/work_item_relations.pyplane_mcp/tools/work_item_types.pyplane_mcp/tools/work_items.pytests/test_integration.pytests/test_list_param_fix.pytests/test_oauth_security.py
💤 Files with no reviewable changes (1)
- tests/test_oauth_security.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_list_param_fix.py
Replace broad `except Exception` with `except (json.JSONDecodeError, TypeError)` to avoid masking unrelated failures during integration debugging. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Alegrowin
added a commit
to Alegrowin/plane-mcp-server
that referenced
this pull request
Mar 7, 2026
Pages (new methods): - list_workspace_pages / list_project_pages (with pagination) - update_workspace_page / update_project_page (partial updates via PATCH) - delete_workspace_page / delete_project_page Uses plane-sdk internal HTTP methods (_get, _patch, _delete) since the SDK's Pages resource class doesn't expose list/update/delete yet. Handles both paginated dict and plain list API responses. Cherry-picked from upstream PR makeplane#76 (ej31): - Fix JSON-encoded string handling for list parameters in MCP tools - Affects cycles, epics, milestones, modules, work_items, work_item_properties, work_item_relations, work_item_types Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Hey @ej31 — just realized we independently hit the same issue and opened #79 with a broader fix covering all 16 affected functions across 8 files. We're closing ours since yours was first and correctly targets Your follow-up comment already identified the additional parameters. Here's the full list we found, in case it's helpful for expanding your PR:
We also extracted the guard into a shared utility ( Our branch with the full implementation is at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's the problem?
Some MCP clients (e.g. Claude Code) serialize
listparameters as JSON-encoded strings when invoking tools. This causes a Pydantic validation error at runtime:The type annotation
list[str]is correct and FastMCP generates the right JSON schema from it. The issue is that some clients wrap["id1", "id2"]into a string'["id1", "id2"]'before sending it over the wire, and Pydantic rejects that.Affected tools
add_work_items_to_module(modules.py)add_work_items_to_cycle(cycles.py)add_work_items_to_milestone(milestones.py)remove_work_items_from_milestone(milestones.py)Fix
Added a small runtime guard at the top of each affected function that calls
json.loads()when the value arrives as astr. It's fully backward-compatible:list→ works exactly as beforeThe type annotation is intentionally kept as
list[str]so the tool schema exposed to MCP clients stays unchanged.Testing
Reproduced the failure with Claude Code where
add_work_items_to_modulewas consistently erroring out. Confirmed it works correctly after the fix.Also added unit tests (
tests/test_list_param_fix.py) covering:issue_idspassed as a JSON-encoded string → parsed and forwarded correctlyissue_idspassed as a nativelist→ works as beforeAll 5 tests pass ✅
Summary by CodeRabbit
New Features
Tests