feat(review): which lenses gate a pull request is a decision of its own

[tpouyer]

Closes #449. Serves O9 — a contributed lens that cannot be treated differently from a shipped one is only half first-class — and O8, at the one file an adopter is meant to edit.

What was wrong

AiReview(lenses=...) was answering two questions at once.

It says which lenses a repository has, and chatops.aspect_from resolves /review <lens> against exactly that map — deliberately, so a repository that added a lens can name it on a thread. But it was also the only lever on which lenses the required check runs. So keeping a lens off the check meant dropping it from the map, which put it out of reach of the comment asking for it. The ordinary want — five lenses available, two gating — was not expressible at all.

The shape of the fix

review_workflows.register(gating=("security", "tests")) is the second lever, and the two are now orthogonal: lenses= is what exists and what the thread can reach, gating= is what the check runs.

It is on the registration rather than beside lenses= because it is a property of the check. Beside lenses= reads better at the call site and worse semantically — a second check over one adapter could not then choose differently.

A lens that is bound but not gating is not run and is still nameable on the thread. That is the property the obvious implementation breaks, so it has its own test from the chat-ops side — asserted after a gating run over the same adapter object rather than before one, because narrowing the adapter is as easy to write at run time as at binding time and a check made first would not see it.

Not a --arg, and not by accident

register re-declares the workflow's signature instead of using functools.wraps. wraps sets __wrapped__, inspect.signature follows it, and in-lockstep run builds its --arg surface by introspecting the registered callable — so a wrapped gating parameter would have been the trampoline lens list that GATE-REVIEW-5 closed, coming back in through the signature. Asserted structurally rather than by grepping the YAML.

Refusals

A gating name the bound adapter does not declare refuses the run by name, listing the unknown lens beside the set that exists, with zero model calls. Resolved at run time, not at registration: a module may call register before it binds Review and there is no container yet. Run time is still before the fan-out, which is the property that matters.

gating=() is refused separately — absent and empty are opposite intentions, and the silent reading of the second is a required check that can never fail.

Both are blocked, which exits 3, so a typo is a red check rather than a green one over nothing.

Here, all four gate — by decision

O10 exists so the shipped lenses are not things we ask adopters to trust on our word, and intent, performance and tests already spent a release shipped-but-never-run once (#241). GATE-REVIEW-9 asserts it by running the workflow this repository's own module registered over a stub declaring five lenses and requiring five branches, so an argument appearing in .lockstep/lockstep.py fails the build.

GATE-REVIEW-5's claim is rescoped to say which half holds universally and which is now an adopter's choice.

Decisions taken

Two of the issue's three open questions were the owner's calls, and both went to the registration and to "a non-gating lens does not run at all" rather than "runs but cannot fail". The third — refuse an unbound name — was taken as settled by the issue's own argument.

Verification

• rm -rf .mypy_cache && make check from cold: 3063 passed, 6 skipped.
• make cov: 92%, floor 90, inside the two-sided band.
• Eleven falsification controls fired and restored from the commit, each failing the test that names its property: gating not narrowing what runs; an unknown name dropped instead of refused; only the first unknown named; gating=() read as "everything"; register wrapping instead of re-declaring; register binding with functools.partial; this repository narrowing its own check; this repository not registering the check at all; the check narrowing the adapter's declaration at run time; and branches ordered by how the selection was typed.
• Two of those controls found something I had written as a comment was not yet load-bearing, and the tests were rewritten until they fired.

Review rounds

Three rounds of this repository's own lenses. Acted on: a test that two lenses independently misread because of a vestigial assert module is not None, and which had a real hole underneath it (nothing proved the registration came from that load); the __qualname__ re-registration property, which was prose in a comment; and the unknown-lens refusal, previously asserted with only one unknown name.

Declined, with reasons: that the branch ordering is uncovered (it is covered — the selection is named in reverse on purpose, the steps assert declared order, and a control ordering by the selection fails that test; raised three times); that gating_empty is not asserted BLOCKED (it is, on the same line as its reason); that the closure captures selection by reference (never rebound, as the lens raising it said); and two notes on assertions that are exact as written.

Not proven

Nothing here has run in a real job. The refusals, the --arg surface, and this repository's own all-four registration are asserted in-process. The first pull request after merge exercises the default path; the narrowing path has no adopter to exercise it.

🤖 Generated with Claude Code

[github-actions]

in-lockstep review — intent

succeeded · $0.0819 · 3 in / 881 out tokens

This change introduces GATE-REVIEW-9: a second lever (register(gating=...)) that separates which lenses a repository has from which lenses gate a pull request. Previously the only way to keep a lens off the required check was to drop it from AiReview(lenses=...), which also removed it from /review <lens> on the thread — the wrong lever. The implementation adds the gating parameter to register, re-declares the inner closure signature deliberately to avoid functools.wraps and its __wrapped__ side-effect, resolves the gating set against the declared set at run time (not at registration), and refuses both the empty and the unknown-name cases as blocked (exit 3). Design documentation, gates table, objectives table, and tests all move in step. The diff does what the description says it does, and the description accounts for every file the diff touches. One thing I checked and found sound: the decision to leave __qualname__ as the closure's own is correctly motivated — _same_declaration compares module and qualname, so a re-execution replaces rather than conflicts, and that property is then tested in both directions. The assertion in test_gate_review_9_registering_twice_replaces_the_selection_rather_than_conflicting tests both directions (narrow-then-wide and wide-then-narrow), as the comment in the gate entry claims.

	location	finding
⚠️	tests/in_lockstep/test_enforced_review.py:300	Test assertion order may not expose a 'later selection did not win' failure reliably.
⚠️	src/in_lockstep/workflows/review.py:109	Ordering of lenses after filtering preserves adapter order, but the comment claims adapter order by design — worth a direct assertion.

[github-actions]

in-lockstep review — performance

succeeded · $0.0724 · 3 in / 233 out tokens

This change adds a gating parameter to review_workflows.register(), separating which lenses a repository has from which lenses gate a pull request. The performance-relevant paths are: the validation in review_all_lenses (a few tuple/set operations over a handful of lens names — not a concern), and the fan-out itself, which is unchanged in structure. The new lenses = tuple(name for name in declared if name in set(asked)) on line 122 builds a set from asked before filtering declared, so it is O(|declared| + |asked|) rather than O(|declared| × |asked|) — both sets are tiny (single-digit lens counts) so this is academic, but it is correct. No unbounded reads, no queries inside loops, no new synchronous calls on hot paths. The print on the gating-narrowed path runs once per check invocation, not per lens. Nothing here scales with request volume or data size.

No findings.

[github-actions]

in-lockstep review — security

succeeded · $0.0731 · 3 in / 274 out tokens

This change introduces a gating parameter to review_workflows.register(), allowing a repository to declare which subset of its bound review lenses gate a pull request, while keeping all lenses reachable via chat-ops comments. The security-relevant surface is the resolution logic in review_all_lenses: unknown lens names are refused before the fan-out, an empty gating set is refused, and the gating parameter is deliberately excluded from the registered callable's signature so it cannot be passed as a --arg from the trampoline YAML. I checked the trust boundary between what register() accepts at module-load time and what reaches the fan-out at run time, the refusal paths and their exit codes, the signature-stripping mechanism that prevents YAML-supplied lens lists from returning, and the _same_declaration / qualname behaviour that governs re-registration. All of these hold as described. The one mechanism I cannot fully verify from the diff alone is _same_declaration itself — the comparison logic lives outside the touched files — but the tests assert both directions of re-registration and the comment is explicit about what __qualname__ must be. No finding from my angle.

No findings.

[github-actions]

in-lockstep review — tests

succeeded · $0.0778 · 3 in / 608 out tokens

This change introduces register(gating=...) as a second lever for controlling which lenses gate a pull request, separate from which lenses exist. The test coverage is thorough and well-structured: each distinct behavioural claim in GATE-REVIEW-9 has a named test, the refusal paths for unknown lenses, empty selection, and the structural no-gating-arg-in-signature constraint are all exercised with assertions tight enough to fail on the wrong implementation. I checked the most common failure mode — assertions that pass with the change reverted — and found the tests hold up: sorted(adapter.asked) would be wrong for any implementation that ran all five lenses when two were named, and the message prefix check (startswith) is specific enough to catch a refusal that named only the declared set without naming the unknown lens. One gap worth naming: the ordering property asserted in test_gate_review_9_only_the_gating_lenses_run_and_the_rest_are_never_asked — that steps run in adapter-declaration order rather than selection order — is asserted on steps but not on adapter.asked, which is a plain list appended in fan-out completion order and may be non-deterministic under async scheduling. That said, sorted(adapter.asked) is what the test uses, so it does not accidentally assert ordering there. The double-registration test asserts both directions but the second direction (register(gating=(...)) after register()) checks again.asked == ["security"] with a bare equality that would fail if ordering changed — this is correct and tight. No test covers the print(...) path that announces the narrowed set, but that is cosmetic output and not a behaviour worth asserting. The diff is complete as shown; no files are named as truncated.

	location	finding
⚠️	tests/in_lockstep/test_enforced_review.py:209	Ordering of adapter.asked in double-registration second direction is asserted with bare equality.