fix(cross-runtime): close post-merge runtime review gaps - #3543
Merged
Conversation
Code review exposed empty Bun selection, conservative lock ownership, symlink-race/cause handling, parser edge cases, poisoned iterators, and Bun's unenforced worker resourceLimits. The changes keep gates fail-closed while retaining Node/Deno hosted-worker coverage. Constraint: Bun 1.3.6 does not enforce node:worker_threads resourceLimits. Constraint: Missing or malformed workspace lock ownership is indeterminate and must be preserved. Rejected: Force-exit Node tests | hides leaked handles. Rejected: Reclaim every markerless Bun lock | races live lock initialization. Confidence: high Scope-risk: moderate Directive: Keep Node and Bun gates dependent on build:npm and fail closed when no tests are selected. Tested: deno task test:node (full pass, natural exit 0; dot reporter did not print an exact count). Tested: deno task test:bun (1297 files, 0 failed). Tested: deno task test:unit (3786 files, 28105 steps, 0 failed, 1 ignored). Tested: deno task test:integration --no-lock (283 files, 2726 steps, 0 failed after a single transient external esm.sh AbortError was confirmed by focused rerun). Tested: Linux Deno 2.7.7 deno task lint:ci. Tested: deno task typecheck. Tested: deno task audit. Tested: deno fmt --check. Tested: Focused Node, Bun, and Deno tests plus mutation checks for every new regression.
📝 WalkthroughWalkthroughThe PR adds Bun-specific worker memory-limit handling, updates Bun test infrastructure and workspace locking, renames the directory-link helper, and expands regression coverage for runtime adapters, request replay, cache normalization, imports, and YAML documentation. ChangesWorker memory-limit handling
Bun test infrastructure
Runtime regression coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Evaluator
participant WorkerRunner
participant Bun
Evaluator->>WorkerRunner: evaluate configuration payload
WorkerRunner->>Bun: detect runtime and worker capability
Bun-->>WorkerRunner: bounded memory limit unavailable
WorkerRunner-->>Evaluator: reject worker-memory-limit-unavailable
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 |
Contributor
Author
|
@coderabbitai review |
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tests/bun/preload.ts`:
- Around line 146-159: Add focused regression coverage for the preload
file-rewrite handler around the `posixPath` normalization and
extension/test-path filter, exercising both `/` and `\` separators for extension
source paths and test files. Verify both path styles trigger the expected
processing and preserve module contents.
In `@tests/bun/runner-args.test.mjs`:
- Around line 50-57: Update the spawnSync invocation in the “the Bun runner
fails loudly when filters select no files” test to import and use
fileURLToPath() when converting the run-tests.mjs file URL, replacing direct
URL.pathname usage while preserving the existing test arguments and behavior.
In `@tests/bun/workspace-packages.mjs`:
- Around line 51-71: The reclaimStalePreparationLock flow can delete a newly
recreated lock during concurrent stale-lock handling. Add an exclusive reclaimer
guard around the stale-lock replacement and new-marker write, revalidate
ownership/staleness after acquiring it, and release it reliably; extend the
existing tests with a two-process race regression confirming a live replacement
lock is not removed.
🪄 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: 0afa3205-3773-49dc-8e2c-538201fe36f0
📒 Files selected for processing (18)
extensions/ext-yaml/src/adapter.tssrc/config/declarative-evaluator-worker-protocol.tssrc/config/declarative-evaluator-worker-runner.test.tssrc/config/declarative-evaluator-worker-runner.tssrc/config/declarative-evaluator.tssrc/extensions/first-party-import.test.tssrc/modules/server/classify.test.tssrc/platform/adapters/runtime/node/http-server.test.tssrc/proxy/retry.test.tssrc/server/shared/renderer/adapter.test.tssrc/transforms/esm/http-cache-helpers.test.tstests/bun/preload.tstests/bun/run-tests.mjstests/bun/runner-args.test.mjstests/bun/workspace-packages.mjstests/bun/workspace-packages.test.mjstests/ensure-npm-links.mjstests/ensure-npm-links.test.mjs
Merged
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.
Summary
Verification
Context
PR #3537 merged before its final review-fix commit reached the branch. This PR contains only that post-merge delta, rebased onto current main.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation