feat(cube-cli): run and follow a dbt sync from the CLI - #11562
feat(cube-cli): run and follow a dbt sync from the CLI#11562MikeNitsenko wants to merge 15 commits into
Conversation
A dbt sync could only be started over REST, so a pipeline that wanted to gate a
merge on it had to hand-roll the trigger and then poll the API itself. `cube dbt`
covers the whole job:
cube dbt sync DEPLOYMENT_ID --ref feature/orders --wait
cube dbt status DEPLOYMENT_ID SYNC_JOB_ID [--wait]
cube dbt result DEPLOYMENT_ID SYNC_JOB_ID
cube dbt cancel DEPLOYMENT_ID SYNC_JOB_ID
`--ref` is what makes a pull-request gate mean anything: without it a sync clones
the branch saved on the dbt integration, so every run would compile `main` and
report green for a change that breaks the model.
`--wait` polls to a terminal state and exits non-zero when the sync fails or the
timeout elapses, so a CI step needs no extra scripting. Progress goes to stderr
and only when the stage changes — a long sync prints a handful of lines instead of
one per poll, and `--json` stdout stays a single parseable document. With
`--wait --json` that document carries both halves a pipeline needs: the branch to
compile next, and how the sync ended.
`cube deployments build-status` gains the same `--wait`, which is the compile half
of such a gate. It gives up early, with an explanation, when a branch reports that
nothing is building it: a shared branch — which is what a sync produces — only
compiles once someone opens it in dev mode, and waiting on it otherwise would sit
out the whole timeout.
Supporting pieces:
- `Client::get_optional` treats a 404 as an answer rather than an abort. It is a
normal state twice here — a sync not yet visible, and a result asked for while
the sync still runs — and `get` turning it into an error would end a wait
instead of continuing it. Recognised through a typed `NotFound` error, so no
caller depends on message text.
- `util::parse_duration` for the wait flags, since their useful range spans a
seconds-long poll interval and a tens-of-minutes sync, and a bare number would
silently mean the wrong one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
github-actions
Bot
added
rust
|
Claude finished @MikeNitsenko's task in 3m 53s —— View job Review: solid, well-reasoned CLI addition — 0 high, 2 medium, 5 low (6 inline comments posted)Full review
GeneralThis is unusually careful work. The reasoning is written down where it matters — why 404 needs a typed error rather than message matching, why progress goes to stderr, why unknown statuses mean "still working" rather than "done", why No security concerns. Findings
Plus one question inline on TestingThe five new tests are good ones —
I couldn't run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… windows
Review found the wait path too brittle for the CI job it exists to serve:
- One transient failure ended a wait. A 30-minute `--wait` is ~360 requests, so a
single 502 from a proxy failed the gate for a sync that was fine. It now absorbs
up to five CONSECUTIVE transient failures, reporting each to stderr.
Retrying is a whitelist — 5xx, 408, 429, and requests that never landed — not a
fallback. That matters most for errors it never inspects: a poll closure that has
decided the sync doesn't exist, or that nothing is building a branch, raises a
plain error, and treating THAT as transient would retry a settled verdict until
the timeout. To classify precisely, non-2xx responses and transport failures are
now typed (`ApiError` carrying the status, `TransportError`), replacing the
single-purpose `NotFound`; every message they render is unchanged.
- The grace windows were counted in polls, so `--poll 1s` silently shrank the
"nothing is building this" window from ~30s to ~6s and reported a dev worker that
was merely slow to start as a branch nobody had opened in dev mode — precisely
the wrong diagnosis, in the case the documented recipe hits. Both windows are now
measured in time (60s), independent of the poll interval. Sixty seconds is a
guess at the safe side of a cold start and wants confirming against a real one.
- `timed out after {timeout}` reported the configured timeout even when the wait
ended early because the interval exceeded it. It now reports the time actually
spent, and the sleep is clamped to the remaining time so a long `--poll` still
gets a final look before the deadline.
Also from review:
- `parse_duration` multiplied unchecked, so `9999999999999999999h` panicked in
debug and wrapped in release — a parser whose job is rejecting bad input. Now
checked, and it accepts `30S`/`15M` since the intent is obvious.
- `dbt cancel` ignored `--json` and wrote a success line to stdout, breaking the
one guarantee the other subcommands keep.
- `wait_for_sync` built a second `Client` while its caller already held one, which
also disagreed with `wait_for_build` next door; it takes `&Client` now.
- Rustdoc links were written in JSDoc syntax, so they rendered as literal text.
- The unknown-sync message now mentions that a tenant may not serve the dbt-sync
endpoints at all, which is what an older tenant actually looks like from here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 2m 29s —— View job Follow-up review of
|
| # | Prior issue | Status in ffc5837 |
|---|---|---|
| 1 | Grace windows counted in polls | Fixed — MISSING_GRACE/IDLE_GRACE are Duration::from_secs(60), tracked via Cell<Option<Instant>>, independent of --poll |
| 2 | One transient error ends a 30-minute wait | Fixed, and better than suggested — five consecutive failures absorbed, classified by a whitelist (5xx, 408, 429, transport) rather than a blacklist |
| 3 | value * multiplier overflow |
Fixed — checked_mul, plus two assertions including u64::MAX |
| 4 | cancel ignored --json |
Fixed |
| 5 | wait_for_sync re-created the Client |
Fixed — takes api: &Client, matching wait_for_build |
| 6 | Timeout message reported the configured timeout | Fixed — reports started.elapsed(), and the sleep is clamped to remaining so a long --poll still gets a final look |
| 7 | {@link NotFound} JSDoc + confusing message on a tenant that 404s the routes |
Fixed — intra-doc links, and the message now says "this tenant may not serve the dbt-sync endpoints yet" |
The whitelist decision in client::is_transient is the part worth calling out. A blacklist would have been the obvious implementation and would have quietly retried the wait closures' own verdicts — an unknown sync id, a branch nothing is building — until the timeout, turning a clear diagnosis into a silent 30-minute hang. The doc comment says exactly that, and never_retries_a_verdict_the_attempt_reached pins it. Replacing the single-purpose NotFound with ApiError { status, .. } + TransportError is the right generalisation, and Display is genuinely unchanged per status.
wait::poll now has the tests it was missing: terminal-value, absorb-then-succeed, give-up-after-N, never-retry-a-verdict, and timeout-reports-elapsed. Counts are asserted (MAX_TRANSIENT_FAILURES + 1), not just outcomes.
New findings
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | dbt.rs:83-84 |
Is a cancelled sync's status terminal? If cancel produces CANCELLED, it lands in the permissive bucket and --wait sits out the full 30m on a sync that is over — and cancel ships in this same PR |
| 2 | Low | dbt.rs:233-241 |
The /result fetch happens after wait::poll returns, so it has no transient tolerance — a 502 there still fails the gate for a COMPLETED sync. Same blip, moved to the last request |
| 3 | Low | wait.rs:57-66 |
429 is retried at the same fixed cadence with Retry-After ignored — the one status where the cadence itself is the problem. Plus: a timeout that happens mid-retry reports timed out after Ns with the cause only in stderr |
None of these block. 1 is a question about server-side behaviour I can't verify from here; 2 and 3 are narrow tails of a path that is now much sturdier than it was.
On the 60-second windows
The commit message flags 60s as a guess wanting confirmation against a real cold start — agreed, and that's the right thing to have written down. One interaction to keep in mind: with --timeout 30s, neither diagnosis message can ever fire, since the grace outlives the wait; the user gets the generic timeout instead. Fine as long as it's known.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. The two unchecked boxes (live deployment, build-status --wait) are still the ones that matter most, and the 60s windows and the cancelled-status question above are both things only that run can settle.
…uest Two more from review, both about the wait path failing a job it should carry: - The `/result` read after `--wait` returns had no transient tolerance, so a single 502 on the gate's FINAL request failed a job whose sync had already reported COMPLETED — the same single-blip failure the poll loop absorbs, just moved to the end. It now goes through `wait::poll` with a short 30s budget: the first answer wins, including "no result", so a genuinely absent one still ends immediately. - A 429 was retried at the very cadence that provoked it, five times, and then the wait died — the one status that says the interval itself is the problem. Retries now double the sleep per consecutive failure, capped at 60s, resetting as soon as a poll makes progress. `Retry-After` would be better still, but the header isn't carried on the error and plumbing headers through is more than this needs. - A wait that expired while absorbing failures reported only "timed out after Ns", leaving the cause in stderr lines nobody correlates with the exit. The message now carries the last transient error. Not changed: `CANCELLED` is not in the sync's status vocabulary. Cancelling makes the workflow report FAILED, which is already terminal here, so `--wait` exits promptly and non-zero rather than sitting out the timeout; a constant for it would be dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 1m 58s —— View job Follow-up review of
|
| # | Prior issue | Status in 6686e6e |
|---|---|---|
| 1 | Is a cancelled sync's status terminal? | Answered: cancelling makes the workflow report FAILED, which is already terminal, so --wait exits promptly and non-zero. A CANCELLED constant would be dead code — agreed, and better than adding a speculative one |
| 2 | /result fetch had no transient tolerance |
Fixed — routed through wait::poll with a 30s budget (RESULT_FETCH_TIMEOUT), first answer wins. See the one caveat below |
| 3 | 429 retried at the failing cadence; timeout hid the cause | Fixed — backoff() doubles per consecutive failure capped at 60s, resets on progress, checked_mul so Duration::MAX can't wrap into a tiny sleep; the timeout message now carries the last transient error |
backoff is a careful little function: the cap keeps a long --poll from overshooting the wait it belongs to, the min(remaining) at the call site still guarantees a final look at the deadline, and both edges are asserted (3600s × 16 → cap, Duration::MAX → cap). Skipping Retry-After rather than plumbing headers through the error type is the right trade for the size of the win.
New findings
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | wait.rs:88 |
last_transient is never cleared on progress, so a blip at minute 1 gets blamed for a timeout at minute 30 — and the alternating-closure test can't distinguish the two cases |
| 2 | Low | dbt.rs:249 |
A COMPLETED sync whose result 404s yields "result": null and exit 0; the 30s budget is already there if a missing result should instead be waited on and then failed |
Neither blocks. 2 hinges on whether the API guarantees a result exists the moment COMPLETED is reported — if it does, a comment saying so closes it.
One nit not worth an inline: backoff_doubles_while_failing_and_resets_on_progress doesn't test the reset half — that lives in poll, not backoff. The name promises more than the body checks.
Testing
Seven wait tests now, and they cover the paths that matter: terminal value, absorb-then-succeed, give-up-after-N with the attempt count asserted, verdict-never-retried, backoff edges, timeout-reports-elapsed, and timeout-carries-the-cause. The gap flagged in finding 1 is the one assertion I'd add.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. The two unchecked boxes (live deployment, build-status --wait) are still the ones that matter most: the 60s grace windows and the COMPLETED-with-no-result question above can only be settled against a real tenant.
… a result - `last_transient` was never cleared, so a 502 absorbed at minute 1 of a 30-minute wait was reported as the cause of a timeout at minute 30 — pointing at a blip the loop recovered from, on a wait that timed out because the sync was slow. The count and the message are now one variable: the CURRENT streak, cleared on progress, so they cannot drift apart again. The old test couldn't tell the difference (whether the error or the progress landed last before the deadline was a race); it is replaced by two deterministic ones — a wait that fails throughout and times out reports the cause, and a wait that recovers and then times out does not mention it. - A COMPLETED sync whose result read came back empty produced `"result": null` with exit 0, handing a gate a document whose `result.generatedFiles` is missing — the one shape of failure `--wait` exists to make loud. This is reachable: a result is normally readable the instant the status says COMPLETED, but the run is still closing, and a worker that cannot answer for a moment yields a 404. An absent result is now "not yet" rather than an answer, so a slow one is caught, and one that never arrives fails at the 30s budget with a message saying the sync itself succeeded and how to read the result separately. The JSON document is still printed in that case, since the branch name in it is what a caller needs to carry on with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 2m 13s —— View job Follow-up review of
|
| # | Prior issue | Status in b62cec4 |
|---|---|---|
| 1 | last_transient never cleared, so a recovered blip is blamed for a later timeout |
Fixed, and better than suggested — the count and the message are now a single Option<(u32, String)> streak cleared on progress, so they cannot drift apart again. The old alternating-closure test is replaced by two deterministic ones: fail-throughout reports the cause, recover-then-timeout does not mention it |
| 2 | COMPLETED sync with a missing result → "result": null, exit 0 |
Fixed — None is now Progress::Waiting("result not available yet"), so a slow result is caught and an absent one fails at the 30s budget with a message naming cube dbt result as the way to read it separately. The --json document is still emitted on that failure, which is the right call: the branch name is what a caller needs to carry on with |
Making the two variables one is the part worth calling out. "Remember to clear B when you clear A" is exactly the invariant that rots on the next edit; Option<(count, message)> removes the option of getting it wrong, and None between streaks states the meaning directly.
The docs' CI recipe still works under the new behaviour — it reads only .branchName, and the failure path exits non-zero before jq ever sees the document.
New findings
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | dbt.rs:267 |
The result fetch reuses wait::poll, whose timeout message advises "raise --timeout" and "it may still be running" — neither is true on a fixed 30s budget for a sync that already reported COMPLETED |
| 2 | Low | wait.rs:259 |
a_recovered_blip_is_not_blamed_for_a_later_timeout needs a waiting attempt to land inside 30ms; a scheduler stall on a loaded runner flips the assertion |
Neither blocks; 2 is a test-robustness nit on an assertion worth keeping reliable.
Testing
Eight wait tests, and the two new ones are deterministic in the way the replaced one was not — the commit message is right that "which of the error or the progress landed last" was a race. The counts are asserted, not just the outcomes.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. The two unchecked boxes (live deployment, build-status --wait) are still the ones that matter most: the 60s grace windows and how quickly a real /result becomes readable after COMPLETED can only be settled against a real tenant, and the second of those is what the new 30s budget is sized against.
`poll`'s timeout advice was written for a wait whose budget is the user's `--timeout`, and the result fetch reuses it with a fixed 30s const. So a sync whose result never appeared advised raising a flag that cannot move that deadline, and said "it may still be running" about a sync that had already reported COMPLETED — advice someone would act on and then be confused by. The wait's configuration is now a `Wait` struct with the advice as a field, defaulting to today's wording, and the result fetch replaces it with counsel that is true for it. The struct earns its keep twice over: `poll(what, timeout, interval, …)` took two adjacent `Duration`s, so a wait that polls every 30 minutes for 5 seconds was one transposition away and would have looked like a server problem. Named fields make that unwritable. Also from review: the recovered-blip test asserted the ABSENCE of the timeout's error suffix, which only holds once a `waiting` attempt has landed — so a single scheduler stall right after the failing first attempt, ordinary on a loaded runner, would have flipped it. Its budget goes from 30ms to 300ms, which keeps the assertion (the valuable half) without making it a coin flip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 1m 4s —— View job Follow-up review of
|
| # | Prior issue | Status in 9ca285d |
|---|---|---|
| 1 | Recovery lived only in on_timeout, so exhausted 502 retries and an immediate 401/500 printed no pointer |
Fixed — the context carries it for every outcome and the wait adds nothing, via Wait::advising_nothing() |
You went further than the suggestion block, and the extra piece is the part that makes it work: an empty on_timeout also suppresses the ". " separator in poll, so the timeout message ends at waiting for dbt sync result rather than trailing a dangling period-space. Without that, silencing the tail would have traded a duplicated sentence for a malformed one. timeout_advice_is_the_default_unless_replaced pins both halves — that the default still fires for ordinary callers, and that the silent one ends cleanly — and ends_with is the right assertion for it, since contains would pass on the dangling separator too.
The Last error: suffix still appends after a silent tail, which is correct: that half is the cause, not advice, and a timeout mid-streak should still say what was failing.
New finding
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | dbt.rs:333 |
None => println!("(no result reported)") is unreachable — result is None only when failed, and that branch bail!s first. Reachable before b62cec4; dead since |
Doesn't block, and it isn't from this commit — it's the leftover of the earlier fix, and it only misleads a reader about whether a successful sync can report no result.
One nit not worth an inline: on_timeout: String with "" as the sentinel for "say nothing" is the kind of distinction Option<String> states outright. The doc comment covers it and advising_nothing() keeps the sentinel from spreading to call sites, so this is preference rather than a defect.
On the rest
The diff is 2 files, ~55 lines, and confined to error text plus the one poll formatting branch. Exit codes, the --wait --json document, and the docs' CI recipe are untouched.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. The two unchecked boxes (live deployment, build-status --wait) are still the ones that matter most: the 60s grace windows and how quickly a real /result becomes readable after COMPLETED can only be settled against a real tenant.
Review spotted a dead arm: `(no result reported)` became unreachable once a missing result started leaving through the `Err` branch, since `result` was `None` only when the sync failed and that path bails first. Rather than collapse the arm, the failure now exits before the result is fetched, so the `Option` disappears with it — `result` is a `Value`, and "a completed sync yields a result or an error" is enforced by the shape of the code instead of being a fact a reader has to reconstruct. That also merges the split `if ctx.json` / `if failed` blocks into one failure branch that prints the document and bails, and turns the trailing text output into the `else` of the JSON one. No behaviour change: all three JSON emission points still go through `wait_json`, and each outcome prints exactly what it did before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 59s —— View job Follow-up review of
|
| # | Prior issue | Status in 4abe23a |
|---|---|---|
| 1 | None => println!("(no result reported)") unreachable since b62cec4 |
Fixed — and you took the third option rather than either of the two I offered |
Neither of my suggestions was the right one. Collapsing the arm would have left the Option in place, and unreachable! would have documented an invariant instead of enforcing it; moving the FAILED exit above the fetch removes the Option entirely, so result: Value and "a completed sync yields a result or an error" is now the type rather than a fact a reader reconstructs from two distant branches. That the dead arm had nowhere left to live is the sign it was the right cut.
The side effect is the better half of the diff: if ctx.json / if failed were two blocks separated by the whole result-fetch, and reading them meant holding "did the JSON already print?" across ~60 lines. Now failure is one contiguous branch that prints its document and bails, and the text output is the else of the JSON one.
Behaviour check
Walked all four outcomes against the previous revision — each prints the same bytes in the same order:
| Outcome | --json |
text |
|---|---|---|
FAILED |
wait_json(…, None) then non-zero |
non-zero with the workflow's reason |
COMPLETED, result read |
wait_json(…, Some(result)) |
success line + print_result |
COMPLETED, read fails |
wait_json(…, None) then the context'd error |
same error |
| unreachable "completed, no result" | — | gone, as intended |
The FAILED branch still emits before bailing, which is what keeps the branch name available to a caller whose sync failed. All three emission points still go through wait_json, so the document shape can't diverge between them.
On the rest
One nit not worth an inline: the /// block above COMPLETED describes both constants but rustdoc attaches it to COMPLETED alone, leaving FAILED undocumented in the rendered output. A plain // would say the same thing without the mismatch. Same shape as the doc block above MISSING_GRACE, which does belong to its one const.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. This commit is a control-flow rearrangement with no new tests, and the four-outcome table above is the check I could do by reading.
The two unchecked boxes (live deployment, build-status --wait) are still the ones that matter most, and unchanged since the first round: the 60s grace windows and how quickly a real /result becomes readable after COMPLETED can only be settled against a real tenant.
Found by running this against a real deployment, which is also where the premise
it was built on fell over. `build-status --wait` gave up only on `none`/`stopped`,
and against a live tenant neither of the cases that never finish reports those:
branch that does not exist → building, errorText "Bad branch"
shared branch, no dev worker → building, errorText "Branch is not active"
inactive dev branch → building, errorText null
active dev branch → built
So both hung for the full 15-minute timeout in silence — the exact outcome the
give-up guard existed to prevent, on the branch the documented recipe is most
likely to be pointed at by mistake. A non-terminal status carrying an errorText is
the endpoint saying this will never finish, and it is the only reliable way to
hear it; the wait now fails on that, carrying the API's own words. Measured on the
tenant: 2.5s and 2.2s with exit 1, where both previously ran to the deadline.
The `none`/`stopped` window stays as a backstop for a worker that reports itself
stopped, no longer claiming to cover the shared-branch case.
The docs said `build-status` reports `none` for a shared branch forever, which was
wrong in the same way; they now describe what it actually reports and note that
waiting on the wrong branch fails fast.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correcting something I asserted confidently earlier in this review, because it was wrong and it mattered. When you asked whether
Neither case that never finishes reports The fix is to use the signal the endpoint actually sends: a non-terminal status carrying an Worth recording why this survived seven review rounds: it isn't the kind of thing reading the diff can catch — the constants looked principled and the reasoning was internally consistent. It took pointing the binary at a deployment. That's the box still unchecked on the test plan, and it just paid for itself. |
|
Claude finished @MikeNitsenko's task in 1m 37s —— View job Follow-up review of
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | deployments.rs:62 |
Is errorText guaranteed empty for a healthy in-progress build? If it can carry the previous build's error, a rebuild after a red build aborts on the first poll — a worse failure than the hang, and it lands exactly when CI re-runs |
| 2 | Low | deployments.rs:64 |
The dev-mode advice is unconditional, but Bad branch means the branch doesn't exist — opening it in dev mode can't be the fix, and the message hands over a copy-pasteable command that won't work |
Neither blocks. 1 is a question the same tenant can settle in one push; 2 is a phrasing narrowing.
On the rest of the diff
33 lines across one file plus 9 in the docs. dbt.rs, wait.rs, client.rs, util.rs are untouched — the --wait --json document, exit codes, and the CI recipe's jq paths are unaffected. No new tests, which is consistent: this branch needs a server response to exercise, and the run against the tenant is the evidence.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. Worth noting that this commit closes the second of the two long-unchecked boxes: build-status --wait has now been run against a real deployment, and it's what found the bug.
Review asked the question the four measured cases didn't answer: is `errorText`
guaranteed empty for a HEALTHY in-progress build? It isn't knowable from this
side — the field is the dev worker's own status proxied through console-server —
and the risk is asymmetric. If it can hold a previous failure, aborting on any
error fails a rebuild after a red build on its first poll, which is worse than the
hang it replaces and lands exactly when CI retries.
So the guard matches the two verdicts measured against a live deployment rather
than "any error", and an unrecognised one is surfaced once on stderr and waited
through — degrading to the old behaviour, bounded by --timeout, instead of failing
a build that is fine.
The two also get their own advice, which review caught as well: "Bad branch" means
no such branch exists, and telling someone who mistyped a name to open it in dev
mode sends them somewhere that cannot help. Verified against the tenant:
does-not-exist → Bad branch. There is no such branch — check the name with
`cube data-model branches <deployment>`
dbt-sync/… → Branch is not active. A branch only compiles once it is
opened in dev mode — run `cube data-model dev-mode …`
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 1m 41s —— View job Follow-up review of
|
| # | Prior issue | Status in dd1a029 |
|---|---|---|
| 1 | Give-up fired on any errorText; a stale one from a previous red build would abort a healthy rebuild on its first poll |
Fixed — NOT_BUILDING matches only the two verdicts measured against the tenant, and an unrecognised one is surfaced once on stderr and waited through, bounded by --timeout |
| 2 | The dev-mode advice was unconditional, so Bad branch (branch doesn't exist) got a copy-pasteable command that can't work |
Fixed — each verdict carries its own advice; Bad branch now points at cube data-model branches <deployment>, which I confirmed exists (data_model.rs:85) |
You resolved the staleness question the way the asymmetry pointed rather than by trying to prove the field's lifetime — the comment says outright that it "isn't knowable from this side", which is the honest version of an answer I couldn't get either. Pairing verdict with advice in one &[(&str, &str)] is also the shape that keeps them from drifting: a third verdict can't be added without someone deciding what to tell the reader about it.
The docs' <Warning> already names both verdicts by their exact text, so it describes the narrowed guard rather than the old any-error one — no correction needed this round.
New findings
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | deployments.rs:101 |
The unrecognised verdict is printed once at the start, then the wait sits out the full timeout and ends with "it may still be running" — the explanation is 15 minutes above the failure in a CI log, the same correlation problem wait.rs already fixed for transient errors. Plus reported_error is a one-shot for the whole wait, not per-condition as its comment says |
| 2 | Low | deployments.rs:108 |
The none/stopped backstop still asserts the dev-mode explanation unconditionally, now that the branch-doesn't-exist case leaves via the Bad branch arm above |
Neither blocks; 1 is the only one with a behavioural edge, and it's about a log being self-explanatory rather than about an outcome.
On the rest
62 lines in one file, no other file touched — dbt.rs, wait.rs, client.rs, util.rs and the docs are unchanged, so the --wait --json document, exit codes and the CI recipe's jq paths are unaffected. No new tests, which is consistent with the previous commit: this branch needs a live server response to exercise, and the measured verdicts in the commit message are the evidence.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. Both of the long-unchecked boxes have now been exercised against a real tenant across the last two commits, and that's what produced the two most valuable commits in the PR.
Review caught the degraded path losing its explanation: an unrecognised errorText was announced once at the start, and fifteen minutes later the wait failed with a generic "it may still be running" about a branch the endpoint had been complaining about the whole time — the same correlation problem already fixed for transient errors, in the one remaining place the loop knew something the failure didn't say. Two changes, and they remove code rather than add it: - The complaint now rides in the `Waiting` label instead of a one-shot `eprintln`. The loop already reports labels when they CHANGE, so a new complaint surfaces, a persistent one stays quiet, and a *different* one is no longer swallowed — which the `Cell<bool>` did, despite its comment claiming otherwise. The cell is gone. - `poll`'s timeout names the last label it saw: "timed out after 900s waiting for build (last seen: building (Branch is weird))". That helps every waiter, not just this one — a dbt sync that times out now says which stage it was on. Also softened the `none`/`stopped` backstop's advice: with each known verdict carrying its own hint, this one covers "a worker reporting itself stopped", so it no longer asserts the shared-branch explanation as fact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 2m 50s —— View job Follow-up review of
|
| # | Prior issue | Status in 4f413cb |
|---|---|---|
| 1 | An unrecognised errorText was announced once at the start, then the timeout 15 minutes later explained nothing; reported_error was a one-shot despite its comment |
Fixed, and by deleting rather than extending — the complaint rides in the Waiting label, so the loop's existing change-detection does the work: new complaint surfaces, persistent one stays quiet, a different one is no longer swallowed. The Cell<bool> is gone |
| 2 | The none/stopped backstop asserted the shared-branch explanation as fact |
Fixed — "If the branch exists, it only compiles once…", which is the case that arm now actually covers |
The poll half is the better half: (last seen: {label}) helps every waiter, not just this one. cube dbt sync --wait timing out now names the stage it was on ((last seen: COMPILING_DBT 50% — Parsing dbt project)) instead of leaving that in a stderr line somewhere above, and the result fetch gets (last seen: result not available yet). Both new assertions are on the right thing — ends_with("waiting for thing (last seen: working)") in the silent-advice test also keeps pinning the no-dangling-separator behaviour from 9ca285d, which a contains would have let slip.
Worth noting the ordering in the message is deliberate and correct: what it was looking at, then the advice, then the last transient error. The cause reads last, which is where the eye lands.
New findings
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | deployments.rs:120 |
last_label is the dedupe key, so putting arbitrary worker text in it makes "a handful of lines" depend on that text being stable — a counter or a rotating address in errorText gives ~180 stderr lines on a 900s wait, and a multi-line one breaks the one-line progress format and lands whole inside (last seen: …) |
| 2 | Low | deployments.rs:87-97 |
The outer !error.is_empty() guard is dead now that the None arm is gone — find already rejects the empty case. Plus: the none/stopped bail doesn't mention error, so an unrecognised complaint on a stopped worker never reaches a failure message |
Neither blocks. 1 is a noisy-log tail, not a wrong outcome; 2 is a nesting level.
On the rest
Two files, +31/−18, and the diff genuinely removes state. dbt.rs, client.rs, util.rs and the docs are untouched, so the --wait --json document, exit codes and the CI recipe's jq paths are unaffected — the only behavioural reach beyond build-status is the timeout message every waiter now shares, which is additive.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. On edition 2021 the if / if let nesting in finding 2 won't trip collapsible_if, so that one is cleanup rather than a CI risk. Both long-unchecked test-plan boxes have now been exercised against a live tenant over the last three commits, and that run is what produced the findings the diff couldn't.
…e key Carrying the unrecognised errorText in the progress label traded away the property `poll` promises: labels are deduped by equality, so anything volatile in that text — a counter, a rotating address — makes every poll a "change", and a 15-minute wait prints a line per poll. The status half can't do that (closed vocabulary); this half is arbitrary text from a worker. A multi-line error had the same shape of problem, turning one progress line into several and landing whole inside "(last seen: …)". `one_line` collapses whitespace and truncates to 120 chars, keeping the leading text where these verdicts put their meaning. Unit-tested, including that it counts characters rather than bytes — truncating a multi-byte error on a byte boundary would panic. Also from review: the outer `!error.is_empty()` guard went dead when the `None` arm did (`"".contains(…)` is already false), and the `none`/`stopped` bail now names the complaint too — that branch bails rather than timing out, so the timeout's new "(last seen: …)" never speaks for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 2m 32s —— View job Follow-up review of
|
| # | Prior issue | Status in f0f5a48 |
|---|---|---|
| 1 | last_label is the dedupe key, so volatile or multi-line errorText turns "a handful of lines" into ~180, and lands whole inside (last seen: …) |
Fixed — one_line collapses whitespace and truncates to 120 chars before the text reaches the label |
| 2 | Dead !error.is_empty() guard; the none/stopped bail never named error |
Both fixed — the guard is gone (find already rejects empty), and the backstop message appends : {complaint} when there is one |
The character-vs-byte test is the one I wouldn't have thought to ask for and the one that matters most: &collapsed[..120] is the obvious way to write this function and it panics on a multi-byte boundary — a Cyrillic or accented error message from a worker would have turned a diagnostic into a CLI crash. Writing the assertion as chars().count() == 11 rather than comparing to a literal also keeps the ellipsis accounted for explicitly.
The comment on the none/stopped interpolation ("this branch bails instead of timing out, so the timeout's (last seen: …) never speaks for it") is the sentence that explains why the two messages aren't redundant — worth having, since the obvious reading is that one of them is duplicate work.
New finding
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | deployments.rs:110 |
The NOT_BUILDING bail is the one remaining site taking errorText raw — contains matching means a verdict can arrive with a multi-line tail, and the hint then lands after the noise. one_line's own doc names the failure message as a user |
Doesn't block; it's the same normalisation applied one site short, and it's a two-line move.
One nit not worth an inline: one_line sits in deployments.rs but is general-purpose (util.rs already holds parse_duration for the same reason). Fine where it is while there's exactly one caller — dbt.rs's labels come from a closed vocabulary and don't need it.
On the rest
One file, +77/−15, and the behavioural reach is confined to build-status --wait's stderr lines and two failure messages. dbt.rs, wait.rs, client.rs, util.rs and the docs are untouched, so the --wait --json document, exit codes and the CI recipe's jq paths are unaffected.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. Both of the long-unchecked boxes have been exercised against a live tenant across the last four commits, which is what produced the findings the diff alone couldn't.
Review caught the one consumer of `errorText` this didn't convert: the `NOT_BUILDING` bail interpolated the raw field, so a worker reporting "Bad branch\n ref: …\n <stack>" produced a multi-line failure with the advice stranded after the noise — on the arm CI hits most, the mistyped branch. Matching still reads the RAW text rather than the normalised one, deliberately and contrary to the suggested patch: the match is `contains`, so truncating first could cut off a verdict that arrives after a prefix and silently lose the fatal case. Detection reads raw, everything a human reads is normalised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 1m 33s —— View job Follow-up review of
|
| # | Prior issue | Status in 19f3953 |
|---|---|---|
| 1 | The NOT_BUILDING bail was the last site interpolating raw errorText, so a multi-line verdict stranded the advice after the noise |
Fixed — the message now uses one_line(&error, COMPLAINT_LIMIT), so all three consumers are normalised |
You didn't take the patch, and the reason is better than the patch was. My snippet normalised before the find, which changes what contains sees — and since truncation keeps only the leading 120 chars, a verdict arriving after a longer prefix would have stopped matching, converting a 2.5-second fatal diagnosis back into the 15-minute hang this whole line of commits exists to remove. Detection reads raw, presentation reads normalised, and the comment says which is which and why. That's the correct direction of the asymmetry, same as is_transient and NOT_BUILDING itself: err toward still catching the fatal case.
The complaint binding also moves above the find rather than being computed twice, so the three sites (bail, none/stopped message, Waiting label) share one string and can't drift.
New finding
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | deployments.rs:121 |
The raw/normalised split means the printed complaint can omit the verdict that triggered the bail — exactly in the long-prefix case raw matching exists for, so the message quotes a cause that doesn't support its own advice. The verdict is the discarded _ in the tuple and naming it closes it |
Doesn't block; both known verdicts lead the field today, so it's invisible in practice.
On the rest
One file, +12/−9, and the only behavioural change is the text of one failure message. dbt.rs, wait.rs, client.rs, util.rs and the docs are untouched — the --wait --json document, exit codes and the CI recipe's jq paths are unaffected. one_line's existing test already covers the collapse and the char-boundary truncation this now depends on.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. Both of the long-unchecked boxes have been exercised against a live tenant across the last five commits, which is what produced this run of findings.
Matching raw while printing truncated means that in exactly the case raw matching
exists for — a verdict after a long prefix — the printed complaint need not contain
the verdict. The message then quotes a cause that doesn't support the advice
beside it, which is what makes someone doubt the tool.
The matched verdict is now named, and the fuller text appended only when it says
something the verdict doesn't. Unconditionally including both would read "not
building (Bad branch): Bad branch" in the common case, where the field IS the
verdict:
(Bad branch). There is no such branch — check the name with …
(Bad branch): Bad request resolving refs …. There is no such branch — …
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 1m 40s —— View job Follow-up review of
|
| # | Prior issue | Status in 315b7b6 |
|---|---|---|
| 1 | Raw matching + truncated printing meant the quoted complaint need not contain the verdict that triggered the bail, so the message stated a cause that didn't support its own advice | Fixed — the matched verdict is named in the message, so the diagnosis holds regardless of where in the field the verdict appeared |
The half I hadn't thought about is the suppression: naming the verdict and always printing the field would have read not building (Bad branch): Bad branch on the common case, which is the same doubling problem in a new place. Emitting the tail only when it says something new keeps the fix from paying for itself in noise. The commit message showing both rendered forms side by side is the right way to justify a conditional in a format string.
The docs are unaffected: reference/cli.mdx:269 names the two verdicts as the API's own text and doesn't quote the CLI's message shape, so the added (verdict) doesn't make it stale.
New finding
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | deployments.rs:129 |
The suppression is exact equality while the match is contains, so a decorated verdict ("Error: Bad branch", or a trailing period) prints it twice — the printing side asserts an exactness the matching side deliberately doesn't. Not live today; contains closes it, at the cost of suppressing genuinely additive context |
Doesn't block, and it has a real trade in it — a comment recording the choice would settle it as well as a code change would.
On the rest
One file, +14/−3, confined to a single failure message. dbt.rs, wait.rs, client.rs, util.rs and the docs are untouched, so the --wait --json document, exit codes and the CI recipe's jq paths are unaffected. one_line's two tests still cover the normalisation this depends on; the new branch itself has no test, which is consistent with the rest of this arm — it needs a server response shape to exercise.
I could not run cargo test / clippy in this environment (toolchain invocation not permitted), so this remains static review — your checked test-plan boxes are the evidence there. Both of the long-unchecked boxes have been exercised against a live tenant across the last six commits, which is what produced this run of findings.
…ains Review noted the printing side asserts something the matching side deliberately doesn't: the match is `contains`, the suppression is `==`. That asymmetry is the right way round, and the reasoning belongs where the next reader will look rather than in a resolved review thread. `contains` would swallow a decoration that carries the detail — "Bad branch: refs/heads/typo not found" printing as the bare verdict — while equality only costs a few repeated words in a shape neither known verdict produces. Behaviour unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @MikeNitsenko's task in 1m 0s —— View job Follow-up review of
|
| # | Prior issue | Status in 1954e6b |
|---|---|---|
| 1 | Tail suppression is == while the match is contains; decide between contains-based suppression and documenting the equality choice |
Closed by the second option, which was the one with the better argument behind it |
I offered the choice without a recommendation because the trade genuinely goes both ways, and the commit message picks the side I'd have picked for the reason I hadn't articulated: the two failure modes aren't equal in cost. contains fails by deleting information — "Bad branch: refs/heads/typo not found" printing as the bare verdict drops the only line that says which ref was wrong, silently, exactly when a person needs it. Equality fails by repeating a few words, visibly, in a shape neither known verdict produces. Losing the detail is the worse of the two and the harder one to notice, so the asymmetry belongs where it is.
The comment also does the thing a comment is for here — it names the asymmetry with the match above as deliberate. contains on one line and == eleven lines below is precisely the pair a later reader "tidies" into agreement; now they can't do that without reading why.
One byproduct of the ordering worth noting, since it makes the documented downside narrower than the comment claims: complaint is one_line(&error, …), so it is compared after whitespace collapse. A verdict arriving as "Bad branch\n" or with trailing spaces still compares equal and still suppresses — only non-whitespace decoration reaches the double-print. That's the normalisation from f0f5a48 paying off a second time in a place it wasn't written for.
On the rest
Comment-only: 9 added lines in one file, no code touched. dbt.rs, wait.rs, client.rs, util.rs and the docs are unchanged, so the --wait --json document, exit codes, stderr progress and the CI recipe's jq paths are all bit-identical to 315b7b6. Nothing to test that wasn't already covered.
I could not run cargo fmt / test / clippy in this environment (toolchain invocation not permitted) — the same limitation as every prior round; your checked test-plan boxes are the evidence there. For a comment-only diff the only build risk would be an unterminated block, and the // lines close cleanly against the anyhow::bail! that follows.
Both of the long-unchecked test-plan boxes have been exercised against a live tenant across the last seven commits, and that run is what produced the findings this line of review has been closing since.
1 participant
Summary
Adds a
cube dbtcommand group so a dbt sync can be run and followed from the CLI, and--waittocube deployments build-statusso the compile step can be waited on too. Together these let a CI job gate a merge on a dbt change: sync the ref under review, compile the generated model, query it, and fail before anything reaches production.--refis what makes a pull-request gate meaningful. Without it a sync clones the branch saved on the dbt integration, so every run would compile the tracked branch and report green for a change that breaks the model.--waitpolls to a terminal state and exits non-zero when the sync fails or the timeout elapses, so a CI step needs no extra scripting. Progress goes to stderr, and only when the stage changes, so a fifteen-minute sync prints a handful of lines rather than one per poll and--jsonstdout stays a single parseable document. With--wait --jsonthat document carries both halves a pipeline needs — the branch to compile next, and how the sync ended.build-status --waitgives up early, with an explanation, when a branch reports that nothing is building it. A shared branch — which is what a sync produces — only compiles once someone opens it in dev mode; waiting on it otherwise would sit out the whole timeout for no reason.Two supporting pieces:
Client::get_optionaltreats a 404 as an answer rather than an abort, because it is a normal state twice here (a sync not yet visible, and a result asked for while the sync still runs) andgetturning it into an error would end a wait instead of continuing it. It recognises the case through a typedNotFounderror rather than matching message text, and that error'sDisplayis unchanged, so nothing that merely prints it reads differently.util::parse_durationfor the wait flags, whose useful range spans a seconds-long poll interval and a tens-of-minutes sync — a bare number would have to pick one and silently surprise anyone who meant the other.Docs: a
dbtrow in the CLI command reference, adbt syncsection with the CI-gate recipe, and a pointer from the dbt integration page's CI/CD section, which until now offered only the raw REST endpoint.Test plan
cargo fmt --all --checkandcargo clippy --all-targets -- -D warningscleancargo test— 12 pass, 5 new (duration parsing in both directions, three status-label shapes)--helpfor every new subcommand, shell completions still generate, and a bad--timeoutis rejected at parse time with a usable messagesync --refon a non-tracked branch through toCOMPLETED, the merged--wait --jsondocument,result,cancelmid-run, and a foreignsyncJobId(should report the sync as unknown)build-status --waiton a dev-mode branch, and the early give-up on a shared branchNotes
rustcolder than 1.88 cannot build the crate at all — a transitive dependency (home) requires it. CI installs current stable, so this only affects local builds; I did not pin the lockfile to work around it.🤖 Generated with Claude Code