Docs/zgong/building rules - #1556
ZhengGong-amd wants to merge 3 commits into
Conversation
Three rules had no home in the authoring contract: unit size and cyclomatic
complexity, cohesion/coupling/layering and duplication, and which code earns a
unit test versus an end-to-end one.
AGENTS.md carries them as rules ("Size is a design signal", "Test the contract,
not the plumbing", and an extended "Clean design"), the style guide carries the
numbers, and copilot-instructions.md carries the review angle. Thresholds are
~60 lines per function, CC 10, ~800 lines per module -- derived from the tree's
own 90th percentiles, not picked.
They are review triggers, not gates. 618 production functions are already above
CC 10, 801 above 60 lines and 92 modules above 800, so a hard gate would red the
tree rather than shape new code. C901 is left commented in pyproject.toml with
that count and the on-demand command, following the precedent already set there
for B/I/UP/SIM/RUF.
Also corrects copilot-instructions.md, which listed cyclomatic complexity under
"What NOT to flag -> ruff/pylint". Neither covers it: ruff selects E/F/W and CI
runs pylint --errors-only, so nothing measured complexity and the review bot was
told to stay silent about it.
Docs-only: no CHANGELOG entry (nothing an operator can observe).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four places went out of date when the previous commit landed: - AGENTS.md had two adjacent bullets both leading with "Size". The first is about diff size, so it is "Diff budget" now. - copilot-instructions.md carried "Architectural correctness" under *How to comment*, which is not a commenting style, and it had come to overlap the new "Cohesion and coupling" review item -- both said "reaches around the owning layer". Merged into that item, keeping what it said uniquely (bypassing an established pipeline, reintroducing a retired construct). - The style guide's coverage paragraph said "add unit tests for logic you introduce", which now reads against the *What to test* split directly above it. Pointed at it instead. - The Pylint section said convention/style messages are out of scope. R0912 and R0915 are refactor-category, so as written it did not explain why nothing measures complexity. Named them. Docs-only: no CHANGELOG entry (nothing an operator can observe). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed the docs-only diff (5 files, +75/-11). Verified the tooling claims against the tree: 1. Checked both halves:
Net effect: the review bot is told to stay silent about something no gate covers — exactly the gap the same commit fixes for complexity. 2. Title and description do not match the diff. The title No CHANGELOG entry is needed here — the change is docs-only with nothing an operator can observe, which is what the repo rule asks for. |
Review caught this on #1556. The previous commit rewrote copilot-instructions' "What NOT to flag" line from "Cyclomatic complexity, unused variables" to "Unused variables, unreachable code" -- swapping one false claim for another instead of removing it. Verified both halves. `ruff check --select ALL` reports nothing for a statement after `return`; Pyflakes has no such rule, and the repo selects E/F/W anyway. Pylint does have it, but as `W0101`, warning-category, which CI's `--errors-only` invocation excludes -- confirmed by running both. So the line told the review bot to stay silent about something no gate covers, which is the exact gap this PR set out to close for complexity. The line now names only what ruff genuinely reports (`F841`/`F401`, both verified), and dead code moves to the "Debt growth" review item where AGENTS.md's "Delete, don't comment out" already puts it. Docs-only: no CHANGELOG entry (nothing an operator can observe). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed (1) in Verified both halves before changing the line:
(2) not addressed yet. |
zoroyihan7
left a comment
zoroyihan7
left a comment
There was a problem hiding this comment.
Reviewed at 2e8c122a, with the goal of improving future agent-authored code and preparing for code simplification.
The 60-line / C901 > 10 / 800-line thresholds are reasonable initial review prompts. I would keep them advisory and use refactoring reviews to identify the redundant implementation, state, decision path, or forwarding layer removed.
The inline comments separate two policy gaps to fix (CI execution of replacement tests and review of changes to already oversized functions), a measurement correction, and suggestions for making the rules support complete simplification. These are policy-review findings; this PR does not itself change runtime behavior or remove tests.
For the upcoming refactoring work, start with a frequently changed responsibility whose consumers and behavior can be verified independently. Preserve supported CLI behavior, persisted state and protocol compatibility, artifact layout, and relevant failure/recovery behavior. Add missing regression assertions before removing the implementation they protect. More involved orchestration and hardware paths need a reliable test boundary before their internals are simplified.
| - `critic_agent_e2e`, `robustness_agent_e2e`, `targeted_build_e2e` | ||
|
|
||
| **Coverage:** CI enforces **90% line coverage** on measured trees (`[tool.coverage.report] fail_under`). CLI drivers, subprocess wrappers, and hardware-only paths are omitted from the denominator — see `[tool.coverage.run] omit`. Add unit tests for logic you introduce; do not chase coverage on omitted paths. | ||
| **What to test:** Pin the **exported surface** — CLI flags, public functions, persisted schemas, artifact layouts — with unit tests that state the contract *and* its failure modes; those are what callers outside this repo depend on. Internal functions that only thread a business flow together do not each need one: per-function tests there assert the current implementation and break on the next refactor. Cover those flows **end to end** (see the `*_e2e` markers above), and unit-test an internal helper when it carries real logic of its own. |
There was a problem hiding this comment.
[P2] Require replacement flow tests to run in normal PR CI.
The recommendation points contributors to the three *_e2e markers, but pytest_ci_args excludes all three, and the local development checklist excludes them too. Following this rule when replacing existing unit tests could therefore remove routinely executed regression protection while leaving the PR green.
The principle of avoiding one test per private function is useful. Please choose the test boundary by the behavior being protected: focused unit tests for deterministic logic, and integration/flow tests for orchestration that run in normal CI. Hardware/external-runtime E2E can supplement that baseline. CLI flags, persisted schemas, and artifact layouts should also be allowed to use CLI/filesystem/serialization integration tests where those protect the contract more directly.
There is already a suitable CLI + fake service + temporary files example in the tree.
Suggested rule: “Preserve contract and failure-mode assertions, and their execution in normal PR CI, when replacing tests.” The aggregate 90% line-coverage floor cannot establish this, particularly for omitted paths. Please align the corresponding AGENTS, Copilot, and PR-template wording.
| not just the line count — and treat an existing long function the change merely edits as | ||
| out of scope. |
There was a problem hiding this comment.
[P2] Exempt existing debt without exempting new complexity added to it.
“An existing long function the change merely edits” does not distinguish a small fix from appending more branches or responsibilities. A function growing from 59 to 65 lines can trigger review, while the same addition to a 150-line function can be treated as out of scope. That leaves an incremental-growth gap against the style guide's “Do not grow it.”
Please make the exemption apply to unrelated cleanup demanded solely because the function was already oversized. Still review a newly crossed threshold or a change that materially increases the complexity/responsibilities of an already oversized function, and allow an explained exception when the structure remains clearer.
For an explicitly scoped simplification PR, the existing implementation and its callers are part of the target responsibility. A normal bug fix should not be required to repay all historical debt. This distinction should also appear in the “new or rewritten code” authoring rule.
| | Function length | ~60 lines | Just above the tree's 90th percentile (55 lines); past this a function is usually two | | ||
| | Cyclomatic complexity | 10 | McCabe default; measurable on demand with `ruff check --select C901` | | ||
| | Module length | ~800 lines | Roughly the tree's 90th percentile (914 lines) | |
There was a problem hiding this comment.
Suggestion: use percentiles to calibrate review volume, and qualify the design inference.
The current distribution supports using these as initial review prompts; it does not establish that a function above 60 lines “is usually two.” For example, build_system_prompt spans 212 physical lines and has Ruff CC 1: its body returns a prompt template. Length alone does not identify a second responsibility.
C901 also has limits: _infera_restart_config_matches is 40 lines with CC 4, yet contains two groups of 6 and 9 field comparisons and substantial short-circuit logic. Those conditions still need semantic review.
I would retain 60 / 10 / 800, replace “usually two” with “inspect whether there is a clear responsibility boundary,” and explicitly allow justified template/data-heavy cases. Splitting should improve ownership, data flow, or testability. There is no evidence here for tightening the numbers or turning them into hard gates.
| # complexity" table. It stays off rather than on-and-ignored: 618 production | ||
| # functions sit above the default max of 10 today, so selecting it would red the | ||
| # tree instead of gating new code. Measure on demand -- no config edit needed: | ||
| # ruff check --select C901 --config "lint.mccabe.max-complexity=10" src/hyperloom |
There was a problem hiding this comment.
Measurement correction: 618 includes tests, and the example scans only one package.
At this head, using Ruff 0.16.7, the style guide's two-package command produces 618 C901 findings: 597 in production code and 21 in tests. The production split is 449 in Hyperloom and 148 in KernelForge. This comment calls all 618 “production functions,” while its example command scans only Hyperloom.
Please state the population and use a command matching it. The line-count statistics also need a reproducible definition: commit, included/excluded paths, whether async/nested functions are counted, and whether “lines” includes comments/docstrings.
For comparison, counting tracked Python in both packages, excluding tests, conftest files, shipped KernelForge data, and the two vendored SDK copies, I get 794 modules and 9,279 functions; function P90 55 and 798 above 60; module P90 916 (nearest-rank) and 91 above 800. Function length here is the inclusive AST definition span, including async/nested definitions and internal blank/comment/docstring lines, excluding decorators; module length uses physical lines. The small differences from 801/914/92 could reflect scope or snapshot choices and do not justify changing the thresholds.
Clarify whether tests and shipped examples follow the same size-review policy as production code.
| - **Size is a design signal.** A function that keeps growing, a branch tree you have to | ||
| scroll, a module that collects everything — that is the design telling you a boundary is | ||
| missing, and the answer is the split, not a bigger screen. For new or rewritten code, a | ||
| function past ~60 lines or cyclomatic complexity 10, or a module past ~800 lines, needs | ||
| a reason in the PR description or a split. These are review triggers, not gates: no |
There was a problem hiding this comment.
Suggestion: make simplification across the full responsibility an explicit success criterion.
For agent-authored refactoring, the rules should reward removing a redundant mechanism: duplicated behavior, an extra state/configuration source, an obsolete decision path, or a forwarding layer without an independent responsibility. Review the affected implementation together with its callers, including newly introduced helpers, interfaces, parameter plumbing, and cross-file dependencies.
Extract a helper when it names a complete responsibility or shares the same semantic rule. Inlining thin wrappers and merging equivalent entry points should be equally valid outcomes. A single-caller helper can be useful; caller count alone should not decide this. Similarly shaped code with different contracts should not be forced into a generic function with extra modes.
The existing “Derive over hardcode” rule could explicitly cover duplicated mutable state and decisions: identify the authoritative owner, derive what can be derived reliably, and state the consistency invariant for any necessary cache or replica.
For a PR whose stated goal is code reduction, report net production-code changes over the affected scope, separately from tests/docs/data, together with the mechanism removed. Avoid a fixed deletion quota or credit for formatting compression; new features may legitimately add code.
| a thing to touch what is behind it. Derive over hardcode — a single computed source | ||
| beats duplicated constants. A second copy of a behaviour is a bug you will later fix | ||
| once and miss elsewhere; extend the existing one, or lift the shared part out. |
There was a problem hiding this comment.
Suggestion: complete replacements and make compatibility serve identified consumers.
Please extend the deletion/reuse rule to cover redundant paths that are still reachable. A replacement should migrate its callers and remove the superseded implementation, obsolete switches/configuration, and tests or fixtures tied only to the old structure, while preserving required regression assertions.
For callers that can migrate together, update the interface and callers in the same change. Avoid leaving wrappers, aliases, dual-format parsing, or fallback routes solely to preserve an old internal shape. Compatibility remains necessary for supported public APIs, CLI behavior, persisted data, plugins, and independently deployed consumers; identify that boundary before deleting anything. Repository location or an empty text search is not sufficient evidence that a consumer can migrate or that an entry point is unused.
If migration must be staged, name the remaining consumer and the retirement condition for the temporary path. Keep the adapter small and avoid making it a new general-purpose entry point. This would give agents a concrete completion condition for simplification and can be folded into the existing deletion/duplication rules.
| - Linked issue(s): close/fix refs | ||
| - Tests: added/updated? commands run? | ||
| - Tests: added/updated? commands run? (exported behaviour pinned by UT; business flow covered e2e) | ||
| - Size/complexity triggers crossed (function >60 lines, CC >10, module >800 lines): split, or why the shape is right: |
There was a problem hiding this comment.
Suggestion: keep one authoritative definition and ask for a short simplification rationale.
These policies now appear in AGENTS, the style guide, the Copilot prompt, and this template. Keep the thresholds, measurement scope, and exceptions authoritative in the style guide; retain concise authoring principles in AGENTS, evidence-oriented review guidance in Copilot, and links plus a few prompts here. Useful reminders can remain without copying the full policy into four places.
For a dedicated simplification PR, three facts are enough: what mechanism was removed, what contract was preserved, and which tests demonstrate that preservation. Define the scope around one responsibility and its callers; a coherent migration can span files without bundling unrelated work.
Later, a lightweight advisory report of before/after metrics for changed units could make the thresholds consistent using Ruff and the agreed line-count definition. There is no need to make a repository-wide debt platform or a fixed percentage-reduction target a prerequisite for this PR.
3 participants
CHANGELOG.mdentry under[Unreleased]: yes/no (if no, why the change is unobservable):