From 2e9386d4f8b6134d9c50527772670401b8020c1b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 03:16:14 +0000 Subject: [PATCH 1/8] Initial plan From bf5b3e98271f3f33be396f333f7d948d47067247 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 03:27:01 +0000 Subject: [PATCH 2/8] fix(update): refresh package assets and plugin refs during update Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/update_actions_content_refs.go | 47 ++++++++++++--- pkg/cli/update_actions_content_refs_test.go | 47 +++++++++++++++ pkg/cli/update_actions_workflow_files.go | 11 +++- pkg/cli/update_manifest.go | 32 ++++------ pkg/cli/update_manifest_test.go | 65 +++++++++++++++++++++ 5 files changed, 169 insertions(+), 33 deletions(-) diff --git a/pkg/cli/update_actions_content_refs.go b/pkg/cli/update_actions_content_refs.go index a087582700d..d86b230c2e3 100644 --- a/pkg/cli/update_actions_content_refs.go +++ b/pkg/cli/update_actions_content_refs.go @@ -23,17 +23,43 @@ func updateSkillRefsInContent(ctx context.Context, content string, allowMajor, v return updateSkillRefsInContentWithResolver(ctx, content, allowMajor, verbose, coolDown, resolveLatestRef) } +func updatePluginRefsInContent(ctx context.Context, content string, allowMajor, verbose bool, coolDown time.Duration) (bool, string, error) { + return updatePluginRefsInContentWithResolver(ctx, content, allowMajor, verbose, coolDown, resolveLatestRef) +} + func updateSkillRefsInContentWithResolver( ctx context.Context, content string, allowMajor, verbose bool, coolDown time.Duration, resolver skillRefUpdateResolver, +) (bool, string, error) { + return updateFrontmatterRepoRefsInContentWithResolver(ctx, content, "skills", "skill", allowMajor, verbose, coolDown, resolver) +} + +func updatePluginRefsInContentWithResolver( + ctx context.Context, + content string, + allowMajor, verbose bool, + coolDown time.Duration, + resolver skillRefUpdateResolver, +) (bool, string, error) { + return updateFrontmatterRepoRefsInContentWithResolver(ctx, content, "plugins", "", allowMajor, verbose, coolDown, resolver) +} + +func updateFrontmatterRepoRefsInContentWithResolver( + ctx context.Context, + content string, + fieldName string, + objectKey string, + allowMajor, verbose bool, + coolDown time.Duration, + resolver skillRefUpdateResolver, ) (bool, string, error) { result, err := parser.ExtractFrontmatterFromContent(content) if err != nil { if verbose { - updateLog.Printf("Skipping skill update for content without parseable frontmatter: %v", err) + updateLog.Printf("Skipping %s update for content without parseable frontmatter: %v", fieldName, err) } return false, content, nil } @@ -41,25 +67,28 @@ func updateSkillRefsInContentWithResolver( return false, content, nil } - rawSkills, ok := result.Frontmatter["skills"].([]any) - if !ok || len(rawSkills) == 0 { + rawRefs, ok := result.Frontmatter[fieldName].([]any) + if !ok || len(rawRefs) == 0 { return false, content, nil } changed := false - for i, rawSkill := range rawSkills { - switch typed := rawSkill.(type) { + for i, rawRef := range rawRefs { + switch typed := rawRef.(type) { case string: updated, updatedRef, err := updateSkillRefValue(ctx, typed, allowMajor, verbose, coolDown, resolver) if err != nil { return false, content, err } if updated { - rawSkills[i] = updatedRef + rawRefs[i] = updatedRef changed = true } case map[string]any: - skillRef, ok := typed["skill"].(string) + if objectKey == "" { + continue + } + skillRef, ok := typed[objectKey].(string) if !ok { continue } @@ -68,7 +97,7 @@ func updateSkillRefsInContentWithResolver( return false, content, err } if updated { - typed["skill"] = updatedRef + typed[objectKey] = updatedRef changed = true } } @@ -76,7 +105,7 @@ func updateSkillRefsInContentWithResolver( if !changed { return false, content, nil } - result.Frontmatter["skills"] = rawSkills + result.Frontmatter[fieldName] = rawRefs updatedFrontmatter, err := yaml.Marshal(result.Frontmatter) if err != nil { diff --git a/pkg/cli/update_actions_content_refs_test.go b/pkg/cli/update_actions_content_refs_test.go index 6b0c3d58dc5..dafa2da8661 100644 --- a/pkg/cli/update_actions_content_refs_test.go +++ b/pkg/cli/update_actions_content_refs_test.go @@ -220,6 +220,53 @@ func TestUpdateSkillRefsInContentWithResolver_NoFrontmatterNoChange(t *testing.T } } +func TestUpdatePluginRefsInContentWithResolver_UpdatesPluginRefs(t *testing.T) { + oldRepoPluginSHA := "1111111111111111111111111111111111111111" + oldPathPluginSHA := "2222222222222222222222222222222222222222" + newRepoPluginSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + newPathPluginSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + input := `--- +name: test +plugins: + - githubnext/plugins@` + oldRepoPluginSHA + ` + - githubnext/plugins/review/security@` + oldPathPluginSHA + ` + - ${{ inputs.dynamic_plugin }} +--- +body +` + + resolver := func(_ context.Context, repo, currentRef string, allowMajor, verbose bool, coolDown time.Duration) (string, error) { + if repo != "githubnext/plugins" { + t.Fatalf("resolver called with repo %q, want githubnext/plugins", repo) + } + switch currentRef { + case oldRepoPluginSHA: + return newRepoPluginSHA, nil + case oldPathPluginSHA: + return newPathPluginSHA, nil + default: + return currentRef, nil + } + } + + changed, got, err := updatePluginRefsInContentWithResolver(context.Background(), input, true, false, 0, resolver) + if err != nil { + t.Fatalf("updatePluginRefsInContentWithResolver() error = %v", err) + } + if !changed { + t.Fatal("updatePluginRefsInContentWithResolver() changed = false, want true") + } + if !strings.Contains(got, "githubnext/plugins@"+newRepoPluginSHA) { + t.Fatalf("updated content missing updated repo plugin ref:\n%s", got) + } + if !strings.Contains(got, "githubnext/plugins/review/security@"+newPathPluginSHA) { + t.Fatalf("updated content missing updated path plugin ref:\n%s", got) + } + if !strings.Contains(got, "- ${{ inputs.dynamic_plugin }}") { + t.Fatalf("updated content unexpectedly modified expression plugin ref:\n%s", got) + } +} + // TestUpdateActionRefsInContent_CooldownFallback verifies that // updateActionRefsInContentWithDeps falls back to an older cooled-down release // when the newest candidate is still within the cooldown window. diff --git a/pkg/cli/update_actions_workflow_files.go b/pkg/cli/update_actions_workflow_files.go index d5c1191a721..b3a1c682adb 100644 --- a/pkg/cli/update_actions_workflow_files.go +++ b/pkg/cli/update_actions_workflow_files.go @@ -96,8 +96,15 @@ func updateActionsInWorkflowFiles(ctx context.Context, deps actionUpdateDeps, op } return nil } + updatedPlugins, newContent, err := updatePluginRefsInContent(ctx, newContent, !opts.disableReleaseBump, opts.verbose, opts.coolDown) + if err != nil { + if opts.verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update plugin refs in %s: %v", path, err))) + } + return nil + } - if !updatedActions && !updatedSkills { + if !updatedActions && !updatedSkills && !updatedPlugins { return nil } @@ -105,7 +112,7 @@ func updateActionsInWorkflowFiles(ctx context.Context, deps actionUpdateDeps, op return fmt.Errorf("failed to write updated workflow %s: %w", path, err) } - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated action/skill references in "+d.Name())) + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated action/skill/plugin references in "+d.Name())) updatedFiles = append(updatedFiles, path) return nil }) diff --git a/pkg/cli/update_manifest.go b/pkg/cli/update_manifest.go index 4db9bd18816..e15d176edb3 100644 --- a/pkg/cli/update_manifest.go +++ b/pkg/cli/update_manifest.go @@ -227,9 +227,9 @@ func reconcileManifestManagedAssets(ctx context.Context, repo string, _ *resolve continue } destPath := filepath.Join(gitRoot, filepath.FromSlash(installable.DestinationPath)) + fileExists := false if _, err := os.Stat(destPath); err == nil { - updateManifestLog.Printf("Skipping new package action workflow because destination already exists: %s", destPath) - continue + fileExists = true } else if !os.IsNotExist(err) { return fmt.Errorf("failed to inspect new package action workflow destination %s: %w", destPath, err) } @@ -243,20 +243,18 @@ func reconcileManifestManagedAssets(ctx context.Context, repo string, _ *resolve if err := os.WriteFile(destPath, content, constants.FilePermPublic); err != nil { return fmt.Errorf("failed to install new package action workflow %s: %w", installable.DestinationPath, err) } - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Added package action workflow: "+filepath.Base(destPath))) + if fileExists { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated package action workflow: "+filepath.Base(destPath))) + } else { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Added package action workflow: "+filepath.Base(destPath))) + } } for _, skill := range latestPkg.SkillFiles { - destPath, err := packageSkillDestinationPath(gitRoot, skill, engineOverride) + _, err := packageSkillDestinationPath(gitRoot, skill, engineOverride) if err != nil { return err } - if _, err := os.Stat(destPath); err == nil { - updateManifestLog.Printf("Skipping new package skill because destination already exists: %s", destPath) - continue - } else if !os.IsNotExist(err) { - return fmt.Errorf("failed to inspect new package skill destination %s: %w", destPath, err) - } 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) @@ -270,25 +268,19 @@ func reconcileManifestManagedAssets(ctx context.Context, repo string, _ *resolve if err := addSkillFileWithTracking(resolved, nil, AddOptions{ EngineOverride: engineOverride, Quiet: false, + Force: true, }, gitRoot); err != nil { return fmt.Errorf("failed to install new package skill %s: %w", skill.SourcePath, err) } } for _, agent := range latestPkg.AgentFiles { - destPath := packageAgentDestinationPath(gitRoot, agent, engineOverride) - if _, err := os.Stat(destPath); err == nil { - updateManifestLog.Printf("Skipping new package agent because destination already exists: %s", destPath) - continue - } else if !os.IsNotExist(err) { - return fmt.Errorf("failed to inspect new package agent destination %s: %w", destPath, err) - } content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repository, agent, latestPkg.ResolvedRef, "") if err != nil { return fmt.Errorf("failed to download new package agent %s: %w", agent, err) } resolved := &ResolvedWorkflow{Content: content, Spec: &WorkflowSpec{WorkflowPath: agent}, IsPackageAgentFile: true} - if err := addAgentFileWithTracking(resolved, nil, AddOptions{EngineOverride: engineOverride}, gitRoot); err != nil { + if err := addAgentFileWithTracking(resolved, nil, AddOptions{EngineOverride: engineOverride, Force: true}, gitRoot); err != nil { return fmt.Errorf("failed to install new package agent %s: %w", agent, err) } } @@ -324,10 +316,6 @@ func packageSkillDestinationPath(gitRoot string, skill resolvedPackageSkillFile, return filepath.Join(gitRoot, workflow.GetEngineSkillDir(engineOverride), skill.SkillName, relPath), nil } -func packageAgentDestinationPath(gitRoot, sourcePath, engineOverride string) string { - return filepath.Join(gitRoot, workflow.GetEngineSubAgentDir(engineOverride), filepath.Base(sourcePath)) -} - func removeManifestManagedWorkflow(workflowPath string) error { updateManifestLog.Printf("Removing manifest-managed workflow no longer in manifest: %s", filepath.Base(workflowPath)) if err := os.Remove(workflowPath); err != nil && !os.IsNotExist(err) { diff --git a/pkg/cli/update_manifest_test.go b/pkg/cli/update_manifest_test.go index dd23b7b52bd..90434b2d59b 100644 --- a/pkg/cli/update_manifest_test.go +++ b/pkg/cli/update_manifest_test.go @@ -110,6 +110,71 @@ func TestReconcileManifestManagedAssets_BranchTrackingInstallsMissingAssets(t *t assert.FileExists(t, filepath.Join(tmpDir, workflow.GetEngineSubAgentDir("copilot"), "reviewer.md")) } +func TestReconcileManifestManagedAssets_RefreshesExistingPackageOwnedAssets(t *testing.T) { + tmpDir := testutil.TempDir(t, "manifest-assets-refresh-*") + require.NoError(t, os.Mkdir(filepath.Join(tmpDir, ".git"), 0o755)) + t.Chdir(tmpDir) + + engine := "copilot" + existingWorkflowPath := filepath.Join(tmpDir, ".github", "workflows", "new.yml") + existingSkillPath := filepath.Join(tmpDir, workflow.GetEngineSkillDir(engine), "review", "scripts", "check.sh") + existingAgentPath := filepath.Join(tmpDir, workflow.GetEngineSubAgentDir(engine), "reviewer.md") + require.NoError(t, os.MkdirAll(filepath.Dir(existingWorkflowPath), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Dir(existingSkillPath), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Dir(existingAgentPath), 0o755)) + require.NoError(t, os.WriteFile(existingWorkflowPath, []byte("name: old action\n"), 0o644)) + require.NoError(t, os.WriteFile(existingSkillPath, []byte("#!/bin/sh\necho old\n"), 0o644)) + require.NoError(t, os.WriteFile(existingAgentPath, []byte("# Old Reviewer\n"), 0o644)) + + originalDownload := downloadPackageFileFromGitHubForHost + t.Cleanup(func() { downloadPackageFileFromGitHubForHost = originalDownload }) + downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { + if owner != "owner" || repo != "repo" || ref != "v2.0.0" { + return nil, fmt.Errorf("unexpected package source %s/%s@%s", owner, repo, ref) + } + switch path { + case ".github/workflows/new.yml": + return []byte("name: new action\n"), nil + case "skills/review/scripts/check.sh": + return []byte("#!/bin/sh\necho new\n"), nil + case "agents/reviewer.md": + return []byte("# New Reviewer\n"), nil + default: + return nil, fmt.Errorf("unexpected package path %s", path) + } + } + + err := reconcileManifestManagedAssets(context.Background(), "owner/repo", + &resolvedRepositoryPackage{}, + &resolvedRepositoryPackage{ + ResolvedRef: "v2.0.0", + InstallationSource: []resolvedPackageInstallable{{ + SourcePath: ".github/workflows/new.yml", + DestinationPath: ".github/workflows/new.yml", + }}, + SkillFiles: []resolvedPackageSkillFile{{ + SourcePath: "skills/review/scripts/check.sh", + SkillName: "review", + }}, + AgentFiles: []string{"agents/reviewer.md"}, + }, + engine, + ) + require.NoError(t, err) + + workflowContent, readErr := os.ReadFile(existingWorkflowPath) + require.NoError(t, readErr) + assert.Equal(t, "name: new action\n", string(workflowContent)) + + skillContent, readErr := os.ReadFile(existingSkillPath) + require.NoError(t, readErr) + assert.Equal(t, "#!/bin/sh\necho new\n", string(skillContent)) + + agentContent, readErr := os.ReadFile(existingAgentPath) + require.NoError(t, readErr) + assert.Equal(t, "# New Reviewer\n", string(agentContent)) +} + func TestResolveManifestAssetEngine(t *testing.T) { tmpDir := testutil.TempDir(t, "manifest-assets-engine-*") workflowPath := filepath.Join(tmpDir, "existing.md") From 1497e8e82dd8410540cb83a38fd5739c9ada3114 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 21 Aug 2026 05:33:58 +0000 Subject: [PATCH 3/8] docs(adr): add draft ADR-54417 for force-refresh package-managed assets on update --- ...efresh-package-managed-assets-on-update.md | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 docs/adr/54417-force-refresh-package-managed-assets-on-update.md diff --git a/docs/adr/54417-force-refresh-package-managed-assets-on-update.md b/docs/adr/54417-force-refresh-package-managed-assets-on-update.md new file mode 100644 index 00000000000..cd29bbd9188 --- /dev/null +++ b/docs/adr/54417-force-refresh-package-managed-assets-on-update.md @@ -0,0 +1,47 @@ +# ADR-54417: Force-Refresh Package-Managed Assets and Unify Frontmatter Ref Updates During `gh aw update` + +**Date**: 2026-08-21 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +`gh aw update` is responsible for keeping a repository's AI workflow configuration in sync with upstream packages and registered skill/plugin sources. Prior to this change, the manifest reconciliation step used add-only semantics: if a destination file already existed on disk, the reconciler logged a skip message and continued without re-downloading the upstream version. Similarly, the frontmatter ref-update pipeline rewrote `skills` entries but left `plugins` entries untouched. The net effect was that running `gh aw update` after an upstream package bumped its content produced no local change — the lock file advanced, but the actual workflow, skill, and agent files remained at their stale versions. Users were forced to manually delete managed files before running `update` to trigger a re-download. + +### Decision + +We will change `reconcileManifestManagedAssets` to apply overwrite (force) semantics unconditionally for all package-managed assets — action workflows, skill files, and agent files — removing the exist-check short-circuit that previously caused skips. Concurrently, we will extract a shared `updateFrontmatterRepoRefsInContentWithResolver` function that handles both `skills` and `plugins` frontmatter entries via a `fieldName`/`objectKey` parameter pair, so that plugin SHA refs are updated by the same resolver path used for skills. This gives `gh aw update` consistent, reliable "sync to upstream" semantics across all asset types. + +### Alternatives Considered + +#### Alternative 1: Add an Opt-In `--refresh` Flag + +Introduce a new `--refresh` (or `--force`) CLI flag to `gh aw update` that enables overwrite semantics per-invocation, while preserving the existing add-only default. Users who want to re-sync managed assets would pass the flag explicitly. + +This option was not chosen because it places the burden on users to discover and use the flag correctly. The add-only default is the source of the reported bug; making overwrite opt-in would perpetuate the confusing behavior for users who run `update` expecting full synchronization. The extra CLI surface also adds maintenance overhead. + +#### Alternative 2: Notify-Only Without Overwriting + +Detect stale managed assets during `update` and report which files are out of date (e.g., by comparing content hashes), but leave overwriting to a separate explicit command. This preserves local modifications and avoids silent data loss. + +This option was not chosen because it introduces a two-step workflow for a common operation and requires persisting or computing content hashes as part of the update pass. It also does not resolve the underlying issue for users who expect `update` to be the single command that brings the environment current. + +### Consequences + +#### Positive +- `gh aw update` now reliably propagates upstream changes to all package-managed assets (workflows, skills, agents) without requiring manual file deletion beforehand. +- A single shared `updateFrontmatterRepoRefsInContentWithResolver` function reduces code duplication and provides a consistent extension point for future frontmatter ref types beyond `skills` and `plugins`. + +#### Negative +- Any local modifications made to package-managed files are silently overwritten during `update`, with no diff shown and no prompt for confirmation. Users who customized managed files will lose those changes. +- The behavior change is not signaled by a flag or confirmation step, so users relying on the old "preserve existing" semantics may be surprised by unexpected overwrites in automated pipelines. + +#### Neutral +- The `packageAgentDestinationPath` helper function is removed since the destination path is now computed inline at the call site; this is a minor API surface reduction with no behavioral impact. +- Test coverage is added for the plugin ref update path and for the manifest refresh (overwrite) path, establishing regression baselines for the new semantics. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 169bdb4266e2c99c3239c49c7d05c6b309554492 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:35:18 +0000 Subject: [PATCH 4/8] fix(update): enforce package ownership before overwriting manifest assets; generic ref-update diagnostics Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/update_actions_content_refs.go | 7 +-- pkg/cli/update_manifest.go | 60 ++++++++++++++++++++++--- pkg/cli/update_manifest_test.go | 62 ++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 12 deletions(-) diff --git a/pkg/cli/update_actions_content_refs.go b/pkg/cli/update_actions_content_refs.go index d86b230c2e3..a08db735100 100644 --- a/pkg/cli/update_actions_content_refs.go +++ b/pkg/cli/update_actions_content_refs.go @@ -76,7 +76,7 @@ func updateFrontmatterRepoRefsInContentWithResolver( for i, rawRef := range rawRefs { switch typed := rawRef.(type) { case string: - updated, updatedRef, err := updateSkillRefValue(ctx, typed, allowMajor, verbose, coolDown, resolver) + updated, updatedRef, err := updateSkillRefValue(ctx, fieldName, typed, allowMajor, verbose, coolDown, resolver) if err != nil { return false, content, err } @@ -92,7 +92,7 @@ func updateFrontmatterRepoRefsInContentWithResolver( if !ok { continue } - updated, updatedRef, err := updateSkillRefValue(ctx, skillRef, allowMajor, verbose, coolDown, resolver) + updated, updatedRef, err := updateSkillRefValue(ctx, fieldName, skillRef, allowMajor, verbose, coolDown, resolver) if err != nil { return false, content, err } @@ -120,6 +120,7 @@ func updateFrontmatterRepoRefsInContentWithResolver( func updateSkillRefValue( ctx context.Context, + fieldName string, skillRef string, allowMajor, verbose bool, coolDown time.Duration, @@ -143,7 +144,7 @@ func updateSkillRefValue( latestRef, err := resolver(ctx, repo, currentRef, allowMajor, verbose, coolDown) if err != nil { if verbose { - updateLog.Printf("Skipping skill update for %s@%s: %v", spec, currentRef, err) + updateLog.Printf("Skipping %s update for %s@%s: %v", fieldName, spec, currentRef, err) } return false, skillRef, nil } diff --git a/pkg/cli/update_manifest.go b/pkg/cli/update_manifest.go index e15d176edb3..ba3e5e9cbfa 100644 --- a/pkg/cli/update_manifest.go +++ b/pkg/cli/update_manifest.go @@ -9,6 +9,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" @@ -201,7 +202,7 @@ func updateManifestWorkflowGroup(ctx context.Context, source string, grouped []* return successes, failures } assetEngine := resolveManifestAssetEngine(grouped, opts) - if err := reconcileManifestManagedAssets(ctx, repoSpec.RepoSlug, currentPkg, latestPkg, assetEngine); err != nil { + if err := reconcileManifestManagedAssets(ctx, repoSpec, currentPkg, latestPkg, assetEngine, opts); err != nil { failures = append(failures, updateFailure{Name: source, Error: err.Error()}) } successes = append(successes, groupedSuccesses...) @@ -211,28 +212,38 @@ func updateManifestWorkflowGroup(ctx context.Context, source string, grouped []* // 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, _ *resolvedRepositoryPackage, latestPkg *resolvedRepositoryPackage, engineOverride string) error { +// frontmatter, so their package ownership is derived from ownership records tracked +// under .github/aw/packages. Existing destinations are only overwritten when they are +// tracked as owned by this package (and unmodified locally, or opts.Force is set); +// otherwise the reconciliation fails rather than clobbering an unrelated file. +func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, _ *resolvedRepositoryPackage, latestPkg *resolvedRepositoryPackage, engineOverride string, opts UpdateWorkflowsOptions) error { gitRoot, err := gitutil.FindGitRoot() if err != nil { return fmt.Errorf("failed to find repository root for package assets: %w", err) } - owner, repository, err := splitRepositoryPackageSlug(repo) + owner, repository, err := splitRepositoryPackageSlug(repoSpec.RepoSlug) if err != nil { return err } + packageBase := repositoryPackageIdentifier(repoSpec.RepoSlug, repoSpec.PackagePath) for _, installable := range latestPkg.InstallationSource { if !isActionWorkflowPath(installable.SourcePath) { continue } - destPath := filepath.Join(gitRoot, filepath.FromSlash(installable.DestinationPath)) + destination := filepath.ToSlash(filepath.Clean(installable.DestinationPath)) + destPath := filepath.Join(gitRoot, filepath.FromSlash(destination)) fileExists := false if _, err := os.Stat(destPath); err == nil { fileExists = true } else if !os.IsNotExist(err) { return fmt.Errorf("failed to inspect new package action workflow destination %s: %w", destPath, err) } + if fileExists { + if err := ensurePackageAssetOverwriteAllowed(gitRoot, destination, packageBase, opts.Force); err != nil { + return err + } + } content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repository, installable.SourcePath, latestPkg.ResolvedRef, "") if err != nil { return fmt.Errorf("failed to download new package action workflow %s: %w", installable.SourcePath, err) @@ -251,10 +262,19 @@ func reconcileManifestManagedAssets(ctx context.Context, repo string, _ *resolve } for _, skill := range latestPkg.SkillFiles { - _, err := packageSkillDestinationPath(gitRoot, skill, engineOverride) + destPath, err := packageSkillDestinationPath(gitRoot, skill, engineOverride) if err != nil { return err } + if fileutil.FileExists(destPath) { + destination, err := filepath.Rel(gitRoot, destPath) + if err != nil { + return fmt.Errorf("failed to resolve relative destination for package skill %s: %w", skill.SourcePath, err) + } + if err := ensurePackageAssetOverwriteAllowed(gitRoot, filepath.ToSlash(destination), packageBase, opts.Force); err != nil { + return err + } + } 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) @@ -275,6 +295,17 @@ func reconcileManifestManagedAssets(ctx context.Context, repo string, _ *resolve } for _, agent := range latestPkg.AgentFiles { + agentsDir := filepath.Join(gitRoot, workflow.GetEngineSubAgentDir(engineOverride)) + destPath := filepath.Join(agentsDir, filepath.Base(agent)) + if fileutil.FileExists(destPath) { + destination, err := filepath.Rel(gitRoot, destPath) + if err != nil { + return fmt.Errorf("failed to resolve relative destination for package agent %s: %w", agent, err) + } + if err := ensurePackageAssetOverwriteAllowed(gitRoot, filepath.ToSlash(destination), packageBase, opts.Force); err != nil { + return err + } + } content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repository, agent, latestPkg.ResolvedRef, "") if err != nil { return fmt.Errorf("failed to download new package agent %s: %w", agent, err) @@ -287,6 +318,23 @@ func reconcileManifestManagedAssets(ctx context.Context, repo string, _ *resolve return nil } +// ensurePackageAssetOverwriteAllowed returns an error unless the given destination is +// safe to overwrite: either the caller passed opts.Force, or the destination is tracked +// as owned by packageBase and has not been modified locally since it was installed. +func ensurePackageAssetOverwriteAllowed(gitRoot, destination, packageBase string, force bool) error { + if force { + return nil + } + owned, drifted := packageOwnershipAllowsOverwrite(gitRoot, destination, packageBase) + if !owned { + return fmt.Errorf("package asset %q already exists and is not tracked as owned by %s; use --force to overwrite", destination, packageBase) + } + if drifted { + return fmt.Errorf("package asset %q has local modifications; use --force to overwrite", destination) + } + return nil +} + func resolveManifestAssetEngine(grouped []*workflowWithSource, opts UpdateWorkflowsOptions) string { if opts.EngineOverride != "" { return opts.EngineOverride diff --git a/pkg/cli/update_manifest_test.go b/pkg/cli/update_manifest_test.go index 90434b2d59b..7671bd1301a 100644 --- a/pkg/cli/update_manifest_test.go +++ b/pkg/cli/update_manifest_test.go @@ -4,6 +4,7 @@ package cli import ( "context" + "encoding/json" "errors" "fmt" "os" @@ -41,7 +42,7 @@ func TestReconcileManifestManagedAssets_AddsPackageOwnedAssets(t *testing.T) { } } - err := reconcileManifestManagedAssets(context.Background(), "owner/repo", + err := reconcileManifestManagedAssets(context.Background(), &RepoSpec{RepoSlug: "owner/repo"}, &resolvedRepositoryPackage{}, &resolvedRepositoryPackage{ ResolvedRef: "v2.0.0", @@ -56,6 +57,7 @@ func TestReconcileManifestManagedAssets_AddsPackageOwnedAssets(t *testing.T) { AgentFiles: []string{"agents/reviewer.md"}, }, "copilot", + UpdateWorkflowsOptions{}, ) require.NoError(t, err) workflowPath := filepath.Join(tmpDir, ".github", "workflows", "new.yml") @@ -102,7 +104,7 @@ func TestReconcileManifestManagedAssets_BranchTrackingInstallsMissingAssets(t *t }}, AgentFiles: []string{"agents/reviewer.md"}, } - err := reconcileManifestManagedAssets(context.Background(), "owner/repo", currentAndLatest, currentAndLatest, "copilot") + err := reconcileManifestManagedAssets(context.Background(), &RepoSpec{RepoSlug: "owner/repo"}, currentAndLatest, currentAndLatest, "copilot", UpdateWorkflowsOptions{}) require.NoError(t, err) assert.FileExists(t, filepath.Join(tmpDir, ".github", "workflows", "new.yml")) @@ -126,6 +128,24 @@ func TestReconcileManifestManagedAssets_RefreshesExistingPackageOwnedAssets(t *t require.NoError(t, os.WriteFile(existingSkillPath, []byte("#!/bin/sh\necho old\n"), 0o644)) require.NoError(t, os.WriteFile(existingAgentPath, []byte("# Old Reviewer\n"), 0o644)) + packageBase := "owner/repo" + record := packageOwnershipRecord{ + SchemaVersion: packageOwnershipSchemaVersion, + Package: packageBase, + Source: packageBase + "@v1.0.0", + Installer: "gh-aw test", + Files: []packageOwnershipFileEntry{ + {Source: ".github/workflows/new.yml", Destination: ".github/workflows/new.yml", SHA256: sha256Bytes([]byte("name: old action\n"))}, + {Source: "skills/review/scripts/check.sh", Destination: filepath.ToSlash(filepath.Join(workflow.GetEngineSkillDir(engine), "review", "scripts", "check.sh")), SHA256: sha256Bytes([]byte("#!/bin/sh\necho old\n"))}, + {Source: "agents/reviewer.md", Destination: filepath.ToSlash(filepath.Join(workflow.GetEngineSubAgentDir(engine), "reviewer.md")), SHA256: sha256Bytes([]byte("# Old Reviewer\n"))}, + }, + } + recordPath := packageOwnershipRecordPath(tmpDir, packageBase) + require.NoError(t, os.MkdirAll(filepath.Dir(recordPath), 0o755)) + recordData, err := json.Marshal(record) + require.NoError(t, err) + require.NoError(t, os.WriteFile(recordPath, recordData, 0o644)) + originalDownload := downloadPackageFileFromGitHubForHost t.Cleanup(func() { downloadPackageFileFromGitHubForHost = originalDownload }) downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { @@ -144,7 +164,7 @@ func TestReconcileManifestManagedAssets_RefreshesExistingPackageOwnedAssets(t *t } } - err := reconcileManifestManagedAssets(context.Background(), "owner/repo", + err = reconcileManifestManagedAssets(context.Background(), &RepoSpec{RepoSlug: packageBase}, &resolvedRepositoryPackage{}, &resolvedRepositoryPackage{ ResolvedRef: "v2.0.0", @@ -159,6 +179,7 @@ func TestReconcileManifestManagedAssets_RefreshesExistingPackageOwnedAssets(t *t AgentFiles: []string{"agents/reviewer.md"}, }, engine, + UpdateWorkflowsOptions{}, ) require.NoError(t, err) @@ -175,6 +196,41 @@ func TestReconcileManifestManagedAssets_RefreshesExistingPackageOwnedAssets(t *t assert.Equal(t, "# New Reviewer\n", string(agentContent)) } +func TestReconcileManifestManagedAssets_RefusesToOverwriteUnownedAsset(t *testing.T) { + tmpDir := testutil.TempDir(t, "manifest-assets-collision-*") + require.NoError(t, os.Mkdir(filepath.Join(tmpDir, ".git"), 0o755)) + t.Chdir(tmpDir) + + existingWorkflowPath := filepath.Join(tmpDir, ".github", "workflows", "new.yml") + require.NoError(t, os.MkdirAll(filepath.Dir(existingWorkflowPath), 0o755)) + require.NoError(t, os.WriteFile(existingWorkflowPath, []byte("name: unrelated workflow\n"), 0o644)) + + originalDownload := downloadPackageFileFromGitHubForHost + t.Cleanup(func() { downloadPackageFileFromGitHubForHost = originalDownload }) + downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { + return []byte("name: new action\n"), nil + } + + err := reconcileManifestManagedAssets(context.Background(), &RepoSpec{RepoSlug: "owner/repo"}, + &resolvedRepositoryPackage{}, + &resolvedRepositoryPackage{ + ResolvedRef: "v2.0.0", + InstallationSource: []resolvedPackageInstallable{{ + SourcePath: ".github/workflows/new.yml", + DestinationPath: ".github/workflows/new.yml", + }}, + }, + "copilot", + UpdateWorkflowsOptions{}, + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "not tracked as owned") + + workflowContent, readErr := os.ReadFile(existingWorkflowPath) + require.NoError(t, readErr) + assert.Equal(t, "name: unrelated workflow\n", string(workflowContent), "unowned file must not be overwritten") +} + func TestResolveManifestAssetEngine(t *testing.T) { tmpDir := testutil.TempDir(t, "manifest-assets-engine-*") workflowPath := filepath.Join(tmpDir, "existing.md") From 2b39f9d7ec18260cea59555a22fb43df90ccf70b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:50:12 +0000 Subject: [PATCH 5/8] docs(adr): finalize ADR-54417 to reflect ownership-gated overwrite semantics Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- ...-refresh-package-managed-assets-on-update.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/adr/54417-force-refresh-package-managed-assets-on-update.md b/docs/adr/54417-force-refresh-package-managed-assets-on-update.md index cd29bbd9188..4a61f73df68 100644 --- a/docs/adr/54417-force-refresh-package-managed-assets-on-update.md +++ b/docs/adr/54417-force-refresh-package-managed-assets-on-update.md @@ -1,8 +1,8 @@ # ADR-54417: Force-Refresh Package-Managed Assets and Unify Frontmatter Ref Updates During `gh aw update` **Date**: 2026-08-21 -**Status**: Draft -**Deciders**: Unknown +**Status**: Accepted +**Deciders**: gh-aw maintainers --- @@ -12,7 +12,7 @@ ### Decision -We will change `reconcileManifestManagedAssets` to apply overwrite (force) semantics unconditionally for all package-managed assets — action workflows, skill files, and agent files — removing the exist-check short-circuit that previously caused skips. Concurrently, we will extract a shared `updateFrontmatterRepoRefsInContentWithResolver` function that handles both `skills` and `plugins` frontmatter entries via a `fieldName`/`objectKey` parameter pair, so that plugin SHA refs are updated by the same resolver path used for skills. This gives `gh aw update` consistent, reliable "sync to upstream" semantics across all asset types. +We will change `reconcileManifestManagedAssets` to overwrite existing package-managed assets — action workflows, skill files, and agent files — instead of skipping them, removing the exist-check short-circuit that previously caused skips. Overwriting is gated by package ownership: a destination is only refreshed automatically when it is tracked as owned by the current package (via `.github/aw/packages` ownership records) and has not drifted from its installed state, or when the caller passes `--force`. Destinations that are not tracked as owned by the package, or that have local modifications, cause `update` to fail with an explicit error rather than silently overwriting an unrelated or locally-modified file. Concurrently, we extract a shared `updateFrontmatterRepoRefsInContentWithResolver` function that handles both `skills` and `plugins` frontmatter entries via a `fieldName`/`objectKey` parameter pair, so that plugin SHA refs are updated by the same resolver path used for skills. This gives `gh aw update` consistent, reliable "sync to upstream" semantics across all asset types while still protecting unrelated or locally-modified files from being clobbered. ### Alternatives Considered @@ -31,17 +31,18 @@ This option was not chosen because it introduces a two-step workflow for a commo ### Consequences #### Positive -- `gh aw update` now reliably propagates upstream changes to all package-managed assets (workflows, skills, agents) without requiring manual file deletion beforehand. +- `gh aw update` now reliably propagates upstream changes to package-managed assets (workflows, skills, agents) it owns, without requiring manual file deletion beforehand. +- Ownership tracking (`.github/aw/packages`) prevents `update` from silently overwriting files that belong to a different package, or that a user has customized locally. - A single shared `updateFrontmatterRepoRefsInContentWithResolver` function reduces code duplication and provides a consistent extension point for future frontmatter ref types beyond `skills` and `plugins`. #### Negative -- Any local modifications made to package-managed files are silently overwritten during `update`, with no diff shown and no prompt for confirmation. Users who customized managed files will lose those changes. -- The behavior change is not signaled by a flag or confirmation step, so users relying on the old "preserve existing" semantics may be surprised by unexpected overwrites in automated pipelines. +- Package-managed files that have not drifted from their installed state are still overwritten during `update` with no diff shown and no interactive confirmation; users who intentionally accept upstream changes without inspecting them will not see a preview. +- Encountering an unowned or drifted destination now surfaces as a hard failure (requiring `--force`) rather than a silent skip, which is a behavior change for scripts/pipelines that previously relied on `update` completing without operator intervention in that case. #### Neutral - The `packageAgentDestinationPath` helper function is removed since the destination path is now computed inline at the call site; this is a minor API surface reduction with no behavioral impact. -- Test coverage is added for the plugin ref update path and for the manifest refresh (overwrite) path, establishing regression baselines for the new semantics. +- Test coverage is added for the plugin ref update path, for the manifest refresh (owned-overwrite) path, and for the refusal to overwrite unowned/drifted destinations, establishing regression baselines for the new semantics. --- -*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* +*ADR created by [adr-writer agent] and finalized to reflect the ownership-gated overwrite semantics implemented in this PR.* From ec5b400382f68d7f8d4077f5b8a936d493eb0bfb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:53:46 +0000 Subject: [PATCH 6/8] refactor(update): name the plugin-ref no-object-key sentinel Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/update_actions_content_refs.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/cli/update_actions_content_refs.go b/pkg/cli/update_actions_content_refs.go index a08db735100..d9f723e583a 100644 --- a/pkg/cli/update_actions_content_refs.go +++ b/pkg/cli/update_actions_content_refs.go @@ -19,6 +19,11 @@ import ( type skillRefUpdateResolver func(ctx context.Context, repo, currentRef string, allowMajor, verbose bool, coolDown time.Duration) (string, error) +// noObjectKey signals to updateFrontmatterRepoRefsInContentWithResolver that the field +// being updated (e.g. "plugins") does not support the map[string]any object form with a +// nested ref key, so object-form entries are left untouched. +const noObjectKey = "" + func updateSkillRefsInContent(ctx context.Context, content string, allowMajor, verbose bool, coolDown time.Duration) (bool, string, error) { return updateSkillRefsInContentWithResolver(ctx, content, allowMajor, verbose, coolDown, resolveLatestRef) } @@ -44,7 +49,7 @@ func updatePluginRefsInContentWithResolver( coolDown time.Duration, resolver skillRefUpdateResolver, ) (bool, string, error) { - return updateFrontmatterRepoRefsInContentWithResolver(ctx, content, "plugins", "", allowMajor, verbose, coolDown, resolver) + return updateFrontmatterRepoRefsInContentWithResolver(ctx, content, "plugins", noObjectKey, allowMajor, verbose, coolDown, resolver) } func updateFrontmatterRepoRefsInContentWithResolver( @@ -85,7 +90,7 @@ func updateFrontmatterRepoRefsInContentWithResolver( changed = true } case map[string]any: - if objectKey == "" { + if objectKey == noObjectKey { continue } skillRef, ok := typed[objectKey].(string) From 73a60ea8972886919a765e8450d5799a4db0604c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:57:42 +0000 Subject: [PATCH 7/8] feat(update): warn instead of overwriting when upstream removes a skill or agent Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/update_manifest.go | 41 ++++++++++++++++++++++++++- pkg/cli/update_manifest_test.go | 50 +++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/pkg/cli/update_manifest.go b/pkg/cli/update_manifest.go index ba3e5e9cbfa..a5d4f866f47 100644 --- a/pkg/cli/update_manifest.go +++ b/pkg/cli/update_manifest.go @@ -216,7 +216,7 @@ func updateManifestWorkflowGroup(ctx context.Context, source string, grouped []* // under .github/aw/packages. Existing destinations are only overwritten when they are // tracked as owned by this package (and unmodified locally, or opts.Force is set); // otherwise the reconciliation fails rather than clobbering an unrelated file. -func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, _ *resolvedRepositoryPackage, latestPkg *resolvedRepositoryPackage, engineOverride string, opts UpdateWorkflowsOptions) error { +func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, currentPkg *resolvedRepositoryPackage, latestPkg *resolvedRepositoryPackage, engineOverride string, opts UpdateWorkflowsOptions) error { gitRoot, err := gitutil.FindGitRoot() if err != nil { return fmt.Errorf("failed to find repository root for package assets: %w", err) @@ -227,6 +227,8 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, _ * } packageBase := repositoryPackageIdentifier(repoSpec.RepoSlug, repoSpec.PackagePath) + warnUpstreamRemovedSkillsAndAgents(currentPkg, latestPkg) + for _, installable := range latestPkg.InstallationSource { if !isActionWorkflowPath(installable.SourcePath) { continue @@ -318,6 +320,43 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, _ * return nil } +// warnUpstreamRemovedSkillsAndAgents reports skill and agent files that were present in +// the currently-installed package manifest but are no longer listed in the latest +// manifest. These assets are intentionally left untouched (not deleted) since the +// removal may be transient or unintended upstream; the local copy is kept and a warning +// is printed so the user can decide whether to remove it manually. +func warnUpstreamRemovedSkillsAndAgents(currentPkg, latestPkg *resolvedRepositoryPackage) { + if currentPkg == nil || latestPkg == nil { + return + } + + latestSkillSources := make(map[string]bool, len(latestPkg.SkillFiles)) + for _, skill := range latestPkg.SkillFiles { + latestSkillSources[skill.SourcePath] = true + } + for _, skill := range currentPkg.SkillFiles { + if latestSkillSources[skill.SourcePath] { + continue + } + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf( + "Skill %q was removed from the upstream package; skipping update and keeping the local copy. Remove it manually if it is no longer needed.", + skill.SourcePath))) + } + + latestAgentSources := make(map[string]bool, len(latestPkg.AgentFiles)) + for _, agent := range latestPkg.AgentFiles { + latestAgentSources[agent] = true + } + for _, agent := range currentPkg.AgentFiles { + if latestAgentSources[agent] { + continue + } + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf( + "Agent %q was removed from the upstream package; skipping update and keeping the local copy. Remove it manually if it is no longer needed.", + agent))) + } +} + // ensurePackageAssetOverwriteAllowed returns an error unless the given destination is // safe to overwrite: either the caller passed opts.Force, or the destination is tracked // as owned by packageBase and has not been modified locally since it was installed. diff --git a/pkg/cli/update_manifest_test.go b/pkg/cli/update_manifest_test.go index 7671bd1301a..c49f5b99264 100644 --- a/pkg/cli/update_manifest_test.go +++ b/pkg/cli/update_manifest_test.go @@ -231,6 +231,56 @@ func TestReconcileManifestManagedAssets_RefusesToOverwriteUnownedAsset(t *testin assert.Equal(t, "name: unrelated workflow\n", string(workflowContent), "unowned file must not be overwritten") } +func TestReconcileManifestManagedAssets_WarnsWhenUpstreamRemovesSkillOrAgent(t *testing.T) { + tmpDir := testutil.TempDir(t, "manifest-assets-removed-*") + require.NoError(t, os.Mkdir(filepath.Join(tmpDir, ".git"), 0o755)) + t.Chdir(tmpDir) + + engine := "copilot" + existingSkillPath := filepath.Join(tmpDir, workflow.GetEngineSkillDir(engine), "review", "scripts", "check.sh") + existingAgentPath := filepath.Join(tmpDir, workflow.GetEngineSubAgentDir(engine), "reviewer.md") + require.NoError(t, os.MkdirAll(filepath.Dir(existingSkillPath), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Dir(existingAgentPath), 0o755)) + require.NoError(t, os.WriteFile(existingSkillPath, []byte("#!/bin/sh\necho old\n"), 0o644)) + require.NoError(t, os.WriteFile(existingAgentPath, []byte("# Old Reviewer\n"), 0o644)) + + originalDownload := downloadPackageFileFromGitHubForHost + t.Cleanup(func() { downloadPackageFileFromGitHubForHost = originalDownload }) + downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { + return nil, fmt.Errorf("unexpected download for removed package path %s", path) + } + + currentPkg := &resolvedRepositoryPackage{ + ResolvedRef: "v1.0.0", + SkillFiles: []resolvedPackageSkillFile{{ + SourcePath: "skills/review/scripts/check.sh", + SkillName: "review", + }}, + AgentFiles: []string{"agents/reviewer.md"}, + } + latestPkg := &resolvedRepositoryPackage{ + ResolvedRef: "v2.0.0", + } + + var err error + output := testutil.CaptureStderr(t, func() { + err = reconcileManifestManagedAssets(context.Background(), &RepoSpec{RepoSlug: "owner/repo"}, currentPkg, latestPkg, engine, UpdateWorkflowsOptions{}) + }) + require.NoError(t, err) + + assert.Contains(t, output, "skills/review/scripts/check.sh") + assert.Contains(t, output, "removed from the upstream package") + assert.Contains(t, output, "agents/reviewer.md") + + skillContent, readErr := os.ReadFile(existingSkillPath) + require.NoError(t, readErr) + assert.Equal(t, "#!/bin/sh\necho old\n", string(skillContent), "removed-upstream skill must not be modified") + + agentContent, readErr := os.ReadFile(existingAgentPath) + require.NoError(t, readErr) + assert.Equal(t, "# Old Reviewer\n", string(agentContent), "removed-upstream agent must not be modified") +} + func TestResolveManifestAssetEngine(t *testing.T) { tmpDir := testutil.TempDir(t, "manifest-assets-engine-*") workflowPath := filepath.Join(tmpDir, "existing.md") From a592e5bd8492351e4601cf7df9a89424d8157263 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:57:55 +0000 Subject: [PATCH 8/8] fix(cli): reword update error messages to satisfy errormessage lint Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/update_actions_content_refs.go | 4 +- pkg/cli/update_actions_workflow_files.go | 6 +-- pkg/cli/update_manifest.go | 58 ++++++++++++------------ 3 files changed, 34 insertions(+), 34 deletions(-) diff --git a/pkg/cli/update_actions_content_refs.go b/pkg/cli/update_actions_content_refs.go index d9f723e583a..36e042d23ac 100644 --- a/pkg/cli/update_actions_content_refs.go +++ b/pkg/cli/update_actions_content_refs.go @@ -114,11 +114,11 @@ func updateFrontmatterRepoRefsInContentWithResolver( updatedFrontmatter, err := yaml.Marshal(result.Frontmatter) if err != nil { - return false, content, fmt.Errorf("failed to marshal updated frontmatter: %w", err) + return false, content, fmt.Errorf("unable to marshal updated frontmatter: %w", err) } updatedContent, err := parser.ReconstructWorkflowFile(parser.QuoteCronExpressions(string(updatedFrontmatter)), result.Markdown) if err != nil { - return false, content, fmt.Errorf("failed to reconstruct workflow file: %w", err) + return false, content, fmt.Errorf("unable to reconstruct workflow file: %w", err) } return true, updatedContent, nil } diff --git a/pkg/cli/update_actions_workflow_files.go b/pkg/cli/update_actions_workflow_files.go index b3a1c682adb..dca9a8490b3 100644 --- a/pkg/cli/update_actions_workflow_files.go +++ b/pkg/cli/update_actions_workflow_files.go @@ -109,7 +109,7 @@ func updateActionsInWorkflowFiles(ctx context.Context, deps actionUpdateDeps, op } if err := os.WriteFile(path, []byte(newContent), constants.FilePermPublic); err != nil { - return fmt.Errorf("failed to write updated workflow %s: %w", path, err) + return fmt.Errorf("unable to write updated workflow %s: %w", path, err) } fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated action/skill/plugin references in "+d.Name())) @@ -117,12 +117,12 @@ func updateActionsInWorkflowFiles(ctx context.Context, deps actionUpdateDeps, op return nil }) if err != nil { - return fmt.Errorf("failed to walk workflows directory: %w", err) + return fmt.Errorf("unable to walk workflows directory: %w", err) } if len(updatedFiles) > 0 && !opts.noCompile { if err := compileWorkflowsForUpdate(ctx, updatedFiles, opts.workflowsDir, opts.engineOverride, opts.verbose, opts.approve); err != nil { - return fmt.Errorf("failed to compile workflows with updated action references: %w", err) + return fmt.Errorf("unable to compile workflows with updated action references: %w", err) } } diff --git a/pkg/cli/update_manifest.go b/pkg/cli/update_manifest.go index a5d4f866f47..9cac58be68a 100644 --- a/pkg/cli/update_manifest.go +++ b/pkg/cli/update_manifest.go @@ -44,7 +44,7 @@ func parseManifestSourceSpec(source string) (*RepoSpec, bool, error) { return nil, false, nil } if err != nil { - return nil, true, fmt.Errorf("invalid manifest source %q: %w", source, err) + return nil, true, fmt.Errorf("manifest source %q is not valid: %w; expected format like \"owner/repo\" or \"owner/repo/path\"", source, err) } if repoSpec == nil { return nil, false, nil @@ -219,7 +219,7 @@ func updateManifestWorkflowGroup(ctx context.Context, source string, grouped []* func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, currentPkg *resolvedRepositoryPackage, latestPkg *resolvedRepositoryPackage, engineOverride string, opts UpdateWorkflowsOptions) error { gitRoot, err := gitutil.FindGitRoot() if err != nil { - return fmt.Errorf("failed to find repository root for package assets: %w", err) + return fmt.Errorf("unable to find repository root for package assets: %w", err) } owner, repository, err := splitRepositoryPackageSlug(repoSpec.RepoSlug) if err != nil { @@ -239,7 +239,7 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, cur if _, err := os.Stat(destPath); err == nil { fileExists = true } else if !os.IsNotExist(err) { - return fmt.Errorf("failed to inspect new package action workflow destination %s: %w", destPath, err) + return fmt.Errorf("unable to inspect new package action workflow destination %s: %w", destPath, err) } if fileExists { if err := ensurePackageAssetOverwriteAllowed(gitRoot, destination, packageBase, opts.Force); err != nil { @@ -248,13 +248,13 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, cur } content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repository, installable.SourcePath, latestPkg.ResolvedRef, "") if err != nil { - return fmt.Errorf("failed to download new package action workflow %s: %w", installable.SourcePath, err) + return fmt.Errorf("unable to download new package action workflow %s: %w", installable.SourcePath, err) } if err := os.MkdirAll(filepath.Dir(destPath), constants.DirPermPublic); err != nil { - return fmt.Errorf("failed to create package action workflow directory: %w", err) + return fmt.Errorf("unable to create package action workflow directory: %w", err) } if err := os.WriteFile(destPath, content, constants.FilePermPublic); err != nil { - return fmt.Errorf("failed to install new package action workflow %s: %w", installable.DestinationPath, err) + return fmt.Errorf("unable to install new package action workflow %s: %w", installable.DestinationPath, err) } if fileExists { fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated package action workflow: "+filepath.Base(destPath))) @@ -271,7 +271,7 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, cur if fileutil.FileExists(destPath) { destination, err := filepath.Rel(gitRoot, destPath) if err != nil { - return fmt.Errorf("failed to resolve relative destination for package skill %s: %w", skill.SourcePath, err) + return fmt.Errorf("unable to resolve relative destination for package skill %s: %w", skill.SourcePath, err) } if err := ensurePackageAssetOverwriteAllowed(gitRoot, filepath.ToSlash(destination), packageBase, opts.Force); err != nil { return err @@ -279,7 +279,7 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, cur } 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) + return fmt.Errorf("unable to download new package skill %s: %w", skill.SourcePath, err) } resolved := &ResolvedWorkflow{ Content: content, @@ -292,7 +292,7 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, cur Quiet: false, Force: true, }, gitRoot); err != nil { - return fmt.Errorf("failed to install new package skill %s: %w", skill.SourcePath, err) + return fmt.Errorf("unable to install new package skill %s: %w", skill.SourcePath, err) } } @@ -302,7 +302,7 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, cur if fileutil.FileExists(destPath) { destination, err := filepath.Rel(gitRoot, destPath) if err != nil { - return fmt.Errorf("failed to resolve relative destination for package agent %s: %w", agent, err) + return fmt.Errorf("unable to resolve relative destination for package agent %s: %w", agent, err) } if err := ensurePackageAssetOverwriteAllowed(gitRoot, filepath.ToSlash(destination), packageBase, opts.Force); err != nil { return err @@ -310,11 +310,11 @@ func reconcileManifestManagedAssets(ctx context.Context, repoSpec *RepoSpec, cur } content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repository, agent, latestPkg.ResolvedRef, "") if err != nil { - return fmt.Errorf("failed to download new package agent %s: %w", agent, err) + return fmt.Errorf("unable to download new package agent %s: %w", agent, err) } resolved := &ResolvedWorkflow{Content: content, Spec: &WorkflowSpec{WorkflowPath: agent}, IsPackageAgentFile: true} if err := addAgentFileWithTracking(resolved, nil, AddOptions{EngineOverride: engineOverride, Force: true}, gitRoot); err != nil { - return fmt.Errorf("failed to install new package agent %s: %w", agent, err) + return fmt.Errorf("unable to install new package agent %s: %w", agent, err) } } return nil @@ -398,7 +398,7 @@ func packageSkillDestinationPath(gitRoot string, skill resolvedPackageSkillFile, } relPath, err := resolveSkillRelativePath(resolved) if err != nil { - return "", fmt.Errorf("failed to resolve destination for package skill %s: %w", skill.SourcePath, err) + return "", fmt.Errorf("unable to resolve destination for package skill %s: %w", skill.SourcePath, err) } return filepath.Join(gitRoot, workflow.GetEngineSkillDir(engineOverride), skill.SkillName, relPath), nil } @@ -406,11 +406,11 @@ func packageSkillDestinationPath(gitRoot string, skill resolvedPackageSkillFile, func removeManifestManagedWorkflow(workflowPath string) error { updateManifestLog.Printf("Removing manifest-managed workflow no longer in manifest: %s", filepath.Base(workflowPath)) if err := os.Remove(workflowPath); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to remove workflow %s: %w", filepath.Base(workflowPath), err) + return fmt.Errorf("unable to remove workflow %s: %w", filepath.Base(workflowPath), err) } lockPath := strings.TrimSuffix(workflowPath, ".md") + ".lock.yml" if err := os.Remove(lockPath); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to remove lock file %s: %w", filepath.Base(lockPath), err) + return fmt.Errorf("unable to remove lock file %s: %w", filepath.Base(lockPath), err) } fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Removed workflow no longer listed in manifest: "+filepath.Base(workflowPath))) return nil @@ -421,7 +421,7 @@ func updateManifestManagedWorkflow(ctx context.Context, update manifestManagedWo sourceSpecCurrent := sourceSpecWithRef(&SourceSpec{Repo: update.repo, Path: update.currentPath}, update.currentRef) newContent, err := downloadWorkflowContentFn(ctx, update.repo, update.latestPath, update.latestRef, opts.Verbose) if err != nil { - return fmt.Errorf("failed to download workflow %s/%s@%s: %w", update.repo, update.latestPath, update.latestRef, err) + return fmt.Errorf("unable to download workflow %s/%s@%s: %w", update.repo, update.latestPath, update.latestRef, err) } if !opts.Force && update.currentRef == update.latestRef && update.currentPath == update.latestPath { @@ -446,12 +446,12 @@ func updateManifestManagedWorkflow(ctx context.Context, update manifestManagedWo } else { currentContent, err := os.ReadFile(update.wf.Path) if err != nil { - return fmt.Errorf("failed to read current workflow: %w", err) + return fmt.Errorf("unable to read current workflow: %w", err) } newSourceSpec := sourceSpecWithRef(&SourceSpec{Repo: update.repo, Path: update.latestPath}, update.latestRef) mergedContent, conflicts, mergeErr := MergeWorkflowContent(string(baseContent), string(currentContent), string(newContent), sourceSpecCurrent, newSourceSpec, update.wf.Path, opts.Verbose) if mergeErr != nil { - return fmt.Errorf("failed to merge workflow content: %w", mergeErr) + return fmt.Errorf("unable to merge workflow content: %w", mergeErr) } finalContent = mergedContent hasConflicts = conflicts @@ -473,7 +473,7 @@ func updateManifestManagedWorkflow(ctx context.Context, update manifestManagedWo finalContent, err = UpdateFieldInFrontmatter(finalContent, "source", update.manifestSource) if err != nil { - return fmt.Errorf("failed to update source frontmatter: %w", err) + return fmt.Errorf("unable to update source frontmatter: %w", err) } if opts.NoStopAfter { @@ -490,15 +490,15 @@ func updateManifestManagedWorkflow(ctx context.Context, update manifestManagedWo if !opts.DisableSecurityScanner { if findings := workflow.ScanMarkdownSecurity(finalContent); len(findings) > 0 { - return fmt.Errorf("workflow '%s' failed security scan: %d issue(s) detected", update.wf.Name, len(findings)) + return fmt.Errorf("workflow '%s' has %d security scan issue(s); review the findings and resolve them before updating, or pass --no-security-scanner to skip this check", update.wf.Name, len(findings)) } } if err := fetchManifestManagedDependencies(ctx, newContent, update.repo, update.latestPath, update.latestRef, filepath.Dir(update.wf.Path), opts.Verbose); err != nil { - return fmt.Errorf("failed to update workflow dependencies: %w", err) + return fmt.Errorf("unable to update workflow dependencies: %w", err) } if err := os.WriteFile(update.wf.Path, []byte(finalContent), constants.FilePermPublic); err != nil { - return fmt.Errorf("failed to write updated workflow: %w", err) + return fmt.Errorf("unable to write updated workflow: %w", err) } if hasConflicts { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Updated %s from %s to %s with CONFLICTS - please review and resolve manually", update.wf.Name, shortRef(update.currentRef), shortRef(update.latestRef)))) @@ -507,7 +507,7 @@ func updateManifestManagedWorkflow(ctx context.Context, update manifestManagedWo fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Updated %s from %s to %s", update.wf.Name, shortRef(update.currentRef), shortRef(update.latestRef)))) if !opts.NoCompile { if err := compileWorkflowsForUpdate(ctx, []string{update.wf.Path}, opts.WorkflowsDir, opts.EngineOverride, opts.Verbose, opts.Approve); err != nil { - return fmt.Errorf("failed to compile updated workflow: %w", err) + return fmt.Errorf("unable to compile updated workflow: %w", err) } } return nil @@ -517,12 +517,12 @@ func addManifestManagedWorkflow(ctx context.Context, targetDir, name, repo, late updateManifestLog.Printf("Adding new manifest-managed workflow %s from %s/%s@%s", name, repo, latestPath, latestRef) newContent, err := downloadWorkflowContentFn(ctx, repo, latestPath, latestRef, opts.Verbose) if err != nil { - return fmt.Errorf("failed to download new manifest workflow %s/%s@%s: %w", repo, latestPath, latestRef, err) + return fmt.Errorf("unable to download new manifest workflow %s/%s@%s: %w", repo, latestPath, latestRef, err) } content, err := UpdateFieldInFrontmatter(string(newContent), "source", manifestSource) if err != nil { - return fmt.Errorf("failed to add source frontmatter for %s: %w", name, err) + return fmt.Errorf("unable to add source frontmatter for %s: %w", name, err) } if opts.NoStopAfter { cleanedContent, err := RemoveFieldFromOnTrigger(content, "stop-after") @@ -537,21 +537,21 @@ func addManifestManagedWorkflow(ctx context.Context, targetDir, name, repo, late } if !opts.DisableSecurityScanner { if findings := workflow.ScanMarkdownSecurity(content); len(findings) > 0 { - return fmt.Errorf("workflow '%s' failed security scan: %d issue(s) detected", name, len(findings)) + return fmt.Errorf("workflow '%s' has %d security scan issue(s); review the findings and resolve them before adding, or pass --no-security-scanner to skip this check", name, len(findings)) } } destPath := filepath.Join(targetDir, name+".md") if err := fetchManifestManagedDependencies(ctx, []byte(content), repo, latestPath, latestRef, targetDir, opts.Verbose); err != nil { - return fmt.Errorf("failed to install workflow dependencies: %w", err) + return fmt.Errorf("unable to install workflow dependencies: %w", err) } if err := os.WriteFile(destPath, []byte(content), constants.FilePermPublic); err != nil { - return fmt.Errorf("failed to write new manifest workflow %s: %w", destPath, err) + return fmt.Errorf("unable to write new manifest workflow %s: %w", destPath, err) } fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Added new workflow from manifest: "+filepath.Base(destPath))) if !opts.NoCompile { if err := compileWorkflowsForUpdate(ctx, []string{destPath}, opts.WorkflowsDir, opts.EngineOverride, opts.Verbose, opts.Approve); err != nil { - return fmt.Errorf("failed to compile new manifest workflow: %w", err) + return fmt.Errorf("unable to compile new manifest workflow: %w", err) } } return nil