ci: add pipeline for iii-lsp - #22
Conversation
📝 WalkthroughWalkthroughAdds a new GitHub Actions workflow file that automates the release process for the iii-lsp Rust binary. The workflow triggers on tag pushes matching Changes
Possibly related PRs
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (1)
.github/workflows/release-lsp-binary.yml (1)
81-84: Remove the checkout step—it's unnecessary for creating releases withtag_nameandgenerate_release_notes.The
create-releasejob calls onlysoftprops/action-gh-release@v2withtag_nameandgenerate_release_notes, which do not require a checked-out repository. The action accesses these via the GitHub Releases API, not the workspace. Removing checkout reduces runner time and limits token exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-lsp-binary.yml around lines 81 - 84, Remove the unnecessary checkout step (the action usage "actions/checkout@v4" block) from the create-release job so the workflow relies solely on softprops/action-gh-release@v2 with tag_name and generate_release_notes; locate the job that invokes "softprops/action-gh-release@v2" and delete the "uses: actions/checkout@v4" step (including its with: token) to reduce runner time and token exposure while leaving the release action and its inputs intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-lsp-binary.yml:
- Around line 35-60: The workflow currently accepts arbitrary manual input in
the Resolve tag step (TAG in steps.resolve) and then parameter-expands away the
prefix in Extract metadata (steps.meta) without validating format, and later
passes the tag to softprops/action-gh-release@v2 without setting
target_commitish; fix by validating the resolved tag in the Resolve step (ensure
it matches the expected pattern like
^iii-lsp/v[0-9]+\.[0-9]+\.[0-9]+(?:-[a-z]+\.[0-9]+| -dry-run\.[0-9]+)?$ or at
minimum starts with "iii-lsp/v" and a semver-like version) and fail the job
early on invalid input, only allow stripping the "iii-lsp/v" prefix in Extract
metadata after validation (use steps.resolve.outputs.tag), and set the release
action's target_commitish to a deterministic ref (use github.sha for
manual_dispatch or github.ref for push events) when calling
softprops/action-gh-release@v2 so malformed or non-existent tags cannot create
releases from the default branch.
- Around line 17-19: The concurrency group currently uses github.ref (group:
release-iii-lsp-${{ github.ref }}) which resolves to branches for
workflow_dispatch; change it to key off the release tag instead by using the
release tag fallback expression, e.g. set group to release-iii-lsp-${{
github.event.release.tag_name || github.ref_name }}, so the concurrency group is
derived from the actual release tag (reference the concurrency group line and
the github.event.release.tag_name / github.ref_name symbols).
---
Nitpick comments:
In @.github/workflows/release-lsp-binary.yml:
- Around line 81-84: Remove the unnecessary checkout step (the action usage
"actions/checkout@v4" block) from the create-release job so the workflow relies
solely on softprops/action-gh-release@v2 with tag_name and
generate_release_notes; locate the job that invokes
"softprops/action-gh-release@v2" and delete the "uses: actions/checkout@v4" step
(including its with: token) to reduce runner time and token exposure while
leaving the release action and its inputs intact.
🪄 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: a873bcd6-44fa-42a3-bfaf-2abe3217ac18
📒 Files selected for processing (1)
.github/workflows/release-lsp-binary.yml
| concurrency: | ||
| group: release-iii-lsp-${{ github.ref }} | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GitHub Actions, for top-level concurrency.group, is inputsavailable onworkflow_dispatch, and what does github.refresolve to onpushvsworkflow_dispatch?
💡 Result:
-
Is
inputsavailable in a top-levelconcurrency.grouponworkflow_dispatch?
Yes. The workflow-levelconcurrencykey supports expressions using thegithub,inputs, andvarscontexts, and theinputscontext is available for workflows triggered byworkflow_dispatch. [1] -
What does
github.refresolve to?- On
push:github.ref/GITHUB_REFis the updated ref that was pushed (branch or tag), e.g.refs/heads/mainorrefs/tags/v1.2.3. [2] - On
workflow_dispatch:github.ref/GITHUB_REFis the branch or tag that received the dispatch (the ref you selected in the UI or passed asrefvia the API/CLI). [3]
- On
Sources
[1] GitHub Docs – Context availability table; concurrency supports github, inputs, vars, and inputs is available for workflow_dispatch workflows. (docs.github.com)
[2] GitHub Docs – push event: GITHUB_REF is the “Updated ref”. (docs.github.com)
[3] GitHub Docs – workflow_dispatch event: GITHUB_REF is “Branch or tag that received dispatch”. (docs.github.com)
Citations:
- 1: https://docs.github.com/en/enterprise-server%403.14/actions/reference/contexts-reference
- 2: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
- 3: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
Use the release tag in the concurrency key instead of the workflow ref.
Line 18 keys concurrency by github.ref. On workflow_dispatch, this resolves to the branch from which the dispatch was triggered (e.g., refs/heads/main), not the release tag. The same release tag can therefore run under different concurrency groups when triggered via workflow_dispatch vs. a direct tag push, allowing concurrent execution instead of serialization.
Proposed fix
concurrency:
- group: release-iii-lsp-${{ github.ref }}
+ group: release-iii-lsp-${{ github.event_name == 'workflow_dispatch' && inputs.tag || github.ref_name }}
cancel-in-progress: false📝 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.
| concurrency: | |
| group: release-iii-lsp-${{ github.ref }} | |
| cancel-in-progress: false | |
| concurrency: | |
| group: release-iii-lsp-${{ github.event_name == 'workflow_dispatch' && inputs.tag || github.ref_name }} | |
| cancel-in-progress: false |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release-lsp-binary.yml around lines 17 - 19, The
concurrency group currently uses github.ref (group: release-iii-lsp-${{
github.ref }}) which resolves to branches for workflow_dispatch; change it to
key off the release tag instead by using the release tag fallback expression,
e.g. set group to release-iii-lsp-${{ github.event.release.tag_name ||
github.ref_name }}, so the concurrency group is derived from the actual release
tag (reference the concurrency group line and the github.event.release.tag_name
/ github.ref_name symbols).
| - name: Resolve tag | ||
| id: resolve | ||
| run: | | ||
| TAG="${{ inputs.tag || github.ref_name }}" | ||
| echo "tag=$TAG" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Extract metadata from tag | ||
| id: meta | ||
| env: | ||
| TAG: ${{ steps.resolve.outputs.tag }} | ||
| run: | | ||
| VERSION="${TAG#iii-lsp/v}" | ||
|
|
||
| if [[ "$VERSION" =~ -dry-run\.[0-9]+$ ]]; then | ||
| echo "dry_run=true" >> "$GITHUB_OUTPUT" | ||
| echo "is_prerelease=true" >> "$GITHUB_OUTPUT" | ||
| elif [[ "$VERSION" =~ -([a-z]+)\.[0-9]+$ ]]; then | ||
| echo "dry_run=false" >> "$GITHUB_OUTPUT" | ||
| echo "is_prerelease=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "dry_run=false" >> "$GITHUB_OUTPUT" | ||
| echo "is_prerelease=false" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
| echo "version=$VERSION" >> "$GITHUB_OUTPUT" | ||
| echo "::notice::iii-lsp release -- tag=$TAG version=$VERSION" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For softprops/action-gh-release@v2, what happens when tag_name does not already exist? Does it create the tag from the workflow commit?
💡 Result:
If you pass a tag_name that doesn’t already exist, softprops/action-gh-release@v2 will still create the release: it calls GitHub’s “Create a release” API, where tag_name may be an existing tag or a new one, and target_commitish specifies what commit/branch the new tag is created from (and is ignored if the tag already exists). [1]
So it does not automatically create the tag from the workflow run’s commit unless you explicitly set:
target_commitish: ${{ github.sha }}
Otherwise, the tag is created from the repo’s default branch by default. [2]
Sources:
[1] GitHub REST API “Create a release” parameters (tag_name, target_commitish) https://docs.github.com/en/rest/releases/releases
[2] softprops/action-gh-release@v2 README (tag_name default, target_commitish behavior/default) https://github.com/softprops/action-gh-release
🏁 Script executed:
# First, let's find and read the workflow file
find . -name "release-lsp-binary.yml" -type fRepository: iii-hq/workers
Length of output: 100
🏁 Script executed:
# If found, read the entire workflow file to understand context
cat -n .github/workflows/release-lsp-binary.ymlRepository: iii-hq/workers
Length of output: 4363
Validate manual dispatch input against tag format before creating release.
Line 38 accepts arbitrary user input for manual dispatch without format validation. Line 46 blindly strips iii-lsp/v via parameter expansion, producing invalid version metadata. More critically, the resolved tag is passed to softprops/action-gh-release@v2 (line 89) without target_commitish set. This means:
- A malformed tag like
iii-lsp/invalidcreates an invalid release - A non-existent tag (e.g., manually-created
iii-lsp/v1.0.0) is created as a new tag from the default branch, not the workflow commit - Release metadata is derived from unvalidated input
Push-triggered events are safe (GitHub enforces iii-lsp/v* pattern), but manual dispatch must validate tag format before processing.
Proposed fix
- name: Resolve tag
id: resolve
run: |
TAG="${{ inputs.tag || github.ref_name }}"
+ if [[ ! "$TAG" =~ ^iii-lsp/v[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?$ ]]; then
+ echo "::error::Invalid tag format: $TAG"
+ exit 1
+ fi
echo "tag=$TAG" >> "$GITHUB_OUTPUT"
+
+ - name: Verify tag exists (manual dispatch)
+ if: github.event_name == 'workflow_dispatch'
+ env:
+ TAG: ${{ steps.resolve.outputs.tag }}
+ run: |
+ git ls-remote --exit-code --tags "https://github.com/${{ github.repository }}.git" "refs/tags/$TAG" > /dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release-lsp-binary.yml around lines 35 - 60, The workflow
currently accepts arbitrary manual input in the Resolve tag step (TAG in
steps.resolve) and then parameter-expands away the prefix in Extract metadata
(steps.meta) without validating format, and later passes the tag to
softprops/action-gh-release@v2 without setting target_commitish; fix by
validating the resolved tag in the Resolve step (ensure it matches the expected
pattern like ^iii-lsp/v[0-9]+\.[0-9]+\.[0-9]+(?:-[a-z]+\.[0-9]+|
-dry-run\.[0-9]+)?$ or at minimum starts with "iii-lsp/v" and a semver-like
version) and fail the job early on invalid input, only allow stripping the
"iii-lsp/v" prefix in Extract metadata after validation (use
steps.resolve.outputs.tag), and set the release action's target_commitish to a
deterministic ref (use github.sha for manual_dispatch or github.ref for push
events) when calling softprops/action-gh-release@v2 so malformed or non-existent
tags cannot create releases from the default branch.
Review follow-up (MOT-4214). CodeRabbit flagged that unregister holds the registrations lock across adapter.unsubscribe, which awaited the consumer drain — up to one whole invocation — stalling every trigger register/unregister behind one slow job. Releasing the lock (the suggested fix) would trade that for a race: a same-key register can run between the handler-lock release and the adapter's map removal, no-op on the adapter's duplicate check, and end up registered with no live consumer. Fixed at the root instead: builtin and rabbitmq unsubscribe now DETACH their worker task (drop the JoinHandle) rather than await it — the in-flight delivery still completes and acks, unsubscribe becomes a broker round-trip, and the handler lock stays held so the no-race property is preserved. The drain unit test now pins the detach contract (prompt return; delivery still completes and acks). Also from review: exact delivery-count assert in the redis fan-out e2e (a duplicate subscription now fails it) and a doc-comment typo. The consumer-stream-end-leaves-dead-record observation is pre-existing (same shape before this branch) and tracked as GH-22 (channel-level error recovery for the shared Arc<Channel>).
… delivery (#681) * feat(queue): per-binding queue subscriptions with metadata delivery Each durable:subscriber registration now gets its own adapter subscription keyed by a stable durable identity, and every delivery carries the trigger's stored metadata (the harness __binding pointer) via the new Invoker::call_delivery — without it every harness-binding delivery is an unresolvable fire (discovery run 4 lost all 18 messages). Subscription identity is derived at the trigger layer: metadata.__binding when present (harness bindings), else the target function id (plain SDK subscribers — the pre-existing queue naming, so live deployments keep their backlogs across the upgrade). SDK trigger ids are fresh UUIDs per registration and never key durable state; duplicate same-key registrations share one live subscription. Hardening from the workflow code review (15 findings, all addressed): - unsubscribe is non-destructive on every adapter: cancel the consumer, drain the in-flight delivery to completion, keep the queue/DLQ/store state a same-id resubscribe reattaches to. It fires on every routine subscriber disconnect, so deleting broker queues or purging the store there wiped exactly the data the trigger exists to keep (store.purge removed again with its only caller). - shutdown stays abort-based (process exit): in-flight jobs are owned by store inflight-recovery / broker redelivery; draining could hang exit behind one blocked invocation. - builtin enqueue snapshots the routing set before store I/O instead of holding the adapter-wide mutex across FileStore writes; the race test asserts observable behavior instead of lock internals. - rabbitmq/redis subscribe hold the write lock from duplicate-check through insert, closing a TOCTOU that leaked an unstoppable duplicate consumer; with queue deletion gone, the unsubscribe/resubscribe race that killed a freshly re-created queue is dissolved. - rabbitmq worker drains its JoinSet instead of abort_all; FIFO deliveries borrow instead of deep-cloning metadata per message; Worker::run carries the repo-conventional too_many_arguments allow. - IiiInvoker::call delegates to call_delivery; redis unsubscribe_locked and its HashMap-only tests are inlined/removed; the priority e2e predeclares the subscriber-id topology subscribe actually consumes. Compat: RabbitMQ topics consumed by harness bindings leave the old shared iii.{topic}.{function_id}.queue/.dlq bound to the fanout exchange with no consumer after upgrade (bindings now use per-binding queues); delete those manually. Their backlog is messages the old code was already dropping as unresolvable. Plain-SDK subscriber queue names are unchanged. * test(queue): pin unsubscribe backlog survival and resubscribe_all dedup Close the two coverage gaps worth closing from the review pass: - rabbitmq e2e (docker-only, no engine): unsubscribe detaches the consumer, publishes while detached buffer on the still-bound durable queue, and a same-id resubscribe drains them — the broker-side half of the non-destructive-unsubscribe contract, previously untested anywhere. - trigger unit test: resubscribe_all collapses registrations sharing a subscription key to one subscribe against the swapped-in adapter. The remaining untested fixes are the subscribe TOCTOU lock scopes — inherently racy to pin, left structural. * fix(queue): detach unsubscribe drains instead of awaiting them Review follow-up (MOT-4214). CodeRabbit flagged that unregister holds the registrations lock across adapter.unsubscribe, which awaited the consumer drain — up to one whole invocation — stalling every trigger register/unregister behind one slow job. Releasing the lock (the suggested fix) would trade that for a race: a same-key register can run between the handler-lock release and the adapter's map removal, no-op on the adapter's duplicate check, and end up registered with no live consumer. Fixed at the root instead: builtin and rabbitmq unsubscribe now DETACH their worker task (drop the JoinHandle) rather than await it — the in-flight delivery still completes and acks, unsubscribe becomes a broker round-trip, and the handler lock stays held so the no-race property is preserved. The drain unit test now pins the detach contract (prompt return; delivery still completes and acks). Also from review: exact delivery-count assert in the redis fan-out e2e (a duplicate subscription now fails it) and a doc-comment typo. The consumer-stream-end-leaves-dead-record observation is pre-existing (same shape before this branch) and tracked as GH-22 (channel-level error recovery for the shared Arc<Channel>).
2 participants
Summary by CodeRabbit