architectural-review

Context

When reviewing a PR that touches core architecture, the reviewer must verify that structural invariants are preserved. This skill covers reviewing existing PRs for architectural quality — not writing proposals (see architectural-proposals/SKILL.md for that).

Use this skill when a PR includes any of:

• New modules, packages, or directories
• Dependency changes (package.json, import additions across package boundaries)
• API surface changes (new or modified exports from index.ts barrel files)
• Cross-package imports (anything crossing the SDK/CLI boundary)
• New abstractions (interfaces, base classes, registries, factories)
• Sweeping refactors (file renames, pattern migrations, bulk conversions)

Patterns

1. Module Boundary Enforcement

The Squad codebase has two core packages with a strict dependency direction:

squad-cli → depends on → squad-sdk
squad-sdk → NEVER depends on → squad-cli

Check every new import statement in the PR:

• CLI files may import from SDK (@bradygaster/squad-sdk)
• SDK files must NEVER import from CLI
• If a PR adds an SDK import that pulls in CLI code, reject it

2. Zero-Dependency Bootstrap Contract

The CLI has protected bootstrap files that must use ONLY Node.js built-in modules (node:fs, node:path, node:child_process, node:util). These files run before the SDK is loaded.

Protected files (authoritative list from copilot-instructions.md):

Review checklist for bootstrap files:

• ❌ No new import or require of anything outside node:*
• ❌ No conversion to FSStorageProvider, StorageProvider, or SDK abstractions
• ✅ Look for — zero dependencies markers in file headers
• ✅ If the PR adds a new bootstrap utility, it must be added to this table and have a zero-dependency regression test

3. Export Surface Safety

Changes to barrel files (index.ts) have downstream impact. Barrel files define the public API of a package.

Review checklist:

• Adding exports: Is the new export intentional? Does it expose implementation details that should stay internal?
• Removing exports: Are downstream consumers accounted for? Removing an export is a breaking change.
• Renaming exports: Same as remove + add — breaks anyone using the old name. Look for typos (e.g., FSStorageProvidr vs. FSStorageProvider).
• Re-exports: If the PR adds export * from './internal-module', verify that the internal module doesn't accidentally expose private types.

4. Sweeping Refactor Safety

When a PR applies a codebase-wide pattern change (e.g., "convert all fs calls to StorageProvider"), verify the 5-step checklist from project instructions:

1. Protected Files checked — None of the protected bootstrap files were converted
2. Zero-dependency markers scanned — Files with — zero dependencies headers were skipped
3. Imports resolve — Every import { X } from '@bradygaster/squad-sdk' references an export that actually exists in the SDK barrel file
4. Not blindly applied — File-specific constraints were respected
5. Batched testing — Changes were tested in logical groups, not all 30 files at once

5. Template Sync Awareness

Template files exist in four locations and must stay consistent:

templates/                          # Source of truth
.squad-templates/                   # Local project templates
packages/squad-cli/templates/       # CLI-bundled templates
.github/workflows/                  # Workflow templates

If a PR modifies a template in one location, check:

• Was the change mirrored to other locations where the same template exists?
• Does TEMPLATE_MANIFEST in templates.ts still correctly map files to agents?
• Were any new templates added to the manifest?

6. .squad/ Leakage Check

.squad/ files (team config, decisions, agent charters) must NOT leak into feature PRs that are about code changes.

Check:

• If the PR is labeled as a feature or bugfix, it should not modify .squad/team.md, .squad/decisions.md, .squad/routing.md, or agent charters — unless the issue specifically calls for it
• .squad/decisions/inbox/ files are acceptable if the PR documents a decision made during implementation
• .squad-templates/ changes are acceptable only if the PR is about template functionality

7. Pattern Consistency

New code should follow established patterns in the codebase. Look for:

• Naming conventions: Does the new module follow existing naming? (e.g., kebab-case file names, PascalCase classes)
• Error handling: Does the new code use SquadError and fatal() from errors.ts, or does it invent its own error patterns?
• Storage abstraction: Does the new code use StorageProvider for file I/O (except in protected bootstrap files)?
• Test patterns: Does the PR include tests that follow the existing test structure?

8. Dependency Direction in core/

The packages/squad-cli/src/cli/core/ directory contains a mix of:

• Early-startup bootstrap utilities (zero external deps — protected)
• Later SDK-dependent modules (may import from SDK)

When reviewing changes to core/ files:

• Verify the file is not in the protected list before accepting SDK imports
• Confirm the SDK is loaded at the point where the file runs in the startup sequence
• Watch for new files added to core/ — determine if they are bootstrap or post-SDK and document accordingly

Examples

Example 1: Reject — SDK import in bootstrap file

PR adds to detect-squad-dir.ts:
  import { FSStorageProvider } from '@bradygaster/squad-sdk';

Finding: REJECT — detect-squad-dir.ts is a protected bootstrap file.
It runs before the SDK is loaded. This import will crash the CLI at startup.
Recommendation: Use node:fs directly. See Protected Files table.

Example 2: Reject — Reverse dependency

PR adds to packages/squad-sdk/src/storage.ts:
  import { detectSquadDir } from '@bradygaster/squad-cli';

Finding: REJECT — SDK must never depend on CLI. Dependency direction is
CLI → SDK, not reverse. Move the shared logic to SDK or extract to a
shared utility within the SDK package.

Example 3: Approve with note — Template change in one location

PR modifies templates/squad.agent.md but not .squad-templates/squad.agent.md

Finding: APPROVE with note — Template was updated in templates/ but the
same file in .squad-templates/ was not synced. Verify whether .squad-templates/
should match or if it intentionally diverges.

Example 4: Reject — Sweeping refactor hits protected file

PR titled "Convert all fs calls to StorageProvider" modifies 15 files,
including packages/squad-cli/src/cli/core/output.ts

Finding: REJECT — output.ts is a protected zero-dependency bootstrap file.
The PR must skip this file. See Sweeping Refactor Rules step 1.

Example 5: Flag — Mass file deletion

PR deletes 25 files as part of "cleanup unused modules"

Finding: FLAG — More than 20 files deleted. This exceeds the red flag
threshold. Verify each deletion is intentional and no downstream imports
are broken. Recommend splitting into smaller PRs.

Review Output Format

Structure your architectural review as:

## Architectural Review

**Verdict:** APPROVE | APPROVE WITH NOTES | REQUEST CHANGES | REJECT

### Findings

1. [severity: critical|high|medium|low] — description
   - File(s): path/to/file.ts
   - Recommendation: what to fix

2. [severity: ...] — ...

### Summary
Brief explanation of architectural impact and any required follow-ups.

Anti-Patterns

• ❌ Approving SDK imports in protected bootstrap files ("it's just one import")
• ❌ Ignoring barrel file changes ("it's just adding an export")
• ❌ Skipping template sync check ("they probably know about the other locations")
• ❌ Accepting sweeping refactors without verifying the 5-step checklist
• ❌ Allowing .squad/ config changes in feature PRs without justification
• ❌ Approving reverse dependencies (SDK importing from CLI) for "convenience"
• ❌ Letting new core/ files slip in without classifying them as bootstrap vs. post-SDK
• ❌ Rubber-stamping PRs that delete >20 files without line-by-line deletion review