fix(metadata): close subpath-resolution mismatch class and add cross-package gate - #370
Merged
Merged
Conversation
…till open
An independent review of the four ungated commits on main found the /library fix
had a sibling still latent, and three claims in that work that the code does not
support. This is all of it.
**The class, gated.** `./vocabulary-rewrite-yaml` pointed at
`src/core/vocabulary-rewrite-yaml.ts` — one directory deeper than
`tsconfig.scripts.json`'s `@metaobjectsdev/metadata/*` → `src/*` substitution can
reach. Exactly the shape that took the `gates` lane down after the FR-043 merge,
and silent for the same reason: tsc misses, falls through to `node_modules`, and
reads `dist/`, which a developer has and a fresh CI checkout does not. It had not
gone red only because nothing in the scripts typecheck's include list imports it
yet. A thin `src/vocabulary-rewrite-yaml.ts` re-export fixes it; the
implementation stays under `core/` beside the canonical-JSON rewriter it mirrors.
The fix that matters is the gate. `subpath-resolves-under-scripts-paths.test.ts`
walks every `@metaobjectsdev/<pkg>/*` entry in the paths map, reads that package's
own exports, and asserts each TypeScript subpath lands on something the
substitution reaches. Asset exports (`./form.css`, whose target is the stylesheet)
are excluded by target kind, since a bundler resolves those and tsc never does.
It fails on `vocabulary-rewrite-yaml` before this commit's fix and passes after.
**Three claims corrected.** All three were mine, in work already on main; the
commits carrying them cannot be amended, so the corrections live here.
The largest: `3adfa2d76` said the old `text.includes(`"${ref}"`)` embed check was
"not currently wrong … fragile rather than broken". That is true of TypeScript,
Java and C# and FALSE of Python, where the check was VACUOUS. There are two embed
generators, not one — `generate_embedded_library.py` emits through `repr()`, which
single-quotes the key and leaves inner `"` raw, so the bare `"ai/db"` in that file
is the payload comment and never the key. Measured this time rather than inferred:
delete each key line from each real embed and the old predicate goes false on
TS/Java/C# and stays true on Python, six refs for six. So the rewrite closed a
live hole on one port, not merely a fragility on four. The same commit's claim
that the backslash exclusion is what separates key from mention holds only for
Java, whose pattern is unanchored; elsewhere the line anchor does it.
`6c39053cb` said pointing a subpath at an index is "what every other subpath
does". `./constants` is a plain `src/constants.ts` and is fine. The invariant is
that the substitution must land on something — which is what the new gate asserts,
and which a file satisfies as well as a directory.
`099d2d87c` cited "8 of 9 passed" as evidence the pre-change file was blind to the
deleted guard. That file had 7 tests; 8 of 9 is the post-change run, which that
message's own last line already reports. The pre-change figure is 7 of 7.
Also: the cross-port set comparison took `NAMES.flatMap(declaredRefs)` as a list
and compared it against a Set, so a ref legitimately declared by two layers of one
library would have failed every port against embeds that were correct. Now
de-duplicated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011cQxyVuTkdPmAduLiedNEg
… not just find a file Six findings from `/code-review high` on the previous commit. The one that mattered: the gate checked that `src/<subpath>` resolves and never compared that result to what the package's `bun` condition declares — so tsc and the runtime could read DIFFERENT modules and the gate stayed green. Proven by reverting `./library`'s `bun` to `library-sources.ts` with `index.ts` still present: the old gate passed while `paths` read the index and bun read the other file. That is the type-level form of the identity defect `tsconfig.scripts.json`'s own rationale is written against, and following `library/index.ts`'s instruction to re-export new modules there would have typechecked clean and been `undefined` at runtime. The gate now resolves both sides and compares them by realpath; the same mutation now fails it, naming both files. Three more holes in the gate itself, all of which let something escape unchecked: a subpath declaring no `bun` condition was skipped rather than failed, though it is precisely a TS subpath the scripts typecheck cannot reach through source; a subpath PATTERN (`./templates/*`) was treated as a literal and would have reported a false miss against a correct package; and a `paths` target not shaped `<pkgRoot>/src/*` made the derived manifest path miss, which `continue`d silently and dropped that whole package from the gate. Patterns are now skipped explicitly, the other two fail loudly. And a premise correction, in three headers and the commit before this one. "A directory deeper than the substitution can reach" is WRONG: `paths` `*` matches across `/`, which is why `@metaobjectsdev/codegen-ts/templates/entity-file` resolves to `src/templates/entity-file.ts` without trouble. The actual rule is that the subpath NAME must mirror the layout under `src/` — `vocabulary-rewrite-yaml` named a module living at `src/core/vocabulary-rewrite-yaml.ts`. Stated the wrong way, the next maintainer concludes a nested module can never back a subpath and flattens files out of `core/` for no reason. Renaming the export to `./core/vocabulary-rewrite-yaml` would have satisfied the rule more cheaply; the shim stays because that subpath is public API of a published package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cQxyVuTkdPmAduLiedNEg
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.
Intent
GOAL: close a latent build-breakage class that already took CI down once, and gate it so it cannot recur. Two commits on a feature branch, to land as a PR against main. The user asked explicitly for the no-mistakes gate AND a PR on this work.
BACKGROUND the diff does not show. After FR-043 Phase 1 merged (PR 369), the hosted
gateslane went red withTS2307: Cannot find module '@metaobjectsdev/metadata/library'x4 while the same gate passed on the developer machine. Cause:tsconfig.scripts.jsonmaps@metaobjectsdev/<pkg>/*to<pkg>/src/*so the repo-rootscripts/typecheck reads workspace SOURCE. When that substitution misses, tsc does not fail — it falls through to node_modules and reads the package's builtdist/, which exists for anyone who has built and does NOT exist on a fresh CI checkout where the lane runsbun installand never builds. Green locally, red on CI, and only for whoever imports the subpath first. That was fixed for./libraryin an earlier commit already on main.An INDEPENDENT REVIEW of that earlier work found the same defect still latent one subpath over, plus wrong claims in the work itself. This branch is the response.
COMMIT 1 (8502803):
./vocabulary-rewrite-yamldeclared its source atsrc/core/vocabulary-rewrite-yaml.tswhile the substitution yieldssrc/vocabulary-rewrite-yaml. Latent, not red, only because nothing in the scripts typecheck's include list imports it yet. Fixed with a thinsrc/vocabulary-rewrite-yaml.tsre-export; the implementation deliberately stays undercore/beside the canonical-JSON rewriter it mirrors. Renaming the export to./core/vocabulary-rewrite-yamlwould have been the cheaper edit and was REJECTED: this package is published (14 npm packages in lockstep, latest 1.0.3) and that subpath is public API.cli/test/subpath-resolves-under-scripts-paths.test.tswalks every@metaobjectsdev/<pkg>/*entry in the paths map, reads each package's own exports, and asserts every TypeScript subpath resolves. Asset exports (./form.css) excluded by target kind because a bundler resolves those and tsc never does.text.includes('"<ref>"')) was "fragile rather than broken". True of TypeScript, Java and C#; FALSE of Python, where it was VACUOUS. There are TWO embed generators, and the Python one emits throughrepr(), which single-quotes the key and leaves inner"raw, so the bare"ai/db"in that file is the payload comment and never the key. Measured by deleting each key line from each real embed and re-running the old predicate: false on TS/Java/C#, TRUE on Python, six refs for six.COMMIT 2 (b1d9ca9) — six findings from
/code-review high, which nm-triage required because this touches a published package.json (TIER: RISK):src/<subpath>resolves but never compared that to thebuncondition the package declares, so tsc and the runtime could read DIFFERENT modules with the gate green. Proven by reverting./library'sbuntolibrary-sources.tswithindex.tsstill present — old gate passed. It now resolves both sides and compares by realpath; that mutation now fails it naming both files.buncondition was skipped rather than failed; a subpath PATTERN (./templates/*) was treated as a literal and would false-miss a correct package; apathstarget not shaped<pkgRoot>/src/*silently dropped a whole package from the gate.paths*matches across/—@metaobjectsdev/codegen-ts/templates/entity-fileresolves tosrc/templates/entity-file.tsfine. The real rule is that the subpath NAME must mirror the layout undersrc/. Stated wrongly, a maintainer would conclude nested modules can never back a subpath and flatten files out ofcore/needlessly.WHAT I MOST WANT CHECKED: the
package.jsonexports change on a PUBLISHED package. An externalnpm installmust resolve/vocabulary-rewrite-yamlfor both ESMimportandtypes, anddist/vocabulary-rewrite-yaml.{js,d.ts}must be emitted and packaged. Its only in-repo consumer iscli/src/commands/upgrade.ts:102via dynamic import.VERIFIED before committing: metadata + cli typecheck exit 0;
bun scripts/check-scripts-typecheck.tsOK; cli suite 1127 pass / 0 fail; metadata suite 2692 pass / 0 fail;tsc -p .emitsdist/vocabulary-rewrite-yaml.{js,d.ts}; the repointed subpath loads through its public specifier andrewriteYamlDocumentis callable; both mutations (delete the guard / diverge the target) fail the gate and only the gate.DELIBERATE, do not flag: the shim file rather than an export rename (published API, stated above).
cli/test/skill-catalog-grounding.test.tsis a prose-drift gate that keys on structure — not a grep test to delete. FR-043 Phase 2 work is deliberately out of scope.REPO CONSTRAINTS: PUBLIC repo — no private or sibling project names, no absolute home paths, anywhere including commit subjects and branch names. CLAUDE.md is a SYMLINK to AGENTS.md; edit AGENTS.md, never rewrite the link. Never run a bare
bun testat the repo root — scope it.What Changed
Closed a latent build-breakage class:
@metaobjectsdev/metadata/vocabulary-rewrite-yamldeclared its source atsrc/core/vocabulary-rewrite-yaml.tsbut thetsconfig.scripts.jsonpaths mapping yieldssrc/vocabulary-rewrite-yaml. Created a shim export at the correct path while preserving the implementation undercore/, maintaining the public API surface of this published package.Added cross-package type-resolution gate: new test
subpath-resolves-under-scripts-paths.test.tswalks every@metaobjectsdev/<pkg>/*entry in the scripts typecheck's paths map, verifies each subpath's TypeScript source resolves under the mapping, and crucially asserts that tsc and the runtime read the SAME module by comparing against thebuncondition in each package'sexports.Corrected documentation claims: expanded comments in the library-manifest test to accurately describe the embed key extraction — distinguishing between the two distinct embedding generators (TS/Java/C# vs Python), explaining why the old substring-based check was vacuous on Python specifically, and clarifying that the real rule is positional (line-initial or after
.put() rather than dependent on escaping conventions.Improved manifest test robustness: converted
Array.includes()toSet.has()for O(1) lookup, de-duplicated library refs when comparing across ports (since refs can legitimately appear in multiple layers), and refined comments on the pattern-position rule that separates embed keys from payload mentions.Risk Assessment
✅ Low: Small, well-bounded fix: a re-export shim to align a public subpath with the scripts-typecheck paths mapping, plus a new gate test that verifies subpath resolution matches the declared
bunruntime target; verified the published package.json exports/files, the shim's target file, and the gate's pass/fail logic all match the stated intent with no gaps.Testing
Drove 5 distinct scenarios against the running product: the gate test suite passed with full coverage (validating missing modules, divergence detection, and unreachable subpaths), a mutation test confirmed the gate catches divergence between tsconfig.scripts.json paths and package.json bun conditions, public API imports work correctly, and all required files are in place. No issues found.
Evidence: Gate test passing output
Evidence: Mutation test divergence detection
Evidence: Test results summary
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
bun test subpath-resolves-under-scripts-paths.test.ts — 3 pass, 0 failMutation test: modified package.json bun condition to diverge from paths resolution — gate correctly detected and failedDirect import: bun -e "await import('@metaobjectsdev/metadata/vocabulary-rewrite-yaml')" — successfully imported rewriteYamlDocumentFile existence: verified re-export at src/vocabulary-rewrite-yaml.ts and implementation at src/core/vocabulary-rewrite-yaml.ts both existPackage.json structure: validated exports map with correct bun/types/default conditions for all 5 exports✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.