build: lint the repo python with ruff #246

Sammanfogat

supernaut sammanfogade 1 incheckning från build/add-ruff in i main

2026-07-29 19:55:57 +00:00

supernaut kommenterad

2026-07-29 19:47:04 +00:00

Ägare

Adds ruff config and fixes everything it found (#61). The repo has 2,077 lines of Python —
including controller.py at 1,192 lines, which provisions and deletes ephemeral CI VMs — and
none of it was linted by anything.

Rule selection is the whole design decision here

Run with the rule set the review proposed (E W F I UP B S SIM PTH RUF), ruff reports 378
findings. 361 of those are E501 line-too-long — 95% cosmetic wrapping in otherwise-fine
code. That is precisely the "hundreds of findings nobody triages" outcome that makes a check worse
than no check, because it trains you to ignore the output.

Three deliberate narrowings, each for a reason rather than to get to green:

line-length = 120. Measured, not guessed: the median line here is 37 characters and p99
is 106, with one outlier at 157. Ruff's default of 88 would flag 361 lines. 120 flags only the
genuine outlier. (100 would flag 57; 160 would flag none and be meaningless.)
PTH dropped. It wanted 5 open() → Path.open() rewrites in working scripts — a
modernisation preference, not correctness, and pure churn in code that manages infrastructure.
S603 ignored. It fired 7 times, and every site uses the safe list form
(subprocess.run(["openstack", ...]) — no shell, no interpolation). The rule cannot distinguish
safe from unsafe, so it has no signal here; the injection risk it nominally guards is already
excluded by never passing shell=True.

That leaves 6 real findings, all fixed:

Finding	Fix
`F401` ×2 — `json` and `re` imported but unused in `controller.py`	Removed. Verified genuinely dead: zero occurrences of `json.` or `re.` in the file.
`E501` — one 157-char Prometheus `# HELP` line	Split across two adjacent literals. Verified byte-identical output — the emitted exposition line is unchanged.
`SIM105` — `try`/`except TimeoutExpired`/`pass`	`contextlib.suppress`, with the comment kept: a timeout there is the success path.
`S310` ×2 — `urlopen` scheme audit	Suppressed per-line with a documented rationale, not blanket-ignored: `base` is the operator-supplied Forgejo URL from the environment, the path is a literal API route, so no attacker-controlled scheme can reach it.

RUF100 immediately earned its place

My first attempt at the S310 rationale began the comment with # noqa S310 on both lines below,
which ruff correctly parsed as a blanket noqa directive and then flagged as unused via RUF100.
Reworded. A dead-suppression rule catching a dead suppression on its first run is a decent argument
for keeping it.

CI wiring is deliberately NOT in this PR

ruff is added to scripts/bake-runner-image.sh (alongside ansible / ansible-lint, same
pip --break-system-packages line) and to the bake's sanity check, so the next image bake includes
it. The CI step itself has to wait until the image is re-baked — adding it now would fail every
PR until then, which is exactly the lockout that bitborg-reconcile-trigger just went through.

Sequence: merge this → re-bake the runner image (scripts/bake-runner-image.sh) → follow-up PR adds
the gating ruff check step.

Verified locally: ruff check reports All checks passed!, all six Python files compile, the
runner-controller test still passes 9/9, and format:check / mdlint / shellcheck /
ansible-lint are green.

Refs #61

Adds `ruff` config and fixes everything it found (#61). The repo has **2,077 lines of Python** — including `controller.py` at 1,192 lines, which provisions and **deletes** ephemeral CI VMs — and none of it was linted by anything. ## Rule selection is the whole design decision here Run with the rule set the review proposed (`E W F I UP B S SIM PTH RUF`), ruff reports **378 findings**. **361 of those are `E501` line-too-long** — 95% cosmetic wrapping in otherwise-fine code. That is precisely the "hundreds of findings nobody triages" outcome that makes a check worse than no check, because it trains you to ignore the output. Three deliberate narrowings, each for a reason rather than to get to green: - **`line-length = 120`.** Measured, not guessed: the median line here is **37** characters and p99 is **106**, with one outlier at 157. Ruff's default of 88 would flag 361 lines. 120 flags only the genuine outlier. (100 would flag 57; 160 would flag none and be meaningless.) - **`PTH` dropped.** It wanted 5 `open()` → `Path.open()` rewrites in working scripts — a modernisation preference, not correctness, and pure churn in code that manages infrastructure. - **`S603` ignored.** It fired 7 times, and **every site uses the safe list form** (`subprocess.run(["openstack", ...])` — no shell, no interpolation). The rule cannot distinguish safe from unsafe, so it has no signal here; the injection risk it nominally guards is already excluded by never passing `shell=True`. That leaves **6 real findings**, all fixed: | Finding | Fix | | --- | --- | | `F401` ×2 — `json` and `re` imported but unused in `controller.py` | Removed. Verified genuinely dead: zero occurrences of `json.` or `re.` in the file. | | `E501` — one 157-char Prometheus `# HELP` line | Split across two adjacent literals. **Verified byte-identical output** — the emitted exposition line is unchanged. | | `SIM105` — `try`/`except TimeoutExpired`/`pass` | `contextlib.suppress`, with the comment kept: a timeout there is the *success* path. | | `S310` ×2 — `urlopen` scheme audit | Suppressed **per-line with a documented rationale**, not blanket-ignored: `base` is the operator-supplied Forgejo URL from the environment, the path is a literal API route, so no attacker-controlled scheme can reach it. | ## RUF100 immediately earned its place My first attempt at the `S310` rationale began the comment with `# noqa S310 on both lines below`, which ruff correctly parsed as a *blanket noqa directive* and then flagged as unused via `RUF100`. Reworded. A dead-suppression rule catching a dead suppression on its first run is a decent argument for keeping it. ## CI wiring is deliberately NOT in this PR `ruff` is added to `scripts/bake-runner-image.sh` (alongside `ansible` / `ansible-lint`, same `pip --break-system-packages` line) and to the bake's sanity check, so the next image bake includes it. **The CI step itself has to wait until the image is re-baked** — adding it now would fail every PR until then, which is exactly the lockout that `bitborg-reconcile-trigger` just went through. Sequence: merge this → re-bake the runner image (`scripts/bake-runner-image.sh`) → follow-up PR adds the gating `ruff check` step. Verified locally: `ruff check` reports `All checks passed!`, all six Python files compile, the runner-controller test still passes `9/9`, and `format:check` / `mdlint` / `shellcheck` / `ansible-lint` are green. Refs #61

supernaut lade till 1 incheckning

2026-07-29 19:47:04 +00:00

build: lint the repo python with ruff

ci / ci (pull_request) Successful in 1m27s

Detaljer

7bc684c155