-
Notifications
You must be signed in to change notification settings - Fork 466
feat: safe-output field aliases for precise agent mistake detection #48230
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.
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
Changes from all commits
29138d8
9a584d9
79036f6
8b23e4d
6acf248
df80177
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)) | ||
|
Comment on lines
+56
to
+60
|
||
| } | ||
|
|
||
| if len(suggestions) == 0 { | ||
| return "" | ||
| } | ||
|
|
||
| sort.Strings(suggestions) | ||
|
|
||
| if len(suggestions) == 1 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/grill-with-docs] The single vs. multiple suggestion formats are inconsistent: single uses 💡 SuggestionUnify to one format, e.g.: if len(suggestions) == 1 {
return fmt.Sprintf("Did you mean %s?", suggestions[0])
}
return fmt.Sprintf("Did you mean %s?", strings.Join(suggestions, " or "))Or keep the colon consistently for both cases. Add a test that pins whichever format you choose. @copilot please address this. |
||
| return fmt.Sprintf("Did you mean %s?", suggestions[0]) | ||
| } | ||
| return fmt.Sprintf("Did you mean: %s?", strings.Join(suggestions, ", ")) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The alias map lookup is case-sensitive, but agents may produce mixed-case field names (e.g.
ADD_COMMENT,Create_Issue_Comment). There is no test covering this boundary.💡 Suggestion
If the schema validator always normalizes property names to lowercase (making this safe), add a comment stating that invariant plus a test confirming the boundary:
{ name: "uppercase alias returns empty (schema validator normalises case)", errorMessage: "additional properties 'ADD_COMMENT' not allowed", jsonPath: "/safe-outputs", want: "", },If not, normalize before the lookup:
safeOutputAliases[strings.ToLower(prop)].@copilot please address this.