-
Notifications
You must be signed in to change notification settings - Fork 483
Add fix codemods for toolset typo and allowed-repos: current legacy alias
#50841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ac60613
Initial plan
Copilot 8ecdf69
Add codemods for toolset typo and allowed-repos: current migration
Copilot bf14c43
Harden comment parsing in allowed-repos codemod after review
Copilot 0a00f94
docs(adr): add draft ADR-50841 for toolset typo and allowed-repos cod…
github-actions[bot] cc924d2
Start PR finisher pass
Copilot 529b043
Merge branch 'main' into copilot/aw-compat-daily-audit
github-actions[bot] 23c4c26
Address codemod review feedback
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
48 changes: 48 additions & 0 deletions
48
docs/adr/50841-add-codemods-for-toolset-typo-and-allowed-repos-current.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # ADR-50841: Add Fix Codemods for `toolset` Typo and `allowed-repos: current` Legacy Alias | ||
|
|
||
| **Date**: 2026-08-06 | ||
| **Status**: Draft | ||
| **Deciders**: pelikhan, copilot-swe-agent | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The daily cross-repo compatibility audit revealed two distinct, unambiguous migration gaps in `gh aw fix --write`: the `tools.github.toolset:` (singular) typo — for which the compiler already emits "Did you mean 'toolsets'?" — and the `tools.github.allowed-repos: current` legacy alias, which strict-mode rejects because only `all`, `public`, or `${{ github.repository }}` are accepted values. Both issues caused compilation failures in external repos (`githubnext/gh-aw-trial-oxpecker-test`, `pelikhan/github-agentic-workflows`) that `fix --write` could not automatically resolve. The existing codemod framework (`GetAllCodemods()`, `applyFrontmatterLineTransform`) provides a well-established pattern for exactly these line-level YAML rewrites, and both transformations are deterministic with no ambiguity about the desired output. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will implement two new codemods — `toolset-singular-to-toolsets` and `allowed-repos-current-to-github-repository` — and register them in `GetAllCodemods()` immediately after the existing `github-repos-to-allowed-repos` codemod. Both codemods use the existing `applyFrontmatterLineTransform` hook with indentation-aware, context-scoped line parsers that skip comments, preserve trailing comments, and are guarded by a frontmatter pre-check to remain no-ops on already-migrated files. The `allowed-repos` codemod adds `findTrailingCommentIndex` to correctly split YAML-comment boundaries from values (a `#` only starts a comment when preceded by whitespace). | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Accept Legacy Forms in the Compiler | ||
|
|
||
| Relax strict-mode validation to silently accept `toolset:` (singular) and `allowed-repos: current` as valid input, interpreting them as their canonical equivalents at parse time. This removes the need for migration codemods entirely. | ||
|
|
||
| Not chosen because it undermines the strict-mode design goal: strict-mode exists specifically to reject ambiguous or deprecated configurations. Silently accepting `current` would mean workflows that rely on the deprecated alias never get upgraded to the more explicit `${{ github.repository }}` form, and the compiler's "Did you mean 'toolsets'?" guidance loses its purpose. | ||
|
|
||
| #### Alternative 2: Require Manual Fixes, Improve Error Messages Only | ||
|
|
||
| Keep `fix --write` unchanged; instead, improve the compiler error messages to be copy-paste-ready (e.g., display the exact replacement YAML inline). Users manually apply the one-line change. | ||
|
|
||
| Not chosen because the codemod framework already exists for exactly this purpose, and "Did you mean 'toolsets'?" is already in place. Adding automation is consistent with the project's existing pattern and directly reduces friction for the 20+ repos affected across future audit runs. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - `gh aw fix --write` now closes two additional identified failure clusters from the daily cross-repo audit, reducing manual intervention needed by external repo maintainers. | ||
| - Both codemods are idempotent and scoped strictly to `tools.github` block lines, preventing false-positive rewrites in other YAML blocks. | ||
| - `findTrailingCommentIndex` correctly handles `#` characters embedded in quoted YAML values (e.g., `"#current"` alias), improving robustness of the comment-preservation logic. | ||
|
|
||
| #### Negative | ||
| - Two more entries in `GetAllCodemods()` increase the ordered list that tests must enumerate; any future reordering requires updating `fix_codemods_test.go` in two places. | ||
| - The line-based YAML parsing approach (rather than a full AST parser) is a simplification that can fail on unusual but valid YAML: multi-document files, block scalars, or flow-style mappings that span multiple lines. These edge cases are untested. | ||
|
|
||
| #### Neutral | ||
| - Both codemods are registered with `IntroducedIn: "0.85.5"`, matching the patch release expected to ship the new migrations. | ||
| - The `allowed-repos` codemod normalizes output to double-quoted `"${{ github.repository }}"` regardless of the input quoting style (`current`, `"current"`, `'current'`), which is a minor stylistic imposition but aligns with the canonical form used throughout the codebase. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/github/gh-aw/pkg/logger" | ||
| ) | ||
|
|
||
| var allowedReposCurrentCodemodLog = logger.New("cli:codemod_allowed_repos_current") | ||
|
|
||
| // getAllowedReposCurrentToGitHubRepositoryCodemod creates a codemod that migrates the | ||
| // unsupported legacy 'current' alias for tools.github.allowed-repos to the equivalent | ||
| // '${{ github.repository }}' expression, which is the only accepted way to scope | ||
| // access to the current repository. | ||
| func getAllowedReposCurrentToGitHubRepositoryCodemod() Codemod { | ||
| return Codemod{ | ||
| 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: "0.85.5", | ||
| Apply: func(content string, frontmatter map[string]any) (string, bool, error) { | ||
| if !hasAllowedReposCurrentValue(frontmatter) { | ||
| return content, false, nil | ||
| } | ||
| newContent, applied, err := applyFrontmatterLineTransform(content, rewriteAllowedReposCurrentValue) | ||
| if applied { | ||
| allowedReposCurrentCodemodLog.Print("Migrated 'tools.github.allowed-repos: current' to '${{ github.repository }}'") | ||
| } | ||
| return newContent, applied, err | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // hasAllowedReposCurrentValue returns true if tools.github.allowed-repos is set to the | ||
| // legacy string value 'current'. | ||
| func hasAllowedReposCurrentValue(frontmatter map[string]any) bool { | ||
| toolsAny, hasTools := frontmatter["tools"] | ||
| if !hasTools { | ||
| return false | ||
| } | ||
| toolsMap, ok := toolsAny.(map[string]any) | ||
| if !ok { | ||
| return false | ||
| } | ||
| githubAny, hasGitHub := toolsMap["github"] | ||
| if !hasGitHub { | ||
| return false | ||
| } | ||
| githubMap, ok := githubAny.(map[string]any) | ||
| if !ok { | ||
| return false | ||
| } | ||
| allowedReposAny, hasAllowedRepos := githubMap["allowed-repos"] | ||
| if !hasAllowedRepos { | ||
| return false | ||
| } | ||
| allowedReposStr, ok := allowedReposAny.(string) | ||
| if !ok { | ||
| return false | ||
| } | ||
| return allowedReposStr == "current" | ||
| } | ||
|
|
||
| // rewriteAllowedReposCurrentValue rewrites the value of 'allowed-repos: current' (or any | ||
| // quoted variant) to '${{ github.repository }}' within the tools.github configuration block. | ||
| func rewriteAllowedReposCurrentValue(lines []string) ([]string, bool) { | ||
| var result []string | ||
| modified := false | ||
|
|
||
| var inTools, inToolsGithub bool | ||
| var toolsIndent, toolsChildIndent, 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 top-level 'tools:' block | ||
| if strings.HasPrefix(trimmed, "tools:") && getIndentation(line) == "" { | ||
| inTools = true | ||
| inToolsGithub = false | ||
| toolsIndent = getIndentation(line) | ||
| toolsChildIndent = "" | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| lineIndent := getIndentation(line) | ||
| if inTools && toolsChildIndent == "" && isDescendant(lineIndent, toolsIndent) { | ||
| toolsChildIndent = lineIndent | ||
| } | ||
|
|
||
| // Detect direct 'github:' block inside 'tools:' | ||
| if inTools && strings.HasPrefix(trimmed, "github:") && lineIndent == toolsChildIndent { | ||
| inToolsGithub = true | ||
| toolsGithubIndent = lineIndent | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| // Rewrite the value of 'allowed-repos: current' when inside tools.github | ||
| if inToolsGithub && strings.HasPrefix(trimmed, "allowed-repos:") { | ||
| 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) { | ||
| parts := strings.SplitN(line, ":", 2) | ||
| if len(parts) < 2 { | ||
| return line, false | ||
| } | ||
|
|
||
| leadingSpace := getIndentation(line) | ||
| valuePart := strings.TrimSpace(parts[1]) | ||
|
|
||
| // Split off any trailing comment. Per YAML rules, a '#' only starts a comment | ||
| // when preceded by whitespace (or at the start of the value), so a '#' embedded | ||
| // inside a quoted value (e.g. "cur#rent") is not mistaken for a comment. | ||
| value := valuePart | ||
| comment := "" | ||
| if idx := findTrailingCommentIndex(valuePart); idx >= 0 { | ||
| value = strings.TrimSpace(valuePart[:idx]) | ||
| comment = " " + valuePart[idx:] | ||
| } | ||
|
|
||
| unquoted := strings.Trim(value, `"'`) | ||
| if unquoted != "current" { | ||
| return line, false | ||
| } | ||
|
|
||
| allowedReposCurrentCodemodLog.Print("Replacing 'current' value with '${{ github.repository }}'") | ||
| return fmt.Sprintf(`%sallowed-repos: "${{ github.repository }}"%s`, leadingSpace, comment), true | ||
| } | ||
|
|
||
| // findTrailingCommentIndex returns the index of the '#' that starts a trailing YAML | ||
| // comment in value, or -1 if there is none. A '#' only starts a comment when it is | ||
| // at the beginning of the (trimmed) value or immediately preceded by whitespace, | ||
| // which prevents a '#' embedded inside a quoted string from being misinterpreted. | ||
| func findTrailingCommentIndex(value string) int { | ||
| for i, r := range value { | ||
| if r != '#' { | ||
| continue | ||
| } | ||
| if i == 0 || value[i-1] == ' ' || value[i-1] == '\t' { | ||
| return i | ||
| } | ||
| } | ||
| return -1 | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
hasAllowedReposCurrentValueunderstands any YAML shape the parser can produce (flow-styletools: {github: {allowed-repos: current}}, block sequences, etc.), butrewriteAllowedReposCurrentValueonly recognizes classic block-style mappings wheretools:andgithub:each start their own line at the expected indentation. If a workflow uses flow-style mapping or a list-based tools block,hasAllowedReposCurrentValuereturnstrue,Applyproceeds to the line transform, but the transform never matches any line and returnsmodified == false. TheCodemod.Applythen returnsapplied: 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-basedtools.githubblocks, 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.