Refactor release workflow to use shared reusable actions#253
Conversation
- Replace local custom actions in release and build workflows with pinned versions from `leynos/shared-actions` repository to ensure reproducibility and maintainability. - Update staging workflow to use a generic `stage-release-artefacts` composite action with improved Windows path handling. - Refactor metadata extraction and release mode determination to shared composites to reduce code duplication. - Update documentation to reflect changes in release automation tooling. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
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. WalkthroughConsolidate local GitHub Actions and Python staging/upload helpers into external shared actions; remove local composite actions, CLI scripts, staging library modules and many related tests; update workflows to reference leynos/shared-actions and rename stage outputs/inputs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Reviewer's GuideRefactors release and packaging workflows to consume pinned reusable actions from leynos/shared-actions for staging, metadata, and release-mode handling, while deleting the local Python-based staging/upload toolchain and aligning docs and workflow outputs with the shared composites. Sequence diagram for release metadata job using shared actionssequenceDiagram
participant GH as GitHub_Actions
participant REL as workflow_release
participant META as job_metadata
participant DRM as determine_release_modes
participant ECV as ensure_cargo_version
participant ECM as export_cargo_metadata
GH->>REL: trigger on release_tag
REL->>META: start job_metadata
META->>DRM: uses determine_release_modes
DRM-->>META: outputs should-publish, dry-run, should-upload-workflow-artifacts
META->>ECV: uses ensure_cargo_version
Note over META,ECV: input check-tag = fromJSON(release_modes.outputs['should-publish'])
ECV-->>META: output crate-version
META->>ECM: uses export_cargo_metadata
Note over META,ECM: inputs manifest-path, fields=bin-name, export-to-env=false
ECM-->>META: output bin-name
META-->>REL: set job outputs
REL-->>REL: expose version, bin_name, should_publish, dry_run, should_upload_workflow_artifacts
Flow diagram for build_and_package job using shared stage_release_artefactsgraph TD
A[Start_job_build_and_package] --> B[Setup_Rust_and_Python]
B --> C[Build_release_binary_using_rust_build_release]
C --> D{Platform}
D -->|linux| E_linux[Package_with_linux_packages]
E_linux --> Z[End_job]
D -->|windows| S_stage_win[Stage_artefacts_using_stage_release_artefacts]
S_stage_win --> S1[Shared_action_normalizes_windows_paths]
S1 --> F_win[Build_Windows_MSI_using_windows_package]
F_win --> G_win[Upload_Windows_artefacts]
G_win --> Z
D -->|macos| S_stage_mac[Stage_artefacts_using_stage_release_artefacts]
S_stage_mac --> F_mac[Build_macOS_pkg_using_macos_package]
F_mac --> G_mac[Upload_macOS_artefacts]
G_mac --> Z
subgraph WindowsOutputs
S_stage_win --> O1_stage_bin[binary-path]
S_stage_win --> O2_stage_license[license-path]
F_win --> O3_msi_path[msi-path]
end
subgraph MacOSOutputs
S_stage_mac --> O4_stage_bin[binary-path]
S_stage_mac --> O5_stage_man[man-path]
end
subgraph SharedUploadInputs
S_stage_win --> O6_stage_dir_win[artifact-dir]
S_stage_mac --> O7_stage_dir_mac[artifact-dir]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This commit deletes the entire Python-based staging helper under .github/actions/stage and the upload-release-assets action and related scripts and tests. It also updates workflow YAMLs to remove references to these deleted actions, replacing them with updated composite actions. The staging logic implemented in Python has been fully removed, including configuration loading, artifact staging, checksum generation, environment helpers, output formatting, path resolution, and tests. Additionally, scripts for reading Cargo manifests, normalising Windows paths, uploading release assets, determining release modes, and their tests have been deleted. Workflows now reference updated actions via explicit SHA revisions, reflecting a migration away from the removed Python-based components. This refactor cleans up the repository by removing obsolete staging helper code and streamlining CI workflows with maintained action releases. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Updated the GitHub workflow files to use a newer commit hash (6cae1ffdbb88) for all leynos/shared-actions references, replacing the previous commit hash (9b84b9fda7ac). This ensures consistent action versions across build-and-package.yml, ci.yml, netsukefile-test.yml, and release.yml workflows. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Updated several GitHub workflow action references from commit 6cae1ffdbb88 to f870d9959d5f039d5b705856428103ea52d28d2f to use latest shared-actions versions in CI, build, package, and release workflows. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Updated all leynos/shared-actions usages in GitHub workflows to commit 1e80300db7c46478d398deae4f48c3d0e61ac571 to ensure latest action versions are used. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Updated multiple GitHub workflow files to reference the latest commit SHA of shared-actions repository, ensuring use of up-to-date CI and release automation steps. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.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:
- Double-check the
upload-artefactinput name in thewindows-packagestep matches the shared action’s expected input (previouslyupload-artifact), and consider standardisingartifact/artefactspelling across workflow inputs/outputs to avoid subtle wiring mistakes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double-check the `upload-artefact` input name in the `windows-package` step matches the shared action’s expected input (previously `upload-artifact`), and consider standardising `artifact/artefact` spelling across workflow inputs/outputs to avoid subtle wiring mistakes.
## Individual Comments
### Comment 1
<location> `.github/workflows/release.yml:63` </location>
<code_context>
- should_publish: ${{ steps.release_modes.outputs.should_publish }}
- dry_run: ${{ steps.release_modes.outputs.dry_run }}
- should_upload_workflow_artifacts: ${{ steps.release_modes.outputs.should_upload_workflow_artifacts }}
+ bin_name: ${{ steps.bin_name.outputs['bin-name'] }}
+ should_publish: ${{ steps.release_modes.outputs['should-publish'] }}
+ dry_run: ${{ steps.release_modes.outputs['dry-run'] }}
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests that exercise the new shared-actions-based metadata and release-mode logic introduced in this workflow.
You replace in-repo Python scripts and custom actions with shared actions (`determine-release-modes`, `ensure-cargo-version`, `export-cargo-metadata`, and `upload-release-assets`) while also changing output names and wiring (e.g. `bin-name`, `should-publish`, `dry-run`). This is a change to the existing release feature, but the diff does not introduce any new tests that validate the updated behaviour (e.g. correct `should_publish`/`dry_run` resolution, tag checking, bin name extraction, or asset upload wiring). Add both behavioural tests (end-to-end or high-level integration checks around the release process) and unit-level tests (covering the logic that you still own and can exercise, e.g. wrapper scripts or configuration validation) to demonstrate that the new workflow behaviour is correct.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 2
<location> `.github/workflows/build-and-package.yml:111` </location>
<code_context>
name: Determine repository name
run: echo "value=${GITHUB_REPOSITORY#*/}" >>"$GITHUB_OUTPUT"
- - name: Setup Python 3.11
- uses: >-
- actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065
- # v5
</code_context>
<issue_to_address>
**issue (review_instructions):** Add tests that validate the new staging and packaging behaviour now delegated to shared actions and the updated output/parameter wiring.
You change the implementation of the build-and-package feature by swapping the local `stage` composite and helper scripts for the shared `stage-release-artefacts`, `windows-package`, `macos-package`, and updated `rust-build-release`/`linux-packages` actions, and you rename or rewire multiple outputs/inputs (e.g. `binary-path`, `man-path`, `artifact-dir`, `upload-artefact`). This materially changes how artefacts are staged and installers are produced, yet the diff shows no new behavioural or unit tests that assert the new behaviour. Add behavioural tests (for example, a workflow or script-driven test that builds and stages artefacts and verifies expected outputs/paths) and unit tests for any remaining in-repo logic that participates in this pipeline to ensure the feature still behaves as intended.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/netsuke-design.md (1)
2121-2135: Expand acronym: PEP.Line 2124 introduces "PEP 723" without expanding it. Expand "Python Enhancement Proposal" on first use.
🔎 Proposed acronym expansion
- macOS releases execute the shared action twice: once on an Intel runner and - again on Apple Silicon. The same composite action interprets the TOML - configuration, emits checksums, and exposes artefact metadata via JSON outputs - before feeding the resulting paths into the `macos-package` action. Embedding - the PEP 723 metadata keeps Cyclopts discoverable without a repository-level + macOS releases execute the shared action twice: once on an Intel runner and + again on Apple Silicon. The same composite action interprets the TOML + (Tom's Obvious, Minimal Language) configuration, emits checksums, and exposes + artefact metadata via JSON outputs before feeding the resulting paths into the + `macos-package` action. Embedding the PEP (Python Enhancement Proposal) 723 + metadata keeps Cyclopts discoverable without a repository-level
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (46)
.github/actions/export-bin-name/action.yml.github/actions/stage/action.yml.github/actions/stage/scripts/stage.py.github/actions/stage/scripts/stage_common/__init__.py.github/actions/stage/scripts/stage_common/checksum_utils.py.github/actions/stage/scripts/stage_common/config.py.github/actions/stage/scripts/stage_common/environment.py.github/actions/stage/scripts/stage_common/errors.py.github/actions/stage/scripts/stage_common/fs_utils.py.github/actions/stage/scripts/stage_common/github_output.py.github/actions/stage/scripts/stage_common/glob_utils.py.github/actions/stage/scripts/stage_common/staging/__init__.py.github/actions/stage/scripts/stage_common/staging/output.py.github/actions/stage/scripts/stage_common/staging/pipeline.py.github/actions/stage/scripts/stage_common/staging/resolution.py.github/actions/stage/scripts/stage_common/template_utils.py.github/actions/upload-release-assets/action.yml.github/workflows/build-and-package.yml.github/workflows/ci.yml.github/workflows/netsukefile-test.yml.github/workflows/release.yml.github/workflows/scripts/determine_release_modes.py.github/workflows/scripts/normalise_windows_paths.py.github/workflows/scripts/read_manifest.pyMakefilecodex/refactor-python-staging-scripts-into-github-actiondocs/netsuke-design.mdscripts/_release_upload_deps.pyscripts/tests/test_upload_release_assets.pyscripts/upload_release_assets.pytests_python/conftest.pytests_python/stage_test_helpers.pytests_python/test_normalise_windows_paths.pytests_python/test_read_manifest.pytests_python/test_release_modes.pytests_python/test_stage_action.pytests_python/test_stage_artefacts.pytests_python/test_stage_cli.pytests_python/test_stage_common_config.pytests_python/test_stage_common_module.pytests_python/test_stage_common_staging.pytests_python/test_stage_config.pytests_python/test_stage_glob.pytests_python/test_stage_helpers.pytests_python/test_stage_integration.pytests_python/test_stage_output.py
💤 Files with no reviewable changes (40)
- scripts/tests/test_upload_release_assets.py
- .github/actions/stage/scripts/stage_common/errors.py
- tests_python/test_stage_helpers.py
- .github/actions/stage/scripts/stage_common/fs_utils.py
- tests_python/test_stage_artefacts.py
- .github/actions/stage/scripts/stage_common/init.py
- .github/actions/stage/scripts/stage_common/checksum_utils.py
- .github/workflows/scripts/normalise_windows_paths.py
- tests_python/test_stage_common_module.py
- .github/actions/stage/scripts/stage.py
- .github/actions/export-bin-name/action.yml
- .github/actions/stage/action.yml
- tests_python/test_normalise_windows_paths.py
- tests_python/test_stage_action.py
- tests_python/stage_test_helpers.py
- scripts/upload_release_assets.py
- .github/actions/upload-release-assets/action.yml
- .github/actions/stage/scripts/stage_common/config.py
- .github/actions/stage/scripts/stage_common/environment.py
- .github/actions/stage/scripts/stage_common/glob_utils.py
- tests_python/test_stage_glob.py
- codex/refactor-python-staging-scripts-into-github-action
- tests_python/test_release_modes.py
- .github/actions/stage/scripts/stage_common/staging/resolution.py
- tests_python/test_stage_common_staging.py
- tests_python/test_stage_output.py
- tests_python/test_stage_cli.py
- .github/workflows/scripts/read_manifest.py
- .github/workflows/scripts/determine_release_modes.py
- tests_python/test_stage_common_config.py
- tests_python/test_read_manifest.py
- .github/actions/stage/scripts/stage_common/template_utils.py
- .github/actions/stage/scripts/stage_common/staging/init.py
- .github/actions/stage/scripts/stage_common/staging/output.py
- .github/actions/stage/scripts/stage_common/github_output.py
- .github/actions/stage/scripts/stage_common/staging/pipeline.py
- scripts/_release_upload_deps.py
- tests_python/test_stage_integration.py
- tests_python/test_stage_config.py
- tests_python/conftest.py
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/netsuke-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/netsuke-design.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/netsuke-design.md
⏰ 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). (8)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (12)
Makefile (1)
1-1: Verify no orphaned references to the removed typecheck target.The removal aligns with eliminating Python tooling from the repository. Ensure that CI workflows and documentation do not invoke
make typecheck.Run the following script to verify no references remain:
#!/bin/bash # Description: Search for references to the removed typecheck target. # Search GitHub Actions workflows echo "=== Checking GitHub Actions workflows ===" fd -e yml -e yaml . .github/workflows/ --exec rg -n 'make\s+typecheck' # Search documentation echo "=== Checking documentation ===" fd -e md . --exec rg -n 'make\s+typecheck|`typecheck`' # Search for any other Makefile references echo "=== Checking for typecheck references in all files ===" rg -n '\btypecheck\b' -g '!Makefile'.github/workflows/netsukefile-test.yml (1)
24-24: Verify the updated SHA is valid and consistent across workflows.The SHA update aligns with the PR's goal of keeping shared-actions references current.
Run the following script to verify the new SHA exists in leynos/shared-actions and that setup-rust usage remains compatible:
#!/bin/bash # Description: Verify the new shared-actions SHA exists and check consistency across workflows. NEW_SHA="df81280dcc1d6e66134114dbc924313328b15f05" # Verify SHA exists in leynos/shared-actions repository echo "Checking if SHA exists in leynos/shared-actions..." gh api repos/leynos/shared-actions/commits/$NEW_SHA --jq '.sha' || echo "ERROR: SHA not found" # Check if setup-rust action exists at this commit echo -e "\nChecking setup-rust action at new SHA..." gh api repos/leynos/shared-actions/contents/.github/actions/setup-rust/action.yml?ref=$NEW_SHA --jq '.name' || echo "ERROR: Action not found at SHA" # Verify consistency: check if this SHA is used consistently across all workflows echo -e "\nSearching for any other shared-actions SHA references in workflows..." rg -n "leynos/shared-actions.*@[a-f0-9]{40}" .github/workflows/ | grep -v "$NEW_SHA" || echo "All references use the new SHA".github/workflows/ci.yml (1)
24-24: Consistent SHA pinning across all shared actions is good practice.All three action references (setup-rust, generate-coverage, upload-codescene-coverage) have been updated to the same commit SHA (
df81280d...), which ensures consistency and aligns with the PR's goal to centralise and pin shared actions for reproducibility. Using explicit commit SHAs is a security best practice for GitHub Actions as it prevents supply chain attacks.Verify that the new SHA exists in the
leynos/shared-actionsrepository and that the actions at this commit are compatible with the current inputs and usage.#!/bin/bash # Description: Verify the new shared-actions commit exists and inspect changes. # Check that the commit exists in leynos/shared-actions echo "Checking if commit df81280dcc1d6e66134114dbc924313328b15f05 exists..." gh api repos/leynos/shared-actions/commits/df81280dcc1d6e66134114dbc924313328b15f05 \ --jq '{sha: .sha, author: .commit.author.name, date: .commit.author.date, message: .commit.message}' \ 2>/dev/null || echo "ERROR: Commit not found in leynos/shared-actions" # Compare the old and new commits to check for breaking changes echo -e "\n--- Comparing old (dd56f18c) vs new (df81280d) commits ---" gh api repos/leynos/shared-actions/compare/dd56f18c...df81280d \ --jq '{status: .status, ahead_by: .ahead_by, behind_by: .behind_by, total_commits: .total_commits, files_changed: [.files[].filename]}' \ 2>/dev/null || echo "ERROR: Unable to compare commits"Also applies to: 43-43, 51-51
.github/workflows/release.yml (4)
63-66: LGTM: Output mappings correctly use bracket notation.The dictionary-style output access with hyphenated keys is the correct GitHub Actions syntax for retrieving outputs from the shared actions.
78-79: Verify the shared-actions repository and pinned commit SHA.Ensure that:
- The repository
leynos/shared-actionsis accessible from GitHub Actions runners- The commit
df81280dcc1d6e66134114dbc924313328b15f05exists and contains the referenced actions- The action inputs/outputs at that SHA match the usage in this workflow
#!/bin/bash # Verify shared-actions repository and commit SHA exist set -euo pipefail REPO="leynos/shared-actions" SHA="df81280dcc1d6e66134114dbc924313328b15f05" echo "Checking if repository ${REPO} is accessible..." gh repo view "${REPO}" --json nameWithOwner,isPrivate echo -e "\nVerifying commit ${SHA} exists..." gh api "repos/${REPO}/commits/${SHA}" --jq '.sha, .commit.message' echo -e "\nChecking for action files at this commit..." ACTIONS=( ".github/actions/determine-release-modes/action.yml" ".github/actions/ensure-cargo-version/action.yml" ".github/actions/export-cargo-metadata/action.yml" ".github/actions/upload-release-assets/action.yml" ) for action in "${ACTIONS[@]}"; do echo "Checking ${action}..." gh api "repos/${REPO}/contents/${action}?ref=${SHA}" --jq '.name, .type' || echo " ❌ Not found" doneAlso applies to: 83-83, 102-103, 216-217
85-85: LGTM: Correct use of fromJSON for boolean conversion.The
fromJSONfunction properly converts the string output from the shared action to a boolean value for thecheck-taginput.
105-105: Verify CARGO_TOML_PATH is defined in the repository.Check whether
CARGO_TOML_PATHis set as a repository variable, secret, or defined elsewhere in the workflows. If it's not used, simply remove the reference and use the default path.#!/bin/bash # Search for CARGO_TOML_PATH usage and definition set -euo pipefail echo "Searching for CARGO_TOML_PATH in workflow files..." rg -n 'CARGO_TOML_PATH' -g '*.yml' -g '*.yaml' -C 3 echo -e "\nSearching for CARGO_TOML_PATH in environment files..." rg -n 'CARGO_TOML_PATH' -g '.env*' -C 2 || echo "No .env files found with CARGO_TOML_PATH" echo -e "\nChecking repository variables..." gh api "repos/${GITHUB_REPOSITORY}/actions/variables" --jq '.variables[] | select(.name == "CARGO_TOML_PATH")' echo -e "\nNote: Cannot check secrets directly, but if CARGO_TOML_PATH is not found above, consider removing the reference.".github/workflows/build-and-package.yml (5)
82-82: Verify the shared-actions repository and pinned commit exist.The workflow now depends on external actions from
leynos/shared-actions. Confirm that the repository is accessible and the commitdf81280dcc1d6e66134114dbc924313328b15f05exists with the expected action definitions.#!/bin/bash # Description: Verify leynos/shared-actions repository and commit validity set -euo pipefail REPO="leynos/shared-actions" SHA="df81280dcc1d6e66134114dbc924313328b15f05" echo "Checking if repository ${REPO} exists..." gh repo view "${REPO}" --json name,isArchived,isFork,visibility || { echo "ERROR: Repository ${REPO} not found or inaccessible" exit 1 } echo "Checking if commit ${SHA} exists..." gh api "repos/${REPO}/commits/${SHA}" --jq '.sha' || { echo "ERROR: Commit ${SHA} not found in ${REPO}" exit 1 } echo "Verifying action paths exist at ${SHA}..." ACTIONS=( ".github/actions/rust-build-release/action.yml" ".github/actions/linux-packages/action.yml" ".github/actions/stage-release-artefacts/action.yml" ".github/actions/windows-package/action.yml" ".github/actions/macos-package/action.yml" ) for action in "${ACTIONS[@]}"; do echo "Checking ${action}..." gh api "repos/${REPO}/contents/${action}?ref=${SHA}" --jq '.name' || { echo "ERROR: Action ${action} not found at ${SHA}" exit 1 } done echo "All actions verified successfully"Also applies to: 90-90, 112-112, 121-121, 139-139
108-116: Verify thestage-release-artefactsaction interface.The
normalize-windows-pathsinput is newly introduced and depends on the shared action's implementation. Confirm that the action at the pinned SHA accepts this input and that the output names (binary-path,license-path,man-path,artifact-dir) match the expected interface.#!/bin/bash # Description: Examine stage-release-artefacts action definition set -euo pipefail REPO="leynos/shared-actions" SHA="df81280dcc1d6e66134114dbc924313328b15f05" ACTION_PATH=".github/actions/stage-release-artefacts/action.yml" echo "Fetching action definition..." gh api "repos/${REPO}/contents/${ACTION_PATH}?ref=${SHA}" \ --jq '.content' | base64 -d > /tmp/stage-action.yml echo "Checking for normalize-windows-paths input..." grep -q "normalize-windows-paths" /tmp/stage-action.yml || { echo "ERROR: normalize-windows-paths input not found" exit 1 } echo "Checking for required outputs..." REQUIRED_OUTPUTS=("binary-path" "license-path" "man-path" "artifact-dir") for output in "${REQUIRED_OUTPUTS[@]}"; do grep -q "${output}:" /tmp/stage-action.yml || { echo "ERROR: Output ${output} not found in action definition" exit 1 } done echo "Action interface verified successfully" cat /tmp/stage-action.yml
126-127: Output references correctly updated to kebab-case format.All stage output references use the new hyphenated naming convention consistently. The bracket notation with quotes (
steps.stage.outputs['binary-path']) is the correct syntax for accessing hyphenated output keys.Note: Line 127 maps
license-pathoutput tolicense-rtf-pathinput, implying the staged license file is already in RTF format. Verify that the staging action produces RTF-formatted licences for Windows.Also applies to: 143-144, 176-176, 186-186
133-133: Spelling aligns with British English conventions.The rename from
upload-artifacttoupload-artefactadopts British English spelling, consistent with the repository's nomenclature (e.g., "artefacts" in job names, output descriptions, and step names throughout the workflow).
108-187: Migration to shared staging action is complete and logically sound.The refactoring correctly:
- Preserves the Linux direct-packaging flow (bypassing staging)
- Migrates Windows and macOS to use the shared staging action
- Updates all output references to the new kebab-case format
- Maintains conditional upload logic based on platform and workflow inputs
The workflow should function identically to the previous implementation, assuming the shared actions provide the expected interfaces.
![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: f9bb5473b4
ℹ️ 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".
…sistency Add new Rust-based tests to verify the wiring and behavior of the build-and-package, release, and shared-actions pinning workflows. These tests ensure workflows use shared actions correctly and maintain consistent action version pins to prevent drift. Also improve release workflow outputs verification and enhance robustness of workflow configuration validation. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/release.yml (1)
100-114: Approve the manifest path resolution; refine the step name.The
manifest_pathstep correctly works around GitHub Actions' lack of a||operator, and the subsequent use ofsteps.manifest_path.outputs.valueis the proper pattern.The step name "Export Cargo bin name" on line 108 is an improvement over the previous "Export Cargo metadata", but it remains slightly imprecise. The action extracts the binary name rather than exporting general metadata. Align the step name with the action's specific purpose.
🔎 Suggested refinement
- id: bin_name - name: Export Cargo bin name + name: Extract binary name from Cargo.toml uses: >- leynos/shared-actions/.github/actions/export-cargo-metadata@df81280dcc1d6e66134114dbc924313328b15f05tests/workflow_build_and_package.rs (1)
6-15: Extract duplicated helper to shared test utilities.This function is identical to
workflow_contentsintests/workflow_release.rs(lines 6-15). See the review comment on that file for the recommended refactor to extract this helper to a shared test utilities module.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release.ymldocs/netsuke-design.mdtests/workflow_build_and_package.rstests/workflow_release.rstests/workflow_shared_actions_pins.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/workflow_build_and_package.rstests/workflow_shared_actions_pins.rstests/workflow_release.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:
tests/workflow_build_and_package.rstests/workflow_shared_actions_pins.rstests/workflow_release.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/netsuke-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/netsuke-design.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/netsuke-design.md
🧠 Learnings (2)
📚 Learning: 2025-12-29T19:00:20.890Z
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Applied to files:
tests/workflow_build_and_package.rstests/workflow_shared_actions_pins.rstests/workflow_release.rs
📚 Learning: 2026-01-03T20:51:29.799Z
Learnt from: leynos
Repo: leynos/netsuke PR: 253
File: .github/workflows/build-and-package.yml:82-82
Timestamp: 2026-01-03T20:51:29.799Z
Learning: In GitHub Actions workflows, the uses: value must be a static string (no runtime interpolation). Do not use expressions like ${{ env.VARIABLE }} or ${{ vars.VARIABLE }} inside uses:. This ensures deterministic action references and supports policy enforcement. If you need to parameterize actions, use inputs or job-level variables, not dynamic action references.
Applied to files:
.github/workflows/release.yml
🧬 Code graph analysis (2)
tests/workflow_build_and_package.rs (1)
tests/workflow_release.rs (1)
workflow_contents(6-15)
tests/workflow_release.rs (1)
tests/workflow_build_and_package.rs (1)
workflow_contents(6-15)
🔍 Remote MCP
Let me search for more specific information about the terragon project and the specific actions being used:Based on the comprehensive pull request context provided, I can now offer a summary of relevant facts that would help in reviewing this PR:
Relevant Context for PR #253 Review
Architecture & Pattern
This PR implements a common GitHub Actions best practice: centralizing custom actions by storing them in a separate repository and calling them from other repositories, allowing actions and reusable workflows to be shared with organizations without publishing them publicly.
Scope of Changes
The refactoring consolidates local implementation logic into external shared actions from leynos/shared-actions. Key removals include:
-
Local GitHub Actions: Three composite actions removed:
export-bin-name: Derived binary names from Cargo.toml via inline Pythonstage(composite): Coordinated artifact staging with TOML configurationupload-release-assets: Discovered and validated release artifacts for upload
-
Python Helper Scripts: Complete removal of workflow orchestration scripts:
determine_release_modes.py: Release mode logic (dry-run, publish flags)normalise_windows_paths.py: Windows path normalizationread_manifest.py: Cargo manifest field extractionupload_release_assets.py: Release artifact upload CLI
-
Staging Pipeline Package: Entire
stage_commonPython package removed, including:- Configuration loaders and validators
- Artifact resolution and templating utilities
- GitHub Actions output formatting
- Filesystem safety checks and glob utilities
- Checksum computation
-
Tests: Corresponding test files deleted (~15 test modules covering configuration, staging, output formatting, and Windows path normalization)
Workflow Updates
build-and-package.yml:
- Replaces local staging with
leynos/shared-actions/.github/actions/stage-release-artefacts - Removes separate Windows path normalization step
- Updates output references to use new naming convention (e.g.,
binary_path→binary-path)
release.yml:
- Replaces Python helpers with shared actions:
determine-release-modes(release mode detection)ensure-cargo-version(version management)export-cargo-metadata(manifest introspection, replacesread_manifest.py)upload-release-assets(replaces local Python script)
- Removes
Setup Python 3.11step previously used for Python helpers
CI Workflows (ci.yml, netsukefile-test.yml):
- Updates action references to pinned shared-actions commits for consistency
Testing & Validation
Three new Rust-based test files validate the refactoring:
tests/workflow_build_and_package.rs: Verifies build-and-package workflow wiring (stage action outputs, path references)tests/workflow_release.rs: Validates release workflow uses shared actions and exports correct fieldstests/workflow_shared_actions_pins.rs: Enforces consistent action pinning across all workflows
Risk Considerations for Review
-
Dependency on External Repository: All release and packaging logic now depends on
leynos/shared-actionsbeing accessible and maintained. Pinned SHAs reduce flexibility but ensure reproducibility. -
Behavioral Equivalence: The PR claims no intended functional behavior changes—shared actions should replicate local implementations exactly. Verify:
- Output field names and formats match (especially
binary-path,man-path,artifact-dir) - Windows path normalization behavior is equivalent
- Artifact discovery and checksumming logic remains consistent
- Output field names and formats match (especially
-
Removed Python Dependencies: No longer requires Python environment for workflows (except shared actions internally). Makefile's
typechecktarget removed—verify type checking is handled elsewhere if needed. -
Integration Points: Windows packaging and packaging step inputs now depend on shared action outputs; verify all downstream references are correctly updated.
[::web_search::] for shared actions pattern documentation
⏰ 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). (9)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, 1.89.0)
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: build-test (ubuntu-latest, nightly, true)
🔇 Additional comments (8)
docs/netsuke-design.md (1)
2051-2137: Release automation sections are well-structured and documentation guidelines are met.The section properly expands all acronyms on first use (Tom's Obvious, Minimal Language (TOML), Astral's Python package manager (uv), Windows Installer XML (WiX), Rich Text Format (RTF), Microsoft Installer (MSI)), maintains consistent 80-column paragraph wrapping, and uses en-GB spelling throughout ("artefacts", "licence"). The technical narrative of the shared-actions refactoring, staging pipeline, and cross-platform packaging flows is clear and accurate.
.github/workflows/release.yml (2)
63-66: LGTM: Dictionary-key access pattern correctly applied.The migration to dictionary-key access for action outputs (
steps.bin_name.outputs['bin-name'],steps.release_modes.outputs['should-publish'], etc.) follows the correct GitHub Actions pattern. ThefromJSONconversion on line 85 properly transforms the string output to a boolean for thecheck-taginput.Also applies to: 78-85
223-224: LGTM: Shared upload action properly integrated.The migration to
leynos/shared-actions/.github/actions/upload-release-assetsis correctly configured with the necessary inputs (release-tag,bin-name,dist-dir) and environment variables. The SHA pinning ensures reproducibility.tests/workflow_release.rs (1)
17-74: LGTM: Tests correctly validate workflow wiring.The three test functions thoroughly verify that the release workflow uses the shared actions correctly, exports the
bin-namefield, and wires therelease_modesoutputs to downstream steps. The assertion messages provide clear diagnostics when failures occur.tests/workflow_shared_actions_pins.rs (3)
7-18: LGTM: Helper functions appropriately scoped.The
repo_root,workflow_dir, andread_workflowhelpers provide clean abstractions for locating and reading workflow files. Theread_workflowfunction's panic message provides clear diagnostics when file access fails.
20-37: LGTM: Pin extraction logic correctly implemented.The
extract_shared_actions_pinsfunction correctly parses workflow YAML lines to extract commit SHAs following the@delimiter. The use offilter_mapwith appropriate edge-case handling (whitespace trimming, empty value filtering) ensures robust extraction.
39-78: LGTM: Tests enforce consistent pinning policy.The unit test provides regression coverage for the pin extraction logic, whilst the behavioural test enforces that all shared-actions references across workflows use identical commit SHAs. The use of
BTreeSetfor uniqueness checking provides deterministic diagnostics when pins diverge.tests/workflow_build_and_package.rs (1)
17-53: LGTM: Test comprehensively validates workflow integration.The test verifies that
build-and-package.ymlcorrectly integrates with the sharedstage-release-artefactsaction, consumes its outputs (binary-path,license-path,man-path,artifact-dir), and properly gates Windows-specific behaviour. The assertion messages provide clear diagnostics for each integration point.
- Introduce a dedicated step to capture staging outputs as 'stage_paths' - Update build-and-package workflow to use unified 'stage_paths' outputs - Minor naming cleanup in release workflow - Add common test utilities for reading workflow files - Update tests to use common utilities and verify path output usage This improves clarity and maintenance of artifact paths usage across OS platforms in CI. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/build-and-package.yml (1)
111-112: Single-lineuses:is preferred for consistency.The
>-block scalar for theuses:field works but is unconventional compared to other action references in this workflow. Flatten to a single line for consistency.🔎 Proposed simplification
- uses: >- - leynos/shared-actions/.github/actions/stage-release-artefacts@df81280dcc1d6e66134114dbc924313328b15f05 + uses: leynos/shared-actions/.github/actions/stage-release-artefacts@df81280dcc1d6e66134114dbc924313328b15f05
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/build-and-package.yml.github/workflows/release.ymltests/common/mod.rstests/workflow_build_and_package.rstests/workflow_release.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/common/mod.rstests/workflow_release.rstests/workflow_build_and_package.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:
tests/common/mod.rstests/workflow_release.rstests/workflow_build_and_package.rs
🧠 Learnings (2)
📚 Learning: 2025-12-29T19:00:20.890Z
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Applied to files:
tests/common/mod.rstests/workflow_release.rstests/workflow_build_and_package.rs
📚 Learning: 2026-01-03T20:51:29.799Z
Learnt from: leynos
Repo: leynos/netsuke PR: 253
File: .github/workflows/build-and-package.yml:82-82
Timestamp: 2026-01-03T20:51:29.799Z
Learning: In GitHub Actions workflows, the uses: value must be a static string (no runtime interpolation). Do not use expressions like ${{ env.VARIABLE }} or ${{ vars.VARIABLE }} inside uses:. This ensures deterministic action references and supports policy enforcement. If you need to parameterize actions, use inputs or job-level variables, not dynamic action references.
Applied to files:
.github/workflows/release.yml.github/workflows/build-and-package.yml
🧬 Code graph analysis (2)
tests/workflow_release.rs (1)
tests/common/mod.rs (1)
workflow_contents(39-44)
tests/workflow_build_and_package.rs (2)
tests/common/mod.rs (1)
workflow_contents(39-44)src/stdlib/which/lookup/workspace/mod.rs (1)
contains(44-46)
⏰ 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). (9)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, nightly, true)
- GitHub Check: build-test (ubuntu-latest, 1.89.0)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (5)
tests/workflow_build_and_package.rs (1)
1-43: LGTM!The test correctly validates the build-and-package workflow wiring by checking for expected shared action references and output mappings. The helper extraction to
common::workflow_contentsaddresses the prior duplication concern.tests/workflow_release.rs (1)
1-64: LGTM!The tests comprehensively validate the release workflow's shared action wiring, bin-name export flow, and release modes output mapping. The refactored helper usage via
common::workflow_contentseliminates the previously flagged duplication..github/workflows/build-and-package.yml (2)
118-129: Good approach to bridge output naming conventions.The
stage_pathsstep effectively translates from the shared action's hyphenated output names (binary-path,man-path, etc.) to underscore-based names (binary_path,man_path) for consumption by downstream steps. This addresses the naming mismatch concern raised in prior review.
82-82: Ensure all shared action references maintain consistent SHA pinning.All five shared action references correctly use the same SHA
df81280dcc1d6e66134114dbc924313328b15f05:
rust-build-release(line 82)linux-packages(line 90)stage-release-artefacts(line 112)windows-package(line 134)macos-package(line 152)This approach maintains reproducibility across the workflow.
.github/workflows/release.yml (1)
78-79: All shared-actions references consistently pinned to single SHA.Four shared actions reference
df81280dcc1d6e66134114dbc924313328b15f05:
determine-release-modes(line 79)ensure-cargo-version(line 83)export-cargo-metadata(line 110)upload-release-assets(line 224)This consistency is correct. The
uses:field cannot be parameterised, so the duplication is necessary. Workflow wiring is sound.
- Changed workflow_contents to return Result with context on error - Added unit tests to verify reading of release.yml workflow file - Added unit tests for build fixtures in common helpers - Updated multiple test modules to use new workflow_contents signature and add tests - Improves test coverage and error handling in common test utilities Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/runner_tool_subcommands_tests.rs (1)
71-74: Remove duplicate test.This test is identical to the one in
tests/runner_tests.rs(lines 65-68) and should be removed along with that one. See the review comment ontests/runner_tests.rsfor the consolidation proposal.tests/workflow_build_and_package.rs (1)
46-50: Remove unused fixture test.This test exercises fixtures that aren't used by the workflow validation test in this file. See the identical issue and fix proposal in
tests/workflow_release.rs(lines 66-70).🔎 Proposed fix
-#[test] -fn unit_common_helpers_build_fixtures() { - let _fixture = create_test_manifest().expect("test manifest fixture should build"); - let _ninja = ninja_with_exit_code(0).expect("fake ninja fixture should build"); -}Also remove the unused imports:
-use common::{create_test_manifest, ninja_with_exit_code, workflow_contents}; +use common::workflow_contents;
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
tests/common/mod.rstests/runner_tests.rstests/runner_tool_subcommands_tests.rstests/workflow_build_and_package.rstests/workflow_release.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/workflow_build_and_package.rstests/workflow_release.rstests/runner_tests.rstests/common/mod.rstests/runner_tool_subcommands_tests.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:
tests/workflow_build_and_package.rstests/workflow_release.rstests/runner_tests.rstests/common/mod.rstests/runner_tool_subcommands_tests.rs
🧠 Learnings (1)
📚 Learning: 2025-12-29T19:00:20.890Z
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Applied to files:
tests/workflow_build_and_package.rstests/workflow_release.rstests/runner_tests.rstests/common/mod.rstests/runner_tool_subcommands_tests.rs
🧬 Code graph analysis (2)
tests/workflow_release.rs (1)
tests/common/mod.rs (3)
create_test_manifest(17-23)ninja_with_exit_code(29-36)workflow_contents(39-44)
tests/runner_tool_subcommands_tests.rs (2)
tests/common/mod.rs (2)
create_test_manifest(17-23)workflow_contents(39-44)tests/runner_tests.rs (1)
unit_workflow_contents_reads_release_workflow(66-68)
🔍 Remote MCP Deepwiki
Let me search for information about composite actions and their benefits for workflow refactoring:
Summary of Relevant Context for PR Review
SHA Pinning Best Practices
Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. The PR's approach of pinning shared actions to specific SHAs (like 9b84b9fda7ac050cc5565bfa065740bf73b50c30) aligns with security best practices. Pinning an action to a full-length commit SHA is currently the only way to use an action as an immutable release.
Why this matters for the PR: The workflow files now use full SHA pins across all shared-actions references (e.g., determine-release-modes@9b84b9f..., stage-release-artefacts@...), which provides reproducibility and protects against tag mutations.
Composite Actions vs. Reusable Workflows
The PR uses reusable actions from leynos/shared-actions. Understanding the architectural choice:
- Composite actions are designed to encapsulate a sequence of actions into a singular, reusable entity, enhancing the modularity and efficiency of workflows.
- Reusable workflows and composite actions both help you avoid duplicating workflow content. Whereas reusable workflows allow you to reuse an entire workflow, with multiple jobs and steps, composite actions combine multiple steps that you can then run within a job step, just like any other action.
Implication: The refactoring centralizes workflow logic in a shared repository, reducing maintenance burden and ensuring consistency across multiple projects.
Transitive Dependency Security Considerations
A notable concern when adopting shared actions: Research on JavaScript Actions found that 54% contain at least one security weakness, with most vulnerabilities coming from indirect dependencies. Additionally, the addition of composite actions and reusable workflows created a dependency system, but the implementation ignored lessons from package management: lockfiles, integrity verification, transitive pinning, dependency visibility.
Recommendation for review: While SHA pinning mitigates direct action tampering, verify that the shared actions repository (leynos/shared-actions) and its internal dependencies are well-maintained and trusted. Consider establishing a dependency management strategy for the shared actions themselves.
Workflow Output Mapping Changes
The PR refactors output references from property-style access (e.g., steps.bin_name.outputs.value) to dictionary-key access (e.g., steps.bin_name.outputs['bin-name']). This aligns with modern GitHub Actions syntax and improves consistency when shared actions export outputs via standardized naming conventions.
Key Test Coverage Points
The PR adds tests that validate:
- Shared action wiring (e.g., verifying
stage-release-artefactsis invoked) - Consistent SHA pinning across all workflows
- Workflow output field availability and naming consistency
This provides confidence that the refactoring maintains functional parity with the original implementation.
[::web_search::],,
⏰ 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). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
tests/common/mod.rs (1)
38-44: LGTM!The helper properly returns
Result<String>and propagates errors with context, addressing the previous review feedback. The implementation correctly usesenv!("CARGO_MANIFEST_DIR")to locate workflow files and provides clear error messages.tests/workflow_release.rs (3)
7-27: LGTM!The test properly validates that the release workflow references the expected shared actions:
determine-release-modes,ensure-cargo-version,export-cargo-metadata, andupload-release-assets. The assertions align with the PR's objective to centralize workflow logic in shared actions.
29-41: LGTM!The test correctly verifies that
bin-namefield export is configured and properly wired through to theupload-release-assetsaction via theneeds.metadata.outputs.bin_namereference.
43-64: LGTM!The test thoroughly validates the release mode outputs wiring, checking that
should-publish,dry-run, andshould-upload-workflow-artifactsare captured from the shared action's dictionary-style outputs and properly referenced in the workflow.tests/workflow_build_and_package.rs (1)
7-44: LGTM!The test comprehensively validates the build-and-package workflow wiring:
- Confirms usage of the shared
stage-release-artefactsaction- Verifies Windows path normalisation logic
- Checks that staged outputs (
binary_path,license_path,man_path,artifact_dir) are properly consumed by downstream stepsThe assertions align well with the PR's refactoring objectives.
…r tests - Moved shared fixtures (create_test_manifest and ninja_with_exit_code) from tests/common/mod.rs to new tests/fixtures/mod.rs - Updated runner_tests.rs and runner_tool_subcommands_tests.rs to use fixtures module - Simplified tests/common/mod.rs to contain only workflow validation helpers - Removed duplicate tests verifying workflow_contents in multiple test files This separation clarifies the distinction between general workflow helpers and runner-specific fixtures, improving code organization and reducing duplication. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
deleted the
terragon/refactor-release-workflow-shared-actions-x1sf00
branch
The rust-build-release shared action only accepts target, project-dir, and bin-name inputs. The version parameter was incorrectly passed during the PR #253 refactor, causing the CI warning "Unexpected input(s) 'version'" and subsequent "Unable to locate executable file: undefined" error during macOS builds. The version input remains correctly passed to packaging actions (macos-package, windows-package, linux-packages) where it is a valid parameter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* Remove invalid version input from rust-build-release action The rust-build-release shared action only accepts target, project-dir, and bin-name inputs. The version parameter was incorrectly passed during the PR #253 refactor, causing the CI warning "Unexpected input(s) 'version'" and subsequent "Unable to locate executable file: undefined" error during macOS builds. The version input remains correctly passed to packaging actions (macos-package, windows-package, linux-packages) where it is a valid parameter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Update shared-actions to bd92b1e for macOS x86_64 sccache fix Updates shared-actions pin from df81280dcc to bd92b1e6f7. The new version includes a fix for sccache no longer being published for macOS x86_64, which was causing the "Unable to locate executable file: undefined" error during macOS builds. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Update shared-actions to bd92b1e for macOS x86_64 sccache fix Updates shared-actions pin from df81280dcc to bd92b1e6f7. The new version includes a fix for sccache no longer being published for macOS x86_64, which was causing the "Unable to locate executable file: undefined" error during macOS builds. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Update shared-actions to 7f4cc57 for macOS build fix 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Changes
Why
Testing plan
Notes
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/fe6889ba-b3ae-45dc-8bbb-80d586190fdf
Summary by Sourcery
Refactor release workflows to consume shared reusable actions for staging, metadata, and packaging while simplifying local tooling and keeping automation reproducible via pinned SHAs.
Enhancements:
Documentation: