Conversation
* feat(params): add secret parameter support * Rename secret parameter field to hidden
bradhe
requested review from
codingcyclist,
Copilot,
giray123,
jo-sm,
konstantinoscs,
sammuti and
socksy
📝 WalkthroughWalkthroughThe PR bumps package version to 0.3.50 and introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR releases version 0.3.50 of the Tower CLI, implementing several bug fixes and a new hidden parameters feature. The changes improve the CLI's robustness when handling signals and broken pipes, fix issues with schedule command argument parsing, and add support for marking parameters as hidden to prevent their values from being displayed in schedule listings.
Changes:
- Added hidden parameter support across API models, towerfile configuration, and MCP tools
- Fixed schedule update/delete commands by replacing external subcommand pattern with explicit positional arguments
- Restored default SIGINT behavior in Python CLI entrypoint to fix ctrl+c handling
- Added graceful broken pipe handling to prevent panics when piping CLI output
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Version bump to 0.3.50 |
| pyproject.toml | Version bump to 0.3.50 |
| Cargo.toml | Version bump to 0.3.50 |
| Cargo.lock | Version bump to 0.3.50 for all workspace crates |
| src/tower/cli.py | Added SIGINT signal handler to restore default behavior |
| tests/tower/test_cli.py | Added test coverage for SIGINT signal handling |
| crates/tower/src/lib.rs | Code formatting improvements for method chains |
| crates/tower-cmd/src/schedules.rs | Fixed update/delete commands to use positional args, improved parse_parameters, added comprehensive tests |
| crates/tower-cmd/src/output.rs | Added write_to_stdout/stderr helpers with broken pipe handling |
| crates/tower-cmd/src/mcp.rs | Added masking of hidden parameter values in schedule listings |
| crates/tower-cmd/src/api.rs | Added hidden field to RunParameter creation, explicitly set update_schedule fields |
| crates/tower-api/src/models/run_parameter.rs | Added hidden field with serde default |
| crates/tower-api/src/models/parameter.rs | Added hidden field with serde default |
| crates/config/src/towerfile.rs | Added hidden field to Parameter struct with tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/config/src/towerfile.rs (1)
122-130: Consider adding ahiddenparameter toadd_parameter()for API completeness.Currently,
add_parameter()always setshidden: false. If programmatic creation of hidden parameters is needed in the future, this method would need modification.💡 Optional enhancement
- pub fn add_parameter(&mut self, name: String, description: String, default: String) { + pub fn add_parameter(&mut self, name: String, description: String, default: String, hidden: bool) { self.parameters.push(Parameter { name, description, default, - hidden: false, + hidden, }); }This would require updating callers to pass
falsefor the new parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/config/src/towerfile.rs` around lines 122 - 130, Update the add_parameter method to accept a hidden: bool argument and use it when constructing the Parameter so callers can create hidden parameters; specifically change the signature of add_parameter (currently pub fn add_parameter(&mut self, name: String, description: String, default: String)) to include hidden: bool and set Parameter { name, description, default, hidden } accordingly, then update all callers to pass false where they previously relied on the default non-hidden behavior.crates/tower-cmd/src/api.rs (2)
881-886: Same suggestion applies here.Consider using
RunParameter::new(key, value)for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/api.rs` around lines 881 - 886, Replace the inline construction of RunParameter in the iterator map with the canonical constructor: change the closure that currently returns RunParameter { name: key, value, hidden: false } to call RunParameter::new(key, value) so the mapping uses the struct's constructor for consistency with other call sites (refer to RunParameter::new and the map producing RunParameter instances).
843-848: Consider usingRunParameter::new()constructor.The struct literal works, but using the constructor would be more maintainable if the struct evolves (fewer places to update).
♻️ Suggested refactor
let run_parameters = parameters.map(|params| { params .into_iter() - .map(|(key, value)| RunParameter { - name: key, - value, - hidden: false, - }) + .map(|(key, value)| RunParameter::new(key, value)) .collect() });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/api.rs` around lines 843 - 848, Replace the inline struct literal used in the map with the RunParameter constructor to centralize construction logic: in the closure mapping (where RunParameter { name: key, value, hidden: false } is created) call RunParameter::new(key, value) (or RunParameter::new(key, value, false) if the constructor requires an explicit hidden flag) so future field changes only need updates in the RunParameter::new implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/config/src/towerfile.rs`:
- Around line 122-130: Update the add_parameter method to accept a hidden: bool
argument and use it when constructing the Parameter so callers can create hidden
parameters; specifically change the signature of add_parameter (currently pub fn
add_parameter(&mut self, name: String, description: String, default: String)) to
include hidden: bool and set Parameter { name, description, default, hidden }
accordingly, then update all callers to pass false where they previously relied
on the default non-hidden behavior.
In `@crates/tower-cmd/src/api.rs`:
- Around line 881-886: Replace the inline construction of RunParameter in the
iterator map with the canonical constructor: change the closure that currently
returns RunParameter { name: key, value, hidden: false } to call
RunParameter::new(key, value) so the mapping uses the struct's constructor for
consistency with other call sites (refer to RunParameter::new and the map
producing RunParameter instances).
- Around line 843-848: Replace the inline struct literal used in the map with
the RunParameter constructor to centralize construction logic: in the closure
mapping (where RunParameter { name: key, value, hidden: false } is created) call
RunParameter::new(key, value) (or RunParameter::new(key, value, false) if the
constructor requires an explicit hidden flag) so future field changes only need
updates in the RunParameter::new implementation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlcrates/config/src/towerfile.rscrates/tower-api/src/models/parameter.rscrates/tower-api/src/models/run_parameter.rscrates/tower-cmd/src/api.rscrates/tower-cmd/src/mcp.rscrates/tower-cmd/src/output.rscrates/tower-cmd/src/schedules.rscrates/tower/src/lib.rspyproject.tomlsrc/tower/cli.pytests/tower/test_cli.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests