-
Notifications
You must be signed in to change notification settings - Fork 1
MCP Server #86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MCP Server #86
Conversation
|
This PR is targeting If this is a regular feature/fix PR, please change the base branch to Current base: |
| @@ -1,8 +1,8 @@ | |||
| use crate::Error; | |||
| use serde::Deserialize; | |||
| use serde::{Deserialize, Serialize}; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now we can both read and write towerfiles, we add the Serialize trait to everything that previously had Deserialize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a comprehensive MCP (Model Context Protocol) server for Tower CLI that enables AI assistants to interact with Tower cloud services through conversational commands. The implementation provides 18 different tools for managing apps, secrets, teams, and deployments through an SSE-based server.
Key Changes
- Adds MCP server functionality with SSE transport for AI assistant integration
- Implements Towerfile generation from pyproject.toml files
- Refactors run command infrastructure to support both interactive and programmatic execution modes
Reviewed Changes
Copilot reviewed 29 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tower-cmd/src/mcp.rs | Core MCP server implementation with 18 tools for Tower operations |
| crates/tower-cmd/src/towerfile_gen.rs | New module for generating Towerfiles from Python project metadata |
| crates/tower-cmd/src/run.rs | Major refactor to support both CLI and programmatic execution with timeout handling |
| tests/integration/ | Complete BDD test suite for MCP server functionality |
| tests/mock-api-server/ | Mock API server for testing Tower operations |
Comments suppressed due to low confidence (1)
crates/tower-cmd/src/run.rs:1
- Line 351 uses
send_errorfor an informational message. This should usesend_lineinstead since it's not actually an error condition but rather normal user-initiated behavior.
use anyhow::Result;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| async fn monitor_status(app: LocalApp) { | ||
| debug!("Starting status monitoring for LocalApp"); | ||
| let mut check_count = 0; | ||
| let max_checks = 600; // 60 seconds with 100ms intervals |
Copilot
AI
Sep 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout calculation comment is incorrect. 600 checks × 100ms = 60 seconds, but the comment should clarify this or use a more explicit calculation like 60 * 1000 / 100 to make the relationship clear.
| fn find_main_script(dir: &Path) -> Option<String> { | ||
| Self::find_script_from_pyproject(dir) | ||
| .or_else(|| Self::find_script_with_main(dir)) | ||
| .or_else(|| Self::find_common_script(dir)) | ||
| .or_else(|| Self::find_any_python_file(dir)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this stuff is some inexact heuristics based on common Python idioms and structures. Very much open to be expanded later, or perhaps there's a way to make it more robust.
Basically we look to see if there's a main python script mentioned in the pyproject.toml and use that as our entry point.
If there isn't, we look to see if there's a python file with __main__ in it somewhere (because of the if __name__ == "__main__": pattern in most python main.py scripts.
Failing that, we look to see if there's any script called one of "main.py", "app.py", "run.py" and "task.py".
Failing all that, we'll just take literally any script ending with .py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be something in pyproject.toml we can use too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean other than the script, as taken on line 55?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit tests are inline in rust
| } | ||
|
|
||
|
|
||
| #[tool(description = "List all Tower apps in your account")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rust MCP library, rmcp, has this nice abstraction where you annotate async functions (in a tool_router struct implementation) with the description of the tool. It then picks out them all and generates the endpoints and everything needed for the MCP server.
|
|
||
| #[tool(description = "List all Tower apps in your account")] | ||
| async fn tower_apps_list(&self) -> Result<CallToolResult, McpError> { | ||
| match api::list_apps(&self.config).await { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the tools are using our already existing functionality in the CLI app — there's a file api.rs in tower-cmd that itself shells out to the crate tower-api, the bulk of which is using https://openapi-generator.tech/ to generate code that interacts with our API using our https://api.tower.dev/v1/openapi.yaml openapi yaml/json
crates/tower-cmd/src/mcp.rs
Outdated
| } | ||
| } | ||
|
|
||
| #[tool(description = "Show the recommended workflow for developing and deploying Tower applications")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea if other MCP tools do this, but I wanted to find a way to give the LLM an ability to learn more about how to use the system. If it were all in the docs directly, then all that text would be prepended to every single prompt for any user using claude code, which seems unideal, since they won't always be working on tower stuff. This way we can have just the docstring for each thing, and then it can choose to call this tool to dynamically learn how to use all the other tools (and specifically, what order it should use them in).
From my testing, it seems to work well, with claude often (always?) choosing to call this tool for docs first.
|
|
||
| For custom ports, adjust the URL accordingly (e.g., `http://127.0.0.1:8080/sse`). | ||
|
|
||
| ##### Cursor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use Cursor or Windsurf so I've not tested either those integrations... Maybe if someone does it would be nice to double check if this setup is correct.
| .about("Run your code in Tower or locally") | ||
| } | ||
|
|
||
| pub async fn do_run(config: Config, args: &ArgMatches, cmd: Option<(&str, &ArgMatches)>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a leftover from when I was calling the inner function from the mcp.rs, which I don't do anymore. The idea was that it separates the IO (eprintln! and exiting) from the pure part of actually running and reading from a stream. I could probably remove it now, especially since do_run_inner isn't an amazing name. However I do like splitting side effectful IO into a wrapper function in general so I didn't feel the urge.
| }).await | ||
| } | ||
|
|
||
| /// do_run_local_capture is like do_run_local but returns captured output lines instead of printing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to have something like this in order to redirect the streamed local output to the mcp server instead of just printing it out (which in this case would mean that the terminal window you run tower mcp-server from would get the logs, but the LLM wouldn't be able to see them)
| ) -> Result<()> { | ||
| match wait_for_run_start(&config, &run).await { | ||
| Err(err) => { | ||
| spinner.failure(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we really don't want this in the main output, because then it would generate a thousand spinning braille events that would be parsed as separate lines in the mcp server.
Note to self: I need to double check it's still there when running directly in the cli 🙃
| async fn do_follow_run_impl( | ||
| config: Config, | ||
| run: &Run, | ||
| sink: &dyn OutputSink, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we use tokio channels for streaming the output now. You can see through this code we now have an OutputSink and StdoutSink — a specific implementation of the OutputSink channel that writes to standard out. This way we can use another channel to get the run output in the MCP server, and stream it directly to the client using SSE events (hence using an SSE server, which sort of seems to be the most common type of MCP server anyway?)
crates/tower-cmd/src/run.rs
Outdated
| /// error. | ||
| fn load_towerfile(path: &PathBuf) -> Towerfile { | ||
| Towerfile::from_path(path.clone()).unwrap_or_else(|err| { | ||
| fn load_towerfile(path: &PathBuf) -> Result<Towerfile> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other change you'll see all over this file is that we switch to wrapping a lot of our outputs in a Result<> type. The reason for this is that previously they would just error out and exit when the command exited, which is... not ideal for an MCP server. Now we can choose whether to exit on an error, or whether to do something else (e.g. like returning an error JSON RCP message to the MCP client). A lot of our functions already did this, but there was a few needed for the MCP server, like this one, that didn't yet.
crates/tower-cmd/src/run.rs
Outdated
| fn load_towerfile(path: &PathBuf) -> Towerfile { | ||
| Towerfile::from_path(path.clone()).unwrap_or_else(|err| { | ||
| fn load_towerfile(path: &PathBuf) -> Result<Towerfile> { | ||
| Towerfile::from_path(path.clone()).map_err(|err| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, unwrap_or_else tries to take the result of a function that returns Result<Self, Error>, and replaces the error result with this new value... which in this case just called std::process::exit(1) (so I guess didn't replace it?). Now we want to keep the Result type, so we want to specifically call a function when we get an error response so that we can add some debug info, which is what map_err does in rust (i.e. it "maps" a function to an "err" value... personally I would have named this function something else but it's a pretty well known rust idiom by now).
| - name: Install Python dependencies | ||
| run: uv sync --locked --group dev | ||
|
|
||
| - name: Start mock API server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the integration tests, I wanted it to be more than a unit test and less than a full E2E test (which can be a bit unreliable since it then depends on the entire backend repo, not to mention much slower). This mock API server is implementing some hardcoded responses for API endpoints, which could in theory go out of sync with the actual API... but since we'll only ever have backwards compatible changes, I'm sure it won't, right? 🙃
| @@ -0,0 +1,94 @@ | |||
| import os | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was just way easier to do in Python than in rust
| @@ -0,0 +1,58 @@ | |||
| Feature: MCP App Management | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a big fan of cucumber tests since my Ruby on Rails days more than a decade ago. I hope you can see why — I think it makes it much more readable and easier to reason about the tests.
In actuality (i.e. in the original git history), this PR was partially TDD based, since I wrote (/generated) some of these tests before all the functionality was implemented. I find that a lot less annoying to do with BDD tests than with, say, unit tests.
| @@ -0,0 +1,8 @@ | |||
| [app] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ends with .j2 as if it's a jinja template, and it sort of is, but in this case I'm quite literally doing a find and replace for these specific variables: 6f04369#diff-365b24b9adc19a6d5ff90a9eb58bbc764d70284234d4dcc97bd35bc5d5a7b5f3R43
| flake-utils.url = "github:numtide/flake-utils"; | ||
| fenix = { | ||
| url = "github:nix-community/fenix"; | ||
| rust-overlay = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was
- to get a recent enough version of rust to be able to use
rmcp - bringing us in line to use the same nix overlay for rust in both our monorepo and this tower-cli repo
1da4a2e to
ea82ff9
Compare
|
FYI @socksy there's a legit failure in the integration test. I think the compilation failure was transient, just tried it again. |
|
Yes, when I said yesterday "it's a legit failure" I was referring to a broken integration test I saw, the compilation one came after that 🙃 |
4c74cbb to
8acf279
Compare
bradhe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good place to start from, to test it out and see how it behaves. There's one bit of feedback around packaging, and obviously a failing test that needs to be addressed. But otherwise, let's ship it and see how it works for folks.
| let source_files = if script.ends_with(".py") { | ||
| vec![script.clone(), "*.py".to_string()] | ||
| } else { | ||
| vec![script.clone()] | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should default to including everything in the project, so why not just make this an empty vec?
| fn find_main_script(dir: &Path) -> Option<String> { | ||
| Self::find_script_from_pyproject(dir) | ||
| .or_else(|| Self::find_script_with_main(dir)) | ||
| .or_else(|| Self::find_common_script(dir)) | ||
| .or_else(|| Self::find_any_python_file(dir)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be something in pyproject.toml we can use too?
890fe89 to
bc9eef7
Compare
- Add Towerfile generation from pyproject.toml - Support working directory parameter for file operations - Generate valid TOML format (not JSON-like) - Add parameter manipulation functions for Towerfile updates
- Add MCP server implementation using rmcp abstractions - Implement core tower operations (apps, secrets, teams, etc.) - Add proper error handling for run operations - Support both local and remote tower operations via MCP protocol
- Add OutputSink trait for pluggable output handling - Improve run completion detection and matching - Restore error handling and ctrl-c support for remote runs - Update documentation for new connection methods
- Replace Rust cucumber tests with Python BDD tests using behave - Add mock API server for isolated testing - Fix async hanging issues with proper behave async support - Implement JWT-based authentication for test scenarios - Add comprehensive test coverage for MCP operations - Include CI workflow for integration tests
- Switch from fenix to rust-overlay in Nix flake - Add uv.lock files for Python dependency management - Update logging configuration to handle setup failures gracefully - Add MCP and testing dependencies to pyproject.toml
The integration tests were failing because the mock API session endpoint response was missing required fields that were added in newer API schema versions. This caused JWT authentication to fail with "UnknownDescribeSessionValue" errors, preventing local app runs from working in tests. Changes: - Add missing required fields to mock API user response (is_confirmed, is_subscribed_to_changelog) - Add comprehensive documentation and debugging guide in tests/mock-api-server/README.md - Enhance test error messages to point to specific debugging steps - Add detailed comments in mock API explaining schema dependencies - Fix workflow file to use --color flag for better CI output 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…use it + return empty vector for sources in towerfile gen
bc9eef7 to
68ce651
Compare
bradhe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, little bit of feedback. Let's discuss.
crates/tower-cmd/src/run.rs
Outdated
| eprintln!("{}", e); | ||
| std::process::exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use output::die here?
crates/tower-cmd/src/run.rs
Outdated
| // For the time being, we should report that we can't run an app by name locally. | ||
| if app_name.is_some() { | ||
| output::die("Running apps by name locally is not supported yet."); | ||
| anyhow::bail!("Running apps by name locally is not supported yet."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please use the output module to make this consistent!
crates/tower-cmd/src/run.rs
Outdated
| }, | ||
| _ = tokio::signal::ctrl_c(), if enable_ctrl_c => { | ||
| sink.send_line("Received Ctrl+C, stopping log streaming...".to_string()); | ||
| sink.send_line("Note: The run will continue in Tower cloud".to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we sent the link for the run along with this (run.dollar_link) so the user can click on it.
crates/tower-cmd/src/run.rs
Outdated
| Err(anyhow::anyhow!("Run failed with error status")) | ||
| }, | ||
| tower_api::models::run::Status::Crashed => { | ||
| sink.send_error(format!("Run #{} for app '{}' crashed", completed_run.number, completed_run.app_name)); | ||
| Err(anyhow::anyhow!("Run crashed")) | ||
| }, | ||
| tower_api::models::run::Status::Cancelled => { | ||
| sink.send_error(format!("Run #{} for app '{}' was cancelled", completed_run.number, completed_run.app_name)); | ||
| Err(anyhow::anyhow!("Run was cancelled")) | ||
| }, | ||
| _ => { | ||
| sink.send_line(format!("Run #{} for app '{}' completed successfully", completed_run.number, completed_run.app_name)); | ||
| Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again with the usage of anyhow here, everywhere else we use the Errors enum...consistency would be really good...
68ce651 to
f6f15eb
Compare
bradhe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| edition = "2021" | ||
|
|
||
| [dependencies] | ||
| anyhow = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
This PR introduces an SSE MCP server that you can run from any tower project (or wannabe tower project) with
tower mcp-server). It provides the following tools (in order as they appear in claude, at least, which is unfortunately somewhat illogical):This PR is exactly the same as what's in #81, but I've got claude to dramatically reduce the amount of commits and to reword the commit messages. I hope that makes it easier to follow — but in case it makes something a bit illogical, the true history is here. Since it was a lot of exploring, there's a lot of stuff that was added in some commits, and the removed/reworked completely in later commits.
I will write proper documentation for this MCP server later, but for now it would be nice to get this in.