fix: bubble real dbt show error instead of generic "Could not parse"#933
fix: bubble real dbt show error instead of generic "Could not parse"#933sahrizvi wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughexecDbtShow now records JSON-mode and plain-text run rejections, attempts to parse recovered stdout for JSON error events, and uses extractDbtError to surface a prioritized, human-readable dbt error instead of always throwing a generic parse-failure. ChangesReal dbt error surfacing in execDbtShow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
`execDbtShow` swallowed `run()` rejections silently — when `dbt show` crashed (e.g. corrupted `dbt_packages/*`, missing `dbt_project.yml`, DB connection refused), callers saw a misleading "Could not parse dbt show output in any format" message and treated it as transient. The real `Runtime Error: ...` from `dbt`'s stderr never surfaced. Capture the `execFile` rejection's `.stderr` and `.stdout`, scan recovered JSON log lines for `level: "error"` events (dbt with `--log-format json` emits structured error events even on crash), and surface the real error. Preserve the existing generic message only when both `run()` invocations exit 0 but the output is genuinely unparseable — the condition the message was actually designed for. - src/dbt-cli.ts: 2 catch blocks now retain the error; new `extractDbtError()` helper picks structured event > stderr > message. - test/dbt-cli.test.ts: 6 new cases (real stderr surfaces, structured event preferred, ENOENT fallback, generic message preserved on exit-0 unparseable). 24/24 pass. Scoped to `execDbtShow`. `execDbtCompile` and `execDbtCompileInline` share the same masking pattern but have manifest.json / `--quiet` fallbacks that reduce impact — addressed separately if needed. Closes #932
sahrizvi
force-pushed
the
fix/bubble-dbt-show-error-932
branch
from
62e21bf to
064b0dd
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @sahrizvi |
The new test at dbt-cli.test.ts L199 ("preserves generic 'Could not
parse' when dbt exited 0 but output unparseable") used the same mock
implementation and asserted the same rejection string as the existing
"Tier 3: throws with helpful message when all tiers fail" test at
L145. The generic-error path is fully covered by Tier 3; any future
change to that behaviour would have had to be kept in sync across
two identical tests.
Addresses cubic-dev-ai review feedback on #933 (P3).
Tests: 23 pass / 0 fail in packages/dbt-tools/test/dbt-cli.test.ts.
Refs #932
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Addresses cubic review feedbackPushed cubic findings — both resolved
Tests + verification
Centralized test bot failures — likely shared infra, not PR-causedThe |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
suryaiyer95
left a comment
suryaiyer95
left a comment
There was a problem hiding this comment.
Multi-model consensus review (Claude + Gemini 3.1 Pro + Kimi K2.5 + Grok 4, 1 convergence round).
Verdict: REQUEST CHANGES — Critical: 1, Major: 1, Minor: 5, Nits: 2.
The intent is excellent and the surrounding structure is clean, but as written the fix is inert in production (CRITICAL 1) and the same change opens a silent-wrong-data path once that's fixed (MAJOR 2). These two are coupled and must land together, with tests that exercise the real execFile callback wiring rather than a hand-decorated mock.
Inline comments below. (Posted as a COMMENT review, not a formal block — flip to Request Changes if you prefer.)
Note: parseJsonLines cannot throw (it wraps every JSON.parse in its own try/catch), so the "fallback breaks if parse throws" concern was considered and rejected.
| lines = [] | ||
| } catch (e) { | ||
| primaryRunError = e as ExecFileError | ||
| lines = parseJsonLines(primaryRunError.stdout ?? "") |
There was a problem hiding this comment.
CRITICAL — the fix is inert in production: primaryRunError.stdout is always undefined.
run() (line 61) rejects with the bare execFile error: if (err) reject(err). Node's execFile passes stdout/stderr as separate callback arguments, not as properties on the error object — and this manual Promise wrapper discards them on rejection (unlike util.promisify(execFile), which decorates the error). So at runtime primaryRunError.stdout and .stderr are undefined:
parseJsonLines(undefined ?? "")→[]→ no structured error eventprimary?.stderr→undefined→ no stderr surfacedextractDbtErrorfalls through toprimary?.message= Node's generic"Command failed: <dbt> show --inline ..."
The real dbt error this PR exists to surface (e.g. "Runtime Error: Failed to read package...") is never shown. Verified empirically by reproducing the exact run() pattern under Node v20.20 and Bun 1.3.14: 'stdout' in err === false, err.stdout === undefined in both.
Fix — attach stdout/stderr in run() before rejecting:
execFile(dbt.path, args, { ... }, (err, stdout, stderr) => {
if (err) {
const execErr = err as ExecFileError
execErr.stdout = stdout
execErr.stderr = stderr
reject(execErr)
} else resolve({ stdout, stderr })
})| } catch { | ||
| lines = [] | ||
| } catch (e) { | ||
| primaryRunError = e as ExecFileError |
There was a problem hiding this comment.
MINOR — unguarded e as ExecFileError cast (also at line 305). Low-risk since execFile realistically rejects with an Error, but a cheap guard documents intent and hardens against a non-Error rejection:
const isExecFileError = (e: unknown): e is ExecFileError => e instanceof Error
primaryRunError = isExecFileError(e) ? e : (new Error(String(e)) as ExecFileError)| // If either run() rejected, dbt actually crashed — surface the real error | ||
| // instead of the generic "Could not parse" message. | ||
| const realError = extractDbtError(lines, primaryRunError, plainRunError) |
There was a problem hiding this comment.
MAJOR — feeding the failed run's stdout into Tier 1/Tier 2 can return bogus rows / mask the error.
Once CRITICAL 1 is fixed and primaryRunError.stdout is actually populated on a crash, line 234 pushes crash log lines into lines, which then flow through Tier 1 (238-242) and the Tier 2 heuristic scan (278-286) before extractDbtError is consulted here at line 310. looksLikeRowData matches any depth-≤5 array whose first element is an object, so an incidental array in a dbt crash log can be returned as spurious "rows" (silent wrong data) — worse than the original misleading error. The existing comment at lines 269-271 already warns about exactly this Tier-2 false-positive hazard.
Fix — when a run errored, extract the error before the heuristic tiers and source success-path lines only from a successful run:
if (primaryRunError || plainRunError) {
const realError = extractDbtError(parseJsonLines(primaryRunError?.stdout ?? ""), primaryRunError, plainRunError)
if (realError) throw new Error(`dbt show failed: ${realError}`)
}Also add a regression test: a failed run whose stdout contains an incidental array-of-objects must throw, not return rows.
| // If either run() rejected, dbt actually crashed — surface the real error | ||
| // instead of the generic "Could not parse" message. | ||
| const realError = extractDbtError(lines, primaryRunError, plainRunError) | ||
| if (realError) { |
There was a problem hiding this comment.
MINOR — extractDbtError is invoked unconditionally even when neither run failed. It self-short-circuits via if (!primary && !plain) return undefined (line 346), so this is cosmetic, but gating the call behind if (primaryRunError || plainRunError) (the MAJOR-2 fix above) makes the intent explicit: we only look for a "real dbt error" when dbt actually errored.
| stdout?: string | ||
| stderr?: string |
There was a problem hiding this comment.
NIT — stdout/stderr typed string but Node delivers string | Buffer. extractDbtError already calls .toString() so runtime is fine, but the interface is a minor type lie. Widen to stdout?: string | Buffer / stderr?: string | Buffer.
|
|
||
| const errorEvent = lines.find( | ||
| (l: any) => l.info?.level === "error" || l.level === "error", | ||
| ) as any |
There was a problem hiding this comment.
NIT — as any undercuts the Record<string, unknown>[] typing (consensus across all three external reviewers). Prefer a small typed shape or predicate:
interface DbtLogLine { info?: { level?: string; msg?: string }; level?: string; msg?: string }
const errorEvent = lines.find((l): l is DbtLogLine =>
(l as DbtLogLine).info?.level === "error" || (l as DbtLogLine).level === "error")| mockExecFile.mockImplementation((_cmd: string, _args: string[], _opts: any, cb: Function) => { | ||
| const err: any = new Error("Command failed: dbt show --inline ...") | ||
| err.code = 1 | ||
| err.stdout = "" | ||
| err.stderr = | ||
| "Runtime Error: Failed to read package: No dbt_project.yml found at expected path dbt_packages/dbt_utils/dbt_project.yml" | ||
| cb(err, err.stdout, err.stderr) |
There was a problem hiding this comment.
This mock is why the tests pass while production fails (see CRITICAL 1). It attaches stdout/stderr onto the error object (err.stdout = ...), then passes them to cb. Node/Bun never put stdout/stderr on the rejected error — they only arrive as the 2nd/3rd callback args, which run() discards. A faithful mock should leave them only on the callback args:
const err: any = new Error("Command failed")
err.code = 1
cb(err, "", "Runtime Error: Failed to read package...") // NOT err.stdout/err.stderrWith that mock, this test would currently fail — which is the point: it would catch CRITICAL 1. Add this faithful-wiring test alongside the run() fix.
| cb(err, err.stdout, err.stderr) | ||
| }) | ||
|
|
||
| await expect(execDbtShow("SELECT 1")).rejects.toThrow(/Compilation Error.*Model 'foo'/) |
There was a problem hiding this comment.
MINOR — missing coverage for the top-level { level: "error" } shape. extractDbtError handles both l.info?.level and top-level l.level === "error" (line 349), but this test only emits the nested info form. Add a sibling case with { level: "error", msg: "..." } so the || l.level === "error" branch is exercised.
| cb(err, err.stdout, err.stderr) | ||
| }) | ||
|
|
||
| await expect(execDbtShow("SELECT 1")).rejects.not.toThrow(/Could not parse dbt show output/) |
There was a problem hiding this comment.
MINOR — negation-only assertion is weak. .rejects.not.toThrow(/Could not parse/) passes even if the function threw something unrelated (or, post-CRITICAL-1, the wrong generic message). Add a positive assertion of the intended behavior:
await expect(execDbtShow("SELECT 1")).rejects.toThrow(/Database Error.*connection refused|dbt show failed/)| }) | ||
|
|
||
| await expect(execDbtShow("SELECT 1")).rejects.toThrow(/spawn ENOENT|dbt show failed/) | ||
| }) |
There was a problem hiding this comment.
MINOR — missing test: JSON run crashes but Tier 3 plain-text retry succeeds. Assert it returns the parsed table and does not throw — this proves the real error is surfaced only when both tiers fail, and locks in the recovery path.
3 participants
PINEAPPLE
Closes #932
(Supersedes #931 — same commits, branch renamed to drop the stale Jira key.)
Summary
altimate-dbt execute --query "..."(which callsexecDbtShowinpackages/dbt-tools/src/dbt-cli.ts) swallows the real error fromdbt showand surfaces a misleading generic message:{ "error": "Could not parse dbt show output in any format (JSON, heuristic, or plain text). Got 0 JSON lines.", "fix": "Run: altimate-dbt doctor" }But running
dbt showdirectly produces a clear actionable error, e.g.:Agents read "could not parse" as transient and retry alternate commands instead of bailing out — burns budget on a project that is structurally broken.
Root cause
packages/dbt-tools/src/dbt-cli.tshas two adjacent catch blocks inexecDbtShowthat swallow therun()rejection silently:When
execFilerejects with a non-zero exit, the error object carries.stderrand.stdout. Both catches discard them. The generic "Could not parse" message was designed for the case "dbt exited 0 but the output is unparseable" — but it currently fires for the "dbt actually crashed" case as well, conflating two very different failure modes.Fix
execFilerejection on both tiers (primaryRunError,plainRunError).stdout— dbt with--log-format jsonemits structuredlevel: "error"events before exit.extractDbtError(...)which picks (in order): structured error event > primary stderr > plain stderr > exception message.extractDbtErrorreturns anything, surface it asdbt show failed: <real msg>. Fall through to the generic message only when both runs exited 0.Net: ~45 lines in
dbt-cli.ts(helper + 2 small guard sites), 6 new test cases.Scope
Limited to
execDbtShow.execDbtCompileandexecDbtCompileInlineshare the samecatch { lines = [] }pattern but have additional fallback paths (manifest.json for compile, no-args plain-text retry for inline) that reduce caller impact. Worth addressing in a follow-up PR if telemetry shows masking there too — kept out of this PR to keep the diff focused.Test plan
bun test test/dbt-cli.test.ts→ 24/24 pass (18 pre-existing + 6 new)dbt showexits 0 but emits unparseable output (regression guard preserved)dbt showsurfaces in the thrown errorlevel: "error"event preferred over raw stderr when both presentspawn ENOENT(no dbt binary) surfaces clearlydbt_packages/<pkg>/dbt_project.yml; expect to see the real "Failed to read package" message, not "Could not parse"Links
03-issues-and-fixes.mdIssue docs: rewrite README for open-source launch #14dbt_packages/fixtureSummary by cubic
Surface the real dbt error from
execDbtShowinstead of the generic “Could not parse…” message, so callers get actionable failures and agents stop wasting retries. Addresses #932.run()errors for both JSON and plain-textdbt show; parse JSON lines from failed stdout to recover structured error events.extractDbtErrorto prefer structured JSON error > primary stderr > plain stderr > exception message; throw asdbt show failed: <message>.execDbtShow.Written for commit 2ea03d7. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests