Skip to content

[aw-compat] Fix Serena codemod output for nested engine config and stale gh-aw source pins #28080

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.

Sign up for GitHub

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 6 commits into from
Apr 23, 2026
Merged

[aw-compat] Fix Serena codemod output for nested engine config and stale gh-aw source pins #28080

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5459cc8
Initial plan
Copilot Apr 23, 2026
100563a
chore: outline plan for serena codemod edge cases
Copilot Apr 23, 2026
b44c15f
fix: handle serena codemod edge cases for engine blocks and source pins
Copilot Apr 23, 2026
ab5bd4e
refactor: simplify serena codemod block scanning and engine insert logic
Copilot Apr 23, 2026
a0a3471
docs(adr): add draft ADR-28080 for multi-path serena codemod
github-actions[bot] Apr 23, 2026
c6f0ecf
fix: address serena codemod review comments
Copilot Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 132 additions & 47 deletions pkg/cli/codemod_serena_import.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,8 @@ func getSerenaToSharedImportCodemod() Codemod {
Description: "Removes 'tools.serena' and adds an equivalent 'imports' entry using shared/mcp/serena.md with languages.",
IntroducedIn: "1.0.0",
Comment on lines 20 to 23
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codemod metadata strings still describe only migrating top-level tools.serena, but the implementation now also supports engine.tools.serena and may rewrite a pinned source: ref to @main. Please update the Name/Description to accurately reflect the new behavior so users understand what the codemod can change.

See below for a potential fix:

// or engine.tools.serena configuration to an equivalent imports entry using
// shared/mcp/serena.md, and may normalize a pinned source ref to @main.
func getSerenaToSharedImportCodemod() Codemod {
	return Codemod{
		ID:           "serena-tools-to-shared-import",
		Name:         "Migrate tools.serena or engine.tools.serena to shared Serena import",
		Description:  "Removes 'tools.serena' or 'engine.tools.serena', adds an equivalent 'imports' entry using shared/mcp/serena.md with languages, and may rewrite a pinned 'source:' ref to '@main'.",

Copilot uses AI. Check for mistakes.
Apply: func(content string, frontmatter map[string]any) (string, bool, error) {
toolsAny, hasTools := frontmatter["tools"]
if !hasTools {
return content, false, nil
}

toolsMap, ok := toolsAny.(map[string]any)
if !ok {
return content, false, nil
}

serenaAny, hasSerena := toolsMap["serena"]
if !hasSerena {
return content, false, nil
}

languages, ok := extractSerenaLanguages(serenaAny)
languages, ok := findSerenaLanguagesForMigration(frontmatter)
if !ok || len(languages) == 0 {
serenaImportCodemodLog.Print("Found tools.serena but languages configuration is invalid or empty - skipping migration; verify tools.serena languages are set")
return content, false, nil
}

Expand All @@ -50,7 +34,8 @@ func getSerenaToSharedImportCodemod() Codemod {
return lines, false
}

result = removeTopLevelBlockIfEmpty(result, "tools")
result = removeBlockIfEmpty(result, "tools")
result = removeBlockIfEmpty(result, "engine")

if alreadyImported {
return result, true
Expand All @@ -59,6 +44,7 @@ func getSerenaToSharedImportCodemod() Codemod {
return addSerenaImport(result, languages), true
})
if applied {
newContent = maybeUpdatePinnedSourceRef(newContent, frontmatter)
if alreadyImported {
serenaImportCodemodLog.Print("Removed tools.serena (shared/mcp/serena.md import already present)")
} else {
Expand All @@ -70,6 +56,53 @@ func getSerenaToSharedImportCodemod() Codemod {
}
}

func findSerenaLanguagesForMigration(frontmatter map[string]any) ([]string, bool) {
toolsAny, hasTools := frontmatter["tools"]
if hasTools {
if toolsMap, ok := toolsAny.(map[string]any); ok {
if serenaAny, hasSerena := toolsMap["serena"]; hasSerena {
languages, ok := extractSerenaLanguages(serenaAny)
if ok && len(languages) > 0 {
return languages, true
}
return nil, false
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In findSerenaLanguagesForMigration, if tools.serena exists but its languages are invalid/empty, the function returns (nil, false) immediately (line 68) and never checks engine.tools.serena. This prevents migration in mixed configs where top-level tools.serena is malformed but the nested engine.tools.serena is valid. Consider continuing to the engine.tools.serena lookup when extractSerenaLanguages fails/returns empty, and only returning false after both locations are checked.

Suggested change
return nil, false

Copilot uses AI. Check for mistakes.
}
}
}

engineAny, hasEngine := frontmatter["engine"]
if !hasEngine {
return nil, false
}

engineMap, ok := engineAny.(map[string]any)
if !ok {
return nil, false
}

engineToolsAny, hasEngineTools := engineMap["tools"]
if !hasEngineTools {
return nil, false
}

engineToolsMap, ok := engineToolsAny.(map[string]any)
if !ok {
return nil, false
}

serenaAny, hasSerena := engineToolsMap["serena"]
if !hasSerena {
return nil, false
}

languages, ok := extractSerenaLanguages(serenaAny)
if !ok || len(languages) == 0 {
return nil, false
}

return languages, true
}

func extractSerenaLanguages(serenaAny any) ([]string, bool) {
switch serena := serenaAny.(type) {
case []string:
Expand Down Expand Up @@ -204,7 +237,13 @@ func addSerenaImport(lines []string, languages []string) []string {
insertAt := 0
for i, line := range lines {
if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), "engine:") {
insertAt = i + 1
insertAt = len(lines)
for j := i + 1; j < len(lines); j++ {
if isTopLevelKey(lines[j]) {
insertAt = j
break
}
}
break
}
}
Expand All @@ -228,42 +267,88 @@ func formatStringArrayInline(values []string) string {
return "[" + strings.Join(quoted, ", ") + "]"
}

func removeTopLevelBlockIfEmpty(lines []string, blockName string) []string {
blockIdx := -1
blockEnd := len(lines)
for i, line := range lines {
if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), blockName+":") {
blockIdx = i
for j := i + 1; j < len(lines); j++ {
if isTopLevelKey(lines[j]) {
blockEnd = j
break
}
}
break
func removeBlockIfEmpty(lines []string, blockName string) []string {
result := make([]string, 0, len(lines))
for i := 0; i < len(lines); {
line := lines[i]
trimmed := strings.TrimSpace(line)
if !strings.HasPrefix(trimmed, blockName+":") {
result = append(result, line)
i++
continue
}
}

if blockIdx == -1 {
return lines
valuePart := strings.TrimSpace(strings.TrimPrefix(trimmed, blockName+":"))
if valuePart != "" && !strings.HasPrefix(valuePart, "#") {
result = append(result, line)
i++
continue
}

hasMeaningfulNestedContent, blockEnd := hasNestedContent(lines, i+1, getIndentation(line))

if hasMeaningfulNestedContent {
result = append(result, line)
i++
continue
}

i = blockEnd
}

hasMeaningfulNestedContent := false
for _, line := range lines[blockIdx+1 : blockEnd] {
trimmed := strings.TrimSpace(line)
if trimmed == "" || strings.HasPrefix(trimmed, "#") {
return result
}

func hasNestedContent(lines []string, startIndex int, blockIndent string) (bool, int) {
for i := startIndex; i < len(lines); i++ {
nestedLine := lines[i]
nestedTrimmed := strings.TrimSpace(nestedLine)
if nestedTrimmed == "" {
continue
}

nestedIndent := getIndentation(nestedLine)
if strings.HasPrefix(nestedTrimmed, "#") {
if len(nestedIndent) <= len(blockIndent) {
return false, i
}
continue
}
hasMeaningfulNestedContent = true
break

if len(nestedIndent) <= len(blockIndent) && strings.Contains(nestedLine, ":") {
return false, i
}

return true, i
}

if hasMeaningfulNestedContent {
return lines
return false, len(lines)
}

func maybeUpdatePinnedSourceRef(content string, frontmatter map[string]any) string {
sourceAny, hasSource := frontmatter["source"]
if !hasSource {
return content
}

result := make([]string, 0, len(lines)-(blockEnd-blockIdx))
result = append(result, lines[:blockIdx]...)
result = append(result, lines[blockEnd:]...)
return result
source, ok := sourceAny.(string)
if !ok || strings.TrimSpace(source) == "" {
return content
}

sourceSpec, err := parseSourceSpec(source)
if err != nil {
return content
}

if sourceSpec.Repo != "github/gh-aw" || !IsCommitSHA(sourceSpec.Ref) {
return content
}

updatedSource := sourceSpec.Repo + "/" + sourceSpec.Path + "@main"
updatedContent, err := UpdateFieldInFrontmatter(content, "source", updatedSource)
if err != nil {
return content
}
return updatedContent
}
68 changes: 68 additions & 0 deletions pkg/cli/codemod_serena_import_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,72 @@ imports:
assert.False(t, applied, "Codemod should not be applied when tools.serena is absent")
assert.Equal(t, content, result, "Content should remain unchanged when no migration is needed")
})

t.Run("migrates engine.tools.serena and preserves engine sibling fields", func(t *testing.T) {
content := `---
engine:
tools:
serena:
languages: ["typescript"]
model: gpt-5.2
id: copilot
---

# Test Workflow
`
frontmatter := map[string]any{
"engine": map[string]any{
"tools": map[string]any{
"serena": map[string]any{
"languages": []any{"typescript"},
},
},
"model": "gpt-5.2",
"id": "copilot",
},
}

result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err, "Codemod should not return an error")
assert.True(t, applied, "Codemod should be applied when engine.tools.serena is present")
assert.Contains(t, result, "imports:", "Codemod should add imports block")
assert.Contains(t, result, "languages: [\"typescript\"]", "Codemod should preserve engine.tools.serena languages")
assert.NotContains(t, result, "\n tools:", "Codemod should remove now-empty engine.tools block")

parsed, parseErr := parser.ExtractFrontmatterFromContent(result)
require.NoError(t, parseErr, "Result should contain valid frontmatter")
engineAny, hasEngine := parsed.Frontmatter["engine"]
require.True(t, hasEngine, "Result should preserve engine block")
engine, ok := engineAny.(map[string]any)
require.True(t, ok, "Engine should remain an object when sibling fields remain")
assert.Equal(t, "gpt-5.2", engine["model"], "Engine model should remain under engine block")
assert.Equal(t, "copilot", engine["id"], "Engine id should remain under engine block")
_, hasEngineTools := engine["tools"]
assert.False(t, hasEngineTools, "Engine tools should be removed after migration")
})

t.Run("updates github/gh-aw source pin from commit SHA to main when migrating serena", func(t *testing.T) {
content := `---
source: github/gh-aw/.github/workflows/duplicate-code-detector.md@852cb06ad52958b402ed982b69957ffc57ca0619
engine: copilot
tools:
serena: ["typescript"]
---

# Test Workflow
`
frontmatter := map[string]any{
"source": "github/gh-aw/.github/workflows/duplicate-code-detector.md@852cb06ad52958b402ed982b69957ffc57ca0619",
"engine": "copilot",
"tools": map[string]any{
"serena": []any{"typescript"},
},
}

result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err, "Codemod should not return an error")
assert.True(t, applied, "Codemod should be applied when tools.serena is present")
assert.Contains(t, result, "source: github/gh-aw/.github/workflows/duplicate-code-detector.md@main", "Codemod should update pinned gh-aw source to main")
assert.Contains(t, result, "- uses: shared/mcp/serena.md", "Codemod should still add shared Serena import")
})
}
10 changes: 5 additions & 5 deletions pkg/cli/spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,11 +1117,11 @@ func TestSpec_PublicAPI_ValidateWorkflowIntent(t *testing.T) {
// Spec: "Sets a field in frontmatter YAML"
func TestSpec_PublicAPI_UpdateFieldInFrontmatter(t *testing.T) {
tests := []struct {
name string
content string
fieldName string
fieldValue string
wantErr bool
name string
content string
fieldName string
fieldValue string
wantErr bool
checkContains string
}{
{
Expand Down
Loading