Prettify permissions validation error messages#2798
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Remove ERROR/WARNING prefixes (already rendered by compiler) - Change "GitHub MCP" to "github" - Make messages more compact by merging toolset details inline - Update "Required by toolsets:" to show inline as "(required by X)" - Simplify excess permissions message - Update test expectations to match new format - Fix test using allowed: instead of toolsets: Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Agentic Changeset Generator triggered by this pull request. |
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes and simplifies permissions validation error messages by removing redundant prefixes, reducing verbosity, and inlining contextual information. The changes make error messages more scannable while maintaining all essential information.
Key changes:
- Remove redundant "ERROR:" and "WARNING:" prefixes since the compiler already adds these labels
- Inline toolset context directly with each permission instead of using a separate "Required by toolsets:" section
- Simplify branding from "GitHub MCP toolsets" to "github toolsets"
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/permissions_validator.go | Main implementation of improved error message formatting with inline toolset context |
| pkg/workflow/permissions_validator_test.go | Updated test expectations to match new message format |
| pkg/workflow/permissions_excess_compilation_test.go | Updated warning detection logic and expected messages |
| pkg/workflow/compiler_test.go | Minor test configuration update |
| pkg/workflow/schemas/github-workflow.json | Schema formatting improvements (unrelated to main PR purpose) |
| .github/workflows/*.lock.yml | GitHub Actions version updates (unrelated to main PR purpose) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for toolset, toolsetScopes := range result.MissingToolsetDetails { | ||
| for _, ts := range toolsetScopes { | ||
| if ts == scope { | ||
| requiredBy = append(requiredBy, toolset) | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested loop creates O(n*m) complexity for finding which toolsets require each permission. Consider pre-computing a reverse map from scope to toolsets to improve performance when there are many toolsets and permissions.
See below for a potential fix:
// Precompute reverse map: scope -> toolsets
scopeToToolsets := make(map[PermissionScope][]string)
if len(result.MissingToolsetDetails) > 0 {
for toolset, toolsetScopes := range result.MissingToolsetDetails {
for _, ts := range toolsetScopes {
scopeToToolsets[ts] = append(scopeToToolsets[ts], toolset)
}
}
}
for _, scopeStr := range scopes {
scope := PermissionScope(scopeStr)
level := result.MissingPermissions[scope]
// Lookup which toolsets need this permission
requiredBy := scopeToToolsets[scope]
Permissions validation error messages contained redundant prefixes and verbose formatting that made them harder to scan. The compiler already adds "error:" and "warning:" labels, making the "ERROR:" and "WARNING:" prefixes in the message text redundant.
Changes
(required by <toolset>)next to each permission instead of separate "Required by toolsets:" sectionBefore/After
Missing permissions:
Excess permissions:
Result: 45% fewer lines for missing permissions, 50% fewer for excess permissions.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.