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?