Uh oh!
There was an error while loading. Please reload this page.
Refresh aw.yml package dependencies during update - #53974
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw update🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53974 does not have the implementation label and has 67 new lines of code in business logic directories, which is below the 100-line threshold.
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
✅ Ponytail Reviewer completed successfully! Ponytail over-engineering review of PR #53974: the diff adds a single thin wrapper (fetchManifestManagedDependencies) used at two call sites to avoid duplicating WorkflowSpec construction before calling the existing fetchAllRemoteDependencies. No dead code, speculative abstractions, reinvented stdlib, or unneeded dependencies found; test additions are proportionate to the new behavior. Lean already. Ship.
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
COMMENT — I did not find a changed-line bug severe enough to block this PR.
Highlights
- The new dependency refresh hook is wired into both update and add paths.
- The regression test exercises stale direct and transitive shared imports plus installation for a newly added workflow.
- I did not find a duplicated existing review comment to add, and there were no actionable blocking findings from my pass.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 6.33 AIC · ⌖ 6.81 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The change correctly wires dependency fetching into both the update and add paths for manifest-managed workflows. Test coverage is solid — it validates transitive imports and the new-workflow case.
One minor observation (non-blocking): fetchManifestManagedDependencies is called before the "already up-to-date" early-return check (line 218 vs line 222), so dependencies are always re-downloaded even for no-op runs. Consider moving the dependency fetch after the early-return guard to avoid redundant network calls when nothing has changed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.3 AIC · ⌖ 8.8 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Pull request overview
Refreshes package-managed workflow dependencies during gh aw update, including dependencies for newly added workflows.
Changes:
- Fetches and overwrites complete remote dependency closures.
- Adds regression coverage for direct, transitive, and new-workflow imports.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/update_manifest.go | Refreshes dependencies during package workflow updates and additions. |
pkg/cli/update_manifest_test.go | Tests stale and newly introduced dependency refreshes. |
Review details
Suppressed comments (2)
pkg/cli/update_manifest.go:218
- Dependencies may be changed here even when the package ref, path, and workflow entry point are unchanged. The fast path immediately below then returns without recompiling, so a branch update that only changes a shared dependency leaves the existing
.lock.ymlembedding stale content. Recompile after refreshing dependencies before taking that return path.
if err := fetchManifestManagedDependencies(ctx, newContent, update.repo, update.latestPath, update.latestRef, filepath.Dir(update.wf.Path), opts.Verbose); err != nil {
pkg/cli/update_manifest.go:340
- This dependency fetch runs after the new workflow has already been written. If a fatal dispatch, worker, or resource dependency error occurs, the operation is reported as failed but
<name>.mdremains installed. Fetch before writing the entry point (as the regular add path does), or track and roll back all writes on failure.
if err := fetchManifestManagedDependencies(ctx, newContent, repo, latestPath, latestRef, targetDir, opts.Verbose); err != nil {
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| }, | ||
| WorkflowPath: workflowPath, | ||
| } | ||
| return fetchAllRemoteDependencies(ctx, string(content), spec, targetDir, verbose, true, nil) |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix and its regression tests are solid; two ordering/consistency observations worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Eager dependency fetch:
fetchManifestManagedDependenciesruns before the "already up to date" guard inupdateManifestManagedWorkflow, performing unnecessary network I/O on no-op updates. - Content variable inconsistency:
addManifestManagedWorkflowpassesnewContent(raw download) tofetchManifestManagedDependencieswhile the frontmatter-modifiedcontentis what gets written to disk.
Positive Highlights
- ✅ Clean, well-scoped helper function
fetchManifestManagedDependenciesthat delegates tofetchAllRemoteDependencies. - ✅ Good regression tests covering stale direct imports, transitive imports, and newly added workflow dependencies.
- ✅ Proper save/restore of the
downloadRemoteImportFilefunction var in test cleanup.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.8 AIC · ⌖ 10 AIC · ⊞ 7.8K
Comment /matt to run again
| if err != nil { | ||
| return fmt.Errorf("failed to download workflow %s/%s@%s: %w", update.repo, update.latestPath, update.latestRef, err) | ||
| } | ||
| if err := fetchManifestManagedDependencies(ctx, newContent, update.repo, update.latestPath, update.latestRef, filepath.Dir(update.wf.Path), opts.Verbose); err != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] Dependencies are fetched before the "already up to date" early-return check (line 222), so every gh aw update run downloads the full dependency closure even when nothing changed — wasting network I/O and risking overwriting local modifications with no-op updates.
💡 Suggested fix
Move fetchManifestManagedDependencies to after the early-return guard so it only runs when an actual update is going to be applied:
if!opts.Force&&update.currentRef==update.latestRef&&... {
// ... up-to-date check ...returnnil
}
// Only fetch dependencies when an update is actually written.iferr:=fetchManifestManagedDependencies(...); err!=nil { ... }@copilot please address this.
| if err := os.WriteFile(destPath, []byte(content), constants.FilePermPublic); err != nil { | ||
| return fmt.Errorf("failed to write new manifest workflow %s: %w", destPath, err) | ||
| } | ||
| if err := fetchManifestManagedDependencies(ctx, newContent, repo, latestPath, latestRef, targetDir, opts.Verbose); err != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] In addManifestManagedWorkflow, fetchManifestManagedDependencies is called with newContent (the raw downloaded bytes) rather than content (the frontmatter-modified string written to disk). If fetchAllRemoteDependencies resolves import paths relative to the workflow content it parses, using the pre-modification bytes is consistent — but if it differs, this is a latent bug. Consider using the same content variable that was written to disk for correctness and consistency.
@copilot please address this.
PR Triage
Fixes gh aw update leaving package-owned shared dependencies stale by refreshing the full dependency closure.
|
gh-aw-bot
commented
Aug 19, 2026
@copilot This PR still needs a maintainer-facing finish pass.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
gh-aw-bot
commented
Aug 19, 2026
@copilot This PR looks close, but it still needs one maintainer-facing finish pass.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Validation: focused regression test, |
PR TriageCategory: chore · Risk: medium · Score: 58/100 (impact 25 + urgency 15 + quality 18) Dependency freshness fix, approved review, moderate footprint; good fast-track candidate pending CI. Automated triage — see run report for full details.
|
🎉 This pull request is included in a new release. Release: |
gh aw updaterefreshed package workflow entry points but left package-owned shared dependencies stale. Compiled workflows could therefore continue using outdated imported files.Dependency refresh
Regression coverage
gh aw updateleaves shared package dependencies stale #53932