Revert "refactor: deduplicate scope error handling between api/client.go and project queries"#12914
Conversation
….go and project queries"
There was a problem hiding this comment.
Pull request overview
This PR reverts #12845 by restoring project-queries-specific handling for GraphQL “insufficient scopes” errors, rather than relying on the deduplicated API client behavior.
Changes:
- Reintroduces missing-scope detection and error formatting inside
pkg/cmd/project/shared/queriesGraphQL error handling. - Adds unit coverage for parsing required scopes from GraphQL server error messages in project queries.
- Adds a TODO note in
api/client.goflagging duplication in the scope-suggestion messaging.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/cmd/project/shared/queries/queries.go | Restores local insufficient-scope parsing + error construction for project GraphQL calls. |
| pkg/cmd/project/shared/queries/queries_test.go | Adds a new unit test for requiredScopesFromServerMessage. |
| api/client.go | Adds a TODO comment noting duplicated scope-suggestion logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var scopesRE = regexp.MustCompile(`one of the following scopes: \[(.+?)]`) | ||
|
|
||
| func requiredScopesFromServerMessage(msg string) []string { | ||
| m := scopesRE.FindStringSubmatch(msg) | ||
| if m == nil { | ||
| return nil | ||
| } | ||
| var scopes []string | ||
| for _, mm := range strings.Split(m[1], ",") { | ||
| scopes = append(scopes, strings.Trim(mm, "' ")) | ||
| } | ||
| return scopes | ||
| } |
There was a problem hiding this comment.
This file now duplicates scopesRE/requiredScopesFromServerMessage logic that already exists in api/client.go. Keeping two copies increases the chance they drift if the server message format changes; consider factoring this into a shared helper (or exporting/reusing the existing one) so both call sites stay in sync.
| msg: "Your token has not been granted the required scopes to execute this query. The 'dataType' field requires one of the following scopes: ['read:project'], but your token has only been granted the: ['codespace', repo'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.", | ||
| want: []string{"read:project"}, | ||
| }, | ||
| { | ||
| name: "multiple scopes", | ||
| msg: "Your token has not been granted the required scopes to execute this query. The 'dataType' field requires one of the following scopes: ['read:project', 'read:discussion', 'codespace'], but your token has only been granted the: [repo'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.", |
There was a problem hiding this comment.
The sample server messages in this test contain mismatched quotes (e.g. repo'], ['codespace', repo']), which makes the fixture unrealistic and confusing. Please update the strings to valid/representative scope lists so the test documents the actual server error format being parsed.
| msg: "Your token has not been granted the required scopes to execute this query. The 'dataType' field requires one of the following scopes: ['read:project'], but your token has only been granted the: ['codespace', repo'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.", | |
| want: []string{"read:project"}, | |
| }, | |
| { | |
| name: "multiple scopes", | |
| msg: "Your token has not been granted the required scopes to execute this query. The 'dataType' field requires one of the following scopes: ['read:project', 'read:discussion', 'codespace'], but your token has only been granted the: [repo'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.", | |
| msg: "Your token has not been granted the required scopes to execute this query. The 'dataType' field requires one of the following scopes: ['read:project'], but your token has only been granted the: ['codespace', 'repo'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.", | |
| want: []string{"read:project"}, | |
| }, | |
| { | |
| name: "multiple scopes", | |
| msg: "Your token has not been granted the required scopes to execute this query. The 'dataType' field requires one of the following scopes: ['read:project', 'read:discussion', 'codespace'], but your token has only been granted the: ['repo'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.", |
| if got := requiredScopesFromServerMessage(tt.msg); !reflect.DeepEqual(got, tt.want) { | ||
| t.Errorf("requiredScopesFromServerMessage() = %v, want %v", got, tt.want) |
There was a problem hiding this comment.
This test pulls in the reflect package just to compare slices, but the rest of the file already uses testify's assert. Consider switching to assert.Equal/assert.ElementsMatch here to keep assertions consistent and avoid the extra import.
| missing := set.NewStringSet() | ||
| for _, e := range gerr.Errors { | ||
| if e.Type != "INSUFFICIENT_SCOPES" { | ||
| continue | ||
| } | ||
| missing.AddValues(requiredScopesFromServerMessage(e.Message)) | ||
| } | ||
| if missing.Len() > 0 { | ||
| s := missing.ToSlice() | ||
| // TODO: this duplicates parts of generateScopesSuggestion | ||
| return fmt.Errorf( | ||
| "error: your authentication token is missing required scopes %v\n"+ | ||
| "To request it, run: gh auth refresh -s %s", | ||
| s, | ||
| strings.Join(s, ",")) | ||
| } | ||
| } | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
handleError was changed to reimplement GraphQL insufficient-scope handling, but there is no test that exercises handleError end-to-end (e.g., an api.GraphQLError containing multiple INSUFFICIENT_SCOPES items and verifying the returned error message). Adding a focused test will help prevent regressions in how project queries surface missing-scope errors.
| missing := set.NewStringSet() | |
| for _, e := range gerr.Errors { | |
| if e.Type != "INSUFFICIENT_SCOPES" { | |
| continue | |
| } | |
| missing.AddValues(requiredScopesFromServerMessage(e.Message)) | |
| } | |
| if missing.Len() > 0 { | |
| s := missing.ToSlice() | |
| // TODO: this duplicates parts of generateScopesSuggestion | |
| return fmt.Errorf( | |
| "error: your authentication token is missing required scopes %v\n"+ | |
| "To request it, run: gh auth refresh -s %s", | |
| s, | |
| strings.Join(s, ",")) | |
| } | |
| } | |
| return err | |
| } | |
| if scopeErr := handleInsufficientScopesError(gerr); scopeErr != nil { | |
| return scopeErr | |
| } | |
| } | |
| return err | |
| } | |
| // handleInsufficientScopesError extracts missing OAuth scopes from a GraphQL error | |
| // and returns a user-facing error describing how to refresh credentials. | |
| // It is split out from handleError to allow focused testing. | |
| func handleInsufficientScopesError(gerr api.GraphQLError) error { | |
| missing := set.NewStringSet() | |
| for _, e := range gerr.Errors { | |
| if e.Type != "INSUFFICIENT_SCOPES" { | |
| continue | |
| } | |
| missing.AddValues(requiredScopesFromServerMessage(e.Message)) | |
| } | |
| if missing.Len() == 0 { | |
| return nil | |
| } | |
| s := missing.ToSlice() | |
| // TODO: this duplicates parts of generateScopesSuggestion | |
| return fmt.Errorf( | |
| "error: your authentication token is missing required scopes %v\n"+ | |
| "To request it, run: gh auth refresh -s %s", | |
| s, | |
| strings.Join(s, ",")) | |
| } |
williammartin
deleted the
revert-12845-refactor/deduplicate-scope-error-handling
branch
Description
Reverts #12845
Part of fixing #12904