Structural Review

Structural Review

Every other review axis in this pack asks "is this wrong?" This one asks "is this making the codebase worse?"

A change can be correct, secure, well-typed, and green while pushing a file past any sane size, scattering a flag across six call sites, or adding a fifth special case to a busy method. That is the only thing this review looks for.

The organising idea is structural judo: hunt for restructurings that preserve behavior while making whole branches, helpers, modes, or layers disappear. "This could be a bit cleaner" is not a finding. "These three conditionals stop existing if the caller passes a resolved value" is.

Non-Coverage

This axis is deliberately single-purpose. It does not cover:

Do not spend a finding on anything in the right-hand column. If the diff is structurally fine, say so and stop; do not pad the report with observations another axis already owns.

Resolving The Two Real Overlaps

Two standards touch a neighboring skill. Resolve them this way rather than firing both skills on the same diff:

• Standard 5 (type and boundary cleanliness) vs type-design-review. type-design-review owns the question "what invalid state does this type permit?" — it is the deeper axis and it wins whenever the diff adds or changes data models, signatures, domain types, parsing boundaries, casts, or state machines. Structural review keeps only the structural consequence of loose boundaries: a signature so untyped that call sites must each re-derive or re-check the same facts, producing duplicated defensive code. State that duplication as the finding. If the sharper statement is "this type permits an invalid state", hand it to type-design-review and do not restate it here.
• Standard 8 (sequential orchestration and non-atomic updates) vs benchmark-verification. benchmark-verification owns any claim about measured speed; it requires baseline-vs-patched evidence and this skill has none. Structural review may only flag orchestration shape: independent work written as a forced sequence, or a multi-step state update with no atomic boundary and a visible partial-failure window. Never assert a change is slow. If the concern is really "is this slower?", route it to benchmark-verification and record that routing instead of a finding.

Precedence

Repo-local convention overrides the generic baseline below, always. Read the repo's AGENTS.md and the policy it names in .agents/agent-workflow.yml first. If the repo documents a different file-size norm, a layering rule, or an accepted pattern that a standard here would flag, the repo wins and the standard is silent. Say which convention you applied.

Step 1 — Pin The Fixed Point Before Reading Any Code

Structural review compares a state against a baseline, so an unpinned or empty diff produces a confidently wrong review. Resolve the base branch from the base_branch key in .agents/agent-workflow.yml (or from PR metadata when a PR is open), then pin the exact merge-base three-dot diff:

base=$(gh pr view --json baseRefName --jq .baseRefName 2>/dev/null || ruby -ryaml -e 'p=(YAML.safe_load(File.read(".agents/agent-workflow.yml"), aliases: false) || {}); puts(p.fetch("base_branch", "main"))')
git rev-parse --verify "origin/$base"
git merge-base "origin/$base" HEAD
git diff --stat "origin/$base...HEAD"
git diff --name-only "origin/$base...HEAD"

Resolve base before the first command. PR metadata wins when a PR is open, because a PR targeting something other than the repo default would otherwise pin the review against the wrong baseline — the failure this step exists to prevent. The .agents/agent-workflow.yml value is the fallback when gh finds no PR. An unset base makes git rev-parse --verify "origin/" fail loudly; that is the intended fail-closed behavior, not a reason to skip the assignment. Keep it exported for the later steps, or re-resolve it the same way.

Stop and report if the ref does not resolve or the diff is empty. Do not substitute a two-dot diff, a working-tree diff, or "recent commits" — fail here rather than producing a review of the wrong change set. Record the base SHA and head SHA; every finding is anchored to them.

For a merged range (the post-merge-audit entry point), pin BASE..HEAD explicitly the same way and record both SHAs.

Step 2 — Read Whole Files, Not Hunks

Structure is invisible in a hunk. For every file with more than about 10 changed lines, read the whole file, not the diff fragment. File size, the number of conditional sites, layer placement, and near-duplicate helpers are all properties of the file, and a hunk hides every one of them.

Get the post-change totals, because Standard 1 is evaluated on the resulting file, not on the lines added:

# Reuse the `base` resolved in Step 1, or re-resolve it in a fresh shell.
git diff --numstat "origin/$base...HEAD"
wc -l path/to/changed_file

Skip what tooling already enforces. If the repo runs a linter, formatter, or type checker, those failures are CI's job and never a finding here.

Step 3 — Apply The Standards

Full catalog with symptoms and worked restructurings: references/code-smells.md. Load it when a candidate finding needs the detailed test; the summary below is the working checklist.

0. Be ambitious about structural simplification. Prefer the restructuring that deletes complexity over the one that rearranges it. Ask what would have to be true for this branch, mode, helper, or layer to not exist.
1. File size. Do not push a file past roughly 1000 lines without strong justification, evaluated on the post-change total. The finding is the growth plus a named split, not the number alone.
2. No random spaghetti growth. A new ad-hoc conditional dropped into an unrelated flow is a design problem, not a nit. Flag the placement, not the condition.
3. Clean the design, do not just accept working code. "It passes" is not the bar when the change leaves a structure the next author must work around.
4. Prefer direct and boring over magical. Flag thin abstractions, pass-through wrappers, indirection with one caller, and configuration that exists to avoid writing the direct call.
5. Type and boundary cleanliness — structural consequence only; see the overlap rule above.
6. Keep logic in the canonical layer. Business rules leaking into controllers, views, serializers, migrations, or scripts is a finding even when the code is correct there.
7. Feature flags must not become permanent branching debt. Flag a new flag with three or more conditional sites, no named cleanup path, nested flag conditions, or a branch that guards a data write — that last one can leave the two paths writing incompatible state, so it is a finding at one site.
8. Orchestration shape — sequencing and atomicity only; see the overlap rule above.

A finding must name: the standard, the location, the structural cost that already exists in the post-change code, and a concrete restructuring. A finding without a restructuring is an opinion; drop it.

Step 4 — Severity

Structural findings are almost never merge blockers. Default them to P2 / should_fix, or P3 / deferred when the work is real but clearly a follow-up. Reserve P1 for a file-size or layer violation the author cannot justify — that is, one where the growth or misplacement was avoidable, was pointed out, and has no stated reason. P0 is not a structural severity; if a structural observation is genuinely release-blocking, it is a correctness or security finding and belongs to another axis.

Human-facing labels map to the shared severities in docs/review-finding-schema.md as:

• BLOCKER → P1 / must_fix (rare here; requires the unjustified-violation test above)
• MAJOR → P2 / should_fix
• SUGGESTION → P3 / deferred

Step 5 — Verdict And Output

Close with exactly one verdict:

• APPROVE — no structural finding, or only P3 follow-ups.
• REFINE — the design is right; specific P2 cleanups are named.
• RETHINK — the change's structure is the problem, and the named restructuring is large enough that the author should reconsider the approach before more work lands on it.

Report:

• base SHA, head SHA, and how the fixed point was pinned
• files read in full vs. read as diff, and why
• repo-local conventions applied that overrode a generic standard
• findings, each with standard, location, structural cost, restructuring, and severity
• overlaps routed to type-design-review or benchmark-verification
• the verdict

Machine-Readable Findings

When structured output is requested, emit findings using docs/review-finding-schema.md with source: structural-review on each finding and set review_receipt.source to structural-review when emitting a receipt. Populate optional receipt provenance.model, provenance.effort, and provenance.usage only from host-reported evidence for the actual review run. Use literal UNKNOWN for unavailable values; never infer them or treat prompt text or model self-report as binding evidence.

Entry Points

• After a batch audit: invoke $structural-review explicitly on the same range audited by $post-merge-audit when you want the codebase-health lens. post-merge-audit does not auto-run structural-review for you.
• On demand pre-merge, when a PR grows a file substantially, adds a flag, or moves logic between layers.

This is not a closeout gate and must not be wired in as one. autoreview's scope discipline — small fixes at the right boundary, no refactors — stays exactly as it is; this skill exists precisely so that discipline does not have to be loosened.

Source Note

Adapted from the upstream thermo-nuclear-code-review skill in intercom/2x-skills, reworked here as a portable seam-driven axis with this pack's severity vocabulary and review-boundary rules. See the pinned upstream MIT license and copyright notice.