Conversation
Look up a user by their Jira account ID: jtk users get 61292e4c4f29230069621c5f Returns display name, email, account ID, and active status. The API method already existed; this adds the CLI command. Closes #176
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: d91066d
Approved with 2 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| security:code-auditor | 2 |
security:code-auditor (2 findings)
💡 Suggestion - tools/jtk/internal/cmd/users/users_test.go:109
TestRunGet_InactiveUser only asserts
Contains(stdout, "no"), which is fragile — it would match any occurrence of "no" in the output (e.g., a display name like "Arnold Nolan"). Consider asserting a more specific substring like"Active: no"to avoid false positives. The existing search tests at line 245 have the same weakness, but this PR has the opportunity to set a better pattern.
💡 Suggestion - tools/jtk/internal/cmd/users/users_test.go:17
newTestUserServer ignores the HTTP request entirely (path, query params, method). The test verifies display formatting but does not confirm that runGet passes the correct account ID to the API. A minimal check on the request URL (e.g., asserting
r.URL.Query().Get("accountId") == expectedID) would catch regressions in argument propagation.
Completed in 9m 41s | $1.27
| Field | Value |
|---|---|
| Reviewers | hybrid-synthesis, database:sql-reviewer, security:code-auditor, harness-engineering:knowledge-reviewer, harness-engineering:enforcement-reviewer, harness-engineering:architecture-reviewer, harness-engineering:legibility-reviewer |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 9m 41s (Reviewers: 3m 03s · Synthesis: 1m 13s) |
| Cost | $1.27 |
| Tokens | 971.5k in / 34.6k out |
| Turns | 73 |
| err = runGet(context.Background(), opts, "abc123") | ||
| testutil.RequireNoError(t, err) | ||
|
|
||
| testutil.Contains(t, stdout.String(), "no") |
There was a problem hiding this comment.
🔵 Low (security:code-auditor): TestRunGet_InactiveUser only asserts Contains(stdout, "no"), which is fragile — it would match any occurrence of "no" in the output (e.g., a display name like "Arnold Nolan"). Consider asserting a more specific substring like "Active: no" to avoid false positives. The existing search tests at line 245 have the same weakness, but this PR has the opportunity to set a better pattern.
| "github.com/open-cli-collective/jira-ticket-cli/internal/cmd/root" | ||
| ) | ||
|
|
||
| func newTestUserServer(_ *testing.T, user api.User) *httptest.Server { |
There was a problem hiding this comment.
🔵 Low (security:code-auditor): newTestUserServer ignores the HTTP request entirely (path, query params, method). The test verifies display formatting but does not confirm that runGet passes the correct account ID to the API. A minimal check on the request URL (e.g., asserting r.URL.Query().Get("accountId") == expectedID) would catch regressions in argument propagation.
Summary
jtk users get <account-id>to look up a user by account ID-o jsonapi.GetUser()method already existed; this adds the CLI commandCloses #176
Test plan
make build && make testpasses