feat(cube-cli): list dbt sync history and read one sync's logs - #11625
feat(cube-cli): list dbt sync history and read one sync's logs#11625MikeNitsenko wants to merge 6 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Claude finished @MikeNitsenko's task in 2m 41s —— View job Review complete — looks good to merge; 0 high, 1 medium, 3 low (3 inline comments)Full review
GeneralWell-built change. Things I specifically checked and found correct:
Findings
Nothing here blocks. Finding 1 is the only one I'd want resolved before merge, and a comment saying " Docs
Not verifiedI could not run |
|
Claude finished @MikeNitsenko's task in 2m 47s —— View job Re-review of
|
| # | Prior finding | Status |
|---|---|---|
| 1 | STATUS skipped pick's two-spelling fallback; blank status silently defeated the "nothing recognised" warning |
Fixed. All cells now go through one cell closure (dbt.rs:352), status keeps a single key with the reason stated inline, and — the actual bug — the warning is re-keyed on ID_COLUMN (dbt.rs:771) instead of rows.iter().flatten().all(is_blank), so one filled column no longer suppresses it. a_history_row_fills_every_column pins HISTORY_COLUMNS[ID_COLUMN] == "SYNC JOB ID", so the warning can't drift onto another column. |
| 2 | Raw-entry fallback re-printed timestamp/stage and dropped error red |
Fixed. log_line (dbt.rs:423) returns paint_failure(value.to_string(), error) early, so the prefix is dropped and the red survives. an_entry_whose_text_this_build_cannot_find_is_shown_as_it_arrived asserts both halves, including assert_ne!(line, failed.to_string()) for the colour. |
| 3 | Server log text printed with control bytes intact | Fixed, and correctly scoped. printable (dbt.rs:283) drops every char::is_control except \n/\t — that includes DEL (U+007F) and the C1 range, which is what an 8-bit CSI would need. Applied to message, and to time/stage via one_cell. The comment gets the reason right: one_line only ate control characters that happen to be whitespace, and ESC isn't. |
Also picked up unprompted, and correct: the as u64 saturation nit (1e30 no longer renders as a five-billion-hour duration — value < u64::MAX as f64 guard plus human_duration_ms("1e30")/("inf") assertions), and cell bounding via CELL_LIMIT — a branchName carrying a newline can no longer break the row it sits in, and a 500-char trigger can't push the other columns off screen. server_text_cannot_drive_the_terminal covers both the entry and the cell path.
新建 pass over the current file
Nothing new that blocks. Things I re-checked and found sound:
human_duration_msstill readsdurationMsonly;a_duration_is_the_servers_own_figure_or_nothingstill pins the both-stamps-no-duration trap.failure()is still the single source of both--waitfailure messages, stillshell_quotes the id.util::status_ofis unaffected — still used by the wait paths (dbt.rs:138,483,522,577,673) and bydeployments.rs; swapping the history cell toone_celldidn't orphan it.pick's blank-is-not-an-answer rule survives theone_cellwrapping, sinceone_cellis applied after the key choice, not before it.rows[ID_COLUMN]indexing is safe:history_rowalways returnsHISTORY_COLUMNS.len()cells, and the test holds that.
Nits (no action expected)
dbt.rs:437— the raw-entry fallback printsvalue.to_string()unbounded, while every other server string on the path is capped atCELL_LIMIT. An entry carrying a large payload field floods the terminal. Deliberate ("shown as it arrived") and arguably right for a fallback; noting only for the asymmetry.printablekeeps\ninmessageby design, so a hostile dbt error can still emit lines that look like additional timeline entries. Strictly weaker than the escape channel that was closed, and un-fixable without giving up multi-line compile errors.
Docs
docs-mintlify/reference/cli.mdx is unchanged since my last pass and still accurate. I re-traced the CI snippet: wait::poll progress goes to stderr, the FAILED branch prints wait_json (carrying syncJobId) to stdout before returning the error, so sync.json is a single valid document and jq -r '.syncJobId // empty' resolves; the [ -n "$SYNC_JOB_ID" ] guard covers the timeout case where nothing was written.
Not verified
cargo test, cargo clippy and cargo fmt were declined by the sandbox again — this review is from reading the code. The PR reports them clean; please rely on CI.
|
All four findings addressed in 1dd2bc4.
Cells are now bounded and single-line too, so a value carrying a newline can no longer break the row it sits in. Validation
|
|
Aligned both commands with the shapes the endpoints publish (f26eb2b):
Two behaviours worth flagging for reviewers:
Validation
|
|
Claude finished @MikeNitsenko's task in 2m 18s —— View job Re-review of
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | Low | dbt.rs:83-88 |
--status/--trigger take no nonempty parser, unlike --ref/--branch in the same file — --status "" sends status= rather than being refused (inline) |
| 2 | Low | dbt.rs:83-88 |
The two filter vocabularies differ in case in adjacent help lines, and a wrong case is a silently empty table rather than the loud 400 the pass-through reasoning assumes (inline, same thread) |
| 3 | Nit | dbt.rs:456 |
A line with a timing but no phase renders [340ms], which reads as a phase named 340ms until you see a two-part bracket next to it. Tested and deliberate; noting only because the bracket now means two things. |
Neither low finding blocks. Both are about the same seam — nothing stands between a typed filter value and the query string — and both would be resolved by a sentence of help text as well as by code.
Docs
docs-mintlify/reference/cli.mdx tracks the code change accurately: the command-reference row names the two new flags, the logs-takes-no-paging-flags statement matches the struct, and the CANCELLED note is a genuinely useful reconciliation — history showing CANCELLED while --wait calls the same run a failure is exactly the kind of thing a CI author hits once and puzzles over. One claim I can't verify from this repo: "A page holds at most 100 runs, so a larger --first returns 100 with pageInfo.hasNextPage set." That's server behaviour, not CLI behaviour — nothing here clamps or warns on --first 500, which is fine, but the sentence is only as good as the endpoint's actual cap.
Not verified
cargo test, cargo clippy and cargo fmt were declined by the sandbox again (third time this PR) — this review is from reading the code. The PR reports them clean; please rely on CI.
|
Claude finished @MikeNitsenko's task in 1m 47s —— View job Re-review of
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | Low | util.rs:195 |
nonempty_filter tests s.trim() but returns s, so --status " FAILED" travels padded — and padding is never meaningful in a five-word server vocabulary, unlike in a branch name (inline) |
| 2 | Nit | cli.mdx:385 |
The reflow left one ~118-char line where the section wraps at ~85 (inline) |
Neither blocks.
Docs
Accurate and still small. The case-sensitivity clause lands in the right sentence — right where both vocabularies are enumerated, so the reader sees the claim and the spellings together rather than in separate paragraphs. Same standing caveat as last time: "A page holds at most 100 runs" is server behaviour I can't verify from this repo.
Not verified
cargo test, cargo clippy and cargo fmt were declined by the sandbox again (fourth time this PR) — this review is from reading the code. The PR reports them clean; please rely on CI.
|
Claude finished @MikeNitsenko's task in 1m 45s —— View job Re-review of
|
| # | Prior finding | Status |
|---|---|---|
| 1 | nonempty_filter guards on s.trim() but returns s, so --status " FAILED" travels padded |
Fixed (util.rs:207). Returns s.trim().to_string(), and the doc comment now states the divergence rather than leaving it to be read as drift — naming branch_or_placeholder as the counter-case, which is exactly where the opposite rule is argued. nonempty_filter(" FAILED\n") == "FAILED" pins it, and — better — the same test pins nonempty(" main ") == " main " and nonempty_ref(" main ") == " main " beside it, so the divergence is asserted from both sides. A later "consistency" edit that trims all three now fails on the two that must not. |
| 2 | cli.mdx:385 left one ~118-char line where the section wraps at ~85 |
Fixed. Lines 382–388 now wrap at 79–86, matching the surrounding prose. Content unchanged. |
Checks around the change
- The trim is correctly scoped and correctly ordered. It applies only to
nonempty_filter, whose only two callers are--status/--trigger(dbt.rs:85,89);nonempty/nonempty_refstill return their input byte-for-byte. The trim also lands after the emptiness check, so an all-whitespace value is still the refusal (with the filter-specific message) rather than an emptyOk. - The command-tree walk is unaffected.
only_the_listed_branch_arguments_refuse_an_empty_valuefilters onid.contains("branch") || id == "ref" || id.ends_with("_ref") || id.starts_with("ref_")(util.rs:677-680), sostatus/triggerare outside it; neither therefusesnor theacceptslist needed an entry, and theEMPTY_VALUE_REFUSEDshared-prefix assertion from the previous commit still holds for all three helpers. --afteris untouched, and that's right. It's the one remaining free-text flag onhistorywithout a parser — but it's untouched across all ~18 paged commands in this CLI (reports,deployments,users,folders, …), and a cursor is an opaque server token where trimming would be a change to the value rather than a cleanup of it. Not this PR's line to move.- Nothing in
dbt.rschanged in this commit beyond what the previous review already covered; the diff isutil.rs(behaviour + comment + tests) and the docs reflow.
Docs
cli.mdx is unchanged in substance and still accurate. Same standing caveat as the last two passes: "A page holds at most 100 runs" is server behaviour I can't verify from this repo.
Not verified
cargo test, cargo clippy and cargo fmt were declined by the sandbox again (fifth time this PR) — this review is from reading the code. The PR reports them clean; please rely on CI.
`cube dbt` could start and follow a sync, but not look back at one. Two commands complete it: - `cube dbt history <deployment>` lists recent syncs — id, status, trigger, start, duration, branch — paged with `--first`/`--after`. - `cube dbt logs <deployment> <sync-job-id>` prints a sync's phase timeline and the text a failed phase produced, colouring failure entries. A duration is the server's own `durationMs` or an empty cell — never the difference of two stamps written by different processes, which can disagree with it and, for a run that fails moments after starting, be negative. The two `--wait` failure messages now name `cube dbt logs` for the run that failed, which is the difference between a CI step that explains itself and one that only says "dbt sync failed"; both paths build that message through one function instead of two copies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Every history cell is read the same way, through one `cell` reader: `status` keeps its single key, now with the reason it is the one field that cannot have a second spelling. - Cells are bounded and single-line, so a value carrying a newline can no longer break the row it sits in, nor an unbounded one the layout. - Server log text is stripped of control characters other than the line breaks and tabs the timeline keeps on purpose. `one_line` was never the guard it looks like: ESC is not whitespace, so a hostile dbt error could have retitled a window or overwritten the lines above it in a CI log. - The raw-entry fallback no longer repeats the timestamp and stage the JSON already carries, and keeps its red when the entry says it is a failure — the entry this build understood least is the last place to drop that signal. - `history`'s "could not read this" warning keys on the sync job id rather than on every cell being blank, which one filled column was enough to defeat. - `human_duration_ms` rejects a float too large to cast, which saturated into a confident five-billion-hour duration instead of passing through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Aligns both commands with what the endpoints they call actually publish. - `dbt history` takes `--status` and `--trigger`, sent through unchecked: the two vocabularies are the server's, and a filter this build has not heard of is one the server can still honour, where a list hard-coded here would refuse it. - `dbt logs` drops `--first`/`--after`. One sync's timeline is one page, bounded by the number of phases it ran, so the flags were accepted here and ignored there — a promise of paging that does not exist. - A log line now carries how long its phase took, sharing one bracket with the phase name so a multi-line failure is interrupted by neither. The timings are half of what makes this a timeline rather than a list of remarks. - Fields are read under the names the endpoints publish, and only those: the second spellings were insurance taken out before the shapes were settled, and every one of them was dead. `status` is no longer the odd column out, since no column carries an alias now. A listed run can be CANCELLED or UNKNOWN as well as the two the status endpoint calls terminal. Nothing here acts on a status, so they pass through as they arrived; the docs note that a cancelled run is still a failure to a `--wait` gate, which needs a terminal answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… out - `--status` and `--trigger` carry a `nonempty_filter` parser, like every other free-text argument in the tree. An empty value is not dropped — `push` sends `status=` — so a CI script whose `$STATUS` did not expand would have listed whatever the server made of an empty filter. - Both vocabularies are the server's and they do not share a case (statuses upper, triggers lower), so the help and the docs now spell that out: a mis-cased value is the one mistake that may come back as an empty table rather than as a complaint, and an empty table reads as an answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A filter is one word out of a vocabulary the server publishes, and no member of it has a space in it — so ` FAILED` could only ever match nothing, landing in the exact failure this argument's help was written to prevent: an empty table that reads as an answer. `$(jq -r …)` and a value read out of a file are the ordinary ways to acquire the padding. The two helpers beside it still return what they were given, because a branch name is the caller's own and `--branch ' x '` can name a branch that exists. A test pins the divergence rather than leaving it to be read as an oversight. Also rewraps the docs paragraph the previous commit left one line too long. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2f36fe1 to
4632e05
比较
|
Claude finished @MikeNitsenko's task in 2m 10s —— View job Re-review of
|
| Previously fixed | Still present |
|---|---|
nonempty_filter returns s.trim().to_string() |
util.rs:207 ✓, with the divergence argued in the doc comment and pinned from both sides by the nonempty(" main ") / nonempty_ref(" main ") assertions beside it |
Field aliases (pick) removed; status no longer the odd column out |
✓ — no pick in the file; every cell reads one name through the single cell closure |
Warning re-keyed on ID_COLUMN rather than all-cells-blank |
dbt.rs:794 ✓, with HISTORY_COLUMNS[ID_COLUMN] == "SYNC JOB ID" still pinned |
printable drops every control char but \n/\t |
✓, applied to message and via one_cell to phase/time/every table cell |
| Raw-entry fallback prints alone and keeps its red | log_line early-returns paint_failure(value.to_string(), error) ✓ |
u64::MAX bound before the float cast |
✓, with human_duration_ms("1e30") / ("inf") asserted |
| Docs paragraph rewrapped | ✓ — lines 382–388 run 81–86 chars |
Rebase-conflict check
The only commit on master touching rust/cube-cli is the v1.7.28 version bump, so there was nothing here for the replay to silently reconcile. I re-checked the four helpers this PR leans on against their current master definitions and all still match the assumptions in the diff:
util::push<T: ToString>—Some("")still sendskey=, sononempty_filteris genuinely the only thing between an unexpanded$STATUSand the query string.output::items— still unwrapsitems/data, still falls back tovec![other]for a non-envelope object, which is exactly the case the id-column warning is there to catch.output::table— still measures cells bychars().count(), soCELL_LIMITbounding a cell actually bounds the column.Client::get_optional— still maps 404 toNone, so both new 404 messages are reached the way they're written.
Fresh pass
Nothing new that blocks. The couple of things I'd flag if this were a first read are all previously-argued and previously-answered: printable keeping \n in message (deliberate — multi-line compile errors are the point), the unbounded raw-entry fallback (deliberate — "shown as it arrived"), and [340ms] for a timing with no phase (a shape the endpoint doesn't produce; tested only so the renderer degrades sanely). One observation, not a finding: log_line writes ANSI to stdout unconditionally — no NO_COLOR or tty check — but that's the file-wide and repo-wide convention (output::table bolds its header on stdout the same way) and --json is the machine path, so it's consistent rather than a gap.
Docs
cli.mdx is unchanged in substance. Same standing caveat as the last three passes: "A page holds at most 100 runs" is server behaviour I can't verify from this repo.
Not verified
cargo test, cargo clippy and cargo fmt were declined by the sandbox again (sixth time this PR) — this review is from reading the code. The PR reports 89 tests passing and clippy/fmt clean; please rely on CI. Since this push is a rebase onto a master that only bumped the version, a green CI run here should be the same green as before.
The fixture claimed an `updatedAt` on a listed run. The endpoint deliberately does not publish one — the column behind it is frozen at the launch insert, so a field with that name would never update — and a fixture that carries what the transport does not is the kind of self-consistent wrong stub that green-lights a reader nobody has actually exercised. Nothing read it, so this is fidelity only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 57s —— View job Re-review of
|
关注-up to #11612, which taught the CLI to start and follow a dbt sync but not to
look back at one. This adds the two reads that were missing, so a dbt pull is fully
drivable from a terminal and from CI.
Summary
cube dbt history <deployment>(aliaseslist,ls) — recent syncs as a table:sync job id, status, trigger, start, duration, branch. Paged with
--first/--after,cursor from
pageInfo.endCursor.cube dbt logs <deployment> <sync-job-id>— one sync's phase timeline and the text afailed phase produced, with failure entries in red. Same paging flags.
SchemaRead, pass--jsonthrough untouched, and answer a 404 with asentence naming what it can mean rather than a bare status line.
--waitfailure messages now namecube dbt logsfor the failed run — thereason alone does not say which phase produced it, and that is the difference between
a red CI step that explains itself and one that only says "dbt sync failed". Both wait
paths build that message through one function instead of two copies of it.
Sync history and logssection in the CLI reference, including the CI shapethat prints the timeline when a gate fails, plus the updated
dbtcommand-reference row.Notes
durationMsor an empty cell — nevercompletedAtminus
startedAt. Those stamps are written by different processes, so their differencecan disagree with the server's figure and, for a run that fails moments after starting,
be negative. A test holds that line.
payloads use for the same thing, so a row still renders under either. Nothing is
derived: a field no key matches stays empty, an unrecognised log entry is printed as it
arrived rather than dropped, and a page nothing was recognised in says so and points at
--json.On a tenant that does not serve them yet both commands exit non-zero saying exactly
that; nothing else in
cube dbtchanges behaviour.Validation
cargo test(86 passed; 11 new assertions across history rows, durations, log entriesand the failure message)
cargo clippy --all-targets -- -D warningscargo fmt --all --check--jsonand 404 paths, the empty and unrecognised-payload cases,and both
--waitfailure paths exercised against a stub API.🤖 Generated with Claude Code