fix: close 15 findings from the cross-port source-resolution review - #316
Merged
Conversation
… thread config providers/libraries into docs 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).
…I, and a closed directory-walk stream
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:<absolute path>" 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) <noreply@anthropic.com>
…ed file list, not a re-walked directory 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) <noreply@anthropic.com>
… guard, and init's overwrite report 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) <noreply@anthropic.com>
…ive a replay against an unmanaged table 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) <noreply@anthropic.com>
…n order; retire the vacuous unknown-keys case name 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #315. A final code review of the merged work found 15 issues; 13 are fixed here, one was a false positive, and one surfaced a deeper divergence recorded as an open follow-up.
Two were regressions introduced by the previous fix wave, which is why this pass exists:
expand()rewrite (the symlink fix) turned a missing or non-directory source from yields nothing into an uncaughtFileNotFoundError/NotADirectoryError— a raw traceback where a coded error used to print.--replay-snapshotreturned 0 at the empty-chain early return before the snapshot tier ran, so the second tier passed vacuously whenever the migrations directory was empty or misconfigured.Cross-port divergences the corpus could not see: Python accepted
schema_version: true(True == 1) where the other three reject a non-number; Java parsed the neutral config with Gson's lenient reader, so unquoted keys and single-quoted strings loaded clean; the C# CLI computed the_pending-filtered file list, discarded it, and re-walked the directory with the loader-level source, loading drafts the other three skip.Also: the Java mojo broke on any path containing a space (
model:file:+new URI); Python'sdocsladder dropped the config'sprovidersandlibraries, sodocsfailed on a project using the shippedailibrary whilegenworked;drop-fk/drop-check/constraint-backeddrop-indexstill aborted a replay because the enclosingALTER TABLEwas unguarded; TypeScript had no symlink-cycle guard despite this work promoting symlink-following to a cross-port contract; andinit --config-onlyreported "left untouched" even when--forcehad replaced the file.Every fix was proven non-vacuous — reverted, observed RED, restored, observed GREEN — because each previous wave on this work produced at least one test that could not fail against the bug it named.
Not fixed, deliberately:
ConfigSchema.strict()rejects a genuinely-unknown config key while the other three ports ignore it. Closing that means changing the reference implementation, so it is documented rather than fixed here.Verification
TS sdk 276, cli 620, migrate-ts 808, typecheck clean across 18 packages; Python 452 config+conformance and 1700 full; C# 934 + 53; Java 1476. Shared corpus 27/27 on all four runners.
🤖 Generated with Claude Code