-
Notifications
You must be signed in to change notification settings - Fork 21
Replace wasm-pack #690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Replace wasm-pack #690
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| image: clickhouse/clickhouse-server:24 | ||
| env: | ||
| "CLICKHOUSE_USER": "clickhouse" | ||
| "CLICKHOUSE_PASSWORD": "clickhouse" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposed secret in .github/workflows/unit-test.yml - high severity
Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
View details in Aikido Security
|
|
||
| FILE="binaryen-version_${BINARYEN_VERSION}-${ARCH}-${OS}.tar.gz" | ||
|
|
||
| mkdir -p ./.bin/tmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obvious duplication: repeated download/extract/cleanup sequence appears in both download_binaryen() and download_wasm_bindgen().
Details
✨ AI Reasoning
1) Both download_binaryen() and download_wasm_bindgen() contain repeated sequences (mkdir -p ./.bin/tmp, cd ./.bin/tmp, download, extract, cleanup).
2) This duplication increases maintenance burden because similar error handling or flow changes would need to be applied in multiple places.
3) The PR introduced these two similar blocks rather than consolidating shared download/extract semantics, so the duplication was introduced/worsened by this change.
🔧 How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| New-Item -ItemType Directory -Path ".\.bin\tmp" -Force | Out-Null | ||
| New-Item -ItemType Directory -Path $OUT_DIR -Force | Out-Null | ||
|
|
||
| function Download-Binaryen { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate download/extract logic in Download-Binaryen and Download-WasmBindgen should be consolidated into a shared helper to avoid repeated maintenance.
Details
✨ AI Reasoning
1) What the code does: Two newly added functions, Download-Binaryen (lines 24-38) and Download-WasmBindgen (lines 40-54), perform nearly identical steps: set ARCH/OS, compose a URL and filename, push into a tmp directory, download an archive, create an extract directory, and extract the archive.
2) Whether it harms maintainability: This duplication means future fixes (error handling, retry logic, path changes) must be applied in multiple places, increasing risk of divergence and bugs.
3) Why violation is true/false: The duplication was introduced by this PR (both functions are new and similar), so it is a true, actionable violation that can and should be consolidated into a shared helper to improve maintainability.
🔧 How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| SCRIPT_DIR="$(cd "$(dirname "$(realpath "$0")")" && pwd)" | ||
| pushd "$SCRIPT_DIR" > /dev/null | ||
|
|
||
| # Create directories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment '# Create directories' only states what the code does; explain why these directories are needed instead.
Details
✨ AI Reasoning
1) The script sets up and runs a wasm build: it changes to the script directory, creates directories, defines download/build functions, downloads toolchains, builds a Cargo target, and runs wasm-bindgen/wasm-opt.
2) The comment '# Create directories' simply restates the obvious code action (mkdir -p ...) and adds no design/intent context, which can clutter maintenance and go stale if the implementation changes.
3) This PR introduced that comment (file added), so it is a new, redundant 'what' comment; a better comment would explain why those specific directories are needed (e.g., 'ensure output and temporary bins exist for tool downloads and build artifacts').
🔧 How do I fix it?
Write comments that explain the purpose, reasoning, or business logic behind the code using words like 'because', 'so that', or 'in order to'.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
|
|
||
| # ------ Functions ------ | ||
|
|
||
| download_binaryen() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function download_binaryen() implements multi-step OS/ARCH detection and archive download/extract but lacks an explanatory comment.
Details
✨ AI Reasoning
1) download_binaryen() downloads and extracts a Binaryen release and detects OS/ARCH.
2) The function has multiple non-obvious branches and steps (OS/ARCH detection, URL construction, download, extraction) that would benefit from a short explanatory comment describing intent, platform assumptions, and side effects.
3) This change introduced a sizable function without documentation, making future maintenance harder.
🔧 How do I fix it?
Add docstrings or comments explaining what the function does, its parameters, and return values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| echo "Using rustc version: $RUSTC_VERSION" | ||
|
|
||
| # Ensure wasm32 target is installed | ||
| LIBDIR=$(rustc --target wasm32-unknown-unknown --print target-libdir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling 'rustc --target ... --print target-libdir' will fail (and exit the script due to set -e) if the target isn't installed, preventing the intended installation check.
Details
✨ AI Reasoning
1) The script attempts to detect whether the wasm32 target is installed by running 'rustc --target wasm32-unknown-unknown --print target-libdir' and then checking if the returned path exists (lines 129-141).
2) Because 'set -e' is enabled at the top, if the wasm32 target is not installed, the rustc invocation will fail and exit non‑zero, causing the whole script to terminate before the subsequent conditional can run; the code therefore cannot reach the intended directory-existence check.
3) This contradicts the apparent intention to detect and install the target if missing, so the control flow is broken and harms the script's reliability when the target is absent.
🔧 How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
See https://blog.rust-lang.org/inside-rust/2025/07/21/sunsetting-the-rustwasm-github-org/