Generate init/upgrade dispatcher file lists from github/gh-aw with embedded fallback#36493
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
.github/aw directories in init/upgrade dispatcher generation
There was a problem hiding this comment.
Pull request overview
This PR fixes gh aw init / gh aw upgrade failing when .github/aw exists but contains no *.md files (e.g., only actions-lock.json and logs/). It updates dispatcher skill generation to treat that state as valid and adds both unit and integration coverage to prevent regressions.
Changes:
- Treat “
.github/awexists but has no markdown files” as a valid state by generating the same minimal dispatcher skill content used when the directory is absent. - Add unit test coverage to assert empty-
.md.github/awproduces identical dispatcher content to the no-directory case. - Add an integration test that runs
initandupgradeagainst a repo with.github/aw/actions-lock.jsonand.github/aw/logs/, ensuring both commands succeed and preserve those artifacts.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/upgrade_integration_test.go | Adds an integration test covering init + upgrade behavior when .github/aw contains only non-markdown artifacts. |
| pkg/cli/copilot_agents.go | Updates dispatcher skill generation to fall back to minimal content when .github/aw exists but has no *.md files. |
| pkg/cli/copilot_agents_test.go | Adds unit coverage asserting empty-.md .github/aw matches the no-directory generated dispatcher content. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36493 does not have the 'implementation' label and has 82 new lines (≤100 threshold) in business logic directories. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with one minor style suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause correctly addressed: The premature error return in
buildAgenticWorkflowsSkillContentwas the exact failure point; the fix targets it directly without over-engineering. - Fallback semantics are clear: Empty
.github/aw→ same minimal output as no.github/awat all. This is the right mental model and the comment in the code explains it well. - Coverage is solid: Unit test asserts output equality with the no-dir case and placeholder replacement; integration test exercises the full
init → upgradecycle and verifies non-markdown artifact preservation.
Positive Highlights
- ✅ One-line fix with zero unrelated changes — surgical and easy to reason about.
- ✅ Integration test covers a realistic repository state (logs dir + lock file) and both commands.
- ✅ Good use of
requirefor setup assertions so failures are reported clearly.
Minor Observation
- One trailing
t.Fatalfin the new unit test could useassert.NotContainsfor consistency (inline comment posted).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1M
| expected, err := buildAgenticWorkflowsSkillContent(withoutAWDir) | ||
| require.NoError(t, err, "buildAgenticWorkflowsSkillContent() without .github/aw directory returned error") | ||
| assert.Equal(t, expected, content, "skill content with empty .github/aw should match content without .github/aw") | ||
| if strings.Contains(content, agenticWorkflowsSkillFileListPlaceholder) { |
There was a problem hiding this comment.
[/tdd] Minor style nit: this trailing t.Fatalf check is inconsistent with the testify imports added just above, and the assert.Equal check already implicitly validates the placeholder is absent.
💡 Suggestion
Replace with assert.NotContains:
assert.NotContains(t, content, agenticWorkflowsSkillFileListPlaceholder,
"expected placeholder to be replaced in skill content")Keeps assertion style uniform throughout the test.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
There was a problem hiding this comment.
Non-blocking observations only — no blockers found.
### Review summary
The fix is correct and the motivation is sound: .github/aw legitimately exists in repos that use non-markdown artifacts (lock files, log dirs) before any workflow .md files are authored, so failing hard there is too strict.
Two non-blocking concerns flagged inline:
-
Silent fallback (medium) — the new empty-dir path produces no warning, so users who accidentally misname their workflow files get no feedback. A
console.FormatWarningMessagecall before the return would preserve the fix without losing discoverability. -
Duplicated return expression (low) — the
len(awFiles)==0path is byte-for-byte identical to theos.IsNotExistpath above it. A tinyminimalSkillContent()helper would make the shared intent explicit and reduce future drift risk.
Tests are solid: the new unit test correctly compares empty-dir output against no-dir output, and the integration test covers the full init→upgrade lifecycle with a real binary.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 154.9K
| return "", fmt.Errorf("no markdown files found in %s - ensure .github/aw contains workflow documentation files", awRoot) | ||
| // .github/aw may exist for non-markdown artifacts (e.g. actions-lock.json, logs/). | ||
| // Emit a minimal skill without an explicit file list in that case. | ||
| return strings.Replace(agenticWorkflowsSkillTemplate, agenticWorkflowsSkillFileListPlaceholder, "", 1), nil |
There was a problem hiding this comment.
Silent degradation hides user misconfiguration: when .github/aw exists but has no .md files, the code silently returns a minimal skill — indistinguishable from the "directory absent" case — with no log or warning.
💡 Why this matters and a suggested fix
A user who accidentally misnamed workflow docs (e.g. workflow.yaml instead of workflow.md, or placed files one directory too deep) will get no signal at all. The original fmt.Errorf was the only indicator that the directory is present but useless.
Since callers already use console.FormatWarningMessage for similar info-messages, emitting a non-fatal warning preserves the new behavior while still alerting users:
if len(awFiles) == 0 {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(
".github/aw exists but contains no markdown workflow files — emitting minimal skill"))
return strings.Replace(agenticWorkflowsSkillTemplate, agenticWorkflowsSkillFileListPlaceholder, "", 1), nil
}This keeps the fix non-breaking while restoring discoverability for misconfigured repos.
| return "", fmt.Errorf("no markdown files found in %s - ensure .github/aw contains workflow documentation files", awRoot) | ||
| // .github/aw may exist for non-markdown artifacts (e.g. actions-lock.json, logs/). | ||
| // Emit a minimal skill without an explicit file list in that case. | ||
| return strings.Replace(agenticWorkflowsSkillTemplate, agenticWorkflowsSkillFileListPlaceholder, "", 1), nil |
There was a problem hiding this comment.
Duplicate code paths are semantically indistinguishable: the new len(awFiles) == 0 return (line 174) is byte-for-byte identical to the os.IsNotExist return at line 157. Two distinct states — directory absent vs directory present but devoid of markdown — now produce the same output with no way to differentiate them.
💡 Suggested refactor
Extract a shared helper to make intent explicit and reduce duplication:
func minimalSkillContent() string {
return strings.Replace(agenticWorkflowsSkillTemplate, agenticWorkflowsSkillFileListPlaceholder, "", 1)
}Then both call sites become return minimalSkillContent(), nil, and a future reader immediately understands this is an intentional shared code path rather than an accidental copy.
|
@copilot review all comments and review comments. apply copilot-review skill. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 2815e20. I reviewed all PR comments/review comments and applied the actionable items: added a shared minimal-skill helper, emit a warning when |
|
@copilot the file list should be based on .github/aw in the github/gh-aw respository folder - NOT THE LOCAL FILES!!!!! Fetch the file list dynamically and bail out to a precomputed list of files into an embedded file in the compiler if connection fails. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 3d70182. Dispatcher file-list generation now fetches |
.github/aw directories in init/upgrade dispatcher generationinit/upgrade dispatcher file lists from github/gh-aw with embedded fallback
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in ce381ba. I investigated the referenced CI job, reproduced the golint errors, and fixed them by removing the unused |
gh aw initandgh aw upgradepreviously generated dispatcher file lists from local.github/awcontents. This caused incorrect behavior when local repository state did not match the canonicalgithub/gh-awworkflow prompt set.This change updates dispatcher generation to use the canonical source and remain resilient when network access fails.
Canonical
.github/awsource for dispatcher generationbuildAgenticWorkflowsSkillContentto fetch markdown workflow file names dynamically fromgithub/gh-aw/.github/aw(GitHub Contents API onmain)..github/awfiles.Embedded fallback when remote fetch fails
pkg/cli/data/agentic_workflows_fallback_aw_files.json).Test updates
init/upgradebehavior with.github/awnon-markdown artifacts (e.g.actions-lock.json,logs/) to ensure commands still succeed and preserve those files.