feat: release v0.1.0#45
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a cached native FFI loader ( ChangesLuaRocks Packaging for lua-qjson
Sequence Diagram(s)sequenceDiagram
participant Consumer as lua/qjson.lua (consumer)
participant Loader as lua/qjson/lib.lua (loader)
participant FFI as LuaJIT FFI (ffi.load)
Consumer->>Loader: require("qjson.lib")
Loader->>FFI: ffi.load("qjson") / ffi.load("libqjson")
Loader->>FFI: ffi.load(resolved package.cpath paths)
FFI-->>Loader: library handle or error
Loader-->>Consumer: return handle or diagnostic error (attempts + last_error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/release.yml (2)
10-13: ⚡ Quick winDeclare workflow token permissions explicitly.
softprops/action-gh-releaseneedscontents: write; relying on repository defaults makes this workflow fragile across environments.Proposed fix
name: Release on: push: @@ - "rockspec/**" + +permissions: + contents: write🤖 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/release.yml around lines 10 - 13, The workflow currently relies on repo defaults for permissions; explicitly declare permissions for the release job so softprops/action-gh-release has contents: write. Add a permissions block (at top-level or within the release job) that includes contents: write to ensure the GitHub token used by the action can create releases; reference the release job and the softprops/action-gh-release action when making the change so the permission is scoped for that job.
33-35: ⚡ Quick winUse
v?instead ofv*in the release-version regex for correct semantics.The regex
^feat: release v*(\S+)will match commit titles, butv*(zero or more) allows unintended patterns likefeat: release vv0.1.0. Usev?(zero or one) to properly match optional single-v prefix. Note:\Sis supported in bash, so that is not a concern.🤖 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/release.yml around lines 33 - 35, Update the release-title regex stored in the variable re so it uses v? instead of v* (change the pattern from ^feat: release v*(\S+) to ^feat: release v?(\S+)) to ensure only an optional single "v" is allowed; keep the same match usage against title and the downstream BASH_REMATCH[1] assignment to build v (v=${BASH_REMATCH[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 @.github/workflows/release.yml:
- Around line 31-33: The script injects github.event.head_commit.message
directly into the shell which risks command injection; fix by passing
github.event.head_commit.message into the runner as an environment variable and
reference that safe variable (e.g., set an env like COMMIT_MSG from
github.event.head_commit.message) then use quoted parameter expansion when
assigning title (title="${COMMIT_MSG}") and keep the regex variable re
unchanged; ensure all uses of github.event.head_commit.message in this job use
the env var and always quote the variable expansions.
In `@rockspec/lua-qjson-0.1.0-1.rockspec`:
- Around line 20-22: The rockspec currently declares "lua >= 5.1" which is too
permissive for this package because multiple modules (lua/qjson.lua,
lua/qjson/lib.lua, lua/qjson/table.lua) rely on LuaJIT's FFI; update the
dependencies table to require LuaJIT explicitly (e.g. replace or add an entry
like "luajit >= 2.1" or the appropriate LuaRocks luajit package name/version) so
installs on plain Lua runtimes are prevented and ensure the dependency change is
applied in the dependencies = { ... } block of the rockspec.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 10-13: The workflow currently relies on repo defaults for
permissions; explicitly declare permissions for the release job so
softprops/action-gh-release has contents: write. Add a permissions block (at
top-level or within the release job) that includes contents: write to ensure the
GitHub token used by the action can create releases; reference the release job
and the softprops/action-gh-release action when making the change so the
permission is scoped for that job.
- Around line 33-35: Update the release-title regex stored in the variable re so
it uses v? instead of v* (change the pattern from ^feat: release v*(\S+) to
^feat: release v?(\S+)) to ensure only an optional single "v" is allowed; keep
the same match usage against title and the downstream BASH_REMATCH[1] assignment
to build v (v=${BASH_REMATCH[1]}).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 867c993c-e932-4e08-9522-e8643e3ef73f
📒 Files selected for processing (6)
.github/workflows/release.ymlREADME.mdlua/qjson.lualua/qjson/lib.lualua/qjson/table.luarockspec/lua-qjson-0.1.0-1.rockspec
There was a problem hiding this comment.
Pull request overview
Adds LuaRocks packaging and release automation for the first lua-qjson release, while keeping the Lua module name as qjson.
Changes:
- Adds a LuaRocks rockspec for building/installing the Rust native library and Lua modules.
- Adds a
qjson.libFFI loader and routes existing Lua wrappers through it. - Adds install documentation and a GitHub Actions release/upload workflow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
rockspec/lua-qjson-0.1.0-1.rockspec |
Defines LuaRocks package metadata, build, and install commands. |
README.md |
Documents LuaRocks installation and module name. |
lua/qjson/lib.lua |
Adds native-library loading via ffi.load and package.cpath. |
lua/qjson.lua |
Uses the shared native-library loader. |
lua/qjson/table.lua |
Uses the shared native-library loader. |
.github/workflows/release.yml |
Adds release creation and LuaRocks upload automation. |
Comments suppressed due to low confidence (4)
.github/workflows/release.yml:32
- Interpolating
github.event.head_commit.messagedirectly into a bash script lets quotes or command substitutions in the commit message alter the generated script. Pass the commit message through an environment variable (or another escaped input mechanism) before parsing it so a crafted squash/merge commit cannot execute arbitrary shell commands in the release job.
title="${{ github.event.head_commit.message }}"
.github/workflows/release.yml:33
- Bash uses extended regular expressions here, not PCRE, so
\Sis not a non-whitespace character class. As written, the release commit messagefeat: release v0.1.0will not match and the workflow exits before creating/uploading the release; use a Bash-compatible character class such as[^[:space:]]+.
re="^feat: release v*(\S+)"
lua/qjson/lib.lua:22
package.cpathcan contain literal all-in-one entries such as.../loadall.sothat do not include?. This code still passes those unchanged paths toffi.load; if such a shared object exists, the loader can accept a library that does not export the qjson symbols and fail later with an unrelated symbol lookup error instead of continuing to search or reporting that qjson was missing. Skip templates without?or validate a qjson symbol after loading.
local path = string.gsub(template, "%?", name)
local lib = try_load(path)
lua/qjson/lib.lua:31
- The new package.cpath fallback is not exercised by the existing Lua CI path, which still loads the library through
LD_LIBRARY_PATH. Because this fallback is what makes installed LuaRocks usage work withoutLD_LIBRARY_PATH, add an automated Lua/LuaRocks test (or a CI variant that clearsLD_LIBRARY_PATHand pointsLUA_CPATHat the built library) so regressions in the release loader are caught before publishing.
local lib = try_load("qjson") or try_load("libqjson") or load_from_cpath()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
99-99: ⚡ Quick winAvoid hard-coding the rockspec version in package validation.
This locks CI to
0.1.0and can miss regressions when the release version changes.Proposed change
- luarocks make rockspec/lua-qjson-0.1.0-1.rockspec --tree /tmp/lua-qjson-rock + ROCKSPEC="$(ls -1 rockspec/lua-qjson-*-1.rockspec | sort -V | tail -n1)" + luarocks make "$ROCKSPEC" --tree /tmp/lua-qjson-rock🤖 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/ci.yml at line 99, In the CI step that runs "luarocks make rockspec/lua-qjson-0.1.0-1.rockspec --tree /tmp/lua-qjson-rock" avoid hard-coding the version; change it to pick the rockspec dynamically (e.g., use a shell glob like rockspec/lua-qjson-*.rockspec or an env var such as $ROCKSPEC or a simple find/ls to select the rockspec file) so CI validates whatever release version is present instead of being locked to 0.1.0.
🤖 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/release.yml:
- Around line 38-45: The current regex re="^feat: release v?(\S+)" in the
release workflow is too permissive; replace it with a stricter SemVer capture
(e.g. re="^feat: release v?([0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?)$") and use
that capture for v and version_without_v when matching title; update the
matching block that sets v=${BASH_REMATCH[1]} and writes to GITHUB_OUTPUT so
only validated SemVer strings are emitted for the GitHub tag and rockspec
filename.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 99: In the CI step that runs "luarocks make
rockspec/lua-qjson-0.1.0-1.rockspec --tree /tmp/lua-qjson-rock" avoid
hard-coding the version; change it to pick the rockspec dynamically (e.g., use a
shell glob like rockspec/lua-qjson-*.rockspec or an env var such as $ROCKSPEC or
a simple find/ls to select the rockspec file) so CI validates whatever release
version is present instead of being locked to 0.1.0.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcb5cc9e-fd37-4c37-bb03-6cf977930630
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/release.yml
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
48-61: ⚡ Quick winConsider uploading to LuaRocks before creating the GitHub release.
Creating the GitHub release (line 48) before uploading to LuaRocks (line 56) means that if the upload fails, the repository will have a release tag without a corresponding LuaRocks package. While the workflow can be re-run, this creates a window of inconsistency where users might see the GitHub release but cannot install via LuaRocks.
Reordering so that LuaRocks upload happens first would ensure that only successful uploads result in a GitHub release:
🔄 Suggested reordering
fi - - name: Create Release - uses: softprops/action-gh-release@v2 - with: - tag_name: ${{ steps.release_env.outputs.version }} - name: ${{ steps.release_env.outputs.version }} - draft: false - prerelease: false - - name: Upload to LuaRocks env: LUAROCKS_TOKEN: ${{ secrets.LUAROCKS_TOKEN }} run: | luarocks install dkjson luarocks upload rockspec/lua-qjson-${{ steps.release_env.outputs.version_without_v }}-1.rockspec --api-key=${LUAROCKS_TOKEN} + + - name: Create Release + uses: softprops/action-gh-release@v2 + with: + tag_name: ${{ steps.release_env.outputs.version }} + name: ${{ steps.release_env.outputs.version }} + draft: false + prerelease: ${{ steps.release_env.outputs.is_prerelease }}This ensures that the GitHub release is only created after a successful LuaRocks upload.
🤖 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/release.yml around lines 48 - 61, Swap the "Create Release" and "Upload to LuaRocks" steps so the LUAROCKS upload runs before the softprops/action-gh-release step; specifically move the entire "Upload to LuaRocks" step (the step that sets LUAROCKS_TOKEN and runs "luarocks install dkjson" + "luarocks upload rockspec/lua-qjson-${{ steps.release_env.outputs.version_without_v }}-1.rockspec --api-key=${LUAROCKS_TOKEN}") above the "Create Release" step that uses softprops/action-gh-release@v2 (which uses tag_name and name from steps.release_env.outputs.version), ensuring the upload succeeds before creating the GitHub release and that both steps still reference the same release_env outputs.
🤖 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/release.yml:
- Around line 56-61: Add a pre-release verification step that checks the
expected rockspec file exists using the same version token used later
(steps.release_env.outputs.version_without_v) before the "Create Release" job
runs; specifically, insert a step (e.g., "Verify rockspec exists") prior to the
"Create Release" step that tests for the file rockspec/lua-qjson-${{
steps.release_env.outputs.version_without_v }}-1.rockspec and fails the workflow
with a clear message if missing so the subsequent "Create Release" and "Upload
to LuaRocks" steps (referenced by their names) are not executed when the
rockspec is absent.
- Around line 37-46: The current matcher uses the full HEAD_COMMIT_MESSAGE
stored in title, which can include additional lines and breaks the ^...$ regex;
before applying the re match, extract just the first line (the commit title)
from title and use that single-line string for the regex match so the existing
re, BASH_REMATCH and subsequent v/BASH_REMATCH[1] logic work unchanged.
- Around line 48-54: The release job currently always sets prerelease: false in
the "Create Release" step (uses: softprops/action-gh-release@v2) so prerelease
tags like 1.0.0-rc1 get published as stable; update the "Extract release name"
step to emit a prerelease output (e.g., steps.release_env.outputs.prerelease) by
testing the version string for a dash/semver prerelease identifier, and then
change the "Create Release" step to use that output for the prerelease field
(prerelease: ${{ steps.release_env.outputs.prerelease }}), ensuring prerelease
releases are marked correctly.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 48-61: Swap the "Create Release" and "Upload to LuaRocks" steps so
the LUAROCKS upload runs before the softprops/action-gh-release step;
specifically move the entire "Upload to LuaRocks" step (the step that sets
LUAROCKS_TOKEN and runs "luarocks install dkjson" + "luarocks upload
rockspec/lua-qjson-${{ steps.release_env.outputs.version_without_v }}-1.rockspec
--api-key=${LUAROCKS_TOKEN}") above the "Create Release" step that uses
softprops/action-gh-release@v2 (which uses tag_name and name from
steps.release_env.outputs.version), ensuring the upload succeeds before creating
the GitHub release and that both steps still reference the same release_env
outputs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62bf3413-6836-4422-a588-b9233f9d648e
📒 Files selected for processing (2)
.github/workflows/release.ymllua/qjson/lib.lua
🚧 Files skipped from review as they are similar to previous changes (1)
- lua/qjson/lib.lua
|
I addressed the pre-merge findings from the CodeRabbit summary. The LuaRocks token is no longer passed as I also added |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
The failed I pinned that job to |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
.github/workflows/ci.yml:136
- This duplicates the lexicographic rockspec selection used above, so the macOS/Linux package job can validate an older version once filenames such as
0.9.0and0.10.0coexist. Use version-aware sorting or pass the exact rockspec to avoid silently testing the wrong package.
ROCKSPEC="$(ls -1 rockspec/lua-qjson-*.rockspec | sort | tail -n1)"
|
The package jobs got past the LuaJIT install after pinning the version, then failed because the action exposes the LuaJIT runtime as I changed the package smoke test to use |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/ci.yml:164
- This duplicate selector has the same raw-string prerelease ordering issue:
rc.2will sort afterrc.10, even though SemVer ordersrc.10later. The package job could therefore build and smoke-test the wrong prerelease rockspec once multiple prereleases are checked in.
major, minor, patch, prerelease, revision = match.groups()
matches.append((int(major), int(minor), int(patch), prerelease is None, prerelease or "", int(revision), str(path)))
if not matches:
raise SystemExit("no lua-qjson rockspec found")
print(max(matches)[-1])
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Add LuaRocks packaging for the first lua-qjson release. The rock name is
lua-qjson, while the Lua module remainsrequire("qjson"), so it avoids the existingqjsonpackage name on LuaRocks.This also adds a small FFI loader that keeps local
LD_LIBRARY_PATHusage working and can load the native library from LuaRockspackage.cpathafter install. The loader now validates the full FFI symbol set before accepting a shared object, and the Lua test suite covers loader failure diagnostics.The release workflow publishes from
feat: release vX.Y.Z[-prerelease]commits, validates the version before using it in tags or filenames, marks prereleases correctly, chooses the matching checked-in rockspec revision with numeric revision ordering, validatesLUAROCKS_TOKENbefore creating a GitHub release, and writes the token to the LuaRocks upload config instead of passing it as a command-line argument. CI validates LuaRocks installs on Ubuntu and macOS.Validated locally with:
cargo test --releasecargo test --release --no-default-featurescargo test --features test-panic --releaseLD_LIBRARY_PATH="$PWD/target/release" LUA_CPATH="./vendor/lua-cjson/?.so;./target/release/lib?.so;;" busted --lua="$(which luajit)" tests/lua --lpath="./lua/?.lua"luarocks make rockspec/lua-qjson-0.1.0-1.rockspec --tree /tmp/lua-qjson-rock/tmp/lua-qjson-rockand noLD_LIBRARY_PATHluarocks lint rockspec/lua-qjson-0.1.0-1.rockspecSummary by CodeRabbit
New Features
Improved Reliability
Documentation
Tests