From 3f51ac71b5027bca2fbf9dce46454021771b3f6d Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 22:11:55 -0400 Subject: [PATCH 1/5] fix(cli): --print-only never writes on the agent-context paths either MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--config-only --print-only` was fixed by moving the config branch below the printOnly guard. The two agent-context branches — `--docs-only` and `--refresh-docs` — return from `init()` above that same guard and were left, so both documented dry runs silently scaffolded for real: the docs, every stack-scoped skill reference, the manifest, and (with --wire-root) a created root CLAUDE.md. The reason it was left is that this write set is DYNAMIC — it depends on the resolved stack, so it cannot be the hardcoded path list the full-scaffold dry run uses. It does not need to be. `writeAgentContext` already computes the complete plan (`planScaffold`) before performing a single write, so the guard goes at the I/O, not at the report: `result.created` is populated from the same plan a real run executes, which makes the dry run's output exactly the real write set with no second list to drift. `.agent-context.json` joins that report. A real run has always written it while omitting it from `created`, and the full-scaffold dry run has always named it — reporting it here settles both against what actually happens on disk. Proven by reverting: both new tests go red (the docs land on disk), and the `--refresh-docs` arm is a genuinely separate door, red on its own. Co-Authored-By: Claude Opus 5 (1M context) --- .../packages/cli/src/commands/init.ts | 43 +++++++++++++------ .../cli/test/unit/init-docs-only.test.ts | 38 +++++++++++++++- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/server/typescript/packages/cli/src/commands/init.ts b/server/typescript/packages/cli/src/commands/init.ts index e82fa7033..c401ffa31 100644 --- a/server/typescript/packages/cli/src/commands/init.ts +++ b/server/typescript/packages/cli/src/commands/init.ts @@ -244,32 +244,49 @@ async function writeAgentContext(opts: InitOptions, result: InitResult): Promise : decision.writes; const conflicts = opts.force ? [] : decision.conflicts; + // --print-only must win outright: a documented dry run must never write. Both + // callers of this function (`--docs-only` and `--refresh-docs`) return from + // `init()` ABOVE the full-scaffold path's own printOnly guard, so without this + // the dry run silently scaffolded for real — the same defect `--config-only` + // carried. The guard lives HERE rather than as a path list beside that one + // because this write set is dynamic (it depends on the resolved stack), and + // `decision` is already the complete plan: suppressing just the I/O reports + // exactly the paths a real run would touch, with no second list to drift. + const dryRun = opts.printOnly === true; + for (const w of writes) { - const abs = join(opts.cwd, w.path); - await mkdir(dirname(abs), { recursive: true }); - await writeFile(abs, w.contents, "utf8"); + if (!dryRun) { + const abs = join(opts.cwd, w.path); + await mkdir(dirname(abs), { recursive: true }); + await writeFile(abs, w.contents, "utf8"); + } result.created.push(w.path); } for (const c of conflicts) { - const abs = join(opts.cwd, c.newPath); - await mkdir(dirname(abs), { recursive: true }); - await writeFile(abs, c.contents, "utf8"); + if (!dryRun) { + const abs = join(opts.cwd, c.newPath); + await mkdir(dirname(abs), { recursive: true }); + await writeFile(abs, c.contents, "utf8"); + } result.created.push(c.newPath); result.warnings.push(`${c.path} appears hand-edited; refreshed version written to ${c.newPath}`); } - const manifestAbs = join(opts.cwd, AGENT_CONTEXT_MANIFEST_PATH); - await mkdir(dirname(manifestAbs), { recursive: true }); - await writeFile(manifestAbs, JSON.stringify(decision.manifest, null, 2) + "\n", "utf8"); + if (!dryRun) { + const manifestAbs = join(opts.cwd, AGENT_CONTEXT_MANIFEST_PATH); + await mkdir(dirname(manifestAbs), { recursive: true }); + await writeFile(manifestAbs, JSON.stringify(decision.manifest, null, 2) + "\n", "utf8"); + } + result.created.push(AGENT_CONTEXT_MANIFEST_PATH); for (const orphan of decision.removed) { result.warnings.push(`${orphan} is no longer part of this stack; orphaned (safe to delete).`); } - if (opts.wireRoot) await wireRootMemory(opts.cwd, result); + if (opts.wireRoot) await wireRootMemory(opts.cwd, result, dryRun); } const ROOT_IMPORT_LINE = "@.metaobjects/AGENTS.md"; -async function wireRootMemory(cwd: string, result: InitResult): Promise { +async function wireRootMemory(cwd: string, result: InitResult, dryRun = false): Promise { const claudePath = join(cwd, "CLAUDE.md"); const agentsPath = join(cwd, "AGENTS.md"); const claudeExists = await fileExists(claudePath); @@ -277,7 +294,7 @@ async function wireRootMemory(cwd: string, result: InitResult): Promise { // If neither root memory file exists, create CLAUDE.md (Claude Code's canonical) with the import. if (!claudeExists && !agentsExists) { - await writeFile(claudePath, `# Project memory\n\n${ROOT_IMPORT_LINE}\n`, "utf8"); + if (!dryRun) await writeFile(claudePath, `# Project memory\n\n${ROOT_IMPORT_LINE}\n`, "utf8"); result.created.push("CLAUDE.md (created with MetaObjects @import)"); return; } @@ -286,7 +303,7 @@ async function wireRootMemory(cwd: string, result: InitResult): Promise { if (!exists) continue; const body = await readFile(path, "utf8"); if (body.includes(ROOT_IMPORT_LINE)) continue; - await writeFile(path, `${body.replace(/\n*$/, "\n")}\n${ROOT_IMPORT_LINE}\n`, "utf8"); + if (!dryRun) await writeFile(path, `${body.replace(/\n*$/, "\n")}\n${ROOT_IMPORT_LINE}\n`, "utf8"); result.warnings.push(`wired ${ROOT_IMPORT_LINE} into ${path.endsWith("AGENTS.md") ? "AGENTS.md" : "CLAUDE.md"} so the MetaObjects context loads`); } } diff --git a/server/typescript/packages/cli/test/unit/init-docs-only.test.ts b/server/typescript/packages/cli/test/unit/init-docs-only.test.ts index 2a9a399fc..23d914ec5 100644 --- a/server/typescript/packages/cli/test/unit/init-docs-only.test.ts +++ b/server/typescript/packages/cli/test/unit/init-docs-only.test.ts @@ -1,5 +1,5 @@ import { test, expect, describe, beforeEach, afterEach } from "bun:test"; -import { mkdtempSync, rmSync, existsSync } from "node:fs"; +import { mkdtempSync, rmSync, existsSync, mkdirSync, readdirSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { init } from "../../src/commands/init.js"; @@ -28,4 +28,40 @@ describe("init() --docs-only", () => { expect(existsSync(join(cwd, "metaobjects.config.ts"))).toBe(false); expect(existsSync(join(cwd, ".metaobjects/config.json"))).toBe(false); }); + + // Same shape as the `--config-only --print-only` bug: the agent-context branches + // return ABOVE the `--print-only` guard the full-scaffold path checks below them, + // so a documented dry run silently wrote the real files. Unlike that one, the write + // set here is DYNAMIC (it depends on the resolved stack), so the guard cannot be a + // hardcoded path list in `init()` — it belongs inside `writeAgentContext`, which + // already computes the exact plan before performing a single write. + test("--docs-only --print-only reports the real write set and writes nothing", async () => { + const planned = await init({ + cwd, docsOnly: true, printOnly: true, wireRoot: true, + servers: ["java"], clients: ["react"], + }); + + // Reported set is the REAL one, not a hardcoded guess: it names the docs, the + // stack-scoped skill reference, and the manifest. + expect(planned.created).toContain(".metaobjects/AGENTS.md"); + expect(planned.created).toContain(".claude/skills/metaobjects-codegen/references/java.md"); + expect(planned.created).toContain(".metaobjects/.agent-context.json"); + + // ...and the directory is untouched. + expect(readdirSync(cwd)).toEqual([]); + }); + + test("--refresh-docs --print-only writes nothing", async () => { + // The second door onto the same guard: refresh short-circuits on its own branch, + // which also sat above the --print-only check. It only engages once the project + // exists, so seed the marker directory first. + mkdirSync(join(cwd, ".metaobjects"), { recursive: true }); + + const planned = await init({ cwd, refreshDocs: true, printOnly: true, servers: ["java"] }); + + expect(planned.created).toContain(".metaobjects/AGENTS.md"); + expect(existsSync(join(cwd, ".metaobjects/AGENTS.md"))).toBe(false); + expect(existsSync(join(cwd, ".metaobjects/.agent-context.json"))).toBe(false); + expect(readdirSync(join(cwd, ".metaobjects"))).toEqual([]); + }); }); From 82d74f0e96a5158c26b265f89e3b8974ed406d0b Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 22:19:00 -0400 Subject: [PATCH 2/5] fix(csharp): a symlink cycle is loud, and three runners stop re-pinning the type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Following symlinked directories became a cross-port contract in #315, which is what makes a symlink CYCLE reachable. TypeScript, Java and Python each added a guard when they picked it up; C# did not, and the failure mode is worse than a hang. `Directory.EnumerateFiles(dir, "*", AllDirectories)` follows links but has no loop guard, and its `EnumerationOptions` default of `IgnoreInaccessible` swallows the kernel's own ELOOP refusal — so a self-referential directory symlink did not hang and did not throw. It COMPLETED NORMALLY, returning 41 copies of one real file at ever-deeper phantom paths (measured, not inferred). Nothing downstream could recover: `SourceResolver` de-duplicates on the LEXICAL full path and every phantom is lexically distinct, so each was admitted as its own source and the same metadata loaded once per level. `Expand` now walks itself, carrying the real ancestor directories on the current branch and raising on revisit. Resolution is component-by-component from the root, because `Directory.ResolveLinkTarget` canonicalizes only the FINAL segment: using it alone leaves a symlinked ancestor unresolved, and the guard then compares a half-resolved path against a real one and misses the loop — recursing forever on exactly the input it exists to catch. Ancestors extend only on the recursive call, never in place, so a diamond still resolves. The corpus gains `a-symlink-cycle-is-an-error`. Adding it exposed that THREE of the four runners had quietly re-pinned what the corpus refuses to pin: the README says `expectError: true` means "raises, type deliberately unpinned", but C# asserted MetaModelException, Python caught ParseError and Java caught MetaDataException. Each scored a correct port as a failure — the guards raise IOException, SymlinkLoopError and FileSystemLoopException respectively. All three now assert on the base type for the `true` form, and still demand the coded type for the string form, which is the arm that actually needs it. The case is documented as a FLOOR. On Linux the kernel's ELOOP makes an unguarded walk raise eventually anyway, so `expectError: true` cannot tell a real guard from a late accident; what it DOES discriminate is a port that swallows that error and reports success, which is precisely what C# did. Immediacy and the diamond distinction are pinned per-port instead — C# gains the two tests the other three already had. Co-Authored-By: Claude Opus 5 (1M context) --- .../source-resolution-conformance/README.md | 30 +++++ .../source-resolution-conformance/cases.json | 9 ++ .../DirectorySourceTests.cs | 69 +++++++++++ .../SourceResolutionConformanceTests.cs | 15 ++- .../MetaObjects/Loader/DirectorySource.cs | 112 +++++++++++++++++- .../SourceResolutionConformanceTest.java | 23 +++- .../test_source_resolution_conformance.py | 16 ++- 7 files changed, 253 insertions(+), 21 deletions(-) diff --git a/fixtures/source-resolution-conformance/README.md b/fixtures/source-resolution-conformance/README.md index 20ac2254f..c01469d27 100644 --- a/fixtures/source-resolution-conformance/README.md +++ b/fixtures/source-resolution-conformance/README.md @@ -31,6 +31,36 @@ README.md `real/…`) — see "Order is deliberately NOT pinned" below for the parallel point about `expectFiles` being exact strings, not just "the same underlying file by any name". Every port's runner must honor this key. + + Following symlinks is what makes a symlink CYCLE reachable, so + `a-symlink-cycle-is-an-error` gates the other half of the same contract: a + directory symlink that revisits a directory already on the current walk + branch must RAISE, not be walked. The failure it exists to catch is not a + hang — it is silent nonsense. Left unguarded, a walk yields the same real + file at ever-deeper phantom paths (`model/loop/model/loop/…/meta.a.json`), + and because de-duplication keys on the LEXICAL path those are all distinct, + so each is admitted as its own source and the same metadata loads over and + over. Which error is raised is deliberately not pinned (hence `expectError: + true`) — the ports raise their own native types. What is pinned is that a + cycle is loud. + + "Already on the current walk BRANCH" is the precise rule, not "already + seen": the ancestor set must be carried down the recursion and never shared + between siblings, so a directory legitimately reachable by two different + symlinked paths — a diamond, not a cycle — still resolves rather than being + falsely rejected. + + This case is a FLOOR, and deliberately so. On Linux the kernel refuses to + traverse past its own symlink-resolution depth (ELOOP around 40 levels), so + an unguarded walk in some runtimes raises anyway — late, and for the wrong + reason, but it raises, and `expectError: true` cannot tell that apart from a + real guard. What the case DOES discriminate is a port that swallows that + kernel error and reports success: C#'s `Directory.EnumerateFiles(..., + AllDirectories)` defaults to `IgnoreInaccessible`, so before this case it + completed normally and returned 41 phantom copies of one file. Because the + floor cannot pin immediacy, each port additionally owns a unit test that the + raise happens on REVISIT rather than at the kernel's limit — `sources.test.ts`, + `test_sources.py`, `DirectorySourceTest.java`, `DirectorySourceTests.cs`. - **`config`** — written verbatim to `.metaobjects/config.json`, under the directory named by `resolveFrom` (project root when `resolveFrom` is absent). When `null`, no config file is created at all. diff --git a/fixtures/source-resolution-conformance/cases.json b/fixtures/source-resolution-conformance/cases.json index aa535f582..5b5fc924c 100644 --- a/fixtures/source-resolution-conformance/cases.json +++ b/fixtures/source-resolution-conformance/cases.json @@ -264,6 +264,15 @@ "symlinks": { "model/linked": "external" }, "config": { "schema_version": 1, "sources": [{ "path": "model" }] }, "expectFiles": ["model/meta.top.json", "model/linked/meta.linked.json"] + }, + { + "name": "a-symlink-cycle-is-an-error", + "tree": { + "model/meta.a.json": "{\"metadata.root\":{\"children\":[]}}" + }, + "symlinks": { "model/loop": "model" }, + "config": { "schema_version": 1, "sources": [{ "path": "model" }] }, + "expectError": true } ] } diff --git a/server/csharp/MetaObjects.Conformance.Tests/DirectorySourceTests.cs b/server/csharp/MetaObjects.Conformance.Tests/DirectorySourceTests.cs index 0dee418dd..0e0f0f4cc 100644 --- a/server/csharp/MetaObjects.Conformance.Tests/DirectorySourceTests.cs +++ b/server/csharp/MetaObjects.Conformance.Tests/DirectorySourceTests.cs @@ -115,4 +115,73 @@ public void Expand_HonorsExcludeGlobs() Directory.Delete(dir, recursive: true); } } + + /// + /// The shared corpus (`a-symlink-cycle-is-an-error`) can only pin THAT a cycle + /// raises, because on Linux the kernel's own ELOOP makes an unguarded walk raise + /// eventually too. These two pin what it cannot: that the raise happens on REVISIT, + /// and that it distinguishes a cycle from a diamond. + /// + [Fact] + public void Expand_RaisesOnRevisit_NotAtTheKernelsSymlinkDepthLimit() + { + string dir = Path.Combine(Path.GetTempPath(), "ds_" + Path.GetRandomFileName()); + Directory.CreateDirectory(dir); + try + { + File.WriteAllText(Path.Combine(dir, "meta.a.json"), "{}"); + Directory.CreateSymbolicLink(Path.Combine(dir, "loop"), dir); + + var ex = Assert.Throws(() => new DirectorySource(dir).Expand().ToList()); + Assert.Contains("symlink loop detected", ex.Message); + + // Immediacy: the message names the FIRST revisit. Before the guard, + // Directory.EnumerateFiles(..., AllDirectories) swallowed the kernel's ELOOP + // (EnumerationOptions.IgnoreInaccessible) and returned ~40 phantom copies of + // meta.a.json with no error at all — so asserting only "it threw" would have + // been satisfied by a walk that had already descended 40 levels. + Assert.Contains(Path.Combine(dir, "loop"), ex.Message); + Assert.DoesNotContain(Path.Combine("loop", "loop"), ex.Message); + } + finally + { + Directory.Delete(dir, recursive: true); + } + } + + [Fact] + public void Expand_FollowsADiamond_WhichIsNotACycle() + { + string dir = Path.Combine(Path.GetTempPath(), "ds_" + Path.GetRandomFileName()); + Directory.CreateDirectory(dir); + try + { + // shared/ is reachable twice — via one/ and via two/ — but never from + // itself. The ancestor set must therefore be per-BRANCH: a set shared + // across siblings would see the second arrival as a revisit and reject a + // perfectly valid tree. + var shared = Path.Combine(dir, "shared"); + Directory.CreateDirectory(shared); + File.WriteAllText(Path.Combine(shared, "meta.shared.json"), "{}"); + Directory.CreateDirectory(Path.Combine(dir, "one")); + Directory.CreateDirectory(Path.Combine(dir, "two")); + Directory.CreateSymbolicLink(Path.Combine(dir, "one", "link"), shared); + Directory.CreateSymbolicLink(Path.Combine(dir, "two", "link"), shared); + + // One real file, reached by three distinct paths. Compared on FilePath, not + // Id — Id is the bare filename, so all three share it and a distinctness + // check there would pass on a single result just as happily. + var paths = new DirectorySource(dir).Expand().Select(f => f.FilePath).ToList(); + + Assert.Equal(3, paths.Count); + Assert.Equal(3, paths.Distinct().Count()); + Assert.Contains(Path.Combine(dir, "one", "link", "meta.shared.json"), paths); + Assert.Contains(Path.Combine(dir, "two", "link", "meta.shared.json"), paths); + Assert.Contains(Path.Combine(shared, "meta.shared.json"), paths); + } + finally + { + Directory.Delete(dir, recursive: true); + } + } } diff --git a/server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs b/server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs index c79347340..2718dac79 100644 --- a/server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs +++ b/server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs @@ -127,12 +127,19 @@ public void ResolvesTheSameFileSet(string name) if (c.ExpectError is not null) { - var ex = Assert.ThrowsAny(() => SourceResolver.ResolveCollection(invokeDir)); - // A string pins the exact code; `true` only pins that it raises — - // see the ExpectError field doc above. + // `true` pins only that resolution RAISES — deliberately not which type, + // so the assertion is on Exception. Narrowing it to MetaModelException + // would silently re-pin the very thing the corpus refuses to pin, and did: + // the symlink-cycle guard raises IOException (the natural type, and the + // one Java's FileSystemLoopException also derives from), which this + // runner scored as a FAILURE even though the port behaved correctly. + // A string still pins the exact code, and that arm requires the richer + // type — see the ExpectError field doc above. + var ex = Assert.ThrowsAny(() => SourceResolver.ResolveCollection(invokeDir)); if (c.ExpectError.Value.ValueKind == JsonValueKind.String) { - Assert.Equal(c.ExpectError.Value.GetString(), ex.Code.ToString()); + var coded = Assert.IsAssignableFrom(ex); + Assert.Equal(c.ExpectError.Value.GetString(), coded.Code.ToString()); } return; } diff --git a/server/csharp/MetaObjects/Loader/DirectorySource.cs b/server/csharp/MetaObjects/Loader/DirectorySource.cs index df06a62a5..70e049838 100644 --- a/server/csharp/MetaObjects/Loader/DirectorySource.cs +++ b/server/csharp/MetaObjects/Loader/DirectorySource.cs @@ -65,11 +65,7 @@ public DirectorySource(string directory, Options? opts = null) /// public IEnumerable Expand() { - SearchOption search = Opts.Recurse - ? SearchOption.AllDirectories - : SearchOption.TopDirectoryOnly; - - IEnumerable files = System.IO.Directory.EnumerateFiles(Directory, "*", search) + IEnumerable files = Collect(Directory, new HashSet(StringComparer.Ordinal)) .Where(p => _supportedExtensions.Contains(Path.GetExtension(p))); if (Opts.ExcludePending) @@ -91,6 +87,112 @@ public IEnumerable Expand() .Select(p => new FileSource(p)); } + /// + /// The recursive walk behind , carrying the REAL + /// (symlink-resolved) ancestor directories already on this walk branch. + /// + /// + /// + /// This replaces Directory.EnumerateFiles(dir, "*", SearchOption.AllDirectories), + /// which cannot express the contract. That overload follows directory symlinks — + /// which is correct and is the cross-port contract — but has no loop guard, and + /// its EnumerationOptions default of IgnoreInaccessible then SWALLOWS + /// the kernel's own ELOOP refusal. So a self-referential symlink did not hang and + /// did not throw: it completed normally, yielding the same real file ~40 times over + /// at ever-deeper phantom paths. Nothing downstream could recover from that, because + /// de-duplicates on the LEXICAL full + /// path and every phantom is lexically distinct — so each one was admitted as its own + /// source and the same metadata loaded once per level. TypeScript, Java and Python all + /// raise here; C# was the only port that reported success. + /// + /// + /// Paths are built by lexical join throughout, exactly as the old enumeration built + /// them — a symlinked directory's OWN name survives in the reported path; only the + /// WALK resolves the link. is extended only on the + /// recursive call and never mutated in place, so it describes the current branch + /// rather than siblings already visited at the same level: a directory legitimately + /// reachable through two different symlinks (a diamond, not a cycle) still resolves. + /// + /// + /// A directory symlink revisits a directory already on this branch. + private IEnumerable Collect(string directory, HashSet ancestors) + { + var real = RealPath(directory); + + if (ancestors.Contains(real)) + throw new IOException( + $"symlink loop detected while expanding metadata directory: {directory} revisits {real}"); + + var nextAncestors = new HashSet(ancestors, StringComparer.Ordinal) { real }; + + // Sorted so traversal is deterministic across filesystems, matching the + // full-path ordinal sort Expand() applies to the result. + var entries = System.IO.Directory.GetFileSystemEntries(directory); + Array.Sort(entries, StringComparer.Ordinal); + + foreach (var entry in entries) + { + // Directory.Exists follows symlinks, so a symlinked subdirectory is + // traversed rather than reported as a file — the behaviour the old + // AllDirectories enumeration had, and the cross-port contract. + if (System.IO.Directory.Exists(entry)) + { + if (!Opts.Recurse) continue; + foreach (var f in Collect(entry, nextAncestors)) yield return f; + } + else + { + yield return entry; + } + } + } + + /// + /// The fully symlink-resolved form of — .NET's + /// realpath(3) stand-in, since only + /// normalizes ./.. and resolves no links at all. + /// + /// + /// Resolution is component-by-component from the root because + /// Directory.ResolveLinkTarget canonicalizes ONLY the final segment. Using it + /// alone would leave a symlinked ANCESTOR unresolved, and the cycle guard would then + /// compare a half-resolved path against a real one and miss the loop — recursing + /// forever on exactly the input it exists to catch. Falls back to the lexical path if + /// the filesystem refuses an answer (the directory vanished mid-walk, or a permission + /// error): that is the enumeration's problem to report, not this guard's. + /// + private static string RealPath(string path) + { + var full = Path.GetFullPath(path); + var root = Path.GetPathRoot(full); + if (string.IsNullOrEmpty(root)) return full; + + var current = root; + foreach (var segment in full.Substring(root.Length) + .Split(new[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar }, + StringSplitOptions.RemoveEmptyEntries)) + { + current = Path.Combine(current, segment); + try + { + // returnFinalTarget walks a chain of links in one call; the bound is the + // OS's own, and a link cycle here surfaces as an IOException we fall back on. + var target = System.IO.Directory.ResolveLinkTarget(current, returnFinalTarget: true) + ?? System.IO.File.ResolveLinkTarget(current, returnFinalTarget: true); + if (target is not null) + { + var t = target.FullName; + current = Path.IsPathRooted(t) + ? Path.GetFullPath(t) + : Path.GetFullPath(Path.Combine(Path.GetDirectoryName(current) ?? root, t)); + } + } + catch (IOException) { /* unresolvable — keep the lexical form for this segment */ } + catch (UnauthorizedAccessException) { /* ditto */ } + } + return current; + } + /// True when any ancestor path component between and /// (i.e. excluding the file's own name) is /// exactly . diff --git a/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java b/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java index d24765fbf..c007adb93 100644 --- a/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java +++ b/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java @@ -44,6 +44,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; /** @@ -192,15 +193,25 @@ public void resolvesTheSameFileSet() throws IOException { } if (testCase.expectError() != null) { + JsonElement expected = testCase.expectError(); + boolean codePinned = expected.isJsonPrimitive() && expected.getAsJsonPrimitive().isString(); try { SourceResolver.resolveCollection(invokeDir); fail("expected " + testCase.expectError() + " for case " + testCase.name()); - } catch (MetaDataException e) { - JsonElement expected = testCase.expectError(); - // A string pins the exact code; `true` only pins that it raises — - // see the Case record's javadoc above. - if (expected.isJsonPrimitive() && expected.getAsJsonPrimitive().isString()) { - assertEquals(expected.getAsString(), e.getCode().orElseThrow().name()); + } catch (Exception e) { + // `true` pins only that it RAISES, so the catch is on Exception. + // Narrowing it to MetaDataException silently re-pinned the very thing + // the corpus refuses to pin: the symlink-cycle guard surfaces a + // FileSystemLoopException, which escaped this catch and failed the + // case even though the port behaved exactly as the contract requires. + // (`fail` above throws AssertionError, an Error — so it still escapes.) + // A string still pins the exact code, and that arm needs the coded type. + if (codePinned) { + assertTrue("case " + testCase.name() + " pins code " + expected.getAsString() + + " so it must raise MetaDataException, got " + e, + e instanceof MetaDataException); + assertEquals(expected.getAsString(), + ((MetaDataException) e).getCode().orElseThrow().name()); } } return; diff --git a/server/python/tests/conformance/test_source_resolution_conformance.py b/server/python/tests/conformance/test_source_resolution_conformance.py index ec5bee412..27a718bf2 100644 --- a/server/python/tests/conformance/test_source_resolution_conformance.py +++ b/server/python/tests/conformance/test_source_resolution_conformance.py @@ -68,13 +68,17 @@ def test_source_resolution_conformance(case: dict, tmp_path: Path) -> None: resolve_from = _materialize(case, tmp_path) if "expectError" in case: - with pytest.raises(ParseError) as e: + # `True` pins only that resolution RAISES, so the expected type is + # Exception. Catching ParseError instead silently re-pinned the very + # thing the corpus refuses to pin, and did: the symlink-cycle guard + # raises SymlinkLoopError, which this runner scored as a FAILURE even + # though the port behaved exactly as the contract requires. A string + # still pins the exact code, and that arm needs the coded type. + expected = case["expectError"] + with pytest.raises(ParseError if isinstance(expected, str) else Exception) as e: resolve_collection(resolve_from) - # A string pins the exact code; `True` only pins that resolution - # RAISES — the malformed-config error code is deliberately not - # pinned cross-port (see the corpus README). - if isinstance(case["expectError"], str): - assert e.value.code.value == case["expectError"] + if isinstance(expected, str): + assert e.value.code.value == expected return # `expectFiles` is project-root-relative even when `resolveFrom` points From 63a470b8726eafd27d5d3bbb17f64437f4b94034 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 22:30:42 -0400 Subject: [PATCH 3/5] docs(conformance): rule the unknown-config-key asymmetry INTENDED, and pin both halves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A genuinely unknown top-level key in `.metaobjects/config.json` throws in TypeScript and resolves fine in Java, C# and Python. The corpus README recorded this as a confirmed cross-port divergence and left it as "an open, human-reviewable follow-up". Reviewed: neither side moves. It is not a defect that leaked — it falls out of ownership. The file is TypeScript's, and TypeScript is the only port that models its whole vocabulary, so it is the only one that CAN tell a typo from a key a sibling owns. `.strict()` is what turns that knowledge into a diagnostic, and the hazard it catches is specific and already pinned: a stripped `scopes` (for `scope`) silently means "everything in scope", and a stripped `migrate.scopee` silently governs the whole database. The other three model the neutral subset, for which every other key is indistinguishable from a TS-owned one; they could imitate strictness only by carrying TypeScript's key list in lockstep, and would then REJECT a config a newer `meta` had just written. Tolerance is the only coherent behaviour for a partial reader, strictness the only coherent behaviour for the owner. So this stays off the shared corpus permanently rather than being deferred again. A shared case asserts ONE outcome and the correct outcome differs by port by design, so adding one could only be done by making some port wrong. Each half is now pinned where it belongs instead — TypeScript's in `config.test.ts` beside the `scopes`/`scopee` cases it shares a rationale with, and the tolerant half in the C#, Python and Java resolver tests, none of which previously covered it. The forward-compatibility cost is stated and accepted: a config written by a newer `meta` hard-fails an older one, which is what `schema_version` is for. Co-Authored-By: Claude Opus 5 (1M context) --- .../source-resolution-conformance/README.md | 35 ++++++++++--- .../SourceResolverTests.cs | 41 ++++++++++++++++ .../config/SourceResolverTest.java | 49 +++++++++++++++++++ .../tests/config/test_source_resolver.py | 24 +++++++++ .../packages/sdk/test/config.test.ts | 14 ++++++ 5 files changed, 155 insertions(+), 8 deletions(-) diff --git a/fixtures/source-resolution-conformance/README.md b/fixtures/source-resolution-conformance/README.md index c01469d27..c1cde7b81 100644 --- a/fixtures/source-resolution-conformance/README.md +++ b/fixtures/source-resolution-conformance/README.md @@ -145,14 +145,33 @@ README.md `.strict()` at the top level (`config.ts`) — so a key no version of TypeScript has ever declared throws a `ZodError` and resolution never reaches the source-listing step at all, while Java/C#/Python all resolve - successfully, silently ignoring it. Not added as a shared `expectFiles` - case here because doing so would need EITHER loosening `ConfigSchema`'s - top-level strictness (a reference-implementation behavior change with a - blast radius well beyond source resolution — every `loadConfig` caller, - not just this corpus) OR asserting a `true`-sentinel `expectError` that - TypeScript alone would satisfy, contradicting the other three ports' - actual success — neither of which this corpus is positioned to decide - unilaterally. Left as an open, human-reviewable follow-up. + successfully, silently ignoring it. + + **RULED: the asymmetry is INTENDED, and neither side moves.** It is not a + defect that leaked; it falls out of who owns the file. `.metaobjects/config.json` + is TypeScript's, and TypeScript is the only port that models its whole + vocabulary — so it is the only port that CAN tell a typo from a key a sibling + owns. `.strict()` is what converts that knowledge into a diagnostic, and the + hazard is specific and proven: a stripped `scopes` (for `scope`) silently means + "everything in scope", and a stripped `migrate.scopee` silently governs the whole + database. Java, C# and Python know only `schema_version` + `sources`; for them + every other key is indistinguishable from a TS-owned one, so strictness there + could only be imitated by embedding TypeScript's key list and keeping it in + lockstep forever — at which point any port a release behind would REJECT a + config a newer `meta` had just written. Tolerance is the only coherent behaviour + for a partial reader, and strictness the only coherent behaviour for the owner. + + So this stays OFF the shared corpus permanently — not deferred. A shared case + asserts ONE outcome, and the correct outcome here differs by port BY DESIGN; + adding one could only be done by making some port wrong. Each half is pinned in + the port that owns it instead: TypeScript's in `sdk/test/config.test.ts` + (unknown top-level key ⇒ throws, alongside the nested `scopes`/`scopee` cases), + and the tolerant half in `SourceResolverTests.cs`, + `tests/config/test_source_resolver.py` and `SourceResolverTest.java` + (unknown top-level key ⇒ resolves normally). The + forward-compatibility cost is real and accepted: a config written by a NEWER + `meta` hard-fails an OLDER one. That is what `schema_version` is for, and a loud + failure beats a `scope` that silently matched everything. ## Order is deliberately NOT pinned diff --git a/server/csharp/MetaObjects.Conformance.Tests/SourceResolverTests.cs b/server/csharp/MetaObjects.Conformance.Tests/SourceResolverTests.cs index 99b131623..19ccd427b 100644 --- a/server/csharp/MetaObjects.Conformance.Tests/SourceResolverTests.cs +++ b/server/csharp/MetaObjects.Conformance.Tests/SourceResolverTests.cs @@ -42,4 +42,45 @@ public void TwoUnresolvablePaths_ReportsTheContentFirstOne() Directory.Delete(root, recursive: true); } } + + /// + /// A genuinely unknown top-level key resolves normally here, and THROWS in + /// TypeScript. That asymmetry is ruled INTENDED in the source-resolution corpus + /// README: TypeScript owns this file and models its whole vocabulary, so only it + /// can tell a typo from a key a sibling owns. This port models the neutral subset + /// (`schema_version` + `sources`), for which every other key is indistinguishable + /// from a TypeScript-owned one — imitating strictness would mean carrying TS's key + /// list and rejecting a config a newer `meta` had just written. + /// + /// + /// Deliberately NOT a shared corpus case: a shared case asserts one outcome and + /// the correct outcome differs by port, so adding one could only be done by making + /// some port wrong. This is the tolerant half. + /// + [Fact] + public void AnUnknownTopLevelConfigKey_IsIgnored_NotRejected() + { + var root = Path.Combine(Path.GetTempPath(), "source-resolver-unknownkey-" + System.Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + try + { + var model = Path.Combine(root, "model"); + Directory.CreateDirectory(model); + File.WriteAllText(Path.Combine(model, "meta.a.json"), "{\"metadata.root\":{\"children\":[]}}"); + + var dotMo = Path.Combine(root, ".metaobjects"); + Directory.CreateDirectory(dotMo); + File.WriteAllText(Path.Combine(dotMo, "config.json"), + "{\"schema_version\":1,\"sources\":[{\"path\":\"model\"}],\"foo\":1}"); + + var resolved = SourceResolver.ResolveCollection(root); + + Assert.Single(resolved); + Assert.Equal(Path.Combine(model, "meta.a.json"), resolved[0]); + } + finally + { + Directory.Delete(root, recursive: true); + } + } } diff --git a/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolverTest.java b/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolverTest.java index c33d4f108..32ac1f9b8 100644 --- a/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolverTest.java +++ b/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolverTest.java @@ -21,10 +21,13 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Comparator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Stream; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @@ -65,4 +68,50 @@ public void twoUnresolvablePathsReportsTheContentFirstOne() throws IOException { Files.delete(root); } } + + /** + * The tolerant half of a ruled asymmetry. A genuinely unknown top-level key + * resolves normally here and THROWS in TypeScript. Intended, and ruled in the + * source-resolution corpus README: TypeScript owns this file and models its whole + * vocabulary, so only it can tell a typo from a key a sibling owns. This port + * models the neutral subset ({@code schema_version} + {@code sources}), for which + * every other key is indistinguishable from a TypeScript-owned one — imitating + * strictness would mean carrying TS's key list and rejecting a config a newer + * {@code meta} had just written. + * + *

Deliberately NOT a shared corpus case: a shared case asserts one outcome and + * the correct outcome differs by port, so adding one could only make some port wrong. + */ + @Test + public void anUnknownTopLevelConfigKeyIsIgnoredNotRejected() throws IOException { + Path root = Files.createTempDirectory("source-resolver-unknownkey-"); + try { + Path model = Files.createDirectory(root.resolve("model")); + Path file = model.resolve("meta.a.json"); + Files.writeString(file, "{\"metadata.root\":{\"children\":[]}}"); + + Path dotMo = Files.createDirectory(root.resolve(".metaobjects")); + Files.writeString(dotMo.resolve("config.json"), + "{\"schema_version\":1,\"sources\":[{\"path\":\"model\"}],\"foo\":1}"); + + List resolved = SourceResolver.resolveCollection(root); + + assertEquals(List.of(file.toAbsolutePath().normalize()), resolved); + } finally { + deleteRecursive(root); + } + } + + private static void deleteRecursive(Path dir) throws IOException { + if (!Files.exists(dir)) return; + try (Stream walk = Files.walk(dir)) { + walk.sorted(Comparator.reverseOrder()).forEach(p -> { + try { + Files.delete(p); + } catch (IOException ignored) { + // best-effort temp-dir cleanup + } + }); + } + } } diff --git a/server/python/tests/config/test_source_resolver.py b/server/python/tests/config/test_source_resolver.py index 5dfef894c..03ba41b12 100644 --- a/server/python/tests/config/test_source_resolver.py +++ b/server/python/tests/config/test_source_resolver.py @@ -92,6 +92,30 @@ def test_collection_with_no_config_and_no_default_raises(tmp_path: Path) -> None assert e.value.code == ErrorCode.ERR_COLLECTION_NOT_FOUND +def test_an_unknown_top_level_config_key_is_ignored_not_rejected(tmp_path: Path) -> None: + """The tolerant half of a ruled asymmetry. + + A genuinely unknown top-level key resolves normally here and THROWS in + TypeScript. Intended, and ruled in the source-resolution corpus README: + TypeScript owns this file and models its whole vocabulary, so only it can tell + a typo from a key a sibling owns. This port models the neutral subset + (``schema_version`` + ``sources``), for which every other key is + indistinguishable from a TypeScript-owned one — imitating strictness would mean + carrying TS's key list and rejecting a config a newer ``meta`` had just written. + + Deliberately NOT a shared corpus case: a shared case asserts one outcome and the + correct outcome differs by port, so adding one could only make some port wrong. + """ + (tmp_path / ".metaobjects").mkdir() + (tmp_path / ".metaobjects" / "config.json").write_text( + json.dumps({"schema_version": 1, "sources": [{"path": "model"}], "foo": 1}) + ) + (tmp_path / "model").mkdir() + (tmp_path / "model" / "meta.a.json").write_text("{}") + got = resolve_collection(tmp_path) + assert _rel(tmp_path, got) == {"model/meta.a.json"} + + def test_declared_sources_replace_the_default(tmp_path: Path) -> None: (tmp_path / ".metaobjects").mkdir() (tmp_path / ".metaobjects" / "config.json").write_text( diff --git a/server/typescript/packages/sdk/test/config.test.ts b/server/typescript/packages/sdk/test/config.test.ts index 1a17d27d5..98972872c 100644 --- a/server/typescript/packages/sdk/test/config.test.ts +++ b/server/typescript/packages/sdk/test/config.test.ts @@ -160,6 +160,20 @@ describe("ConfigSchema — phase-1 source resolution", () => { ConfigSchema.parse({ schema_version: 1, migrate: { d1: { bindingg: "DB" } } }), ).toThrow(); }); + test("rejects a genuinely unknown TOP-LEVEL key — TypeScript's half of a ruled asymmetry", () => { + // Java, C# and Python model only `schema_version` + `sources`, so they resolve + // this config successfully, ignoring `foo`. That divergence is INTENDED and is + // ruled in fixtures/source-resolution-conformance/README.md: TypeScript owns + // this file and is the only port that knows its whole vocabulary, so it is the + // only one that can tell a typo from a key a sibling owns. A partial reader + // could only imitate strictness by embedding TS's key list, and would then + // reject a config a newer `meta` had just written. + // + // Deliberately kept OFF the shared corpus: a shared case asserts one outcome, + // and the correct outcome here differs by port. This is TS's half; the tolerant + // half is pinned in each of the other three ports. + expect(() => ConfigSchema.parse({ schema_version: 1, foo: 1 })).toThrow(); + }); test("an existing config with no new keys still parses (back-compat)", () => { const p = ConfigSchema.parse({ schema_version: 1, pending_in_git: true, From 514f70721f9b6feee96dc38e6b442dfcf3db4a90 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Thu, 20 Aug 2026 08:41:56 -0400 Subject: [PATCH 4/5] =?UTF-8?q?fix:=20three=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20a=20too-broad=20widening,=20a=20case-folding=20hole?= =?UTF-8?q?,=20a=20tense?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the preceding two commits found three defects in them. 1. The `expectError: true` widening was too broad, and weakened six cases to buy one. `true` had a stronger meaning than the README's wording admitted, and all three coded ports implemented it identically: "raises the port's coded metadata error; WHICH code is unpinned". The type was pinned. Relaxing the whole arm to bare Exception/`Throwable` so the symlink cycle could raise a native IOException also relaxed `sources-must-be-an-array-not-an-object` and its five siblings, which would now PASS on a raw NullReferenceException / NPE / TypeError — a crash scoring as a correct rejection. The allowance is now scoped to the single case that needs it via a new optional `errorIsNative`, the coded type is required everywhere else, and the README says which of the two `true` pins (the type) and which it does not (the code). TypeScript is unaffected and stays loose: it propagates the raw parser error, which is why the code is unpinned in the first place. 2. The cycle guard's ancestor set compared `StringComparer.Ordinal` while `RealPath` necessarily mixes spellings — a non-link segment keeps the caller's casing via `GetFullPath`, a resolved link comes back in the filesystem's canonical casing. On a case-insensitive volume `MODEL/loop -> model` therefore walked straight past the guard, back down to the kernel ELOOP floor that the new immediacy test claims to have removed — and this is the one port whose enumeration SWALLOWS ELOOP, so there is no backstop under it. Now Ordinal on Linux (where `Model` and `model` are genuinely different directories and folding would reject a valid tree) and case-insensitive elsewhere. 3. `--print-only` suppressed the writes but not the past-tense reporting, so a dry run announced `wired @.metaobjects/AGENTS.md into CLAUDE.md` and `refreshed version written to .new` for edits it had not made. Worse than the silent write it replaced: it names a side effect on a file the user owns, which they can go look for and will not find. All three messages are future-tense under a dry run, pinned by a test asserting no past-tense form survives. That test caught its own fix at first: `\bcreated with\b` also matches the CORRECT "(would be created with …)". Anchored. Co-Authored-By: Claude Opus 5 (1M context) --- .../source-resolution-conformance/README.md | 26 ++++++++--- .../source-resolution-conformance/cases.json | 3 +- .../SourceResolutionConformanceTests.cs | 39 ++++++++++------- .../MetaObjects/Loader/DirectorySource.cs | 22 +++++++++- .../SourceResolutionConformanceTest.java | 43 ++++++++++++------- .../test_source_resolution_conformance.py | 16 ++++--- .../packages/cli/src/commands/init.ts | 24 +++++++++-- .../cli/test/unit/init-docs-only.test.ts | 12 ++++++ .../source-resolution-conformance.test.ts | 6 +++ 9 files changed, 143 insertions(+), 48 deletions(-) diff --git a/fixtures/source-resolution-conformance/README.md b/fixtures/source-resolution-conformance/README.md index c1cde7b81..b911f36bd 100644 --- a/fixtures/source-resolution-conformance/README.md +++ b/fixtures/source-resolution-conformance/README.md @@ -11,7 +11,7 @@ are read, `scope` filters what is emitted from them. ## Shape ``` -cases.json # { cases: [{ name, tree, symlinks?, config, resolveFrom?, expectFiles?, expectError? }] } +cases.json # { cases: [{ name, tree, symlinks?, config, resolveFrom?, expectFiles?, expectError?, errorIsNative? }] } README.md ``` @@ -78,10 +78,26 @@ README.md `resolveFrom`), compared as an **UNORDERED SET**. See "Order is deliberately not pinned" below. - **`expectError`** — either a STRING error code the resolution must fail with - exactly, or the literal `true` meaning "must raise, but which error/code is - deliberately not pinned across ports" (see "Also deliberately NOT pinned: - the malformed-config error code" below for why the latter form exists). - Exactly one of `expectFiles` / `expectError` is present per case. + exactly, or the literal `true` meaning "must raise the port's own coded + metadata error, but WHICH code is deliberately not pinned across ports" (see + "Also deliberately NOT pinned: the malformed-config error code" below for why + the latter form exists). Exactly one of `expectFiles` / `expectError` is + present per case. Note what `true` still pins in a port that HAS a coded error + type: the type. Java, C# and Python all require `MetaDataException` / + `MetaModelException` / `ParseError` on this arm, so a malformed config that + crashes with a raw `NullPointerException` fails the case rather than passing + it. (TypeScript alone cannot: it propagates the raw parser error, which is the + whole reason the code is unpinned.) +- **`errorIsNative`** — OPTIONAL boolean, only meaningful beside + `expectError: true`. Means the failure surfaces as a PLATFORM-native error + rather than the port's coded metadata error, so the type requirement above is + lifted for this case alone. Exactly one case sets it: + `a-symlink-cycle-is-an-error`, where the raise comes from the filesystem walk + (`IOException` in C#, `FileSystemLoopException` in Java, `SymlinkLoopError` in + Python) and never passes through a coded-error constructor. It exists so that + admitting that one case did not silently relax the other six `true` cases from + "a coded parse failure" to "anything at all" — which is precisely what a blanket + widening of the runners does, and did. ## Semantics pinned here diff --git a/fixtures/source-resolution-conformance/cases.json b/fixtures/source-resolution-conformance/cases.json index 5b5fc924c..e367db2f7 100644 --- a/fixtures/source-resolution-conformance/cases.json +++ b/fixtures/source-resolution-conformance/cases.json @@ -272,7 +272,8 @@ }, "symlinks": { "model/loop": "model" }, "config": { "schema_version": 1, "sources": [{ "path": "model" }] }, - "expectError": true + "expectError": true, + "errorIsNative": true } ] } diff --git a/server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs b/server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs index 2718dac79..9973a9adf 100644 --- a/server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs +++ b/server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs @@ -27,14 +27,20 @@ private sealed record Case( // count silently goes stale; the structural description above does not.) string ResolveFrom, string[]? ExpectFiles, - // A JSON string pins the exact error code raised; JSON `true` pins only - // that resolution RAISES — the malformed-config error code is - // deliberately not pinned cross-port (see the corpus README). + // A JSON string pins the exact error code raised; JSON `true` leaves the + // CODE unpinned but still requires the port's coded MetaModelException — + // the malformed-config error code is deliberately not pinned cross-port + // (see the corpus README), the TYPE is. JsonElement? ExpectError, // Optional: linkPath -> targetPath, both project-root-relative, materialized // AFTER `Tree` (I1 — a symlinked source root, or a symlinked subdirectory // inside a walked tree). - Dictionary Symlinks); + Dictionary Symlinks, + // Optional: this case's failure surfaces as a PLATFORM-native error rather + // than a coded MetaModelException, so the type requirement above is lifted + // for it alone (the symlink-cycle case, whose raise comes from the + // filesystem walk and never reaches a coded-error constructor). + bool ErrorIsNative); public static TheoryData CaseNames() { @@ -78,7 +84,8 @@ private static List LoadCases() foreach (var p in sl.EnumerateObject()) symlinks[p.Name] = p.Value.GetString()!; } - cases.Add(new Case(el.GetProperty("name").GetString()!, tree, cfg, resolveFrom, expectFiles, expectError, symlinks)); + bool errorIsNative = el.TryGetProperty("errorIsNative", out var ein) && ein.GetBoolean(); + cases.Add(new Case(el.GetProperty("name").GetString()!, tree, cfg, resolveFrom, expectFiles, expectError, symlinks, errorIsNative)); } return cases; } @@ -127,19 +134,21 @@ public void ResolvesTheSameFileSet(string name) if (c.ExpectError is not null) { - // `true` pins only that resolution RAISES — deliberately not which type, - // so the assertion is on Exception. Narrowing it to MetaModelException - // would silently re-pin the very thing the corpus refuses to pin, and did: - // the symlink-cycle guard raises IOException (the natural type, and the - // one Java's FileSystemLoopException also derives from), which this - // runner scored as a FAILURE even though the port behaved correctly. - // A string still pins the exact code, and that arm requires the richer - // type — see the ExpectError field doc above. + // A string pins the exact code. `true` leaves the CODE unpinned but + // still requires MetaModelException — a malformed config that crashes + // with a raw NullReferenceException must FAIL this case, not pass it. + // `ErrorIsNative` lifts only the type requirement, for the one case + // whose raise comes from the filesystem walk (IOException) and never + // reaches a coded-error constructor; widening the whole arm instead + // would quietly relax the six malformed-config cases to "anything". var ex = Assert.ThrowsAny(() => SourceResolver.ResolveCollection(invokeDir)); - if (c.ExpectError.Value.ValueKind == JsonValueKind.String) + if (!c.ErrorIsNative) { var coded = Assert.IsAssignableFrom(ex); - Assert.Equal(c.ExpectError.Value.GetString(), coded.Code.ToString()); + if (c.ExpectError.Value.ValueKind == JsonValueKind.String) + { + Assert.Equal(c.ExpectError.Value.GetString(), coded.Code.ToString()); + } } return; } diff --git a/server/csharp/MetaObjects/Loader/DirectorySource.cs b/server/csharp/MetaObjects/Loader/DirectorySource.cs index 70e049838..75cd79392 100644 --- a/server/csharp/MetaObjects/Loader/DirectorySource.cs +++ b/server/csharp/MetaObjects/Loader/DirectorySource.cs @@ -63,9 +63,27 @@ public DirectorySource(string directory, Options? opts = null) /// by full path (ordinal). The sort happens on the full path so that nested /// directory traversal is also deterministic. /// + ///

+ /// How two resolved directory paths are compared when deciding "is this an + /// ancestor I have already walked". + /// + /// + /// Ordinal on Linux, where the filesystem genuinely distinguishes Model from + /// model and case-folding would REJECT a valid tree. Case-insensitive + /// elsewhere, because necessarily mixes spellings — a + /// non-link segment keeps whatever casing the caller wrote (via + /// ) while a resolved link comes back in the + /// filesystem's own canonical casing — so on a case-insensitive volume an ordinal + /// compare lets MODEL/loop -> model slip past the guard. Slipping past is not + /// benign here: this is the one port whose enumeration SWALLOWS the kernel's ELOOP, + /// so there is no backstop underneath it. + /// + private static readonly StringComparer PathComparer = + OperatingSystem.IsLinux() ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase; + public IEnumerable Expand() { - IEnumerable files = Collect(Directory, new HashSet(StringComparer.Ordinal)) + IEnumerable files = Collect(Directory, new HashSet(PathComparer)) .Where(p => _supportedExtensions.Contains(Path.GetExtension(p))); if (Opts.ExcludePending) @@ -123,7 +141,7 @@ private IEnumerable Collect(string directory, HashSet ancestors) throw new IOException( $"symlink loop detected while expanding metadata directory: {directory} revisits {real}"); - var nextAncestors = new HashSet(ancestors, StringComparer.Ordinal) { real }; + var nextAncestors = new HashSet(ancestors, PathComparer) { real }; // Sorted so traversal is deterministic across filesystems, matching the // full-path ordinal sort Expand() applies to the result. diff --git a/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java b/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java index c007adb93..ceef14eb5 100644 --- a/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java +++ b/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java @@ -73,12 +73,18 @@ public class SourceResolutionConformanceTest { */ /** * {@code expectError}: a JSON string pins the exact error code raised; JSON - * {@code true} pins only that resolution RAISES — the malformed-config error - * code is deliberately not pinned cross-port (see the corpus README). + * {@code true} leaves the CODE unpinned but still requires the port's coded + * {@link MetaDataException} — the malformed-config error code is deliberately + * not pinned cross-port (see the corpus README), the TYPE is. + * + *

{@code errorIsNative}: this case's failure surfaces as a PLATFORM-native + * error rather than a coded exception, lifting the type requirement for it + * alone — the symlink-cycle case, whose raise comes from the filesystem walk + * ({@code FileSystemLoopException}) and never reaches a coded-error constructor. */ private record Case(String name, Map tree, JsonObject config, String resolveFrom, List expectFiles, JsonElement expectError, - Map symlinks) {} + Map symlinks, boolean errorIsNative) {} /** Package-private (not private): shared with {@link SourceResolutionCorpusNotEmptyTest}, * which needs to locate the same committed corpus file without a second definition @@ -143,7 +149,9 @@ public static Collection cases() throws IOException { } } - rows.add(new Object[]{name, new Case(name, tree, config, resolveFrom, expectFiles, expectError, symlinks)}); + boolean errorIsNative = c.has("errorIsNative") && c.get("errorIsNative").getAsBoolean(); + + rows.add(new Object[]{name, new Case(name, tree, config, resolveFrom, expectFiles, expectError, symlinks, errorIsNative)}); } return rows; } @@ -199,19 +207,24 @@ public void resolvesTheSameFileSet() throws IOException { SourceResolver.resolveCollection(invokeDir); fail("expected " + testCase.expectError() + " for case " + testCase.name()); } catch (Exception e) { - // `true` pins only that it RAISES, so the catch is on Exception. - // Narrowing it to MetaDataException silently re-pinned the very thing - // the corpus refuses to pin: the symlink-cycle guard surfaces a - // FileSystemLoopException, which escaped this catch and failed the - // case even though the port behaved exactly as the contract requires. + // The catch is on Exception so a native raise can reach the body at + // all; the TYPE requirement is asserted here rather than by the catch + // clause. `true` leaves the CODE unpinned but still demands + // MetaDataException — a malformed config that crashes with a raw NPE + // must FAIL this case, not pass it. `errorIsNative` lifts that for the + // one case whose raise comes from the filesystem walk + // (FileSystemLoopException) and never reaches a coded-error + // constructor; widening the whole arm instead would quietly relax the + // six malformed-config cases to "anything at all". // (`fail` above throws AssertionError, an Error — so it still escapes.) - // A string still pins the exact code, and that arm needs the coded type. - if (codePinned) { - assertTrue("case " + testCase.name() + " pins code " + expected.getAsString() - + " so it must raise MetaDataException, got " + e, + if (!testCase.errorIsNative()) { + assertTrue("case " + testCase.name() + + " must raise MetaDataException, got " + e, e instanceof MetaDataException); - assertEquals(expected.getAsString(), - ((MetaDataException) e).getCode().orElseThrow().name()); + if (codePinned) { + assertEquals(expected.getAsString(), + ((MetaDataException) e).getCode().orElseThrow().name()); + } } } return; diff --git a/server/python/tests/conformance/test_source_resolution_conformance.py b/server/python/tests/conformance/test_source_resolution_conformance.py index 27a718bf2..da5d71725 100644 --- a/server/python/tests/conformance/test_source_resolution_conformance.py +++ b/server/python/tests/conformance/test_source_resolution_conformance.py @@ -68,14 +68,16 @@ def test_source_resolution_conformance(case: dict, tmp_path: Path) -> None: resolve_from = _materialize(case, tmp_path) if "expectError" in case: - # `True` pins only that resolution RAISES, so the expected type is - # Exception. Catching ParseError instead silently re-pinned the very - # thing the corpus refuses to pin, and did: the symlink-cycle guard - # raises SymlinkLoopError, which this runner scored as a FAILURE even - # though the port behaved exactly as the contract requires. A string - # still pins the exact code, and that arm needs the coded type. + # A string pins the exact code. `True` leaves the CODE unpinned but still + # requires ParseError — a malformed config that crashes with a raw + # TypeError must fail this case, not pass it. `errorIsNative` lifts only + # the type requirement, for the one case whose raise comes from the + # filesystem walk (SymlinkLoopError) and never reaches a coded-error + # constructor; widening the whole arm instead would quietly relax the six + # malformed-config cases from "a coded parse failure" to "anything". expected = case["expectError"] - with pytest.raises(ParseError if isinstance(expected, str) else Exception) as e: + expected_type = Exception if case.get("errorIsNative") else ParseError + with pytest.raises(expected_type) as e: resolve_collection(resolve_from) if isinstance(expected, str): assert e.value.code.value == expected diff --git a/server/typescript/packages/cli/src/commands/init.ts b/server/typescript/packages/cli/src/commands/init.ts index c401ffa31..2112ce535 100644 --- a/server/typescript/packages/cli/src/commands/init.ts +++ b/server/typescript/packages/cli/src/commands/init.ts @@ -269,7 +269,14 @@ async function writeAgentContext(opts: InitOptions, result: InitResult): Promise await writeFile(abs, c.contents, "utf8"); } result.created.push(c.newPath); - result.warnings.push(`${c.path} appears hand-edited; refreshed version written to ${c.newPath}`); + // Past tense only when it actually happened — a dry run that reports "written + // to .new" is claiming an edit-preserving side effect the user can go + // look for and will not find. + result.warnings.push( + dryRun + ? `${c.path} appears hand-edited; refreshed version would be written to ${c.newPath}` + : `${c.path} appears hand-edited; refreshed version written to ${c.newPath}`, + ); } if (!dryRun) { const manifestAbs = join(opts.cwd, AGENT_CONTEXT_MANIFEST_PATH); @@ -295,7 +302,11 @@ async function wireRootMemory(cwd: string, result: InitResult, dryRun = false): // If neither root memory file exists, create CLAUDE.md (Claude Code's canonical) with the import. if (!claudeExists && !agentsExists) { if (!dryRun) await writeFile(claudePath, `# Project memory\n\n${ROOT_IMPORT_LINE}\n`, "utf8"); - result.created.push("CLAUDE.md (created with MetaObjects @import)"); + result.created.push( + dryRun + ? "CLAUDE.md (would be created with MetaObjects @import)" + : "CLAUDE.md (created with MetaObjects @import)", + ); return; } // Otherwise append the import to whichever exist (idempotent — never double-add). @@ -304,7 +315,14 @@ async function wireRootMemory(cwd: string, result: InitResult, dryRun = false): const body = await readFile(path, "utf8"); if (body.includes(ROOT_IMPORT_LINE)) continue; if (!dryRun) await writeFile(path, `${body.replace(/\n*$/, "\n")}\n${ROOT_IMPORT_LINE}\n`, "utf8"); - result.warnings.push(`wired ${ROOT_IMPORT_LINE} into ${path.endsWith("AGENTS.md") ? "AGENTS.md" : "CLAUDE.md"} so the MetaObjects context loads`); + // Past tense only when it actually happened — this one mutates a file the user + // owns, so a dry run reporting it as done is the most misleading of the three. + const target = path.endsWith("AGENTS.md") ? "AGENTS.md" : "CLAUDE.md"; + result.warnings.push( + dryRun + ? `would wire ${ROOT_IMPORT_LINE} into ${target} so the MetaObjects context loads` + : `wired ${ROOT_IMPORT_LINE} into ${target} so the MetaObjects context loads`, + ); } } diff --git a/server/typescript/packages/cli/test/unit/init-docs-only.test.ts b/server/typescript/packages/cli/test/unit/init-docs-only.test.ts index 23d914ec5..9e7d404a6 100644 --- a/server/typescript/packages/cli/test/unit/init-docs-only.test.ts +++ b/server/typescript/packages/cli/test/unit/init-docs-only.test.ts @@ -49,6 +49,18 @@ describe("init() --docs-only", () => { // ...and the directory is untouched. expect(readdirSync(cwd)).toEqual([]); + + // Nothing reports in the PAST tense. A dry run claiming it "created" a root + // CLAUDE.md or "wired" an @import names a side effect on a file the user owns, + // which they can go look for and will not find — a more expensive lie than the + // silent write, because it reads as a completed action. + // Anchored, not `\bcreated with\b` — that also matches the CORRECT "(would be + // created with …)" and the assertion fails on its own fix. + const past = [...planned.created, ...planned.warnings].filter( + (m) => m.includes("(created with") || /^wired /.test(m) || m.includes("version written to"), + ); + expect(past).toEqual([]); + expect(planned.created).toContain("CLAUDE.md (would be created with MetaObjects @import)"); }); test("--refresh-docs --print-only writes nothing", async () => { diff --git a/server/typescript/packages/sdk/test/source-resolution-conformance.test.ts b/server/typescript/packages/sdk/test/source-resolution-conformance.test.ts index f9f387a93..d1c61d17b 100644 --- a/server/typescript/packages/sdk/test/source-resolution-conformance.test.ts +++ b/server/typescript/packages/sdk/test/source-resolution-conformance.test.ts @@ -19,6 +19,12 @@ interface Case { * resolution RAISES — the malformed-config error code is deliberately not * pinned cross-port (see the corpus README). */ readonly expectError?: string | true; + /** Optional: the failure surfaces as a PLATFORM-native error rather than a coded + * one, lifting the coded-TYPE requirement for this case. Java/C#/Python honour + * it; TypeScript has nothing to lift — it propagates the raw parser error, which + * is why the code is unpinned here in the first place — so it is unread. Declared + * anyway so this interface stays a faithful mirror of the case schema. */ + readonly errorIsNative?: boolean; /** Optional: linkPath -> targetPath, both project-root-relative, materialized * AFTER `tree` (I1 — a symlinked source root, or a symlinked subdirectory * inside a walked tree). */ From f3ba96504ba848ae1e5d0c01677e4173882cbb54 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Thu, 20 Aug 2026 09:24:53 -0400 Subject: [PATCH 5/5] refactor: simplify the cycle guard's path resolution and the dry-run write sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-review simplification pass. No behaviour change; all five constraints the review pass established are intact and still proven by their tests. `DirectorySource.RealPath` drops the `File.ResolveLinkTarget` fallback and the manual rooted/relative branch. Both were dead code, verified rather than assumed: `Directory.ResolveLinkTarget(path, returnFinalTarget: true).FullName` already returns a fully-qualified, symlink-resolved absolute path for a relative one-hop target, a relative chain, an absolute target and a `.` self-link — checked with the process CWD set to `/`, which is exactly where a relative target resolved against the wrong base would have shown itself. Since `Collect` only ever calls `RealPath` on a path it has already confirmed is a directory, `File.`- and `Directory.ResolveLinkTarget` could never disagree either. Thirteen lines become two; the component-by-component LOOP is untouched, which is the part that carries the correctness (resolving only the final segment misses a cycle reached through a symlinked ancestor). Also fixes a doc-comment bug introduced by the previous commit: the new `PathComparer` block was inserted between `Expand()`'s existing `

` and `Expand()` itself, so two adjacent `///` blocks merged into one and XML-doc tooling attached the pair to `PathComparer`, orphaning `Expand()`'s own summary. `init.ts` factors the four-times-repeated `if (!dryRun) { mkdir; writeFile }` into `writeUnlessDryRun`, and the three past/future tense ternaries into `verbed(dryRun, pastParticiple)` — each call site now states only its own past participle rather than spelling out both tenses of a whole sentence. This normalises one message from active "would wire" to passive "would be wired", to match the two that already read that way; the non-dry-run wording is unchanged, which is what the tests pin. Co-Authored-By: Claude Opus 5 (1M context) --- .../MetaObjects/Loader/DirectorySource.cs | 30 +++++----- .../packages/cli/src/commands/init.ts | 60 +++++++++---------- 2 files changed, 43 insertions(+), 47 deletions(-) diff --git a/server/csharp/MetaObjects/Loader/DirectorySource.cs b/server/csharp/MetaObjects/Loader/DirectorySource.cs index 75cd79392..496200f11 100644 --- a/server/csharp/MetaObjects/Loader/DirectorySource.cs +++ b/server/csharp/MetaObjects/Loader/DirectorySource.cs @@ -58,11 +58,6 @@ public DirectorySource(string directory, Options? opts = null) Opts = opts ?? new Options(); } - /// - /// Enumerate the matched files as instances, sorted - /// by full path (ordinal). The sort happens on the full path so that nested - /// directory traversal is also deterministic. - /// /// /// How two resolved directory paths are compared when deciding "is this an /// ancestor I have already walked". @@ -81,6 +76,11 @@ public DirectorySource(string directory, Options? opts = null) private static readonly StringComparer PathComparer = OperatingSystem.IsLinux() ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase; + /// + /// Enumerate the matched files as instances, sorted + /// by full path (ordinal). The sort happens on the full path so that nested + /// directory traversal is also deterministic. + /// public IEnumerable Expand() { IEnumerable files = Collect(Directory, new HashSet(PathComparer)) @@ -193,17 +193,15 @@ private static string RealPath(string path) current = Path.Combine(current, segment); try { - // returnFinalTarget walks a chain of links in one call; the bound is the - // OS's own, and a link cycle here surfaces as an IOException we fall back on. - var target = System.IO.Directory.ResolveLinkTarget(current, returnFinalTarget: true) - ?? System.IO.File.ResolveLinkTarget(current, returnFinalTarget: true); - if (target is not null) - { - var t = target.FullName; - current = Path.IsPathRooted(t) - ? Path.GetFullPath(t) - : Path.GetFullPath(Path.Combine(Path.GetDirectoryName(current) ?? root, t)); - } + // returnFinalTarget walks a chain of links (relative or absolute + // targets, either) in one call and hands back an already + // fully-qualified target. Every path this loop resolves is a + // directory — Collect only ever calls RealPath on one it has + // already confirmed exists — so Directory.ResolveLinkTarget alone + // is authoritative; a link cycle here surfaces as an IOException + // we fall back on. + var target = System.IO.Directory.ResolveLinkTarget(current, returnFinalTarget: true); + if (target is not null) current = target.FullName; } catch (IOException) { /* unresolvable — keep the lexical form for this segment */ } catch (UnauthorizedAccessException) { /* ditto */ } diff --git a/server/typescript/packages/cli/src/commands/init.ts b/server/typescript/packages/cli/src/commands/init.ts index 2112ce535..9f0f2c5c3 100644 --- a/server/typescript/packages/cli/src/commands/init.ts +++ b/server/typescript/packages/cli/src/commands/init.ts @@ -218,6 +218,23 @@ async function stackForAgentContext(opts: InitOptions, prior: Manifest | undefin return resolveStack(opts.cwd, overrides); } +/** Writes `contents` to `path` (relative to `cwd`), unless `dryRun` — in which case + * the write is skipped entirely and the caller still records what WOULD have + * landed. Factors the mkdir+writeFile pair shared by every write site below. */ +async function writeUnlessDryRun(cwd: string, dryRun: boolean, path: string, contents: string): Promise { + if (dryRun) return; + const abs = join(cwd, path); + await mkdir(dirname(abs), { recursive: true }); + await writeFile(abs, contents, "utf8"); +} + +/** "would be VERBED" during a dry run, plain VERBED otherwise — the one tense + * marker every reported write shares, so each call site states only its own + * past participle instead of writing out both tenses of the whole sentence. */ +function verbed(dryRun: boolean, pastParticiple: string): string { + return dryRun ? `would be ${pastParticiple}` : pastParticiple; +} + async function writeAgentContext(opts: InitOptions, result: InitResult): Promise { warnIfMonorepoSubdir(opts, result); const prior = await readManifest(opts.cwd); @@ -255,34 +272,23 @@ async function writeAgentContext(opts: InitOptions, result: InitResult): Promise const dryRun = opts.printOnly === true; for (const w of writes) { - if (!dryRun) { - const abs = join(opts.cwd, w.path); - await mkdir(dirname(abs), { recursive: true }); - await writeFile(abs, w.contents, "utf8"); - } + await writeUnlessDryRun(opts.cwd, dryRun, w.path, w.contents); result.created.push(w.path); } for (const c of conflicts) { - if (!dryRun) { - const abs = join(opts.cwd, c.newPath); - await mkdir(dirname(abs), { recursive: true }); - await writeFile(abs, c.contents, "utf8"); - } + await writeUnlessDryRun(opts.cwd, dryRun, c.newPath, c.contents); result.created.push(c.newPath); // Past tense only when it actually happened — a dry run that reports "written // to .new" is claiming an edit-preserving side effect the user can go // look for and will not find. result.warnings.push( - dryRun - ? `${c.path} appears hand-edited; refreshed version would be written to ${c.newPath}` - : `${c.path} appears hand-edited; refreshed version written to ${c.newPath}`, + `${c.path} appears hand-edited; refreshed version ${verbed(dryRun, "written")} to ${c.newPath}`, ); } - if (!dryRun) { - const manifestAbs = join(opts.cwd, AGENT_CONTEXT_MANIFEST_PATH); - await mkdir(dirname(manifestAbs), { recursive: true }); - await writeFile(manifestAbs, JSON.stringify(decision.manifest, null, 2) + "\n", "utf8"); - } + await writeUnlessDryRun( + opts.cwd, dryRun, AGENT_CONTEXT_MANIFEST_PATH, + JSON.stringify(decision.manifest, null, 2) + "\n", + ); result.created.push(AGENT_CONTEXT_MANIFEST_PATH); for (const orphan of decision.removed) { @@ -301,12 +307,8 @@ async function wireRootMemory(cwd: string, result: InitResult, dryRun = false): // If neither root memory file exists, create CLAUDE.md (Claude Code's canonical) with the import. if (!claudeExists && !agentsExists) { - if (!dryRun) await writeFile(claudePath, `# Project memory\n\n${ROOT_IMPORT_LINE}\n`, "utf8"); - result.created.push( - dryRun - ? "CLAUDE.md (would be created with MetaObjects @import)" - : "CLAUDE.md (created with MetaObjects @import)", - ); + await writeUnlessDryRun(cwd, dryRun, "CLAUDE.md", `# Project memory\n\n${ROOT_IMPORT_LINE}\n`); + result.created.push(`CLAUDE.md (${verbed(dryRun, "created")} with MetaObjects @import)`); return; } // Otherwise append the import to whichever exist (idempotent — never double-add). @@ -314,15 +316,11 @@ async function wireRootMemory(cwd: string, result: InitResult, dryRun = false): if (!exists) continue; const body = await readFile(path, "utf8"); if (body.includes(ROOT_IMPORT_LINE)) continue; - if (!dryRun) await writeFile(path, `${body.replace(/\n*$/, "\n")}\n${ROOT_IMPORT_LINE}\n`, "utf8"); + const target = path.endsWith("AGENTS.md") ? "AGENTS.md" : "CLAUDE.md"; + await writeUnlessDryRun(cwd, dryRun, target, `${body.replace(/\n*$/, "\n")}\n${ROOT_IMPORT_LINE}\n`); // Past tense only when it actually happened — this one mutates a file the user // owns, so a dry run reporting it as done is the most misleading of the three. - const target = path.endsWith("AGENTS.md") ? "AGENTS.md" : "CLAUDE.md"; - result.warnings.push( - dryRun - ? `would wire ${ROOT_IMPORT_LINE} into ${target} so the MetaObjects context loads` - : `wired ${ROOT_IMPORT_LINE} into ${target} so the MetaObjects context loads`, - ); + result.warnings.push(`${verbed(dryRun, "wired")} ${ROOT_IMPORT_LINE} into ${target} so the MetaObjects context loads`); } }