Skip to content

fix(cli): fall back to channel capabilities for auth#1496

Open
G7CNF wants to merge 2 commits intonearai:stagingfrom
G7CNF:codex/issue-1384-telegram-capabilities
Open

fix(cli): fall back to channel capabilities for auth#1496
G7CNF wants to merge 2 commits intonearai:stagingfrom
G7CNF:codex/issue-1384-telegram-capabilities

Conversation

@G7CNF
Copy link
Copy Markdown
Contributor

@G7CNF G7CNF commented Mar 20, 2026

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:

  • cargo fmt --all -- --check
  • cargo clippy -p ironclaw --lib --all-features -- -D warnings
  • cargo test -p ironclaw test_resolve_capabilities_path_falls_back_to_channels_dir -- --nocapture

@github-actions github-actions bot added scope: channel/cli TUI / CLI channel size: M 50-199 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels Mar 20, 2026
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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 ironclaw CLI's tool authentication and setup process by introducing a robust capability file resolution strategy. Previously, ironclaw exclusively looked for capability files in the tools directory, which caused issues for WASM channels installed via the registry, as their capability files reside in the channels directory. The changes enable the CLI to automatically fall back to the channels directory, streamlining the authentication workflow and eliminating the need for manual workarounds for these types of tools.

Highlights

  • Tool Authentication: Implemented a fallback mechanism for ironclaw tool auth to locate capability files in the channels directory if they are not found in the default tools directory. This resolves authentication issues for registry-installed WASM channels.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/cli/tool.rs Outdated
Copy link
Copy Markdown
Member

@ilblackdragon ilblackdragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(cli): fall back to channel capabilities for auth

Must-fix

  1. Duplicate path in error message — when dir is None, resolve_capabilities_path returns ~/.ironclaw/channels/<name>.capabilities.json as 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.

  1. combine_provider_scopes only scans tools directory — in auth_tool, tools_dir is 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

  1. Stale --dir help textAuth and Setup subcommand definitions still say "default: ~/.ironclaw/tools/" but effective default now prefers ~/.ironclaw/channels/.

  2. Fourth copy of default_channels_dir() — already defined in wasm/loader.rs (pub), config/channels.rs, cli/status.rs. Import the public one from crate::channels::wasm::loader.

  3. Tests should use tempfile crate — per project testing rules, use tempfile::tempdir() instead of std::env::temp_dir() with UUID suffixes.

  4. Missing test cases — no test for tools-only fallback (no channel file), no test for explicit --dir bypass, no test for neither-file-exists case.

  5. assert!(x.is_ok()) discards errors — use .expect() in test setup code to preserve error details on failure.

Copy link
Copy Markdown
Member

@ilblackdragon ilblackdragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/cli/tool.rs
"Tool '{}' not found or has no capabilities file at {} (or {} for WASM channels)",
name,
caps_path.display()
caps_path.display(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/cli/tool.rs
default_channels_dir()
.join(format!("{}.capabilities.json", name))
.display()
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilblackdragon
Copy link
Copy Markdown
Member

Architectural / consistency note:

This PR fixes auth and setup to look in ~/.ironclaw/channels/, but the rest of the ironclaw tool subcommands remain tools-only:

  • tool info <name> (lines 400-410 in the PR) still resolves wasm_path and its sibling .capabilities.json only under the tools dir, so you cannot inspect a channel's caps with ironclaw tool info telegram.
  • tool list, tool remove, tool install similarly operate only on the tools dir.

The result is a partially-unified surface: users can auth a channel but cannot info it through the same command. Consider either (a) extending the resolver to info/list/remove in a follow-up, or (b) documenting clearly in the PR body that only auth/setup are channel-aware.

Also, per .claude/rules/testing.md ("Test Through the Caller, Not Just the Helper"): resolve_capabilities_path_in_base is a transform whose output gates OAuth flows and secret writes. The tests cover the helper in isolation but do not drive auth_tool or setup_tool end-to-end. A caller-level integration test (e.g. a temp-dir auth_tool test with a channels-dir caps file and stubbed OAuth) would have surfaced the combine_provider_scopes / tools_dir mismatch flagged inline — that's exactly the class of wrapper-silently-drops-an-input bug the rule was added to prevent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules scope: channel/cli TUI / CLI channel size: M 50-199 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't configure telegram channel because ironclaw tool auth looks for capabilities elsewhere

2 participants