Improve --help: add detailed about/examples/color, snapshot test (clo…#371
Conversation
github-actions
bot
added
scope: channel/cli
Summary of ChangesHello @BroccoliFin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the command-line interface's help system, making it more user-friendly and informative. By incorporating detailed descriptions and examples, users can more easily understand the functionality and usage of various commands. The addition of colored output further enhances the user experience, while a new snapshot test provides a robust mechanism to prevent regressions in the help documentation. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the usability of the CLI by enhancing the --help output. The addition of detailed descriptions, usage examples, and automatic color support is a great enhancement for users. Introducing snapshot testing for the help output with insta is an excellent practice to ensure consistency and prevent regressions. The implementation is straightforward and well-executed. I have one minor suggestion for code consistency.
| long_about = "IronClaw is a secure AI assistant. Use 'ironclaw <subcommand> --help' for details.\nExamples:\n ironclaw run # Start the agent\n ironclaw config list # List configs" | ||
| )] | ||
| #[command(version)] | ||
| #[command(color = ColorChoice::Auto)] // Включи авто-цвет для help (если терминал поддерживает) |
There was a problem hiding this comment.
For consistency with the rest of the codebase, which is in English, this comment should also be in English. This improves maintainability for all contributors.
| #[command(color = ColorChoice::Auto)] // Включи авто-цвет для help (если терминал поддерживает) | |
| #[command(color = ColorChoice::Auto)] // Enable auto-color for help (if the terminal supports it) |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Summary
This is a straightforward, low-risk PR that improves CLI help text by adding long_about descriptions with usage examples and a snapshot test. No security concerns, no runtime behavior changes beyond help output. Two findings warrant attention before merge: (1) the MCP example in long_about is incorrect — mcp add requires two positional args but the example only provides one, and (2) the snapshot test uses render_help() (short help) rather than render_long_help(), so the primary new content (the long_about strings) is not actually tested. The pairing example has a similar issue. The rest are minor: internal commands should be hidden from help, redundant doc comments create divergence risk, and the explicit ColorChoice::Auto is a no-op.
| #[command(subcommand)] | ||
| #[command( | ||
| subcommand, | ||
| about = "Manage MCP servers", |
There was a problem hiding this comment.
MCP long_about example is incorrect — missing required argument
The long_about for the Mcp variant contains the example ironclaw mcp add my-server-url, but McpCommand::Add requires TWO positional arguments: name: String and url: String. Running this example will fail with a missing argument error. The example treats my-server-url as a single positional arg (the name), leaving url unprovided.
Suggested fix:
Change the example to include both arguments, e.g.:
`long_about = "Add, auth, list, or test MCP servers.\nExample: ironclaw mcp add notion https://mcp.notion.com"`Severity: medium · Confidence: high
| #[command(subcommand)] | ||
| #[command( | ||
| subcommand, | ||
| about = "Manage DM pairing", |
There was a problem hiding this comment.
Pairing long_about example is incomplete — missing required arguments
The long_about for Pairing contains ironclaw pairing approve, but PairingCommand::Approve requires two positional arguments: channel: String (e.g. 'telegram') and code: String (e.g. 'ABC12345'). Running this example will produce a missing-argument error. While approve is a valid subcommand name, the example is misleading as presented — it implies it can be run as-is.
Suggested fix:
Change the example to something valid, e.g.:
`long_about = "Approve or manage pairing requests.\nExamples:\n ironclaw pairing list telegram\n ironclaw pairing approve telegram ABC12345"`Severity: low · Confidence: high
|
|
||
| #[test] | ||
| fn test_help_output() { | ||
| let mut cmd = Cli::command(); |
There was a problem hiding this comment.
Snapshot test only captures short help — long_about content is untested
The new test_help_output test calls cmd.render_help() which returns the SHORT help text (triggered by -h). The long_about strings — which are the primary new feature of this PR — are only displayed via --help (the long form) and would be rendered by cmd.render_long_help(). As a result, the main new content (detailed descriptions with usage examples) is not covered by any test. If someone introduces a typo or removes a long_about, the snapshot test will not catch it.
Suggested fix:
Add a second snapshot test for long help:
```rust
#[test]
fn test_long_help_output() {
let mut cmd = Cli::command();
let help = cmd.render_long_help().to_string();
assert_snapshot!(help);
}
```Severity: medium · Confidence: high
| long_about = "Generates shell completion scripts.\nExample: ironclaw completion --shell bash > ironclaw.bash" | ||
| )] | ||
| Completion(Completion), | ||
|
|
There was a problem hiding this comment.
Internal commands (Worker, ClaudeBridge) visible in user-facing help
The Worker and ClaudeBridge variants are explicitly documented as internal-use (This is invoked automatically by the orchestrator, not by users directly), yet they appear in the --help output visible to all users. The snapshot confirms they're listed alongside user-facing commands. This is a pre-existing issue, but since this PR specifically aims to improve user-facing help quality, it's a missed opportunity to hide internal commands.
Suggested fix:
Add `#[command(hide = true)]` to both `Worker` and `ClaudeBridge` variants:
```rust
/// Run as a sandboxed worker (internal use).
#[command(hide = true)]
Worker { ... },
/// Run as a Claude Code bridge (internal use).
#[command(hide = true)]
ClaudeBridge { ... },
```Severity: low · Confidence: high
|
@BroccoliFin if you resolve the conflict we can merge after CI pass |
BroccoliFin
force-pushed
the
feat/help-improvements
branch
from
5827cc4 to
d1872fb
Compare
- Add --grep/-g flag for regex filtering (conflicts with --follow) - Add --ignore-case for case-insensitive matching - Add --context N to show lines around matches - Implement filter_lines() with regex crate - Add 6 comprehensive tests - Update documentation with examples Branching from staging as suggested in nearai#371.
* feat(cli/logs): add --grep support for filtering log output - Add --grep/-g flag for regex filtering (conflicts with --follow) - Add --ignore-case for case-insensitive matching - Add --context N to show lines around matches - Implement filter_lines() with regex crate - Add 6 comprehensive tests - Update documentation with examples Branching from staging as suggested in #371. * refactor(cli/logs): simplify regex compilation per bot feedback - Use RegexBuilder::new().case_insensitive(ignore_case) instead of if/else - Fix test assertion to avoid move issue (single unwrap_err() call) - Apply cargo fmt formatting * fix(cli/logs): address maintainer feedback - Use saturating_add to prevent integer overflow in context calculation - Add test_filter_lines_with_non_overlapping_context to verify BTreeSet dedup logic Addresses review comments on PR #1533.
2 participants
Improved --help output:
Closes feat: group chat privacy, channel-aware prompts, and safety hardening #285. Tested: cargo test passes, ironclaw --help shows enhanced colored output with examples.
@ilblackdragon @serrrfirat…ses feat: group chat privacy, channel-aware prompts, and safety hardening #285)
Updated based on feedback:
All suggestions have been taken into account (moderate/low severity findings have been resolved). Ready for re-review! @serrrfirat