Open
Conversation
📝 WalkthroughWalkthroughConsolidates numerous per-resource "list" and "retrieve" MCP tools into a new configuration-driven pair of tools ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client (MCP Caller)
participant MCP as FastMCP (tool registry)
participant Unified as entity_{list,retrieve} tool
participant Plane as PlaneClient
Caller->>MCP: invoke entity_list(entity_type, scope ids, params?)
MCP->>Unified: route to entity_list
Unified->>Plane: resolve client attr (e.g., client.work_items)
Unified->>Plane: call configured list method (with workspace_slug + scope ids + params?)
Plane-->>Unified: returns result / paginated response
Unified-->>MCP: normalize and return results
MCP-->>Caller: deliver list response
Caller->>MCP: invoke entity_retrieve(entity_type, entity_id or identifiers)
MCP->>Unified: route to entity_retrieve
Unified->>Plane: resolve client attr and call retrieve method (special-case identifier flow if needed)
Plane-->>Unified: returns single entity
Unified-->>MCP: return entity
MCP-->>Caller: deliver entity response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Apr 12, 2026
Docstrings generation was requested by @sunder-ch. * #103 (comment) The following files were modified: * `plane_mcp/tools/__init__.py` * `plane_mcp/tools/cycles.py` * `plane_mcp/tools/epics.py` * `plane_mcp/tools/initiatives.py` * `plane_mcp/tools/intake.py` * `plane_mcp/tools/labels.py` * `plane_mcp/tools/milestones.py` * `plane_mcp/tools/modules.py` * `plane_mcp/tools/projects.py` * `plane_mcp/tools/states.py` * `plane_mcp/tools/unified.py` * `plane_mcp/tools/work_item_activities.py` * `plane_mcp/tools/work_item_comments.py` * `plane_mcp/tools/work_item_links.py` * `plane_mcp/tools/work_item_properties.py` * `plane_mcp/tools/work_item_relations.py` * `plane_mcp/tools/work_item_types.py` * `plane_mcp/tools/work_items.py` * `plane_mcp/tools/work_logs.py` * `plane_mcp/tools/workspaces.py`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plane_mcp/tools/work_item_activities.py (1)
6-8: Consider removing this empty stub module.This registration function is now a no-op since all tools have been migrated to the unified interface. Consider removing this file and its corresponding import/call in
__init__.pyto reduce dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/tools/work_item_activities.py` around lines 6 - 8, The function register_work_item_activity_tools in plane_mcp.tools.work_item_activities is now a no-op and should be removed along with the empty module: delete plane_mcp/tools/work_item_activities.py and remove any import and invocation of register_work_item_activity_tools from the package initializer (__init__.py) or other call sites; ensure there are no remaining references to register_work_item_activity_tools (search for that symbol) and run tests/linters to confirm nothing else depends on it.plane_mcp/tools/unified.py (1)
158-158: Unusedcontainer_id_kwargvariable - consider refactoring or removing from config.The static analyzer correctly flags that
container_id_kwargis unpacked but never used. The kwarg names are instead hardcoded in the scope-based logic at lines 184-193. Either:
- Use
container_id_kwargto dynamically build the kwargs (removing duplication), or- Remove
container_id_kwargfromENTITY_LIST_CONFIGif it's not needed.Option 1: Use container_id_kwarg dynamically
- attr_path, method_name, container_id_kwarg, scope = ENTITY_LIST_CONFIG[entity_type] + attr_path, method_name, container_id_kwarg, scope = ENTITY_LIST_CONFIG[entity_type] method_key = f"{attr_path}.{method_name}" # ... validation code ... # Build kwargs kwargs: dict[str, Any] = {"workspace_slug": workspace_slug} if scope in ("project", "cycle", "module", "milestone", "work_item", "work_item_type"): kwargs["project_id"] = project_id - if scope == "cycle": - kwargs["cycle_id"] = cycle_id - elif scope == "module": - kwargs["module_id"] = module_id - elif scope == "milestone": - kwargs["milestone_id"] = milestone_id - elif scope == "work_item": - kwargs["work_item_id"] = work_item_id - elif scope == "work_item_type": - kwargs["type_id"] = type_id + + # Add container ID using the configured kwarg name + if container_id_kwarg: + container_id_map = { + "cycle_id": cycle_id, + "module_id": module_id, + "milestone_id": milestone_id, + "work_item_id": work_item_id, + "type_id": type_id, + } + kwargs[container_id_kwarg] = container_id_map[container_id_kwarg]Option 2: Remove unused field from config (simpler)
-# Config format: entity_type -> (client_attr_path, method, container_id_kwarg, scope) -# scope: "workspace" | "project" | "cycle" | "module" | "milestone" | "work_item" | "work_item_type" -# container_id_kwarg: the kwarg name for the container ID (cycle_id, module_id, etc.), or None if not needed -ENTITY_LIST_CONFIG: dict[str, tuple[str, str, str | None, str]] = { - "projects": ("projects", "list", None, "workspace"), +# Config format: entity_type -> (client_attr_path, method, scope) +ENTITY_LIST_CONFIG: dict[str, tuple[str, str, str]] = { + "projects": ("projects", "list", "workspace"), # ... update all entries similarlyThen update line 158:
- attr_path, method_name, container_id_kwarg, scope = ENTITY_LIST_CONFIG[entity_type] + attr_path, method_name, scope = ENTITY_LIST_CONFIG[entity_type]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/tools/unified.py` at line 158, ENTITY_LIST_CONFIG currently unpacks container_id_kwarg but the scope-based branches use hardcoded kwarg names; update the scope-handling logic in the same function (where attr_path, method_name, container_id_kwarg, scope are unpacked) to build kwargs dynamically using container_id_kwarg (e.g. kwargs = {container_id_kwarg: container_id} or {container_id_kwarg: board_id} depending on scope) and pass that kwargs into the calls instead of hardcoded names, so container_id_kwarg is actually used and duplication is removed; ensure ENTITY_LIST_CONFIG stays unchanged and remove any now-unused hardcoded variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plane_mcp/tools/unified.py`:
- Line 158: ENTITY_LIST_CONFIG currently unpacks container_id_kwarg but the
scope-based branches use hardcoded kwarg names; update the scope-handling logic
in the same function (where attr_path, method_name, container_id_kwarg, scope
are unpacked) to build kwargs dynamically using container_id_kwarg (e.g. kwargs
= {container_id_kwarg: container_id} or {container_id_kwarg: board_id} depending
on scope) and pass that kwargs into the calls instead of hardcoded names, so
container_id_kwarg is actually used and duplication is removed; ensure
ENTITY_LIST_CONFIG stays unchanged and remove any now-unused hardcoded
variables.
In `@plane_mcp/tools/work_item_activities.py`:
- Around line 6-8: The function register_work_item_activity_tools in
plane_mcp.tools.work_item_activities is now a no-op and should be removed along
with the empty module: delete plane_mcp/tools/work_item_activities.py and remove
any import and invocation of register_work_item_activity_tools from the package
initializer (__init__.py) or other call sites; ensure there are no remaining
references to register_work_item_activity_tools (search for that symbol) and run
tests/linters to confirm nothing else depends on it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 959e531b-f6ad-48a2-9344-e060cda24015
📒 Files selected for processing (20)
plane_mcp/tools/__init__.pyplane_mcp/tools/cycles.pyplane_mcp/tools/epics.pyplane_mcp/tools/initiatives.pyplane_mcp/tools/intake.pyplane_mcp/tools/labels.pyplane_mcp/tools/milestones.pyplane_mcp/tools/modules.pyplane_mcp/tools/projects.pyplane_mcp/tools/states.pyplane_mcp/tools/unified.pyplane_mcp/tools/work_item_activities.pyplane_mcp/tools/work_item_comments.pyplane_mcp/tools/work_item_links.pyplane_mcp/tools/work_item_properties.pyplane_mcp/tools/work_item_relations.pyplane_mcp/tools/work_item_types.pyplane_mcp/tools/work_items.pyplane_mcp/tools/work_logs.pyplane_mcp/tools/workspaces.py
💤 Files with no reviewable changes (13)
- plane_mcp/tools/work_item_relations.py
- plane_mcp/tools/workspaces.py
- plane_mcp/tools/work_logs.py
- plane_mcp/tools/initiatives.py
- plane_mcp/tools/work_item_properties.py
- plane_mcp/tools/work_item_types.py
- plane_mcp/tools/work_item_links.py
- plane_mcp/tools/intake.py
- plane_mcp/tools/milestones.py
- plane_mcp/tools/labels.py
- plane_mcp/tools/cycles.py
- plane_mcp/tools/projects.py
- plane_mcp/tools/work_item_comments.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plane_mcp/tools/unified.py (3)
69-77: Normalize list responses by shape instead of an allowlist.Line 199 makes pagination behavior depend on
_DIRECT_RETURN_METHODS, which is another table to keep in sync with the SDK. Returningresult.resultswhenever that attribute exists is safer and still preserves the list-only contract for tool consumers.♻️ Proposed simplification
- if method_key in _DIRECT_RETURN_METHODS: - return result - return result.results + if hasattr(result, "results"): + return result.results + return resultBased on learnings "In plane_mcp/tools, ensure list endpoints (e.g., list_cycles, list_work_items, list_initiatives, list_epics, etc.) return only response.results (the list of items) and do not include full pagination metadata. Pagination should be handled by passing the cursor parameter on subsequent calls, not via metadata in the initial response."
Also applies to: 197-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/tools/unified.py` around lines 69 - 77, Replace the allowlist-based branching that checks _DIRECT_RETURN_METHODS with a shape-based check: if the SDK response object has a results attribute, return result.results (the list) directly; otherwise return the full response. Locate the code referencing _DIRECT_RETURN_METHODS and the conditional around result/results (the block that currently decides pagination behavior) and remove the allowlist check so the presence of result.results alone dictates returning the list-only payload, preserving callers that expect just the items and leaving pagination to cursor parameters.
12-60: Collapse the dispatch metadata into one source of truth.The supported entity names, SDK routing,
paramssupport, and bothLiteral[...]enums are all maintained separately here. A missed edit later will either expose a value that fails withKeyErroror silently apply the wrong behavior for a valid entity. A single typed descriptor per entity would remove most of that drift.Also applies to: 62-67, 93-118, 205-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/tools/unified.py` around lines 12 - 60, Replace the two parallel dicts ENTITY_LIST_CONFIG and ENTITY_RETRIEVE_CONFIG with a single typed descriptor mapping (e.g., a dataclass or TypedDict named EntityDescriptor with fields: client_attr_path, method, entity_id_kwarg, scope, supports_params, operation_type) and a single registry like ENTITY_DESCRIPTORS: dict[str, EntityDescriptor]; populate entries formerly in both dicts (use operation_type/list vs retrieve to distinguish). Update any code that referenced ENTITY_LIST_CONFIG or ENTITY_RETRIEVE_CONFIG to read the descriptor and branch on descriptor.operation_type (or descriptor.method) for routing (ensure callers that expected entity_id_kwarg, scope, or params use those descriptor fields). Derive any Literal[...] enums or supported-entity sets from ENTITY_DESCRIPTORS.keys() so there’s one source of truth and update usages that referenced "client_attr_path" names like "work_items.activities" accordingly.
125-126: Keep the tool return types concrete.
list[Any]andAnyerase the plane-sdk model contract that the rest ofplane_mcp/toolsexposes. Please use typed unions or wrapper models instead of returning opaqueAnyhere.As per coding guidelines "Tool functions must return Pydantic models from plane-sdk".
Also applies to: 231-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/tools/unified.py` around lines 125 - 126, The function signature currently uses opaque types (params: dict[str, Any] | None and -> list[Any]) which violates the "Tool functions must return Pydantic models from plane-sdk" rule; replace the return annotation and any inner Any usages with concrete pydantic model types (or a union/wrapper model) from plane-sdk (e.g., MyToolResultModel or Union[ModelA, ModelB]) and update the implementation to construct and return those models instead of raw dicts/Any; do the same for the other occurrence noted (the second signature at the referenced spot) and add the necessary imports for the plane-sdk models.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plane_mcp/tools/unified.py`:
- Around line 69-77: Replace the allowlist-based branching that checks
_DIRECT_RETURN_METHODS with a shape-based check: if the SDK response object has
a results attribute, return result.results (the list) directly; otherwise return
the full response. Locate the code referencing _DIRECT_RETURN_METHODS and the
conditional around result/results (the block that currently decides pagination
behavior) and remove the allowlist check so the presence of result.results alone
dictates returning the list-only payload, preserving callers that expect just
the items and leaving pagination to cursor parameters.
- Around line 12-60: Replace the two parallel dicts ENTITY_LIST_CONFIG and
ENTITY_RETRIEVE_CONFIG with a single typed descriptor mapping (e.g., a dataclass
or TypedDict named EntityDescriptor with fields: client_attr_path, method,
entity_id_kwarg, scope, supports_params, operation_type) and a single registry
like ENTITY_DESCRIPTORS: dict[str, EntityDescriptor]; populate entries formerly
in both dicts (use operation_type/list vs retrieve to distinguish). Update any
code that referenced ENTITY_LIST_CONFIG or ENTITY_RETRIEVE_CONFIG to read the
descriptor and branch on descriptor.operation_type (or descriptor.method) for
routing (ensure callers that expected entity_id_kwarg, scope, or params use
those descriptor fields). Derive any Literal[...] enums or supported-entity sets
from ENTITY_DESCRIPTORS.keys() so there’s one source of truth and update usages
that referenced "client_attr_path" names like "work_items.activities"
accordingly.
- Around line 125-126: The function signature currently uses opaque types
(params: dict[str, Any] | None and -> list[Any]) which violates the "Tool
functions must return Pydantic models from plane-sdk" rule; replace the return
annotation and any inner Any usages with concrete pydantic model types (or a
union/wrapper model) from plane-sdk (e.g., MyToolResultModel or Union[ModelA,
ModelB]) and update the implementation to construct and return those models
instead of raw dicts/Any; do the same for the other occurrence noted (the second
signature at the referenced spot) and add the necessary imports for the
plane-sdk models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2004b7e-b0c8-45f1-888c-4d7695db5b30
📒 Files selected for processing (3)
plane_mcp/tools/__init__.pyplane_mcp/tools/unified.pyplane_mcp/tools/work_item_activities.py
💤 Files with no reviewable changes (1)
- plane_mcp/tools/work_item_activities.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plane_mcp/tools/init.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plane_mcp/tools/unified.py (1)
12-37: Keep each list endpoint’s behavior in one config entry.
ENTITY_LIST_CONFIG,_NO_PARAMS_METHODS, and_DIRECT_RETURN_METHODSare three separate sources of truth for the same dispatch record. For example,work_items.relations.listhas to be declared in all three places today. Missing one later turns into a runtimeTypeErrororresult.resultscrash in the central read path. I’d foldsupports_paramsanddirect_returninto the list config itself.Also applies to: 62-77, 160-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/tools/unified.py` around lines 12 - 37, The three separate sources of truth (ENTITY_LIST_CONFIG, _NO_PARAMS_METHODS, _DIRECT_RETURN_METHODS) create bugs when a list endpoint isn’t declared in all places; modify ENTITY_LIST_CONFIG entries to include two new fields (supports_params: bool and direct_return: bool) in each tuple, update any code that previously read _NO_PARAMS_METHODS or _DIRECT_RETURN_METHODS to read these new tuple fields instead (e.g., the central read/dispatch logic that inspects method names and expects result.results), and remove the now-redundant _NO_PARAMS_METHODS and _DIRECT_RETURN_METHODS constants; ensure you adjust all call sites that construct requests (e.g., where cycle_work_items, module_work_items, work_item_relations, work_items.* entries are used) to honor the new flags so missing declarations no longer cause TypeError or result.results crashes.
🤖 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/unified.py`:
- Line 125: The params parameter is being used to tunnel arbitrary filters;
enforce its contract by validating and rejecting unexpected keys instead of
forwarding them. In the functions/methods that accept params (the parameter
named params: dict[str, Any] | None and any callers that reference
_NO_PARAMS_METHODS), add validation logic: if the current method/endpoint is
listed in _NO_PARAMS_METHODS and params is not None/empty, raise a ValueError;
otherwise, require params to be either None or contain only an explicit allowed
set of keys (whitelist) and raise on any unknown keys. Locate the params
handling code paths (where params is passed through to the SDK and where
_NO_PARAMS_METHODS is referenced) and replace the passthrough with this
validation/whitelisting and explicit error raises.
---
Nitpick comments:
In `@plane_mcp/tools/unified.py`:
- Around line 12-37: The three separate sources of truth (ENTITY_LIST_CONFIG,
_NO_PARAMS_METHODS, _DIRECT_RETURN_METHODS) create bugs when a list endpoint
isn’t declared in all places; modify ENTITY_LIST_CONFIG entries to include two
new fields (supports_params: bool and direct_return: bool) in each tuple, update
any code that previously read _NO_PARAMS_METHODS or _DIRECT_RETURN_METHODS to
read these new tuple fields instead (e.g., the central read/dispatch logic that
inspects method names and expects result.results), and remove the now-redundant
_NO_PARAMS_METHODS and _DIRECT_RETURN_METHODS constants; ensure you adjust all
call sites that construct requests (e.g., where cycle_work_items,
module_work_items, work_item_relations, work_items.* entries are used) to honor
the new flags so missing declarations no longer cause TypeError or
result.results crashes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdd63bf1-7d4e-4876-acef-09f2cab0d5dc
📒 Files selected for processing (1)
plane_mcp/tools/unified.py
| milestone_id: str | None = None, | ||
| work_item_id: str | None = None, | ||
| type_id: str | None = None, | ||
| params: dict[str, Any] | None = None, |
There was a problem hiding this comment.
Enforce the params contract instead of tunneling arbitrary filters through it.
Right now the schema/docstring says filtering is unsupported, but the implementation still forwards any keys straight to the SDK. That means callers can sneak filter params back in, and endpoints in _NO_PARAMS_METHODS will silently ignore supplied params instead of failing fast.
Suggested fix
+ if params is not None and method_key in _NO_PARAMS_METHODS:
+ raise ValueError(f"params is not supported for entity_type='{entity_type}'")
+
+ allowed_params = {"per_page", "cursor", "order_by", "expand", "fields"}
+ sanitized_params: dict[str, Any] | None = None
+ if params is not None:
+ unsupported = set(params) - allowed_params
+ if unsupported:
+ unsupported_list = ", ".join(sorted(unsupported))
+ raise ValueError(
+ f"Unsupported params for entity_list: {unsupported_list}. "
+ "Only the documented pagination/shape params are allowed."
+ )
+ sanitized_params = {key: value for key, value in params.items() if value is not None}
+
- if method_key not in _NO_PARAMS_METHODS:
- kwargs["params"] = params
+ if method_key not in _NO_PARAMS_METHODS and sanitized_params is not None:
+ kwargs["params"] = sanitized_paramsAlso applies to: 151-153, 196-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plane_mcp/tools/unified.py` at line 125, The params parameter is being used
to tunnel arbitrary filters; enforce its contract by validating and rejecting
unexpected keys instead of forwarding them. In the functions/methods that accept
params (the parameter named params: dict[str, Any] | None and any callers that
reference _NO_PARAMS_METHODS), add validation logic: if the current
method/endpoint is listed in _NO_PARAMS_METHODS and params is not None/empty,
raise a ValueError; otherwise, require params to be either None or contain only
an explicit allowed set of keys (whitelist) and raise on any unknown keys.
Locate the params handling code paths (where params is passed through to the SDK
and where _NO_PARAMS_METHODS is referenced) and replace the passthrough with
this validation/whitelisting and explicit error raises.
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
1 participant
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.
Description
Creates two new tools:
These replace individual list and retrieve tools from each of the entity types.
Reduces total tool count to ~20
Summary by CodeRabbit
New Features
Refactor
Breaking Changes