From f9e8bc05b38b13f0282f919821277e2b91f0e98e Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Mon, 22 Jun 2026 17:42:20 +0200 Subject: [PATCH 1/2] fix(appkit): ensure @ast-grep/napi binding at install (napi-postinstall) Signed-off-by: Atila Fassina --- packages/shared/package.json | 1 + packages/shared/scripts/postinstall.d.ts | 14 ++++ packages/shared/scripts/postinstall.js | 62 +++++++++++++-- .../shared/src/scripts/postinstall.test.ts | 78 +++++++++++++++++++ packages/shared/tsconfig.json | 2 +- pnpm-lock.yaml | 10 +++ 6 files changed, 161 insertions(+), 6 deletions(-) create mode 100644 packages/shared/scripts/postinstall.d.ts create mode 100644 packages/shared/src/scripts/postinstall.test.ts diff --git a/packages/shared/package.json b/packages/shared/package.json index 00aafed93..6a6443084 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -40,6 +40,7 @@ "@standard-schema/spec": "1.1.0", "@clack/prompts": "1.0.1", "commander": "12.1.0", + "napi-postinstall": "0.3.4", "zod": "4.3.6" } } diff --git a/packages/shared/scripts/postinstall.d.ts b/packages/shared/scripts/postinstall.d.ts new file mode 100644 index 000000000..8656e52c2 --- /dev/null +++ b/packages/shared/scripts/postinstall.d.ts @@ -0,0 +1,14 @@ +// Type declarations for the self-contained postinstall script. The script itself +// (postinstall.js) ships verbatim in the published package; this .d.ts is dev-only +// (not copied by tools/dist-appkit.ts) and exists so the unit test can import the +// script under TypeScript without enabling allowJs. + +/** + * Pre-fetch the @ast-grep/napi native host binding at install time. No-op unless + * running under npm. Best-effort and NON-FATAL: any failure logs a single warning + * and resolves normally. + */ +export function ensureAstGrepBinding(): Promise; + +/** Print the hint telling users how to set up AI assistant instructions. */ +export function printSetupHint(): void; diff --git a/packages/shared/scripts/postinstall.js b/packages/shared/scripts/postinstall.js index 22f1c09dd..271324ef5 100644 --- a/packages/shared/scripts/postinstall.js +++ b/packages/shared/scripts/postinstall.js @@ -1,6 +1,58 @@ #!/usr/bin/env node -console.log(""); -console.log("[@databricks/appkit] To setup AI assistant instructions, run:"); -console.log(""); -console.log(" npx appkit setup --write"); -console.log(""); +// This script is copied STANDALONE into the published @databricks/appkit package +// (see tools/dist-appkit.ts). It must stay SELF-CONTAINED: it may only import +// external runtime deps (declared in packages/shared/package.json -> dependencies) +// and must NEVER import from appkit's own src/dist — those paths do not exist in +// the published layout. +import { fileURLToPath } from "node:url"; + +/** + * Pre-fetch the @ast-grep/napi native host binding at install time. + * + * `appkit generate-types` loads @ast-grep/napi, which resolves a platform-specific + * optional dependency (e.g. @ast-grep/napi-linux-x64-gnu). npm sometimes silently + * skips that optional binary (npm/cli#4828, or a supply-chain cutoff), which makes + * the binding fail to load at runtime and crashes the app's own postinstall on + * Databricks Apps. `napi-postinstall` re-resolves and materializes the correct + * native package so the binding is guaranteed present. + * + * This is a best-effort, NON-FATAL step: + * - It only runs under npm (the package manager with the optional-dep bug). Under + * pnpm/yarn-PnP the approach does not apply, so it is a no-op. + * - Any failure prints a single concise warning and resolves normally. A failed + * pre-fetch must NEVER break `npm install`. + */ +export async function ensureAstGrepBinding() { + // Only npm exhibits the optional-dependency skip this works around. Other package + // managers (pnpm, yarn PnP) either resolve correctly or don't support this flow. + if (!process.env.npm_config_user_agent?.startsWith("npm/")) { + return; + } + + try { + // napi-postinstall is CommonJS; this script is ESM, so load it via dynamic import. + const { checkAndPreparePackage } = await import("napi-postinstall"); + await checkAndPreparePackage("@ast-grep/napi"); + } catch (err) { + // Non-fatal: never throw, never exit non-zero. console.error writes to stderr. + console.error( + `[@databricks/appkit] Could not pre-fetch @ast-grep/napi native binding: ${err?.message ?? err}`, + ); + } +} + +/** Print the hint telling users how to set up AI assistant instructions. */ +export function printSetupHint() { + console.log(""); + console.log("[@databricks/appkit] To setup AI assistant instructions, run:"); + console.log(""); + console.log(" npx appkit setup --write"); + console.log(""); +} + +// Only run side effects when executed directly (e.g. as the package postinstall), +// not when imported from a test. +if (fileURLToPath(import.meta.url) === process.argv[1]) { + await ensureAstGrepBinding(); + printSetupHint(); +} diff --git a/packages/shared/src/scripts/postinstall.test.ts b/packages/shared/src/scripts/postinstall.test.ts new file mode 100644 index 000000000..dbe17ac58 --- /dev/null +++ b/packages/shared/src/scripts/postinstall.test.ts @@ -0,0 +1,78 @@ +import { + afterEach, + beforeEach, + describe, + expect, + type Mock, + test, + vi, +} from "vitest"; + +// The system under test lives in the standalone, self-contained postinstall script +// that ships verbatim in the published package (see tools/dist-appkit.ts). We import +// it here (not a copy) so the published behavior is what gets exercised. The script's +// top-level main guard means importing it does NOT trigger the install side effect. + +// vi.mock factories are hoisted above the imports, so the spy must be created in a +// hoisted block too (a plain const would be in the TDZ when the factory runs). +const { checkAndPreparePackage } = vi.hoisted(() => ({ + checkAndPreparePackage: vi.fn(async () => {}), +})); + +// napi-postinstall is loaded via dynamic `await import("napi-postinstall")` inside +// ensureAstGrepBinding. Mock it so the test never touches the network or filesystem. +vi.mock("napi-postinstall", () => ({ checkAndPreparePackage })); + +import { ensureAstGrepBinding } from "../../scripts/postinstall.js"; + +describe("ensureAstGrepBinding (postinstall native-binding pre-fetch)", () => { + const prevUserAgent = process.env.npm_config_user_agent; + let consoleError: Mock; + + beforeEach(() => { + vi.clearAllMocks(); + // Silence the non-fatal warning (console.error -> stderr) so failing-path + // tests don't pollute output, and so we can assert it fired exactly once. + consoleError = vi + .spyOn(console, "error") + .mockImplementation(() => {}) as Mock; + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (prevUserAgent === undefined) { + delete process.env.npm_config_user_agent; + } else { + process.env.npm_config_user_agent = prevUserAgent; + } + }); + + test("pre-fetches @ast-grep/napi under an npm user agent", async () => { + process.env.npm_config_user_agent = "npm/10.0.0 node/v22.0.0 darwin arm64"; + + await ensureAstGrepBinding(); + + expect(checkAndPreparePackage).toHaveBeenCalledTimes(1); + expect(checkAndPreparePackage).toHaveBeenCalledWith("@ast-grep/napi"); + }); + + test("is non-fatal when checkAndPreparePackage rejects", async () => { + process.env.npm_config_user_agent = "npm/10.0.0 node/v22.0.0 darwin arm64"; + checkAndPreparePackage.mockRejectedValueOnce(new Error("network down")); + + // Must resolve (not reject) — a failed pre-fetch must never break `npm install`. + await expect(ensureAstGrepBinding()).resolves.toBeUndefined(); + // And it warns exactly once on stderr (via console.error). + expect(consoleError).toHaveBeenCalledTimes(1); + expect(consoleError.mock.calls[0]?.[0]).toContain("@ast-grep/napi"); + }); + + test("does nothing under a non-npm user agent (e.g. pnpm)", async () => { + process.env.npm_config_user_agent = + "pnpm/10.21.0 npm/? node/v22.0.0 darwin arm64"; + + await ensureAstGrepBinding(); + + expect(checkAndPreparePackage).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/shared/tsconfig.json b/packages/shared/tsconfig.json index 5e195c3b6..efd3b9d52 100644 --- a/packages/shared/tsconfig.json +++ b/packages/shared/tsconfig.json @@ -7,6 +7,6 @@ "@/*": ["src/*"] } }, - "include": ["src/**/*"], + "include": ["src/**/*", "scripts/**/*.d.ts"], "exclude": ["node_modules", "dist", "src/**/fixtures"] } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6bf9f44a1..2669354b7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -554,6 +554,9 @@ importers: commander: specifier: 12.1.0 version: 12.1.0 + napi-postinstall: + specifier: 0.3.4 + version: 0.3.4 zod: specifier: 4.3.6 version: 4.3.6 @@ -8938,6 +8941,11 @@ packages: napi-build-utils@2.0.0: resolution: {integrity: sha512-GEbrYkbfF7MoNaoh2iGG84Mnf/WZfB0GdGEsM8wz7Expx/LlWf5U8t9nvJKXSp3qr5IsEbK04cBGhol/KwOsWA==} + napi-postinstall@0.3.4: + resolution: {integrity: sha512-PHI5f1O0EP5xJ9gQmFGMS6IZcrVvTjpXjz7Na41gTE7eE2hK11lg04CECCYEEjdc17EV4DO+fkGEtt7TpTaTiQ==} + engines: {node: ^12.20.0 || ^14.18.0 || >=16.0.0} + hasBin: true + natural-compare@1.4.0: resolution: {integrity: sha512-OWND8ei3VtNC9h7V60qff3SVobHr996CTwgxubgyQYEpg290h9J0buyECNNJexkFm5sOajh5G116RYA1c8ZMSw==} @@ -21960,6 +21968,8 @@ snapshots: napi-build-utils@2.0.0: optional: true + napi-postinstall@0.3.4: {} + natural-compare@1.4.0: {} negotiator@0.6.3: {} From 06aac677d094a8b1fd327786743c0acc74d2a850 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 23 Jun 2026 15:10:41 +0200 Subject: [PATCH 2/2] fix(appkit): time-bound the @ast-grep/napi install pre-fetch Signed-off-by: Atila Fassina --- packages/shared/scripts/postinstall.d.ts | 6 +-- packages/shared/scripts/postinstall.js | 48 ++++++++++++++----- .../shared/src/scripts/postinstall.test.ts | 44 ++++++++++------- 3 files changed, 67 insertions(+), 31 deletions(-) diff --git a/packages/shared/scripts/postinstall.d.ts b/packages/shared/scripts/postinstall.d.ts index 8656e52c2..a78372752 100644 --- a/packages/shared/scripts/postinstall.d.ts +++ b/packages/shared/scripts/postinstall.d.ts @@ -5,10 +5,10 @@ /** * Pre-fetch the @ast-grep/napi native host binding at install time. No-op unless - * running under npm. Best-effort and NON-FATAL: any failure logs a single warning - * and resolves normally. + * running under npm. Best-effort and NON-FATAL: runs in a child process bounded by + * a hard timeout, and any failure or timeout logs a single warning and returns. */ -export function ensureAstGrepBinding(): Promise; +export function ensureAstGrepBinding(): void; /** Print the hint telling users how to set up AI assistant instructions. */ export function printSetupHint(): void; diff --git a/packages/shared/scripts/postinstall.js b/packages/shared/scripts/postinstall.js index 271324ef5..0aaccdd87 100644 --- a/packages/shared/scripts/postinstall.js +++ b/packages/shared/scripts/postinstall.js @@ -4,37 +4,61 @@ // external runtime deps (declared in packages/shared/package.json -> dependencies) // and must NEVER import from appkit's own src/dist — those paths do not exist in // the published layout. +import { execFileSync } from "node:child_process"; +import path from "node:path"; import { fileURLToPath } from "node:url"; +// Hard ceiling (ms) on the best-effort native-binding pre-fetch. napi-postinstall +// shells out to a SYNCHRONOUS `npm install` internally, which an in-process timer +// could not interrupt — so the pre-fetch runs in a child process that we kill if it +// exceeds this deadline. A timeout (like any other failure) is non-fatal: a still- +// missing binding is handled gracefully by AppKit's lazy @ast-grep/napi loader. +const PREFETCH_TIMEOUT_MS = 60_000; + +// Runs in the child: re-resolve and materialize the @ast-grep/napi host binding. +// `.catch(exit 1)` keeps the child quiet and turns a rejection into a non-zero exit +// that the parent treats as a failed (non-fatal) pre-fetch. +const PREFETCH_SCRIPT = + "Promise.resolve(require('napi-postinstall').checkAndPreparePackage('@ast-grep/napi')).catch(() => process.exit(1))"; + /** * Pre-fetch the @ast-grep/napi native host binding at install time. * * `appkit generate-types` loads @ast-grep/napi, which resolves a platform-specific * optional dependency (e.g. @ast-grep/napi-linux-x64-gnu). npm sometimes silently * skips that optional binary (npm/cli#4828, or a supply-chain cutoff), which makes - * the binding fail to load at runtime and crashes the app's own postinstall on - * Databricks Apps. `napi-postinstall` re-resolves and materializes the correct - * native package so the binding is guaranteed present. + * the binding fail to load and crashes the app's own postinstall on Databricks Apps. + * `napi-postinstall` re-resolves and materializes the correct native package. * - * This is a best-effort, NON-FATAL step: - * - It only runs under npm (the package manager with the optional-dep bug). Under + * Best-effort and NON-FATAL: + * - Runs only under npm (the package manager with the optional-dep bug). Under * pnpm/yarn-PnP the approach does not apply, so it is a no-op. - * - Any failure prints a single concise warning and resolves normally. A failed + * - Runs in a child process bounded by PREFETCH_TIMEOUT_MS, so a hung/slow fetch + * is killed rather than blocking `npm install` indefinitely (napi-postinstall's + * internal `npm install` is synchronous, so an in-process timer cannot bound it). + * - Any failure or timeout prints a single warning and returns normally. A failed * pre-fetch must NEVER break `npm install`. */ -export async function ensureAstGrepBinding() { +export function ensureAstGrepBinding() { // Only npm exhibits the optional-dependency skip this works around. Other package // managers (pnpm, yarn PnP) either resolve correctly or don't support this flow. if (!process.env.npm_config_user_agent?.startsWith("npm/")) { return; } + // Package root (…/scripts/postinstall.js -> package dir) so the child resolves + // `napi-postinstall` and `@ast-grep/napi` from the installed node_modules tree. + const pkgDir = path.dirname(path.dirname(fileURLToPath(import.meta.url))); + try { - // napi-postinstall is CommonJS; this script is ESM, so load it via dynamic import. - const { checkAndPreparePackage } = await import("napi-postinstall"); - await checkAndPreparePackage("@ast-grep/napi"); + execFileSync(process.execPath, ["-e", PREFETCH_SCRIPT], { + cwd: pkgDir, + stdio: "inherit", + timeout: PREFETCH_TIMEOUT_MS, + }); } catch (err) { - // Non-fatal: never throw, never exit non-zero. console.error writes to stderr. + // Non-fatal: execFileSync throws on a non-zero child exit AND on a timeout kill. + // console.error writes to stderr. console.error( `[@databricks/appkit] Could not pre-fetch @ast-grep/napi native binding: ${err?.message ?? err}`, ); @@ -53,6 +77,6 @@ export function printSetupHint() { // Only run side effects when executed directly (e.g. as the package postinstall), // not when imported from a test. if (fileURLToPath(import.meta.url) === process.argv[1]) { - await ensureAstGrepBinding(); + ensureAstGrepBinding(); printSetupHint(); } diff --git a/packages/shared/src/scripts/postinstall.test.ts b/packages/shared/src/scripts/postinstall.test.ts index dbe17ac58..85beedec6 100644 --- a/packages/shared/src/scripts/postinstall.test.ts +++ b/packages/shared/src/scripts/postinstall.test.ts @@ -15,13 +15,14 @@ import { // vi.mock factories are hoisted above the imports, so the spy must be created in a // hoisted block too (a plain const would be in the TDZ when the factory runs). -const { checkAndPreparePackage } = vi.hoisted(() => ({ - checkAndPreparePackage: vi.fn(async () => {}), +const { execFileSync } = vi.hoisted(() => ({ + execFileSync: vi.fn(), })); -// napi-postinstall is loaded via dynamic `await import("napi-postinstall")` inside -// ensureAstGrepBinding. Mock it so the test never touches the network or filesystem. -vi.mock("napi-postinstall", () => ({ checkAndPreparePackage })); +// ensureAstGrepBinding runs the pre-fetch in a time-bounded child process via +// execFileSync. Mock it so the test never spawns a real process or touches the +// network/filesystem. +vi.mock("node:child_process", () => ({ execFileSync })); import { ensureAstGrepBinding } from "../../scripts/postinstall.js"; @@ -47,32 +48,43 @@ describe("ensureAstGrepBinding (postinstall native-binding pre-fetch)", () => { } }); - test("pre-fetches @ast-grep/napi under an npm user agent", async () => { + test("pre-fetches @ast-grep/napi in a time-bounded child under an npm user agent", () => { process.env.npm_config_user_agent = "npm/10.0.0 node/v22.0.0 darwin arm64"; - await ensureAstGrepBinding(); + ensureAstGrepBinding(); - expect(checkAndPreparePackage).toHaveBeenCalledTimes(1); - expect(checkAndPreparePackage).toHaveBeenCalledWith("@ast-grep/napi"); + expect(execFileSync).toHaveBeenCalledTimes(1); + const [bin, args, opts] = execFileSync.mock.calls[0] as [ + string, + string[], + { timeout?: number }, + ]; + expect(bin).toBe(process.execPath); + expect(args.join(" ")).toContain("@ast-grep/napi"); + // The pre-fetch MUST be bounded so a hung/slow fetch cannot block `npm install`. + expect(opts.timeout).toBeGreaterThan(0); }); - test("is non-fatal when checkAndPreparePackage rejects", async () => { + test("is non-fatal when the pre-fetch fails or times out", () => { process.env.npm_config_user_agent = "npm/10.0.0 node/v22.0.0 darwin arm64"; - checkAndPreparePackage.mockRejectedValueOnce(new Error("network down")); + // execFileSync throws on both a non-zero child exit and a timeout kill. + execFileSync.mockImplementation(() => { + throw new Error("ETIMEDOUT"); + }); - // Must resolve (not reject) — a failed pre-fetch must never break `npm install`. - await expect(ensureAstGrepBinding()).resolves.toBeUndefined(); + // Must not throw — a failed/timed-out pre-fetch must never break `npm install`. + expect(() => ensureAstGrepBinding()).not.toThrow(); // And it warns exactly once on stderr (via console.error). expect(consoleError).toHaveBeenCalledTimes(1); expect(consoleError.mock.calls[0]?.[0]).toContain("@ast-grep/napi"); }); - test("does nothing under a non-npm user agent (e.g. pnpm)", async () => { + test("does nothing under a non-npm user agent (e.g. pnpm)", () => { process.env.npm_config_user_agent = "pnpm/10.21.0 npm/? node/v22.0.0 darwin arm64"; - await ensureAstGrepBinding(); + ensureAstGrepBinding(); - expect(checkAndPreparePackage).not.toHaveBeenCalled(); + expect(execFileSync).not.toHaveBeenCalled(); }); });