Release dry-run workflow with per-binary Linux packaging#129
Conversation
Refactor the release workflow to use the updated shared-actions commit (df81280dcc1d6e66134114dbc924313328b15f05) and add support for dry run mode via a reusable workflow pattern. Changes to release.yml: - Add workflow_call trigger with dry-run and publish inputs - Add metadata job using determine-release-modes and ensure-cargo-version - Replace hardcoded version extraction with metadata job outputs - Add conditional artifact upload based on release mode flags - Replace softprops/action-gh-release with upload-release-assets action - Add concurrency group to prevent parallel release runs - Add permissions block for contents: write New release-dry-run.yml: - Triggers on PR events (opened, synchronize, reopened, ready_for_review) - Calls release.yml with dry-run: true - Enables CI verification of release packaging without publishing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduce a reusable callable release workflow and a dry-run entrypoint; compute release metadata and conditionally upload artifacts; gate a publish job that prepares a draft release and uploads two assets; add binaries for two crates; extend test utilities to detect the new upload-release-assets publisher; expose test-only metadata helpers and simplify a test hook field. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GHA as GitHub Actions
participant Metadata as Metadata Job
participant Builder as Build-Packages Job
participant Artifacts as Artifact Storage
participant Release as Publish Job
PR->>GHA: Trigger workflow (release-dry-run or release)
GHA->>Metadata: Run metadata job to compute version and flags
Metadata-->>GHA: Return version, should_publish, dry_run, should_upload_workflow_artifacts
GHA->>Builder: Run build-packages with computed version
Builder->>Artifacts: Upload artifacts if flagged
GHA->>Release: If should_publish == true, prepare draft release
Release->>Artifacts: Download dist artifacts
Release->>Release: Upload assets (comenq, comenqd) to draft release
Release-->>GHA: Finalise publish or end dry-run
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Comment |
Reviewer's GuideMakes the release workflow reusable with metadata-driven versioning, adds a PR-triggered dry‑run workflow, switches Linux packaging to use the package version, and replaces softprops GH releases with shared upload-release-assets actions plus stronger tests around shared-actions usage. Sequence diagram for PR-triggered release dry-run workflowsequenceDiagram
actor Developer
participant GitHubActions
participant ReleaseDryRunWorkflow
participant ReleaseWorkflow
participant MetadataJob
participant BuildPackagesJob
participant ReleaseJob
participant DetermineModes
participant EnsureVersion
participant RustBuild
participant LinuxPackages
Developer->>GitHubActions: Open or update pull request
GitHubActions->>ReleaseDryRunWorkflow: Trigger on pull_request event
ReleaseDryRunWorkflow->>ReleaseWorkflow: Call with dry-run true
ReleaseWorkflow->>MetadataJob: Start metadata job
MetadataJob->>DetermineModes: Run determine-release-modes
DetermineModes-->>MetadataJob: should-publish, dry-run, should-upload-workflow-artifacts
MetadataJob->>EnsureVersion: Run ensure-cargo-version
EnsureVersion-->>MetadataJob: version
MetadataJob-->>ReleaseWorkflow: Expose outputs version and mode flags
ReleaseWorkflow->>BuildPackagesJob: Start build-packages matrix job
BuildPackagesJob->>RustBuild: Build binaries for each target
RustBuild-->>BuildPackagesJob: Build artifacts
BuildPackagesJob->>LinuxPackages: Package deb and rpm using version
LinuxPackages-->>BuildPackagesJob: Packaged artifacts
alt should-upload-workflow-artifacts or should-publish
BuildPackagesJob-->>GitHubActions: Upload workflow artifacts
else no uploads in dry run
BuildPackagesJob-->>GitHubActions: Skip artifact upload
end
alt should-publish is true
ReleaseWorkflow->>ReleaseJob: Run release job
ReleaseJob-->>GitHubActions: Ensure draft release and upload assets
else dry-run or no publish
ReleaseWorkflow-->>GitHubActions: Skip release job
end
Flow diagram for release mode and conditional artifact handlingflowchart TD
Start["Start release.yml workflow"] --> CallModes["Run determine-release-modes"]
CallModes --> ModesOut["Outputs: should-publish, dry-run, should-upload-workflow-artifacts"]
ModesOut --> CallVersion["Run ensure-cargo-version with check-tag from should-publish"]
CallVersion --> VersionOut["Output: version"]
VersionOut --> BuildPackages["Run build-packages job matrix"]
BuildPackages --> CondUpload["Condition: should-upload-workflow-artifacts is true OR should-publish is true"]
CondUpload -->|true| UploadArtifacts["Upload build artefacts as workflow artifacts"]
CondUpload -->|false| SkipUpload["Skip workflow artifact upload"]
ModesOut --> CondRelease["Condition: should-publish is true"]
CondRelease -->|true| RunRelease["Run release job"]
CondRelease -->|false| SkipRelease["Skip release job (dry run or no publish)"]
RunRelease --> EnsureDraft["Ensure draft GitHub release exists"]
EnsureDraft --> DownloadArtifacts["Download workflow artifacts to dist directory"]
DownloadArtifacts --> UploadComenq["Upload comenq release assets"]
DownloadArtifacts --> UploadComenqd["Upload comenqd release assets"]
UploadComenq --> CheckComenq["If upload-error is true, print error-message and fail"]
UploadComenqd --> CheckComenqd["If upload-error is true, print error-message and fail"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Add explicit [[bin]] sections to both comenq and comenqd crates to ensure Cargo correctly identifies and builds the binaries when building the workspace. Without explicit declarations, Cargo's auto-detection was failing because both crates have both lib.rs and main.rs files. Changes: - crates/comenq/Cargo.toml: Add [[bin]] section for comenq binary - crates/comenqd/Cargo.toml: Add [lib] and [[bin]] sections for clarity - test-support/src/workflow.rs: Update shared-actions SHA to df81280dcc1d6e66134114dbc924313328b15f05 and replace softprops/ action-gh-release check with upload-release-assets action reference This fixes the release dry-run workflow failure where cargo build completed but the binary was not found at the expected path target/<target>/release/<binary>. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
workflow_callinputs (publish,dry-run) aren’t passed intodetermine-release-modes, so it’s not obvious how those flags influenceshould_publish/dry_run; consider wiring the inputs explicitly into that action (or documenting that it derives everything from context only) to avoid surprises when reusing the workflow. - The
rust-build-releasestep no longer receives aversioninput whereaslinux-packagesdoes; if the build action still supports/needs explicit versioning, it may be clearer and safer to pass throughneeds.metadata.outputs.versionthere as well for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `workflow_call` inputs (`publish`, `dry-run`) aren’t passed into `determine-release-modes`, so it’s not obvious how those flags influence `should_publish`/`dry_run`; consider wiring the inputs explicitly into that action (or documenting that it derives everything from context only) to avoid surprises when reusing the workflow.
- The `rust-build-release` step no longer receives a `version` input whereas `linux-packages` does; if the build action still supports/needs explicit versioning, it may be clearer and safer to pass through `needs.metadata.outputs.version` there as well for consistency.
## Individual Comments
### Comment 1
<location> `.github/workflows/release.yml:7-16` </location>
<code_context>
push:
tags:
- 'v*.*.*'
+ workflow_call:
+ inputs:
+ publish:
+ description: >-
+ Whether the workflow should publish artefacts to the GitHub release
+ associated with the tag. Defaults to `false` when invoked as a
+ reusable workflow.
+ required: false
+ type: boolean
+ default: false
+ dry-run:
+ description: >-
</code_context>
<issue_to_address>
**issue (bug_risk):** workflow_call inputs are defined but never referenced in jobs, so they currently don't influence behaviour
`publish` and `dry-run` are defined as `workflow_call` inputs but not used anywhere (e.g., via `inputs.publish` / `inputs['dry-run']` or when calling `determine-release-modes`). As a result, callers can set them but they won’t affect behaviour, which is currently driven only by `determine-release-modes`. Consider either wiring these inputs into that action/conditions or removing them if they’re not needed.
</issue_to_address>
### Comment 2
<location> `test-support/src/workflow.rs:87` </location>
<code_context>
mod tests {
use super::{
- EXPECTED_RUST_BUILDER, EXPECTED_SHARED_ACTIONS_COMMIT, uses_shared_release_actions,
+ EXPECTED_RUST_BUILDER, EXPECTED_SHARED_ACTIONS_COMMIT, EXPECTED_UPLOAD_RELEASE_ASSETS,
+ uses_shared_release_actions,
};
</code_context>
<issue_to_address>
**suggestion (testing):** Add negative tests for the new upload-release-assets action detection (missing, wrong commit, unpinned).
Current tests only validate the happy path and builder-related failures; they don’t independently exercise the new `UPLOAD_RELEASE_ASSETS_PREFIX` / `EXPECTED_UPLOAD_RELEASE_ASSETS` logic.
Please add negative tests for the upload-release-assets action itself, e.g.:
- Workflow has the correct builder step but no upload-release-assets step → `uses_shared_release_actions` returns `false`.
- Workflow has the correct builder step and upload-release-assets with the right prefix but wrong commit → `false`.
- Workflow uses an unpinned upload-release-assets reference (e.g. `@v1` tag) instead of a commit → `false`.
These can mirror the existing builder negative tests so the pinning requirement is enforced consistently for both steps.
Suggested implementation:
```rust
#[cfg(test)]
mod tests {
use super::{
EXPECTED_RUST_BUILDER,
EXPECTED_SHARED_ACTIONS_COMMIT,
EXPECTED_UPLOAD_RELEASE_ASSETS,
uses_shared_release_actions,
};
// Existing tests remain here...
}
```
Please add the following new tests inside the `mod tests` block (reusing whatever workflow-building helpers the existing builder tests use). They should live alongside the existing negative tests for the builder action.
```rust
#[test]
fn uses_shared_release_actions_false_when_upload_release_assets_step_missing() {
// Workflow has the correct builder step but NO upload-release-assets step.
let workflow = Workflow::from_yaml_str(
r#"
jobs:
release:
steps:
- name: Build release
uses: ${EXPECTED_RUST_BUILDER}
"#,
)
.expect("valid workflow yaml");
assert!(
!uses_shared_release_actions(&workflow),
"Expected uses_shared_release_actions to be false when upload-release-assets step is missing",
);
}
#[test]
fn uses_shared_release_actions_false_when_upload_release_assets_wrong_commit() {
// Workflow has the correct builder step and upload-release-assets step,
// but the upload-release-assets reference is pinned to the wrong commit.
let workflow = Workflow::from_yaml_str(
r#"
jobs:
release:
steps:
- name: Build release
uses: ${EXPECTED_RUST_BUILDER}
- name: Upload release assets
uses: leynos/shared-actions/.github/actions/upload-release-assets@aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
"#,
)
.expect("valid workflow yaml");
assert!(
!uses_shared_release_actions(&workflow),
"Expected uses_shared_release_actions to be false when upload-release-assets is pinned to the wrong commit",
);
}
#[test]
fn uses_shared_release_actions_false_when_upload_release_assets_unpinned() {
// Workflow has the correct builder step and upload-release-assets step,
// but upload-release-assets is referenced via a tag (e.g. @v1) instead of a commit SHA.
let workflow = Workflow::from_yaml_str(
r#"
jobs:
release:
steps:
- name: Build release
uses: ${EXPECTED_RUST_BUILDER}
- name: Upload release assets
uses: leynos/shared-actions/.github/actions/upload-release-assets@v1
"#,
)
.expect("valid workflow yaml");
assert!(
!uses_shared_release_actions(&workflow),
"Expected uses_shared_release_actions to be false when upload-release-assets is unpinned (tagged ref)",
);
}
```
You will need to adjust the workflow-construction to match the helpers already in this file. For example:
- If there is a helper like `parse_workflow(yaml: &str) -> Workflow`, use that instead of `Workflow::from_yaml_str`.
- If tests interpolate constants differently (e.g. via `format!()` instead of `${...}`), update the YAML strings accordingly:
```rust
let yaml = format!(
r#"
jobs:
release:
steps:
- name: Build release
uses: {builder}
- name: Upload release assets
uses: {upload_assets}
"#,
builder = EXPECTED_RUST_BUILDER,
upload_assets = format!(
"leynos/shared-actions/.github/actions/upload-release-assets@{}",
EXPECTED_SHARED_ACTIONS_COMMIT,
),
);
let workflow = parse_workflow(&yaml);
```
Also ensure `EXPECTED_UPLOAD_RELEASE_ASSETS` is defined (likely mirroring `EXPECTED_RUST_BUILDER`), and consider using it in tests where you want the *correct* upload-release-assets reference so the negative tests clearly show only the mismatching parts (wrong commit or tag).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Line 109: The workflow conditional mixes boolean parsing and string
comparison: update the conditional that references
should_upload_workflow_artifacts and should_publish so both use the same boolean
parsing approach; specifically, wrap should_publish with fromJSON() (or
conversely remove fromJSON from should_upload_workflow_artifacts) so the if:
expression consistently evaluates booleans (e.g., use
fromJSON(needs.metadata.outputs.should_upload_workflow_artifacts) ||
fromJSON(needs.metadata.outputs.should_publish)).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release-dry-run.yml.github/workflows/release.ymlcrates/comenq/Cargo.tomlcrates/comenqd/Cargo.tomltest-support/src/workflow.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.
Files:
test-support/src/workflow.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
test-support/src/workflow.rs
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (12)
crates/comenqd/Cargo.toml (1)
6-11: LGTM!The explicit
[lib]and[[bin]]target declarations correctly separate the library and binary components, ensuring Cargo reliably builds both. This aligns with the PR's multi-binary release handling.crates/comenq/Cargo.toml (1)
9-11: LGTM!The
[[bin]]target declaration forcomenqfollows the same pattern ascomenqdand correctly enables the binary build for the release workflow's multi-binary handling.test-support/src/workflow.rs (4)
21-37: LGTM!The new constants for the upload-release-assets action follow the established pattern for the rust-build-release action. The compile-time
concat!approach avoids runtime allocations, consistent with the existingEXPECTED_RUST_BUILDERconstant.
71-76: LGTM!The logic correctly detects the upload-release-assets action by stripping the prefix and comparing the commit hash against
SHARED_ACTIONS_COMMIT, mirroring the existing builder detection pattern.
91-107: LGTM!The test correctly validates that both
EXPECTED_RUST_BUILDERandEXPECTED_UPLOAD_RELEASE_ASSETSend with the expected commit hash, and that the function detects both actions when present.
109-121: LGTM!Good addition of a test case verifying that missing the builder action causes the check to fail, even when the publisher action is present.
.github/workflows/release-dry-run.yml (1)
1-12: LGTM!The dry-run workflow correctly reuses the release workflow with
dry-run: true, enabling PR-triggered validation of the packaging process without publishing artefacts. Thesecrets: inheritdirective ensures required secrets are available to the called workflow..github/workflows/release.yml (5)
3-40: LGTM!The reusable workflow structure with inputs, outputs, and concurrency grouping is well-designed. The
cancel-in-progress: truesetting appropriately prevents redundant release runs for the same tag.
43-61: LGTM!The metadata job correctly centralizes version and release mode determination, making downstream jobs cleaner and ensuring consistency across the workflow.
130-141: LGTM!The idempotent release creation pattern using
gh release view ... || gh release create ...correctly handles both new and existing releases. Setting bothGITHUB_TOKENandGH_TOKENensures compatibility with both GitHub Actions and theghCLI.
166-177: Error checks execute after both uploads complete.The error-checking steps for
comenqandcomenqdrun sequentially after both upload steps, meaning ifupload_comenqfails,upload_comenqdstill executes before the workflow reports the error. If you intend to fail fast on the first upload error, move each error check immediately after its corresponding upload step. If the intent is to attempt all uploads and report all errors, this ordering is correct—add a comment clarifying this design choice.
146-165: LGTM!The per-binary upload steps correctly separate concerns and enable independent error tracking for each artefact. The action reference is pinned to the expected commit hash.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b9dcb00e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The rust-build-release shared action builds the manifest at
project-dir/Cargo.toml. Without project-dir, it defaults to the
workspace root which builds comenq-lib (a library with no binary).
By setting project-dir to crates/${{ matrix.bin }}, the action builds
each binary crate directly, ensuring the expected binary is produced
at target/<target>/release/<bin>.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Line 110: The condition mixes JSON-parsed and string comparisons; make them
consistent by parsing both outputs as booleans. Replace the second operand so
the line reads: if:
fromJSON(needs.metadata.outputs.should_upload_workflow_artifacts) ||
fromJSON(needs.metadata.outputs.should_publish) so both
`needs.metadata.outputs.should_upload_workflow_artifacts` and
`needs.metadata.outputs.should_publish` are evaluated via fromJSON().
- Around line 167-178: Replace direct interpolation of `${{
steps.upload_comenq.outputs.error-message }}` and `${{
steps.upload_comenqd.outputs.error-message }}` in the run blocks with
environment variables to avoid shell injection: for each check step (the steps
named "Check comenq upload errors" and "Check comenqd upload errors") add an env
entry that maps ERROR_MESSAGE (or a distinct name like
COMENQ_ERROR_MESSAGE/COMENQD_ERROR_MESSAGE) to the corresponding output (`${{
steps.upload_comenq.outputs.error-message }}` / `${{
steps.upload_comenqd.outputs.error-message }}`), then reference that env var in
the run script and ensure you expand it quoted (e.g., "$ERROR_MESSAGE") when
passing to printf/echo so newlines and special chars are handled safely.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (3)
.github/workflows/release.yml (3)
3-40: Well-structured reusable workflow configuration.The inputs/outputs and concurrency settings are sensible. Using
release-${{ github.ref }}for the concurrency group ensures that concurrent runs for the same tag are properly managed.
43-61: Metadata job correctly extracts version and release modes.Pinning the shared actions to a specific commit SHA (
df81280dcc1d6e66134114dbc924313328b15f05) is good practice for reproducibility and security.
131-142: Verify intentional draft-only release behaviour.The release is created with
--draftbut never undrafted. Confirm this is the intended workflow (i.e., manual publish after review) or add a step to publish the release after successful artifact uploads.
Update shared-actions to cb06757 which supports the manifest-path
parameter for rust-build-release. This allows building workspace
member crates by specifying their Cargo.toml path directly.
The previous project-dir approach did not work because the action
still used --manifest-path Cargo.toml relative to the checkout root,
building only the root comenq-lib package (a library with no binary).
With manifest-path: crates/${{ matrix.bin }}/Cargo.toml, the action
builds each binary crate directly, producing the expected binaries at
target/<target>/release/<bin>.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
In release builds (without test targets), code only used by tests appears as dead code. The -D warnings flag treats these as errors. Changes: - Gate METADATA_FILE_NAMES and is_metadata_file() with #[cfg(any(test, feature = "test-support"))] since they are only used in tests - Remove incorrect #[expect(dead_code)] from WorkerHooks::drained field, which is actually used in production code via notify_drained_if_empty() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Line 110: The conditional mixes boolean parsing styles: it uses
fromJSON(needs.metadata.outputs.should_upload_workflow_artifacts) but compares
needs.metadata.outputs.should_publish == 'true'; change the second check to
fromJSON(needs.metadata.outputs.should_publish) so both operands use fromJSON()
for consistent boolean handling, updating the if condition that references
should_upload_workflow_artifacts and should_publish accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.ymlcrates/comenqd/src/util.rscrates/comenqd/src/worker.rstest-support/src/workflow.rs
💤 Files with no reviewable changes (1)
- crates/comenqd/src/worker.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.
Files:
crates/comenqd/src/util.rstest-support/src/workflow.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
crates/comenqd/src/util.rstest-support/src/workflow.rs
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: release / Build comenqd for aarch64-unknown-linux-gnu
- GitHub Check: release / Build comenqd for x86_64-unknown-linux-gnu
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (12)
crates/comenqd/src/util.rs (4)
1-3: LGTM!Module-level documentation is present and clearly explains the module's purpose.
5-12: LGTM!Conditional compilation correctly gates test-only utilities. The
pub(crate)visibility forMETADATA_FILE_NAMESis appropriate.
30-53: LGTM!Tests are well-structured with parameterised
rstestcases covering both positive and negative scenarios. Good inclusion of edge cases like the empty string.
14-28: No issues identified. The doc example import path is correct—is_metadata_fileis re-exported from thedaemonmodule at line 23 ofdaemon.rs. The example assertions are also accurate: "version" is inMETADATA_FILE_NAMES, and "0001" is not. The doctest will pass.test-support/src/workflow.rs (4)
21-24: LGTM: New constants follow the established pattern.The new
UPLOAD_RELEASE_ASSETS_PREFIXandEXPECTED_UPLOAD_RELEASE_ASSETSconstants mirror the existing builder constants appropriately. The#[cfg(test)]gating onEXPECTED_UPLOAD_RELEASE_ASSETScorrectly restricts it to test builds only.Also applies to: 32-37
71-76: LGTM: Publisher detection logic correctly mirrors builder detection.The pattern of
strip_prefixfollowed byis_some_andfor commit verification is consistent with the existing approach at lines 65-70.
86-106: LGTM: Tests updated to verify both builder and publisher actions.The
detects_shared_actionstest now validates that both action references end with the expected commit hash and that the function correctly detects workflows containing both actions.
109-165: LGTM: Negative test cases appropriately updated.The
missing_builder_fails,mismatched_builder_commit_fails, andunpinned_builder_failstests now include the publisher action where appropriate, ensuring they test the intended failure conditions in isolation..github/workflows/release.yml (4)
3-40: LGTM: Reusable workflow configuration is well-structured.The
workflow_calltrigger withpublishanddry-runinputs enables flexible reuse. The concurrency group keyed ongithub.refwithcancel-in-progress: truecorrectly prevents parallel runs for the same tag.
43-62: LGTM: Metadata job centralises version and mode computation.Extracting version reading and release mode determination into a dedicated job ensures consistent values propagate to downstream jobs.
131-142: LGTM: Draft release creation with idempotent check.The
gh release view ... || gh release createpattern correctly ensures a draft release exists without failing if one already exists. The--verify-tagflag adds useful validation.
140-142: Remove redundantGITHUB_TOKENfrom thegh release createstep; verify token requirements for custom action.Lines 140-142 set both tokens for the
gh release createcommand, which uses onlyGH_TOKEN. Remove the redundantGITHUB_TOKENhere.For lines 154-156 and 164-166, the
leynos/shared-actionsrepository is not publicly accessible. Verify with the action's source code whetherupload-release-assetsrequires both tokens or onlyGH_TOKEN. If only one is needed, remove the redundant variable from all three locations.
|
@coderabbitai Have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6466 🏁 Script executed: Length of output: 6579 @leynos, unfortunately, neither issue has been fully resolved: Issue 1: workflow_call inputs not being used ❌ Not resolvedThe
This means callers can set these inputs, but they won't affect behaviour. Issue 2: Missing negative tests for upload-release-assets
|
Pass inputs.dry-run and inputs.publish to the determine-release-modes action so that when release-dry-run.yml invokes the workflow with dry-run: true, the action respects that override instead of relying solely on event context. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace string comparison `== 'true'` with `fromJSON()` for consistent boolean parsing across all workflow conditions - Combine sequential upload error checks into a single step that reports all errors before failing, rather than terminating on the first failure 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 38-40: The workflow sets concurrency with group: release-${{
github.ref }} and cancel-in-progress: true which can abort tag-triggered release
runs mid-upload; change cancel-in-progress to false or narrow the concurrency
group (e.g., include the tag name or a more specific identifier) so that release
workflows (the concurrency block using group and cancel-in-progress) are not
cancelled while performing multi-step uploads and can complete fully.
- Around line 170-186: The "Check upload errors" step interpolates outputs like
steps.upload_comenq.outputs.error-message directly into the run script which
risks shell injection; instead export those outputs as environment variables
(e.g., ERROR_MSG_COMENQ and ERROR_MSG_COMENQD and flags
UPLOAD_ERROR_COMENQ/UPLOAD_ERROR_COMENQD) via the step's env: block, then
reference them inside the script as "$ERROR_MSG_COMENQ" and "$ERROR_MSG_COMENQD"
(and test "$UPLOAD_ERROR_COMENQ" == "true") and use printf '%s\n'
"$ERROR_MSG_COMENQ" to safely print the messages. Ensure you update the step
named "Check upload errors" to use these env names and preserve the has_error
logic and exit 1 behavior.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml
🧰 Additional context used
🔍 Remote MCP Deepwiki
Based on my search of the comenq repository, here's the relevant context for reviewing this PR:
Summary of Relevant Repository Context
Project Overview
The comenq system is a fault-tolerant GitHub comment management service implemented in Rust that enables automated posting of GitHub Pull Request comments with mandatory rate limiting. The system consists of a comenq CLI client and a comenqd daemon that manages persistent queuing.
Binary Structure
The comenq binary acts as a command-line interface tool that parses user input and connects to the comenqd daemon via a Unix Domain Socket, while comenqd is a long-running background process responsible for managing a persistent job queue and interacting with the GitHub API.
Current Release Process
The release process is automated and triggered by pushing a semantic version tag to the repository, using GitHub Actions and GoReleaser to build, package, and distribute binaries for various platforms. The workflow uses goreleaser/goreleaser-action@v5 which handles building binaries, creating archives, generating native packages (RPM/DEB) for Linux, and creating Homebrew formulas for macOS.
Key Context for PR Review
Workflow Inputs vs. Determine-Release-Modes Action
The PR introduces workflow_call inputs (publish and dry-run) to the reusable release workflow, but according to the reviewer's concerns in the PR objectives, these inputs are defined but not being passed to the determine-release-modes action—this is a critical gap since callers of the workflow (like the new release-dry-run.yml) can set these inputs without affecting behavior.
Testing Coverage
The test suite includes comprehensive BDD tests with dedicated World objects like ReleaseWorld that validate aspects of the release workflow, but the PR reviewer notes that negative tests for the new upload-release-assets action detection are missing (mismatched commit and unpinned ref scenarios).
Packaging and Service Integration
The package includes systemd integration for Linux with security hardening, automated user/group creation, and service file installation, which is relevant for understanding the full release artifact scope.,,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: release / Build comenqd for aarch64-unknown-linux-gnu
- GitHub Check: release / Build comenqd for x86_64-unknown-linux-gnu
- GitHub Check: build-test
- GitHub Check: Sourcery review
🔇 Additional comments (4)
.github/workflows/release.yml (4)
54-64: Inputs correctly wired todetermine-release-modes.The
dry-runandpublishinputs are now properly forwarded to thedetermine-release-modesaction, and thefromJSON()usage for boolean parsing incheck-tagis appropriate. This addresses the reviewer's earlier concern about inputs being defined but not referenced.
134-145: Draft release creation logic is sound.The
gh release view || gh release createpattern handles the idempotent creation of draft releases appropriately. Setting bothGITHUB_TOKENandGH_TOKENis slightly redundant (theghCLI prefersGH_TOKEN), but it ensures compatibility and causes no harm.
150-169: Asset upload steps are well-structured.Separating uploads for
comenqandcomenqdwith distinct step IDs enables granular error tracking. The shared action is correctly pinned to a specific commit for reproducibility.
95-107: Remove this concern.The
rust-build-releaseaction does not accept aversioninput parameter. Its inputs are limited totarget,project-dir,manifest-path, andbin-name. Since the action reads build metadata directly from theCargo.tomlfile specified viamanifest-path, passing an explicit version parameter would be both unnecessary and impossible (the action would reject an unknown input).Likely an incorrect or invalid review comment.
- Set cancel-in-progress to false so release workflows complete fully rather than being cancelled mid-upload when a new workflow starts - Fix shell injection risk in upload error checking by exporting step outputs as environment variables and referencing them safely within the script 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Add tests to verify uses_shared_release_actions returns false when the upload-release-assets action is pinned to a different commit SHA or uses a tag reference instead of a commit SHA. - mismatched_publisher_commit_fails: verifies detection of wrong SHA - unpinned_publisher_fails: verifies detection of tag reference (@v1) These tests are analogous to the existing mismatched_builder_commit_fails and unpinned_builder_fails tests for the rust-build-release action. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test-support/src/workflow.rs`:
- Around line 167-195: The four near-identical tests
mismatched_builder_commit_fails, unpinned_builder_fails,
mismatched_publisher_commit_fails, and unpinned_publisher_fails should be
consolidated using rstest parameterisation to remove duplication; replace the
four separate #[test] functions with a single #[rstest] function that takes
parameters for the step string (builder vs publisher variant) and an expected
boolean, call uses_shared_release_actions(&yaml).expect("parse") inside the
parametrised test, and reference the EXPECTED_RUST_BUILDER constant and the
uses_shared_release_actions function to build each yaml case; ensure the test
attributes map the four cases to the same assertion logic.
Replace four near-identical test functions (mismatched_builder_commit_fails, unpinned_builder_fails, mismatched_publisher_commit_fails, unpinned_publisher_fails) with a single parameterised rstest function that covers all four cases. - Add rstest as dev-dependency to test-support crate - Use #[case] attributes to define each invalid pinning scenario - Reduce test code duplication while maintaining full coverage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test-support/src/workflow.rs`:
- Around line 138-166: The test function invalid_action_pinning_fails uses
.expect("parse") but is missing the lint-expect attribute; add
#[expect(clippy::expect_used, reason = "simplify test output")] immediately
after the existing #[rstest] attribute above fn invalid_action_pinning_fails to
match other tests and silence the clippy expectation for the .expect call.
1 participant
Summary
Changes
Workflows
Packaging & Versioning
Code & Tests
Why
Testing plan
Notes
📎 Task: https://www.terragonlabs.com/task/30929c96-3b12-4ade-b223-2d92ca677aa1