Skip to content

refactor(workflow): rename same-name/different-semantics helpers to eliminate navigation confusion #44149

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 12 commits into from
Jul 8, 2026
+28 −28
Merged

refactor(workflow): rename same-name/different-semantics helpers to eliminate navigation confusion #44149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0bbc815
Initial plan
Copilot Jul 8, 2026
5c148a3
refactor: rename same-name/different-semantics helpers to avoid confu…
Copilot Jul 8, 2026
80640b5
Merge remote-tracking branch 'origin/main' into copilot/refactor-sema…
Copilot Jul 8, 2026
164dd59
chore: merge main and recompile workflows
Copilot Jul 8, 2026
ade02bd
wip: plan for addressing review threads
Copilot Jul 8, 2026
19c0297
refactor: address review feedback — rename helpers for consistency an…
Copilot Jul 8, 2026
255e5a2
Merge remote-tracking branch 'origin/main' into copilot/refactor-sema…
Copilot Jul 8, 2026
d2a9af6
Apply remaining changes
Copilot Jul 8, 2026
ee01991
Merge remote-tracking branch 'origin/main' into copilot/refactor-sema…
Copilot Jul 8, 2026
7ec831a
chore: merge main and recompile workflows
Copilot Jul 8, 2026
5607ad3
Merge remote-tracking branch 'origin/main' into copilot/refactor-sema…
Copilot Jul 8, 2026
5890c9e
Merge branch 'main' into copilot/refactor-semantic-function-clustering
pelikhan Jul 8, 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
14 changes: 7 additions & 7 deletions pkg/workflow/agentic_workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func TestAgenticWorkflowsSyntaxVariations(t *testing.T) {
c := testCompiler()

// Extract tools from frontmatter
tools := extractToolsFromFrontmatter(frontmatter)
tools := extractToolsMapFromFrontmatter(frontmatter)

// Merge tools
mergedTools, err := c.mergeToolsAndMCPServers(tools, make(map[string]any), "")
Expand Down Expand Up @@ -268,7 +268,7 @@ func TestAgenticWorkflowsErrorCases(t *testing.T) {
c := testCompiler()

// Extract tools from frontmatter
tools := extractToolsFromFrontmatter(frontmatter)
tools := extractToolsMapFromFrontmatter(frontmatter)

// Merge tools
mergedTools, err := c.mergeToolsAndMCPServers(tools, make(map[string]any), "")
Expand Down Expand Up @@ -360,7 +360,7 @@ func TestAgenticWorkflowsNilSafety(t *testing.T) {
}
}

// TestAgenticWorkflowsExtractToolsEdgeCases tests edge cases in extractToolsFromFrontmatter
// TestAgenticWorkflowsExtractToolsEdgeCases tests edge cases in extractToolsMapFromFrontmatter
func TestAgenticWorkflowsExtractToolsEdgeCases(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -418,11 +418,11 @@ func TestAgenticWorkflowsExtractToolsEdgeCases(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Test that extractToolsFromFrontmatter doesn't panic
// Test that extractToolsMapFromFrontmatter doesn't panic
var result map[string]any
assert.NotPanics(t, func() {
result = extractToolsFromFrontmatter(tt.frontmatter)
}, "extractToolsFromFrontmatter should handle edge cases without panicking")
result = extractToolsMapFromFrontmatter(tt.frontmatter)
}, "extractToolsMapFromFrontmatter should handle edge cases without panicking")

// Verify the expected result
if tt.expectTools {
Expand All @@ -432,7 +432,7 @@ func TestAgenticWorkflowsExtractToolsEdgeCases(t *testing.T) {
"extracted tools should contain agentic-workflows for: %s", tt.description)
} else {
// ExtractMapField returns empty map (not nil) when field is missing or invalid
assert.NotNil(t, result, "extractToolsFromFrontmatter should always return non-nil map")
assert.NotNil(t, result, "extractToolsMapFromFrontmatter should always return non-nil map")
assert.Empty(t, result, "should return empty tools map for: %s", tt.description)
}
})
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/compiler_orchestrator_tools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func (c *Compiler) resolveToolsConfiguration(
importsResult *parser.ImportsResult,
agenticEngine CodingAgentEngine,
) (*mergedToolsData, error) {
topTools := extractToolsFromFrontmatter(result.Frontmatter)
topTools := extractToolsMapFromFrontmatter(result.Frontmatter)
if err := ValidateToolsSection(topTools); err != nil {
return nil, err
}
Expand All @@ -185,7 +185,7 @@ func (c *Compiler) resolveToolsConfiguration(
return nil, fmt.Errorf("failed to expand includes for tools: %w", err)
}
allIncludedTools := strings.Join(nonEmptyStrings(importsResult.MergedTools, includedTools), "\n")
mcpServers := extractMCPServersFromFrontmatter(result.Frontmatter)
mcpServers := extractMCPServersMapFromFrontmatter(result.Frontmatter)
resolvedMCPServers, err := c.mergeImportedMCPServers(mcpServers, importsResult.MergedMCPServers)
if err != nil {
return nil, err
Expand Down Expand Up @@ -264,7 +264,7 @@ func (c *Compiler) extractToolTimeouts(tools map[string]any) (string, string, er
}

func (c *Compiler) resolveRuntimes(frontmatter map[string]any, importsResult *parser.ImportsResult) (map[string]any, bool, error) {
topRuntimes := extractRuntimesFromFrontmatter(frontmatter)
topRuntimes := extractRuntimesMapFromFrontmatter(frontmatter)
orchestratorToolsLog.Printf("Merging runtimes")
runtimes, err := mergeRuntimes(topRuntimes, importsResult.MergedRuntimes)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/copilot_git_commands_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func (c *Compiler) parseCopilotWorkflowMarkdownContentWithToolArgs(content strin
safeOutputs := c.extractSafeOutputsConfig(result.Frontmatter)

// Extract and process tools
topTools := extractToolsFromFrontmatter(result.Frontmatter)
topTools := extractToolsMapFromFrontmatter(result.Frontmatter)
topTools = c.applyDefaultTools(topTools, safeOutputs, nil, nil)

// Build basic workflow data for testing
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/expression_extraction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ func SubstituteImportInputs(content string, importInputs map[string]any) string
}
path := matches[1]
// Resolve potentially dotted path (e.g. "config.apiKey" for object inputs)
if value, found := resolveImportInputPath(importInputs, path); found {
if value, found := resolveImportInputByPath(importInputs, path); found {
strValue := marshalImportInputValue(value)
expressionExtractionLog.Printf("Substituting github.aw.%s.%s with value: %s", inputCategory, path, strValue)
return strValue
Expand Down Expand Up @@ -529,11 +529,11 @@ func marshalImportInputValue(value any) string {
return fmt.Sprintf("%v", value)
}

// resolveImportInputPath resolves a potentially dotted key path from the importInputs map.
// resolveImportInputByPath resolves a potentially dotted key path from the importInputs map.
// For scalar inputs ("count"), it looks up importInputs["count"] directly.
// For object sub-key paths ("config.apiKey"), it looks up importInputs["config"]["apiKey"],
// supporting one level of nesting as defined by import-schema object types.
// Returns the resolved value and true on success, or nil and false when the path is not found.
func resolveImportInputPath(importInputs map[string]any, path string) (any, bool) {
func resolveImportInputByPath(importInputs map[string]any, path string) (any, bool) {
return importinpututil.ResolvePathValue(importInputs, path)
}
2 changes: 1 addition & 1 deletion pkg/workflow/firewall_workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func TestFirewallWorkflowCompilation(t *testing.T) {
c.SetSkipValidation(true)

// Extract and verify tools
tools := extractToolsFromFrontmatter(frontmatter)
tools := extractToolsMapFromFrontmatter(frontmatter)
if _, exists := tools["web-fetch"]; !exists {
t.Error("web-fetch tool should be present in firewall workflow")
}
Expand Down
12 changes: 6 additions & 6 deletions pkg/workflow/frontmatter_extraction_metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,17 +301,17 @@ func (c *Compiler) extractToolsStartupTimeout(tools map[string]any) (string, err
return "", nil
}

// extractToolsFromFrontmatter extracts tools section from frontmatter map
func extractToolsFromFrontmatter(frontmatter map[string]any) map[string]any {
// extractToolsMapFromFrontmatter extracts tools section from frontmatter map
func extractToolsMapFromFrontmatter(frontmatter map[string]any) map[string]any {

@github-actions github-actions Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

...Map suffix is inconsistent with the two sibling helpers it's supposed to align with: extractMCPServersFromFrontmatter and extractRuntimesFromFrontmatter both return map[string]any and both delegate to ExtractMapField — yet neither uses the ...Map suffix.

💡 Details

The PR description justifies the new name:

The ...Map suffix aligns with sibling helpers extractMCPServersFromFrontmatter / extractRuntimesFromFrontmatter.

But those siblings don't use the ...Map suffix:

// frontmatter_extraction_metadata.go
func extractToolsMapFromFrontmatter(...)     map[string]any { ... }  // renamed, has Map
func extractMCPServersFromFrontmatter(...)   map[string]any { ... }  // sibling, no Map
func extractRuntimesFromFrontmatter(...)     map[string]any { ... }  // sibling, no Map

The rename has created asymmetry rather than resolved it. Either all three should carry ...Map or none should. If the goal is purely to avoid collision with parser.extractToolsFromFrontmatter, the comment on the function is a better place to note the distinction than embedding implementation detail in the name.

Copilot AI Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied ...Map to both siblings as well: extractMCPServersFromFrontmatter → extractMCPServersMapFromFrontmatter and extractRuntimesFromFrontmatter → extractRuntimesMapFromFrontmatter. All three helpers in the group now carry the suffix consistently.

return ExtractMapField(frontmatter, "tools")
}

// extractMCPServersFromFrontmatter extracts mcp-servers section from frontmatter
func extractMCPServersFromFrontmatter(frontmatter map[string]any) map[string]any {
// extractMCPServersMapFromFrontmatter extracts mcp-servers section from frontmatter
func extractMCPServersMapFromFrontmatter(frontmatter map[string]any) map[string]any {
return ExtractMapField(frontmatter, "mcp-servers")
}

// extractRuntimesFromFrontmatter extracts runtimes section from frontmatter map
func extractRuntimesFromFrontmatter(frontmatter map[string]any) map[string]any {
// extractRuntimesMapFromFrontmatter extracts runtimes section from frontmatter map
func extractRuntimesMapFromFrontmatter(frontmatter map[string]any) map[string]any {
return ExtractMapField(frontmatter, "runtimes")
}
2 changes: 1 addition & 1 deletion pkg/workflow/git_commands_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ func (c *Compiler) parseWorkflowMarkdownContentWithToolsString(content string) (
safeOutputs := c.extractSafeOutputsConfig(result.Frontmatter)

// Extract and process tools
topTools := extractToolsFromFrontmatter(result.Frontmatter)
topTools := extractToolsMapFromFrontmatter(result.Frontmatter)
topTools = c.applyDefaultTools(topTools, safeOutputs, nil, nil)

// Extract cache-memory config
Expand Down
12 changes: 6 additions & 6 deletions pkg/workflow/tools_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ func TestPlaywrightConfigParsing(t *testing.T) {
})
}

func TestExtractToolsFromFrontmatter(t *testing.T) {
func TestExtractToolsMapFromFrontmatter(t *testing.T) {
frontmatter := map[string]any{
"tools": map[string]any{
"github": nil,
Expand All @@ -370,7 +370,7 @@ func TestExtractToolsFromFrontmatter(t *testing.T) {
},
}

result := extractToolsFromFrontmatter(frontmatter)
result := extractToolsMapFromFrontmatter(frontmatter)

Copilot AI Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit — renamed to TestExtractToolsMapFromFrontmatter. Also renamed the sibling test functions (TestExtractMCPServersMapFromFrontmatter, TestExtractRuntimesMapFromFrontmatter) for consistency with the sibling helper renames.


if len(result) != 2 {
t.Errorf("expected 2 tools, got %d", len(result))
Expand All @@ -390,7 +390,7 @@ func TestExtractToolsFromFrontmatter(t *testing.T) {
}
}

func TestExtractMCPServersFromFrontmatter(t *testing.T) {
func TestExtractMCPServersMapFromFrontmatter(t *testing.T) {
frontmatter := map[string]any{
"tools": map[string]any{
"github": nil,
Expand All @@ -401,7 +401,7 @@ func TestExtractMCPServersFromFrontmatter(t *testing.T) {
},
}

result := extractMCPServersFromFrontmatter(frontmatter)
result := extractMCPServersMapFromFrontmatter(frontmatter)

if len(result) != 2 {
t.Errorf("expected 2 MCP servers, got %d", len(result))
Expand All @@ -421,7 +421,7 @@ func TestExtractMCPServersFromFrontmatter(t *testing.T) {
}
}

func TestExtractRuntimesFromFrontmatter(t *testing.T) {
func TestExtractRuntimesMapFromFrontmatter(t *testing.T) {
frontmatter := map[string]any{
"tools": map[string]any{
"github": nil,
Expand All @@ -432,7 +432,7 @@ func TestExtractRuntimesFromFrontmatter(t *testing.T) {
},
}

result := extractRuntimesFromFrontmatter(frontmatter)
result := extractRuntimesMapFromFrontmatter(frontmatter)

if len(result) != 2 {
t.Errorf("expected 2 runtimes, got %d", len(result))
Expand Down
Loading