diff --git a/.copilot/skills/architectural-review/SKILL.md b/.copilot/skills/architectural-review/SKILL.md new file mode 100644 index 000000000..8b8e11347 --- /dev/null +++ b/.copilot/skills/architectural-review/SKILL.md @@ -0,0 +1,200 @@ +--- +name: "architectural-review" +description: "How to review PRs for architectural quality — module boundaries, dependency direction, export surface, pattern consistency" +domain: "architecture" +confidence: "medium" +source: "extracted from copilot-instructions.md patterns and codebase conventions" +--- + +## 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):** + +| File | Purpose | +|------|---------| +| `packages/squad-cli/src/cli/core/detect-squad-dir.ts` | Finds `.squad/` at startup | +| `packages/squad-cli/src/cli/core/errors.ts` | Error classes (`SquadError`, `fatal()`) | +| `packages/squad-cli/src/cli/core/gh-cli.ts` | GitHub CLI wrapper | +| `packages/squad-cli/src/cli/core/output.ts` | Color/emoji console output | +| `packages/squad-cli/src/cli/core/history-split.ts` | Portable knowledge separator | + +**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 diff --git a/.copilot/skills/reviewer-protocol/SKILL.md b/.copilot/skills/reviewer-protocol/SKILL.md index 5d589105c..cc3543edc 100644 --- a/.copilot/skills/reviewer-protocol/SKILL.md +++ b/.copilot/skills/reviewer-protocol/SKILL.md @@ -68,6 +68,12 @@ When an artifact is **rejected** by a Reviewer: 4. Hockney: "Verbal, then" 5. Coordinator spawns Verbal +## Related Review Skills + +For domain-specific review checklists, see: +- **Architectural Review:** `.copilot/skills/architectural-review/SKILL.md` — module boundaries, dependency direction, export surface, sweeping refactor safety +- **Security Review:** `.copilot/skills/security-review/SKILL.md` — credentials, injection, workflow permissions, supply chain + ## Anti-Patterns - ❌ Allowing the original author to self-revise after rejection diff --git a/.copilot/skills/security-review/SKILL.md b/.copilot/skills/security-review/SKILL.md new file mode 100644 index 000000000..aa79a8734 --- /dev/null +++ b/.copilot/skills/security-review/SKILL.md @@ -0,0 +1,266 @@ +--- +name: "security-review" +description: "How to review PRs for security — credentials, injection, workflow permissions, supply chain, git operation safety" +domain: "security" +confidence: "medium" +source: "extracted from copilot-instructions.md patterns and GitHub Actions security best practices" +--- + +## Context + +Every PR that touches authentication, credentials, environment variables, file system operations, child process execution, user input handling, GitHub API calls, or workflow files must be reviewed for security. This skill provides a systematic checklist for catching security issues before they reach production. + +Use this skill when a PR includes any of: +- Changes to `.github/workflows/` files +- New or modified environment variable usage +- File system read/write operations driven by user input +- Child process execution (`child_process.exec`, `child_process.spawn`) +- GitHub API calls (Octokit, `gh` CLI wrappers) +- New npm dependencies +- Credential or token handling +- Template rendering with dynamic values + +## Patterns + +### 1. Secrets and Credentials + +**No secrets in source code — ever.** + +**Review checklist:** +- ❌ No API keys, tokens, passwords, or connection strings in committed files +- ❌ No hardcoded `GITHUB_TOKEN`, `NPM_TOKEN`, or similar values +- ❌ No `.env` files committed (verify `.gitignore` covers them) +- ✅ Secrets must be referenced via `process.env.X` or `${{ secrets.X }}` in workflows +- ✅ If the PR adds a new secret reference, verify it's documented in setup instructions + +**Watch for disguised secrets:** +- Base64-encoded strings that decode to tokens +- URLs with embedded credentials (`https://user:token@host`) +- Test fixtures that contain real credentials from copy-paste + +### 2. Personal Data in Committed Files + +**Personal data (especially email addresses) must never be written to committed files.** + +**Squad-specific rule:** `git config user.email` is explicitly banned from being written to any file that gets committed. This prevents PII leakage into the repository. + +**Review checklist:** +- ❌ No email addresses written to committed files (even in config generation) +- ❌ No `git config user.email` output captured and stored in source +- ✅ If user identity is needed at runtime, read it dynamically and never persist to a committed file + +### 3. Environment Variable Safety + +**Review checklist:** +- Are environment variables validated before use? (e.g., checking for `undefined`) +- Are sensitive environment variables excluded from logs and error messages? +- Does the PR avoid writing environment variable values to committed files? +- If a new `process.env.X` reference is added, is the variable documented? + +### 4. File System Safety + +**Path traversal prevention:** +- ❌ No direct concatenation of user input into file paths (`path.join(baseDir, userInput)` without validation) +- ✅ Validate that resolved paths stay within expected boundaries (use `path.resolve` and verify the result starts with the expected base directory) +- ✅ Reject paths containing `..`, absolute paths when relative are expected, or null bytes + +**Arbitrary file write prevention:** +- If user input determines a file path, verify the code constrains writes to a known safe directory +- Template rendering that writes files should validate output paths against a whitelist or known directory + +### 5. Child Process Safety + +**Shell injection prevention:** +- ❌ No unsanitized user input passed to `child_process.exec()` (exec uses a shell and is vulnerable to injection) +- ✅ Prefer `child_process.execFile()` or `child_process.spawn()` with argument arrays (no shell interpretation) +- ✅ If `exec` must be used, validate/sanitize all interpolated values +- ✅ Check for template literals in shell commands — `exec(\`git commit -m "${userMessage}"\`)` is injectable + +**Review checklist for `gh` CLI wrappers:** +- The `gh-cli.ts` module wraps GitHub CLI calls. Verify that arguments are passed as array elements, not interpolated into a shell string +- If the PR modifies `gh-cli.ts` or adds new `gh` commands, check for injection vectors + +### 6. GitHub Actions Workflow Security + +**`pull_request_target` safety:** +- `pull_request_target` + checking out PR code = **critical security risk** (the workflow runs with write token but executes untrusted code from the fork) +- `pull_request_target` + NO code checkout = **safe** (write token used only with trusted workflow code) +- If a PR adds `pull_request_target`, verify it does NOT checkout `${{ github.event.pull_request.head.ref }}` or `${{ github.event.pull_request.head.sha }}` + +**Token scope minimization:** +- Workflows should declare explicit `permissions:` blocks +- Follow principle of least privilege — no unnecessary `write` permissions +- If the PR adds `permissions: write-all` or omits permissions (defaults to broad), flag it + +**Recursion safety:** +- `GITHUB_TOKEN`-generated events do NOT trigger new workflow runs (GitHub prevents infinite loops) +- If a workflow uses a PAT (Personal Access Token) instead of `GITHUB_TOKEN`, it CAN trigger downstream workflows — verify this is intentional + +**Secrets in workflows:** +- ❌ No `${{ secrets.X }}` in workflow `run:` steps where the value could leak to logs +- ✅ Use `::add-mask::` to mask secret values in workflow output +- ✅ Minimize the number of steps that have access to secrets + +### 7. Git Operation Safety + +**These rules are from the project's mandatory Git Safety guidelines:** + +**Staging safety:** +- ❌ No `git add .` or `git add -A` — these stage unintended deletions from incomplete working trees +- ❌ No `git commit -a` — same risk +- ✅ Only `git add path/to/specific/file.ts` with explicit file paths + +**Push safety:** +- ❌ No `git push --force` or `--force-with-lease` to shared branches (`dev`, `main`) +- ❌ No direct push to `dev` or `main` — must use a PR +- ✅ Feature branches only: `squad/{issue-number}-{slug}` + +**If a PR adds git operations (e.g., in a script, workflow, or agent action), verify every `git add`, `git commit`, and `git push` command follows these rules.** + +### 8. Export/Import Integrity + +Typos in export names cause silent runtime failures — the export resolves to `undefined` instead of throwing. + +**Review checklist:** +- Check for typos in export names (e.g., `FSStorageProvidr` vs. `FSStorageProvider`) +- Verify new re-exports actually reference existing modules +- If the PR renames an export, confirm all import sites are updated +- Watch for `export { X as Y }` where `X` doesn't exist — TypeScript may not catch this in all configurations + +### 9. Dependency Supply Chain + +**New npm dependencies should be audited:** +- Is the package well-maintained? (check last publish date, download count, open issues) +- Does the package have known vulnerabilities? (`npm audit`) +- Is there a built-in Node.js alternative? (prefer `node:fs`, `node:path`, `node:crypto` over third-party equivalents) +- Does the package pull in a large transitive dependency tree? +- Is the package from a trusted publisher? + +**Squad-specific:** Protected bootstrap files must use ONLY `node:*` built-in modules. New dependencies in these files are a **critical** finding. + +### 10. Multi-Account Auth Isolation + +When the `gh` CLI is used in contexts where multiple GitHub accounts might be active: +- Verify that auth tokens are scoped to the correct account +- Check that `gh auth status` is validated before operations +- If the PR adds GitHub API calls, verify it handles auth failures gracefully + +## Examples + +**Example 1: Critical — Shell injection in git command** +``` +PR adds: + exec(`git commit -m "${commitMessage}"`) + +Finding: [critical] Shell injection — commitMessage is user-controlled. +A message containing "; rm -rf /" would execute arbitrary commands. +Fix: Use execFile('git', ['commit', '-m', commitMessage]) instead. +``` + +**Example 2: Critical — pull_request_target with checkout** +``` +PR adds workflow: + on: pull_request_target + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.ref }} + - run: npm test + +Finding: [critical] pull_request_target + PR head checkout = untrusted +code execution with write token. An attacker can modify package.json +scripts in their fork to exfiltrate GITHUB_TOKEN. +Fix: Use 'pull_request' trigger instead, or do not checkout PR code. +``` + +**Example 3: High — Email written to committed file** +``` +PR adds to init.ts: + const email = execSync('git config user.email').toString().trim(); + fs.writeFileSync('.squad/config.json', JSON.stringify({ author: email })); + +Finding: [high] Personal data (email) written to committed file. +This violates the PII-in-source rule. Squad config files are committed +to the repo and shared across contributors. +Fix: Read email at runtime only, never persist to committed files. +``` + +**Example 4: Medium — Overly broad workflow permissions** +``` +PR adds workflow with no permissions block (inherits default write-all): + on: push + jobs: + deploy: + runs-on: ubuntu-latest + +Finding: [medium] No explicit permissions block. Workflow inherits +default permissions which may be overly broad. +Fix: Add 'permissions:' block with minimum required scopes. +``` + +**Example 5: Medium — New dependency when built-in exists** +``` +PR adds to package.json: + "dependencies": { "mkdirp": "^3.0.0" } + +Finding: [medium] mkdirp is unnecessary — Node.js built-in +fs.mkdirSync(path, { recursive: true }) provides the same functionality. +Fix: Use node:fs built-in instead of adding a new dependency. +``` + +**Example 6: Low — Missing environment variable validation** +``` +PR uses process.env.SQUAD_API_KEY without checking if it's defined: + const apiKey = process.env.SQUAD_API_KEY; + fetch(url, { headers: { Authorization: `Bearer ${apiKey}` } }); + +Finding: [low] No validation that SQUAD_API_KEY is defined. +If undefined, the Bearer token will be "Bearer undefined" which +may produce confusing auth errors. +Fix: Check for undefined and throw a descriptive error early. +``` + +## Review Output Format + +Structure your security review with severity levels: + +``` +## Security Review + +**Verdict:** APPROVE | APPROVE WITH NOTES | REQUEST CHANGES | REJECT + +### Findings + +1. [severity: critical] — description + - File(s): path/to/file.ts:L42 + - Risk: what could go wrong + - Fix: specific remediation + +2. [severity: high] — ... + +3. [severity: medium] — ... + +4. [severity: low] — ... + +### Severity Guide +- **Critical:** Exploitable vulnerability, credential exposure, or code execution risk. Must fix before merge. +- **High:** Security design flaw that could lead to exploitation. Must fix before merge. +- **Medium:** Security best practice violation. Should fix before merge. +- **Low:** Minor hardening opportunity. Can fix in a follow-up PR. + +### Summary +Brief overview of security posture and required actions. +``` + +## Anti-Patterns + +- ❌ Approving PRs with `exec()` and user-controlled input ("it's just a commit message") +- ❌ Ignoring `pull_request_target` trigger changes ("the workflow looks fine otherwise") +- ❌ Skipping dependency audit for "small" packages ("it's only 10 lines of code") +- ❌ Treating `git add .` in scripts as acceptable ("it works on my machine") +- ❌ Allowing secrets in test fixtures ("it's just a test, not production") +- ❌ Approving workflows without explicit `permissions:` blocks ("defaults are probably fine") +- ❌ Dismissing PII in committed files ("it's just an email address") +- ❌ Ignoring export typos ("TypeScript would catch that") — not always true with re-exports +- ❌ Allowing force pushes to shared branches in scripts ("we know what we're doing") +- ❌ Skipping review of new `node_modules` additions in bootstrap files ("it's a small package") diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 8a843118f..64f680876 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -106,6 +106,16 @@ When opening a PR: - If this is a 🟡 needs-review task, add to the PR description: `⚠️ This task was flagged as "needs review" — please have a squad member review before merging.` - Follow any project conventions in `.squad/decisions.md` +## PR Review Skills + +When reviewing or creating PRs, consult these skills for domain-specific checklists: + +- **Reviewer Protocol:** `.copilot/skills/reviewer-protocol/SKILL.md` — rejection workflow, lockout rules +- **Architectural Review:** `.copilot/skills/architectural-review/SKILL.md` — module boundaries, dependency safety, pattern consistency +- **Security Review:** `.copilot/skills/security-review/SKILL.md` — credentials, injection, workflow permissions, supply chain + +Read the relevant skill(s) before submitting or reviewing a PR that touches their domain. + ## Decisions If you make a decision that affects other team members, write it to: