ci: streamline Python test matrix - #1131
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean CI-only refactor, no blocking findings.
What I checked
- Single file touched:
.github/workflows/ci.yml(+44/-10), a matrix streamlining + pip caching + added job timeouts. - Reviewer verified every Python version still runs tests, the
fail_under=80coverage gate still fires on the 3.12 leg, and gating ruff/mypy to 3.12 loses no coverage since both pintarget py310independent of interpreter. - No public-surface / semver, forward-compat, import-layering, generated-code, ctx_metadata, or CI-gate-disarming concerns triggered — this diff touches none of those load-bearing areas.
- The
if: always()→!cancelled()change referenced in the PR body lands in a later commit (0f0dc37) outside the reviewed head, so it is out of scope, not a defect.
Gate checks
gated_paths: false— row 2 not triggered.high_risk: false, no deletions — rows 3/5 not triggered.- No author no-auto-approve team match — row 7 not triggered.
- Zero findings (no critical/high/medium) — rows 1, 4, 8 not triggered.
review_decision: REVIEW_REQUIRED does not force a downgrade because gated_paths is false. Falls through to row 9 → approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
CI-only refactor to .github/workflows/ci.yml (+47/-11). No blocking findings.
What I checked:
if: always()→if: ${{ !cancelled() }}on the storyboard-required-gate is correct: the storyboard matrix runsfail-fast: false, so a failing leg still fails the aggregate gate while only run-level cancellation now skips it — closing the superseded-run concurrency stall without weakening the gate.- Gating static analysis (ruff/mypy/type-checks) to the 3.12 leg loses no coverage: mypy is pinned to
python_version 3.10and ruff topy310, so results are Python-version-independent. - Job timeouts and pip caching are pure hygiene.
- CI gates remain armed — no test disabled, no ruff rule dropped, no blanket mypy silencing.
No Critical/High/Medium findings. gated_paths is false; no high-risk paths; no author team gate. Falls through to row 9.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
CI-only refactor of .github/workflows/ci.yml (+66/-16, single file, no source or wire surface touched). Verified against the decision table top-down:
- No critical/high/medium findings (row 1, 4, 5, 6, 8 do not fire).
gated_paths: false— row 2 does not apply despitereview_decision: REVIEW_REQUIRED.high_risk: false— no deletion/modification escalation (rows 3, 5).- No no-auto-approve team match (row 7 does not fire).
- Falls through to row 9 → approve.
Reviewer confirmed the storyboard gate move from if: always() to if: ${{ !cancelled() }} (correct idiom — still fails the required check on matrix failure while letting concurrency-cancelled superseded runs release the slot), static-analysis folded into the required 3.12 matrix leg with pytest running exactly once per leg, and added job timeouts plus pip caching. CI gates remain armed (no disabled tests, no dropped ruff rules, no blanket mypy silencing).
Medium findings
None.
One non-blocking Low note: mypy now runs only under 3.12 — acceptable follow-up, not a blocker.
Summary
Why
The four-version matrix repeated static analysis and coverage even though those results are not version-specific. Recent successful matrix legs took roughly 16–21 minutes; most of that time is pytest, so this is a conservative cleanup rather than test sharding without measurements.
Static analysis stays inside the already-required
Test Python 3.12check. This preserves the current ruleset gate without requiring a repository-settings change.The previous
storyboard-required-gateused job-levelif: always(). A superseding push canceled every other job in the old run, but that gate remained queued and held the workflow concurrency slot, leaving the replacement run pending.if: ${{ !cancelled() }}still runs the aggregate gate after matrix failures, while allowing a canceled workflow to terminate.Validation
actionlintv1.7.7src/adcp/Review
Three independent CI/security reviewers found no workflow correctness blocker. Their required-check concern was resolved by keeping the one static-analysis run in the required Python 3.12 matrix context.
Follow-up
This refactor does not change the separate CodeQL workflow. Auditing
ci.ymlwith zizmor and pinning its remaining third-party actions should be handled as a focused security-hardening change because the current audit intentionally excludes this workflow.