fix(cross-runtime): make YAML, workspace imports and std/testing resolve off Deno - #3526
Conversation
…lve off Deno `deno task test:node` failed on 284 test files. Three of the four causes are fixed here, taking it to 101 on the same machine and command. **YAML (94 files).** `jsr:@std/yaml` resolves under Deno alone. Putting `npm:yaml` in the root import map fixes resolution and breaks a product invariant: `audit-core-deps.ts:25` exempts `jsr:@std/*` and nothing else, so core may depend on Deno stdlib but never on a third party. The sanctioned split is used instead -- `npm:yaml` lives in `extensions/ext-yaml`, a `YamlParserProvider` contract sits in core, and the compat shim resolves the contract and delegates, as `platform/compat/opaque-deps.ts` already does for `DocumentExtractor`. All five call sites move onto it. The 1.2 core schema drops YAML 1.1 timestamps, `<<` merge keys and `1_000` underscore separators; each difference is pinned by a test and documented in the extension README. **Workspace member imports (72 files, partial).** Deno applies a member's own `imports` to modules inside that member's directory; the Node resolver only read the root map, so `react/`'s aliases escaped to a real package lookup. The resolver now derives scopes from the member configs it already parses, deepest match wins. This does not close the class on its own -- see below. **`#std/testing/time` and unexported subpaths (52 files).** The specifier had no local shim, so Node reported the `#` import undefined. Adds a cross-runtime `FakeTime` and the missing export paths. **A ratchet so the class cannot return.** `lint:cross-runtime-jsr` fails on a `jsr:` mapping that neither alternate harness can substitute a local file for, and on new dependents of the ones already baselined. Wired into `lint:ci`. Deno is unaffected: 3781 passed, 0 failed. `lint:core-deps`, `lint:dependency-boundaries`, `lint:ci`, `typecheck` and `fmt --check` all exit 0 -- core still carries no third-party runtime dependency. Not fixed here, and the reason `test:node` still cannot pass from a clean checkout: the `@veryfront/react-*-upstream` packages are dnt build artifacts (`scripts/build/npm-react-shims.ts`), so they exist only after `deno task build:npm`. `tests/ensure-npm-links.mjs:50` returns silently when `npm/node_modules` is absent, so the dependency is undeclared and fails open. That has to be settled before either runtime is worth gating in CI.
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds provider-based YAML parsing, cross-runtime JSR auditing, ChangesYAML parser portability
Cross-runtime JSR audit
Cross-runtime fake time
Workspace-aware Node resolution
Local React SSR test setup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
scripts/lint/audit-cross-runtime-jsr.test.ts (1)
97-112: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for two wildcard keys that share a prefix.
The current cases use keys with empty suffixes, so the second loop in
resolveTsconfigPathalways picks a compatible key. A case such as{ "#a/*.ts": "./typed/*.ts", "#a/*": "./plain/*" }resolved for"#a/x.ts"exposes the key-selection defect flagged inscripts/lint/audit-cross-runtime-jsr.tsat Line 320.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lint/audit-cross-runtime-jsr.test.ts` around lines 97 - 112, Add a test in the resolveTsconfigPath suite covering overlapping wildcard keys with a shared prefix, such as "`#a/`*.ts" and "`#a/`*", and resolve "`#a/x.ts`" to verify the more specific compatible pattern and corresponding substitution are selected. Keep the existing exact-match, longest-prefix, and unmapped-specifier cases unchanged.src/platform/compat/std/testing/time.test.ts (1)
152-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a non-finite tick duration.
The suite covers a negative duration. It does not cover
tick(NaN), which currently passes the backwards check and fires every pending timer. Add the case with the guard proposed onsrc/platform/compat/std/testing/time.tslines 101-105.As per coding guidelines: "For behavior changes, add or update a focused failing test before changing implementation."
💚 Proposed test
it("refuses to move the clock backwards", () => { using time = new FakeTime(1000); assertThrows(() => time.tick(-1), RangeError); }); + + it("refuses a non-finite tick duration", () => { + using time = new FakeTime(1000); + let fired = false; + + setTimeout(() => { + fired = true; + }, 10); + + assertThrows(() => time.tick(Number.NaN), RangeError); + assertEquals(fired, false); + assertEquals(time.now, 1000); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/compat/std/testing/time.test.ts` around lines 152 - 156, Add a focused test alongside the existing “refuses to move the clock backwards” case in the FakeTime suite that asserts time.tick(NaN) throws RangeError, covering non-finite tick durations before updating the corresponding guard in FakeTime.tick.Source: Coding guidelines
src/platform/compat/std/testing/time.ts (1)
216-228: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider an
applytrap soDate()reads the fake clock.The proxy traps
constructandget. It does not trapapply. A call toDate()withoutnewtherefore returns a string built from the real clock. Add anapplytrap if you want full parity with the faked clock.♻️ Proposed trap
return new Proxy(this.#originals.Date, { + apply: (target) => new target(readNow()).toString(), construct(target, args, newTarget) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/compat/std/testing/time.ts` around lines 216 - 228, Update `#createDate`() to add an apply trap for direct Date() calls, returning a date string based on readNow() rather than the real clock. Preserve the existing construct behavior for new Date() and get behavior for Date.now.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/ext-yaml/src/adapter.ts`:
- Around line 80-110: Update parseAllDocuments in the adapter’s YAML parsing
flow to pass schema: jsonSchema ? "json" : "core". Adjust the document
diagnostic handling so only UNRESOLVED_SCALAR diagnostics are ignored under the
JSON schema, while all other errors remain failures; preserve ordinary unquoted
metadata such as name: research and decode value: 0o7 as the string "0o7". Add
coverage for both cases.
In `@scripts/lint/audit-cross-runtime-jsr.test.ts`:
- Around line 1-2: Update the imports in the audit cross-runtime test to use
assertEquals from `#veryfront/testing/assert.ts` and describe/it from
`#veryfront/testing/bdd.ts`, matching the established testing convention used by
the related parity test.
In `@scripts/lint/audit-cross-runtime-jsr.ts`:
- Around line 299-331: Update scripts/lint/audit-cross-runtime-jsr.ts lines
299-331 so resolveTsconfigPath retains the matched prefix, suffix, and target
together during one pass, eliminating the second loop and ensuring the selected
wildcard key is used. Add a shared wildcard-suffix test case in
scripts/lint/audit-cross-runtime-jsr.test.ts lines 97-112 using keys such as
"`#a/`*.ts" and "`#a/`*". In src/config/tsconfig-paths-parity.test.ts lines 32-55,
import and use the shared resolveTsconfigPath helper instead of maintaining a
duplicate resolveThroughPaths implementation.
- Around line 464-486: Update collectCrossRuntimeFiles so the for-await
iteration over Deno.readDir(root) is inside the try/catch; ignore only
Deno.errors.NotFound and rethrow all other errors, while preserving the existing
directory traversal and file collection behavior.
In `@src/config/tsconfig-paths-parity.test.ts`:
- Around line 18-29: Update readRepoJson in the parity test to use a
runtime-neutral filesystem API that does not reference the Deno global, so Node
and Bun execute the test and preserve the existing expected coverage. Keep the
repository-root URL and JSON parsing behavior unchanged.
In `@src/extensions/parser/yaml-defaults.ts`:
- Around line 13-16: Update the first-party import in yaml-defaults.ts to use
the `#veryfront/extensions/first-party-import.ts` internal alias, while keeping
sibling imports such as ./yaml-parser.ts relative.
In `@src/platform/compat/std/testing/time.ts`:
- Around line 101-105: Update the tick method to validate that ms is finite
before computing target, rejecting NaN and positive or negative infinity with an
appropriate RangeError. Preserve the existing backwards-time check and timer
behavior for finite values.
In `@tests/node-resolver-workspace-imports.test.ts`:
- Around line 14-21: Move this test file beside resolver-hooks.mjs under the
tests/node directory, naming it resolver-hooks.test.ts, and update its relative
import from "./node/resolver-hooks.mjs" to reference the colocated module.
In `@tests/node/resolver-hooks.mjs`:
- Around line 256-265: Update resolveAliasSpecifier to resolve scope.imports
before the root import map whenever a workspace scope exists, using the root map
only when the member map has no match; retain fallbackAliasMap handling
afterward. Add a focused resolve() test in
tests/node-resolver-workspace-imports.test.ts covering a member parent URL and
colliding React specifier, before implementing the resolver change.
In `@tsconfig.json`:
- Around line 20-21: Update the `#std/testing/time` and `#std/testing/time.ts`
entries in deno.json to point to ./src/platform/compat/std/testing/time.ts,
matching the TypeScript paths and ensuring runtime resolution uses FakeTime
instead of the JSR target.
---
Nitpick comments:
In `@scripts/lint/audit-cross-runtime-jsr.test.ts`:
- Around line 97-112: Add a test in the resolveTsconfigPath suite covering
overlapping wildcard keys with a shared prefix, such as "`#a/`*.ts" and "`#a/`*",
and resolve "`#a/x.ts`" to verify the more specific compatible pattern and
corresponding substitution are selected. Keep the existing exact-match,
longest-prefix, and unmapped-specifier cases unchanged.
In `@src/platform/compat/std/testing/time.test.ts`:
- Around line 152-156: Add a focused test alongside the existing “refuses to
move the clock backwards” case in the FakeTime suite that asserts time.tick(NaN)
throws RangeError, covering non-finite tick durations before updating the
corresponding guard in FakeTime.tick.
In `@src/platform/compat/std/testing/time.ts`:
- Around line 216-228: Update `#createDate`() to add an apply trap for direct
Date() calls, returning a date string based on readNow() rather than the real
clock. Preserve the existing construct behavior for new Date() and get behavior
for Date.now.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e06c0372-a159-4178-bc54-e29eeae230bd
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
deno.jsondocs/api-reference/veryfront/extensions.mdextensions/ext-yaml/README.mdextensions/ext-yaml/THIRD_PARTY_NOTICES.mdextensions/ext-yaml/deno.jsonextensions/ext-yaml/src/adapter.test.tsextensions/ext-yaml/src/adapter.tsextensions/ext-yaml/src/index.test.tsextensions/ext-yaml/src/index.tsscripts/build/npm-package-metadata.tsscripts/lint/audit-cross-runtime-jsr.test.tsscripts/lint/audit-cross-runtime-jsr.tssrc/build/compiler/mdx-compiler/frontmatter-parser.tssrc/build/compiler/mdx-to-js.tssrc/config/tsconfig-paths-parity.test.tssrc/extensions/parser/index.tssrc/extensions/parser/yaml-defaults.tssrc/extensions/parser/yaml-parser.test.tssrc/extensions/parser/yaml-parser.tssrc/extensions/recommendations.tssrc/platform/compat/shims/std-front-matter.tssrc/platform/compat/std/front-matter-yaml.tssrc/platform/compat/std/testing/time.test.tssrc/platform/compat/std/testing/time.tssrc/platform/compat/std/yaml.test.tssrc/platform/compat/std/yaml.tssrc/react/compat/ssr-adapter/_test-setup.tssrc/rendering/rsc/server-renderer/rsc-renderer.test.tssrc/rendering/rsc/server-renderer/tree-processor.test.tstests/bun/preload.tstests/node-resolver-workspace-imports.test.tstests/node/resolver-hooks.mjstsconfig.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a51e4b008e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ng members Two review findings, both real, both reproduced before fixing. `schema: "json"` was computed and never passed to the parser, so the core schema ran and `0o7` decoded to the number 7 -- the exact widening the option exists to prevent at a Skill-document trust boundary. It is forwarded now. Forwarding it alone would have been worse than the bug. Under this schema the library raises TAG_RESOLVE_FAILED for every ordinary unquoted string, so `name: code-review` raises it twice and the existing check -- which threw on `errors[0] ?? warnings[0]` -- would have rejected every Skill document. The diagnostic is filtered per entry rather than by position, because a document that raises it also raises the real ones beside it: measured, `a: 1\na: 2` reports TAG_RESOLVE_FAILED twice before DUPLICATE_KEY, so reading the first would have hidden the duplicate. `!!binary` still decodes to a Buffer here, so the explicit-tag assertion stays load-bearing. Separately, the Node resolver consulted the root import map before a workspace member's own. Deno gives the member precedence inside that member, and the previous order silently resolved the six React specifiers that appear in both maps to the root's targets, so the aliases the member declared never applied. Checked in the direction that matters: reverting the schema forwarding fails the suite, and widening the diagnostic filter to swallow every diagnostic -- which would hide DUPLICATE_KEY -- fails it too.
`FakeTime.tick(NaN)` passed the backwards check, because `NaN < now` is false, and then `#due` treated every pending timer as due, because `due > NaN` is false too. The clock ended up NaN and every later assertion on it was quietly meaningless. Rejected now, with a test that fails without the guard. `collectCrossRuntimeFiles` guarded the `Deno.readDir` call, but readDir is lazy: a missing root raises when iteration starts, so the catch never ran. The loop is guarded instead, and only NotFound is swallowed -- a permissions or I/O failure is a reason to stop, not to audit fewer files than the caller believes were scanned. The lint's test imports its helpers from `#veryfront/testing`, as its sibling in this PR already did, and `yaml-defaults.ts` reaches `first-party-import` through the `#veryfront/` alias rather than a relative path out of its module. The new FakeTime case was written with `Deno.test` at first, which would have put `Deno.` into a file both alternate runners exclude on sight -- removing the coverage it was added for. It uses describe/it like the rest of the file, and the file names no runtime API.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/ext-yaml/src/adapter.test.ts`:
- Around line 166-172: Strengthen the test in the “still reports the real error
hiding behind those diagnostics” case by capturing the thrown SyntaxError from
parseYamlSource and asserting its message or diagnostic identifies the duplicate
key. Keep the existing JSON-schema duplicate-input setup and ensure the
assertion would fail if the earlier TAG_RESOLVE_FAILED diagnostic were exposed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c5b7e36-4acb-4575-b69c-d535d3aeff40
📒 Files selected for processing (3)
extensions/ext-yaml/src/adapter.test.tsextensions/ext-yaml/src/adapter.tstests/node/resolver-hooks.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/ext-yaml/src/adapter.ts
CodeQL was right, and it was the red check rather than infrastructure:
`best.target.replace("*", captured)` substitutes one occurrence, so a tsconfig
target carrying two wildcards kept a literal `*` and compared unequal. Uses
replaceAll.
The workspace containment check appended a literal `/` while both sides come
from pathResolve and fileURLToPath. On Windows those are backslash-separated,
so no file matched its member scope and every member map silently failed to
apply -- the platform separator is used instead.
And the duplicate-key test I added in the previous commit asserted only that a
SyntaxError was thrown, which the benign TAG_RESOLVE_FAILED preceding it would
also have satisfied. It asserts the message now, and reverting the diagnostic
filter to `errors[0]` fails it -- which the earlier version did not.
That is the third test in this branch to have passed for the wrong reason, and
the second one I wrote while fixing the first.
`tests-proxy-binary` verifies the lock is current by regenerating it and failing on any diff. Routing YAML through `ext-yaml` removed `jsr:@std/yaml` from the proxy's dependency graph, so the committed lock still named a dependency the proxy no longer has. Regenerated rather than hand-edited. The removal of that specifier is the whole change and is exactly what the extension move implies. I called this failure infrastructure twice on the strength of a `curl 404` in the setup step. It was reproducible on this branch all three times, and the failing step was "Verify proxy dependency lock is current" throughout.
The first cross-runtime PR landed the Deno-side resolution, but the verified follow-up tree still carries the CI gates and residue cleanup that make Node and Bun failures visible instead of silently passing with stale links or unresolved runtime assumptions. This keeps the already-merged #3526 behavior intact, preserves current main's filesystem adapter behavior, and regenerates the API reference from the combined tree so the public docs match the resolved exports. Constraint: PR #3526 is merged, so this branch applies only the 31e6360..6750f88bf follow-up delta onto current origin/main. Rejected: Push more changes to fix/cross-runtime-node-suite | the source PR is merged and stale. Rejected: Restore the redundant node-filesystem-adapter-remove test | current main already carries the a63ffc0 filesystem adapter behavior this follow-up must preserve. Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep Node and Bun CI gates fail-loud; do not weaken these checks without re-running the clean-state runtime suites. Tested: deno task lint:ci Tested: deno task typecheck Tested: deno fmt --check (4980 files) Tested: git diff --check Tested: Deno 4246 pass / 32114 steps / 1 ignored Tested: Node clean-state 4031 pass Tested: Bun 6 contracts plus 1297 files Related: #3526
deno task test:nodefails on 284 test files onmain. This fixes three of the four causes, taking it to 101 on the same machine and command.Cannot find package '@std/yaml'ERR_PACKAGE_IMPORT_NOT_DEFINEDERR_PACKAGE_PATH_NOT_EXPORTED@veryfront/react-*-upstreamDeno is unaffected: 3,781 passed, 0 failed.
YAML — third-party parser in an extension, contract in core
jsr:@std/yamlresolves under Deno alone. The obvious fix —npm:yamlin the root import map — works and is wrong:scripts/lint/audit-core-deps.ts:25exemptsjsr:@std/*and nothing else, so core may depend on Deno stdlib but never on a third party. Three lints enforce that, each with its own test.So the sanctioned split is used instead:
npm:yaml@2.9.0lives only inextensions/ext-yaml, where npm deps already live (jose,esbuild,es-module-lexer)YamlParserProvidercontract sits in coresrc/platform/compat/std/yaml.tsresolves the contract and delegates, exactly asplatform/compat/opaque-deps.tsalready does forDocumentExtractorAll five call sites move onto it — including
extensions/ext-yaml/src/adapter.ts, whose{allowDuplicateKeys: false, schema: "json"}semantics are preserved.Deliberate behaviour differences, each pinned by a test and documented in the extension README: the YAML 1.2 core schema drops 1.1 timestamps,
<<merge keys, and1_000underscore separators. The Skill-document path is unchanged, becauseschema: "json"already suppressed those.Workspace member imports
Deno applies a member's own
importsto modules inside that member's directory; the Node resolver read only the root map, soreact/'s aliases escaped to a real package lookup. Scopes are now derived from the member configs the loader already parses, deepest match wins — no hand-maintained table.#std/testing/timeand unexported subpathsThe specifier had no local shim, so Node reported the
#import undefined. Adds a cross-runtimeFakeTimeplus the missing export paths.A ratchet so the class cannot return
lint:cross-runtime-jsrfails on ajsr:mapping neither alternate harness can substitute a local file for, and on new dependents of the ones already baselined (5 today). Wired intolint:ci.Gates
fmt --check,lint,lint:core-deps,lint:dependency-boundaries,lint:ci,typecheck— all exit 0, re-run after a rebase onto current main. Core still carries no third-party runtime dependency.Why
test:nodestill cannot pass from a clean checkoutNot because of anything unfixed here.
@veryfront/react-*-upstreamare dnt build artifacts produced byscripts/build/npm-react-shims.ts; they exist only afterdeno task build:npmand are absent from any source checkout.tests/ensure-npm-links.mjs:50returns silently whennpm/node_modulesis missing — "Best effort; tests can still rely on existing node_modules resolution." — so the dependency is undeclared and fails open.That accounts for the remaining ~79 files and must be settled before either runtime is worth gating in CI: today a Node job would be green or red depending on whether a build artifact happened to be lying around.
Two further caveats, stated rather than buried:
deno task test:nodedoes not self-terminate — measured on pristinemaintoo, so pre-existing and not introduced here.Closes part of veryfront-issue-inbox#433.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests