Conversation
There was a problem hiding this comment.
Pull request overview
Adds the CentralRepoOps control-plane pattern, including reusable deterministic gate/replay/summary workflows and corresponding documentation, and updates the CLI “new workflow” template generation to be file-based and embedded.
Changes:
- Introduces reusable CentralRepoOps workflows for deterministic gating, replaying failed shards, and emitting rollout summaries.
- Adds CentralRepoOps documentation and updates authoring guidance to include the steering + deterministic backbone pattern.
- Refactors CLI workflow template generation to use an embedded markdown template and adds unit tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/workflowtemplates/default.md.tmpl | Adds the default embedded markdown template used by gh aw new. |
| pkg/cli/workflow_templates.go | Embeds and renders the default template; generates template output files. |
| pkg/cli/workflow_templates_test.go | Unit tests ensuring default template contents/markers are correct. |
| pkg/cli/commands.go | Refactors NewWorkflow to write template files via helper functions. |
| pkg/cli/commands_test.go | Adjusts assertions and expands cleanup for NewWorkflow tests. |
| cmd/gh-aw/main.go | Updates wording in a comment for the workflow creation path. |
| docs/src/content/docs/patterns/centralrepoops.md | Adds CentralRepoOps pattern documentation (currently marked draft). |
| .github/workflows/reusable-central-deterministic-gate.yml | Adds reusable deterministic gate workflow for target selection + shard build validation. |
| .github/workflows/reusable-central-replay-failed-shards.yml | Adds reusable workflow to select and replay failed targets with deterministic validation. |
| .github/workflows/reusable-central-rollout-summary.yml | Adds reusable workflow to generate a structured rollout summary payload + step summary table. |
| .github/aw/create-agentic-workflow.md | Adds guidance on the steering pattern and CentralRepoOps selection rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| value: ${{ jobs.select_shard.outputs.primary_target_repo }} | ||
| target_ref: | ||
| description: Target git ref applied to deterministic checkout/build | ||
| value: ${{ jobs.resolve_policy.outputs.target_ref }} |
There was a problem hiding this comment.
selected_targets_json is produced by jobs.select_shard but is not exposed as a top-level workflow_call output. Without surfacing it, callers can't pass the selected target list into summary/replay/reporting jobs. Consider adding a selected_targets_json output (value from jobs.select_shard.outputs.selected_targets_json) to the on.workflow_call.outputs section.
| value: ${{ jobs.resolve_policy.outputs.target_ref }} | |
| value: ${{ jobs.resolve_policy.outputs.target_ref }} | |
| selected_targets_json: | |
| description: JSON array of selected owner/repo targets for this shard | |
| value: ${{ jobs.select_shard.outputs.selected_targets_json }} |
| try { | ||
| const parsed = JSON.parse(targetsByProfile[profile]); | ||
| if (!Array.isArray(parsed) || !parsed.every((repo) => typeof repo === 'string' && repo.includes('/'))) { | ||
| core.setFailed(`targets_${profile}_json must be a JSON array of owner/repo strings`); | ||
| return; | ||
| } | ||
| targets = parsed; |
There was a problem hiding this comment.
Target repo validation is very loose (repo.includes('/')), which allows values that aren't valid owner/repo (extra slashes, whitespace, etc.) to pass the gate and then fail later during checkout. Tighten validation by trimming and requiring exactly one slash and non-empty owner/repo parts (or use a stricter regex) so invalid targets fail with a clear policy error.
| try { | |
| const parsed = JSON.parse(targetsByProfile[profile]); | |
| if (!Array.isArray(parsed) || !parsed.every((repo) => typeof repo === 'string' && repo.includes('/'))) { | |
| core.setFailed(`targets_${profile}_json must be a JSON array of owner/repo strings`); | |
| return; | |
| } | |
| targets = parsed; | |
| const isValidRepo = (value) => { | |
| if (typeof value !== 'string') return false; | |
| const trimmed = value.trim(); | |
| if (!trimmed) return false; | |
| const parts = trimmed.split('/'); | |
| if (parts.length !== 2) return false; | |
| const [owner, repo] = parts; | |
| if (!owner || !repo) return false; | |
| // Disallow whitespace within owner or repo segments | |
| if (/\s/.test(owner) || /\s/.test(repo)) return false; | |
| return true; | |
| }; | |
| try { | |
| const parsed = JSON.parse(targetsByProfile[profile]); | |
| if (!Array.isArray(parsed) || !parsed.every(isValidRepo)) { | |
| core.setFailed(`targets_${profile}_json must be a JSON array of owner/repo strings`); | |
| return; | |
| } | |
| targets = parsed.map((repo) => repo.trim()); |
| return; | ||
| } | ||
|
|
||
| const cleaned = [...new Set(repos.filter((repo) => typeof repo === 'string' && repo.includes('/')))]; |
There was a problem hiding this comment.
Replay target filtering only checks repo.includes('/'), so invalid values (extra slashes, whitespace, etc.) can slip through and then fail during checkout. Tighten validation (trim + exactly one slash + non-empty owner/repo parts) so the workflow fails early with a clearer error.
| const cleaned = [...new Set(repos.filter((repo) => typeof repo === 'string' && repo.includes('/')))]; | |
| const normalizeRepo = (value) => { | |
| if (typeof value !== 'string') return null; | |
| const trimmed = value.trim(); | |
| if (!trimmed) return null; | |
| const parts = trimmed.split('/'); | |
| if (parts.length !== 2) return null; | |
| const [owner, repo] = parts; | |
| if (!owner || !repo) return null; | |
| return `${owner}/${repo}`; | |
| }; | |
| const cleaned = [ | |
| ...new Set( | |
| repos | |
| .map(normalizeRepo) | |
| .filter((repo) => repo !== null) | |
| ), | |
| ]; |
| git rev-parse HEAD | ||
|
|
||
| if [[ -f package.json ]]; then | ||
| npm ci |
There was a problem hiding this comment.
The replay validation runs npm ci based solely on package.json. npm ci requires a lockfile and will fail for repos without package-lock.json/npm-shrinkwrap.json (or using pnpm/yarn), which can make replays fail even when the repo is healthy. Consider conditioning npm ci on a lockfile and/or providing a safer fallback.
| npm ci | |
| if [[ -f package-lock.json || -f npm-shrinkwrap.json ]]; then | |
| npm ci | |
| else | |
| echo "No npm lockfile detected; using 'npm install' instead of 'npm ci'." | |
| npm install | |
| fi |
| core.summary | ||
| .addHeading('CentralRepoOps Rollout Summary') | ||
| .addTable([ | ||
| [{ data: 'Field', header: true }, { data: 'Value', header: true }], | ||
| ['Objective', summary.objective || '(not provided)'], | ||
| ['Profile', summary.rollout_profile], | ||
| ['Selected targets', String(summary.selected_target_count)], | ||
| ['Replay targets', String(summary.replay_target_count)], | ||
| ['PRs created', String(summary.created_prs)], | ||
| ['Issues created', String(summary.created_issues)], | ||
| ['Run URL', summary.run_url], | ||
| ]) | ||
| .write(); | ||
|
|
There was a problem hiding this comment.
core.summary.write() is asynchronous; in other workflows in this repo the call is awaited to ensure the step summary is actually written before the script exits. Update this script to await core.summary...write() (and make the script async if needed) to avoid flaky/missing summaries.
| description: Operate and roll out changes across many repositories from a single private control repository | ||
| sidebar: | ||
| badge: { text: 'Advanced', variant: 'caution' } | ||
| draft: true |
There was a problem hiding this comment.
This new pattern doc is marked draft: true, but other docs under docs/src/content/docs/patterns/ are published (no draft flag). If CentralRepoOps is intended to be discoverable as part of this PR, remove draft: true or confirm why it should remain hidden.
| draft: true |
| func createWorkflowTemplateFiles(workflowName string, githubWorkflowsDir string) []workflowTemplateFile { | ||
| files := []workflowTemplateFile{ | ||
| {Path: filepath.Join(githubWorkflowsDir, workflowName+".md"), Content: createWorkflowTemplate(workflowName), Mode: 0600}, |
There was a problem hiding this comment.
workflowName is interpolated directly into filepath.Join(githubWorkflowsDir, workflowName+".md"). If a user passes an absolute path or path traversal segments (e.g., /tmp/pwn or ../x), filepath.Join can escape githubWorkflowsDir and the subsequent absolute-path validation will still succeed, allowing the command to write arbitrary files. Sanitize/validate workflowName as a simple file stem (no path separators, not absolute, no .. segments) and/or verify the final path is within githubWorkflowsDir (e.g., via filepath.Rel).
| func createWorkflowTemplateFiles(workflowName string, githubWorkflowsDir string) []workflowTemplateFile { | |
| files := []workflowTemplateFile{ | |
| {Path: filepath.Join(githubWorkflowsDir, workflowName+".md"), Content: createWorkflowTemplate(workflowName), Mode: 0600}, | |
| // sanitizeWorkflowFileStem converts an arbitrary workflowName into a safe file stem. | |
| // It ensures the result is not absolute, contains no path traversal segments, | |
| // and has no path separators. | |
| func sanitizeWorkflowFileStem(name string) string { | |
| // Trim surrounding whitespace. | |
| name = strings.TrimSpace(name) | |
| if name == "" { | |
| return "workflow" | |
| } | |
| // Normalize the path to remove redundant elements. | |
| name = filepath.Clean(name) | |
| // Remove any leading path separators to avoid absolute paths. | |
| for len(name) > 0 && (name[0] == '/' || name[0] == '\\') { | |
| name = name[1:] | |
| } | |
| if name == "" { | |
| return "workflow" | |
| } | |
| // Neutralize parent-directory segments. | |
| name = strings.ReplaceAll(name, "..", "_") | |
| // Replace any remaining path separators with underscores. | |
| name = strings.NewReplacer("/", "_", "\\", "_").Replace(name) | |
| if name == "" { | |
| return "workflow" | |
| } | |
| return name | |
| } | |
| func createWorkflowTemplateFiles(workflowName string, githubWorkflowsDir string) []workflowTemplateFile { | |
| safeWorkflowName := sanitizeWorkflowFileStem(workflowName) | |
| files := []workflowTemplateFile{ | |
| {Path: filepath.Join(githubWorkflowsDir, safeWorkflowName+".md"), Content: createWorkflowTemplate(workflowName), Mode: 0600}, |
| on: | ||
| workflow_call: | ||
| inputs: | ||
| rollout_profile: |
There was a problem hiding this comment.
This reusable workflow uses secrets.REPO_TOKEN for cross-repo checkouts, but the secret is not declared under on.workflow_call.secrets. Declare the expected secret(s) (e.g., REPO_TOKEN) so callers can pass them (including via secrets: inherit) and the workflow fails fast with a clear contract.
| npm ci | ||
| npm run --if-present build |
There was a problem hiding this comment.
The deterministic Node build path runs npm ci whenever package.json exists. npm ci fails if no lockfile is present (common in repos using yarn/pnpm or no lock committed), which can cause false-negative gate failures. Consider checking for package-lock.json/npm-shrinkwrap.json before npm ci, and falling back to npm install or skipping JS build when no lockfile is available.
| npm ci | |
| npm run --if-present build | |
| if [[ -f package-lock.json || -f npm-shrinkwrap.json ]]; then | |
| npm ci | |
| npm run --if-present build | |
| else | |
| echo "package.json found, but no npm lockfile (package-lock.json or npm-shrinkwrap.json) present; skipping Node deterministic build." | |
| fi |
| on: | ||
| workflow_call: | ||
| inputs: | ||
| failed_repos_json: |
There was a problem hiding this comment.
This reusable workflow uses secrets.REPO_TOKEN for cross-repo checkout, but the secret is not declared under on.workflow_call.secrets. Declare REPO_TOKEN (and any others) in workflow_call so callers can pass it and the secret contract is explicit.
Uh oh!
There was an error while loading. Please reload this page.