bradygaster/squad: Branch protection blocking merges + CI gap analysis

[diberry]

Scope: This issue is specific to bradygaster/squad (Brady's repo). It covers branch protection rulesets blocking merges and what CI checks should replace branch protection on this specific repo. For the broader Squad product question of workflow defaults, minutes budgets, and upgrade safety across all customer repos, see #98.

Problem

Brady and Tamir are stuck merging PRs to dev on bradygaster/squad because of branch protection rulesets.

Current ruleset on bradygaster/squad

Active ruleset: "Copilot review for default branch"

• Applies to: ~DEFAULT_BRANCH (which is dev)
• Rules: deletion, non_fast_forward, copilot_code_review
• Bypass actors: none (nobody can bypass)
• Enforcement: active

Disabled ruleset: "main" (not blocking anything)

Impact

The copilot_code_review rule means every PR targeting dev requires Copilot to review and have no unresolved comments before merging. If Brady or Tamir want to merge their own code quickly, they are blocked by the Copilot review gate.

Options for Brady (repo owner)

Option	What changes	Copilot still reviews?	Can merge without waiting?
Add bypass actors	Brady + Tamir can merge even if Copilot hasn't approved	Yes (comments still appear)	Yes
Change to evaluate mode	Ruleset runs but doesn't block merges	Yes (advisory only)	Yes
Disable ruleset	No Copilot review on dev	No	Yes
Keep as-is	Nothing changes	Yes (hard gate)	No

Recommendation

Add bypass actors (Brady + Tamir) -- this keeps Copilot review as a quality signal while letting trusted maintainers merge when needed. External contributors (like PRs from forks) still go through the full Copilot gate.

Settings path: Settings > Rules > Rulesets > "Copilot review for default branch" > Bypass list

Related

This also affects our fork-first pipeline -- when we publish PRs to upstream, the Copilot review gate on dev means Brady can't merge even clean PRs without Copilot approval. Our pipeline already fixes Copilot issues before publishing, so the gate is redundant for our PRs.

---

CI Check Approaches -- Comparison

Approach	Best for	Brady can toggle?
Repo variables (vars.SQUAD_*)	Global on/off for entire check categories (ESLint, audit, coverage)	Yes -- Settings UI, no code changes
PR labels (skip-lint, skip-tests)	Per-PR overrides when a contributor has a legitimate reason to skip	Yes -- Add/remove label on any PR
Branch protection rulesets	Hard gates that nobody can bypass (or bypass-list for trusted maintainers)	Yes -- Settings UI, but personal repos can't add bypass actors
Required status checks	Blocking merge until specific CI jobs pass	Yes -- Settings UI, pairs with repo variables for which jobs run
CODEOWNERS + review rules	Requiring specific people to approve changes to critical paths	(!) Requires GitHub Teams or paid plan for enforcement
Workflow if conditions	Conditional logic within a workflow (path filters, changed-file checks)	No -- Requires editing .yml files

Recommended combination

1. Repo variables for global toggles (Brady flips a switch, no PRs needed)
2. PR labels for per-PR exceptions (visible in PR history, auditable)
3. Advisory mode first -- new checks warn but don't block until proven stable
4. No branch protection -- all value delivered through CI checks instead

[diberry]

CI Checks Analysis: Replacing Branch Protection Rulesets

Executive Summary

The branch protection ruleset currently provides 3 key protections:

1. Deletion protection -- prevents accidental branch deletion
2. Non-fast-forward protection -- enforces linear history (no force pushes)
3. Copilot code review requirement -- blocks merge until Copilot reviews PR

All three must move into CI checks to maintain these guarantees after ruleset removal.

---

Current CI Coverage (squad-ci.yml)

(ok) What's Already Running on PRs

Check	Purpose	Trigger	Pass/Fail
Lint docs markdown	Validates markdown formatting	Every PR	Blocks if violations
Spell check docs	Checks spelling in docs & README	Every PR	Blocks if typos
Tests	Unit tests via vitest	Every PR	Blocks if tests fail
Source tree canary	Detects accidental file deletion	Every PR	Blocks if >1 critical file missing
Large deletion guard	Warns on >50 file deletions	Every PR only	Blocks unless large-deletion-approved label
Publish policy	Ensures npm publish commands are workspace-scoped	Every PR	Blocks if policy violated
TypeScript type check (implicit in build)	Validates TypeScript compilation	Every PR	Blocks if types invalid
Build	Compiles SDK & CLI	Every PR	Blocks if build fails

⚠️ Gaps: What's Missing to Replace Branch Protection

---

Recommended NEW Checks to Implement

(!) CRITICAL (Blocking) -- Must Implement Before Ruleset Removal

1. Copilot Code Review as CI Check

• What it checks: Automatically triggers GitHub Copilot code review on PR creation/push
• Why it matters: The ruleset currently requires copilot_code_review before merge. Without this check, no automated review happens.
• Effort: MEDIUM -- Need to integrate with GitHub Copilot Code Review API or use a GitHub App that coordinates with Copilot
• Pass/fail: Advisory (review posted, not blocking) OR blocking (if a Copilot Code Review action exists)
• Implementation notes:
  • Check if bradygaster/squad has access to GitHub Copilot Code Review beta features
  • Can use actions/copilot-code-review or similar (if available) OR manually trigger via gh
  • Or: Create a custom action that calls the Copilot API endpoint to request a review
  • Current status: Not in any workflow

2. Merge/Push Protection Check (Non-Fast-Forward Prevention)

• What it checks: Detects attempted force pushes and blocks them
• Why it matters: The ruleset prevents non-fast-forward merges (force pushes). Without this, accidental history rewriting is possible.
• Effort: LOW -- Add a pre-commit hook check via CI or use GitHub branch protection via API
• Pass/fail: Blocking
• Implementation notes:
  • GitHub Actions can't directly prevent force pushes (that's a branch setting)
  • BETTER APPROACH: Use gh api in CI to verify branch protection settings are enforced OR add a check to prevent commits that rewrite history
  • OR: This might already be handled by GitHub's branch protection at the API level after ruleset; verify with Brady
  • Current status: Not enforced in CI

3. Deletion Protection Check

• What it checks: Ensures critical source files are not deleted in PR
• Why it matters: The ruleset prevents branch deletion. This protects against accidental removal of critical files.
• Effort: LOW -- Already partially implemented! (source-tree-canary-check exists)
• Pass/fail: Blocking
• Enhancements needed:
  • Expand canary check to include all critical files (not just 4 hardcoded ones)
  • Add a check for .squad/ and .ai-team/ directories (already checked on preview branch in squad-preview.yml)
  • Verify threshold for large deletion guard (currently 50 files)
• Current status: squad-ci.yml has basic version; squad-preview.yml has more checks

---

🟡 MEDIUM PRIORITY (Recommended Blocking Checks)

4. Test Coverage Threshold Enforcement

• What it checks: Verifies test coverage meets a minimum threshold (e.g., >80%)
• Why it matters: Ensures code quality and reduces regression risk
• Effort: MEDIUM -- Vitest already has coverage support; need to add enforcement
• Pass/fail: Blocking
• Implementation notes:
  • vitest already installed with @vitest/coverage-v8
  • Add --coverage and --coverage.lines 80 flags to npm test
  • Parse coverage report and fail if below threshold
  • Consider using codecov or similar for PR comments
• Current status: Coverage runs but no threshold enforcement

5. Commit History Quality Check

• What it checks:
  • Warns if PR has >1 commit (merge commits should be clean)
  • Suggests squash-and-rebase workflow
  • Detects incomplete commit messages
• Why it matters: Keeps git history clean and readable
• Effort: LOW
• Pass/fail: Advisory (comment on PR) or warning
• Current status: Not implemented

6. ESLint Type Enforcement

• What it checks: Runs ESLint on all JS/TS files in PR
• Why it matters: Catches code style, logical errors, and type issues early
• Effort: LOW -- ESLint already installed
• Pass/fail: Blocking
• Implementation notes:
  • npm run lint:eslint is defined but not run in squad-ci.yml
  • Add step to run this on every PR
• Current status: Defined but NOT running in CI

7. License Compliance Check

• What it checks: Scans dependencies for incompatible or unknown licenses
• Why it matters: Prevents legal/compliance issues
• Effort: MEDIUM -- Need to add license scanning tool (e.g., FOSSA, Black Duck, or license-report)
• Pass/fail: Blocking (for high-risk licenses) or advisory (for warnings)
• Current status: Not implemented

8. Dependency Audit (npm audit)

• What it checks: Scans for known security vulnerabilities in dependencies
• Why it matters: Catches security issues before merge
• Effort: LOW -- Native npm command
• Pass/fail: Blocking (for high/critical vulnerabilities) or advisory (for low/moderate)
• Current status: Not implemented

---

🟢 LOW PRIORITY (Nice to Have, Advisory)

9. Docs Build Verification

• What it checks: Ensures docs site builds without errors
• Why it matters: Catches doc generation failures early
• Effort: LOW -- npm run docs:build already defined
• Pass/fail: Advisory (run but don't block) or blocking for certain branches
• Current status: Runs on push to main only; not on PR

10. External Links Validation

• What it checks: Validates all external links in docs are reachable
• Why it matters: Keeps documentation useful and up-to-date
• Effort: LOW -- squad-docs-links.yml already exists
• Pass/fail: Advisory (runs weekly, creates issue on failures)
• Current status: Runs on schedule weekly; consider running on PRs that touch docs

11. Squad-Specific Checks (If Applicable)

• Skills discovery test: Validate all skills are properly registered
• NAP/Compact tests: Ensure agent spawn behavior is correct
• Heartbeat test: Validate triage rules work as expected
• Why it matters: Ensures framework features work correctly
• Effort: MEDIUM-HIGH -- Depends on test infrastructure
• Pass/fail: Blocking (if critical) or advisory
• Current status: Tests exist but scattered; not centralized in CI

12. File Encoding Validation

• What it checks: Ensures all text files use UTF-8 encoding (no accidental binary commits)
• Why it matters: Prevents encoding issues and accidental large binary files
• Effort: LOW
• Pass/fail: Advisory or blocking
• Current status: Not implemented

13. Automated CHANGELOG Check

• What it checks: Verifies CHANGELOG.md is updated for version-bumping PRs
• Why it matters: Keeps release notes up-to-date
• Effort: LOW -- Already validated in squad-preview.yml and squad-release.yml
• Pass/fail: Blocking (for release PRs) or advisory
• Current status: Check exists but only on preview/release branches

---

Summary Table: CI Check Implementation Roadmap

Check	Category	Status	Blocking?	Effort	Priority
Copilot Review	Code Review	(!) Missing	YES	MEDIUM	P0
Non-FF Protection	Safety	(!) Missing	YES	LOW	P0
Deletion Protection (Enhanced)	Safety	🟡 Partial	YES	LOW	P0
ESLint Enforcement	Code Quality	(!) Missing	YES	LOW	P1
Test Coverage Threshold	Testing	(!) Missing	YES	MEDIUM	P1
npm audit Security Scan	Security	(!) Missing	YES	LOW	P1
License Compliance	Compliance	(!) Missing	YES	MEDIUM	P1
Commit Quality Check	PR Hygiene	(!) Missing	NO	LOW	P2
Docs Build Verification	Docs	🟡 Partial	NO	LOW	P2
External Links Check	Docs	🟡 Partial	NO	LOW	P2
CHANGELOG Update Check	Release	🟡 Partial	YES	LOW	P2
Encoding Validation	Safety	(!) Missing	NO	LOW	P3
Squad Skills Tests	Framework	🟡 Partial	YES	MEDIUM	P2

---

Implementation Sequence (Recommended)

Phase 1 (Critical -- Do First):

1. (ok) Enhance deletion protection with expanded file list
2. (ok) Add ESLint to CI (1-line PR change)
3. (ok) Integrate Copilot Code Review trigger
4. (ok) Document non-FF protection approach with Brady

Phase 2 (High Value -- Week 1):
5. Add npm audit + license check
6. Add test coverage threshold enforcement
7. Enhance commit quality check

Phase 3 (Nice to Have -- Ongoing):
8. Add encoding validation
9. Improve docs checks on PR
10. Centralize squad-specific tests

---

Questions for Brady

1. Copilot Code Review: Is there a GitHub API endpoint or action available in the bradygaster/squad org for triggering Copilot code reviews programmatically?
2. Non-FF Protection: Should this be enforced at the branch protection API level or via CI check? (CI cannot prevent force pushes, only detect them after the fact)
3. Coverage Threshold: What's an acceptable minimum? (Recommend 80% for new code)
4. Blocking vs. Advisory: Which checks should block merge vs. just comment?

[diberry]

CI Feature Flags -- Configurable Checks

Since branch protection is being removed, all new CI checks should be toggleable so Brady can control what runs without editing workflow files.

Recommended approach: Repo Variables + PR Labels

Global toggles (repo variables):
Brady sets these in Settings > Secrets and variables > Actions > Variables tab.

Variable	Default	Effect
SQUAD_ESLINT_ENABLED	true	Run ESLint on PRs
SQUAD_AUDIT_ENABLED	true	Run npm audit on PRs
SQUAD_COVERAGE_THRESHOLD	0	Minimum test coverage % (0 = no enforcement)
SQUAD_DELETION_GUARD	true	Block PRs deleting critical files
SQUAD_SPELLCHECK_ENABLED	false	Run cspell on changed files

Workflow reads them with:

jobs:
  eslint:
    if: vars.SQUAD_ESLINT_ENABLED != 'false'

Per-PR overrides (labels):
Any PR can have these labels added to skip specific checks:

Label	Skips
skip-lint	ESLint
skip-audit	npm audit
skip-tests	Test suite
skip-docs	Docs build

Workflow checks:

if: "!contains(github.event.pull_request.labels.*.name, 'skip-lint')"

Why this matters

• Brady and Tamir can merge without branch protection blocking them
• New checks can be added "off by default" and turned on when ready
• Per-PR labels let contributors skip checks for legitimate reasons (visible in PR history)
• No workflow file changes needed to toggle -- just repo settings or labels

Implementation

Each new CI check we add should follow this pattern:

1. Add repo variable for global toggle (default: enabled for blocking, disabled for advisory)
2. Add PR label for per-PR override
3. Document the variable/label in the workflow file comments
4. New checks start as advisory (warn, don't fail) until proven stable

[diberry]

Resolved — branch protection research completed and posted to bradygaster#640. Recommendation: Rulesets + CODEOWNERS.