diff --git a/.copilot/skills/pr-lifecycle/SKILL.md b/.copilot/skills/pr-lifecycle/SKILL.md new file mode 100644 index 000000000..6716ee62d --- /dev/null +++ b/.copilot/skills/pr-lifecycle/SKILL.md @@ -0,0 +1,537 @@ +--- +name: "pr-lifecycle" +description: "Complete issue → PR → merge lifecycle with readiness checks" +domain: "workflow" +confidence: "high" +source: "extracted from pr-readiness.mjs, CONTRIBUTING.md, PR_REQUIREMENTS.md, squad-ci.yml" +--- + +## Context + +This skill is the **canonical Copilot-agent lifecycle for the Squad repository**. It covers the full path from picking up a GitHub issue to merging a PR. Where older docs (templates, copilot-instructions, CONTRIBUTING.md) conflict with this skill, **this skill takes precedence** for Copilot agents. + +For advanced scenarios (worktrees, multi-repo coordination), see `.copilot/skills/git-workflow/SKILL.md`. + +## Scope + +✅ **THIS SKILL COVERS:** +- Issue pickup and branch creation +- Implementation, build, test, lint workflow +- Pre-push safety verification +- PR creation with correct target, title, body, and labels +- All 11 PR readiness checks (what they check, how to pass, how to fix) +- Post-creation maintenance (review feedback, rebasing, CI) +- Merge preconditions and cleanup + +❌ **THIS SKILL DOES NOT COVER:** +- Worktree-based parallel work (see `git-workflow` skill) +- Multi-repo coordinated PRs (see `git-workflow` skill) +- Release process / publishing (see `.squad/skills/release-process`) +- Reviewer lockout protocol (see `.copilot/skills/reviewer-protocol`) +- Architectural or security review checklists + +--- + +## Lifecycle Procedure + +### Phase 1 — Issue Pickup + +1. **Read the issue.** Understand the acceptance criteria before writing code. + +2. **Confirm capability fit.** Check your capability profile in `.squad/team.md`. 🟢 = proceed. 🟡 = proceed but flag in PR. 🔴 = comment on issue and stop. + +3. **Branch from dev:** + ```bash + git fetch origin dev + git checkout dev + git rebase origin/dev + git checkout -b squad/{issue-number}-{slug} + ``` + - Branch name format: `squad/{issue-number}-{kebab-case-slug}` + - Example: `squad/42-fix-login-validation` + - **Never** branch from `main` + +4. **Mark in-progress** (optional): + ```bash + gh issue edit {number} --add-label "status:in-progress" + ``` + +--- + +### Phase 2 — Implementation + +1. **Make your changes.** Follow the codebase conventions: + - TypeScript strict mode, no `@ts-ignore` + - ESM-only, async/await + - JSDoc on new public APIs + +2. **Build:** + ```bash + npm run build + ``` + +3. **Test:** + ```bash + npm test + ``` + +4. **Type check:** + ```bash + npm run lint + ``` + +5. **Stage specific files only:** + ```bash + git add path/to/file1.ts path/to/file2.ts + ``` + - ❌ **NEVER** `git add .`, `git add -A`, or `git commit -a` + - ✅ **ALWAYS** name each file explicitly + +6. **Pre-push safety check:** + ```bash + # Verify file count matches intent (expect ≤10 files for most fixes) + git diff --cached --stat + + # Verify NO unintended deletions + git diff --cached --diff-filter=D --name-only + ``` + If you see unexpected files or deletions, unstage them: `git reset HEAD ` + +7. **Single commit with issue reference:** + ```bash + git commit -m "Brief description of change + + Closes #{issue-number} + + Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>" + ``` + - If you already have multiple commits, squash: `git rebase -i origin/dev` and squash all into one + - **Never** use `git reset --soft` to squash — it picks up delta from dev and contaminates the commit + +8. **Add changeset (if required):** + A changeset is required when your PR modifies files under `packages/squad-sdk/src/` or `packages/squad-cli/src/`. + + ```bash + npx changeset add + ``` + This prompts for: which packages changed, bump type (patch/minor/major), summary. + + Or create manually at `.changeset/{descriptive-name}.md`: + ```markdown + --- + '@bradygaster/squad-cli': patch + --- + + Brief description of the change + ``` + + If the changeset creates a second commit, squash it into your main commit. + +--- + +### Phase 3 — PR Creation + +1. **Push:** + ```bash + git push -u origin squad/{issue-number}-{slug} + ``` + +2. **Create PR targeting dev:** + ```bash + gh pr create --repo bradygaster/squad --base dev \ + --title "fix: brief description (#issue-number)" \ + --body "Closes #{issue-number} + + ## Summary + What this PR does and why. + + ## Changes + - File-level description of changes + + Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>" + ``` + +3. **Title conventions:** + - Bug fix: `fix: description (#N)` + - Feature: `feat: description (#N)` + - Docs: `docs: description (#N)` + - Chore/infra: `chore: description (#N)` + +4. **Labels:** + - `fix`, `feat`, `docs`, or `repo-health` for type + - `squad:{agent-name}` if working as a squad member + - `skip-changelog` only with reviewer approval (escape hatch) + +5. **Scope rules by label:** + - `repo-health`: Only modify `.github/`, `scripts/`, root config, tests, docs. **Never** modify `packages/*/src/` + - `fix` or `feat`: May modify product source. Must include changeset when touching `packages/*/src/` + +--- + +### Phase 4 — PR Readiness + +An automated readiness check runs on every push and posts a checklist comment on the PR. All 11 checks must pass before review (check 11 is informational-only). + +#### Check 1: Single Commit + +| | | +|---|---| +| **What** | PR must contain exactly 1 commit | +| **Pass** | Push a single, squashed commit | +| **Fix** | `git rebase -i origin/dev` → squash all commits into one → `git push --force-with-lease` | +| **Gotcha** | Never `git reset --soft` to squash — contaminates the commit with unrelated changes | + +#### Check 2: Not in Draft + +| | | +|---|---| +| **What** | PR must not be marked as draft | +| **Pass** | Create PR as ready, or convert: `gh pr ready` | +| **Fix** | `gh pr ready {number}` or use the GitHub UI | +| **Gotcha** | The readiness check still runs on drafts but will show ❌ until you mark ready | + +#### Check 3: Branch Up to Date + +| | | +|---|---| +| **What** | PR branch must not be behind `dev` | +| **Pass** | Rebase onto latest dev before pushing | +| **Fix** | `git fetch origin dev && git rebase origin/dev && git push --force-with-lease` | +| **Gotcha** | After rebasing, the readiness check re-runs automatically on the new push | + +#### Check 4: Copilot Review + +| | | +|---|---| +| **What** | The `copilot-pull-request-reviewer` bot must post an `APPROVED` review | +| **Pass** | Wait — Copilot review is triggered automatically on PR creation/push | +| **Fix** | If Copilot hasn't reviewed after 5 minutes, push an empty commit to re-trigger: `git commit --allow-empty -m "trigger review" && git push` then squash before merge | +| **Gotcha** | Copilot review state is `APPROVED`, `CHANGES_REQUESTED`, or `COMMENTED`. Only `APPROVED` passes | + +#### Check 5: Changeset Present + +| | | +|---|---| +| **What** | PRs that modify `packages/squad-sdk/src/` or `packages/squad-cli/src/` must include a `.changeset/*.md` file or a `CHANGELOG.md` edit | +| **Pass** | Run `npx changeset add` and commit the generated file | +| **Fix** | `npx changeset add` → select affected package(s) → select bump type → write summary → `git add .changeset/ && git commit --amend --no-edit && git push --force-with-lease` | +| **Gotcha** | The `skip-changelog` label bypasses this check but requires reviewer approval. Non-source changes (docs, config, tests) don't need a changeset | + +#### Check 6: No Merge Conflicts + +| | | +|---|---| +| **What** | PR must be cleanly mergeable with the base branch | +| **Pass** | Keep branch rebased on dev | +| **Fix** | `git fetch origin dev && git rebase origin/dev` → resolve conflicts → `git push --force-with-lease` | +| **Gotcha** | GitHub may show `null` mergeability briefly while computing — the check treats this as passing | + +#### Check 7: Scope Clean + +| | | +|---|---| +| **What** | Warns if PR includes `.squad/` or `docs/proposals/` files | +| **Pass** | Don't include team state or proposal files in product PRs | +| **Fix** | Remove unintended files: `git reset HEAD .squad/ docs/proposals/` then amend your commit | +| **Gotcha** | This check is **informational only** — it always passes but flags attention. Including these files is OK if intentional (e.g., updating agent history) | + +#### Check 8: Copilot Threads Resolved + +| | | +|---|---| +| **What** | All review threads opened by `copilot-pull-request-reviewer` must be resolved | +| **Pass** | Address each Copilot comment, then click "Resolve conversation" in the GitHub UI | +| **Fix** | Go to the PR's "Files changed" tab → find unresolved Copilot threads → fix the code or explain why no change is needed → click "Resolve conversation" | +| **Gotcha** | Outdated threads (on code that's since changed) are automatically skipped. Only active, unresolved threads block | + +#### Check 9: CI Passing + +| | | +|---|---| +| **What** | All CI check runs (excluding the readiness check itself) must be green | +| **Pass** | Fix any build, test, or lint failures and push | +| **Fix** | Read the failing check's logs (`gh run view {run-id} --log-failed`), fix the issue, push | +| **Gotcha** | Pending/in-progress checks also show ❌. Wait for all checks to complete before evaluating. The readiness check re-runs after CI completes via `workflow_run` trigger | + +#### Check 10: Issue Linked + +| | | +|---|---| +| **What** | PR body or commit message must reference an issue (`Closes #N`, `Fixes #N`, `Resolves #N`, or `Part of #N`) | +| **Pass** | Include `Closes #N` in PR body or commit message | +| **Fix** | Edit PR body to add `Closes #{issue-number}`, or amend commit message | +| **Gotcha** | Case-insensitive matching. Only closing keywords are recognized | + +#### Check 11: Protected Files (informational) + +| | | +|---|---| +| **What** | Warns when zero-dependency bootstrap files are modified (always passes) | +| **Pass** | Always passes — this is informational only | +| **Fix** | If flagged, verify the changed bootstrap file still has zero external dependencies | +| **Gotcha** | Protected files are listed in `copilot-instructions.md`. The check warns but does not block | + +--- + +### Phase 5 — Post-Creation Maintenance + +1. **Handling Copilot review feedback:** + - Read each comment carefully + - Fix valid issues in your code + - Resolve each thread after addressing it + - Amend your single commit: `git commit --amend --no-edit && git push --force-with-lease` + +2. **Rebasing when behind dev:** + ```bash + git fetch origin dev + git rebase origin/dev + # Resolve any conflicts + git push --force-with-lease + ``` + +3. **Re-running CI:** + - CI re-runs automatically on every push + - To re-run without changes: `gh run rerun {run-id} --failed` + +4. **If readiness check is stale:** + - The readiness check re-runs on push and after CI completes + - If it's stale, push a trivial fix or empty commit to re-trigger + +--- + +### Phase 6 — Merge & Cleanup + +1. **Merge preconditions:** + - All 11 readiness checks pass (✅ across the board) + - Human reviewer has approved (or maintainer merges directly) + - No open blocking conversations + +2. **Who merges:** + - Agents do **not** merge PRs themselves + - Maintainers (bradygaster or designated reviewers) merge via GitHub UI + - The repo uses squash merge, so commit history is clean regardless + +3. **Post-merge cleanup:** + ```bash + git checkout dev + git pull origin dev + git branch -d squad/{issue-number}-{slug} + git push origin --delete squad/{issue-number}-{slug} + ``` + +4. **Verify issue auto-close:** + - If PR body contains `Closes #{N}`, the issue closes automatically on merge + - If not, manually close: `gh issue close {number}` + +--- + +## Examples + +### Example: Bug fix PR (no source changes) + +```bash +# Branch +git fetch origin dev && git checkout dev && git rebase origin/dev +git checkout -b squad/610-fix-broken-link + +# Fix +# ... edit docs/some-file.md ... + +# Validate +npm run build && npm test + +# Commit (no changeset needed — docs only) +git add docs/some-file.md +git diff --cached --stat # verify: 1 file +git commit -m "docs: fix broken link in contributing guide + +Closes #610 + +Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>" + +# Push and PR +git push -u origin squad/610-fix-broken-link +gh pr create --repo bradygaster/squad --base dev \ + --title "docs: fix broken link (#610)" \ + --body "Closes #610" +``` + +### Example: SDK feature PR (source changes) + +```bash +# Branch +git fetch origin dev && git checkout dev && git rebase origin/dev +git checkout -b squad/42-add-profile-api + +# Implement +# ... edit packages/squad-sdk/src/profile/index.ts ... +# ... edit packages/squad-sdk/src/index.ts (re-export) ... + +# Validate +npm run build && npm test && npm run lint + +# Changeset (required — touches packages/squad-sdk/src/) +npx changeset add +# Select: @bradygaster/squad-sdk, minor, "Add profile API" + +# Stage and commit +git add packages/squad-sdk/src/profile/index.ts packages/squad-sdk/src/index.ts .changeset/ +git diff --cached --stat # verify file count +git diff --cached --diff-filter=D --name-only # verify no deletions +git commit -m "feat: add profile API + +Closes #42 + +Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>" + +# Push and PR +git push -u origin squad/42-add-profile-api +gh pr create --repo bradygaster/squad --base dev \ + --title "feat: add profile API (#42)" \ + --body "Closes #42 + +## Summary +Adds profile resolution API to the SDK. + +## Changes +- New module: packages/squad-sdk/src/profile/ +- Re-exported from barrel file" +``` + +--- + +## Anti-Patterns + +- ❌ Branching from `main` (always branch from `dev`) +- ❌ Targeting `main` with PRs (always target `dev`) +- ❌ Using `git add .` or `git add -A` (stage specific files only) +- ❌ Using `git commit -a` (same risk as broad staging) +- ❌ Using `git reset --soft` to squash (contaminates commit with dev delta) +- ❌ Force-pushing to `dev` or `main` (only force-push your own feature branch) +- ❌ Merging your own PR (maintainers merge) +- ❌ Skipping the changeset when source files changed (CI will fail) +- ❌ Self-resolving Copilot threads without addressing the feedback +- ❌ Pushing >1 commit without squashing (readiness check will flag it) +- ❌ Including `.squad/` files in product PRs without intention (scope check warns) +- ❌ Mixing product and infrastructure changes in one PR (create separate PRs) + +--- + +## Readiness Check Gaps & Recommendations + +After analyzing `scripts/pr-readiness.mjs`, `.github/workflows/squad-ci.yml`, and `.github/workflows/squad-repo-health.yml`, three gaps were identified. Gaps 1 and 3 are now implemented (checks 10 and 11). Gap 2 is deferred. + +### Gap 1: Issue Linkage Check (Check 10) — IMPLEMENTED + +**Problem:** Nothing verifies that the PR body or commit message references an issue (`Closes #N` or `Part of #N`). Orphan PRs are hard to trace. + +**Recommendation:** Add `checkIssueLinkage(prBody, commitMessages)` to `pr-readiness.mjs`. + +```javascript +/** + * Check: Issue linkage. + * @param {string} prBody — PR description text + * @param {Array<{ commit: { message: string } }>} commits + * @returns {{ pass: boolean, detail: string }} + */ +export function checkIssueLinkage(prBody, commits) { + const issuePattern = /(closes|fixes|resolves|part of)\s+#\d+/i; + const bodyHasRef = issuePattern.test(prBody || ''); + const commitHasRef = (commits || []).some( + (c) => issuePattern.test(c.commit?.message || '') + ); + if (bodyHasRef || commitHasRef) { + return { pass: true, detail: 'Issue reference found' }; + } + return { + pass: false, + detail: 'No issue reference — add `Closes #N` to PR body or commit message', + }; +} +``` + +**Integration point:** After check 2 (draft status), before check 3 (branch freshness). Data is already available from the commits and PR body fetched in the orchestrator. + +### Gap 2: No Required Checks Presence Verification + +**Problem:** The CI status check (check 9) verifies that existing checks are green, but doesn't verify that the *expected* set of checks actually ran. If a workflow is misconfigured or skipped, the PR could pass with zero CI checks. + +**Recommendation:** Add `checkRequiredChecksPresent(checkRuns, files)` to `pr-readiness.mjs`. + +```javascript +/** Minimum required check names that must appear for source PRs. */ +export const REQUIRED_CHECKS = ['Squad CI / test']; + +/** + * Check: Required CI checks are present. + * @param {Array<{ name: string }>} checkRuns + * @param {Array<{ filename: string }>} files + * @returns {{ pass: boolean, detail: string }} + */ +export function checkRequiredChecksPresent(checkRuns, files) { + const touchesSource = (files || []).some( + (f) => SOURCE_PATTERN.test(f.filename) + ); + if (!touchesSource) { + return { pass: true, detail: 'No source changes — required checks not enforced' }; + } + const checkNames = new Set((checkRuns || []).map((cr) => cr.name)); + const missing = REQUIRED_CHECKS.filter((name) => !checkNames.has(name)); + if (missing.length > 0) { + return { + pass: false, + detail: `Required check(s) not found: ${missing.join(', ')}`, + }; + } + return { pass: true, detail: 'All required checks present' }; +} +``` + +**Integration point:** After check 9 (CI status). Uses the same `checkRuns` and `files` data already fetched. + +### Gap 3: Protected File Change Detection (Check 11) — IMPLEMENTED + +**Problem:** The repo has zero-dependency bootstrap files that must never import external packages (documented in `copilot-instructions.md`). The `squad-repo-health.yml` workflow runs a bootstrap protection check, but the PR readiness comment doesn't surface it — contributors don't see the warning until they check the separate workflow. + +**Recommendation:** Add `checkProtectedFiles(files)` to `pr-readiness.mjs` as an **informational** check (always passes, like scope clean). + +```javascript +/** Bootstrap files that must remain zero-dependency. */ +export const PROTECTED_FILES = [ + 'packages/squad-cli/src/cli/core/detect-squad-dir.ts', + 'packages/squad-cli/src/cli/core/errors.ts', + 'packages/squad-cli/src/cli/core/gh-cli.ts', + 'packages/squad-cli/src/cli/core/output.ts', + 'packages/squad-cli/src/cli/core/history-split.ts', +]; + +/** + * Check: Protected file changes (informational). + * @param {Array<{ filename: string }>} files + * @returns {{ pass: boolean, detail: string }} + */ +export function checkProtectedFiles(files) { + const touched = (files || []).filter( + (f) => PROTECTED_FILES.includes(f.filename) + ); + if (touched.length === 0) { + return { pass: true, detail: 'No protected bootstrap files changed' }; + } + return { + pass: true, + detail: `⚠️ ${touched.length} protected bootstrap file(s) changed: ${touched.map((f) => f.filename.split('/').pop()).join(', ')} — verify zero-dependency constraint`, + }; +} +``` + +**Integration point:** After check 7 (scope clean). Uses the same `files` data. Informational only — warns but doesn't block. + +### Summary of Recommended Changes + +| Check | Type | Blocks PR? | Status | +|-------|------|------------|--------| +| Issue linkage (Check 10) | Hard gate | Yes | Implemented | +| Required checks present | Hard gate | Yes | Deferred | +| Protected file warning (Check 11) | Informational | No | Implemented | + +All three use data that `pr-readiness.mjs` already fetches — no new API calls needed. Total addition: ~60 lines of check functions + ~10 lines of orchestration wiring. diff --git a/.github/workflows/squad-ci.yml b/.github/workflows/squad-ci.yml index a7e7b9d0b..b2720ef74 100644 --- a/.github/workflows/squad-ci.yml +++ b/.github/workflows/squad-ci.yml @@ -24,6 +24,7 @@ jobs: timeout-minutes: 3 outputs: docs: ${{ steps.filter.outputs.docs }} + code: ${{ steps.filter.outputs.code }} steps: - uses: actions/checkout@v4 with: @@ -44,6 +45,12 @@ jobs: else echo "docs=false" >> "$GITHUB_OUTPUT" fi + # Detect non-docs changes (code, config, workflows, etc.) + if echo "$CHANGED" | grep -qvE '^(docs/|README\.md|\.markdownlint|\.cspell|cspell\.json)'; then + echo "code=true" >> "$GITHUB_OUTPUT" + else + echo "code=false" >> "$GITHUB_OUTPUT" + fi docs-quality: needs: changes @@ -80,6 +87,13 @@ jobs: run: npx cspell --no-progress --dot "docs/src/content/**/*.md" "README.md" test: + needs: changes + # Fail-open: run test if changes job failed (don't let path filter break testing) + if: >- + always() && !cancelled() + && (github.event_name == 'push' + || needs.changes.result != 'success' + || needs.changes.outputs.code == 'true') runs-on: ubuntu-latest timeout-minutes: 15 steps: @@ -205,15 +219,20 @@ jobs: # to the label sync config to auto-create them in new repos. # ════════════════════════════════════════════════════════════════════════ - changelog-gate: - # ── Local testing ────────────────────────────────────────────────────── - # To test this gate locally: - # 1. Identify the merge-base: git merge-base dev HEAD - # 2. Check for SDK/CLI source changes: - # git diff --name-only ...HEAD | grep -E '^packages/squad-(sdk|cli)/src/' - # 3. If any match, verify an Added/Modified .changeset/*.md (excluding README) OR CHANGELOG.md: - # ────────────────────────────────────────────────────────────────────── - if: github.event_name == 'pull_request' + # ═══════════════════════════════════════════════════════════════════════ + # Consolidated policy gates — runs 4 lightweight checks on a single + # runner. Saves ~3 runner boots per PR push vs separate jobs. + # Individual gates: changelog-gate, changelog-protection, + # workspace-integrity, prerelease-version-guard. + # ═══════════════════════════════════════════════════════════════════════ + policy-gates: + name: Policy Gates + needs: changes + if: >- + github.event_name == 'pull_request' + && !cancelled() + && (needs.changes.outputs.code == 'true' + || needs.changes.result == 'failure') runs-on: ubuntu-latest timeout-minutes: 5 steps: @@ -221,115 +240,174 @@ jobs: with: fetch-depth: 0 - - name: Check feature flag - id: flag - # Default: gate is ENABLED. When vars.SQUAD_CHANGELOG_CHECK is - # undefined (not set in repo/org variables), the bash comparison - # [ "" = "false" ] evaluates to false, so skip stays "false" and - # the gate runs. Set vars.SQUAD_CHANGELOG_CHECK to "false" to - # explicitly disable. - run: | - if [ "${{ vars.SQUAD_CHANGELOG_CHECK }}" = "false" ]; then - echo "skip=true" >> "$GITHUB_OUTPUT" - echo "CHANGELOG gate disabled via vars.SQUAD_CHANGELOG_CHECK" - else - echo "skip=false" >> "$GITHUB_OUTPUT" - fi + - uses: actions/setup-node@v4 + with: + node-version: 22 - - name: Check skip label - if: steps.flag.outputs.skip == 'false' - id: label + - name: Fetch PR labels + id: labels run: | LABELS=$(gh pr view ${{ github.event.pull_request.number }} --json labels --jq '.labels[].name' 2>/dev/null || echo "") - if echo "$LABELS" | grep -q "skip-changelog"; then - echo "skip=true" >> "$GITHUB_OUTPUT" - echo "Skipping CHANGELOG gate (skip-changelog label present)" - else - echo "skip=false" >> "$GITHUB_OUTPUT" - fi + echo "all<> "$GITHUB_OUTPUT" + echo "$LABELS" >> "$GITHUB_OUTPUT" + echo "EOF" >> "$GITHUB_OUTPUT" + # Per-gate skip flags + echo "$LABELS" | grep -q "skip-changelog" && echo "skip_changelog=true" >> "$GITHUB_OUTPUT" || echo "skip_changelog=false" >> "$GITHUB_OUTPUT" + echo "$LABELS" | grep -q "skip-workspace-check" && echo "skip_workspace=true" >> "$GITHUB_OUTPUT" || echo "skip_workspace=false" >> "$GITHUB_OUTPUT" + echo "$LABELS" | grep -q "skip-version-check" && echo "skip_version=true" >> "$GITHUB_OUTPUT" || echo "skip_version=false" >> "$GITHUB_OUTPUT" env: GH_TOKEN: ${{ github.token }} - - name: Require CHANGELOG update for SDK/CLI source changes - if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' + # ─── Changelog Gate ─────────────────────────────────────────────── + - name: "Gate: Changelog" + if: >- + always() + && steps.labels.outputs.skip_changelog != 'true' + && vars.SQUAD_CHANGELOG_CHECK != 'false' run: | + echo "## 📋 Changelog Gate" >> $GITHUB_STEP_SUMMARY BASE="${{ github.event.pull_request.base.sha }}" HEAD="${{ github.event.pull_request.head.sha }}" - # Three-dot diff (base...head) finds the merge-base automatically, - # so it works correctly even when the PR branch contains merge - # commits from syncing with the base branch. It compares against - # the common ancestor, not the literal base SHA. CHANGED=$(git diff --name-only "$BASE"..."$HEAD") - - # Change detection regex: ^packages/squad-(sdk|cli)/src/ - # Matches any file under packages/squad-sdk/src/ or packages/squad-cli/src/ - # This intentionally excludes config files, tests, and docs — only source changes - # require a CHANGELOG entry. SDK_CLI_CHANGED=$(echo "$CHANGED" | grep -E '^packages/squad-(sdk|cli)/src/' || true) if [ -z "$SDK_CLI_CHANGED" ]; then echo "No SDK/CLI source changes detected -- CHANGELOG gate not applicable" + echo "✅ Not applicable (no SDK/CLI source changes)" >> $GITHUB_STEP_SUMMARY exit 0 fi - echo "SDK/CLI source files changed:" echo "$SDK_CLI_CHANGED" - - # Accept EITHER a .changeset/*.md file OR a direct CHANGELOG.md edit. - # Changesets are the preferred workflow — they prevent merge conflicts - # when multiple PRs are open simultaneously. - # Use --diff-filter=AM to count only Added/Modified files (not deletions), - # and exclude .changeset/README.md which is repo documentation, not a changeset. CHANGESET_ADDED=$(git diff --diff-filter=AM --name-only "$BASE"..."$HEAD" | grep -E '^\.changeset/[^/]+\.md$' | grep -vxF '.changeset/README.md' || true) CHANGELOG_CHANGED=$(echo "$CHANGED" | grep -E '^CHANGELOG\.md$' || true) - if [ -n "$CHANGESET_ADDED" ]; then echo "Changeset file(s) detected -- gate passed" echo "$CHANGESET_ADDED" + echo "✅ Changeset file detected" >> $GITHUB_STEP_SUMMARY exit 0 fi - if [ -n "$CHANGELOG_CHANGED" ]; then echo "CHANGELOG.md updated -- gate passed" + echo "✅ CHANGELOG.md updated" >> $GITHUB_STEP_SUMMARY exit 0 fi - echo "" echo "::error::No changeset or CHANGELOG.md update found, but SDK/CLI source files were changed." echo "::error::Preferred: run 'npx changeset add' to create a .changeset/*.md file." echo "::error::Alternative: edit CHANGELOG.md directly." echo "::error::Escape hatch: add the 'skip-changelog' label to your PR." + echo "❌ No changeset or CHANGELOG.md update" >> $GITHUB_STEP_SUMMARY exit 1 - changelog-protection: - name: CHANGELOG Write Protection - runs-on: ubuntu-latest - if: github.event_name == 'pull_request' - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Check CHANGELOG.md modifications + # ─── CHANGELOG Write Protection ────────────────────────────────── + - name: "Gate: CHANGELOG Write Protection" + if: always() env: APPROVED_AUTHORS: 'bradygaster github-actions[bot] copilot-swe-agent[bot]' PR_AUTHOR: ${{ github.event.pull_request.user.login }} run: | + echo "## 🔒 CHANGELOG Write Protection" >> $GITHUB_STEP_SUMMARY CHANGED=$(git diff --name-only origin/${{ github.base_ref }}...HEAD | grep '^CHANGELOG.md$' || true) if [ -n "$CHANGED" ]; then if echo "$APPROVED_AUTHORS" | tr ' ' '\n' | grep -qxF "$PR_AUTHOR"; then echo "✅ $PR_AUTHOR is approved to modify CHANGELOG.md" + echo "✅ $PR_AUTHOR is approved" >> $GITHUB_STEP_SUMMARY else echo "❌ $PR_AUTHOR is not approved to modify CHANGELOG.md directly" echo "" echo "CHANGELOG.md is managed by the release process." echo "Use 'npx changeset add' to add a changeset file instead." echo "See CONTRIBUTING.md for details." + echo "❌ $PR_AUTHOR is not approved to modify CHANGELOG.md" >> $GITHUB_STEP_SUMMARY exit 1 fi else echo "✅ CHANGELOG.md not modified" + echo "✅ CHANGELOG.md not modified" >> $GITHUB_STEP_SUMMARY fi + # ─── Workspace Integrity ────────────────────────────────────────── + - name: "Gate: Workspace Integrity" + if: >- + always() + && steps.labels.outputs.skip_workspace != 'true' + && vars.SQUAD_WORKSPACE_CHECK != 'false' + run: | + echo "## 🔗 Workspace Integrity" >> $GITHUB_STEP_SUMMARY + node -e " + const fs = require('fs'); + const lock = JSON.parse(fs.readFileSync('package-lock.json', 'utf8')); + const pkgs = lock.packages || {}; + const problems = []; + for (const [key, val] of Object.entries(pkgs)) { + if (!key.includes('node_modules/@bradygaster/squad-')) continue; + if (val.resolved && val.resolved.startsWith('https://')) { + problems.push({ path: key, resolved: val.resolved }); + } + } + if (problems.length > 0) { + console.error('::error::WORKSPACE INTEGRITY FAILURE — npm resolved registry packages instead of local workspace copies.'); + console.error('::error::This likely means a version mismatch between workspace packages (see PR #640).'); + console.error(''); + problems.forEach(p => { + console.error(' STALE: ' + p.path + (p.resolved ? ' → ' + p.resolved : ' (version: ' + p.version + ', not a workspace link)')); + }); + console.error(''); + console.error('To fix: ensure all workspace package version ranges match local versions,'); + console.error('then run npm install at the repo root to regenerate the lockfile.'); + process.exit(1); + } + console.log('✅ All workspace packages resolve to local file: links'); + " + echo "✅ All workspace packages resolve to local links" >> $GITHUB_STEP_SUMMARY + + # ─── Prerelease Version Guard ───────────────────────────────────── + - name: "Gate: Prerelease Version Guard" + if: >- + always() + && steps.labels.outputs.skip_version != 'true' + && vars.SQUAD_VERSION_CHECK != 'false' + run: | + echo "## 🏷️ Prerelease Version Guard" >> $GITHUB_STEP_SUMMARY + node -e " + const fs = require('fs'); + const path = require('path'); + const pkgDirs = fs.readdirSync('packages', { withFileTypes: true }) + .filter(d => d.isDirectory()) + .map(d => d.name); + const violations = []; + for (const dir of pkgDirs) { + const pkgPath = path.join('packages', dir, 'package.json'); + if (!fs.existsSync(pkgPath)) continue; + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); + if (pkg.version && /-/.test(pkg.version)) { + violations.push({ name: pkg.name, version: pkg.version, path: pkgPath }); + } + } + if (violations.length > 0) { + console.error('::error::PRERELEASE VERSION DETECTED — packages with prerelease versions cannot merge to dev/main.'); + console.error(''); + violations.forEach(v => { + console.error(' ' + v.name + '@' + v.version + ' (' + v.path + ')'); + }); + console.error(''); + console.error('Prerelease suffixes (-build, -alpha, -beta, -rc) must be removed before merging.'); + console.error('To fix: update the version field in each listed package.json to a release version.'); + console.error('To skip: add the \"skip-version-check\" label to your PR.'); + process.exit(1); + } + console.log('✅ All package versions are release versions (no prerelease suffixes)'); + pkgDirs.forEach(dir => { + const pkgPath = path.join('packages', dir, 'package.json'); + if (fs.existsSync(pkgPath)) { + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); + if (pkg.version) console.log(' ' + pkg.name + '@' + pkg.version); + } + }); + " + echo "✅ All versions are release versions" >> $GITHUB_STEP_SUMMARY + exports-map-check: + needs: changes # ── Local testing ────────────────────────────────────────────────────── # To test this gate locally: # 1. Check for SDK source changes: @@ -337,7 +415,11 @@ jobs: # 2. If any match, run the exports map script: # node scripts/check-exports-map.mjs # ────────────────────────────────────────────────────────────────────── - if: github.event_name == 'pull_request' + if: >- + !cancelled() + && (github.event_name != 'pull_request' + || needs.changes.outputs.code == 'true' + || needs.changes.result == 'failure') runs-on: ubuntu-latest timeout-minutes: 5 steps: @@ -405,13 +487,18 @@ jobs: run: node scripts/check-exports-map.mjs samples-build: + needs: changes # ── Local testing ────────────────────────────────────────────────────── # To test this gate locally: # 1. Build the SDK: npm run build -w packages/squad-sdk # 2. Loop over samples: for d in samples/*/; do (cd "$d" && npm install && npm run build); done # 3. Or test a single sample: cd samples/ && npm install && npm run build && npm test # ────────────────────────────────────────────────────────────────────── - if: github.event_name == 'pull_request' + if: >- + !cancelled() + && (github.event_name != 'pull_request' + || needs.changes.outputs.code == 'true' + || needs.changes.result == 'failure') runs-on: ubuntu-latest timeout-minutes: 15 steps: @@ -571,192 +658,11 @@ jobs: fi echo "✅ All npm publish commands are workspace-scoped" - workspace-integrity: - # ────────────────────────────────────────────────────────────────────── - # Workspace Integrity Check - # Purpose: Verify workspace packages resolve to local file: links, - # not stale registry versions in the lockfile. - # Catches: npm silently resolving a published registry copy instead - # of the local workspace symlink due to version mismatches. - # Why: Added after PR #640 prerelease version incident where - # >=0.9.0 didn't match 0.9.1-build.4, so npm pulled the - # stale published SDK from the registry. - # Cost: Zero-install — reads package-lock.json only. - # ────────────────────────────────────────────────────────────────────── - # ── Local testing ────────────────────────────────────────────────────── - # To test this gate locally: - # 1. Inspect lockfile entries for workspace packages: - # node -e "const l=JSON.parse(require('fs').readFileSync('package-lock.json','utf8')); - # Object.entries(l.packages||{}).filter(([k])=>k.includes('@bradygaster/squad-')) - # .forEach(([k,v])=>console.log(k, v.resolved||v.version, v.link?'(link)':''))" - # 2. All entries should show "(link)" — any with https:// URLs are stale. - # ────────────────────────────────────────────────────────────────────── - if: github.event_name == 'pull_request' - runs-on: ubuntu-latest - timeout-minutes: 5 - steps: - - uses: actions/checkout@v4 - - - name: Check feature flag - id: flag - # Default: gate is ENABLED. When vars.SQUAD_WORKSPACE_CHECK is - # undefined (not set in repo/org variables), the bash comparison - # [ "" = "false" ] evaluates to false, so skip stays "false" and - # the gate runs. Set vars.SQUAD_WORKSPACE_CHECK to "false" to - # explicitly disable. - run: | - if [ "${{ vars.SQUAD_WORKSPACE_CHECK }}" = "false" ]; then - echo "skip=true" >> "$GITHUB_OUTPUT" - echo "Workspace integrity check disabled via vars.SQUAD_WORKSPACE_CHECK" - else - echo "skip=false" >> "$GITHUB_OUTPUT" - fi - - - name: Check skip label - if: steps.flag.outputs.skip == 'false' - id: label - run: | - LABELS='${{ toJSON(github.event.pull_request.labels.*.name) }}' - if echo "$LABELS" | grep -q "skip-workspace-check"; then - echo "skip=true" >> "$GITHUB_OUTPUT" - echo "Skipping workspace integrity check (skip-workspace-check label present)" - else - echo "skip=false" >> "$GITHUB_OUTPUT" - fi - - - name: Verify workspace packages resolve locally - if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' - run: | - node -e " - const fs = require('fs'); - const lock = JSON.parse(fs.readFileSync('package-lock.json', 'utf8')); - const pkgs = lock.packages || {}; - const problems = []; - - for (const [key, val] of Object.entries(pkgs)) { - if (!key.includes('node_modules/@bradygaster/squad-')) continue; - if (val.resolved && val.resolved.startsWith('https://')) { - problems.push({ path: key, resolved: val.resolved }); - } - } - - if (problems.length > 0) { - console.error('::error::WORKSPACE INTEGRITY FAILURE — npm resolved registry packages instead of local workspace copies.'); - console.error('::error::This likely means a version mismatch between workspace packages (see PR #640).'); - console.error(''); - problems.forEach(p => { - console.error(' STALE: ' + p.path + (p.resolved ? ' → ' + p.resolved : ' (version: ' + p.version + ', not a workspace link)')); - }); - console.error(''); - console.error('To fix: ensure all workspace package version ranges match local versions,'); - console.error('then run npm install at the repo root to regenerate the lockfile.'); - process.exit(1); - } - - console.log('✅ All workspace packages resolve to local file: links'); - " - - prerelease-version-guard: - # ────────────────────────────────────────────────────────────────────── - # Prerelease Version Guard - # Purpose: Prevent prerelease version strings (-build, -alpha, -beta, - # -rc) from being committed to dev or main. - # Catches: Forgotten prerelease suffixes that break semver range - # resolution in workspace dependencies. - # Why: Added after PR #640 prerelease version incident where a - # -build.N suffix caused npm to skip the local workspace - # copy during dependency resolution. - # Cost: Zero-install — reads package.json files only. - # ────────────────────────────────────────────────────────────────────── - # ── Local testing ────────────────────────────────────────────────────── - # To test this gate locally: - # 1. Scan for prerelease versions: - # node -e "require('fs').readdirSync('packages').forEach(d=>{ - # const p=require('./packages/'+d+'/package.json'); - # if(/-/.test(p.version)) console.log(p.name+'@'+p.version+' ← prerelease!')})" - # 2. A clean run should produce no output. - # ────────────────────────────────────────────────────────────────────── - if: github.event_name == 'pull_request' - runs-on: ubuntu-latest - timeout-minutes: 5 - steps: - - uses: actions/checkout@v4 - - - name: Check feature flag - id: flag - # Default: gate is ENABLED. When vars.SQUAD_VERSION_CHECK is - # undefined (not set in repo/org variables), the bash comparison - # [ "" = "false" ] evaluates to false, so skip stays "false" and - # the gate runs. Set vars.SQUAD_VERSION_CHECK to "false" to - # explicitly disable. - run: | - if [ "${{ vars.SQUAD_VERSION_CHECK }}" = "false" ]; then - echo "skip=true" >> "$GITHUB_OUTPUT" - echo "Prerelease version guard disabled via vars.SQUAD_VERSION_CHECK" - else - echo "skip=false" >> "$GITHUB_OUTPUT" - fi - - - name: Check skip label - if: steps.flag.outputs.skip == 'false' - id: label - run: | - LABELS='${{ toJSON(github.event.pull_request.labels.*.name) }}' - if echo "$LABELS" | grep -q "skip-version-check"; then - echo "skip=true" >> "$GITHUB_OUTPUT" - echo "Skipping prerelease version guard (skip-version-check label present)" - else - echo "skip=false" >> "$GITHUB_OUTPUT" - fi - - - name: Scan packages for prerelease versions - if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' - run: | - node -e " - const fs = require('fs'); - const path = require('path'); - const pkgDirs = fs.readdirSync('packages', { withFileTypes: true }) - .filter(d => d.isDirectory()) - .map(d => d.name); - - const violations = []; - for (const dir of pkgDirs) { - const pkgPath = path.join('packages', dir, 'package.json'); - if (!fs.existsSync(pkgPath)) continue; - const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); - // Version regex: /-/ — matches any hyphen in the version string. - // In semver, a hyphen after the patch number indicates a prerelease - // suffix (e.g. 1.0.0-alpha.1, 0.9.1-build.4). Stable releases - // like 1.0.0 contain no hyphen and pass this check. - if (pkg.version && /-/.test(pkg.version)) { - violations.push({ name: pkg.name, version: pkg.version, path: pkgPath }); - } - } - - if (violations.length > 0) { - console.error('::error::PRERELEASE VERSION DETECTED — packages with prerelease versions cannot merge to dev/main.'); - console.error(''); - violations.forEach(v => { - console.error(' ' + v.name + '@' + v.version + ' (' + v.path + ')'); - }); - console.error(''); - console.error('Prerelease suffixes (-build, -alpha, -beta, -rc) must be removed before merging.'); - console.error('To fix: update the version field in each listed package.json to a release version.'); - console.error('To skip: add the \"skip-version-check\" label to your PR.'); - process.exit(1); - } - - console.log('✅ All package versions are release versions (no prerelease suffixes)'); - pkgDirs.forEach(dir => { - const pkgPath = path.join('packages', dir, 'package.json'); - if (fs.existsSync(pkgPath)) { - const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); - if (pkg.version) console.log(' ' + pkg.name + '@' + pkg.version); - } - }); - " + # workspace-integrity and prerelease-version-guard are now part of + # the consolidated policy-gates job above. export-smoke-test: + needs: changes # ────────────────────────────────────────────────────────────────────── # Export Smoke Test # Purpose: Verify that subpath exports actually resolve after build. @@ -782,7 +688,11 @@ jobs: # 3. To create a test PR to verify this gate: # gh pr edit --add-label skip-export-smoke # ────────────────────────────────────────────────────────────────────── - if: github.event_name == 'pull_request' + if: >- + !cancelled() + && (github.event_name != 'pull_request' + || needs.changes.outputs.code == 'true' + || needs.changes.result == 'failure') runs-on: ubuntu-latest timeout-minutes: 5 steps: diff --git a/.github/workflows/squad-docs-links.yml b/.github/workflows/squad-docs-links.yml index db3380bfa..9ff2f8961 100644 --- a/.github/workflows/squad-docs-links.yml +++ b/.github/workflows/squad-docs-links.yml @@ -3,6 +3,10 @@ name: Docs — Weekly Link Check on: workflow_dispatch: +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: true + jobs: linkcheck: runs-on: ubuntu-latest diff --git a/.github/workflows/squad-insider-publish.yml b/.github/workflows/squad-insider-publish.yml index c3b6c86d7..acc830de8 100644 --- a/.github/workflows/squad-insider-publish.yml +++ b/.github/workflows/squad-insider-publish.yml @@ -5,30 +5,15 @@ on: branches: - insider +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + permissions: contents: read jobs: - build: - runs-on: ubuntu-latest - timeout-minutes: 10 - strategy: - matrix: - node-version: [22] - steps: - - uses: actions/checkout@v4 - - # CI Hardening: Composite action replaces duplicated setup-node + retry - # pattern (item 7). See .github/actions/setup-squad-node/action.yml. - - uses: ./.github/actions/setup-squad-node - with: - node-version: ${{ matrix.node-version }} - - - name: Build - run: npm run build - test: - needs: build runs-on: ubuntu-latest timeout-minutes: 15 strategy: diff --git a/.github/workflows/squad-insider-release.yml b/.github/workflows/squad-insider-release.yml index 36a1121bf..f266b1fe2 100644 --- a/.github/workflows/squad-insider-release.yml +++ b/.github/workflows/squad-insider-release.yml @@ -4,6 +4,10 @@ on: push: branches: [insider] +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + permissions: contents: write diff --git a/.github/workflows/squad-npm-publish.yml b/.github/workflows/squad-npm-publish.yml index b6e40f1d4..bee98cf8c 100644 --- a/.github/workflows/squad-npm-publish.yml +++ b/.github/workflows/squad-npm-publish.yml @@ -8,6 +8,10 @@ on: description: 'Version to publish (e.g., 0.9.1)' required: true type: string + +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false permissions: contents: read id-token: write diff --git a/.github/workflows/squad-pr-readiness.yml b/.github/workflows/squad-pr-readiness.yml index 7809ceda5..223918765 100644 --- a/.github/workflows/squad-pr-readiness.yml +++ b/.github/workflows/squad-pr-readiness.yml @@ -3,7 +3,9 @@ name: Squad PR Readiness on: pull_request_target: branches: [dev, main, insider] - types: [opened, synchronize, reopened, edited, ready_for_review] + # opened + synchronize + reopened removed: redundant with workflow_run + # trigger (Squad CI fires on those events, then workflow_run fires PR readiness) + types: [edited, ready_for_review] workflow_run: workflows: ["Squad CI"] types: [completed] diff --git a/.github/workflows/squad-preview.yml b/.github/workflows/squad-preview.yml index 9df39e079..da3e308ca 100644 --- a/.github/workflows/squad-preview.yml +++ b/.github/workflows/squad-preview.yml @@ -4,6 +4,10 @@ on: push: branches: [preview] +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + permissions: contents: read diff --git a/.github/workflows/squad-promote.yml b/.github/workflows/squad-promote.yml index 9d315b1d1..ed71bd79c 100644 --- a/.github/workflows/squad-promote.yml +++ b/.github/workflows/squad-promote.yml @@ -10,6 +10,10 @@ on: type: choice options: ['false', 'true'] +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + permissions: contents: write diff --git a/.github/workflows/squad-release.yml b/.github/workflows/squad-release.yml index 6ae0f07fd..fca4f27db 100644 --- a/.github/workflows/squad-release.yml +++ b/.github/workflows/squad-release.yml @@ -4,6 +4,10 @@ on: push: branches: [main] +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + permissions: contents: write diff --git a/.github/workflows/squad-repo-health.yml b/.github/workflows/squad-repo-health.yml index 4fd722927..2ef89a5c5 100644 --- a/.github/workflows/squad-repo-health.yml +++ b/.github/workflows/squad-repo-health.yml @@ -17,26 +17,36 @@ concurrency: cancel-in-progress: true jobs: - # ─── Bootstrap Protection (BLOCKING) ──────────────────────────────── - bootstrap-protection: - name: Bootstrap Protection + # ═══════════════════════════════════════════════════════════════════════ + # Consolidated repo-health job — runs all checks on a single runner. + # Each check is a step with if: always() so all checks run even if + # one fails. Saves ~4 runner boots per PR push vs separate jobs. + # ═══════════════════════════════════════════════════════════════════════ + repo-health: + name: Repo Health runs-on: ubuntu-latest - timeout-minutes: 3 + timeout-minutes: 10 if: github.actor != 'dependabot[bot]' steps: - uses: actions/checkout@v4 with: - sparse-checkout: | - scripts/check-bootstrap-deps.mjs - sparse-checkout-cone-mode: false + fetch-depth: 0 + - name: Fetch PR head (data only — not executed) - run: git fetch origin ${{ github.event.pull_request.head.sha }} + run: | + git fetch origin ${{ github.event.pull_request.head.sha }} + git fetch origin dev --quiet + - uses: actions/setup-node@v4 with: node-version: '22' - - name: Check bootstrap dependencies + + # ─── Bootstrap Protection (BLOCKING) ────────────────────────────── + - name: "Check: Bootstrap Protection" id: bootstrap + if: always() run: | + echo "## 🔒 Bootstrap Protection" >> $GITHUB_STEP_SUMMARY set +e OUTPUT=$(node scripts/check-bootstrap-deps.mjs --ref ${{ github.event.pull_request.head.sha }} 2>&1) EXIT_CODE=$? @@ -45,142 +55,127 @@ jobs: echo "$OUTPUT" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT echo "exit_code=$EXIT_CODE" >> $GITHUB_OUTPUT + if [ "$EXIT_CODE" -eq 0 ]; then + echo "✅ Passed" >> $GITHUB_STEP_SUMMARY + else + echo "❌ Failed" >> $GITHUB_STEP_SUMMARY + fi + echo '```' >> $GITHUB_STEP_SUMMARY + echo "$OUTPUT" >> $GITHUB_STEP_SUMMARY + echo '```' >> $GITHUB_STEP_SUMMARY exit $EXIT_CODE - # ─── Diff Size Guard (WARNING) ───────────────────────────────────── - diff-guard: - name: Diff Size Guard - runs-on: ubuntu-latest - timeout-minutes: 2 - if: github.actor != 'dependabot[bot]' - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Fetch PR head (data only — not executed) - run: git fetch origin ${{ github.event.pull_request.head.sha }} - - name: Check for likely contamination + # ─── Diff Size Guard (WARNING) ──────────────────────────────────── + - name: "Check: Diff Size Guard" + if: always() run: | - git fetch origin dev --quiet + echo "## 📏 Diff Size Guard" >> $GITHUB_STEP_SUMMARY FILE_COUNT=$(git diff --name-only origin/dev...${{ github.event.pull_request.head.sha }} | wc -l) COMMIT_COUNT=$(git rev-list --count origin/dev..${{ github.event.pull_request.head.sha }}) - - # Heuristic: if a single-commit PR touches more than 30 files, it's likely contaminated if [ "$COMMIT_COUNT" -le 2 ] && [ "$FILE_COUNT" -gt 30 ]; then echo "::warning::⚠️ This PR has $COMMIT_COUNT commit(s) but touches $FILE_COUNT files." echo "::warning::This may indicate branch contamination from broad staging (--all or .) on a stale branch." echo "::warning::Please verify all changed files are intentional: git diff --name-only origin/dev...${{ github.event.pull_request.head.sha }}" + echo "⚠️ Warning: $COMMIT_COUNT commit(s) but $FILE_COUNT file(s) — possible contamination" >> $GITHUB_STEP_SUMMARY else - echo "✅ Diff looks proportional: $COMMIT_COUNT commit(s), $FILE_COUNT file(s)." + echo "✅ Diff looks proportional: $COMMIT_COUNT commit(s), $FILE_COUNT file(s)." >> $GITHUB_STEP_SUMMARY fi - # ─── Squad File Leakage (WARNING) ─────────────────────────────────── - squad-leakage: - name: Squad File Leakage - runs-on: ubuntu-latest - timeout-minutes: 3 - if: github.actor != 'dependabot[bot]' - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Fetch PR head (data only — not executed) - run: git fetch origin ${{ github.event.pull_request.head.sha }} - - uses: actions/setup-node@v4 - with: - node-version: '22' - - name: Detect .squad/ leakage + # ─── Squad File Leakage (WARNING) ───────────────────────────────── + - name: "Check: Squad File Leakage" id: leakage + if: always() run: | - git fetch origin dev --quiet + echo "## 🔍 Squad File Leakage" >> $GITHUB_STEP_SUMMARY + echo "result<> $GITHUB_OUTPUT + echo "(no output)" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT OUTPUT=$(node scripts/check-squad-leakage.mjs origin/dev ${{ github.event.pull_request.head.sha }} 2>&1) echo "$OUTPUT" echo "result<> $GITHUB_OUTPUT echo "$OUTPUT" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT - - name: Comment on leakage + echo '```' >> $GITHUB_STEP_SUMMARY + echo "$OUTPUT" >> $GITHUB_STEP_SUMMARY + echo '```' >> $GITHUB_STEP_SUMMARY + + - name: "Comment: Leakage" if: always() uses: actions/github-script@v7 + env: + LEAKAGE_OUTPUT: ${{ steps.leakage.outputs.result }} with: script: | const { run } = await import(`${process.env.GITHUB_WORKSPACE}/scripts/repo-health-comment.mjs`); await run({ github, context, - output: `${{ steps.leakage.outputs.result }}`, + output: process.env.LEAKAGE_OUTPUT ?? '', job: 'leakage', }); - # ─── Architectural Review (INFORMATIONAL) ─────────────────────────── - architectural-review: - name: Architectural Review — Structure & Design Rules - runs-on: ubuntu-latest - timeout-minutes: 5 - if: github.actor != 'dependabot[bot]' - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Fetch PR head (data only — not executed) - run: git fetch origin ${{ github.event.pull_request.head.sha }} - - uses: actions/setup-node@v4 - with: - node-version: '22' - - name: Run architectural review + # ─── Architectural Review (INFORMATIONAL) ───────────────────────── + - name: "Check: Architectural Review" id: arch + if: always() run: | - git fetch origin dev --quiet + echo "## 🏗️ Architectural Review" >> $GITHUB_STEP_SUMMARY + echo "result<> $GITHUB_OUTPUT + echo "(no output)" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT OUTPUT=$(node scripts/architectural-review.mjs origin/dev ${{ github.event.pull_request.head.sha }} 2>&1) echo "$OUTPUT" echo "result<> $GITHUB_OUTPUT echo "$OUTPUT" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT - - name: Comment on findings + echo '```' >> $GITHUB_STEP_SUMMARY + echo "$OUTPUT" >> $GITHUB_STEP_SUMMARY + echo '```' >> $GITHUB_STEP_SUMMARY + + - name: "Comment: Architectural Review" if: always() uses: actions/github-script@v7 + env: + ARCH_OUTPUT: ${{ steps.arch.outputs.result }} with: script: | const { run } = await import(`${process.env.GITHUB_WORKSPACE}/scripts/repo-health-comment.mjs`); await run({ github, context, - output: `${{ steps.arch.outputs.result }}`, + output: process.env.ARCH_OUTPUT ?? '', job: 'architectural', }); - # ─── Security Review (INFORMATIONAL) ──────────────────────────────── - security-review: - name: Security Review — Permissions & Secrets - runs-on: ubuntu-latest - timeout-minutes: 5 - if: github.actor != 'dependabot[bot]' - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Fetch PR head (data only — not executed) - run: git fetch origin ${{ github.event.pull_request.head.sha }} - - uses: actions/setup-node@v4 - with: - node-version: '22' - - name: Run security review + # ─── Security Review (INFORMATIONAL) ────────────────────────────── + - name: "Check: Security Review" id: security + if: always() run: | - git fetch origin dev --quiet + echo "## 🔐 Security Review" >> $GITHUB_STEP_SUMMARY + echo "result<> $GITHUB_OUTPUT + echo "(no output)" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT OUTPUT=$(node scripts/security-review.mjs origin/dev ${{ github.event.pull_request.head.sha }} 2>&1) echo "$OUTPUT" echo "result<> $GITHUB_OUTPUT echo "$OUTPUT" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT - - name: Comment on findings + echo '```' >> $GITHUB_STEP_SUMMARY + echo "$OUTPUT" >> $GITHUB_STEP_SUMMARY + echo '```' >> $GITHUB_STEP_SUMMARY + + - name: "Comment: Security Review" if: always() uses: actions/github-script@v7 + env: + SECURITY_OUTPUT: ${{ steps.security.outputs.result }} with: script: | const { run } = await import(`${process.env.GITHUB_WORKSPACE}/scripts/repo-health-comment.mjs`); await run({ github, context, - output: `${{ steps.security.outputs.result }}`, + output: process.env.SECURITY_OUTPUT ?? '', job: 'security', }); diff --git a/.github/workflows/squad-scope-check.yml b/.github/workflows/squad-scope-check.yml index 477dc19c5..7aeec1370 100644 --- a/.github/workflows/squad-scope-check.yml +++ b/.github/workflows/squad-scope-check.yml @@ -7,6 +7,10 @@ permissions: pull-requests: read contents: read +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + jobs: scope-boundary: name: "Scope Boundary" diff --git a/.github/workflows/sync-squad-labels.yml b/.github/workflows/sync-squad-labels.yml index 699fc680f..be3b15230 100644 --- a/.github/workflows/sync-squad-labels.yml +++ b/.github/workflows/sync-squad-labels.yml @@ -7,6 +7,10 @@ on: - '.ai-team/team.md' workflow_dispatch: +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: true + permissions: issues: write contents: read diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ff58fd677..d64591dbc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -170,8 +170,6 @@ An automated readiness check runs on every PR and posts a checklist comment. Add | **No merge conflicts** | Resolve any conflicts with the target branch | | **CI passing** | All CI checks (build, test, lint) must be green | -The readiness comment also includes a **file list with line stats** — each changed file is shown with per-file addition/deletion counts, a scope classification (Product/Infrastructure/Mixed), and totals. This helps reviewers quickly gauge PR size and impact. - The readiness check is **informational** — it helps you self-serve before a human reviewer looks at your PR. It automatically re-runs after Squad CI completes, so the checklist stays up to date without manual intervention. See `.github/PR_REQUIREMENTS.md` for the full requirements spec. ## Code Style & Conventions diff --git a/scripts/pr-readiness.mjs b/scripts/pr-readiness.mjs index 7b49cbdd5..e052771c0 100644 --- a/scripts/pr-readiness.mjs +++ b/scripts/pr-readiness.mjs @@ -25,6 +25,15 @@ export const SELF_CHECK_NAMES = ['readiness', 'PR Readiness Check']; /** Regex for source files that require a changeset. */ export const SOURCE_PATTERN = /^packages\/squad-(sdk|cli)\/src\//; +/** Bootstrap files that must remain zero-dependency. */ +export const PROTECTED_FILES = [ + 'packages/squad-cli/src/cli/core/detect-squad-dir.ts', + 'packages/squad-cli/src/cli/core/errors.ts', + 'packages/squad-cli/src/cli/core/gh-cli.ts', + 'packages/squad-cli/src/cli/core/output.ts', + 'packages/squad-cli/src/cli/core/history-split.ts', +]; + // --------------------------------------------------------------------------- // Pure check functions // --------------------------------------------------------------------------- @@ -246,6 +255,47 @@ export function checkCIStatus(checkRuns, statuses) { return { pass: true, detail: 'All checks passing' }; } +/** + * Check 10: Issue linkage — PR body or commit message references an issue. + * @param {string} prBody — PR description text + * @param {Array<{ commit: { message: string } }>} commits + * @returns {{ pass: boolean, detail: string }} + */ +export function checkIssueLinkage(prBody, commits) { + const issuePattern = /(closes|fixes|resolves|part of)\s+#\d+/i; + const bodyHasRef = issuePattern.test(prBody || ''); + const commitHasRef = (commits || []).some( + (c) => issuePattern.test(c.commit?.message || ''), + ); + if (bodyHasRef || commitHasRef) { + return { pass: true, detail: 'Issue reference found' }; + } + return { + pass: false, + detail: 'No issue reference — add `Closes #N` to PR body or commit message', + }; +} + +/** + * Check 11: Protected file changes (informational). + * Warns when bootstrap zero-dependency files are modified. + * @param {Array<{ filename: string }>} files — files changed in the PR + * @returns {{ pass: boolean, detail: string }} + */ +export function checkProtectedFiles(files) { + const touched = (files || []).filter( + (f) => PROTECTED_FILES.includes(f.filename), + ); + if (touched.length === 0) { + return { pass: true, detail: 'No protected bootstrap files changed' }; + } + const names = touched.map((f) => f.filename.split('/').pop()).join(', '); + return { + pass: true, + detail: `⚠️ ${touched.length} protected bootstrap file(s) changed: ${names} — verify zero-dependency constraint`, + }; +} + // --------------------------------------------------------------------------- // Scope classification // --------------------------------------------------------------------------- @@ -586,6 +636,13 @@ export async function run({ env = process.env, fetchFn = globalThis.fetch } = {} } checks.push({ name: 'CI passing', ...checkCIStatus(checkRuns, statusEntries) }); + // 10. Issue linkage + const prBody = prData?.body || ''; + checks.push({ name: 'Issue linked', ...checkIssueLinkage(prBody, commits) }); + + // 11. Protected file changes (informational) + checks.push({ name: 'Protected files', ...checkProtectedFiles(files) }); + // ── Build checklist and upsert comment ── const body = buildChecklist(checks, owner, repo, prBaseRef, prHeadSha, files); diff --git a/scripts/security-review.mjs b/scripts/security-review.mjs index eef502ed8..1c2f06518 100644 --- a/scripts/security-review.mjs +++ b/scripts/security-review.mjs @@ -183,6 +183,8 @@ const GIT_UNSAFE_PATTERNS = [ ]; for (const file of changedFiles) { + // Skill docs reference unsafe patterns as warnings — skip them + if (file.startsWith('.copilot/skills/') && file.endsWith('.md')) continue; const added = addedByFile.get(file) || []; for (const { line, text } of added) { for (const { pattern, label } of GIT_UNSAFE_PATTERNS) { diff --git a/test/pr-readiness.test.ts b/test/pr-readiness.test.ts index c78425cfe..473f36807 100644 --- a/test/pr-readiness.test.ts +++ b/test/pr-readiness.test.ts @@ -16,6 +16,8 @@ import { checkMergeability, checkCopilotThreads, checkCIStatus, + checkIssueLinkage, + checkProtectedFiles, buildChecklist, buildFileList, sanitizeFilename, @@ -26,6 +28,7 @@ import { COMMENT_MARKER, SELF_CHECK_NAMES, SOURCE_PATTERN, + PROTECTED_FILES, } from '../scripts/pr-readiness.mjs'; // --------------------------------------------------------------------------- @@ -454,6 +457,105 @@ describe('checkCopilotThreads', () => { }); }); +// --------------------------------------------------------------------------- +// buildChecklist +// --------------------------------------------------------------------------- +// checkIssueLinkage +// --------------------------------------------------------------------------- + +describe('checkIssueLinkage', () => { + it('passes when PR body has Closes #N', () => { + const result = checkIssueLinkage('Closes #42', []); + expect(result.pass).toBe(true); + expect(result.detail).toContain('Issue reference found'); + }); + + it('passes when PR body has Fixes #N (case-insensitive)', () => { + const result = checkIssueLinkage('fixes #100', []); + expect(result.pass).toBe(true); + }); + + it('passes when PR body has Resolves #N', () => { + const result = checkIssueLinkage('Resolves #7', []); + expect(result.pass).toBe(true); + }); + + it('passes when PR body has Part of #N', () => { + const result = checkIssueLinkage('Part of #55', []); + expect(result.pass).toBe(true); + }); + + it('passes when commit message has issue reference', () => { + const commits = [{ commit: { message: 'fix: thing\n\nCloses #10' } }]; + const result = checkIssueLinkage('', commits); + expect(result.pass).toBe(true); + }); + + it('fails when neither body nor commits have issue reference', () => { + const commits = [{ commit: { message: 'update docs' } }]; + const result = checkIssueLinkage('Some description', commits); + expect(result.pass).toBe(false); + expect(result.detail).toContain('No issue reference'); + }); + + it('handles null/empty inputs gracefully', () => { + const result = checkIssueLinkage(null as unknown as string, null as unknown as []); + expect(result.pass).toBe(false); + }); + + it('handles empty string body with empty commits', () => { + const result = checkIssueLinkage('', []); + expect(result.pass).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// checkProtectedFiles +// --------------------------------------------------------------------------- + +describe('checkProtectedFiles', () => { + it('passes when no protected files are changed', () => { + const files = [{ filename: 'packages/squad-sdk/src/index.ts' }]; + const result = checkProtectedFiles(files); + expect(result.pass).toBe(true); + expect(result.detail).toContain('No protected bootstrap files'); + }); + + it('passes with warning when protected file is changed', () => { + const files = [ + { filename: 'packages/squad-cli/src/cli/core/errors.ts' }, + { filename: 'packages/squad-sdk/src/index.ts' }, + ]; + const result = checkProtectedFiles(files); + expect(result.pass).toBe(true); + expect(result.detail).toContain('⚠️'); + expect(result.detail).toContain('errors.ts'); + expect(result.detail).toContain('zero-dependency'); + }); + + it('counts multiple protected files', () => { + const files = [ + { filename: 'packages/squad-cli/src/cli/core/errors.ts' }, + { filename: 'packages/squad-cli/src/cli/core/output.ts' }, + ]; + const result = checkProtectedFiles(files); + expect(result.pass).toBe(true); + expect(result.detail).toContain('2 protected bootstrap file(s)'); + }); + + it('handles null/undefined files gracefully', () => { + const result = checkProtectedFiles(null as unknown as []); + expect(result.pass).toBe(true); + expect(result.detail).toContain('No protected bootstrap files'); + }); + + it('exports PROTECTED_FILES constant', () => { + expect(PROTECTED_FILES).toBeDefined(); + expect(PROTECTED_FILES.length).toBeGreaterThan(0); + expect(PROTECTED_FILES).toContain('packages/squad-cli/src/cli/core/errors.ts'); + }); +}); + // --------------------------------------------------------------------------- // buildChecklist // --------------------------------------------------------------------------- @@ -820,11 +922,11 @@ describe('run()', () => { function createMockFetch(overrides = {}) { const defaults = { - commits: [{ sha: 'abc123' }], + commits: [{ sha: 'abc123', commit: { message: 'fix: thing\n\nCloses #42' } }], compare: { behind_by: 0 }, reviews: [{ user: { login: 'copilot-pull-request-reviewer' }, state: 'APPROVED', submitted_at: '2025-01-01T00:00:00Z' }], files: [{ filename: '.changeset/feat.md', additions: 5, deletions: 0 }], - pr: { mergeable: true }, + pr: { mergeable: true, body: 'Closes #42' }, checkRuns: { check_runs: [{ name: 'build', conclusion: 'success', status: 'completed' }] }, status: { statuses: [{ state: 'success' }] }, comments: [], @@ -873,7 +975,7 @@ describe('run()', () => { const result = await run({ env: baseEnv, fetchFn: mockFetch }); expect(result.action).toBe('created'); - expect(result.checks).toHaveLength(9); + expect(result.checks).toHaveLength(11); expect(result.checks.every((c) => c.pass)).toBe(true); // Verify POST was called for comment creation (not PATCH) @@ -1006,10 +1108,10 @@ describe('run()', () => { expect(copilotCheck.detail).toContain('No Copilot review yet'); }); - it('always produces 9 checks', async () => { + it('always produces 11 checks', async () => { const mockFetch = createMockFetch(); const result = await run({ env: baseEnv, fetchFn: mockFetch }); - expect(result.checks).toHaveLength(9); + expect(result.checks).toHaveLength(11); const names = result.checks.map((c) => c.name); expect(names).toEqual([ @@ -1022,6 +1124,8 @@ describe('run()', () => { 'No merge conflicts', 'Copilot threads resolved', 'CI passing', + 'Issue linked', + 'Protected files', ]); });