Update gh aw update to refresh upstream skills/plugins and package-managed assets

docs/adr/54417-force-refresh-package-managed-assets-on-update.md

@@ -0,0 +1,48 @@
+# ADR-54417: Force-Refresh Package-Managed Assets and Unify Frontmatter Ref Updates During `gh aw update`
+
+**Date**: 2026-08-21
+**Status**: Accepted
+**Deciders**: gh-aw maintainers
+
+---
+
+### 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 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
+
+#### 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 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
+- 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, 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] and finalized to reflect the ownership-gated overwrite semantics implemented in this PR.*

pkg/cli/update_actions_content_refs.go

@@ -19,78 +19,113 @@ 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)
 }
 
+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", noObjectKey, 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
 	}
 	if result == nil || result.Frontmatter == nil {
 		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)
+			updated, updatedRef, err := updateSkillRefValue(ctx, fieldName, 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 == noObjectKey {
+				continue
+			}
+			skillRef, ok := typed[objectKey].(string)
 			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
 			}
 			if updated {
-				typed["skill"] = updatedRef
+				typed[objectKey] = updatedRef
 				changed = true
 			}
 		}
 	}
 	if !changed {
 		return false, content, nil
 	}
-	result.Frontmatter["skills"] = rawSkills
+	result.Frontmatter[fieldName] = rawRefs
 
 	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
 }
 
 func updateSkillRefValue(
 	ctx context.Context,
+	fieldName string,
 	skillRef string,
 	allowMajor, verbose bool,
 	coolDown time.Duration,
@@ -114,7 +149,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
 	}

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.

pkg/cli/update_actions_workflow_files.go

@@ -96,26 +96,33 @@ 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
 		}
 
 		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 references in "+d.Name()))
+		fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated action/skill/plugin references in "+d.Name()))
 		updatedFiles = append(updatedFiles, path)
 		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)
 		}
 	}