Skip to content

fix(cube-cli): version from lerna.json, "Cube CLI" branding, sk- API-key detection#11311

Merged
paveltiunov merged 6 commits into
masterfrom
claude/cube-rust-cli-tool-oiv9br
Jul 21, 2026
Merged

fix(cube-cli): version from lerna.json, "Cube CLI" branding, sk- API-key detection#11311
paveltiunov merged 6 commits into
masterfrom
claude/cube-rust-cli-tool-oiv9br

Conversation

@paveltiunov

@paveltiunov paveltiunov commented Jul 21, 2026

Copy link
Copy Markdown
Member

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made

Cube CLI fixes, follow-up to #11291:

Version was stale outside release builds. The release version-bump commit updates lerna.json but not rust/cube-cli/Cargo.toml, so source builds reported the old version (1.7.2 on a 1.7.5 tree). Now build.rs reads the repo-root lerna.json and injects CUBE_CLI_VERSION at compile time — source builds and release builds both report the real Cube version with no manifest syncing. The Cargo.toml version becomes a 0.0.0 placeholder used only as a fallback for out-of-tree builds, and the release workflow's sed step is replaced by a tag-vs-lerna.json consistency check (fails the release job on drift). All version references (--version, User-Agent, update check, telemetry cliVersion) use the injected version.

Branding. cube --version now prints Cube CLI 1.7.5 (clap display_name) instead of cube 1.7.5; the update notice and cube update output say "Cube CLI" too. Version propagation to subcommands is dropped — it rendered as Cube CLI-deployments 1.7.5.

Device login was broken by credential-scheme detection. The Bearer/Api-Key heuristic from #11291 classified credentials by JWT shape, but OAuth access tokens aren't necessarily three-segment JWTs — freshly minted device-flow tokens were sent as Api-Key and the post-login /users/me validation failed with 401. Cube Cloud API keys carry an sk- prefix, so that's now the discriminator: sk-Api-Key, everything else → Bearer (CUBE_AUTH_SCHEME still overrides). Verified end-to-end on staging with both credential types.

(A tenant-discovery login fallback via generic cubecloud.dev sign-in was prototyped and reverted within this branch — there's no server to mint device codes on the generic host.)

🤖 Generated with Claude Code


Generated by Claude Code

…-version

The release version-bump commit updates lerna.json but not the crate
manifest, so source builds (and any path that skipped the workflow's sed)
reported a stale version. build.rs now reads the repo-root lerna.json and
injects CUBE_CLI_VERSION at compile time — source and release builds both
report the real Cube version, and the workflow's sed step is replaced
with a tag-vs-lerna.json consistency check. The Cargo.toml version is a
0.0.0 placeholder used only for out-of-tree builds.

--version now prints 'Cube CLI 1.7.5' (clap display_name) instead of
'cube 1.7.5'; version propagation to subcommands is dropped (it rendered
as 'Cube CLI-deployments').

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdcyqFwUF9BbWLeX4uERnk
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Jul 21, 2026
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 45s —— View job


Code Review

Verdict: LGTM — 0 blocking, 2 low-severity notes. Expand for details.
  • Read PR diff
  • Review build.rs, workflow, source changes
  • Post review

Summary

The change cleanly moves the CLI version source of truth to lerna.json via a build.rs script that injects CUBE_CLI_VERSION at compile time, replacing the release-time sed with a tag/lerna consistency check. All version consumers (--version, User-Agent, telemetry cliVersion, update check CURRENT_VERSION) migrate consistently to the new env var. display_name = "Cube CLI" + dropping propagate_version = true also fixes the odd "Cube CLI-deployments" subcommand rendering. Approach and execution look good.

Low-severity notes

  1. 0.0.0 fallback is silentrust/cube-cli/build.rs:11 falls back to CARGO_PKG_VERSION when lerna.json is missing or malformed. Since Cargo.toml is now the 0.0.0 placeholder, an in-tree build where the path resolution or JSON parse fails silently yields a cube-cli 0.0.0 binary rather than a build failure. The Cargo.toml comment says this fallback is only for out-of-tree builds, but there's no guardrail enforcing that. Consider a println!("cargo:warning=...") when the fallback is taken so accidental in-tree misconfigurations surface, or panic in build.rs if ../../lerna.json exists but fails to parse (as opposed to genuinely missing).

  2. jq assumed on the runner.github/workflows/publish.yml:812 uses jq -r .version lerna.json. jq is preinstalled on GitHub-hosted ubuntu-* and macos-* runners, so this is fine in practice; just worth noting since it's the only new tool dep in the workflow. No change needed unless self-hosted runners get involved.

Testing/Docs

README update (rust/cube-cli/README.md) accurately reflects the new flow. No unit tests are added — reasonable given this is build-plumbing; the release-workflow consistency check is the guard against drift. Local verification (cube --versionCube CLI 1.7.5) is documented in the PR description.

Nothing else surfaces on code quality, security, or performance.

• Branch: [claude/cube-rust-cli-tool-oiv9br](https://github.com/cube-js/cube/tree/claude/cube-rust-cli-tool-oiv9br)

Users don't always know their tenant URL. The login prompt now accepts
an empty answer (with a help hint) and falls back to the generic
https://cubecloud.dev sign-in, which resolves the tenant from the
signed-in account; the device token response's tenantUrl is then used
as the saved context URL and for credential validation. --url and
--api-key behavior unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdcyqFwUF9BbWLeX4uERnk
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 55s —— View job


Code Review (updated for 023b9a9)

Verdict: LGTM — 0 blocking, 1 low-severity note on the new tenant-discovery commit. Expand for details.
  • Read PR diff (both commits)
  • Review new tenant-discovery flow in login.rs / oauth.rs
  • Re-check build.rs / workflow / version wiring
  • Post review

Summary of the new commit (023b9a9)

cube login now supports an "I don't know my tenant" path: an empty URL prompt (or omitted --url) falls back to https://cubecloud.dev, and the token response's optional tenantUrl is used as the saved context URL. Wiring looks clean — TokenResponse gets a tenant_url: Option<String> with a tenantUrl serde alias, device_login returns it as a third tuple element, and the caller overwrites the local url before Client::new validates.

Prior notes on build.rs (silent 0.0.0 fallback) and jq on the workflow runner still stand from the earlier pass — both low-severity, no change required.

Low-severity notes

  1. --api-key + omitted --url silently targets cubecloud.devrust/cube-cli/src/commands/login.rs:38-54. When the user passes --api-key but no --url, the interactive prompt still fires; pressing Enter defaults url to GENERIC_URL. The tenant-discovery branch is None in the api_key arm, so the saved context ends up with url = https://cubecloud.dev and a tenant-scoped API key — the Client::new validation call will fail against the wrong host, and the error won't obviously point at "you needed to give a tenant URL." Consider either (a) requiring --url when --api-key is set, or (b) failing fast with a clear message when the resolved url is GENERIC_URL in the api-key branch. Not a blocker; just a rough edge.

  2. Trust in server-supplied tenantUrlrust/cube-cli/src/commands/login.rs:49-51. The saved context URL is taken verbatim from the OAuth token response. This is fine given the response is over TLS from the OAuth authority you just authenticated to, but worth a note: if the OAuth endpoint were ever misconfigured to echo attacker-controlled data, the CLI would save whatever URL comes back. A cheap defense is to sanity-check the scheme is https:// before assigning.

Nits (optional, no need to address)

  • README (rust/cube-cli/README.md) doesn't mention the new "omit --url for generic sign-in" UX; a one-liner in the login section would help.
  • Consider a targeted unit test for the tenant_url deserialization (both tenantUrl alias and absence) — it's the one bit of new state that would silently regress.

Testing/Docs

Local cargo test/clippy/fmt per the PR description; no new tests added. The Cargo.lock update reflects serde_json build-dep addition. display_name = "Cube CLI" + removal of propagate_version = true cleanly addresses the Cube CLI-deployments subcommand rendering noted earlier.

Nothing else surfaces on code quality, security, or performance.

• Branch: claude/cube-rust-cli-tool-oiv9br

@paveltiunov paveltiunov changed the title fix(cube-cli): version from lerna.json at build time, "Cube CLI" in --version fix(cube-cli): version from lerna.json, "Cube CLI" branding, tenant discovery in login Jul 21, 2026
…tice

The Bearer/Api-Key heuristic classified credentials by JWT shape, which
broke device login: OAuth access tokens aren't necessarily three-segment
JWTs, so freshly minted tokens were sent as Api-Key and the post-login
/users/me validation failed with 401. Cube Cloud API keys carry an sk-
prefix — use that as the discriminator: sk- → Api-Key, everything else →
Bearer (CUBE_AUTH_SCHEME still overrides).

Update-notice wording: 'A new release of Cube CLI is available' (and
Cube CLI in cube update output) instead of 'cube'.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdcyqFwUF9BbWLeX4uERnk
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 40s —— View job


Code Review (updated for ae002da)

Verdict: LGTM — 0 blocking, 1 low-severity note on the auth-scheme heuristic change.
  • Read new commit diff
  • Review sk- prefix detection + branding tweaks
  • Re-check prior notes still apply
  • Post review

Summary of the new commit (ae002da)

Two changes:

  1. Auth-scheme detection reworked (rust/cube-cli/src/client.rs:74-89). Previous heuristic: three-dot JWT → Bearer, else Api-Key. New heuristic: sk- prefix → Api-Key, else Bearer. The CUBE_AUTH_SCHEME=bearer|api-key override is preserved. Doc comment updated to note OAuth tokens aren't necessarily JWTs.
  2. "Cube CLI" branding propagated to cube update success message and the update-available notice (update.rs, commands/update.rs) — consistent with the --version change in the earlier commit.

Low-severity note

  • Default fallback flipped from Api-Key to Bearerclient.rs:82-88. The old heuristic defaulted anything that didn't look like a 3-segment JWT to Api-Key; the new one defaults anything without sk- to Bearer. This is safer for OAuth (which was the original bug driver — non-JWT OAuth tokens were being sent with Api-Key), but it does mean any pre-existing tenant API key that predates the sk- prefix convention will now silently be sent as Bearer and fail auth. If there's a possibility of legacy keys in the wild, CUBE_AUTH_SCHEME=api-key is the escape hatch — worth a mention in the README or the --api-key help text so users hitting a sudden 401 after upgrade have a discoverable fix. Not a blocker: the sk- prefix is the current convention and the override exists.

Prior notes recap (both still stand, both low-severity, no change required):

  • build.rs silently falls back to CARGO_PKG_VERSION (= 0.0.0) if lerna.json is missing/malformed for in-tree builds — consider a cargo:warning= or a panic when the file exists but parse fails.
  • cube login --api-key ... with omitted --url silently defaults to https://cubecloud.dev, which will fail validation against a tenant-scoped key — a clearer error or requiring --url alongside --api-key would be friendlier.
  • Server-supplied tenantUrl from the token response is trusted verbatim; cheap defense is asserting https:// scheme before saving.

Nothing else surfaces on code quality, security, or performance.

• Branch: claude/cube-rust-cli-tool-oiv9br

…login

The Enter fallback in cube login targets https://cubecloud.dev, which
doesn't serve the device-OAuth endpoints yet; the env override lets the
generic sign-in be pointed at a console that does (verified against
staging: Enter → device code minted, no 404).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdcyqFwUF9BbWLeX4uERnk
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 39s —— View job


Code Review (updated for 398e864)

Verdict: LGTM — 0 blocking on the new commit. Small delta; prior low-severity notes still stand.
  • Read new commit diff (398e864)
  • Re-check prior notes still apply
  • Post review

Summary of the new commit (398e864)

GENERIC_URL const becomes a generic_url() fn that reads CUBE_GENERIC_LOGIN_URL and falls back to https://cubecloud.dev. Small, mechanical, well-scoped — useful for pointing the tenant-discovery flow at a staging console without editing config. Doc comment on the fn covers the intent.

Note on the new code

  • Function is called twice at most per invocation but re-reads the env var each time (once when defaulting url, and it's a String allocation each call). Not a concern — this is a CLI startup path, not a hot loop — just flagging that a once_cell::sync::Lazy or reading it once at the top of command would be equivalent and marginally cleaner. Skip if not worth the churn.

Prior notes recap (unchanged from the earlier passes, all low-severity, no blockers):

  1. build.rs silently falls back to CARGO_PKG_VERSION (= 0.0.0) if lerna.json is missing/malformed. A cargo:warning= or panic-on-parse-error when the file exists but fails to parse would catch accidental in-tree misconfig.
  2. cube login --api-key ... with omitted --url silently defaults to the generic URL; combined with a tenant-scoped API key that will fail Client::new validation with a non-obvious error. Consider requiring --url alongside --api-key, or a clearer message when the resolved url is the generic one in the api-key branch.
  3. Server-supplied tenantUrl from the OAuth token response is trusted verbatim (login.rs:52-54) — cheap defense is asserting https:// scheme before saving. With the new env override, this becomes marginally more relevant since the "authority" host is now user-configurable and could point at plain-HTTP dev endpoints.
  4. Default auth-scheme heuristic flipped from Api-Key to Bearer (client.rs). Legacy tenant API keys without the sk- prefix will now be sent as Bearer and fail auth; CUBE_AUTH_SCHEME=api-key is the escape hatch — worth surfacing in the README or --api-key help.

Nothing else surfaces on code quality, security, or performance. jq in the release workflow is still fine on GitHub-hosted runners.

• Branch: claude/cube-rust-cli-tool-oiv9br

In the tenant-discovery flow the browser link is now
{host}/auth?redirect_to=<percent-encoded device approval path> instead
of the bare device page, so the user signs in first (which resolves
their tenant) and lands on the approval page. Tenant-URL logins keep
the direct verification link.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdcyqFwUF9BbWLeX4uERnk
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 48s —— View job


Code Review (updated for 0edf4bf)

Verdict: LGTM — 0 blocking on the new commit. Prior low-severity notes still stand.
  • Read new commit diff (0edf4bf)
  • Sanity-check path_and_query / percent_encode helpers
  • Re-check prior notes still apply
  • Post review

Summary of the new commit (0edf4bf)

In the tenant-discovery branch, the browser link is now {host}/auth?redirect_to=<percent-encoded device approval path> instead of the bare device verification URL, so users sign in first (which resolves the tenant) and land on the approval page. Tenant-URL logins (--url provided) keep the direct device link. The rewrite is gated by a small generic flag threaded into device_login — clean and well-scoped.

The two new helpers are a reasonable minimal dep-free approach:

  • path_and_query strips the scheme/host by finding the first / after ://. Returns None if the URL has no path — in which case the fallback silently uses the un-rewritten verification URI, which is a fine degrade.
  • percent_encode preserves the RFC 3986 unreserved set (A-Za-z0-9-_.~) and %-encodes everything else. Correct for a query-parameter value.

Minor notes on the new code (no change required)

  1. Trailing-slash sensitivity on CUBE_GENERIC_LOGIN_URLlogin.rs:112. format!("{url}/auth?redirect_to=...") will produce https://host//auth if the env override happens to include a trailing slash. The default (https://cubecloud.dev) is fine; only relevant if the env var is set with a trailing /. A one-line url.trim_end_matches('/') would harden this.
  2. Silent no-op when path_and_query returns None — if the OAuth server returned a verification_uri without a path (unusual but not impossible), the generic-flow rewrite is skipped and the bare URL is shown. Not wrong, just worth knowing the fallback exists.
  3. Hand-rolled percent-encoding — the codebase already pulls reqwest, which transitively pulls percent-encoding. Using percent_encoding::utf8_percent_encode(raw, NON_ALPHANUMERIC) would be equivalent and drops ~10 lines. Purely stylistic — the current impl is correct and self-contained.

Prior notes recap (unchanged, all low-severity, no blockers):

  1. build.rs silently falls back to CARGO_PKG_VERSION (0.0.0) if lerna.json is missing/malformed — a cargo:warning= or panic-on-parse-error when the file exists but fails to parse would surface accidental in-tree misconfig.
  2. cube login --api-key ... with omitted --url silently defaults to the generic URL and will fail Client::new validation against a tenant-scoped key with a non-obvious error. Consider requiring --url alongside --api-key, or a clearer message.
  3. Server-supplied tenantUrl from the OAuth token response is trusted verbatim (login.rs:52-54) — cheap defense is asserting https:// scheme before saving. Marginally more relevant now that the "authority" host is user-configurable via CUBE_GENERIC_LOGIN_URL.
  4. Default auth-scheme heuristic flipped from Api-Key to Bearer — legacy tenant API keys without the sk- prefix will now be sent as Bearer and fail auth. CUBE_AUTH_SCHEME=api-key is the escape hatch; worth surfacing in the README or --api-key help.

Nothing else surfaces on code quality, security, or performance.

• Branch: claude/cube-rust-cli-tool-oiv9br

There's no server to mint device codes on the generic cubecloud.dev
host, so the Enter fallback can't work — POST /auth/device/code 404s
before the browser ever opens. Reverts the generic sign-in fallback,
the /auth?redirect_to= link wrapping, the CUBE_GENERIC_LOGIN_URL
override, and the tenantUrl token field; cube login requires the
tenant URL again.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdcyqFwUF9BbWLeX4uERnk
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 44s —— View job


Code Review (updated for 42948d9)

Verdict: LGTM — 0 blocking. 2 low-severity notes remain; several prior notes are now moot after the revert.
  • Read new commit diff (42948d9)
  • Re-check which prior notes still apply
  • Post review

Summary of the new commit (42948d9)

Reverts the tenant-discovery login fallback. Reason (per the commit message): there's no server minting device codes on the generic cubecloud.dev host, so POST /auth/device/code 404s before the browser link is ever shown — the Enter-to-discover path can't actually work. Undoes:

  • Empty-URL fallback + help hint in login.rs
  • CUBE_GENERIC_LOGIN_URL env override / generic_url() fn
  • /auth?redirect_to=<encoded device page> rewrite + the path_and_query / percent_encode helpers
  • TokenResponse::tenant_url field and its tenantUrl serde alias
  • device_login signature returns to (access_token, refresh_token)

Clean revert; cube login requires the tenant URL again, matching pre-023b9a9 behavior.

What the PR now contains (after the revert)

  1. Version from lerna.json via build.rs injecting CUBE_CLI_VERSION; release workflow replaces the sed step with a tag-vs-lerna.json consistency check.
  2. --version prints Cube CLI <version> via display_name; propagate_version = true dropped (fixes the Cube CLI-deployments rendering).
  3. sk- prefix → Api-Key; else Bearer. CUBE_AUTH_SCHEME override preserved.
  4. "Cube CLI" branding in the update-available notice / cube update success message.

Remaining low-severity notes (unchanged, still low-severity, no blockers)

  1. build.rs silent 0.0.0 fallbackrust/cube-cli/build.rs:11. If ../../lerna.json exists but fails to parse (or the path resolution silently misses in an in-tree context), the fallback yields a cube-cli 0.0.0 binary rather than a build failure. A println!("cargo:warning=…") when the fallback fires, or a panic when the file exists but serde_json::from_str fails, would surface accidental misconfig. Fix this →

  2. Default auth-scheme flipped Api-KeyBearerrust/cube-cli/src/client.rs. Legacy tenant API keys without the sk- prefix will now be sent as Bearer and fail auth. CUBE_AUTH_SCHEME=api-key is the escape hatch — worth surfacing in the README or in the --api-key help text so a user hitting a sudden 401 has a discoverable fix. Fix this →

Now-moot prior notes (the revert makes these irrelevant)

  • --api-key + omitted --url silently targets cubecloud.dev — omit---url path is gone.
  • Trust in server-supplied tenantUrl — field removed.
  • Trailing-slash / path_and_query / hand-rolled percent-encoding — helpers removed.
  • generic_url() re-reads env on each call — fn removed.

jq on the release workflow runner is still a non-issue on GitHub-hosted runners.

Nothing else surfaces on code quality, security, or performance.

• Branch: claude/cube-rust-cli-tool-oiv9br

@paveltiunov paveltiunov changed the title fix(cube-cli): version from lerna.json, "Cube CLI" branding, tenant discovery in login fix(cube-cli): version from lerna.json, "Cube CLI" branding, sk- API-key detection Jul 21, 2026
@paveltiunov
paveltiunov merged commit 47a9adc into master Jul 21, 2026
16 checks passed
@paveltiunov
paveltiunov deleted the claude/cube-rust-cli-tool-oiv9br branch July 21, 2026 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants