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 1 commit 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/dbt-tools/test/dbt-cli.test.ts">
<violation number="1" location="packages/dbt-tools/test/dbt-cli.test.ts:205">
P3: Duplicate test: "preserves generic 'Could not parse' when dbt exited 0 but output unparseable" tests the exact same scenario as the existing "Tier 3: throws with helpful message when all tiers fail" test. Both use an identical mock (cb(null, "some unparseable output", "")) and assert the same rejection string. The new test adds zero coverage and creates maintenance overhead — any change to the generic-error path must be kept in sync across two identical tests.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| cb(null, "some unparseable output", "") | ||
| }) | ||
|
|
||
| await expect(execDbtShow("SELECT 1")).rejects.toThrow("Could not parse dbt show output in any format") |
There was a problem hiding this comment.
P3: Duplicate test: "preserves generic 'Could not parse' when dbt exited 0 but output unparseable" tests the exact same scenario as the existing "Tier 3: throws with helpful message when all tiers fail" test. Both use an identical mock (cb(null, "some unparseable output", "")) and assert the same rejection string. The new test adds zero coverage and creates maintenance overhead — any change to the generic-error path must be kept in sync across two identical tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/dbt-tools/test/dbt-cli.test.ts, line 205:
<comment>Duplicate test: "preserves generic 'Could not parse' when dbt exited 0 but output unparseable" tests the exact same scenario as the existing "Tier 3: throws with helpful message when all tiers fail" test. Both use an identical mock (cb(null, "some unparseable output", "")) and assert the same rejection string. The new test adds zero coverage and creates maintenance overhead — any change to the generic-error path must be kept in sync across two identical tests.</comment>
<file context>
@@ -149,6 +149,73 @@ describe("execDbtShow", () => {
+ cb(null, "some unparseable output", "")
+ })
+
+ await expect(execDbtShow("SELECT 1")).rejects.toThrow("Could not parse dbt show output in any format")
+ })
+
</file context>
`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 |
2 participants
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 064b0dd. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
dbt show: surfaces actual dbt errors instead of generic "Could not parse" messages, prioritizes structured JSON error events, and includes stderr or execution failure details in thrown messages.Tests