feat(cli): add ironclaw channels list subcommand#933
Conversation
github-actions
bot
added
scope: channel/cli
Summary of ChangesHello, 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 introduces a new 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 introduces a new channels list subcommand to the CLI, which is a great first step for channel management. However, a potential security vulnerability exists where sensitive information, specifically the Signal channel's URL and account (phone number), is printed in verbose output without redaction, which could lead to the leakage of credentials or PII. This is inconsistent with existing redaction logic and violates the principle of preventing sensitive information leakage in tool output. Additionally, there are opportunities to improve code clarity and robustness by reducing code duplication, enhancing JSON serialization logic, and adding more informative error handling when scanning for WASM channels.
| ("http_url", sig.http_url.clone()), | ||
| ("account", sig.account.clone()), |
There was a problem hiding this comment.
The channels list --verbose command prints the Signal http_url and account (phone number) directly to stdout. The http_url may contain sensitive credentials if configured with basic auth, and the account is Personally Identifiable Information (PII). In src/main.rs, the URL is redacted before being logged, indicating it is considered sensitive. Printing these details in the CLI output can lead to accidental leakage of credentials or PII if the output is shared (e.g., in bug reports or logs).
References
- Tool output should be managed to prevent excessive memory/bandwidth usage and reduce the risk of leaking sensitive information.
| if let Some(ref gw) = config.gateway { | ||
| channels.push(ChannelInfo { | ||
| name: "gateway".to_string(), | ||
| kind: "built-in", | ||
| enabled: true, | ||
| details: vec![("host", gw.host.clone()), ("port", gw.port.to_string())], | ||
| }); | ||
| } else { | ||
| channels.push(ChannelInfo { | ||
| name: "gateway".to_string(), | ||
| kind: "built-in", | ||
| enabled: false, | ||
| details: vec![], | ||
| }); | ||
| } |
There was a problem hiding this comment.
The logic for creating ChannelInfo for optional channels like gateway, http, and signal is duplicated. This can be refactored to reduce repetition and improve maintainability. By determining the enabled status and details within the if let block, you can use a single channels.push call.
let (enabled, details) = if let Some(ref gw) = config.gateway {
(true, vec![("host", gw.host.clone()), ("port", gw.port.to_string())])
} else {
(false, vec![])
};
channels.push(ChannelInfo {
name: "gateway".to_string(),
kind: "built-in",
enabled,
details,
});| let entries: Vec<serde_json::Value> = channels | ||
| .iter() | ||
| .map(|ch| { | ||
| let mut v = serde_json::json!({ | ||
| "name": ch.name, | ||
| "kind": ch.kind, | ||
| "enabled": ch.enabled, | ||
| }); | ||
| if verbose { | ||
| let details: serde_json::Map<String, serde_json::Value> = ch | ||
| .details | ||
| .iter() | ||
| .map(|(k, v)| (k.to_string(), serde_json::Value::String(v.clone()))) | ||
| .collect(); | ||
| v["details"] = serde_json::Value::Object(details); | ||
| } | ||
| v | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
Manually constructing serde_json::Value can be verbose and less type-safe. Consider defining a dedicated serializable struct for the JSON output. This improves readability and maintainability. You can use #[serde(skip_serializing_if = "Option::is_none")] to conditionally include the details field for verbose output.
For example:
#[derive(serde::Serialize)]
struct JsonChannel<'a> {
name: &'a str,
kind: &'a str,
enabled: bool,
#[serde(skip_serializing_if = "Option::is_none")]
details: Option<std::collections::HashMap<&'a str, &'a str>>,
}Then you can map your ChannelInfo instances to this struct and serialize the collection.
| let mut entries = match tokio::fs::read_dir(dir).await { | ||
| Ok(entries) => entries, | ||
| Err(_) => return names, | ||
| }; |
There was a problem hiding this comment.
The read_dir error is currently ignored, which means the function will silently return an empty list for any error, including permission issues. It's better to log a warning for errors other than NotFound to provide more feedback to the user during debugging.
let mut entries = match tokio::fs::read_dir(dir).await {
Ok(entries) => entries,
Err(e) => {
if e.kind() != std::io::ErrorKind::NotFound {
eprintln!(
"Warning: Could not read WASM channels directory '{}': {}",
dir.display(),
e
);
}
return names;
}
};
|
Part of #83 |
|
Could you review this when you get a chance? @zmanian |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: Add ironclaw channels list subcommand
Well-structured CLI addition that follows established patterns in the codebase.
Positives:
- Thorough module-level documentation explaining why enable/disable/status are deferred (config source split, no IPC control plane) -- this is the right call
- WASM channel discovery matches the real loader's load_from_dir() behavior (top-level *.wasm only)
- --verbose and --json flags consistent with other CLI subcommands
- Good test coverage for WASM discovery edge cases (missing dir, non-wasm files, subdirectories)
Minor note:
- This PR and #918 (skills CLI) both modify src/cli/mod.rs and snapshot files. Whichever merges second will need a conflict resolution pass. Not blocking.
LGTM.
|
If ready to merge, you merge one of them first. and then I will fix another one. |
reidliu41
force-pushed
the
feat/channels-cli
branch
from
525be99 to
f13a6e3
Compare
- Add channels list with --verbose and --json flags to show all
configured built-in and WASM channels with their enabled state
- WASM discovery scans top-level *.wasm files, matching the real
loader's load_from_dir() behavior
- Respect global --config flag via Config::from_env_with_toml()
- Document why enable/disable/status are deferred: ChannelsConfig::resolve()
reads env vars only, settings.channels.* is not consumed, so toggling
via settings would silently fail; runtime status needs IPC not yet built
- Update FEATURE_PARITY.md channels row to partial (list only)
reidliu41
force-pushed
the
feat/channels-cli
branch
from
f13a6e3 to
9262e80
Compare
|
No conflict. |
2 participants
Summary
Change Type
Linked Issue
None
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-featuresSecurity Impact
None
Database Impact
None
Blast Radius
Rollback Plan
Review track: