Skip to content

feat(cli): run agent evals (CUB-3667) - #11953

Open
MikeNitsenko wants to merge 8 commits into
masterfrom
codex/cub-3667-eval-cli
Open

MikeNitsenko wants to merge 8 commits into
masterfrom
codex/cub-3667-eval-cli

Conversation

@MikeNitsenko

@MikeNitsenko MikeNitsenko commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add cube evals run, status, and results
  • support CI-friendly --wait behavior that fails closed unless every graded question passes
  • document CLI usage and a GitHub Actions example

Depends on the public API from cubedevinc/cubejs-enterprise#15265.

Verification

  • cargo +1.96.0 fmt --all --check
  • cargo +1.96.0 clippy --all-targets -- -D warnings
  • cargo +1.96.0 test --locked (103 passed)

@mintlify

mintlify Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
cubed3 🟢 Ready View Preview Sep 22, 2026, 8:28 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

@github-actions github-actions Bot added docs Issues that require a documentation improvement rust Pull requests that update Rust code pr:community Contribution from Cube.js community members. labels Sep 21, 2026
@claude

claude Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 2m 41s —— View job


Review complete — 1 high, 2 medium, 3 low, posted inline

Details
  • Read changed files
  • Compare against sibling CLI commands (dbt.rs, deployments.rs, util.rs, wait.rs, output.rs)
  • Check prior review threads
  • Post findings

Findings

Sev Where Concern
High rust/cube-cli/src/commands/evals.rs:191 The results GET sends no paging params, and passing_results takes passed and total from that one page — an all-pass first page exits 0 while later failing questions are never read. Fail-open in the exact CI-gate case --wait was written for.
Medium rust/cube-cli/src/commands/evals.rs:216 run --wait never prints the evaluation id before waiting, so a timeout tells the reader to "re-run the status command" with an id that appears nowhere in the log. dbt sync --wait prints it to stderr (dbt.rs:902).
Medium rust/cube-cli/src/commands/evals.rs:112 Terminal states are only completed/failed; a cancelled or errored run is read as progress and polled for the full timeout. deployments.rs:21 carries ["failed", "cancelled"].
Low rust/cube-cli/src/commands/evals.rs:75 Local nonempty_option duplicates the util.rs guard family with a message that drops what the empty value would do; util::nonempty_path drops in for --file.
Low rust/cube-cli/src/commands/evals.rs:33 --file help says "relative to agents/" while both docs pages pass eval_questions/revenue.yml.
Low rust/cube-cli/src/commands/evals.rs:125 result_items is a one-line wrapper with a single caller.

What holds up well

The fail-closed shape of ensure_passed is right: an empty result set and a non-pass verdict both bail, and the reason is asserted in empty_or_non_passing_suites_fail_closed. print_completed runs before ensure_passed, so --json emits the document on the failing path too — easy to get backwards. The --branch guard is registered in the only_the_listed_branch_arguments_refuse_an_empty_value partition rather than just added, and the comment there says why. Status comparison goes through util::status_of, so a padded state still ends the poll.

Review threads

The listing returned 0 threads of mine — no prior threads, none resolved, none re-affirmed. All six findings posted fresh.

Not verified

Did not run cargo test / clippy — the build command needed approval in this environment, so the PR's own reported run (102 passed) is the only evidence here. The high finding also depends on the response shape of the /evaluations/{id}/results endpoint in cubedevinc/cubejs-enterprise#15265, which is not visible from this repo; it's stated as something to confirm rather than as a reproduced failure.

· branch `codex/cub-3667-eval-cli`

Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread rust/cube-cli/src/commands/evals.rs Outdated
@MikeNitsenko
MikeNitsenko force-pushed the codex/cub-3667-eval-cli branch from 076b401 to 96e41b7 Compare September 21, 2026 21:40
@claude

claude Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 4m 13s —— View job


Review: 1 high, 2 medium, 3 low — --wait can't grade a suite past the first results page

Full review
  • Read changed files
  • Review the Rust CLI code
  • Review the docs
  • Check prior review threads
  • Post findings inline

Findings

# Severity Where What
1 High rust/cube-cli/src/commands/evals.rs:144 ensure_passed bails on pageInfo.hasNextPage, and the results GET never pages. Any suite bigger than the server's default page fails the gate while green, with no --first anywhere to widen it.
2 Medium rust/cube-cli/src/commands/evals.rs:252-260 evals results silently prints a truncated page — no --first/--after, no hasNextPage signal. Every other list command in the CLI carries the pair.
3 Medium rust/cube-cli/src/commands/evals.rs:33-35 --file borrows util::nonempty_path, whose refusal advises - for stdin. The path is server-side; - parses and ships verbatim as questionFile.
4 Low docs-mintlify/reference/cli.mdx:348-357, :181 "evaluation" / EVALUATION_ID against docs-mintlify/CLAUDE.md's Evals terminology rule. The CLI's own help and error strings have it throughout too.
5 Low rust/cube-cli/src/commands/evals.rs:104-113 Only completed/failed are terminal, with no note on whether that set is exhaustive. A cancelled run would burn the whole --timeout. deployments.rs:11-21 and dbt.rs:664 both document their sets.
6 Low rust/cube-cli/src/commands/evals.rs:305-330 No test reaches the FAILED arm of ensure_passed, nor proves the status lowercasing at :104/:141.

What's good

The fail-closed design is right and unusually well thought through: total == 0 is a failure rather than a vacuous pass, an unread page refuses to be graded rather than grading what it has, requires = "wait" keeps --timeout/--poll from looking honoured when they aren't, and --branch is registered in util.rs's refuse-an-empty-value list with a comment naming the exact CI case. Progress goes to stderr so --json stays one document. Reusing wait::poll gets transient-failure absorption and the deadline-bounded attempt for free. The module also stays refreshingly light on comments for this codebase — nothing to flag under the comment rules.

Not verified

Couldn't run cargo test / clippy — the sandbox declined the invocations, so the PR body's "102 passed" is unconfirmed from here. Findings 1 and 5 hinge on the enterprise API's behaviour (default results page size, and whether an eval run has terminal states beyond completed/failed), which isn't in this repo; both are framed so the answer decides the fix.

Review threads

The listing returned 0 threads of mine — no prior rounds on this PR. Nothing resolved, nothing re-affirmed, no duplicates to suppress.

· branch [`codex/cub-3667-eval-cli`](https://github.com/cube-js/cube/tree/codex/cub-3667-eval-cli)

Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread docs-mintlify/reference/cli.mdx Outdated
Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread rust/cube-cli/src/commands/evals.rs
@MikeNitsenko
MikeNitsenko force-pushed the codex/cub-3667-eval-cli branch from 96e41b7 to 551fa64 Compare September 21, 2026 21:48
@claude

claude Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 2m 42s —— View job


1 medium, 4 low — the fail-closed grading logic is sound; the medium is that --wait prints a green summary (and a partial --json document) before the incomplete-page guard bails. No prior review threads on this PR.

Full review
  • Gather context (diff, changed files, CLI conventions in wait.rs/output.rs/dbt.rs)
  • Review rust/cube-cli/src/commands/evals.rs
  • Review docs changes
  • Reconcile prior review threads
  • Post findings

Findings

# Severity Location Issue
1 Medium rust/cube-cli/src/commands/evals.rs:193 print_completed runs before ensure_passed, so a truncated results page prints Eval run 42: 20/20 passed — and a partial --json document — before the hasNextPage bail
2 Low rust/cube-cli/src/commands/evals.rs:141 questionTextSnapshot (and status's branchName/status) go into output::table unbounded and without control-char stripping; dbt.rs:578-595 has printable/one_cell for exactly this
3 Low rust/cube-cli/src/commands/evals.rs:116 Poll label is the raw status string; every other waiter bounds it with util::one_line(.., COMPLAINT_LIMIT)
4 Low rust/cube-cli/src/commands/evals.rs:147 ensure_passed re-derives the run id from the payload, so a payload missing id yields eval run failed; the caller already holds it
5 Low rust/cube-cli/src/commands/evals.rs:14 The terminal-status comment sits on Args' doc comment instead of on the consts it explains

What holds up well

  • The gate really does fail closed: failed, zero results, and any non-pass verdict are all separate refusals (evals.rs:146-165), and the hasNextPage guard covers the one shape that could silently grade a subset.
  • run --wait keeps stdout a single JSON document by sending the started-run line to stderr (evals.rs:216), matching wait::poll's stated contract.
  • --branch carries util::nonempty, and the addition to the refuses list in util.rs:776-781 states why — an unset $GITHUB_HEAD_REF must not become a server-picked branch.
  • Unknown statuses stay progress rather than being guessed at as terminal, consistent with status_of's documented rule.
  • Docs cover the CI shape (docs-mintlify/admin/ai/evals.mdx) and the command table (docs-mintlify/reference/cli.mdx); the exit-code contract is stated in both.

Review threads

The thread listing for this PR returned 0 threads of mine (empty, single page), so nothing to resolve and no finding suppressed as a duplicate. The five review comments visible in the PR description context are MikeNitsenko's own, which I do not touch.

Not verified

cargo test / cargo clippy are outside this run's allowed tools, so I could not re-run the suite — every finding above is from reading the code. The PR reports 102 passing.

· branch [`codex/cub-3667-eval-cli`](https://github.com/cube-js/cube/tree/codex/cub-3667-eval-cli)

Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread rust/cube-cli/src/commands/evals.rs Outdated
Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread docs-mintlify/admin/ai/evals.mdx Outdated
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 3m 1s —— View job


Reviewed — no blockers. 0 high, 1 medium, 2 low; 3 inline comments posted.

Review details
  • Gather context (diff, changed files)
  • Review rust/cube-cli/src/commands/evals.rs
  • Review util.rs, dbt.rs, wait.rs, docs
  • Handle prior review threads
  • Post findings

Findings

# Severity Where Concern
1 Medium docs-mintlify/admin/ai/evals.mdx:154 A review verdict is by design for a question with no ground truth (verdict table, evals.mdx:298), and ensure_passed requires every verdict to be pass — so such a suite fails the CI gate on every PR forever. The CI section doesn't warn about it.
2 Low rust/cube-cli/src/commands/evals.rs:158 The four completeness bails abort before any output and name no next command; cube evals results <deployment> <run> is the recovery and doesn't apply the check. Same for the with_context at evals.rs:194. dbt.rs:554 sets the "reason + command to run" convention.
3 Low rust/cube-cli/src/commands/evals.rs:304 --after's cursor is never printed on the human path, and the table gives no sign a next page exists, so --first output can't be paged without re-running under --json.

What looks right

  • Fail-closed gating is thorough: failed status, zero results, any non-pass verdict, missing/non-boolean/true hasNextPage, and a non-array items are each a distinct refusal with its own test (evals.rs:363-433).
  • --wait writes the started-run line to stderr, keeping stdout a single JSON document.
  • printable/CELL_LIMIT/one_cell moved into util.rs rather than duplicated, and both dbt and evals now share one terminal-safety budget; the ESC-injection test travelled with it (util.rs:441).
  • nonempty on --branch is registered in the exhaustive parser test, so an unset GITHUB_HEAD_REF can't silently become a server-chosen branch.
  • The bounded results-fetch retry (RESULTS_FETCH_TIMEOUT, advising_nothing()) correctly avoids advising --timeout, a flag that can't move that deadline.

Notes

  • cargo check/cargo test are not permitted in this sandbox, so compilation and tests were not re-run here; the PR body reports cargo +1.96.0 fmt --check, clippy -D warnings, and 103 passing tests.
  • Review threads: the listing returned 0 threads authored by me — no prior threads, none resolved, none re-affirmed.
· branch `codex/cub-3667-eval-cli`

Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread docs-mintlify/admin/ai/evals.mdx
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 4m 37s —— View job


Reviewed — no blockers. 3 findings posted inline (1 medium docs, 2 low).

Review detail
  • Read the diff and the changed files in full
  • Analyze CI-gate semantics, pagination handling, terminal safety, docs accuracy
  • Reconcile prior review threads
  • Post findings

Findings

# Where Severity Issue
1 docs-mintlify/admin/ai/evals.mdx:230-236 medium The Platform API jq gate compares .evalRun.status == "completed" and .verdict == "pass" case-sensitively, while the poll loop eight lines above downcases and the CLI is case-insensitive throughout. The prose claims parity with the CLI. Fails closed, but a COMPLETED/PASS response would fail a branch the CLI passes.
2 rust/cube-cli/src/commands/evals.rs:262-270 low RESULTS_FETCH_TIMEOUT opens a fresh 30s budget the user's --timeout cannot reach, so --wait --timeout 30m can return at 30m30s. Passing the remaining budget through keeps the recovery window inside the flag.
3 rust/cube-cli/src/commands/evals.rs:169-175 low The endCursor-missing branch of next_page_hint has no test; the !cursor.is_empty() guard — the only thing preventing a suggested --after '' that re-fetches page one — could be deleted with the suite green.

What holds up well

The CI gate is genuinely fail-closed: ensure_complete_results (evals.rs:184) rejects a missing items array, hasNextPage: true, a missing pageInfo.hasNextPage, and a non-boolean one with four distinguishable messages, and it runs before print_completed, so no partial 20/20 passed summary or partial JSON document is ever emitted. ensure_passed requires a non-empty suite and every verdict to pass, so a review verdict or an empty file fails rather than passing vacuously.

Moving printable / CELL_LIMIT / one_cell from dbt.rs into util.rs is the right call — evals needed exactly the same ESC-stripping and width bound, and the poll label at evals.rs:125 goes through it too, so server-supplied status text can't retitle a CI log. The dbt call sites were updated wholesale with no behavior change.

--branch on util::nonempty with the exhaustive parser test entry (util.rs:833) is the right guard for a CI gate: an unset github.head_ref becomes a parse error rather than a run against a server-chosen branch. --timeout/--poll both requires = "wait", and the no-wait forms are covered in the same test.

Notes

  • Not verified by running. cargo test / cargo clippy need network approval that this run does not have, so the PR's own reported 103 passed is the only evidence the suite is green. The new tests read as correct on inspection.
  • A finding I drafted about evals results --first/--after being undocumented was withdrawndocs-mintlify/reference/cli.mdx:181 already lists them in the command table.
  • --after with no validator matches the repo-wide convention (every other after: Option<String> across commands/ is declared the same way), so it is not raised.

Review threads

The listing returned 0 of my own threads (paged to the end; no unresolved threads of any author on this PR). 0 resolved, 0 re-affirmed. All three findings above are new.

· branch `codex/cub-3667-eval-cli`

Comment thread docs-mintlify/admin/ai/evals.mdx
Comment thread rust/cube-cli/src/commands/evals.rs
Comment thread rust/cube-cli/src/commands/evals.rs
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 2m 43s —— View job


1 medium finding — zero-length results-fetch budget when a run finishes at the --timeout boundary. Everything else in this round looks sound.

Review details
  • Gather context (changed files, PR history)
  • Review rust/cube-cli/src/commands/evals.rs
  • Review util.rs / dbt.rs cell-helper extraction
  • Review docs (admin/ai/evals.mdx, reference/cli.mdx)
  • Handle prior review threads
  • Post findings

Finding

evals.rs:264, helper at evals.rs:21 — medium. results_fetch_timeout has no floor, so when wait_for_run returns Done on its last look at the deadline (wait.rs:166 guarantees that last look), fetch_complete_results gets Duration::ZERO and timeout_at expires before the GET resolves. A completed, all-passing run then exits non-zero with timed out after 0s waiting for eval results, never naming the run's outcome. Posted inline with a clamp to a small floor; note that results_fetch_stays_within_the_user_timeout (evals.rs:520) currently pins the zero case.

Checked and not flagged

  • Completeness guard before printing (ensure_complete_results) — the four branches (malformed items, missing, non-boolean, hasNextPage: true) are exhaustive and fail closed, and the ordering matches the intent stated in earlier rounds.
  • --after has no nonempty guard, but neither does any other list command (users, reports, folders, variables, regions, groups, workspace), so evals is consistent with the CLI's existing convention.
  • one_cell/printable/CELL_LIMIT extraction into util.rs is a clean move; dbt.rs call sites and the terminal-safety test moved with it.
  • install-cli.sh reads CUBE_VERSION (install-cli.sh:9), so the docs recipe's env var name is correct, and test -n "$CUBE_VERSION" fails the job when the repo variable is unset.
  • Docs terminology matches docs-mintlify/CLAUDE.md ("Evals", "eval run"); the Platform API recipe's jq checks mirror the CLI's gate, including pageInfo.hasNextPage == false and the array/length guards.
  • cargo test could not be run in this environment (cargo invocation not permitted here), so the finding is from reading evals.rs and wait.rs rather than from a reproduction.

Review threads

The listing returned 0 threads of mine on this PR — no prior threads, so none resolved and none re-affirmed.

· branch `codex/cub-3667-eval-cli`

Comment thread rust/cube-cli/src/commands/evals.rs
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 2m 47s —— View job


No blocking findings — 0 high, 0 medium, 1 low (non-blocking note)

Review details
  • Gather context (diff, changed files)
  • Review rust/cube-cli/src/commands/evals.rs
  • Review util.rs / dbt.rs refactor
  • Review docs changes
  • Handle prior review threads
  • Post findings

Review threads

The listing returned 0 unresolved threads (0 of mine); 0 resolved, 0 re-affirmed. Nothing to dedupe.

What I verified

Fail-closed gateensure_passed (rust/cube-cli/src/commands/evals.rs:231) and ensure_complete_results (:193) cover every exit: failed status, zero results, any non-pass verdict, hasNextPage: true, a missing or non-boolean hasNextPage, and a non-array items. wait_for_run (:124) only ever hands back completed/failed, so ensure_passed's implicit "not failed ⇒ completed" holds.

Envelope consistencyensure_complete_results asserts results["items"] is an array, and output::items (rust/cube-cli/src/output.rs:24) prefers items over the deprecated data, so the array the completeness check validates is the same one passing_results grades. No items/data split.

Timeout budgetresults_fetch_timeout (:24) is clamp(remaining, 5s, 30s). Total wall clock exceeds --timeout only when remaining < 5s, i.e. by at most the documented five seconds; RESULTS_FETCH_FLOOR < RESULTS_FETCH_TIMEOUT so the clamp cannot panic. The Instant starts before wait_for_run, so the elapsed figure is the real one. Tests at :515 pin all four boundaries including elapsed == timeout.

Terminal safety — every server-supplied cell now goes through util::one_cell (rust/cube-cli/src/util.rs:301), including the Progress::Waiting label at :134 and the status table at :335. The dbt → util move is behaviour-preserving; util.rs:441 keeps the ESC-stripping assertion.

Stream discipline — the started-run notice (:301), progress, and the next-page hint (:360) go to stderr, so --json stdout stays one parseable document. The --after cursor is shell_quoted (:189), and the missing/empty-endCursor fallback is covered at :491.

Arg validation--branch is in the exhaustive nonempty branch-arg list (rust/cube-cli/src/util.rs:833), so an unset ${{ github.head_ref }} can't become a server-selected branch. --timeout/--poll requires = "wait" is tested both ways (:638, :667). --first/--after carry no validator, matching every other paginated command in the CLI (deployments.rs:209, dbt.rs:133).

Docsinstall-cli.sh:29 does read CUBE_VERSION, so the pinned-install recipe works, and the test -n "$CUBE_VERSION" guard stops an unset repo variable from silently falling through to latest. The Platform API jq gate mirrors the CLI's checks (completed + non-empty array + hasNextPage == false + all pass), and the review-verdict caveat is stated.

Low (non-blocking)

docs-mintlify/admin/ai/evals.mdx — the Platform API recipe's shared reads array uses --retry-all-errors, which retries definitively-answered failures (401, 403, 404) four times at two-second intervals before giving up. The CLI deliberately does not: client::is_transient excludes them (see rust/cube-cli/src/wait.rs:14). The gate still fails closed, so this only costs ~8s and a noisier log on a misconfigured key. --retry 4 --retry-delay 2 without --retry-all-errors already covers the transient 5xx/connection cases the comment above it describes.

Not run

cargo test/cargo clippy are blocked in this sandbox (bash approval). The PR body reports cargo +1.96.0 test --locked passing at 103 tests, and this branch adds tests beyond that count; CI is the source of truth here.

· branch `codex/cub-3667-eval-cli`

@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 3m 51s —— View job


Review complete — 0 blocking, 3 low (all docs/help text); Rust logic looks sound

Full review
  • Gather context (diff, changed files)
  • Review rust/cube-cli/src/commands/evals.rs
  • Review util.rs / dbt.rs refactor
  • Review docs changes
  • Build + test — not run, cargo is denied in this sandbox
  • Handle prior review threads
  • Post findings

Inline findings

# Location Concern
1 docs-mintlify/admin/ai/evals.mdx:191 curl --retry doesn't cover connection-level failures (ECONNREFUSED, error 56), so the Platform API recipe is strictly less resilient than the parity claim on line 169 — one TCP reset in a 360-read poll loop reds a healthy gate. The CLI absorbs 5 consecutive TransportErrors (client.rs:101, wait.rs:15).
2 docs-mintlify/admin/ai/evals.mdx:158 "writes eval.json … including on a failed verdict" — a results-fetch timeout or any ensure_complete_results bail leaves a 0-byte eval.json that if-no-files-found: ignore still uploads.
3 rust/cube-cli/src/commands/evals.rs:49 --file help says "within the agents directory" while both doc examples pass eval_questions/revenue.yml; the path anchor is unstated.

What I checked and found clean

  • Fail-closed gate. ensure_complete_resultsprint_completedensure_passed in finish_wait is the right order: a partial page never gets printed as a verdict, while a complete-but-failing suite still prints its table before the non-zero exit. All four completeness branches (missing items array, hasNextPage: true/missing/non-boolean) are distinguishable in the error text and covered by empty_or_non_passing_suites_fail_closed.
  • Timeout arithmetic. results_fetch_timeout clamps timeout - elapsed into [5s, 30s], started is taken before wait_for_run, and fetch_complete_results caps the poll at RESULTS_FETCH_POLL_MAX, so a --poll 1h can't overshoot. The +5s overshoot is documented in both the flag help and reference/cli.mdx.
  • Terminal safety. Every server-supplied cell in the results/status tables and the non-terminal poll label goes through util::one_cell; --json stays lossless via serde_json. The util::printable/CELL_LIMIT/one_cell lift out of dbt.rs is a pure move with call sites updated, and dbt.rs's deliberate whole-message printable path is preserved at dbt.rs:683.
  • Injection. next_page_hint runs the cursor through util::shell_quote, and the hint goes to stderr after an explicit stdout flush, so --json stays one parseable document.
  • Argument hygiene. --branch on nonempty is registered in the exhaustive parser test, and the requires = "wait" coupling for --timeout/--poll is covered for both run and status. --first/--after left unvalidated matches every other paginated command in the CLI, so no finding there.

Not verified

cargo test / cargo clippy are denied here. The PR body reports cargo +1.96.0 test --locked (103 passed) plus fmt and clippy clean; I read the new tests but did not execute them.

Review threads

Listing returned 0 threads (one page, hasNextPage: false) — no prior threads, none resolved, none re-affirmed.

· branch `codex/cub-3667-eval-cli`

Comment thread docs-mintlify/admin/ai/evals.mdx Outdated
Comment thread docs-mintlify/admin/ai/evals.mdx Outdated
Comment thread rust/cube-cli/src/commands/evals.rs Outdated
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MikeNitsenko's task in 4m 6s —— View job


Review complete — no blocking findings (0 high, 0 medium, 1 low observation, not posted inline)

Full review
  • Gather context (diff, changed files)
  • Review rust/cube-cli/src/commands/evals.rs
  • Review util.rs / dbt.rs refactor
  • Review docs (reference/cli.mdx, admin/ai/evals.mdx)
  • Check prior review threads
  • Post findings

Review threads

The listing returned 0 of my own threads (gh list-review-threads cube-js cube 11953 came back with an empty nodes array and hasNextPage: false on the only page). Nothing to resolve, nothing to re-affirm — every thread from the earlier rounds is already settled.

What I checked and found clean

Fail-closed gatingensure_complete_results at rust/cube-cli/src/commands/evals.rs:193 discriminates all four shapes (items non-array, hasNextPage true / missing / non-boolean) with a distinct message each, and ensure_passed at :231 refuses an empty suite and any non-pass verdict. finish_wait at :270 runs the completeness check before printing, so a truncated page can never produce an "N/N passed" line or a partial JSON document. Tests at :409 cover every branch including the Ok paths.

Timeout arithmeticresults_fetch_timeout at :24 is clamp(FLOOR=5s, CAP=30s) over timeout - elapsed; FLOOR < CAP are both consts so the clamp cannot panic. The boundary case (run turns terminal exactly at the deadline → 5s of extra budget) matches the doc claim at docs-mintlify/reference/cli.mdx:357. util::parse_duration rejects zero (rust/cube-cli/src/util.rs:407), so neither wait_for_run nor fetch_complete_results can be driven into a zero-interval busy loop via --poll 0s.

Retry semanticsfetch_complete_results delegates to wait::poll, which retries only client::is_transient errors and caps at 5 consecutive; ensure_complete_results failures are returned straight out, not retried. .advising_nothing() at :221 is the right call given finish_wait supplies recovery context on every outcome.

Terminal safetyutil::one_cell is applied to every server-supplied cell (:159-161, :335-338) and to the poll label at :134, so an ESC sequence in questionTextSnapshot cannot retitle a window or overwrite a CI log. The printable/CELL_LIMIT/one_cell move out of dbt.rs into util.rs is behaviour-preserving — the dbt call sites and its CELL_LIMIT + 1 assertion are updated in step.

Pagination hintnext_page_hint at :168 shell-quotes the cursor via util::shell_quote, preserves --first, falls back to a safe --json pointer when endCursor is empty or absent, and writes to stderr after an explicit io::stdout().flush() so ordering holds under a pipe. --first/--after on evals results match the declaration style used by embed, workbooks, data_model, deployments, github, and environments.

Docs — the CUBE_VERSION env var the install step sets is genuinely honoured by install-cli.sh:29. The evals row in the command table at docs-mintlify/reference/cli.mdx:181 lists --first/--after, consistent with how every other command's flags appear there. The jq gate in the Platform API recipe applies the same four checks as the CLI (status == completed, items is a non-empty array, hasNextPage == false, every verdict pass).

One low observation (not worth an inline comment)

docs-mintlify/admin/ai/evals.mdx:198--retry 4 --retry-delay 2 --retry-all-errors means a permanent read failure (401 from a wrong secret, 404 from a wrong deployment id) retries four times at 2s intervals before the job fails, so a misconfiguration reads in the log like a flaky API. Plain --retry would cover 408/429/5xx without that, at the cost of not retrying a bare connection reset. The prose at :172-176 discloses the tradeoff explicitly, so this is a considered choice rather than an oversight — flagging it only so the next reader knows it was weighed.

Not verified

I could not run the Rust test suite — cargo test required approval in this environment and was denied, so the 103-test result in the PR description is unverified from my side. The evals module's own tests read correct against the code, but I have not executed them.

· branch `codex/cub-3667-eval-cli`

This branch was successfully deployed

1 active deployment
staging - docs-mintlify 16314566 Deployed Sep 22, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Issues that require a documentation improvement pr:community Contribution from Cube.js community members. rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant