feat: add Parseable Cloud API key login#110
Conversation
📝 WalkthroughWalkthroughAdds Parseable Cloud device authorization and API-key profile management, expands profile authentication modes, centralizes authenticated requests, updates PromQL commands, and adds structured output support across root, profile, status, logout, and tail commands. ChangesCloud authentication and profile management
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/login.go (1)
60-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated cloud-profile construction.
cloudProfileFromAPIKeybuilds aconfig.Profilefrom the validation result identically toCloudProfileAddCmd.RunEincmd/cloud.go(lines 96-105). Consider extracting a single helper (e.g.profileFromValidation(apiKey, orchestratorURL, result)) used by both paths so the field mapping stays in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/login.go` around lines 60 - 77, The cloud-profile construction logic is duplicated between cloudProfileFromAPIKey and CloudProfileAddCmd.RunE, so extract the shared mapping into a single helper such as profileFromValidation that accepts the apiKey, orchestratorURL, and validation result and returns the config.Profile. Update both call sites to use that helper so the field assignments stay consistent in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/cloud.go`:
- Around line 184-200: saveCloudProfile currently treats every
ReadConfigFromFile error as an empty config and then writes it back, which can
wipe existing profiles, and it also overwrites an existing profile when the
derived or provided name already exists. Update saveCloudProfile to distinguish
a missing config file from other read errors so non-fatal parse/read failures
are surfaced instead of replacing the config, and add an overwrite check before
assigning fileConfig.Profiles[name]. Use the existing stepConfirmReplace pattern
from the interactive login flow to warn or confirm when a profile name collision
would replace an existing entry, then only call WriteConfigToFile after the user
has explicitly accepted the overwrite.
- Around line 33-37: The default Cloud orchestrator URL is currently pointing at
the staging endpoint, so `defaultCloudOrchestratorURL` in `cmd/cloud.go` should
be changed to the production orchestrator unless staging is explicitly intended.
Update the default used by the Cloud validation flow, and make sure the
`envCloudOrchestratorURL` override still works so users can opt into a different
endpoint if needed.
In `@cmd/tail.go`:
- Around line 120-132: The tailAuthMetadata helper is missing bearer-token
handling and currently falls back to Basic auth for token-only profiles. Update
tailAuthMetadata to mirror the same auth precedence used by AddAuthHeaders:
handle profile.Token before the Basic fallback, returning an Authorization
header with Bearer <token> when a token is present, while keeping the existing
cloud x-api-key and tenant behavior unchanged.
In `@pkg/config/config.go`:
- Line 101: The config write path in the file-opening logic does not harden
permissions for already-existing files, so broader modes can persist after
writing sensitive values like api_key. Update the config save flow around
os.OpenFile in the config write function to explicitly set restrictive
permissions on the opened file after it is created/opened, using
file.Chmod(0600) or equivalent, so both new and existing config files end up
with 0600.
---
Nitpick comments:
In `@cmd/login.go`:
- Around line 60-77: The cloud-profile construction logic is duplicated between
cloudProfileFromAPIKey and CloudProfileAddCmd.RunE, so extract the shared
mapping into a single helper such as profileFromValidation that accepts the
apiKey, orchestratorURL, and validation result and returns the config.Profile.
Update both call sites to use that helper so the field assignments stay
consistent in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19834b75-b4c7-47e5-8e5b-cdc99993cf39
📒 Files selected for processing (13)
cmd/cloud.gocmd/login.gocmd/promql.gocmd/queryList.gocmd/tail.gomain.gopkg/config/config.gopkg/datasets/datasets.gopkg/http/http.gopkg/model/login/login.gopkg/model/promql.gopkg/model/query.gopkg/model/savedQueries.go
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/model/login/login.go (1)
579-593: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender the actual Cloud authentication mode.
OAuth browser profiles are currently labeled as Cloud API-key profiles, while Cloud API-key profiles render two
AUTHrows.Proposed fix
if m.Profile.Cloud { + auth := "Cloud API key (stored after validation)" + if m.CloudBrowserLogin { + auth = "Cloud OAuth (browser)" + } b.WriteString(" " + labelStyle.Render("AUTH ")) - b.WriteString(normalStyle.Render("Cloud API key (stored after validation)")) + b.WriteString(normalStyle.Render(auth)) b.WriteString("\n") } - if m.Profile.APIKey != "" { + if !m.Profile.Cloud && m.Profile.APIKey != "" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/model/login/login.go` around lines 579 - 593, Update the profile rendering logic around the Cloud and APIKey checks to distinguish OAuth browser profiles from Cloud API-key profiles: render the Cloud authentication label according to the actual authentication mode, and avoid emitting a second AUTH row for Cloud API-key profiles. Preserve the existing username rendering and stored-key messaging for non-Cloud profiles.cmd/queryList.go (1)
53-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate saved-query request failures.
When
NewRequestrejects an invalid profile,fetchFiltersprints the error to stdout and returns nil. JSON mode then emits[]and exits successfully, producing invalid mixed output and masking authentication failures.Change
fetchFiltersto return([]Item, error)and letRunEreturn the error without printing inside the data layer.Also applies to: 181-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/queryList.go` around lines 53 - 69, The fetchFilters data path currently suppresses request errors, causing JSON mode to output an empty array and succeed. Update fetchFilters to return ([]Item, error), propagate NewRequest failures without printing, and update RunE and all callers to handle the returned error by returning it before formatting output; preserve successful JSON and non-JSON behavior.
🧹 Nitpick comments (3)
cmd/dataset_test.go (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the authentication header in at least one handler.
Supplying
APIKeyonly satisfiesAuthMode; these tests still pass ifx-api-keyis never sent.Proposed assertion
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("x-api-key"); got != "test-api-key" { + t.Fatalf("x-api-key mismatch: got %q", got) + }Also applies to: 66-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/dataset_test.go` at line 22, Update the handlers tested in cmd/dataset_test.go to assert that requests include the expected x-api-key authentication header, using the configured "test-api-key" value. Add this assertion in at least one handler, including the additionally referenced handler, while preserving the existing test behavior.cmd/promql.go (2)
384-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated flag/positional-arg override boilerplate across ~7 subcommands.
The same pattern (
val, _ := cmd.Flags().GetString(...); if len(args) > idx { val = args[idx] }) is duplicated inpromqlLabelsCmd,promqlLabelValuesCmd,promqlSeriesCmd,promqlCardinalityLabelNamesCmd,promqlCardinalityLabelValuesCmd,promqlCardinalityActiveSeriesCmd, andpromqlTSDBCmd. Consider extracting a small helper to centralize this.♻️ Proposed helper and example usage
// argOrFlag returns args[idx] if present, otherwise the current value of the given string flag. func argOrFlag(cmd *cobra.Command, args []string, idx int, flagName string) string { val, _ := cmd.Flags().GetString(flagName) if len(args) > idx { return args[idx] } return val }- stream, _ := cmd.Flags().GetString("dataset") - if len(args) > 0 { - stream = args[0] - } + stream := argOrFlag(cmd, args, 0, "dataset")Apply the same substitution at the other six call sites.
Also applies to: 438-448, 494-503, 569-578, 620-633, 717-732, 843-852
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/promql.go` around lines 384 - 393, The positional-argument-overrides-flag logic is duplicated across the PromQL subcommands. Add an argOrFlag helper near the command implementations, then update promqlLabelsCmd and the corresponding promqlLabelValuesCmd, promqlSeriesCmd, promqlCardinalityLabelNamesCmd, promqlCardinalityLabelValuesCmd, promqlCardinalityActiveSeriesCmd, and promqlTSDBCmd call sites to use it with the appropriate argument index and flag name.
620-633: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPositional order for
label-valuesis reversed between sibling commands.
promql label-valuestakes[label_name] [stream](Line 438), whilepromql cardinality label-valuestakes[stream] [label](Line 620) — same subcommand name, swapped order, no type validation on either positional. A user transposing the args between the two contexts would silently query the wrong stream/label instead of getting an error.Worth a deliberate decision (and doc note) on whether to align ordering with the top-level command or keep it consistent with the other
cardinalitysubcommands (which are all stream-first).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/promql.go` around lines 620 - 633, Deliberately resolve the positional argument order mismatch between the top-level label-values command and the cardinality label-values command. Update the cardinality label-values definition, including its Use, Example, and RunE argument mapping around stream and labelName, to follow the chosen ordering consistently; document the supported order and preserve compatibility expectations where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/cloud.go`:
- Around line 216-218: Update the direct exchange call in
cloudProfileFromDirectCodeExchange’s caller to pass the explicit clerk session
token from the --clerk-session-token option instead of the browser token
currently supplied twice. Apply the same token-source correction to the
corresponding direct-exchange paths at the other referenced locations,
preserving browser-token behavior only when no explicit flag is provided.
- Line 41: Update cloudDefaultOrchestratorAuthToken to an empty value so the
access-token fallback is used instead of sending the placeholder bearer token.
Preserve the existing fallback behavior in the default browser login and
interactive pb login Cloud flow, allowing organization resolution without an
explicit token override.
- Around line 858-864: Update cloudClerkScriptURL to replace the moving `@latest`
Clerk SDK tag with one fixed, tested version in both the CDN fallback URL and
the host-based URL, keeping the existing path selection behavior unchanged.
In `@cmd/logout.go`:
- Around line 61-64: Ensure the logout flow remains non-interactive when --yes
is set: before calling selectLogoutProfile, detect the
no-default/multiple-profile case and return an ambiguity error or require an
explicit profile. Preserve normal selector behavior for interactive invocations
and update the existing condition around outputFormat accordingly.
In `@cmd/promql.go`:
- Around line 384-393: Add tests in cmd/promql_test.go covering optional
positional-argument parsing for promqlLabelsCmd, promqlLabelValuesCmd,
promqlSeriesCmd, promqlCardinalityLabelValuesCmd,
promqlCardinalityActiveSeriesCmd, and promqlTSDBCmd, verifying each command uses
the positional argument when provided and preserves the dataset flag/default
behavior otherwise.
In `@cmd/status.go`:
- Around line 35-39: Update the status command’s RunE flow to route preflight
failures such as missing configuration or an active profile through statusOutput
when the output format is JSON, while still returning an error so the command
exits nonzero. Preserve the existing human-readable error behavior for other
formats and use the established statusOutput rendering path.
In `@cmd/tail.go`:
- Around line 154-161: Update the Flight client credential setup in the tail
command to use TLS for config.AuthCloudAPIKey and config.AuthCloudOAuth
profiles, ensuring API keys and session cookies are transmitted over encrypted
gRPC. Preserve insecure.NewCredentials() only for the intentional self-hosted
path, and keep the existing metadata construction unchanged.
In `@pkg/config/config.go`:
- Around line 85-137: The authentication model currently conflates migrated
legacy token values with API keys. Update Profile, AuthMode, and AddAuthHeaders
to retain a distinct legacy token field and authentication mode, classify token
profiles separately from API-key profiles, and continue emitting Authorization:
Bearer for them. Preserve existing cloud, basic, and API-key validation behavior
while rejecting ambiguous combinations consistently.
In `@pkg/datasets/datasets.go`:
- Around line 116-123: Update the dataset-building loop around fetchDatasetType
to avoid serial HTTP requests for every name. Fetch dataset types with bounded
concurrency, ensuring all results are safely associated with their corresponding
Dataset and the client’s request limits are respected, while preserving the
existing behavior when a type lookup fails.
In `@pkg/http/http.go`:
- Around line 61-62: Update NewRequest to validate client.Profile before
constructing the request URL or invoking baseAPIURL, returning the existing
“profile is nil” error through the normal error path. Ensure
DefaultClient(nil).NewRequest(...) does not dereference the nil profile, while
preserving the existing AddAuthHeaders behavior for valid profiles.
In `@pkg/model/login/login.go`:
- Line 110: Configure apiKeyInput to use masked/no-echo input, matching the
existing password input behavior, while preserving its prompt and length limit.
Update the newInput invocation or input configuration associated with
apiKeyInput so pasted API keys are not displayed during entry.
---
Outside diff comments:
In `@cmd/queryList.go`:
- Around line 53-69: The fetchFilters data path currently suppresses request
errors, causing JSON mode to output an empty array and succeed. Update
fetchFilters to return ([]Item, error), propagate NewRequest failures without
printing, and update RunE and all callers to handle the returned error by
returning it before formatting output; preserve successful JSON and non-JSON
behavior.
In `@pkg/model/login/login.go`:
- Around line 579-593: Update the profile rendering logic around the Cloud and
APIKey checks to distinguish OAuth browser profiles from Cloud API-key profiles:
render the Cloud authentication label according to the actual authentication
mode, and avoid emitting a second AUTH row for Cloud API-key profiles. Preserve
the existing username rendering and stored-key messaging for non-Cloud profiles.
---
Nitpick comments:
In `@cmd/dataset_test.go`:
- Line 22: Update the handlers tested in cmd/dataset_test.go to assert that
requests include the expected x-api-key authentication header, using the
configured "test-api-key" value. Add this assertion in at least one handler,
including the additionally referenced handler, while preserving the existing
test behavior.
In `@cmd/promql.go`:
- Around line 384-393: The positional-argument-overrides-flag logic is
duplicated across the PromQL subcommands. Add an argOrFlag helper near the
command implementations, then update promqlLabelsCmd and the corresponding
promqlLabelValuesCmd, promqlSeriesCmd, promqlCardinalityLabelNamesCmd,
promqlCardinalityLabelValuesCmd, promqlCardinalityActiveSeriesCmd, and
promqlTSDBCmd call sites to use it with the appropriate argument index and flag
name.
- Around line 620-633: Deliberately resolve the positional argument order
mismatch between the top-level label-values command and the cardinality
label-values command. Update the cardinality label-values definition, including
its Use, Example, and RunE argument mapping around stream and labelName, to
follow the chosen ordering consistently; document the supported order and
preserve compatibility expectations where applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf1c6043-814d-4416-9f93-e62991edae58
📒 Files selected for processing (17)
cmd/cloud.gocmd/dataset_test.gocmd/login.gocmd/logout.gocmd/profile.gocmd/promql.gocmd/queryList.gocmd/status.gocmd/tail.gomain.gopkg/config/config.gopkg/datasets/datasets.gopkg/http/http.gopkg/model/login/login.gopkg/model/promql.gopkg/model/query.gopkg/model/savedQueries.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/login.go
- pkg/model/promql.go
| func cloudClerkScriptURL(publishableKey string) string { | ||
| host := cloudClerkFrontendHost(publishableKey) | ||
| if host == "" { | ||
| return "https://cdn.jsdelivr.net/npm/@clerk/clerk-js@latest/dist/clerk.browser.js" | ||
| } | ||
| return "https://" + host + "/npm/@clerk/clerk-js@latest/dist/clerk.browser.js" | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What versioned URL format does Clerk recommend for loading clerk-js, and which stable version supports mountSignIn, handleRedirectCallback, and session.getToken?
💡 Result:
Clerk recommends loading clerk-js using a versioned URL format that specifies the exact version to ensure stability and predictability. Typically, this is achieved by pinning the version in your import path, for example via a CDN like jsDelivr: https://cdn.jsdelivr.net/npm/@clerk/clerk-js@/dist/clerk.browser.js [1][2]. Regarding the requested methods mountSignIn, handleRedirectCallback, and session.getToken: 1. mountSignIn: This method has been a stable part of the Clerk class API for rendering the SignIn component and is available in current versions of @clerk/clerk-js, including the v6.x series [3][1]. 2. handleRedirectCallback: This method is used to complete authentication flows (such as OAuth or SAML) and is available in current v6.x versions [1][4]. 3. session.getToken: The getToken() method on the Session object is a core, stable feature of the Clerk SDK used for retrieving session tokens or JWT templates and is available in current v6.x versions [5][6]. As of July 2026, these methods are supported in all stable versions within the @clerk/clerk-js v6.x release line [1][2]. It is recommended to use the latest stable version (e.g., v6.7.7 or higher) to ensure access to the most recent features and security updates [2]. [7].
Citations:
- 1: https://cdn.jsdelivr.net/npm/@clerk/clerk-js@6.4.0/dist/types/core/clerk.d.ts
- 2: https://registry.npmjs.org/@clerk/clerk-js
- 3: https://clerk.com/docs/js-frontend/reference/components/authentication/sign-in
- 4: https://clerk.com/docs/nuxt/reference/objects/clerk
- 5: https://clerk.com/docs/js-frontend/reference/objects/session
- 6: https://clerk.com/docs/guides/development/upgrading/upgrading-from-v2-to-v3
- 7: https://clerk.com/docs/guides/development/upgrading/versioning
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and find usages of the Clerk script URL helper/version literals.
git ls-files cmd/cloud.go
printf '\n--- outline ---\n'
ast-grep outline cmd/cloud.go --view expanded || true
printf '\n--- occurrences ---\n'
rg -n 'cloudClerkScriptURL|clerk-js@latest|clerk.browser.js|cloudClerkFrontendHost|session.getToken|handleRedirectCallback|mountSignIn' cmd/cloud.go .Repository: parseablehq/pb
Length of output: 7232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '707,845p' cmd/cloud.goRepository: parseablehq/pb
Length of output: 5164
Pin the Clerk JavaScript version on both URL paths.
These pages load the SDK in the sign-in and redirect flow, so @latest can introduce breaking behavior in login or token handling. Use one tested version instead of the moving tag.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/cloud.go` around lines 858 - 864, Update cloudClerkScriptURL to replace
the moving `@latest` Clerk SDK tag with one fixed, tested version in both the CDN
fallback URL and the host-based URL, keeping the existing path selection
behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config.go (1)
183-194: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftWrite the config atomically instead of truncating it in place.
A chmod failure after Line 183 already leaves the existing config empty, while a short write can leave partial TOML. Write a
0600temporary file in the same directory, close/sync it, then atomically rename it over the destination.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/config.go` around lines 183 - 194, Update the config-writing flow around os.OpenFile and file.Write to use a 0600 temporary file in the destination directory, write the complete TOML, handle short writes, sync and close it, then atomically rename it over filePath. Apply permissions before replacing the destination, and remove the in-place truncation path while cleaning up the temporary file on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/cloud.go`:
- Line 253: Restore optional default-profile handling: in cmd/cloud.go at lines
253-253, bind the cloud profile add command’s --default boolean and pass its
value to saveCloudProfile instead of hard-coding true; in
cmd/cloud_profile_test.go at lines 9-15, replace the absence assertion with
coverage for both preserving the existing default without the flag and updating
it when --default is provided.
- Around line 401-408: Update the device polling switch on oauthErr.Error to
handle "slow_down" by increasing interval by 5 seconds and continuing the
polling loop, while preserving the existing authorization_pending,
expired_token, and default failure behavior.
- Around line 68-72: Add a client ID field to cloudDeviceTokenRequest,
serialized as client_id, and populate it with the existing pb-cli client ID
wherever the device token exchange request is constructed. Keep the value
consistent with the client ID used during device-code issuance.
In `@cmd/profile.go`:
- Line 185: Update the profile-add flow around the fileConfig.DefaultProfile
assignment so adding a profile only sets DefaultProfile when no current default
exists. Preserve the existing active default for secondary profiles, and retain
explicit --default handling separately.
In `@cmd/promql_test.go`:
- Around line 85-96: Update the cardinality label-values and active-series test
cases for promqlCardinalityLabelValuesCmd and promqlCardinalityActiveSeriesCmd
to set conflicting --label and active-series selector flag values through
flagSetup, while keeping the expected query values sourced from the positional
arguments.
In `@pkg/analytics/analytics.go`:
- Line 157: Update the analytics delivery error handling around the existing
fmt.Println call to write the diagnostic to stderr instead of stdout, preserving
the current error message and behavior while keeping command JSON output clean.
In `@pkg/http/refresh.go`:
- Around line 43-45: Update the refresh flow around transport.refresh in the
request orchestrator to close the original response and return a wrapped error
when refresh or token persistence fails, rather than returning the response with
a nil error. Preserve the successful refresh path and include the underlying
failure for diagnosis.
---
Outside diff comments:
In `@pkg/config/config.go`:
- Around line 183-194: Update the config-writing flow around os.OpenFile and
file.Write to use a 0600 temporary file in the destination directory, write the
complete TOML, handle short writes, sync and close it, then atomically rename it
over filePath. Apply permissions before replacing the destination, and remove
the in-place truncation path while cleaning up the temporary file on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2cf1f6b-de56-48ef-9b2b-72b4c20fc256
📒 Files selected for processing (23)
cmd/cloud.gocmd/cloud_profile_test.gocmd/login.gocmd/logout.gocmd/logout_test.gocmd/profile.gocmd/promql.gocmd/promql_test.gocmd/status.gocmd/status_test.gocmd/tail.gocmd/tail_test.gopkg/analytics/analytics.gopkg/analytics/analytics_test.gopkg/config/config.gopkg/datasets/datasets.gopkg/datasets/datasets_test.gopkg/http/http.gopkg/http/http_test.gopkg/http/refresh.gopkg/model/login/login.gopkg/model/login/login_test.gopkg/model/promql.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/model/promql.go
- cmd/logout.go
- cmd/status.go
- pkg/model/login/login.go
- pkg/http/http.go
- cmd/promql.go
| type cloudDeviceTokenRequest struct { | ||
| GrantType string `json:"grant_type"` | ||
| DeviceCode string `json:"device_code,omitempty"` | ||
| RefreshToken string `json:"refresh_token,omitempty"` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and surrounding call sites.
git ls-files cmd/cloud.go
wc -l cmd/cloud.go
sed -n '1,220p' cmd/cloud.go
printf '\n--- Search for cloudDeviceTokenRequest and device code payloads ---\n'
rg -n "cloudDeviceTokenRequest|device_code|client_id|slow_down|grant_type" cmd/cloud.go .Repository: parseablehq/pb
Length of output: 7693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' cmd/cloud.go
printf '\n---\n'
rg -n "cloudDeviceTokenRequest|client_id|slow_down|device_code|grant_type" cmd/cloud.go .Repository: parseablehq/pb
Length of output: 7599
Include client_id in the device token request. The token exchange is sent without client authentication, so this public client still needs to identify itself with the same pb-cli client ID used for device-code issuance; otherwise the device-login flow can fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/cloud.go` around lines 68 - 72, Add a client ID field to
cloudDeviceTokenRequest, serialized as client_id, and populate it with the
existing pb-cli client ID wherever the device token exchange request is
constructed. Keep the value consistent with the client ID used during
device-code issuance.
| WorkspaceName: result.WorkspaceName, | ||
| OrchestratorURL: orchestratorURL, | ||
| } | ||
| if err := saveCloudProfile(profileName, profile, cloudForceOverwrite, true); err != nil { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the optional --default profile behavior.
The PR contract advertises pb cloud profile add ... --default, but the command exposes no such flag and instead always changes the active profile.
cmd/cloud.go#L253-L253: bind a--defaultboolean and pass its value tosaveCloudProfilerather than hard-codingtrue.cmd/cloud_profile_test.go#L9-L15: replace the absence assertion with tests verifying both default-preserving and--defaultbehavior.
📍 Affects 2 files
cmd/cloud.go#L253-L253(this comment)cmd/cloud_profile_test.go#L9-L15
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/cloud.go` at line 253, Restore optional default-profile handling: in
cmd/cloud.go at lines 253-253, bind the cloud profile add command’s --default
boolean and pass its value to saveCloudProfile instead of hard-coding true; in
cmd/cloud_profile_test.go at lines 9-15, replace the absence assertion with
coverage for both preserving the existing default without the flag and updating
it when --default is provided.
| switch oauthErr.Error { | ||
| case "authorization_pending": | ||
| continue | ||
| case "expired_token": | ||
| return nil, errors.New("cloud device code expired; run login again") | ||
| default: | ||
| return nil, fmt.Errorf("cloud device login failed: %s", cloudOAuthErrorMessage(oauthErr)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the polling loop around the cited lines.
ast-grep outline cmd/cloud.go --view expanded || true
sed -n '340,450p' cmd/cloud.go
# Search for device-flow error handling and interval updates elsewhere in the repo.
rg -n 'slow_down|authorization_pending|expired_token|interval \+=' cmd cloud.go . || trueRepository: parseablehq/pb
Length of output: 8614
Handle slow_down in device polling. Increase interval by 5s and continue; otherwise a recoverable RFC 8628 backoff response aborts login.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/cloud.go` around lines 401 - 408, Update the device polling switch on
oauthErr.Error to handle "slow_down" by increasing interval by 5 seconds and
continuing the polling loop, while preserving the existing
authorization_pending, expired_token, and default failure behavior.
| if fileConfig.DefaultProfile == "" { | ||
| fileConfig.DefaultProfile = name | ||
| } | ||
| fileConfig.DefaultProfile = name |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not switch the active profile on every add.
Adding a secondary profile now unconditionally replaces DefaultProfile, redirecting subsequent commands without an explicit --default request. Preserve the current default unless none exists.
Proposed fix
fileConfig.Profiles[name] = profile
- fileConfig.DefaultProfile = name
+ if fileConfig.DefaultProfile == "" {
+ fileConfig.DefaultProfile = name
+ }
commandError = config.WriteConfigToFile(fileConfig)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fileConfig.DefaultProfile = name | |
| fileConfig.Profiles[name] = profile | |
| if fileConfig.DefaultProfile == "" { | |
| fileConfig.DefaultProfile = name | |
| } | |
| commandError = config.WriteConfigToFile(fileConfig) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/profile.go` at line 185, Update the profile-add flow around the
fileConfig.DefaultProfile assignment so adding a profile only sets
DefaultProfile when no current default exists. Preserve the existing active
default for secondary profiles, and retain explicit --default handling
separately.
| name: "cardinality label values stream and label", | ||
| cmd: promqlCardinalityLabelValuesCmd, | ||
| args: []string{"positional-cardinality", "service.name"}, | ||
| wantPath: "/prometheus/api/v1/cardinality/label_values", | ||
| wantQuery: map[string]string{"stream": "positional-cardinality", "label_name": "service.name"}, | ||
| }, | ||
| { | ||
| name: "active series stream and selector", | ||
| cmd: promqlCardinalityActiveSeriesCmd, | ||
| args: []string{"positional-active", `{job="api"}`}, | ||
| wantPath: "/prometheus/api/v1/cardinality/active_series", | ||
| wantQuery: map[string]string{"stream": "positional-active", "selector": `{job="api"}`}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set conflicting label and selector flags before asserting positional overrides.
These cases never set --label or the active-series selector flag, so they pass even if positional values do not override those flags. Add conflicting values via flagSetup and retain the current expected positional query values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/promql_test.go` around lines 85 - 96, Update the cardinality label-values
and active-series test cases for promqlCardinalityLabelValuesCmd and
promqlCardinalityActiveSeriesCmd to set conflicting --label and active-series
selector flag values through flagSetup, while keeping the expected query values
sourced from the positional arguments.
| // contain queries, passwords, API keys, session tokens, or server details. | ||
| err := sendEvent(name, executionTime) | ||
| if err != nil { | ||
| fmt.Println("Error sending analytics event:", err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep analytics failures off structured stdout.
When analytics delivery fails, this line appends plain text to command JSON output. Write the diagnostic to stderr instead.
Proposed fix
- fmt.Println("Error sending analytics event:", err)
+ fmt.Fprintln(os.Stderr, "Error sending analytics event:", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Println("Error sending analytics event:", err) | |
| fmt.Fprintln(os.Stderr, "Error sending analytics event:", err) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/analytics/analytics.go` at line 157, Update the analytics delivery error
handling around the existing fmt.Println call to write the diagnostic to stderr
instead of stdout, preserving the current error message and behavior while
keeping command JSON output clean.
| if err := transport.refresh(req); err != nil { | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not hide cloud refresh or token-persistence failures as an ordinary 401.
The orchestrator may have rotated the refresh token before UpdateCloudTokens fails. Returning the original response with a nil error discards that diagnosis and can leave the profile permanently using the invalid old token. Close the original response and propagate a wrapped refresh error.
Proposed fix
if err := transport.refresh(req); err != nil {
- return resp, nil
+ resp.Body.Close()
+ return nil, fmt.Errorf("failed to refresh cloud session: %w", err)
}Also applies to: 122-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/http/refresh.go` around lines 43 - 45, Update the refresh flow around
transport.refresh in the request orchestrator to close the original response and
return a wrapped error when refresh or token persistence fails, rather than
returning the response with a nil error. Preserve the successful refresh path
and include the underlying failure for diagnosis.
1 participant
Summary
Adds Parseable Cloud API key support to
pb.Users can now create a Cloud profile using either:
or interactive login:
Then select Parseable Cloud and paste API key.
What Changed
Added pb cloud profile add
Enabled Parseable Cloud option in interactive pb login
Added cloud-aware profile fields:
Added API key validation through orchestrator:
Added cloud request auth headers:
Kept self-hosted BasicAuth/token profiles backward compatible
Config file writes with 0600 permissions because API key is stored locally
Summary by CodeRabbit
--output/-o(text/json) and JSON help; extended JSON output tostatus,logout, andtail.tailnow supports--output text|json, and several PromQL commands accept optional positional arguments to override flag-based inputs.