Add Python and Rust lint tiers to template#10
Merged
Merged
Conversation
Import the PyPy-backed Pylint tier after Ruff, add Rust Clippy, Whitaker, and nextest-aware test wiring for generated Rust extension projects, and update the stub modules so the stricter gates pass. Validate generated Python-only and Rust projects through their public Makefile gates so template regressions fail in the template test suite.
Keep the template branch focused on source and configuration changes by removing a committed Python bytecode artefact from the test tree.
Keep the template branch focused on source and configuration changes by removing committed Python bytecode and ignoring local pytest cache output.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughModernise the copier template: add a reusable Makefile, refactor tests to run generated quality gates ( ChangesTemplate System Modernisation
Sequence Diagram(s)sequenceDiagram
participant Tester as "pytest harness"
participant Act as "act (local runner)"
participant GeneratedCI as "Rendered CI workflow"
participant ArtifactDir as "artifact-server-path"
Tester->>Act: run `act pull_request -j lint-test --json --artifact-server-path`
Act->>GeneratedCI: execute workflow steps
GeneratedCI->>ArtifactDir: upload `coverage.xml` (Cobertura)
GeneratedCI->>Tester: emit JSON logs parsed by tests
Possibly Related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1827e1339
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@README.md`:
- Line 14: Replace the ambiguous phrase "Pylint through the PyPy shim" in
README.md with explicit wording clarifying whether Pylint is executed under PyPy
or via a wrapper; update the text to something like "Pylint (via a PyPy-backed
runner)" or "Pylint run under the PyPy interpreter" depending on the actual
implementation, ensuring the original phrase "Pylint through the PyPy shim" is
removed and the chosen clarification is used inline with the list that includes
"Ruff", "`ty`", and "pytest".
- Around line 15-16: Replace the ambiguous sentence "Ensure these tools are
available before running `pytest`" in README.md with a clarified paragraph that
states the test suite requires only the `pytest-copier` plugin to render and
validate generated projects, and that generated projects themselves install and
run their own tooling (Ruff, Pylint via PyPy, `ty`, pytest, and—if Rust is
enabled—Clippy, Whitaker, and nextest); specifically locate the sentence
fragment "Ensure these tools are available before running `pytest`" and swap it
for the proposed clarification so readers know to install only the
`pytest-copier` test dependency, not all listed tools.
In `@template/.github/workflows/ci.yml.jinja`:
- Around line 29-45: Add a caching step before the "Install Rust lint and test
tools" step to avoid re-installing cargo-nextest and whitaker-installer every CI
run: insert a new job step after the "Set up Rust" step that uses a cache action
(e.g., Swatinem/rust-cache@v2 or actions/cache) configured to cache Rust tool
installs and cargo bin targets so subsequent runs restore the cache and skip
re-installation; reference the existing step names "Set up Rust" and the tools
"cargo-nextest" and "whitaker-installer" when placing and configuring the cache
step.
In `@template/Makefile.jinja`:
- Around line 19-33: The TEST_CMD detection can silently succeed when CARGO is
just the literal "cargo" even if cargo is not installed, causing later test
execution to fail; add an explicit cargo availability check (e.g., define a
CARGO_AVAILABLE variable that runs command -v $(CARGO) or similar) and use it in
the test target or when computing TEST_CMD to error out with a clear message if
cargo is missing, updating references to CARGO, TEST_CMD and the test target to
bail early with a helpful message rather than falling back to an unusable
"cargo" invocation.
In `@tests/test_template.py`:
- Around line 45-47: The three bare assertions in tests/test_template.py —
assert (proj / "rust_extension").exists(), assert (proj / "docs" /
"rust-extension.md").exists(), and assert "maturin" in (proj /
"pyproject.toml").read_text() — should include descriptive assertion messages;
update each to use the form assert <condition>, "<message>" with clear messages
like "rust_extension directory should exist for Rust template", "Rust
documentation should be generated for Rust template", and "maturin should be in
pyproject.toml for Rust template" so failures produce helpful output.
- Around line 62-64: Add descriptive assertion messages to the two bare asserts
in tests/test_template.py: update the assertion checking (proj /
"rust_extension").exists() to include a message like "rust_extension directory
should exist for custom package Rust template" and update the assertion
verifying "custom_pkg" in text (from pyproject.toml) to include a message like
"custom package name should appear in pyproject.toml"; modify the assertions in
the same test where these symbols appear so failures report clear reasons.
- Around line 29-31: The three bare assertions should include descriptive
messages: update the assertions that reference (proj /
"rust_extension").exists(), (proj / "docs" / "rust-extension.md").exists(), and
"maturin" not in (proj / "pyproject.toml").read_text() to use the assert ...,
"message" form so failures state which condition failed (e.g., indicate that the
rust_extension directory, the rust-extension.md doc, or the maturin entry in
pyproject.toml should not exist for the Python-only template).
🪄 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: 2d5aec83-2a51-4573-80d9-eb605b1d8503
📒 Files selected for processing (15)
.gitignoreREADME.mdscripts/setup_test_deps.shtemplate/.github/workflows/ci.yml.jinjatemplate/Makefiletemplate/Makefile.jinjatemplate/pyproject.toml.jinjatemplate/tests/test_stub.py.jinjatemplate/{% if use_rust %}rust_extension{% endif %}/Cargo.toml.jinjatemplate/{% if use_rust %}rust_extension{% endif %}/clippy.tomltemplate/{% if use_rust %}rust_extension{% endif %}/src/lib.rs.jinjatemplate/{{ package_name }}/__init__.py.jinjatemplate/{{ package_name }}/pure.pytests/conftest.pytests/test_template.py
💤 Files with no reviewable changes (1)
- template/Makefile
Replace the generated workflow's manual coverage step with the shared `leynos/shared-actions` coverage action and shared Rust setup action. Keep Rust-extension projects on a cargo manifest path that works from the Python project root. Add an opt-in `act` pytest harness that renders Python-only and Rust-extension projects, runs the generated CI workflow as a black-box integration test, and asserts that coverage and template tests are exercised. Record the current `act` composite-action archive limitation as a narrow xfail after coverage is produced.
Clarify README wording around the PyPy-backed Pylint runner and the local pytest dependency. Cache Rust tool installs in generated CI, make generated Rust tests fail early when cargo is unavailable, and add assertion messages to template validation tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_github_actions_integration.py`:
- Around line 139-180: The two near-duplicate tests
(test_python_only_workflow_runs_with_shared_coverage_action and
test_rust_extension_workflow_runs_with_shared_coverage_action) should be
collapsed into a single parametrized test; create a fixture act_ready that calls
require_act() and inject it into the new
test_generated_workflow_runs_with_shared_coverage_action, parameterize over
(name, package, use_rust, artifact_dir), update copier.copy to use tmp_path /
package and the parameterized values, call prepare_git_repository(project) and
run_act(project, artifact_dir=tmp_path / artifact_dir), and conditionally assert
the Rust-specific line in the workflow only when use_rust is True; ensure you
reuse symbols prepare_git_repository, run_act, and assert_act_result and place
the act_ready fixture in conftest.py or fixtures per guidelines.
- Around line 57-59: The test builds an "act" command and always appends
"--container-daemon-socket" with env.get("DOCKER_HOST", "") which can pass an
empty string; change the logic in tests/test_github_actions_integration.py where
the command list is constructed (variable name "command") to only append the
flag when env.get("DOCKER_HOST") returns a non-empty value: fetch docker_host =
env.get("DOCKER_HOST") and if docker_host truthy then extend command with
["--container-daemon-socket", docker_host], otherwise do not add the flag.
🪄 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: f4943c00-0c52-4b4c-b027-4bdc032dc5f5
📒 Files selected for processing (5)
pytest.initemplate/.github/workflows/ci.yml.jinjatemplate/docs/local-validation-of-github-actions-with-act-and-pytest.mdtests/fixtures/pull_request.event.jsontests/test_github_actions_integration.py
Group the generated Pylint `enable` list into the requested logging, pattern matching, control-flow, iterator, runtime, text hygiene, mutation, and complexity categories without changing the rule set.
Reviewer's GuideImports Python (Ruff, PyPy-backed Pylint, ty) and Rust (Clippy, Whitaker, nextest) lint/test tiers into the Python template, wires CI coverage to a shared composite action, and adds black-box GitHub Actions validation via act and pytest plus corresponding config, docs, and tests updates. Sequence diagram for CI workflow with shared coverage and Rust tierssequenceDiagram
actor Dev
participant GitHub_Actions as GitHubActions_ci.yml
participant Setup as setup_steps
participant Make as make_targets
participant Coverage as generate_coverage_action
participant CodeScene as CodeScene_CLI
Dev->>GitHub_Actions: push or pull_request
GitHub_Actions->>Setup: actions/checkout
GitHub_Actions->>Setup: setup-uv
alt use_rust
GitHub_Actions->>Setup: leynos/setup-rust
GitHub_Actions->>Setup: rust-cache
GitHub_Actions->>Setup: install whitaker-installer
end
GitHub_Actions->>Setup: uv tool install mbake
GitHub_Actions->>Make: make lint
GitHub_Actions->>Make: make typecheck
GitHub_Actions->>Make: make test
Note over Make: runs ruff, PyPy-backed pylint, ty, pytest
alt use_rust
Note over Make: also runs cargo doc, clippy, whitaker, nextest
end
GitHub_Actions->>Coverage: leynos/shared-actions generate-coverage
Note over Coverage: uses coverage.xml
alt use_rust
Note over Coverage: cargo-manifest=rust_extension/Cargo.toml
end
GitHub_Actions->>CodeScene: CodeScene Coverage CLI
CodeScene-->>GitHub_Actions: coverage upload status
Flow diagram for new Makefile lint and test tiersflowchart LR
Dev[Developer runs make all] --> All[all]
All --> Build["build (uv sync dev deps)"]
All --> CheckFmt[check-fmt]
All --> Lint[lint]
All --> Typecheck[typecheck]
All --> Test[test]
Lint --> RuffCheck[ruff check]
Lint --> PylintPyPy[pylint-pypy via PyPy]
Typecheck --> TyCheck[ty check]
Test --> Pytest[pytest -v -n auto]
subgraph Rust_extension_enabled[use_rust == true]
Lint --> RustDoc[cargo doc]
Lint --> Clippy[cargo clippy]
Lint --> Whitaker[whitaker --all]
Test --> Nextest[cargo nextest run]
Test --> RustDocTests[cargo test --doc]
end
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc991f9bda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Make generated Rust lint targets depend on a `whitaker` target that no-ops when Whitaker is already present and otherwise installs the pinned Whitaker installer before running the local lint gate. Document the generated `make all` quality-gate flow with an accessible Mermaid caption so local Python and Rust gate behaviour is visible from the README.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@template/Makefile.jinja`:
- Around line 86-97: The whitaker target may call whitaker-installer before
~/.cargo/bin is on PATH; update the whitaker target so the post-install
invocation runs with a PATH prefixed by $(USER_BIN_PATH) (e.g., invoke
whitaker-installer with PATH="$(USER_BIN_PATH):$$PATH" whitaker-installer
--cranelift) to ensure the newly installed binary is resolvable; keep the
existing cargo install invocation and use the same variables $(WHITAKER),
$(CARGO_AVAILABLE), $(CARGO), and $(WHITAKER_INSTALLER_REV).
🪄 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: 6660e7bb-f984-4e0a-a18a-6de01b3ab6e4
📒 Files selected for processing (2)
README.mdtemplate/Makefile.jinja
Clean generated uv cache directories, allow act validation to use Docker or Podman, and tighten structured act log matching so Python, Rust, and coverage signals must come from the shared coverage step. Expand Markdown acronyms in the local GitHub Actions validation guide.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@template/Makefile.jinja`:
- Around line 131-133: The Makefile target uses ensure_tool with the literal
"nixie", ignoring the NIXIE variable and any overrides; update the nixie target
to call ensure_tool with the configured command by passing $(NIXIE) (or the
appropriate expansion) instead of the hard-coded name so the runtime check
validates the actual command in the NIXIE variable; change the ensure_tool
invocation in the nixie target from ensure_tool,nixie to ensure_tool,$(NIXIE)
and keep the subsequent use of $(NIXIE) --no-sandbox.
In `@tests/test_github_actions_integration.py`:
- Line 153: Several bare asserts in tests/test_github_actions_integration.py
(notably the check for (project / "coverage.xml").exists() and the asserts
around lines 184, 207, 208) lack messages; update each bare assert to include a
clear failure message so CI failures are actionable. Locate the assert
expressions (e.g., the assertion verifying coverage.xml existence and the
subsequent file/path asserts) and replace them with assertions that include a
concise explanatory string (for example describing which file or workflow
artifact is missing) so test failures report exactly what was not found.
🪄 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: e8eba249-193d-4cec-b2b0-ea9d27b99536
📒 Files selected for processing (3)
template/Makefile.jinjatemplate/docs/local-validation-of-github-actions-with-act-and-pytest.mdtests/test_github_actions_integration.py
Add module documentation for template tests, split generated linting into language-specific targets, and document the generated quality gate design in user, developer, and ADR docs. Add focused generated-file assertions for Makefile lint target structure, clean-up paths, and cargo failure messaging. Keep act log parsing separated from workflow assertions so JSON format handling stays isolated.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/adr-001-template-quality-gates.md`:
- Around line 9-12: The context statement sentence "Generated projects need one
public local gate that is simple to run, while still keeping Python and Rust
tooling independently understandable. The template also needs generated GitHub
Actions workflows to match the local gate closely enough that failures are
predictable." is awkward; replace it with a single clear sentence such as:
"Generated projects must include one public local gate that is simple to run and
preserve independent clarity for Python and Rust tooling, and the template
should generate GitHub Actions workflows that closely match the local gate so
failures are predictable." Ensure en‑GB spelling/grammar (oxendict) is used when
applying the rewrite.
In `@tests/test_template.py`:
- Around line 10-15: The test currently invokes individual make targets; update
run_quality_gates to run the public aggregate target instead by replacing the
sequence of project.run("make check-fmt"), project.run("make lint"),
project.run("make typecheck"), project.run("make test") with a single
project.run("make all") call so the generated project is validated through the
same public entrypoint; locate the function run_quality_gates (and its use of
the CopierProject instance) and change the calls accordingly.
🪄 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: 54e621d3-d54d-4ecc-b2b3-08004e1c2e73
📒 Files selected for processing (6)
docs/adr-001-template-quality-gates.mddocs/developers-guide.mdtemplate/Makefile.jinjatemplate/docs/users-guide.md.jinjatests/test_github_actions_integration.pytests/test_template.py
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/test_github_actions_integration.py (2)
67-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOmit
--container-daemon-socketwhen DOCKER_HOST is not configured.Do not pass
--container-daemon-socketwith an empty string. The flag exhibits unreliable behaviour on some systems when passed via CLI with an empty value. Omit the flag entirely unlessDOCKER_HOSTis explicitly set.Proposed fix
command = [ "act", "pull_request", "-j", "lint-test", "-e", str(EVENT), "-P", ACT_IMAGE, "--artifact-server-path", str(artifact_dir), "--json", "-b", - "--container-daemon-socket", - env.get("DOCKER_HOST", ""), ] + docker_host = env.get("DOCKER_HOST") + if docker_host: + command.extend(["--container-daemon-socket", docker_host])🤖 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_github_actions_integration.py` around lines 67 - 68, The test currently always passes the "--container-daemon-socket" flag with env.get("DOCKER_HOST", "") which can result in an empty-string CLI arg; change the construction so you only append the flag and its value when env.get("DOCKER_HOST") returns a non-empty value (e.g., assign host = env.get("DOCKER_HOST") and if host: extend the args with "--container-daemon-socket" and host), ensuring no empty-string flag is passed.
180-228: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCollapse duplicated act tests into a single parametrized test.
Replace the two near-identical tests with
@pytest.mark.parametrizeand moverequire_act()into a fixture. This reduces duplication and aligns with project guidelines.Proposed refactor
+@pytest.fixture +def act_ready() -> None: + """Skip test if act environment is unavailable.""" + require_act() + + `@pytest.mark.act` -def test_python_only_workflow_runs_with_shared_coverage_action( - copier: CopierFixture, tmp_path: Path -) -> None: - """Validate the Python-only generated CI workflow through act.""" - require_act() - project = copier.copy( - tmp_path / "act-pure", - project_name="ActPure", - package_name="act_pure", - use_rust=False, - ) - prepare_git_repository(project) - - code, logs = run_act(project, artifact_dir=tmp_path / "pure-artifacts") - - assert ( - "leynos/shared-actions/.github/actions/generate-coverage" - in ( - project / ".github" / "workflows" / "ci.yml" - ).read_text() - ), "Python-only workflow should use the shared generate-coverage action" - assert_act_result(project, code, logs, use_rust=False) - - -@pytest.mark.act -def test_rust_extension_workflow_runs_with_shared_coverage_action( - copier: CopierFixture, tmp_path: Path +@pytest.mark.parametrize( + ("name", "package", "use_rust", "artifact_subdir"), + [ + ("ActPure", "act_pure", False, "pure-artifacts"), + ("ActRust", "act_rust", True, "rust-artifacts"), + ], +) +def test_workflow_runs_with_shared_coverage_action( + copier: CopierFixture, + tmp_path: Path, + act_ready: None, + name: str, + package: str, + use_rust: bool, + artifact_subdir: str, ) -> None: - """Validate the Rust-extension generated CI workflow through act.""" - require_act() + """Validate the generated CI workflow through act.""" project = copier.copy( - tmp_path / "act-rust", - project_name="ActRust", - package_name="act_rust", - use_rust=True, + tmp_path / package, + project_name=name, + package_name=package, + use_rust=use_rust, ) prepare_git_repository(project) - - code, logs = run_act(project, artifact_dir=tmp_path / "rust-artifacts") + code, logs = run_act(project, artifact_dir=tmp_path / artifact_subdir) workflow = (project / ".github" / "workflows" / "ci.yml").read_text() - assert ( - "leynos/shared-actions/.github/actions/generate-coverage" in workflow - ), "Rust workflow should use the shared generate-coverage action" - assert ( - "cargo-manifest: rust_extension/Cargo.toml" in workflow - ), "Rust workflow should pass the Rust extension manifest to coverage" - assert_act_result(project, code, logs, use_rust=True) + assert "leynos/shared-actions/.github/actions/generate-coverage" in workflow, \ + "workflow should use the shared generate-coverage action" + if use_rust: + assert "cargo-manifest: rust_extension/Cargo.toml" in workflow, \ + "Rust workflow should pass the Rust extension manifest to coverage" + assert_act_result(project, code, logs, use_rust=use_rust)As per coding guidelines: "Use
pytestfixtures for shared setup" and "Replace duplicate tests with@pytest.mark.parametrize".🤖 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_github_actions_integration.py` around lines 180 - 228, Collapse the two tests test_python_only_workflow_runs_with_shared_coverage_action and test_rust_extension_workflow_runs_with_shared_coverage_action into one parametrized test (e.g., `@pytest.mark.parametrize`("use_rust,artifact_subdir,expected_manifest", [(False,"pure-artifacts",None),(True,"rust-artifacts","cargo-manifest: rust_extension/Cargo.toml")]) that calls copier.copy with use_rust, prepares the git repo, runs run_act and asserts the shared action string is in the workflow and, when use_rust is True, also asserts the expected_manifest is present, then calls assert_act_result with use_rust; remove direct require_act() calls from the test and instead create a small fixture that invokes require_act() (and is used by the parametrized test) so shared setup lives in a fixture; keep references to functions copier.copy, run_act, assert_act_result and the workflow variable to locate where to change the code.template/Makefile.jinja (1)
92-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrefix PATH when invoking
whitaker-installerafter cargo install.The
whitaker-installerbinary is installed to~/.cargo/bin, which may not be on PATH immediately aftercargo install. Prefix the invocation with the user binary paths to ensure the newly installed binary is found.Proposed fix
$(CARGO) install --locked \ --git https://github.com/leynos/whitaker \ --rev "$(WHITAKER_INSTALLER_REV)" \ whitaker-installer; \ - whitaker-installer --cranelift; \ + PATH="$(USER_BIN_PATH):$(PATH)" whitaker-installer --cranelift; \ fi🤖 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 92 - 97, The Makefile invokes `whitaker-installer` immediately after `$(CARGO) install` but doesn't ensure the user cargo bin dir is on PATH; update the invocation that currently reads `whitaker-installer --cranelift` to run with the cargo bin directory prefixed (e.g. use `PATH="$(CARGO_HOME)/bin:$$PATH" whitaker-installer --cranelift` or call `$(CARGO_HOME)/bin/whitaker-installer --cranelift`) so the freshly installed binary is found; modify the block containing the `$(CARGO) install --locked ... whitaker-installer; \` and the subsequent `whitaker-installer --cranelift; \` line accordingly.
🤖 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 `@template/Makefile.jinja`:
- Around line 50-51: The build-release Makefile target currently calls python
directly; change the recipe in the build-release target to invoke the virtualenv
wrapper variable UV by using "$(UV) run python -m build --sdist --wheel" so it
runs inside the same environment as the other targets (update the build-release
target to use $(UV) run rather than plain python).
---
Duplicate comments:
In `@template/Makefile.jinja`:
- Around line 92-97: The Makefile invokes `whitaker-installer` immediately after
`$(CARGO) install` but doesn't ensure the user cargo bin dir is on PATH; update
the invocation that currently reads `whitaker-installer --cranelift` to run with
the cargo bin directory prefixed (e.g. use `PATH="$(CARGO_HOME)/bin:$$PATH"
whitaker-installer --cranelift` or call `$(CARGO_HOME)/bin/whitaker-installer
--cranelift`) so the freshly installed binary is found; modify the block
containing the `$(CARGO) install --locked ... whitaker-installer; \` and the
subsequent `whitaker-installer --cranelift; \` line accordingly.
In `@tests/test_github_actions_integration.py`:
- Around line 67-68: The test currently always passes the
"--container-daemon-socket" flag with env.get("DOCKER_HOST", "") which can
result in an empty-string CLI arg; change the construction so you only append
the flag and its value when env.get("DOCKER_HOST") returns a non-empty value
(e.g., assign host = env.get("DOCKER_HOST") and if host: extend the args with
"--container-daemon-socket" and host), ensuring no empty-string flag is passed.
- Around line 180-228: Collapse the two tests
test_python_only_workflow_runs_with_shared_coverage_action and
test_rust_extension_workflow_runs_with_shared_coverage_action into one
parametrized test (e.g.,
`@pytest.mark.parametrize`("use_rust,artifact_subdir,expected_manifest",
[(False,"pure-artifacts",None),(True,"rust-artifacts","cargo-manifest:
rust_extension/Cargo.toml")]) that calls copier.copy with use_rust, prepares the
git repo, runs run_act and asserts the shared action string is in the workflow
and, when use_rust is True, also asserts the expected_manifest is present, then
calls assert_act_result with use_rust; remove direct require_act() calls from
the test and instead create a small fixture that invokes require_act() (and is
used by the parametrized test) so shared setup lives in a fixture; keep
references to functions copier.copy, run_act, assert_act_result and the workflow
variable to locate where to change the code.
🪄 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: abe067e6-3e8c-407d-9ed9-68b0995b87c3
📒 Files selected for processing (2)
template/Makefile.jinjatests/test_github_actions_integration.py
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. Please address the comments from this code review: |
This comment was marked as resolved.
This comment was marked as resolved.
Make the generated-file helper match its contract by passing an explicit UTF-8 encoding to `Path.read_text`.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8837 🏁 Script executed: Length of output: 7800
Overall Comments
Individual Comments
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Collapse the Python-only and Rust workflow validation cases into one parameterized test and move the act readiness check into shared pytest fixture setup. Avoid passing an empty container socket argument to act when DOCKER_HOST is unset.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Share the test Docker environment helper and canonicalise accepted unix socket values before forwarding them to act. Try both Docker and Podman runtime probes before skipping act validation, and make generated `pyproject.toml` reads explicit UTF-8 reads.
Canonicalise the fallback Podman socket before exposing it through `DOCKER_HOST`, and keep accepted sockets inside the current user's runtime subtree.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3311 🏁 Script executed: Length of output: 444 All encoding fixes and the DOCKER_HOST validation are resolved. The one outstanding item is the 🏁 Script executed: Length of output: 58
|
Expand the public test helper and fixture docstrings into structured NumPy-style documentation, including module-level usage notes. Quote the generated import check as a Python script passed through `shlex.quote` so package names are imported dynamically rather than interpolated into shell code.
Run release builds through `uv run` so they use the generated project's managed environment. Prefix the user cargo bin directory when invoking `whitaker-installer` so a freshly installed binary is available immediately.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@template/Makefile.jinja`:
- Around line 103-105: Add a cargo-availability guard and call it before every
Rust-invoking recipe: define ensure_cargo (uses CARGO_AVAILABLE/CARGO) and
invoke it at the top of the fmt, check-fmt, and lint-rust targets so any
invocation of $(CARGO) or Rust steps (references: fmt target, check-fmt target,
lint-rust target, and the $(CARGO) symbol) first runs $(call ensure_cargo) to
fail fast with a clear diagnostic when cargo is missing.
🪄 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: 13926b66-f248-44a1-8f13-6bc86bc0a5da
📒 Files selected for processing (1)
template/Makefile.jinja
Add a shared `ensure_cargo` recipe guard and run it before generated Rust formatting and linting commands invoke `$(CARGO)`.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add a shared `ensure_uv` guard before generated targets invoke `$(UV)`, and make virtualenv tool checks depend on the synced build target.
Owner
Author
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@template/Makefile.jinja`:
- Around line 171-173: The help target currently uses awk with FS=":" which
leaves prerequisites and the "##" marker in $$2; update the awk field separator
to FS=":.*##" so the second field contains only the description for targets like
build: .venv ## Build..., and trim leading whitespace from the description
(e.g., with gsub) before printing; locate and modify the help target block using
the "help" recipe in the Makefile.jinja to implement these changes.
🪄 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: a2c91af8-d06e-42fc-a0d0-1c2b4c1d3a5f
📒 Files selected for processing (1)
template/Makefile.jinja
✅ Actions performedReviews paused. |
Parse generated help descriptions after the `##` marker so prerequisite text is not included in help output. Add syrupy to the parent test setup and snapshot the generated Python-only `make help` output to lock the formatting contract.
This was referenced May 24, 2026
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 Python and Rust lint contracts into the Python project template so generated projects exercise the same lint and test tiers that the source repositories use. Python projects now run Ruff followed by PyPy-backed Pylint, and Rust-extension projects add Clippy, Whitaker, and nextest-aware Rust tests after the Python gates.
The generated CI workflow now delegates coverage to
leynos/shared-actionsthrough the sharedgenerate-coverageaction, with Rust-extension renders passingrust_extension/Cargo.tomlas the cargo manifest. The branch also adds black-box local GitHub Actions validation throughactand pytest for Python-only and Rust-extension template renders.This update also addresses review feedback by clarifying README wording, caching generated Rust CI tool installs, failing generated Rust tests early when cargo is missing, installing Whitaker before local generated Rust linting, cleaning generated uv state, respecting the configured Nixie command, relaxing local
actvalidation for Docker or Podman CLIs, tighteningactstructured log matching, adding action-test assertion messages, reading generated files as UTF-8 in tests, splitting generated lint targets intolint-pythonandlint-rust, documenting user/developer quality-gate behaviour, recording the quality-gate ADR, validating generated projects throughmake all, adding generated-file assertions for edge cases, grouping the generated Pylintenablerules by category, and documenting the generatedmake allgate flow.No linked issue, roadmap task, or execplan was found for this branch.
Review walkthrough
ty, pytest, and the conditional Rust Clippy, Whitaker, and nextest-aware test targets.enablerules, currentmaturinbound, and generated build-backend fixes.make allvalidation, UTF-8 generated-file reads, and generated-file assertions that cover Makefile target structure, clean-up paths, and cargo failure messaging.actlog matching, the parametrized shared coverage workflow test, and action-test assertion messages.actvalidation approach.Validation
pytest -q: passed,3 passed in 110.37s; log:/tmp/test-agent-template-python-import-lints.out.pytest -q: passed after bytecode cleanup,3 passed in 111.15s; log:/tmp/test-agent-template-python-import-lints-pr-bytecode-removal.out.pytest -q: passed after shared coverage action tests were added,3 passed, 2 skipped in 112.89s; log:/tmp/test-agent-template-python-import-lints-final.out.RUN_ACT_VALIDATION=1 pytest -q tests/test_github_actions_integration.py::test_python_only_workflow_runs_with_shared_coverage_action: xfailed after the generated workflow ran Python tests and producedcoverage.xml; log:/tmp/act-python-agent-template-python-import-lints.out.RUN_ACT_VALIDATION=1 pytest -q tests/test_github_actions_integration.py::test_rust_extension_workflow_runs_with_shared_coverage_action: xfailed after the generated workflow observed the Rust and Python test paths and producedcoverage.xml; log:/tmp/act-rust-agent-template-python-import-lints.out.pytest -q: passed after workflow validation review fixes,3 passed, 2 skipped in 106.43s; log:/tmp/test-agent-template-python-import-lints-code-review-comments.out.RUN_ACT_VALIDATION=1 pytest -q tests/test_github_actions_integration.py -m act: xfailed at the known shared-action archive boundary for both generated workflows,2 xfailed in 631.81s; log:/tmp/act-agent-template-python-import-lints-code-review-comments.out.pytest -q: passed after architecture review fixes,3 passed, 2 skipped in 109.01s; log:/tmp/test-agent-template-python-import-lints-architecture-review.out.pytest -q: passed aftermake allfollow-up commit,3 passed, 2 skipped in 128.19s; log:/tmp/test-agent-template-python-import-lints-make-all-followup-post-commit.out.pytest -q: passed after assertion-message follow-up commit,3 passed, 2 skipped in 110.39s; log:/tmp/test-agent-template-python-import-lints-assert-messages-post-commit.out.pytest -q: passed after UTF-8 read follow-up,3 passed, 2 skipped in 113.71s; log:/tmp/test-agent-template-python-import-lints-utf8-read.out.pytest -q: passed after UTF-8 read follow-up commit,3 passed, 2 skipped in 112.02s; log:/tmp/test-agent-template-python-import-lints-utf8-read-post-commit.out.git diff --check HEAD: passed with no output; log:/tmp/diff-check-agent-template-python-import-lints-utf8-read-post-commit.out.git push: passed; log:/tmp/push-agent-template-python-import-lints-utf8-read.out.pytest -q: passed after parametrizingactworkflow tests,3 passed, 2 skipped in 119.44s; log:/tmp/test-agent-template-python-import-lints-act-parametrize.out.RUN_ACT_VALIDATION=1 pytest -q tests/test_github_actions_integration.py -m act: xfailed at the known shared-action archive boundary for both parametrized generated workflows,2 xfailed in 606.10s; log:/tmp/act-agent-template-python-import-lints-act-parametrize.out.pytest -q: passed after parametrizedactworkflow test commit,3 passed, 2 skipped in 111.48s; log:/tmp/test-agent-template-python-import-lints-act-parametrize-postcommit.out.RUN_ACT_VALIDATION=1 pytest -q tests/test_github_actions_integration.py -m act: xfailed at the known shared-action archive boundary for both parametrized generated workflows,2 xfailed in 622.06s; log:/tmp/act-agent-template-python-import-lints-act-parametrize-postcommit.out.git diff --check HEAD: passed with no output after parametrizingactworkflow tests.git push: passed; log:/tmp/push-agent-template-python-import-lints-act-parametrize.out.Notes
maturinlatest as1.13.3on PyPI andpyo3latest as0.28.3on crates.io before the dependency updates were applied.cargo metadatato resolve from a directory containing aCargo.toml.actxfail is intentionally narrow: the local workflows reached the generated Python/Rust tests and coverage output, then failed in the sharedgenerate-coveragecomposite action output/archive phase withParameter INPUT_ARTEFACT_NAME_SUFFIX specified multiple timesand an empty upload-artifact name underact. This is retained as an explicit local-runner limitation rather than treating unrelated workflow failures as acceptable.