Add default gh aw fix codemod to lowercase discussion trigger categories in frontmatter#31872
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw fix codemod to lowercase discussion trigger categories in frontmatter
There was a problem hiding this comment.
Pull request overview
Adds a new default-on gh aw fix codemod that rewrites discussion trigger types values in workflow frontmatter to lowercase so the source matches the compiler’s runtime-normalized behavior.
Changes:
- Registered a new codemod (
discussion-trigger-categories-lowercase) in the defaultGetAllCodemods()pipeline. - Implemented a frontmatter line transformer to lowercase values under
on.discussion.typesandon.discussion_comment.types(block and inline list forms). - Added unit tests covering mixed-case rewrites, no-op behavior, and registry/order expectations.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/fix_codemods.go | Registers the new codemod in the default codemod sequence. |
| pkg/cli/fix_codemods_test.go | Updates codemod ID/order expectations to include the new codemod. |
| pkg/cli/codemod_discussion_trigger_categories.go | Implements the new codemod and the YAML-frontmatter line transformation logic. |
| pkg/cli/codemod_discussion_trigger_categories_test.go | Adds focused tests for codemod metadata and rewrite/no-op behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/codemod_discussion_trigger_categories.go:143
- The trigger/type detection is overly strict: it requires
trimmed == "discussion:"/"discussion_comment:"and treatstypes:as a block list only when the remainder aftertypes:is empty. This means common YAML likediscussion: # commentortypes: # commentfollowed by- ...items will not be rewritten. Consider matching these keys withHasPrefix("discussion:")/HasPrefix("types:")and treating a remainder that begins with#as a block-list start.
if len(indent) > len(onIndent) && (trimmed == "discussion:" || trimmed == "discussion_comment:") {
currentTrigger = strings.TrimSuffix(trimmed, ":")
triggerIndent = indent
inTypes = false
continue
}
if currentTrigger == "" {
continue
}
if inTypes && len(indent) <= len(typesIndent) {
inTypes = false
}
if strings.HasPrefix(trimmed, "types:") {
typesIndent = indent
afterColon := strings.TrimSpace(strings.TrimPrefix(trimmed, "types:"))
if strings.HasPrefix(afterColon, "[") && strings.HasSuffix(afterColon, "]") {
updatedLine, changed := lowercaseInlineTypesArrayLine(line)
if changed {
result[i] = updatedLine
modified = true
}
inTypes = false
} else if afterColon == "" {
inTypes = true
}
continue
- Files reviewed: 4/4 changed files
- Comments generated: 1
| if isTopLevelKey(line) && strings.HasPrefix(trimmed, "on:") { | ||
| inOn = true | ||
| onIndent = indent | ||
| currentTrigger = "" | ||
| inTypes = false | ||
| continue |
Generated by Design Decision Gate workflow. Documents the decision to add a default-on codemod that lowercases discussion trigger category values so source frontmatter matches compile-time normalized form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (354 new lines under AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: 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. 📋 What's in the draft ADRThe draft captures the decision to add
Please verify each section reflects your actual reasoning — especially the alternatives you weighed and any consequences I may have missed. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References:
|
![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 — this is a new feature addition with accompanying tests, so test quality and behavioural coverage are the right lens.
Key Themes
- Expression-guard inconsistency (correctness bug):
lowercaseYAMLListItemLinecorrectly skips values containing${{, butlowercaseInlineTypesArrayLinedoes not. Any inline-array entry that is a GitHub Actions expression would be silently lowercased. This is the most important issue to fix before merge. - Comment-detection fragility: Using
strings.Index(line, "#")to find inline comments will misfire on a#inside a quoted value. Using#(space + hash) is a safer heuristic that matches standard YAML convention. - Test coverage gaps: The two existing tests cover the happy path and the no-op path, but the inline-array specific code paths (quoted values, expression guards) have no test coverage. The expression-guard bug above would have been caught with such a test.
Positive Highlights
- ✅ Clean two-phase design: frontmatter parse to decide whether to apply, then line transform to do the work — consistent with every other codemod in the package.
- ✅ Correct build-tag (
//go:build !integration) and use ofrequirevsassertin tests. - ✅ No-op guard in
hasMixedCaseDiscussionTypeValuesavoids unnecessary line-by-line scanning for files that need no change. - ✅ Registration in
GetAllCodemods()and the order test updated together — no drift.
Verdict
Requesting changes on the ${{ expression guard and a couple of targeted tests before merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 6.2M
| for i, part := range parts { | ||
| trimmed := strings.TrimSpace(part) | ||
| unquoted := strings.Trim(trimmed, `"'`) | ||
| lower := strings.ToLower(unquoted) |
There was a problem hiding this comment.
[/tdd] lowercaseInlineTypesArrayLine does not guard against ${{ expressions, but its sibling lowercaseYAMLListItemLine does (line 240). A category value like types: [${{ env.CATEGORY }}] would be silently mangled.
Apply the same guard used in the list-item path:
for i, part := range parts {
trimmed := strings.TrimSpace(part)
if strings.Contains(trimmed, "${{") {
continue // leave expressions untouched
}
// ...
}A matching test case (types: [${{ env.CATEGORY }}] → no-op) would confirm the fix is regression-proof.
| } | ||
|
|
||
| func lowercaseInlineTypesArrayLine(line string) (string, bool) { | ||
| commentIndex := strings.Index(line, "#") |
There was a problem hiding this comment.
[/tdd] Comment detection via strings.Index(line, "#") will false-positive on a # inside a quoted value — e.g. types: ["#general"] would treat #general"] as a comment, producing a garbled replacement.
The same issue affects the list-item path implicitly, but is less likely there. Consider searching for # (space+hash) instead, which is the conventional inline-comment separator in YAML:
commentIndex := strings.Index(line, " #")Or alternatively, only search for # outside any quoted segment.
| result, applied, err := codemod.Apply(content, frontmatter) | ||
|
|
||
| require.NoError(t, err) | ||
| assert.True(t, applied) |
There was a problem hiding this comment.
[/tdd] The tests cover the two main paths (mixed-case rewrite, already-lowercase no-op) but miss the inline-array edge cases that the new code explicitly handles:
- Quoted values —
types: ["General", 'Special']should becometypes: ["general", 'special'] - Expression guard —
types: [${{ env.CATEGORY }}]should be left unchanged
These scenarios exercise distinct branches in lowercaseInlineTypesArrayLine and would have caught the missing ${{ guard mentioned in the companion comment.
🧪 Test Quality Sentinel ReportTest Quality Score: 77/100
Test Classification DetailsView per-test classification table
Flagged Tests — Requires Review
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: 77/100. 33.3% of new tests are classified as low-value implementation tests, exceeding the 30% threshold. The TestGetDiscussionTriggerCategoriesLowercaseCodemod test checks only struct metadata without exercising any behavioral logic. Additionally, all assertions in the new test file are missing the required descriptive message arguments. Please review the flagged tests in the comment above.
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
…add-lowercase-discussion-category-codemod Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
✨ Enhancement
gh-awalready normalizes discussion category/type strings to lowercase at compile time, but workflow source could remain mixed-case and drift from runtime behavior. This adds a default-on codemod sogh aw fix --writerewrites source to the same lowercase form the compiler uses.What does this improve?
Agentic Workflowsin source vsagentic workflowsat runtime.Why is this valuable?
gh aw fixcodemod pipeline (no opt-in flag).Implementation approach:
discussion-trigger-categories-lowercase.on.discussion.typeson.discussion_comment.typesGetAllCodemods()sequence.