The split has to say why | Nate Swenson Dev Log

Shipped

The first run of the 0.7.0 review loop worked an issue that bundled four unrelated follow-ups, each between 180 and 520 changed lines, and split them into four stacked draft pull requests. Every item passed the only test a split had to pass, which was “reviewable and mergeable alone”, and every small fix passes that test. Nothing weighed what a lane costs: its own pull request, its own review loop of up to four rounds, a stack layer, a landing. I folded the four back into one before merging.

Version 0.7.1 makes one pull request per issue the default. A ## Work items heading in the plan is now only for a change too large to review as one, its first line has to be Why split: followed by the size or the layer in numbers, and the parser refuses the heading without it. This guide is about that shape: a default the tool enforces, and an exception that has to justify itself in the artifact, not in the conversation.

When small is the wrong default

Everything written about review size says smaller. Google’s engineering practices guide on small CLs puts a reasonable change around a hundred lines and calls a thousand usually too large, and the companion page on review speed tells a reviewer facing a huge change to ask for it to be split into smaller ones that build on each other. SmartBear’s summary of the Cisco review study says developers should review no more than 200 to 400 lines at a time.

All of that assumes the review is a person’s attention, which is the scarce thing a split protects. In an automated loop the scarce thing moves. Each pull request buys its own finder and verifier rounds, its own GitHub review per round, its own rebase onto the lane below. Four 300-line fixes that share nothing are still four review loops, where one pull request with four commits is one loop and a reviewer who can read the commits in order. The line-count advice is still right about what a reviewer can hold at once; it is just not the only cost any more.

So the default flipped, and the exception has to earn its place: more than about five hundred changed lines, or a shared layer that other layers build on and a reviewer needs to read alone.

Step 1: the parser refuses a split with no reason

Create plan.mjs. The plan is markdown; the function finds the ## Work items section, requires the reason line, and never reads the reason line as a work item.

// plan.mjs
export function workItemsFromPlan(markdown) {
  const section = /^##\s+Work items\s*$([\s\S]*?)(?=^##\s|\s*$(?![\s\S]))/m.exec(markdown);
  if (!section) return { items: [], why: null };            // one pull request: the default
  const body = section[1];

  // [ \t], not \s: an empty reason must not slurp the next line as the reason.
  const why = /^[ \t]*(?:[-*][ \t]+)?\**why split\**[ \t]*:[ \t]*\**[ \t]*(\S.*)$/im.exec(body);
  if (!why) {
    throw new Error('the plan has a `## Work items` heading but no `Why split: <the size or the layer, in numbers>` line under it; ' +
      'one pull request per issue is the default, and a split has to say what makes this change too large to review as one');
  }

  const items = [];
  for (const line of body.split('\n')) {
    if (/^\s*(?:[-*]\s+)?\**why split\**\s*:/i.test(line)) continue;   // the reason line is never a lane
    const m = /^\s*[-*]\s+([a-z0-9][a-z0-9-]*):\s+(\S.*)$/i.exec(line);
    if (m) items.push({ slug: m[1], what: m[2].trim() });
  }
  if (items.length < 2) throw new Error('a split with fewer than two work items is not a split');
  return { items, why: why[1].trim() };
}

Step 2: tell the planner, and tell the red team

The parser is the last line. The planner’s brief carries the same rule in prose so the model rarely reaches it: one pull request is the default even when the issue bundles several independent fixes, those become one pull request with one commit each, and the plan says so under its Approach heading. And the red team that attacks the plan now attacks the split before the items: several small independent fixes split into lanes is a high finding, which in this loop blocks approval.

Putting the rule in three places is deliberate. The brief shapes the artifact, the reviewer catches the artifact that ignored the brief, and the parser refuses the artifact that got past both.

Use it

Save this as demo.mjs beside plan.mjs:

// demo.mjs
import { workItemsFromPlan } from './plan.mjs';

const attempt = (label, md) => {
  try { console.log(`${label}:`, JSON.stringify(workItemsFromPlan(md))); }
  catch (e) { console.log(`${label}: REFUSED (${e.message.split(';')[0]})`); }
};

attempt('no heading', '## Approach\n\nFour independent fixes, one commit each.\n');

attempt('heading, no reason', '## Work items\n\n- notes: lock the rewrite\n- obs: validate the enum\n');

attempt('empty reason', '## Work items\n\nWhy split:\n- notes: lock the rewrite\n- obs: validate the enum\n');

attempt('with reason', [
  '## Work items', '',
  'Why split: 1,140 changed lines; the storage layer is read by both tools and must land first',
  '- storage: content handles and the locked rewrite',
  '- tools: update and delete take a handle',
  '',
].join('\n'));

Run node demo.mjs. You should see the default pass through as no items, two refusals, and one accepted split with its reason attached:

no heading: {"items":[],"why":null}
heading, no reason: REFUSED (the plan has a `## Work items` heading but no `Why split: <the size or the layer, in numbers>` line under it)
empty reason: REFUSED (the plan has a `## Work items` heading but no `Why split: <the size or the layer, in numbers>` line under it)
with reason: {"items":[{"slug":"storage","what":"content handles and the locked rewrite"},{"slug":"tools","what":"update and delete take a handle"}],"why":"1,140 changed lines; the storage layer is read by both tools and must land first"}

Gotchas

An empty reason line slurps the next line. The obvious regex puts \s* after the colon. \s matches a newline, so Why split: followed by nothing made the next line, the first work item, into the reason, and the parser accepted a split whose only justification was its own first lane. Use [ \t] on that line; a reason has to be on the line that claims to be one.

The reason line looks like a work item. It sits under the same heading and can start with the same bullet. The loop that collects items has to skip it explicitly, or a plan gets a phantom lane named why.

A frozen fixture from before the rule is a real artifact, not a test to rewrite. My baseline plan predates Why split:, and it really did split. It was carried into the new shape with one marked reason line and its text otherwise unchanged, so the baseline still describes a run that happened rather than one that would pass today.

Sources

Small CLs, Google Engineering Practices — what “small” means and why small changes review better
Speed of Code Reviews, Google Engineering Practices — the reviewer’s standing move on a huge change is to ask for a split
Best Practices for Peer Code Review, SmartBear — the 200 to 400 line review window from the Cisco study

Changelog

fix(issueflow): one pull request per issue is the default — a split has to say why (#249) (0920c04)