diff --git a/src/bundle/compose.ts b/src/bundle/compose.ts index 9f954a5..3532e3d 100644 --- a/src/bundle/compose.ts +++ b/src/bundle/compose.ts @@ -19,6 +19,10 @@ export function compose(name: string, index: Map): Resol return merged!; } +export function composeChain(name: string, index: Map): string[] { + return linearize(name, index); +} + function stripExtends(b: BundleManifest): ResolvedBundle { const { extends: _e, ...rest } = b; return rest; diff --git a/src/bundle/discover.ts b/src/bundle/discover.ts index e3572a9..59d0cfa 100644 --- a/src/bundle/discover.ts +++ b/src/bundle/discover.ts @@ -11,6 +11,8 @@ export interface BundleEntry { path: string; manifest?: BundleManifest; malformed: boolean; + error?: string; + warnings?: string[]; shadowed: boolean; } @@ -43,10 +45,15 @@ function scanScope(dir: string, scope: BundleScope): BundleEntry[] { const name = file.slice(0, -3); let manifest: BundleManifest | undefined; let malformed = false; + let error: string | undefined; + let warnings: string[] | undefined; try { - manifest = loadManifest(path).manifest; - } catch { + const result = loadManifest(path); + manifest = result.manifest; + warnings = result.warnings; + } catch (e) { malformed = true; + error = e instanceof Error ? e.message : String(e); } out.push({ name: manifest?.name ?? name, @@ -54,6 +61,8 @@ function scanScope(dir: string, scope: BundleScope): BundleEntry[] { path, ...(manifest !== undefined ? { manifest } : {}), malformed, + ...(error !== undefined ? { error } : {}), + ...(warnings !== undefined && warnings.length > 0 ? { warnings } : {}), shadowed: false, }); } diff --git a/src/bundle/exec.ts b/src/bundle/exec.ts index 7d86b84..9f2036d 100644 --- a/src/bundle/exec.ts +++ b/src/bundle/exec.ts @@ -1,9 +1,9 @@ import { homedir } from "node:os"; import { dirname, join } from "node:path"; -import { NotFoundError } from "../errors.ts"; +import { NotFoundError, UsageError } from "../errors.ts"; import { computeClaudeArgs } from "./claude-args.ts"; import { compile } from "./compile.ts"; -import { type ResolvedBundle, compose } from "./compose.ts"; +import { type ResolvedBundle, compose, composeChain } from "./compose.ts"; import { discoverBundles } from "./discover.ts"; import { artifactRoots, @@ -76,21 +76,33 @@ export function resolveBundle( name: string, index: BundleIndex, env: NodeJS.ProcessEnv, -): { resolved: ResolvedBundle; sources: ResolvedSources } { +): { resolved: ResolvedBundle; sources: ResolvedSources; warnings: string[] } { const ix = new Map(); for (const e of index.entries) { if (e.manifest && !ix.has(e.name)) ix.set(e.name, e.manifest); } if (!ix.has(name)) { + const malformed = index.entries.find((e) => e.malformed && e.name === name); + if (malformed) { + throw new UsageError(malformed.error ?? `bundle '${name}' is malformed`); + } throw new NotFoundError(`bundle '${name}' not found`); } const resolved = compose(name, ix); + const chain = new Set(composeChain(name, ix)); + const warnings = [ + ...new Set( + index.entries + .filter((e) => !e.shadowed && chain.has(e.name)) + .flatMap((e) => e.warnings ?? []), + ), + ]; const projectSkillsDir = join(dirname(index.projectDir), "skills"); const sources = resolveSources(resolved, { roots: artifactRoots(env), projectSkillsDir, }); - return { resolved, sources }; + return { resolved, sources, warnings }; } export interface PrepareOpts { @@ -109,12 +121,13 @@ export interface PreparedInvocation { args: string[]; env: NodeJS.ProcessEnv; cacheDir: string; + warnings: string[]; } export function prepareBundleInvocation(opts: PrepareOpts): PreparedInvocation { const { name, claudeArgs, env, cwd } = opts; const index = opts.preloadedIndex ?? loadBundleIndex(env, cwd); - const { resolved, sources } = resolveBundle(name, index, env); + const { resolved, sources, warnings } = resolveBundle(name, index, env); const { cacheDir, version } = compile(resolved, sources, { cacheRoot: bundleCacheRoot(env), ...(opts.onBuild ? { onBuild: opts.onBuild } : {}), @@ -133,5 +146,6 @@ export function prepareBundleInvocation(opts: PrepareOpts): PreparedInvocation { args: [...computeClaudeArgs(resolved, cacheDir), ...claudeArgs], env: spawnEnv, cacheDir, + warnings, }; } diff --git a/src/bundle/list.ts b/src/bundle/list.ts index 49d1672..5876fad 100644 --- a/src/bundle/list.ts +++ b/src/bundle/list.ts @@ -49,12 +49,17 @@ function formatGroups(entries: BundleEntry[], dirs: ListScopeDirs, opts: RenderL return `${out.join("\n")}\n`; } +function malformedReason(error: string | undefined): string { + if (!error) return "malformed"; + return error.split("\n", 1)[0]!.replace(/^bundle .*?: /, ""); +} + function formatTable(rows: BundleEntry[], opts: RenderListOpts): string[] { const headers = ["NAME", "DESCRIPTION", "EXTENDS", "PINNED"]; const pinned = new Set(opts.pinnedNames ?? []); const data: string[][] = rows.map((r) => [ - r.name, - r.manifest?.description ?? "—", + r.malformed ? `${r.name} ⚠` : r.name, + r.malformed ? malformedReason(r.error) : (r.manifest?.description ?? "—"), (r.manifest?.extends ?? []).join(", ") || "—", pinned.has(r.name) ? (r.name === opts.defaultName ? "yes*" : "yes") : "—", ]); diff --git a/src/bundle/manifest.ts b/src/bundle/manifest.ts index e831127..d977f60 100644 --- a/src/bundle/manifest.ts +++ b/src/bundle/manifest.ts @@ -63,11 +63,24 @@ export interface ManifestResult { const NAME_RE = /^[a-z][a-z0-9-]{1,40}$/; +function parseFrontmatter( + path: string, + raw: string, +): { data: Record; body: string } { + try { + const parsed = matter(raw); + return { data: parsed.data as Record, body: parsed.content }; + } catch (e) { + const first = e instanceof Error ? e.message.split("\n", 1)[0] : String(e); + throw new UsageError( + `bundle ${path}: invalid YAML in frontmatter: ${first}\nHint: if a value contains \`{...}\`, \`[...]\`, or unquoted colons (e.g. code snippets in description), wrap it as a YAML block scalar:\n description: >-\n your text here`, + ); + } +} + export function loadManifest(path: string): ManifestResult { const raw = readFileSync(path, "utf8"); - const parsed = matter(raw); - const data = parsed.data as Record; - const body = parsed.content; + const { data, body } = parseFrontmatter(path, raw); if (typeof data.name !== "string" || data.name.length === 0) { throw new UsageError(`bundle ${path}: 'name' is required`); diff --git a/src/bundle/show.ts b/src/bundle/show.ts index d8a352c..8ee7b73 100644 --- a/src/bundle/show.ts +++ b/src/bundle/show.ts @@ -7,6 +7,7 @@ import type { ResolvedSources } from "./resolve.ts"; export interface ShowOpts { projectMcpPath?: string; + warnings?: string[]; } export function renderShow( @@ -25,9 +26,10 @@ export function renderShow( sections.push("## mcp"); sections.push(formatMcpDiff(bundle, sources, opts)); - if (sources.warnings.length > 0) { + const warnings = [...new Set([...sources.warnings, ...(opts.warnings ?? [])])]; + if (warnings.length > 0) { sections.push("## warnings"); - sections.push(sources.warnings.map((w) => `- ${w}`).join("\n")); + sections.push(warnings.map((w) => `- ${w}`).join("\n")); } return `${sections.join("\n\n")}\n`; diff --git a/src/run.ts b/src/run.ts index 2045362..e34f2fe 100644 --- a/src/run.ts +++ b/src/run.ts @@ -316,6 +316,7 @@ async function runBundleRun(rest: string[], env: NodeJS.ProcessEnv, cwd: string) // otherwise look like a freeze before claude's first output appears. onBuild: () => process.stderr.write(`building bundle '${resolvedName}'…\n`), }); + emitWarnings(prepared.warnings); const result = spawnSync(prepared.command, prepared.args, { env: prepared.env as NodeJS.ProcessEnv, stdio: "inherit", @@ -368,21 +369,30 @@ async function runBundleApply( name = picked.name; } - const built = buildBundle(name, index, env); + const { cacheDir, warnings } = buildBundle(name, index, env); + emitWarnings(warnings); const path = writePin(cwd, homedir(), name); process.stdout.write(`pinned bundle '${name}' at ${path}\n`); - process.stdout.write(`built ${built}\n`); + process.stdout.write(`built ${cacheDir}\n`); return 0; } +function emitWarnings(warnings: string[]): void { + for (const w of warnings) process.stderr.write(`${w}\n`); +} + function buildBundle( name: string, index: BundleIndex, env: NodeJS.ProcessEnv, forceRebuild = false, -): string { - const { resolved, sources } = resolveBundle(name, index, env); - return compile(resolved, sources, { cacheRoot: bundleCacheRoot(env), forceRebuild }).cacheDir; +): { cacheDir: string; warnings: string[] } { + const { resolved, sources, warnings } = resolveBundle(name, index, env); + const { cacheDir } = compile(resolved, sources, { + cacheRoot: bundleCacheRoot(env), + forceRebuild, + }); + return { cacheDir, warnings }; } function runBundleUnpin(cwd: string): number { @@ -425,8 +435,9 @@ async function runBundleBuild( if (typeof picked === "number") return picked; name = picked; } - const built = buildBundle(name, index, env, noCache); - process.stdout.write(`${built}\n`); + const { cacheDir, warnings } = buildBundle(name, index, env, noCache); + emitWarnings(warnings); + process.stdout.write(`${cacheDir}\n`); return 0; } @@ -438,9 +449,9 @@ async function runBundleShow(rest: string[], env: NodeJS.ProcessEnv, cwd: string if (typeof picked === "number") return picked; name = picked; } - const { resolved, sources } = resolveBundle(name, index, env); + const { resolved, sources, warnings } = resolveBundle(name, index, env); const projectMcpPath = join(cwd, ".mcp.json"); - process.stdout.write(renderShow(resolved, sources, { projectMcpPath })); + process.stdout.write(renderShow(resolved, sources, { projectMcpPath, warnings })); return 0; } diff --git a/test/integration/cli.bundle-malformed.test.ts b/test/integration/cli.bundle-malformed.test.ts new file mode 100644 index 0000000..546dff6 --- /dev/null +++ b/test/integration/cli.bundle-malformed.test.ts @@ -0,0 +1,61 @@ +import { spawnSync } from "node:child_process"; +import { existsSync, mkdirSync, writeFileSync } from "node:fs"; +import { dirname, join, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { cleanup, makeTmpDir } from "../helpers/tmp.ts"; + +const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); +const CLI = join(ROOT, "dist", "cli.js"); + +describe("cli bundle exit codes", () => { + let root: string; + let artifacts: string; + let bundles: string; + + beforeEach(() => { + if (!existsSync(CLI)) { + throw new Error(`dist/cli.js not built — run \`npm run build\` first (looked at ${CLI})`); + } + root = makeTmpDir(); + artifacts = join(root, "artifacts"); + bundles = join(artifacts, "bundles"); + mkdirSync(bundles, { recursive: true }); + }); + afterEach(() => cleanup(root)); + + function runCli(args: string[]): { status: number; stdout: string; stderr: string } { + const r = spawnSync("node", [CLI, ...args], { + encoding: "utf8", + cwd: root, + env: { + ...process.env, + HOME: root, + UMBEL_ARTIFACTS_DIR: artifacts, + UMBEL_CACHE_DIR: join(root, "cache"), + }, + }); + return { status: r.status ?? -1, stdout: r.stdout, stderr: r.stderr }; + } + + it("exit 2 with the real validation message on a malformed bundle", () => { + writeFileSync(join(bundles, "bad.md"), "---\nname: bad\nsettings:\n notAllowed: 1\n---\n"); + const r = runCli(["build", "bad"]); + expect(r.status).toBe(2); + expect(r.stderr).toMatch(/not in the whitelist/); + expect(r.stderr).not.toMatch(/not found/); + }); + + it("exit 3 on a genuinely nonexistent bundle name", () => { + const r = runCli(["build", "ghost"]); + expect(r.status).toBe(3); + expect(r.stderr).toMatch(/not found/); + }); + + it("exit 0 with a stderr warning on an unknown frontmatter field", () => { + writeFileSync(join(bundles, "typo.md"), "---\nname: typo\nbogusKey: 123\n---\nbody\n"); + const r = runCli(["build", "typo"]); + expect(r.status).toBe(0); + expect(r.stderr).toMatch(/unknown field 'bogusKey'/); + }); +}); diff --git a/test/unit/bundle/compose.test.ts b/test/unit/bundle/compose.test.ts index 8535d75..6f7b3de 100644 --- a/test/unit/bundle/compose.test.ts +++ b/test/unit/bundle/compose.test.ts @@ -149,3 +149,15 @@ describe("compose", () => { }); import type { ResolvedBundle } from "../../../src/bundle/compose.ts"; +import { composeChain } from "../../../src/bundle/compose.ts"; + +describe("composeChain", () => { + it("composeChain returns the linearized extends chain, ancestors first", () => { + const ix = new Map([ + ["base", { name: "base", body: "", sourcePath: "base.md" }], + ["mid", { name: "mid", extends: ["base"], body: "", sourcePath: "mid.md" }], + ["leaf", { name: "leaf", extends: ["mid"], body: "", sourcePath: "leaf.md" }], + ]); + expect(composeChain("leaf", ix)).toEqual(["base", "mid", "leaf"]); + }); +}); diff --git a/test/unit/bundle/discover.test.ts b/test/unit/bundle/discover.test.ts index 6a5450e..e21bf5d 100644 --- a/test/unit/bundle/discover.test.ts +++ b/test/unit/bundle/discover.test.ts @@ -78,4 +78,30 @@ describe("discoverBundles", () => { const out = discoverBundles({ userDir, projectDir }); expect(out.map((e) => e.name)).toEqual(["ok"]); }); + + it("preserves the validation error on a malformed entry", () => { + mkdirSync(userDir, { recursive: true }); + writeFileSync(join(userDir, "bad.md"), "---\nname: bad\nsettings:\n notAllowed: 1\n---\n"); + const out = discoverBundles({ userDir, projectDir }); + const bad = out.find((e) => e.name === "bad"); + expect(bad?.malformed).toBe(true); + expect(bad?.error).toMatch(/not in the whitelist/); + expect(bad?.manifest).toBeUndefined(); + }); + + it("preserves unknown-field warnings on a healthy entry", () => { + mkdirSync(userDir, { recursive: true }); + writeFileSync(join(userDir, "warn.md"), "---\nname: warn\nbogusKey: 1\n---\nbody\n"); + const out = discoverBundles({ userDir, projectDir }); + const warn = out.find((e) => e.name === "warn"); + expect(warn?.malformed).toBe(false); + expect(warn?.warnings?.some((w) => w.includes("bogusKey"))).toBe(true); + }); + + it("a clean entry has neither error nor warnings", () => { + writeBundleFile(userDir, "clean"); + const clean = discoverBundles({ userDir, projectDir }).find((e) => e.name === "clean"); + expect(clean?.error).toBeUndefined(); + expect(clean?.warnings).toBeUndefined(); + }); }); diff --git a/test/unit/bundle/exec.test.ts b/test/unit/bundle/exec.test.ts index 364a803..c632f95 100644 --- a/test/unit/bundle/exec.test.ts +++ b/test/unit/bundle/exec.test.ts @@ -1,8 +1,14 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { prepareBundleInvocation, resolveBundleName } from "../../../src/bundle/exec.ts"; +import { discoverBundles } from "../../../src/bundle/discover.ts"; +import { + prepareBundleInvocation, + resolveBundle, + resolveBundleName, +} from "../../../src/bundle/exec.ts"; import { writePin } from "../../../src/bundle/pin.ts"; +import { NotFoundError, UsageError } from "../../../src/errors.ts"; import { cleanup, makeTmpDir } from "../../helpers/tmp.ts"; describe("prepareBundleInvocation", () => { @@ -128,6 +134,12 @@ describe("prepareBundleInvocation", () => { }); expect(inv.args).not.toContain("--settings"); }); + + it("carries the bundle's unknown-field warnings out on the invocation", () => { + bundleFile("warny", "---\nname: warny\nbogusKey: 1\n---\n"); + const inv = prepareBundleInvocation({ name: "warny", claudeArgs: [], env: envWith(), cwd }); + expect(inv.warnings.some((w) => w.includes("bogusKey"))).toBe(true); + }); }); describe("resolveBundleName", () => { @@ -229,3 +241,57 @@ describe("resolveBundleName", () => { }); }); }); + +describe("resolveBundle", () => { + let root: string; + let userDir: string; + let projectDir: string; + + beforeEach(() => { + root = makeTmpDir("rb-"); + userDir = join(root, "user"); + projectDir = join(root, "project"); + mkdirSync(userDir, { recursive: true }); + mkdirSync(projectDir, { recursive: true }); + }); + afterEach(() => cleanup(root)); + + function indexOf() { + return { userDir, projectDir, entries: discoverBundles({ userDir, projectDir }) }; + } + + it("surfaces a malformed bundle's error as UsageError (exit-2), not NotFoundError", () => { + writeFileSync(join(userDir, "bad.md"), "---\nname: bad\nsettings:\n notAllowed: 1\n---\n"); + let caught: unknown; + try { + resolveBundle("bad", indexOf(), { UMBEL_ARTIFACTS_DIR: root }); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(UsageError); + expect((caught as Error).message).toMatch(/not in the whitelist/); + }); + + it("reports a genuinely missing bundle as NotFoundError (exit-3)", () => { + expect(() => resolveBundle("ghost", indexOf(), { UMBEL_ARTIFACTS_DIR: root })).toThrow( + NotFoundError, + ); + }); + + it("a healthy bundle shadowing a malformed same-named one resolves normally", () => { + writeFileSync(join(projectDir, "dup.md"), "---\nname: dup\n---\nbody\n"); + writeFileSync(join(userDir, "dup.md"), "---\nname: dup\nsettings:\n notAllowed: 1\n---\n"); + expect(() => resolveBundle("dup", indexOf(), { UMBEL_ARTIFACTS_DIR: root })).not.toThrow(); + }); + + it("aggregates unknown-field warnings across the extends chain", () => { + writeFileSync(join(userDir, "base.md"), "---\nname: base\nbogusBase: 1\n---\n"); + writeFileSync( + join(userDir, "child.md"), + "---\nname: child\nextends: [base]\nbogusChild: 2\n---\n", + ); + const { warnings } = resolveBundle("child", indexOf(), { UMBEL_ARTIFACTS_DIR: root }); + expect(warnings.some((w) => w.includes("bogusBase"))).toBe(true); + expect(warnings.some((w) => w.includes("bogusChild"))).toBe(true); + }); +}); diff --git a/test/unit/bundle/list.test.ts b/test/unit/bundle/list.test.ts index cb580b2..526c55e 100644 --- a/test/unit/bundle/list.test.ts +++ b/test/unit/bundle/list.test.ts @@ -100,4 +100,24 @@ describe("renderList", () => { expect(out).not.toContain("PROJECT"); expect(out).toContain("USER"); }); + + it("marks a malformed bundle with a NAME marker and the reason in DESCRIPTION", () => { + const malformed: BundleEntry = { + name: "badsettings", + scope: "user", + path: "/x/badsettings.md", + malformed: true, + error: + "bundle /x/badsettings.md: settings.notAllowed is not in the whitelist (allowed: model)", + shadowed: false, + }; + const out = renderList([malformed], SCOPE_DIRS); + expect(out).toContain("badsettings ⚠"); + expect(out).toContain("settings.notAllowed is not in the whitelist"); + }); + + it("leaves a healthy row's NAME unmarked", () => { + const out = renderList([entry({ name: "web-dev", scope: "user" })], SCOPE_DIRS); + expect(out).not.toContain("⚠"); + }); }); diff --git a/test/unit/bundle/manifest.test.ts b/test/unit/bundle/manifest.test.ts index 617d403..87cf009 100644 --- a/test/unit/bundle/manifest.test.ts +++ b/test/unit/bundle/manifest.test.ts @@ -2,6 +2,7 @@ import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { loadManifest } from "../../../src/bundle/manifest.ts"; +import { UsageError } from "../../../src/errors.ts"; import { cleanup, makeTmpDir } from "../../helpers/tmp.ts"; describe("loadManifest", () => { @@ -210,4 +211,16 @@ plugins: [foo] expect(warnings.join("\n")).toMatch(/totallyMadeUp/); expect(warnings.join("\n")).toMatch(/plugins/); }); + + it("wraps a frontmatter parse error as an actionable UsageError, not a raw YAMLException", () => { + const path = writeBundle("broken", "---\nname: ok\ndescription: this: breaks\n---\nbody\n"); + let caught: unknown; + try { + loadManifest(path); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(UsageError); + expect((caught as Error).message).toMatch(/description: >-|block scalar/); + }); }); diff --git a/test/unit/bundle/show.test.ts b/test/unit/bundle/show.test.ts index 52000b9..4f9aa85 100644 --- a/test/unit/bundle/show.test.ts +++ b/test/unit/bundle/show.test.ts @@ -136,4 +136,19 @@ describe("renderShow", () => { expect(out).toMatch(/warnings/i); expect(out).toContain("- bundle 'x'"); }); + + it("includes manifest-level warnings in the ## warnings section", () => { + const out = renderShow(bundle({ name: "demo" }), emptySources(), { + warnings: ["bundle /x/demo.md: unknown field 'bogusKey' (ignored)"], + }); + expect(out).toContain("## warnings"); + expect(out).toContain("unknown field 'bogusKey'"); + }); + + it("de-duplicates warnings shared between sources and manifest", () => { + const sources = emptySources(); + sources.warnings = ["dup warning"]; + const out = renderShow(bundle({ name: "demo" }), sources, { warnings: ["dup warning"] }); + expect(out.match(/dup warning/g)?.length).toBe(1); + }); });