From df14bcef9344808542ad682979ac116f4723631c Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 20:57:49 -0400 Subject: [PATCH 1/6] fix(python): reject boolean schema_version, catch expand()'s OSError, thread config providers/libraries into docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 — `schema_version: true` was accepted: Python's `bool` subclasses `int` and `True == 1`, so `version != 1` silently passed a JSON boolean where Java's `isNumber()` and C#'s `ValueKind != Number` both reject it. Booleans are now excluded explicitly before the numeric-equality check. F2 (regression from our own symlink-following fix) — `DirectorySource.expand()` moved from `rglob` to a manual `iterdir()` walk so it could follow a symlinked subdirectory, but `iterdir()` raises OSError (FileNotFoundError / NotADirectoryError, or SymlinkLoopError on a cycle) when the top-level directory itself can't be walked. Nothing on the `_load_root` path caught it, so `metaobjects gen /does/not/exist --out out` died with a raw traceback. `_load_root` now catches OSError and reports it through the same coded "error: failed to load metadata" convention every other load failure uses — mirroring the TypeScript reference, which wraps the equivalent `readdir` failure in a clean Error at the same boundary. F4 — `docs`'s no-positional branch loaded `metaobjects.config.yaml` only to read its `metadata` key, then reloaded via `_load_root_from_paths` with neither the config's own `providers` nor its `libraries` — so a project declaring `libraries: ["ai"]` failed `docs` with `ERR_UNRESOLVED_SUPER` on metadata `gen` loads cleanly. `MetaDataLoader.from_uris` and `_load_root_from_paths` both gained the same `libraries` prepend `from_directory` already has, and `_cmd_docs` now merges the config's declared providers alongside the CLI's `--provider` set and threads `config.libraries` through. Each fix was proven non-vacuous: a test was added first, run to confirm it failed against the unpatched code (RED), then the fix applied and the test re-run (GREEN). --- server/python/src/metaobjects/cli.py | 66 ++++++++++++++----- .../src/metaobjects/config/neutral_config.py | 7 +- .../metaobjects/loader/meta_data_loader.py | 14 +++- .../tests/codegen/test_cli_config_gen.py | 24 +++++++ .../tests/config/test_neutral_config.py | 11 ++++ .../test_source_resolution_conformance.py | 43 ++++++++++++ 6 files changed, 146 insertions(+), 19 deletions(-) diff --git a/server/python/src/metaobjects/cli.py b/server/python/src/metaobjects/cli.py index 1a337d0eb..655a62747 100644 --- a/server/python/src/metaobjects/cli.py +++ b/server/python/src/metaobjects/cli.py @@ -256,19 +256,32 @@ def _load_root( registered ``trace-helper`` generator exists to consume — the generator was reachable from the CLI while its input was not. """ - if providers: - from metaobjects.core_types import core_providers - - result = MetaDataLoader.from_directory( - metadata_dir, - providers=[*core_providers, *providers], - strict=strict, - libraries=libraries, - ) - else: - result = MetaDataLoader.from_directory( - metadata_dir, strict=strict, libraries=libraries - ) + try: + if providers: + from metaobjects.core_types import core_providers + + result = MetaDataLoader.from_directory( + metadata_dir, + providers=[*core_providers, *providers], + strict=strict, + libraries=libraries, + ) + else: + result = MetaDataLoader.from_directory( + metadata_dir, strict=strict, libraries=libraries + ) + except OSError as e: + # `DirectorySource.expand()` walks via `iterdir()` (not `rglob()`, so it + # can follow a symlinked subdirectory — the I1 fix) and raises a plain + # OSError (FileNotFoundError / NotADirectoryError, or `SymlinkLoopError` + # on a symlink cycle) when `metadata_dir` itself can't be walked. Nothing + # upstream of this call catches it, so it would otherwise surface as a + # raw Python traceback instead of the coded "error: failed to load + # metadata" every other `_load_root` failure prints. Mirrors the + # TypeScript reference, which wraps the equivalent `readdir` failure in + # a clean `Error` at the same boundary (`metadata-files.ts`, + # `listMetadataFiles`: "cannot read metadata directory ..."). + return None, [f"cannot read metadata directory {metadata_dir}: {e}"] if result.errors: msgs = [f"{e.code}: {e.message}" for e in result.errors] return None, msgs @@ -279,6 +292,7 @@ def _load_root_from_paths( paths: list[str], strict: bool = False, providers: list[object] | None = None, + libraries: list[str] | None = None, ) -> tuple[MetaData | None, list[str]]: """Load metadata from an explicit file list rather than a single directory. @@ -287,17 +301,20 @@ def _load_root_from_paths( individual files, which a single ``from_directory`` call cannot express — so this loads each resolved file as its own ``file://`` source via :meth:`MetaDataLoader.from_uris`. Mirrors :func:`_load_root`'s ``strict``/ - ``providers`` contract exactly. + ``providers``/``libraries`` contract exactly. """ uris = [Path(p).resolve().as_uri() for p in paths] if providers: from metaobjects.core_types import core_providers result = MetaDataLoader.from_uris( - uris, providers=[*core_providers, *providers], strict=strict + uris, + providers=[*core_providers, *providers], + strict=strict, + libraries=libraries, ) else: - result = MetaDataLoader.from_uris(uris, strict=strict) + result = MetaDataLoader.from_uris(uris, strict=strict, libraries=libraries) if result.errors: msgs = [f"{e.code}: {e.message}" for e in result.errors] return None, msgs @@ -543,7 +560,22 @@ def _cmd_docs(args: argparse.Namespace) -> int: paths = _resolve_metadata_location_or_print_error(config, root_dir) if paths is None: return 1 - root, errors = _load_root_from_paths(paths, providers=providers) + docs_libraries: list[str] | None = None + if config is not None: + # `config` was already loaded above (to resolve rung 2's `metadata` + # key) — its OWN `providers`/`libraries` must reach this load too, + # exactly as `gen`'s config-mode load does (`_cmd_gen_config`), or a + # project relying on either (e.g. `libraries: ["ai"]` to resolve + # `extends: metaobjects::ai::LlmCallBase`) fails `docs` with + # `ERR_UNRESOLVED_SUPER` while `gen` on the same config succeeds. + config_providers, config_providers_ok = _config_providers(config) + if not config_providers_ok: + return 1 + providers = [*providers, *config_providers] + docs_libraries = config.libraries + root, errors = _load_root_from_paths( + paths, providers=providers, libraries=docs_libraries + ) project_default = root_dir.name else: root, errors = _load_root(args.metadata_dir, providers=providers) diff --git a/server/python/src/metaobjects/config/neutral_config.py b/server/python/src/metaobjects/config/neutral_config.py index b1075ba0e..feedadea4 100644 --- a/server/python/src/metaobjects/config/neutral_config.py +++ b/server/python/src/metaobjects/config/neutral_config.py @@ -49,7 +49,12 @@ def read_neutral_config(config_dir: Path) -> NeutralConfig | None: ) version = raw.get("schema_version") - if version != 1: + # `isinstance(version, bool)` must be checked FIRST and separately: Python's + # `bool` is a subclass of `int` and `True == 1`, so a bare `version != 1` + # check accepts `schema_version: true` — a divergence from Java's + # `isNumber()` and C#'s `ValueKind != Number`, both of which reject a JSON + # boolean outright. + if isinstance(version, bool) or not isinstance(version, (int, float)) or version != 1: raise ParseError( f"{path}: unsupported schema_version {version!r} (expected 1)", code=ErrorCode.ERR_COLLECTION_NOT_FOUND, diff --git a/server/python/src/metaobjects/loader/meta_data_loader.py b/server/python/src/metaobjects/loader/meta_data_loader.py index cb16a04de..e391bb381 100644 --- a/server/python/src/metaobjects/loader/meta_data_loader.py +++ b/server/python/src/metaobjects/loader/meta_data_loader.py @@ -189,13 +189,25 @@ def from_uris( uris: list[str], providers: list[Provider] | None = None, strict: bool = False, + libraries: list[str] | None = None, ) -> LoadResult: """Load metadata from a list of URIs (file:// or http(s)://). ``strict`` (ADR-0023) — see :meth:`from_directory`. + + ``libraries`` — see :meth:`from_directory`; prepended the same way, so a + caller resolving an explicit file list (e.g. the CLI's + ``.metaobjects/config.json`` ``sources`` rung) can opt in to a shipped + library package exactly like a single-directory load can. """ loader = cls(providers=providers, strict=strict) - return loader.load([UriSource(u) for u in uris]) + sources: list[MetaDataSource] = [] + if libraries: + from metaobjects.library import library_sources + + sources.extend(library_sources(libraries)) + sources.extend(UriSource(u) for u in uris) + return loader.load(sources) @classmethod def from_string( diff --git a/server/python/tests/codegen/test_cli_config_gen.py b/server/python/tests/codegen/test_cli_config_gen.py index 26f9f8078..d694553ac 100644 --- a/server/python/tests/codegen/test_cli_config_gen.py +++ b/server/python/tests/codegen/test_cli_config_gen.py @@ -224,6 +224,30 @@ def test_gen_flag_path_ignores_config_when_present(tmp_path: Path) -> None: assert (out / "Program.py").exists() +def test_gen_missing_metadata_dir_errors_cleanly(tmp_path: Path, capsys) -> None: + """An explicit that does not exist must fail with a coded + CLI error, not an uncaught FileNotFoundError traceback. `DirectorySource` + switched from `rglob` to a manual `iterdir()` walk (the symlink-following + fix) — `iterdir()` raises OSError on a directory that never existed; + nothing on the `_load_root` path caught it.""" + missing = tmp_path / "does" / "not" / "exist" + rc = main(["gen", str(missing), "--out", str(tmp_path / "out")]) + assert rc == 1 + err = capsys.readouterr().err + assert "error: failed to load metadata" in err + + +def test_gen_metadata_dir_is_a_file_errors_cleanly(tmp_path: Path, capsys) -> None: + """Same as above, for the `NotADirectoryError` arm: resolves + to a plain file rather than a directory.""" + not_a_dir = tmp_path / "somefile.txt" + not_a_dir.write_text("not a directory") + rc = main(["gen", str(not_a_dir), "--out", str(tmp_path / "out")]) + assert rc == 1 + err = capsys.readouterr().err + assert "error: failed to load metadata" in err + + def test_gen_no_args_no_yaml_falls_back_to_neutral_config( tmp_path: Path, monkeypatch ) -> None: diff --git a/server/python/tests/config/test_neutral_config.py b/server/python/tests/config/test_neutral_config.py index 2f9f58e64..b24cc178e 100644 --- a/server/python/tests/config/test_neutral_config.py +++ b/server/python/tests/config/test_neutral_config.py @@ -65,6 +65,17 @@ def test_wrong_schema_version_raises(tmp_path: Path) -> None: assert e.value.code == ErrorCode.ERR_COLLECTION_NOT_FOUND +def test_boolean_schema_version_raises(tmp_path: Path) -> None: + # Python's `True == 1` is a language quirk the other three ports don't + # share (Gson's `isNumber()` and System.Text.Json's `ValueKind` both + # reject a JSON boolean outright) — a boolean must never satisfy the + # `schema_version == 1` check. + _write_config(tmp_path, {"schema_version": True, "sources": [{"path": "model"}]}) + with pytest.raises(ParseError) as e: + read_neutral_config(tmp_path) + assert e.value.code == ErrorCode.ERR_COLLECTION_NOT_FOUND + + def test_null_sources_raises(tmp_path: Path) -> None: # A present `sources: null` is present-but-wrong-typed, same as a bare # object — it must not silently read as "absent" (see the shared diff --git a/server/python/tests/conformance/test_source_resolution_conformance.py b/server/python/tests/conformance/test_source_resolution_conformance.py index a929d0784..ec5bee412 100644 --- a/server/python/tests/conformance/test_source_resolution_conformance.py +++ b/server/python/tests/conformance/test_source_resolution_conformance.py @@ -134,3 +134,46 @@ def test_docs_with_no_positional_falls_back_to_neutral_config( rc = main(["docs", "--out", str(out)]) assert rc == 0 assert (out / "api" / "python" / "README.md").exists() + + +def test_docs_with_metaobjects_config_yaml_honors_declared_libraries( + tmp_path: Path, monkeypatch +) -> None: + """`docs`'s no-positional branch loads `metaobjects.config.yaml` (rung 2 of + the ladder) purely to read its `metadata` key, then reloads via + `_load_root_from_paths` — which dropped both `config.providers` and + `config.libraries`, even though `config` was already sitting right there. + A project declaring `libraries: ["ai"]` and `extends: + metaobjects::ai::LlmCallBase` must resolve through `docs` exactly as it + already does through `gen` / `verify --codegen` (see + `test_shipped_library_ai.py::TestTheCliCanLoadTheLibrary`). + """ + (tmp_path / "metadata").mkdir() + (tmp_path / "metadata" / "meta.app.yaml").write_text( + "metadata:\n" + " package: app::trace\n" + " children:\n" + " - object.entity:\n" + " name: AdopterCall\n" + " extends: metaobjects::ai::LlmCallBase\n" + " children:\n" + " - source.rdb: { table: adopter_call, role: primary }\n" + " - identity.primary: { name: id, fields: [\"spanId\"] }\n", + encoding="utf-8", + ) + (tmp_path / "metaobjects.config.yaml").write_text( + "metadata: metadata\n" + 'libraries: ["ai"]\n' + "targets:\n" + " main:\n" + " outDir: out\n", + encoding="utf-8", + ) + + from metaobjects.cli import main + + monkeypatch.chdir(tmp_path) + out = tmp_path / "docs-out" + rc = main(["docs", "--out", str(out)]) + assert rc == 0 + assert (out / "api" / "python" / "README.md").exists() From 0966e463988fe3d46cd07530ada69c4dfbefd6de Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 21:13:04 -0400 Subject: [PATCH 2/6] fix(java): strict neutral-config JSON parsing, a space-safe model: URI, and a closed directory-walk stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F8 — NeutralConfig used Gson's `JsonParser.parseString`, which ALWAYS parses leniently regardless of any JsonReader configuration (a well-known Gson quirk: both `JsonParser.parseReader` and `Gson#fromJson(JsonReader, ...)` force `setLenient(true)` on the reader internally). `{schema_version: 1, sources: [{path: 'model'}]}` (unquoted keys, single-quoted strings) and `NaN`/`Infinity` literals loaded clean, contradicting the class's own javadoc and diverging from Python/TS/C#'s strict-by-default JSON parsers. A `JsonReader` walked directly with `setLenient(false)` now validates the document first (discarding its result — `JsonParser.parseString` still builds the real tree, unchanged for every valid-JSON case). F9 — the mojo's port-neutral fallback hands `MetaDataLoader.processSources` a "model:file:" string per resolved source, which reaches `URIHelper.constructValidatedURI`'s `new URI(uriStr)` — the single-String constructor requires already-well-formed RFC 2396 syntax and throws on a raw space, which is common in a checkout path (e.g. ".../My Projects/..."), more so than the colon case a prior commit hardened. The 3-arg (scheme, scheme-specific-part, fragment) constructor quotes illegal characters instead of rejecting them; `toURIModel(URI)` was updated in lockstep to decode via `getSchemeSpecificPart()` rather than re-parsing `toString()`'s re-quoted form, so the space survives the construct-then-read round trip losslessly (verified empirically: identical output to the old construction whenever the source needed no quoting at all). F14 — `SourceResolver.resolveSources`'s directory branch consumed `DirectorySource.expand()` (which wraps `Files.walk`) via a bare `.forEach()` with no `close()`; the JDK documents `Files.walk` as requiring try-with-resources to promptly release its underlying directory-handle resource. Now wrapped. Each fix (F8, F9) was proven non-vacuous: reverted, watched the new test fail (F9's reproduces the exact `IllegalArgumentException` stack the finding described), then restored and watched it pass. F14 (a resource-leak class, "Minor") has no fast, portable way to observe the leak in a unit test without OS-level file-descriptor-limit manipulation in a forked process, disproportionate machinery for this fix; verified instead by confirming the full metadata + maven-plugin suite (1475 tests) stays green with no behavior change. F3 ("the mojo silently loads nothing when the neutral config resolves to zero files") was investigated and found to be a FALSE POSITIVE: a new regression test reproducing the exact shape named in the finding — a pom-silent module whose declared source is a real, empty directory — already passes (getMetaObjects().size() == 0, no exception) on the UNCHANGED mojo, because `MetaDataLoader.configure()`'s own `sources != null && !sources.isEmpty()` guard treats an explicit empty list and an absent one identically; there is no code path where `resolveNeutralSourcesIfPomIsSilent`'s ambiguous empty-list return is externally observable. Forcing this case to raise (matching the CHANGELOG's "no config AND no default dir" ERR_COLLECTION_NOT_FOUND precedent) would also contradict the shared corpus's own `an-empty-directory-source-resolves-to-no-files` case, which requires success with zero files for exactly this shape. The new test is kept as coverage for a previously-untested mojo-integration path (the shared corpus only gates `SourceResolver` directly, never the mojo), not as a bug fix. Co-Authored-By: Claude Opus 5 (1M context) --- .../mojo/NeutralConfigMojoFallbackTest.java | 72 +++++++++++++++++++ .../com/metaobjects/config/NeutralConfig.java | 29 ++++++++ .../metaobjects/config/SourceResolver.java | 13 +++- .../com/metaobjects/loader/uri/URIHelper.java | 34 +++++++-- .../metaobjects/config/NeutralConfigTest.java | 23 ++++++ 5 files changed, 162 insertions(+), 9 deletions(-) diff --git a/server/java/maven-plugin/src/test/java/com/metaobjects/mojo/NeutralConfigMojoFallbackTest.java b/server/java/maven-plugin/src/test/java/com/metaobjects/mojo/NeutralConfigMojoFallbackTest.java index 1f1c5f6e9..0e64459e7 100644 --- a/server/java/maven-plugin/src/test/java/com/metaobjects/mojo/NeutralConfigMojoFallbackTest.java +++ b/server/java/maven-plugin/src/test/java/com/metaobjects/mojo/NeutralConfigMojoFallbackTest.java @@ -144,6 +144,78 @@ public void createLoaderHandlesAColonInTheResolvedAbsolutePath() throws IOExcept } } + @Test + public void createLoaderHandlesASpaceInTheResolvedAbsolutePath() throws IOException { + // F9 — a checkout under a directory containing a space (far more common + // than the colon case above, e.g. "My Projects") used to die with an + // uncoded IllegalArgumentException: `resolveNeutralSourcesIfPomIsSilent` + // hands `MetaDataLoader.processSources` a "model:file:" + // string, which reaches `URIHelper.constructValidatedURI`'s + // `new URI(uriStr)` — the single-String URI constructor requires + // already-well-formed RFC 2396 syntax and throws on a raw space. The + // neutral fallback makes this the DEFAULT path for every pom-silent + // module, so any checkout under a spaced directory name broke every + // `mvn metaobjects:generate`/`:verify` invocation. + Path root = Files.createTempDirectory("mo-mojo-neutral-space-").toAbsolutePath().normalize(); + try { + Path weirdRoot = root.resolve("My Projects"); + Files.createDirectories(weirdRoot); + Path metaDir = weirdRoot.resolve("custom-metadata"); + Files.createDirectories(metaDir); + Files.write(metaDir.resolve("meta.widget.json"), WIDGET_JSON.getBytes(StandardCharsets.UTF_8)); + + Path dotMo = weirdRoot.resolve(".metaobjects"); + Files.createDirectories(dotMo); + Files.write(dotMo.resolve("config.json"), + "{\"schema_version\":1,\"sources\":[{\"path\":\"custom-metadata\"}]}" + .getBytes(StandardCharsets.UTF_8)); + + MetaDataGeneratorMojo mojo = mojoWithSilentPom(weirdRoot); + MetaDataLoader loaded = mojo.createLoader(mojo.createProjectClassLoader()); + + assertEquals(1, loaded.getMetaObjects().size()); + assertEquals("Widget", loaded.getMetaObjects().get(0).getShortName()); + } finally { + deleteRecursive(root); + } + } + + @Test + public void createLoaderResolvesToZeroObjectsWhenTheDeclaredSourceIsAnEmptyDirectory() throws IOException { + // F3 — a pom-silent module whose .metaobjects/config.json DECLARES a real + // (existing) but empty source directory. `resolveNeutralSourcesIfPomIsSilent` + // overloads an empty List return to mean two different things: "the + // pom owns the location" (skip — keep whatever the pom's own was) + // and "the neutral rung WAS consulted and legitimately resolved to zero + // files" (must still win — `sources` must become the empty list, not fall + // back to the pom's own null). Mirrors the shared corpus's + // `an-empty-directory-source-resolves-to-no-files` case, which the Java + // conformance runner gates only at the `SourceResolver` layer — never through + // this mojo — so a regression here was invisible to that gate. + Path root = Files.createTempDirectory("mo-mojo-neutral-empty-dir-").toAbsolutePath().normalize(); + try { + Path emptyDir = root.resolve("empty-model"); + Files.createDirectories(emptyDir); + + Path dotMo = root.resolve(".metaobjects"); + Files.createDirectories(dotMo); + Files.write(dotMo.resolve("config.json"), + "{\"schema_version\":1,\"sources\":[{\"path\":\"empty-model\"}]}" + .getBytes(StandardCharsets.UTF_8)); + + MetaDataGeneratorMojo mojo = mojoWithSilentPom(root); + MetaDataLoader loaded = mojo.createLoader(mojo.createProjectClassLoader()); + + assertEquals("the declared source resolved to zero files — a legitimate, " + + "non-error outcome per the shared corpus — so the loader " + + "must reflect exactly that (zero objects), not silently fall " + + "back to whatever an unconfigured loader would have done", + 0, loaded.getMetaObjects().size()); + } finally { + deleteRecursive(root); + } + } + @Test(expected = MetaDataException.class) public void createLoaderRaisesWhenPomIsSilentAndNoCollectionExists() throws IOException { // Neither a neutral config nor a default "metaobjects/" directory — the final diff --git a/server/java/metadata/src/main/java/com/metaobjects/config/NeutralConfig.java b/server/java/metadata/src/main/java/com/metaobjects/config/NeutralConfig.java index 391cb0d0c..924749fa1 100644 --- a/server/java/metadata/src/main/java/com/metaobjects/config/NeutralConfig.java +++ b/server/java/metadata/src/main/java/com/metaobjects/config/NeutralConfig.java @@ -19,10 +19,14 @@ import com.google.gson.JsonObject; import com.google.gson.JsonParser; import com.google.gson.JsonSyntaxException; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonToken; +import com.google.gson.stream.MalformedJsonException; import com.metaobjects.ErrorCode; import com.metaobjects.MetaDataException; import java.io.IOException; +import java.io.StringReader; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -91,6 +95,31 @@ public static Optional read(Path configDir) { ErrorCode.ERR_MALFORMED_JSON); } + // `JsonParser.parseString` ALWAYS parses leniently, no matter how the + // caller would like to configure it — a well-known Gson quirk: both + // `JsonParser.parseReader` and `Gson#fromJson(JsonReader, ...)` force + // `setLenient(true)` on the reader internally before walking it, so + // unquoted keys, single-quoted strings, and `NaN`/`Infinity` literals all + // parse clean — silently accepting a file that is not actually valid JSON, + // contradicting this class's own javadoc and diverging from every other + // port's stock JSON parser (TS `JSON.parse`, Python `json.loads`, C# + // `System.Text.Json`, all strict by default). A `JsonReader` walked + // directly with `setLenient(false)` is the only way to get genuinely + // strict parsing out of Gson; used here purely to VALIDATE (the walked + // structure is discarded — `JsonParser.parseString` below still builds the + // actual `JsonElement` tree, unchanged). + try (JsonReader strict = new JsonReader(new StringReader(content))) { + strict.setLenient(false); + strict.skipValue(); + if (strict.peek() != JsonToken.END_DOCUMENT) { + throw new MalformedJsonException("trailing content after the top-level value"); + } + } catch (IOException e) { + throw new MetaDataException( + path + " exists but could not be parsed as JSON: " + e.getMessage(), + ErrorCode.ERR_MALFORMED_JSON); + } + JsonElement parsed; try { parsed = JsonParser.parseString(content); diff --git a/server/java/metadata/src/main/java/com/metaobjects/config/SourceResolver.java b/server/java/metadata/src/main/java/com/metaobjects/config/SourceResolver.java index 74b7e7f3a..a22ceaabb 100644 --- a/server/java/metadata/src/main/java/com/metaobjects/config/SourceResolver.java +++ b/server/java/metadata/src/main/java/com/metaobjects/config/SourceResolver.java @@ -18,6 +18,7 @@ import com.metaobjects.ErrorCode; import com.metaobjects.MetaDataException; import com.metaobjects.loader.DirectorySource; +import com.metaobjects.loader.FileSource; import java.nio.file.Files; import java.nio.file.Path; @@ -25,6 +26,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.stream.Stream; /** * Turns a declared source SET ({@code .metaobjects/config.json}'s {@code sources}, or @@ -97,8 +99,15 @@ public static List resolveSources(Path configDir, List // DirectorySource directly gets every file) — this CLI-facing // resolver turns it ON, since `_pending/` is the TypeScript CLI's // pending/promote-workflow concept, not a loader concept. - new DirectorySource(target, new DirectorySource.Options().setExcludePending(true)).expand() - .forEach(fs -> seen.add(fs.getPath().toAbsolutePath().normalize())); + // try-with-resources: expand() wraps Files.walk(), which the JDK + // documents as requiring prompt closing of its underlying + // directory-handle resource ("must be used within a try-with-resources + // statement or similar control structure") — a plain `.forEach()` + // with no `close()` leaks it on every directory source resolved. + try (Stream stream = + new DirectorySource(target, new DirectorySource.Options().setExcludePending(true)).expand()) { + stream.forEach(fs -> seen.add(fs.getPath().toAbsolutePath().normalize())); + } } else { seen.add(target.toAbsolutePath().normalize()); } diff --git a/server/java/metadata/src/main/java/com/metaobjects/loader/uri/URIHelper.java b/server/java/metadata/src/main/java/com/metaobjects/loader/uri/URIHelper.java index c5adca2fd..b6c1e47ff 100644 --- a/server/java/metadata/src/main/java/com/metaobjects/loader/uri/URIHelper.java +++ b/server/java/metadata/src/main/java/com/metaobjects/loader/uri/URIHelper.java @@ -40,7 +40,16 @@ public static URIModel toURIModel( String in ) { } public static URIModel toURIModel( URI in ) { - return toURIModel( in.toString() ); + // NOT `in.toString()`: a URI built by `constructValidatedURI` below + // quotes characters that need it (a space -> "%20") in its string + // form, but `parseURIModel` is a dumb substring splitter with no + // percent-decoding of its own — feeding it the ENCODED string would + // hand a source string containing a literal "%20" downstream to + // `new File(...)`, silently looking for the wrong path. + // `getSchemeSpecificPart()` returns the DECODED form, so reassembling + // ":" round-trips back to exactly what + // was originally passed to `constructValidatedURI`. + return toURIModel( in.getScheme() + ":" + in.getSchemeSpecificPart() ); } public static URI toURI( String in ) { @@ -58,20 +67,31 @@ public static URI constructURI(String uriType, String uriSourceType, String sour } public static URI constructValidatedURI(String uriType, String uriSourceType, String source, Map args) { - String uriStr = uriType + ":" + uriSourceType + ":" + source; + String ssp = uriSourceType + ":" + source; if ( args != null && !args.isEmpty() ) { - uriStr += ";"; + ssp += ";"; boolean first = true; for (String key : args.keySet() ) { if ( first ) first = false; - else uriStr += "&"; - uriStr += key+"="+args.get(key); + else ssp += "&"; + ssp += key+"="+args.get(key); } } try { - return new URI(uriStr ); + // The single-String `new URI(String)` constructor requires the input + // to already be fully well-formed RFC 2396 syntax and THROWS on any + // character that needs quoting — a raw space in an absolute + // filesystem path (e.g. a checkout under ".../My Projects/...") is + // the common case, and it is more common than the colon ambiguity + // this method's caller was previously hardened against. The 3-arg + // (scheme, scheme-specific-part, fragment) constructor QUOTES illegal + // characters instead of rejecting them — `toURIModel(URI)` above + // undoes the quoting on the way back out, so the round trip is + // lossless (verified: identical `toString()` output to the old + // single-arg construction whenever `source` needed no quoting at all). + return new URI(uriType, ssp, null); } catch( URISyntaxException e ) { - throw new IllegalArgumentException( "URI Syntax exception ["+uriStr+"]: "+ e.getMessage(), e ); + throw new IllegalArgumentException( "URI Syntax exception ["+uriType+":"+ssp+"]: "+ e.getMessage(), e ); } } diff --git a/server/java/metadata/src/test/java/com/metaobjects/config/NeutralConfigTest.java b/server/java/metadata/src/test/java/com/metaobjects/config/NeutralConfigTest.java index 7dd719138..82d5d86ee 100644 --- a/server/java/metadata/src/test/java/com/metaobjects/config/NeutralConfigTest.java +++ b/server/java/metadata/src/test/java/com/metaobjects/config/NeutralConfigTest.java @@ -69,6 +69,29 @@ public void schemaVersionAsFloatLiteralIsAcceptedLikeTheOtherThreePorts() throws assertEquals(1, cfg.getSources().size()); } + @Test + public void unquotedKeysAndSingleQuotedStringsAreRejected() throws IOException { + // F8 — Gson's `JsonParser.parseString` ALWAYS parses leniently regardless + // of any JsonReader configuration (a well-known Gson quirk: both + // `JsonParser.parseReader` and `Gson.fromJson(JsonReader, ...)` force + // `setLenient(true)` internally before walking the reader). That silently + // accepted unquoted keys and single-quoted strings — syntax the class + // javadoc promises is rejected, and which TS's `JSON.parse`, Python's + // `json.loads`, and C#'s `System.Text.Json` all reject. + Path dir = writeConfig("{schema_version: 1, sources: [{path: 'model'}]}"); + MetaDataException ex = assertThrows(MetaDataException.class, () -> NeutralConfig.read(dir)); + assertEquals(ErrorCode.ERR_MALFORMED_JSON, ex.getCode().orElseThrow()); + } + + @Test + public void nanAndInfinityLiteralsAreRejected() throws IOException { + // Same Gson-leniency quirk as above, for the numeric-literal extensions + // (`NaN` / `Infinity` / `-Infinity`) lenient mode also accepts. + Path dir = writeConfig("{ \"schema_version\": NaN, \"sources\": [] }"); + MetaDataException ex = assertThrows(MetaDataException.class, () -> NeutralConfig.read(dir)); + assertEquals(ErrorCode.ERR_MALFORMED_JSON, ex.getCode().orElseThrow()); + } + @Test public void schemaVersionNonIntegralRaisesRatherThanTruncating() throws IOException { // Regression: Gson's JsonPrimitive#getAsInt() TRUNCATES a non-integral From 7196697770f2c7bd66bcc5cee672a5937f2afd20 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 21:25:47 -0400 Subject: [PATCH 3/6] fix(csharp): the .metaobjects/config.json ladder loads its own resolved file list, not a re-walked directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F5/F15 — Program.cs's ResolveMetadataDirOrExit resolved the declared source via SourceResolver.ResolveSources purely for its kind/existence validation, discarded the (already `_pending`-draft-excluded) file list it returned, and handed callers a bare directory string instead. Every command (gen/docs/verify) then loaded that directory via MetaDataLoader.FromDirectory, which both re-walked the tree a second time (doubling I/O per invocation — F15) AND used the loader-level DirectorySource default of ExcludePending = false, so a `_pending/` draft that TS, Java and Python all keep invisible to codegen leaked straight into generated output (F5). ResolveMetadataDirOrExit now returns a small ResolvedMetadata(Directory, Files) struct: Files is null for an explicit CLI argument (unaffected, byte-identical), and the resolver's own file list for a ladder-resolved source. GenCommand.Run, DocsCommand.Run, and VerifyCommand's two internal load sites each gained a LoadResult-based overload alongside their existing metadataDir-based one (a thin FromDirectory-calling wrapper now, fully backward compatible — no existing call site or test needed to change); Program.cs calls the new overload with a MetaDataLoader.FromUris(...) load built from the resolved file list when one is available. MetaDataLoader gained a `strict`-aware FromUris overload to support verify's --lax. Every gate now loads metadata exactly once per `dotnet meta` invocation, using the one already-filtered file list, regardless of how many verify subverbs run. Verified non-vacuous: a new subprocess-driven test (mirroring MetadataDirFallbackTests' existing pattern of exercising the real built CLI) declares a `_pending/` draft entity under a ladder-resolved source; reverted just the production files and confirmed the test failed with the draft's `DraftWidget.g.cs` actually present in the generated output (RED), then restored and confirmed only the real entity's file was written (GREEN). Co-Authored-By: Claude Opus 5 (1M context) --- .../MetadataDirFallbackTests.cs | 39 +++++++++ server/csharp/MetaObjects.Cli/DocsCommand.cs | 14 +++- server/csharp/MetaObjects.Cli/GenCommand.cs | 18 +++- server/csharp/MetaObjects.Cli/Program.cs | 83 ++++++++++++++----- .../csharp/MetaObjects.Cli/VerifyCommand.cs | 35 +++++++- .../MetaObjects/Loader/MetaDataLoader.cs | 11 ++- 6 files changed, 171 insertions(+), 29 deletions(-) diff --git a/server/csharp/MetaObjects.Cli.Tests/MetadataDirFallbackTests.cs b/server/csharp/MetaObjects.Cli.Tests/MetadataDirFallbackTests.cs index e56667c1b..e59d47125 100644 --- a/server/csharp/MetaObjects.Cli.Tests/MetadataDirFallbackTests.cs +++ b/server/csharp/MetaObjects.Cli.Tests/MetadataDirFallbackTests.cs @@ -131,6 +131,45 @@ public void Gen_with_no_positional_metadataDir_and_a_single_FILE_source_refuses_ Assert.False(Directory.Exists(outDir)); } + [Fact] + public void Gen_with_no_positional_metadataDir_excludes_pending_drafts() + { + // F5 — the ladder path used to discard SourceResolver.ResolveSources's own + // return value (the `_pending`-excluded file list) and hand + // MetaDataLoader.FromDirectory a bare directory instead, whose default + // DirectorySource.Options has ExcludePending = false — so a `_pending/` + // draft that TS/Java/Python all keep invisible to codegen leaked into the + // generated output here. `_pending/` is excluded at ANY depth under the + // declared source, matching the other three ports. + var modelDir = Path.Combine(_tmp, "model"); + Directory.CreateDirectory(modelDir); + File.WriteAllText(Path.Combine(modelDir, "meta.acme.json"), Metadata); + var pendingDir = Path.Combine(modelDir, "_pending"); + Directory.CreateDirectory(pendingDir); + File.WriteAllText(Path.Combine(pendingDir, "meta.draft.json"), """ + { "metadata.root": { "package": "acme", "children": [ + { "object.entity": { "name": "DraftWidget", "children": [ + { "source.rdb": { "@table": "draft_widgets" } }, + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}} + ]}} + """); + var cfgDir = Path.Combine(_tmp, ".metaobjects"); + Directory.CreateDirectory(cfgDir); + File.WriteAllText( + Path.Combine(cfgDir, "config.json"), + """{ "schema_version": 1, "sources": [ { "path": "model" } ] }"""); + + var outDir = Path.Combine(_tmp, "generated"); + var (exitCode, stdout, stderr) = RunCli(_tmp, "gen", "--out", outDir, "--namespace", "Acme.Generated"); + + Assert.True(exitCode == 0, $"exit={exitCode}\nstdout={stdout}\nstderr={stderr}"); + Assert.True(File.Exists(Path.Combine(outDir, "Subscriber.g.cs")), stdout + stderr); + Assert.False(File.Exists(Path.Combine(outDir, "DraftWidget.g.cs")), + "a _pending/ draft must never reach generated output: " + stdout + stderr); + } + [Fact] public void Gen_with_an_explicit_positional_metadataDir_is_unaffected() { diff --git a/server/csharp/MetaObjects.Cli/DocsCommand.cs b/server/csharp/MetaObjects.Cli/DocsCommand.cs index c16b13517..c7a2688b0 100644 --- a/server/csharp/MetaObjects.Cli/DocsCommand.cs +++ b/server/csharp/MetaObjects.Cli/DocsCommand.cs @@ -41,8 +41,20 @@ public sealed record Outcome( public static Outcome Run( string metadataDir, string outDir, string project, string ns, string apiSubDir = DefaultApiSubDir, string? modelBaseUrl = null) + => Run(MetaDataLoader.FromDirectory(metadataDir), outDir, project, ns, apiSubDir, modelBaseUrl); + + /// + /// Same as the metadataDir overload above, but starting from an + /// ALREADY-LOADED — see the identical overload on + /// for why (the CLI's config-ladder path resolves + + /// loads once via MetaDataLoader.FromUris, correctly excluding + /// _pending drafts; a second FromDirectory call here would both + /// re-walk the tree and silently lose that exclusion). + /// + public static Outcome Run( + LoadResult load, string outDir, string project, string ns, + string apiSubDir = DefaultApiSubDir, string? modelBaseUrl = null) { - var load = MetaDataLoader.FromDirectory(metadataDir); var loadErrors = load.Errors.Select(e => e.Code.ToString()).ToList(); if (loadErrors.Count > 0) return new Outcome(loadErrors, []); diff --git a/server/csharp/MetaObjects.Cli/GenCommand.cs b/server/csharp/MetaObjects.Cli/GenCommand.cs index 16ca4f8fb..7902e8eb8 100644 --- a/server/csharp/MetaObjects.Cli/GenCommand.cs +++ b/server/csharp/MetaObjects.Cli/GenCommand.cs @@ -68,8 +68,24 @@ public static Outcome Run(string metadataDir, string outDir, string ns, bool emi public static Outcome Run( string metadataDir, string outDir, string ns, bool emitAbstractShapes, IReadOnlyList? generatorNames, string? templateRoot, string? templateSpecPath = null) + => Run(MetaDataLoader.FromDirectory(metadataDir), outDir, ns, emitAbstractShapes, + generatorNames, templateRoot, templateSpecPath); + + /// + /// Same as the metadataDir overload above, but starting from an + /// ALREADY-LOADED — used by the CLI's + /// .metaobjects/config.json ladder path (Program.cs's + /// ResolveMetadataDirOrExit), which resolves AND loads the declared + /// source set itself via + /// (honoring the _pending-draft exclusion SourceResolver applies). + /// Calling + /// again here would re-walk the directory tree a second time AND silently lose + /// that exclusion (FromDirectory's own default is to include _pending). + /// + public static Outcome Run( + LoadResult load, string outDir, string ns, bool emitAbstractShapes, + IReadOnlyList? generatorNames, string? templateRoot, string? templateSpecPath = null) { - var load = MetaDataLoader.FromDirectory(metadataDir); var loadErrors = load.Errors.Select(e => e.Code.ToString()).ToList(); if (loadErrors.Count > 0) return new Outcome(loadErrors, null); diff --git a/server/csharp/MetaObjects.Cli/Program.cs b/server/csharp/MetaObjects.Cli/Program.cs index fa6a457e8..4689bee8e 100644 --- a/server/csharp/MetaObjects.Cli/Program.cs +++ b/server/csharp/MetaObjects.Cli/Program.cs @@ -88,7 +88,7 @@ static int RunGen(string[] rest) // Rung 1 (explicit positional) is honored as-is; an omitted metadataDir // falls back to the port-neutral .metaobjects/config.json ladder. - metadataDir = ResolveMetadataDirOrExit(metadataDir); + var resolvedMeta = ResolveMetadataDirOrExit(metadataDir); // Advisory: nudge a re-scaffold if the copied-in agent context predates this build. // Never throws, never changes the exit code (a missing/corrupt manifest is ignored). @@ -97,7 +97,16 @@ static int RunGen(string[] rest) var generatorNames = generatorsCsv ?.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); - var outcome = GenCommand.Run(metadataDir, outDir, ns, emitAbstractShapes, generatorNames, templateRoot, templateSpecPath); + // A ladder-resolved (non-null Files) source loads via the already-resolved, + // `_pending`-excluded file list (MetaDataLoader.FromUris) — never a second + // FromDirectory walk of resolvedMeta.Directory, which would both duplicate + // the walk ResolveMetadataDirOrExit already did AND silently include `_pending`. + var outcome = resolvedMeta.Files is { } files + ? GenCommand.Run( + MetaObjects.Loader.MetaDataLoader.FromUris(files.Select(f => new Uri(f)).ToList()), + outDir, ns, emitAbstractShapes, generatorNames, templateRoot, templateSpecPath) + : GenCommand.Run( + resolvedMeta.Directory, outDir, ns, emitAbstractShapes, generatorNames, templateRoot, templateSpecPath); if (!outcome.Ok) { foreach (var e in outcome.LoadErrors) Console.Error.WriteLine($" load error: {e}"); @@ -141,13 +150,20 @@ static int RunDocs(string[] rest) // Rung 1 (explicit positional) is honored as-is; an omitted metadataDir // falls back to the port-neutral .metaobjects/config.json ladder. - metadataDir = ResolveMetadataDirOrExit(metadataDir); + var resolvedMeta = ResolveMetadataDirOrExit(metadataDir); // Default the project label to the input directory's leaf name (cosmetic — surfaces // in the AGENT-API header). Trailing-separator-safe. - project ??= new DirectoryInfo(Path.TrimEndingDirectorySeparator(Path.GetFullPath(metadataDir))).Name; - - var outcome = DocsCommand.Run(metadataDir, outDir, project, ns, modelBaseUrl: modelBaseUrl); + project ??= new DirectoryInfo(Path.TrimEndingDirectorySeparator(Path.GetFullPath(resolvedMeta.Directory))).Name; + + // See the identical comment in RunGen above: a ladder-resolved source loads + // via its already-resolved, `_pending`-excluded file list, never a second + // (unfiltered) directory walk. + var outcome = resolvedMeta.Files is { } files + ? DocsCommand.Run( + MetaObjects.Loader.MetaDataLoader.FromUris(files.Select(f => new Uri(f)).ToList()), + outDir, project, ns, modelBaseUrl: modelBaseUrl) + : DocsCommand.Run(resolvedMeta.Directory, outDir, project, ns, modelBaseUrl: modelBaseUrl); if (!outcome.Ok) { foreach (var e in outcome.LoadErrors) Console.Error.WriteLine($" load error: {e}"); @@ -174,13 +190,27 @@ static int RunDocs(string[] rest) // docs, verify) so an omitted positional argument is never a hard requirement // wherever a project's config can name the location instead. // -// Never returns null: either hands back a real directory, or prints a -// diagnostic and terminates the process — callers may treat the result as -// always-present and keep their existing (now-unreachable-when-omitted) -// null checks for the OTHER positional/option they still require. -static string ResolveMetadataDirOrExit(string? metadataDir) +// The metadata-location ladder's result: always a directory (explicit-arg +// back-compat, and cosmetic labeling even on the ladder path), and — when +// resolution went through the .metaobjects/config.json ladder rather than an +// explicit CLI argument — the ladder's OWN already-resolved, `_pending`-draft- +// excluded file list too. A caller with a non-null Files must load via +// MetaDataLoader.FromUris(Files) rather than FromDirectory(Directory): the +// latter would both re-walk a tree this function already walked once (via +// SourceResolver) AND silently lose the `_pending` exclusion, since +// DirectorySource.Options.ExcludePending defaults to false at the loader +// level (SourceResolver is the one place that turns it on). Declared at file +// scope below the entry point (top-level-statement files require type +// declarations to follow every top-level statement / local function). + +// Never exits without a usable result: either hands back a real directory +// (+ file list, when ladder-resolved), or prints a diagnostic and terminates +// the process — callers may treat the result as always-present and keep +// their existing (now-unreachable-when-omitted) null checks for the OTHER +// positional/option they still require. +static ResolvedMetadata ResolveMetadataDirOrExit(string? metadataDir) { - if (metadataDir is not null) return metadataDir; + if (metadataDir is not null) return new ResolvedMetadata(metadataDir, null); var cwd = Directory.GetCurrentDirectory(); try @@ -190,11 +220,13 @@ static string ResolveMetadataDirOrExit(string? metadataDir) if (specs.Count == 0) { - // No declared sources — validate + apply the DEFAULT directory through + // No declared sources — resolve + apply the DEFAULT directory through // the same ladder the shared conformance corpus gates (raises - // ERR_COLLECTION_NOT_FOUND when the default is also absent). - _ = MetaObjects.Config.SourceResolver.ResolveCollection(cwd); - return Path.Combine(cwd, MetaObjects.Config.NeutralConfig.DefaultMetadataDir); + // ERR_COLLECTION_NOT_FOUND when the default is also absent). The + // returned file list IS the load — no second walk needed. + var defaultFiles = MetaObjects.Config.SourceResolver.ResolveCollection(cwd); + return new ResolvedMetadata( + Path.Combine(cwd, MetaObjects.Config.NeutralConfig.DefaultMetadataDir), defaultFiles); } if (specs.Count > 1) @@ -213,9 +245,9 @@ static string ResolveMetadataDirOrExit(string? metadataDir) // Exactly one declared source. Resolve + validate it through the same // kind/existence checks ResolveSources applies (ERR_SOURCE_KIND_UNSUPPORTED / - // ERR_SOURCE_UNRESOLVED), then hand the loader that spec's OWN root — never - // the default directory name, which this project may not even have. - MetaObjects.Config.SourceResolver.ResolveSources(cwd, specs); + // ERR_SOURCE_UNRESOLVED) — its return value IS the (already `_pending`- + // excluded) file list to load, not just a validation signal to discard. + var files = MetaObjects.Config.SourceResolver.ResolveSources(cwd, specs); var rawPath = specs[0]["path"]; // guaranteed present: ResolveSources above // would already have thrown otherwise. var resolved = Path.IsPathRooted(rawPath) ? rawPath : Path.GetFullPath(Path.Combine(cwd, rawPath)); @@ -235,7 +267,7 @@ static string ResolveMetadataDirOrExit(string? metadataDir) throw new InvalidOperationException("unreachable"); } - return resolved; + return new ResolvedMetadata(resolved, files); } catch (MetaObjects.MetaModelException e) { @@ -306,7 +338,7 @@ static int RunVerify(string[] rest) // Rung 1 (explicit positional) is honored as-is; an omitted metadataDir // falls back to the port-neutral .metaobjects/config.json ladder. - metadataDir = ResolveMetadataDirOrExit(metadataDir); + var resolvedMeta = ResolveMetadataDirOrExit(metadataDir); // The templates gate needs a root. Bare verify (defaults to templates) and an // explicit --templates both require it; surface a clear usage error if absent. @@ -328,7 +360,11 @@ static int RunVerify(string[] rest) var opts = new VerifyCommand.Options { - MetadataDir = metadataDir, + MetadataDir = resolvedMeta.Directory, + // A ladder-resolved source loads via this already-resolved, + // `_pending`-excluded file list (see VerifyCommand.LoadMetadata) — never a + // second (unfiltered) directory walk of MetadataDir. + MetadataFiles = resolvedMeta.Files, TemplatesRoot = templatesRoot, OutDir = outDir, Namespace = ns, @@ -390,3 +426,6 @@ static int RunVerify(string[] rest) return result.ExitCode; } + +// See the doc comment on ResolveMetadataDirOrExit above. +readonly record struct ResolvedMetadata(string Directory, IReadOnlyList? Files); diff --git a/server/csharp/MetaObjects.Cli/VerifyCommand.cs b/server/csharp/MetaObjects.Cli/VerifyCommand.cs index bcde9c9e2..627899c4b 100644 --- a/server/csharp/MetaObjects.Cli/VerifyCommand.cs +++ b/server/csharp/MetaObjects.Cli/VerifyCommand.cs @@ -79,6 +79,17 @@ public sealed record Options { /// Metadata directory (positional arg). public required string MetadataDir { get; init; } + /// + /// When set, the ALREADY-RESOLVED (and _pending-draft-excluded) file list + /// the CLI's .metaobjects/config.json ladder path computed + /// (Program.cs's ResolveMetadataDirOrExit) — both subverb gates load + /// from this via MetaDataLoader.FromUris instead of re-walking + /// via FromDirectory, which would both duplicate + /// the walk already done to resolve it AND silently lose the exclusion + /// (FromDirectory's own default is to include _pending). Null when + /// came from an explicit CLI argument instead. + /// + public IReadOnlyList? MetadataFiles { get; init; } /// Templates root for the --templates gate (--templates <root>). public string? TemplatesRoot { get; init; } /// The committed output dir for the --codegen gate (--out <dir>). @@ -153,7 +164,7 @@ public static SubverbResult RunSubverbs(Options opts) Outcome? templatesOutcome = null; if (runTemplates) { - templatesOutcome = Run(opts.MetadataDir, opts.TemplatesRoot ?? "", opts.Strict); + templatesOutcome = Run(LoadMetadata(opts), opts.TemplatesRoot ?? ""); if (!templatesOutcome.Ok) exit = Math.Max(exit, 1); } @@ -185,6 +196,17 @@ public static SubverbResult RunSubverbs(Options opts) }; } + /// + /// Loads 's metadata once, the same way for both subverb + /// gates: via the pre-resolved (config-ladder + /// path — MetaDataLoader.FromUris, no re-walk, _pending already + /// excluded) when present, else FromDirectory(opts.MetadataDir) (an + /// explicit CLI argument — the legacy, unfiltered directory load). + /// + private static LoadResult LoadMetadata(Options opts) => opts.MetadataFiles is { } files + ? MetaDataLoader.FromUris(files.Select(f => new Uri(f)).ToList(), opts.Strict) + : MetaDataLoader.FromDirectory(opts.MetadataDir, strict: opts.Strict); + /// /// Run the codegen-drift gate: load metadata, resolve the generator suite (default /// or the --generators selection), and diff a fresh regen against the @@ -201,7 +223,7 @@ private static Codegen.CodegenDrift.Result RunCodegenDrift(Options opts) "generated output to diff against.", }; - var load = MetaDataLoader.FromDirectory(opts.MetadataDir, strict: opts.Strict); + var load = LoadMetadata(opts); if (load.Errors.Count > 0) return new Codegen.CodegenDrift.Result { @@ -252,8 +274,15 @@ private static void AddIfPresent(List refs, object? attr) } public static Outcome Run(string metadataDir, string templatesRoot, bool strict = true) + => Run(MetaDataLoader.FromDirectory(metadataDir, strict: strict), templatesRoot); + + /// + /// Same as the metadataDir overload above, but starting from an + /// ALREADY-LOADED — see / + /// 's identical overload for why. + /// + public static Outcome Run(LoadResult load, string templatesRoot) { - var load = MetaDataLoader.FromDirectory(metadataDir, strict: strict); var loadErrors = load.Errors.Select(e => e.Code.ToString()).ToList(); var provider = new FilesystemProvider(templatesRoot); diff --git a/server/csharp/MetaObjects/Loader/MetaDataLoader.cs b/server/csharp/MetaObjects/Loader/MetaDataLoader.cs index 230c86f57..c56663d7e 100644 --- a/server/csharp/MetaObjects/Loader/MetaDataLoader.cs +++ b/server/csharp/MetaObjects/Loader/MetaDataLoader.cs @@ -134,13 +134,20 @@ public static LoadResult FromDirectory(string directory, TypeRegistry registry, public static LoadResult FromUris(IReadOnlyList uris) => FromUris(uris, DefaultRegistry()); + /// + /// Convenience: as above, with (ADR-0023) — see + /// . + /// + public static LoadResult FromUris(IReadOnlyList uris, bool strict) + => FromUris(uris, DefaultRegistry(), strict); + /// /// Registry-aware overload: wrap each URI in a and /// load using the supplied . /// - public static LoadResult FromUris(IReadOnlyList uris, TypeRegistry registry) + public static LoadResult FromUris(IReadOnlyList uris, TypeRegistry registry, bool strict = false) { - var loader = new MetaDataLoader(registry); + var loader = new MetaDataLoader(registry, strict: strict); var sources = uris.Select(u => (IMetaDataSource)new UriSource(u)).ToList(); return loader.Load(sources); } From 70f8a93888d185537ef6458c53c170968f6d69ff Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 21:34:03 -0400 Subject: [PATCH 4/6] fix(typescript): --replay-snapshot on an empty chain, a symlink-cycle guard, and init's overwrite report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F6 — `meta verify --replay-snapshot`'s empty-chain early return fired unconditionally, before `runReplaySnapshotTier` ever ran, so a wrong `migrate.outDir` or a `migrate baseline --from-db`-adopted project (whose own chain is empty by construction) reported success having compared NOTHING against a committed snapshot that may record dozens of tables. Tier 1 (does the chain apply?) is legitimately done on an empty chain — there is nothing that could fail — but tier 2 (does the replay reproduce the snapshot?) is not, and now falls through to it. F7 — the reference walk (`listMetadataFiles`) follows a symlinked directory via `stat` with no loop guard, even though this PR promoted symlink-following to a cross-port contract and both Java and Python added cycle detection when they picked it up. Empirically, a self-referential directory symlink (`model/loop -> model`) doesn't hang outright on Linux — the kernel's own ELOOP eventually kicks in — but it silently returns ~40 phantom duplicate file paths (`model/loop/loop/.../meta.a.json`) with no error at all, which is its own bug independent of the hang concern the finding raises. Now guarded with a `realpath`-keyed ancestor set (mirroring Python's `SymlinkLoopError` approach) that raises immediately with a clear message instead. F11 — `writeConfigFile`'s destructive-replacement branch (an existing, unparseable `.metaobjects/config.json`, replaced with defaults under --force) called `writeFresh()` but never pushed onto `result.created`, unlike the other two `writeFresh()` call sites in the same function. The CLI's `--config-only` summary keys on `result.created.includes(...)` alone to choose between "Wrote ..." and "already exists — left untouched.", so a config the caller had just destroyed and replaced with defaults was reported as untouched — the opposite of what happened, and contradicting the warning line printed directly below it. A genuinely preserved (valid, merged) existing config still correctly reports "left untouched" via the separate `result.preserved` bucket, unaffected by this fix. Each fix was proven non-vacuous: reverted, watched the new/extended test fail (F6's against the real committed snapshot; F7's actually resolving with 41 phantom paths instead of throwing; F11's `result.created` empty), then restored and confirmed green. Co-Authored-By: Claude Opus 5 (1M context) --- .../packages/cli/src/commands/init.ts | 7 ++++ .../packages/cli/src/commands/verify.ts | 21 +++++++---- .../typescript/packages/cli/test/init.test.ts | 24 +++++++++++++ .../packages/cli/test/verify-replay.test.ts | 30 +++++++++++++++- .../packages/sdk/src/metadata-files.ts | 36 +++++++++++++++++-- .../packages/sdk/test/sources.test.ts | 13 +++++++ 6 files changed, 122 insertions(+), 9 deletions(-) diff --git a/server/typescript/packages/cli/src/commands/init.ts b/server/typescript/packages/cli/src/commands/init.ts index 25134f17d..e82fa7033 100644 --- a/server/typescript/packages/cli/src/commands/init.ts +++ b/server/typescript/packages/cli/src/commands/init.ts @@ -368,6 +368,13 @@ async function writeConfigFile(opts: InitOptions, result: InitResult, agentDir: log.warn(priorContent); result.warnings.push("invalid .metaobjects/config.json replaced with defaults"); await writeFresh(); + // F11 — matches the OTHER two `writeFresh()` call sites above: this IS a + // fresh write (a destructive one, replacing content that could not be + // parsed), not a no-op. Omitting this left it in neither `created` nor + // `preserved`, so the `--config-only` CLI summary (which keys on + // `result.created.includes(...)` alone) reported "already exists — left + // untouched" for a config it had just overwritten with defaults. + result.created.push(".metaobjects/config.json"); } } diff --git a/server/typescript/packages/cli/src/commands/verify.ts b/server/typescript/packages/cli/src/commands/verify.ts index f3c702ddd..840c39301 100644 --- a/server/typescript/packages/cli/src/commands/verify.ts +++ b/server/typescript/packages/cli/src/commands/verify.ts @@ -332,16 +332,25 @@ export async function verifyCommand( // and a gate that is quiet when it checked nothing cannot be told from one that // passed. Every migration is pending against a fresh engine, so an empty // `pending` means the directory held none. + // + // This return is for TIER 1 ONLY: an empty chain trivially "applies" (there is + // nothing that could fail), so tier 1 is done. Tier 2 is NOT done — its job is + // "does the replay reproduce the committed snapshot?", and a wrong + // `migrate.outDir` or a project adopted via `migrate baseline --from-db` (whose + // own chain is empty by construction) both look identical to this point. Return + // ONLY when `--replay-snapshot` was not requested; otherwise fall through so an + // empty replay is still compared against a snapshot that may record dozens of + // tables, rather than reporting success having compared nothing. if (applied.pending.length === 0) { log.info(`meta verify --replay: no committed migrations — nothing to replay`); - return 0; + if (!flags.replaySnapshot) return 0; + } else { + log.info( + `meta verify --replay — the committed chain applies to an empty ${dialect} database ` + + `(${applied.applied.length} migration(s)).`, + ); } - log.info( - `meta verify --replay — the committed chain applies to an empty ${dialect} database ` + - `(${applied.applied.length} migration(s)).`, - ); - if (!flags.replaySnapshot) return 0; return await runReplaySnapshotTier(engine, dialect, dir); } finally { diff --git a/server/typescript/packages/cli/test/init.test.ts b/server/typescript/packages/cli/test/init.test.ts index de0ba0e26..788bf3530 100644 --- a/server/typescript/packages/cli/test/init.test.ts +++ b/server/typescript/packages/cli/test/init.test.ts @@ -290,6 +290,30 @@ describe("init() --config-only", () => { expect(result.warnings).toContain("invalid .metaobjects/config.json replaced with defaults"); const cfg = JSON.parse(readFileSync(join(cwd, ".metaobjects", "config.json"), "utf8")); expect(cfg.sources).toEqual([]); + // F11 — this destructive replacement must be reported in `result.created` + // (matching the OTHER two `writeFresh()` call sites in `writeConfigFile`), + // not silently omitted from both `created` and `preserved`. The CLI's + // `--config-only` summary keys on `result.created.includes(...)` alone to + // choose between "Wrote ..." and "already exists — left untouched." — + // without this, a config the caller just DESTROYED and replaced with + // defaults is reported as "left untouched", the opposite of what happened. + expect(result.created).toContain(".metaobjects/config.json"); + }); + + test("preserving a valid existing config is reported separately from a fresh write", async () => { + // The sibling of the case above: a VALID existing config is genuinely left + // untouched (merged in place via saveConfig, not replaced with defaults) — + // `result.preserved`, not `result.created`, is the correct bucket for it. + mkdirSync(join(cwd, ".metaobjects"), { recursive: true }); + writeFileSync( + join(cwd, ".metaobjects", "config.json"), + JSON.stringify({ schema_version: 1, sources: [{ path: "model" }] }), + ); + + const result = await init({ cwd, configOnly: true, force: true }); + + expect(result.preserved).toContain(".metaobjects/config.json"); + expect(result.created).not.toContain(".metaobjects/config.json"); }); }); diff --git a/server/typescript/packages/cli/test/verify-replay.test.ts b/server/typescript/packages/cli/test/verify-replay.test.ts index 0686a6188..3917f7a23 100644 --- a/server/typescript/packages/cli/test/verify-replay.test.ts +++ b/server/typescript/packages/cli/test/verify-replay.test.ts @@ -12,7 +12,7 @@ * against a real project on disk. */ import { describe, test, expect, afterAll } from "bun:test"; -import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises"; +import { mkdtemp, mkdir, writeFile, rm, readdir, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { parseVerifyArgs } from "../src/lib/args.js"; @@ -222,4 +222,32 @@ describe("verify --replay-snapshot runs BOTH tiers", () => { await withApplyingChain(root); expect(await verifyCommand(["--replay-snapshot"], root)).toBe(0); }); + + // F6 — the empty-chain early return in runReplayVerify fired unconditionally, + // before runReplaySnapshotTier ever ran — so `--replay-snapshot` against an + // EMPTY chain reported success having compared NOTHING, even when a real + // committed snapshot on disk records tables the (empty) replay could never + // produce. Reproduces both hazards the surrounding comment names: a + // `migrate.outDir` that no longer points at the real chain, and a project + // adopted via `migrate baseline --from-db` (whose own chain is empty by + // construction). Tier 1 (`--replay`) is right to pass here — an empty chain + // trivially "applies" — but tier 2 owes a real answer, and that answer must be + // "does not reproduce the snapshot", not silence mistaken for a pass. + test("an empty chain with a real committed snapshot fails under --replay-snapshot", async () => { + const root = await project(); + await withGeneratedChain(root); // writes a chain AND a snapshot recording "jobs" + + // Empty the chain while leaving the committed snapshot (a sibling file, not + // inside any migration folder) untouched — the exact shape of a stale + // `migrate.outDir` or a baseline-from-db adoption. + const migrationsDir = join(root, ".metaobjects", "migrations"); + for (const entry of await readdir(migrationsDir)) { + if ((await stat(join(migrationsDir, entry))).isDirectory()) { + await rm(join(migrationsDir, entry), { recursive: true, force: true }); + } + } + + expect(await verifyCommand(["--replay"], root)).toBe(0); // tier 1: trivially applies + expect(await verifyCommand(["--replay-snapshot"], root)).toBe(1); // tier 2: must not pass vacuously + }); }); diff --git a/server/typescript/packages/sdk/src/metadata-files.ts b/server/typescript/packages/sdk/src/metadata-files.ts index bcddc4679..3cc1ad54e 100644 --- a/server/typescript/packages/sdk/src/metadata-files.ts +++ b/server/typescript/packages/sdk/src/metadata-files.ts @@ -15,7 +15,7 @@ // sides import is the fix; a lazy `await import()` inside `loadMemory` is not // — that hides the cycle rather than removing it. import { extname, join } from "node:path"; -import { readdir, stat } from "node:fs/promises"; +import { readdir, realpath, stat } from "node:fs/promises"; /** * The DEFAULT value of `sources` — the directory scanned when @@ -86,10 +86,42 @@ const PENDING_DIR = "_pending"; * read the directory itself still throws — that is the "you have no metadata * here" case callers report. * + * A symlink CYCLE (e.g. `metaobjects/link -> ..`) is a loud error rather than + * unbounded recursion — see {@link listMetadataFilesGuarded}. + * * Format selection (parsing) happens downstream in `FileSource` from * `@metaobjectsdev/metadata`, which infers the parser from file extension. */ export async function listMetadataFiles(dir: string): Promise { + return listMetadataFilesGuarded(dir, new Set()); +} + +/** + * {@link listMetadataFiles}'s recursive worker, carrying the REAL (symlink- + * resolved) ancestor directories already on this walk branch. + * + * This walk follows symlinked directories on purpose (`stat`, not `lstat`, + * below — matching `DirectorySource` in `@metaobjectsdev/metadata`), so an + * unguarded directory symlink that revisits an ancestor recurses forever: + * Java and Python both added this exact guard when this PR promoted + * symlink-following to a cross-port contract; the TypeScript reference itself + * did not, even though the corpus cites it as authoritative. `ancestors` is + * extended only on the recursive call (never mutated in place), so it + * reflects the current branch, not siblings visited earlier at the same + * level — a directory legitimately reachable via two different symlinked + * paths (not a cycle) is not falsely rejected. + */ +async function listMetadataFilesGuarded(dir: string, ancestors: ReadonlySet): Promise { + // `realpath` failing (e.g. `dir` vanished between being listed and now) is + // not this guard's problem — fall back to the given path and let `readdir` + // below raise its own coded error. + const real = await realpath(dir).catch(() => dir); + if (ancestors.has(real)) { + throw new Error(`symlink loop detected while expanding metadata directory: ${dir} revisits ${real}`); + } + const nextAncestors = new Set(ancestors); + nextAncestors.add(real); + let entries: string[]; try { entries = await readdir(dir); @@ -120,7 +152,7 @@ export async function listMetadataFiles(dir: string): Promise { // Recurse into subdirectories after collecting files at this level. // `subdirs` is already in sorted order (built from the sorted `entries` above). for (const sub of subdirs) { - paths.push(...(await listMetadataFiles(sub))); + paths.push(...(await listMetadataFilesGuarded(sub, nextAncestors))); } return paths; } diff --git a/server/typescript/packages/sdk/test/sources.test.ts b/server/typescript/packages/sdk/test/sources.test.ts index 07f503686..ac2d10bf5 100644 --- a/server/typescript/packages/sdk/test/sources.test.ts +++ b/server/typescript/packages/sdk/test/sources.test.ts @@ -124,6 +124,19 @@ describe("resolveSources", () => { expect(out).toHaveLength(2); }); + test("a symlink cycle raises a clear error, not a hang or a stack overflow", async () => { + // F7 — the reference walk (`listMetadataFiles`, `metadata-files.ts`) follows a + // symlinked directory via `stat` (not `lstat`) with no loop guard. This PR + // promoted symlink-following to a cross-port contract and Java/Python both + // added cycle detection when they picked it up; the reference itself did not. + // `model/loop -> model` is self-referential: walking into `loop` re-lists + // `model` (now reached as `model/loop`), which contains `loop` again, forever. + write("model/meta.a.json"); + const { symlinkSync } = await import("node:fs"); + symlinkSync(join(root, "model"), join(root, "model/loop"), "dir"); + await expect(resolveSources(root, [{ path: "model" }])).rejects.toThrow(); + }); + test("a dangling symlink inside a source directory is skipped, not a raw ENOENT crash", async () => { // DirectorySource in @metaobjectsdev/metadata catches and skips exactly // this case (directory-source.ts). Before the fix, the bare `stat()` in From 75137163993dc75a185cdf791a11920e44741821 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 21:42:32 -0400 Subject: [PATCH 5/6] fix(migrate-ts): drop-fk/drop-check/constraint-backed drop-index survive a replay against an unmanaged table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F10 — the #313 guarantee ("a committed chain must apply to a virgin database") was partial for three change kinds. `DROP CONSTRAINT IF EXISTS` guards the constraint NAME, but Postgres still requires the enclosing `ALTER TABLE`'s TABLE to exist just to parse the statement — so a table another tool owns (never created by any migration in the chain) still killed the replay with `relation "x" does not exist` for `drop-fk`, `drop-check`, and the constraint-backed `drop-index` arm. All three now also carry `ALTER TABLE IF EXISTS`, closing the gap the same way `DROP TABLE IF EXISTS` already does for whole-table drops. Separately, `--allow drop-unmanaged`'s provenance guard (`snapshotAbsentDrops` in cli/migrate.ts) only inspected `drop-table`/`drop-view`, so authoring one of these three constraint-level drops against an unmanaged object required no permission at all — the diff went through silently even though the resulting migration could not replay. The guard now also checks `drop-fk`/`drop-check`/ `drop-index` at the CONSTRAINT grain: a table can be fully managed while carrying a constraint another tool added directly, and that constraint's absence from the snapshotted table's own `foreignKeys`/`checks`/`indexes` is exactly the same "never claimed" signal as a whole table missing from `snapshot.tables`. Verified non-vacuous two ways. The SQL-emission half: a real Postgres engine (PGlite, in-process — `replay-emitted-chain.test.ts`) applying an emitted chain that creates one table and drops an fk/check/constraint-backed-index on a table the chain never created — reverted, watched it fail with the actual `relation "theirs" does not exist` error, restored, watched it apply cleanly. The provenance-guard half: a full `meta migrate --from-db` run (SQLite) over a live FK the metadata never declares — reverted, watched it write the migration with exit 0 and no refusal at all, restored, watched it refuse with exit 2 (and `--allow drop-unmanaged` let it through). Four pre-existing unit tests were pinning the old bare `ALTER TABLE "table" DROP CONSTRAINT IF EXISTS` text as expected output; updated alongside the fix. Co-Authored-By: Claude Opus 5 (1M context) --- .../packages/cli/src/commands/migrate.ts | 55 +++++--- .../cli/test/migrate-drop-unmanaged.test.ts | 119 ++++++++++++++++++ .../packages/migrate-ts/src/emit/postgres.ts | 15 ++- .../check-evolution/drop-check-down.test.ts | 5 +- .../test/check/emit-postgres-check.test.ts | 5 +- .../test/emit-drop-if-exists.test.ts | 10 +- .../integrity/replay-emitted-chain.test.ts | 42 +++++++ .../test/unit/emit-postgres.test.ts | 20 ++- 8 files changed, 247 insertions(+), 24 deletions(-) diff --git a/server/typescript/packages/cli/src/commands/migrate.ts b/server/typescript/packages/cli/src/commands/migrate.ts index 5becfd328..5e1817766 100644 --- a/server/typescript/packages/cli/src/commands/migrate.ts +++ b/server/typescript/packages/cli/src/commands/migrate.ts @@ -49,6 +49,7 @@ import { type D1Runner, type SchemaProvenance, type SchemaSnapshot, + type TableDescriptor, } from "@metaobjectsdev/migrate-ts"; import { buildWranglerExecuteArgs, @@ -303,8 +304,9 @@ function summarizeChanges(changes: Change[]): Record { } /** - * The qualified names of tables/views this diff proposes to DROP that the committed - * snapshot never contained — i.e. objects this toolchain never managed (#313). + * The qualified names of tables/views/constraints this diff proposes to DROP that + * the committed snapshot never contained — i.e. objects this toolchain never + * managed (#313). * * FAILS OPEN. No snapshot on disk, or one that cannot be read, yields an empty list: * a project that has never generated one is not in an error state, and refusing there @@ -312,10 +314,21 @@ function summarizeChanges(changes: Change[]): Record { * migrate's own error to raise elsewhere, with its own message, not a silent refusal * here. * - * Names come from `qualifiedDbName` and nothing else. Three independent sets already - * have to agree on this spelling — the diff's identity maps, the `@unmanaged` - * exclusion set, and the out-of-scope set — and a fourth encoding of "absent schema - * means public" would silently un-guard every object it disagreed about. + * Table/view names come from `qualifiedDbName` and nothing else. Three independent + * sets already have to agree on this spelling — the diff's identity maps, the + * `@unmanaged` exclusion set, and the out-of-scope set — and a fourth encoding of + * "absent schema means public" would silently un-guard every object it disagreed + * about. + * + * `drop-fk`/`drop-check`/constraint-backed `drop-index` are checked at the + * CONSTRAINT grain, not just the table's: the emitter's `IF EXISTS` on the + * enclosing `ALTER TABLE` (the SQL half of this same #313 gap) only stops the + * replay from failing outright — it says nothing about whether AUTHORING the + * drop was ever supposed to be permission-free. A table can be fully managed + * while carrying a constraint another tool added directly against the live + * database; that constraint's name is absent from the snapshotted table's own + * `foreignKeys`/`checks`/`indexes`, exactly like a whole unmanaged table is + * absent from `snapshot.tables`. */ async function snapshotAbsentDrops(changes: Change[], snapPath: string): Promise { let snapshot: SchemaSnapshot | null; @@ -326,17 +339,31 @@ async function snapshotAbsentDrops(changes: Change[], snapPath: string): Promise } if (snapshot === null) return []; - const managed = new Set(); - for (const t of snapshot.tables) managed.add(qualifiedDbName(t)); - for (const v of snapshot.views) managed.add(qualifiedDbName(v)); + const managedTables = new Map(); + for (const t of snapshot.tables) managedTables.set(qualifiedDbName(t), t); + const managedViews = new Set(); + for (const v of snapshot.views) managedViews.add(qualifiedDbName(v)); const absent: string[] = []; for (const c of changes) { - const name = - c.kind === "drop-table" ? qualifiedDbName({ name: c.table, schema: c.schema }) - : c.kind === "drop-view" ? qualifiedDbName({ name: c.view, schema: c.schema }) - : undefined; - if (name !== undefined && !managed.has(name)) absent.push(name); + if (c.kind === "drop-table") { + const name = qualifiedDbName({ name: c.table, schema: c.schema }); + if (!managedTables.has(name)) absent.push(name); + } else if (c.kind === "drop-view") { + const name = qualifiedDbName({ name: c.view, schema: c.schema }); + if (!managedViews.has(name)) absent.push(name); + } else if (c.kind === "drop-fk" || c.kind === "drop-check" || c.kind === "drop-index") { + const tableName = qualifiedDbName({ name: c.table, schema: c.schema }); + const table = managedTables.get(tableName); + const constraintName = c.kind === "drop-fk" ? c.fk : c.kind === "drop-check" ? c.check : c.index; + const recorded = + c.kind === "drop-fk" ? table?.foreignKeys + : c.kind === "drop-check" ? table?.checks + : table?.indexes; + if (recorded === undefined || !recorded.some((d) => d.name === constraintName)) { + absent.push(`${tableName}.${constraintName}`); + } + } } return absent; } diff --git a/server/typescript/packages/cli/test/migrate-drop-unmanaged.test.ts b/server/typescript/packages/cli/test/migrate-drop-unmanaged.test.ts index bc326014c..fa550fab7 100644 --- a/server/typescript/packages/cli/test/migrate-drop-unmanaged.test.ts +++ b/server/typescript/packages/cli/test/migrate-drop-unmanaged.test.ts @@ -145,3 +145,122 @@ describe("meta migrate refuses a snapshot-absent drop", () => { expect(await run(f)).toBe(0); }); }); + +// F10 — the guard above only inspected `drop-table`/`drop-view`, so a `drop-fk` +// (or `drop-check`/constraint-backed `drop-index`) on a table the committed +// snapshot never recorded that FK for slid through with no permission check at +// all — even though the resulting migration cannot replay against a fresh +// database (the emit fix in `emit/postgres.ts` closes the SQL half of #313; +// this is the "the diff isn't even refused" half named alongside it). +describe("meta migrate refuses a snapshot-absent FK drop (F10)", () => { + const FK_MODEL = JSON.stringify({ + "metadata.root": { + package: "acme::platform", + children: [ + { + "object.entity": { + name: "Job", + children: [ + { "source.rdb": { name: "src", "@table": "jobs" } }, + { "field.long": { name: "id" } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }, + { + "object.entity": { + name: "Order", + children: [ + { "source.rdb": { name: "src", "@table": "orders" } }, + { "field.long": { name: "id" } }, + // A plain field, not a declared relationship — the live FK below is + // extra beyond anything the metadata asks for, exactly the "another + // tool owns this constraint" shape. + { "field.long": { name: "jobId" } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }, + ], + }, + }); + + /** + * `jobs` + `orders`, where `orders.job_id` carries a live FK to `jobs` that the + * metadata never declares — so the diff proposes `drop-fk`. `seedSnapshot` + * decides whether the committed snapshot recorded that FK as part of `orders`' + * own shape (`fk-managed`, what a prior migration in THIS chain creating it + * would produce) or not (`fk-unmanaged`, another tool's constraint). + */ + async function fkFixture(seedSnapshot: "fk-managed" | "fk-unmanaged"): Promise { + const root = await mkdtemp(join(tmpdir(), "drop-unmanaged-fk-")); + dirs.push(root); + await mkdir(join(root, "metaobjects"), { recursive: true }); + await writeFile(join(root, "metaobjects", "meta.platform.json"), FK_MODEL, "utf8"); + await mkdir(join(root, ".metaobjects", "migrations"), { recursive: true }); + await writeFile( + join(root, ".metaobjects", "config.json"), + JSON.stringify({ schema_version: 1, migrate: { dialect: "sqlite" } }), + "utf8", + ); + + const db = join(root, "t.db"); + const k = await buildKyselyFromUrl(`file:${db}`, "sqlite"); + try { + await sql`CREATE TABLE "jobs" (id INTEGER NOT NULL PRIMARY KEY)`.execute(k.db); + await sql`CREATE TABLE "orders" (id INTEGER NOT NULL PRIMARY KEY, job_id INTEGER REFERENCES "jobs"(id))`.execute(k.db); + } finally { + await k.close(); + } + + const jobsTable: TableDescriptor = { + name: "jobs", columns: [{ name: "id", sqlType: { kind: "integer", bits: 64 }, nullable: false }], + indexes: [], foreignKeys: [], checks: [], primaryKey: ["id"], + }; + const ordersColumns = [ + { name: "id", sqlType: { kind: "integer", bits: 64 } as const, nullable: false }, + { name: "job_id", sqlType: { kind: "integer", bits: 64 } as const, nullable: true }, + ]; + // Matches the synthesized-name convention both introspect/sqlite.ts and + // expected-schema.ts use for an unnamed sqlite FK: `__fk`. + const fk = { name: "orders_job_id_fk", columns: ["job_id"], refTable: "jobs", refColumns: ["id"] }; + const ordersTable: TableDescriptor = { + name: "orders", columns: ordersColumns, indexes: [], checks: [], primaryKey: ["id"], + foreignKeys: seedSnapshot === "fk-managed" ? [fk] : [], + }; + + await writeSnapshot(snapshotPath(join(root, ".metaobjects", "migrations"), "sqlite"), { + tables: [jobsTable, ordersTable], + views: [], + }); + return { root, db }; + } + + const runFk = (f: Fixture, extra: string[] = []): Promise => + migrateCommand( + ["--from-db", "--db", `file:${f.db}`, "--dialect", "sqlite", "--slug", "x", + "--allow", ["drop-fk", ...extra].join(",")], + f.root, + ); + + test("refuses, and writes nothing", async () => { + const f = await fkFixture("fk-unmanaged"); + expect(await runFk(f)).toBe(2); + expect(await migrationCount(f.root)).toBe(0); + }); + + test("--allow drop-unmanaged lets it through", async () => { + const f = await fkFixture("fk-unmanaged"); + expect(await runFk(f, ["drop-unmanaged"])).toBe(0); + expect(await migrationCount(f.root)).toBe(1); + }); + + // The non-false-fire: a chain removing an FK it created itself (recorded in + // its own snapshot) needs no special permission beyond the ordinary `drop-fk` + // gate — this is what keeps the guard from breaking a normal FK removal. + test("a drop for an FK the snapshot DOES record proceeds without the flag", async () => { + const f = await fkFixture("fk-managed"); + expect(await runFk(f)).toBe(0); + expect(await migrationCount(f.root)).toBe(1); + }); +}); diff --git a/server/typescript/packages/migrate-ts/src/emit/postgres.ts b/server/typescript/packages/migrate-ts/src/emit/postgres.ts index 68ea5d2ec..a44504a85 100644 --- a/server/typescript/packages/migrate-ts/src/emit/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/emit/postgres.ts @@ -129,12 +129,20 @@ function renderUp(c: Change): string { // schema adopted from Drizzle has constraint-backed unique indexes. // Both arms carry the #313 `IF EXISTS`: they are two renderings of the SAME // `drop-index` change, and guarding one would leave the change kind half-covered. + // The constraint-backed arm ALSO guards the enclosing `ALTER TABLE` (not just + // the constraint name) — see the `drop-fk`/`drop-check` comment below for why. case "drop-index": return c.restore?.constraint !== undefined - ? `ALTER TABLE ${quoteQualified(c.table, c.schema)} DROP CONSTRAINT IF EXISTS ${quote(c.index)};` + ? `ALTER TABLE IF EXISTS ${quoteQualified(c.table, c.schema)} DROP CONSTRAINT IF EXISTS ${quote(c.index)};` : `DROP INDEX IF EXISTS ${quoteIndexQualified(c.index, c.schema)};`; case "add-fk": return renderAddFk(c.table, c.schema, c.fk); - case "drop-fk": return `ALTER TABLE ${quoteQualified(c.table, c.schema)} DROP CONSTRAINT IF EXISTS ${quote(c.fk)};`; + // #313 (constraint-level): `DROP CONSTRAINT IF EXISTS` alone only guards the + // constraint NAME — Postgres still requires the TABLE to exist to parse an + // `ALTER TABLE` at all, so a table another tool owns (never created by any + // migration in this chain) still killed the replay with `relation "x" does + // not exist`. Postgres supports `ALTER TABLE IF EXISTS` directly; using it + // closes the gap the same way `DROP TABLE IF EXISTS` above already does. + case "drop-fk": return `ALTER TABLE IF EXISTS ${quoteQualified(c.table, c.schema)} DROP CONSTRAINT IF EXISTS ${quote(c.fk)};`; // `drop-check` IS produced by the diff — diff/index.ts:579 and :592 both push it, // and an evolved `field.enum @values` is a live producer. (A comment here used to // claim these arms were unreachable "declared, not yet produced" stubs; that was @@ -148,7 +156,8 @@ function renderUp(c: Change): string { // references the dropped constraint. SQLite is already replay-safe by construction; // guarding Postgres makes the two dialects agree. case "add-check": return `ALTER TABLE ${quoteQualified(c.table, c.schema)} ADD CONSTRAINT ${quote(c.check.name)} CHECK (${c.check.expression});`; - case "drop-check": return `ALTER TABLE ${quoteQualified(c.table, c.schema)} DROP CONSTRAINT IF EXISTS ${quote(c.check)};`; + // Same `ALTER TABLE IF EXISTS` gap as `drop-fk` above. + case "drop-check": return `ALTER TABLE IF EXISTS ${quoteQualified(c.table, c.schema)} DROP CONSTRAINT IF EXISTS ${quote(c.check)};`; case "create-view": return renderCreateView(c.view, c.schema, /* orReplace */ false); case "drop-view": return renderDropView(c); case "replace-view": return renderCreateView(c.view, c.schema, /* orReplace */ true); diff --git a/server/typescript/packages/migrate-ts/test/check-evolution/drop-check-down.test.ts b/server/typescript/packages/migrate-ts/test/check-evolution/drop-check-down.test.ts index 485adb6b0..43b31b69e 100644 --- a/server/typescript/packages/migrate-ts/test/check-evolution/drop-check-down.test.ts +++ b/server/typescript/packages/migrate-ts/test/check-evolution/drop-check-down.test.ts @@ -9,8 +9,9 @@ describe("drop-check: restore down + allow gating", () => { test("drop-check with restore → down re-adds the constraint", () => { const c = { kind: "drop-check", table: "orders", check: CHK.name, restore: CHK, status: { state: "allowed" } } as unknown as Change; const r = emit([c], { dialect: "postgres" }); - // #313 — the forward drop carries IF EXISTS; the down (asserted below) stays bare. - expect(r.up).toContain(`ALTER TABLE "orders" DROP CONSTRAINT IF EXISTS "orders_qty_numeric_chk";`); + // #313 — the forward drop carries IF EXISTS on both the constraint AND the + // enclosing ALTER TABLE (F10); the down (asserted below) stays bare. + expect(r.up).toContain(`ALTER TABLE IF EXISTS "orders" DROP CONSTRAINT IF EXISTS "orders_qty_numeric_chk";`); expect(r.down).toContain(`ALTER TABLE "orders" ADD CONSTRAINT "orders_qty_numeric_chk" CHECK (qty >= 1);`); }); test("drop-check is blocked unless allow.dropCheck", () => { diff --git a/server/typescript/packages/migrate-ts/test/check/emit-postgres-check.test.ts b/server/typescript/packages/migrate-ts/test/check/emit-postgres-check.test.ts index 52e03a2d6..db3b0c714 100644 --- a/server/typescript/packages/migrate-ts/test/check/emit-postgres-check.test.ts +++ b/server/typescript/packages/migrate-ts/test/check/emit-postgres-check.test.ts @@ -24,6 +24,9 @@ describe("emit postgres — checks", () => { const r = emit([{ kind: "drop-check", table: "orders", check: "orders_status_chk", status: ALLOWED } as unknown as Change], { dialect: "postgres" }); // #313 — the forward drop carries IF EXISTS. The add-check test above asserts the // matching DOWN, which stays bare; the two together pin the direction split. - expect(r.up).toContain(`ALTER TABLE "orders" DROP CONSTRAINT IF EXISTS "orders_status_chk";`); + // F10 — `IF EXISTS` on the enclosing `ALTER TABLE` too, not just the + // constraint name: `DROP CONSTRAINT IF EXISTS` alone still requires the + // TABLE to exist, so a table another tool owns still failed the replay. + expect(r.up).toContain(`ALTER TABLE IF EXISTS "orders" DROP CONSTRAINT IF EXISTS "orders_status_chk";`); }); }); diff --git a/server/typescript/packages/migrate-ts/test/emit-drop-if-exists.test.ts b/server/typescript/packages/migrate-ts/test/emit-drop-if-exists.test.ts index c45ac7c59..a51223830 100644 --- a/server/typescript/packages/migrate-ts/test/emit-drop-if-exists.test.ts +++ b/server/typescript/packages/migrate-ts/test/emit-drop-if-exists.test.ts @@ -36,6 +36,10 @@ describe("forward drops tolerate an absent object (#313)", () => { expect(up).toContain('DROP INDEX IF EXISTS "idx_gone";'); }); + // F10 — `DROP CONSTRAINT IF EXISTS` alone only guards the constraint NAME; + // the enclosing `ALTER TABLE` needs its own `IF EXISTS` too, or a table this + // toolchain never created (only #285's constraint-backed index sits on it) + // still fails the replay with `relation "t" does not exist`. test("postgres drop-index, constraint-backed (#285)", () => { const { up } = renderPostgres([ { @@ -46,12 +50,12 @@ describe("forward drops tolerate an absent object (#313)", () => { restore: { name: "uq_gone", columns: ["a"], unique: true, constraint: "unique" }, }, ]); - expect(up).toContain('ALTER TABLE "t" DROP CONSTRAINT IF EXISTS "uq_gone";'); + expect(up).toContain('ALTER TABLE IF EXISTS "t" DROP CONSTRAINT IF EXISTS "uq_gone";'); }); test("postgres drop-fk", () => { const { up } = renderPostgres([{ kind: "drop-fk", table: "t", fk: "fk_gone", status: ALLOWED }]); - expect(up).toContain('ALTER TABLE "t" DROP CONSTRAINT IF EXISTS "fk_gone";'); + expect(up).toContain('ALTER TABLE IF EXISTS "t" DROP CONSTRAINT IF EXISTS "fk_gone";'); }); // drop-check IS produced by the diff (diff/index.ts:579, :592) — an evolved @@ -61,7 +65,7 @@ describe("forward drops tolerate an absent object (#313)", () => { const { up } = renderPostgres([ { kind: "drop-check", table: "t", check: "t_qty_chk", status: ALLOWED }, ]); - expect(up).toContain('ALTER TABLE "t" DROP CONSTRAINT IF EXISTS "t_qty_chk";'); + expect(up).toContain('ALTER TABLE IF EXISTS "t" DROP CONSTRAINT IF EXISTS "t_qty_chk";'); }); test("sqlite drop-table", () => { diff --git a/server/typescript/packages/migrate-ts/test/integrity/replay-emitted-chain.test.ts b/server/typescript/packages/migrate-ts/test/integrity/replay-emitted-chain.test.ts index 5206cd2a1..5f0499633 100644 --- a/server/typescript/packages/migrate-ts/test/integrity/replay-emitted-chain.test.ts +++ b/server/typescript/packages/migrate-ts/test/integrity/replay-emitted-chain.test.ts @@ -88,6 +88,48 @@ describe("an EMITTED chain applies to an empty database (#313)", () => { } }); + // F10 — the CONSTRAINT-level analogue of the REPORTED case above: `theirs` is + // never created by this chain, and the diff proposes dropping an FK, a CHECK, + // and a constraint-backed (unique) index on it — the three change kinds whose + // renderer guards the CONSTRAINT name with `IF EXISTS` but leaves the + // enclosing `ALTER TABLE` bare, so it still fails against a virgin database + // with `relation "theirs" does not exist`; the #313 guarantee was partial for + // exactly these three kinds. Postgres-only: SQLite emits no standalone + // statement for drop-fk/drop-check at all (folded into a table recreate that + // rebuilds from the EXPECTED descriptor), so it cannot exhibit this bug. + test("postgres: dropping an fk/check/constraint-backed-index on a table the chain never created still applies", async () => { + const dir = mkdtempSync(join(tmpdir(), "replay-emitted-unmanaged-constraint-")); + const engine = await openReplayEngine("postgres"); + const unmanagedConstraintDrops: Change[] = [ + { + kind: "create-table", + status: ALLOWED, + table: { + name: "mine", + columns: [{ name: "id", sqlType: { kind: "integer", bits: 64 }, nullable: false }], + indexes: [], + foreignKeys: [], + checks: [], + primaryKey: ["id"], + }, + }, + { kind: "drop-fk", table: "theirs", fk: "theirs_owner_fk", status: ALLOWED }, + { kind: "drop-check", table: "theirs", check: "theirs_status_chk", status: ALLOWED }, + { + kind: "drop-index", table: "theirs", index: "theirs_code_uniq", status: ALLOWED, + restore: { name: "theirs_code_uniq", columns: ["code"], unique: true, constraint: "unique" }, + }, + ]; + try { + await writeMigration(emit(unmanagedConstraintDrops, { dialect: "postgres" }), { dir, slug: "init" }); + const applied = await applyPending(engine.db, dir, { dryRun: false, dialect: "postgres" }); + expect(applied.applied).toHaveLength(1); + } finally { + await engine.dispose(); + rmSync(dir, { recursive: true, force: true }); + } + }); + // The control: without it the cases above could pass because `applyPending` // swallows a failing statement rather than because the emitter stopped writing // one. This proves the assertion has teeth. diff --git a/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts b/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts index afcf65795..9a9f13379 100644 --- a/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts +++ b/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts @@ -165,6 +165,19 @@ describe("renderPostgres — indexes + FKs", () => { expect(norm(up)).toBe(`DROP INDEX IF EXISTS "old_idx";`); }); + // F10 — the constraint-backed arm ALSO guards the enclosing ALTER TABLE, not + // just the constraint name (see the `drop-fk` test below for the failure mode). + test("drop-index (constraint-backed)", () => { + const changes: Change[] = [{ + kind: "drop-index", table: "work_items", index: "work_items_message_id_uniq", status: ALLOWED, + restore: { name: "work_items_message_id_uniq", columns: ["message_id"], unique: true, constraint: "unique" }, + }]; + const { up } = emit(changes, { dialect: "postgres" }); + expect(norm(up)).toBe( + `ALTER TABLE IF EXISTS "work_items" DROP CONSTRAINT IF EXISTS "work_items_message_id_uniq";`, + ); + }); + test("add-fk with ON DELETE CASCADE", () => { const changes: Change[] = [{ kind: "add-fk", table: "weeks", @@ -182,10 +195,15 @@ describe("renderPostgres — indexes + FKs", () => { ); }); + // F10 — `DROP CONSTRAINT IF EXISTS` alone only guards the constraint NAME; + // Postgres still requires the TABLE to exist to parse `ALTER TABLE` at all, so + // this failed to replay against a virgin database for a table another tool + // owns (never created by any migration in the chain) with `relation "x" does + // not exist`. `ALTER TABLE IF EXISTS` closes that gap. test("drop-fk", () => { const changes: Change[] = [{ kind: "drop-fk", table: "weeks", fk: "weeks_program_id_fk", status: ALLOWED }]; const { up } = emit(changes, { dialect: "postgres" }); - expect(norm(up)).toBe(`ALTER TABLE "weeks" DROP CONSTRAINT IF EXISTS "weeks_program_id_fk";`); + expect(norm(up)).toBe(`ALTER TABLE IF EXISTS "weeks" DROP CONSTRAINT IF EXISTS "weeks_program_id_fk";`); }); }); From bebc6e413f5329a9d8a201da0c9b3cec0c8332de Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 19 Aug 2026 21:52:22 -0400 Subject: [PATCH 6/6] fix(conformance): match the reference's content-sorted spec resolution order; retire the vacuous unknown-keys case name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F12 — Java/C#/Python's SourceResolver.resolveSources processed declared source specs in DECLARED order, while each port's own comment claimed to mirror the TypeScript reference's `orderedPathSpecs` (`sources.ts`), which kind-validates in declared order but then resolves in CONTENT order (`JSON.stringify(spec)`, ascending — for a validated `path`-only spec this reduces to an ordinal sort of the path string). The three ports' comments were half-true: they implemented the whole-list-kind-validation half but not the sort. With two simultaneously-unresolvable declared paths, TS's ERR_SOURCE_UNRESOLVED names the content-first one; the other three named whichever was declared first instead. Verified this changes ONLY which of several unresolvable paths gets named in the raised error, never the resolved file SET: de-duplication into the result is order-independent by construction (a Map/LinkedHashSet/HashSet keyed on normalized path), and file order was already outside the cross-port contract per the corpus README. All three now sort the validated path specs ordinally before Pass 2 (matching JS's UTF-16 code-unit string comparison — Java's `Collections.sort` on `String`, C#'s `StringComparer.Ordinal`, Python's `sorted(..., key=...)` on `str` all agree with it for the ASCII paths in scope). A new unit test per port (mirroring an empirical probe against the TS reference) declares two unresolvable paths out of content order and asserts the content-first one is named; reverted each fix, confirmed the wrong path was named (RED), restored, confirmed the content-first one is (GREEN). Full per-port suites (Python 1700, Java 1476, C# 934) stay green, so no other behavior depends on the prior declared-order processing. F13 — the `unknown-top-level-keys-are-ignored` corpus case is vacuous for TypeScript: all four keys it supplies (`pending_in_git`, `confidence_thresholds`, `extract`, `migrate`) are TS's OWN recognized top-level config keys, so TS's `ConfigSchema` (`config.ts`, `.strict()`) passes the case by RECOGNIZING them, not by ignoring an unknown key — the other three ports genuinely don't know these keys and correctly ignore them, but the case can't tell that apart from TS's different reason for the same outcome. Renamed to `typescript-owned-top-level-keys-do-not-affect-source-resolution` (inputs unchanged — not weakened) and the README section rewritten to state the narrower, TRUE claim precisely. A genuinely unrecognized key (e.g. `"foo": 1`, unknown to all four ports) IS a real, confirmed cross-port divergence, verified empirically: `resolveCollection` → `loadConfig` → `ConfigSchema.parse` is `.strict()` at the top level, so an unrecognized key throws a ZodError before source resolution is ever reached, while Java/C#/Python all resolve successfully, silently ignoring it. NOT added as a shared corpus case and NOT fixed: doing either would mean changing the reference implementation (`config.ts`/`collection.ts`, explicitly out of scope) — loosening `ConfigSchema`'s top-level strictness has a blast radius well beyond source resolution (every `loadConfig` caller), which is a deliberate call for a human to make, not one this pass should make unilaterally. Documented in the README as an open, human-reviewable follow-up rather than silently dropped. Co-Authored-By: Claude Opus 5 (1M context) --- .../source-resolution-conformance/README.md | 33 ++++++++- .../source-resolution-conformance/cases.json | 2 +- .../SourceResolverTests.cs | 45 ++++++++++++ .../MetaObjects/Config/SourceResolver.cs | 15 +++- .../metaobjects/config/SourceResolver.java | 12 ++++ .../config/SourceResolverTest.java | 68 +++++++++++++++++++ .../src/metaobjects/config/source_resolver.py | 11 ++- .../tests/config/test_source_resolver.py | 14 ++++ 8 files changed, 194 insertions(+), 6 deletions(-) create mode 100644 server/csharp/MetaObjects.Conformance.Tests/SourceResolverTests.cs create mode 100644 server/java/metadata/src/test/java/com/metaobjects/config/SourceResolverTest.java diff --git a/fixtures/source-resolution-conformance/README.md b/fixtures/source-resolution-conformance/README.md index 2ff6be920..20ac2254f 100644 --- a/fixtures/source-resolution-conformance/README.md +++ b/fixtures/source-resolution-conformance/README.md @@ -93,9 +93,36 @@ README.md loop) reports whichever error comes first in declaration order instead, diverging on exactly one of the two cases depending on which order it happens to process first. -- **Unknown top-level config keys are IGNORED.** The file carries - TypeScript-owned keys no other port models. `schema_version` and `sources` are - the neutral subset; each port validates those strictly and ignores the rest. +- **A TypeScript-owned top-level key does not affect source resolution in any + port.** `schema_version` and `sources` are the neutral subset every port + models; `pending_in_git` / `confidence_thresholds` / `extract` / `migrate` + are TypeScript's own, and `typescript-owned-top-level-keys-do-not-affect- + source-resolution` pins that their presence resolves the same file set + everywhere. Read that case name literally — it is narrower than "unknown + keys are ignored" on purpose. Those four keys are UNKNOWN to Java/C#/Python + (which ignore any key outside `schema_version`/`sources`, by design) but + KNOWN to TypeScript's own `ConfigSchema` (`sdk/src/config.ts`), which + recognizes and validates them as part of its own project state. A case + built only from keys TS recognizes cannot tell "TS ignored this because it + doesn't affect resolution" apart from "TS ignored this because it doesn't + affect resolution AND happened to also validate it" — the two are + indistinguishable from the outside, and only the first is what every other + port's "ignore the rest" behavior demonstrates. + **A genuinely unrecognized key (e.g. `"foo": 1`, unknown to all four ports) + is a real, confirmed, cross-port DIVERGENCE, not covered by this corpus.** + Verified empirically: `resolveCollection` (`collection.ts`) calls + `loadConfig`, which parses the WHOLE file through `ConfigSchema.parse` — + `.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. ## Order is deliberately NOT pinned diff --git a/fixtures/source-resolution-conformance/cases.json b/fixtures/source-resolution-conformance/cases.json index 4ed9faafc..aa535f582 100644 --- a/fixtures/source-resolution-conformance/cases.json +++ b/fixtures/source-resolution-conformance/cases.json @@ -178,7 +178,7 @@ "expectError": "ERR_COLLECTION_NOT_FOUND" }, { - "name": "unknown-top-level-keys-are-ignored", + "name": "typescript-owned-top-level-keys-do-not-affect-source-resolution", "tree": { "model/meta.a.json": "{\"metadata.root\":{\"children\":[]}}" }, diff --git a/server/csharp/MetaObjects.Conformance.Tests/SourceResolverTests.cs b/server/csharp/MetaObjects.Conformance.Tests/SourceResolverTests.cs new file mode 100644 index 000000000..99b131623 --- /dev/null +++ b/server/csharp/MetaObjects.Conformance.Tests/SourceResolverTests.cs @@ -0,0 +1,45 @@ +using System.Collections.Generic; +using System.IO; +using MetaObjects; +using MetaObjects.Config; +using Xunit; + +namespace MetaObjects.Conformance.Tests; + +/// +/// Focused unit coverage for shapes +/// not gated by the shared source-resolution-conformance corpus. +/// +public sealed class SourceResolverTests +{ + // F12 — Pass 2 resolves in CONTENT order (ordinal path-string sort), not + // declared order, mirroring the TypeScript reference's `orderedPathSpecs` + // (`sources.ts`: kind-validated, then sorted by `JSON.stringify(spec)`, + // which for a validated `path`-only spec reduces to the path string alone + // — verified empirically: `resolveSources(dir, [{path:"zzz-missing"}, + // {path:"aaa-missing"}])` names "aaa-missing", the content-first one, even + // though "zzz-missing" is declared first). With BOTH paths unresolvable, + // only a port that content-sorts before Pass 2 names "aaa-missing" here; + // a declared-order implementation names "zzz-missing" instead. + [Fact] + public void TwoUnresolvablePaths_ReportsTheContentFirstOne() + { + var root = Path.Combine(Path.GetTempPath(), "source-resolver-order-" + System.Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + try + { + var specs = new List> + { + new Dictionary { ["path"] = "zzz-missing" }, + new Dictionary { ["path"] = "aaa-missing" }, + }; + var ex = Assert.Throws(() => SourceResolver.ResolveSources(root, specs)); + Assert.Contains("aaa-missing", ex.Message); + Assert.DoesNotContain("zzz-missing", ex.Message); + } + finally + { + Directory.Delete(root, recursive: true); + } + } +} diff --git a/server/csharp/MetaObjects/Config/SourceResolver.cs b/server/csharp/MetaObjects/Config/SourceResolver.cs index d4e6b0669..5f92699a0 100644 --- a/server/csharp/MetaObjects/Config/SourceResolver.cs +++ b/server/csharp/MetaObjects/Config/SourceResolver.cs @@ -24,7 +24,11 @@ public static class SourceResolver /// or missing path happens to sit first — the corpus pins that an unsupported /// KIND anywhere in the list wins over an unresolved PATH regardless of which /// is declared first (`unsupported-kind-precedes-unresolved-path-when-path-is- - /// declared-first`/`-second`). + /// declared-first`/`-second`). Pass 2 then resolves the validated specs in + /// CONTENT order (ordinal path-string sort), also matching `orderedPathSpecs` + /// — not the resolved file SET (de-dup is order-independent), only which + /// declared path's `ERR_SOURCE_UNRESOLVED` fires first when more than one is + /// simultaneously unresolvable. public static IReadOnlyList ResolveSources( string configDir, IReadOnlyList> specs) @@ -45,6 +49,15 @@ public static IReadOnlyList ResolveSources( ErrorCode.ERR_SOURCE_KIND_UNSUPPORTED); } } + // Content order (ordinal — matches the reference's UTF-16 code-unit + // comparison), not declared order — mirrors `orderedPathSpecs` in + // `sources.ts` (kind-validated, then sorted by `JSON.stringify(spec)`, + // which for a validated `path`-only spec reduces to the path string + // alone). Does not change the resolved file SET (de-dup is + // order-independent); only decides which declared path's + // `ERR_SOURCE_UNRESOLVED` fires first when more than one is + // simultaneously unresolvable. + pathSpecs.Sort(StringComparer.Ordinal); // Pass 2 — resolve each validated path spec against the filesystem. var seen = new List(); diff --git a/server/java/metadata/src/main/java/com/metaobjects/config/SourceResolver.java b/server/java/metadata/src/main/java/com/metaobjects/config/SourceResolver.java index a22ceaabb..e500b26e1 100644 --- a/server/java/metadata/src/main/java/com/metaobjects/config/SourceResolver.java +++ b/server/java/metadata/src/main/java/com/metaobjects/config/SourceResolver.java @@ -23,6 +23,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -59,6 +60,16 @@ private SourceResolver() {} * an unresolved PATH regardless of which is declared first * ({@code unsupported-kind-precedes-unresolved-path-when-path-is-declared-first}/ * {@code -second}). + * + *

Pass 2 processes the validated specs in CONTENT order (natural string + * ordering of each spec's {@code path}), not declared order — mirroring the + * reference implementation's {@code orderedPathSpecs} + * ({@code sources.ts}: kind-validated, then sorted by + * {@code JSON.stringify(spec)}, which for a validated {@code path}-only spec + * reduces to sorting by the path string alone). This does not change the + * resolved file SET — de-duplication is order-independent — only which + * declared path's {@code ERR_SOURCE_UNRESOLVED} fires first when more than one + * is simultaneously unresolvable, which order-independence alone cannot pin. */ public static List resolveSources(Path configDir, List> specs) { // Pass 1 — kind validation across the WHOLE set, no filesystem I/O yet. @@ -73,6 +84,7 @@ public static List resolveSources(Path configDir, List } pathSpecs.add(rawPath); } + Collections.sort(pathSpecs); // Pass 2 — resolve each validated path spec against the filesystem. LinkedHashSet seen = new LinkedHashSet<>(); 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 new file mode 100644 index 000000000..c33d4f108 --- /dev/null +++ b/server/java/metadata/src/test/java/com/metaobjects/config/SourceResolverTest.java @@ -0,0 +1,68 @@ +/* + * Copyright 2003 Doug Mealing LLC dba Meta Objects + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.metaobjects.config; + +import com.metaobjects.MetaDataException; +import org.junit.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +/** + * Focused unit coverage for {@link SourceResolver#resolveSources} shapes not gated + * by the shared {@code source-resolution-conformance} corpus. + */ +public class SourceResolverTest { + + private static Map pathSpec(String path) { + Map m = new LinkedHashMap<>(); + m.put("path", path); + return m; + } + + // F12 — Pass 2 resolves in CONTENT order (natural string ordering of each + // spec's `path`), not declared order, mirroring the TypeScript reference's + // `orderedPathSpecs` (`sources.ts`: kind-validated, then sorted by + // `JSON.stringify(spec)`, which for a validated `path`-only spec reduces to + // the path string alone — verified empirically: `resolveSources(dir, + // [{path:"zzz-missing"},{path:"aaa-missing"}])` names "aaa-missing", the + // content-first one, even though "zzz-missing" is declared first). With + // BOTH paths unresolvable, only a port that content-sorts before Pass 2 + // names "aaa-missing" here; a declared-order implementation names + // "zzz-missing" instead. + @Test + public void twoUnresolvablePathsReportsTheContentFirstOne() throws IOException { + Path root = Files.createTempDirectory("source-resolver-order-"); + try { + List> specs = List.of(pathSpec("zzz-missing"), pathSpec("aaa-missing")); + MetaDataException ex = assertThrows(MetaDataException.class, + () -> SourceResolver.resolveSources(root, specs)); + assertTrue("expected \"aaa-missing\" (content-first) in: " + ex.getMessage(), + ex.getMessage().contains("aaa-missing")); + assertTrue("must NOT name \"zzz-missing\" (declared-first, content-second): " + ex.getMessage(), + !ex.getMessage().contains("zzz-missing")); + } finally { + Files.delete(root); + } + } +} diff --git a/server/python/src/metaobjects/config/source_resolver.py b/server/python/src/metaobjects/config/source_resolver.py index 4f5ea94af..fd7ea7d6b 100644 --- a/server/python/src/metaobjects/config/source_resolver.py +++ b/server/python/src/metaobjects/config/source_resolver.py @@ -72,9 +72,18 @@ def resolve_sources(config_dir: Path, specs: list[dict[str, str]]) -> list[Path] # Whole-list kind validation FIRST — see `_validate_kinds`. _validate_kinds(specs) + # Resolve in CONTENT order (ordinal path-string sort), not declared order — + # mirrors `orderedPathSpecs` in `sources.ts` (kind-validated, then sorted by + # `JSON.stringify(spec)`, which for a validated `path`-only spec reduces to + # the path string alone). Does not change the resolved file SET (the `seen` + # de-dup below is order-independent); only decides which declared path's + # `ERR_SOURCE_UNRESOLVED` fires first when more than one is simultaneously + # unresolvable. + ordered_specs = sorted(specs, key=lambda s: s["path"]) + seen: dict[Path, None] = {} - for spec in specs: + for spec in ordered_specs: raw = Path(spec["path"]) target = raw if raw.is_absolute() else (config_dir / raw) diff --git a/server/python/tests/config/test_source_resolver.py b/server/python/tests/config/test_source_resolver.py index 00bab4bf8..5dfef894c 100644 --- a/server/python/tests/config/test_source_resolver.py +++ b/server/python/tests/config/test_source_resolver.py @@ -65,6 +65,20 @@ def test_kind_error_precedes_unresolved_path_regardless_of_order(tmp_path: Path) assert e_resource_first.value.code == ErrorCode.ERR_SOURCE_KIND_UNSUPPORTED +def test_two_unresolvable_paths_reports_the_content_first_one(tmp_path: Path) -> None: + # F12 — Pass 2 resolves in CONTENT order (ordinal path-string sort), not + # declared order, mirroring the TypeScript reference's `orderedPathSpecs` + # (verified empirically: `resolveSources(dir, [{path:"zzz-missing"}, + # {path:"aaa-missing"}])` names "aaa-missing", the content-first one, even + # though "zzz-missing" is declared first). With BOTH paths unresolvable, + # only the port that content-sorts before Pass 2 names "aaa-missing" here; + # a declared-order implementation would name "zzz-missing" instead. + with pytest.raises(ParseError) as e: + resolve_sources(tmp_path, [{"path": "zzz-missing"}, {"path": "aaa-missing"}]) + assert "aaa-missing" in str(e.value) + assert "zzz-missing" not in str(e.value) + + def test_collection_falls_back_to_default_dir(tmp_path: Path) -> None: (tmp_path / "metaobjects").mkdir() (tmp_path / "metaobjects" / "a.json").write_text("{}")