feat: add two codemods for persistent cross-repo compile failures (maui, azure-rest-api-specs)#38097
Conversation
…codemods - Add `workflow-dispatch-required-false-with-slash-command` codemod that rewrites workflow_dispatch.inputs.*.required: true → required: false when on.slash_command or on.label_command is also present (fixes dotnet/maui pattern) - Add `top-level-env-secrets-guided-error` codemod that detects secrets in the top-level env: block and returns a guided error with manual fix instructions (addresses Azure/azure-rest-api-specs codemod gap) - Register both codemods in fix_codemods.go registry - Update fix_codemods_test.go to include both new IDs - Add comprehensive unit tests for both codemods Co-authored-by: pelikhan <[email protected]>
… top-level env codemod Co-authored-by: pelikhan <[email protected]>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds two new gh aw fix codemods to address recurring cross-repo compile/validation failures: one auto-rewrite for workflow_dispatch.inputs.*.required when command triggers are present, and one guided-error detector for secrets in the top-level env: block.
Changes:
- Register
workflow-dispatch-required-false-with-slash-commandandtop-level-env-secrets-guided-errorin the codemod registry and expected-order tests. - Implement a YAML frontmatter line-transform codemod that rewrites
required: true→required: falseunderon.workflow_dispatch.inputs.*whenon.slash_command/on.label_commandis present. - Add a guided-error codemod that detects
${{ secrets.* }}usage in the top-levelenv:block and returns a documentation-linked error.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/fix_codemods.go | Registers the two new codemods in GetAllCodemods(). |
| pkg/cli/fix_codemods_test.go | Updates expected codemod presence + ordering lists to include the new IDs. |
| pkg/cli/codemod_workflow_dispatch_required.go | Implements the workflow_dispatch required→false rewrite codemod. |
| pkg/cli/codemod_workflow_dispatch_required_test.go | Adds tests for the new required→false codemod (and also includes the top-level env guided-error tests). |
| pkg/cli/codemod_engine_env_secrets.go | Adds the top-level env: secrets guided-error codemod implementation. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
| // Enter inputs: within workflow_dispatch: (block form only, no inline value). | ||
| if inWD && !inInputs && strings.HasPrefix(trimmed, "inputs:") && | ||
| strings.TrimSpace(strings.TrimPrefix(trimmed, "inputs:")) == "" { | ||
| inInputs = true | ||
| inputsIndent = indent | ||
| result = append(result, line) | ||
| continue | ||
| } | ||
|
|
||
| // Enter an individual input entry (direct child key of inputs:). | ||
| if inInputs && !inInputEntry && len(indent) > len(inputsIndent) { | ||
| inInputEntry = true | ||
| inputEntryIndent = indent | ||
| result = append(result, line) | ||
| continue | ||
| } |
| var secretRefs []string | ||
| for _, expr := range secretExpressions { | ||
| secretRefs = append(secretRefs, expr) | ||
| } |
| func TestTopLevelEnvSecretsGuidedErrorCodemod(t *testing.T) { | ||
| codemod := getTopLevelEnvSecretsGuidedErrorCodemod() | ||
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on one correctness bug and a few test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Correctness bug (line transformer):
inputs: # commentsilently bypasses theinInputsstate, causing the rewrite to produce a no-op. Needs a one-line condition fix and a regression test. - Non-deterministic error message:
secretRefsis built from map iteration;sort.Stringswould make the guided error stable and future-proof against snapshot tests. - Missing boundary test: mixed
required: true/required: falseinputs are not exercised; the all-true and all-false cases are covered but not the partial case. - Undocumented semantic trade-off: the
required: falserewrite also removes input validation for manual UI runs. Worth documenting in the codemodDescriptionor surfacing in the apply message.
Positive Highlights
- ✅ Two-tier approach (auto-fix vs. guided error) is well-reasoned and the distinction is clearly explained
- ✅ Test file is 2× the size of the implementation — strong coverage intent
- ✅ No-op guards are thoroughly tested for every early-exit branch
- ✅ The frontmatter pre-check prevents the line transformer from running unnecessarily
- ✅ Codemod ordering placement relative to
engine-env-secrets-to-engine-configis logical and the snapshot test is kept in sync
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 357.9 AIC · ⌖ 45.7 AIC
|
|
||
| // Enter inputs: within workflow_dispatch: (block form only, no inline value). | ||
| if inWD && !inInputs && strings.HasPrefix(trimmed, "inputs:") && | ||
| strings.TrimSpace(strings.TrimPrefix(trimmed, "inputs:")) == "" { |
There was a problem hiding this comment.
[/tdd] inputs: with a trailing comment (inputs: # some comment) silently bypasses this check, causing the codemod to produce a no-op when the transform should fire.
💡 Details and suggested fix
The condition strings.TrimSpace(strings.TrimPrefix(trimmed, "inputs:")) == "" rejects a line like inputs: # some comment because the trimmed remainder is "# some comment", not "".
Change the condition to also accept a trailing comment:
remainder := strings.TrimSpace(strings.TrimPrefix(trimmed, "inputs:"))
if inWD && !inInputs && strings.HasPrefix(trimmed, "inputs:") &&
(remainder == "" || strings.HasPrefix(remainder, "#")) {A regression test to cover this case:
t.Run("rewrites when inputs: has trailing comment", func(t *testing.T) {
content := `---
on:
slash_command:
name: run
workflow_dispatch:
inputs: # PR inputs
pr_number:
required: true
---
`
// ... assert applied == true
})|
|
||
| var secretRefs []string | ||
| for _, expr := range secretExpressions { | ||
| secretRefs = append(secretRefs, expr) |
There was a problem hiding this comment.
[/tdd] The secretRefs slice is built by iterating over secretExpressions, which itself comes from map iteration — making the order of secrets in the error message non-deterministic across runs.
💡 Suggested fix
Add sort.Strings(secretRefs) after building the slice so the error message is stable:
import "sort"
// ...
var secretRefs []string
for _, expr := range secretExpressions {
secretRefs = append(secretRefs, expr)
}
sort.Strings(secretRefs) // ensure deterministic outputNon-deterministic error messages make log analysis harder and can cause fragile snapshot tests in future. The existing tests already use assert.Contains (order-agnostic), but any future snapshot-style test would be flaky without this fix.
| result, applied, err := codemod.Apply(content, frontmatter) | ||
| require.NoError(t, err) | ||
| assert.True(t, applied) | ||
| assert.NotContains(t, result, "required: true") |
There was a problem hiding this comment.
[/tdd] Missing boundary condition: a mix of required: true and required: false inputs. The test suite covers all-true and all-false, but not the case where only some inputs need rewriting.
💡 Suggested test to add after this one
t.Run("rewrites only required: true inputs, leaves required: false unchanged", func(t *testing.T) {
content := `---
on:
slash_command:
name: run
workflow_dispatch:
inputs:
pr_number:
description: "PR number"
required: true
type: number
dry_run:
description: "Dry run flag"
required: false
type: boolean
---
# Run
`
frontmatter := map[string]any{
"on": map[string]any{
"slash_command": map[string]any{"name": "run"},
"workflow_dispatch": map[string]any{
"inputs": map[string]any{
"pr_number": map[string]any{"required": true},
"dry_run": map[string]any{"required": false},
},
},
},
}
result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err)
assert.True(t, applied)
assert.NotContains(t, result, "required: true")
// dry_run must still be explicitly false (not removed)
assert.Contains(t, result, "required: false")
})This guards against the line transformer accidentally touching required: false lines or missing one of multiple inputs.
|
|
||
| newContent, applied, err := applyFrontmatterLineTransform(content, rewriteWorkflowDispatchRequiredFalse) | ||
| if applied { | ||
| workflowDispatchRequiredLog.Print("Applied workflow-dispatch-required-false-with-slash-command codemod") |
There was a problem hiding this comment.
[/grill-with-docs] The apply log says the codemod fired, but there is no user-visible message explaining the semantic trade-off: setting required: false also disables the required-input guard for users who trigger the workflow manually via the GitHub UI.
💡 Suggestion
Codemods that change user semantics (not just syntax) benefit from a brief advisory in the structured error or log output. Consider returning a non-fatal warning (or at minimum surfacing a note in the compile output) similar to:
Note: workflow_dispatch inputs set to required: false also lose input validation for manual UI runs.
If strict input enforcement is needed for UI runs, consider runtime guards in the workflow body.
Alternatively, document this in the codemod Description field so it surfaces in gh aw audit output. The current description only mentions that auto-dispatched triggers are the reason, but the implication for manual runs is left unstated.
| assert.Contains(t, result, "type: number") | ||
| assert.Contains(t, result, `default: "0"`) | ||
| assert.Contains(t, result, "required: false") | ||
| assert.NotContains(t, result, "required: true") |
There was a problem hiding this comment.
[/tdd] No test covers the inputs: trailing-comment edge case (inputs: # comment), which would silently skip the rewrite (see corresponding comment on line 138 of the implementation). A dedicated test here would either document the known limitation or prove the fix is correct.
💡 Suggested test skeleton
t.Run("rewrites when inputs: line has a trailing comment", func(t *testing.T) {
content := `---
on:
slash_command:
name: run
workflow_dispatch:
inputs: # workflow inputs
pr_number:
description: "PR"
required: true
type: number
---
# Run
`
frontmatter := map[string]any{
"on": map[string]any{
"slash_command": map[string]any{"name": "run"},
"workflow_dispatch": map[string]any{
"inputs": map[string]any{
"pr_number": map[string]any{"required": true},
},
},
},
}
result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err)
assert.True(t, applied, "should apply despite trailing comment on inputs: line")
assert.NotContains(t, result, "required: true")
})
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <[email protected]>
Co-authored-by: pelikhan <[email protected]>
Implemented the requested pr-finisher pass and pushed fixes in 616edee and ff33872. Changes address inline/trailing-comment workflow_dispatch input rewrites, deduplicated/sorted top-level env secret reporting, and test coverage/file organization updates, with local make fmt/lint/test-unit/test/recompile passing. |
|
Request final maintainer review and post a brief completion summary.
|
Daily cross-repo compile audit identified two recurring failures not covered by existing codemods:
dotnet/maui(required: trueinputs on auto-dispatched triggers) andAzure/azure-rest-api-specs(secrets in top-levelenv:block evadingengine-env-secrets-to-engine-config).workflow-dispatch-required-false-with-slash-commandWhen
on.slash_commandoron.label_commandcoexists withon.workflow_dispatch, rewritesinputs.*.required: true→required: false. Auto-dispatched triggers can never supply required manual inputs, so this is a safe pure rewrite.top-level-env-secrets-guided-errorExtends secret detection to the top-level
env:block (the existingengine-env-secrets-to-engine-configcodemod only inspectsengine.env). Cannot auto-strip — the token may be legitimately required — so returns a guided error with a documentation link instead:Both codemods are registered in
GetAllCodemods()with unit tests and expected-order test list updated.