Add fix codemods for toolset typo and allowed-repos: current legacy alias - #50841
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
toolset typo and allowed-repos: current legacy alias
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. 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. |
|
✅ Test Quality Sentinel completed test quality analysis. 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. |
|
✅ 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. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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. |
There was a problem hiding this comment.
Pull request overview
Adds two gh aw fix codemods for unambiguous GitHub tool configuration migrations.
Changes:
- Renames singular
toolsetkeys totoolsets. - Replaces
allowed-repos: currentwith${{ github.repository }}. - Registers and tests both codemods.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/fix_codemods.go |
Registers both codemods. |
pkg/cli/fix_codemods_test.go |
Updates registry expectations. |
pkg/cli/codemod_toolset_singular.go |
Implements the toolset rename. |
pkg/cli/codemod_toolset_singular_test.go |
Tests singular-key migration. |
pkg/cli/codemod_allowed_repos_current.go |
Implements legacy alias migration. |
pkg/cli/codemod_allowed_repos_current_test.go |
Tests value rewriting and preservation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (2)
pkg/cli/codemod_toolset_singular.go:99
- This block detector accepts any nested
github:key belowtools, not only the directtools.githubentry. For example, with a custom tool containingcustom.github.toolsetplus the realtools.github.toolset, this codemod renames both fields even though the precondition only inspected the real GitHub tool. Track the direct-child indentation (or use a YAML-node-aware transform) before entering the GitHub block so unrelated custom-tool configuration is never rewritten.
if inTools && strings.HasPrefix(trimmed, "github:") {
pkg/cli/codemod_allowed_repos_current.go:104
- This also enters any nested
github:mapping under a custom tool. If bothtools.custom.github.allowed-repos: currentand the intendedtools.github.allowed-repos: currentexist, the transform rewrites both, despite the frontmatter precondition being scoped to the direct GitHub tool. Restrict detection to the direct child oftoolsso the codemod remains conservative.
if inTools && strings.HasPrefix(trimmed, "github:") {
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
| ID: "toolset-singular-to-toolsets", | ||
| Name: "Rename 'tools.github.toolset' to 'tools.github.toolsets'", | ||
| Description: "Renames the mistyped singular 'toolset:' field to the correct plural 'toolsets:' inside the tools.github configuration block.", | ||
| IntroducedIn: "1.0.0", |
| ID: "allowed-repos-current-to-github-repository", | ||
| Name: "Migrate 'tools.github.allowed-repos: current' to '${{ github.repository }}'", | ||
| Description: "Rewrites the legacy 'current' alias for tools.github.allowed-repos to the accepted '${{ github.repository }}' expression.", | ||
| IntroducedIn: "1.0.0", |
| getToolsetSingularToToolsetsCodemod(), // Rename mistyped tools.github.toolset to tools.github.toolsets | ||
| getAllowedReposCurrentToGitHubRepositoryCodemod(), // Migrate legacy tools.github.allowed-repos: current to ${{ github.repository }} |
|
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. Test Quality Sentinel 🧪Score: 94/100 — ✅ Excellent SummaryThis PR adds two new codemods ( Metrics:
Detailed AnalysisTestAllowedReposCurrentToGitHubRepositoryCodemod (8 subtests)
TestToolsetSingularToToolsetsCodemod (5 subtests)
fix_codemods_test.go modifications
Strengths
No Violations Detected✅ All required Go test build tags present ( Verdict✅ APPROVE — This PR demonstrates exceptional test quality. The test suite is comprehensive, behaviors are well-defined with strong design invariants, and no guideline violations exist. The codemods address real compatibility issues (typo migration and legacy alias support) with thorough verification.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Clean implementation following established codemod patterns. Both codemods are correct and idempotent:
- Ordering:
github-repos-to-allowed-reposruns beforeallowed-repos-current-to-github-repository— chaining works correctly for inputs that have both issues. - No false positives:
findAndReplaceInLine(line, "toolset", "toolsets")checks for prefixtoolset:which does not matchtoolsets:, so already-migrated files are safely skipped. - Comment safety: Lines trimmed to start with
#do not matchHasPrefix(trimmed, "toolset:")— comments are never modified. - Trailing comment preservation:
findTrailingCommentIndexcorrectly identifies comment-start#only when preceded by whitespace or at position 0. - One non-blocking inline note on the
IntroducedInversion placeholder.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 44.2 AIC · ⊞ 5.4K
| ID: "toolset-singular-to-toolsets", | ||
| Name: "Rename 'tools.github.toolset' to 'tools.github.toolsets'", | ||
| Description: "Renames the mistyped singular 'toolset:' field to the correct plural 'toolsets:' inside the tools.github configuration block.", | ||
| IntroducedIn: "1.0.0", |
There was a problem hiding this comment.
The IntroducedIn value "1.0.0" is a placeholder and does not reflect the actual release shipping this codemod. Other recent codemods also use "1.0.0", so this appears to be an established pattern — but if the version matters for gh aw fix --since filtering, consider setting it to the real release version. Non-blocking. @copilot please address this.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: non-blocking, follows existing precedent but with real gaps
The two new codemods correctly mirror the established pattern used by codemod_github_repos.go (parsed-frontmatter guard + line-based rewrite), and test coverage is solid for the documented block-style cases (quoting, comments, embedded #, no-ops). No correctness bugs found in the mainline block-style path.
Themes
- Both new codemods share a structural limitation with prior codemods: the parsed-frontmatter check and the line-based text rewrite are independently implemented and can disagree on flow-style/list-based YAML, producing a silent
applied:falseno-op with no error surfaced. This is pre-existing risk, not a new regression, but it is unverified by any test in this PR. codemod_toolset_singular.goduplicates ~60 lines of scanning logic fromcodemod_allowed_repos_current.gonearly verbatim - worth extracting to a shared helper before a third similar codemod is added.- Test coverage is good for the scenarios that matter most for this PR's stated purpose (typo migration + legacy alias rewrite), and edge cases around comment/quote handling are well tested.
|
|
||
| for i, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
|
|
||
| // Skip empty lines without resetting state | ||
| if trimmed == "" { | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| // Exit blocks when indentation signals we've left them | ||
| if !strings.HasPrefix(trimmed, "#") { | ||
| if inToolsGithub && hasExitedBlock(line, toolsGithubIndent) { | ||
| inToolsGithub = false | ||
| } | ||
| if inTools && hasExitedBlock(line, toolsIndent) { | ||
| inTools = false | ||
| inToolsGithub = false | ||
| } | ||
| } | ||
|
|
||
| // Detect 'tools:' block | ||
| if strings.HasPrefix(trimmed, "tools:") { | ||
| inTools = true | ||
| inToolsGithub = false | ||
| toolsIndent = getIndentation(line) | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| // Detect 'github:' block inside 'tools:' | ||
| if inTools && strings.HasPrefix(trimmed, "github:") { | ||
| inToolsGithub = true | ||
| toolsGithubIndent = getIndentation(line) | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| // Rewrite the value of 'allowed-repos: current' when inside tools.github | ||
| if inToolsGithub && strings.HasPrefix(trimmed, "allowed-repos:") { | ||
| lineIndent := getIndentation(line) | ||
| if isDescendant(lineIndent, toolsGithubIndent) { | ||
| if newLine, replaced := replaceAllowedReposCurrentLineValue(line); replaced { | ||
| result = append(result, newLine) | ||
| modified = true | ||
| allowedReposCurrentCodemodLog.Printf("Migrated 'allowed-repos: current' on line %d", i+1) | ||
| continue | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result = append(result, line) | ||
| } | ||
|
|
||
| return result, modified | ||
| } | ||
|
|
||
| // replaceAllowedReposCurrentLineValue replaces the value of an 'allowed-repos:' line with | ||
| // '${{ github.repository }}' if the current value (unquoted, single-quoted, or double-quoted) | ||
| // is 'current'. Preserves indentation and trailing comments. | ||
| func replaceAllowedReposCurrentLineValue(line string) (string, bool) { |
There was a problem hiding this comment.
The parsed-frontmatter check (hasAllowedReposCurrentValue) and the line-based rewriter (rewriteAllowedReposCurrentValue) use independent detection logic, so a case the map-based check flags as needing migration can still be silently skipped by the line scanner.
💡 Detail
hasAllowedReposCurrentValue understands any YAML shape the parser can produce (flow-style tools: {github: {allowed-repos: current}}, block sequences, etc.), but rewriteAllowedReposCurrentValue only recognizes classic block-style mappings where tools: and github: each start their own line at the expected indentation. If a workflow uses flow-style mapping or a list-based tools block, hasAllowedReposCurrentValue returns true, Apply proceeds to the line transform, but the transform never matches any line and returns modified == false. The Codemod.Apply then returns applied: false, err: nil — a silent no-op for a config the codemod itself already detected as needing migration, with no error, log entry, or indication to the caller/user that the fix was skipped.
This mirrors a pre-existing limitation in codemod_github_repos.go, but since this is new code with no test coverage for flow-style/list-based tools.github blocks, the gap goes unverified here. Consider either detecting when the parsed check and line-scan check disagree (and returning an error) or adding tests documenting the known limitation.
| _, hasToolset := githubMap["toolset"] | ||
| _, hasToolsets := githubMap["toolsets"] // only check existence, not value | ||
| if hasToolset && !hasToolsets { | ||
| toolsetSingularCodemodLog.Print("Mistyped singular 'toolset' field found in tools.github") | ||
| } | ||
| return hasToolset && !hasToolsets | ||
| } | ||
|
|
||
| // renameToolsetSingularToToolsets renames 'toolset:' to 'toolsets:' within the | ||
| // tools.github configuration block. | ||
| func renameToolsetSingularToToolsets(lines []string) ([]string, bool) { | ||
| var result []string | ||
| modified := false | ||
|
|
||
| var inTools, inToolsGithub bool | ||
| var toolsIndent, toolsGithubIndent string | ||
|
|
||
| for i, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
|
|
||
| // Skip empty lines without resetting state | ||
| if trimmed == "" { | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| // Exit blocks when indentation signals we've left them | ||
| if !strings.HasPrefix(trimmed, "#") { | ||
| if inToolsGithub && hasExitedBlock(line, toolsGithubIndent) { | ||
| inToolsGithub = false | ||
| } | ||
| if inTools && hasExitedBlock(line, toolsIndent) { | ||
| inTools = false | ||
| inToolsGithub = false | ||
| } | ||
| } | ||
|
|
||
| // Detect 'tools:' block | ||
| if strings.HasPrefix(trimmed, "tools:") { | ||
| inTools = true | ||
| inToolsGithub = false | ||
| toolsIndent = getIndentation(line) | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| // Detect 'github:' block inside 'tools:' | ||
| if inTools && strings.HasPrefix(trimmed, "github:") { | ||
| inToolsGithub = true | ||
| toolsGithubIndent = getIndentation(line) | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| // Rename 'toolset:' to 'toolsets:' when inside tools.github | ||
| if inToolsGithub && strings.HasPrefix(trimmed, "toolset:") { | ||
| lineIndent := getIndentation(line) | ||
| if isDescendant(lineIndent, toolsGithubIndent) { | ||
| newLine, replaced := findAndReplaceInLine(line, "toolset", "toolsets") | ||
| if replaced { | ||
| result = append(result, newLine) | ||
| modified = true | ||
| toolsetSingularCodemodLog.Printf("Renamed 'toolset' to 'toolsets' on line %d", i+1) |
There was a problem hiding this comment.
Same silent-no-op risk as the allowed-repos codemod: hasSingularToolsetField (parsed map check) and renameToolsetSingularToToolsets (line-based scanner) can disagree on flow-style or list-based tools.github blocks, causing Apply to report applied: false with no error even though the field was detected as needing migration.
💡 Detail
This file duplicates ~60 lines of block-scanning logic (inTools/inToolsGithub tracking, tools:/github: prefix detection, hasExitedBlock/isDescendant usage) nearly verbatim from codemod_allowed_repos_current.go. Beyond the maintainability cost of two copies that can drift, both share the same blind spot: they assume block-style YAML with tools:/github: each starting their own line at the expected indent. No test in this PR covers flow-style mappings, so a regression here would go unnoticed.
Suggest extracting the shared inTools/inToolsGithub scanning loop into a common helper (e.g. taking a per-line callback for the matched field) to avoid duplicating this logic a third time as more codemods are added.
|
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. Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (627 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. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd — requesting changes on two minor but concrete issues.
📋 Key Themes & Highlights
Issues
- Incorrect
IntroducedInversion (both codemods): both use"1.0.0"despite being added afterv1.0.47andv1.5.0. Should be the actual upcoming release version. - Missing edge-case tests: no test covers
toolset:orallowed-repos: currentappearing outside thetools.githubblock, and no test for single-quoted'current'.
Positive Highlights
- ✅ Robust YAML line-transform pattern: two-phase design (frontmatter check → line rewrite) is clean and consistent with existing codemods
- ✅
findTrailingCommentIndexcorrectly implements YAML comment semantics (whitespace-preceded#only) - ✅ Good idempotency: both codemods check for already-migrated state before applying
- ✅ Comprehensive test coverage of the happy path and comment preservation
- ✅
findAndReplaceInLinereuse prevents accidental substring collisions (e.g.,toolsets:won't be touched)
| ID: "toolset-singular-to-toolsets", | ||
| Name: "Rename 'tools.github.toolset' to 'tools.github.toolsets'", | ||
| Description: "Renames the mistyped singular 'toolset:' field to the correct plural 'toolsets:' inside the tools.github configuration block.", | ||
| IntroducedIn: "1.0.0", |
There was a problem hiding this comment.
[/tdd] IntroducedIn is set to "1.0.0" but this codemod is being added now, well after 1.0.47 and 1.5.0 already exist in the registry. This misleads maintainers about when the migration was introduced and may affect tooling that filters codemods by version.
💡 Fix
Set IntroducedIn to the actual release version this codemod ships in. Check the most recently added codemods (codemod_effective_tokens_to_ai_credits.go at "1.0.47", codemod_workflow_dispatch_required.go at "1.5.0") for the current version baseline, then use the next planned release tag.
@copilot please address this.
| assert.Contains(t, result, "toolsets: default", "Should rename toolset key") | ||
| assert.Contains(t, result, "# toolset: legacy comment", "Should not rename toolset in comments") | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test: toolset: key appearing outside the tools.github block (e.g. under a different parent). The indentation state machine should ignore it, but without a test this silent correctness invariant is unverified.
💡 Suggested test
t.Run("no-op when toolset: appears outside tools.github", func(t *testing.T) {
content := `---
engine: copilot
other:
toolset: some-value
---
# Test Workflow
`
frontmatter := map[string]any{
"engine": "copilot",
"other": map[string]any{"toolset": "some-value"},
}
result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err)
assert.False(t, applied, "Should not rename toolset outside tools.github")
assert.Equal(t, content, result)
})@copilot please address this.
| ID: "allowed-repos-current-to-github-repository", | ||
| Name: "Migrate 'tools.github.allowed-repos: current' to '${{ github.repository }}'", | ||
| Description: "Rewrites the legacy 'current' alias for tools.github.allowed-repos to the accepted '${{ github.repository }}' expression.", | ||
| IntroducedIn: "1.0.0", |
There was a problem hiding this comment.
[/tdd] Same IntroducedIn: "1.0.0" issue as the toolset codemod — this codemod is new, not from v1.0.0. Should use the upcoming release version.
@copilot please address this.
| assert.True(t, applied, "Should have applied the codemod") | ||
| assert.Contains(t, result, `allowed-repos: "${{ github.repository }}" # see docs on "#current" alias`, "Should preserve the full comment including embedded hash") | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test: allowed-repos: current appearing inside a tools.github block that is itself nested under a non-top-level tools: key (i.e., verify the indentation state machine correctly scopes to the top-level tools: block only). Also missing: single-quoted 'current' value — the strings.Trim(value, "') handles it, but it's untested.
@copilot please address this.
github-actions
Bot
added
pr-action:fast_track
pr-agent:copilot
pr-priority:medium
labels
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Maintainer triage for this PR:
Run: https://github.com/github/gh-aw/actions/runs/31113572782
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The daily AW cross-repo compatibility audit found two low-risk, unambiguous migrations with no
gh aw fixcodemod: thetoolset:(singular) typo undertools.github— for which the compiler already suggests "Did you mean 'toolsets'?" — and the legacyallowed-repos: currentalias, which strict-mode rejects since onlyall,public, or${{ github.repository }}are accepted.Changes
toolset-singular-to-toolsetscodemod (pkg/cli/codemod_toolset_singular.go): renamestools.github.toolset:totools.github.toolsets:, scoped to thetools.githubblock, skipped iftoolsetsalready present.allowed-repos-current-to-github-repositorycodemod (pkg/cli/codemod_allowed_repos_current.go): rewritestools.github.allowed-repos: current(quoted or unquoted) toallowed-repos: "${{ github.repository }}", preserving indentation and trailing comments.GetAllCodemods();fix_codemods_test.goexpected ID/order lists updated.#in comments).Example migration:
Run: https://github.com/github/gh-aw/actions/runs/31113572782> Generated by 👨🍳 PR Sous Chef · gpt54 · 17.2 AIC · ⊞ 8.3K · ◷