Add release workflow actions migrated from netsuke#219
Conversation
Migrate four reusable GitHub Actions from leynos/netsuke: - export-cargo-metadata: Extract Cargo.toml metadata as outputs/env vars - determine-release-modes: Normalize event payloads into release flags - upload-release-assets: Upload staged artefacts to GitHub releases - stage-release-artefacts: Stage artefacts using TOML configuration Also adds shared cargo_utils.py for Cargo.toml parsing, now used by both export-cargo-metadata and ensure-cargo-version actions. All actions include comprehensive tests (147 new tests total) and follow the established patterns: PEP 723 headers, cyclopts CLI, and plumbum for subprocess execution. 🤖 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 CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughSummarise added composite GitHub Actions for release workflows (determine-release-modes, export-cargo-metadata, stage-release-artefacts, upload-release-assets), introduce cargo_utils and bool_utils helpers, implement a stage_common staging library, refactor ensure-cargo-version to reuse cargo_utils, and add tests, behavioural workflows and documentation for local validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgb(240,248,255)
participant WF as Workflow
participant Action as Composite Action
participant Script as Action Script
participant FS as Filesystem
participant GOut as GITHUB_OUTPUT
participant GH as gh CLI
end
WF->>Action: invoke (inputs, event)
Action->>Script: execute (env: INPUT_*)
Script->>FS: read event / manifest / config
Script->>FS: resolve artefact paths & templates
alt determine-release-modes
Script->>GOut: write outputs (dry-run, should-publish, should-upload-workflow-artifacts)
Script-->>Action: exit 0
else stage-release-artefacts then upload-release-assets
Script->>FS: copy staged artefacts, write checksum sidecars
Script->>GOut: write staged outputs (staged-files, artefact-map, checksum-map)
Script->>GH: invoke gh release upload (dry-run optional)
GH-->>Script: respond with status
Script->>GOut: write upload results (uploaded-count, upload-error)
Script-->>Action: exit with status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3){.github/actions/**/src/**/*.{ts,tsx,js,sh},.rules/**/*.py,**/*.py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*test*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-12-27T17:51:29.917ZApplied to files:
🧬 Code graph analysis (1)tests/workflows/test_action_behaviors.py (1)
🔍 Remote MCPLet me try searching for context about the original netsuke project and the GitHub Actions patterns being used:Based on my search for relevant context to this pull request, here's the key additional information for PR review: GitHub Actions Architecture & Best PracticesComposite actions allow you to collect a series of workflow job steps into a single action which you can then run as a single job step in multiple workflows. This PR introduces four well-structured composite actions following this pattern. Key architectural points from the PR:
Testing with
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Address CodeScene feedback on code health: - Refactor cargo_utils.py resolve_version function by extracting helpers: _require_package_table, _is_workspace_inherited, _resolve_inherited_version, and _require_version_string. Reduces cyclomatic complexity from 12 to ~3. - Refactor determine_release_modes.py by extracting _determine_dry_run and _determine_should_publish helpers. Reduces complexity from 9 to ~4. - Refactor read_manifest.py main function by extracting _resolve_manifest_path, _validate_export_flag, _load_manifest, _export_field, and _process_fields. Reduces complexity from 15 to ~4. - Fix Bumpy Road in resolution.py _resolve_glob_pattern by extracting _parts_to_pattern, _resolve_absolute_glob, and _resolve_windows_glob. - Fix Bumpy Road in output.py write_github_output by extracting _format_list_output and _format_scalar_output helpers. - Reduce test duplication in test_cargo_utils.py TestGetWorkspaceVersion using pytest.mark.parametrize. - Reduce test duplication in test_read_manifest.py by consolidating 7 similar _extract_field tests into a single parametrized test with _load_and_extract helper. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix EM102 lint errors in cargo_utils.py by assigning f-strings to variables before raising exceptions - Fix staged_files output format in output.py to return list[str] instead of newline-joined string, ensuring proper heredoc output that matches the README promise of newline-separated list - Add type validation in config.py for artefact 'required' (must be bool) and 'alternatives' (must be list[str]) fields, preventing silent iteration over string characters if TOML is malformed - Add unknown field validation in read_manifest.py with warning when unsupported fields are requested, making typos visible in workflow logs instead of silently returning no value 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: .github/actions/stage-release-artefacts/scripts/stage_common/config.py Comment on lines +170 to +221 def _make_artefacts(
common: dict[str, typ.Any], target_cfg: dict[str, typ.Any], config_path: Path
) -> list[ArtefactConfig]:
"""Build list of ArtefactConfig from common and target sections."""
entries = [*common.get("artefacts", []), *target_cfg.get("artefacts", [])]
if not entries:
msg = "No artefacts configured to stage."
raise StageError(msg)
artefacts: list[ArtefactConfig] = []
for index, entry in enumerate(entries, start=1):
source = entry.get("source")
if not isinstance(source, str) or not source:
msg = (
"Missing required artefact key 'source' "
f"in entry #{index} of {config_path}"
)
raise StageError(msg)
required = entry.get("required", True)
if not isinstance(required, bool):
msg = (
f"Artefact 'required' must be a boolean, got {type(required).__name__} "
f"in entry #{index} of {config_path}"
)
raise StageError(msg)
alternatives = entry.get("alternatives", [])
if not isinstance(alternatives, list):
alt_type = type(alternatives).__name__
msg = (
f"Artefact 'alternatives' must be a list, got {alt_type} "
f"in entry #{index} of {config_path}"
)
raise StageError(msg)
for alt_idx, alt in enumerate(alternatives):
if not isinstance(alt, str):
msg = (
f"Artefact alternatives[{alt_idx}] must be a string, "
f"got {type(alt).__name__} in entry #{index} of {config_path}"
)
raise StageError(msg)
artefacts.append(
ArtefactConfig(
source=source,
required=required,
output=entry.get("output"),
destination=entry.get("destination"),
alternatives=alternatives,
)
)
return artefacts❌ New issue: Bumpy Road Ahead |
- Extract validation helpers from _make_artefacts in config.py to fix Bumpy Road: _validate_source, _validate_required, _validate_alternatives, _parse_artefact_entry - Extract _extract_first_bin_name from get_bin_name in cargo_utils.py to reduce module complexity below threshold - Add _setup_github_env helper in test_read_manifest.py to reduce duplication across test_main_* functions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: .github/actions/stage-release-artefacts/scripts/stage_common/config.py Comment on lines +170 to +181 def _validate_source(
entry: dict[str, typ.Any], index: int, config_path: Path
) -> str:
"""Validate and extract the source field from an artefact entry."""
source = entry.get("source")
if not isinstance(source, str) or not source:
msg = (
"Missing required artefact key 'source' "
f"in entry #{index} of {config_path}"
)
raise StageError(msg)
return source❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
- Add _validate_field_type[T] generic function using Python 3.12+ type parameter syntax to consolidate type-checking logic - Simplify _validate_source to 4 lines using the helper with allow_empty_str=False for required non-empty strings - Simplify _validate_required to 4 lines using the helper for bool types - Preserves all existing error messages exactly (including "boolean" instead of "bool" for bool type mismatches) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: .github/actions/stage-release-artefacts/scripts/stage_common/config.py Comment on lines +202 to +208 def _validate_source(
entry: dict[str, typ.Any], index: int, config_path: Path
) -> str:
"""Validate and extract the source field from an artefact entry."""
return _validate_field_type(
entry.get("source"), "source", str, index, config_path, allow_empty_str=False
)❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: .github/actions/stage-release-artefacts/scripts/stage_common/config.py Comment on lines +202 to +206 def _validate_source(entry: dict[str, typ.Any], index: int, config_path: Path) -> str:
"""Validate and extract the source field from an artefact entry."""
return _validate_field_type(
entry.get("source"), "source", str, index, config_path, allow_empty_str=False
)❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
Eliminate all nested if statements by using three sequential top-level conditionals: 1. Valid non-empty string case - returns immediately 2. Invalid non-empty string case - raises error (only executes if #1 didn't return) 3. General type mismatch - unchanged Extract `is_nonempty_str` variable to keep line length within 88 chars. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
force-pushed
the
import-shared-actions-from-netsuke
branch
from
4de6e0d to
1413eeb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: .github/actions/stage-release-artefacts/scripts/stage_common/config.py Comment on file For strings with ``allow_empty_str=False``, also checks the value is non-empty.
"""
is_nonempty_str = isinstance(value, str) and value
if expected_type is str and not allow_empty_str and is_nonempty_str:❌ New issue: Complex Conditional |
This comment was marked as resolved.
This comment was marked as resolved.
Add dedicated helper for string validation with simple single-condition checks: - Type check: isinstance(value, str) raises "must be a string" error - Empty check: not allow_empty and not value raises "Missing required" Simplify _validate_field_type to delegate string validation entirely, eliminating the complex 3-way AND conditional. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: .github/actions/stage-release-artefacts/scripts/stage_common/config.py Comment on lines +170 to +196 def _validate_string_field(
value: object,
field_name: str,
index: int,
config_path: Path,
*,
allow_empty: bool,
) -> str:
"""Validate a string field value.
When ``allow_empty=False``, also checks the string is non-empty.
"""
if not isinstance(value, str):
msg = (
f"Artefact '{field_name}' must be a string, "
f"got {type(value).__name__} in entry #{index} of {config_path}"
)
raise StageError(msg)
if not allow_empty and not value:
msg = (
f"Missing required artefact key '{field_name}' "
f"in entry #{index} of {config_path}"
)
raise StageError(msg)
return value❌ New issue: Excess Number of Function Arguments |
Replace inline boolean coercion logic with the shared coerce_bool helper from bool_utils for consistency with the rest of the codebase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/workflows/conftest.py
🧰 Additional context used
📓 Path-based instructions (3)
{.github/actions/**/src/**/*.{ts,tsx,js,sh},.rules/**/*.py,**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
After modifying any Python code or GitHub Action logic, ensure
make check-fmt,make typecheck,make lint, andmake testcomplete successfully before requesting review
Files:
tests/workflows/conftest.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For Python development in actions, follow guidelines in .rules/ directory including Python 3.13 style conventions, context managers, generators, project configuration, exception design, return patterns, and typing
**/*.py: Use snake_case.py for Python file names, named to reflect contents (e.g.,http_client.py,task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright type-checking. Use typing everywhere
UseTypedDictordataclassfor structured data, preferring@dataclass(slots=True)for internal-only usage
AvoidAnytype annotation. Prefer precise types (TypeVar,Protocol,Literal,Union) and usetyping.cast[...]only when necessary with justification. Useobjectfor unknown-but-opaque values
Be explicit with return type annotations. Use-> None,-> str, etc., for all public functions and class methods
Favour immutability. Prefer tuples to lists, andtypes.MappingProxyTypefor read-only mappings
Avoid side effects at import time. Modules should not modify global state or perform actions on import
Use Ruff for formatting. Let Ruff handle whitespace and formatting entirely—do not override
Document public functions, classes, and modules using NumPy docstring format
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, reducing the risk of forgetting to release resources (files, locks, connections, etc.) and simplifying error handling
Use@contextlib.contextmanagerdecorator for straightforward procedural setup/teardown with linear control flow and no persistent state
Implement__enter__and__exit__methods in a class-based context manager when internal state or lifecycle logic spans methods, or when you need...
Files:
tests/workflows/conftest.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/workflows/conftest.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*test*.py: Use pytest idioms. Prefer fixtures to setup/teardown methods. Parametrize broadly. Avoid unnecessary mocks
Group related tests usingclasswith method names prefixed bytest_
Write tests from a user's perspective. Test public behaviour, not internals
Avoid mocking too much. Prefer test doubles only for external services or non-deterministic behavioursIn tests, use
pytest.raises()with specific exception types and optional regex matchers to constrain expected exceptions, avoiding overly broad exception assertions (B017)
Files:
tests/workflows/conftest.py
🧠 Learnings (1)
📚 Learning: 2025-12-27T17:51:29.917Z
Learnt from: leynos
Repo: leynos/shared-actions PR: 219
File: cargo_utils.py:40-41
Timestamp: 2025-12-27T17:51:29.917Z
Learning: In Python files that have 'from __future__ import annotations' (which makes type annotations be evaluated as strings), avoid creating runtime references to types just by annotating attributes. For types used only in annotations or docstrings, import them under a TYPE_CHECKING guard instead of at module import time. Do runtime imports only for actual usage (e.g., Path(...) construction, isinstance checks, classmethod calls, or default values that reference the type). For cargo_utils.py in leynos/shared-actions, ensure runtime behavior is not affected by type-only imports and prefer 'if TYPE_CHECKING' or local imports inside functions where the type is needed at runtime.
Applied to files:
tests/workflows/conftest.py
🧬 Code graph analysis (1)
tests/workflows/conftest.py (2)
bool_utils.py (1)
coerce_bool(15-57)tests/workflows/test_action_behaviors.py (1)
artifact_dir(25-27)
⏰ 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
Add helper function to consolidate repeated regex assertion pattern in behavioural tests. Refactor TestDetermineReleaseModes, TestExportCargoMetadata, and TestStageReleaseArtefacts to use it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Update actions/checkout version comment from v4 to v4.3.1 across all test workflow files (verified via GitHub API) - Fix _container_runtime_available() to capture and validate exit code instead of discarding the return value from plumbum's run() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add syspath-hack dependency to export-cargo-metadata and stage-release-artefacts PEP 723 script metadata - Change bool|str union types to str in CLI function signatures to fix cyclopts env var parsing (bool and str consume different token counts) The coerce_bool utility already handles string-to-bool conversion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.github/actions/export-cargo-metadata/scripts/read_manifest.py.github/actions/stage-release-artefacts/scripts/stage.py.github/actions/upload-release-assets/scripts/upload_release_assets.py.github/workflows/test-determine-release-modes.yml.github/workflows/test-export-cargo-metadata.yml.github/workflows/test-stage-release-artefacts.yml.github/workflows/test-upload-release-assets.ymltests/workflows/conftest.pytests/workflows/test_action_behaviors.py
🧰 Additional context used
📓 Path-based instructions (4)
{.github/workflows/**/*.{yml,yaml},.github/actions/**/action.yml}
📄 CodeRabbit inference engine (AGENTS.md)
{.github/workflows/**/*.{yml,yaml},.github/actions/**/action.yml}: Pin third-party actions to a full commit SHA, not just @v1
Request minimum permission set in workflows and avoid GITHUB_TOKEN write unless essential
Job-level permissions: override workflow-level permissions; audit both when setting permissions
Files:
.github/workflows/test-export-cargo-metadata.yml.github/workflows/test-upload-release-assets.yml.github/workflows/test-determine-release-modes.yml.github/workflows/test-stage-release-artefacts.yml
{.github/actions/**/src/**/*.{ts,tsx,js,sh},.rules/**/*.py,**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
After modifying any Python code or GitHub Action logic, ensure
make check-fmt,make typecheck,make lint, andmake testcomplete successfully before requesting review
Files:
tests/workflows/conftest.pytests/workflows/test_action_behaviors.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For Python development in actions, follow guidelines in .rules/ directory including Python 3.13 style conventions, context managers, generators, project configuration, exception design, return patterns, and typing
**/*.py: Use snake_case.py for Python file names, named to reflect contents (e.g.,http_client.py,task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright type-checking. Use typing everywhere
UseTypedDictordataclassfor structured data, preferring@dataclass(slots=True)for internal-only usage
AvoidAnytype annotation. Prefer precise types (TypeVar,Protocol,Literal,Union) and usetyping.cast[...]only when necessary with justification. Useobjectfor unknown-but-opaque values
Be explicit with return type annotations. Use-> None,-> str, etc., for all public functions and class methods
Favour immutability. Prefer tuples to lists, andtypes.MappingProxyTypefor read-only mappings
Avoid side effects at import time. Modules should not modify global state or perform actions on import
Use Ruff for formatting. Let Ruff handle whitespace and formatting entirely—do not override
Document public functions, classes, and modules using NumPy docstring format
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, reducing the risk of forgetting to release resources (files, locks, connections, etc.) and simplifying error handling
Use@contextlib.contextmanagerdecorator for straightforward procedural setup/teardown with linear control flow and no persistent state
Implement__enter__and__exit__methods in a class-based context manager when internal state or lifecycle logic spans methods, or when you need...
Files:
tests/workflows/conftest.pytests/workflows/test_action_behaviors.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/workflows/conftest.pytests/workflows/test_action_behaviors.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*test*.py: Use pytest idioms. Prefer fixtures to setup/teardown methods. Parametrize broadly. Avoid unnecessary mocks
Group related tests usingclasswith method names prefixed bytest_
Write tests from a user's perspective. Test public behaviour, not internals
Avoid mocking too much. Prefer test doubles only for external services or non-deterministic behavioursIn tests, use
pytest.raises()with specific exception types and optional regex matchers to constrain expected exceptions, avoiding overly broad exception assertions (B017)
Files:
tests/workflows/conftest.pytests/workflows/test_action_behaviors.py
🧠 Learnings (1)
📚 Learning: 2025-12-27T17:51:29.917Z
Learnt from: leynos
Repo: leynos/shared-actions PR: 219
File: cargo_utils.py:40-41
Timestamp: 2025-12-27T17:51:29.917Z
Learning: In Python files that have 'from __future__ import annotations' (which makes type annotations be evaluated as strings), avoid creating runtime references to types just by annotating attributes. For types used only in annotations or docstrings, import them under a TYPE_CHECKING guard instead of at module import time. Do runtime imports only for actual usage (e.g., Path(...) construction, isinstance checks, classmethod calls, or default values that reference the type). For cargo_utils.py in leynos/shared-actions, ensure runtime behavior is not affected by type-only imports and prefer 'if TYPE_CHECKING' or local imports inside functions where the type is needed at runtime.
Applied to files:
tests/workflows/conftest.pytests/workflows/test_action_behaviors.py
🧬 Code graph analysis (5)
tests/workflows/conftest.py (2)
bool_utils.py (1)
coerce_bool(15-57)tests/workflows/test_action_behaviors.py (1)
artifact_dir(25-27)
.github/actions/export-cargo-metadata/scripts/read_manifest.py (1)
cargo_utils.py (5)
ManifestError(44-68)get_bin_name(168-212)get_package_field(107-152)read_manifest(71-104)resolve_version(327-373)
tests/workflows/test_action_behaviors.py (1)
tests/workflows/conftest.py (2)
ActConfig(56-62)run_act(65-123)
.github/actions/stage-release-artefacts/scripts/stage.py (5)
.github/actions/stage-release-artefacts/scripts/stage_common/errors.py (1)
StageError(6-7).github/actions/stage-release-artefacts/scripts/stage_common/config.py (2)
load_config(83-138)staging_dir(53-55).github/actions/stage-release-artefacts/scripts/stage_common/environment.py (1)
require_env_path(13-35).github/actions/stage-release-artefacts/scripts/stage_common/pipeline.py (1)
stage_artefacts(85-150)bool_utils.py (1)
coerce_bool(15-57)
.github/actions/upload-release-assets/scripts/upload_release_assets.py (1)
bool_utils.py (1)
coerce_bool(15-57)
🔍 Remote MCP
I'll search for relevant information to help contextualize this pull request about migrating release workflow actions.Based on my research, I can now provide contextual information to help with reviewing this pull request.
Summary of Additional Context for PR Review
Technologies & Patterns Used
Cyclopts CLI Framework:
Cyclopts is an intuitive CLI framework with advanced type hinting support, rich help generation, and customizable converters and validators. The PR adopts this framework consistently across multiple action scripts for standardized CLI parameter parsing and environment variable handling.
GitHub Composite Actions Best Practices:
Composite actions allow you to collect a series of workflow job steps into a single action which you can then run as a single job step in multiple workflows. Composite actions combine multiple steps that you can then run within a job step, whereas reusable workflows allow you to reuse an entire workflow with multiple jobs and steps. This PR appropriately uses composite actions for step-level encapsulation, stored in the recommended .github/actions subfolder pattern.
Code Organization & Dependency Structure
The PR follows established patterns:
- Local action placement: All four new actions (determine-release-modes, export-cargo-metadata, stage-release-artefacts, upload-release-assets) follow the standard
.github/actions/<action-name>/directory structure withaction.ymlmetadata files. - Shared utilities: Centralized
cargo_utils.pyandbool_utils.pyat the repository root promote code reuse, addressing the duplication issues identified in PR comments about boolean coercion logic. - Test harness patterns: The new
tests/workflows/test infrastructure withconftest.py, fixture files, and behavioral tests usingactdemonstrate effort to validate composite actions end-to-end.
Review Considerations Based on PR Comments
The PR feedback history indicates several focused improvements:
- Complexity reduction: Validation logic in TOML parsing was refactored to reduce nested conditions while preserving error messages.
- Boolean coercion centralization: Previously duplicated logic is now unified in
bool_utils.pywithcoerce_bool()andcoerce_bool_strict()for consistent input handling across scripts. - Output writing: Directory creation before writing to
GITHUB_OUTPUTwas added to handle missing parent directories. - Path collision avoidance: Checksum maps updated to use relative paths rather than basenames to prevent overwrites.
- Test coverage: 147 new unit tests added, though some edge cases (e.g., parent directory creation regression tests) were iteratively identified and addressed.
Key Files & Patterns to Verify
action.ymlfiles: Each composite action defines inputs, outputs, and a two-step pattern: setup uv viaastral-sh/setup-uv@, then run a Python script with environment variable bindings.- PEP 723 compliance: Scripts include script metadata (via docstrings or headers) for dependency declaration.
- Environment variable passing: Inputs are passed as uppercase
INPUT_*environment variables fromaction.ymlto Python scripts, consistent with GitHub Actions conventions. - Error handling & output: Scripts use GitHub Actions output format (
GITHUB_OUTPUTfile writing) and stderr for diagnostic messages.
[::web_search::], [::Context7::], [::Deepwiki::]
⏰ 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). (6)
- GitHub Check: python-tests (macos-15)
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: python-tests-windows
- GitHub Check: build-release (windows-latest, aarch64-pc-windows-gnullvm)
- GitHub Check: build-release (ubuntu-latest, x86_64-unknown-linux-gnu)
- GitHub Check: build-release (windows-latest, x86_64-pc-windows-gnullvm)
🔇 Additional comments (30)
.github/workflows/test-export-cargo-metadata.yml (3)
6-10: LGTM!Job-level permissions correctly declare
contents: read, following least-privilege principles. This addresses previous review feedback.
14-29: LGTM!Action invocation and output verification logic are correct. The semver regex appropriately validates the core version whilst permitting pre-release suffixes.
12-12: No action required. The SHA 34e114876b0b11c390a56381ad16ebd13914f8d5 correctly corresponds to v4.3.1 and the comment is accurate. The commit message references "v6 auth style" as the cleanup topic, not the release version..github/actions/export-cargo-metadata/scripts/read_manifest.py (4)
1-51: LGTM on module setup and imports.The PEP 723 header correctly declares dependencies, and the import structure properly uses
syspath_hackto enable shared utility imports. Thecycloptsconfiguration for environment variable parsing follows the established pattern.
54-73: LGTM on output/env helpers.Both
_write_outputand_write_envcorrectly create parent directories before writing, addressing resilience concerns from previous reviews.
102-122: LGTM on field extraction with structural pattern matching.The
match/casedispatch is clean and handles the description case safely by checkingisinstance(package, dict)before accessing it.
182-209: LGTM on main entry point.The
mainfunction correctly validates the boolean input usingcoerce_bool_strict, loads the manifest with error handling, and processes fields with appropriate output. The flow is clear and error states are properly communicated via GitHub Actions annotations..github/actions/upload-release-assets/scripts/upload_release_assets.py (7)
1-53: LGTM on module setup.The PEP 723 dependencies correctly include
syspath-hack, and the import structure is sound. TheTYPE_CHECKINGguard forBoundCommandis appropriate.
55-66: LGTM on data structures.
AssetErrorandReleaseAssetare well-defined. The frozen dataclass ensures immutability for release assets.
117-160: LGTM on asset discovery.The
discover_assetsfunction properly validates directory existence, detects empty artefacts, checks for name collisions, and raisesAssetErrorwith clear messages. The docstring follows NumPy format.
173-228: LGTM on upload logic.The
upload_assetsfunction correctly handles dry-run mode with accurate clobber flag representation, and lazily initialises theghcommand only when needed.
231-239: LGTM on output helper.The
_write_outputfunction correctly creates parent directories before writing, matching the pattern in other actions.
242-302: LGTM on main entry point.The error handling covers both
AssetErrorfrom discovery andProcessExecutionError/CommandNotFoundfrom upload. Output variables are consistently written for both success and failure cases.
305-327: LGTM on CLI wrapper.The
clifunction correctly usescoerce_boolwith appropriate defaults fordry_runandclobberparameters..github/workflows/test-upload-release-assets.yml (1)
1-36: LGTM on workflow definition.The workflow correctly:
- Requests minimal
contents: readpermissions- Pins
actions/checkoutto a full commit SHA- Uses kebab-case output references matching the action definition
- Validates dry-run behaviour with appropriate assertions
.github/workflows/test-stage-release-artefacts.yml (1)
1-60: LGTM on workflow definition.The workflow correctly:
- Requests minimal
contents: readpermissions- Pins
actions/checkoutto a full commit SHA- Uses kebab-case output references consistently
- Creates a valid TOML staging configuration
- Verifies the staging directory, binary, and checksum sidecar exist
.github/actions/stage-release-artefacts/scripts/stage.py (2)
1-48: LGTM on module setup.The PEP 723 dependencies are correct, and the dual use of
prepend_to_syspath(for localstage_common) andprepend_project_root(for sharedbool_utils) is appropriate given the module structure.
51-93: LGTM on main entry point.The
mainfunction correctly:
- Loads configuration with proper path handling
- Coerces boolean input with
coerce_bool- Handles errors with GitHub Actions-formatted messages
- Reports success with a relative path summary
tests/workflows/conftest.py (4)
1-16: LGTM on module setup.The imports are appropriate,
FIXTURES_DIRcorrectly usespathlib.Path, and thecoerce_boolimport enables consistent boolean handling.
18-42: LGTM on availability checks.The helper functions correctly:
- Use
shutil.whichfor executable detection- Capture and validate the exit code from container runtime checks
- Use
coerce_boolfor environment variable parsing
44-52: LGTM on skip markers.The pytest skip markers are well-structured with clear reasons.
55-123: LGTM on ActConfig and run_act.The
ActConfigdataclass usesslots=Trueper guidelines, andrun_actproperly:
- Creates the artefact directory
- Falls back to fixture event paths
- Handles timeouts gracefully
- Returns combined stdout/stderr for log assertions
tests/workflows/test_action_behaviors.py (7)
1-22: LGTM on module setup.The module docstring uses en-GB spelling, and the
TYPE_CHECKINGguard forPathcorrectly avoids runtime import costs per the learnings.
24-55: LGTM on fixtures and helpers.The
artifact_dirfixture is appropriately scoped, and_run_act_and_get_logsprovides a clean abstraction for test setup with built-in success assertion.
57-73: LGTM on assertion helper.The
_assert_log_patternsfunction consolidates regex assertions with clear error messages, reducing duplication across tests.
75-127: LGTM on TestDetermineReleaseModes.The parametrised
test_dry_run_modeconsolidates duplicate tests, and the regex patterns are sufficiently specific to avoid false positives.
130-152: LGTM on TestExportCargoMetadata.The test verifies non-empty values for
nameandversionusing appropriate regex patterns.
154-181: LGTM on TestStageReleaseArtefacts.The test verifies staging outputs with appropriate patterns for
artifact-dirandstaged-files.
184-205: LGTM on TestUploadReleaseAssets.The test correctly extracts and validates
uploaded-countas a numeric value and checksupload-error=false..github/workflows/test-determine-release-modes.yml (1)
19-20: LGTM!Minimal permissions (
contents: read) appropriately scoped for a test workflow that checks out code and validates action outputs.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/workflows/test_action_behaviors.py: What lead to degradation?The module contains 2 functions with similar structure: TestDetermineReleaseModes.test_dry_run_mode,TestDetermineReleaseModes.test_push_tag_event Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
- Consolidate TestDetermineReleaseModes tests into parametrized test - Add comment explaining field name kebab-case normalisation - Use match/case in _is_candidate for artefact detection - Wire workflow_call inputs through to determine-release-modes action 🤖 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
♻️ Duplicate comments (1)
.github/workflows/test-determine-release-modes.yml (1)
31-39: Strengthen test coverage with event-specific value assertions.The verification only checks outputs are boolean strings, but does not assert expected values for specific event types. Add conditional assertions to verify:
workflow_callevents producedry-run=truepull_requestevents producedry-run=trueandshould-publish=falsepushevents onv*tags producedry-run=falseandshould-publish=trueThis will catch regressions in the action's event-normalisation logic rather than only validating output format.
🔎 Example enhanced verification
- name: Verify outputs run: | echo "dry-run=${{ steps.modes.outputs.dry-run }}" echo "should-publish=${{ steps.modes.outputs.should-publish }}" echo "should-upload-workflow-artifacts=${{ steps.modes.outputs.should-upload-workflow-artifacts }}" # Verify outputs are boolean strings [[ "${{ steps.modes.outputs.dry-run }}" =~ ^(true|false)$ ]] [[ "${{ steps.modes.outputs.should-publish }}" =~ ^(true|false)$ ]] [[ "${{ steps.modes.outputs.should-upload-workflow-artifacts }}" =~ ^(true|false)$ ]] + + - name: Verify logic for pull_request events + if: github.event_name == 'pull_request' + run: | + # Expect dry-run for PRs + [[ "${{ steps.modes.outputs.dry-run }}" == "true" ]] + [[ "${{ steps.modes.outputs.should-publish }}" == "false" ]] + + - name: Verify logic for tag push events + if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v') + run: | + # Expect publish for version tags + [[ "${{ steps.modes.outputs.dry-run }}" == "false" ]] + [[ "${{ steps.modes.outputs.should-publish }}" == "true" ]] + + - name: Verify logic for workflow_call events + if: github.event_name == 'workflow_call' + run: | + # Workflow call respects inputs (defaults: dry-run=true, publish=false) + [[ "${{ steps.modes.outputs.dry-run }}" == "${{ inputs.dry-run }}" ]] + [[ "${{ steps.modes.outputs.should-publish }}" == "${{ inputs.publish }}" ]]🤖 Prompt for AI Agents
.github/workflows/test-determine-release-modes.yml around lines 31-39: add event-specific value assertions to verify correctness of outputs per trigger scenario—after the existing boolean format checks, add three new conditional steps: (1) a step with if: github.event_name == 'pull_request' that asserts dry-run == "true" and should-publish == "false"; (2) a step with if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v') that asserts dry-run == "false" and should-publish == "true"; (3) a step with if: github.event_name == 'workflow_call' that asserts dry-run == inputs.dry-run and should-publish == inputs.publish; use bash [[ ]] assertions with descriptive comments in each step so the workflow catches regressions in event-normalisation logic.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/actions/export-cargo-metadata/scripts/read_manifest.py.github/actions/upload-release-assets/scripts/upload_release_assets.py.github/workflows/test-determine-release-modes.ymltests/workflows/test_action_behaviors.py
🧰 Additional context used
📓 Path-based instructions (4)
{.github/workflows/**/*.{yml,yaml},.github/actions/**/action.yml}
📄 CodeRabbit inference engine (AGENTS.md)
{.github/workflows/**/*.{yml,yaml},.github/actions/**/action.yml}: Pin third-party actions to a full commit SHA, not just @v1
Request minimum permission set in workflows and avoid GITHUB_TOKEN write unless essential
Job-level permissions: override workflow-level permissions; audit both when setting permissions
Files:
.github/workflows/test-determine-release-modes.yml
{.github/actions/**/src/**/*.{ts,tsx,js,sh},.rules/**/*.py,**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
After modifying any Python code or GitHub Action logic, ensure
make check-fmt,make typecheck,make lint, andmake testcomplete successfully before requesting review
Files:
tests/workflows/test_action_behaviors.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For Python development in actions, follow guidelines in .rules/ directory including Python 3.13 style conventions, context managers, generators, project configuration, exception design, return patterns, and typing
**/*.py: Use snake_case.py for Python file names, named to reflect contents (e.g.,http_client.py,task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright type-checking. Use typing everywhere
UseTypedDictordataclassfor structured data, preferring@dataclass(slots=True)for internal-only usage
AvoidAnytype annotation. Prefer precise types (TypeVar,Protocol,Literal,Union) and usetyping.cast[...]only when necessary with justification. Useobjectfor unknown-but-opaque values
Be explicit with return type annotations. Use-> None,-> str, etc., for all public functions and class methods
Favour immutability. Prefer tuples to lists, andtypes.MappingProxyTypefor read-only mappings
Avoid side effects at import time. Modules should not modify global state or perform actions on import
Use Ruff for formatting. Let Ruff handle whitespace and formatting entirely—do not override
Document public functions, classes, and modules using NumPy docstring format
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, reducing the risk of forgetting to release resources (files, locks, connections, etc.) and simplifying error handling
Use@contextlib.contextmanagerdecorator for straightforward procedural setup/teardown with linear control flow and no persistent state
Implement__enter__and__exit__methods in a class-based context manager when internal state or lifecycle logic spans methods, or when you need...
Files:
tests/workflows/test_action_behaviors.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/workflows/test_action_behaviors.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*test*.py: Use pytest idioms. Prefer fixtures to setup/teardown methods. Parametrize broadly. Avoid unnecessary mocks
Group related tests usingclasswith method names prefixed bytest_
Write tests from a user's perspective. Test public behaviour, not internals
Avoid mocking too much. Prefer test doubles only for external services or non-deterministic behavioursIn tests, use
pytest.raises()with specific exception types and optional regex matchers to constrain expected exceptions, avoiding overly broad exception assertions (B017)
Files:
tests/workflows/test_action_behaviors.py
🧠 Learnings (1)
📚 Learning: 2025-12-27T17:51:29.917Z
Learnt from: leynos
Repo: leynos/shared-actions PR: 219
File: cargo_utils.py:40-41
Timestamp: 2025-12-27T17:51:29.917Z
Learning: In Python files that have 'from __future__ import annotations' (which makes type annotations be evaluated as strings), avoid creating runtime references to types just by annotating attributes. For types used only in annotations or docstrings, import them under a TYPE_CHECKING guard instead of at module import time. Do runtime imports only for actual usage (e.g., Path(...) construction, isinstance checks, classmethod calls, or default values that reference the type). For cargo_utils.py in leynos/shared-actions, ensure runtime behavior is not affected by type-only imports and prefer 'if TYPE_CHECKING' or local imports inside functions where the type is needed at runtime.
Applied to files:
tests/workflows/test_action_behaviors.py
🧬 Code graph analysis (3)
.github/actions/export-cargo-metadata/scripts/read_manifest.py (2)
bool_utils.py (1)
coerce_bool_strict(60-100)cargo_utils.py (5)
ManifestError(44-68)get_bin_name(168-212)get_package_field(107-152)read_manifest(71-104)resolve_version(327-373)
tests/workflows/test_action_behaviors.py (1)
tests/workflows/conftest.py (2)
ActConfig(56-62)run_act(65-123)
.github/actions/upload-release-assets/scripts/upload_release_assets.py (1)
bool_utils.py (1)
coerce_bool(15-57)
⏰ 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). (6)
- GitHub Check: python-tests (macos-15)
- GitHub Check: python-tests-windows
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: build-release (windows-latest, x86_64-pc-windows-gnullvm)
- GitHub Check: build-release (macos-15, x86_64-apple-darwin)
- GitHub Check: build-release (windows-latest, aarch64-pc-windows-gnullvm)
🔇 Additional comments (16)
.github/actions/export-cargo-metadata/scripts/read_manifest.py (3)
54-73: Excellent resilience improvement.Both output-writing helpers now create parent directories before opening files, making them consistent with other actions in the repository and resilient to missing directory structure.
120-120: Verify test coverage for description trimming.The implementation correctly strips whitespace from description values (line 120). Confirm that a unit test validates this behaviour—the past review comments indicate a description-trimming test was outstanding.
#!/bin/bash # Description: Check for tests covering description whitespace trimming. # Search for tests that verify description field extraction and whitespace handling rg -n -C5 --type=py 'description.*strip|strip.*description|test.*description.*whitespace' .github/actions/export-cargo-metadata/tests/
54-73: Verify test coverage for parent directory creation.Both
_write_outputand_write_envnow create parent directories (lines 60, 71). Confirm that a regression test validates this behaviour—the past review comments indicate a GITHUB_OUTPUT parent-directory test was outstanding.#!/bin/bash # Description: Check for tests covering parent directory creation for output/env files. # Search for tests that verify mkdir behaviour or missing parent directories rg -n -C5 --type=py 'mkdir|parent.*exist|GITHUB_OUTPUT.*parent|GITHUB_ENV.*parent|test.*output.*directory' .github/actions/export-cargo-metadata/tests/tests/workflows/test_action_behaviors.py (7)
1-21: LGTM: proper module structure and TYPE_CHECKING usage.The module docstring uses correct en-GB spelling throughout, and the TYPE_CHECKING guard properly isolates the Path import for type-checking only, consistent with the project's typing conventions.
24-27: LGTM: clean fixture implementation.The fixture follows pytest conventions and uses proper type hints.
30-54: LGTM: well-structured helper function.The helper properly encapsulates act execution with success assertion, includes comprehensive documentation, and provides clear error messages on failure.
57-72: LGTM: clean assertion helper.The helper provides good abstraction for regex-based log assertions with proper type hints and keyword-only flags parameter.
75-115: LGTM: effective parametrised test.The test consolidates three scenarios using
@pytest.mark.parametrize, employs the extracted helper functions, and uses flexible regex patterns that handle output format variations. The parametrisation eliminates duplication while maintaining clear test intent.
118-139: LGTM: strong metadata validation.The test uses regex patterns that verify both presence and non-empty values for exported metadata, with clear error messages distinguishing between missing and empty values.
142-169: LGTM: consistent staging validation.The test follows the same strong assertion pattern as the cargo metadata test, verifying both presence and non-empty values for staging outputs.
.github/actions/upload-release-assets/scripts/upload_release_assets.py (6)
1-53: Excellent setup—all past issues resolved.The PEP 723 dependencies now correctly include syspath-hack, cyclopts version constraint has been updated to allow 3.x releases, and the script imports
coerce_boolfrom the sharedbool_utilsmodule rather than duplicating the logic. The module docstring is clear and provides helpful usage examples.
55-68: Clean data model and configuration.The
AssetErrorexception provides clear error signalling, the frozenReleaseAssetdataclass is well-designed, and the App configuration correctly uses theINPUT_prefix for GitHub Actions environment variables.
83-96: Path resolution and iteration logic is sound.The
_resolve_asset_namefunction correctly generates unique names for nested artefacts by replacing path separators with double underscores, and_iter_candidate_pathsensures deterministic ordering by sorting results. Both helpers are well-designed.
99-162: Robust discovery and validation logic.The validation helpers (
_require_non_empty,_register_asset) provide excellent defensive checks, anddiscover_assetsorchestrates them correctly. Error messages are informative, the docstring is comprehensive, and the function handles all edge cases appropriately (missing directory, empty files, name collisions, no artefacts found).
165-241: Upload logic correctly addresses all past review feedback.The dry-run output now conditionally includes
--clobberbased on the parameter value (line 214), and_write_outputdefensively creates parent directories before writing (line 239). Theupload_assetsfunction handles both dry-run and live modes cleanly, with appropriate error handling forProcessExecutionErrorandCommandNotFound.
244-332: Well-structured entry points with comprehensive error handling.The
mainfunction correctly handles all error cases (discovery failures, upload failures) and writes appropriate outputs for downstream steps. Thecliwrapper usescoerce_boolfrom the sharedbool_utilsmodule, eliminating the code duplication flagged in earlier reviews. All error paths are properly surfaced via GITHUB_OUTPUT.
Add conditional verification steps that assert expected output values for each trigger type: - pull_request: dry-run=true, should-publish=false - push tag: dry-run=false, should-publish=true - workflow_call: outputs match provided inputs This strengthens test coverage to catch regressions in the action's event-normalisation logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Revert match/case to simpler if/else chain in _is_candidate as the pattern match offered no structural benefits - Remove redundant uploaded_match.group(1) assertion since the regex (\d+) guarantees a non-empty digit group 🤖 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/upload-release-assets/scripts/upload_release_assets.pytests/workflows/test_action_behaviors.py
🧰 Additional context used
📓 Path-based instructions (3)
{.github/actions/**/src/**/*.{ts,tsx,js,sh},.rules/**/*.py,**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
After modifying any Python code or GitHub Action logic, ensure
make check-fmt,make typecheck,make lint, andmake testcomplete successfully before requesting review
Files:
tests/workflows/test_action_behaviors.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For Python development in actions, follow guidelines in .rules/ directory including Python 3.13 style conventions, context managers, generators, project configuration, exception design, return patterns, and typing
**/*.py: Use snake_case.py for Python file names, named to reflect contents (e.g.,http_client.py,task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright type-checking. Use typing everywhere
UseTypedDictordataclassfor structured data, preferring@dataclass(slots=True)for internal-only usage
AvoidAnytype annotation. Prefer precise types (TypeVar,Protocol,Literal,Union) and usetyping.cast[...]only when necessary with justification. Useobjectfor unknown-but-opaque values
Be explicit with return type annotations. Use-> None,-> str, etc., for all public functions and class methods
Favour immutability. Prefer tuples to lists, andtypes.MappingProxyTypefor read-only mappings
Avoid side effects at import time. Modules should not modify global state or perform actions on import
Use Ruff for formatting. Let Ruff handle whitespace and formatting entirely—do not override
Document public functions, classes, and modules using NumPy docstring format
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, reducing the risk of forgetting to release resources (files, locks, connections, etc.) and simplifying error handling
Use@contextlib.contextmanagerdecorator for straightforward procedural setup/teardown with linear control flow and no persistent state
Implement__enter__and__exit__methods in a class-based context manager when internal state or lifecycle logic spans methods, or when you need...
Files:
tests/workflows/test_action_behaviors.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/workflows/test_action_behaviors.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*test*.py: Use pytest idioms. Prefer fixtures to setup/teardown methods. Parametrize broadly. Avoid unnecessary mocks
Group related tests usingclasswith method names prefixed bytest_
Write tests from a user's perspective. Test public behaviour, not internals
Avoid mocking too much. Prefer test doubles only for external services or non-deterministic behavioursIn tests, use
pytest.raises()with specific exception types and optional regex matchers to constrain expected exceptions, avoiding overly broad exception assertions (B017)
Files:
tests/workflows/test_action_behaviors.py
🧠 Learnings (1)
📚 Learning: 2025-12-27T17:51:29.917Z
Learnt from: leynos
Repo: leynos/shared-actions PR: 219
File: cargo_utils.py:40-41
Timestamp: 2025-12-27T17:51:29.917Z
Learning: In Python files that have 'from __future__ import annotations' (which makes type annotations be evaluated as strings), avoid creating runtime references to types just by annotating attributes. For types used only in annotations or docstrings, import them under a TYPE_CHECKING guard instead of at module import time. Do runtime imports only for actual usage (e.g., Path(...) construction, isinstance checks, classmethod calls, or default values that reference the type). For cargo_utils.py in leynos/shared-actions, ensure runtime behavior is not affected by type-only imports and prefer 'if TYPE_CHECKING' or local imports inside functions where the type is needed at runtime.
Applied to files:
tests/workflows/test_action_behaviors.py
🧬 Code graph analysis (1)
tests/workflows/test_action_behaviors.py (1)
tests/workflows/conftest.py (2)
ActConfig(56-62)run_act(65-123)
⏰ 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). (5)
- GitHub Check: python-tests-windows
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: python-tests (macos-15)
- GitHub Check: build-release (windows-latest, aarch64-pc-windows-gnullvm)
- GitHub Check: build-release (windows-latest, x86_64-pc-windows-gnullvm)
🔇 Additional comments (9)
tests/workflows/test_action_behaviors.py (8)
1-9: Well-documented module with correct en-GB spelling.The docstring clearly explains the test suite's purpose, prerequisites, and execution instructions.
11-21: Import structure follows best practices.Proper use of
TYPE_CHECKINGguard forPathsince it's only used in type annotations. Based on learnings, this avoids unnecessary runtime imports.
24-27: Simple and effective fixture.
30-54: Clean helper with informative failure messages.The assertion includes the full logs on failure, which aids debugging. NumPy docstring format is correctly applied.
57-72: Useful helper reducing assertion duplication.The
flags: inttype is acceptable sincere.RegexFlagis anIntFlag. For stricter typing, usere.RegexFlaginstead, but this is optional.
75-115: Well-structured parametrized test with flexible patterns.Good consolidation of the three event scenarios into a single parametrized test. The regex patterns correctly handle format variations (hyphens/underscores, different delimiters).
118-139: Appropriate assertions verifying non-empty metadata values.Using
\S+to ensure values are non-empty is a good approach.
142-169: Consistent test structure with appropriate assertions..github/actions/upload-release-assets/scripts/upload_release_assets.py (1)
1-330: Excellent implementation—all past review concerns resolved.The code demonstrates high quality throughout:
- All dependencies correctly specified (syspath-hack present, cyclopts version updated).
- Boolean coercion properly centralised to bool_utils.
- Dry-run output accurately reflects the clobber parameter.
_is_candidateuses straightforward if statements rather than unnecessary pattern matching._write_outputdefensively creates parent directories before writing.- Comprehensive error handling for discovery and upload failures.
- Clear separation of concerns with well-focused helper functions.
- Thorough documentation and appropriate type hints.
The implementation is production-ready.
Replace direct re.search calls with the shared _assert_log_patterns helper for consistency. Remove unnecessary capturing group from the uploaded-count pattern (\d+ instead of (\d+)). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Migrate four reusable GitHub Actions from leynos/netsuke:
Also adds shared cargo_utils.py for Cargo.toml parsing, now used by both export-cargo-metadata and ensure-cargo-version actions.
All actions include comprehensive tests (147 new tests total) and follow the established patterns: PEP 723 headers, cyclopts CLI, and plumbum for subprocess execution.
🤖 Generated with Claude Code
Summary by Sourcery
Migrate shared Cargo manifest and boolean parsing utilities, and add new reusable GitHub Actions for release metadata, staging, mode determination, and asset upload.
New Features:
Enhancements:
Tests: