Cover top-level app: migration and complete breaking-change release notes#42794
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
app: migration and complete breaking-change release notes
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. Grumpy-coder adjudication complete. All 7 findings were either pre-existing/out-of-diff (findings 2, 3, 4, 6, 7), logically impossible (finding 1), or already covered by the inline review comment submitted earlier (finding 5). No additional safe-output actions needed. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This pull request restores and expands breaking-change migration coverage and documentation for gh-aw, focusing on (1) top-level app: → github-app: frontmatter migration and (2) the removal of imports.if support.
Changes:
- Extend the
app-to-github-appcodemod to also rename a top-levelapp:key in workflow frontmatter (in addition to existing nested auth locations). - Add regression tests covering both the codemod behavior and
gh aw fixprocessing to ensure markdown is preserved while rewriting frontmatter. - Complete breaking-change release documentation by fixing the
major-remove-imports-ifchangeset frontmatter and adding Unreleased changelog guidance forimports.ifremoval and the broaderapp:rename scope.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/fix_command_test.go | Adds fix-command regression coverage for workflows with a top-level app: block. |
| pkg/cli/codemod_github_app.go | Updates the codemod to detect and rename top-level app: → github-app: while keeping existing nested handling. |
| pkg/cli/codemod_github_app_test.go | Adds a focused unit test ensuring top-level app: is rewritten by the codemod. |
| CHANGELOG.md | Adds missing Unreleased breaking-change entry and migration guidance for imports.if removal; clarifies app: breaking change scope. |
| .changeset/minor-rename-app-to-github-app.md | Clarifies breaking-change wording and migration guidance to explicitly include top-level and nested auth blocks. |
| .changeset/major-remove-imports-if.md | Repairs changeset frontmatter and expands migration guidance with a concrete {{#if ...}} + {{#runtime-import ...}} example. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Low
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (114 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 56.6 AIC · ⌖ 9.09 AIC · ⊞ 4.9K
| } | ||
| } | ||
|
|
||
| func TestFixCommand_AppToGitHubAppMigration(t *testing.T) { |
There was a problem hiding this comment.
The new test uses bare t.Error/t.Fatal while the surrounding tests (e.g. TestFixCommand_SandboxFalseToAgentFalseMigration) use assert/require from testify. Consider converting for consistency:
require.NoError(t, err)
assert.True(t, fixed, "Expected file to be modified")
assert.NotContains(t, updatedStr, "\napp:\n")
assert.Contains(t, updatedStr, "\ngithub-app:\n")
assert.Contains(t, updatedStr, "# Test Workflow")
assert.Contains(t, updatedStr, "This workflow uses top-level app auth.")Non-blocking style nit. @copilot please address this.
|
|
||
| // Rename 'app:' to 'github-app:' when inside a target block | ||
| // Rename a top-level 'app:' key. | ||
| if strings.HasPrefix(trimmed, "app:") && isTopLevelKey(line) { |
There was a problem hiding this comment.
The top-level app: rename path (line 156) calls continue after replacement, so the block-state flags (inTools, inSafeOutputs, inCheckout) are never set for a top-level app: key — which is correct since app: 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 an app: key before a tools: key, the tool works fine — but the ordering also means that a top-level app: 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.
There was a problem hiding this comment.
The implementation is correct. The hasDeprecatedAppField / renameAppToGitHubApp split is clean, isTopLevelKey correctly guards the new arm, and the block-exit logic ensures the two renaming paths are mutually exclusive. The changeset frontmatter fix (--- delimiters) is essential and right.
One non-blocking gap
The only actionable item is a missing combined-scenario unit test (top-level app: + nested app: in the same document). See inline comment for a suggested addition. The code logic should already handle it correctly, but the combined path is untested.
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.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 160.1 AIC · ⌖ 7.18 AIC · ⊞ 1.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/codemod_github_app_test.go:239
Missing combined-scenario coverage: the "renames app in all three sections" test only combines tools.github.app + safe-outputs.app — it doesn't include a top-level app: alongside nested ones. The two renaming arms (isTopLevelKey branch and block-tracking branch) are independent code paths; a regression in either would go undetected in a combined-document scenario, which is exactly the real-world case this PR targets.
<details>
<summary>💡 Suggested addition</summary>
Add one mor…
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/28535406702
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Stabilization pass complete (commit Combined-scenario coverage added — ADR linked — the PR description now includes a Design section with a direct link to ADR-42794. Windows Integration failure — Local |
This follows up on the daily breaking-change audit by closing gaps in migration coverage and release documentation. In particular, it restores
gh aw fixcoverage for theapp:→github-app:rename at the top level and completes the missingimports.ifbreaking-change documentation.Release-note and changeset fixes
major-remove-imports-ifchangeset so it is parseable by the changeset toolingimports.ifmigration guidance with a concrete replacement pattern using{{#if ...}}+{{#runtime-import ...}}imports.ifremovalapp:→github-app:changeset and changelog text to cover both top-level and nested auth blocksgh aw fixmigration coverageapp-to-github-appcodemod to rename a top-levelapp:block in addition to the existing nested cases undertools.github,safe-outputs, andcheckoutRegression coverage
app:migrationapp:are rewritten togithub-app:while preserving surrounding markdown