fix(ci): disambiguate WASM bundle filenames to prevent tool/channel collision#964
fix(ci): disambiguate WASM bundle filenames to prevent tool/channel collision#964
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 addresses a critical CI build issue where WASM bundle filenames would collide when a tool and a channel shared the same name, leading to incorrect artifact URLs and SHA256 checksums in registry manifests. The solution involves introducing a kind-prefix to WASM bundle filenames and updating the CI scripts to correctly parse these prefixes, ensuring unique and accurate artifact linking. Additionally, unnecessary WIT-versioned hosting changes were reverted from the installer. Highlights
Changelog
Ignored Files
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
|
There was a problem hiding this comment.
Pull request overview
Updates the release CI pipeline to avoid WASM bundle filename collisions between tools and channels that share the same registry filename (e.g. slack, telegram) by introducing a kind- prefix, and adds a small regression test script to validate the new parsing behavior.
Changes:
- Prefix WASM bundle filenames with
kind(tool-...vschannel-...) and update manifest patching logic accordingly in the release workflow. - Add
scripts/test-ci-artifact-naming.shto validate kind-prefixed filename parsing into the correct registry manifest path. - Bump crate version/changelog to
0.18.0and adjust acfg(unix)import insrc/main.rs.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/main.rs |
Moves ChannelSecretUpdater import into the #[cfg(unix)] block to avoid non-unix import issues. |
scripts/test-ci-artifact-naming.sh |
Adds a parsing regression test for kind-prefixed artifact filenames. |
Cargo.toml |
Bumps package version to 0.18.0. |
Cargo.lock |
Updates lockfile to reflect 0.18.0. |
CHANGELOG.md |
Adds a 0.18.0 release section. |
.github/workflows/release.yml |
Implements kind-prefixed bundling and kind-aware registry manifest patching to prevent collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/test-ci-artifact-naming.sh
Outdated
| @@ -0,0 +1,58 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This script uses #!/bin/bash, but the other repo scripts consistently use #!/usr/bin/env bash for portability (e.g. scripts/build-all.sh, scripts/build-wasm-extensions.sh). Align the shebang here to match the project convention.
| #!/bin/bash | |
| #!/usr/bin/env bash |
There was a problem hiding this comment.
Fixed in latest push — updated to #\!/usr/bin/env bash.
| set -euo pipefail | ||
|
|
||
| PASS=0 | ||
| FAIL=0 | ||
|
|
||
| assert_parse() { | ||
| local filename="$1" expected_kind="$2" expected_name="$3" | ||
| local kind name manifest | ||
|
|
||
| kind=$(echo "$filename" | cut -d'-' -f1) | ||
| name=$(echo "$filename" | sed "s/^${kind}-//" | sed 's/-[0-9].*-wasm32-wasip2\.tar\.gz$//') | ||
| manifest="registry/${kind}s/${name}.json" |
There was a problem hiding this comment.
Most scripts cd to the repo root early (so they work when invoked from any working directory), but this script relies on relative paths like registry/... without changing directories. Consider adding a cd "$(git rev-parse --show-toplevel)" (or cd "$(dirname "$0")/..") near the top so it runs reliably.
There was a problem hiding this comment.
Fixed in latest push — added cd "$(dirname "$0")/.." at the top.
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the WASM bundle filename collision by prefixing them with their 'kind' (tool or channel). The changes to the CI scripts and the addition of a dedicated test script are well-thought-out. The refactoring in src/main.rs to localize a use statement is also a good improvement for code clarity. I have one suggestion to make the new test script's parsing logic even more robust.
| local kind name manifest | ||
|
|
||
| kind=$(echo "$filename" | cut -d'-' -f1) | ||
| name=$(echo "$filename" | sed "s/^${kind}-//" | sed 's/-[0-9].*-wasm32-wasip2\.tar\.gz$//') |
There was a problem hiding this comment.
The current parsing logic for the extension name is a bit fragile. It uses a greedy sed pattern that could fail if an extension name contains a hyphen followed by a digit (e.g., my-tool-v2).
A more robust approach would be to use a single, more specific sed command with a capture group that explicitly matches a semantic version number. This would make the parsing logic less ambiguous and prevent potential mis-parsing of valid extension names.
| name=$(echo "$filename" | sed "s/^${kind}-//" | sed 's/-[0-9].*-wasm32-wasip2\.tar\.gz$//') | |
| name=$(echo "$filename" | sed -E 's/^'"$kind"'-(.+)-[0-9]+\.[0-9]+\.[0-9]+.*-wasm32-wasip2\.tar\.gz$/\1/') | |
There was a problem hiding this comment.
Intentionally not changing — this test script mirrors release.yml's parsing logic exactly (same sed pattern). Changing only the test would create divergence and false confidence. If the parsing ever needs to become more robust, both release.yml and this test should be updated together.
…ollision When a tool and channel share the same name (e.g. slack, telegram), the CI build produced identical bundle filenames, causing the second to overwrite the first. Both manifests then pointed to the wrong binary. Prefix bundle filenames with the extension kind (tool-slack-... vs channel-slack-...) and parse the prefix when patching manifests, so each manifest receives the correct artifact URL and SHA256. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ee8e7fd to
873698a
Compare
Regression tests for the CI artifact collision fix (PR #964). Verifies: - extract_tar_gz rejects archives with wrong wasm name (the collision bug) - Tool bundle extracts slack-tool.wasm correctly - Channel bundle extracts slack.wasm correctly - Tool and channel manifests install to separate directories Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| kind=$(echo "$filename" | cut -d'-' -f1) | ||
| name=$(echo "$filename" | sed "s/^${kind}-//" | sed 's/-[0-9].*-wasm32-wasip2\.tar\.gz$//') | ||
| url="https://github.com/nearai/ironclaw/releases/download/${RELEASE_TAG}/${filename}" | ||
|
|
||
| for manifest in registry/tools/${name}.json registry/channels/${name}.json; do | ||
| if [ -f "$manifest" ]; then | ||
| jq --arg sha "$sha256" --arg url "$url" \ | ||
| '.artifacts["wasm32-wasip2"].sha256 = $sha | .artifacts["wasm32-wasip2"].url = $url' \ | ||
| "$manifest" > "${manifest}.tmp" && mv "${manifest}.tmp" "$manifest" | ||
| echo "Patched $manifest with sha256=$sha256 url=$url" | ||
| fi | ||
| done | ||
| manifest="registry/${kind}s/${name}.json" | ||
| if [ -f "$manifest" ]; then |
There was a problem hiding this comment.
The name extraction uses sed 's/-[0-9].*-wasm32-wasip2\.tar\.gz$//', which will strip too early if the extension file_stem contains a hyphen followed by a digit (e.g. tool-google-2fa-0.2.0-... would parse as google instead of google-2fa). Consider parsing by first removing the fixed suffix, then removing the version starting at the last -[0-9] segment (e.g. via bash parameter expansion or a regex anchored to the last match) so hyphen+digit in names remains supported.
There was a problem hiding this comment.
Valid theoretical concern, but no current or planned extension names contain -digit patterns that aren't version numbers. The sed pattern is intentionally identical across build-local-artifacts, update-registry-checksums, and the test script — they must stay in sync. If we ever add an extension like google-2fa, we'd need to update the parsing everywhere (likely switching to a more structured approach). Tracked but not blocking.
| manifest="registry/${kind}s/${name}.json" | ||
| if [ -f "$manifest" ]; then | ||
| jq --arg sha "$sha256" --arg url "$url" \ |
There was a problem hiding this comment.
kind is taken directly from the checksum filename and then interpolated into registry/${kind}s/.... If kind is empty/unexpected, this silently skips patching and can produce incorrect artifact URLs/SHA256s for a release. Consider validating kind is exactly tool or channel (and failing the job or emitting an explicit error) before using it to build paths.
There was a problem hiding this comment.
Good catch — will add validation that kind is tool or channel before using it in paths.
| file_stem=$(basename "$manifest" .json) | ||
| # kind: "tool" or "channel" — used as bundle filename prefix to avoid | ||
| # collisions when a tool and channel share the same file_stem (e.g. slack). | ||
| kind=$(jq -r '.kind' "$manifest") |
There was a problem hiding this comment.
kind=$(jq -r '.kind' ...) is used as a bundle filename prefix. If .kind is missing/typoed, this will generate bundles like null-... and later checksum patching won’t update the intended manifest. Consider checking kind is tool or channel (and erroring out) before continuing for each manifest.
| kind=$(jq -r '.kind' "$manifest") | |
| kind=$(jq -r '.kind' "$manifest") | |
| if [ "$kind" != "tool" ] && [ "$kind" != "channel" ]; then | |
| echo "::error::Manifest '$manifest' has invalid or missing .kind ('$kind'); expected 'tool' or 'channel'" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Good catch — will add this validation. If .kind is missing or invalid, we should fail loudly rather than produce a broken bundle.
| # Parse kind-prefixed filename: "tool-slack-0.2.1-wasm32-wasip2.tar.gz" | ||
| # → kind=tool, name=slack | ||
| kind=$(echo "$filename" | cut -d'-' -f1) | ||
| name=$(echo "$filename" | sed "s/^${kind}-//" | sed 's/-[0-9].*-wasm32-wasip2\.tar\.gz$//') |
There was a problem hiding this comment.
Same parsing issue as earlier in the workflow: name=$(... | sed 's/-[0-9].*-wasm32-wasip2\.tar\.gz$//') can drop parts of names that contain -<digit> (e.g. google-2fa). Consider using a parsing method that removes the version starting at the last -[0-9] occurrence after stripping the fixed suffix, so filenames remain reversible for all valid manifest names.
| name=$(echo "$filename" | sed "s/^${kind}-//" | sed 's/-[0-9].*-wasm32-wasip2\.tar\.gz$//') | |
| base="${filename#${kind}-}" | |
| base="${base%-wasm32-wasip2.tar.gz}" | |
| name="${base%-[0-9]*}" |
There was a problem hiding this comment.
Same as the earlier comment — the sed pattern is intentionally kept in sync across all three locations. No current extension names trigger this edge case.
- Validate .kind is "tool" or "channel" before using in build-wasm-extensions (hard error) - Filter checksums.txt to *-wasm32-wasip2.tar.gz entries before parsing, avoiding noisy warnings from binary artifact entries in build-local-artifacts - Add kind validation with warning+skip in both checksum-parsing loops Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ollision (nearai#964) * fix(ci): disambiguate WASM bundle filenames to prevent tool/channel collision When a tool and channel share the same name (e.g. slack, telegram), the CI build produced identical bundle filenames, causing the second to overwrite the first. Both manifests then pointed to the wrong binary. Prefix bundle filenames with the extension kind (tool-slack-... vs channel-slack-...) and parse the prefix when patching manifests, so each manifest receives the correct artifact URL and SHA256. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(registry): add installer tests for tool/channel name disambiguation Regression tests for the CI artifact collision fix (PR nearai#964). Verifies: - extract_tar_gz rejects archives with wrong wasm name (the collision bug) - Tool bundle extracts slack-tool.wasm correctly - Channel bundle extracts slack.wasm correctly - Tool and channel manifests install to separate directories Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ci): add kind validation and filter non-WASM checksum entries - Validate .kind is "tool" or "channel" before using in build-wasm-extensions (hard error) - Filter checksums.txt to *-wasm32-wasip2.tar.gz entries before parsing, avoiding noisy warnings from binary artifact entries in build-local-artifacts - Add kind validation with warning+skip in both checksum-parsing loops Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt formatting in installer tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
tool-slack-...vschannel-slack-...) in the CI build to prevent collisions when a tool and channel share the same name (e.g. slack, telegram).kindfield istoolorchannelbefore using in paths/filenameschecksums.txtbefore parsing to avoid noisy warningsRoot Cause
In
build-wasm-extensions, the bundleslack-0.2.1-wasm32-wasip2.tar.gzwas produced twice (once for the tool, once for the channel) — the second overwrote the first intarget/wasm-bundles/. Thenbuild-local-artifactsandupdate-registry-checksumspatched bothregistry/tools/slack.jsonandregistry/channels/slack.jsonwith the surviving artifact's URL + SHA256, causing the tool installer to fail with "tar.gz archive does not contain 'slack-tool.wasm'".Changes
build-wasm-extensions: Extractkindfrom manifest JSON, validate it, prefix bundle filename with kindbuild-local-artifacts: Filter to WASM entries, parse kind prefix (cut -d'-' -f1), validate kind, target correct manifestupdate-registry-checksums: Same filtering + kind-prefix parsing fixinstaller.rs: Add#[derive(Debug)]toExtractResult, add 4 regression tests for tool/channel name disambiguationscripts/test-ci-artifact-naming.sh: 15 test cases verifying kind-prefixed filename parsingTest plan
cargo fmt— cleancargo clippy --all --benches --tests --examples --all-features— zero warningscargo test— all 18 installer tests pass (14 existing + 4 new)scripts/test-ci-artifact-naming.sh— 15/15 passtool-slack-*andchannel-slack-*artifacts🤖 Generated with Claude Code