Architecture review

Iron Law

DEPENDENCIES FLOW INWARD -- INNER LAYERS NEVER DEPEND ON OUTER LAYERS
YOU MUST verify dependency direction for every new class and every refactor. A layer violation in a PR means the PR is NOT mergeable until it is fixed. No exceptions.

Violating the letter of this rule is violating the spirit of this rule.

Announce at start: "I am using the architecture-review skill to review [component/file]."

---

The Layer Architecture

Software layers are concentric: inner layers hold domain logic and policy; outer layers hold I/O, frameworks, UI, and external integrations. Dependencies point inward only -- an inner layer never imports, calls, or names a type from an outer layer. Every file belongs to exactly one layer. Judge each change by which layer its file occupies and which direction its dependencies run.

---

Hyrum's Law Gate

"With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended upon by somebody."

Before modifying any observable behavior of an existing component -- including undocumented behaviors, error messages, return value nuances, timing, side effects -- ask:

"What else could currently be depending on this?"

This includes:

• Tests that happen to rely on the current error message text
• Callers that rely on the current return value for edge cases not covered by docs
• Visual tests that rely on the current rendering output being exactly pixel-perfect

Gate: If you cannot enumerate all call sites that might depend on the behavior being changed, dispatch an explore agent to find them before proceeding. Changing behavior without knowing who relies on it is a silent breaking change.

This does not mean "never change behavior." It means: be deliberate. Know what you're breaking. Break it intentionally, with all dependents updated in the same commit.

---

Barricade Model

The codebase has a dirty zone (data that has not been validated) and a clean zone (data that has passed all validation). The barricade is the boundary between them.

Rules:

• Code in the clean zone never handles dirty data. If it receives data, that data is already validated.
• Validation happens at the barricade, not scattered through clean-zone code
• Data enters dirty through file I/O, user input, and external APIs. It becomes clean after parsing and validation in the input handlers.

Zone	Contains	May assume
Dirty	File bytes, user input strings, socket data	Nothing -- validate everything
Barricade	Parser, validator, loader functions	Converts dirty -> clean
Clean	Validated domain objects past the barricade	Data is valid -- no defensive checks needed

Violation signal: An inner-layer domain class checking for null or empty data it received from an orchestrator that already validated it. That check belongs at the barricade, not in the clean zone.

---

BEFORE PROCEEDING

Run every item for each file under review:

1. Does the new or modified class belong to a defined layer?
2. Does it import or call code from an outer layer? (VIOLATION if yes)
3. Does an orchestrator or coordinator class orchestrate rather than implement? (it must delegate -- business, rendering, or compute logic belongs in domain classes, not the orchestrator)
4. Do any production (src/) files import from tests/? (VIOLATION -- production code must never depend on test code)
5. Are there circular #include or import dependencies between any two files in the same layer?

[+] All pass -> verdict: APPROVE [-] Any fail -> verdict: REQUEST CHANGES -- document every failure in the Review Report

---

Review Report Format and Dispatch Pattern

For the report table template and per-file dispatch instructions, see references/ARCH_REVIEW_TEMPLATES.md.

A verdict of REQUEST CHANGES means the PR is NOT mergeable until every finding in the table is resolved (findings classified judgment call do not block approval on their own).

---

Red Flags -- STOP

If you catch yourself thinking any of the following, STOP before writing your verdict:

• "It only imports one symbol from an outer layer, that is a tiny coupling" -> Stop. One outward-pointing import is still an inward-dependency violation. Flag it.
• "The orchestrator has just this one piece of logic inline, that is fine" -> Stop. Orchestration means delegation. Business, rendering, or compute logic belongs in a domain class, not the orchestrator.
• "The test utility is small, it won't hurt in src/" -> Stop. Test code in production is a maintenance trap. Flag it.
• "It's in the same layer, so the dependency is acceptable" -> Stop. Same layer, different files -- check for circular dependencies. Design smell if found.
• "The violation is minor, I'll note it but give APPROVE" -> Stop. There is no APPROVE with open violations -- only a finding classified judgment call may stand without blocking. Verdict is REQUEST CHANGES.
• "It's what we already use/know" -> Stop. Familiarity is not fit. Name the problem-fit argument or run a comparison against at least one alternative; familiarity is a trade-off to state, not a silent default.

---

Rationalization Prevention

Excuse	Reality
"Just a small dependency, doesn't affect architecture"	Architecture violations compound. Small ones become load-bearing. Fix them now.
"The test utility is tiny, no harm putting it in src/"	Test-only code in production is a maintenance trap and blurs the test boundary.
"Circular dependency is fine since they're in the same layer"	Same layer does not mean circular is acceptable. It is still a design smell requiring resolution.
"I'll refactor it properly later"	Later never comes. Fix the boundary violation before this code ships.
"The PR is urgent, we can clean up the architecture next sprint"	Urgency is the most common rationalization for shipping violations. The law applies under urgency too.

---

Related Skills

• code-quality -- naming conventions and C++ patterns; architecture-review checks structure, code-quality checks form
• testing -- governs what lives in tests/ vs src/testing/; architecture-review enforces the boundary
• infrastructure-review -- CI/CD pipeline, build, and packaging configuration; architecture-review checks source structure
• oop-principles -- sub-domain skill; run Is-A / Has-A and SOLID (Single Responsibility, Open/Closed, Liskov Substitution, Interface Segregation, Dependency Inversion) gate for every class hierarchy change reviewed here

Design patterns and architectural principles: references/DESIGN_PATTERNS.md and references/ANTIPATTERNS.md