Add codemod to migrate tools.serena to shared Serena import

[Copilot]

Summary

• add a dedicated codemod that migrates tools.serena to imports with shared/mcp/serena.md
• preserve extracted language configuration via with.languages (short and long syntax supported)
• avoid duplicate Serena imports when one already exists, while still removing tools.serena
• register the codemod in the global codemod registry
• update codemod registry tests for count/order and add focused codemod unit tests

Validation

• go test -v -run "TestSerenaToSharedImportCodemod|TestGetAllCodemods" ./pkg/cli/ ✅
• make agent-finish ⚠️ fails in security-gosec due to pre-existing repository findings unrelated to this change

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 95/100

✅ Excellent test quality

Metric	Value
New/modified tests analyzed	6 (4 sub-tests + 2 registration assertions)
✅ Design tests (behavioral contracts)	6 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	6 (100%)
Duplicate test clusters	0
Test inflation detected	No (155 test lines / 269 production lines = 0.58 ratio)
🚨 Coding-guideline violations	None

---

Test Classification Details

View All Test Classifications

Test	File	Classification	Notes
TestSerenaToSharedImportCodemod / migrates tools.serena short syntax to imports	pkg/cli/codemod_serena_import_test.go:16	✅ Design	Verifies content transformation, removes serena key, adds import block, validates output parsability
TestSerenaToSharedImportCodemod / migrates tools.serena long syntax languages object to imports	pkg/cli/codemod_serena_import_test.go:58	✅ Design	Verifies object-style language keys are flattened to array — tests a distinct input variant
TestSerenaToSharedImportCodemod / removes tools.serena when shared import already exists	pkg/cli/codemod_serena_import_test.go:100	✅ Design	Verifies idempotency / no-duplicate guarantee — strong edge case
TestSerenaToSharedImportCodemod / does not modify workflows without tools.serena	pkg/cli/codemod_serena_import_test.go:137	✅ Design	Verifies no-op condition when guard is absent — explicit negative path
TestGetAllCodemods_ContainsExpectedCodemods (modified)	pkg/cli/fix_codemods_test.go:83	✅ Design	Registration behavioral contract: new codemod is registered
TestGetAllCodemods_InExpectedOrder (modified)	pkg/cli/fix_codemods_test.go:137	✅ Design	Ordering behavioral contract: codemod appears in expected position

---

Compliance Checks

Check	Result
//go:build !integration on new test file	✅ Present on line 1
No forbidden mock libraries (gomock, testify/mock, .EXPECT(), .On())	✅ None found
Descriptive assertion messages	✅ All assertions include a context string
require.* used for setup / assert.* for validations	✅ Correct discipline
Test inflation (ratio > 2:1)	✅ 0.58 — well within bounds

---

Minor Observation (not a flag)

The four sub-tests all assert require.NoError on the happy-path return, which is good hygiene. One gap: there are no tests for inputs that should cause Apply to return an error (e.g., malformed YAML, completely corrupt frontmatter). This is a minor coverage gap for the error-return branch of codemod.Apply. Not enough to flag, but worth adding in a follow-up.

---

Language Support

• 🐹 Go (*_test.go): 6 test scenarios — unit (//go:build !integration)
• 🟨 JavaScript (*.test.cjs, *.test.js): 0 tests modified

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). All tests verify observable behavioral contracts: the codemod correctly transforms two distinct input syntaxes, prevents duplicate imports, and implements a clean no-op path. No coding-guideline violations detected.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §24680939253

🧪 Test quality analysis by Test Quality Sentinel · ● 600.7K · ◷

[github-actions]

✅ Test Quality Sentinel: 95/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 4 sub-tests in the new codemod test file verify observable behavioral contracts with proper error checking, edge cases (duplicate prevention, no-op path), and descriptive assertion messages. No coding-guideline violations detected.

[Copilot]

Pull request overview

Adds a new CLI codemod to migrate removed tools.serena configuration to the shared Serena workflow import (shared/mcp/serena.md), and wires it into the global codemod registry with accompanying tests.

Changes:

• Introduce serena-tools-to-shared-import codemod to remove tools.serena and add an equivalent imports: - uses: shared/mcp/serena.md entry (preserving languages).
• Register the new codemod in GetAllCodemods() and update registry tests for count/ordering.
• Add dedicated unit tests covering short/long Serena language syntax and duplicate-import avoidance.

Show a summary per file

File	Description
pkg/cli/fix_codemods.go	Registers the new Serena migration codemod in the global registry.
pkg/cli/fix_codemods_test.go	Updates registry tests for new codemod count, presence, and ordering.
pkg/cli/codemod_serena_import.go	Implements the tools.serena → imports migration logic.
pkg/cli/codemod_serena_import_test.go	Adds unit tests for the new Serena migration codemod 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 (3)

pkg/cli/codemod_serena_import.go:77

• extractSerenaLanguages treats []string values differently from the []any case: it doesn’t trim whitespace or filter out empty strings, and it returns ok based on the original slice length rather than the cleaned result. This can migrate invalid language entries like "" or " go" into the generated imports.with.languages list. Consider normalizing []string by trimming/filtering empties (and basing ok on the normalized slice length) to match the behavior of the []any path.

func extractSerenaLanguages(serenaAny any) ([]string, bool) {
	switch serena := serenaAny.(type) {
	case []string:
		return sliceutil.Deduplicate(serena), len(serena) > 0
	case []any:

pkg/cli/codemod_serena_import.go:208

• addSerenaImport inserts a new top-level imports: block immediately after the first engine: line (insertAt = i + 1). If engine: is a mapping block (e.g. engine:\n id: ...), inserting imports: before the indented engine children will break the YAML structure. The insertion point should be after the end of the entire engine block (or another stable top-level boundary), not just after the engine: header line.

	insertAt := 0
	for i, line := range lines {
		if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), "engine:") {
			insertAt = i + 1
			break

pkg/cli/codemod_serena_import.go:201

• When an imports: key already exists, the codemod appends an indented list entry without checking whether imports: is declared in flow/scalar form on the same line (e.g. imports: [], imports: ["..."], or imports: shared/mcp/serena.md). In those cases, appending block-sequence lines ( - uses: ...) will produce invalid YAML. Consider detecting an inline value after imports: and rewriting it into a multi-line block sequence before appending, or falling back to a structured frontmatter rewrite for imports.

		trimmed := strings.TrimSpace(line)
		if isTopLevelKey(line) && strings.HasPrefix(trimmed, "imports:") {
			importsIdx = i
			for j := i + 1; j < len(lines); j++ {
				if isTopLevelKey(lines[j]) {
					importsEnd = j
					break
				}
			}
			break
		}
	}

	if importsIdx >= 0 {
		result := make([]string, 0, len(lines)+len(entry))
		result = append(result, lines[:importsEnd]...)
		result = append(result, entry...)
		result = append(result, lines[importsEnd:]...)
		return result

• Files reviewed: 4/4 changed files
• Comments generated: 2

[Copilot]

The new codemod tests cover the common block-style YAML cases, but they don’t exercise the edge cases that the implementation is currently sensitive to (e.g. engine: as a mapping block and imports: declared in flow/scalar form like imports: []). Adding focused cases for these formats would help prevent regressions and would have caught the YAML-invalid insertion paths.

[Copilot]

If tools.serena is present but the languages value is invalid/empty, the codemod currently returns (content, false, nil) and leaves tools.serena in place (lines 39-43). Since tools.serena is no longer a recognized built-in tool (and workflows using it fail to compile per the docs), it’s safer for the codemod to still remove tools.serena even when it cannot confidently construct the replacement import (optionally logging a warning that the user must add imports: - uses: shared/mcp/serena.md manually).

This issue also appears in the following locations of the same file:

• line 73
• line 183
• line 204