Feat/implement mcp #2
Conversation
ryderstorm
force-pushed
the
feat/implement-mcp
branch
from
56a824f to
a452d35
Compare
- add pre-commit config with ruff, ty, and markdownlint hooks - pin dev toolchain versions in pyproject and lockfile
- Implement config module for workspace paths, transports, and logging - Build prompts loader to ingest Markdown prompts with YAML frontmatter - Create application factory registering prompts with FastMCP - Add pytest fixtures and tests for prompt loading - Configure hatchling build system for package distribution Completes tasks 1.1-1.5 from spec 0001
- Create comprehensive operations.md with deployment and configuration details - Add Quick Start section to README with installation and usage examples - Document STDIO and HTTP transport options - Include MCP client integration examples for Claude Desktop and VS Code Completes task 1.6 from spec 0001
All subtasks of 1.0 (Establish FastMCP server foundation) are now complete: - Package layout and configuration - Config module with environment overrides - Prompts loader with YAML frontmatter parsing - Application factory with FastMCP integration - Pytest fixtures and comprehensive tests - Documentation for operations and quick start
- Create server.py as the standard entrypoint (follows FastMCP best practices) - CLI now auto-discovers 'mcp' instance without explicit path - Update all documentation to use 'fastmcp run server.py' instead of '__init__.py:app' - Simplifies command from 'uvx fastmcp dev mcp_server/__init__.py:app' to 'uvx fastmcp dev server.py' This aligns with FastMCP conventions where the CLI looks for 'mcp', 'server', or 'app' instances at module level.
Update task documentation to reflect current implementation status: - Mark Task 2.0 as unnecessary (ResourceTemplates not required) - Document deferral of HTTP hardening to issue #3 - Document deferral of K8s packaging to issue #4 - Document deferral of protocol extensions to issue #5 - Add status notes to specification slices for clarity These updates provide clear tracking of completed work versus features deferred to focused future implementations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
ryderstorm
force-pushed
the
feat/implement-mcp
branch
from
e767837 to
9228712
Compare
WalkthroughAdds a FastMCP app factory and server entrypoint, runtime configuration, prompt parsing/registration utilities and prompt content updates; introduces tests and fixtures, docs/operations guide, task/spec markdown, dev tooling in pyproject, and removes a small hello script and placeholder test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI
participant ServerMod as server.py
participant AppFactory as mcp_server.__init__.create_app
participant Config as mcp_server.config
participant Loader as mcp_server.prompts_loader
participant Utils as mcp_server.prompt_utils
participant FS as FileSystem
CLI->>ServerMod: import server / discover `mcp`
ServerMod->>AppFactory: create_app()
AppFactory->>Config: read env & defaults
Config-->>AppFactory: config instance
AppFactory->>Loader: register_prompts(mcp, prompts_dir)
Loader->>FS: list and read `*.md`
FS-->>Loader: file contents
Loader->>Utils: parse_frontmatter / load_markdown_prompt
Utils-->>Loader: MarkdownPrompt objects
loop register each prompt
Loader->>AppFactory: register prompt decorator + handler
end
AppFactory-->>ServerMod: return FastMCP instance (`mcp`)
sequenceDiagram
autonumber
participant Loader as prompts_loader
participant FS as FileSystem
participant App as FastMCP
App->>Loader: register_prompts(mcp, prompts_dir)
alt prompts_dir missing
Loader->>App: raise ValueError("does not exist")
else valid directory
Loader->>FS: iterate `*.md`
FS-->>Loader: files
Loader->>App: register prompts (name, docstring, handler)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
8-14: Remove duplicate dependencies from the main dependencies list.Development-only dependencies (
pre-commit,pytest,pytest-cov,ruff) should not be in the maindependencieslist—they belong exclusively in[dependency-groups].dev. This duplication creates maintenance overhead and risks version conflicts (e.g.,ruff>=0.14.0here vsruff>=0.13.3in dev).Apply this diff to remove the duplicates:
dependencies = [ "fastmcp>=2.12.4", - "pre-commit>=4.3.0", - "pytest>=8.4.2", - "pytest-cov>=7.0.0", - "ruff>=0.14.0", ]
🧹 Nitpick comments (3)
pyproject.toml (1)
23-23: Consider updating ruff to match the main dependency version.Once the duplicate ruff dependency is removed from main, update this constraint to match the intended minimum version (was
>=0.14.0in main).- "ruff>=0.13.3", + "ruff>=0.14.0",tests/test_prompts.py (1)
54-67: Consider using pytest-anyio's async test support for cleaner code.The current pattern of defining inner async functions and wrapping them with
anyio.run()works but is verbose. Sincepytest-anyiois a dev dependency, you can write async test methods directly.Add to
pyproject.toml:[tool.pytest.ini_options] minversion = "8.0" addopts = "-ra" testpaths = ["tests"] asyncio_mode = "auto" # Enable auto-detection of async testsThen refactor tests to use async/await directly:
- def test_load_prompts_from_directory(self, mcp_server, temp_prompts_dir): + async def test_load_prompts_from_directory(self, mcp_server, temp_prompts_dir): """Test loading prompts from a directory.""" load_prompts_from_directory(mcp_server, temp_prompts_dir) - # Get the loaded prompts - async def get_prompts(): - return await mcp_server.get_prompts() - - prompts = anyio.run(get_prompts) + prompts = await mcp_server.get_prompts()Apply similar changes to the other test methods that use this pattern (lines 69-80, 89-106).
mcp_server/prompts_loader.py (1)
90-93: Consider logging malformed YAML frontmatter.Malformed YAML is silently ignored (returns empty dict), which could mask configuration errors in prompt files. While graceful degradation is appropriate, logging a warning would help operators diagnose issues.
# Parse YAML frontmatter try: frontmatter = yaml.safe_load(parts[1]) or {} except yaml.YAMLError: + # Consider adding: logger.warning(f"Malformed YAML frontmatter in {prompt_file}") frontmatter = {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
docs/operations.md(1 hunks)hello.py(0 hunks)mcp_server/__init__.py(1 hunks)mcp_server/config.py(1 hunks)mcp_server/prompts_loader.py(1 hunks)pyproject.toml(1 hunks)server.py(1 hunks)tasks/0001-spec-sdd-mcp-poc.md(1 hunks)tasks/tasks-0001-spec-sdd-mcp-poc.md(1 hunks)tasks/tasks-0002-spec-open-source-ready.md(1 hunks)tests/conftest.py(1 hunks)tests/test_placeholder.py(0 hunks)tests/test_prompts.py(1 hunks)
💤 Files with no reviewable changes (2)
- hello.py
- tests/test_placeholder.py
🧰 Additional context used
🧬 Code graph analysis (3)
server.py (2)
tests/conftest.py (1)
mcp_server(61-67)mcp_server/__init__.py (1)
create_app(15-39)
tests/test_prompts.py (2)
tests/conftest.py (2)
mcp_server(61-67)temp_prompts_dir(25-57)mcp_server/prompts_loader.py (2)
_parse_frontmatter(72-97)load_prompts_from_directory(15-29)
mcp_server/__init__.py (1)
mcp_server/prompts_loader.py (1)
load_prompts_from_directory(15-29)
🪛 GitHub Actions: Run tests and linting
mcp_server/config.py
[error] 1-1: ruff format check failed. File would be reformatted.
🔇 Additional comments (6)
pyproject.toml (1)
24-24: Verify the need for an alpha-version dependency.The
ty>=0.0.1a21package is at an alpha version, which may introduce instability. Ensure this is intentional and document why this alpha version is required if it's a critical dependency.tests/conftest.py (1)
1-67: LGTM!The test fixtures are well-structured and appropriate for the test scenarios:
temp_workspacecreates isolated workspace directories for testingtemp_prompts_dirprovides realistic test prompts with valid YAML frontmattermcp_serverreturns a clean FastMCP instance for each testtests/test_prompts.py (1)
9-48: LGTM!The frontmatter parsing tests comprehensively cover valid YAML, missing frontmatter, and invalid YAML scenarios with appropriate assertions and fallback behavior.
tasks/tasks-0001-spec-sdd-mcp-poc.md (1)
1-89: LGTM! Well-structured task tracking document.The task list provides clear breakdown of work with:
- Comprehensive file references with status tracking
- Useful development notes and best practices
- Detailed task breakdown with demo criteria and proof artifacts
- Appropriate deferral of tasks to follow-up issues
mcp_server/prompts_loader.py (2)
49-61: LGTM! Factory pattern correctly captures prompt body.The factory function properly creates closures over
prompt_body, ensuring each registered prompt returns its own content. The PromptMessage structure aligns with the MCP protocol requirements.
15-29: LGTM! Defensive directory validation.The function appropriately validates directory existence before attempting to load prompts, providing clear error messaging for misconfiguration.
- Introduce a MarkdownPrompt helper that normalizes frontmatter metadata - Ensures prompt registrations emit deterministic kwargs - Update the loader, prompts, and tests to consume the new structure
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)
prompts/generate-task-list-from-spec.md (1)
31-31: Fix typo: "infrastructre" → "infrastructure".Apply this diff:
-4. **Assess Current State:** Review the existing codebase to understand existing infrastructre, architectural patterns and conventions. Also, identify any existing components or features that already exist and could be relevant to the Spec requirements. Then, identify existing related files, components, and utilities that can be leveraged or need modification. +4. **Assess Current State:** Review the existing codebase to understand existing infrastructure, architectural patterns and conventions. Also, identify any existing components or features that already exist and could be relevant to the Spec requirements. Then, identify existing related files, components, and utilities that can be leveraged or need modification.
🧹 Nitpick comments (5)
prompts/generate-task-list-from-spec.md (2)
9-11: Moveallowed-toolsinto themetasection.The
allowed-toolsfield is currently at the root level of the frontmatter, but according to the parsing logic inmcp_server/prompt_utils.py(lines 59-73), fields outside the known set (name,title,description,tags,arguments,meta,enabled) are automatically merged intometaasadditional_meta. While this works, it creates inconsistent frontmatter structure.For clarity and maintainability, explicitly nest
allowed-toolsinside themetablock:Apply this diff:
meta: category: spec-development + allowed-tools: Glob, Grep, LS, Read, Edit, MultiEdit, Write, WebFetch, WebSearch -allowed-tools: Glob, Grep, LS, Read, Edit, MultiEdit, Write, WebFetch, WebSearch
33-33: Add missing period at end of sentence.Apply this diff:
-6. **Inform the user:** Present these tasks to the user in the specified format (without sub-tasks yet) For example, say "I have generated the high-level tasks based on the Spec. Ready to generate the sub-tasks? Respond with 'Generate sub tasks' to proceed." . +6. **Inform the user:** Present these tasks to the user in the specified format (without sub-tasks yet). For example, say "I have generated the high-level tasks based on the Spec. Ready to generate the sub-tasks? Respond with 'Generate sub tasks' to proceed."prompts/manage-tasks.md (1)
9-11: Moveallowed-toolsinto themetasection for consistency.Same issue as in
generate-task-list-from-spec.md: theallowed-toolsfield should be nested inside themetablock rather than at the root level for consistent frontmatter structure.Apply this diff:
meta: category: task-management -allowed-tools: Glob, Grep, LS, Read, Edit, MultiEdit, Write, WebFetch, WebSearch + allowed-tools: Glob, Grep, LS, Read, Edit, MultiEdit, Write, WebFetch, WebSearchmcp_server/prompt_utils.py (2)
127-133: Validate thatrequiredis a boolean.The code accepts any value for
requiredfrom the frontmatter and relies on Python's truthiness. If a user accidentally providesrequired: "yes"orrequired: 1, it would be treated as truthy without validation.Apply this diff to add type validation:
+ required_raw = entry.get("required", True) + if not isinstance(required_raw, bool): + raise ValueError(f"Argument '{name}' has non-boolean 'required': {required_raw}") + normalized.append( PromptArgumentSpec( name=name, description=entry.get("description"), - required=entry.get("required", True), + required=required_raw, ) )
138-145: Validate that tags are strings before normalization.The function applies
str(tag)to each tag without validating that the input is string-like. This could mask configuration errors if someone accidentally provides complex objects in the tags list.Apply this diff to add validation:
def _ensure_tag_set(raw: Any) -> set[str] | None: if raw is None: return None if isinstance(raw, Iterable) and not isinstance(raw, (str, bytes)): - tags = {str(tag) for tag in raw} + tags = set() + for tag in raw: + if not isinstance(tag, str): + raise ValueError(f"Tag must be a string, got {type(tag).__name__}: {tag}") + tags.add(tag) return tags or None return {str(raw)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
mcp_server/__init__.py(1 hunks)mcp_server/prompt_utils.py(1 hunks)mcp_server/prompts_loader.py(1 hunks)prompts/generate-spec.md(1 hunks)prompts/generate-task-list-from-spec.md(1 hunks)prompts/manage-tasks.md(1 hunks)tests/conftest.py(1 hunks)tests/test_prompts.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- prompts/generate-spec.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_prompts.py
- tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (2)
mcp_server/__init__.py (1)
mcp_server/prompts_loader.py (1)
register_prompts(23-36)
mcp_server/prompts_loader.py (1)
mcp_server/prompt_utils.py (3)
MarkdownPrompt(19-42)load_markdown_prompt(45-87)decorator_kwargs(30-42)
🔇 Additional comments (2)
mcp_server/prompts_loader.py (2)
14-20: Reassigning__name__after decoration may be ignored. Decorators typically capture the function name at definition time, so settingprompt_handler.__name__afterward might not update FastMCP’s prompt registry. Confirm via tests or FastMCP docs, or pass the handler name explicitly throughdecorator_kwargs()if supported.
17-18: Return type is valid: Returning a plainstraligns with FastMCP v2.12.4’s PromptResult requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mcp_server/config.py (1)
33-34: Validate logging configuration values.The
log_levelandlog_formatvalues are not validated, which could lead to runtime errors or unexpected behavior when the logging system is initialized.Apply this diff to add validation:
- self.log_level = os.getenv("SDD_LOG_LEVEL", "INFO") - self.log_format = os.getenv("SDD_LOG_FORMAT", "json") # json or text + log_level = os.getenv("SDD_LOG_LEVEL", "INFO").upper() + valid_levels = ["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] + if log_level not in valid_levels: + raise ValueError(f"Invalid log level: {log_level}. Must be one of {valid_levels}") + self.log_level = log_level + + log_format = os.getenv("SDD_LOG_FORMAT", "json").lower() + if log_format not in ("json", "text"): + raise ValueError(f"Invalid log format: {log_format}. Must be 'json' or 'text'") + self.log_format = log_format
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mcp_server/__init__.py (1)
15-39: LGTM! Appropriate skeleton implementation.The
create_app()factory function is clean and well-structured for a POC/skeleton:
- Follows the factory pattern correctly
- Registers prompts from configuration
- Includes a basic example tool for verification
- TODOs are tracked in task documents (per PR objectives)
For production readiness in future PRs, consider adding error handling around
register_prompts()(which can raiseValueErrorper the loader implementation) and initialization logging to aid troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/operations.md(1 hunks)mcp_server/__init__.py(1 hunks)mcp_server/config.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/operations.md
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp_server/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
mcp_server/__init__.py (1)
mcp_server/prompts_loader.py (1)
register_prompts(23-36)
🔇 Additional comments (1)
mcp_server/__init__.py (1)
1-12: LGTM! Clean module structure.The imports, docstring, and version declaration are well-organized. The factory pattern approach (exposing only
create_app()without a module-level instance) addresses the previous review concern about redundant instantiation.
If this is truly roadmap content, what do you think about using a |
I like Maybe |
@jguice Great points. I want to rework this based on some feedback from Amber on the process, but i want to do it in a separate PR. This one is already big. |
@jguice good catch. I already moved them out to separate issues in this repo: https://github.com/liatrio-labs/spec-driven-workflow-mcp/issues, so I think we can skip the ROADMAP.md. |
Why?
What Changed?
mcp_server.create_app()with FastMCP wiring, basic sample tool, and dynamic prompt registration viaprompts_loader.register_prompts().mcp_server/config.py) supporting transport, logging, CORS, and workspace directory bootstrap.mcp_server/prompt_utils.py) that ingest Markdown with YAML frontmatter, normalize arguments/tags, and expose metadata to FastMCP decorators.prompts/*.mdto align with the loader, including structured tags and metadata.docs/operations.md) covering setup, transports, client integrations, and troubleshooting.tasks/0001-spec-sdd-mcp-poc.mdand backlog updates in the task tracker.tests/conftest.pyandtests/test_prompts.py; removed obsoletetests/test_placeholder.py.pyproject.toml,uv.lock) and removed the unusedhello.pyscript.Additional Notes
uv run pytestuvx fastmcp run server.pySummary by CodeRabbit
New Features
Documentation
Tests
Chores