Re-resolve package manifests during workflow updates#54119
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This update fixes part of manifest re-resolution, but it still misses newly added action workflows when the package uses the .yaml extension, so gh aw update can leave the checkout out of sync after an upgrade.
Blocking theme
- The new reconciliation path special-cases action workflows with a raw
.ymlsuffix check instead of using the existing workflow-path predicate, so support for.yamldrifts here and valid package assets are skipped.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 13.4 AIC · ⌖ 4.87 AIC · ⊞ 2.7K
Comment /review to run again
| currentInstallables[installable.DestinationPath] = struct{}{} | ||
| } | ||
| for _, installable := range latestPkg.InstallationSource { | ||
| if _, existed := currentInstallables[installable.DestinationPath]; existed || !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".yml") { |
There was a problem hiding this comment.
This update only installs newly added action workflows when the manifest source path ends in .yml, so valid package additions like .github/workflows/release.yaml are silently skipped and gh aw update still leaves the repo incomplete.
💡 Why this is a real regression and how to fix it
The manifest parser already treats .yaml action workflows as unsupported today in some places, but the rest of the CLI and naming helpers explicitly handle both .yml and .yaml. Hard-coding strings.HasSuffix(..., ".yml") here means update behavior now diverges from add/enable behavior and any package that ships .yaml workflow files will never be reconciled after a version bump.
Please reuse the existing action-workflow predicate instead of open-coding the extension check, e.g.:
if _, existed := currentInstallables[installable.DestinationPath]; existed || !isActionWorkflowPath(installable.SourcePath) {
continue
}That keeps the extension policy in one place and prevents future drift.
There was a problem hiding this comment.
Overall: The new reconcileManifestManagedAssets function is well-structured and the test coverage is solid. One correctness issue found: the .yml-only suffix filter silently drops valid .yaml action workflow files.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.5 AIC · ⌖ 8.84 AIC · ⊞ 5.7K
| if err != nil { | ||
| return fmt.Errorf("failed to download new package action workflow %s: %w", installable.SourcePath, err) | ||
| } | ||
| destPath := filepath.Join(gitRoot, filepath.FromSlash(installable.DestinationPath)) |
There was a problem hiding this comment.
The suffix check only accepts .yml but GitHub Actions workflows are equally valid with a .yaml extension. A package providing action workflows named *.yaml will be silently skipped during updates.
// Before
!strings.HasSuffix(strings.ToLower(installable.SourcePath), ".yml")
// After
ext := strings.ToLower(filepath.Ext(installable.SourcePath))
ext != ".yml" && ext != ".yaml"@copilot please address this.
There was a problem hiding this comment.
Ponytail review (over-engineering only).
One finding: the three asset-reconciliation loops in reconcileManifestManagedAssets (action workflows, skills, agents) repeat the same seen-set/skip/download/install shape. A small generic helper would shrink this considerably.
net: -35 lines possible.
Generated by ✂️ Ponytail Reviewer for #54119 · auto · 27.2 AIC · ⌖ 7.97 AIC · ⊞ 7.3K
Comment /ponytail to run again
| return err | ||
| } | ||
|
|
||
| currentInstallables := make(map[string]struct{}, len(currentPkg.InstallationSource)) |
There was a problem hiding this comment.
L216-286: yagni: three near-identical diff-and-download loops (installables/skills/agents) each rebuild a seen-set, skip existing, download, then install. Single generic helper over a slice of {seenKey, download, install} would collapse ~50 lines to ~15.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a few correctness and test coverage issues.
📋 Key Themes & Highlights
Key Themes
- Download-before-check ordering: The
.ymlaction workflow loop downloads the file before verifying the destination doesn't exist, wasting a network call for files that will be skipped. - Partial-failure behaviour: A single download error in the skills/agents loops aborts all remaining installations, potentially leaving the repo half-installed with no summary of what succeeded.
- Test coverage gaps: The skip-if-exists path and file content correctness are untested.
- Silent filter: Non-
.ymlInstallationSourceentries are silently skipped with no log; this may surprise future contributors adding other asset types to the manifest.
Positive Highlights
- ✅ Clean separation into
reconcileManifestManagedAssets— easy to read and test independently. - ✅ Good use of set-membership lookups (
map[string]struct{}) for the "already installed" check. - ✅ Existence guard before overwrite (
os.Statcheck) is the right safe-write pattern. - ✅ Regression test added alongside the new function — good TDD discipline.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 48.6 AIC · ⌖ 10.5 AIC · ⊞ 7.8K
Comment /matt to run again
| if _, existed := currentInstallables[installable.DestinationPath]; existed || !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".yml") { | ||
| continue | ||
| } | ||
| content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repository, installable.SourcePath, latestPkg.ResolvedRef, "") |
There was a problem hiding this comment.
[/diagnosing-bugs] The network download at line 224 happens before the destination existence check — if the file already exists on disk the download was wasted and any transient network error will abort the whole reconcile unnecessarily.
💡 Suggested fix
Move the existence check before the download:
destPath := filepath.Join(gitRoot, filepath.FromSlash(installable.DestinationPath))
if _, err := os.Stat(destPath); err == nil {
updateManifestLog.Printf("Skipping new package action workflow because destination already exists: %s", destPath)
continue
} else if !os.IsNotExist(err) {
return fmt.Errorf("failed to inspect new package action workflow destination %s: %w", destPath, err)
}
content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repository, installable.SourcePath, latestPkg.ResolvedRef, "")@copilot please address this.
| currentInstallables[installable.DestinationPath] = struct{}{} | ||
| } | ||
| for _, installable := range latestPkg.InstallationSource { | ||
| if _, existed := currentInstallables[installable.DestinationPath]; existed || !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".yml") { |
There was a problem hiding this comment.
[/diagnosing-bugs] The .yml filter on installable.SourcePath (line 221) only installs action workflows, silently skipping non-.yml installables (e.g. shell scripts, markdown assets). If the manifest ever includes such entries under InstallationSource, they will be permanently ignored during update with no log or error.
💡 Suggestion
Either document that InstallationSource is intentionally limited to .yml action workflows, or log a debug message when skipping non-.yml entries so operators can diagnose unexpected omissions:
if !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".yml") {
updateManifestLog.Printf("Skipping non-yml installable: %s", installable.SourcePath)
continue
}@copilot please address this.
| assert.FileExists(t, filepath.Join(tmpDir, workflow.GetEngineSkillDir("copilot"), "review", "scripts", "check.sh")) | ||
| assert.FileExists(t, filepath.Join(tmpDir, workflow.GetEngineSubAgentDir("copilot"), "reviewer.md")) | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] The test only asserts that files exist but doesn't verify their content matches what the mock download returned. A content check would catch regressions where the write path silently truncates or transforms the file.
💡 Suggestion
Add content assertions after the FileExists checks:
content, err := os.ReadFile(filepath.Join(tmpDir, ".github", "workflows", "new.yml"))
require.NoError(t, err)
assert.Equal(t, "name: new action\n", string(content))@copilot please address this.
| return fmt.Errorf("failed to download new package action workflow %s: %w", installable.SourcePath, err) | ||
| } | ||
| destPath := filepath.Join(gitRoot, filepath.FromSlash(installable.DestinationPath)) | ||
| if _, err := os.Stat(destPath); err == nil { |
There was a problem hiding this comment.
[/tdd] There's no test for the case where a destination file already exists — the skip-if-exists logic (line 229–231) has zero test coverage, so a future regression that accidentally overwrites existing files would go undetected.
💡 Suggested test case
func TestReconcileManifestManagedAssets_SkipsExistingDestination(t *testing.T) {
tmpDir := testutil.TempDir(t, "manifest-assets-skip-*")
require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".git"), 0o755))
require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".github", "workflows"), 0o755))
existing := filepath.Join(tmpDir, ".github", "workflows", "existing.yml")
require.NoError(t, os.WriteFile(existing, []byte("original"), 0o644))
t.Chdir(tmpDir)
downloadCalled := false
downloadPackageFileFromGitHubForHost = func(...) ([]byte, error) {
downloadCalled = true
return []byte("overwrite"), nil
}
err := reconcileManifestManagedAssets(context.Background(), "owner/repo",
&resolvedRepositoryPackage{},
&resolvedRepositoryPackage{
ResolvedRef: "v2",
InstallationSource: []resolvedPackageInstallable{{
SourcePath: ".github/workflows/existing.yml",
DestinationPath: ".github/workflows/existing.yml",
}},
},
UpdateWorkflowsOptions{},
)
require.NoError(t, err)
assert.False(t, downloadCalled, "should not download when destination already exists")
content, _ := os.ReadFile(existing)
assert.Equal(t, "original", string(content), "existing file should not be overwritten")
}@copilot please address this.
| } | ||
| content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repository, skill.SourcePath, latestPkg.ResolvedRef, "") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to download new package skill %s: %w", skill.SourcePath, err) |
There was a problem hiding this comment.
[/diagnosing-bugs] A single download error in the skills or agents loops causes the entire reconcile to fail (via return err), aborting installation of all subsequent assets. This is an early-exit-on-first-failure pattern that could leave the repository in a partially installed state with no indication of which assets were installed vs skipped.
💡 Consider accumulating errors
Accumulate failures and continue — similar to how updateManifestWorkflowGroup collects failures []updateFailure rather than returning on the first error:
var errs []error
for _, skill := range latestPkg.SkillFiles {
if err := installSkill(...); err != nil {
errs = append(errs, err)
}
}
return errors.Join(errs...)This ensures all installable assets are attempted and the caller gets a complete picture.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Reconciles package manifests during gh aw update so newly added package assets are installed.
Changes:
- Adds reconciliation for action workflows, skills, and agents.
- Preserves existing destination files.
- Adds mixed-asset regression coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/update_manifest.go |
Resolves and installs newly declared package assets. |
pkg/cli/update_manifest_test.go |
Tests mixed package asset installation. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| for _, installable := range currentPkg.InstallationSource { | ||
| currentInstallables[installable.DestinationPath] = struct{}{} | ||
| } | ||
| for _, installable := range latestPkg.InstallationSource { | ||
| if _, existed := currentInstallables[installable.DestinationPath]; existed || !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".yml") { |
| // reconcileManifestManagedAssets installs package-owned action workflows, skills, and | ||
| // agents that were added to the latest manifest. These assets do not carry source | ||
| // frontmatter, so their package ownership is derived from the package manifest itself. | ||
| func reconcileManifestManagedAssets(ctx context.Context, repo string, currentPkg, latestPkg *resolvedRepositoryPackage, opts UpdateWorkflowsOptions) error { |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (140 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel ReportScore: 100/100 ✅ ExcellentSummary: 1 new behavioral test added with strong coverage of the new Key Metrics
Test DetailsTestReconcileManifestManagedAssets_AddsPackageOwnedAssets (48 lines, new)Design Invariant: Verifies that Classification:
Assertions & Coverage:
Mocking Strategy:
Line Coverage:
Compliance Status✅ All checks passed
Next Steps
|
|
@copilot This PR looks close, but it still needs a maintainer-facing finish pass.
Run: https://github.com/github/gh-aw/actions/runs/32337181772
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in Handoff:
Remaining risk: check status is currently stale/unverified for the new head until maintainer-triggered CI runs. |
PR TriageCategory: feature | Risk: medium | Priority: medium (score 55/100)
|
PR TriageCategory: Approved by automated reviewer. Adds manifest re-resolution capability with no deletions (net-new, low regression risk). Automated triage — see the [triage report issue] for full context.
|
gh aw updatepreviously updated only already-installed workflows withsource:fields. Newaw.ymlincludes—such as action workflows, skills, and agents—were silently omitted.Example newly recognized manifest additions:
gh aw updatedoes not install new includes added to anaw.yml#54075