Update gh aw update --org to support workflow-targeted updates and repo prefiltering#41617
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw update --org to support workflow-targeted updates and repo prefiltering
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
❌ Test Quality Sentinel failed during test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This pull request updates gh aw update --org so that positional workflow arguments are honored in org mode, narrowing both repository discovery and per-repo scanning to only the requested workflow(s). This reduces unnecessary work by prefiltering candidate repositories via workflow-aware GitHub code search queries.
Changes:
- Thread
UpdateWorkflowsOptions.WorkflowNamesthrough org-mode repo discovery and per-repo workflow scanning. - Optimize org repo discovery by generating deterministic code-search queries with
filename:filters for requested workflows. - Add unit tests covering query generation and workflow-filter propagation through the org update flow.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/update_org.go | Adapts org-mode discovery and per-repo scanning to respect WorkflowNames. |
| pkg/cli/update_org_test.go | Updates stubs for the new search signature and adds a test verifying workflow filters are passed through. |
| pkg/cli/update_org_search.go | Extends org repo discovery to accept workflow filters and builds a deterministic OR-based filename: query. |
| pkg/cli/update_org_search_test.go | Adds focused tests for buildOrgWorkflowSearchQuery output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
| return runCommandForOrg(ctx, org, repoGlobs, orgRunCallbacks{ | ||
| SearchFn: searchOrgWorkflowReposFn, | ||
| SearchFn: searchFn, | ||
| ScanFn: scanFn, | ||
| ReportFn: renderOrgPreviewReport, | ||
| ApplyFn: func(ctx context.Context, preview orgRepoPreview, v bool) error { |
Generated by design-decision-gate agent from PR #41617 diff. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (≈106 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
📊 Metrics & Test Classification (2 new tests analyzed)
Go: 2 new (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /grill-with-docs, and /zoom-out — overall a clean, well-structured feature. Six observations posted as inline comments; none are blocking.
📋 Key Themes & Highlights
Key Themes
- Documentation gaps:
buildOrgWorkflowSearchQueryhas no doc comment, andsearchOrgWorkflowRepos's docstring doesn't mention the narrowing behaviour ofworkflowNames. ThesearchFnclosure adapter also lacks an explanatory comment. - Test coverage: Edge cases for
buildOrgWorkflowSearchQuery(single name, all-empty names) are untested.TestRunUpdateForOrgPassesWorkflowNameFiltersverifies propagation but not the filter effect at thepreviewOrgRepoUpdateslevel. - Semantic shift:
TotalWorkflowsnow reflects the post-filter count whenWorkflowNamesis specified, quietly changing the meaning of the progress log line ("%d workflow(s)"). Worth a rename or doc note.
Positive Highlights
- ✅ Clean closure adapter pattern to keep
orgRunCallbacks.SearchFnbackward-compatible with the upgrade command - ✅ Deterministic, sorted OR-query construction with deduplication is a nice touch
- ✅ Both new tests (
TestBuildOrgWorkflowSearchQueryandTestRunUpdateForOrgPassesWorkflowNameFilters) are well-scoped and easy to follow - ✅ Fully backward-compatible: empty
WorkflowNamesproduces the original search query unchanged
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 92.6 AIC · ⌖ 9.99 AIC · ⊞ 6.5K
Comments that could not be inline-anchored
pkg/cli/update_org_search_test.go:11
[/tdd] Three edge cases are missing from this test: a single-workflow input (no OR combinator, but parentheses still wrap it), all-invalid names where every entry normalizes to empty (exercises the len(filenameFilters)==0 fallback on line 62), and a workflow name that is a full path (e.g. .github/workflows/triage.md).
<details>
<summary>💡 Suggested additions</summary>
// Single name — one filename filter, parentheses still present
assert.Equal(t,
`org:octo path:.github/work…
</details>
<details><summary>pkg/cli/update_org_search.go:42</summary>
**[/grill-with-docs]** `buildOrgWorkflowSearchQuery` is undocumented. Given its non-trivial contract — normalization, deduplication, deterministic sort, and base-query fallback — a doc comment would help future maintainers understand when the fallback fires and why the OR filters are sorted.
<details>
<summary>💡 Suggested doc comment</summary>
```go
// buildOrgWorkflowSearchQuery constructs a GitHub code-search query that finds
// .md files under .github/workflows/ with a "source:" field in …
</details>
<details><summary>pkg/cli/update_org_search.go:37</summary>
**[/grill-with-docs]** The existing doc comment for `searchOrgWorkflowRepos` doesn't mention the new `workflowNames` parameter or its narrowing effect. A reader looking at the function signature won't know whether an empty slice means "match all" or "match none".
<details>
<summary>💡 Suggested addition to the doc comment</summary>
Add a sentence after the existing description:// When workflowNames is non-empty, the search is narrowed to repos that contain
// at least one of the named w…
pkg/cli/update_org_test.go:95
[/tdd] This test verifies that WorkflowNames is propagated to both the search and preview functions, which is valuable. However, the core claim in the PR description — "non-requested workflows are skipped when scanning a target repository" — isn't exercised here because previewOrgRepoUpdatesFn is mocked out entirely.
A complementary integration-style unit test that calls the real previewOrgRepoUpdates (with a fake checkout) and checks that a workflow absent from WorkflowNames is …
pkg/cli/update_org.go:73
[/zoom-out] The closure adapts searchOrgWorkflowReposFn's updated three-parameter signature to orgRunCallbacks.SearchFn's two-parameter interface. Without a comment this looks like unnecessary indirection — future readers may wonder why the function isn't assigned directly.
<details>
<summary>💡 Suggested inline comment</summary>
// Wrap searchOrgWorkflowReposFn to adapt its (org, workflowNames, verbose)
// signature to the (org, verbose) SearchFn interface expected by
// runCom…
</details>
<details><summary>pkg/cli/update_org.go:158</summary>
**[/grill-with-docs]** `TotalWorkflows` is now assigned `len(workflows)` after `findWorkflowsWithSource` has already applied the `WorkflowNames` filter. When the user specifies target workflows, `TotalWorkflows` will reflect the *filtered* count, not the total number of source-managed workflows in the repo.
This silently changes the meaning of the field and the log message at line 84 (`"%s: %d workflow(s), %d with updates"`), which now reads as the number of *requested* workflows rather than t…
</details>
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
Non-blocking observations
The change is well-scoped. Workflow name filters are correctly threaded through both the search query (as filename: predicates) and the per-repo scan (findWorkflowsWithSource), so filtering is enforced at two independent layers. The new tests cover query generation, dedup, and filter propagation.
Issues found (2, both non-blocking)
-
Silent fallback in
buildOrgWorkflowSearchQuery(update_org_search.go:64): when all workflow names normalize to empty, the function returns the base (unfiltered) query with no warning. Correctness is still preserved by the per-repo scan, but it silently wastes API search quota. Inline comment added. -
Missing boundary tests in
TestBuildOrgWorkflowSearchQuery(update_org_search_test.go:29):[]string{}and[]string{""}(all-empty-normalization path) are not covered. Inline comment added.
🔎 Code quality review by PR Code Quality Reviewer · 106.2 AIC · ⌖ 7.3 AIC · ⊞ 5.2K
| } | ||
| if len(filenameFilters) == 0 { | ||
| return base | ||
| } |
There was a problem hiding this comment.
Silent fallback to full-org scan when all workflow names normalize to empty: when workflowNames is non-empty but every entry reduces to "" via normalizeWorkflowID, the function silently returns the base query — no filename predicates, no warning — causing searchOrgReposByQuery to scan the entire org.
💡 Details and mitigation
Correctness is preserved by the backstop in previewOrgRepoUpdates: even if the search over-fetches repos, findWorkflowsWithSource re-filters them using opts.WorkflowNames. So no wrong updates are applied. The risk is pure API quota waste (an org-wide search instead of a targeted one) with no signal to the caller.
This is hard to reach in practice since CLI validation blocks empty names upstream, but it is worth hardening:
if len(filenameFilters) == 0 {
// All provided names normalized to empty; caller should not hit this path.
// Fall back to broad search and let per-repo scan enforce the filter.
return base
}A fmt.Fprintln(os.Stderr, console.FormatWarningMessage("...")) or debug log here would make unexpected fallback observable without blocking the operation.
| `org:octo path:.github/workflows extension:md "source:" (filename:repo-assist.md)`, | ||
| buildOrgWorkflowSearchQuery("octo", []string{"repo-assist", ".github/workflows/repo-assist.md"}), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Missing boundary test for []string{} (empty slice) and all-empty-normalization path: the test covers nil, two names, and dedup, but not []string{} or a slice where every entry normalizes to "" (e.g. []string{""}).
💡 Suggested additions
// empty slice → base query (same as nil)
assert.Equal(t,
`org:octo path:.github/workflows extension:md "source:"`,
buildOrgWorkflowSearchQuery("octo", []string{}),
)
// all entries normalize to empty → base query (silent fallback path)
assert.Equal(t,
`org:octo path:.github/workflows extension:md "source:"`,
buildOrgWorkflowSearchQuery("octo", []string{""}),
)These cases exercise the if len(filenameFilters) == 0 { return base } guard at line 62, which is the silent-fallback path discussed in the adjacent comment.
|
@copilot run pr-finisher skill |
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>
Completed a pr-finisher pass in 10400da: added the fallback clarification in |
update --orgpreviously treated org mode as all-or-nothing across source-managed workflows. This change makes positional workflow arguments effective in org mode: only requested workflows are considered for update, and repo discovery is narrowed to repositories that contain those workflow files.Scope behavior in
--orgmodeUpdateWorkflowsOptions.WorkflowNamesthrough org-mode discovery and per-repo preview.Repo discovery optimization
Search/query plumbing
searchOrgWorkflowReposto accept workflow filters.buildOrgWorkflowSearchQueryto normalize IDs, dedupe, and emit deterministic OR-based filename filters.Coverage updates