fix: address review feedback on MCP parameter handling and run status

[bradhe]

Addresses review feedback on #237.

Changes:

• Block rename collisions in edit_parameter (mcp.rs): If a new_name is provided and that name already exists in the Towerfile, the edit now returns an error instead of silently creating a duplicate.

• Handle Status::Failed in the monitor loop (run.rs): The local run polling loop now explicitly matches Status::Failed and returns immediately, rather than falling through to _ => continue and polling forever.

• Handle Status::Failed in the post-monitor result match (run.rs): After monitoring completes, Status::Failed is now an explicit arm that surfaces the error code and message to the user, rather than hitting the generic unexpected-status branch.

[coderabbitai]

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and concisely summarizes the main changes: addressing review feedback on parameter handling and run status across two files (mcp.rs and run.rs).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/coderabbit-pr237-feedback

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[socksy]

What? Why?

[bradhe]

Ah sorry yeah Claude hallucinated. Will fix this.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/tower-cmd/src/mcp.rs (1)

880-888: ⚠️ Potential issue | 🟠 Major

Enforce visible-parameter invariants on edit as well.

After edits, hidden=false can still produce empty description/default (e.g., toggling hidden→visible without providing values). That bypasses the same invariant enforced on add.

Proposed fix

             let param = Parameter {
                 name: target_name,
                 description: request.description.unwrap_or_else(|| existing.description.clone()),
                 default: request.default.unwrap_or_else(|| existing.default.clone()),
                 hidden: request.hidden.unwrap_or(existing.hidden),
             };
+            if !param.hidden {
+                if param.description.trim().is_empty() {
+                    return Err("description is required when hidden=false".into());
+                }
+                if param.default.trim().is_empty() {
+                    return Err("default is required when hidden=false".into());
+                }
+            }
             if param.hidden && !param.default.is_empty() {
                 return Err("hidden and default are mutually exclusive".into());
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/tower-cmd/src/mcp.rs` around lines 880 - 888, When constructing the
edited Parameter (the local variable param built from request and existing),
enforce the same visible-parameter invariants as on add: after creating param
(and after the existing hidden+default mutual-exclusion check), validate that if
param.hidden is false then param.description and param.default are non-empty and
return an Err with a clear message if either is empty; keep the existing
hidden+default check as well. Refer to the Parameter struct and the local
variables param, request, and existing to locate where to add these checks.

🤖 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/mcp.rs`:
- Around line 846-848: The check currently only rejects None for visible params;
update the validation around request.default (the branch that returns
Self::text_error("default is required when hidden=false".into())) to also reject
empty or whitespace-only strings by trimming and treating them as missing.
Locate the conditional that uses request.default.is_none() and replace it with a
test that treats Some(s) where s.trim().is_empty() as invalid (so it calls
Self::text_error in that case as well), keeping the same error message.

---

Outside diff comments:
In `@crates/tower-cmd/src/mcp.rs`:
- Around line 880-888: When constructing the edited Parameter (the local
variable param built from request and existing), enforce the same
visible-parameter invariants as on add: after creating param (and after the
existing hidden+default mutual-exclusion check), validate that if param.hidden
is false then param.description and param.default are non-empty and return an
Err with a clear message if either is empty; keep the existing hidden+default
check as well. Refer to the Parameter struct and the local variables param,
request, and existing to locate where to add these checks.

🪄 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

Run ID: fae6f15e-f41f-4bf1-9e89-c7e7045039ca

📥 Commits

Reviewing files that changed from the base of the PR and between 7c33c8c and f7eb961.

📒 Files selected for processing (2)

• crates/tower-cmd/src/mcp.rs
• crates/tower-cmd/src/run.rs