fix(cross-runtime): keep Node and Bun gates fail-loud - #3537
Conversation
|
Warning Review limit reached
Next review available in: 4 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 (11)
📝 WalkthroughWalkthroughThe pull request adds Node and Bun CI jobs, expands cross-runtime test infrastructure, updates import and workspace resolution, and adjusts runtime adapters and test harnesses. It also applies focused fixes for media types, URL normalization, server lifecycle handling, schema safety, and compatibility behavior. ChangesCross-runtime CI and resolution
Portable runtime behavior
Test harness updates
Documentation and assertions
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant DenoTask
participant BunRunner
participant WorkspacePackages
participant BunTestFile
CI->>DenoTask: run test:bun
DenoTask->>WorkspacePackages: prepare generated workspace packages
DenoTask->>BunRunner: select and schedule test files
BunRunner->>BunTestFile: start isolated Bun process
BunTestFile-->>BunRunner: return test result
BunRunner->>WorkspacePackages: run cleanup
BunRunner-->>CI: report aggregate status
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 142415cbf5
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/declarative-evaluator-worker-runner.ts (1)
528-533: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not rely on
resourceLimitsunder Bun 1.3.6. Bun honorsenv: {}, butresourceLimitsis not enforced. Use a runtime path that enforces the memory bound, or reject Bun for this evaluator. Add a Bun-specific regression test.🤖 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/config/declarative-evaluator-worker-runner.ts` around lines 528 - 533, Update createRuntimeWorkerEndpoint so the Bun path does not use createNodeWorkerEndpoint with unenforced resourceLimits; either route Bun through a runtime endpoint that enforces the evaluator memory bound or explicitly reject Bun. Add a Bun-specific regression test verifying the selected behavior and preserving the existing Deno and Node paths.
🧹 Nitpick comments (6)
tests/bun/workspace-packages.test.mjs (1)
29-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReconsider the hardcoded workspace count threshold.
assert.ok(publishedWorkspaces.length > 25)couples this test to the current number of published workspaces. A legitimate consolidation of extensions breaks the test with a message that does not name the real cause. The loop below already proves that every published workspace gets a generated package. Assert that the list is non-empty, or compare it against the generated package set instead of a magic number.🤖 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 `@tests/bun/workspace-packages.test.mjs` around lines 29 - 33, Replace the hardcoded publishedWorkspaces.length > 25 assertion with a non-empty check or a comparison against the generated package set. Keep the loop’s per-workspace generation coverage unchanged and avoid coupling the test to a fixed workspace count.tests/bun/preload.ts (1)
145-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the path separator handling consistent in the extension branch.
Line 146 matches
/extensions/ext-<name>/src/with forward slashes. Line 149 tests for${sep}extensions${sep}. On Windowsargs.pathuses backslashes, so the filter never matches extension sources and theseptest becomes unreachable.tests/bun/workspace-packages.mjshandleswin32junctions, so the harness targets Windows too. Normalize the path once and use the same form in both places.♻️ Proposed normalization
(args) => { + const posixPath = args.path.split(sep).join("/"); const source = readFileSync(args.path, "utf8"); - let contents = args.path.includes(`${sep}extensions${sep}`) + let contents = posixPath.includes("/extensions/") ? rewriteModuleSpecifiers( source, (specifier) => workspaceModuleSpecifier(args.path, specifier), ) ?? source : source;Also relax the filter so it accepts both separators.
🤖 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 `@tests/bun/preload.ts` around lines 145 - 154, Normalize args.path to a consistent separator form before matching, then use that normalized path for both the build.onLoad filter and the extension-branch check. Relax the filter to accept either slash style while preserving the existing extension-source pattern and rewriteModuleSpecifiers behavior.tests/bun/workspace-packages.mjs (1)
148-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the exported directory link helper.
tests/ensure-npm-links.mjsexportsresolveDirectoryLinkType(platform), which returns the same"junction"or"dir"value. Import it here instead of repeating the platform test. One helper keeps both link paths in agreement.♻️ Proposed reuse
symlinkSync( sourceRoot, join(packageRoot, "source"), - process.platform === "win32" ? "junction" : "dir", + resolveDirectoryLinkType(), );Add the import at the top of the file:
+import { resolveDirectoryLinkType } from "../ensure-npm-links.mjs";🤖 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 `@tests/bun/workspace-packages.mjs` around lines 148 - 152, Update the symlinkSync call in the workspace package setup to use the exported resolveDirectoryLinkType helper from tests/ensure-npm-links.mjs instead of repeating the process.platform conditional, adding the required import and passing the current platform to preserve the existing junction/dir behavior.src/platform/adapters/runtime/node/http-server.ts (1)
583-585: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd boundary tests for port validation.
The implementation now rejects non-integer ports and values outside
0through65_535. Add focused tests for the lower boundary, upper boundary, negative values, values above65_535, and fractional values.As per coding guidelines, behavior changes need a focused failing test before implementation.
🤖 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/adapters/runtime/node/http-server.ts` around lines 583 - 585, Add focused tests for the Node server port validation covering accepted boundaries 0 and 65,535, plus rejection of negative values, values above 65,535, and fractional ports. Place them with the existing runtime HTTP server tests and assert the invalid cases throw RangeError.Source: Coding guidelines
src/platform/compat/std/testing/time.ts (1)
226-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused test for callable
Date().The
applytrap changes the result ofDate()to the fake clock time. Add a test that installsFakeTime, callsDate(), and compares the result with the expectedDatestring.As per coding guidelines, behavior changes need a focused failing test before implementation.
🤖 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 226 - 228, Add a focused test for the callable-date behavior handled by the apply trap: install FakeTime with a controlled clock, invoke Date() without new, and assert the returned string matches the expected Date string for that fake time. Place it with the existing FakeTime tests and ensure it fails before the apply implementation change.Source: Coding guidelines
src/proxy/retry.ts (1)
203-209: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for sequential tee replay.
The body replay implementation changed from repeated
Request.clone()calls to chainedReadableStream.tee()calls. Add a focused test that consumes each returned body in attempt order and verifies identical bytes for a multi-chunk body with more than one retry.As per coding guidelines, behavior changes need a focused failing test before implementation.
🤖 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/proxy/retry.ts` around lines 203 - 209, Add a focused regression test for the body replay logic around the sequential tee construction, using a multi-chunk request body and more than one retry. Consume each body in attempt order and assert that every replay produces identical bytes, ensuring the test fails before the chained ReadableStream.tee implementation is applied.Source: Coding guidelines
🤖 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`:
- Line 14: Update the explanatory comment near the parser-options description to
remove the duplicated “so” conjunction, leaving a grammatical, concise sentence
in direct public language.
In `@src/extensions/first-party-import.ts`:
- Around line 296-315: Add focused tests for the Bun missing-module parsing
branch covering relative specifiers, package specifiers, and object-shaped
errors whose message property is a string. Exercise the parser through its
existing public or nearest visible entry point and verify the expected
normalized result for each case before finalizing the implementation.
In `@src/modules/server/classify.ts`:
- Around line 36-45: Add focused tests in classify.test.ts for versioned
cross-project paths handled by normalizeCrossProjectVersionOperators, covering
both lowercase %5e and uppercase %5E before "/@/". Assert each case normalizes
to the expected caret-prefixed version path.
In `@src/transforms/esm/http-cache-helpers.ts`:
- Around line 189-201: Update parseURLSearchParams to avoid for...of over the
stringSplit result: create an empty IntrinsicURLSearchParams, iterate the parts
by numeric index, and append each decoded name/value pair through a captured
URLSearchParams.prototype.append intrinsic so duplicate keys remain preserved.
Add a focused poisoning test that replaces Array.prototype[Symbol.iterator] and
verifies query parsing and normalized cache identity are unchanged.
In `@tests/bun/run-tests.mjs`:
- Around line 120-142: Update runIsolatedTests to treat an empty runs/files
selection as failure before creating workers or reporting success. Return false
(and preserve the existing failure reporting style) when no test files are
available, while keeping normal worker execution unchanged for non-empty
selections.
In `@tests/bun/workspace-packages.mjs`:
- Around line 21-45: Update acquirePreparationLock to recover stale lock
directories when mkdirSync reports EEXIST: inspect the existing lock marker,
reclaim it if the marker is missing/invalid or its recorded process is confirmed
dead, then retry lock acquisition; continue throwing the active-lock error for a
live or indeterminate owner. Add the necessary marker-reading helper near
acquirePreparationLock and preserve cleanup of partially written locks.
In `@tests/ensure-npm-links.mjs`:
- Around line 9-16: Update ensureDirectorySymlink to ignore only EEXIST from
symlinkSync so concurrent link creation remains successful, while preserving the
original error for all other failures. Also capture the caught error and attach
it as the cause in the readdirSync catch blocks at the referenced locations,
without changing their existing error messages.
---
Outside diff comments:
In `@src/config/declarative-evaluator-worker-runner.ts`:
- Around line 528-533: Update createRuntimeWorkerEndpoint so the Bun path does
not use createNodeWorkerEndpoint with unenforced resourceLimits; either route
Bun through a runtime endpoint that enforces the evaluator memory bound or
explicitly reject Bun. Add a Bun-specific regression test verifying the selected
behavior and preserving the existing Deno and Node paths.
---
Nitpick comments:
In `@src/platform/adapters/runtime/node/http-server.ts`:
- Around line 583-585: Add focused tests for the Node server port validation
covering accepted boundaries 0 and 65,535, plus rejection of negative values,
values above 65,535, and fractional ports. Place them with the existing runtime
HTTP server tests and assert the invalid cases throw RangeError.
In `@src/platform/compat/std/testing/time.ts`:
- Around line 226-228: Add a focused test for the callable-date behavior handled
by the apply trap: install FakeTime with a controlled clock, invoke Date()
without new, and assert the returned string matches the expected Date string for
that fake time. Place it with the existing FakeTime tests and ensure it fails
before the apply implementation change.
In `@src/proxy/retry.ts`:
- Around line 203-209: Add a focused regression test for the body replay logic
around the sequential tee construction, using a multi-chunk request body and
more than one retry. Consume each body in attempt order and assert that every
replay produces identical bytes, ensuring the test fails before the chained
ReadableStream.tee implementation is applied.
In `@tests/bun/preload.ts`:
- Around line 145-154: Normalize args.path to a consistent separator form before
matching, then use that normalized path for both the build.onLoad filter and the
extension-branch check. Relax the filter to accept either slash style while
preserving the existing extension-source pattern and rewriteModuleSpecifiers
behavior.
In `@tests/bun/workspace-packages.mjs`:
- Around line 148-152: Update the symlinkSync call in the workspace package
setup to use the exported resolveDirectoryLinkType helper from
tests/ensure-npm-links.mjs instead of repeating the process.platform
conditional, adding the required import and passing the current platform to
preserve the existing junction/dir behavior.
In `@tests/bun/workspace-packages.test.mjs`:
- Around line 29-33: Replace the hardcoded publishedWorkspaces.length > 25
assertion with a non-empty check or a comparison against the generated package
set. Keep the loop’s per-workspace generation coverage unchanged and avoid
coupling the test to a fixed workspace count.
🪄 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: 777de500-1059-487a-9980-3c0b35e5eaa3
📒 Files selected for processing (94)
.github/workflows/cicd.ymldeno.jsondocs/api-reference/veryfront/embedding.mddocs/api-reference/veryfront/eval.mddocs/api-reference/veryfront/extensions.mddocs/api-reference/veryfront/schemas.mddocs/api-reference/veryfront/workflow.mdextensions/ext-blob-gcs/src/gcs-storage.test.tsextensions/ext-blob-s3/src/s3-storage.test.tsextensions/ext-bundler-esbuild/src/binary.test.tsextensions/ext-bundler-esbuild/src/binary.tsextensions/ext-node-websocket-ws/src/package-boundary.test.tsextensions/ext-yaml/README.mdextensions/ext-yaml/src/adapter.tsscripts/lint/audit-cross-runtime-jsr.test.tsscripts/lint/audit-cross-runtime-jsr.tssrc/agent/hosted/veryfront-cloud-agent-service.test.tssrc/agent/streaming/fork-runtime-stream.test.tssrc/build/asset-pipeline/css-optimizer/optimization-engine.tssrc/build/production-build/static-generation.test.tssrc/chat/upload-handler.test.tssrc/chat/upload-handler.tssrc/client/spa/ClientApp.reactivity.test.tsxsrc/client/spa/component-loader.test.tssrc/config/declarative-evaluator-worker-runner.test.tssrc/config/declarative-evaluator-worker-runner.tssrc/config/tsconfig-paths-parity.test.tssrc/config/tsconfig-paths.tssrc/embedding/upload-handler.test.tssrc/embedding/upload-handler.tssrc/eval/runner.tssrc/extensions/first-party-import.tssrc/extensions/parser/index.tssrc/middleware/core/pipeline/pipeline.test.tssrc/modules/import-map/loader-primordial-poisoning.worker.tssrc/modules/import-map/loader.test.tssrc/modules/import-map/preloader-primordial-poisoning.worker.tssrc/modules/import-map/preloader.test.tssrc/modules/server/classify.test.tssrc/modules/server/classify.tssrc/observability/auto-instrument.test.tssrc/observability/telemetry-error.test.tssrc/observability/telemetry-error.tssrc/platform/adapters/fs/wrapper.test.tssrc/platform/adapters/runtime/bun/http-server.test.tssrc/platform/adapters/runtime/bun/websocket-adapter.test.tssrc/platform/adapters/runtime/bun/websocket-adapter.tssrc/platform/adapters/runtime/node/http-server.test.tssrc/platform/adapters/runtime/node/http-server.tssrc/platform/compat/http/pinned-fetch.test.tssrc/platform/compat/std/testing/time.test.tssrc/platform/compat/std/testing/time.tssrc/platform/compat/std/yaml.tssrc/provider/veryfront-cloud/provider.test.tssrc/proxy/retry.tssrc/proxy/routing-invalidation.test.tssrc/react/compat/ssr-adapter/string-renderer.test.tssrc/react/compat/ssr-adapter/test-setup.tssrc/react/components/ui/select.test.tsxsrc/react/runtime/head-client.test.tsxsrc/rendering/client/router.test.tssrc/rendering/layouts/utils/applicator.test.tssrc/rendering/rsc/server-renderer/rsc-renderer.test.tssrc/rendering/rsc/server-renderer/tree-processor.test.tssrc/runtime/runtime-bridge.test.tssrc/sandbox/sandbox.test.tssrc/schemas/primitives.tssrc/security/input-validation/parsers.test.tssrc/server/handlers/dev/dashboard/index.test.tssrc/server/handlers/dev/projects/method-policy.test.tssrc/server/handlers/request/api/api-handler-wrapper.test.tssrc/server/service-server.test.tssrc/skill/operation-budget.tssrc/transforms/esm/http-cache-helpers.tssrc/transforms/pipeline/cache-identity.test.tssrc/utils/hash-utils.test.tssrc/utils/response-body.test.tssrc/workflow/claude-code/websocket-publisher.test.tssrc/workflow/claude-code/websocket-publisher.tstests/bun/dynamic-alias-resolution.test.tstests/bun/npm-protocol-imports.tstests/bun/npm-protocol-resolution.test.tstests/bun/preload.tstests/bun/run-tests.mjstests/bun/runner-args.mjstests/bun/runner-args.test.mjstests/bun/workspace-packages.mjstests/bun/workspace-packages.test.mjstests/bun/workspace-resolution.test.tstests/ensure-npm-links.mjstests/ensure-npm-links.test.mjstests/node-resolver-workspace-imports.test.tstests/node/resolver-hooks.mjstsconfig.json
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
8d6b811 to
9657771
Compare
Summary
Follows #3526 with the remaining verified cross-runtime gate hardening and residue cleanup.
Verification
Local verification completed before this follow-up handoff:
deno task lint:cideno task typecheckdeno fmt --checkgit diff --checkPush hook verification on this branch also passed:
Summary by CodeRabbit
Bug Fixes
New Features
Documentation