-
Notifications
You must be signed in to change notification settings - Fork 38
fix: altimate-dbt commands fail with hardcoded CI path in published binary
#467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
baa5dfd
fix: patch hardcoded `__dirname` in `dbt-tools` bundle so `altimate-d…
suryaiyer95 2cc41eb
fix: address PR review — add Node 18 fallback and broaden path assert…
suryaiyer95 da04c8e
fix: drop Node 18 fallback — `path` is not in scope before `__dirname…
suryaiyer95 d6c92af
fix: copy `node_python_bridge.py` in `publish.ts` so npm tarball incl…
suryaiyer95 f90f52a
fix: spread real `child_process` in `mock.module` to prevent `execFil…
suryaiyer95 66c7e6c
fix: use __require fallback for __dirname to support Node < 20.11.0
mdesmet 6764fef
style: format `dbt-cli.test.ts` and `publish.ts` with Prettier
anandgupta42 825750b
test: add adversarial tests for `__dirname` patch and build integrity
anandgupta42 9d6336f
Merge branch 'main' into fix/dbt-tools-python-bridge-dirname
anandgupta42 1e8f16b
fix: address code review — existence check, comment fix, better diagn…
anandgupta42 93eba4b
test: expand adversarial tests to 43 cases covering runtime, publish,…
anandgupta42 44b1e81
fix: eliminate flaky CI test failures in dispatcher, dbt, and tracer …
anandgupta42 8260216
fix: replace environment-coupled `runner` assertion with directory ex…
anandgupta42 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,34 @@ | ||
| import { cpSync } from "fs" | ||
| import { cpSync, readFileSync, writeFileSync } from "fs" | ||
| import { dirname, join } from "path" | ||
|
|
||
| const dist = join(import.meta.dir, "..", "dist") | ||
|
|
||
| // 1. Copy altimate_python_packages | ||
| const resolved = require.resolve("@altimateai/dbt-integration") | ||
| const source = join(dirname(resolved), "altimate_python_packages") | ||
| const target = join(import.meta.dir, "..", "dist", "altimate_python_packages") | ||
|
|
||
| cpSync(source, target, { recursive: true }) | ||
| cpSync(source, join(dist, "altimate_python_packages"), { recursive: true }) | ||
| console.log(`Copied altimate_python_packages → dist/`) | ||
|
|
||
| // 2. Copy node_python_bridge.py into dist so it lives next to index.js | ||
| // node_python_bridge.py is shipped in dbt-integration's dist | ||
| const bridgePy = join(dirname(resolved), "node_python_bridge.py") | ||
| cpSync(bridgePy, join(dist, "node_python_bridge.py")) | ||
| console.log(`Copied node_python_bridge.py → dist/`) | ||
|
|
||
| // 3. Fix the hardcoded __dirname that bun bakes at compile time. | ||
| // Replace it with a runtime resolution so the bridge script is found | ||
| // relative to the built index.js, not the CI runner's node_modules. | ||
| const indexPath = join(dist, "index.js") | ||
| let code = readFileSync(indexPath, "utf8") | ||
| const pattern = /var __dirname\s*=\s*"[^"]*python-bridge[^"]*"/ | ||
| if (pattern.test(code)) { | ||
| // import.meta.dirname is supported by Bun and Node >= 20.11.0. | ||
| // Node 18 is EOL (April 2025), so no fallback needed. | ||
| const replacement = `var __dirname = typeof import.meta.dirname === "string" ? import.meta.dirname : __require("path").dirname(__require("url").fileURLToPath(import.meta.url))` | ||
| code = code.replace(pattern, replacement) | ||
| writeFileSync(indexPath, code) | ||
| console.log(`Patched __dirname in dist/index.js`) | ||
| } else { | ||
| console.error(`ERROR: could not find python-bridge __dirname to patch — the bundle format may have changed`) | ||
| process.exit(1) | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,255 @@ | ||
| import { describe, test, expect, beforeAll } from "bun:test" | ||
| import { readFileSync, writeFileSync, mkdtempSync, cpSync, existsSync } from "fs" | ||
| import { join, resolve } from "path" | ||
| import { tmpdir } from "os" | ||
| import { $ } from "bun" | ||
|
|
||
| const dist = join(import.meta.dir, "../dist") | ||
| const scriptDir = join(import.meta.dir, "../script") | ||
|
|
||
| // ─── Helpers ───────────────────────────────────────────────────────── | ||
| // Simulate what copy-python.ts does against arbitrary bundle content, | ||
| // without touching the real dist/index.js. | ||
| function runPatchLogic(bundleContent: string): { patched: boolean; output: string } { | ||
| const pattern = /var __dirname\s*=\s*"[^"]*python-bridge[^"]*"/ | ||
| if (pattern.test(bundleContent)) { | ||
| const replacement = `var __dirname = typeof import.meta.dirname === "string" ? import.meta.dirname : __require("path").dirname(__require("url").fileURLToPath(import.meta.url))` | ||
| return { patched: true, output: bundleContent.replace(pattern, replacement) } | ||
| } | ||
| return { patched: false, output: bundleContent } | ||
| } | ||
|
|
||
| // ─── Adversarial: Regex edge cases ────────────────────────────────── | ||
| describe("adversarial: patch regex", () => { | ||
| test("matches typical CI runner path (Linux)", () => { | ||
| const bundle = `var __dirname = "/home/runner/work/altimate-code/node_modules/.bun/[email protected]/node_modules/python-bridge"` | ||
| const result = runPatchLogic(bundle) | ||
| expect(result.patched).toBe(true) | ||
| expect(result.output).toContain("import.meta.dirname") | ||
| expect(result.output).not.toContain("/home/runner") | ||
| }) | ||
|
|
||
| test("matches macOS dev path", () => { | ||
| const bundle = `var __dirname = "/Users/dev/code/node_modules/python-bridge"` | ||
| const result = runPatchLogic(bundle) | ||
| expect(result.patched).toBe(true) | ||
| expect(result.output).not.toContain("/Users/dev") | ||
| }) | ||
|
|
||
| test("matches Windows CI path", () => { | ||
| const bundle = `var __dirname = "C:\\Users\\runneradmin\\work\\node_modules\\python-bridge"` | ||
| const result = runPatchLogic(bundle) | ||
| expect(result.patched).toBe(true) | ||
| expect(result.output).not.toContain("C:\\Users") | ||
| }) | ||
|
|
||
| test("matches path with @scope in node_modules", () => { | ||
| const bundle = `var __dirname = "/opt/ci/node_modules/.bun/[email protected]/node_modules/python-bridge"` | ||
| const result = runPatchLogic(bundle) | ||
| expect(result.patched).toBe(true) | ||
| }) | ||
|
|
||
| test("does NOT match unrelated __dirname (no python-bridge)", () => { | ||
| const bundle = `var __dirname = "/home/runner/work/some-other-module"` | ||
| const result = runPatchLogic(bundle) | ||
| expect(result.patched).toBe(false) | ||
| // The original hardcoded path is preserved — this is CORRECT behavior. | ||
| // The patch should only touch the python-bridge dirname. | ||
| }) | ||
|
|
||
| test("does NOT match __dirname that's already patched", () => { | ||
| const bundle = `var __dirname = typeof import.meta.dirname === "string" ? import.meta.dirname : __require("path").dirname(__require("url").fileURLToPath(import.meta.url))` | ||
| const result = runPatchLogic(bundle) | ||
| expect(result.patched).toBe(false) | ||
| }) | ||
|
|
||
| test("handles extra whitespace in assignment", () => { | ||
| const bundle = `var __dirname = "/some/path/to/python-bridge"` | ||
| const result = runPatchLogic(bundle) | ||
| expect(result.patched).toBe(true) | ||
| }) | ||
|
|
||
| test("only patches FIRST match (non-global)", () => { | ||
| const bundle = [`var __dirname = "/first/path/python-bridge"`, `var __dirname = "/second/path/python-bridge"`].join( | ||
| "\n", | ||
| ) | ||
| const result = runPatchLogic(bundle) | ||
| expect(result.patched).toBe(true) | ||
| // First one is patched | ||
| expect(result.output).toContain("import.meta.dirname") | ||
| // Second one survives — this is a known limitation | ||
| expect(result.output).toContain("/second/path/python-bridge") | ||
| }) | ||
|
|
||
| test("does NOT match if quotes are single quotes", () => { | ||
| const bundle = `var __dirname = '/home/runner/python-bridge'` | ||
| const result = runPatchLogic(bundle) | ||
| // Bun uses double quotes, so single-quote paths should not match | ||
| expect(result.patched).toBe(false) | ||
| }) | ||
|
|
||
| test("does NOT match let or const declarations", () => { | ||
| const bundle = `const __dirname = "/home/runner/python-bridge"` | ||
| const result = runPatchLogic(bundle) | ||
| // Pattern specifically matches `var __dirname` — const/let won't match | ||
| expect(result.patched).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| // ─── Adversarial: Built output invariants ─────────────────────────── | ||
| describe("adversarial: built bundle invariants", () => { | ||
| beforeAll(async () => { | ||
| // Ensure we have a fresh build | ||
| if (!existsSync(join(dist, "index.js"))) { | ||
| await $`bun run build`.cwd(join(import.meta.dir, "..")) | ||
| } | ||
| }) | ||
|
|
||
| test("exactly ONE __dirname assignment exists in bundle", () => { | ||
| const code = readFileSync(join(dist, "index.js"), "utf8") | ||
| const matches = code.match(/var __dirname\s*=/g) | ||
| expect(matches).not.toBeNull() | ||
| expect(matches!.length).toBe(1) | ||
| }) | ||
|
|
||
| test("__dirname is used to resolve PYTHON_BRIDGE_SCRIPT", () => { | ||
| const code = readFileSync(join(dist, "index.js"), "utf8") | ||
| expect(code).toContain('path.join(__dirname, "node_python_bridge.py")') | ||
| }) | ||
|
|
||
| test("no CI runner paths leaked anywhere in bundle", () => { | ||
| const code = readFileSync(join(dist, "index.js"), "utf8") | ||
| // Common CI runner path prefixes | ||
| expect(code).not.toContain("/home/runner/work/") | ||
| expect(code).not.toContain("/github/workspace/") | ||
| expect(code).not.toContain("D:\\a\\altimate-code\\") | ||
| }) | ||
|
|
||
| test("patched __dirname includes both primary and fallback paths", () => { | ||
| const code = readFileSync(join(dist, "index.js"), "utf8") | ||
| const line = code.split("\n").find((l) => l.includes("var __dirname")) | ||
| expect(line).toBeDefined() | ||
| // Primary: import.meta.dirname | ||
| expect(line).toContain("import.meta.dirname") | ||
| // Fallback: fileURLToPath | ||
| expect(line).toContain("fileURLToPath") | ||
| // Uses __require (Bun's bundled require) | ||
| expect(line).toContain("__require") | ||
| }) | ||
|
|
||
| test("node_python_bridge.py in dist matches the source", () => { | ||
| const resolved = require.resolve("@altimateai/dbt-integration") | ||
| const sourcePy = join(require("path").dirname(resolved), "node_python_bridge.py") | ||
| if (!existsSync(sourcePy)) return // skip if source not available | ||
|
|
||
| const source = readFileSync(sourcePy, "utf8") | ||
| const copied = readFileSync(join(dist, "node_python_bridge.py"), "utf8") | ||
| expect(copied).toBe(source) | ||
| }) | ||
|
|
||
| test("altimate_python_packages directory was copied", () => { | ||
| expect(existsSync(join(dist, "altimate_python_packages"))).toBe(true) | ||
| }) | ||
| }) | ||
|
|
||
| // ─── Adversarial: Runtime resolution simulation ───────────────────── | ||
| describe("adversarial: runtime resolution", () => { | ||
| test("patched __dirname evaluates to a real directory at runtime", () => { | ||
| // import.meta.dirname should resolve to THIS test file's directory | ||
| // In the real bundle, it would resolve to dist/ | ||
| const dirname = import.meta.dir | ||
| expect(typeof dirname).toBe("string") | ||
| expect(dirname.length).toBeGreaterThan(0) | ||
| expect(dirname).not.toContain("runner") | ||
| }) | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| test("node_python_bridge.py is findable relative to dist/index.js", () => { | ||
| // This is the critical runtime check: __dirname should be dist/, | ||
| // and node_python_bridge.py should be in dist/ | ||
| const bridgePath = join(dist, "node_python_bridge.py") | ||
| expect(existsSync(bridgePath)).toBe(true) | ||
|
|
||
| // Verify it's actually a Python file, not garbage | ||
| const content = readFileSync(bridgePath, "utf8") | ||
| expect(content).toContain("import") // Python imports | ||
| expect(content.length).toBeGreaterThan(100) // not truncated | ||
| }) | ||
|
|
||
| test("bundle can be loaded from a DIFFERENT directory without path errors", async () => { | ||
| // Simulate running the binary from /tmp — __dirname should still | ||
| // resolve to where index.js lives, not the CWD | ||
| const tmpDir = mkdtempSync(join(tmpdir(), "adversarial-")) | ||
| const originalCwd = process.cwd() | ||
|
|
||
| try { | ||
| process.chdir(tmpDir) | ||
|
|
||
| // The patched code uses import.meta.dirname which is compile-time | ||
| // resolved to the file's actual location, not CWD. Verify this. | ||
| const indexPath = join(dist, "index.js") | ||
| expect(existsSync(indexPath)).toBe(true) | ||
|
|
||
| // Read the bundle and verify the __dirname line doesn't reference CWD | ||
| const code = readFileSync(indexPath, "utf8") | ||
| expect(code).not.toContain(tmpDir) | ||
| } finally { | ||
| process.chdir(originalCwd) | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| // ─── Adversarial: Double-patch protection ─────────────────────────── | ||
| describe("adversarial: idempotency", () => { | ||
| test("running the patch twice does not corrupt the bundle", () => { | ||
| const original = `var __dirname = "/ci/path/python-bridge"` | ||
|
|
||
| // First patch | ||
| const first = runPatchLogic(original) | ||
| expect(first.patched).toBe(true) | ||
|
|
||
| // Second patch on already-patched output — should be a no-op | ||
| const second = runPatchLogic(first.output) | ||
| expect(second.patched).toBe(false) | ||
| expect(second.output).toBe(first.output) | ||
| }) | ||
|
|
||
| test("patch output is syntactically valid JS", () => { | ||
| const bundle = `var __dirname = "/home/runner/python-bridge";` | ||
| const result = runPatchLogic(bundle) | ||
|
|
||
| // The patched code should not have unmatched quotes or parens | ||
| const openParens = (result.output.match(/\(/g) || []).length | ||
| const closeParens = (result.output.match(/\)/g) || []).length | ||
| expect(openParens).toBe(closeParens) | ||
|
|
||
| const openQuotes = (result.output.match(/"/g) || []).length | ||
| expect(openQuotes % 2).toBe(0) // even number of quotes | ||
| }) | ||
| }) | ||
|
|
||
| // ─── Adversarial: CI smoke test alignment ─────────────────────────── | ||
| describe("adversarial: CI smoke test parity", () => { | ||
| test("CI regex catches what build-integrity test catches", () => { | ||
| // CI uses: grep -qE 'var __dirname\\s*=\\s*"(/|[A-Za-z]:\\\\)' (shell regex) | ||
| // Test uses: /var __dirname\s*=\s*"(?:[A-Za-z]:\\\\|\/)/ | ||
| // Both should flag the same hardcoded paths | ||
|
|
||
| const linuxPath = `var __dirname = "/home/runner/python-bridge"` | ||
| const windowsPath = `var __dirname = "C:\\\\Users\\\\runner\\\\python-bridge"` | ||
| const patchedPath = `var __dirname = typeof import.meta.dirname === "string" ? import.meta.dirname : __require("path").dirname(__require("url").fileURLToPath(import.meta.url))` | ||
|
|
||
| const ciRegex = /var __dirname\s*=\s*"(\/|[A-Za-z]:\\)/ | ||
| const testRegex = /var __dirname\s*=\s*"(?:[A-Za-z]:\\\\|\/)/ | ||
|
|
||
| // Both should flag hardcoded paths | ||
| expect(ciRegex.test(linuxPath)).toBe(true) | ||
| expect(testRegex.test(linuxPath)).toBe(true) | ||
|
|
||
| expect(ciRegex.test(windowsPath)).toBe(true) | ||
| expect(testRegex.test(windowsPath)).toBe(true) | ||
|
|
||
| // Neither should flag the patched version | ||
| expect(ciRegex.test(patchedPath)).toBe(false) | ||
| expect(testRegex.test(patchedPath)).toBe(false) | ||
| }) | ||
| }) | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import { describe, test, expect, beforeAll } from "bun:test" | ||
| import { existsSync, readFileSync } from "fs" | ||
| import { join } from "path" | ||
| import { $ } from "bun" | ||
|
|
||
| const dist = join(import.meta.dir, "../dist") | ||
|
|
||
| describe("build integrity", () => { | ||
| beforeAll(async () => { | ||
| // Rebuild to test the actual build output | ||
| await $`bun run build`.cwd(join(import.meta.dir, "..")) | ||
| }) | ||
|
|
||
| test("node_python_bridge.py exists in dist", () => { | ||
| expect(existsSync(join(dist, "node_python_bridge.py"))).toBe(true) | ||
| }) | ||
|
|
||
| test("no hardcoded absolute paths in __dirname", () => { | ||
| const code = readFileSync(join(dist, "index.js"), "utf8") | ||
| // Catch both Unix ("/...") and Windows ("C:\\...") hardcoded paths | ||
| expect(code).not.toMatch(/var __dirname\s*=\s*"(?:[A-Za-z]:\\\\|\/)/) | ||
| }) | ||
|
|
||
| test("__dirname is patched to runtime resolution", () => { | ||
| const code = readFileSync(join(dist, "index.js"), "utf8") | ||
| expect(code).toContain("import.meta.dirname") | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.