Skip to content

Raise the per-PR diff target from ~300 to ~1000 lines, and exclude generated files #191

Description

@theCodeDrift

The change

CLAUDE.md currently says:

Prefer stacking, and aim to keep an individual diff under ~300 lines. A 900-line PR does not get reviewed, it gets approved. Tests count toward the total but never split from the code they cover — if a unit is oversized because of its tests, that is usually a sign the unit itself should be smaller.

Raise the target to ~1000 lines. 300 is too small for the shape of work this repository actually produces.

Why 300 does not hold here

Tests dominate, and the rule already forbids splitting them out. The guidance counts tests toward the total while requiring they ship with the code they cover. In practice a change with real coverage is majority test code, so the budget is spent on the part the rule will not let you separate. Recent examples:

PR total hand-written, excluding generated test share
#182 (ast-grep 0.45.2) 515 308 181 of 308 insertions
#190 (remote tier boundary) 1387 1387 roughly two thirds
#187 (dogfood house style) — 308 most of the remainder is rules and fixtures

Generated and vendored files inflate the count with nothing to review. #182 was 515 lines, of which 346 was pnpm-lock.yaml and a regenerated JSON schema. Measured against the 300 target it looks like a 1.7x overrun; measured against what a human reads, it is comfortably inside. The rule does not say to discount them, so every judgement call starts with an argument about what counts.

Prose changes have no natural 300-line seam. Agent recipes (packages/cli/src/agent/*.txt), CLAUDE.md, and OpenSpec artifacts routinely move several hundred lines in one coherent edit. Splitting them produces PRs that are individually incoherent, which costs more review attention than it saves.

The observed failure mode was never size alone. The stack that motivated the original guidance had a real problem, but it was depth: #103 reached "ready for review" as a ~93-file change having never been linted, typechecked, or tested in CI, because a branches: filter silently stopped matching seven hops from main. That is a CI-coverage failure, and it is separately fixed. A 300-line target would not have caught it.

Why not remove the target entirely

The sentence it justifies is still true: a 900-line PR does not get reviewed, it gets approved. The number should be a real ceiling that a reviewer can hold, not one that is routinely overrun and therefore ignored. A target everyone exceeds teaches people the guidance is decorative, which is worse than a looser number everyone respects.

~1000 lines is roughly where a diff stops being readable in one sitting, and it is above the natural size of the units this repository produces, so exceeding it becomes a real signal again.

Suggested wording

Prefer stacking, and aim to keep an individual diff under ~1000 lines of hand-written change. Generated files (lockfiles, regenerated schemas, vendored artifacts) do not count toward the total — a reviewer does not read them. Tests do count, but never split from the code they cover; if a unit is oversized excluding generated files and its tests are proportionate, it is probably the right size. A diff well past this is not automatically wrong, but it should come with a reason.

Notes

Raised after landing #182, #187 and #190, each of which was a coherent single unit that the 300-line target would have called oversized. Nothing here changes the preference for stacking, the merge-down rules, or the changeset placement rules.

Activity

  1. theCodeDrift commented on Aug 26, 2026

    @theCodeDrift
    MemberAuthor

    Decision: adopt, at ~1200 lines, excluding generated files.

    1200 rather than 1000 for a specific reason: the threshold has to be comfortable for a change that carries task changes, code, and tests together, which is the normal shape here. The repository's own rules already forbid splitting tests from the code they cover, and OpenSpec work adds proposal, design, spec deltas and tasks on top of the implementation. A number that only fits the code half pushes people to split along the seam the guidance elsewhere tells them not to.

    Measured against this session, 1200 sits above the coherent units and below the point where a diff stops being readable in one sitting:

    PR hand-written verdict at 1200
    #182 (ast-grep 0.45.2) 308 (of 515 total; 346 was lockfile + regenerated schema) comfortable
    #187 (dogfood house style) ~308 comfortable
    #190 (remote tier boundary) ~1387 slightly over, and correctly so — it was four units

    #190 is the useful case: at 1200 it is just over, which is the right signal for a change that really was four independently-safe units and could have been a stack. At 300 it was a 4.6x overrun, which is no signal at all.

    Suggested wording:

    Prefer stacking, and aim to keep an individual diff under ~1200 lines of hand-written change. Generated files — lockfiles, regenerated schemas, vendored artifacts — do not count toward the total; a reviewer does not read them. Tests and OpenSpec artifacts do count, but never split from the change they describe. The number is set so that a task change, its implementation, and its tests fit together comfortably. A diff well past this is not automatically wrong, but it should come with a reason.

    The rest of the stacking guidance is unchanged: the preference for stacking, the merge-down rules, and changeset placement all stand.

  2. added a commit that references this issue on Aug 26, 2026
    dde086c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions