Skip benchmark-ratchet on non-performance pull requests (4.4.3.) - #289
Skip benchmark-ratchet on non-performance pull requests (4.4.3.)#289leynos wants to merge 6 commits into
Conversation
|
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:
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
Summary
WalkthroughConfigure actionlint and spell-check exceptions. Add a ChangesCI benchmark workflow
Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant changes
participant benchmark-ratchet
GitHubActions->>changes: Detect changed paths
changes-->>GitHubActions: Publish bench output and run summary
GitHubActions->>benchmark-ratchet: Run for non-PR events or relevant PRs
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 warnings, 1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches 💡 1🛠️ 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.
Reviewer's GuideAdds a lightweight changes classification job to CI and gates the paid benchmark-ratchet workflow on performance-relevant file changes, plus configures actionlint to recognize the Ubicloud runner label. Flow diagram for gating benchmark-ratchet on performance-relevant changesflowchart LR
GitHubEvent[GitHub event]
ChangesJob[changes job]
PathsFilter[dorny/paths-filter]
BenchOutput{needs.changes.outputs.bench}
BenchmarkRatchet[benchmark-ratchet job]
GitHubEvent --> ChangesJob
ChangesJob --> PathsFilter
PathsFilter --> BenchOutput
BenchOutput -- pull_request and bench == 'true' --> BenchmarkRatchet
GitHubEvent -- event_name != 'pull_request' --> BenchmarkRatchet
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/actionlint.yaml:
- Line 9: Update the config-variables setting in actionlint.yaml from null to an
allow-list containing only CODESCENE_CLI_SHA256, matching the sole vars.*
workflow variable.
🪄 Autofix
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: 16a12af4-b20a-4299-906c-ee4719a4b012
📒 Files selected for processing (2)
.github/actionlint.yaml.github/workflows/ci.yml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
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.
`actionlint` rejects `ubicloud-standard-4-ubuntu-2404` as an unknown runner label without a `self-hosted-runner` declaration. The other Ubicloud-using repositories in the estate (lille, wildside, nile-valley, chutoro) already carry `.github/actionlint.yaml` for this reason; bring cuprum in line so workflow changes can be linted.
`benchmark-ratchet` is the only paid Ubicloud job in this workflow, and a July 2026 usage audit found it running on every one of 420 CI runs — including docs edits and Dependabot github-actions batches that cannot change pipeline throughput. Add a `changes` job (GitHub-hosted, seconds) that classifies the diff with `dorny/paths-filter`, and gate `benchmark-ratchet` on it for pull requests. Performance-relevant paths — `cuprum/`, `rust/`, `benchmarks/`, `conftest.py`, the `Makefile`, `pyproject.toml`, `uv.lock`, and the workflow itself — still trigger the ratchet, so dependency bumps that could alter performance are still measured. Pushes to main are never gated: the ratchet must run there to refresh the `benchmark-ratchet-main-baseline` artifact that pull-request runs compare against. The `changes` job itself runs on every event so the `needs` edge cannot leave `benchmark-ratchet` skipped on main.
The estate-wide typos base no longer exempts inline code spans wholesale, so regenerating typos.toml drops the rule the committed copy still carried and three real identifiers start failing the spelling gate: a helper name, a third-party command-line flag, and the style guide's own example of a US-spelled API name. Exempt them one at a time in typos.local.toml rather than reinstating the blanket rule; prose inside backticks should still be spell-checked. Commit the regenerated typos.toml so the gate stops re-deriving the drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate in front of benchmark-ratchet lives entirely in declarative configuration, and every part of it fails silently in the direction that costs money or hides a regression: invert the condition and a genuine performance change merges unbenchmarked, drop the event clause and pushes to main stop refreshing the baseline that pull-request runs compare against. No ordinary test notices, so parse ci.yml and assert the contract — the bench output wiring, the needs edge, the gate expression verbatim, the exact filter path set, and the runner the detector uses. Property tests over sampled changed-path sets then check the rule those parts encode, against a path model a companion test stops from drifting away from the filter it claims to describe. Record the decision — event, filter verdict, ran or skipped — in the run summary of every run, because a skipped job and a broken gate look identical in the run list, and this gate exists to be audited against paid-runner spend. Add a per-ref concurrency policy while here. Superseded pull-request runs are cancelled, since a stale run only spends benchmark minutes on a diff nobody will merge; runs on main are not, because cancelling one abandons the baseline upload, and queueing them keeps two quick merges from publishing that artifact out of commit order. Narrow actionlint's config-variables to the one variable the workflows read, so a typo in a vars.* reference is an error rather than an empty string at run time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
22ac97a to
d80b30d
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.
|
Two red checks on this branch are pre-existing and unrelated to the change; recording the evidence.
That 0.760 comes from the Python 3.15a fails four All local commit gates pass on the rebased branch: |
|
@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. (Treat o11y, code safety, documentation and validation coverage as in scope). ❌ Failed checks (1 error, 5 warnings)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== PR files and checks =='
gh pr view 289 --repo leynos/cuprum --json headRefName,baseRefName,files,statusCheckRollup,url \
--jq '{url, headRefName, baseRefName, files: [.files[].path], checks: [.statusCheckRollup[] | {name, conclusion, status}]}'
printf '%s\n' '== Relevant repository files =='
fd -H -t f -a '^(ci\.yml|actionlint\.yaml|developers-guide\.md|.*benchmark.*|.*workflow.*|.*execplan.*)$' . \
| sort
printf '%s\n' '== Workflow outline and relevant content =='
ast-grep outline .github/workflows/ci.yml --items all || true
sed -n '1,280p' .github/workflows/ci.yml
printf '%s\n' '== actionlint configuration =='
cat .github/actionlint.yaml
printf '%s\n' '== Workflow/paths-filter/benchmark test and documentation references =='
rg -n -i -C 3 \
'(paths-filter|benchmark-ratchet|workflow contract|workflow.*test|benchmark.*gate|ubic[l]?oud|concurrency|ExecPlan)' \
-g '!uv.lock' .
printf '%s\n' '== Candidate follow-up issues =='
gh issue list --repo leynos/cuprum --state open --limit 100 \
--search 'benchmark ratchet OR paths-filter OR workflow gating OR concurrency OR observability' \
--json number,title,url,body \
--jq '.[] | {number,title,url,body}'Length of output: 186921 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Changes and benchmark job =='
rg -n -A 105 -B 8 '^[[:space:]]{2}changes:|^[[:space:]]{2}benchmark-ratchet:' .github/workflows/ci.yml
printf '%s\n' '== Gate contract test outline =='
ast-grep outline cuprum/unittests/test_benchmark_gate_ci_contract.py --items all
printf '%s\n' '== Gate contract test =='
sed -n '1,430p' cuprum/unittests/test_benchmark_gate_ci_contract.py
printf '%s\n' '== Documentation and ExecPlan gate sections =='
sed -n '2200,2265p' docs/developers-guide.md
sed -n '195,214p' docs/execplans/4-4-3-ratchet-rust-performance.md
printf '%s\n' '== Remaining failure issues =='
gh issue view 219 --repo leynos/cuprum --json number,state,title,url --jq '{number,state,title,url}'
gh issue view 293 --repo leynos/cuprum --json number,state,title,url --jq '{number,state,title,url}'Length of output: 30718
Keep the two remaining red checks out of this PR’s scope:
Do not create a new issue. Do not add further implementation work to this PR. No AI agent prompt is required. You are interacting with an AI system. |
The contract tests pin what ci.yml declares; they do not say what those declarations decide for a pull request someone would recognize. Add the behavioural half — docs-only, a Rust change, a dependency bump, a mixed diff, an empty diff, and a push to main — so the gate is covered in both of the styles this repository requires. Move the workflow parsing and the path model into tests/helpers/workflow.py so the two suites read one ci.yml through one model, rather than agreeing by coincidence. Pin the absence of a status function in the gate as well. GitHub inserts an implicit success() unless the expression names one, so a failed detector currently skips the paid job; `if: always() && (...)` reads as a harmless robustness tweak and is the single edit that would turn a broken detector into an unconditional paid run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Update:
The baseline for The only remaining red check is the Python 3.15a row, which is |
|
Both of the red checks discussed above are now tracked in their existing threads, so they need no further action on this pull request:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/ci.yml:
- Around line 12-21: Separate main-branch baseline publication from pull-request
cancellation in the workflow’s concurrency configuration: choose and explicitly
implement a main-only queue that retains required runs, including the
queue-limit behavior, or document latest-run semantics if that is the intended
policy. Update cuprum/unittests/test_benchmark_gate_ci_contract.py at lines
297-318 to assert the selected policy, and revise the concurrency and
baseline-publication claims in docs/cuprum-design.md lines 2469-2482,
docs/developers-guide.md lines 2246-2250, and docs/users-guide.md lines
1633-1638 to match it.
In `@docs/execplans/4-4-3-ratchet-rust-performance.md`:
- Around line 200-219: Update the earlier completion criteria and retrospective
in the execplan to match the current ratchet policy: pull-request benchmarks run
only for performance-relevant path changes, and regressions fail only when above
0.30. Remove or revise claims requiring every pull request and a 10% threshold,
while preserving the later amendment and its documented gating behavior.
In `@tests/helpers/workflow.py`:
- Line 71: Update _require so condition is keyword-only and remove the FBT001
suppression; if positional Boolean usage is required, retain the narrow
suppression only with an inline justification explaining that necessity.
- Around line 39-180: Document the public interfaces with comprehensive
NumPy-style docstrings: in tests/helpers/workflow.py (lines 39-180), add
Parameters, Returns, and Raises sections where applicable for Step, Job,
Workflow, WorkflowContractError, and public helpers; keep private helpers
concise. In cuprum/unittests/test_benchmark_gate_ci_contract.py (lines 98-318),
document every public pytest test function using the required sections where
applicable. In tests/behaviour/test_benchmark_path_gate_behaviour.py (lines
20-161), document Event, Decision, and all public scenario and step functions in
the same style; do not alter behavior.
🪄 Autofix
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: 0701c131-6447-4b38-9cda-37d35e6bd966
📒 Files selected for processing (13)
.github/actionlint.yaml.github/workflows/ci.ymlcuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_benchmark_gate_ci_contract.pydocs/cuprum-design.mddocs/developers-guide.mddocs/execplans/4-4-3-ratchet-rust-performance.mddocs/users-guide.mdtests/behaviour/test_benchmark_path_gate_behaviour.pytests/features/benchmark_path_gate.featuretests/helpers/workflow.pytypos.local.tomltypos.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
| ### Later amendment: gating the job on changed paths | ||
|
|
||
| A July 2026 audit of Ubicloud usage found this job — the repository's only | ||
| paid job — running on all 420 CI runs that month, including documentation | ||
| edits and Dependabot `github-actions` batches. A `changes` job now classifies | ||
| the diff with `dorny/paths-filter`, and `benchmark-ratchet` runs on pull | ||
| requests only when a performance-relevant path changed. The | ||
| `push`-to-`main` trigger stated above is deliberately left ungated: that run | ||
| publishes the baseline artefact every later comparison reads, so filtering it | ||
| would fail open rather than merely cost less. `.github/actionlint.yaml` | ||
| declares the Ubicloud runner label so `actionlint` can check the workflow. | ||
|
|
||
| Both test styles cover the gate, as this plan's constraints require: | ||
| `cuprum/unittests/test_benchmark_gate_ci_contract.py` pins the declarations | ||
| and property-tests the rule they encode, while | ||
| `tests/behaviour/test_benchmark_path_gate_behaviour.py` and | ||
| `tests/features/benchmark_path_gate.feature` state the decision for | ||
| recognizable pull requests. See "Gating the paid benchmark job" in | ||
| `docs/developers-guide.md`. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the completed plan with the current ratchet policy.
Update the earlier completion criteria and retrospective. They still require a
benchmark on every pull request and a 10% regression threshold. The workflow
now filters pull requests and fails only above 0.30. The later amendment does
not remove those contradictory claims.
Triage: [type:docstyle]
As per coding guidelines, “Keep design decisions, roadmap completion status, and
any execplan up to date.”
🧰 Tools
🪛 LanguageTool
[uncategorized] ~209-~209: The official name of this software platform is spelled with a capital “H”.
Context: ...fail open rather than merely cost less. .github/actionlint.yaml declares the Ubicloud ...
(GITHUB)
🤖 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/execplans/4-4-3-ratchet-rust-performance.md` around lines 200 - 219,
Update the earlier completion criteria and retrospective in the execplan to
match the current ratchet policy: pull-request benchmarks run only for
performance-relevant path changes, and regressions fail only when above 0.30.
Remove or revise claims requiring every pull request and a 10% threshold, while
preserving the later amendment and its documented gating behavior.
Sources: Coding guidelines, Path instructions
| class Step(typ.TypedDict, total=False): | ||
| """One step of a job, declaring only the keys these helpers read.""" | ||
|
|
||
| id: object | ||
| uses: object | ||
| run: object | ||
|
|
||
|
|
||
| class Job(typ.TypedDict, total=False): | ||
| """One job of a workflow, declaring only the keys these helpers read.""" | ||
|
|
||
| needs: object | ||
| outputs: object | ||
| steps: list[Step] | ||
|
|
||
|
|
||
| class Workflow(typ.TypedDict, total=False): | ||
| """A parsed workflow file, declaring only the keys these helpers read.""" | ||
|
|
||
| concurrency: object | ||
| jobs: dict[str, Job] | ||
|
|
||
|
|
||
| class WorkflowContractError(AssertionError): | ||
| """The workflow does not have the shape the contract tests require. | ||
|
|
||
| Raised rather than asserted so that a malformed workflow fails with the | ||
| same message whichever suite read it, and so that a suite can distinguish | ||
| "the file is not shaped like a workflow" from "the contract is not met". | ||
| """ | ||
|
|
||
|
|
||
| def _require(condition: bool, message: str) -> None: # noqa: FBT001 | ||
| """Raise `WorkflowContractError` when a shape requirement is unmet.""" | ||
| if not condition: | ||
| raise WorkflowContractError(message) | ||
|
|
||
|
|
||
| def mapping(value: object, message: str) -> dict[str, object]: | ||
| """Require that a value read from the workflow is a mapping, and type it. | ||
|
|
||
| `yaml.safe_load` produces mappings of unknown key type, which makes every | ||
| subsequent `.get("…")` a type error rather than a narrowing. | ||
| """ | ||
| _require(isinstance(value, dict), message) | ||
| return typ.cast("dict[str, object]", value) | ||
|
|
||
|
|
||
| @functools.cache | ||
| def workflow() -> Workflow: | ||
| """Parse the CI workflow.""" | ||
| parsed = yaml.safe_load((repo_root() / CI_WORKFLOW).read_text(encoding="utf-8")) | ||
| _require(isinstance(parsed, dict), f"{CI_WORKFLOW} must parse to a mapping") | ||
| return typ.cast("Workflow", parsed) | ||
|
|
||
|
|
||
| def job(job_name: str) -> dict[str, object]: | ||
| """Return a named job, failing with the available names when absent.""" | ||
| jobs = mapping(workflow().get("jobs"), f"{CI_WORKFLOW} must declare a jobs mapping") | ||
| return mapping( | ||
| jobs.get(job_name), | ||
| f"{CI_WORKFLOW} must declare a {job_name!r} job; found {sorted(jobs)}", | ||
| ) | ||
|
|
||
|
|
||
| def steps(job_name: str) -> list[dict[str, object]]: | ||
| """Return the steps of a named job.""" | ||
| declared = job(job_name).get("steps") | ||
| _require(isinstance(declared, list), f"the {job_name!r} job must declare steps") | ||
| return typ.cast("list[dict[str, object]]", declared) | ||
|
|
||
|
|
||
| def step_with_id(job_name: str, step_id: str) -> dict[str, object]: | ||
| """Return the step of a job carrying a given `id:`.""" | ||
| found = next( | ||
| (step for step in steps(job_name) if step.get("id") == step_id), | ||
| None, | ||
| ) | ||
| return mapping( | ||
| found, f"the {job_name!r} job must declare a step with id {step_id!r}" | ||
| ) | ||
|
|
||
|
|
||
| def benchmark_gate() -> str: | ||
| """Return the `if:` expression gating the benchmark job.""" | ||
| condition = job(BENCHMARK_JOB).get("if") | ||
| _require( | ||
| isinstance(condition, str), | ||
| f"the {BENCHMARK_JOB!r} job must declare an `if:` condition", | ||
| ) | ||
| return typ.cast("str", condition) | ||
|
|
||
|
|
||
| @functools.cache | ||
| def filter_paths() -> frozenset[str]: | ||
| """Return the path patterns the `bench` filter declares.""" | ||
| step = step_with_id(CHANGES_JOB, FILTER_STEP_ID) | ||
| inputs = mapping( | ||
| step.get("with"), | ||
| f"the {FILTER_STEP_ID!r} step must pass inputs to the filter action", | ||
| ) | ||
| filters = mapping( | ||
| yaml.safe_load(str(inputs["filters"])), | ||
| "the `filters` input must parse to a mapping", | ||
| ) | ||
| patterns = filters.get(FILTER_NAME) | ||
| _require( | ||
| isinstance(patterns, list), | ||
| f"the filter must declare a {FILTER_NAME!r} list; found {sorted(filters)}", | ||
| ) | ||
| return frozenset(str(pattern) for pattern in typ.cast("list[object]", patterns)) | ||
|
|
||
|
|
||
| def matches_filter(pattern: str, path: str) -> bool: | ||
| """Return whether a changed `path` matches a declared filter `pattern`. | ||
|
|
||
| A bounded model of the two pattern forms the filter is allowed to use: a | ||
| literal path, and a `dir/**` prefix. A contract test fails when a pattern | ||
| outside those forms is declared, so the model cannot silently stop | ||
| describing the filter it stands in for. | ||
| """ | ||
| if pattern.endswith("/**"): | ||
| return path.startswith(pattern.removesuffix("**")) | ||
| return path == pattern | ||
|
|
||
|
|
||
| def bench_output(changed_paths: cabc.Collection[str]) -> bool: | ||
| """Model the `bench` output the filter produces for a set of changes.""" | ||
| return any( | ||
| matches_filter(pattern, path) | ||
| for pattern in filter_paths() | ||
| for path in changed_paths | ||
| ) | ||
|
|
||
|
|
||
| def benchmark_runs(*, event_name: str, bench: bool) -> bool: | ||
| """Model the gate, returning whether `benchmark-ratchet` runs. | ||
|
|
||
| Mirrors the `if:` expression a contract test pins verbatim; the pin is what | ||
| keeps this model and the workflow from drifting apart. | ||
| """ | ||
| return event_name != "pull_request" or bench |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add structured NumPy-style docstrings to the new public Python interfaces.
Document public classes and functions with Parameters, Returns, and
Raises sections where applicable. Keep private helpers concise.
tests/helpers/workflow.py#L39-L180: document exported TypedDict classes,
WorkflowContractError, and shared workflow helper functions.cuprum/unittests/test_benchmark_gate_ci_contract.py#L98-L318: document the
public pytest test functions to the required format.tests/behaviour/test_benchmark_path_gate_behaviour.py#L20-L161: document
Event,Decision, and public scenario and step functions to the required
format.
As per coding guidelines, “Public Python functions, classes, and methods must
have comprehensive NumPy-style docstrings.” As per path instructions,
“Docstrings must follow the numpy style guide.”
📍 Affects 3 files
tests/helpers/workflow.py#L39-L180(this comment)cuprum/unittests/test_benchmark_gate_ci_contract.py#L98-L318tests/behaviour/test_benchmark_path_gate_behaviour.py#L20-L161
🤖 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/workflow.py` around lines 39 - 180, Document the public
interfaces with comprehensive NumPy-style docstrings: in
tests/helpers/workflow.py (lines 39-180), add Parameters, Returns, and Raises
sections where applicable for Step, Job, Workflow, WorkflowContractError, and
public helpers; keep private helpers concise. In
cuprum/unittests/test_benchmark_gate_ci_contract.py (lines 98-318), document
every public pytest test function using the required sections where applicable.
In tests/behaviour/test_benchmark_path_gate_behaviour.py (lines 20-161),
document Event, Decision, and all public scenario and step functions in the same
style; do not alter behavior.
Sources: Coding guidelines, Path instructions
| """ | ||
|
|
||
|
|
||
| def _require(condition: bool, message: str) -> None: # noqa: FBT001 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove or justify the FBT001 suppression.
Make condition keyword-only, or add an inline reason that explains why a
positional Boolean is necessary. The policy forbids an unexplained # noqa
suppression.
As per path instructions, “Only narrow in-line disables (# noqa: XYZ) are
permitted, must be accompanied by justification and used only as a last resort.”
🤖 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/workflow.py` at line 71, Update _require so condition is
keyword-only and remove the FBT001 suppression; if positional Boolean usage is
required, retain the narrow suppression only with an inline justification
explaining that necessity.
Source: Path instructions
The contract test asserted the summary step mentioned the right words. A script that emitted nothing, or the opposite verdict, contains the same words: the check could not fail for the reason it existed. Extract the step's script from ci.yml and run it under bash for each combination of event and detector state, then read back the row it emitted. Verified by inverting the decision in the script: five cases fail. Record the detector's own status too, and run the step on !cancelled() rather than the implicit success(). A failed detector is the case most worth recording — the benchmark then skips for a reason unrelated to the diff — and a summary that stops being written exactly when the gate misbehaves documents only the runs that needed no explanation. An absent verdict now reads `unknown` rather than `false`, which would have asserted "no performance-relevant changes" on the strength of nothing. Correct the concurrency claim while here. Queueing per ref does not order anything: GitHub replaces a pending run when a newer one arrives and promises nothing about completion order, so two merges in quick succession may still publish baselines out of commit order. State what the policy actually buys and where ordering would have to be enforced instead. Fold the workflow parsing in test_extension_ci_contract.py into tests/helpers/workflow.py so one parser reads ci.yml, and drop WorkflowContractError, which no caller distinguished from AssertionError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
leynos
changed the title
|
Addressed, with one skipped. Verified each finding against head first. Testing (Overall) — valid, fixed. The summary test only searched for tokens, so a script that emitted nothing or the opposite verdict would have passed it. Observability — valid, fixed. The step now carries Concurrency And State — valid, claim removed. You are right that per-ref queueing orders nothing: GitHub replaces a pending run when a newer one arrives and promises nothing about completion order. The comment, the test's rationale and the developers' guide now state what the policy actually buys (superseded pull-request runs cancelled; Architectural Complexity — valid, fixed. Title check — skipped as stated, applied anyway. No merged pull request in this repository carries a All six |
2 participants
Summary
This branch stops the
benchmark-ratchetjob — the only paid Ubicloud jobin this repository — from running on pull requests that cannot affect
performance. A July 2026 Ubicloud usage audit found the job firing on every
one of 420 CI runs that month, including docs-only edits and Dependabot
github-actions batches, making cuprum the estate's dominant premium-4
consumer.
A new
changesjob (GitHub-hosted, completes in seconds) classifies thediff with
dorny/paths-filter;benchmark-ratchetnow runs on pullrequests only when performance-relevant paths change. Pushes to
mainarenever gated, because the main run refreshes the
benchmark-ratchet-main-baselineartifact that pull-request runs compareagainst. Dependency bumps that touch
uv.lockorrust/still benchmarkby design, since a dependency change can legitimately alter throughput.
A companion commit adds
.github/actionlint.yaml
declaring the
ubicloud-standard-4-ubuntu-2404label, matching theconvention already present in lille, wildside, nile-valley, and chutoro,
so that
actionlintpasses on this repository's workflows.Review walkthrough
changesjob and the gatedneeds/ifonbenchmark-ratchetin.github/workflows/ci.yml
— the comments explain why
changesruns on every event (a skippedneedsdependency would otherwise skip the main-branch baseline run).for the runner-label declaration.
Validation
actionlint .github/workflows/ci.yml: clean (with the new labeldeclaration).
uv run pytest cuprum/unittests/test_extension_ci_contract.py cuprum/unittests/test_fetch_main_benchmark_baseline.py -q: 15 passed.Notes
dorny/paths-filteris pinned to the v3.0.2 commit(
de90cc6f…), consistent with the repository's SHA-pinning policy; thegrouped github-actions Dependabot ecosystem will keep it updated.
benchmark-ratchetis a required status check, a skipped run countsas satisfied, so gated pull requests still merge normally.
.github/actionlint.yamlnow listsCODESCENE_CLI_SHA256rather thandisabling the configuration-variable check, so a typo in a
vars.*reference is an actionlint error rather than an empty string at run time.
dictionary stopped exempting inline code spans, so regenerating
typos.tomlbroke the spelling gate on three pre-existing identifiers.They are exempted individually in
typos.local.tomlrather than byreinstating the blanket rule.
Review feedback addressed
one
ci.ymlthrough one model intests/helpers/workflow.py.cuprum/unittests/test_benchmark_gate_ci_contract.pyparsesci.ymland asserts the gate: thebenchoutput wiring, theneedsedge, the gate expression verbatim, the exact filter path set, the
GitHub-hosted runner for
changes, the summary step, and theconcurrency policy. Property tests (Hypothesis) over sampled changed-path
sets check the rule those parts encode — any watched path benchmarks
however it is mixed with docs, a diff touching nothing watched skips, and
a non-pull-request event always benchmarks. A companion test fails if a
filter pattern outside the two modelled forms is added, so the path model
cannot silently stop describing the filter. Verified non-vacuous by
inverting the gate and deleting a filter path: both mutations fail.
tests/behaviour/test_benchmark_path_gate_behaviour.py, withtests/features/benchmark_path_gate.feature, states the decision forrecognizable pull requests: docs-only, a Rust change, a dependency bump, a
mixed diff, an empty diff, and a push to
main. A further test pins theabsence of a status function in the gate — GitHub inserts an implicit
success()unless the expression names one, so a failed detector skips thepaid job, and
if: always() && (…)would be the single edit that turns abroken detector into an unconditional paid run.
changesjob appends the decision — event, filterverdict, and whether the benchmark ran or was skipped — to
$GITHUB_STEP_SUMMARYon every run. All three fields are closed sets, sothe summaries stay countable; a skipped job and a broken gate are
otherwise indistinguishable in the run list.
concurrency: ci-${{ github.ref }}withcancel-in-progresstrue onlyfor pull requests. Superseded pull-request runs are cancelled; runs on
mainare not, because a cancelled run abandons the baseline upload, andqueueing them keeps two quick merges from publishing that artifact out of
commit order.
docs/developers-guide.mdrecords the path list, the runner label, themain-push behaviour, and why
changesis ungated;docs/users-guide.mdand
docs/cuprum-design.mdstate the gate where they describe theratchet, and ExecPlan 4.4.3 carries an amendment note.
Summary by Sourcery
Reduce paid benchmark usage by running benchmark-ratchet only for performance-relevant pull requests while preserving main-branch baseline updates.
Enhancements:
CI:
Documentation:
Tests:
Chores:
References