feat(TOW-1415): Claude Code plugin with bundled MCP server and skill#247
feat(TOW-1415): Claude Code plugin with bundled MCP server and skill#247
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
socksy
requested review from
bradhe,
codingcyclist,
giray123 and
konstantinoscs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/tower-cmd/src/skill.rs (1)
13-16: Prefer fallible stdout writes overprint!for CLI reliability.If output is piped and the consumer closes early,
print!can panic. Consider writing withstdout().lock().write_all(...)and returningio::Result<()>so caller can handle non-broken-pipe errors cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/skill.rs` around lines 13 - 16, Replace the panic-prone print! in do_skill_generate with a fallible stdout write: change the signature of do_skill_generate to return io::Result<()> and write the generated content using std::io::stdout().lock().write_all(content.as_bytes()) (or write_all from a mutable stdout handle) and propagate the Result; reference the existing do_skill_generate function and the generate_skill_md call to locate where to produce and write the content and ensure errors (including broken pipes) are returned instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/tower-cmd/src/skill.rs`:
- Around line 57-96: The named-argument filter and subsequent unwraps currently
skip short-only flags and panic if long is missing: adjust the filter on
sub.get_arguments() (the named Vec) to include arguments that have either
get_long().is_some() OR get_short().is_some() (i.e., don't require a long), and
update the rendering loop (where you call arg.get_long().unwrap() and
arg.get_short()) to handle the four cases (both short+long, long-only,
short-only, neither) by conditionally formatting the output based on presence of
arg.get_long() and arg.get_short() while still using arg.get_id()/get_help() as
before.
In `@skills/tower/SKILL.md`:
- Around line 68-72: Multiple fenced code blocks in SKILL.md (containing
examples like tower_file_generate, tower_run_local, tower_file_update,
tower_file_add/edit/remove_parameter, tower_file_validate, tower_apps_create,
tower_deploy, tower_run_remote, tower_apps_list, tower_apps_show,
tower_apps_logs, tower_teams_list, tower_teams_switch, tower_secrets_create,
tower_secrets_list, and the schedules commands) are missing language identifiers
and trigger markdownlint MD040; update each triple-backtick fence to include a
language tag (recommend "text") so lines like ``` become ```text for every block
shown (including the blocks containing the three examples, the pipeline arrow
lines, the single-command lines, the schedules block, and the apps/teams/secrets
blocks) to satisfy the linter.
- Around line 136-157: Regenerate the command reference section in
skills/tower/SKILL.md from the current CLI metadata and commit the updated file:
run whatever CLI help/metadata exporter your project uses to produce the
SKILL.md contents so the generated text (e.g., entries for commands like `tower
login`, `tower apps list`, `tower apps show`, `tower secrets list`) matches the
current .about() output, replace the outdated blocks in SKILL.md with the newly
generated output, and ensure similar outdated fragments around the later region
(lines corresponding to the other occurrences mentioned) are updated before
committing.
---
Nitpick comments:
In `@crates/tower-cmd/src/skill.rs`:
- Around line 13-16: Replace the panic-prone print! in do_skill_generate with a
fallible stdout write: change the signature of do_skill_generate to return
io::Result<()> and write the generated content using
std::io::stdout().lock().write_all(content.as_bytes()) (or write_all from a
mutable stdout handle) and propagate the Result; reference the existing
do_skill_generate function and the generate_skill_md call to locate where to
produce and write the content and ensure errors (including broken pipes) are
returned instead of panicking.
🪄 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: CHILL
Plan: Pro
Run ID: 67544b98-455f-4c96-b7e5-705a7ba9851e
📒 Files selected for processing (10)
.claude-plugin/plugin.json.coderabbit.yaml.gitignore.mcp.jsoncrates/tower-cmd/src/apps.rscrates/tower-cmd/src/lib.rscrates/tower-cmd/src/mcp.rscrates/tower-cmd/src/secrets.rscrates/tower-cmd/src/skill.rsskills/tower/SKILL.md
| let named: Vec<_> = sub | ||
| .get_arguments() | ||
| .filter(|a| !a.is_positional() && !a.is_hide_set() && a.get_long().is_some()) | ||
| .collect(); | ||
|
|
||
| if !positional.is_empty() || !named.is_empty() { | ||
| out.push_str("**Arguments:**\n\n"); | ||
| for arg in &positional { | ||
| let req = if arg.is_required_set() { | ||
| " *(required)*" | ||
| } else { | ||
| "" | ||
| }; | ||
| let help = arg | ||
| .get_help() | ||
| .map(|h| format!(" — {}", h)) | ||
| .unwrap_or_default(); | ||
| out.push_str(&format!( | ||
| "- `<{}>` {}{}\n", | ||
| arg.get_id(), | ||
| req, | ||
| help | ||
| )); | ||
| } | ||
| for arg in &named { | ||
| let long = arg.get_long().unwrap(); | ||
| let req = if arg.is_required_set() { | ||
| " *(required)*" | ||
| } else { | ||
| "" | ||
| }; | ||
| let help = arg | ||
| .get_help() | ||
| .map(|h| format!(" — {}", h)) | ||
| .unwrap_or_default(); | ||
| if let Some(short) = arg.get_short() { | ||
| out.push_str(&format!("- `-{}`, `--{}`{}{}\n", short, long, req, help)); | ||
| } else { | ||
| out.push_str(&format!("- `--{}`{}{}\n", long, req, help)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/tower-cmd/src/skill.rs | sed -n '50,100p'Repository: tower/tower-cli
Length of output: 2169
🏁 Script executed:
# Search for argument definitions with get_short or get_long usage
rg "get_short|get_long" crates/tower-cmd/src/ -B 2 -A 2Repository: tower/tower-cli
Length of output: 1142
🏁 Script executed:
# Search for clap command/arg definitions to see if short-only flags are used
rg "\.short\(" crates/tower-cmd/ -B 2 -A 2 | head -100Repository: tower/tower-cli
Length of output: 6082
🏁 Script executed:
# Search for all .short() calls and see the full Arg definition to check if they have .long()
rg "\.short\(" crates/tower-cmd/ -B 5 | grep -A 5 "Arg::new"Repository: tower/tower-cli
Length of output: 10144
🏁 Script executed:
# Check if there are any .short() calls without a corresponding .long() in the same Arg definition
rg "Arg::new" crates/tower-cmd/src/ -A 10 | grep -B 5 "\.short\(" | grep -v "\.long\("Repository: tower/tower-cli
Length of output: 106
🏁 Script executed:
# Better search: find all Arg::new blocks and check if they have both short and long, or only short
rg "Arg::new\(" crates/tower-cmd/src/ -A 15 | python3 << 'EOF'
import sys
import re
content = sys.stdin.read()
arg_blocks = re.split(r'(?=crates/tower-cmd)', content)
short_only = []
current_arg = None
has_short = False
has_long = False
for block in arg_blocks:
if 'Arg::new' in block:
lines = block.split('\n')
for i, line in enumerate(lines):
if 'Arg::new' in line:
# Extract the arg name
match = re.search(r'Arg::new\("([^"]+)"\)', line)
if match:
current_arg = match.group(1)
has_short = False
has_long = False
elif '.short(' in line:
has_short = True
elif '.long(' in line:
has_long = True
elif i > 0 and ('Arg::new' in lines[i] or (i == len(lines) - 1)):
# Check if we found a short-only arg
if current_arg and has_short and not has_long:
short_only.append(current_arg)
current_arg = None
if short_only:
print("Short-only arguments found:")
for arg in short_only:
print(f" - {arg}")
else:
print("No short-only arguments found. All arguments with .short() also have .long()")
EOFRepository: tower/tower-cli
Length of output: 135
🏁 Script executed:
# Let's check the specific argument definitions more carefully for json, tower_url, and follow which appeared in output
rg "Arg::new\((\"json\"|\"tower_url\"|\"follow\")" crates/tower-cmd/src/ -A 8Repository: tower/tower-cli
Length of output: 1706
Filter prevents short-only flags from appearing in docs, though none currently exist in codebase.
Line 59 filters named args to a.get_long().is_some(), which excludes any arguments that have only a short form. While no short-only flags currently exist (all arguments define both short and long forms), this filter is overly restrictive and would silently omit them if added. The suggested fix makes the code more robust by handling all four combinations of short/long presence:
Suggested fix
- let named: Vec<_> = sub
- .get_arguments()
- .filter(|a| !a.is_positional() && !a.is_hide_set() && a.get_long().is_some())
- .collect();
+ let named: Vec<_> = sub
+ .get_arguments()
+ .filter(|a| {
+ !a.is_positional()
+ && !a.is_hide_set()
+ && (a.get_long().is_some() || a.get_short().is_some())
+ })
+ .collect();
@@
- for arg in &named {
- let long = arg.get_long().unwrap();
+ for arg in &named {
let req = if arg.is_required_set() {
" *(required)*"
} else {
""
};
@@
- if let Some(short) = arg.get_short() {
- out.push_str(&format!("- `-{}`, `--{}`{}{}\n", short, long, req, help));
- } else {
- out.push_str(&format!("- `--{}`{}{}\n", long, req, help));
- }
+ match (arg.get_short(), arg.get_long()) {
+ (Some(short), Some(long)) => {
+ out.push_str(&format!("- `-{}`, `--{}`{}{}\n", short, long, req, help));
+ }
+ (Some(short), None) => {
+ out.push_str(&format!("- `-{}`{}{}\n", short, req, help));
+ }
+ (None, Some(long)) => {
+ out.push_str(&format!("- `--{}`{}{}\n", long, req, help));
+ }
+ (None, None) => {}
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tower-cmd/src/skill.rs` around lines 57 - 96, The named-argument
filter and subsequent unwraps currently skip short-only flags and panic if long
is missing: adjust the filter on sub.get_arguments() (the named Vec) to include
arguments that have either get_long().is_some() OR get_short().is_some() (i.e.,
don't require a long), and update the rendering loop (where you call
arg.get_long().unwrap() and arg.get_short()) to handle the four cases (both
short+long, long-only, short-only, neither) by conditionally formatting the
output based on presence of arg.get_long() and arg.get_short() while still using
arg.get_id()/get_help() as before.
| ``` | ||
| tower_file_generate({}) # current directory | ||
| tower_file_generate({"working_directory": "/path/to/app"}) # explicit path | ||
| tower_run_local({"working_directory": "../other-app"}) | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint MD040.
These fences currently omit a language and trigger the static-analysis warnings.
🛠️ Suggested patch
-```
+```text
tower_file_generate({}) # current directory
tower_file_generate({"working_directory": "/path/to/app"}) # explicit path
tower_run_local({"working_directory": "../other-app"})- +text
tower_file_generate → tower_file_update → tower_file_add/edit/remove_parameter → tower_file_validate
-```
+```text
tower_run_local
- +text
tower_apps_create → tower_deploy → tower_run_remote
-```
+```text
tower_schedules_create # set up cron-based recurring runs
tower_schedules_list # view existing schedules
tower_schedules_update # modify timing or parameters
tower_schedules_delete # remove a schedule
- +text
tower_apps_list # list all apps
tower_apps_show # details and recent runs
tower_apps_logs # logs from a specific run
tower_teams_list, tower_teams_switch # manage team context
tower_secrets_create, tower_secrets_list # store credentials and API keys
Also applies to: 86-88, 94-96, 102-104, 110-115, 119-125
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/tower/SKILL.md` around lines 68 - 72, Multiple fenced code blocks in
SKILL.md (containing examples like tower_file_generate, tower_run_local,
tower_file_update, tower_file_add/edit/remove_parameter, tower_file_validate,
tower_apps_create, tower_deploy, tower_run_remote, tower_apps_list,
tower_apps_show, tower_apps_logs, tower_teams_list, tower_teams_switch,
tower_secrets_create, tower_secrets_list, and the schedules commands) are
missing language identifiers and trigger markdownlint MD040; update each
triple-backtick fence to include a language tag (recommend "text") so lines like
``` become ```text for every block shown (including the blocks containing the
three examples, the pipeline arrow lines, the single-command lines, the
schedules block, and the apps/teams/secrets blocks) to satisfy the linter.
socksy
force-pushed
the
feat/tow-1415-claude-code-plugin
branch
from
9475545 to
4869376
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/tower-cmd/src/skill.rs (2)
13-16: Consider removingasyncor adding a comment explaining its presence.The function has no
awaitpoints. If it's intentionally async for API consistency with other command handlers, a brief comment would clarify this. Otherwise, making it synchronous would better reflect its behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/skill.rs` around lines 13 - 16, The function do_skill_generate is declared async but contains no await points (it simply calls generate_skill_md and prints), so either remove the async from do_skill_generate to make it synchronous or keep it async but add a brief clarifying comment above do_skill_generate stating that it is intentionally async for API/handler consistency; update the signature and any callers if you remove async, or add the comment referencing do_skill_generate and generate_skill_md to explain why no awaits appear.
179-183: Add language identifiers to fenced code blocks to fix MD040 warnings.Several code blocks in
WORKFLOW_HEADERlack language specifiers, which causes markdownlint MD040 warnings in the generatedSKILL.md. Since these aren't bash commands but rather MCP tool invocations/pseudo-code, usetextas the language identifier.♻️ Suggested fix for code blocks in WORKFLOW_HEADER
-``` +```text tower_file_generate({}) # current directory tower_file_generate({"working_directory": "/path/to/app"}) # explicit path tower_run_local({"working_directory": "../other-app"})Apply the same pattern (`text` language identifier) to the other unlabeled fenced code blocks at lines 197, 205, 213, 221, and 230. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@crates/tower-cmd/src/skill.rsaround lines 179 - 183, Update the
WORKFLOW_HEADER constant to add language identifiers to its unlabeled fenced
code blocks so generated SKILL.md doesn't trigger MD040; replace every
triple-backtick block used for MCP tool invocations (the examples inside
WORKFLOW_HEADER such as the block containing tower_file_generate/tower_run_local
and the other unlabeled blocks at the same logical locations) to start with
text instead of; ensure you make the same change for all occurrences
referenced in WORKFLOW_HEADER (the blocks currently around the examples at lines
corresponding to the other unlabeled blocks) so all code fences in the generated
SKILL.md are explicitly labeled.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@crates/tower-cmd/src/skill.rs:
- Around line 13-16: The function do_skill_generate is declared async but
contains no await points (it simply calls generate_skill_md and prints), so
either remove the async from do_skill_generate to make it synchronous or keep it
async but add a brief clarifying comment above do_skill_generate stating that it
is intentionally async for API/handler consistency; update the signature and any
callers if you remove async, or add the comment referencing do_skill_generate
and generate_skill_md to explain why no awaits appear.- Around line 179-183: Update the WORKFLOW_HEADER constant to add language
identifiers to its unlabeled fenced code blocks so generated SKILL.md doesn't
trigger MD040; replace every triple-backtick block used for MCP tool invocations
(the examples inside WORKFLOW_HEADER such as the block containing
tower_file_generate/tower_run_local and the other unlabeled blocks at the same
logical locations) to start withtext instead of; ensure you make the
same change for all occurrences referenced in WORKFLOW_HEADER (the blocks
currently around the examples at lines corresponding to the other unlabeled
blocks) so all code fences in the generated SKILL.md are explicitly labeled.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `2987fa79-3b97-4995-b01e-e94afea87ca6` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 94755459962746cd34ada630a745bc7134d6400c and 486937600e888b1fc0ced1c774b5ef6ad26e54d3. </details> <details> <summary>📒 Files selected for processing (11)</summary> * `.claude-plugin/plugin.json` * `.coderabbit.yaml` * `.github/workflows/regenerate-skill.yml` * `.gitignore` * `.mcp.json` * `crates/tower-cmd/src/apps.rs` * `crates/tower-cmd/src/lib.rs` * `crates/tower-cmd/src/mcp.rs` * `crates/tower-cmd/src/secrets.rs` * `crates/tower-cmd/src/skill.rs` * `skills/tower/SKILL.md` </details> <details> <summary>✅ Files skipped from review due to trivial changes (8)</summary> * .gitignore * crates/tower-cmd/src/secrets.rs * .mcp.json * crates/tower-cmd/src/apps.rs * crates/tower-cmd/src/mcp.rs * .claude-plugin/plugin.json * .coderabbit.yaml * .github/workflows/regenerate-skill.yml </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * crates/tower-cmd/src/lib.rs </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
2 participants
Summary
Ships tower-cli as a Claude Code plugin. Users can install the whole thing — MCP server + skill — directly from this GitHub repo.
.claude-plugin/plugin.json— plugin manifest (name, version, metadata).mcp.json— startsuvx tower mcp-servervia stdio so Claude has structured Tower tools availableskills/tower/SKILL.md— generated fromtower skill generatewith auth detection and install fallbacks (uvx → pip → nix)tower skill generate— new CLI subcommand that emits the skill doc from clap metadata so the command reference stays in sync with the CLI.about()strings aligned with MCP tool descriptions where they'd drifted; sync comment added inmcp.rs(proc macros can't reference constants so full single-source would need a build script)Notes
The existing
.mcp.json(with dev-only Grafana config) has moved to.mcp.json.localand is gitignored. Developers who want both can keep their local overrides there.Summary by CodeRabbit
New Features
skill generatesubcommand for generating workflow documentation.Documentation
Chores
apps,secrets, and related commands.