ci: consolidate workflows, path filters, concurrency (PRD #872 Phases 0-2, 4-5)#873
ci: consolidate workflows, path filters, concurrency (PRD #872 Phases 0-2, 4-5)#873
Conversation
🛫 PR Readiness Check
PR Scope: 🔧 Infrastructure
|
| Status | Check | Details |
|---|---|---|
| ✅ | Single commit | 1 commit — clean history |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | No source files changed — changeset not required |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ✅ | Copilot threads resolved | 2 active Copilot thread(s) resolved (2 outdated skipped) |
| ❌ | CI passing | 6 check(s) still running |
Files Changed (17 files, +981 −359)
| File | +/− |
|---|---|
.copilot/skills/pr-lifecycle/SKILL.md |
+537 −0 |
.github/workflows/squad-ci.yml |
+159 −249 |
.github/workflows/squad-docs-links.yml |
+4 −0 |
.github/workflows/squad-insider-publish.yml |
+4 −19 |
.github/workflows/squad-insider-release.yml |
+4 −0 |
.github/workflows/squad-npm-publish.yml |
+4 −0 |
.github/workflows/squad-pr-readiness.yml |
+3 −1 |
.github/workflows/squad-preview.yml |
+4 −0 |
.github/workflows/squad-promote.yml |
+4 −0 |
.github/workflows/squad-release.yml |
+4 −0 |
.github/workflows/squad-repo-health.yml |
+78 −83 |
.github/workflows/squad-scope-check.yml |
+4 −0 |
.github/workflows/sync-squad-labels.yml |
+4 −0 |
CONTRIBUTING.md |
+0 −2 |
scripts/pr-readiness.mjs |
+57 −0 |
scripts/security-review.mjs |
+2 −0 |
test/pr-readiness.test.ts |
+109 −5 |
Total: +981 −359
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
🏗️ Architectural Review
Automated architectural review — informational only. |
🟠 Impact Analysis — PR #873Risk tier: 🟠 HIGH 📊 Summary
🎯 Risk Factors
📦 Modules Affectedci-workflows (12 files)
copilot-config (1 file)
root (1 file)
scripts (2 files)
tests (1 file)
This report is generated automatically for every PR. See #733 for details. |
diberry
changed the title
diberry
changed the title
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🔐 Security Review — RETRO
Verdict: ✅ PASS — no security blockers
I reviewed all 12 modified workflow files for permission escalation, pull_request_target safety, token scope, cancel-in-progress correctness, secret handling, and template injection.
1. Permission Escalation — ✅ CLEAR
All permissions: blocks are unchanged from the base branch. No workflow requests broader permissions after consolidation:
| Workflow | Permissions | Change? |
|---|---|---|
| squad-ci | contents: read |
None |
| squad-repo-health | contents: read, pull-requests: write |
None |
| squad-pr-readiness | contents: read, pull-requests: write, issues: write, statuses: read, checks: read |
None |
| squad-insider-publish | contents: read |
None |
| squad-npm-publish | contents: read, id-token: write |
None |
| squad-release | contents: write |
None |
| squad-insider-release | contents: write |
None |
| squad-promote | contents: write |
None |
| squad-scope-check | pull-requests: read, contents: read |
None |
| squad-docs-links | job-level issues: write |
None |
| sync-squad-labels | issues: write, contents: read |
None |
| squad-preview | contents: read |
None |
2. pull_request_target Safety — ✅ SAFE
squad-repo-health.yml: Consolidated job still checks out the BASE branch (default actions/checkout@v4 behavior on pull_request_target). PR head is only fetched as a git ref for diff analysis:
git fetch origin ${{ github.event.pull_request.head.sha }}All scripts (check-bootstrap-deps.mjs, check-squad-leakage.mjs, architectural-review.mjs, security-review.mjs) come from the trusted base branch. No PR-supplied code is executed.
Note: The old bootstrap-protection job used sparse-checkout to limit the checkout to a single script. The consolidated version uses fetch-depth: 0 (full base checkout). This is still safe because it's the base branch, but it's a slight widening of the checkout scope. Acceptable tradeoff for consolidation.
squad-pr-readiness.yml: Still uses sparse-checkout: scripts/pr-readiness.mjs — only runs trusted script with API-sourced env vars. Removal of synchronize/reopened from trigger types reduces attack surface (fewer trigger events).
3. Concurrency Groups — ✅ NO SECRETS
All 12 concurrency groups use only github.workflow, github.ref, github.event.pull_request.number, or github.event.workflow_run.pull_requests[0].number. Zero secret values in any group:
squad-ci: ${{ github.workflow }}-${{ github.ref }}
squad-repo-health: ${{ github.workflow }}-${{ github.event.pull_request.number }}
squad-pr-readiness: ${{ github.workflow }}-${{ ...number || ...number }}
squad-scope-check: ${{ github.workflow }}-${{ github.event.pull_request.number }}
squad-preview: ${{ github.workflow }}-${{ github.ref }}
All publish/release: ${{ github.workflow }}
sync-squad-labels: ${{ github.workflow }}
squad-docs-links: ${{ github.workflow }}
4. cancel-in-progress — ✅ CORRECT
All 5 publish/release workflows correctly set cancel-in-progress: false:
- ✅
squad-insider-publish.yml— false - ✅
squad-npm-publish.yml— false - ✅
squad-release.yml— false - ✅
squad-promote.yml— false - ✅
squad-insider-release.yml— false
All validation workflows use cancel-in-progress: true — correct for canceling stale runs.
5. Secret References — ✅ PROPERLY SCOPED
secrets.GITHUB_TOKEN/github.token: Used only inenv:blocks forGH_TOKEN(release/promote) andGH_TOKEN(CI label fetch). Never echoed.secrets.NPM_TOKEN: Used only inenv: NODE_AUTH_TOKENblocks in insider-publish and npm-publish. The validation step checks[ -z "$NODE_AUTH_TOKEN" ]without logging the value.- No
SQUAD_PATreferences in any modified file. - No new secrets or tokens introduced.
6. Template Injection — ✅ NO NEW RISK (pre-existing notes below)
All ${{ }} expressions in run: blocks use safe context values:
github.event_name— fixed enum, safegithub.event.pull_request.number— numeric, safegithub.event.pull_request.base.sha/.head.sha— hex SHA, safegithub.base_ref— branch name, admin-controlled, safevars.SQUAD_*— repo variables, admin-controlled, safe
Pre-existing observations (not introduced by this PR, noting for awareness):
${{ steps.*.outputs.result }} injected into JS template literals in actions/github-script. If script output contained backticks or ${...}, it could theoretically break the template. This has pull-requests: write scope. Low probability but worth hardening eventually (e.g., pass via process.env instead).
LABELS='${{ toJSON(github.event.pull_request.labels.*.name) }}' — label names with single quotes could break shell quoting. Mitigated by pull_request trigger (read-only token for forks). Pre-existing pattern.
7. Insider-Publish Build Job Removal — ✅ NO VALIDATION GAP
The removed build job only ran npm run build. The test job already runs npm run build before tests. The publish job also runs its own targeted build (npm -w packages/squad-sdk run build && npm -w packages/squad-cli run build). No validation is lost.
Summary: This PR is clean from a security perspective. All changes are structural (consolidation + concurrency), with no permission escalation, no new secrets, and no new injection vectors. Ship it. 🚢
— RETRO, Security Specialist
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🐕 FIDO — Quality Gate Preservation Review
Verdict: ✅ APPROVE (with minor notes)
I reviewed every consolidated workflow against the original safety gates on dev. All quality gates are preserved.
1. Repo Health (squad-repo-health.yml) — ✅ All 5 gates present
| # | Original Job | PR Step | if: always() |
$GITHUB_STEP_SUMMARY |
|---|---|---|---|---|
| 1 | bootstrap-protection |
"Check: Bootstrap Protection" | ✅ | ✅ |
| 2 | diff-guard |
"Check: Diff Size Guard" | ✅ | ✅ |
| 3 | squad-leakage |
"Check: Squad File Leakage" | ✅ | ✅ |
| 4 | architectural-review |
"Check: Architectural Review" | ✅ | ✅ |
| 5 | security-review |
"Check: Security Review" | ✅ | ✅ |
Comment steps ("Comment: Leakage", "Comment: Architectural Review", "Comment: Security Review") also have if: always(). ✅
2. Policy Gates (squad-ci.yml) — ✅ All 4 gates present
| # | Original Job | PR Step | if: always() |
$GITHUB_STEP_SUMMARY |
|---|---|---|---|---|
| 1 | changelog-gate |
"Gate: Changelog" | ✅ | ✅ |
| 2 | changelog-protection |
"Gate: CHANGELOG Write Protection" | ✅ | ✅ |
| 3 | workspace-integrity |
"Gate: Workspace Integrity" | ✅ | ✅ |
| 4 | prerelease-version-guard |
"Gate: Prerelease Version Guard" | ✅ | ✅ |
3. publish-policy remains its own job — ✅
Confirmed: publish-policy is not part of the policy-gates consolidation. It has no needs: or if: condition, so it runs on both pull_request and push events, matching original behavior.
4. if: always() on consolidated steps — ✅
All gate/check steps in both consolidated jobs have if: always() (sometimes with additional conditions like skip-labels). Setup steps (checkout, setup-node) intentionally do not have if: always() — this is correct since subsequent always() steps will still fire and report appropriate errors.
5. $GITHUB_STEP_SUMMARY usage — ✅
All 9 consolidated check steps write to $GITHUB_STEP_SUMMARY for clear error reporting. Original standalone jobs did not have this — this is an improvement.
6. Path filter logic — ✅
code=false(docs-only PRs): Correctly skipstest,policy-gates,exports-map-check,samples-build,export-smoke-test.docs-qualitystill runs (no code condition). ✅- Push events:
testhasgithub.event_name == 'push'fail-open. Policy gates only run on PRs (same as before). ✅ - Fail-open semantics: All gated jobs include
needs.changes.result != 'success'— if path filter fails, all jobs run. ✅
7. Regression check — ✅ No missing checks
dev jobs: changes, docs-quality, test, changelog-gate, changelog-protection, exports-map-check, samples-build, publish-policy, workspace-integrity, prerelease-version-guard, export-smoke-test (11 jobs)
PR jobs: changes, docs-quality, test, policy-gates (4-in-1), exports-map-check, samples-build, publish-policy, export-smoke-test (8 jobs, same 11 checks)
Every original check is accounted for in the PR. No gates removed.
8. Phase 3 revert — ✅ Clean
Zero template files appear in the diff. squad-triage.yml, squad-issue-assign.yml, and squad-label-enforce.yml are untouched — they match dev exactly.
Minor Notes (non-blocking)
-
Defensive default pattern in repo-health steps: The leakage, arch-review, and security-review steps write
resultoutput twice (first "(no output)" default, then actual output). This is a smartset -eguard — if the node script crashes, the comment step still has a value to work with. GitHub Actions resolves duplicate heredoc keys to the last value. Clever but worth a comment in the YAML for future maintainers. -
Label fetch approach change: Original
workspace-integrityandprerelease-version-guardusedtoJSON(github.event.pull_request.labels.*.name)(webhook snapshot). PR usesgh pr view(real-time API call). This is actually an improvement — labels added after the PR event are now picked up. -
Sparse checkout removed for repo-health: Original
bootstrap-protectionused sparse checkout (onlyscripts/check-bootstrap-deps.mjs). Consolidated job does full checkout withfetch-depth: 0. Minor time increase per run, but saves ~4 runner boots overall — net positive.
Summary: All quality gates are preserved, if: always() is correctly applied, step summaries are added, path filtering is sound with fail-open semantics, and Phase 3 is cleanly deferred. Ship it. 🚢
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🔧 EECOM DX Review — PR #873 (CI Consolidation)
Reviewed the full diff across all 12 workflow files from a daily developer workflow perspective.
1. Time-to-green
Verdict: Net improvement ✅
- repo-health: 5 parallel jobs → 1 sequential job. The old jobs each paid ~30-60s runner boot overhead (checkout + setup-node × 5 runners). The new consolidated job pays that once. The checks themselves are lightweight scripts (<5s each). Net: ~2-3 min faster wall-clock.
- policy-gates: 4 jobs → 1 job. Same math — saves ~3 runner boots. Each gate is a simple script. Sequential overhead is negligible.
- insider-publish: Removing the redundant
buildjob saves a full runner boot cycle (~60s). - Path filters: Docs-only PRs now skip
test,policy-gates,exports-map-check,samples-build,export-smoke-test— this is a huge DX win for doc contributors.
2. Failure clarity
Verdict: Excellent ✅
Every consolidated step has clear, descriptive names following a consistent pattern:
- repo-health:
"Check: Bootstrap Protection","Check: Diff Size Guard","Check: Squad File Leakage", etc. - policy-gates:
"Gate: Changelog","Gate: CHANGELOG Write Protection","Gate: Workspace Integrity","Gate: Prerelease Version Guard"
GITHUB_STEP_SUMMARY is used throughout with emoji headers (## 🔒 Bootstrap Protection, ## 📏 Diff Size Guard, etc.). When you click into a failed check, the Job Summary tab will show exactly which gate failed with ✅/❌ indicators. This is better than the old approach where you had to find the right job.
3. PR checks list
Verdict: Acceptable tradeoff
Before: ~12 separate check entries in the PR UI.
After: Fewer, coarser entries (Repo Health, Policy Gates, test, docs-quality, etc.)
The tradeoff: you lose at-a-glance granularity (can't see "Bootstrap Protection ✅" directly in the checks list). You see Repo Health ❌ and have to click in to find which sub-check failed. But the step-level detail and GITHUB_STEP_SUMMARY make drilling in fast. For a team of this size, this is fine.
4. Path filter behavior (docs-only PRs)
Verdict: Correct ✅
For docs-only pushes, needs.changes.outputs.code will be 'false'. The test job condition:
always() && !cancelled()
&& (github.event_name == 'push'
|| needs.changes.result != 'success'
|| needs.changes.outputs.code == 'true')
evaluates to false → job shows as "Skipped" (grey ⊘) in GitHub UI. Not "pending forever." ✅
The !cancelled() status function prevents GitHub from prepending the implicit success(), so fail-open semantics work correctly: if the changes job itself fails, needs.changes.result != 'success' is true → tests still run. Smart.
Note for future: The PR description confirms no branch protection rules on dev. If required checks are added later, GitHub's current behavior (since Feb 2023) treats skipped required checks as passing, so this should still work. Worth documenting.
5. Skip label granularity
Verdict: Preserved ✅
The consolidated policy-gates job fetches all labels once and computes per-gate flags:
skip-changelog→skip_changelog=true→ skips "Gate: Changelog"skip-workspace-check→skip_workspace=true→ skips "Gate: Workspace Integrity"skip-version-check→skip_version=true→ skips "Gate: Prerelease Version Guard"
Each gate step has its own if condition. Individual skip labels work exactly as before.
6. Insider-publish optimization
Verdict: Correct ✅
Before: build → test (needs: build) → registry-check (needs: test) → publish (needs: [test, registry-check])
After: test → registry-check (needs: test) → publish (needs: [test, registry-check])
The old test job already ran npm run build as its first step (and the publish job also runs its own build). The separate build job was pure waste — it built, and then test rebuilt from scratch on a different runner. Removing it saves a full runner cycle with zero risk.
7. Phase 3 (issue workflow consolidation)
Verdict: Clean ✅
Phase 3 was deferred, not reverted. The issue workflow files (squad-triage.yml, squad-issue-assign.yml, squad-label-enforce.yml) don't appear in the diff at all. No revert needed.
Minor observations (non-blocking)
-
GITHUB_OUTPUT double-write pattern (leakage/arch/security steps): The
resultoutput is set to"(no output)"as a default, then overwritten with actual output. This is a good defensive pattern — if the script crashes withset -e, the "Comment" step still has a value to work with. -
Style inconsistency: The
testjob usesalways() && !cancelled()whileexports-map-check,samples-build,export-smoke-testuse just!cancelled(). Both work identically (thecancelled()status function prevents implicitsuccess()prepend), but the inconsistency could confuse future maintainers. Consider standardizing on one form. -
Concurrency groups are well-designed: Validation workflows use
cancel-in-progress: true(good — stale runs die fast). Publish/release usecancel-in-progress: false(correct — never cancel mid-publish). Global-mutating workflows use workflow-scoped groups withoutgithub.ref(correct — prevents label/promote race conditions).
Overall: LGTM from a DX perspective. The consolidation reduces runner cost without sacrificing failure clarity. The path filters are a big win for doc contributors. The fail-open semantics are correctly implemented throughout.
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🔍 Booster Self-Review — PR #873
✅ Verified Correct
YAML validity: All 12 workflow files parse correctly. if: conditions use >- block scalars with proper indentation. No quoting issues.
Job dependency chains: All needs: in squad-ci.yml point to changes — no circular deps, no missing refs.
Step ordering: All if: always() steps come after their dependencies. Comment-posting steps correctly follow their check steps.
Concurrency groups: 10 workflows now have concurrency. Group names use ${{ github.workflow }} (unique per workflow) — no cross-workflow collision risk.
- Validation workflows:
cancel-in-progress: true✅ - Publish/release workflows:
cancel-in-progress: false✅ - PR-scoped workflows (scope-check):
...-${{ github.event.pull_request.number }}✅ - Ref-scoped (preview, CI):
...-${{ github.ref }}✅
Repo-health consolidation (5 checks → 1 job):
- ✅ All 5 original checks present as steps: bootstrap, diff-guard, leakage, arch, security
- ✅
fetch-depth: 0set (needed for git diff) - ✅
git fetch origin dev --quietmoved to shared setup - ✅ Step outputs (
steps.leakage.outputs.result, etc.) correctly read by comment-posting steps - ✅
actions/setup-node@v4+ checkout done once at top
Policy-gates consolidation (4 jobs → 1 job):
- ✅ Checkout + setup-node done once
- ✅ Labels fetched once, per-gate skip flags extracted
- ✅ All 4 gates: changelog, CHANGELOG write protection, workspace integrity, prerelease version guard
- ✅ SDK-only gates (changelog) still check
packages/squad-(sdk|cli)/src/before applying - ✅ Feature flags (
vars.SQUAD_*_CHECK) and skip labels preserved
Path filter code output:
- ✅ Detection:
grep -qvE '^(docs/|README\.md|\.markdownlint|\.cspell|cspell\.json)'correctly identifies non-docs changes - ✅ Fail-open semantics:
needs.changes.result != 'success'runs jobs when filter fails - ✅ Push events always run all jobs (
github.event_name == 'push') - ✅ Gated jobs use consistent condition syntax with
!cancelled()
Permissions: No job-level permission changes needed — workflow-level contents: read covers consolidated jobs.
Skip labels: skip-changelog, skip-workspace-check, skip-version-check all correctly wired to their respective gates via steps.labels.outputs.skip_*.
Template files: squad-triage.yml, squad-issue-assign.yml, squad-label-enforce.yml confirmed NOT modified ✅
Insider-publish optimization: Redundant build job removed — test already builds via npm run build step.
PR readiness: synchronize + reopened removed from pull_request_target — correct, these fire via workflow_run after Squad CI completes.
⚠️ Non-Blocking Findings (monitor / follow-up)
1. Double GITHUB_OUTPUT writes in repo-health (leakage, arch, security steps)
Pattern writes a default result value, then overwrites after script runs:
echo "result<<EOF" >> $GITHUB_OUTPUT
echo "(no output)" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUT
OUTPUT=$(node scripts/...) # may crash here
echo "result<<EOF" >> $GITHUB_OUTPUT
echo "$OUTPUT" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUTThis works because: (a) if script crashes, only the first write exists → comment step reads "(no output)"; (b) if script succeeds, GitHub Actions takes the last value. Risk: behavior depends on GitHub taking last-wins for duplicate heredoc keys. Consider adopting the set +e / capture-exit-code pattern used by the bootstrap step for consistency.
2. Empty $CHANGED edge case in code detection
If $CHANGED is empty (zero-change PR or initial push fallback), echo "" | grep -qvE '...' matches the empty line → code=true. This means zero-change PRs would run all code jobs. Harmless in practice (PRs always have changes), but could be tightened with [ -n "$CHANGED" ] && echo "$CHANGED" | grep -qvE ....
3. Minor inconsistency: CHANGELOG Write Protection uses origin/${{ github.base_ref }}...HEAD while Changelog Gate uses "$BASE"..."$HEAD" (explicit SHAs)
Both produce the same result in normal CI, but the SHA-based approach is more deterministic. This inconsistency pre-dates this PR — just noting for future cleanup.
🎯 Verdict
LGTM with notes. All structural changes are correct. No regressions to skip-label handling, permissions, or concurrency. The three non-blocking findings are edge cases worth tracking but not PR-blocking. Net -86 lines, ~9-11 runners per PR push (down from ~19).
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🏗️ Architecture Sign-Off Review — Flight (Lead)
PRD: #872 | Phases Implemented: 0, 1, 2, 4, 5 | Verdict: ✅ APPROVE
PRD Compliance
| Phase | Status | Notes |
|---|---|---|
| Phase 0 — Branch Protection Audit | ✅ | No rules found on dev (404). All job renames safe. |
| Phase 1 — Repo Health Consolidation (5→1) | ✅ | if: always() on all steps, $GITHUB_STEP_SUMMARY, comments preserved. Clean consolidation. |
| Phase 1 — Policy Gates Consolidation (4→1) | ✅ | changelog-gate, changelog-protection, workspace-integrity, prerelease-version-guard in one job. Per-step skip labels + feature flags preserved. |
| Phase 1 — PR Readiness De-dup | ✅ | Removed synchronize/reopened; correctly kept opened (see deviations). |
| Phase 2 — Path Filters | ✅ | Inline bash detection for code output. Fail-open on test job. Docs-only PRs skip heavy jobs. |
| Phase 3 — Issue Consolidation | ⏭️ Deferred | Correct — these are product templates. Tracked separately. |
| Phase 4 — Publish Optimization | ✅ | Removed redundant build job from insider-publish. npm caching already present. |
| Phase 5 — Concurrency Groups | ✅ | Split policy correct: cancel-in-progress: true (validation), false (publish/release). |
Team Review Feedback Verification
| Reviewer | Requirement | Status |
|---|---|---|
| Booster | cancel-in-progress: false on publish workflows |
✅ All 5 publish/release workflows |
| FIDO | if: always() + $GITHUB_STEP_SUMMARY on consolidated steps |
✅ Repo-health and policy-gates |
| RETRO | No permission escalation in consolidated jobs | ✅ No new permissions or secrets |
| EECOM | Debugging visibility preserved | ✅ Clear step names, summaries, comment steps preserved |
Smart Deviations from PRD (approved)
- Kept
openedin PR Readiness — PRD said remove it, butworkflow_runonly fires after CI completes. Removingopenedwould delay the initial readiness comment by ~10 minutes. Good UX call. exports-map-checknot consolidated — PRD listed it as "lightweight" but it requiresnpm run build. Correctly kept as separate job.publish-policykept separate — Runs on push events too, not just PRs. Correct.squad-docs-linksgot concurrency — PRD dropped it, but adding it is harmless defensive measure.
⚠️ One Fix Needed Before Merge
Incomplete fail-open on 4 gated jobs. The test job correctly uses always() for fail-open:
# test job — CORRECT fail-open pattern:
if: >-
always() && !cancelled()
&& (github.event_name == 'push'
|| needs.changes.result != 'success'
|| needs.changes.outputs.code == 'true')But policy-gates, exports-map-check, samples-build, and export-smoke-test omit always():
# policy-gates — MISSING always():
if: >-
github.event_name == 'pull_request'
&& !cancelled()
&& (needs.changes.result != 'success' || needs.changes.outputs.code == 'true')Without always(), GitHub Actions applies an implicit success() gate — if changes fails, these 4 jobs silently skip instead of running (fail-open). The needs.changes.result != 'success' check is never evaluated.
Fix: Add always() && at the start of each condition, matching the test job pattern.
Greenfield Compatibility
All changes move toward the target architecture (4-5 runners/PR). No structural decisions conflict with the greenfield path. Consolidated jobs absorb cleanly into the future squad-pr-validate.yml layout.
CI Status
Latest run all green ✅. Earlier cancelled run was correctly superseded by concurrency groups.
Verdict
✅ APPROVE. Clean, well-engineered implementation of PRD #872. The fail-open fix is needed before merge but is mechanical (add always() to 4 job conditions). Net -86 lines, ~19 runners → ~9-11, all safety gates preserved, greenfield-compatible.
Adds !cancelled() and failure fallback to policy-gates, exports-map-check, samples-build, and export-smoke-test so they run even if the changes detection job fails. Addresses Flight's review finding on PR #873. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
diberry
force-pushed
the
squad/872-ci-optimization
branch
from
559d0c2 to
e141ce7
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements phases of PRD #872 to reduce GitHub Actions runner usage by consolidating checks into fewer jobs/steps, adding concurrency controls, and introducing path-based gating to skip expensive work on docs-only changes.
Changes:
- Consolidate repo-health and CI policy gates into single-runner jobs with sequential steps.
- Add concurrency groups across validation and publish/release workflows (with “don’t cancel” for publish/release).
- Add “code” path detection and use it to conditionally run test/policy/export/sample jobs.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/squad-ci.yml | Adds code change detection and gates test/policy/other jobs; consolidates multiple policy jobs into policy-gates. |
| .github/workflows/squad-repo-health.yml | Consolidates multiple repo-health jobs into a single job with step-level checks and step summaries. |
| .github/workflows/squad-pr-readiness.yml | Adjusts PR readiness triggers to reduce redundant events and adds concurrency. |
| .github/workflows/squad-insider-publish.yml | Removes redundant build job and adds publish-safe concurrency policy. |
| .github/workflows/squad-scope-check.yml | Adds PR-scoped concurrency to cancel stale scope-check runs. |
| .github/workflows/sync-squad-labels.yml | Adds workflow-scoped concurrency for label-sync runs. |
| .github/workflows/squad-docs-links.yml | Adds concurrency to prevent overlapping manual link-check runs. |
| .github/workflows/squad-preview.yml | Adds ref-scoped concurrency to cancel stale preview validation runs. |
| .github/workflows/squad-release.yml | Adds non-cancelling concurrency to avoid mid-release cancellation. |
| .github/workflows/squad-promote.yml | Adds non-cancelling concurrency to avoid mid-promotion cancellation. |
| .github/workflows/squad-insider-release.yml | Adds non-cancelling concurrency to avoid mid-release cancellation. |
| .github/workflows/squad-npm-publish.yml | Adds non-cancelling concurrency to avoid partial publish due to cancellation. |
Comments suppressed due to low confidence (1)
.github/workflows/squad-repo-health.yml:175
actions/github-scriptscript block is embedding a step output via${{ steps.security.outputs.result }}inside a JS template literal. Because the interpolated text can contain untrusted PR-derived content, this pattern risks breaking the script or enabling injection. Pass outputs viaenv:/process.envinstead of${{ }}inside the script body.
- name: "Comment: Security Review"
if: always()
uses: actions/github-script@v7
with:
script: |
const { run } = await import(`${process.env.GITHUB_WORKSPACE}/scripts/repo-health-comment.mjs`);
await run({
github,
context,
output: `${{ steps.security.outputs.result }}`,
job: 'security',
});
e141ce7 to
6cdb184
Compare
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🛡️ RETRO Security Review — PR #873 (Expanded)
A. New: squad-comment-moderation.yml
Permissions — ✅ Minimal & job-scoped
comment-filter:issues: writeonlymoderate-new-content:contents: read+issues: writeauto-lock-stale:issues: write+pull-requests: write
No top-level permissions: block — each job gets only what it needs. Good.
Triggers — ✅ Safe
issue_comment: [created, edited]— runs from default branch, not fork code. No code injection vector.issues: [opened]— same.schedule: cron '0 6 * * *'— daily stale lock.
None of these triggers execute attacker-controlled code. The actions/checkout only pulls scripts/moderate-spam.mjs via sparse-checkout from the repo's default branch.
Token — ✅ GITHUB_TOKEN (not PAT)
Passed via env: block, not interpolated into shell. Scope is automatically limited to the repository. Appropriate for the operations performed (close, label, comment, lock).
Third-party action — ✅ SHA-pinned
DecimalTurn/Comment-Filter@9c95bdb06ae1dd6b8185d58f52a07a2a71e19d94 — pinned to full SHA. Good supply-chain hygiene.
schedule job would prevent overlapping daily runs if one hangs.
B. moderate-spam.mjs — Edge Cases
| Check | Status |
|---|---|
| Null/empty body | ✅ if (!body) body = '' + tests for both null and '' |
Missing issue.user |
✅ Early return before any API calls |
| Null author profile | ✅ Fallback +1 score (profile-unavailable) |
| Profile fetch network error | ✅ try/catch, continues scoring |
Regex lastIndex statefulness |
✅ pattern.lastIndex = 0 before each .test() + regression test |
| User login in URL | ✅ encodeURIComponent(author) — prevents path traversal |
| Spam comment body injection | ✅ Reasons are code-controlled strings, no user input interpolated |
| API call failure resilience | ✅ Each action (close/label/comment) is independent try/catch — partial failure doesn't block others |
| Unicode bypass |
Test coverage: Good. 377 lines covering scoring thresholds, edge cases, API failure resilience, and regex statefulness.
C. lock-stale.mjs — ✅ Safe
- Rate limiting: 500ms delay between API calls
MAX_ITEMS: 50cap prevents runaway API usage- 422 (already locked) handled gracefully
- Required env var validation
D. Previously Flagged Items — Still Addressed
| Finding | Status |
|---|---|
cancel-in-progress: false on publish workflows |
✅ Confirmed on insider-publish, insider-release, npm-publish, release, promote |
| No permission escalation in consolidated jobs | ✅ policy-gates and repo-health don't add new permissions vs. their predecessor jobs |
pull_request_target safety |
✅ Reduced to [opened, edited, ready_for_review] — removed synchronize/reopened |
E. Existing Reviewer Findings I Concur With
-
Expression injection in
github-scriptblocks (repo-health lines ~113, ~145):${{ steps.leakage.outputs.result }}and${{ steps.arch.outputs.result }}are interpolated directly into JS template literals. These outputs contain PR-derived data. Should pass viaenv:and read fromprocess.env. — This is a real injection vector. -
policy-gatesruns onpushbut assumes PR payload: Steps referencegithub.event.pull_request.*which is null on push events. Should restrict to PR-only or add push-safe fallbacks.
Verdict
No blocking security issues in the new moderation workflow. Permissions are minimal, token usage is correct, the script handles edge cases well, and the third-party action is SHA-pinned. The two expression-injection findings in repo-health (flagged by the existing reviewer) remain the highest-priority items to address in this PR.
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🛫 Flight's Architectural Review — PR #873
Verdict: REQUEST CHANGES
1. Remove .squad/ session artifacts (blocking)
Two .squad/ files are included that should never ship in a feature PR:
.squad/agents/procedures/history.md— agent work log (session artifact).squad/decisions/inbox/procedures-pr-lifecycle-skill.md— decision inbox file
The repo-health bot already flagged this as Squad File Leakage. These are Procedures' session traces, not deliverables. Remove them from this commit.
2. Split spam protection (#800) into its own PR (blocking)
This PR claims the theme is "CI infrastructure" (PRD #872), but spam protection is a new community-management feature, not a CI optimization. It adds:
- 2 new scripts (
lock-stale.mjs,moderate-spam.mjs— 333 lines) - 2 new test suites (724 lines)
- 1 new workflow (
squad-comment-moderation.yml) - CONTRIBUTING.md docs update
- Its own changeset file
This is self-contained, independently reviewable, and independently revertable. Bundling it inflates the PR to 25 files / HIGH risk when the CI optimization alone would be ~17 files / MEDIUM. Ship #800 separately — it has zero coupling to the CI consolidation work.
The PR readiness expansion (#870) is fine to keep — it's tightly coupled to the CI/readiness infrastructure being modified here.
3. Fix ${{ }} injection in squad-repo-health.yml (blocking)
Copilot reviewer correctly identified script injection at lines 113 and 145. Step outputs containing PR-derived text are interpolated via ${{ steps.*.outputs.result }} directly into actions/github-script JS blocks. This is already causing the Security Review CI check to crash with SyntaxError: Unexpected identifier 'git' — the step output text is being parsed as JavaScript.
Fix: Pass the values via env: and read from process.env inside the script block.
4. Restrict policy-gates to PR events (blocking)
The policy-gates job condition now allows it to fire on push events (github.event_name != 'pull_request' makes the if true), but multiple steps reference github.event.pull_request.base.sha, github.event.pull_request.head.sha, etc. — all null on push. Either restrict the job back to github.event_name == 'pull_request' or add push-safe fallback logic.
5. PR description is accurate (good)
The description honestly discloses the expanded scope and lists all three source PRs. That transparency is appreciated — but it also makes the case for splitting: if you need a paragraph explaining why three PRs were merged, they probably shouldn't have been.
Summary
| Item | Severity | Action |
|---|---|---|
.squad/ session artifacts |
🔴 Blocking | Remove from commit |
| Spam protection (#800) scope | 🔴 Blocking | Split to own PR |
| Script injection in repo-health | 🔴 Blocking | Use env: instead of ${{ }} |
| policy-gates push event | 🔴 Blocking | Restrict to PR events |
| PR readiness (#870) bundled | ✅ Fine | Keeps — tightly coupled |
| CI consolidation (Phases 0-2, 4-5) | ✅ Good | Clean job reduction |
Fix #3 and #4, remove .squad/ files, and split spam protection out. Then this is a clean CI infrastructure PR ready for merge.
— Flight (Lead)
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🐕 FIDO Quality Review — PR #873
Verdict: COMMENT (approve pending 2 items)
✅ Readiness Checks — Correctly Integrated
Both checkIssueLinkage() and checkProtectedFiles() are well-implemented:
- Issue linkage (check 10): Proper case-insensitive regex for Closes/Fixes/Resolves/Part of. Checks both PR body and commit messages. Hard gate — correctly fails when no reference found.
- Protected files (check 11): Informational-only (always pass: true). Correctly uses the same PROTECTED_FILES list from copilot-instructions. Zero new API calls — both checks reuse data already fetched by the orchestrator.
un() updated at lines 636–644, checks array assertion updated to 11. Mock data in tests updated with Closes #42 to keep the happy path green.
✅ pr-readiness.test.ts — 13 New Tests, Good Coverage
- checkIssueLinkage (8 tests): Covers all 4 keywords (Closes/Fixes/Resolves/Part of), commit message fallback, failure case, null inputs, and empty inputs.
- checkProtectedFiles (5 tests): No protected files, single protected file warning, multiple files count, null/undefined handling, and PROTECTED_FILES export verification.
- Edge cases covered well. The null-coercion tests (
ull as unknown as string) validate defensive coding.
✅ Spam Protection Tests — Comprehensive
- lock-stale.test.ts (~15 tests): buildCutoffDate, findStaleItems (issues/PRs/empty/API errors/maxItems), lockStaleItems (lock calls, 422 already-locked, server failure + continue, network error + continue), run (summary, missing env, no stale items).
- moderate-spam.test.ts (~17 tests): calculateSpamScore (clean/shortURLs/crypto/adult/fileSharing/massMentions/newAccounts/combinations/null/empty/profile-unavailable/regex-lastIndex-safety/overlapping), moderateContent (env validation, score≥5 close+label+comment, score 3-4 needs-review, score<3 no action, network error resilience, close failure doesn't block label+comment).
- Regex safety: lastIndex resets in calculateSpamScore are correct and explicitly tested. Good practice for global-flag regexes.
✅ PR Lifecycle Skill — Accurate for 11-Check System
SKILL.md correctly documents all 11 checks with fix-it instructions and gotchas. The "Phases 1–6" runbook structure is clear and actionable.
⚠️ Issue 1: .squad/ state files should be removed
Two agent artifacts are included that shouldn't be in a product PR:
- .squad/agents/procedures/history.md — agent history entry
- .squad/decisions/inbox/procedures-pr-lifecycle-skill.md — decision inbox
These are flagged by the PR's own scope-clean check (Check 7). Remove them from this branch — the Scribe should handle decisions inbox separately.
⚠️ Issue 2: SKILL.md contains redundant gap analysis code
The bottom ~100 lines of SKILL.md (Readiness Check Gaps & Recommendations) include full implementation code samples for checks 10 and 11 marked as "IMPLEMENTED". Since the checks are already implemented in this same PR, this section is confusing — it reads like a proposal for work that's already done. Recommend trimming to just the summary table, or moving the gap analysis to a separate design doc. The deferred Gap 2 (required checks presence) can stay as a brief note.
ℹ️ Minor: Changeset has empty frontmatter
.changeset/comment-spam-protection.md has ---\n--- (no packages listed). Since no packages/*/src/ files are modified, no changeset is required — this is harmless but the empty frontmatter may confuse @changesets/cli. Consider either removing the changeset or adding a package bump if any downstream consumers are affected.
Summary
| Area | Status |
|---|---|
| checkIssueLinkage integration | ✅ Correct |
| checkProtectedFiles integration | ✅ Correct (informational) |
| pr-readiness.test.ts (13 tests) | ✅ Good coverage |
| lock-stale.test.ts | ✅ Comprehensive |
| moderate-spam.test.ts | ✅ Comprehensive |
| PR lifecycle skill accuracy | ✅ Matches 11-check system |
| .squad/ state files in PR | |
| SKILL.md gap analysis redundancy |
Overall quality is high. The two
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🔍 EECOM Review — PR #873 (Developer Experience)
1. Single-Concern Check: ❌ Three PRs in a trenchcoat
This started as CI optimization (PRD #872) but absorbed #800 (comment spam protection) and #870 (readiness check expansion). These are now three distinct concerns:
| Concern | Files | Lines | Risk |
|---|---|---|---|
| CI optimization (workflows, concurrency, path filters) | ~14 | ~500 | Low — mechanical |
| Comment spam protection (scripts, workflow, tests) | 5 | ~1060 | Medium — new runtime scripts |
| PR readiness expansion + lifecycle skill | 4 | ~700 | Low — additive |
The concurrency-group additions across 10 workflows are mechanical 4-liners — easy to review. But the spam scripts + tests (~1000 lines) and the 537-line SKILL.md are substantial, unrelated additions. A human reviewer will struggle to give each concern proper attention at this size.
Recommendation: Split into 3 PRs — (a) CI optimization, (b) comment spam protection, (c) readiness expansion + lifecycle skill. Each would be <15 files and independently reviewable.
2. .squad/ State Files: ⚠️ Leaked
Two agent artifacts are in this diff:
.squad/agents/procedures/history.md(+14) — agent session history.squad/decisions/inbox/procedures-pr-lifecycle-skill.md(+37) — decision inbox
The repo's own leakage detector already flagged this (see bot comment). These are agent work artifacts from the #870 absorption. They should be removed — git reset HEAD .squad/ && git commit --amend.
3. CONTRIBUTING.md Change: ✅ Acceptable (but belongs in #800 split)
The +10/-2 change removes a stale paragraph about file-list stats and adds a "Comment Moderation" section documenting the new spam scripts. Content is accurate and useful. However, it logically belongs with the spam protection PR if split.
4. vitest.config.ts Shebang Plugin: ✅ Safe
The strip-shebang plugin is a clean, minimal transform:
enforce: 'pre'— runs before other transforms- Only activates on files starting with
#! - Simple regex: replaces
#!line with empty string
This is needed because the new scripts/*.mjs files have shebangs that Vitest can't parse. The implementation is correct. Note: this dependency exists solely because of the spam scripts (#800 content).
5. Reviewability at 25 files / +2180 lines: ❌ Too large
The automated impact analysis already rated this 🟠 HIGH. Across 6 modules and 25 files, a human reviewer cannot effectively verify:
- Workflow YAML correctness across 13 workflow files
- Script security (spam scoring, issue locking)
- Readiness check logic changes
- 537-line skill document accuracy
all in one pass. The architectural review bot also flagged the size.
6. Unresolved Copilot Threads (4)
All 4 are legitimate and should be addressed before merge:
squad-repo-health.yml:113—${{ steps.leakage.outputs.result }}interpolated insideactions/github-scriptJS template literal → script injection risk. Useenv:instead.squad-repo-health.yml:145— Same pattern with${{ steps.arch.outputs.result }}.squad-ci.yml:234—policy-gatesruns on push events but referencesgithub.event.pull_request.*which is null on push → will fail or behave unpredictably. Restrict to PR events or add push-safe fallback.squad-pr-readiness.yml:8—pull_request_targetstill hasopenedtype → duplicate runs when combined withworkflow_runtrigger.
7. Changeset Note
The changeset has empty frontmatter (---\n---) with no package specified. Since no packages/*/src/ files are touched, this is technically correct — but an empty changeset is unusual. Consider whether skip-changelog label would be cleaner.
Bottom line: The individual changes are well-implemented, but absorbing #800 and #870 has made this unreviewable as a single unit. Split it, remove the .squad/ files, and address the 4 Copilot threads. Each resulting PR would be clean and mergeable.
diberry
left a comment
diberry
left a comment
There was a problem hiding this comment.
🔧 Booster CI/CD Review — PR #873
✅ Verified (all good)
Workflow consolidation (12 files):
All 12 original workflow files have proper concurrency groups:
| Workflow | Group Pattern | cancel-in-progress |
|---|---|---|
| squad-ci | workflow-ref |
true |
| squad-docs-links | workflow |
true |
| squad-insider-publish | workflow |
false ✅ (publish) |
| squad-insider-release | workflow |
false ✅ (publish) |
| squad-npm-publish | workflow |
false ✅ (publish) |
| squad-pr-readiness | workflow-PR# |
true |
| squad-preview | workflow-ref |
true |
| squad-promote | workflow |
false ✅ (deploy) |
| squad-release | workflow |
false ✅ (publish) |
| squad-repo-health | workflow-PR# |
true |
| squad-scope-check | workflow-PR# |
true |
| sync-squad-labels | workflow |
true |
Validation-type workflows use cancel-in-progress: true, publish/deploy use false. Pattern is consistent. ✅
PR Readiness (#870 content):
- All 11 check functions exported and called in
run():checkCommitCount,checkDraftStatus,checkBranchFreshness,checkCopilotReview,checkChangeset,checkMergeability,checkScopeClean,checkCopilotThreads,checkCIStatus,checkIssueLinkage(new),checkProtectedFiles(new). ✅ squad-pr-readiness.ymlcorrectly wiresPR_LABELSenv var for the new checks. ✅
Comment moderation (#800 content):
lock-stale.mjs— Clean DI pattern (fetchFn,env), proper rate-limit delay, handles 422 (already locked). ✅moderate-spam.mjs— Score thresholds (≥5 close, ≥3 flag, <3 pass), regexlastIndexreset, profile fallback. ✅- Workflow has correct triggers:
issue_comment→ filter,issues→ moderate,schedule→ lock-stale. ✅ - Minimal permissions per job (
issues: write,contents: read). ✅
Tests:
lock-stale.test.ts— 4/4 exports tested, error resilience, batch failures. ✅moderate-spam.test.ts— 2/3 exports tested (see minor note below), all score thresholds, API mocking. ✅pr-readiness.test.ts— 11/11 checks + 6 utilities, 46KB of comprehensive tests. ✅
Changeset: Empty frontmatter (no package bump) is correct — CI-only changes don't touch packages/*/src/. ✅
YAML syntax: All 13 workflow files parse cleanly via GitHub API. ✅
CONTRIBUTING.md: Comment Moderation section added, accurate description. ✅
vitest.config.ts: strip-shebang plugin added for #!/usr/bin/env node scripts — needed for the new .mjs test imports. ✅
🔴 Issue: Missing concurrency group on squad-comment-moderation.yml
Every other workflow in this PR has a concurrency group — this is the one exception. The schedule cron job (auto-lock-stale) could overlap with itself if it runs longer than 24h (unlikely but possible at scale), and rapid issue_comment edits could spawn parallel comment-filter runs.
Suggested fix — add at the workflow level:
concurrency:
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.issue.number || 'scheduled' }}
cancel-in-progress: trueThis gives each issue its own lane while preventing duplicate scheduled runs.
🟡 Issue: .squad/ state files leaked into PR
Two files should not be in this diff:
.squad/agents/procedures/history.md(+14 lines) — agent execution history.squad/decisions/inbox/procedures-pr-lifecycle-skill.md(+37 lines, new) — decision inbox entry
These are squad coordination artifacts, not deliverables. The repo-health check-squad-leakage job will flag these. Remove before merge:
git reset HEAD .squad/agents/procedures/history.md .squad/decisions/inbox/procedures-pr-lifecycle-skill.md
git checkout -- .squad/agents/procedures/history.md
git rm --cached .squad/decisions/inbox/procedures-pr-lifecycle-skill.md
git commit --amend --no-edit📝 Minor note
moderate-spam.test.ts doesn't import/test the SPAM_SIGNALS constant. Low priority — it's a config object exercised indirectly through calculateSpamScore tests — but a shape assertion would guard against accidental mutation.
Verdict: Two actionable items (concurrency group + state file cleanup), otherwise this is solid work consolidating 3 PRs into a clean 25-file diff. The DI patterns in the new scripts are excellent for testability.
diberry
force-pushed
the
squad/872-ci-optimization
branch
2 times, most recently
from
d154044 to
26c9911
Compare
…sion (PRD #872) Phases 0-2, 4-5 of CI optimization PRD: - Consolidate repo-health (5 jobs -> 1 sequential) - Consolidate policy-gates (4 jobs -> 1 sequential) - Fix PR readiness double-trigger (remove synchronize/reopened from pull_request_target) - Add code path filter with fail-open semantics - Add 10 concurrency groups (cancel-in-progress for validation, not for publish) - Remove redundant insider-publish build job - Expand readiness checks 9 -> 11 (issue linkage + protected files) - Add PR lifecycle skill (.copilot/skills/pr-lifecycle/SKILL.md) - Fix script injection in repo-health (env: pattern for github-script) - Skip .copilot/skills/*.md in security scanner (band-aid for SyntaxError) Estimated savings: ~40-45% reduction in Actions minutes (~11,400 min/month). Closes #872 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
diberry
force-pushed
the
squad/872-ci-optimization
branch
from
26c9911 to
d82c14b
Compare
tamirdresher
left a comment
tamirdresher
left a comment
There was a problem hiding this comment.
✅ Reviewed by Tamir + Squad (Data & Picard). Solid CI consolidation — saves ~90s/PR from runner boot reduction. Concurrency controls are correct (cancel-in-progress for PRs, no-cancel for publish). Security fix for template literal injection is textbook. #870 overlap resolved (Dina closing #870). Skills security CI failure tracked as follow-up via microsoft/waza. Ship it.
${{ }}script injection insquad-repo-health.yml— all 3 comment steps (leakage, arch, security) useenv:block +process.env(not inline interpolation)security-review.mjsexcludes.copilot/skills/*.mdfrom unsafe-git check (prevents false positives from documentation examples).squad/artifacts in PR diffpull_requestevents only (resolved)pull_request_targettrigger removessynchronize/reopened(resolved)pull_request_targetruns the base-branch (dev) workflow which has the old inline injection and no.copilot/skills/exclusion; both fixes are in this PR and will take effect on merge