From 60652cc6b62afee2adb1ff7f00bbe002b4db1b4b Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 20 Oct 2026 14:19:15 +0200 Subject: [PATCH 01/16] feat(appkit): appkit plugin add with --auth-mode and --set resource config Adds a plugin from appkit.plugins.json to an existing app: registers it in the server, appends .env values, and patches app.yaml and databricks.yml. Flags and semantics follow databricks apps init: --auth-mode obo|sp default, --set plugin.resourceKey.authMode=obo|sp|both per resource, --set for values (keys validated against the manifest's declared resources), prompts when interactive. OBO adds user_api_scopes and a literal app.yaml value; SP binds the resource with its permission. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../shared/src/cli/commands/plugin/add/add.ts | 332 ++++++++++++++ .../src/cli/commands/plugin/add/plan.test.ts | 194 ++++++++ .../src/cli/commands/plugin/add/plan.ts | 417 ++++++++++++++++++ .../shared/src/cli/commands/plugin/index.ts | 4 + .../src/cli/commands/registry/config-plan.ts | 5 +- .../commands/registry/config-writer.test.ts | 29 ++ .../cli/commands/registry/config-writer.ts | 23 +- .../src/cli/commands/registry/env-writer.ts | 4 +- .../cli/commands/registry/server-register.ts | 7 +- packages/shared/src/cli/deploy-config.ts | 13 +- 10 files changed, 1016 insertions(+), 12 deletions(-) create mode 100644 packages/shared/src/cli/commands/plugin/add/add.ts create mode 100644 packages/shared/src/cli/commands/plugin/add/plan.test.ts create mode 100644 packages/shared/src/cli/commands/plugin/add/plan.ts diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts new file mode 100644 index 000000000..bab01fbc2 --- /dev/null +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -0,0 +1,332 @@ +import fs from "node:fs"; +import path from "node:path"; +import process from "node:process"; + +import { Command } from "commander"; +import pc from "picocolors"; + +import { profileFromEnv } from "../../registry/add"; +import { + planHasContent, + reportConfigWrite, + validateBundle, + writeConfig, +} from "../../registry/config-writer"; +import { serializeEnvAppend } from "../../registry/env-reconcile"; +import { + AUTH_MODE_BOTH, + AUTH_MODE_FIELD, + AUTH_MODE_OBO, + AUTH_MODE_SP, + type AuthMode, + authKey, + buildAddPlan, + findMissingValues, + type ManifestPlugin, + type ManifestResource, + oboCapable, + parseSetAuthModes, + parseSetValues, + resolveAuthModes, + resourceConfigured, + selectResources, +} from "./plan"; + +const MANIFEST_FILE = "appkit.plugins.json"; +const SERVER_SUBDIRS = ["server", "api", "backend"]; + +interface AddOptions { + set: string[]; + authMode?: string; + cwd?: string; + yes?: boolean; + register?: boolean; + profile?: string; +} + +function loadPlugin(cwd: string, name: string): ManifestPlugin { + const file = path.join(cwd, MANIFEST_FILE); + if (!fs.existsSync(file)) { + throw new Error( + `${MANIFEST_FILE} not found in ${cwd}; run \`appkit plugin sync --write\` first`, + ); + } + const manifest = JSON.parse(fs.readFileSync(file, "utf-8")) as { + plugins?: Record; + }; + const plugins = manifest.plugins ?? {}; + const plugin = plugins[name]; + if (!plugin) { + throw new Error( + `unknown plugin "${name}"; available: ${Object.keys(plugins).sort().join(", ")}`, + ); + } + return { + ...plugin, + name, + resources: { + required: plugin.resources?.required ?? [], + optional: plugin.resources?.optional ?? [], + }, + }; +} + +function authModeLabel(mode: AuthMode): string { + if (mode === AUTH_MODE_OBO) return "on behalf of user"; + if (mode === AUTH_MODE_BOTH) return "service principal and on behalf of user"; + return "service principal"; +} + +/** Bails out of the command on Ctrl+C in a prompt. */ +async function answered(value: T | symbol): Promise { + const { cancel, isCancel } = await import("@clack/prompts"); + if (isCancel(value)) { + cancel("Cancelled."); + process.exit(1); + } + return value as T; +} + +/** Asks which optional resources to configure (they're only added with values). */ +async function promptOptional( + plugin: ManifestPlugin, + values: Record, +): Promise { + const { confirm } = await import("@clack/prompts"); + const picked: ManifestResource[] = []; + for (const r of plugin.resources.optional) { + if (resourceConfigured(r, values)) continue; + const yes = await answered( + await confirm({ + message: `Configure ${r.alias}? ${pc.dim(`${r.description ?? ""} (optional)`)}`, + initialValue: false, + }), + ); + if (yes) picked.push(r); + } + return picked; +} + +/** + * As `apps init`: one question for the app, and for "Mixed" one per + * OBO-capable resource. Resources without --set authMode only. + */ +async function promptAuthModes( + resources: ManifestResource[], +): Promise> { + const capable = resources.filter(oboCapable); + if (capable.length === 0) return {}; + const { log, select } = await import("@clack/prompts"); + const appMode = await answered( + await select({ + message: "How should the app access resources?", + options: [ + { value: AUTH_MODE_SP, label: "Service principal" }, + { value: AUTH_MODE_OBO, label: "On behalf of user" }, + { value: "mixed", label: "Mixed (choose per resource)" }, + ], + }), + ); + const modes: Record = {}; + for (const r of capable) { + modes[authKey(r)] = + appMode === "mixed" + ? await answered( + await select({ + message: `How should the app access ${r.alias}?`, + options: [ + { value: AUTH_MODE_OBO, label: "On behalf of user" }, + { value: AUTH_MODE_SP, label: "Service principal" }, + { value: AUTH_MODE_BOTH, label: "Both" }, + ], + }), + ) + : (appMode as AuthMode); + log.info(`${r.alias}: ${authModeLabel(modes[authKey(r)])}`); + } + for (const r of resources) { + if (!oboCapable(r)) { + log.info(`${r.alias}: ${authModeLabel(AUTH_MODE_SP)} (only option)`); + } + } + return modes; +} + +async function runPluginAdd(name: string, opts: AddOptions): Promise { + const cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd(); + if ( + opts.authMode !== undefined && + opts.authMode !== AUTH_MODE_SP && + opts.authMode !== AUTH_MODE_OBO + ) { + throw new Error( + `invalid --auth-mode value: "${opts.authMode}" (must be obo or sp; use --set ..${AUTH_MODE_FIELD}=both for a single resource)`, + ); + } + const plugin = loadPlugin(cwd, name); + const values = parseSetValues(opts.set, plugin); + const setModes = parseSetAuthModes(opts.set, plugin); + const interactive = !opts.yes && Boolean(process.stdin.isTTY); + + let resources = selectResources(plugin, values); + if (interactive) resources.push(...(await promptOptional(plugin, values))); + + const promptedModes = + interactive && opts.authMode === undefined + ? await promptAuthModes( + resources.filter((r) => !(authKey(r) in setModes)), + ) + : {}; + const { modes, keptSP } = resolveAuthModes( + resources, + setModes, + promptedModes, + opts.authMode, + ); + if (keptSP.length > 0) { + console.log( + `Note: these resources cannot be accessed on behalf of the user and use the service principal: ${keptSP.join(", ")}`, + ); + } + + const missing = findMissingValues(plugin.name, resources, values, modes); + if (missing.length > 0 && !interactive) { + const m = missing[0]; + throw new Error( + `missing value for ${m.setKey}; use --set ${m.setKey}=value`, + ); + } + if (missing.length > 0) { + const { text } = await import("@clack/prompts"); + for (const m of missing) { + const field = m.resource.fields?.[m.field]; + const value = await answered( + await text({ + message: `${m.resource.alias}: ${m.field}`, + placeholder: field?.description, + validate: (v) => + !v?.trim() || /[\r\n]/.test(v) ? "Enter a value" : undefined, + }), + ); + values[`${m.resource.resourceKey}.${m.field}`] = value.trim(); + } + } + resources = selectResources(plugin, values); + + const plan = buildAddPlan(plugin, resources, values, modes); + + if (opts.register !== false) await registerInServer(cwd, plugin); + + const { appendToFile, readEnvFile } = + await import("../../registry/env-writer.js"); + const envPath = path.join(cwd, ".env"); + const existing = readEnvFile(envPath); + const newEnv = plan.dotEnv.filter( + (e, i) => + !(e.env in existing) && + plan.dotEnv.findIndex((x) => x.env === e.env) === i, + ); + appendToFile(envPath, serializeEnvAppend(newEnv)); + if (newEnv.length > 0) { + console.log( + `${pc.green("Updated .env:")} ${newEnv.map((e) => e.env).join(", ")}`, + ); + } + + if (planHasContent(plan)) { + const result = writeConfig(cwd, plan); + reportConfigWrite(result); + if (result.databricksYmlChanged) { + validateBundle(cwd, opts.profile ?? profileFromEnv(cwd)); + } + } +} + +async function registerInServer( + cwd: string, + plugin: ManifestPlugin, +): Promise { + const serverRoot = + SERVER_SUBDIRS.map((s) => path.join(cwd, s)).find((p) => + fs.existsSync(p), + ) ?? cwd; + const importPath = + plugin.stability === "beta" ? `${plugin.package}/beta` : plugin.package; + // Lazy: server-register pulls in the @ast-grep/napi native addon. + const { registerPluginInServer } = + await import("../../registry/server-register.js"); + { + { + const result = registerPluginInServer( + serverRoot, + importPath, + plugin.name, + ); + const shown = + result.file && path.relative(cwd, path.join(serverRoot, result.file)); + if (result.status === "wired") { + console.log(`${pc.green("Registered")} ${plugin.name} in ${shown}`); + } else if (result.status === "already") { + console.log(pc.dim(`${plugin.name} is already registered in ${shown}`)); + } else { + console.log( + pc.yellow( + `Couldn't auto-register ${plugin.name}: ${result.reason}.`, + ) + + `\nAdd it to your server's createApp call:\n` + + pc.dim( + ` import { ${plugin.name} } from "${importPath}";\n` + + ` createApp({ plugins: [${plugin.name}(), /* ... */] });`, + ), + ); + } + } + } +} + +export const pluginAddCommand = new Command("add") + .description( + "Add a plugin from appkit.plugins.json to the app: server registration, .env, app.yaml and databricks.yml", + ) + .argument("", "Plugin name, as in appkit.plugins.json") + .option( + "--set ", + "Set resource values (format: plugin.resourceKey.field=value, can specify multiple)", + (v: string, acc: string[]) => [...acc, v], + [] as string[], + ) + .option( + "--auth-mode ", + "Default resource auth mode: obo (on behalf of the user) or sp (service principal). With obo, resources that cannot be accessed on behalf of the user use sp. Prompts if not provided in interactive mode, otherwise sp.", + ) + .option("-C, --cwd ", "Run as if started in ") + .option("-y, --yes", "Don't prompt; fail on missing required values") + .option("--no-register", "Don't edit the server entry to register the plugin") + .option( + "-p, --profile ", + "Databricks profile for bundle validate (defaults to the app's DATABRICKS_CONFIG_PROFILE)", + ) + .addHelpText( + "after", + ` +Resource configuration (--set), as in \`databricks apps init\`: + Set resource values using --set plugin.resourceKey.field=value + Keys are defined in the app's appkit.plugins.json manifest. + Multi-field resources (e.g., database, secret) require all fields to be set together. + Use --set plugin.resourceKey.authMode=obo|sp|both to choose how the app accesses a resource: + on behalf of the user (obo), as the app's service principal (sp), or both. + obo/both add the resource's user_api_scopes; sp/both bind it with its permission. + +Existing entries in .env, app.yaml and databricks.yml are never overwritten. + +Examples: + $ appkit plugin add analytics --set analytics.sql-warehouse.id=abc123 + $ appkit plugin add analytics --auth-mode obo --set analytics.sql-warehouse.id=abc123 + $ appkit plugin add jobs --auth-mode obo --set jobs.job.authMode=sp --set jobs.job.id=42`, + ) + .action((name: string, opts: AddOptions) => + runPluginAdd(name, opts).catch((err: Error) => { + console.error(pc.red(err.message)); + process.exit(1); + }), + ); diff --git a/packages/shared/src/cli/commands/plugin/add/plan.test.ts b/packages/shared/src/cli/commands/plugin/add/plan.test.ts new file mode 100644 index 000000000..f2ae70319 --- /dev/null +++ b/packages/shared/src/cli/commands/plugin/add/plan.test.ts @@ -0,0 +1,194 @@ +import { describe, expect, test } from "vitest"; + +import { + buildAddPlan, + findMissingValues, + type ManifestPlugin, + parseSetAuthModes, + parseSetValues, + resolveAuthModes, + selectResources, + validSetKeys, +} from "./plan"; + +const warehouse = { + type: "sql_warehouse", + alias: "SQL Warehouse", + resourceKey: "sql-warehouse", + description: "Warehouse", + permission: "CAN_USE", + fields: { id: { env: "DATABRICKS_WAREHOUSE_ID", description: "id" } }, + scope: "sql", + binding: { yamlKey: "sql_warehouse", varFields: [["id", "id"]] }, +} satisfies ManifestPlugin["resources"]["required"][number]; + +const secret = { + type: "secret", + alias: "Secret", + resourceKey: "secret", + permission: "READ", + fields: { key: { env: "SECRET_KEY" }, scope: { env: "SECRET_SCOPE" } }, + appOnly: true, + binding: { + yamlKey: "secret", + varFields: [ + ["scope", "scope"], + ["key", "key"], + ], + }, +} satisfies ManifestPlugin["resources"]["optional"][number]; + +const job = { + type: "job", + alias: "Job", + resourceKey: "job", + permission: "CAN_MANAGE_RUN", + fields: { id: { env: "DATABRICKS_JOB_ID" } }, + binding: { yamlKey: "job", varFields: [["id", "id"]] }, +} satisfies ManifestPlugin["resources"]["optional"][number]; + +const plugin: ManifestPlugin = { + name: "analytics", + package: "@databricks/appkit", + scopes: ["ai-gateway"], + resources: { required: [warehouse], optional: [secret, job] }, +}; + +describe("parseSetValues", () => { + test("keys values by resourceKey.field and skips authMode", () => { + expect( + parseSetValues( + [ + "analytics.sql-warehouse.id=wh", + "analytics.sql-warehouse.authMode=obo", + ], + plugin, + ), + ).toEqual({ "sql-warehouse.id": "wh" }); + }); + + test.each([ + ["other.sql-warehouse.id=x", "unknown plugin"], + ["analytics.nope.id=x", "does not match a resource declared"], + ["analytics.sql-warehouse=x", "invalid --set key"], + ["analytics.secret.scope=s", "incomplete resource"], + ])("%s → %s", (sv, err) => { + expect(() => parseSetValues([sv], plugin)).toThrow(err); + }); +}); + +test("validSetKeys derives keys and values from the manifest", () => { + const keys = validSetKeys(plugin); + expect([...keys.keys()]).toEqual([ + "analytics.sql-warehouse.id", + "analytics.sql-warehouse.authMode", + "analytics.secret.key", + "analytics.secret.scope", + "analytics.job.id", + "analytics.job.authMode", + ]); + expect(keys.get("analytics.job.authMode")).toEqual(["sp"]); + expect(() => + parseSetValues(["analytics.sql-warehouse.name=x"], plugin), + ).toThrow("valid keys: analytics.sql-warehouse.id"); + expect(() => + parseSetAuthModes(["analytics.nope.authMode=sp"], plugin), + ).toThrow("does not match a resource declared"); +}); + +describe("parseSetAuthModes", () => { + test.each([ + ["analytics.sql-warehouse.authMode=user", "invalid auth mode"], + ["analytics.secret.authMode=sp", "always accessed by the app's service"], + ["analytics.job.authMode=obo", "cannot be accessed on behalf of the user"], + ])("%s → %s", (sv, err) => { + expect(() => parseSetAuthModes([sv], plugin)).toThrow(err); + }); + + test("accepts both and sp without scope", () => { + expect( + parseSetAuthModes( + ["analytics.sql-warehouse.authMode=both", "analytics.job.authMode=sp"], + plugin, + ), + ).toEqual({ "sql_warehouse:sql-warehouse": "both", "job:job": "sp" }); + }); +}); + +test("resolveAuthModes: --set wins, obo default skips non-capable", () => { + const { modes, keptSP } = resolveAuthModes( + [warehouse, secret, job], + {}, + {}, + "obo", + ); + expect(modes).toEqual({ "sql_warehouse:sql-warehouse": "obo" }); + expect(keptSP).toEqual(["secret", "job"]); + expect( + resolveAuthModes( + [warehouse], + { "sql_warehouse:sql-warehouse": "sp" }, + {}, + "obo", + ).modes, + ).toEqual({}); +}); + +describe("buildAddPlan", () => { + const values = { "sql-warehouse.id": "wh", "job.id": "42" }; + const resources = selectResources(plugin, values); + + test("sp binds the resource with its permission and no scopes", () => { + const plan = buildAddPlan(plugin, resources, values, {}); + expect(resources.map((r) => r.resourceKey)).toEqual([ + "sql-warehouse", + "job", + ]); + expect(plan.userApiScopes).toEqual([]); + expect(plan.appYamlEnv).toEqual([ + { name: "DATABRICKS_WAREHOUSE_ID", valueFrom: "sql-warehouse" }, + { name: "DATABRICKS_JOB_ID", valueFrom: "job" }, + ]); + expect(plan.resourceBindings[0]).toEqual({ + name: "sql-warehouse", + type: "sql_warehouse", + permission: "CAN_USE", + fields: { id: "${var.sql_warehouse_id}" }, + }); + expect(plan.bundleVariables[0]).toMatchObject({ + name: "sql_warehouse_id", + value: "wh", + }); + expect(plan.dotEnv).toContainEqual({ + env: "DATABRICKS_WAREHOUSE_ID", + value: "wh", + }); + }); + + test("obo uses a literal value, no binding, and adds scopes", () => { + const modes = { "sql_warehouse:sql-warehouse": "obo" } as const; + const plan = buildAddPlan(plugin, resources, values, modes); + expect(plan.appYamlEnv[0]).toEqual({ + name: "DATABRICKS_WAREHOUSE_ID", + value: "wh", + }); + expect(plan.resourceBindings.map((b) => b.name)).toEqual(["job"]); + expect(plan.userApiScopes).toEqual(["sql", "ai-gateway"]); + }); + + test("both binds and adds the scope", () => { + const modes = { "sql_warehouse:sql-warehouse": "both" } as const; + const plan = buildAddPlan(plugin, resources, values, modes); + expect(plan.resourceBindings.map((b) => b.name)).toEqual([ + "sql-warehouse", + "job", + ]); + expect(plan.userApiScopes).toEqual(["sql", "ai-gateway"]); + }); +}); + +test("findMissingValues names the --set key", () => { + expect( + findMissingValues("analytics", [warehouse], {}, {}).map((m) => m.setKey), + ).toEqual(["analytics.sql-warehouse.id"]); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/plan.ts b/packages/shared/src/cli/commands/plugin/add/plan.ts new file mode 100644 index 000000000..2f3204443 --- /dev/null +++ b/packages/shared/src/cli/commands/plugin/add/plan.ts @@ -0,0 +1,417 @@ +import type { AppYamlEnvEntry, ResourceBinding } from "../../../deploy-config"; +import type { BundleVariable, ConfigPlan } from "../../registry/config-plan"; +import { isValidEnvName } from "../../registry/requirements"; + +/** + * Pure planning for `appkit plugin add`: a port of the Databricks CLI's + * `apps init` resource handling (cmd/apps/init.go + libs/apps/generator), so a + * plugin added later gets the same .env, app.yaml and databricks.yml entries + * (and the same --set / --auth-mode semantics) it would have had at init. + */ + +export const AUTH_MODE_SP = "sp"; +export const AUTH_MODE_OBO = "obo"; +export const AUTH_MODE_BOTH = "both"; +export type AuthMode = + | typeof AUTH_MODE_SP + | typeof AUTH_MODE_OBO + | typeof AUTH_MODE_BOTH; + +/** Reserved --set field that selects a resource's auth mode. */ +export const AUTH_MODE_FIELD = "authMode"; + +export interface ManifestField { + env?: string; + description?: string; + value?: string; + localOnly?: boolean; + bundleIgnore?: boolean; +} + +export interface ManifestResource { + type: string; + alias: string; + resourceKey: string; + description?: string; + permission?: string; + fields?: Record; + scope?: string; + appOnly?: boolean; + binding?: { + yamlKey: string; + varFields: [string, string][]; + staticFields?: [string, string][]; + }; +} + +/** A plugin entry of the app's appkit.plugins.json (the fields used here). */ +export interface ManifestPlugin { + name: string; + package: string; + stability?: string; + scopes?: string[]; + resources: { required: ManifestResource[]; optional: ManifestResource[] }; +} + +/** Same identity as resource de-duplication: type + key. */ +export function authKey(r: ManifestResource): string { + return `${r.type}:${r.resourceKey}`; +} + +/** Keys `resourceKey.field`, as the CLI's resourceValues map. */ +function valueKey(r: ManifestResource, field: string): string { + return `${r.resourceKey}.${field}`; +} + +function fieldNames(r: ManifestResource): string[] { + return Object.keys(r.fields ?? {}).sort(); +} + +function allResources(p: ManifestPlugin): ManifestResource[] { + return [...p.resources.required, ...p.resources.optional]; +} + +/** Manifest fields the resource's databricks.yml binding references. */ +function bindingVarFields(r: ManifestResource): string[] { + return (r.binding?.varFields ?? []).map(([f]) => f); +} + +/** + * The --set keys the plugin manifest declares, each with its valid values: + * `plugin.resourceKey.field` for every declared field and binding var field + * (any value), plus `plugin.resourceKey.authMode` for resources that are not + * app-only (obo/both only when the resource declares a user scope). + */ +export function validSetKeys( + plugin: ManifestPlugin, +): Map { + const keys = new Map(); + for (const r of allResources(plugin)) { + const prefix = `${plugin.name}.${r.resourceKey}.`; + for (const f of new Set([...fieldNames(r), ...bindingVarFields(r)])) { + keys.set(prefix + f, null); + } + if (!r.appOnly) { + keys.set( + prefix + AUTH_MODE_FIELD, + r.scope + ? [AUTH_MODE_OBO, AUTH_MODE_SP, AUTH_MODE_BOTH] + : [AUTH_MODE_SP], + ); + } + } + return keys; +} + +/** Throws unless the key is one the plugin manifest declares. */ +function assertDeclared(plugin: ManifestPlugin, sv: string, key: string): void { + const keys = validSetKeys(plugin); + if (keys.has(key)) return; + throw new Error( + `--set "${sv}" does not match a resource declared by plugin "${plugin.name}"; valid keys: ${[...keys.keys()].join(", ") || "(none, the plugin declares no resources)"}`, + ); +} + +function splitSetKey(sv: string): { + pluginName: string; + resourceKey: string; + field: string; + value: string; +} { + const eq = sv.indexOf("="); + const key = eq === -1 ? "" : sv.slice(0, eq); + if (!key) { + throw new Error( + `invalid --set format "${sv}", expected plugin.resourceKey.field=value`, + ); + } + const parts = key.split("."); + if (parts.length < 3) { + throw new Error( + `invalid --set key "${key}", expected plugin.resourceKey.field`, + ); + } + return { + pluginName: parts[0], + resourceKey: parts[1], + field: parts.slice(2).join("."), + value: sv.slice(eq + 1), + }; +} + +/** + * Parses `--set plugin.resourceKey.field=value` into `resourceKey.field` → value, + * validating plugin, resource and field against the manifest and that + * multi-field resources are set together. `authMode` pairs are skipped (see + * {@link parseSetAuthModes}). + */ +export function parseSetValues( + setValues: string[], + plugin: ManifestPlugin, +): Record { + const rv: Record = {}; + for (const sv of setValues) { + const { pluginName, resourceKey, field, value } = splitSetKey(sv); + if (field === AUTH_MODE_FIELD) continue; + if (pluginName !== plugin.name) { + throw new Error( + `unknown plugin "${pluginName}" in --set "${sv}"; this command adds "${plugin.name}"`, + ); + } + assertDeclared(plugin, sv, `${pluginName}.${resourceKey}.${field}`); + rv[`${resourceKey}.${field}`] = value; + } + + for (const r of allResources(plugin)) { + const checkable = fieldNames(r).filter( + (f) => !r.fields?.[f].bundleIgnore && !r.fields?.[f].localOnly, + ); + if (checkable.length <= 1) continue; + const missing = checkable.filter((f) => !rv[valueKey(r, f)]); + if (missing.length > 0 && missing.length < checkable.length) { + throw new Error( + `incomplete resource "${r.resourceKey}": missing fields [${missing.map((f) => valueKey(r, f)).join(" ")}] (all fields must be set together)`, + ); + } + } + return rv; +} + +/** Parses `--set plugin.resourceKey.authMode=obo|sp|both` into authKey → mode. */ +export function parseSetAuthModes( + setValues: string[], + plugin: ManifestPlugin, +): Record { + const modes: Record = {}; + for (const sv of setValues) { + const { pluginName, resourceKey, field, value } = splitSetKey(sv); + if (field !== AUTH_MODE_FIELD) continue; + if (pluginName !== plugin.name) { + throw new Error( + `unknown plugin "${pluginName}" in --set "${sv}"; this command adds "${plugin.name}"`, + ); + } + const r = allResources(plugin).find((x) => x.resourceKey === resourceKey); + if (!r) + throw assertDeclared(plugin, sv, `${pluginName}.${resourceKey}.${field}`); + if ( + value !== AUTH_MODE_SP && + value !== AUTH_MODE_OBO && + value !== AUTH_MODE_BOTH + ) { + throw new Error( + `invalid auth mode "${value}" in --set "${sv}" (must be obo, sp, or both)`, + ); + } + if (r.appOnly) { + throw new Error( + `resource "${resourceKey}" of plugin "${plugin.name}" is always accessed by the app's service principal; remove --set ${sv}`, + ); + } + if (value !== AUTH_MODE_SP && !r.scope) { + throw new Error( + `resource "${resourceKey}" of plugin "${plugin.name}" cannot be accessed on behalf of the user; use ${plugin.name}.${resourceKey}.${AUTH_MODE_FIELD}=sp`, + ); + } + modes[authKey(r)] = value; + } + return modes; +} + +/** True when the resource can be accessed on behalf of the user. */ +export function oboCapable(r: ManifestResource): boolean { + return Boolean(r.scope) && !r.appOnly; +} + +/** + * Auth mode per resource. Precedence: --set authMode, the prompted choice, the + * --auth-mode default, then sp. The obo default only applies to OBO-capable + * resources; the others are returned in `keptSP`. Only obo/both are stored. + */ +export function resolveAuthModes( + resources: ManifestResource[], + setModes: Record, + promptedModes: Record, + defaultMode: string | undefined, +): { modes: Record; keptSP: string[] } { + const modes: Record = {}; + const keptSP: string[] = []; + for (const r of resources) { + let mode: AuthMode | undefined = + setModes[authKey(r)] ?? promptedModes[authKey(r)]; + if (mode === undefined && defaultMode === AUTH_MODE_OBO) { + if (!oboCapable(r)) { + keptSP.push(r.resourceKey); + continue; + } + mode = AUTH_MODE_OBO; + } + if (mode === AUTH_MODE_OBO || mode === AUTH_MODE_BOTH) { + modes[authKey(r)] = mode; + } + } + return { modes, keptSP }; +} + +/** True when any field value of the resource is set. */ +export function resourceConfigured( + r: ManifestResource, + values: Record, +): boolean { + return [...fieldNames(r), ...bindingVarFields(r)].some( + (f) => values[valueKey(r, f)], + ); +} + +/** + * The resources that end up in the app: every required one, plus optional + * ones that have a value. + */ +export function selectResources( + plugin: ManifestPlugin, + values: Record, +): ManifestResource[] { + return [ + ...plugin.resources.required, + ...plugin.resources.optional.filter((r) => resourceConfigured(r, values)), + ]; +} + +export interface MissingValue { + resource: ManifestResource; + field: string; + /** The --set key that supplies it. */ + setKey: string; +} + +/** + * Values still needed, in prompt order: for each resource bound to the service + * principal (sp/both), every field its binding references; for resources + * accessed only on behalf of the user, every field with an env var (its + * app.yaml entry carries the literal value). + */ +export function findMissingValues( + pluginName: string, + resources: ManifestResource[], + values: Record, + modes: Record, +): MissingValue[] { + const missing: MissingValue[] = []; + for (const r of resources) { + const fields = + modes[authKey(r)] === AUTH_MODE_OBO + ? fieldNames(r).filter( + (f) => r.fields?.[f].env && !r.fields[f].localOnly, + ) + : variableFields(r); + for (const field of fields) { + if (values[valueKey(r, field)] || r.fields?.[field]?.value) continue; + missing.push({ + resource: r, + field, + setKey: `${pluginName}.${r.resourceKey}.${field}`, + }); + } + } + return missing; +} + +/** Fields that become bundle variables: manifest fields plus binding var fields. */ +function variableFields(r: ManifestResource): string[] { + const out = fieldNames(r).filter( + (f) => !r.fields?.[f].bundleIgnore && !r.fields?.[f].localOnly, + ); + const covered = new Set(fieldNames(r)); + for (const f of bindingVarFields(r)) { + if (!covered.has(f)) out.push(f); + } + return out; +} + +function varName(r: ManifestResource, field: string): string { + return `${r.resourceKey.replaceAll("-", "_")}_${field}`; +} + +export interface AddPlan extends ConfigPlan { + /** .env lines to add (skipped later when the name is already present). */ + dotEnv: { env: string; value: string }[]; +} + +/** + * Builds the file changes for a plugin's resources, mirroring the CLI + * generator: OBO-only resources get a literal app.yaml `value` and no bundle + * binding; sp/both resources get variables, a binding and `valueFrom`; + * obo/both resources add their scope (plus the plugin's own scopes) to + * user_api_scopes. + */ +export function buildAddPlan( + plugin: ManifestPlugin, + resources: ManifestResource[], + values: Record, + modes: Record, +): AddPlan { + const dotEnv: AddPlan["dotEnv"] = []; + const appYamlEnv: AppYamlEnvEntry[] = []; + const bundleVariables: BundleVariable[] = []; + const resourceBindings: ResourceBinding[] = []; + const unverifiedTypes: string[] = []; + const scopes: string[] = []; + + for (const r of resources) { + const mode = modes[authKey(r)]; + const oboOnly = mode === AUTH_MODE_OBO; + if ((mode === AUTH_MODE_OBO || mode === AUTH_MODE_BOTH) && r.scope) { + scopes.push(r.scope); + } + + for (const f of fieldNames(r)) { + const field = r.fields?.[f] ?? {}; + if (!field.env || !isValidEnvName(field.env)) continue; + const value = (values[valueKey(r, f)] || field.value || "").replace( + /[\r\n]/g, + "", + ); + dotEnv.push({ env: field.env, value }); + if (field.localOnly) continue; + appYamlEnv.push( + oboOnly + ? { name: field.env, value } + : { name: field.env, valueFrom: r.resourceKey }, + ); + } + + if (oboOnly) continue; + if (!r.binding) { + unverifiedTypes.push(r.type); + continue; + } + for (const f of variableFields(r)) { + bundleVariables.push({ + name: varName(r, f), + description: r.fields?.[f]?.description || r.description, + value: values[valueKey(r, f)] || undefined, + }); + } + const fields: Record = {}; + for (const [mf, yf] of r.binding.varFields) { + fields[yf] = `\${var.${varName(r, mf)}}`; + } + for (const [yf, v] of r.binding.staticFields ?? []) fields[yf] = v; + resourceBindings.push({ + name: r.resourceKey, + type: r.binding.yamlKey, + permission: r.permission, + fields, + }); + } + + if (scopes.length > 0) scopes.push(...(plugin.scopes ?? [])); + return { + dotEnv, + appYamlEnv, + bundleVariables, + resourceBindings, + unverifiedTypes, + userApiScopes: [...new Set(scopes)], + }; +} diff --git a/packages/shared/src/cli/commands/plugin/index.ts b/packages/shared/src/cli/commands/plugin/index.ts index 6612d205a..810ae673b 100644 --- a/packages/shared/src/cli/commands/plugin/index.ts +++ b/packages/shared/src/cli/commands/plugin/index.ts @@ -1,6 +1,7 @@ import { Command } from "commander"; import { pluginAddResourceCommand } from "./add-resource/add-resource"; +import { pluginAddCommand } from "./add/add"; import { pluginCreateCommand } from "./create/create"; import { pluginListCommand } from "./list/list"; import { pluginPromoteCommand } from "./promote/promote"; @@ -14,6 +15,7 @@ import { pluginValidateCommand } from "./validate/validate"; * - create: Scaffold a new plugin (interactive) * - validate: Validate manifest(s) against the JSON schema * - list: List plugins from appkit.plugins.json or a directory + * - add: Add a plugin to the app (server, .env, app.yaml, databricks.yml) * - add-resource: Add a resource requirement to a plugin manifest (interactive) * - promote: Promote a plugin to a higher stability tier */ @@ -23,6 +25,7 @@ export const pluginCommand = new Command("plugin") .addCommand(pluginCreateCommand) .addCommand(pluginValidateCommand) .addCommand(pluginListCommand) + .addCommand(pluginAddCommand) .addCommand(pluginAddResourceCommand) .addCommand(pluginPromoteCommand) .addHelpText( @@ -33,6 +36,7 @@ Examples: $ appkit plugin create --placement in-repo --path plugins/my-plugin --name my-plugin --description "Does X" $ appkit plugin validate . $ appkit plugin list --json + $ appkit plugin add analytics --auth-mode obo --set analytics.sql-warehouse.id=abc123 $ appkit plugin add-resource --path plugins/my-plugin --type sql_warehouse $ appkit plugin promote my-plugin --to ga`, ); diff --git a/packages/shared/src/cli/commands/registry/config-plan.ts b/packages/shared/src/cli/commands/registry/config-plan.ts index 01260cf8a..168bd19f1 100644 --- a/packages/shared/src/cli/commands/registry/config-plan.ts +++ b/packages/shared/src/cli/commands/registry/config-plan.ts @@ -26,6 +26,8 @@ export interface ConfigPlan { appYamlEnv: AppYamlEnvEntry[]; bundleVariables: BundleVariable[]; resourceBindings: ResourceBinding[]; + /** Entries for resources.apps.app.user_api_scopes (added if absent). */ + userApiScopes?: string[]; /** Resource types encountered that have no verified binding spec. */ unverifiedTypes: string[]; } @@ -178,6 +180,7 @@ export function planHasContent(plan: ConfigPlan): boolean { return ( plan.appYamlEnv.length > 0 || plan.bundleVariables.length > 0 || - plan.resourceBindings.length > 0 + plan.resourceBindings.length > 0 || + (plan.userApiScopes?.length ?? 0) > 0 ); } diff --git a/packages/shared/src/cli/commands/registry/config-writer.test.ts b/packages/shared/src/cli/commands/registry/config-writer.test.ts index 9bcf7f04e..53c5152d7 100644 --- a/packages/shared/src/cli/commands/registry/config-writer.test.ts +++ b/packages/shared/src/cli/commands/registry/config-writer.test.ts @@ -195,3 +195,32 @@ describe("writeConfig — additive patching", () => { expect(sameYaml(db, db)).toBe(true); }); }); + +describe("writeConfig — user_api_scopes and literal env values", () => { + it("adds missing scopes once and writes literal app.yaml values", () => { + const cwd = makeTempDir(); + fs.writeFileSync( + path.join(cwd, "databricks.yml"), + "resources:\n apps:\n app:\n user_api_scopes:\n - sql\n", + ); + const result = writeConfig(cwd, { + appYamlEnv: [{ name: "DATABRICKS_WAREHOUSE_ID", value: "wh" }], + bundleVariables: [], + resourceBindings: [], + userApiScopes: ["sql", "files.files"], + unverifiedTypes: [], + }); + const db = parseDocument( + fs.readFileSync(path.join(cwd, "databricks.yml"), "utf-8"), + ).toJSON(); + expect(db.resources.apps.app.user_api_scopes).toEqual([ + "sql", + "files.files", + ]); + expect(result.databricksYmlChanged).toBe(true); + const app = parseDocument( + fs.readFileSync(path.join(cwd, "app.yaml"), "utf-8"), + ).toJSON(); + expect(app.env).toEqual([{ name: "DATABRICKS_WAREHOUSE_ID", value: "wh" }]); + }); +}); diff --git a/packages/shared/src/cli/commands/registry/config-writer.ts b/packages/shared/src/cli/commands/registry/config-writer.ts index 479d30ed7..065d15f8d 100644 --- a/packages/shared/src/cli/commands/registry/config-writer.ts +++ b/packages/shared/src/cli/commands/registry/config-writer.ts @@ -52,7 +52,7 @@ function patchAppYaml(file: string, entries: AppYamlEnvEntry[]): string[] { const added: string[] = []; for (const entry of entries) { if (existingNames.has(entry.name)) continue; - seq.add(doc.createNode({ name: entry.name, valueFrom: entry.valueFrom })); + seq.add(doc.createNode(entry)); added.push(entry.name); } @@ -89,7 +89,11 @@ function patchDatabricksYml( file: string, plan: ConfigPlan, ): { added: string[]; changed: boolean } { - if (plan.bundleVariables.length === 0 && plan.resourceBindings.length === 0) { + if ( + plan.bundleVariables.length === 0 && + plan.resourceBindings.length === 0 && + !plan.userApiScopes?.length + ) { return { added: [], changed: false }; } const doc = loadDoc(file); @@ -127,6 +131,21 @@ function patchDatabricksYml( } } + if (plan.userApiScopes?.length) { + const app = ensureMap(doc, ["resources", "apps", "app"]); + let scopes = app.get("user_api_scopes") as YAMLSeq | undefined; + if (!scopes || typeof (scopes as YAMLSeq).add !== "function") { + app.set("user_api_scopes", doc.createNode([])); + scopes = app.get("user_api_scopes") as YAMLSeq; + } + const existing = new Set(scopes.toJSON() as unknown[]); + for (const scope of plan.userApiScopes) { + if (existing.has(scope)) continue; + scopes.add(scope); + added.push(scope); + } + } + // Target-level variable values. Tracked separately from `added` because the // binding/top-level var may already exist (e.g. from scaffold) while the // target VALUE is still missing — in that case nothing is in `added` yet the diff --git a/packages/shared/src/cli/commands/registry/env-writer.ts b/packages/shared/src/cli/commands/registry/env-writer.ts index 4f06b66e8..7f136a578 100644 --- a/packages/shared/src/cli/commands/registry/env-writer.ts +++ b/packages/shared/src/cli/commands/registry/env-writer.ts @@ -46,13 +46,13 @@ const MANUAL = "__manual__"; const PICKER_LIMIT = 25; /** Reads a `.env`-style file into a map; empty when the file is absent. */ -function readEnvFile(file: string): Record { +export function readEnvFile(file: string): Record { if (!fs.existsSync(file)) return {}; return parseEnv(fs.readFileSync(file, "utf-8")); } /** Appends text to a file, creating it (with a trailing newline) if needed. */ -function appendToFile(file: string, text: string): void { +export function appendToFile(file: string, text: string): void { if (text === "") return; if (fs.existsSync(file)) { const existing = fs.readFileSync(file, "utf-8"); diff --git a/packages/shared/src/cli/commands/registry/server-register.ts b/packages/shared/src/cli/commands/registry/server-register.ts index 327b0f4ee..eab825269 100644 --- a/packages/shared/src/cli/commands/registry/server-register.ts +++ b/packages/shared/src/cli/commands/registry/server-register.ts @@ -91,11 +91,14 @@ export function registerPluginInServer( // exportName and importPath are interpolated into the user's server source. // Registry items are untrusted, so refuse anything that isn't a plain JS // identifier / clean relative module path — prevents code injection via a - // crafted export name or import path. + // crafted export name or import path. Core plugins import from the AppKit package. if (!JS_IDENTIFIER.test(exportName)) { return { status: "skipped", reason: "invalid plugin export name" }; } - if (!/^[.][./A-Za-z0-9_-]*$/.test(importPath)) { + if ( + !/^[.][./A-Za-z0-9_-]*$/.test(importPath) && + !/^@databricks\/appkit(\/beta)?$/.test(importPath) + ) { return { status: "skipped", reason: "invalid plugin import path" }; } diff --git a/packages/shared/src/cli/deploy-config.ts b/packages/shared/src/cli/deploy-config.ts index 9da8c0df4..b0bdec65d 100644 --- a/packages/shared/src/cli/deploy-config.ts +++ b/packages/shared/src/cli/deploy-config.ts @@ -10,11 +10,14 @@ export const DATABRICKS_YML_FILE = "databricks.yml"; /** Canonical Databricks Apps runtime config file name. */ export const APP_YAML_FILE = "app.yaml"; -/** An `app.yaml` env entry: `- name: `, `valueFrom: `. */ -export interface AppYamlEnvEntry { - name: string; - valueFrom: string; -} +/** + * An `app.yaml` env entry: `- name: ` with `valueFrom: `, + * or a literal `value` (resources accessed only on behalf of the user have no + * binding to reference). + */ +export type AppYamlEnvEntry = + | { name: string; valueFrom: string } + | { name: string; value: string }; /** * A `databricks.yml` app-resource binding under `resources.apps..resources[]`, From 1232c23f6cb81932e072e9c2767377b9e2efa507 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 20 Oct 2026 14:34:20 +0200 Subject: [PATCH 02/16] fix(appkit): plugin add OBO files auth, genie scopes, lakebase derived values, local imports - files/aiSearch with only OBO resources are registered with { auth: "on-behalf-of-user" } so the runtime matches the unbound plan - plugin-level scopes (genie) are always written, as the init template does - fields with a resolver (lakebase PGHOST, PGDATABASE, LAKEBASE_ENDPOINT) are resolved from the branch/database like apps init, else prompted or rejected; never written empty - local plugin packages are rebased from the app root to the server file Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../shared/src/cli/commands/plugin/add/add.ts | 176 +++++++++++++----- .../src/cli/commands/plugin/add/plan.test.ts | 25 ++- .../src/cli/commands/plugin/add/plan.ts | 14 +- .../cli/commands/plugin/add/resolve.test.ts | 63 +++++++ .../src/cli/commands/plugin/add/resolve.ts | 49 +++++ .../commands/registry/server-register.test.ts | 15 ++ .../cli/commands/registry/server-register.ts | 6 +- 7 files changed, 292 insertions(+), 56 deletions(-) create mode 100644 packages/shared/src/cli/commands/plugin/add/resolve.test.ts create mode 100644 packages/shared/src/cli/commands/plugin/add/resolve.ts diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index bab01fbc2..add3a941a 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -24,6 +24,7 @@ import { findMissingValues, type ManifestPlugin, type ManifestResource, + type MissingValue, oboCapable, parseSetAuthModes, parseSetValues, @@ -31,6 +32,7 @@ import { resourceConfigured, selectResources, } from "./plan"; +import { resolveDerivedValues } from "./resolve"; const MANIFEST_FILE = "appkit.plugins.json"; const SERVER_SUBDIRS = ["server", "api", "backend"]; @@ -152,6 +154,34 @@ async function promptAuthModes( return modes; } +/** Prompts for each missing value, or fails on the first one when not interactive. */ +async function fillMissing( + missing: MissingValue[], + values: Record, + interactive: boolean, +): Promise { + if (missing.length > 0 && !interactive) { + const m = missing[0]; + throw new Error( + `missing value for ${m.setKey}; use --set ${m.setKey}=value`, + ); + } + if (missing.length === 0) return; + const { text } = await import("@clack/prompts"); + for (const m of missing) { + const field = m.resource.fields?.[m.field]; + const value = await answered( + await text({ + message: `${m.resource.alias}: ${m.field}`, + placeholder: field?.description, + validate: (v) => + !v?.trim() || /[\r\n]/.test(v) ? "Enter a value" : undefined, + }), + ); + values[`${m.resource.resourceKey}.${m.field}`] = value.trim(); + } +} + async function runPluginAdd(name: string, opts: AddOptions): Promise { const cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd(); if ( @@ -189,33 +219,42 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { ); } - const missing = findMissingValues(plugin.name, resources, values, modes); - if (missing.length > 0 && !interactive) { - const m = missing[0]; - throw new Error( - `missing value for ${m.setKey}; use --set ${m.setKey}=value`, - ); - } - if (missing.length > 0) { - const { text } = await import("@clack/prompts"); - for (const m of missing) { - const field = m.resource.fields?.[m.field]; - const value = await answered( - await text({ - message: `${m.resource.alias}: ${m.field}`, - placeholder: field?.description, - validate: (v) => - !v?.trim() || /[\r\n]/.test(v) ? "Enter a value" : undefined, - }), + const missingNow = () => + findMissingValues(plugin.name, resources, values, modes); + // Values the user supplies come first: the derived ones need them. + await fillMissing( + missingNow().filter((m) => !m.resource.fields?.[m.field]?.resolve), + values, + interactive, + ); + const profile = opts.profile ?? profileFromEnv(cwd); + for (const r of resources) { + try { + for (const [k, v] of Object.entries( + await resolveDerivedValues(r, values, profile), + )) { + values[k] ||= v; + } + } catch (err) { + console.warn( + pc.yellow( + `Could not resolve derived values for ${r.alias}: ${(err as Error).message}`, + ), ); - values[`${m.resource.resourceKey}.${m.field}`] = value.trim(); } } + await fillMissing(missingNow(), values, interactive); resources = selectResources(plugin, values); const plan = buildAddPlan(plugin, resources, values, modes); - if (opts.register !== false) await registerInServer(cwd, plugin); + if (opts.register !== false) { + await registerInServer( + cwd, + plugin, + pluginCallArgs(plugin, resources, modes), + ); + } const { appendToFile, readEnvFile } = await import("../../registry/env-writer.js"); @@ -237,50 +276,91 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { const result = writeConfig(cwd, plan); reportConfigWrite(result); if (result.databricksYmlChanged) { - validateBundle(cwd, opts.profile ?? profileFromEnv(cwd)); + validateBundle(cwd, profile); } } } +/** + * Plugins whose identity is a plugin-level `auth` option (default service + * principal) rather than per-request: OBO-only resources need it set, or the + * runtime uses the service principal that the OBO plan leaves unbound. + */ +const AUTH_CONFIG_PLUGINS = new Set(["files", "aiSearch"]); + +/** The `createApp` call arguments for the plugin: the auth option, if any. */ +function pluginCallArgs( + plugin: ManifestPlugin, + resources: ManifestResource[], + modes: Record, +): string { + if (!AUTH_CONFIG_PLUGINS.has(plugin.name)) return ""; + const obo = resources.filter((r) => modes[authKey(r)] === AUTH_MODE_OBO); + if (obo.length === 0) return ""; + if (obo.length < resources.length) { + console.warn( + pc.yellow( + `${plugin.name} has one auth option for all its resources; set auth per volume in its config to mix on-behalf-of-user (${obo.map((r) => r.resourceKey).join(", ")}) with service principal access.`, + ), + ); + return ""; + } + return `{ auth: "on-behalf-of-user" }`; +} + async function registerInServer( cwd: string, plugin: ManifestPlugin, + callArgs: string, ): Promise { const serverRoot = SERVER_SUBDIRS.map((s) => path.join(cwd, s)).find((p) => fs.existsSync(p), ) ?? cwd; - const importPath = - plugin.stability === "beta" ? `${plugin.package}/beta` : plugin.package; // Lazy: server-register pulls in the @ast-grep/napi native addon. - const { registerPluginInServer } = + const { findServerFile, registerPluginInServer } = await import("../../registry/server-register.js"); - { - { - const result = registerPluginInServer( - serverRoot, - importPath, - plugin.name, + let importPath = + plugin.stability === "beta" ? `${plugin.package}/beta` : plugin.package; + // Local plugins are recorded relative to the app root; imports resolve + // relative to the server file. + if (plugin.package.startsWith(".")) { + const from = path.dirname(findServerFile(serverRoot) ?? serverRoot); + const rel = path + .relative(from, path.resolve(cwd, plugin.package)) + .split(path.sep) + .join("/"); + importPath = rel.startsWith(".") ? rel : `./${rel}`; + } + const call = `${plugin.name}(${callArgs})`; + const result = registerPluginInServer( + serverRoot, + importPath, + plugin.name, + callArgs, + ); + const shown = + result.file && path.relative(cwd, path.join(serverRoot, result.file)); + if (result.status === "wired") { + console.log(`${pc.green("Registered")} ${call} in ${shown}`); + } else if (result.status === "already") { + console.log(pc.dim(`${plugin.name} is already registered in ${shown}`)); + if (callArgs) { + console.warn( + pc.yellow( + ` Make sure it is configured as ${call} for the chosen auth mode.`, + ), ); - const shown = - result.file && path.relative(cwd, path.join(serverRoot, result.file)); - if (result.status === "wired") { - console.log(`${pc.green("Registered")} ${plugin.name} in ${shown}`); - } else if (result.status === "already") { - console.log(pc.dim(`${plugin.name} is already registered in ${shown}`)); - } else { - console.log( - pc.yellow( - `Couldn't auto-register ${plugin.name}: ${result.reason}.`, - ) + - `\nAdd it to your server's createApp call:\n` + - pc.dim( - ` import { ${plugin.name} } from "${importPath}";\n` + - ` createApp({ plugins: [${plugin.name}(), /* ... */] });`, - ), - ); - } } + } else { + console.log( + pc.yellow(`Couldn't auto-register ${plugin.name}: ${result.reason}.`) + + "\nAdd it to your server's createApp call:\n" + + pc.dim( + ` import { ${plugin.name} } from "${importPath}";\n` + + ` createApp({ plugins: [${call}, /* ... */] });`, + ), + ); } } diff --git a/packages/shared/src/cli/commands/plugin/add/plan.test.ts b/packages/shared/src/cli/commands/plugin/add/plan.test.ts index f2ae70319..5fb75a28c 100644 --- a/packages/shared/src/cli/commands/plugin/add/plan.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/plan.test.ts @@ -138,13 +138,14 @@ describe("buildAddPlan", () => { const values = { "sql-warehouse.id": "wh", "job.id": "42" }; const resources = selectResources(plugin, values); - test("sp binds the resource with its permission and no scopes", () => { + test("sp binds the resource with its permission; only plugin scopes", () => { const plan = buildAddPlan(plugin, resources, values, {}); expect(resources.map((r) => r.resourceKey)).toEqual([ "sql-warehouse", "job", ]); - expect(plan.userApiScopes).toEqual([]); + // Plugin scopes (genie-style always-OBO routes) apply in sp mode too. + expect(plan.userApiScopes).toEqual(["ai-gateway"]); expect(plan.appYamlEnv).toEqual([ { name: "DATABRICKS_WAREHOUSE_ID", valueFrom: "sql-warehouse" }, { name: "DATABRICKS_JOB_ID", valueFrom: "job" }, @@ -192,3 +193,23 @@ test("findMissingValues names the --set key", () => { findMissingValues("analytics", [warehouse], {}, {}).map((m) => m.setKey), ).toEqual(["analytics.sql-warehouse.id"]); }); + +test("fields with a resolver are required even when localOnly", () => { + const pg = { + type: "postgres", + alias: "Postgres", + resourceKey: "postgres", + appOnly: true, + fields: { + branch: {}, + host: { env: "PGHOST", localOnly: true, resolve: "postgres:host" }, + port: { env: "PGPORT", localOnly: true, value: "5432" }, + }, + binding: { yamlKey: "postgres", varFields: [["branch", "branch"]] }, + } satisfies ManifestPlugin["resources"]["required"][number]; + expect( + findMissingValues("lakebase", [pg], { "postgres.branch": "b" }, {}).map( + (m) => m.setKey, + ), + ).toEqual(["lakebase.postgres.host"]); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/plan.ts b/packages/shared/src/cli/commands/plugin/add/plan.ts index 2f3204443..e812edc99 100644 --- a/packages/shared/src/cli/commands/plugin/add/plan.ts +++ b/packages/shared/src/cli/commands/plugin/add/plan.ts @@ -26,6 +26,8 @@ export interface ManifestField { value?: string; localOnly?: boolean; bundleIgnore?: boolean; + /** Derived-value resolver key, e.g. "postgres:host" (see resolve.ts). */ + resolve?: string; } export interface ManifestResource { @@ -288,7 +290,8 @@ export interface MissingValue { * Values still needed, in prompt order: for each resource bound to the service * principal (sp/both), every field its binding references; for resources * accessed only on behalf of the user, every field with an env var (its - * app.yaml entry carries the literal value). + * app.yaml entry carries the literal value). Fields with a resolver (e.g. + * Lakebase PGHOST) are always needed: local .env must carry them to connect. */ export function findMissingValues( pluginName: string, @@ -304,7 +307,8 @@ export function findMissingValues( (f) => r.fields?.[f].env && !r.fields[f].localOnly, ) : variableFields(r); - for (const field of fields) { + const resolved = fieldNames(r).filter((f) => r.fields?.[f].resolve); + for (const field of new Set([...fields, ...resolved])) { if (values[valueKey(r, field)] || r.fields?.[field]?.value) continue; missing.push({ resource: r, @@ -341,7 +345,7 @@ export interface AddPlan extends ConfigPlan { * Builds the file changes for a plugin's resources, mirroring the CLI * generator: OBO-only resources get a literal app.yaml `value` and no bundle * binding; sp/both resources get variables, a binding and `valueFrom`; - * obo/both resources add their scope (plus the plugin's own scopes) to + * obo/both resources add their scope, and the plugin always adds its own, to * user_api_scopes. */ export function buildAddPlan( @@ -405,7 +409,9 @@ export function buildAddPlan( }); } - if (scopes.length > 0) scopes.push(...(plugin.scopes ?? [])); + // Plugin scopes (e.g. genie) cover routes that always run on behalf of the + // user, so they apply whatever the resources' auth mode is. + scopes.push(...(plugin.scopes ?? [])); return { dotEnv, appYamlEnv, diff --git a/packages/shared/src/cli/commands/plugin/add/resolve.test.ts b/packages/shared/src/cli/commands/plugin/add/resolve.test.ts new file mode 100644 index 000000000..7cb3b0d60 --- /dev/null +++ b/packages/shared/src/cli/commands/plugin/add/resolve.test.ts @@ -0,0 +1,63 @@ +import { expect, test, vi } from "vitest"; + +import type { ManifestResource } from "./plan"; +import { resolveDerivedValues } from "./resolve"; + +const listEndpointsIter = vi.fn(async function* () { + yield { name: "ro", status: { endpointType: "ENDPOINT_TYPE_READ_ONLY" } }; + yield { + name: "b/endpoints/rw", + status: { endpointType: "ENDPOINT_TYPE_READ_WRITE", hosts: { host: "h" } }, + }; +}); +const listDatabasesIter = vi.fn(async function* () { + yield { name: "b/databases/db", status: { postgresDatabase: "appdb" } }; +}); +vi.mock("../../registry/workspace-picker.js", () => ({ + makeWorkspaceClient: () => ({ + postgres: { listEndpointsIter, listDatabasesIter }, + }), +})); + +const pg: ManifestResource = { + type: "postgres", + alias: "Postgres", + resourceKey: "postgres", + fields: { + branch: {}, + database: {}, + host: { env: "PGHOST", resolve: "postgres:host" }, + databaseName: { env: "PGDATABASE", resolve: "postgres:databaseName" }, + endpointPath: { + env: "LAKEBASE_ENDPOINT", + resolve: "postgres:endpointPath", + }, + }, +}; + +test("maps the read-write endpoint and database to resolver fields", async () => { + const values = { + "postgres.branch": "b", + "postgres.database": "b/databases/db", + }; + expect(await resolveDerivedValues(pg, values, undefined)).toEqual({ + "postgres.host": "h", + "postgres.databaseName": "appdb", + "postgres.endpointPath": "b/endpoints/rw", + }); +}); + +test("skips the API when every derived field is set", async () => { + listEndpointsIter.mockClear(); + await resolveDerivedValues( + pg, + { + "postgres.branch": "b", + "postgres.host": "x", + "postgres.databaseName": "x", + "postgres.endpointPath": "x", + }, + undefined, + ); + expect(listEndpointsIter).not.toHaveBeenCalled(); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/resolve.ts b/packages/shared/src/cli/commands/plugin/add/resolve.ts new file mode 100644 index 000000000..94d4d69e2 --- /dev/null +++ b/packages/shared/src/cli/commands/plugin/add/resolve.ts @@ -0,0 +1,49 @@ +import type { ManifestResource } from "./plan"; + +/** + * Derived values for fields with a `resolve` key, a port of the Databricks + * CLI's ResolvePostgresValues (libs/apps/prompt): the branch's read-write + * endpoint gives `postgres:host` and `postgres:endpointPath`, the database + * gives `postgres:databaseName`. Returns `resourceKey.field` → value for the + * fields it could resolve; throws on API errors. + */ +export async function resolveDerivedValues( + r: ManifestResource, + values: Record, + profile: string | undefined, +): Promise> { + const branch = values[`${r.resourceKey}.branch`]; + const database = values[`${r.resourceKey}.database`]; + const needs = Object.entries(r.fields ?? {}).some( + ([name, f]) => + f.resolve?.startsWith("postgres:") && !values[`${r.resourceKey}.${name}`], + ); + if (r.type !== "postgres" || !needs || !branch) return {}; + + const { makeWorkspaceClient } = + await import("../../registry/workspace-picker.js"); + const pg = makeWorkspaceClient(profile).postgres; + const resolved: Record = {}; + for await (const ep of pg.listEndpointsIter({ parent: branch })) { + if (ep.status?.endpointType !== "ENDPOINT_TYPE_READ_WRITE") continue; + if (ep.name) resolved["postgres:endpointPath"] = ep.name; + if (ep.status.hosts?.host) resolved["postgres:host"] = ep.status.hosts.host; + break; + } + if (database) { + for await (const db of pg.listDatabasesIter({ parent: branch })) { + if (db.name !== database) continue; + if (db.status?.postgresDatabase) { + resolved["postgres:databaseName"] = db.status.postgresDatabase; + } + break; + } + } + + const out: Record = {}; + for (const [name, field] of Object.entries(r.fields ?? {})) { + const v = field.resolve && resolved[field.resolve]; + if (v) out[`${r.resourceKey}.${name}`] = v; + } + return out; +} diff --git a/packages/shared/src/cli/commands/registry/server-register.test.ts b/packages/shared/src/cli/commands/registry/server-register.test.ts index 0298545c7..fb96744ad 100644 --- a/packages/shared/src/cli/commands/registry/server-register.test.ts +++ b/packages/shared/src/cli/commands/registry/server-register.test.ts @@ -38,6 +38,21 @@ afterEach(() => { }); describe("registerPluginInServer", () => { + it("passes call args and accepts the AppKit package import path", () => { + const dir = makeTempDir(); + const file = writeServer(dir, "server.ts", "server()"); + const result = registerPluginInServer( + dir, + "@databricks/appkit", + "files", + `{ auth: "on-behalf-of-user" }`, + ); + expect(result.status).toBe("wired"); + const out = fs.readFileSync(file, "utf-8"); + expect(out).toContain(`files({ auth: "on-behalf-of-user" }), server()`); + expect(out).toContain(`import { files } from "@databricks/appkit";`); + }); + it("wires a plugin into a server root that is a subdir (e.g. api/)", () => { // The server root is the resolved subdir, not the repo root — entry files // are looked up within it, so an app laid out under api/ still gets wired. diff --git a/packages/shared/src/cli/commands/registry/server-register.ts b/packages/shared/src/cli/commands/registry/server-register.ts index eab825269..96442541c 100644 --- a/packages/shared/src/cli/commands/registry/server-register.ts +++ b/packages/shared/src/cli/commands/registry/server-register.ts @@ -20,7 +20,7 @@ export interface RegisterResult { reason?: string; } -function findServerFile(serverRoot: string): string | null { +export function findServerFile(serverRoot: string): string | null { for (const candidate of SERVER_FILE_CANDIDATES) { const p = path.join(serverRoot, candidate); if (fs.existsSync(p)) return p; @@ -87,6 +87,8 @@ export function registerPluginInServer( serverRoot: string, importPath: string, exportName: string, + /** Call arguments, e.g. `{ auth: "on-behalf-of-user" }` (CLI-built, never registry data). */ + callArgs = "", ): RegisterResult { // exportName and importPath are interpolated into the user's server source. // Registry items are untrusted, so refuse anything that isn't a plain JS @@ -127,7 +129,7 @@ export function registerPluginInServer( const edits = []; // toPlugin exports are factories, registered as a call: `hello()`. - const newElem = `${exportName}()`; + const newElem = `${exportName}(${callArgs})`; // Insert before the first element, matching its indentation so the array // formatting is preserved (or inline for a single-line array). From 6f1cb430f60e647b318d6f3b50c1d161b368eb22 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 20 Oct 2026 14:44:39 +0200 Subject: [PATCH 03/16] fix(appkit): plugin add sets aiSearch OBO auth on its default index aiSearch routes read auth per index (indexes..auth), not from the plugin config, so an OBO add now registers aiSearch({ indexes: { default: { auth: "on-behalf-of-user" } } }). Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../src/cli/commands/plugin/add/add.test.ts | 34 +++++++++++++++++++ .../shared/src/cli/commands/plugin/add/add.ts | 21 +++++++----- 2 files changed, 47 insertions(+), 8 deletions(-) create mode 100644 packages/shared/src/cli/commands/plugin/add/add.test.ts diff --git a/packages/shared/src/cli/commands/plugin/add/add.test.ts b/packages/shared/src/cli/commands/plugin/add/add.test.ts new file mode 100644 index 000000000..c3bac7389 --- /dev/null +++ b/packages/shared/src/cli/commands/plugin/add/add.test.ts @@ -0,0 +1,34 @@ +import { expect, test } from "vitest"; + +import { pluginCallArgs } from "./add"; +import type { ManifestPlugin, ManifestResource } from "./plan"; + +const index: ManifestResource = { + type: "vector_search_index", + alias: "Index", + resourceKey: "vector-search-index", + scope: "vector-search", +}; +const plugin = (name: string): ManifestPlugin => ({ + name, + package: "@databricks/appkit", + resources: { required: [index], optional: [] }, +}); +const obo = { "vector_search_index:vector-search-index": "obo" } as const; + +test("aiSearch OBO sets auth on the default index, where the routes read it", () => { + expect(pluginCallArgs(plugin("aiSearch"), [index], obo)).toBe( + `{ indexes: { default: { auth: "on-behalf-of-user" } } }`, + ); +}); + +test("files OBO sets the plugin-level auth", () => { + expect(pluginCallArgs(plugin("files"), [index], obo)).toBe( + `{ auth: "on-behalf-of-user" }`, + ); +}); + +test("no args for sp/both or plugins without an auth option", () => { + expect(pluginCallArgs(plugin("aiSearch"), [index], {})).toBe(""); + expect(pluginCallArgs(plugin("analytics"), [index], obo)).toBe(""); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index add3a941a..781906fa1 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -282,30 +282,35 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { } /** - * Plugins whose identity is a plugin-level `auth` option (default service - * principal) rather than per-request: OBO-only resources need it set, or the - * runtime uses the service principal that the OBO plan leaves unbound. + * `createApp` arguments that make a plugin access its resources on behalf of + * the user, for plugins that otherwise default to the service principal (which + * an OBO plan leaves unbound). files reads a plugin-level `auth`; aiSearch reads + * it per index, and its `default` index takes DATABRICKS_VS_INDEX_NAME. */ -const AUTH_CONFIG_PLUGINS = new Set(["files", "aiSearch"]); +const OBO_CALL_ARGS: Record = { + files: `{ auth: "on-behalf-of-user" }`, + aiSearch: `{ indexes: { default: { auth: "on-behalf-of-user" } } }`, +}; /** The `createApp` call arguments for the plugin: the auth option, if any. */ -function pluginCallArgs( +export function pluginCallArgs( plugin: ManifestPlugin, resources: ManifestResource[], modes: Record, ): string { - if (!AUTH_CONFIG_PLUGINS.has(plugin.name)) return ""; + const oboArgs = OBO_CALL_ARGS[plugin.name]; + if (!oboArgs) return ""; const obo = resources.filter((r) => modes[authKey(r)] === AUTH_MODE_OBO); if (obo.length === 0) return ""; if (obo.length < resources.length) { console.warn( pc.yellow( - `${plugin.name} has one auth option for all its resources; set auth per volume in its config to mix on-behalf-of-user (${obo.map((r) => r.resourceKey).join(", ")}) with service principal access.`, + `${plugin.name} is registered with one auth setting for all its resources; set auth per volume or index in its config to mix on-behalf-of-user (${obo.map((r) => r.resourceKey).join(", ")}) with service principal access.`, ), ); return ""; } - return `{ auth: "on-behalf-of-user" }`; + return oboArgs; } async function registerInServer( From aafef48a242524cc812bb365123d76efc7025ba0 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 20 Oct 2026 14:53:09 +0200 Subject: [PATCH 04/16] fix(appkit): plugin add writes aiSearch index columns so production starts aiSearch refuses to start outside development without columns. plugin add now discovers them like the plugin's dev mode (Delta Sync source table minus embedding columns), prompts when that fails, and without a terminal fails before writing anything. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../src/cli/commands/plugin/add/add.test.ts | 30 +++-- .../shared/src/cli/commands/plugin/add/add.ts | 127 ++++++++++++++---- .../cli/commands/plugin/add/resolve.test.ts | 20 ++- .../src/cli/commands/plugin/add/resolve.ts | 28 ++++ 4 files changed, 165 insertions(+), 40 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/add.test.ts b/packages/shared/src/cli/commands/plugin/add/add.test.ts index c3bac7389..d8c7e540a 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.test.ts @@ -1,6 +1,6 @@ import { expect, test } from "vitest"; -import { pluginCallArgs } from "./add"; +import { pluginConfig, toSource } from "./add"; import type { ManifestPlugin, ManifestResource } from "./plan"; const index: ManifestResource = { @@ -16,19 +16,29 @@ const plugin = (name: string): ManifestPlugin => ({ }); const obo = { "vector_search_index:vector-search-index": "obo" } as const; -test("aiSearch OBO sets auth on the default index, where the routes read it", () => { - expect(pluginCallArgs(plugin("aiSearch"), [index], obo)).toBe( - `{ indexes: { default: { auth: "on-behalf-of-user" } } }`, +test("aiSearch sets columns and OBO auth on the default index", () => { + const config = pluginConfig(plugin("aiSearch"), [index], obo, ["id", "t"]); + expect(toSource(config)).toBe( + `{ indexes: { default: { columns: ["id", "t"], auth: "on-behalf-of-user" } } }`, ); + expect(pluginConfig(plugin("aiSearch"), [index], {}, ["id"])).toEqual({ + indexes: { default: { columns: ["id"] } }, + }); }); test("files OBO sets the plugin-level auth", () => { - expect(pluginCallArgs(plugin("files"), [index], obo)).toBe( - `{ auth: "on-behalf-of-user" }`, - ); + expect(pluginConfig(plugin("files"), [index], obo)).toEqual({ + auth: "on-behalf-of-user", + }); +}); + +test("no config for sp or plugins without an auth option", () => { + expect(pluginConfig(plugin("files"), [index], {})).toBeUndefined(); + expect(pluginConfig(plugin("analytics"), [index], obo)).toBeUndefined(); }); -test("no args for sp/both or plugins without an auth option", () => { - expect(pluginCallArgs(plugin("aiSearch"), [index], {})).toBe(""); - expect(pluginCallArgs(plugin("analytics"), [index], obo)).toBe(""); +test("toSource escapes strings and quotes non-identifier keys", () => { + expect(toSource({ "a-b": ['x"); evil()'] })).toBe( + `{ "a-b": ["x\\"); evil()"] }`, + ); }); diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index 781906fa1..3e0691699 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -32,7 +32,7 @@ import { resourceConfigured, selectResources, } from "./plan"; -import { resolveDerivedValues } from "./resolve"; +import { discoverIndexColumns, resolveDerivedValues } from "./resolve"; const MANIFEST_FILE = "appkit.plugins.json"; const SERVER_SUBDIRS = ["server", "api", "backend"]; @@ -182,6 +182,51 @@ async function fillMissing( } } +/** + * Columns for the aiSearch default index: discovered from the workspace, else + * prompted. Without a terminal it fails before anything is written, since the + * app would not start in production without them. + */ +async function aiSearchColumns( + values: Record, + profile: string | undefined, + interactive: boolean, +): Promise { + const indexName = values["vector-search-index.id"]; + try { + const columns = await discoverIndexColumns(indexName, profile); + if (columns.length > 0) { + console.log(pc.dim(`Index columns: ${columns.join(", ")}`)); + return columns; + } + } catch (err) { + console.warn( + pc.yellow( + `Could not discover the columns of ${indexName}: ${(err as Error).message}`, + ), + ); + } + if (!interactive) { + throw new Error( + `aiSearch needs the columns of ${indexName} to start in production; run with workspace access (--profile) or interactively to enter them`, + ); + } + const { text } = await import("@clack/prompts"); + const answer = await answered( + await text({ + message: `Columns to return from ${indexName} (comma-separated)`, + validate: (v) => + v?.split(",").some((c) => c.trim()) + ? undefined + : "Enter at least one column", + }), + ); + return answer + .split(",") + .map((c) => c.trim()) + .filter(Boolean); +} + async function runPluginAdd(name: string, opts: AddOptions): Promise { const cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd(); if ( @@ -246,14 +291,19 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { await fillMissing(missingNow(), values, interactive); resources = selectResources(plugin, values); + const config = pluginConfig( + plugin, + resources, + modes, + plugin.name === "aiSearch" && opts.register !== false + ? await aiSearchColumns(values, profile, interactive) + : undefined, + ); + const plan = buildAddPlan(plugin, resources, values, modes); if (opts.register !== false) { - await registerInServer( - cwd, - plugin, - pluginCallArgs(plugin, resources, modes), - ); + await registerInServer(cwd, plugin, config ? toSource(config) : ""); } const { appendToFile, readEnvFile } = @@ -281,36 +331,55 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { } } +/** Plugins that default to the service principal unless told otherwise. */ +const AUTH_OPTION_PLUGINS = new Set(["files", "aiSearch"]); +const OBO = "on-behalf-of-user"; + /** - * `createApp` arguments that make a plugin access its resources on behalf of - * the user, for plugins that otherwise default to the service principal (which - * an OBO plan leaves unbound). files reads a plugin-level `auth`; aiSearch reads - * it per index, and its `default` index takes DATABRICKS_VS_INDEX_NAME. + * The `createApp` config for the plugin, or undefined for none. An OBO plan + * leaves the service principal unbound, so plugins that default to it must be + * told: files reads a plugin-level `auth`, aiSearch reads it per index. The + * aiSearch `default` index takes DATABRICKS_VS_INDEX_NAME, and production + * refuses to start without its `columns`. */ -const OBO_CALL_ARGS: Record = { - files: `{ auth: "on-behalf-of-user" }`, - aiSearch: `{ indexes: { default: { auth: "on-behalf-of-user" } } }`, -}; - -/** The `createApp` call arguments for the plugin: the auth option, if any. */ -export function pluginCallArgs( +export function pluginConfig( plugin: ManifestPlugin, resources: ManifestResource[], modes: Record, -): string { - const oboArgs = OBO_CALL_ARGS[plugin.name]; - if (!oboArgs) return ""; - const obo = resources.filter((r) => modes[authKey(r)] === AUTH_MODE_OBO); - if (obo.length === 0) return ""; - if (obo.length < resources.length) { - console.warn( - pc.yellow( - `${plugin.name} is registered with one auth setting for all its resources; set auth per volume or index in its config to mix on-behalf-of-user (${obo.map((r) => r.resourceKey).join(", ")}) with service principal access.`, - ), + columns?: string[], +): Record | undefined { + let obo = false; + if (AUTH_OPTION_PLUGINS.has(plugin.name)) { + const oboRes = resources.filter((r) => modes[authKey(r)] === AUTH_MODE_OBO); + obo = oboRes.length > 0 && oboRes.length === resources.length; + if (oboRes.length > 0 && !obo) { + console.warn( + pc.yellow( + `${plugin.name} is registered with one auth setting for all its resources; set auth per volume or index in its config to mix on-behalf-of-user (${oboRes.map((r) => r.resourceKey).join(", ")}) with service principal access.`, + ), + ); + } + } + if (plugin.name === "aiSearch") { + const index = { ...(columns && { columns }), ...(obo && { auth: OBO }) }; + return Object.keys(index).length > 0 + ? { indexes: { default: index } } + : undefined; + } + return obo ? { auth: OBO } : undefined; +} + +/** Source text for a config literal: `{ auth: "x", columns: ["a"] }`. */ +export function toSource(v: unknown): string { + if (Array.isArray(v)) return `[${v.map(toSource).join(", ")}]`; + if (v && typeof v === "object") { + const props = Object.entries(v).map( + ([k, x]) => + `${/^[A-Za-z_$][\w$]*$/.test(k) ? k : JSON.stringify(k)}: ${toSource(x)}`, ); - return ""; + return `{ ${props.join(", ")} }`; } - return oboArgs; + return JSON.stringify(v); } async function registerInServer( diff --git a/packages/shared/src/cli/commands/plugin/add/resolve.test.ts b/packages/shared/src/cli/commands/plugin/add/resolve.test.ts index 7cb3b0d60..5338f681a 100644 --- a/packages/shared/src/cli/commands/plugin/add/resolve.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/resolve.test.ts @@ -1,7 +1,7 @@ import { expect, test, vi } from "vitest"; import type { ManifestResource } from "./plan"; -import { resolveDerivedValues } from "./resolve"; +import { discoverIndexColumns, resolveDerivedValues } from "./resolve"; const listEndpointsIter = vi.fn(async function* () { yield { name: "ro", status: { endpointType: "ENDPOINT_TYPE_READ_ONLY" } }; @@ -16,6 +16,20 @@ const listDatabasesIter = vi.fn(async function* () { vi.mock("../../registry/workspace-picker.js", () => ({ makeWorkspaceClient: () => ({ postgres: { listEndpointsIter, listDatabasesIter }, + vectorSearch: { + getVectorIndex: async () => ({ + indexSpec: { + $case: "deltaSyncIndexSpec", + deltaSyncIndexSpec: { + sourceTable: "c.s.t", + embeddingVectorColumns: [{ name: "vec" }], + }, + }, + }), + }, + tables: { + getTable: async () => ({ columns: [{ name: "id" }, { name: "vec" }] }), + }, }), })); @@ -61,3 +75,7 @@ test("skips the API when every derived field is set", async () => { ); expect(listEndpointsIter).not.toHaveBeenCalled(); }); + +test("discoverIndexColumns drops embedding columns", async () => { + expect(await discoverIndexColumns("c.s.i", undefined)).toEqual(["id"]); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/resolve.ts b/packages/shared/src/cli/commands/plugin/add/resolve.ts index 94d4d69e2..9e326e2c2 100644 --- a/packages/shared/src/cli/commands/plugin/add/resolve.ts +++ b/packages/shared/src/cli/commands/plugin/add/resolve.ts @@ -47,3 +47,31 @@ export async function resolveDerivedValues( } return out; } + +/** + * Columns for an AI Search index, as the plugin's development-mode discovery + * finds them: the Delta Sync source table's columns minus embedding vectors. + * Production refuses to start without `columns`, so they are written into the + * plugin config at add time. Throws when the index has no source table. + */ +export async function discoverIndexColumns( + indexName: string, + profile: string | undefined, +): Promise { + const { makeWorkspaceClient } = + await import("../../registry/workspace-picker.js"); + const client = makeWorkspaceClient(profile); + const index = await client.vectorSearch.getVectorIndex({ name: indexName }); + const spec = + index.indexSpec?.$case === "deltaSyncIndexSpec" + ? index.indexSpec.deltaSyncIndexSpec + : undefined; + if (!spec?.sourceTable) { + throw new Error(`index ${indexName} has no Delta Sync source table`); + } + const excluded = new Set(spec.embeddingVectorColumns?.map((c) => c.name)); + const table = await client.tables.getTable({ fullNameArg: spec.sourceTable }); + return (table.columns ?? []).flatMap((c) => + c.name && !excluded.has(c.name) ? [c.name] : [], + ); +} From f4050589302661312262e5c6aef1eaa01acfc461 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 20 Oct 2026 16:01:20 +0200 Subject: [PATCH 05/16] fix(appkit): plugin add agents OBO, .env placeholders, defaults, app key, packages, bindings - agents (like files) is registered with auth: on-behalf-of-user when its OBO-capable resources are OBO; resources that can't be OBO don't block it - empty .env placeholders are filled in place; a conflicting set value warns - manifest field defaults fill the bundle variable value - databricks.yml edits go to the bundle's only app, whatever its key - plugins from external npm packages are registered (package specifier) - resources without binding metadata use the built-in binding for the type Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../src/cli/commands/plugin/add/add.test.ts | 11 ++++ .../shared/src/cli/commands/plugin/add/add.ts | 52 ++++++++++++++----- .../src/cli/commands/plugin/add/plan.test.ts | 36 +++++++++++++ .../src/cli/commands/plugin/add/plan.ts | 39 ++++++++++---- .../commands/registry/config-writer.test.ts | 30 +++++++++++ .../cli/commands/registry/config-writer.ts | 18 ++++++- .../commands/registry/server-register.test.ts | 11 ++++ .../cli/commands/registry/server-register.ts | 5 +- 8 files changed, 174 insertions(+), 28 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/add.test.ts b/packages/shared/src/cli/commands/plugin/add/add.test.ts index d8c7e540a..d2e2144c2 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.test.ts @@ -42,3 +42,14 @@ test("toSource escapes strings and quotes non-identifier keys", () => { `{ "a-b": ["x\\"); evil()"] }`, ); }); + +test("agents OBO sets plugin auth; resources that can't be OBO don't block it", () => { + const experiment: ManifestResource = { + type: "experiment", + alias: "Experiment", + resourceKey: "agents-mlflow-experiment", + }; + expect(pluginConfig(plugin("agents"), [index, experiment], obo)).toEqual({ + auth: "on-behalf-of-user", + }); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index 3e0691699..4cdcd25d0 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -310,16 +310,36 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { await import("../../registry/env-writer.js"); const envPath = path.join(cwd, ".env"); const existing = readEnvFile(envPath); - const newEnv = plan.dotEnv.filter( - (e, i) => - !(e.env in existing) && - plan.dotEnv.findIndex((x) => x.env === e.env) === i, + const dotEnv = plan.dotEnv.filter( + (e, i) => plan.dotEnv.findIndex((x) => x.env === e.env) === i, ); + // Empty placeholders (`KEY=`) are filled in place; set values are kept. + const filled = dotEnv.filter((e) => existing[e.env] === "" && e.value); + if (filled.length > 0) { + let content = fs.readFileSync(envPath, "utf-8"); + for (const e of filled) { + // e.env is a validated identifier, so it is safe in the pattern. + content = content.replace( + new RegExp(`^(\\s*(?:export\\s+)?${e.env}\\s*=).*$`, "m"), + (_, key: string) => key + e.value, + ); + } + fs.writeFileSync(envPath, content); + } + const newEnv = dotEnv.filter((e) => !(e.env in existing)); appendToFile(envPath, serializeEnvAppend(newEnv)); - if (newEnv.length > 0) { - console.log( - `${pc.green("Updated .env:")} ${newEnv.map((e) => e.env).join(", ")}`, - ); + const updated = [...filled, ...newEnv].map((e) => e.env); + if (updated.length > 0) { + console.log(`${pc.green("Updated .env:")} ${updated.join(", ")}`); + } + for (const e of dotEnv) { + if (existing[e.env] && e.value && existing[e.env] !== e.value) { + console.warn( + pc.yellow( + `.env already sets ${e.env}; kept it (the app.yaml and databricks.yml entries use the new value).`, + ), + ); + } } if (planHasContent(plan)) { @@ -332,13 +352,13 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { } /** Plugins that default to the service principal unless told otherwise. */ -const AUTH_OPTION_PLUGINS = new Set(["files", "aiSearch"]); +const AUTH_OPTION_PLUGINS = new Set(["files", "aiSearch", "agents"]); const OBO = "on-behalf-of-user"; /** * The `createApp` config for the plugin, or undefined for none. An OBO plan * leaves the service principal unbound, so plugins that default to it must be - * told: files reads a plugin-level `auth`, aiSearch reads it per index. The + * told: files and agents read a plugin-level `auth`, aiSearch reads it per index. The * aiSearch `default` index takes DATABRICKS_VS_INDEX_NAME, and production * refuses to start without its `columns`. */ @@ -350,8 +370,11 @@ export function pluginConfig( ): Record | undefined { let obo = false; if (AUTH_OPTION_PLUGINS.has(plugin.name)) { - const oboRes = resources.filter((r) => modes[authKey(r)] === AUTH_MODE_OBO); - obo = oboRes.length > 0 && oboRes.length === resources.length; + // Resources that can't be accessed on behalf of the user (e.g. an agents + // tracing experiment) stay on the service principal and don't count. + const capable = resources.filter(oboCapable); + const oboRes = capable.filter((r) => modes[authKey(r)] === AUTH_MODE_OBO); + obo = oboRes.length > 0 && oboRes.length === capable.length; if (oboRes.length > 0 && !obo) { console.warn( pc.yellow( @@ -394,8 +417,11 @@ async function registerInServer( // Lazy: server-register pulls in the @ast-grep/napi native addon. const { findServerFile, registerPluginInServer } = await import("../../registry/server-register.js"); + // The /beta subpath is AppKit's; other packages export their plugins as is. let importPath = - plugin.stability === "beta" ? `${plugin.package}/beta` : plugin.package; + plugin.stability === "beta" && plugin.package === "@databricks/appkit" + ? `${plugin.package}/beta` + : plugin.package; // Local plugins are recorded relative to the app root; imports resolve // relative to the server file. if (plugin.package.startsWith(".")) { diff --git a/packages/shared/src/cli/commands/plugin/add/plan.test.ts b/packages/shared/src/cli/commands/plugin/add/plan.test.ts index 5fb75a28c..e7be3da51 100644 --- a/packages/shared/src/cli/commands/plugin/add/plan.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/plan.test.ts @@ -213,3 +213,39 @@ test("fields with a resolver are required even when localOnly", () => { ), ).toEqual(["lakebase.postgres.host"]); }); + +test("a manifest default fills the bundle variable", () => { + const p: ManifestPlugin = { + ...plugin, + resources: { + required: [ + { + ...warehouse, + fields: { id: { env: "DATABRICKS_WAREHOUSE_ID", value: "dflt" } }, + }, + ], + optional: [], + }, + }; + const plan = buildAddPlan(p, p.resources.required, {}, {}); + expect(plan.bundleVariables[0]).toMatchObject({ + name: "sql_warehouse_id", + value: "dflt", + }); +}); + +test("a resource without binding metadata uses the built-in binding", () => { + const { binding: _, ...noBinding } = warehouse; + const plan = buildAddPlan( + plugin, + [noBinding], + { "sql-warehouse.id": "wh" }, + {}, + ); + expect(plan.unverifiedTypes).toEqual([]); + expect(plan.resourceBindings[0]).toMatchObject({ + name: "sql-warehouse", + type: "sql_warehouse", + fields: { id: "${var.sql_warehouse_id}" }, + }); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/plan.ts b/packages/shared/src/cli/commands/plugin/add/plan.ts index e812edc99..8a5a7bf19 100644 --- a/packages/shared/src/cli/commands/plugin/add/plan.ts +++ b/packages/shared/src/cli/commands/plugin/add/plan.ts @@ -1,3 +1,4 @@ +import { DABS_BINDING_BY_TYPE } from "../../../../schemas/manifest"; import type { AppYamlEnvEntry, ResourceBinding } from "../../../deploy-config"; import type { BundleVariable, ConfigPlan } from "../../registry/config-plan"; import { isValidEnvName } from "../../registry/requirements"; @@ -39,11 +40,26 @@ export interface ManifestResource { fields?: Record; scope?: string; appOnly?: boolean; - binding?: { - yamlKey: string; - varFields: [string, string][]; - staticFields?: [string, string][]; - }; + binding?: Binding; +} + +type FieldPair = readonly [string, string]; +interface Binding { + yamlKey: string; + varFields: readonly FieldPair[]; + staticFields?: readonly FieldPair[]; +} + +/** + * The resource's databricks.yml binding: the manifest's, else the built-in + * one for its type (`binding` is optional in the manifest schema; the + * Databricks CLI generator falls back the same way). + */ +function bindingOf(r: ManifestResource): Binding | undefined { + return ( + r.binding ?? + (DABS_BINDING_BY_TYPE as Record)[r.type] + ); } /** A plugin entry of the app's appkit.plugins.json (the fields used here). */ @@ -75,7 +91,7 @@ function allResources(p: ManifestPlugin): ManifestResource[] { /** Manifest fields the resource's databricks.yml binding references. */ function bindingVarFields(r: ManifestResource): string[] { - return (r.binding?.varFields ?? []).map(([f]) => f); + return (bindingOf(r)?.varFields ?? []).map(([f]) => f); } /** @@ -385,7 +401,8 @@ export function buildAddPlan( } if (oboOnly) continue; - if (!r.binding) { + const binding = bindingOf(r); + if (!binding) { unverifiedTypes.push(r.type); continue; } @@ -393,17 +410,17 @@ export function buildAddPlan( bundleVariables.push({ name: varName(r, f), description: r.fields?.[f]?.description || r.description, - value: values[valueKey(r, f)] || undefined, + value: values[valueKey(r, f)] || r.fields?.[f]?.value || undefined, }); } const fields: Record = {}; - for (const [mf, yf] of r.binding.varFields) { + for (const [mf, yf] of binding.varFields) { fields[yf] = `\${var.${varName(r, mf)}}`; } - for (const [yf, v] of r.binding.staticFields ?? []) fields[yf] = v; + for (const [yf, v] of binding.staticFields ?? []) fields[yf] = v; resourceBindings.push({ name: r.resourceKey, - type: r.binding.yamlKey, + type: binding.yamlKey, permission: r.permission, fields, }); diff --git a/packages/shared/src/cli/commands/registry/config-writer.test.ts b/packages/shared/src/cli/commands/registry/config-writer.test.ts index 53c5152d7..51a13cb7c 100644 --- a/packages/shared/src/cli/commands/registry/config-writer.test.ts +++ b/packages/shared/src/cli/commands/registry/config-writer.test.ts @@ -224,3 +224,33 @@ describe("writeConfig — user_api_scopes and literal env values", () => { expect(app.env).toEqual([{ name: "DATABRICKS_WAREHOUSE_ID", value: "wh" }]); }); }); + +describe("writeConfig — app key", () => { + it("writes scopes and bindings to the bundle's only app, whatever its key", () => { + const cwd = makeTempDir(); + fs.writeFileSync( + path.join(cwd, "databricks.yml"), + "resources:\n apps:\n dashboard:\n name: dash\n", + ); + writeConfig(cwd, { + appYamlEnv: [], + bundleVariables: [], + resourceBindings: [ + { + name: "wh", + type: "sql_warehouse", + permission: "CAN_USE", + fields: {}, + }, + ], + userApiScopes: ["sql"], + unverifiedTypes: [], + }); + const apps = parseDocument( + fs.readFileSync(path.join(cwd, "databricks.yml"), "utf-8"), + ).toJSON().resources.apps; + expect(Object.keys(apps)).toEqual(["dashboard"]); + expect(apps.dashboard.user_api_scopes).toEqual(["sql"]); + expect(apps.dashboard.resources[0].name).toBe("wh"); + }); +}); diff --git a/packages/shared/src/cli/commands/registry/config-writer.ts b/packages/shared/src/cli/commands/registry/config-writer.ts index 065d15f8d..d2cd1e9da 100644 --- a/packages/shared/src/cli/commands/registry/config-writer.ts +++ b/packages/shared/src/cli/commands/registry/config-writer.ts @@ -79,6 +79,20 @@ function ensureMap( return node; } +/** + * Key of the bundle's app under `resources.apps`: the only one there (apps + * are often named after the project, not `app`), else `app`. Throws when + * several apps exist and none is `app`, rather than guess or add a new one. + */ +function appKey(doc: ReturnType): string { + const keys = Object.keys(doc.toJSON()?.resources?.apps ?? {}); + if (keys.length === 1) return keys[0]; + if (keys.length === 0 || keys.includes("app")) return "app"; + throw new Error( + `databricks.yml defines several apps (${keys.join(", ")}); add the resources to the right one by hand`, + ); +} + /** * Additively patches `databricks.yml`: adds bundle `variables`, the app * `resources` bindings, and the target-level variable values — each only if @@ -113,7 +127,7 @@ function patchDatabricksYml( // App resource bindings. if (plan.resourceBindings.length > 0) { - const app = ensureMap(doc, ["resources", "apps", "app"]); + const app = ensureMap(doc, ["resources", "apps", appKey(doc)]); let bindings = app.get("resources") as YAMLSeq | undefined; if (!bindings || typeof (bindings as YAMLSeq).add !== "function") { app.set("resources", doc.createNode([])); @@ -132,7 +146,7 @@ function patchDatabricksYml( } if (plan.userApiScopes?.length) { - const app = ensureMap(doc, ["resources", "apps", "app"]); + const app = ensureMap(doc, ["resources", "apps", appKey(doc)]); let scopes = app.get("user_api_scopes") as YAMLSeq | undefined; if (!scopes || typeof (scopes as YAMLSeq).add !== "function") { app.set("user_api_scopes", doc.createNode([])); diff --git a/packages/shared/src/cli/commands/registry/server-register.test.ts b/packages/shared/src/cli/commands/registry/server-register.test.ts index fb96744ad..34d089a22 100644 --- a/packages/shared/src/cli/commands/registry/server-register.test.ts +++ b/packages/shared/src/cli/commands/registry/server-register.test.ts @@ -38,6 +38,17 @@ afterEach(() => { }); describe("registerPluginInServer", () => { + it("accepts an external npm package and rejects code in the path", () => { + const dir = makeTempDir(); + writeServer(dir, "server.ts", "server()"); + expect( + registerPluginInServer(dir, "@example/analytics", "analytics").status, + ).toBe("wired"); + expect(registerPluginInServer(dir, `x"; evil(); "`, "other").status).toBe( + "skipped", + ); + }); + it("passes call args and accepts the AppKit package import path", () => { const dir = makeTempDir(); const file = writeServer(dir, "server.ts", "server()"); diff --git a/packages/shared/src/cli/commands/registry/server-register.ts b/packages/shared/src/cli/commands/registry/server-register.ts index 96442541c..26b10eb33 100644 --- a/packages/shared/src/cli/commands/registry/server-register.ts +++ b/packages/shared/src/cli/commands/registry/server-register.ts @@ -93,13 +93,14 @@ export function registerPluginInServer( // exportName and importPath are interpolated into the user's server source. // Registry items are untrusted, so refuse anything that isn't a plain JS // identifier / clean relative module path — prevents code injection via a - // crafted export name or import path. Core plugins import from the AppKit package. + // crafted export name or import path. Plugins declared by an npm package + // (AppKit's or another) import from a plain package specifier. if (!JS_IDENTIFIER.test(exportName)) { return { status: "skipped", reason: "invalid plugin export name" }; } if ( !/^[.][./A-Za-z0-9_-]*$/.test(importPath) && - !/^@databricks\/appkit(\/beta)?$/.test(importPath) + !/^(@[a-z0-9][\w.-]*\/)?[a-z0-9][\w.-]*(\/[\w.-]+)*$/i.test(importPath) ) { return { status: "skipped", reason: "invalid plugin import path" }; } From ee2aba3564de7e23a938d8cf6147ccd8b46c7bb4 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 20 Oct 2026 16:11:03 +0200 Subject: [PATCH 06/16] fix(appkit): plugin add agents OBO follows the model endpoint only The skills volume is read by the service principal at boot whatever the agent auth is, so an SP skills volume no longer keeps an OBO model on the service principal. An OBO skills volume now warns that it needs sp/both. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../src/cli/commands/plugin/add/add.test.ts | 38 ++++++++++++++++++- .../shared/src/cli/commands/plugin/add/add.ts | 27 ++++++++++--- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/add.test.ts b/packages/shared/src/cli/commands/plugin/add/add.test.ts index d2e2144c2..59d27286f 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.test.ts @@ -49,7 +49,43 @@ test("agents OBO sets plugin auth; resources that can't be OBO don't block it", alias: "Experiment", resourceKey: "agents-mlflow-experiment", }; - expect(pluginConfig(plugin("agents"), [index, experiment], obo)).toEqual({ + const endpoint: ManifestResource = { + type: "serving_endpoint", + alias: "LLM", + resourceKey: "agents-serving-endpoint", + scope: "model-serving", + }; + expect( + pluginConfig(plugin("agents"), [endpoint, experiment], { + "serving_endpoint:agents-serving-endpoint": "obo", + }), + ).toEqual({ + auth: "on-behalf-of-user", + }); +}); + +test("agents auth follows the model endpoint, not the skills volume", () => { + const endpoint: ManifestResource = { + type: "serving_endpoint", + alias: "LLM", + resourceKey: "agents-serving-endpoint", + scope: "model-serving", + }; + const skills: ManifestResource = { + type: "volume", + alias: "Skills", + resourceKey: "agents-skills", + scope: "files", + }; + const modelObo = { + "serving_endpoint:agents-serving-endpoint": "obo", + } as const; + expect(pluginConfig(plugin("agents"), [endpoint, skills], modelObo)).toEqual({ auth: "on-behalf-of-user", }); + expect( + pluginConfig(plugin("agents"), [endpoint, skills], { + "volume:agents-skills": "obo", + }), + ).toBeUndefined(); }); diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index 4cdcd25d0..a7b44b302 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -370,11 +370,28 @@ export function pluginConfig( ): Record | undefined { let obo = false; if (AUTH_OPTION_PLUGINS.has(plugin.name)) { - // Resources that can't be accessed on behalf of the user (e.g. an agents - // tracing experiment) stay on the service principal and don't count. - const capable = resources.filter(oboCapable); - const oboRes = capable.filter((r) => modes[authKey(r)] === AUTH_MODE_OBO); - obo = oboRes.length > 0 && oboRes.length === capable.length; + // The resources whose access the option governs. For agents that is the + // model endpoint: skills load from their volume as the service principal + // at boot whatever `auth` is, and the tracing experiment can't be OBO. + // Elsewhere, every resource that can be accessed on behalf of the user. + const governed = resources.filter( + (r) => + oboCapable(r) && + (plugin.name !== "agents" || r.type === "serving_endpoint"), + ); + const oboRes = governed.filter((r) => modes[authKey(r)] === AUTH_MODE_OBO); + obo = oboRes.length > 0 && oboRes.length === governed.length; + if (plugin.name === "agents") { + for (const r of resources) { + if (r.type === "volume" && modes[authKey(r)] === AUTH_MODE_OBO) { + console.warn( + pc.yellow( + `agents loads skills from ${r.resourceKey} as the service principal at boot, which gets no access in obo mode; use --set agents.${r.resourceKey}.authMode=sp (or both).`, + ), + ); + } + } + } if (oboRes.length > 0 && !obo) { console.warn( pc.yellow( From b28a02a40980b8ea587f792c26b904fd4e08ed84 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 20 Oct 2026 16:50:31 +0200 Subject: [PATCH 07/16] fix(appkit): plugin add limits aiSearch columns to the index's synced columns Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../cli/commands/plugin/add/resolve.test.ts | 26 +++++++++++++++++-- .../src/cli/commands/plugin/add/resolve.ts | 12 ++++++--- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/resolve.test.ts b/packages/shared/src/cli/commands/plugin/add/resolve.test.ts index 5338f681a..936862b03 100644 --- a/packages/shared/src/cli/commands/plugin/add/resolve.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/resolve.test.ts @@ -3,6 +3,7 @@ import { expect, test, vi } from "vitest"; import type { ManifestResource } from "./plan"; import { discoverIndexColumns, resolveDerivedValues } from "./resolve"; +let columnsToSync: string[] | undefined; const listEndpointsIter = vi.fn(async function* () { yield { name: "ro", status: { endpointType: "ENDPOINT_TYPE_READ_ONLY" } }; yield { @@ -23,12 +24,20 @@ vi.mock("../../registry/workspace-picker.js", () => ({ deltaSyncIndexSpec: { sourceTable: "c.s.t", embeddingVectorColumns: [{ name: "vec" }], + columnsToSync, }, }, }), }, tables: { - getTable: async () => ({ columns: [{ name: "id" }, { name: "vec" }] }), + getTable: async () => ({ + columns: [ + { name: "id" }, + { name: "text" }, + { name: "vec" }, + { name: "notes" }, + ], + }), }, }), })); @@ -77,5 +86,18 @@ test("skips the API when every derived field is set", async () => { }); test("discoverIndexColumns drops embedding columns", async () => { - expect(await discoverIndexColumns("c.s.i", undefined)).toEqual(["id"]); + columnsToSync = undefined; + expect(await discoverIndexColumns("c.s.i", undefined)).toEqual([ + "id", + "text", + "notes", + ]); +}); + +test("discoverIndexColumns keeps only synced columns", async () => { + columnsToSync = ["id", "text", "vec"]; + expect(await discoverIndexColumns("c.s.i", undefined)).toEqual([ + "id", + "text", + ]); }); diff --git a/packages/shared/src/cli/commands/plugin/add/resolve.ts b/packages/shared/src/cli/commands/plugin/add/resolve.ts index 9e326e2c2..e27a14ef4 100644 --- a/packages/shared/src/cli/commands/plugin/add/resolve.ts +++ b/packages/shared/src/cli/commands/plugin/add/resolve.ts @@ -49,8 +49,9 @@ export async function resolveDerivedValues( } /** - * Columns for an AI Search index, as the plugin's development-mode discovery - * finds them: the Delta Sync source table's columns minus embedding vectors. + * Columns for an AI Search index: the Delta Sync source table's columns minus + * embedding vectors, limited to `columnsToSync` when the index syncs a subset + * (other source columns aren't in the index, and querying them fails). * Production refuses to start without `columns`, so they are written into the * plugin config at add time. Throws when the index has no source table. */ @@ -71,7 +72,12 @@ export async function discoverIndexColumns( } const excluded = new Set(spec.embeddingVectorColumns?.map((c) => c.name)); const table = await client.tables.getTable({ fullNameArg: spec.sourceTable }); + const synced = spec.columnsToSync?.length + ? new Set(spec.columnsToSync) + : undefined; return (table.columns ?? []).flatMap((c) => - c.name && !excluded.has(c.name) ? [c.name] : [], + c.name && !excluded.has(c.name) && (!synced || synced.has(c.name)) + ? [c.name] + : [], ); } From 2959afcac3988d6215926226ae2296ae814b76ec Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 20 Oct 2026 16:56:52 +0200 Subject: [PATCH 08/16] fix(appkit): plugin add honors the columnsToIndex alias for aiSearch columns Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../src/cli/commands/plugin/add/resolve.test.ts | 12 ++++++++++++ .../shared/src/cli/commands/plugin/add/resolve.ts | 10 ++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/resolve.test.ts b/packages/shared/src/cli/commands/plugin/add/resolve.test.ts index 936862b03..0e756aadc 100644 --- a/packages/shared/src/cli/commands/plugin/add/resolve.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/resolve.test.ts @@ -4,6 +4,7 @@ import type { ManifestResource } from "./plan"; import { discoverIndexColumns, resolveDerivedValues } from "./resolve"; let columnsToSync: string[] | undefined; +let columnsToIndex: string[] | undefined; const listEndpointsIter = vi.fn(async function* () { yield { name: "ro", status: { endpointType: "ENDPOINT_TYPE_READ_ONLY" } }; yield { @@ -25,6 +26,7 @@ vi.mock("../../registry/workspace-picker.js", () => ({ sourceTable: "c.s.t", embeddingVectorColumns: [{ name: "vec" }], columnsToSync, + columnsToIndex, }, }, }), @@ -101,3 +103,13 @@ test("discoverIndexColumns keeps only synced columns", async () => { "text", ]); }); + +test("discoverIndexColumns honors the columnsToIndex alias", async () => { + columnsToSync = undefined; + columnsToIndex = ["id", "text", "vec"]; + expect(await discoverIndexColumns("c.s.i", undefined)).toEqual([ + "id", + "text", + ]); + columnsToIndex = undefined; +}); diff --git a/packages/shared/src/cli/commands/plugin/add/resolve.ts b/packages/shared/src/cli/commands/plugin/add/resolve.ts index e27a14ef4..58d47da7b 100644 --- a/packages/shared/src/cli/commands/plugin/add/resolve.ts +++ b/packages/shared/src/cli/commands/plugin/add/resolve.ts @@ -50,7 +50,7 @@ export async function resolveDerivedValues( /** * Columns for an AI Search index: the Delta Sync source table's columns minus - * embedding vectors, limited to `columnsToSync` when the index syncs a subset + * embedding vectors, limited to `columnsToSync` (or its alias `columnsToIndex`) when the index syncs a subset * (other source columns aren't in the index, and querying them fails). * Production refuses to start without `columns`, so they are written into the * plugin config at add time. Throws when the index has no source table. @@ -72,9 +72,11 @@ export async function discoverIndexColumns( } const excluded = new Set(spec.embeddingVectorColumns?.map((c) => c.name)); const table = await client.tables.getTable({ fullNameArg: spec.sourceTable }); - const synced = spec.columnsToSync?.length - ? new Set(spec.columnsToSync) - : undefined; + // columnsToIndex is the response's alias of columnsToSync. + const subset = spec.columnsToSync?.length + ? spec.columnsToSync + : spec.columnsToIndex; + const synced = subset?.length ? new Set(subset) : undefined; return (table.columns ?? []).flatMap((c) => c.name && !excluded.has(c.name) && (!synced || synced.has(c.name)) ? [c.name] From 151c6df396880be093b95bf17fafc7e174c5687d Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 27 Oct 2026 18:31:07 +0100 Subject: [PATCH 09/16] fix(shared): plugin registration extends an existing import from the same module registerPluginInServer added a second import line (e.g. another import { analytics } from "@databricks/appkit") when the server already imported from that module. It now appends the name to the existing value import (single- or multi-line), stays idempotent, and still adds a new line when the only import from the module is type-only. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../commands/registry/server-register.test.ts | 60 +++++++++++++++++-- .../cli/commands/registry/server-register.ts | 34 ++++++++++- 2 files changed, 88 insertions(+), 6 deletions(-) diff --git a/packages/shared/src/cli/commands/registry/server-register.test.ts b/packages/shared/src/cli/commands/registry/server-register.test.ts index 34d089a22..ddffd3bb9 100644 --- a/packages/shared/src/cli/commands/registry/server-register.test.ts +++ b/packages/shared/src/cli/commands/registry/server-register.test.ts @@ -38,6 +38,55 @@ afterEach(() => { }); describe("registerPluginInServer", () => { + it("extends an existing import from the same module, idempotently", () => { + const dir = makeTempDir(); + const file = writeServer(dir, "server.ts", "server()"); + registerPluginInServer(dir, "@databricks/appkit", "analytics"); + registerPluginInServer(dir, "@databricks/appkit", "analytics"); + const out = fs.readFileSync(file, "utf-8"); + expect(out).toContain( + `import { createApp, analytics } from "@databricks/appkit";`, + ); + expect(out.match(/@databricks\/appkit"/g)).toHaveLength(1); + expect(out.match(/analytics\(\)/g)).toHaveLength(1); + }); + + it("extends the /beta import and keeps a multi-line list's layout", () => { + const dir = makeTempDir(); + const file = path.join(dir, "server.ts"); + fs.writeFileSync( + file, + `import { createApp, server } from "@databricks/appkit";\n` + + `import {\n agents,\n} from "@databricks/appkit/beta";\n\n` + + `createApp({ plugins: [server(), agents()] });\n`, + ); + registerPluginInServer(dir, "@databricks/appkit/beta", "aiSearch"); + const out = fs.readFileSync(file, "utf-8"); + expect(out).toContain( + `import {\n agents,\n aiSearch,\n} from "@databricks/appkit/beta";`, + ); + expect(out).toContain( + `import { createApp, server } from "@databricks/appkit";`, + ); + }); + + it("adds a new line rather than extending a type-only import", () => { + const dir = makeTempDir(); + const file = path.join(dir, "server.ts"); + fs.writeFileSync( + file, + `import type { PluginData } from "@databricks/appkit/beta";\n` + + `import { createApp } from "@databricks/appkit";\n\n` + + `createApp({ plugins: [] });\n`, + ); + registerPluginInServer(dir, "@databricks/appkit/beta", "agents"); + const out = fs.readFileSync(file, "utf-8"); + expect(out).toContain( + `import type { PluginData } from "@databricks/appkit/beta";`, + ); + expect(out).toContain(`import { agents } from "@databricks/appkit/beta";`); + }); + it("accepts an external npm package and rejects code in the path", () => { const dir = makeTempDir(); writeServer(dir, "server.ts", "server()"); @@ -61,7 +110,9 @@ describe("registerPluginInServer", () => { expect(result.status).toBe("wired"); const out = fs.readFileSync(file, "utf-8"); expect(out).toContain(`files({ auth: "on-behalf-of-user" }), server()`); - expect(out).toContain(`import { files } from "@databricks/appkit";`); + expect(out).toContain( + `import { createApp, files } from "@databricks/appkit";`, + ); }); it("wires a plugin into a server root that is a subdir (e.g. api/)", () => { @@ -123,10 +174,11 @@ describe("registerPluginInServer", () => { expect(result.status).toBe("wired"); const written = fs.readFileSync(file, "utf-8"); - expect(written).toContain('import { hello } from "./plugins/hello";'); + // `hello` is bound by extending the same-path import; HelloPlugin stays. + expect(written).toContain( + 'import { HelloPlugin, hello } from "./plugins/hello";', + ); expect(written).toContain("hello()"); - // The pre-existing, differently-named import is left intact. - expect(written).toContain('import { HelloPlugin } from "./plugins/hello";'); }); it("does not add a conflicting import when the binding already exists", () => { diff --git a/packages/shared/src/cli/commands/registry/server-register.ts b/packages/shared/src/cli/commands/registry/server-register.ts index 26b10eb33..cfeb4b543 100644 --- a/packages/shared/src/cli/commands/registry/server-register.ts +++ b/packages/shared/src/cli/commands/registry/server-register.ts @@ -28,6 +28,21 @@ export function findServerFile(serverRoot: string): string | null { return null; } +/** + * The named-imports node (`{ a, b }`) of a value import from `module`, to + * extend with a new name instead of adding a second import line. Type-only + * imports are skipped: a value added there would be erased at compile time. + */ +function namedImportsFrom(stmts: SgNode[], module: string): SgNode | null { + for (const stmt of stmts) { + const source = stmt.field("source")?.text().slice(1, -1); + if (source !== module || /^import\s+type\b/.test(stmt.text())) continue; + const named = stmt.find({ rule: { kind: "named_imports" } }); + if (named) return named; + } + return null; +} + /** The `plugins: [...]` array node inside a createApp call, if present. */ function findPluginsArray(root: SgNode): SgNode | null { for (const pair of root.findAll({ rule: { kind: "pair" } })) { @@ -159,13 +174,28 @@ export function registerPluginInServer( importBindingNames(s).has(exportName), ); const importLine = `import { ${exportName} } from "${importPath}";`; - if (!hasBinding && importStmts.length > 0) { + const named = hasBinding ? null : namedImportsFrom(importStmts, importPath); + if (named) { + // Append after the last specifier, matching a multi-line list's layout. + const specs = named.findAll({ rule: { kind: "import_specifier" } }); + const lastSpec = specs[specs.length - 1]; + if (!lastSpec) { + edits.push(named.replace(`{ ${exportName} }`)); + } else { + const startIdx = lastSpec.range().start.index; + const lineStart = content.lastIndexOf("\n", startIdx - 1); + const indent = content.slice(lineStart + 1, startIdx); + const multiline = named.text().includes("\n") && /^[ \t]*$/.test(indent); + const sep = multiline ? `,\n${indent}` : ", "; + edits.push(lastSpec.replace(`${lastSpec.text()}${sep}${exportName}`)); + } + } else if (!hasBinding && importStmts.length > 0) { const last = importStmts[importStmts.length - 1]; edits.push(last.replace(`${last.text()}\n${importLine}`)); } let output = root.commitEdits(edits); - if (!hasBinding && importStmts.length === 0) { + if (!hasBinding && !named && importStmts.length === 0) { output = `${importLine}\n${output}`; } fs.writeFileSync(serverFile, output); From 17d641e5c2940f461b75faa24b582c68f3dcab1e Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 27 Oct 2026 18:42:02 +0100 Subject: [PATCH 10/16] fix(shared): plugin add type-only import detection, agents model on shared env - type-only imports are detected from the syntax tree, so a comment between import and type no longer gets a value added to an erased import - when DATABRICKS_SERVING_ENDPOINT_NAME is already bound (e.g. by serving), agents gets the requested endpoint as defaultModel instead of a value that never reaches the app; any other env name bound to a different resource or value fails before anything is written Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../src/cli/commands/plugin/add/add.test.ts | 30 ++++++- .../shared/src/cli/commands/plugin/add/add.ts | 81 ++++++++++++++++--- .../commands/registry/server-register.test.ts | 19 +++++ .../cli/commands/registry/server-register.ts | 6 +- 4 files changed, 123 insertions(+), 13 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/add.test.ts b/packages/shared/src/cli/commands/plugin/add/add.test.ts index 59d27286f..d3dbc8ba8 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.test.ts @@ -1,6 +1,10 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + import { expect, test } from "vitest"; -import { pluginConfig, toSource } from "./add"; +import { envConflicts, pluginConfig, toSource } from "./add"; import type { ManifestPlugin, ManifestResource } from "./plan"; const index: ManifestResource = { @@ -89,3 +93,27 @@ test("agents auth follows the model endpoint, not the skills volume", () => { }), ).toBeUndefined(); }); + +test("envConflicts flags env names bound to another resource or value", () => { + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), "plugin-add-")); + fs.writeFileSync(path.join(cwd, ".env"), "A=old\nB=\n"); + fs.writeFileSync( + path.join(cwd, "app.yaml"), + "env:\n - name: C\n valueFrom: serving-endpoint\n - name: D\n valueFrom: same\n", + ); + const plan = { + dotEnv: [ + { env: "A", value: "new" }, + { env: "B", value: "x" }, + ], + appYamlEnv: [ + { name: "C", valueFrom: "agents-serving-endpoint" }, + { name: "D", valueFrom: "same" }, + ], + bundleVariables: [], + resourceBindings: [], + unverifiedTypes: [], + }; + expect(envConflicts(cwd, plan)).toEqual(["A", "C"]); + fs.rmSync(cwd, { recursive: true, force: true }); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index a7b44b302..b31037c5d 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -4,7 +4,9 @@ import process from "node:process"; import { Command } from "commander"; import pc from "picocolors"; +import { parseDocument } from "yaml"; +import { APP_YAML_FILE } from "../../../deploy-config"; import { profileFromEnv } from "../../registry/add"; import { planHasContent, @@ -12,12 +14,13 @@ import { validateBundle, writeConfig, } from "../../registry/config-writer"; -import { serializeEnvAppend } from "../../registry/env-reconcile"; +import { parseEnv, serializeEnvAppend } from "../../registry/env-reconcile"; import { AUTH_MODE_BOTH, AUTH_MODE_FIELD, AUTH_MODE_OBO, AUTH_MODE_SP, + type AddPlan, type AuthMode, authKey, buildAddPlan, @@ -227,6 +230,43 @@ async function aiSearchColumns( .filter(Boolean); } +/** The env var agents falls back to for its model; serving binds it too. */ +const AGENTS_MODEL_ENV = "DATABRICKS_SERVING_ENDPOINT_NAME"; + +/** + * Env names in the plan already bound to something else: a different + * non-empty value in .env, or an app.yaml entry with another valueFrom/value. + */ +export function envConflicts(cwd: string, plan: AddPlan): string[] { + const dotEnv = fs.existsSync(path.join(cwd, ".env")) + ? parseEnv(fs.readFileSync(path.join(cwd, ".env"), "utf-8")) + : {}; + const appYamlFile = path.join(cwd, APP_YAML_FILE); + const appEnv = new Map(); + if (fs.existsSync(appYamlFile)) { + const env = parseDocument(fs.readFileSync(appYamlFile, "utf-8")).toJSON() + ?.env as + | { name?: string; valueFrom?: string; value?: string }[] + | undefined; + for (const e of env ?? []) { + if (e?.name) appEnv.set(e.name, JSON.stringify([e.valueFrom, e.value])); + } + } + const out = new Set(); + for (const e of plan.dotEnv) { + if (dotEnv[e.env] && e.value && dotEnv[e.env] !== e.value) out.add(e.env); + } + for (const e of plan.appYamlEnv) { + const want = JSON.stringify([ + "valueFrom" in e ? e.valueFrom : undefined, + "value" in e ? e.value : undefined, + ]); + const have = appEnv.get(e.name); + if (have !== undefined && have !== want) out.add(e.name); + } + return [...out]; +} + async function runPluginAdd(name: string, opts: AddOptions): Promise { const cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd(); if ( @@ -291,7 +331,7 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { await fillMissing(missingNow(), values, interactive); resources = selectResources(plugin, values); - const config = pluginConfig( + let config = pluginConfig( plugin, resources, modes, @@ -302,6 +342,34 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { const plan = buildAddPlan(plugin, resources, values, modes); + // An env var holds one value: if another resource already binds one of + // these names to something else, the new value would never reach the app. + const conflicts = envConflicts(cwd, plan); + const model = values["agents-serving-endpoint.name"]; + if ( + plugin.name === "agents" && + model && + conflicts.includes(AGENTS_MODEL_ENV) + ) { + // agents takes its model from defaultModel before that env var. + config = { ...config, defaultModel: model }; + plan.dotEnv = plan.dotEnv.filter((e) => e.env !== AGENTS_MODEL_ENV); + plan.appYamlEnv = plan.appYamlEnv.filter( + (e) => e.name !== AGENTS_MODEL_ENV, + ); + conflicts.splice(conflicts.indexOf(AGENTS_MODEL_ENV), 1); + console.log( + pc.dim( + `${AGENTS_MODEL_ENV} is already used by another resource; agents gets its model as defaultModel instead.`, + ), + ); + } + if (conflicts.length > 0) { + throw new Error( + `${conflicts.join(", ")} already set to a different resource or value in .env or app.yaml; nothing was written. Edit those entries by hand, then rerun.`, + ); + } + if (opts.register !== false) { await registerInServer(cwd, plugin, config ? toSource(config) : ""); } @@ -332,15 +400,6 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { if (updated.length > 0) { console.log(`${pc.green("Updated .env:")} ${updated.join(", ")}`); } - for (const e of dotEnv) { - if (existing[e.env] && e.value && existing[e.env] !== e.value) { - console.warn( - pc.yellow( - `.env already sets ${e.env}; kept it (the app.yaml and databricks.yml entries use the new value).`, - ), - ); - } - } if (planHasContent(plan)) { const result = writeConfig(cwd, plan); diff --git a/packages/shared/src/cli/commands/registry/server-register.test.ts b/packages/shared/src/cli/commands/registry/server-register.test.ts index ddffd3bb9..e270c424d 100644 --- a/packages/shared/src/cli/commands/registry/server-register.test.ts +++ b/packages/shared/src/cli/commands/registry/server-register.test.ts @@ -70,6 +70,25 @@ describe("registerPluginInServer", () => { ); }); + it("treats a commented `import /* x */ type` as type-only", () => { + const dir = makeTempDir(); + const file = path.join(dir, "server.ts"); + fs.writeFileSync( + file, + `import /* types used below */ type { Plugin } from "@databricks/appkit";\n` + + `import { createApp } from "@databricks/appkit";\n\n` + + `createApp({ plugins: [] });\n`, + ); + registerPluginInServer(dir, "@databricks/appkit", "analytics"); + const out = fs.readFileSync(file, "utf-8"); + expect(out).toContain( + `import /* types used below */ type { Plugin } from "@databricks/appkit";`, + ); + expect(out).toContain( + `import { createApp, analytics } from "@databricks/appkit";`, + ); + }); + it("adds a new line rather than extending a type-only import", () => { const dir = makeTempDir(); const file = path.join(dir, "server.ts"); diff --git a/packages/shared/src/cli/commands/registry/server-register.ts b/packages/shared/src/cli/commands/registry/server-register.ts index cfeb4b543..10d473fe8 100644 --- a/packages/shared/src/cli/commands/registry/server-register.ts +++ b/packages/shared/src/cli/commands/registry/server-register.ts @@ -36,7 +36,11 @@ export function findServerFile(serverRoot: string): string | null { function namedImportsFrom(stmts: SgNode[], module: string): SgNode | null { for (const stmt of stmts) { const source = stmt.field("source")?.text().slice(1, -1); - if (source !== module || /^import\s+type\b/.test(stmt.text())) continue; + // `import type` shows as an unnamed `type` token, wherever comments sit. + const typeOnly = stmt + .children() + .some((c) => c.kind() === "type" && !c.isNamed()); + if (source !== module || typeOnly) continue; const named = stmt.find({ rule: { kind: "named_imports" } }); if (named) return named; } From 836f89ded99b088463be42b30f17322a2524aad5 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 27 Oct 2026 18:52:40 +0100 Subject: [PATCH 11/16] fix(shared): plugin add prints required config with --no-register, rejects multi-app bundles first - with --no-register, a required plugin config (auth, columns, agents defaultModel) is printed as the createApp entry to add by hand - a databricks.yml with several apps and none named app is rejected before any file is written (plugin add and the registry add command) Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../shared/src/cli/commands/plugin/add/add.ts | 16 ++++++++++++++-- .../shared/src/cli/commands/registry/add.ts | 3 +++ .../commands/registry/config-writer.test.ts | 18 +++++++++++++++++- .../src/cli/commands/registry/config-writer.ts | 5 +++++ 4 files changed, 39 insertions(+), 3 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index b31037c5d..8e900a160 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -9,6 +9,7 @@ import { parseDocument } from "yaml"; import { APP_YAML_FILE } from "../../../deploy-config"; import { profileFromEnv } from "../../registry/add"; import { + assertBundleApp, planHasContent, reportConfigWrite, validateBundle, @@ -360,7 +361,7 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { conflicts.splice(conflicts.indexOf(AGENTS_MODEL_ENV), 1); console.log( pc.dim( - `${AGENTS_MODEL_ENV} is already used by another resource; agents gets its model as defaultModel instead.`, + `${AGENTS_MODEL_ENV} is already used by another resource; agents gets the endpoint as defaultModel in its registration instead.`, ), ); } @@ -370,7 +371,18 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { ); } - if (opts.register !== false) { + // Fail before any write rather than leave a partial install. + assertBundleApp(cwd); + + if (opts.register === false) { + if (config) { + // The config carries required settings (auth mode, columns, model). + console.log( + `${pc.bold("Register it with this config in your createApp call:")}\n` + + pc.dim(` ${plugin.name}(${toSource(config)})`), + ); + } + } else { await registerInServer(cwd, plugin, config ? toSource(config) : ""); } diff --git a/packages/shared/src/cli/commands/registry/add.ts b/packages/shared/src/cli/commands/registry/add.ts index 086239108..78191878a 100644 --- a/packages/shared/src/cli/commands/registry/add.ts +++ b/packages/shared/src/cli/commands/registry/add.ts @@ -22,6 +22,7 @@ import { planHasContent, } from "./config-plan"; import { + assertBundleApp, reportConfigWrite, validateBundle, writeConfig, @@ -449,6 +450,8 @@ export function partitionVerified( async function runAdd(refs: string[], opts: AddOptions): Promise { const cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd(); + // The deploy-config write refuses an ambiguous bundle; check before any file. + if (opts.resources !== false) assertBundleApp(cwd); // Resolve the full graph and fetch the verified index concurrently (two // independent round-trips). Item resolution is read-only — nothing is written diff --git a/packages/shared/src/cli/commands/registry/config-writer.test.ts b/packages/shared/src/cli/commands/registry/config-writer.test.ts index 51a13cb7c..82cb37f7e 100644 --- a/packages/shared/src/cli/commands/registry/config-writer.test.ts +++ b/packages/shared/src/cli/commands/registry/config-writer.test.ts @@ -6,7 +6,7 @@ import { afterEach, describe, expect, it } from "vitest"; import { parseDocument } from "yaml"; import { buildConfigPlan } from "./config-plan"; -import { writeConfig } from "./config-writer"; +import { assertBundleApp, writeConfig } from "./config-writer"; import type { ResourceRequirementRow } from "./requirements"; const FIXTURES = path.join(__dirname, "__fixtures__"); @@ -254,3 +254,19 @@ describe("writeConfig — app key", () => { expect(apps.dashboard.resources[0].name).toBe("wh"); }); }); + +describe("assertBundleApp", () => { + it("throws for several apps without one named app, before any write", () => { + const cwd = makeTempDir(); + fs.writeFileSync( + path.join(cwd, "databricks.yml"), + "resources:\n apps:\n frontend: {}\n backend: {}\n", + ); + expect(() => assertBundleApp(cwd)).toThrow("several apps"); + fs.writeFileSync( + path.join(cwd, "databricks.yml"), + "resources:\n apps:\n dash: {}\n", + ); + expect(() => assertBundleApp(cwd)).not.toThrow(); + }); +}); diff --git a/packages/shared/src/cli/commands/registry/config-writer.ts b/packages/shared/src/cli/commands/registry/config-writer.ts index d2cd1e9da..78e94ce27 100644 --- a/packages/shared/src/cli/commands/registry/config-writer.ts +++ b/packages/shared/src/cli/commands/registry/config-writer.ts @@ -93,6 +93,11 @@ function appKey(doc: ReturnType): string { ); } +/** Throws (see {@link appKey}) when databricks.yml has no single target app. */ +export function assertBundleApp(cwd: string): void { + appKey(loadDoc(path.join(cwd, DATABRICKS_YML_FILE))); +} + /** * Additively patches `databricks.yml`: adds bundle `variables`, the app * `resources` bindings, and the target-level variable values — each only if From e4d5c20d188592dad13eb420a716bf89c90c29bf Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Wed, 28 Oct 2026 10:32:32 +0100 Subject: [PATCH 12/16] feat(shared): appkit plugin add takes several plugins or a data plan in one run `appkit plugin add a b ...` and `--from-plan .appkit/data-plan.json` (v1: plugins[].name/authMode/set) plan every plugin first, then write server.ts, .env, app.yaml and databricks.yml once each, so a watching dev server restarts once. Nothing is written if any plugin fails; env clashes between plugins of the same run are caught like clashes with existing files; a rerun with the same input writes nothing. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../shared/src/cli/commands/plugin/add/add.ts | 370 +++++++++++++----- .../src/cli/commands/plugin/add/run.test.ts | 163 ++++++++ .../src/cli/commands/registry/env-writer.ts | 2 +- .../commands/registry/server-register.test.ts | 46 ++- .../cli/commands/registry/server-register.ts | 63 ++- 5 files changed, 535 insertions(+), 109 deletions(-) create mode 100644 packages/shared/src/cli/commands/plugin/add/run.test.ts diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index 8e900a160..ec57ea0af 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -5,9 +5,11 @@ import process from "node:process"; import { Command } from "commander"; import pc from "picocolors"; import { parseDocument } from "yaml"; +import { z } from "zod"; import { APP_YAML_FILE } from "../../../deploy-config"; import { profileFromEnv } from "../../registry/add"; +import type { ConfigPlan } from "../../registry/config-plan"; import { assertBundleApp, planHasContent, @@ -43,6 +45,7 @@ const SERVER_SUBDIRS = ["server", "api", "backend"]; interface AddOptions { set: string[]; + fromPlan?: string; authMode?: string; cwd?: string; yes?: boolean; @@ -238,7 +241,12 @@ const AGENTS_MODEL_ENV = "DATABRICKS_SERVING_ENDPOINT_NAME"; * Env names in the plan already bound to something else: a different * non-empty value in .env, or an app.yaml entry with another valueFrom/value. */ -export function envConflicts(cwd: string, plan: AddPlan): string[] { +export function envConflicts( + cwd: string, + plan: AddPlan, + /** Plans of earlier plugins in the same run, not yet written. */ + pending: AddPlan[] = [], +): string[] { const dotEnv = fs.existsSync(path.join(cwd, ".env")) ? parseEnv(fs.readFileSync(path.join(cwd, ".env"), "utf-8")) : {}; @@ -253,6 +261,20 @@ export function envConflicts(cwd: string, plan: AddPlan): string[] { if (e?.name) appEnv.set(e.name, JSON.stringify([e.valueFrom, e.value])); } } + for (const p of pending) { + for (const e of p.dotEnv) if (!dotEnv[e.env]) dotEnv[e.env] = e.value; + for (const e of p.appYamlEnv) { + if (!appEnv.has(e.name)) { + appEnv.set( + e.name, + JSON.stringify([ + "valueFrom" in e ? e.valueFrom : undefined, + "value" in e ? e.value : undefined, + ]), + ); + } + } + } const out = new Set(); for (const e of plan.dotEnv) { if (dotEnv[e.env] && e.value && dotEnv[e.env] !== e.value) out.add(e.env); @@ -268,27 +290,49 @@ export function envConflicts(cwd: string, plan: AddPlan): string[] { return [...out]; } -async function runPluginAdd(name: string, opts: AddOptions): Promise { - const cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd(); +/** One plugin of a run: its name, its --set pairs and its default auth mode. */ +export interface PluginRequest { + name: string; + set: string[]; + authMode?: string; +} + +interface PreparedPlugin { + plugin: ManifestPlugin; + plan: AddPlan; + config?: Record; +} + +/** + * Everything for one plugin short of writing: prompts, derived values, the + * file plan and its registration config. Throws on conflicts with the app's + * files or with earlier plugins of the run (`pending`). + */ +async function preparePlugin( + cwd: string, + req: PluginRequest, + opts: AddOptions, + pending: AddPlan[], +): Promise { if ( - opts.authMode !== undefined && - opts.authMode !== AUTH_MODE_SP && - opts.authMode !== AUTH_MODE_OBO + req.authMode !== undefined && + req.authMode !== AUTH_MODE_SP && + req.authMode !== AUTH_MODE_OBO ) { throw new Error( - `invalid --auth-mode value: "${opts.authMode}" (must be obo or sp; use --set ..${AUTH_MODE_FIELD}=both for a single resource)`, + `invalid --auth-mode value: "${req.authMode}" (must be obo or sp; use --set ..${AUTH_MODE_FIELD}=both for a single resource)`, ); } - const plugin = loadPlugin(cwd, name); - const values = parseSetValues(opts.set, plugin); - const setModes = parseSetAuthModes(opts.set, plugin); + const plugin = loadPlugin(cwd, req.name); + const values = parseSetValues(req.set, plugin); + const setModes = parseSetAuthModes(req.set, plugin); const interactive = !opts.yes && Boolean(process.stdin.isTTY); let resources = selectResources(plugin, values); if (interactive) resources.push(...(await promptOptional(plugin, values))); const promptedModes = - interactive && opts.authMode === undefined + interactive && req.authMode === undefined ? await promptAuthModes( resources.filter((r) => !(authKey(r) in setModes)), ) @@ -297,7 +341,7 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { resources, setModes, promptedModes, - opts.authMode, + req.authMode, ); if (keptSP.length > 0) { console.log( @@ -345,7 +389,7 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { // An env var holds one value: if another resource already binds one of // these names to something else, the new value would never reach the app. - const conflicts = envConflicts(cwd, plan); + const conflicts = envConflicts(cwd, plan, pending); const model = values["agents-serving-endpoint.name"]; if ( plugin.name === "agents" && @@ -371,11 +415,106 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { ); } + return { plugin, plan, config }; +} + +/** Requests from `plugin add a b --set ...`: each plugin gets its own --set pairs. */ +export function requestsFromArgs( + names: string[], + set: string[], + authMode: string | undefined, +): PluginRequest[] { + for (const sv of set) { + const prefix = sv.split(".")[0]; + if (!names.includes(prefix)) { + throw new Error( + `--set "${sv}" is for "${prefix}", which this run doesn't add (${names.join(", ")})`, + ); + } + } + return names.map((name) => ({ + name, + set: set.filter((sv) => sv.split(".")[0] === name), + authMode, + })); +} + +const dataPlanSchema = z.object({ + version: z.literal(1), + plugins: z + .array( + z.object({ + name: z.string().min(1), + authMode: z.enum([AUTH_MODE_OBO, AUTH_MODE_SP]).optional(), + /** `resourceKey.field` (or `resourceKey.authMode`) → value. */ + set: z.record(z.string(), z.string()).optional(), + }), + ) + .min(1), +}); + +/** Requests from a data plan file (`.appkit/data-plan.json`, version 1). */ +export function requestsFromPlan(file: string): PluginRequest[] { + const parsed = dataPlanSchema.safeParse( + JSON.parse(fs.readFileSync(file, "utf-8")), + ); + if (!parsed.success) { + throw new Error( + `invalid data plan ${file}: ${parsed.error.issues.map((i) => `${i.path.join(".")}: ${i.message}`).join("; ")}`, + ); + } + return parsed.data.plugins.map((p) => ({ + name: p.name, + authMode: p.authMode, + set: Object.entries(p.set ?? {}).map(([k, v]) => `${p.name}.${k}=${v}`), + })); +} + +/** + * Adds every requested plugin in one pass: all are planned first (nothing is + * written if any fails), then each file is written at most once, so a dev + * server watching them restarts once. + */ +async function runPluginAdd(names: string[], opts: AddOptions): Promise { + const cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd(); + if ( + opts.fromPlan && + (names.length > 0 || opts.set.length > 0 || opts.authMode) + ) { + throw new Error( + "--from-plan can't be combined with plugin names, --set or --auth-mode", + ); + } + if (!opts.fromPlan && names.length === 0) { + throw new Error("name at least one plugin, or pass --from-plan "); + } + const requests = opts.fromPlan + ? requestsFromPlan(path.resolve(cwd, opts.fromPlan)) + : requestsFromArgs(names, opts.set, opts.authMode); + const seen = new Set(); + for (const r of requests) { + if (seen.has(r.name)) throw new Error(`plugin "${r.name}" is listed twice`); + seen.add(r.name); + } + + const prepared: PreparedPlugin[] = []; + for (const req of requests) { + prepared.push( + await preparePlugin( + cwd, + req, + opts, + prepared.map((p) => p.plan), + ), + ); + } + // Fail before any write rather than leave a partial install. assertBundleApp(cwd); if (opts.register === false) { - if (config) { + for (const { plugin, config } of prepared) { + if (!config) continue; // The config carries required settings (auth mode, columns, model). console.log( `${pc.bold("Register it with this config in your createApp call:")}\n` + @@ -383,43 +522,65 @@ async function runPluginAdd(name: string, opts: AddOptions): Promise { ); } } else { - await registerInServer(cwd, plugin, config ? toSource(config) : ""); + await registerInServer(cwd, prepared); } - const { appendToFile, readEnvFile } = - await import("../../registry/env-writer.js"); + const plans = prepared.map((p) => p.plan); + await writeDotEnv( + cwd, + plans.flatMap((p) => p.dotEnv), + ); + + const merged: ConfigPlan = { + appYamlEnv: plans.flatMap((p) => p.appYamlEnv), + bundleVariables: plans.flatMap((p) => p.bundleVariables), + resourceBindings: plans.flatMap((p) => p.resourceBindings), + userApiScopes: [...new Set(plans.flatMap((p) => p.userApiScopes ?? []))], + unverifiedTypes: [...new Set(plans.flatMap((p) => p.unverifiedTypes))], + }; + if (planHasContent(merged)) { + const result = writeConfig(cwd, merged); + reportConfigWrite(result); + if (result.databricksYmlChanged) { + validateBundle(cwd, opts.profile ?? profileFromEnv(cwd)); + } + } +} + +/** + * Adds the entries to .env in one write: empty placeholders (`KEY=`) are + * filled in place, new names appended, set values kept. + */ +async function writeDotEnv( + cwd: string, + entries: { env: string; value: string }[], +): Promise { + const { readEnvFile } = await import("../../registry/env-writer.js"); const envPath = path.join(cwd, ".env"); const existing = readEnvFile(envPath); - const dotEnv = plan.dotEnv.filter( - (e, i) => plan.dotEnv.findIndex((x) => x.env === e.env) === i, + const dotEnv = entries.filter( + (e, i) => entries.findIndex((x) => x.env === e.env) === i, ); - // Empty placeholders (`KEY=`) are filled in place; set values are kept. const filled = dotEnv.filter((e) => existing[e.env] === "" && e.value); - if (filled.length > 0) { - let content = fs.readFileSync(envPath, "utf-8"); - for (const e of filled) { - // e.env is a validated identifier, so it is safe in the pattern. - content = content.replace( - new RegExp(`^(\\s*(?:export\\s+)?${e.env}\\s*=).*$`, "m"), - (_, key: string) => key + e.value, - ); - } - fs.writeFileSync(envPath, content); - } const newEnv = dotEnv.filter((e) => !(e.env in existing)); - appendToFile(envPath, serializeEnvAppend(newEnv)); - const updated = [...filled, ...newEnv].map((e) => e.env); - if (updated.length > 0) { - console.log(`${pc.green("Updated .env:")} ${updated.join(", ")}`); + if (filled.length === 0 && newEnv.length === 0) return; + let content = fs.existsSync(envPath) ? fs.readFileSync(envPath, "utf-8") : ""; + for (const e of filled) { + // e.env is a validated identifier, so it is safe in the pattern. + content = content.replace( + new RegExp(`^(\\s*(?:export\\s+)?${e.env}\\s*=).*$`, "m"), + (_, key: string) => key + e.value, + ); } - - if (planHasContent(plan)) { - const result = writeConfig(cwd, plan); - reportConfigWrite(result); - if (result.databricksYmlChanged) { - validateBundle(cwd, profile); - } + const append = serializeEnvAppend(newEnv); + if (append) { + if (content.length > 0 && !content.endsWith("\n")) content += "\n"; + content += append; } + fs.writeFileSync(envPath, content); + console.log( + `${pc.green("Updated .env:")} ${[...filled, ...newEnv].map((e) => e.env).join(", ")}`, + ); } /** Plugins that default to the service principal unless told otherwise. */ @@ -495,68 +656,77 @@ export function toSource(v: unknown): string { async function registerInServer( cwd: string, - plugin: ManifestPlugin, - callArgs: string, + prepared: PreparedPlugin[], ): Promise { const serverRoot = SERVER_SUBDIRS.map((s) => path.join(cwd, s)).find((p) => fs.existsSync(p), ) ?? cwd; // Lazy: server-register pulls in the @ast-grep/napi native addon. - const { findServerFile, registerPluginInServer } = + const { findServerFile, registerPluginsInServer } = await import("../../registry/server-register.js"); - // The /beta subpath is AppKit's; other packages export their plugins as is. - let importPath = - plugin.stability === "beta" && plugin.package === "@databricks/appkit" - ? `${plugin.package}/beta` - : plugin.package; - // Local plugins are recorded relative to the app root; imports resolve - // relative to the server file. - if (plugin.package.startsWith(".")) { - const from = path.dirname(findServerFile(serverRoot) ?? serverRoot); - const rel = path - .relative(from, path.resolve(cwd, plugin.package)) - .split(path.sep) - .join("/"); - importPath = rel.startsWith(".") ? rel : `./${rel}`; - } - const call = `${plugin.name}(${callArgs})`; - const result = registerPluginInServer( - serverRoot, - importPath, - plugin.name, - callArgs, - ); - const shown = - result.file && path.relative(cwd, path.join(serverRoot, result.file)); - if (result.status === "wired") { - console.log(`${pc.green("Registered")} ${call} in ${shown}`); - } else if (result.status === "already") { - console.log(pc.dim(`${plugin.name} is already registered in ${shown}`)); - if (callArgs) { - console.warn( - pc.yellow( - ` Make sure it is configured as ${call} for the chosen auth mode.`, - ), + const serverFile = findServerFile(serverRoot); + const entries = prepared.map(({ plugin, config }) => { + // The /beta subpath is AppKit's; other packages export their plugins as is. + let importPath = + plugin.stability === "beta" && plugin.package === "@databricks/appkit" + ? `${plugin.package}/beta` + : plugin.package; + // Local plugins are recorded relative to the app root; imports resolve + // relative to the server file. + if (plugin.package.startsWith(".")) { + const from = path.dirname(serverFile ?? serverRoot); + const rel = path + .relative(from, path.resolve(cwd, plugin.package)) + .split(path.sep) + .join("/"); + importPath = rel.startsWith(".") ? rel : `./${rel}`; + } + return { + importPath, + exportName: plugin.name, + callArgs: config ? toSource(config) : "", + }; + }); + const results = registerPluginsInServer(serverRoot, entries); + results.forEach((result, i) => { + const { importPath, exportName, callArgs } = entries[i]; + const call = `${exportName}(${callArgs})`; + const shown = + result.file && path.relative(cwd, path.join(serverRoot, result.file)); + if (result.status === "wired") { + console.log(`${pc.green("Registered")} ${call} in ${shown}`); + } else if (result.status === "already") { + console.log(pc.dim(`${exportName} is already registered in ${shown}`)); + if (callArgs) { + console.warn( + pc.yellow( + ` Make sure it is configured as ${call} for the chosen auth mode.`, + ), + ); + } + } else { + console.log( + pc.yellow(`Couldn't auto-register ${exportName}: ${result.reason}.`) + + "\nAdd it to your server's createApp call:\n" + + pc.dim( + ` import { ${exportName} } from "${importPath}";\n` + + ` createApp({ plugins: [${call}, /* ... */] });`, + ), ); } - } else { - console.log( - pc.yellow(`Couldn't auto-register ${plugin.name}: ${result.reason}.`) + - "\nAdd it to your server's createApp call:\n" + - pc.dim( - ` import { ${plugin.name} } from "${importPath}";\n` + - ` createApp({ plugins: [${call}, /* ... */] });`, - ), - ); - } + }); } export const pluginAddCommand = new Command("add") .description( - "Add a plugin from appkit.plugins.json to the app: server registration, .env, app.yaml and databricks.yml", + "Add plugins from appkit.plugins.json to the app: server registration, .env, app.yaml and databricks.yml, each written once", + ) + .argument("[plugins...]", "Plugin names, as in appkit.plugins.json") + .option( + "--from-plan ", + "Add the plugins listed in a data plan file (e.g. .appkit/data-plan.json, version 1)", ) - .argument("", "Plugin name, as in appkit.plugins.json") .option( "--set ", "Set resource values (format: plugin.resourceKey.field=value, can specify multiple)", @@ -585,15 +755,29 @@ Resource configuration (--set), as in \`databricks apps init\`: on behalf of the user (obo), as the app's service principal (sp), or both. obo/both add the resource's user_api_scopes; sp/both bind it with its permission. +Several plugins can be added in one run; each file is written once, so a dev +server watching them restarts once. Rerunning with the same input changes nothing. Existing entries in .env, app.yaml and databricks.yml are never overwritten. +Data plan file (--from-plan), version 1: + { "version": 1, + "plugins": [ + { "name": "analytics", "authMode": "obo", + "set": { "sql-warehouse.id": "abc123" } }, + { "name": "genie", "set": { "genie-space.id": "s1", "genie-space.name": "Sales" } } ] } + "set" keys are the --set keys without the plugin prefix ("authMode" works per + resource, e.g. "sql-warehouse.authMode": "both"); "authMode" is the plugin's default. + Examples: $ appkit plugin add analytics --set analytics.sql-warehouse.id=abc123 $ appkit plugin add analytics --auth-mode obo --set analytics.sql-warehouse.id=abc123 - $ appkit plugin add jobs --auth-mode obo --set jobs.job.authMode=sp --set jobs.job.id=42`, + $ appkit plugin add jobs --auth-mode obo --set jobs.job.authMode=sp --set jobs.job.id=42 + $ appkit plugin add analytics genie --set analytics.sql-warehouse.id=abc123 \\ + --set genie.genie-space.id=s1 --set genie.genie-space.name=Sales + $ appkit plugin add --from-plan .appkit/data-plan.json --yes`, ) - .action((name: string, opts: AddOptions) => - runPluginAdd(name, opts).catch((err: Error) => { + .action((names: string[], opts: AddOptions) => + runPluginAdd(names, opts).catch((err: Error) => { console.error(pc.red(err.message)); process.exit(1); }), diff --git a/packages/shared/src/cli/commands/plugin/add/run.test.ts b/packages/shared/src/cli/commands/plugin/add/run.test.ts new file mode 100644 index 000000000..85f5ee9a1 --- /dev/null +++ b/packages/shared/src/cli/commands/plugin/add/run.test.ts @@ -0,0 +1,163 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { afterEach, describe, expect, test, vi } from "vitest"; + +import { pluginAddCommand, requestsFromArgs, requestsFromPlan } from "./add"; + +// bundle validate would shell out to the Databricks CLI. +vi.mock("../../registry/config-writer", async (importOriginal) => ({ + ...(await importOriginal()), + validateBundle: () => true, +})); + +const MANIFEST = path.resolve( + __dirname, + "../../../../../../../template/appkit.plugins.json", +); +const dirs: string[] = []; + +function makeApp(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "plugin-add-run-")); + dirs.push(dir); + fs.copyFileSync(MANIFEST, path.join(dir, "appkit.plugins.json")); + fs.mkdirSync(path.join(dir, "server")); + fs.writeFileSync( + path.join(dir, "server", "server.ts"), + `import { createApp, server } from "@databricks/appkit";\n\ncreateApp({ plugins: [server()] });\n`, + ); + return dir; +} + +afterEach(() => { + for (const d of dirs) fs.rmSync(d, { recursive: true, force: true }); + dirs.length = 0; + vi.restoreAllMocks(); +}); + +describe("requestsFromArgs", () => { + test("gives each plugin its own --set pairs", () => { + expect( + requestsFromArgs( + ["analytics", "genie"], + ["analytics.sql-warehouse.id=wh", "genie.genie-space.id=s"], + "obo", + ), + ).toEqual([ + { + name: "analytics", + set: ["analytics.sql-warehouse.id=wh"], + authMode: "obo", + }, + { name: "genie", set: ["genie.genie-space.id=s"], authMode: "obo" }, + ]); + }); + + test("rejects --set for a plugin the run doesn't add", () => { + expect(() => + requestsFromArgs(["analytics"], ["genie.genie-space.id=s"], undefined), + ).toThrow("doesn't add"); + }); +}); + +describe("requestsFromPlan", () => { + test("maps plan entries to prefixed --set pairs", () => { + const dir = makeApp(); + const file = path.join(dir, "plan.json"); + fs.writeFileSync( + file, + JSON.stringify({ + version: 1, + plugins: [ + { + name: "analytics", + authMode: "sp", + set: { "sql-warehouse.id": "wh" }, + }, + ], + }), + ); + expect(requestsFromPlan(file)).toEqual([ + { + name: "analytics", + authMode: "sp", + set: ["analytics.sql-warehouse.id=wh"], + }, + ]); + }); + + test("rejects an unknown version or auth mode", () => { + const dir = makeApp(); + const file = path.join(dir, "plan.json"); + fs.writeFileSync( + file, + JSON.stringify({ + version: 2, + plugins: [{ name: "a", authMode: "both" }], + }), + ); + expect(() => requestsFromPlan(file)).toThrow( + /invalid data plan.*version.*authMode/s, + ); + }); +}); + +test("adds several plugins writing each file once, and nothing on a rerun", async () => { + const dir = makeApp(); + fs.mkdirSync(path.join(dir, ".appkit")); + fs.writeFileSync( + path.join(dir, ".appkit", "data-plan.json"), + JSON.stringify({ + version: 1, + plugins: [ + { + name: "analytics", + authMode: "obo", + set: { "sql-warehouse.id": "wh-1" }, + }, + { + name: "genie", + set: { "genie-space.id": "space-1", "genie-space.name": "Sales" }, + }, + ], + }), + ); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "warn").mockImplementation(() => {}); + const run = () => + pluginAddCommand.parseAsync( + ["--from-plan", ".appkit/data-plan.json", "--yes", "-C", dir], + { from: "user" }, + ); + + const writes = vi.spyOn(fs, "writeFileSync"); + await run(); + const written = writes.mock.calls.map((c) => + path.relative(dir, String(c[0])), + ); + expect(written.sort()).toEqual([ + ".env", + "app.yaml", + "databricks.yml", + "server/server.ts", + ]); + + const server = fs.readFileSync( + path.join(dir, "server", "server.ts"), + "utf-8", + ); + expect(server).toContain( + `import { createApp, server, analytics, genie } from "@databricks/appkit";`, + ); + expect(fs.readFileSync(path.join(dir, ".env"), "utf-8")).toContain( + "DATABRICKS_WAREHOUSE_ID=wh-1", + ); + const yml = fs.readFileSync(path.join(dir, "databricks.yml"), "utf-8"); + expect(yml).toContain("- sql"); + expect(yml).toContain("- genie"); + + writes.mockClear(); + await run(); + expect(writes).not.toHaveBeenCalled(); +}); diff --git a/packages/shared/src/cli/commands/registry/env-writer.ts b/packages/shared/src/cli/commands/registry/env-writer.ts index 7f136a578..13094a703 100644 --- a/packages/shared/src/cli/commands/registry/env-writer.ts +++ b/packages/shared/src/cli/commands/registry/env-writer.ts @@ -52,7 +52,7 @@ export function readEnvFile(file: string): Record { } /** Appends text to a file, creating it (with a trailing newline) if needed. */ -export function appendToFile(file: string, text: string): void { +function appendToFile(file: string, text: string): void { if (text === "") return; if (fs.existsSync(file)) { const existing = fs.readFileSync(file, "utf-8"); diff --git a/packages/shared/src/cli/commands/registry/server-register.test.ts b/packages/shared/src/cli/commands/registry/server-register.test.ts index e270c424d..4809d31c2 100644 --- a/packages/shared/src/cli/commands/registry/server-register.test.ts +++ b/packages/shared/src/cli/commands/registry/server-register.test.ts @@ -2,9 +2,12 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; -import { registerPluginInServer } from "./server-register"; +import { + registerPluginInServer, + registerPluginsInServer, +} from "./server-register"; const tempDirs: string[] = []; @@ -37,6 +40,45 @@ afterEach(() => { tempDirs.length = 0; }); +describe("registerPluginsInServer", () => { + it("registers several plugins with one write, and none on a rerun", () => { + const dir = makeTempDir(); + const file = writeServer(dir, "server.ts", "server()"); + const entries = [ + { importPath: "@databricks/appkit", exportName: "analytics" }, + { importPath: "@databricks/appkit", exportName: "genie" }, + { + importPath: "@databricks/appkit/beta", + exportName: "agents", + callArgs: `{ auth: "on-behalf-of-user" }`, + }, + ]; + const writes = vi.spyOn(fs, "writeFileSync"); + const first = registerPluginsInServer(dir, entries); + expect(first.map((r) => r.status)).toEqual(["wired", "wired", "wired"]); + expect(writes).toHaveBeenCalledTimes(1); + const out = fs.readFileSync(file, "utf-8"); + expect(out).toContain( + `import { createApp, analytics, genie } from "@databricks/appkit";`, + ); + expect(out).toContain(`import { agents } from "@databricks/appkit/beta";`); + expect(out).toContain( + `agents({ auth: "on-behalf-of-user" }), genie(), analytics(), server()`, + ); + + writes.mockClear(); + const second = registerPluginsInServer(dir, entries); + expect(second.map((r) => r.status)).toEqual([ + "already", + "already", + "already", + ]); + expect(writes).not.toHaveBeenCalled(); + expect(fs.readFileSync(file, "utf-8")).toBe(out); + writes.mockRestore(); + }); +}); + describe("registerPluginInServer", () => { it("extends an existing import from the same module, idempotently", () => { const dir = makeTempDir(); diff --git a/packages/shared/src/cli/commands/registry/server-register.ts b/packages/shared/src/cli/commands/registry/server-register.ts index 10d473fe8..0b21a51c3 100644 --- a/packages/shared/src/cli/commands/registry/server-register.ts +++ b/packages/shared/src/cli/commands/registry/server-register.ts @@ -102,13 +102,60 @@ function importBindingNames(stmt: SgNode): Set { * standard shape (a `plugins: [...]` array literal); returns `skipped` otherwise * so the caller can fall back to printing manual instructions. Idempotent. */ +/** A plugin to register: `import { exportName } from importPath` + `exportName(callArgs)`. */ +export interface RegisterEntry { + importPath: string; + exportName: string; + /** Call arguments, e.g. `{ auth: "on-behalf-of-user" }` (CLI-built, never registry data). */ + callArgs?: string; +} + export function registerPluginInServer( serverRoot: string, importPath: string, exportName: string, - /** Call arguments, e.g. `{ auth: "on-behalf-of-user" }` (CLI-built, never registry data). */ callArgs = "", ): RegisterResult { + return registerPluginsInServer(serverRoot, [ + { importPath, exportName, callArgs }, + ])[0]; +} + +/** + * Registers several plugins in the server entry, writing the file once (a + * dev server watching it restarts once). Entries apply in order; each + * result is per entry. + */ +export function registerPluginsInServer( + serverRoot: string, + entries: RegisterEntry[], +): RegisterResult[] { + const serverFile = findServerFile(serverRoot); + if (!serverFile) { + return entries.map(() => ({ + status: "skipped", + reason: "no server entry file found", + })); + } + const file = path.relative(serverRoot, serverFile); + const lang = serverFile.endsWith(".tsx") ? Lang.Tsx : Lang.TypeScript; + const original = fs.readFileSync(serverFile, "utf-8"); + let content = original; + const results = entries.map((e): RegisterResult => { + const { output, ...r } = registerInSource(content, lang, e); + if (output !== undefined) content = output; + return r.status === "skipped" ? r : { ...r, file }; + }); + if (content !== original) fs.writeFileSync(serverFile, content); + return results; +} + +/** Applies one entry to the server source; `output` is set when it changed. */ +function registerInSource( + content: string, + lang: Lang, + { importPath, exportName, callArgs = "" }: RegisterEntry, +): RegisterResult & { output?: string } { // exportName and importPath are interpolated into the user's server source. // Registry items are untrusted, so refuse anything that isn't a plain JS // identifier / clean relative module path — prevents code injection via a @@ -124,13 +171,6 @@ export function registerPluginInServer( return { status: "skipped", reason: "invalid plugin import path" }; } - const serverFile = findServerFile(serverRoot); - if (!serverFile) { - return { status: "skipped", reason: "no server entry file found" }; - } - - const content = fs.readFileSync(serverFile, "utf-8"); - const lang = serverFile.endsWith(".tsx") ? Lang.Tsx : Lang.TypeScript; const root = parse(lang, content).root(); const arr = findPluginsArray(root); @@ -141,9 +181,8 @@ export function registerPluginInServer( }; } - const file = path.relative(serverRoot, serverFile); if (arrayElementNames(arr).has(exportName)) { - return { status: "already", file }; + return { status: "already" }; } const edits = []; @@ -202,7 +241,5 @@ export function registerPluginInServer( if (!hasBinding && !named && importStmts.length === 0) { output = `${importLine}\n${output}`; } - fs.writeFileSync(serverFile, output); - - return { status: "wired", file }; + return { status: "wired", output }; } From 9313e0c8ecc3cfc52a6ed6422c4dd7d6ec6f6bee Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Wed, 28 Oct 2026 10:44:49 +0100 Subject: [PATCH 13/16] fix(shared): plugin add plans agents last; registry add skips the bundle check for UI-only items - agents is planned after the other plugins of a run, so its fallback for a shared DATABRICKS_SERVING_ENDPOINT_NAME works in any argument order - the registry add command checks the bundle's app only when a fetched item declares resources, so UI-only items install in multi-app projects again - plugin add's command is built by createPluginAddCommand (tests parse fresh instances; commander keeps parsed values per instance) Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../shared/src/cli/commands/plugin/add/add.ts | 87 +++++++++++-------- .../src/cli/commands/plugin/add/run.test.ts | 38 +++++++- .../registry/add-package-manager.test.ts | 26 ++++++ .../shared/src/cli/commands/registry/add.ts | 11 ++- 4 files changed, 121 insertions(+), 41 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index ec57ea0af..0dbe99be1 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -497,8 +497,14 @@ async function runPluginAdd(names: string[], opts: AddOptions): Promise { seen.add(r.name); } + // agents is planned last: it is the one plugin that adapts to an env clash + // (see preparePlugin), so it must see the others' entries in any order. + const ordered = [ + ...requests.filter((r) => r.name !== "agents"), + ...requests.filter((r) => r.name === "agents"), + ]; const prepared: PreparedPlugin[] = []; - for (const req of requests) { + for (const req of ordered) { prepared.push( await preparePlugin( cwd, @@ -718,35 +724,40 @@ async function registerInServer( }); } -export const pluginAddCommand = new Command("add") - .description( - "Add plugins from appkit.plugins.json to the app: server registration, .env, app.yaml and databricks.yml, each written once", - ) - .argument("[plugins...]", "Plugin names, as in appkit.plugins.json") - .option( - "--from-plan ", - "Add the plugins listed in a data plan file (e.g. .appkit/data-plan.json, version 1)", - ) - .option( - "--set ", - "Set resource values (format: plugin.resourceKey.field=value, can specify multiple)", - (v: string, acc: string[]) => [...acc, v], - [] as string[], - ) - .option( - "--auth-mode ", - "Default resource auth mode: obo (on behalf of the user) or sp (service principal). With obo, resources that cannot be accessed on behalf of the user use sp. Prompts if not provided in interactive mode, otherwise sp.", - ) - .option("-C, --cwd ", "Run as if started in ") - .option("-y, --yes", "Don't prompt; fail on missing required values") - .option("--no-register", "Don't edit the server entry to register the plugin") - .option( - "-p, --profile ", - "Databricks profile for bundle validate (defaults to the app's DATABRICKS_CONFIG_PROFILE)", - ) - .addHelpText( - "after", - ` +/** A fresh `plugin add` command (commander keeps parsed values per instance). */ +export const createPluginAddCommand = () => + new Command("add") + .description( + "Add plugins from appkit.plugins.json to the app: server registration, .env, app.yaml and databricks.yml, each written once", + ) + .argument("[plugins...]", "Plugin names, as in appkit.plugins.json") + .option( + "--from-plan ", + "Add the plugins listed in a data plan file (e.g. .appkit/data-plan.json, version 1)", + ) + .option( + "--set ", + "Set resource values (format: plugin.resourceKey.field=value, can specify multiple)", + (v: string, acc: string[]) => [...acc, v], + [] as string[], + ) + .option( + "--auth-mode ", + "Default resource auth mode: obo (on behalf of the user) or sp (service principal). With obo, resources that cannot be accessed on behalf of the user use sp. Prompts if not provided in interactive mode, otherwise sp.", + ) + .option("-C, --cwd ", "Run as if started in ") + .option("-y, --yes", "Don't prompt; fail on missing required values") + .option( + "--no-register", + "Don't edit the server entry to register the plugin", + ) + .option( + "-p, --profile ", + "Databricks profile for bundle validate (defaults to the app's DATABRICKS_CONFIG_PROFILE)", + ) + .addHelpText( + "after", + ` Resource configuration (--set), as in \`databricks apps init\`: Set resource values using --set plugin.resourceKey.field=value Keys are defined in the app's appkit.plugins.json manifest. @@ -775,10 +786,12 @@ Examples: $ appkit plugin add analytics genie --set analytics.sql-warehouse.id=abc123 \\ --set genie.genie-space.id=s1 --set genie.genie-space.name=Sales $ appkit plugin add --from-plan .appkit/data-plan.json --yes`, - ) - .action((names: string[], opts: AddOptions) => - runPluginAdd(names, opts).catch((err: Error) => { - console.error(pc.red(err.message)); - process.exit(1); - }), - ); + ) + .action((names: string[], opts: AddOptions) => + runPluginAdd(names, opts).catch((err: Error) => { + console.error(pc.red(err.message)); + process.exit(1); + }), + ); + +export const pluginAddCommand = createPluginAddCommand(); diff --git a/packages/shared/src/cli/commands/plugin/add/run.test.ts b/packages/shared/src/cli/commands/plugin/add/run.test.ts index 85f5ee9a1..8a76296a1 100644 --- a/packages/shared/src/cli/commands/plugin/add/run.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/run.test.ts @@ -4,7 +4,11 @@ import path from "node:path"; import { afterEach, describe, expect, test, vi } from "vitest"; -import { pluginAddCommand, requestsFromArgs, requestsFromPlan } from "./add"; +import { + createPluginAddCommand, + requestsFromArgs, + requestsFromPlan, +} from "./add"; // bundle validate would shell out to the Databricks CLI. vi.mock("../../registry/config-writer", async (importOriginal) => ({ @@ -126,7 +130,7 @@ test("adds several plugins writing each file once, and nothing on a rerun", asyn vi.spyOn(console, "log").mockImplementation(() => {}); vi.spyOn(console, "warn").mockImplementation(() => {}); const run = () => - pluginAddCommand.parseAsync( + createPluginAddCommand().parseAsync( ["--from-plan", ".appkit/data-plan.json", "--yes", "-C", dir], { from: "user" }, ); @@ -161,3 +165,33 @@ test("adds several plugins writing each file once, and nothing on a rerun", asyn await run(); expect(writes).not.toHaveBeenCalled(); }); + +test.each([[["agents", "serving"]], [["serving", "agents"]]])( + "agents and serving work in either order (%j)", + async (names) => { + const dir = makeApp(); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "warn").mockImplementation(() => {}); + await createPluginAddCommand().parseAsync( + [ + ...names, + "--set", + "agents.agents-serving-endpoint.name=agent-model", + "--set", + "serving.serving-endpoint.name=serving-model", + "--yes", + "-C", + dir, + ], + { from: "user" }, + ); + const server = fs.readFileSync( + path.join(dir, "server", "server.ts"), + "utf-8", + ); + expect(server).toContain(`agents({ defaultModel: "agent-model" })`); + expect(fs.readFileSync(path.join(dir, ".env"), "utf-8")).toContain( + "DATABRICKS_SERVING_ENDPOINT_NAME=serving-model", + ); + }, +); diff --git a/packages/shared/src/cli/commands/registry/add-package-manager.test.ts b/packages/shared/src/cli/commands/registry/add-package-manager.test.ts index 065e73b3c..4e7ef4cbb 100644 --- a/packages/shared/src/cli/commands/registry/add-package-manager.test.ts +++ b/packages/shared/src/cli/commands/registry/add-package-manager.test.ts @@ -83,4 +83,30 @@ describe("registry add package manager", () => { ); }, ); + + it("adds a UI-only item to a bundle with several apps", async () => { + fs.writeFileSync( + path.join(cwd, "databricks.yml"), + "resources:\n apps:\n frontend: {}\n backend: {}\n", + ); + vi.mocked(fetchRegistryItem).mockResolvedValue({ + name: "pm-fixture", + files: [ + { + type: "registry:component", + path: "banner.tsx", + target: "components/banner.tsx", + content: "export {};\n", + }, + ], + }); + + await addCommand.parseAsync(["pm-fixture", "--cwd", cwd, "--yes"], { + from: "user", + }); + + expect(fs.existsSync(path.join(cwd, "components", "banner.tsx"))).toBe( + true, + ); + }); }); diff --git a/packages/shared/src/cli/commands/registry/add.ts b/packages/shared/src/cli/commands/registry/add.ts index 78191878a..bb492e8c6 100644 --- a/packages/shared/src/cli/commands/registry/add.ts +++ b/packages/shared/src/cli/commands/registry/add.ts @@ -450,8 +450,6 @@ export function partitionVerified( async function runAdd(refs: string[], opts: AddOptions): Promise { const cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd(); - // The deploy-config write refuses an ambiguous bundle; check before any file. - if (opts.resources !== false) assertBundleApp(cwd); // Resolve the full graph and fetch the verified index concurrently (two // independent round-trips). Item resolution is read-only — nothing is written @@ -489,6 +487,15 @@ async function runAdd(refs: string[], opts: AddOptions): Promise { } } + // The deploy-config write refuses an ambiguous bundle: check before any + // file, and only when an item has resources to write (UI items don't). + if ( + opts.resources !== false && + items.some((i) => extractRequirements(i).length > 0) + ) { + assertBundleApp(cwd); + } + const hasUi = items.some((i) => !isPluginItem(i)); const hasPlugin = items.some(isPluginItem); const frontendRoot = hasUi ? findFrontendRoot(cwd) : cwd; From 0db52f0363bb2846b248fee5dbeedced35a44a05 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Wed, 28 Oct 2026 10:53:46 +0100 Subject: [PATCH 14/16] ci: install the pnpm template smoke test with a fresh store pnpm 11 drops a package's bundledDependencies when it reuses that package from a store, and appkit/appkit-ui ship patched SDKs that way. CI restores a cached pnpm store, so every run after the first failed to find @databricks/sdk-statementexecution. Users with a warm store are affected too; that needs a packaging fix, separate from this test. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- tools/template-smoke-runner.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tools/template-smoke-runner.ts b/tools/template-smoke-runner.ts index 18541702c..8dc8add04 100644 --- a/tools/template-smoke-runner.ts +++ b/tools/template-smoke-runner.ts @@ -138,7 +138,17 @@ export async function runTemplateSmoke( writeFileSync(lockPath, committedLock); run("npm", ["ci", "--no-audit", "--no-fund"]); } else { - run("pnpm", ["install", "--frozen-lockfile"]); + // A fresh store: pnpm 11 drops a package's bundledDependencies when it + // reuses that package from a store, and appkit ships patched SDKs that + // way. CI restores a cached store, so a shared one breaks every run + // after the first. Users with a warm store are affected too; that needs + // a packaging fix, not this test. + run("pnpm", [ + "install", + "--frozen-lockfile", + "--store-dir", + join(guardDir, "pnpm-store"), + ]); const host: Record = { os: process.platform, cpu: process.arch, From a06e7852caabe16825e2a6f9adfea01f26aa1b22 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Wed, 28 Oct 2026 11:04:25 +0100 Subject: [PATCH 15/16] fix(shared): plugin add sets files auth per volume for mixed modes, merges shared resources - files with some OBO volumes and some SP volumes is registered with volumes..auth for the OBO ones (the key comes from the volume's DATABRICKS_VOLUME_ env var), instead of plugin-wide SP - a batch where plugins share a resource writes its app.yaml entry, bundle variable and binding once Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../src/cli/commands/plugin/add/add.test.ts | 15 +++++++++ .../shared/src/cli/commands/plugin/add/add.ts | 29 +++++++++++++++-- .../src/cli/commands/plugin/add/run.test.ts | 31 +++++++++++++++++++ 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/add.test.ts b/packages/shared/src/cli/commands/plugin/add/add.test.ts index d3dbc8ba8..4656ab5b0 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.test.ts @@ -117,3 +117,18 @@ test("envConflicts flags env names bound to another resource or value", () => { expect(envConflicts(cwd, plan)).toEqual(["A", "C"]); fs.rmSync(cwd, { recursive: true, force: true }); }); + +test("files with mixed modes sets auth on the OBO volumes only", () => { + const vol = (key: string, env: string): ManifestResource => ({ + type: "volume", + alias: key, + resourceKey: key, + scope: "files", + fields: { path: { env } }, + }); + const priv = vol("files", "DATABRICKS_VOLUME_FILES"); + const exp = vol("exports", "DATABRICKS_VOLUME_EXPORTS"); + expect( + pluginConfig(plugin("files"), [priv, exp], { "volume:files": "obo" }), + ).toEqual({ volumes: { files: { auth: "on-behalf-of-user" } } }); +}); diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index 0dbe99be1..42c05a48e 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -537,10 +537,14 @@ async function runPluginAdd(names: string[], opts: AddOptions): Promise { plans.flatMap((p) => p.dotEnv), ); + // Plugins can share a resource (same env var, variable and binding); + // envConflicts already rejected any that differ, so keep the first. + const byName = (xs: T[]) => + xs.filter((x, i) => xs.findIndex((y) => y.name === x.name) === i); const merged: ConfigPlan = { - appYamlEnv: plans.flatMap((p) => p.appYamlEnv), - bundleVariables: plans.flatMap((p) => p.bundleVariables), - resourceBindings: plans.flatMap((p) => p.resourceBindings), + appYamlEnv: byName(plans.flatMap((p) => p.appYamlEnv)), + bundleVariables: byName(plans.flatMap((p) => p.bundleVariables)), + resourceBindings: byName(plans.flatMap((p) => p.resourceBindings)), userApiScopes: [...new Set(plans.flatMap((p) => p.userApiScopes ?? []))], unverifiedTypes: [...new Set(plans.flatMap((p) => p.unverifiedTypes))], }; @@ -630,6 +634,16 @@ export function pluginConfig( } } } + // files takes auth per volume too: a volume's key is the lowercased + // suffix of its DATABRICKS_VOLUME_ env var (FilesPlugin.discoverVolumes). + if (plugin.name === "files" && oboRes.length > 0 && !obo) { + const keys = oboRes.map((r) => filesVolumeKey(r)); + if (keys.every(Boolean)) { + return { + volumes: Object.fromEntries(keys.map((k) => [k, { auth: OBO }])), + }; + } + } if (oboRes.length > 0 && !obo) { console.warn( pc.yellow( @@ -647,6 +661,15 @@ export function pluginConfig( return obo ? { auth: OBO } : undefined; } +/** The files plugin's volume key for a volume resource, from its env var. */ +function filesVolumeKey(r: ManifestResource): string | undefined { + const prefix = "DATABRICKS_VOLUME_"; + const env = Object.values(r.fields ?? {}) + .map((f) => f.env) + .find((e) => e?.startsWith(prefix) && e.length > prefix.length); + return env?.slice(prefix.length).toLowerCase(); +} + /** Source text for a config literal: `{ auth: "x", columns: ["a"] }`. */ export function toSource(v: unknown): string { if (Array.isArray(v)) return `[${v.map(toSource).join(", ")}]`; diff --git a/packages/shared/src/cli/commands/plugin/add/run.test.ts b/packages/shared/src/cli/commands/plugin/add/run.test.ts index 8a76296a1..b15f9dae0 100644 --- a/packages/shared/src/cli/commands/plugin/add/run.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/run.test.ts @@ -195,3 +195,34 @@ test.each([[["agents", "serving"]], [["serving", "agents"]]])( ); }, ); + +test("plugins sharing a resource in one run write its entries once", async () => { + const dir = makeApp(); + const file = path.join(dir, "appkit.plugins.json"); + const manifest = JSON.parse(fs.readFileSync(file, "utf-8")); + manifest.plugins.reporting = { + ...manifest.plugins.analytics, + name: "reporting", + }; + fs.writeFileSync(file, JSON.stringify(manifest)); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "warn").mockImplementation(() => {}); + await createPluginAddCommand().parseAsync( + [ + "analytics", + "reporting", + "--set", + "analytics.sql-warehouse.id=wh", + "--set", + "reporting.sql-warehouse.id=wh", + "--yes", + "-C", + dir, + ], + { from: "user" }, + ); + const appYaml = fs.readFileSync(path.join(dir, "app.yaml"), "utf-8"); + expect(appYaml.match(/DATABRICKS_WAREHOUSE_ID/g)).toHaveLength(1); + const yml = fs.readFileSync(path.join(dir, "databricks.yml"), "utf-8"); + expect(yml.match(/- name: sql-warehouse/g)).toHaveLength(1); +}); From be497de71d9a2b011e9df8d556c711955e249950 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 29 Oct 2026 17:19:47 +0100 Subject: [PATCH 16/16] fix(shared): plugin add --from-plan accepts set keys with the plugin prefix A plan with set {"analytics.sql-warehouse.id": ...} got the prefix added twice and failed. Keys of 3+ parts led by the plugin name are now used as is; resourceKey.field keys still get the prefix. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../shared/src/cli/commands/plugin/add/add.ts | 15 +++++-- .../src/cli/commands/plugin/add/run.test.ts | 42 +++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/packages/shared/src/cli/commands/plugin/add/add.ts b/packages/shared/src/cli/commands/plugin/add/add.ts index 42c05a48e..846007b68 100644 --- a/packages/shared/src/cli/commands/plugin/add/add.ts +++ b/packages/shared/src/cli/commands/plugin/add/add.ts @@ -446,7 +446,10 @@ const dataPlanSchema = z.object({ z.object({ name: z.string().min(1), authMode: z.enum([AUTH_MODE_OBO, AUTH_MODE_SP]).optional(), - /** `resourceKey.field` (or `resourceKey.authMode`) → value. */ + /** + * `resourceKey.field` (or `resourceKey.authMode`) → value; the full + * --set key with the plugin prefix is accepted too. + */ set: z.record(z.string(), z.string()).optional(), }), ) @@ -466,7 +469,13 @@ export function requestsFromPlan(file: string): PluginRequest[] { return parsed.data.plugins.map((p) => ({ name: p.name, authMode: p.authMode, - set: Object.entries(p.set ?? {}).map(([k, v]) => `${p.name}.${k}=${v}`), + set: Object.entries(p.set ?? {}).map(([k, v]) => { + // Prefixed = 3+ parts led by the plugin name; counting parts keeps + // "files.path" (resourceKey "files") unprefixed. + const parts = k.split("."); + const prefixed = parts.length >= 3 && parts[0] === p.name; + return `${prefixed ? k : `${p.name}.${k}`}=${v}`; + }), })); } @@ -799,7 +808,7 @@ Data plan file (--from-plan), version 1: { "name": "analytics", "authMode": "obo", "set": { "sql-warehouse.id": "abc123" } }, { "name": "genie", "set": { "genie-space.id": "s1", "genie-space.name": "Sales" } } ] } - "set" keys are the --set keys without the plugin prefix ("authMode" works per + "set" keys are the --set keys, with or without the plugin prefix ("authMode" works per resource, e.g. "sql-warehouse.authMode": "both"); "authMode" is the plugin's default. Examples: diff --git a/packages/shared/src/cli/commands/plugin/add/run.test.ts b/packages/shared/src/cli/commands/plugin/add/run.test.ts index b15f9dae0..db102843c 100644 --- a/packages/shared/src/cli/commands/plugin/add/run.test.ts +++ b/packages/shared/src/cli/commands/plugin/add/run.test.ts @@ -91,6 +91,25 @@ describe("requestsFromPlan", () => { ]); }); + test("accepts set keys that already carry the plugin prefix", () => { + const dir = makeApp(); + const file = path.join(dir, "plan.json"); + fs.writeFileSync( + file, + JSON.stringify({ + version: 1, + plugins: [ + { name: "analytics", set: { "analytics.sql-warehouse.id": "wh" } }, + { name: "files", set: { "files.path": "/Volumes/c/s/v" } }, + ], + }), + ); + expect(requestsFromPlan(file).map((r) => r.set)).toEqual([ + ["analytics.sql-warehouse.id=wh"], + ["files.files.path=/Volumes/c/s/v"], + ]); + }); + test("rejects an unknown version or auth mode", () => { const dir = makeApp(); const file = path.join(dir, "plan.json"); @@ -226,3 +245,26 @@ test("plugins sharing a resource in one run write its entries once", async () => const yml = fs.readFileSync(path.join(dir, "databricks.yml"), "utf-8"); expect(yml.match(/- name: sql-warehouse/g)).toHaveLength(1); }); + +test("--from-plan with prefixed set keys adds the plugin", async () => { + const dir = makeApp(); + fs.mkdirSync(path.join(dir, ".appkit")); + fs.writeFileSync( + path.join(dir, ".appkit", "data-plan.json"), + JSON.stringify({ + version: 1, + plugins: [ + { name: "analytics", set: { "analytics.sql-warehouse.id": "wh-9" } }, + ], + }), + ); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "warn").mockImplementation(() => {}); + await createPluginAddCommand().parseAsync( + ["--from-plan", ".appkit/data-plan.json", "--yes", "-C", dir], + { from: "user" }, + ); + expect(fs.readFileSync(path.join(dir, ".env"), "utf-8")).toContain( + "DATABRICKS_WAREHOUSE_ID=wh-9", + ); +});