feat: enhance FIDO with PR requirements enforcement + workflow guide#676
feat: enhance FIDO with PR requirements enforcement + workflow guide#676tamirdresher wants to merge 1 commit intodevfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Enhances repo contribution quality guidance by extending the FIDO (Quality Owner) charter to enforce PR requirements pre-CI, updating Copilot repo instructions with a pre-push checklist, and adding a comprehensive GitHub workflow guide intended to orient new contributors.
Changes:
- Add PR-requirements enforcement responsibilities and runnable pre-push validation steps to FIDO’s charter.
- Add a PR requirements pre-push checklist to
.github/copilot-instructions.md. - Add a long-form workflow/reference guide with Mermaid diagrams and tables describing repo processes and CI gates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| docs/GITHUB_WORKFLOW_GUIDE.md | New end-to-end workflow guide (roles, branches, CI, labels, requirements) with Mermaid diagrams and reference tables. |
| .squad/agents/fido/charter.md | Expands FIDO responsibilities to include PR requirements enforcement and pre-push/PR-review checks. |
| .github/copilot-instructions.md | Adds a pre-push quality checklist aligned with PR requirements and CI gates. |
| ### 🆕 Added by Dina: | ||
|
|
||
| | CI Job | What It Checks | PR | | ||
| |--------|---------------|-----| | ||
| | **`changelog-gate`** | If you change SDK/CLI source, you MUST update CHANGELOG.md | #673 | | ||
| | **`exports-map-check`** | New `src/*/index.ts` barrels must have matching `package.json` exports | #673 | | ||
| | **`samples-build`** | Builds all 11 sample projects against your PR's SDK code | #674 | |
There was a problem hiding this comment.
This table lists a samples-build CI job (and references PR #674), but .github/workflows/squad-ci.yml currently does not define a samples-build job. Update the guide to reflect the actual CI jobs, or mark this as planned (e.g., #103) rather than already present.
| ### 🆕 Added by Dina: | |
| | CI Job | What It Checks | PR | | |
| |--------|---------------|-----| | |
| | **`changelog-gate`** | If you change SDK/CLI source, you MUST update CHANGELOG.md | #673 | | |
| | **`exports-map-check`** | New `src/*/index.ts` barrels must have matching `package.json` exports | #673 | | |
| | **`samples-build`** | Builds all 11 sample projects against your PR's SDK code | #674 | | |
| ### 🆕 Added by Dina (includes planned jobs): | |
| | CI Job | What It Checks | PR | | |
| |--------|---------------|-----| | |
| | **`changelog-gate`** | If you change SDK/CLI source, you MUST update CHANGELOG.md | #673 | | |
| | **`exports-map-check`** | New `src/*/index.ts` barrels must have matching `package.json` exports | #673 | | |
| | **`samples-build`** | Planned: builds all 11 sample projects against your PR's SDK code (job not yet active in `.github/workflows/squad-ci.yml`) | #674 (planned) | |
| |----------|---------|---------|----------------------------| | ||
| | `SQUAD_CHANGELOG_CHECK` | CHANGELOG gate | ✅ On | Skips CHANGELOG requirement globally | | ||
| | `SQUAD_EXPORTS_CHECK` | Exports map check | ✅ On | Skips exports check globally | | ||
| | `SQUAD_SAMPLES_CI` | Samples build | ✅ On | Skips sample validation globally | |
There was a problem hiding this comment.
SQUAD_SAMPLES_CI is documented here as a repo variable, but there is currently no SQUAD_SAMPLES_CI feature flag referenced anywhere in workflows/scripts. Please remove it or clearly label it as future/planned so readers don’t look for a setting that doesn’t exist.
| | `SQUAD_SAMPLES_CI` | Samples build | ✅ On | Skips sample validation globally | | |
| | `SQUAD_SAMPLES_CI` | (Planned) Samples build toggle — not yet wired into CI | N/A (planned) | Future flag; safe to ignore for now | |
| | `skip-changelog` | CHANGELOG gate | CI/infra-only changes that don't need a changelog entry | | ||
| | `skip-exports-check` | Exports map check | Missing export is intentional or tracked separately | | ||
| | `skip-samples-ci` | Samples build | Sample updates coming in a follow-up PR | | ||
| | `large-deletion-approved` | Deletion guard (>50 files) | Intentional mass refactors or migrations | |
There was a problem hiding this comment.
skip-samples-ci is documented as a PR skip label, but the current CI workflow doesn’t check for it (and there is no samples CI gate to skip). Please remove it from the guide or align it with an implemented gate/label.
| ### 3. Samples build | ||
| If you changed SDK source files, all 11 sample projects will be built against your changes in CI. Run `npm run build` in any affected sample directory to catch issues early. Bypass: `skip-samples-ci` label (requires reviewer approval). |
There was a problem hiding this comment.
This checklist references a skip-samples-ci label and says CI builds all 11 samples for SDK changes, but there is currently no samples-build job, skip-samples-ci label check, or SQUAD_SAMPLES_CI feature flag in .github/workflows/squad-ci.yml. This will mislead contributors; either remove/mark this as a future (#103) manual-only requirement or implement the corresponding CI gate + label/flag support.
| 6. **Template completeness**: Verify the PR description includes non-empty content for: What, Why, How, Testing. If SDK/CLI files changed, Docs section must mention CHANGELOG. | ||
| 7. **Issue reference**: PR body must contain `Closes #N` or `Part of #N`. | ||
| 8. **Breaking changes**: If any public API signature changed (parameter name, type, return type, removed export), the Breaking Changes section must be filled. | ||
| 9. **Waiver documentation**: If any skip label (`skip-changelog`, `skip-exports-check`, `skip-samples-ci`) is present, verify a `## Waivers` section exists with reason and reviewer approval. |
There was a problem hiding this comment.
This charter adds skip-samples-ci as a recognized skip label / escape hatch, but the repo currently doesn't implement a samples CI gate or reference skip-samples-ci anywhere in workflows (only skip-changelog, skip-exports-check, and large-deletion-approved exist). To keep the charter aligned with actual enforcement, remove skip-samples-ci (or mark samples checks as manual-only until the gate from #103 exists).
| 9. **Waiver documentation**: If any skip label (`skip-changelog`, `skip-exports-check`, `skip-samples-ci`) is present, verify a `## Waivers` section exists with reason and reviewer approval. | |
| 9. **Waiver documentation**: If any skip label (`skip-changelog`, `skip-exports-check`) is present, verify a `## Waivers` section exists with reason and reviewer approval. |
| LABEL --> L3[skip-samples-ci] | ||
| LABEL --> L4[large-deletion-approved] | ||
|
|
||
| FLAG --> F1[SQUAD_CHANGELOG_CHECK] | ||
| FLAG --> F2[SQUAD_EXPORTS_CHECK] | ||
| FLAG --> F3[SQUAD_SAMPLES_CI] |
There was a problem hiding this comment.
The escape-hatch diagram lists skip-samples-ci and SQUAD_SAMPLES_CI, but the current CI workflow only supports skip-changelog, skip-exports-check, and large-deletion-approved (no samples gate/flag exists yet). Please remove these items or clearly label them as planned/future so contributors don’t look for non-existent bypass mechanisms.
| LABEL --> L3[skip-samples-ci] | |
| LABEL --> L4[large-deletion-approved] | |
| FLAG --> F1[SQUAD_CHANGELOG_CHECK] | |
| FLAG --> F2[SQUAD_EXPORTS_CHECK] | |
| FLAG --> F3[SQUAD_SAMPLES_CI] | |
| LABEL --> L3[skip-samples-ci (planned)] | |
| LABEL --> L4[large-deletion-approved] | |
| FLAG --> F1[SQUAD_CHANGELOG_CHECK] | |
| FLAG --> F2[SQUAD_EXPORTS_CHECK] | |
| FLAG --> F3[SQUAD_SAMPLES_CI (planned)] |
| P2[🧪 test\nbuild + test\n+ canary + deletion guard] | ||
| P3[📋 changelog-gate\nSDK/CLI change?\nCHANGELOG updated?] | ||
| P4[📦 exports-map-check\nnew barrel?\nexport in package.json?] | ||
| P5[🎨 samples-build\nbuild all 11 samples\nagainst PR code] |
There was a problem hiding this comment.
The CI pipeline diagram includes a samples-build job, but .github/workflows/squad-ci.yml currently has no samples-build job (and no samples CI gate). This should be updated to match the actual set of CI jobs, or annotated as a future/planned gate (e.g., tracked by #103).
| P5[🎨 samples-build\nbuild all 11 samples\nagainst PR code] | |
| P5[🎨 samples-build (planned)\ntracked in #103\nbuild all 11 samples] |
|
|
||
| | CI Job | What It Checks | | ||
| |--------|---------------| | ||
| | **`docs-quality`** | Markdown formatting (`markdownlint`) + spell check (`cspell`) on docs | |
There was a problem hiding this comment.
This section states docs-quality runs markdownlint + cspell "on docs", but the repo’s markdownlint/cspell configs are currently scoped to docs/src/content/**/*.md and README.md only (so this guide file isn’t checked). Consider tightening the wording to match the actual globs, and/or expanding the globs to include docs/*.md if these files are intended to be covered by docs-quality.
| | **`docs-quality`** | Markdown formatting (`markdownlint`) + spell check (`cspell`) on docs | | |
| | **`docs-quality`** | Markdown formatting (`markdownlint`) + spell check (`cspell`) on docs content (`docs/src/content/**/*.md`) and `README.md` | |
|
@copilot please review this PR. Focus on whether the FIDO charter enforcement rules align correctly with the existing CI gates in squad-ci.yml and the PR requirements spec in .github/PR_REQUIREMENTS.md. |
Reviewed and aligned all three files with the actual CI gates in
|
|
All 8 Copilot review findings have been addressed across commits:
Remaining \skip-samples-ci\ references in the workflow guide and copilot-instructions are explicitly marked as planned/not yet active. Ready for human review. |
Add FIDO routing rules for PR requirements enforcement. Align docs and charter with actual CI gates. Fix review findings. Co-authored-by: tamirdresher <tamirdresher@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🚀 Full Squad Review — feat: enhance FIDO with PR requirements enforcementDomain: quality/fido-enforcement
All 21 squad members reviewed and approved. |
15f7ebd to
c3f058f
Compare
🚀 Squad Team Review — PR #676Major FIDO enhancement: PR requirements enforcement (9 checks), GitHub Workflow Guide (14 chapters, 7 Mermaid diagrams), routing updates. 4 files, +611/-3. |
|
📋 PR Lifecycle: Team review complete. Labeled \squad:pr-reviewed. Waiting for Dina's review. Add \squad:pr-dina-approved\ when ready to proceed. |
…gaster#666, bradygaster#676) (bradygaster#682) * docs(ai-team): CLI UI Polish PRD finalized — 20 issues created Session: 2026-03-01T20-13-00Z-ui-polish-prd Requested by: Brady Changes: - Created 6 orchestration logs (.squad/orchestration-log/2026-03-01T20-24-57Z-*.md) - Created session log (.squad/log/2026-03-01T20-13-00Z-ui-polish-prd.md) - Merged 5 decision files into decisions.md (PRD strategy, cast confirmation, processing state, Brady directives) - Deleted inbox files after merge (deduplication complete) - Updated team history files (Cheritto, Kovash, Redfoot, Marquez, Keaton, Fenster) - PRD created (docs/prd-cli-ui-polish.md) with 20 discrete issues (bradygaster#662-681) for alpha-1 release - Pragmatic alpha-first strategy: P0 blockers (blank screens, spinner, banner) + P1 quick wins, defer grand redesign Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(tui): consolidate separators, fix empty space, add info hierarchy and breathing room (bradygaster#655, bradygaster#670, bradygaster#671, bradygaster#677) - Create shared Separator component in components/Separator.tsx (bradygaster#677) All inline separator rendering (box.h.repeat) replaced across App.tsx, AgentPanel.tsx, and MessageStream.tsx - Remove flexGrow={1} from MessageStream outer Box (bradygaster#655) This was pushing content to bottom of viewport with empty space above - Bold primary CTAs in dim contexts (bradygaster#670) Header: @agent and /help now bold within dimColor usage line First-run: 'Try:' prompt bolded AgentPanel empty state: 'Send a message' and '/help' bolded - Add whitespace breathing room (bradygaster#671) Header wrapper gets marginBottom={1} Turn separators get marginTop={1} AgentPanel bottom separators upgraded from marginTop={0} to marginTop={1} Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: update Cheritto history and add separator decision Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: wire onPermissionRequest handler in CLI session creation (bradygaster#651) Add approveAllPermissions handler to all 4 client.createSession() calls in the CLI shell. The handler was defined in SDK adapter types but never wired, causing a raw SDK error for users. - Add approve-all handler in shell/index.ts (CLI runs locally with user trust) - Export SquadPermissionHandler types from @bradygaster/squad-sdk/client - Add clear error guidance in adapter/client.ts for missing handler Closes bradygaster#651 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(tui): bump secondary text contrast and wire table wrapping (bradygaster#666, bradygaster#676) - Replace dimColor with color="gray" for secondary text (system messages, italic markdown, duration labels, agent activity feed) — higher contrast than dim on dark terminals - Add GRAY ANSI constant and secondary() helper to output.ts - Add wrapTableContent() + truncateTableColumns() to MessageStream — tables exceeding terminal width are column-truncated with ellipsis - Wire table wrapping into both message history and streaming content paths Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Closing as stale. Reason: Stale — FIDO enforcement has evolved significantly since this PR was opened. |
What
Enhances FIDO (Quality Owner) with PR requirements enforcement capabilities, updates routing rules, adds a pre-push quality checklist for @copilot, and adds a comprehensive GitHub workflow guide with visual Mermaid diagrams.
Why
FIDO already owns quality gates and PR blocking authority, but had no knowledge of the PR requirements spec (
.github/PR_REQUIREMENTS.md) introduced in PR #672. The routing table had no entry for PR requirements enforcement work. Meanwhile, the @copilot coding agent had no pre-push quality checklist. This creates a gap where CI catches issues that could have been prevented earlier.Additionally, new contributors need a single-page reference explaining how the repo works — branches, PRs, CI gates, labels, triage, and escape hatches.
Refs #100, #103
How
1. FIDO charter upgrade (
.squad/agents/fido/charter.md)PR Requirements Enforcementsection with 9 specific checks2. Routing rules update (
.squad/routing.md)PR requirements enforcementwork type routed to FIDOsquadinto using GitHub #8: spawn FIDO for PR compliance checks before opening PRs3. Copilot instructions update (
.github/copilot-instructions.md)PR Requirements — Pre-Push Quality Checklistsection with 6 items4. Workflow guide (
docs/GITHUB_WORKFLOW_GUIDE.md)Testing
pm run buildnot applicable (no source code changes)pm testnot applicable (no test changes)Docs
docs/GITHUB_WORKFLOW_GUIDE.md.squad/state files and.github/infrastructure, which are explicitly exempt per PR_REQUIREMENTS.md)Exports
N/A — no new modules
Breaking Changes
None
Waivers
None required — this PR is categorically exempt from Documentation (d), Exports (e), and Samples (f) requirements per the Edge Case Exemptions table in PR_REQUIREMENTS.md (CI/GitHub Actions workflow changes + internal .squad/ state files).