fix(cli): fall back to channel capabilities for auth#1496
fix(cli): fall back to channel capabilities for auth#1496G7CNF wants to merge 2 commits intonearai:stagingfrom
Conversation
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 enhances 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a robust fallback mechanism for resolving capabilities files for WASM tools. The system now intelligently checks the channels directory if a capabilities file is not found in the primary tools directory or a user-specified location. This enhancement directly addresses the issue of authenticating registry-installed WASM channels, which often store their capabilities alongside channel artifacts, without requiring manual workarounds. The auth_tool and setup_tool functions have been updated to leverage this new resolution logic, and the error messages have been improved to provide clearer guidance to users by indicating both potential lookup paths. A dedicated unit test has been added to ensure the fallback behavior functions as expected, contributing to the overall reliability of the CLI tool management. The changes are well-implemented and enhance the user experience for managing WASM tools.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f787c52466
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ilblackdragon
left a comment
There was a problem hiding this comment.
Review: fix(cli): fall back to channel capabilities for auth
Must-fix
- Duplicate path in error message — when
dirisNone,resolve_capabilities_pathreturns~/.ironclaw/channels/<name>.capabilities.jsonas primary. The error message then shows this same path twice:
Tool 'telegram' not found at ~/.ironclaw/channels/telegram.capabilities.json
(or ~/.ironclaw/channels/telegram.capabilities.json for WASM channels)
Fix to show both the channels path AND the tools path.
combine_provider_scopesonly scans tools directory — inauth_tool,tools_diris always resolved as the tools directory. When a channel's capabilities are found in~/.ironclaw/channels/, other channels sharing the same OAuth credential won't have their scopes combined, leading to incomplete scope requests.
Should-fix
-
Stale
--dirhelp text —AuthandSetupsubcommand definitions still say"default: ~/.ironclaw/tools/"but effective default now prefers~/.ironclaw/channels/. -
Fourth copy of
default_channels_dir()— already defined inwasm/loader.rs(pub),config/channels.rs,cli/status.rs. Import the public one fromcrate::channels::wasm::loader. -
Tests should use
tempfilecrate — per project testing rules, usetempfile::tempdir()instead ofstd::env::temp_dir()with UUID suffixes. -
Missing test cases — no test for tools-only fallback (no channel file), no test for explicit
--dirbypass, no test for neither-file-exists case. -
assert!(x.is_ok())discards errors — use.expect()in test setup code to preserve error details on failure.
ilblackdragon
left a comment
There was a problem hiding this comment.
Verdict: Request changes
The PR's core idea is right, but the current commit introduces new bugs while partially fixing the original one, and earlier review feedback is still unaddressed.
Must-fix
1. Duplicate path in error message
src/cli/tool.rs:596–606 (and the mirror in setup_tool): when dir is None, resolve_capabilities_path returns ~/.ironclaw/channels/<name>.capabilities.json as the primary path. The error then prints that same path a second time via default_channels_dir().join(...):
not found ... at ~/.ironclaw/channels/telegram.capabilities.json
(or ~/.ironclaw/channels/telegram.capabilities.json for WASM channels)
Print the tools path as the "or" branch, or show both unconditionally.
2. `combine_provider_scopes` only scans the tools directory
`src/cli/tool.rs:694` calls `combine_provider_scopes(&tools_dir, ...)` where `tools_dir` is hard-coded as `dir.unwrap_or_else(default_tools_dir)` (`:595`) — even when `caps_path` actually resolved into `~/.ironclaw/channels/`. For a channel whose OAuth credential is shared with installed tools (or vice versa), the scope merge silently skips the other directory and produces an incomplete authorization URL. Fix: scan both `default_tools_dir()` and `default_channels_dir()` (plus any user `--dir`) and deduplicate.
3. Collision semantics merely inverted, not resolved
The previous P1 ("tool wins over channel") was addressed by making `channels/` the primary lookup at `:33`. But this just inverts the failure: if a user has a legitimate same-named tool installed, `ironclaw tool auth ` will now silently select the channel's caps. The CLI has no way to express intent. Options: (a) require `--kind tool|channel` when both exist, (b) prefer tools and only fall back to channels (the original design), (c) fail with a disambiguation error listing both paths. The current test `test_resolve_capabilities_path_prefers_channels_dir_on_collision` just locks in the inverted behavior without justification.
Should-fix
4. Stale `--dir` help text
`src/cli/tool.rs:124` and `:138`: `Auth` and `Setup` subcommands still advertise `"default: ~/.ironclaw/tools/"`. Update to reflect channel-first resolution.
5. Fifth copy of `default_channels_dir()` (review said "fourth")
Existing copies: `src/channels/wasm/loader.rs:421` (pub), `src/channels/wasm/mod.rs:352`, `src/config/channels.rs:398`, `src/cli/status.rs:244`. This PR adds a 5th at `src/cli/tool.rs:22`. Import `crate::channels::wasm::loader::default_channels_dir` instead.
6. `tools_dir` variable kept only for `combine_provider_scopes`
After the resolver change, `tools_dir` at `src/cli/tool.rs:595` is otherwise dead. Once #2 is applied, this local should disappear entirely.
Nits
- Tests use `std::env::temp_dir()` + UUID (`:1188`, `:1212`) — project convention is `tempfile::tempdir()` which auto-cleans on drop and survives panics.
- `assert!(std::fs::create_dir_all(...).is_ok(), ...)` discards the underlying error. Use `.expect("create tools dir")`.
- Missing test coverage: tools-only fallback when no channel caps exist; explicit `--dir` bypassing fallback; neither-file-exists error path; the error-message format (which currently has bug #1).
- Doc comment on `combine_provider_scopes` still says "Scan the tools directory".
🤖 Generated with Claude Code
| "Tool '{}' not found or has no capabilities file at {} (or {} for WASM channels)", | ||
| name, | ||
| caps_path.display() | ||
| caps_path.display(), |
There was a problem hiding this comment.
High: combine_provider_scopes(&tools_dir, ...) is called with tools_dir = dir.clone().unwrap_or_else(default_tools_dir) — still ~/.ironclaw/tools by default. But when the capabilities file was resolved from ~/.ironclaw/channels/ via the new fallback, this scan goes against the wrong directory. The OAuth-scope consolidation silently misses sibling channel caps that share the same secret_name, so a user running ironclaw tool auth telegram on a channel-resolved caps file still gets a tools-dir-only scope merge. Either switch the scan to caps_path.parent() (the directory the caps actually came from), or scan both dirs. A caller-level test on auth_tool — per .claude/rules/testing.md ("Test Through the Caller, Not Just the Helper") — would have caught this: the helper is tested but its sole non-trivial downstream consumer is not.
| default_channels_dir() | ||
| .join(format!("{}.capabilities.json", name)) | ||
| .display() | ||
| ); |
There was a problem hiding this comment.
Medium: Clap help text drift. The --dir flags on Auth and Setup still say "Directory to look for tool (default: ~/.ironclaw/tools/)" (lines 94-96 and 108-110, unchanged by this PR). After this change the actual default precedence is channels-first, tools-fallback. Please update the help strings so ironclaw tool auth --help / ironclaw tool setup --help reflect the new behavior (e.g. "default: ~/.ironclaw/channels/, then ~/.ironclaw/tools/"). Same drift applies to the Setup variant.
|
Architectural / consistency note: This PR fixes
The result is a partially-unified surface: users can Also, per |
Fixes #1384.
now falls back to when no tool-scoped capabilities file exists, so registry-installed WASM channels can be authenticated without a symlink workaround.
Validation: