Fix: HTTP transport for FastMCP and Pad Sketch in PartDesign - #84
Conversation
📝 WalkthroughWalkthroughHTTP transport startup now configures FastMCP via the ChangesHTTP Transport Configuration
Pad Sketch Tool API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/freecad_mcp/tools/partdesign.py (2)
268-285: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd regression tests for the new
side_typeAPI surface.Current coverage (see
tests/unit/test_tools_partdesign.py:196-219) exercises only the default"One side"path. Please add tests for"Two side","Symmetric", and an invalidside_typeinput.As per coding guidelines, "All code must have tests - create unit tests, integration tests, edge cases, and regression tests".
Also applies to: 321-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/freecad_mcp/tools/partdesign.py` around lines 268 - 285, The test suite currently only covers pad_sketch's default "One side" behavior; add unit tests in tests/unit/test_tools_partdesign.py that call pad_sketch (the async function) with side_type="Two side", side_type="Symmetric", and with an invalid side_type value to exercise error handling; for the valid variations assert the returned dict contains the expected extrusion parameters (e.g., correct length distribution / flags that indicate two-sided or symmetric behavior and reversed handling when reversed=True) and for the invalid input assert that the function raises the appropriate exception or returns the expected error structure, mirroring how existing tests assert on pad_sketch's result for "One side".
271-321:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
side_typebefore sending it to FreeCAD.The docstring constrains valid values, but Line 321 forwards any string to
pad.SideType. Invalid values fail later insideexecute_pythonwith a less actionable error path.Suggested patch
async def pad_sketch( sketch_name: str, length: float, side_type: str = "One side", @@ ) -> dict[str, Any]: @@ - bridge = await get_bridge() + allowedSideTypes = {"One side", "Two side", "Symmetric"} + if side_type not in allowedSideTypes: + raise ValueError( + "Invalid side_type. Use one of: One side, Two side, Symmetric." + ) + + bridge = await get_bridge()
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/freecad_mcp/server.py`:
- Around line 427-435: Add a regression test that exercises the HTTP startup
path where config.transport == TransportType.HTTP: instantiate or mock a config
with http_port and log_level, patch os.environ (or capture environment changes)
and mock mcp.run, then call the server startup function (the function that
contains this block which sets os.environ and calls mcp.run) and assert that
os.environ["FASTMCP_HOST"] == "0.0.0.0", os.environ["FASTMCP_PORT"] ==
str(config.http_port), os.environ["FASTMCP_LOG_LEVEL"] == config.log_level, and
that mcp.run was invoked with transport="streamable-http"; use deterministic
cleanup/reset of environment and restore mocks after the test.
- Around line 428-435: The current code sets FASTMCP_* environment variables
after the FastMCP instance (`mcp`) and its Pydantic `Settings` were already
instantiated, so those env vars have no effect; instead, mutate `mcp.settings`
directly (set `mcp.settings.host`, `mcp.settings.port`, and
`mcp.settings.log_level` from `config.http_port`/`config.log_level`) before
calling `mcp.run(transport="streamable-http")`; also update the test
`test_main_http_transport` to stop asserting a non-existent `port` argument on
`mcp.run()` and instead assert that `mcp.settings.port`, `mcp.settings.host`,
and `mcp.settings.log_level` are set correctly prior to `mcp.run()` being
invoked.
---
Outside diff comments:
In `@src/freecad_mcp/tools/partdesign.py`:
- Around line 268-285: The test suite currently only covers pad_sketch's default
"One side" behavior; add unit tests in tests/unit/test_tools_partdesign.py that
call pad_sketch (the async function) with side_type="Two side",
side_type="Symmetric", and with an invalid side_type value to exercise error
handling; for the valid variations assert the returned dict contains the
expected extrusion parameters (e.g., correct length distribution / flags that
indicate two-sided or symmetric behavior and reversed handling when
reversed=True) and for the invalid input assert that the function raises the
appropriate exception or returns the expected error structure, mirroring how
existing tests assert on pad_sketch's result for "One side".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e55f022-310c-43a6-bd25-75866edee708
📒 Files selected for processing (2)
src/freecad_mcp/server.pysrc/freecad_mcp/tools/partdesign.py
- Use mcp.settings.host/port/log_level instead of os.environ - Update test to verify settings are set correctly before run() - Fixes ineffective workaround for mcp 1.27.0+ HTTP transport
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/test_server.py (1)
313-319:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win“Before run()” is not actually enforced by this test.
These assertions run after
main()returns, so the test would still pass if
settings were applied aftermcp.run(). Assert settings at call time via a
side_effectonmock_run.Proposed fix
with ( patch.object(sys, "argv", DEFAULT_ARGV), patch.object(server_module, "get_config", return_value=mock_config), patch.object(server_module.mcp, "run") as mock_run, patch("builtins.print"), ): + def _assert_settings_at_run( + *_args: object, **_kwargs: object + ) -> None: + assert server_module.mcp.settings.host == "0.0.0.0" + assert server_module.mcp.settings.port == 8080 + assert server_module.mcp.settings.log_level == "INFO" + + mock_run.side_effect = _assert_settings_at_run server_module.main() - # Verify settings were set correctly BEFORE run() was called - assert server_module.mcp.settings.host == "0.0.0.0" - assert server_module.mcp.settings.port == 8080 - assert server_module.mcp.settings.log_level == "INFO" - # Verify run() was called with HTTP transport mock_run.assert_called_once_with(transport="streamable-http")As per coding guidelines, "All code must have tests - create unit tests,
integration tests, edge cases, and regression tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_server.py` around lines 313 - 319, The test currently asserts server_module.mcp.settings after main() returns, which doesn't guarantee they were set before mcp.run() was called; modify the test to install a side_effect on mock_run (the mocked mcp.run) that asserts server_module.mcp.settings.host == "0.0.0.0", port == 8080 and log_level == "INFO" at the moment run() is invoked, then optionally return None; keep the existing post-call assertions or remove them if redundant so the behavior is enforced at call time in main().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/unit/test_server.py`:
- Around line 313-319: The test currently asserts server_module.mcp.settings
after main() returns, which doesn't guarantee they were set before mcp.run() was
called; modify the test to install a side_effect on mock_run (the mocked
mcp.run) that asserts server_module.mcp.settings.host == "0.0.0.0", port == 8080
and log_level == "INFO" at the moment run() is invoked, then optionally return
None; keep the existing post-call assertions or remove them if redundant so the
behavior is enforced at call time in main().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ae29764-80ea-44a1-978b-01c2342cd4a5
📒 Files selected for processing (2)
src/freecad_mcp/server.pytests/unit/test_server.py
1 participant
Summary
Two bug fixes for the MCP server:
Fix 1: MCP HTTP transport for mcp 1.27.0
The MCP server failed to start in HTTP mode (port 8000) due to an incompatible API change in
mcplibrary version 1.27.0.Problem:
TypeError: FastMCP.run() got an unexpected keyword argument 'host'
Solution: Set FastMCP configuration via environment variables (
FASTMCP_HOST,FASTMCP_PORT,FASTMCP_LOG_LEVEL) instead of passing them as keyword arguments torun().Files changed:
src/freecad_mcp/server.pyFix 2: pad_sketch use SideType instead of Symmetric
The
pad_sketchtool failed with:AttributeError: 'PartDesign.Feature' object has no attribute 'Symmetric'
Problem: The
Symmetricproperty doesn't exist in FreeCAD 1.1.1 for PartDesign::Pad.Solution: Use
SideTypeproperty with values:"One side"- single direction (default)"Two side"- two sides"Symmetric"- symmetricChanged parameter:
symmetric: bool→side_type: str = "One side"Files changed:
src/freecad_mcp/tools/partdesign.pySummary by CodeRabbit