fix: improve local WASM fallback installs and soften Ollama model listing#1167
fix: improve local WASM fallback installs and soften Ollama model listing#1167JZKK720 wants to merge 11 commits intonearai:stagingfrom
Conversation
- Add host.docker.internal patterns for Database (PostgreSQL), Ollama, and LM Studio - Improve configuration hints for users running local LLM providers on host machine while using Docker containers - Benefit: Cross-platform guide for accessing host services from Docker without code changes - No functional changes, documentation/configuration only
github-actions
bot
added
scope: llm
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 primarily enhances the robustness of local development and extension management. It addresses issues with WASM extension installations, making them more resilient to missing artifacts and flexible in handling different naming conventions within tarball bundles. Additionally, it improves the reliability of Ollama model discovery by implementing a fallback mechanism, and provides clearer Docker networking instructions for local model providers. These changes collectively aim to streamline the development workflow and prevent common setup pitfalls. 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 several valuable fixes and improvements. The softening of Ollama model discovery with a fallback mechanism is well-implemented and robust. The changes to support crate-named WASM bundles and improve local installation fallbacks are also solid additions that enhance the developer experience. The documentation updates for Docker networking are clear and helpful.
My review includes a couple of suggestions to refactor the duplicated archive_filename_candidates helper function in both src/extensions/manager.rs and src/registry/installer.rs. Using a HashSet instead of a Vec for deduplication would be more idiomatic and slightly more efficient. Overall, this is a great set of changes.
| fn archive_filename_candidates( | ||
| extension_name: &str, | ||
| archive_crate_name: Option<&str>, | ||
| suffix: &str, | ||
| ) -> Vec<String> { | ||
| let mut candidates = Vec::new(); | ||
|
|
||
| for base in [Some(extension_name), archive_crate_name] { | ||
| if let Some(base) = base { | ||
| let raw = format!("{}{}", base, suffix); | ||
| if !candidates.contains(&raw) { | ||
| candidates.push(raw); | ||
| } | ||
|
|
||
| let snake = format!("{}{}", base.replace('-', "_"), suffix); | ||
| if !candidates.contains(&snake) { | ||
| candidates.push(snake); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| candidates | ||
| } |
There was a problem hiding this comment.
The implementation of archive_filename_candidates can be made more efficient and idiomatic. Using a Vec with repeated contains() checks for deduplication is less performant than using a HashSet. For a small number of items, the performance difference is negligible, but using a HashSet is cleaner and considered a best practice for managing unique collections.
fn archive_filename_candidates(
extension_name: &str,
archive_crate_name: Option<&str>,
suffix: &str,
) -> Vec<String> {
use std::collections::HashSet;
let mut candidates: HashSet<String> = HashSet::new();
for base in [Some(extension_name), archive_crate_name].into_iter().flatten() {
candidates.insert(format!("{}{}", base, suffix));
candidates.insert(format!("{}{}", base.replace('-', "_"), suffix));
}
candidates.into_iter().collect()
}| fn archive_filename_candidates( | ||
| extension_name: &str, | ||
| archive_crate_name: Option<&str>, | ||
| suffix: &str, | ||
| ) -> Vec<String> { | ||
| let mut candidates = Vec::new(); | ||
|
|
||
| for base in [Some(extension_name), archive_crate_name] { | ||
| if let Some(base) = base { | ||
| let raw = format!("{}{}", base, suffix); | ||
| if !candidates.contains(&raw) { | ||
| candidates.push(raw); | ||
| } | ||
|
|
||
| let snake = format!("{}{}", base.replace('-', "_"), suffix); | ||
| if !candidates.contains(&snake) { | ||
| candidates.push(snake); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| candidates | ||
| } |
There was a problem hiding this comment.
This function is a duplicate of archive_filename_candidates in src/extensions/manager.rs and has the same opportunity for improvement. Using a Vec with repeated contains() checks for deduplication can be replaced with a more efficient and idiomatic HashSet.
Additionally, since this logic is now duplicated in two modules, it might be worth considering extracting it into a shared utility function in a follow-up PR to improve maintainability.
fn archive_filename_candidates(
extension_name: &str,
archive_crate_name: Option<&str>,
suffix: &str,
) -> Vec<String> {
use std::collections::HashSet;
let mut candidates: HashSet<String> = HashSet::new();
for base in [Some(extension_name), archive_crate_name].into_iter().flatten() {
candidates.insert(format!("{}{}", base, suffix));
candidates.insert(format!("{}{}", base.replace('-', "_"), suffix));
}
candidates.into_iter().collect()
}There was a problem hiding this comment.
Pull request overview
This PR improves local development ergonomics for WASM extension/channel installs (especially when release artifacts are missing) and makes Ollama model discovery more resilient by falling back to the configured model when tag listing fails.
Changes:
- Expand tar.gz extraction to accept crate-named (and snake_case) bundle entries for both registry installs and runtime extension installs.
- Add Ollama
/api/tagsmodel listing with best-effort fallback behavior in the rig-based LLM adapter, wiring it up for the Ollama provider. - Update local Docker/dev guidance and configs (compose app service, env example hints) and align the WhatsApp channel lockfile version.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/registry/installer.rs |
Accept multiple filename candidates when extracting tar.gz artifacts; adds tests for crate-named bundles. |
src/extensions/manager.rs |
Pass preferred crate name into tar extraction; accept crate-named bundle entries; adds test coverage. |
src/llm/rig_adapter.rs |
Implement list_models() for Ollama with fallback-to-configured-model behavior; add test. |
src/llm/mod.rs |
Wire Ollama base URL into RigAdapter so list_models() can query /api/tags. |
docker-compose.yml |
Add app service and change Postgres port publishing behavior. |
channels-src/whatsapp/Cargo.lock |
Update lockfile package version to match crate version. |
.env.example |
Add Docker networking guidance for host-based local model providers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| image: pgvector/pgvector:pg16 | ||
| ports: | ||
| - "127.0.0.1:5432:5432" | ||
| - "5432:5432" |
| # For Docker: use host.docker.internal instead of localhost to access services on the host machine | ||
| DATABASE_URL=postgres://localhost/ironclaw |
| let mut candidates = Vec::new(); | ||
|
|
||
| for base in [Some(extension_name), archive_crate_name] { | ||
| if let Some(base) = base { | ||
| let raw = format!("{}{}", base, suffix); | ||
| if !candidates.contains(&raw) { | ||
| candidates.push(raw); | ||
| } | ||
|
|
||
| let snake = format!("{}{}", base.replace('-', "_"), suffix); | ||
| if !candidates.contains(&snake) { | ||
| candidates.push(snake); | ||
| } | ||
| } | ||
| } | ||
|
|
|
Follow-up after the rustfmt-only push (
If maintainers want an immediate unblock for the regression policy specifically, the workflow itself suggests the |
There was a problem hiding this comment.
Pull request overview
This PR improves local development and installation resilience by expanding WASM bundle extraction name matching, adding a Docker “app” service to support local source-build fallbacks, and making Ollama model discovery best-effort (falling back to the configured model when listing fails).
Changes:
- Extend tar.gz extraction to accept multiple archive entry naming conventions (extension-id, crate name, snake_case) for both WASM and capabilities files.
- Add an Ollama-aware
list_models()implementation that falls back to the configured model on request/parse failures. - Update local Docker/dev configuration (compose +
.env.example) and align WhatsApp channel lockfile version.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/registry/installer.rs |
Expands tar.gz extraction to accept crate-named/variant bundle entries; adds test coverage. |
src/extensions/manager.rs |
Mirrors bundle entry candidate logic for gateway installs; passes through preferred crate name; adds tests. |
src/llm/rig_adapter.rs |
Adds Ollama model listing via /api/tags with fallback behavior and a regression test. |
src/llm/mod.rs |
Wires Ollama base URL into RigAdapter for model listing. |
docker-compose.yml |
Publishes Postgres broadly and adds an app service with mounted sources for local installs. |
channels-src/telegram/src/lib.rs |
Refines slash-command emission behavior for Submission parsing and /start payload handling; updates tests. |
channels-src/whatsapp/Cargo.lock |
Aligns lockfile package version with crate version. |
.env.example |
Adds Docker networking guidance for host-based local model providers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| image: pgvector/pgvector:pg16 | ||
| ports: | ||
| - "127.0.0.1:5432:5432" | ||
| - "5432:5432" |
| - ./extensions/ironclaw-home:/home/ironclaw/.ironclaw | ||
| - ./tools-src:/app/tools-src:ro | ||
| - ./channels-src:/app/channels-src:ro |
| # For Docker: use host.docker.internal instead of localhost to access services on the host machine | ||
| DATABASE_URL=postgres://localhost/ironclaw |
| tracing::warn!( | ||
| model = %self.model_name, | ||
| base_url, | ||
| %status, | ||
| body, | ||
| "Ollama /api/tags returned an error; falling back to configured model" | ||
| ); |
| let expected = format!("/start@{}", bot); | ||
| command.eq_ignore_ascii_case(&expected) | ||
| } else { | ||
| command.len() > "/start@".len() && command[..7].eq_ignore_ascii_case("/start@") |
github-actions
bot
added
scope: channel/wasm
There was a problem hiding this comment.
Pull request overview
This PR improves local development and install robustness by enhancing WASM bundle extraction/fallback behavior, adding a safer Ollama model-listing fallback, and extending Docker/dev configuration to better support host-based services and channel runtime overrides.
Changes:
- Make WASM tarball extraction accept multiple filename candidates (extension id, crate name, snake_case variants) for both registry installs and URL installs.
- Add Ollama
list_models()best-effort behavior that falls back to the configured model when/api/tagsis unavailable or malformed. - Add local dev ergonomics: Docker compose
appservice + networking guidance, Telegram polling override config, and bundled capabilities fallback when an installed channel is missing its sidecar.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/registry/installer.rs |
Expands tar.gz extraction to match multiple wasm/capabilities entry names; adds tests for crate-named entries. |
src/extensions/manager.rs |
Updates URL installer extraction logic to support crate-named tarball entries; threads crate-name hint through install path; adds tests. |
src/llm/rig_adapter.rs |
Adds optional Ollama base URL and implements list_models() with HTTP fetch + fallback behavior and a regression test. |
src/llm/mod.rs |
Wires Ollama provider creation to set the base URL used for model listing. |
src/config/channels.rs |
Introduces TELEGRAM_POLLING_ENABLED config flag and tests. |
src/config/mod.rs |
Extends Config::for_testing() to populate the new channels config field. |
src/channels/wasm/setup.rs |
Injects polling_enabled into the Telegram WASM channel config when the override is enabled. |
src/channels/wasm/loader.rs |
Adds capability-sidecar resolution fallback to bundled capabilities when installed sidecar is missing; adds tests. |
src/channels/wasm/bundled.rs |
Exposes a helper to locate bundled channel capabilities sidecar; adds a test. |
channels-src/telegram/src/lib.rs |
Refines Telegram command parsing/emission logic, especially for /start and slash commands with arguments; updates tests. |
docker-compose.yml |
Adds an app service for local Docker dev; adjusts Postgres port publishing. |
.env.example |
Adds Docker networking guidance and documents TELEGRAM_POLLING_ENABLED. |
channels-src/whatsapp/Cargo.lock |
Aligns the lockfile package version with the crate version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| let previous = std::env::var_os("IRONCLAW_CHANNELS_SRC"); | ||
| unsafe { | ||
| std::env::set_var("IRONCLAW_CHANNELS_SRC", bundled_root.path()); | ||
| } | ||
|
|
||
| let resolved = super::resolve_capabilities_path("telegram", &installed_wasm); | ||
|
|
||
| match previous { | ||
| Some(value) => unsafe { | ||
| std::env::set_var("IRONCLAW_CHANNELS_SRC", value); | ||
| }, | ||
| None => unsafe { | ||
| std::env::remove_var("IRONCLAW_CHANNELS_SRC"); | ||
| }, | ||
| } |
| fn archive_filename_candidates( | ||
| extension_name: &str, | ||
| archive_crate_name: Option<&str>, | ||
| suffix: &str, | ||
| ) -> Vec<String> { | ||
| let mut candidates = Vec::new(); | ||
|
|
||
| for base in [Some(extension_name), archive_crate_name] { | ||
| if let Some(base) = base { | ||
| let raw = format!("{}{}", base, suffix); | ||
| if !candidates.contains(&raw) { | ||
| candidates.push(raw); | ||
| } | ||
|
|
||
| let snake = format!("{}{}", base.replace('-', "_"), suffix); | ||
| if !candidates.contains(&snake) { | ||
| candidates.push(snake); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| candidates | ||
| } |
| image: pgvector/pgvector:pg16 | ||
| ports: | ||
| - "127.0.0.1:5432:5432" | ||
| - "5432:5432" |
| retries: 5 | ||
|
|
||
| app: | ||
| image: ironclaw:latest |
| let body = response.text().await.unwrap_or_default(); | ||
| tracing::warn!( | ||
| model = %self.model_name, | ||
| base_url, | ||
| %status, | ||
| body, |
|
Prepared a fix branch for this PR’s failing checks, since I can’t push to Branch: Commits to cherry-pick onto
What these address:
Local verification on that branch:
|
|
Prepared a fix branch for this PR’s failing checks, since I can’t push to directly. Branch: Commits to cherry-pick onto :
What these address:
Local verification on that branch:
|
There was a problem hiding this comment.
Pull request overview
This PR improves local development and install robustness for WASM extensions/channels, and makes Ollama model discovery best-effort to avoid failing callers when model listing is unavailable.
Changes:
- Extend tar.gz extraction to accept multiple filename candidates (extension name, crate name, snake_case) for
.wasmand capabilities sidecars. - Add Ollama model listing via
/api/tagswith fallback to the configured model when the endpoint fails or returns unusable data. - Add Telegram channel runtime overrides/config and loader-side capabilities fallbacks; update local Docker/dev guidance and introduce a CI script to detect panic-style calls in production Rust diffs.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/registry/installer.rs |
Accept crate-/snake-named entries when extracting registry tar.gz bundles; adds tests. |
src/extensions/manager.rs |
Extend URL install tar.gz extraction to accept crate-/snake-named bundle entries; adds tests/helpers. |
src/llm/rig_adapter.rs |
Add optional Ollama base URL and implement list_models() with fallback behavior + test. |
src/llm/mod.rs |
Wire Ollama base URL into the RigAdapter so listing is enabled for Ollama providers. |
src/config/mod.rs |
Add telegram_polling_enabled default in test config. |
src/config/channels.rs |
Add TELEGRAM_POLLING_ENABLED env/config support + unit test coverage. |
src/channels/wasm/setup.rs |
Inject Telegram polling_enabled runtime config override into the WASM channel. |
src/channels/wasm/loader.rs |
Fall back to bundled capabilities sidecar when an installed channel is missing its local sidecar; adds tests. |
src/channels/wasm/bundled.rs |
Add helper to locate bundled channel capabilities sidecar; adds test. |
channels-src/telegram/src/lib.rs |
Adjust emitted agent content handling for slash commands and /start@bot forms; expands tests. |
channels-src/whatsapp/Cargo.lock |
Align WhatsApp channel lockfile crate version. |
docker-compose.yml |
Add an app service and update Postgres port binding for local Docker stack. |
.env.example |
Add Docker networking guidance (host access via host.docker.internal) and Telegram polling flag. |
scripts/check_no_panics.py |
New diff-aware checker used by CI to block new unwrap/expect/assert in production Rust code. |
.github/workflows/code_style.yml |
Switch “no panics” job to use scripts/check_no_panics.py. |
Cargo.lock |
Update workspace lockfile dependency resolutions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| let expected = format!("/start@{}", bot); | ||
| command.eq_ignore_ascii_case(&expected) | ||
| } else { | ||
| command.len() > "/start@".len() && command[..7].eq_ignore_ascii_case("/start@") |
| tracing::warn!( | ||
| model = %self.model_name, | ||
| base_url, | ||
| %status, | ||
| body, |
| image: pgvector/pgvector:pg16 | ||
| ports: | ||
| - "127.0.0.1:5432:5432" | ||
| - "5432:5432" |
| # Database Configuration | ||
| # For Docker: use host.docker.internal instead of localhost to access services on the host machine | ||
| DATABASE_URL=postgres://localhost/ironclaw |
| self.assertFalse(contexts[5]) | ||
| self.assertFalse(contexts[6]) | ||
|
|
||
| def test_named_tests_module_without_cfg_is_ignored(self) -> None: |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: Local WASM fallback installs and Ollama model listing
The WASM archive filename resolution logic is useful (handling crate_name vs extension_name mismatches), but there's a security issue and scope concerns.
Blocking
1. Security: PostgreSQL exposed to all interfaces
In `docker-compose.yml`, the postgres port binding changed from:
```yaml
- "127.0.0.1:5432:5432"
```
to:
```yaml - "5432:5432"
```
This exposes the database to all network interfaces instead of localhost-only. On any machine with a public IP, this makes the database accessible from the internet with default credentials (`ironclaw:ironclaw`). Please revert to the localhost binding.
2. Scope too broad
This PR bundles 4+ distinct changes across 10 commits:
- Docker compose app service addition
- .env.example Docker networking docs
- WASM archive filename candidate logic
- Ollama model listing changes
- Registry installer modifications
These should be separate PRs for reviewability. At minimum, the docker-compose security change should not be bundled with WASM install logic.
Non-blocking
- The `archive_filename_candidates()` function is well-designed -- handles hyphenated vs snake_case name mismatches
- The `preferred_archive_crate_name()` fallback chain (source -> fallback_source) is a reasonable approach
Title check: partially accurate -- title mentions WASM fallback and Ollama but doesn't mention docker-compose changes or .env.example docs.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: REQUEST CHANGES
Previous blocking feedback remains unresolved (no commits since March 14th).
Still blocking from prior review
1. PostgreSQL exposed on all interfaces
docker-compose.yml still has "5432:5432" with default credentials (ironclaw:ironclaw). Must revert to "127.0.0.1:5432:5432".
2. Scope too broad
PR bundles 6+ distinct changes: WASM archive resolution, Ollama model listing, Telegram slash command parsing, Docker compose app service, CI script replacement, bundled channel capabilities fallback, dependency update.
New issues found
3. Duplicated archive_filename_candidates() -- identical function in both extensions/manager.rs and registry/installer.rs. Flagged by multiple reviewers across multiple rounds, never addressed. Extract to a shared module.
4. Untrusted response body logged in full -- list_models() reads entire Ollama response with .unwrap_or_default() and logs at warn level. Should truncate before logging (use truncate_for_preview or similar).
5. is_start_command_token() with None bot_username accepts /start@AnyBot in group chats -- could react to commands intended for other bots. Should only match bare /start when bot_username is None.
6. Test uses unsafe { std::env::set_var } without synchronization -- resolve_capabilities_path_falls_back_to_bundled_sidecar in loader.rs can race with parallel tests.
What's good
archive_filename_candidates()design is sound -- handles the hyphen/underscore mismatch correctly- Ollama
list_models()fallback strategy is well-structured with appropriate logging - Good test coverage for archive extraction, Ollama fallback, and Telegram commands
check_no_panics.pyis a meaningful improvement over inline shell
Recommendation
Split into at minimum 3 PRs:
- WASM archive resolution + Ollama model listing
- Telegram channel improvements
- Docker/CI infrastructure
CI is green. The individual changes are mostly good -- they just need to be separated and the blockers addressed.
Summary
telegram_tool.wasmand similar variants.list_models()falls back to the configured model when/api/tagsis unavailable or malformed instead of failing callers.Change Type
Linked Issue
None
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-featurescargo test list_models_falls_back_to_configured_model_on_request_failure --libweb-search,telegram-mtproto,whatsapp, anddiscordSecurity Impact
Low. This change does not expand secret scope or sandbox permissions. It keeps extension fallback local to mounted source/build directories and changes Ollama model listing to best-effort behavior against the already configured Ollama base URL.
Database Impact
None
Blast Radius
Touches the local Docker development path, gateway extension install flow, registry tarball extraction, and Ollama-backed model listing. If this regresses, likely symptoms are failed local extension installs, missing capability extraction from bundles, or incomplete model lists for Ollama-backed providers.
Rollback Plan
Revert the branch commits that introduced this behavior:
b57f62b(fix: soften Ollama model listing fallback)2fa27a6(Fix local WASM extension fallback installs)2e3ca8d(docs: update .env.example with Docker networking guidance)If rollback is needed quickly, reverting those three commits restores the pre-change runtime and docs behavior without requiring schema or data rollback.
Review track: C