feat(cli): show credential auth status in tool info - #1572
Conversation
`ironclaw tool info` now checks the secrets store and shows whether each required credential is configured or missing, consolidated into a single Auth section that deduplicates across http.credentials, auth, and setup.required_secrets. Secrets already shown in Auth are filtered from the Secrets section to avoid redundancy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: agent
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 significantly improves the user experience for managing tool authentication by centralizing and clarifying credential status checks within the Highlights
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. 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 enhances the ironclaw tool info command to display the configuration status of required credentials, consolidating information from various parts of the capabilities file into a unified 'Auth' section. The implementation is solid and achieves the goal described. I've suggested a refactoring to the logic that collects and deduplicates secrets to use a HashMap instead of a HashSet and linear scans. This would improve code clarity and performance, especially as the number of secrets grows.
| let mut auth_secrets: Vec<AuthSecretInfo> = Vec::new(); | ||
| let mut seen: std::collections::HashSet<String> = std::collections::HashSet::new(); | ||
|
|
||
| // auth.display_name is the best label — seed first. | ||
| if let Some(ref auth) = caps.auth { | ||
| seen.insert(auth.secret_name.clone()); | ||
| auth_secrets.push(AuthSecretInfo { | ||
| secret_name: auth.secret_name.clone(), | ||
| description: auth.display_name.clone(), | ||
| location: None, | ||
| }); | ||
| } | ||
|
|
||
| // setup.required_secrets.prompt is second-best label. | ||
| if let Some(ref setup) = caps.setup { | ||
| for secret in &setup.required_secrets { | ||
| if seen.insert(secret.name.clone()) { | ||
| auth_secrets.push(AuthSecretInfo { | ||
| secret_name: secret.name.clone(), | ||
| description: Some(secret.prompt.clone()), | ||
| location: None, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Merge injection location from http.credentials. | ||
| if let Some(ref http) = caps.http { | ||
| for cred in http.credentials.values() { | ||
| let loc = format!("{:?}", cred.location); | ||
| if let Some(existing) = auth_secrets | ||
| .iter_mut() | ||
| .find(|s| s.secret_name == cred.secret_name) | ||
| { | ||
| existing.location = Some(loc); | ||
| } else if seen.insert(cred.secret_name.clone()) { | ||
| auth_secrets.push(AuthSecretInfo { | ||
| secret_name: cred.secret_name.clone(), | ||
| description: None, | ||
| location: Some(loc), | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic for collecting and deduplicating authentication secrets from different parts of the capabilities file is a bit complex. It uses a HashSet to track seen secrets and a linear scan (find) within a loop to update existing entries. This can be simplified and made more efficient by using a HashMap to map secret names to their index in the auth_secrets vector. This avoids the O(n) scan for each credential and makes the logic for updating vs. inserting more direct.
let mut auth_secrets: Vec<AuthSecretInfo> = Vec::new();
let mut seen: std::collections::HashMap<String, usize> = std::collections::HashMap::new();
// auth.display_name is the best label — seed first.
if let Some(ref auth) = caps.auth {
let index = auth_secrets.len();
seen.insert(auth.secret_name.clone(), index);
auth_secrets.push(AuthSecretInfo {
secret_name: auth.secret_name.clone(),
description: auth.display_name.clone(),
location: None,
});
}
// setup.required_secrets.prompt is second-best label.
if let Some(ref setup) = caps.setup {
for secret in &setup.required_secrets {
if !seen.contains_key(&secret.name) {
let index = auth_secrets.len();
seen.insert(secret.name.clone(), index);
auth_secrets.push(AuthSecretInfo {
secret_name: secret.name.clone(),
description: Some(secret.prompt.clone()),
location: None,
});
}
}
}
// Merge injection location from http.credentials.
if let Some(ref http) = caps.http {
for cred in http.credentials.values() {
let loc = format!("{:?}", cred.location);
if let Some(&index) = seen.get(&cred.secret_name) {
auth_secrets[index].location = Some(loc);
} else {
let index = auth_secrets.len();
seen.insert(cred.secret_name.clone(), index);
auth_secrets.push(AuthSecretInfo {
secret_name: cred.secret_name.clone(),
description: None,
location: Some(loc),
});
}
}
}| let extra: Vec<_> = secrets | ||
| .allowed_names | ||
| .iter() | ||
| .filter(|name| !seen.contains(name.as_str())) |
There was a problem hiding this comment.
If you adopt my other suggestion to use a HashMap for the seen collection, you will also need to update this line to use contains_key to check for the presence of a secret.
.filter(|name| !seen.contains_key(name.as_str()))There was a problem hiding this comment.
Resolved — the filtering now uses collected.seen_names: HashSet which uses .contains() correctly.
github-actions
Bot
added
size: L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let secrets_store = match init_secrets_store().await { | ||
| Ok(store) => Some(store), | ||
| Err(e) => { | ||
| eprintln!(" Warning: could not init secrets store: {}", e); | ||
| None | ||
| } | ||
| }; |
There was a problem hiding this comment.
tool info initializes the secrets store unconditionally; when secrets are not configured (e.g., missing SECRETS_MASTER_KEY), this will emit a warning even for tools that have no auth-related secrets. Consider lazily initializing the secrets store only if the parsed capabilities actually contain auth/setup/http.credentials secrets that you plan to check.
There was a problem hiding this comment.
Resolved in 9632302 — secrets store is now lazily initialized only when caps.auth, caps.setup.required_secrets, or caps.http.credentials are present.
| } | ||
|
|
||
| // Only filter secrets already in auth when we will actually render the Auth section. | ||
| let will_render_auth = secrets_store.is_some() && !auth_secrets.is_empty(); |
There was a problem hiding this comment.
Auth output is currently gated on secrets_store.is_some(). If secrets store initialization fails, the Auth: section is suppressed entirely, which can hide required credential names/locations that are still useful to display. Consider always rendering the consolidated Auth section when auth_secrets is non-empty, using an unknown status (or similar) when the store is unavailable, and still applying the Secrets-section dedupe accordingly.
| let will_render_auth = secrets_store.is_some() && !auth_secrets.is_empty(); | |
| let will_render_auth = !auth_secrets.is_empty(); |
There was a problem hiding this comment.
Resolved in 9632302 — Auth section is always rendered when auth_secrets is non-empty, using ? unknown when the store is unavailable. Secrets-section dedupe is also applied unconditionally now since Auth always renders.
| // Build auth_secrets the same way print_capabilities_detail does. | ||
| let mut auth_secrets: Vec<AuthSecretInfo> = Vec::new(); | ||
| let mut seen: std::collections::HashMap<String, usize> = std::collections::HashMap::new(); | ||
|
|
||
| if let Some(ref auth) = caps.auth { | ||
| let index = auth_secrets.len(); | ||
| seen.insert(auth.secret_name.clone(), index); | ||
| auth_secrets.push(AuthSecretInfo { | ||
| secret_name: auth.secret_name.clone(), | ||
| description: auth.display_name.clone(), | ||
| location: None, | ||
| }); | ||
| } | ||
| if let Some(ref setup) = caps.setup { | ||
| for secret in &setup.required_secrets { | ||
| if !seen.contains_key(&secret.name) { | ||
| let index = auth_secrets.len(); | ||
| seen.insert(secret.name.clone(), index); | ||
| auth_secrets.push(AuthSecretInfo { | ||
| secret_name: secret.name.clone(), | ||
| description: Some(secret.prompt.clone()), | ||
| location: None, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| if let Some(ref http) = caps.http { | ||
| for cred in http.credentials.values() { | ||
| let loc = format!("{:?}", cred.location); | ||
| if let Some(&index) = seen.get(&cred.secret_name) { | ||
| auth_secrets[index].location = Some(loc); | ||
| } else { | ||
| let index = auth_secrets.len(); | ||
| seen.insert(cred.secret_name.clone(), index); | ||
| auth_secrets.push(AuthSecretInfo { | ||
| secret_name: cred.secret_name.clone(), | ||
| description: None, | ||
| location: Some(loc), | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This test re-implements the auth-secret collection logic inline rather than exercising print_capabilities_detail (or a shared helper), so it can pass even if the production dedup/merge behavior diverges. Consider extracting the auth-secret collection/dedup (and secrets filtering decision) into a helper that returns structured data, and unit test that helper directly (and/or capture/assert rendered output).
There was a problem hiding this comment.
Resolved in 9632302 — extracted collect_auth_secrets() helper that returns CollectedAuthSecrets. Test now calls this helper directly instead of re-implementing the logic. Also added test_collect_auth_secrets_empty_caps for the empty-capabilities edge case.
…h section Address review feedback: - Extract dedup logic into `collect_auth_secrets()` so the test exercises the same code path as production (not a re-implementation) - Always render the Auth section when auth secrets exist, showing "? unknown" status when the secrets store is unavailable instead of hiding credential names entirely - Lazily init secrets store only when capabilities contain auth secrets, avoiding spurious warnings for tools with no auth - Add test for empty capabilities edge case Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressing @zmanian's review suggestions:
|
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Code Review — show credential auth status in tool info
+268 / -14 in src/cli/tool.rs. Adds credential status checking (✓ configured / ✗ missing) to ironclaw tool info. Nice UX improvement.
Issues
1. Secrets store errors silently suppress Auth section (medium)
When init_secrets_store() fails (commands.rs:451-454), secrets_store is None and print_capabilities_detail receives no store. The Auth section then shows ? unknown for every secret. This is handled, but the eprintln! warning could be easy to miss in a wall of output.
More importantly, collect_auth_secrets still runs and populates seen_names, so secrets covered by auth entries are filtered from the Secrets section — but their status is shown as ? unknown. The user loses information about which secrets are needed. Consider showing the Auth section with ? unknown status AND keeping those entries in the Secrets section when the store is unavailable.
2. --user flag defaults to "default" silently (low-medium)
The --user flag (tool.rs:83) defaults to "default", but ironclaw tool auth --user X can store credentials under any user ID. A user who ran tool auth --user myid will see ✗ missing in tool info unless they also pass --user myid. Consider documenting this in the --help text, e.g., "Must match the --user used with tool auth".
3. Debug formatting for credential location (low)
format!("{:?}", cred.location) (tool.rs:580) uses Rust's Debug trait, producing output like AuthorizationBearer — which is the variant name without spaces. If CredentialLocation has a Display impl, prefer that. If not, this is fine but slightly jarring for users.
4. Test exercises collection logic but not rendering (low)
test_auth_secret_dedup_and_status (tool.rs:1305) tests collect_auth_secrets and the store's exists() method, but doesn't exercise print_capabilities_detail. The dedup logic could diverge from the rendering path. This is acceptable for a first pass — the collection function is the interesting part.
What's good
- Lazy secrets store init: only initializes when auth sections exist (avoids errors for tools without auth)
- Clean dedup with priority: auth.display_name > setup.prompt, injection location merged from http.credentials
- Secrets section filtering: removes entries already shown in Auth, keeps wildcards (
gh_*) - Sorted output:
collected.secrets.sort_by(...)gives deterministic output - Good test coverage: dedup, empty caps, store integration with create/exists
Verdict
Approve. No blocking issues. Item 1 (store failure suppressing auth info) is the main thing to consider improving, but the current ? unknown fallback is reasonable for a v1.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "credentials": { | ||
| "github": { | ||
| "secret_name": "gh_token", | ||
| "location": "AuthorizationBearer", |
There was a problem hiding this comment.
The JSON fixture in test_auth_secret_dedup_and_status uses an invalid http.credentials.*.location value ("AuthorizationBearer"). In the capabilities schema, location is a tagged object (e.g. { "type": "bearer" }, { "type": "header", ... }), so CapabilitiesFile::from_json(...).unwrap() will panic and the test will fail. Update the fixture to use the correct schema representation for location (and, if you expect the rendered output to say AuthorizationBearer, adjust the rendering logic accordingly; today it will format the schema variant as Bearer).
| "location": "AuthorizationBearer", | |
| "location": { "type": "bearer" }, |
There was a problem hiding this comment.
Fixed in 19c118d — updated test fixture to use {"type": "bearer"} which matches the CredentialLocationSchema tagged enum format.
The CredentialLocationSchema uses serde tagged enum format
({"type": "bearer"}), not a bare string ("AuthorizationBearer").
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@zmanian Thanks for both reviews. Here's where each item landed: First review (non-blocking suggestions):
Second review (4 issues):
|
* feat(cli): show credential auth status in `tool info`
`ironclaw tool info` now checks the secrets store and shows whether
each required credential is configured or missing, consolidated into
a single Auth section that deduplicates across http.credentials,
auth, and setup.required_secrets. Secrets already shown in Auth are
filtered from the Secrets section to avoid redundancy.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(cli): address review feedback on tool info auth status
- Fix clippy collapsible-if by using `if let` + `&&`
- Use HashMap<String, usize> for O(1) dedup instead of HashSet + linear scan
- Add --user flag to `tool info` for checking non-default user credentials
- Show "? unknown" on secrets store errors instead of silently reporting missing
- Surface secrets store init failure via eprintln instead of silent .ok()
- Sort auth entries by secret name for deterministic output
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(cli): only filter secrets when auth section renders, add regression test
When the secrets store fails to initialize, the Auth section is not
rendered. Previously, secret names were still filtered from the Secrets
section, causing credential names to disappear entirely. Now secrets
are only filtered when the Auth section will actually be displayed.
Adds test verifying auth secret deduplication across auth, setup, and
http.credentials sections, plus secrets store existence checks.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(cli): extract collect_auth_secrets helper, always render Auth section
Address review feedback:
- Extract dedup logic into `collect_auth_secrets()` so the test exercises
the same code path as production (not a re-implementation)
- Always render the Auth section when auth secrets exist, showing
"? unknown" status when the secrets store is unavailable instead of
hiding credential names entirely
- Lazily init secrets store only when capabilities contain auth secrets,
avoiding spurious warnings for tools with no auth
- Add test for empty capabilities edge case
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* style(cli): move HashMap/HashSet imports to top of file
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(cli): use correct tagged JSON format for credential location in test
The CredentialLocationSchema uses serde tagged enum format
({"type": "bearer"}), not a bare string ("AuthorizationBearer").
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(cli): show credential auth status in `tool info`
`ironclaw tool info` now checks the secrets store and shows whether
each required credential is configured or missing, consolidated into
a single Auth section that deduplicates across http.credentials,
auth, and setup.required_secrets. Secrets already shown in Auth are
filtered from the Secrets section to avoid redundancy.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(cli): address review feedback on tool info auth status
- Fix clippy collapsible-if by using `if let` + `&&`
- Use HashMap<String, usize> for O(1) dedup instead of HashSet + linear scan
- Add --user flag to `tool info` for checking non-default user credentials
- Show "? unknown" on secrets store errors instead of silently reporting missing
- Surface secrets store init failure via eprintln instead of silent .ok()
- Sort auth entries by secret name for deterministic output
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(cli): only filter secrets when auth section renders, add regression test
When the secrets store fails to initialize, the Auth section is not
rendered. Previously, secret names were still filtered from the Secrets
section, causing credential names to disappear entirely. Now secrets
are only filtered when the Auth section will actually be displayed.
Adds test verifying auth secret deduplication across auth, setup, and
http.credentials sections, plus secrets store existence checks.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(cli): extract collect_auth_secrets helper, always render Auth section
Address review feedback:
- Extract dedup logic into `collect_auth_secrets()` so the test exercises
the same code path as production (not a re-implementation)
- Always render the Auth section when auth secrets exist, showing
"? unknown" status when the secrets store is unavailable instead of
hiding credential names entirely
- Lazily init secrets store only when capabilities contain auth secrets,
avoiding spurious warnings for tools with no auth
- Add test for empty capabilities edge case
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* style(cli): move HashMap/HashSet imports to top of file
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(cli): use correct tagged JSON format for credential location in test
The CredentialLocationSchema uses serde tagged enum format
({"type": "bearer"}), not a bare string ("AuthorizationBearer").
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
ironclaw tool info <name>now checks the secrets store and shows whether each required credential is configured or missinghttp.credentials,auth, andsetup.required_secretsinto a single deduplicated Auth section with✓ configured/✗ missingstatusBefore
After
Test plan
ironclaw tool info <tool_with_auth>— verify single Auth section with statusironclaw tool info <tool_without_auth>— verify no Auth section appearsironclaw tool auth, re-runtool info— verify✓ configured🤖 Generated with Claude Code