Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request substantially expands testing infrastructure for the Rust Copier template by introducing shared test helpers, audit gates, and act-based GitHub Actions local validation. Changes span parent repository test configuration, documentation, CI workflows, and a comprehensive suite of rendered template tests with focused tooling contracts. ChangesTesting Infrastructure, Audit Gates, and Act Validation
Possibly Related Issues
Possibly Related PRs
Suggested Labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
leynos
marked this pull request as ready for review
June 3, 2026 15:56
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
leynos
changed the title
leynos
marked this pull request as draft
June 3, 2026 18:19
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
June 3, 2026 18:41
Reviewer's GuideAdds a generated Rust workspace dependency audit gate, refactors template tests into shared helpers, and introduces opt-in GitHub Actions validation via act while tightening Makefile and CI contracts for doctests, coverage, and tooling. Sequence diagram for the opt-in act-based GitHub Actions validation in testssequenceDiagram
actor Developer
participant Makefile
participant Pytest
participant Act
Developer->>Makefile: make test
Makefile->>Pytest: $(UV) --with pytest-copier --with pyyaml --with syrupy --with make-parser pytest tests/
alt RUN_ACT_VALIDATION not set
Pytest-->>Developer: test results
else RUN_ACT_VALIDATION=1
Pytest->>Act: act
Act-->>Pytest: act results
Pytest-->>Developer: test + act results
end
Flow diagram for the generated Rust workspace audit gateflowchart LR
Developer[Developer]
CI[GitHub_Actions_ci_job]
MakeAudit[make_audit_target]
RustAudit[make_rust-audit_target]
CargoMetadata[cargo_metadata]
PythonExtract[python3_manifest_extractor]
CargoAudit[cargo_audit]
Developer --> MakeAudit
CI --> MakeAudit
MakeAudit --> RustAudit
RustAudit --> CargoMetadata
CargoMetadata --> PythonExtract
PythonExtract --> CargoAudit
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of tests/test_helpers.py for socket/utilities tests and format mismatch test
cat tests/test_helpers.py 2>/dev/null || echo "FILE NOT FOUND"
echo "---"
# Check tests/utilities.py to confirm its current content
cat tests/utilities.py 2>/dev/null || echo "FILE NOT FOUND"Length of output: 16880
Comment 1 — All ten socket/utilities unit tests are present in
Comment 2 — The annulment instruction applies to the original suggestions' American spellings (e.g. "normalize", "normalized") — those were in the suggestions, not binding requirements on the implementation. |
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (2 errors, 3 warnings, 2 inconclusive)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of the relevant files
echo "=== tests/helpers/tooling_contracts.py - _parse_makefile_rules usage ==="
rg -n "_parse_makefile_rules\|_load_and_parse_makefile_rules" tests/helpers/tooling_contracts.py
echo ""
echo "=== tests/utilities.py - docker_environment current implementation ==="
cat tests/utilities.py
echo ""
echo "=== docs/users-guide.md - check for make audit ==="
cat docs/users-guide.md 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== tests/helpers/tooling_contracts.py - private helper usage ==="
rg -n "^def _\|^ def _" tests/helpers/tooling_contracts.py
echo ""
echo "=== Check which private helpers are actually called ==="
rg -n "_assert_cargo_package_contracts\|_assert_makefile_contracts\|_assert_cargo_config_contracts\|_assert_ci_workflow_contracts\|_assert_release_workflow_contracts\|_parse_makefile_rules\|_load_and_parse_makefile_rules" tests/helpers/tooling_contracts.py
echo ""
echo "=== tests/test_github_actions_integration.py - timing/debug markers ==="
rg -n "time\|timing\|debug\|phase\|logging\|logger" tests/test_github_actions_integration.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== Check for existing GitHub issues related to these items ==="
gh issue list --repo leynos/agent-template-rust --state open --limit 20Length of output: 8602 🏁 Script executed: #!/bin/bash
echo "=== File existence check ==="
fd --type f "tooling_contracts.py"
fd --type f "test_github_actions_integration.py"
echo ""
echo "=== Direct cat of tooling_contracts.py ==="
cat tests/helpers/tooling_contracts.py 2>/dev/null | head -100 || echo "FILE NOT FOUND"
echo ""
echo "=== Count private functions and check for _parse_makefile_rules ==="
grep -n "^def _\|^ def _\|_parse_makefile_rules\|_load_and_parse" tests/helpers/tooling_contracts.py 2>/dev/null || echo "NO MATCHES OR FILE MISSING"
echo ""
echo "=== All function definitions in tooling_contracts.py ==="
grep -n "^def " tests/helpers/tooling_contracts.py 2>/dev/null || echo "NO MATCHES OR FILE MISSING"
echo ""
echo "=== Check test_github_actions_integration.py for timing ==="
grep -n "time\.\|perf_counter\|monotonic\|debug\|phase" tests/test_github_actions_integration.py 2>/dev/null || echo "FILE NOT FOUND OR NO MATCHES"Length of output: 6772 🏁 Script executed: #!/bin/bash
echo "=== Imports in test_github_actions_integration.py ==="
grep -n "^import\|^from" tests/test_github_actions_integration.py
echo ""
echo "=== Call sites of private helpers within tooling_contracts.py ==="
grep -n "_assert_cargo_package_contracts\|_assert_makefile_contracts\|_assert_cargo_config_contracts\|_assert_ci_workflow_contracts\|_assert_release_workflow_contracts" tests/helpers/tooling_contracts.py
echo ""
echo "=== assert_generated_tooling_contracts body (lines 96-242) ==="
sed -n '96,242p' tests/helpers/tooling_contracts.pyLength of output: 7098
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
template/Makefile.jinja (1)
38-40: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPrefer the positive condition
{% if flavour == 'lib' %}for clarity.The change from
{% if flavour == 'lib' %}to{% if flavour != 'app' %}broadens the gate unnecessarily. If only two flavours exist (libandapp), the expressions are equivalent, but the negative condition is less explicit about intent. Retain the positive match unless a third flavour is anticipated that should also run doctests.♻️ Proposed reversion to positive condition
-{% if flavour != 'app' %} +{% if flavour == 'lib' %} RUSTFLAGS="$(RUST_FLAGS)" $(CARGO) test --doc --workspace --all-features {% endif %}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@template/Makefile.jinja` around lines 38 - 40, The conditional in the template uses a negative check "{% if flavour != 'app' %}" which is broader than intended; change it to the positive match "{% if flavour == 'lib' %}" so the doctest invocation (the RUSTFLAGS="$(RUST_FLAGS)" $(CARGO) test --doc --workspace --all-features line) only runs for the lib flavour; update the if-statement around that command to use "flavour == 'lib'" instead of "flavour != 'app'".tests/test_template.py (1)
1-521: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftDecompose the test module to comply with the 400-line limit.
The file contains 521 lines, significantly exceeding the 400 logical-line limit. Split into focused test modules by concern to improve maintainability and readability.
♻️ Proposed decomposition
Restructure into a subpackage:
tests/test_template/ ├── __init__.py ├── conftest.py # Shared fixtures (_cargo_metadata_for, _write_fake_cargo) ├── test_basic_rendering.py # Lines 36-83 (flavour rendering) ├── test_makefile_validation.py # Lines 86-97 (Makefile validation) ├── test_lint_targets.py # Lines 100-176 (Clippy, Whitaker) ├── test_compilation.py # Lines 178-190 (cargo check) ├── test_doctests.py # Lines 193-232 (doctest regression) ├── test_audit_targets.py # Lines 235-385 (audit tests) ├── test_tooling_contracts.py # Lines 388-446 (contract validation) └── test_snapshots.py # Lines 449-479 (snapshot tests)Move the private helpers
_cargo_metadata_forand_write_fake_cargointoconftest.pyas pytest fixtures to share across the audit test modules.As per coding guidelines, files must not exceed 400 logical lines and should be decomposed when necessary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_template.py` around lines 1 - 521, The module exceeds the 400-line limit; split it into a tests/test_template package and move shared helpers into conftest.py: create new files (e.g. test_basic_rendering.py for test_template_renders/test_template_renders_app_flavour/test_template_renders_lib_flavour, test_makefile_validation.py for test_makefile_validates, test_lint_targets.py for test_clippy_runs and test_makefile_resolves_whitaker_fallback, test_compilation.py for test_template_compiles, test_doctests.py for test_library_make_test_runs_doctests, test_audit_targets.py for test_makefile_rust_audit_* tests, test_tooling_contracts.py for test_generated_tooling_contracts, and test_snapshots.py for test_generated_structured_file_snapshots), move the helper functions _cargo_metadata_for and _write_fake_cargo into tests/test_template/conftest.py as fixtures (exporting them or wrapping as fixtures used by audit tests), and update imports in each new test file to reference those helpers and reuse existing test function names (e.g., test_makefile_rust_audit_uses_workspace_metadata, test_makefile_rust_audit_honours_documented_ignores, test_makefile_rust_audit_propagates_failures) so each file stays under 400 logical lines.tests/helpers/tooling_contracts.py (1)
1-592: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftDecompose the module to comply with the 400-line limit.
The file contains 592 lines, exceeding the 400 logical-line limit specified in the coding guidelines. Split the module into a subpackage structure to improve maintainability.
♻️ Proposed decomposition
Restructure
tests/helpers/tooling_contracts.pyinto a subpackage:tests/helpers/tooling_contracts/ ├── __init__.py # Re-export public assert_* functions ├── documentation.py # assert_documentation_navigation_contracts ├── makefile.py # _assert_makefile_contracts, _load_and_parse_makefile_rules ├── cargo.py # _assert_cargo_package_contracts, _assert_cargo_config_contracts ├── workflows.py # _assert_ci_workflow_contracts, _assert_release_workflow_contracts, │ # assert_ci_coverage_action_contract, extract_checkout_steps └── orchestration.py # assert_generated_tooling_contractsUpdate imports in
tests/test_template.pyto reference the public API fromtests.helpers.tooling_contracts.As per coding guidelines, files must not exceed 400 logical lines and should be decomposed into subpackages when necessary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helpers/tooling_contracts.py` around lines 1 - 592, The module tests/helpers/tooling_contracts.py exceeds the 400-line guideline; split it into a subpackage tests/helpers/tooling_contracts with smaller modules and re-exports: move assert_documentation_navigation_contracts to documentation.py; move _assert_makefile_contracts and _load_and_parse_makefile_rules to makefile.py; move _assert_cargo_package_contracts and _assert_cargo_config_contracts to cargo.py; move _assert_ci_workflow_contracts, _assert_release_workflow_contracts, assert_ci_coverage_action_contract and extract_checkout_steps to workflows.py; keep orchestration/assert_generated_tooling_contracts in orchestration.py which imports the split helpers; add __init__.py to re-export the public assert_* functions so existing imports (e.g. tests/test_template.py) can continue to import from tests.helpers.tooling_contracts without code changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_helpers.py`:
- Line 13: Replace the aliased import "import tests.utilities as utilities" with
a direct module import "import tests.utilities" so it conforms to PLR0402;
update any references if needed but note existing monkeypatch calls referencing
utilities will still work unchanged.
---
Outside diff comments:
In `@template/Makefile.jinja`:
- Around line 38-40: The conditional in the template uses a negative check "{%
if flavour != 'app' %}" which is broader than intended; change it to the
positive match "{% if flavour == 'lib' %}" so the doctest invocation (the
RUSTFLAGS="$(RUST_FLAGS)" $(CARGO) test --doc --workspace --all-features line)
only runs for the lib flavour; update the if-statement around that command to
use "flavour == 'lib'" instead of "flavour != 'app'".
In `@tests/helpers/tooling_contracts.py`:
- Around line 1-592: The module tests/helpers/tooling_contracts.py exceeds the
400-line guideline; split it into a subpackage tests/helpers/tooling_contracts
with smaller modules and re-exports: move
assert_documentation_navigation_contracts to documentation.py; move
_assert_makefile_contracts and _load_and_parse_makefile_rules to makefile.py;
move _assert_cargo_package_contracts and _assert_cargo_config_contracts to
cargo.py; move _assert_ci_workflow_contracts,
_assert_release_workflow_contracts, assert_ci_coverage_action_contract and
extract_checkout_steps to workflows.py; keep
orchestration/assert_generated_tooling_contracts in orchestration.py which
imports the split helpers; add __init__.py to re-export the public assert_*
functions so existing imports (e.g. tests/test_template.py) can continue to
import from tests.helpers.tooling_contracts without code changes.
In `@tests/test_template.py`:
- Around line 1-521: The module exceeds the 400-line limit; split it into a
tests/test_template package and move shared helpers into conftest.py: create new
files (e.g. test_basic_rendering.py for
test_template_renders/test_template_renders_app_flavour/test_template_renders_lib_flavour,
test_makefile_validation.py for test_makefile_validates, test_lint_targets.py
for test_clippy_runs and test_makefile_resolves_whitaker_fallback,
test_compilation.py for test_template_compiles, test_doctests.py for
test_library_make_test_runs_doctests, test_audit_targets.py for
test_makefile_rust_audit_* tests, test_tooling_contracts.py for
test_generated_tooling_contracts, and test_snapshots.py for
test_generated_structured_file_snapshots), move the helper functions
_cargo_metadata_for and _write_fake_cargo into tests/test_template/conftest.py
as fixtures (exporting them or wrapping as fixtures used by audit tests), and
update imports in each new test file to reference those helpers and reuse
existing test function names (e.g.,
test_makefile_rust_audit_uses_workspace_metadata,
test_makefile_rust_audit_honours_documented_ignores,
test_makefile_rust_audit_propagates_failures) so each file stays under 400
logical lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: daca2eb5-2cb7-40da-95a9-593a0ca3538b
📒 Files selected for processing (16)
.github/workflows/ci.ymlMakefileREADME.mddocs/developers-guide.mddocs/users-guide.mdtemplate/.github/workflows/ci.ymltemplate/Makefile.jinjatests/__snapshots__/test_template.ambrtests/conftest.pytests/helpers/generated_files.pytests/helpers/tooling_contracts.pytests/test_github_actions_integration.pytests/test_helpers.pytests/test_parent_ci.pytests/test_template.pytests/utilities.py
Move the large tooling contract helper and rendered template test module into focused packages while preserving the existing public helper imports. Tighten the generated Makefile doctest condition to the library flavour and use a direct `tests.utilities` import in helper tests.
Owner
Author
|
@coderabbitai Have the following now been resolved? template/Makefile.jinja (1)
♻️ Proposed reversion to positive condition-{% if flavour != 'app' %}
+{% if flavour == 'lib' %}
RUSTFLAGS="$(RUST_FLAGS)" $(CARGO) test --doc --workspace --all-features
{% endif %}🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In tests/test_template.py (1)
♻️ Proposed decompositionAs per coding guidelines, files must not exceed 400 logical lines and should be decomposed when necessary. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In tests/helpers/tooling_contracts.py (1)
♻️ Proposed decompositionAs per coding guidelines, files must not exceed 400 logical lines and should be decomposed into subpackages when necessary. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== template/Makefile.jinja - doctest condition ==="
grep -n "flavour\|doctest\|test --doc" template/Makefile.jinja
echo ""
echo "=== tests/test_template.py - line count ==="
wc -l tests/test_template.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== tests/test_template/ directory (if it exists) ==="
ls -la tests/test_template/ 2>/dev/null || echo "DIRECTORY NOT FOUND"
echo ""
echo "=== tests/helpers/tooling_contracts.py - line count ==="
wc -l tests/helpers/tooling_contracts.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== tests/helpers/tooling_contracts/ directory (if it exists) ==="
ls -la tests/helpers/tooling_contracts/ 2>/dev/null || echo "DIRECTORY NOT FOUND"Length of output: 2677
🧠 Learnings used |
Add a library target to generated application projects so `cargo test --doc` has doctests to compile instead of failing on a binary-only package. Make the rendered `make test` doctest invocation unconditional and extend the regression test to cover both app and library flavours.
Move Act-enabled template tests into separate parent and rendered `act-validation.yml` workflows. Keep the main CI workflows on the ordinary test and coverage path so container-backed Act checks can run in parallel.
Owner
Author
|
@coderabbitai Have the following now been resolved? tests/helpers/tooling_contracts.py (2)
🛡️ Proposed fixfor step in job.get("steps", [])
if isinstance(step, dict)
- and step.get("uses", "").startswith("actions/checkout@")
+ and str(step.get("uses", "")).startswith("actions/checkout@")
]🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (2 errors, 1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Check if tooling_contracts is now a package ==="
ls -la tests/helpers/tooling_contracts/ 2>/dev/null || echo "DIRECTORY NOT FOUND"
ls tests/helpers/tooling_contracts.py 2>/dev/null || echo "FLAT FILE NOT FOUND"
echo ""
echo "=== extract_checkout_steps - find and show implementation ==="
rg -n "extract_checkout_steps\|startswith.*checkout\|uses.*checkout" tests/ -A3 -B1
echo ""
echo "=== Public function docstrings - assert_generated_tooling_contracts ==="
rg -n "def assert_generated_tooling_contracts\|def assert_documentation_navigation_contracts\|def assert_ci_coverage_action_contract\|def extract_checkout_steps" tests/ -A 15Length of output: 1594 🏁 Script executed: #!/bin/bash
echo "=== workflows.py - full content ==="
cat tests/helpers/tooling_contracts/workflows.py
echo ""
echo "=== orchestration.py - full content ==="
cat tests/helpers/tooling_contracts/orchestration.py
echo ""
echo "=== __init__.py - full content ==="
cat tests/helpers/tooling_contracts/__init__.pyLength of output: 14582
|
This comment was marked as resolved.
This comment was marked as resolved.
Adapt the generated quality gate flow chart from the Python template README for Rust projects. Show the generated Make targets, Rust lint/test/audit branches, coverage upload path, and separate Act validation workflow.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)
82-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument new test modules and fixtures.
Add brief entries for the new test modules and fixtures introduced in this PR. Insert after line 96:
tests/conftest.pydefines theact_readyfixture that probes container runtime availability before act-based workflow tests.tests/test_parent_ci.pyvalidates parent CI workflow contracts: main workflow excludes act setup; act-validation workflow includes act installation and WITH_ACT=1.tests/test_github_actions_integration.pyruns rendered act-validation workflows through act and asserts black-box Rust test execution evidence from JSON event logs.📝 Proposed addition
Container-aware support for optional `act` tests lives in `tests/utilities.py`. Direct helper edge-case tests live in `tests/test_helpers.py`; rendered project tests should use the shared helper APIs instead of reimplementing file parsing or workflow schema checks. + +Direct test modules for parent repository contracts: + +- `tests/conftest.py` defines the `act_ready` fixture that probes container + runtime availability before act-based workflow tests. +- `tests/test_parent_ci.py` validates parent CI workflow contracts: main + workflow excludes act setup; act-validation workflow includes act + installation and WITH_ACT=1. +- `tests/test_github_actions_integration.py` runs rendered act-validation + workflows through act and asserts black-box Rust test execution evidence + from JSON event logs.As per coding guidelines, "New or changed internal APIs, architectural boundaries or abstractions, tooling or build requirements must be clearly documented in the developer's guide".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/developers-guide.md` around lines 82 - 96, Add documentation entries after the existing Test Helper Layout section describing the new test modules and fixtures: document that tests/conftest.py defines the act_ready fixture which probes container runtime availability, tests/test_parent_ci.py validates parent CI workflow contracts (main workflow excludes act setup; act-validation workflow includes act installation and WITH_ACT=1), and tests/test_github_actions_integration.py runs rendered act-validation workflows through act and asserts black-box Rust test execution evidence from JSON event logs; keep the style consistent with existing bullet entries in the Test Helper Layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/act-validation.yml:
- Around line 3-7: Add a top-level concurrency block to the workflow to prevent
redundant runs: under the workflow root (same level as the existing "on:" key)
add a "concurrency" key with a unique group identifier (for example using the
repo and branch/PR number or workflow name) and set "cancel-in-progress: true"
so any in-progress act-validation run is canceled when a newer run starts;
ensure the group string references context like github.ref or github.head_ref to
differentiate PRs/branches.
In `@README.md`:
- Around line 62-64: Add a comma after "workflow" in the sentence "Parent and
generated-project CI run this mode in a separate `act-validation.yml` workflow
so container-backed workflow checks run in parallel with the normal test and
coverage workflow." so it reads "... a separate `act-validation.yml` workflow,
so container-backed ..." to improve readability; update the README.md line
containing that sentence accordingly.
In `@template/docs/repository-layout.md.jinja`:
- Around line 73-74: The docs claim `src/lib.rs` "contains ... doctested
examples" but the generated `src/lib.rs` stub only defines `greet()` with no
doctest; either add a doctest example to the `greet()` function in `src/lib.rs`
(e.g., a /// doc comment with a code block showing `assert_eq!(greet(), "...")`)
or change the README line referencing `src/lib.rs` to say it "contains library
support for application logic with space for doctested examples" so the wording
matches the current stub.
In `@template/src/`{% if flavour == 'app' %}lib.rs{% endif %}.jinja:
- Around line 4-6: The greet() function lacks a doctest example as promised by
the docs; add a minimal doctest above pub const fn greet() that shows usage and
an assertion—include the Examples section and a fenced code block calling
greet() and asserting it equals "Hello from {{ project_name }}!" (use the crate
import form with {{ package_name }}::greet) so generated lib.rs demonstrates
doctest conventions.
In `@tests/helpers/tooling_contracts/makefile.py`:
- Line 12: The parameter "flavour" on _assert_makefile_contracts is unused;
either remove it from the function signature and from all callers (update the
call in the orchestration module that passes the flavour) or keep it and add a
short docstring/TODO explaining its reserved purpose and use it in future
flavour-specific checks; modify the function _assert_makefile_contracts and the
call site in the orchestration module accordingly so signatures match.
In `@tests/test_template/conftest.py`:
- Line 32: Update the write_fake_cargo fixture's return type to be explicit to
match cargo_metadata_for: change the annotation from Callable[..., Path] to
Callable[[Path, list[Path]], Path] so the factory signature is clear (or, if the
factory truly has a complex keyword-only signature with defaults, add a brief
inline comment on write_fake_cargo explaining why Callable[..., Path] is
intentionally used). Reference the write_fake_cargo fixture and
cargo_metadata_for for consistency.
In `@tests/test_template/test_snapshots.py`:
- Around line 43-52: The assertion message omits act_workflow while comparing
five keys; update the assertion's failure message to list all compared variables
(cargo_config, makefile, act_workflow, ci_workflow, release_workflow) so it
accurately reflects the comparison between the constructed dict and snapshot
used in the test (variables: cargo_config, makefile, act_workflow, ci_workflow,
release_workflow).
---
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 82-96: Add documentation entries after the existing Test Helper
Layout section describing the new test modules and fixtures: document that
tests/conftest.py defines the act_ready fixture which probes container runtime
availability, tests/test_parent_ci.py validates parent CI workflow contracts
(main workflow excludes act setup; act-validation workflow includes act
installation and WITH_ACT=1), and tests/test_github_actions_integration.py runs
rendered act-validation workflows through act and asserts black-box Rust test
execution evidence from JSON event logs; keep the style consistent with existing
bullet entries in the Test Helper Layout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80154a66-0f10-4d70-9975-c8a93cee1b9c
📒 Files selected for processing (32)
.github/workflows/act-validation.ymlREADME.mddocs/developers-guide.mddocs/users-guide.mdtemplate/.github/workflows/act-validation.ymltemplate/.github/workflows/ci.ymltemplate/Makefile.jinjatemplate/docs/developers-guide.md.jinjatemplate/docs/repository-layout.md.jinjatemplate/docs/users-guide.md.jinjatemplate/src/{% if flavour == 'app' %}lib.rs{% endif %}.jinjatests/helpers/tooling_contracts/__init__.pytests/helpers/tooling_contracts/cargo.pytests/helpers/tooling_contracts/documentation.pytests/helpers/tooling_contracts/makefile.pytests/helpers/tooling_contracts/orchestration.pytests/helpers/tooling_contracts/workflows.pytests/test_github_actions_integration.pytests/test_helpers.pytests/test_parent_ci.pytests/test_template.pytests/test_template/__init__.pytests/test_template/__snapshots__/test_snapshots.ambrtests/test_template/conftest.pytests/test_template/test_audit_targets.pytests/test_template/test_basic_rendering.pytests/test_template/test_compilation.pytests/test_template/test_doctests.pytests/test_template/test_lint_targets.pytests/test_template/test_makefile_validation.pytests/test_template/test_snapshots.pytests/test_template/test_tooling_contracts.py
💤 Files with no reviewable changes (3)
- template/.github/workflows/ci.yml
- template/Makefile.jinja
- tests/test_template.py
Use the preferred `from tests import utilities` import form in helper tests and extend the developer guide test-helper layout with the Act fixture, rendered workflow integration test, and parent CI contract test entrypoints.
Owner
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Add concurrency cancellation to the parent Act validation workflow, document the Act helper entrypoints more precisely, and add a doctest example to the rendered app library stub. Remove the unused Makefile contract flavour parameter and clarify the fake cargo fixture typing.
Set `RUSTFLAGS` to an empty value while generated CI installs cargo-nextest, cargo-audit, and Whitaker so inherited compiler flags do not break tool installation. Enforce the cleared environment in the rendered workflow contract.
Move all parent and rendered workflow uses of `leynos/shared-actions` to revision `455d9ed03477c0026da96c2541ca26569a74acac` and update the rendered workflow contracts and snapshots that enforce the pin.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch imports the
act-testsenhancements into the Rust Copier template, adds a generated dependency audit gate, and proves that generatedmake testcompiles library doctests instead of relying oncargo nextest runalone. It also wiresactinto CI at both levels: parent CI runs the full template test suite withWITH_ACT=1, and rendered projects receive a separateact-validationworkflow job so the generated workflow path is checked independently from the main coverage job.The generated audit path is adapted from
leynos/podbot:make auditdelegates tomake rust-audit, derives the Rust workspace root fromcargo metadata, logs workspace member manifests, translates documentedCARGO_AUDIT_IGNORESvalues into repeatedcargo audit --ignoreflags, and runscargo auditonce from the workspace root. Generated CI installscargo-audit, sets up Python for metadata parsing, and callsmake auditbefore linting.The branch also adds deterministic unit coverage for the container socket resolution helpers used by optional
actvalidation. These tests cover malformed Docker socket URLs, allowed unix socket paths, per-user Podman socket derivation, act container socket forwarding, the coverage action format invariant, generated-file schema helper invariants through Hypothesis, and removal of inherited credential-bearing environment variables before invokingact.Latest review follow-ups harden parent test support: tests/utilities.py now builds an allowlisted subprocess environment, tests/helpers/tooling_contracts.py exposes Makefile parser I/O in the helper name, tests/test_github_actions_integration.py xfails current act runtime limitations for upstream
runs.using: node24actions before asserting coverage evidence, and docs/users-guide.md documents generated audit prerequisites.No issue or roadmap task is referenced for this branch.
Review walkthrough
audit,rust-audit,CARGO_AUDIT_IGNORES, and audit phase/error messages.build-testand the separateact-validationjob.act, checks Docker, keepsmake helpindependent fromuvx, and runsmake test WITH_ACT=1.rust-audittests, and the regression provingCARGO_AUDIT_IGNORESbecomes--ignoreflags without network access.actvalidation path, phase markers, timing diagnostics, git failure diagnostics, act compatibility xfails, archive-failure handling, and container runtime checks.Validation
uvx --with pytest-copier --with pyyaml --with syrupy --with make-parser --with hypothesis pytest tests/test_github_actions_integration.py tests/test_helpers.py 2>&1 | tee /tmp/act-helper-tests-agent-template-rust-act-tests.out:28 passed,1 skippedmake test 2>&1 | tee /tmp/test-agent-template-rust-act-tests.out:48 passed,1 skippedbefore commit963d340make test 2>&1 | tee /tmp/test-agent-template-rust-act-tests.out:48 passed,1 skippedafter commit963d340Notes
actworkflow validation path whenRUN_ACT_VALIDATION=1is not set in the local shell. Parent CI now runsmake test WITH_ACT=1, which enables that path in CI.runs.using: node24is addressed by xfail handling before coverage assertions, with a focused regression test for that log shape.docker_environment()only passes a small safe allowlist plus a validatedDOCKER_HOST; inherited values such asGITHUB_TOKEN, API keys, and cloud credentials are not forwarded toact.tests/helpers/tooling_contracts.pyare unused is stale: each private helper is referenced, so no speculative restructure was made.