Skip to content

feat: add mcp tools#311

Merged
jdx merged 1 commit into
jdx:mainfrom
gaojunran:feat-mcp
Apr 9, 2026
Merged

feat: add mcp tools#311
jdx merged 1 commit into
jdx:mainfrom
gaojunran:feat-mcp

Conversation

@gaojunran

Copy link
Copy Markdown
Contributor

addresses #304.

@greptile-apps

greptile-apps Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a pitchfork mcp subcommand that launches a stdio-based MCP server, exposing five tools (pitchfork_status, pitchfork_start, pitchfork_stop, pitchfork_restart, pitchfork_logs) so AI assistants can manage pitchfork daemons via JSON-RPC. The previously raised concerns — a misleading stop success message and .unwrap() on serialization — have both been addressed: the stop handler now pre-snapshots running daemons to accurately report what was stopped, and serialization errors are properly propagated.

Confidence Score: 5/5

Safe to merge — previously raised P1 concerns are resolved and only minor P2 suggestions remain.

Both issues flagged in prior review rounds (misleading stop success message and .unwrap() on serialization) have been properly addressed. The remaining findings are P2 style suggestions (uncapped log lines, startup eprintln) that do not affect correctness or safety.

src/cli/mcp.rs — minor log-line cap and eprintln suggestions only

Vulnerabilities

No security concerns identified. The MCP server operates over local stdio, all daemon identifiers are resolved through PitchforkToml::resolve_ids before use, and there is no external input directly passed to shell commands.

Important Files Changed

Filename Overview
src/cli/mcp.rs New MCP server implementation with five tool handlers; stop handler correctly pre-snapshots running daemons; minor: no upper bound on log line count n
src/cli/mod.rs Registers the new Mcp subcommand in the CLI dispatcher — straightforward addition with no issues
Cargo.toml Adds rmcp dependency for MCP server support; indirect dependency additions look correct
docs/cli/mcp.md Generated CLI reference doc for the new mcp command — no manual edits needed

Sequence Diagram

sequenceDiagram
    participant AI as AI Assistant
    participant MCP as pitchfork mcp (stdio)
    participant IPC as IPC Client
    participant Sup as Supervisor

    AI->>MCP: initialize
    MCP-->>AI: ServerInfo + tool list

    AI->>MCP: call pitchfork_status
    MCP->>IPC: connect(auto_start=true)
    IPC->>Sup: get_all_daemons
    Sup-->>IPC: daemon list
    IPC-->>MCP: entries
    MCP-->>AI: JSON daemon status

    AI->>MCP: call pitchfork_stop({id:[...]})
    MCP->>IPC: connect(auto_start=false)
    IPC->>Sup: get_running_daemons (snapshot)
    Sup-->>IPC: running ids
    MCP->>IPC: stop_daemons(ids)
    IPC->>Sup: stop
    Sup-->>IPC: StopResult
    MCP-->>AI: Stopped: X (based on pre-snapshot)

    AI->>MCP: call pitchfork_logs({id:[...], n:50})
    MCP->>MCP: read_last_n_lines from log files
    MCP-->>AI: log text output
Loading

Reviews (2): Last reviewed commit: "feat: add mcp tools" | Re-trigger Greptile

Comment thread src/cli/mcp.rs
Comment thread src/cli/mcp.rs Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new mcp command to the pitchfork CLI, enabling a Model Context Protocol (MCP) server to run over stdin/stdout. This server exposes daemon management functionalities (start, stop, restart, status, logs) to AI assistants via JSON-RPC. The changes involve adding several new Rust dependencies, implementing the Mcp command in src/cli/mcp.rs, and updating documentation. Review feedback highlights several areas for improvement, including handling unwrap() calls gracefully in the pitchfork_status tool, refining the pitchfork_stop success message to only report actually stopped daemons, clarifying the behavior of n=0 for log retrieval in pitchfork_logs, and logging errors when reading state files or opening log files to improve diagnostic capabilities.

Comment thread src/cli/mcp.rs Outdated
})
.collect();

let text = serde_json::to_string_pretty(&daemons).unwrap();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using unwrap() here can cause the server to panic if JSON serialization fails for any reason. While it might be unlikely with the current data structure, it's safer to handle the potential error gracefully in a server context to prevent crashes.

Suggested change
let text = serde_json::to_string_pretty(&daemons).unwrap();
let text = serde_json::to_string_pretty(&daemons)
.map_err(|e| internal_err(format!("Failed to serialize daemon status: {e}")))?;

Comment thread src/cli/mcp.rs
Comment thread src/cli/mcp.rs Outdated
)]));
}

let n = if params.n == 0 { 50 } else { params.n };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current logic treats n = 0 as a request for the default number of lines (50). This can be surprising for an API client, which would likely expect 0 lines to be returned when n=0 is specified. The #[serde(default = ...)] attribute already handles the case where n is not provided by the client. To make the API more predictable, consider removing this conditional logic and using params.n directly in the call to read_last_n_lines on line 354.

Comment thread src/cli/mcp.rs
Comment thread src/cli/mcp.rs
@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedcargo/​rmcp@​1.3.086100100100100

View full report

@socket-security

Copy link
Copy Markdown

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Warn High
Obfuscated code: cargo rmcp is 98.0% likely obfuscated

Confidence: 0.98

Location: Package overview

From: Cargo.tomlcargo/rmcp@1.3.0

ℹ Read more on: This package | This alert | What is obfuscated code?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore cargo/rmcp@1.3.0. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@gaojunran

Copy link
Copy Markdown
Contributor Author

@jdx ready for review!

@jdx jdx merged commit 602927e into jdx:main Apr 9, 2026
5 checks passed
@jdx jdx mentioned this pull request Apr 9, 2026
jdx added a commit that referenced this pull request Apr 9, 2026
## 🤖 New release

* `pitchfork-cli`: 2.3.0 -> 2.4.0

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [2.4.0](v2.3.0...v2.4.0) -
2026-04-09

### Added

- add mcp tools ([#311](#311))
- impl container mode
([#305](#305))

### Fixed

- use correct base dir for `.config/pitchfork.toml` case
([#307](#307))
- use FSEvent on macos to avoid `Too many files`
([#301](#301))

### Other

- *(deps)* lock file maintenance
([#310](#310))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk release bookkeeping only: updates version metadata and the
changelog, without changing runtime code paths.
> 
> **Overview**
> Bumps `pitchfork-cli` from **2.3.0** to **2.4.0** in
`Cargo.toml`/`Cargo.lock` and adds the `2.4.0` entry to `CHANGELOG.md`
documenting the release contents (MCP tools, container mode, and a few
fixes).
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
b7647a4. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants