feat: safe-output field aliases for precise agent mistake detection - #48230
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add safeOutputAliases map with 60+ common mistakes (MCP tool names, underscore variants) mapped to their correct hyphenated safe-output fields - Add safeOutputAliasSuggestion() that fires before general field suggestions when an unknown property under /safe-outputs matches a known alias - Hook alias check into generateSchemaBasedSuggestions - Add unit tests for the alias function and integration tests through the full validation pipeline Example: 'create-issue-comment' in safe-outputs now suggests 'add-comment' instead of showing a generic 'Valid fields are: ...' message Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Summary: Adds safe-output field alias detection (e.g. Next action: Defer — author undraft and CI must pass before review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds targeted safe-output alias suggestions for common MCP and underscore naming mistakes.
Changes:
- Adds alias-to-canonical field mappings and suggestion generation.
- Prioritizes precise aliases over generic field suggestions.
- Adds unit/pipeline tests and linter workflow guidance.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schema_suggestions.go |
Prioritizes alias suggestions. |
pkg/parser/schema_safe_output_aliases.go |
Defines aliases and formatting logic. |
pkg/parser/schema_safe_output_aliases_test.go |
Tests alias behavior and validation integration. |
.github/skills/agentic-workflows/SKILL.md |
Adds linter workflow guidance reference. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| "update_project": "update-project", | ||
| "create_project": "create-project", | ||
| "report_failure_as_issue": "report-failure-as-issue", | ||
| "create_agent_session": "create-agent-session", | ||
| "create_agent_task": "create-agent-task", |
| if _, already := seen[canonical]; already { | ||
| continue | ||
| } | ||
| seen[canonical] = struct{}{} | ||
| suggestions = append(suggestions, fmt.Sprintf("'%s'", canonical)) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: safe-output field aliases
Clean, well-scoped addition. The alias map is comprehensive, the priority insertion in generateSchemaBasedSuggestions is correct, and tests cover unit-level cases, deduplication, and full pipeline integration. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.8 AIC · ⌖ 5.14 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — commenting only; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Case-sensitivity boundary: The alias lookup at line 96 of
schema_safe_output_aliases.gois case-sensitive. If the schema validator always normalises property names to lowercase, this is fine — but that invariant is undocumented and untested. - Format inconsistency: Single-match returns
"Did you mean 'x'?"while multi-match returns"Did you mean: 'x', 'y'?"(note the colon). Minor but worth pinning with a test.
Positive Highlights
- ✅ 60+ aliases covering MCP tool names, underscore↔hyphen swaps, and common misspellings — thorough coverage
- ✅ Excellent test structure: unit tests validate alias lookup and deduplication; integration tests verify the full validation pipeline
- ✅ Correct priority ordering in
schema_suggestions.go— alias hints fire before the generic field-list fallback - ✅ Zero-impact scoping: only activates for
/safe-outputsadditional-properties errors - ✅ Clean deduplication when multiple aliases resolve to the same canonical field
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.2 AIC · ⌖ 10.2 AIC · ⊞ 6.7K
Comment /matt to run again
| var suggestions []string | ||
| seen := make(map[string]struct{}) | ||
| for _, prop := range invalidProps { | ||
| canonical, ok := safeOutputAliases[prop] |
There was a problem hiding this comment.
[/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.
| return "" | ||
| } | ||
|
|
||
| if len(suggestions) == 1 { |
There was a problem hiding this comment.
[/grill-with-docs] The single vs. multiple suggestion formats are inconsistent: single uses "Did you mean 'x'?" while multiple uses "Did you mean: 'x', 'y'?" (note the colon). This is a minor inconsistency but could feel odd to agents reading the error.
💡 Suggestion
Unify 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.
There was a problem hiding this comment.
REQUEST_CHANGES — 1 high + 2 medium issues
The alias-suggestion feature is a good UX improvement, but has a correctness gap and two test hygiene problems that should be fixed before merge.
### Blocking issues
1 (high) — Case-sensitive alias lookup (schema_safe_output_aliases.go:88): extractAdditionalPropertyNames returns property names verbatim; the alias map has only lowercase keys. Any mixed-case YAML key from an agent (e.g. Create_Issue_Comment) silently produces no suggestion. Fix: safeOutputAliases[strings.ToLower(prop)].
2 (medium) — Wrong build tag on integration test (schema_safe_output_aliases_test.go:276): TestSafeOutputAliasSuggestion_Integration is named as an integration test but tagged !integration, so it runs in every unit test pass and is skipped in the integration CI — opposite of every other integration test file in this package.
3 (medium) — Cross-file contains() dependency (schema_safe_output_aliases_test.go:339): Silently depends on a helper defined in frontmatter_mcp_test.go. Use strings.Contains inline instead.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 51.3 AIC · ⌖ 4.89 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/parser/schema_safe_output_aliases.go:88
Case-sensitive alias lookup silently misses mixed-case property names: prop from extractAdditionalPropertyNames is used verbatim as a map key, but all keys in safeOutputAliases are lowercase — so inputs like Create_Issue_Comment or Add_Comment will never match.
<details>
<summary>💡 Suggested fix</summary>
Normalize before the lookup:
for _, prop := range invalidProps {
canonical, ok := safeOutputAliases[strings.ToLower(prop)]lowerError already exists in this …
pkg/parser/schema_safe_output_aliases_test.go:276
Integration-weight test is tagged !integration — it runs in every unit test pass: TestSafeOutputAliasSuggestion_Integration writes real files to a temp dir and exercises the full validation pipeline. The file's build tag is //go:build !integration, so it is included in every normal go test run and excluded from integration CI — the opposite of the convention used everywhere else in this package.
<details>
<summary>💡 Suggested fix</summary>
Either:
- Drop the
_Integrationsuffi…
pkg/parser/schema_safe_output_aliases_test.go:339
Invisible dependency on contains() defined in a different test file: contains(haystack, needle string) is defined in frontmatter_mcp_test.go, not in this file or a shared helper. This compiles only because they're in the same package, but there's no indication of the dependency.
<details>
<summary>💡 Suggested fix</summary>
Replace with the stdlib call directly, which is unambiguous and has no hidden dependency:
if !strings.Contains(validationErr.Error(), tt.wantInErr) {…
Captures the design decision to use a curated static alias map in the schema validation layer instead of fuzzy matching, enabling precise "Did you mean 'X'?" hints for common agent mistakes in safe-outputs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (327 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot dash/underscores before doing levenstein search |
- Add missing_tool, missing_data, report_incomplete, create_project_status_update to safeOutputAliases map - Sort suggestions slice before formatting to ensure deterministic output regardless of additional-properties error order - Add unit tests for all four new aliases Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit The CI failure (
|
Replace underscores with hyphens in both target and candidate before computing Levenshtein distance. This makes underscore variants like add_comment find their hyphenated canonical form (add-comment) at distance 0, producing a natural "Did you mean 'add-comment'?" hint without requiring an explicit alias entry for every _ → - swap. The alias map in schema_safe_output_aliases.go is still needed for true remappings (e.g. create_issue_comment → add-comment) where the normalized form is still far from any valid field. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. |
|
@copilot simplify mapping and remove entries that have distance 0 |
…ization Simple underscore→hyphen swaps like add_labels → add-labels are now handled automatically by FindClosestMatches separator normalization (distance 0 after _ → - replacement). Removed all 47 such entries; kept only the 8 true concept remappings for add-comment (MCP tool names and misphrases like create-issue-comment, post-comment, add-issue-comment that remain distance > 3 from add-comment even after normalization). Update unit tests to drop removed cases and use two in-map aliases for the deduplication test. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Removed all 47 pure underscore→hyphen entries (e.g. |
|
🎉 This pull request is included in a new release. Release: |
Agents routinely use MCP tool names (e.g.
create_issue_comment) or underscore variants (e.g.add_comment) insafe-outputsinstead of the correct hyphenated field names. Levenshtein distance is useless here —create-issue-comment→add-commentis distance ~14 — so users got a generic "Valid fields are: ..." dump with no actionable signal.Changes
schema_safe_output_aliases.go— newsafeOutputAliasesmap (60+ entries) covering MCP tool name variants, underscore↔hyphen swaps, and common misphrases (post-comment,add-issue-comment, etc.);safeOutputAliasSuggestion()fires only for/safe-outputsadditional-properties errors and returns a targeted suggestion, deduplicating when multiple aliases resolve to the same canonical fieldschema_suggestions.go— callssafeOutputAliasSuggestionbefore the generaladditionalPropertiesSuggestionso the precise alias hint takes priorityschema_safe_output_aliases_test.go— unit tests for alias lookups, deduplication, non-matching paths; integration tests through the full validation pipeline using real on-disk workflow filesBefore:
After: