From 0fe1752620f12cf18d7f2902f456737ef72df634 Mon Sep 17 00:00:00 2001 From: Luiz Lima Date: Mon, 21 Sep 2026 18:52:51 -0300 Subject: [PATCH 1/3] chore: clean up after the unify, fingerprint and MCP fixes Stale comments and test titles from withdrawn rulings go, the lstat-failure refusal no longer claims the path exists, the MCP tests assert values and the three-writer case, and the symlink tests run on Windows through a junction. --- src/cli.ts | 13 +++++++------ src/emitters/shared.ts | 3 +++ test/cli.test.ts | 22 ++++++---------------- test/emitters/claude-code.test.ts | 13 +++++++++++++ test/emitters/kiro.test.ts | 5 +++-- test/unify.test.ts | 22 ++++++---------------- 6 files changed, 38 insertions(+), 40 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 8655401..f40dc1e 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -342,7 +342,7 @@ forge } const result = await applyPlan(base, variant, diff, toApply, { discardVariantMeta: o.take === "base" }); - // Ruling 33, dry pass: every recipe and profile edit the cascade will make is checked before + // Ruling 33, dry pass: every recipe `ingredients` rewrite the cascade will make is checked before // the first byte is written, so a refusal (an aliased reference) leaves the Forge untouched. const cascadeFiles = result.resolved ? await checkRecipeCascade(f, base.ref, variant.ref) : []; @@ -541,16 +541,17 @@ async function realpathOfNearest(abs: string): Promise { head = parent; continue; } - throw cannotResolve(head, lstatError); + throw cannotResolve(head, lstatError, "cannot be inspected"); } - throw cannotResolve(head, realpathError); + throw cannotResolve(head, realpathError, "exists but cannot be resolved"); } } } -function cannotResolve(p: string, e: unknown): Error { - const code = (e as NodeJS.ErrnoException).code ?? (e instanceof Error ? e.message : String(e)); - return new Error(`${p} exists but cannot be resolved (${code}) — unify cannot prove the target lies outside the Forge`); +/** `lstat` failing means the path could not even be inspected, so it is not known to exist. */ +function cannotResolve(p: string, e: unknown, what: "cannot be inspected" | "exists but cannot be resolved"): Error { + const code = (e as NodeJS.ErrnoException | null)?.code ?? (e instanceof Error ? e.message : String(e)); + return new Error(`${p} ${what} (${code}) — unify cannot prove the target lies outside the Forge`); } /** Whether anything — a file, a directory, even a dangling symlink — already sits at `p`. */ diff --git a/src/emitters/shared.ts b/src/emitters/shared.ts index 2978c0e..f14c509 100644 --- a/src/emitters/shared.ts +++ b/src/emitters/shared.ts @@ -36,6 +36,9 @@ export function mcpServers(ctx: EmitContext, target: string, file: string): Reco const key = outName(ing.meta); const prev = writtenBy.get(key); if (prev) ctx.warn(`${target}: two ingredients write the MCP server "${key}" into ${file}: ${prev} and ${ing.ref} (last wins)`); + // Last wins on the value, but a reassigned key keeps the slot where it was first written, so + // after a collision the surviving server can sit in the dropped one's position. It is warned + // about above; `sameJson` compares key order, so such a file may read as `update`. servers[key] = ing.meta.server; writtenBy.set(key, ing.ref); } diff --git a/test/cli.test.ts b/test/cli.test.ts index 0ab5054..7ef4233 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -771,8 +771,6 @@ describe("cli", () => { expect(await fs.readFile(path.join(root, "recipes/base.yaml"), "utf8")).toBe(before); }); - // Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - // Ruling 30: this test used to check Ruling 25's hint. That hint is gone, so its old assertion // (`not.toContain("sits inside the Forge")`) could never fail; it now checks the refusal and the // outside plan's content instead. @@ -894,8 +892,6 @@ describe("cli — forge unify final review", () => { }); }); -// Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - function gitStatus(dir: string): string { return execFileSync("git", ["-C", dir, "status", "--porcelain"], { encoding: "utf8" }); } @@ -916,8 +912,6 @@ describe("cli — forge unify cascade refusals write nothing (Ruling 33)", () => expect(await fs.readFile(path.join(root, "ingredients/rules/wf/rule.md"), "utf8")).toBe("a\n"); }); - // Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - // A write that fails after writing began (a read-only recipe here; an I/O error or a locked file // on Windows in the wild) cannot be prevented by the dry pass — it must name what was written. // Root ignores the read-only bit on POSIX, so the scenario cannot fail there. @@ -1002,8 +996,8 @@ describe("cli — forge unify --save-plan never writes into the Forge, never ove }); describe("cli — forge unify --save-plan symlink escape (Ruling 30)", () => { - // Creating a symlink needs privileges on Windows; this runs on Linux CI. - it.skipIf(process.platform === "win32")("refuses a target that reaches inside the Forge through a symlink, and writes nothing", async () => { + // On Windows a directory junction needs no privilege, so this runs on both platforms. + it("refuses a target that reaches inside the Forge through a symlink, and writes nothing", async () => { const root = await tmpDir("craftar-cli-forge-"); cleanups.push(() => fs.rm(root, { recursive: true, force: true })); // A real `recipes/` directory, so the link below resolves inside the Forge — without a recipe @@ -1018,7 +1012,7 @@ describe("cli — forge unify --save-plan symlink escape (Ruling 30)", () => { const outside = await tmpDir("craftar-cli-plan-"); cleanups.push(() => fs.rm(outside, { recursive: true, force: true })); const link = path.join(outside, "into-forge"); - await fs.symlink(path.join(root, "recipes"), link, "dir"); + await fs.symlink(path.join(root, "recipes"), link, process.platform === "win32" ? "junction" : "dir"); const before = await snapshot(root); const r = runCli(["forge", "unify", "rule/wf", "--profile", "acme", "--save-plan", path.join(link, "plan.yaml"), "--forge", root]); @@ -1134,8 +1128,6 @@ describe("cli — forge unify refuses index-flagged paths (Ruling 39)", () => { }); describe("cli — forge unify checks the cascade's own files are held by git (Ruling 37)", () => { - // Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - it("refuses when only a recipe the cascade would rewrite is untracked (showUntrackedFiles=no), and leaves it unchanged", async () => { const root = await tmpDir("craftar-cli-forge-"); cleanups.push(() => fs.rm(root, { recursive: true, force: true })); @@ -1188,8 +1180,6 @@ describe("cli — forge unify's held-by-git refusal names every path, Forge-rela }); }); -// Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - describe("cli — forge unify's held-by-git refusal caps its list (docs-author nit)", () => { it("lists the first ten paths git does not hold and counts the rest", async () => { const root = await tmpDir("craftar-cli-forge-"); @@ -1316,8 +1306,8 @@ describe("cli — forge unify's cascade rewrites ingredients only (Ruling 42)", }); describe("cli — forge unify --save-plan through a dangling symlink (Ruling 30, CI round)", () => { - // Creating a symlink needs privileges on Windows; this runs on Linux CI. - it.skipIf(process.platform === "win32")("refuses a target whose path passes through a dangling symlink, and writes nothing", async () => { + // On Windows a directory junction needs no privilege, so this runs on both platforms. + it("refuses a target whose path passes through a dangling symlink, and writes nothing", async () => { const root = await tmpDir("craftar-cli-forge-"); cleanups.push(() => fs.rm(root, { recursive: true, force: true })); await makeForge(root, { ingredients: [rule("wf", "a\n"), rule("wf--acme", "b\n", { as: "wf" })] }); @@ -1327,7 +1317,7 @@ describe("cli — forge unify --save-plan through a dangling symlink (Ruling 30, const outside = await tmpDir("craftar-cli-plan-"); cleanups.push(() => fs.rm(outside, { recursive: true, force: true })); const link = path.join(outside, "dangling"); - await fs.symlink(path.join(root, "not-there-yet"), link, "dir"); // points into the Forge, at nothing + await fs.symlink(path.join(root, "not-there-yet"), link, process.platform === "win32" ? "junction" : "dir"); // points into the Forge, at nothing const before = await snapshot(root); const r = runCli(["forge", "unify", "rule/wf", "--profile", "acme", "--save-plan", path.join(link, "plan.yaml"), "--forge", root]); diff --git a/test/emitters/claude-code.test.ts b/test/emitters/claude-code.test.ts index b92f069..2847701 100644 --- a/test/emitters/claude-code.test.ts +++ b/test/emitters/claude-code.test.ts @@ -57,6 +57,19 @@ describe("claude-code emitter", () => { expect(text(p, ".mcp.json")).toBe(JSON.stringify({ mcpServers: { srv: { command: "npx", args: ["acme-server"] } } }, null, 2) + "\n"); }); + it("names each dropped writer once when three MCP ingredients emit the same server name", async () => { + const p = await planFor([ + { meta: { type: "mcp", name: "srv", server: { command: "a" } } }, + { meta: { type: "mcp", name: "srv--b", as: "srv", server: { command: "b" } } }, + { meta: { type: "mcp", name: "srv--c", as: "srv", server: { command: "c" } } }, + ]); + expect(p.warnings.filter((w) => w.includes('MCP server "srv"'))).toEqual([ + 'claude-code: two ingredients write the MCP server "srv" into .mcp.json: mcp/srv and mcp/srv--b (last wins)', + 'claude-code: two ingredients write the MCP server "srv" into .mcp.json: mcp/srv--b and mcp/srv--c (last wins)', + ]); + expect(text(p, ".mcp.json")).toBe(JSON.stringify({ mcpServers: { srv: { command: "c" } } }, null, 2) + "\n"); + }); + it("warns when two MCP ingredients emit the same server name, instead of dropping one silently", async () => { const p = await planFor([ { meta: { type: "mcp", name: "srv", server: { command: "npx", args: ["public-server"] } } }, diff --git a/test/emitters/kiro.test.ts b/test/emitters/kiro.test.ts index 636f14f..e0267e8 100644 --- a/test/emitters/kiro.test.ts +++ b/test/emitters/kiro.test.ts @@ -85,8 +85,8 @@ describe("kiro emitter", () => { it("emits an MCP variant under its original server name", async () => { const p = await planFor([{ meta: { type: "mcp", name: "srv--acme", as: "srv", server: { command: "npx", args: ["acme-server"] } } }]); - const json = JSON.parse(file(p, ".kiro/settings/mcp.json")!.content.toString("utf8")); - expect(Object.keys(json.mcpServers)).toEqual(["srv"]); + const f = file(p, ".kiro/settings/mcp.json")!; + expect(f.content.toString("utf8")).toBe(JSON.stringify({ mcpServers: { srv: { command: "npx", args: ["acme-server"] } } }, null, 2).replace(/\n/g, "\r\n") + "\r\n"); }); it("warns when two MCP ingredients emit the same server name, instead of dropping one silently", async () => { @@ -95,5 +95,6 @@ describe("kiro emitter", () => { { meta: { type: "mcp", name: "srv--acme", as: "srv", server: { command: "npx", args: ["acme-server"] } } }, ]); expect(p.warnings).toContain('kiro: two ingredients write the MCP server "srv" into .kiro/settings/mcp.json: mcp/srv and mcp/srv--acme (last wins)'); + expect(JSON.parse(file(p, ".kiro/settings/mcp.json")!.content.toString("utf8")).mcpServers).toEqual({ srv: { command: "npx", args: ["acme-server"] } }); }); }); diff --git a/test/unify.test.ts b/test/unify.test.ts index cf3fc4f..6925286 100644 --- a/test/unify.test.ts +++ b/test/unify.test.ts @@ -370,8 +370,6 @@ describe("rewriteRecipes", () => { expect(reloaded.recipes.get("base--acme")!.ingredients).toEqual(["rule/workflow", "rule/other"]); }); - // Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - it("leaves a suffixed recipe alone when it has no unsuffixed sibling", async () => { const forge = await forgeWith({ ingredients: [rule("workflow", "a\n"), rule("workflow--acme", "b\n", { as: "workflow" })], @@ -385,7 +383,7 @@ describe("rewriteRecipes", () => { expect(reloaded.profiles.get("acme")!.recipes).toEqual(["solo--acme"]); }); - it("does not delete a sibling whose ingredients differ beyond the variant", async () => { + it("does not report a sibling whose ingredients differ beyond the variant as identical", async () => { const forge = await forgeWith({ ingredients: [rule("workflow", "a\n"), rule("workflow--acme", "b\n", { as: "workflow" })], recipes: [ @@ -395,14 +393,14 @@ describe("rewriteRecipes", () => { profiles: [profile("acme", ["base--acme"])], }); const out = await rewriteRecipes(forge, "rule/workflow", "rule/workflow--acme", "acme"); - expect(out.identicalToSibling).toEqual([]); // Ruling 42: `deleted` is gone; not identical, not reported + expect(out.identicalToSibling).toEqual([]); }); }); /** - * Loads a Forge from hand-written files rather than `makeForge`, so a test can put a recipe or - * profile under a filename/dirname that disagrees with its own `name` field, or control raw - * bytes (comments, EOL, BOM) precisely. `rewriteRecipes` never touches `ingredients/`, so these + * Loads a Forge from hand-written files rather than `makeForge`, so a test can put a recipe under + * a filename that disagrees with its own `name` field, or control the raw bytes (comments, EOL, + * BOM) of recipes and profiles precisely. `rewriteRecipes` never touches `ingredients/`, so these * scenarios skip writing ingredient directories entirely. */ async function bareForge(files: Record): Promise { @@ -412,7 +410,7 @@ async function bareForge(files: Record): Promise { return loadForge(root); } -describe("rewriteRecipes — file identity, byte fidelity and safety (Rulings 9, 10, 12, 13, 14)", () => { +describe("rewriteRecipes — file identity, byte fidelity and safety (Rulings 9, 10, 13, 14)", () => { it("finds a recipe by its `name` field, not its filename, and never touches a same-named decoy", async () => { const forge = await bareForge({ // The real "base--acme" recipe lives in a file named after something else (Ruling 9). @@ -435,10 +433,6 @@ describe("rewriteRecipes — file identity, byte fidelity and safety (Rulings 9, expect(decoyText).toBe("name: solo\ningredients:\n - rule/other\n"); }); - // Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - - // Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - it("keeps a CRLF, BOM-prefixed recipe file's EOL and BOM after the rewrite (Ruling 13)", async () => { const BOM = ""; const crlf = `${BOM}name: solo--acme\r\ningredients:\r\n - rule/workflow--acme\r\n - rule/other\r\n`; @@ -563,7 +557,3 @@ describe("unify — non-UTF-8 content (Ruling 31)", () => { expect(r.write["rule.md"]).toBe(`${BOM}a\nnew\n`); }); }); - -// Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. - -// Ruling 42 withdrew this behaviour (the cascade no longer deletes recipes or edits profiles or extends); its test was removed. From cddb5f47dad5a893d4314d3a8b64bfbad00a49a3 Mon Sep 17 00:00:00 2001 From: Luiz Lima Date: Mon, 21 Sep 2026 19:01:27 -0300 Subject: [PATCH 2/3] test(unify): pin the cannot-be-inspected refusal and fix two comments A symlink loop in a --save-plan target is refused on both platforms, as "cannot be inspected" on Linux. The mcpServers comment named the wrong state: a reordered MCP file with no lock entry reads as collision, not update. --- src/cli.ts | 16 +++++++++++----- src/emitters/shared.ts | 3 ++- test/cli.test.ts | 24 ++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index f40dc1e..f9c941b 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -518,10 +518,12 @@ async function readText(p: string): Promise { * path to exist, and a plan target usually does not yet — so the part that exists is resolved * (symlinks and all) and the part that does not is appended as written. * - * It walks up only past a component that truly does not exist (`lstat` says ENOENT/ENOTDIR). A - * component that exists but will not resolve — a dangling symlink, a symlink loop, a permission - * error — throws instead: resolving the rest lexically would let a link that points into the Forge - * pass the containment check, so unify fails closed rather than guess where the target lands. + * It walks up only past a component that truly does not exist (`lstat` says ENOENT/ENOTDIR). Any + * other outcome throws instead. A component `lstat` cannot inspect (a symlink loop or a permission + * error on Linux) "cannot be inspected"; one it can inspect but `realpath` cannot follow (a + * dangling symlink or junction, a loop on Windows) "exists but cannot be resolved". Resolving the + * rest lexically would let a link that points into the Forge pass the containment check, so unify + * fails closed rather than guess where the target lands. */ async function realpathOfNearest(abs: string): Promise { let head = abs; @@ -548,7 +550,11 @@ async function realpathOfNearest(abs: string): Promise { } } -/** `lstat` failing means the path could not even be inspected, so it is not known to exist. */ +/** + * The refusal for a `--save-plan` target that cannot be resolved. `what` is "cannot be inspected" + * when `lstat` itself failed, so the path is not known to exist, and "exists but cannot be + * resolved" when `lstat` succeeded and `realpath` did not. + */ function cannotResolve(p: string, e: unknown, what: "cannot be inspected" | "exists but cannot be resolved"): Error { const code = (e as NodeJS.ErrnoException | null)?.code ?? (e instanceof Error ? e.message : String(e)); return new Error(`${p} ${what} (${code}) — unify cannot prove the target lies outside the Forge`); diff --git a/src/emitters/shared.ts b/src/emitters/shared.ts index f14c509..67d3a27 100644 --- a/src/emitters/shared.ts +++ b/src/emitters/shared.ts @@ -38,7 +38,8 @@ export function mcpServers(ctx: EmitContext, target: string, file: string): Reco if (prev) ctx.warn(`${target}: two ingredients write the MCP server "${key}" into ${file}: ${prev} and ${ing.ref} (last wins)`); // Last wins on the value, but a reassigned key keeps the slot where it was first written, so // after a collision the surviving server can sit in the dropped one's position. It is warned - // about above; `sameJson` compares key order, so such a file may read as `update`. + // about above. Such a file does not adopt: `sameJson` is key-order sensitive, so a workspace + // file with no lock entry reads as `collision`. servers[key] = ing.meta.server; writtenBy.set(key, ing.ref); } diff --git a/test/cli.test.ts b/test/cli.test.ts index 7ef4233..7a3396a 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -1327,4 +1327,28 @@ describe("cli — forge unify --save-plan through a dangling symlink (Ruling 30, expect(r.stderr).not.toContain("ENOENT: no such file"); expect(await snapshot(root)).toEqual(before); }); + + it("refuses a target whose path passes through a symlink loop, and writes nothing", async () => { + const root = await tmpDir("craftar-cli-forge-"); + cleanups.push(() => fs.rm(root, { recursive: true, force: true })); + await makeForge(root, { ingredients: [rule("wf", "a\n"), rule("wf--acme", "b\n", { as: "wf" })] }); + gitInit(root); + gitCommitAll(root, "init"); + + const outside = await tmpDir("craftar-cli-plan-"); + cleanups.push(() => fs.rm(outside, { recursive: true, force: true })); + const type = process.platform === "win32" ? "junction" : "dir"; + await fs.symlink(path.join(outside, "loopb"), path.join(outside, "loopa"), type); + await fs.symlink(path.join(outside, "loopa"), path.join(outside, "loopb"), type); + + const before = await snapshot(root); + const r = runCli(["forge", "unify", "rule/wf", "--profile", "acme", "--save-plan", path.join(outside, "loopa", "plan.yaml"), "--forge", root]); + expect(r.code).toBe(1); + expect(r.stderr).toContain("cannot prove the target lies outside the Forge"); + // `lstat` through a symlink loop fails with ELOOP on Linux, so the path cannot even be + // inspected; on Windows `lstat` sees the junction and `realpath` fails, the other branch. + if (process.platform !== "win32") expect(r.stderr).toContain("cannot be inspected (ELOOP)"); + else expect(r.stderr).toContain("exists but cannot be resolved"); + expect(await snapshot(root)).toEqual(before); + }); }); From e9fa051abd6172c63a8c2d2a57f5619b86f5b949 Mon Sep 17 00:00:00 2001 From: Luiz Lima Date: Mon, 21 Sep 2026 19:04:46 -0300 Subject: [PATCH 3/3] chore: update version 0.2.3: patch, for the post-unify cleanup. --- package-lock.json | 4 ++-- package.json | 2 +- src/cli.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package-lock.json b/package-lock.json index 66db67d..b72741c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "craftar", - "version": "0.2.2", + "version": "0.2.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "craftar", - "version": "0.2.2", + "version": "0.2.3", "license": "MIT", "dependencies": { "commander": "^13.1.0", diff --git a/package.json b/package.json index 6e989c8..1a768cf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "craftar", - "version": "0.2.2", + "version": "0.2.3", "description": "Craft, sync and convert AI-coding workspace harnesses across clients and tools.", "license": "MIT", "type": "module", diff --git a/src/cli.ts b/src/cli.ts index f9c941b..70332a8 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -25,7 +25,7 @@ import { UnifyPlanSchema, type IngredientRef, type Take, type UnifyPlan } from " process.stdout.on("error", (e: NodeJS.ErrnoException) => { if (e.code === "EPIPE") process.exit(0); }); const program = new Command(); -program.name("craftar").description("Craft, sync and convert AI-coding workspace harnesses.").version("0.2.2"); +program.name("craftar").description("Craft, sync and convert AI-coding workspace harnesses.").version("0.2.3"); /* ---------------------------------------------------------------- import */ program