refactor: extract shared org-wide runner for update and upgrade commands#41553
Conversation
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 the org-wide update and upgrade CLI flows by extracting their shared “discover → filter → (optional scan) → sort → report → apply/issue with cancellation + rate-limit handling” algorithm into a new runCommandForOrg helper, and rewires both commands to use it.
Changes:
- Added
pkg/cli/org_runner.gowithorgRunCallbacksandrunCommandForOrgto centralize org discovery, cancellation, rate-limit handling, sorting, reporting, and per-repo error recovery. - Updated
runUpdateForOrgandrunUpgradeForOrgto delegate torunCommandForOrg(withScanFnenabled for update and disabled for upgrade). - Updated upgrade org behavior/tests to continue past per-repo failures and only error when all repos fail; also includes workflow
.lock.ymlupdates (model string normalization and a few run-command tweaks).
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/org_runner.go | New shared org-wide runner implementing discovery/scan/report/apply logic with cancellation and rate-limit handling. |
| pkg/cli/update_org.go | Refactors org update mode into callbacks + shared runner; keeps per-repo preview logic in ScanFn. |
| pkg/cli/upgrade_org.go | Refactors org upgrade mode into callbacks + shared runner (no scan phase). |
| pkg/cli/upgrade_org_test.go | Updates org upgrade tests to assert “skip failures, only error if all fail”. |
| .github/workflows/unbloat-docs.lock.yml | Updates generated workflow lock env (COPILOT_MODEL value normalization). |
| .github/workflows/spec-enforcer.lock.yml | Updates generated workflow lock env (COPILOT_MODEL value normalization). |
| .github/workflows/smoke-pi.lock.yml | Updates generated workflow lock env (COPILOT_MODEL value normalization). |
| .github/workflows/smoke-opencode.lock.yml | Updates generated workflow lock run command (adds cd $GITHUB_WORKSPACE) and related generated output. |
| .github/workflows/smoke-crush.lock.yml | Updates generated workflow lock run command (adds cd $GITHUB_WORKSPACE) and related generated output. |
| .github/workflows/smoke-antigravity.lock.yml | Updates generated workflow lock run command (adds cd $GITHUB_WORKSPACE) and related generated output. |
| .github/workflows/schema-consistency-checker.lock.yml | Updates generated workflow lock env (COPILOT_MODEL value normalization). |
| .github/workflows/poem-bot.lock.yml | Updates generated workflow lock env (COPILOT_MODEL value normalization). |
| .github/workflows/lint-monster.lock.yml | Updates generated workflow lock env (COPILOT_MODEL value normalization). |
| .github/workflows/issue-monster.lock.yml | Updates generated workflow lock env (COPILOT_MODEL value normalization). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 14/14 changed files
- Comments generated: 4
- Review effort level: Low
| if err != nil { | ||
| return err | ||
| } | ||
| orgUpdateLog.Printf("Previewing updates for repositories in %s", org) |
| // scanFn previews a single repo and decides whether to include it. | ||
| // It also prints per-repo progress to stderr so the user can see which | ||
| // workflows are being inspected. |
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Cancellation requested; stopping after %d/%d repositories", i, total))) | ||
| orgUpdateLog.Printf("Context canceled during scan at repo %d/%d: %v", i, total, ctx.Err()) | ||
| stopped = true | ||
| break |
| COPILOT_DUMMY_BYOK: dummy-byok-key-for-offline-mode | ||
| COPILOT_GITHUB_TOKEN: ${{ secrets.COPILOT_GITHUB_TOKEN }} | ||
| COPILOT_MODEL: copilot/gpt-5.4 | ||
| COPILOT_MODEL: gpt-5.4 |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (360 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Note on assertion messages: Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The updated test TestRunUpgradeForOrgSkipsFailedRepos correctly captures the new behavioral contract (skip failures, try all repos).
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture, /zoom-out, and /tdd — requesting changes on two gaps before merging.
📋 Key Themes & Highlights
Positive Highlights
✅ Clean extraction — the orgRunCallbacks struct + runCommandForOrg pattern elegantly removes ~220 lines of duplication while keeping both callers readable as thin wrappers.
✅ Good upgrade capability uplift — signal handling and skip-on-error are real improvements over the old stop-on-first-error behaviour, and the PR description table is a nice way to surface the delta.
✅ Test name communicates intent — TestRunUpgradeForOrgSkipsFailedRepos reads as a spec, which is exactly the /tdd spirit.
✅ Well-documented struct — every callback field has a doc comment explaining semantics, including the nil-means-skip contract for ScanFn.
Issues to Address
orgUpdateLogin shared code (org_runner.go:140) — upgrade org debug logs appear undercli:update_org, confusing future diagnostics. Low effort to fix.- Nil guards missing for required callbacks (
org_runner.go:98) —SearchFn/ReportFnare called unconditionally; a nil value panics. Worth guarding early. --create-issuebehavior change in upgrade is untested (upgrade_org.go:49) — the old code stopped on first error; the new shared loop skips and continues. This is the most important gap: the apply-path change has a test, the issue-path change does not.- Sort order for upgrade not pinned by a test (
upgrade_org_test.go:222) — alphabetical order is the effective guarantee; a test would prevent accidental regressions. createPR && createIssueboth-true contract not documented (org_runner.go:213) — a comment or guard would help future callers.- Log detail regression in
update_org.go:73— minor nit; the repo count was useful in debug output.
The blocking items are #2 (panic potential) and #3 (untested behavioral change). The others are non-blocking suggestions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 104.7 AIC · ⌖ 12.9 AIC · ⊞ 6.5K
| // the report for the work completed so far. | ||
| if ctx.Err() != nil { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Cancellation requested; stopping after %d/%d repositories", i, total))) | ||
| orgUpdateLog.Printf("Context canceled during scan at repo %d/%d: %v", i, total, ctx.Err()) |
There was a problem hiding this comment.
[/improve-codebase-architecture] orgUpdateLog is defined in update_org.go as logger.New("cli:update_org"). Using it in this shared file means all upgrade org operations emit debug logs under the cli:update_org namespace, making it harder to isolate upgrade-specific output when debugging.
💡 Suggestion
Move — or add — a neutral shared logger in org_runner.go:
var orgLog = logger.New("cli:org_runner")Then use orgLog throughout this file. update_org.go keeps orgUpdateLog for its own update-specific lines, and a future third org command won't silently inherit a misleading tag.
| } | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(discMsg)) | ||
|
|
||
| repoPaths, err := cbs.SearchFn(ctx, org, verbose) |
There was a problem hiding this comment.
[/improve-codebase-architecture] SearchFn and ReportFn are called unconditionally but the struct doesn't enforce that they're non-nil. A caller that accidentally omits either field will get a nil-pointer panic at runtime; the optional fields (ScanFn, etc.) are nil-checked but required ones aren't.
💡 Suggestion
Add a guard just after the existing validation:
if cbs.SearchFn == nil {
return errors.New("orgRunCallbacks.SearchFn must not be nil")
}
if cbs.ReportFn == nil {
return errors.New("orgRunCallbacks.ReportFn must not be nil")
}You could also add a doc comment on SearchFn and ReportFn marking them as required, and note that ApplyFn/IssueFn must be non-nil when createPR/createIssue are set — whichever approach fits the project's style.
| // the user see results even if the run is stopped early. | ||
| cbs.ReportFn(results, createPR || createIssue) | ||
|
|
||
| if !createPR && !createIssue { |
There was a problem hiding this comment.
[/zoom-out] When both createPR and createIssue are true, the if createIssue branch runs and returns without ever reaching the apply loop — only issues are created. This is safe because callers enforce mutual exclusion upstream, but runCommandForOrg doesn't document or assert this invariant. A future caller could easily be surprised.
💡 Suggestion
Either add an early guard:
if createPR && createIssue {
return errors.New("createPR and createIssue are mutually exclusive")
}Or at minimum add a comment above this block:
// Callers are expected to enforce mutual exclusion; if both are set,
// only the issue path runs.Making the contract explicit here helps the next reader understand the intent without tracing back to command-flag validation.
| ApplyFn: func(ctx context.Context, preview orgRepoPreview, v bool) error { | ||
| return runUpgradeForTargetRepoFn(ctx, preview.Repo, opts, v) | ||
| }, | ||
| IssueFn: func(ctx context.Context, preview orgRepoPreview, v bool) error { |
There was a problem hiding this comment.
[/tdd] The old runUpgradeForOrg stopped on the first createIssue error (return err); the new shared loop skips failures and only returns an error when every repo fails. This is a behavioral change for --create-issue that isn't mentioned in the PR description's before/after table and has no corresponding test.
💡 Suggestion
Add a test mirroring TestRunUpgradeForOrgSkipsFailedRepos for the issue-creation path:
func TestRunUpgradeForOrgCreateIssueSkipsFailedRepos(t *testing.T) {
// stub SearchFn, IssueFn to always fail
// assert all repos attempted
// assert error contains "failed to create issues in any repository"
}The update org equivalent is TestRunUpgradeForOrgCreateIssueReturnsErrorWhenAllIssueCreatesFail — a symmetrical test for upgrade closes the coverage gap and documents the new semantics.
| assert.Equal(t, []string{"octo/api"}, called, "should stop after first failure") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "failed to upgrade any repository") | ||
| assert.Equal(t, []string{"octo/api", "octo/web"}, called, "should attempt all repos and skip failures") |
There was a problem hiding this comment.
[/tdd] The PR description's table lists sorted-by-oldest-edit as a new capability for upgrade, but there's no test for it. With ScanFn == nil, all repos get zero OldestEdit, so the sort falls back to alphabetical order — which is still a meaningful, testable guarantee.
💡 Suggested test
func TestRunUpgradeForOrgSortsAlphabetically(t *testing.T) {
// ScanFn is nil → OldestEdit stays zero → ties broken alphabetically
searchOrgAnyWorkflowReposFn = func(...) ([]string, error) {
return []string{"octo/zoo", "octo/alpha", "octo/middle"}, nil
}
var called []string
runUpgradeForTargetRepoFn = func(_ context.Context, repo string, _ upgradeOptions, _ bool) error {
called = append(called, repo)
return nil
}
...
_ = runUpgradeForOrg(context.Background(), "octo", nil, upgradeOptions{ctx: context.Background()}, true, false, false)
assert.Equal(t, []string{"octo/alpha", "octo/middle", "octo/zoo"}, called)
}This pins the deterministic ordering as a regression guard without needing real timestamps.
| if err != nil { | ||
| return err | ||
| } | ||
| orgUpdateLog.Printf("Previewing updates for repositories in %s", org) |
There was a problem hiding this comment.
[/improve-codebase-architecture] The previous log line was "Previewing updates for %d repositories in %s", total, org — the count was useful for correlating log output with how many repos were scanned. The refactor drops it because total is no longer available at this call site.
💡 Suggestion (optional)
This is a low-severity nit. One option is to log the count inside runCommandForOrg after filtering (the runner already has len(repos) at that point). Alternatively, accept the regression as a trade-off of the extraction — the user-visible progress output still shows [n/total] per repo.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
REQUEST_CHANGES — one blocking issue in runCommandForOrg, plus a description inaccuracy.
Missing nil guards on required callbacks
runCommandForOrg calls cbs.SearchFn, cbs.ReportFn, cbs.ApplyFn, and cbs.IssueFn without any nil checks. The struct documentation marks only ScanFn as optional, leaving the other four implicitly required but unenforced. Both current callers set all callbacks correctly, so this will not panic today — but the abstraction is designed for reuse, and a future caller that omits a required field will trigger a runtime nil function-call panic with no helpful error context. Add four nil checks immediately after validateRepoGlobs (see inline comment on line 98).
Minor observation: upgrade sort is alphabetical, not by oldest edit
When ScanFn is nil (upgrade command), all orgRepoPreview entries are built with OldestEdit at its zero value. The sort comparator first branch — if a.OldestEdit.IsZero() && b.OldestEdit.IsZero() — always fires, giving purely alphabetical ordering, not oldest-edit ordering. Deterministic and harmless, but the PR description table entry "sorted by oldest edit" for upgrade is inaccurate; the actual ordering is alphabetical for any command using ScanFn = nil.
🔎 Code quality review by PR Code Quality Reviewer · 199 AIC · ⌖ 9.36 AIC · ⊞ 5.2K
| } | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(discMsg)) | ||
|
|
||
| repoPaths, err := cbs.SearchFn(ctx, org, verbose) |
There was a problem hiding this comment.
Missing nil guards on required callbacks will cause runtime panics: cbs.SearchFn is called here without a nil check; the same applies to cbs.ReportFn (line 211), cbs.IssueFn (line 240, when createIssue=true), and cbs.ApplyFn (line 280, when createPR=true). Only ScanFn is documented as optional, but the other four are never validated before use. Both current callers are complete, so this will not panic today — but as this shared runner gains more callers, a missing callback will produce a cryptic nil function-call panic instead of a clear error.
💡 Suggested fix
Add a validation block immediately after the existing validateRepoGlobs check (line 83):
if cbs.SearchFn == nil {
return errors.New("orgRunCallbacks.SearchFn is required")
}
if cbs.ReportFn == nil {
return errors.New("orgRunCallbacks.ReportFn is required")
}
if createPR && cbs.ApplyFn == nil {
return errors.New("orgRunCallbacks.ApplyFn is required when createPR is true")
}
if createIssue && cbs.IssueFn == nil {
return errors.New("orgRunCallbacks.IssueFn is required when createIssue is true")
}Alternatively, add // Required. to the struct field godocs for SearchFn, ReportFn, ApplyFn, and IssueFn to make the contract explicit at the declaration site.
| } | ||
|
|
||
| if len(results) == 0 { | ||
| if stopped { |
There was a problem hiding this comment.
Partial scan with non-zero results silently falls through to the apply/issue loop: when the scan is stopped early (stopped=true) due to a rate-limit-critical condition but len(results) > 0, the stopped flag is only checked inside if len(results) == 0. Code falls straight through to cbs.ReportFn and the createPR/createIssue apply loops with no indication that the scan was incomplete. In the rate-limit-critical case the context is NOT cancelled, so waitForOrgRateLimitFn in the apply loop is the only thing that prevents PR/issue creation — and if the limit has reset (mocked in tests, or on a long run straddling the reset window), PRs or issues will be created for only the partial subset of repos without any user warning.
💡 Suggested fix
Check stopped before entering the apply loops, not just inside the empty-results guard:
if len(results) == 0 {
if stopped { /* NoResultsStopMsg */ }
/* NoResultsMsg */
return nil
}
// NEW: warn about partial scan even when results are non-empty
if stopped {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Scan was stopped early; report and any subsequent operations cover only the repositories inspected so far"))
}Alternatively, when stopped=true skip the apply/issue loops entirely (return after the report) to avoid acting on a partial scan.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented a pr-finisher pass in commit |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey The PR is well-packaged: an ADR in
|
runUpdateForOrgandrunUpgradeForOrgduplicated the same structural algorithm — discover repos, filter, loop with rate limiting, report, apply/issue. This extracts that algorithm into a sharedrunCommandForOrgand rewires both commands through it.New:
pkg/cli/org_runner.goIntroduces
orgRunCallbacksandrunCommandForOrg. All structural concerns live here:ScanFn;nil= include all discovered repos without scanning)update_org.gorunUpdateForOrgbecomes a thin wrapper: buildsorgRunCallbackswith the update-specificScanFn(wrapspreviewOrgRepoUpdates) and delegates torunCommandForOrg. Behavior unchanged.upgrade_org.gorunUpgradeForOrgdelegates withScanFn = nil— no per-repo API scan needed. Upgrade org mode gains capabilities it previously lacked:upgrade_org_test.goTestRunUpgradeForOrgStopsOnFirstError→TestRunUpgradeForOrgSkipsFailedRepos, updated to assert all repos are attempted and an error is only returned when every one fails.