Repository navigation
fix(opencode): put altimate-dbt on PATH after npm install, with or without postinstall #1702
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,23 @@ function run(target) { | |
| // available to agents without manual PATH configuration. | ||
| env.ALTIMATE_BIN_DIR = scriptDir | ||
|
|
||
| // altimate_change start — #982: put the bundled dbt toolkit on PATH for the whole app. | ||
| // Agent commands, the built-in terminal and the dbt-schema-verify validator (which | ||
| // spawns `altimate-dbt` with the app's own PATH) all inherit this. postinstall normally | ||
| // links altimate-dbt into bin/; when lifecycle scripts were skipped (--ignore-scripts, | ||
| // pnpm/bun allow-lists) fall back to the bundled dbt-tools/bin, so the agent doesn't | ||
| // silently drop to plain dbt. | ||
| const pathKey = Object.keys(env).find((k) => k.toUpperCase() === "PATH") || "PATH" | ||
| const toolDirs = [scriptDir] | ||
| const dbtToolsBin = path.join(scriptDir, "..", "dbt-tools", "bin") | ||
| if (!fs.existsSync(path.join(scriptDir, "altimate-dbt")) && fs.existsSync(path.join(dbtToolsBin, "altimate-dbt"))) { | ||
| toolDirs.push(dbtToolsBin) | ||
| } | ||
|
Comment on lines
+51
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const currentPath = (env[pathKey] || "").split(path.delimiter).filter(Boolean) | ||
| // Tool dirs go first even if already on PATH further down, so an older altimate-dbt cannot shadow ours. | ||
| env[pathKey] = [...toolDirs, ...currentPath.filter((d) => !toolDirs.includes(d))].join(path.delimiter) | ||
| // altimate_change end | ||
|
|
||
| try { | ||
| const resolvedTarget = fs.realpathSync(target) | ||
| const targetDir = path.dirname(path.dirname(resolvedTarget)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { describe, test, expect, afterEach } from "bun:test" | ||
| import path from "path" | ||
| import fs from "fs" | ||
| import { spawnSync } from "child_process" | ||
| import { | ||
| installTmpdir, | ||
| createBinaryPackage, | ||
|
|
@@ -30,6 +31,122 @@ function copyBinWrapper(destDir: string): string { | |
| return wrapperPath | ||
| } | ||
|
|
||
| // altimate_change start — #982: the launcher puts the bundled dbt toolkit on the app's PATH | ||
| describe.each([ | ||
| ["bin/altimate-code", BIN_WRAPPER_SCRIPT], | ||
| ["bin/altimate", path.join(path.dirname(BIN_WRAPPER_SCRIPT), "altimate")], | ||
| ])("%s puts altimate-dbt on the app's PATH (#982)", (_name, wrapperSource) => { | ||
| // Published layout: <pkg>/bin/<wrapper> and <pkg>/dbt-tools/bin/altimate-dbt. | ||
| function makeLayout(dir: string, opts: { linkedIntoBin: boolean }) { | ||
| const pkg = path.join(dir, "node_modules", "altimate-code") | ||
| const binDir = path.join(pkg, "bin") | ||
| const dbtToolsBin = path.join(pkg, "dbt-tools", "bin") | ||
| fs.mkdirSync(binDir, { recursive: true }) | ||
| fs.mkdirSync(dbtToolsBin, { recursive: true }) | ||
| const wrapperPath = path.join(binDir, path.basename(wrapperSource)) | ||
| fs.copyFileSync(wrapperSource, wrapperPath) | ||
| fs.writeFileSync(path.join(dbtToolsBin, "altimate-dbt"), '#!/bin/sh\necho "bundled-dbt-tools"') | ||
| fs.chmodSync(path.join(dbtToolsBin, "altimate-dbt"), 0o755) | ||
| if (opts.linkedIntoBin) { | ||
| fs.writeFileSync(path.join(binDir, "altimate-dbt"), '#!/bin/sh\necho "linked-by-postinstall"') | ||
| fs.chmodSync(path.join(binDir, "altimate-dbt"), 0o755) | ||
| } | ||
| // Stand-in for the compiled app: runs altimate-dbt from PATH, then prints PATH. | ||
| const app = path.join(dir, "app") | ||
| fs.writeFileSync(app, '#!/bin/sh\naltimate-dbt\necho "PATH=$PATH"') | ||
| fs.chmodSync(app, 0o755) | ||
| return { wrapperPath, binDir, dbtToolsBin: fs.realpathSync(dbtToolsBin), app } | ||
| } | ||
|
|
||
| unixtest("falls back to dbt-tools/bin when postinstall did not link altimate-dbt", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: These launcher tests never execute on Windows because Prompt for AI agents |
||
| const { dir, cleanup: c } = installTmpdir() | ||
| cleanup = c | ||
| const { wrapperPath, app } = makeLayout(dir, { linkedIntoBin: false }) | ||
|
|
||
| const result = runBinWrapper(wrapperPath, [], { ALTIMATE_CODE_BIN_PATH: app }) | ||
| expect(result.exitCode).toBe(0) | ||
| expect(result.stdout).toContain("bundled-dbt-tools") | ||
| }) | ||
|
|
||
| unixtest("uses the postinstall-linked altimate-dbt and does not add dbt-tools/bin", () => { | ||
| const { dir, cleanup: c } = installTmpdir() | ||
| cleanup = c | ||
| const { wrapperPath, dbtToolsBin, app } = makeLayout(dir, { linkedIntoBin: true }) | ||
|
|
||
| const result = runBinWrapper(wrapperPath, [], { ALTIMATE_CODE_BIN_PATH: app }) | ||
| expect(result.exitCode).toBe(0) | ||
| expect(result.stdout).toContain("linked-by-postinstall") | ||
| expect(result.stdout).not.toContain(dbtToolsBin) | ||
| }) | ||
|
|
||
| unixtest("keeps the user's PATH and does not duplicate the bin folder", () => { | ||
| const { dir, cleanup: c } = installTmpdir() | ||
| cleanup = c | ||
| const { wrapperPath, app } = makeLayout(dir, { linkedIntoBin: true }) | ||
| const realBinDir = path.dirname(fs.realpathSync(wrapperPath)) | ||
|
|
||
| const result = runBinWrapper(wrapperPath, [], { | ||
| ALTIMATE_CODE_BIN_PATH: app, | ||
| PATH: [realBinDir, process.env.PATH ?? ""].join(path.delimiter), | ||
| }) | ||
| expect(result.exitCode).toBe(0) | ||
| const pathLine = result.stdout.split("\n").find((l) => l.startsWith("PATH="))! | ||
| const entries = pathLine.slice("PATH=".length).split(path.delimiter) | ||
| expect(entries.filter((e) => e === realBinDir).length).toBe(1) | ||
| for (const original of (process.env.PATH ?? "").split(path.delimiter).filter(Boolean)) { | ||
| expect(entries).toContain(original) | ||
| } | ||
| }) | ||
|
|
||
| // Ours must win even when our folder is already on PATH behind an older altimate-dbt. | ||
| unixtest("moves the tool folders ahead of an older altimate-dbt already on PATH", () => { | ||
| const { dir, cleanup: c } = installTmpdir() | ||
| cleanup = c | ||
| const { wrapperPath, dbtToolsBin, app } = makeLayout(dir, { linkedIntoBin: false }) | ||
| const stale = path.join(dir, "stale") | ||
| fs.mkdirSync(stale) | ||
| fs.writeFileSync(path.join(stale, "altimate-dbt"), '#!/bin/sh\necho "stale-altimate-dbt"') | ||
| fs.chmodSync(path.join(stale, "altimate-dbt"), 0o755) | ||
|
|
||
| const result = runBinWrapper(wrapperPath, [], { | ||
| ALTIMATE_CODE_BIN_PATH: app, | ||
| PATH: [stale, dbtToolsBin, process.env.PATH ?? ""].join(path.delimiter), | ||
| }) | ||
| expect(result.exitCode).toBe(0) | ||
| expect(result.stdout).toContain("bundled-dbt-tools") | ||
| expect(result.stdout).not.toContain("stale-altimate-dbt") | ||
| const pathLine = result.stdout.split("\n").find((l) => l.startsWith("PATH="))! | ||
| expect(pathLine.slice("PATH=".length).split(path.delimiter).filter((e) => e === dbtToolsBin).length).toBe(1) | ||
| }) | ||
|
|
||
| // Windows commonly spells the variable `Path`. The launcher must extend that key, not add a second `PATH`. | ||
| unixtest("extends a mixed-case Path key instead of adding PATH", () => { | ||
| const { dir, cleanup: c } = installTmpdir() | ||
| cleanup = c | ||
| const { wrapperPath, dbtToolsBin } = makeLayout(dir, { linkedIntoBin: false }) | ||
| const app = path.join(dir, "print-env") | ||
| fs.writeFileSync(app, '#!/bin/sh\necho "Path=$Path"\necho "PATH=$PATH"') | ||
| fs.chmodSync(app, 0o755) | ||
| const env: Record<string, string> = { ...process.env, ALTIMATE_CODE_BIN_PATH: app, Path: "/original/bin" } as Record< | ||
| string, | ||
| string | ||
| > | ||
| delete env.PATH | ||
| delete env.ALTIMATE_BIN_DIR | ||
|
|
||
| const result = spawnSync(process.execPath, [wrapperPath], { encoding: "utf-8", timeout: 10_000, env }) | ||
| expect(result.status).toBe(0) | ||
| const pathLine = result.stdout.split("\n").find((l) => l.startsWith("Path="))! | ||
| const entries = pathLine.slice("Path=".length).split(path.delimiter) | ||
| expect(entries).toContain(dbtToolsBin) | ||
| expect(entries.at(-1)).toBe("/original/bin") | ||
| // No PATH key was created: /bin/sh fills in its own default PATH, which never holds our folders. | ||
| const upperLine = result.stdout.split("\n").find((l) => l.startsWith("PATH=")) ?? "" | ||
| expect(upperLine).not.toContain(dbtToolsBin) | ||
| }) | ||
| }) | ||
| // altimate_change end | ||
|
|
||
| describe("bin/altimate-code wrapper", () => { | ||
| unixtest("uses ALTIMATE_CODE_BIN_PATH env var when set", () => { | ||
| const { dir, cleanup: c } = installTmpdir() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| /** | ||
| * #982 — `altimate-dbt` must be on PATH after `npm install -g altimate-code`, | ||
| * even when lifecycle scripts never run. | ||
| * | ||
| * Before the fix the published package.json only declared `altimate` and | ||
| * `altimate-code` as bins. `altimate-dbt` was reachable only through the | ||
| * postinstall symlink into the package's own bin/ folder, so: | ||
| * - users never got `altimate-dbt` in their shell, and | ||
| * - with --ignore-scripts (or pnpm/bun script allow-lists) the agent lost it too. | ||
| * | ||
| * The e2e tests pack a package with the published layout, install it globally | ||
| * into a temp prefix with --ignore-scripts, and run `altimate-dbt` from PATH. | ||
| * They install a local tarball with no dependencies, so they need npm but no network. | ||
| */ | ||
| import { describe, test, expect } from "bun:test" | ||
| import fs from "fs" | ||
| import os from "os" | ||
| import path from "path" | ||
| import { spawnSync } from "child_process" | ||
|
|
||
| const REPO_PKG_DIR = path.resolve(import.meta.dir, "../..") | ||
| const REPO_ROOT = path.resolve(REPO_PKG_DIR, "../..") | ||
| const DBT_BIN_SOURCE = path.join(REPO_ROOT, "packages/dbt-tools/bin/altimate-dbt") | ||
| const PUBLISH_SCRIPT = fs.readFileSync(path.join(REPO_PKG_DIR, "script/publish.ts"), "utf-8") | ||
| const DBT_BIN_ENTRY = '"altimate-dbt": "./dbt-tools/bin/altimate-dbt"' | ||
|
|
||
| // Bound every npm call so a stalled registry or cache lock fails the test instead of hanging CI. | ||
| const NPM_TIMEOUT_MS = 120_000 | ||
| const npm = spawnSync("npm", ["--version"], { encoding: "utf-8", timeout: 10_000 }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const HAS_NPM = npm.status === 0 | ||
| const IS_WINDOWS = process.platform === "win32" | ||
|
|
||
| describe("publish.ts declares altimate-dbt as a bin (#982)", () => { | ||
| test("both the scoped and unscoped package.json declare altimate-dbt", () => { | ||
| const occurrences = PUBLISH_SCRIPT.split(DBT_BIN_ENTRY).length - 1 | ||
| expect(occurrences).toBe(2) | ||
| }) | ||
|
|
||
| test("the bin target is the file copyAssets ships", () => { | ||
| expect(PUBLISH_SCRIPT).toContain("cp ../dbt-tools/bin/altimate-dbt ${targetDir}/dbt-tools/bin/altimate-dbt") | ||
| expect(fs.existsSync(DBT_BIN_SOURCE)).toBe(true) | ||
| }) | ||
| }) | ||
|
|
||
| /** Build a package directory with the published layout and return its path. */ | ||
| function makePublishedLayout(root: string, opts: { declareDbtBin: boolean }): string { | ||
| const pkg = path.join(root, "altimate-code") | ||
| fs.mkdirSync(path.join(pkg, "bin"), { recursive: true }) | ||
| fs.mkdirSync(path.join(pkg, "dbt-tools", "bin"), { recursive: true }) | ||
| fs.mkdirSync(path.join(pkg, "dbt-tools", "dist"), { recursive: true }) | ||
|
|
||
| fs.writeFileSync(path.join(pkg, "bin", "altimate"), "#!/usr/bin/env node\nconsole.log('altimate')\n", { mode: 0o755 }) | ||
| fs.writeFileSync(path.join(pkg, "bin", "altimate-code"), "#!/usr/bin/env node\nconsole.log('altimate-code')\n", { | ||
| mode: 0o755, | ||
| }) | ||
| // The real launcher shipped by copyAssets, so the test tracks its import path. | ||
| fs.copyFileSync(DBT_BIN_SOURCE, path.join(pkg, "dbt-tools", "bin", "altimate-dbt")) | ||
| fs.chmodSync(path.join(pkg, "dbt-tools", "bin", "altimate-dbt"), 0o755) | ||
| fs.writeFileSync(path.join(pkg, "dbt-tools", "package.json"), JSON.stringify({ type: "module" }) + "\n") | ||
| fs.writeFileSync( | ||
| path.join(pkg, "dbt-tools", "dist", "index.js"), | ||
| 'import { argv } from "node:process"\nconsole.log(JSON.stringify({ ok: true, args: argv.slice(2) }))\n', | ||
| ) | ||
|
|
||
| const bin: Record<string, string> = { altimate: "./bin/altimate", "altimate-code": "./bin/altimate-code" } | ||
| if (opts.declareDbtBin) bin["altimate-dbt"] = "./dbt-tools/bin/altimate-dbt" | ||
| fs.writeFileSync( | ||
| path.join(pkg, "package.json"), | ||
| JSON.stringify( | ||
| { | ||
| name: "altimate-code", | ||
| version: "0.0.0-test", | ||
| bin, | ||
| // Must NOT run: proves the bin works without lifecycle scripts. | ||
| scripts: { postinstall: "node -e \"process.exit(1)\"" }, | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| ) | ||
| return pkg | ||
| } | ||
|
|
||
| /** npm pack + npm install -g --ignore-scripts into a temp prefix; returns the prefix bin dir. */ | ||
| function packAndInstall(root: string, pkgDir: string): string { | ||
| const packed = spawnSync("npm", ["pack", "--silent", "--pack-destination", root], { | ||
|
ekkundia-build marked this conversation as resolved.
Resolved
|
||
| cwd: pkgDir, | ||
| encoding: "utf-8", | ||
| timeout: NPM_TIMEOUT_MS, | ||
| }) | ||
| expect(packed.status).toBe(0) | ||
| const tarball = path.join(root, packed.stdout.trim().split("\n").pop()!) | ||
| const prefix = path.join(root, "prefix") | ||
| const installed = spawnSync( | ||
| "npm", | ||
| ["install", "-g", "--prefix", prefix, "--ignore-scripts", "--no-audit", "--no-fund", "--offline", tarball], | ||
| { encoding: "utf-8", timeout: NPM_TIMEOUT_MS }, | ||
| ) | ||
| expect(installed.status).toBe(0) | ||
| return path.join(prefix, "bin") | ||
| } | ||
|
|
||
| function runFromPath(binDir: string, command: string, args: string[]) { | ||
| return spawnSync(command, args, { | ||
| cwd: os.tmpdir(), | ||
| encoding: "utf-8", | ||
| timeout: 30_000, | ||
| // The install prefix first, then the normal PATH (the launcher needs `node` via env). | ||
| env: { ...process.env, PATH: [binDir, process.env.PATH ?? ""].join(path.delimiter) }, | ||
| }) | ||
| } | ||
|
|
||
| describe.skipIf(!HAS_NPM || IS_WINDOWS)("altimate-dbt on PATH after global install (#982)", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This skips the only end-to-end install test on Windows, so regressions in the Windows npm shim and PATH behavior can pass CI unnoticed. Run the test on Windows with the generated Prompt for AI agents |
||
| test("control: without the bin entry, altimate-dbt is not on PATH (the bug)", () => { | ||
| const root = fs.mkdtempSync(path.join(os.tmpdir(), "dbt-on-path-before-")) | ||
| try { | ||
| const binDir = packAndInstall(root, makePublishedLayout(root, { declareDbtBin: false })) | ||
| expect(fs.existsSync(path.join(binDir, "altimate"))).toBe(true) | ||
| expect(fs.existsSync(path.join(binDir, "altimate-dbt"))).toBe(false) | ||
| } finally { | ||
| fs.rmSync(root, { recursive: true, force: true }) | ||
| } | ||
| }) | ||
|
|
||
| test("with the bin entry, altimate-dbt is linked and runs from any directory, with scripts skipped", () => { | ||
| const root = fs.mkdtempSync(path.join(os.tmpdir(), "dbt-on-path-after-")) | ||
| try { | ||
| const binDir = packAndInstall(root, makePublishedLayout(root, { declareDbtBin: true })) | ||
| expect(fs.existsSync(path.join(binDir, "altimate-dbt"))).toBe(true) | ||
|
|
||
| const result = runFromPath(binDir, "altimate-dbt", ["doctor"]) | ||
| expect(result.status).toBe(0) | ||
| expect(JSON.parse(result.stdout.trim())).toEqual({ ok: true, args: ["doctor"] }) | ||
| } finally { | ||
| fs.rmSync(root, { recursive: true, force: true }) | ||
| } | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: Duplicated PATH-setup block
This block is copied verbatim into
bin/altimate-code, so thescriptDir/dbt-tools/binprecedence, the case-insensitivePATHkey lookup, and the dedupe rule now live in two places and must be changed together. Both launchers already duplicaterun()andfindBinary(); a small shared helper (e.g.bin/env.cjsrequired via__dirname) would keep this new logic in sync and is still shippable becausepublish.tscopies the wholebin/directory.One factual note for the comment above:
path.join(scriptDir, "..", "dbt-tools", "bin")is normalized, so on Windows the fallback directory only contains the extensionless Node shim (no.cmd). That is fine because the npmbinentry is the Windows path, but the fallback itself is effectively POSIX-only.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, and you’re right that publish.ts copies the whole bin/ folder, so a helper would ship. I kept it as a small altimate_change block in each launcher because these files track upstream opencode, and a shared module adds another file to reconcile on upstream merges. Happy to extract it into bin/env.cjs if maintainers prefer. The Windows note matches the description: the bin entry is the Windows path, and the fallback is effectively POSIX-only.