Add frontmatter formatting command - #59971
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 Thanks for this contribution,
This aligns perfectly with the project's focus on agentic workflow tooling. The PR is ready for review by the team. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add fuzz tests |
There was a problem hiding this comment.
🟡 Changes recommended
--dir formatting currently includes Markdown files without YAML frontmatter (e.g. .github/workflows/shared/*.md), causing the command to fail instead of skipping non-workflow files.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new gh aw format development command to apply registered codemods and canonicalize agentic workflow YAML frontmatter (indentation + deterministic ordering) while preserving comments and the Markdown body.
Changes:
- Introduces
pkg/cli/format_command.goimplementing the formatter (codemods + YAML normalization + ordering). - Wires the new
formatcommand into the CLI root command and command grouping. - Adds unit tests covering formatting behavior, ordering, block scalar safety, and basic CLI wiring.
File summaries
| File | Description |
|---|---|
| pkg/cli/format_command.go | Implements gh aw format: file resolution, codemod application, frontmatter normalization + key ordering. |
| pkg/cli/format_command_test.go | Adds tests for codemod application, comment preservation, ordering, block scalar handling, and command wiring. |
| cmd/gh-aw/main.go | Registers the new format command and assigns it to the “development” command group. |
| cmd/gh-aw/command_groups_test.go | Verifies format is grouped under “development”. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if len(workflowIDs) == 0 { | ||
| return getMarkdownWorkflowFiles(workflowDir) | ||
| } |
| func TestNewFormatCommand(t *testing.T) { | ||
| t.Parallel() | ||
| cmd := NewFormatCommand() | ||
|
|
||
| assert.Equal(t, "format", cmd.Name()) | ||
| assert.NotNil(t, cmd.Flags().Lookup("dir")) | ||
| assert.NotNil(t, cmd.ValidArgsFunction) | ||
| } |
|
✅ PR Code Quality Reviewer completed the code quality review. No additional PR write was needed after the grumpy-coder finished: its only finding duplicated an already-covered concern about bulk format behavior on non-workflow markdown.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
🏗️ ADR Required - draft added for PR #59971I enforced the design-decision gate for this PR because the prefetch summary shows 456 additions in default business-logic directories, which is above the 100-line threshold. Evidence reviewed
OutcomeI generated and committed a draft ADR at:
Next action for the authorPlease review and refine the draft ADR so it accurately captures the intended long-term architectural decision, trade-offs, and alternatives for introducing Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Reviewed with Impeccable harden/audit guidance adapted for CLI correctness/reliability (this PR is a CLI feature, not a UI change, so most UI-specific modes don't apply).
Key finding: gh aw format writes files unconditionally on every invocation, with no dry-run-by-default / --write gate like the sibling fix command has. Given it reorders YAML keys and re-indents frontmatter across potentially the whole .github/workflows tree, this is a meaningful reliability/safety gap worth addressing before merge. A second reviewer comment already flagged that formatting all *.md files in a custom --dir (including non-workflow markdown) will fail since frontmatter is required — also valid and should be fixed.
No blocking security issues found; using COMMENT since these are correctness/UX concerns rather than security-blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 52.7 AIC · ⌖ 13.6 AIC · ⊞ 8.4K
| } | ||
| return files, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
format writes changed files unconditionally with no --write/dry-run gate, unlike the sibling fix command (fix defaults to dry-run and requires --write to modify files, see pkg/cli/fix_command.go). Running gh aw format (e.g. accidentally, or in a script/CI step) will silently rewrite every matching workflow file's frontmatter with no way to preview the diff first. This is a real risk for a command that reorders YAML keys and re-indents content across an entire .github/workflows tree.
Please add a dry-run-by-default mode (print which files would change, similar to fix) with an explicit --write flag to apply changes, for consistency with fix and to avoid unexpected mass-editing of workflow files.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design on the new gh aw format command.
📋 Key Themes & Highlights
Key Themes
- Error handling diverges from
fixcommand's proven pattern:runFormatCommandreturns on the first file error instead of accumulating errors/guided-errors and reporting a summary (asrunFixCommanddoes). Combined with the existing unresolved bot comment that no-args/--dirformats all*.mdfiles (not just workflow files), a single non-workflow or malformed file can silently abort formatting the rest of the batch. - Test coverage gap: no test locks in the desired multi-file error behavior (partial success vs. full abort), so this regression risk isn't caught by CI.
- Existing unresolved review comments from Copilot already flag the
--dir-without-args scope issue and a related untested failure mode — these should be resolved before merge, not just left open.
Positive Highlights
- ✅ Comprehensive roundtrip/idempotency tests (
TestFormatWorkflowContentRoundTrips) are a great TDD practice for a formatter — running twice and asserting equality is exactly the right property to test. - ✅ Careful handling of block scalars so an embedded
---inside a YAML literal block isn't mistaken for the frontmatter delimiter (TestNormalizeFrontmatterDoesNotTreatBlockScalarAsDelimiter) — good edge-case thinking. - ✅ Clean reuse of the existing
Codemod/GuidedErrorinfrastructure fromfix_codemods.gorather than reinventing it.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 62 AIC · ⌖ 15.2 AIC · ⊞ 10.3K
Comment /matt to run again
|
|
||
| codemods := GetAllCodemods() | ||
| for _, file := range files { | ||
| if err := formatWorkflowFile(file, codemods, verbose); err != nil { |
There was a problem hiding this comment.
[/codebase-design] Unlike runFixCommand (which processes every file, tallies successes/failures, and reports a summary even when some files error), runFormatCommand aborts on the very first file that errors and silently skips the rest. When formatting --dir or all workflows, one bad file (e.g. missing frontmatter, guided-error codemod) means no other file in the batch gets formatted, with no summary of what happened.
💡 Suggested fix
Follow the fix_command.go pattern: accumulate per-file errors (and GuidedErrors) in the loop, keep processing remaining files, and report a summary (e.g. "Formatted N of M files, X failed") instead of returning on the first failure.
var totalFormatted, totalErrors int
for _, file := range files {
if err := formatWorkflowFile(file, codemods, verbose); err != nil {
fmt.Fprintf(os.Stderr, "%s\n", console.FormatErrorMessage(fmt.Sprintf("Error formatting %s: %v", filepath.Base(file), err)))
totalErrors++
continue
}
totalFormatted++
}This is especially important combined with the other reviewer's comment that --dir/no-args formats all *.md files, including non-workflow markdown — a single non-workflow file will currently abort the whole run.
@copilot please address this.
| "bbb:", | ||
| "zzz:", | ||
| } | ||
| previous := -1 |
There was a problem hiding this comment.
[/tdd] TestRunFormatFormatsSelectedWorkflow only covers the happy path for a single file. Given the loop-abort behavior flagged above, there's no test asserting what happens when RunFormat is called with multiple WorkflowIDs and one file fails (e.g. missing frontmatter) — currently the whole batch aborts and any already-formatted files are silently left formatted with no reported summary.
💡 Suggested test
func TestRunFormatContinuesAfterFileError(t *testing.T) {
t.Parallel()
tempDir := t.TempDir()
good := filepath.Join(tempDir, "good.md")
bad := filepath.Join(tempDir, "bad.md")
require.NoError(t, os.WriteFile(good, []byte("---\nengine: copilot\non:\n workflow_dispatch:\n---\n# Body"), 0o644))
require.NoError(t, os.WriteFile(bad, []byte("# no frontmatter"), 0o644))
err := RunFormat(FormatConfig{WorkflowIDs: []string{good, bad}, WorkflowDir: tempDir})
// Decide/assert desired behavior: does `good` still get formatted even though `bad` fails?
require.Error(t, err)
}This red test would surface the current abort-on-first-error behavior and let the author decide (and lock in) whether it's intentional, per the accompanying [/codebase-design] comment.
@copilot please address this.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
The new formatter's file discovery is still wrong for bulk runs: it is too permissive at the top level and too shallow for the actual workflow tree, so gh aw format does not reliably normalize the set of workflow files it claims to handle.
Details
The blocking issue from this pass is that the no-arg path only scans workflowDir/*.md, which skips imported workflows under .github/workflows/shared/**. That means a repo-wide format run leaves a large chunk of the workflow corpus stale.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 52 AIC · ⌖ 5.67 AIC · ⊞ 22.9K
Comment /review to run again
| } | ||
|
|
||
| if len(workflowIDs) == 0 { | ||
| return getMarkdownWorkflowFiles(workflowDir) |
There was a problem hiding this comment.
This bulk path only scans workflowDir/*.md, so gh aw format silently skips imported workflows under .github/workflows/shared/** and never normalizes most of the workflow tree.
💡 Why this blocks merge
The command help promises that running gh aw format with no explicit workflow IDs formats all workflow markdown, but getMarkdownWorkflowFiles is a non-recursive glob. In this repo that leaves a large set of shared/imported workflows untouched, including files under shared/mcp/. I verified the current implementation formats a top-level file while leaving shared/mcp/nested.md unchanged in the same run.
Please switch discovery here to the same kind of recursive workflow traversal we use for directory expansion, then filter to actual workflow files before formatting so the bulk command covers the full workflow tree instead of just the root directory.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot reuse the same function as gh aw compile to discover the agentic workflows to format. Do not reimplement. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 5d093a7. Directory formatting now reuses compile's
Fixed in 5d093a7. The full |
|
🎉 This pull request is included in a new release. Release: |
Adds
gh aw formatto canonicalize agentic workflow frontmatter while preserving comments and Markdown content.Changes