-
Notifications
You must be signed in to change notification settings - Fork 444
Cover top-level app: migration and complete breaking-change release notes
#42794
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
pelikhan
merged 5 commits into
main
from
copilot/breaking-change-daily-analysis-2026-07-01
Jul 1, 2026
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
2cda973
Initial plan
Copilot a4d79ac
Cover top-level app migration and document imports.if removal
Copilot 22c74b1
Use shared top-level key check in app codemod
Copilot 0bd963f
docs(adr): add draft ADR-42794 for top-level app codemod extension
github-actions[bot] c4decf6
test: add combined top-level + nested app codemod scenario coverage
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.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
43 changes: 43 additions & 0 deletions
43
docs/adr/42794-extend-app-to-github-app-codemod-to-top-level-key.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,43 @@ | ||
| # ADR-42794: Extend `app-to-github-app` Codemod to Cover Top-Level `app:` Key | ||
|
|
||
| **Date**: 2026-07-01 | ||
| **Status**: Draft | ||
| **Deciders**: pelikhan, copilot-swe-agent | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The `gh aw fix` command provides automated codemods to help users migrate their workflow frontmatter across breaking changes. A previous breaking change renamed the `app:` frontmatter field to `github-app:`, and a codemod was implemented to automate that rename. However, the codemod only handled `app:` when it appeared nested under `tools.github`, `safe-outputs`, or `checkout` blocks — it did not handle the top-level `app:` key. Users whose workflows used the top-level `app:` auth configuration would run `gh aw fix` and see no changes applied, then encounter validation failures when the field was rejected by the tooling. This gap was surfaced through a daily breaking-change audit. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will extend the `hasDeprecatedAppField` detection function and the `renameAppToGitHubApp` rewrite function in `pkg/cli/codemod_github_app.go` to also detect and rename a top-level `app:` key in workflow YAML frontmatter. The top-level check uses the existing `isTopLevelKey` helper (shared frontmatter key detection logic) and is evaluated before the nested-section checks to avoid double-processing. This preserves the existing behavior for nested locations while closing the coverage gap. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Document-only migration (no codemod for top-level) | ||
|
|
||
| Rather than extending the codemod, we could have documented the top-level `app:` rename in the migration guide and relied on users to make the change manually. This was rejected because the entire purpose of `gh aw fix` is to automate breaking-change migrations, and leaving one common configuration location uncovered would undermine user trust in the tool and create inconsistent migration outcomes. | ||
|
|
||
| #### Alternative 2: Generic "rename any `app:` key" approach | ||
|
|
||
| An alternative was to rename every occurrence of `app:` in any position within the frontmatter, without checking whether it is at the top level or inside a specific section. This was rejected because it would be too broad: it could incorrectly rename `app:` keys that appear inside unrelated nested structures, or inside the workflow body rather than the frontmatter, leading to false positives. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - `gh aw fix` now covers all known locations of the deprecated `app:` field, giving users a fully automated migration path for this breaking change. | ||
| - Regression tests (`codemod_github_app_test.go` and `fix_command_test.go`) were added for top-level rename coverage, increasing confidence that this behavior does not regress in future changes. | ||
|
|
||
| #### Negative | ||
| - The `hasDeprecatedAppField` and `renameAppToGitHubApp` functions are now more complex, with an explicit ordering dependency: top-level checks run before nested-section checks. | ||
| - Future contributors adding new `app:` rename locations must understand this ordering, which is not self-documenting. | ||
|
|
||
| #### Neutral | ||
| - The top-level check reuses the existing `isTopLevelKey` shared helper, avoiding any new YAML parsing logic. | ||
| - Changeset and CHANGELOG documentation was updated to reflect that both top-level and nested `app:` locations are affected by the breaking change. | ||
|
|
||
| --- | ||
|
|
||
| *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
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
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 top-level
app:rename path (line 156) callscontinueafter replacement, so the block-state flags (inTools,inSafeOutputs,inCheckout) are never set for a top-levelapp:key — which is correct sinceapp:at the top level is never a block header for those nested sections. However, the current code first tries the top-level rename before checking for block entries (tools:,safe-outputs:,checkout:). If a YAML document somehow had anapp:key before atools:key, the tool works fine — but the ordering also means that a top-levelapp:line is renamed without first having set any block flags, which is the expected behaviour. The logic is correct but brittle; consider a brief comment clarifying why the top-level check comes before the nested block entry detection, to aid future maintainers.