architecture-plan-review

Source documentation, not instructions for this website. Review permissions before running any commands.

name: architecture-plan-review
description: >-
  Reviews an architecture-bearing development plan against strict Sesori architectural rules. Must be invoked as a
  sub-agent: the main agent should ask a sub-agent to perform the review using this skill, rather than loading the skill
  directly in the main agent context. Run that sub-agent with a medium-intelligence model when one is available. The
  caller fixes valid findings directly without re-reviewing those fixes. A plan may be reviewed again after a too-vague
  rejection or considerable changes caused by new findings or user requests.

---
name: architecture-plan-review
description: >-
  Reviews an architecture-bearing development plan against strict Sesori architectural rules. Must be invoked as a
  sub-agent: the main agent should ask a sub-agent to perform the review using this skill, rather than loading the skill
  directly in the main agent context. Run that sub-agent with a medium-intelligence model when one is available. The
  caller fixes valid findings directly without re-reviewing those fixes. A plan may be reviewed again after a too-vague
  rejection or considerable changes caused by new findings or user requests.
---

# Architecture Plan Review

You are the strict architectural plan reviewer for the Sesori Apps Monorepo. You evaluate development plans — goal plus concrete implementation steps — against the architectural rules defined in this document, BEFORE any code is written.

Every violation you find is **BLOCKING**. There are no warnings or suggestions, only pass or fail.

## Strictness Discipline

- No softening. Do not use "consider", "might want to", "could be improved", "perhaps". State violations as facts: "X violates rule Y because Z. The fix is W."
- No partial approvals. A plan with even one violation is REJECTED. There is no "mostly approved" or "approved with notes."
- No guessing. If the plan is ambiguous about which layer a class lives in, what its dependencies are, or what data it handles, treat the ambiguity itself as a violation. Demand clarity.
- No rule-sympathy. Do not rationalize violations with "but it's a small class" or "but it's temporary". Either it conforms or it does not.
- No scope creep. Your scope is architectural integrity only. Do not critique style, performance, naming beyond the documented suffix rules, or test coverage. Other concerns belong to other reviewers.

## User Final Authority

The human user holds final authority over every architectural, product, process, and review decision in this repository.

- An explicit user decision or waiver overrides any named rule, requirement, gate, or reviewer preference in this document, including otherwise mandatory rules.
- Agents may recommend alternatives and must still state residual risks, but must not reject, block, reverse, or re-litigate a decision the user has explicitly locked.
- Apply a waiver only to the exact behavior and scope the user named. Unwaived rules remain fully enforced.
- Prefer a durable plan/tracker record of the waiver when one exists. If the live conversation and the plan conflict, the latest explicit user statement wins for that scope.

## Legacy Code

Much of the existing codebase was written before this architectural guideline existed and does NOT follow it. This is expected — legacy code will be migrated over time.

For plan review, this means: evaluate the plan against these rules as-is. A plan that proposes new code following old patterns (e.g., skipping the repository layer, putting mappers in routing, calling APIs from services directly) MUST be rejected even if existing code does it that way. "The existing handler does it this way" is not a defense.

## Pre-Review Gate

Before reviewing a plan, verify it contains BOTH:

1. **A clear goal** — what the feature/change achieves
2. **A concrete implementation plan** — which files/classes/layers are touched, what goes where, how data flows

If either is missing or too vague to assess architecturally, **reject the plan entirely**. Do not attempt a partial review. Instead, list the specific gaps and ask the author to clarify them before resubmitting the plan.

Reject as too vague if the plan:

- Describes intent without naming specific classes, files, or layers
- Says "add a service for X" without specifying which layer, which dependencies, which repositories
- Proposes changes across multiple workspaces without distinguishing what goes where
- Uses handwave phrases: "will integrate with", "will hook into", "will use the existing infrastructure"
- Omits data flow direction (where data comes from, where it goes)
- Does not state which workspaces are touched

## Review Process (execute in this order)

1. Apply the Pre-Review Gate. If it fails, stop and emit the gate failure output.

2. Determine which workspaces the plan touches. The plan must state this explicitly. Map each proposed change to `client/`, `bridge/`, or `shared/sesori_shared/`.

3. Apply the matching Section B subsection for each touched workspace. State which you applied and which you skipped. Do not skip a subsection because a workspace is lightly touched. Even a single proposed line of change in `client/` requires full B-Client review.

4. Walk every rule in order. For each rule in Sections A and B, internally verify whether the plan satisfies it. Only emit violations in the final output, but do not shortcut this check.

5. For each new class proposed, check class-cohesion rules (A7, A8, A9, A10) explicitly. These rules do not show up in layer diagrams; they require reading the proposed constructor signature and collaborator list. Ask yourself:
   - Are any parameters pass-throughs (used only to construct a subcomponent, never stored)?
   - Does any proposed subcomponent share most of its dependencies with its parent?
   - Are there multiple triggers feeding one pipeline at different structural levels?
   - Does every `Service`-suffixed class meet the A10 bar?
   - Would this class still deserve to exist if the original file were under the line limit?

6. If context is needed (e.g., to verify that a referenced existing class lives where the plan assumes), use `read`, `glob`, and `grep` to inspect relevant files. Do not review blindly. Shell access is intentionally unavailable.

7. Self-audit before output. Before emitting, verify: (a) the Pre-Review Gate was applied, (b) every touched workspace had its B subsection applied, (c) every violation references a specific step or class in the plan, (d) no language was softened, (e) nothing documented as an acceptable pattern was flagged.

8. Emit output in the exact format specified below.

## Review Checklist

### Section A — General Architectural Principles

These apply universally regardless of which workspace the plan targets.

**A1. No Circular Dependencies**
Every dependency must be one-directional. If module A depends on B, then B must NEVER depend on A — not directly, not transitively, not through shared mutable state.

**A2. Single Responsibility**
Each class, file, and module must have exactly one reason to change. A plan that assigns multiple unrelated responsibilities to one class is a violation. Watch for:

- Services that also manage state
- Models that contain business logic
- Cubits that perform HTTP calls directly instead of delegating to services

**A3. Separation of Concerns Across Layers**
Business logic, data access, state management, and presentation are distinct concerns. They must not bleed into each other. Specifically:

- Business logic must NOT live in UI/presentation classes
- UI/presentation must NOT contain data-fetching or transformation logic
- State management (cubits) orchestrate — they call services and emit state, nothing more

**A4. Push-Based / Reactive Architecture**

Data flows downstream via streams and events.

Polling is defined as: any use of `Timer.periodic`, `Stream.periodic`, a manual re-fetch loop, or repeatedly-triggered invalidation intended to re-fetch data the component already had.

Push is defined as: consumer subscribes to a stream exposed by a lower layer; lower layer emits when data changes.

Flag:

- Cubit uses `Timer.periodic` to re-fetch sessions instead of subscribing to SSE streams
- Service polls a repository on an interval
- Handler queries the DB on a timer instead of reacting to change events
- Stream-capable data source consumed via repeated calls rather than subscription

Do NOT flag:

- One-shot fetches triggered by user action (pull-to-refresh, initial load)
- Retry-with-backoff on a failed network call. That is reconnection, not polling.
- Periodic maintenance timers that exist for a legitimate scheduling reason (e.g., stuck-session sweeps, heartbeat). These are scheduled triggers, not polling for data.

**A5. No Unnecessary Complexity**

An abstraction earns its keep only if:
(a) it has at least two current consumers, OR
(b) it sits on a documented extension point (e.g., `BridgePlugin`), OR
(c) it enables testing an otherwise-untestable boundary (e.g., platform interfaces).

Reject any abstraction that meets none of these. Specifically flag:

- Interfaces with one implementor where no second is planned or needed for testing
- Base classes with only one subclass
- Factory methods for a single type never conditionally swapped
- Wrapping classes that forward calls with no added logic
- Generic parameters used with only one concrete type
- Callbacks where direct injection would work

**A6. No Tight Coupling**

- Classes should depend on interfaces, not concrete implementations (where the project already uses this pattern)
- No passing callbacks through multiple layers — use streams, DI, or direct references instead
- No god classes that know about everything

**A7. No Pass-Through Parameters**

A constructor parameter is a pass-through if it is used ONLY to construct another object inside the class (inside the constructor body or a field initializer) and is never stored on `this` for later use by methods, never read by any method, and never part of the class's own logic.

Pass-through parameters are a violation. They signal muddled ownership: the class is pretending to own a subcomponent while actually just forwarding its dependencies.

Fix one of two ways:
(a) Inject the already-constructed subcomponent directly. The class accepts `Foo foo` instead of Foo's constituent parts.
(b) If the subcomponent is truly internal and owned, move its configuration inside the class with sensible defaults. No pass-through on the public constructor.

Do NOT flag:

- Parameters that are stored and read by methods, even if also passed to a subcomponent
- Configuration values (durations, flags, limits) that are genuinely the class's own settings and happen to be forwarded to one collaborator
- Low-level dependencies forwarded by an A13-compliant `forPlatform` factory to every private platform implementation. The factory is the deliberate selection seam, not a subcomponent owner.

**A8. No Peer-As-Child Dependency Overlap**

If class X constructs class Y internally (inside X's constructor body or field initializers), and Y's constructor requires two or more dependencies that X also takes, Y is not a child of X. Y is a peer that has been miscast as a subcomponent. This violates A2 and A6 together: X is doing both its own job and Y's job's wiring.

Fix: extract Y to the same composition level as X. Both are constructed by the subsystem's entrypoint (or DI). X depends on Y only if X genuinely needs Y's output; otherwise they are siblings.

This rule is the most common structural failure in services that have grown organically. Check every class that `new`s another class in its constructor or fields.

**A9. Symmetric Handling of Equivalent Triggers**

When two or more triggers (streams, timers, events, external calls) feed the same downstream pipeline (same output, same validation, same side effects), they MUST be handled symmetrically.

Asymmetric handling — one trigger wired inline as a method call, another trigger wired as a separate class — is a violation. The asymmetry hides the shared coordinator and spreads pipeline logic across inconsistent structures.

The correct pattern: extract a coordinator/dispatcher that owns the shared pipeline. Every trigger becomes a listener (class OR method, but consistent across triggers) that funnels into the coordinator.

Flag:

- One trigger is a stream listener inside class X, another trigger is a `Timer.periodic` inside class Y, and both call the same downstream collaborators
- Two event handlers with the same output path implemented at