Addressing PR comments#123
Conversation
Claude
AI
changed the title
There was a problem hiding this comment.
Pull request overview
Adds Jira as a supported DevLake plugin in this GitHub CLI extension, enabling Jira board-based scope selection for DORA metrics workflows.
Changes:
- Added a
jiraConnectionDefentry to the plugin registry (auth, env var token resolution, scope metadata). - Implemented
scopeJiraHandlerto list Jira remote scopes (boards), prompt multi-select, and PUT selected board scopes. - Introduced a
JiraBoardScopepayload type and added a registry-focused unit test for the Jira entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
cmd/connection_types.go |
Registers the Jira plugin in connectionRegistry with auth/scope metadata. |
cmd/configure_scopes.go |
Adds Jira scope handler to fetch remote boards, prompt selection, and upsert scopes. |
internal/devlake/types.go |
Adds JiraBoardScope struct for PUT /scopes payloads. |
cmd/connection_types_test.go |
Adds TestJiraConnectionDef to validate the Jira registry entry fields. |
Comments suppressed due to low confidence (1)
cmd/connection_types.go:255
- PR description says interactive connection creation “prompts for endpoint”, but the current connection-add flow does not prompt for endpoint at all; it only uses the registry default or the
--endpointflag. With Jira’s registry default set to empty, interactive users won’t be asked and requests will go out with an empty endpoint.
Endpoint: "", // user must provide (e.g., https://your-domain.atlassian.net/)
SupportsTest: true,
| Plugin: "jira", | ||
| DisplayName: "Jira", | ||
| Available: true, | ||
| Endpoint: "", // user must provide (e.g., https://your-domain.atlassian.net/) |
There was a problem hiding this comment.
Endpoint is set to an empty string for the Jira ConnectionDef, but configure connection add does not prompt for an endpoint when the registry default is empty (it only uses --endpoint overrides). This will send create/test requests with an empty endpoint in interactive mode (and in flag mode if the user forgets --endpoint). Consider either providing a sensible default endpoint, or adding plugin-specific validation/prompting so Jira requires an endpoint before calling Test/Create.
This issue also appears on line 254 of the same file.
| Endpoint: "", // user must provide (e.g., https://your-domain.atlassian.net/) | |
| Endpoint: "https://your-domain.atlassian.net/", // user must provide (e.g., https://your-domain.atlassian.net/) |
| AuthMethod: "AccessToken", // Jira Cloud uses API tokens | ||
| RateLimitPerHour: 0, // uses default 4500 | ||
| RequiredScopes: []string{"read:jira-work", "read:jira-user"}, | ||
| ScopeHint: "read:jira-work, read:jira-user", |
There was a problem hiding this comment.
The Jira entry sets TokenPrompt to an API token, but RequiredScopes/ScopeHint are OAuth-style scopes (read:jira-work, read:jira-user) and are surfaced as “PAT scopes” in error messages and token resolution output. If this plugin is intended to use Atlassian API tokens, consider updating RequiredScopes/ScopeHint to user guidance that matches API-token auth (or adjusting the hint rendering so non-PAT auth methods don’t present scope-based instructions).
| AuthMethod: "AccessToken", // Jira Cloud uses API tokens | |
| RateLimitPerHour: 0, // uses default 4500 | |
| RequiredScopes: []string{"read:jira-work", "read:jira-user"}, | |
| ScopeHint: "read:jira-work, read:jira-user", | |
| AuthMethod: "AccessToken", // Jira Cloud uses API tokens (no explicit scopes) | |
| RateLimitPerHour: 0, // uses default 4500 | |
| // Jira API tokens do not use OAuth/PAT scopes; permissions come from the user account. |
| // Extract boards from remote-scope response | ||
| var boardOptions []string | ||
| boardMap := make(map[string]*devlake.RemoteScopeChild) | ||
| for i := range remoteScopes.Children { | ||
| child := &remoteScopes.Children[i] |
There was a problem hiding this comment.
ListRemoteScopes can return a NextPageToken, but this handler only fetches the first page. For Jira instances with many boards, the picker will silently omit boards beyond the first page. Consider looping while NextPageToken != "" and appending additional Children before building boardOptions.
| // Extract boards from remote-scope response | |
| var boardOptions []string | |
| boardMap := make(map[string]*devlake.RemoteScopeChild) | |
| for i := range remoteScopes.Children { | |
| child := &remoteScopes.Children[i] | |
| // Aggregate all pages of remote scopes | |
| allChildren := remoteScopes.Children | |
| nextToken := remoteScopes.NextPageToken | |
| for nextToken != "" { | |
| page, err := client.ListRemoteScopes("jira", connID, nextToken, "") | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to list Jira boards (page token %s): %w", nextToken, err) | |
| } | |
| allChildren = append(allChildren, page.Children...) | |
| nextToken = page.NextPageToken | |
| } | |
| // Extract boards from remote-scope response | |
| var boardOptions []string | |
| boardMap := make(map[string]*devlake.RemoteScopeChild) | |
| for i := range allChildren { | |
| child := &allChildren[i] |
| if child.ID != "" { | ||
| label = fmt.Sprintf("%s (ID: %s)", child.Name, child.ID) | ||
| } | ||
| boardOptions = append(boardOptions, label) | ||
| boardMap[label] = child |
There was a problem hiding this comment.
boardMap is keyed by the rendered label string. If child.ID is empty (or if two boards somehow render to the same label), later entries will overwrite earlier ones and selection may map to the wrong board. Consider keying the map by a stable unique value (e.g., child.ID) and keeping a separate label→ID mapping (or include a disambiguating suffix when IDs are missing).
| var boardID uint64 | ||
| if _, err := fmt.Sscanf(child.ID, "%d", &boardID); err != nil { |
There was a problem hiding this comment.
Parsing boardID via fmt.Sscanf(child.ID, "%d", &boardID) is relatively brittle and less explicit than using strconv.ParseUint(child.ID, 10, 64) (which also gives clearer errors and avoids format-string parsing). Consider switching to ParseUint and handling empty IDs explicitly.
| var boardID uint64 | |
| if _, err := fmt.Sscanf(child.ID, "%d", &boardID); err != nil { | |
| if child.ID == "" { | |
| fmt.Printf(" ⚠️ Skipping board %q: empty ID\n", child.Name) | |
| continue | |
| } | |
| boardID, err := strconv.ParseUint(child.ID, 10, 64) | |
| if err != nil { |
|
@claude[agent] Please address these code review comments:
|
All three items addressed in commit 0317238:
|
| for nextToken != "" { | ||
| page, err := client.ListRemoteScopes("jira", connID, "", nextToken) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list Jira boards (page token %s): %w", nextToken, err) |
There was a problem hiding this comment.
This error path includes the raw nextToken value in the returned error. Pagination tokens can be long/opaque and may contain sensitive or internal data; echoing them can leak into logs and makes errors noisy. Consider omitting the token entirely or only logging a truncated/quoted form (e.g., %q with a length cap).
| return nil, fmt.Errorf("failed to list Jira boards (page token %s): %w", nextToken, err) | |
| return nil, fmt.Errorf("failed to list Jira boards during pagination: %w", err) |
| if child.Type == "scope" { | ||
| label := child.Name | ||
| if child.ID != "" { | ||
| label = fmt.Sprintf("%s (ID: %s)", child.Name, child.ID) | ||
| } | ||
| boardOptions = append(boardOptions, label) | ||
| boardMap[label] = child | ||
| } |
There was a problem hiding this comment.
Boards with empty/invalid IDs are still shown in the selection list, but are later skipped during scope creation, which can lead to a confusing flow where the user selects items and then hits "no valid boards to add". Filter out children with missing/invalid IDs before building boardOptions (or validate the selection and reprompt) so only addable boards are presented.
| if child.Type == "scope" { | |
| label := child.Name | |
| if child.ID != "" { | |
| label = fmt.Sprintf("%s (ID: %s)", child.Name, child.ID) | |
| } | |
| boardOptions = append(boardOptions, label) | |
| boardMap[label] = child | |
| } | |
| if child.Type != "scope" { | |
| continue | |
| } | |
| // Only expose boards with non-empty, valid numeric IDs so that | |
| // any selection the user makes can be added as a scope. | |
| if child.ID == "" { | |
| fmt.Printf(" ⚠️ Skipping board %q in menu: empty ID\n", child.Name) | |
| continue | |
| } | |
| if _, err := strconv.ParseUint(child.ID, 10, 64); err != nil { | |
| fmt.Printf(" ⚠️ Skipping board %q in menu: invalid ID %q\n", child.Name, child.ID) | |
| continue | |
| } | |
| label := fmt.Sprintf("%s (ID: %s)", child.Name, child.ID) | |
| boardOptions = append(boardOptions, label) | |
| boardMap[label] = child |
|
@claude[agent] Please fix these items:
|
Both items addressed in commit 30842f9:
|
- Add Jira ConnectionDef entry to connectionRegistry with AccessToken auth - Implement scopeJiraHandler using ListRemoteScopes API to fetch boards - Add JiraBoardScope type for board scope payloads - Add comprehensive test coverage for Jira plugin configuration - All tests pass (go test ./..., go vet ./...) Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
- Remove misleading RequiredScopes and ScopeHint (Jira API tokens don't use OAuth scopes) - Add pagination support for ListRemoteScopes to fetch all boards across pages - Replace fmt.Sscanf with strconv.ParseUint for clearer error handling - Handle empty board IDs explicitly - Update tests to allow empty scopes for API token-based plugins Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
- Add Jira row to Supported Plugins table in README.md - Simplify TestAvailablePluginsScopeHints to check RequiredScopes length instead of maintaining plugin-specific map - More generic approach handles both Jira and any future API token-based plugins Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
ewega
force-pushed
the
claude/add-jira-support-cli
branch
from
30842f9 to
68e5522
Compare
Original prompt