Add tests and typecheck step#116
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR extracts workflow logic into scripts, adds production and lockfile-sync workflows, rewires package/tooling commands, updates repository config, and adjusts Rust/TypeScript tests and release automation. ChangesTesting and repository tooling
Nightly, production, publish, and sync workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant production as production.yml
participant git as git tags
participant gh as gh CLI
participant build as Build and Publish N-API
Operator->>production: workflow_dispatch version
production->>git: validate v$VERSION and package.json version
production->>git: compute stable and nightly tags
production->>git: create and push v$VERSION
production->>gh: gh release create Release $VERSION
production->>build: workflow-dispatch version_override
sequenceDiagram
participant nightly as nightly.yml
participant prodcheck as production_workflow.sh
participant filter as filter_significant.sh
participant release as create_release.sh
nightly->>prodcheck: check if production ran today
prodcheck-->>nightly: should_skip true/false
alt should_skip true
nightly->>nightly: exit 0
else should_skip false
nightly->>filter: check for significant commits
filter-->>nightly: significant true/false
nightly->>release: create nightly release
end
end
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 5</summary>
<details>
<summary>✅ Passed checks (5 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :---------------------------------------------------------------------------------------------------------------------------------- |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title matches a real part of the PR: it adds tests and introduces a typecheck step, though the change set is broader than that. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches</summary>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
- [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `chore/main`
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->
---
Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=soteenstudio/lightvm&utm_content=116)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
<details>
<summary>❤️ Share</summary>
- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai)
- [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)
- [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)
- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
</details>
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 38: The test:rust npm script is using a misspelled executable, which will
fail when invoked. Update the package.json script for test:rust to use node with
scripts/test-rust.js, and verify the Run Rust Tests CI step still calls the
corrected script successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a73104b-512f-4de7-9d24-60a56286261c
📒 Files selected for processing (3)
.github/workflows/ci.ymlpackage.jsonscripts/test-rust.js
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ts/tests/vmerror.test.ts (1)
32-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the required
hintDetailsarray toVMError. Ints/tests/vmerror.test.ts:35,VMErroris constructed without the required second argument, so this should benew VMError("Test error", []). Also,expect(err.code).toContain("LVM500")is redundant next to the exact match above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ts/tests/vmerror.test.ts` around lines 32 - 37, The VMError test is constructing VMError with only the message, but this class requires the hintDetails array as the second argument. Update the VMError instantiation inside the VMError throwable test to pass an empty array as the required hintDetails parameter, and remove the redundant code assertion if it is duplicated by the exact match. Use the VMError test block and its constructor call as the anchor for the change.
🧹 Nitpick comments (1)
ts/tests/vmerror.test.ts (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate assertion on
err.code.Line 22 (
toBe("LVM500")) and Line 29 (toContain("LVM500")) both assert the same property with overlapping intent. Consider removing one to reduce noise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ts/tests/vmerror.test.ts` around lines 22 - 29, The VMError test has a redundant duplicate assertion on err.code, so simplify the checks in vmerror.test.ts by keeping only one expectation that verifies the code value in the relevant test block around err.code. Remove the overlapping assertion near the end of the same test and retain the one that best matches the intended behavior, using VMError and the existing err.code assertions to locate the spot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ts/tests/vmerror.test.ts`:
- Around line 32-37: The VMError test is constructing VMError with only the
message, but this class requires the hintDetails array as the second argument.
Update the VMError instantiation inside the VMError throwable test to pass an
empty array as the required hintDetails parameter, and remove the redundant code
assertion if it is duplicated by the exact match. Use the VMError test block and
its constructor call as the anchor for the change.
---
Nitpick comments:
In `@ts/tests/vmerror.test.ts`:
- Around line 22-29: The VMError test has a redundant duplicate assertion on
err.code, so simplify the checks in vmerror.test.ts by keeping only one
expectation that verifies the code value in the relevant test block around
err.code. Remove the overlapping assertion near the end of the same test and
retain the one that best matches the intended behavior, using VMError and the
existing err.code assertions to locate the spot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: bc401ae4-5634-4e28-b877-ecc1df8a0c1f
📒 Files selected for processing (1)
ts/tests/vmerror.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/production.yml (1)
41-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNightly-tag filtering uses lexicographic string comparison, not version comparison.
awk -v last="$LAST_STABLE" '$0 > last'compares tag strings lexicographically. This can misorder tags whenever the base version changes width (e.g.v0.9.0vsv0.10.0-nightly...), silently including/excluding nightly entries from the generated release notes.♻️ Suggested fix: use version-aware sort/compare
- FINAL_NIGHTLIES=$(echo "$NIGHTLIES" | awk -v last="$LAST_STABLE" '$0 > last') + FINAL_NIGHTLIES=$(printf '%s\n%s\n' "$LAST_STABLE" "$NIGHTLIES" | sort -V | sed -n "/^${LAST_STABLE//./\\.}$/,\$p" | tail -n +2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/production.yml around lines 41 - 58, The nightly-tag filter in the release-notes generation step is using plain string comparison, so it can misclassify tags when versions change width. Update the tag selection logic in the workflow block that builds LAST_STABLE, NIGHTLIES, and FINAL_NIGHTLIES to use version-aware comparison/sorting instead of awk lexical ordering, so nightly tags are filtered consistently relative to the last stable tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/nightly.yml:
- Around line 39-44: The “Stop if Prod already ran” step in the nightly workflow
only exits that step, so the rest of the job can still continue; update the
downstream steps in nightly.yml to also check
`steps.check_prod.outputs.should_skip != 'true'`, or move the prod check into a
separate gating job. Use the existing `check_prod` and `should_skip` outputs to
ensure tagging and release steps are skipped whenever production already ran.
- Around line 19-37: The check in the nightly workflow is using the latest
production workflow run regardless of outcome, so a failed or cancelled run can
incorrectly set should_skip=true. Update the logic in the "Check if Production
Workflow ran today" step to only consider successful runs from gh run list for
production.yml, or filter by conclusion before comparing createdAt against
TODAY. Keep the should_skip output tied to a successful same-day run only.
In @.github/workflows/production.yml:
- Around line 17-19: The checkout step in the workflow leaves the GITHUB_TOKEN
persisted in local git config, which broadens token exposure across the whole
job. Update the actions/checkout@v4 usage to set persist-credentials to false,
then make the git push/gh steps use GH_TOKEN explicitly so only those steps
receive the token. Keep the change focused around the checkout and later
push-related steps in this workflow.
- Line 23: Move the workflow_dispatch version input out of the inline shell
assignment in the production workflow; the VERSION handling in the run blocks is
vulnerable to shell injection. Update the affected run steps to use env
variables for github.event.inputs.version, then validate VERSION against a
strict version format before any shell use. Keep the fix centered around the
production.yml run blocks that currently assign VERSION directly.
---
Nitpick comments:
In @.github/workflows/production.yml:
- Around line 41-58: The nightly-tag filter in the release-notes generation step
is using plain string comparison, so it can misclassify tags when versions
change width. Update the tag selection logic in the workflow block that builds
LAST_STABLE, NIGHTLIES, and FINAL_NIGHTLIES to use version-aware
comparison/sorting instead of awk lexical ordering, so nightly tags are filtered
consistently relative to the last stable tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71a2e07a-9b6f-4865-a9c4-ae9fc86ac4fd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/production.ymlpackage.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- package.json
Comment on lines
+39
to
+44
| - name: Stop if Prod already ran | ||
| if: steps.check_prod.outputs.should_skip == 'true' | ||
| run: | | ||
| echo "Prod sudah jalan, nightly cabut dulu ya. Bye!" | ||
| exit 0 | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant workflow section with line numbers.
sed -n '1,220p' .github/workflows/nightly.yml | cat -nRepository: soteenstudio/lightvm
Length of output: 9237
Gate the rest of the job on should_skip
exit 0 only ends this step; the remaining steps still run. Add if: steps.check_prod.outputs.should_skip != 'true' to the downstream steps (or move the prod check into a separate gating job), otherwise nightly can still tag and release on the same day as production.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/nightly.yml around lines 39 - 44, The “Stop if Prod
already ran” step in the nightly workflow only exits that step, so the rest of
the job can still continue; update the downstream steps in nightly.yml to also
check `steps.check_prod.outputs.should_skip != 'true'`, or move the prod check
into a separate gating job. Use the existing `check_prod` and `should_skip`
outputs to ensure tagging and release steps are skipped whenever production
already ran.
Comment on lines
+17
to
+19
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Checkout doesn't opt out of credential persistence.
persist-credentials: false isn't set, so the GITHUB_TOKEN is left in the local git config for the whole job — flagged by zizmor (artipacked). Since subsequent steps here use git push/gh with the token intentionally, this may be a deliberate tradeoff, but consider scoping token exposure by using GH_TOKEN explicitly per step and disabling persistence on checkout.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 17-19: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/production.yml around lines 17 - 19, The checkout step in
the workflow leaves the GITHUB_TOKEN persisted in local git config, which
broadens token exposure across the whole job. Update the actions/checkout@v4
usage to set persist-credentials to false, then make the git push/gh steps use
GH_TOKEN explicitly so only those steps receive the token. Keep the change
focused around the checkout and later push-related steps in this workflow.
Source: Linters/SAST tools
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/production.yml (2)
17-19: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
persist-credentials: falsestill not set on checkout.The
GITHUB_TOKENremains persisted in local git config for the whole job. This was flagged previously without a fix applied.🔒 Suggested fix
- uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/production.yml around lines 17 - 19, The checkout step is still persisting the GITHUB_TOKEN in local git config because actions/checkout is missing persist-credentials set to false. Update the existing checkout configuration in the workflow to include persist-credentials: false alongside fetch-depth: 0 so the token is not retained for the rest of the job.
21-23: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winTemplate-injection fix is incomplete —
github.event.inputs.versionis still interpolated directly into the shell command.Wrapping the expression in double quotes (
"${{ github.event.inputs.version }}") does not prevent injection: GitHub Actions substitutes the expression textually into therun:script before bash parses it, so a value containing a"followed by;/backticks can break out of the quotes and execute arbitrary commands on the runner. Static analysis (zizmor) still flags this exact line. Pass the input viaenv:instead so it goes through environment-variable substitution rather than script-text interpolation.🔒 Suggested fix
- name: Validate Version & Check Tag - run: bash .github/scripts/production/validate_version_tag.sh "${{ github.event.inputs.version }}" + run: bash .github/scripts/production/validate_version_tag.sh "$VERSION" + env: + VERSION: ${{ github.event.inputs.version }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/production.yml around lines 21 - 23, The Validate Version & Check Tag step still interpolates github.event.inputs.version directly into the run script, so the shell command remains injectable even with double quotes. Update the workflow step to pass the version through env: and have the script read that environment variable instead of embedding the expression in the bash command. Use the Validate Version & Check Tag step as the place to fix this and keep the existing production/validate_version_tag.sh invocation unchanged except for sourcing the input from an environment variable.Source: Linters/SAST tools
🧹 Nitpick comments (2)
.github/scripts/production/validate_version_tag.sh (1)
1-11: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd
set -euo pipefailfor safer failure behavior.The script has no shebang or strict-mode settings. If
jqis missing/fails,FILE_VERSIONsilently becomes empty instead of the script aborting, potentially masking a broken environment.🛡️ Proposed fix
+#!/bin/bash +set -euo pipefail + VERSION="$1" if git rev-parse "refs/tags/v$VERSION" >/dev/null 2>&1; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/production/validate_version_tag.sh around lines 1 - 11, The version validation script is missing strict failure handling, so a bad or missing jq can leave FILE_VERSION empty and hide environment issues. Add shell strict mode at the top of the script with the existing VERSION and FILE_VERSION flow in validate_version_tag.sh, and keep the jq-based package.json check so the script aborts immediately on unset variables or command failures..github/scripts/publish/build_release.sh (1)
3-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "ensure ./cross exists" bootstrap across musl and default branches.
The new
muslbranch repeats the identicalif [ ! -f "./cross" ]; then curl ... | tar xz; chmod +x cross; fiblock already present in theelsebranch. Extracting this into a small function (or hoisting it above the conditional, since it's identical in both non-android branches) would remove the duplication and centralize any future change (e.g., adding checksum verification or targeting a non-x86_64 host).♻️ Suggested consolidation
if [[ "$PLATFORM" == *"android"* ]]; then cargo build --release --target $TARGET -elif [[ "$PLATFORM" == *"musl"* ]]; then - if [ ! -f "./cross" ]; then - curl -L https://github.com/cross-rs/cross/releases/latest/download/cross-x86_64-unknown-linux-musl.tar.gz | tar xz - chmod +x cross - fi - RUSTFLAGS="-C target-feature=-crt-static" ./cross build --release --target $TARGET else if [ ! -f "./cross" ]; then curl -L https://github.com/cross-rs/cross/releases/latest/download/cross-x86_64-unknown-linux-musl.tar.gz | tar xz chmod +x cross fi - ./cross build --release --target $TARGET + if [[ "$PLATFORM" == *"musl"* ]]; then + RUSTFLAGS="-C target-feature=-crt-static" ./cross build --release --target $TARGET + else + ./cross build --release --target $TARGET + fi fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/publish/build_release.sh around lines 3 - 14, There is duplicated `./cross` bootstrap logic in `build_release.sh` across the `musl` and default branches. Refactor the repeated “ensure cross exists” block into a shared helper or hoist it before the platform split, then have both branches call that shared logic and keep only the branch-specific build command (`RUSTFLAGS=... ./cross build` for musl, plain `./cross build` otherwise).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/production/validate_version_tag.sh:
- Line 1: The validate_version_tag.sh script currently assigns VERSION from $1
without verifying it is present, which allows empty input to fall through into
misleading tag checks. Add an upfront guard in the script before VERSION is
used, and fail fast with a clear error message when the version argument is
missing or empty. Use the VERSION assignment and subsequent tag-validation logic
in validate_version_tag.sh as the place to enforce this check so the script
exits early instead of continuing with an empty version.
In @.github/scripts/stats/fetch_update_gist.sh:
- Around line 1-9: The GET requests in fetch_update_gist.sh are swallowing API
failures and turning auth errors into zero counts. Update the curl calls used
for the clones and views requests to fail on HTTP errors, matching the existing
PATCH behavior, so invalid or expired LIGHTVM_PAT credentials cause the script
to stop instead of letting jq in the CLONES and TRAFFIC_DATA parsing paths
default to 0.
---
Duplicate comments:
In @.github/workflows/production.yml:
- Around line 17-19: The checkout step is still persisting the GITHUB_TOKEN in
local git config because actions/checkout is missing persist-credentials set to
false. Update the existing checkout configuration in the workflow to include
persist-credentials: false alongside fetch-depth: 0 so the token is not retained
for the rest of the job.
- Around line 21-23: The Validate Version & Check Tag step still interpolates
github.event.inputs.version directly into the run script, so the shell command
remains injectable even with double quotes. Update the workflow step to pass the
version through env: and have the script read that environment variable instead
of embedding the expression in the bash command. Use the Validate Version &
Check Tag step as the place to fix this and keep the existing
production/validate_version_tag.sh invocation unchanged except for sourcing the
input from an environment variable.
---
Nitpick comments:
In @.github/scripts/production/validate_version_tag.sh:
- Around line 1-11: The version validation script is missing strict failure
handling, so a bad or missing jq can leave FILE_VERSION empty and hide
environment issues. Add shell strict mode at the top of the script with the
existing VERSION and FILE_VERSION flow in validate_version_tag.sh, and keep the
jq-based package.json check so the script aborts immediately on unset variables
or command failures.
In @.github/scripts/publish/build_release.sh:
- Around line 3-14: There is duplicated `./cross` bootstrap logic in
`build_release.sh` across the `musl` and default branches. Refactor the repeated
“ensure cross exists” block into a shared helper or hoist it before the platform
split, then have both branches call that shared logic and keep only the
branch-specific build command (`RUSTFLAGS=... ./cross build` for musl, plain
`./cross build` otherwise).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57033bb8-23f1-4594-881a-671343267ab6
📒 Files selected for processing (14)
.github/scripts/ci/deep_audit.sh.github/scripts/production/generate_release_notes.sh.github/scripts/production/validate_version_tag.sh.github/scripts/publish/build_release.sh.github/scripts/stats/fetch_update_gist.sh.github/scripts/tokens/create_issue.js.github/workflows/check-tokens.yml.github/workflows/production.yml.github/workflows/publish.yml.github/workflows/stats.yml.github/workflows/sync-deps.yml.lintstagedrc.jscspell.config.jsscripts/bench/ts-bench.js
💤 Files with no reviewable changes (1)
- .github/scripts/tokens/create_issue.js
✅ Files skipped from review due to trivial changes (2)
- .lintstagedrc.js
- cspell.config.js
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/stats.yml
- .github/scripts/production/generate_release_notes.sh
- .github/workflows/check-tokens.yml
- .github/workflows/sync-deps.yml
- .github/scripts/ci/deep_audit.sh
- .github/workflows/publish.yml
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Collaborator
Author
|
fix-lockfile |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
rust/src/types/instructions.rs (1)
559-567: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
assert!(matches!(...))over match+assert!(true).The match arm using
assert!(true)on match is a no-op; the real assertion is implicit success by not panicking. Usingmatches!is more idiomatic and readable.♻️ Suggested refactor
- match instr { - Instructions::Push(Value::Int16(_)) => assert!(true), - _ => panic!("Wrong data type!"), - } + assert!(matches!(instr, Instructions::Push(Value::Int16(_))), "Wrong data type!");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/types/instructions.rs` around lines 559 - 567, The test_instruction_round_trip test uses a match with assert!(true), which is redundant and less idiomatic. Update the assertion around Instructions::from_json_array and Instructions::Push(Value::Int16(_)) to use assert!(matches!(...)) instead of matching and asserting true, keeping the same type check while making the test clearer..coderabbit.yml (1)
4-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider noting this block is auto-generated.
This section is fully overwritten by
scripts/utils/sync-agents.json every sync. A short comment (e.g. "auto-generated from AGENTS.md, do not edit directly") would prevent someone from editing it here and losing the change on the next sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yml around lines 4 - 24, Add a short note in the system_instructions block of .coderabbit.yml indicating it is auto-generated and should not be edited directly. Keep the wording explicit enough to warn future editors that scripts/utils/sync-agents.js overwrites this section on sync, and make the change within the AGENTS_START/AGENTS_END block so the generated source remains clear..lintstagedrc.js (1)
14-18: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider the pre-commit latency cost of
cargo test --no-run.Compiling the test binary on every staged Rust file adds real latency to local commits, and CI (per the stack context) already runs the Rust test suite. This duplicates coverage at the cost of developer commit speed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.lintstagedrc.js around lines 14 - 18, The Rust staged-file hook in the lint-staged config is doing extra work by running cargo test --no-run on every change, which slows pre-commit without adding unique coverage. Update the Rust entry in the .lintstagedrc.js configuration to keep formatting and clippy checks, but remove the cargo test --no-run step so test compilation is left to CI; use the rust/**/*.rs hook block as the place to adjust this.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/vm/run.rs`:
- Around line 47-52: The test in run and test_run_with_valid_json is currently
using an unrecognized opcode casing and a weak assertion that can pass on error
output. Update the JSON program to use the lowercase opcode names expected by
run, such as push and stop, and replace the loose contains check with an
explicit assertion on the successful result shape so the test only passes when
the VM executes correctly.
In `@solas.toml`:
- Around line 21-23: The description for the test-ts script contains a typo in
its desc string, where “file$” should be “files”. Update the [scripts.test-ts]
entry in solas.toml to match the wording used by the test-rust description so
the script description is correct and consistent.
---
Nitpick comments:
In @.coderabbit.yml:
- Around line 4-24: Add a short note in the system_instructions block of
.coderabbit.yml indicating it is auto-generated and should not be edited
directly. Keep the wording explicit enough to warn future editors that
scripts/utils/sync-agents.js overwrites this section on sync, and make the
change within the AGENTS_START/AGENTS_END block so the generated source remains
clear.
In @.lintstagedrc.js:
- Around line 14-18: The Rust staged-file hook in the lint-staged config is
doing extra work by running cargo test --no-run on every change, which slows
pre-commit without adding unique coverage. Update the Rust entry in the
.lintstagedrc.js configuration to keep formatting and clippy checks, but remove
the cargo test --no-run step so test compilation is left to CI; use the
rust/**/*.rs hook block as the place to adjust this.
In `@rust/src/types/instructions.rs`:
- Around line 559-567: The test_instruction_round_trip test uses a match with
assert!(true), which is redundant and less idiomatic. Update the assertion
around Instructions::from_json_array and Instructions::Push(Value::Int16(_)) to
use assert!(matches!(...)) instead of matching and asserting true, keeping the
same type check while making the test clearer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fdbcf12-060d-460a-ab93-147f3a8b7c45
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.coderabbit.yml.gitattributes.github/scripts/production/validate_version_tag.sh.github/scripts/stats/fetch_update_gist.sh.github/workflows/sync-deps.yml.lintstagedrc.jscspell.config.jspackage.jsonrust/src/types/instructions.rsrust/src/types/value.rsrust/src/vm/execute.rsrust/src/vm/run.rsscripts/utils/sync-agents.jssolas.toml
✅ Files skipped from review due to trivial changes (2)
- rust/src/types/value.rs
- cspell.config.js
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/scripts/stats/fetch_update_gist.sh
- .github/scripts/production/validate_version_tag.sh
- package.json
Comment on lines
+47
to
+52
| #[test] | ||
| fn test_run_with_valid_json() { | ||
| let json = r#"[["PushInt32", 10], ["Stop"]]"#; | ||
| let result = run(json, None); | ||
| assert!(result.contains("10") || result.contains("status")); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether "PushInt32" is a recognized opcode string in from_json_array
rg -n '"push' rust/src/types/instructions.rs
rg -n 'op_bytes' -A60 rust/src/types/instructions.rs | rg -n 'PushInt32|pushint32'Repository: soteenstudio/lightvm
Length of output: 262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test and the relevant parsing/serialization code paths.
printf '\n== rust/src/vm/run.rs ==\n'
sed -n '1,120p' rust/src/vm/run.rs
printf '\n== rust/src/types/instructions.rs (instruction JSON parsing) ==\n'
sed -n '120,220p' rust/src/types/instructions.rs
printf '\n== rust/src/types/instructions.rs (opcode dispatch/encoding) ==\n'
sed -n '240,320p' rust/src/types/instructions.rs
printf '\n== any other references to PushInt32 / pushint32 / status in run.rs ==\n'
rg -n 'PushInt32|pushint32|status' rust/src/vm/run.rs rust/src/types/instructions.rsRepository: soteenstudio/lightvm
Length of output: 8250
Use a lowercase opcode and assert the success shape directly. PushInt32 is not recognized here, so this test goes through the error path; result.contains("10") || result.contains("status") still passes on that failure response. Use push/stop and check the expected success payload explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/vm/run.rs` around lines 47 - 52, The test in run and
test_run_with_valid_json is currently using an unrecognized opcode casing and a
weak assertion that can pass on error output. Update the JSON program to use the
lowercase opcode names expected by run, such as push and stop, and replace the
loose contains check with an explicit assertion on the successful result shape
so the test only passes when the VM executes correctly.
Comment on lines
21
to
+23
| [scripts.test-ts] | ||
| desc = "Running unit tests on .ts file$" | ||
| commands = ["node ./scripts/run-tests.js"] | ||
| commands = ["node ./scripts/test/test-ts.js"] |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo in description: "file$" should be "files".
Compare to the test-rust desc above which correctly says "files".
✏️ Proposed fix
-desc = "Running unit tests on .ts file$"
+desc = "Running unit tests on .ts files"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [scripts.test-ts] | |
| desc = "Running unit tests on .ts file$" | |
| commands = ["node ./scripts/run-tests.js"] | |
| commands = ["node ./scripts/test/test-ts.js"] | |
| [scripts.test-ts] | |
| desc = "Running unit tests on .ts files" | |
| commands = ["node ./scripts/test/test-ts.js"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@solas.toml` around lines 21 - 23, The description for the test-ts script
contains a typo in its desc string, where “file$” should be “files”. Update the
[scripts.test-ts] entry in solas.toml to match the wording used by the test-rust
description so the script description is correct and consistent.
Collaborator
Author
|
fix-lockfile |
1 similar comment
Collaborator
Author
|
fix-lockfile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What did you change?
Change type
Checklist
Link Issue (if any)
Summary by CodeRabbit
New Features
Bug Fixes
details/code/hint behavior.Chores