Adopt Python quality gates (ruff, pylint, interrogate, ty) - #435
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
SummaryAdd Python quality gates with pinned Ruff, Pylint, interrogate, and ty tooling.
WalkthroughChangesQuality and workflow hardening
Possibly related PRs
Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 inconclusive)
✅ Passed checks (15 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix: Run leynos/shared-actions@927edd4 |
This comment was marked as resolved.
This comment was marked as resolved.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
7548d38 to
d56c359
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1e27821 to
4dda8eb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
4dda8eb to
a8b24a0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
a8b24a0 to
119a83f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? scripts/rotate_session_key.py (1)
Proposed fix- "Zero-downtime rotation requires at least 2 replicas.",
+ f"Zero-downtime rotation requires at least "
+ f"{MIN_REPLICAS_FOR_ZERO_DOWNTIME} replicas.",🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In scripts/sync_workspace_members.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions scripts/local_k8s/cluster.py (1)
Proposed fix-@dc.dataclass(frozen=True)
+@dc.dataclass(frozen=True, slots=True)
class _ProviderCommandSpec:🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== File outlines ==='
ast-grep outline scripts/rotate_session_key.py --items all --match 'replica|MIN_REPLICAS' || true
ast-grep outline scripts/sync_workspace_members.py --items all --match '_calculate_bracket_depth_change|_find_members_array_bounds' || true
ast-grep outline scripts/local_k8s/cluster.py --items all --match '_ProviderCommandSpec' || true
printf '%s\n' '=== Replica threshold and warning ==='
rg -n -C 8 'MIN_REPLICAS_FOR_ZERO_DOWNTIME|Zero-downtime rotation requires' scripts/rotate_session_key.py
printf '%s\n' '=== Private helper definitions and docstrings ==='
rg -n -A 55 -B 3 \
'def _calculate_bracket_depth_change\b|def _find_members_array_bounds\b' \
scripts/sync_workspace_members.py
printf '%s\n' '=== Provider command specification ==='
rg -n -A 18 -B 4 \
'class _ProviderCommandSpec\b|`@dc`\.dataclass' \
scripts/local_k8s/cluster.pyLength of output: 6195
Mark these findings as follows:
✏️ Learnings added
You are interacting with an AI system. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0c8ba71 to
85cc423
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. scripts/warm_pg_embedded_cache_test.py Comment on file ],
ids=["missing-marker", "non-executable-postgres", "complete-cache"],
)
def test_cache_is_complete(❌ New issue: Excess Number of Function Arguments |
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add a config-only root `pyproject.toml` (no `[project]` table, so uv keeps treating the helper scripts as non-project scripts) carrying the Ruff, Pylint, interrogate, and ty rule sets ported from leynos/lading (primary template) and leynos/prosidy-darn, and bring every Python file under `scripts/` and `tests/workflow_contracts/` up to those standards: - Ruff: import conventions (`typ`/`dc`/`cabc` aliases), type-checking blocks, exception-message locals (`EM`/`TRY`), docstring layout, line length, and formatting across 27 files. - interrogate: docstring coverage on `scripts/` raised from 78.5% to 100% (docstrings added to `sync_workspace_members.py`, `warm_pg_embedded_cache_test.py`, and friends). - Pylint (PyPy-shim rule families): implicit-booleaness and match-pattern fixes; rating 10.00/10. - ty: literal narrowing casts in `local_k8s/config.py` and isinstance narrowing in the workflow-contract tests. Targeted suppressions, each justified in place: `E402` in `test_properties.py` (deliberate `pytest.importorskip` ordering), `S404`/`S603`/`S607` at the two intentional subprocess boundaries, and `DTZ011` for the wall-clock review-date check. No global rule was weakened and no behaviour changed; all 83 script tests and 6 workflow contract tests pass unchanged.
Mirror the leynos/lading Makefile shapes: - `lint-python` runs Ruff 0.15.12, interrogate `--fail-under 100` over `scripts/`, and Pylint on a uv-managed PyPy via the pylint-pypy shim; it joins the `lint` aggregate. - `fmt` gains `ruff format` and `ruff check --select I --fix`; `check-fmt` gains `ruff format --check`, with a standalone `check-fmt-python` target for a discrete CI step. - `typecheck-python` (a `typecheck` prerequisite) materializes the script dependencies into `.venv` and runs `ty check --python-version 3.13` over all Python sources. A materialized venv is required because ty cannot discover the layered environment that `uv run --with` builds; `[tool.ty.environment]` supplies the `scripts/` search path. - Existing bare `uv run` invocations gain `--no-project` so the new root `pyproject.toml` cannot change their semantics (uv would otherwise sync a phantom project and write a `uv.lock`). CI gets `make check-fmt-python` and `make lint-python` steps; the existing `make typecheck` step picks up the ty gate automatically. uv downloads a managed PyPy on demand, so no separate CI install step is needed. Ignore `.venv/` and `.ruff_cache/`.
The root pyproject.toml added by this branch is configuration-only (Ruff, Pylint, interrogate, and ty settings, no [project] table). The generate-coverage action's auto-detection treats any root pyproject.toml as a Python project, so it classified Wildside as mixed; mixed runs only support Cobertura, which would break the Rust-only lcov.info that CodeScene consumes. leynos/shared-actions#373 added a backwards-compatible `language` input to the action. Set `language: rust` on both `Generate Rust coverage` steps so the detector emits lang=rust and keeps the lcov path, and bump only the generate-coverage pin to the merged shared-actions SHA (074f7d8ba75a6e5d18532b72cbe38fccbda4e9c6) that carries the new input. The setup-rust and upload-codescene-coverage pins stay at 18bed1ca to remain in lockstep with audit.yml and mutation-testing.yml, which this change does not touch. The ci workflow contract now asserts language: rust in the coverage mapping while retaining the LCOV/CodeScene assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The local-k8s Make targets now pass `uv run --no-project` so the root pyproject.toml this branch adds cannot change their resolution semantics. The preview CLI fake matched a fixed argv (`args[:2] == ["run", "scripts/local_k8s.py"]`), so the extra option made it report an unexpected command and the make-target smoke tests failed. Skip leading `uv run` options before matching the script path, mirroring the existing `_unwrap` handling for `env` and `systemd-run` wrappers, and exec the resolved script arguments. The fake now tracks the Makefile's invocation rather than one fixed argv, so future option changes do not break it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six review findings, each verified against the current tree before acting. Pin the shared Python test dependencies. pytest, pytest-mock, hypothesis, cryptography, and tomli were bare package names, so typecheck-python and test-scripts could resolve different versions from one run to the next. Pin them with the Makefile's dominant `FOO_VERSION ?=` style at the versions already resolving today, and factor the shared set into PY_TEST_DEPS used by both targets — the comment above PY_TYPECHECK_DEPS claims the versions mirror test-scripts, which only holds if both sites share one definition. Give _ProviderCommandSpec `slots=True` alongside `frozen=True`. Every other non-exempt dataclass under scripts/ already pairs them, and no usage relies on per-instance __dict__. Interpolate MIN_REPLICAS_FOR_ZERO_DOWNTIME into the replica warning rather than hard-coding 2, so the message cannot drift from the threshold the adjacent check enforces. Document the public helpers in sync_workspace_members.py and check_redoc_ignore.py with NumPy Examples sections, using doctest skips where an example would touch the filesystem or manifest state; format_members is pure, so its example executes. Correct main()'s Returns description in sync_workspace_members.py: it returns 0 on success but does not always return, because SystemExit from _find_members_array_bounds propagates through update_manifest uncaught. Collapse the two private helpers to one-line summaries, matching the convention every other private helper under scripts/ follows. _find_members_array_bounds keeps its Raises section, since dropping it would discard behaviour callers depend on. Also widen _assert_command_logged's predicate parameter to Sequence. list is invariant, so the narrowed list read back out of a log entry did not satisfy list[object] once this branch's ty gate began covering the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Python quality gates and the Rust-only coverage selection had no contract protecting them, so a later edit could drop a gate step or re-enable Python coverage detection without any test noticing. Assert in the CI contract that the build job drives each Python gate through its Makefile target, so CI and local runs cannot diverge onto separate definitions. Add a coverage-main contract covering the workflow this branch changed. It pins the reviewed generate-coverage commit, fixes the full Rust-only ratcheted LCOV input mapping, and ties the CodeScene upload to the report the generation step writes, including the immutable upload-codescene-coverage pin. Extend the Makefile tooling contracts with recipe-level coverage of check-fmt-python, lint-python, and typecheck-python, and with command-level coverage of every target that moved to isolated uv execution. The tests reuse the existing command doubles, so they assert the observable invocations — which runner, which flags, which targets — without downloading or running the real tools. The typecheck assertions also require each declared dependency to carry a version constraint, guarding the pins restored earlier on this branch, and require the separately gated spelling helper to stay outside the repository-wide surface. Document the gates in the developers' guide: what each target runs, that the aggregate lint and typecheck targets include them, and why the uv invocations pass --no-project and the coverage workflows pass language: rust. Both follow from the root pyproject.toml being tooling configuration rather than a managed uv project, which is the part a future reader is most likely to undo by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five review findings, each verified against the current tree first. Bound both PyYAML declarations to `>=6,<7`. An open-ended floor would admit PyYAML 7 into the typecheck environment and the workflow-contract runner without review. The contract test now rejects any unbounded range rather than merely requiring some constraint, so the next dependency added cannot reintroduce an open floor. Pin the interpreter `typecheck-python` builds. An unpinned `uv venv` takes whichever Python uv resolves first, which on this machine is 3.14 while ty analyses the sources as 3.13 — so ty resolved a standard library it was not checking against. Both the venv and ty's `--python-version` now read PY_TYPECHECK_VERSION, making the match structural rather than a coincidence of two literals. The contract test asserts that relationship instead of a version literal, so it keeps holding when the version moves. The review asked for 3.14 here on the grounds that ty already analyses as that version. It does not: `--python-version 3.13`, `[tool.ty.environment]`, ruff's target-version, and requires-python all say 3.13, so 3.14 would have widened the mismatch rather than closed it. Correct the `discover_members` docstring example, which showed unsorted output for a function that returns `sorted(...)`. Explain the S607 suppression in the Bash test harness. The justification is a plain comment, not a second `# noqa:` directive, which Ruff would flag as unused. Stop the mutation-testing contract normalizing every falsey `workflow_dispatch` value to an empty mapping. Only `None` is YAML's shorthand for an empty mapping; treating a list or scalar the same way let malformed workflow input pass the mapping assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bound the workflow-contract runner's pytest requirement, which was the last open-ended floor in the Makefile. The review asked for `>=8,<9`. That would have downgraded this runner from pytest 9.1.1 to 8.4.2 — PYTEST_VERSION is 9.1.1 and every other Python target resolves 9.x, so `<9` would have left the contract suite alone on pytest 8. `>=8,<10` closes the unbounded major without moving the resolved version. The contract test now checks every `--with` requirement, not just the typecheck dependency set, so an unbounded floor cannot reappear in an ad-hoc runner. Lower Pylint's max-module-lines from 800 to 400 and split the two modules that exceeded it. Both were test modules, split along the seams already present in them: - scripts/local_k8s/unittests/test_deployment.py (505) keeps the deploy, build, and Helm tests; the session-secret tests and their helpers move to test_session_secret.py, which is what they actually exercise. - scripts/warm_pg_embedded_cache_test.py (677) keeps version normalization, cache locking, and cache-state tests. The checksum, download, release-URL, and end-to-end paths move to warm_pg_embedded_cache_download_test.py, which keeps them with the curl stub and archive fixtures they share. The Bash harness both modules need (run_bash, result_diagnostics, and the script paths) moves to warm_pg_embedded_cache_support.py rather than being duplicated. No test was added, removed, or rewritten: the counts are unchanged at 105 for the preview suite and 29 for the warm-up suite. The 800-line threshold was a deliberate choice carried over from leynos/lading, so the earlier round left it alone; with the split done, 400 holds with the largest remaining module at 395 lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The upper bounds on pytest and PyYAML read as arbitrary without the reasoning, and pytest's in particular invites being "corrected" to <9. Note at the shared-pin block that PyYAML stays a range because no target needs a particular 6.x, and that the ceiling exists to keep 7.0 out until reviewed. Note at the workflow-contract runner that it sits outside PY_TEST_DEPS on purpose, and that pytest's ceiling is <10 rather than <9 because PYTEST_VERSION pins 9.1.1 everywhere else: <9 would hold this suite on pytest 8 and split the repository across two majors. Both comments sit above their targets rather than inside a recipe, which would push the body past checkmake's maxbodylength limit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaying the Python quality-gates section onto main left a doubled blank line before each of its three headings, which MD012 rejects. The section reads the same; only the stray separators go. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three cache_is_complete tests differed only in whether the .complete marker existed, the mode on the postgres binary, and the expected result; each repeated the same six lines of directory setup. Fold them into one parametrized test with named ids, so the cases read as a table and a fourth is a row rather than another copy of the setup. The failure cases now assert the exact return code instead of merely non-zero. cache_is_complete is a single `[[ -f ... && -x ... ]]` test, so it can only return 0 or 1; both failure modes were confirmed to return 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeScene flagged test_cache_is_complete for excess arguments: parametrizing four fields alongside tmp_path put it at five, one over the configured limit. Carry the four fields in a frozen, slotted _CacheCompletenessCase instead, so the test takes only tmp_path and case. The three scenarios, their expected exit statuses, and their pytest ids are unchanged, as is the shell command under test. The cases are constructed with keyword arguments. Passing has_complete_marker positionally trips Ruff's FBT003 boolean-trap rule, and naming the fields also keeps the table readable now that a row spans several lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3a7943c to
498c019
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
2 participants
Summary
This branch imports the Python quality tooling used by leynos/lading and
leynos/prosidy-darn — Ruff, Pylint (PyPy-backed), interrogate, and ty — and
wires it into the Makefile and CI so the repository's Python helper scripts
are held to the same standard as those reference projects. The scripts under
scripts/andtests/workflow_contracts/had never been linted as a gate;this branch both establishes the gates and brings every file into
conformance, with docstring coverage on the shipped helper code raised from
78.5% to 100% and a Pylint rating of 10.00/10.
The work lands as two commits:
adds the rule sets and conforms the code.
turns the gates on.
Review walkthrough
for the imported rule sets. It is deliberately config-only (no
[project]table) so uv keeps treating the helper scripts as non-project scripts; the
vendored spelling-rollout helper is excluded because
make spelling-helper-testgates it separately with its own pinned Ruff.Python tooling block and the
lint-python,check-fmt-python, andtypecheck-pythontargets.typecheck-pythonmaterialises dependenciesinto
.venvbecause ty cannot discover the layered environment thatuv run --withbuilds; existing bareuv runinvocations gain--no-projectso the new rootpyproject.tomlcannot alter theirsemantics.
adds discrete
Python format checkandPython lintsteps; the existingmake typecheckstep picks up ty automatically. Pylint runs on auv-managed PyPy (downloaded on demand) via the pylint-pypy shim, mirroring
leynos/lading.
scripts/local_k8s/
and the top-level scripts adopt the import conventions (
typ/dc/cabcaliases), type-checking blocks, exception-message locals, and missing
docstrings. Behaviour is unchanged: error message text and CLI flags are
preserved verbatim.
the global rule set:
E402intest_properties.py
for the deliberate
pytest.importorskipordering,S404/S603/S607atthe intentional subprocess boundaries, and
DTZ011for the wall-clockreview-date check in
check_redoc_ignore.py.
Validation
Post-rebase, on this exact tree:
make check-fmt: passes (Rust, Biome,ruff format --check).make typecheck: passes (ty over all Python sources,tscover the TS workspaces).make test: passes — nextest 1438 passed/4 skipped, vitest 90, bun 43,15 workflow-contract tests, 100 local-preview script tests.
make lint: the Python/architecture/Biome/specs/Makefile/actions tiers allpass.
lint-whitakerfails onbackend/src/domain/jobs/generate_route/tests.rs:312(
no_expect_outside_testsingenerate_route_job_strategy). That file isbyte-identical to the base branch and this PR changes no Rust; Define job structs for GenerateRouteJob and EnrichmentJob (5.2.2) #376's own CI
fails the same "Whitaker lint" step, so it is pre-existing there and owned by
that branch.
Notes
ty 0.0.59, interrogate 1.7.0, and a pinned pylint-pypy shim commit) so
local and CI runs cannot skew.
mainafter the Podman/kind preview work((7.1) Support Podman and kind previews #378) merged; the conformance commit was re-applied over that final
merged code, including its new
kind.pyandsession_secret.pymodules.Rebased onto the 5.2.2 job-structs branch
This PR now targets
backend-5-2-2-job-structs-for-generate-route-and-enrichment(#376) ratherthan
main, and has been rebased onto it.Notes from that rebase:
patched the same advisories and gone further:
brace-expansion5.0.9 (vs the5.0.7 here),
fast-uri3.1.5 (vs 3.1.4), and an npm-visibleresolutionsblock that lets Bun audit consume the patched versions directly — which in
turn let Define job structs for GenerateRouteJob and EnrichmentJob (5.2.2) #376 retire most of the Bun exception ledger. Every package pinned
here had an equal-or-newer counterpart there, so this branch no longer
touches
package.json,pnpm-lock.yaml, orsecurity/audit-exceptions.json.the Python gates this branch enables:
tests/workflow_contracts/ nixie_toolchain_test.py(import conventions, andpytest.fail()in place ofassert False) andtests/workflow_contracts/makefile_tooling_test.py(
S404boundary annotation, formatting). This is the same "bring every fileinto conformance" work the first commit already does.
local-k8s-*Make targets now combine both branches' intent:$(UV) run --no-project ...keeps Define job structs for GenerateRouteJob and EnrichmentJob (5.2.2) #376's injectable$(UV)(its newmakefile_tooling_test.pydepends on it) alongside the--no-projectthisbranch needs so the new root
pyproject.tomlcannot changeuv runsemantics. The final commit teaches the preview-CLI fake to skip
uv runoptions so it tracks the Makefile rather than one fixed argv.
Rust LCOV coverage fix (added after review)
This branch also fixes a coverage regression its own root
pyproject.tomlwould otherwise cause. The config-only
pyproject.toml(no[project]table)made the shared
generate-coverageaction's auto-detection classify Wildsideas mixed, and mixed runs only support Cobertura — breaking the Rust-only
lcov.infothat CodeScene consumes.(merged,
074f7d8ba75a6e5d18532b72cbe38fccbda4e9c6) adds abackwards-compatible
languageinput (auto/rust/python/mixed); theforced
python/mixedprerequisite matches the action's realuv sync --inexact --pythoncontract (a[project]table), so a tooling-onlypyproject.tomlno longer forces a Python run.Generate Rust coveragesteps setlanguage: rust, and only thegenerate-coveragepin is bumped to the merged SHA (thesetup-rustandupload-codescene-coveragepins stay at18bed1cato remain in lockstep withaudit.yml/mutation-testing.yml). The CI workflow contract now assertslanguage: rustwhile retaining its LCOV/CodeScene assertions.References