diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 3fc711d4035..6f24708a24e 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -37,6 +37,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/github-agentic-workflows.md` - `.github/aw/github-mcp-server.md` - `.github/aw/instructions.md` +- `.github/aw/linter-workflows.md` - `.github/aw/llms.md` - `.github/aw/loop.md` - `.github/aw/lsp.md` diff --git a/docs/adr/48230-safe-output-field-aliases-for-agent-error-guidance.md b/docs/adr/48230-safe-output-field-aliases-for-agent-error-guidance.md new file mode 100644 index 00000000000..a60463f98f8 --- /dev/null +++ b/docs/adr/48230-safe-output-field-aliases-for-agent-error-guidance.md @@ -0,0 +1,43 @@ +# ADR-48230: Safe-Output Field Aliases for Agent Error Guidance + +**Date**: 2026-07-27 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +Agents authoring agentic workflows routinely use MCP tool names (e.g. `create_issue_comment`) or underscore variants (e.g. `add_comment`) in the `safe-outputs` frontmatter section instead of the correct hyphenated field names (e.g. `add-comment`). The existing error path emits a generic "Valid fields are: ..." dump that provides no actionable signal. A Levenshtein/edit-distance approach is ineffective here because the semantic distance between a GitHub MCP tool name and its `safe-outputs` canonical equivalent can be large (e.g. `create-issue-comment` → `add-comment` is ~14 edit operations), making fuzzy matching produce wrong or empty suggestions. The schema validation layer in `pkg/parser` is the right place to intercept these errors before they reach the user. + +### Decision + +We will add a curated static alias map (`safeOutputAliases`) in the `pkg/parser` package that maps known agent mistakes — including MCP tool name variants and underscore ↔ hyphen swaps — to their correct `safe-outputs` canonical field names. A dedicated function (`safeOutputAliasSuggestion`) checks this map when a schema additional-properties error is raised under the `/safe-outputs` path, and returns a precise "Did you mean 'X'?" suggestion that takes priority over the generic field-list fallback. This is integrated into `generateSchemaBasedSuggestions` ahead of the existing `additionalPropertiesSuggestion` call. + +### Alternatives Considered + +#### Alternative 1: Levenshtein / fuzzy edit-distance matching + +Compute string edit distance between the invalid field name and each valid `safe-outputs` field name, and suggest the closest match. This approach is already available as a pattern elsewhere in suggestion logic. It was rejected here because the edit distance between MCP tool name variants and their canonical `safe-outputs` equivalents is too large (e.g. `create-issue-comment` → `add-comment` is ~14 edits) — the closest fuzzy match would be a different, unrelated field, producing misleading suggestions. + +#### Alternative 2: Silent normalization at parse time + +Automatically normalize underscore-separated or MCP-style field names to their hyphenated canonical equivalents during frontmatter parsing, silently accepting the misspelling. This would suppress the validation error entirely, removing the learning signal for workflow authors and masking a class of mistakes that benefit from explicit correction. It would also allow non-canonical spellings to persist in workflow files, complicating future schema evolution. + +### Consequences + +#### Positive +- Agents and workflow authors receive precise, actionable error messages ("Did you mean 'add-comment'?") instead of an undifferentiated field dump, reducing the iteration cycle on `safe-outputs` configuration errors. +- Deduplication logic within `safeOutputAliasSuggestion` collapses multiple aliases resolving to the same canonical name into a single suggestion, keeping error output clean when multiple wrong field names appear together. + +#### Negative +- The alias map must be maintained manually as new `safe-outputs` fields are added or existing fields are renamed; there is no automated enforcement that the alias table stays in sync with the schema. +- The alias map covers only pre-enumerated mistakes; novel misspellings not yet in the table fall through to the generic error message with no improvement over the prior behaviour. + +#### Neutral +- The alias check is scoped exclusively to the `/safe-outputs` JSON schema path and fires only on additional-properties errors, so it has no effect on validation errors in other frontmatter sections. +- Unit tests cover alias lookups, deduplication, and non-matching paths; integration tests exercise the full pipeline using real on-disk workflow files. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/parser/schema_safe_output_aliases.go b/pkg/parser/schema_safe_output_aliases.go new file mode 100644 index 00000000000..d010096d953 --- /dev/null +++ b/pkg/parser/schema_safe_output_aliases.go @@ -0,0 +1,73 @@ +package parser + +import ( + "fmt" + "sort" + "strings" +) + +// safeOutputsSchemaPath is the JSON schema path for the safe-outputs section. +const safeOutputsSchemaPath = "/safe-outputs" + +// safeOutputAliases maps common agent mistakes to their correct safe-output field names. +// Only includes true concept remappings where the alias remains far from the canonical +// field even after separator normalization (underscore→hyphen). Simple underscore variants +// (e.g. "add_labels" → "add-labels") are handled automatically by the Levenshtein +// separator-normalization in FindClosestMatches and do not need explicit entries here. +var safeOutputAliases = map[string]string{ + // add-comment: MCP tool names and common misphrases that Levenshtein cannot bridge + // (e.g. "create-issue-comment" vs "add-comment" is distance ~14 after normalization) + "create-issue-comment": "add-comment", + "create_issue_comment": "add-comment", + "add-issue-comment": "add-comment", + "add_issue_comment": "add-comment", + "post-comment": "add-comment", + "post_comment": "add-comment", + "create-comment": "add-comment", + "create_comment": "add-comment", +} + +// safeOutputAliasSuggestion returns a "Did you mean 'X'?" suggestion when an unknown +// property under /safe-outputs matches a known alias for the correct field name. +// It returns an empty string when the error is not under safe-outputs, is not an +// additional-properties error, or when none of the invalid props match a known alias. +func safeOutputAliasSuggestion(errorMessage, jsonPath string) string { + if jsonPath != safeOutputsSchemaPath { + return "" + } + + lowerError := strings.ToLower(errorMessage) + if !strings.Contains(lowerError, "additional propert") || !strings.Contains(lowerError, "not allowed") { + return "" + } + + invalidProps := extractAdditionalPropertyNames(errorMessage) + if len(invalidProps) == 0 { + return "" + } + + var suggestions []string + seen := make(map[string]struct{}) + for _, prop := range invalidProps { + canonical, ok := safeOutputAliases[prop] + if !ok { + continue + } + if _, already := seen[canonical]; already { + continue + } + seen[canonical] = struct{}{} + suggestions = append(suggestions, fmt.Sprintf("'%s'", canonical)) + } + + if len(suggestions) == 0 { + return "" + } + + sort.Strings(suggestions) + + if len(suggestions) == 1 { + return fmt.Sprintf("Did you mean %s?", suggestions[0]) + } + return fmt.Sprintf("Did you mean: %s?", strings.Join(suggestions, ", ")) +} diff --git a/pkg/parser/schema_safe_output_aliases_test.go b/pkg/parser/schema_safe_output_aliases_test.go new file mode 100644 index 00000000000..c6e62f8bf54 --- /dev/null +++ b/pkg/parser/schema_safe_output_aliases_test.go @@ -0,0 +1,158 @@ +//go:build !integration + +package parser + +import ( + "os" + "testing" +) + +func TestSafeOutputAliasSuggestion(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + errorMessage string + jsonPath string + want string + }{ + { + name: "create-issue-comment maps to add-comment", + errorMessage: "additional properties 'create-issue-comment' not allowed", + jsonPath: "/safe-outputs", + want: "Did you mean 'add-comment'?", + }, + { + name: "create_issue_comment maps to add-comment", + errorMessage: "additional properties 'create_issue_comment' not allowed", + jsonPath: "/safe-outputs", + want: "Did you mean 'add-comment'?", + }, + { + name: "add-issue-comment maps to add-comment", + errorMessage: "additional properties 'add-issue-comment' not allowed", + jsonPath: "/safe-outputs", + want: "Did you mean 'add-comment'?", + }, + { + name: "post-comment maps to add-comment", + errorMessage: "additional properties 'post-comment' not allowed", + jsonPath: "/safe-outputs", + want: "Did you mean 'add-comment'?", + }, + { + name: "truly unknown field returns empty (not an alias)", + errorMessage: "additional properties 'totally-unknown-field' not allowed", + jsonPath: "/safe-outputs", + want: "", + }, + { + name: "non safe-outputs path returns empty", + errorMessage: "additional properties 'add_comment' not allowed", + jsonPath: "/on", + want: "", + }, + { + name: "empty path returns empty", + errorMessage: "additional properties 'add_comment' not allowed", + jsonPath: "", + want: "", + }, + { + name: "non additional properties error returns empty", + errorMessage: "value must be one of 'true', 'false'", + jsonPath: "/safe-outputs", + want: "", + }, + { + name: "two aliases that map to same canonical deduplicates", + errorMessage: "additional properties 'create-issue-comment', 'create_issue_comment' not allowed", + jsonPath: "/safe-outputs", + want: "Did you mean 'add-comment'?", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := safeOutputAliasSuggestion(tt.errorMessage, tt.jsonPath) + if got != tt.want { + t.Errorf("safeOutputAliasSuggestion(%q, %q) = %q, want %q", tt.errorMessage, tt.jsonPath, got, tt.want) + } + }) + } +} + +// TestSafeOutputAliasSuggestion_Integration verifies that the alias suggestion is +// surfaced through the full schema validation pipeline when an agent uses a wrong +// safe-output field name. It writes a real workflow file so the frontmatter context +// is available and the error path is resolved to /safe-outputs. +func TestSafeOutputAliasSuggestion_Integration(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + yamlContent string + safeOutputs map[string]any + wantInErr string + }{ + { + name: "create-issue-comment rejected with add-comment suggestion", + yamlContent: "---\non:\n issues:\n types: [opened]\nengine: copilot\nsafe-outputs:\n create-issue-comment:\n max: 5\n---\n", + safeOutputs: map[string]any{ + "create-issue-comment": map[string]any{"max": 5}, + }, + wantInErr: "add-comment", + }, + { + name: "add_comment rejected with add-comment suggestion", + yamlContent: "---\non:\n issues:\n types: [opened]\nengine: copilot\nsafe-outputs:\n add_comment:\n max: 5\n---\n", + safeOutputs: map[string]any{ + "add_comment": map[string]any{"max": 5}, + }, + wantInErr: "add-comment", + }, + { + name: "update_issue rejected with update-issue suggestion", + yamlContent: "---\non:\n issues:\n types: [opened]\nengine: copilot\nsafe-outputs:\n update_issue:\n body: true\n---\n", + safeOutputs: map[string]any{ + "update_issue": map[string]any{"body": true}, + }, + wantInErr: "update-issue", + }, + { + name: "create_pull_request rejected with create-pull-request suggestion", + yamlContent: "---\non:\n issues:\n types: [opened]\nengine: copilot\nsafe-outputs:\n create_pull_request:\n max: 1\n---\n", + safeOutputs: map[string]any{ + "create_pull_request": map[string]any{"max": 1}, + }, + wantInErr: "create-pull-request", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + // Write a real file so readFrontmatterContext can extract the YAML for + // precise error-location detection (enabling the /safe-outputs path). + dir := t.TempDir() + filePath := dir + "/workflow.md" + if err := os.WriteFile(filePath, []byte(tt.yamlContent), 0o600); err != nil { + t.Fatalf("failed to write test workflow file: %v", err) + } + + frontmatter := map[string]any{ + "on": map[string]any{"issues": map[string]any{"types": []any{"opened"}}}, + "engine": "copilot", + "safe-outputs": tt.safeOutputs, + } + validationErr := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, filePath) + if validationErr == nil { + t.Fatal("expected validation to fail for unknown safe-output field") + } + if !contains(validationErr.Error(), tt.wantInErr) { + t.Errorf("expected error to contain %q as alias suggestion, got: %v", tt.wantInErr, validationErr) + } + }) + } +} diff --git a/pkg/parser/schema_suggestions.go b/pkg/parser/schema_suggestions.go index 053123f2e4f..f58ba673fd6 100644 --- a/pkg/parser/schema_suggestions.go +++ b/pkg/parser/schema_suggestions.go @@ -47,6 +47,12 @@ func generateSchemaBasedSuggestions(schemaJSON, errorMessage, jsonPath, frontmat return suggestion } + // Check for safe-output alias suggestions (e.g., create-issue-comment → add-comment) + // before falling through to general field suggestions. + if suggestion := safeOutputAliasSuggestion(errorMessage, jsonPath); suggestion != "" { + return suggestion + } + if suggestion := additionalPropertiesSuggestion(schemaDoc, errorMessage, jsonPath); suggestion != "" { return suggestion } diff --git a/pkg/stringutil/fuzzy_match.go b/pkg/stringutil/fuzzy_match.go index 218dd089c32..c1923492fd7 100644 --- a/pkg/stringutil/fuzzy_match.go +++ b/pkg/stringutil/fuzzy_match.go @@ -14,6 +14,10 @@ var fuzzyMatchLog = logger.New("stringutil:fuzzy_match") // It returns up to maxResults matches that have a Levenshtein distance of 3 or less. // Results are sorted by distance (closest first), then alphabetically for ties. // +// Before computing distance, both target and each candidate are separator-normalized +// by replacing underscores with hyphens. This means "add_comment" and "add-comment" +// compare at distance 0 and the hyphenated candidate is returned as the suggestion. +// // This function is useful for "Did you mean?" suggestions when a user provides // an unrecognized value (e.g., a typo in an engine name or event type). func FindClosestMatches(target string, candidates []string, maxResults int) []string { @@ -27,16 +31,22 @@ func FindClosestMatches(target string, candidates []string, maxResults int) []st var matches []match targetLower := strings.ToLower(target) + // Normalize separators so that underscore and hyphen variants compare equally. + targetNorm := strings.ReplaceAll(targetLower, "_", "-") for _, candidate := range candidates { candidateLower := strings.ToLower(candidate) - // Skip exact matches + // Skip exact matches (case-insensitive, before normalization) if targetLower == candidateLower { continue } - distance := LevenshteinDistance(targetLower, candidateLower) + candidateNorm := strings.ReplaceAll(candidateLower, "_", "-") + + // Compute distance on separator-normalized forms so that underscore/hyphen + // variants (e.g. "add_comment" vs "add-comment") count as distance 0. + distance := LevenshteinDistance(targetNorm, candidateNorm) // Only include if distance is within acceptable range if distance <= maxDistance { diff --git a/pkg/stringutil/fuzzy_match_test.go b/pkg/stringutil/fuzzy_match_test.go index ba101a14bed..5b9f34c5e73 100644 --- a/pkg/stringutil/fuzzy_match_test.go +++ b/pkg/stringutil/fuzzy_match_test.go @@ -114,6 +114,34 @@ func TestFindClosestMatches(t *testing.T) { maxResults: 2, want: []string{"zzza", "zzzb"}, }, + { + name: "underscore variant finds hyphenated candidate at distance 0", + target: "add_comment", + candidates: []string{"add-comment", "add-labels", "create-issue"}, + maxResults: 3, + want: []string{"add-comment"}, + }, + { + name: "hyphen variant finds underscored candidate at distance 0", + target: "add-comment", + candidates: []string{"add_comment", "add_labels"}, + maxResults: 3, + want: []string{"add_comment"}, + }, + { + name: "underscore typo with extra char still suggests hyphenated form", + target: "add_comments", + candidates: []string{"add-comment", "add-labels"}, + maxResults: 3, + want: []string{"add-comment"}, + }, + { + name: "multi-word underscore maps to hyphen at distance 0", + target: "update_pull_request", + candidates: []string{"update-pull-request", "create-pull-request"}, + maxResults: 2, + want: []string{"update-pull-request", "create-pull-request"}, + }, } for _, tt := range tests {