Skip to content

feat(cube-cli): GitHub import, upload deploy, install/self-update, telemetry#11291

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

feat(cube-cli): GitHub import, upload deploy, install/self-update, telemetry#11291
paveltiunov merged 15 commits into
masterfrom
claude/cube-rust-cli-tool-oiv9br

Conversation

@paveltiunov

@paveltiunov paveltiunov commented Jul 18, 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

Follow-up batch to #11289 for the Cube CLI (rust/cube-cli). Everything below was verified end-to-end against Cube Cloud staging.

New commands

  • cube github (gh) — the GitHub import flow: status, installations, repos, branches, and connect (clones a repo into the deployment's git storage and triggers the first build; installationId/name/branch contract).
  • cube deploy — the legacy cubejs deploy reimplemented on the public API: SHA-1 hashes local files, diffs against GET …/data-model/file-hashes, uploads only changed files through the transactional multipart endpoints (upload/startupload/fileupload/finish), prunes deleted files (--keep-missing opts out), sets a -m/--message commit message, and triggers a single build. Verified idempotent (unchanged tree → 0 uploads).
  • cube deployments build-status — normalized build/dev-mode status incl. compile-error text; cube deployments advance-step/reset-step — the new onboarding endpoints; cube data-model file-hashes; cube data-model commit --branch.
  • cube api-keys — list/create/get/delete (for the upcoming public API-key CRUD).
  • cube update — self-update: downloads the latest release asset for the platform and atomically swaps the running binary (rename-aside, Windows-safe, rollback on failure). Every run also checks for a newer release in the background and prints a notice on interactive terminals (CUBE_NO_UPDATE_CHECK=1 opts out).

Install scripts (install/): install.sh (Linux/macOS) and install.ps1 (Windows) download the latest release binary for the detected platform and put it on PATH (CUBE_INSTALL_DIR, CUBE_VERSION overrides).

Telemetry: anonymous usage events wire-compatible with the legacy cubejs CLI pipeline (track.cube.dev): one Cube CLI Command event per invocation plus the legacy Cube Cloud CLI Authenticate/Deploy/Deploy Success/Error names; anonymous id is a SHA-256 of the OS machine id. Disabled in CI or via CUBE_NO_TELEMETRY=1/CUBEJS_TELEMETRY=false.

Auth: API keys are sent as Authorization: Api-Key … — the client picks the scheme per credential (JWT → Bearer, opaque key → Api-Key; CUBE_AUTH_SCHEME overrides). Verified against staging with an admin API key across base-served, build-served, and scoped endpoints.

Robustness fixes found while testing: device-login polling survives transient connection resets; 200-with-HTML responses (unknown route → SPA fallback) produce a clear "endpoint not available on this tenant" error; TLS uses rustls-tls-native-roots so SSL_CERT_FILE/OS CAs work behind TLS-inspecting proxies (still rustls, still static-musl); SIGPIPE restored on unix so piped output doesn't panic; github tables use the real API field names.

Removed: the ai-engineer command group.

🤖 Generated with Claude Code


Generated by Claude Code

Drop the ai-engineer command group (AI Engineer settings) from the CLI:
delete the commands/ai_engineer module, its dispatch in main.rs, and the
README coverage row.

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 18, 2026
@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

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


Code Review

Verdict: LGTM — clean, mechanical removal. ✅

Details
  • Read PR diff
  • Verify no dangling references to ai_engineer / AiEngineer / ai-engineer remain in rust/cube-cli
  • Check module registration, dispatch arm, and README table are all consistently updated

Findings

  • All four touched files consistently drop the ai-engineer command:
    • rust/cube-cli/src/commands/ai_engineer.rs deleted
    • rust/cube-cli/src/commands/mod.rspub mod ai_engineer; removed
    • rust/cube-cli/src/main.rsAiEngineer variant and dispatch arm removed
    • rust/cube-cli/README.mdai-engineer row removed from coverage table
  • Grep for ai[_-]?engineer|AiEngineer (case-insensitive) across rust/cube-cli returns zero matches — no lingering references, imports, or docs.
  • Alphabetical ordering in both the Command enum and the mod.rs module list is preserved.
  • No tests referenced the command, so nothing else needs pruning.
  • Diff is deletion-only (+0/-50); no security, performance, or error-handling surface changed.

Nits: none.

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

Add a github command group (alias gh) covering the new GitHub
integration public REST API:

- github status — link/install state + browser URLs
- github installations — the user's GitHub App installations
- github repos <installation> — repositories visible to an installation
- github branches <owner/repo> --installation <id> — repo branches
- github connect <deployment> <owner/repo> --installation <id>
  [--branch] — build-served import: clones the repo into the
  deployment's git storage and triggers the first build

Also make the device-flow token poll resilient to transient network
failures: a dropped connection mid-poll now keeps polling until the
device code expires instead of aborting the login.

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

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 4s —— View job


Code Review

Verdict: LGTM — the ai-engineer removal is clean, and the new github command group + oauth retry are well-scoped. No blockers.

Details
  • Read PR diff (across both commits)
  • Verify ai-engineer removal is complete (no dangling refs)
  • Review new commands/github.rs
  • Review oauth.rs transient-error retry
  • Cross-check consistency with existing command style

Scope

Two commits on this branch:

  1. refactor(cube-cli): remove ai-engineer command — pure deletion (matches PR title/description).
  2. feat(cube-cli): github integration commands — adds commands/github.rs, wires it into main.rs/mod.rs, updates README, and makes poll_for_token resilient to transient network errors. Worth noting that the PR title/description mention only the ai-engineer removal, so a reader looking at just the description might miss the github feature work. Not a blocker, but consider expanding the PR body (or splitting) so reviewers know to look at both.

commands/github.rs

  • split_repo (line 55) correctly rejects empty owner/name and embedded slashes — a decent guardrail before interpolating into paths. Nit: it doesn't reject leading/trailing whitespace, but clap already trims arg values so it's fine in practice.
  • Slight inconsistency in how installation is passed:
    • Repos { installation: i64 } — positional
    • Branches { installation } and Connect { installation }--installation
      Since the field name is identical across three subcommands, having it positional in one and a flag in the other two is a small UX papercut. Making Repos also use --installation (or making all three positional) would be more predictable. Not blocking.
  • Cmd::Connect composes the JSON body by inserting installationId, owner, repo, then optional branch, then merging arbitrary --data on top. Because parse_data is applied last, a caller can override installationId/owner/repo via -d — probably intentional as an escape hatch, but worth confirming that's the desired precedence.
  • All request paths interpolate validated inputs (installation is i64; owner/name come from split_repo), so no injection surface.
  • Table column choices for installations and repos look right; Branches prints a single BRANCH column which is fine.

oauth.rs (lines 133–141)

  • The retry-on-transport-error is the right call — a server redeploy mid-login shouldn't nuke the entire device-code flow. The tokio::time::sleep(interval) at the top of the loop and the deadline guard keep it from spinning and bound the total wait, so no runaway.
  • One tradeoff worth flagging: the transport error is fully swallowed. If the user has a genuinely broken endpoint (bad URL, DNS, cert), they'll wait until device.expires_in before seeing any signal. Consider surfacing a warning after N consecutive failures, or logging the error under a verbose flag. Not a blocker.

Housekeeping

  • README table updated to add github and remove ai-engineer.
  • Command enum and dispatch arm ordering are internally consistent with the added row's placement in mod.rs.
  • No tests exist for the CLI command layer in this crate (pre-existing gap); this PR doesn't regress that.

Nits

  • Consider a brief README example for cube github connect <deployment> <owner/repo> --installation <id> — it's the most novel subcommand and the multi-arg shape isn't obvious from --help alone.
• Branch: `claude/cube-rust-cli-tool-oiv9br`

Cube Cloud answers unknown API routes with 200 + the SPA HTML. Detect
an HTML body on a successful response and fail with "endpoint is not
available on this tenant" instead of treating the page as a JSON
string, which rendered as a misleading empty table.

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

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 10s —— View job


Code Review

Verdict: LGTM — the ai-engineer removal, the new github command group, the oauth retry, and the web-app-response guard are all well-scoped. No blockers. 0 high, 2 medium, 1 low.

Details
  • Read PR diff across all three commits
  • Verify ai-engineer removal is complete
  • Review new commands/github.rs
  • Review oauth.rs transient-error retry
  • Review client.rs HTML-response guard

Scope

Three commits on this branch:

  1. refactor(cube-cli): remove ai-engineer command — pure deletion, matches the PR title.
  2. feat(cube-cli): github integration commands — adds commands/github.rs, wires it into main.rs/mod.rs, updates the README, and makes poll_for_token resilient to transient network errors.
  3. fix(cube-cli): clear error when a route returns the web app — surfaces "endpoint not available" instead of silently rendering an empty table when Cube Cloud serves the SPA on an unknown route.

The PR title/description still describe only the ai-engineer removal, so a reader looking only at the description will miss the github feature and the client fix. Consider expanding the body (or splitting) so reviewers know to look at all three.

commands/github.rs (medium)

  • installation is passed inconsistently across subcommands: positional in Repos { installation: i64 } (rust/cube-cli/src/commands/github.rs:23), but a --installation flag in Branches (line 30–31) and Connect (line 41–42). Same field name, three subcommands, two different shapes — small UX papercut. Making all three consistent (I'd lean toward --installation everywhere, since it's clearer when combined with repo) would be more predictable.
  • In Cmd::Connect, parse_data is applied after installationId/owner/repo are inserted (github.rs:114–121), so -d '{"owner":"…"}' silently overrides the positional args. If that's an intentional escape hatch it's fine; otherwise consider inserting the parsed data first and then the explicit fields on top so the CLI flags always win. Worth confirming precedence is deliberate.
  • split_repo (line 55) correctly rejects empty owner/name and embedded slashes. Minor nit: doesn't reject whitespace, but clap already trims arg values so it's fine in practice.
  • All request paths interpolate validated inputs (installation is i64, owner/name come from split_repo), so no injection surface.

oauth.rs (lines 133–141) (low)

The retry-on-transport-error is the right call — a server redeploy mid-login shouldn't nuke the device-code flow. Bounded by deadline and the interval sleep, so no runaway. Tradeoff: transport errors are fully swallowed — if the user has a genuinely broken endpoint (bad DNS, TLS, URL), they'll wait until device.expires_in before seeing any signal. Consider surfacing a warning after N consecutive failures or under a verbose flag. Not a blocker.

client.rs HTML guard (lines 152–166) (medium)

Nice UX fix. Two thoughts:

  • The looks_like_html check (trimmed.starts_with('<') && !trimmed.starts_with("<?xml")) is fine for this API's JSON contract (JSON never starts with <), but it also catches responses that begin with an HTML-ish char even if the endpoint intended to return something else. In practice all Cube Cloud endpoints return JSON, so this is safe today; worth a one-line comment pinning that assumption.
  • The error message says the endpoint "is not available on this tenant". That's accurate for the common cause (older server version) but also fires on genuine path typos in cube api and on future-added routes that this CLI version doesn't know about. Consider also hinting at "double-check the path" — small wording tweak, not a bug.

Housekeeping

  • README table updated to add github and remove ai-engineer. Command enum and dispatch arm ordering in main.rs are internally consistent.
  • No tests exist for the CLI command layer in this crate (pre-existing gap); this PR doesn't regress that.

Nits

  • Brief README example for cube github connect <deployment> <owner/repo> --installation <id> would help — it's the most novel subcommand and the multi-arg shape isn't obvious from --help.
• Branch: `claude/cube-rust-cli-tool-oiv9br`

Align the github installations/repos/branches table columns with the
field names the API actually returns (installationId/login, name/
htmlUrl, name/isDefault), verified against staging.

Restore the default SIGPIPE disposition on unix so piping output
(e.g. `cube github repos ... | head`) exits quietly instead of
panicking with a broken-pipe backtrace.

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

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 4s —— View job


Code Review

Verdict: LGTM on the new commit (06f2a64). No new blockers. Prior findings from the earlier review round still apply — none have been addressed in this push, but none are blocking either.

Details
  • Diff the new commit against the previously-reviewed tip
  • Sanity-check the SIGPIPE change
  • Sanity-check the field-name updates in github.rs
  • Re-check that prior findings are still accurate against the current tree

What's new in 06f2a64

Two independent fixes:

  1. SIGPIPE default disposition (rust/cube-cli/src/main.rs:197-202, Cargo.toml). Restoring SIGPIPE → SIG_DFL before any I/O is the standard idiom for Rust CLIs — fixes the broken-pipe backtrace when piping into head/less. Placement is right (before Cli::parse() and before any writes), the #[cfg(unix)] guard is correct, and libc is scoped as a unix-only dep in Cargo.toml. The unsafe block is minimal and unavoidable. No concerns.

  2. Table column alignment (rust/cube-cli/src/commands/github.rs:71,81,95). Column keys now match the actual API response shape (installationId/login, name/htmlUrl, name/isDefault). Commit message says these were verified against staging; nothing to critique from static review. The dropped PRIVATE/TYPE/DEFAULT BRANCH columns are a reasonable trade — the table gets narrower and the JSON view (--json) still exposes everything.

Prior findings — status against current tip

Still open (not blocking):

  • installation is positional on Repos but a --installation flag on Branches/Connect (github.rs:23 vs 30–31/41–42). Small UX inconsistency.
  • In Cmd::Connect, parse_data is merged after the explicit fields (github.rs:107–113), so -d '{"owner":"…"}' silently overrides the positional args. Fine as an escape hatch, worth confirming that's intentional.
  • oauth.rs transient-error retry swallows the underlying error until device.expires_in fires (oauth.rs:133–141). A warning after N consecutive failures would improve the UX on a genuinely broken endpoint.
  • client.rs HTML-response guard (lines 152–166) — message says "not available on this tenant"; also fires on path typos. Wording nit only.

Housekeeping

  • No tests for the CLI command layer in this crate; pre-existing gap, not regressed.
  • PR title/description still only mention the ai-engineer removal; the branch now contains four distinct changes (removal + github commands + web-app-response guard + SIGPIPE/field-names). Consider expanding the PR body so reviewers know to look at everything.
• Branch: `claude/cube-rust-cli-tool-oiv9br`

The connect endpoint takes the repository as `name` (the owner is
implied by the installation), so send installationId/name/branch and
accept either a plain repo name or owner/repo on the command line.
Verified end-to-end against staging: connect clones the repo into the
deployment's git storage, and the typed error paths (already connected,
deployment not found) surface cleanly.

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

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — commit 43b89ea

Verdict: LGTM. The API-contract alignment is a small, focused change. One minor validation gap; prior findings from earlier rounds still apply.

Details
  • Diff 43b89ea against previously-reviewed tip
  • Re-read commands/github.rs end to end
  • Check prior findings against current tree

What changed in 43b89ea

Cmd::Connect now sends { installationId, name, branch? } instead of { installationId, owner, repo, branch? } — the owner is implied by the installation. The CLI accepts either a plain repo name or owner/repo and strips the owner client-side via rsplit_once('/'). Help text updated. Verified end-to-end against staging per commit message. Clean and well-scoped.

New observation (low)

rust/cube-cli/src/commands/github.rs:108-111 — the rsplit_once('/') accepts inputs the server will reject:

  • ""name = ""
  • "foo/"name = ""
  • "a/b/c"name = "c" (silently drops the middle segment)

Not a security issue (all fields are typed / URL-safe when interpolated), but a one-line anyhow::bail! on an empty result would give a clearer error than the server's 400. Trivial to skip if you'd rather let the server own that message.

Prior findings — status against current tip (all still open, none blocking)

  • installation argument shape is inconsistent — positional on Repos { installation: i64 } (github.rs:23), --installation flag on Branches/Connect (github.rs:31, 43). Minor UX papercut.
  • parse_data still merged after explicit fields (github.rs:113-118) — -d '{"installationId": 99}' overrides the positional/flag values. Fine as an escape hatch; worth confirming it's deliberate. This finding is now more relevant since the connect body has fewer fields to override.
  • oauth.rs:133-141 transient-error retry swallows the underlying error until device.expires_in fires — a warning after N consecutive failures would help debugging genuinely-broken endpoints.
  • client.rs HTML guard — "not available on this tenant" also fires on path typos; wording nit only.
  • Branches still requires owner/repo while Connect now accepts a plain name. Different endpoints legitimately need different shapes (the /repositories/{owner}/{name}/branches path needs both), so this is defensible — just worth being aware of the inconsistency for users.

Housekeeping

  • No new tests; CLI command layer remains uncovered (pre-existing gap, not regressed).
  • PR title/body still only mention the ai-engineer removal; the branch now contains five distinct changes. Consider expanding the description so reviewers know to look at everything.
• Branch: `claude/cube-rust-cli-tool-oiv9br`

Add `deployments advance-step` and `deployments reset-step` for the
new dedicated onboarding endpoints (POST /creation-step/advance with
{creationStep}, POST /creation-step/reset) now that creationStep is
stripped from the generic update input.

Give `data-model commit` a --branch flag (sent as branchName) so a
non-dev-mode branch can be committed, matching the other branch-aware
data-model commands.

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

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — commit 07309f9

Verdict: LGTM on the new commit. Small nits only; prior findings from earlier rounds still apply, none blocking.

Details
  • Diff 07309f9 against previously-reviewed tip (43b89ea)
  • Read the touched files end to end
  • Re-check prior findings against current tree
  • Dedup vs. existing inline threads (none exist — nothing to resolve)

What changed in 07309f9

Three small additions:

  1. data-model commit gains an optional --branch flag (rust/cube-cli/src/commands/data_model.rs:113-117, 373-377), sent as branchName in the body. Defaults to the active dev-mode branch server-side. Naming matches the API contract style already used for branches.
  2. Two new deployment subcommands (rust/cube-cli/src/commands/deployments.rs:89-100, 200-217):
    • advance-step <deployment> <step>POST /api/v1/deployments/{id}/creation-step/advance with {"creationStep": step}.
    • reset-step <deployment>POST /api/v1/deployments/{id}/creation-step/reset.
  3. README coverage table updated to list both.

Wiring, error propagation, and JSON output all follow the existing patterns in the file. deployment: i64 gives free clap-level validation for the path segment.

New observations (low)

  • step: String (deployments.rs:94) is free-form — anything typed goes to the server. The docstring lists eight valid values (project, upload, schema, github, ssh, databases, ready, demo). Two small options:
    • Use a #[derive(clap::ValueEnum)] so --help shows the values, tab-completion works, and typos fail locally with a clean message.
    • Or leave as String (defensible: server owns the source of truth, and CLI won't need a release when a new step is added) but then consider dropping the enumeration from the docstring so it doesn't drift silently. Trade-off is discoverability vs. drift.
  • The docstring list is also not sorted or referenced from a docs URL — if it drifts, the --help output will lie without any signal. Minor.
  • No --data escape hatch on either new subcommand (unlike create/update). Almost certainly fine — these endpoints don't take other fields — just noting for consistency.

Prior findings — status against current tip (all still open, none blocking)

  • installation arg shape inconsistent across github subcommands (positional on Repos, --installation on Branches/Connect).
  • parse_data merged after explicit fields in several places, letting -d override typed args — intentional as an escape hatch, worth confirming.
  • oauth.rs:133-141 retry swallows transport errors until expires_in — a warning after N consecutive failures would help diagnose broken endpoints.
  • client.rs HTML guard message ("not available on this tenant") also fires on path typos; wording nit.

Housekeeping

  • No new tests; CLI command layer remains uncovered (pre-existing gap, not regressed by this commit).
  • PR title/body still only describe the ai-engineer removal; the branch now contains six distinct changes. Consider expanding the description so reviewers know to look at everything, or split into follow-up PRs.
• Branch: `claude/cube-rust-cli-tool-oiv9br`

Reimplements the legacy `cubejs deploy` on the public API as `cube
deploy`: hash local files (sha1), diff against GET data-model/file-hashes,
upload only changed files through the transactional multipart endpoints
(upload/start, upload/file, upload/finish), and finish with a manifest
that prunes deleted files (--keep-missing opts out) and triggers a single
build. Adds `deployments build-status` (normalized build/dev-mode status
with compile-error text), `data-model file-hashes`, `deployments
advance-step`/`reset-step` for the new onboarding endpoints, and
`data-model commit --branch`. The HTTP client gains a multipart POST
with the same 401 auto-refresh retry as JSON requests.

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

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 16s —— View job


Code Review — commit 6359d0f

Verdict: LGTM on the new commit. Small notes only; prior review-thread findings from earlier rounds still apply, none blocking.

Details
  • Diff 6359d0f against previously-reviewed tip (07309f9)
  • Read the new commands/deploy.rs end to end
  • Review the post_multipart addition in client.rs
  • Sanity-check the new data-model file-hashes and deployments build-status subcommands
  • Re-check prior findings against current tree

What's new in 6359d0f

Four related additions that together port the legacy cubejs deploy flow onto the public API:

  1. client.rsClient::post_multipart (rust/cube-cli/src/client.rs:135-160) and a shared finish_response tail. Handles the 401→refresh retry by taking a Fn() -> Form closure so a fresh form can be built per attempt (multipart forms are single-use). Sha1 comes in as a Cargo dep.
  2. commands/data_model.rs:119-125, 398-408data-model file-hashes (GET /build/api/v1/deployments/{id}/data-model/file-hashes?branchName=…), JSON-only output.
  3. commands/deployments.rs:89-96, 208-218deployments build-status (GET /api/v1/deployments/{id}/build-status?branchName=…), JSON-only output.
  4. commands/deploy.rs — new cube deploy command: walks the directory, hashes files, diffs against the server, opens a transaction (upload/start), uploads only changed files as multipart, then commits with upload/finish (with autoRemoveFiles = !--keep-missing).

Wiring in main.rs and mod.rs matches the existing style. Commit message says it was verified end-to-end against staging.

Observations (low)

  • collect_files (rust/cube-cli/src/commands/deploy.rs:34-59) uses path.is_dir(), which follows symlinks. A symlink cycle (e.g. . linking to a parent) would recurse until it blows the stack or the fs limits. Unlikely in a real data-model tree, but a let meta = entry.metadata()?; if meta.file_type().is_symlink() { continue; } guard (or a visited-inode set) would be cheap defense. Legacy cubejs deploy probably has the same edge, so not a regression.
  • Filesystem I/O is synchronous (std::fs::read_dir, std::fs::read) inside an async function. Fine for CLI use — data-model files are small and there's no concurrent request work happening at the same time — but worth noting if this ever grows to parallel uploads.
  • Uploads are strictly sequential (rust/cube-cli/src/commands/deploy.rs:118-137). For a large first-time deploy where every file is "changed", this can be noticeably slower than legacy which pipelined a few. Not a correctness issue; a follow-up to bound concurrency (futures::stream::iter().buffer_unordered(4)) would be an easy win later.
  • transaction_str = serde_json::to_string(&transaction)? (deploy.rs:104) sends the transaction as a JSON-encoded value in the multipart text field, while the same transaction is later placed as a JSON Value in the finish body (deploy.rs:141). That's presumably what the server expects (commit says verified), but it's the kind of asymmetry that's easy to trip on if you refactor later — one line of comment pinning "multipart takes stringified JSON here" would help future editors.
  • .mime_str("application/octet-stream").expect("static mime type is valid") (deploy.rs:127-130) is unavoidable given Part::mime_str's return type — fine.
  • to_upload holds every changed file's bytes in memory simultaneously, and each multipart send clones them again for the retry-safe closure. Data-model files are tiny, so this is fine in practice; worth being aware of before repurposing this path for anything larger.
  • data_model file-hashes and deployments build-status are JSON-only (no table view). Consistent with other "raw" endpoints in the CLI; probably intentional. Fine.

Prior findings — status against current tree (all still open, none blocking)

  • installation arg shape inconsistent across github subcommands (positional on Repos, --installation on Branches/Connect).
  • parse_data merged after explicit fields in several places, letting -d override typed args — intentional as an escape hatch, worth confirming.
  • oauth.rs:133-141 retry swallows transport errors until expires_in — a warning after N consecutive failures would help diagnose broken endpoints.
  • client.rs HTML guard message ("not available on this tenant") also fires on path typos; wording nit.
  • deployments advance-step's step: String is free-form; #[derive(ValueEnum)] would give tab-completion and local validation.

Housekeeping

  • No new tests; CLI command layer remains uncovered (pre-existing gap, not regressed by this commit).
  • PR title/body still describe only the ai-engineer removal, but the branch now contains seven distinct changes (removal + github commands + web-app-response guard + SIGPIPE/field-names + api-contract alignment + creation-step + deploy/upload). Consider expanding the description so reviewers know to look at everything, or splitting into follow-up PRs.

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

Manage API keys for programmatic access: list, create (secret returned
once), get, delete/revoke. API keys are accepted as the Bearer
credential across all public APIs; `cube login --api-key` stores one
as a context.

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

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — commit d08a5c9

Verdict: LGTM on the new commit. Small notes only; prior review-thread findings from earlier rounds still apply, none blocking.

Details
  • Diff d08a5c9 against previously-reviewed tip (6359d0f)
  • Read commands/api_keys.rs end to end
  • Verify wiring in main.rs and commands/mod.rs
  • Re-check prior findings against current tree

What's new in d08a5c9

A standard-shaped api-keys command group (rust/cube-cli/src/commands/api_keys.rs) with list/create/get/delete — and matching wiring in main.rs:174-176, 244 plus mod.rs. Mirrors the patterns already used by other resource groups (e.g. deployments), so the surface is predictable.

Observations (low)

  • Precedence is opposite to github connect — probably correct. In Cmd::Create (api_keys.rs:48-52) util::set(&mut body, "name", &name) runs after parse_data, so the explicit --name flag wins over -d '{"name":"…"}'. That's the sensible default (flags beat freeform data). Worth calling out because it's the opposite order used in github.rs Connect (still open finding), which lets -d override the positional args. Two subcommands in the same crate applying -d with different precedence is the surprising bit — pick one convention.
  • delete has no confirmation prompt. API keys are effectively credentials — a revoke mistake immediately breaks any client using that key. Consistent with other resource deletes in the CLI, and --json scripting is a legitimate use case, so not a blocker. A --yes/interactive Are you sure? for TTY is worth considering as a follow-up (across all destructive commands, not just this one).
  • Delete returns whatever the server sends. In JSON mode we print res verbatim, which for a DELETE may be {} or null depending on server behavior. Fine, just noting that the printed value is not stable.
  • --name on create is Option<String>. If the server requires a name, letting clap enforce it (#[arg(long)] without Option) gives a clearer local error than a 400. If the server auto-generates a name when omitted, ignore this. Trivial either way.
  • Secret-only-returned-once (from the doc comment) is a real footgun for shell users. A one-liner in the Create help text along the lines of "the response contains a secret field returned only once — capture it now" would help; even better, in table mode, print the secret prominently. Currently output::print_json(&res) is used for Create, which is the safe default.
  • No --data escape hatch on Get/Delete. Consistent with other resource commands; noted for symmetry only.

Prior findings — status against current tree (all still open, none blocking)

  • installation arg shape inconsistent across github subcommands (positional on Repos, --installation on Branches/Connect).
  • parse_data merged after explicit fields in github connect (opposite of the new api-keys create here) — see precedence note above.
  • oauth.rs:133-141 retry swallows transport errors until expires_in.
  • client.rs HTML guard message fires on path typos as well; wording nit.
  • deployments advance-step's step: String is free-form; ValueEnum would be nicer.
  • deploy.rs follows symlinks in collect_files and holds all changed-file bytes in memory concurrently.

Housekeeping

  • No new tests; CLI command layer remains uncovered (pre-existing gap, not regressed by this commit).
  • PR title/body still describe only the ai-engineer removal, but the branch now contains eight distinct changes. Consider expanding the description so reviewers know to look at everything, or splitting into follow-up PRs.

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

Distribution and self-update:
- install/install.sh (Linux/macOS) and install/install.ps1 (Windows)
  download the latest release asset for the detected platform and install
  it onto PATH; CUBE_INSTALL_DIR and CUBE_VERSION override location/tag.
- Every run spawns a background check against the GitHub releases API and
  prints an update notice (stderr, interactive terminals only) after the
  command output; CUBE_NO_UPDATE_CHECK=1 disables it. Skipped for
  `completion` (shell-eval'd) and `update` itself.
- `cube update` downloads the latest release, extracts the binary, and
  atomically replaces the running executable (rename-aside strategy that
  also works on Windows, with rollback on failure and stale-.old cleanup
  on the next run). `cube update --check` only reports.
- reqwest now uses rustls-tls-native-roots so SSL_CERT_FILE and
  OS-installed CAs are honored (TLS-inspecting corporate proxies) —
  still rustls, still static-musl-friendly.

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

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 51s —— View job


Code Review — commit e0b6eb5

Verdict: LGTM. 0 high, 1 medium, 3 low. Prior findings from earlier rounds still apply, none blocking.

Details
  • Diff e0b6eb5 against previously-reviewed tip (d08a5c9)
  • Read src/update.rs, src/commands/update.rs, both install scripts, main.rs wiring
  • Re-check prior findings against current tree
  • Dedup vs. existing inline threads (none present)

What's new in e0b6eb5

Three related additions:

  1. Install scriptsinstall/install.sh (Linux/macOS, curl-piped-to-sh style) and install/install.ps1 (Windows PowerShell). Both fetch the platform asset from releases/latest/download/…, extract, drop onto PATH; overridable via CUBE_INSTALL_DIR / CUBE_VERSION. Windows script also appends %LOCALAPPDATA%\cube\bin to the user PATH.
  2. update command (src/commands/update.rs) — cube update / cube update --check. Downloads the tar.gz for the current target, extracts the cube/cube.exe entry, and swaps the running exe via rename-aside so Windows can overwrite in-use files. Leftover .old is cleaned up on the next run.
  3. Background update check on every invocation (src/update.rs, wired in main.rs:212-226) — spawns a concurrent latest_release fetch, prints a one-line notice on stderr only if stderr is a TTY. Bounded by a 1.5s grace period after the command finishes. Opt-out via CUBE_NO_UPDATE_CHECK. Skipped for update and completion.

Observations

  • spawn_check hits GitHub even when not attached to a terminal (medium)src/update.rs:99-122 spawns the API call unconditionally; only print_notice (line 126) gates on stderr().is_terminal(). So a scripted user piping cube ... | jq still spends a GitHub unauthenticated API request (60/hr per IP) on every invocation, discards the result, and gets nothing for it. In tight loops this can rate-limit the user's IP against unrelated GitHub usage. Moving the is_terminal() check into spawn_check (early-return before spawning) or persisting a "last checked at" timestamp under ~/.cube/ with a 24h TTL would both fix it. Not a blocker — the current behavior is functionally silent — but it's wasteful. Fix this →

  • newer_than and is_newer are the same function, duplicated (low)src/update.rs:83-94 (newer_than, private) and src/commands/update.rs:64-75 (is_newer). Making update::newer_than pub and calling it from commands/update.rs avoids the two drifting. Trivial.

  • Version parse stops at the first non-numeric segment (low) — both copies of the comparator use map_while(|s| s.parse::<u64>().ok()), so 1.7.10 and 1.7.10-beta.2 both flatten to [1, 7, 10] and compare equal, and the stable release wouldn't trigger a notice over a beta with the same base. If pre-release tags aren't actually shipped this is a non-issue; if they might be (v1.8.0-rc1), semver's ordering rule (pre-release < release with same base) is the standard fix. Since the check silently no-ops on the corner case, low impact — worth being aware of before publishing pre-releases through releases/latest.

  • Install scripts have no checksum verification (low)install.sh and install.ps1 both download the tar.gz and extract directly, trusting the transport. This matches what most single-binary CLIs do (rustup, gh, etc.), but publishing a SHA256SUMS alongside the release artifacts and having the scripts verify would harden the install path against a compromised mirror or MITM on a user's LAN. Not a blocker; noting for the roadmap.

  • install.ps1 PATH mutation is one-way (nit) — appends $Dir;$UserPath if missing; there's no uninstaller path. Fine for a first-run tool; noting for symmetry.

Prior findings — status against current tree (all still open, none blocking)

  • installation arg shape inconsistent across github subcommands (positional on Repos, --installation on Branches/Connect).
  • parse_data precedence inconsistent between github connect (data wins) and api-keys create (flags win) — worth picking a single convention.
  • oauth.rs:133-141 retry swallows transport errors until expires_in.
  • client.rs HTML guard message fires on genuine path typos too; wording nit.
  • deployments advance-step's step: String is free-form; ValueEnum would give tab-completion.
  • deploy.rs collect_files follows symlinks; holds all changed-file bytes in memory concurrently.

Housekeeping

  • The newer_than unit test is nice — first test coverage in this crate. Consider extending to cover pre-release edges if pre-releases are on the roadmap.
  • Otherwise no new tests; CLI command layer remains uncovered (pre-existing gap, not regressed).
  • PR title/body still describe only the ai-engineer removal, but the branch now contains nine distinct changes (removal + github + web-app guard + SIGPIPE/field-names + api-contract alignment + creation-step + deploy/upload + api-keys + install/update). Consider expanding the description so reviewers know to look at everything, or splitting into follow-up PRs.
• Branch: `claude/cube-rust-cli-tool-oiv9br`

Adds telemetry mirroring the legacy cubejs CLI's track() from
@cubejs-backend/shared: events POST to track.cube.dev/track as a JSON
array with the same field names (event, cliVersion, clientTimestamp,
id, platform/arch in Node vocabulary, anonymousId, sentFrom, sentAt) so
they land in the existing pipeline. anonymousId is a SHA-256 of the OS
machine id (dbus/systemd id, IOPlatformUUID, MachineGuid — same sources
as the legacy machine-id module), hostname fallback.

Emitted events: one 'Cube CLI Command' per invocation (command group +
success flag), 'Error' with the error text on failure, and the legacy
names 'Cube Cloud CLI Authenticate' on login and 'Cube Cloud CLI
Deploy'/'Deploy Success' around cube deploy. Flush is bounded (2s,
silent failure) and runs after command output; completion is exempt.
Disabled in CI (CI env, legacy parity) and via CUBE_NO_TELEMETRY=1 or
CUBEJS_TELEMETRY=false. CUBE_TELEMETRY_URL overrides the sink for
testing.

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

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 2m 7s —— View job


Code Review — commit de3da7b

Verdict: LGTM. 0 high, 1 medium, 4 low. Prior review-thread findings from earlier rounds still apply, none blocking.

Details
  • Diff de3da7b against previously-reviewed tip (e0b6eb5)
  • Read src/telemetry.rs end-to-end
  • Review telemetry wiring in main.rs, commands/login.rs, commands/deploy.rs
  • Cross-check README claims against implementation
  • Re-check prior findings against current tree

What's new in de3da7b

New src/telemetry.rs (216 lines) plus wiring:

  • enabled() gates on CI/CUBE_NO_TELEMETRY/CUBEJS_TELEMETRY=false|0 — matches legacy @cubejs-backend/shared parity.
  • event(name, props) enqueues into a Mutex<Vec<Value>>; flush() POSTs the batch to track.cube.dev/track as a JSON array with legacy field names (event, cliVersion, clientTimestamp, id, platform, arch, anonymousId, sentFrom, sentAt). 2s timeout, silent on failure.
  • anonymous_id() = SHA-256 of the OS machine id (/etc/machine-id / IOPlatformUUID / MachineGuid), hostname fallback.
  • timestamp() computes ISO-8601 UTC by hand (Howard Hinnant civil-from-days) to avoid a chrono dep.
  • main.rs emits Cube CLI Command (with command/success) after every invocation; Error with format!("{err:#}") on failure. login.rs and deploy.rs emit legacy event names for parity.
  • README documents the pipeline and opt-outs.

Observations

  • [Medium — Privacy] The Error event body includes the full anyhow chain, which routinely contains local file paths and other PII (rust/cube-cli/src/main.rs:228-234props.insert("error", format!("{err:#}"))). Because a lot of the deploy/data-model paths call .with_context(|| format!("failed to read {}", path.display())), a real-world failure produces something like failed to read /home/alice/customer-x-prototype/schema.js: No such file… — that string then gets shipped to track.cube.dev. The README (README.md:41-44) says "No personal data is collected; the anonymous id is a SHA-256 hash of the OS machine id", but with the error message included that promise doesn't fully hold. Legacy @cubejs-backend/shared shipped a similar shape, so this is parity — worth deciding whether to (a) redact/truncate the error text, (b) send only the error type (root cause Display without the with_context chain), or (c) soften the README wording. Legacy parity is fine, but the doc claim should match the wire. Fix this →

  • [Low — Perf] anonymous_id() shells out on every call, not cached (telemetry.rs:139-144). A typical failing run enqueues up to three events (Cube CLI Command + Error + a domain event like Cube Cloud CLI Deploy), and each event evaluation on macOS/Windows spawns ioreg/REG. That's fast but wasteful and pins a subprocess dependency; a std::sync::OnceLock<String> around machine_id() would collapse it to one shell-out per process.

  • [Low] CUBEJS_TELEMETRY env parsing is stricter than the legacy JS (telemetry.rs:27-30). Only false/0 are matched; False/FALSE/no all enable telemetry. The legacy Node code typically lowercases before comparing. Consider Ok(v) if v.eq_ignore_ascii_case("false") || v == "0" for full parity.

  • [Low] random_id() acquires QUEUE.lock() to read .len() (telemetry.rs:114) while event() also acquires the same lock at line 51. Two separate acquisitions in sequence, not a deadlock today, but coupling random_id to the queue is fragile — inlining or moving the id generation would silently regress. Either drop the length input (nanos + pid + process-local counter is enough entropy) or pass the length in as an arg.

  • [Low] Command::Deploy enqueues Cube Cloud CLI Deploy/Deploy Success, but the failure case doesn't emit a Deploy Failure event (commands/deploy.rs:104, 148). Any ?-bailout between Deploy and Deploy Success means the dashboard sees a hanging Deploy with no closure. The legacy CLI had the same issue AFAIK, and the generic Error event still fires from main.rs, so this is minor — but a matching failure event would make the funnel query cleaner.

  • [Nit] platform()/arch() mapping to Node vocab (telemetry.rs:119-134) covers macos/windows/x86_64/aarch64 but not arm/i686/freebsd/etc — anything else falls through as its Rust name. Fine as long as the pipeline is tolerant.

  • [Nit] sentFrom: "backend" (telemetry.rs:46) is inherited from the JS CLI where it meant "server-side, not browser". For a native Rust CLI it still fits the schema but reads oddly; consider a comment pinning why. Or introduce sentFrom: "cli-rust" if the pipeline allows a new value — makes it possible to split legacy vs. native usage in dashboards.

Prior findings — status against current tree (all still open, none blocking)

  • github subcommands: installation shape inconsistent (positional on Repos, --installation on Branches/Connect).
  • parse_data precedence inconsistent between github connect (data wins) and api-keys create (flags win) — pick one convention.
  • oauth.rs:133-141 retry swallows transport errors until expires_in.
  • client.rs HTML-response guard message fires on path typos too — wording nit.
  • deployments advance-step's step: String is free-form; ValueEnum would give tab-completion.
  • deploy.rs collect_files follows symlinks (no cycle guard); holds all changed-file bytes in memory concurrently.
  • update.rs spawn_check hits GitHub even when stderr isn't a TTY — 60/hr unauthenticated GitHub rate limit per IP burns quickly in scripted use.

Housekeeping

  • The two new telemetry unit tests (timestamp_is_iso8601, ids_are_stable_length_and_distinct) are welcome — coverage in this crate is still very thin (this + the newer_than test from the previous commit), but each addition helps.
  • PR title/body still describe only the ai-engineer removal, but the branch now contains ten distinct changes. Consider expanding the description so reviewers know to look at everything, or splitting into follow-up PRs.
• Branch: `claude/cube-rust-cli-tool-oiv9br`

Cube Cloud API keys authenticate as `Authorization: Api-Key <key>`
rather than Bearer. The client now picks the scheme per credential:
three-segment JWTs (OAuth access tokens, legacy deploy JWTs) send
Bearer, opaque keys send Api-Key; CUBE_AUTH_SCHEME=bearer|api-key
overrides the heuristic. `cube deploy` gains -m/--message, forwarded
as commitMessage on upload/finish.

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

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 6s —— View job


Code Review — commit a3119b0

Verdict: LGTM. 0 high, 0 medium, 2 low. Prior findings from earlier rounds still apply, none blocking.

Details
  • Diff a3119b0 against previously-reviewed tip (de3da7b)
  • Read client.rs auth-scheme helper end-to-end
  • Verify --message wiring in commands/deploy.rs
  • Re-check prior findings against current tree

What's new in a3119b0

Two small, independent changes:

  1. Client::authorization() picks Bearer vs Api-Key per credential (rust/cube-cli/src/client.rs:77-89, used in both send_once at line 100-103 and post_multipart at line 161). Heuristic: a token with three dot-separated segments is treated as a JWT (OAuth access token or legacy deploy JWT) and gets Bearer; anything else is treated as a Cube Cloud API key and gets Api-Key. CUBE_AUTH_SCHEME=bearer|api-key is the escape hatch. Docstring explains the reasoning clearly.

  2. cube deploy --message <msg> / -m <msg> (rust/cube-cli/src/commands/deploy.rs:29-30, 155) — attaches a commitMessage on the upload/finish body. util::set skips the key when None, so no wire change for the default path.

Observations (low)

  • Auth-scheme heuristic is by dot count, not shape (client.rs:82-86). Two segments-or-not-three sends Api-Key; three sends Bearer. This is fine for the two real credential shapes today, but nothing enforces that the JWT-shaped branch is actually base64url in each segment. If Cube Cloud ever issues API keys with two dots (e.g. <region>.<tenant>.<opaque>), they'd silently be sent as Bearer. Not a blocker — the CUBE_AUTH_SCHEME override is the escape valve — but a one-liner like "each segment must be non-empty base64url" would harden the check without cost. Alternatively, drop the heuristic and set the scheme explicitly at login time (store it alongside the token in the credentials file), which is more robust long-term.

  • CUBE_AUTH_SCHEME env parsing is case-sensitive-ish (client.rs:82-84). Matches bearer/Bearer and api-key/Api-Key, but not BEARER / API-KEY / apikey. eq_ignore_ascii_case on a lowercased read would cover the whole surface without a match tree. Trivial.

  • Nit — --message accepts arbitrary length. Not a bug (the server owns limits), but if the deploy pipeline caps commit messages at some N, a client-side check with a clearer error would beat a 400. Cheap to add if the limit is known.

Prior findings — status against current tree (all still open, none blocking)

  • Telemetry Error event still ships the full anyhow chain (main.rs:230, format!("{err:#}")), which includes local paths from .with_context() wrappers — README privacy wording needs to match, or the payload should be redacted.
  • update::spawn_check still hits api.github.com even when stderr isn't a TTY (update.rs:99-122) — burns unauthenticated GitHub rate limit on scripted runs.
  • github subcommands: installation is positional on Repos, --installation on Branches/Connect.
  • parse_data precedence: github connect lets data override typed args, api-keys create lets typed args win — pick one convention.
  • oauth.rs:133-141 retry swallows transport errors until expires_in.
  • client.rs HTML guard message fires on genuine path typos as well; wording nit.
  • deployments advance-step's step: String is free-form; ValueEnum would give tab-completion.
  • deploy.rs collect_files follows symlinks (no cycle guard); holds all changed-file bytes in memory concurrently.

Housekeeping

  • No new tests. CLI command layer remains uncovered (pre-existing gap, not regressed).
  • PR title/body still describe only the ai-engineer removal, but the branch now contains eleven distinct changes (removal + github commands + web-app-response guard + SIGPIPE/field-names + api-contract alignment + creation-step + deploy/upload + api-keys + install/update + telemetry + auth-scheme/commit-message). Consider expanding the description so reviewers know to look at everything, or splitting into follow-up PRs.

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

@paveltiunov paveltiunov changed the title refactor(cube-cli): remove ai-engineer command feat(cube-cli): GitHub import, upload deploy, install/self-update, telemetry Jul 20, 2026
`cube logs <deployment> [--pod] [--container]` tails the deployment's
pod logs via GET /api/v1/deployments/:id/logs (same source as the UI
logs page; cube-store pods excluded server-side). Renders as
`date [pod] message` lines, raw with --json.

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

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — commit b02c984

Verdict: LGTM. 0 high, 0 medium, 3 low. Prior findings from earlier rounds still apply, none blocking.

Details
  • Diff b02c984 against previously-reviewed tip (a3119b0)
  • Read commands/logs.rs end to end
  • Verify wiring in main.rs and commands/mod.rs
  • Re-check prior findings against current tree

What's new in b02c984

Adds cube logs <deployment> (rust/cube-cli/src/commands/logs.rs, 52 lines) plus wiring in main.rs:126-127, 218, 306 and commands/mod.rs:19. GETs /api/v1/deployments/{id}/logs with optional --pod and --container query params; prints date [pod] message in table mode or the raw payload in --json mode. Follows the existing resource-command shape.

Observations (low)

  • Doc-comment says "tail" but the command doesn't stream (logs.rs:3-4, main.rs:126). This is a single GET — users coming from kubectl logs -f / docker logs -f will expect -f/--follow. Either reword the docstring to "Fetch recent log lines for a deployment" or add a --follow mode (poll with a since cursor / SSE if the endpoint supports it). Wording fix is the trivially-cheap version.

  • No --since / --limit / --tail bounds (logs.rs:8-17). Whatever the server's default window is, the CLI can't narrow or widen it. Non-blocking — obvious follow-up once the endpoint contract is stable — but worth noting since typical log-fetch UX assumes at least a line-count knob.

  • container is accepted but its effect isn't documented (logs.rs:15-17). If the server ignores it when --pod isn't set, or if only a fixed set of container names is valid (cubejs-server, worker, etc.), a --help hint or ValueEnum would spare users a round-trip. Same class of nit as the still-open deployments advance-step finding.

  • Nit — format!("[{pod}]").cyan() (logs.rs:47) — allocates a String per line just to color the brackets. Fine at CLI-log scale; noting only for symmetry with the other tight loops in deploy.rs.

Prior findings — status against current tree (all still open, none blocking)

  • Telemetry Error event still ships the full anyhow chain incl. local paths from .with_context() — README privacy wording needs to match, or the payload should be redacted.
  • update::spawn_check still hits api.github.com on every invocation even when stderr isn't a TTY.
  • github subcommands: installation positional on Repos, --installation on Branches/Connect.
  • parse_data precedence inconsistent across github connect (data wins) vs. api-keys create (flags win).
  • oauth.rs:133-141 retry swallows transport errors until expires_in.
  • client.rs HTML-response guard message fires on path typos too; wording nit.
  • deploy.rs collect_files follows symlinks (no cycle guard); holds all changed-file bytes in memory concurrently.
  • Auth-scheme heuristic in client.rs is by dot count only — a two-dot API key would silently be sent as Bearer.

Housekeeping

  • No new tests for logs; CLI command layer remains uncovered (pre-existing gap, not regressed).
  • PR title/body now describe the full batch — good. The branch contains twelve distinct changes; consider splitting into follow-up PRs if any land contentious in code review.

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

The server now defaults to the cubejs-server (Cube API) container;
document the known containers in --help and add the -c short form.

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

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — commit 0edf5e2

Verdict: LGTM. 0 high, 0 medium, 1 low. Prior findings from earlier rounds still apply, none blocking.

Details
  • Diff 0edf5e2 against previously-reviewed tip (b02c984)
  • Read commands/logs.rs and README update
  • List existing review threads (none present)
  • Re-check prior findings against current tree

What's new in 0edf5e2

Two tiny follow-ups to the logs command:

  1. --container gains a -c short flag and its docstring now enumerates the common container names (cubejs-server default, vector, api-proxy, …) — directly addresses the "container effect not documented" nit from the previous round.
  2. README coverage table gains a logs row (previously missing) describing the flag surface.

Both changes are pure additions; no behavior change beyond a new CLI alias.

Observations (low)

  • Docstring lists container names without a ValueEnum (logs.rs:15-17). Same trade-off as deployments advance-step: string arg gives the server freedom to add containers without a CLI release, but if the enumerated names drift, --help will silently lie. A #[derive(clap::ValueEnum)] for the known set + a --container-name freeform escape hatch would be the belt-and-braces version. Non-blocking; noting for symmetry.

  • Nit — logs doc-comment still says "tail" (logs.rs:3-4 and README:131) but the command remains a single GET, not a streaming follower. Users used to kubectl logs -f will hit this expectation gap. Either drop "tail" from the wording ("fetch recent log lines") or add a --follow/-f streaming mode later. Same nit as before; the README change just re-exposes it.

Prior findings — status against current tree (all still open, none blocking)

  • Telemetry Error event ships full anyhow chain incl. local paths from .with_context() — README privacy wording should match, or payload should be redacted (main.rs:230).
  • update::spawn_check hits api.github.com even when stderr isn't a TTY — burns unauthenticated GitHub rate limit on scripted runs.
  • github subcommands: installation positional on Repos, --installation on Branches/Connect.
  • parse_data precedence inconsistent between github connect (data wins) and api-keys create (flags win).
  • oauth.rs:133-141 retry swallows transport errors until expires_in.
  • client.rs HTML-response guard message fires on genuine path typos as well; wording nit.
  • deploy.rs collect_files follows symlinks (no cycle guard); holds all changed-file bytes in memory concurrently.
  • Auth-scheme heuristic in client.rs uses dot count only — a two-dot API key would silently be sent as Bearer.
  • deployments advance-step's step: String is free-form; ValueEnum would give tab-completion.

Housekeeping

  • No new tests for logs; CLI command layer remains uncovered (pre-existing gap, not regressed).
  • PR title/body now describe the full batch (twelve distinct changes) — good.

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

…orker)

Forward the logs endpoint's new ?source= param: production (default,
pod-based) or dev (dev-mode worker logs; --pod/--container ignored).
Verified on staging: source=dev surfaced the real crash-loop error of a
deployment whose production pods never scheduled.

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

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — commit a6ab60b

Verdict: LGTM. 0 high, 0 medium, 2 low. Prior findings from earlier rounds still apply, none blocking.

Details
  • Diff a6ab60b against previously-reviewed tip (0edf5e2)
  • Read commands/logs.rs
  • List existing review threads (none present)
  • Re-check prior findings against current tree

What's new in a6ab60b

Single small addition to cube logs: a --source flag (rust/cube-cli/src/commands/logs.rs:18-21, 29) constrained to production (default, existing pod-based behavior) or dev (dev-mode worker; ignores --pod/--container). Uses clap's value_parser for a fixed set — nice — so typos fail locally with a clean message.

Observations (low)

  • source: Option<String> with a hardcoded value_parser list would be cleaner as a ValueEnum (logs.rs:20). Same trade-off as the still-open deployments advance-step finding, but here the set is intentionally closed (there's no server-side extension point), so a #[derive(clap::ValueEnum)] enum Source { Production, Dev } would be strictly better — type-safe, no string comparisons, and --help renders the values just as well. Non-blocking; both approaches produce the same UX.

  • Nit — docstring says --source dev "ignores --pod/--container" (logs.rs:18-19), but the code sends all three unconditionally when set (logs.rs:27-29). The server presumably ignores them for source=dev, but nothing here validates or warns if a user passes --source dev --pod foo. A cheap client-side check + warning would spare a silent no-op. Alternatively, drop the docstring claim and let the server behavior speak for itself.

Prior findings — status against current tree (all still open, none blocking)

  • Telemetry Error event ships full anyhow chain incl. local paths from .with_context() — README privacy wording should match, or payload should be redacted (main.rs:230).
  • update::spawn_check hits api.github.com even when stderr isn't a TTY — burns unauthenticated GitHub rate limit on scripted runs.
  • github subcommands: installation positional on Repos, --installation on Branches/Connect.
  • parse_data precedence inconsistent between github connect (data wins) and api-keys create (flags win).
  • oauth.rs:133-141 retry swallows transport errors until expires_in.
  • client.rs HTML-response guard message fires on genuine path typos as well; wording nit.
  • deploy.rs collect_files follows symlinks (no cycle guard); holds all changed-file bytes in memory concurrently.
  • Auth-scheme heuristic in client.rs uses dot count only — a two-dot API key would silently be sent as Bearer.
  • deployments advance-step's step: String is free-form; ValueEnum would give tab-completion.
  • logs command docstring still says "tail" but is a single GET, not streaming.

Housekeeping

  • No new tests; CLI command layer remains uncovered (pre-existing gap, not regressed).
  • PR title/body describe the full batch — good.

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

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — commit fb5aba0

Verdict: LGTM. 0 high, 0 medium, 1 low. Prior findings from earlier rounds still apply, none blocking.

Details
  • Diff fb5aba0 against previously-reviewed tip (a6ab60b)
  • Verify the rename covers every reference (README, workflow, script self-references)
  • List existing review threads (none present)
  • Re-check prior findings against current tree

What's new in fb5aba0

Pure relocation: rust/cube-cli/install/install.{sh,ps1} → repo-root install-cli.{sh,ps1}. Trims the one-liner URL. Three consequential edits ride along:

  1. .github/workflows/cube-cli.ymlpaths: triggers now also watch install-cli.sh / install-cli.ps1 at the repo root so edits to the scripts still exercise CI.
  2. Both scripts' self-referencing docstring URLs updated to the new paths.
  3. rust/cube-cli/README.md — install snippets updated.

No behavior change; the scripts themselves are byte-for-byte identical apart from the header URL.

Observations (low)

  • [Low] Root-level install scripts break the tidy monorepo boundary. The rest of the Rust CLI lives under rust/cube-cli/; putting install-cli.{sh,ps1} at the repo root gives them top-level visibility that pulls the eye when browsing this large monorepo (which is otherwise organized as packages/, rust/, docs-mintlify/, etc.). Trade-off is genuine — a shorter install URL is a real UX win for a curl-piped-to-sh entry point, and adding a /install/ prefix (.../master/install/cube-cli.sh) would recover the tidiness at the cost of a few chars. Non-blocking; the current choice is defensible. If you keep them at root, a one-line comment at the top of each script pointing readers to rust/cube-cli/README.md for the full docs would help drive-by explorers back to the right place.

  • [Nit] No .gitattributes LF pin for install-cli.sh. On a Windows checkout with core.autocrlf=true, install-cli.sh would be materialized with CRLF endings — a sh running that script over stdin (via curl | sh) is fine because the download comes from the raw GitHub URL, not the local checkout, so this is more theoretical than practical. But if someone edits the script on Windows and pushes back, an inadvertent CRLF flip would break the shebang for downstream sh. A install-cli.sh text eol=lf in .gitattributes is a cheap belt.

Prior findings — status against current tree (all still open, none blocking)

  • Telemetry Error event ships full anyhow chain incl. local paths from .with_context() — README privacy wording should match, or payload should be redacted (rust/cube-cli/src/main.rs:230).
  • update::spawn_check hits api.github.com even when stderr isn't a TTY — burns unauthenticated GitHub rate limit on scripted runs.
  • github subcommands: installation positional on Repos, --installation on Branches/Connect.
  • parse_data precedence inconsistent between github connect (data wins) and api-keys create (flags win).
  • oauth.rs:133-141 retry swallows transport errors until expires_in.
  • client.rs HTML-response guard message fires on genuine path typos as well; wording nit.
  • deploy.rs collect_files follows symlinks (no cycle guard); holds all changed-file bytes in memory concurrently.
  • Auth-scheme heuristic in client.rs uses dot count only — a two-dot API key would silently be sent as Bearer.
  • deployments advance-step's step: String is free-form; ValueEnum would give tab-completion.
  • logs command docstring still says "tail" but is a single GET, not streaming.

Housekeeping

  • No new tests; CLI command layer remains uncovered (pre-existing gap, not regressed).
  • PR title/body describe the full batch — good.

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

@paveltiunov
paveltiunov merged commit 11c67c4 into master Jul 21, 2026
16 checks passed
@paveltiunov
paveltiunov deleted the claude/cube-rust-cli-tool-oiv9br branch July 21, 2026 01:26
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