fix: altimate-dbt commands fail with hardcoded CI path in published binary#467
Merged
anandgupta42 merged 13 commits intomainfrom Mar 26, 2026
Merged
fix: altimate-dbt commands fail with hardcoded CI path in published binary#467anandgupta42 merged 13 commits intomainfrom
altimate-dbt commands fail with hardcoded CI path in published binary#467anandgupta42 merged 13 commits intomainfrom
Conversation
…bt` works in published releases `bun build` replaces `__dirname` with a compile-time constant when bundling `python-bridge` (transitive dep of `@altimateai/dbt-integration`). In CI this bakes `/home/runner/work/...` into the bundle, causing `altimate-dbt build` and all Python-bridge commands to fail with ENOENT on every user's machine. Fix: - Copy `node_python_bridge.py` into `dist/` alongside `index.js` - Post-process the bundle to replace the frozen path with `import.meta.dirname` - Fail the build if the patch pattern isn't found (safety net) - Add CI smoke test to prevent regression Broken since PR #201. Closes #466. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCopies Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI (release workflow)
participant Builder as bun build (dbt-tools)
participant CopyScript as copy-python.ts
participant Test as build-integrity.test / adversarial tests
participant PublishPack as packages/opencode/script/publish.ts
CI->>Builder: trigger build of packages/dbt-tools
Builder->>CopyScript: run post-build copy/patch steps
CopyScript->>Builder: copy node_python_bridge.py into dist and patch dist/index.js
CopyScript-->>Builder: emit patched dist artifacts
CI->>Test: run smoke tests / build-integrity tests against dist
Test-->>CI: pass/fail result
CI->>PublishPack: package assets (includes dbt-tools/dist/*) if smoke test passed
PublishPack-->>CI: proceed to npm publish
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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 |
There was a problem hiding this comment.
Pull request overview
Fixes a regression in the @altimateai/dbt-tools bundled output where bun build freezes __dirname to a CI runner path, breaking altimate-dbt commands that rely on python-bridge.
Changes:
- Adds a post-build script to copy
node_python_bridge.pyintodist/and patch the frozen__dirnamein the bundle. - Adds a build-integrity test to prevent regressions (no CI path leaks;
node_python_bridge.pypresent; patched dirname marker). - Adds a release-workflow smoke test to validate the dbt-tools bundle before publishing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/dbt-tools/script/copy-python.ts |
Copies Python assets into dist/ and patches the bundled __dirname string. |
packages/dbt-tools/test/build-integrity.test.ts |
Rebuilds and asserts the dist/ output doesn’t contain CI-path leaks and includes required Python bridge file. |
packages/dbt-tools/package.json |
Includes the new build-integrity test in the package test suite. |
.github/workflows/release.yml |
Adds a publish-time smoke test to catch bundle regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
165-173: Harden smoke checks to catch any hardcoded absolute__dirnamepath.Line 165 currently keys on a specific CI substring. Consider also failing on any absolute
var __dirname = "..."assignment and using fixed-string grep for literals.Suggested workflow hardening
- if grep -q 'home/runner' packages/dbt-tools/dist/index.js; then + if grep -Fq 'home/runner' packages/dbt-tools/dist/index.js; then echo "::error::dbt-tools bundle contains hardcoded CI runner path" exit 1 fi # Verify __dirname was patched to runtime resolution - if ! grep -q 'import.meta.dirname' packages/dbt-tools/dist/index.js; then + if ! grep -Fq 'import.meta.dirname' packages/dbt-tools/dist/index.js; then echo "::error::dbt-tools bundle missing import.meta.dirname patch" exit 1 fi + # Verify no absolute __dirname assignment remains (unix/windows) + if grep -Eq 'var __dirname[[:space:]]*=[[:space:]]*"([A-Za-z]:\\\\|/)' packages/dbt-tools/dist/index.js; then + echo "::error::dbt-tools bundle still has hardcoded absolute __dirname" + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 165 - 173, The smoke-checks should fail on any hardcoded absolute __dirname assignment and use fixed-string matching; update the checks that inspect packages/dbt-tools/dist/index.js to (1) use fixed-string grep (-F) for the existing home/runner check, and (2) add a regex grep (eg. grep -E) to detect literal assignments like var __dirname = "..." that start with an absolute path (patterns matching leading / or Windows drive letters, e.g. grep -E 'var __dirname = ["'\''](/|[A-Za-z]:\\)') and fail if found; keep the existing import.meta.dirname check (searching for import.meta.dirname) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dbt-tools/script/copy-python.ts`:
- Line 25: The patch sets __dirname via import.meta.dirname in the line with
code.replace(pattern, `var __dirname = import.meta.dirname`), which requires
Node >=20.11.0; either add an engines constraint to the package (add "engines":
{ "node": ">=20.11.0" } in the package.json for this package) or change the
generated replacement to a backwards-compatible fallback that uses
path.dirname(fileURLToPath(import.meta.url)) when import.meta.dirname isn't
available; update the code generation/replace logic in copy-python.ts (the
code.replace(...) call) to emit the fallback expression or ensure package.json
contains the engines.node minimum.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 165-173: The smoke-checks should fail on any hardcoded absolute
__dirname assignment and use fixed-string matching; update the checks that
inspect packages/dbt-tools/dist/index.js to (1) use fixed-string grep (-F) for
the existing home/runner check, and (2) add a regex grep (eg. grep -E) to detect
literal assignments like var __dirname = "..." that start with an absolute path
(patterns matching leading / or Windows drive letters, e.g. grep -E 'var
__dirname = ["'\''](/|[A-Za-z]:\\)') and fail if found; keep the existing
import.meta.dirname check (searching for import.meta.dirname) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 99a24cac-e090-40ef-8693-99382a5406b7
📒 Files selected for processing (4)
.github/workflows/release.ymlpackages/dbt-tools/package.jsonpackages/dbt-tools/script/copy-python.tspackages/dbt-tools/test/build-integrity.test.ts
suryaiyer95
changed the title
__dirname in dbt-tools bundlealtimate-dbt commands fail with hardcoded CI path in published binary
…ions - Use `fileURLToPath(import.meta.url)` fallback for Node < 20.11.0 - Broaden smoke test and build integrity checks to catch any hardcoded absolute path (Unix + Windows), not just `home/runner`
…` in bundle GLM-5 review correctly identified that `path` is defined AFTER `__dirname` in the bundled output, so the `path.dirname(fileURLToPath(...))` fallback would throw `ReferenceError` on Node < 20.11.0. Since Node 18 is EOL (April 2025), use `import.meta.dirname` unconditionally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…udes it `copyAssets` copied `dbt-tools/dist/index.js` but not the `.py` file the patched `__dirname` resolves to. Without this, the fix would still break in published packages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eSync` leak
`mock.module("child_process")` in `dbt-cli.test.ts` only provided `execFile`,
causing `dbt-resolve.test.ts` to fail with `Export named 'execFileSync' not found`
when Bun leaks the mock across test files.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
`import.meta.dirname` is unavailable before Node 20.11.0. The previous fallback was dropped because `path`/`fileURLToPath` weren't in scope at that point in the bun-generated __commonJS IIFE. Using `__require` (a module-level closure bun always emits) works at any Node version. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
22 tests covering regex edge cases, runtime resolution, idempotency, CI smoke test parity, and built bundle invariants. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/dbt-tools/test/build-adversarial.test.ts (1)
101-105: Avoid staledistartifacts in adversarial invariantsThe setup says “fresh build” but only rebuilds when
dist/index.jsis missing. Ifdistexists but is stale, these tests may validate old output. Consider always rebuilding (or cleaning first) in this suite.Proposed fix
beforeAll(async () => { - // Ensure we have a fresh build - if (!existsSync(join(dist, "index.js"))) { - await $`bun run build`.cwd(join(import.meta.dir, "..")) - } + // Ensure we have a fresh build for deterministic invariants + await $`bun run build`.cwd(join(import.meta.dir, "..")) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dbt-tools/test/build-adversarial.test.ts` around lines 101 - 105, The beforeAll setup in build-adversarial.test.ts currently only runs the build when existsSync(join(dist, "index.js")) is false, which can leave stale artifacts; modify the beforeAll block (the setup surrounding the existsSync check) to ensure a fresh build every run by either removing/cleaning the dist directory before building or by always invoking the build command unconditionally (e.g., always run `bun run build` in the same cwd used now) so tests never use stale dist/index.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dbt-tools/test/build-adversarial.test.ts`:
- Around line 157-164: The test "patched __dirname evaluates to a real directory
at runtime" currently asserts dirname.not.toContain("runner"), which is
environment-coupled and flaky; remove that substring exclusion and instead
validate dirname as a real directory (e.g. using import.meta.dir value stored in
dirname and verifying it's a non-empty string, an absolute path, and that it
exists/is a directory via fs.existsSync + fs.statSync(...).isDirectory()).
Update the assertions around import.meta.dir / dirname in this test to check
directory validity rather than excluding "runner".
---
Nitpick comments:
In `@packages/dbt-tools/test/build-adversarial.test.ts`:
- Around line 101-105: The beforeAll setup in build-adversarial.test.ts
currently only runs the build when existsSync(join(dist, "index.js")) is false,
which can leave stale artifacts; modify the beforeAll block (the setup
surrounding the existsSync check) to ensure a fresh build every run by either
removing/cleaning the dist directory before building or by always invoking the
build command unconditionally (e.g., always run `bun run build` in the same cwd
used now) so tests never use stale dist/index.js.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 90a5eb0f-b3a1-435a-9cfa-9deba7726be6
📒 Files selected for processing (1)
packages/dbt-tools/test/build-adversarial.test.ts
…ostics - Add `existsSync` check before copying `node_python_bridge.py` with a clear error message if the file is missing from the dependency - Fix misleading comment that said "no fallback needed" while fallback code existed — now accurately describes the `__require` fallback - Log the nearest `__dirname` match on pattern mismatch to aid debugging Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… and mutation New test categories: - Script execution: verify exit codes, progress output, build determinism - Bundle runtime structure: `__require` origin, `__commonJS` scope, spawn chain - Publish pipeline: patched artifacts integrity before copy - Mutation testing: verify guards catch removal of key fix components - Regex performance: no catastrophic backtracking on 100KB+ input - Malformed inputs: unicode, spaces, special chars, surrounding code preservation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tests Root causes: - `dispatcher.test.ts`: Bun's multi-file runner leaks `_ensureRegistered` hook from other files' `native/index.ts` imports. `reset()` clears handlers but not the hook, so `call()` triggers lazy registration instead of throwing. Fix: clear hook in `beforeEach`. - `dbt-first-execution.test.ts`: `mock.module` for DuckDB leaked across files. Fix: spread real module exports + ensure native/index.ts import. - `tracing-adversarial-final.test.ts`: 50ms snapshot waits too tight for CI under load. Fix: increase to 200ms/300ms. Result: 0 failures in full suite (4961 pass, 340 skip). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…istence check
Address CodeRabbit review: `expect(dirname).not.toContain("runner")`
would fail on GitHub CI where test runs under `/home/runner/`. Use
`existsSync(dirname)` to validate the directory is real instead.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
altimate-dbtcommands failing in published binaries becausebun buildfreezes__dirnameas a compile-time string, baking the CI runner path (/home/runner/work/...) into every releasealtimate-dbtcommands that use the Python bridge (build,run,test,compile,execute) have been broken since PR feat: consolidate dbt skills and add altimate-dbt CLI package #201node_python_bridge.pyintodist/and patches the frozen__dirnametoimport.meta.dirname(runtime resolution)Type of change
Issue for this PR
Closes #466
How did you verify your code works?
bun run buildprints "Patched __dirname"grep 'var __dirname' dist/index.jsshowsimport.meta.dirname, not a hardcoded pathnode_python_bridge.pyexists indist/home/runnerstrings in bundleChecklist
Summary by CodeRabbit
Tests
Chores