Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .claude/skills/review-pr/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Blocking means one of three things, and the repo treats them as equal:
|---|---|
| Wrong behaviour | correctness, crash, data or precision error, compatibility break, security, performance regression |
| Desync | an operator-observable change the PR description never states; PR title or description does not match the diff at the current head |
| Contract violation | the diff breaks a rule `AGENTS.md` states outright, and the finding names both the bullet it breaks and the module in this repo that already owns the concern |
| Contract violation | the diff breaks a rule `AGENTS.md` states outright, and the finding names the bullet it breaks plus, where that bullet is about ownership, the module in this repo that already owns the concern |

The third kind is the narrow one. "This could be simpler", "I would have put it elsewhere" and
"consider extracting a helper" are reviewer taste and are deleted like anything else. What
Expand Down Expand Up @@ -84,7 +84,7 @@ Keep the `$WORK` it prints. Read `diff.txt` and `body.txt` before going on.

Open the index at the top of [`rules.md`](rules.md) and take every row whose trigger matches
`files.txt` and a skim of `diff.txt`. Write the union of their rule ids into `$WORK/rules.txt`, one
per line, then read only those bodies. **Never read `rules.md` whole** — it holds 53 rules across 9
per line, then read only those bodies. **Never read `rules.md` whole** — it holds 55 rules across 9
families, and a reviewer told to attend to all of them attends to none. Match rows generously: a row
you are unsure about is taken, never dropped. V1-V6 and X2 are on every list.

Expand Down
86 changes: 82 additions & 4 deletions .claude/skills/review-pr/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,14 @@ the rules listed. Rows overlap; a rule listed twice is read once.

| The diff... | Read |
|---|---|
| changes any non-test file under `src/` | X1 X3 |
| changes any non-test file under `src/` | X1 X3 D12 |
| adds or changes a value that crosses a boundary: status literal, enum member, dataclass or TypedDict field, keyword argument, signature, return semantics | C1 C3 C5 |
| adds or changes a knob or a pin: CLI flag, env var, config key, default value, a pinned external version, ref or sha, an install script, `docs/compatibility.rst`, or the argv or extra-args list one is assembled into | C3 C4 S6 T4 X5 X7 D8 D9 |
| adds or changes a knob or a pin: CLI flag, env var, config key, default value, a pinned external version, ref or sha, an install script, `docs/compatibility.rst`, or the argv or extra-args list one is assembled into | C3 C4 S6 T4 X5 X7 D8 D9 D11 |
| removes a flag, env var, enum member, test, fallback/legacy/bypass route or whole file, or tightens a comparison (`<` returns as `==`, a new `all(...)`) | C3 T2 X4 D3 |
| fixes one site of an operation that has siblings (executors, per-framework patchers, sync and async twins), or moves, copies or consolidates code | C2 T4 D1 D2 D4 |
| fixes one site of an operation that has siblings (executors, per-framework patchers, sync and async twins), or moves, copies or consolidates code | C2 T4 D1 D2 D4 D11 |
| adds a second implementation of an operation the repo already owns (patch deploy, revert, snapshot, cleanup, revalidation), or a `pre_applied`/`skip_*`/already-done branch that short-circuits one | D1 D2 D10 |
| defines, outside the module that owns the concern, a constant, precedence list, parser or client constructor the owner exports: an LLM model, SDK client, API key, base URL or header read outside `llm_config.py`, a backend registered outside `agent_backends/registry.py` | D5 D10 |
| repairs a value where it is consumed: a local re-merge of `os.environ`, a second parse of a handoff blob or serialized mapping, or any re-derivation of something another module already publishes | D11 V3 |
| touches persisted or shared state: `SCHEMA_VERSION`, `from_dict`, `CREATE TABLE`, a `record_*`/`read_*`/`seal_*` pair, `.save()`, a spec, manifest or recipe, a context manager, recovery or resume | X6 R4 P4 P5 |
| touches a prompt, `SKILL.md`, `docs/**`, `*.md` or `*.rst` | X3 X5 |
| adds error handling or a default: `except`, `contextlib.suppress`, `ignore_errors=True`, `.get(k, 0)`, `or {}`, an early `isinstance` guard, a noop or degraded implementation | S1 S2 S3 S4 S5 S7 |
Expand Down Expand Up @@ -709,6 +710,81 @@ which is what removes the layering excuse.
**Report as:** `D10: <file>:<line> defines <name>, already owned and exported by <owner>:<line>
-- import it, or state the barrier that prevents it`

### D11 -- Repair a wrong value at its producer, not by re-deriving it inside one consumer

**Severity:** blocking
**Fires when:** the diff makes a value correct by re-reading, re-parsing, re-merging or
re-deriving it inside a consumer, while the single place that produces or publishes that value
to every reader is untouched. The shape is a local `{**os.environ, **something}`, a second
`json.loads` of a handoff blob, or a helper that reassembles upstream's output before using it.
**The rule:** count the readers before accepting the repair. When a value is wrong because its
producer never published it in the form readers expect, repairing one reader leaves the rest on
the old answer and adds a second resolution path -- the defect stops being "this value is
missing" and becomes "this value means two things", which is strictly harder to find and is the
state the repair was supposed to end. One repaired reader out of N blocks; changing the producer
fixes all N without touching any of them. This is D10's dual: D10 is a concern copied beside its
caller while an owner exports it, D11 is an owner that exists but does not publish, compensated
for downstream. `AGENTS.md` *Clean design* ("a second copy of a behaviour is a bug you will later
fix once and miss elsewhere") is the bullet, and the finding names the producer module that
should have changed instead.
**Seen in:** PR #1797 -- `--extra-env` pins reached only the `INFERENCE_OPTIMIZER_EXTRA_ENV`
blob and never the individual names, so the first version re-merged that blob inside
`agentx_env_for_conc`. Every other reader kept its bare `os.environ.get`, and a pinned
`HYPERLOOM_AGENTIC_BACKEND` then selected the MLPerf client while the seeded grading axis, the
persisted backend identity and the benchmark timeout all still resolved `aiperf` -- a session
that REVERTed every round. Exporting each pin at the producer, `_export_operator_launch_shape`,
corrected all of them without changing a single reader.
**Not a finding when:** the producer is out of tree, or cannot carry the value for a reason the
diff or body states; the consumer is provably the only reader and the diff says so; the
re-derivation is a declared rung in a documented precedence ladder, which is one resolution
point rather than a second; or the producer is being changed in a stated follow-up and the body
names the readers left on the old answer until then.
**Evidence:** `$WORK/diff.txt` for the added re-derivation; a grep of the value's name across
the head tree for the readers the diff did not touch -- that count is the finding.
**Report as:** `D11 <file>:<line> -- re-derives <value> at 1 of <N> readers; <producer module>
publishes it and is untouched, so <other reader>:<line> still resolves it as <old answer>`

### D12 -- A function this diff adds, or whose complexity it raises, may not exceed cyclomatic complexity 20

**Severity:** blocking
**Fires when:** the diff adds a `def` or `async def` under `src/` outside a `tests/` directory,
or adds branches to an existing one, and the measurement below puts that function above 20 at
the head while the merge base had it at 20 or below, absent, or at a lower number.
**The rule:** measure, do not estimate -- the verdict is a comparison of two numbers, and a
function that reads long is not the same claim. Run the ceiling against the changed files at the
head, then against the same paths at `base.txt`, and match per function name:

```bash
ruff check --select C901 --config "lint.mccabe.max-complexity=20" --force-exclude \
--output-format concise $(cut -f3 "$WORK/numstat.txt" | grep '^src/.*\.py$' | grep -v '/tests/')

git show "$(cat "$WORK/base.txt")":<path> > "$WORK/base-<name>.py"
ruff check --select C901 --config "lint.mccabe.max-complexity=20" --isolated \
--output-format concise "$WORK/base-<name>.py"
```

`--force-exclude` keeps Ruff's `extend-exclude` (vendored SDK copies, `src/kernelforge/examples`)
out when paths are passed explicitly; `--isolated` is what lets the base copy be checked from
outside the tree. Present at head and absent at base, or a higher number at head than at base,
fires. The same number on both sides is backlog and is silent. This is not a rule duplicating a
static gate: `C901` is absent from `[tool.ruff.lint] select` and Pylint runs `--errors-only`, so
nothing in CI measures it and review is the only place it surfaces. `AGENTS.md` *Size is a design
signal* carries the gate and the style guide § *Complexity ceiling* carries the two cases; the
finding names the function, both numbers, and the branch the diff added.
**Seen in:** no single PR -- this is the gate written into the style guide's *Size and
complexity* table rather than a shape clustered from review history. The tree held 124 units
above 20 when it was introduced, which is the backlog the comparison exists to keep out of the
finding.
**Not a finding when:** the base number equals the head number (the unit was already over and
this diff did not grow it); the file is a test, or sits in `extend-exclude`, which
`--force-exclude` already settles; the added branches are a flat dispatch or validation table
whose arms share no state and the body says so -- that is the trigger's accepted answer, and it
survives at the ceiling only when the arms are genuinely independent.
**Evidence:** both Ruff invocations above, with the head and base numbers quoted; `$WORK/diff.txt`
for the branches that moved the number.
**Report as:** `D12 <file>:<line> -- <function> is complexity <head> (base: <base|absent>), over
the ceiling of 20; split it or keep <added branch> out of it`

## V -- Review method and PR hygiene

### V1 -- Confirm the finding is introduced by this diff, and that the diff contains nothing it did not intend
Expand Down Expand Up @@ -768,7 +844,9 @@ author to treat review feedback as a hypothesis; this is the reviewer-side oblig
comparison, not the guard the PR added; PR #1097's body attributed the KB hardware dimension
to `detect_gfx_arch()` when it comes from `kb_hardware_slug(gpu_type, ...)`.
**Not a finding when:** the body's stated cause matches the data flow, even if the fix is
narrower than you would have written.
narrower than you would have written -- narrowness on its own is taste. It stops being taste
when the narrow fix leaves the cause in place and adds a second way to resolve something the
tree already resolves elsewhere: that is D11, and it blocks on its own terms.
**Evidence:** `$WORK/body.txt`, `$WORK/commits.txt`, `$WORK/diff.txt`.
**Report as:** `V3: the body states <claim>, but <consumer> reads <actual source> -- correct
the description` / `V3: the diff narrows <window> without removing <cause>`
Expand Down
7 changes: 4 additions & 3 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ Advisory review. Flag only what static gates can't.
- **Size and complexity**: a unit past a trigger in the style guide's
[Size and complexity](../docs/contributing/style-guide.md#size-and-complexity), which
owns the numbers and the exceptions. No linter covers this (Ruff runs `E`/`F`/`W`;
Pylint is `--errors-only`), so review is the only place it surfaces. Name the seam to
split on, not just the line count, and don't ask for cleanup of an already oversized
function merely because the change edits it — but do flag a newly crossed threshold, or
Pylint is `--errors-only`), so review is the only place it surfaces — including the one
case that blocks, a unit the change adds or whose complexity it raises past 20. Name
the seam to split on, not just the line count, and don't ask for cleanup of an already
oversized function merely because the change edits it — but do flag a newly crossed threshold, or
branches and responsibilities added to a unit already over.
- **Cohesion and coupling**: a module that has acquired a second job, a responsibility
moved to the wrong layer, a dependency pointing back up the layers, a new import cycle,
Expand Down
12 changes: 7 additions & 5 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,13 @@ doc is the authority on where that boundary lives.
crosses a trigger in the style guide § *Size and complexity* — the authority on the
numbers, how to measure them, and when a long unit is fine as it stands — needs a reason
in the PR description or a split. Editing a unit that was already over is not a demand to
repay its debt; adding branches or a second responsibility to it is. These are review
triggers, not gates: no linter measures them today and the tree carries a backlog above
all three. Maintainability, readability, extensibility, and reliability are what the
thresholds stand in for; when a threshold and one of those disagree, say so and keep the
clearer code.
repay its debt; adding branches or a second responsibility to it is. The three numbers
are review triggers, not gates: no linter measures them today and the tree carries a
backlog above all three. Above them sits one gate — **a function this change adds, or
whose cyclomatic complexity this change raises, may not exceed 20**; the style guide §
*Complexity ceiling* has the measurement and the two cases it covers. Maintainability,
readability, extensibility, and reliability are what the thresholds stand in for; when a
threshold and one of those disagree, say so and keep the clearer code.
- **Review feedback is a hypothesis.** A comment can be wrong, or right about the symptom
and wrong about the fix. Before acting on one, ask what you would build if this code did
not exist yet, and whether the mechanism under discussion should exist at all. Answering
Expand Down
25 changes: 22 additions & 3 deletions docs/contributing/style-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ ruff format --check . # or `ruff format .` to apply

### Size and complexity

Nothing enforces these today: Ruff selects `E`/`F`/`W` only (no `C901`), and CI's Pylint is `--errors-only`, which excludes `R0912`/`R0915`. They are **review triggers for new and rewritten code** — the point at which a reviewer asks for a split or for the reason the shape is right.
No linter enforces these today: Ruff selects `E`/`F`/`W` only (no `C901`), and CI's Pylint is `--errors-only`, which excludes `R0912`/`R0915`. They are **review triggers for new and rewritten code** — the point at which a reviewer asks for a split or for the reason the shape is right. One of them has a ceiling above it that a reviewer blocks on; see [Complexity ceiling](#complexity-ceiling).

| Unit | Trigger | Where the number comes from |
|------|---------|-----------------------------|
Expand All @@ -73,10 +73,29 @@ Measure rather than argue:
ruff check --select C901 --config "lint.mccabe.max-complexity=10" src/hyperloom src/kernelforge
```

Passing a trigger is not a merge blocker — it means the PR description says why, or the change splits. The tree carries a backlog above all three: **do not grow it**, and prefer leaving a file you touched smaller than you found it. Editing a unit that was already over the trigger is not a demand to repay its debt; adding branches or a second responsibility to it is. Extracting a helper while you are in there is in scope; a standalone rewrite of an unrelated module is a separate PR (see [`AGENTS.md`](../../AGENTS.md) § *One concern per change*).
Passing a trigger is not a merge blocker — it means the PR description says why, or the change splits. Passing the ceiling below is. The tree carries a backlog above all three: **do not grow it**, and prefer leaving a file you touched smaller than you found it. Editing a unit that was already over the trigger is not a demand to repay its debt; adding branches or a second responsibility to it is. Extracting a helper while you are in there is in scope; a standalone rewrite of an unrelated module is a separate PR (see [`AGENTS.md`](../../AGENTS.md) § *One concern per change*).

Structure the split along the boundaries the code already has — one job per module, cohesive inside, dependencies pointing one way down the layers. A split that only moves lines to a second file, leaving the two halves reaching into each other, trades one long file for a cycle.

#### Complexity ceiling

Cyclomatic complexity above 20 is the one number here a review blocks on. It covers two cases: a function the change adds, and a function whose complexity the change raises — from at or below 20 to above it, or higher still when it was already above. A unit that stood above 20 before the change and that the change does not make worse is backlog, not this PR's debt.

The ceiling sits at twice the trigger because the trigger asks a question the ceiling has stopped accepting answers to: at 10, "this is one dispatch table" settles it; at 20 the unit carries more branches than a reviewer can keep in their head while reading it, and a paragraph in the description does not make it reviewable. Split it, or keep the new branches out of it.

Measure both sides — the head tree, and the same file at the merge base, since the verdict is a comparison:

```bash
ruff check --select C901 --config "lint.mccabe.max-complexity=20" --force-exclude \
--output-format concise <changed .py files under src/, tests excluded>

git show "$(git merge-base origin/main HEAD)":<path> > /tmp/base.py
ruff check --select C901 --config "lint.mccabe.max-complexity=20" --isolated \
--output-format concise /tmp/base.py
```

The tree carried 124 units above 20 when the ceiling was introduced (2026-10-10). The ceiling is what stops that number growing; it is not a demand to pay the 124 down.

### Module structure

Follow patterns in existing packages (for example, `hyperloom.orchestrator`):
Expand Down Expand Up @@ -126,7 +145,7 @@ Bandit scans production code (`src/hyperloom`, `scripts/`). Tests are excluded.

### Pylint

CI runs `pylint --errors-only` on core packages (fatal/error severity only). Fix new error-level issues in touched modules; convention, refactor, and style messages are intentionally out of scope — including `R0912`/`R0915`, which is why the thresholds in [Size and complexity](#size-and-complexity) are carried by review rather than by a gate.
CI runs `pylint --errors-only` on core packages (fatal/error severity only). Fix new error-level issues in touched modules; convention, refactor, and style messages are intentionally out of scope — including `R0912`/`R0915`, which is why the thresholds in [Size and complexity](#size-and-complexity) are carried by review rather than by CI.

### Tests (pytest)

Expand Down
Loading