Import and merge on.bots from shared workflows - #57968
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot investigate the top level bots. If it is a legacy field, mark as error and ensure codemod upgrades to on.bot |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 8678746 and 1ed23d7. Top-level |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. 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
- "proxy.golang.org"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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
- "proxy.golang.org"See Network Configuration for more information.
|
|
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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "raw.githubusercontent.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
ADR RequiredI generated a draft ADR for this PR because ADR enforcement applies here: the PR adds more than 100 lines in business-logic directories ( Evidence reviewed
Draft ADR added
Next actionPlease review and refine the draft ADR so it accurately captures the intended long-term design, then keep it with the PR as the architectural record for this change. 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
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
🟡 Changes recommended
The codemod can select nested fields, corrupt lists containing blank lines, and discard comments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Corrects imported bot allowlists and updates legacy migration behavior.
Changes:
- Extract and merge imported
on.botsvalues. - Reject legacy top-level
bots. - Extend regression and codemod coverage.
File summaries
| File | Description |
|---|---|
pkg/parser/import_field_extractor.go |
Reads imported bots from on. |
pkg/parser/schema_test.go |
Tests top-level bots rejection. |
pkg/workflow/bots_test.go |
Covers imported bot merging. |
pkg/cli/codemod_bots.go |
Merges legacy and current bot lists. |
pkg/cli/codemod_bots_test.go |
Tests codemod merging. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
| for i := start; i < end; i++ { | ||
| line := lines[i] | ||
| lineIndent := len(getIndentation(line)) | ||
| if (!nested && lineIndent != indent) || (nested && lineIndent <= indent) || !strings.HasPrefix(strings.TrimSpace(line), "bots:") { |
| continue | ||
| } | ||
| blockEnd := i | ||
| for j := i + 1; j < end && isNestedUnder(lines[j], getIndentation(line)); j++ { |
| if (i >= topBotsStart && i <= topBotsEnd) || (i >= onBotsStart && i <= onBotsEnd) { | ||
| continue |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
A couple of spots are doing more parsing and setup than the surrounding code needs; trimming them would simplify the patch without changing behavior. net: -36 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57968 · codex · mai10 · 8.79 AIC · ⌖ 0.589 AIC · ⊞ 12.8K
Comment /ponytail to run again
| return merged, true | ||
| } | ||
|
|
||
| func mergeLegacyBotsLines(lines []string, bots []string) ([]string, bool) { |
There was a problem hiding this comment.
pkg/cli/codemod_bots.go:67-124: yagni: 60-line custom YAML block finder/rewriter for a single merge. The existing plus a tiny post-transform would avoid introducing a second parser and JSON serialization path.
| return merged, true | ||
| } | ||
|
|
||
| func mergeLegacyBotsLines(lines []string, bots []string) ([]string, bool) { |
There was a problem hiding this comment.
pkg/cli/codemod_bots.go:67-124: yagni: 60-line custom YAML block finder/rewriter for a single bots merge. The existing newMoveTopLevelKeyToOnBlockCodemod plus a tiny post-transform would avoid introducing a second parser and JSON serialization path.
| compiler := NewCompiler() | ||
|
|
||
| t.Run("imported_bots_merged_with_top_level_bots", func(t *testing.T) { | ||
| t.Run("imported_bots_merged_with_main_workflow_bots", func(t *testing.T) { |
There was a problem hiding this comment.
pkg/workflow/bots_test.go:333-455: shrink: three subtests repeat the same tempdir/file-writing scaffolding. A single table-driven case with a tiny helper would remove most of the boilerplate.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — this is a well-scoped, well-tested bug fix (union merge for on.bots from imports, plus a codemod for legacy top-level bots merging into an existing on.bots). Reused existing helpers (extractOnSectionFieldFromMap, mergeUniqueLogged) consistent with the skip-roles/skip-bots patterns already in the codebase — good architectural consistency per /codebase-design.
📋 Key Themes & Highlights
Key Themes
mergeLegacyBotsincodemod_bots.goassumes bothon.botsand top-levelbotsare[]any; a scalar string value for either (if still schema-permitted) causes a silent no-op rather than a successful migration or explicit error.- Line-splicing logic in
mergeLegacyBotsLinesis only exercised by one scenario (bots:as the last key inon:); an additional case withbots:first underon:plus trailing sibling keys would harden confidence in the block-removal logic.
Positive Highlights
- ✅ Fixing
mergeBotsto source fromon.botsmirrors the establishedskip-roles/skip-botsmerge pattern exactly — no new abstractions introduced. - ✅ New/updated tests (
TestBotsToOnBotsCodemod_MergesWhenOnBotsExists,TestBotsImportMergevariants) clearly assert the new merged/deduped behavior. - ✅ Schema test addition (
botsnow rejected at top level) closes the gap the bug depended on.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 54.4 AIC · ⌖ 15.8 AIC · ⊞ 10.3K
Comment /matt to run again
| return baseApply(content, frontmatter) | ||
| } | ||
|
|
||
| mergedBots, ok := mergeLegacyBots(onBots, topBots) |
There was a problem hiding this comment.
[/tdd] mergeLegacyBots only handles []any bot lists; if either on.bots or top-level bots is a bare scalar string (which the schema likely still permits for single-bot shorthand), it returns ok=false and the codemod silently no-ops, leaving the legacy top-level bots field unmigrated with no error surfaced to the user.
💡 Suggested coverage
Add a case in TestBotsToOnBotsCodemod_* where on.bots or top-level bots is a scalar string (e.g. bots: renovate[bot]) to confirm the intended fallback behavior, and consider normalizing scalars to a single-element list before merging so the migration still succeeds instead of silently skipping.
@copilot please address this.
| return merged, true | ||
| } | ||
|
|
||
| func mergeLegacyBotsLines(lines []string, bots []string) ([]string, bool) { |
There was a problem hiding this comment.
[/tdd] mergeLegacyBotsLines covers the case where on: contains other keys before/after bots:, but there is no test for bots: being the first key under on: (i.e. onBotsStart == onStart+1) combined with additional sibling keys after it in on: — worth a regression test since the block-removal/reinsertion logic (topBotsStart/topBotsEnd vs onBotsStart/onBotsEnd splicing) is easy to get subtly wrong with multi-line array removal.
💡 Suggested test
content := `---
on:
bots:
- dependabot
issues:
types: [opened]
bots:
- renovate
---
`Verify the merged bots: line lands correctly under on: and issues: is preserved untouched.
@copilot please address this.
There was a problem hiding this comment.
Reviewed the on.bots merge fix. The parser change correctly switches to extractOnSectionFieldFromMap (consistent with the existing pattern used for skip-roles/skip-bots), and the codemod now properly merges legacy top-level bots into on.bots instead of silently dropping it. The merge/line-transform helpers (mergeLegacyBots, mergeLegacyBotsLines, findBotsBlock) are type-safe and fail closed (return unmodified) on unexpected shapes. Good regression coverage for imported-only, combined, and overlapping allowlist scenarios, plus the schema test addition for rejecting top-level bots. No blocking issues found.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 40.7 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
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. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "raw.githubusercontent.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Request changes
The import-side fix is fine, but the codemod now takes on a much riskier job: rewriting frontmatter when both legacy bots and on.bots exist. That new path is driven by indentation-sensitive line surgery rather than the parsed YAML structure, so it is easy to skip or mis-shape valid files that do not match the exact formatting this helper expects.
Blocking theme
The parser change addresses the real bug. The codemod change adds a second behavior surface that is substantially more fragile than the existing move-to-on helpers, yet the tests only cover one happy-path formatting shape. That is not enough confidence for a migration tool that rewrites user-authored workflow frontmatter.
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
api.github.comgithub.comraw.githubusercontent.comregistry.npmjs.org
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "raw.githubusercontent.com"
- "registry.npmjs.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 37.7 AIC · ⌖ 7.4 AIC · ⊞ 23.5K
Comment /review to run again
| mergeJSONStringListField(fm, "bots", "[]", acc.botsSet, &acc.bots, func(m map[string]any, field string) (string, error) { | ||
| return extractFieldJSONFromMap(m, field, "[]") | ||
| }) | ||
| mergeJSONStringListField(fm, "bots", "[]", acc.botsSet, &acc.bots, extractOnSectionFieldFromMap) |
There was a problem hiding this comment.
This change breaks imported expression-based bot allowlists entirely: mergeBots now reads only on.bots, so shared workflows that still expose legacy top-level bots are ignored and the compiled guard falls back to author_association, which can reject valid bot-authored comments before membership checks ever run.
💡 Why this blocks the merge
The regression is already visible in TestCommentAuthorAssociationImportedExpressionBot: the imported "${{ vars.TRUSTED_BOT }}" bot never reaches GH_AW_ALLOWED_BOTS, so pre_activation.if keeps the static author_association gate. That changes runtime behavior, not just migration output.
A minimal fix is to keep import extraction backward-compatible for shared workflows by reading on.bots first and falling back to the legacy top-level bots field when importing, at least until old shared components have been codemodded.
|
@copilot Please address the remaining blocking codemod feedback before this PR moves forward.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed: the merge path now ignores descendant |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
|
@copilot legacy top level bots is not supported anymore. Users are expected to run the codemod |
|
@copilot update teh codemod implementation to be more reliable. |
Removed the import fallback for legacy top-level |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Shared workflows could not contribute
on.botsallowlists because imports incorrectly readbotsfrom the top level.Changes
on.botspath.