Export a safe public MCP tool contract - #15
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Readiness check at
PR remains draft; no merge or deployment performed. |
|
CI repair pushed and PR head read-back confirms |
|
Final check read-back: all CI, Vercel preview, and CodeRabbit checks pass at exact head |
|
Ready for draft review at |
WalkthroughThe PR replaces query-string MCP authentication with Authorization-header-only handling, adds a deterministic 15-tool manifest exporter and artifact, introduces local stdio protocol smoke tests, updates CI validation, and revises connection documentation and examples. ChangesMCP contract and validation
Sequence Diagram(s)sequenceDiagram
participant CI
participant ManifestExporter
participant ToolRegistry
participant StdioSmokeTest
participant MCPServer
CI->>ManifestExporter: Export and validate manifest
ManifestExporter->>ToolRegistry: Read 15 registered tools
ToolRegistry-->>ManifestExporter: Tool definitions
CI->>CI: Compare committed manifest
CI->>StdioSmokeTest: Run protocol smoke test
StdioSmokeTest->>MCPServer: initialize and tools/list over stdio
MCPServer-->>StdioSmokeTest: Server metadata and tool inventory
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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 (1)
.github/workflows/ci.yml (1)
40-43: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence on checkout.
Neither the
testnorvalidate-mcpjob pushes or authenticates via git afterward, yet both checkouts leave theGITHUB_TOKENcredential persisted in the local git config for the remainder of the job (defaultpersist-credentials: true). Any subsequent step in the job (e.g. a compromised transitive pip dependency duringpip install) could use it to push/exfiltrate.🔒 Proposed fix
- uses: actions/checkout@v4 with: # Manifest tests read back the exact registry-owning commit. fetch-depth: 0 + persist-credentials: false- uses: actions/checkout@v4 with: # Ignore GitHub's synthetic PR merge and retain source history. fetch-depth: 0 + persist-credentials: falseAlso applies to: 64-67
🤖 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 @.github/workflows/ci.yml around lines 40 - 43, Update the checkout steps in both the test and validate-mcp jobs to disable credential persistence by setting persist-credentials to false, while retaining the existing full fetch-depth required by manifest tests.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@scripts/export_mcp_manifest.py`:
- Around line 18-24: Update SOURCE_PATHS in the export manifest script to
include Python files under server/tools/_core, either by changing the existing
server/tools glob to recursive rglob("*.py") or by explicitly adding the shared
_core modules. Preserve the existing repository-relative sorting and inclusion
of api/index.py so validate_source_revision covers all exported tool metadata
sources.
In `@scripts/smoke_stdio_mcp.py`:
- Around line 42-44: Update the main function’s asyncio.run call around
probe_stdio to enforce a 10-second timeout, matching the existing test behavior,
so a hung server causes the CI entrypoint to fail promptly.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 40-43: Update the checkout steps in both the test and validate-mcp
jobs to disable credential persistence by setting persist-credentials to false,
while retaining the existing full fetch-depth required by manifest tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 954c3079-3a6a-4d95-af76-539f9b8ce911
📒 Files selected for processing (14)
.github/workflows/ci.ymlCLAUDE.mdREADME.mdREPO-MAP.mdapi/index.pyexamples/claude/config.jsonexamples/cursor/mcp.jsonexamples/local/config.jsonmcp-tools.public.jsonscripts/export_mcp_manifest.pyscripts/smoke_stdio_mcp.pytests/test_integration.pytests/test_mcp_manifest.pytests/test_mcp_stdio.py
|
Addressed every actionable review finding in 6cf88dd: recursive provenance now covers shared server/tools/_core modules, the executable stdio probe has a 10-second protocol timeout, and both full-history checkouts disable persisted credentials. Fresh local proof: manifest regeneration clean; 74 pytest tests passed; Ruff passed; mypy passed; real stdio initialization listed all 15 tools. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_mcp_stdio.py (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign these tests with the repository’s pytest conventions.
Make
test_stdio_initialize_and_tools_listanasync deftest that awaitsasyncio.wait_for(...)instead of callingasyncio.run(...). Also place both tests in a PascalCase test class.Proposed structure
-def test_stdio_initialize_and_tools_list(): - result = asyncio.run(asyncio.wait_for(smoke.probe_stdio(), timeout=10)) +class TestMcpStdio: + async def test_stdio_initialize_and_tools_list(self): + result = await asyncio.wait_for(smoke.probe_stdio(), timeout=10) - def test_cli_entrypoint_enforces_the_protocol_timeout(monkeypatch, capsys): + def test_cli_entrypoint_enforces_the_protocol_timeout(self, monkeypatch, capsys):As per coding guidelines,
tests/**/*.pymust use pytest with pytest-asyncio, andtests/test_*.pymust organize tests as class-based tests.Also applies to: 17-34
🤖 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/test_mcp_stdio.py` around lines 8 - 14, Update test_stdio_initialize_and_tools_list to be an async def test and await asyncio.wait_for(smoke.probe_stdio(), timeout=10) directly, removing asyncio.run. Organize this test and the related tests in the file within a PascalCase pytest test class, preserving their existing assertions and behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/test_mcp_stdio.py`:
- Around line 8-14: Update test_stdio_initialize_and_tools_list to be an async
def test and await asyncio.wait_for(smoke.probe_stdio(), timeout=10) directly,
removing asyncio.run. Organize this test and the related tests in the file
within a PascalCase pytest test class, preserving their existing assertions and
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 41067ba4-d8f4-4c5a-965d-b40e2367c64c
📒 Files selected for processing (5)
.github/workflows/ci.ymlscripts/export_mcp_manifest.pyscripts/smoke_stdio_mcp.pytests/test_mcp_manifest.pytests/test_mcp_stdio.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- tests/test_mcp_manifest.py
- scripts/export_mcp_manifest.py
|
Final review handoff at |
|
Addressed the remaining valid style review on the existing tests: both MCP test modules now follow the repository-required class-based pytest convention, and the protocol smoke test awaits the probe natively under pytest-asyncio. No production behavior changed. Clean Python 3.11 proof remains green: 75 tests, Ruff, mypy, deterministic manifest regeneration, and real 15-tool stdio handshake with MCP 1.29.0. @coderabbitai review |
|
✅ Action performedReview finished.
|
|
Closeout review: source-owned export, deterministic 15-tool registry, executable stdio handshake, MCP 1.x bound, environment-based bearer auth, and the query-token documentation regression guard are all green. CodeRabbit approved the addressed findings and all eight checks pass at exact head 017733f. The important product boundary is explicit: stdio is supported now; hosted transport remains a separate authenticated follow-up. Proceeding with the authorized squash merge. |
1 participant
Closes #13
Customer outcome
Agents can discover Ghostshell’s 15 public MCP tools and connect through the supported stdio transport without copying unsafe credentials into URLs.
What changed
_coremodulesREPO-MAP.mdValidation
017733ffc855713bab729e5e026d719b5dbc0dd4b653818071c082574749a8617b7229b8b6a40a086b4fd2fd5fccdb3d548247c6Boundary
No merge, deployment, or credential change was performed. Ready for an independent human reviewer.