orisys-sdk/docs/code-review.md

Code review: the automated part

Four gates existed before this document (the test suites, apps/studio/tools/ui_check.py, the real-GL smoke tests, the binding-purity test) and none of them is a linter. This adds the missing static layer, deliberately small.

Why it exists, in one measured fact

Until now the three repos had no Ruff configuration at all. ruff check . therefore ran whatever the installed version enables by default — Ruff 0.16 enables a very broad set (UP/RUF/PLR/S/TRY/PERF/FURB/YTT/ASYNC…) — so the same tree produced 1038 / 438 / 117 findings in orisys-app / orisys-sdk / orisys-sensor-design, and a different Ruff version produced different ones. A gate built on that is not a gate. ruff.toml now pins it.

What is gated, and what is deliberately not

rule	meaning	gated?	why
BLE001	broad except Exception	no	A broad catch is the right choice around device I/O, GL teardown and camera probing. This codebase already records that choice inline as # noqa: BLE001 - <reason>. Gating it would produce ~100 findings whose only correct fix is "add a reasoned suppression".
S110, S112	handler is only pass / continue	yes	The complaint is not the breadth of the catch, it is that the failure becomes invisible. Silence is acceptable only with a reasoned suppression or a handler that reports/re-raises.
E722	bare except:	yes	Strictly worse than except Exception: — it also swallows KeyboardInterrupt/SystemExit.
F	unused import, redefinition, undefined name, unused variable	yes	Real defects: F821/F822 are latent NameErrors, F811 can be a shadowed definition.
B023	closure over a loop variable	yes	Late binding silently produces wrong geometry in the headless Blender scripts.
PLW1510	subprocess.run without check	yes	A swallowed tool failure.
RUF100	unused suppression	reported, never auto-fixed	The - reason in a noqa comment is documentation. A machine must not delete it. Triage by hand.
UP, I, PIE, RET501, PLR1711, SIM114, RUF022, RUF023	mechanical modernisation	in the lint config, not in the gate	Applied once by --fix; gating them would only churn AI-authored diffs.

E402 (module import not at top of file) is selected but not gated. import after os.environ[...] is deliberate in this codebase — the environment must be set before Qt/cv2/the SDK load — so gating it would be pure noise. But it is the only rule that would catch an accidental import moved above that setup, so it stays visible. When you touch one of those files, record the intent as # noqa: E402 - <reason>; E701/E702 are style, which ruff format owns. A gate that cries wolf gets ignored, which is worse than no gate.

The three commands

# 1. The agent loop, after every edit — mechanical, no review needed.
ruff check <changed files> --fix            # never --unsafe-fixes
ruff check <changed files> --output-format json   # what remains, machine-readable

# 2. The gate before a change lands. Changed files only, machine-readable, non-zero on findings.
python tools/lint_gate.py                   # vs HEAD
python tools/lint_gate.py --base origin/main
python tools/lint_gate.py --all             # gate rules over the whole repo
python tools/lint_gate.py --json

# 3. Formatting, changed files only — never a whole-tree rewrite mid-diff.
ruff format <changed files>

$RUFF may point at a specific ruff binary; otherwise tools/lint_gate.py uses PATH or the one beside the interpreter. Pin the version: the gate prints the version it ran, and findings differ between versions, so a runner must install the same one the developers use (currently 0.16.9).

What this found when it was introduced

Filtering to rules that can indicate a defect produced ~18 candidates rather than style noise — which is the argument for the narrow set:

• six latent NameErrors (np, sys, Callable, FrameIdentity, tessellate_polygon, and an __all__ entry naming a function that does not exist) — none caught by any test;
• a duplicated function definition that silently shadowed the first;
• lower == lower / upper == upper in a range check, and the same pattern inside a test assertion that could never fail;
• 13 closures over loop variables in the headless Blender/patch scripts;
• two subprocess.run calls whose failure was silently ignored.

Follow-up deliberately not done yet

PLR0124 ("name compared with itself") is not in select. Eleven of its sites are the deliberate value != value NaN idiom and must become math.isnan(...) (or carry a reasoned suppression) before the rule can be gated — otherwise it is noise. Once those are converted, add PLR0124 to select and to GATE_SELECT in tools/lint_gate.py.