Split update-actions implementation into focused modules#52084
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors action-update logic into focused modules while preserving public APIs.
Changes:
- Separates dependency caching, release resolution, and workflow rewriting.
- Retains lockfile orchestration in
update_actions.go. - Refreshes unrelated generated workflow version metadata.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/update_actions.go |
Retains lockfile update orchestration. |
pkg/cli/update_actions_deps.go |
Adds shared dependencies and caching. |
pkg/cli/update_actions_release.go |
Houses release and SHA resolution. |
pkg/cli/update_actions_workflow_refs.go |
Houses workflow reference rewriting. |
.github/workflows/daily-pr-review-cursor.lock.yml |
Updates generated Copilot version metadata. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| @@ -1,4 +1,4 @@ | |||
| # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"f51fba46e48ad3e580030bf320ba97a7c033bfb2eed6bb72a787807047d8694a","body_hash":"bf1cf21f4246f5ae31a56495c39972606baec878241331ad7f299696d6230278","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.78","copilot-sdk":"1.0.8"}} | |||
| # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"f51fba46e48ad3e580030bf320ba97a7c033bfb2eed6bb72a787807047d8694a","body_hash":"bf1cf21f4246f5ae31a56495c39972606baec878241331ad7f299696d6230278","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.79","copilot-sdk":"1.0.8"}} | |||
| } | ||
| } | ||
|
|
||
| // UpdateActions updates GitHub Actions versions in .github/aw/actions-lock.json |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 67 AIC · ⌖ 6.75 AIC · ⊞ 7.1K
Comment /matt to run again
| // the latest major version; pass disableReleaseBump=true to only update core | ||
| // (actions/*) references. | ||
| func UpdateActionsInWorkflowFiles(ctx context.Context, workflowsDir, engineOverride string, verbose, disableReleaseBump bool, noCompile bool, coolDown time.Duration, approve bool) error { | ||
| return updateActionsInWorkflowFiles(ctx, defaultActionUpdateDeps(), updateActionsOptions{ |
There was a problem hiding this comment.
[/codebase-design] UpdateActionsInWorkflowFiles passes defaultActionUpdateDeps() (uncached) to the inner function, bypassing the memoization layer built in update_actions_deps.go. When multiple workflow files reference the same actions, this triggers redundant GitHub API calls.
💡 Suggested fix
return updateActionsInWorkflowFiles(ctx, newCachedActionUpdateDeps(defaultActionUpdateDeps()), ...)update_command.go line 202 already does this correctly — the public entry point should match.
@copilot please address this.
| cooldowns := make(map[string]coolDownCheckResult) | ||
|
|
||
| cached := base | ||
| cached.getLatestRelease = func(ctx context.Context, repo, currentVersion string, allowMajor, verbose bool) (string, string, error) { |
There was a problem hiding this comment.
[/codebase-design] newCachedActionUpdateDeps memoizes getLatestRelease, runGHReleasesAPI, getActionSHAForTag, and checkCoolDown but not getLatestReleaseViaGit. The git-fallback path (triggered when the GitHub API returns an auth error or empty release list) bypasses the cache entirely — a single actions/checkout reference could trigger multiple uncached git ls-remote calls across workflow files.
💡 Suggested fix
Add a memoization wrapper for getLatestReleaseViaGit inside newCachedActionUpdateDeps, keyed on repo|currentVersion|allowMajor.
@copilot please address this.
| } | ||
| } | ||
|
|
||
| // UpdateActions updates GitHub Actions versions in .github/aw/actions-lock.json |
There was a problem hiding this comment.
[/codebase-design] The GoDoc block for UpdateActions is placed here in update_actions_deps.go at line 117, but the exported function lives in update_actions.go. Go's documentation tool (go doc) associates a comment block with the immediately following declaration — this comment is not adjacent to the function, so it is orphaned and won't appear in go doc output. The existing review comment on this line confirms the issue; the fix is straightforward.
💡 Suggested fix
Remove the comment block from update_actions_deps.go and place it directly above func UpdateActions in update_actions.go.
@copilot please address this.
|
|
||
| type skillRefUpdateResolver func(ctx context.Context, repo, currentRef string, allowMajor, verbose bool, coolDown time.Duration) (string, error) | ||
|
|
||
| func updateSkillRefsInContent(ctx context.Context, content string, allowMajor, verbose bool, coolDown time.Duration) (bool, string, error) { |
There was a problem hiding this comment.
[/codebase-design] updateSkillRefsInContent (and its WithResolver variant) does not accept deps actionUpdateDeps, so skill source: updates never use the shared memoized client — each skill ref lookup goes through its own uncached resolveLatestRef path. If a workflow file has both uses: and source: references to the same repo, the API is hit twice.
💡 Suggested fix
Thread deps actionUpdateDeps through updateSkillRefsInContent (and its resolver), or ensure resolveLatestRef internally uses the same cache. The simplest path is to pass deps from updateActionsInWorkflowFiles into updateSkillRefsInContentWithResolver.
@copilot please address this.
There was a problem hiding this comment.
Review: Split update-actions implementation into focused modules
The module split is a clean mechanical extraction. All three new files (update_actions_deps.go, update_actions_release.go, update_actions_workflow_refs.go) stay in the cli package so cross-file references to updateLog, cooldownLog, and helper functions resolve correctly. Logic is identical to what was removed from update_actions.go.
Two existing inline comments capture the actionable items already found:
- Truncated godoc comment at EOF of
update_actions_deps.go— theUpdateActionsdoc block was moved here but is now detached from theUpdateActionsfunction (which lives inupdate_actions.go) and ends mid-sentence. - Lock-file version bump in
daily-pr-review-cursor.lock.ymlis unrelated to this PR scope.
No additional blocking concerns found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 63.8 AIC · ⌖ 6.18 AIC · ⊞ 5.4K
Records the architectural decision to split pkg/cli/update_actions.go into four focused modules, preserving the public API unchanged.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (918 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you when they need to understand why the code is structured the way it is. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot Please address the open review feedback, refresh the branch if needed, and use the pr-finisher skill before handing back. Open review threads (newest reviewer feedback first):
No failed checks were reported in the compact candidate set for this run.
|
PR Triage: #52084
Automated triage — see labels for machine-readable classification.
|
pkg/cli/update_actions.goexceeded the repository file-size threshold and combined release resolution, caching, lockfile updates, and workflow reference rewriting. This refactor separates those responsibilities without changing the public API.Dependency and caching
update_actions_deps.gofor dependency injection and shared GitHub-read memoization.Release resolution
update_actions_release.gofor action classification, API/git release lookup, SHA resolution, and cooldown fallback.Lockfile updates
UpdateActionsand lockfile orchestration in the reducedupdate_actions.go.Workflow references
update_actions_workflow_refs.goforuses:and skillsource:updates in workflow Markdown.Run: https://github.com/github/gh-aw/actions/runs/31511130973> Generated by 👨🍳 PR Sous Chef · gpt54 · 8.27 AIC · ⌖ 5.37 AIC · ⊞ 8.5K · ◷