Support package resources with scoped ownership#54120
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds manifest-declared repository resources with package-scoped ownership, update, and removal support.
Changes:
- Adds resource parsing, validation, resolution, and installation.
- Introduces ownership records and drift detection.
- Extends update/remove behavior, tests, schema, and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schemas/aw_manifest_schema.json |
Defines resource mappings. |
pkg/cli/update_manifest.go |
Synchronizes resources during updates. |
pkg/cli/spec.go |
Marks package resource specs. |
pkg/cli/remove_command.go |
Initiates owned-file cleanup. |
pkg/cli/bootstrap_profile_inference.go |
Excludes resources from inference. |
pkg/cli/bootstrap_profile_helpers.go |
Excludes resources from auth detection. |
pkg/cli/add_workflow_resolution.go |
Resolves local and remote resources. |
pkg/cli/add_package_ownership.go |
Implements ownership and synchronization. |
pkg/cli/add_package_manifest.go |
Extends the resolved package model. |
pkg/cli/add_package_manifest_test.go |
Tests manifest resource resolution. |
pkg/cli/add_package_manifest_resources.go |
Validates and normalizes resources. |
pkg/cli/add_package_manifest_resolve.go |
Adds remote resource resolution. |
pkg/cli/add_package_manifest_parse.go |
Parses resource declarations. |
pkg/cli/add_command.go |
Installs resources and records ownership. |
pkg/cli/add_command_test.go |
Tests installation and drift handling. |
package.md |
Adds package resource guidance. |
docs/src/content/docs/specs/repository-package-manifest-specification.md |
Specifies resource lifecycle and safeguards. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 17/17 changed files
- Comments generated: 14
- Review effort level: Balanced
| case strings.HasPrefix(destination, constants.GithubDir+"aw/"): | ||
| remaining := strings.TrimPrefix(destination, constants.GithubDir+"aw/") | ||
| if remaining == "" || strings.HasPrefix(remaining, "../") { | ||
| return errorsForResourceDestination() | ||
| } |
| absolutePath := filepath.Clean(filepath.Join(packageDir, filepath.FromSlash(resource.Source))) | ||
| if err := validateLocalPackageMappingSource(absolutePath, packageDir, resource.Source); err != nil { | ||
| return nil, err |
| tracker.TrackCreated(destFile) | ||
| } | ||
| } | ||
| if err := os.WriteFile(destFile, resolved.Content, constants.FilePermPublic); err != nil { |
| path := filepath.Join(gitRoot, filepath.FromSlash(destination)) | ||
| current, digestErr := fileSHA256(path) | ||
| if digestErr == nil && current == entry.SHA256 { | ||
| if err := os.Remove(path); err != nil && !os.IsNotExist(err) { |
| if err := os.MkdirAll(filepath.Dir(destPath), constants.DirPermPublic); err != nil { | ||
| return fmt.Errorf("failed to create package resource directory: %w", err) | ||
| } | ||
| if err := os.WriteFile(destPath, content, constants.FilePermPublic); err != nil { |
| if strings.HasPrefix(destination, constants.WorkflowsDirSlash) { | ||
| continue | ||
| } |
| fileExists := fileutil.FileExists(destFile) | ||
| if fileExists && !opts.Force { | ||
| packageSource := packageSourceForSpec(resolved.Spec, resolved.SourceInfo) | ||
| if owned, drifted := packageOwnershipAllowsOverwrite(gitRoot, rel, packageSource); !owned || drifted { |
| if len(installationSources) == 0 && len(resourceFiles) == 0 && len(extensionFiles.skillFiles) == 0 && len(extensionFiles.agentFiles) == 0 { | ||
| return nil, fmt.Errorf("repository %q does not contain any installable workflows, resources, skills, or agents (either explicitly declared or auto-discovered). Add workflows under 'workflows/', resources in aw.yml, skills under 'skills/', or agents under 'agents/', or declare them explicitly in aw.yml", repositoryPackageIdentifier(repoSpec.RepoSlug, packagePath)) |
| if fileutil.FileExists(destPath) && !opts.Force { | ||
| if owned, drifted := packageOwnershipAllowsOverwrite(gitRoot, destination, packageBase); !owned || drifted { |
| if spec.RepoSlug == "" { | ||
| return "" | ||
| } |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ 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. ✅
|
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
Resource support adds useful capability, but the current implementation leaves the package updater in an inconsistent state and the removal path does not correctly clean up package-owned assets.
Blocking themes
- resource updates are not atomic: stale assets are deleted before replacement work is known to succeed
- package update reporting can mark the same workflow as both success and failure
- package-owned resource cleanup keys off
*.mdworkflows only, so resource-only / action-only packages leak owned files
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 11.2 AIC · ⌖ 8.33 AIC · ⊞ 4.6K
Comment /review to run again
| successes = append(successes, name) | ||
| } | ||
|
|
||
| if err := syncManifestManagedResources(ctx, repoSpec, latestPkg, latestRef, opts); err != nil { |
There was a problem hiding this comment.
This update can report the same workflow as both updated and failed, which breaks the caller’s result accounting and can turn a partial resource sync failure into a misleading success summary.
💡 Why this is a real bug
`successes` is populated before `syncManifestManagedResources` runs. If resource sync then fails, lines 195-198 append a failure entry for every workflow in `grouped` without removing the already-recorded successes. The same workflow name is therefore returned in both lists, so higher-level reporting cannot tell whether that workflow actually succeeded.A safer pattern is to treat resource sync as part of the package update transaction: either run it before appending workflow successes, or clear/reclassify the package’s successes when resource sync fails.
| desired[filepath.ToSlash(filepath.Clean(resource.DestinationPath))] = resource | ||
| } | ||
|
|
||
| var kept []packageOwnershipFileEntry |
There was a problem hiding this comment.
The updater deletes stale resource files before it knows the new package state can be installed, so any later download/write error leaves the repository partially updated with files already removed.
💡 Why this should block merge
The loop at lines 271-289 eagerly removes resources that disappeared from the manifest, but the subsequent loop still has multiple failure points: download failure, mkdir failure, write failure, and digest failure. Hitting any of those exits early after destructive deletions have already happened, which violates the atomicity guarantee described in the spec update and makes `gh aw update` non-rollbackable for resources.Please stage removals until all downloads and writes succeed, or write into a temp area and swap/update the ownership record only after the whole resource set has been applied successfully.
| return os.WriteFile(recordPath, data, constants.FilePermPublic) | ||
| } | ||
|
|
||
| func packageHasRemainingWorkflows(gitRoot, packageBase string) bool { |
There was a problem hiding this comment.
This cleanup path never removes package-owned resources when a package installs only action workflows or only resources, so uninstalling the last remaining package content leaves .github/aw / issue-template assets orphaned forever.
💡 Why this is broken
`removePackageOwnedFilesIfUnused` decides whether a package is still present by scanning only `constants.GetWorkflowDir()/*.md`. That misses two cases introduced or already supported here:- packages that install only resources and no markdown workflows, and
- packages whose remaining managed content is action workflows copied as
.yml.
In both cases RemoveWorkflows has no surviving .md marker to inspect, so resource ownership cleanup is never triggered correctly. The new feature depends on ownership records for removal, so the presence check needs to account for all package-managed artifacts, not just markdown workflows.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on security and correctness gaps. The inline comments already identify 14 concrete issues; this summary groups the most critical themes.
📋 Key Themes & Risks
🔴 Security — Must Fix
-
Trust-store self-poisoning (
add_package_manifest_resources.go:85): Allowed destinations include.github/aw/packages/**— a malicious package can install a forged ownership record for another package. Exclude the trust-store path from the destination allowlist entirely. -
Symlink traversal — source path (
add_package_manifest_resources.go:112):Lstatonly checks the leaf; a source likelinked-dir/secret.jsonwherelinked-diris a symlink escapes the package root. Callfilepath.EvalSymlinkson the full resolved source before the containment check. -
Symlink write-through — destination (
add_command.go:519,add_package_ownership.go:309):os.WriteFilefollows existing destination symlinks;--forcethen allows writing outside the repository.Lstatthe final destination before writing and reject symlinks. -
Unvalidated deletes from ownership records (
add_package_ownership.go:395): Paths loaded from JSON are deleted without namespace or containment checks. A forged record with../../can delete arbitrary files. Apply the same validation used for incoming manifest destinations.
🟡 Correctness — Should Fix
-
Non-atomic stale-delete then write (
add_package_ownership.go:281): Stale files are removed before new ones are written; a failed download leaves the installation in a partial state. Buffer writes first, then delete staleness only after all writes succeed. -
Tag vs. commit SHA (
add_package_ownership.go:260):latestRefmay be a mutable release tag stored asResolvedCommit. Resolve tags to their underlying commit SHA before recording. -
Success/failure ambiguity (
update_manifest.go:199): Workflows entersuccessesbefore resource sync; a resource failure produces entries in both lists. -
Resource-only packages invisible to update/remove (
add_package_manifest_resolve.go:57): Update and remove discovery scans installed*.mdfiles; a resources-only package leaves no marker and becomes unmanageable. -
Stale workflow metadata in ownership records (
add_package_ownership.go:264): The update path copies the previousFileslist without re-deriving added/removed/updated workflows.
✅ Positive Highlights
- Clean layered validation in
add_package_manifest_resources.go(absolute path, traversal, duplicate destination, namespace checks) - SHA-256 digest tracking is the right foundation for ownership safety
- Destination allowlist is a sound security boundary — the self-poisoning issue is a gap in the list, not a gap in the approach
The security items (1–4) are blockers; they allow a malicious package or forged record to escape the repository boundary. I'd recommend a focused pass on those before merging.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 17.5 AIC · ⌖ 11.2 AIC · ⊞ 7.8K
Comment /matt to run again
There was a problem hiding this comment.
Review: Support package resources with scoped ownership
This PR adds declarative resources support to the aw.yml package manifest, enabling packages to install inert repository assets (issue templates, CODEOWNERS, .github/aw/** files) alongside workflows.
The implementation is well-structured: destination validation runs at manifest-parse time (validateManifestResourceDestination), the allowlist is enforced, path-traversal guards are in place, and ownership records are written per-package.
Existing review comments already cover the blocking issues:
The 14 existing inline comments identify the high-signal issues:
- Trust-store self-poisoning (
add_package_manifest_resources.go:85) — the.github/aw/packages/**allowlist entry lets a package overwrite ownership records for other packages - Symlink traversal on source (
add_package_manifest_resources.go:112) —Lstatonly checks the final path; symlinked parent directories in the source pass through - Symlink traversal on destination write (
add_command.go:519,add_package_ownership.go:309) —os.WriteFilefollows existing destination symlinks;--forcebecomes a write-anywhere primitive - Non-atomic update (
add_package_ownership.go:281) — stale resource files are deleted before replacement downloads succeed, leaving inconsistent state on failure - Path traversal in ownership record destinations (
add_package_ownership.go:395) — destinations in the JSON record are used for deletion without re-running the allowlist check - Stale ownership metadata (
add_package_ownership.go:264) — only upserting resources leaves prior entries for updated/removed workflows intact - Case-sensitive collision gap (
add_command.go:502,add_package_ownership.go:295) — exact-matchFileExistsmisses differently-cased names on case-insensitive filesystems - Success recorded before resource sync (
update_manifest.go:199) — resource sync failures append tofailuresaftersuccessesis already populated .github/workflows/resources silently skipped (add_package_ownership.go:391) — action workflows installed as resources bypass ownership tracking- Local-only resources lack ownership records (
add_package_ownership.go:94) — local-manifest resources skip ownership metadata entirely
No new blocking issues found beyond those already flagged. The overall design and validation chain are sound; addressing the existing comments — particularly the trust-store self-poisoning and symlink traversal issues — is needed before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38.9 AIC · ⌖ 8.99 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail review — cut opportunities only (correctness/security are out of scope for this pass; existing Copilot comments already flag those).
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #54120 · auto · 44.6 AIC · ⌖ 6.34 AIC · ⊞ 7.3K
Comment /ponytail to run again
| tracker.TrackCreated(recordPath) | ||
| } | ||
| } | ||
| data, err := json.MarshalIndent(record, "", " ") |
There was a problem hiding this comment.
L64-70/331-337/411-416: yagni: marshal-indent-append-newline-write JSON record is duplicated 3x across this file. Extract one writeOwnershipRecordFile(path string, record *packageOwnershipRecord) error helper.
| } | ||
| } | ||
|
|
||
| func errorsForResourceDestination() error { |
There was a problem hiding this comment.
L92-94: yagni: errorsForResourceDestination() wraps a single static error with one caller pattern used 3x. Inline errors.New(...) at each call site or return it once from validateManifestResourceDestination.
…rship Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ADR Required — Architecture Decision Record MissingStatus: 🔴 Blocked — a draft ADR has been committed to this branch for your review before merging. This PR adds 856 new lines in business-logic directories ( A draft ADR has been generated and committed to the branch at: What to do next
Why ADRs MatterArchitecture Decision Records capture why a significant design choice was made — not just what was built. They help future contributors understand constraints, avoid re-litigating settled decisions, and safely evolve the system. See the ADR index for examples. ADR sections requiredThe draft follows the Michael Nygard template and must contain:
|
|
@copilot This PR still needs a maintainer-facing finish pass. Current blockers to address in one pass:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in I completed the finish pass items I could action in-agent:
Maintainer follow-up still required:
|
PR TriageCategory: feature | Risk: high | Priority: medium (score 50/100)
|
PR TriageCategory: Large feature addition (1026 additions, 18 files). Multiple unresolved CHANGES_REQUESTED reviews; mergeable_state blocked. Needs human review. Automated triage — see the [triage report issue] for full context.
|
Repository packages could install workflows, skills, and agents, but not related repository assets such as Issue Forms, CODEOWNERS, or policy files. This adds manifest-declared
resourcesplus package-scoped provenance so optional packages can coexist without overwriting or removing each other’s files.resources:mappings with package-relativesourceand repository-relativedestination..github/ISSUE_TEMPLATE/*.yml|*.yaml,.github/CODEOWNERS, and.github/aw/**.Install and ownership
.github/aw/packages/*.json.Update/remove safety
--forceis used.Docs and schema