fix: patch SDK models for float state sequence and expandable work item fields#84
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds runtime patching applied at import: Plane SDK Pydantic models are modified to accept float sequences for State.sequence and expanded WorkItem.state shapes; patches are invoked automatically when Changes
Sequence Diagram(s)No sequence diagrams generated. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
This PR applies runtime patches to plane-sdk Pydantic models so plane_mcp can successfully validate real Plane API responses for state sequences (floats) and expanded work item state objects when expand=state is used.
Changes:
- Patch
State/CreateState/UpdateState.sequenceto acceptfloat | Noneand rebuild related paginated response models. - Patch
WorkItem.stateto acceptstr | StateLite | Noneand rebuild related paginated response models. - Update MCP tool signatures for
create_state/update_stateto accept floatsequence.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
plane_mcp/tools/states.py |
Updates tool parameter typing to accept float sequence values. |
plane_mcp/patches.py |
Introduces runtime patching/rebuild logic for plane-sdk models. |
plane_mcp/__init__.py |
Applies patches at package import time to ensure tools see patched models. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| def _rebuild_field(model: type, field_name: str, annotation: type) -> None: | ||
| """Update a Pydantic model field's type annotation and rebuild the validator.""" | ||
| model.__annotations__[field_name] = annotation |
| def apply_patches() -> None: | ||
| """Apply all SDK model patches. Must be called before any tool registration.""" | ||
| _patch_state_sequence() | ||
| _patch_work_item_expandable_fields() |
|
|
||
| from __future__ import annotations | ||
|
|
||
| from plane.models.states import CreateState, PaginatedStateResponse, State, StateLite, UpdateState |
lifeiscontent
force-pushed
the
fix/pydantic-validation-bugs
branch
from
3de2005 to
37806f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/patches.py`:
- Around line 24-31: The _rebuild_field function currently replaces
model.model_fields[field_name] with a new FieldInfo containing only annotation
and default, discarding metadata; instead, grab the existing FieldInfo from
model.model_fields[field_name], call its copy() to clone all metadata, set the
clone's annotation to the new annotation, and reassign that clone back to
model.model_fields[field_name] (also keep updating
model.__annotations__[field_name] as already done) so
description/alias/default_factory/constraints are preserved for the FieldInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d02f1e11-aa53-4624-8733-e473d0f2195c
📒 Files selected for processing (3)
plane_mcp/__init__.pyplane_mcp/patches.pyplane_mcp/tools/states.py
| def _rebuild_field(model: type, field_name: str, annotation: type) -> None: | ||
| """Update a Pydantic model field's type annotation and rebuild the validator.""" | ||
| model.__annotations__[field_name] = annotation | ||
| current = model.model_fields[field_name] | ||
| model.model_fields[field_name] = FieldInfo( | ||
| annotation=annotation, | ||
| default=current.default, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Pydantic v2, what is the supported way to update an existing BaseModel field annotation at runtime while preserving the original FieldInfo metadata (alias, default_factory, constraints, validators)?
💡 Result:
In Pydantic v2, there is no direct supported way to update an existing BaseModel's field annotation at runtime while preserving the original FieldInfo metadata on the same model class. BaseModel field definitions are finalized during class creation and are not designed for runtime mutation of annotations or FieldInfo on the existing class. The recommended approach for scenarios requiring modified field definitions (e.g., changing type while keeping metadata like alias, default_factory, constraints, validators) is to create a new dynamic model using create_model that derives fields from the original model's model_fields. This preserves all original metadata. Use the official pattern from the Pydantic documentation for dynamic models: 1. Iterate over original_model.model_fields. 2. For each field, use field_info.asdict to get a dictionary with annotation, metadata, and attributes. 3. Construct a new annotation with the updated type, preserving metadata and attributes via Annotated and Field(attributes). 4. Pass to create_model(base=original_model, new_fields) to inherit validators etc. Example to update a specific field's annotation to new_annotation while preserving metadata: from typing import Annotated, get_args from pydantic import BaseModel, Field, create_model def update_field_annotation(original_model: type[BaseModel], field_name: str, new_annotation) -> type[BaseModel]: new_fields = {} for f_name, f_info in original_model.model_fields.items: f_dct = f_info.asdict if f_name == field_name: # Update annotation here, preserve metadata/attributes ann = Annotated[new_annotation, *f_dct['metadata'], Field(f_dct['attributes'])] new_fields[f_name] = (ann, None) # or appropriate default else: # Copy others unchanged ann = Annotated[f_dct['annotation'], *f_dct['metadata'], Field(f_dct['attributes'])] new_fields[f_name] = (ann, None) return create_model(original_model.name + 'Updated', base=original_model, new_fields) This creates a new model class with the updated annotation and all original FieldInfo metadata (alias, constraints, validators via inheritance) preserved. Direct mutation of model_fields or FieldInfo (e.g., via field._copy or deepcopy) is unsupported, fragile, and may break across versions (e.g., changes in v2.12). FieldInfo is treated as mostly immutable; while deepcopy or private field._copy can work for creating modified copies for new models, it's not recommended for production as it's not public API.
Citations:
- 1: https://docs.pydantic.dev/latest/examples/dynamic_models/
- 2:
FieldInfo2.12 issues pydantic/pydantic#12374 - 3: Reusing the model field in a new class causes the annotations of the original class change pydantic/pydantic#10187
- 4: How to modify a FieldInfo pydantic/pydantic#12428
🏁 Script executed:
# Check Pydantic version requirements
find . -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" | head -20Repository: makeplane/plane-mcp-server
Length of output: 86
🏁 Script executed:
# Check the patches.py file and surrounding context
cat -n plane_mcp/patches.py 2>/dev/null | head -80Repository: makeplane/plane-mcp-server
Length of output: 3082
🏁 Script executed:
# Search for where _rebuild_field is called
rg "_rebuild_field" --type py -B 2 -A 2Repository: makeplane/plane-mcp-server
Length of output: 1174
🏁 Script executed:
# Search for tests related to patches
fd -e py -p "test" | xargs grep -l "patches\|_rebuild_field" 2>/dev/null | head -10Repository: makeplane/plane-mcp-server
Length of output: 91
🏁 Script executed:
# Check pyproject.toml for Pydantic version
cat pyproject.toml | grep -A 50 dependenciesRepository: makeplane/plane-mcp-server
Length of output: 818
🏁 Script executed:
# Check the test file
cat -n tests/test_patches.pyRepository: makeplane/plane-mcp-server
Length of output: 4618
🏁 Script executed:
# Check if plane-sdk is installed and what Pydantic version it uses
python3 << 'EOF'
try:
import plane
import pydantic
print(f"plane version: {plane.__version__ if hasattr(plane, '__version__') else 'unknown'}")
print(f"pydantic version: {pydantic.__version__}")
except ImportError as e:
print(f"Import error: {e}")
EOFRepository: makeplane/plane-mcp-server
Length of output: 107
🏁 Script executed:
# Let's check the actual field definitions in plane-sdk to see what metadata exists
python3 << 'EOF'
try:
from plane.models.states import State
from plane.models.work_items import WorkItem
# Check State.sequence field
seq_field = State.model_fields.get('sequence')
if seq_field:
print("=== State.sequence field ===")
print(f"annotation: {seq_field.annotation}")
print(f"default: {seq_field.default}")
print(f"alias: {seq_field.alias}")
print(f"default_factory: {seq_field.default_factory}")
print(f"title: {seq_field.title}")
print(f"description: {seq_field.description}")
print(f"metadata: {seq_field.metadata}")
# Check WorkItem.state field
state_field = WorkItem.model_fields.get('state')
if state_field:
print("\n=== WorkItem.state field ===")
print(f"annotation: {state_field.annotation}")
print(f"default: {state_field.default}")
print(f"alias: {state_field.alias}")
print(f"default_factory: {state_field.default_factory}")
print(f"title: {state_field.title}")
print(f"description: {state_field.description}")
print(f"metadata: {state_field.metadata}")
except Exception as e:
print(f"Error: {e}")
import traceback
traceback.print_exc()
EOFRepository: makeplane/plane-mcp-server
Length of output: 235
🏁 Script executed:
# Check if copy() actually works on FieldInfo and preserves all attributes
python3 << 'EOF'
try:
from copy import copy
from plane.models.states import State
original = State.model_fields['sequence']
copied = copy(original)
print("Original:")
print(f" annotation: {original.annotation}")
print(f" default: {original.default}")
print("\nCopied:")
print(f" annotation: {copied.annotation}")
print(f" default: {copied.default}")
# Try to mutate
copied.annotation = float | None
print("\nAfter mutation:")
print(f" copied.annotation: {copied.annotation}")
print(f" original.annotation: {original.annotation}")
except Exception as e:
print(f"Error: {e}")
import traceback
traceback.print_exc()
EOFRepository: makeplane/plane-mcp-server
Length of output: 235
🏁 Script executed:
# Check what happens when we try to inspect plane-sdk without installing
# Look at plane-sdk version and its documentation
python3 << 'EOF'
# Since plane-sdk is not installed, let's check pydantic version available
import pydantic
print(f"Pydantic version in sandbox: {pydantic.__version__}")
# Check if FieldInfo can be mutated and if copy works
from pydantic.fields import FieldInfo
from pydantic import Field
from copy import copy
# Create a test field with metadata
test_field = Field(default=None, alias="test_alias", description="test desc")
print(f"\nTest Field type: {type(test_field)}")
print(f"Test Field: {test_field}")
# Test if we can copy and mutate
if isinstance(test_field, FieldInfo):
copied = copy(test_field)
print(f"\nCopied successfully: {type(copied)}")
# Check if annotation is mutable
try:
copied.annotation = float
print("Annotation mutation succeeded")
except AttributeError as e:
print(f"Annotation mutation failed: {e}")
EOFRepository: makeplane/plane-mcp-server
Length of output: 367
🏁 Script executed:
# Check the actual plane-sdk source code to understand the field definitions
# Look for plane-sdk in pip or check if we can examine it
pip show plane-sdk 2>/dev/null | head -20Repository: makeplane/plane-mcp-server
Length of output: 52
🏁 Script executed:
# Let's examine the review comment's proposed fix more carefully
# Check if there are any issues with the proposed approach
python3 << 'EOF'
from pydantic.fields import FieldInfo
from pydantic import BaseModel, Field
from copy import copy
from typing import Any
# Create a test model to simulate the patching
class TestModel(BaseModel):
sequence: int = Field(default=None, description="test sequence")
print("=== Original field ===")
original_field = TestModel.model_fields['sequence']
print(f"Type: {type(original_field)}")
print(f"annotation: {original_field.annotation}")
print(f"default: {original_field.default}")
print(f"description: {original_field.description}")
print("\n=== After copy() + mutation (proposed fix) ===")
updated = copy(original_field)
updated.annotation = float | None
print(f"Type: {type(updated)}")
print(f"annotation: {updated.annotation}")
print(f"default: {updated.default}")
print(f"description: {updated.description}")
print("\n=== After FieldInfo() recreation (current implementation) ===")
recreated = FieldInfo(
annotation=float | None,
default=original_field.default,
)
print(f"Type: {type(recreated)}")
print(f"annotation: {recreated.annotation}")
print(f"default: {recreated.default}")
print(f"description: {recreated.description}")
# The key difference
print(f"\nDescription preserved with copy()? {updated.description == original_field.description}")
print(f"Description preserved with FieldInfo()? {recreated.description == original_field.description}")
EOFRepository: makeplane/plane-mcp-server
Length of output: 606
Preserve existing FieldInfo metadata when patching field types.
On lines 37–40, the current implementation recreates FieldInfo with only annotation and default, discarding other metadata (e.g., description, alias, default_factory, constraints). This is demonstrated in testing: FieldInfo(annotation=..., default=...) loses the description field.
The proposed fix using copy() plus mutation of the annotation attribute preserves all metadata and is functionally correct in Pydantic 2.12.5. While direct FieldInfo mutation is not part of the official public API, it works reliably in the version in use.
For the specific fields being patched here (State.sequence and WorkItem.state), the fields appear simple without critical metadata, so this is primarily a correctness and robustness concern. However, using the proposed approach ensures future safety if field definitions change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plane_mcp/patches.py` around lines 24 - 31, The _rebuild_field function
currently replaces model.model_fields[field_name] with a new FieldInfo
containing only annotation and default, discarding metadata; instead, grab the
existing FieldInfo from model.model_fields[field_name], call its copy() to clone
all metadata, set the clone's annotation to the new annotation, and reassign
that clone back to model.model_fields[field_name] (also keep updating
model.__annotations__[field_name] as already done) so
description/alias/default_factory/constraints are preserved for the FieldInfo.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_patches.py (1)
77-79: Prefer narrowing the union instead of usingtype: ignore.
work_item.stateis stillstr | StateLite | Noneafter the null check. Add an explicit type guard before accessing.idto keep the test type-safe and remove the ignore.♻️ Suggested update
assert work_item.state is not None - assert work_item.state.id == "fe22aafd-1234" # type: ignore[union-attr] + assert not isinstance(work_item.state, str) + assert work_item.state.id == "fe22aafd-1234"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_patches.py` around lines 77 - 79, The test currently asserts work_item.state is not None but then accesses .id while its type is still str | StateLite | None; replace the type: ignore by narrowing the union with an explicit type guard: check isinstance(work_item.state, StateLite) (or use typing.cast if appropriate) and then assert work_item.state.id == "fe22aafd-1234" inside that branch, or fail the test in the else branch—this removes the ignore and keeps the test type-safe (symbols: work_item, state, StateLite).
🤖 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_patches.py`:
- Around line 121-122: The test currently only checks that response.results
exists and that response.results[0].state is not None; strengthen it by
asserting the parsed expanded state's concrete shape and values: after locating
response.results and the first result (response.results[0]), narrow to its
.state and assert that required fields exist and have expected types/values
(e.g., state["id"] equals the expected id or isinstance(state["id"], str) and
other known keys/types). Update the assertion(s) to validate at least one
concrete field such as state["id"] and optionally state["type"] or similar to
catch parsing regressions.
---
Nitpick comments:
In `@tests/test_patches.py`:
- Around line 77-79: The test currently asserts work_item.state is not None but
then accesses .id while its type is still str | StateLite | None; replace the
type: ignore by narrowing the union with an explicit type guard: check
isinstance(work_item.state, StateLite) (or use typing.cast if appropriate) and
then assert work_item.state.id == "fe22aafd-1234" inside that branch, or fail
the test in the else branch—this removes the ignore and keeps the test type-safe
(symbols: work_item, state, StateLite).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eac6b614-9e7f-4d71-a837-c3c89894ae2f
📒 Files selected for processing (4)
plane_mcp/__init__.pyplane_mcp/patches.pyplane_mcp/tools/states.pytests/test_patches.py
🚧 Files skipped from review as they are similar to previous changes (3)
- plane_mcp/patches.py
- plane_mcp/init.py
- plane_mcp/tools/states.py
lifeiscontent
force-pushed
the
fix/pydantic-validation-bugs
branch
from
37806f4 to
f4a3ab5
Compare
…em fields (makeplane#83) Fix two Pydantic validation bugs: 1. list_states fails because the API returns sequence as a float (the Django model uses FloatField), but the SDK model expects int. 2. list_work_items with expand=state fails because the API returns expanded state objects (dicts), but the SDK WorkItem model expects state to be a plain UUID string. Both fixes are applied as runtime patches to the plane-sdk Pydantic models in a new patches.py module, which is called at package import time. The patches update model field annotations and rebuild validators so the SDK models match what the Plane API actually returns. Closes makeplane#83
lifeiscontent
force-pushed
the
fix/pydantic-validation-bugs
branch
from
f4a3ab5 to
0ce3daa
Compare
Summary
Fixes two Pydantic validation bugs that cause
list_statesandlist_work_items(withexpand) to fail.Closes #83
The root cause is in the
plane-sdkPydantic models — a fix has been submitted upstream: makeplane/plane-python-sdk#23This PR applies runtime patches as a temporary workaround until the SDK fix is released, at which point
plane_mcp/patches.pycan be removed.Bug 1:
list_statesfails with float sequence valuesThe Plane API's
Statemodel uses a DjangoFloatFieldforsequence(e.g.,-57052.5,8482.5), but theplane-sdkPydantic model declares it asint | None, causing validation errors.Bug 2:
list_work_itemswithexpand=statefailsWhen
expand=stateis passed, the API returns expanded state objects (dicts withid,name,color,group), but the SDK'sWorkItemmodel only acceptsstate: str | None(a UUID string). TheWorkItemDetailmodel already handles this correctly (str | StateLite | None), butWorkItem(used byPaginatedWorkItemResponse) does not.Approach
Since the root fixes belong in
plane-sdk(PR: makeplane/plane-python-sdk#23), this PR applies temporary runtime patches to the SDK models at package import time viaplane_mcp/patches.py. The patches:State.sequence,CreateState.sequence, andUpdateState.sequencefromint | Nonetofloat | NoneWorkItem.statefromstr | Nonetostr | StateLite | Nonecreate_stateandupdate_statetool function signatures to acceptfloatforsequencetests/test_patches.pyOnce
plane-sdkis released with the fixes, this PR's patches module should be removed and the SDK version bumped.Summary by CodeRabbit
Bug Fixes
Tests